Message-ID: <13790724.uLZWGnKmhe@workhorse>
Date: Mon, 02 Jun 2025 14:15:45 +0200
From: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
To: Linus Walleij <linus.walleij@...aro.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 Uwe Kleine-König <ukleinek@...nel.org>,
 William Breathitt Gray <wbg@...nel.org>,
 Sebastian Reichel <sebastian.reichel@...labora.com>,
 Kever Yang <kever.yang@...k-chips.com>,
 Heiko Stübner <heiko@...ech.de>
Cc: linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org,
 linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org,
 linux-iio@...r.kernel.org, kernel@...labora.com,
 Jonas Karlman <jonas@...boo.se>,
 Detlev Casanova <detlev.casanova@...labora.com>
Subject: Re: [PATCH 4/7] soc: rockchip: add mfpwm driver

On Saturday, 31 May 2025 23:48:29 Central European Summer Time Heiko Stübner wrote:
> Am Dienstag, 8. April 2025, 14:32:16 Mitteleuropäische Sommerzeit schrieb Nicolas Frattaroli:
> > With the Rockchip RK3576, the PWM IP used by Rockchip has changed
> > substantially. Looking at both the downstream pwm-rockchip driver as
> > well as the mainline pwm-rockchip driver made it clear that with all its
> > additional features and its differences from previous IP revisions, it
> > is best supported in a new driver.
> > 
> > This brings us to the question as to what such a new driver should be.
> > To me, it soon became clear that it should actually be several new
> > drivers, most prominently when Uwe Kleine-König let me know that I
> > should not implement the pwm subsystem's capture callback, but instead
> > write a counter driver for this functionality.
> > 
> > Combined with the other as-of-yet unimplemented functionality of this
> > new IP, it became apparent that it needs to be spread across several
> > subsystems.
> > 
> > For this reason, we add a new platform bus based driver, called mfpwm
> > (short for "Multi-function PWM"). This "parent" driver makes sure that
> > only one device function driver is using the device at a time, and is in
> > charge of registering the platform bus devices for the individual device
> > functions offered by the device.
> > 
> > An acquire/release pattern is used to guarantee that device function
> > drivers don't step on each other's toes.
> > 
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
> 
> actually trying to compile this, led me to
> 
> aarch64-linux-gnu-ld: drivers/soc/rockchip/mfpwm.o: in function `mfpwm_reg_read':
> /home/devel/hstuebner/00_git-repos/linux-rockchip/_build-arm64/../include/soc/rockchip/mfpwm.h:423: multiple definition of `mfpwm_reg_read'; drivers/pwm/pwm-rockchip-v4.o:/home/devel/hstuebner/00_git-repos/linux-rockchip/_build-arm64/../include/soc/rockchip/mfpwm.h:423: first defined here
> aarch64-linux-gnu-ld: drivers/soc/rockchip/mfpwm.o: in function `mfpwm_reg_write':
> /home/devel/hstuebner/00_git-repos/linux-rockchip/_build-arm64/../include/soc/rockchip/mfpwm.h:428: multiple definition of `mfpwm_reg_write'; drivers/pwm/pwm-rockchip-v4.o:/home/devel/hstuebner/00_git-repos/linux-rockchip/_build-arm64/../include/soc/rockchip/mfpwm.h:428: first defined here
> make[3]: *** [../scripts/Makefile.vmlinux_o:72: vmlinux.o] Fehler 1
> 
> 
> during the linking stage - with the driver as builtin
> 
> 
> > +inline u32 mfpwm_reg_read(void __iomem *base, u32 reg)
> > +{
> > +	return readl(base + reg);
> > +}
> > +
> > +inline void mfpwm_reg_write(void __iomem *base, u32 reg, u32 val)
> > +{
> > +	writel(val, base + reg);
> > +}
> 
> making that a "static inline ..." solves that.

Ack, will change

> 
> 
> On a more general note, what is the differentiation to an MFD here?
> 
> Like you can already bind dt-nodes to MFD subdevices, and can implement
> the exclusivity API thing on top of a general mfd device, to make sure only
> one mfd-cell gets activated at one time.
> 
> Other than that, this looks like it reimplements MFDs?

What initially made me not make this an MFD was Uwe Kleine-König expressing
some doubts, which lead me to alternatives like the auxiliary bus. Reading the
auxiliary bus docs I found:

  A key requirement for utilizing the auxiliary bus is that there is no
  dependency on a physical bus, device, register accesses or regmap support.
  These individual devices split from the core cannot live on the platform
  bus as they are not physical devices that are controlled by DT/ACPI. The
  same argument applies for not using MFD in this scenario as MFD relies on
  individual function devices being physical devices.

Additionally, LWN[1] about the auxiliary bus, which I've read up on during my
ill-fated journey into that version of the driver, also goes further into why
MFD is sometimes a bad fit:

  Linux already includes a number of drivers for multi-function devices. One
  of the ways to support them is the Multi-Function Devices (MFD) subsystem.
  It handles independent devices "glued" together into one hardware block
  which may contain some shared resources. MFD allows access to device
  registers either directly, or using a common bus. In this second case, it
  conveniently multiplexes accesses on Inter-Integrated Circuit (I2C) or
  Serial Peripheral Interface (SPI) buses. As the MFD sub-devices are
  separate, MFD drivers do not share a common state.

  The devices Ertman addresses do not fit well into the MFD model. Devices
  using the auxiliary bus provide subsets of the capabilities of a single
  hardware device. They do not expose separate register sets for each
  function; thus they cannot be described by devicetrees or discovered by
  ACPI. Their drivers need to share access to the hardware. Events concerning
  all sub-functionalities (like power management) need to be properly handled
  by all drivers.

The individual function devices may be all pointing at the same physical
device here, but they're not distinct parts of the device. However, there
still *is* a physical device, which convinced me that auxiliary bus wasn't
the right one either, and the idea for just using the platform bus came
during a work meeting. If someone with experience on aux bus vs platform bus
(what this uses) vs MFD, then feel free to chime in. Unfortunately, as is the
norm, I can't seem to find much in terms of MFD documentation. Needing to know
what type of exclusion they guarantee and what type of abstractions they bring
with them that would make them more useful than my solution would need some
justification in more than just an auto-generated header listing.

I am very inclined to start pretending things that aren't documented do
not actually exist in the kernel, because it's very annoying to have to
constantly deal with this.

> 
> Also handing around a regmap might be nicer, compared to readl/writel.

Strong disagree, adding error handling around every single register read
and write, and needing to always read into a variable rather than getting
the read value as a return value, made the drivers a lot uglier in a
previous iteration of this.

> 
> 
> Heiko
> 

Kind regards,
Nicolas Frattaroli

[1]: https://lwn.net/Articles/840416/

Powered by blists - more mailing lists