mbox series

[v6,00/18] Modernize rest of the krait drivers

Message ID 20220321231548.14276-1-ansuelsmth@gmail.com (mailing list archive)
Headers show
Series Modernize rest of the krait drivers | expand

Message

Christian Marangi March 21, 2022, 11:15 p.m. UTC
This is a follow-up to the ipq806x gcc modernize series. Manu cleanup
changes and also some discoveries of wrong definition notice only with
all these conversions.

The first patch is an improvement of the clk_hw_get_parent_index. The
original idea of clk_hw_get_parent_index was to give a way to access the
parent index but for some reason the final version limited it to the
current index. We change it to give the current parent if is not
provided and to give the requested parent if provided. Any user of this
function is updated to follow the new implementation.

The patch 2 and 3 are some additional fixes for gcc.
The first one is a fix that register the pxo and cxo fixed clock only if
they are not defined in DTS.
The patch 3 require some explaination. In short is a big HACK to prevent
kernel panic with this series.

The kpss-xcc driver is a mess.
The Documentation declare that the clocks should be provided but for some
reason it was never followed.
In fact in the ipq8064 DTSI only the clocks for l2cc are declared but
for cpu0 and cpu1 the clocks are not defined.
The kpss-xcc driver use parent_names so the clks are ignored and never
used so till now it wasn't a problem (ignoring the fact that they
doesn't follow documentation at all)
On top of that, the l2cc node declare the pxo clock in a really strange
way. It's declared using the PXO_SRC gcc clock that is never defined in
the gcc ipq8064 clock table. (the correct way was to declare a fixed
clock in dts and reference that)
To prevent any kind of problem we use the patch 3 and provide the clk
for PXO_SRC in the gcc clock table. We manually provide the clk after
gcc probe.

Patch 4 is just a minor cleanup where we use the poll macro

Patch 5 is the actually kpss-xcc conversion to parent data

Patch 6-7 should be a fixup of a real conver case

Patch 8 converts the krait-cc to parent_data
Patch 9 give some love to the code with some minor fixup
Patch 10 drop the hardcoded safe sel and use the new
clk_hw_get_parent_index to get the safe parent index.
(also I discovered that the parent order was wrong)

Patch 11 is an additional fixup to force the reset of the muxes even
more.

Patch 12-13 are some additiona taken from the qsdk that were missing in
the upstream driver

Patch 14 converts krait-cc to yaml

Patch 15 add to krait-cc Documentation the L2 clocks

Patch 16 converts the kpss-acc driver to yaml and fix some Documentation
error

Patch 17 convets the kpss-gcc driver to yaml

Patch 18 finally adds all this stuff to the ipq8064 dtsi (and fix the
stupid PXO_SRC phandle)

I tested this series on a ipq8064 SoC by running a cache benchmark test
to make sure the changes are correct and we don't silently cause
regressions. Also I compared the output of the clk_summary every time
and we finally have a sane output where the mux are correctly placed in
the correct parent. (till now we had the cpu aux clock all over the
place, probably never cause problems but who knows.)

v6:
- Move dts patch as last patch
- Address commencts from Rob
- Fix warning from make dtbs_check
v5:
- Address comments from Krzysztof
v4:
- Fix more dt-bindings bog errors
v3:
- Split Documentation files for kpss and krait-cc
v2:
- introduce new API instead of fixing the existing one
- do not reorganize variables in krait-cc
- fix some comments error and improve it
- return better error for patch 7
- fix missing new line on patch 16

Ansuel Smith (18):
  clk: introduce clk_hw_get_index_of_parent new API
  clk: qcom: gcc-ipq806x: skip pxo/cxo fixed clk if already present
  clk: qcom: gcc-ipq806x: add PXO_SRC in clk table
  clk: qcom: clk-hfpll: use poll_timeout macro
  clk: qcom: kpss-xcc: convert to parent data API
  clk: qcom: clk-krait: unlock spin after mux completion
  clk: qcom: clk-krait: add hw_parent check for div2_round_rate
  clk: qcom: krait-cc: convert to parent_data API
  clk: qcom: krait-cc: drop pr_info and register qsb only if needed
  clk: qcom: krait-cc: drop hardcoded safe_sel
  clk: qcom: krait-cc: force sec_mux to QSB
  clk: qcom: clk-krait: add apq/ipq8064 errata workaround
  clk: qcom: clk-krait: add enable disable ops
  dt-bindings: clock: Convert qcom,krait-cc to yaml
  dt-bindings: clock: Add L2 clocks to qcom,krait-cc Documentation
  dt-bindings: arm: msm: Convert kpss-acc driver Documentation to yaml
  dt-bindings: arm: msm: Convert kpss-gcc driver Documentation to yaml
  ARM: dts: qcom: qcom-ipq8064: add missing krait-cc compatible and
    clocks

 .../bindings/arm/msm/qcom,kpss-acc.txt        |  49 -----
 .../bindings/arm/msm/qcom,kpss-acc.yaml       |  94 +++++++++
 .../bindings/arm/msm/qcom,kpss-gcc.txt        |  44 -----
 .../bindings/arm/msm/qcom,kpss-gcc.yaml       |  69 +++++++
 .../bindings/clock/qcom,krait-cc.txt          |  34 ----
 .../bindings/clock/qcom,krait-cc.yaml         |  65 ++++++
 arch/arm/boot/dts/qcom-ipq8064.dtsi           |  24 ++-
 drivers/clk/clk.c                             |  14 ++
 drivers/clk/qcom/clk-hfpll.c                  |  13 +-
 drivers/clk/qcom/clk-krait.c                  |  44 ++++-
 drivers/clk/qcom/clk-krait.h                  |   1 +
 drivers/clk/qcom/gcc-ipq806x.c                |  27 ++-
 drivers/clk/qcom/kpss-xcc.c                   |  25 +--
 drivers/clk/qcom/krait-cc.c                   | 186 ++++++++++--------
 include/linux/clk-provider.h                  |   1 +
 15 files changed, 453 insertions(+), 237 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.txt
 create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml
 delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
 create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
 delete mode 100644 Documentation/devicetree/bindings/clock/qcom,krait-cc.txt
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,krait-cc.yaml

