[U-Boot] [PATCH 09/14] sunxi: dram: Add a helper function 'mctl_get_number_of_lanes'

[Siarhei Siamashka]

On Mon, 21 Jul 2014 20:41:33 +0100
Ian Campbell <ijc at hellion.org.uk> wrote:

> On Fri, 2014-07-18 at 19:23 +0300, Siarhei Siamashka wrote:
> > It is going to be useful in more than one place.
> > 
> > Signed-off-by: Siarhei Siamashka <siarhei.siamashka at gmail.com>
> > ---
> >  arch/arm/cpu/armv7/sunxi/dram.c | 30 +++++++++++++++++++-----------
> >  1 file changed, 19 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arch/arm/cpu/armv7/sunxi/dram.c b/arch/arm/cpu/armv7/sunxi/dram.c
> > index 18a5c3b..49d1770 100644
> > --- a/arch/arm/cpu/armv7/sunxi/dram.c
> > +++ b/arch/arm/cpu/armv7/sunxi/dram.c
> > @@ -115,23 +115,31 @@ static void mctl_enable_dll0(u32 phase)
> >  	udelay(22);
> >  }
> >  
> > +/* Get the number of DDR byte lanes */
> > +static u32 mctl_get_number_of_lanes(void)
> > +{
> > +	struct sunxi_dram_reg *dram = (struct sunxi_dram_reg *)SUNXI_DRAMC_BASE;
> > +	switch (readl(&dram->dcr) & DRAM_DCR_BUS_WIDTH_MASK) {
> > +	case DRAM_DCR_BUS_WIDTH(DRAM_DCR_BUS_WIDTH_32BIT):
> > +		return 4;
> > +	case DRAM_DCR_BUS_WIDTH(DRAM_DCR_BUS_WIDTH_16BIT):
> > +		return 2;
> > +	default:
> > +		return 1;
> > +	}
> > +}
> > +
> >  /*
> >   * Note: This differs from pm/standby in that it checks the bus width
> >   */
> >  static void mctl_enable_dllx(u32 phase)
> >  {
> >  	struct sunxi_dram_reg *dram = (struct sunxi_dram_reg *)SUNXI_DRAMC_BASE;
> > -	u32 i, n, bus_width;
> > -
> > -	bus_width = readl(&dram->dcr);
> > +	u32 i, number_of_lanes;
> >  
> > -	if ((bus_width & DRAM_DCR_BUS_WIDTH_MASK) ==
> > -	    DRAM_DCR_BUS_WIDTH(DRAM_DCR_BUS_WIDTH_32BIT))
> > -		n = DRAM_DCR_NR_DLLCR_32BIT;
> > -	else
> > -		n = DRAM_DCR_NR_DLLCR_16BIT;
> 
> Either DRAM_DCR_NR_DLLCR_??BIT are obsolete now and should be removed or
> they should be be adjusted and used in the new function.
>
> ISTM they don't add much so removing would be fine by me.

Agreed, I also think that they are better to be dropped.

> > +	number_of_lanes = mctl_get_number_of_lanes();
> 
> There is a subtle functional change here since number_of_lanes can be 1
> whereas n could never have been 2. Is that intended/ok? Please mention
> in the commit message.

I tried to experiment with setting the 8-bit bus width and it is
semi-workable (single byte access is OK, but accessing more than
one byte is broken). This part of the patch looks like a forgotten
leftover of these experiments. But it clearly has no practical
value and we only normally deal with the 16-bit or 32-bit bus width.

The most correct way of handling this unexpected code branch would
be to panic. But that's an unnecessarily increase of the code size.
So I think that the best solution is just to keep the old code
logic (expect only 16-bit or 32-bit bus width and 2 or 4 lanes).

-- 
Best regards,
Siarhei Siamashka