[PATCH 0/3] phy: rockchip: emmc: Enable internal strobe pull-down by default

[PATCH 0/3] phy: rockchip: emmc: Enable internal strobe pull-down by default

[Folker Schwesinger via B4 Relay]

Various RK3399 boards, including the Rock Pi 4 series, the NanoPi 4
series and possibly others showed instabilities with some eMMC modules
when operating in HS400 mode [1]. Later, a missing pull-down resistor on
the eMMC strobe line was identified as the root cause of these
instabilities [2].

This series changes the Rockchip eMMC PHY driver to enable the
internal pull-down for the strobe line by default, effectively
restoring the behavior of the vendor kernel [3].
Addressing the problem in the driver avoids fix-ups in individual
board DTs.

As this change makes the optional DT property enable-strobe-pulldown
obsolete, it is replaced by a new property disable-strobe-pulldown,
that, when used, allows to disable the internal pull-down.

While at it, the enable-strobe-pulldown property was removed from the
NanoPi4 series DTS (which was the only user of the property in the kernel).

[1]: https://lore.kernel.org/all/20230705144255.115299-1-chris.obbard@collabora.com/
[2]: https://lore.kernel.org/all/20220822074139.3810-1-jensenhuang@friendlyarm.com/
[3]: https://github.com/rockchip-linux/kernel/blob/develop-4.4/drivers/phy/rockchip/phy-rockchip-emmc.c#L310

Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
---
Folker Schwesinger (3):
      phy: rockchip: emmc: Enable pulldown for strobe line
      devicetree: phy: rockchip-emmc: Document changed strobe-pulldown property
      arm64: dts: rockchip: Remove enable-strobe-pulldown for NanoPi4 boards

 Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt | 4 ++--
 arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi            | 1 -
 drivers/phy/rockchip/phy-rockchip-emmc.c                    | 6 +++---
 3 files changed, 5 insertions(+), 6 deletions(-)
---
base-commit: 4cece764965020c22cff7665b18a012006359095
change-id: 20240326-rk-default-enable-strobe-pulldown-3356c8cf1b4f

Best regards,
-- 
Folker Schwesinger <dev@folker-schwesinger.de>

[PATCH 0/3] phy: rockchip: emmc: Enable internal strobe pull-down by default

[Folker Schwesinger via B4 Relay]

Various RK3399 boards, including the Rock Pi 4 series, the NanoPi 4
series and possibly others showed instabilities with some eMMC modules
when operating in HS400 mode [1]. Later, a missing pull-down resistor on
the eMMC strobe line was identified as the root cause of these
instabilities [2].

This series changes the Rockchip eMMC PHY driver to enable the
internal pull-down for the strobe line by default, effectively
restoring the behavior of the vendor kernel [3].
Addressing the problem in the driver avoids fix-ups in individual
board DTs.

As this change makes the optional DT property enable-strobe-pulldown
obsolete, it is replaced by a new property disable-strobe-pulldown,
that, when used, allows to disable the internal pull-down.

While at it, the enable-strobe-pulldown property was removed from the
NanoPi4 series DTS (which was the only user of the property in the kernel).

[1]: https://lore.kernel.org/all/20230705144255.115299-1-chris.obbard@collabora.com/
[2]: https://lore.kernel.org/all/20220822074139.3810-1-jensenhuang@friendlyarm.com/
[3]: https://github.com/rockchip-linux/kernel/blob/develop-4.4/drivers/phy/rockchip/phy-rockchip-emmc.c#L310

Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
---
Folker Schwesinger (3):
      phy: rockchip: emmc: Enable pulldown for strobe line
      devicetree: phy: rockchip-emmc: Document changed strobe-pulldown property
      arm64: dts: rockchip: Remove enable-strobe-pulldown for NanoPi4 boards

 Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt | 4 ++--
 arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi            | 1 -
 drivers/phy/rockchip/phy-rockchip-emmc.c                    | 6 +++---
 3 files changed, 5 insertions(+), 6 deletions(-)
---
base-commit: 4cece764965020c22cff7665b18a012006359095
change-id: 20240326-rk-default-enable-strobe-pulldown-3356c8cf1b4f

Best regards,
-- 
Folker Schwesinger <dev@folker-schwesinger.de>



-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH 0/3] phy: rockchip: emmc: Enable internal strobe pull-down by default

[Folker Schwesinger via B4 Relay]

Various RK3399 boards, including the Rock Pi 4 series, the NanoPi 4
series and possibly others showed instabilities with some eMMC modules
when operating in HS400 mode [1]. Later, a missing pull-down resistor on
the eMMC strobe line was identified as the root cause of these
instabilities [2].

This series changes the Rockchip eMMC PHY driver to enable the
internal pull-down for the strobe line by default, effectively
restoring the behavior of the vendor kernel [3].
Addressing the problem in the driver avoids fix-ups in individual
board DTs.

As this change makes the optional DT property enable-strobe-pulldown
obsolete, it is replaced by a new property disable-strobe-pulldown,
that, when used, allows to disable the internal pull-down.

While at it, the enable-strobe-pulldown property was removed from the
NanoPi4 series DTS (which was the only user of the property in the kernel).

[1]: https://lore.kernel.org/all/20230705144255.115299-1-chris.obbard@collabora.com/
[2]: https://lore.kernel.org/all/20220822074139.3810-1-jensenhuang@friendlyarm.com/
[3]: https://github.com/rockchip-linux/kernel/blob/develop-4.4/drivers/phy/rockchip/phy-rockchip-emmc.c#L310

Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
---
Folker Schwesinger (3):
      phy: rockchip: emmc: Enable pulldown for strobe line
      devicetree: phy: rockchip-emmc: Document changed strobe-pulldown property
      arm64: dts: rockchip: Remove enable-strobe-pulldown for NanoPi4 boards

 Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt | 4 ++--
 arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi            | 1 -
 drivers/phy/rockchip/phy-rockchip-emmc.c                    | 6 +++---
 3 files changed, 5 insertions(+), 6 deletions(-)
---
base-commit: 4cece764965020c22cff7665b18a012006359095
change-id: 20240326-rk-default-enable-strobe-pulldown-3356c8cf1b4f

Best regards,
-- 
Folker Schwesinger <dev@folker-schwesinger.de>



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH 0/3] phy: rockchip: emmc: Enable internal strobe pull-down by default

[Folker Schwesinger via B4 Relay]

Various RK3399 boards, including the Rock Pi 4 series, the NanoPi 4
series and possibly others showed instabilities with some eMMC modules
when operating in HS400 mode [1]. Later, a missing pull-down resistor on
the eMMC strobe line was identified as the root cause of these
instabilities [2].

This series changes the Rockchip eMMC PHY driver to enable the
internal pull-down for the strobe line by default, effectively
restoring the behavior of the vendor kernel [3].
Addressing the problem in the driver avoids fix-ups in individual
board DTs.

As this change makes the optional DT property enable-strobe-pulldown
obsolete, it is replaced by a new property disable-strobe-pulldown,
that, when used, allows to disable the internal pull-down.

While at it, the enable-strobe-pulldown property was removed from the
NanoPi4 series DTS (which was the only user of the property in the kernel).

[1]: https://lore.kernel.org/all/20230705144255.115299-1-chris.obbard@collabora.com/
[2]: https://lore.kernel.org/all/20220822074139.3810-1-jensenhuang@friendlyarm.com/
[3]: https://github.com/rockchip-linux/kernel/blob/develop-4.4/drivers/phy/rockchip/phy-rockchip-emmc.c#L310

Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
---
Folker Schwesinger (3):
      phy: rockchip: emmc: Enable pulldown for strobe line
      devicetree: phy: rockchip-emmc: Document changed strobe-pulldown property
      arm64: dts: rockchip: Remove enable-strobe-pulldown for NanoPi4 boards

 Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt | 4 ++--
 arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi            | 1 -
 drivers/phy/rockchip/phy-rockchip-emmc.c                    | 6 +++---
 3 files changed, 5 insertions(+), 6 deletions(-)
---
base-commit: 4cece764965020c22cff7665b18a012006359095
change-id: 20240326-rk-default-enable-strobe-pulldown-3356c8cf1b4f

Best regards,
-- 
Folker Schwesinger <dev@folker-schwesinger.de>



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 0/3] phy: rockchip: emmc: Enable internal strobe pull-down by default

[Folker Schwesinger]

Various RK3399 boards, including the Rock Pi 4 series, the NanoPi 4
series and possibly others showed instabilities with some eMMC modules
when operating in HS400 mode [1]. Later, a missing pull-down resistor on
the eMMC strobe line was identified as the root cause of these
instabilities [2].

This series changes the Rockchip eMMC PHY driver to enable the
internal pull-down for the strobe line by default, effectively
restoring the behavior of the vendor kernel [3].
Addressing the problem in the driver avoids fix-ups in individual
board DTs.

As this change makes the optional DT property enable-strobe-pulldown
obsolete, it is replaced by a new property disable-strobe-pulldown,
that, when used, allows to disable the internal pull-down.

While at it, the enable-strobe-pulldown property was removed from the
NanoPi4 series DTS (which was the only user of the property in the kernel).

[1]: https://lore.kernel.org/all/20230705144255.115299-1-chris.obbard@collabora.com/
[2]: https://lore.kernel.org/all/20220822074139.3810-1-jensenhuang@friendlyarm.com/
[3]: https://github.com/rockchip-linux/kernel/blob/develop-4.4/drivers/phy/rockchip/phy-rockchip-emmc.c#L310

Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
---
Folker Schwesinger (3):
      phy: rockchip: emmc: Enable pulldown for strobe line
      devicetree: phy: rockchip-emmc: Document changed strobe-pulldown property
      arm64: dts: rockchip: Remove enable-strobe-pulldown for NanoPi4 boards

 Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt | 4 ++--
 arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi            | 1 -
 drivers/phy/rockchip/phy-rockchip-emmc.c                    | 6 +++---
 3 files changed, 5 insertions(+), 6 deletions(-)
---
base-commit: 4cece764965020c22cff7665b18a012006359095
change-id: 20240326-rk-default-enable-strobe-pulldown-3356c8cf1b4f

Best regards,
-- 
Folker Schwesinger <dev@folker-schwesinger.de>

[PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Folker Schwesinger via B4 Relay]

From: Folker Schwesinger <dev@folker-schwesinger.de>

Restore the behavior of the Rockchip kernel that undconditionally
enables the internal strobe pulldown.
As the DT property rockchip,enable-strobe-pulldown is obsolete now,
replace it with a property to disable the internal pulldown.

This fixes I/O errors observed on various Rock Pi 4 and NanoPi4 series
boards with some eMMC modules. Other boards may also be affected.

An example of these errors is as follows:

[  290.060817] mmc1: running CQE recovery
[  290.061337] blk_update_request: I/O error, dev mmcblk1, sector 1411072 op 0x1:(WRITE) flags 0x800 phys_seg 36 prio class 0
[  290.061370] EXT4-fs warning (device mmcblk1p1): ext4_end_bio:348: I/O error 10 writing to inode 29547 starting block 176466)
[  290.061484] Buffer I/O error on device mmcblk1p1, logical block 172288

Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe line in dts")
Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
---
 drivers/phy/rockchip/phy-rockchip-emmc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
index 20023f6eb994..6e637f3e1b19 100644
--- a/drivers/phy/rockchip/phy-rockchip-emmc.c
+++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
@@ -376,14 +376,14 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
 	rk_phy->reg_offset = reg_offset;
 	rk_phy->reg_base = grf;
 	rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
-	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
+	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
 	rk_phy->output_tapdelay_select = PHYCTRL_OTAPDLYSEL_DEFAULT;
 
 	if (!of_property_read_u32(dev->of_node, "drive-impedance-ohm", &val))
 		rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val);
 
-	if (of_property_read_bool(dev->of_node, "rockchip,enable-strobe-pulldown"))
-		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
+	if (of_property_read_bool(dev->of_node, "rockchip,disable-strobe-pulldown"))
+		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
 
 	if (!of_property_read_u32(dev->of_node, "rockchip,output-tapdelay-select", &val)) {
 		if (val <= PHYCTRL_OTAPDLYSEL_MAXVALUE)

-- 
2.44.0

[PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Folker Schwesinger via B4 Relay]

From: Folker Schwesinger <dev@folker-schwesinger.de>

Restore the behavior of the Rockchip kernel that undconditionally
enables the internal strobe pulldown.
As the DT property rockchip,enable-strobe-pulldown is obsolete now,
replace it with a property to disable the internal pulldown.

This fixes I/O errors observed on various Rock Pi 4 and NanoPi4 series
boards with some eMMC modules. Other boards may also be affected.

An example of these errors is as follows:

[  290.060817] mmc1: running CQE recovery
[  290.061337] blk_update_request: I/O error, dev mmcblk1, sector 1411072 op 0x1:(WRITE) flags 0x800 phys_seg 36 prio class 0
[  290.061370] EXT4-fs warning (device mmcblk1p1): ext4_end_bio:348: I/O error 10 writing to inode 29547 starting block 176466)
[  290.061484] Buffer I/O error on device mmcblk1p1, logical block 172288

Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe line in dts")
Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
---
 drivers/phy/rockchip/phy-rockchip-emmc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
index 20023f6eb994..6e637f3e1b19 100644
--- a/drivers/phy/rockchip/phy-rockchip-emmc.c
+++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
@@ -376,14 +376,14 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
 	rk_phy->reg_offset = reg_offset;
 	rk_phy->reg_base = grf;
 	rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
-	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
+	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
 	rk_phy->output_tapdelay_select = PHYCTRL_OTAPDLYSEL_DEFAULT;
 
 	if (!of_property_read_u32(dev->of_node, "drive-impedance-ohm", &val))
 		rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val);
 
-	if (of_property_read_bool(dev->of_node, "rockchip,enable-strobe-pulldown"))
-		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
+	if (of_property_read_bool(dev->of_node, "rockchip,disable-strobe-pulldown"))
+		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
 
 	if (!of_property_read_u32(dev->of_node, "rockchip,output-tapdelay-select", &val)) {
 		if (val <= PHYCTRL_OTAPDLYSEL_MAXVALUE)

-- 
2.44.0



-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Folker Schwesinger via B4 Relay]

From: Folker Schwesinger <dev@folker-schwesinger.de>

Restore the behavior of the Rockchip kernel that undconditionally
enables the internal strobe pulldown.
As the DT property rockchip,enable-strobe-pulldown is obsolete now,
replace it with a property to disable the internal pulldown.

This fixes I/O errors observed on various Rock Pi 4 and NanoPi4 series
boards with some eMMC modules. Other boards may also be affected.

An example of these errors is as follows:

[  290.060817] mmc1: running CQE recovery
[  290.061337] blk_update_request: I/O error, dev mmcblk1, sector 1411072 op 0x1:(WRITE) flags 0x800 phys_seg 36 prio class 0
[  290.061370] EXT4-fs warning (device mmcblk1p1): ext4_end_bio:348: I/O error 10 writing to inode 29547 starting block 176466)
[  290.061484] Buffer I/O error on device mmcblk1p1, logical block 172288

Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe line in dts")
Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
---
 drivers/phy/rockchip/phy-rockchip-emmc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
index 20023f6eb994..6e637f3e1b19 100644
--- a/drivers/phy/rockchip/phy-rockchip-emmc.c
+++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
@@ -376,14 +376,14 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
 	rk_phy->reg_offset = reg_offset;
 	rk_phy->reg_base = grf;
 	rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
-	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
+	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
 	rk_phy->output_tapdelay_select = PHYCTRL_OTAPDLYSEL_DEFAULT;
 
 	if (!of_property_read_u32(dev->of_node, "drive-impedance-ohm", &val))
 		rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val);
 
-	if (of_property_read_bool(dev->of_node, "rockchip,enable-strobe-pulldown"))
-		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
+	if (of_property_read_bool(dev->of_node, "rockchip,disable-strobe-pulldown"))
+		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
 
 	if (!of_property_read_u32(dev->of_node, "rockchip,output-tapdelay-select", &val)) {
 		if (val <= PHYCTRL_OTAPDLYSEL_MAXVALUE)

-- 
2.44.0



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Folker Schwesinger via B4 Relay]

From: Folker Schwesinger <dev@folker-schwesinger.de>

Restore the behavior of the Rockchip kernel that undconditionally
enables the internal strobe pulldown.
As the DT property rockchip,enable-strobe-pulldown is obsolete now,
replace it with a property to disable the internal pulldown.

This fixes I/O errors observed on various Rock Pi 4 and NanoPi4 series
boards with some eMMC modules. Other boards may also be affected.

An example of these errors is as follows:

[  290.060817] mmc1: running CQE recovery
[  290.061337] blk_update_request: I/O error, dev mmcblk1, sector 1411072 op 0x1:(WRITE) flags 0x800 phys_seg 36 prio class 0
[  290.061370] EXT4-fs warning (device mmcblk1p1): ext4_end_bio:348: I/O error 10 writing to inode 29547 starting block 176466)
[  290.061484] Buffer I/O error on device mmcblk1p1, logical block 172288

Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe line in dts")
Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
---
 drivers/phy/rockchip/phy-rockchip-emmc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
index 20023f6eb994..6e637f3e1b19 100644
--- a/drivers/phy/rockchip/phy-rockchip-emmc.c
+++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
@@ -376,14 +376,14 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
 	rk_phy->reg_offset = reg_offset;
 	rk_phy->reg_base = grf;
 	rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
-	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
+	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
 	rk_phy->output_tapdelay_select = PHYCTRL_OTAPDLYSEL_DEFAULT;
 
 	if (!of_property_read_u32(dev->of_node, "drive-impedance-ohm", &val))
 		rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val);
 
-	if (of_property_read_bool(dev->of_node, "rockchip,enable-strobe-pulldown"))
-		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
+	if (of_property_read_bool(dev->of_node, "rockchip,disable-strobe-pulldown"))
+		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
 
 	if (!of_property_read_u32(dev->of_node, "rockchip,output-tapdelay-select", &val)) {
 		if (val <= PHYCTRL_OTAPDLYSEL_MAXVALUE)

-- 
2.44.0



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Folker Schwesinger]

Restore the behavior of the Rockchip kernel that undconditionally
enables the internal strobe pulldown.
As the DT property rockchip,enable-strobe-pulldown is obsolete now,
replace it with a property to disable the internal pulldown.

This fixes I/O errors observed on various Rock Pi 4 and NanoPi4 series
boards with some eMMC modules. Other boards may also be affected.

An example of these errors is as follows:

[  290.060817] mmc1: running CQE recovery
[  290.061337] blk_update_request: I/O error, dev mmcblk1, sector 1411072 op 0x1:(WRITE) flags 0x800 phys_seg 36 prio class 0
[  290.061370] EXT4-fs warning (device mmcblk1p1): ext4_end_bio:348: I/O error 10 writing to inode 29547 starting block 176466)
[  290.061484] Buffer I/O error on device mmcblk1p1, logical block 172288

Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe line in dts")
Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
---
 drivers/phy/rockchip/phy-rockchip-emmc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
index 20023f6eb994..6e637f3e1b19 100644
--- a/drivers/phy/rockchip/phy-rockchip-emmc.c
+++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
@@ -376,14 +376,14 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
 	rk_phy->reg_offset = reg_offset;
 	rk_phy->reg_base = grf;
 	rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
-	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
+	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
 	rk_phy->output_tapdelay_select = PHYCTRL_OTAPDLYSEL_DEFAULT;
 
 	if (!of_property_read_u32(dev->of_node, "drive-impedance-ohm", &val))
 		rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val);
 
-	if (of_property_read_bool(dev->of_node, "rockchip,enable-strobe-pulldown"))
-		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
+	if (of_property_read_bool(dev->of_node, "rockchip,disable-strobe-pulldown"))
+		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
 
 	if (!of_property_read_u32(dev->of_node, "rockchip,output-tapdelay-select", &val)) {
 		if (val <= PHYCTRL_OTAPDLYSEL_MAXVALUE)

-- 
2.44.0

[PATCH 2/3] devicetree: phy: rockchip-emmc: Document changed strobe-pulldown property

[Folker Schwesinger via B4 Relay]

From: Folker Schwesinger <dev@folker-schwesinger.de>

Document the changes regarding the optional strobe-pulldown property.
These changes are necessary as the default behavior of the driver was
restored to the Rockchip kernel behavior of enabling the internal
pulldown by default.

Fixes: f34e43f12382 ("devicetree: phy: rockchip-emmc: pulldown property")
Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
---
 Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt b/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
index 57d28c0d5696..10c05437f7ab 100644
--- a/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
+++ b/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
@@ -16,8 +16,8 @@ Optional properties:
  - drive-impedance-ohm: Specifies the drive impedance in Ohm.
                         Possible values are 33, 40, 50, 66 and 100.
                         If not set, the default value of 50 will be applied.
- - rockchip,enable-strobe-pulldown: Enable internal pull-down for the strobe
-                                    line.  If not set, pull-down is not used.
+ - rockchip,disable-strobe-pulldown: Disable internal pull-down for the strobe
+                                     line.  If not set, pull-down is used.
  - rockchip,output-tapdelay-select: Specifies the phyctrl_otapdlysec register.
                                     If not set, the register defaults to 0x4.
                                     Maximum value 0xf.

-- 
2.44.0

[PATCH 2/3] devicetree: phy: rockchip-emmc: Document changed strobe-pulldown property

[Folker Schwesinger via B4 Relay]

From: Folker Schwesinger <dev@folker-schwesinger.de>

Document the changes regarding the optional strobe-pulldown property.
These changes are necessary as the default behavior of the driver was
restored to the Rockchip kernel behavior of enabling the internal
pulldown by default.

Fixes: f34e43f12382 ("devicetree: phy: rockchip-emmc: pulldown property")
Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
---
 Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt b/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
index 57d28c0d5696..10c05437f7ab 100644
--- a/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
+++ b/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
@@ -16,8 +16,8 @@ Optional properties:
  - drive-impedance-ohm: Specifies the drive impedance in Ohm.
                         Possible values are 33, 40, 50, 66 and 100.
                         If not set, the default value of 50 will be applied.
- - rockchip,enable-strobe-pulldown: Enable internal pull-down for the strobe
-                                    line.  If not set, pull-down is not used.
+ - rockchip,disable-strobe-pulldown: Disable internal pull-down for the strobe
+                                     line.  If not set, pull-down is used.
  - rockchip,output-tapdelay-select: Specifies the phyctrl_otapdlysec register.
                                     If not set, the register defaults to 0x4.
                                     Maximum value 0xf.

-- 
2.44.0



-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH 2/3] devicetree: phy: rockchip-emmc: Document changed strobe-pulldown property

[Folker Schwesinger via B4 Relay]

From: Folker Schwesinger <dev@folker-schwesinger.de>

Document the changes regarding the optional strobe-pulldown property.
These changes are necessary as the default behavior of the driver was
restored to the Rockchip kernel behavior of enabling the internal
pulldown by default.

Fixes: f34e43f12382 ("devicetree: phy: rockchip-emmc: pulldown property")
Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
---
 Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt b/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
index 57d28c0d5696..10c05437f7ab 100644
--- a/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
+++ b/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
@@ -16,8 +16,8 @@ Optional properties:
  - drive-impedance-ohm: Specifies the drive impedance in Ohm.
                         Possible values are 33, 40, 50, 66 and 100.
                         If not set, the default value of 50 will be applied.
- - rockchip,enable-strobe-pulldown: Enable internal pull-down for the strobe
-                                    line.  If not set, pull-down is not used.
+ - rockchip,disable-strobe-pulldown: Disable internal pull-down for the strobe
+                                     line.  If not set, pull-down is used.
  - rockchip,output-tapdelay-select: Specifies the phyctrl_otapdlysec register.
                                     If not set, the register defaults to 0x4.
                                     Maximum value 0xf.

-- 
2.44.0



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH 2/3] devicetree: phy: rockchip-emmc: Document changed strobe-pulldown property

[Folker Schwesinger via B4 Relay]

From: Folker Schwesinger <dev@folker-schwesinger.de>

Document the changes regarding the optional strobe-pulldown property.
These changes are necessary as the default behavior of the driver was
restored to the Rockchip kernel behavior of enabling the internal
pulldown by default.

Fixes: f34e43f12382 ("devicetree: phy: rockchip-emmc: pulldown property")
Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
---
 Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt b/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
index 57d28c0d5696..10c05437f7ab 100644
--- a/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
+++ b/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
@@ -16,8 +16,8 @@ Optional properties:
  - drive-impedance-ohm: Specifies the drive impedance in Ohm.
                         Possible values are 33, 40, 50, 66 and 100.
                         If not set, the default value of 50 will be applied.
- - rockchip,enable-strobe-pulldown: Enable internal pull-down for the strobe
-                                    line.  If not set, pull-down is not used.
+ - rockchip,disable-strobe-pulldown: Disable internal pull-down for the strobe
+                                     line.  If not set, pull-down is used.
  - rockchip,output-tapdelay-select: Specifies the phyctrl_otapdlysec register.
                                     If not set, the register defaults to 0x4.
                                     Maximum value 0xf.

-- 
2.44.0



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] devicetree: phy: rockchip-emmc: Document changed strobe-pulldown property

[Folker Schwesinger]

Document the changes regarding the optional strobe-pulldown property.
These changes are necessary as the default behavior of the driver was
restored to the Rockchip kernel behavior of enabling the internal
pulldown by default.

Fixes: f34e43f12382 ("devicetree: phy: rockchip-emmc: pulldown property")
Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
---
 Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt b/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
index 57d28c0d5696..10c05437f7ab 100644
--- a/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
+++ b/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
@@ -16,8 +16,8 @@ Optional properties:
  - drive-impedance-ohm: Specifies the drive impedance in Ohm.
                         Possible values are 33, 40, 50, 66 and 100.
                         If not set, the default value of 50 will be applied.
- - rockchip,enable-strobe-pulldown: Enable internal pull-down for the strobe
-                                    line.  If not set, pull-down is not used.
+ - rockchip,disable-strobe-pulldown: Disable internal pull-down for the strobe
+                                     line.  If not set, pull-down is used.
  - rockchip,output-tapdelay-select: Specifies the phyctrl_otapdlysec register.
                                     If not set, the register defaults to 0x4.
                                     Maximum value 0xf.

-- 
2.44.0

[PATCH 3/3] arm64: dts: rockchip: Remove enable-strobe-pulldown for NanoPi4 boards

[Folker Schwesinger via B4 Relay]

From: Folker Schwesinger <dev@folker-schwesinger.de>

The Rockchip eMMC PHY driver enables the internal strobe pulldown by
default. So, remove the enable-strobe-pulldown property from the
NanoPi4 series boards DTS.

Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
---
 arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
index b7f1e47978a6..38af91b9e756 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
@@ -168,7 +168,6 @@ &cpu_l3 {
 };
 
 &emmc_phy {
-	rockchip,enable-strobe-pulldown;
 	status = "okay";
 };
 

-- 
2.44.0

[PATCH 3/3] arm64: dts: rockchip: Remove enable-strobe-pulldown for NanoPi4 boards

[Folker Schwesinger via B4 Relay]

From: Folker Schwesinger <dev@folker-schwesinger.de>

The Rockchip eMMC PHY driver enables the internal strobe pulldown by
default. So, remove the enable-strobe-pulldown property from the
NanoPi4 series boards DTS.

Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
---
 arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
index b7f1e47978a6..38af91b9e756 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
@@ -168,7 +168,6 @@ &cpu_l3 {
 };
 
 &emmc_phy {
-	rockchip,enable-strobe-pulldown;
 	status = "okay";
 };
 

-- 
2.44.0



-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH 3/3] arm64: dts: rockchip: Remove enable-strobe-pulldown for NanoPi4 boards

[Folker Schwesinger via B4 Relay]

From: Folker Schwesinger <dev@folker-schwesinger.de>

The Rockchip eMMC PHY driver enables the internal strobe pulldown by
default. So, remove the enable-strobe-pulldown property from the
NanoPi4 series boards DTS.

Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
---
 arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
index b7f1e47978a6..38af91b9e756 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
@@ -168,7 +168,6 @@ &cpu_l3 {
 };
 
 &emmc_phy {
-	rockchip,enable-strobe-pulldown;
 	status = "okay";
 };
 

-- 
2.44.0



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH 3/3] arm64: dts: rockchip: Remove enable-strobe-pulldown for NanoPi4 boards

[Folker Schwesinger via B4 Relay]

From: Folker Schwesinger <dev@folker-schwesinger.de>

The Rockchip eMMC PHY driver enables the internal strobe pulldown by
default. So, remove the enable-strobe-pulldown property from the
NanoPi4 series boards DTS.

Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
---
 arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
index b7f1e47978a6..38af91b9e756 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
@@ -168,7 +168,6 @@ &cpu_l3 {
 };
 
 &emmc_phy {
-	rockchip,enable-strobe-pulldown;
 	status = "okay";
 };
 

-- 
2.44.0



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] arm64: dts: rockchip: Remove enable-strobe-pulldown for NanoPi4 boards

[Folker Schwesinger]

