Re: [RFC PATCH 3/3] nvmem: uniphier-efuse: Add ti,am62p-efuse compatible

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Using byte-wise register reads (`readb()`) on TI K3 AM62p efuse registers will cause a synchronous external abort (bus fault).
- [Medium] Reusing a vendor-specific driver (Socionext Uniphier) for an unrelated vendor's SoC (Texas Instruments AM62p) purely for simple MMIO reads is a design anti-pattern.
- [Medium] The new `ti,am62p-efuse` compatible incorrectly inherits legacy NVMEM device tree binding behavior.
- [Medium] The driver does not ensure that clocks and power domains are enabled before accessing hardware registers.
--

--- Patch [3]: [RFC PATCH 3/3] nvmem: uniphier-efuse: Add ti,am62p-efuse compatible ---
commit 364e40673a46df742ec59f8dd05432dba10e9a3a
Author: Judith Mendez <[email protected]>

nvmem: uniphier-efuse: Add ti,am62p-efuse compatible

Add ti,am62p-efuse compatible to uniphier-efuse. Leverage the
nvmem provider to supply silicon revision information from GP_SW
efuse registers on AM62p device.

> diff --git a/drivers/nvmem/uniphier-efuse.c b/drivers/nvmem/uniphier-efuse.c
> index 6ad3295d31951..35977e1864135 100644
> --- a/drivers/nvmem/uniphier-efuse.c
> +++ b/drivers/nvmem/uniphier-efuse.c
> @@ -60,6 +60,7 @@ static int uniphier_efuse_probe(struct platform_device *pdev)
>  
>  static const struct of_device_id uniphier_efuse_of_match[] = {
>  	{ .compatible = "socionext,uniphier-efuse",},
> +	{ .compatible = "ti,am62p-efuse",},

[Severity: Medium]
Is it appropriate to reuse a vendor-specific driver (Socionext Uniphier) for
an unrelated vendor's SoC (Texas Instruments AM62p) purely for simple MMIO
reads?

If a simple generic MMIO-based NVMEM provider is needed, would it be better to
use a generic nvmem-mmio driver or syscon-based nvmem rather than piggybacking
onto another vendor's specific driver?

[Severity: High]
Will the byte-wise register reads in this driver cause a synchronous external
abort (bus fault) on TI K3 AM62p hardware?

The uniphier-efuse driver hardcodes a 1-byte word size and stride, and uses
readb() to read data:

drivers/nvmem/uniphier-efuse.c:uniphier_reg_read() {
    ...
    *val++ = readb(priv->base + reg + offs);
    ...
}

Since TI K3 SoC peripherals strictly require 32-bit aligned reads (readl()),
could performing 8-bit reads on these hardware registers result in an
external bus exception and crash the system?

[Severity: Medium]
By binding the new TI compatible to this driver, does the AM62p incorrectly
inherit legacy NVMEM device tree binding behavior?

The driver unconditionally enables legacy parsing:

drivers/nvmem/uniphier-efuse.c:uniphier_efuse_probe() {
    ...
    econfig.add_legacy_fixed_of_cells = true;
    ...
}

This tells the NVMEM core to fall back to the deprecated #address-cells and
#size-cells parsing. Shouldn't new NVMEM providers use the standard
nvmem-layout binding?

[Severity: Medium]
How does this driver ensure that clocks and power domains are enabled before
accessing hardware registers on the AM62p?

The uniphier-efuse probe function performs raw memory-mapped reads without any
prior runtime power management, such as pm_runtime_enable() or
pm_runtime_get_sync().

On TI K3 platforms, accessing peripheral registers when the corresponding
TI SCI power domain is disabled typically leads to a system hang or bus fault.

Are there protections in place to prevent this if the domain is ever
power-gated?

>  	{/* sentinel */},
>  };
>  MODULE_DEVICE_TABLE(of, uniphier_efuse_of_match);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3