sun6i / A31 ir receiver support

Before this commit sunxi_factors_register uses of_iomap(node, 0) to get the clk registers. The sun6i prcm has factor clocks, for which we want to use sunxi_factors_register, but of_iomap(node, 0) does not work for the prcm factor clocks, because the prcm uses the mfd framework, so the registers are not part of the dt-node, instead they are added to the platform_device, as platform_device resources.

This commit makes getting the registers the callers duty, so that sunxi_factors_register can be used with mfd instantiated platform device too.

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/clk/sunxi/clk-factors.c    | 10 ++++------
 drivers/clk/sunxi/clk-factors.h    |  7 ++++---
 drivers/clk/sunxi/clk-mod0.c       |  6 ++++--
 drivers/clk/sunxi/clk-sun8i-mbus.c |  2 +-
 drivers/clk/sunxi/clk-sunxi.c      |  3 ++-
 5 files changed, 15 insertions(+), 13 deletions(-)

drivers/clk/sunxi/clk-factors.c +4 -6

index f83ba09..fc4f4b5 100644
@@ -156,9 +156,10 @@ static const struct clk_ops clk_factors_ops = {
 	.set_rate = clk_factors_set_rate,
 };
 
-struct clk * __init sunxi_factors_register(struct device_node *node,
-					   const struct factors_data *data,
-					   spinlock_t *lock)
+struct clk *sunxi_factors_register(struct device_node *node,
+				   const struct factors_data *data,
+				   spinlock_t *lock,
+				   void __iomem *reg)
 {
 	struct clk *clk;
 	struct clk_factors *factors;

@@ -168,11 +169,8 @@ struct clk * __init sunxi_factors_register(struct device_node *node,
 	struct clk_hw *mux_hw = NULL;
 	const char *clk_name = node->name;
 	const char *parents[FACTORS_MAX_PARENTS];
-	void __iomem *reg;
 	int i = 0;
 
-	reg = of_iomap(node, 0);
-
 	/* if we have a mux, we will have >1 parents */
 	while (i < FACTORS_MAX_PARENTS &&
 	       (parents[i] = of_clk_get_parent_name(node, i)) != NULL)

drivers/clk/sunxi/clk-factors.h +0 -0

index 9913840..1f5526d 100644

Hi Hans,

Quoted 11 lines · Hans de Goede

Before this commit sunxi_factors_register uses of_iomap(node, 0) to get the clk registers. The sun6i prcm has factor clocks, for which we want to use sunxi_factors_register, but of_iomap(node, 0) does not work for the prcm factor clocks, because the prcm uses the mfd framework, so the registers are not part of the dt-node, instead they are added to the platform_device, as platform_device resources.

This commit makes getting the registers the callers duty, so that sunxi_factors_register can be used with mfd instantiated platform device too.

Signed-off-by Hans de Goede <[email protected]>

Funny, I was thinking of doing exactly the same thing for MMC clocks :)

Quoted 46 lines

---
 drivers/clk/sunxi/clk-factors.c    | 10 ++++------
 drivers/clk/sunxi/clk-factors.h    |  7 ++++---
 drivers/clk/sunxi/clk-mod0.c       |  6 ++++--
 drivers/clk/sunxi/clk-sun8i-mbus.c |  2 +-
 drivers/clk/sunxi/clk-sunxi.c      |  3 ++-
 5 files changed, 15 insertions(+), 13 deletions(-)

drivers/clk/sunxi/clk-factors.c +4 -6

index f83ba09..fc4f4b5 100644
@@ -156,9 +156,10 @@ static const struct clk_ops clk_factors_ops = {
 	.set_rate = clk_factors_set_rate,
 };
 
-struct clk * __init sunxi_factors_register(struct device_node *node,
-					   const struct factors_data *data,
-					   spinlock_t *lock)
+struct clk *sunxi_factors_register(struct device_node *node,
+				   const struct factors_data *data,
+				   spinlock_t *lock,
+				   void __iomem *reg)
 {
 	struct clk *clk;
 	struct clk_factors *factors;

@@ -168,11 +169,8 @@ struct clk * __init sunxi_factors_register(struct device_node *node,
 	struct clk_hw *mux_hw = NULL;
 	const char *clk_name = node->name;
 	const char *parents[FACTORS_MAX_PARENTS];
-	void __iomem *reg;
 	int i = 0;
 
-	reg = of_iomap(node, 0);
-
 	/* if we have a mux, we will have >1 parents */
 	while (i < FACTORS_MAX_PARENTS &&
 	       (parents[i] = of_clk_get_parent_name(node, i)) != NULL)

drivers/clk/sunxi/clk-factors.h +0 -0

index 9913840..1f5526d 100644
@@ -37,8 +37,9 @@ struct clk_factors {
 	spinlock_t *lock;
 };
 

drivers/clk/sunxi/clk-factors.h struct clk_factors { · line 43

Quoted 7 lines

drivers/clk/sunxi/clk-factors.h +4 -3

-struct clk * __init sunxi_factors_register(struct device_node *node,
-					   const struct factors_data *data,
-					   spinlock_t *lock);
+struct clk *sunxi_factors_register(struct device_node *node,
+				   const struct factors_data *data,
+				   spinlock_t *lock,
+				   void __iomem *reg);

Why are you dropping the __init there?

Quoted 8 lines

drivers/clk/sunxi/clk-factors.h +0 -0

 #endif

drivers/clk/sunxi/clk-mod0.c +0 -0

index 4a56385..9530833 100644

drivers/clk/sunxi/clk-mod0.c static DEFINE_SPINLOCK(sun4i_a10_mod0_lock); · line 82

Quoted 6 lines

drivers/clk/sunxi/clk-mod0.c +2 -1

@@ -78,7 +78,8 @@ static DEFINE_SPINLOCK(sun4i_a10_mod0_lock);
 
 static void __init sun4i_a10_mod0_setup(struct device_node *node)
 {
-	sunxi_factors_register(node, &sun4i_a10_mod0_data, &sun4i_a10_mod0_lock);
+	sunxi_factors_register(node, &sun4i_a10_mod0_data,
+			       &sun4i_a10_mod0_lock, of_iomap(node, 0));

As of_iomap can fail, I'd rather check the returned value before calling sunxi_factors_register.

I know it wasn't done before, but it's the right thing to do, as it would lead to an instant crash if that fails.

Hi,

Quoted 63 lines · Maxime Ripard

Hi Hans,

Quoted 11 lines · Hans de Goede

Before this commit sunxi_factors_register uses of_iomap(node, 0) to get the clk registers. The sun6i prcm has factor clocks, for which we want to use sunxi_factors_register, but of_iomap(node, 0) does not work for the prcm factor clocks, because the prcm uses the mfd framework, so the registers are not part of the dt-node, instead they are added to the platform_device, as platform_device resources.

This commit makes getting the registers the callers duty, so that sunxi_factors_register can be used with mfd instantiated platform device too.

Signed-off-by Hans de Goede <[email protected]>

Funny, I was thinking of doing exactly the same thing for MMC clocks :)

Quoted 46 lines

---
 drivers/clk/sunxi/clk-factors.c    | 10 ++++------
 drivers/clk/sunxi/clk-factors.h    |  7 ++++---
 drivers/clk/sunxi/clk-mod0.c       |  6 ++++--
 drivers/clk/sunxi/clk-sun8i-mbus.c |  2 +-
 drivers/clk/sunxi/clk-sunxi.c      |  3 ++-
 5 files changed, 15 insertions(+), 13 deletions(-)

drivers/clk/sunxi/clk-factors.c +4 -6

index f83ba09..fc4f4b5 100644
@@ -156,9 +156,10 @@ static const struct clk_ops clk_factors_ops = {
 	.set_rate = clk_factors_set_rate,
 };
 
-struct clk * __init sunxi_factors_register(struct device_node *node,
-					   const struct factors_data *data,
-					   spinlock_t *lock)
+struct clk *sunxi_factors_register(struct device_node *node,
+				   const struct factors_data *data,
+				   spinlock_t *lock,
+				   void __iomem *reg)
 {
 	struct clk *clk;
 	struct clk_factors *factors;

@@ -168,11 +169,8 @@ struct clk * __init sunxi_factors_register(struct device_node *node,
 	struct clk_hw *mux_hw = NULL;
 	const char *clk_name = node->name;
 	const char *parents[FACTORS_MAX_PARENTS];
-	void __iomem *reg;
 	int i = 0;
 
-	reg = of_iomap(node, 0);
-
 	/* if we have a mux, we will have >1 parents */
 	while (i < FACTORS_MAX_PARENTS &&
 	       (parents[i] = of_clk_get_parent_name(node, i)) != NULL)

drivers/clk/sunxi/clk-factors.h +0 -0

index 9913840..1f5526d 100644
@@ -37,8 +37,9 @@ struct clk_factors {
 	spinlock_t *lock;
 };
 

Quoted 9 lines

drivers/clk/sunxi/clk-factors.h struct clk_factors { · line 43

Quoted 7 lines

drivers/clk/sunxi/clk-factors.h +4 -3

-struct clk * __init sunxi_factors_register(struct device_node *node,
-					   const struct factors_data *data,
-					   spinlock_t *lock);
+struct clk *sunxi_factors_register(struct device_node *node,
+				   const struct factors_data *data,
+				   spinlock_t *lock,
+				   void __iomem *reg);

Why are you dropping the __init there?

Because it is going to be used from mfd instantiation, so from a platform_dev probe function which is not __init.

Quoted 19 lines

Quoted 9 lines

drivers/clk/sunxi/clk-factors.h +0 -0

 #endif

drivers/clk/sunxi/clk-mod0.c +0 -0

index 4a56385..9530833 100644

drivers/clk/sunxi/clk-mod0.c static DEFINE_SPINLOCK(sun4i_a10_mod0_lock); · line 82

Quoted 6 lines

drivers/clk/sunxi/clk-mod0.c +2 -1

@@ -78,7 +78,8 @@ static DEFINE_SPINLOCK(sun4i_a10_mod0_lock);
 
 static void __init sun4i_a10_mod0_setup(struct device_node *node)
 {
-	sunxi_factors_register(node, &sun4i_a10_mod0_data, &sun4i_a10_mod0_lock);
+	sunxi_factors_register(node, &sun4i_a10_mod0_data,
+			       &sun4i_a10_mod0_lock, of_iomap(node, 0));

As of_iomap can fail, I'd rather check the returned value before calling sunxi_factors_register.

Quoted 2 lines

I know it wasn't done before, but it's the right thing to do, as it would lead to an instant crash if that fails.

Ok, I'll wait for you to review the rest of the series and then do a v2 of the patch-set with this fixed (as time permits).

Add a driver for mod0 clocks found in the prcm. Currently there is only one mod0 clocks in the prcm, the ir clock.

Signed-off-by Hans de Goede <[email protected]>

---
 Documentation/devicetree/bindings/clock/sunxi.txt |  1 +
 drivers/clk/sunxi/Makefile                        |  2 +-
 drivers/clk/sunxi/clk-sun6i-prcm-mod0.c           | 63 +++++++++++++++++++++++
 drivers/mfd/sun6i-prcm.c                          | 14 +++++
 4 files changed, 79 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/sunxi/clk-sun6i-prcm-mod0.c

Documentation/devicetree/bindings/clock/sunxi.txt +1 -0

index ed116df..342c75a 100644
@@ -56,6 +56,7 @@ Required properties:
        "allwinner,sun4i-a10-usb-clk" - for usb gates + resets on A10 / A20
        "allwinner,sun5i-a13-usb-clk" - for usb gates + resets on A13
        "allwinner,sun6i-a31-usb-clk" - for usb gates + resets on A31
+       "allwinner,sun6i-a31-ir-clk" - for the ir clock on A31
 
 Required properties for all clocks:
 - reg : shall be the control register address for the clock.

drivers/clk/sunxi/Makefile +1 -1

index 7ddc2b5..daf8b1c 100644
@@ -10,4 +10,4 @@ obj-y += clk-sun8i-mbus.o
 
 obj-$(CONFIG_MFD_SUN6I_PRCM) += \
        clk-sun6i-ar100.o clk-sun6i-apb0.o clk-sun6i-apb0-gates.o \
-       clk-sun8i-apb0.o
+       clk-sun8i-apb0.o clk-sun6i-prcm-mod0.o

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c new file +17 -0

new file mode 100644
index 0000000..e80f18e
@@ -0,0 +1,63 @@
+/*
+ * Allwinner A31 PRCM mod0 clock driver
+ *
+ * Copyright (C) 2014 Hans de Goede <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk-provider.h>

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c new file +36 -0

+       { /* sentinel */ }
+};
+
+static DEFINE_SPINLOCK(sun6i_prcm_mod0_lock);
+
+static int sun6i_a31_prcm_mod0_clk_probe(struct platform_device *pdev)
+{
+       struct device_node *np = pdev->dev.of_node;
+       struct resource *r;
+       void __iomem *reg;
+
+       if (!np)
+               return -ENODEV;
+
+       r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       reg = devm_ioremap_resource(&pdev->dev, r);
+       if (IS_ERR(reg))
+               return PTR_ERR(reg);
+
+       sunxi_factors_register(np, &sun4i_a10_mod0_data,
+                              &sun6i_prcm_mod0_lock, reg);
+       return 0;
+}
+
+static struct platform_driver sun6i_a31_prcm_mod0_clk_driver = {
+       .driver = {
+               .name = "sun6i-a31-prcm-mod0-clk",
+               .of_match_table = sun6i_a31_prcm_mod0_clk_dt_ids,
+       },
+       .probe = sun6i_a31_prcm_mod0_clk_probe,
+};
+module_platform_driver(sun6i_a31_prcm_mod0_clk_driver);
+
+MODULE_DESCRIPTION("Allwinner A31 PRCM mod0 clock driver");
+MODULE_AUTHOR("Hans de Goede <[email protected]>");
+MODULE_LICENSE("GPL");

drivers/mfd/sun6i-prcm.c +14 -1

index 283ab8d..ff1254f 100644
@@ -41,6 +41,14 @@ static const struct resource sun6i_a31_apb0_gates_clk_res[] = {
        },
 };
 
+static const struct resource sun6i_a31_ir_clk_res[] = {
+       {
+               .start = 0x54,
+               .end = 0x57,
+               .flags = IORESOURCE_MEM,
+       },
+};
+
 static const struct resource sun6i_a31_apb0_rstc_res[] = {
        {
                .start = 0xb0,

@@ -69,6 +77,12 @@ static const struct mfd_cell sun6i_a31_prcm_subdevs[] = {
                .resources = sun6i_a31_apb0_gates_clk_res,
        },
        {
+               .name = "sun6i-a31-ir-clk",
+               .of_compatible = "allwinner,sun6i-a31-ir-clk",
+               .num_resources = ARRAY_SIZE(sun6i_a31_ir_clk_res),
+               .resources = sun6i_a31_ir_clk_res,
+       },
+       {
                .name = "sun6i-a31-apb0-clock-reset",
                .of_compatible = "allwinner,sun6i-a31-clock-reset",
                .num_resources = ARRAY_SIZE(sun6i_a31_apb0_rstc_res),
--

2.1.0

--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe at googlegroups.com. For more options, visit https://groups.google.com/d/optout.

Hi,

Quoted 62 lines · Hans de Goede

Add a driver for mod0 clocks found in the prcm. Currently there is only one mod0 clocks in the prcm, the ir clock.

Signed-off-by Hans de Goede <[email protected]>

---
 Documentation/devicetree/bindings/clock/sunxi.txt |  1 +
 drivers/clk/sunxi/Makefile                        |  2 +-
 drivers/clk/sunxi/clk-sun6i-prcm-mod0.c           | 63 +++++++++++++++++++++++
 drivers/mfd/sun6i-prcm.c                          | 14 +++++
 4 files changed, 79 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/sunxi/clk-sun6i-prcm-mod0.c

Documentation/devicetree/bindings/clock/sunxi.txt +1 -0

index ed116df..342c75a 100644
@@ -56,6 +56,7 @@ Required properties:
        "allwinner,sun4i-a10-usb-clk" - for usb gates + resets on A10 / A20
        "allwinner,sun5i-a13-usb-clk" - for usb gates + resets on A13
        "allwinner,sun6i-a31-usb-clk" - for usb gates + resets on A31
+       "allwinner,sun6i-a31-ir-clk" - for the ir clock on A31
 
 Required properties for all clocks:
 - reg : shall be the control register address for the clock.

drivers/clk/sunxi/Makefile +1 -1

index 7ddc2b5..daf8b1c 100644
@@ -10,4 +10,4 @@ obj-y += clk-sun8i-mbus.o
 
 obj-$(CONFIG_MFD_SUN6I_PRCM) += \
        clk-sun6i-ar100.o clk-sun6i-apb0.o clk-sun6i-apb0-gates.o \
-       clk-sun8i-apb0.o
+       clk-sun8i-apb0.o clk-sun6i-prcm-mod0.o

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c new file +22 -0

new file mode 100644
index 0000000..e80f18e
@@ -0,0 +1,63 @@
+/*
+ * Allwinner A31 PRCM mod0 clock driver
+ *
+ * Copyright (C) 2014 Hans de Goede <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c line 27

Quoted 5 lines

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c new file +5 -0

+#include "clk-factors.h"
+#include "clk-mod0.h"
+
+static const struct of_device_id sun6i_a31_prcm_mod0_clk_dt_ids[] = {
+       { .compatible = "allwinner,sun6i-a31-ir-clk" },

Could we use a generic name, like "sun6i-a31-prcm-mod0-clk"? IIRC, there is another one, the module clock for the 1-wire interface.

Add a driver for mod0 clocks found in the prcm. Currently there is only one mod0 clocks in the prcm, the ir clock.

Signed-off-by Hans de Goede <[email protected]>

---
 Documentation/devicetree/bindings/clock/sunxi.txt |  1 +
 drivers/clk/sunxi/Makefile                        |  2 +-
 drivers/clk/sunxi/clk-sun6i-prcm-mod0.c           | 63 +++++++++++++++++++++++
 drivers/mfd/sun6i-prcm.c                          | 14 +++++
 4 files changed, 79 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/sunxi/clk-sun6i-prcm-mod0.c

Documentation/devicetree/bindings/clock/sunxi.txt +1 -0

index ed116df..342c75a 100644
@@ -56,6 +56,7 @@ Required properties:
 	"allwinner,sun4i-a10-usb-clk" - for usb gates + resets on A10 / A20
 	"allwinner,sun5i-a13-usb-clk" - for usb gates + resets on A13
 	"allwinner,sun6i-a31-usb-clk" - for usb gates + resets on A31
+	"allwinner,sun6i-a31-ir-clk" - for the ir clock on A31
 
 Required properties for all clocks:
 - reg : shall be the control register address for the clock.

drivers/clk/sunxi/Makefile +1 -1

index 7ddc2b5..daf8b1c 100644
@@ -10,4 +10,4 @@ obj-y += clk-sun8i-mbus.o
 
 obj-$(CONFIG_MFD_SUN6I_PRCM) += \
 	clk-sun6i-ar100.o clk-sun6i-apb0.o clk-sun6i-apb0-gates.o \
-	clk-sun8i-apb0.o
+	clk-sun8i-apb0.o clk-sun6i-prcm-mod0.o

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c new file +53 -0

new file mode 100644
index 0000000..e80f18e
@@ -0,0 +1,63 @@
+/*
+ * Allwinner A31 PRCM mod0 clock driver
+ *
+ * Copyright (C) 2014 Hans de Goede <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+
+#include "clk-factors.h"
+#include "clk-mod0.h"
+
+static const struct of_device_id sun6i_a31_prcm_mod0_clk_dt_ids[] = {
+	{ .compatible = "allwinner,sun6i-a31-ir-clk" },
+	{ /* sentinel */ }
+};
+
+static DEFINE_SPINLOCK(sun6i_prcm_mod0_lock);
+
+static int sun6i_a31_prcm_mod0_clk_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct resource *r;
+	void __iomem *reg;
+
+	if (!np)
+		return -ENODEV;
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	reg = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(reg))
+		return PTR_ERR(reg);
+
+	sunxi_factors_register(np, &sun4i_a10_mod0_data,
+			       &sun6i_prcm_mod0_lock, reg);
+	return 0;
+}
+
+static struct platform_driver sun6i_a31_prcm_mod0_clk_driver = {
+	.driver = {

Hi,

Quoted 93 lines · Hans de Goede

Add a driver for mod0 clocks found in the prcm. Currently there is only one mod0 clocks in the prcm, the ir clock.

Signed-off-by Hans de Goede <[email protected]>

---
 Documentation/devicetree/bindings/clock/sunxi.txt |  1 +
 drivers/clk/sunxi/Makefile                        |  2 +-
 drivers/clk/sunxi/clk-sun6i-prcm-mod0.c           | 63 +++++++++++++++++++++++
 drivers/mfd/sun6i-prcm.c                          | 14 +++++
 4 files changed, 79 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/sunxi/clk-sun6i-prcm-mod0.c

Documentation/devicetree/bindings/clock/sunxi.txt +1 -0

index ed116df..342c75a 100644
@@ -56,6 +56,7 @@ Required properties:
 	"allwinner,sun4i-a10-usb-clk" - for usb gates + resets on A10 / A20
 	"allwinner,sun5i-a13-usb-clk" - for usb gates + resets on A13
 	"allwinner,sun6i-a31-usb-clk" - for usb gates + resets on A31
+	"allwinner,sun6i-a31-ir-clk" - for the ir clock on A31
 
 Required properties for all clocks:
 - reg : shall be the control register address for the clock.

drivers/clk/sunxi/Makefile +1 -1

index 7ddc2b5..daf8b1c 100644
@@ -10,4 +10,4 @@ obj-y += clk-sun8i-mbus.o
 
 obj-$(CONFIG_MFD_SUN6I_PRCM) += \
 	clk-sun6i-ar100.o clk-sun6i-apb0.o clk-sun6i-apb0-gates.o \
-	clk-sun8i-apb0.o
+	clk-sun8i-apb0.o clk-sun6i-prcm-mod0.o

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c new file +53 -0

new file mode 100644
index 0000000..e80f18e
@@ -0,0 +1,63 @@
+/*
+ * Allwinner A31 PRCM mod0 clock driver
+ *
+ * Copyright (C) 2014 Hans de Goede <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+
+#include "clk-factors.h"
+#include "clk-mod0.h"
+
+static const struct of_device_id sun6i_a31_prcm_mod0_clk_dt_ids[] = {
+	{ .compatible = "allwinner,sun6i-a31-ir-clk" },
+	{ /* sentinel */ }
+};
+
+static DEFINE_SPINLOCK(sun6i_prcm_mod0_lock);
+
+static int sun6i_a31_prcm_mod0_clk_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct resource *r;
+	void __iomem *reg;
+
+	if (!np)
+		return -ENODEV;
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	reg = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(reg))
+		return PTR_ERR(reg);
+
+	sunxi_factors_register(np, &sun4i_a10_mod0_data,
+			       &sun6i_prcm_mod0_lock, reg);
+	return 0;
+}
+
+static struct platform_driver sun6i_a31_prcm_mod0_clk_driver = {
+	.driver = {

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c line 63

Quoted 10 lines

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c new file +10 -0

+		.name = "sun6i-a31-prcm-mod0-clk",
+		.of_match_table = sun6i_a31_prcm_mod0_clk_dt_ids,
+	},
+	.probe = sun6i_a31_prcm_mod0_clk_probe,
+};
+module_platform_driver(sun6i_a31_prcm_mod0_clk_driver);
+
+MODULE_DESCRIPTION("Allwinner A31 PRCM mod0 clock driver");
+MODULE_AUTHOR("Hans de Goede <[email protected]>");
+MODULE_LICENSE("GPL");

I don't think this is the right approach, mainly for two reasons: the compatible shouldn't change, and you're basically duplicating code there.

I understand that you need the new compatible in order to avoid a double probing: one by CLK_OF_DECLARE, and one by the usual mechanism, and that also implies the second reason.

However, as those are not critical clocks that need to be here early at boot, you can also fix this by turning the mod0 driver into a platform driver itself. The compatible will be kept, the driver will be the same.

The only thing we need to pay attention to is how "client" drivers react when they cannot grab their clock. They should return -EPROBE_DEFER, but that remains to be checked.

Hi,

Quoted 111 lines · Maxime Ripard

Hi,

Quoted 93 lines · Hans de Goede

Add a driver for mod0 clocks found in the prcm. Currently there is only one mod0 clocks in the prcm, the ir clock.

Signed-off-by Hans de Goede <[email protected]>

---
 Documentation/devicetree/bindings/clock/sunxi.txt |  1 +
 drivers/clk/sunxi/Makefile                        |  2 +-
 drivers/clk/sunxi/clk-sun6i-prcm-mod0.c           | 63 +++++++++++++++++++++++
 drivers/mfd/sun6i-prcm.c                          | 14 +++++
 4 files changed, 79 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/sunxi/clk-sun6i-prcm-mod0.c

Documentation/devicetree/bindings/clock/sunxi.txt +1 -0

index ed116df..342c75a 100644
@@ -56,6 +56,7 @@ Required properties:
 	"allwinner,sun4i-a10-usb-clk" - for usb gates + resets on A10 / A20
 	"allwinner,sun5i-a13-usb-clk" - for usb gates + resets on A13
 	"allwinner,sun6i-a31-usb-clk" - for usb gates + resets on A31
+	"allwinner,sun6i-a31-ir-clk" - for the ir clock on A31
 
 Required properties for all clocks:
 - reg : shall be the control register address for the clock.

drivers/clk/sunxi/Makefile +1 -1

index 7ddc2b5..daf8b1c 100644
@@ -10,4 +10,4 @@ obj-y += clk-sun8i-mbus.o
 
 obj-$(CONFIG_MFD_SUN6I_PRCM) += \
 	clk-sun6i-ar100.o clk-sun6i-apb0.o clk-sun6i-apb0-gates.o \
-	clk-sun8i-apb0.o
+	clk-sun8i-apb0.o clk-sun6i-prcm-mod0.o

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c new file +53 -0

new file mode 100644
index 0000000..e80f18e
@@ -0,0 +1,63 @@
+/*
+ * Allwinner A31 PRCM mod0 clock driver
+ *
+ * Copyright (C) 2014 Hans de Goede <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+
+#include "clk-factors.h"
+#include "clk-mod0.h"
+
+static const struct of_device_id sun6i_a31_prcm_mod0_clk_dt_ids[] = {
+	{ .compatible = "allwinner,sun6i-a31-ir-clk" },
+	{ /* sentinel */ }
+};
+
+static DEFINE_SPINLOCK(sun6i_prcm_mod0_lock);
+
+static int sun6i_a31_prcm_mod0_clk_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct resource *r;
+	void __iomem *reg;
+
+	if (!np)
+		return -ENODEV;
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	reg = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(reg))
+		return PTR_ERR(reg);
+
+	sunxi_factors_register(np, &sun4i_a10_mod0_data,
+			       &sun6i_prcm_mod0_lock, reg);
+	return 0;
+}
+
+static struct platform_driver sun6i_a31_prcm_mod0_clk_driver = {
+	.driver = {

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c line 63

Quoted 10 lines

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c new file +10 -0

+		.name = "sun6i-a31-prcm-mod0-clk",
+		.of_match_table = sun6i_a31_prcm_mod0_clk_dt_ids,
+	},
+	.probe = sun6i_a31_prcm_mod0_clk_probe,
+};
+module_platform_driver(sun6i_a31_prcm_mod0_clk_driver);
+
+MODULE_DESCRIPTION("Allwinner A31 PRCM mod0 clock driver");
+MODULE_AUTHOR("Hans de Goede <[email protected]>");
+MODULE_LICENSE("GPL");

