[PATCH 01/11] ARM: shmobile: r8a7778: cleanup registration of sh_eth

[PATCH 01/11] ARM: shmobile: r8a7778: cleanup registration of sh_eth

[Kuninori Morimoto]

sh_eth driver which needs platform data at the time of
registration is used from BockW only.
Now, ARM/shmobile aims to support DT,
and the C code base board support will be removed in the future.
The driver registration method which needs platform data
and which is not shared complicates codes.
This patch registers it on board code as cleanup C code

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 arch/arm/mach-shmobile/board-bockw.c          |   15 ++++++++++++++-
 arch/arm/mach-shmobile/include/mach/r8a7778.h |    2 --
 arch/arm/mach-shmobile/setup-r8a7778.c        |   14 --------------
 3 files changed, 14 insertions(+), 17 deletions(-)

diff --git a/arch/arm/mach-shmobile/board-bockw.c b/arch/arm/mach-shmobile/board-bockw.c
index 6f541ad..82a5559 100644
--- a/arch/arm/mach-shmobile/board-bockw.c
+++ b/arch/arm/mach-shmobile/board-bockw.c
@@ -25,6 +25,7 @@
 #include <linux/platform_device.h>
 #include <linux/regulator/fixed.h>
 #include <linux/regulator/machine.h>
+#include <linux/sh_eth.h>
 #include <linux/smsc911x.h>
 #include <linux/spi/spi.h>
 #include <linux/spi/flash.h>
@@ -83,6 +84,12 @@ static struct sh_mobile_sdhi_info sdhi0_info = {
 	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT,
 };
 
+/* Ether */
+static struct resource ether_resources[] __initdata = {
+	DEFINE_RES_MEM(0xfde00000, 0x400),
+	DEFINE_RES_IRQ(gic_iid(0x89)),
+};
+
 static struct sh_eth_plat_data ether_platform_data __initdata = {
 	.phy		= 0x01,
 	.edmac_endian	= EDMAC_LITTLE_ENDIAN,
@@ -179,7 +186,6 @@ static void __init bockw_init(void)
 	r8a7778_clock_init();
 	r8a7778_init_irq_extpin(1);
 	r8a7778_add_standard_devices();
-	r8a7778_add_ether_device(&ether_platform_data);
 	r8a7778_add_i2c_device(0);
 	r8a7778_add_hspi_device(0);
 	r8a7778_add_mmc_device(&sh_mmcif_plat);
@@ -193,6 +199,13 @@ static void __init bockw_init(void)
 				  ARRAY_SIZE(bockw_pinctrl_map));
 	r8a7778_pinmux_init();
 
