From mboxrd@z Thu Jan  1 00:00:00 1970
From: Nikita Kiryanov <nikita@compulab.co.il>
Date: Tue, 19 Aug 2014 17:51:13 +0300
Subject: [U-Boot] [PATCH V2 18/18] arm: mx6: cm_fx6: add sata support
In-Reply-To: <53EC60E8.1070204@compulab.co.il>
References: <1407690780-19645-1-git-send-email-nikita@compulab.co.il>
	<1407690780-19645-10-git-send-email-nikita@compulab.co.il>
	<53EC60E8.1070204@compulab.co.il>
Message-ID: <53F36461.8030305@compulab.co.il>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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