ARM: mackerel: extended DT support

Two of four interrupt controllers in sh7372.dtsi are missing the compulsory "#interrupt-cells" properties. Fix this.

Mackerel's .dts Device Tree description file should derive from the SoC's .dtsi, not from skeleton.dtsi directly.

Signed-off-by Guennadi Liakhovetski <[email protected]>

Extend DT interrupt controller initialisation to automatically fall back to platform data based configuration, if booting without DT. This simplifies implementing boards, capable of booting in either mode with a single kernel.

Signed-off-by Guennadi Liakhovetski <[email protected]>

---
 arch/arm/mach-shmobile/intc-sh7372.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

arch/arm/mach-shmobile/intc-sh7372.c +6 -0

index c923518..9c13ecc 100644
@@ -23,6 +23,7 @@
 #include <linux/irq.h>
 #include <linux/io.h>
 #include <linux/sh_intc.h>
+#include <mach/common.h>
 #include <mach/intc.h>
 #include <mach/irqs.h>
 #include <asm/mach-types.h>

@@ -629,6 +630,11 @@ static const struct of_device_id irq_of_match[] __initconst = {
 
 void __init sh7372_init_irq_of(void)
 {
+	if (!of_have_populated_dt()) {
+		sh7372_init_irq();
+		return;
+	}
+
 	of_irq_init(irq_of_match);
 
 	sh7372_init_intc(0xe6940000, 0xe6950000, 0xffd20000, 0xffd50000,

Signature · 1 line

1.7.2.5

Quoted 3 lines · Guennadi Liakhovetski

Extend DT interrupt controller initialisation to automatically fall back to platform data based configuration, if booting without DT. This simplifies implementing boards, capable of booting in either mode with a single kernel.

Hi Guennadi,

Do you have a case in mind where this will be used?
My thinking until now has been that sh7372_init_irq_of() should only be called when a board is being initialised using DT.

Quoted 28 lines

Signed-off-by Guennadi Liakhovetski <[email protected]>

---
 arch/arm/mach-shmobile/intc-sh7372.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

arch/arm/mach-shmobile/intc-sh7372.c +6 -0

index c923518..9c13ecc 100644
@@ -23,6 +23,7 @@
 #include <linux/irq.h>
 #include <linux/io.h>
 #include <linux/sh_intc.h>
+#include <mach/common.h>
 #include <mach/intc.h>
 #include <mach/irqs.h>
 #include <asm/mach-types.h>

@@ -629,6 +630,11 @@ static const struct of_device_id irq_of_match[] __initconst = {
 
 void __init sh7372_init_irq_of(void)
 {
+	if (!of_have_populated_dt()) {
+		sh7372_init_irq();
+		return;
+	}
+
 	of_irq_init(irq_of_match);
 

Quoted 4 lines

arch/arm/mach-shmobile/intc-sh7372.c +0 -0

 	sh7372_init_intc(0xe6940000, 0xe6950000, 0xffd20000, 0xffd50000,

Signature · 1 line

1.7.2.5

When booting with DT, devices are named differently. To get their clocks additional entries have to be added to the lookup table.

Signed-off-by Guennadi Liakhovetski <[email protected]>

When booting with DT, devices are named differently. To get their clocks additional entries have to be added to the lookup table.

For boards booting without DT no changes should be caused by this patch. When booting with DT, devices, whose drivers support DT probing, will not be registered.

Signed-off-by Guennadi Liakhovetski <[email protected]>

---
 arch/arm/mach-shmobile/setup-sh7372.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

arch/arm/mach-shmobile/setup-sh7372.c +14 -3

index bbe6e2a..3e6bf3d 100644
@@ -981,9 +981,12 @@ static struct platform_device *sh7372_early_devices[] __initdata = {
 	&tmu01_device,
 };
 
-static struct platform_device *sh7372_late_devices[] __initdata = {
+static struct platform_device *sh7372_late_devices_dt[] __initdata = {
 	&iic0_device,
 	&iic1_device,
+};
+
+static struct platform_device *sh7372_late_devices[] __initdata = {
 	&dma0_device,
 	&dma1_device,
 	&dma2_device,

@@ -1012,13 +1015,11 @@ void __init sh7372_add_standard_devices(void)
 		{ "A3SP", &scif4_device, },
 		{ "A3SP", &scif5_device, },
 		{ "A3SP", &scif6_device, },
-		{ "A3SP", &iic1_device, },
 		{ "A3SP", &dma0_device, },
 		{ "A3SP", &dma1_device, },
 		{ "A3SP", &dma2_device, },
 		{ "A3SP", &usb_dma0_device, },
 		{ "A3SP", &usb_dma1_device, },
-		{ "A4R", &iic0_device, },
 		{ "A4R", &veu0_device, },
 		{ "A4R", &veu1_device, },
 		{ "A4R", &veu2_device, },

@@ -1027,6 +1028,10 @@ void __init sh7372_add_standard_devices(void)
 		{ "A4R", &tmu00_device, },
 		{ "A4R", &tmu01_device, },
 	};
+	struct pm_domain_device domain_devices_dt[] = {
+		{ "A3SP", &iic1_device, },
+		{ "A4R", &iic0_device, },
+	};
 
 	sh7372_init_pm_domains();
 

@@ -1035,9 +1040,15 @@ void __init sh7372_add_standard_devices(void)
 
 	platform_add_devices(sh7372_late_devices,
 			    ARRAY_SIZE(sh7372_late_devices));
+	if (!of_have_populated_dt())
+		platform_add_devices(sh7372_late_devices_dt,
+				     ARRAY_SIZE(sh7372_late_devices_dt));
 
 	rmobile_add_devices_to_domains(domain_devices,
 				       ARRAY_SIZE(domain_devices));
+	if (!of_have_populated_dt())
+		rmobile_add_devices_to_domains(domain_devices_dt,
+					       ARRAY_SIZE(domain_devices_dt));
 }
 
 static void __init sh7372_earlytimer_init(void)

Signature · 6 lines

1.7.2.5

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 3 lines · Guennadi Liakhovetski

For boards booting without DT no changes should be caused by this patch. When booting with DT, devices, whose drivers support DT probing, will not be registered.

This relates in part to my comment on "ARM: sh7372: support mixed DT and board code interrupt controller init".

It is my understanding that sh7372_add_standard_devices_dt() already exists in setup-sh7372.c and that it is appropriate to use for bring up boards with DT.

Quoted 63 lines

Signed-off-by Guennadi Liakhovetski <[email protected]>

---
 arch/arm/mach-shmobile/setup-sh7372.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

arch/arm/mach-shmobile/setup-sh7372.c +14 -3

index bbe6e2a..3e6bf3d 100644
@@ -981,9 +981,12 @@ static struct platform_device *sh7372_early_devices[] __initdata = {
 	&tmu01_device,
 };
 
-static struct platform_device *sh7372_late_devices[] __initdata = {
+static struct platform_device *sh7372_late_devices_dt[] __initdata = {
 	&iic0_device,
 	&iic1_device,
+};
+
+static struct platform_device *sh7372_late_devices[] __initdata = {
 	&dma0_device,
 	&dma1_device,
 	&dma2_device,

@@ -1012,13 +1015,11 @@ void __init sh7372_add_standard_devices(void)
 		{ "A3SP", &scif4_device, },
 		{ "A3SP", &scif5_device, },
 		{ "A3SP", &scif6_device, },
-		{ "A3SP", &iic1_device, },
 		{ "A3SP", &dma0_device, },
 		{ "A3SP", &dma1_device, },
 		{ "A3SP", &dma2_device, },
 		{ "A3SP", &usb_dma0_device, },
 		{ "A3SP", &usb_dma1_device, },
-		{ "A4R", &iic0_device, },
 		{ "A4R", &veu0_device, },
 		{ "A4R", &veu1_device, },
 		{ "A4R", &veu2_device, },

@@ -1027,6 +1028,10 @@ void __init sh7372_add_standard_devices(void)
 		{ "A4R", &tmu00_device, },
 		{ "A4R", &tmu01_device, },
 	};
+	struct pm_domain_device domain_devices_dt[] = {
+		{ "A3SP", &iic1_device, },
+		{ "A4R", &iic0_device, },
+	};
 
 	sh7372_init_pm_domains();
 

@@ -1035,9 +1040,15 @@ void __init sh7372_add_standard_devices(void)
 
 	platform_add_devices(sh7372_late_devices,
 			    ARRAY_SIZE(sh7372_late_devices));
+	if (!of_have_populated_dt())
+		platform_add_devices(sh7372_late_devices_dt,
+				     ARRAY_SIZE(sh7372_late_devices_dt));
 
 	rmobile_add_devices_to_domains(domain_devices,
 				       ARRAY_SIZE(domain_devices));
+	if (!of_have_populated_dt())
+		rmobile_add_devices_to_domains(domain_devices_dt,
+					       ARRAY_SIZE(domain_devices_dt));
 }
 

Quoted 3 lines

arch/arm/mach-shmobile/setup-sh7372.c +0 -0

 static void __init sh7372_earlytimer_init(void)

Signature · 1 line

1.7.2.5

This patch adds dynamic switching to booting either with or without DT. So far only a part of the board initialisation can be done via DT. Devices, that still need platform data are kept that way. Devices, that can be initialised from DT will not be supplied from the platform data, if a DT image is detected.

arch/arm/mach-shmobile/board-mackerel.c +29 -11

 		{ "A3SP", &usbhs0_device, },
 		{ "A3SP", &usbhs1_device, },
 		{ "A3SP", &nand_flash_device, },
+		{ "A4R", &ceu_device, },
+	};
+	struct pm_domain_device domain_devices_dt[] = {
 		{ "A3SP", &sh_mmcif_device, },
 		{ "A3SP", &sdhi0_device, },
 #if !defined(CONFIG_MMC_SH_MMCIF) && !defined(CONFIG_MMC_SH_MMCIF_MODULE)
 		{ "A3SP", &sdhi1_device, },
 #endif
 		{ "A3SP", &sdhi2_device, },
-		{ "A4R", &ceu_device, },
 	};
 	u32 srcr4;
 	struct clk *clk;
 
-	regulator_register_always_on(0, "fixed-1.8V", fixed1v8_power_consumers,
-				     ARRAY_SIZE(fixed1v8_power_consumers), 1800000);
-	regulator_register_always_on(1, "fixed-3.3V", fixed3v3_power_consumers,
-				     ARRAY_SIZE(fixed3v3_power_consumers), 3300000);
-	regulator_register_fixed(2, dummy_supplies, ARRAY_SIZE(dummy_supplies));
+	if (!of_have_populated_dt()) {
+		regulator_register_always_on(0, "fixed-1.8V", fixed1v8_power_consumers,
+					     ARRAY_SIZE(fixed1v8_power_consumers), 1800000);
+		regulator_register_always_on(1, "fixed-3.3V", fixed3v3_power_consumers,
+					     ARRAY_SIZE(fixed3v3_power_consumers), 3300000);
+		regulator_register_fixed(2, dummy_supplies, ARRAY_SIZE(dummy_supplies));
+	}
 
 	/* External clock source */
 	clk_set_rate(&sh7372_dv_clki_clk, 27000000);

@@ -1633,22 +1658,35 @@ static void __init mackerel_init(void)
 	udelay(50);
 	__raw_writel(srcr4 & ~(1 << 13), SRCR4);
 
-	i2c_register_board_info(0, i2c0_devices,
-				ARRAY_SIZE(i2c0_devices));
-	i2c_register_board_info(1, i2c1_devices,
-				ARRAY_SIZE(i2c1_devices));
+	if (!of_have_populated_dt()) {
+		i2c_register_board_info(0, i2c0_devices,
+					ARRAY_SIZE(i2c0_devices));
+		i2c_register_board_info(1, i2c1_devices,
+					ARRAY_SIZE(i2c1_devices));
+	} else {
+		bus_register_notifier(&i2c_bus_type,
+				      &mackerel_i2c_notifier);
+	}
 
 	sh7372_add_standard_devices();
 
 	platform_add_devices(mackerel_devices, ARRAY_SIZE(mackerel_devices));
+	if (!of_have_populated_dt())
+		platform_add_devices(mackerel_devices_dt,
+				     ARRAY_SIZE(mackerel_devices_dt));
 
 	rmobile_add_devices_to_domains(domain_devices,
 				       ARRAY_SIZE(domain_devices));
+	if (!of_have_populated_dt())
+		rmobile_add_devices_to_domains(domain_devices_dt,
+					       ARRAY_SIZE(domain_devices_dt));
 
 	hdmi_init_pm_clock();
 	sh7372_pm_init();
 	pm_clk_add(&fsi_device.dev, "spu2");
 	pm_clk_add(&hdmi_lcdc_device.dev, "hdmi");
+
+	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
 }
 
 static const char *mackerel_boards_compat_dt[] __initdata = {

@@ -1659,10 +1697,20 @@ static const char *mackerel_boards_compat_dt[] __initdata = {
 DT_MACHINE_START(MACKEREL_DT, "mackerel")
 	.map_io		= sh7372_map_io,
 	.init_early	= sh7372_add_early_devices,
-	.init_irq	= sh7372_init_irq,
+	.init_irq	= sh7372_init_irq_of,
+	.handle_irq	= shmobile_handle_irq_intc,

arch/arm/mach-shmobile/board-mackerel.c +0 -1

 	.handle_irq	= shmobile_handle_irq_intc,
 	.init_machine	= mackerel_init,
 	.init_late	= sh7372_pm_init_late,
 	.timer		= &shmobile_timer,
-	.dt_compat  = mackerel_boards_compat_dt,
 MACHINE_END

Signature · 6 lines

1.7.2.5

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 5 lines · Guennadi Liakhovetski

This patch adds dynamic switching to booting either with or without DT. So far only a part of the board initialisation can be done via DT. Devices, that still need platform data are kept that way. Devices, that can be initialised from DT will not be supplied from the platform data, if a DT image is detected.

Hi Guennadi,

I'm not sure that I'm entirely comfortable with the implications for users here.

In the beginning there was no DT for Mackerel, all boots were non-DT, and in general the board and its devices have been well supported.

At the present time Mackerel only boots using DT, though almost all the initialisation is done in C. Thus currently a DT boot fairly full support for the board and its devices.

If I understand things correctly, with this change, a DT boot becomes a limited boot that basically only supports devices that can be initialised using DT. And users are expected to go back to a non-DT boot if they want fuller support. This seems undesirable.

The approach that I took with the kzm9g was to provide an alternate dts and board file, and compatibility string. Clearly that is not an entirely elegant solution. But it does avoid the problem that I describe above.

Quoted 66 lines

Signed-off-by Guennadi Liakhovetski <[email protected]>

---
 arch/arm/mach-shmobile/board-mackerel.c |   84 ++++++++++++++++++++++++-------
 1 files changed, 66 insertions(+), 18 deletions(-)

arch/arm/mach-shmobile/board-mackerel.c +26 -6

index 39b8f2e..a6358c9 100644
@@ -1326,7 +1326,6 @@ static struct platform_device mackerel_camera = {
 
 static struct platform_device *mackerel_devices[] __initdata = {
 	&nor_flash_device,
-	&smc911x_device,
 	&lcdc_device,
 	&usbhs0_device,
 	&usbhs1_device,

@@ -1335,17 +1334,21 @@ static struct platform_device *mackerel_devices[] __initdata = {
 	&fsi_ak4643_device,
 	&fsi_hdmi_device,
 	&nand_flash_device,
+	&ceu_device,
+	&mackerel_camera,
+	&hdmi_device,
+	&hdmi_lcdc_device,
+	&meram_device,
+};
+
+static struct platform_device *mackerel_devices_dt[] __initdata = {
+	&smc911x_device,
 	&sdhi0_device,
 #if !defined(CONFIG_MMC_SH_MMCIF) && !defined(CONFIG_MMC_SH_MMCIF_MODULE)
 	&sdhi1_device,
 #endif
 	&sdhi2_device,
 	&sh_mmcif_device,
-	&ceu_device,
-	&mackerel_camera,
-	&hdmi_device,
-	&hdmi_lcdc_device,
-	&meram_device,
 };
 
 /* Keypad Initialization */

@@ -1404,6 +1407,24 @@ static struct i2c_board_info i2c1_devices[] = {
 	},
 };
 
+static int mackerel_i2c_bus_notify(struct notifier_block *nb,
+				   unsigned long action, void *data)
+{
+	struct device *dev = data;
+
+	if (action != BUS_NOTIFY_ADD_DEVICE ||
+	    strcmp(dev_name(dev->parent), "fff20000.i2c"))
+		return NOTIFY_DONE;
+
+	i2c_new_device(to_i2c_adapter(dev), &i2c0_devices[1]);
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block mackerel_i2c_notifier = {
+	.notifier_call = mackerel_i2c_bus_notify,
+};

arch/arm/mach-shmobile/board-mackerel.c static void __init mackerel_init(void)

Quoted 5 lines

arch/arm/mach-shmobile/board-mackerel.c +1 -0

+
 #define GPIO_PORT9CR	IOMEM(0xE6051009)
 #define GPIO_PORT10CR	IOMEM(0xE605100A)
 #define GPIO_PORT167CR	IOMEM(0xE60520A7)

@@ -1420,22 +1441,26 @@ static void __init mackerel_init(void)

I wonder if it would be cleaner to achieve this by providing mackerel_init_dt().

Quoted 83 lines

arch/arm/mach-shmobile/board-mackerel.c +33 -11

 		{ "A3SP", &usbhs0_device, },
 		{ "A3SP", &usbhs1_device, },
 		{ "A3SP", &nand_flash_device, },
+		{ "A4R", &ceu_device, },
+	};
+	struct pm_domain_device domain_devices_dt[] = {
 		{ "A3SP", &sh_mmcif_device, },
 		{ "A3SP", &sdhi0_device, },
 #if !defined(CONFIG_MMC_SH_MMCIF) && !defined(CONFIG_MMC_SH_MMCIF_MODULE)
 		{ "A3SP", &sdhi1_device, },
 #endif
 		{ "A3SP", &sdhi2_device, },
-		{ "A4R", &ceu_device, },
 	};
 	u32 srcr4;
 	struct clk *clk;
 
-	regulator_register_always_on(0, "fixed-1.8V", fixed1v8_power_consumers,
-				     ARRAY_SIZE(fixed1v8_power_consumers), 1800000);
-	regulator_register_always_on(1, "fixed-3.3V", fixed3v3_power_consumers,
-				     ARRAY_SIZE(fixed3v3_power_consumers), 3300000);
-	regulator_register_fixed(2, dummy_supplies, ARRAY_SIZE(dummy_supplies));
+	if (!of_have_populated_dt()) {
+		regulator_register_always_on(0, "fixed-1.8V", fixed1v8_power_consumers,
+					     ARRAY_SIZE(fixed1v8_power_consumers), 1800000);
+		regulator_register_always_on(1, "fixed-3.3V", fixed3v3_power_consumers,
+					     ARRAY_SIZE(fixed3v3_power_consumers), 3300000);
+		regulator_register_fixed(2, dummy_supplies, ARRAY_SIZE(dummy_supplies));
+	}
 
 	/* External clock source */
 	clk_set_rate(&sh7372_dv_clki_clk, 27000000);

@@ -1633,22 +1658,35 @@ static void __init mackerel_init(void)
 	udelay(50);
 	__raw_writel(srcr4 & ~(1 << 13), SRCR4);
 
-	i2c_register_board_info(0, i2c0_devices,
-				ARRAY_SIZE(i2c0_devices));
-	i2c_register_board_info(1, i2c1_devices,
-				ARRAY_SIZE(i2c1_devices));
+	if (!of_have_populated_dt()) {
+		i2c_register_board_info(0, i2c0_devices,
+					ARRAY_SIZE(i2c0_devices));
+		i2c_register_board_info(1, i2c1_devices,
+					ARRAY_SIZE(i2c1_devices));
+	} else {
+		bus_register_notifier(&i2c_bus_type,
+				      &mackerel_i2c_notifier);
+	}
 
 	sh7372_add_standard_devices();
 
 	platform_add_devices(mackerel_devices, ARRAY_SIZE(mackerel_devices));
+	if (!of_have_populated_dt())
+		platform_add_devices(mackerel_devices_dt,
+				     ARRAY_SIZE(mackerel_devices_dt));
 
 	rmobile_add_devices_to_domains(domain_devices,
 				       ARRAY_SIZE(domain_devices));
+	if (!of_have_populated_dt())
+		rmobile_add_devices_to_domains(domain_devices_dt,
+					       ARRAY_SIZE(domain_devices_dt));
 
 	hdmi_init_pm_clock();
 	sh7372_pm_init();
 	pm_clk_add(&fsi_device.dev, "spu2");
 	pm_clk_add(&hdmi_lcdc_device.dev, "hdmi");
+
+	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
 }
 
 static const char *mackerel_boards_compat_dt[] __initdata = {

@@ -1659,10 +1697,20 @@ static const char *mackerel_boards_compat_dt[] __initdata = {
 DT_MACHINE_START(MACKEREL_DT, "mackerel")
 	.map_io		= sh7372_map_io,
 	.init_early	= sh7372_add_early_devices,
-	.init_irq	= sh7372_init_irq,
+	.init_irq	= sh7372_init_irq_of,
+	.handle_irq	= shmobile_handle_irq_intc,
+	.init_machine	= mackerel_init,
+	.init_late	= sh7372_pm_init_late,
+	.timer		= &shmobile_timer,
+	.dt_compat	= mackerel_boards_compat_dt,

arch/arm/mach-shmobile/board-mackerel.c static const char *mackerel_boards_compat_dt[] __initdata = { · line 1711

Quoted 6 lines

arch/arm/mach-shmobile/board-mackerel.c +6 -0

+MACHINE_END
+
+MACHINE_START(MACKEREL, "mackerel")
+	.map_io		= sh7372_map_io,
+	.init_early	= sh7372_add_early_devices,
+	.init_irq	= sh7372_init_irq_of,

Could sh7372_init_irq be used here ?

Quoted 8 lines

arch/arm/mach-shmobile/board-mackerel.c +0 -1

 	.handle_irq	= shmobile_handle_irq_intc,
 	.init_machine	= mackerel_init,
 	.init_late	= sh7372_pm_init_late,
 	.timer		= &shmobile_timer,
-	.dt_compat  = mackerel_boards_compat_dt,
 MACHINE_END

Signature · 1 line

1.7.2.5

Hi Simon

Thanks for your comments. I'll reply to other your reviews later, I think, let me just briefly clarify this one.

Quoted 15 lines · Simon Horman

Quoted 5 lines · Guennadi Liakhovetski

This patch adds dynamic switching to booting either with or without DT. So far only a part of the board initialisation can be done via DT. Devices, that still need platform data are kept that way. Devices, that can be initialised from DT will not be supplied from the platform data, if a DT image is detected.

Hi Guennadi,

I'm not sure that I'm entirely comfortable with the implications for users here.

In the beginning there was no DT for Mackerel, all boots were non-DT, and in general the board and its devices have been well supported.

Quoted 8 lines

At the present time Mackerel only boots using DT, though almost all the initialisation is done in C. Thus currently a DT boot fairly full support for the board and its devices.

If I understand things correctly, with this change, a DT boot becomes a limited boot that basically only supports devices that can be initialised using DT. And users are expected to go back to a non-DT boot if they want fuller support. This seems undesirable.

No, it's in a way the contrary. As you correctly point out after a recent change mackerel _must_ only be booted with DT, and, I think, this way an undesirable change. After this series of patches both becomes possible:
booting with and without DT. And in both cases all devices are supported. If booting without DT, all devices are supported in the "legacy" way from the board file. If booting with DT _some_ devices, for which sufficient DT support already exist are initialised from the .dts file, others are still initialised from the .c. This way all devices are still supported. Note, however, that some devices don't have a complete DT support yet, e.g., you cannot specify card-detect GPIOs in .dts because of the lacking pincontrol / GPIO DT support. As soon as that is added, .dts will have to be extended respectively. Similarly, as more devices get DT support they can be added to the .dts and excluded from the DT boot by putting them under

(quoted diff) +1 -0

+	if (!of_have_populated_dt()) {

clauses. If we ever come to a point, that no-DT booting will not have to be supported any more, these clauses and respective devices can be easily removed from board-mackerel.c. (Continued below)

Quoted 70 lines

The approach that I took with the kzm9g was to provide an alternate dts and board file, and compatibility string. Clearly that is not an entirely elegant solution. But it does avoid the problem that I describe above.

Quoted 66 lines

Signed-off-by Guennadi Liakhovetski <[email protected]>

---
 arch/arm/mach-shmobile/board-mackerel.c |   84 ++++++++++++++++++++++++-------
 1 files changed, 66 insertions(+), 18 deletions(-)

arch/arm/mach-shmobile/board-mackerel.c +26 -6

index 39b8f2e..a6358c9 100644
@@ -1326,7 +1326,6 @@ static struct platform_device mackerel_camera = {
 
 static struct platform_device *mackerel_devices[] __initdata = {
 	&nor_flash_device,
-	&smc911x_device,
 	&lcdc_device,
 	&usbhs0_device,
 	&usbhs1_device,

@@ -1335,17 +1334,21 @@ static struct platform_device *mackerel_devices[] __initdata = {
 	&fsi_ak4643_device,
 	&fsi_hdmi_device,
 	&nand_flash_device,
+	&ceu_device,
+	&mackerel_camera,
+	&hdmi_device,
+	&hdmi_lcdc_device,
+	&meram_device,
+};
+
+static struct platform_device *mackerel_devices_dt[] __initdata = {
+	&smc911x_device,
 	&sdhi0_device,
 #if !defined(CONFIG_MMC_SH_MMCIF) && !defined(CONFIG_MMC_SH_MMCIF_MODULE)
 	&sdhi1_device,
 #endif
 	&sdhi2_device,
 	&sh_mmcif_device,
-	&ceu_device,
-	&mackerel_camera,
-	&hdmi_device,
-	&hdmi_lcdc_device,
-	&meram_device,
 };
 
 /* Keypad Initialization */

@@ -1404,6 +1407,24 @@ static struct i2c_board_info i2c1_devices[] = {
 	},
 };
 
+static int mackerel_i2c_bus_notify(struct notifier_block *nb,
+				   unsigned long action, void *data)
+{
+	struct device *dev = data;
+
+	if (action != BUS_NOTIFY_ADD_DEVICE ||
+	    strcmp(dev_name(dev->parent), "fff20000.i2c"))
+		return NOTIFY_DONE;
+
+	i2c_new_device(to_i2c_adapter(dev), &i2c0_devices[1]);
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block mackerel_i2c_notifier = {
+	.notifier_call = mackerel_i2c_bus_notify,
+};

Quoted 8 lines

arch/arm/mach-shmobile/board-mackerel.c static void __init mackerel_init(void)

Quoted 5 lines

arch/arm/mach-shmobile/board-mackerel.c +1 -0

+
 #define GPIO_PORT9CR	IOMEM(0xE6051009)
 #define GPIO_PORT10CR	IOMEM(0xE605100A)
 #define GPIO_PORT167CR	IOMEM(0xE60520A7)

@@ -1420,22 +1441,26 @@ static void __init mackerel_init(void)

I wonder if it would be cleaner to achieve this by providing mackerel_init_dt().

I thought about this, but initialisation is interleaved and in some cases order is important, so, you would need something like "early_dt()," "mid_early_common()," "late_dt()" etc., which doesn't seem an improvement to me:-)

Quoted 83 lines

Quoted 83 lines

arch/arm/mach-shmobile/board-mackerel.c +33 -11

 		{ "A3SP", &usbhs0_device, },
 		{ "A3SP", &usbhs1_device, },
 		{ "A3SP", &nand_flash_device, },
+		{ "A4R", &ceu_device, },
+	};
+	struct pm_domain_device domain_devices_dt[] = {
 		{ "A3SP", &sh_mmcif_device, },
 		{ "A3SP", &sdhi0_device, },
 #if !defined(CONFIG_MMC_SH_MMCIF) && !defined(CONFIG_MMC_SH_MMCIF_MODULE)
 		{ "A3SP", &sdhi1_device, },
 #endif
 		{ "A3SP", &sdhi2_device, },
-		{ "A4R", &ceu_device, },
 	};
 	u32 srcr4;
 	struct clk *clk;
 
-	regulator_register_always_on(0, "fixed-1.8V", fixed1v8_power_consumers,
-				     ARRAY_SIZE(fixed1v8_power_consumers), 1800000);
-	regulator_register_always_on(1, "fixed-3.3V", fixed3v3_power_consumers,
-				     ARRAY_SIZE(fixed3v3_power_consumers), 3300000);
-	regulator_register_fixed(2, dummy_supplies, ARRAY_SIZE(dummy_supplies));
+	if (!of_have_populated_dt()) {
+		regulator_register_always_on(0, "fixed-1.8V", fixed1v8_power_consumers,
+					     ARRAY_SIZE(fixed1v8_power_consumers), 1800000);
+		regulator_register_always_on(1, "fixed-3.3V", fixed3v3_power_consumers,
+					     ARRAY_SIZE(fixed3v3_power_consumers), 3300000);
+		regulator_register_fixed(2, dummy_supplies, ARRAY_SIZE(dummy_supplies));
+	}
 
 	/* External clock source */
 	clk_set_rate(&sh7372_dv_clki_clk, 27000000);

@@ -1633,22 +1658,35 @@ static void __init mackerel_init(void)
 	udelay(50);
 	__raw_writel(srcr4 & ~(1 << 13), SRCR4);
 
-	i2c_register_board_info(0, i2c0_devices,
-				ARRAY_SIZE(i2c0_devices));
-	i2c_register_board_info(1, i2c1_devices,
-				ARRAY_SIZE(i2c1_devices));
+	if (!of_have_populated_dt()) {
+		i2c_register_board_info(0, i2c0_devices,
+					ARRAY_SIZE(i2c0_devices));
+		i2c_register_board_info(1, i2c1_devices,
+					ARRAY_SIZE(i2c1_devices));
+	} else {
+		bus_register_notifier(&i2c_bus_type,
+				      &mackerel_i2c_notifier);
+	}
 
 	sh7372_add_standard_devices();
 
 	platform_add_devices(mackerel_devices, ARRAY_SIZE(mackerel_devices));
+	if (!of_have_populated_dt())
+		platform_add_devices(mackerel_devices_dt,
+				     ARRAY_SIZE(mackerel_devices_dt));
 
 	rmobile_add_devices_to_domains(domain_devices,
 				       ARRAY_SIZE(domain_devices));
+	if (!of_have_populated_dt())
+		rmobile_add_devices_to_domains(domain_devices_dt,
+					       ARRAY_SIZE(domain_devices_dt));
 
 	hdmi_init_pm_clock();
 	sh7372_pm_init();
 	pm_clk_add(&fsi_device.dev, "spu2");
 	pm_clk_add(&hdmi_lcdc_device.dev, "hdmi");
+
+	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
 }
 
 static const char *mackerel_boards_compat_dt[] __initdata = {

@@ -1659,10 +1697,20 @@ static const char *mackerel_boards_compat_dt[] __initdata = {
 DT_MACHINE_START(MACKEREL_DT, "mackerel")
 	.map_io		= sh7372_map_io,
 	.init_early	= sh7372_add_early_devices,
-	.init_irq	= sh7372_init_irq,
+	.init_irq	= sh7372_init_irq_of,
+	.handle_irq	= shmobile_handle_irq_intc,
+	.init_machine	= mackerel_init,
+	.init_late	= sh7372_pm_init_late,
+	.timer		= &shmobile_timer,
+	.dt_compat	= mackerel_boards_compat_dt,

Quoted 8 lines

arch/arm/mach-shmobile/board-mackerel.c static const char *mackerel_boards_compat_dt[] __initdata = { · line 1711

Quoted 6 lines

arch/arm/mach-shmobile/board-mackerel.c +6 -0

+MACHINE_END
+
+MACHINE_START(MACKEREL, "mackerel")
+	.map_io		= sh7372_map_io,
+	.init_early	= sh7372_add_early_devices,
+	.init_irq	= sh7372_init_irq_of,

Could sh7372_init_irq be used here ?

Yes, it could. I'll reply to your other review separately, briefly, by using the conditional, that I proposed in my patch 3/7 we could make the non-dt version static and let everyone just use sh7372_init_irq_of. But this is just an idea, we can keep sh7372_init_irq() too if this is prefered.

Thanks
Guennadi

Quoted 8 lines

Quoted 8 lines

arch/arm/mach-shmobile/board-mackerel.c +0 -1

 	.handle_irq	= shmobile_handle_irq_intc,
 	.init_machine	= mackerel_init,
 	.init_late	= sh7372_pm_init_late,
 	.timer		= &shmobile_timer,
-	.dt_compat  = mackerel_boards_compat_dt,
 MACHINE_END

Signature · 1 line

1.7.2.5

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

This patch adds dynamic switching to booting either with or without DT. So far only a part of the board initialisation can be done via DT. Devices, that still need platform data are kept that way. Devices, that can be initialised from DT will not be supplied from the platform data, if a DT image is detected.

Signed-off-by Guennadi Liakhovetski <[email protected]>

---

arch/arm/mach-shmobile/board-mackerel.c +7 -0

+static int mackerel_i2c_bus_notify(struct notifier_block *nb,
+				   unsigned long action, void *data)
+{
+	struct device *dev = data;
+
+	if (action != BUS_NOTIFY_ADD_DEVICE ||
+	    strcmp(dev_name(dev->parent), "fff20000.i2c"))

Quoted 24 lines · Guennadi Liakhovetski

This patch adds dynamic switching to booting either with or without DT. So far only a part of the board initialisation can be done via DT. Devices, that still need platform data are kept that way. Devices, that can be initialised from DT will not be supplied from the platform data, if a DT image is detected.

Signed-off-by Guennadi Liakhovetski <[email protected]>

---

arch/arm/mach-shmobile/board-mackerel.c +15 -0

+static int mackerel_i2c_bus_notify(struct notifier_block *nb,
+				   unsigned long action, void *data)
+{
+	struct device *dev = data;
+
+	if (action != BUS_NOTIFY_ADD_DEVICE ||
+	    strcmp(dev_name(dev->parent), "fff20000.i2c"))
+		return NOTIFY_DONE;
+
+	i2c_new_device(to_i2c_adapter(dev), &i2c0_devices[1]);
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block mackerel_i2c_notifier = {

arch/arm/mach-shmobile/board-mackerel.c static struct i2c_board_info i2c1_devices[] = { · line 1426

Quoted 2 lines

arch/arm/mach-shmobile/board-mackerel.c +2 -0

+	.notifier_call = mackerel_i2c_bus_notify,
+};

This looks dodgy. It appears that the purpose of this hook is to create the tca6408-keys device because it has some platform data. However, this hook will try to create the device every time BUS_NOTIFY_ADD_DEVICE happens on the fff20000.i2c bus *including* when the tca6408-keys device gets added.

The correct approach when you need to add specific platform data is to still put the device into the device tree and make the notifier look for that specific device. Then the platform data pointer can be attached at BUS_NOTIFY_ADD_DEVICE time.

However, it doesn't look like you need a notifier at all. From what I can see the tca6408-keys device does have platform data, but it is all simple (no callback pointers). You should instead encode the platform data into device tree properties and extract them from the driver.

Hi Grant

Quoted 25 lines · Grant Likely

Quoted 24 lines · Guennadi Liakhovetski

This patch adds dynamic switching to booting either with or without DT. So far only a part of the board initialisation can be done via DT. Devices, that still need platform data are kept that way. Devices, that can be initialised from DT will not be supplied from the platform data, if a DT image is detected.

Signed-off-by Guennadi Liakhovetski <[email protected]>

---

arch/arm/mach-shmobile/board-mackerel.c +15 -0

+static int mackerel_i2c_bus_notify(struct notifier_block *nb,
+				   unsigned long action, void *data)
+{
+	struct device *dev = data;
+
+	if (action != BUS_NOTIFY_ADD_DEVICE ||
+	    strcmp(dev_name(dev->parent), "fff20000.i2c"))
+		return NOTIFY_DONE;
+
+	i2c_new_device(to_i2c_adapter(dev), &i2c0_devices[1]);
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block mackerel_i2c_notifier = {

Quoted 8 lines

arch/arm/mach-shmobile/board-mackerel.c static struct i2c_board_info i2c1_devices[] = { · line 1426

Quoted 2 lines

arch/arm/mach-shmobile/board-mackerel.c +2 -0

+	.notifier_call = mackerel_i2c_bus_notify,
+};

This looks dodgy. It appears that the purpose of this hook is to create the tca6408-keys device because it has some platform data. However, this hook will try to create the device every time BUS_NOTIFY_ADD_DEVICE happens on the fff20000.i2c bus *including* when the tca6408-keys device gets added.

I think, this is only called once, when the i2c adapter device is added in i2c_register_adapter(). I2C client devices have these adapter devices as their parents, and those devices have "i2c-%d" as their names. Since all client devices get destroyed when the adapter is unbound, the above should be safe.

Quoted 10 lines

The correct approach when you need to add specific platform data is to still put the device into the device tree and make the notifier look for that specific device. Then the platform data pointer can be attached at BUS_NOTIFY_ADD_DEVICE time.

However, it doesn't look like you need a notifier at all. From what I can see the tca6408-keys device does have platform data, but it is all simple (no callback pointers). You should instead encode the platform data into device tree properties and extract them from the driver.

Yes, this is the ultimate goal. But the purpose of this patch series is to move whatever devices are possible right _now_ to DT. Ultimately all of them should be migrated. So, yes, we could try to begin with tca6408, because the above hack is certainly the ugliest one in this series, but in principle this is just one of several devices, that we have to keep in platform data for now and aim at removing these temporary hacks as soon as respective drivers implement sufficient DT support.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Quoted 38 lines · Guennadi Liakhovetski

Hi Grant

Quoted 25 lines · Grant Likely

Quoted 24 lines · Guennadi Liakhovetski

This patch adds dynamic switching to booting either with or without DT. So far only a part of the board initialisation can be done via DT. Devices, that still need platform data are kept that way. Devices, that can be initialised from DT will not be supplied from the platform data, if a DT image is detected.

Signed-off-by Guennadi Liakhovetski <[email protected]>

---

arch/arm/mach-shmobile/board-mackerel.c +15 -0

+static int mackerel_i2c_bus_notify(struct notifier_block *nb,
+				   unsigned long action, void *data)
+{
+	struct device *dev = data;
+
+	if (action != BUS_NOTIFY_ADD_DEVICE ||
+	    strcmp(dev_name(dev->parent), "fff20000.i2c"))
+		return NOTIFY_DONE;
+
+	i2c_new_device(to_i2c_adapter(dev), &i2c0_devices[1]);
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block mackerel_i2c_notifier = {

Quoted 8 lines

arch/arm/mach-shmobile/board-mackerel.c static struct i2c_board_info i2c1_devices[] = { · line 1426

Quoted 2 lines

arch/arm/mach-shmobile/board-mackerel.c +2 -0

+	.notifier_call = mackerel_i2c_bus_notify,
+};

This looks dodgy. It appears that the purpose of this hook is to create the tca6408-keys device because it has some platform data. However, this hook will try to create the device every time BUS_NOTIFY_ADD_DEVICE happens on the fff20000.i2c bus *including* when the tca6408-keys device gets added.

Quoted 5 lines

I think, this is only called once, when the i2c adapter device is added in i2c_register_adapter(). I2C client devices have these adapter devices as their parents, and those devices have "i2c-%d" as their names. Since all client devices get destroyed when the adapter is unbound, the above should be safe.

Take another look. The way the bus notifiers work is they get called once for every device registered with the given bus type. That means i2c_client objects, not i3c

Quoted 18 lines

Quoted 9 lines

The correct approach when you need to add specific platform data is to still put the device into the device tree and make the notifier look for that specific device. Then the platform data pointer can be attached at BUS_NOTIFY_ADD_DEVICE time.

However, it doesn't look like you need a notifier at all. From what I can see the tca6408-keys device does have platform data, but it is all simple (no callback pointers). You should instead encode the platform data into device tree properties and extract them from the driver.

Yes, this is the ultimate goal. But the purpose of this patch series is to move whatever devices are possible right _now_ to DT. Ultimately all of them should be migrated. So, yes, we could try to begin with tca6408, because the above hack is certainly the ugliest one in this series, but in principle this is just one of several devices, that we have to keep in platform data for now and aim at removing these temporary hacks as soon as respective drivers implement sufficient DT support.

Quoted 6 lines

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Signature · 2 lines

Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

Hi Grant

Quoted 25 lines · Grant Likely

Quoted 24 lines · Guennadi Liakhovetski

This patch adds dynamic switching to booting either with or without DT. So far only a part of the board initialisation can be done via DT. Devices, that still need platform data are kept that way. Devices, that can be initialised from DT will not be supplied from the platform data, if a DT image is detected.

Signed-off-by Guennadi Liakhovetski <[email protected]>

---

arch/arm/mach-shmobile/board-mackerel.c +15 -0

+static int mackerel_i2c_bus_notify(struct notifier_block *nb,
+				   unsigned long action, void *data)
+{
+	struct device *dev = data;
+
+	if (action != BUS_NOTIFY_ADD_DEVICE ||
+	    strcmp(dev_name(dev->parent), "fff20000.i2c"))
+		return NOTIFY_DONE;
+
+	i2c_new_device(to_i2c_adapter(dev), &i2c0_devices[1]);
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block mackerel_i2c_notifier = {

Quoted 8 lines

arch/arm/mach-shmobile/board-mackerel.c static struct i2c_board_info i2c1_devices[] = { · line 1426

Quoted 2 lines

arch/arm/mach-shmobile/board-mackerel.c +2 -0

+	.notifier_call = mackerel_i2c_bus_notify,
+};

This looks dodgy. It appears that the purpose of this hook is to create the tca6408-keys device because it has some platform data. However, this hook will try to create the device every time BUS_NOTIFY_ADD_DEVICE happens on the fff20000.i2c bus *including* when the tca6408-keys device gets added.

I think, this is only called once, when the i2c adapter device is added in i2c_register_adapter(). I2C client devices have these adapter devices as their parents, and those devices have "i2c-%d" as their names. Since all client devices get destroyed when the adapter is unbound, the above should be safe.

Quoted 10 lines

The correct approach when you need to add specific platform data is to still put the device into the device tree and make the notifier look for that specific device. Then the platform data pointer can be attached at BUS_NOTIFY_ADD_DEVICE time.

However, it doesn't look like you need a notifier at all. From what I can see the tca6408-keys device does have platform data, but it is all simple (no callback pointers). You should instead encode the platform data into device tree properties and extract them from the driver.

Yes, this is the ultimate goal. But the purpose of this patch series is to move whatever devices are possible right _now_ to DT. Ultimately all of them should be migrated. So, yes, we could try to begin with tca6408, because the above hack is certainly the ugliest one in this series, but in principle this is just one of several devices, that we have to keep in platform data for now and aim at removing these temporary hacks as soon as respective drivers implement sufficient DT support.

This patch adds dynamic switching to booting either with or without DT. So far only a part of the board initialisation can be done via DT. Devices, that still need platform data are kept that way. Devices, that can be initialised from DT will not be supplied from the platform data, if a DT image is detected.

Signed-off-by Guennadi Liakhovetski <[email protected]>

---

static const char *mackerel_boards_compat_dt[] __initdata = {

arch/arm/mach-shmobile/board-mackerel.c +8 -0

@@ -1659,10 +1715,20 @@ static const char *mackerel_boards_compat_dt[] __initdata = {
 DT_MACHINE_START(MACKEREL_DT, "mackerel")
 	.map_io		= sh7372_map_io,
 	.init_early	= sh7372_add_early_devices,
+	.init_irq	= sh7372_init_irq_of,
+	.handle_irq	= shmobile_handle_irq_intc,
+	.init_machine	= mackerel_init,
+	.init_late	= sh7372_pm_init_late,
+	.timer		= &shmobile_timer,
+	.dt_compat	= mackerel_boards_compat_dt,
+MACHINE_END
+