Messages in this thread | | | Date | Wed, 8 Mar 2023 16:42:55 +0100 | Subject | Re: [PATCH V4 2/2] nvmem: add generic driver for devices with MMIO access | From | Rafał Miłecki <> |
| |
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 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).
> On 28/02/2023 07:29, Rafał Miłecki wrote: >> From: Rafał Miłecki <rafal@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@1eff0000 { compatible = "mmio-nvmem"; reg = <0x1eff0000 0x10000>; };
| |