I don't think this is the right approach, mainly for two reasons: the compatible shouldn't change, and you're basically duplicating code there.

Quoted 3 lines

I understand that you need the new compatible in order to avoid a double probing: one by CLK_OF_DECLARE, and one by the usual mechanism, and that also implies the second reason.

Not only for that, we need a new compatible also because the mfd framework needs a separate compatible per sub-node as that is how it finds nodes to attach to the platform devices, so we need a new compatible anyways, with your make the mod0 clock driver a platform driver solution we could use:

	compatible = "allwinner,sun6i-a31-ir-clk", "allwinner,sun4i-a10-mod0-clk";

To work around this, but there are other problems, your make mod0clk a platform driver solution cannot work because the clocks node in the dtsi is not simple-bus compatible, so no platform-devs will be instantiated for the clocks there.

Besides the compatible (which as said we need a separate one anyways), your other worry is code duplication, but I've avoided that as much as possible already, the new drivers/clk/sunxi/clk-sun6i-prcm-mod0.c is just a very thin wrapper, waying in with all of 63 lines including 16 lines GPL header.

So sorry, I disagree I believe that this is the best solution.

Quoted 8 lines

However, as those are not critical clocks that need to be here early at boot, you can also fix this by turning the mod0 driver into a platform driver itself. The compatible will be kept, the driver will be the same.

