[PATCH] mtd: nand: use regmap_update_bits() in marvell_nand for syscon access

[PATCH] mtd: nand: use regmap_update_bits() in marvell_nand for syscon access

[Thomas Petazzoni]

The marvell_nfc_init() function fiddles with some bits of a system
controller on Armada 7K/8K. However:

 - It does a regmap_write() on GENCONF_SOC_DEVICE_MUX, potentially
   losing other bits that might have been set by other drivers.

 - It does a read/modify/write sequence on GENCONF_CLK_GATING_CTRL and
   GENCONF_ND_CLK_CTRL, which isn't safe from a concurrency point of
   view, as the regmap lock isn't taken accross the read/modify/write
   sequence.

To solve both issues, use regmap_update_bits().

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
NOTE: This was only compile tested, and not runtime tested.
---
 drivers/mtd/nand/raw/marvell_nand.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
index ebb1d141b900..742f2765b424 100644
--- a/drivers/mtd/nand/raw/marvell_nand.c
+++ b/drivers/mtd/nand/raw/marvell_nand.c
@@ -2691,24 +2691,24 @@ static int marvell_nfc_init(struct marvell_nfc *nfc)
 		struct regmap *sysctrl_base =
 			syscon_regmap_lookup_by_phandle(np,
 							"marvell,system-controller");
-		u32 reg;
+		u32 mask;
 
 		if (IS_ERR(sysctrl_base))
 			return PTR_ERR(sysctrl_base);
 
-		reg = GENCONF_SOC_DEVICE_MUX_NFC_EN |
-		      GENCONF_SOC_DEVICE_MUX_ECC_CLK_RST |
-		      GENCONF_SOC_DEVICE_MUX_ECC_CORE_RST |
-		      GENCONF_SOC_DEVICE_MUX_NFC_INT_EN;
-		regmap_write(sysctrl_base, GENCONF_SOC_DEVICE_MUX, reg);
-
-		regmap_read(sysctrl_base, GENCONF_CLK_GATING_CTRL, &reg);
-		reg |= GENCONF_CLK_GATING_CTRL_ND_GATE;
-		regmap_write(sysctrl_base, GENCONF_CLK_GATING_CTRL, reg);
-
-		regmap_read(sysctrl_base, GENCONF_ND_CLK_CTRL, &reg);
-		reg |= GENCONF_ND_CLK_CTRL_EN;
-		regmap_write(sysctrl_base, GENCONF_ND_CLK_CTRL, reg);
+		mask = GENCONF_SOC_DEVICE_MUX_NFC_EN |
+			GENCONF_SOC_DEVICE_MUX_ECC_CLK_RST |
+			GENCONF_SOC_DEVICE_MUX_ECC_CORE_RST |
+			GENCONF_SOC_DEVICE_MUX_NFC_INT_EN;
+		regmap_update_bits(sysctrl_base, GENCONF_SOC_DEVICE_MUX,
+				   mask, mask);
+
+		regmap_update_bits(sysctrl_base, GENCONF_CLK_GATING_CTRL,
+				   GENCONF_CLK_GATING_CTRL_ND_GATE,
+				   GENCONF_CLK_GATING_CTRL_ND_GATE);
+		regmap_update_bits(sysctrl_base, GENCONF_ND_CLK_CTRL,
+				   GENCONF_ND_CLK_CTRL_EN,
+				   GENCONF_ND_CLK_CTRL_EN);
 	}
 
 	/* Configure the DMA if appropriate */
-- 
2.14.4

Re: [PATCH] mtd: nand: use regmap_update_bits() in marvell_nand for syscon access

[Miquel Raynal]

Hi Thomas,

Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote on Wed,  1 Aug
2018 12:25:05 +0200:

> The marvell_nfc_init() function fiddles with some bits of a system
> controller on Armada 7K/8K. However:
> 
>  - It does a regmap_write() on GENCONF_SOC_DEVICE_MUX, potentially
>    losing other bits that might have been set by other drivers.

AFAIR, this was done on purpose. In the aim of being as independent as
possible from the earlier stages, we set here the full configuration of
register 0xF2440208 "SoC Device Multiplex Register". Bits are either
reserved or NAND controller related, nobody else is supposed to poke
here. I'm not sure using regmap_update_bits() here is relevant?