+	platform_device_register_resndata(
+		&platform_bus, "r8a777x-ether", -1,
+		ether_resources,
+		ARRAY_SIZE(ether_resources),
+		&ether_platform_data,
+		sizeof(struct sh_eth_plat_data));
+
 	/* for SMSC */
 	base = ioremap_nocache(FPGA, SZ_1M);
 	if (base) {
diff --git a/arch/arm/mach-shmobile/include/mach/r8a7778.h b/arch/arm/mach-shmobile/include/mach/r8a7778.h
index 5d0e120..fab62d0 100644
--- a/arch/arm/mach-shmobile/include/mach/r8a7778.h
+++ b/arch/arm/mach-shmobile/include/mach/r8a7778.h
@@ -20,12 +20,10 @@
 
 #include <linux/mmc/sh_mmcif.h>
 #include <linux/mmc/sh_mobile_sdhi.h>
-#include <linux/sh_eth.h>
 #include <linux/platform_data/usb-rcar-phy.h>
 
 extern void r8a7778_add_standard_devices(void);
 extern void r8a7778_add_standard_devices_dt(void);
-extern void r8a7778_add_ether_device(struct sh_eth_plat_data *pdata);
 extern void r8a7778_add_i2c_device(int id);
 extern void r8a7778_add_hspi_device(int id);
 extern void r8a7778_add_mmc_device(struct sh_mmcif_plat_data *info);
diff --git a/arch/arm/mach-shmobile/setup-r8a7778.c b/arch/arm/mach-shmobile/setup-r8a7778.c
index a3a2e37..21db023 100644
--- a/arch/arm/mach-shmobile/setup-r8a7778.c
+++ b/arch/arm/mach-shmobile/setup-r8a7778.c
@@ -188,20 +188,6 @@ static struct platform_device_info hci##_info __initdata = {	\
 USB_PLATFORM_INFO(ehci);
 USB_PLATFORM_INFO(ohci);
 
-/* Ether */
-static struct resource ether_resources[] __initdata = {
-	DEFINE_RES_MEM(0xfde00000, 0x400),
-	DEFINE_RES_IRQ(gic_iid(0x89)),
-};
-
-void __init r8a7778_add_ether_device(struct sh_eth_plat_data *pdata)
-{
-	platform_device_register_resndata(&platform_bus, "r8a777x-ether", -1,
-					  ether_resources,
-					  ARRAY_SIZE(ether_resources),
-					  pdata, sizeof(*pdata));
-}
-
 /* PFC/GPIO */
 static struct resource pfc_resources[] __initdata = {
 	DEFINE_RES_MEM(0xfffc0000, 0x118),
-- 
1.7.9.5

Re: [PATCH 01/11] ARM: shmobile: r8a7778: cleanup registration of sh_eth

[Sergei Shtylyov]

On 11.07.2013 12:31, Kuninori Morimoto wrote:

> sh_eth driver which needs platform data at the time of
> registration is used from BockW only.
> Now, ARM/shmobile aims to support DT,
> and the C code base board support will be removed in the future.
> The driver registration method which needs platform data
> and which is not shared complicates codes.
> This patch registers it on board code as cleanup C code

> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

    NAK.

WBR, Sergei

Re: [PATCH 01/11] ARM: shmobile: r8a7778: cleanup registration of sh_eth

[Simon Horman]

On Thu, Jul 11, 2013 at 03:25:55PM +0400, Sergei Shtylyov wrote:
> On 11.07.2013 12:31, Kuninori Morimoto wrote:
> 
> >sh_eth driver which needs platform data at the time of
> >registration is used from BockW only.
> >Now, ARM/shmobile aims to support DT,
> >and the C code base board support will be removed in the future.
> >The driver registration method which needs platform data
> >and which is not shared complicates codes.
> >This patch registers it on board code as cleanup C code
> 
> >Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
>    NAK.

Please provide some reasoning for your objection to this change.
Likewise for other patches in this series to which you have replied
in the same manner. I for one do not understand what it is you object to.

Re: [PATCH 01/11] ARM: shmobile: r8a7778: cleanup registration of sh_eth

[Sergei Shtylyov]

Hello.

On 12-07-2013 4:56, Simon Horman wrote:

>>> sh_eth driver which needs platform data at the time of
>>> registration is used from BockW only.
>>> Now, ARM/shmobile aims to support DT,
>>> and the C code base board support will be removed in the future.
>>> The driver registration method which needs platform data
>>> and which is not shared complicates codes.
>>> This patch registers it on board code as cleanup C code

>>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

>>     NAK.

> Please provide some reasoning for your objection to this change.
> Likewise for other patches in this series to which you have replied
> in the same manner. I for one do not understand what it is you object to.

    Sorry, I was just out of words when I saw this. I for one do not 
understand how this change will help the DT support and to me it seems 
no more than a pointless churn and step backward from what we had. Under 
no circumstances I will accept this change -- I'm totally opposed to the 
idea of moving the SoC devices to the board code.

WBR, Sergei

Re: [PATCH 01/11] ARM: shmobile: r8a7778: cleanup registration of sh_eth

[Simon Horman]

On Fri, Jul 12, 2013 at 03:05:28PM +0400, Sergei Shtylyov wrote:
> Hello.
> 
> On 12-07-2013 4:56, Simon Horman wrote:
> 
> >>>sh_eth driver which needs platform data at the time of
> >>>registration is used from BockW only.
> >>>Now, ARM/shmobile aims to support DT,
> >>>and the C code base board support will be removed in the future.
> >>>The driver registration method which needs platform data
> >>>and which is not shared complicates codes.
> >>>This patch registers it on board code as cleanup C code
> 
> >>>Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> >>    NAK.
> 
> >Please provide some reasoning for your objection to this change.
> >Likewise for other patches in this series to which you have replied
> >in the same manner. I for one do not understand what it is you object to.
> 
>    Sorry, I was just out of words when I saw this. I for one do not
> understand how this change will help the DT support and to me it
> seems no more than a pointless churn and step backward from what we
> had. Under no circumstances I will accept this change -- I'm totally
> opposed to the idea of moving the SoC devices to the board code.

Hi,

I believe that the crux of your concern is that the way that
SH ethernet support has been added recently is to place SoC portions
in setup-xxx.c and board-specific portions into board-YYY.c

That much seems reasonable to me.

And furthermore you don't see how this change aids adding DT support.

This question also seems reasonable to me.

Magnus, Morimoto-san, could you explain the motivation for this change
more clearly?

Re: [PATCH 01/11] ARM: shmobile: r8a7778: cleanup registration of sh_eth

[Kuninori Morimoto]

Hi

> > >>    NAK.
> > 
> > >Please provide some reasoning for your objection to this change.
> > >Likewise for other patches in this series to which you have replied
> > >in the same manner. I for one do not understand what it is you object to.
> > 
> >    Sorry, I was just out of words when I saw this. I for one do not
> > understand how this change will help the DT support and to me it
> > seems no more than a pointless churn and step backward from what we
> > had. Under no circumstances I will accept this change -- I'm totally
> > opposed to the idea of moving the SoC devices to the board code.
(snip)
> And furthermore you don't see how this change aids adding DT support.
> 
> This question also seems reasonable to me.

Now, we are aiming to support DT.
If we have perfect DT support, these all r8a77xx_add_xxx_device()
and board-xxx.c will be removed, since these are not needed on DT any more.
(board-xxxx-reference.c will be survived)

Now, our r8a77xx_add_xxx_device() has 2 types.
 - r8a77xx_add_xxx_device()
 - r8a77xx_add_xxx_device(pdata)

In DT point of view, "main" device setting code will be exist on r8a77xx.dtsi,
and we can add "additional" settings on r8a77xx-board.dts.
So, these will be replaced like this

 - r8a77xx_add_xxx_device()      -> r8a77xx.dtsi
 - r8a77xx_add_xxx_device(pdata) -> r8a77xx.dtsi + r8a77xx-board.dts

In C base code point of view,
of course we need r8a77xx_add_xxx_device(pdata) for code sharing
if we have some boards which are using same SoC.
But, As you know, we have 1 board - 1 SoC pair only.
So now, we can cleanup these code, because
 1) we don't need this kind of non-shared sharing code
 2) these code will be removed if we have DT support
 3) these non-shared sharing code complicates code