The only thing we need to pay attention to is how "client" drivers react when they cannot grab their clock. They should return -EPROBE_DEFER, but that remains to be checked.

-EPROBE_DEFER is yet another reason to not make mod0-clk a platform driver. Yes drivers should be able to handle this, but it is a pain, and the more we use it the more pain it is. Also it makes booting a lot slower as any driver using a mod0 clk now, will not complete its probe until all the other drivers are done probing and the late_initcall which starts the deferred-probe wq runs.

Quoted 118 lines · Hans de Goede

Hi,

Quoted 114 lines · Maxime Ripard

Hi,

Quoted 93 lines · Hans de Goede

Add a driver for mod0 clocks found in the prcm. Currently there is only one mod0 clocks in the prcm, the ir clock.

Signed-off-by Hans de Goede <[email protected]>

---
  Documentation/devicetree/bindings/clock/sunxi.txt |  1 +
  drivers/clk/sunxi/Makefile                        |  2 +-
  drivers/clk/sunxi/clk-sun6i-prcm-mod0.c           | 63 +++++++++++++++++++++++
  drivers/mfd/sun6i-prcm.c                          | 14 +++++
  4 files changed, 79 insertions(+), 1 deletion(-)

create mode 100644 drivers/clk/sunxi/clk-sun6i-prcm-mod0.c

Documentation/devicetree/bindings/clock/sunxi.txt +1 -0

index ed116df..342c75a 100644
@@ -56,6 +56,7 @@ Required properties:
  	"allwinner,sun4i-a10-usb-clk" - for usb gates + resets on A10 / A20
  	"allwinner,sun5i-a13-usb-clk" - for usb gates + resets on A13
  	"allwinner,sun6i-a31-usb-clk" - for usb gates + resets on A31
+	"allwinner,sun6i-a31-ir-clk" - for the ir clock on A31
 
  Required properties for all clocks:
  - reg : shall be the control register address for the clock.

drivers/clk/sunxi/Makefile +1 -1

index 7ddc2b5..daf8b1c 100644
@@ -10,4 +10,4 @@ obj-y += clk-sun8i-mbus.o
 
  obj-$(CONFIG_MFD_SUN6I_PRCM) += \
  	clk-sun6i-ar100.o clk-sun6i-apb0.o clk-sun6i-apb0-gates.o \
-	clk-sun8i-apb0.o
+	clk-sun8i-apb0.o clk-sun6i-prcm-mod0.o

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c new file +53 -0

new file mode 100644
index 0000000..e80f18e
@@ -0,0 +1,63 @@
+/*
+ * Allwinner A31 PRCM mod0 clock driver
+ *
+ * Copyright (C) 2014 Hans de Goede <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+
+#include "clk-factors.h"
+#include "clk-mod0.h"
+
+static const struct of_device_id sun6i_a31_prcm_mod0_clk_dt_ids[] = {
+	{ .compatible = "allwinner,sun6i-a31-ir-clk" },
+	{ /* sentinel */ }
+};
+
+static DEFINE_SPINLOCK(sun6i_prcm_mod0_lock);
+
+static int sun6i_a31_prcm_mod0_clk_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct resource *r;
+	void __iomem *reg;
+
+	if (!np)
+		return -ENODEV;
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	reg = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(reg))
+		return PTR_ERR(reg);
+
+	sunxi_factors_register(np, &sun4i_a10_mod0_data,
+			       &sun6i_prcm_mod0_lock, reg);
+	return 0;
+}
+
+static struct platform_driver sun6i_a31_prcm_mod0_clk_driver = {
+	.driver = {

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c line 63

Quoted 10 lines

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c new file +10 -0

+		.name = "sun6i-a31-prcm-mod0-clk",
+		.of_match_table = sun6i_a31_prcm_mod0_clk_dt_ids,
+	},
+	.probe = sun6i_a31_prcm_mod0_clk_probe,
+};
+module_platform_driver(sun6i_a31_prcm_mod0_clk_driver);
+
+MODULE_DESCRIPTION("Allwinner A31 PRCM mod0 clock driver");
+MODULE_AUTHOR("Hans de Goede <[email protected]>");
+MODULE_LICENSE("GPL");

I don't think this is the right approach, mainly for two reasons: the compatible shouldn't change, and you're basically duplicating code there.

I understand that you need the new compatible in order to avoid a double probing: one by CLK_OF_DECLARE, and one by the usual mechanism, and that also implies the second reason.

Quoted 5 lines

Not only for that, we need a new compatible also because the mfd framework needs a separate compatible per sub-node as that is how it finds nodes to attach to the platform devices, so we need a new compatible anyways, with your make the mod0 clock driver a platform driver solution we could use:

We have a single mod0 clock in there, so no, not really.

Plus, that seems like a bogus limitation from MFD, and it really shouldn't work that way.

Quoted 6 lines

	compatible = "allwinner,sun6i-a31-ir-clk", "allwinner,sun4i-a10-mod0-clk";

To work around this, but there are other problems, your make mod0clk a platform driver solution cannot work because the clocks node in the dtsi is not simple-bus compatible, so no platform-devs will be instantiated for the clocks there.

Then add the simple-bus compatible.

Quoted 5 lines

Besides the compatible (which as said we need a separate one anyways), your other worry is code duplication, but I've avoided that as much as possible already, the new drivers/clk/sunxi/clk-sun6i-prcm-mod0.c is just a very thin wrapper, waying in with all of 63 lines including 16 lines GPL header.

47 lines that are doing exactly the same thing as the other code is doing. I'm sorry but you can't really argue it's not code duplication, it really is.

Quoted 11 lines

So sorry, I disagree I believe that this is the best solution.

Quoted 8 lines

However, as those are not critical clocks that need to be here early at boot, you can also fix this by turning the mod0 driver into a platform driver itself. The compatible will be kept, the driver will be the same.

The only thing we need to pay attention to is how "client" drivers react when they cannot grab their clock. They should return -EPROBE_DEFER, but that remains to be checked.

Quoted 6 lines

-EPROBE_DEFER is yet another reason to not make mod0-clk a platform driver. Yes drivers should be able to handle this, but it is a pain, and the more we use it the more pain it is. Also it makes booting a lot slower as any driver using a mod0 clk now, will not complete its probe until all the other drivers are done probing and the late_initcall which starts the deferred-probe wq runs.

The whole EPROBE_DEFER mechanism might need some fixing, but it's not really the point is it?

Well one reasons why clocks are instantiated the way they are is to have them available as early as possible, which is really convenient and works really well.

You are asking for a whole lot of stuff to be changed, arguably in a way which makes it worse, just to save 47 lines of code...

Hi,

Quoted 125 lines · Maxime Ripard

Quoted 123 lines · Hans de Goede

Hi,

Quoted 111 lines · Maxime Ripard

Hi,

Quoted 93 lines · Hans de Goede

Add a driver for mod0 clocks found in the prcm. Currently there is only one mod0 clocks in the prcm, the ir clock.

Signed-off-by Hans de Goede <[email protected]>

---
 Documentation/devicetree/bindings/clock/sunxi.txt |  1 +
 drivers/clk/sunxi/Makefile                        |  2 +-
 drivers/clk/sunxi/clk-sun6i-prcm-mod0.c           | 63 +++++++++++++++++++++++
 drivers/mfd/sun6i-prcm.c                          | 14 +++++
 4 files changed, 79 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/sunxi/clk-sun6i-prcm-mod0.c

Documentation/devicetree/bindings/clock/sunxi.txt +1 -0

index ed116df..342c75a 100644
@@ -56,6 +56,7 @@ Required properties:
 	"allwinner,sun4i-a10-usb-clk" - for usb gates + resets on A10 / A20
 	"allwinner,sun5i-a13-usb-clk" - for usb gates + resets on A13
 	"allwinner,sun6i-a31-usb-clk" - for usb gates + resets on A31
