[PATCH 0/5] drm/sun4i: Support LVDS on D1s/T113 combo D-PHY

[PATCH 0/5] drm/sun4i: Support LVDS on D1s/T113 combo D-PHY

[Kuba Szczodrzyński]

Some Allwinner chips (notably the D1s/T113 and the A100) have a "combo
MIPI DSI D-PHY" which is required when using single-link LVDS0. The same
PD0..PD9 pins are used for either DSI or LVDS.

Other than having to use the combo D-PHY, LVDS output is configured in
the same way as on older chips.

This series enables the sun6i MIPI D-PHY to also work in LVDS mode. It
is then configured by the LCD TCON, which allows connecting a
single-link LVDS display panel.

Kuba Szczodrzyński (5):
  phy: allwinner: phy-sun6i-mipi-dphy: Support LVDS in combo D-PHY
  drm/sun4i: Support LVDS using MIPI DSI combo D-PHY
  drm/sun4i: Enable LVDS output on sun20i D1s/T113
  riscv: dts: allwinner: d1s-t113: Add D-PHY to TCON LCD0
  riscv: dts: allwinner: d1s-t113: Add LVDS0 pins

 .../boot/dts/allwinner/sunxi-d1s-t113.dtsi    | 10 +++
 drivers/gpu/drm/sun4i/sun4i_tcon.c            | 40 ++++++++++++
 drivers/gpu/drm/sun4i/sun4i_tcon.h            |  6 ++
 drivers/phy/allwinner/phy-sun6i-mipi-dphy.c   | 65 ++++++++++++++++++-
 4 files changed, 119 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH 1/5] phy: allwinner: phy-sun6i-mipi-dphy: Support LVDS in combo D-PHY

[Kuba Szczodrzyński]

Some Allwinner chips (notably the D1s/T113 and the A100) have a "combo
MIPI DSI D-PHY" which is required when using single-link LVDS0.

In this mode, the DSI peripheral is not used and the PHY is not
configured for DSI. Instead, the COMBO_PHY_REGx registers are set to
enable LVDS operation.

Enable the PHY driver to work in LVDS mode on chips with a combo D-PHY.

Signed-off-by: Kuba Szczodrzyński <kuba@szczodrzynski.pl>
---
 drivers/phy/allwinner/phy-sun6i-mipi-dphy.c | 65 ++++++++++++++++++++-
 1 file changed, 63 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c b/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c
index 36eab9527..f958e34da 100644
--- a/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c
+++ b/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c
@@ -166,8 +166,8 @@
 #define SUN50I_COMBO_PHY_REG0_EN_CP		BIT(0)
 
 #define SUN50I_COMBO_PHY_REG1		0x114
-#define SUN50I_COMBO_PHY_REG2_REG_VREF1P6(n)	(((n) & 0x7) << 4)
-#define SUN50I_COMBO_PHY_REG2_REG_VREF0P8(n)	((n) & 0x7)
+#define SUN50I_COMBO_PHY_REG1_REG_VREF1P6(n)	(((n) & 0x7) << 4)
+#define SUN50I_COMBO_PHY_REG1_REG_VREF0P8(n)	((n) & 0x7)
 
 #define SUN50I_COMBO_PHY_REG2		0x118
 #define SUN50I_COMBO_PHY_REG2_HS_STOP_DLY(n)	((n) & 0xff)
@@ -181,7 +181,9 @@ struct sun6i_dphy;
 
 struct sun6i_dphy_variant {
 	void	(*tx_power_on)(struct sun6i_dphy *dphy);
+	void	(*lvds_power_on)(struct sun6i_dphy *dphy);
 	bool	rx_supported;
+	bool	is_combo_dphy;
 };
 
 struct sun6i_dphy {
@@ -222,6 +224,18 @@ static int sun6i_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
 	return 0;
 }
 
+static int sun6i_dphy_set_mode(struct phy *phy, enum phy_mode mode, int submode)
+{
+	struct sun6i_dphy *dphy = phy_get_drvdata(phy);
+
+	if (mode == PHY_MODE_LVDS && !dphy->variant->is_combo_dphy) {
+		/* Not a combo D-PHY: LVDS is not supported */
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static void sun6i_a31_mipi_dphy_tx_power_on(struct sun6i_dphy *dphy)
 {
 	u8 lanes_mask = GENMASK(dphy->config.lanes - 1, 0);
@@ -329,6 +343,37 @@ static void sun50i_a100_mipi_dphy_tx_power_on(struct sun6i_dphy *dphy)
 	udelay(1);
 }
 
+static void sun50i_a100_mipi_dphy_lvds_power_on(struct sun6i_dphy *dphy)
+{
+	regmap_write(dphy->regs, SUN50I_COMBO_PHY_REG1,
+		     SUN50I_COMBO_PHY_REG1_REG_VREF1P6(4) |
+		     SUN50I_COMBO_PHY_REG1_REG_VREF0P8(3));
+
+	regmap_write(dphy->regs, SUN50I_COMBO_PHY_REG0,
+		     SUN50I_COMBO_PHY_REG0_EN_CP);
+	udelay(5);
+	regmap_update_bits(dphy->regs, SUN50I_COMBO_PHY_REG0,
+			   SUN50I_COMBO_PHY_REG0_EN_LVDS,
+			   SUN50I_COMBO_PHY_REG0_EN_LVDS);
+	udelay(5);
+	regmap_update_bits(dphy->regs, SUN50I_COMBO_PHY_REG0,
+			   SUN50I_COMBO_PHY_REG0_EN_COMBOLDO,
+			   SUN50I_COMBO_PHY_REG0_EN_COMBOLDO);
+	udelay(5);
+	regmap_update_bits(dphy->regs, SUN50I_COMBO_PHY_REG0,
+			   SUN50I_COMBO_PHY_REG0_EN_MIPI,
+			   SUN50I_COMBO_PHY_REG0_EN_MIPI);
+
+	regmap_write(dphy->regs, SUN6I_DPHY_ANA4_REG,
+		     SUN6I_DPHY_ANA4_REG_EN_MIPI |
+		     SUN6I_DPHY_ANA4_REG_IB(2));
+	regmap_write(dphy->regs, SUN6I_DPHY_ANA3_REG,
+		     SUN6I_DPHY_ANA3_EN_LDOR |
+		     SUN6I_DPHY_ANA3_EN_LDOD);
+	regmap_write(dphy->regs, SUN6I_DPHY_ANA2_REG, 0);
+	regmap_write(dphy->regs, SUN6I_DPHY_ANA1_REG, 0);
+}
+
 static int sun6i_dphy_tx_power_on(struct sun6i_dphy *dphy)
 {
 	u8 lanes_mask = GENMASK(dphy->config.lanes - 1, 0);
@@ -492,6 +537,14 @@ static int sun6i_dphy_power_on(struct phy *phy)
 {
 	struct sun6i_dphy *dphy = phy_get_drvdata(phy);
 
+	if (phy->attrs.mode == PHY_MODE_LVDS && dphy->variant->is_combo_dphy) {
+		if (dphy->variant->lvds_power_on) {
+			dphy->variant->lvds_power_on(dphy);
+			return 0;
+		}
+		return -EINVAL;
+	}
+
 	switch (dphy->direction) {
 	case SUN6I_DPHY_DIRECTION_TX:
 		return sun6i_dphy_tx_power_on(dphy);
@@ -514,6 +567,11 @@ static int sun6i_dphy_power_off(struct phy *phy)
 	regmap_write(dphy->regs, SUN6I_DPHY_ANA3_REG, 0);
 	regmap_write(dphy->regs, SUN6I_DPHY_ANA4_REG, 0);
 
+	if (phy->attrs.mode == PHY_MODE_LVDS && dphy->variant->is_combo_dphy) {
+		regmap_write(dphy->regs, SUN50I_COMBO_PHY_REG1, 0);
+		regmap_write(dphy->regs, SUN50I_COMBO_PHY_REG0, 0);
+	}
+
 	return 0;
 }
 
@@ -533,6 +591,7 @@ static const struct phy_ops sun6i_dphy_ops = {
 	.configure	= sun6i_dphy_configure,
 	.power_on	= sun6i_dphy_power_on,
 	.power_off	= sun6i_dphy_power_off,
+	.set_mode	= sun6i_dphy_set_mode,
 	.init		= sun6i_dphy_init,
 	.exit		= sun6i_dphy_exit,
 };
@@ -619,6 +678,8 @@ static const struct sun6i_dphy_variant sun6i_a31_mipi_dphy_variant = {
 
 static const struct sun6i_dphy_variant sun50i_a100_mipi_dphy_variant = {
 	.tx_power_on	= sun50i_a100_mipi_dphy_tx_power_on,
+	.lvds_power_on	= sun50i_a100_mipi_dphy_lvds_power_on,
+	.is_combo_dphy	= true,
 };
 
 static const struct of_device_id sun6i_dphy_of_table[] = {
-- 
2.25.1

[PATCH 2/5] drm/sun4i: Support LVDS using MIPI DSI combo D-PHY

[Kuba Szczodrzyński]

On Allwinner chips with a combo D-PHY, the TCON LCD0 should fetch it
from device tree, in order to enable LVDS. Since the PHY also needs to
be powered off to disable LVDS, add a function to the quirks.

Signed-off-by: Kuba Szczodrzyński <kuba@szczodrzynski.pl>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 12 ++++++++++++
 drivers/gpu/drm/sun4i/sun4i_tcon.h |  6 ++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index 960e83c82..ccf335a61 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -12,6 +12,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
+#include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/reset.h>
@@ -183,6 +184,8 @@ static void sun4i_tcon_lvds_set_status(struct sun4i_tcon *tcon,
 	} else {
 		regmap_update_bits(tcon->regs, SUN4I_TCON0_LVDS_IF_REG,
 				   SUN4I_TCON0_LVDS_IF_EN, 0);
+		if (tcon->quirks->disable_lvds_phy)
+			tcon->quirks->disable_lvds_phy(tcon, encoder);
 	}
 }
 
@@ -1245,6 +1248,15 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
 		goto err_free_dclk;
 	}
 
+	if (tcon->quirks->has_combo_dphy) {
+		tcon->dphy = devm_phy_get(dev, "dphy");
+		if (IS_ERR(tcon->dphy)) {
+			dev_err(dev, "Couldn't get the combo D-PHY\n");
+			ret = PTR_ERR(tcon->dphy);
+			goto err_free_dclk;
+		}
+	}
+
 	if (tcon->quirks->has_channel_0) {
 		/*
 		 * If we have an LVDS panel connected to the TCON, we should
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h
index fa23aa23f..b270d1a13 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.h
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h
@@ -239,6 +239,7 @@ struct sun4i_tcon_quirks {
 	bool	has_channel_0;	/* a83t does not have channel 0 on second TCON */
 	bool	has_channel_1;	/* a33 does not have channel 1 */
 	bool	has_lvds_alt;	/* Does the LVDS clock have a parent other than the TCON clock? */
+	bool	has_combo_dphy; /* Is the D-PHY used for LVDS output? */
 	bool	needs_de_be_mux; /* sun6i needs mux to select backend */
 	bool    needs_edp_reset; /* a80 edp reset needed for tcon0 access */
 	bool	supports_lvds;   /* Does the TCON support an LVDS output? */
@@ -250,6 +251,8 @@ struct sun4i_tcon_quirks {
 	/* handler for LVDS setup routine */
 	void	(*setup_lvds_phy)(struct sun4i_tcon *tcon,
 				  const struct drm_encoder *encoder);
+	void	(*disable_lvds_phy)(struct sun4i_tcon *tcon,
+				    const struct drm_encoder *encoder);
 };
 
 struct sun4i_tcon {
@@ -282,6 +285,9 @@ struct sun4i_tcon {
 	/* Associated crtc */
 	struct sun4i_crtc		*crtc;
 
+	/* Associated D-PHY */
+	struct phy			*dphy;
+
 	int				id;
 
 	/* TCON list management */
-- 
2.25.1

[PATCH 3/5] drm/sun4i: Enable LVDS output on sun20i D1s/T113

[Kuba Szczodrzyński]

The Allwinner D1s/T113 needs to use the combo D-PHY to enable LVDS
output.

Enable LVDS support in the TCON and configure it using the PHY.

Signed-off-by: Kuba Szczodrzyński <kuba@szczodrzynski.pl>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index ccf335a61..58230a552 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -171,6 +171,30 @@ static void sun6i_tcon_setup_lvds_phy(struct sun4i_tcon *tcon,
 			  SUN6I_TCON0_LVDS_ANA0_EN_DRVD(val));
 }
 
+static void sun20i_tcon_setup_lvds_dphy(struct sun4i_tcon *tcon,
+					const struct drm_encoder *encoder)
+{
+	union phy_configure_opts opts = { };
+
+	if (!tcon->quirks->has_combo_dphy || !tcon->dphy)
+		return;
+
+	phy_init(tcon->dphy);
+	phy_set_mode(tcon->dphy, PHY_MODE_LVDS);
+	phy_configure(tcon->dphy, &opts);
+	phy_power_on(tcon->dphy);
+}
+
+static void sun20i_tcon_disable_lvds_dphy(struct sun4i_tcon *tcon,
+					  const struct drm_encoder *encoder)
+{
+	if (!tcon->quirks->has_combo_dphy || !tcon->dphy)
+		return;
+
+	phy_power_off(tcon->dphy);
+	phy_exit(tcon->dphy);
+}
+
 static void sun4i_tcon_lvds_set_status(struct sun4i_tcon *tcon,
 				       const struct drm_encoder *encoder,
 				       bool enabled)
@@ -1550,8 +1574,12 @@ static const struct sun4i_tcon_quirks sun9i_a80_tcon_tv_quirks = {
 
 static const struct sun4i_tcon_quirks sun20i_d1_lcd_quirks = {
 	.has_channel_0		= true,
+	.has_combo_dphy		= true,
+	.supports_lvds		= true,
 	.dclk_min_div		= 1,
 	.set_mux		= sun8i_r40_tcon_tv_set_mux,
+	.setup_lvds_phy		= sun20i_tcon_setup_lvds_dphy,
+	.disable_lvds_phy	= sun20i_tcon_disable_lvds_dphy,
 };
 
 /* sun4i_drv uses this list to check if a device node is a TCON */
-- 
2.25.1

[PATCH 4/5] riscv: dts: allwinner: d1s-t113: Add D-PHY to TCON LCD0

[Kuba Szczodrzyński]

The sun4i TCON needs a reference to the D-PHY in order to support LVDS
on Allwinner D1s/T113.

Signed-off-by: Kuba Szczodrzyński <kuba@szczodrzynski.pl>
---
 arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi b/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi
index e4175adb0..d241ba306 100644
--- a/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi
+++ b/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi
@@ -828,6 +828,8 @@ tcon_lcd0: lcd-controller@5461000 {
 			resets = <&ccu RST_BUS_TCON_LCD0>,
 				 <&ccu RST_BUS_LVDS0>;
 			reset-names = "lcd", "lvds";
+			phys = <&dphy>;
+			phy-names = "dphy";
 			#clock-cells = <0>;
 
 			ports {
-- 
2.25.1

[PATCH 5/5] riscv: dts: allwinner: d1s-t113: Add LVDS0 pins

[Kuba Szczodrzyński]

Add LVDS pins to the PIO since it's now supported on D1s/T113.

Signed-off-by: Kuba Szczodrzyński <kuba@szczodrzynski.pl>
---
 arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi b/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi
index d241ba306..174b6d8f9 100644
--- a/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi
+++ b/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi
@@ -78,6 +78,15 @@ dsi_4lane_pins: dsi-4lane-pins {
 				function = "dsi";
 			};
 
+			/omit-if-no-ref/
+			lvds0_pins: lvds0-pins {
+				pins = "PD0", "PD1", "PD2", "PD3", "PD4", "PD5",
+				       "PD6", "PD7", "PD8", "PD9";
+				function = "lvds0";
+				drive-strength = <30>;
+				bias-disable;
+			};
+
 			/omit-if-no-ref/
 			lcd_rgb666_pins: lcd-rgb666-pins {
 				pins = "PD0", "PD1", "PD2", "PD3", "PD4", "PD5",
-- 
2.25.1

Re: [PATCH 1/5] phy: allwinner: phy-sun6i-mipi-dphy: Support LVDS in combo D-PHY

[Paul Kocialkowski]

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

Hi,

Thanks for working on this! See a few comments below.

On Fri 21 Feb 25, 17:17, Kuba Szczodrzyński wrote:
> Some Allwinner chips (notably the D1s/T113 and the A100) have a "combo
> MIPI DSI D-PHY" which is required when using single-link LVDS0.
> 
> In this mode, the DSI peripheral is not used and the PHY is not
> configured for DSI. Instead, the COMBO_PHY_REGx registers are set to
> enable LVDS operation.
> 
> Enable the PHY driver to work in LVDS mode on chips with a combo D-PHY.
> 
> Signed-off-by: Kuba Szczodrzyński <kuba@szczodrzynski.pl>
> ---
>  drivers/phy/allwinner/phy-sun6i-mipi-dphy.c | 65 ++++++++++++++++++++-
>  1 file changed, 63 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c b/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c
> index 36eab9527..f958e34da 100644
> --- a/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c
> +++ b/drivers/phy/allwinner/phy-sun6i-mipi-dphy.c
> @@ -166,8 +166,8 @@
>  #define SUN50I_COMBO_PHY_REG0_EN_CP		BIT(0)
>  
>  #define SUN50I_COMBO_PHY_REG1		0x114
> -#define SUN50I_COMBO_PHY_REG2_REG_VREF1P6(n)	(((n) & 0x7) << 4)
> -#define SUN50I_COMBO_PHY_REG2_REG_VREF0P8(n)	((n) & 0x7)
> +#define SUN50I_COMBO_PHY_REG1_REG_VREF1P6(n)	(((n) & 0x7) << 4)
> +#define SUN50I_COMBO_PHY_REG1_REG_VREF0P8(n)	((n) & 0x7)

Good catch! Would be good to mention in the commit log (or split in a separate
patch but that might be overdoing it since this register wasn't used so far).

>  #define SUN50I_COMBO_PHY_REG2		0x118
>  #define SUN50I_COMBO_PHY_REG2_HS_STOP_DLY(n)	((n) & 0xff)
> @@ -181,7 +181,9 @@ struct sun6i_dphy;
>  
>  struct sun6i_dphy_variant {
>  	void	(*tx_power_on)(struct sun6i_dphy *dphy);
> +	void	(*lvds_power_on)(struct sun6i_dphy *dphy);
>  	bool	rx_supported;
> +	bool	is_combo_dphy;
>  };
>  
>  struct sun6i_dphy {
> @@ -222,6 +224,18 @@ static int sun6i_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
>  	return 0;
>  }
>  
> +static int sun6i_dphy_set_mode(struct phy *phy, enum phy_mode mode, int submode)
> +{
> +	struct sun6i_dphy *dphy = phy_get_drvdata(phy);
> +
> +	if (mode == PHY_MODE_LVDS && !dphy->variant->is_combo_dphy) {
> +		/* Not a combo D-PHY: LVDS is not supported */

Missing a final . in the comment.

> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static void sun6i_a31_mipi_dphy_tx_power_on(struct sun6i_dphy *dphy)
>  {
>  	u8 lanes_mask = GENMASK(dphy->config.lanes - 1, 0);
> @@ -329,6 +343,37 @@ static void sun50i_a100_mipi_dphy_tx_power_on(struct sun6i_dphy *dphy)
>  	udelay(1);
>  }
>  
> +static void sun50i_a100_mipi_dphy_lvds_power_on(struct sun6i_dphy *dphy)
> +{
> +	regmap_write(dphy->regs, SUN50I_COMBO_PHY_REG1,
> +		     SUN50I_COMBO_PHY_REG1_REG_VREF1P6(4) |
> +		     SUN50I_COMBO_PHY_REG1_REG_VREF0P8(3));
> +
> +	regmap_write(dphy->regs, SUN50I_COMBO_PHY_REG0,
> +		     SUN50I_COMBO_PHY_REG0_EN_CP);
> +	udelay(5);

Please add a white space here...

> +	regmap_update_bits(dphy->regs, SUN50I_COMBO_PHY_REG0,
> +			   SUN50I_COMBO_PHY_REG0_EN_LVDS,
> +			   SUN50I_COMBO_PHY_REG0_EN_LVDS);
> +	udelay(5);

here too...

> +	regmap_update_bits(dphy->regs, SUN50I_COMBO_PHY_REG0,
> +			   SUN50I_COMBO_PHY_REG0_EN_COMBOLDO,
> +			   SUN50I_COMBO_PHY_REG0_EN_COMBOLDO);
> +	udelay(5);

here too...

> +	regmap_update_bits(dphy->regs, SUN50I_COMBO_PHY_REG0,
> +			   SUN50I_COMBO_PHY_REG0_EN_MIPI,
> +			   SUN50I_COMBO_PHY_REG0_EN_MIPI);
> +
> +	regmap_write(dphy->regs, SUN6I_DPHY_ANA4_REG,
> +		     SUN6I_DPHY_ANA4_REG_EN_MIPI |
> +		     SUN6I_DPHY_ANA4_REG_IB(2));
here too...

> +	regmap_write(dphy->regs, SUN6I_DPHY_ANA3_REG,
> +		     SUN6I_DPHY_ANA3_EN_LDOR |
> +		     SUN6I_DPHY_ANA3_EN_LDOD);

here too...

> +	regmap_write(dphy->regs, SUN6I_DPHY_ANA2_REG, 0);

and here too in order to match the coding style.

> +	regmap_write(dphy->regs, SUN6I_DPHY_ANA1_REG, 0);
> +}
> +
>  static int sun6i_dphy_tx_power_on(struct sun6i_dphy *dphy)
>  {
>  	u8 lanes_mask = GENMASK(dphy->config.lanes - 1, 0);
> @@ -492,6 +537,14 @@ static int sun6i_dphy_power_on(struct phy *phy)
>  {
>  	struct sun6i_dphy *dphy = phy_get_drvdata(phy);
>  
> +	if (phy->attrs.mode == PHY_MODE_LVDS && dphy->variant->is_combo_dphy) {
> +		if (dphy->variant->lvds_power_on) {
> +			dphy->variant->lvds_power_on(dphy);
> +			return 0;
> +		}
> +		return -EINVAL;

This would look better the other way round: first check:
	if (!dphy->variant->lvds_power_on)
		return -EINVAL;

and then call the function pointer and return 0 without extra indentation.

> +	}
> +
>  	switch (dphy->direction) {
>  	case SUN6I_DPHY_DIRECTION_TX:
>  		return sun6i_dphy_tx_power_on(dphy);
> @@ -514,6 +567,11 @@ static int sun6i_dphy_power_off(struct phy *phy)
>  	regmap_write(dphy->regs, SUN6I_DPHY_ANA3_REG, 0);
>  	regmap_write(dphy->regs, SUN6I_DPHY_ANA4_REG, 0);
>  
> +	if (phy->attrs.mode == PHY_MODE_LVDS && dphy->variant->is_combo_dphy) {
> +		regmap_write(dphy->regs, SUN50I_COMBO_PHY_REG1, 0);
> +		regmap_write(dphy->regs, SUN50I_COMBO_PHY_REG0, 0);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -533,6 +591,7 @@ static const struct phy_ops sun6i_dphy_ops = {
>  	.configure	= sun6i_dphy_configure,
>  	.power_on	= sun6i_dphy_power_on,
>  	.power_off	= sun6i_dphy_power_off,
> +	.set_mode	= sun6i_dphy_set_mode,
>  	.init		= sun6i_dphy_init,
>  	.exit		= sun6i_dphy_exit,
>  };
> @@ -619,6 +678,8 @@ static const struct sun6i_dphy_variant sun6i_a31_mipi_dphy_variant = {
>  
>  static const struct sun6i_dphy_variant sun50i_a100_mipi_dphy_variant = {
>  	.tx_power_on	= sun50i_a100_mipi_dphy_tx_power_on,
> +	.lvds_power_on	= sun50i_a100_mipi_dphy_lvds_power_on,
> +	.is_combo_dphy	= true,
>  };
>  
>  static const struct of_device_id sun6i_dphy_of_table[] = {
> -- 
> 2.25.1
> 
> 

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Expert in multimedia, graphics and embedded hardware support with Linux.

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

Re: [PATCH 2/5] drm/sun4i: Support LVDS using MIPI DSI combo D-PHY

[Paul Kocialkowski]

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

Hi,

Some comments below.

On Fri 21 Feb 25, 17:17, Kuba Szczodrzyński wrote:
> On Allwinner chips with a combo D-PHY, the TCON LCD0 should fetch it
> from device tree, in order to enable LVDS. Since the PHY also needs to
> be powered off to disable LVDS, add a function to the quirks.
> 
> Signed-off-by: Kuba Szczodrzyński <kuba@szczodrzynski.pl>
> ---
>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 12 ++++++++++++
>  drivers/gpu/drm/sun4i/sun4i_tcon.h |  6 ++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index 960e83c82..ccf335a61 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -12,6 +12,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_platform.h>
> +#include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  #include <linux/reset.h>
> @@ -183,6 +184,8 @@ static void sun4i_tcon_lvds_set_status(struct sun4i_tcon *tcon,
>  	} else {
>  		regmap_update_bits(tcon->regs, SUN4I_TCON0_LVDS_IF_REG,
>  				   SUN4I_TCON0_LVDS_IF_EN, 0);

Does the T113-S3/D1 also have LVDS support in TCON regsiters?
My understanding is that it only has the combo-phy, so this register write
should not take place.

I think it would be easier to just move this to a common callback for users of
the tcon0 lvds phy and always call 

> +		if (tcon->quirks->disable_lvds_phy)
> +			tcon->quirks->disable_lvds_phy(tcon, encoder);

Also "disable" is often used as opposite of "enable", so I would suggest using
"teardown" as opposite of "setup" instead.

>  	}
>  }
>  
> @@ -1245,6 +1248,15 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master,
>  		goto err_free_dclk;
>  	}
>  
> +	if (tcon->quirks->has_combo_dphy) {
> +		tcon->dphy = devm_phy_get(dev, "dphy");

I don't think it makes sense to call this dphy since it's used in LVDS mode
and not in D-PHY mode. I would suggest using "combo" as phy name instead.

It would have made more sense to also have the compatible reflect this
(by calling it combo-phy instead of mipi-dphy) but it's too late now.

> +		if (IS_ERR(tcon->dphy)) {
> +			dev_err(dev, "Couldn't get the combo D-PHY\n");
> +			ret = PTR_ERR(tcon->dphy);
> +			goto err_free_dclk;
> +		}
> +	}
> +
>  	if (tcon->quirks->has_channel_0) {
>  		/*
>  		 * If we have an LVDS panel connected to the TCON, we should
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h
> index fa23aa23f..b270d1a13 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.h
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h
> @@ -239,6 +239,7 @@ struct sun4i_tcon_quirks {
>  	bool	has_channel_0;	/* a83t does not have channel 0 on second TCON */
>  	bool	has_channel_1;	/* a33 does not have channel 1 */
>  	bool	has_lvds_alt;	/* Does the LVDS clock have a parent other than the TCON clock? */
> +	bool	has_combo_dphy; /* Is the D-PHY used for LVDS output? */
>  	bool	needs_de_be_mux; /* sun6i needs mux to select backend */
>  	bool    needs_edp_reset; /* a80 edp reset needed for tcon0 access */
>  	bool	supports_lvds;   /* Does the TCON support an LVDS output? */
> @@ -250,6 +251,8 @@ struct sun4i_tcon_quirks {
>  	/* handler for LVDS setup routine */
>  	void	(*setup_lvds_phy)(struct sun4i_tcon *tcon,
>  				  const struct drm_encoder *encoder);
> +	void	(*disable_lvds_phy)(struct sun4i_tcon *tcon,
> +				    const struct drm_encoder *encoder);
>  };
>  
>  struct sun4i_tcon {
> @@ -282,6 +285,9 @@ struct sun4i_tcon {
>  	/* Associated crtc */
>  	struct sun4i_crtc		*crtc;
>  
> +	/* Associated D-PHY */
> +	struct phy			*dphy;
> +
>  	int				id;
>  
>  	/* TCON list management */
> -- 
> 2.25.1
> 
> 

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Expert in multimedia, graphics and embedded hardware support with Linux.

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

Re: [PATCH 3/5] drm/sun4i: Enable LVDS output on sun20i D1s/T113

[Paul Kocialkowski]

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

Hi,

Some comments below.

On Fri 21 Feb 25, 17:17, Kuba Szczodrzyński wrote:
> The Allwinner D1s/T113 needs to use the combo D-PHY to enable LVDS
> output.
> 
> Enable LVDS support in the TCON and configure it using the PHY.
> 
> Signed-off-by: Kuba Szczodrzyński <kuba@szczodrzynski.pl>
> ---
>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index ccf335a61..58230a552 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -171,6 +171,30 @@ static void sun6i_tcon_setup_lvds_phy(struct sun4i_tcon *tcon,
>  			  SUN6I_TCON0_LVDS_ANA0_EN_DRVD(val));
>  }
>  
> +static void sun20i_tcon_setup_lvds_dphy(struct sun4i_tcon *tcon,
> +					const struct drm_encoder *encoder)
> +{
> +	union phy_configure_opts opts = { };
> +
> +	if (!tcon->quirks->has_combo_dphy || !tcon->dphy)
> +		return;
> +
> +	phy_init(tcon->dphy);
> +	phy_set_mode(tcon->dphy, PHY_MODE_LVDS);
> +	phy_configure(tcon->dphy, &opts);
> +	phy_power_on(tcon->dphy);

While the callback doesn't have any return code (and it probably should, feel
free to fix that), the return codes for the phy_ calls should be checked and
the function aborted early in case of error.

> +}
> +
> +static void sun20i_tcon_disable_lvds_dphy(struct sun4i_tcon *tcon,
> +					  const struct drm_encoder *encoder)
> +{
> +	if (!tcon->quirks->has_combo_dphy || !tcon->dphy)
> +		return;
> +
> +	phy_power_off(tcon->dphy);
> +	phy_exit(tcon->dphy);

Same comment here.

> +}
> +
>  static void sun4i_tcon_lvds_set_status(struct sun4i_tcon *tcon,
>  				       const struct drm_encoder *encoder,
>  				       bool enabled)
> @@ -1550,8 +1574,12 @@ static const struct sun4i_tcon_quirks sun9i_a80_tcon_tv_quirks = {
>  
>  static const struct sun4i_tcon_quirks sun20i_d1_lcd_quirks = {
>  	.has_channel_0		= true,
> +	.has_combo_dphy		= true,
> +	.supports_lvds		= true,
>  	.dclk_min_div		= 1,
>  	.set_mux		= sun8i_r40_tcon_tv_set_mux,
> +	.setup_lvds_phy		= sun20i_tcon_setup_lvds_dphy,
> +	.disable_lvds_phy	= sun20i_tcon_disable_lvds_dphy,
>  };
>  
>  /* sun4i_drv uses this list to check if a device node is a TCON */
> -- 
> 2.25.1
> 
> 

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Expert in multimedia, graphics and embedded hardware support with Linux.

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

Re: [PATCH 4/5] riscv: dts: allwinner: d1s-t113: Add D-PHY to TCON LCD0

[Paul Kocialkowski]

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

Hi,

On Fri 21 Feb 25, 17:17, Kuba Szczodrzyński wrote:
> The sun4i TCON needs a reference to the D-PHY in order to support LVDS
> on Allwinner D1s/T113.
> 
> Signed-off-by: Kuba Szczodrzyński <kuba@szczodrzynski.pl>
> ---
>  arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi b/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi
> index e4175adb0..d241ba306 100644
> --- a/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi
> +++ b/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi
> @@ -828,6 +828,8 @@ tcon_lcd0: lcd-controller@5461000 {
>  			resets = <&ccu RST_BUS_TCON_LCD0>,
>  				 <&ccu RST_BUS_LVDS0>;
>  			reset-names = "lcd", "lvds";
> +			phys = <&dphy>;
> +			phy-names = "dphy";

Like I mentionned in the other patch, I don't think the name "dphy" makes sense.
"combo-phy" probably makes a lot more sense here.

Also the dt binding should be updated to reflect the introduction of the
property.

Paul

>  			#clock-cells = <0>;
>  
>  			ports {
> -- 
> 2.25.1
> 
> 

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Expert in multimedia, graphics and embedded hardware support with Linux.

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

Re: [PATCH 5/5] riscv: dts: allwinner: d1s-t113: Add LVDS0 pins

[Paul Kocialkowski]

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

Hi,

On Fri 21 Feb 25, 17:17, Kuba Szczodrzyński wrote:
> Add LVDS pins to the PIO since it's now supported on D1s/T113.
> 
> Signed-off-by: Kuba Szczodrzyński <kuba@szczodrzynski.pl>
> ---
>  arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi b/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi
> index d241ba306..174b6d8f9 100644
> --- a/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi
> +++ b/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi
> @@ -78,6 +78,15 @@ dsi_4lane_pins: dsi-4lane-pins {
>  				function = "dsi";
>  			};
>  
> +			/omit-if-no-ref/
> +			lvds0_pins: lvds0-pins {

This usually has a lcd_ prefix (see sun7i-a20.dtsi and sun8i-a83t.dtsi).
It should also be sorted alphabetically.

Thanks!

Paul

> +				pins = "PD0", "PD1", "PD2", "PD3", "PD4", "PD5",
> +				       "PD6", "PD7", "PD8", "PD9";
> +				function = "lvds0";
> +				drive-strength = <30>;
> +				bias-disable;
> +			};
> +
>  			/omit-if-no-ref/
>  			lcd_rgb666_pins: lcd-rgb666-pins {
>  				pins = "PD0", "PD1", "PD2", "PD3", "PD4", "PD5",
> -- 
> 2.25.1
> 
> 

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Expert in multimedia, graphics and embedded hardware support with Linux.

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

Re: [PATCH 0/5] drm/sun4i: Support LVDS on D1s/T113 combo D-PHY

[Paul Kocialkowski]

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

Hi,

Thanks for your work!

On Fri 21 Feb 25, 17:17, Kuba Szczodrzyński wrote:
> Some Allwinner chips (notably the D1s/T113 and the A100) have a "combo
> MIPI DSI D-PHY" which is required when using single-link LVDS0. The same
> PD0..PD9 pins are used for either DSI or LVDS.
> 
> Other than having to use the combo D-PHY, LVDS output is configured in
> the same way as on older chips.

From what I understand of section 5.1.4.2 LVDS Mode Configuration Process
there's two LVDS outputs:
- LVDS0, driven by the combo-phy
- LVDS1, driven by the usual TCON0 LVDS PHY

As far as I understand the LVDS_IF register still has to be configured for
LVDS0. The D1 manual mentions a LVDS1_IF register at offset 0x244 (which I don't
see in the T113-S3 manual, but is probably also there), which is liekely used to
configure LVDS1. Then we find our LVDS_ANA0/ANA1 registers that are likely just
used for LVDS1.

While this series adds support for LVDS0 only, it would be good to also be able
to support LVDS1, including dual-link modes. So eventually we'd need a way to
actually support both cases.

It would be great if you could include these details somewhere so they don't get
lost. And this seems to be the exact same situation as the A133 by the way.

All the best,

Paul

> This series enables the sun6i MIPI D-PHY to also work in LVDS mode. It
> is then configured by the LCD TCON, which allows connecting a
> single-link LVDS display panel.
> 
> Kuba Szczodrzyński (5):
>   phy: allwinner: phy-sun6i-mipi-dphy: Support LVDS in combo D-PHY
>   drm/sun4i: Support LVDS using MIPI DSI combo D-PHY
>   drm/sun4i: Enable LVDS output on sun20i D1s/T113
>   riscv: dts: allwinner: d1s-t113: Add D-PHY to TCON LCD0
>   riscv: dts: allwinner: d1s-t113: Add LVDS0 pins
> 
>  .../boot/dts/allwinner/sunxi-d1s-t113.dtsi    | 10 +++
>  drivers/gpu/drm/sun4i/sun4i_tcon.c            | 40 ++++++++++++
>  drivers/gpu/drm/sun4i/sun4i_tcon.h            |  6 ++
>  drivers/phy/allwinner/phy-sun6i-mipi-dphy.c   | 65 ++++++++++++++++++-
>  4 files changed, 119 insertions(+), 2 deletions(-)
> 
> -- 
> 2.25.1
> 
> 

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Expert in multimedia, graphics and embedded hardware support with Linux.

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

Re: [PATCH 2/5] drm/sun4i: Support LVDS using MIPI DSI combo D-PHY

[Paul Kocialkowski]

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

On Wed 25 Jun 25, 10:03, Paul Kocialkowski wrote:
> On Fri 21 Feb 25, 17:17, Kuba Szczodrzyński wrote:
> > @@ -183,6 +184,8 @@ static void sun4i_tcon_lvds_set_status(struct sun4i_tcon *tcon,
> >  	} else {
> >  		regmap_update_bits(tcon->regs, SUN4I_TCON0_LVDS_IF_REG,
> >  				   SUN4I_TCON0_LVDS_IF_EN, 0);
> 
> Does the T113-S3/D1 also have LVDS support in TCON regsiters?
> My understanding is that it only has the combo-phy, so this register write
> should not take place.
> 
> I think it would be easier to just move this to a common callback for users of
> the tcon0 lvds phy and always call 

Nevermind this comment, the LVDS_IF writes are actually necessary and
independent from the LVDS PHY configuration.

All the best,

Paul

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Expert in multimedia, graphics and embedded hardware support with Linux.

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