Re: [PATCH] i2c: piix4: Replace piix4_smbus driver's cd6h/cd7h port io accesses with mmio accesses

Re: [PATCH] i2c: piix4: Replace piix4_smbus driver's cd6h/cd7h port io accesses with mmio accesses

[Terry Bowman]

Hi Jean and Guenter,

This is a gentle reminder to review my previous response when possible. Thanks!

Regards,
Terry

On 12/13/21 11:48 AM, Terry Bowman wrote:
> Hi Jean and Guenter,
> 
> Jean, Thanks for your responses. I added comments below.
> 
> I added Guenter to this email because his input is needed for adding the same
> changes to the sp5100_tco driver. The sp5100_tco and piix4_smbus driver
> must use the same synchronization logic for the shared register.
> 
> On 11/5/21 11:05, Jean Delvare wrote:
>> On Tue, 7 Sep 2021 18:37:20 +0200, Jean Delvare wrote:
>>> More generally, I am worried about the overall design. The driver
>>> originally used per-access I/O port requesting because keeping the I/O
>>> ports busy all the time would prevent other drivers from working. Do we
>>> still need to do the same with the new code? If it is possible and safe
>>> to have a permanent mapping to the memory ports, that would be a lot
>>> faster.
>>>
> 
> Permanent mapping would likely improve performance but will not provide the
> needed synchronization. As you mentioned below the sp5100 driver only uses
> the DECODEEN register during initialization but the access must be
> synchronized or an i2c transaction or sp5100_tco timer enable access may be
> lost. I considered alternatives but most lead to driver coupling or considerable
> complexity.
> 
>>> On the other hand, the read-modify-write cycle in
>>> piix4_setup_sb800_smba() is unsafe if 2 drivers can actually call
>>> request_mem_region() on the same memory area successfully.
>>>
>>> I'm not opposed to taking your patch with minimal changes (as long as
>>> the code is safe) and working on performance improvements later.
>>
> 
> I confirmed through testing the request_mem_region() and request_muxed_region() 
> macros provide exclusive locking. One difference between the 2 macros is the 
> flag parameter, IORESOURCE_MUXED. request_muxed_region() uses the 
> IORESOURCE_MUXED flag to retry the region lock if it's already locked. 
> request_mem_region() does not use the IORESOURCE_MUXED and as a result will 
> return -EBUSY immediately if the region is already locked.
> 
> I must clarify: the piix4_smbus v1 patch uses request_mem_region() which is not 
> correct because it doesn't retry locking an already locked region.  The driver 
> must support retrying the lock or piix4_smbus and sp5100_tco drivers may 
> potentially fail loading. I added proposed piix4_smbus v2 changes below to solve.
> 
> I propose reusing the existing request_*() framework from include/linux/ioport.h 
> and kernel/resource.c. A new helper macro will be required to provide an 
> interface to the "muxed" iomem locking functionality already present in 
> kernel/resource.c. The new macro will be similar to request_muxed_region() 
> but will instead operate on iomem. This should provide the same performance 
> while using the existing framework.
> 
> My plan is to add the following to include/linux/ioport.h in v2. This macro
> will add the interface for using "muxed" iomem support:
> #define request_mem_muxed_region(start,n,name)  __request_region(&iomem_resource, (start), (n), (name), IORESOURCE_MUXED)
> 
> The proposed changes will need review from more than one subsystem maintainer.
> The macro addition in include/linux/ioport.h would reside in a
> different maintainer's tree than this driver. The change to use the
> request_mem_muxed_region() macro will also be made to the sp5100_tco driver.
> The v2 review will include maintainers from subsystems owning piix4_smbus
> driver, sp5100_tco driver, and include/linux/ioport.h.
> 
> The details provided above are described in a piix4_smbus context but would also be 
> applied to the sp5100_tco driver for synchronizing the shared register.
> 
> Jean and Guenter, do you have concerns or changes you prefer to the proposal I 
> described above? 
> 
>> I looked some more at the code. I was thinking that maybe if the
>> registers accessed by the two drivers (i2c-piix4 and sp5100_tco) were
>> disjoint, then each driver could simply request subsets of the mapped
>> memory.
>>
>> Unfortunately, while most registers are indeed exclusively used by one
>> of the drivers, there's one register (0x00 = IsaDecode) which is used
>> by both. So this simple approach isn't possible.
>>
>> That being said, the register in question is only accessed at device
>> initialization time (on the sp5100_tco side, that's in function
>> sp5100_tco_setupdevice) and only for some devices (Embedded FCH). So
>> one approach which may work is to let the i2c-piix4 driver instantiate
>> the watchdog platform device in that case, instead of having sp5100_tco
>> instantiate its own device as is currently the case. That way, the
>> i2c-piix4 driver would request the "shared" memory area, perform the
>> initialization steps for both functions (SMBus and watchdog) and then
>> instantiate the watchdog device so that sp5100_tco gets loaded and goes
>> on with the runtime management of the watchdog device.
>>
>> If I'm not mistaken, this is what the i2c-i801 driver is already doing
>> for the watchdog device in all recent Intel chipsets. So maybe the same
>> approach can work for the i2c-piix4 driver for the AMD chipsets.
>> However I must confess that I did not try to do it nor am I familiar
>> with the sp5100_tco driver details, so maybe it's not possible for some
>> reason.
>>
>> If it's not possible then the only safe approach would be to migrate
>> i2c-piix4 and sp5100_tco to a true MFD setup with 3 separate drivers:
>> one new MFD PCI driver binding to the PCI device, providing access to
>> the registers with proper locking, and instantiating the platform
>> device, one driver for SMBus (basically i2c-piix4 converted to a
>> platform driver and relying on the MFD driver for register access) and
>> one driver for the watchdog (basically sp5100_tco converted to a
>> platform driver and relying on the MFD driver for register access).
>> That's a much larger change though, so I suppose we'd try avoid it if
>> at all possible.
>>

