mfd: add basic sun6i A31 PRCM support

Add DT bindings documentation for sunxi's reset controllers.

Signed-off-by Boris BREZILLON <[email protected]>

The current implementation uses sunxi_reset_init function for both early init and platform device probe.

The sunxi_reset_init function uses DT to retrieve device resources, which will be an issue if reset controllers are registered from an MFD device that define resources from mfd_cell definition.

Moreover, we can make of devm functions when we're in the probe context.

Signed-off-by Boris BREZILLON <[email protected]>

---
 drivers/reset/reset-sunxi.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

drivers/reset/reset-sunxi.c +1 -1

index 695bd34..1b5fea6 100644
@@ -145,7 +145,24 @@ MODULE_DEVICE_TABLE(of, sunxi_reset_dt_ids);
 
 static int sunxi_reset_probe(struct platform_device *pdev)
 {
-	return sunxi_reset_init(pdev->dev.of_node);
+	struct sunxi_reset_data *data;

Quoted 20 lines · Boris BREZILLON

The current implementation uses sunxi_reset_init function for both early init and platform device probe.

The sunxi_reset_init function uses DT to retrieve device resources, which will be an issue if reset controllers are registered from an MFD device that define resources from mfd_cell definition.

Moreover, we can make of devm functions when we're in the probe context.

Signed-off-by Boris BREZILLON <[email protected]>

---
 drivers/reset/reset-sunxi.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

drivers/reset/reset-sunxi.c +0 -0

index 695bd34..1b5fea6 100644
@@ -145,7 +145,24 @@ MODULE_DEVICE_TABLE(of, sunxi_reset_dt_ids);
 

Quoted 4 lines

drivers/reset/reset-sunxi.c +1 -1

 static int sunxi_reset_probe(struct platform_device *pdev)
 {
-	return sunxi_reset_init(pdev->dev.of_node);
+	struct sunxi_reset_data *data;

drivers/reset/reset-sunxi.c MODULE_DEVICE_TABLE(of, sunxi_reset_dt_ids); · line 158

Quoted 10 lines

drivers/reset/reset-sunxi.c +10 -0

+	struct resource *res;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	data->membase = devm_request_and_ioremap(&pdev->dev, res);
+	if (!data->membase)
+		return -ENOMEM;

You'd probably be better off using devm_ioremap_resource so that you get a meaningful error code.

Apart from this, you have my

Acked-by Maxime Ripard <[email protected]>

Thanks!
Maxime

The PRCM (Power/Reset/Clock Management) block exposes several subdevices in different subsystems (clk, reset ...)

Add basic support for the PRCM unit with clk (AR100, AHB0, and APB0 clks) and reset controller subdevices.

Other subdevices might be added later (if needed).

Signed-off-by Boris BREZILLON <[email protected]>

The PRCM (Power/Reset/Clock Management) block exposes several subdevices in different subsystems (clk, reset ...)

Add basic support for the PRCM unit with clk (AR100, AHB0, and APB0 clks) and reset controller subdevices.

Other subdevices might be added later (if needed).

Signed-off-by Boris BREZILLON <[email protected]>

---
 drivers/mfd/Kconfig      |   8 +++
 drivers/mfd/Makefile     |   1 +
 drivers/mfd/sun6i-prcm.c | 151 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 160 insertions(+)
 create mode 100644 drivers/mfd/sun6i-prcm.c

drivers/mfd/sun6i-prcm.c new file +8 -0

+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/err.h>
+#include <linux/spinlock.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/mutex.h>

drivers/mfd/sun6i-prcm.c new file +6 -0

+};
+module_platform_driver(sun6i_prcm_driver);
+
+MODULE_AUTHOR("Boris BREZILLON <[email protected]>");
+MODULE_DESCRIPTION("Allwinner sun6i PRCM driver");
+MODULE_LICENSE("GPL v2");

Quoted 15 lines

The PRCM (Power/Reset/Clock Management) block exposes several subdevices in different subsystems (clk, reset ...)

Add basic support for the PRCM unit with clk (AR100, AHB0, and APB0 clks) and reset controller subdevices.

Other subdevices might be added later (if needed).

Signed-off-by Boris BREZILLON <[email protected]>

---
 drivers/mfd/Kconfig      |   8 +++
 drivers/mfd/Makefile     |   1 +
 drivers/mfd/sun6i-prcm.c | 151 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 160 insertions(+)
 create mode 100644 drivers/mfd/sun6i-prcm.c

[...]

Quoted 12 lines

drivers/mfd/sun6i-prcm.c new file +12 -0

+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/err.h>
+#include <linux/spinlock.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/mutex.h>
+#include <linux/completion.h>
+#include <linux/irq.h>
+#include <linux/jiffies.h>
+#include <linux/bitops.h>

drivers/mfd/sun6i-prcm.c line 28

Quoted 6 lines

drivers/mfd/sun6i-prcm.c new file +6 -0

+#include <linux/fs.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/uaccess.h>
+#include <linux/mfd/core.h>

I'm pretty sure you don't need half of these. Please only add the ones you make use of.

[...]

drivers/mfd/sun6i-prcm.c line 134

Quoted 1 line

drivers/mfd/sun6i-prcm.c new file +1 -0

+	dev_info(&pdev->dev, "PRCM initialized\n");

Please remove this from here.

[...]

drivers/mfd/sun6i-prcm.c line 145

Quoted 7 lines

drivers/mfd/sun6i-prcm.c new file +7 -0

+static struct platform_driver sun6i_prcm_driver = {
+	.driver = {
+		.name = "sun6i-prcm",
+		.owner = THIS_MODULE,
+		.of_match_table = sun6i_prcm_dt_ids,
+	},
+	.probe = sun6i_prcm_probe,

You need a .remove() call-back.

Quoted 7 lines

drivers/mfd/sun6i-prcm.c new file +6 -0

+};
+module_platform_driver(sun6i_prcm_driver);
+
+MODULE_AUTHOR("Boris BREZILLON <[email protected]>");
+MODULE_DESCRIPTION("Allwinner sun6i PRCM driver");
+MODULE_LICENSE("GPL v2");

Quoted 29 lines

Quoted 15 lines

The PRCM (Power/Reset/Clock Management) block exposes several subdevices in different subsystems (clk, reset ...)

Add basic support for the PRCM unit with clk (AR100, AHB0, and APB0 clks) and reset controller subdevices.

Other subdevices might be added later (if needed).

Signed-off-by Boris BREZILLON <[email protected]>

---
 drivers/mfd/Kconfig      |   8 +++
 drivers/mfd/Makefile     |   1 +
 drivers/mfd/sun6i-prcm.c | 151 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 160 insertions(+)
 create mode 100644 drivers/mfd/sun6i-prcm.c

[...]

Quoted 12 lines

drivers/mfd/sun6i-prcm.c new file +12 -0

+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/err.h>
+#include <linux/spinlock.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/mutex.h>
+#include <linux/completion.h>
+#include <linux/irq.h>
+#include <linux/jiffies.h>
+#include <linux/bitops.h>

Quoted 8 lines

drivers/mfd/sun6i-prcm.c line 28

Quoted 6 lines

drivers/mfd/sun6i-prcm.c new file +6 -0

+#include <linux/fs.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/uaccess.h>
+#include <linux/mfd/core.h>

I'm pretty sure you don't need half of these. Please only add the ones you make use of.

Sure (this is what happens when you copy/paste from another driver :-))

