[PATCH] clk: clk-gpio: Add dt option to propagate rate change to parent

[PATCH] clk: clk-gpio: Add dt option to propagate rate change to parent

[Alexandru Ardelean]

From: Michael Hennerich <michael.hennerich@analog.com>

For certain setups/boards it's useful to propagate the rate change of the
clock up one level to the parent clock.

This change implements this by defining a `clk-set-rate-parent` device-tree
property which sets the `CLK_SET_RATE_PARENT` flag to the clock (when set).

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/clk/clk-gpio.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
index 9d930edd6516..6dfbc4b952fe 100644
--- a/drivers/clk/clk-gpio.c
+++ b/drivers/clk/clk-gpio.c
@@ -241,6 +241,7 @@ static int gpio_clk_driver_probe(struct platform_device *pdev)
 	struct device_node *node = pdev->dev.of_node;
 	const char **parent_names, *gpio_name;
 	unsigned int num_parents;
+	unsigned long clk_flags;
 	struct gpio_desc *gpiod;
 	struct clk *clk;
 	bool is_mux;
@@ -274,13 +275,16 @@ static int gpio_clk_driver_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	clk_flags = of_property_read_bool(node, "clk-set-rate-parent") ?
+			CLK_SET_RATE_PARENT : 0;
+
 	if (is_mux)
 		clk = clk_register_gpio_mux(&pdev->dev, node->name,
-				parent_names, num_parents, gpiod, 0);
+				parent_names, num_parents, gpiod, clk_flags);
 	else
 		clk = clk_register_gpio_gate(&pdev->dev, node->name,
 				parent_names ?  parent_names[0] : NULL, gpiod,
-				0);
+				clk_flags);
 	if (IS_ERR(clk))
 		return PTR_ERR(clk);
 
-- 
2.20.1

Re: [PATCH] clk: clk-gpio: Add dt option to propagate rate change to parent

[Stephen Boyd]

Quoting Alexandru Ardelean (2019-11-06 03:35:51)
> From: Michael Hennerich <michael.hennerich@analog.com>
> 
> For certain setups/boards it's useful to propagate the rate change of the
> clock up one level to the parent clock.
> 
> This change implements this by defining a `clk-set-rate-parent` device-tree
> property which sets the `CLK_SET_RATE_PARENT` flag to the clock (when set).
> 
> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/clk/clk-gpio.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
> index 9d930edd6516..6dfbc4b952fe 100644
> --- a/drivers/clk/clk-gpio.c
> +++ b/drivers/clk/clk-gpio.c
> @@ -241,6 +241,7 @@ static int gpio_clk_driver_probe(struct platform_device *pdev)
>         struct device_node *node = pdev->dev.of_node;
>         const char **parent_names, *gpio_name;
>         unsigned int num_parents;
> +       unsigned long clk_flags;
>         struct gpio_desc *gpiod;
>         struct clk *clk;
>         bool is_mux;
> @@ -274,13 +275,16 @@ static int gpio_clk_driver_probe(struct platform_device *pdev)
>                 return ret;
>         }
>  
> +       clk_flags = of_property_read_bool(node, "clk-set-rate-parent") ?
> +                       CLK_SET_RATE_PARENT : 0;

Is there a DT binding update somewhere? It looks like a linux-ism from
the DT perspective. I wonder if we can somehow figure out that it's OK
to call clk_set_rate() on the parent here? Or is it safe to assume that
we can just always call set rate on the parent? I think for a gate it's
good and we can just do so, but for a mux maybe not. Care to describe
your scenario a little more so we can understand why you want to set
this flag? Is it for a mux or a gate type gpio?

Re: [PATCH] clk: clk-gpio: Add dt option to propagate rate change to parent

[Ardelean, Alexandru]

