[U-Boot] [PATCH V2] i.MX6: mx6q_sabrelite: add SATA bindings

[Eric Nelson]

Hi Stefano,

On 03/24/2012 01:19 AM, stefano babic wrote:
> Am 24/03/2012 08:09, schrieb Dirk Behme:
>> Hi Stefano,
>
> Hi Dirk,
>
>> On 13.03.2012 17:59, Eric Nelson wrote:
>>> Signed-off-by: Eric Nelson<eric.nelson at boundarydevices.com>
>>
>> Should this patch go into your u-boot-imx.git -next?
>
> mmhh...in patchworks this is marked as "Changes requested" - I do not
> remember now why, but I have a couple of open questions rereading the
> patch...
>
Funny how that happens with time.

>>> diff --git a/board/freescale/mx6qsabrelite/mx6qsabrelite.c
>>> b/board/freescale/mx6qsabrelite/mx6qsabrelite.c
>>> index 1d09a72..5915159 100644
>>> --- a/board/freescale/mx6qsabrelite/mx6qsabrelite.c
>>> +++ b/board/freescale/mx6qsabrelite/mx6qsabrelite.c
>>>
 >>> <snip>
 >>>
>>> +    u32 reg = 0;
>>> +    s32 timeout = 100000;
>>> +    struct imx_ccm_reg *const imx_ccm
>>> +        = (struct imx_ccm_reg *) CCM_BASE_ADDR;
>>> +    struct iomuxc_base_regs *const iomuxc_regs
>>> +        = (struct iomuxc_base_regs *) IOMUXC_BASE_ADDR;
>>> +
>>> +    /* Enable sata clock */
>>> +    reg = readl(&imx_ccm->CCGR5); /* CCGR5 */
>>> +    reg |= MXC_CCM_CCGR5_CG2_MASK;
>>> +    writel(reg,&imx_ccm->CCGR5);
>>> +
>>> +    /* Enable PLLs */
>>> +    reg = readl(&imx_ccm->analog_pll_enet);
>>> +    reg&= ~BM_ANADIG_PLL_SYS_POWERDOWN;
>>> +    writel(reg,&imx_ccm->analog_pll_enet);
>>> +    reg |= BM_ANADIG_PLL_SYS_ENABLE;
>>> +    while (timeout--) {
>>> +        if (readl(&imx_ccm->analog_pll_enet)&   BM_ANADIG_PLL_SYS_LOCK)
>>> +            break;
>>> +    }
>>> +    if (timeout<= 0)
>>> +        return -1;
>>> +    reg&= ~BM_ANADIG_PLL_SYS_BYPASS;
>>> +    writel(reg,&imx_ccm->analog_pll_enet);
>>> +    reg |= BM_ANADIG_PLL_ENET_ENABLE_SATA;
>>> +    writel(reg,&imx_ccm->analog_pll_enet);
>
> Is it all this part really board specific or mx6 specific ? I would like
> to split this part into a common and a board specific parts, and making
> the common part available for other boards.
>

This is all generic MX6 SATA enablement and could be moved into
common code.

Is cpu/armv7/mx6/clock.c the right place?

How about routine named 'enable_sata_clock()'?

>>> +
>>> +    /* Enable sata phy */
>>> +    reg = readl(&iomuxc_regs->gpr[13])
>>> +&   (IOMUXC_GPR13_SDMA_STOP_REQ
>>> +           |IOMUXC_GPR13_CAN2_STOP_REQ
>>> +           |IOMUXC_GPR13_CAN1_STOP_REQ
>>> +           |IOMUXC_GPR13_ENET_STOP_REQ);
>
> Why do you need to touch CAN bits ?
>
Not touching them, just keeping them the way they are and
clearing out the rest (which are all SATA fields).

Perhaps this would be clearer:
	reg = readl(&iomuxc->gpr[13])
		& ~(IOMUXC_GPR13_SATA_PHY_8_MASK
		   |IOMUXC_GPR13_SATA_PHY_7_MASK
		   |IOMUXC_GPR13_SATA_PHY_6_MASK
                    |IOMUXC_GPR13_SATA_SPEED_MASK
		   |IOMUXC_GPR13_SATA_PHY_5_MASK
		   |IOMUXC_GPR13_SATA_PHY_4_MASK
		   |IOMUXC_GPR13_SATA_PHY_3_MASK
		   |IOMUXC_GPR13_SATA_PHY_2_MASK
		   |IOMUXC_GPR13_SATA_PHY_1_MASK);

Or better yet, maybe a macro in iomux-v3.h:

#define IOMUXC_GPR13_SATA_MASK (IOMUXC_GPR13_SATA_PHY_8_MASK \
				|IOMUXC_GPR13_SATA_PHY_7_MASK \
				|IOMUXC_GPR13_SATA_PHY_6_MASK \
                                 |IOMUXC_GPR13_SATA_SPEED_MASK \
				|IOMUXC_GPR13_SATA_PHY_5_MASK \
				|IOMUXC_GPR13_SATA_PHY_4_MASK \
				|IOMUXC_GPR13_SATA_PHY_3_MASK \
				|IOMUXC_GPR13_SATA_PHY_2_MASK \
				|IOMUXC_GPR13_SATA_PHY_1_MASK)

and this code in setup_sata():
	reg = readl(&iomuxc->gpr[13])
		& ~(IOMUXC_GPR13_SATA_MASK)