Re: [PATCH] clk: add specified-rate clock

Re: [PATCH] clk: add specified-rate clock

[James Hogan]

On 29/05/13 18:39, Mike Turquette wrote:
> Quoting James Hogan (2013-05-10 05:44:22)
>> The frequency of some SoC's external oscillators (for example TZ1090's
>> XTAL1) are configured by the board using pull-ups/pull-downs of
>> configuration pins, the logic values of which are automatically latched
>> on reset and available in an SoC register. Add a generic clock component
>> and DT bindings to handle this.
>>
>> It behaves similar to a fixed rate clock (read-only), except it needs
>> information about a register field (reg, shift, width), and the
>> clock-frequency is a mapping from register field values to clock
>> frequencies.
>>
> 
> James,
> 
> Thanks for sending this!  It looks mostly good and is a useful clock
> type to support.  Comments below.

Hi Mike,

Sorry for slight delay getting back to you. I had another think about
this stuff yesterday...

> 
> <snip>
>> diff --git a/Documentation/devicetree/bindings/clock/specified-clock.txt b/Documentation/devicetree/bindings/clock/specified-clock.txt
>> new file mode 100644
>> index 0000000..b36ccf9
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/specified-clock.txt
>> @@ -0,0 +1,39 @@
>> +Binding for fixed-rate clock sources with readable configuration.
>> +
>> +This binding uses the common clock binding[1].
>> +
>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>> +
>> +Required properties:
>> +- compatible         : Shall be "specified-clock".
>> +- #clock-cells       : From common clock binding; shall be set to 0.
>> +- reg                : Address of configuration register.
>> +- shift              : Shift of config value field in configuration register.
>> +- width              : Width of config value field in configuration register.
> 
> It might be better to make this a mask instead of the width.  We have
> already hit this issue with the mux table on Nvidia where arbitrary
> masks are necessary.  Mask + shift probably helps future-proof us a bit.

Yes, thanks. I've now borrowed the bit-mask and bit-shift from your mux
binding proposals (including defaulting shift to ffs(mask)-1 ... nice).

> 
> The rest of the binding looks good.
> 
> <snip>
>> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
>> index e7f7fe9..1343179 100644
>> --- a/drivers/clk/Makefile
>> +++ b/drivers/clk/Makefile
>> @@ -8,6 +8,7 @@ obj-$(CONFIG_COMMON_CLK)        += clk-fixed-rate.o
>>  obj-$(CONFIG_COMMON_CLK)       += clk-gate.o
>>  obj-$(CONFIG_COMMON_CLK)       += clk-mux.o
>>  obj-$(CONFIG_COMMON_CLK)       += clk-composite.o
>> +obj-$(CONFIG_COMMON_CLK)       += clk-specified-rate.o
> 
> One thing that does occur to me is that this could potentially be
> combined with the fixed-rate clock.  If the properties for a specified
> rate existing in the DT data then this code applies, otherwise the
> existing fixed-rate code is used.  I don't have a strong opinion about
> combining the code, but something to consider.

That's actually a much neater solution. Because the clock is still
fixed, the register value can be read once while processing the DT node,
and an otherwise normal fixed rate clock created at the right frequency.
It doesn't even need to allocate memory to store the table.

The remaining question is whether to extend the fixed-clock binding or
have a separate one. I'm in two minds. On the one hand it is a
fixed-rate clock, on the other hand the only shared properties at the
moment would be standard clock properties (clock-output-names etc), so
I'm probably leaning towards a separate binding. I'll send an updated
patchset soon.