When we have DT support,
"remove un-necessary board code" is more easy than
"remove un-necessary board code" + "remove unused code from setup.c"

On r8a77xx_add_xxx_device() case,
these are automatically registered from SoC code,
and this is same style with DT.

On this style, some non-necessary driver will be registered automatically,
and it will waste some memory.
But now we have enough memory, and Sergei is already understanding
that why these styles don't become trouble, since he is using same style on USB code.

> I believe that the crux of your concern is that the way that
> SH ethernet support has been added recently is to place SoC portions
> in setup-xxx.c and board-specific portions into board-YYY.c

I wonder where can I find this
"recently added board code which support sh ethernet, and which needs SoC" ?

Best regards
---
Kuninori Morimoto

Re: [PATCH 01/11] ARM: shmobile: r8a7778: cleanup registration of sh_eth

[Simon Horman]

On Wed, Jul 17, 2013 at 06:52:05PM -0700, Kuninori Morimoto wrote:
> 
> Hi
> 
> > > >>    NAK.
> > > 
> > > >Please provide some reasoning for your objection to this change.
> > > >Likewise for other patches in this series to which you have replied
> > > >in the same manner. I for one do not understand what it is you object to.
> > > 
> > >    Sorry, I was just out of words when I saw this. I for one do not
> > > understand how this change will help the DT support and to me it
> > > seems no more than a pointless churn and step backward from what we
> > > had. Under no circumstances I will accept this change -- I'm totally
> > > opposed to the idea of moving the SoC devices to the board code.
> (snip)
> > And furthermore you don't see how this change aids adding DT support.
> > 
> > This question also seems reasonable to me.
> 
> Now, we are aiming to support DT.
> If we have perfect DT support, these all r8a77xx_add_xxx_device()
> and board-xxx.c will be removed, since these are not needed on DT any more.
> (board-xxxx-reference.c will be survived)
> 
> Now, our r8a77xx_add_xxx_device() has 2 types.
>  - r8a77xx_add_xxx_device()
>  - r8a77xx_add_xxx_device(pdata)
> 
> In DT point of view, "main" device setting code will be exist on r8a77xx.dtsi,
> and we can add "additional" settings on r8a77xx-board.dts.
> So, these will be replaced like this
> 
>  - r8a77xx_add_xxx_device()      -> r8a77xx.dtsi
>  - r8a77xx_add_xxx_device(pdata) -> r8a77xx.dtsi + r8a77xx-board.dts
> 
> In C base code point of view,
> of course we need r8a77xx_add_xxx_device(pdata) for code sharing
> if we have some boards which are using same SoC.
> But, As you know, we have 1 board - 1 SoC pair only.
> So now, we can cleanup these code, because
>  1) we don't need this kind of non-shared sharing code
>  2) these code will be removed if we have DT support
>  3) these non-shared sharing code complicates code

But by that logic we could add all device initialisation
into board code for the case where we only support one board
for a given SoC.

This change seems to treat sh eth as a special case.
I don't have a particularly strong opinion about it.
But it doesn't seem like a good idea to me.

> When we have DT support,
> "remove un-necessary board code" is more easy than
> "remove un-necessary board code" + "remove unused code from setup.c"

Sure, I accept that.
But is it really that much easier?
Does this series remove all the other unused code from setup.c?
Does it really matter if we forget to remove some bits of setup.c
once they are unused?

> On r8a77xx_add_xxx_device() case,
> these are automatically registered from SoC code,
> and this is same style with DT.
> 
> On this style, some non-necessary driver will be registered automatically,
> and it will waste some memory.
> But now we have enough memory, and Sergei is already understanding
> that why these styles don't become trouble, since he is using same style on USB code.
> 
> > I believe that the crux of your concern is that the way that
> > SH ethernet support has been added recently is to place SoC portions
> > in setup-xxx.c and board-specific portions into board-YYY.c
> 
> I wonder where can I find this
> "recently added board code which support sh ethernet, and which needs SoC" ?

I am referring to the code that you are changing, sorry for not being
clearer.

In board-bockw.c there is ether_platform_data which provides
board-specific configuration. And in setup-r8a7778.c there
is ether_resources which provides SoC specific information.

Re: [PATCH 01/11] ARM: shmobile: r8a7778: cleanup registration of sh_eth

[Kuninori Morimoto]

Hi Simon

Thank you for your reply

> > In C base code point of view,
> > of course we need r8a77xx_add_xxx_device(pdata) for code sharing
> > if we have some boards which are using same SoC.
> > But, As you know, we have 1 board - 1 SoC pair only.
> > So now, we can cleanup these code, because
> >  1) we don't need this kind of non-shared sharing code
> >  2) these code will be removed if we have DT support
> >  3) these non-shared sharing code complicates code
> 
> But by that logic we could add all device initialisation
> into board code for the case where we only support one board
> for a given SoC.

The target of these cleanup codes are r8a77xx_add_xxx_device(pdata) type,
not r8a77xx_add_xxx_device() at this point,
But the later will be removed if we have DT support.

The confusing things is that we have DT/non-DT code.
I don't move r8a77xx_add_xxx_device() from setup code to board code,
because we have board-xxx-reference.c.
This means these are shared.

> But is it really that much easier?
> Does this series remove all the other unused code from setup.c?
> Does it really matter if we forget to remove some bits of setup.c
> once they are unused?

The point is not "unused code", but "non-shared sharing code".
I guess people use copy-paste base development.
So, if we don't clean-up these "non-shared sharing code" now,
we will have more and more r8a77xx_add_xxx_device(pdata) type method
(which is the "non-shared sharing code" :)
in both board-xxx.c and setup-xxx.c before we have DT support.
This means clean-up these "after" DT support will be more complex.
I don't want this type of nightmare.

> In board-bockw.c there is ether_platform_data which provides
> board-specific configuration. And in setup-r8a7778.c there
> is ether_resources which provides SoC specific information.
(snip)
> This change seems to treat sh eth as a special case.
> I don't have a particularly strong opinion about it.
> But it doesn't seem like a good idea to me.

Sorry, I still can't understand about this...
bockw - r8a7778 sh_eth clean-up code is caring
its resource (from setup) and board configuration (from bockw).
I don't think there is conflict, but do I mistake ?

Do you mean r8a7779 sh_eth clean-up code ?
If so, 1) I can't find "board" code which is using r8a7779_add_ether_device(xx),
2) I guess marzen board doesn't have sh_eth HW implementation.

