[U-Boot] [PATCH V2 18/18] arm: mx6: cm_fx6: add sata support

Nikita Kiryanov
2014-08-19T14:51:13Z

Hi Igor,

On 14/08/14 10:10, Igor Grinberg wrote:
>> +
>> +static void cm_fx6_sata_power(int on)
>
> Will it be/look better to use bool here?

This will create a situation where a bool is passed to a function which
takes an int (gpio_direction_output()), and that irks me a little. At
least treating ints as bools is a common C idiom; the inverse is not.

>
>> +{
>> + int i;
>> +
>> + if (!on) { /* tell the iSSD that the power will be removed */
>> + gpio_direction_output(CM_FX6_SATA_PWLOSS_INT, 1);
>> + mdelay(10);
>> + }
>> +
>> + for (i = 0; i < ARRAY_SIZE(cm_fx6_issd_gpios); i++) {
>> + gpio_direction_output(cm_fx6_issd_gpios[i], on);
>> + udelay(100);
>> + }
>> +
>> + if (!on) /* for compatibility lower the power loss interrupt */
>> + gpio_direction_output(CM_FX6_SATA_PWLOSS_INT, 0);
>> +}
>
> [...]
>
>> +static void cm_fx6_setup_issd(void)
>> +{
>> + SETUP_IOMUX_PADS(sata_pads);
>> + /* Make sure this gpio has logical 0 value */
>> + gpio_direction_output(CM_FX6_SATA_PWLOSS_INT, 0);
>> + udelay(100);
>> +
>> + cm_fx6_sata_power(0);
>> + mdelay(250);
>> + cm_fx6_sata_power(1);
>> +}
>> +
>> +#define CM_FX6_SATA_INIT_RETRIES 10
>> +int sata_initialize(void)
>> +{
>> + int err, i;
>> +
>> + cm_fx6_setup_issd();
>> + for (i = 0; i < CM_FX6_SATA_INIT_RETRIES; i++) {
>> + err = setup_sata();
>> + if (err) {
>> + printf("SATA setup failed: %d\n", err);
>> + return err;
>> + }
>> +
>> + udelay(100);
>> +
>> + err = __sata_initialize();
>> + if (!err)
>> + break;
>> +
>> + /* There is no device on the SATA port */
>> + if (sata_port_status(0, 0) == 0)
>> + break;
>> +
>> + /* There's a device, but link not established. Retry */
>> + }
>> +
>> + return err;
>> +}
>> +#else
>> +static void cm_fx6_setup_issd(void) {}
>
> Why do you need this one?
> It is only called from sata_initialize(), which is inside
> #ifdef CONFIG_DWC_AHSATA
> and is not available otherwise.
>

Yes you're right. This is a remnant of a past version.
Will remove.

>> +#endif
>> +
>> #ifdef CONFIG_SYS_I2C_MXC
>> #define I2C_PAD_CTRL (PAD_CTL_PUS_100K_UP | PAD_CTL_SPEED_MED | \
>> PAD_CTL_DSE_40ohm | PAD_CTL_HYS | \
>
> [...]
>
>> diff --git a/include/configs/cm_fx6.h b/include/configs/cm_fx6.h
>> index adfd55e..88925cd 100644
>> --- a/include/configs/cm_fx6.h
>> +++ b/include/configs/cm_fx6.h
>> @@ -132,6 +132,19 @@
>> "mmcboot=echo Booting from mmc ...; " \
>> "run mmcargs; " \
>> "run doboot\0" \
>> + "satadev=0\0" \
>> + "sataroot=/dev/sda2 rw rootwait\0" \
>> + "sataargs=setenv bootargs console=${console} " \
>> + "root=${sataroot} " \
>> + "${video}\0" \
>> + "loadsatabootscript=fatload sata ${satadev} ${loadaddr} ${bootscr}\0" \
>> + "satabootscript=echo Running bootscript from sata ...; " \
>> + "source ${loadaddr}\0" \
>> + "sataloadkernel=fatload sata ${satadev} ${loadaddr} ${kernel}\0" \
>> + "sataloadfdt=fatload sata ${satadev} ${fdtaddr} ${fdtfile}\0" \
>
> Can we switch to use load instead of explicit fatload?

Sure