On 30/04/2022 08:01, Ansuel Smith wrote:
> Convert kpss-gcc driver Documentation to yaml.
> Add #clock-cells additional binding to required bindings and example
> as it's a required binding for clock-output-names.
>
> Signed-off-by: Ansuel Smith <[email protected]>
(...)
> +properties:
> + compatible:
> + items:
> + - enum:
> + - qcom,kpss-gcc-ipq8064
> + - qcom,kpss-gcc-apq8064
> + - qcom,kpss-gcc-msm8974
> + - qcom,kpss-gcc-msm8960
> + - const: qcom,kpss-gcc
> +
> + reg:
> + maxItems: 1
> +
> + clocks:
> + items:
> + - description: phandle to pll8_vote
> + - description: phandle to pxo_board
> +
> + clock-names:
> + items:
> + - const: pll8_vote
> + - const: pxo
> +
> + clock-output-names:
> + const: acpu_l2_aux
It does not make sense having a constant output name. What is the
meaning this property in such case? The original binding did not enforce it.
Best regards,
Krzysztof
On Sat, Apr 30, 2022 at 04:40:54PM +0200, Krzysztof Kozlowski wrote:
> On 30/04/2022 08:01, Ansuel Smith wrote:
> > Convert kpss-gcc driver Documentation to yaml.
> > Add #clock-cells additional binding to required bindings and example
> > as it's a required binding for clock-output-names.
> >
> > Signed-off-by: Ansuel Smith <[email protected]>
>
>
> (...)
>
> > +properties:
> > + compatible:
> > + items:
> > + - enum:
> > + - qcom,kpss-gcc-ipq8064
> > + - qcom,kpss-gcc-apq8064
> > + - qcom,kpss-gcc-msm8974
> > + - qcom,kpss-gcc-msm8960
> > + - const: qcom,kpss-gcc
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + clocks:
> > + items:
> > + - description: phandle to pll8_vote
> > + - description: phandle to pxo_board
> > +
> > + clock-names:
> > + items:
> > + - const: pll8_vote
> > + - const: pxo
> > +
> > + clock-output-names:
> > + const: acpu_l2_aux
>
> It does not make sense having a constant output name. What is the
> meaning this property in such case? The original binding did not enforce it.
>
>
>
> Best regards,
> Krzysztof
Mh. Should I just drop the const and put a description referring to an
advised name? The driver with the kpss-gcc hardcode the name to
acpu_l2_aux that's why I thought it was a correct conversion using a
const but I assume this is another problem of not making a correct 1:1
conversion and adding fixes on pure conversion.
Think I should drop it and put a description to it. (and then later fix
it when I will push the other series with all the tweaks)
What do you think?
--
Ansuel
On 30/04/2022 09:42, Ansuel Smith wrote:
> On Sat, Apr 30, 2022 at 04:40:54PM +0200, Krzysztof Kozlowski wrote:
>> On 30/04/2022 08:01, Ansuel Smith wrote:
>>> Convert kpss-gcc driver Documentation to yaml.
>>> Add #clock-cells additional binding to required bindings and example
>>> as it's a required binding for clock-output-names.
>>>
>>> Signed-off-by: Ansuel Smith <[email protected]>
>>
>>
>> (...)
>>
>>> +properties:
>>> + compatible:
>>> + items:
>>> + - enum:
>>> + - qcom,kpss-gcc-ipq8064
>>> + - qcom,kpss-gcc-apq8064
>>> + - qcom,kpss-gcc-msm8974
>>> + - qcom,kpss-gcc-msm8960
>>> + - const: qcom,kpss-gcc
>>> +
>>> + reg:
>>> + maxItems: 1
>>> +
>>> + clocks:
>>> + items:
>>> + - description: phandle to pll8_vote
>>> + - description: phandle to pxo_board
>>> +
>>> + clock-names:
>>> + items:
>>> + - const: pll8_vote
>>> + - const: pxo
>>> +
>>> + clock-output-names:
>>> + const: acpu_l2_aux
>>
>> It does not make sense having a constant output name. What is the
>> meaning this property in such case? The original binding did not enforce it.
>>
>>
>>
>> Best regards,
>> Krzysztof
>
> Mh. Should I just drop the const and put a description referring to an
> advised name? The driver with the kpss-gcc hardcode the name to
> acpu_l2_aux that's why I thought it was a correct conversion using a
> const but I assume this is another problem of not making a correct 1:1
> conversion and adding fixes on pure conversion.
Hard-coding a name by implementation is not a reason to put that name in
DTS. DTS is not a place for values stored in the driver.
> Think I should drop it and put a description to it. (and then later fix
> it when I will push the other series with all the tweaks)
The driver kpss-gcc does not use this property at all, so I am not sure
if there is a point to even keep it. Any other user of bindings makes
use of the property?
Best regards,
Krzysztof
On Sun, May 01, 2022 at 10:26:47AM +0200, Krzysztof Kozlowski wrote:
> On 30/04/2022 09:42, Ansuel Smith wrote:
> > On Sat, Apr 30, 2022 at 04:40:54PM +0200, Krzysztof Kozlowski wrote:
> >> On 30/04/2022 08:01, Ansuel Smith wrote:
> >>> Convert kpss-gcc driver Documentation to yaml.
> >>> Add #clock-cells additional binding to required bindings and example
> >>> as it's a required binding for clock-output-names.
> >>>
> >>> Signed-off-by: Ansuel Smith <[email protected]>
> >>
> >>
> >> (...)
> >>
> >>> +properties:
> >>> + compatible:
> >>> + items:
> >>> + - enum:
> >>> + - qcom,kpss-gcc-ipq8064
> >>> + - qcom,kpss-gcc-apq8064
> >>> + - qcom,kpss-gcc-msm8974
> >>> + - qcom,kpss-gcc-msm8960
> >>> + - const: qcom,kpss-gcc
> >>> +
> >>> + reg:
> >>> + maxItems: 1
> >>> +
> >>> + clocks:
> >>> + items:
> >>> + - description: phandle to pll8_vote
> >>> + - description: phandle to pxo_board
> >>> +
> >>> + clock-names:
> >>> + items:
> >>> + - const: pll8_vote
> >>> + - const: pxo
> >>> +
> >>> + clock-output-names:
> >>> + const: acpu_l2_aux
> >>
> >> It does not make sense having a constant output name. What is the
> >> meaning this property in such case? The original binding did not enforce it.
> >>
> >>
> >>
> >> Best regards,
> >> Krzysztof
> >
> > Mh. Should I just drop the const and put a description referring to an
> > advised name? The driver with the kpss-gcc hardcode the name to
> > acpu_l2_aux that's why I thought it was a correct conversion using a
> > const but I assume this is another problem of not making a correct 1:1
> > conversion and adding fixes on pure conversion.
>
> Hard-coding a name by implementation is not a reason to put that name in
> DTS. DTS is not a place for values stored in the driver.
>
> > Think I should drop it and put a description to it. (and then later fix
> > it when I will push the other series with all the tweaks)
>
> The driver kpss-gcc does not use this property at all, so I am not sure
> if there is a point to even keep it. Any other user of bindings makes
> use of the property?
>
>
> Best regards,
> Krzysztof
The idea is that you put the clk name in 'clock-output-names' and the
driver needs to have support for it (and set the clk name based on the
name defined in the dts)
This driver doesn't have support for it and is actually hardcoded.
So you are right and I should just drop it.
But now another question... Since #clock-cells was added as a
requirement for clock-output-names, should I drop also that?
In theory #clock-cells should always be declared for clock providers, is
it right to add it in the conversion commit or I should put this change
in another commit? (since it's now an addition and now something required
to fix a bot warning)
--
Ansuel
On Sat 30 Apr 02:42 CDT 2022, Ansuel Smith wrote:
> On Sat, Apr 30, 2022 at 04:40:54PM +0200, Krzysztof Kozlowski wrote:
> > On 30/04/2022 08:01, Ansuel Smith wrote:
> > > Convert kpss-gcc driver Documentation to yaml.
> > > Add #clock-cells additional binding to required bindings and example
> > > as it's a required binding for clock-output-names.
> > >
> > > Signed-off-by: Ansuel Smith <[email protected]>
> >
> >
> > (...)
> >
> > > +properties:
> > > + compatible:
> > > + items:
> > > + - enum:
> > > + - qcom,kpss-gcc-ipq8064
> > > + - qcom,kpss-gcc-apq8064
> > > + - qcom,kpss-gcc-msm8974
> > > + - qcom,kpss-gcc-msm8960
> > > + - const: qcom,kpss-gcc
> > > +
> > > + reg:
> > > + maxItems: 1
> > > +
> > > + clocks:
> > > + items:
> > > + - description: phandle to pll8_vote
> > > + - description: phandle to pxo_board
> > > +
> > > + clock-names:
> > > + items:
> > > + - const: pll8_vote
> > > + - const: pxo
> > > +
> > > + clock-output-names:
> > > + const: acpu_l2_aux
> >
> > It does not make sense having a constant output name. What is the
> > meaning this property in such case? The original binding did not enforce it.
> >
> >
> >
> > Best regards,
> > Krzysztof
>
> Mh. Should I just drop the const and put a description referring to an
> advised name? The driver with the kpss-gcc hardcode the name to
> acpu_l2_aux that's why I thought it was a correct conversion using a
> const but I assume this is another problem of not making a correct 1:1
> conversion and adding fixes on pure conversion.
> Think I should drop it and put a description to it. (and then later fix
> it when I will push the other series with all the tweaks)
>
> What do you think?
>
The typical reason for using clock-output-names is that we have some
consumer that finds the clock based on global name lookup. Over time
we've been moving these to use .fw_name or .index based lookup, which
removes this problem.
But I don't see that being the case here. So my suggestion is that you
just drop clock-output-names from the binding, which will solve
Krzysztof's objection.
From there we can review what needs to be done in the Linux driver to
work with the improved binding.
Regards,
Bjorn