* 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