> 
> <snip>
>> +#ifdef CONFIG_OF
>> +/**
>> + * of_specified_clk_setup() - Setup function for specified fixed rate clock
>> + */
>> +void __init of_specified_clk_setup(struct device_node *node)
>> +{
>> +       struct clk *clk;
>> +       const char *clk_name = node->name;
>> +       u32 shift, width, rate;
>> +       void __iomem *reg;
>> +       int len, num_rates, i;
>> +       struct property *prop;
>> +       struct clk_specified_rate_entry *rates;
>> +       const __be32 *p;
>> +
>> +       of_property_read_string(node, "clock-output-names", &clk_name);
>> +
>> +       if (of_property_read_u32(node, "shift", &shift)) {
>> +               pr_err("%s(%s): could not read shift property\n",
>> +                      __func__, clk_name);
>> +               return;
>> +       }
>> +
>> +       if (of_property_read_u32(node, "width", &width)) {
>> +               pr_err("%s(%s): could not read width property\n",
>> +                      __func__, clk_name);
>> +               return;
>> +       }
>> +
>> +       reg = of_iomap(node, 0);
>> +       if (!reg) {
>> +               pr_err("%s(%s): of_iomap failed\n",
>> +                      __func__, clk_name);
>> +               return;
>> +       }
>> +
>> +       /* check clock-frequency exists */
>> +       prop = of_find_property(node, "clock-frequency", &len);
>> +       if (!prop) {
>> +               pr_err("%s(%s): could not find clock-frequency property\n",
>> +                      __func__, clk_name);
>> +               goto err_iounmap;
>> +       }
>> +
>> +       if (len & (sizeof(u32)*2 - 1)) {
>> +               pr_err("%s(%s): clock-frequency has invalid size of %d bytes\n",
>> +                      __func__, clk_name, len);
>> +               goto err_iounmap;
>> +       }
>> +       num_rates = len / (sizeof(*rates)*2);
> 
> This tripped me up for a few minutes.  I think it is a bit weird to
> count bytes as a way to validate length and determine the number of
> pairs.

Yes, and I actually recently discovered that this is wrong anyway. rates
is a struct of two elements so it ends up dividing twice, and it just so
happened that all the boards I have (until I hacked up qemu to report a
different frequency) specify a value in the first half of those defined. :)

Thanks
James

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

Re: [PATCH] clk: add specified-rate clock

[Tomasz Figa]

Hi James, Mike,

On Wednesday 13 of November 2013 14:09:56 James Hogan wrote:
> On 29/05/13 18:39, Mike Turquette wrote:
> > Quoting James Hogan (2013-05-10 05:44:22)
> >> The frequency of some SoC's external oscillators (for example TZ1090's
> >> XTAL1) are configured by the board using pull-ups/pull-downs of
> >> configuration pins, the logic values of which are automatically latched
> >> on reset and available in an SoC register. Add a generic clock component
> >> and DT bindings to handle this.
> >>
> >> It behaves similar to a fixed rate clock (read-only), except it needs
> >> information about a register field (reg, shift, width), and the
> >> clock-frequency is a mapping from register field values to clock
> >> frequencies.
> >>
> > 
> > James,
> > 
> > Thanks for sending this!  It looks mostly good and is a useful clock
> > type to support.  Comments below.
> 
> Hi Mike,
> 
> Sorry for slight delay getting back to you. I had another think about
> this stuff yesterday...
> 

Just a random idea that came to my mind while reading this thread:

What about modelling this as a set of fixed rate clocks fed into
a read-only mux?

Best regards,
Tomasz

