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

From: Jean Delvare <jdelvare@suse.de>
To: Terry Bowman <Terry.Bowman@amd.com>
Cc: linux@roeck-us.net, linux-watchdog@vger.kernel.org,
	linux-i2c@vger.kernel.org, wsa@kernel.org,
	andy.shevchenko@gmail.com, rafael.j.wysocki@intel.com,
	linux-kernel@vger.kernel.org, wim@linux-watchdog.org,
	rrichter@amd.com, thomas.lendacky@amd.com,
	sudheesh.mavila@amd.com, Nehal-bakulchandra.Shah@amd.com,
	Basavaraj.Natikar@amd.com, Shyam-sundar.S-k@amd.com,
	Mario.Limonciello@amd.com
Subject: Re: [PATCH v4 0/9] i2c: piix4: Replace cd6h/cd7h port I/O accesses with MMIO accesses
Date: Tue, 8 Feb 2022 22:46:53 +0100	[thread overview]
Message-ID: <20220208224653.6a62ba22@endymion.delvare> (raw)
In-Reply-To: <4ae57999-0f23-7578-008d-2009bc36d46b@amd.com>

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?

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?

-- 
Jean Delvare
SUSE L3 Support