+	"allwinner,sun6i-a31-ir-clk" - for the ir clock on A31
 
 Required properties for all clocks:
 - reg : shall be the control register address for the clock.

drivers/clk/sunxi/Makefile +1 -1

index 7ddc2b5..daf8b1c 100644
@@ -10,4 +10,4 @@ obj-y += clk-sun8i-mbus.o
 
 obj-$(CONFIG_MFD_SUN6I_PRCM) += \
 	clk-sun6i-ar100.o clk-sun6i-apb0.o clk-sun6i-apb0-gates.o \
-	clk-sun8i-apb0.o
+	clk-sun8i-apb0.o clk-sun6i-prcm-mod0.o

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c new file +53 -0

new file mode 100644
index 0000000..e80f18e
@@ -0,0 +1,63 @@
+/*
+ * Allwinner A31 PRCM mod0 clock driver
+ *
+ * Copyright (C) 2014 Hans de Goede <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+
+#include "clk-factors.h"
+#include "clk-mod0.h"
+
+static const struct of_device_id sun6i_a31_prcm_mod0_clk_dt_ids[] = {
+	{ .compatible = "allwinner,sun6i-a31-ir-clk" },
+	{ /* sentinel */ }
+};
+
+static DEFINE_SPINLOCK(sun6i_prcm_mod0_lock);
+
+static int sun6i_a31_prcm_mod0_clk_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct resource *r;
+	void __iomem *reg;
+
+	if (!np)
+		return -ENODEV;
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	reg = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(reg))
+		return PTR_ERR(reg);
+
+	sunxi_factors_register(np, &sun4i_a10_mod0_data,
+			       &sun6i_prcm_mod0_lock, reg);
+	return 0;
+}
+
+static struct platform_driver sun6i_a31_prcm_mod0_clk_driver = {
+	.driver = {

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c line 63

Quoted 10 lines

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c new file +10 -0

+		.name = "sun6i-a31-prcm-mod0-clk",
+		.of_match_table = sun6i_a31_prcm_mod0_clk_dt_ids,
+	},
+	.probe = sun6i_a31_prcm_mod0_clk_probe,
+};
+module_platform_driver(sun6i_a31_prcm_mod0_clk_driver);
+
+MODULE_DESCRIPTION("Allwinner A31 PRCM mod0 clock driver");
+MODULE_AUTHOR("Hans de Goede <[email protected]>");
+MODULE_LICENSE("GPL");

I don't think this is the right approach, mainly for two reasons: the compatible shouldn't change, and you're basically duplicating code there.

Quoted 3 lines

I understand that you need the new compatible in order to avoid a double probing: one by CLK_OF_DECLARE, and one by the usual mechanism, and that also implies the second reason.

Not only for that, we need a new compatible also because the mfd framework needs a separate compatible per sub-node as that is how it finds nodes to attach to the platform devices, so we need a new compatible anyways, with your make the mod0 clock driver a platform driver solution we could use:

Quoted 1 line

We have a single mod0 clock in there, so no, not really.

We have a single mod0 clock there today, but who knows what tomorrow brings, arguably the 1wire clock is a mod0 clock too, so we already have 2 today.

Quoted 2 lines

Plus, that seems like a bogus limitation from MFD, and it really shouldn't work that way.

Well AFAIK that is how it works.

Quoted 8 lines

Quoted 6 lines

	compatible = "allwinner,sun6i-a31-ir-clk", "allwinner,sun4i-a10-mod0-clk";

To work around this, but there are other problems, your make mod0clk a platform driver solution cannot work because the clocks node in the dtsi is not simple-bus compatible, so no platform-devs will be instantiated for the clocks there.

Then add the simple-bus compatible.

No, just no, that goes against the whole design of how of-clocks work.

Quoted 9 lines

Quoted 5 lines

Besides the compatible (which as said we need a separate one anyways), your other worry is code duplication, but I've avoided that as much as possible already, the new drivers/clk/sunxi/clk-sun6i-prcm-mod0.c is just a very thin wrapper, waying in with all of 63 lines including 16 lines GPL header.

47 lines that are doing exactly the same thing as the other code is doing. I'm sorry but you can't really argue it's not code duplication, it really is.

It is not doing the exact same thing, it is a platform driver, where as the other code is an of_clk driver.

Quoted 18 lines

Quoted 17 lines

So sorry, I disagree I believe that this is the best solution.

Quoted 8 lines

However, as those are not critical clocks that need to be here early at boot, you can also fix this by turning the mod0 driver into a platform driver itself. The compatible will be kept, the driver will be the same.

The only thing we need to pay attention to is how "client" drivers react when they cannot grab their clock. They should return -EPROBE_DEFER, but that remains to be checked.

-EPROBE_DEFER is yet another reason to not make mod0-clk a platform driver. Yes drivers should be able to handle this, but it is a pain, and the more we use it the more pain it is. Also it makes booting a lot slower as any driver using a mod0 clk now, will not complete its probe until all the other drivers are done probing and the late_initcall which starts the deferred-probe wq runs.

Quoted 2 lines

The whole EPROBE_DEFER mechanism might need some fixing, but it's not really the point is it?

Well one reasons why clocks are instantiated the way they are is to have them available as early as possible, which is really convenient and works really well.

You are asking for a whole lot of stuff to be changed, arguably in a way which makes it worse, just to save 47 lines of code...

Hi,

Quoted 128 lines · Hans de Goede

Hi,

Quoted 125 lines · Maxime Ripard

Quoted 123 lines · Hans de Goede

Hi,

Quoted 111 lines · Maxime Ripard

Hi,

Quoted 93 lines · Hans de Goede

Add a driver for mod0 clocks found in the prcm. Currently there is only one mod0 clocks in the prcm, the ir clock.

Signed-off-by Hans de Goede <[email protected]>

---
  Documentation/devicetree/bindings/clock/sunxi.txt |  1 +
  drivers/clk/sunxi/Makefile                        |  2 +-
  drivers/clk/sunxi/clk-sun6i-prcm-mod0.c           | 63 +++++++++++++++++++++++
  drivers/mfd/sun6i-prcm.c                          | 14 +++++
  4 files changed, 79 insertions(+), 1 deletion(-)

create mode 100644 drivers/clk/sunxi/clk-sun6i-prcm-mod0.c

Documentation/devicetree/bindings/clock/sunxi.txt +1 -0

index ed116df..342c75a 100644
@@ -56,6 +56,7 @@ Required properties:
  	"allwinner,sun4i-a10-usb-clk" - for usb gates + resets on A10 / A20
  	"allwinner,sun5i-a13-usb-clk" - for usb gates + resets on A13
  	"allwinner,sun6i-a31-usb-clk" - for usb gates + resets on A31
+	"allwinner,sun6i-a31-ir-clk" - for the ir clock on A31
 
  Required properties for all clocks:
  - reg : shall be the control register address for the clock.

drivers/clk/sunxi/Makefile +1 -1

index 7ddc2b5..daf8b1c 100644
@@ -10,4 +10,4 @@ obj-y += clk-sun8i-mbus.o
 
  obj-$(CONFIG_MFD_SUN6I_PRCM) += \
  	clk-sun6i-ar100.o clk-sun6i-apb0.o clk-sun6i-apb0-gates.o \
-	clk-sun8i-apb0.o
+	clk-sun8i-apb0.o clk-sun6i-prcm-mod0.o

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c new file +53 -0

new file mode 100644
index 0000000..e80f18e
@@ -0,0 +1,63 @@
+/*
+ * Allwinner A31 PRCM mod0 clock driver
+ *
+ * Copyright (C) 2014 Hans de Goede <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+
+#include "clk-factors.h"
+#include "clk-mod0.h"
+
+static const struct of_device_id sun6i_a31_prcm_mod0_clk_dt_ids[] = {
+	{ .compatible = "allwinner,sun6i-a31-ir-clk" },
+	{ /* sentinel */ }
+};
+
+static DEFINE_SPINLOCK(sun6i_prcm_mod0_lock);
+
+static int sun6i_a31_prcm_mod0_clk_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct resource *r;
+	void __iomem *reg;
+
+	if (!np)
+		return -ENODEV;
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	reg = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(reg))
+		return PTR_ERR(reg);
+
+	sunxi_factors_register(np, &sun4i_a10_mod0_data,
+			       &sun6i_prcm_mod0_lock, reg);
+	return 0;
+}
+
+static struct platform_driver sun6i_a31_prcm_mod0_clk_driver = {
+	.driver = {

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c line 63

Quoted 10 lines

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c new file +10 -0

+		.name = "sun6i-a31-prcm-mod0-clk",
+		.of_match_table = sun6i_a31_prcm_mod0_clk_dt_ids,
+	},
+	.probe = sun6i_a31_prcm_mod0_clk_probe,
+};
+module_platform_driver(sun6i_a31_prcm_mod0_clk_driver);
+
+MODULE_DESCRIPTION("Allwinner A31 PRCM mod0 clock driver");
+MODULE_AUTHOR("Hans de Goede <[email protected]>");
+MODULE_LICENSE("GPL");

I don't think this is the right approach, mainly for two reasons: the compatible shouldn't change, and you're basically duplicating code there.

Quoted 3 lines

I understand that you need the new compatible in order to avoid a double probing: one by CLK_OF_DECLARE, and one by the usual mechanism, and that also implies the second reason.

Not only for that, we need a new compatible also because the mfd framework needs a separate compatible per sub-node as that is how it finds nodes to attach to the platform devices, so we need a new compatible anyways, with your make the mod0 clock driver a platform driver solution we could use:

Quoted 4 lines

Quoted 1 line

We have a single mod0 clock in there, so no, not really.

We have a single mod0 clock there today, but who knows what tomorrow brings, arguably the 1wire clock is a mod0 clock too, so we already have 2 today.

I remember someone (Chen-Yu? Boris?) saying that the 1wire clock was not really a mod0 clk. From what I could gather from the source code, it seems to have a wider m divider, so we could argue that it should need a new compatible.

Quoted 3 lines

Quoted 2 lines

Plus, that seems like a bogus limitation from MFD, and it really shouldn't work that way.

Quoted 1 line

Well AFAIK that is how it works.

That kind of argument doesn't work. You could make exactly the same statement about the fact that two identical hardware blocks in the DT should have the same compatible, and it's how it works.

With the minor exception that we can't change that fact for the DT, while we can change the MTD code.

Quoted 1 line

Quoted 1 line

Quoted 1 line

	compatible = "allwinner,sun6i-a31-ir-clk", "allwinner,sun4i-a10-mod0-clk";

Quoted 9 lines

Quoted 7 lines

Quoted 5 lines

To work around this, but there are other problems, your make mod0clk a platform driver solution cannot work because the clocks node in the dtsi is not simple-bus compatible, so no platform-devs will be instantiated for the clocks there.

Then add the simple-bus compatible.

No, just no, that goes against the whole design of how of-clocks work.

Which is... ?

There's a number of other clocks being platform drivers, including on other platforms, and as far as I know, CLK_OF_DECLARE is only really useful on clocks that are critical. Mike, what's your view on this?

Quoted 34 lines

Quoted 9 lines

Quoted 5 lines

Besides the compatible (which as said we need a separate one anyways), your other worry is code duplication, but I've avoided that as much as possible already, the new drivers/clk/sunxi/clk-sun6i-prcm-mod0.c is just a very thin wrapper, waying in with all of 63 lines including 16 lines GPL header.

47 lines that are doing exactly the same thing as the other code is doing. I'm sorry but you can't really argue it's not code duplication, it really is.

It is not doing the exact same thing, it is a platform driver, where as the other code is an of_clk driver.

Quoted 20 lines

Quoted 11 lines

So sorry, I disagree I believe that this is the best solution.

Quoted 8 lines

However, as those are not critical clocks that need to be here early at boot, you can also fix this by turning the mod0 driver into a platform driver itself. The compatible will be kept, the driver will be the same.

The only thing we need to pay attention to is how "client" drivers react when they cannot grab their clock. They should return -EPROBE_DEFER, but that remains to be checked.

Quoted 6 lines

-EPROBE_DEFER is yet another reason to not make mod0-clk a platform driver. Yes drivers should be able to handle this, but it is a pain, and the more we use it the more pain it is. Also it makes booting a lot slower as any driver using a mod0 clk now, will not complete its probe until all the other drivers are done probing and the late_initcall which starts the deferred-probe wq runs.

