[PATCH 0/2] Fix USB2 PHY operation on Exynos542x

[PATCH 0/2] Fix USB2 PHY operation on Exynos542x

[Marek Szyprowski]

Dear All,

This patchset finally fixes the last remaining issue related to the
system suspend/resume on Odroid XU3/XU4/HC1 board family. It can be
observed that system suspend/resume fails on XU4/HC1 when kernel is
compiled from multi_v7_defconfig. Chaning the configuration a bit -
switching Exynos USB2 PHY driver to be built-in surprisingly fixed that
issue. An investigation revealed that the Exynos USB2 PHY driver poked
wrong registers in the PMU area on Exynos5420 SoCs breaking the USB3.0
DRD PHY operation, what caused the suspend failure. Fix this by learning
the Exynos USB2 PHY driver about the Exynos5420 variant.

Best regards,
Marek Szyprowski


Patch summary:

Marek Szyprowski (2):
  phy: samsung: add support for the Exynos5420 variant of the USB2 PHY
  ARM: dts: exynos: use Exynos5420 dedicated USB2 PHY compatible

 .../devicetree/bindings/phy/samsung-phy.txt   |  1 +
 arch/arm/boot/dts/exynos54xx.dtsi             |  6 +--
 drivers/phy/samsung/Kconfig                   |  7 ++-
 drivers/phy/samsung/phy-exynos5250-usb2.c     | 48 +++++++++++++------
 drivers/phy/samsung/phy-samsung-usb2.c        |  6 +++
 drivers/phy/samsung/phy-samsung-usb2.h        |  1 +
 6 files changed, 51 insertions(+), 18 deletions(-)

-- 
2.17.1

[PATCH 1/2] phy: samsung: Add support for the Exynos5420 variant of the USB2 PHY

[Marek Szyprowski]

Exynos5420 differs a bit from Exynos5250 in USB2 PHY related registers in
the PMU region. Add a variant for the Exynos5420 case. Till now, USB2 PHY
worked only because the bootloader enabled the PHY, but then driver messed
USB 3.0 DRD related registers during the suspend/resume cycle.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 .../devicetree/bindings/phy/samsung-phy.txt   |  1 +
 drivers/phy/samsung/Kconfig                   |  7 ++-
 drivers/phy/samsung/phy-exynos5250-usb2.c     | 48 +++++++++++++------
 drivers/phy/samsung/phy-samsung-usb2.c        |  6 +++
 drivers/phy/samsung/phy-samsung-usb2.h        |  1 +
 5 files changed, 48 insertions(+), 15 deletions(-)

diff --git a/Documentation/devicetree/bindings/phy/samsung-phy.txt b/Documentation/devicetree/bindings/phy/samsung-phy.txt
index 7510830a79bd..8f51aee91101 100644
--- a/Documentation/devicetree/bindings/phy/samsung-phy.txt
+++ b/Documentation/devicetree/bindings/phy/samsung-phy.txt
@@ -47,6 +47,7 @@ Required properties:
 	- "samsung,exynos4210-usb2-phy"
 	- "samsung,exynos4x12-usb2-phy"
 	- "samsung,exynos5250-usb2-phy"
+	- "samsung,exynos5420-usb2-phy"
 	- "samsung,s5pv210-usb2-phy"
 - reg : a list of registers used by phy driver
 	- first and obligatory is the location of phy modules registers
diff --git a/drivers/phy/samsung/Kconfig b/drivers/phy/samsung/Kconfig
index e20d2fcc9fe7..0f51d3bf38cc 100644
--- a/drivers/phy/samsung/Kconfig
+++ b/drivers/phy/samsung/Kconfig
@@ -64,7 +64,12 @@ config PHY_EXYNOS4X12_USB2
 config PHY_EXYNOS5250_USB2
 	bool
 	depends on PHY_SAMSUNG_USB2
-	default SOC_EXYNOS5250 || SOC_EXYNOS5420
+	default SOC_EXYNOS5250
+
+config PHY_EXYNOS5420_USB2
+	bool
+	depends on PHY_SAMSUNG_USB2
+	default SOC_EXYNOS5420
 
 config PHY_S5PV210_USB2
 	bool "Support for S5PV210"
diff --git a/drivers/phy/samsung/phy-exynos5250-usb2.c b/drivers/phy/samsung/phy-exynos5250-usb2.c
index 4f53b711fd6f..e198010e1bfd 100644
--- a/drivers/phy/samsung/phy-exynos5250-usb2.c
+++ b/drivers/phy/samsung/phy-exynos5250-usb2.c
@@ -117,9 +117,9 @@
 
 /* Isolation, configured in the power management unit */
 #define EXYNOS_5250_USB_ISOL_OTG_OFFSET		0x704