Re: [PATCH] i2c: piix4: Replace piix4_smbus driver's cd6h/cd7h port io accesses with mmio accesses

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 6727 bytes --]

Hi Jean and Guenter,

> This is a gentle reminder to review my previous response when possible. Thanks!

Quite some modern AMD laptops seem to suffer from slow touchpads and
this patch is part of the fix [1]. So, if you could comment on Terry's
questions, this is highly appreciated!

Thanks and all the best,

   Wolfram

[1] https://lore.kernel.org/r/CAPoEpV0ZSidL6aMXvB6LN1uS-3CUHS4ggT8RwFgmkzzCiYJ-XQ@mail.gmail.com

> 
> Regards,
> Terry
> 
> On 12/13/21 11:48 AM, Terry Bowman wrote:
> > Hi Jean and Guenter,
> > 
> > Jean, Thanks for your responses. I added comments below.
> > 
> > I added Guenter to this email because his input is needed for adding the same
> > changes to the sp5100_tco driver. The sp5100_tco and piix4_smbus driver
> > must use the same synchronization logic for the shared register.
> > 
> > On 11/5/21 11:05, Jean Delvare wrote:
> >> On Tue, 7 Sep 2021 18:37:20 +0200, Jean Delvare wrote:
> >>> More generally, I am worried about the overall design. The driver
> >>> originally used per-access I/O port requesting because keeping the I/O
> >>> ports busy all the time would prevent other drivers from working. Do we
> >>> still need to do the same with the new code? If it is possible and safe
> >>> to have a permanent mapping to the memory ports, that would be a lot
> >>> faster.
> >>>
> > 
> > Permanent mapping would likely improve performance but will not provide the
> > needed synchronization. As you mentioned below the sp5100 driver only uses
> > the DECODEEN register during initialization but the access must be
> > synchronized or an i2c transaction or sp5100_tco timer enable access may be
> > lost. I considered alternatives but most lead to driver coupling or considerable
> > complexity.
> > 
> >>> On the other hand, the read-modify-write cycle in
> >>> piix4_setup_sb800_smba() is unsafe if 2 drivers can actually call
> >>> request_mem_region() on the same memory area successfully.
> >>>
> >>> I'm not opposed to taking your patch with minimal changes (as long as
> >>> the code is safe) and working on performance improvements later.
> >>
> > 
> > I confirmed through testing the request_mem_region() and request_muxed_region() 
> > macros provide exclusive locking. One difference between the 2 macros is the 
> > flag parameter, IORESOURCE_MUXED. request_muxed_region() uses the 
> > IORESOURCE_MUXED flag to retry the region lock if it's already locked. 
> > request_mem_region() does not use the IORESOURCE_MUXED and as a result will 
> > return -EBUSY immediately if the region is already locked.
> > 
> > I must clarify: the piix4_smbus v1 patch uses request_mem_region() which is not 
> > correct because it doesn't retry locking an already locked region.  The driver 
> > must support retrying the lock or piix4_smbus and sp5100_tco drivers may 
> > potentially fail loading. I added proposed piix4_smbus v2 changes below to solve.
> > 
> > I propose reusing the existing request_*() framework from include/linux/ioport.h 
> > and kernel/resource.c. A new helper macro will be required to provide an 
> > interface to the "muxed" iomem locking functionality already present in 
> > kernel/resource.c. The new macro will be similar to request_muxed_region() 
> > but will instead operate on iomem. This should provide the same performance 
> > while using the existing framework.
> > 
> > My plan is to add the following to include/linux/ioport.h in v2. This macro
> > will add the interface for using "muxed" iomem support:
> > #define request_mem_muxed_region(start,n,name)  __request_region(&iomem_resource, (start), (n), (name), IORESOURCE_MUXED)
> > 
> > The proposed changes will need review from more than one subsystem maintainer.
> > The macro addition in include/linux/ioport.h would reside in a
> > different maintainer's tree than this driver. The change to use the
> > request_mem_muxed_region() macro will also be made to the sp5100_tco driver.
> > The v2 review will include maintainers from subsystems owning piix4_smbus
> > driver, sp5100_tco driver, and include/linux/ioport.h.
> > 
> > The details provided above are described in a piix4_smbus context but would also be 
> > applied to the sp5100_tco driver for synchronizing the shared register.
> > 
> > Jean and Guenter, do you have concerns or changes you prefer to the proposal I 
> > described above? 
> > 
> >> I looked some more at the code. I was thinking that maybe if the
> >> registers accessed by the two drivers (i2c-piix4 and sp5100_tco) were
> >> disjoint, then each driver could simply request subsets of the mapped
> >> memory.
> >>
> >> Unfortunately, while most registers are indeed exclusively used by one
> >> of the drivers, there's one register (0x00 = IsaDecode) which is used
> >> by both. So this simple approach isn't possible.
> >>
> >> That being said, the register in question is only accessed at device
> >> initialization time (on the sp5100_tco side, that's in function
> >> sp5100_tco_setupdevice) and only for some devices (Embedded FCH). So
> >> one approach which may work is to let the i2c-piix4 driver instantiate
> >> the watchdog platform device in that case, instead of having sp5100_tco
> >> instantiate its own device as is currently the case. That way, the
> >> i2c-piix4 driver would request the "shared" memory area, perform the
> >> initialization steps for both functions (SMBus and watchdog) and then
> >> instantiate the watchdog device so that sp5100_tco gets loaded and goes
> >> on with the runtime management of the watchdog device.
> >>
> >> If I'm not mistaken, this is what the i2c-i801 driver is already doing
> >> for the watchdog device in all recent Intel chipsets. So maybe the same
> >> approach can work for the i2c-piix4 driver for the AMD chipsets.
> >> However I must confess that I did not try to do it nor am I familiar
> >> with the sp5100_tco driver details, so maybe it's not possible for some
> >> reason.
> >>
> >> If it's not possible then the only safe approach would be to migrate
> >> i2c-piix4 and sp5100_tco to a true MFD setup with 3 separate drivers:
> >> one new MFD PCI driver binding to the PCI device, providing access to
> >> the registers with proper locking, and instantiating the platform
> >> device, one driver for SMBus (basically i2c-piix4 converted to a
> >> platform driver and relying on the MFD driver for register access) and
> >> one driver for the watchdog (basically sp5100_tco converted to a
> >> platform driver and relying on the MFD driver for register access).
> >> That's a much larger change though, so I suppose we'd try avoid it if
> >> at all possible.
> >>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: piix4: Replace piix4_smbus driver's cd6h/cd7h port io accesses with mmio accesses

