[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1b3024876259eab4464db9ca676a884f@walle.cc>
Date: Wed, 01 Feb 2023 21:15:50 +0100
From: Michael Walle <michael@...le.cc>
To: Rob Herring <robh@...nel.org>
Cc: Rafał Miłecki <zajec5@...il.com>,
devicetree@...r.kernel.org, hayashi.kunihiko@...ionext.com,
krzysztof.kozlowski+dt@...aro.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-mediatek@...ts.infradead.org, matthias.bgg@...il.com,
mhiramat@...nel.org, rafal@...ecki.pl,
srinivas.kandagatla@...aro.org
Subject: Re: [PATCH 3/4] nvmem: mtk-efuse: replace driver with a generic MMIO
one
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
Powered by blists - more mailing lists