Re: [PATCH 3/3] arm64: dts: imx91_93_common: Enable BBNSM NVMEM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] arm64: dts: imx91_93_common: invalid #address-cells/#size-cells on bbnsm_nvmem
- [Medium] nvmem: imx-bbnsm: select MFD_SYSCON instead of REGMAP_MMIO in Kconfig
--

commit 0a2632d30940d432e106da0e2de92818ce87b154
Author: Stefan Wahren <wahrenst@gmx.net>

arm64: dts: imx91_93_common: Enable BBNSM NVMEM

This series adds devicetree bindings and an NVMEM driver for the i.MX9
Battery-Backed Non-Secure Module (BBNSM) general purpose registers, and
enables the BBNSM NVMEM node in imx91_93_common.dtsi.

> diff --git a/Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml b/Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml
> index b1ade64a15541..4c4b7c3fff2a2 100644
> --- a/Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml
> +++ b/Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml
> @@ -31,6 +31,20 @@ properties:
>    reg:
>      maxItems: 1
>  
> +  nvmem:
> +    type: object
> +    $ref: /schemas/nvmem/nvmem.yaml#
> +
> +    properties:
> +      compatible:
> +        enum:
> +          - nxp,imx93-bbnsm-nvmem
> +
> +    required:
> +      - compatible
> +
> +    additionalProperties: false
> +
>    rtc:
>      type: object
>      $ref: /schemas/rtc/rtc.yaml#
> @@ -88,6 +102,12 @@ examples:
>          compatible = "nxp,imx93-bbnsm", "syscon", "simple-mfd";
>          reg = <0x44440000 0x10000>;
>  
> +        nvmem {
> +            compatible = "nxp,imx93-bbnsm-nvmem";
> +            #address-cells = <1>;
> +            #size-cells = <1>;
> +        };
> +
>          bbnsm_rtc: rtc {
>              compatible = "nxp,imx93-bbnsm-rtc";
>              interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>;
> diff --git a/arch/arm64/boot/dts/freescale/imx91_93_common.dtsi b/arch/arm64/boot/dts/freescale/imx91_93_common.dtsi
> index a1a7e6a0571bd..12a505a2c1546 100644
> --- a/arch/arm64/boot/dts/freescale/imx91_93_common.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx91_93_common.dtsi
> @@ -357,6 +357,12 @@ bbnsm: bbnsm@44440000 {
>  				compatible = "nxp,imx93-bbnsm", "syscon", "simple-mfd";
>  				reg = <0x44440000 0x10000>;
>  
> +				bbnsm_nvmem: nvmem {
> +					compatible = "nxp,imx93-bbnsm-nvmem";
> +					#address-cells = <1>;
> +					#size-cells = <1>;
> +				};

[Severity: Medium]
Should the nvmem schema in Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml
use unevaluatedProperties: false instead of additionalProperties: false, and
should #address-cells and #size-cells be dropped from both the binding example
and bbnsm_nvmem in arch/arm64/boot/dts/freescale/imx91_93_common.dtsi?

Because properties.nvmem in Documentation/devicetree/bindings/mfd/nxp,bbnsm.yaml
references /schemas/nvmem/nvmem.yaml# via $ref and specifies
additionalProperties: false with only compatible listed under properties, any
other property inherited from nvmem.yaml (such as #address-cells, #size-cells,
or nvmem-layout) is rejected during dt_binding_check and dtbs_check.

In addition, imx_bbnsm_probe() in drivers/nvmem/imx-bbnsm.c registers the
device without setting add_legacy_fixed_of_cells:

drivers/nvmem/imx-bbnsm.c:imx_bbnsm_probe() {
	...
	priv->cfg.stride = 4;
	priv->cfg.word_size = 4;
	priv->cfg.size = gpr_words * sizeof(u32);
	priv->cfg.reg_read = imx_bbnsm_read;
	priv->cfg.reg_write = imx_bbnsm_write;

	nvmem = devm_nvmem_register(dev, &priv->cfg);
}

Since nvmem_register() only parses direct child cell nodes when
config->add_legacy_fixed_of_cells is true (and nxp,bbnsm.yaml does not
reference nvmem-deprecated-cells.yaml), cells defined directly under
bbnsm_nvmem are ignored at runtime, while defining cells inside an
nvmem-layout subnode will trigger dtc avoid_unnecessary_addr_size warnings if
#address-cells and #size-cells remain on the parent bbnsm_nvmem node.

[ ... ]

> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index c36c2a4c2a0b7..8d492d05f1602 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -185,6 +185,18 @@ config NVMEM_BRCM_NVRAM
>  	  This driver provides support for Broadcom's NVRAM that can be accessed
>  	  using I/O mapping.
>  
> +config NVMEM_IMX_BBNSM
> +	tristate "i.MX BBNSM general purpose register support"
> +	depends on ARCH_MXC || COMPILE_TEST
> +	depends on OF
> +	select REGMAP_MMIO

[Severity: Medium]
Should NVMEM_IMX_BBNSM select MFD_SYSCON (and depend on HAS_IOMEM) instead of
selecting REGMAP_MMIO?

In drivers/nvmem/imx-bbnsm.c, imx_bbnsm_probe() obtains its regmap via
syscon_node_to_regmap(parent):

	priv->regmap = syscon_node_to_regmap(parent);
	of_node_put(parent);
	if (IS_ERR(priv->regmap))
		return PTR_ERR(priv->regmap);

If CONFIG_MFD_SYSCON is not enabled, syscon_node_to_regmap() falls back to the
stub in include/linux/mfd/syscon.h that returns ERR_PTR(-ENOTSUPP), causing
imx_bbnsm_probe() to fail at runtime. Selecting MFD_SYSCON (which itself
selects REGMAP_MMIO and depends on HAS_IOMEM) also avoids selecting
REGMAP_MMIO on architectures without HAS_IOMEM under COMPILE_TEST.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101200140.141642-1-wahrenst@gmx.net?part=3