[Guenter Roeck]

On 1/4/22 11:34 AM, Terry Bowman wrote:
> Hi Jean and Guenter,
> 
> This is a gentle reminder to review my previous response when possible. Thanks!
> 
> Regards,
> Terry
> 
> On 12/13/21 11:48 AM, Terry Bowman wrote:
>> Hi Jean and Guenter,
>>
>> Jean, Thanks for your responses. I added comments below.
>>
>> I added Guenter to this email because his input is needed for adding the same
>> changes to the sp5100_tco driver. The sp5100_tco and piix4_smbus driver
>> must use the same synchronization logic for the shared register.
>>
>> On 11/5/21 11:05, Jean Delvare wrote:
>>> On Tue, 7 Sep 2021 18:37:20 +0200, Jean Delvare wrote:
>>>> More generally, I am worried about the overall design. The driver
>>>> originally used per-access I/O port requesting because keeping the I/O
>>>> ports busy all the time would prevent other drivers from working. Do we
>>>> still need to do the same with the new code? If it is possible and safe
>>>> to have a permanent mapping to the memory ports, that would be a lot
>>>> faster.
>>>>
>>
>> Permanent mapping would likely improve performance but will not provide the
>> needed synchronization. As you mentioned below the sp5100 driver only uses
>> the DECODEEN register during initialization but the access must be
>> synchronized or an i2c transaction or sp5100_tco timer enable access may be
>> lost. I considered alternatives but most lead to driver coupling or considerable
>> complexity.
>>
>>>> On the other hand, the read-modify-write cycle in
>>>> piix4_setup_sb800_smba() is unsafe if 2 drivers can actually call
>>>> request_mem_region() on the same memory area successfully.
>>>>
>>>> I'm not opposed to taking your patch with minimal changes (as long as
>>>> the code is safe) and working on performance improvements later.
>>>
>>
>> I confirmed through testing the request_mem_region() and request_muxed_region()
>> macros provide exclusive locking. One difference between the 2 macros is the
>> flag parameter, IORESOURCE_MUXED. request_muxed_region() uses the
>> IORESOURCE_MUXED flag to retry the region lock if it's already locked.
>> request_mem_region() does not use the IORESOURCE_MUXED and as a result will
>> return -EBUSY immediately if the region is already locked.
>>
>> I must clarify: the piix4_smbus v1 patch uses request_mem_region() which is not
>> correct because it doesn't retry locking an already locked region.  The driver
>> must support retrying the lock or piix4_smbus and sp5100_tco drivers may
>> potentially fail loading. I added proposed piix4_smbus v2 changes below to solve.
>>
>> I propose reusing the existing request_*() framework from include/linux/ioport.h
>> and kernel/resource.c. A new helper macro will be required to provide an
>> interface to the "muxed" iomem locking functionality already present in
>> kernel/resource.c. The new macro will be similar to request_muxed_region()
>> but will instead operate on iomem. This should provide the same performance
>> while using the existing framework.
>>
>> My plan is to add the following to include/linux/ioport.h in v2. This macro
>> will add the interface for using "muxed" iomem support:
>> #define request_mem_muxed_region(start,n,name)  __request_region(&iomem_resource, (start), (n), (name), IORESOURCE_MUXED)
>>
>> The proposed changes will need review from more than one subsystem maintainer.
>> The macro addition in include/linux/ioport.h would reside in a
>> different maintainer's tree than this driver. The change to use the
>> request_mem_muxed_region() macro will also be made to the sp5100_tco driver.
>> The v2 review will include maintainers from subsystems owning piix4_smbus
>> driver, sp5100_tco driver, and include/linux/ioport.h.
>>
>> The details provided above are described in a piix4_smbus context but would also be
>> applied to the sp5100_tco driver for synchronizing the shared register.
>>
>> Jean and Guenter, do you have concerns or changes you prefer to the proposal I
>> described above?
>>

