Next round of DT enablement for ux500/Snowball

During Device Tree enablement it is necessary to remove snowball_<device>* platform_data segments one at at time, as and when particular devices are DT enabled. This patch provides a temporary solution. Once this new struct is empty it will be removed again.

Signed-off-by Lee Jones <[email protected]>

---
?arch/arm/mach-ux500/board-mop500.c | ? 18 ++++++++++++++++-- ?1 files changed, 16 insertions(+), 2 deletions(-)

arch/arm/mach-ux500/board-mop500.c +0 -0

index 77d03c1..29e0ade 100644

Quoted 7 lines · Lee Jones

During Device Tree enablement it is necessary to remove snowball_<device>* platform_data segments one at at time, as and when particular devices are DT enabled. This patch provides a temporary solution. Once this new struct is empty it will be removed again.

Signed-off-by Lee Jones<[email protected]>

This is not exactly elegant and I cannot quite see how the pieces fit together.

Quoted 12 lines

---
  arch/arm/mach-ux500/board-mop500.c |   18 ++++++++++++++++--
  1 files changed, 16 insertions(+), 2 deletions(-)

arch/arm/mach-ux500/board-mop500.c +0 -0

index 77d03c1..29e0ade 100644
@@ -609,6 +609,13 @@ static struct platform_device *snowball_platform_devs[] __initdata = {
        &ab8500_device,
  };
 

