Re: [PATCH 3/4] nvmem: mtk-efuse: replace driver with a generic MMIO one

[Michael Walle]

Am 2023-02-01 19:54, schrieb Rob Herring:
> On Wed, Feb 01, 2023 at 11:46:01AM +0100, Michael Walle wrote:
> > > Before I convert brcm,nvram to NVMEM layout I need some binding & driver
> > > providing MMIO device access. How to handle that?
> >
> > I'm not arguing against having the mmio nvmem driver. But I don't
> > think we should sacrifice possible write access with other drivers. 
> > And
> > I presume write access won't be possible with your generic driver as 
> > it
> > probably isn't just a memcpy_toio().
> >
> > It is a great fallback for some nvmem peripherals which just maps a
> > memory region, but doesn't replace a proper driver for an nvmem 
> > device.
> >
> > What bothers me the most isn't the driver change. The driver can be
> > resurrected once someone will do proper write access, but the generic
> > "mediatek,efuse" compatible together with the comment above the older
> > compatible string. These imply that you should use "mediatek,efuse",
> > but we don't know if all mediatek efuse peripherals will be the
> > same - esp. for writing which is usually more complex than the 
> > reading.
>
> Because the kernel can't pick the "best" driver when there are multiple
> matches, it's all Mediatek platforms use the generic driver or all use
> the Mediatek driver.

Isn't that the whole point of having multiple compatible strings?
  compatible = "fsl,imx27-mmc", "fsl,imx21-mmc";
The OS might either load the driver for "fsl,imx21-mmc" or one for
"fsl,imx27-mmc", with the latter considered to be the preferred one.

> Personally, I think it is easy enough to revive the driver if needed
> unless writing is a soon and likely feature.

That what was actually triggered my initial reply. We are planning a
new board with a mediatek SoC and we'll likely need the write support.

But I thought the "mediatek,efuse" was a new compatible with this patch
and the (new!) comment make it looks like these compatible are 
deprecated
in favor of "mmio-nvmem". Which would make it impossible to distinguish
between the different efuse peripherals and thus make it impossible to
add write support.

> The other way to share is providing library functions for drivers to
> use. Then the Mediatek driver can use the generic read functions and
> custom write functions.
>
> > nitpick btw: why not "nvmem-mmio"?
> >
> > So it's either:
> >  (1) compatible = "mediatek,mt8173-efuse"
> >  (2) compatible = "mediatek,mt8173-efuse", "mmio-nvmem"
> >
> > (1) will be supported any anyway for older dts and you need to add
> > the specific compatibles to the nvmem-mmio driver - or keep the
> > driver as is.
> >
> > With (2) you wouldn't need to do that and the kernel can load the
> > proper driver if available or fall back to the nvmem-mmio one. I'd
> > even make that one "default y" so it will be available on future
> > kernels and boards can already make use of the nvmem device even
> > if there is no proper driver for them.
> >
> > I'd prefer (2). Dunno what the dt maintainers agree.
>
> No because you are changing the DT. The DT can't change when you want 
> to
> change drivers. This thinking is one reason why 'generic' bindings are
> rejected.

There is no change in the DT. Newer bindings will have

  compatible = "vendor,ip-block", "mmio-nvmem"

when the ip block is compatible with mmio-nvmem. Otherwise I don't get
why there is a mmio-nvmem compatible at all. Just having

  compatible = "mmio-nvmem"

looks wrong as it would just work correctly in some minor cases, i.e.
when write support is just a memcpy_toio() - or we deliberately ignore
any write support. But even then, you always tell people to add specific
compatibles for the case when quirks are needed..

-michael