Re: [PATCH] ARM: shmobile: r8a7740: Fix ethernet device name in clock definition

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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