On Wed, 2019-11-06 at 14:43 -0800, Stephen Boyd wrote:
> Quoting Alexandru Ardelean (2019-11-06 03:35:51)
> > From: Michael Hennerich <michael.hennerich@analog.com>
> > 
> > For certain setups/boards it's useful to propagate the rate change of
> > the
> > clock up one level to the parent clock.
> > 
> > This change implements this by defining a `clk-set-rate-parent` device-
> > tree
> > property which sets the `CLK_SET_RATE_PARENT` flag to the clock (when
> > set).
> > 
> > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> >  drivers/clk/clk-gpio.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
> > index 9d930edd6516..6dfbc4b952fe 100644
> > --- a/drivers/clk/clk-gpio.c
> > +++ b/drivers/clk/clk-gpio.c
> > @@ -241,6 +241,7 @@ static int gpio_clk_driver_probe(struct
> > platform_device *pdev)
> >         struct device_node *node = pdev->dev.of_node;
> >         const char **parent_names, *gpio_name;
> >         unsigned int num_parents;
> > +       unsigned long clk_flags;
> >         struct gpio_desc *gpiod;
> >         struct clk *clk;
> >         bool is_mux;
> > @@ -274,13 +275,16 @@ static int gpio_clk_driver_probe(struct
> > platform_device *pdev)
> >                 return ret;
> >         }
> >  
> > +       clk_flags = of_property_read_bool(node, "clk-set-rate-parent")
> > ?
> > +                       CLK_SET_RATE_PARENT : 0;
> 
> Is there a DT binding update somewhere? It looks like a linux-ism from

Good point. I did not think about the DT, and I guess I didn't search it
thoroughly enough. Found DT files now.

> the DT perspective. I wonder if we can somehow figure out that it's OK
> to call clk_set_rate() on the parent here? Or is it safe to assume that
> we can just always call set rate on the parent? I think for a gate it's
> good and we can just do so, but for a mux maybe not. Care to describe
> your scenario a little more so we can understand why you want to set
> this flag? Is it for a mux or a gate type gpio?
> 

For our case we are using it here [with a slight name variation in the prop
name]:
https://github.com/analogdevicesinc/linux/blob/master/arch/arm/boot/dts/zynq-adrv9361-z7035.dtsi#L43

And on this board:
https://wiki.analog.com/resources/eval/user-guides/adrv936x_rfsom/user-guide/introduction

It's for a gate-type GPIO.
The clock defined in that DT (ad9361_clkin) is attached to a clock that has
a fixed rate (xo_40mhz_fixed_clk), that can become a variable rate [from an
external source, when it is provided].

Our understanding, is that a GPIO gate clock should propagate the rate
change to the parent clock. The same goes for the GPIO MUX clock.
So, the default mode would be to always set CLK_SET_RATE_PARENT.

But, given that there are users of this driver, such a behavior change
could break other users, so we are using this DT prop.

There seems to be only one user of gpio-mux-clock in
arch/arm64/boot/dts/renesas/ulcb-kf.dtsi

Whereas for gpio-gate-clock, there are multiple users. I can't say whether
this change would break anything for them, or it would be fine.

Re: [PATCH] clk: clk-gpio: Add dt option to propagate rate change to parent

[Ardelean, Alexandru]

