Re: [PATCH v3 1/1] mfd: Add I2C based System Configuaration (SYSCON) access

[Lee Jones]

On Fri, 19 Jun 2020, Chen-Yu Tsai wrote:

> On Fri, Jun 19, 2020 at 2:41 PM Lee Jones <lee.jones@xxxxxxxxxx> wrote:
> >
> > On Fri, 19 Jun 2020, Chen-Yu Tsai wrote:
> >
> > > On Thu, Jun 18, 2020 at 6:07 PM Lee Jones <lee.jones@xxxxxxxxxx> wrote:
> > > >
> > > > On Thu, 18 Jun 2020, Arnd Bergmann wrote:
> > > >
> > > > > On Thu, Jun 18, 2020 at 10:03 AM Lee Jones <lee.jones@xxxxxxxxxx> wrote:
> > > > > >
> > > > > > The existing SYSCON implementation only supports MMIO (memory mapped)
> > > > > > accesses, facilitated by Regmap.  This extends support for registers
> > > > > > held behind I2C busses.
> > > > > >
> > > > > > Signed-off-by: Lee Jones <lee.jones@xxxxxxxxxx>
> > > > >
> > > > > The implementation looks fine to me, but can you explain how this is going to
> > > > > be used, and what the advantage is over open-coding the devm_regmap_init_i2c()
> > > > > in each driver that would use this?
> > > >
> > > > Does Regmap let you register/initialise an I2C address more than once?
> > > >
> > > > When I attempt it, I get:
> > > >
> > > > [    0.522988] i2c i2c-0: Failed to register i2c client tmp105 at 0x32 (-16)
> > > > [    0.523341] i2c i2c-0: of_i2c: Failure registering /bus@4000000/motherboard/iofpga@7,00000000/i2c@16000/temp@32
> > > > [    0.523691] i2c i2c-0: Failed to create I2C device for /bus@4000000/motherboard/iofpga@7,00000000/i2c@16000/temp@32
> > > >
> > > > > Is this about using proper locking through the regmap framework for
> > > > > shared i2c clients, or to reduce memory consumption when lots of drivers
> > > > > access the same regmap?
> > > >
> > > > All of those things are valid.
> > > >
> > > > My use-case is regarding MFDs sharing an I2C interfaced address space
> > > > with their children.
> > >
> > > Is that an issue with the standard mfd + regmap pattern?
> >
> > There is no relationship between MFD and Regmap.  It is not more
> > closely related to Regmap than it is any other public API provided
> > within the kernel.  *Some* parent drivers initialise one large,
> > encompassing Regmap address space and pass it to their children, but
> > this isn't suitable in all cases.
> >
> > > For the AXP20x PMICs, we register the regmap in the parent mfd driver [1],
> > > and store that in dev_data for child drivers to fetch [2]. You could
> > > easily just fetch the regmap with dev_get_regmap() and a pointer to the
> > > parent device.
> >
> > Remember, not all use-cases are the same.  Just because your H/W fits
> > well within the current framework, doesn't mean all will.
> >
> > Initialising in the parent is no problem if the driver is meaningful
> > in other ways, but what if that's all the parent driver does?  In
> > these cases Syscon can be used instead, rendering the driver
> > superfluous. Meaning it can (and *should*) then be omitted.
> 
> I'm guessing in your use case there isn't a need for a parent driver,
> and you are looking for something like "simple-mfd", but for listing
> sub-devices within an I2C slave device? In that case I understand.

Bingo!

Actually this will be used *with* "simple-mfd".  "simple-mfd" will
ensure the sub-devices are probed and "syscon" will allow them to
share an address space.  This is currently possible for MMIO, but not
so for register maps located behind an I2C interface.

> > > > > My impression of the existing syscon code is that the main value-add over
> > > > > other ways of doing the same is the syscon_regmap_lookup_by_phandle()
> > > > > interface that gives other drivers a much simpler way of getting the
> > > > > regmap just based on the DT node. Are you planning to add something
> > > > > like that here as well? An ideal driver interface might allow
> > > > > syscon_regmap_lookup_by_phandle() to work for both mmio and i2c
> > > > > based syscons, or additional ones as well, but implementing this would
> > > > > be rather tricky when the i2c core is a loadable module.
> > >
> > > The current MMIO syscon is decoupled from the DM, and there is no way
> > > for drivers to export or register a syscon, meaning I have to open code
> > > syscon_regmap_lookup_by_phandle() [3] if I want to only expose certain
> > > registers and not the full address range, or if I want to share the
> > > regmap with the existing driver (for locking purposes), or both [4].
> >
> > Not sure I understand the problem.
> >
> > Could you explain why the current implementation doesn't work for you?
> >
> > Open coding your own implementation of Syscon is non-optimal.
> 
> For the DWMAC Ethernet controllers, the platform glue almost always has
> a register for tuning the delays of the TX and RX clocks. In almost all
> later Allwinner chips, this is in a separate area, which we use a syscon
> for. However in one hybrid chip, this is located in the clock controller.
> We deemed it risky to also have the whole clock controller address range
> mapped as a syscon, and so we export a custom regmap.
> 
> The Ethernet driver has to deal with both cases.
> 
> Looking at it again, since syscon still has a platform driver, maybe I
> should just use the dev_get_regmap() route for both cases.
> 
> > > Maybe there's room for improvement here? The same applies to the new
> > > I2C case, and likely any other future syscon variants.
> > >
> > > IMHO people are getting it wrong if they have both a syscon and a driver
> > > for the same device.
> >
> > Syscon is just a means to obtain a group of registers either a)
> > without a dedicated driver OR b) to share amongst more than 1,
> > potentially unrelated, user.  So in the case of a) which appears to
> > sit well with-in your use-case and expectations, you are correct.
> > Whereas in the case of b) you are not.
> >
> > I hope that helps clarity the situation somewhat.
> 
> The concern was mostly due to the commit message of
> 
>     bdb0066df96e mfd: syscon: Decouple syscon interface from platform devices
> 
> which mentions
> 
>    there is a need to have a dedicated driver for such system controller
>    but also share registers with other drivers. The latter is where the
>    syscon interface is helpful.
> 
> But does not provide any sort of coordination between the dedicated driver
> and the syscon. I suppose the intention might have been that the driver
> would get a syscon using its own device node. We avoided that but I wonder
> if the extra code is worth it or not. Other platforms seem to do ok.

What sort of co-ordination do you require beyond what is offered?

> Thank you for helping me clear things up.

Never a problem.

-- 
Lee Jones [æçæ]
Senior Technical Lead - Developer Services
Linaro.org â Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog