diff mbox

[v2,6/6] ARM: davinci: da850: configure system configuration chip(CFGCHIP3) for emac

Message ID 1359380879-26306-7-git-send-email-prabhakar.lad@ti.com (mailing list archive)
State Changes Requested
Headers show

Commit Message

Lad, Prabhakar Jan. 28, 2013, 1:47 p.m. UTC 
From: Lad, Prabhakar <prabhakar.lad@ti.com>

The system configuration chip CFGCHIP3, controls the emac module.
This patch appropriately configures this register for emac and
sets DA850_MII_MDIO_CLKEN_PIN GPIO pin appropriately.

Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: davinci-linux-open-source@linux.davincidsp.com
Cc: netdev@vger.kernel.org
Cc: devicetree-discuss@lists.ozlabs.org
Cc: Sekhar Nori <nsekhar@ti.com>
Cc: Heiko Schocher <hs@denx.de>
---
 arch/arm/mach-davinci/da8xx-dt.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

Comments

Sekhar Nori Feb. 3, 2013, 12:45 p.m. UTC | #1 
On 1/28/2013 7:17 PM, Prabhakar Lad wrote:
> From: Lad, Prabhakar <prabhakar.lad@ti.com>
> 
> The system configuration chip CFGCHIP3, controls the emac module.
> This patch appropriately configures this register for emac and
> sets DA850_MII_MDIO_CLKEN_PIN GPIO pin appropriately.
> 
> Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Cc: davinci-linux-open-source@linux.davincidsp.com
> Cc: netdev@vger.kernel.org
> Cc: devicetree-discuss@lists.ozlabs.org
> Cc: Sekhar Nori <nsekhar@ti.com>
> Cc: Heiko Schocher <hs@denx.de>
> ---
>  arch/arm/mach-davinci/da8xx-dt.c |   28 ++++++++++++++++++++++++++++
>  1 files changed, 28 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c
> index e533a0a..4a096e3 100644
> --- a/arch/arm/mach-davinci/da8xx-dt.c
> +++ b/arch/arm/mach-davinci/da8xx-dt.c
> @@ -8,6 +8,7 @@
>   * published by the Free Software Foundation.
>   */
>  #include <linux/io.h>
> +#include <linux/gpio.h>
>  #include <linux/of_irq.h>
>  #include <linux/of_platform.h>
>  #include <linux/irqdomain.h>
> @@ -39,6 +40,32 @@ static void __init da8xx_init_irq(void)
>  
>  #ifdef CONFIG_ARCH_DAVINCI_DA850
>  
> +static void __init da8xx_config_emac(void)
> +{
> +#define DA850_MII_MDIO_CLKEN_PIN	GPIO_TO_PIN(2, 6)
> +#define DA850_EMAC_MODE_SELECT		BIT(8)
> +	void __iomem *cfg_chip3_base;
> +	int ret;
> +	u32 val;
> +
> +	cfg_chip3_base = DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP3_REG);
> +
> +	val = __raw_readl(cfg_chip3_base);
> +	val &= ~DA850_EMAC_MODE_SELECT;
> +	/* configure the CFGCHIP3 register for MII */
> +	__raw_writel(val, cfg_chip3_base);

Use readl/writel instead.

> +	pr_info("EMAC: MII PHY configured\n");
> +
> +	ret = gpio_request(DA850_MII_MDIO_CLKEN_PIN, "mdio_clk_en");
> +	if (ret) {
> +		pr_warn("Cannot open GPIO %d\n",
> +					DA850_MII_MDIO_CLKEN_PIN);
> +		return;
> +	}
> +	/* Enable/Disable MII MDIO clock */
> +	gpio_direction_output(DA850_MII_MDIO_CLKEN_PIN, 0);
> +}
> +
>  struct of_dev_auxdata da8xx_auxdata[] __initdata = {
>  	OF_DEV_AUXDATA("ti,davinci_mdio", 0x01e24000, "davinci_mdio.0", NULL),
>  	OF_DEV_AUXDATA("ti,davinci-dm6467-emac", 0x01e20000, "davinci_emac.1",
> @@ -52,6 +79,7 @@ static void __init da850_init_machine(void)
>  			     da8xx_auxdata, NULL);
>  
>  	da8xx_uart_clk_enable();
> +	da8xx_config_emac();

There are couple of issues with this implementation.

1) da8xx_config_emac() is specific to DA850 EVM, but masquerades as
   generic for da8xx. Looks like you need two functions, one for soc
   specific configuration and one board specific.
