[PATCH v2] nvmem: mtk-efuse: Only register socinfo device if needed cells are there

[PATCH v2] nvmem: mtk-efuse: Only register socinfo device if needed cells are there

[Nícolas F. R. A. Prado]

Not every efuse region has cells storing SoC information. Only register
an socinfo device if the required cells are present.

This prevents the pointless process of creating an socinfo device,
probing it with the socinfo driver only to ultimately error out like so

  mtk-socinfo mtk-socinfo.0.auto: error -ENOENT: Failed to get socinfo data
  mtk-socinfo mtk-socinfo.0.auto: probe with driver mtk-socinfo failed with error -2

This issue is observed on the mt8183-kukui-jacuzzi-juniper-sku16
platform, which has two efuse regions, but only one of them contains the
SoC data.

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
---
Changes in v2:
- Added missing include for of.h
- Link to v1: https://lore.kernel.org/r/20240708-mtk-socinfo-no-data-probe-err-v1-1-fb2acd3a47bf@collabora.com
---
 drivers/nvmem/mtk-efuse.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/nvmem/mtk-efuse.c b/drivers/nvmem/mtk-efuse.c
index 9caf04667341..74def409bc20 100644
--- a/drivers/nvmem/mtk-efuse.c
+++ b/drivers/nvmem/mtk-efuse.c
@@ -11,6 +11,7 @@
 #include <linux/nvmem-provider.h>
 #include <linux/platform_device.h>
 #include <linux/property.h>
