All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition
@ 2013-07-16  9:19 Laurent Pinchart
  2013-07-16 12:27 ` Sergei Shtylyov
                   ` (15 more replies)
  0 siblings, 16 replies; 17+ messages in thread
From: Laurent Pinchart @ 2013-07-16  9:19 UTC (permalink / raw)
  To: linux-sh

The ethernet device is named r8a7740-gether, fix the clock definition
accordingly.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 arch/arm/mach-shmobile/clock-r8a7740.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-shmobile/clock-r8a7740.c b/arch/arm/mach-shmobile/clock-r8a7740.c
index de10fd7..e1e8710 100644
--- a/arch/arm/mach-shmobile/clock-r8a7740.c
+++ b/arch/arm/mach-shmobile/clock-r8a7740.c
@@ -595,7 +595,7 @@ static struct clk_lookup lookups[] = {
 	CLKDEV_DEV_ID("sh_mmcif",		&mstp_clks[MSTP312]),
 	CLKDEV_DEV_ID("e6bd0000.mmcif",         &mstp_clks[MSTP312]),
 	CLKDEV_DEV_ID("r8a7740-gether",		&mstp_clks[MSTP309]),
-	CLKDEV_DEV_ID("e9a00000.sh-eth",	&mstp_clks[MSTP309]),
+	CLKDEV_DEV_ID("e9a00000.r8a7740-gether", &mstp_clks[MSTP309]),
 	CLKDEV_DEV_ID("renesas_tpu_pwm",	&mstp_clks[MSTP304]),
 
 	CLKDEV_DEV_ID("sh_mobile_sdhi.2",	&mstp_clks[MSTP415]),
-- 
Regards,

Laurent Pinchart


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition
  2013-07-16  9:19 [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition Laurent Pinchart
@ 2013-07-16 12:27 ` Sergei Shtylyov
  2013-07-16 14:26 ` Sergei Shtylyov
                   ` (14 subsequent siblings)
  15 siblings, 0 replies; 17+ messages in thread
From: Sergei Shtylyov @ 2013-07-16 12:27 UTC (permalink / raw)
  To: linux-sh

Hello.

On 16-07-2013 13:19, Laurent Pinchart wrote:

> The ethernet device is named r8a7740-gether, fix the clock definition
> accordingly.

> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>   arch/arm/mach-shmobile/clock-r8a7740.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

> diff --git a/arch/arm/mach-shmobile/clock-r8a7740.c b/arch/arm/mach-shmobile/clock-r8a7740.c
> index de10fd7..e1e8710 100644
> --- a/arch/arm/mach-shmobile/clock-r8a7740.c
> +++ b/arch/arm/mach-shmobile/clock-r8a7740.c
> @@ -595,7 +595,7 @@ static struct clk_lookup lookups[] = {
>   	CLKDEV_DEV_ID("sh_mmcif",		&mstp_clks[MSTP312]),
>   	CLKDEV_DEV_ID("e6bd0000.mmcif",         &mstp_clks[MSTP312]),
>   	CLKDEV_DEV_ID("r8a7740-gether",		&mstp_clks[MSTP309]),
> -	CLKDEV_DEV_ID("e9a00000.sh-eth",	&mstp_clks[MSTP309]),
> +	CLKDEV_DEV_ID("e9a00000.r8a7740-gether", &mstp_clks[MSTP309]),

    Al Ethernet devices should be named "ethernet", according to ePAPR spec.

WBR, Sergei


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition
  2013-07-16  9:19 [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition Laurent Pinchart
  2013-07-16 12:27 ` Sergei Shtylyov
@ 2013-07-16 14:26 ` Sergei Shtylyov
  2013-07-17 10:47 ` Laurent Pinchart
                   ` (13 subsequent siblings)
  15 siblings, 0 replies; 17+ messages in thread
From: Sergei Shtylyov @ 2013-07-16 14:26 UTC (permalink / raw)
  To: linux-sh

Hello.

On 07/16/2013 04:27 PM, Sergei Shtylyov wrote:

>> The ethernet device is named r8a7740-gether, fix the clock definition
>> accordingly.

>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>> ---
>>   arch/arm/mach-shmobile/clock-r8a7740.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)

>> diff --git a/arch/arm/mach-shmobile/clock-r8a7740.c
>> b/arch/arm/mach-shmobile/clock-r8a7740.c
>> index de10fd7..e1e8710 100644
>> --- a/arch/arm/mach-shmobile/clock-r8a7740.c
>> +++ b/arch/arm/mach-shmobile/clock-r8a7740.c
>> @@ -595,7 +595,7 @@ static struct clk_lookup lookups[] = {
>>       CLKDEV_DEV_ID("sh_mmcif",        &mstp_clks[MSTP312]),
>>       CLKDEV_DEV_ID("e6bd0000.mmcif",         &mstp_clks[MSTP312]),
>>       CLKDEV_DEV_ID("r8a7740-gether",        &mstp_clks[MSTP309]),
>> -    CLKDEV_DEV_ID("e9a00000.sh-eth",    &mstp_clks[MSTP309]),
>> +    CLKDEV_DEV_ID("e9a00000.r8a7740-gether", &mstp_clks[MSTP309]),

>     Al Ethernet devices should be named "ethernet", according to ePAPR spec.

    BTW, I'm not seeing a patch to r8a7740.dtsi, describing this device.

WBR, Sergei


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition
  2013-07-16  9:19 [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition Laurent Pinchart
  2013-07-16 12:27 ` Sergei Shtylyov
  2013-07-16 14:26 ` Sergei Shtylyov
@ 2013-07-17 10:47 ` Laurent Pinchart
  2013-07-17 13:05 ` Sergei Shtylyov
                   ` (12 subsequent siblings)
  15 siblings, 0 replies; 17+ messages in thread
From: Laurent Pinchart @ 2013-07-17 10:47 UTC (permalink / raw)
  To: linux-sh

Hi Sergei,

On Tuesday 16 July 2013 18:26:23 Sergei Shtylyov wrote:
> Hello.
> 
> On 07/16/2013 04:27 PM, Sergei Shtylyov wrote:
> >> The ethernet device is named r8a7740-gether, fix the clock definition
> >> accordingly.
> >> 
> >> Signed-off-by: Laurent Pinchart
> >> <laurent.pinchart+renesas@ideasonboard.com>
> >> ---
> >> 
> >>   arch/arm/mach-shmobile/clock-r8a7740.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/arch/arm/mach-shmobile/clock-r8a7740.c
> >> b/arch/arm/mach-shmobile/clock-r8a7740.c
> >> index de10fd7..e1e8710 100644
> >> --- a/arch/arm/mach-shmobile/clock-r8a7740.c
> >> +++ b/arch/arm/mach-shmobile/clock-r8a7740.c
> >> @@ -595,7 +595,7 @@ static struct clk_lookup lookups[] = {
> >> 
> >>       CLKDEV_DEV_ID("sh_mmcif",        &mstp_clks[MSTP312]),
> >>       CLKDEV_DEV_ID("e6bd0000.mmcif",         &mstp_clks[MSTP312]),
> >>       CLKDEV_DEV_ID("r8a7740-gether",        &mstp_clks[MSTP309]),
> >> 
> >> -    CLKDEV_DEV_ID("e9a00000.sh-eth",    &mstp_clks[MSTP309]),
> >> +    CLKDEV_DEV_ID("e9a00000.r8a7740-gether", &mstp_clks[MSTP309]),
> >> 
> >     Al Ethernet devices should be named "ethernet", according to ePAPR
> >     spec.
> 
>     BTW, I'm not seeing a patch to r8a7740.dtsi, describing this device.

Let's delay this patch until the device gets added to r8a7740.dtsi then.

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition
  2013-07-16  9:19 [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition Laurent Pinchart
                   ` (2 preceding siblings ...)
  2013-07-17 10:47 ` Laurent Pinchart
@ 2013-07-17 13:05 ` Sergei Shtylyov
  2013-07-17 13:11 ` Laurent Pinchart
                   ` (11 subsequent siblings)
  15 siblings, 0 replies; 17+ messages in thread
From: Sergei Shtylyov @ 2013-07-17 13:05 UTC (permalink / raw)
  To: linux-sh

Hello.

On 17-07-2013 14:47, Laurent Pinchart wrote:

>>>> The ethernet device is named r8a7740-gether, fix the clock definition
>>>> accordingly.

>>>> Signed-off-by: Laurent Pinchart
>>>> <laurent.pinchart+renesas@ideasonboard.com>
>>>> ---

>>>>    arch/arm/mach-shmobile/clock-r8a7740.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)

>>>> diff --git a/arch/arm/mach-shmobile/clock-r8a7740.c
>>>> b/arch/arm/mach-shmobile/clock-r8a7740.c
>>>> index de10fd7..e1e8710 100644
>>>> --- a/arch/arm/mach-shmobile/clock-r8a7740.c
>>>> +++ b/arch/arm/mach-shmobile/clock-r8a7740.c
>>>> @@ -595,7 +595,7 @@ static struct clk_lookup lookups[] = {
>>>>
>>>>        CLKDEV_DEV_ID("sh_mmcif",        &mstp_clks[MSTP312]),
>>>>        CLKDEV_DEV_ID("e6bd0000.mmcif",         &mstp_clks[MSTP312]),
>>>>        CLKDEV_DEV_ID("r8a7740-gether",        &mstp_clks[MSTP309]),
>>>>
>>>> -    CLKDEV_DEV_ID("e9a00000.sh-eth",    &mstp_clks[MSTP309]),
>>>> +    CLKDEV_DEV_ID("e9a00000.r8a7740-gether", &mstp_clks[MSTP309]),

>>>      Al Ethernet devices should be named "ethernet", according to ePAPR
>>>      spec.

>>      BTW, I'm not seeing a patch to r8a7740.dtsi, describing this device.

> Let's delay this patch until the device gets added to r8a7740.dtsi then.

    I don't see a use for this line even then. sh-eth.c can't be converted to 
device tree due to procedural platform data, so I'm planning to use 
OF_DEV_AUXDATA() for it which doesn't require defining an extra clock.

WBR, Sergei


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition
  2013-07-16  9:19 [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition Laurent Pinchart
                   ` (3 preceding siblings ...)
  2013-07-17 13:05 ` Sergei Shtylyov
@ 2013-07-17 13:11 ` Laurent Pinchart
  2013-07-17 13:40 ` Sergei Shtylyov
                   ` (10 subsequent siblings)
  15 siblings, 0 replies; 17+ messages in thread
From: Laurent Pinchart @ 2013-07-17 13:11 UTC (permalink / raw)
  To: linux-sh

Hi Sergei,

On Wednesday 17 July 2013 17:05:50 Sergei Shtylyov wrote:
> Hello.
> 
> On 17-07-2013 14:47, Laurent Pinchart wrote:
> >>>> The ethernet device is named r8a7740-gether, fix the clock definition
> >>>> accordingly.
> >>>> 
> >>>> Signed-off-by: Laurent Pinchart
> >>>> <laurent.pinchart+renesas@ideasonboard.com>
> >>>> ---
> >>>> 
> >>>>    arch/arm/mach-shmobile/clock-r8a7740.c | 2 +-
> >>>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>> 
> >>>> diff --git a/arch/arm/mach-shmobile/clock-r8a7740.c
> >>>> b/arch/arm/mach-shmobile/clock-r8a7740.c
> >>>> index de10fd7..e1e8710 100644
> >>>> --- a/arch/arm/mach-shmobile/clock-r8a7740.c
> >>>> +++ b/arch/arm/mach-shmobile/clock-r8a7740.c
> >>>> @@ -595,7 +595,7 @@ static struct clk_lookup lookups[] = {
> >>>> 
> >>>>        CLKDEV_DEV_ID("sh_mmcif",        &mstp_clks[MSTP312]),
> >>>>        CLKDEV_DEV_ID("e6bd0000.mmcif",         &mstp_clks[MSTP312]),
> >>>>        CLKDEV_DEV_ID("r8a7740-gether",        &mstp_clks[MSTP309]),
> >>>> 
> >>>> -    CLKDEV_DEV_ID("e9a00000.sh-eth",    &mstp_clks[MSTP309]),
> >>>> +    CLKDEV_DEV_ID("e9a00000.r8a7740-gether", &mstp_clks[MSTP309]),
> >>>> 
> >>>      Al Ethernet devices should be named "ethernet", according to ePAPR
> >>>      spec.
> >>      
> >>      BTW, I'm not seeing a patch to r8a7740.dtsi, describing this device.
> > 
> > Let's delay this patch until the device gets added to r8a7740.dtsi then.
> 
> I don't see a use for this line even then. sh-eth.c can't be converted
> to device tree due to procedural platform data, so I'm planning to use
> OF_DEV_AUXDATA() for it which doesn't require defining an extra clock.

The usage of OF_DEV_AUXDATA() is discouraged. A quick grep shows that the only 
board code callback in sh_eth_plat_data (.set_mdio_gate) isn't used on ARM 
platforms, so the driver should support pure DT bindings without auxiliary 
data.

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition
  2013-07-16  9:19 [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition Laurent Pinchart
                   ` (4 preceding siblings ...)
  2013-07-17 13:11 ` Laurent Pinchart
@ 2013-07-17 13:40 ` Sergei Shtylyov
  2013-07-17 14:04 ` Laurent Pinchart
                   ` (9 subsequent siblings)
  15 siblings, 0 replies; 17+ messages in thread
From: Sergei Shtylyov @ 2013-07-17 13:40 UTC (permalink / raw)
  To: linux-sh

Hello.

On 17-07-2013 17:11, Laurent Pinchart wrote:

>>>>>> The ethernet device is named r8a7740-gether, fix the clock definition
>>>>>> accordingly.
>>>>>>
>>>>>> Signed-off-by: Laurent Pinchart
>>>>>> <laurent.pinchart+renesas@ideasonboard.com>
>>>>>> ---
>>>>>>
>>>>>>     arch/arm/mach-shmobile/clock-r8a7740.c | 2 +-
>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)

>>>>>> diff --git a/arch/arm/mach-shmobile/clock-r8a7740.c
>>>>>> b/arch/arm/mach-shmobile/clock-r8a7740.c
>>>>>> index de10fd7..e1e8710 100644
>>>>>> --- a/arch/arm/mach-shmobile/clock-r8a7740.c
>>>>>> +++ b/arch/arm/mach-shmobile/clock-r8a7740.c
>>>>>> @@ -595,7 +595,7 @@ static struct clk_lookup lookups[] = {
>>>>>>
>>>>>>         CLKDEV_DEV_ID("sh_mmcif",        &mstp_clks[MSTP312]),
>>>>>>         CLKDEV_DEV_ID("e6bd0000.mmcif",         &mstp_clks[MSTP312]),
>>>>>>         CLKDEV_DEV_ID("r8a7740-gether",        &mstp_clks[MSTP309]),
>>>>>>
>>>>>> -    CLKDEV_DEV_ID("e9a00000.sh-eth",    &mstp_clks[MSTP309]),
>>>>>> +    CLKDEV_DEV_ID("e9a00000.r8a7740-gether", &mstp_clks[MSTP309]),

>>>>>       Al Ethernet devices should be named "ethernet", according to ePAPR
>>>>>       spec.

>>>>       BTW, I'm not seeing a patch to r8a7740.dtsi, describing this device.

>>> Let's delay this patch until the device gets added to r8a7740.dtsi then.

>> I don't see a use for this line even then. sh-eth.c can't be converted
>> to device tree due to procedural platform data, so I'm planning to use
>> OF_DEV_AUXDATA() for it which doesn't require defining an extra clock.

> The usage of OF_DEV_AUXDATA() is discouraged. A quick grep shows that the only

    We have the case where it's the only resort. And the other platforms use 
it quite often, even if only to support [devm_]clk_get() in the drivers.

> board code callback in sh_eth_plat_data (.set_mdio_gate) isn't used on ARM
> platforms, so the driver should support pure DT bindings without auxiliary
> data.

    Maybe it isn't used on ARM but it exists. IMO that's enough reason not to 
convert the platform data to the DT properties.

WBR, Sergei


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition
  2013-07-16  9:19 [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition Laurent Pinchart
                   ` (5 preceding siblings ...)
  2013-07-17 13:40 ` Sergei Shtylyov
@ 2013-07-17 14:04 ` Laurent Pinchart
  2013-07-17 14:20 ` Sergei Shtylyov
                   ` (8 subsequent siblings)
  15 siblings, 0 replies; 17+ messages in thread
From: Laurent Pinchart @ 2013-07-17 14:04 UTC (permalink / raw)
  To: linux-sh

Hi Sergei,

On Wednesday 17 July 2013 17:40:17 Sergei Shtylyov wrote:
> On 17-07-2013 17:11, Laurent Pinchart wrote:
> >>>>>> The ethernet device is named r8a7740-gether, fix the clock definition
> >>>>>> accordingly.
> >>>>>> 
> >>>>>> Signed-off-by: Laurent Pinchart
> >>>>>> <laurent.pinchart+renesas@ideasonboard.com>
> >>>>>> ---
> >>>>>> 
> >>>>>>     arch/arm/mach-shmobile/clock-r8a7740.c | 2 +-
> >>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>> 
> >>>>>> diff --git a/arch/arm/mach-shmobile/clock-r8a7740.c
> >>>>>> b/arch/arm/mach-shmobile/clock-r8a7740.c
> >>>>>> index de10fd7..e1e8710 100644
> >>>>>> --- a/arch/arm/mach-shmobile/clock-r8a7740.c
> >>>>>> +++ b/arch/arm/mach-shmobile/clock-r8a7740.c
> >>>>>> @@ -595,7 +595,7 @@ static struct clk_lookup lookups[] = {
> >>>>>> 
> >>>>>>         CLKDEV_DEV_ID("sh_mmcif",        &mstp_clks[MSTP312]),
> >>>>>>         CLKDEV_DEV_ID("e6bd0000.mmcif",         &mstp_clks[MSTP312]),
> >>>>>>         CLKDEV_DEV_ID("r8a7740-gether",        &mstp_clks[MSTP309]),
> >>>>>> 
> >>>>>> -    CLKDEV_DEV_ID("e9a00000.sh-eth",    &mstp_clks[MSTP309]),
> >>>>>> +    CLKDEV_DEV_ID("e9a00000.r8a7740-gether", &mstp_clks[MSTP309]),
> >>>>>> 
> >>>>>       Al Ethernet devices should be named "ethernet", according to
> >>>>>       ePAPR
> >>>>>       spec.
> >>>>       
> >>>>       BTW, I'm not seeing a patch to r8a7740.dtsi, describing this
> >>>>       device.
> >>> 
> >>> Let's delay this patch until the device gets added to r8a7740.dtsi then.
> >> 
> >> I don't see a use for this line even then. sh-eth.c can't be converted
> >> to device tree due to procedural platform data, so I'm planning to use
> >> OF_DEV_AUXDATA() for it which doesn't require defining an extra clock.
> > 
> > The usage of OF_DEV_AUXDATA() is discouraged. A quick grep shows that the
> > only
>
> We have the case where it's the only resort. And the other platforms use it
> quite often, even if only to support [devm_]clk_get() in the drivers.
>
> > board code callback in sh_eth_plat_data (.set_mdio_gate) isn't used on ARM
> > platforms, so the driver should support pure DT bindings without auxiliary
> > data.
> 
> Maybe it isn't used on ARM but it exists. IMO that's enough reason not to
> convert the platform data to the DT properties.

I don't agree. The proper fix would be to fix the SuperH platform that uses 
that callback (there's one only) to replace the callback function with a 
proper kernel framework. As arch/sh/ has seen very little activity lately I 
don't think that will happen soon, so we'll need to keep support for the 
.set_mdio_gate() callback in the sh-eth driver, but new platforms should not 
use it, and it shouldn't be a reason not to implement proper DT bindings.

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition
  2013-07-16  9:19 [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition Laurent Pinchart
                   ` (6 preceding siblings ...)
  2013-07-17 14:04 ` Laurent Pinchart
@ 2013-07-17 14:20 ` Sergei Shtylyov
  2013-07-17 14:48 ` Laurent Pinchart
                   ` (7 subsequent siblings)
  15 siblings, 0 replies; 17+ messages in thread
From: Sergei Shtylyov @ 2013-07-17 14:20 UTC (permalink / raw)
  To: linux-sh

Hello.

On 17-07-2013 18:04, Laurent Pinchart wrote:

>>>>>>>> The ethernet device is named r8a7740-gether, fix the clock definition
>>>>>>>> accordingly.

>>>>>>>> Signed-off-by: Laurent Pinchart
>>>>>>>> <laurent.pinchart+renesas@ideasonboard.com>
>>>>>>>> ---

>>>>>>>>      arch/arm/mach-shmobile/clock-r8a7740.c | 2 +-
>>>>>>>>      1 file changed, 1 insertion(+), 1 deletion(-)

>>>>>>>> diff --git a/arch/arm/mach-shmobile/clock-r8a7740.c
>>>>>>>> b/arch/arm/mach-shmobile/clock-r8a7740.c
>>>>>>>> index de10fd7..e1e8710 100644
>>>>>>>> --- a/arch/arm/mach-shmobile/clock-r8a7740.c
>>>>>>>> +++ b/arch/arm/mach-shmobile/clock-r8a7740.c
>>>>>>>> @@ -595,7 +595,7 @@ static struct clk_lookup lookups[] = {
>>>>>>>>
>>>>>>>>          CLKDEV_DEV_ID("sh_mmcif",        &mstp_clks[MSTP312]),
>>>>>>>>          CLKDEV_DEV_ID("e6bd0000.mmcif",         &mstp_clks[MSTP312]),
>>>>>>>>          CLKDEV_DEV_ID("r8a7740-gether",        &mstp_clks[MSTP309]),
>>>>>>>>
>>>>>>>> -    CLKDEV_DEV_ID("e9a00000.sh-eth",    &mstp_clks[MSTP309]),
>>>>>>>> +    CLKDEV_DEV_ID("e9a00000.r8a7740-gether", &mstp_clks[MSTP309]),

>>>>>>>        Al Ethernet devices should be named "ethernet", according to
>>>>>>>        ePAPR
>>>>>>>        spec.

>>>>>>        BTW, I'm not seeing a patch to r8a7740.dtsi, describing this
>>>>>>        device.

>>>>> Let's delay this patch until the device gets added to r8a7740.dtsi then.

>>>> I don't see a use for this line even then. sh-eth.c can't be converted
>>>> to device tree due to procedural platform data, so I'm planning to use
>>>> OF_DEV_AUXDATA() for it which doesn't require defining an extra clock.

>>> The usage of OF_DEV_AUXDATA() is discouraged. A quick grep shows that the
>>> only

>> We have the case where it's the only resort. And the other platforms use it
>> quite often, even if only to support [devm_]clk_get() in the drivers.

>>> board code callback in sh_eth_plat_data (.set_mdio_gate) isn't used on ARM
>>> platforms, so the driver should support pure DT bindings without auxiliary
>>> data.

>> Maybe it isn't used on ARM but it exists. IMO that's enough reason not to
>> convert the platform data to the DT properties.

> I don't agree. The proper fix would be to fix the SuperH platform that uses
> that callback (there's one only) to replace the callback function with a
> proper kernel framework.

    At least suggest such framework first.

> As arch/sh/ has seen very little activity lately I
> don't think that will happen soon,

    I can take this in my own hands, if you get any ideas.

> so we'll need to keep support for the
> .set_mdio_gate() callback in the sh-eth driver, but new platforms should not
> use it, and it shouldn't be a reason not to implement proper DT bindings.

    What if they have to use it again?
    Besides, it's not the only obstacle on this way: the platform data 
includes some fields which should really be inside the driver's data 
structures, as they're SoC, not board specific...

WBR, Sergei


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition
  2013-07-16  9:19 [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition Laurent Pinchart
                   ` (7 preceding siblings ...)
  2013-07-17 14:20 ` Sergei Shtylyov
@ 2013-07-17 14:48 ` Laurent Pinchart
  2013-07-17 23:04 ` Simon Horman
                   ` (6 subsequent siblings)
  15 siblings, 0 replies; 17+ messages in thread
From: Laurent Pinchart @ 2013-07-17 14:48 UTC (permalink / raw)
  To: linux-sh

Hi Sergei,

On Wednesday 17 July 2013 18:20:48 Sergei Shtylyov wrote:
> Hello.
> 
> On 17-07-2013 18:04, Laurent Pinchart wrote:
> >>>>>>>> The ethernet device is named r8a7740-gether, fix the clock
> >>>>>>>> definition
> >>>>>>>> accordingly.
> >>>>>>>> 
> >>>>>>>> Signed-off-by: Laurent Pinchart
> >>>>>>>> <laurent.pinchart+renesas@ideasonboard.com>
> >>>>>>>> ---
> >>>>>>>> 
> >>>>>>>>      arch/arm/mach-shmobile/clock-r8a7740.c | 2 +-
> >>>>>>>>      1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>> 
> >>>>>>>> diff --git a/arch/arm/mach-shmobile/clock-r8a7740.c
> >>>>>>>> b/arch/arm/mach-shmobile/clock-r8a7740.c
> >>>>>>>> index de10fd7..e1e8710 100644
> >>>>>>>> --- a/arch/arm/mach-shmobile/clock-r8a7740.c
> >>>>>>>> +++ b/arch/arm/mach-shmobile/clock-r8a7740.c
> >>>>>>>> @@ -595,7 +595,7 @@ static struct clk_lookup lookups[] = {
> >>>>>>>> 
> >>>>>>>>          CLKDEV_DEV_ID("sh_mmcif",        &mstp_clks[MSTP312]),
> >>>>>>>>          CLKDEV_DEV_ID("e6bd0000.mmcif",        
> >>>>>>>>          &mstp_clks[MSTP312]),
> >>>>>>>>          CLKDEV_DEV_ID("r8a7740-gether",       
> >>>>>>>>          &mstp_clks[MSTP309]),
> >>>>>>>> 
> >>>>>>>> -    CLKDEV_DEV_ID("e9a00000.sh-eth",    &mstp_clks[MSTP309]),
> >>>>>>>> +    CLKDEV_DEV_ID("e9a00000.r8a7740-gether", &mstp_clks[MSTP309]),
> >>>>>>>> 
> >>>>>>> Al Ethernet devices should be named "ethernet", according to ePAPR
> >>>>>>> spec.
> >>>>>>        
> >>>>>> BTW, I'm not seeing a patch to r8a7740.dtsi, describing this device.
> >>>>> 
> >>>>> Let's delay this patch until the device gets added to r8a7740.dtsi
> >>>>> then.
> >>>> 
> >>>> I don't see a use for this line even then. sh-eth.c can't be converted
> >>>> to device tree due to procedural platform data, so I'm planning to use
> >>>> OF_DEV_AUXDATA() for it which doesn't require defining an extra clock.
> >>> 
> >>> The usage of OF_DEV_AUXDATA() is discouraged. A quick grep shows that
> >>> the only
> >> 
> >> We have the case where it's the only resort. And the other platforms use
> >> it quite often, even if only to support [devm_]clk_get() in the drivers.
> >> 
> >>> board code callback in sh_eth_plat_data (.set_mdio_gate) isn't used on
> >>> ARM platforms, so the driver should support pure DT bindings without
> >>> auxiliary data.
> >> 
> >> Maybe it isn't used on ARM but it exists. IMO that's enough reason not to
> >> convert the platform data to the DT properties.
> > 
> > I don't agree. The proper fix would be to fix the SuperH platform that
> > uses that callback (there's one only) to replace the callback function
> > with a proper kernel framework.
> 
> At least suggest such framework first.

I would first need to understand what the board code implementes in the 
set_mdio_gate() callback. The callback is used by the SH7757LCR board only, do 
you have access to the board schematics and SH7757 datasheet ?

> > As arch/sh/ has seen very little activity lately I don't think that will
> > happen soon,
> 
> I can take this in my own hands, if you get any ideas.

That would be great.

> > so we'll need to keep support for the .set_mdio_gate() callback in the sh-
> > eth driver, but new platforms should not use it, and it shouldn't be a
> > reason not to implement proper DT bindings.
>
> What if they have to use it again?

Then we'll need to use a proper abstraction in the kernel, and possibly create 
one if none exists. We've created many abstractions layers in the past (GPIO, 
regulators, CCF, ...) to get rid of platform callbacks, a new one can be added 
if needed.

> Besides, it's not the only obstacle on this way: the platform data includes
> some fields which should really be inside the driver's data structures, as
> they're SoC, not board specific...

Those should then be moved to the sh_eth_cpu_data structure, referenced from 
the data field of both the struct platform_device_id and struct of_device_id 
arrays.

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition
  2013-07-16  9:19 [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition Laurent Pinchart
                   ` (8 preceding siblings ...)
  2013-07-17 14:48 ` Laurent Pinchart
@ 2013-07-17 23:04 ` Simon Horman
  2013-07-18 20:28 ` Sergei Shtylyov
                   ` (5 subsequent siblings)
  15 siblings, 0 replies; 17+ messages in thread
From: Simon Horman @ 2013-07-17 23:04 UTC (permalink / raw)
  To: linux-sh

[ Cc Magnus ]

On Wed, Jul 17, 2013 at 03:11:16PM +0200, Laurent Pinchart wrote:
> Hi Sergei,
> 
> On Wednesday 17 July 2013 17:05:50 Sergei Shtylyov wrote:
> > Hello.
> > 
> > On 17-07-2013 14:47, Laurent Pinchart wrote:
> > >>>> The ethernet device is named r8a7740-gether, fix the clock definition
> > >>>> accordingly.
> > >>>> 
> > >>>> Signed-off-by: Laurent Pinchart
> > >>>> <laurent.pinchart+renesas@ideasonboard.com>
> > >>>> ---
> > >>>> 
> > >>>>    arch/arm/mach-shmobile/clock-r8a7740.c | 2 +-
> > >>>>    1 file changed, 1 insertion(+), 1 deletion(-)
> > >>>> 
> > >>>> diff --git a/arch/arm/mach-shmobile/clock-r8a7740.c
> > >>>> b/arch/arm/mach-shmobile/clock-r8a7740.c
> > >>>> index de10fd7..e1e8710 100644
> > >>>> --- a/arch/arm/mach-shmobile/clock-r8a7740.c
> > >>>> +++ b/arch/arm/mach-shmobile/clock-r8a7740.c
> > >>>> @@ -595,7 +595,7 @@ static struct clk_lookup lookups[] = {
> > >>>> 
> > >>>>        CLKDEV_DEV_ID("sh_mmcif",        &mstp_clks[MSTP312]),
> > >>>>        CLKDEV_DEV_ID("e6bd0000.mmcif",         &mstp_clks[MSTP312]),
> > >>>>        CLKDEV_DEV_ID("r8a7740-gether",        &mstp_clks[MSTP309]),
> > >>>> 
> > >>>> -    CLKDEV_DEV_ID("e9a00000.sh-eth",    &mstp_clks[MSTP309]),
> > >>>> +    CLKDEV_DEV_ID("e9a00000.r8a7740-gether", &mstp_clks[MSTP309]),
> > >>>> 
> > >>>      Al Ethernet devices should be named "ethernet", according to ePAPR
> > >>>      spec.
> > >>      
> > >>      BTW, I'm not seeing a patch to r8a7740.dtsi, describing this device.
> > > 
> > > Let's delay this patch until the device gets added to r8a7740.dtsi then.
> > 
> > I don't see a use for this line even then. sh-eth.c can't be converted
> > to device tree due to procedural platform data, so I'm planning to use
> > OF_DEV_AUXDATA() for it which doesn't require defining an extra clock.
> 
> The usage of OF_DEV_AUXDATA() is discouraged. A quick grep shows that the only 
> board code callback in sh_eth_plat_data (.set_mdio_gate) isn't used on ARM 
> platforms, so the driver should support pure DT bindings without auxiliary 
> data.

Magnus has just spend some time removing auxdata.
Please don't add it back in again.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition
  2013-07-16  9:19 [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition Laurent Pinchart
                   ` (9 preceding siblings ...)
  2013-07-17 23:04 ` Simon Horman
@ 2013-07-18 20:28 ` Sergei Shtylyov
  2013-07-18 20:57 ` Laurent Pinchart
                   ` (4 subsequent siblings)
  15 siblings, 0 replies; 17+ messages in thread
From: Sergei Shtylyov @ 2013-07-18 20:28 UTC (permalink / raw)
  To: linux-sh

Hello.

On 07/17/2013 06:48 PM, Laurent Pinchart wrote:

>>>>>>>>>> The ethernet device is named r8a7740-gether, fix the clock
>>>>>>>>>> definition
>>>>>>>>>> accordingly.

>>>>>>>>>> Signed-off-by: Laurent Pinchart
>>>>>>>>>> <laurent.pinchart+renesas@ideasonboard.com>
>>>>>>>>>> ---

>>>>>>>>>>       arch/arm/mach-shmobile/clock-r8a7740.c | 2 +-
>>>>>>>>>>       1 file changed, 1 insertion(+), 1 deletion(-)

>>>>>>>>>> diff --git a/arch/arm/mach-shmobile/clock-r8a7740.c
>>>>>>>>>> b/arch/arm/mach-shmobile/clock-r8a7740.c
>>>>>>>>>> index de10fd7..e1e8710 100644
>>>>>>>>>> --- a/arch/arm/mach-shmobile/clock-r8a7740.c
>>>>>>>>>> +++ b/arch/arm/mach-shmobile/clock-r8a7740.c
>>>>>>>>>> @@ -595,7 +595,7 @@ static struct clk_lookup lookups[] = {
>>>>>>>>>>
>>>>>>>>>>           CLKDEV_DEV_ID("sh_mmcif",        &mstp_clks[MSTP312]),
>>>>>>>>>>           CLKDEV_DEV_ID("e6bd0000.mmcif",
>>>>>>>>>>           &mstp_clks[MSTP312]),
>>>>>>>>>>           CLKDEV_DEV_ID("r8a7740-gether",
>>>>>>>>>>           &mstp_clks[MSTP309]),
>>>>>>>>>>
>>>>>>>>>> -    CLKDEV_DEV_ID("e9a00000.sh-eth",    &mstp_clks[MSTP309]),
>>>>>>>>>> +    CLKDEV_DEV_ID("e9a00000.r8a7740-gether", &mstp_clks[MSTP309]),

>>>>>>>>> Al Ethernet devices should be named "ethernet", according to ePAPR
>>>>>>>>> spec.

>>>>>>>> BTW, I'm not seeing a patch to r8a7740.dtsi, describing this device.

>>>>>>> Let's delay this patch until the device gets added to r8a7740.dtsi
>>>>>>> then.

>>>>>> I don't see a use for this line even then. sh-eth.c can't be converted
>>>>>> to device tree due to procedural platform data, so I'm planning to use
>>>>>> OF_DEV_AUXDATA() for it which doesn't require defining an extra clock.

>>>>> The usage of OF_DEV_AUXDATA() is discouraged. A quick grep shows that
>>>>> the only

>>>> We have the case where it's the only resort. And the other platforms use
>>>> it quite often, even if only to support [devm_]clk_get() in the drivers.

>>>>> board code callback in sh_eth_plat_data (.set_mdio_gate) isn't used on
>>>>> ARM platforms, so the driver should support pure DT bindings without
>>>>> auxiliary data.

>>>> Maybe it isn't used on ARM but it exists. IMO that's enough reason not to
>>>> convert the platform data to the DT properties.

>>> I don't agree. The proper fix would be to fix the SuperH platform that
>>> uses that callback (there's one only) to replace the callback function
>>> with a proper kernel framework.

>> At least suggest such framework first.

> I would first need to understand what the board code implementes in the
> set_mdio_gate() callback. The callback is used by the SH7757LCR board only, do
> you have access to the board schematics and SH7757 datasheet ?

    No, only for SH7751 manual by coincidence. This SoC doesn't have Ether.
Maybe the original commit (sh: add GETHER's platform_device in 
board-sh7757lcr) author, Shimoda-san, could help us here? I've CC'ed him.

>>> As arch/sh/ has seen very little activity lately I don't think that will
>>> happen soon,

>> I can take this in my own hands, if you get any ideas.

> That would be great.

    We have an obligation to convert sh_eth to DT anyway.

>>> so we'll need to keep support for the .set_mdio_gate() callback in the sh-
>>> eth driver, but new platforms should not use it, and it shouldn't be a
>>> reason not to implement proper DT bindings.

>> What if they have to use it again?

> Then we'll need to use a proper abstraction in the kernel, and possibly create
> one if none exists. We've created many abstractions layers in the past (GPIO,
> regulators, CCF, ...) to get rid of platform callbacks, a new one can be added
> if needed.

    I'd like to share your optimism here...

>> Besides, it's not the only obstacle on this way: the platform data includes
>> some fields which should really be inside the driver's data structures, as
>> they're SoC, not board specific...

> Those should then be moved to the sh_eth_cpu_data structure, referenced from
> the data field of both the struct platform_device_id and struct of_device_id
> arrays.

    Yes.
    The problem is some old arch/sh/ SoCs have platform code inadequate to the 
modern state of the sh_eth platform data, e.g. they pass the PHY address as 
the platform data pointer (!). We should either fix them first (or maybe 
remove their support, as the Renesas management suggested).

WBR, Sergei


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition
  2013-07-16  9:19 [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition Laurent Pinchart
                   ` (10 preceding siblings ...)
  2013-07-18 20:28 ` Sergei Shtylyov
@ 2013-07-18 20:57 ` Laurent Pinchart
  2013-07-19  4:30 ` Shimoda, Yoshihiro
                   ` (3 subsequent siblings)
  15 siblings, 0 replies; 17+ messages in thread
From: Laurent Pinchart @ 2013-07-18 20:57 UTC (permalink / raw)
  To: linux-sh

Hi Sergei,

On Friday 19 July 2013 00:28:29 Sergei Shtylyov wrote:
> Hello.
> 
> On 07/17/2013 06:48 PM, Laurent Pinchart wrote:
> >>>>>>>>>> The ethernet device is named r8a7740-gether, fix the clock
> >>>>>>>>>> definition
> >>>>>>>>>> accordingly.
> >>>>>>>>>> 
> >>>>>>>>>> Signed-off-by: Laurent Pinchart
> >>>>>>>>>> <laurent.pinchart+renesas@ideasonboard.com>
> >>>>>>>>>> ---
> >>>>>>>>>> 
> >>>>>>>>>>       arch/arm/mach-shmobile/clock-r8a7740.c | 2 +-
> >>>>>>>>>>       1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>>>> 
> >>>>>>>>>> diff --git a/arch/arm/mach-shmobile/clock-r8a7740.c
> >>>>>>>>>> b/arch/arm/mach-shmobile/clock-r8a7740.c
> >>>>>>>>>> index de10fd7..e1e8710 100644
> >>>>>>>>>> --- a/arch/arm/mach-shmobile/clock-r8a7740.c
> >>>>>>>>>> +++ b/arch/arm/mach-shmobile/clock-r8a7740.c
> >>>>>>>>>> @@ -595,7 +595,7 @@ static struct clk_lookup lookups[] = {
> >>>>>>>>>> 
> >>>>>>>>>>           CLKDEV_DEV_ID("sh_mmcif",        &mstp_clks[MSTP312]),
> >>>>>>>>>>           CLKDEV_DEV_ID("e6bd0000.mmcif",
> >>>>>>>>>>           &mstp_clks[MSTP312]),
> >>>>>>>>>>           CLKDEV_DEV_ID("r8a7740-gether",
> >>>>>>>>>>           &mstp_clks[MSTP309]),
> >>>>>>>>>> 
> >>>>>>>>>> -    CLKDEV_DEV_ID("e9a00000.sh-eth",    &mstp_clks[MSTP309]),
> >>>>>>>>>> +    CLKDEV_DEV_ID("e9a00000.r8a7740-gether",
> >>>>>>>>>> &mstp_clks[MSTP309]),
> >>>>>>>>> 
> >>>>>>>>> Al Ethernet devices should be named "ethernet", according to ePAPR
> >>>>>>>>> spec.
> >>>>>>>> 
> >>>>>>>> BTW, I'm not seeing a patch to r8a7740.dtsi, describing this
> >>>>>>>> device.
> >>>>>>> 
> >>>>>>> Let's delay this patch until the device gets added to r8a7740.dtsi
> >>>>>>> then.
> >>>>>> 
> >>>>>> I don't see a use for this line even then. sh-eth.c can't be
> >>>>>> converted to device tree due to procedural platform data, so I'm
> >>>>>> planning to use OF_DEV_AUXDATA() for it which doesn't require
> >>>>>> defining an extra clock.
> >>>>> 
> >>>>> The usage of OF_DEV_AUXDATA() is discouraged. A quick grep shows that
> >>>>> the only
> >>>> 
> >>>> We have the case where it's the only resort. And the other platforms
> >>>> use it quite often, even if only to support [devm_]clk_get() in the
> >>>> drivers.
> >>>> 
> >>>>> board code callback in sh_eth_plat_data (.set_mdio_gate) isn't used on
> >>>>> ARM platforms, so the driver should support pure DT bindings without
> >>>>> auxiliary data.
> >>>> 
> >>>> Maybe it isn't used on ARM but it exists. IMO that's enough reason not
> >>>> to convert the platform data to the DT properties.
> >>> 
> >>> I don't agree. The proper fix would be to fix the SuperH platform that
> >>> uses that callback (there's one only) to replace the callback function
> >>> with a proper kernel framework.
> >> 
> >> At least suggest such framework first.
> > 
> > I would first need to understand what the board code implementes in the
> > set_mdio_gate() callback. The callback is used by the SH7757LCR board
> > only, do you have access to the board schematics and SH7757 datasheet ?
> 
> No, only for SH7751 manual by coincidence. This SoC doesn't have Ether.
> Maybe the original commit (sh: add GETHER's platform_device in board-
> sh7757lcr) author, Shimoda-san, could help us here? I've CC'ed him.
> 
> >>> As arch/sh/ has seen very little activity lately I don't think that will
> >>> happen soon,
> >> 
> >> I can take this in my own hands, if you get any ideas.
> > 
> > That would be great.
> 
> We have an obligation to convert sh_eth to DT anyway.
> 
> >>> so we'll need to keep support for the .set_mdio_gate() callback in the
> >>> sh-eth driver, but new platforms should not use it, and it shouldn't be
> >>> a reason not to implement proper DT bindings.
> >> 
> >> What if they have to use it again?
> > 
> > Then we'll need to use a proper abstraction in the kernel, and possibly
> > create one if none exists. We've created many abstractions layers in the
> > past (GPIO, regulators, CCF, ...) to get rid of platform callbacks, a new
> > one can be added if needed.
> 
> I'd like to share your optimism here...
> 
> >> Besides, it's not the only obstacle on this way: the platform data
> >> includes some fields which should really be inside the driver's data
> >> structures, as they're SoC, not board specific...
> > 
> > Those should then be moved to the sh_eth_cpu_data structure, referenced
> > from the data field of both the struct platform_device_id and struct
> > of_device_id arrays.
> 
> Yes.
> The problem is some old arch/sh/ SoCs have platform code inadequate to the
> modern state of the sh_eth platform data, e.g. they pass the PHY address as
> the platform data pointer (!). We should either fix them first (or maybe
> remove their support, as the Renesas management suggested).

That's a good plan. I had to go through a similar process for at least 
backlight drivers and the PFC driver, I know how painful it can be.

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition
  2013-07-16  9:19 [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition Laurent Pinchart
                   ` (11 preceding siblings ...)
  2013-07-18 20:57 ` Laurent Pinchart
@ 2013-07-19  4:30 ` Shimoda, Yoshihiro
  2013-07-19 12:17 ` Sergei Shtylyov
                   ` (2 subsequent siblings)
  15 siblings, 0 replies; 17+ messages in thread
From: Shimoda, Yoshihiro @ 2013-07-19  4:30 UTC (permalink / raw)
  To: linux-sh

Hello,

(2013/07/19 5:28), Sergei Shtylyov wrote:
> Hello.
>
> On 07/17/2013 06:48 PM, Laurent Pinchart wrote:
< snip >
>
>>>>>> board code callback in sh_eth_plat_data (.set_mdio_gate) isn't used on
>>>>>> ARM platforms, so the driver should support pure DT bindings without
>>>>>> auxiliary data.
>
>>>>> Maybe it isn't used on ARM but it exists. IMO that's enough reason not to
>>>>> convert the platform data to the DT properties.
>
>>>> I don't agree. The proper fix would be to fix the SuperH platform that
>>>> uses that callback (there's one only) to replace the callback function
>>>> with a proper kernel framework.
>
>>> At least suggest such framework first.
>
>> I would first need to understand what the board code implementes in the
>> set_mdio_gate() callback. The callback is used by the SH7757LCR board only, do
>> you have access to the board schematics and SH7757 datasheet ?
>
>    No, only for SH7751 manual by coincidence. This SoC doesn't have Ether.
> Maybe the original commit (sh: add GETHER's platform_device in board-sh7757lcr) author, Shimoda-san, could help us here? I've CC'ed him.

- The SH7757 has 2 MDIO/MDC channels for Gigabit Ethernet.
- The board has VSC8244 Gigabit Ethernet PHY.
- The VSC8244 has 4 MAC channels. But it has one MDIO/MDC port.
- The board's schematic about MDIO/MDC is like below:
+--------+ <--- GETHER ch0's MDIO/MDC ---> +-----------+
| SH7757 | <--- GETHER ch1's MDIO/MDC ---> | bus swith | <--- MDIO/MDC ---> VSC8244
+--------+ <--- GPIO --------------------> +-----------+

So, I added to control the GPIO using the set_mdio_gate() callback on the board.

Best regards,
Yoshihiro Shimoda

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition
  2013-07-16  9:19 [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition Laurent Pinchart
                   ` (12 preceding siblings ...)
  2013-07-19  4:30 ` Shimoda, Yoshihiro
@ 2013-07-19 12:17 ` Sergei Shtylyov
  2013-07-22  9:15 ` Shimoda, Yoshihiro
  2013-07-22 11:47 ` Sergei Shtylyov
  15 siblings, 0 replies; 17+ messages in thread
From: Sergei Shtylyov @ 2013-07-19 12:17 UTC (permalink / raw)
  To: linux-sh

Hello.

On 19-07-2013 8:30, Shimoda, Yoshihiro wrote:

> < snip >

>>>>>>> board code callback in sh_eth_plat_data (.set_mdio_gate) isn't used on
>>>>>>> ARM platforms, so the driver should support pure DT bindings without
>>>>>>> auxiliary data.

>>>>>> Maybe it isn't used on ARM but it exists. IMO that's enough reason not to
>>>>>> convert the platform data to the DT properties.

>>>>> I don't agree. The proper fix would be to fix the SuperH platform that
>>>>> uses that callback (there's one only) to replace the callback function
>>>>> with a proper kernel framework.

>>>> At least suggest such framework first.

>>> I would first need to understand what the board code implementes in the
>>> set_mdio_gate() callback. The callback is used by the SH7757LCR board only, do
>>> you have access to the board schematics and SH7757 datasheet ?

>>     No, only for SH7751 manual by coincidence. This SoC doesn't have Ether.
>> Maybe the original commit (sh: add GETHER's platform_device in board-sh7757lcr) author, Shimoda-san, could help us here? I've CC'ed him.

> - The SH7757 has 2 MDIO/MDC channels for Gigabit Ethernet.
> - The board has VSC8244 Gigabit Ethernet PHY.
> - The VSC8244 has 4 MAC channels. But it has one MDIO/MDC port.
> - The board's schematic about MDIO/MDC is like below:
> +--------+ <--- GETHER ch0's MDIO/MDC ---> +-----------+
> | SH7757 | <--- GETHER ch1's MDIO/MDC ---> | bus swith | <--- MDIO/MDC ---> VSC8244
> +--------+ <--- GPIO --------------------> +-----------+

    You apparently forgot to mention 2 Ether devices for which set_mdio_gate() 
method is also used (this is introduced by the same patch). What is the 
GBECONT register you also didn't mention?

> So, I added to control the GPIO using the set_mdio_gate() callback on the board.

> Best regards,
> Yoshihiro Shimoda

WBR, Sergei


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition
  2013-07-16  9:19 [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition Laurent Pinchart
                   ` (13 preceding siblings ...)
  2013-07-19 12:17 ` Sergei Shtylyov
@ 2013-07-22  9:15 ` Shimoda, Yoshihiro
  2013-07-22 11:47 ` Sergei Shtylyov
  15 siblings, 0 replies; 17+ messages in thread
From: Shimoda, Yoshihiro @ 2013-07-22  9:15 UTC (permalink / raw)
  To: linux-sh

Hello,

(2013/07/19 21:17), Sergei Shtylyov wrote:
> Hello.
> 
> On 19-07-2013 8:30, Shimoda, Yoshihiro wrote:
> 
>> < snip >
> 
>>>>>>>> board code callback in sh_eth_plat_data (.set_mdio_gate) isn't used on
>>>>>>>> ARM platforms, so the driver should support pure DT bindings without
>>>>>>>> auxiliary data.
> 
>>>>>>> Maybe it isn't used on ARM but it exists. IMO that's enough reason not to
>>>>>>> convert the platform data to the DT properties.
> 
>>>>>> I don't agree. The proper fix would be to fix the SuperH platform that
>>>>>> uses that callback (there's one only) to replace the callback function
>>>>>> with a proper kernel framework.
> 
>>>>> At least suggest such framework first.
> 
>>>> I would first need to understand what the board code implementes in the
>>>> set_mdio_gate() callback. The callback is used by the SH7757LCR board only, do
>>>> you have access to the board schematics and SH7757 datasheet ?
> 
>>>     No, only for SH7751 manual by coincidence. This SoC doesn't have Ether.
>>> Maybe the original commit (sh: add GETHER's platform_device in board-sh7757lcr) author, Shimoda-san, could help us here? I've CC'ed him.
> 
>> - The SH7757 has 2 MDIO/MDC channels for Gigabit Ethernet.
>> - The board has VSC8244 Gigabit Ethernet PHY.
>> - The VSC8244 has 4 MAC channels. But it has one MDIO/MDC port.
>> - The board's schematic about MDIO/MDC is like below:
>> +--------+ <--- GETHER ch0's MDIO/MDC ---> +-----------+
>> | SH7757 | <--- GETHER ch1's MDIO/MDC ---> | bus swith | <--- MDIO/MDC ---> VSC8244
>> +--------+ <--- GPIO --------------------> +-----------+
> 
>    You apparently forgot to mention 2 Ether devices for which set_mdio_gate() method is also used (this is introduced by the same patch). What is the GBECONT register you also didn't mention?

Sorry, I forgot to mention the GBECONT register.
So, I would like to rewrite my description about .set_mdio_gate() of board-sh7757lcr.c:

- The SH7757 has 2 Gigabit Ethernet controllers (GETHER) and
  2 Fast Ethernet controllers (ETHER).
- The SH7757 has 2 MDIO/MDC channels, but those are multiplexed port.
- The SH7757 has the GBECONT register:
 - If "RMII0" is set to 1, the ETHER channel 0 can use the MDIO/MDC channel 0.
 - If "RMII0" is set to 0, the GETHER channel 0 can use the MDIO/MDC channel 0.
 - If "RMII1" is set to 1, the ETHER channel 1 can use the MDIO/MDC channel 1.
 - If "RMII1" is set to 0, the GETHER channel 1 can use the MDIO/MDC channel 1.
- The board has 2 DP83848C Fast Ethernet PHYs.
- The board has VSC8244 Gigabit Ethernet PHY.
- The VSC8244 has 4 MAC channels. But it has one MDIO/MDC port.
- The board's schematic about MDIO/MDC is like below:
+--------+                     +--> DP83848C
|        | <-- MDIO/MDC ch 0 --+--> +------------+
| SH7757 | <-- GPIO --------------> | bus switch | <-- MDIO/MDC --> VSC8244
|        | <-- MDIO/MDC ch 1 --+--> +------------+
+--------+                     +--> DP83848C
- About the bus switch:
 - If the GPIO is set to high, the bus switch will conenct the MDIO/MDC ch 0 to VSC8244.
 - If the GPIO is set to low, the bus switch will conenct the MDIO/MDC ch 1 to VSC8244.

So, if the ETHER channel 0 wants to use the MDIO/MDC ch 0,
the "RMII0" should be set to 1.
So, if the GETHER channel 0 wants to use the MDIO/MDC ch 0,
the "RMII0" should be set to 0 and the GPIO should be set to High.

Best regards,
Yoshihiro Shimoda

>> So, I added to control the GPIO using the set_mdio_gate() callback on the board.
> 
>> Best regards,
>> Yoshihiro Shimoda
> 
> WBR, Sergei
> 
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition
  2013-07-16  9:19 [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition Laurent Pinchart
                   ` (14 preceding siblings ...)
  2013-07-22  9:15 ` Shimoda, Yoshihiro
@ 2013-07-22 11:47 ` Sergei Shtylyov
  15 siblings, 0 replies; 17+ messages in thread
From: Sergei Shtylyov @ 2013-07-22 11:47 UTC (permalink / raw)
  To: linux-sh

Hello.

On 22-07-2013 13:15, Shimoda, Yoshihiro wrote:

>>>>>>>>> board code callback in sh_eth_plat_data (.set_mdio_gate) isn't used on
>>>>>>>>> ARM platforms, so the driver should support pure DT bindings without
>>>>>>>>> auxiliary data.

>>>>>>>> Maybe it isn't used on ARM but it exists. IMO that's enough reason not to
>>>>>>>> convert the platform data to the DT properties.

>>>>>>> I don't agree. The proper fix would be to fix the SuperH platform that
>>>>>>> uses that callback (there's one only) to replace the callback function
>>>>>>> with a proper kernel framework.

>>>>>> At least suggest such framework first.

>>>>> I would first need to understand what the board code implementes in the
>>>>> set_mdio_gate() callback. The callback is used by the SH7757LCR board only, do
>>>>> you have access to the board schematics and SH7757 datasheet ?

>>>>      No, only for SH7751 manual by coincidence. This SoC doesn't have Ether.
>>>> Maybe the original commit (sh: add GETHER's platform_device in board-sh7757lcr) author, Shimoda-san, could help us here? I've CC'ed him.

>>> - The SH7757 has 2 MDIO/MDC channels for Gigabit Ethernet.
>>> - The board has VSC8244 Gigabit Ethernet PHY.
>>> - The VSC8244 has 4 MAC channels. But it has one MDIO/MDC port.
>>> - The board's schematic about MDIO/MDC is like below:
>>> +--------+ <--- GETHER ch0's MDIO/MDC ---> +-----------+
>>> | SH7757 | <--- GETHER ch1's MDIO/MDC ---> | bus swith | <--- MDIO/MDC ---> VSC8244
>>> +--------+ <--- GPIO --------------------> +-----------+

>>     You apparently forgot to mention 2 Ether devices for which set_mdio_gate() method is also used (this is introduced by the same patch). What is the GBECONT register you also didn't mention?

> Sorry, I forgot to mention the GBECONT register.
> So, I would like to rewrite my description about .set_mdio_gate() of board-sh7757lcr.c:

> - The SH7757 has 2 Gigabit Ethernet controllers (GETHER) and
>    2 Fast Ethernet controllers (ETHER).
> - The SH7757 has 2 MDIO/MDC channels, but those are multiplexed port.
> - The SH7757 has the GBECONT register:
>   - If "RMII0" is set to 1, the ETHER channel 0 can use the MDIO/MDC channel 0.
>   - If "RMII0" is set to 0, the GETHER channel 0 can use the MDIO/MDC channel 0.
>   - If "RMII1" is set to 1, the ETHER channel 1 can use the MDIO/MDC channel 1.
>   - If "RMII1" is set to 0, the GETHER channel 1 can use the MDIO/MDC channel 1.
> - The board has 2 DP83848C Fast Ethernet PHYs.
> - The board has VSC8244 Gigabit Ethernet PHY.
> - The VSC8244 has 4 MAC channels. But it has one MDIO/MDC port.
> - The board's schematic about MDIO/MDC is like below:
> +--------+                     +--> DP83848C
> |        | <-- MDIO/MDC ch 0 --+--> +------------+
> | SH7757 | <-- GPIO --------------> | bus switch | <-- MDIO/MDC --> VSC8244
> |        | <-- MDIO/MDC ch 1 --+--> +------------+
> +--------+                     +--> DP83848C
> - About the bus switch:
>   - If the GPIO is set to high, the bus switch will conenct the MDIO/MDC ch 0 to VSC8244.
>   - If the GPIO is set to low, the bus switch will conenct the MDIO/MDC ch 1 to VSC8244.
>
> So, if the ETHER channel 0 wants to use the MDIO/MDC ch 0,
> the "RMII0" should be set to 1.
> So, if the GETHER channel 0 wants to use the MDIO/MDC ch 0,
> the "RMII0" should be set to 0 and the GPIO should be set to High.

    Thank you for the quick replies, Shimoda-san.

> Best regards,
> Yoshihiro Shimoda

WBR, Sergei


^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2013-07-22 11:47 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-16  9:19 [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition Laurent Pinchart
2013-07-16 12:27 ` Sergei Shtylyov
2013-07-16 14:26 ` Sergei Shtylyov
2013-07-17 10:47 ` Laurent Pinchart
2013-07-17 13:05 ` Sergei Shtylyov
2013-07-17 13:11 ` Laurent Pinchart
2013-07-17 13:40 ` Sergei Shtylyov
2013-07-17 14:04 ` Laurent Pinchart
2013-07-17 14:20 ` Sergei Shtylyov
2013-07-17 14:48 ` Laurent Pinchart
2013-07-17 23:04 ` Simon Horman
2013-07-18 20:28 ` Sergei Shtylyov
2013-07-18 20:57 ` Laurent Pinchart
2013-07-19  4:30 ` Shimoda, Yoshihiro
2013-07-19 12:17 ` Sergei Shtylyov
2013-07-22  9:15 ` Shimoda, Yoshihiro
2013-07-22 11:47 ` Sergei Shtylyov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.