Quoted 38 lines

Quoted 37 lines

Quoted 15 lines

The PRCM (Power/Reset/Clock Management) block exposes several subdevices in different subsystems (clk, reset ...)

Add basic support for the PRCM unit with clk (AR100, AHB0, and APB0 clks) and reset controller subdevices.

Other subdevices might be added later (if needed).

Signed-off-by Boris BREZILLON <[email protected]>

---
 drivers/mfd/Kconfig      |   8 +++
 drivers/mfd/Makefile     |   1 +
 drivers/mfd/sun6i-prcm.c | 151 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 160 insertions(+)
 create mode 100644 drivers/mfd/sun6i-prcm.c

[...]

Quoted 8 lines

drivers/mfd/sun6i-prcm.c new file +8 -0

+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/err.h>
+#include <linux/spinlock.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/mutex.h>

drivers/mfd/sun6i-prcm.c line 28

Quoted 10 lines

drivers/mfd/sun6i-prcm.c new file +10 -0

+#include <linux/completion.h>
+#include <linux/irq.h>
+#include <linux/jiffies.h>
+#include <linux/bitops.h>
+#include <linux/fs.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/uaccess.h>
+#include <linux/mfd/core.h>

I'm pretty sure you don't need half of these. Please only add the ones you make use of.

Quoted 1 line

