[U-Boot] [PATCH 5/7] mx35: Fix clock dividers

[Benoît Thébaudeau]

Hi Stefano,

On Saturday, September 1, 2012 11:15:35 AM, Stefano Babic wrote:
> On 14/08/2012 22:33, Benoît Thébaudeau wrote:
> > The clock dividers that were used do not match at all the reference
> > manual. They
> > were either completely broken, or came from an early silicon
> > revision
> > incompatible with the current one.
> > 
> > Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau at advansee.com>
> > Cc: Stefano Babic <sbabic at denx.de>
> > ---
> >  .../arch/arm/cpu/arm1136/mx35/generic.c            |   48
> >  ++++++++------------
> >  .../arch/arm/include/asm/arch-mx35/crm_regs.h      |   42
> >  ++++++-----------
> >  2 files changed, 31 insertions(+), 59 deletions(-)
> > 
> > diff --git u-boot-4d3c95f.orig/arch/arm/cpu/arm1136/mx35/generic.c
> > u-boot-4d3c95f/arch/arm/cpu/arm1136/mx35/generic.c
> > index e369c86..4af052c 100644
> > --- u-boot-4d3c95f.orig/arch/arm/cpu/arm1136/mx35/generic.c
> > +++ u-boot-4d3c95f/arch/arm/cpu/arm1136/mx35/generic.c
> > @@ -171,17 +171,14 @@ static u32 get_ipg_per_clk(void)
> >  	u32 pdr4 = readl(&ccm->pdr4);
> >  	u32 div;
> >  	if (pdr0 & MXC_CCM_PDR0_PER_SEL) {
> > -		div = (CCM_GET_DIVIDER(pdr4,
> > -			MXC_CCM_PDR4_PER0_PRDF_MASK,
> > -			MXC_CCM_PDR4_PER0_PODF_OFFSET) + 1) *
> > -			(CCM_GET_DIVIDER(pdr4,
> 
> It seems also to me that the current code is wrong if
> MXC_CCM_PDR0_PER_SEL is set. Maybe it was never set. As I see in
> figure
> 5-4, the ipg_per_clk depends only on pdr[21:16]. No idea where the
> second multiplier comes.

It looks like the current code is based on a pre(PRDF)-/post(PODF)-divider
scheme. Perhaps the first silicon revision was different and incompatible, or it
was just a bug in the older revisions of the reference manual. The history of
the reference manual says that this figure and some CCM register descriptions
have been updated at some point. Anyway, Linux does like my patch.

> > +		div = CCM_GET_DIVIDER(pdr4,
> >  			MXC_CCM_PDR4_PER0_PODF_MASK,
> > -			MXC_CCM_PDR4_PER0_PODF_OFFSET) + 1);
> > +			MXC_CCM_PDR4_PER0_PODF_OFFSET) + 1;
> 
> The name remains quite confusing. In the manual is PER0_DIV, which is
> the meaning of PODF here ?

It the abbreviation FSL uses for post-dividers. If the pre-divider is merged
with the post-divider to form a single divider, the naming from the RM makes
more sense. Do you want a new version changing this naming?

> Anyway, the masks you set are correct, I agree.
> 
> >  	} else {
> >  		div = CCM_GET_DIVIDER(pdr0,
> >  			MXC_CCM_PDR0_PER_PODF_MASK,
> >  			MXC_CCM_PDR0_PER_PODF_OFFSET) + 1;
> > -		freq /= get_ahb_div(pdr0);
> > +		div *= get_ahb_div(pdr0);
> 
> This does not change the behavior.

It does: Dividing twice in a row affects rounding.

> >  	}
> >  	return freq / div;
> >  }
> > @@ -199,19 +196,16 @@ u32 imx_get_uartclk(void)
> >  		freq = decode_pll(readl(&ccm->ppctl),
> >  			CONFIG_MX35_HCLK_FREQ);
> >  	}
> > -	freq /= ((CCM_GET_DIVIDER(pdr4,
> > -			MXC_CCM_PDR4_UART_PRDF_MASK,
> > -			MXC_CCM_PDR4_UART_PRDF_OFFSET) + 1) *
> > -		(CCM_GET_DIVIDER(pdr4,
> > +	freq /= CCM_GET_DIVIDER(pdr4,
> >  			MXC_CCM_PDR4_UART_PODF_MASK,
> > -			MXC_CCM_PDR4_UART_PODF_OFFSET) + 1));
> > +			MXC_CCM_PDR4_UART_PODF_OFFSET) + 1;
> 
> This is also right. I am only asking myself why it works correctly
> now.

It currently works... or not depending on the divider settings selected by board
inits. As long as the bits in the current PRDF bit-fields are kept cleared
(luck...), it works.

> >  	return freq;
> >  }
> >  
> >  unsigned int mxc_get_main_clock(enum mxc_main_clock clk)
> >  {
> >  	u32 nfc_pdf, hsp_podf;
> > -	u32 pll, ret_val = 0, usb_prdf, usb_podf;
> > +	u32 pll, ret_val = 0, usb_podf;
> >  	struct ccm_regs *ccm =
> >  		(struct ccm_regs *)IMX_CCM_BASE;
> >  
> > @@ -255,8 +249,7 @@ unsigned int mxc_get_main_clock(enum
> > mxc_main_clock clk)
> >  		ret_val = pll / (nfc_pdf + 1);
> >  		break;
> >  	case USB_CLK:
> > -		usb_prdf = (reg4 >> 25) & 0x7;
> > -		usb_podf = (reg4 >> 22) & 0x7;
> > +		usb_podf = (reg4 >> 22) & 0x3F;
> 
> Agree again. The code seems generated from another manual. Maybe the
> USB_DIV field was split into two fields. With this in mind, there is
> no
> apparent error in current code, but it cannot be derived from the
> manual.

The resulting frequency is different here too if some PRDF bits are set.

> >  		break;
> >  	default:
> >  		printf("Unknown clock: %d\n", clk);
> > @@ -287,11 +280,10 @@ unsigned int mxc_get_peri_clock(enum
> > mxc_peri_clock clk)
> >  	case UART2_BAUD:
> >  	case UART3_BAUD:
> >  		clk_sel = mpdr3 & (1 << 14);
> > -		pre_pdf = (mpdr4 >> 13) & 0x7;
> > -		pdf = (mpdr4 >> 10) & 0x7;
> > +		pdf = (mpdr4 >> 10) & 0x3F;
> 
> Right !
> 
> 
> The main issue about this patch is that it does not fix one problem,
> but
> a lot of. Really it should be split into several patches.

Indeed, but these problems have a common root cause: wrong RM in some way.

> Anyway, I
> will
> make some tests on the mx35 boards I have - if I will not get
> problems,
> I will push it, hoping that someone else can make some further tests.

OK. I have also tested it on a board that I have here.

The best test would be to set some PRDF bits and to scope the resulting
frequency (e.g. with PER0_DIV and I²C SCL). I'm not sure I have already
performed this specific test. I'll do that when possible.

There is also stuff like AUTO_CON and AUTO_MUX_DIV that are now marked as
reserved in the RM. I've kept this code since it can not be harmful contrary to
the wrong decoding of the divider fields.

Best regards,
Benoît