+#include <linux/of.h>
 
 struct mtk_efuse_pdata {
 	bool uses_post_processing;
@@ -60,6 +61,8 @@ static void mtk_efuse_fixup_dt_cell_info(struct nvmem_device *nvmem,
 		cell->read_post_process = mtk_efuse_gpu_speedbin_pp;
 }
 
+static const char socinfo_data_first_name[] = "socinfo-data1";
+
 static int mtk_efuse_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -69,6 +72,7 @@ static int mtk_efuse_probe(struct platform_device *pdev)
 	struct mtk_efuse_priv *priv;
 	const struct mtk_efuse_pdata *pdata;
 	struct platform_device *socinfo;
+	struct device_node *np;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -92,10 +96,16 @@ static int mtk_efuse_probe(struct platform_device *pdev)
 	if (IS_ERR(nvmem))
 		return PTR_ERR(nvmem);
 
-	socinfo = platform_device_register_data(&pdev->dev, "mtk-socinfo",
-						PLATFORM_DEVID_AUTO, NULL, 0);
-	if (IS_ERR(socinfo))
-		dev_info(dev, "MediaTek SoC Information will be unavailable\n");
+	np = of_get_child_by_name(pdev->dev.of_node, socinfo_data_first_name);
+	if (np) {
+		of_node_put(np);
+		socinfo = platform_device_register_data(&pdev->dev, "mtk-socinfo",
+							PLATFORM_DEVID_AUTO, NULL, 0);
+		if (IS_ERR(socinfo))
+			dev_info(dev, "MediaTek SoC Information will be unavailable\n");
+	} else {
+		dev_info(dev, "Efuse region does not contain SoC information - skipping socinfo driver setup\n");
+	}
 
 	platform_set_drvdata(pdev, socinfo);
 	return 0;

---
base-commit: 0b58e108042b0ed28a71cd7edf5175999955b233
change-id: 20240708-mtk-socinfo-no-data-probe-err-d7558343dc82

Best regards,
-- 
Nícolas F. R. A. Prado <nfraprado@collabora.com>

Re: [PATCH v2] nvmem: mtk-efuse: Only register socinfo device if needed cells are there

[AngeloGioacchino Del Regno]

Il 08/07/24 21:43, Nícolas F. R. A. Prado ha scritto:
> Not every efuse region has cells storing SoC information. Only register
> an socinfo device if the required cells are present.
> 
> This prevents the pointless process of creating an socinfo device,
> probing it with the socinfo driver only to ultimately error out like so
> 
>    mtk-socinfo mtk-socinfo.0.auto: error -ENOENT: Failed to get socinfo data
>    mtk-socinfo mtk-socinfo.0.auto: probe with driver mtk-socinfo failed with error -2
> 
> This issue is observed on the mt8183-kukui-jacuzzi-juniper-sku16
> platform, which has two efuse regions, but only one of them contains the
> SoC data.
> 

I think that we should rather remove or disable the first eFuse region, as
even though that is enabled:

  - This is the only SoC having two regions
    - I'm not even sure that the region at 0x8000000 is really efuse
    - Not even referenced in datasheets....
  - It's unused, as in, it's not exposing any information and no declared cells

Don't misunderstand me, this is not an invalid change, but I rather prefer
to resolve this by disabling that (effectively unused!) node, avoiding to
add more lines to this driver that would be useless after fixing that small
single thing.

Cheers,
Angelo


> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> ---
> Changes in v2:
> - Added missing include for of.h
> - Link to v1: https://lore.kernel.org/r/20240708-mtk-socinfo-no-data-probe-err-v1-1-fb2acd3a47bf@collabora.com
> ---
>   drivers/nvmem/mtk-efuse.c | 18 ++++++++++++++----
>   1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nvmem/mtk-efuse.c b/drivers/nvmem/mtk-efuse.c
> index 9caf04667341..74def409bc20 100644
> --- a/drivers/nvmem/mtk-efuse.c
> +++ b/drivers/nvmem/mtk-efuse.c
> @@ -11,6 +11,7 @@
>   #include <linux/nvmem-provider.h>
>   #include <linux/platform_device.h>
>   #include <linux/property.h>
> +#include <linux/of.h>
>   
>   struct mtk_efuse_pdata {
>   	bool uses_post_processing;
> @@ -60,6 +61,8 @@ static void mtk_efuse_fixup_dt_cell_info(struct nvmem_device *nvmem,
>   		cell->read_post_process = mtk_efuse_gpu_speedbin_pp;
>   }
>   
> +static const char socinfo_data_first_name[] = "socinfo-data1";
> +
>   static int mtk_efuse_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
> @@ -69,6 +72,7 @@ static int mtk_efuse_probe(struct platform_device *pdev)
>   	struct mtk_efuse_priv *priv;
>   	const struct mtk_efuse_pdata *pdata;
>   	struct platform_device *socinfo;
> +	struct device_node *np;
>   
>   	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>   	if (!priv)
> @@ -92,10 +96,16 @@ static int mtk_efuse_probe(struct platform_device *pdev)
>   	if (IS_ERR(nvmem))
>   		return PTR_ERR(nvmem);
>   
> -	socinfo = platform_device_register_data(&pdev->dev, "mtk-socinfo",
> -						PLATFORM_DEVID_AUTO, NULL, 0);
> -	if (IS_ERR(socinfo))
> -		dev_info(dev, "MediaTek SoC Information will be unavailable\n");
> +	np = of_get_child_by_name(pdev->dev.of_node, socinfo_data_first_name);
> +	if (np) {
> +		of_node_put(np);
> +		socinfo = platform_device_register_data(&pdev->dev, "mtk-socinfo",
> +							PLATFORM_DEVID_AUTO, NULL, 0);
> +		if (IS_ERR(socinfo))
> +			dev_info(dev, "MediaTek SoC Information will be unavailable\n");
> +	} else {
> +		dev_info(dev, "Efuse region does not contain SoC information - skipping socinfo driver setup\n");
> +	}
>   
>   	platform_set_drvdata(pdev, socinfo);
>   	return 0;
> 
> ---
> base-commit: 0b58e108042b0ed28a71cd7edf5175999955b233
> change-id: 20240708-mtk-socinfo-no-data-probe-err-d7558343dc82
> 
> Best regards,

Re: [PATCH v2] nvmem: mtk-efuse: Only register socinfo device if needed cells are there

[Nícolas F. R. A. Prado]

On Wed, Jul 10, 2024 at 11:31:11AM +0200, AngeloGioacchino Del Regno wrote:
> Il 08/07/24 21:43, Nícolas F. R. A. Prado ha scritto:
> > Not every efuse region has cells storing SoC information. Only register
> > an socinfo device if the required cells are present.
> > 
> > This prevents the pointless process of creating an socinfo device,
> > probing it with the socinfo driver only to ultimately error out like so
> > 
> >    mtk-socinfo mtk-socinfo.0.auto: error -ENOENT: Failed to get socinfo data
> >    mtk-socinfo mtk-socinfo.0.auto: probe with driver mtk-socinfo failed with error -2
> > 
> > This issue is observed on the mt8183-kukui-jacuzzi-juniper-sku16
> > platform, which has two efuse regions, but only one of them contains the
> > SoC data.
> > 
> 
> I think that we should rather remove or disable the first eFuse region, as
> even though that is enabled:
> 
>  - This is the only SoC having two regions
>    - I'm not even sure that the region at 0x8000000 is really efuse
>    - Not even referenced in datasheets....
>  - It's unused, as in, it's not exposing any information and no declared cells
> 
> Don't misunderstand me, this is not an invalid change, but I rather prefer
> to resolve this by disabling that (effectively unused!) node, avoiding to
> add more lines to this driver that would be useless after fixing that small
> single thing.

I'm not confident that we can say that that efuse is not exposing any
information. Indeed there are no cells so it's not used by any other driver, but
the efuse contents are still exposed to userspace if CONFIG_NVMEM_SYSFS is
enabled.

I dumped it on one of the mt8183-kukui-jacuzzi-juniper-sku16 units:

  $ ls -l /sys/bus/nvmem/devices/
  total 0
  lrwxrwxrwx    1 root     root             0 Jul 18 21:43 mmtd0 -> ../../../devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/mtd0
  lrwxrwxrwx    1 root     root             0 Jul 18 21:43 nvmem0 -> ../../../devices/platform/soc/8000000.efuse/nvmem0
  lrwxrwxrwx    1 root     root             0 Jul 18 21:43 nvmem1 -> ../../../devices/platform/soc/11f10000.efuse/nvmem1
  
  $ hexdump -C /sys/bus/nvmem/devices/nvmem0/nvmem
  00000000  88 07 00 00 00 8a 00 00  00 ca 00 00 00 00 00 00  |................|
  00000010

I power cycled the unit and ran this again and it still showed the same
contents. I also ran the same on a different unit of the same model and it
showed the same contents. Of course this doesn't prove anything, but given that
the contents seem to be constant across reboots and even different units, it
does look like it could be an efuse to me. :)

As to whether the contents are useful at all, or if there are
userspace applications making use of it I have no clue. But if in doubt,
shouldn't we keep it around?

Thanks,
Nícolas

Re: [PATCH v2] nvmem: mtk-efuse: Only register socinfo device if needed cells are there

[AngeloGioacchino Del Regno]

Il 19/07/24 00:07, Nícolas F. R. A. Prado ha scritto:
> On Wed, Jul 10, 2024 at 11:31:11AM +0200, AngeloGioacchino Del Regno wrote:
>> Il 08/07/24 21:43, Nícolas F. R. A. Prado ha scritto:
>>> Not every efuse region has cells storing SoC information. Only register
>>> an socinfo device if the required cells are present.
>>>
>>> This prevents the pointless process of creating an socinfo device,
>>> probing it with the socinfo driver only to ultimately error out like so
>>>
>>>     mtk-socinfo mtk-socinfo.0.auto: error -ENOENT: Failed to get socinfo data
>>>     mtk-socinfo mtk-socinfo.0.auto: probe with driver mtk-socinfo failed with error -2
>>>
>>> This issue is observed on the mt8183-kukui-jacuzzi-juniper-sku16
>>> platform, which has two efuse regions, but only one of them contains the
>>> SoC data.
>>>
>>
>> I think that we should rather remove or disable the first eFuse region, as
>> even though that is enabled:
>>
>>   - This is the only SoC having two regions
>>     - I'm not even sure that the region at 0x8000000 is really efuse
>>     - Not even referenced in datasheets....
>>   - It's unused, as in, it's not exposing any information and no declared cells
>>
>> Don't misunderstand me, this is not an invalid change, but I rather prefer
>> to resolve this by disabling that (effectively unused!) node, avoiding to
>> add more lines to this driver that would be useless after fixing that small
>> single thing.
> 
> I'm not confident that we can say that that efuse is not exposing any
> information. Indeed there are no cells so it's not used by any other driver, but
> the efuse contents are still exposed to userspace if CONFIG_NVMEM_SYSFS is
> enabled.
> 
> I dumped it on one of the mt8183-kukui-jacuzzi-juniper-sku16 units:
> 
>    $ ls -l /sys/bus/nvmem/devices/
>    total 0
>    lrwxrwxrwx    1 root     root             0 Jul 18 21:43 mmtd0 -> ../../../devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/mtd0
>    lrwxrwxrwx    1 root     root             0 Jul 18 21:43 nvmem0 -> ../../../devices/platform/soc/8000000.efuse/nvmem0
>    lrwxrwxrwx    1 root     root             0 Jul 18 21:43 nvmem1 -> ../../../devices/platform/soc/11f10000.efuse/nvmem1
>    
>    $ hexdump -C /sys/bus/nvmem/devices/nvmem0/nvmem
>    00000000  88 07 00 00 00 8a 00 00  00 ca 00 00 00 00 00 00  |................|
>    00000010
> 
> I power cycled the unit and ran this again and it still showed the same
> contents. I also ran the same on a different unit of the same model and it
> showed the same contents. Of course this doesn't prove anything, but given that
> the contents seem to be constant across reboots and even different units, it
> does look like it could be an efuse to me. :)
> 
> As to whether the contents are useful at all, or if there are
> userspace applications making use of it I have no clue. But if in doubt,
> shouldn't we keep it around?

(Added William-tw Lin from MediaTek to the loop)

I'll say yes only if MediaTek (please!) says that this region has useful
information, and only if MediaTek actually tells us what those fuses are.

The reason is that sometimes when SoCs have multiple efuse regions, one contains
uncalibrated data for factory calibration (etc etc), one contains data that derives
from the uncalibrated regions and that is supposed to be used by the OS.

If we got the uncalibrated data that is *not* for OS usage in the MT8183 DT, then
we can as well just remove it.

Besides, I have no concern about any userspace application using that.

Cheers!

> 
> Thanks,
> Nícolas

Re: [PATCH v2] nvmem: mtk-efuse: Only register socinfo device if needed cells are there

[Nícolas F. R. A. Prado]

On Fri, Jul 19, 2024 at 11:29:03AM +0200, AngeloGioacchino Del Regno wrote:
> Il 19/07/24 00:07, Nícolas F. R. A. Prado ha scritto:
> > On Wed, Jul 10, 2024 at 11:31:11AM +0200, AngeloGioacchino Del Regno wrote:
> > > Il 08/07/24 21:43, Nícolas F. R. A. Prado ha scritto:
> > > > Not every efuse region has cells storing SoC information. Only register
> > > > an socinfo device if the required cells are present.
> > > > 
> > > > This prevents the pointless process of creating an socinfo device,
> > > > probing it with the socinfo driver only to ultimately error out like so
> > > > 
> > > >     mtk-socinfo mtk-socinfo.0.auto: error -ENOENT: Failed to get socinfo data
> > > >     mtk-socinfo mtk-socinfo.0.auto: probe with driver mtk-socinfo failed with error -2
> > > > 
> > > > This issue is observed on the mt8183-kukui-jacuzzi-juniper-sku16
> > > > platform, which has two efuse regions, but only one of them contains the
> > > > SoC data.
> > > > 
> > > 
> > > I think that we should rather remove or disable the first eFuse region, as
> > > even though that is enabled:
> > > 
> > >   - This is the only SoC having two regions
> > >     - I'm not even sure that the region at 0x8000000 is really efuse
> > >     - Not even referenced in datasheets....
> > >   - It's unused, as in, it's not exposing any information and no declared cells
> > > 
> > > Don't misunderstand me, this is not an invalid change, but I rather prefer
> > > to resolve this by disabling that (effectively unused!) node, avoiding to
> > > add more lines to this driver that would be useless after fixing that small
> > > single thing.
> > 
> > I'm not confident that we can say that that efuse is not exposing any
> > information. Indeed there are no cells so it's not used by any other driver, but
> > the efuse contents are still exposed to userspace if CONFIG_NVMEM_SYSFS is
> > enabled.
> > 
> > I dumped it on one of the mt8183-kukui-jacuzzi-juniper-sku16 units:
> > 
> >    $ ls -l /sys/bus/nvmem/devices/
> >    total 0
> >    lrwxrwxrwx    1 root     root             0 Jul 18 21:43 mmtd0 -> ../../../devices/platform/soc/11010000.spi/spi_master/spi1/spi1.0/mtd/mtd0/mtd0
> >    lrwxrwxrwx    1 root     root             0 Jul 18 21:43 nvmem0 -> ../../../devices/platform/soc/8000000.efuse/nvmem0
> >    lrwxrwxrwx    1 root     root             0 Jul 18 21:43 nvmem1 -> ../../../devices/platform/soc/11f10000.efuse/nvmem1
> >    $ hexdump -C /sys/bus/nvmem/devices/nvmem0/nvmem
> >    00000000  88 07 00 00 00 8a 00 00  00 ca 00 00 00 00 00 00  |................|
> >    00000010
> > 
> > I power cycled the unit and ran this again and it still showed the same
> > contents. I also ran the same on a different unit of the same model and it
> > showed the same contents. Of course this doesn't prove anything, but given that
> > the contents seem to be constant across reboots and even different units, it
> > does look like it could be an efuse to me. :)
> > 
> > As to whether the contents are useful at all, or if there are
> > userspace applications making use of it I have no clue. But if in doubt,
> > shouldn't we keep it around?
> 
> (Added William-tw Lin from MediaTek to the loop)
> 
> I'll say yes only if MediaTek (please!) says that this region has useful
> information, and only if MediaTek actually tells us what those fuses are.
> 
> The reason is that sometimes when SoCs have multiple efuse regions, one contains
> uncalibrated data for factory calibration (etc etc), one contains data that derives
> from the uncalibrated regions and that is supposed to be used by the OS.
> 
> If we got the uncalibrated data that is *not* for OS usage in the MT8183 DT, then
> we can as well just remove it.
> 
> Besides, I have no concern about any userspace application using that.

