Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <99427c18b1fcca3bc21e69609500abdbbef59167.camel@samsung.com>
Subject: Re: [RFC PATCH v3 4/7] arm: dts: exynos: Add interconnect bindings
 for Exynos4412
From:   Artur =?UTF-8?Q?=C5=9Awigo=C5=84?= <a.swigon@samsung.com>
To:     Krzysztof Kozlowski <krzk@kernel.org>
Cc:     devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-pm@vger.kernel.org, dri-devel@lists.freedesktop.org,
        cw00.choi@samsung.com, myungjoo.ham@samsung.com,
        inki.dae@samsung.com, sw0312.kim@samsung.com,
        georgi.djakov@linaro.org, leonard.crestez@nxp.com,
        m.szyprowski@samsung.com, b.zolnierkie@samsung.com
Date:   Tue, 31 Dec 2019 10:41:47 +0100
In-Reply-To: <20191231092254.GA6939@pi3>
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <20191220115653.6487-1-a.swigon@samsung.com>
        <CGME20191220120144eucas1p119ececf161a6d45a6a194e432bbbd1f9@eucas1p1.samsung.com>
        <20191220115653.6487-5-a.swigon@samsung.com> <20191230154405.GC4918@pi3>
        <2922135223b01126277ef92a53e6b294bc17bb5c.camel@samsung.com>
        <20191231092254.GA6939@pi3>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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,
-- 
Artur Świgoń
Samsung R&D Institute Poland
Samsung Electronics