[U-Boot] [PATCH 1/2] v2 i.MX28: Fix ref_cpu clock setup

[U-Boot] [PATCH 1/2] v2 i.MX28: Fix ref_cpu clock setup

[Marek Vasut]

> Hi,
> 
> This patch fixes ref_cpu clock setup. This bug leads to a hanging board
> after rebooting from the Kernel, due to failing memory size detection:
> U-Boot 2011.12-svn342 (Feb 02 2012 - 17:20:00)
> 
> Freescale i.MX28 family
> I2C:   ready
> DRAM:  0 Bytes
> 
> The cause of the bug is register hw_clkctrl_frac0 being accessed as
> a 32-bit long, whereas the manual specifically states it can be accessed
> as bytes only.
> 
> This patches introduces an 8-bit wide register type, mx28_register_8.
> The already existing mx28_register has been renamed mx28_register_32.
> 
> With this patch, U-Boot no longer hangs after an i.mx28 based board
> was reset from the Kernel.
> 
> (PS: I hope this email is properly formatted now, after fight our exchange
> server for a whole morning and loosing in the end)
> 
> Signed-off-by: Robert Delien <robert@delien.nl>
> ---
>  arch/arm/cpu/arm926ejs/mx28/clock.c           |   74 +++-----
>  arch/arm/cpu/arm926ejs/mx28/iomux.c           |    6 +-
>  arch/arm/cpu/arm926ejs/mx28/mx28.c            |    6 +-
>  arch/arm/cpu/arm926ejs/mx28/spl_mem_init.c    |   23 +--
>  arch/arm/include/asm/arch-mx28/regs-apbh.h    |  254
> ++++++++++++------------ arch/arm/include/asm/arch-mx28/regs-bch.h     |  
> 42 ++--
>  arch/arm/include/asm/arch-mx28/regs-clkctrl.h |  101 +++++------
>  arch/arm/include/asm/arch-mx28/regs-common.h  |   28 ++-
>  arch/arm/include/asm/arch-mx28/regs-gpmi.h    |   26 ++--
>  arch/arm/include/asm/arch-mx28/regs-i2c.h     |   28 ++--
>  arch/arm/include/asm/arch-mx28/regs-ocotp.h   |   86 ++++----
>  arch/arm/include/asm/arch-mx28/regs-pinctrl.h |  168 ++++++++--------
>  arch/arm/include/asm/arch-mx28/regs-power.h   |   28 ++--
>  arch/arm/include/asm/arch-mx28/regs-rtc.h     |   28 ++--
>  arch/arm/include/asm/arch-mx28/regs-ssp.h     |   40 ++--
>  arch/arm/include/asm/arch-mx28/regs-timrot.h  |   38 ++--
>  arch/arm/include/asm/arch-mx28/regs-usbphy.h  |   20 +-
>  arch/arm/include/asm/arch-mx28/sys_proto.h    |    6 +-
>  drivers/gpio/mxs_gpio.c                       |   16 +-
>  drivers/usb/host/ehci-mxs.c                   |    8 +-
>  20 files changed, 505 insertions(+), 521 deletions(-)
> 
> diff --git a/arch/arm/cpu/arm926ejs/mx28/clock.c
> b/arch/arm/cpu/arm926ejs/mx28/clock.c index f698506..c0eea9e 100644
> --- a/arch/arm/cpu/arm926ejs/mx28/clock.c
> +++ b/arch/arm/cpu/arm926ejs/mx28/clock.c
> @@ -46,8 +46,8 @@ static uint32_t mx28_get_pclk(void)
>  	struct mx28_clkctrl_regs *clkctrl_regs =
>  		(struct mx28_clkctrl_regs *)MXS_CLKCTRL_BASE;
> 
> -	uint32_t clkctrl, clkseq, clkfrac;
> -	uint32_t frac, div;
> +	uint32_t clkctrl, clkseq, div;
> +	uint8_t clkfrac, frac;
> 
>  	clkctrl = readl(&clkctrl_regs->hw_clkctrl_cpu);
> 
> @@ -67,8 +67,8 @@ static uint32_t mx28_get_pclk(void)
>  	}
> 
>  	/* REF Path */
> -	clkfrac = readl(&clkctrl_regs->hw_clkctrl_frac0);
> -	frac = clkfrac & CLKCTRL_FRAC0_CPUFRAC_MASK;
> +	clkfrac = readb(&clkctrl_regs->hw_clkctrl_frac0[CLKCTRL_FRAC0_CPU]);
> +	frac = clkfrac & CLKCTRL_FRAC0_FRAC_MASK;
>  	div = clkctrl & CLKCTRL_CPU_DIV_CPU_MASK;
>  	return (PLL_FREQ_MHZ * PLL_FREQ_COEF / frac) / div;
>  }
> @@ -96,8 +96,8 @@ static uint32_t mx28_get_emiclk(void)
>  	struct mx28_clkctrl_regs *clkctrl_regs =
>  		(struct mx28_clkctrl_regs *)MXS_CLKCTRL_BASE;
> 
> -	uint32_t frac, div;
> -	uint32_t clkctrl, clkseq, clkfrac;
> +	uint32_t clkctrl, clkseq, div;
> +	uint8_t clkfrac, frac;
> 
>  	clkseq = readl(&clkctrl_regs->hw_clkctrl_clkseq);
>  	clkctrl = readl(&clkctrl_regs->hw_clkctrl_emi);
> @@ -109,11 +109,9 @@ static uint32_t mx28_get_emiclk(void)
>  		return XTAL_FREQ_MHZ / div;
>  	}
> 
> -	clkfrac = readl(&clkctrl_regs->hw_clkctrl_frac0);
> -
>  	/* REF Path */
> -	frac = (clkfrac & CLKCTRL_FRAC0_EMIFRAC_MASK) >>
> -		CLKCTRL_FRAC0_EMIFRAC_OFFSET;
> +	clkfrac = readb(&clkctrl_regs->hw_clkctrl_frac0[CLKCTRL_FRAC0_EMI]);
> +	frac = clkfrac & CLKCTRL_FRAC0_FRAC_MASK;
>  	div = clkctrl & CLKCTRL_EMI_DIV_EMI_MASK;
>  	return (PLL_FREQ_MHZ * PLL_FREQ_COEF / frac) / div;
>  }
> @@ -123,8 +121,8 @@ static uint32_t mx28_get_gpmiclk(void)
>  	struct mx28_clkctrl_regs *clkctrl_regs =
>  		(struct mx28_clkctrl_regs *)MXS_CLKCTRL_BASE;
> 
> -	uint32_t frac, div;
> -	uint32_t clkctrl, clkseq, clkfrac;
> +	uint32_t clkctrl, clkseq, div;
> +	uint8_t clkfrac, frac;
> 
>  	clkseq = readl(&clkctrl_regs->hw_clkctrl_clkseq);
>  	clkctrl = readl(&clkctrl_regs->hw_clkctrl_gpmi);
> @@ -135,11 +133,9 @@ static uint32_t mx28_get_gpmiclk(void)
>  		return XTAL_FREQ_MHZ / div;
>  	}
> 
> -	clkfrac = readl(&clkctrl_regs->hw_clkctrl_frac1);
> -
>  	/* REF Path */
> -	frac = (clkfrac & CLKCTRL_FRAC1_GPMIFRAC_MASK) >>
> -		CLKCTRL_FRAC1_GPMIFRAC_OFFSET;
> +	clkfrac = readb(&clkctrl_regs->hw_clkctrl_frac1[CLKCTRL_FRAC1_GPMI]);
> +	frac = clkfrac & CLKCTRL_FRAC1_FRAC_MASK;
>  	div = clkctrl & CLKCTRL_GPMI_DIV_MASK;
>  	return (PLL_FREQ_MHZ * PLL_FREQ_COEF / frac) / div;
>  }
> @@ -152,11 +148,12 @@ void mx28_set_ioclk(enum mxs_ioclock io, uint32_t
> freq) struct mx28_clkctrl_regs *clkctrl_regs =
>  		(struct mx28_clkctrl_regs *)MXS_CLKCTRL_BASE;
>  	uint32_t div;
> +	int io_reg;
> 
>  	if (freq == 0)
>  		return;
> 
> -	if (io > MXC_IOCLK1)
> +	if ((io < MXC_IOCLK0) || (io > MXC_IOCLK1))
>  		return;
> 
>  	div = (PLL_FREQ_KHZ * PLL_FREQ_COEF) / freq;
> @@ -167,23 +164,13 @@ void mx28_set_ioclk(enum mxs_ioclock io, uint32_t
> freq) if (div > 35)
>  		div = 35;
> 
> -	if (io == MXC_IOCLK0) {
> -		writel(CLKCTRL_FRAC0_CLKGATEIO0,
> -			&clkctrl_regs->hw_clkctrl_frac0_set);
> -		clrsetbits_le32(&clkctrl_regs->hw_clkctrl_frac0,
> -				CLKCTRL_FRAC0_IO0FRAC_MASK,
> -				div << CLKCTRL_FRAC0_IO0FRAC_OFFSET);
> -		writel(CLKCTRL_FRAC0_CLKGATEIO0,
> -			&clkctrl_regs->hw_clkctrl_frac0_clr);
> -	} else {
> -		writel(CLKCTRL_FRAC0_CLKGATEIO1,
> -			&clkctrl_regs->hw_clkctrl_frac0_set);
> -		clrsetbits_le32(&clkctrl_regs->hw_clkctrl_frac0,
> -				CLKCTRL_FRAC0_IO1FRAC_MASK,
> -				div << CLKCTRL_FRAC0_IO1FRAC_OFFSET);
> -		writel(CLKCTRL_FRAC0_CLKGATEIO1,
> -			&clkctrl_regs->hw_clkctrl_frac0_clr);
> -	}

