Re: [RFC PATCH v3 4/7] arm: dts: exynos: Add interconnect bindings for Exynos4412

[Artur ÅwigoÅ]

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@xxxxxxxxxxx>
> > > > ---
> > > >  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,
-- 
Artur ÅwigoÅ
Samsung R&D Institute Poland
Samsung Electronics