Re: [PATCH v4 0/9] i2c: piix4: Replace cd6h/cd7h port I/O accesses with MMIO accesses

[Terry Bowman <Terry.Bowman@amd.com>]

Hi Jean,

On 2/8/22 15:46, Jean Delvare wrote:
> On Tue, 8 Feb 2022 13:36:55 -0600, Terry Bowman wrote:
>> On 2/8/22 11:11, Jean Delvare wrote:
>>> Unfortunately I'm not really able to test this series. While I do have
>>> an AMD-based laptop which uses the i2c-piix4 driver, the SMBus has
>>> never been usable on it. The driver creates 3 i2c buses (port 0 at
>>> 0b00, port 2 at 0b00 and port 1 at 0b20). The first 2 do not seem to
>>> have any device on them (i2cdetect returns empty). The last one must
>>> have some devices on it, I see address 0x1c answers the ping,
>>> unfortunately as soon as probing reaches address 0x2c, all later pings
>>> return success, regardless of the address. It seems that some I2C
>>> device (probably the one at 0x2c, but I don't know what it is) is
>>> holding SDA low forever, therefore preventing any use of the whole
>>> SMBus port until the next reboot.
>>
>> My understanding is the OEM may have different i2c devices because it
>> is mainboard specific.
> 
> Yes, the devices on the SMBus could be anything Lenovo decided to use.
> Tomorrow I'll try to scan the SMBus but skipping the problematic
> address, to see if it works around the problem.
> 
>>>> (...)
>>>> Changes in v4:
>>>> (...)
>>>>  - Removed iowrite32(ioread32(...), ...). Unnecessary because MMIO is
>>>>    already enabled. (Terry Bowman)  
>>>
>>> I'm curious, how can you be sure of that actually?
>>
>> The removed code was using a MMIO region to write 1 to 'mmioen'. This was 
>> using the MMIO region to enable same MMIO region.
> 
> Ah ah, I get it now. Nice chicken or the egg situation :-)
> 
>> Programmer's manual shows FCH::PM::ISACONTROL[mmioen] has a '1' reset value.
>> Per programmer's manual, FCH::PM::ISACONTROL[mmioen] enables MMIO mapping 
>> at FED8_0000h..FED8_1FFFh. The FCH::PM::ISACONTROL register is MMIO 
>> mapped at FED8_0304h. 'mmioen' was intended to be set using port I/O to 
>> enable MMIO but, port I/O access to these registers is now disabled.
> 
> Is my understanding correct that there is some overlapping of the
> access methods and there are certain chipsets where both legacy I/O and
> MMIO access is possible?
> 
Yes. 

> If so, while there's indeed nothing to be done for the most recent
> systems where only MMIO access is possible, you may still need to
> enable MMIO access through legacy I/O if you try to use MMIO on
> chipsets where both are possible. I'm not sure what exactly where you
> set the limit. In the last patch you say that 0x51 is the first
> revision of the family 17h CPUs, but is family 17h the first where MMIO
> is available, or the first where legacy I/O isn't?
> 
Family 17h, SMBus PCI ID >= 0x51 is the first where cd6h/cd7h port I/O is disabled. 
If SMBus PCI ID < 0x51 then cd6h/cd7h port I/O is used. 
  
Regards,
Terry