On Thu, 2019-11-07 at 14:53 -0800, Stephen Boyd wrote:
> Quoting Ardelean, Alexandru (2019-11-07 05:25:38)
> > On Wed, 2019-11-06 at 14:43 -0800, Stephen Boyd wrote:
> > > Quoting Alexandru Ardelean (2019-11-06 03:35:51)
> > > > From: Michael Hennerich <michael.hennerich@analog.com>
> > > > 
> > > > For certain setups/boards it's useful to propagate the rate change
> > > > of
> > > > the
> > > > clock up one level to the parent clock.
> > > > 
> > > > This change implements this by defining a `clk-set-rate-parent`
> > > > device-
> > > > tree
> > > > property which sets the `CLK_SET_RATE_PARENT` flag to the clock
> > > > (when
> > > > set).
> > > > 
> > > > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > > ---
> > > >  drivers/clk/clk-gpio.c | 8 ++++++--
> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
> > > > index 9d930edd6516..6dfbc4b952fe 100644
> > > > --- a/drivers/clk/clk-gpio.c
> > > > +++ b/drivers/clk/clk-gpio.c
> > > > @@ -241,6 +241,7 @@ static int gpio_clk_driver_probe(struct
> > > > platform_device *pdev)
> > > >         struct device_node *node = pdev->dev.of_node;
> > > >         const char **parent_names, *gpio_name;
> > > >         unsigned int num_parents;
> > > > +       unsigned long clk_flags;
> > > >         struct gpio_desc *gpiod;
> > > >         struct clk *clk;
> > > >         bool is_mux;
> > > > @@ -274,13 +275,16 @@ static int gpio_clk_driver_probe(struct
> > > > platform_device *pdev)
> > > >                 return ret;
> > > >         }
> > > >  
> > > > +       clk_flags = of_property_read_bool(node, "clk-set-rate-
> > > > parent")
> > > > ?
> > > > +                       CLK_SET_RATE_PARENT : 0;
> > > 
> > > Is there a DT binding update somewhere? It looks like a linux-ism
> > > from
> > 
> > Good point. I did not think about the DT, and I guess I didn't search
> > it
> > thoroughly enough. Found DT files now.
> > 
> > > the DT perspective. I wonder if we can somehow figure out that it's
> > > OK
> > > to call clk_set_rate() on the parent here? Or is it safe to assume
> > > that
> > > we can just always call set rate on the parent? I think for a gate
> > > it's
> > > good and we can just do so, but for a mux maybe not. Care to describe
> > > your scenario a little more so we can understand why you want to set
> > > this flag? Is it for a mux or a gate type gpio?
> > > 
> > 
> > For our case we are using it here [with a slight name variation in the
> > prop
> > name]:
> > https://github.com/analogdevicesinc/linux/blob/master/arch/arm/boot/dts/zynq-adrv9361-z7035.dtsi#L43
> > 
> > And on this board:
> > https://wiki.analog.com/resources/eval/user-guides/adrv936x_rfsom/user-guide/introduction
> > 
> > It's for a gate-type GPIO.
> > The clock defined in that DT (ad9361_clkin) is attached to a clock that
> > has
> > a fixed rate (xo_40mhz_fixed_clk), that can become a variable rate
> > [from an
> > external source, when it is provided].
> > 
> > Our understanding, is that a GPIO gate clock should propagate the rate
> > change to the parent clock. The same goes for the GPIO MUX clock.
> > So, the default mode would be to always set CLK_SET_RATE_PARENT.
> > 
> > But, given that there are users of this driver, such a behavior change
> > could break other users, so we are using this DT prop.
> > 
> > There seems to be only one user of gpio-mux-clock in
> > arch/arm64/boot/dts/renesas/ulcb-kf.dtsi
> > 
> > Whereas for gpio-gate-clock, there are multiple users. I can't say
> > whether
> > this change would break anything for them, or it would be fine.
> 
> I think we can just add CLK_SET_RATE_PARENT for all gpio gate clks then
> and you'll be happy? Care to send that patch and Cc Geert and other
> Renesas maintainers? That avoids any DT property and should be fine for
> anyone as far as I can tell. Maybe the inverse will be desired, but not
> wanting rate to propagate is an edge case that we may want to call out
> explicitly in DT at some point in the future, but shouldn't worry about
> until then.
> 

Will resend, thanks.
Geert is CC-ed here as well [unless there is another Geert].

I'll send a V2 in-reply-to this thread.

Thanks
Alex

Re: [PATCH] clk: clk-gpio: Add dt option to propagate rate change to parent

[Ardelean, Alexandru]

On Fri, 2019-11-08 at 06:50 +0000, Ardelean, Alexandru wrote:
> [External]
> 
> On Thu, 2019-11-07 at 14:53 -0800, Stephen Boyd wrote:
> > Quoting Ardelean, Alexandru (2019-11-07 05:25:38)
> > > On Wed, 2019-11-06 at 14:43 -0800, Stephen Boyd wrote:
> > > > Quoting Alexandru Ardelean (2019-11-06 03:35:51)
> > > > > From: Michael Hennerich <michael.hennerich@analog.com>
> > > > > 
> > > > > For certain setups/boards it's useful to propagate the rate
> > > > > change
> > > > > of
> > > > > the
> > > > > clock up one level to the parent clock.
> > > > > 
> > > > > This change implements this by defining a `clk-set-rate-parent`
> > > > > device-
> > > > > tree
> > > > > property which sets the `CLK_SET_RATE_PARENT` flag to the clock
> > > > > (when
> > > > > set).
> > > > > 
> > > > > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > > > ---
> > > > >  drivers/clk/clk-gpio.c | 8 ++++++--
> > > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
> > > > > index 9d930edd6516..6dfbc4b952fe 100644
> > > > > --- a/drivers/clk/clk-gpio.c
> > > > > +++ b/drivers/clk/clk-gpio.c
> > > > > @@ -241,6 +241,7 @@ static int gpio_clk_driver_probe(struct
> > > > > platform_device *pdev)
> > > > >         struct device_node *node = pdev->dev.of_node;
> > > > >         const char **parent_names, *gpio_name;
> > > > >         unsigned int num_parents;
> > > > > +       unsigned long clk_flags;
> > > > >         struct gpio_desc *gpiod;
> > > > >         struct clk *clk;
> > > > >         bool is_mux;
> > > > > @@ -274,13 +275,16 @@ static int gpio_clk_driver_probe(struct
> > > > > platform_device *pdev)
> > > > >                 return ret;
> > > > >         }
> > > > >  
> > > > > +       clk_flags = of_property_read_bool(node, "clk-set-rate-
> > > > > parent")
> > > > > ?
> > > > > +                       CLK_SET_RATE_PARENT : 0;
> > > > 
> > > > Is there a DT binding update somewhere? It looks like a linux-ism
> > > > from
> > > 
> > > Good point. I did not think about the DT, and I guess I didn't search
> > > it
> > > thoroughly enough. Found DT files now.
> > > 
> > > > the DT perspective. I wonder if we can somehow figure out that it's
> > > > OK
> > > > to call clk_set_rate() on the parent here? Or is it safe to assume
> > > > that
> > > > we can just always call set rate on the parent? I think for a gate
> > > > it's
> > > > good and we can just do so, but for a mux maybe not. Care to
> > > > describe
> > > > your scenario a little more so we can understand why you want to
> > > > set
> > > > this flag? Is it for a mux or a gate type gpio?
> > > > 
> > > 
> > > For our case we are using it here [with a slight name variation in
> > > the
> > > prop
> > > name]:
> > > https://github.com/analogdevicesinc/linux/blob/master/arch/arm/boot/dts/zynq-adrv9361-z7035.dtsi#L43
> > > 
> > > And on this board:
> > > https://wiki.analog.com/resources/eval/user-guides/adrv936x_rfsom/user-guide/introduction
> > > 
> > > It's for a gate-type GPIO.
> > > The clock defined in that DT (ad9361_clkin) is attached to a clock
> > > that
> > > has
> > > a fixed rate (xo_40mhz_fixed_clk), that can become a variable rate
> > > [from an
> > > external source, when it is provided].
> > > 
> > > Our understanding, is that a GPIO gate clock should propagate the
> > > rate
> > > change to the parent clock. The same goes for the GPIO MUX clock.
> > > So, the default mode would be to always set CLK_SET_RATE_PARENT.
> > > 
> > > But, given that there are users of this driver, such a behavior
> > > change
> > > could break other users, so we are using this DT prop.
> > > 
> > > There seems to be only one user of gpio-mux-clock in
> > > arch/arm64/boot/dts/renesas/ulcb-kf.dtsi
> > > 
> > > Whereas for gpio-gate-clock, there are multiple users. I can't say
> > > whether
> > > this change would break anything for them, or it would be fine.
> > 
> > I think we can just add CLK_SET_RATE_PARENT for all gpio gate clks then
> > and you'll be happy? Care to send that patch and Cc Geert and other
> > Renesas maintainers? That avoids any DT property and should be fine for
> > anyone as far as I can tell. Maybe the inverse will be desired, but not
> > wanting rate to propagate is an edge case that we may want to call out
> > explicitly in DT at some point in the future, but shouldn't worry about
> > until then.
> > 
> 
> Will resend, thanks.
> Geert is CC-ed here as well [unless there is another Geert].

Oh right.
You CC-ed Geert this time. I assumed that I did, from the get_maintainers
script. It's still seems like black-art trying to see how tag people when
sending patches. Especially if you're new.

> 
> I'll send a V2 in-reply-to this thread.
> 
> Thanks
> Alex

[PATCH v2] clk: clk-gpio: propagate rate change to parent

[Alexandru Ardelean]

From: Michael Hennerich <michael.hennerich@analog.com>

For an external clock source, which is gated or (mux-ed) via a GPIO, the
rate change should typically be propagated to the parent clock.

The situation where we are requiring this propagation, is when an
external clock is connected to override an internal clock (which typically
has a fixed rate). The external clock can have a different rate than the
internal one, and may also be variable, thus requiring the rate
propagation.

This rate change wasn't propagated until now, and it's unclear about cases
where this shouldn't be propagated. Thus, it's unclear whether this is
fixing a bug, or extending the current driver behavior. Also, it's unsure
about whether this may break any existing setups; in the case that it does,
a device-tree property may be added to disable this flag.

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/clk/clk-gpio.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
index 9d930edd6516..255ed505444c 100644
--- a/drivers/clk/clk-gpio.c
+++ b/drivers/clk/clk-gpio.c
@@ -276,11 +276,12 @@ static int gpio_clk_driver_probe(struct platform_device *pdev)
 
 	if (is_mux)
 		clk = clk_register_gpio_mux(&pdev->dev, node->name,
-				parent_names, num_parents, gpiod, 0);
+				parent_names, num_parents, gpiod,
+				CLK_SET_RATE_PARENT);
 	else
 		clk = clk_register_gpio_gate(&pdev->dev, node->name,
 				parent_names ?  parent_names[0] : NULL, gpiod,
-				0);
+				CLK_SET_RATE_PARENT);
 	if (IS_ERR(clk))
 		return PTR_ERR(clk);
 