The Rockchip eMMC PHY driver enables the internal strobe pulldown by
default. So, remove the enable-strobe-pulldown property from the
NanoPi4 series boards DTS.

Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
---
 arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
index b7f1e47978a6..38af91b9e756 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
@@ -168,7 +168,6 @@ &cpu_l3 {
 };
 
 &emmc_phy {
-	rockchip,enable-strobe-pulldown;
 	status = "okay";
 };
 

-- 
2.44.0

Re: [PATCH 2/3] devicetree: phy: rockchip-emmc: Document changed strobe-pulldown property

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2010 bytes --]

On Tue, Mar 26, 2024 at 07:54:36PM +0100, Folker Schwesinger via B4 Relay wrote:
> From: Folker Schwesinger <dev@folker-schwesinger.de>

The prefix is "dt-bindings" not "devicetree" FYI.

> 
> Document the changes regarding the optional strobe-pulldown property.
> These changes are necessary as the default behavior of the driver was
> restored to the Rockchip kernel behavior of enabling the internal
> pulldown by default.

I don't think this is a valid justification, but it'll be easier for me
to explain this on the driver patch.

Thanks,
Conor.

> 
> Fixes: f34e43f12382 ("devicetree: phy: rockchip-emmc: pulldown property")
> Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
> ---
>  Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt b/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
> index 57d28c0d5696..10c05437f7ab 100644
> --- a/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
> +++ b/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
> @@ -16,8 +16,8 @@ Optional properties:
>   - drive-impedance-ohm: Specifies the drive impedance in Ohm.
>                          Possible values are 33, 40, 50, 66 and 100.
>                          If not set, the default value of 50 will be applied.
> - - rockchip,enable-strobe-pulldown: Enable internal pull-down for the strobe
> -                                    line.  If not set, pull-down is not used.
> + - rockchip,disable-strobe-pulldown: Disable internal pull-down for the strobe
> +                                     line.  If not set, pull-down is used.
>   - rockchip,output-tapdelay-select: Specifies the phyctrl_otapdlysec register.
>                                      If not set, the register defaults to 0x4.
>                                      Maximum value 0xf.
> 
> -- 
> 2.44.0
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/3] devicetree: phy: rockchip-emmc: Document changed strobe-pulldown property

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 2010 bytes --]

On Tue, Mar 26, 2024 at 07:54:36PM +0100, Folker Schwesinger via B4 Relay wrote:
> From: Folker Schwesinger <dev@folker-schwesinger.de>

The prefix is "dt-bindings" not "devicetree" FYI.

> 
> Document the changes regarding the optional strobe-pulldown property.
> These changes are necessary as the default behavior of the driver was
> restored to the Rockchip kernel behavior of enabling the internal
> pulldown by default.

I don't think this is a valid justification, but it'll be easier for me
to explain this on the driver patch.

Thanks,
Conor.

> 
> Fixes: f34e43f12382 ("devicetree: phy: rockchip-emmc: pulldown property")
> Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
> ---
>  Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt b/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
> index 57d28c0d5696..10c05437f7ab 100644
> --- a/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
> +++ b/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
> @@ -16,8 +16,8 @@ Optional properties:
>   - drive-impedance-ohm: Specifies the drive impedance in Ohm.
>                          Possible values are 33, 40, 50, 66 and 100.
>                          If not set, the default value of 50 will be applied.
> - - rockchip,enable-strobe-pulldown: Enable internal pull-down for the strobe
> -                                    line.  If not set, pull-down is not used.
> + - rockchip,disable-strobe-pulldown: Disable internal pull-down for the strobe
> +                                     line.  If not set, pull-down is used.
>   - rockchip,output-tapdelay-select: Specifies the phyctrl_otapdlysec register.
>                                      If not set, the register defaults to 0x4.
>                                      Maximum value 0xf.
> 
> -- 
> 2.44.0
> 
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 2/3] devicetree: phy: rockchip-emmc: Document changed strobe-pulldown property

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 2010 bytes --]

On Tue, Mar 26, 2024 at 07:54:36PM +0100, Folker Schwesinger via B4 Relay wrote:
> From: Folker Schwesinger <dev@folker-schwesinger.de>

The prefix is "dt-bindings" not "devicetree" FYI.

> 
> Document the changes regarding the optional strobe-pulldown property.
> These changes are necessary as the default behavior of the driver was
> restored to the Rockchip kernel behavior of enabling the internal
> pulldown by default.

I don't think this is a valid justification, but it'll be easier for me
to explain this on the driver patch.

Thanks,
Conor.

> 
> Fixes: f34e43f12382 ("devicetree: phy: rockchip-emmc: pulldown property")
> Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
> ---
>  Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt b/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
> index 57d28c0d5696..10c05437f7ab 100644
> --- a/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
> +++ b/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
> @@ -16,8 +16,8 @@ Optional properties:
>   - drive-impedance-ohm: Specifies the drive impedance in Ohm.
>                          Possible values are 33, 40, 50, 66 and 100.
>                          If not set, the default value of 50 will be applied.
> - - rockchip,enable-strobe-pulldown: Enable internal pull-down for the strobe
> -                                    line.  If not set, pull-down is not used.
> + - rockchip,disable-strobe-pulldown: Disable internal pull-down for the strobe
> +                                     line.  If not set, pull-down is used.
>   - rockchip,output-tapdelay-select: Specifies the phyctrl_otapdlysec register.
>                                      If not set, the register defaults to 0x4.
>                                      Maximum value 0xf.
> 
> -- 
> 2.44.0
> 
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] devicetree: phy: rockchip-emmc: Document changed strobe-pulldown property

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 2010 bytes --]

On Tue, Mar 26, 2024 at 07:54:36PM +0100, Folker Schwesinger via B4 Relay wrote:
> From: Folker Schwesinger <dev@folker-schwesinger.de>

The prefix is "dt-bindings" not "devicetree" FYI.

> 
> Document the changes regarding the optional strobe-pulldown property.
> These changes are necessary as the default behavior of the driver was
> restored to the Rockchip kernel behavior of enabling the internal
> pulldown by default.

I don't think this is a valid justification, but it'll be easier for me
to explain this on the driver patch.

Thanks,
Conor.

> 
> Fixes: f34e43f12382 ("devicetree: phy: rockchip-emmc: pulldown property")
> Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
> ---
>  Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt b/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
> index 57d28c0d5696..10c05437f7ab 100644
> --- a/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
> +++ b/Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt
> @@ -16,8 +16,8 @@ Optional properties:
>   - drive-impedance-ohm: Specifies the drive impedance in Ohm.
>                          Possible values are 33, 40, 50, 66 and 100.
>                          If not set, the default value of 50 will be applied.
> - - rockchip,enable-strobe-pulldown: Enable internal pull-down for the strobe
> -                                    line.  If not set, pull-down is not used.
> + - rockchip,disable-strobe-pulldown: Disable internal pull-down for the strobe
> +                                     line.  If not set, pull-down is used.
>   - rockchip,output-tapdelay-select: Specifies the phyctrl_otapdlysec register.
>                                      If not set, the register defaults to 0x4.
>                                      Maximum value 0xf.
> 
> -- 
> 2.44.0
> 
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3387 bytes --]

On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4 Relay wrote:
> From: Folker Schwesinger <dev@folker-schwesinger.de>
> 
> Restore the behavior of the Rockchip kernel that undconditionally
> enables the internal strobe pulldown.

What do you mean "restore the behaviour of the rockchip kernel"? Did
mainline behave the same as the rockchip kernel previously? If not,
using "restore" here is misleading. "Unconditionally" is also incorrect,
because you have a property that disables it.

> As the DT property rockchip,enable-strobe-pulldown is obsolete now,
> replace it with a property to disable the internal pulldown.
> 
> This fixes I/O errors observed on various Rock Pi 4 and NanoPi4 series
> boards with some eMMC modules. Other boards may also be affected.
> 
> An example of these errors is as follows:
> 
> [  290.060817] mmc1: running CQE recovery
> [  290.061337] blk_update_request: I/O error, dev mmcblk1, sector 1411072 op 0x1:(WRITE) flags 0x800 phys_seg 36 prio class 0
> [  290.061370] EXT4-fs warning (device mmcblk1p1): ext4_end_bio:348: I/O error 10 writing to inode 29547 starting block 176466)
> [  290.061484] Buffer I/O error on device mmcblk1p1, logical block 172288
> 
> Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe line in dts")
> Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
> ---
>  drivers/phy/rockchip/phy-rockchip-emmc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
> index 20023f6eb994..6e637f3e1b19 100644
> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
> @@ -376,14 +376,14 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>  	rk_phy->reg_offset = reg_offset;
>  	rk_phy->reg_base = grf;
>  	rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
> -	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
> +	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
>  	rk_phy->output_tapdelay_select = PHYCTRL_OTAPDLYSEL_DEFAULT;
>  
>  	if (!of_property_read_u32(dev->of_node, "drive-impedance-ohm", &val))
>  		rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val);
>  
> -	if (of_property_read_bool(dev->of_node, "rockchip,enable-strobe-pulldown"))
> -		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
> +	if (of_property_read_bool(dev->of_node, "rockchip,disable-strobe-pulldown"))
> +		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;

Unfortunately you cannot do this.
Previously no property at all meant disabled and a property was required
to enable it. With this change the absence of a property means that it
will be enabled.
An old devicetree is that wanted this to be disabled would have no
property and will now end up with it enabled. This is an ABI break and is
clearly not backwards compatible, that's a NAK unless it is demonstrable
that noone actually wants to disable it at all.

If this patch fixes a problem on a board that you have, I would suggest
that you add the property to enable it, as the binding tells you to.

Thanks,
Conor.

>  	if (!of_property_read_u32(dev->of_node, "rockchip,output-tapdelay-select", &val)) {
>  		if (val <= PHYCTRL_OTAPDLYSEL_MAXVALUE)
> 
> -- 
> 2.44.0
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 3387 bytes --]

On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4 Relay wrote:
> From: Folker Schwesinger <dev@folker-schwesinger.de>
> 
> Restore the behavior of the Rockchip kernel that undconditionally
> enables the internal strobe pulldown.

What do you mean "restore the behaviour of the rockchip kernel"? Did
mainline behave the same as the rockchip kernel previously? If not,
using "restore" here is misleading. "Unconditionally" is also incorrect,
because you have a property that disables it.

> As the DT property rockchip,enable-strobe-pulldown is obsolete now,
> replace it with a property to disable the internal pulldown.
> 
> This fixes I/O errors observed on various Rock Pi 4 and NanoPi4 series
> boards with some eMMC modules. Other boards may also be affected.
> 
> An example of these errors is as follows:
> 
> [  290.060817] mmc1: running CQE recovery
> [  290.061337] blk_update_request: I/O error, dev mmcblk1, sector 1411072 op 0x1:(WRITE) flags 0x800 phys_seg 36 prio class 0
> [  290.061370] EXT4-fs warning (device mmcblk1p1): ext4_end_bio:348: I/O error 10 writing to inode 29547 starting block 176466)
> [  290.061484] Buffer I/O error on device mmcblk1p1, logical block 172288
> 
> Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe line in dts")
> Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
> ---
>  drivers/phy/rockchip/phy-rockchip-emmc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
> index 20023f6eb994..6e637f3e1b19 100644
> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
> @@ -376,14 +376,14 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>  	rk_phy->reg_offset = reg_offset;
>  	rk_phy->reg_base = grf;
>  	rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
> -	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
> +	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
>  	rk_phy->output_tapdelay_select = PHYCTRL_OTAPDLYSEL_DEFAULT;
>  
>  	if (!of_property_read_u32(dev->of_node, "drive-impedance-ohm", &val))
>  		rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val);
>  
> -	if (of_property_read_bool(dev->of_node, "rockchip,enable-strobe-pulldown"))
> -		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
> +	if (of_property_read_bool(dev->of_node, "rockchip,disable-strobe-pulldown"))
> +		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;

Unfortunately you cannot do this.
Previously no property at all meant disabled and a property was required
to enable it. With this change the absence of a property means that it
will be enabled.
An old devicetree is that wanted this to be disabled would have no
property and will now end up with it enabled. This is an ABI break and is
clearly not backwards compatible, that's a NAK unless it is demonstrable
that noone actually wants to disable it at all.

If this patch fixes a problem on a board that you have, I would suggest
that you add the property to enable it, as the binding tells you to.

Thanks,
Conor.

>  	if (!of_property_read_u32(dev->of_node, "rockchip,output-tapdelay-select", &val)) {
>  		if (val <= PHYCTRL_OTAPDLYSEL_MAXVALUE)
> 
> -- 
> 2.44.0
> 
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 3387 bytes --]

On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4 Relay wrote:
> From: Folker Schwesinger <dev@folker-schwesinger.de>
> 
> Restore the behavior of the Rockchip kernel that undconditionally
> enables the internal strobe pulldown.

What do you mean "restore the behaviour of the rockchip kernel"? Did
mainline behave the same as the rockchip kernel previously? If not,
using "restore" here is misleading. "Unconditionally" is also incorrect,
because you have a property that disables it.

> As the DT property rockchip,enable-strobe-pulldown is obsolete now,
> replace it with a property to disable the internal pulldown.
> 
> This fixes I/O errors observed on various Rock Pi 4 and NanoPi4 series
> boards with some eMMC modules. Other boards may also be affected.
> 
> An example of these errors is as follows:
> 
> [  290.060817] mmc1: running CQE recovery
> [  290.061337] blk_update_request: I/O error, dev mmcblk1, sector 1411072 op 0x1:(WRITE) flags 0x800 phys_seg 36 prio class 0
> [  290.061370] EXT4-fs warning (device mmcblk1p1): ext4_end_bio:348: I/O error 10 writing to inode 29547 starting block 176466)
> [  290.061484] Buffer I/O error on device mmcblk1p1, logical block 172288
> 
> Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe line in dts")
> Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
> ---
>  drivers/phy/rockchip/phy-rockchip-emmc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
> index 20023f6eb994..6e637f3e1b19 100644
> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
> @@ -376,14 +376,14 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>  	rk_phy->reg_offset = reg_offset;
>  	rk_phy->reg_base = grf;
>  	rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
> -	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
> +	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
>  	rk_phy->output_tapdelay_select = PHYCTRL_OTAPDLYSEL_DEFAULT;
>  
>  	if (!of_property_read_u32(dev->of_node, "drive-impedance-ohm", &val))
>  		rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val);
>  
> -	if (of_property_read_bool(dev->of_node, "rockchip,enable-strobe-pulldown"))
> -		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
> +	if (of_property_read_bool(dev->of_node, "rockchip,disable-strobe-pulldown"))
> +		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;

Unfortunately you cannot do this.
Previously no property at all meant disabled and a property was required
to enable it. With this change the absence of a property means that it
will be enabled.
An old devicetree is that wanted this to be disabled would have no
property and will now end up with it enabled. This is an ABI break and is
clearly not backwards compatible, that's a NAK unless it is demonstrable
that noone actually wants to disable it at all.

If this patch fixes a problem on a board that you have, I would suggest
that you add the property to enable it, as the binding tells you to.

Thanks,
Conor.

>  	if (!of_property_read_u32(dev->of_node, "rockchip,output-tapdelay-select", &val)) {
>  		if (val <= PHYCTRL_OTAPDLYSEL_MAXVALUE)
> 
> -- 
> 2.44.0
> 
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 3387 bytes --]

On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4 Relay wrote:
> From: Folker Schwesinger <dev@folker-schwesinger.de>
> 
> Restore the behavior of the Rockchip kernel that undconditionally
> enables the internal strobe pulldown.

What do you mean "restore the behaviour of the rockchip kernel"? Did
mainline behave the same as the rockchip kernel previously? If not,
using "restore" here is misleading. "Unconditionally" is also incorrect,
because you have a property that disables it.

> As the DT property rockchip,enable-strobe-pulldown is obsolete now,
> replace it with a property to disable the internal pulldown.
> 
> This fixes I/O errors observed on various Rock Pi 4 and NanoPi4 series
> boards with some eMMC modules. Other boards may also be affected.
> 
> An example of these errors is as follows:
> 
> [  290.060817] mmc1: running CQE recovery
> [  290.061337] blk_update_request: I/O error, dev mmcblk1, sector 1411072 op 0x1:(WRITE) flags 0x800 phys_seg 36 prio class 0
> [  290.061370] EXT4-fs warning (device mmcblk1p1): ext4_end_bio:348: I/O error 10 writing to inode 29547 starting block 176466)
> [  290.061484] Buffer I/O error on device mmcblk1p1, logical block 172288
> 
> Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe line in dts")
> Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
> ---
>  drivers/phy/rockchip/phy-rockchip-emmc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c b/drivers/phy/rockchip/phy-rockchip-emmc.c
> index 20023f6eb994..6e637f3e1b19 100644
> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
> @@ -376,14 +376,14 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
>  	rk_phy->reg_offset = reg_offset;
>  	rk_phy->reg_base = grf;
>  	rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
> -	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
> +	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
>  	rk_phy->output_tapdelay_select = PHYCTRL_OTAPDLYSEL_DEFAULT;
>  
>  	if (!of_property_read_u32(dev->of_node, "drive-impedance-ohm", &val))
>  		rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val);
>  
> -	if (of_property_read_bool(dev->of_node, "rockchip,enable-strobe-pulldown"))
> -		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
> +	if (of_property_read_bool(dev->of_node, "rockchip,disable-strobe-pulldown"))
> +		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;

Unfortunately you cannot do this.
Previously no property at all meant disabled and a property was required
to enable it. With this change the absence of a property means that it
will be enabled.
An old devicetree is that wanted this to be disabled would have no
property and will now end up with it enabled. This is an ABI break and is
clearly not backwards compatible, that's a NAK unless it is demonstrable
that noone actually wants to disable it at all.

If this patch fixes a problem on a board that you have, I would suggest
that you add the property to enable it, as the binding tells you to.

Thanks,
Conor.

>  	if (!of_property_read_u32(dev->of_node, "rockchip,output-tapdelay-select", &val)) {
>  		if (val <= PHYCTRL_OTAPDLYSEL_MAXVALUE)
> 
> -- 
> 2.44.0
> 
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Dragan Simic]

Hello Conor and Folker,

On 2024-03-26 20:46, Conor Dooley wrote:
> On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4 
> Relay wrote:
>> From: Folker Schwesinger <dev@folker-schwesinger.de>
>> 
>> Restore the behavior of the Rockchip kernel that undconditionally
>> enables the internal strobe pulldown.
> 
> What do you mean "restore the behaviour of the rockchip kernel"? Did
> mainline behave the same as the rockchip kernel previously? If not,
> using "restore" here is misleading. "Unconditionally" is also 
> incorrect,
> because you have a property that disables it.
> 
>> As the DT property rockchip,enable-strobe-pulldown is obsolete now,
>> replace it with a property to disable the internal pulldown.
>> 
>> This fixes I/O errors observed on various Rock Pi 4 and NanoPi4 series
>> boards with some eMMC modules. Other boards may also be affected.
>> 
>> An example of these errors is as follows:
>> 
>> [  290.060817] mmc1: running CQE recovery
>> [  290.061337] blk_update_request: I/O error, dev mmcblk1, sector 
>> 1411072 op 0x1:(WRITE) flags 0x800 phys_seg 36 prio class 0
>> [  290.061370] EXT4-fs warning (device mmcblk1p1): ext4_end_bio:348: 
>> I/O error 10 writing to inode 29547 starting block 176466)
>> [  290.061484] Buffer I/O error on device mmcblk1p1, logical block 
>> 172288
>> 
>> Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe line in 
>> dts")
>> Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
>> ---
>>  drivers/phy/rockchip/phy-rockchip-emmc.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c 
>> b/drivers/phy/rockchip/phy-rockchip-emmc.c
>> index 20023f6eb994..6e637f3e1b19 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
>> @@ -376,14 +376,14 @@ static int rockchip_emmc_phy_probe(struct 
>> platform_device *pdev)
>>  	rk_phy->reg_offset = reg_offset;
>>  	rk_phy->reg_base = grf;
>>  	rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
>> -	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
>> +	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
>>  	rk_phy->output_tapdelay_select = PHYCTRL_OTAPDLYSEL_DEFAULT;
>> 
>>  	if (!of_property_read_u32(dev->of_node, "drive-impedance-ohm", 
>> &val))
>>  		rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val);
>> 
>> -	if (of_property_read_bool(dev->of_node, 
>> "rockchip,enable-strobe-pulldown"))
>> -		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
>> +	if (of_property_read_bool(dev->of_node, 
>> "rockchip,disable-strobe-pulldown"))
>> +		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
> 
> Unfortunately you cannot do this.
> Previously no property at all meant disabled and a property was 
> required
> to enable it. With this change the absence of a property means that it
> will be enabled.
> An old devicetree is that wanted this to be disabled would have no
> property and will now end up with it enabled. This is an ABI break and 
> is
> clearly not backwards compatible, that's a NAK unless it is 
> demonstrable
> that noone actually wants to disable it at all.

Moreover, as I already explained some time ago, [1] some boards and
devices are unfortunately miswired, and we don't want to enable the
DATA STROBE pull-down on such boards.

[1] 
https://lore.kernel.org/linux-rockchip/ca5b7cad01f645c7c559ab26a8db8085@manjaro.org/#t

> If this patch fixes a problem on a board that you have, I would suggest
> that you add the property to enable it, as the binding tells you to.
> 
> Thanks,
> Conor.
> 
>>  	if (!of_property_read_u32(dev->of_node, 
>> "rockchip,output-tapdelay-select", &val)) {
>>  		if (val <= PHYCTRL_OTAPDLYSEL_MAXVALUE)
>> 
>> --
>> 2.44.0
>> 
>> 
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Dragan Simic]

Hello Conor and Folker,

On 2024-03-26 20:46, Conor Dooley wrote:
> On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4 
> Relay wrote:
>> From: Folker Schwesinger <dev@folker-schwesinger.de>
>> 
>> Restore the behavior of the Rockchip kernel that undconditionally
>> enables the internal strobe pulldown.
> 
> What do you mean "restore the behaviour of the rockchip kernel"? Did
> mainline behave the same as the rockchip kernel previously? If not,
> using "restore" here is misleading. "Unconditionally" is also 
> incorrect,
> because you have a property that disables it.
> 
>> As the DT property rockchip,enable-strobe-pulldown is obsolete now,
>> replace it with a property to disable the internal pulldown.
>> 
>> This fixes I/O errors observed on various Rock Pi 4 and NanoPi4 series
>> boards with some eMMC modules. Other boards may also be affected.
>> 
>> An example of these errors is as follows:
>> 
>> [  290.060817] mmc1: running CQE recovery
>> [  290.061337] blk_update_request: I/O error, dev mmcblk1, sector 
>> 1411072 op 0x1:(WRITE) flags 0x800 phys_seg 36 prio class 0
>> [  290.061370] EXT4-fs warning (device mmcblk1p1): ext4_end_bio:348: 
>> I/O error 10 writing to inode 29547 starting block 176466)
>> [  290.061484] Buffer I/O error on device mmcblk1p1, logical block 
>> 172288
>> 
>> Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe line in 
>> dts")
>> Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
>> ---
>>  drivers/phy/rockchip/phy-rockchip-emmc.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c 
>> b/drivers/phy/rockchip/phy-rockchip-emmc.c
>> index 20023f6eb994..6e637f3e1b19 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
>> @@ -376,14 +376,14 @@ static int rockchip_emmc_phy_probe(struct 
>> platform_device *pdev)
>>  	rk_phy->reg_offset = reg_offset;
>>  	rk_phy->reg_base = grf;
>>  	rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
>> -	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
>> +	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
>>  	rk_phy->output_tapdelay_select = PHYCTRL_OTAPDLYSEL_DEFAULT;
>> 
>>  	if (!of_property_read_u32(dev->of_node, "drive-impedance-ohm", 
>> &val))
>>  		rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val);
>> 
>> -	if (of_property_read_bool(dev->of_node, 
>> "rockchip,enable-strobe-pulldown"))
>> -		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
>> +	if (of_property_read_bool(dev->of_node, 
>> "rockchip,disable-strobe-pulldown"))
>> +		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
> 
> Unfortunately you cannot do this.
> Previously no property at all meant disabled and a property was 
> required
> to enable it. With this change the absence of a property means that it
> will be enabled.
> An old devicetree is that wanted this to be disabled would have no
> property and will now end up with it enabled. This is an ABI break and 
> is
> clearly not backwards compatible, that's a NAK unless it is 
> demonstrable
> that noone actually wants to disable it at all.

Moreover, as I already explained some time ago, [1] some boards and
devices are unfortunately miswired, and we don't want to enable the
DATA STROBE pull-down on such boards.

[1] 
https://lore.kernel.org/linux-rockchip/ca5b7cad01f645c7c559ab26a8db8085@manjaro.org/#t

> If this patch fixes a problem on a board that you have, I would suggest
> that you add the property to enable it, as the binding tells you to.
> 
> Thanks,
> Conor.
> 
>>  	if (!of_property_read_u32(dev->of_node, 
>> "rockchip,output-tapdelay-select", &val)) {
>>  		if (val <= PHYCTRL_OTAPDLYSEL_MAXVALUE)
>> 
>> --
>> 2.44.0
>> 
>> 
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Dragan Simic]

Hello Conor and Folker,

On 2024-03-26 20:46, Conor Dooley wrote:
> On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4 
> Relay wrote:
>> From: Folker Schwesinger <dev@folker-schwesinger.de>
>> 
>> Restore the behavior of the Rockchip kernel that undconditionally
>> enables the internal strobe pulldown.
> 
> What do you mean "restore the behaviour of the rockchip kernel"? Did
> mainline behave the same as the rockchip kernel previously? If not,
> using "restore" here is misleading. "Unconditionally" is also 
> incorrect,
> because you have a property that disables it.
> 
>> As the DT property rockchip,enable-strobe-pulldown is obsolete now,
>> replace it with a property to disable the internal pulldown.
>> 
>> This fixes I/O errors observed on various Rock Pi 4 and NanoPi4 series
>> boards with some eMMC modules. Other boards may also be affected.
>> 
>> An example of these errors is as follows:
>> 
>> [  290.060817] mmc1: running CQE recovery
>> [  290.061337] blk_update_request: I/O error, dev mmcblk1, sector 
>> 1411072 op 0x1:(WRITE) flags 0x800 phys_seg 36 prio class 0
>> [  290.061370] EXT4-fs warning (device mmcblk1p1): ext4_end_bio:348: 
>> I/O error 10 writing to inode 29547 starting block 176466)
>> [  290.061484] Buffer I/O error on device mmcblk1p1, logical block 
>> 172288
>> 
>> Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe line in 
>> dts")
>> Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
>> ---
>>  drivers/phy/rockchip/phy-rockchip-emmc.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c 
>> b/drivers/phy/rockchip/phy-rockchip-emmc.c
>> index 20023f6eb994..6e637f3e1b19 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
>> @@ -376,14 +376,14 @@ static int rockchip_emmc_phy_probe(struct 
>> platform_device *pdev)
>>  	rk_phy->reg_offset = reg_offset;
>>  	rk_phy->reg_base = grf;
>>  	rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
>> -	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
>> +	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
>>  	rk_phy->output_tapdelay_select = PHYCTRL_OTAPDLYSEL_DEFAULT;
>> 
>>  	if (!of_property_read_u32(dev->of_node, "drive-impedance-ohm", 
>> &val))
>>  		rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val);
>> 
>> -	if (of_property_read_bool(dev->of_node, 
>> "rockchip,enable-strobe-pulldown"))
>> -		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
>> +	if (of_property_read_bool(dev->of_node, 
>> "rockchip,disable-strobe-pulldown"))
>> +		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
> 
> Unfortunately you cannot do this.
> Previously no property at all meant disabled and a property was 
> required
> to enable it. With this change the absence of a property means that it
> will be enabled.
> An old devicetree is that wanted this to be disabled would have no
> property and will now end up with it enabled. This is an ABI break and 
> is
> clearly not backwards compatible, that's a NAK unless it is 
> demonstrable
> that noone actually wants to disable it at all.

Moreover, as I already explained some time ago, [1] some boards and
devices are unfortunately miswired, and we don't want to enable the
DATA STROBE pull-down on such boards.

[1] 
https://lore.kernel.org/linux-rockchip/ca5b7cad01f645c7c559ab26a8db8085@manjaro.org/#t

> If this patch fixes a problem on a board that you have, I would suggest
> that you add the property to enable it, as the binding tells you to.
> 
> Thanks,
> Conor.
> 
>>  	if (!of_property_read_u32(dev->of_node, 
>> "rockchip,output-tapdelay-select", &val)) {
>>  		if (val <= PHYCTRL_OTAPDLYSEL_MAXVALUE)
>> 
>> --
>> 2.44.0
>> 
>> 
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Dragan Simic]

Hello Conor and Folker,