I think you'll need approval from someone with authority to accept the
suggested change in include/linux/ioport.h. No idea who that would be.

Guenter

>>> I looked some more at the code. I was thinking that maybe if the
>>> registers accessed by the two drivers (i2c-piix4 and sp5100_tco) were
>>> disjoint, then each driver could simply request subsets of the mapped
>>> memory.
>>>
>>> Unfortunately, while most registers are indeed exclusively used by one
>>> of the drivers, there's one register (0x00 = IsaDecode) which is used
>>> by both. So this simple approach isn't possible.
>>>
>>> That being said, the register in question is only accessed at device
>>> initialization time (on the sp5100_tco side, that's in function
>>> sp5100_tco_setupdevice) and only for some devices (Embedded FCH). So
>>> one approach which may work is to let the i2c-piix4 driver instantiate
>>> the watchdog platform device in that case, instead of having sp5100_tco
>>> instantiate its own device as is currently the case. That way, the
>>> i2c-piix4 driver would request the "shared" memory area, perform the
>>> initialization steps for both functions (SMBus and watchdog) and then
>>> instantiate the watchdog device so that sp5100_tco gets loaded and goes
>>> on with the runtime management of the watchdog device.
>>>
>>> If I'm not mistaken, this is what the i2c-i801 driver is already doing
>>> for the watchdog device in all recent Intel chipsets. So maybe the same
>>> approach can work for the i2c-piix4 driver for the AMD chipsets.
>>> However I must confess that I did not try to do it nor am I familiar
>>> with the sp5100_tco driver details, so maybe it's not possible for some
>>> reason.
>>>
>>> If it's not possible then the only safe approach would be to migrate
>>> i2c-piix4 and sp5100_tco to a true MFD setup with 3 separate drivers:
>>> one new MFD PCI driver binding to the PCI device, providing access to
>>> the registers with proper locking, and instantiating the platform
>>> device, one driver for SMBus (basically i2c-piix4 converted to a
>>> platform driver and relying on the MFD driver for register access) and
>>> one driver for the watchdog (basically sp5100_tco converted to a
>>> platform driver and relying on the MFD driver for register access).
>>> That's a much larger change though, so I suppose we'd try avoid it if
>>> at all possible.
>>>

Re: [PATCH] i2c: piix4: Replace piix4_smbus driver's cd6h/cd7h port io accesses with mmio accesses

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 440 bytes --]


> I think you'll need approval from someone with authority to accept the
> suggested change in include/linux/ioport.h. No idea who that would be.

