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

[Artur ÅwigoÅ]

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@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).
> > 
> 
> 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.

-- 
Artur ÅwigoÅ
Samsung R&D Institute Poland
Samsung Electronics