>  - It does a read/modify/write sequence on GENCONF_CLK_GATING_CTRL and
>    GENCONF_ND_CLK_CTRL, which isn't safe from a concurrency point of
>    view, as the regmap lock isn't taken accross the read/modify/write
>    sequence.

Yes, for this one I totally agree.

> To solve both issues, use regmap_update_bits().
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

I think this a good candidate for stable if there is a v2, could you
please add the relevant tags? The problem is laying since:
02f26ecf8c77 ("mtd: nand: add reworked Marvell NAND controller driver")

> ---
> NOTE: This was only compile tested, and not runtime tested.
> ---
>  drivers/mtd/nand/raw/marvell_nand.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
> index ebb1d141b900..742f2765b424 100644
> --- a/drivers/mtd/nand/raw/marvell_nand.c
> +++ b/drivers/mtd/nand/raw/marvell_nand.c
> @@ -2691,24 +2691,24 @@ static int marvell_nfc_init(struct marvell_nfc *nfc)
>  		struct regmap *sysctrl_base =
>  			syscon_regmap_lookup_by_phandle(np,
>  							"marvell,system-controller");
> -		u32 reg;
> +		u32 mask;
>  
>  		if (IS_ERR(sysctrl_base))
>  			return PTR_ERR(sysctrl_base);
>  
> -		reg = GENCONF_SOC_DEVICE_MUX_NFC_EN |
> -		      GENCONF_SOC_DEVICE_MUX_ECC_CLK_RST |
> -		      GENCONF_SOC_DEVICE_MUX_ECC_CORE_RST |
> -		      GENCONF_SOC_DEVICE_MUX_NFC_INT_EN;
> -		regmap_write(sysctrl_base, GENCONF_SOC_DEVICE_MUX, reg);
> -
> -		regmap_read(sysctrl_base, GENCONF_CLK_GATING_CTRL, &reg);
> -		reg |= GENCONF_CLK_GATING_CTRL_ND_GATE;
> -		regmap_write(sysctrl_base, GENCONF_CLK_GATING_CTRL, reg);
> -
> -		regmap_read(sysctrl_base, GENCONF_ND_CLK_CTRL, &reg);
> -		reg |= GENCONF_ND_CLK_CTRL_EN;
> -		regmap_write(sysctrl_base, GENCONF_ND_CLK_CTRL, reg);
> +		mask = GENCONF_SOC_DEVICE_MUX_NFC_EN |
> +			GENCONF_SOC_DEVICE_MUX_ECC_CLK_RST |
> +			GENCONF_SOC_DEVICE_MUX_ECC_CORE_RST |
> +			GENCONF_SOC_DEVICE_MUX_NFC_INT_EN;

Minor nit: indentation looks wrong here?

> +		regmap_update_bits(sysctrl_base, GENCONF_SOC_DEVICE_MUX,
> +				   mask, mask);
> +
> +		regmap_update_bits(sysctrl_base, GENCONF_CLK_GATING_CTRL,
> +				   GENCONF_CLK_GATING_CTRL_ND_GATE,
> +				   GENCONF_CLK_GATING_CTRL_ND_GATE);
> +		regmap_update_bits(sysctrl_base, GENCONF_ND_CLK_CTRL,
> +				   GENCONF_ND_CLK_CTRL_EN,
> +				   GENCONF_ND_CLK_CTRL_EN);
>  	}
>  
>  	/* Configure the DMA if appropriate */


Thanks,
Miquèl

Re: [PATCH] mtd: nand: use regmap_update_bits() in marvell_nand for syscon access

[Thomas Petazzoni]

Hello,

On Wed, 1 Aug 2018 14:27:41 +0200, Miquel Raynal wrote:

> Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote on Wed,  1 Aug
> 2018 12:25:05 +0200:
> 
> > The marvell_nfc_init() function fiddles with some bits of a system
> > controller on Armada 7K/8K. However:
> > 
> >  - It does a regmap_write() on GENCONF_SOC_DEVICE_MUX, potentially
> >    losing other bits that might have been set by other drivers.  
> 
> AFAIR, this was done on purpose. In the aim of being as independent as
> possible from the earlier stages, we set here the full configuration of
> register 0xF2440208 "SoC Device Multiplex Register". Bits are either
> reserved or NAND controller related, nobody else is supposed to poke
> here. I'm not sure using regmap_update_bits() here is relevant?

