Re: [PATCH 3/4] davinci: da850: add support for SATA interface

[Sergei Shtylyov <sshtylyov@mvista.com>]

Hello.

On 24-03-2011 12:08, Nori, Sekhar wrote:

>>> diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
>>> index 68fe4c2..276199d 100644
>>> --- a/arch/arm/mach-davinci/da850.c
>>> +++ b/arch/arm/mach-davinci/da850.c
>>> @@ -373,6 +373,14 @@ static struct clk spi1_clk = {
>>>    	.flags		= DA850_CLK_ASYNC3,
>>>    };
>>>
>>> +static struct clk sata_clk = {
>>> +	.name		= "sata",
>>> +	.parent		=&pll0_sysclk2,
>>> +	.lpsc		= DA850_LPSC1_SATA,
>>> +	.gpsc		= 1,
>>> +	.flags		= PSC_FORCE,
>>> +};
>>> +
>>>    static struct clk_lookup da850_clks[] = {
>>>    	CLK(NULL,		"ref",		&ref_clk),
>>>    	CLK(NULL,		"pll0",		&pll0_clk),
>>> @@ -419,6 +427,7 @@ static struct clk_lookup da850_clks[] = {
>>>    	CLK(NULL,		"usb20",	&usb20_clk),
>>>    	CLK("spi_davinci.0",	NULL,		&spi0_clk),
>>>    	CLK("spi_davinci.1",	NULL,		&spi1_clk),
>>> +	CLK("ahci",		NULL,		&sata_clk),
>>>    	CLK(NULL,		NULL,		NULL),
>>>    };

>>      I'd put the above into a separate patch...

> Why should addition of clock data not be in the same patch
> as the one which adds platform resources etc? It is not a big
> deal to change, but I would like to know why you request this.

    I didn't request anything, I just said what I'd have done. :-)
I think modifying the DA8xx-common and DA850-specific files should better be 
done separately. Although in this case we're adding DA850 only device, so 
perhaps the added code in devices-da8xx.c should be enclosed into #ifdef?

>>> diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-davinci/devices-da8xx.c
>>> index 625d4b6..e061396 100644
>>> --- a/arch/arm/mach-davinci/devices-da8xx.c
>>> +++ b/arch/arm/mach-davinci/devices-da8xx.c
>> [...]
>>> @@ -834,3 +836,139 @@ int __init da8xx_register_spi(int instance, struct spi_board_info *info,
>> [...]
>>> +/* Supported DA850 SATA crystal frequencies */
>>> +#define KHZ_TO_HZ(freq) ((freq) * 1000)
>>> +static unsigned long da850_sata_xtal[] = {
>>> +	KHZ_TO_HZ(300000),
>>> +	KHZ_TO_HZ(250000),
>>> +	0,			/* Reserved */

>>      Why reserve a place for it at all?

> Because then this table maps one-to-one to the hardware
> defined table.

    Ah, sorry, have missed that...

>>> +		val = SATA_PHY_MPY(i + 1);
>>> +	}
>>> +
>> [...]

>>> diff --git a/arch/arm/mach-davinci/include/mach/da8xx.h b/arch/arm/mach-davinci/include/mach/da8xx.h
>>> index e4fc1af..aa6f08e 100644
>>> --- a/arch/arm/mach-davinci/include/mach/da8xx.h
>>> +++ b/arch/arm/mach-davinci/include/mach/da8xx.h
>> [...]
>>> @@ -65,6 +66,7 @@ extern unsigned int da850_max_speed;
>>>    #define DA8XX_GPIO_BASE		0x01e26000
>>>    #define DA8XX_PSC1_BASE		0x01e27000
>>>    #define DA8XX_LCD_CNTRL_BASE	0x01e13000
>>> +#define DA850_SATA_BASE		0x01e18000

>>      It's used only in devices-da8xx.c -- shouldn't it be declared there?

> Yes, will move. Base addresses for modules like LCD and MMCSD can be
> moved as well - should be subject of some future clean-up patch.

    Yes, maybe I'll submit one...

> Thanks,
> Sekhar

WBR, Sergei