ioport.h has no dedicated maintainer. I would modify it via my tree if
we have enough review. I'd think Guenter, me, and maybe Andy
(Shevchenko), and Rafael should be a good crowd. So, I suggest to do
your v2 and add all these people to CC. It is usually easier to talk
about existing code.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: piix4: Replace piix4_smbus driver's cd6h/cd7h port io accesses with mmio accesses

[Andy Shevchenko]

On Mon, Jan 10, 2022 at 6:25 AM Wolfram Sang <wsa@kernel.org> wrote:
>
> > I think you'll need approval from someone with authority to accept the
> > suggested change in include/linux/ioport.h. No idea who that would be.
>
> ioport.h has no dedicated maintainer. I would modify it via my tree if
> we have enough review. I'd think Guenter, me, and maybe Andy
> (Shevchenko), and Rafael should be a good crowd. So, I suggest to do
> your v2 and add all these people to CC. It is usually easier to talk
> about existing code.

I have briefly read the discussion by the link you provided above in
this thread. I'm not sure I understand the issue and if Intel hardware
is affected. Is there any summary of the problem?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] i2c: piix4: Replace piix4_smbus driver's cd6h/cd7h port io accesses with mmio accesses

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 452 bytes --]


> I have briefly read the discussion by the link you provided above in
> this thread. I'm not sure I understand the issue and if Intel hardware
> is affected. Is there any summary of the problem?

I guess the original patch description should explain it. You can find
it here:

http://patchwork.ozlabs.org/project/linux-i2c/patch/20210715221828.244536-1-Terry.Bowman@amd.com/

If this is not sufficient, hopefully Terry can provide more information?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: piix4: Replace piix4_smbus driver's cd6h/cd7h port io accesses with mmio accesses

[Terry Bowman]

+Robert Richter

Hi Andy,

The cd6h/cd7h port I/O can be disabled on recent AMD processors and these
changes replace the cd6h/cd7h port I/O accesses with with MMIO accesses. 
I can provide more details or answer questions.

Regards,
Terry

On 1/11/22 6:39 AM, Wolfram Sang wrote:
> 
>> I have briefly read the discussion by the link you provided above in
>> this thread. I'm not sure I understand the issue and if Intel hardware
>> is affected. Is there any summary of the problem?
> 
> I guess the original patch description should explain it. You can find
> it here:
> 
> http://patchwork.ozlabs.org/project/linux-i2c/patch/20210715221828.244536-1-Terry.Bowman@amd.com/
> 
> If this is not sufficient, hopefully Terry can provide more information?
> 

Re: [PATCH] i2c: piix4: Replace piix4_smbus driver's cd6h/cd7h port io accesses with mmio accesses

[Andy Shevchenko]

On Tue, Jan 11, 2022 at 4:13 PM Terry Bowman <Terry.Bowman@amd.com> wrote:
> The cd6h/cd7h port I/O can be disabled on recent AMD processors and these
> changes replace the cd6h/cd7h port I/O accesses with with MMIO accesses.
> I can provide more details or answer questions.

AFAIU the issue the list of questions looks like this (correct me, if
I'm wrong):
- some chips switched from I/O to MMIO
- the bus driver has shared resources with another (TCO) driver

Now, technically what you are trying is to find a way to keep the
original functionality on old machines and support new ones without
much trouble.

From what I see, the silver bullet may be the switch to regmap as we
have done in I2C DesignWare driver implementation.

Yes, it's a much more invasive solution, but at the same time it's
much cleaner from my p.o.v. And you may easily split it to logical
parts (prepare drivers, switch to regmap, add a new functionality).

I might be missing something and above not gonna work, please tell me
what I miss in that case.

> On 1/11/22 6:39 AM, Wolfram Sang wrote:
> >
> >> I have briefly read the discussion by the link you provided above in
> >> this thread. I'm not sure I understand the issue and if Intel hardware
> >> is affected. Is there any summary of the problem?
> >
> > I guess the original patch description should explain it. You can find
> > it here:
> >
> > http://patchwork.ozlabs.org/project/linux-i2c/patch/20210715221828.244536-1-Terry.Bowman@amd.com/
> >
> > If this is not sufficient, hopefully Terry can provide more information?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] i2c: piix4: Replace piix4_smbus driver's cd6h/cd7h port io accesses with mmio accesses

[Andy Shevchenko]

