Hi,
Here's the V12 of my patchset to add support for the Timer/Counter Unit
(TCU) present on the JZ47xx SoCs from Ingenic.
This patchset is much shorter at only 13 patches vs. 27 patches in V11;
the remaining patches will be sent in parallel (if applicable) or as a
follow-up patchset once this one is merged.
In V11 the clocksource maintainers weren't happy with the size of the
ingenic-timer driver, which included clocks and irqchip setup code.
On the other hand, devicetree maintainers wanted one single node for
the TCU hardware since it's effectively just one hardware block.
In this patchset the functionality is cut in four different drivers:
a MFD one to provide the regmap, probe the children and which provides
several API functions; a clocks driver; a irqchip driver; a clocksource
driver. All these drivers work with the same regmap, have the same
compatible strings, and will probe _with the same devicetree node_.
Regards,
-Paul
The default clock (48 MHz) is too fast for the system timer.
Signed-off-by: Paul Cercueil <[email protected]>
---
Notes:
v5: New patch
v6: Set also the rate for the clocksource channel's clock
v7: No change
v8: No change
v9: Don't configure clock timer1, as the OS Timer is used as
clocksource on this SoC
v10: Revert back to v8 bahaviour. Let the user choose what
clocksource should be used.
v11-v12: No change
arch/mips/boot/dts/ingenic/ci20.dts | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/arch/mips/boot/dts/ingenic/ci20.dts b/arch/mips/boot/dts/ingenic/ci20.dts
index 4f7b1fa31cf5..2e9952311ecd 100644
--- a/arch/mips/boot/dts/ingenic/ci20.dts
+++ b/arch/mips/boot/dts/ingenic/ci20.dts
@@ -2,6 +2,7 @@
/dts-v1/;
#include "jz4780.dtsi"
+#include <dt-bindings/clock/ingenic,tcu.h>
#include <dt-bindings/gpio/gpio.h>
/ {
@@ -238,3 +239,9 @@
bias-disable;
};
};
+
+&tcu {
+ /* 3 MHz for the system timer and clocksource */
+ assigned-clocks = <&tcu TCU_CLK_TIMER0>, <&tcu TCU_CLK_TIMER1>;
+ assigned-clock-rates = <3000000>, <3000000>;
+};
--
2.21.0.593.g511ec345e18
This driver will provide a regmap that can be retrieved very early in
the boot process through the API function ingenic_tcu_get_regmap().
Additionally, it will call devm_of_platform_populate() so that all the
children devices will be probed.
Signed-off-by: Paul Cercueil <[email protected]>
---
Notes:
v12: New patch
drivers/mfd/Kconfig | 8 +++
drivers/mfd/Makefile | 1 +
drivers/mfd/ingenic-tcu.c | 113 ++++++++++++++++++++++++++++++++
include/linux/mfd/ingenic-tcu.h | 8 +++
4 files changed, 130 insertions(+)
create mode 100644 drivers/mfd/ingenic-tcu.c
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 294d9567cc71..a13544474e05 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -494,6 +494,14 @@ config HTC_I2CPLD
This device provides input and output GPIOs through an I2C
interface to one or more sub-chips.
+config INGENIC_TCU
+ bool "Ingenic Timer/Counter Unit (TCU) support"
+ depends on MIPS || COMPILE_TEST
+ select REGMAP_MMIO
+ help
+ Say yes here to support the Timer/Counter Unit (TCU) IP present
+ in the JZ47xx SoCs from Ingenic.
+
config MFD_INTEL_QUARK_I2C_GPIO
tristate "Intel Quark MFD I2C GPIO"
depends on PCI
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 52b1a90ff515..fb89e131ae98 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -180,6 +180,7 @@ obj-$(CONFIG_AB8500_CORE) += ab8500-core.o ab8500-sysctrl.o
obj-$(CONFIG_MFD_TIMBERDALE) += timberdale.o
obj-$(CONFIG_PMIC_ADP5520) += adp5520.o
obj-$(CONFIG_MFD_KEMPLD) += kempld-core.o
+obj-$(CONFIG_INGENIC_TCU) += ingenic-tcu.o
obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO) += intel_quark_i2c_gpio.o
obj-$(CONFIG_LPC_SCH) += lpc_sch.o
obj-$(CONFIG_LPC_ICH) += lpc_ich.o
diff --git a/drivers/mfd/ingenic-tcu.c b/drivers/mfd/ingenic-tcu.c
new file mode 100644
index 000000000000..6c1d5e4310c1
--- /dev/null
+++ b/drivers/mfd/ingenic-tcu.c
@@ -0,0 +1,113 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * JZ47xx SoCs TCU MFD driver
+ * Copyright (C) 2019 Paul Cercueil <[email protected]>
+ */
+
+#include <linux/mfd/ingenic-tcu.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+struct ingenic_soc_info {
+ unsigned int num_channels;
+};
+
+static struct regmap *tcu_regmap __initdata;
+
+static const struct regmap_config ingenic_tcu_regmap_config = {
+ .reg_bits = 32,
+ .val_bits = 32,
+ .reg_stride = 4,
+ .max_register = TCU_REG_OST_CNTHBUF,
+};
+
+static const struct ingenic_soc_info jz4740_soc_info = {
+ .num_channels = 8,
+};
+
+static const struct ingenic_soc_info jz4725b_soc_info = {
+ .num_channels = 6,
+};
+
+static const struct of_device_id ingenic_tcu_of_match[] = {
+ { .compatible = "ingenic,jz4740-tcu", .data = &jz4740_soc_info, },
+ { .compatible = "ingenic,jz4725b-tcu", .data = &jz4725b_soc_info, },
+ { .compatible = "ingenic,jz4770-tcu", .data = &jz4740_soc_info, },
+ { }
+};
+
+static struct regmap * __init ingenic_tcu_create_regmap(struct device_node *np)
+{
+ struct resource res;
+ void __iomem *base;
+ struct regmap *map;
+
+ if (!of_match_node(ingenic_tcu_of_match, np))
+ return ERR_PTR(-EINVAL);
+
+ base = of_io_request_and_map(np, 0, "TCU");
+ if (IS_ERR(base))
+ return ERR_PTR(PTR_ERR(base));
+
+ map = regmap_init_mmio(NULL, base, &ingenic_tcu_regmap_config);
+ if (IS_ERR(map))
+ goto err_iounmap;
+
+ return map;
+
+err_iounmap:
+ iounmap(base);
+ of_address_to_resource(np, 0, &res);
+ release_mem_region(res.start, resource_size(&res));
+
+ return map;
+}
+
+static int __init ingenic_tcu_probe(struct platform_device *pdev)
+{
+ struct regmap *map = ingenic_tcu_get_regmap(pdev->dev.of_node);
+
+ platform_set_drvdata(pdev, map);
+
+ regmap_attach_dev(&pdev->dev, map, &ingenic_tcu_regmap_config);
+
+ return devm_of_platform_populate(&pdev->dev);
+}
+
+static struct platform_driver ingenic_tcu_driver = {
+ .driver = {
+ .name = "ingenic-tcu",
+ .of_match_table = ingenic_tcu_of_match,
+ },
+};
+
+static int __init ingenic_tcu_platform_init(void)
+{
+ return platform_driver_probe(&ingenic_tcu_driver,
+ ingenic_tcu_probe);
+}
+subsys_initcall(ingenic_tcu_platform_init);
+
+struct regmap * __init ingenic_tcu_get_regmap(struct device_node *np)
+{
+ if (!tcu_regmap)
+ tcu_regmap = ingenic_tcu_create_regmap(np);
+
+ return tcu_regmap;
+}
+
+bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned int channel)
+{
+ const struct ingenic_soc_info *soc = device_get_match_data(dev->parent);
+
+ /* Enable all TCU channels for PWM use by default except channels 0/1 */
+ u32 pwm_channels_mask = GENMASK(soc->num_channels - 1, 2);
+
+ device_property_read_u32(dev->parent, "ingenic,pwm-channels-mask",
+ &pwm_channels_mask);
+
+ return !!(pwm_channels_mask & BIT(channel));
+}
+EXPORT_SYMBOL_GPL(ingenic_tcu_pwm_can_use_chn);
diff --git a/include/linux/mfd/ingenic-tcu.h b/include/linux/mfd/ingenic-tcu.h
index 2083fa20821d..21df23916cd2 100644
--- a/include/linux/mfd/ingenic-tcu.h
+++ b/include/linux/mfd/ingenic-tcu.h
@@ -6,6 +6,11 @@
#define __LINUX_MFD_INGENIC_TCU_H_
#include <linux/bitops.h>
+#include <linux/init.h>
+
+struct device;
+struct device_node;
+struct regmap;
#define TCU_REG_WDT_TDR 0x00
#define TCU_REG_WDT_TCER 0x04
@@ -53,4 +58,7 @@
#define TCU_REG_TCNTc(c) (TCU_REG_TCNT0 + ((c) * TCU_CHANNEL_STRIDE))
#define TCU_REG_TCSRc(c) (TCU_REG_TCSR0 + ((c) * TCU_CHANNEL_STRIDE))
+struct regmap * __init ingenic_tcu_get_regmap(struct device_node *np);
+bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned int channel);
+
#endif /* __LINUX_MFD_INGENIC_TCU_H_ */
--
2.21.0.593.g511ec345e18
On Tue, May 21, 2019 at 4:51 PM Paul Cercueil <[email protected]> wrote:
>
> Hi,
>
> Here's the V12 of my patchset to add support for the Timer/Counter Unit
> (TCU) present on the JZ47xx SoCs from Ingenic.
>
> This patchset is much shorter at only 13 patches vs. 27 patches in V11;
> the remaining patches will be sent in parallel (if applicable) or as a
> follow-up patchset once this one is merged.
>
> In V11 the clocksource maintainers weren't happy with the size of the
> ingenic-timer driver, which included clocks and irqchip setup code.
> On the other hand, devicetree maintainers wanted one single node for
> the TCU hardware since it's effectively just one hardware block.
>
> In this patchset the functionality is cut in four different drivers:
> a MFD one to provide the regmap, probe the children and which provides
> several API functions; a clocks driver; a irqchip driver; a clocksource
> driver. All these drivers work with the same regmap, have the same
> compatible strings, and will probe _with the same devicetree node_.
For the series:
Tested-by: Mathieu Malaterre <[email protected]>
System: MIPS Creator CI20
For reference, here is my local patch:
diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi
b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index 1bfac58da5df..e7b7da32f278 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
#include <dt-bindings/clock/jz4780-cgu.h>
+#include <dt-bindings/clock/ingenic,tcu.h>
#include <dt-bindings/dma/jz4780-dma.h>
/ {
@@ -80,6 +81,15 @@
interrupt-parent = <&intc>;
interrupts = <27 26 25>;
+
+ watchdog: watchdog@0 {
+ compatible = "ingenic,jz4780-watchdog";
+ reg = <0x0 0xc>;
+
+ clocks = <&tcu TCU_CLK_WDT>;
+ clock-names = "wdt";
+ };
+
};
rtc_dev: rtc@10003000 {
@@ -287,14 +297,6 @@
status = "disabled";
};
- watchdog: watchdog@10002000 {
- compatible = "ingenic,jz4780-watchdog";
- reg = <0x10002000 0x10>;
-
- clocks = <&cgu JZ4780_CLK_RTCLK>;
- clock-names = "rtc";
- };
-
nemc: nemc@13410000 {
compatible = "ingenic,jz4780-nemc";
reg = <0x13410000 0x10000>;
> Regards,
> -Paul
>
>
Hi,
I'll make a V13 of this patchset on top of -rc6, any feedback
for this MFD driver? It's been already one month.
Thanks,
-Paul
Le mar. 21 mai 2019 ? 16:51, Paul Cercueil <[email protected]> a
?crit :
> This driver will provide a regmap that can be retrieved very early in
> the boot process through the API function ingenic_tcu_get_regmap().
>
> Additionally, it will call devm_of_platform_populate() so that all the
> children devices will be probed.
>
> Signed-off-by: Paul Cercueil <[email protected]>
> ---
>
> Notes:
> v12: New patch
>
> drivers/mfd/Kconfig | 8 +++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/ingenic-tcu.c | 113
> ++++++++++++++++++++++++++++++++
> include/linux/mfd/ingenic-tcu.h | 8 +++
> 4 files changed, 130 insertions(+)
> create mode 100644 drivers/mfd/ingenic-tcu.c
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 294d9567cc71..a13544474e05 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -494,6 +494,14 @@ config HTC_I2CPLD
> This device provides input and output GPIOs through an I2C
> interface to one or more sub-chips.
>
> +config INGENIC_TCU
> + bool "Ingenic Timer/Counter Unit (TCU) support"
> + depends on MIPS || COMPILE_TEST
> + select REGMAP_MMIO
> + help
> + Say yes here to support the Timer/Counter Unit (TCU) IP present
> + in the JZ47xx SoCs from Ingenic.
> +
> config MFD_INTEL_QUARK_I2C_GPIO
> tristate "Intel Quark MFD I2C GPIO"
> depends on PCI
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 52b1a90ff515..fb89e131ae98 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -180,6 +180,7 @@ obj-$(CONFIG_AB8500_CORE) += ab8500-core.o
> ab8500-sysctrl.o
> obj-$(CONFIG_MFD_TIMBERDALE) += timberdale.o
> obj-$(CONFIG_PMIC_ADP5520) += adp5520.o
> obj-$(CONFIG_MFD_KEMPLD) += kempld-core.o
> +obj-$(CONFIG_INGENIC_TCU) += ingenic-tcu.o
> obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO) += intel_quark_i2c_gpio.o
> obj-$(CONFIG_LPC_SCH) += lpc_sch.o
> obj-$(CONFIG_LPC_ICH) += lpc_ich.o
> diff --git a/drivers/mfd/ingenic-tcu.c b/drivers/mfd/ingenic-tcu.c
> new file mode 100644
> index 000000000000..6c1d5e4310c1
> --- /dev/null
> +++ b/drivers/mfd/ingenic-tcu.c
> @@ -0,0 +1,113 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * JZ47xx SoCs TCU MFD driver
> + * Copyright (C) 2019 Paul Cercueil <[email protected]>
> + */
> +
> +#include <linux/mfd/ingenic-tcu.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +struct ingenic_soc_info {
> + unsigned int num_channels;
> +};
> +
> +static struct regmap *tcu_regmap __initdata;
> +
> +static const struct regmap_config ingenic_tcu_regmap_config = {
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = 4,
> + .max_register = TCU_REG_OST_CNTHBUF,
> +};
> +
> +static const struct ingenic_soc_info jz4740_soc_info = {
> + .num_channels = 8,
> +};
> +
> +static const struct ingenic_soc_info jz4725b_soc_info = {
> + .num_channels = 6,
> +};
> +
> +static const struct of_device_id ingenic_tcu_of_match[] = {
> + { .compatible = "ingenic,jz4740-tcu", .data = &jz4740_soc_info, },
> + { .compatible = "ingenic,jz4725b-tcu", .data = &jz4725b_soc_info, },
> + { .compatible = "ingenic,jz4770-tcu", .data = &jz4740_soc_info, },
> + { }
> +};
> +
> +static struct regmap * __init ingenic_tcu_create_regmap(struct
> device_node *np)
> +{
> + struct resource res;
> + void __iomem *base;
> + struct regmap *map;
> +
> + if (!of_match_node(ingenic_tcu_of_match, np))
> + return ERR_PTR(-EINVAL);
> +
> + base = of_io_request_and_map(np, 0, "TCU");
> + if (IS_ERR(base))
> + return ERR_PTR(PTR_ERR(base));
> +
> + map = regmap_init_mmio(NULL, base, &ingenic_tcu_regmap_config);
> + if (IS_ERR(map))
> + goto err_iounmap;
> +
> + return map;
> +
> +err_iounmap:
> + iounmap(base);
> + of_address_to_resource(np, 0, &res);
> + release_mem_region(res.start, resource_size(&res));
> +
> + return map;
> +}
> +
> +static int __init ingenic_tcu_probe(struct platform_device *pdev)
> +{
> + struct regmap *map = ingenic_tcu_get_regmap(pdev->dev.of_node);
> +
> + platform_set_drvdata(pdev, map);
> +
> + regmap_attach_dev(&pdev->dev, map, &ingenic_tcu_regmap_config);
> +
> + return devm_of_platform_populate(&pdev->dev);
> +}
> +
> +static struct platform_driver ingenic_tcu_driver = {
> + .driver = {
> + .name = "ingenic-tcu",
> + .of_match_table = ingenic_tcu_of_match,
> + },
> +};
> +
> +static int __init ingenic_tcu_platform_init(void)
> +{
> + return platform_driver_probe(&ingenic_tcu_driver,
> + ingenic_tcu_probe);
> +}
> +subsys_initcall(ingenic_tcu_platform_init);
> +
> +struct regmap * __init ingenic_tcu_get_regmap(struct device_node *np)
> +{
> + if (!tcu_regmap)
> + tcu_regmap = ingenic_tcu_create_regmap(np);
> +
> + return tcu_regmap;
> +}
> +
> +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned int
> channel)
> +{
> + const struct ingenic_soc_info *soc =
> device_get_match_data(dev->parent);
> +
> + /* Enable all TCU channels for PWM use by default except channels
> 0/1 */
> + u32 pwm_channels_mask = GENMASK(soc->num_channels - 1, 2);
> +
> + device_property_read_u32(dev->parent, "ingenic,pwm-channels-mask",
> + &pwm_channels_mask);
> +
> + return !!(pwm_channels_mask & BIT(channel));
> +}
> +EXPORT_SYMBOL_GPL(ingenic_tcu_pwm_can_use_chn);
> diff --git a/include/linux/mfd/ingenic-tcu.h
> b/include/linux/mfd/ingenic-tcu.h
> index 2083fa20821d..21df23916cd2 100644
> --- a/include/linux/mfd/ingenic-tcu.h
> +++ b/include/linux/mfd/ingenic-tcu.h
> @@ -6,6 +6,11 @@
> #define __LINUX_MFD_INGENIC_TCU_H_
>
> #include <linux/bitops.h>
> +#include <linux/init.h>
> +
> +struct device;
> +struct device_node;
> +struct regmap;
>
> #define TCU_REG_WDT_TDR 0x00
> #define TCU_REG_WDT_TCER 0x04
> @@ -53,4 +58,7 @@
> #define TCU_REG_TCNTc(c) (TCU_REG_TCNT0 + ((c) * TCU_CHANNEL_STRIDE))
> #define TCU_REG_TCSRc(c) (TCU_REG_TCSR0 + ((c) * TCU_CHANNEL_STRIDE))
>
> +struct regmap * __init ingenic_tcu_get_regmap(struct device_node
> *np);
> +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned int
> channel);
> +
> #endif /* __LINUX_MFD_INGENIC_TCU_H_ */
> --
> 2.21.0.593.g511ec345e18
>
On Tue, 21 May 2019, Paul Cercueil wrote:
> This driver will provide a regmap that can be retrieved very early in
> the boot process through the API function ingenic_tcu_get_regmap().
>
> Additionally, it will call devm_of_platform_populate() so that all the
> children devices will be probed.
>
> Signed-off-by: Paul Cercueil <[email protected]>
> ---
>
> Notes:
> v12: New patch
>
> drivers/mfd/Kconfig | 8 +++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/ingenic-tcu.c | 113 ++++++++++++++++++++++++++++++++
> include/linux/mfd/ingenic-tcu.h | 8 +++
> 4 files changed, 130 insertions(+)
> create mode 100644 drivers/mfd/ingenic-tcu.c
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 294d9567cc71..a13544474e05 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -494,6 +494,14 @@ config HTC_I2CPLD
> This device provides input and output GPIOs through an I2C
> interface to one or more sub-chips.
>
> +config INGENIC_TCU
> + bool "Ingenic Timer/Counter Unit (TCU) support"
> + depends on MIPS || COMPILE_TEST
> + select REGMAP_MMIO
> + help
> + Say yes here to support the Timer/Counter Unit (TCU) IP present
> + in the JZ47xx SoCs from Ingenic.
> +
> config MFD_INTEL_QUARK_I2C_GPIO
> tristate "Intel Quark MFD I2C GPIO"
> depends on PCI
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 52b1a90ff515..fb89e131ae98 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -180,6 +180,7 @@ obj-$(CONFIG_AB8500_CORE) += ab8500-core.o ab8500-sysctrl.o
> obj-$(CONFIG_MFD_TIMBERDALE) += timberdale.o
> obj-$(CONFIG_PMIC_ADP5520) += adp5520.o
> obj-$(CONFIG_MFD_KEMPLD) += kempld-core.o
> +obj-$(CONFIG_INGENIC_TCU) += ingenic-tcu.o
> obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO) += intel_quark_i2c_gpio.o
> obj-$(CONFIG_LPC_SCH) += lpc_sch.o
> obj-$(CONFIG_LPC_ICH) += lpc_ich.o
> diff --git a/drivers/mfd/ingenic-tcu.c b/drivers/mfd/ingenic-tcu.c
> new file mode 100644
> index 000000000000..6c1d5e4310c1
> --- /dev/null
> +++ b/drivers/mfd/ingenic-tcu.c
> @@ -0,0 +1,113 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * JZ47xx SoCs TCU MFD driver
Nit: Another line here please.
> + * Copyright (C) 2019 Paul Cercueil <[email protected]>
> + */
> +
> +#include <linux/mfd/ingenic-tcu.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +struct ingenic_soc_info {
> + unsigned int num_channels;
> +};
> +
> +static struct regmap *tcu_regmap __initdata;
> +
> +static const struct regmap_config ingenic_tcu_regmap_config = {
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = 4,
> + .max_register = TCU_REG_OST_CNTHBUF,
> +};
> +
> +static const struct ingenic_soc_info jz4740_soc_info = {
> + .num_channels = 8,
> +};
> +
> +static const struct ingenic_soc_info jz4725b_soc_info = {
> + .num_channels = 6,
> +};
> +
> +static const struct of_device_id ingenic_tcu_of_match[] = {
> + { .compatible = "ingenic,jz4740-tcu", .data = &jz4740_soc_info, },
> + { .compatible = "ingenic,jz4725b-tcu", .data = &jz4725b_soc_info, },
> + { .compatible = "ingenic,jz4770-tcu", .data = &jz4740_soc_info, },
> + { }
> +};
> +
> +static struct regmap * __init ingenic_tcu_create_regmap(struct device_node *np)
> +{
> + struct resource res;
> + void __iomem *base;
> + struct regmap *map;
> +
> + if (!of_match_node(ingenic_tcu_of_match, np))
> + return ERR_PTR(-EINVAL);
> +
> + base = of_io_request_and_map(np, 0, "TCU");
> + if (IS_ERR(base))
> + return ERR_PTR(PTR_ERR(base));
> +
> + map = regmap_init_mmio(NULL, base, &ingenic_tcu_regmap_config);
> + if (IS_ERR(map))
> + goto err_iounmap;
> +
> + return map;
> +
> +err_iounmap:
> + iounmap(base);
> + of_address_to_resource(np, 0, &res);
> + release_mem_region(res.start, resource_size(&res));
> +
> + return map;
> +}
Why does this need to be set-up earlier than probe()?
> +static int __init ingenic_tcu_probe(struct platform_device *pdev)
> +{
> + struct regmap *map = ingenic_tcu_get_regmap(pdev->dev.of_node);
> +
> + platform_set_drvdata(pdev, map);
> +
> + regmap_attach_dev(&pdev->dev, map, &ingenic_tcu_regmap_config);
> +
> + return devm_of_platform_populate(&pdev->dev);
> +}
> +
> +static struct platform_driver ingenic_tcu_driver = {
> + .driver = {
> + .name = "ingenic-tcu",
> + .of_match_table = ingenic_tcu_of_match,
> + },
> +};
> +
> +static int __init ingenic_tcu_platform_init(void)
> +{
> + return platform_driver_probe(&ingenic_tcu_driver,
> + ingenic_tcu_probe);
What? Why?
> +}
> +subsys_initcall(ingenic_tcu_platform_init);
> +
> +struct regmap * __init ingenic_tcu_get_regmap(struct device_node *np)
> +{
> + if (!tcu_regmap)
> + tcu_regmap = ingenic_tcu_create_regmap(np);
> +
> + return tcu_regmap;
> +}
This makes me pretty uncomfortable.
What calls it?
> +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned int channel)
> +{
> + const struct ingenic_soc_info *soc = device_get_match_data(dev->parent);
> +
> + /* Enable all TCU channels for PWM use by default except channels 0/1 */
> + u32 pwm_channels_mask = GENMASK(soc->num_channels - 1, 2);
> +
> + device_property_read_u32(dev->parent, "ingenic,pwm-channels-mask",
> + &pwm_channels_mask);
> +
> + return !!(pwm_channels_mask & BIT(channel));
> +}
> +EXPORT_SYMBOL_GPL(ingenic_tcu_pwm_can_use_chn);
> diff --git a/include/linux/mfd/ingenic-tcu.h b/include/linux/mfd/ingenic-tcu.h
> index 2083fa20821d..21df23916cd2 100644
> --- a/include/linux/mfd/ingenic-tcu.h
> +++ b/include/linux/mfd/ingenic-tcu.h
> @@ -6,6 +6,11 @@
> #define __LINUX_MFD_INGENIC_TCU_H_
>
> #include <linux/bitops.h>
> +#include <linux/init.h>
> +
> +struct device;
> +struct device_node;
> +struct regmap;
>
> #define TCU_REG_WDT_TDR 0x00
> #define TCU_REG_WDT_TCER 0x04
> @@ -53,4 +58,7 @@
> #define TCU_REG_TCNTc(c) (TCU_REG_TCNT0 + ((c) * TCU_CHANNEL_STRIDE))
> #define TCU_REG_TCSRc(c) (TCU_REG_TCSR0 + ((c) * TCU_CHANNEL_STRIDE))
>
> +struct regmap * __init ingenic_tcu_get_regmap(struct device_node *np);
> +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned int channel);
> +
> #endif /* __LINUX_MFD_INGENIC_TCU_H_ */
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Hi Lee,
Le mer. 26 juin 2019 à 15:18, Lee Jones <[email protected]> a
écrit :
> On Tue, 21 May 2019, Paul Cercueil wrote:
>
>> This driver will provide a regmap that can be retrieved very early
>> in
>> the boot process through the API function ingenic_tcu_get_regmap().
>>
>> Additionally, it will call devm_of_platform_populate() so that all
>> the
>> children devices will be probed.
>>
>> Signed-off-by: Paul Cercueil <[email protected]>
>> ---
>>
>> Notes:
>> v12: New patch
>>
>> drivers/mfd/Kconfig | 8 +++
>> drivers/mfd/Makefile | 1 +
>> drivers/mfd/ingenic-tcu.c | 113
>> ++++++++++++++++++++++++++++++++
>> include/linux/mfd/ingenic-tcu.h | 8 +++
>> 4 files changed, 130 insertions(+)
>> create mode 100644 drivers/mfd/ingenic-tcu.c
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index 294d9567cc71..a13544474e05 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -494,6 +494,14 @@ config HTC_I2CPLD
>> This device provides input and output GPIOs through an I2C
>> interface to one or more sub-chips.
>>
>> +config INGENIC_TCU
>> + bool "Ingenic Timer/Counter Unit (TCU) support"
>> + depends on MIPS || COMPILE_TEST
>> + select REGMAP_MMIO
>> + help
>> + Say yes here to support the Timer/Counter Unit (TCU) IP present
>> + in the JZ47xx SoCs from Ingenic.
>> +
>> config MFD_INTEL_QUARK_I2C_GPIO
>> tristate "Intel Quark MFD I2C GPIO"
>> depends on PCI
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index 52b1a90ff515..fb89e131ae98 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -180,6 +180,7 @@ obj-$(CONFIG_AB8500_CORE) += ab8500-core.o
>> ab8500-sysctrl.o
>> obj-$(CONFIG_MFD_TIMBERDALE) += timberdale.o
>> obj-$(CONFIG_PMIC_ADP5520) += adp5520.o
>> obj-$(CONFIG_MFD_KEMPLD) += kempld-core.o
>> +obj-$(CONFIG_INGENIC_TCU) += ingenic-tcu.o
>> obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO) += intel_quark_i2c_gpio.o
>> obj-$(CONFIG_LPC_SCH) += lpc_sch.o
>> obj-$(CONFIG_LPC_ICH) += lpc_ich.o
>> diff --git a/drivers/mfd/ingenic-tcu.c b/drivers/mfd/ingenic-tcu.c
>> new file mode 100644
>> index 000000000000..6c1d5e4310c1
>> --- /dev/null
>> +++ b/drivers/mfd/ingenic-tcu.c
>> @@ -0,0 +1,113 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * JZ47xx SoCs TCU MFD driver
>
> Nit: Another line here please.
>
>> + * Copyright (C) 2019 Paul Cercueil <[email protected]>
>> + */
>> +
>> +#include <linux/mfd/ingenic-tcu.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +
>> +struct ingenic_soc_info {
>> + unsigned int num_channels;
>> +};
>> +
>> +static struct regmap *tcu_regmap __initdata;
>> +
>> +static const struct regmap_config ingenic_tcu_regmap_config = {
>> + .reg_bits = 32,
>> + .val_bits = 32,
>> + .reg_stride = 4,
>> + .max_register = TCU_REG_OST_CNTHBUF,
>> +};
>> +
>> +static const struct ingenic_soc_info jz4740_soc_info = {
>> + .num_channels = 8,
>> +};
>> +
>> +static const struct ingenic_soc_info jz4725b_soc_info = {
>> + .num_channels = 6,
>> +};
>> +
>> +static const struct of_device_id ingenic_tcu_of_match[] = {
>> + { .compatible = "ingenic,jz4740-tcu", .data = &jz4740_soc_info, },
>> + { .compatible = "ingenic,jz4725b-tcu", .data = &jz4725b_soc_info,
>> },
>> + { .compatible = "ingenic,jz4770-tcu", .data = &jz4740_soc_info, },
>> + { }
>> +};
>> +
>> +static struct regmap * __init ingenic_tcu_create_regmap(struct
>> device_node *np)
>> +{
>> + struct resource res;
>> + void __iomem *base;
>> + struct regmap *map;
>> +
>> + if (!of_match_node(ingenic_tcu_of_match, np))
>> + return ERR_PTR(-EINVAL);
>> +
>> + base = of_io_request_and_map(np, 0, "TCU");
>> + if (IS_ERR(base))
>> + return ERR_PTR(PTR_ERR(base));
>> +
>> + map = regmap_init_mmio(NULL, base, &ingenic_tcu_regmap_config);
>> + if (IS_ERR(map))
>> + goto err_iounmap;
>> +
>> + return map;
>> +
>> +err_iounmap:
>> + iounmap(base);
>> + of_address_to_resource(np, 0, &res);
>> + release_mem_region(res.start, resource_size(&res));
>> +
>> + return map;
>> +}
>
> Why does this need to be set-up earlier than probe()?
See the explanation below.
>> +static int __init ingenic_tcu_probe(struct platform_device *pdev)
>> +{
>> + struct regmap *map = ingenic_tcu_get_regmap(pdev->dev.of_node);
>> +
>> + platform_set_drvdata(pdev, map);
>> +
>> + regmap_attach_dev(&pdev->dev, map, &ingenic_tcu_regmap_config);
>> +
>> + return devm_of_platform_populate(&pdev->dev);
>> +}
>> +
>> +static struct platform_driver ingenic_tcu_driver = {
>> + .driver = {
>> + .name = "ingenic-tcu",
>> + .of_match_table = ingenic_tcu_of_match,
>> + },
>> +};
>> +
>> +static int __init ingenic_tcu_platform_init(void)
>> +{
>> + return platform_driver_probe(&ingenic_tcu_driver,
>> + ingenic_tcu_probe);
>
> What? Why?
The device driver probed here will populate the children devices,
which will be able to retrieve the pointer to the regmap through
device_get_regmap(dev->parent).
The children devices are normal platform drivers that can be probed
the normal way. These are the PWM driver, the watchdog driver, and the
OST (OS Timer) clocksource driver, all part of the same hardware block
(the Timer/Counter Unit or TCU).
>> +}
>> +subsys_initcall(ingenic_tcu_platform_init);
>> +
>> +struct regmap * __init ingenic_tcu_get_regmap(struct device_node
>> *np)
>> +{
>> + if (!tcu_regmap)
>> + tcu_regmap = ingenic_tcu_create_regmap(np);
>> +
>> + return tcu_regmap;
>> +}
>
> This makes me pretty uncomfortable.
>
> What calls it?
The TCU IRQ driver (patch [06/13]), clocks driver (patch [05/13]), and
the
non-OST clocksource driver (patch [07/13]) all probe very early in the
boot
process, and share the same devicetree node. They call this function to
get
a pointer to the regmap.
>> +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned int
>> channel)
>> +{
>> + const struct ingenic_soc_info *soc =
>> device_get_match_data(dev->parent);
>> +
>> + /* Enable all TCU channels for PWM use by default except channels
>> 0/1 */
>> + u32 pwm_channels_mask = GENMASK(soc->num_channels - 1, 2);
>> +
>> + device_property_read_u32(dev->parent, "ingenic,pwm-channels-mask",
>> + &pwm_channels_mask);
>> +
>> + return !!(pwm_channels_mask & BIT(channel));
>> +}
>> +EXPORT_SYMBOL_GPL(ingenic_tcu_pwm_can_use_chn);
>> diff --git a/include/linux/mfd/ingenic-tcu.h
>> b/include/linux/mfd/ingenic-tcu.h
>> index 2083fa20821d..21df23916cd2 100644
>> --- a/include/linux/mfd/ingenic-tcu.h
>> +++ b/include/linux/mfd/ingenic-tcu.h
>> @@ -6,6 +6,11 @@
>> #define __LINUX_MFD_INGENIC_TCU_H_
>>
>> #include <linux/bitops.h>
>> +#include <linux/init.h>
>> +
>> +struct device;
>> +struct device_node;
>> +struct regmap;
>>
>> #define TCU_REG_WDT_TDR 0x00
>> #define TCU_REG_WDT_TCER 0x04
>> @@ -53,4 +58,7 @@
>> #define TCU_REG_TCNTc(c) (TCU_REG_TCNT0 + ((c) *
>> TCU_CHANNEL_STRIDE))
>> #define TCU_REG_TCSRc(c) (TCU_REG_TCSR0 + ((c) *
>> TCU_CHANNEL_STRIDE))
>>
>> +struct regmap * __init ingenic_tcu_get_regmap(struct device_node
>> *np);
>> +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned int
>> channel);
>> +
>> #endif /* __LINUX_MFD_INGENIC_TCU_H_ */
>
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
On Wed, 26 Jun 2019, Paul Cercueil wrote:
> Le mer. 26 juin 2019 à 15:18, Lee Jones <[email protected]> a écrit :
> > On Tue, 21 May 2019, Paul Cercueil wrote:
> >
> > > This driver will provide a regmap that can be retrieved very early
> > > in
> > > the boot process through the API function ingenic_tcu_get_regmap().
> > >
> > > Additionally, it will call devm_of_platform_populate() so that all
> > > the
> > > children devices will be probed.
> > >
> > > Signed-off-by: Paul Cercueil <[email protected]>
> > > ---
> > >
> > > Notes:
> > > v12: New patch
> > >
> > > drivers/mfd/Kconfig | 8 +++
> > > drivers/mfd/Makefile | 1 +
> > > drivers/mfd/ingenic-tcu.c | 113
> > > ++++++++++++++++++++++++++++++++
> > > include/linux/mfd/ingenic-tcu.h | 8 +++
> > > 4 files changed, 130 insertions(+)
> > > create mode 100644 drivers/mfd/ingenic-tcu.c
[...]
> > > +static struct regmap * __init ingenic_tcu_create_regmap(struct
> > > device_node *np)
> > > +{
> > > + struct resource res;
> > > + void __iomem *base;
> > > + struct regmap *map;
> > > +
> > > + if (!of_match_node(ingenic_tcu_of_match, np))
> > > + return ERR_PTR(-EINVAL);
Drop this check.
> > > + base = of_io_request_and_map(np, 0, "TCU");
> > > + if (IS_ERR(base))
> > > + return ERR_PTR(PTR_ERR(base));
> > > +
> > > + map = regmap_init_mmio(NULL, base, &ingenic_tcu_regmap_config);
> > > + if (IS_ERR(map))
> > > + goto err_iounmap;
Place this inside probe().
> > > + return map;
> > > +
> > > +err_iounmap:
> > > + iounmap(base);
> > > + of_address_to_resource(np, 0, &res);
> > > + release_mem_region(res.start, resource_size(&res));
> > > +
> > > + return map;
> > > +}
> >
> > Why does this need to be set-up earlier than probe()?
>
> See the explanation below.
I think the answer is, it doesn't.
> > > +static int __init ingenic_tcu_probe(struct platform_device *pdev)
> > > +{
> > > + struct regmap *map = ingenic_tcu_get_regmap(pdev->dev.of_node);
> > > +
> > > + platform_set_drvdata(pdev, map);
> > > +
> > > + regmap_attach_dev(&pdev->dev, map, &ingenic_tcu_regmap_config);
> > > +
> > > + return devm_of_platform_populate(&pdev->dev);
> > > +}
> > > +
> > > +static struct platform_driver ingenic_tcu_driver = {
> > > + .driver = {
> > > + .name = "ingenic-tcu",
> > > + .of_match_table = ingenic_tcu_of_match,
> > > + },
> > > +};
> > > +
> > > +static int __init ingenic_tcu_platform_init(void)
> > > +{
> > > + return platform_driver_probe(&ingenic_tcu_driver,
> > > + ingenic_tcu_probe);
> >
> > What? Why?
>
> The device driver probed here will populate the children devices,
> which will be able to retrieve the pointer to the regmap through
> device_get_regmap(dev->parent).
I've never heard of this call. Where is it?
> The children devices are normal platform drivers that can be probed
> the normal way. These are the PWM driver, the watchdog driver, and the
> OST (OS Timer) clocksource driver, all part of the same hardware block
> (the Timer/Counter Unit or TCU).
If they are normal devices, then there is no need to roll your own
regmap-getter implementation like this.
> > > +}
> > > +subsys_initcall(ingenic_tcu_platform_init);
> > > +
> > > +struct regmap * __init ingenic_tcu_get_regmap(struct device_node
> > > *np)
> > > +{
> > > + if (!tcu_regmap)
> > > + tcu_regmap = ingenic_tcu_create_regmap(np);
> > > +
> > > + return tcu_regmap;
> > > +}
> >
> > This makes me pretty uncomfortable.
> >
> > What calls it?
>
> The TCU IRQ driver (patch [06/13]), clocks driver (patch [05/13]), and the
> non-OST clocksource driver (patch [07/13]) all probe very early in the boot
> process, and share the same devicetree node. They call this function to get
> a pointer to the regmap.
Horrible!
Instead, you should send it through platform_set_drvdata() and collect
it in the child drivers with platform_get_drvdata(dev->parent).
> > > +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned int
> > > channel)
> > > +{
> > > + const struct ingenic_soc_info *soc =
> > > device_get_match_data(dev->parent);
> > > +
> > > + /* Enable all TCU channels for PWM use by default except channels
> > > 0/1 */
> > > + u32 pwm_channels_mask = GENMASK(soc->num_channels - 1, 2);
> > > +
> > > + device_property_read_u32(dev->parent, "ingenic,pwm-channels-mask",
> > > + &pwm_channels_mask);
Doesn't this call overwrite the previous assignment above?
> > > + return !!(pwm_channels_mask & BIT(channel));
> > > +}
> > > +EXPORT_SYMBOL_GPL(ingenic_tcu_pwm_can_use_chn);
Where is this called from?
I think this needs a review by the DT guys.
> > > diff --git a/include/linux/mfd/ingenic-tcu.h
> > > b/include/linux/mfd/ingenic-tcu.h
> > > index 2083fa20821d..21df23916cd2 100644
> > > --- a/include/linux/mfd/ingenic-tcu.h
> > > +++ b/include/linux/mfd/ingenic-tcu.h
> > > @@ -6,6 +6,11 @@
> > > #define __LINUX_MFD_INGENIC_TCU_H_
> > >
> > > #include <linux/bitops.h>
> > > +#include <linux/init.h>
> > > +
> > > +struct device;
> > > +struct device_node;
> > > +struct regmap;
> > >
> > > #define TCU_REG_WDT_TDR 0x00
> > > #define TCU_REG_WDT_TCER 0x04
> > > @@ -53,4 +58,7 @@
> > > #define TCU_REG_TCNTc(c) (TCU_REG_TCNT0 + ((c) *
> > > TCU_CHANNEL_STRIDE))
> > > #define TCU_REG_TCSRc(c) (TCU_REG_TCSR0 + ((c) *
> > > TCU_CHANNEL_STRIDE))
> > >
> > > +struct regmap * __init ingenic_tcu_get_regmap(struct device_node
> > > *np);
> > > +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned int
> > > channel);
> > > +
> > > #endif /* __LINUX_MFD_INGENIC_TCU_H_ */
> >
>
>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Le jeu. 27 juin 2019 à 8:58, Lee Jones <[email protected]> a écrit
:
> On Wed, 26 Jun 2019, Paul Cercueil wrote:
>> Le mer. 26 juin 2019 à 15:18, Lee Jones <[email protected]> a
>> écrit :
>> > On Tue, 21 May 2019, Paul Cercueil wrote:
>> >
>> > > This driver will provide a regmap that can be retrieved very
>> early
>> > > in
>> > > the boot process through the API function
>> ingenic_tcu_get_regmap().
>> > >
>> > > Additionally, it will call devm_of_platform_populate() so that
>> all
>> > > the
>> > > children devices will be probed.
>> > >
>> > > Signed-off-by: Paul Cercueil <[email protected]>
>> > > ---
>> > >
>> > > Notes:
>> > > v12: New patch
>> > >
>> > > drivers/mfd/Kconfig | 8 +++
>> > > drivers/mfd/Makefile | 1 +
>> > > drivers/mfd/ingenic-tcu.c | 113
>> > > ++++++++++++++++++++++++++++++++
>> > > include/linux/mfd/ingenic-tcu.h | 8 +++
>> > > 4 files changed, 130 insertions(+)
>> > > create mode 100644 drivers/mfd/ingenic-tcu.c
>
> [...]
>
>> > > +static struct regmap * __init ingenic_tcu_create_regmap(struct
>> > > device_node *np)
>> > > +{
>> > > + struct resource res;
>> > > + void __iomem *base;
>> > > + struct regmap *map;
>> > > +
>> > > + if (!of_match_node(ingenic_tcu_of_match, np))
>> > > + return ERR_PTR(-EINVAL);
>
> Drop this check.
>
>> > > + base = of_io_request_and_map(np, 0, "TCU");
>> > > + if (IS_ERR(base))
>> > > + return ERR_PTR(PTR_ERR(base));
>> > > +
>> > > + map = regmap_init_mmio(NULL, base,
>> &ingenic_tcu_regmap_config);
>> > > + if (IS_ERR(map))
>> > > + goto err_iounmap;
>
> Place this inside probe().
>
>> > > + return map;
>> > > +
>> > > +err_iounmap:
>> > > + iounmap(base);
>> > > + of_address_to_resource(np, 0, &res);
>> > > + release_mem_region(res.start, resource_size(&res));
>> > > +
>> > > + return map;
>> > > +}
>> >
>> > Why does this need to be set-up earlier than probe()?
>>
>> See the explanation below.
>
> I think the answer is, it doesn't.
>
>> > > +static int __init ingenic_tcu_probe(struct platform_device
>> *pdev)
>> > > +{
>> > > + struct regmap *map =
>> ingenic_tcu_get_regmap(pdev->dev.of_node);
>> > > +
>> > > + platform_set_drvdata(pdev, map);
>> > > +
>> > > + regmap_attach_dev(&pdev->dev, map,
>> &ingenic_tcu_regmap_config);
>> > > +
>> > > + return devm_of_platform_populate(&pdev->dev);
>> > > +}
>> > > +
>> > > +static struct platform_driver ingenic_tcu_driver = {
>> > > + .driver = {
>> > > + .name = "ingenic-tcu",
>> > > + .of_match_table = ingenic_tcu_of_match,
>> > > + },
>> > > +};
>> > > +
>> > > +static int __init ingenic_tcu_platform_init(void)
>> > > +{
>> > > + return platform_driver_probe(&ingenic_tcu_driver,
>> > > + ingenic_tcu_probe);
>> >
>> > What? Why?
>>
>> The device driver probed here will populate the children devices,
>> which will be able to retrieve the pointer to the regmap through
>> device_get_regmap(dev->parent).
>
> I've never heard of this call. Where is it?
dev_get_regmap, in <linux/regmap.h>.
>> The children devices are normal platform drivers that can be probed
>> the normal way. These are the PWM driver, the watchdog driver, and
>> the
>> OST (OS Timer) clocksource driver, all part of the same hardware
>> block
>> (the Timer/Counter Unit or TCU).
>
> If they are normal devices, then there is no need to roll your own
> regmap-getter implementation like this.
>
>> > > +}
>> > > +subsys_initcall(ingenic_tcu_platform_init);
>> > > +
>> > > +struct regmap * __init ingenic_tcu_get_regmap(struct
>> device_node
>> > > *np)
>> > > +{
>> > > + if (!tcu_regmap)
>> > > + tcu_regmap = ingenic_tcu_create_regmap(np);
>> > > +
>> > > + return tcu_regmap;
>> > > +}
>> >
>> > This makes me pretty uncomfortable.
>> >
>> > What calls it?
>>
>> The TCU IRQ driver (patch [06/13]), clocks driver (patch [05/13]),
>> and the
>> non-OST clocksource driver (patch [07/13]) all probe very early in
>> the boot
>> process, and share the same devicetree node. They call this
>> function to get
>> a pointer to the regmap.
>
> Horrible!
>
> Instead, you should send it through platform_set_drvdata() and collect
> it in the child drivers with platform_get_drvdata(dev->parent).
The IRQ, clocks and clocksource driver do NOT have a "struct device" to
begin with. They are not platform drivers, and cannot be platform
drivers,
as they must register so early in the boot process, before "struct
device"
is even a thing.
All they get is a pointer to the same devicetree node. Since all of
these
have to use the same registers, they need to use a shared regmap, which
they obtain by calling ingenic_tcu_get_regmap() below.
Then, when this driver's probe gets called, the regmap is retrieved and
attached to the struct device, and then the children devices will be
probed: the watchdog device, the PWM device, the OST device. These three
will retrieve the regmap by calling dev_get_regmap(dev->parent, NULL).
>> > > +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned
>> int
>> > > channel)
>> > > +{
>> > > + const struct ingenic_soc_info *soc =
>> > > device_get_match_data(dev->parent);
>> > > +
>> > > + /* Enable all TCU channels for PWM use by default except
>> channels
>> > > 0/1 */
>> > > + u32 pwm_channels_mask = GENMASK(soc->num_channels - 1, 2);
>> > > +
>> > > + device_property_read_u32(dev->parent,
>> "ingenic,pwm-channels-mask",
>> > > + &pwm_channels_mask);
>
> Doesn't this call overwrite the previous assignment above?
Yes, that's intended. You have a default value, that can be overriden
by a device property.
>> > > + return !!(pwm_channels_mask & BIT(channel));
>> > > +}
>> > > +EXPORT_SYMBOL_GPL(ingenic_tcu_pwm_can_use_chn);
>
> Where is this called from?
This is called from the PWM driver.
> I think this needs a review by the DT guys.
Rob already acked the bindings, which describe this property.
>> > > diff --git a/include/linux/mfd/ingenic-tcu.h
>> > > b/include/linux/mfd/ingenic-tcu.h
>> > > index 2083fa20821d..21df23916cd2 100644
>> > > --- a/include/linux/mfd/ingenic-tcu.h
>> > > +++ b/include/linux/mfd/ingenic-tcu.h
>> > > @@ -6,6 +6,11 @@
>> > > #define __LINUX_MFD_INGENIC_TCU_H_
>> > >
>> > > #include <linux/bitops.h>
>> > > +#include <linux/init.h>
>> > > +
>> > > +struct device;
>> > > +struct device_node;
>> > > +struct regmap;
>> > >
>> > > #define TCU_REG_WDT_TDR 0x00
>> > > #define TCU_REG_WDT_TCER 0x04
>> > > @@ -53,4 +58,7 @@
>> > > #define TCU_REG_TCNTc(c) (TCU_REG_TCNT0 + ((c) *
>> > > TCU_CHANNEL_STRIDE))
>> > > #define TCU_REG_TCSRc(c) (TCU_REG_TCSR0 + ((c) *
>> > > TCU_CHANNEL_STRIDE))
>> > >
>> > > +struct regmap * __init ingenic_tcu_get_regmap(struct
>> device_node
>> > > *np);
>> > > +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned
>> int
>> > > channel);
>> > > +
>> > > #endif /* __LINUX_MFD_INGENIC_TCU_H_ */
>> >
>>
>>
>
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
On Thu, 27 Jun 2019, Paul Cercueil wrote:
> Le jeu. 27 juin 2019 à 8:58, Lee Jones <[email protected]> a écrit :
> > On Wed, 26 Jun 2019, Paul Cercueil wrote:
> > > Le mer. 26 juin 2019 à 15:18, Lee Jones <[email protected]> a
> > > écrit :
> > > > On Tue, 21 May 2019, Paul Cercueil wrote:
> > > >
> > > > > This driver will provide a regmap that can be retrieved very
> > > early
> > > > > in
> > > > > the boot process through the API function
> > > ingenic_tcu_get_regmap().
> > > > >
> > > > > Additionally, it will call devm_of_platform_populate() so that
> > > all
> > > > > the
> > > > > children devices will be probed.
> > > > >
> > > > > Signed-off-by: Paul Cercueil <[email protected]>
> > > > > ---
> > > > >
> > > > > Notes:
> > > > > v12: New patch
> > > > >
> > > > > drivers/mfd/Kconfig | 8 +++
> > > > > drivers/mfd/Makefile | 1 +
> > > > > drivers/mfd/ingenic-tcu.c | 113
> > > > > ++++++++++++++++++++++++++++++++
> > > > > include/linux/mfd/ingenic-tcu.h | 8 +++
> > > > > 4 files changed, 130 insertions(+)
> > > > > create mode 100644 drivers/mfd/ingenic-tcu.c
> >
> > [...]
> >
> > > > > +static struct regmap * __init ingenic_tcu_create_regmap(struct
> > > > > device_node *np)
> > > > > +{
> > > > > + struct resource res;
> > > > > + void __iomem *base;
> > > > > + struct regmap *map;
> > > > > +
> > > > > + if (!of_match_node(ingenic_tcu_of_match, np))
> > > > > + return ERR_PTR(-EINVAL);
> >
> > Drop this check.
> >
> > > > > + base = of_io_request_and_map(np, 0, "TCU");
> > > > > + if (IS_ERR(base))
> > > > > + return ERR_PTR(PTR_ERR(base));
> > > > > +
> > > > > + map = regmap_init_mmio(NULL, base,
> > > &ingenic_tcu_regmap_config);
> > > > > + if (IS_ERR(map))
> > > > > + goto err_iounmap;
> >
> > Place this inside probe().
> >
> > > > > + return map;
> > > > > +
> > > > > +err_iounmap:
> > > > > + iounmap(base);
> > > > > + of_address_to_resource(np, 0, &res);
> > > > > + release_mem_region(res.start, resource_size(&res));
> > > > > +
> > > > > + return map;
> > > > > +}
> > > >
> > > > Why does this need to be set-up earlier than probe()?
> > >
> > > See the explanation below.
> >
> > I think the answer is, it doesn't.
> >
> > > > > +static int __init ingenic_tcu_probe(struct platform_device
> > > *pdev)
> > > > > +{
> > > > > + struct regmap *map =
> > > ingenic_tcu_get_regmap(pdev->dev.of_node);
> > > > > +
> > > > > + platform_set_drvdata(pdev, map);
> > > > > +
> > > > > + regmap_attach_dev(&pdev->dev, map,
> > > &ingenic_tcu_regmap_config);
> > > > > +
> > > > > + return devm_of_platform_populate(&pdev->dev);
> > > > > +}
> > > > > +
> > > > > +static struct platform_driver ingenic_tcu_driver = {
> > > > > + .driver = {
> > > > > + .name = "ingenic-tcu",
> > > > > + .of_match_table = ingenic_tcu_of_match,
> > > > > + },
> > > > > +};
> > > > > +
> > > > > +static int __init ingenic_tcu_platform_init(void)
> > > > > +{
> > > > > + return platform_driver_probe(&ingenic_tcu_driver,
> > > > > + ingenic_tcu_probe);
> > > >
> > > > What? Why?
> > >
> > > The device driver probed here will populate the children devices,
> > > which will be able to retrieve the pointer to the regmap through
> > > device_get_regmap(dev->parent).
> >
> > I've never heard of this call. Where is it?
>
> dev_get_regmap, in <linux/regmap.h>.
>
> > > The children devices are normal platform drivers that can be probed
> > > the normal way. These are the PWM driver, the watchdog driver, and
> > > the
> > > OST (OS Timer) clocksource driver, all part of the same hardware
> > > block
> > > (the Timer/Counter Unit or TCU).
> >
> > If they are normal devices, then there is no need to roll your own
> > regmap-getter implementation like this.
> >
> > > > > +}
> > > > > +subsys_initcall(ingenic_tcu_platform_init);
> > > > > +
> > > > > +struct regmap * __init ingenic_tcu_get_regmap(struct
> > > device_node
> > > > > *np)
> > > > > +{
> > > > > + if (!tcu_regmap)
> > > > > + tcu_regmap = ingenic_tcu_create_regmap(np);
> > > > > +
> > > > > + return tcu_regmap;
> > > > > +}
> > > >
> > > > This makes me pretty uncomfortable.
> > > >
> > > > What calls it?
> > >
> > > The TCU IRQ driver (patch [06/13]), clocks driver (patch [05/13]),
> > > and the
> > > non-OST clocksource driver (patch [07/13]) all probe very early in
> > > the boot
> > > process, and share the same devicetree node. They call this
> > > function to get
> > > a pointer to the regmap.
> >
> > Horrible!
> >
> > Instead, you should send it through platform_set_drvdata() and collect
> > it in the child drivers with platform_get_drvdata(dev->parent).
>
> The IRQ, clocks and clocksource driver do NOT have a "struct device" to
> begin with. They are not platform drivers, and cannot be platform drivers,
> as they must register so early in the boot process, before "struct device"
> is even a thing.
>
> All they get is a pointer to the same devicetree node. Since all of these
> have to use the same registers, they need to use a shared regmap, which
> they obtain by calling ingenic_tcu_get_regmap() below.
>
> Then, when this driver's probe gets called, the regmap is retrieved and
> attached to the struct device, and then the children devices will be
> probed: the watchdog device, the PWM device, the OST device. These three
> will retrieve the regmap by calling dev_get_regmap(dev->parent, NULL).
That makes sense.
This explanation certainly belongs in the commit log.
Can you send your v14, as you intended. I will re-review it with new
eyes when you do.
> > > > > +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned
> > > int
> > > > > channel)
> > > > > +{
> > > > > + const struct ingenic_soc_info *soc =
> > > > > device_get_match_data(dev->parent);
> > > > > +
> > > > > + /* Enable all TCU channels for PWM use by default except
> > > channels
> > > > > 0/1 */
> > > > > + u32 pwm_channels_mask = GENMASK(soc->num_channels - 1, 2);
> > > > > +
> > > > > + device_property_read_u32(dev->parent,
> > > "ingenic,pwm-channels-mask",
> > > > > + &pwm_channels_mask);
> >
> > Doesn't this call overwrite the previous assignment above?
>
> Yes, that's intended. You have a default value, that can be overriden
> by a device property.
You should provide a comment here to make your intentions clear.
> > > > > + return !!(pwm_channels_mask & BIT(channel));
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(ingenic_tcu_pwm_can_use_chn);
> >
> > Where is this called from?
>
> This is called from the PWM driver.
Why can't it live in the PWM driver?
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Le jeu. 27 juin 2019 à 11:01, Lee Jones <[email protected]> a
écrit :
> On Thu, 27 Jun 2019, Paul Cercueil wrote:
>> Le jeu. 27 juin 2019 à 8:58, Lee Jones <[email protected]> a
>> écrit :
>> > On Wed, 26 Jun 2019, Paul Cercueil wrote:
>> > > Le mer. 26 juin 2019 à 15:18, Lee Jones
>> <[email protected]> a
>> > > écrit :
>> > > > On Tue, 21 May 2019, Paul Cercueil wrote:
>> > > >
>> > > > > This driver will provide a regmap that can be retrieved
>> very
>> > > early
>> > > > > in
>> > > > > the boot process through the API function
>> > > ingenic_tcu_get_regmap().
>> > > > >
>> > > > > Additionally, it will call devm_of_platform_populate() so
>> that
>> > > all
>> > > > > the
>> > > > > children devices will be probed.
>> > > > >
>> > > > > Signed-off-by: Paul Cercueil <[email protected]>
>> > > > > ---
>> > > > >
>> > > > > Notes:
>> > > > > v12: New patch
>> > > > >
>> > > > > drivers/mfd/Kconfig | 8 +++
>> > > > > drivers/mfd/Makefile | 1 +
>> > > > > drivers/mfd/ingenic-tcu.c | 113
>> > > > > ++++++++++++++++++++++++++++++++
>> > > > > include/linux/mfd/ingenic-tcu.h | 8 +++
>> > > > > 4 files changed, 130 insertions(+)
>> > > > > create mode 100644 drivers/mfd/ingenic-tcu.c
>> >
>> > [...]
>> >
>> > > > > +static struct regmap * __init
>> ingenic_tcu_create_regmap(struct
>> > > > > device_node *np)
>> > > > > +{
>> > > > > + struct resource res;
>> > > > > + void __iomem *base;
>> > > > > + struct regmap *map;
>> > > > > +
>> > > > > + if (!of_match_node(ingenic_tcu_of_match, np))
>> > > > > + return ERR_PTR(-EINVAL);
>> >
>> > Drop this check.
>> >
>> > > > > + base = of_io_request_and_map(np, 0, "TCU");
>> > > > > + if (IS_ERR(base))
>> > > > > + return ERR_PTR(PTR_ERR(base));
>> > > > > +
>> > > > > + map = regmap_init_mmio(NULL, base,
>> > > &ingenic_tcu_regmap_config);
>> > > > > + if (IS_ERR(map))
>> > > > > + goto err_iounmap;
>> >
>> > Place this inside probe().
>> >
>> > > > > + return map;
>> > > > > +
>> > > > > +err_iounmap:
>> > > > > + iounmap(base);
>> > > > > + of_address_to_resource(np, 0, &res);
>> > > > > + release_mem_region(res.start, resource_size(&res));
>> > > > > +
>> > > > > + return map;
>> > > > > +}
>> > > >
>> > > > Why does this need to be set-up earlier than probe()?
>> > >
>> > > See the explanation below.
>> >
>> > I think the answer is, it doesn't.
>> >
>> > > > > +static int __init ingenic_tcu_probe(struct
>> platform_device
>> > > *pdev)
>> > > > > +{
>> > > > > + struct regmap *map =
>> > > ingenic_tcu_get_regmap(pdev->dev.of_node);
>> > > > > +
>> > > > > + platform_set_drvdata(pdev, map);
>> > > > > +
>> > > > > + regmap_attach_dev(&pdev->dev, map,
>> > > &ingenic_tcu_regmap_config);
>> > > > > +
>> > > > > + return devm_of_platform_populate(&pdev->dev);
>> > > > > +}
>> > > > > +
>> > > > > +static struct platform_driver ingenic_tcu_driver = {
>> > > > > + .driver = {
>> > > > > + .name = "ingenic-tcu",
>> > > > > + .of_match_table = ingenic_tcu_of_match,
>> > > > > + },
>> > > > > +};
>> > > > > +
>> > > > > +static int __init ingenic_tcu_platform_init(void)
>> > > > > +{
>> > > > > + return platform_driver_probe(&ingenic_tcu_driver,
>> > > > > + ingenic_tcu_probe);
>> > > >
>> > > > What? Why?
>> > >
>> > > The device driver probed here will populate the children
>> devices,
>> > > which will be able to retrieve the pointer to the regmap
>> through
>> > > device_get_regmap(dev->parent).
>> >
>> > I've never heard of this call. Where is it?
>>
>> dev_get_regmap, in <linux/regmap.h>.
>>
>> > > The children devices are normal platform drivers that can be
>> probed
>> > > the normal way. These are the PWM driver, the watchdog driver,
>> and
>> > > the
>> > > OST (OS Timer) clocksource driver, all part of the same
>> hardware
>> > > block
>> > > (the Timer/Counter Unit or TCU).
>> >
>> > If they are normal devices, then there is no need to roll your own
>> > regmap-getter implementation like this.
>> >
>> > > > > +}
>> > > > > +subsys_initcall(ingenic_tcu_platform_init);
>> > > > > +
>> > > > > +struct regmap * __init ingenic_tcu_get_regmap(struct
>> > > device_node
>> > > > > *np)
>> > > > > +{
>> > > > > + if (!tcu_regmap)
>> > > > > + tcu_regmap = ingenic_tcu_create_regmap(np);
>> > > > > +
>> > > > > + return tcu_regmap;
>> > > > > +}
>> > > >
>> > > > This makes me pretty uncomfortable.
>> > > >
>> > > > What calls it?
>> > >
>> > > The TCU IRQ driver (patch [06/13]), clocks driver (patch
>> [05/13]),
>> > > and the
>> > > non-OST clocksource driver (patch [07/13]) all probe very
>> early in
>> > > the boot
>> > > process, and share the same devicetree node. They call this
>> > > function to get
>> > > a pointer to the regmap.
>> >
>> > Horrible!
>> >
>> > Instead, you should send it through platform_set_drvdata() and
>> collect
>> > it in the child drivers with platform_get_drvdata(dev->parent).
>>
>> The IRQ, clocks and clocksource driver do NOT have a "struct
>> device" to
>> begin with. They are not platform drivers, and cannot be platform
>> drivers,
>> as they must register so early in the boot process, before "struct
>> device"
>> is even a thing.
>>
>> All they get is a pointer to the same devicetree node. Since all of
>> these
>> have to use the same registers, they need to use a shared regmap,
>> which
>> they obtain by calling ingenic_tcu_get_regmap() below.
>>
>> Then, when this driver's probe gets called, the regmap is retrieved
>> and
>> attached to the struct device, and then the children devices will be
>> probed: the watchdog device, the PWM device, the OST device. These
>> three
>> will retrieve the regmap by calling dev_get_regmap(dev->parent,
>> NULL).
>
> That makes sense.
>
> This explanation certainly belongs in the commit log.
Right.
> Can you send your v14, as you intended. I will re-review it with new
> eyes when you do.
Could you review v13 instead? v14 will be a v13 with tiny teeny
non-code fixes (delete some newlines, replace %i with %d, and
convert the documentation from .txt to .rst).
>> > > > > +bool ingenic_tcu_pwm_can_use_chn(struct device *dev,
>> unsigned
>> > > int
>> > > > > channel)
>> > > > > +{
>> > > > > + const struct ingenic_soc_info *soc =
>> > > > > device_get_match_data(dev->parent);
>> > > > > +
>> > > > > + /* Enable all TCU channels for PWM use by default except
>> > > channels
>> > > > > 0/1 */
>> > > > > + u32 pwm_channels_mask = GENMASK(soc->num_channels - 1,
>> 2);
>> > > > > +
>> > > > > + device_property_read_u32(dev->parent,
>> > > "ingenic,pwm-channels-mask",
>> > > > > + &pwm_channels_mask);
>> >
>> > Doesn't this call overwrite the previous assignment above?
>>
>> Yes, that's intended. You have a default value, that can be
>> overriden
>> by a device property.
>
> You should provide a comment here to make your intentions clear.
Ok.
>> > > > > + return !!(pwm_channels_mask & BIT(channel));
>> > > > > +}
>> > > > > +EXPORT_SYMBOL_GPL(ingenic_tcu_pwm_can_use_chn);
>> >
>> > Where is this called from?
>>
>> This is called from the PWM driver.
>
> Why can't it live in the PWM driver?
It totally can. I'll move it there.
>
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog