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

[Rob Herring <robh@kernel.org>]

On Wed, Feb 01, 2023 at 09:15:50PM +0100, Michael Walle wrote:
> 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.

No, there's some assumption they would be similar enough to be served by 
the same driver.

While it seems like we'd want to fix this, it's been an issue I've been 
aware of as long as I've been involved with DT and yet I don't recall 
anyone really having an issue. Could just be everyone couples their 
kernel and dtb versions...

> > 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.

If write support is on the horizon, then I'd say keep the Mediatek 
driver.

> 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.

No, it's in the schema and dts files.

> 
> > 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..

Yes, I do. I was assuming these are simple cases unlikely to need 
platform specific support. We're just reading static bits from 
registers...

If you may need write support or have other complications, then 
"mmio-nvmem" should not be used. You can use the driver, but it should 
match with a platform specific compatible, not the generic one.

Rob