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

[Michael Walle <michael@walle.cc>]

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