The whole EPROBE_DEFER mechanism might need some fixing, but it's not really the point is it?

Quoted 3 lines

Well one reasons why clocks are instantiated the way they are is to have them available as early as possible, which is really convenient and works really well.

Yeah, you need them early for critical and early stuff like timers, console, etc. You really don't need this mechanism if you don't have to handle early probed devices.

Quoted 2 lines

You are asking for a whole lot of stuff to be changed, arguably in a way which makes it worse, just to save 47 lines of code...

No, really, I'm not. All I ask for is adding 1 line in the DT, and removing 5 lines in clk-sunxi.c. That's all.

Hi,

Quoted 171 lines · Maxime Ripard

Hi,

Quoted 147 lines · Hans de Goede

Hi,

Quoted 143 lines · Maxime Ripard

Quoted 139 lines · Hans de Goede

Hi,

Quoted 123 lines · Maxime Ripard

Hi,

Quoted 103 lines · Hans de Goede

Add a driver for mod0 clocks found in the prcm. Currently there is only
one mod0 clocks in the prcm, the ir clock.

Signed-off-by Hans de Goede <[email protected]>

---
  Documentation/devicetree/bindings/clock/sunxi.txt |  1 +
  drivers/clk/sunxi/Makefile                        |  2 +-
  drivers/clk/sunxi/clk-sun6i-prcm-mod0.c           | 63
+++++++++++++++++++++++
  drivers/mfd/sun6i-prcm.c                          | 14 +++++
  4 files changed, 79 insertions(+), 1 deletion(-)

create mode 100644 drivers/clk/sunxi/clk-sun6i-prcm-mod0.c

Documentation/devicetree/bindings/clock/sunxi.txt +0 -0

index ed116df..342c75a 100644
@@ -56,6 +56,7 @@ Required properties:
        "allwinner,sun4i-a10-usb-clk" - for usb gates + resets on A10

/ A20
"allwinner,sun5i-a13-usb-clk" - for usb gates + resets on A13 "allwinner,sun6i-a31-usb-clk" - for usb gates + resets on A31

Documentation/devicetree/bindings/clock/sunxi.txt +1 -0

+       "allwinner,sun6i-a31-ir-clk" - for the ir clock on A31
 
  Required properties for all clocks:
  - reg : shall be the control register address for the clock.

drivers/clk/sunxi/Makefile +1 -1

index 7ddc2b5..daf8b1c 100644
@@ -10,4 +10,4 @@ obj-y += clk-sun8i-mbus.o
 
  obj-$(CONFIG_MFD_SUN6I_PRCM) += \
        clk-sun6i-ar100.o clk-sun6i-apb0.o clk-sun6i-apb0-gates.o \
-       clk-sun8i-apb0.o
+       clk-sun8i-apb0.o clk-sun6i-prcm-mod0.o

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c new file +6 -0

new file mode 100644
index 0000000..e80f18e
@@ -0,0 +1,63 @@
+/*
+ * Allwinner A31 PRCM mod0 clock driver
+ *
+ * Copyright (C) 2014 Hans de Goede <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or

modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c new file +11 -0

+
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+
+#include "clk-factors.h"
+#include "clk-mod0.h"
+
+static const struct of_device_id sun6i_a31_prcm_mod0_clk_dt_ids[] =

{

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c new file +7 -0

+       { .compatible = "allwinner,sun6i-a31-ir-clk" },
+       { /* sentinel */ }
+};
+
+static DEFINE_SPINLOCK(sun6i_prcm_mod0_lock);
+
+static int sun6i_a31_prcm_mod0_clk_probe(struct platform_device

*pdev)

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c new file +20 -0

+{
+       struct device_node *np = pdev->dev.of_node;
+       struct resource *r;
+       void __iomem *reg;
+
+       if (!np)
+               return -ENODEV;
+
+       r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       reg = devm_ioremap_resource(&pdev->dev, r);
+       if (IS_ERR(reg))
+               return PTR_ERR(reg);
+
+       sunxi_factors_register(np, &sun4i_a10_mod0_data,
+                              &sun6i_prcm_mod0_lock, reg);
+       return 0;
+}
+
+static struct platform_driver sun6i_a31_prcm_mod0_clk_driver = {
+       .driver = {

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c

Quoted 10 lines

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c new file +10 -0

+               .name = "sun6i-a31-prcm-mod0-clk",
+               .of_match_table = sun6i_a31_prcm_mod0_clk_dt_ids,
+       },
+       .probe = sun6i_a31_prcm_mod0_clk_probe,
+};
+module_platform_driver(sun6i_a31_prcm_mod0_clk_driver);
+
+MODULE_DESCRIPTION("Allwinner A31 PRCM mod0 clock driver");
+MODULE_AUTHOR("Hans de Goede <[email protected]>");
+MODULE_LICENSE("GPL");

I don't think this is the right approach, mainly for two reasons: the compatible shouldn't change, and you're basically duplicating code there.

Quoted 3 lines

I understand that you need the new compatible in order to avoid a double probing: one by CLK_OF_DECLARE, and one by the usual mechanism, and that also implies the second reason.

Not only for that, we need a new compatible also because the mfd framework
needs a separate compatible per sub-node as that is how it finds nodes to attach to the platform devices, so we need a new compatible anyways, with your make the mod0 clock driver a platform driver solution we could
use:

Quoted 6 lines

Quoted 1 line

We have a single mod0 clock in there, so no, not really.

We have a single mod0 clock there today, but who knows what tomorrow brings,
arguably the 1wire clock is a mod0 clock too, so we already have 2 today.

I remember someone (Chen-Yu? Boris?) saying that the 1wire clock was not really a mod0 clk. From what I could gather from the source code, it seems to have a wider m divider, so we could argue that it should need a new compatible.

Quoted 5 lines

Quoted 2 lines

Plus, that seems like a bogus limitation from MFD, and it really shouldn't work that way.

Well AFAIK that is how it works.

Quoted 6 lines

That kind of argument doesn't work. You could make exactly the same statement about the fact that two identical hardware blocks in the DT should have the same compatible, and it's how it works.

With the minor exception that we can't change that fact for the DT, while we can change the MTD code.

As I already said, we could do something like this to make clear this is a mod0 clock:

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

        compatible = "allwinner,sun6i-a31-ir-clk",

"allwinner,sun4i-a10-mod0-clk";

The problem with that is it will cause early binding because of CLK_OF_DECLARE and early binding does not work on MFD instantiated devices, because they have no reg property.

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 7 lines

To work around this, but there are other problems, your make mod0clk a platform driver solution cannot work because the clocks node in the dtsi
is not simple-bus compatible, so no platform-devs will be instantiated for
the clocks there.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

Then add the simple-bus compatible.

No, just no, that goes against the whole design of how of-clocks work.

Which is... ?

That they are child nodes of a clocks node, which is NOT simple-bus compatible
and get instantiated early using CLK_OF_DECLARE.

Quoted 43 lines

There's a number of other clocks being platform drivers, including on other platforms, and as far as I know, CLK_OF_DECLARE is only really useful on clocks that are critical. Mike, what's your view on this?

Quoted 39 lines

Quoted 10 lines

Quoted 5 lines

Besides the compatible (which as said we need a separate one anyways), your other worry is code duplication, but I've avoided that as much as possible already, the new drivers/clk/sunxi/clk-sun6i-prcm-mod0.c is just a very thin wrapper, waying in with all of 63 lines including 16 lines GPL header.

47 lines that are doing exactly the same thing as the other code is doing. I'm sorry but you can't really argue it's not code duplication, it really is.

It is not doing the exact same thing, it is a platform driver, where as the other code is an of_clk driver.

Quoted 22 lines

Quoted 12 lines

So sorry, I disagree I believe that this is the best solution.

Quoted 8 lines

However, as those are not critical clocks that need to be here early at boot, you can also fix this by turning the mod0 driver into a platform driver itself. The compatible will be kept, the driver will be the same.

The only thing we need to pay attention to is how "client" drivers react when they cannot grab their clock. They should return -EPROBE_DEFER, but that remains to be checked.

Quoted 6 lines

-EPROBE_DEFER is yet another reason to not make mod0-clk a platform driver. Yes drivers should be able to handle this, but it is a pain, and the more we use it the more pain it is. Also it makes booting a lot slower as any driver using a mod0 clk now, will not complete its probe until all the other drivers are done probing and the late_initcall which starts the deferred-probe wq runs.

The whole EPROBE_DEFER mechanism might need some fixing, but it's not really the point is it?

Quoted 8 lines

Quoted 3 lines

Well one reasons why clocks are instantiated the way they are is to have them available as early as possible, which is really convenient and works really well.

Yeah, you need them early for critical and early stuff like timers, console, etc. You really don't need this mechanism if you don't have to handle early probed devices.

And we do need some clocks for that, if we add the simple bus compatible then
*all* of them become platform devices.

Or are you suggesting to still bind all the other clocks using CLK_OF_DECLARE
and only turn mod0 into a platform driver, that would be rather inconsistent.

Quoted 6 lines

Quoted 2 lines

You are asking for a whole lot of stuff to be changed, arguably in a way which makes it worse, just to save 47 lines of code...

No, really, I'm not. All I ask for is adding 1 line in the DT, and removing 5 lines in clk-sunxi.c. That's all.

And adding platform driver glue for the mod0 clks, which would pretty much introduce the same 47 lines in a different place.

I notice that you've not responded to my proposal to simple make the clock node a child node of the clocks node in the dt, that should work nicely, and avoid the need for any kernel level changes to support it, I'm beginning to think that that is probably the best solution.

Hi,

On Thu, Nov 27, 2014 at 4:41 PM, Hans de Goede <[email protected]> wrote:

Quoted 4 lines

I notice that you've not responded to my proposal to simple make the clock node a child node of the clocks node in the dt, that should work nicely, and avoid the need for any kernel level changes to support it, I'm beginning to think that that is probably the best solution.

Would that not cause an overlap of the io regions, and cause one of them to fail? AFAIK the OF subsystem doesn't like overlapping resources.

Hi,

Quoted 3 lines · Chen-Yu Tsai

Hi,

On Thu, Nov 27, 2014 at 4:41 PM, Hans de Goede <[email protected]> wrote:

<snip>

Quoted 5 lines

Quoted 4 lines

I notice that you've not responded to my proposal to simple make the clock node a child node of the clocks node in the dt, that should work nicely, and avoid the need for any kernel level changes to support it, I'm beginning to think that that is probably the best solution.

Quoted 2 lines

Would that not cause an overlap of the io regions, and cause one of them to fail? AFAIK the OF subsystem doesn't like overlapping resources.

No the overlap check is done by the platform dev resource code, and of_clk_declare does not use that. So the overlap would be there, but not an issue (in theory I did not test this).

Thinking more about this, I believe that using the MFD framework for the prcm seems like a mistake to me. It gains us nothing, since we have no irq to de-multiplex or some such. We're not using MFD for the CMU, why use it for CMU2 (which the prcm effectively is) ?

So I think it would be best to remove the prcm node from the dt, and simply put the different blocks inside it directly under the soc node, this will work fine with current kernels, since as said we do not use any MFD features, we use it to create platform devs and assign resources, something which will happen automatically if we put the nodes directly under the soc node, since then simple-bus will do the work for us.

And then in a release or 2 we can remove the mfd prcm driver from the kernel, and we have the same functionality we have now with less code.

We could then also chose to move the existing prcm clock nodes to of_clk_declare (this will work once they are nodes directly under soc with a proper reg property). and the ir-clk can use allwinner,sun4i-a10-mod0-clk compatible and can live under either clocks or soc, depending on what we want to do with the other prcm clocks.

Hi,

Quoted 14 lines · Hans de Goede

Hi,

Quoted 3 lines · Chen-Yu Tsai

Hi,

On Thu, Nov 27, 2014 at 4:41 PM, Hans de Goede <[email protected]> wrote:

<snip>

Quoted 5 lines

Quoted 4 lines

I notice that you've not responded to my proposal to simple make the clock node a child node of the clocks node in the dt, that should work nicely, and avoid the need for any kernel level changes to support it, I'm beginning to think that that is probably the best solution.

Quoted 6 lines

Quoted 2 lines