arch/arm/mach-ux500/board-mop500.c static struct platform_device *snowball_platform_devs[] __initdata = { · line 617

Quoted 6 lines

arch/arm/mach-ux500/board-mop500.c +6 -0

+static struct platform_device *snowball_of_platform_devs[] __initdata = {
+&snowball_led_dev,
+&snowball_key_dev,
+&snowball_sbnet_dev,
+&ab8500_device,
+};

So above this array is an identical array which is going to become unused, then why
not just rename the other array with _of_ or just skip the whole business altogether?

arch/arm/mach-ux500/board-mop500.c static void __init u8500_init_machine(void) · line 790

Quoted 10 lines

arch/arm/mach-ux500/board-mop500.c +1 -2

+
  static void __init mop500_init_machine(void)
  {
        struct device *parent = NULL;

@@ -786,8 +793,15 @@ static void __init u8500_init_machine(void)
                mop500_sdi_init(parent);
        } else if (of_machine_is_compatible("calaosystems,snowball-a9500")) {
                snowball_pins_init();
-               platform_add_devices(snowball_platform_devs,
-                               ARRAY_SIZE(snowball_platform_devs));

So first you remove ths use of that array...

arch/arm/mach-ux500/board-mop500.c static void __init u8500_init_machine(void) · line 802

Quoted 7 lines

arch/arm/mach-ux500/board-mop500.c +7 -0

+
+               /* Devices to be DT:ed:
+                    snowball_led_dev   = todo
+                    snowball_key_dev   = todo
+                    snowball_sbnet_dev = todo
+                    ab8500_device      = todo
+               */

/*
* Comment style
*/

arch/arm/mach-ux500/board-mop500.c static void __init u8500_init_machine(void) · line 804

Quoted 2 lines

arch/arm/mach-ux500/board-mop500.c +2 -0

+               platform_add_devices(snowball_of_platform_devs,
+                               ARRAY_SIZE(snowball_of_platform_devs));

And insert an identical array?

The only change is that you now have an *unused* array with all it's parents set and then an array with all parents set to NULL which you use. I don't get it ...

Nothing is unused. When DT is disabled snowball_platform_devs will be used to add the platform devices. This is, and will remain fully populated for non-DT boots. The DT'ed version will decrease in size until the final element is removed along with the definition.

I have ran this past Arnd, who thought this was a good idea. It's only a temporary solution which well aide us in enabling devices spanned over multiple submissions, rather than attempting to enable everything in a single patch-set - which isn't going to happen.

Quoted 4 lines · Lee Jones

Nothing is unused. When DT is disabled snowball_platform_devs will be used to add the platform devices. This is, and will remain fully populated for non-DT boots. The DT'ed version will decrease in size until the final element is removed along with the definition.

OK then..

Quoted 4 lines

I have ran this past Arnd, who thought this was a good idea. It's only a temporary solution which well aide us in enabling devices spanned over multiple submissions, rather than attempting to enable everything in a single patch-set - which isn't going to happen.

Sure so let's say that I add this patch immediately before the first patch removing atleast *one* of the entries, because until then it is certainly not needed.

+// TODO: Once all pieces are DT:ed, remove completely

+// TODO: Once all pieces are DT:ed, remove completely

+ ? ? ? /* Requires clock name bindings. */
+ ? ? ? OF_DEV_AUXDATA("stmicroelectronics,nomadik-gpio", 0x8012e000, "gpio.0", NULL),
+ ? ? ? OF_DEV_AUXDATA("stmicroelectronics,nomadik-gpio", 0x8012e080, "gpio.1", NULL),
+ ? ? ? OF_DEV_AUXDATA("stmicroelectronics,nomadik-gpio", 0x8000e000, "gpio.2", NULL),
+ ? ? ? OF_DEV_AUXDATA("stmicroelectronics,nomadik-gpio", 0x8000e080, "gpio.3", NULL),
+ ? ? ? OF_DEV_AUXDATA("stmicroelectronics,nomadik-gpio", 0x8000e100, "gpio.4", NULL),
+ ? ? ? OF_DEV_AUXDATA("stmicroelectronics,nomadik-gpio", 0x8000e180, "gpio.5", NULL),
+ ? ? ? OF_DEV_AUXDATA("stmicroelectronics,nomadik-gpio", 0x8011e000, "gpio.6", NULL),
+ ? ? ? OF_DEV_AUXDATA("stmicroelectronics,nomadik-gpio", 0x8011e080, "gpio.7", NULL),
+ ? ? ? OF_DEV_AUXDATA("stmicroelectronics,nomadik-gpio", 0xa03fe000, "gpio.8", NULL),

Quoted 10 lines · Lee Jones

+ ? ? ? /* Requires clock name bindings. */
+ ? ? ? OF_DEV_AUXDATA("stmicroelectronics,nomadik-gpio", 0x8012e000, "gpio.0", NULL),
+ ? ? ? OF_DEV_AUXDATA("stmicroelectronics,nomadik-gpio", 0x8012e080, "gpio.1", NULL),
+ ? ? ? OF_DEV_AUXDATA("stmicroelectronics,nomadik-gpio", 0x8000e000, "gpio.2", NULL),
+ ? ? ? OF_DEV_AUXDATA("stmicroelectronics,nomadik-gpio", 0x8000e080, "gpio.3", NULL),
+ ? ? ? OF_DEV_AUXDATA("stmicroelectronics,nomadik-gpio", 0x8000e100, "gpio.4", NULL),
+ ? ? ? OF_DEV_AUXDATA("stmicroelectronics,nomadik-gpio", 0x8000e180, "gpio.5", NULL),
+ ? ? ? OF_DEV_AUXDATA("stmicroelectronics,nomadik-gpio", 0x8011e000, "gpio.6", NULL),
+ ? ? ? OF_DEV_AUXDATA("stmicroelectronics,nomadik-gpio", 0x8011e080, "gpio.7", NULL),
+ ? ? ? OF_DEV_AUXDATA("stmicroelectronics,nomadik-gpio", 0xa03fe000, "gpio.8", NULL),

Needs updating to use the st,foo prefix I guess.

Please use the defined block offset addresses from <mach/db8500-regs.h> U8500_GPIO0_BASE etc. instead of hardcoded values.

Add required documentation for specific gpio-nomadik DT bindings.

Signed-off-by Lee Jones <[email protected]>

---
 .../devicetree/bindings/gpio/gpio-nmk.txt          |   29 ++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-nmk.txt

Documentation/devicetree/bindings/gpio/gpio-nmk.txt new file +0 -0

new file mode 100644
index 0000000..1555029

Documentation/devicetree/bindings/gpio/gpio-nmk.txt new file +12 -0

+
+Example:
+
+                gpio1: gpio at 8012e080 {
+                        compatible = "stmicroelectronics,nomadik-gpio";
+                        reg =  <0x8012e080 0x80>;
+                        interrupts = <0 120 0x4>;
+                        #gpio-cells = <2>;
+                        gpio-controller;
+                        supports-sleepmode;
+                        gpio-bank = <1>;
+                };

Signature · 1 line

1.7.9.1

Quoted 8 lines · Lee Jones

Add required documentation for specific gpio-nomadik DT bindings.

Signed-off-by Lee Jones<[email protected]>

---
  .../devicetree/bindings/gpio/gpio-nmk.txt          |   29 ++++++++++++++++++++
  1 files changed, 29 insertions(+), 0 deletions(-)

create mode 100644 Documentation/devicetree/bindings/gpio/gpio-nmk.txt

Documentation/devicetree/bindings/gpio/gpio-nmk.txt line 4

Quoted 10 lines

Documentation/devicetree/bindings/gpio/gpio-nmk.txt new file +4 -0

new file mode 100644
index 0000000..1555029
@@ -0,0 +1,29 @@
+Nomadik GPIO controller
+
+Required properties:
+- compatible           : Should be "stmicroelectronics,nomadik-gpio".

"stmicroelectronics," is a really long prefix. You can use simply "st," here since it has already been defined and documented in Documentation/devicetree/bindings/vendor-prefixes.txt

Documentation/devicetree/bindings/gpio/gpio-nmk.txt line 14

Quoted 10 lines

Documentation/devicetree/bindings/gpio/gpio-nmk.txt new file +10 -0

+- reg                  : Physical base address and length of the controller's registers.
+- interrupts           : The interrupt outputs from the controller.
+- #gpio-cells          : Should be two:
+                           The first cell is the pin number.
+                           The second cell is used to specify optional parameters:
+                             - bits[3:0] trigger type and level flags:
+                                 1 = low-to-high edge triggered.
+                                 2 = high-to-low edge triggered.
+                                 4 = active high level-sensitive.
+                                 8 = active low level-sensitive.

Those look like interrupt flags, not gpio flags. If the gpio lines can be used as generic irq input lines, then this node should also declare itself as an interrupt controller.

Documentation/devicetree/bindings/gpio/gpio-nmk.txt line 16

Quoted 2 lines

Documentation/devicetree/bindings/gpio/gpio-nmk.txt new file +2 -0

+- gpio-controller      : Marks the device node as a GPIO controller.
+- supports-sleepmode   : Specifies whether controller can sleep or not

Typically, custom properites that are for a specific device should be prefixed with the manufacturer name. So, something like:
"st,has-sleepmode".

Documentation/devicetree/bindings/gpio/gpio-nmk.txt line 17

Quoted 1 line

Documentation/devicetree/bindings/gpio/gpio-nmk.txt new file +1 -0

+- gpio-bank            : Specifies which bank a controller owns.

What is this for (how is it used)? It shouldn't be needed to specify a bank number.

Quoted 15 lines

Documentation/devicetree/bindings/gpio/gpio-nmk.txt new file +12 -1

+
+Example:
+
+                gpio1: gpio at 8012e080 {
+                        compatible = "stmicroelectronics,nomadik-gpio";
+                        reg =<0x8012e080 0x80>;
+                        interrupts =<0 120 0x4>;
+                        #gpio-cells =<2>;
+                        gpio-controller;
+                        supports-sleepmode;
+                        gpio-bank =<1>;
+                };
--

1.7.9.1

Quoted 9 lines · Grant Likely

Quoted 8 lines · Lee Jones

Add required documentation for specific gpio-nomadik DT bindings.

Signed-off-by Lee Jones<[email protected]>

---
  .../devicetree/bindings/gpio/gpio-nmk.txt          |   29 ++++++++++++++++++++
  1 files changed, 29 insertions(+), 0 deletions(-)

create mode 100644 Documentation/devicetree/bindings/gpio/gpio-nmk.txt

Quoted 14 lines

Documentation/devicetree/bindings/gpio/gpio-nmk.txt line 4

Quoted 10 lines

Documentation/devicetree/bindings/gpio/gpio-nmk.txt new file +4 -0

new file mode 100644
index 0000000..1555029
@@ -0,0 +1,29 @@
+Nomadik GPIO controller
+
+Required properties:
+- compatible           : Should be "stmicroelectronics,nomadik-gpio".

"stmicroelectronics," is a really long prefix. You can use simply "st," here since it has already been defined and documented in Documentation/devicetree/bindings/vendor-prefixes.txt

Absolutely.

Quoted 11 lines

Documentation/devicetree/bindings/gpio/gpio-nmk.txt line 14

Quoted 10 lines

Documentation/devicetree/bindings/gpio/gpio-nmk.txt new file +10 -0

+- reg                  : Physical base address and length of the controller's registers.
+- interrupts           : The interrupt outputs from the controller.
+- #gpio-cells          : Should be two:
+                           The first cell is the pin number.
+                           The second cell is used to specify optional parameters:
+                             - bits[3:0] trigger type and level flags:
+                                 1 = low-to-high edge triggered.
+                                 2 = high-to-low edge triggered.
+                                 4 = active high level-sensitive.
+                                 8 = active low level-sensitive.

Quoted 3 lines

Those look like interrupt flags, not gpio flags. If the gpio lines can be used as generic irq input lines, then this node should also declare itself as an interrupt controller.

They can and it will do in an up-coming patch.

Quoted 6 lines

Documentation/devicetree/bindings/gpio/gpio-nmk.txt line 16

Quoted 2 lines

Documentation/devicetree/bindings/gpio/gpio-nmk.txt new file +2 -0

+- gpio-controller      : Marks the device node as a GPIO controller.
+- supports-sleepmode   : Specifies whether controller can sleep or not

Typically, custom properites that are for a specific device should be prefixed with the manufacturer name. So, something like:
"st,has-sleepmode".

No problem.

Quoted 4 lines

Documentation/devicetree/bindings/gpio/gpio-nmk.txt line 17

Quoted 1 line

Documentation/devicetree/bindings/gpio/gpio-nmk.txt new file +1 -0

+- gpio-bank            : Specifies which bank a controller owns.

What is this for (how is it used)? It shouldn't be needed to specify a bank number.

Briefly, it is used like this:

of_property_read_u32(np, "gpio-bank", &bank);
chip->base = bank * NMK_GPIO_PER_CHIP;
nmk_chip->bank = bank;

Is that wrong? If so, is there a better way to do it?

Are the banks all part of a single GPIO device?

Quoted 1 line · Grant Likely

Are the banks all part of a single GPIO device?

No. The banks are individual pieces of silicon instance, so they each have separate reset and clock lines for example.

Apart from that I just vaguely grasp what you're discussing here :-)

Quoted 8 lines · Linus Walleij

Quoted 1 line · Grant Likely

Are the banks all part of a single GPIO device?

No. The banks are individual pieces of silicon instance, so they each have separate reset and clock lines for example.

Apart from that I just vaguely grasp what you're discussing here :-)

Don't worry about it. After reading further, it looks okay.

Ensure each button is individually identifiable by using their correct unique given device names.

Signed-off-by Lee Jones <[email protected]>

drivers/mmc/host/mmci.c static const struct mmc_host_ops mmci_ops = { · line 1221

Quoted 3 lines · Lee Jones

drivers/mmc/host/mmci.c +3 -0

+	of_property_read_u32(np, "clock-frequency",&pdata->f_max);
+	if (!pdata->f_max)
+		pr_warn("%s has no 'clock-frequency' property\n", np->full_name);

To be anal about this (which is what we should be about DT bindings) this is not a 'clock frequency' but a maximum clock frequency for the bus. Or to put it another way, a clock frequency limit.

A 'clock-frequency' property suggests (at least to me) that it's specifying the rate of the MCLK signal, which is not what it's doing.

Quoted 9 lines · Russell King - ARM Linux

drivers/mmc/host/mmci.c static const struct mmc_host_ops mmci_ops = { · line 1221

Quoted 3 lines · Lee Jones

drivers/mmc/host/mmci.c +3 -0

+	of_property_read_u32(np, "clock-frequency",&pdata->f_max);
+	if (!pdata->f_max)
+		pr_warn("%s has no 'clock-frequency' property\n", np->full_name);

To be anal about this (which is what we should be about DT bindings) this is not a 'clock frequency' but a maximum clock frequency for the bus. Or to put it another way, a clock frequency limit.

Quoted 2 lines

A 'clock-frequency' property suggests (at least to me) that it's specifying the rate of the MCLK signal, which is not what it's doing.

Sounds sensible.

I initially had it as 'f-max', but was told to change it to 'clock-frequency'. I'm happy to take other suggestions however.

Yes, "clock-frequency" already has established usage as an exact clock specification. If it is a maximum as you say then something like "max-frequency" makes more sense.

Someone was working on a common MMC DT binding. Really all of the MMC drivers should use the same binding for the common bits.

Agreed.

Should we use max-frequency or clock-frequency for sdhci though? It looks like this is both the maximum frequency that it can use and the base frequency that the actual clock rate is derived from (can be a fraction).

This provides full documentation detailing each compulsory, optional and device (variant) specific properties available for use within the MMCI Device Tree.

Signed-off-by Lee Jones <[email protected]>

Quoted 5 lines · Lee Jones

This provides full documentation detailing each compulsory, optional and device (variant) specific properties available for use within the MMCI Device Tree.

Signed-off-by Lee Jones <[email protected]>

Acked-by Arnd Bergmann <[email protected]>

Hi Arnd,

Quoted 5 lines · Arnd Bergmann

Quoted 4 lines · Lee Jones

This provides full documentation detailing each compulsory, optional and device (variant) specific properties available for use within the MMCI Device Tree.

Quoted 3 lines

Quoted 1 line

Signed-off-by Lee Jones<[email protected]>

Acked-by Arnd Bergmann<[email protected]>

Did you really mean to ack this version or the newer one, with "max-frequency" instead of "clock-frequency"?

Quoted 8 lines · Pawel Moll

Hi Arnd,

Quoted 5 lines · Arnd Bergmann

Quoted 4 lines · Lee Jones

This provides full documentation detailing each compulsory, optional and device (variant) specific properties available for use within the MMCI Device Tree.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Signed-off-by Lee Jones<[email protected]>

Acked-by Arnd Bergmann<[email protected]>

Did you really mean to ack this version or the newer one, with "max-frequency" instead of "clock-frequency"?

He meant the newer one. :)