[U-Boot,2/2] i.MX6: mx6q_sabrelite: add SATA bindings

On 03/26/2012 01:35 AM, Stefano Babic wrote:
> On 26/03/2012 01:00, Eric Nelson wrote:
>> V2 has been stripped of the board-independent changes and
>> uses clrsetbits_le32() instead of twiddling bits by hand.
>>
>> Signed-off-by: Eric Nelson<eric.nelson@boundarydevices.com>
>> ---
>
> Hi Eric,
>
>>   board/freescale/mx6qsabrelite/mx6qsabrelite.c |   32 +++++++++++++++++++++++++
>>   include/configs/mx6qsabrelite.h               |   13 ++++++++++
>>   2 files changed, 45 insertions(+), 0 deletions(-)
>>
>> diff --git a/board/freescale/mx6qsabrelite/mx6qsabrelite.c b/board/freescale/mx6qsabrelite/mx6qsabrelite.c
>> index 1d09a72..afb1245 100644
>> --- a/board/freescale/mx6qsabrelite/mx6qsabrelite.c
>> +++ b/board/freescale/mx6qsabrelite/mx6qsabrelite.c
>> @@ -25,6 +25,8 @@
>>   #include<asm/arch/imx-regs.h>
>>   #include<asm/arch/mx6x_pins.h>
>>   #include<asm/arch/iomux-v3.h>
>> +#include<asm/arch/ccm_regs.h>
>> +#include<asm/arch/clock.h>
>>   #include<asm/errno.h>
>>   #include<asm/gpio.h>
>>   #include<mmc.h>
>> @@ -267,6 +269,32 @@ int board_eth_init(bd_t *bis)
>>   	return 0;
>>   }
>>
>> +#ifdef CONFIG_CMD_SATA
>> +
>> +int setup_sata(void)
>> +{
>> +	int rval = enable_sata_clock();
>
> What about to return at this point if there is an error ?
>

I'm not sure I understand. Do you mean re-structure the code with
two returns like this?

On 26/03/2012 15:35, Eric Nelson wrote:
> On 03/26/2012 01:35 AM, Stefano Babic wrote:
>> On 26/03/2012 01:00, Eric Nelson wrote:
>>> V2 has been stripped of the board-independent changes and
>>> uses clrsetbits_le32() instead of twiddling bits by hand.
>>>
>>> Signed-off-by: Eric Nelson<eric.nelson@boundarydevices.com>
>>> ---
>>
>> Hi Eric,
>>
>>>   board/freescale/mx6qsabrelite/mx6qsabrelite.c |   32
>>> +++++++++++++++++++++++++
>>>   include/configs/mx6qsabrelite.h               |   13 ++++++++++
>>>   2 files changed, 45 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/board/freescale/mx6qsabrelite/mx6qsabrelite.c
>>> b/board/freescale/mx6qsabrelite/mx6qsabrelite.c
>>> index 1d09a72..afb1245 100644
>>> --- a/board/freescale/mx6qsabrelite/mx6qsabrelite.c
>>> +++ b/board/freescale/mx6qsabrelite/mx6qsabrelite.c
>>> @@ -25,6 +25,8 @@
>>>   #include<asm/arch/imx-regs.h>
>>>   #include<asm/arch/mx6x_pins.h>
>>>   #include<asm/arch/iomux-v3.h>
>>> +#include<asm/arch/ccm_regs.h>
>>> +#include<asm/arch/clock.h>
>>>   #include<asm/errno.h>
>>>   #include<asm/gpio.h>
>>>   #include<mmc.h>
>>> @@ -267,6 +269,32 @@ int board_eth_init(bd_t *bis)
>>>       return 0;
>>>   }
>>>
>>> +#ifdef CONFIG_CMD_SATA
>>> +
>>> +int setup_sata(void)
>>> +{
>>> +    int rval = enable_sata_clock();
>>
>> What about to return at this point if there is an error ?
>>
> 
> I'm not sure I understand. Do you mean re-structure the code with
> two returns like this?


No, much easier - I find the code is easy to understand if it looks like
if the function returns immediately in case of error.

	if (do_something())
		return ERROR;

	< code when no error happens>

Your  enable_sata_clock() return only -1 in case of error. You could
easy write:

	if (enable_sata_clock())
		return -1 (or better a value in errno.h)
        struct iomuxc_base_regs *const iomuxc_regs
                   = (struct iomuxc_base_regs *) IOMUXC_BASE_ADDR;
        clrsetbits_le32(&iomuxc_regs->gpr[13],
                       IOMUXC_GPR13_SATA_MASK,
                       IOMUXC_GPR13_SATA_PHY_8_RXEQ_3P0DB
                       |IOMUXC_GPR13_SATA_PHY_7_SATA2M
                       |IOMUXC_GPR13_SATA_SPEED_3G
                       |(3<<IOMUXC_GPR13_SATA_PHY_6_SHIFT)
                       |IOMUXC_GPR13_SATA_SATA_PHY_5_SS_DISABLED
                       |IOMUXC_GPR13_SATA_SATA_PHY_4_ATTEN_9_16
                       |IOMUXC_GPR13_SATA_PHY_3_TXBOOST_0P00_DB
                       |IOMUXC_GPR13_SATA_PHY_2_TX_1P104V
                       |IOMUXC_GPR13_SATA_PHY_1_SLOW);

	return 0;	


Best regards,
Stefano Babic

On 03/26/2012 06:42 AM, Stefano Babic wrote:
> On 26/03/2012 15:35, Eric Nelson wrote:
>> On 03/26/2012 01:35 AM, Stefano Babic wrote:
>>> On 26/03/2012 01:00, Eric Nelson wrote:
>>>> V2 has been stripped of the board-independent changes and
>>>>
 >>>> <snip>
 >>>>
>>>> +#ifdef CONFIG_CMD_SATA
>>>> +
>>>> +int setup_sata(void)
>>>> +{
>>>> +    int rval = enable_sata_clock();
>>>
>>> What about to return at this point if there is an error ?
>>>
>> I'm not sure I understand. Do you mean re-structure the code with
>> two returns like this?
>
> No, much easier - I find the code is easy to understand if it looks like
> if the function returns immediately in case of error.
>
> 	if (do_something())
> 		return ERROR;
>
> 	<  code when no error happens>
>
> Your  enable_sata_clock() return only -1 in case of error. You could
> easy write:
>
> 	if (enable_sata_clock())
> 		return -1 (or better a value in errno.h)
 >
The choice of error code is better made inside enable_sata_clock(),
although I'm not really sure what error code to choose.

The error occurs if the PLL fails to lock and would indicate that
something's horribly wrong.

I'm guessing -EIO is probably the right choice.

If you agree, I'll send V2 of "i.MX6: add enable_sata_clock()" and
V3 of "i.MX6: mx6q_sabrelite: add SATA bindings".

Please advise,


Eric

On 03/26/2012 09:59 AM, Eric Nelson wrote:
> On 03/26/2012 06:42 AM, Stefano Babic wrote:
>> On 26/03/2012 15:35, Eric Nelson wrote:
>>> On 03/26/2012 01:35 AM, Stefano Babic wrote:
>>>> On 26/03/2012 01:00, Eric Nelson wrote:
>>>>> V2 has been stripped of the board-independent changes and
>>>>>
>  >>>> <snip>
>  >>>>
>>>>> +#ifdef CONFIG_CMD_SATA
>>>>> +
>>>>> +int setup_sata(void)
>>>>> +{
>>>>> + int rval = enable_sata_clock();
>>>>
>>>> What about to return at this point if there is an error ?
>>>>
>>> I'm not sure I understand. Do you mean re-structure the code with
>>> two returns like this?
>>
>> No, much easier - I find the code is easy to understand if it looks like
>> if the function returns immediately in case of error.
>>
>> if (do_something())
>> return ERROR;
>>
>> < code when no error happens>
>>
>> Your enable_sata_clock() return only -1 in case of error. You could
>> easy write:
>>
>> if (enable_sata_clock())
>> return -1 (or better a value in errno.h)
>  >
> The choice of error code is better made inside enable_sata_clock(),
> although I'm not really sure what error code to choose.
>
> The error occurs if the PLL fails to lock and would indicate that
> something's horribly wrong.
>
> I'm guessing -EIO is probably the right choice.
>
> If you agree, I'll send V2 of "i.MX6: add enable_sata_clock()" and
> V3 of "i.MX6: mx6q_sabrelite: add SATA bindings".
>
Hi Stefano,

It looks like I was waiting for you and never did send V3 of this patch.

I'll forward one shortly.

Regards,


Eric