Best regards
---
Kuninori Morimoto

Re: [PATCH 01/11] ARM: shmobile: r8a7778: cleanup registration of sh_eth

[Sergei Shtylyov]

Hello.

On 18-07-2013 3:11, Simon Horman wrote:

>>>>> sh_eth driver which needs platform data at the time of
>>>>> registration is used from BockW only.
>>>>> Now, ARM/shmobile aims to support DT,
>>>>> and the C code base board support will be removed in the future.
>>>>> The driver registration method which needs platform data
>>>>> and which is not shared complicates codes.
>>>>> This patch registers it on board code as cleanup C code

>>>>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

>>>>     NAK.

>>> Please provide some reasoning for your objection to this change.
>>> Likewise for other patches in this series to which you have replied
>>> in the same manner. I for one do not understand what it is you object to.

>>     Sorry, I was just out of words when I saw this. I for one do not
>> understand how this change will help the DT support and to me it
>> seems no more than a pointless churn and step backward from what we
>> had. Under no circumstances I will accept this change -- I'm totally
>> opposed to the idea of moving the SoC devices to the board code.

> Hi,

> I believe that the crux of your concern is that the way that
> SH ethernet support has been added recently is to place SoC portions
> in setup-xxx.c and board-specific portions into board-YYY.c

    Not only Ether but all the SoC devices. Hence were the NAKs for other patches.

WBR, Sergei

Re: [PATCH 01/11] ARM: shmobile: r8a7778: cleanup registration of sh_eth

[Sergei Shtylyov]

Hello.

On 18-07-2013 10:35, Kuninori Morimoto wrote:

>> But is it really that much easier?
>> Does this series remove all the other unused code from setup.c?
>> Does it really matter if we forget to remove some bits of setup.c
>> once they are unused?

> The point is not "unused code", but "non-shared sharing code".
> I guess people use copy-paste base development.
> So, if we don't clean-up these "non-shared sharing code" now,
> we will have more and more r8a77xx_add_xxx_device(pdata) type method
> (which is the "non-shared sharing code" :)

    And that will teach them doing things the right, scalable way from the start.

> in both board-xxx.c and setup-xxx.c before we have DT support.
> This means clean-up these "after" DT support will be more complex.
> I don't want this type of nightmare.

    And I don't want "SoC device in the board code" non-scalable nightmare
when people will start producing their own boards based on Renesas SoCs. I 
don't want to give them the bad example that your USB code was before my 
9-patch cleanup, and another several bad examples which you're now to trying 
to push into renesas.git. I don't want my efforts wasted.

> Do you mean r8a7779 sh_eth clean-up code ?
> If so, 1) I can't find "board" code which is using r8a7779_add_ether_device(xx),
> 2) I guess marzen board doesn't have sh_eth HW implementation.

    It has several types of daughter boards with Ethernet PHY and connector. 
See my off-list email.

> Best regards
> ---
> Kuninori Morimoto

WBR, Sergei

Re: [PATCH 01/11] ARM: shmobile: r8a7778: cleanup registration of sh_eth

[Simon Horman]

On Thu, Jul 18, 2013 at 03:54:33PM +0400, Sergei Shtylyov wrote:
> Hello.
> 
> On 18-07-2013 3:11, Simon Horman wrote:
> 
> >>>>>sh_eth driver which needs platform data at the time of
> >>>>>registration is used from BockW only.
> >>>>>Now, ARM/shmobile aims to support DT,
> >>>>>and the C code base board support will be removed in the future.
> >>>>>The driver registration method which needs platform data
> >>>>>and which is not shared complicates codes.
> >>>>>This patch registers it on board code as cleanup C code
> 
> >>>>>Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> >>>>    NAK.
> 
> >>>Please provide some reasoning for your objection to this change.
> >>>Likewise for other patches in this series to which you have replied
> >>>in the same manner. I for one do not understand what it is you object to.
> 
> >>    Sorry, I was just out of words when I saw this. I for one do not
> >>understand how this change will help the DT support and to me it
> >>seems no more than a pointless churn and step backward from what we
> >>had. Under no circumstances I will accept this change -- I'm totally
> >>opposed to the idea of moving the SoC devices to the board code.
> 
> >Hi,
> 
> >I believe that the crux of your concern is that the way that
> >SH ethernet support has been added recently is to place SoC portions
> >in setup-xxx.c and board-specific portions into board-YYY.c
> 
>    Not only Ether but all the SoC devices. Hence were the NAKs for other patches.

