Re: [PATCH 2/2] ARM: dts: exynos: use Exynos5420 dedicated USB2 PHY compatible

From: Arnd Bergmann <arnd@kernel.org>
To: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Krzysztof Kozlowski <krzk@kernel.org>,
	"moderated list:ARM/SAMSUNG EXYNOS ARM ARCHITECTURES" 
	<linux-samsung-soc@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Sylwester Nawrocki <s.nawrocki@samsung.com>,
	Vinod Koul <vkoul@kernel.org>,
	Kishon Vijay Abraham I <kishon@ti.com>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
	Willy Wolff <willy.mh.wolff.ml@gmail.com>,
	Marian Mihailescu <mihailescu2m@gmail.com>
Subject: Re: [PATCH 2/2] ARM: dts: exynos: use Exynos5420 dedicated USB2 PHY compatible
Date: Tue, 26 Jan 2021 23:44:26 +0100	[thread overview]
Message-ID: <CAK8P3a3QvbfYe5d_X4YcaRZaO4rykqoM_hm7nF-jwJGn0AQaug@mail.gmail.com> (raw)
In-Reply-To: <14102bab-33a1-45e0-0faf-883c7b1133a5@samsung.com>

On Fri, Nov 20, 2020 at 12:10 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> On 20.11.2020 12:05, Krzysztof Kozlowski wrote:
> > On Fri, Nov 20, 2020 at 09:56:37AM +0100, Marek Szyprowski wrote:
> >> USB2.0 PHY in Exynos5420 differs from Exynos5250 variant a bit, so use the
> >> recently introduced dedicated compatible for Exynos5420.
> >>
> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >> ---
> >>   arch/arm/boot/dts/exynos54xx.dtsi | 6 +++---
> >>   1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/arch/arm/boot/dts/exynos54xx.dtsi b/arch/arm/boot/dts/exynos54xx.dtsi
> >> index fe9d34c23374..2ddb7a5f12b3 100644
> >> --- a/arch/arm/boot/dts/exynos54xx.dtsi
> >> +++ b/arch/arm/boot/dts/exynos54xx.dtsi
> >> @@ -188,7 +188,7 @@
> >>                      compatible = "samsung,exynos4210-ehci";
> >>                      reg = <0x12110000 0x100>;
> >>                      interrupts = <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>;
> >> -                    phys = <&usb2_phy 1>;
> >> +                    phys = <&usb2_phy 0>;
> >>                      phy-names = "host";
> >>              };
> >>
> >> @@ -196,12 +196,12 @@
> >>                      compatible = "samsung,exynos4210-ohci";
> >>                      reg = <0x12120000 0x100>;
> >>                      interrupts = <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>;
> >> -                    phys = <&usb2_phy 1>;
> >> +                    phys = <&usb2_phy 0>;
> >>                      phy-names = "host";
> >>              };
> >>
> >>              usb2_phy: phy@12130000 {
> >> -                    compatible = "samsung,exynos5250-usb2-phy";
> >> +                    compatible = "samsung,exynos5420-usb2-phy";
> > The DTS change will wait till PHY driver adjustements get merged... or
> > if the difference is not critical, maybe using both compatibles (5420
> > and 5250) would have sense?
>
> It won't work easily with both compatibles, because in the 5420 variant
> I've also changed the PHY indices (5420 has no device and second hsic
> phy). IMHO the dts change can wait for the next release.

I see this made it into the pull request now, but I had not been aware
of the change earlier, and I'm slightly annoyed to have received it this
way:

- This is clearly an incompatible change to the dtb, and you all
   noticed that because it would cause a bisection problem. As
   a general rule, if a dts change does not work across bisection,
   we should not merge it at all, because it causes problems for
   anyone with external dts or dtb files.

- It would likely have been possible to define the new binding in
  a backward-compatible way. I don't see a reason why the index
  values in the binding had to change here, other than a slight
  inconvenience for the driver.

- If the change was really unavoidable, I would have expected
  a long explanation about why it had to be done in both the
  commit message and in the tag description for the pull
  request.

I've dropped the pull request for now, maybe this can still
be sorted out with another driver change that makes the
new compatible string backward-compatible.

        Arnd