Re: [PATCH V4 2/2] nvmem: add generic driver for devices with MMIO access

[Srinivas Kandagatla]

On 08/03/2023 15:42, Rafał Miłecki wrote:
> On 8.03.2023 14:31, Srinivas Kandagatla wrote:
> > Thanks for doing this,
>
> Thank you for reviewing. Sadly it seems it still isn't clear if we can
> have this generic driver.

I don't mean to be rude, but TBH, I don't see any value for this ATM, it 
is going to add something that we need to keep updating for every user.

Unless anyone thinks otherwise.

> I guess I missed some important questions or comments. In previous
> series we were discussing implementation details so I thought it's OK to
> have this driver after all. Not sure if I didn't waste time working on
> V4. I'll see if I can I address your concerns (see below).
Lets not waste your time for now, we can revist this once we have more 
users.

thanks,
srini
> > On 28/02/2023 07:29, Rafał Miłecki wrote:
> > > From: Rafał Miłecki <rafal@xxxxxxxxxx>
> > >
> > > Some NVMEM devices can be accessed by simply mapping memory and reading
> > > from / writing to it. This driver adds support for a generic
> > > "mmio-nvmem" DT binding used by such devices.
> > >
> > > One of such devices is Broadcom's NVRAM. It's already supported (see
> > > NVMEM_BRCM_NVRAM) but existing driver covers both:
> >
> > What will happen to the old "brcm,nvram" compatible and the dt 
> > firmware that already have this node?
>
> I treat backward compatibility with previouly used bindings very
> seriously. I'm going to keep it. I may make an attempt to drop it in
> few years if it's very unlikely to break any setups.
>
>
> > If there is only one user for this then one would object that why do 
> > we need this DT level of abstraction to start with?
> > If this is not the case please consider adding those patches to this 
> > series.
>
> Existing Linux drivers prove that there is more hardware with MMIO based
> read access: brcm_nvram, mtk-efuse, uniphier-efuse. Migration of other
> drivers (mtk, unipher) is on hold as apparently there may be support for
> writing support soon. In any case this MMIO solution isn't completely
> unique to Broadcom.
> I don't have other patches to add to it right now.
>
>
> > > 1. NVMEM device access
> > > 2. NVMEM content parsing
> > >
> > > Once we get support for NVMEM layouts then existing NVRAM driver will
> > > get converted into a layout and generic driver will take over
> > > responsibility for data access.
> >
> > Even though this series is simple, but it is really confusing for two 
> > reasons.
> >
> > 1> Generic mmio nvmem bindings are incomplete and potentially 
> > change/evolve on every new user. Ex clks, regulators, endianess ... So 
> > it looks really fragile and incomplete to me as a generic bindings.
> > Is this want you are expecting?
>
> All 3 existing hardware support MMIO reads without extra clocks or
> regulators. I'm not sure if endianess belongs to this layer. Isn't that
> NVMEM content thing?
>
> I'm not claiming this driver is in its final and perfect state. For
> simple hardware that needs minor fixups we can add those later to this
> generic driver. Adding clocks should be possible, fine and easy.
>
> I'm sure there will be more complex hardware that we will not be able
> to support with this driver. It's require another driver and I'm fine
> with that.
>
>
> > 2> As you mentioned that this will replace broadcom NVMRAM, but this 
> > patch does nothing in relation to updating that driver, so the code is 
> > dead as it is. If you are considering to use it for Broadcom NVMRAM, 
> > please add those patches to this series so that we could see the real 
> > user for this code.
>
> Of course it does nothing because there are no layouts yet. I could
> migrate brcm_nvram into layout once there is layouts support.
>
> I don't agree this code is dead. It support new binding. It works.
> Every new binding and its driver are "dead" until you add first DT
> users.
>
> Here is real use:
>
> nvmem@1eff0000 {
>      compatible = "mmio-nvmem";
>      reg = <0x1eff0000 0x10000>;
> };