Would that not cause an overlap of the io regions, and cause one of them to fail? AFAIK the OF subsystem doesn't like overlapping resources.

No the overlap check is done by the platform dev resource code, and of_clk_declare does not use that. So the overlap would be there, but not an issue (in theory I did not test this).

An overlap is always an issue.

Quoted 4 lines

Thinking more about this, I believe that using the MFD framework for the prcm seems like a mistake to me. It gains us nothing, since we have no irq to de-multiplex or some such. We're not using MFD for the CMU, why use it for CMU2 (which the prcm effectively is) ?

Because the PRCM is much more than that. It also handles the power domains for example.

Hi,

Quoted 31 lines · Maxime Ripard

Hi,

Quoted 14 lines · Hans de Goede

Hi,

Quoted 3 lines · Chen-Yu Tsai

Hi,

On Thu, Nov 27, 2014 at 4:41 PM, Hans de Goede <[email protected]> wrote:

<snip>

Quoted 5 lines

Quoted 4 lines

I notice that you've not responded to my proposal to simple make the clock node a child node of the clocks node in the dt, that should work nicely, and avoid the need for any kernel level changes to support it, I'm beginning to think that that is probably the best solution.

Quoted 6 lines

Quoted 2 lines

Would that not cause an overlap of the io regions, and cause one of them to fail? AFAIK the OF subsystem doesn't like overlapping resources.

No the overlap check is done by the platform dev resource code, and of_clk_declare does not use that. So the overlap would be there, but not an issue (in theory I did not test this).

An overlap is always an issue.

Quoted 4 lines

Thinking more about this, I believe that using the MFD framework for the prcm seems like a mistake to me. It gains us nothing, since we have no irq to de-multiplex or some such. We're not using MFD for the CMU, why use it for CMU2 (which the prcm effectively is) ?

Quoted 2 lines

Because the PRCM is much more than that. It also handles the power domains for example.

Ok, so thinking more about this, I'm still convinced that the MFD framework is only getting in the way here.

But I can see having things represented in devicetree properly, with the clocks, etc. as child nodes of the prcm being something which we want.

So since all we are using the MFD for is to instantiate platform devices under the prcm nodes, and assign an io resource for the regs to them, why not simply make the prcm node itself a simple-bus.

This does everything the MFD prcm driver currently does, without actually needing a specific kernel driver, and as added bonus this will move the definition of the mfd function reg offsets out of the kernel and into the devicetree where they belong in the first place.

Please see the attached patches, I've tested this on sun6i, if we go this route we should make the same change on sun8i (I can make the change, but not test it).

Regards,

Hans

From 6756574293a1f291a8dcc29427b27f32f83acb2d Mon Sep 17 00:00:00 2001
From: Hans de Goede <[email protected]>
Date: Fri, 28 Nov 2014 13:48:58 +0100
Subject: [PATCH v2 1/2] ARM: dts: sun6i: Change prcm node into a simple-bus

The prcm node's purpose is to group the various prcm sub-devices together, it does not need any special handling beyond that, there is no need to handle shared resources like a shared (multiplexed) interrupt or a shared i2c bus.

As such there really is no need to have a separate compatible for it, using simple-bus for it works fine. This also allows us to specify the register offsets of the various child-devices directly into the dts, rather then having to specify them in the OS implementation, putting the register offsets where the belong.

Signed-off-by Hans de Goede <[email protected]>

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

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

index 29e6438..4b8541f 100644

Quoted 2 lines

Ok, so thinking more about this, I'm still convinced that the MFD framework is only getting in the way here.

You still haven't said of what exactly it's getting in the way of.

Quoted 3 lines

But I can see having things represented in devicetree properly, with the clocks, etc. as child nodes of the prcm being something which we want.

Clocks and reset are the only thing set so far, because we need reference to them from the DT itself, nothing more.

We could very much have more devices instatiated from the MFD itself.

Quoted 3 lines

So since all we are using the MFD for is to instantiate platform devices under the prcm nodes, and assign an io resource for the regs to them, why not simply make the prcm node itself a simple-bus.

No, this is really not a bus. It shouldn't be described at all as such. It is a device, that has multiple functionnalities in the system => MFD. It really is that simple.

Quoted 4 lines

This does everything the MFD prcm driver currently does, without actually needing a specific kernel driver, and as added bonus this will move the definition of the mfd function reg offsets out of the kernel and into the devicetree where they belong in the first place.

Which was nacked in the first place because such offsets are not supposed to be in the DT.

Really, we have something that work here, there's no need to refactor it.

Hi,

On 12/02/2014 04:45 PM, Maxime Ripard wrote:

Quoted 4 lines

Quoted 2 lines

Ok, so thinking more about this, I'm still convinced that the MFD framework is only getting in the way here.

You still haven't said of what exactly it's getting in the way of.

Of using of_clk_define to bind to the mod0 clk in the prcm, because the ir_clk node does not have its own reg property when the mfd framework is used and of_clk_define requires the node to have its own reg property.

Quoted 14 lines

Quoted 4 lines

But I can see having things represented in devicetree properly, with the clocks, etc. as child nodes of the prcm being something which we want.

Clocks and reset are the only thing set so far, because we need reference to them from the DT itself, nothing more.

We could very much have more devices instatiated from the MFD itself.

Quoted 3 lines

So since all we are using the MFD for is to instantiate platform devices under the prcm nodes, and assign an io resource for the regs to them, why not simply make the prcm node itself a simple-bus.

Quoted 3 lines

No, this is really not a bus. It shouldn't be described at all as such. It is a device, that has multiple functionnalities in the system => MFD. It really is that simple.

Ok, I can live with that, but likewise the clocks node is not a bus either!

So it should not have a simple-bus compatible either, and as such we cannot simply change the mod0 driver from of_clk_define to a platform driver because then we need to instantiate platform devs for the mod0 clock nodes, which means making the clock node a simple-bus.

I can see your logic in wanting the ir_clk prcm sub-node to use the mod0 compatible string, so how about we make the mod0 driver both register through of_declare and as a platform driver. Note this means that it will try to bind twice to the ir_clk node, since of_clk_declare will cause it to try and bind there too AFAIK.

The of_clk_declare bind will fail though because there is no regs property, so this double bind is not an issue as long as we do not log errors on the first bind failure.

Note that the ir_clk node will still need an "ir-clk" compatible as well for the MFD to find it and assign the proper resources to it.

But this way we will have the clk driver binding to the mod0 clk compatible, which is what you want, while having the MFD assign resources on the fact that it is the ir-clk node, so that things will still work if there are multiple mod0 clks in the prcm.

On Wed, Dec 03, 2014 at 10:49:20AM +0100, Hans de Goede wrote:

Quoted 4 lines

So it should not have a simple-bus compatible either, and as such we cannot simply change the mod0 driver from of_clk_define to a platform driver because then we need to instantiate platform devs for the mod0 clock nodes, which means making the clock node a simple-bus.

I guess we can do that as a temporary measure until we get things right on that front. I'm totally open to doing that work, so I'm not asking you to do it.

Quoted 5 lines

I can see your logic in wanting the ir_clk prcm sub-node to use the mod0 compatible string, so how about we make the mod0 driver both register through of_declare and as a platform driver. Note this means that it will try to bind twice to the ir_clk node, since of_clk_declare will cause it to try and bind there too AFAIK.

Hmmm, I could live with that for a while too. That shouldn't even require too much work, since the first thing we check in the mod0 code is that we actually have something in reg, which will not be the case in the OF_CLK_DECLARE case.

Quoted 3 lines

The of_clk_declare bind will fail though because there is no regs property, so this double bind is not an issue as long as we do not log errors on the first bind failure.

Yep, exactly.

Quoted 2 lines

Note that the ir_clk node will still need an "ir-clk" compatible as well for the MFD to find it and assign the proper resources to it.

No, it really doesn't. At least for now, we have a single mod0 clock under the PRCM MFD. If (and only if) one day, we find ourselves in a position where we have two mod0 clocks under the PRCM, then we'll fix the MFD code to deal with that, because it really should deal with it.

Hi,

Quoted 1 line · Maxime Ripard

On Wed, Dec 03, 2014 at 10:49:20AM +0100, Hans de Goede wrote:

<snip>

Quoted 26 lines

Quoted 4 lines

So it should not have a simple-bus compatible either, and as such we cannot simply change the mod0 driver from of_clk_define to a platform driver because then we need to instantiate platform devs for the mod0 clock nodes, which means making the clock node a simple-bus.

I guess we can do that as a temporary measure until we get things right on that front. I'm totally open to doing that work, so I'm not asking you to do it.

Quoted 5 lines

I can see your logic in wanting the ir_clk prcm sub-node to use the mod0 compatible string, so how about we make the mod0 driver both register through of_declare and as a platform driver. Note this means that it will try to bind twice to the ir_clk node, since of_clk_declare will cause it to try and bind there too AFAIK.

Hmmm, I could live with that for a while too. That shouldn't even require too much work, since the first thing we check in the mod0 code is that we actually have something in reg, which will not be the case in the OF_CLK_DECLARE case.

Quoted 3 lines

The of_clk_declare bind will fail though because there is no regs property, so this double bind is not an issue as long as we do not log errors on the first bind failure.

Yep, exactly.

Quoted 7 lines

Quoted 2 lines

Note that the ir_clk node will still need an "ir-clk" compatible as well for the MFD to find it and assign the proper resources to it.

No, it really doesn't. At least for now, we have a single mod0 clock under the PRCM MFD. If (and only if) one day, we find ourselves in a position where we have two mod0 clocks under the PRCM, then we'll fix the MFD code to deal with that, because it really should deal with it.

Ok, using only the mod0 compat string works for me. I'll respin my patch-set (minus the one patch you've already merged) to make the modo clk driver use both of_clk_declare and make it a platfrom driver, and use the mod0 compat string for the ir-clk node.

Not sure when I'll get this done exactly though, but we still have a while before 3.20 :)

Hi,

Quoted 144 lines · Maxime Ripard

Hi,

Quoted 128 lines · Hans de Goede

Hi,

Quoted 125 lines · Maxime Ripard

Quoted 123 lines · Hans de Goede

Hi,

Quoted 111 lines · Maxime Ripard

Hi,

Quoted 93 lines · Hans de Goede

Add a driver for mod0 clocks found in the prcm. Currently there is only one mod0 clocks in the prcm, the ir clock.

Signed-off-by Hans de Goede <[email protected]>

---
 Documentation/devicetree/bindings/clock/sunxi.txt |  1 +
 drivers/clk/sunxi/Makefile                        |  2 +-
 drivers/clk/sunxi/clk-sun6i-prcm-mod0.c           | 63 +++++++++++++++++++++++
 drivers/mfd/sun6i-prcm.c                          | 14 +++++
 4 files changed, 79 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/sunxi/clk-sun6i-prcm-mod0.c

Documentation/devicetree/bindings/clock/sunxi.txt +1 -0

index ed116df..342c75a 100644
@@ -56,6 +56,7 @@ Required properties:
 	"allwinner,sun4i-a10-usb-clk" - for usb gates + resets on A10 / A20
 	"allwinner,sun5i-a13-usb-clk" - for usb gates + resets on A13
 	"allwinner,sun6i-a31-usb-clk" - for usb gates + resets on A31
+	"allwinner,sun6i-a31-ir-clk" - for the ir clock on A31
 
 Required properties for all clocks:
 - reg : shall be the control register address for the clock.

drivers/clk/sunxi/Makefile +1 -1

index 7ddc2b5..daf8b1c 100644
@@ -10,4 +10,4 @@ obj-y += clk-sun8i-mbus.o
 
 obj-$(CONFIG_MFD_SUN6I_PRCM) += \
 	clk-sun6i-ar100.o clk-sun6i-apb0.o clk-sun6i-apb0-gates.o \
-	clk-sun8i-apb0.o
+	clk-sun8i-apb0.o clk-sun6i-prcm-mod0.o

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c new file +53 -0