Sure (this is what happens when you copy/paste from another driver :-))

Also, alphabetic order would be nice.

Quoted 8 lines

drivers/mfd/sun6i-prcm.c line 145

Quoted 7 lines

drivers/mfd/sun6i-prcm.c new file +7 -0

+static struct platform_driver sun6i_prcm_driver = {
+	.driver = {
+		.name = "sun6i-prcm",
+		.owner = THIS_MODULE,
+		.of_match_table = sun6i_prcm_dt_ids,
+	},
+	.probe = sun6i_prcm_probe,

You need a .remove() call-back.

This driver cannot be compiled as module (see the Kconfig definition) and the devices are not hotpluggable, as a result a probed device will never be removed.

Do you still want me to implement the remove function ?

Quoted 5 lines

Quoted 5 lines

drivers/mfd/sun6i-prcm.c new file +5 -0

+module_platform_driver(sun6i_prcm_driver);
+
+MODULE_AUTHOR("Boris BREZILLON <[email protected]>");
+MODULE_DESCRIPTION("Allwinner sun6i PRCM driver");
+MODULE_LICENSE("GPL v2");

Quoted 9 lines

Quoted 8 lines

drivers/mfd/sun6i-prcm.c line 145

Quoted 7 lines

drivers/mfd/sun6i-prcm.c new file +7 -0

+static struct platform_driver sun6i_prcm_driver = {
+	.driver = {
+		.name = "sun6i-prcm",
+		.owner = THIS_MODULE,
+		.of_match_table = sun6i_prcm_dt_ids,
+	},
+	.probe = sun6i_prcm_probe,

You need a .remove() call-back.

Quoted 5 lines

This driver cannot be compiled as module (see the Kconfig definition) and the devices are not hotpluggable, as a result a probed device will never be removed.

Do you still want me to implement the remove function ?

.remove() also be run on shut down.

It's best practice to have one.

Quoted 6 lines

Quoted 5 lines

Quoted 5 lines

drivers/mfd/sun6i-prcm.c new file +5 -0

+module_platform_driver(sun6i_prcm_driver);
+
+MODULE_AUTHOR("Boris BREZILLON <[email protected]>");
+MODULE_DESCRIPTION("Allwinner sun6i PRCM driver");
+MODULE_LICENSE("GPL v2");

Quoted 13 lines

Quoted 13 lines

Quoted 8 lines

drivers/mfd/sun6i-prcm.c line 145

Quoted 7 lines

drivers/mfd/sun6i-prcm.c new file +7 -0

+static struct platform_driver sun6i_prcm_driver = {
+	.driver = {
+		.name = "sun6i-prcm",
+		.owner = THIS_MODULE,
+		.of_match_table = sun6i_prcm_dt_ids,
+	},
+	.probe = sun6i_prcm_probe,

You need a .remove() call-back.

This driver cannot be compiled as module (see the Kconfig definition) and the devices are not hotpluggable, as a result a probed device will never be removed.

Quoted 3 lines

Quoted 1 line

Do you still want me to implement the remove function ?

.remove() also be run on shut down.

That is not true, if your device needs to do anything special at shutdown you need to add a shutdown callback. Devices are kept as is (not torn down) on shutdown.

Quoted 15 lines

Quoted 15 lines

Quoted 14 lines

Quoted 8 lines

drivers/mfd/sun6i-prcm.c line 145

Quoted 7 lines

drivers/mfd/sun6i-prcm.c new file +7 -0

+static struct platform_driver sun6i_prcm_driver = {
+	.driver = {
+		.name = "sun6i-prcm",
+		.owner = THIS_MODULE,
+		.of_match_table = sun6i_prcm_dt_ids,
+	},
+	.probe = sun6i_prcm_probe,

You need a .remove() call-back.

This driver cannot be compiled as module (see the Kconfig definition) and the devices are not hotpluggable, as a result a probed device will never be removed.

Do you still want me to implement the remove function ?

Quoted 5 lines

Quoted 1 line

.remove() also be run on shut down.

That is not true, if your device needs to do anything special at shutdown you need to add a shutdown callback. Devices are kept as is (not torn down) on shutdown.

Ah okay, I'll bow to your knowledge. So there's no reason for this driver to ever call mfd_remove_devices() then?

Hi,

Quoted 12 lines · Lee Jones

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 8 lines

Review

Quoted 7 lines

(quoted diff) +7 -0

+static struct platform_driver sun6i_prcm_driver = {
+	.driver = {
+		.name = "sun6i-prcm",
+		.owner = THIS_MODULE,
+		.of_match_table = sun6i_prcm_dt_ids,
+	},
+	.probe = sun6i_prcm_probe,

You need a .remove() call-back.

This driver cannot be compiled as module (see the Kconfig definition) and the devices are not hotpluggable, as a result a probed device will never be removed.

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Quoted 1 line

Do you still want me to implement the remove function ?

.remove() also be run on shut down.

That is not true, if your device needs to do anything special at shutdown you need to add a shutdown callback. Devices are kept as is (not torn down) on shutdown.

Ah okay, I'll bow to your knowledge. So there's no reason for this driver to ever call mfd_remove_devices() then?

No, this is an integral part of the SOC, which never gets removed in any way.

Regards,

Hans

Quoted 17 lines

Quoted 17 lines

Quoted 17 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Review

Quoted 7 lines

(quoted diff) +7 -0

+static struct platform_driver sun6i_prcm_driver = {
+	.driver = {
+		.name = "sun6i-prcm",
+		.owner = THIS_MODULE,
+		.of_match_table = sun6i_prcm_dt_ids,
+	},
+	.probe = sun6i_prcm_probe,

You need a .remove() call-back.

Quoted 6 lines

Quoted 5 lines

This driver cannot be compiled as module (see the Kconfig definition) and the devices are not hotpluggable, as a result a probed device will never be removed.

Do you still want me to implement the remove function ?

.remove() also be run on shut down.

That is not true, if your device needs to do anything special at shutdown you need to add a shutdown callback. Devices are kept as is (not torn down) on shutdown.

Quoted 3 lines

Quoted 2 lines

Ah okay, I'll bow to your knowledge. So there's no reason for this driver to ever call mfd_remove_devices() then?

No, this is an integral part of the SOC, which never gets removed in any way.

Lee, I'm about to send a 3rd version of this series, is it okay for you if I leave the remove function unimplemented ?

The PRCM (Power/Reset/Clock Management) unit provides several clock devices:
- AR100 clk: used to clock the Power Management co-processor
- AHB0 clk: used to clock the AHB0 bus
- APB0 clk and gates: used to clk peripherals connected to the APB0 bus

Add support for these clks in a separate driver so that they can be probed as platform devices instead of registered during early init. This is needed to be able to probe PRCM MFD subdevices.

Signed-off-by Boris BREZILLON <[email protected]>

Add DT definitions for PRCM (Power/Reset/Clock Management) clock and reset controller subdevices.

Signed-off-by Boris BREZILLON <[email protected]>

---
 arch/arm/boot/dts/sun6i-a31.dtsi | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

arch/arm/boot/dts/sun6i-a31.dtsi +0 -0

index ec3253a..b69be0b 100644

arch/arm/boot/dts/sun6i-a31.dtsi +19 -0

 			compatible = "allwinner,sun6i-a31-prcm";
 			reg = <0x01f01400 0x200>;
+
+			ar100: ar100_clk {
+				compatible = "allwinner,sun6i-a31-ar100-clk";
+				#clock-cells = <0>;
+				clocks = <&osc32k>, <&osc24M>, <&pll6>, <&pll6>;
+			};
+
+			ahb0: ahb0_clk {
+				compatible = "fixed-factor-clock";
+				#clock-cells = <0>;
+				clock-div = <1>;
+				clock-mult = <1>;
+				clocks = <&ar100>;
+				clock-output-names = "ahb0";
+			};
+
+			apb0: apb0_clk {
+				compatible = "allwinner,sun6i-a31-apb0-clk";
+				#clock-cells = <0>;

arch/arm/boot/dts/sun6i-a31.dtsi +8 -0

+						"apb0_uart", "apb0_1wire",
+						"apb0_i2c";
+			};
+
+			apb0_rst: apb0_rst {
+				compatible = "allwinner,sun6i-a31-clock-reset";
+				#reset-cells = <1>;
+			};
 		};
 	};
 };

Signature · 1 line

1.8.3.2

Quoted 13 lines · Boris BREZILLON

Add DT definitions for PRCM (Power/Reset/Clock Management) clock and reset controller subdevices.

Signed-off-by Boris BREZILLON <[email protected]>

---
 arch/arm/boot/dts/sun6i-a31.dtsi | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

arch/arm/boot/dts/sun6i-a31.dtsi +0 -0

index ec3253a..b69be0b 100644

arch/arm/boot/dts/sun6i-a31.dtsi line 501

Quoted 5 lines

arch/arm/boot/dts/sun6i-a31.dtsi +1 -1

@@ -498,9 +498,46 @@
                      reg = <0x01f01c00 0x300>;
              };
 
-             prcm at 01f01c00 {
+             prcm at 01f01400 {

This has already been fixed by Hans.

Quoted 25 lines

arch/arm/boot/dts/sun6i-a31.dtsi +23 -0

                      compatible = "allwinner,sun6i-a31-prcm";
                      reg = <0x01f01400 0x200>;
+
+                     ar100: ar100_clk {
+                             compatible = "allwinner,sun6i-a31-ar100-clk";
+                             #clock-cells = <0>;
+                             clocks = <&osc32k>, <&osc24M>, <&pll6>, <&pll6>;
+                     };
+
+                     ahb0: ahb0_clk {
+                             compatible = "fixed-factor-clock";
+                             #clock-cells = <0>;
+                             clock-div = <1>;
+                             clock-mult = <1>;
+                             clocks = <&ar100>;
+                             clock-output-names = "ahb0";
+                     };
+
+                     apb0: apb0_clk {
+                             compatible = "allwinner,sun6i-a31-apb0-clk";
+                             #clock-cells = <0>;
+                             clocks = <&ahb0>;
+                             clock-output-names = "apb0";
+                     };
+

arch/arm/boot/dts/sun6i-a31.dtsi line 532

Quoted 6 lines

arch/arm/boot/dts/sun6i-a31.dtsi +6 -0

+                     apb0_gates: apb0_gates_clk {
+                             compatible = "allwinner,sun6i-a31-apb0-gates-clk";
+                             #clock-cells = <1>;
+                             clocks = <&apb0>;
+                             clock-output-names = "apb0_pio", "apb0_ir",
+                                             "apb0_timer01", "apb0_p2wi",

timer01 ? is this a typo?

Quoted 14 lines

arch/arm/boot/dts/sun6i-a31.dtsi +8 -1

+                                             "apb0_uart", "apb0_1wire",
+                                             "apb0_i2c";
+                     };
+
+                     apb0_rst: apb0_rst {
+                             compatible = "allwinner,sun6i-a31-clock-reset";
+                             #reset-cells = <1>;
+                     };
              };
      };
 };
--

1.8.3.2

Quoted 7 lines

Quoted 6 lines

arch/arm/boot/dts/sun6i-a31.dtsi +6 -0

+                     apb0_gates: apb0_gates_clk {
+                             compatible = "allwinner,sun6i-a31-apb0-gates-clk";
+                             #clock-cells = <1>;
+                             clocks = <&apb0>;
+                             clock-output-names = "apb0_pio", "apb0_ir",
+                                             "apb0_timer01", "apb0_p2wi",

Quoted 1 line

timer01 ? is this a typo?

A23 manual lists the clock gate as "r_timer0_1", so I put the name on the wiki. Allwinner sun6i code uses "r_tmr" or just "tmr". I see no problem naming this clock output as "apb0_timer" though.

Quoted 6 lines · Chen-Yu Tsai

Quoted 6 lines

Quoted 6 lines

arch/arm/boot/dts/sun6i-a31.dtsi +6 -0

+                     apb0_gates: apb0_gates_clk {
+                             compatible = "allwinner,sun6i-a31-apb0-gates-clk";
+                             #clock-cells = <1>;
+                             clocks = <&apb0>;
+                             clock-output-names = "apb0_pio", "apb0_ir",
+                                             "apb0_timer01", "apb0_p2wi",

Quoted 4 lines

Quoted 1 line

timer01 ? is this a typo?

A23 manual lists the clock gate as "r_timer0_1", so I put the name on the wiki. Allwinner sun6i code uses "r_tmr" or just "tmr". I see no problem naming this clock output as "apb0_timer" though.

Yep, it seems better.

Maxime