> > 
> > <snip>
> >> diff --git a/Documentation/devicetree/bindings/clock/specified-clock.txt b/Documentation/devicetree/bindings/clock/specified-clock.txt
> >> new file mode 100644
> >> index 0000000..b36ccf9
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/clock/specified-clock.txt
> >> @@ -0,0 +1,39 @@
> >> +Binding for fixed-rate clock sources with readable configuration.
> >> +
> >> +This binding uses the common clock binding[1].
> >> +
> >> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> >> +
> >> +Required properties:
> >> +- compatible         : Shall be "specified-clock".
> >> +- #clock-cells       : From common clock binding; shall be set to 0.
> >> +- reg                : Address of configuration register.
> >> +- shift              : Shift of config value field in configuration register.
> >> +- width              : Width of config value field in configuration register.
> > 
> > It might be better to make this a mask instead of the width.  We have
> > already hit this issue with the mux table on Nvidia where arbitrary
> > masks are necessary.  Mask + shift probably helps future-proof us a bit.
> 
> Yes, thanks. I've now borrowed the bit-mask and bit-shift from your mux
> binding proposals (including defaulting shift to ffs(mask)-1 ... nice).
> 
> > 
> > The rest of the binding looks good.
> > 
> > <snip>
> >> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> >> index e7f7fe9..1343179 100644
> >> --- a/drivers/clk/Makefile
> >> +++ b/drivers/clk/Makefile
> >> @@ -8,6 +8,7 @@ obj-$(CONFIG_COMMON_CLK)        += clk-fixed-rate.o
> >>  obj-$(CONFIG_COMMON_CLK)       += clk-gate.o
> >>  obj-$(CONFIG_COMMON_CLK)       += clk-mux.o
> >>  obj-$(CONFIG_COMMON_CLK)       += clk-composite.o
> >> +obj-$(CONFIG_COMMON_CLK)       += clk-specified-rate.o
> > 
> > One thing that does occur to me is that this could potentially be
> > combined with the fixed-rate clock.  If the properties for a specified
> > rate existing in the DT data then this code applies, otherwise the
> > existing fixed-rate code is used.  I don't have a strong opinion about
> > combining the code, but something to consider.
> 
> That's actually a much neater solution. Because the clock is still
> fixed, the register value can be read once while processing the DT node,
> and an otherwise normal fixed rate clock created at the right frequency.
> It doesn't even need to allocate memory to store the table.
> 
> The remaining question is whether to extend the fixed-clock binding or
> have a separate one. I'm in two minds. On the one hand it is a
> fixed-rate clock, on the other hand the only shared properties at the
> moment would be standard clock properties (clock-output-names etc), so
> I'm probably leaning towards a separate binding. I'll send an updated
> patchset soon.
> 
> > 
> > <snip>
> >> +#ifdef CONFIG_OF
> >> +/**
> >> + * of_specified_clk_setup() - Setup function for specified fixed rate clock
> >> + */
> >> +void __init of_specified_clk_setup(struct device_node *node)
> >> +{
> >> +       struct clk *clk;
> >> +       const char *clk_name = node->name;
> >> +       u32 shift, width, rate;
> >> +       void __iomem *reg;
> >> +       int len, num_rates, i;
> >> +       struct property *prop;
> >> +       struct clk_specified_rate_entry *rates;
> >> +       const __be32 *p;
> >> +
> >> +       of_property_read_string(node, "clock-output-names", &clk_name);
> >> +
> >> +       if (of_property_read_u32(node, "shift", &shift)) {
> >> +               pr_err("%s(%s): could not read shift property\n",
> >> +                      __func__, clk_name);
> >> +               return;
> >> +       }
> >> +
> >> +       if (of_property_read_u32(node, "width", &width)) {
> >> +               pr_err("%s(%s): could not read width property\n",
> >> +                      __func__, clk_name);
> >> +               return;
> >> +       }
> >> +
> >> +       reg = of_iomap(node, 0);
> >> +       if (!reg) {
> >> +               pr_err("%s(%s): of_iomap failed\n",
> >> +                      __func__, clk_name);
> >> +               return;
> >> +       }
> >> +
> >> +       /* check clock-frequency exists */
> >> +       prop = of_find_property(node, "clock-frequency", &len);
> >> +       if (!prop) {
> >> +               pr_err("%s(%s): could not find clock-frequency property\n",
> >> +                      __func__, clk_name);
> >> +               goto err_iounmap;
> >> +       }
> >> +
> >> +       if (len & (sizeof(u32)*2 - 1)) {
> >> +               pr_err("%s(%s): clock-frequency has invalid size of %d bytes\n",
> >> +                      __func__, clk_name, len);
> >> +               goto err_iounmap;
> >> +       }
> >> +       num_rates = len / (sizeof(*rates)*2);
> > 
> > This tripped me up for a few minutes.  I think it is a bit weird to
> > count bytes as a way to validate length and determine the number of
> > pairs.
> 
> Yes, and I actually recently discovered that this is wrong anyway. rates
> is a struct of two elements so it ends up dividing twice, and it just so
> happened that all the boards I have (until I hacked up qemu to report a
> different frequency) specify a value in the first half of those defined. :)
> 
> Thanks
> James
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] clk: add specified-rate clock