-- 
2.20.1

[PATCH v3] clk: clk-gpio: propagate rate change to parent

[Alexandru Ardelean]

From: Michael Hennerich <michael.hennerich@analog.com>

For an external clock source, which is gated via a GPIO, the
rate change should typically be propagated to the parent clock.

The situation where we are requiring this propagation, is when an
external clock is connected to override an internal clock (which typically
has a fixed rate). The external clock can have a different rate than the
internal one, and may also be variable, thus requiring the rate
propagation.

This rate change wasn't propagated until now, and it's unclear about cases
where this shouldn't be propagated. Thus, it's unclear whether this is
fixing a bug, or extending the current driver behavior. Also, it's unsure
about whether this may break any existing setups; in the case that it does,
a device-tree property may be added to disable this flag.

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---

Changelog v2 -> v3:
* limited CLK_SET_RATE_PARENT to both gate clk-gpio drivers; did not pay
  close attention to Stephen's comment when he mentioned to only the gate
  clk-gpio driver

Changelog v1 -> v2:
* added CLK_SET_RATE_PARENT to both gate & mux clk-gpio drivers

 drivers/clk/clk-gpio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
index 9d930edd6516..13304cf5f2a8 100644
--- a/drivers/clk/clk-gpio.c
+++ b/drivers/clk/clk-gpio.c
@@ -280,7 +280,7 @@ static int gpio_clk_driver_probe(struct platform_device *pdev)
 	else
 		clk = clk_register_gpio_gate(&pdev->dev, node->name,
 				parent_names ?  parent_names[0] : NULL, gpiod,
-				0);
+				CLK_SET_RATE_PARENT);
 	if (IS_ERR(clk))
 		return PTR_ERR(clk);
 