Comments

Rob Herring March 22, 2022, 1:56 a.m. UTC | #1
On Mon, Mar 21, 2022 at 6:45 PM Ansuel Smith <ansuelsmth@gmail.com> wrote:
>
> This is a follow-up to the ipq806x gcc modernize series. Manu cleanup
> changes and also some discoveries of wrong definition notice only with
> all these conversions.
>
> The first patch is an improvement of the clk_hw_get_parent_index. The
> original idea of clk_hw_get_parent_index was to give a way to access the
> parent index but for some reason the final version limited it to the
> current index. We change it to give the current parent if is not
> provided and to give the requested parent if provided. Any user of this
> function is updated to follow the new implementation.
>
> The patch 2 and 3 are some additional fixes for gcc.
> The first one is a fix that register the pxo and cxo fixed clock only if
> they are not defined in DTS.
> The patch 3 require some explaination. In short is a big HACK to prevent
> kernel panic with this series.
>
> The kpss-xcc driver is a mess.
> The Documentation declare that the clocks should be provided but for some
> reason it was never followed.
> In fact in the ipq8064 DTSI only the clocks for l2cc are declared but
> for cpu0 and cpu1 the clocks are not defined.
> The kpss-xcc driver use parent_names so the clks are ignored and never
> used so till now it wasn't a problem (ignoring the fact that they
> doesn't follow documentation at all)
> On top of that, the l2cc node declare the pxo clock in a really strange
> way. It's declared using the PXO_SRC gcc clock that is never defined in
> the gcc ipq8064 clock table. (the correct way was to declare a fixed
> clock in dts and reference that)
> To prevent any kind of problem we use the patch 3 and provide the clk
> for PXO_SRC in the gcc clock table. We manually provide the clk after
> gcc probe.
>
> Patch 4 is just a minor cleanup where we use the poll macro
>
> Patch 5 is the actually kpss-xcc conversion to parent data
>
> Patch 6-7 should be a fixup of a real conver case
>
> Patch 8 converts the krait-cc to parent_data
> Patch 9 give some love to the code with some minor fixup
> Patch 10 drop the hardcoded safe sel and use the new
> clk_hw_get_parent_index to get the safe parent index.
> (also I discovered that the parent order was wrong)
>
> Patch 11 is an additional fixup to force the reset of the muxes even
> more.
>
> Patch 12-13 are some additiona taken from the qsdk that were missing in
> the upstream driver
>
> Patch 14 converts krait-cc to yaml
>
> Patch 15 add to krait-cc Documentation the L2 clocks
>
> Patch 16 converts the kpss-acc driver to yaml and fix some Documentation
> error
>
> Patch 17 convets the kpss-gcc driver to yaml
>
> Patch 18 finally adds all this stuff to the ipq8064 dtsi (and fix the
> stupid PXO_SRC phandle)
>
> I tested this series on a ipq8064 SoC by running a cache benchmark test
> to make sure the changes are correct and we don't silently cause
> regressions. Also I compared the output of the clk_summary every time
> and we finally have a sane output where the mux are correctly placed in
> the correct parent. (till now we had the cpu aux clock all over the
> place, probably never cause problems but who knows.)
>
> v6:
> - Move dts patch as last patch
> - Address commencts from Rob
> - Fix warning from make dtbs_check
> v5:
> - Address comments from Krzysztof
> v4:
> - Fix more dt-bindings bog errors
> v3:
> - Split Documentation files for kpss and krait-cc
> v2:
> - introduce new API instead of fixing the existing one
> - do not reorganize variables in krait-cc
> - fix some comments error and improve it
> - return better error for patch 7
> - fix missing new line on patch 16

6 versions in a week is too many, especially with the merge window
starting. Please give some time for review of all the patches and by
more than one person. It doesn't look like any of the clk driver
patches have been reviewed since v1 for example.

Rob
Christian Marangi March 22, 2022, 1:43 p.m. UTC | #2
On Mon, Mar 21, 2022 at 08:56:13PM -0500, Rob Herring wrote:
> On Mon, Mar 21, 2022 at 6:45 PM Ansuel Smith <ansuelsmth@gmail.com> wrote:
> >
> > This is a follow-up to the ipq806x gcc modernize series. Manu cleanup
> > changes and also some discoveries of wrong definition notice only with
> > all these conversions.
> >
> > The first patch is an improvement of the clk_hw_get_parent_index. The
> > original idea of clk_hw_get_parent_index was to give a way to access the
> > parent index but for some reason the final version limited it to the
> > current index. We change it to give the current parent if is not
> > provided and to give the requested parent if provided. Any user of this
> > function is updated to follow the new implementation.
> >
> > The patch 2 and 3 are some additional fixes for gcc.
> > The first one is a fix that register the pxo and cxo fixed clock only if
> > they are not defined in DTS.
> > The patch 3 require some explaination. In short is a big HACK to prevent
> > kernel panic with this series.
> >
> > The kpss-xcc driver is a mess.
> > The Documentation declare that the clocks should be provided but for some
> > reason it was never followed.
> > In fact in the ipq8064 DTSI only the clocks for l2cc are declared but
> > for cpu0 and cpu1 the clocks are not defined.
> > The kpss-xcc driver use parent_names so the clks are ignored and never
> > used so till now it wasn't a problem (ignoring the fact that they
> > doesn't follow documentation at all)
> > On top of that, the l2cc node declare the pxo clock in a really strange
> > way. It's declared using the PXO_SRC gcc clock that is never defined in
> > the gcc ipq8064 clock table. (the correct way was to declare a fixed
> > clock in dts and reference that)
> > To prevent any kind of problem we use the patch 3 and provide the clk
> > for PXO_SRC in the gcc clock table. We manually provide the clk after
> > gcc probe.
> >
> > Patch 4 is just a minor cleanup where we use the poll macro
> >
> > Patch 5 is the actually kpss-xcc conversion to parent data
> >
> > Patch 6-7 should be a fixup of a real conver case
> >
> > Patch 8 converts the krait-cc to parent_data
> > Patch 9 give some love to the code with some minor fixup
> > Patch 10 drop the hardcoded safe sel and use the new
> > clk_hw_get_parent_index to get the safe parent index.
> > (also I discovered that the parent order was wrong)
> >
> > Patch 11 is an additional fixup to force the reset of the muxes even
> > more.
> >
> > Patch 12-13 are some additiona taken from the qsdk that were missing in
> > the upstream driver
> >
> > Patch 14 converts krait-cc to yaml
> >
> > Patch 15 add to krait-cc Documentation the L2 clocks
> >
> > Patch 16 converts the kpss-acc driver to yaml and fix some Documentation
> > error
> >
> > Patch 17 convets the kpss-gcc driver to yaml
> >
> > Patch 18 finally adds all this stuff to the ipq8064 dtsi (and fix the
> > stupid PXO_SRC phandle)
> >
> > I tested this series on a ipq8064 SoC by running a cache benchmark test
> > to make sure the changes are correct and we don't silently cause
> > regressions. Also I compared the output of the clk_summary every time
> > and we finally have a sane output where the mux are correctly placed in
> > the correct parent. (till now we had the cpu aux clock all over the
> > place, probably never cause problems but who knows.)
> >
> > v6:
> > - Move dts patch as last patch
> > - Address commencts from Rob
> > - Fix warning from make dtbs_check
> > v5:
> > - Address comments from Krzysztof
> > v4:
> > - Fix more dt-bindings bog errors
> > v3:
> > - Split Documentation files for kpss and krait-cc
> > v2:
> > - introduce new API instead of fixing the existing one
> > - do not reorganize variables in krait-cc
> > - fix some comments error and improve it
> > - return better error for patch 7
> > - fix missing new line on patch 16
> 
> 6 versions in a week is too many, especially with the merge window
> starting. Please give some time for review of all the patches and by
> more than one person. It doesn't look like any of the clk driver
> patches have been reviewed since v1 for example.
> 
> Rob

Yes sorry. There was an initial review for the clk driver from v1 to
v2 but nothing else. I was trying to make the Documentation ready while
I wait for a second review of the clk code.

Will wait for clk code review to send v7 hoping it will be the final
version.
Dmitry Baryshkov April 13, 2022, 5:31 p.m. UTC | #3
On 22/03/2022 02:15, Ansuel Smith wrote:
> This is a follow-up to the ipq806x gcc modernize series. Manu cleanup
> changes and also some discoveries of wrong definition notice only with
> all these conversions.