[James Hogan]

On 13/11/13 14:18, Tomasz Figa wrote:
> Hi James, Mike,
> 
> On Wednesday 13 of November 2013 14:09:56 James Hogan wrote:
>> On 29/05/13 18:39, Mike Turquette wrote:
>>> Quoting James Hogan (2013-05-10 05:44:22)
>>>> The frequency of some SoC's external oscillators (for example TZ1090's
>>>> XTAL1) are configured by the board using pull-ups/pull-downs of
>>>> configuration pins, the logic values of which are automatically latched
>>>> on reset and available in an SoC register. Add a generic clock component
>>>> and DT bindings to handle this.
>>>>
>>>> It behaves similar to a fixed rate clock (read-only), except it needs
>>>> information about a register field (reg, shift, width), and the
>>>> clock-frequency is a mapping from register field values to clock
>>>> frequencies.
>>>>
>>>
>>> James,
>>>
>>> Thanks for sending this!  It looks mostly good and is a useful clock
>>> type to support.  Comments below.
>>
>> Hi Mike,
>>
>> Sorry for slight delay getting back to you. I had another think about
>> this stuff yesterday...
>>
> 
> Just a random idea that came to my mind while reading this thread:
> 
> What about modelling this as a set of fixed rate clocks fed into
> a read-only mux?

Yes, that had occurred to me too. I suppose the arguments against would be:
* it doesn't describe the hardware, there is no mux, just a fixed rate
clock with a discoverable frequency.
* it would sort of work for my small case of only having 9 possible
frequencies (although it would be a bit verbose), but wouldn't scale
nicely or be extendible to if the frequency was encoded more
continuously in the register value. E.g. if the frequency was 1MHz *
(the register value - 1) or something crazy like that. Of course that's
conjecture and SoC designers probably aren't going to want to use more
pins for bootstrap config than necessary.

Cheers
James

Re: [PATCH] clk: add specified-rate clock

[Tomasz Figa]

On Wednesday 13 of November 2013 14:31:25 James Hogan wrote:
> On 13/11/13 14:18, Tomasz Figa wrote:
> > Hi James, Mike,
> > 
> > On Wednesday 13 of November 2013 14:09:56 James Hogan wrote:
> >> On 29/05/13 18:39, Mike Turquette wrote:
> >>> Quoting James Hogan (2013-05-10 05:44:22)
> >>>> The frequency of some SoC's external oscillators (for example TZ1090's
> >>>> XTAL1) are configured by the board using pull-ups/pull-downs of
> >>>> configuration pins, the logic values of which are automatically latched
> >>>> on reset and available in an SoC register. Add a generic clock component
> >>>> and DT bindings to handle this.
> >>>>
> >>>> It behaves similar to a fixed rate clock (read-only), except it needs
> >>>> information about a register field (reg, shift, width), and the
> >>>> clock-frequency is a mapping from register field values to clock
> >>>> frequencies.
> >>>>
> >>>
> >>> James,
> >>>
> >>> Thanks for sending this!  It looks mostly good and is a useful clock
> >>> type to support.  Comments below.
> >>
> >> Hi Mike,
> >>
> >> Sorry for slight delay getting back to you. I had another think about
> >> this stuff yesterday...
> >>
> > 
> > Just a random idea that came to my mind while reading this thread:
> > 
> > What about modelling this as a set of fixed rate clocks fed into
> > a read-only mux?
> 
> Yes, that had occurred to me too. I suppose the arguments against would be:
> * it doesn't describe the hardware, there is no mux, just a fixed rate
> clock with a discoverable frequency.

