[3/4] nvmem: mtk-efuse: replace driver with a generic MMIO one
Commit Message
From: Rafał Miłecki <rafal@milecki.pl>
Mediatek EFUSE uses a simple MMIO that can be handled with a generic
driver. Replace this driver to avoid code duplication.
Keep Kconfig symbol for a release or two to help with "make oldconfig".
Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
drivers/nvmem/Kconfig | 7 ++-
drivers/nvmem/Makefile | 2 -
drivers/nvmem/mmio.c | 3 ++
drivers/nvmem/mtk-efuse.c | 97 ---------------------------------------
4 files changed, 6 insertions(+), 103 deletions(-)
delete mode 100644 drivers/nvmem/mtk-efuse.c
Comments
> From: Rafał Miłecki <rafal@milecki.pl>
>
> Mediatek EFUSE uses a simple MMIO that can be handled with a generic
> driver. Replace this driver to avoid code duplication.
I don't think this is the correct approach. You'll restrict that driver
to being read-only. I admit that right now, it's read only, but it can
be extended to also support efuse writing. With this changes, it's not
possible.
> static const struct of_device_id mmio_nvmem_of_match_table[] = {
> { .compatible = "mmio-nvmem", },
>+ /* Custom bindings that were introduced before the mmio one */
>+ { .compatible = "mediatek,mt8173-efuse", },
>+ { .compatible = "mediatek,efuse", },
Why do you assume that all mediatek efuses will be the same? This should
rather be something like (in the dts/binding):
compatible = "mediatek,mt8173-efuse", "mmio-nvmem";
So if there is no driver for the particular efuse, it will fall back to the
generic one.
-michael
On 1.02.2023 09:48, Michael Walle wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> Mediatek EFUSE uses a simple MMIO that can be handled with a generic
>> driver. Replace this driver to avoid code duplication.
>
> I don't think this is the correct approach. You'll restrict that driver
> to being read-only. I admit that right now, it's read only, but it can
> be extended to also support efuse writing. With this changes, it's not
> possible.
>
>> static const struct of_device_id mmio_nvmem_of_match_table[] = {
>> { .compatible = "mmio-nvmem", },
>> + /* Custom bindings that were introduced before the mmio one */
>> + { .compatible = "mediatek,mt8173-efuse", },
>> + { .compatible = "mediatek,efuse", },
>
> Why do you assume that all mediatek efuses will be the same? This should
> rather be something like (in the dts/binding):
>
> compatible = "mediatek,mt8173-efuse", "mmio-nvmem";
>
> So if there is no driver for the particular efuse, it will fall back to the
> generic one.
Oh great, I'm making circles now.
Rob suggested I should convert existing drivers, see:
[PATCH 2/2] nvmem: add generic driver for devices with I/O based access
and I thought efuse ones should be good.
Please tell me how I should handle brcm,nvram without wasting more time.
I thought I had it sorted out but I just wasted 2 days.
I believe I need to make brcm,nvram NVMEM layout. Without converting it
I'm afraid you'll refuse my changes adding cell post processing (that
happened to my U-Boot attempts).
Before I convert brcm,nvram to NVMEM layout I need some binding & driver
providing MMIO device access. How to handle that?
> 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.
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.
-michael
On 1.02.2023 11:46, 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.
OK, then maybe I'll retry again with generic MMIO and without converting
existing specific drivers. That is what (AFAIU) Rob asked though.
> 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.
mediatek,efuse was already there, don't blame me for it ;)
> nitpick btw: why not "nvmem-mmio"?
Because I read from left to right ;)
It's MMIO based NVMEM. Not MMIO on top of NVMEM.
Sure, we have "nvmem-cells" but that is because those are cells of NVMEM
device.
Unless my English knowledge fails me.
> 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.
(2) looks OK, Rob, Krzysztof?
>> nitpick btw: why not "nvmem-mmio"?
>
> Because I read from left to right ;)
if you have more compatibles do you prefer
a-grp
bcdef-grp
gh-grp
or
grp-a
grp-bcdef
grp-gh
To my eyes the latter looks nicer, but it's just a matter of taste ;)
-michael
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.
Personally, I think it is easy enough to revive the driver if needed
unless writing is a soon and likely feature.
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.
Rob
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
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
@@ -184,12 +184,11 @@ config NVMEM_MTK_EFUSE
tristate "Mediatek SoCs EFUSE support"
depends on ARCH_MEDIATEK || COMPILE_TEST
depends on HAS_IOMEM
+ select NVMEM_MMIO
help
- This is a driver to access hardware related data like sensor
- calibration, HDMI impedance etc.
+ This driver has been replaced by a generic MMIO implementation.
- This driver can also be built as a module. If so, the module
- will be called efuse-mtk.
+ Update your config as this symbol will be dropped in the next release.
config NVMEM_MXS_OCOTP
tristate "Freescale MXS On-Chip OTP Memory Support"
@@ -38,8 +38,6 @@ obj-$(CONFIG_NVMEM_MICROCHIP_OTPC) += nvmem-microchip-otpc.o
nvmem-microchip-otpc-y := microchip-otpc.o
obj-$(CONFIG_NVMEM_MMIO) += nvmem-mmio.o
nvmem-mmio-y := mmio.o
-obj-$(CONFIG_NVMEM_MTK_EFUSE) += nvmem_mtk-efuse.o
-nvmem_mtk-efuse-y := mtk-efuse.o
obj-$(CONFIG_NVMEM_MXS_OCOTP) += nvmem-mxs-ocotp.o
nvmem-mxs-ocotp-y := mxs-ocotp.o
obj-$(CONFIG_NVMEM_NINTENDO_OTP) += nvmem-nintendo-otp.o
@@ -57,6 +57,9 @@ static int mmio_nvmem_probe(struct platform_device *pdev)
static const struct of_device_id mmio_nvmem_of_match_table[] = {
{ .compatible = "mmio-nvmem", },
+ /* Custom bindings that were introduced before the mmio one */
+ { .compatible = "mediatek,mt8173-efuse", },
+ { .compatible = "mediatek,efuse", },
{},
};
deleted file mode 100644
@@ -1,97 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * Copyright (c) 2015 MediaTek Inc.
- * Author: Andrew-CT Chen <andrew-ct.chen@mediatek.com>
- */
-
-#include <linux/device.h>
-#include <linux/module.h>
-#include <linux/mod_devicetable.h>
-#include <linux/io.h>
-#include <linux/nvmem-provider.h>
-#include <linux/platform_device.h>
-
-struct mtk_efuse_priv {
- void __iomem *base;
-};
-
-static int mtk_reg_read(void *context,
- unsigned int reg, void *_val, size_t bytes)
-{
- struct mtk_efuse_priv *priv = context;
- void __iomem *addr = priv->base + reg;
- u8 *val = _val;
- int i;
-
- for (i = 0; i < bytes; i++, val++)
- *val = readb(addr + i);
-
- return 0;
-}
-
-static int mtk_efuse_probe(struct platform_device *pdev)
-{
- struct device *dev = &pdev->dev;
- struct resource *res;
- struct nvmem_device *nvmem;
- struct nvmem_config econfig = {};
- struct mtk_efuse_priv *priv;
-
- priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
- if (!priv)
- return -ENOMEM;
-
- priv->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
- if (IS_ERR(priv->base))
- return PTR_ERR(priv->base);
-
- econfig.stride = 1;
- econfig.word_size = 1;
- econfig.reg_read = mtk_reg_read;
- econfig.size = resource_size(res);
- econfig.priv = priv;
- econfig.dev = dev;
- nvmem = devm_nvmem_register(dev, &econfig);
-
- return PTR_ERR_OR_ZERO(nvmem);
-}
-
-static const struct of_device_id mtk_efuse_of_match[] = {
- { .compatible = "mediatek,mt8173-efuse",},
- { .compatible = "mediatek,efuse",},
- {/* sentinel */},
-};
-MODULE_DEVICE_TABLE(of, mtk_efuse_of_match);
-
-static struct platform_driver mtk_efuse_driver = {
- .probe = mtk_efuse_probe,
- .driver = {
- .name = "mediatek,efuse",
- .of_match_table = mtk_efuse_of_match,
- },
-};
-
-static int __init mtk_efuse_init(void)
-{
- int ret;
-
- ret = platform_driver_register(&mtk_efuse_driver);
- if (ret) {
- pr_err("Failed to register efuse driver\n");
- return ret;
- }
-
- return 0;
-}
-
-static void __exit mtk_efuse_exit(void)
-{
- return platform_driver_unregister(&mtk_efuse_driver);
-}
-
-subsys_initcall(mtk_efuse_init);
-module_exit(mtk_efuse_exit);
-
-MODULE_AUTHOR("Andrew-CT Chen <andrew-ct.chen@mediatek.com>");
-MODULE_DESCRIPTION("Mediatek EFUSE driver");
-MODULE_LICENSE("GPL v2");