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

[Eric Nelson]

Hi Stefano,

On 03/25/2012 03:17 AM, Stefano Babic wrote:
> 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 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.
>
> 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.
>
Ack that 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>
>>>>>
>>>>> +    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.
>
Will do.

>>
>>>>> +
>>>>> +    /* 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 ?
>
There's a simple answer for this: I hadn't heard of 'clrsetbits' before.

>>
>> 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.
>
Ok. I'll address in a V2.