Messages in this thread | | | Date | Thu, 9 Mar 2023 09:56:13 +0000 | Subject | Re: [PATCH V4 2/2] nvmem: add generic driver for devices with MMIO access | From | 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@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>; > }; >
| |