new file mode 100644
index 0000000..e80f18e
@@ -0,0 +1,63 @@
+/*
+ * Allwinner A31 PRCM mod0 clock driver
+ *
+ * Copyright (C) 2014 Hans de Goede <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+
+#include "clk-factors.h"
+#include "clk-mod0.h"
+
+static const struct of_device_id sun6i_a31_prcm_mod0_clk_dt_ids[] = {
+	{ .compatible = "allwinner,sun6i-a31-ir-clk" },
+	{ /* sentinel */ }
+};
+
+static DEFINE_SPINLOCK(sun6i_prcm_mod0_lock);
+
+static int sun6i_a31_prcm_mod0_clk_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct resource *r;
+	void __iomem *reg;
+
+	if (!np)
+		return -ENODEV;
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	reg = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(reg))
+		return PTR_ERR(reg);
+
+	sunxi_factors_register(np, &sun4i_a10_mod0_data,
+			       &sun6i_prcm_mod0_lock, reg);
+	return 0;
+}
+
+static struct platform_driver sun6i_a31_prcm_mod0_clk_driver = {
+	.driver = {

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c line 63

Quoted 10 lines

drivers/clk/sunxi/clk-sun6i-prcm-mod0.c new file +10 -0

+		.name = "sun6i-a31-prcm-mod0-clk",
+		.of_match_table = sun6i_a31_prcm_mod0_clk_dt_ids,
+	},
+	.probe = sun6i_a31_prcm_mod0_clk_probe,
+};
+module_platform_driver(sun6i_a31_prcm_mod0_clk_driver);
+
+MODULE_DESCRIPTION("Allwinner A31 PRCM mod0 clock driver");
+MODULE_AUTHOR("Hans de Goede <[email protected]>");
+MODULE_LICENSE("GPL");

I don't think this is the right approach, mainly for two reasons: the compatible shouldn't change, and you're basically duplicating code there.

Quoted 3 lines

I understand that you need the new compatible in order to avoid a double probing: one by CLK_OF_DECLARE, and one by the usual mechanism, and that also implies the second reason.

Not only for that, we need a new compatible also because the mfd framework needs a separate compatible per sub-node as that is how it finds nodes to attach to the platform devices, so we need a new compatible anyways, with your make the mod0 clock driver a platform driver solution we could use:

Quoted 4 lines

Quoted 1 line

We have a single mod0 clock in there, so no, not really.

We have a single mod0 clock there today, but who knows what tomorrow brings, arguably the 1wire clock is a mod0 clock too, so we already have 2 today.

I remember someone (Chen-Yu? Boris?) saying that the 1wire clock was not really a mod0 clk. From what I could gather from the source code, it seems to have a wider m divider, so we could argue that it should need a new compatible.

Quoted 3 lines

Quoted 2 lines

Plus, that seems like a bogus limitation from MFD, and it really shouldn't work that way.

Quoted 8 lines

Quoted 1 line

Well AFAIK that is how it works.

That kind of argument doesn't work. You could make exactly the same statement about the fact that two identical hardware blocks in the DT should have the same compatible, and it's how it works.

With the minor exception that we can't change that fact for the DT, while we can change the MTD code.

As I already said, we could do something like this to make clear this is a mod0 clock:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

	compatible = "allwinner,sun6i-a31-ir-clk", "allwinner,sun4i-a10-mod0-clk";

The problem with that is it will cause early binding because of CLK_OF_DECLARE and early binding does not work on MFD instantiated devices, because they have no reg property.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

To work around this, but there are other problems, your make mod0clk a platform driver solution cannot work because the clocks node in the dtsi is not simple-bus compatible, so no platform-devs will be instantiated for the clocks there.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Then add the simple-bus compatible.

No, just no, that goes against the whole design of how of-clocks work.

Which is... ?

That they are child nodes of a clocks node, which is NOT simple-bus compatible and get instantiated early using CLK_OF_DECLARE.

Quoted 38 lines

There's a number of other clocks being platform drivers, including on other platforms, and as far as I know, CLK_OF_DECLARE is only really useful on clocks that are critical. Mike, what's your view on this?

Quoted 34 lines

Quoted 9 lines

Quoted 5 lines

Besides the compatible (which as said we need a separate one anyways), your other worry is code duplication, but I've avoided that as much as possible already, the new drivers/clk/sunxi/clk-sun6i-prcm-mod0.c is just a very thin wrapper, waying in with all of 63 lines including 16 lines GPL header.

47 lines that are doing exactly the same thing as the other code is doing. I'm sorry but you can't really argue it's not code duplication, it really is.

It is not doing the exact same thing, it is a platform driver, where as the other code is an of_clk driver.

Quoted 20 lines

Quoted 11 lines

So sorry, I disagree I believe that this is the best solution.

Quoted 8 lines

However, as those are not critical clocks that need to be here early at boot, you can also fix this by turning the mod0 driver into a platform driver itself. The compatible will be kept, the driver will be the same.

The only thing we need to pay attention to is how "client" drivers react when they cannot grab their clock. They should return -EPROBE_DEFER, but that remains to be checked.

Quoted 6 lines

-EPROBE_DEFER is yet another reason to not make mod0-clk a platform driver. Yes drivers should be able to handle this, but it is a pain, and the more we use it the more pain it is. Also it makes booting a lot slower as any driver using a mod0 clk now, will not complete its probe until all the other drivers are done probing and the late_initcall which starts the deferred-probe wq runs.

The whole EPROBE_DEFER mechanism might need some fixing, but it's not really the point is it?

Quoted 7 lines

Quoted 3 lines

Well one reasons why clocks are instantiated the way they are is to have them available as early as possible, which is really convenient and works really well.

Yeah, you need them early for critical and early stuff like timers, console, etc. You really don't need this mechanism if you don't have to handle early probed devices.

And we do need some clocks for that, if we add the simple bus compatible then *all* of them become platform devices.

Or are you suggesting to still bind all the other clocks using CLK_OF_DECLARE and only turn mod0 into a platform driver, that would be rather inconsistent.

Quoted 5 lines

Quoted 2 lines

You are asking for a whole lot of stuff to be changed, arguably in a way which makes it worse, just to save 47 lines of code...

No, really, I'm not. All I ask for is adding 1 line in the DT, and removing 5 lines in clk-sunxi.c. That's all.

And adding platform driver glue for the mod0 clks, which would pretty much introduce the same 47 lines in a different place.

I notice that you've not responded to my proposal to simple make the clock node a child node of the clocks node in the dt, that should work nicely, and avoid the need for any kernel level changes to support it, I'm beginning to think that that is probably the best solution.

I remember someone (Chen-Yu? Boris?) saying that the 1wire clock was not really a mod0 clk. From what I could gather from the source code, it seems to have a wider m divider, so we could argue that it should need a new compatible.

Hi,

On Wed, 26 Nov 2014 22:13:18 +0100
Maxime Ripard <[email protected]> wrote:

[...]

Quoted 5 lines

I remember someone (Chen-Yu? Boris?) saying that the 1wire clock was not really a mod0 clk. From what I could gather from the source code, it seems to have a wider m divider, so we could argue that it should need a new compatible.

Wasn't me :-).

Regarding the rest of the discussion I miss some context, but here's what I remember decided us to choose the MFD approach for the PRCM block:

1) it's embedding several unrelated functional blocks (reset, clk, and some other things I don't remember).
2) none of the functionalities provided by the PRCM were required in the early boot stage
3) We wanted to represent the HW blocks as they are really described in the memory mapping instead of splitting small register chunks over the DT.

Can someone sum-up the current issue you're trying to solve ?

IMHO, if you really want to split those functionalities over the DT (some nodes under clks and other under reset controller), then I suggest to use..............
(Maxime, please stop smiling :P)
..............

SYSCON

On sun6i the cir block is attached to the reset controller, add support for de-asserting the reset if a reset controller is specified in dt.

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/media/rc/sunxi-cir.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

drivers/media/rc/sunxi-cir.c +23 -2

index bcee8e1..895fb65 100644
@@ -23,6 +23,7 @@
 #include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/of_platform.h>
+#include <linux/reset.h>
 #include <media/rc-core.h>
 
 #define SUNXI_IR_DEV "sunxi-ir"

@@ -95,6 +96,7 @@ struct sunxi_ir {
 	int             irq;
 	struct clk      *clk;
 	struct clk      *apb_clk;
+	struct reset_control *rst;
 	const char      *map_name;
 };
 

@@ -166,15 +168,29 @@ static int sunxi_ir_probe(struct platform_device *pdev)
 		return PTR_ERR(ir->clk);
 	}
 
+	/* Reset (optional) */
+	ir->rst = devm_reset_control_get_optional(dev, NULL);
+	if (IS_ERR(ir->rst)) {
+		ret = PTR_ERR(ir->rst);
+		if (ret == -EPROBE_DEFER)
+			return ret;
+		ir->rst = NULL;
+	} else {
+		ret = reset_control_deassert(ir->rst);
+		if (ret)
+			return ret;
+	}
+
 	ret = clk_set_rate(ir->clk, SUNXI_IR_BASE_CLK);
 	if (ret) {
 		dev_err(dev, "set ir base clock failed!\n");
-		return ret;
+		goto exit_reset_assert;
 	}
 
 	if (clk_prepare_enable(ir->apb_clk)) {
 		dev_err(dev, "try to enable apb_ir_clk failed\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto exit_reset_assert;
 	}
 
 	if (clk_prepare_enable(ir->clk)) {

@@ -271,6 +287,9 @@ exit_clkdisable_clk:
 	clk_disable_unprepare(ir->clk);
 exit_clkdisable_apb_clk:
 	clk_disable_unprepare(ir->apb_clk);
+exit_reset_assert:
+	if (ir->rst)
+		reset_control_assert(ir->rst);
 
 	return ret;
 }

@@ -282,6 +301,8 @@ static int sunxi_ir_remove(struct platform_device *pdev)
 
 	clk_disable_unprepare(ir->clk);
 	clk_disable_unprepare(ir->apb_clk);
+	if (ir->rst)
+		reset_control_assert(ir->rst);
 
 	spin_lock_irqsave(&ir->ir_lock, flags);
 	/* disable IR IRQ */

Em Thu, 20 Nov 2014 16:55:23 +0100
Hans de Goede <[email protected]> escreveu:

Quoted 4 lines

On sun6i the cir block is attached to the reset controller, add support for de-asserting the reset if a reset controller is specified in dt.

Signed-off-by Hans de Goede <[email protected]>

As this is meant to be merged via some other tree:

Acked-by Mauro Carvalho Chehab <[email protected]>

On sun6i the cir block is attached to the reset controller, add support for de-asserting the reset if a reset controller is specified in dt.

Signed-off-by Hans de Goede <[email protected]>

---
 drivers/media/rc/sunxi-cir.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

drivers/media/rc/sunxi-cir.c +23 -3

index bcee8e1..895fb65 100644
@@ -23,6 +23,7 @@
 #include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/of_platform.h>
+#include <linux/reset.h>
 #include <media/rc-core.h>
 
 #define SUNXI_IR_DEV "sunxi-ir"

@@ -95,6 +96,7 @@ struct sunxi_ir {
        int             irq;
        struct clk      *clk;
        struct clk      *apb_clk;
+       struct reset_control *rst;
        const char      *map_name;
 };
 

@@ -166,15 +168,29 @@ static int sunxi_ir_probe(struct platform_device *pdev)
                return PTR_ERR(ir->clk);
        }
 
+       /* Reset (optional) */
+       ir->rst = devm_reset_control_get_optional(dev, NULL);
+       if (IS_ERR(ir->rst)) {
+               ret = PTR_ERR(ir->rst);
+               if (ret == -EPROBE_DEFER)
+                       return ret;
+               ir->rst = NULL;
+       } else {
+               ret = reset_control_deassert(ir->rst);
+               if (ret)
+                       return ret;
+       }
+
        ret = clk_set_rate(ir->clk, SUNXI_IR_BASE_CLK);
        if (ret) {
                dev_err(dev, "set ir base clock failed!\n");
-               return ret;
+               goto exit_reset_assert;
        }
 
        if (clk_prepare_enable(ir->apb_clk)) {
                dev_err(dev, "try to enable apb_ir_clk failed\n");
-               return -EINVAL;
+               ret = -EINVAL;
+               goto exit_reset_assert;
        }
 
        if (clk_prepare_enable(ir->clk)) {

@@ -271,6 +287,9 @@ exit_clkdisable_clk:
        clk_disable_unprepare(ir->clk);
 exit_clkdisable_apb_clk:
        clk_disable_unprepare(ir->apb_clk);
+exit_reset_assert:
+       if (ir->rst)
+               reset_control_assert(ir->rst);
 
        return ret;
 }

