| Message ID | 20180801102505.18383-1-thomas.petazzoni@bootlin.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Miquel Raynal |
| Headers | show |
| Series | mtd: nand: use regmap_update_bits() in marvell_nand for syscon access | expand |
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 |= GENCONF_CLK_GATING_CTRL_ND_GATE; > - regmap_write(sysctrl_base, GENCONF_CLK_GATING_CTRL, reg); > - > - regmap_read(sysctrl_base, GENCONF_ND_CLK_CTRL, ®); > - 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
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 |= 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
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 |= 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
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 |= GENCONF_CLK_GATING_CTRL_ND_GATE; - regmap_write(sysctrl_base, GENCONF_CLK_GATING_CTRL, reg); - - regmap_read(sysctrl_base, GENCONF_ND_CLK_CTRL, ®); - 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 */
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(-)