Re: [PATCH 3/4] nvmem: imx-ocotp: Support accessing controller for i.MX8M Nano

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Am Freitag, 31. Januar 2025, 17:06:23 CET schrieb Frank Li:
> On Fri, Jan 31, 2025 at 02:54:06PM +0100, Alexander Stein wrote:
> > Hi,
> >
> > Am Donnerstag, 30. Januar 2025, 17:42:32 CET schrieb Frank Li:
> > > On Thu, Jan 30, 2025 at 02:01:00PM +0100, Alexander Stein wrote:
> > > > i.MX8M OCOTP supports a specific peripheral or function being fused
> > > > which means disabled, so
> > > >  - Introduce disable_fuse for a list of possible fused peripherals.
> > > >  - Iterate all nodes to check accessing permission. If not
> > > >    allowed to be accessed, detach the node
> > > >
> > > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > > > ---
> > > >  drivers/nvmem/Kconfig     |   3 ++
> > > >  drivers/nvmem/imx-ocotp.c | 105 +++++++++++++++++++++++++++++++++++++-
> > > >  2 files changed, 107 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> > > > index 8671b7c974b93..ba5c928cab520 100644
> > > > --- a/drivers/nvmem/Kconfig
> > > > +++ b/drivers/nvmem/Kconfig
> > > > @@ -84,6 +84,9 @@ config NVMEM_IMX_OCOTP
> > > >  	  This driver can also be built as a module. If so, the module
> > > >  	  will be called nvmem-imx-ocotp.
> > > >
> > > > +	  If built as modules, any other driver relying on this working
> > > > +	  as access controller also needs to be a module as well.
> > > > +
> > > >  config NVMEM_IMX_OCOTP_ELE
> > > >  	tristate "i.MX On-Chip OTP Controller support"
> > > >  	depends on ARCH_MXC || COMPILE_TEST
> > > > diff --git a/drivers/nvmem/imx-ocotp.c b/drivers/nvmem/imx-ocotp.c
> > > > index c5086a16450ac..e3ea026a37d0d 100644
> > > > --- a/drivers/nvmem/imx-ocotp.c
> > > > +++ b/drivers/nvmem/imx-ocotp.c
> > > > @@ -23,6 +23,7 @@
> > > >  #include <linux/of.h>
> > > >  #include <linux/platform_device.h>
> > > >  #include <linux/slab.h>
> > > > +#include <dt-bindings/nvmem/fsl,imx8mn-ocotp.h>
> > > >
> > > >  #define IMX_OCOTP_OFFSET_B0W0		0x400 /* Offset from base address of the
> > > >  					       * OTP Bank0 Word0
> > > > @@ -91,11 +92,20 @@ struct ocotp_ctrl_reg {
> > > >  	u32 bm_rel_shadows;
> > > >  };
> > > >
> > > > +#define OCOTP_MAX_NUM_GATE_WORDS 4
> > > > +
> > > > +struct disable_fuse {
> > > > +	u32 fuse_addr;
> > > > +	u32 mask;
> > > > +};
> > > > +
> > > >  struct ocotp_params {
> > > >  	unsigned int nregs;
> > > >  	unsigned int bank_address_words;
> > > >  	void (*set_timing)(struct ocotp_priv *priv);
> > > >  	struct ocotp_ctrl_reg ctrl;
> > > > +	u32 num_disables;
> > > > +	struct disable_fuse *disables;
> > > >  };
> > > >
> > > >  static int imx_ocotp_wait_for_busy(struct ocotp_priv *priv, u32 flags)
> > > > @@ -552,11 +562,25 @@ static const struct ocotp_params imx8mm_params = {
> > > >  	.ctrl = IMX_OCOTP_BM_CTRL_DEFAULT,
> > > >  };
> > > >
> > > > +struct disable_fuse imx8mn_disable_fuse[] = {
> > > > +		[IMX8MN_OCOTP_M7_DISABLE]	= { .fuse_addr = 20, .mask = BIT(8) },
> > > > +		[IMX8MN_OCOTP_M7_MPU_DISABLE]	= { .fuse_addr = 20, .mask = BIT(9) },
> > > > +		[IMX8MN_OCOTP_M7_FPU_DISABLE]	= { .fuse_addr = 20, .mask = BIT(10) },
> > > > +		[IMX8MN_OCOTP_USB_OTG1_DISABLE]	= { .fuse_addr = 20, .mask = BIT(11) },
> > > > +		[IMX8MN_OCOTP_GPU3D_DISABLE]	= { .fuse_addr = 20, .mask = BIT(24) },
> > > > +		[IMX8MN_OCOTP_MIPI_DSI_DISABLE]	= { .fuse_addr = 20, .mask = BIT(28) },
> > > > +		[IMX8MN_OCOTP_ENET_DISABLE]	= { .fuse_addr = 20, .mask = BIT(29) },
> > > > +		[IMX8MN_OCOTP_MIPI_CSI_DISABLE]	= { .fuse_addr = 20, .mask = BIT(30) },
> > > > +		[IMX8MN_OCOTP_ASRC_DISABLE]	= { .fuse_addr = 20, .mask = BIT(31) },
> > > > +};
> > >
> > > Can we direct define IMX8MN_OCOTP_M7_DISABLE as BIT(8), so avoid this
> > > map data?
> >
> > This would be possible for imx8mn, but not for imx8mp which uses
> > multiples fuses for disables. This is an excerpt from imx8mp WIP
> > > struct disable_fuse imx8mp_disable_fuse[] = {
> > > 	[IMX8MP_OCOTP_CAN_DISABLE]		= { .fuse_addr = 16, .mask = BIT(28) },
> > > 	[IMX8MP_OCOTP_CAN_FD_DISABLE]		= { .fuse_addr = 16, .mask = BIT(29) },
> > > 	[IMX8MP_OCOTP_VPU_VC8000E_DISABLE]	= { .fuse_addr = 16, .mask = BIT(30) },
> > > 	[IMX8MP_OCOTP_IMG_ISP1_DISABLE]		= { .fuse_addr = 20, .mask = BIT(0) },
> > > 	[IMX8MP_OCOTP_IMG_ISP2_DISABLE]		= { .fuse_addr = 20, .mask = BIT(1) },
> > > 	[IMX8MP_OCOTP_IMG_DEWARP_DISABLE]	= { .fuse_addr = 20, .mask = BIT(2) },
> > > };
> >
> > Notice the fuse_addr of 16 and 20.
> 
> Yes, I am not sure if it good idea to encode fuse_addr to IMX8MP_OCOTP_CAN_DISABLE
> 
> like
> 
> #define IMX8MP_OCOTP_CAN_DISABLE  16 << 16 | BIT(28)
> 
> So dt-bindings/nvmem/fsl,imx8mn-ocotp.h can be moved to dts directory.

Mh, I personally don't like encoding offsets into bits. How about using
> '#access-controller-cells = <2>'
and using the defines like this
> #define IMX8MP_OCOTP_CAN_DISABLE  16 0x10000000

DT stays the same:
> access-controllers = <&ocotp IMX8MP_OCOTP_CAN_DISABLE>;

Note: It seems BIT(x) is not usable in DT.

Best regards,
Alexander
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/