I think you're mixing two things together above. This patch and some kind of a 
cleanup. But ok, thinking of this, it seems context related.

> +	io_reg = CLKCTRL_FRAC0_IO0 - (io - MXC_IOCLK0);

Uh ... ioreg = (io == MXC_IOCLK0) ? something : another; or stuff like that 
might be better. The math above is really confusing. Or you can even enumerate 
enum mxs_ioclock so that you won't need this math at all, which is even better.

> +	writeb(CLKCTRL_FRAC0_CLKGATE,
> +		&clkctrl_regs->hw_clkctrl_frac0_set[io_reg]);
> +	writeb(CLKCTRL_FRAC0_CLKGATE | (div & CLKCTRL_FRAC0_FRAC_MASK),
> +		&clkctrl_regs->hw_clkctrl_frac0[io_reg]);
> +	writeb(CLKCTRL_FRAC0_CLKGATE,
> +		&clkctrl_regs->hw_clkctrl_frac0_clr[io_reg]);
>  }
> 
>  /*
> @@ -193,19 +180,16 @@ static uint32_t mx28_get_ioclk(enum mxs_ioclock io)
>  {
>  	struct mx28_clkctrl_regs *clkctrl_regs =
>  		(struct mx28_clkctrl_regs *)MXS_CLKCTRL_BASE;
> -	uint32_t tmp, ret;
> +	uint8_t ret;
> +	int io_reg;
> 
> -	if (io > MXC_IOCLK1)
> +	if ((io < MXC_IOCLK0) || (io > MXC_IOCLK1))
>  		return 0;
> 
> -	tmp = readl(&clkctrl_regs->hw_clkctrl_frac0);
> +	io_reg = CLKCTRL_FRAC0_IO0 - (io - MXC_IOCLK0);
> 
> -	if (io == MXC_IOCLK0)
> -		ret = (tmp & CLKCTRL_FRAC0_IO0FRAC_MASK) >>
> -			CLKCTRL_FRAC0_IO0FRAC_OFFSET;
> -	else
> -		ret = (tmp & CLKCTRL_FRAC0_IO1FRAC_MASK) >>
> -			CLKCTRL_FRAC0_IO1FRAC_OFFSET;
> +	ret = readb(&clkctrl_regs->hw_clkctrl_frac0[io_reg]) &
> +		CLKCTRL_FRAC0_FRAC_MASK;
> 
>  	return (PLL_FREQ_KHZ * PLL_FREQ_COEF) / ret;
>  }
> @@ -223,7 +207,7 @@ void mx28_set_sspclk(enum mxs_sspclock ssp, uint32_t
> freq, int xtal) return;
> 
>  	clkreg = (uint32_t)(&clkctrl_regs->hw_clkctrl_ssp0) +
> -			(ssp * sizeof(struct mx28_register));
> +			(ssp * sizeof(struct mx28_register_32));
> 
>  	clrbits_le32(clkreg, CLKCTRL_SSP_CLKGATE);
>  	while (readl(clkreg) & CLKCTRL_SSP_CLKGATE)
> @@ -272,7 +256,7 @@ static uint32_t mx28_get_sspclk(enum mxs_sspclock ssp)
>  		return XTAL_FREQ_KHZ;
> 
>  	clkreg = (uint32_t)(&clkctrl_regs->hw_clkctrl_ssp0) +
> -			(ssp * sizeof(struct mx28_register));
> +			(ssp * sizeof(struct mx28_register_32));
> 
>  	tmp = readl(clkreg) & CLKCTRL_SSP_DIV_MASK;
> 
> diff --git a/arch/arm/cpu/arm926ejs/mx28/iomux.c
> b/arch/arm/cpu/arm926ejs/mx28/iomux.c index 9ea411f..12916b6 100644
> --- a/arch/arm/cpu/arm926ejs/mx28/iomux.c
> +++ b/arch/arm/cpu/arm926ejs/mx28/iomux.c
> @@ -43,7 +43,7 @@ int mxs_iomux_setup_pad(iomux_cfg_t pad)
>  {
>  	u32 reg, ofs, bp, bm;
>  	void *iomux_base = (void *)MXS_PINCTRL_BASE;
> -	struct mx28_register *mxs_reg;
> +	struct mx28_register_32 *mxs_reg;
> 
>  	/* muxsel */
>  	ofs = 0x100;
> @@ -70,7 +70,7 @@ int mxs_iomux_setup_pad(iomux_cfg_t pad)
>  	/* vol */
>  	if (PAD_VOL_VALID(pad)) {
>  		bp = PAD_PIN(pad) % 8 * 4 + 2;
> -		mxs_reg = (struct mx28_register *)(iomux_base + ofs);
> +		mxs_reg = (struct mx28_register_32 *)(iomux_base + ofs);
>  		if (PAD_VOL(pad))
>  			writel(1 << bp, &mxs_reg->reg_set);
>  		else
> @@ -82,7 +82,7 @@ int mxs_iomux_setup_pad(iomux_cfg_t pad)
>  		ofs = PULL_OFFSET;
>  		ofs += PAD_BANK(pad) * 0x10;
>  		bp = PAD_PIN(pad);
> -		mxs_reg = (struct mx28_register *)(iomux_base + ofs);
> +		mxs_reg = (struct mx28_register_32 *)(iomux_base + ofs);
>  		if (PAD_PULL(pad))
>  			writel(1 << bp, &mxs_reg->reg_set);
>  		else
> diff --git a/arch/arm/cpu/arm926ejs/mx28/mx28.c
> b/arch/arm/cpu/arm926ejs/mx28/mx28.c index 683777f..0e69193 100644
> --- a/arch/arm/cpu/arm926ejs/mx28/mx28.c
> +++ b/arch/arm/cpu/arm926ejs/mx28/mx28.c
> @@ -63,7 +63,7 @@ void reset_cpu(ulong ignored)
>  		;
>  }
> 
> -int mx28_wait_mask_set(struct mx28_register *reg, uint32_t mask, int
> timeout) +int mx28_wait_mask_set(struct mx28_register_32 *reg, uint32_t


Can you actually separate out the rename to register_32 and then the fixup patch 
for register_8?

Rest seems ok

M

[U-Boot] [PATCH 1/2] v2 i.MX28: Fix ref_cpu clock setup

[Marek Vasut]

> Hi,
> 
> This patch fixes ref_cpu clock setup. This bug leads to a hanging board
> after rebooting from the Kernel, due to failing memory size detection:
> U-Boot 2011.12-svn342 (Feb 02 2012 - 17:20:00)
> 
> Freescale i.MX28 family
> I2C:   ready
> DRAM:  0 Bytes
> 
> The cause of the bug is register hw_clkctrl_frac0 being accessed as
> a 32-bit long, whereas the manual specifically states it can be accessed
> as bytes only.
> 
> This patches introduces an 8-bit wide register type, mx28_register_8.
> The already existing mx28_register has been renamed mx28_register_32.
> 
> With this patch, U-Boot no longer hangs after an i.mx28 based board
> was reset from the Kernel.
> 
> (PS: I hope this email is properly formatted now, after fight our exchange
> server for a whole morning and loosing in the end)
> 
btw 2/2 is missing?

M

[U-Boot] [PATCH 1/2] v2 i.MX28: Fix ref_cpu clock setup

[Robert Deliën]

Hi Marek,

> -     if (io == MXC_IOCLK0) {
> -             writel(CLKCTRL_FRAC0_CLKGATEIO0,
> -                     &clkctrl_regs->hw_clkctrl_frac0_set);
> -             clrsetbits_le32(&clkctrl_regs->hw_clkctrl_frac0,
> -                             CLKCTRL_FRAC0_IO0FRAC_MASK,
> -                             div << CLKCTRL_FRAC0_IO0FRAC_OFFSET);
> -             writel(CLKCTRL_FRAC0_CLKGATEIO0,
> -                     &clkctrl_regs->hw_clkctrl_frac0_clr);
> -     } else {
> -             writel(CLKCTRL_FRAC0_CLKGATEIO1,
> -                     &clkctrl_regs->hw_clkctrl_frac0_set);
> -             clrsetbits_le32(&clkctrl_regs->hw_clkctrl_frac0,
> -                             CLKCTRL_FRAC0_IO1FRAC_MASK,
> -                             div << CLKCTRL_FRAC0_IO1FRAC_OFFSET);
> -             writel(CLKCTRL_FRAC0_CLKGATEIO1,
> -                     &clkctrl_regs->hw_clkctrl_frac0_clr);
> -     }
>
> I think you're mixing two things together above. This patch and some kind of a
> cleanup. But ok, thinking of this, it seems context related.

Yeah, duplicate code didn't feel right. Besides, the code didn't check for values
larger than MXC_IOCLK0, which is does now.

> > +     io_reg = CLKCTRL_FRAC0_IO0 - (io - MXC_IOCLK0);
>
> Uh ... ioreg = (io == MXC_IOCLK0) ? something : another; or stuff like that
> might be better. The math above is really confusing. Or you can even enumerate
> enum mxs_ioclock so that you won't need this math at all, which is even better.

I came accross an enumerator in the code, but I found that too big of a change
to slip in, because it alters the external interface. But we're not done with the code
yet. Plenty of oppertunities left.

ioreg = (io == MXC_IOCLK0) is not the same. The actual value of
io != MXC_IOCLK0 it compiler implementation depending.

> Can you actually separate out the rename to register_32 and then the fixup patch
> for register_8?

I'd rather not: I'm down to my neck it work and I don't these two parts in my archive
so that means manual merging, verifying and for another two hours.

> Rest seems ok

Can you see if our server swapped tabs for spaces? It's beyond my grasp how M$
considers it a good idea to alter to contents of my email.

Cheers,

        Robert.

[U-Boot] [PATCH 1/2] v2 i.MX28: Fix ref_cpu clock setup

[Robert Deliën]

> btw 2/2 is missing?

Not missing, just not sent again; It hasn't been changed.

[U-Boot] [PATCH 1/2] v2 i.MX28: Fix ref_cpu clock setup

[Marek Vasut]

> Hi Marek,
> 
> > -     if (io == MXC_IOCLK0) {
> > -             writel(CLKCTRL_FRAC0_CLKGATEIO0,
> > -                     &clkctrl_regs->hw_clkctrl_frac0_set);
> > -             clrsetbits_le32(&clkctrl_regs->hw_clkctrl_frac0,
> > -                             CLKCTRL_FRAC0_IO0FRAC_MASK,
> > -                             div << CLKCTRL_FRAC0_IO0FRAC_OFFSET);
> > -             writel(CLKCTRL_FRAC0_CLKGATEIO0,
> > -                     &clkctrl_regs->hw_clkctrl_frac0_clr);
> > -     } else {
> > -             writel(CLKCTRL_FRAC0_CLKGATEIO1,
> > -                     &clkctrl_regs->hw_clkctrl_frac0_set);
> > -             clrsetbits_le32(&clkctrl_regs->hw_clkctrl_frac0,
> > -                             CLKCTRL_FRAC0_IO1FRAC_MASK,
> > -                             div << CLKCTRL_FRAC0_IO1FRAC_OFFSET);
> > -             writel(CLKCTRL_FRAC0_CLKGATEIO1,
> > -                     &clkctrl_regs->hw_clkctrl_frac0_clr);
> > -     }
> > 
> > I think you're mixing two things together above. This patch and some kind
> > of a cleanup. But ok, thinking of this, it seems context related.
> 
> Yeah, duplicate code didn't feel right. Besides, the code didn't check for
> values larger than MXC_IOCLK0, which is does now.
> 
> > > +     io_reg = CLKCTRL_FRAC0_IO0 - (io - MXC_IOCLK0);
> > 
> > Uh ... ioreg = (io == MXC_IOCLK0) ? something : another; or stuff like
> > that might be better. The math above is really confusing. Or you can
> > even enumerate enum mxs_ioclock so that you won't need this math at all,
> > which is even better.
> 
> I came accross an enumerator in the code, but I found that too big of a
> change to slip in, because it alters the external interface. But we're not
> done with the code yet. Plenty of oppertunities left.

Not really, fixing the enumeration should be fine I believe.
> 
> ioreg = (io == MXC_IOCLK0) is not the same. The actual value of
> io != MXC_IOCLK0 it compiler implementation depending.

That's why I put the ternary operator there ?
> 
> > Can you actually separate out the rename to register_32 and then the
> > fixup patch for register_8?
> 
> I'd rather not: I'm down to my neck it work and I don't these two parts in
> my archive so that means manual merging, verifying and for another two
> hours.

It's impossible to review like this though and much more prone to pull in bugs.
> 
> > Rest seems ok
> 
> Can you see if our server swapped tabs for spaces? It's beyond my grasp how
> M$ considers it a good idea to alter to contents of my email.

It didnt.
> 
> Cheers,
> 
>         Robert.

[U-Boot] [PATCH 1/2] v2 i.MX28: Fix ref_cpu clock setup

[Marek Vasut]

> > btw 2/2 is missing?
> 
> Not missing, just not sent again; It hasn't been changed.

Uh ... I don't see this patch in in-reply-to either. So I can't find it, really.

M

[U-Boot] [PATCH 1/2] v2 i.MX28: Fix ref_cpu clock setup

[Marek Vasut]

> Hi Marek,
> 
> > -     if (io == MXC_IOCLK0) {
> > -             writel(CLKCTRL_FRAC0_CLKGATEIO0,
> > -                     &clkctrl_regs->hw_clkctrl_frac0_set);
> > -             clrsetbits_le32(&clkctrl_regs->hw_clkctrl_frac0,
> > -                             CLKCTRL_FRAC0_IO0FRAC_MASK,
> > -                             div << CLKCTRL_FRAC0_IO0FRAC_OFFSET);
> > -             writel(CLKCTRL_FRAC0_CLKGATEIO0,
> > -                     &clkctrl_regs->hw_clkctrl_frac0_clr);
> > -     } else {
> > -             writel(CLKCTRL_FRAC0_CLKGATEIO1,
> > -                     &clkctrl_regs->hw_clkctrl_frac0_set);
> > -             clrsetbits_le32(&clkctrl_regs->hw_clkctrl_frac0,
> > -                             CLKCTRL_FRAC0_IO1FRAC_MASK,
> > -                             div << CLKCTRL_FRAC0_IO1FRAC_OFFSET);
> > -             writel(CLKCTRL_FRAC0_CLKGATEIO1,
> > -                     &clkctrl_regs->hw_clkctrl_frac0_clr);
> > -     }
> > 
> > I think you're mixing two things together above. This patch and some kind
> > of a cleanup. But ok, thinking of this, it seems context related.
> 
> Yeah, duplicate code didn't feel right. Besides, the code didn't check for
> values larger than MXC_IOCLK0, which is does now.
> 
> > > +     io_reg = CLKCTRL_FRAC0_IO0 - (io - MXC_IOCLK0);
> > 
> > Uh ... ioreg = (io == MXC_IOCLK0) ? something : another; or stuff like
> > that might be better. The math above is really confusing. Or you can
> > even enumerate enum mxs_ioclock so that you won't need this math at all,
> > which is even better.
> 
> I came accross an enumerator in the code, but I found that too big of a
> change to slip in, because it alters the external interface. But we're not
> done with the code yet. Plenty of oppertunities left.
> 
> ioreg = (io == MXC_IOCLK0) is not the same. The actual value of
> io != MXC_IOCLK0 it compiler implementation depending.
> 
> > Can you actually separate out the rename to register_32 and then the
> > fixup patch for register_8?
> 
> I'd rather not: I'm down to my neck it work and I don't these two parts in
> my archive so that means manual merging, verifying and for another two
> hours.
> 
> > Rest seems ok
> 
> Can you see if our server swapped tabs for spaces? It's beyond my grasp how
> M$ considers it a good idea to alter to contents of my email.
> 
> Cheers,
> 
>         Robert.

btw. a quick hint:

git reset HEAD^
git add -p

Add only the reg32 changes, commit, add the rest, commit. Send ;-)

