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 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