-#define EXYNOS_5250_USB_ISOL_OTG		BIT(0)
 #define EXYNOS_5250_USB_ISOL_HOST_OFFSET	0x708
-#define EXYNOS_5250_USB_ISOL_HOST		BIT(0)
+#define EXYNOS_5420_USB_ISOL_HOST_OFFSET	0x70C
+#define EXYNOS_5250_USB_ISOL_ENABLE		BIT(0)
 
 /* Mode swtich register */
 #define EXYNOS_5250_MODE_SWITCH_OFFSET		0x230
@@ -132,7 +132,6 @@ enum exynos4x12_phy_id {
 	EXYNOS5250_HOST,
 	EXYNOS5250_HSIC0,
 	EXYNOS5250_HSIC1,
-	EXYNOS5250_NUM_PHYS,
 };
 
 /*
@@ -176,20 +175,19 @@ static void exynos5250_isol(struct samsung_usb2_phy_instance *inst, bool on)
 {
 	struct samsung_usb2_phy_driver *drv = inst->drv;
 	u32 offset;
-	u32 mask;
+	u32 mask = EXYNOS_5250_USB_ISOL_ENABLE;
 
-	switch (inst->cfg->id) {
-	case EXYNOS5250_DEVICE:
+	if (drv->cfg == &exynos5250_usb2_phy_config &&
+	    inst->cfg->id == EXYNOS5250_DEVICE)
 		offset = EXYNOS_5250_USB_ISOL_OTG_OFFSET;
-		mask = EXYNOS_5250_USB_ISOL_OTG;
-		break;
-	case EXYNOS5250_HOST:
+	else if (drv->cfg == &exynos5250_usb2_phy_config &&
+		 inst->cfg->id == EXYNOS5250_HOST)
 		offset = EXYNOS_5250_USB_ISOL_HOST_OFFSET;
-		mask = EXYNOS_5250_USB_ISOL_HOST;
-		break;
-	default:
+	else if (drv->cfg == &exynos5420_usb2_phy_config &&
+		 inst->cfg->id == EXYNOS5250_HOST)
+		offset = EXYNOS_5420_USB_ISOL_HOST_OFFSET;
+	else
 		return;
-	}
 
 	regmap_update_bits(drv->reg_pmu, offset, mask, on ? 0 : mask);
 }
@@ -390,9 +388,31 @@ static const struct samsung_usb2_common_phy exynos5250_phys[] = {
 	},
 };
 
+static const struct samsung_usb2_common_phy exynos5420_phys[] = {
+	{
+		.label		= "host",
+		.id		= EXYNOS5250_HOST,
+		.power_on	= exynos5250_power_on,
+		.power_off	= exynos5250_power_off,
+	},
+	{
+		.label		= "hsic",
+		.id		= EXYNOS5250_HSIC0,
+		.power_on	= exynos5250_power_on,
+		.power_off	= exynos5250_power_off,
+	},
+};
+
 const struct samsung_usb2_phy_config exynos5250_usb2_phy_config = {
 	.has_mode_switch	= 1,
-	.num_phys		= EXYNOS5250_NUM_PHYS,
+	.num_phys		= ARRAY_SIZE(exynos5250_phys),
 	.phys			= exynos5250_phys,
 	.rate_to_clk		= exynos5250_rate_to_clk,
 };
+
+const struct samsung_usb2_phy_config exynos5420_usb2_phy_config = {
+	.has_mode_switch	= 1,
+	.num_phys		= ARRAY_SIZE(exynos5420_phys),
+	.phys			= exynos5420_phys,
+	.rate_to_clk		= exynos5250_rate_to_clk,
+};
diff --git a/drivers/phy/samsung/phy-samsung-usb2.c b/drivers/phy/samsung/phy-samsung-usb2.c
index f79f605cff79..3908153f2ce5 100644
--- a/drivers/phy/samsung/phy-samsung-usb2.c
+++ b/drivers/phy/samsung/phy-samsung-usb2.c
@@ -128,6 +128,12 @@ static const struct of_device_id samsung_usb2_phy_of_match[] = {
 		.data = &exynos5250_usb2_phy_config,
 	},
 #endif
+#ifdef CONFIG_PHY_EXYNOS5420_USB2
+	{
+		.compatible = "samsung,exynos5420-usb2-phy",
+		.data = &exynos5420_usb2_phy_config,
+	},
+#endif
 #ifdef CONFIG_PHY_S5PV210_USB2
 	{
 		.compatible = "samsung,s5pv210-usb2-phy",
diff --git a/drivers/phy/samsung/phy-samsung-usb2.h b/drivers/phy/samsung/phy-samsung-usb2.h
index 77fb23bc218f..ebaf43bfc5a2 100644
--- a/drivers/phy/samsung/phy-samsung-usb2.h
+++ b/drivers/phy/samsung/phy-samsung-usb2.h
@@ -66,5 +66,6 @@ extern const struct samsung_usb2_phy_config exynos3250_usb2_phy_config;
 extern const struct samsung_usb2_phy_config exynos4210_usb2_phy_config;
 extern const struct samsung_usb2_phy_config exynos4x12_usb2_phy_config;
 extern const struct samsung_usb2_phy_config exynos5250_usb2_phy_config;
+extern const struct samsung_usb2_phy_config exynos5420_usb2_phy_config;
 extern const struct samsung_usb2_phy_config s5pv210_usb2_phy_config;
 #endif
-- 
2.17.1

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

[Marek Szyprowski]

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";
 			reg = <0x12130000 0x100>;
 			#phy-cells = <1>;
 		};
-- 
2.17.1

Re: [PATCH 1/2] phy: samsung: Add support for the Exynos5420 variant of the USB2 PHY

[Krzysztof Kozlowski]

On Fri, Nov 20, 2020 at 09:56:36AM +0100, Marek Szyprowski wrote:
> Exynos5420 differs a bit from Exynos5250 in USB2 PHY related registers in
> the PMU region. Add a variant for the Exynos5420 case. Till now, USB2 PHY
> worked only because the bootloader enabled the PHY, but then driver messed
> USB 3.0 DRD related registers during the suspend/resume cycle.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  .../devicetree/bindings/phy/samsung-phy.txt   |  1 +
>  drivers/phy/samsung/Kconfig                   |  7 ++-
>  drivers/phy/samsung/phy-exynos5250-usb2.c     | 48 +++++++++++++------
>  drivers/phy/samsung/phy-samsung-usb2.c        |  6 +++
>  drivers/phy/samsung/phy-samsung-usb2.h        |  1 +
>  5 files changed, 48 insertions(+), 15 deletions(-)
> 

Acked-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

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?

Best regards,
Krzysztof

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

[Marek Szyprowski]

Hi Krzysztof,

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.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 1/2] phy: samsung: Add support for the Exynos5420 variant of the USB2 PHY

[Vinod Koul]

On 20-11-20, 09:56, Marek Szyprowski wrote:
> Exynos5420 differs a bit from Exynos5250 in USB2 PHY related registers in
> the PMU region. Add a variant for the Exynos5420 case. Till now, USB2 PHY
> worked only because the bootloader enabled the PHY, but then driver messed
> USB 3.0 DRD related registers during the suspend/resume cycle.

Applied, thanks

-- 
~Vinod

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

[Krzysztof Kozlowski]

On Fri, Nov 20, 2020 at 12:07:44PM +0100, Marek Szyprowski wrote:
> Hi Krzysztof,
> 
> 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.

Thanks, applied.

Best regards,
Krzysztof

Re: (subset) [PATCH 0/2] Fix USB2 PHY operation on Exynos542x

[Krzysztof Kozlowski]

On Fri, 20 Nov 2020 09:56:35 +0100, Marek Szyprowski wrote:
> This patchset finally fixes the last remaining issue related to the
> system suspend/resume on Odroid XU3/XU4/HC1 board family. It can be
> observed that system suspend/resume fails on XU4/HC1 when kernel is
> compiled from multi_v7_defconfig. Chaning the configuration a bit -
> switching Exynos USB2 PHY driver to be built-in surprisingly fixed that
> issue. An investigation revealed that the Exynos USB2 PHY driver poked
> wrong registers in the PMU area on Exynos5420 SoCs breaking the USB3.0
> DRD PHY operation, what caused the suspend failure. Fix this by learning
> the Exynos USB2 PHY driver about the Exynos5420 variant.
> 
> [...]

Applied, thanks!

[2/2] ARM: dts: exynos: use Exynos5420 dedicated USB2 PHY compatible
      commit: 75681980c4e3d89c55b5b8f20b8f4c1aace601be

Best regards,
-- 
Krzysztof Kozlowski <krzk@kernel.org>

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

[Arnd Bergmann]

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

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

[Krzysztof Kozlowski]

On Tue, Jan 26, 2021 at 11:44:26PM +0100, Arnd Bergmann wrote:
> 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.

Hi Arnd,

No, it does not create a bisection problem. The driver change adding new
compatible is already in v5.11-rc1.

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

It does not matter since it's a new compatible and old one is not
affected. Nothing got broken before this patch, nothing got broken after
applying it via samsung-soc. No backwards compatibility is affected.

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

It's a different hardware. New hardware does not have to be compatible
with old hardware. However old DTB is still doing fine (although with
the original issue not fixed).

Best regards,
Krzysztof

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

[Arnd Bergmann]

On Wed, Jan 27, 2021 at 8:59 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Tue, Jan 26, 2021 at 11:44:26PM +0100, Arnd Bergmann wrote:
> > On Fri, Nov 20, 2020 at 12:10 PM Marek Szyprowski <m.szyprowski@samsung.com> wrote:

> > > 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.
>
> Hi Arnd,
>
> No, it does not create a bisection problem. The driver change adding new
> compatible is already in v5.11-rc1.

What I meant is that you knew there would be a bisection problem
if you had not delayed this patch.

> > - 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.
>
> It does not matter since it's a new compatible and old one is not
> affected. Nothing got broken before this patch, nothing got broken after
> applying it via samsung-soc. No backwards compatibility is affected.
>
> > - 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.
>
> It's a different hardware. New hardware does not have to be compatible
> with old hardware. However old DTB is still doing fine (although with
> the original issue not fixed).

There are around ten boards including this file, and most (maybe all)
of them are not newly added machines, so there is a good chance
that there are existing users. You are right that you took care that
the combination of an old dtb with a new kernel would not be any
worse than before, and that is good. What is however missing is
the consideration of the reverse: If anyone wants to dual-boot between
old and new kernels, they are stuck with the old dtb and is missing
the bugfix along with any additional changes that may get added
in the future.

The same is true if there are any non-Linux operating systems running
on these. For instance, FreeBSD runs on Peach Pit, and if they
were using the old dtb from Linux (I have not checked if they
were compatible before this change), then booting with the latest
dtb from Linux will require the same changes to their driver to avoid
a regression.

I can live with an explanation of "we've looked at all the alternatives
and decided to break old kernels with new dtbs in this particular
case because ...", but I don't like the idea of silently changing dts
in a way that breaks using them with anything but the latest kernel
and arguing that it's not even worth debating.

       Arnd

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

[Krzysztof Kozlowski]

On Sat, Jan 30, 2021 at 03:14:22PM +0100, Arnd Bergmann wrote:
> On Wed, Jan 27, 2021 at 8:59 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > On Tue, Jan 26, 2021 at 11:44:26PM +0100, Arnd Bergmann wrote:
> > > On Fri, Nov 20, 2020 at 12:10 PM Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> 
> > > > 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.
> >
> > Hi Arnd,
> >
> > No, it does not create a bisection problem. The driver change adding new
> > compatible is already in v5.11-rc1.
> 
> What I meant is that you knew there would be a bisection problem
> if you had not delayed this patch.

Of course, there are tons of such examples and delaying a patch for DTS
is perfectly safe, sane and regular way to deal with it.

>
> > > - 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.
> >
> > It does not matter since it's a new compatible and old one is not
> > affected. Nothing got broken before this patch, nothing got broken after
> > applying it via samsung-soc. No backwards compatibility is affected.
> >
> > > - 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.
> >
> > It's a different hardware. New hardware does not have to be compatible
> > with old hardware. However old DTB is still doing fine (although with
> > the original issue not fixed).
> 
> There are around ten boards including this file, and most (maybe all)
> of them are not newly added machines, so there is a good chance
> that there are existing users. You are right that you took care that
> the combination of an old dtb with a new kernel would not be any
> worse than before, and that is good. What is however missing is
> the consideration of the reverse: If anyone wants to dual-boot between
> old and new kernels, they are stuck with the old dtb and is missing
> the bugfix along with any additional changes that may get added
> in the future.

First of all, out of tree is not our concern. Let me quote DT guru, Rob:
"If it's not upstream, it doesn't exist."
https://lore.kernel.org/lkml/20201028152303.GA4041470@bogus/T/#m602ef29cade6f9ed49efd52159210d2a6813eec9

Second of all, out of tree DTB (so old DTB in your meaning) will work
perfectly fine (the same) and nothing is broken. So why I would need to
care about something which "does not exist" while I also do not break
it?

> The same is true if there are any non-Linux operating systems running
> on these. For instance, FreeBSD runs on Peach Pit, and if they
> were using the old dtb from Linux (I have not checked if they
> were compatible before this change), then booting with the latest
> dtb from Linux will require the same changes to their driver to avoid
> a regression.

If anyone takes kernel DTS and applies to his project, and then he takes
blindly the latter changes to DTS (but without earlier changes to
bindings! so he picks up only selective choice), then it's his
responsibility to be up to date. I am sorry, but I cannot take care
about any ridiculous and irresponsible idea in the world.

Seriously, do you expect us to think about out of tree unkown users of
an intree DTS when that *user does not follow bindings*?

It's the first time I hear it.

> I can live with an explanation of "we've looked at all the alternatives
> and decided to break old kernels with new dtbs in this particular
> case because ...", but I don't like the idea of silently changing dts
> in a way that breaks using them with anything but the latest kernel
> and arguing that it's not even worth debating.

There is no single need for intree kernel DTS to be backwards compatible
with old kernel. There is no single rule for it, either (because it's
not ABI). 

Best regards,
Krzysztof