| Message ID | 20230201064717.18410-4-zajec5@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | nvmem: add and use generic MMIO NVMEM | expand |
> 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
diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig index 9eb5e93f0455..4d652c7382a6 100644 --- a/drivers/nvmem/Kconfig +++ b/drivers/nvmem/Kconfig @@ -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" diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile index 2f2bed7cdf24..7a8e29ea408e 100644 --- a/drivers/nvmem/Makefile +++ b/drivers/nvmem/Makefile @@ -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 diff --git a/drivers/nvmem/mmio.c b/drivers/nvmem/mmio.c index 19c8880dc675..253ade72e0c3 100644 --- a/drivers/nvmem/mmio.c +++ b/drivers/nvmem/mmio.c @@ -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", }, {}, }; diff --git a/drivers/nvmem/mtk-efuse.c b/drivers/nvmem/mtk-efuse.c deleted file mode 100644 index a08e0aedd21c..000000000000 --- a/drivers/nvmem/mtk-efuse.c +++ /dev/null @@ -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");