-- 
2.20.1

Re: [PATCH v3] clk: clk-gpio: propagate rate change to parent

[Stephen Boyd]

Quoting Alexandru Ardelean (2019-11-07 23:17:18)
> From: Michael Hennerich <michael.hennerich@analog.com>
> 
> For an external clock source, which is gated via a GPIO, the
> rate change should typically be propagated to the parent clock.
> 
> The situation where we are requiring this propagation, is when an
> external clock is connected to override an internal clock (which typically
> has a fixed rate). The external clock can have a different rate than the
> internal one, and may also be variable, thus requiring the rate
> propagation.
> 
> This rate change wasn't propagated until now, and it's unclear about cases
> where this shouldn't be propagated. Thus, it's unclear whether this is
> fixing a bug, or extending the current driver behavior. Also, it's unsure
> about whether this may break any existing setups; in the case that it does,
> a device-tree property may be added to disable this flag.
> 
> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---

Applied to clk-next

Next time please send as a new topic instead of a reply to the original
patch. Makes it easier for me to apply the patch.

Re: [PATCH v3] clk: clk-gpio: propagate rate change to parent

[Ardelean, Alexandru]

On Fri, 2019-11-08 at 13:08 -0800, Stephen Boyd wrote:
> Quoting Alexandru Ardelean (2019-11-07 23:17:18)
> > From: Michael Hennerich <michael.hennerich@analog.com>
> > 
> > For an external clock source, which is gated via a GPIO, the
> > rate change should typically be propagated to the parent clock.
> > 
> > The situation where we are requiring this propagation, is when an
> > external clock is connected to override an internal clock (which
> > typically
> > has a fixed rate). The external clock can have a different rate than
> > the
> > internal one, and may also be variable, thus requiring the rate
> > propagation.
> > 
> > This rate change wasn't propagated until now, and it's unclear about
> > cases
> > where this shouldn't be propagated. Thus, it's unclear whether this is
> > fixing a bug, or extending the current driver behavior. Also, it's
> > unsure
> > about whether this may break any existing setups; in the case that it
> > does,
> > a device-tree property may be added to disable this flag.
> > 
> > Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> 
> Applied to clk-next
> 
> Next time please send as a new topic instead of a reply to the original
> patch. Makes it easier for me to apply the patch.
> 

Ack.
Will do that.

Thanks
Alex