I believe that I agree with your concerns.
Morimoto-san, could you drop these cleanup patches?

Re: [PATCH 01/11] ARM: shmobile: r8a7778: cleanup registration of sh_eth

[Magnus Damm]

Hello Sergei,

On Fri, Jul 12, 2013 at 8:05 PM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> Hello.
>
>
> On 12-07-2013 4:56, Simon Horman wrote:
>
>>>> sh_eth driver which needs platform data at the time of
>>>> registration is used from BockW only.
>>>> Now, ARM/shmobile aims to support DT,
>>>> and the C code base board support will be removed in the future.
>>>> The driver registration method which needs platform data
>>>> and which is not shared complicates codes.
>>>> This patch registers it on board code as cleanup C code
>
>
>>>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>
>
>>>     NAK.
>
>
>> Please provide some reasoning for your objection to this change.
>> Likewise for other patches in this series to which you have replied
>> in the same manner. I for one do not understand what it is you object to.
>
>
>    Sorry, I was just out of words when I saw this. I for one do not
> understand how this change will help the DT support and to me it seems no
> more than a pointless churn and step backward from what we had. Under no
> circumstances I will accept this change -- I'm totally opposed to the idea
> of moving the SoC devices to the board code.

Thanks for emailing once more and explaining your reasons behind the
NAK. Please allow me to step in here and explain things a bit.

Originally on SH and ARM mach-shmobile we have divided platform
devices into two categories - SoC specific and board specific. As
expected, SoC specific devices go in setup-xxx.c and board specific
devices go into board-xxx.c. So far so good.

It may become a bit more unclear in some cases when part of the
platform device data is board specific but other parts are SoC
specific. Historically we have simply made those board specific. For
various reasons I'd like to keep the code that way.

You may now ask why do I want to keep the code that way. One reason is
that adding new abstractions and functions for SoCs is pointless
unless we have multiple boards upstream using the same SoC code. Which
we don't really have. So adding code following that style will just
bloat the code base. I do however think that sharing data in a more
efficient way is a good thing for the future, but I believe that
discussion should happen with DT reference implementations. So please
put your focus there if you want to improve things. The legacy C SoC
and board code is no place for innovation.

As you know, the legacy C code for r8a7778 at this point includes
various functions in the SoC code that introduces some alternative
more verbose way to deal with platform devices. This has been added
without changing the style for other SoCs under mach-shmobile. This
means that the r8a7778 code is implemented differently than the other
SoCs. This becomes a nightmare for me since I'm pretty much the only
person who deals with tree wide changes and understands how the
MACHINE_START bits and the init order works for different cases.
Because of this I want all the legacy C SoC and board code to follow
the same style and behave the same way. Moving code around here just
for fun makes things difficult with no real upside IMO.

Morimoto-san has kindly helped me to rework the r8a7778 code into
something more standard which I really appreciate. Because of this I'm
going to ask Simon to merge his patches to make r8a7778 follow the
same style as the rest of mach-shmobile.

If you want to help defining next generation board and SoC design then
please work with us how the DT reference SoC and board support should
look like.

Thanks,

/ magnus

Re: [PATCH 01/11] ARM: shmobile: r8a7778: cleanup registration of sh_eth

[Sergei Shtylyov]

Hello.

On 24-07-2013 8:45, Magnus Damm wrote:

>>>>> sh_eth driver which needs platform data at the time of
>>>>> registration is used from BockW only.
>>>>> Now, ARM/shmobile aims to support DT,
>>>>> and the C code base board support will be removed in the future.
>>>>> The driver registration method which needs platform data
>>>>> and which is not shared complicates codes.
>>>>> This patch registers it on board code as cleanup C code

>>>>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

>>>>      NAK.

>>> Please provide some reasoning for your objection to this change.
>>> Likewise for other patches in this series to which you have replied
>>> in the same manner. I for one do not understand what it is you object to.

>>     Sorry, I was just out of words when I saw this. I for one do not
>> understand how this change will help the DT support and to me it seems no
>> more than a pointless churn and step backward from what we had. Under no
>> circumstances I will accept this change -- I'm totally opposed to the idea
>> of moving the SoC devices to the board code.

> Thanks for emailing once more and explaining your reasons behind the
> NAK. Please allow me to step in here and explain things a bit.

