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

[Stefano Babic <sbabic@denx.de>]

On 24/03/2012 22:01, Eric Nelson wrote:
> 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@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.

Well, it is not a case ;-). I scan regularly patchwork and I check for
not worked patches - however, if a patch is in "Changes requested", I
expect a new version and I do not care about it. Dirk's message let me
convince that the patch was not deeply reviewed and I checked it again.
So many thanks to Dirk.

> 
>>>> 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()'?

ok, agree.

> 
>>>> +
>>>> +    /* 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).

Then these three are superfluous, are not they ? You read the register,
and putting in "or" with IOMUXC_GPR13_CANx_STOP_REQ does nothing.

Why do you not use clrsetbits to change the sata bits ?

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

clrsetbits does exactly what you want, I think.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================