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

[Miquel Raynal <miquel.raynal@bootlin.com>]

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