2) da8xx_config_emac() goes through all the time, whether the
   particular board has emac module or not. Shouldn't
   da8xx_config_emac() check if emac is actually enabled in the passed
   dtb and only the do the configuration?
3) The function assumes mii is used always, you can use the rmii_en dt
   property to check if rmii/mii is enabled and configure the soc/board
   accordingly.
4) If the same function can work both for da850 and da830, then it can
   be implemented outside of CONFIG_ARCH_DAVINCI_DA850.

Thanks,
Sekhar
Lad, Prabhakar Feb. 4, 2013, 5:03 a.m. UTC | #2 
Sekhar ,

On Sun, Feb 3, 2013 at 6:15 PM, Sekhar Nori <nsekhar@ti.com> wrote:
> On 1/28/2013 7:17 PM, Prabhakar Lad wrote:
>> From: Lad, Prabhakar <prabhakar.lad@ti.com>
>>
>> The system configuration chip CFGCHIP3, controls the emac module.
>> This patch appropriately configures this register for emac and
>> sets DA850_MII_MDIO_CLKEN_PIN GPIO pin appropriately.
>>
>> Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: davinci-linux-open-source@linux.davincidsp.com
>> Cc: netdev@vger.kernel.org
>> Cc: devicetree-discuss@lists.ozlabs.org
>> Cc: Sekhar Nori <nsekhar@ti.com>
>> Cc: Heiko Schocher <hs@denx.de>
>> ---
>>  arch/arm/mach-davinci/da8xx-dt.c |   28 ++++++++++++++++++++++++++++
>>  1 files changed, 28 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c
>> index e533a0a..4a096e3 100644
>> --- a/arch/arm/mach-davinci/da8xx-dt.c
>> +++ b/arch/arm/mach-davinci/da8xx-dt.c
>> @@ -8,6 +8,7 @@
>>   * published by the Free Software Foundation.
>>   */
>>  #include <linux/io.h>
>> +#include <linux/gpio.h>
>>  #include <linux/of_irq.h>
>>  #include <linux/of_platform.h>
>>  #include <linux/irqdomain.h>
>> @@ -39,6 +40,32 @@ static void __init da8xx_init_irq(void)
>>
>>  #ifdef CONFIG_ARCH_DAVINCI_DA850
>>
>> +static void __init da8xx_config_emac(void)
>> +{
>> +#define DA850_MII_MDIO_CLKEN_PIN     GPIO_TO_PIN(2, 6)
>> +#define DA850_EMAC_MODE_SELECT               BIT(8)
>> +     void __iomem *cfg_chip3_base;
>> +     int ret;
>> +     u32 val;
>> +
>> +     cfg_chip3_base = DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP3_REG);
>> +
>> +     val = __raw_readl(cfg_chip3_base);
>> +     val &= ~DA850_EMAC_MODE_SELECT;
>> +     /* configure the CFGCHIP3 register for MII */
>> +     __raw_writel(val, cfg_chip3_base);
>
> Use readl/writel instead.
>
Ok.

>> +     pr_info("EMAC: MII PHY configured\n");
>> +
>> +     ret = gpio_request(DA850_MII_MDIO_CLKEN_PIN, "mdio_clk_en");
>> +     if (ret) {
>> +             pr_warn("Cannot open GPIO %d\n",
>> +                                     DA850_MII_MDIO_CLKEN_PIN);
>> +             return;
>> +     }
>> +     /* Enable/Disable MII MDIO clock */
>> +     gpio_direction_output(DA850_MII_MDIO_CLKEN_PIN, 0);
>> +}
>> +
>>  struct of_dev_auxdata da8xx_auxdata[] __initdata = {
>>       OF_DEV_AUXDATA("ti,davinci_mdio", 0x01e24000, "davinci_mdio.0", NULL),
>>       OF_DEV_AUXDATA("ti,davinci-dm6467-emac", 0x01e20000, "davinci_emac.1",
>> @@ -52,6 +79,7 @@ static void __init da850_init_machine(void)
>>                            da8xx_auxdata, NULL);
>>
>>       da8xx_uart_clk_enable();
>> +     da8xx_config_emac();
>
> There are couple of issues with this implementation.
>
> 1) da8xx_config_emac() is specific to DA850 EVM, but masquerades as
>    generic for da8xx. Looks like you need two functions, one for soc
>    specific configuration and one board specific.
Ok.

> 2) da8xx_config_emac() goes through all the time, whether the
>    particular board has emac module or not. Shouldn't
>    da8xx_config_emac() check if emac is actually enabled in the passed
>    dtb and only the do the configuration?

Ok. Can you give some pointers how we can access the dtb in board files.