M

[U-Boot] [PATCH 1/2] v2 i.MX28: Fix ref_cpu clock setup

[Robert Deliën]

Hi,

> That's why I put the ternary operator there ?

Ah; without anything behind it, it really just looked like a question mark.
But using a ternary caters only for a set of two. So enumeration would
be better ideed.

> It's impossible to review like this though and much more prone to pull in bugs.

Can you point me to a good and tutorial for GIT then? Just a brief description
of the work flow, from cloning the public archive, to submitting patches with
a couple of examples.

Now, for every bit of rework, I clone a new archive and manually
patch in all my changes to make a clean patch. That is two hours of work and
I'm not willing to spend that time on every remark.

Cheers,

        Robert.

[U-Boot] [PATCH 1/2] v2 i.MX28: Fix ref_cpu clock setup

[Marek Vasut]

> Hi,
> 
> > That's why I put the ternary operator there ?
> 
> Ah; without anything behind it, it really just looked like a question mark.
> But using a ternary caters only for a set of two. So enumeration would
> be better ideed.
> 
> > It's impossible to review like this though and much more prone to pull in
> > bugs.
> 
> Can you point me to a good and tutorial for GIT then? Just a brief
> description of the work flow, from cloning the public archive, to
> submitting patches with a couple of examples.

I just sent you an howto in a subsequent mail.
> 
> Now, for every bit of rework, I clone a new archive and manually
> patch in all my changes to make a clean patch.

