[3/9] clk: sunxi: Add prcm mod0 clock driver

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 <hdegoede@redhat.com>
---
 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

Hi,

On Thu, Nov 20, 2014 at 7:55 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> 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 <hdegoede@redhat.com>
> ---
>  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
>
> diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt
> index ed116df..342c75a 100644
> --- a/Documentation/devicetree/bindings/clock/sunxi.txt
> +++ b/Documentation/devicetree/bindings/clock/sunxi.txt
> @@ -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.
> diff --git a/drivers/clk/sunxi/Makefile b/drivers/clk/sunxi/Makefile
> index 7ddc2b5..daf8b1c 100644
> --- a/drivers/clk/sunxi/Makefile
> +++ b/drivers/clk/sunxi/Makefile
> @@ -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
> diff --git a/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c b/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c
> new file mode 100644
> index 0000000..e80f18e
> --- /dev/null
> +++ b/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c
> @@ -0,0 +1,63 @@
> +/*
> + * Allwinner A31 PRCM mod0 clock driver
> + *
> + * Copyright (C) 2014 Hans de Goede <hdegoede@redhat.com>
> + *
> + * 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" },

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.

Same for the DT patches.

ChenYu

> +       { /* 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 <hdegoede@redhat.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/mfd/sun6i-prcm.c b/drivers/mfd/sun6i-prcm.c
> index 283ab8d..ff1254f 100644
> --- a/drivers/mfd/sun6i-prcm.c
> +++ b/drivers/mfd/sun6i-prcm.c
> @@ -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@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi,

On 11/20/2014 07:24 PM, Chen-Yu Tsai wrote:
> Hi,
> 
> On Thu, Nov 20, 2014 at 7:55 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> 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 <hdegoede@redhat.com>
>> ---
>>  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
>>
>> diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt
>> index ed116df..342c75a 100644
>> --- a/Documentation/devicetree/bindings/clock/sunxi.txt
>> +++ b/Documentation/devicetree/bindings/clock/sunxi.txt
>> @@ -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.
>> diff --git a/drivers/clk/sunxi/Makefile b/drivers/clk/sunxi/Makefile
>> index 7ddc2b5..daf8b1c 100644
>> --- a/drivers/clk/sunxi/Makefile
>> +++ b/drivers/clk/sunxi/Makefile
>> @@ -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
>> diff --git a/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c b/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c
>> new file mode 100644
>> index 0000000..e80f18e
>> --- /dev/null
>> +++ b/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c
>> @@ -0,0 +1,63 @@
>> +/*
>> + * Allwinner A31 PRCM mod0 clock driver
>> + *
>> + * Copyright (C) 2014 Hans de Goede <hdegoede@redhat.com>
>> + *
>> + * 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" },
> 
> 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.

I wish we could use a generic name, but that does not work for mfd device
subnodes, as the mfd framework attaches resources (such as registers) to
the subnodes based on the compatible.

BTW it seems that that the 1-wire clock is not 100% mod0 clock compatible,
at least the ccmu.h in the allwinner SDK uses a different struct definition
for it.

Regards,

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

Hi,

On Thu, Nov 20, 2014 at 04:55:22PM +0100, Hans de Goede wrote:
> 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 <hdegoede@redhat.com>
> ---
>  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
> 
> diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt
> index ed116df..342c75a 100644
> --- a/Documentation/devicetree/bindings/clock/sunxi.txt
> +++ b/Documentation/devicetree/bindings/clock/sunxi.txt
> @@ -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.
> diff --git a/drivers/clk/sunxi/Makefile b/drivers/clk/sunxi/Makefile
> index 7ddc2b5..daf8b1c 100644
> --- a/drivers/clk/sunxi/Makefile
> +++ b/drivers/clk/sunxi/Makefile
> @@ -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
> diff --git a/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c b/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c
> new file mode 100644
> index 0000000..e80f18e
> --- /dev/null
> +++ b/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c
> @@ -0,0 +1,63 @@
> +/*
> + * Allwinner A31 PRCM mod0 clock driver
> + *
> + * Copyright (C) 2014 Hans de Goede <hdegoede@redhat.com>
> + *
> + * 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 = {
> +		.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 <hdegoede@redhat.com>");
> +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.

Maxime

Hi,

On 11/21/2014 09:49 AM, Maxime Ripard wrote:
> Hi,
> 
> On Thu, Nov 20, 2014 at 04:55:22PM +0100, Hans de Goede wrote:
>> 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 <hdegoede@redhat.com>
>> ---
>>  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
>>
>> diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt
>> index ed116df..342c75a 100644
>> --- a/Documentation/devicetree/bindings/clock/sunxi.txt
>> +++ b/Documentation/devicetree/bindings/clock/sunxi.txt
>> @@ -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.
>> diff --git a/drivers/clk/sunxi/Makefile b/drivers/clk/sunxi/Makefile
>> index 7ddc2b5..daf8b1c 100644
>> --- a/drivers/clk/sunxi/Makefile
>> +++ b/drivers/clk/sunxi/Makefile
>> @@ -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
>> diff --git a/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c b/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c
>> new file mode 100644
>> index 0000000..e80f18e
>> --- /dev/null
>> +++ b/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c
>> @@ -0,0 +1,63 @@
>> +/*
>> + * Allwinner A31 PRCM mod0 clock driver
>> + *
>> + * Copyright (C) 2014 Hans de Goede <hdegoede@redhat.com>
>> + *
>> + * 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 = {
>> +		.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 <hdegoede@redhat.com>");
>> +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.

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.

> 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.

Regards,

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

On Fri, Nov 21, 2014 at 10:13:10AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 11/21/2014 09:49 AM, Maxime Ripard wrote:
> > Hi,
> > 
> > On Thu, Nov 20, 2014 at 04:55:22PM +0100, Hans de Goede wrote:
> >> 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 <hdegoede@redhat.com>
> >> ---
> >>  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
> >>
> >> diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt
> >> index ed116df..342c75a 100644
> >> --- a/Documentation/devicetree/bindings/clock/sunxi.txt
> >> +++ b/Documentation/devicetree/bindings/clock/sunxi.txt
> >> @@ -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.
> >> diff --git a/drivers/clk/sunxi/Makefile b/drivers/clk/sunxi/Makefile
> >> index 7ddc2b5..daf8b1c 100644
> >> --- a/drivers/clk/sunxi/Makefile
> >> +++ b/drivers/clk/sunxi/Makefile
> >> @@ -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
> >> diff --git a/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c b/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c
> >> new file mode 100644
> >> index 0000000..e80f18e
> >> --- /dev/null
> >> +++ b/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c
> >> @@ -0,0 +1,63 @@
> >> +/*
> >> + * Allwinner A31 PRCM mod0 clock driver
> >> + *
> >> + * Copyright (C) 2014 Hans de Goede <hdegoede@redhat.com>
> >> + *
> >> + * 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 = {
> >> +		.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 <hdegoede@redhat.com>");
> >> +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.
> 
> 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.

> 	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.

> 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.

> So sorry, I disagree I believe that this is the best solution.
> 
> > 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.

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

Maxime

Hi,

On 11/24/2014 11:03 PM, Maxime Ripard wrote:
> On Fri, Nov 21, 2014 at 10:13:10AM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 11/21/2014 09:49 AM, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Thu, Nov 20, 2014 at 04:55:22PM +0100, Hans de Goede wrote:
>>>> 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 <hdegoede@redhat.com>
>>>> ---
>>>>   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
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt
>>>> index ed116df..342c75a 100644
>>>> --- a/Documentation/devicetree/bindings/clock/sunxi.txt
>>>> +++ b/Documentation/devicetree/bindings/clock/sunxi.txt
>>>> @@ -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.
>>>> diff --git a/drivers/clk/sunxi/Makefile b/drivers/clk/sunxi/Makefile
>>>> index 7ddc2b5..daf8b1c 100644
>>>> --- a/drivers/clk/sunxi/Makefile
>>>> +++ b/drivers/clk/sunxi/Makefile
>>>> @@ -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
>>>> diff --git a/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c b/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c
>>>> new file mode 100644
>>>> index 0000000..e80f18e
>>>> --- /dev/null
>>>> +++ b/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c
>>>> @@ -0,0 +1,63 @@
>>>> +/*
>>>> + * Allwinner A31 PRCM mod0 clock driver
>>>> + *
>>>> + * Copyright (C) 2014 Hans de Goede <hdegoede@redhat.com>
>>>> + *
>>>> + * 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 = {
>>>> +		.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 <hdegoede@redhat.com>");
>>>> +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.
>>
>> 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.

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.

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

Well AFAIK that is how it works.

>> 	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.

>> 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.

>> So sorry, I disagree I believe that this is the best solution.
>>
>>> 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.
>
> 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...

Regards,

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

Hi,

On 11/25/2014 09:29 AM, Hans de Goede wrote:

<snip>

> 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...

Thinking more about this one alternative which should work is to just put the
clocks in the prcm in the clocks node, then they get their own reg property,
rather then being part of the prcm reg range, and the standard of_clk mod0
driver we have will just work.

Regards,

Hans

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

Hi,

On Tue, Nov 25, 2014 at 09:29:21AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 11/24/2014 11:03 PM, Maxime Ripard wrote:
> >On Fri, Nov 21, 2014 at 10:13:10AM +0100, Hans de Goede wrote:
> >>Hi,
> >>
> >>On 11/21/2014 09:49 AM, Maxime Ripard wrote:
> >>>Hi,
> >>>
> >>>On Thu, Nov 20, 2014 at 04:55:22PM +0100, Hans de Goede wrote:
> >>>>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 <hdegoede@redhat.com>
> >>>>---
> >>>>  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
> >>>>
> >>>>diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt
> >>>>index ed116df..342c75a 100644
> >>>>--- a/Documentation/devicetree/bindings/clock/sunxi.txt
> >>>>+++ b/Documentation/devicetree/bindings/clock/sunxi.txt
> >>>>@@ -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.
> >>>>diff --git a/drivers/clk/sunxi/Makefile b/drivers/clk/sunxi/Makefile
> >>>>index 7ddc2b5..daf8b1c 100644
> >>>>--- a/drivers/clk/sunxi/Makefile
> >>>>+++ b/drivers/clk/sunxi/Makefile
> >>>>@@ -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
> >>>>diff --git a/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c b/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c
> >>>>new file mode 100644
> >>>>index 0000000..e80f18e
> >>>>--- /dev/null
> >>>>+++ b/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c
> >>>>@@ -0,0 +1,63 @@
> >>>>+/*
> >>>>+ * Allwinner A31 PRCM mod0 clock driver
> >>>>+ *
> >>>>+ * Copyright (C) 2014 Hans de Goede <hdegoede@redhat.com>
> >>>>+ *
> >>>>+ * 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 = {
> >>>>+		.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 <hdegoede@redhat.com>");
> >>>>+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.
> >>
> >>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.
> 
> 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.

> >Plus, that seems like a bogus limitation from MFD, and it really
> >shouldn't work that way.
> 
> 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.

> >>	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.

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?

> >>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.
> 
> >>So sorry, I disagree I believe that this is the best solution.
> >>
> >>>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.
> >
> >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.

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.

> 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.

You're the one being unhappy with EPROBE_DEFER, not me.

Maxime

Hi,

On 11/26/2014 10:13 PM, Maxime Ripard wrote:
> Hi,
>
> On Tue, Nov 25, 2014 at 09:29:21AM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 11/24/2014 11:03 PM, Maxime Ripard wrote:
>>> On Fri, Nov 21, 2014 at 10:13:10AM +0100, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 11/21/2014 09:49 AM, Maxime Ripard wrote:
>>>>> Hi,
>>>>>
>>>>> On Thu, Nov 20, 2014 at 04:55:22PM +0100, Hans de Goede wrote:
>>>>>> 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 <hdegoede@redhat.com>
>>>>>> ---
>>>>>>   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
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt
>>>>>> index ed116df..342c75a 100644
>>>>>> --- a/Documentation/devicetree/bindings/clock/sunxi.txt
>>>>>> +++ b/Documentation/devicetree/bindings/clock/sunxi.txt
>>>>>> @@ -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.
>>>>>> diff --git a/drivers/clk/sunxi/Makefile b/drivers/clk/sunxi/Makefile
>>>>>> index 7ddc2b5..daf8b1c 100644
>>>>>> --- a/drivers/clk/sunxi/Makefile
>>>>>> +++ b/drivers/clk/sunxi/Makefile
>>>>>> @@ -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
>>>>>> diff --git a/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c b/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c
>>>>>> new file mode 100644
>>>>>> index 0000000..e80f18e
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c
>>>>>> @@ -0,0 +1,63 @@
>>>>>> +/*
>>>>>> + * Allwinner A31 PRCM mod0 clock driver
>>>>>> + *
>>>>>> + * Copyright (C) 2014 Hans de Goede <hdegoede@redhat.com>
>>>>>> + *
>>>>>> + * 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 = {
>>>>>> +		.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 <hdegoede@redhat.com>");
>>>>>> +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.
>>>>
>>>> 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.
>>
>> 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.
>
>>> Plus, that seems like a bogus limitation from MFD, and it really
>>> shouldn't work that way.
>>
>> 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:

>>>> 	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.



>>>>
>>>> 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... ?

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

> 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?
>
>>>> 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.
>>
>>>> So sorry, I disagree I believe that this is the best solution.
>>>>
>>>>> 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.
>>>
>>> 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.
>
> 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.

>> 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.

Regards,

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

Hi,

On Thu, Nov 27, 2014 at 4:41 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 11/26/2014 10:13 PM, Maxime Ripard wrote:
>>
>> Hi,
>>
>> On Tue, Nov 25, 2014 at 09:29:21AM +0100, Hans de Goede wrote:
>>>
>>> Hi,
>>>
>>> On 11/24/2014 11:03 PM, Maxime Ripard wrote:
>>>>
>>>> On Fri, Nov 21, 2014 at 10:13:10AM +0100, Hans de Goede wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 11/21/2014 09:49 AM, Maxime Ripard wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On Thu, Nov 20, 2014 at 04:55:22PM +0100, Hans de Goede wrote:
>>>>>>>
>>>>>>> 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 <hdegoede@redhat.com>
>>>>>>> ---
>>>>>>>   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
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt
>>>>>>> b/Documentation/devicetree/bindings/clock/sunxi.txt
>>>>>>> index ed116df..342c75a 100644
>>>>>>> --- a/Documentation/devicetree/bindings/clock/sunxi.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/clock/sunxi.txt
>>>>>>> @@ -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.
>>>>>>> diff --git a/drivers/clk/sunxi/Makefile b/drivers/clk/sunxi/Makefile
>>>>>>> index 7ddc2b5..daf8b1c 100644
>>>>>>> --- a/drivers/clk/sunxi/Makefile
>>>>>>> +++ b/drivers/clk/sunxi/Makefile
>>>>>>> @@ -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
>>>>>>> diff --git a/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c
>>>>>>> b/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c
>>>>>>> new file mode 100644
>>>>>>> index 0000000..e80f18e
>>>>>>> --- /dev/null
>>>>>>> +++ b/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c
>>>>>>> @@ -0,0 +1,63 @@
>>>>>>> +/*
>>>>>>> + * Allwinner A31 PRCM mod0 clock driver
>>>>>>> + *
>>>>>>> + * Copyright (C) 2014 Hans de Goede <hdegoede@redhat.com>
>>>>>>> + *
>>>>>>> + * 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 = {
>>>>>>> +               .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 <hdegoede@redhat.com>");
>>>>>>> +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.
>>>>>
>>>>>
>>>>> 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.
>>>
>>>
>>> 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.
>>
>>>> Plus, that seems like a bogus limitation from MFD, and it really
>>>> shouldn't work that way.
>>>
>>>
>>> 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:
>
>>>>>         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.
>
>
>
>>>>>
>>>>> 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... ?
>
>
> That they are child nodes of a clocks node, which is NOT simple-bus
> compatible
> and get instantiated early using CLK_OF_DECLARE.
>
>
>> 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?
>>
>>>>> 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.
>>>
>>>>> So sorry, I disagree I believe that this is the best solution.
>>>>>
>>>>>> 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.
>>>>
>>>>
>>>> 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.
>>
>>
>> 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.
>
>>> 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.

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.

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

Hi,

On 11/27/2014 10:28 AM, Chen-Yu Tsai wrote:
> Hi,
>
> On Thu, Nov 27, 2014 at 4:41 PM, Hans de Goede <hdegoede@redhat.com> wrote:

<snip>

>> 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.

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.

Regards,

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

Hi,

On Wed, 26 Nov 2014 22:13:18 +0100
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

[...]

> 
> 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

Best Regards,

Boris

On Thu, Nov 27, 2014 at 09:41:09AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 11/26/2014 10:13 PM, Maxime Ripard wrote:
> >Hi,
> >
> >On Tue, Nov 25, 2014 at 09:29:21AM +0100, Hans de Goede wrote:
> >>Hi,
> >>
> >>On 11/24/2014 11:03 PM, Maxime Ripard wrote:
> >>>On Fri, Nov 21, 2014 at 10:13:10AM +0100, Hans de Goede wrote:
> >>>>Hi,
> >>>>
> >>>>On 11/21/2014 09:49 AM, Maxime Ripard wrote:
> >>>>>Hi,
> >>>>>
> >>>>>On Thu, Nov 20, 2014 at 04:55:22PM +0100, Hans de Goede wrote:
> >>>>>>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 <hdegoede@redhat.com>
> >>>>>>---
> >>>>>>  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
> >>>>>>
> >>>>>>diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt
> >>>>>>index ed116df..342c75a 100644
> >>>>>>--- a/Documentation/devicetree/bindings/clock/sunxi.txt
> >>>>>>+++ b/Documentation/devicetree/bindings/clock/sunxi.txt
> >>>>>>@@ -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.
> >>>>>>diff --git a/drivers/clk/sunxi/Makefile b/drivers/clk/sunxi/Makefile
> >>>>>>index 7ddc2b5..daf8b1c 100644
> >>>>>>--- a/drivers/clk/sunxi/Makefile
> >>>>>>+++ b/drivers/clk/sunxi/Makefile
> >>>>>>@@ -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
> >>>>>>diff --git a/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c b/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c
> >>>>>>new file mode 100644
> >>>>>>index 0000000..e80f18e
> >>>>>>--- /dev/null
> >>>>>>+++ b/drivers/clk/sunxi/clk-sun6i-prcm-mod0.c
> >>>>>>@@ -0,0 +1,63 @@
> >>>>>>+/*
> >>>>>>+ * Allwinner A31 PRCM mod0 clock driver
> >>>>>>+ *
> >>>>>>+ * Copyright (C) 2014 Hans de Goede <hdegoede@redhat.com>
> >>>>>>+ *
> >>>>>>+ * 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 = {
> >>>>>>+		.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 <hdegoede@redhat.com>");
> >>>>>>+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.
> >>>>
> >>>>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.
> >>
> >>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.
> >
> >>>Plus, that seems like a bogus limitation from MFD, and it really
> >>>shouldn't work that way.
> >>
> >>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:
> 
> >>>>	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.

That won't cause any early binding if you drop the CLK_OF_DECLARE and
have only a regular platform driver,  which was what I've been
suggesting all along.

> >>>>
> >>>>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... ?
> 
> That they are child nodes of a clocks node, which is NOT simple-bus compatible
> and get instantiated early using CLK_OF_DECLARE.

Depending on who you ask to, this might or might not be true. Mark
Rutland for example has been a strong opponent to that clock node. So
your it's-by-design argument is just flawed.

CLK_OF_DECLARE has been designed to be able to parse the DT early to
instantiate the early clocks. It doesn't do anything beyond that. The
clocks node has nothing to do with that whole discussion.

> >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?
> >
> >>>>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.
> >>
> >>>>So sorry, I disagree I believe that this is the best solution.
> >>>>
> >>>>>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.
> >>>
> >>>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.
> >
> >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.

Not really. Some others clocks that can be probed by MFD are already
platform drivers.

> >>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.

What? It's exactly what your patch is doing. How is that more work?

> 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.

And then you're talking about inconsistencies.... Having some of these
clocks and devices exposed through MFD and some others through clocks
is very consistent indeed.

Maxime

Hi,

On Thu, Nov 27, 2014 at 11:10:51AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 11/27/2014 10:28 AM, Chen-Yu Tsai wrote:
> >Hi,
> >
> >On Thu, Nov 27, 2014 at 4:41 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> 
> <snip>
> 
> >>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.
> 
> 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.

> 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.

And also because the 1 node per clock is no longer the current trend
and that Mike discourages to use that model nowadays.

> 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.

Have you considered the SMP code in that smooth plan?

The one that is in neither a platform driver, nor a clock, nor does it
have a compatible of its own, but still needs to access some of the
PRCM registers?

Maxime

On Thu, Nov 27, 2014 at 05:40:56PM +0100, Boris Brezillon wrote:
> Hi,
> 
> On Wed, 26 Nov 2014 22:13:18 +0100
> Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> 
> [...]
> 
> > 
> > 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 ?

There's (at least) one module0 clock exposed in the PRCM block. We
have a disagreement on whether all module0 clocks should be platform
drivers to support probing that one clock or just to introduce a new
compatible for that one clock in the PRCM alone.

> 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

We don't really need to share anything, these components are isolated
in separate registers, so syscon doesn't really bring anything here.

Maxime

Hi,

On Fri, Nov 28, 2014 at 02:37:14PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 11/27/2014 08:05 PM, Maxime Ripard wrote:
> >Hi,
> >
> >On Thu, Nov 27, 2014 at 11:10:51AM +0100, Hans de Goede wrote:
> >>Hi,
> >>
> >>On 11/27/2014 10:28 AM, Chen-Yu Tsai wrote:
> >>>Hi,
> >>>
> >>>On Thu, Nov 27, 2014 at 4:41 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >><snip>
> >>
> >>>>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.
> >>
> >>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.
> >
> >>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.
> 
> 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.

> 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.

> 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.

> 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.

> 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 <hdegoede@redhat.com>
> 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 <hdegoede@redhat.com>
> ---
>  arch/arm/boot/dts/sun6i-a31.dtsi | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
> index 29e6438..4b8541f 100644
> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
> @@ -846,11 +846,15 @@
>  		};
>  
>  		prcm@01f01400 {
> -			compatible = "allwinner,sun6i-a31-prcm";
> +			compatible = "simple-bus";

And this breaks the SMP bringup code.

Maxime

Hi,

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

 >> 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.

>> 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.
>
>> 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.

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.

>> 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.

Ok, but that does bring us back to the original problem wrt the ir-clk,
see above for how I think we should solve this then. If you agree I
can implement the proposed fix.

Regards,

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

On Wed, Dec 03, 2014 at 10:49:20AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 12/02/2014 04:45 PM, Maxime Ripard wrote:
> 
> >> 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.

Yes. And the obvious solution to that is to just not use
OF_CLK_DECLARE but a platform_driver, which is totally fine.

> 
> >>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.
> >
> >>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.
> 
> Ok, I can live with that, but likewise the clocks node is not a bus either!

True, and just like we just saw with Chen-Yu patches, it can even leak
implementation details. Which was one of the arguments from Mark
against it iirc.

> 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.

> 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.

> 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.

> 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.

> 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.

I really think having a different compatible is both wrong on a
theoritical point of view and doesn't bring anything to the table now
(and might never do)

Maxime

Hi,

On 07-12-14 19:08, Maxime Ripard wrote:
> On Wed, Dec 03, 2014 at 10:49:20AM +0100, Hans de Goede wrote:

<snip>

>> 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.
>
>> 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.
>
>> 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.
>
>> 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 :)

Regards,

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

On Mon, Dec 08, 2014 at 09:19:02AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 07-12-14 19:08, Maxime Ripard wrote:
> >On Wed, Dec 03, 2014 at 10:49:20AM +0100, Hans de Goede wrote:
> 
> <snip>
> 
> >>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.
> >
> >>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.
> >
> >>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.
> >
> >>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 :)

Indeed :)

Thanks!
Maxime