> 3) The function assumes mii is used always, you can use the rmii_en dt
>    property to check if rmii/mii is enabled and configure the soc/board
>    accordingly.

Ok.

> 4) If the same function can work both for da850 and da830, then it can
>    be implemented outside of CONFIG_ARCH_DAVINCI_DA850.
>
Ok.

Regards,
--Prabhakar

> Thanks,
> Sekhar
Sekhar Nori Feb. 4, 2013, 5:28 a.m. UTC | #3 
On 2/3/2013 6:15 PM, Sekhar Nori wrote:
> On 1/28/2013 7:17 PM, Prabhakar Lad wrote:
>> From: Lad, Prabhakar <prabhakar.lad@ti.com>

> There are couple of issues with this implementation.
> 
> 1) da8xx_config_emac() is specific to DA850 EVM, but masquerades as
>    generic for da8xx. Looks like you need two functions, one for soc
>    specific configuration and one board specific.
> 2) da8xx_config_emac() goes through all the time, whether the
>    particular board has emac module or not. Shouldn't
>    da8xx_config_emac() check if emac is actually enabled in the passed
>    dtb and only the do the configuration?
> 3) The function assumes mii is used always, you can use the rmii_en dt
>    property to check if rmii/mii is enabled and configure the soc/board
>    accordingly.
> 4) If the same function can work both for da850 and da830, then it can
>    be implemented outside of CONFIG_ARCH_DAVINCI_DA850.

Another thing I forgot to mention:

5) This patch replicates a lot of code form board-da850-evm.c. This
needs to be avoided. May be create function to configure emac in
da850.c. This can then be called in board-da850-evm.c as well as in the
dt case.

Thanks,
Sekhar
Lad, Prabhakar March 8, 2013, 1:28 p.m. UTC | #4 
Sekhar,

On Mon, Feb 4, 2013 at 10:33 AM, Prabhakar Lad
<prabhakar.csengg@gmail.com> wrote:
> Sekhar ,
>
> On Sun, Feb 3, 2013 at 6:15 PM, Sekhar Nori <nsekhar@ti.com> wrote:
>> On 1/28/2013 7:17 PM, Prabhakar Lad wrote:
>>> From: Lad, Prabhakar <prabhakar.lad@ti.com>
>>>
>>> The system configuration chip CFGCHIP3, controls the emac module.
>>> This patch appropriately configures this register for emac and
>>> sets DA850_MII_MDIO_CLKEN_PIN GPIO pin appropriately.
>>>
>>> Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
>>> Cc: linux-arm-kernel@lists.infradead.org
>>> Cc: linux-kernel@vger.kernel.org
>>> Cc: davinci-linux-open-source@linux.davincidsp.com
>>> Cc: netdev@vger.kernel.org
>>> Cc: devicetree-discuss@lists.ozlabs.org
>>> Cc: Sekhar Nori <nsekhar@ti.com>
>>> Cc: Heiko Schocher <hs@denx.de>
>>> ---
>>>  arch/arm/mach-davinci/da8xx-dt.c |   28 ++++++++++++++++++++++++++++
>>>  1 files changed, 28 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c
>>> index e533a0a..4a096e3 100644
>>> --- a/arch/arm/mach-davinci/da8xx-dt.c
>>> +++ b/arch/arm/mach-davinci/da8xx-dt.c
>>> @@ -8,6 +8,7 @@
>>>   * published by the Free Software Foundation.
>>>   */
>>>  #include <linux/io.h>
>>> +#include <linux/gpio.h>
>>>  #include <linux/of_irq.h>
>>>  #include <linux/of_platform.h>
>>>  #include <linux/irqdomain.h>
>>> @@ -39,6 +40,32 @@ static void __init da8xx_init_irq(void)
>>>
>>>  #ifdef CONFIG_ARCH_DAVINCI_DA850
>>>
>>> +static void __init da8xx_config_emac(void)
>>> +{
>>> +#define DA850_MII_MDIO_CLKEN_PIN     GPIO_TO_PIN(2, 6)
>>> +#define DA850_EMAC_MODE_SELECT               BIT(8)
>>> +     void __iomem *cfg_chip3_base;
>>> +     int ret;
>>> +     u32 val;
>>> +
>>> +     cfg_chip3_base = DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP3_REG);
>>> +
>>> +     val = __raw_readl(cfg_chip3_base);
>>> +     val &= ~DA850_EMAC_MODE_SELECT;
>>> +     /* configure the CFGCHIP3 register for MII */
>>> +     __raw_writel(val, cfg_chip3_base);
>>
>> Use readl/writel instead.
>>
> Ok.
>
>>> +     pr_info("EMAC: MII PHY configured\n");
>>> +
>>> +     ret = gpio_request(DA850_MII_MDIO_CLKEN_PIN, "mdio_clk_en");
>>> +     if (ret) {
>>> +             pr_warn("Cannot open GPIO %d\n",
>>> +                                     DA850_MII_MDIO_CLKEN_PIN);
>>> +             return;
>>> +     }
>>> +     /* Enable/Disable MII MDIO clock */
>>> +     gpio_direction_output(DA850_MII_MDIO_CLKEN_PIN, 0);
>>> +}
>>> +
>>>  struct of_dev_auxdata da8xx_auxdata[] __initdata = {
>>>       OF_DEV_AUXDATA("ti,davinci_mdio", 0x01e24000, "davinci_mdio.0", NULL),
>>>       OF_DEV_AUXDATA("ti,davinci-dm6467-emac", 0x01e20000, "davinci_emac.1",
>>> @@ -52,6 +79,7 @@ static void __init da850_init_machine(void)
>>>                            da8xx_auxdata, NULL);
>>>
>>>       da8xx_uart_clk_enable();
>>> +     da8xx_config_emac();
>>
>> There are couple of issues with this implementation.
>>
>> 1) da8xx_config_emac() is specific to DA850 EVM, but masquerades as
>>    generic for da8xx. Looks like you need two functions, one for soc
>>    specific configuration and one board specific.
> Ok.
>
>> 2) da8xx_config_emac() goes through all the time, whether the
>>    particular board has emac module or not. Shouldn't
>>    da8xx_config_emac() check if emac is actually enabled in the passed
>>    dtb and only the do the configuration?
>
> Ok. Can you give some pointers how we can access the dtb in board files.
>
Any pointers on this could be helpful.

Regards,
--Prabhakar

>> 3) The function assumes mii is used always, you can use the rmii_en dt
>>    property to check if rmii/mii is enabled and configure the soc/board
>>    accordingly.
>
> Ok.
>
>> 4) If the same function can work both for da850 and da830, then it can
>>    be implemented outside of CONFIG_ARCH_DAVINCI_DA850.
>>
> Ok.
>
> Regards,
> --Prabhakar
>
>> Thanks,
>> Sekhar
diff mbox

Patch

diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c
index e533a0a..4a096e3 100644
--- a/arch/arm/mach-davinci/da8xx-dt.c
+++ b/arch/arm/mach-davinci/da8xx-dt.c
@@ -8,6 +8,7 @@ 
  * published by the Free Software Foundation.
  */
 #include <linux/io.h>
+#include <linux/gpio.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/irqdomain.h>
@@ -39,6 +40,32 @@  static void __init da8xx_init_irq(void)
 
 #ifdef CONFIG_ARCH_DAVINCI_DA850
 
+static void __init da8xx_config_emac(void)
+{
+#define DA850_MII_MDIO_CLKEN_PIN	GPIO_TO_PIN(2, 6)
+#define DA850_EMAC_MODE_SELECT		BIT(8)
+	void __iomem *cfg_chip3_base;
+	int ret;
+	u32 val;
+
+	cfg_chip3_base = DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP3_REG);
+
+	val = __raw_readl(cfg_chip3_base);
+	val &= ~DA850_EMAC_MODE_SELECT;
+	/* configure the CFGCHIP3 register for MII */
+	__raw_writel(val, cfg_chip3_base);
+	pr_info("EMAC: MII PHY configured\n");
+
+	ret = gpio_request(DA850_MII_MDIO_CLKEN_PIN, "mdio_clk_en");
+	if (ret) {
+		pr_warn("Cannot open GPIO %d\n",
+					DA850_MII_MDIO_CLKEN_PIN);
+		return;
+	}
+	/* Enable/Disable MII MDIO clock */
+	gpio_direction_output(DA850_MII_MDIO_CLKEN_PIN, 0);
+}
+
 struct of_dev_auxdata da8xx_auxdata[] __initdata = {
 	OF_DEV_AUXDATA("ti,davinci_mdio", 0x01e24000, "davinci_mdio.0", NULL),
 	OF_DEV_AUXDATA("ti,davinci-dm6467-emac", 0x01e20000, "davinci_emac.1",
@@ -52,6 +79,7 @@  static void __init da850_init_machine(void)
 			     da8xx_auxdata, NULL);
 
 	da8xx_uart_clk_enable();
+	da8xx_config_emac();
 }
 
 static const char *da850_boards_compat[] __initdata = {