On 2024-03-26 20:46, Conor Dooley wrote:
> On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4 
> Relay wrote:
>> From: Folker Schwesinger <dev@folker-schwesinger.de>
>> 
>> Restore the behavior of the Rockchip kernel that undconditionally
>> enables the internal strobe pulldown.
> 
> What do you mean "restore the behaviour of the rockchip kernel"? Did
> mainline behave the same as the rockchip kernel previously? If not,
> using "restore" here is misleading. "Unconditionally" is also 
> incorrect,
> because you have a property that disables it.
> 
>> As the DT property rockchip,enable-strobe-pulldown is obsolete now,
>> replace it with a property to disable the internal pulldown.
>> 
>> This fixes I/O errors observed on various Rock Pi 4 and NanoPi4 series
>> boards with some eMMC modules. Other boards may also be affected.
>> 
>> An example of these errors is as follows:
>> 
>> [  290.060817] mmc1: running CQE recovery
>> [  290.061337] blk_update_request: I/O error, dev mmcblk1, sector 
>> 1411072 op 0x1:(WRITE) flags 0x800 phys_seg 36 prio class 0
>> [  290.061370] EXT4-fs warning (device mmcblk1p1): ext4_end_bio:348: 
>> I/O error 10 writing to inode 29547 starting block 176466)
>> [  290.061484] Buffer I/O error on device mmcblk1p1, logical block 
>> 172288
>> 
>> Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe line in 
>> dts")
>> Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
>> ---
>>  drivers/phy/rockchip/phy-rockchip-emmc.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c 
>> b/drivers/phy/rockchip/phy-rockchip-emmc.c
>> index 20023f6eb994..6e637f3e1b19 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
>> @@ -376,14 +376,14 @@ static int rockchip_emmc_phy_probe(struct 
>> platform_device *pdev)
>>  	rk_phy->reg_offset = reg_offset;
>>  	rk_phy->reg_base = grf;
>>  	rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
>> -	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
>> +	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
>>  	rk_phy->output_tapdelay_select = PHYCTRL_OTAPDLYSEL_DEFAULT;
>> 
>>  	if (!of_property_read_u32(dev->of_node, "drive-impedance-ohm", 
>> &val))
>>  		rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val);
>> 
>> -	if (of_property_read_bool(dev->of_node, 
>> "rockchip,enable-strobe-pulldown"))
>> -		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
>> +	if (of_property_read_bool(dev->of_node, 
>> "rockchip,disable-strobe-pulldown"))
>> +		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
> 
> Unfortunately you cannot do this.
> Previously no property at all meant disabled and a property was 
> required
> to enable it. With this change the absence of a property means that it
> will be enabled.
> An old devicetree is that wanted this to be disabled would have no
> property and will now end up with it enabled. This is an ABI break and 
> is
> clearly not backwards compatible, that's a NAK unless it is 
> demonstrable
> that noone actually wants to disable it at all.

Moreover, as I already explained some time ago, [1] some boards and
devices are unfortunately miswired, and we don't want to enable the
DATA STROBE pull-down on such boards.

[1] 
https://lore.kernel.org/linux-rockchip/ca5b7cad01f645c7c559ab26a8db8085@manjaro.org/#t

> If this patch fixes a problem on a board that you have, I would suggest
> that you add the property to enable it, as the binding tells you to.
> 
> Thanks,
> Conor.
> 
>>  	if (!of_property_read_u32(dev->of_node, 
>> "rockchip,output-tapdelay-select", &val)) {
>>  		if (val <= PHYCTRL_OTAPDLYSEL_MAXVALUE)
>> 
>> --
>> 2.44.0
>> 
>> 
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Folker Schwesinger]

[-- Attachment #1.1: Type: text/plain, Size: 3995 bytes --]

Hi Conor and Dragan,

thanks for your feedback!

On Tue Mar 26, 2024 at 8:55 PM CET, Dragan Simic wrote:
> On 2024-03-26 20:46, Conor Dooley wrote:
> > On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4
> > Relay wrote:
> >> From: Folker Schwesinger <dev@folker-schwesinger.de>
> >>
> >> Restore the behavior of the Rockchip kernel that undconditionally
> >> enables the internal strobe pulldown.
> >
> > What do you mean "restore the behaviour of the rockchip kernel"? Did
> > mainline behave the same as the rockchip kernel previously? If not,
> > using "restore" here is misleading. "Unconditionally" is also
> > incorrect,
> > because you have a property that disables it.

Apologizes for the misleading commit message. Prior to 5.11 the Linux
kernel did not touch the pull-down registers. However, it seems the
register's (factory?) default was set to enable the pull-down. As it
was mentioned elsewhere that was the configuration recommended by
Rockchip. The 4.4 vendor (Rockchip) kernel reflects that by enabling the
pull-down in its kernel.
Of course, this has nothing to do with the Linux kernel, so "restore"
was a bad choice here.

I previously had split the driver patch into two separate patches, one
for changing the default (unconditionally at that point), the other for
adding the disable property. As both changes were minimal I decided to
squash the commits. I updated the cover letter, but forgot to update the
commit message. Sorry.

> >> Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe line in
> >> dts")
> >> Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
> >> ---
> >>  drivers/phy/rockchip/phy-rockchip-emmc.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c
> >> b/drivers/phy/rockchip/phy-rockchip-emmc.c
> >> index 20023f6eb994..6e637f3e1b19 100644
> >> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
> >> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
> >> @@ -376,14 +376,14 @@ static int rockchip_emmc_phy_probe(struct
> >> platform_device *pdev)
> >>  	rk_phy->reg_offset = reg_offset;
> >>  	rk_phy->reg_base = grf;
> >>  	rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
> >> -	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
> >> +	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
> >>  	rk_phy->output_tapdelay_select = PHYCTRL_OTAPDLYSEL_DEFAULT;
> >>
> >>  	if (!of_property_read_u32(dev->of_node, "drive-impedance-ohm",
> >> &val))
> >>  		rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val);
> >>
> >> -	if (of_property_read_bool(dev->of_node,
> >> "rockchip,enable-strobe-pulldown"))
> >> -		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
> >> +	if (of_property_read_bool(dev->of_node,
> >> "rockchip,disable-strobe-pulldown"))
> >> +		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
> >
> > Unfortunately you cannot do this.
> > Previously no property at all meant disabled and a property was
> > required
> > to enable it. With this change the absence of a property means that it
> > will be enabled.
> > An old devicetree is that wanted this to be disabled would have no
> > property and will now end up with it enabled. This is an ABI break and
> > is
> > clearly not backwards compatible, that's a NAK unless it is
> > demonstrable
> > that noone actually wants to disable it at all.
> >
>
> Moreover, as I already explained some time ago, [1] some boards and
> devices are unfortunately miswired, and we don't want to enable the
> DATA STROBE pull-down on such boards.
>
> [1]
> https://lore.kernel.org/linux-rockchip/ca5b7cad01f645c7c559ab26a8db8085@manjaro.org/#t
>
> > If this patch fixes a problem on a board that you have, I would suggest
> > that you add the property to enable it, as the binding tells you to.

I agree, I'll post the patches later.

Best regards

Folker


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Folker Schwesinger]

[-- Attachment #1.1: Type: text/plain, Size: 3995 bytes --]

Hi Conor and Dragan,

thanks for your feedback!

On Tue Mar 26, 2024 at 8:55 PM CET, Dragan Simic wrote:
> On 2024-03-26 20:46, Conor Dooley wrote:
> > On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4
> > Relay wrote:
> >> From: Folker Schwesinger <dev@folker-schwesinger.de>
> >>
> >> Restore the behavior of the Rockchip kernel that undconditionally
> >> enables the internal strobe pulldown.
> >
> > What do you mean "restore the behaviour of the rockchip kernel"? Did
> > mainline behave the same as the rockchip kernel previously? If not,
> > using "restore" here is misleading. "Unconditionally" is also
> > incorrect,
> > because you have a property that disables it.

Apologizes for the misleading commit message. Prior to 5.11 the Linux
kernel did not touch the pull-down registers. However, it seems the
register's (factory?) default was set to enable the pull-down. As it
was mentioned elsewhere that was the configuration recommended by
Rockchip. The 4.4 vendor (Rockchip) kernel reflects that by enabling the
pull-down in its kernel.
Of course, this has nothing to do with the Linux kernel, so "restore"
was a bad choice here.

I previously had split the driver patch into two separate patches, one
for changing the default (unconditionally at that point), the other for
adding the disable property. As both changes were minimal I decided to
squash the commits. I updated the cover letter, but forgot to update the
commit message. Sorry.

> >> Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe line in
> >> dts")
> >> Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
> >> ---
> >>  drivers/phy/rockchip/phy-rockchip-emmc.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c
> >> b/drivers/phy/rockchip/phy-rockchip-emmc.c
> >> index 20023f6eb994..6e637f3e1b19 100644
> >> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
> >> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
> >> @@ -376,14 +376,14 @@ static int rockchip_emmc_phy_probe(struct
> >> platform_device *pdev)
> >>  	rk_phy->reg_offset = reg_offset;
> >>  	rk_phy->reg_base = grf;
> >>  	rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
> >> -	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
> >> +	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
> >>  	rk_phy->output_tapdelay_select = PHYCTRL_OTAPDLYSEL_DEFAULT;
> >>
> >>  	if (!of_property_read_u32(dev->of_node, "drive-impedance-ohm",
> >> &val))
> >>  		rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val);
> >>
> >> -	if (of_property_read_bool(dev->of_node,
> >> "rockchip,enable-strobe-pulldown"))
> >> -		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
> >> +	if (of_property_read_bool(dev->of_node,
> >> "rockchip,disable-strobe-pulldown"))
> >> +		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
> >
> > Unfortunately you cannot do this.
> > Previously no property at all meant disabled and a property was
> > required
> > to enable it. With this change the absence of a property means that it
> > will be enabled.
> > An old devicetree is that wanted this to be disabled would have no
> > property and will now end up with it enabled. This is an ABI break and
> > is
> > clearly not backwards compatible, that's a NAK unless it is
> > demonstrable
> > that noone actually wants to disable it at all.
> >
>
> Moreover, as I already explained some time ago, [1] some boards and
> devices are unfortunately miswired, and we don't want to enable the
> DATA STROBE pull-down on such boards.
>
> [1]
> https://lore.kernel.org/linux-rockchip/ca5b7cad01f645c7c559ab26a8db8085@manjaro.org/#t
>
> > If this patch fixes a problem on a board that you have, I would suggest
> > that you add the property to enable it, as the binding tells you to.

I agree, I'll post the patches later.

Best regards

Folker


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Folker Schwesinger]

[-- Attachment #1.1: Type: text/plain, Size: 3995 bytes --]

Hi Conor and Dragan,

thanks for your feedback!

On Tue Mar 26, 2024 at 8:55 PM CET, Dragan Simic wrote:
> On 2024-03-26 20:46, Conor Dooley wrote:
> > On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4
> > Relay wrote:
> >> From: Folker Schwesinger <dev@folker-schwesinger.de>
> >>
> >> Restore the behavior of the Rockchip kernel that undconditionally
> >> enables the internal strobe pulldown.
> >
> > What do you mean "restore the behaviour of the rockchip kernel"? Did
> > mainline behave the same as the rockchip kernel previously? If not,
> > using "restore" here is misleading. "Unconditionally" is also
> > incorrect,
> > because you have a property that disables it.

Apologizes for the misleading commit message. Prior to 5.11 the Linux
kernel did not touch the pull-down registers. However, it seems the
register's (factory?) default was set to enable the pull-down. As it
was mentioned elsewhere that was the configuration recommended by
Rockchip. The 4.4 vendor (Rockchip) kernel reflects that by enabling the
pull-down in its kernel.
Of course, this has nothing to do with the Linux kernel, so "restore"
was a bad choice here.

I previously had split the driver patch into two separate patches, one
for changing the default (unconditionally at that point), the other for
adding the disable property. As both changes were minimal I decided to
squash the commits. I updated the cover letter, but forgot to update the
commit message. Sorry.

> >> Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe line in
> >> dts")
> >> Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
> >> ---
> >>  drivers/phy/rockchip/phy-rockchip-emmc.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c
> >> b/drivers/phy/rockchip/phy-rockchip-emmc.c
> >> index 20023f6eb994..6e637f3e1b19 100644
> >> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
> >> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
> >> @@ -376,14 +376,14 @@ static int rockchip_emmc_phy_probe(struct
> >> platform_device *pdev)
> >>  	rk_phy->reg_offset = reg_offset;
> >>  	rk_phy->reg_base = grf;
> >>  	rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
> >> -	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
> >> +	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
> >>  	rk_phy->output_tapdelay_select = PHYCTRL_OTAPDLYSEL_DEFAULT;
> >>
> >>  	if (!of_property_read_u32(dev->of_node, "drive-impedance-ohm",
> >> &val))
> >>  		rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val);
> >>
> >> -	if (of_property_read_bool(dev->of_node,
> >> "rockchip,enable-strobe-pulldown"))
> >> -		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
> >> +	if (of_property_read_bool(dev->of_node,
> >> "rockchip,disable-strobe-pulldown"))
> >> +		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
> >
> > Unfortunately you cannot do this.
> > Previously no property at all meant disabled and a property was
> > required
> > to enable it. With this change the absence of a property means that it
> > will be enabled.
> > An old devicetree is that wanted this to be disabled would have no
> > property and will now end up with it enabled. This is an ABI break and
> > is
> > clearly not backwards compatible, that's a NAK unless it is
> > demonstrable
> > that noone actually wants to disable it at all.
> >
>
> Moreover, as I already explained some time ago, [1] some boards and
> devices are unfortunately miswired, and we don't want to enable the
> DATA STROBE pull-down on such boards.
>
> [1]
> https://lore.kernel.org/linux-rockchip/ca5b7cad01f645c7c559ab26a8db8085@manjaro.org/#t
>
> > If this patch fixes a problem on a board that you have, I would suggest
> > that you add the property to enable it, as the binding tells you to.

I agree, I'll post the patches later.

Best regards

Folker


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Folker Schwesinger]

[-- Attachment #1: Type: text/plain, Size: 3995 bytes --]

Hi Conor and Dragan,

thanks for your feedback!

On Tue Mar 26, 2024 at 8:55 PM CET, Dragan Simic wrote:
> On 2024-03-26 20:46, Conor Dooley wrote:
> > On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4
> > Relay wrote:
> >> From: Folker Schwesinger <dev@folker-schwesinger.de>
> >>
> >> Restore the behavior of the Rockchip kernel that undconditionally
> >> enables the internal strobe pulldown.
> >
> > What do you mean "restore the behaviour of the rockchip kernel"? Did
> > mainline behave the same as the rockchip kernel previously? If not,
> > using "restore" here is misleading. "Unconditionally" is also
> > incorrect,
> > because you have a property that disables it.

Apologizes for the misleading commit message. Prior to 5.11 the Linux
kernel did not touch the pull-down registers. However, it seems the
register's (factory?) default was set to enable the pull-down. As it
was mentioned elsewhere that was the configuration recommended by
Rockchip. The 4.4 vendor (Rockchip) kernel reflects that by enabling the
pull-down in its kernel.
Of course, this has nothing to do with the Linux kernel, so "restore"
was a bad choice here.

I previously had split the driver patch into two separate patches, one
for changing the default (unconditionally at that point), the other for
adding the disable property. As both changes were minimal I decided to
squash the commits. I updated the cover letter, but forgot to update the
commit message. Sorry.

> >> Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe line in
> >> dts")
> >> Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
> >> ---
> >>  drivers/phy/rockchip/phy-rockchip-emmc.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c
> >> b/drivers/phy/rockchip/phy-rockchip-emmc.c
> >> index 20023f6eb994..6e637f3e1b19 100644
> >> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
> >> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
> >> @@ -376,14 +376,14 @@ static int rockchip_emmc_phy_probe(struct
> >> platform_device *pdev)
> >>  	rk_phy->reg_offset = reg_offset;
> >>  	rk_phy->reg_base = grf;
> >>  	rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
> >> -	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
> >> +	rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
> >>  	rk_phy->output_tapdelay_select = PHYCTRL_OTAPDLYSEL_DEFAULT;
> >>
> >>  	if (!of_property_read_u32(dev->of_node, "drive-impedance-ohm",
> >> &val))
> >>  		rk_phy->drive_impedance = convert_drive_impedance_ohm(pdev, val);
> >>
> >> -	if (of_property_read_bool(dev->of_node,
> >> "rockchip,enable-strobe-pulldown"))
> >> -		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_ENABLE;
> >> +	if (of_property_read_bool(dev->of_node,
> >> "rockchip,disable-strobe-pulldown"))
> >> +		rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
> >
> > Unfortunately you cannot do this.
> > Previously no property at all meant disabled and a property was
> > required
> > to enable it. With this change the absence of a property means that it
> > will be enabled.
> > An old devicetree is that wanted this to be disabled would have no
> > property and will now end up with it enabled. This is an ABI break and
> > is
> > clearly not backwards compatible, that's a NAK unless it is
> > demonstrable
> > that noone actually wants to disable it at all.
> >
>
> Moreover, as I already explained some time ago, [1] some boards and
> devices are unfortunately miswired, and we don't want to enable the
> DATA STROBE pull-down on such boards.
>
> [1]
> https://lore.kernel.org/linux-rockchip/ca5b7cad01f645c7c559ab26a8db8085@manjaro.org/#t
>
> > If this patch fixes a problem on a board that you have, I would suggest
> > that you add the property to enable it, as the binding tells you to.

I agree, I'll post the patches later.

Best regards

Folker


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 1965 bytes --]

On Wed, Mar 27, 2024 at 04:21:45PM +0000, Folker Schwesinger wrote:
> Hi Conor and Dragan,
> 
> thanks for your feedback!
> 
> On Tue Mar 26, 2024 at 8:55 PM CET, Dragan Simic wrote:
> > On 2024-03-26 20:46, Conor Dooley wrote:
> > > On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4
> > > Relay wrote:
> > >> From: Folker Schwesinger <dev@folker-schwesinger.de>
> > >>
> > >> Restore the behavior of the Rockchip kernel that undconditionally
> > >> enables the internal strobe pulldown.
> > >
> > > What do you mean "restore the behaviour of the rockchip kernel"? Did
> > > mainline behave the same as the rockchip kernel previously? If not,
> > > using "restore" here is misleading. "Unconditionally" is also
> > > incorrect,
> > > because you have a property that disables it.
> 
> Apologizes for the misleading commit message. Prior to 5.11 the Linux
> kernel did not touch the pull-down registers. However, it seems the
> register's (factory?) default was set to enable the pull-down. As it
> was mentioned elsewhere that was the configuration recommended by
> Rockchip. The 4.4 vendor (Rockchip) kernel reflects that by enabling the
> pull-down in its kernel.

Yeah, seems like a bit of a sticky situation. Probably the wrong
polarity was chosen when the property was implemented and the property
should have been the one you wanted to switch to given the default
before it existed was the factory defaults.

> Of course, this has nothing to do with the Linux kernel, so "restore"
> was a bad choice here.
>
> I previously had split the driver patch into two separate patches, one
> for changing the default (unconditionally at that point), the other for
> adding the disable property. As both changes were minimal I decided to
> squash the commits. I updated the cover letter, but forgot to update the
> commit message. Sorry.

No worries. Squashing them was probably the right thing to do anyway.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 1965 bytes --]

On Wed, Mar 27, 2024 at 04:21:45PM +0000, Folker Schwesinger wrote:
> Hi Conor and Dragan,
> 
> thanks for your feedback!
> 
> On Tue Mar 26, 2024 at 8:55 PM CET, Dragan Simic wrote:
> > On 2024-03-26 20:46, Conor Dooley wrote:
> > > On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4
> > > Relay wrote:
> > >> From: Folker Schwesinger <dev@folker-schwesinger.de>
> > >>
> > >> Restore the behavior of the Rockchip kernel that undconditionally
> > >> enables the internal strobe pulldown.
> > >
> > > What do you mean "restore the behaviour of the rockchip kernel"? Did
> > > mainline behave the same as the rockchip kernel previously? If not,
> > > using "restore" here is misleading. "Unconditionally" is also
> > > incorrect,
> > > because you have a property that disables it.
> 
> Apologizes for the misleading commit message. Prior to 5.11 the Linux
> kernel did not touch the pull-down registers. However, it seems the
> register's (factory?) default was set to enable the pull-down. As it
> was mentioned elsewhere that was the configuration recommended by
> Rockchip. The 4.4 vendor (Rockchip) kernel reflects that by enabling the
> pull-down in its kernel.

Yeah, seems like a bit of a sticky situation. Probably the wrong
polarity was chosen when the property was implemented and the property
should have been the one you wanted to switch to given the default
before it existed was the factory defaults.

> Of course, this has nothing to do with the Linux kernel, so "restore"
> was a bad choice here.
>
> I previously had split the driver patch into two separate patches, one
> for changing the default (unconditionally at that point), the other for
> adding the disable property. As both changes were minimal I decided to
> squash the commits. I updated the cover letter, but forgot to update the
> commit message. Sorry.

No worries. Squashing them was probably the right thing to do anyway.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1965 bytes --]

On Wed, Mar 27, 2024 at 04:21:45PM +0000, Folker Schwesinger wrote:
> Hi Conor and Dragan,
> 
> thanks for your feedback!
> 
> On Tue Mar 26, 2024 at 8:55 PM CET, Dragan Simic wrote:
> > On 2024-03-26 20:46, Conor Dooley wrote:
> > > On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4
> > > Relay wrote:
> > >> From: Folker Schwesinger <dev@folker-schwesinger.de>
> > >>
> > >> Restore the behavior of the Rockchip kernel that undconditionally
> > >> enables the internal strobe pulldown.
> > >
> > > What do you mean "restore the behaviour of the rockchip kernel"? Did
> > > mainline behave the same as the rockchip kernel previously? If not,
> > > using "restore" here is misleading. "Unconditionally" is also
> > > incorrect,
> > > because you have a property that disables it.
> 
> Apologizes for the misleading commit message. Prior to 5.11 the Linux
> kernel did not touch the pull-down registers. However, it seems the
> register's (factory?) default was set to enable the pull-down. As it
> was mentioned elsewhere that was the configuration recommended by
> Rockchip. The 4.4 vendor (Rockchip) kernel reflects that by enabling the
> pull-down in its kernel.

Yeah, seems like a bit of a sticky situation. Probably the wrong
polarity was chosen when the property was implemented and the property
should have been the one you wanted to switch to given the default
before it existed was the factory defaults.

> Of course, this has nothing to do with the Linux kernel, so "restore"
> was a bad choice here.
>
> I previously had split the driver patch into two separate patches, one
> for changing the default (unconditionally at that point), the other for
> adding the disable property. As both changes were minimal I decided to
> squash the commits. I updated the cover letter, but forgot to update the
> commit message. Sorry.

No worries. Squashing them was probably the right thing to do anyway.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 1965 bytes --]

On Wed, Mar 27, 2024 at 04:21:45PM +0000, Folker Schwesinger wrote:
> Hi Conor and Dragan,
> 
> thanks for your feedback!
> 
> On Tue Mar 26, 2024 at 8:55 PM CET, Dragan Simic wrote:
> > On 2024-03-26 20:46, Conor Dooley wrote:
> > > On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4
> > > Relay wrote:
> > >> From: Folker Schwesinger <dev@folker-schwesinger.de>
> > >>
> > >> Restore the behavior of the Rockchip kernel that undconditionally
> > >> enables the internal strobe pulldown.
> > >
> > > What do you mean "restore the behaviour of the rockchip kernel"? Did
> > > mainline behave the same as the rockchip kernel previously? If not,
> > > using "restore" here is misleading. "Unconditionally" is also
> > > incorrect,
> > > because you have a property that disables it.
> 
> Apologizes for the misleading commit message. Prior to 5.11 the Linux
> kernel did not touch the pull-down registers. However, it seems the
> register's (factory?) default was set to enable the pull-down. As it
> was mentioned elsewhere that was the configuration recommended by
> Rockchip. The 4.4 vendor (Rockchip) kernel reflects that by enabling the
> pull-down in its kernel.

Yeah, seems like a bit of a sticky situation. Probably the wrong
polarity was chosen when the property was implemented and the property
should have been the one you wanted to switch to given the default
before it existed was the factory defaults.

> Of course, this has nothing to do with the Linux kernel, so "restore"
> was a bad choice here.
>
> I previously had split the driver patch into two separate patches, one
> for changing the default (unconditionally at that point), the other for
> adding the disable property. As both changes were minimal I decided to
> squash the commits. I updated the cover letter, but forgot to update the
> commit message. Sorry.

No worries. Squashing them was probably the right thing to do anyway.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] devicetree: phy: rockchip-emmc: Document changed strobe-pulldown property

[Krzysztof Kozlowski]

On 26/03/2024 19:54, Folker Schwesinger via B4 Relay wrote:
> From: Folker Schwesinger <dev@folker-schwesinger.de>
> 
> Document the changes regarding the optional strobe-pulldown property.
> These changes are necessary as the default behavior of the driver was
> restored to the Rockchip kernel behavior of enabling the internal
> pulldown by default.
> 

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.

It's: dt-bindings: phy: rockchi.......

Best regards,
Krzysztof

Re: [PATCH 2/3] devicetree: phy: rockchip-emmc: Document changed strobe-pulldown property

[Krzysztof Kozlowski]

On 26/03/2024 19:54, Folker Schwesinger via B4 Relay wrote:
> From: Folker Schwesinger <dev@folker-schwesinger.de>
> 
> Document the changes regarding the optional strobe-pulldown property.
> These changes are necessary as the default behavior of the driver was
> restored to the Rockchip kernel behavior of enabling the internal
> pulldown by default.
> 

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.

It's: dt-bindings: phy: rockchi.......

Best regards,
Krzysztof


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 2/3] devicetree: phy: rockchip-emmc: Document changed strobe-pulldown property

[Krzysztof Kozlowski]

On 26/03/2024 19:54, Folker Schwesinger via B4 Relay wrote:
> From: Folker Schwesinger <dev@folker-schwesinger.de>
> 
> Document the changes regarding the optional strobe-pulldown property.
> These changes are necessary as the default behavior of the driver was
> restored to the Rockchip kernel behavior of enabling the internal
> pulldown by default.
> 

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.

It's: dt-bindings: phy: rockchi.......

Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH 2/3] devicetree: phy: rockchip-emmc: Document changed strobe-pulldown property

[Krzysztof Kozlowski]

On 26/03/2024 19:54, Folker Schwesinger via B4 Relay wrote:
> From: Folker Schwesinger <dev@folker-schwesinger.de>
> 
> Document the changes regarding the optional strobe-pulldown property.
> These changes are necessary as the default behavior of the driver was
> restored to the Rockchip kernel behavior of enabling the internal
> pulldown by default.
> 

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.

It's: dt-bindings: phy: rockchi.......

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Alban Browaeys]