General comment regarding this patch series. It contains fixes, clock 
conversion for several drivers, dts fixes, etc. It's, for example, not 
straightforwardly obvious if Bjorn can pickup patches 04 or 06 without 
picking up other patches.

If would be best if you can split this series or at least pull fixes to 
be the first patches in the pile.

Patch 01 is only used by patch 10, they can stay close.

In some of the commit messages you describe what do they do, but you 
completely omit the reason for the change, why is the change necessary.
(Yes, I spot that because I also too often skip that).

> 
> The first patch is an improvement of the clk_hw_get_parent_index. The
> original idea of clk_hw_get_parent_index was to give a way to access the
> parent index but for some reason the final version limited it to the
> current index. We change it to give the current parent if is not
> provided and to give the requested parent if provided. Any user of this
> function is updated to follow the new implementation.
> 
> The patch 2 and 3 are some additional fixes for gcc.
> The first one is a fix that register the pxo and cxo fixed clock only if
> they are not defined in DTS.
> The patch 3 require some explaination. In short is a big HACK to prevent
> kernel panic with this series.
> 
> The kpss-xcc driver is a mess.
> The Documentation declare that the clocks should be provided but for some
> reason it was never followed.
> In fact in the ipq8064 DTSI only the clocks for l2cc are declared but
> for cpu0 and cpu1 the clocks are not defined.
> The kpss-xcc driver use parent_names so the clks are ignored and never
> used so till now it wasn't a problem (ignoring the fact that they
> doesn't follow documentation at all)
> On top of that, the l2cc node declare the pxo clock in a really strange
> way. It's declared using the PXO_SRC gcc clock that is never defined in
> the gcc ipq8064 clock table. (the correct way was to declare a fixed
> clock in dts and reference that)
> To prevent any kind of problem we use the patch 3 and provide the clk
> for PXO_SRC in the gcc clock table. We manually provide the clk after
> gcc probe.
> 
> Patch 4 is just a minor cleanup where we use the poll macro
> 
> Patch 5 is the actually kpss-xcc conversion to parent data
> 
> Patch 6-7 should be a fixup of a real conver case
> 
> Patch 8 converts the krait-cc to parent_data
> Patch 9 give some love to the code with some minor fixup
> Patch 10 drop the hardcoded safe sel and use the new
> clk_hw_get_parent_index to get the safe parent index.
> (also I discovered that the parent order was wrong)
> 
> Patch 11 is an additional fixup to force the reset of the muxes even
> more.
> 
> Patch 12-13 are some additiona taken from the qsdk that were missing in
> the upstream driver
> 
> Patch 14 converts krait-cc to yaml
> 
> Patch 15 add to krait-cc Documentation the L2 clocks
> 
> Patch 16 converts the kpss-acc driver to yaml and fix some Documentation
> error
> 
> Patch 17 convets the kpss-gcc driver to yaml
> 
> Patch 18 finally adds all this stuff to the ipq8064 dtsi (and fix the
> stupid PXO_SRC phandle)
> 
> I tested this series on a ipq8064 SoC by running a cache benchmark test
> to make sure the changes are correct and we don't silently cause
> regressions. Also I compared the output of the clk_summary every time
> and we finally have a sane output where the mux are correctly placed in
> the correct parent. (till now we had the cpu aux clock all over the
> place, probably never cause problems but who knows.)
> 
> v6:
> - Move dts patch as last patch
> - Address commencts from Rob
> - Fix warning from make dtbs_check
> v5:
> - Address comments from Krzysztof
> v4:
> - Fix more dt-bindings bog errors
> v3:
> - Split Documentation files for kpss and krait-cc
> v2:
> - introduce new API instead of fixing the existing one
> - do not reorganize variables in krait-cc
> - fix some comments error and improve it
> - return better error for patch 7
> - fix missing new line on patch 16
> 
> Ansuel Smith (18):
>    clk: introduce clk_hw_get_index_of_parent new API
>    clk: qcom: gcc-ipq806x: skip pxo/cxo fixed clk if already present
>    clk: qcom: gcc-ipq806x: add PXO_SRC in clk table
>    clk: qcom: clk-hfpll: use poll_timeout macro
>    clk: qcom: kpss-xcc: convert to parent data API
>    clk: qcom: clk-krait: unlock spin after mux completion
>    clk: qcom: clk-krait: add hw_parent check for div2_round_rate
>    clk: qcom: krait-cc: convert to parent_data API
>    clk: qcom: krait-cc: drop pr_info and register qsb only if needed
>    clk: qcom: krait-cc: drop hardcoded safe_sel
>    clk: qcom: krait-cc: force sec_mux to QSB
>    clk: qcom: clk-krait: add apq/ipq8064 errata workaround
>    clk: qcom: clk-krait: add enable disable ops
>    dt-bindings: clock: Convert qcom,krait-cc to yaml
>    dt-bindings: clock: Add L2 clocks to qcom,krait-cc Documentation
>    dt-bindings: arm: msm: Convert kpss-acc driver Documentation to yaml
>    dt-bindings: arm: msm: Convert kpss-gcc driver Documentation to yaml
>    ARM: dts: qcom: qcom-ipq8064: add missing krait-cc compatible and
>      clocks
> 
>   .../bindings/arm/msm/qcom,kpss-acc.txt        |  49 -----
>   .../bindings/arm/msm/qcom,kpss-acc.yaml       |  94 +++++++++
>   .../bindings/arm/msm/qcom,kpss-gcc.txt        |  44 -----
>   .../bindings/arm/msm/qcom,kpss-gcc.yaml       |  69 +++++++
>   .../bindings/clock/qcom,krait-cc.txt          |  34 ----
>   .../bindings/clock/qcom,krait-cc.yaml         |  65 ++++++
>   arch/arm/boot/dts/qcom-ipq8064.dtsi           |  24 ++-
>   drivers/clk/clk.c                             |  14 ++
>   drivers/clk/qcom/clk-hfpll.c                  |  13 +-
>   drivers/clk/qcom/clk-krait.c                  |  44 ++++-
>   drivers/clk/qcom/clk-krait.h                  |   1 +
>   drivers/clk/qcom/gcc-ipq806x.c                |  27 ++-
>   drivers/clk/qcom/kpss-xcc.c                   |  25 +--
>   drivers/clk/qcom/krait-cc.c                   | 186 ++++++++++--------
>   include/linux/clk-provider.h                  |   1 +
>   15 files changed, 453 insertions(+), 237 deletions(-)
>   delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.txt
>   create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml
>   delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
>   create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
>   delete mode 100644 Documentation/devicetree/bindings/clock/qcom,krait-cc.txt
>   create mode 100644 Documentation/devicetree/bindings/clock/qcom,krait-cc.yaml
>
Christian Marangi April 13, 2022, 5:48 p.m. UTC | #4
On Wed, Apr 13, 2022 at 08:31:31PM +0300, Dmitry Baryshkov wrote:
> On 22/03/2022 02:15, Ansuel Smith wrote:
> > This is a follow-up to the ipq806x gcc modernize series. Manu cleanup
> > changes and also some discoveries of wrong definition notice only with
> > all these conversions.
> 
> General comment regarding this patch series. It contains fixes, clock
> conversion for several drivers, dts fixes, etc. It's, for example, not
> straightforwardly obvious if Bjorn can pickup patches 04 or 06 without
> picking up other patches.
> 
> If would be best if you can split this series or at least pull fixes to be
> the first patches in the pile.
>

Considering that now this is grown to 21 patch in v7 (that is still have
to push)... Yes I think I have to split this...
Wonder if you can give me some hint. 

- Series for krait-cc
- Series for kpss-acc/gcc
- Single patch for hfpll
- Single patch for gcc fixes
- Series for kpss-xcc
- Series for clk-krait
- Series for dts fixes?

Wonder if this kind of split can work?

> Patch 01 is only used by patch 10, they can stay close.
> 
> In some of the commit messages you describe what do they do, but you
> completely omit the reason for the change, why is the change necessary.
> (Yes, I spot that because I also too often skip that).
> 
> > 
> > The first patch is an improvement of the clk_hw_get_parent_index. The
> > original idea of clk_hw_get_parent_index was to give a way to access the
> > parent index but for some reason the final version limited it to the
> > current index. We change it to give the current parent if is not
> > provided and to give the requested parent if provided. Any user of this
> > function is updated to follow the new implementation.
> > 
> > The patch 2 and 3 are some additional fixes for gcc.
> > The first one is a fix that register the pxo and cxo fixed clock only if
> > they are not defined in DTS.
> > The patch 3 require some explaination. In short is a big HACK to prevent
> > kernel panic with this series.
> > 
> > The kpss-xcc driver is a mess.
> > The Documentation declare that the clocks should be provided but for some
> > reason it was never followed.
> > In fact in the ipq8064 DTSI only the clocks for l2cc are declared but
> > for cpu0 and cpu1 the clocks are not defined.
> > The kpss-xcc driver use parent_names so the clks are ignored and never
> > used so till now it wasn't a problem (ignoring the fact that they
> > doesn't follow documentation at all)
> > On top of that, the l2cc node declare the pxo clock in a really strange
> > way. It's declared using the PXO_SRC gcc clock that is never defined in
> > the gcc ipq8064 clock table. (the correct way was to declare a fixed
> > clock in dts and reference that)
> > To prevent any kind of problem we use the patch 3 and provide the clk
> > for PXO_SRC in the gcc clock table. We manually provide the clk after
> > gcc probe.
> > 
> > Patch 4 is just a minor cleanup where we use the poll macro
> > 
> > Patch 5 is the actually kpss-xcc conversion to parent data
> > 
> > Patch 6-7 should be a fixup of a real conver case
> > 
> > Patch 8 converts the krait-cc to parent_data
> > Patch 9 give some love to the code with some minor fixup
> > Patch 10 drop the hardcoded safe sel and use the new
> > clk_hw_get_parent_index to get the safe parent index.
> > (also I discovered that the parent order was wrong)
> > 
> > Patch 11 is an additional fixup to force the reset of the muxes even
> > more.
> > 
> > Patch 12-13 are some additiona taken from the qsdk that were missing in
> > the upstream driver
> > 
> > Patch 14 converts krait-cc to yaml
> > 
> > Patch 15 add to krait-cc Documentation the L2 clocks
> > 
> > Patch 16 converts the kpss-acc driver to yaml and fix some Documentation
> > error
> > 
> > Patch 17 convets the kpss-gcc driver to yaml
> > 
> > Patch 18 finally adds all this stuff to the ipq8064 dtsi (and fix the
> > stupid PXO_SRC phandle)
> > 
> > I tested this series on a ipq8064 SoC by running a cache benchmark test
> > to make sure the changes are correct and we don't silently cause
> > regressions. Also I compared the output of the clk_summary every time
> > and we finally have a sane output where the mux are correctly placed in
> > the correct parent. (till now we had the cpu aux clock all over the
> > place, probably never cause problems but who knows.)
> > 
> > v6:
> > - Move dts patch as last patch
> > - Address commencts from Rob
> > - Fix warning from make dtbs_check
> > v5:
> > - Address comments from Krzysztof
> > v4:
> > - Fix more dt-bindings bog errors
> > v3:
> > - Split Documentation files for kpss and krait-cc
> > v2:
> > - introduce new API instead of fixing the existing one
> > - do not reorganize variables in krait-cc
> > - fix some comments error and improve it
> > - return better error for patch 7
> > - fix missing new line on patch 16
> > 
> > Ansuel Smith (18):
> >    clk: introduce clk_hw_get_index_of_parent new API
> >    clk: qcom: gcc-ipq806x: skip pxo/cxo fixed clk if already present
> >    clk: qcom: gcc-ipq806x: add PXO_SRC in clk table
> >    clk: qcom: clk-hfpll: use poll_timeout macro
> >    clk: qcom: kpss-xcc: convert to parent data API
> >    clk: qcom: clk-krait: unlock spin after mux completion
> >    clk: qcom: clk-krait: add hw_parent check for div2_round_rate
> >    clk: qcom: krait-cc: convert to parent_data API
> >    clk: qcom: krait-cc: drop pr_info and register qsb only if needed
> >    clk: qcom: krait-cc: drop hardcoded safe_sel
> >    clk: qcom: krait-cc: force sec_mux to QSB
> >    clk: qcom: clk-krait: add apq/ipq8064 errata workaround
> >    clk: qcom: clk-krait: add enable disable ops
> >    dt-bindings: clock: Convert qcom,krait-cc to yaml
> >    dt-bindings: clock: Add L2 clocks to qcom,krait-cc Documentation
> >    dt-bindings: arm: msm: Convert kpss-acc driver Documentation to yaml
> >    dt-bindings: arm: msm: Convert kpss-gcc driver Documentation to yaml
> >    ARM: dts: qcom: qcom-ipq8064: add missing krait-cc compatible and
> >      clocks
> > 
> >   .../bindings/arm/msm/qcom,kpss-acc.txt        |  49 -----
> >   .../bindings/arm/msm/qcom,kpss-acc.yaml       |  94 +++++++++
> >   .../bindings/arm/msm/qcom,kpss-gcc.txt        |  44 -----
> >   .../bindings/arm/msm/qcom,kpss-gcc.yaml       |  69 +++++++
> >   .../bindings/clock/qcom,krait-cc.txt          |  34 ----
> >   .../bindings/clock/qcom,krait-cc.yaml         |  65 ++++++
> >   arch/arm/boot/dts/qcom-ipq8064.dtsi           |  24 ++-
> >   drivers/clk/clk.c                             |  14 ++
> >   drivers/clk/qcom/clk-hfpll.c                  |  13 +-
> >   drivers/clk/qcom/clk-krait.c                  |  44 ++++-
> >   drivers/clk/qcom/clk-krait.h                  |   1 +
> >   drivers/clk/qcom/gcc-ipq806x.c                |  27 ++-
> >   drivers/clk/qcom/kpss-xcc.c                   |  25 +--
> >   drivers/clk/qcom/krait-cc.c                   | 186 ++++++++++--------
> >   include/linux/clk-provider.h                  |   1 +
> >   15 files changed, 453 insertions(+), 237 deletions(-)
> >   delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.txt
> >   create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml
> >   delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
> >   create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
> >   delete mode 100644 Documentation/devicetree/bindings/clock/qcom,krait-cc.txt
> >   create mode 100644 Documentation/devicetree/bindings/clock/qcom,krait-cc.yaml
> > 
> 
> 
> -- 
> With best wishes
> Dmitry
Dmitry Baryshkov April 13, 2022, 7:49 p.m. UTC | #5
On Wed, 13 Apr 2022 at 20:48, Ansuel Smith <ansuelsmth@gmail.com> wrote:
>
> On Wed, Apr 13, 2022 at 08:31:31PM +0300, Dmitry Baryshkov wrote:
> > On 22/03/2022 02:15, Ansuel Smith wrote:
> > > This is a follow-up to the ipq806x gcc modernize series. Manu cleanup
> > > changes and also some discoveries of wrong definition notice only with
> > > all these conversions.
> >
> > General comment regarding this patch series. It contains fixes, clock
> > conversion for several drivers, dts fixes, etc. It's, for example, not
> > straightforwardly obvious if Bjorn can pickup patches 04 or 06 without
> > picking up other patches.
> >
> > If would be best if you can split this series or at least pull fixes to be
> > the first patches in the pile.
> >
>
> Considering that now this is grown to 21 patch in v7 (that is still have
> to push)... Yes I think I have to split this...
> Wonder if you can give me some hint.
>
> - Series for krait-cc
> - Series for kpss-acc/gcc
> - Single patch for hfpll
> - Single patch for gcc fixes
> - Series for kpss-xcc
> - Series for clk-krait
> - Series for dts fixes?

