[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 at milecki.pl>
>>>
>>> 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 at 1eff0000 {
>      compatible = "mmio-nvmem";
>      reg = <0x1eff0000 0x10000>;
> };
>