All of
 help / color / mirror / Atom feed
From: Jean Delvare <>
To: Terry Bowman <>
Cc:,,, Guenter Roeck <>
Subject: Re: [PATCH] i2c: piix4: Replace piix4_smbus driver's cd6h/cd7h port io accesses with mmio accesses
Date: Fri, 5 Nov 2021 17:05:50 +0100	[thread overview]
Message-ID: <20211105170550.746443b9@endymion> (raw)
In-Reply-To: <20210907183720.6e0be6b6@endymion>

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.
> 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 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

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

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.

Jean Delvare
SUSE L3 Support

  reply	other threads:[~2021-11-05 16:05 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-15 22:18 [PATCH] i2c: piix4: Replace piix4_smbus driver's cd6h/cd7h port io accesses with mmio accesses Terry Bowman
2021-09-07 16:37 ` Jean Delvare
2021-11-05 16:05   ` Jean Delvare [this message]
2021-12-13 17:48     ` Terry Bowman
2022-01-04 19:34       ` Terry Bowman
2022-01-06 13:01         ` Wolfram Sang
2022-01-06 18:18         ` Guenter Roeck
2022-01-08 21:49           ` Wolfram Sang
2022-01-10 10:29             ` Andy Shevchenko
2022-01-11 12:39               ` Wolfram Sang
2022-01-11 14:13                 ` Terry Bowman
2022-01-11 14:53                   ` Andy Shevchenko
2022-01-11 14:54                     ` Andy Shevchenko
2022-01-11 15:50                       ` Terry Bowman
2022-01-11 16:17                         ` Andy Shevchenko
2022-01-12  0:40                           ` Terry Bowman
2022-01-13  7:42                         ` Wolfram Sang
2022-01-13 10:24                           ` Andy Shevchenko
2022-01-18 13:09                             ` Jean Delvare
2022-01-18 14:22       ` Jean Delvare
     [not found] <20210715221828.244536-1-Terry.Bowman () amd ! com>
2021-08-16 16:55 ` Terry Bowman
2021-08-16 17:03 ` Terry Bowman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211105170550.746443b9@endymion \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.