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

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

[Sekhar Nori]

Add support for SATA controller on the
DA850/OMAP-L138/AM18x devices.

The patch adds the necessary clocks, platform
resources and a routine to initialize the SATA
controller.

The PHY configuration in this patch is
courtesy the work done by Zegeye Alemu,
Swaminathan and Mansoor Ahamed from TI.

While testing this patch, enable port multiplier
support iff you are actually using one. The
reasons of this behaviour are discussed
here: http://patchwork.ozlabs.org/patch/78163/

Signed-off-by: Sekhar Nori <nsekhar@ti.com>
Cc: linux-ide@vger.kernel.org
---
 arch/arm/mach-davinci/da850.c              |    9 ++
 arch/arm/mach-davinci/devices-da8xx.c      |  138 ++++++++++++++++++++++++++++
 arch/arm/mach-davinci/include/mach/da8xx.h |    3 +
 3 files changed, 150 insertions(+), 0 deletions(-)

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),
 };
 
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
@@ -14,6 +14,8 @@
 #include <linux/platform_device.h>
 #include <linux/dma-mapping.h>
 #include <linux/serial_8250.h>
+#include <linux/ahci_platform.h>
+#include <linux/clk.h>
 
 #include <mach/cputype.h>
 #include <mach/common.h>
@@ -834,3 +836,139 @@ int __init da8xx_register_spi(int instance, struct spi_board_info *info,
 
 	return platform_device_register(&da8xx_spi_device[instance]);
 }