No, it's really simple, see the email.

> That is two hours of work
> and I'm not willing to spend that time on every remark.

Yep ... basically, you need to learn:

git rebase -i (select patches you want to edit, reword ...)
git commit --amend (add stuff to top-of-head patch)

And the stuff I sent you -- "git reset" to reset changes from the index, git add 
-p to add changes selectively.

M

> 
> Cheers,
> 
>         Robert.

[U-Boot] [PATCH 1/2] v2 i.MX28: Fix ref_cpu clock setup

[Robert Deliën]

> btw. a quick hint:
> 
> git reset HEAD^
> git add -p
> 
> Add only the reg32 changes, commit, add the rest, commit. Send ;-)

Well, I made it a little more work than that. But who would have 
thought? GIT is actually growing on me!

I've got my changes in my repository now, in 4 separate commits. I've even
found an SMTP server on the network here. So we're looking good.

[U-Boot] [PATCH 1/2] v2 i.MX28: Fix ref_cpu clock setup

[Marek Vasut]

> > btw. a quick hint:
> > 
> > git reset HEAD^
> > git add -p
> > 
> > Add only the reg32 changes, commit, add the rest, commit. Send ;-)
> 
> Well, I made it a little more work than that. But who would have
> thought? GIT is actually growing on me!
> 
> I've got my changes in my repository now, in 4 separate commits. I've even
> found an SMTP server on the network here. So we're looking good.

Yes, finally run git format-patch -o somewhere and tools/checkpatch.pl on the 
patches, fix the remaining trouble and git send-email