For me, a set of configuration pins that select clock frequency sounds
like a mux. I'd rather say that there are no physical clocks that would
be its inputs, but that's probably just an implementation detail.

> * it would sort of work for my small case of only having 9 possible
> frequencies (although it would be a bit verbose), but wouldn't scale
> nicely or be extendible to if the frequency was encoded more
> continuously in the register value. E.g. if the frequency was 1MHz *
> (the register value - 1) or something crazy like that.

I guess you would need an implementation specific code to calculate the
frequency in such cases, which is not what you suggested above (but
rather "a mapping from register field values to clock frequencies"
inside a property).

Best regards,
Tomasz

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

Re: [PATCH] clk: add specified-rate clock

[James Hogan]

On 13/11/13 14:38, Tomasz Figa wrote:
> On Wednesday 13 of November 2013 14:31:25 James Hogan wrote:
>> On 13/11/13 14:18, Tomasz Figa wrote:
>>> Hi James, Mike,
>>>
>>> On Wednesday 13 of November 2013 14:09:56 James Hogan wrote:
>>>> On 29/05/13 18:39, Mike Turquette wrote:
>>>>> Quoting James Hogan (2013-05-10 05:44:22)
>>>>>> The frequency of some SoC's external oscillators (for example TZ1090's
>>>>>> XTAL1) are configured by the board using pull-ups/pull-downs of
>>>>>> configuration pins, the logic values of which are automatically latched
>>>>>> on reset and available in an SoC register. Add a generic clock component
>>>>>> and DT bindings to handle this.
>>>>>>
>>>>>> It behaves similar to a fixed rate clock (read-only), except it needs
>>>>>> information about a register field (reg, shift, width), and the
>>>>>> clock-frequency is a mapping from register field values to clock
>>>>>> frequencies.
>>>>>>
>>>>>
>>>>> James,
>>>>>
>>>>> Thanks for sending this!  It looks mostly good and is a useful clock
>>>>> type to support.  Comments below.
>>>>
>>>> Hi Mike,
>>>>
>>>> Sorry for slight delay getting back to you. I had another think about
>>>> this stuff yesterday...
>>>>
>>>
>>> Just a random idea that came to my mind while reading this thread:
>>>
>>> What about modelling this as a set of fixed rate clocks fed into
>>> a read-only mux?
>>
>> Yes, that had occurred to me too. I suppose the arguments against would be:
>> * it doesn't describe the hardware, there is no mux, just a fixed rate
>> clock with a discoverable frequency.
> 
> For me, a set of configuration pins that select clock frequency sounds
> like a mux. I'd rather say that there are no physical clocks that would
> be its inputs, but that's probably just an implementation detail.

Sure, it sounds a bit like one and has a similar register interface, but
it's still not one. The hardware to latch the pins on reset and present
the logic values in a register is independent of the hardware to get the
clock signal from the fixed rate oscillator on the board into the SoC.

>> * it would sort of work for my small case of only having 9 possible
>> frequencies (although it would be a bit verbose), but wouldn't scale
>> nicely or be extendible to if the frequency was encoded more
>> continuously in the register value. E.g. if the frequency was 1MHz *
>> (the register value - 1) or something crazy like that.
> 
> I guess you would need an implementation specific code to calculate the
> frequency in such cases, which is not what you suggested above (but
> rather "a mapping from register field values to clock frequencies"
> inside a property).

Indeed. It's worth noting though that we already have a variety of such
variations handled by the generic clock divider driver (table based
lookup, one based, power of two, etc), so we should probably allow for
others to extend this later too.

Cheers
James