+
+static struct resource da850_sata_resources[] = {
+	{
+		.start	= DA850_SATA_BASE,
+		.end	= DA850_SATA_BASE + 0x1fff,
+		.flags	= IORESOURCE_MEM,
+	},
+	{
+		.start	= IRQ_DA850_SATAINT,
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+/* SATA PHY Control Register offset from AHCI base */
+#define SATA_P0PHYCR_REG	0x178
+
+#define SATA_PHY_MPY(x)		((x) << 0)
+#define SATA_PHY_LB(x)		((x) << 4)
+#define SATA_PHY_LOS(x)		((x) << 6)
+#define SATA_PHY_RXINVPAIR(x)	((x) << 7)
+#define SATA_PHY_RXTERM(x)	((x) << 8)
+#define SATA_PHY_RXCDR(x)	((x) << 10)
+#define SATA_PHY_RXEQ(x)	((x) << 13)
+#define SATA_PHY_TXINVPAIR(x)	((x) << 17)
+#define SATA_PHY_TXCM(x)	((x) << 18)
+#define SATA_PHY_TXSWING(x)	((x) << 19)
+#define SATA_PHY_TXDE(x)	((x) << 22)
+#define SATA_PHY_OVERRIDE(x)	((x) << 30)
+#define SATA_PHY_ENPLL(x)	((x) << 31)
+
+static struct clk *da850_sata_clk;
+static unsigned long da850_sata_refclkpn;
+
+/* 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 */
+	KHZ_TO_HZ(187500),
+	KHZ_TO_HZ(150000),
+	KHZ_TO_HZ(125000),
+	KHZ_TO_HZ(120000),
+	KHZ_TO_HZ(100000),
+	KHZ_TO_HZ(75000),
+	KHZ_TO_HZ(60000),
+};
+
+static int da850_sata_init(struct device *dev, void __iomem *addr)
+{
+	int i, ret;
+	unsigned int val;
+
+	da850_sata_clk = clk_get(dev, NULL);
+	if (IS_ERR(da850_sata_clk))
+		return PTR_ERR(da850_sata_clk);
+
+	ret = clk_enable(da850_sata_clk);
+	if (ret)
+		goto err0;
+
+	/* Enable SATA clock receiver */
+	val = __raw_readl(DA8XX_SYSCFG1_VIRT(DA8XX_PWRDN_REG));
+	val &= ~BIT(0);
+	__raw_writel(val, DA8XX_SYSCFG1_VIRT(DA8XX_PWRDN_REG));
+
+	/* Get the multiplier needed for 1.5GHz PLL output */
+	for (i = 0; i < ARRAY_SIZE(da850_sata_xtal); i++) {
+		if (da850_sata_xtal[i] == da850_sata_refclkpn)
+			break;
+	}
+
+	if (i == ARRAY_SIZE(da850_sata_xtal)) {
+		ret = -EINVAL;
+		goto err1;
+	} else {
+		val = SATA_PHY_MPY(i + 1);
+	}
+
+	val |= SATA_PHY_LB(0) |
+		SATA_PHY_LOS(1) |
+		SATA_PHY_RXINVPAIR(0) |
+		SATA_PHY_RXTERM(0) |
+		SATA_PHY_RXCDR(4) |
+		SATA_PHY_RXEQ(1) |
+		SATA_PHY_TXINVPAIR(0) |
+		SATA_PHY_TXCM(0) |
+		SATA_PHY_TXSWING(3) |
+		SATA_PHY_TXDE(0) |
+		SATA_PHY_OVERRIDE(0) |
+		SATA_PHY_ENPLL(1);
+
+	__raw_writel(val, addr + SATA_P0PHYCR_REG);
+
+	return 0;
+
+err1:
+	clk_disable(da850_sata_clk);
+err0:
+	clk_put(da850_sata_clk);
+	return ret;
+}
+
+static void da850_sata_exit(struct device *dev)
+{
+	clk_disable(da850_sata_clk);
+	clk_put(da850_sata_clk);
+}
+
+static struct ahci_platform_data da850_sata_pdata = {
+	.init	= da850_sata_init,
+	.exit	= da850_sata_exit,
+};
+
+static u64 da850_sata_dmamask = DMA_BIT_MASK(32);
+
+static struct platform_device da850_sata_device = {
+	.name	= "ahci",
+	.id	= -1,
+	.dev	= {
+		.platform_data		= &da850_sata_pdata,
+		.dma_mask		= &da850_sata_dmamask,
+		.coherent_dma_mask	= DMA_BIT_MASK(32),
+	},
+	.num_resources	= ARRAY_SIZE(da850_sata_resources),
+	.resource	= da850_sata_resources,
+};
+
+int __init da850_register_sata(unsigned long refclkpn)
+{
+	da850_sata_refclkpn = refclkpn;
+	if (!da850_sata_refclkpn)
+		return -EINVAL;
+
+	return platform_device_register(&da850_sata_device);
+}
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
@@ -57,6 +57,7 @@ extern unsigned int da850_max_speed;
 #define DA8XX_SYSCFG1_BASE	(IO_PHYS + 0x22C000)
 #define DA8XX_SYSCFG1_VIRT(x)	(da8xx_syscfg1_base + (x))
 #define DA8XX_DEEPSLEEP_REG	0x8
+#define DA8XX_PWRDN_REG		0x18
 
 #define DA8XX_PSC0_BASE		0x01c10000
 #define DA8XX_PLL0_BASE		0x01c11000
@@ -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
 #define DA8XX_PLL1_BASE		0x01e1a000
 #define DA8XX_MMCSD0_BASE	0x01c40000
 #define DA8XX_AEMIF_CS2_BASE	0x60000000
@@ -93,6 +95,7 @@ int da850_register_cpufreq(char *async_clk);
 int da8xx_register_cpuidle(void);
 void __iomem * __init da8xx_get_mem_ctlr(void);
 int da850_register_pm(struct platform_device *pdev);
+int __init da850_register_sata(unsigned long refclkpn);
 
 extern struct platform_device da8xx_serial_device;
 extern struct emac_platform_data da8xx_emac_pdata;
-- 
1.7.3.2

[PATCH 4/4] davinci: da850 evm: register SATA device

[Sekhar Nori]

Register the platform device for SATA interface
present on the DA850/OMAP-L138/AM18x EVM.

Signed-off-by: Sekhar Nori <nsekhar@ti.com>
Cc: linux-ide@vger.kernel.org
---
 arch/arm/mach-davinci/board-da850-evm.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
index a7b41bf..3d2c0d7 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -1117,6 +1117,8 @@ static __init int da850_evm_init_cpufreq(void)
 static __init int da850_evm_init_cpufreq(void) { return 0; }
 #endif
 
+#define DA850EVM_SATA_REFCLKPN_RATE	(100 * 1000 * 1000)
+
 static __init void da850_evm_init(void)
 {
 	int ret;
@@ -1237,6 +1239,11 @@ static __init void da850_evm_init(void)
 	if (ret)
 		pr_warning("da850_evm_init: spi 1 registration failed: %d\n",
 				ret);
+
+	ret = da850_register_sata(DA850EVM_SATA_REFCLKPN_RATE);
+	if (ret)
+		pr_warning("da850_evm_init: sata registration failed: %d\n",
+				ret);
 }
 
 #ifdef CONFIG_SERIAL_8250_CONSOLE
-- 
1.7.3.2

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

[Sergei Shtylyov]

Hello.

On 23-03-2011 14:32, Sekhar Nori wrote:

> Add support for SATA controller on the
> DA850/OMAP-L138/AM18x devices.

> The patch adds the necessary clocks, platform
> resources and a routine to initialize the SATA
> controller.

> The PHY configuration in this patch is
> courtesy the work done by Zegeye Alemu,
> Swaminathan and Mansoor Ahamed from TI.

> While testing this patch, enable port multiplier
> support iff you are actually using one. The
> reasons of this behaviour are discussed
> here: http://patchwork.ozlabs.org/patch/78163/

> Signed-off-by: Sekhar Nori<nsekhar@ti.com>
> Cc: linux-ide@vger.kernel.org
[...]

> 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...

> 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?

> +	KHZ_TO_HZ(187500),
> +	KHZ_TO_HZ(150000),
> +	KHZ_TO_HZ(125000),
> +	KHZ_TO_HZ(120000),
> +	KHZ_TO_HZ(100000),
> +	KHZ_TO_HZ(75000),
> +	KHZ_TO_HZ(60000),
> +};
> +
> +static int da850_sata_init(struct device *dev, void __iomem *addr)
> +{
> +	int i, ret;
> +	unsigned int val;
> +
> +	da850_sata_clk = clk_get(dev, NULL);
> +	if (IS_ERR(da850_sata_clk))
> +		return PTR_ERR(da850_sata_clk);
> +
> +	ret = clk_enable(da850_sata_clk);
> +	if (ret)
> +		goto err0;
> +
> +	/* Enable SATA clock receiver */
> +	val = __raw_readl(DA8XX_SYSCFG1_VIRT(DA8XX_PWRDN_REG));
> +	val&= ~BIT(0);
> +	__raw_writel(val, DA8XX_SYSCFG1_VIRT(DA8XX_PWRDN_REG));
> +
> +	/* Get the multiplier needed for 1.5GHz PLL output */
> +	for (i = 0; i<  ARRAY_SIZE(da850_sata_xtal); i++) {
> +		if (da850_sata_xtal[i] == da850_sata_refclkpn)
> +			break;
> +	}

    {} not needed.

> +
> +	if (i == ARRAY_SIZE(da850_sata_xtal)) {
> +		ret = -EINVAL;
> +		goto err1;
> +	} else {

    *else* not needed here, after *goto*.

> +		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?

WBR, Sergei

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

[Nori, Sekhar]

Hi Sergei,

On Wed, Mar 23, 2011 at 17:39:44, Sergei Shtylyov 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.

> 
> > 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. This in turn keeps the init code pretty
simple. Plus, if and when hardware adds support for a
crystal there, the changes to rest of the code are minimal.

> 
> > +	KHZ_TO_HZ(187500),
> > +	KHZ_TO_HZ(150000),
> > +	KHZ_TO_HZ(125000),
> > +	KHZ_TO_HZ(120000),
> > +	KHZ_TO_HZ(100000),
> > +	KHZ_TO_HZ(75000),
> > +	KHZ_TO_HZ(60000),
> > +};
> > +
> > +static int da850_sata_init(struct device *dev, void __iomem *addr)
> > +{
> > +	int i, ret;
> > +	unsigned int val;
> > +
> > +	da850_sata_clk = clk_get(dev, NULL);
> > +	if (IS_ERR(da850_sata_clk))
> > +		return PTR_ERR(da850_sata_clk);
> > +
> > +	ret = clk_enable(da850_sata_clk);
> > +	if (ret)
> > +		goto err0;
> > +
> > +	/* Enable SATA clock receiver */
> > +	val = __raw_readl(DA8XX_SYSCFG1_VIRT(DA8XX_PWRDN_REG));
> > +	val&= ~BIT(0);
> > +	__raw_writel(val, DA8XX_SYSCFG1_VIRT(DA8XX_PWRDN_REG));
> > +
> > +	/* Get the multiplier needed for 1.5GHz PLL output */
> > +	for (i = 0; i<  ARRAY_SIZE(da850_sata_xtal); i++) {
> > +		if (da850_sata_xtal[i] == da850_sata_refclkpn)
> > +			break;
> > +	}
> 
>     {} not needed.

Okay, will remove.

> 
> > +
> > +	if (i == ARRAY_SIZE(da850_sata_xtal)) {
> > +		ret = -EINVAL;
> > +		goto err1;
> > +	} else {
> 
>     *else* not needed here, after *goto*.

Yup, will fix.

> 
> > +		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.

Thanks,
Sekhar

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

[Sergei Shtylyov]

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

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

[Nori, Sekhar]

Hi Sergei,

On Thu, Mar 24, 2011 at 18:34:26, Sergei Shtylyov wrote:
> 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. :-)

Okay. I guess I will keep it as is.

> 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?

Good point. Will add the #ifdef.

> >>> 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 for the help!

Regards,
Sekhar

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

[Sergei Shtylyov]

Hello.

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. :-)

> Okay. I guess I will keep it as is.

>> 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?

> Good point. Will add the #ifdef.

    Or perhaps the device should just be placed in da850.c instead?

WBR, Sergei

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

[Nori, Sekhar]

Hi Sergei,

On Thu, Mar 24, 2011 at 23:31:57, Sergei Shtylyov wrote:
> Hello.
> 
> 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. :-)
> 
> > Okay. I guess I will keep it as is.
> 
> >> 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?
> 
> > Good point. Will add the #ifdef.
> 
>     Or perhaps the device should just be placed in da850.c instead?

Um, no. da850.c is currently free from any peripheral specific
functionality and would like to keep it that way. MMC/SD1 is
DA850 specific as well, and has been kept in devices-da8xx.c.
I think consolidating all peripheral specific code in devices-da8xx.c
is a better idea.

Thanks,
Sekhar