@@ -282,6 +301,8 @@ static int sunxi_ir_remove(struct platform_device *pdev)
 
        clk_disable_unprepare(ir->clk);
        clk_disable_unprepare(ir->apb_clk);
+       if (ir->rst)
+               reset_control_assert(ir->rst);
 
        spin_lock_irqsave(&ir->ir_lock, flags);
        /* disable IR IRQ */
--

2.1.0

--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe at googlegroups.com. For more options, visit https://groups.google.com/d/optout.

Add support for the larger fifo found on sun5i and sun6i, having a separate compatible for the ir found on sun5i & sun6i also is useful if we ever want to add ir transmit support, because the sun5i & sun6i version do not have transmit support.

Note this commits also adds checking for the end-of-packet interrupt flag (which was already enabled), as the fifo-data-available interrupt flag only gets set when the trigger-level is exceeded. So far we've been getting away with not doing this because of the low trigger-level, but this is something which we should have done since day one.

Signed-off-by Hans de Goede <[email protected]>

---
 .../devicetree/bindings/media/sunxi-ir.txt          |  2 +-
 drivers/media/rc/sunxi-cir.c                        | 21 ++++++++++++---------
 2 files changed, 13 insertions(+), 10 deletions(-)

Documentation/devicetree/bindings/media/sunxi-ir.txt +1 -1

index 23dd5ad..5767128 100644
@@ -1,7 +1,7 @@
 Device-Tree bindings for SUNXI IR controller found in sunXi SoC family
 
 Required properties:
-- compatible	    : should be "allwinner,sun4i-a10-ir";
+- compatible	    : "allwinner,sun4i-a10-ir" or "allwinner,sun5i-a13-ir"
 - clocks	    : list of clock specifiers, corresponding to
 		      entries in clock-names property;
 - clock-names	    : should contain "apb" and "ir" entries;

drivers/media/rc/sunxi-cir.c +12 -9

index 895fb65..559b0e3 100644
@@ -56,12 +56,12 @@
 #define REG_RXINT_RAI_EN		BIT(4)
 
 /* Rx FIFO available byte level */
-#define REG_RXINT_RAL(val)    (((val) << 8) & (GENMASK(11, 8)))
+#define REG_RXINT_RAL(val)    ((val) << 8)
 
 /* Rx Interrupt Status */
 #define SUNXI_IR_RXSTA_REG    0x30
 /* RX FIFO Get Available Counter */
-#define REG_RXSTA_GET_AC(val) (((val) >> 8) & (GENMASK(5, 0)))
+#define REG_RXSTA_GET_AC(val) (((val) >> 8) & (ir->fifo_size * 2 - 1))
 /* Clear all interrupt status value */
 #define REG_RXSTA_CLEARALL    0xff
 

@@ -72,10 +72,6 @@
 /* CIR_REG register idle threshold */
 #define REG_CIR_ITHR(val)    (((val) << 8) & (GENMASK(15, 8)))
 
-/* Hardware supported fifo size */
-#define SUNXI_IR_FIFO_SIZE    16
-/* How many messages in FIFO trigger IRQ */
-#define TRIGGER_LEVEL         8
 /* Required frequency for IR0 or IR1 clock in CIR mode */
 #define SUNXI_IR_BASE_CLK     8000000
 /* Frequency after IR internal divider  */

@@ -94,6 +90,7 @@ struct sunxi_ir {
 	struct rc_dev   *rc;
 	void __iomem    *base;
 	int             irq;
+	int		fifo_size;
 	struct clk      *clk;
 	struct clk      *apb_clk;
 	struct reset_control *rst;

@@ -115,11 +112,11 @@ static irqreturn_t sunxi_ir_irq(int irqno, void *dev_id)
 	/* clean all pending statuses */
 	writel(status | REG_RXSTA_CLEARALL, ir->base + SUNXI_IR_RXSTA_REG);
 
-	if (status & REG_RXINT_RAI_EN) {
+	if (status & (REG_RXINT_RAI_EN | REG_RXINT_RPEI_EN)) {
 		/* How many messages in fifo */
 		rc  = REG_RXSTA_GET_AC(status);
 		/* Sanity check */
-		rc = rc > SUNXI_IR_FIFO_SIZE ? SUNXI_IR_FIFO_SIZE : rc;
+		rc = rc > ir->fifo_size ? ir->fifo_size : rc;
 		/* If we have data */
 		for (cnt = 0; cnt < rc; cnt++) {
 			/* for each bit in fifo */

@@ -156,6 +153,11 @@ static int sunxi_ir_probe(struct platform_device *pdev)
 	if (!ir)
 		return -ENOMEM;
 
+	if (of_device_is_compatible(dn, "allwinner,sun5i-a13-ir"))
+		ir->fifo_size = 64;
+	else
+		ir->fifo_size = 16;
+
 	/* Clock */
 	ir->apb_clk = devm_clk_get(dev, "apb");
 	if (IS_ERR(ir->apb_clk)) {

@@ -271,7 +273,7 @@ static int sunxi_ir_probe(struct platform_device *pdev)
 	 * level
 	 */
 	writel(REG_RXINT_ROI_EN | REG_RXINT_RPEI_EN |
-	       REG_RXINT_RAI_EN | REG_RXINT_RAL(TRIGGER_LEVEL - 1),
+	       REG_RXINT_RAI_EN | REG_RXINT_RAL(ir->fifo_size / 2 - 1),
 	       ir->base + SUNXI_IR_RXINT_REG);
 
 	/* Enable IR Module */

@@ -319,6 +321,7 @@ static int sunxi_ir_remove(struct platform_device *pdev)
 
 static const struct of_device_id sunxi_ir_match[] = {
 	{ .compatible = "allwinner,sun4i-a10-ir", },
+	{ .compatible = "allwinner,sun5i-a13-ir", },
 	{},
 };
 

Em Thu, 20 Nov 2014 16:55:24 +0100
Hans de Goede <[email protected]> escreveu:

Quoted 11 lines

Add support for the larger fifo found on sun5i and sun6i, having a separate compatible for the ir found on sun5i & sun6i also is useful if we ever want to add ir transmit support, because the sun5i & sun6i version do not have transmit support.

Note this commits also adds checking for the end-of-packet interrupt flag (which was already enabled), as the fifo-data-available interrupt flag only gets set when the trigger-level is exceeded. So far we've been getting away with not doing this because of the low trigger-level, but this is something which we should have done since day one.

Quoted 1 line

Signed-off-by Hans de Goede <[email protected]>

As this is meant to be merged via some other tree:

Acked-by Mauro Carvalho Chehab <[email protected]>

Hi Mauro,

Quoted 16 lines · Mauro Carvalho Chehab

Em Thu, 20 Nov 2014 16:55:24 +0100
Hans de Goede <[email protected]> escreveu:

Quoted 12 lines

Add support for the larger fifo found on sun5i and sun6i, having a separate compatible for the ir found on sun5i & sun6i also is useful if we ever want to add ir transmit support, because the sun5i & sun6i version do not have transmit support.

Note this commits also adds checking for the end-of-packet interrupt flag (which was already enabled), as the fifo-data-available interrupt flag only gets set when the trigger-level is exceeded. So far we've been getting away with not doing this because of the low trigger-level, but this is something which we should have done since day one.

Signed-off-by Hans de Goede <[email protected]>

Quoted 3 lines

As this is meant to be merged via some other tree:

Acked-by Mauro Carvalho Chehab <[email protected]>

I think merging it through your tree would be just fine.

Acked-by Maxime Ripard <[email protected]>

Hi,

Quoted 19 lines · Maxime Ripard

Hi Mauro,

Quoted 16 lines · Mauro Carvalho Chehab

Em Thu, 20 Nov 2014 16:55:24 +0100
Hans de Goede <[email protected]> escreveu:

Quoted 12 lines

Add support for the larger fifo found on sun5i and sun6i, having a separate compatible for the ir found on sun5i & sun6i also is useful if we ever want to add ir transmit support, because the sun5i & sun6i version do not have transmit support.

Note this commits also adds checking for the end-of-packet interrupt flag (which was already enabled), as the fifo-data-available interrupt flag only gets set when the trigger-level is exceeded. So far we've been getting away with not doing this because of the low trigger-level, but this is something which we should have done since day one.

Signed-off-by Hans de Goede <[email protected]>

Quoted 7 lines

Quoted 3 lines

As this is meant to be merged via some other tree:

Acked-by Mauro Carvalho Chehab <[email protected]>

I think merging it through your tree would be just fine.

Acked-by Maxime Ripard <[email protected]>

Heh, I was thinking it would be best if it went through Maxime's tree because it also has some deps on new clk stuff (well the dts have deps on that), but either way works for me.

Maxime if you want this go through Mauro's tree, I can send a pull-req to Mauro (I'm a linux-media sub-maintainer), so if that is the case let me know and I'll prepare a pull-req (after fixing the missing reset documentation in the bindings).

Quoted 30 lines · Hans de Goede

Hi,

Quoted 19 lines · Maxime Ripard

Hi Mauro,

Quoted 16 lines · Mauro Carvalho Chehab

Em Thu, 20 Nov 2014 16:55:24 +0100
Hans de Goede <[email protected]> escreveu:

Quoted 12 lines

Add support for the larger fifo found on sun5i and sun6i, having a separate compatible for the ir found on sun5i & sun6i also is useful if we ever want to add ir transmit support, because the sun5i & sun6i version do not have transmit support.

Note this commits also adds checking for the end-of-packet interrupt flag (which was already enabled), as the fifo-data-available interrupt flag only gets set when the trigger-level is exceeded. So far we've been getting away with not doing this because of the low trigger-level, but this is something which we should have done since day one.

Signed-off-by Hans de Goede <[email protected]>

Quoted 7 lines

Quoted 3 lines

As this is meant to be merged via some other tree:

Acked-by Mauro Carvalho Chehab <[email protected]>

I think merging it through your tree would be just fine.

Acked-by Maxime Ripard <[email protected]>

Quoted 7 lines

Heh, I was thinking it would be best if it went through Maxime's tree because it also has some deps on new clk stuff (well the dts have deps on that), but either way works for me.

Maxime if you want this go through Mauro's tree, I can send a pull-req to Mauro (I'm a linux-media sub-maintainer), so if that is the case let me know and I'll prepare a pull-req (after fixing the missing reset documentation in the bindings).

So much for not reading the cover letter... Sorry.

We're getting quite close to the end of the ARM merge window, and I got a couple comments, Lee hasn't commented yet, so I'd say it's a bit too late for this to come in.

Hi,

Quoted 41 lines · Maxime Ripard

Quoted 30 lines · Hans de Goede

Hi,

Quoted 19 lines · Maxime Ripard

Hi Mauro,

Quoted 16 lines · Mauro Carvalho Chehab

Em Thu, 20 Nov 2014 16:55:24 +0100
Hans de Goede <[email protected]> escreveu:

Quoted 12 lines

Add support for the larger fifo found on sun5i and sun6i, having a separate compatible for the ir found on sun5i & sun6i also is useful if we ever want to add ir transmit support, because the sun5i & sun6i version do not have transmit support.

Note this commits also adds checking for the end-of-packet interrupt flag (which was already enabled), as the fifo-data-available interrupt flag only gets set when the trigger-level is exceeded. So far we've been getting away with not doing this because of the low trigger-level, but this is something which we should have done since day one.

Signed-off-by Hans de Goede <[email protected]>

Quoted 7 lines

Quoted 3 lines

As this is meant to be merged via some other tree:

Acked-by Mauro Carvalho Chehab <[email protected]>

I think merging it through your tree would be just fine.

Acked-by Maxime Ripard <[email protected]>

Quoted 7 lines

Heh, I was thinking it would be best if it went through Maxime's tree because it also has some deps on new clk stuff (well the dts have deps on that), but either way works for me.

Maxime if you want this go through Mauro's tree, I can send a pull-req to Mauro (I'm a linux-media sub-maintainer), so if that is the case let me know and I'll prepare a pull-req (after fixing the missing reset documentation in the bindings).

So much for not reading the cover letter... Sorry.

Quoted 3 lines

We're getting quite close to the end of the ARM merge window, and I got a couple comments, Lee hasn't commented yet, so I'd say it's a bit too late for this to come in.

Oh, but this was not intended for 3.19, this can wait till 3.20 from my pov, sorry if that was not clear. I was assuming that the merge window was more or less closed already, so that this going into 3.20 was expected.