> Originally on SH and ARM mach-shmobile we have divided platform
> devices into two categories - SoC specific and board specific. As
> expected, SoC specific devices go in setup-xxx.c and board specific
> devices go into board-xxx.c. So far so good.

> It may become a bit more unclear in some cases when part of the
> platform device data is board specific but other parts are SoC
> specific. Historically we have simply made those board specific. For
> various reasons I'd like to keep the code that way.

    I beg to differ. USB PHY doesn't have SoC specific platform data, yet 
Morimoto-san is again moving it to the board code. Why?
    Also, concerning sh_eth device: hopefully, it's going to lose its SoC 
specific platform data soon due to my efforts. What then, move the device to 
the SoC code again?

> You may now ask why do I want to keep the code that way. One reason is
> that adding new abstractions and functions for SoCs is pointless
> unless we have multiple boards upstream using the same SoC code. Which
> we don't really have.

    It's not pointless if you look further into the future.

> So adding code following that style will just
> bloat the code base.

    I don't see any significant bloat, frankly.

> I do however think that sharing data in a more
> efficient way is a good thing for the future, but I believe that
> discussion should happen with DT reference implementations. So please
> put your focus there if you want to improve things. The legacy C SoC
> and board code is no place for innovation.

    We weren't aware of that (somewhat dubious) policy before. You should have 
spoken much earlier.

> As you know, the legacy C code for r8a7778 at this point includes
> various functions in the SoC code that introduces some alternative
> more verbose way to deal with platform devices. This has been added
> without changing the style for other SoCs under mach-shmobile.

    Not true. We also changed R8A7779 code in the same way. We have no 
obligations from Renesas with regards to the other mach-shmobile code
and we have no bandwidth for global cleanups.

> This
> means that the r8a7778 code is implemented differently than the other
> SoCs. This becomes a nightmare for me since I'm pretty much the only
> person who deals with tree wide changes and understands how the
> MACHINE_START bits and the init order works for different cases.
> Because of this I want all the legacy C SoC and board code to follow
> the same style and behave the same way. Moving code around here just
> for fun makes things difficult with no real upside IMO.

> Morimoto-san has kindly helped me to rework the r8a7778 code into
> something more standard which I really appreciate. Because of this I'm
> going to ask Simon to merge his patches to make r8a7778 follow the
> same style as the rest of mach-shmobile.

    I'd like to hear replies to my unanswered questions first as 
Morimoto-san's seems to have gotten a habit of simply ignoring my questions. 
Also, I think his changelogs are not very adequate in the light of what you 
said here.

> If you want to help defining next generation board and SoC design then
> please work with us how the DT reference SoC and board support should
> look like.

    We have our areas of work from Renesas for which we're paid (and to which 
Renesas periodically adds new tasks with higher priority), and typically have 
no bandwidth to work on something else. DT support is quite low in our 
priority lists, though it's definitely there.

> Thanks,

> / magnus

WBR, Sergei

Re: [PATCH 01/11] ARM: shmobile: r8a7778: cleanup registration of sh_eth

[Magnus Damm]

Hi Sergei,

On Wed, Jul 24, 2013 at 9:58 PM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> Hello.
>
>
> On 24-07-2013 8:45, Magnus Damm wrote:
>
>>>>>> sh_eth driver which needs platform data at the time of
>>>>>> registration is used from BockW only.
>>>>>> Now, ARM/shmobile aims to support DT,
>>>>>> and the C code base board support will be removed in the future.
>>>>>> The driver registration method which needs platform data
>>>>>> and which is not shared complicates codes.
>>>>>> This patch registers it on board code as cleanup C code
>
>
>>>>>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>
>
>>>>>      NAK.
>
>
>>>> Please provide some reasoning for your objection to this change.
>>>> Likewise for other patches in this series to which you have replied
>>>> in the same manner. I for one do not understand what it is you object
>>>> to.
>
>
>>>     Sorry, I was just out of words when I saw this. I for one do not
>>> understand how this change will help the DT support and to me it seems no
>>> more than a pointless churn and step backward from what we had. Under no
>>> circumstances I will accept this change -- I'm totally opposed to the
>>> idea
>>> of moving the SoC devices to the board code.
>
>
>> Thanks for emailing once more and explaining your reasons behind the
>> NAK. Please allow me to step in here and explain things a bit.
>
>
>> Originally on SH and ARM mach-shmobile we have divided platform
>> devices into two categories - SoC specific and board specific. As
>> expected, SoC specific devices go in setup-xxx.c and board specific
>> devices go into board-xxx.c. So far so good.
>
>
>> It may become a bit more unclear in some cases when part of the
>> platform device data is board specific but other parts are SoC
>> specific. Historically we have simply made those board specific. For
>> various reasons I'd like to keep the code that way.
>
>
>    I beg to differ.