Well, using regmap_update_bits() still makes you "independent from the
earlier stages", as long as you set/clear all the bits that you think
should be set/clear. I'm not sure it's very wise to ruthlessly
overwrite those reserved bits.

> > -		regmap_read(sysctrl_base, GENCONF_ND_CLK_CTRL, &reg);
> > -		reg |= GENCONF_ND_CLK_CTRL_EN;
> > -		regmap_write(sysctrl_base, GENCONF_ND_CLK_CTRL, reg);
> > +		mask = GENCONF_SOC_DEVICE_MUX_NFC_EN |
> > +			GENCONF_SOC_DEVICE_MUX_ECC_CLK_RST |
> > +			GENCONF_SOC_DEVICE_MUX_ECC_CORE_RST |
> > +			GENCONF_SOC_DEVICE_MUX_NFC_INT_EN;  
> 
> Minor nit: indentation looks wrong here?

This is what Emacs did, so it must be correct ? :-)

I'll fix up in v2, once we agree about the GENCONF_SOC_DEVICE_MUX
register situation.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] mtd: nand: use regmap_update_bits() in marvell_nand for syscon access

[Miquel Raynal]

Hi Thomas,

Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote on Wed, 1 Aug
2018 14:34:17 +0200:

> Hello,
> 
> On Wed, 1 Aug 2018 14:27:41 +0200, Miquel Raynal wrote:
> 
> > Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote on Wed,  1 Aug
> > 2018 12:25:05 +0200:
> >   
> > > The marvell_nfc_init() function fiddles with some bits of a system
> > > controller on Armada 7K/8K. However:
> > > 
> > >  - It does a regmap_write() on GENCONF_SOC_DEVICE_MUX, potentially
> > >    losing other bits that might have been set by other drivers.    
> > 
> > AFAIR, this was done on purpose. In the aim of being as independent as
> > possible from the earlier stages, we set here the full configuration of
> > register 0xF2440208 "SoC Device Multiplex Register". Bits are either
> > reserved or NAND controller related, nobody else is supposed to poke
> > here. I'm not sure using regmap_update_bits() here is relevant?  
> 
> Well, using regmap_update_bits() still makes you "independent from the
> earlier stages", as long as you set/clear all the bits that you think
> should be set/clear. I'm not sure it's very wise to ruthlessly
> overwrite those reserved bits.

I'm not sure my regmap knowledge is enough to understand the depth of
the above comment. This is what (I think) I know, please correct me:

Overwriting reserved bits (ie. writing 0 to them) is what we always do.
Even if reserved these bits are not in the regmap mask, as the regmap is
defined to do 32-bit access only, I suppose a regmap_update_bits() will
just read these bytes (all should be 0, tells the spec) then rewrite
them (still 0). Other fields being at this time initialized to 0 or 1.
So if we want to use regmap_update_bits() here then we should probably
mask all bits but the one reserved?

I'm really not against this change, but I don't think I get the
impact/difference of this change with "just writing" the register.

> 
> > > -		regmap_read(sysctrl_base, GENCONF_ND_CLK_CTRL, &reg);
> > > -		reg |= GENCONF_ND_CLK_CTRL_EN;
> > > -		regmap_write(sysctrl_base, GENCONF_ND_CLK_CTRL, reg);
> > > +		mask = GENCONF_SOC_DEVICE_MUX_NFC_EN |
> > > +			GENCONF_SOC_DEVICE_MUX_ECC_CLK_RST |
> > > +			GENCONF_SOC_DEVICE_MUX_ECC_CORE_RST |
> > > +			GENCONF_SOC_DEVICE_MUX_NFC_INT_EN;    
> > 
> > Minor nit: indentation looks wrong here?  
> 
> This is what Emacs did, so it must be correct ? :-)

Hehe, I do have the same setup. If you find out how to fix it, please
share :)

> 
> I'll fix up in v2, once we agree about the GENCONF_SOC_DEVICE_MUX
> register situation.
> 
> Thanks!
> 
> Thomas

Thanks,
Miquèl