Yes, this sounds more or less logical. hfpll can go together with any
of the krait patches.
If you'd like a fewer series, the following looks sane too:
- fixes for hfpll, clk-krait, etc. Small changes that can be picked up
immediately.
- modernize gcc,
- update dts to follow gcc changes
- cpufreq drivers conversion
- update dts to follow cpufreq changes

As a generic notice: it might sound awkward, but could you please
split dt-bindings conversions (that were not R-B yet) into separate
parts:
- Just convert to yaml, no changes
- fix this-and-that.

I think Krzyshtof and Rob has already asked for that, but it's still
worth mentioning.

To stop from flooding the list, what about:
- posting fixes patches
- posting and polishing gcc conversion + dts updates

The rest can come up later. It might sound like a delay, but in
reality it might be easier to review.

> Wonder if this kind of split can work?
>
> > Patch 01 is only used by patch 10, they can stay close.
> >
> > In some of the commit messages you describe what do they do, but you
> > completely omit the reason for the change, why is the change necessary.
> > (Yes, I spot that because I also too often skip that).
> >
> > >
> > > The first patch is an improvement of the clk_hw_get_parent_index. The
> > > original idea of clk_hw_get_parent_index was to give a way to access the
> > > parent index but for some reason the final version limited it to the
> > > current index. We change it to give the current parent if is not
> > > provided and to give the requested parent if provided. Any user of this
> > > function is updated to follow the new implementation.
> > >
> > > The patch 2 and 3 are some additional fixes for gcc.
> > > The first one is a fix that register the pxo and cxo fixed clock only if
> > > they are not defined in DTS.
> > > The patch 3 require some explaination. In short is a big HACK to prevent
> > > kernel panic with this series.
> > >
> > > The kpss-xcc driver is a mess.
> > > The Documentation declare that the clocks should be provided but for some
> > > reason it was never followed.
> > > In fact in the ipq8064 DTSI only the clocks for l2cc are declared but
> > > for cpu0 and cpu1 the clocks are not defined.
> > > The kpss-xcc driver use parent_names so the clks are ignored and never
> > > used so till now it wasn't a problem (ignoring the fact that they
> > > doesn't follow documentation at all)
> > > On top of that, the l2cc node declare the pxo clock in a really strange
> > > way. It's declared using the PXO_SRC gcc clock that is never defined in
> > > the gcc ipq8064 clock table. (the correct way was to declare a fixed
> > > clock in dts and reference that)
> > > To prevent any kind of problem we use the patch 3 and provide the clk
> > > for PXO_SRC in the gcc clock table. We manually provide the clk after
> > > gcc probe.
> > >
> > > Patch 4 is just a minor cleanup where we use the poll macro
> > >
> > > Patch 5 is the actually kpss-xcc conversion to parent data
> > >
> > > Patch 6-7 should be a fixup of a real conver case
> > >
> > > Patch 8 converts the krait-cc to parent_data
> > > Patch 9 give some love to the code with some minor fixup
> > > Patch 10 drop the hardcoded safe sel and use the new
> > > clk_hw_get_parent_index to get the safe parent index.
> > > (also I discovered that the parent order was wrong)
> > >
> > > Patch 11 is an additional fixup to force the reset of the muxes even
> > > more.
> > >
> > > Patch 12-13 are some additiona taken from the qsdk that were missing in
> > > the upstream driver
> > >
> > > Patch 14 converts krait-cc to yaml
> > >
> > > Patch 15 add to krait-cc Documentation the L2 clocks
> > >
> > > Patch 16 converts the kpss-acc driver to yaml and fix some Documentation
> > > error
> > >
> > > Patch 17 convets the kpss-gcc driver to yaml
> > >
> > > Patch 18 finally adds all this stuff to the ipq8064 dtsi (and fix the
> > > stupid PXO_SRC phandle)
> > >
> > > I tested this series on a ipq8064 SoC by running a cache benchmark test
> > > to make sure the changes are correct and we don't silently cause
> > > regressions. Also I compared the output of the clk_summary every time
> > > and we finally have a sane output where the mux are correctly placed in
> > > the correct parent. (till now we had the cpu aux clock all over the
> > > place, probably never cause problems but who knows.)
> > >
> > > v6:
> > > - Move dts patch as last patch
> > > - Address commencts from Rob
> > > - Fix warning from make dtbs_check
> > > v5:
> > > - Address comments from Krzysztof
> > > v4:
> > > - Fix more dt-bindings bog errors
> > > v3:
> > > - Split Documentation files for kpss and krait-cc
> > > v2:
> > > - introduce new API instead of fixing the existing one
> > > - do not reorganize variables in krait-cc
> > > - fix some comments error and improve it
> > > - return better error for patch 7
> > > - fix missing new line on patch 16
> > >
> > > Ansuel Smith (18):
> > >    clk: introduce clk_hw_get_index_of_parent new API
> > >    clk: qcom: gcc-ipq806x: skip pxo/cxo fixed clk if already present
> > >    clk: qcom: gcc-ipq806x: add PXO_SRC in clk table
> > >    clk: qcom: clk-hfpll: use poll_timeout macro
> > >    clk: qcom: kpss-xcc: convert to parent data API
> > >    clk: qcom: clk-krait: unlock spin after mux completion
> > >    clk: qcom: clk-krait: add hw_parent check for div2_round_rate
> > >    clk: qcom: krait-cc: convert to parent_data API
> > >    clk: qcom: krait-cc: drop pr_info and register qsb only if needed
> > >    clk: qcom: krait-cc: drop hardcoded safe_sel
> > >    clk: qcom: krait-cc: force sec_mux to QSB
> > >    clk: qcom: clk-krait: add apq/ipq8064 errata workaround
> > >    clk: qcom: clk-krait: add enable disable ops
> > >    dt-bindings: clock: Convert qcom,krait-cc to yaml
> > >    dt-bindings: clock: Add L2 clocks to qcom,krait-cc Documentation
> > >    dt-bindings: arm: msm: Convert kpss-acc driver Documentation to yaml
> > >    dt-bindings: arm: msm: Convert kpss-gcc driver Documentation to yaml
> > >    ARM: dts: qcom: qcom-ipq8064: add missing krait-cc compatible and
> > >      clocks
> > >
> > >   .../bindings/arm/msm/qcom,kpss-acc.txt        |  49 -----
> > >   .../bindings/arm/msm/qcom,kpss-acc.yaml       |  94 +++++++++
> > >   .../bindings/arm/msm/qcom,kpss-gcc.txt        |  44 -----
> > >   .../bindings/arm/msm/qcom,kpss-gcc.yaml       |  69 +++++++
> > >   .../bindings/clock/qcom,krait-cc.txt          |  34 ----
> > >   .../bindings/clock/qcom,krait-cc.yaml         |  65 ++++++
> > >   arch/arm/boot/dts/qcom-ipq8064.dtsi           |  24 ++-
> > >   drivers/clk/clk.c                             |  14 ++
> > >   drivers/clk/qcom/clk-hfpll.c                  |  13 +-
> > >   drivers/clk/qcom/clk-krait.c                  |  44 ++++-
> > >   drivers/clk/qcom/clk-krait.h                  |   1 +
> > >   drivers/clk/qcom/gcc-ipq806x.c                |  27 ++-
> > >   drivers/clk/qcom/kpss-xcc.c                   |  25 +--
> > >   drivers/clk/qcom/krait-cc.c                   | 186 ++++++++++--------
> > >   include/linux/clk-provider.h                  |   1 +
> > >   15 files changed, 453 insertions(+), 237 deletions(-)
> > >   delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.txt
> > >   create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml
> > >   delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
> > >   create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
> > >   delete mode 100644 Documentation/devicetree/bindings/clock/qcom,krait-cc.txt
> > >   create mode 100644 Documentation/devicetree/bindings/clock/qcom,krait-cc.yaml
> > >
> >
> >
> > --
> > With best wishes
> > Dmitry
>
> --
>         Ansuel