What? You mean that that's not the way we did things the last 5 years?

> USB PHY doesn't have SoC specific platform data, yet
> Morimoto-san is again moving it to the board code. Why?

I don't know, why don't you ask him? Frankly I don't care very much either.

>    Also, concerning sh_eth device: hopefully, it's going to lose its SoC
> specific platform data soon due to my efforts. What then, move the device to
> the SoC code again?

I trust Morimoto-san to do whatever he needs to do to make the code
follow the same style as the rest of the mach-shmobile stuff.

>> You may now ask why do I want to keep the code that way. One reason is
>> that adding new abstractions and functions for SoCs is pointless
>> unless we have multiple boards upstream using the same SoC code. Which
>> we don't really have.
>
>
>    It's not pointless if you look further into the future.

Further into the future is DT, and because of that there is no need to
optimize the C case. It's as simple as that. You are welcome to help
out with the DT bits.

>> So adding code following that style will just
>> bloat the code base.
>
>
>    I don't see any significant bloat, frankly.

I guess it's a matter of preference. In your case you add a function
prototype to the SoC header, add a special function, then call that
from the board. Even though there is only a single board. With that
you have to look at multiple places to find both platform data and
resources. It's not the same as the other SoCs and I dislike that.

For the single board case not doing that will save some lines and
remove pointless abstractions. If you have multiple boards for
upstream then we can of course reconsider. But until then it's all
pointless abstractions.

>> I do however think that sharing data in a more
>> efficient way is a good thing for the future, but I believe that
>> discussion should happen with DT reference implementations. So please
>> put your focus there if you want to improve things. The legacy C SoC
>> and board code is no place for innovation.
>
>
>    We weren't aware of that (somewhat dubious) policy before. You should
> have spoken much earlier.

Well, maybe so. But the person that implemented the r8a7778 could also
have followed the same style as the rest of the subarchitecture. I
can't see how that would be so difficult. Regardless it's no big deal
since we can easily move things around.

>> As you know, the legacy C code for r8a7778 at this point includes
>> various functions in the SoC code that introduces some alternative
>> more verbose way to deal with platform devices. This has been added
>> without changing the style for other SoCs under mach-shmobile.
>
>
>    Not true. We also changed R8A7779 code in the same way. We have no
> obligations from Renesas with regards to the other mach-shmobile code
> and we have no bandwidth for global cleanups.

So now you have 1.5 special case of out 8 SoCs. But I want them all to
be uniform.

>> This
>> means that the r8a7778 code is implemented differently than the other
>> SoCs. This becomes a nightmare for me since I'm pretty much the only
>> person who deals with tree wide changes and understands how the
>> MACHINE_START bits and the init order works for different cases.
>> Because of this I want all the legacy C SoC and board code to follow
>> the same style and behave the same way. Moving code around here just
>> for fun makes things difficult with no real upside IMO.
>
>
>> Morimoto-san has kindly helped me to rework the r8a7778 code into
>> something more standard which I really appreciate. Because of this I'm
>> going to ask Simon to merge his patches to make r8a7778 follow the
>> same style as the rest of mach-shmobile.
>
>
>    I'd like to hear replies to my unanswered questions first as
> Morimoto-san's seems to have gotten a habit of simply ignoring my questions.
> Also, I think his changelogs are not very adequate in the light of what you
> said here.

I don't know about the change logs. I think he is quite useful. He is
not very noisy and he doesn't NAK randomly. He also doesn't comment on
minor details.

>> If you want to help defining next generation board and SoC design then
>> please work with us how the DT reference SoC and board support should
>> look like.
>
>    We have our areas of work from Renesas for which we're paid (and to which
> Renesas periodically adds new tasks with higher priority), and typically
> have no bandwidth to work on something else. DT support is quite low in our
> priority lists, though it's definitely there.

Well, I thought so. I'm just staying that you can use your code rework
energy somewhere it really matters instead of simply moving platform
devices around.

Cheers,

/ magnus