Message-ID: <20230224154659.432a4d4c@xps-13>
Date:   Fri, 24 Feb 2023 15:46:59 +0100
From:   Miquel Raynal <miquel.raynal@...tlin.com>
To:     Saravana Kannan <saravanak@...gle.com>
Cc:     Maxim Kiselev <bigunclemax@...il.com>,
        Sudeep Holla <sudeep.holla@....com>,
        Naresh Kamboju <naresh.kamboju@...aro.org>,
        abel.vesa@...aro.org, alexander.stein@...tq-group.com,
        andriy.shevchenko@...ux.intel.com, brgl@...ev.pl,
        colin.foster@...advantage.com, cristian.marussi@....com,
        devicetree@...r.kernel.org, dianders@...omium.org,
        djrscally@...il.com, dmitry.baryshkov@...aro.org,
        festevam@...il.com, fido_max@...ox.ru, frowand.list@...il.com,
        geert+renesas@...der.be, geert@...ux-m68k.org,
        gregkh@...uxfoundation.org, heikki.krogerus@...ux.intel.com,
        jpb@...nel.org, jstultz@...gle.com, kernel-team@...roid.com,
        kernel@...gutronix.de, lenb@...nel.org, linus.walleij@...aro.org,
        linux-acpi@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-gpio@...r.kernel.org, linux-imx@....com,
        linux-kernel@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
        linux@...ck-us.net, lkft@...aro.org, luca.weiss@...rphone.com,
        magnus.damm@...il.com, martin.kepplinger@...i.sm, maz@...nel.org,
        rafael@...nel.org, robh+dt@...nel.org, s.hauer@...gutronix.de,
        sakari.ailus@...ux.intel.com, shawnguo@...nel.org,
        tglx@...utronix.de, tony@...mide.com,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Subject: Re: [PATCH v2 00/11] fw_devlink improvements

Hi Saravana,

> > > > > So, can you please retest config 1 with all pr_debug and dev_dbg in
> > > > > drivers/core/base.c changed to the _info variants? And then share the
> > > > > kernel log from the beginning of boot? Maybe attach it to the email so
> > > > > it doesn't get word wrapped by your email client. And please point me
> > > > > to the .dts that corresponds to your board. Without that, I can't
> > > > > debug much.  
> > > >
> > > > Ok, I retested config 1 with all _debug logs changed to the _info. I
> > > > added the kernel log and the dts file to the attachment of this email.  
> > >
> > > Ah, so your device is not supported/present upstream? Even though it's
> > > not upstream, I'll help fix this because it should fix what I believe
> > > are unreported issues in upstream.
> > >
> > > Ok I know why configs 1 - 4 behaved the way they did and why my test
> > > patch didn't help.
> > >
> > > After staring at mtd/nvmem code for a few hours I think mtd/nvmem
> > > interaction is kind of a mess.  
> >
> > nvmem is a recent subsystem but mtd carries a lot of legacy stuff we
> > cannot really re-wire without breaking users, so nvmem on top of mtd
> > of course inherit from the fragile designs in place.  
> 
> Thanks for the context. Yeah, I figured. That's why I explicitly
> limited my comment to "interaction". Although, I'd love to see the MTD
> parsers all be converted to proper drivers that probe. MTD is
> essentially repeating the driver matching logic. I think it can be
> cleaned up to move to proper drivers and still not break backward
> compatibility. Not saying it'll be trivial, but it should be possible.
> Ironically MTD uses mtd_class but has real drivers that work on the
> device (compared to nvmem_bus below).
> 
> > > mtd core creates "partition" platform
> > > devices (including for nvmem-cells) that are probed by drivers in
> > > drivers/nvmem. However, there's no driver for "nvmem-cells" partition
> > > platform device. However, the nvmem core creates nvmem_device when
> > > nvmem_register() is called by MTD or these partition platform devices
> > > created by MTD. But these nvmem_devices are added to a nvmem_bus but
> > > the bus has no means to even register a driver (it should really be a
> > > nvmem_class and not nvmem_bus).  
> >
> > Srinivas, do you think we could change this?  
> 
> Yeah, this part gets a bit tricky. It depends on whether the sysfs
> files for nvmem devices is considered an ABI. Changing from bus to
> class would change the sysfs path for nvmem devices from:
> /sys/class/nvmem to /sys/bus/nvmem

Ok, so this is a no :)

> > > And the nvmem_device sometimes points
> > > to the DT node of the MTD device or sometimes the partition platform
> > > devices or maybe no DT node at all.  
> >
> > I guess this comes from the fact that this is not strongly defined in
> > mtd and depends on the situation (not mentioning 20 years of history
> > there as well). "mtd" is a bit inconsistent on what it means. Older
> > designs mixed: controllers, ECC engines when relevant and memories;
> > while these three components are completely separated. Hence
> > sometimes the mtd device ends up being the top level controller,
> > sometimes it's just one partition...
> >
> > But I'm surprised not all of them point to a DT node. Could you show us
> > an example? Because that might likely be unexpected (or perhaps I am
> > missing something).  
> 
> Well, the logic that sets the DT node for nvmem_device is like so:
> 
>         if (config->of_node)
>                 nvmem->dev.of_node = config->of_node;
>         else if (!config->no_of_node)
>                 nvmem->dev.of_node = config->dev->of_node;
> 
> So there's definitely a path (where both if's could be false) where
> the DT node will not get set. I don't know if that path is possible
> with the existing users of nvmem_register(), but it's definitely
> possible.

It's an actual path. I just checked more in details, this is the change
from 2018 which uses the no_of_node flag:
c4dfa25ab307 ("mtd: add support for reading MTD devices via the nvmem API")

It basically allows any mtd device to be accessible (read-only) through
nvmem. So mtd partitions or such which are not described in the DT may
just be accessed through nvmem (that is my current understanding).

There was later a patch in 2021 which prevented this flag to be
automatically set, so that if partitions (well, mtd devices in general)
were described in the DT, they would provide a valid of_node in order
to be used as cell providers (again, my understanding):
658c4448bbbf ("mtd: core: add nvmem-cells compatible to parse mtd as nvmem cells")

But I guess the major problem comes from the nvmem-cell compatible. I
am wondering if it would make sense to kind of transpose the meaning of
this compatible into a property. But, well, backward compatibility
would still be a problem I guess...

> > > So it's a mess of multiple devices pointing to the same DT node with
> > > no clear way to identify which ones will point to a DT node and which
> > > ones will probe and which ones won't. In the future, we shouldn't
> > > allow adding new compatible strings for partitions for which we don't
> > > plan on adding nvmem drivers.
> > >
> > > Can you give the patch at the end of the email a shot? It should fix
> > > the issue with this series and without this series. It just avoids
> > > this whole mess by not creating useless platform device for
> > > nvmem-cells compatible DT nodes.  
> >
> > Thanks a lot for your help.  
> 
> No problem. I want fw_devlink to work for everyone.
> 

Thanks,
Miquèl

Powered by blists - more mailing lists