On Tue, Jan 11, 2022 at 4:53 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Jan 11, 2022 at 4:13 PM Terry Bowman <Terry.Bowman@amd.com> wrote:
> > The cd6h/cd7h port I/O can be disabled on recent AMD processors and these
> > changes replace the cd6h/cd7h port I/O accesses with with MMIO accesses.
> > I can provide more details or answer questions.
>
> AFAIU the issue the list of questions looks like this (correct me, if
> I'm wrong):
> - some chips switched from I/O to MMIO
> - the bus driver has shared resources with another (TCO) driver
>
> Now, technically what you are trying is to find a way to keep the
> original functionality on old machines and support new ones without
> much trouble.
>
> From what I see, the silver bullet may be the switch to regmap as we
> have done in I2C DesignWare driver implementation.
>
> Yes, it's a much more invasive solution, but at the same time it's
> much cleaner from my p.o.v. And you may easily split it to logical
> parts (prepare drivers, switch to regmap, add a new functionality).
>
> I might be missing something and above not gonna work, please tell me
> what I miss in that case.

On top of that I'm wondering why slow I/O is used? Do we have anything
that really needs that or is it simply a cargo-cult?

> > On 1/11/22 6:39 AM, Wolfram Sang wrote:
> > >
> > >> I have briefly read the discussion by the link you provided above in
> > >> this thread. I'm not sure I understand the issue and if Intel hardware
> > >> is affected. Is there any summary of the problem?
> > >
> > > I guess the original patch description should explain it. You can find
> > > it here:
> > >
> > > http://patchwork.ozlabs.org/project/linux-i2c/patch/20210715221828.244536-1-Terry.Bowman@amd.com/
> > >
> > > If this is not sufficient, hopefully Terry can provide more information?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] i2c: piix4: Replace piix4_smbus driver's cd6h/cd7h port io accesses with mmio accesses

[Terry Bowman]

On 1/11/22 8:54 AM, Andy Shevchenko wrote:
> On Tue, Jan 11, 2022 at 4:53 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Tue, Jan 11, 2022 at 4:13 PM Terry Bowman <Terry.Bowman@amd.com> wrote:
>>> The cd6h/cd7h port I/O can be disabled on recent AMD processors and these
>>> changes replace the cd6h/cd7h port I/O accesses with with MMIO accesses.
>>> I can provide more details or answer questions.
>>
>> AFAIU the issue the list of questions looks like this (correct me, if
>> I'm wrong):
>> - some chips switched from I/O to MMIO
>> - the bus driver has shared resources with another (TCO) driver
>>
Correct

>> Now, technically what you are trying is to find a way to keep the
>> original functionality on old machines and support new ones without
>> much trouble.
>>
>> From what I see, the silver bullet may be the switch to regmap as we
>> have done in I2C DesignWare driver implementation.
>>
>> Yes, it's a much more invasive solution, but at the same time it's
>> much cleaner from my p.o.v. And you may easily split it to logical
>> parts (prepare drivers, switch to regmap, add a new functionality).
>>
>> I might be missing something and above not gonna work, please tell me
>> what I miss in that case.
> 
> On top of that I'm wondering why slow I/O is used? Do we have anything
> that really needs that or is it simply a cargo-cult?

The efch SMBUS & WDT previously only supported a port I/O interface 
(until recently) and thus dictated the HW access method.  

Wolfram pointed out some AMD laptops suffer from slow trackpad [1] and 
this is part of the fix. 

[1] https://lore.kernel.org/r/CAPoEpV0ZSidL6aMXvB6LN1uS-3CUHS4ggT8RwFgmkzzCiYJ-XQ@mail.gmail.com

> 
>>> On 1/11/22 6:39 AM, Wolfram Sang wrote:
>>>>
>>>>> I have briefly read the discussion by the link you provided above in
>>>>> this thread. I'm not sure I understand the issue and if Intel hardware
>>>>> is affected. Is there any summary of the problem?
>>>>
>>>> I guess the original patch description should explain it. You can find
>>>> it here:
>>>>
>>>> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatchwork.ozlabs.org%2Fproject%2Flinux-i2c%2Fpatch%2F20210715221828.244536-1-Terry.Bowman%40amd.com%2F&amp;data=04%7C01%7CTerry.Bowman%40amd.com%7C89e551e0ebe94607beaf08d9d51288f9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637775097863907004%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=gvJ0FC9MVacQunc8uMJ6oJEw0pGcisu9muQkE8u4rxY%3D&amp;reserved=0
>>>>
>>>> If this is not sufficient, hopefully Terry can provide more information?
> 

Re: [PATCH] i2c: piix4: Replace piix4_smbus driver's cd6h/cd7h port io accesses with mmio accesses

[Andy Shevchenko]

On Tue, Jan 11, 2022 at 5:50 PM Terry Bowman <Terry.Bowman@amd.com> wrote:
> On 1/11/22 8:54 AM, Andy Shevchenko wrote:
> > On Tue, Jan 11, 2022 at 4:53 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> >> On Tue, Jan 11, 2022 at 4:13 PM Terry Bowman <Terry.Bowman@amd.com> wrote:
> >>> The cd6h/cd7h port I/O can be disabled on recent AMD processors and these
> >>> changes replace the cd6h/cd7h port I/O accesses with with MMIO accesses.
> >>> I can provide more details or answer questions.
> >>
> >> AFAIU the issue the list of questions looks like this (correct me, if
> >> I'm wrong):
> >> - some chips switched from I/O to MMIO
> >> - the bus driver has shared resources with another (TCO) driver
> >>
> Correct
>
> >> Now, technically what you are trying is to find a way to keep the
> >> original functionality on old machines and support new ones without
> >> much trouble.
> >>
> >> From what I see, the silver bullet may be the switch to regmap as we
> >> have done in I2C DesignWare driver implementation.
> >>
> >> Yes, it's a much more invasive solution, but at the same time it's
> >> much cleaner from my p.o.v. And you may easily split it to logical
> >> parts (prepare drivers, switch to regmap, add a new functionality).
> >>
> >> I might be missing something and above not gonna work, please tell me
> >> what I miss in that case.

> > On top of that I'm wondering why slow I/O is used? Do we have anything
> > that really needs that or is it simply a cargo-cult?
>
> The efch SMBUS & WDT previously only supported a port I/O interface
> (until recently) and thus dictated the HW access method.

I believe you didn't get my question. Sorry for that. Elaboration below.

The code is using in*_p() and out*_p() accessors (pay attention to the _p part).

My question is about that.

> Wolfram pointed out some AMD laptops suffer from slow trackpad [1] and
> this is part of the fix.
>
> [1] https://lore.kernel.org/r/CAPoEpV0ZSidL6aMXvB6LN1uS-3CUHS4ggT8RwFgmkzzCiYJ-XQ@mail.gmail.com

I see, but still it never worked, correct?

> >>> On 1/11/22 6:39 AM, Wolfram Sang wrote:
> >>>>
> >>>>> I have briefly read the discussion by the link you provided above in
> >>>>> this thread. I'm not sure I understand the issue and if Intel hardware
> >>>>> is affected. Is there any summary of the problem?
> >>>>
> >>>> I guess the original patch description should explain it. You can find
> >>>> it here:
> >>>>
> >>>> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatchwork.ozlabs.org%2Fproject%2Flinux-i2c%2Fpatch%2F20210715221828.244536-1-Terry.Bowman%40amd.com%2F&amp;data=04%7C01%7CTerry.Bowman%40amd.com%7C89e551e0ebe94607beaf08d9d51288f9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637775097863907004%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=gvJ0FC9MVacQunc8uMJ6oJEw0pGcisu9muQkE8u4rxY%3D&amp;reserved=0
> >>>>
> >>>> If this is not sufficient, hopefully Terry can provide more information?
> >



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] i2c: piix4: Replace piix4_smbus driver's cd6h/cd7h port io accesses with mmio accesses

[Terry Bowman]

On 1/11/22 10:17, Andy Shevchenko wrote:
> On Tue, Jan 11, 2022 at 5:50 PM Terry Bowman <Terry.Bowman@amd.com> wrote:
>> On 1/11/22 8:54 AM, Andy Shevchenko wrote:
>>> On Tue, Jan 11, 2022 at 4:53 PM Andy Shevchenko
>>> <andy.shevchenko@gmail.com> wrote:
>>>> On Tue, Jan 11, 2022 at 4:13 PM Terry Bowman <Terry.Bowman@amd.com> wrote:
>>>>> The cd6h/cd7h port I/O can be disabled on recent AMD processors and these
>>>>> changes replace the cd6h/cd7h port I/O accesses with with MMIO accesses.
>>>>> I can provide more details or answer questions.
>>>>
>>>> AFAIU the issue the list of questions looks like this (correct me, if
>>>> I'm wrong):
>>>> - some chips switched from I/O to MMIO
>>>> - the bus driver has shared resources with another (TCO) driver
>>>>
>> Correct
>>
>>>> Now, technically what you are trying is to find a way to keep the
>>>> original functionality on old machines and support new ones without
>>>> much trouble.
>>>>
>>>> From what I see, the silver bullet may be the switch to regmap as we
>>>> have done in I2C DesignWare driver implementation.
>>>>
>>>> Yes, it's a much more invasive solution, but at the same time it's
>>>> much cleaner from my p.o.v. And you may easily split it to logical
>>>> parts (prepare drivers, switch to regmap, add a new functionality).
>>>>
>>>> I might be missing something and above not gonna work, please tell me
>>>> what I miss in that case.
> 
>>> On top of that I'm wondering why slow I/O is used? Do we have anything
>>> that really needs that or is it simply a cargo-cult?
>>
>> The efch SMBUS & WDT previously only supported a port I/O interface
>> (until recently) and thus dictated the HW access method.
> 
> I believe you didn't get my question. Sorry for that. Elaboration below.
> 
> The code is using in*_p() and out*_p() accessors (pay attention to the _p part).
> 
> My question is about that.
> 
>> Wolfram pointed out some AMD laptops suffer from slow trackpad [1] and
>> this is part of the fix.
>>
>> [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fr%2FCAPoEpV0ZSidL6aMXvB6LN1uS-3CUHS4ggT8RwFgmkzzCiYJ-XQ%40mail.gmail.com&amp;data=04%7C01%7CTerry.Bowman%40amd.com%7Cded2c3a486854ef44c3408d9d51e0cad%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637775147318596002%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=lKNVA1xFkS5bIxDS4%2BdCXVPKlrIOY9PV%2BW9sLtnR630%3D&amp;reserved=0
> 
> I see, but still it never worked, correct?
> 

I was not familiar with the trackpad issue until a few days ago. According 
to Miroslav's post, one of the issues is resolved but their is an interrupt 
flood still to be resolved. 

>>>>> On 1/11/22 6:39 AM, Wolfram Sang wrote:
>>>>>>
>>>>>>> I have briefly read the discussion by the link you provided above in
>>>>>>> this thread. I'm not sure I understand the issue and if Intel hardware
>>>>>>> is affected. Is there any summary of the problem?
>>>>>>
>>>>>> I guess the original patch description should explain it. You can find
>>>>>> it here:
>>>>>>
>>>>>> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatchwork.ozlabs.org%2Fproject%2Flinux-i2c%2Fpatch%2F20210715221828.244536-1-Terry.Bowman%40amd.com%2F&amp;data=04%7C01%7CTerry.Bowman%40amd.com%7Cded2c3a486854ef44c3408d9d51e0cad%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637775147318596002%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=oNfdc6ozDE57vqwnEH4n2KQfXdxcF9rAiI9R592CKv4%3D&amp;reserved=0
>>>>>>
>>>>>> If this is not sufficient, hopefully Terry can provide more information?
>>>
> 
> 
> 

Re: [PATCH] i2c: piix4: Replace piix4_smbus driver's cd6h/cd7h port io accesses with mmio accesses

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 378 bytes --]


> > On top of that I'm wondering why slow I/O is used? Do we have anything
> > that really needs that or is it simply a cargo-cult?
> 
> The efch SMBUS & WDT previously only supported a port I/O interface 
> (until recently) and thus dictated the HW access method.  

Is this enough information to start v2 of this series? Or does the
approach need more discussion?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: piix4: Replace piix4_smbus driver's cd6h/cd7h port io accesses with mmio accesses

[Andy Shevchenko]

On Thu, Jan 13, 2022 at 9:42 AM Wolfram Sang <wsa@kernel.org> wrote:
>
>
> > > On top of that I'm wondering why slow I/O is used? Do we have anything
> > > that really needs that or is it simply a cargo-cult?
> >
> > The efch SMBUS & WDT previously only supported a port I/O interface
> > (until recently) and thus dictated the HW access method.
>
> Is this enough information to start v2 of this series? Or does the
> approach need more discussion?

I dunno why slow I/O is chosen, but it only affects design (read:
ugliness) of the new code.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] i2c: piix4: Replace piix4_smbus driver's cd6h/cd7h port io accesses with mmio accesses

[Jean Delvare]

On Thu, 13 Jan 2022 12:24:41 +0200, Andy Shevchenko wrote:
> On Thu, Jan 13, 2022 at 9:42 AM Wolfram Sang <wsa@kernel.org> wrote:
> > > > On top of that I'm wondering why slow I/O is used? Do we have anything
> > > > that really needs that or is it simply a cargo-cult?  
> > >
> > > The efch SMBUS & WDT previously only supported a port I/O interface
> > > (until recently) and thus dictated the HW access method.  
> >
> > Is this enough information to start v2 of this series? Or does the
> > approach need more discussion?  
> 
> I dunno why slow I/O is chosen, but it only affects design (read:
> ugliness) of the new code.

I've been wondering about the use of slow (*_p) I/O accessors for some
time too. All the SMBus controller drivers doing that originate from the
lm_sensors project (i2c-ali1535, i2c-ali1563, i2c-ali15x3, i2c-amd756,
i2c-i801, i2c-nforce2, i2c-piix4 and i2c-viapro). So basically *all*
SMBus controller drivers for non-embedded x86.

I suspect that most of this is the result of copy-and-paste from one
driver to the next as support for different chipsets was added in the
late 90's and early 2000's. I wouldn't be surprised if most, if not
all, can be replaced with non-pausing counterparts. But I've been too
shy to give it a try so far.

I must say I find it pretty funny that Andy is asking about it in the
i2c-piix4 driver when the i2c-i801 driver, which he's been helping with
quite a lot in the last few years, does exactly the same.

-- 
Jean Delvare
SUSE L3 Support