Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove overriding bus timing defaults #473

Merged
merged 1 commit into from
Sep 14, 2021
Merged

Remove overriding bus timing defaults #473

merged 1 commit into from
Sep 14, 2021

Conversation

williambj1
Copy link
Contributor

@williambj1 williambj1 commented Sep 10, 2021 •
edited
Loading

Hi devs!

This PR allows accepting a single set of bus *CNT values (hcnt, lcnt, and sda hold) for a certain bus frequency (Standard Mode and Fast Mode) and leaves the missing sets as default. Which will actually reflect the log that says "using defaults where necessary".

VoodooI2C/VoodooI2C/VoodooI2C/VoodooI2CController/VoodooI2CControllerDriver.cpp

Line 468 in 9ddc72a

IOLog("%s::%s Warning: Error getting bus config, using defaults where necessary\n", getName(), bus_device.name);

Previously if one set of the *CNT values for a certain frequency is missing, all of the values will be set back to default. This is problematic on platforms where the OEM only provides a set of customized values for the desired frequency and leaves others unset.

Here is an example from an ASUS laptop (Zephyrus S GX531GS):

// Line 60904
Scope (_SB.PCI0.I2C1)
{
    // Only FMCN is provided
    Method (FMCN, 0, NotSerialized)
    {
        Name (PKG, Package (0x03)
        {
            0x0101, 
            0x012C, 
            0x62
        })
        Return (PKG)
    }
    Device (ETPD)
    {
        Name (SBFB, ResourceTemplate ()
        {                                                // 0x61A80 is 400KHz, Fast Mode
            I2cSerialBusV2 (0x004C, ControllerInitiated, 0x00061A80,
                AddressingMode7Bit, "\\_SB.PCI0.I2C1",
                0x00, ResourceConsumer, _Y34, Exclusive,
                )
        })
        Name (SBFI, ResourceTemplate ()
        {
            Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive, ,, )
            {
                0x0000005F,
            }
        })
        // ...

Original DSDT for reference:
DSDT.aml.zip

BTW, I realized that the controller code is ported from the Linux DesignWare I2C driver and there is a way to read the tx and rx fifo depth. Currently the tx and rx fifo depth is hardcoded, is there any particular reason for that?

https://github.com/torvalds/linux/blob/bf9f243f23e6623f310ba03fbb14e10ec3a61290/drivers/i2c/busses/i2c-designware-common.c#L572-L598

Thanks and have a nice day.

@williambj1 williambj1 changed the title Actually use bus timing config defaults where necessary Use bus timing defaults where necessary Sep 10, 2021
@ben9923
Copy link
Member

ben9923 commented Sep 10, 2021

Thanks for this, it's indeed a welcome change.
I do wonder if the VoodooI2CControllerBusConfig defaults are enough so no setting should be done here on failure whatsoever.

BTW, I plan to support per-platform defaults in the near future, as the current defaults are Haswell era (Defined hsw_config in Linux)...
We've seen they don't usually fit CFL/CNL+ hardware, requiring custom SSDTs with SSCN/FMCN.
Correctly implemented for over 5 years in the Linux LPSS code (Used for SKL+) :P

Regarding FIFO size, I don't know. Could legacy hardware (Haswell) used hardocded values in the past?
You can look at the Linux changelog, I'll do so too later.

@williambj1
Copy link
Contributor Author

@ben9923 Thank you very much for the reply. Per-platform defaults will certainly be a great improvement.

I do wonder if the VoodooI2CControllerBusConfig defaults are enough so no setting should be done here on failure whatsoever.

I'm not sure if there are cases where the buffer returned from FMCN/SSCN is incomplete/invalid. If we don't care about it (I don't think linux does either), then it's fine to remove the assignments.

I haven't looked at the git history regarding the FIFO size, will probably do so tomorrow.

BTW, the DesignWare Driver has a way to calculate/approximate the missing *CNT values. But I did not understand the ic_clk.

@ben9923
Copy link
Member

ben9923 commented Sep 14, 2021

I'm not sure if there are cases where the buffer returned from FMCN/SSCN is incomplete/invalid. If we don't care about it (I don't think linux does either), then it's fine to remove the assignments.

Looking at the getACPIParams implementation, I think it's safe to use the defaults and not re-assign at all.

I haven't looked at the git history regarding the FIFO size, will probably do so tomorrow.

It appears to originate from the very first Lynx Point dw commit: torvalds/linux@b61b141
It was removed 1.5 years ago (So relatively new...): torvalds/linux@3f35064
Currently implemented here:
https://github.com/torvalds/linux/blob/6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f/drivers/i2c/busses/i2c-designware-common.c#L572-L598

I have no idea if any intel PCH has a FIFO size that's higher than 32, but it's simple enough to add for the chance it'll optimize some platforms. I'll try looking those values up on 400 series, etc.

BTW, the DesignWare Driver has a way to calculate/approximate the missing *CNT values. But I did not understand the ic_clk.

Yeah, that's what I was referring to. ic_clk is provided in the LPSS driver for SKL+. Haswell doesn't need it as SCL/SDA values are hardcoded (See i2c_dw_clk_rate docs).

I can do both :)

@williambj1 williambj1 changed the title Use bus timing defaults where necessary Remove overriding bus timing defaults Sep 14, 2021
@ben9923 ben9923 requested a review from kprinssu September 14, 2021 14:04
@ben9923 ben9923 closed this Sep 14, 2021
@ben9923
Copy link
Member

ben9923 commented Sep 14, 2021 •
edited
Loading

@williambj1 Accidentally messed up the remote while trying to rebase :/
Could you re-push the changes rebased with upstream? I'll be able to re-open once you do so.
It auto-closed so I can't fix it myself.

Sorry!
(I'll remove that silly requirement for a rebase/merge of master into the source branch prior to merge)

@williambj1
Use bus timing defaults where necessary
d5718c2 
Some devices only provide the set of timing values for their operating bus frequency.

Remove overriding bus timing defaults when `getBusConfig` fails.
@williambj1
Copy link
Contributor Author

@ben9923 Done :)

GitHub doesn't allow me to reopen the PR, please let me know if a new PR should be created. Thanks!

@ben9923 ben9923 reopened this Sep 14, 2021
@ben9923 ben9923 self-requested a review September 14, 2021 15:08
ben9923
ben9923 approved these changes Sep 14, 2021
View reviewed changes
Copy link
Member

@ben9923 ben9923 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

kprinssu
kprinssu approved these changes Sep 14, 2021
View reviewed changes
Copy link
Collaborator

@kprinssu kprinssu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@ben9923 ben9923 merged commit e8443d1 into VoodooI2C:master Sep 14, 2021
@zhen-zen
Copy link
Contributor

Specific to this issue, maybe it's better to check the return of validateObject first? We don't need to proceed nor report error in case of kIOReturnNotFound.

@ben9923
Copy link
Member

ben9923 commented Sep 15, 2021

@zhen-zen I believe we should treat it as an error until proper default bus config exists for SKL+, as currently default aren't good.

Once per-platform defaults feature is merged - You're right, it should not be treated as an error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ben9923 ben9923 ben9923 approved these changes

@kprinssu kprinssu kprinssu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@williambj1 @ben9923 @zhen-zen @kprinssu