Le mardi 26 mars 2024 à 19:46 +0000, Conor Dooley a écrit :
> On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4
> Relay wrote:
> > From: Folker Schwesinger <dev@folker-schwesinger.de>
> > -	if (of_property_read_bool(dev->of_node, "rockchip,enable-
> > strobe-pulldown"))
> > -		rk_phy->enable_strobe_pulldown =
> > PHYCTRL_REN_STRB_ENABLE;
> > +	if (of_property_read_bool(dev->of_node, "rockchip,disable-
> > strobe-pulldown"))
> > +		rk_phy->enable_strobe_pulldown =
> > PHYCTRL_REN_STRB_DISABLE;
> 
> Unfortunately you cannot do this.
> Previously no property at all meant disabled and a property was
> required
> to enable it. With this change the absence of a property means that
> it
> will be enabled.
> An old devicetree is that wanted this to be disabled would have no
> property and will now end up with it enabled. This is an ABI break
> and is
> clearly not backwards compatible, that's a NAK unless it is
> demonstrable
> that noone actually wants to disable it at all.


But the patch that introduced the new default to disable the pulldown
explicitely introduced a regression for at least 4 boards.
It took time to sort out that the default to disable pulldown was the
culprit but still.
Will we carry this new behavor that breaks the default design for
rk3399 because since the regression was introduced new board definition
might have expceted this new behavior.

Could the best option be to revert to énot set a default enable/disable
pulldown" (as before the commit that introduced the regression) and
allow one to force the pulldown via the enable/disable pulldown
property?
I mean the commit that introduced a default value for the pulldown did
not seem to be about fixing anything. But it broke a lot. ANd it was
really really hard to find the description of this commit to understand
that one had to enable pulldown to restore hs400.

In more than 3 years, only one board maintainer noticed that this
property was required to get back HS400  and thanks to a user telling
me that this board was working I found from this board that this
property was "missing" from most board definitions (while it was not
required before).


I am all for not breaking ABI. But what about not reverting a patch
that already broke ABI because this patch introduced a new ABI that we
don't want to break?
I mean shouldn't a new commit with a new ABI that regressed the kernel
be reverted?

Mind fixing the initial regression 8b5c2b45b8f0 "phy: rockchip: set
pulldown for strobe line in dts" does not necessarily mean changing the
default to the opposite value but could also be reverting to not
setting a default.
Though I don't know if there are pros to setting a default.


> If this patch fixes a problem on a board that you have, I would
> suggest
> that you add the property to enable it, as the binding tells you to.
> 
> Thanks,
> Conor.


Regards,
Alban

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Alban Browaeys]

Le mardi 26 mars 2024 à 19:46 +0000, Conor Dooley a écrit :
> On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4
> Relay wrote:
> > From: Folker Schwesinger <dev@folker-schwesinger.de>
> > -	if (of_property_read_bool(dev->of_node, "rockchip,enable-
> > strobe-pulldown"))
> > -		rk_phy->enable_strobe_pulldown =
> > PHYCTRL_REN_STRB_ENABLE;
> > +	if (of_property_read_bool(dev->of_node, "rockchip,disable-
> > strobe-pulldown"))
> > +		rk_phy->enable_strobe_pulldown =
> > PHYCTRL_REN_STRB_DISABLE;
> 
> Unfortunately you cannot do this.
> Previously no property at all meant disabled and a property was
> required
> to enable it. With this change the absence of a property means that
> it
> will be enabled.
> An old devicetree is that wanted this to be disabled would have no
> property and will now end up with it enabled. This is an ABI break
> and is
> clearly not backwards compatible, that's a NAK unless it is
> demonstrable
> that noone actually wants to disable it at all.


But the patch that introduced the new default to disable the pulldown
explicitely introduced a regression for at least 4 boards.
It took time to sort out that the default to disable pulldown was the
culprit but still.
Will we carry this new behavor that breaks the default design for
rk3399 because since the regression was introduced new board definition
might have expceted this new behavior.

Could the best option be to revert to énot set a default enable/disable
pulldown" (as before the commit that introduced the regression) and
allow one to force the pulldown via the enable/disable pulldown
property?
I mean the commit that introduced a default value for the pulldown did
not seem to be about fixing anything. But it broke a lot. ANd it was
really really hard to find the description of this commit to understand
that one had to enable pulldown to restore hs400.

In more than 3 years, only one board maintainer noticed that this
property was required to get back HS400  and thanks to a user telling
me that this board was working I found from this board that this
property was "missing" from most board definitions (while it was not
required before).


I am all for not breaking ABI. But what about not reverting a patch
that already broke ABI because this patch introduced a new ABI that we
don't want to break?
I mean shouldn't a new commit with a new ABI that regressed the kernel
be reverted?

Mind fixing the initial regression 8b5c2b45b8f0 "phy: rockchip: set
pulldown for strobe line in dts" does not necessarily mean changing the
default to the opposite value but could also be reverting to not
setting a default.
Though I don't know if there are pros to setting a default.


> If this patch fixes a problem on a board that you have, I would
> suggest
> that you add the property to enable it, as the binding tells you to.
> 
> Thanks,
> Conor.


Regards,
Alban

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Alban Browaeys]

Le mardi 26 mars 2024 à 19:46 +0000, Conor Dooley a écrit :
> On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4
> Relay wrote:
> > From: Folker Schwesinger <dev@folker-schwesinger.de>
> > -	if (of_property_read_bool(dev->of_node, "rockchip,enable-
> > strobe-pulldown"))
> > -		rk_phy->enable_strobe_pulldown =
> > PHYCTRL_REN_STRB_ENABLE;
> > +	if (of_property_read_bool(dev->of_node, "rockchip,disable-
> > strobe-pulldown"))
> > +		rk_phy->enable_strobe_pulldown =
> > PHYCTRL_REN_STRB_DISABLE;
> 
> Unfortunately you cannot do this.
> Previously no property at all meant disabled and a property was
> required
> to enable it. With this change the absence of a property means that
> it
> will be enabled.
> An old devicetree is that wanted this to be disabled would have no
> property and will now end up with it enabled. This is an ABI break
> and is
> clearly not backwards compatible, that's a NAK unless it is
> demonstrable
> that noone actually wants to disable it at all.


But the patch that introduced the new default to disable the pulldown
explicitely introduced a regression for at least 4 boards.
It took time to sort out that the default to disable pulldown was the
culprit but still.
Will we carry this new behavor that breaks the default design for
rk3399 because since the regression was introduced new board definition
might have expceted this new behavior.

Could the best option be to revert to énot set a default enable/disable
pulldown" (as before the commit that introduced the regression) and
allow one to force the pulldown via the enable/disable pulldown
property?
I mean the commit that introduced a default value for the pulldown did
not seem to be about fixing anything. But it broke a lot. ANd it was
really really hard to find the description of this commit to understand
that one had to enable pulldown to restore hs400.

In more than 3 years, only one board maintainer noticed that this
property was required to get back HS400  and thanks to a user telling
me that this board was working I found from this board that this
property was "missing" from most board definitions (while it was not
required before).


I am all for not breaking ABI. But what about not reverting a patch
that already broke ABI because this patch introduced a new ABI that we
don't want to break?
I mean shouldn't a new commit with a new ABI that regressed the kernel
be reverted?

Mind fixing the initial regression 8b5c2b45b8f0 "phy: rockchip: set
pulldown for strobe line in dts" does not necessarily mean changing the
default to the opposite value but could also be reverting to not
setting a default.
Though I don't know if there are pros to setting a default.


> If this patch fixes a problem on a board that you have, I would
> suggest
> that you add the property to enable it, as the binding tells you to.
> 
> Thanks,
> Conor.


Regards,
Alban

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Alban Browaeys]

Le mardi 26 mars 2024 à 19:46 +0000, Conor Dooley a écrit :
> On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4
> Relay wrote:
> > From: Folker Schwesinger <dev@folker-schwesinger.de>
> > -	if (of_property_read_bool(dev->of_node, "rockchip,enable-
> > strobe-pulldown"))
> > -		rk_phy->enable_strobe_pulldown =
> > PHYCTRL_REN_STRB_ENABLE;
> > +	if (of_property_read_bool(dev->of_node, "rockchip,disable-
> > strobe-pulldown"))
> > +		rk_phy->enable_strobe_pulldown =
> > PHYCTRL_REN_STRB_DISABLE;
> 
> Unfortunately you cannot do this.
> Previously no property at all meant disabled and a property was
> required
> to enable it. With this change the absence of a property means that
> it
> will be enabled.
> An old devicetree is that wanted this to be disabled would have no
> property and will now end up with it enabled. This is an ABI break
> and is
> clearly not backwards compatible, that's a NAK unless it is
> demonstrable
> that noone actually wants to disable it at all.


But the patch that introduced the new default to disable the pulldown
explicitely introduced a regression for at least 4 boards.
It took time to sort out that the default to disable pulldown was the
culprit but still.
Will we carry this new behavor that breaks the default design for
rk3399 because since the regression was introduced new board definition
might have expceted this new behavior.

Could the best option be to revert to énot set a default enable/disable
pulldown" (as before the commit that introduced the regression) and
allow one to force the pulldown via the enable/disable pulldown
property?
I mean the commit that introduced a default value for the pulldown did
not seem to be about fixing anything. But it broke a lot. ANd it was
really really hard to find the description of this commit to understand
that one had to enable pulldown to restore hs400.

In more than 3 years, only one board maintainer noticed that this
property was required to get back HS400  and thanks to a user telling
me that this board was working I found from this board that this
property was "missing" from most board definitions (while it was not
required before).


I am all for not breaking ABI. But what about not reverting a patch
that already broke ABI because this patch introduced a new ABI that we
don't want to break?
I mean shouldn't a new commit with a new ABI that regressed the kernel
be reverted?

Mind fixing the initial regression 8b5c2b45b8f0 "phy: rockchip: set
pulldown for strobe line in dts" does not necessarily mean changing the
default to the opposite value but could also be reverting to not
setting a default.
Though I don't know if there are pros to setting a default.


> If this patch fixes a problem on a board that you have, I would
> suggest
> that you add the property to enable it, as the binding tells you to.
> 
> Thanks,
> Conor.


Regards,
Alban

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3829 bytes --]

On Thu, Mar 28, 2024 at 06:00:03PM +0100, Alban Browaeys wrote:
> Le mardi 26 mars 2024 à 19:46 +0000, Conor Dooley a écrit :
> > On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4
> > Relay wrote:
> > > From: Folker Schwesinger <dev@folker-schwesinger.de>
> > > -	if (of_property_read_bool(dev->of_node, "rockchip,enable-
> > > strobe-pulldown"))
> > > -		rk_phy->enable_strobe_pulldown =
> > > PHYCTRL_REN_STRB_ENABLE;
> > > +	if (of_property_read_bool(dev->of_node, "rockchip,disable-
> > > strobe-pulldown"))
> > > +		rk_phy->enable_strobe_pulldown =
> > > PHYCTRL_REN_STRB_DISABLE;
> > 
> > Unfortunately you cannot do this.
> > Previously no property at all meant disabled and a property was
> > required
> > to enable it. With this change the absence of a property means that
> > it
> > will be enabled.
> > An old devicetree is that wanted this to be disabled would have no
> > property and will now end up with it enabled. This is an ABI break
> > and is
> > clearly not backwards compatible, that's a NAK unless it is
> > demonstrable
> > that noone actually wants to disable it at all.
> 
> 
> But the patch that introduced the new default to disable the pulldown
> explicitely introduced a regression for at least 4 boards.
> It took time to sort out that the default to disable pulldown was the
> culprit but still.
> Will we carry this new behavor that breaks the default design for
> rk3399 because since the regression was introduced new board definition
> might have expceted this new behavior.
> 
> Could the best option be to revert to énot set a default enable/disable
> pulldown" (as before the commit that introduced the regression) and
> allow one to force the pulldown via the enable/disable pulldown
> property?
> I mean the commit that introduced a default value for the pulldown did
> not seem to be about fixing anything. But it broke a lot. ANd it was
> really really hard to find the description of this commit to understand
> that one had to enable pulldown to restore hs400.
> 
> In more than 3 years, only one board maintainer noticed that this
> property was required to get back HS400  and thanks to a user telling
> me that this board was working I found from this board that this
> property was "missing" from most board definitions (while it was not
> required before).
> 
> 
> I am all for not breaking ABI. But what about not reverting a patch
> that already broke ABI because this patch introduced a new ABI that we
> don't want to break?
> I mean shouldn't a new commit with a new ABI that regressed the kernel
> be reverted?

I think I said it after OP replied to me yesterday, but this is a pretty
shitty situation in that the original default picked for the property
was actually incorrect. Given it's been like this for four years before
anyone noticed, and others probably depend on the current behaviour,
that's hard to justify.

> Mind fixing the initial regression 8b5c2b45b8f0 "phy: rockchip: set
> pulldown for strobe line in dts" does not necessarily mean changing the
> default to the opposite value but could also be reverting to not
> setting a default.

That's also problematic, as the only way to do this is make setting
one of the enabled or disabled properties required, which is also an ABI
break, since you'd then be rejecting probe if one is not present.

> Though I don't know if there are pros to setting a default.

What you probably have to weigh up is the cons of each side. If what you
lose is HS400 mode with what's in the kernel right now but switching to
what's been proposed would entirely break some boards, I know which
I think the lesser of two evils is.

It's probably up to the platform maintainer to weigh in at this point.

Hope that helps?
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 3829 bytes --]

On Thu, Mar 28, 2024 at 06:00:03PM +0100, Alban Browaeys wrote:
> Le mardi 26 mars 2024 à 19:46 +0000, Conor Dooley a écrit :
> > On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4
> > Relay wrote:
> > > From: Folker Schwesinger <dev@folker-schwesinger.de>
> > > -	if (of_property_read_bool(dev->of_node, "rockchip,enable-
> > > strobe-pulldown"))
> > > -		rk_phy->enable_strobe_pulldown =
> > > PHYCTRL_REN_STRB_ENABLE;
> > > +	if (of_property_read_bool(dev->of_node, "rockchip,disable-
> > > strobe-pulldown"))
> > > +		rk_phy->enable_strobe_pulldown =
> > > PHYCTRL_REN_STRB_DISABLE;
> > 
> > Unfortunately you cannot do this.
> > Previously no property at all meant disabled and a property was
> > required
> > to enable it. With this change the absence of a property means that
> > it
> > will be enabled.
> > An old devicetree is that wanted this to be disabled would have no
> > property and will now end up with it enabled. This is an ABI break
> > and is
> > clearly not backwards compatible, that's a NAK unless it is
> > demonstrable
> > that noone actually wants to disable it at all.
> 
> 
> But the patch that introduced the new default to disable the pulldown
> explicitely introduced a regression for at least 4 boards.
> It took time to sort out that the default to disable pulldown was the
> culprit but still.
> Will we carry this new behavor that breaks the default design for
> rk3399 because since the regression was introduced new board definition
> might have expceted this new behavior.
> 
> Could the best option be to revert to énot set a default enable/disable
> pulldown" (as before the commit that introduced the regression) and
> allow one to force the pulldown via the enable/disable pulldown
> property?
> I mean the commit that introduced a default value for the pulldown did
> not seem to be about fixing anything. But it broke a lot. ANd it was
> really really hard to find the description of this commit to understand
> that one had to enable pulldown to restore hs400.
> 
> In more than 3 years, only one board maintainer noticed that this
> property was required to get back HS400  and thanks to a user telling
> me that this board was working I found from this board that this
> property was "missing" from most board definitions (while it was not
> required before).
> 
> 
> I am all for not breaking ABI. But what about not reverting a patch
> that already broke ABI because this patch introduced a new ABI that we
> don't want to break?
> I mean shouldn't a new commit with a new ABI that regressed the kernel
> be reverted?

I think I said it after OP replied to me yesterday, but this is a pretty
shitty situation in that the original default picked for the property
was actually incorrect. Given it's been like this for four years before
anyone noticed, and others probably depend on the current behaviour,
that's hard to justify.

> Mind fixing the initial regression 8b5c2b45b8f0 "phy: rockchip: set
> pulldown for strobe line in dts" does not necessarily mean changing the
> default to the opposite value but could also be reverting to not
> setting a default.

That's also problematic, as the only way to do this is make setting
one of the enabled or disabled properties required, which is also an ABI
break, since you'd then be rejecting probe if one is not present.

> Though I don't know if there are pros to setting a default.

What you probably have to weigh up is the cons of each side. If what you
lose is HS400 mode with what's in the kernel right now but switching to
what's been proposed would entirely break some boards, I know which
I think the lesser of two evils is.

It's probably up to the platform maintainer to weigh in at this point.

Hope that helps?
Conor.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 3829 bytes --]

On Thu, Mar 28, 2024 at 06:00:03PM +0100, Alban Browaeys wrote:
> Le mardi 26 mars 2024 à 19:46 +0000, Conor Dooley a écrit :
> > On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4
> > Relay wrote:
> > > From: Folker Schwesinger <dev@folker-schwesinger.de>
> > > -	if (of_property_read_bool(dev->of_node, "rockchip,enable-
> > > strobe-pulldown"))
> > > -		rk_phy->enable_strobe_pulldown =
> > > PHYCTRL_REN_STRB_ENABLE;
> > > +	if (of_property_read_bool(dev->of_node, "rockchip,disable-
> > > strobe-pulldown"))
> > > +		rk_phy->enable_strobe_pulldown =
> > > PHYCTRL_REN_STRB_DISABLE;
> > 
> > Unfortunately you cannot do this.
> > Previously no property at all meant disabled and a property was
> > required
> > to enable it. With this change the absence of a property means that
> > it
> > will be enabled.
> > An old devicetree is that wanted this to be disabled would have no
> > property and will now end up with it enabled. This is an ABI break
> > and is
> > clearly not backwards compatible, that's a NAK unless it is
> > demonstrable
> > that noone actually wants to disable it at all.
> 
> 
> But the patch that introduced the new default to disable the pulldown
> explicitely introduced a regression for at least 4 boards.
> It took time to sort out that the default to disable pulldown was the
> culprit but still.
> Will we carry this new behavor that breaks the default design for
> rk3399 because since the regression was introduced new board definition
> might have expceted this new behavior.
> 
> Could the best option be to revert to énot set a default enable/disable
> pulldown" (as before the commit that introduced the regression) and
> allow one to force the pulldown via the enable/disable pulldown
> property?
> I mean the commit that introduced a default value for the pulldown did
> not seem to be about fixing anything. But it broke a lot. ANd it was
> really really hard to find the description of this commit to understand
> that one had to enable pulldown to restore hs400.
> 
> In more than 3 years, only one board maintainer noticed that this
> property was required to get back HS400  and thanks to a user telling
> me that this board was working I found from this board that this
> property was "missing" from most board definitions (while it was not
> required before).
> 
> 
> I am all for not breaking ABI. But what about not reverting a patch
> that already broke ABI because this patch introduced a new ABI that we
> don't want to break?
> I mean shouldn't a new commit with a new ABI that regressed the kernel
> be reverted?

I think I said it after OP replied to me yesterday, but this is a pretty
shitty situation in that the original default picked for the property
was actually incorrect. Given it's been like this for four years before
anyone noticed, and others probably depend on the current behaviour,
that's hard to justify.

> Mind fixing the initial regression 8b5c2b45b8f0 "phy: rockchip: set
> pulldown for strobe line in dts" does not necessarily mean changing the
> default to the opposite value but could also be reverting to not
> setting a default.

That's also problematic, as the only way to do this is make setting
one of the enabled or disabled properties required, which is also an ABI
break, since you'd then be rejecting probe if one is not present.

> Though I don't know if there are pros to setting a default.

What you probably have to weigh up is the cons of each side. If what you
lose is HS400 mode with what's in the kernel right now but switching to
what's been proposed would entirely break some boards, I know which
I think the lesser of two evils is.

It's probably up to the platform maintainer to weigh in at this point.

Hope that helps?
Conor.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 3829 bytes --]

On Thu, Mar 28, 2024 at 06:00:03PM +0100, Alban Browaeys wrote:
> Le mardi 26 mars 2024 à 19:46 +0000, Conor Dooley a écrit :
> > On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4
> > Relay wrote:
> > > From: Folker Schwesinger <dev@folker-schwesinger.de>
> > > -	if (of_property_read_bool(dev->of_node, "rockchip,enable-
> > > strobe-pulldown"))
> > > -		rk_phy->enable_strobe_pulldown =
> > > PHYCTRL_REN_STRB_ENABLE;
> > > +	if (of_property_read_bool(dev->of_node, "rockchip,disable-
> > > strobe-pulldown"))
> > > +		rk_phy->enable_strobe_pulldown =
> > > PHYCTRL_REN_STRB_DISABLE;
> > 
> > Unfortunately you cannot do this.
> > Previously no property at all meant disabled and a property was
> > required
> > to enable it. With this change the absence of a property means that
> > it
> > will be enabled.
> > An old devicetree is that wanted this to be disabled would have no
> > property and will now end up with it enabled. This is an ABI break
> > and is
> > clearly not backwards compatible, that's a NAK unless it is
> > demonstrable
> > that noone actually wants to disable it at all.
> 
> 
> But the patch that introduced the new default to disable the pulldown
> explicitely introduced a regression for at least 4 boards.
> It took time to sort out that the default to disable pulldown was the
> culprit but still.
> Will we carry this new behavor that breaks the default design for
> rk3399 because since the regression was introduced new board definition
> might have expceted this new behavior.
> 
> Could the best option be to revert to énot set a default enable/disable
> pulldown" (as before the commit that introduced the regression) and
> allow one to force the pulldown via the enable/disable pulldown
> property?
> I mean the commit that introduced a default value for the pulldown did
> not seem to be about fixing anything. But it broke a lot. ANd it was
> really really hard to find the description of this commit to understand
> that one had to enable pulldown to restore hs400.
> 
> In more than 3 years, only one board maintainer noticed that this
> property was required to get back HS400  and thanks to a user telling
> me that this board was working I found from this board that this
> property was "missing" from most board definitions (while it was not
> required before).
> 
> 
> I am all for not breaking ABI. But what about not reverting a patch
> that already broke ABI because this patch introduced a new ABI that we
> don't want to break?
> I mean shouldn't a new commit with a new ABI that regressed the kernel
> be reverted?

I think I said it after OP replied to me yesterday, but this is a pretty
shitty situation in that the original default picked for the property
was actually incorrect. Given it's been like this for four years before
anyone noticed, and others probably depend on the current behaviour,
that's hard to justify.

> Mind fixing the initial regression 8b5c2b45b8f0 "phy: rockchip: set
> pulldown for strobe line in dts" does not necessarily mean changing the
> default to the opposite value but could also be reverting to not
> setting a default.

That's also problematic, as the only way to do this is make setting
one of the enabled or disabled properties required, which is also an ABI
break, since you'd then be rejecting probe if one is not present.

> Though I don't know if there are pros to setting a default.

What you probably have to weigh up is the cons of each side. If what you
lose is HS400 mode with what's in the kernel right now but switching to
what's been proposed would entirely break some boards, I know which
I think the lesser of two evils is.

It's probably up to the platform maintainer to weigh in at this point.

Hope that helps?
Conor.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Dragan Simic]

Hello Alban,

On 2024-03-28 18:00, Alban Browaeys wrote:
> Le mardi 26 mars 2024 à 19:46 +0000, Conor Dooley a écrit :
>> On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4
>> Relay wrote:
>> > From: Folker Schwesinger <dev@folker-schwesinger.de>
>> > -	if (of_property_read_bool(dev->of_node, "rockchip,enable-
>> > strobe-pulldown"))
>> > -		rk_phy->enable_strobe_pulldown =
>> > PHYCTRL_REN_STRB_ENABLE;
>> > +	if (of_property_read_bool(dev->of_node, "rockchip,disable-
>> > strobe-pulldown"))
>> > +		rk_phy->enable_strobe_pulldown =
>> > PHYCTRL_REN_STRB_DISABLE;
>> 
>> Unfortunately you cannot do this.
>> Previously no property at all meant disabled and a property was
>> required
>> to enable it. With this change the absence of a property means that
>> it
>> will be enabled.
>> An old devicetree is that wanted this to be disabled would have no
>> property and will now end up with it enabled. This is an ABI break
>> and is
>> clearly not backwards compatible, that's a NAK unless it is
>> demonstrable
>> that noone actually wants to disable it at all.
> 
> But the patch that introduced the new default to disable the pulldown
> explicitely introduced a regression for at least 4 boards.
> It took time to sort out that the default to disable pulldown was the
> culprit but still.
> Will we carry this new behavor that breaks the default design for
> rk3399 because since the regression was introduced new board definition
> might have expceted this new behavior.
> 
> Could the best option be to revert to énot set a default enable/disable
> pulldown" (as before the commit that introduced the regression) and
> allow one to force the pulldown via the enable/disable pulldown
> property?
> I mean the commit that introduced a default value for the pulldown did
> not seem to be about fixing anything. But it broke a lot. ANd it was
> really really hard to find the description of this commit to understand
> that one had to enable pulldown to restore hs400.

Quite frankly, I think it's better to leave the default as-is, and
to fix the dts files for the boards that have been (or will be) tested
to work as expected and reliably in the HS400 mode.  Perhaps this is
also a good opportunity to revisit the reliability of the HS400 mode
on various boards.

In other words, it could be that some boards now rely on the pull-down
being disabled by default, so enabling it by default might actually
break such boards.  I know, the troublesome commit that disabled the
pull-down caused breakage, but fixing that might actually cause more
breakage at this point.

> In more than 3 years, only one board maintainer noticed that this
> property was required to get back HS400  and thanks to a user telling
> me that this board was working I found from this board that this
> property was "missing" from most board definitions (while it was not
> required before).

A couple of years ago I've also spent some time debugging HS400 not
working on a Rock 4, but ended up with limiting the speed to HS200 as
a workaround, so I agree about the whole thing being a mess.

> I am all for not breaking ABI. But what about not reverting a patch
> that already broke ABI because this patch introduced a new ABI that we
> don't want to break?
> I mean shouldn't a new commit with a new ABI that regressed the kernel
> be reverted?
> 
> Mind fixing the initial regression 8b5c2b45b8f0 "phy: rockchip: set
> pulldown for strobe line in dts" does not necessarily mean changing the
> default to the opposite value but could also be reverting to not
> setting a default.
> Though I don't know if there are pros to setting a default.
> 
> 
>> If this patch fixes a problem on a board that you have, I would
>> suggest
>> that you add the property to enable it, as the binding tells you to.
>> 
>> Thanks,
>> Conor.
> 
> 
> Regards,
> Alban
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Dragan Simic]

Hello Alban,

On 2024-03-28 18:00, Alban Browaeys wrote:
> Le mardi 26 mars 2024 à 19:46 +0000, Conor Dooley a écrit :
>> On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4
>> Relay wrote:
>> > From: Folker Schwesinger <dev@folker-schwesinger.de>
>> > -	if (of_property_read_bool(dev->of_node, "rockchip,enable-
>> > strobe-pulldown"))
>> > -		rk_phy->enable_strobe_pulldown =
>> > PHYCTRL_REN_STRB_ENABLE;
>> > +	if (of_property_read_bool(dev->of_node, "rockchip,disable-
>> > strobe-pulldown"))
>> > +		rk_phy->enable_strobe_pulldown =
>> > PHYCTRL_REN_STRB_DISABLE;
>> 
>> Unfortunately you cannot do this.
>> Previously no property at all meant disabled and a property was
>> required
>> to enable it. With this change the absence of a property means that
>> it
>> will be enabled.
>> An old devicetree is that wanted this to be disabled would have no
>> property and will now end up with it enabled. This is an ABI break
>> and is
>> clearly not backwards compatible, that's a NAK unless it is
>> demonstrable
>> that noone actually wants to disable it at all.
> 
> But the patch that introduced the new default to disable the pulldown
> explicitely introduced a regression for at least 4 boards.
> It took time to sort out that the default to disable pulldown was the
> culprit but still.
> Will we carry this new behavor that breaks the default design for
> rk3399 because since the regression was introduced new board definition
> might have expceted this new behavior.
> 
> Could the best option be to revert to énot set a default enable/disable
> pulldown" (as before the commit that introduced the regression) and
> allow one to force the pulldown via the enable/disable pulldown
> property?
> I mean the commit that introduced a default value for the pulldown did
> not seem to be about fixing anything. But it broke a lot. ANd it was
> really really hard to find the description of this commit to understand
> that one had to enable pulldown to restore hs400.

Quite frankly, I think it's better to leave the default as-is, and
to fix the dts files for the boards that have been (or will be) tested
to work as expected and reliably in the HS400 mode.  Perhaps this is
also a good opportunity to revisit the reliability of the HS400 mode
on various boards.

In other words, it could be that some boards now rely on the pull-down
being disabled by default, so enabling it by default might actually
break such boards.  I know, the troublesome commit that disabled the
pull-down caused breakage, but fixing that might actually cause more
breakage at this point.

> In more than 3 years, only one board maintainer noticed that this
> property was required to get back HS400  and thanks to a user telling
> me that this board was working I found from this board that this
> property was "missing" from most board definitions (while it was not
> required before).

A couple of years ago I've also spent some time debugging HS400 not
working on a Rock 4, but ended up with limiting the speed to HS200 as
a workaround, so I agree about the whole thing being a mess.

> I am all for not breaking ABI. But what about not reverting a patch
> that already broke ABI because this patch introduced a new ABI that we
> don't want to break?
> I mean shouldn't a new commit with a new ABI that regressed the kernel
> be reverted?
> 
> Mind fixing the initial regression 8b5c2b45b8f0 "phy: rockchip: set
> pulldown for strobe line in dts" does not necessarily mean changing the
> default to the opposite value but could also be reverting to not
> setting a default.
> Though I don't know if there are pros to setting a default.
> 
> 
>> If this patch fixes a problem on a board that you have, I would
>> suggest
>> that you add the property to enable it, as the binding tells you to.
>> 
>> Thanks,
>> Conor.
> 
> 
> Regards,
> Alban
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Dragan Simic]

Hello Alban,

On 2024-03-28 18:00, Alban Browaeys wrote:
> Le mardi 26 mars 2024 à 19:46 +0000, Conor Dooley a écrit :
>> On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4
>> Relay wrote:
>> > From: Folker Schwesinger <dev@folker-schwesinger.de>
>> > -	if (of_property_read_bool(dev->of_node, "rockchip,enable-
>> > strobe-pulldown"))
>> > -		rk_phy->enable_strobe_pulldown =
>> > PHYCTRL_REN_STRB_ENABLE;
>> > +	if (of_property_read_bool(dev->of_node, "rockchip,disable-
>> > strobe-pulldown"))
>> > +		rk_phy->enable_strobe_pulldown =
>> > PHYCTRL_REN_STRB_DISABLE;
>> 
>> Unfortunately you cannot do this.
>> Previously no property at all meant disabled and a property was
>> required
>> to enable it. With this change the absence of a property means that
>> it
>> will be enabled.
>> An old devicetree is that wanted this to be disabled would have no
>> property and will now end up with it enabled. This is an ABI break
>> and is
>> clearly not backwards compatible, that's a NAK unless it is
>> demonstrable
>> that noone actually wants to disable it at all.
> 
> But the patch that introduced the new default to disable the pulldown
> explicitely introduced a regression for at least 4 boards.
> It took time to sort out that the default to disable pulldown was the
> culprit but still.
> Will we carry this new behavor that breaks the default design for
> rk3399 because since the regression was introduced new board definition
> might have expceted this new behavior.
> 
> Could the best option be to revert to énot set a default enable/disable
> pulldown" (as before the commit that introduced the regression) and
> allow one to force the pulldown via the enable/disable pulldown
> property?
> I mean the commit that introduced a default value for the pulldown did
> not seem to be about fixing anything. But it broke a lot. ANd it was
> really really hard to find the description of this commit to understand
> that one had to enable pulldown to restore hs400.

Quite frankly, I think it's better to leave the default as-is, and
to fix the dts files for the boards that have been (or will be) tested
to work as expected and reliably in the HS400 mode.  Perhaps this is
also a good opportunity to revisit the reliability of the HS400 mode
on various boards.

In other words, it could be that some boards now rely on the pull-down
being disabled by default, so enabling it by default might actually
break such boards.  I know, the troublesome commit that disabled the
pull-down caused breakage, but fixing that might actually cause more
breakage at this point.

> In more than 3 years, only one board maintainer noticed that this
> property was required to get back HS400  and thanks to a user telling
> me that this board was working I found from this board that this
> property was "missing" from most board definitions (while it was not
> required before).

A couple of years ago I've also spent some time debugging HS400 not
working on a Rock 4, but ended up with limiting the speed to HS200 as
a workaround, so I agree about the whole thing being a mess.

> I am all for not breaking ABI. But what about not reverting a patch
> that already broke ABI because this patch introduced a new ABI that we
> don't want to break?
> I mean shouldn't a new commit with a new ABI that regressed the kernel
> be reverted?
> 
> Mind fixing the initial regression 8b5c2b45b8f0 "phy: rockchip: set
> pulldown for strobe line in dts" does not necessarily mean changing the
> default to the opposite value but could also be reverting to not
> setting a default.
> Though I don't know if there are pros to setting a default.
> 
> 
>> If this patch fixes a problem on a board that you have, I would
>> suggest
>> that you add the property to enable it, as the binding tells you to.
>> 
>> Thanks,
>> Conor.
> 
> 
> Regards,
> Alban
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Dragan Simic]

Hello Alban,

On 2024-03-28 18:00, Alban Browaeys wrote:
> Le mardi 26 mars 2024 à 19:46 +0000, Conor Dooley a écrit :
>> On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via B4
>> Relay wrote:
>> > From: Folker Schwesinger <dev@folker-schwesinger.de>
>> > -	if (of_property_read_bool(dev->of_node, "rockchip,enable-
>> > strobe-pulldown"))
>> > -		rk_phy->enable_strobe_pulldown =
>> > PHYCTRL_REN_STRB_ENABLE;
>> > +	if (of_property_read_bool(dev->of_node, "rockchip,disable-
>> > strobe-pulldown"))
>> > +		rk_phy->enable_strobe_pulldown =
>> > PHYCTRL_REN_STRB_DISABLE;
>> 
>> Unfortunately you cannot do this.
>> Previously no property at all meant disabled and a property was
>> required
>> to enable it. With this change the absence of a property means that
>> it
>> will be enabled.
>> An old devicetree is that wanted this to be disabled would have no
>> property and will now end up with it enabled. This is an ABI break
>> and is
>> clearly not backwards compatible, that's a NAK unless it is
>> demonstrable
>> that noone actually wants to disable it at all.
> 
> But the patch that introduced the new default to disable the pulldown
> explicitely introduced a regression for at least 4 boards.
> It took time to sort out that the default to disable pulldown was the
> culprit but still.
> Will we carry this new behavor that breaks the default design for
> rk3399 because since the regression was introduced new board definition
> might have expceted this new behavior.
> 
> Could the best option be to revert to énot set a default enable/disable
> pulldown" (as before the commit that introduced the regression) and
> allow one to force the pulldown via the enable/disable pulldown
> property?
> I mean the commit that introduced a default value for the pulldown did
> not seem to be about fixing anything. But it broke a lot. ANd it was
> really really hard to find the description of this commit to understand
> that one had to enable pulldown to restore hs400.

Quite frankly, I think it's better to leave the default as-is, and
to fix the dts files for the boards that have been (or will be) tested
to work as expected and reliably in the HS400 mode.  Perhaps this is
also a good opportunity to revisit the reliability of the HS400 mode
on various boards.

In other words, it could be that some boards now rely on the pull-down
being disabled by default, so enabling it by default might actually
break such boards.  I know, the troublesome commit that disabled the
pull-down caused breakage, but fixing that might actually cause more
breakage at this point.

> In more than 3 years, only one board maintainer noticed that this
> property was required to get back HS400  and thanks to a user telling
> me that this board was working I found from this board that this
> property was "missing" from most board definitions (while it was not
> required before).

A couple of years ago I've also spent some time debugging HS400 not
working on a Rock 4, but ended up with limiting the speed to HS200 as
a workaround, so I agree about the whole thing being a mess.

> I am all for not breaking ABI. But what about not reverting a patch
> that already broke ABI because this patch introduced a new ABI that we
> don't want to break?
> I mean shouldn't a new commit with a new ABI that regressed the kernel
> be reverted?
> 
> Mind fixing the initial regression 8b5c2b45b8f0 "phy: rockchip: set
> pulldown for strobe line in dts" does not necessarily mean changing the
> default to the opposite value but could also be reverting to not
> setting a default.
> Though I don't know if there are pros to setting a default.
> 
> 
>> If this patch fixes a problem on a board that you have, I would
>> suggest
>> that you add the property to enable it, as the binding tells you to.
>> 
>> Thanks,
>> Conor.
> 
> 
> Regards,
> Alban
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Alban Browaeys]

Le jeudi 28 mars 2024 à 18:01 +0000, Conor Dooley a écrit :
> On Thu, Mar 28, 2024 at 06:00:03PM +0100, Alban Browaeys wrote:
> > Le mardi 26 mars 2024 à 19:46 +0000, Conor Dooley a écrit :
> > > On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via
> > > B4
> > > Relay wrote:
> > > > From: Folker Schwesinger <dev@folker-schwesinger.de>
> > > > -	if (of_property_read_bool(dev->of_node,
> > > > "rockchip,enable-
> > > > strobe-pulldown"))
> > > > -		rk_phy->enable_strobe_pulldown =
> > > > PHYCTRL_REN_STRB_ENABLE;
> > > > +	if (of_property_read_bool(dev->of_node,
> > > > "rockchip,disable-
> > > > strobe-pulldown"))
> > > > +		rk_phy->enable_strobe_pulldown =
> > > > PHYCTRL_REN_STRB_DISABLE;
> > > 
> > > Unfortunately you cannot do this.
> > > Previously no property at all meant disabled and a property was
> > > required
> > > to enable it. With this change the absence of a property means
> > > that
> > > it
> > > will be enabled.
> > > An old devicetree is that wanted this to be disabled would have
> > > no
> > > property and will now end up with it enabled. This is an ABI
> > > break
> > > and is
> > > clearly not backwards compatible, that's a NAK unless it is
> > > demonstrable
> > > that noone actually wants to disable it at all.
> > 
> > 
> > But the patch that introduced the new default to disable the
> > pulldown
> > explicitely introduced a regression for at least 4 boards.
> > It took time to sort out that the default to disable pulldown was
> > the
> > culprit but still.
> > Will we carry this new behavor that breaks the default design for
> > rk3399 because since the regression was introduced new board
> > definition
> > might have expceted this new behavior.
> > 
> > Could the best option be to revert to énot set a default
> > enable/disable
> > pulldown" (as before the commit that introduced the regression) and
> > allow one to force the pulldown via the enable/disable pulldown
> > property?
> > I mean the commit that introduced a default value for the pulldown
> > did
> > not seem to be about fixing anything. But it broke a lot. ANd it
> > was
> > really really hard to find the description of this commit to
> > understand
> > that one had to enable pulldown to restore hs400.
> > 
> > In more than 3 years, only one board maintainer noticed that this
> > property was required to get back HS400  and thanks to a user
> > telling
> > me that this board was working I found from this board that this
> > property was "missing" from most board definitions (while it was
> > not
> > required before).
> > 
> > 
> > I am all for not breaking ABI. But what about not reverting a patch
> > that already broke ABI because this patch introduced a new ABI that
> > we
> > don't want to break?
> > I mean shouldn't a new commit with a new ABI that regressed the
> > kernel
> > be reverted?
> 
> I think I said it after OP replied to me yesterday, but this is a
> pretty
> shitty situation in that the original default picked for the property
> was actually incorrect. Given it's been like this for four years
> before
> anyone noticed, and others probably depend on the current behaviour,
> that's hard to justify.
> 

A lot of people noticed fast that HS400 was broken in the 5.10 branch
but due to another commit (more later, ie double regulator init that
messed up emmc) this second breakage was not detected. But mostly
downstream. And most if not all rk3399 boards in Armbian had HS400
disabled.


It took 3 years to detect that HS400 was broken on a few boards like
Rock Pi4 in the upstream kernel. Any might still be broken.
I would not count on the fact that keeping the current behavior equals
no more broken boards.

From the previous exchanges the boards that requires the pulldown to be
disabled seems well known.

Though I am fine with adding a property to set enable pulldown to any
board definition file where that is required.

Only I do not believe keeping the statu quo equal everything works
because it has been 3 years.
In fact this commit reached the downstream kernels way later. Any
stayed with the 5.10 branch for years.

But on the other side the disable pulldown by default is alraedy in
stable/linux-rolling-lts .



> > Mind fixing the initial regression 8b5c2b45b8f0 "phy: rockchip: set
> > pulldown for strobe line in dts" does not necessarily mean changing
> > the
> > default to the opposite value but could also be reverting to not
> > setting a default.
> 
> That's also problematic, as the only way to do this is make setting
> one of the enabled or disabled properties required, which is also an
> ABI
> break, since you'd then be rejecting probe if one is not present.


I don't understand.
How reverting to not set either pulldown enabled or disabled by default
force all board to set either enabled or disabled.
I was telling about making the pulldown set by kernel optional be it
enabled or disabled to revert to the previous behavior. 

I mean before the patch to set a default pulldown value (to disabled)
there were no forced value.



> > Though I don't know if there are pros to setting a default.
> 
> What you probably have to weigh up is the cons of each side. If what
> you
> lose is HS400 mode with what's in the kernel right now but switching
> to
> what's been proposed would entirely break some boards, I know which
> I think the lesser of two evils is.

More boards (even if not the most wide spread it seems) are broken by
the current behavior.

I agree that only HS400 is broken by keeping the status quo. But as far
as I understand only HS400 will be broken either way.
Be that by keeping the current disable pulldown which break the boards
based on the rockchip default design or the boards that are non-
standard or have a broken design.
Both case this lead to data corruption on boot to eMMC.

The only pro of keeping the current value the default is that most
board broken by the new default introduced in 2020 "might" already be
fixed (but that is just a guess).



> It's probably up to the platform maintainer to weigh in at this
> point.

I am not knowledged into the delegation scope. You mean that from now
on it is up to the rockchip maintainer?
I am fine with it either way.

I just wanted to point out that maybe we don't have to set a pulldown
value after all. And that then all boards will be fine as before
setting the pulldown explicitly was introduced.


In fact I am more eager to get this fixed be it by adding a enable-
pulldown property to the board definitions, than to change the current
behavior.
Just wanted to sort out if that was not the wrong way to fix this
issue. (ie if adding a setting on most boards was wrong).


> Hope that helps?
> Conor.


During more than 2 years, I tried various patches and discussed on
forums about the HS400 breakage. I had bisected the regulator init
issue in the 5.10 branch. Sadly it took so much time for this issue to
be understood that when the force pulldown to disable commit was
introduced downstream before the first issue go fixed.
This only made the matter worse because when one fixed the double
regulator init issue HS400 was still broken, this time because the
pulldown was forced to disable. But nobody noticed this commit that
forced a default pulldown state (that was older than the regulator
commit from 5.13 backported to the 5.10 stable branch commit, but that
reached downstream later due to not having been backported to 5.10 from
5.11).
Otherwise we would have emailed immeditaly.
Bisecting was only able to catch the first breakage (as it was only
fixed after the second breakage was introduced).

Maybe the problem is that me and others did not complained to the
kernel upstream ML because we were using heavily patched downstream
kernels (like most if not all downstream ARM kernels). So sadly, the
forums from back then are filled with complaints but nothing seemed to
have reached the Linux ML.



About the regulator double init, stable downstream branches were hit by
a bug in the 5.10 stable branch in May 2021 before they switched to
5.11 were this default pulldown was introduced. Thus they could not
detect that this pulldown broke HS400 because HS400 was already broken
by a double regulator init, backported in 5.10 from 5.13:
"
commit 06653ebc0ad2e0b7d799cd71a5c2933ed2fb7a66
Author: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Date:   Thu May 20 01:12:23 2021 +0300

	regulator: core: resolve supply for boot-on/always-on
regulators
   
	commit 98e48cd9283dbac0e1445ee780889f10b3d1db6a upstream.
   
	For the boot-on/always-on regulators the
set_machine_constrainst() is
	called before resolving rdev->supply. Thus the code would try
to enable
	rdev before enabling supplying regulator. Enforce resolving
supply
	regulator before enabling rdev.
   
	Fixes: aea6cb99703e ("regulator: resolve supply after creating
regulator")
	Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
	Link:
https://lore.kernel.org/r/20210519221224.2868496-1-dmitry.baryshkov@linaro.org
	Signed-off-by: Mark Brown <broonie@kernel.org>
	Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
"
and which to my knowledge was only fixed in v6.1-rc1 "
commit 8a866d527ac0441c0eb14a991fa11358b476b11d
Author: Christian Kohlschütter <christian@kohlschutter.com>
Date:   Thu Aug 18 12:46:47 2022 +0000

    regulator: core: Resolve supply name earlier to prevent double-init
    
    Previously, an unresolved regulator supply reference upon calling
    regulator_register on an always-on or boot-on regulator caused
    set_machine_constraints to be called twice.
    
    This in turn may initialize the regulator twice, leading to voltage
    glitches that are timing-dependent. A simple, unrelated
configuration
    change may be enough to hide this problem, only to be surfaced by
    chance.
    
    One such example is the SD-Card voltage regulator in a NanoPI R4S
that
    would not initialize reliably unless the registration flow was just
    complex enough to allow the regulator to properly reset between
calls.
    
    Fix this by re-arranging regulator_register, trying resolve the
    regulator's supply early enough that set_machine_constraints does
not
    need to be called twice.
    
    Signed-off-by: Christian Kohlschütter <christian@kohlschutter.com>
    Link:
https://lore.kernel.org/r/20220818124646.6005-1-christian@kohlschutter.com
    Signed-off-by: Mark Brown <broonie@kernel.org>

"

So most boards were already broken when the commit to force a pulldown
value was introduced.




Regards
Alban

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Alban Browaeys]

Le jeudi 28 mars 2024 à 18:01 +0000, Conor Dooley a écrit :
> On Thu, Mar 28, 2024 at 06:00:03PM +0100, Alban Browaeys wrote:
> > Le mardi 26 mars 2024 à 19:46 +0000, Conor Dooley a écrit :
> > > On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via
> > > B4
> > > Relay wrote:
> > > > From: Folker Schwesinger <dev@folker-schwesinger.de>
> > > > -	if (of_property_read_bool(dev->of_node,
> > > > "rockchip,enable-
> > > > strobe-pulldown"))
> > > > -		rk_phy->enable_strobe_pulldown =
> > > > PHYCTRL_REN_STRB_ENABLE;
> > > > +	if (of_property_read_bool(dev->of_node,
> > > > "rockchip,disable-
> > > > strobe-pulldown"))
> > > > +		rk_phy->enable_strobe_pulldown =
> > > > PHYCTRL_REN_STRB_DISABLE;
> > > 
> > > Unfortunately you cannot do this.
> > > Previously no property at all meant disabled and a property was
> > > required
> > > to enable it. With this change the absence of a property means
> > > that
> > > it
> > > will be enabled.
> > > An old devicetree is that wanted this to be disabled would have
> > > no
> > > property and will now end up with it enabled. This is an ABI
> > > break
> > > and is
> > > clearly not backwards compatible, that's a NAK unless it is
> > > demonstrable
> > > that noone actually wants to disable it at all.
> > 
> > 
> > But the patch that introduced the new default to disable the
> > pulldown
> > explicitely introduced a regression for at least 4 boards.
> > It took time to sort out that the default to disable pulldown was
> > the
> > culprit but still.
> > Will we carry this new behavor that breaks the default design for
> > rk3399 because since the regression was introduced new board
> > definition
> > might have expceted this new behavior.
> > 
> > Could the best option be to revert to énot set a default
> > enable/disable
> > pulldown" (as before the commit that introduced the regression) and
> > allow one to force the pulldown via the enable/disable pulldown
> > property?
> > I mean the commit that introduced a default value for the pulldown
> > did
> > not seem to be about fixing anything. But it broke a lot. ANd it
> > was
> > really really hard to find the description of this commit to
> > understand
> > that one had to enable pulldown to restore hs400.
> > 
> > In more than 3 years, only one board maintainer noticed that this
> > property was required to get back HS400  and thanks to a user
> > telling
> > me that this board was working I found from this board that this
> > property was "missing" from most board definitions (while it was
> > not
> > required before).
> > 
> > 
> > I am all for not breaking ABI. But what about not reverting a patch
> > that already broke ABI because this patch introduced a new ABI that
> > we
> > don't want to break?
> > I mean shouldn't a new commit with a new ABI that regressed the
> > kernel
> > be reverted?
> 
> I think I said it after OP replied to me yesterday, but this is a
> pretty
> shitty situation in that the original default picked for the property
> was actually incorrect. Given it's been like this for four years
> before
> anyone noticed, and others probably depend on the current behaviour,
> that's hard to justify.
> 

A lot of people noticed fast that HS400 was broken in the 5.10 branch
but due to another commit (more later, ie double regulator init that
messed up emmc) this second breakage was not detected. But mostly
downstream. And most if not all rk3399 boards in Armbian had HS400
disabled.


It took 3 years to detect that HS400 was broken on a few boards like
Rock Pi4 in the upstream kernel. Any might still be broken.
I would not count on the fact that keeping the current behavior equals
no more broken boards.

From the previous exchanges the boards that requires the pulldown to be
disabled seems well known.

Though I am fine with adding a property to set enable pulldown to any
board definition file where that is required.

Only I do not believe keeping the statu quo equal everything works
because it has been 3 years.
In fact this commit reached the downstream kernels way later. Any
stayed with the 5.10 branch for years.

But on the other side the disable pulldown by default is alraedy in
stable/linux-rolling-lts .



> > Mind fixing the initial regression 8b5c2b45b8f0 "phy: rockchip: set
> > pulldown for strobe line in dts" does not necessarily mean changing
> > the
> > default to the opposite value but could also be reverting to not
> > setting a default.
> 
> That's also problematic, as the only way to do this is make setting
> one of the enabled or disabled properties required, which is also an
> ABI
> break, since you'd then be rejecting probe if one is not present.


I don't understand.
How reverting to not set either pulldown enabled or disabled by default
force all board to set either enabled or disabled.
I was telling about making the pulldown set by kernel optional be it
enabled or disabled to revert to the previous behavior. 

I mean before the patch to set a default pulldown value (to disabled)
there were no forced value.



> > Though I don't know if there are pros to setting a default.
> 
> What you probably have to weigh up is the cons of each side. If what
> you
> lose is HS400 mode with what's in the kernel right now but switching
> to
> what's been proposed would entirely break some boards, I know which
> I think the lesser of two evils is.

More boards (even if not the most wide spread it seems) are broken by
the current behavior.

I agree that only HS400 is broken by keeping the status quo. But as far
as I understand only HS400 will be broken either way.
Be that by keeping the current disable pulldown which break the boards
based on the rockchip default design or the boards that are non-
standard or have a broken design.
Both case this lead to data corruption on boot to eMMC.

The only pro of keeping the current value the default is that most
board broken by the new default introduced in 2020 "might" already be
fixed (but that is just a guess).



> It's probably up to the platform maintainer to weigh in at this
> point.

I am not knowledged into the delegation scope. You mean that from now
on it is up to the rockchip maintainer?
I am fine with it either way.

I just wanted to point out that maybe we don't have to set a pulldown
value after all. And that then all boards will be fine as before
setting the pulldown explicitly was introduced.


In fact I am more eager to get this fixed be it by adding a enable-
pulldown property to the board definitions, than to change the current
behavior.
Just wanted to sort out if that was not the wrong way to fix this
issue. (ie if adding a setting on most boards was wrong).


> Hope that helps?
> Conor.


During more than 2 years, I tried various patches and discussed on
forums about the HS400 breakage. I had bisected the regulator init
issue in the 5.10 branch. Sadly it took so much time for this issue to
be understood that when the force pulldown to disable commit was
introduced downstream before the first issue go fixed.
This only made the matter worse because when one fixed the double
regulator init issue HS400 was still broken, this time because the
pulldown was forced to disable. But nobody noticed this commit that
forced a default pulldown state (that was older than the regulator
commit from 5.13 backported to the 5.10 stable branch commit, but that
reached downstream later due to not having been backported to 5.10 from
5.11).
Otherwise we would have emailed immeditaly.
Bisecting was only able to catch the first breakage (as it was only
fixed after the second breakage was introduced).

Maybe the problem is that me and others did not complained to the
kernel upstream ML because we were using heavily patched downstream
kernels (like most if not all downstream ARM kernels). So sadly, the
forums from back then are filled with complaints but nothing seemed to
have reached the Linux ML.



About the regulator double init, stable downstream branches were hit by
a bug in the 5.10 stable branch in May 2021 before they switched to
5.11 were this default pulldown was introduced. Thus they could not
detect that this pulldown broke HS400 because HS400 was already broken
by a double regulator init, backported in 5.10 from 5.13:
"
commit 06653ebc0ad2e0b7d799cd71a5c2933ed2fb7a66
Author: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Date:   Thu May 20 01:12:23 2021 +0300

	regulator: core: resolve supply for boot-on/always-on
regulators
   
	commit 98e48cd9283dbac0e1445ee780889f10b3d1db6a upstream.
   
	For the boot-on/always-on regulators the
set_machine_constrainst() is
	called before resolving rdev->supply. Thus the code would try
to enable
	rdev before enabling supplying regulator. Enforce resolving
supply
	regulator before enabling rdev.
   
	Fixes: aea6cb99703e ("regulator: resolve supply after creating
regulator")
	Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
	Link:
https://lore.kernel.org/r/20210519221224.2868496-1-dmitry.baryshkov@linaro.org
	Signed-off-by: Mark Brown <broonie@kernel.org>
	Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
"
and which to my knowledge was only fixed in v6.1-rc1 "
commit 8a866d527ac0441c0eb14a991fa11358b476b11d
Author: Christian Kohlschütter <christian@kohlschutter.com>
Date:   Thu Aug 18 12:46:47 2022 +0000

    regulator: core: Resolve supply name earlier to prevent double-init
    
    Previously, an unresolved regulator supply reference upon calling
    regulator_register on an always-on or boot-on regulator caused
    set_machine_constraints to be called twice.
    
    This in turn may initialize the regulator twice, leading to voltage
    glitches that are timing-dependent. A simple, unrelated
configuration
    change may be enough to hide this problem, only to be surfaced by
    chance.
    
    One such example is the SD-Card voltage regulator in a NanoPI R4S
that
    would not initialize reliably unless the registration flow was just
    complex enough to allow the regulator to properly reset between
calls.
    
    Fix this by re-arranging regulator_register, trying resolve the
    regulator's supply early enough that set_machine_constraints does
not
    need to be called twice.
    
    Signed-off-by: Christian Kohlschütter <christian@kohlschutter.com>
    Link:
https://lore.kernel.org/r/20220818124646.6005-1-christian@kohlschutter.com
    Signed-off-by: Mark Brown <broonie@kernel.org>

"

So most boards were already broken when the commit to force a pulldown
value was introduced.




Regards
Alban

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Alban Browaeys]

Le jeudi 28 mars 2024 à 18:01 +0000, Conor Dooley a écrit :
> On Thu, Mar 28, 2024 at 06:00:03PM +0100, Alban Browaeys wrote:
> > Le mardi 26 mars 2024 à 19:46 +0000, Conor Dooley a écrit :
> > > On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via
> > > B4
> > > Relay wrote:
> > > > From: Folker Schwesinger <dev@folker-schwesinger.de>
> > > > -	if (of_property_read_bool(dev->of_node,
> > > > "rockchip,enable-
> > > > strobe-pulldown"))
> > > > -		rk_phy->enable_strobe_pulldown =
> > > > PHYCTRL_REN_STRB_ENABLE;
> > > > +	if (of_property_read_bool(dev->of_node,
> > > > "rockchip,disable-
> > > > strobe-pulldown"))
> > > > +		rk_phy->enable_strobe_pulldown =
> > > > PHYCTRL_REN_STRB_DISABLE;
> > > 
> > > Unfortunately you cannot do this.
> > > Previously no property at all meant disabled and a property was
> > > required
> > > to enable it. With this change the absence of a property means
> > > that
> > > it
> > > will be enabled.
> > > An old devicetree is that wanted this to be disabled would have
> > > no
> > > property and will now end up with it enabled. This is an ABI
> > > break
> > > and is
> > > clearly not backwards compatible, that's a NAK unless it is
> > > demonstrable
> > > that noone actually wants to disable it at all.
> > 
> > 
> > But the patch that introduced the new default to disable the
> > pulldown
> > explicitely introduced a regression for at least 4 boards.
> > It took time to sort out that the default to disable pulldown was
> > the
> > culprit but still.
> > Will we carry this new behavor that breaks the default design for
> > rk3399 because since the regression was introduced new board
> > definition
> > might have expceted this new behavior.
> > 
> > Could the best option be to revert to énot set a default
> > enable/disable
> > pulldown" (as before the commit that introduced the regression) and
> > allow one to force the pulldown via the enable/disable pulldown
> > property?
> > I mean the commit that introduced a default value for the pulldown
> > did
> > not seem to be about fixing anything. But it broke a lot. ANd it
> > was
> > really really hard to find the description of this commit to
> > understand
> > that one had to enable pulldown to restore hs400.
> > 
> > In more than 3 years, only one board maintainer noticed that this
> > property was required to get back HS400  and thanks to a user
> > telling
> > me that this board was working I found from this board that this
> > property was "missing" from most board definitions (while it was
> > not
> > required before).
> > 
> > 
> > I am all for not breaking ABI. But what about not reverting a patch
> > that already broke ABI because this patch introduced a new ABI that
> > we
> > don't want to break?
> > I mean shouldn't a new commit with a new ABI that regressed the
> > kernel
> > be reverted?
> 
> I think I said it after OP replied to me yesterday, but this is a
> pretty
> shitty situation in that the original default picked for the property
> was actually incorrect. Given it's been like this for four years
> before
> anyone noticed, and others probably depend on the current behaviour,
> that's hard to justify.
> 

A lot of people noticed fast that HS400 was broken in the 5.10 branch
but due to another commit (more later, ie double regulator init that
messed up emmc) this second breakage was not detected. But mostly
downstream. And most if not all rk3399 boards in Armbian had HS400
disabled.


It took 3 years to detect that HS400 was broken on a few boards like
Rock Pi4 in the upstream kernel. Any might still be broken.
I would not count on the fact that keeping the current behavior equals
no more broken boards.

From the previous exchanges the boards that requires the pulldown to be
disabled seems well known.

Though I am fine with adding a property to set enable pulldown to any
board definition file where that is required.

Only I do not believe keeping the statu quo equal everything works
because it has been 3 years.
In fact this commit reached the downstream kernels way later. Any
stayed with the 5.10 branch for years.

But on the other side the disable pulldown by default is alraedy in
stable/linux-rolling-lts .



> > Mind fixing the initial regression 8b5c2b45b8f0 "phy: rockchip: set
> > pulldown for strobe line in dts" does not necessarily mean changing
> > the
> > default to the opposite value but could also be reverting to not
> > setting a default.
> 
> That's also problematic, as the only way to do this is make setting
> one of the enabled or disabled properties required, which is also an
> ABI
> break, since you'd then be rejecting probe if one is not present.


I don't understand.
How reverting to not set either pulldown enabled or disabled by default
force all board to set either enabled or disabled.
I was telling about making the pulldown set by kernel optional be it
enabled or disabled to revert to the previous behavior. 

I mean before the patch to set a default pulldown value (to disabled)
there were no forced value.



> > Though I don't know if there are pros to setting a default.
> 
> What you probably have to weigh up is the cons of each side. If what
> you
> lose is HS400 mode with what's in the kernel right now but switching
> to
> what's been proposed would entirely break some boards, I know which
> I think the lesser of two evils is.

More boards (even if not the most wide spread it seems) are broken by
the current behavior.

I agree that only HS400 is broken by keeping the status quo. But as far
as I understand only HS400 will be broken either way.
Be that by keeping the current disable pulldown which break the boards
based on the rockchip default design or the boards that are non-
standard or have a broken design.
Both case this lead to data corruption on boot to eMMC.

The only pro of keeping the current value the default is that most
board broken by the new default introduced in 2020 "might" already be
fixed (but that is just a guess).



> It's probably up to the platform maintainer to weigh in at this
> point.

I am not knowledged into the delegation scope. You mean that from now
on it is up to the rockchip maintainer?
I am fine with it either way.

I just wanted to point out that maybe we don't have to set a pulldown
value after all. And that then all boards will be fine as before
setting the pulldown explicitly was introduced.


In fact I am more eager to get this fixed be it by adding a enable-
pulldown property to the board definitions, than to change the current
behavior.
Just wanted to sort out if that was not the wrong way to fix this
issue. (ie if adding a setting on most boards was wrong).


> Hope that helps?
> Conor.


During more than 2 years, I tried various patches and discussed on
forums about the HS400 breakage. I had bisected the regulator init
issue in the 5.10 branch. Sadly it took so much time for this issue to
be understood that when the force pulldown to disable commit was
introduced downstream before the first issue go fixed.
This only made the matter worse because when one fixed the double
regulator init issue HS400 was still broken, this time because the
pulldown was forced to disable. But nobody noticed this commit that
forced a default pulldown state (that was older than the regulator
commit from 5.13 backported to the 5.10 stable branch commit, but that
reached downstream later due to not having been backported to 5.10 from
5.11).
Otherwise we would have emailed immeditaly.
Bisecting was only able to catch the first breakage (as it was only
fixed after the second breakage was introduced).

Maybe the problem is that me and others did not complained to the
kernel upstream ML because we were using heavily patched downstream
kernels (like most if not all downstream ARM kernels). So sadly, the
forums from back then are filled with complaints but nothing seemed to
have reached the Linux ML.



About the regulator double init, stable downstream branches were hit by
a bug in the 5.10 stable branch in May 2021 before they switched to
5.11 were this default pulldown was introduced. Thus they could not
detect that this pulldown broke HS400 because HS400 was already broken
by a double regulator init, backported in 5.10 from 5.13:
"
commit 06653ebc0ad2e0b7d799cd71a5c2933ed2fb7a66
Author: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Date:   Thu May 20 01:12:23 2021 +0300

	regulator: core: resolve supply for boot-on/always-on
regulators
   
	commit 98e48cd9283dbac0e1445ee780889f10b3d1db6a upstream.
   
	For the boot-on/always-on regulators the
set_machine_constrainst() is
	called before resolving rdev->supply. Thus the code would try
to enable
	rdev before enabling supplying regulator. Enforce resolving
supply
	regulator before enabling rdev.
   
	Fixes: aea6cb99703e ("regulator: resolve supply after creating
regulator")
	Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
	Link:
https://lore.kernel.org/r/20210519221224.2868496-1-dmitry.baryshkov@linaro.org
	Signed-off-by: Mark Brown <broonie@kernel.org>
	Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
"
and which to my knowledge was only fixed in v6.1-rc1 "
commit 8a866d527ac0441c0eb14a991fa11358b476b11d
Author: Christian Kohlschütter <christian@kohlschutter.com>
Date:   Thu Aug 18 12:46:47 2022 +0000

    regulator: core: Resolve supply name earlier to prevent double-init
    
    Previously, an unresolved regulator supply reference upon calling
    regulator_register on an always-on or boot-on regulator caused
    set_machine_constraints to be called twice.
    
    This in turn may initialize the regulator twice, leading to voltage
    glitches that are timing-dependent. A simple, unrelated
configuration
    change may be enough to hide this problem, only to be surfaced by
    chance.
    
    One such example is the SD-Card voltage regulator in a NanoPI R4S
that
    would not initialize reliably unless the registration flow was just
    complex enough to allow the regulator to properly reset between
calls.
    
    Fix this by re-arranging regulator_register, trying resolve the
    regulator's supply early enough that set_machine_constraints does
not
    need to be called twice.
    
    Signed-off-by: Christian Kohlschütter <christian@kohlschutter.com>
    Link:
https://lore.kernel.org/r/20220818124646.6005-1-christian@kohlschutter.com
    Signed-off-by: Mark Brown <broonie@kernel.org>

"

So most boards were already broken when the commit to force a pulldown
value was introduced.




Regards
Alban

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Alban Browaeys]

Le jeudi 28 mars 2024 à 18:01 +0000, Conor Dooley a écrit :
> On Thu, Mar 28, 2024 at 06:00:03PM +0100, Alban Browaeys wrote:
> > Le mardi 26 mars 2024 à 19:46 +0000, Conor Dooley a écrit :
> > > On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via
> > > B4
> > > Relay wrote:
> > > > From: Folker Schwesinger <dev@folker-schwesinger.de>
> > > > -	if (of_property_read_bool(dev->of_node,
> > > > "rockchip,enable-
> > > > strobe-pulldown"))
> > > > -		rk_phy->enable_strobe_pulldown =
> > > > PHYCTRL_REN_STRB_ENABLE;
> > > > +	if (of_property_read_bool(dev->of_node,
> > > > "rockchip,disable-
> > > > strobe-pulldown"))
> > > > +		rk_phy->enable_strobe_pulldown =
> > > > PHYCTRL_REN_STRB_DISABLE;
> > > 
> > > Unfortunately you cannot do this.
> > > Previously no property at all meant disabled and a property was
> > > required
> > > to enable it. With this change the absence of a property means
> > > that
> > > it
> > > will be enabled.
> > > An old devicetree is that wanted this to be disabled would have
> > > no
> > > property and will now end up with it enabled. This is an ABI
> > > break
> > > and is
> > > clearly not backwards compatible, that's a NAK unless it is
> > > demonstrable
> > > that noone actually wants to disable it at all.
> > 
> > 
> > But the patch that introduced the new default to disable the
> > pulldown
> > explicitely introduced a regression for at least 4 boards.
> > It took time to sort out that the default to disable pulldown was
> > the
> > culprit but still.
> > Will we carry this new behavor that breaks the default design for
> > rk3399 because since the regression was introduced new board
> > definition
> > might have expceted this new behavior.
> > 
> > Could the best option be to revert to énot set a default
> > enable/disable
> > pulldown" (as before the commit that introduced the regression) and
> > allow one to force the pulldown via the enable/disable pulldown
> > property?
> > I mean the commit that introduced a default value for the pulldown
> > did
> > not seem to be about fixing anything. But it broke a lot. ANd it
> > was
> > really really hard to find the description of this commit to
> > understand
> > that one had to enable pulldown to restore hs400.
> > 
> > In more than 3 years, only one board maintainer noticed that this
> > property was required to get back HS400  and thanks to a user
> > telling
> > me that this board was working I found from this board that this
> > property was "missing" from most board definitions (while it was
> > not
> > required before).
> > 
> > 
> > I am all for not breaking ABI. But what about not reverting a patch
> > that already broke ABI because this patch introduced a new ABI that
> > we
> > don't want to break?
> > I mean shouldn't a new commit with a new ABI that regressed the
> > kernel
> > be reverted?
> 
> I think I said it after OP replied to me yesterday, but this is a
> pretty
> shitty situation in that the original default picked for the property
> was actually incorrect. Given it's been like this for four years
> before
> anyone noticed, and others probably depend on the current behaviour,
> that's hard to justify.
> 

A lot of people noticed fast that HS400 was broken in the 5.10 branch
but due to another commit (more later, ie double regulator init that
messed up emmc) this second breakage was not detected. But mostly
downstream. And most if not all rk3399 boards in Armbian had HS400
disabled.


It took 3 years to detect that HS400 was broken on a few boards like
Rock Pi4 in the upstream kernel. Any might still be broken.
I would not count on the fact that keeping the current behavior equals
no more broken boards.

From the previous exchanges the boards that requires the pulldown to be
disabled seems well known.

Though I am fine with adding a property to set enable pulldown to any
board definition file where that is required.

Only I do not believe keeping the statu quo equal everything works
because it has been 3 years.
In fact this commit reached the downstream kernels way later. Any
stayed with the 5.10 branch for years.

But on the other side the disable pulldown by default is alraedy in
stable/linux-rolling-lts .



> > Mind fixing the initial regression 8b5c2b45b8f0 "phy: rockchip: set
> > pulldown for strobe line in dts" does not necessarily mean changing
> > the
> > default to the opposite value but could also be reverting to not
> > setting a default.
> 
> That's also problematic, as the only way to do this is make setting
> one of the enabled or disabled properties required, which is also an
> ABI
> break, since you'd then be rejecting probe if one is not present.


I don't understand.
How reverting to not set either pulldown enabled or disabled by default
force all board to set either enabled or disabled.
I was telling about making the pulldown set by kernel optional be it
enabled or disabled to revert to the previous behavior. 

I mean before the patch to set a default pulldown value (to disabled)
there were no forced value.



> > Though I don't know if there are pros to setting a default.
> 
> What you probably have to weigh up is the cons of each side. If what
> you
> lose is HS400 mode with what's in the kernel right now but switching
> to
> what's been proposed would entirely break some boards, I know which
> I think the lesser of two evils is.

More boards (even if not the most wide spread it seems) are broken by
the current behavior.

I agree that only HS400 is broken by keeping the status quo. But as far
as I understand only HS400 will be broken either way.
Be that by keeping the current disable pulldown which break the boards
based on the rockchip default design or the boards that are non-
standard or have a broken design.
Both case this lead to data corruption on boot to eMMC.

The only pro of keeping the current value the default is that most
board broken by the new default introduced in 2020 "might" already be
fixed (but that is just a guess).



> It's probably up to the platform maintainer to weigh in at this
> point.

I am not knowledged into the delegation scope. You mean that from now
on it is up to the rockchip maintainer?
I am fine with it either way.

I just wanted to point out that maybe we don't have to set a pulldown
value after all. And that then all boards will be fine as before
setting the pulldown explicitly was introduced.


In fact I am more eager to get this fixed be it by adding a enable-
pulldown property to the board definitions, than to change the current
behavior.
Just wanted to sort out if that was not the wrong way to fix this
issue. (ie if adding a setting on most boards was wrong).


> Hope that helps?
> Conor.


During more than 2 years, I tried various patches and discussed on
forums about the HS400 breakage. I had bisected the regulator init
issue in the 5.10 branch. Sadly it took so much time for this issue to
be understood that when the force pulldown to disable commit was
introduced downstream before the first issue go fixed.
This only made the matter worse because when one fixed the double
regulator init issue HS400 was still broken, this time because the
pulldown was forced to disable. But nobody noticed this commit that
forced a default pulldown state (that was older than the regulator
commit from 5.13 backported to the 5.10 stable branch commit, but that
reached downstream later due to not having been backported to 5.10 from
5.11).
Otherwise we would have emailed immeditaly.
Bisecting was only able to catch the first breakage (as it was only
fixed after the second breakage was introduced).

Maybe the problem is that me and others did not complained to the
kernel upstream ML because we were using heavily patched downstream
kernels (like most if not all downstream ARM kernels). So sadly, the
forums from back then are filled with complaints but nothing seemed to
have reached the Linux ML.



About the regulator double init, stable downstream branches were hit by
a bug in the 5.10 stable branch in May 2021 before they switched to
5.11 were this default pulldown was introduced. Thus they could not
detect that this pulldown broke HS400 because HS400 was already broken
by a double regulator init, backported in 5.10 from 5.13:
"
commit 06653ebc0ad2e0b7d799cd71a5c2933ed2fb7a66
Author: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Date:   Thu May 20 01:12:23 2021 +0300

	regulator: core: resolve supply for boot-on/always-on
regulators
   
	commit 98e48cd9283dbac0e1445ee780889f10b3d1db6a upstream.
   
	For the boot-on/always-on regulators the
set_machine_constrainst() is
	called before resolving rdev->supply. Thus the code would try
to enable
	rdev before enabling supplying regulator. Enforce resolving
supply
	regulator before enabling rdev.
   
	Fixes: aea6cb99703e ("regulator: resolve supply after creating
regulator")
	Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
	Link:
https://lore.kernel.org/r/20210519221224.2868496-1-dmitry.baryshkov@linaro.org
	Signed-off-by: Mark Brown <broonie@kernel.org>
	Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
"
and which to my knowledge was only fixed in v6.1-rc1 "
commit 8a866d527ac0441c0eb14a991fa11358b476b11d
Author: Christian Kohlschütter <christian@kohlschutter.com>
Date:   Thu Aug 18 12:46:47 2022 +0000

    regulator: core: Resolve supply name earlier to prevent double-init
    
    Previously, an unresolved regulator supply reference upon calling
    regulator_register on an always-on or boot-on regulator caused
    set_machine_constraints to be called twice.
    
    This in turn may initialize the regulator twice, leading to voltage
    glitches that are timing-dependent. A simple, unrelated
configuration
    change may be enough to hide this problem, only to be surfaced by
    chance.
    
    One such example is the SD-Card voltage regulator in a NanoPI R4S
that
    would not initialize reliably unless the registration flow was just
    complex enough to allow the regulator to properly reset between
calls.
    
    Fix this by re-arranging regulator_register, trying resolve the
    regulator's supply early enough that set_machine_constraints does
not
    need to be called twice.
    
    Signed-off-by: Christian Kohlschütter <christian@kohlschutter.com>
    Link:
https://lore.kernel.org/r/20220818124646.6005-1-christian@kohlschutter.com
    Signed-off-by: Mark Brown <broonie@kernel.org>

"

So most boards were already broken when the commit to force a pulldown
value was introduced.




Regards
Alban

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 9867 bytes --]

On Wed, Apr 10, 2024 at 08:28:57PM +0200, Alban Browaeys wrote:
> Le jeudi 28 mars 2024 à 18:01 +0000, Conor Dooley a écrit :
> > On Thu, Mar 28, 2024 at 06:00:03PM +0100, Alban Browaeys wrote:
> > > Le mardi 26 mars 2024 à 19:46 +0000, Conor Dooley a écrit :
> > > > On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via
> > > > B4
> > > > Relay wrote:
> > > > > From: Folker Schwesinger <dev@folker-schwesinger.de>
> > > > > -	if (of_property_read_bool(dev->of_node,
> > > > > "rockchip,enable-
> > > > > strobe-pulldown"))
> > > > > -		rk_phy->enable_strobe_pulldown =
> > > > > PHYCTRL_REN_STRB_ENABLE;
> > > > > +	if (of_property_read_bool(dev->of_node,
> > > > > "rockchip,disable-
> > > > > strobe-pulldown"))
> > > > > +		rk_phy->enable_strobe_pulldown =
> > > > > PHYCTRL_REN_STRB_DISABLE;
> > > > 
> > > > Unfortunately you cannot do this.
> > > > Previously no property at all meant disabled and a property was
> > > > required
> > > > to enable it. With this change the absence of a property means
> > > > that
> > > > it
> > > > will be enabled.
> > > > An old devicetree is that wanted this to be disabled would have
> > > > no
> > > > property and will now end up with it enabled. This is an ABI
> > > > break
> > > > and is
> > > > clearly not backwards compatible, that's a NAK unless it is
> > > > demonstrable
> > > > that noone actually wants to disable it at all.
> > > 
> > > 
> > > But the patch that introduced the new default to disable the
> > > pulldown
> > > explicitely introduced a regression for at least 4 boards.
> > > It took time to sort out that the default to disable pulldown was
> > > the
> > > culprit but still.
> > > Will we carry this new behavor that breaks the default design for
> > > rk3399 because since the regression was introduced new board
> > > definition
> > > might have expceted this new behavior.
> > > 
> > > Could the best option be to revert to énot set a default
> > > enable/disable
> > > pulldown" (as before the commit that introduced the regression) and
> > > allow one to force the pulldown via the enable/disable pulldown
> > > property?
> > > I mean the commit that introduced a default value for the pulldown
> > > did
> > > not seem to be about fixing anything. But it broke a lot. ANd it
> > > was
> > > really really hard to find the description of this commit to
> > > understand
> > > that one had to enable pulldown to restore hs400.
> > > 
> > > In more than 3 years, only one board maintainer noticed that this
> > > property was required to get back HS400  and thanks to a user
> > > telling
> > > me that this board was working I found from this board that this
> > > property was "missing" from most board definitions (while it was
> > > not
> > > required before).
> > > 
> > > 
> > > I am all for not breaking ABI. But what about not reverting a patch
> > > that already broke ABI because this patch introduced a new ABI that
> > > we
> > > don't want to break?
> > > I mean shouldn't a new commit with a new ABI that regressed the
> > > kernel
> > > be reverted?
> > 
> > I think I said it after OP replied to me yesterday, but this is a
> > pretty
> > shitty situation in that the original default picked for the property
> > was actually incorrect. Given it's been like this for four years
> > before
> > anyone noticed, and others probably depend on the current behaviour,
> > that's hard to justify.
> > 
> 
> A lot of people noticed fast that HS400 was broken in the 5.10 branch
> but due to another commit (more later, ie double regulator init that
> messed up emmc) this second breakage was not detected. But mostly
> downstream. And most if not all rk3399 boards in Armbian had HS400
> disabled.
> 
> 
> It took 3 years to detect that HS400 was broken on a few boards like
> Rock Pi4 in the upstream kernel. Any might still be broken.
> I would not count on the fact that keeping the current behavior equals
> no more broken boards.
> 
> From the previous exchanges the boards that requires the pulldown to be
> disabled seems well known.
> 
> Though I am fine with adding a property to set enable pulldown to any
> board definition file where that is required.
> 
> Only I do not believe keeping the statu quo equal everything works
> because it has been 3 years.

FWIW, I didn't say this. Clearly if that was the case, this patch would
never have arrived.

> In fact this commit reached the downstream kernels way later. Any
> stayed with the 5.10 branch for years.
> 
> But on the other side the disable pulldown by default is alraedy in
> stable/linux-rolling-lts .
> 
> > > Mind fixing the initial regression 8b5c2b45b8f0 "phy: rockchip: set
> > > pulldown for strobe line in dts" does not necessarily mean changing
> > > the
> > > default to the opposite value but could also be reverting to not
> > > setting a default.
> > 
> > That's also problematic, as the only way to do this is make setting
> > one of the enabled or disabled properties required, which is also an
> > ABI
> > break, since you'd then be rejecting probe if one is not present.
> 
> 
> I don't understand.
> How reverting to not set either pulldown enabled or disabled by default
> force all board to set either enabled or disabled.
> I was telling about making the pulldown set by kernel optional be it
> enabled or disabled to revert to the previous behavior. 
> 
> I mean before the patch to set a default pulldown value (to disabled)
> there were no forced value.

Ah, maybe I misunderstood what the code originally did. Did the original
code leave the bit however the bootloader or reset value had left it?
In that case, probe wouldn't be rejected and you'd not have the sort of
issue that I mentioned above.

> > > Though I don't know if there are pros to setting a default.
> > 
> > What you probably have to weigh up is the cons of each side. If what
> > you
> > lose is HS400 mode with what's in the kernel right now but switching
> > to
> > what's been proposed would entirely break some boards, I know which
> > I think the lesser of two evils is.
> 
> More boards (even if not the most wide spread it seems) are broken by
> the current behavior.
> 
> I agree that only HS400 is broken by keeping the status quo. But as far
> as I understand only HS400 will be broken either way.
> Be that by keeping the current disable pulldown which break the boards
> based on the rockchip default design or the boards that are non-
> standard or have a broken design.
> Both case this lead to data corruption on boot to eMMC.
> 
> The only pro of keeping the current value the default is that most
> board broken by the new default introduced in 2020 "might" already be
> fixed (but that is just a guess).
> 
> > It's probably up to the platform maintainer to weigh in at this
> > point.
> 
> I am not knowledged into the delegation scope. You mean that from now
> on it is up to the rockchip maintainer?
> I am fine with it either way.

Yes, I meant the rockchip maintainer. I'm only a lowly bindings
maintainer, without any knowledge of rockchip specfics or the type of
boards we're talking about being broken here. Someone has to make a
judgement call about which "no property" behaviour is used going forward
and I don't want that to be me!

> I just wanted to point out that maybe we don't have to set a pulldown
> value after all. And that then all boards will be fine as before
> setting the pulldown explicitly was introduced.

By "all boards will be fine" you mean "all boards that expected the
kernel didn't touch this bit will be fine". The boards that need the
kernel to set this bit because it {comes out of reset,is set by firmware}
incorrectly are going to need a property added if we revert the default
behaviour to not touching the bit.

> In fact I am more eager to get this fixed be it by adding a enable-
> pulldown property to the board definitions, than to change the current
> behavior.
> Just wanted to sort out if that was not the wrong way to fix this
> issue. (ie if adding a setting on most boards was wrong).

> During more than 2 years, I tried various patches and discussed on
> forums about the HS400 breakage. I had bisected the regulator init
> issue in the 5.10 branch. Sadly it took so much time for this issue to
> be understood that when the force pulldown to disable commit was
> introduced downstream before the first issue go fixed.
> This only made the matter worse because when one fixed the double
> regulator init issue HS400 was still broken, this time because the
> pulldown was forced to disable. But nobody noticed this commit that
> forced a default pulldown state (that was older than the regulator
> commit from 5.13 backported to the 5.10 stable branch commit, but that
> reached downstream later due to not having been backported to 5.10 from
> 5.11).
> Otherwise we would have emailed immeditaly.
> Bisecting was only able to catch the first breakage (as it was only
> fixed after the second breakage was introduced).
> 
> Maybe the problem is that me and others did not complained to the
> kernel upstream ML because we were using heavily patched downstream
> kernels (like most if not all downstream ARM kernels). So sadly, the
> forums from back then are filled with complaints but nothing seemed to
> have reached the Linux ML.

Aye, and all I can really say there is to buy boards from a vendor that
doesn't use some horribly hacked downstream kernel, which I know is
clearly an unsatisfactory suggestion. That said, we probably should have
caught that the new default behaviour when the changes were made was not
the default before. There was only one DT maintainer then though, and
things just slip by :/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 9867 bytes --]

On Wed, Apr 10, 2024 at 08:28:57PM +0200, Alban Browaeys wrote:
> Le jeudi 28 mars 2024 à 18:01 +0000, Conor Dooley a écrit :
> > On Thu, Mar 28, 2024 at 06:00:03PM +0100, Alban Browaeys wrote:
> > > Le mardi 26 mars 2024 à 19:46 +0000, Conor Dooley a écrit :
> > > > On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via
> > > > B4
> > > > Relay wrote:
> > > > > From: Folker Schwesinger <dev@folker-schwesinger.de>
> > > > > -	if (of_property_read_bool(dev->of_node,
> > > > > "rockchip,enable-
> > > > > strobe-pulldown"))
> > > > > -		rk_phy->enable_strobe_pulldown =
> > > > > PHYCTRL_REN_STRB_ENABLE;
> > > > > +	if (of_property_read_bool(dev->of_node,
> > > > > "rockchip,disable-
> > > > > strobe-pulldown"))
> > > > > +		rk_phy->enable_strobe_pulldown =
> > > > > PHYCTRL_REN_STRB_DISABLE;
> > > > 
> > > > Unfortunately you cannot do this.
> > > > Previously no property at all meant disabled and a property was
> > > > required
> > > > to enable it. With this change the absence of a property means
> > > > that
> > > > it
> > > > will be enabled.
> > > > An old devicetree is that wanted this to be disabled would have
> > > > no
> > > > property and will now end up with it enabled. This is an ABI
> > > > break
> > > > and is
> > > > clearly not backwards compatible, that's a NAK unless it is
> > > > demonstrable
> > > > that noone actually wants to disable it at all.
> > > 
> > > 
> > > But the patch that introduced the new default to disable the
> > > pulldown
> > > explicitely introduced a regression for at least 4 boards.
> > > It took time to sort out that the default to disable pulldown was
> > > the
> > > culprit but still.
> > > Will we carry this new behavor that breaks the default design for
> > > rk3399 because since the regression was introduced new board
> > > definition
> > > might have expceted this new behavior.
> > > 
> > > Could the best option be to revert to énot set a default
> > > enable/disable
> > > pulldown" (as before the commit that introduced the regression) and
> > > allow one to force the pulldown via the enable/disable pulldown
> > > property?
> > > I mean the commit that introduced a default value for the pulldown
> > > did
> > > not seem to be about fixing anything. But it broke a lot. ANd it
> > > was
> > > really really hard to find the description of this commit to
> > > understand
> > > that one had to enable pulldown to restore hs400.
> > > 
> > > In more than 3 years, only one board maintainer noticed that this
> > > property was required to get back HS400  and thanks to a user
> > > telling
> > > me that this board was working I found from this board that this
> > > property was "missing" from most board definitions (while it was
> > > not
> > > required before).
> > > 
> > > 
> > > I am all for not breaking ABI. But what about not reverting a patch
> > > that already broke ABI because this patch introduced a new ABI that
> > > we
> > > don't want to break?
> > > I mean shouldn't a new commit with a new ABI that regressed the
> > > kernel
> > > be reverted?
> > 
> > I think I said it after OP replied to me yesterday, but this is a
> > pretty
> > shitty situation in that the original default picked for the property
> > was actually incorrect. Given it's been like this for four years
> > before
> > anyone noticed, and others probably depend on the current behaviour,
> > that's hard to justify.
> > 
> 
> A lot of people noticed fast that HS400 was broken in the 5.10 branch
> but due to another commit (more later, ie double regulator init that
> messed up emmc) this second breakage was not detected. But mostly
> downstream. And most if not all rk3399 boards in Armbian had HS400
> disabled.
> 
> 
> It took 3 years to detect that HS400 was broken on a few boards like
> Rock Pi4 in the upstream kernel. Any might still be broken.
> I would not count on the fact that keeping the current behavior equals
> no more broken boards.
> 
> From the previous exchanges the boards that requires the pulldown to be
> disabled seems well known.
> 
> Though I am fine with adding a property to set enable pulldown to any
> board definition file where that is required.
> 
> Only I do not believe keeping the statu quo equal everything works
> because it has been 3 years.

FWIW, I didn't say this. Clearly if that was the case, this patch would
never have arrived.

> In fact this commit reached the downstream kernels way later. Any
> stayed with the 5.10 branch for years.
> 
> But on the other side the disable pulldown by default is alraedy in
> stable/linux-rolling-lts .
> 
> > > Mind fixing the initial regression 8b5c2b45b8f0 "phy: rockchip: set
> > > pulldown for strobe line in dts" does not necessarily mean changing
> > > the
> > > default to the opposite value but could also be reverting to not
> > > setting a default.
> > 
> > That's also problematic, as the only way to do this is make setting
> > one of the enabled or disabled properties required, which is also an
> > ABI
> > break, since you'd then be rejecting probe if one is not present.
> 
> 
> I don't understand.
> How reverting to not set either pulldown enabled or disabled by default
> force all board to set either enabled or disabled.
> I was telling about making the pulldown set by kernel optional be it
> enabled or disabled to revert to the previous behavior. 
> 
> I mean before the patch to set a default pulldown value (to disabled)
> there were no forced value.

Ah, maybe I misunderstood what the code originally did. Did the original
code leave the bit however the bootloader or reset value had left it?
In that case, probe wouldn't be rejected and you'd not have the sort of
issue that I mentioned above.

> > > Though I don't know if there are pros to setting a default.
> > 
> > What you probably have to weigh up is the cons of each side. If what
> > you
> > lose is HS400 mode with what's in the kernel right now but switching
> > to
> > what's been proposed would entirely break some boards, I know which
> > I think the lesser of two evils is.
> 
> More boards (even if not the most wide spread it seems) are broken by
> the current behavior.
> 
> I agree that only HS400 is broken by keeping the status quo. But as far
> as I understand only HS400 will be broken either way.
> Be that by keeping the current disable pulldown which break the boards
> based on the rockchip default design or the boards that are non-
> standard or have a broken design.
> Both case this lead to data corruption on boot to eMMC.
> 
> The only pro of keeping the current value the default is that most
> board broken by the new default introduced in 2020 "might" already be
> fixed (but that is just a guess).
> 
> > It's probably up to the platform maintainer to weigh in at this
> > point.
> 
> I am not knowledged into the delegation scope. You mean that from now
> on it is up to the rockchip maintainer?
> I am fine with it either way.

Yes, I meant the rockchip maintainer. I'm only a lowly bindings
maintainer, without any knowledge of rockchip specfics or the type of
boards we're talking about being broken here. Someone has to make a
judgement call about which "no property" behaviour is used going forward
and I don't want that to be me!

> I just wanted to point out that maybe we don't have to set a pulldown
> value after all. And that then all boards will be fine as before
> setting the pulldown explicitly was introduced.

By "all boards will be fine" you mean "all boards that expected the
kernel didn't touch this bit will be fine". The boards that need the
kernel to set this bit because it {comes out of reset,is set by firmware}
incorrectly are going to need a property added if we revert the default
behaviour to not touching the bit.

> In fact I am more eager to get this fixed be it by adding a enable-
> pulldown property to the board definitions, than to change the current
> behavior.
> Just wanted to sort out if that was not the wrong way to fix this
> issue. (ie if adding a setting on most boards was wrong).

> During more than 2 years, I tried various patches and discussed on
> forums about the HS400 breakage. I had bisected the regulator init
> issue in the 5.10 branch. Sadly it took so much time for this issue to
> be understood that when the force pulldown to disable commit was
> introduced downstream before the first issue go fixed.
> This only made the matter worse because when one fixed the double
> regulator init issue HS400 was still broken, this time because the
> pulldown was forced to disable. But nobody noticed this commit that
> forced a default pulldown state (that was older than the regulator
> commit from 5.13 backported to the 5.10 stable branch commit, but that
> reached downstream later due to not having been backported to 5.10 from
> 5.11).
> Otherwise we would have emailed immeditaly.
> Bisecting was only able to catch the first breakage (as it was only
> fixed after the second breakage was introduced).
> 
> Maybe the problem is that me and others did not complained to the
> kernel upstream ML because we were using heavily patched downstream
> kernels (like most if not all downstream ARM kernels). So sadly, the
> forums from back then are filled with complaints but nothing seemed to
> have reached the Linux ML.

Aye, and all I can really say there is to buy boards from a vendor that
doesn't use some horribly hacked downstream kernel, which I know is
clearly an unsatisfactory suggestion. That said, we probably should have
caught that the new default behaviour when the changes were made was not
the default before. There was only one DT maintainer then though, and
things just slip by :/

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 9867 bytes --]

On Wed, Apr 10, 2024 at 08:28:57PM +0200, Alban Browaeys wrote:
> Le jeudi 28 mars 2024 à 18:01 +0000, Conor Dooley a écrit :
> > On Thu, Mar 28, 2024 at 06:00:03PM +0100, Alban Browaeys wrote:
> > > Le mardi 26 mars 2024 à 19:46 +0000, Conor Dooley a écrit :
> > > > On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via
> > > > B4
> > > > Relay wrote:
> > > > > From: Folker Schwesinger <dev@folker-schwesinger.de>
> > > > > -	if (of_property_read_bool(dev->of_node,
> > > > > "rockchip,enable-
> > > > > strobe-pulldown"))
> > > > > -		rk_phy->enable_strobe_pulldown =
> > > > > PHYCTRL_REN_STRB_ENABLE;
> > > > > +	if (of_property_read_bool(dev->of_node,
> > > > > "rockchip,disable-
> > > > > strobe-pulldown"))
> > > > > +		rk_phy->enable_strobe_pulldown =
> > > > > PHYCTRL_REN_STRB_DISABLE;
> > > > 
> > > > Unfortunately you cannot do this.
> > > > Previously no property at all meant disabled and a property was
> > > > required
> > > > to enable it. With this change the absence of a property means
> > > > that
> > > > it
> > > > will be enabled.
> > > > An old devicetree is that wanted this to be disabled would have
> > > > no
> > > > property and will now end up with it enabled. This is an ABI
> > > > break
> > > > and is
> > > > clearly not backwards compatible, that's a NAK unless it is
> > > > demonstrable
> > > > that noone actually wants to disable it at all.
> > > 
> > > 
> > > But the patch that introduced the new default to disable the
> > > pulldown
> > > explicitely introduced a regression for at least 4 boards.
> > > It took time to sort out that the default to disable pulldown was
> > > the
> > > culprit but still.
> > > Will we carry this new behavor that breaks the default design for
> > > rk3399 because since the regression was introduced new board
> > > definition
> > > might have expceted this new behavior.
> > > 
> > > Could the best option be to revert to énot set a default
> > > enable/disable
> > > pulldown" (as before the commit that introduced the regression) and
> > > allow one to force the pulldown via the enable/disable pulldown
> > > property?
> > > I mean the commit that introduced a default value for the pulldown
> > > did
> > > not seem to be about fixing anything. But it broke a lot. ANd it
> > > was
> > > really really hard to find the description of this commit to
> > > understand
> > > that one had to enable pulldown to restore hs400.
> > > 
> > > In more than 3 years, only one board maintainer noticed that this
> > > property was required to get back HS400  and thanks to a user
> > > telling
> > > me that this board was working I found from this board that this
> > > property was "missing" from most board definitions (while it was
> > > not
> > > required before).
> > > 
> > > 
> > > I am all for not breaking ABI. But what about not reverting a patch
> > > that already broke ABI because this patch introduced a new ABI that
> > > we
> > > don't want to break?
> > > I mean shouldn't a new commit with a new ABI that regressed the
> > > kernel
> > > be reverted?
> > 
> > I think I said it after OP replied to me yesterday, but this is a
> > pretty
> > shitty situation in that the original default picked for the property
> > was actually incorrect. Given it's been like this for four years
> > before
> > anyone noticed, and others probably depend on the current behaviour,
> > that's hard to justify.
> > 
> 
> A lot of people noticed fast that HS400 was broken in the 5.10 branch
> but due to another commit (more later, ie double regulator init that
> messed up emmc) this second breakage was not detected. But mostly
> downstream. And most if not all rk3399 boards in Armbian had HS400
> disabled.
> 
> 
> It took 3 years to detect that HS400 was broken on a few boards like
> Rock Pi4 in the upstream kernel. Any might still be broken.
> I would not count on the fact that keeping the current behavior equals
> no more broken boards.
> 
> From the previous exchanges the boards that requires the pulldown to be
> disabled seems well known.
> 
> Though I am fine with adding a property to set enable pulldown to any
> board definition file where that is required.
> 
> Only I do not believe keeping the statu quo equal everything works
> because it has been 3 years.

FWIW, I didn't say this. Clearly if that was the case, this patch would
never have arrived.

> In fact this commit reached the downstream kernels way later. Any
> stayed with the 5.10 branch for years.
> 
> But on the other side the disable pulldown by default is alraedy in
> stable/linux-rolling-lts .
> 
> > > Mind fixing the initial regression 8b5c2b45b8f0 "phy: rockchip: set
> > > pulldown for strobe line in dts" does not necessarily mean changing
> > > the
> > > default to the opposite value but could also be reverting to not
> > > setting a default.
> > 
> > That's also problematic, as the only way to do this is make setting
> > one of the enabled or disabled properties required, which is also an
> > ABI
> > break, since you'd then be rejecting probe if one is not present.
> 
> 
> I don't understand.
> How reverting to not set either pulldown enabled or disabled by default
> force all board to set either enabled or disabled.
> I was telling about making the pulldown set by kernel optional be it
> enabled or disabled to revert to the previous behavior. 
> 
> I mean before the patch to set a default pulldown value (to disabled)
> there were no forced value.

Ah, maybe I misunderstood what the code originally did. Did the original
code leave the bit however the bootloader or reset value had left it?
In that case, probe wouldn't be rejected and you'd not have the sort of
issue that I mentioned above.

> > > Though I don't know if there are pros to setting a default.
> > 
> > What you probably have to weigh up is the cons of each side. If what
> > you
> > lose is HS400 mode with what's in the kernel right now but switching
> > to
> > what's been proposed would entirely break some boards, I know which
> > I think the lesser of two evils is.
> 
> More boards (even if not the most wide spread it seems) are broken by
> the current behavior.
> 
> I agree that only HS400 is broken by keeping the status quo. But as far
> as I understand only HS400 will be broken either way.
> Be that by keeping the current disable pulldown which break the boards
> based on the rockchip default design or the boards that are non-
> standard or have a broken design.
> Both case this lead to data corruption on boot to eMMC.
> 
> The only pro of keeping the current value the default is that most
> board broken by the new default introduced in 2020 "might" already be
> fixed (but that is just a guess).
> 
> > It's probably up to the platform maintainer to weigh in at this
> > point.
> 
> I am not knowledged into the delegation scope. You mean that from now
> on it is up to the rockchip maintainer?
> I am fine with it either way.

Yes, I meant the rockchip maintainer. I'm only a lowly bindings
maintainer, without any knowledge of rockchip specfics or the type of
boards we're talking about being broken here. Someone has to make a
judgement call about which "no property" behaviour is used going forward
and I don't want that to be me!

> I just wanted to point out that maybe we don't have to set a pulldown
> value after all. And that then all boards will be fine as before
> setting the pulldown explicitly was introduced.

By "all boards will be fine" you mean "all boards that expected the
kernel didn't touch this bit will be fine". The boards that need the
kernel to set this bit because it {comes out of reset,is set by firmware}
incorrectly are going to need a property added if we revert the default
behaviour to not touching the bit.

> In fact I am more eager to get this fixed be it by adding a enable-
> pulldown property to the board definitions, than to change the current
> behavior.
> Just wanted to sort out if that was not the wrong way to fix this
> issue. (ie if adding a setting on most boards was wrong).

> During more than 2 years, I tried various patches and discussed on
> forums about the HS400 breakage. I had bisected the regulator init
> issue in the 5.10 branch. Sadly it took so much time for this issue to
> be understood that when the force pulldown to disable commit was
> introduced downstream before the first issue go fixed.
> This only made the matter worse because when one fixed the double
> regulator init issue HS400 was still broken, this time because the
> pulldown was forced to disable. But nobody noticed this commit that
> forced a default pulldown state (that was older than the regulator
> commit from 5.13 backported to the 5.10 stable branch commit, but that
> reached downstream later due to not having been backported to 5.10 from
> 5.11).
> Otherwise we would have emailed immeditaly.
> Bisecting was only able to catch the first breakage (as it was only
> fixed after the second breakage was introduced).
> 
> Maybe the problem is that me and others did not complained to the
> kernel upstream ML because we were using heavily patched downstream
> kernels (like most if not all downstream ARM kernels). So sadly, the
> forums from back then are filled with complaints but nothing seemed to
> have reached the Linux ML.

Aye, and all I can really say there is to buy boards from a vendor that
doesn't use some horribly hacked downstream kernel, which I know is
clearly an unsatisfactory suggestion. That said, we probably should have
caught that the new default behaviour when the changes were made was not
the default before. There was only one DT maintainer then though, and
things just slip by :/

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 9867 bytes --]

On Wed, Apr 10, 2024 at 08:28:57PM +0200, Alban Browaeys wrote:
> Le jeudi 28 mars 2024 à 18:01 +0000, Conor Dooley a écrit :
> > On Thu, Mar 28, 2024 at 06:00:03PM +0100, Alban Browaeys wrote:
> > > Le mardi 26 mars 2024 à 19:46 +0000, Conor Dooley a écrit :
> > > > On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via
> > > > B4
> > > > Relay wrote:
> > > > > From: Folker Schwesinger <dev@folker-schwesinger.de>
> > > > > -	if (of_property_read_bool(dev->of_node,
> > > > > "rockchip,enable-
> > > > > strobe-pulldown"))
> > > > > -		rk_phy->enable_strobe_pulldown =
> > > > > PHYCTRL_REN_STRB_ENABLE;
> > > > > +	if (of_property_read_bool(dev->of_node,
> > > > > "rockchip,disable-
> > > > > strobe-pulldown"))
> > > > > +		rk_phy->enable_strobe_pulldown =
> > > > > PHYCTRL_REN_STRB_DISABLE;
> > > > 
> > > > Unfortunately you cannot do this.
> > > > Previously no property at all meant disabled and a property was
> > > > required
> > > > to enable it. With this change the absence of a property means
> > > > that
> > > > it
> > > > will be enabled.
> > > > An old devicetree is that wanted this to be disabled would have
> > > > no
> > > > property and will now end up with it enabled. This is an ABI
> > > > break
> > > > and is
> > > > clearly not backwards compatible, that's a NAK unless it is
> > > > demonstrable
> > > > that noone actually wants to disable it at all.
> > > 
> > > 
> > > But the patch that introduced the new default to disable the
> > > pulldown
> > > explicitely introduced a regression for at least 4 boards.
> > > It took time to sort out that the default to disable pulldown was
> > > the
> > > culprit but still.
> > > Will we carry this new behavor that breaks the default design for
> > > rk3399 because since the regression was introduced new board
> > > definition
> > > might have expceted this new behavior.
> > > 
> > > Could the best option be to revert to énot set a default
> > > enable/disable
> > > pulldown" (as before the commit that introduced the regression) and
> > > allow one to force the pulldown via the enable/disable pulldown
> > > property?
> > > I mean the commit that introduced a default value for the pulldown
> > > did
> > > not seem to be about fixing anything. But it broke a lot. ANd it
> > > was
> > > really really hard to find the description of this commit to
> > > understand
> > > that one had to enable pulldown to restore hs400.
> > > 
> > > In more than 3 years, only one board maintainer noticed that this
> > > property was required to get back HS400  and thanks to a user
> > > telling
> > > me that this board was working I found from this board that this
> > > property was "missing" from most board definitions (while it was
> > > not
> > > required before).
> > > 
> > > 
> > > I am all for not breaking ABI. But what about not reverting a patch
> > > that already broke ABI because this patch introduced a new ABI that
> > > we
> > > don't want to break?
> > > I mean shouldn't a new commit with a new ABI that regressed the
> > > kernel
> > > be reverted?
> > 
> > I think I said it after OP replied to me yesterday, but this is a
> > pretty
> > shitty situation in that the original default picked for the property
> > was actually incorrect. Given it's been like this for four years
> > before
> > anyone noticed, and others probably depend on the current behaviour,
> > that's hard to justify.
> > 
> 
> A lot of people noticed fast that HS400 was broken in the 5.10 branch
> but due to another commit (more later, ie double regulator init that
> messed up emmc) this second breakage was not detected. But mostly
> downstream. And most if not all rk3399 boards in Armbian had HS400
> disabled.
> 
> 
> It took 3 years to detect that HS400 was broken on a few boards like
> Rock Pi4 in the upstream kernel. Any might still be broken.
> I would not count on the fact that keeping the current behavior equals
> no more broken boards.
> 
> From the previous exchanges the boards that requires the pulldown to be
> disabled seems well known.
> 
> Though I am fine with adding a property to set enable pulldown to any
> board definition file where that is required.
> 
> Only I do not believe keeping the statu quo equal everything works
> because it has been 3 years.

FWIW, I didn't say this. Clearly if that was the case, this patch would
never have arrived.

> In fact this commit reached the downstream kernels way later. Any
> stayed with the 5.10 branch for years.
> 
> But on the other side the disable pulldown by default is alraedy in
> stable/linux-rolling-lts .
> 
> > > Mind fixing the initial regression 8b5c2b45b8f0 "phy: rockchip: set
> > > pulldown for strobe line in dts" does not necessarily mean changing
> > > the
> > > default to the opposite value but could also be reverting to not
> > > setting a default.
> > 
> > That's also problematic, as the only way to do this is make setting
> > one of the enabled or disabled properties required, which is also an
> > ABI
> > break, since you'd then be rejecting probe if one is not present.
> 
> 
> I don't understand.
> How reverting to not set either pulldown enabled or disabled by default
> force all board to set either enabled or disabled.
> I was telling about making the pulldown set by kernel optional be it
> enabled or disabled to revert to the previous behavior. 
> 
> I mean before the patch to set a default pulldown value (to disabled)
> there were no forced value.

Ah, maybe I misunderstood what the code originally did. Did the original
code leave the bit however the bootloader or reset value had left it?
In that case, probe wouldn't be rejected and you'd not have the sort of
issue that I mentioned above.

> > > Though I don't know if there are pros to setting a default.
> > 
> > What you probably have to weigh up is the cons of each side. If what
> > you
> > lose is HS400 mode with what's in the kernel right now but switching
> > to
> > what's been proposed would entirely break some boards, I know which
> > I think the lesser of two evils is.
> 
> More boards (even if not the most wide spread it seems) are broken by
> the current behavior.
> 
> I agree that only HS400 is broken by keeping the status quo. But as far
> as I understand only HS400 will be broken either way.
> Be that by keeping the current disable pulldown which break the boards
> based on the rockchip default design or the boards that are non-
> standard or have a broken design.
> Both case this lead to data corruption on boot to eMMC.
> 
> The only pro of keeping the current value the default is that most
> board broken by the new default introduced in 2020 "might" already be
> fixed (but that is just a guess).
> 
> > It's probably up to the platform maintainer to weigh in at this
> > point.
> 
> I am not knowledged into the delegation scope. You mean that from now
> on it is up to the rockchip maintainer?
> I am fine with it either way.

Yes, I meant the rockchip maintainer. I'm only a lowly bindings
maintainer, without any knowledge of rockchip specfics or the type of
boards we're talking about being broken here. Someone has to make a
judgement call about which "no property" behaviour is used going forward
and I don't want that to be me!

> I just wanted to point out that maybe we don't have to set a pulldown
> value after all. And that then all boards will be fine as before
> setting the pulldown explicitly was introduced.

By "all boards will be fine" you mean "all boards that expected the
kernel didn't touch this bit will be fine". The boards that need the
kernel to set this bit because it {comes out of reset,is set by firmware}
incorrectly are going to need a property added if we revert the default
behaviour to not touching the bit.

> In fact I am more eager to get this fixed be it by adding a enable-
> pulldown property to the board definitions, than to change the current
> behavior.
> Just wanted to sort out if that was not the wrong way to fix this
> issue. (ie if adding a setting on most boards was wrong).

> During more than 2 years, I tried various patches and discussed on
> forums about the HS400 breakage. I had bisected the regulator init
> issue in the 5.10 branch. Sadly it took so much time for this issue to
> be understood that when the force pulldown to disable commit was
> introduced downstream before the first issue go fixed.
> This only made the matter worse because when one fixed the double
> regulator init issue HS400 was still broken, this time because the
> pulldown was forced to disable. But nobody noticed this commit that
> forced a default pulldown state (that was older than the regulator
> commit from 5.13 backported to the 5.10 stable branch commit, but that
> reached downstream later due to not having been backported to 5.10 from
> 5.11).
> Otherwise we would have emailed immeditaly.
> Bisecting was only able to catch the first breakage (as it was only
> fixed after the second breakage was introduced).
> 
> Maybe the problem is that me and others did not complained to the
> kernel upstream ML because we were using heavily patched downstream
> kernels (like most if not all downstream ARM kernels). So sadly, the
> forums from back then are filled with complaints but nothing seemed to
> have reached the Linux ML.

Aye, and all I can really say there is to buy boards from a vendor that
doesn't use some horribly hacked downstream kernel, which I know is
clearly an unsatisfactory suggestion. That said, we probably should have
caught that the new default behaviour when the changes were made was not
the default before. There was only one DT maintainer then though, and
things just slip by :/

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Heiko Stübner]

Am Donnerstag, 11. April 2024, 17:42:24 CEST schrieb Conor Dooley:
> On Wed, Apr 10, 2024 at 08:28:57PM +0200, Alban Browaeys wrote:
> > Le jeudi 28 mars 2024 à 18:01 +0000, Conor Dooley a écrit :
> > > On Thu, Mar 28, 2024 at 06:00:03PM +0100, Alban Browaeys wrote:
> > > > Le mardi 26 mars 2024 à 19:46 +0000, Conor Dooley a écrit :
> > > > > On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via
> > > > > B4
> > > > > Relay wrote:
> > > > > > From: Folker Schwesinger <dev@folker-schwesinger.de>
> > > > > > -	if (of_property_read_bool(dev->of_node,
> > > > > > "rockchip,enable-
> > > > > > strobe-pulldown"))
> > > > > > -		rk_phy->enable_strobe_pulldown =
> > > > > > PHYCTRL_REN_STRB_ENABLE;
> > > > > > +	if (of_property_read_bool(dev->of_node,
> > > > > > "rockchip,disable-
> > > > > > strobe-pulldown"))
> > > > > > +		rk_phy->enable_strobe_pulldown =
> > > > > > PHYCTRL_REN_STRB_DISABLE;
> > > > > 
> > > > > Unfortunately you cannot do this.
> > > > > Previously no property at all meant disabled and a property was
> > > > > required
> > > > > to enable it. With this change the absence of a property means
> > > > > that
> > > > > it
> > > > > will be enabled.
> > > > > An old devicetree is that wanted this to be disabled would have
> > > > > no
> > > > > property and will now end up with it enabled. This is an ABI
> > > > > break
> > > > > and is
> > > > > clearly not backwards compatible, that's a NAK unless it is
> > > > > demonstrable
> > > > > that noone actually wants to disable it at all.
> > > > 
> > > > 
> > > > But the patch that introduced the new default to disable the
> > > > pulldown
> > > > explicitely introduced a regression for at least 4 boards.
> > > > It took time to sort out that the default to disable pulldown was
> > > > the
> > > > culprit but still.
> > > > Will we carry this new behavor that breaks the default design for
> > > > rk3399 because since the regression was introduced new board
> > > > definition
> > > > might have expceted this new behavior.
> > > > 
> > > > Could the best option be to revert to énot set a default
> > > > enable/disable
> > > > pulldown" (as before the commit that introduced the regression) and
> > > > allow one to force the pulldown via the enable/disable pulldown
> > > > property?
> > > > I mean the commit that introduced a default value for the pulldown
> > > > did
> > > > not seem to be about fixing anything. But it broke a lot. ANd it
> > > > was
> > > > really really hard to find the description of this commit to
> > > > understand
> > > > that one had to enable pulldown to restore hs400.
> > > > 
> > > > In more than 3 years, only one board maintainer noticed that this
> > > > property was required to get back HS400  and thanks to a user
> > > > telling
> > > > me that this board was working I found from this board that this
> > > > property was "missing" from most board definitions (while it was
> > > > not
> > > > required before).
> > > > 
> > > > 
> > > > I am all for not breaking ABI. But what about not reverting a patch
> > > > that already broke ABI because this patch introduced a new ABI that
> > > > we
> > > > don't want to break?
> > > > I mean shouldn't a new commit with a new ABI that regressed the
> > > > kernel
> > > > be reverted?
> > > 
> > > I think I said it after OP replied to me yesterday, but this is a
> > > pretty
> > > shitty situation in that the original default picked for the property
> > > was actually incorrect. Given it's been like this for four years
> > > before
> > > anyone noticed, and others probably depend on the current behaviour,
> > > that's hard to justify.
> > > 
> > 
> > A lot of people noticed fast that HS400 was broken in the 5.10 branch
> > but due to another commit (more later, ie double regulator init that
> > messed up emmc) this second breakage was not detected. But mostly
> > downstream. And most if not all rk3399 boards in Armbian had HS400
> > disabled.
> > 
> > 
> > It took 3 years to detect that HS400 was broken on a few boards like
> > Rock Pi4 in the upstream kernel. Any might still be broken.
> > I would not count on the fact that keeping the current behavior equals
> > no more broken boards.
> > 
> > From the previous exchanges the boards that requires the pulldown to be
> > disabled seems well known.
> > 
> > Though I am fine with adding a property to set enable pulldown to any
> > board definition file where that is required.
> > 
> > Only I do not believe keeping the statu quo equal everything works
> > because it has been 3 years.
> 
> FWIW, I didn't say this. Clearly if that was the case, this patch would
> never have arrived.
> 
> > In fact this commit reached the downstream kernels way later. Any
> > stayed with the 5.10 branch for years.
> > 
> > But on the other side the disable pulldown by default is alraedy in
> > stable/linux-rolling-lts .
> > 
> > > > Mind fixing the initial regression 8b5c2b45b8f0 "phy: rockchip: set
> > > > pulldown for strobe line in dts" does not necessarily mean changing
> > > > the
> > > > default to the opposite value but could also be reverting to not
> > > > setting a default.
> > > 
> > > That's also problematic, as the only way to do this is make setting
> > > one of the enabled or disabled properties required, which is also an
> > > ABI
> > > break, since you'd then be rejecting probe if one is not present.
> > 
> > 
> > I don't understand.
> > How reverting to not set either pulldown enabled or disabled by default
> > force all board to set either enabled or disabled.
> > I was telling about making the pulldown set by kernel optional be it
> > enabled or disabled to revert to the previous behavior. 
> > 
> > I mean before the patch to set a default pulldown value (to disabled)
> > there were no forced value.
> 
> Ah, maybe I misunderstood what the code originally did. Did the original
> code leave the bit however the bootloader or reset value had left it?
> In that case, probe wouldn't be rejected and you'd not have the sort of
> issue that I mentioned above.
> 
> > > > Though I don't know if there are pros to setting a default.
> > > 
> > > What you probably have to weigh up is the cons of each side. If what
> > > you
> > > lose is HS400 mode with what's in the kernel right now but switching
> > > to
> > > what's been proposed would entirely break some boards, I know which
> > > I think the lesser of two evils is.
> > 
> > More boards (even if not the most wide spread it seems) are broken by
> > the current behavior.
> > 
> > I agree that only HS400 is broken by keeping the status quo. But as far
> > as I understand only HS400 will be broken either way.
> > Be that by keeping the current disable pulldown which break the boards
> > based on the rockchip default design or the boards that are non-
> > standard or have a broken design.
> > Both case this lead to data corruption on boot to eMMC.
> > 
> > The only pro of keeping the current value the default is that most
> > board broken by the new default introduced in 2020 "might" already be
> > fixed (but that is just a guess).

which I guess are the least stale boards too.

> > > It's probably up to the platform maintainer to weigh in at this
> > > point.
> > 
> > I am not knowledged into the delegation scope. You mean that from now
> > on it is up to the rockchip maintainer?
> > I am fine with it either way.
> 
> Yes, I meant the rockchip maintainer. I'm only a lowly bindings
> maintainer, without any knowledge of rockchip specfics or the type of
> boards we're talking about being broken here. Someone has to make a
> judgement call about which "no property" behaviour is used going forward
> and I don't want that to be me!

I'm somehow all for not changing defaults again.

I think in the past there was a similar example in some other kernel part,
where some change broke the ABI, but meanwhile another ABI depended
on the changed behaviour, so a revert was not possible.

I think it's somewhat similar here. If the change has been in the kernel
for 3-4 years now, I do think that ship has sailed somehow.

As was said above, board introduced since 2020 might already be fixed
and essentially for boards that weren't, it does look like these didn't run
a mainline kernel for like 4 years now.

So if it comes down to deciding who to keep working, I'm more in favor of
those that did run on mainline in the years since.


Though not sure if I understood all the details here yet.


Heiko

> 
> > I just wanted to point out that maybe we don't have to set a pulldown
> > value after all. And that then all boards will be fine as before
> > setting the pulldown explicitly was introduced.
> 
> By "all boards will be fine" you mean "all boards that expected the
> kernel didn't touch this bit will be fine". The boards that need the
> kernel to set this bit because it {comes out of reset,is set by firmware}
> incorrectly are going to need a property added if we revert the default
> behaviour to not touching the bit.
> 
> > In fact I am more eager to get this fixed be it by adding a enable-
> > pulldown property to the board definitions, than to change the current
> > behavior.
> > Just wanted to sort out if that was not the wrong way to fix this
> > issue. (ie if adding a setting on most boards was wrong).
> 
> > During more than 2 years, I tried various patches and discussed on
> > forums about the HS400 breakage. I had bisected the regulator init
> > issue in the 5.10 branch. Sadly it took so much time for this issue to
> > be understood that when the force pulldown to disable commit was
> > introduced downstream before the first issue go fixed.
> > This only made the matter worse because when one fixed the double
> > regulator init issue HS400 was still broken, this time because the
> > pulldown was forced to disable. But nobody noticed this commit that
> > forced a default pulldown state (that was older than the regulator
> > commit from 5.13 backported to the 5.10 stable branch commit, but that
> > reached downstream later due to not having been backported to 5.10 from
> > 5.11).
> > Otherwise we would have emailed immeditaly.
> > Bisecting was only able to catch the first breakage (as it was only
> > fixed after the second breakage was introduced).
> > 
> > Maybe the problem is that me and others did not complained to the
> > kernel upstream ML because we were using heavily patched downstream
> > kernels (like most if not all downstream ARM kernels). So sadly, the
> > forums from back then are filled with complaints but nothing seemed to
> > have reached the Linux ML.
> 
> Aye, and all I can really say there is to buy boards from a vendor that
> doesn't use some horribly hacked downstream kernel, which I know is
> clearly an unsatisfactory suggestion. That said, we probably should have
> caught that the new default behaviour when the changes were made was not
> the default before. There was only one DT maintainer then though, and
> things just slip by :/
> 





-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Heiko Stübner]

Am Donnerstag, 11. April 2024, 17:42:24 CEST schrieb Conor Dooley:
> On Wed, Apr 10, 2024 at 08:28:57PM +0200, Alban Browaeys wrote:
> > Le jeudi 28 mars 2024 à 18:01 +0000, Conor Dooley a écrit :
> > > On Thu, Mar 28, 2024 at 06:00:03PM +0100, Alban Browaeys wrote:
> > > > Le mardi 26 mars 2024 à 19:46 +0000, Conor Dooley a écrit :
> > > > > On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via
> > > > > B4
> > > > > Relay wrote:
> > > > > > From: Folker Schwesinger <dev@folker-schwesinger.de>
> > > > > > -	if (of_property_read_bool(dev->of_node,
> > > > > > "rockchip,enable-
> > > > > > strobe-pulldown"))
> > > > > > -		rk_phy->enable_strobe_pulldown =
> > > > > > PHYCTRL_REN_STRB_ENABLE;
> > > > > > +	if (of_property_read_bool(dev->of_node,
> > > > > > "rockchip,disable-
> > > > > > strobe-pulldown"))
> > > > > > +		rk_phy->enable_strobe_pulldown =
> > > > > > PHYCTRL_REN_STRB_DISABLE;
> > > > > 
> > > > > Unfortunately you cannot do this.
> > > > > Previously no property at all meant disabled and a property was
> > > > > required
> > > > > to enable it. With this change the absence of a property means
> > > > > that
> > > > > it
> > > > > will be enabled.
> > > > > An old devicetree is that wanted this to be disabled would have
> > > > > no
> > > > > property and will now end up with it enabled. This is an ABI
> > > > > break
> > > > > and is
> > > > > clearly not backwards compatible, that's a NAK unless it is
> > > > > demonstrable
> > > > > that noone actually wants to disable it at all.
> > > > 
> > > > 
> > > > But the patch that introduced the new default to disable the
> > > > pulldown
> > > > explicitely introduced a regression for at least 4 boards.
> > > > It took time to sort out that the default to disable pulldown was
> > > > the
> > > > culprit but still.
> > > > Will we carry this new behavor that breaks the default design for
> > > > rk3399 because since the regression was introduced new board
> > > > definition
> > > > might have expceted this new behavior.
> > > > 
> > > > Could the best option be to revert to énot set a default
> > > > enable/disable
> > > > pulldown" (as before the commit that introduced the regression) and
> > > > allow one to force the pulldown via the enable/disable pulldown
> > > > property?
> > > > I mean the commit that introduced a default value for the pulldown
> > > > did
> > > > not seem to be about fixing anything. But it broke a lot. ANd it
> > > > was
> > > > really really hard to find the description of this commit to
> > > > understand
> > > > that one had to enable pulldown to restore hs400.
> > > > 
> > > > In more than 3 years, only one board maintainer noticed that this
> > > > property was required to get back HS400  and thanks to a user
> > > > telling
> > > > me that this board was working I found from this board that this
> > > > property was "missing" from most board definitions (while it was
> > > > not
> > > > required before).
> > > > 
> > > > 
> > > > I am all for not breaking ABI. But what about not reverting a patch
> > > > that already broke ABI because this patch introduced a new ABI that
> > > > we
> > > > don't want to break?
> > > > I mean shouldn't a new commit with a new ABI that regressed the
> > > > kernel
> > > > be reverted?
> > > 
> > > I think I said it after OP replied to me yesterday, but this is a
> > > pretty
> > > shitty situation in that the original default picked for the property
> > > was actually incorrect. Given it's been like this for four years
> > > before
> > > anyone noticed, and others probably depend on the current behaviour,
> > > that's hard to justify.
> > > 
> > 
> > A lot of people noticed fast that HS400 was broken in the 5.10 branch
> > but due to another commit (more later, ie double regulator init that
> > messed up emmc) this second breakage was not detected. But mostly
> > downstream. And most if not all rk3399 boards in Armbian had HS400
> > disabled.
> > 
> > 
> > It took 3 years to detect that HS400 was broken on a few boards like
> > Rock Pi4 in the upstream kernel. Any might still be broken.
> > I would not count on the fact that keeping the current behavior equals
> > no more broken boards.
> > 
> > From the previous exchanges the boards that requires the pulldown to be
> > disabled seems well known.
> > 
> > Though I am fine with adding a property to set enable pulldown to any
> > board definition file where that is required.
> > 
> > Only I do not believe keeping the statu quo equal everything works
> > because it has been 3 years.
> 
> FWIW, I didn't say this. Clearly if that was the case, this patch would
> never have arrived.
> 
> > In fact this commit reached the downstream kernels way later. Any
> > stayed with the 5.10 branch for years.
> > 
> > But on the other side the disable pulldown by default is alraedy in
> > stable/linux-rolling-lts .
> > 
> > > > Mind fixing the initial regression 8b5c2b45b8f0 "phy: rockchip: set
> > > > pulldown for strobe line in dts" does not necessarily mean changing
> > > > the
> > > > default to the opposite value but could also be reverting to not
> > > > setting a default.
> > > 
> > > That's also problematic, as the only way to do this is make setting
> > > one of the enabled or disabled properties required, which is also an
> > > ABI
> > > break, since you'd then be rejecting probe if one is not present.
> > 
> > 
> > I don't understand.
> > How reverting to not set either pulldown enabled or disabled by default
> > force all board to set either enabled or disabled.
> > I was telling about making the pulldown set by kernel optional be it
> > enabled or disabled to revert to the previous behavior. 
> > 
> > I mean before the patch to set a default pulldown value (to disabled)
> > there were no forced value.
> 
> Ah, maybe I misunderstood what the code originally did. Did the original
> code leave the bit however the bootloader or reset value had left it?
> In that case, probe wouldn't be rejected and you'd not have the sort of
> issue that I mentioned above.
> 
> > > > Though I don't know if there are pros to setting a default.
> > > 
> > > What you probably have to weigh up is the cons of each side. If what
> > > you
> > > lose is HS400 mode with what's in the kernel right now but switching
> > > to
> > > what's been proposed would entirely break some boards, I know which
> > > I think the lesser of two evils is.
> > 
> > More boards (even if not the most wide spread it seems) are broken by
> > the current behavior.
> > 
> > I agree that only HS400 is broken by keeping the status quo. But as far
> > as I understand only HS400 will be broken either way.
> > Be that by keeping the current disable pulldown which break the boards
> > based on the rockchip default design or the boards that are non-
> > standard or have a broken design.
> > Both case this lead to data corruption on boot to eMMC.
> > 
> > The only pro of keeping the current value the default is that most
> > board broken by the new default introduced in 2020 "might" already be
> > fixed (but that is just a guess).

which I guess are the least stale boards too.

> > > It's probably up to the platform maintainer to weigh in at this
> > > point.
> > 
> > I am not knowledged into the delegation scope. You mean that from now
> > on it is up to the rockchip maintainer?
> > I am fine with it either way.
> 
> Yes, I meant the rockchip maintainer. I'm only a lowly bindings
> maintainer, without any knowledge of rockchip specfics or the type of
> boards we're talking about being broken here. Someone has to make a
> judgement call about which "no property" behaviour is used going forward
> and I don't want that to be me!

I'm somehow all for not changing defaults again.

I think in the past there was a similar example in some other kernel part,
where some change broke the ABI, but meanwhile another ABI depended
on the changed behaviour, so a revert was not possible.

I think it's somewhat similar here. If the change has been in the kernel
for 3-4 years now, I do think that ship has sailed somehow.

As was said above, board introduced since 2020 might already be fixed
and essentially for boards that weren't, it does look like these didn't run
a mainline kernel for like 4 years now.

So if it comes down to deciding who to keep working, I'm more in favor of
those that did run on mainline in the years since.


Though not sure if I understood all the details here yet.


Heiko

> 
> > I just wanted to point out that maybe we don't have to set a pulldown
> > value after all. And that then all boards will be fine as before
> > setting the pulldown explicitly was introduced.
> 
> By "all boards will be fine" you mean "all boards that expected the
> kernel didn't touch this bit will be fine". The boards that need the
> kernel to set this bit because it {comes out of reset,is set by firmware}
> incorrectly are going to need a property added if we revert the default
> behaviour to not touching the bit.
> 
> > In fact I am more eager to get this fixed be it by adding a enable-
> > pulldown property to the board definitions, than to change the current
> > behavior.
> > Just wanted to sort out if that was not the wrong way to fix this
> > issue. (ie if adding a setting on most boards was wrong).
> 
> > During more than 2 years, I tried various patches and discussed on
> > forums about the HS400 breakage. I had bisected the regulator init
> > issue in the 5.10 branch. Sadly it took so much time for this issue to
> > be understood that when the force pulldown to disable commit was
> > introduced downstream before the first issue go fixed.
> > This only made the matter worse because when one fixed the double
> > regulator init issue HS400 was still broken, this time because the
> > pulldown was forced to disable. But nobody noticed this commit that
> > forced a default pulldown state (that was older than the regulator
> > commit from 5.13 backported to the 5.10 stable branch commit, but that
> > reached downstream later due to not having been backported to 5.10 from
> > 5.11).
> > Otherwise we would have emailed immeditaly.
> > Bisecting was only able to catch the first breakage (as it was only
> > fixed after the second breakage was introduced).
> > 
> > Maybe the problem is that me and others did not complained to the
> > kernel upstream ML because we were using heavily patched downstream
> > kernels (like most if not all downstream ARM kernels). So sadly, the
> > forums from back then are filled with complaints but nothing seemed to
> > have reached the Linux ML.
> 
> Aye, and all I can really say there is to buy boards from a vendor that
> doesn't use some horribly hacked downstream kernel, which I know is
> clearly an unsatisfactory suggestion. That said, we probably should have
> caught that the new default behaviour when the changes were made was not
> the default before. There was only one DT maintainer then though, and
> things just slip by :/
> 

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Heiko Stübner]

Am Donnerstag, 11. April 2024, 17:42:24 CEST schrieb Conor Dooley:
> On Wed, Apr 10, 2024 at 08:28:57PM +0200, Alban Browaeys wrote:
> > Le jeudi 28 mars 2024 à 18:01 +0000, Conor Dooley a écrit :
> > > On Thu, Mar 28, 2024 at 06:00:03PM +0100, Alban Browaeys wrote:
> > > > Le mardi 26 mars 2024 à 19:46 +0000, Conor Dooley a écrit :
> > > > > On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via
> > > > > B4
> > > > > Relay wrote:
> > > > > > From: Folker Schwesinger <dev@folker-schwesinger.de>
> > > > > > -	if (of_property_read_bool(dev->of_node,
> > > > > > "rockchip,enable-
> > > > > > strobe-pulldown"))
> > > > > > -		rk_phy->enable_strobe_pulldown =
> > > > > > PHYCTRL_REN_STRB_ENABLE;
> > > > > > +	if (of_property_read_bool(dev->of_node,
> > > > > > "rockchip,disable-
> > > > > > strobe-pulldown"))
> > > > > > +		rk_phy->enable_strobe_pulldown =
> > > > > > PHYCTRL_REN_STRB_DISABLE;
> > > > > 
> > > > > Unfortunately you cannot do this.
> > > > > Previously no property at all meant disabled and a property was
> > > > > required
> > > > > to enable it. With this change the absence of a property means
> > > > > that
> > > > > it
> > > > > will be enabled.
> > > > > An old devicetree is that wanted this to be disabled would have
> > > > > no
> > > > > property and will now end up with it enabled. This is an ABI
> > > > > break
> > > > > and is
> > > > > clearly not backwards compatible, that's a NAK unless it is
> > > > > demonstrable
> > > > > that noone actually wants to disable it at all.
> > > > 
> > > > 
> > > > But the patch that introduced the new default to disable the
> > > > pulldown
> > > > explicitely introduced a regression for at least 4 boards.
> > > > It took time to sort out that the default to disable pulldown was
> > > > the
> > > > culprit but still.
> > > > Will we carry this new behavor that breaks the default design for
> > > > rk3399 because since the regression was introduced new board
> > > > definition
> > > > might have expceted this new behavior.
> > > > 
> > > > Could the best option be to revert to énot set a default
> > > > enable/disable
> > > > pulldown" (as before the commit that introduced the regression) and
> > > > allow one to force the pulldown via the enable/disable pulldown
> > > > property?
> > > > I mean the commit that introduced a default value for the pulldown
> > > > did
> > > > not seem to be about fixing anything. But it broke a lot. ANd it
> > > > was
> > > > really really hard to find the description of this commit to
> > > > understand
> > > > that one had to enable pulldown to restore hs400.
> > > > 
> > > > In more than 3 years, only one board maintainer noticed that this
> > > > property was required to get back HS400  and thanks to a user
> > > > telling
> > > > me that this board was working I found from this board that this
> > > > property was "missing" from most board definitions (while it was
> > > > not
> > > > required before).
> > > > 
> > > > 
> > > > I am all for not breaking ABI. But what about not reverting a patch
> > > > that already broke ABI because this patch introduced a new ABI that
> > > > we
> > > > don't want to break?
> > > > I mean shouldn't a new commit with a new ABI that regressed the
> > > > kernel
> > > > be reverted?
> > > 
> > > I think I said it after OP replied to me yesterday, but this is a
> > > pretty
> > > shitty situation in that the original default picked for the property
> > > was actually incorrect. Given it's been like this for four years
> > > before
> > > anyone noticed, and others probably depend on the current behaviour,
> > > that's hard to justify.
> > > 
> > 
> > A lot of people noticed fast that HS400 was broken in the 5.10 branch
> > but due to another commit (more later, ie double regulator init that
> > messed up emmc) this second breakage was not detected. But mostly
> > downstream. And most if not all rk3399 boards in Armbian had HS400
> > disabled.
> > 
> > 
> > It took 3 years to detect that HS400 was broken on a few boards like
> > Rock Pi4 in the upstream kernel. Any might still be broken.
> > I would not count on the fact that keeping the current behavior equals
> > no more broken boards.
> > 
> > From the previous exchanges the boards that requires the pulldown to be
> > disabled seems well known.
> > 
> > Though I am fine with adding a property to set enable pulldown to any
> > board definition file where that is required.
> > 
> > Only I do not believe keeping the statu quo equal everything works
> > because it has been 3 years.
> 
> FWIW, I didn't say this. Clearly if that was the case, this patch would
> never have arrived.
> 
> > In fact this commit reached the downstream kernels way later. Any
> > stayed with the 5.10 branch for years.
> > 
> > But on the other side the disable pulldown by default is alraedy in
> > stable/linux-rolling-lts .
> > 
> > > > Mind fixing the initial regression 8b5c2b45b8f0 "phy: rockchip: set
> > > > pulldown for strobe line in dts" does not necessarily mean changing
> > > > the
> > > > default to the opposite value but could also be reverting to not
> > > > setting a default.
> > > 
> > > That's also problematic, as the only way to do this is make setting
> > > one of the enabled or disabled properties required, which is also an
> > > ABI
> > > break, since you'd then be rejecting probe if one is not present.
> > 
> > 
> > I don't understand.
> > How reverting to not set either pulldown enabled or disabled by default
> > force all board to set either enabled or disabled.
> > I was telling about making the pulldown set by kernel optional be it
> > enabled or disabled to revert to the previous behavior. 
> > 
> > I mean before the patch to set a default pulldown value (to disabled)
> > there were no forced value.
> 
> Ah, maybe I misunderstood what the code originally did. Did the original
> code leave the bit however the bootloader or reset value had left it?
> In that case, probe wouldn't be rejected and you'd not have the sort of
> issue that I mentioned above.
> 
> > > > Though I don't know if there are pros to setting a default.
> > > 
> > > What you probably have to weigh up is the cons of each side. If what
> > > you
> > > lose is HS400 mode with what's in the kernel right now but switching
> > > to
> > > what's been proposed would entirely break some boards, I know which
> > > I think the lesser of two evils is.
> > 
> > More boards (even if not the most wide spread it seems) are broken by
> > the current behavior.
> > 
> > I agree that only HS400 is broken by keeping the status quo. But as far
> > as I understand only HS400 will be broken either way.
> > Be that by keeping the current disable pulldown which break the boards
> > based on the rockchip default design or the boards that are non-
> > standard or have a broken design.
> > Both case this lead to data corruption on boot to eMMC.
> > 
> > The only pro of keeping the current value the default is that most
> > board broken by the new default introduced in 2020 "might" already be
> > fixed (but that is just a guess).

which I guess are the least stale boards too.

> > > It's probably up to the platform maintainer to weigh in at this
> > > point.
> > 
> > I am not knowledged into the delegation scope. You mean that from now
> > on it is up to the rockchip maintainer?
> > I am fine with it either way.
> 
> Yes, I meant the rockchip maintainer. I'm only a lowly bindings
> maintainer, without any knowledge of rockchip specfics or the type of
> boards we're talking about being broken here. Someone has to make a
> judgement call about which "no property" behaviour is used going forward
> and I don't want that to be me!

I'm somehow all for not changing defaults again.

I think in the past there was a similar example in some other kernel part,
where some change broke the ABI, but meanwhile another ABI depended
on the changed behaviour, so a revert was not possible.

I think it's somewhat similar here. If the change has been in the kernel
for 3-4 years now, I do think that ship has sailed somehow.

As was said above, board introduced since 2020 might already be fixed
and essentially for boards that weren't, it does look like these didn't run
a mainline kernel for like 4 years now.

So if it comes down to deciding who to keep working, I'm more in favor of
those that did run on mainline in the years since.


Though not sure if I understood all the details here yet.


Heiko

> 
> > I just wanted to point out that maybe we don't have to set a pulldown
> > value after all. And that then all boards will be fine as before
> > setting the pulldown explicitly was introduced.
> 
> By "all boards will be fine" you mean "all boards that expected the
> kernel didn't touch this bit will be fine". The boards that need the
> kernel to set this bit because it {comes out of reset,is set by firmware}
> incorrectly are going to need a property added if we revert the default
> behaviour to not touching the bit.
> 
> > In fact I am more eager to get this fixed be it by adding a enable-
> > pulldown property to the board definitions, than to change the current
> > behavior.
> > Just wanted to sort out if that was not the wrong way to fix this
> > issue. (ie if adding a setting on most boards was wrong).
> 
> > During more than 2 years, I tried various patches and discussed on
> > forums about the HS400 breakage. I had bisected the regulator init
> > issue in the 5.10 branch. Sadly it took so much time for this issue to
> > be understood that when the force pulldown to disable commit was
> > introduced downstream before the first issue go fixed.
> > This only made the matter worse because when one fixed the double
> > regulator init issue HS400 was still broken, this time because the
> > pulldown was forced to disable. But nobody noticed this commit that
> > forced a default pulldown state (that was older than the regulator
> > commit from 5.13 backported to the 5.10 stable branch commit, but that
> > reached downstream later due to not having been backported to 5.10 from
> > 5.11).
> > Otherwise we would have emailed immeditaly.
> > Bisecting was only able to catch the first breakage (as it was only
> > fixed after the second breakage was introduced).
> > 
> > Maybe the problem is that me and others did not complained to the
> > kernel upstream ML because we were using heavily patched downstream
> > kernels (like most if not all downstream ARM kernels). So sadly, the
> > forums from back then are filled with complaints but nothing seemed to
> > have reached the Linux ML.
> 
> Aye, and all I can really say there is to buy boards from a vendor that
> doesn't use some horribly hacked downstream kernel, which I know is
> clearly an unsatisfactory suggestion. That said, we probably should have
> caught that the new default behaviour when the changes were made was not
> the default before. There was only one DT maintainer then though, and
> things just slip by :/
> 





_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 1/3] phy: rockchip: emmc: Enable pulldown for strobe line

[Heiko Stübner]

Am Donnerstag, 11. April 2024, 17:42:24 CEST schrieb Conor Dooley:
> On Wed, Apr 10, 2024 at 08:28:57PM +0200, Alban Browaeys wrote:
> > Le jeudi 28 mars 2024 à 18:01 +0000, Conor Dooley a écrit :
> > > On Thu, Mar 28, 2024 at 06:00:03PM +0100, Alban Browaeys wrote:
> > > > Le mardi 26 mars 2024 à 19:46 +0000, Conor Dooley a écrit :
> > > > > On Tue, Mar 26, 2024 at 07:54:35PM +0100, Folker Schwesinger via
> > > > > B4
> > > > > Relay wrote:
> > > > > > From: Folker Schwesinger <dev@folker-schwesinger.de>
> > > > > > -	if (of_property_read_bool(dev->of_node,
> > > > > > "rockchip,enable-
> > > > > > strobe-pulldown"))
> > > > > > -		rk_phy->enable_strobe_pulldown =
> > > > > > PHYCTRL_REN_STRB_ENABLE;
> > > > > > +	if (of_property_read_bool(dev->of_node,
> > > > > > "rockchip,disable-
> > > > > > strobe-pulldown"))
> > > > > > +		rk_phy->enable_strobe_pulldown =
> > > > > > PHYCTRL_REN_STRB_DISABLE;
> > > > > 
> > > > > Unfortunately you cannot do this.
> > > > > Previously no property at all meant disabled and a property was
> > > > > required
> > > > > to enable it. With this change the absence of a property means
> > > > > that
> > > > > it
> > > > > will be enabled.
> > > > > An old devicetree is that wanted this to be disabled would have
> > > > > no
> > > > > property and will now end up with it enabled. This is an ABI
> > > > > break
> > > > > and is
> > > > > clearly not backwards compatible, that's a NAK unless it is
> > > > > demonstrable
> > > > > that noone actually wants to disable it at all.
> > > > 
> > > > 
> > > > But the patch that introduced the new default to disable the
> > > > pulldown
> > > > explicitely introduced a regression for at least 4 boards.
> > > > It took time to sort out that the default to disable pulldown was
> > > > the
> > > > culprit but still.
> > > > Will we carry this new behavor that breaks the default design for
> > > > rk3399 because since the regression was introduced new board
> > > > definition
> > > > might have expceted this new behavior.
> > > > 
> > > > Could the best option be to revert to énot set a default
> > > > enable/disable
> > > > pulldown" (as before the commit that introduced the regression) and
> > > > allow one to force the pulldown via the enable/disable pulldown
> > > > property?
> > > > I mean the commit that introduced a default value for the pulldown
> > > > did
> > > > not seem to be about fixing anything. But it broke a lot. ANd it
> > > > was
> > > > really really hard to find the description of this commit to
> > > > understand
> > > > that one had to enable pulldown to restore hs400.
> > > > 
> > > > In more than 3 years, only one board maintainer noticed that this
> > > > property was required to get back HS400  and thanks to a user
> > > > telling
> > > > me that this board was working I found from this board that this
> > > > property was "missing" from most board definitions (while it was
> > > > not
> > > > required before).
> > > > 
> > > > 
> > > > I am all for not breaking ABI. But what about not reverting a patch
> > > > that already broke ABI because this patch introduced a new ABI that
> > > > we
> > > > don't want to break?
> > > > I mean shouldn't a new commit with a new ABI that regressed the
> > > > kernel
> > > > be reverted?
> > > 
> > > I think I said it after OP replied to me yesterday, but this is a
> > > pretty
> > > shitty situation in that the original default picked for the property
> > > was actually incorrect. Given it's been like this for four years
> > > before
> > > anyone noticed, and others probably depend on the current behaviour,
> > > that's hard to justify.
> > > 
> > 
> > A lot of people noticed fast that HS400 was broken in the 5.10 branch
> > but due to another commit (more later, ie double regulator init that
> > messed up emmc) this second breakage was not detected. But mostly
> > downstream. And most if not all rk3399 boards in Armbian had HS400
> > disabled.
> > 
> > 
> > It took 3 years to detect that HS400 was broken on a few boards like
> > Rock Pi4 in the upstream kernel. Any might still be broken.
> > I would not count on the fact that keeping the current behavior equals
> > no more broken boards.
> > 
> > From the previous exchanges the boards that requires the pulldown to be
> > disabled seems well known.
> > 
> > Though I am fine with adding a property to set enable pulldown to any
> > board definition file where that is required.
> > 
> > Only I do not believe keeping the statu quo equal everything works
> > because it has been 3 years.
> 
> FWIW, I didn't say this. Clearly if that was the case, this patch would
> never have arrived.
> 
> > In fact this commit reached the downstream kernels way later. Any
> > stayed with the 5.10 branch for years.
> > 
> > But on the other side the disable pulldown by default is alraedy in
> > stable/linux-rolling-lts .
> > 
> > > > Mind fixing the initial regression 8b5c2b45b8f0 "phy: rockchip: set
> > > > pulldown for strobe line in dts" does not necessarily mean changing
> > > > the
> > > > default to the opposite value but could also be reverting to not
> > > > setting a default.
> > > 
> > > That's also problematic, as the only way to do this is make setting
> > > one of the enabled or disabled properties required, which is also an
> > > ABI
> > > break, since you'd then be rejecting probe if one is not present.
> > 
> > 
> > I don't understand.
> > How reverting to not set either pulldown enabled or disabled by default
> > force all board to set either enabled or disabled.
> > I was telling about making the pulldown set by kernel optional be it
> > enabled or disabled to revert to the previous behavior. 
> > 
> > I mean before the patch to set a default pulldown value (to disabled)
> > there were no forced value.
> 
> Ah, maybe I misunderstood what the code originally did. Did the original
> code leave the bit however the bootloader or reset value had left it?
> In that case, probe wouldn't be rejected and you'd not have the sort of
> issue that I mentioned above.
> 
> > > > Though I don't know if there are pros to setting a default.
> > > 
> > > What you probably have to weigh up is the cons of each side. If what
> > > you
> > > lose is HS400 mode with what's in the kernel right now but switching
> > > to
> > > what's been proposed would entirely break some boards, I know which
> > > I think the lesser of two evils is.
> > 
> > More boards (even if not the most wide spread it seems) are broken by
> > the current behavior.
> > 
> > I agree that only HS400 is broken by keeping the status quo. But as far
> > as I understand only HS400 will be broken either way.
> > Be that by keeping the current disable pulldown which break the boards
> > based on the rockchip default design or the boards that are non-
> > standard or have a broken design.
> > Both case this lead to data corruption on boot to eMMC.
> > 
> > The only pro of keeping the current value the default is that most
> > board broken by the new default introduced in 2020 "might" already be
> > fixed (but that is just a guess).

which I guess are the least stale boards too.

> > > It's probably up to the platform maintainer to weigh in at this
> > > point.
> > 
> > I am not knowledged into the delegation scope. You mean that from now
> > on it is up to the rockchip maintainer?
> > I am fine with it either way.
> 
> Yes, I meant the rockchip maintainer. I'm only a lowly bindings
> maintainer, without any knowledge of rockchip specfics or the type of
> boards we're talking about being broken here. Someone has to make a
> judgement call about which "no property" behaviour is used going forward
> and I don't want that to be me!

I'm somehow all for not changing defaults again.

I think in the past there was a similar example in some other kernel part,
where some change broke the ABI, but meanwhile another ABI depended
on the changed behaviour, so a revert was not possible.

I think it's somewhat similar here. If the change has been in the kernel
for 3-4 years now, I do think that ship has sailed somehow.

As was said above, board introduced since 2020 might already be fixed
and essentially for boards that weren't, it does look like these didn't run
a mainline kernel for like 4 years now.

So if it comes down to deciding who to keep working, I'm more in favor of
those that did run on mainline in the years since.


Though not sure if I understood all the details here yet.


Heiko

> 
> > I just wanted to point out that maybe we don't have to set a pulldown
> > value after all. And that then all boards will be fine as before
> > setting the pulldown explicitly was introduced.
> 
> By "all boards will be fine" you mean "all boards that expected the
> kernel didn't touch this bit will be fine". The boards that need the
> kernel to set this bit because it {comes out of reset,is set by firmware}
> incorrectly are going to need a property added if we revert the default
> behaviour to not touching the bit.
> 
> > In fact I am more eager to get this fixed be it by adding a enable-
> > pulldown property to the board definitions, than to change the current
> > behavior.
> > Just wanted to sort out if that was not the wrong way to fix this
> > issue. (ie if adding a setting on most boards was wrong).
> 
> > During more than 2 years, I tried various patches and discussed on
> > forums about the HS400 breakage. I had bisected the regulator init
> > issue in the 5.10 branch. Sadly it took so much time for this issue to
> > be understood that when the force pulldown to disable commit was
> > introduced downstream before the first issue go fixed.
> > This only made the matter worse because when one fixed the double
> > regulator init issue HS400 was still broken, this time because the
> > pulldown was forced to disable. But nobody noticed this commit that
> > forced a default pulldown state (that was older than the regulator
> > commit from 5.13 backported to the 5.10 stable branch commit, but that
> > reached downstream later due to not having been backported to 5.10 from
> > 5.11).
> > Otherwise we would have emailed immeditaly.
> > Bisecting was only able to catch the first breakage (as it was only
> > fixed after the second breakage was introduced).
> > 
> > Maybe the problem is that me and others did not complained to the
> > kernel upstream ML because we were using heavily patched downstream
> > kernels (like most if not all downstream ARM kernels). So sadly, the
> > forums from back then are filled with complaints but nothing seemed to
> > have reached the Linux ML.
> 
> Aye, and all I can really say there is to buy boards from a vendor that
> doesn't use some horribly hacked downstream kernel, which I know is
> clearly an unsatisfactory suggestion. That said, we probably should have
> caught that the new default behaviour when the changes were made was not
> the default before. There was only one DT maintainer then though, and
> things just slip by :/
> 





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel