[RFC,v3,4/7] arm: dts: exynos: Add interconnect bindings for Exynos4412
diff mbox series

Message ID 20191220115653.6487-5-a.swigon@samsung.com
State Under Review
Headers show
Series
  • [RFC,v3,1/7] interconnect: Export of_icc_get_from_provider()
Related show

Commit Message

Artur Świgoń Dec. 20, 2019, 11:56 a.m. UTC
This patch adds the following properties to the Exynos4412 DT:
  - exynos,interconnect-parent-node: to declare connections between
    nodes in order to guarantee PM QoS requirements between nodes;
  - #interconnect-cells: required by the interconnect framework.

Note that #interconnect-cells is always zero and node IDs are not
hardcoded anywhere.

Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
---
 arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Chanwoo Choi Dec. 21, 2019, 8 p.m. UTC | #1
Hi,

On Fri, Dec 20, 2019 at 9:02 PM Artur Świgoń <a.swigon@samsung.com> wrote:
>
> This patch adds the following properties to the Exynos4412 DT:
>   - exynos,interconnect-parent-node: to declare connections between
>     nodes in order to guarantee PM QoS requirements between nodes;
>   - #interconnect-cells: required by the interconnect framework.
>
> Note that #interconnect-cells is always zero and node IDs are not
> hardcoded anywhere.
>
> Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
> ---
>  arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> index 4ce3d77a6704..d9d70eacfcaf 100644
> --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> @@ -90,6 +90,7 @@
>  &bus_dmc {
>         exynos,ppmu-device = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>;
>         vdd-supply = <&buck1_reg>;
> +       #interconnect-cells = <0>;
>         status = "okay";
>  };
>
> @@ -106,6 +107,8 @@
>  &bus_leftbus {
>         exynos,ppmu-device = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>;
>         vdd-supply = <&buck3_reg>;
> +       exynos,interconnect-parent-node = <&bus_dmc>;
> +       #interconnect-cells = <0>;
>         status = "okay";
>  };
>
> @@ -116,6 +119,8 @@
>
>  &bus_display {
>         exynos,parent-bus = <&bus_leftbus>;
> +       exynos,interconnect-parent-node = <&bus_leftbus>;
> +       #interconnect-cells = <0>;
>         status = "okay";
>  };
>
> --
> 2.17.1
>

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

I has not yet tested on target. I'll test it on next week
and then reply the test result.
Krzysztof Kozlowski Dec. 30, 2019, 3:44 p.m. UTC | #2
On Fri, Dec 20, 2019 at 12:56:50PM +0100, Artur Świgoń wrote:
> This patch adds the following properties to the Exynos4412 DT:
>   - exynos,interconnect-parent-node: to declare connections between
>     nodes in order to guarantee PM QoS requirements between nodes;
>   - #interconnect-cells: required by the interconnect framework.
> 
> Note that #interconnect-cells is always zero and node IDs are not
> hardcoded anywhere.
> 
> Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
> ---
>  arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)

The order of patches is confusing. Patches 4 and 6 are split - do the
depend on 5? I doubt but...

Adjust the title to match the contents - you are not adding bindings but
properties to bus nodes. Also the prefix is ARM: (look at recent
commits).

> 
> diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> index 4ce3d77a6704..d9d70eacfcaf 100644
> --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> @@ -90,6 +90,7 @@
>  &bus_dmc {
>  	exynos,ppmu-device = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>;
>  	vdd-supply = <&buck1_reg>;
> +	#interconnect-cells = <0>;

This does not look like property of Odroid but Exynos4412 or Exynos4.

Best regards,
Krzysztof
Artur Świgoń Dec. 31, 2019, 7:18 a.m. UTC | #3
Hi,

On Mon, 2019-12-30 at 16:44 +0100, Krzysztof Kozlowski wrote:
> On Fri, Dec 20, 2019 at 12:56:50PM +0100, Artur Świgoń wrote:
> > This patch adds the following properties to the Exynos4412 DT:
> >   - exynos,interconnect-parent-node: to declare connections between
> >     nodes in order to guarantee PM QoS requirements between nodes;
> >   - #interconnect-cells: required by the interconnect framework.
> > 
> > Note that #interconnect-cells is always zero and node IDs are not
> > hardcoded anywhere.
> > 
> > Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
> > ---
> >  arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 5 +++++
> >  1 file changed, 5 insertions(+)
> 
> The order of patches is confusing. Patches 4 and 6 are split - do the
> depend on 5? I doubt but...

Let me elaborate:

The order of the patches in this series is such that every subsequent
patch adds some functionality (and, of course, applying patches one-by-one
yields a working kernel at every step). Specifically for patches 04--07:

 -- patch 04 adds interconnect _provider_ properties for Exynos4412;
 -- patch 05 implements interconnect provider logic (depends on patch 04);
 -- patch 06 adds interconnect _consumer_ properties for Exynos4412 mixer;
 -- patch 07 implements interconnect consumer logic (depends on patches
    05 & 06);

My reasoning is that this order allows to e.g., merge the interconnect
provider for exynos-bus and leave the consumers for later (not limited to
the mixer). I hope this makes sense.

> Adjust the title to match the contents - you are not adding bindings but
> properties to bus nodes. Also the prefix is ARM: (look at recent
> commits).

OK.

> > 
> > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > index 4ce3d77a6704..d9d70eacfcaf 100644
> > --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > @@ -90,6 +90,7 @@
> >  &bus_dmc {
> >  	exynos,ppmu-device = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>;
> >  	vdd-supply = <&buck1_reg>;
> > +	#interconnect-cells = <0>;
> 
> This does not look like property of Odroid but Exynos4412 or Exynos4.

Strangely enough, this file is where the 'exynos,parent-bus' (aka. 'devfreq')
properties are located (and everything in this RFC concerns devfreq).

Regards,
Krzysztof Kozlowski Dec. 31, 2019, 9:22 a.m. UTC | #4
On Tue, Dec 31, 2019 at 08:18:01AM +0100, Artur Świgoń wrote:
> Hi,
> 
> On Mon, 2019-12-30 at 16:44 +0100, Krzysztof Kozlowski wrote:
> > On Fri, Dec 20, 2019 at 12:56:50PM +0100, Artur Świgoń wrote:
> > > This patch adds the following properties to the Exynos4412 DT:
> > >   - exynos,interconnect-parent-node: to declare connections between
> > >     nodes in order to guarantee PM QoS requirements between nodes;
> > >   - #interconnect-cells: required by the interconnect framework.
> > > 
> > > Note that #interconnect-cells is always zero and node IDs are not
> > > hardcoded anywhere.
> > > 
> > > Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
> > > ---
> > >  arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > 
> > The order of patches is confusing. Patches 4 and 6 are split - do the
> > depend on 5? I doubt but...
> 
> Let me elaborate:
> 
> The order of the patches in this series is such that every subsequent
> patch adds some functionality (and, of course, applying patches one-by-one
> yields a working kernel at every step). Specifically for patches 04--07:
> 
>  -- patch 04 adds interconnect _provider_ properties for Exynos4412;
>  -- patch 05 implements interconnect provider logic (depends on patch 04);
>  -- patch 06 adds interconnect _consumer_ properties for Exynos4412 mixer;
>  -- patch 07 implements interconnect consumer logic (depends on patches
>     05 & 06);
> 
> My reasoning is that this order allows to e.g., merge the interconnect
> provider for exynos-bus and leave the consumers for later (not limited to
> the mixer). I hope this makes sense.

It is wrong. The driver should not depend on DTS changes because:
1. DTS always go through separate branch and tree, so last patch
   will have to wait up to 3 cycles (!!!),
2. You break backward compatibility.

In certain cases dependency on DTS changes is ok:
1. Cleaning up deprecated properties,
2. Ignoring the backward compatibility for e.g. new platforms.

None of these are applicable here.

You need to rework it, put DTS changes at the end. This clearly shows
that there is no wrong dependency.

> 
> > Adjust the title to match the contents - you are not adding bindings but
> > properties to bus nodes. Also the prefix is ARM: (look at recent
> > commits).
> 
> OK.
> 
> > > 
> > > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > index 4ce3d77a6704..d9d70eacfcaf 100644
> > > --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > @@ -90,6 +90,7 @@
> > >  &bus_dmc {
> > >  	exynos,ppmu-device = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>;
> > >  	vdd-supply = <&buck1_reg>;
> > > +	#interconnect-cells = <0>;
> > 
> > This does not look like property of Odroid but Exynos4412 or Exynos4.
> 
> Strangely enough, this file is where the 'exynos,parent-bus' (aka. 'devfreq')
> properties are located (and everything in this RFC concerns devfreq).

I cannot find exynos,parent-bus in exynos4412-odroid-common.dtsi. Can
you elaborate?

Best regards,
Krzysztof
Artur Świgoń Dec. 31, 2019, 9:41 a.m. UTC | #5
On Tue, 2019-12-31 at 10:22 +0100, Krzysztof Kozlowski wrote:
> On Tue, Dec 31, 2019 at 08:18:01AM +0100, Artur Świgoń wrote:
> > Hi,
> > 
> > On Mon, 2019-12-30 at 16:44 +0100, Krzysztof Kozlowski wrote:
> > > On Fri, Dec 20, 2019 at 12:56:50PM +0100, Artur Świgoń wrote:
> > > > This patch adds the following properties to the Exynos4412 DT:
> > > >   - exynos,interconnect-parent-node: to declare connections between
> > > >     nodes in order to guarantee PM QoS requirements between nodes;
> > > >   - #interconnect-cells: required by the interconnect framework.
> > > > 
> > > > Note that #interconnect-cells is always zero and node IDs are not
> > > > hardcoded anywhere.
> > > > 
> > > > Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
> > > > ---
> > > >  arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > 
> > > The order of patches is confusing. Patches 4 and 6 are split - do the
> > > depend on 5? I doubt but...
> > 
> > Let me elaborate:
> > 
> > The order of the patches in this series is such that every subsequent
> > patch adds some functionality (and, of course, applying patches one-by-one
> > yields a working kernel at every step). Specifically for patches 04--07:
> > 
> >  -- patch 04 adds interconnect _provider_ properties for Exynos4412;
> >  -- patch 05 implements interconnect provider logic (depends on patch 04);
> >  -- patch 06 adds interconnect _consumer_ properties for Exynos4412 mixer;
> >  -- patch 07 implements interconnect consumer logic (depends on patches
> >     05 & 06);
> > 
> > My reasoning is that this order allows to e.g., merge the interconnect
> > provider for exynos-bus and leave the consumers for later (not limited to
> > the mixer). I hope this makes sense.
> 
> It is wrong. The driver should not depend on DTS changes because:
> 1. DTS always go through separate branch and tree, so last patch
>    will have to wait up to 3 cycles (!!!),
> 2. You break backward compatibility.

It is up to the definition of "depends". The driver is _not_ broken without
the DTS patches, but the interconnect functionality will not be available.

The only requirement is that if we want to have a working interconnect
consumer, there needs to be a working interconnet provider (and I used
the word "depends" to specify what needs what in order to work as intended).

I still think the order of these patches is the most logical one for someone
reading this RFC as a whole.

> In certain cases dependency on DTS changes is ok:
> 1. Cleaning up deprecated properties,
> 2. Ignoring the backward compatibility for e.g. new platforms.
> 
> None of these are applicable here.
> 
> You need to rework it, put DTS changes at the end. This clearly shows
> that there is no wrong dependency.
> 
> > 
> > > Adjust the title to match the contents - you are not adding bindings but
> > > properties to bus nodes. Also the prefix is ARM: (look at recent
> > > commits).
> > 
> > OK.
> > 
> > > > 
> > > > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > > index 4ce3d77a6704..d9d70eacfcaf 100644
> > > > --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > > +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > > @@ -90,6 +90,7 @@
> > > >  &bus_dmc {
> > > >  	exynos,ppmu-device = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>;
> > > >  	vdd-supply = <&buck1_reg>;
> > > > +	#interconnect-cells = <0>;
> > > 
> > > This does not look like property of Odroid but Exynos4412 or Exynos4.
> > 
> > Strangely enough, this file is where the 'exynos,parent-bus' (aka. 'devfreq')
> > properties are located (and everything in this RFC concerns devfreq).
> 
> I cannot find exynos,parent-bus in exynos4412-odroid-common.dtsi. Can
> you elaborate?

Currently a name change is being made: 'devfreq' -> 'exynos,parent-bus'
https://patchwork.kernel.org/patch/11304549/
(a dependency of this RFC; also available in devfreq-testing branch)

Best regards,
Krzysztof Kozlowski Dec. 31, 2019, 10:02 a.m. UTC | #6
On Tue, Dec 31, 2019 at 10:41:47AM +0100, Artur Świgoń wrote:
> On Tue, 2019-12-31 at 10:22 +0100, Krzysztof Kozlowski wrote:
> > On Tue, Dec 31, 2019 at 08:18:01AM +0100, Artur Świgoń wrote:
> > > Hi,
> > > 
> > > On Mon, 2019-12-30 at 16:44 +0100, Krzysztof Kozlowski wrote:
> > > > On Fri, Dec 20, 2019 at 12:56:50PM +0100, Artur Świgoń wrote:
> > > > > This patch adds the following properties to the Exynos4412 DT:
> > > > >   - exynos,interconnect-parent-node: to declare connections between
> > > > >     nodes in order to guarantee PM QoS requirements between nodes;
> > > > >   - #interconnect-cells: required by the interconnect framework.
> > > > > 
> > > > > Note that #interconnect-cells is always zero and node IDs are not
> > > > > hardcoded anywhere.
> > > > > 
> > > > > Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
> > > > > ---
> > > > >  arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 5 +++++
> > > > >  1 file changed, 5 insertions(+)
> > > > 
> > > > The order of patches is confusing. Patches 4 and 6 are split - do the
> > > > depend on 5? I doubt but...
> > > 
> > > Let me elaborate:
> > > 
> > > The order of the patches in this series is such that every subsequent
> > > patch adds some functionality (and, of course, applying patches one-by-one
> > > yields a working kernel at every step). Specifically for patches 04--07:
> > > 
> > >  -- patch 04 adds interconnect _provider_ properties for Exynos4412;
> > >  -- patch 05 implements interconnect provider logic (depends on patch 04);
> > >  -- patch 06 adds interconnect _consumer_ properties for Exynos4412 mixer;
> > >  -- patch 07 implements interconnect consumer logic (depends on patches
> > >     05 & 06);
> > > 
> > > My reasoning is that this order allows to e.g., merge the interconnect
> > > provider for exynos-bus and leave the consumers for later (not limited to
> > > the mixer). I hope this makes sense.
> > 
> > It is wrong. The driver should not depend on DTS changes because:
> > 1. DTS always go through separate branch and tree, so last patch
> >    will have to wait up to 3 cycles (!!!),
> > 2. You break backward compatibility.
> 
> It is up to the definition of "depends". The driver is _not_ broken without
> the DTS patches, but the interconnect functionality will not be available.
> 
> The only requirement is that if we want to have a working interconnect
> consumer, there needs to be a working interconnet provider (and I used
> the word "depends" to specify what needs what in order to work as intended).
> 

The order of patches should reflect first of all real dependency.
Whether it compiles, works at all and does not break anything.  Logical
dependency of "when the feature will start working" is
irrelevant to DTS because DTS goes in separate way and driver is
independent of it.

> I still think the order of these patches is the most logical one for someone
> reading this RFC as a whole.

I am sorry but it brings only confusion. DTS is orthogonal of the
driver code. You could even post the patchset without DTS (although then
it would raise questions where is the user of it, but still, you
could).

Further, DTS describes also hardware so you could send certain DTS
patches without driver implementation to describe the hardware.

Driver code and DTS are kind of different worlds so mixing them up for
logical review does not really make any sense.

Not mentioning it is different than most of other patches on mailing
lists.

BTW, it is the same as bindings which should (almost) always go first as
separate patches.

> 
> > In certain cases dependency on DTS changes is ok:
> > 1. Cleaning up deprecated properties,
> > 2. Ignoring the backward compatibility for e.g. new platforms.
> > 
> > None of these are applicable here.
> > 
> > You need to rework it, put DTS changes at the end. This clearly shows
> > that there is no wrong dependency.
> > 
> > > 
> > > > Adjust the title to match the contents - you are not adding bindings but
> > > > properties to bus nodes. Also the prefix is ARM: (look at recent
> > > > commits).
> > > 
> > > OK.
> > > 
> > > > > 
> > > > > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > > > index 4ce3d77a6704..d9d70eacfcaf 100644
> > > > > --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > > > +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > > > @@ -90,6 +90,7 @@
> > > > >  &bus_dmc {
> > > > >  	exynos,ppmu-device = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>;
> > > > >  	vdd-supply = <&buck1_reg>;
> > > > > +	#interconnect-cells = <0>;
> > > > 
> > > > This does not look like property of Odroid but Exynos4412 or Exynos4.
> > > 
> > > Strangely enough, this file is where the 'exynos,parent-bus' (aka. 'devfreq')
> > > properties are located (and everything in this RFC concerns devfreq).
> > 
> > I cannot find exynos,parent-bus in exynos4412-odroid-common.dtsi. Can
> > you elaborate?
> 
> Currently a name change is being made: 'devfreq' -> 'exynos,parent-bus'
> https://patchwork.kernel.org/patch/11304549/
> (a dependency of this RFC; also available in devfreq-testing branch)

I see. That property also does not look like board (Odroid) specific so
it should be moved to Exynos4412 DTSI.

Best regards,
Krzysztof
Artur Świgoń Dec. 31, 2019, 10:23 a.m. UTC | #7
On Tue, 2019-12-31 at 11:02 +0100, Krzysztof Kozlowski wrote:
> On Tue, Dec 31, 2019 at 10:41:47AM +0100, Artur Świgoń wrote:
> > On Tue, 2019-12-31 at 10:22 +0100, Krzysztof Kozlowski wrote:
> > > On Tue, Dec 31, 2019 at 08:18:01AM +0100, Artur Świgoń wrote:
> > > > Hi,
> > > > 
> > > > On Mon, 2019-12-30 at 16:44 +0100, Krzysztof Kozlowski wrote:
> > > > > On Fri, Dec 20, 2019 at 12:56:50PM +0100, Artur Świgoń wrote:
> > > > > > This patch adds the following properties to the Exynos4412 DT:
> > > > > >   - exynos,interconnect-parent-node: to declare connections between
> > > > > >     nodes in order to guarantee PM QoS requirements between nodes;
> > > > > >   - #interconnect-cells: required by the interconnect framework.
> > > > > > 
> > > > > > Note that #interconnect-cells is always zero and node IDs are not
> > > > > > hardcoded anywhere.
> > > > > > 
> > > > > > Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
> > > > > > ---
> > > > > >  arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 5 +++++
> > > > > >  1 file changed, 5 insertions(+)
> > > > > 
> > > > > The order of patches is confusing. Patches 4 and 6 are split - do the
> > > > > depend on 5? I doubt but...
> > > > 
> > > > Let me elaborate:
> > > > 
> > > > The order of the patches in this series is such that every subsequent
> > > > patch adds some functionality (and, of course, applying patches one-by-one
> > > > yields a working kernel at every step). Specifically for patches 04--07:
> > > > 
> > > >  -- patch 04 adds interconnect _provider_ properties for Exynos4412;
> > > >  -- patch 05 implements interconnect provider logic (depends on patch 04);
> > > >  -- patch 06 adds interconnect _consumer_ properties for Exynos4412 mixer;
> > > >  -- patch 07 implements interconnect consumer logic (depends on patches
> > > >     05 & 06);
> > > > 
> > > > My reasoning is that this order allows to e.g., merge the interconnect
> > > > provider for exynos-bus and leave the consumers for later (not limited to
> > > > the mixer). I hope this makes sense.
> > > 
> > > It is wrong. The driver should not depend on DTS changes because:
> > > 1. DTS always go through separate branch and tree, so last patch
> > >    will have to wait up to 3 cycles (!!!),
> > > 2. You break backward compatibility.
> > 
> > It is up to the definition of "depends". The driver is _not_ broken without
> > the DTS patches, but the interconnect functionality will not be available.
> > 
> > The only requirement is that if we want to have a working interconnect
> > consumer, there needs to be a working interconnet provider (and I used
> > the word "depends" to specify what needs what in order to work as intended).
> > 
> 
> The order of patches should reflect first of all real dependency.
> Whether it compiles, works at all and does not break anything.  Logical
> dependency of "when the feature will start working" is
> irrelevant to DTS because DTS goes in separate way and driver is
> independent of it.

The order of patches does indeed reflect real dependency. I can also reorder
them (preserving the dependencies) so that DTS patches go first in the series
if this is the more preferred way.

> > I still think the order of these patches is the most logical one for someone
> > reading this RFC as a whole.
> 
> I am sorry but it brings only confusion. DTS is orthogonal of the
> driver code. You could even post the patchset without DTS (although then
> it would raise questions where is the user of it, but still, you
> could).
> 
> Further, DTS describes also hardware so you could send certain DTS
> patches without driver implementation to describe the hardware.
> 
> Driver code and DTS are kind of different worlds so mixing them up for
> logical review does not really make any sense.
> 
> Not mentioning it is different than most of other patches on mailing
> lists.
> 
> BTW, it is the same as bindings which should (almost) always go first as
> separate patches.

Thanks for elaborating on this, I appreciate it.
Regarding your original concern, patches 04 & 06 are separate for several
reasons, one of which is that they are related to two different drivers
(exynos-bus vs. exynos-mixer).

> > 
> > > In certain cases dependency on DTS changes is ok:
> > > 1. Cleaning up deprecated properties,
> > > 2. Ignoring the backward compatibility for e.g. new platforms.
> > > 
> > > None of these are applicable here.
> > > 
> > > You need to rework it, put DTS changes at the end. This clearly shows
> > > that there is no wrong dependency.
> > > 
> > > > 
> > > > > Adjust the title to match the contents - you are not adding bindings but
> > > > > properties to bus nodes. Also the prefix is ARM: (look at recent
> > > > > commits).
> > > > 
> > > > OK.
> > > > 
> > > > > > 
> > > > > > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > > > > index 4ce3d77a6704..d9d70eacfcaf 100644
> > > > > > --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > > > > +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > > > > @@ -90,6 +90,7 @@
> > > > > >  &bus_dmc {
> > > > > >  	exynos,ppmu-device = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>;
> > > > > >  	vdd-supply = <&buck1_reg>;
> > > > > > +	#interconnect-cells = <0>;
> > > > > 
> > > > > This does not look like property of Odroid but Exynos4412 or Exynos4.
> > > > 
> > > > Strangely enough, this file is where the 'exynos,parent-bus' (aka. 'devfreq')
> > > > properties are located (and everything in this RFC concerns devfreq).
> > > 
> > > I cannot find exynos,parent-bus in exynos4412-odroid-common.dtsi. Can
> > > you elaborate?
> > 
> > Currently a name change is being made: 'devfreq' -> 'exynos,parent-bus'
> > https://patchwork.kernel.org/patch/11304549/
> > (a dependency of this RFC; also available in devfreq-testing branch)
> 
> I see. That property also does not look like board (Odroid) specific so
> it should be moved to Exynos4412 DTSI.

Makes sense to me. Just from looking at the patch I referenced above, there is
a significant level of code duplication between
* arch/arm/boot/dts/exynos4412-itop-scp-core.dtsi
* arch/arm/boot/dts/exynos4412-midas.dtsi
* arch/arm/boot/dts/exynos4412-odroid-common.dtsi
with relation to the devfreq*/exynos,* properties.
Krzysztof Kozlowski Dec. 31, 2019, 10:38 a.m. UTC | #8
On Tue, 31 Dec 2019 at 11:23, Artur Świgoń <a.swigon@samsung.com> wrote:
> >
> > The order of patches should reflect first of all real dependency.
> > Whether it compiles, works at all and does not break anything.  Logical
> > dependency of "when the feature will start working" is
> > irrelevant to DTS because DTS goes in separate way and driver is
> > independent of it.
>
> The order of patches does indeed reflect real dependency. I can also reorder
> them (preserving the dependencies) so that DTS patches go first in the series
> if this is the more preferred way.

It looks wrong then. Driver should not depend on DTS. I cannot find
the patch changing bindings (should be first in patchset) which could
also point to this problem.

It seems you added requirement for interconnect properties while it
should be rather optional.

> > > I still think the order of these patches is the most logical one for someone
> > > reading this RFC as a whole.
> >
> > I am sorry but it brings only confusion. DTS is orthogonal of the
> > driver code. You could even post the patchset without DTS (although then
> > it would raise questions where is the user of it, but still, you
> > could).
> >
> > Further, DTS describes also hardware so you could send certain DTS
> > patches without driver implementation to describe the hardware.
> >
> > Driver code and DTS are kind of different worlds so mixing them up for
> > logical review does not really make any sense.
> >
> > Not mentioning it is different than most of other patches on mailing
> > lists.
> >
> > BTW, it is the same as bindings which should (almost) always go first as
> > separate patches.
>
> Thanks for elaborating on this, I appreciate it.
> Regarding your original concern, patches 04 & 06 are separate for several
> reasons, one of which is that they are related to two different drivers
> (exynos-bus vs. exynos-mixer).

It's okay then (for them to be split).

>
> > >
> > > > In certain cases dependency on DTS changes is ok:
> > > > 1. Cleaning up deprecated properties,
> > > > 2. Ignoring the backward compatibility for e.g. new platforms.
> > > >
> > > > None of these are applicable here.
> > > >
> > > > You need to rework it, put DTS changes at the end. This clearly shows
> > > > that there is no wrong dependency.
> > > >
> > > > >
> > > > > > Adjust the title to match the contents - you are not adding bindings but
> > > > > > properties to bus nodes. Also the prefix is ARM: (look at recent
> > > > > > commits).
> > > > >
> > > > > OK.
> > > > >
> > > > > > >
> > > > > > > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > > > > > index 4ce3d77a6704..d9d70eacfcaf 100644
> > > > > > > --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > > > > > +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > > > > > @@ -90,6 +90,7 @@
> > > > > > >  &bus_dmc {
> > > > > > >     exynos,ppmu-device = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>;
> > > > > > >     vdd-supply = <&buck1_reg>;
> > > > > > > +   #interconnect-cells = <0>;
> > > > > >
> > > > > > This does not look like property of Odroid but Exynos4412 or Exynos4.
> > > > >
> > > > > Strangely enough, this file is where the 'exynos,parent-bus' (aka. 'devfreq')
> > > > > properties are located (and everything in this RFC concerns devfreq).
> > > >
> > > > I cannot find exynos,parent-bus in exynos4412-odroid-common.dtsi. Can
> > > > you elaborate?
> > >
> > > Currently a name change is being made: 'devfreq' -> 'exynos,parent-bus'
> > > https://patchwork.kernel.org/patch/11304549/
> > > (a dependency of this RFC; also available in devfreq-testing branch)
> >
> > I see. That property also does not look like board (Odroid) specific so
> > it should be moved to Exynos4412 DTSI.
>
> Makes sense to me. Just from looking at the patch I referenced above, there is
> a significant level of code duplication between
> * arch/arm/boot/dts/exynos4412-itop-scp-core.dtsi
> * arch/arm/boot/dts/exynos4412-midas.dtsi
> * arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> with relation to the devfreq*/exynos,* properties.

If you have in mind all the nodes with "status=okay", it's fine to
duplicate them.

Best regards,
Krzysztof
Artur Świgoń Dec. 31, 2019, 11:03 a.m. UTC | #9
On Tue, 2019-12-31 at 11:38 +0100, Krzysztof Kozlowski wrote:
> On Tue, 31 Dec 2019 at 11:23, Artur Świgoń <a.swigon@samsung.com> wrote:
> > > 
> > > The order of patches should reflect first of all real dependency.
> > > Whether it compiles, works at all and does not break anything.  Logical
> > > dependency of "when the feature will start working" is
> > > irrelevant to DTS because DTS goes in separate way and driver is
> > > independent of it.
> > 
> > The order of patches does indeed reflect real dependency. I can also reorder
> > them (preserving the dependencies) so that DTS patches go first in the series
> > if this is the more preferred way.
> 
> It looks wrong then. Driver should not depend on DTS. I cannot find
> the patch changing bindings (should be first in patchset) which could
> also point to this problem.
> 
> It seems you added requirement for interconnect properties while it
> should be rather optional.

No, there is no requirement for interconnect properties (other than that it
simply does not make any sense to use the interconnect driver code and not the
DTS properties for it in the long run).

In case of the exynos-bus driver (code: patch 05, DTS: patch 04) if the DTS
properties ('exynos,interconnect-parent-node') are missing, the new code handles
it gracefully returning NULL from exynos_bus_icc_get_parent() (it is not an
error condition).

In case of the exynos-mixer driver (code: patch 07, DTS: patch 06) if the DTS
property ('interconnects') is missing, of_icc_get() returns NULL and the code does
not try to set any contraints for a NULL path. Same thing happens if
CONFIG_INTERCONNECT is 'n'.

The only case when something breaks is when you try to use the interconnect
consumer (implemented in patches 06 & 07) when there is no interconnect provider
(patches 04 & 05), in which case of_icc_get() returns an error (since it cannot
find a path). From what I understand, it probably makes sense to merge any
interconnect consumers one cycle later than the provider.

> > > > I still think the order of these patches is the most logical one for someone
> > > > reading this RFC as a whole.
> > > 
> > > I am sorry but it brings only confusion. DTS is orthogonal of the
> > > driver code. You could even post the patchset without DTS (although then
> > > it would raise questions where is the user of it, but still, you
> > > could).
> > > 
> > > Further, DTS describes also hardware so you could send certain DTS
> > > patches without driver implementation to describe the hardware.
> > > 
> > > Driver code and DTS are kind of different worlds so mixing them up for
> > > logical review does not really make any sense.
> > > 
> > > Not mentioning it is different than most of other patches on mailing
> > > lists.
> > > 
> > > BTW, it is the same as bindings which should (almost) always go first as
> > > separate patches.
> > 
> > Thanks for elaborating on this, I appreciate it.
> > Regarding your original concern, patches 04 & 06 are separate for several
> > reasons, one of which is that they are related to two different drivers
> > (exynos-bus vs. exynos-mixer).
> 
> It's okay then (for them to be split).
> 
> > 
> > > > 
> > > > > In certain cases dependency on DTS changes is ok:
> > > > > 1. Cleaning up deprecated properties,
> > > > > 2. Ignoring the backward compatibility for e.g. new platforms.
> > > > > 
> > > > > None of these are applicable here.
> > > > > 
> > > > > You need to rework it, put DTS changes at the end. This clearly shows
> > > > > that there is no wrong dependency.
> > > > > 
> > > > > > 
> > > > > > > Adjust the title to match the contents - you are not adding bindings but
> > > > > > > properties to bus nodes. Also the prefix is ARM: (look at recent
> > > > > > > commits).
> > > > > > 
> > > > > > OK.
> > > > > > 
> > > > > > > > 
> > > > > > > > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > > > > > > index 4ce3d77a6704..d9d70eacfcaf 100644
> > > > > > > > --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > > > > > > +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > > > > > > @@ -90,6 +90,7 @@
> > > > > > > >  &bus_dmc {
> > > > > > > >     exynos,ppmu-device = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>;
> > > > > > > >     vdd-supply = <&buck1_reg>;
> > > > > > > > +   #interconnect-cells = <0>;
> > > > > > > 
> > > > > > > This does not look like property of Odroid but Exynos4412 or Exynos4.
> > > > > > 
> > > > > > Strangely enough, this file is where the 'exynos,parent-bus' (aka. 'devfreq')
> > > > > > properties are located (and everything in this RFC concerns devfreq).
> > > > > 
> > > > > I cannot find exynos,parent-bus in exynos4412-odroid-common.dtsi. Can
> > > > > you elaborate?
> > > > 
> > > > Currently a name change is being made: 'devfreq' -> 'exynos,parent-bus'
> > > > https://patchwork.kernel.org/patch/11304549/
> > > > (a dependency of this RFC; also available in devfreq-testing branch)
> > > 
> > > I see. That property also does not look like board (Odroid) specific so
> > > it should be moved to Exynos4412 DTSI.
> > 
> > Makes sense to me. Just from looking at the patch I referenced above, there is
> > a significant level of code duplication between
> > * arch/arm/boot/dts/exynos4412-itop-scp-core.dtsi
> > * arch/arm/boot/dts/exynos4412-midas.dtsi
> > * arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > with relation to the devfreq*/exynos,* properties.
> 
> If you have in mind all the nodes with "status=okay", it's fine to
> duplicate them.
OK.

Regards,
Georgi Djakov Jan. 22, 2020, 4:54 p.m. UTC | #10
Hi Artur,

Thank you for your continuous work on this.

On 12/20/19 13:56, Artur Świgoń wrote:
> This patch adds the following properties to the Exynos4412 DT:
>   - exynos,interconnect-parent-node: to declare connections between
>     nodes in order to guarantee PM QoS requirements between nodes;

Is this DT property documented somewhere? I believe that there should be a patch
to document it somewhere in Documentation/devicetree/bindings/ before using it.

Thanks,
Georgi

>   - #interconnect-cells: required by the interconnect framework.
> 
> Note that #interconnect-cells is always zero and node IDs are not
> hardcoded anywhere.
> 
> Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
> ---
>  arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> index 4ce3d77a6704..d9d70eacfcaf 100644
> --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> @@ -90,6 +90,7 @@
>  &bus_dmc {
>  	exynos,ppmu-device = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>;
>  	vdd-supply = <&buck1_reg>;
> +	#interconnect-cells = <0>;
>  	status = "okay";
>  };
>  
> @@ -106,6 +107,8 @@
>  &bus_leftbus {
>  	exynos,ppmu-device = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>;
>  	vdd-supply = <&buck3_reg>;
> +	exynos,interconnect-parent-node = <&bus_dmc>;
> +	#interconnect-cells = <0>;
>  	status = "okay";
>  };
>  
> @@ -116,6 +119,8 @@
>  
>  &bus_display {
>  	exynos,parent-bus = <&bus_leftbus>;
> +	exynos,interconnect-parent-node = <&bus_leftbus>;
> +	#interconnect-cells = <0>;
>  	status = "okay";
>  };
>  
>
Artur Świgoń Jan. 24, 2020, 11:22 a.m. UTC | #11
Hi,

On Wed, 2020-01-22 at 18:54 +0200, Georgi Djakov wrote:
> Hi Artur,
> 
> Thank you for your continuous work on this.
> 
> On 12/20/19 13:56, Artur Świgoń wrote:
> > This patch adds the following properties to the Exynos4412 DT:
> >   - exynos,interconnect-parent-node: to declare connections between
> >     nodes in order to guarantee PM QoS requirements between nodes;
> 
> Is this DT property documented somewhere? I believe that there should be a patch
> to document it somewhere in Documentation/devicetree/bindings/ before using it.

It will be documented in Documentation/devicetree/bindings/devfreq/exynos-bus.txt
in the next version.

> >   - #interconnect-cells: required by the interconnect framework.
> > 
> > Note that #interconnect-cells is always zero and node IDs are not
> > hardcoded anywhere.
> > 
> > Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
> > ---
> >  arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > index 4ce3d77a6704..d9d70eacfcaf 100644
> > --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > @@ -90,6 +90,7 @@
> >  &bus_dmc {
> >  	exynos,ppmu-device = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>;
> >  	vdd-supply = <&buck1_reg>;
> > +	#interconnect-cells = <0>;
> >  	status = "okay";
> >  };
> >  
> > @@ -106,6 +107,8 @@
> >  &bus_leftbus {
> >  	exynos,ppmu-device = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>;
> >  	vdd-supply = <&buck3_reg>;
> > +	exynos,interconnect-parent-node = <&bus_dmc>;
> > +	#interconnect-cells = <0>;
> >  	status = "okay";
> >  };
> >  
> > @@ -116,6 +119,8 @@
> >  
> >  &bus_display {
> >  	exynos,parent-bus = <&bus_leftbus>;
> > +	exynos,interconnect-parent-node = <&bus_leftbus>;
> > +	#interconnect-cells = <0>;
> >  	status = "okay";
> >  };

Patch
diff mbox series

diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
index 4ce3d77a6704..d9d70eacfcaf 100644
--- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
+++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
@@ -90,6 +90,7 @@ 
 &bus_dmc {
 	exynos,ppmu-device = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>;
 	vdd-supply = <&buck1_reg>;
+	#interconnect-cells = <0>;
 	status = "okay";
 };
 
@@ -106,6 +107,8 @@ 
 &bus_leftbus {
 	exynos,ppmu-device = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>;
 	vdd-supply = <&buck3_reg>;
+	exynos,interconnect-parent-node = <&bus_dmc>;
+	#interconnect-cells = <0>;
 	status = "okay";
 };
 
@@ -116,6 +119,8 @@ 
 
 &bus_display {
 	exynos,parent-bus = <&bus_leftbus>;
+	exynos,interconnect-parent-node = <&bus_leftbus>;
+	#interconnect-cells = <0>;
 	status = "okay";
 };