No reply, so I've sent v3.

Thanks,
Nícolas

Re: [PATCH v2] nvmem: mtk-efuse: Only register socinfo device if needed cells are there

[William-tw Lin (林鼎崴)]

On Mon, 2024-08-05 at 13:37 +0200, AngeloGioacchino Del Regno wrote:
> Il 03/08/24 16:34, Nícolas F. R. A. Prado ha scritto:
> > On Fri, Jul 19, 2024 at 11:29:03AM +0200, AngeloGioacchino Del
> > Regno wrote:
> > > Il 19/07/24 00:07, Nícolas F. R. A. Prado ha scritto:
> > > > On Wed, Jul 10, 2024 at 11:31:11AM +0200, AngeloGioacchino Del
> > > > Regno wrote:
> > > > > Il 08/07/24 21:43, Nícolas F. R. A. Prado ha scritto:
> > > > > > Not every efuse region has cells storing SoC information.
> > > > > > Only register
> > > > > > an socinfo device if the required cells are present.
> > > > > > 
> > > > > > This prevents the pointless process of creating an socinfo
> > > > > > device,
> > > > > > probing it with the socinfo driver only to ultimately error
> > > > > > out like so
> > > > > > 
> > > > > >      mtk-socinfo mtk-socinfo.0.auto: error -ENOENT: Failed
> > > > > > to get socinfo data
> > > > > >      mtk-socinfo mtk-socinfo.0.auto: probe with driver mtk-
> > > > > > socinfo failed with error -2
> > > > > > 
> > > > > > This issue is observed on the mt8183-kukui-jacuzzi-juniper-
> > > > > > sku16
> > > > > > platform, which has two efuse regions, but only one of them
> > > > > > contains the
> > > > > > SoC data.
> > > > > > 
> > > > > 
> > > > > I think that we should rather remove or disable the first
> > > > > eFuse region, as
> > > > > even though that is enabled:
> > > > > 
> > > > >    - This is the only SoC having two regions
> > > > >      - I'm not even sure that the region at 0x8000000 is
> > > > > really efuse
> > > > >      - Not even referenced in datasheets....
> > > > >    - It's unused, as in, it's not exposing any information
> > > > > and no declared cells
> > > > > 
> > > > > Don't misunderstand me, this is not an invalid change, but I
> > > > > rather prefer
> > > > > to resolve this by disabling that (effectively unused!) node,
> > > > > avoiding to
> > > > > add more lines to this driver that would be useless after
> > > > > fixing that small
> > > > > single thing.
> > > > 
> > > > I'm not confident that we can say that that efuse is not
> > > > exposing any
> > > > information. Indeed there are no cells so it's not used by any
> > > > other driver, but
> > > > the efuse contents are still exposed to userspace if
> > > > CONFIG_NVMEM_SYSFS is
> > > > enabled.
> > > > 
> > > > I dumped it on one of the mt8183-kukui-jacuzzi-juniper-sku16
> > > > units:
> > > > 
> > > >     $ ls -l /sys/bus/nvmem/devices/
> > > >     total 0
> > > >     lrwxrwxrwx    1 root     root             0 Jul 18 21:43
> > > > mmtd0 ->
> > > > ../../../devices/platform/soc/11010000.spi/spi_master/spi1/spi1
> > > > .0/mtd/mtd0/mtd0
> > > >     lrwxrwxrwx    1 root     root             0 Jul 18 21:43
> > > > nvmem0 -> ../../../devices/platform/soc/8000000.efuse/nvmem0
> > > >     lrwxrwxrwx    1 root     root             0 Jul 18 21:43
> > > > nvmem1 -> ../../../devices/platform/soc/11f10000.efuse/nvmem1
> > > >     $ hexdump -C /sys/bus/nvmem/devices/nvmem0/nvmem
> > > >     00000000  88 07 00 00 00 8a 00 00  00 ca 00 00 00 00 00
> > > > 00  |................|
> > > >     00000010
> > > > 
> > > > I power cycled the unit and ran this again and it still showed
> > > > the same
> > > > contents. I also ran the same on a different unit of the same
> > > > model and it
> > > > showed the same contents. Of course this doesn't prove
> > > > anything, but given that
> > > > the contents seem to be constant across reboots and even
> > > > different units, it
> > > > does look like it could be an efuse to me. :)
> > > > 
> > > > As to whether the contents are useful at all, or if there are
> > > > userspace applications making use of it I have no clue. But if
> > > > in doubt,
> > > > shouldn't we keep it around?
> > > 
> > > (Added William-tw Lin from MediaTek to the loop)
> > > 
> > > I'll say yes only if MediaTek (please!) says that this region has
> > > useful
> > > information, and only if MediaTek actually tells us what those
> > > fuses are.
This node contains some SoC-related information. However, this is
unrelated to the mtk-socninfo driver.
> > > 
> > > The reason is that sometimes when SoCs have multiple efuse
> > > regions, one contains
> > > uncalibrated data for factory calibration (etc etc), one contains
> > > data that derives
> > > from the uncalibrated regions and that is supposed to be used by
> > > the OS.
> > > 
> > > If we got the uncalibrated data that is *not* for OS usage in the
> > > MT8183 DT, then
> > > we can as well just remove it.
> > > 
> > > Besides, I have no concern about any userspace application using
> > > that.
> > 
> > No reply, so I've sent v3.
> > 
> 
> Resolved with devicetree change. Please ignore this patch.
> 
> Cheers,
> Angelo
>