[PATCH v11 0/7] drm/sun4i: Allwinner A64 MIPI-DSI support

[PATCH v11 0/7] drm/sun4i: Allwinner A64 MIPI-DSI support

[Jagan Teki]

This is v11 version for Allwinner A64 MIPI-DSI support
and here is the previous version set[1]

Changes for v11:
- fix dt-bindings for dphy
- fix dt-bindings for dsi controller
- add bus clock handling code
- tested on A64, A33 boards.
Changes for v10:
- updated dt-bindings as per .yaml format
- rebased on drm-misc/for-linux-next
Changes for v9:
- moved dsi fixes in separate series on top of A33
- rebase on linux-next
Changes for v8:
- rebased on drm-misc change along with linux-next
- reworked video start delay patch
- tested on 4 different dsi panels
- reworked commit messages
Changes for v7:
- moved vcc-dsi binding to required filed.
- drop quotes on fallback dphy bindings.
- drop min_rate clock pll-mipi patches.
- introduce dclk divider computation as like A64 BSP.
- add A64 DSI quark patches.
- fixed A64 DSI pipeline.
- add proper commit messages.
- collect Merlijn Wajer Tested-by credits.
Changes for v6:
- dropped unneeded changes, patches
- fixed all burst mode patches as per previous version comments
- rebase on master
- update proper commit message
- dropped unneeded comments
- order the patches that make review easy
Changes for v5:
- collect Rob, Acked-by
- droped "Fix VBP size calculation" patch
- updated vblk timing calculation.
- droped techstar, bananapi dsi panel drivers which may require
  bridge or other setup. it's under discussion.
Changes for v4:
- droppoed untested CCU_FEATURE_FIXED_POSTDIV check code in
  nkm min, max rate patches
- create two patches for "Add Allwinner A64 MIPI DSI support"
  one for has_mod_clk quirk and other one for A64 support
- use existing driver code construct for hblk computation
- dropped "Increase hfp packet overhead" patch [2], though BSP added
  this but we have no issues as of now.
  (no issues on panel side w/o this change)
- create separate function for vblk computation 
- enable vcc-dsi regulator in dsi_runtime_resume
- collect Rob, Acked-by
- update MAINTAINERS file for panel drivers
- cleanup commit messages
- fixed checkpatch warnings/errors

[1] https://patchwork.freedesktop.org/series/67632/

Any inputs?
Jagan.

Jagan Teki (7):
  dt-bindings: sun6i-dsi: Document A64 MIPI-DSI controller
  dt-bindings: sun6i-dsi: Add A64 DPHY compatible (w/ A31 fallback)
  drm/sun4i: dsi: Add has_mod_clk quirk
  drm/sun4i: dsi: Handle bus clock explicitly 
  drm/sun4i: dsi: Add Allwinner A64 MIPI DSI support
  arm64: dts: allwinner: a64: Add MIPI DSI pipeline
  [DO NOT MERGE] arm64: dts: allwinner: bananapi-m64: Enable Bananapi S070WV20-CT16 DSI panel

 .../display/allwinner,sun6i-a31-mipi-dsi.yaml | 20 ++++++-
 .../phy/allwinner,sun6i-a31-mipi-dphy.yaml    |  6 +-
 .../dts/allwinner/sun50i-a64-bananapi-m64.dts | 31 +++++++++++
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 37 +++++++++++++
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c        | 55 +++++++++++++++----
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h        |  5 ++
 6 files changed, 139 insertions(+), 15 deletions(-)

-- 
2.18.0.321.gffc6fa0e3

[PATCH v11 1/7] dt-bindings: sun6i-dsi: Document A64 MIPI-DSI controller

[Jagan Teki]

The MIPI DSI controller in Allwinner A64 is similar to A33.

But unlike A33, A64 doesn't have DSI_SCLK gating so it is valid
to have separate compatible for A64 on the same driver.

DSI_SCLK uses mod clock-names on dt-bindings, so the same
is not required for A64.

On that note
- A64 require minimum of 1 clock like the bus clock
- A33 require minimum of 2 clocks like both bus, mod clocks

So, update dt-bindings so-that it can document both A33,
A64 bindings requirements.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 .../display/allwinner,sun6i-a31-mipi-dsi.yaml | 20 +++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml
index dafc0980c4fa..2b7016ca382c 100644
--- a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml
+++ b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml
@@ -15,7 +15,9 @@ properties:
   "#size-cells": true
 
   compatible:
-    const: allwinner,sun6i-a31-mipi-dsi
+    oneOf:
+      - const: allwinner,sun6i-a31-mipi-dsi
+      - const: allwinner,sun50i-a64-mipi-dsi
 
   reg:
     maxItems: 1
@@ -24,6 +26,8 @@ properties:
     maxItems: 1
 
   clocks:
+    minItems: 1
+    maxItems: 2
     items:
       - description: Bus Clock
       - description: Module Clock
@@ -63,13 +67,25 @@ required:
   - reg
   - interrupts
   - clocks
-  - clock-names
   - phys
   - phy-names
   - resets
   - vcc-dsi-supply
   - port
 
+allOf:
+  - if:
+      properties:
+         compatible:
+           contains:
+             const: allwinner,sun6i-a31-mipi-dsi
+      then:
+        properties:
+          clocks:
+            minItems: 2
+        required:
+          - clock-names
+
 additionalProperties: false
 
 examples:
-- 
2.18.0.321.gffc6fa0e3

[PATCH v11 2/7] dt-bindings: sun6i-dsi: Add A64 DPHY compatible (w/ A31 fallback)

[Jagan Teki]

The MIPI DSI PHY controller on Allwinner A64 is similar
on the one on A31.

Add A64 compatible and append A31 compatible as fallback.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 .../bindings/phy/allwinner,sun6i-a31-mipi-dphy.yaml         | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/phy/allwinner,sun6i-a31-mipi-dphy.yaml b/Documentation/devicetree/bindings/phy/allwinner,sun6i-a31-mipi-dphy.yaml
index fa46670de299..8841938050b2 100644
--- a/Documentation/devicetree/bindings/phy/allwinner,sun6i-a31-mipi-dphy.yaml
+++ b/Documentation/devicetree/bindings/phy/allwinner,sun6i-a31-mipi-dphy.yaml
@@ -15,7 +15,11 @@ properties:
     const: 0
 
   compatible:
-    const: allwinner,sun6i-a31-mipi-dphy
+    oneOf:
+      - const: allwinner,sun6i-a31-mipi-dphy
+      - items:
+          - const: allwinner,sun50i-a64-mipi-dphy
+          - const: allwinner,sun6i-a31-mipi-dphy
 
   reg:
     maxItems: 1
-- 
2.18.0.321.gffc6fa0e3

[PATCH v11 3/7] drm/sun4i: dsi: Add has_mod_clk quirk

[Jagan Teki]

As per the user manual, look like mod clock is not mandatory
for all Allwinner MIPI DSI controllers, it is connected to
CLK_DSI_SCLK for A31 and not available in A64.

So add has_mod_clk quirk and process the mod clk accordingly.

Tested-by: Merlijn Wajer <merlijn@wizzup.org>
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 38 ++++++++++++++++++--------
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h |  5 ++++
 2 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index c958ca9bae63..8c4c541224dd 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -11,6 +11,7 @@
 #include <linux/crc-ccitt.h>
 #include <linux/module.h>
 #include <linux/of_address.h>
+#include <linux/of_device.h>
 #include <linux/phy/phy-mipi-dphy.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
@@ -1093,6 +1094,7 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
 	dsi->dev = dev;
 	dsi->host.ops = &sun6i_dsi_host_ops;
 	dsi->host.dev = dev;
+	dsi->variant = of_device_get_match_data(dev);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	base = devm_ioremap_resource(dev, res);
@@ -1120,17 +1122,20 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
 		return PTR_ERR(dsi->reset);
 	}
 
-	dsi->mod_clk = devm_clk_get(dev, "mod");
-	if (IS_ERR(dsi->mod_clk)) {
-		dev_err(dev, "Couldn't get the DSI mod clock\n");
-		return PTR_ERR(dsi->mod_clk);
+	if (dsi->variant->has_mod_clk) {
+		dsi->mod_clk = devm_clk_get(dev, "mod");
+		if (IS_ERR(dsi->mod_clk)) {
+			dev_err(dev, "Couldn't get the DSI mod clock\n");
+			return PTR_ERR(dsi->mod_clk);
+		}
 	}
 
 	/*
 	 * In order to operate properly, that clock seems to be always
 	 * set to 297MHz.
 	 */
-	clk_set_rate_exclusive(dsi->mod_clk, 297000000);
+	if (dsi->variant->has_mod_clk)
+		clk_set_rate_exclusive(dsi->mod_clk, 297000000);
 
 	dsi->dphy = devm_phy_get(dev, "dphy");
 	if (IS_ERR(dsi->dphy)) {
@@ -1160,7 +1165,8 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
 err_pm_disable:
 	pm_runtime_disable(dev);
 err_unprotect_clk:
-	clk_rate_exclusive_put(dsi->mod_clk);
+	if (dsi->variant->has_mod_clk)
+		clk_rate_exclusive_put(dsi->mod_clk);
 	return ret;
 }
 
@@ -1172,7 +1178,8 @@ static int sun6i_dsi_remove(struct platform_device *pdev)
 	component_del(&pdev->dev, &sun6i_dsi_ops);
 	mipi_dsi_host_unregister(&dsi->host);
 	pm_runtime_disable(dev);
-	clk_rate_exclusive_put(dsi->mod_clk);
+	if (dsi->variant->has_mod_clk)
+		clk_rate_exclusive_put(dsi->mod_clk);
 
 	return 0;
 }
@@ -1189,7 +1196,8 @@ static int __maybe_unused sun6i_dsi_runtime_resume(struct device *dev)
 	}
 
 	reset_control_deassert(dsi->reset);
-	clk_prepare_enable(dsi->mod_clk);
+	if (dsi->variant->has_mod_clk)
+		clk_prepare_enable(dsi->mod_clk);
 
 	/*
 	 * Enable the DSI block.
@@ -1217,7 +1225,8 @@ static int __maybe_unused sun6i_dsi_runtime_suspend(struct device *dev)
 {
 	struct sun6i_dsi *dsi = dev_get_drvdata(dev);
 
-	clk_disable_unprepare(dsi->mod_clk);
+	if (dsi->variant->has_mod_clk)
+		clk_disable_unprepare(dsi->mod_clk);
 	reset_control_assert(dsi->reset);
 	regulator_disable(dsi->regulator);
 
@@ -1230,9 +1239,16 @@ static const struct dev_pm_ops sun6i_dsi_pm_ops = {
 			   NULL)
 };
 
+static const struct sun6i_dsi_variant sun6i_a31_mipi_dsi = {
+	.has_mod_clk = true,
+};
+
 static const struct of_device_id sun6i_dsi_of_table[] = {
-	{ .compatible = "allwinner,sun6i-a31-mipi-dsi" },
-	{ }
+	{
+		.compatible = "allwinner,sun6i-a31-mipi-dsi",
+		.data = &sun6i_a31_mipi_dsi,
+	},
+	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, sun6i_dsi_of_table);
 
diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
index 3f4846f581ef..d791c9f6fccf 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
@@ -15,6 +15,10 @@
 
 #define SUN6I_DSI_TCON_DIV	4
 
+struct sun6i_dsi_variant {
+	bool			has_mod_clk;
+};
+
 struct sun6i_dsi {
 	struct drm_connector	connector;
 	struct drm_encoder	encoder;
@@ -31,6 +35,7 @@ struct sun6i_dsi {
 	struct sun4i_drv	*drv;
 	struct mipi_dsi_device	*device;
 	struct drm_panel	*panel;
+	const struct sun6i_dsi_variant	*variant;
 };
 
 static inline struct sun6i_dsi *host_to_sun6i_dsi(struct mipi_dsi_host *host)
-- 
2.18.0.321.gffc6fa0e3

[PATCH v11 4/7] drm/sun4i: dsi: Handle bus clock explicitly

[Jagan Teki]

Usage of clocks are varies between different Allwinner
DSI controllers. Clocking in A33 would need bus and
mod clocks where as A64 would need only bus clock.

To support this kind of clocking structure variants
in the same dsi driver, explicit handling of common
clock would require since the A64 doesn't need to
mention the clock-names explicitly in dts since it
support only one bus clock.

Also pass clk_id NULL instead "bus" to regmap clock
init function since the single clock variants no need
to mention clock-names explicitly.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index 8c4c541224dd..eacdfcff64ad 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -1109,7 +1109,7 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
 		return PTR_ERR(dsi->regulator);
 	}
 
-	dsi->regs = devm_regmap_init_mmio_clk(dev, "bus", base,
+	dsi->regs = devm_regmap_init_mmio_clk(dev, NULL, base,
 					      &sun6i_dsi_regmap_config);
 	if (IS_ERR(dsi->regs)) {
 		dev_err(dev, "Couldn't create the DSI encoder regmap\n");
@@ -1122,6 +1122,12 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
 		return PTR_ERR(dsi->reset);
 	}
 
+	dsi->bus_clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(dsi->bus_clk)) {
+		dev_err(dev, "Couldn't get the DSI bus clock\n");
+		return PTR_ERR(dsi->bus_clk);
+	}
+
 	if (dsi->variant->has_mod_clk) {
 		dsi->mod_clk = devm_clk_get(dev, "mod");
 		if (IS_ERR(dsi->mod_clk)) {
@@ -1196,6 +1202,7 @@ static int __maybe_unused sun6i_dsi_runtime_resume(struct device *dev)
 	}
 
 	reset_control_deassert(dsi->reset);
+	clk_prepare_enable(dsi->bus_clk);
 	if (dsi->variant->has_mod_clk)
 		clk_prepare_enable(dsi->mod_clk);
 
@@ -1227,6 +1234,7 @@ static int __maybe_unused sun6i_dsi_runtime_suspend(struct device *dev)
 
 	if (dsi->variant->has_mod_clk)
 		clk_disable_unprepare(dsi->mod_clk);
+	clk_disable_unprepare(dsi->bus_clk);
 	reset_control_assert(dsi->reset);
 	regulator_disable(dsi->regulator);
 
-- 
2.18.0.321.gffc6fa0e3

[PATCH v11 5/7] drm/sun4i: dsi: Add Allwinner A64 MIPI DSI support

[Jagan Teki]

The MIPI DSI controller in Allwinner A64 is similar to A33.

But unlike A33, A64 doesn't have DSI_SCLK gating so add compatible
for Allwinner A64 with uninitialized has_mod_clk driver.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
Tested-by: Merlijn Wajer <merlijn@wizzup.org>
---
 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index eacdfcff64ad..4dda96e0febd 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -1251,11 +1251,18 @@ static const struct sun6i_dsi_variant sun6i_a31_mipi_dsi = {
 	.has_mod_clk = true,
 };
 
+static const struct sun6i_dsi_variant sun50i_a64_mipi_dsi = {
+};
+
 static const struct of_device_id sun6i_dsi_of_table[] = {
 	{
 		.compatible = "allwinner,sun6i-a31-mipi-dsi",
 		.data = &sun6i_a31_mipi_dsi,
 	},
+	{
+		.compatible = "allwinner,sun50i-a64-mipi-dsi",
+		.data = &sun50i_a64_mipi_dsi,
+	},
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, sun6i_dsi_of_table);
-- 
2.18.0.321.gffc6fa0e3

[PATCH v11 6/7] arm64: dts: allwinner: a64: Add MIPI DSI pipeline

[Jagan Teki]

Add MIPI DSI pipeline for Allwinner A64.

- dsi node, with A64 compatible since it doesn't support
  DSI_SCLK gating unlike A33
- dphy node, with A64 compatible with A33 fallback since
  DPHY on A64 and A33 is similar
- finally, attach the dsi_in to tcon0 for complete MIPI DSI

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
Tested-by: Merlijn Wajer <merlijn@wizzup.org>
---
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 37 +++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index 69128a6dfc46..a52dfa98ac5e 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -382,6 +382,12 @@
 					#address-cells = <1>;
 					#size-cells = <0>;
 					reg = <1>;
+
+					tcon0_out_dsi: endpoint@1 {
+						reg = <1>;
+						remote-endpoint = <&dsi_in_tcon0>;
+						allwinner,tcon-channel = <1>;
+					};
 				};
 			};
 		};
@@ -1003,6 +1009,37 @@
 			status = "disabled";
 		};
 
+		dsi: dsi@1ca0000 {
+			compatible = "allwinner,sun50i-a64-mipi-dsi";
+			reg = <0x01ca0000 0x1000>;
+			interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_MIPI_DSI>;
+			resets = <&ccu RST_BUS_MIPI_DSI>;
+			phys = <&dphy>;
+			phy-names = "dphy";
+			status = "disabled";
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port {
+				dsi_in_tcon0: endpoint {
+					remote-endpoint = <&tcon0_out_dsi>;
+				};
+			};
+		};
+
+		dphy: d-phy@1ca1000 {
+			compatible = "allwinner,sun50i-a64-mipi-dphy",
+				     "allwinner,sun6i-a31-mipi-dphy";
+			reg = <0x01ca1000 0x1000>;
+			clocks = <&ccu CLK_BUS_MIPI_DSI>,
+				 <&ccu CLK_DSI_DPHY>;
+			clock-names = "bus", "mod";
+			resets = <&ccu RST_BUS_MIPI_DSI>;
+			status = "disabled";
+			#phy-cells = <0>;
+		};
+
 		hdmi: hdmi@1ee0000 {
 			compatible = "allwinner,sun50i-a64-dw-hdmi",
 				     "allwinner,sun8i-a83t-dw-hdmi";
-- 
2.18.0.321.gffc6fa0e3

[DO NOT MERGE] [PATCH v11 7/7] arm64: dts: allwinner: bananapi-m64: Enable Bananapi S070WV20-CT16 DSI panel

[Jagan Teki]

This patch add support for Bananapi S070WV20-CT16 DSI panel to
BPI-M64 board.

DSI panel connected via board DSI port with,
- DLDO1 as VCC-DSI supply
- DCDC1 as VDD supply
- PD7 gpio for lcd enable pin
- PD6 gpio for lcd reset pin
- PD5 gpio for backlight enable pin

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 .../dts/allwinner/sun50i-a64-bananapi-m64.dts | 31 +++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
index 208373efee49..6beaecdd802a 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
@@ -45,6 +45,7 @@
 #include "sun50i-a64.dtsi"
 
 #include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/pwm/pwm.h>
 
 / {
 	model = "BananaPi-M64";
@@ -56,6 +57,14 @@
 		serial1 = &uart1;
 	};
 
+	backlight: backlight {
+		compatible = "pwm-backlight";
+		pwms = <&r_pwm 0 50000 PWM_POLARITY_INVERTED>;
+		brightness-levels = <1 2 4 8 16 32 64 128 255>;
+		default-brightness-level = <2>;
+		enable-gpios = <&pio 3 5 GPIO_ACTIVE_HIGH>; /* LCD-BL-EN: PD5 */
+	};
+
 	chosen {
 		stdout-path = "serial0:115200n8";
 	};
@@ -116,6 +125,24 @@
 	status = "okay";
 };
 
+&dphy {
+	status = "okay";
+};
+
+&dsi {
+	vcc-dsi-supply = <&reg_dldo1>;		/* VCC3V3-DSI */
+	status = "okay";
+
+	panel@0 {
+		compatible = "bananapi,s070wv20-ct16-icn6211";
+		reg = <0>;
+		enable-gpios = <&pio 3 7 GPIO_ACTIVE_HIGH>; /* LCD-PWR-EN: PD7 */
+		reset-gpios = <&pio 3 6 GPIO_ACTIVE_HIGH>; /* LCD-RST: PD6 */
+		vdd-supply = <&reg_dcdc1>;
+		backlight = <&backlight>;
+	};
+};
+
 &ehci0 {
 	status = "okay";
 };
@@ -206,6 +233,10 @@
 	status = "okay";
 };
 
+&r_pwm {
+	status = "okay";
+};
+
 &r_rsb {
 	status = "okay";
 
-- 
2.18.0.321.gffc6fa0e3

Re: [PATCH v11 2/7] dt-bindings: sun6i-dsi: Add A64 DPHY compatible (w/ A31 fallback)

[Rob Herring]

On Fri, 25 Oct 2019 23:26:20 +0530, Jagan Teki wrote:
> The MIPI DSI PHY controller on Allwinner A64 is similar
> on the one on A31.
> 
> Add A64 compatible and append A31 compatible as fallback.
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>  .../bindings/phy/allwinner,sun6i-a31-mipi-dphy.yaml         | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 

Please add Acked-by/Reviewed-by tags when posting new versions. However,
there's no need to repost patches *only* to add the tags. The upstream
maintainer will do that for acks received on the version they apply.

If a tag was not added on purpose, please state why and what changed.

Re: [PATCH v11 4/7] drm/sun4i: dsi: Handle bus clock explicitly

[Maxime Ripard]

On Fri, Oct 25, 2019 at 11:26:22PM +0530, Jagan Teki wrote:
> Usage of clocks are varies between different Allwinner
> DSI controllers. Clocking in A33 would need bus and
> mod clocks where as A64 would need only bus clock.
>
> To support this kind of clocking structure variants
> in the same dsi driver,

There's no variance in the clock structure as far as the bus clock is
concerned.

> explicit handling of common clock would require since the A64
> doesn't need to mention the clock-names explicitly in dts since it
> support only one bus clock.
>
> Also pass clk_id NULL instead "bus" to regmap clock init function
> since the single clock variants no need to mention clock-names
> explicitly.

You don't need explicit clock handling. Passing NULL as the argument
in regmap_init_mmio_clk will make it use the first clock, which is the
bus clock.

Maxime

Re: [PATCH v11 4/7] drm/sun4i: dsi: Handle bus clock explicitly

[Jagan Teki]

Hi Maxime,

On Mon, Oct 28, 2019 at 9:06 PM Maxime Ripard <mripard@kernel.org> wrote:
>
> On Fri, Oct 25, 2019 at 11:26:22PM +0530, Jagan Teki wrote:
> > Usage of clocks are varies between different Allwinner
> > DSI controllers. Clocking in A33 would need bus and
> > mod clocks where as A64 would need only bus clock.
> >
> > To support this kind of clocking structure variants
> > in the same dsi driver,
>
> There's no variance in the clock structure as far as the bus clock is
> concerned.
>
> > explicit handling of common clock would require since the A64
> > doesn't need to mention the clock-names explicitly in dts since it
> > support only one bus clock.
> >
> > Also pass clk_id NULL instead "bus" to regmap clock init function
> > since the single clock variants no need to mention clock-names
> > explicitly.
>
> You don't need explicit clock handling. Passing NULL as the argument
> in regmap_init_mmio_clk will make it use the first clock, which is the
> bus clock.

Indeed I tried that, since NULL clk_id wouldn't enable the bus clock
during regmap_mmio_gen_context code, passing NULL triggering vblank
timeout.

Re: [PATCH v11 2/7] dt-bindings: sun6i-dsi: Add A64 DPHY compatible (w/ A31 fallback)

[Jagan Teki]

On Mon, Oct 28, 2019 at 2:47 AM Rob Herring <robh@kernel.org> wrote:
>
> On Fri, 25 Oct 2019 23:26:20 +0530, Jagan Teki wrote:
> > The MIPI DSI PHY controller on Allwinner A64 is similar
> > on the one on A31.
> >
> > Add A64 compatible and append A31 compatible as fallback.
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> >  .../bindings/phy/allwinner,sun6i-a31-mipi-dphy.yaml         | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
>
> Please add Acked-by/Reviewed-by tags when posting new versions. However,
> there's no need to repost patches *only* to add the tags. The upstream
> maintainer will do that for acks received on the version they apply.
>
> If a tag was not added on purpose, please state why and what changed.

I usually collect the tags when I send next version w/o any change.
but this dt-binding patch has a fixed version compared to previous
version. I have updated changelog on cover patch and may be will write
it on respective patch itself.

Re: [PATCH v11 4/7] drm/sun4i: dsi: Handle bus clock explicitly

[Maxime Ripard]

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

On Tue, Oct 29, 2019 at 04:03:56AM +0530, Jagan Teki wrote:
> > > explicit handling of common clock would require since the A64
> > > doesn't need to mention the clock-names explicitly in dts since it
> > > support only one bus clock.
> > >
> > > Also pass clk_id NULL instead "bus" to regmap clock init function
> > > since the single clock variants no need to mention clock-names
> > > explicitly.
> >
> > You don't need explicit clock handling. Passing NULL as the argument
> > in regmap_init_mmio_clk will make it use the first clock, which is the
> > bus clock.
>
> Indeed I tried that, since NULL clk_id wouldn't enable the bus clock
> during regmap_mmio_gen_context code, passing NULL triggering vblank
> timeout.

There's a bunch of users of NULL in tree, so finding out why NULL
doesn't work is the way forward.

Maxime

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

Re: [PATCH v11 1/7] dt-bindings: sun6i-dsi: Document A64 MIPI-DSI controller

[Rob Herring]

On Fri, Oct 25, 2019 at 11:26:19PM +0530, Jagan Teki wrote:
> The MIPI DSI controller in Allwinner A64 is similar to A33.
> 
> But unlike A33, A64 doesn't have DSI_SCLK gating so it is valid
> to have separate compatible for A64 on the same driver.
> 
> DSI_SCLK uses mod clock-names on dt-bindings, so the same
> is not required for A64.
> 
> On that note
> - A64 require minimum of 1 clock like the bus clock
> - A33 require minimum of 2 clocks like both bus, mod clocks
> 
> So, update dt-bindings so-that it can document both A33,
> A64 bindings requirements.
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>  .../display/allwinner,sun6i-a31-mipi-dsi.yaml | 20 +++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml
> index dafc0980c4fa..2b7016ca382c 100644
> --- a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml
> +++ b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml
> @@ -15,7 +15,9 @@ properties:
>    "#size-cells": true
>  
>    compatible:
> -    const: allwinner,sun6i-a31-mipi-dsi
> +    oneOf:
> +      - const: allwinner,sun6i-a31-mipi-dsi
> +      - const: allwinner,sun50i-a64-mipi-dsi

Use 'enum' instead of oneOf+const.

With that fixed,

Reviewed-by: Rob Herring <robh@kernel.org>

>  
>    reg:
>      maxItems: 1
> @@ -24,6 +26,8 @@ properties:
>      maxItems: 1
>  
>    clocks:
> +    minItems: 1
> +    maxItems: 2
>      items:
>        - description: Bus Clock
>        - description: Module Clock
> @@ -63,13 +67,25 @@ required:
>    - reg
>    - interrupts
>    - clocks
> -  - clock-names
>    - phys
>    - phy-names
>    - resets
>    - vcc-dsi-supply
>    - port
>  
> +allOf:
> +  - if:
> +      properties:
> +         compatible:
> +           contains:
> +             const: allwinner,sun6i-a31-mipi-dsi
> +      then:
> +        properties:
> +          clocks:
> +            minItems: 2
> +        required:
> +          - clock-names
> +
>  additionalProperties: false
>  
>  examples:
> -- 
> 2.18.0.321.gffc6fa0e3
> 

Re: [PATCH v11 4/7] drm/sun4i: dsi: Handle bus clock explicitly

[Jagan Teki]

Hi Maxime,

On Tue, Oct 29, 2019 at 2:24 PM Maxime Ripard <mripard@kernel.org> wrote:
>
> On Tue, Oct 29, 2019 at 04:03:56AM +0530, Jagan Teki wrote:
> > > > explicit handling of common clock would require since the A64
> > > > doesn't need to mention the clock-names explicitly in dts since it
> > > > support only one bus clock.
> > > >
> > > > Also pass clk_id NULL instead "bus" to regmap clock init function
> > > > since the single clock variants no need to mention clock-names
> > > > explicitly.
> > >
> > > You don't need explicit clock handling. Passing NULL as the argument
> > > in regmap_init_mmio_clk will make it use the first clock, which is the
> > > bus clock.
> >
> > Indeed I tried that, since NULL clk_id wouldn't enable the bus clock
> > during regmap_mmio_gen_context code, passing NULL triggering vblank
> > timeout.
>
> There's a bunch of users of NULL in tree, so finding out why NULL
> doesn't work is the way forward.

I'd have looked the some of the users before checking the code as
well. As I said passing NULL clk_id to devm_regmap_init_mmio_clk =>
__devm_regmap_init_mmio_clk would return before processing the clock.

Here is the code snippet on the tree just to make sure I'm on the same
page or not.

static struct regmap_mmio_context *regmap_mmio_gen_context(struct device *dev,
                                        const char *clk_id,
                                        void __iomem *regs,
                                        const struct regmap_config *config)
{
        -----------------------
        --------------
        if (clk_id == NULL)
                return ctx;

        ctx->clk = clk_get(dev, clk_id);
        if (IS_ERR(ctx->clk)) {
                ret = PTR_ERR(ctx->clk);
                goto err_free;
        }

        ret = clk_prepare(ctx->clk);
        if (ret < 0) {
                clk_put(ctx->clk);
                goto err_free;
        }
        -------------
        ---------------
}

Yes, I did check on the driver in the tree before committing explicit
clock handle, which make similar requirements like us in [1]. this
imx2 wdt driver is handling the explicit clock as well. I'm sure this
driver is updated as I have seen few changes related to this driver in
ML.

Let me know if I still miss any key change or note here, I will dig
further on this for sure.

[1] https://elixir.bootlin.com/linux/v5.4-rc4/source/drivers/watchdog/imx2_wdt.c#L264

thanks,
Jagan.

Re: [PATCH v11 4/7] drm/sun4i: dsi: Handle bus clock explicitly

[Maxime Ripard]

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

On Fri, Nov 01, 2019 at 07:42:55PM +0530, Jagan Teki wrote:
> Hi Maxime,
>
> On Tue, Oct 29, 2019 at 2:24 PM Maxime Ripard <mripard@kernel.org> wrote:
> >
> > On Tue, Oct 29, 2019 at 04:03:56AM +0530, Jagan Teki wrote:
> > > > > explicit handling of common clock would require since the A64
> > > > > doesn't need to mention the clock-names explicitly in dts since it
> > > > > support only one bus clock.
> > > > >
> > > > > Also pass clk_id NULL instead "bus" to regmap clock init function
> > > > > since the single clock variants no need to mention clock-names
> > > > > explicitly.
> > > >
> > > > You don't need explicit clock handling. Passing NULL as the argument
> > > > in regmap_init_mmio_clk will make it use the first clock, which is the
> > > > bus clock.
> > >
> > > Indeed I tried that, since NULL clk_id wouldn't enable the bus clock
> > > during regmap_mmio_gen_context code, passing NULL triggering vblank
> > > timeout.
> >
> > There's a bunch of users of NULL in tree, so finding out why NULL
> > doesn't work is the way forward.
>
> I'd have looked the some of the users before checking the code as
> well. As I said passing NULL clk_id to devm_regmap_init_mmio_clk =>
> __devm_regmap_init_mmio_clk would return before processing the clock.
>
> Here is the code snippet on the tree just to make sure I'm on the same
> page or not.
>
> static struct regmap_mmio_context *regmap_mmio_gen_context(struct device *dev,
>                                         const char *clk_id,
>                                         void __iomem *regs,
>                                         const struct regmap_config *config)
> {
>         -----------------------
>         --------------
>         if (clk_id == NULL)
>                 return ctx;
>
>         ctx->clk = clk_get(dev, clk_id);
>         if (IS_ERR(ctx->clk)) {
>                 ret = PTR_ERR(ctx->clk);
>                 goto err_free;
>         }
>
>         ret = clk_prepare(ctx->clk);
>         if (ret < 0) {
>                 clk_put(ctx->clk);
>                 goto err_free;
>         }
>         -------------
>         ---------------
> }
>
> Yes, I did check on the driver in the tree before committing explicit
> clock handle, which make similar requirements like us in [1]. this
> imx2 wdt driver is handling the explicit clock as well. I'm sure this
> driver is updated as I have seen few changes related to this driver in
> ML.

I guess we have two ways to go at this then.

Either we remove the return, but it might have a few side-effects, or
we call clk_get with NULL or bus depending on the case, and then call
regmap_mmio_attach_clk.

Maxime

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

Re: [PATCH v11 4/7] drm/sun4i: dsi: Handle bus clock explicitly

[Jagan Teki]

Hi Maxime,

On Sun, Nov 3, 2019 at 11:02 PM Maxime Ripard <mripard@kernel.org> wrote:
>
> On Fri, Nov 01, 2019 at 07:42:55PM +0530, Jagan Teki wrote:
> > Hi Maxime,
> >
> > On Tue, Oct 29, 2019 at 2:24 PM Maxime Ripard <mripard@kernel.org> wrote:
> > >
> > > On Tue, Oct 29, 2019 at 04:03:56AM +0530, Jagan Teki wrote:
> > > > > > explicit handling of common clock would require since the A64
> > > > > > doesn't need to mention the clock-names explicitly in dts since it
> > > > > > support only one bus clock.
> > > > > >
> > > > > > Also pass clk_id NULL instead "bus" to regmap clock init function
> > > > > > since the single clock variants no need to mention clock-names
> > > > > > explicitly.
> > > > >
> > > > > You don't need explicit clock handling. Passing NULL as the argument
> > > > > in regmap_init_mmio_clk will make it use the first clock, which is the
> > > > > bus clock.
> > > >
> > > > Indeed I tried that, since NULL clk_id wouldn't enable the bus clock
> > > > during regmap_mmio_gen_context code, passing NULL triggering vblank
> > > > timeout.
> > >
> > > There's a bunch of users of NULL in tree, so finding out why NULL
> > > doesn't work is the way forward.
> >
> > I'd have looked the some of the users before checking the code as
> > well. As I said passing NULL clk_id to devm_regmap_init_mmio_clk =>
> > __devm_regmap_init_mmio_clk would return before processing the clock.
> >
> > Here is the code snippet on the tree just to make sure I'm on the same
> > page or not.
> >
> > static struct regmap_mmio_context *regmap_mmio_gen_context(struct device *dev,
> >                                         const char *clk_id,
> >                                         void __iomem *regs,
> >                                         const struct regmap_config *config)
> > {
> >         -----------------------
> >         --------------
> >         if (clk_id == NULL)
> >                 return ctx;
> >
> >         ctx->clk = clk_get(dev, clk_id);
> >         if (IS_ERR(ctx->clk)) {
> >                 ret = PTR_ERR(ctx->clk);
> >                 goto err_free;
> >         }
> >
> >         ret = clk_prepare(ctx->clk);
> >         if (ret < 0) {
> >                 clk_put(ctx->clk);
> >                 goto err_free;
> >         }
> >         -------------
> >         ---------------
> > }
> >
> > Yes, I did check on the driver in the tree before committing explicit
> > clock handle, which make similar requirements like us in [1]. this
> > imx2 wdt driver is handling the explicit clock as well. I'm sure this
> > driver is updated as I have seen few changes related to this driver in
> > ML.
>
> I guess we have two ways to go at this then.
>
> Either we remove the return, but it might have a few side-effects, or
> we call clk_get with NULL or bus depending on the case, and then call
> regmap_mmio_attach_clk.

Thanks for the inputs.

Please have a look at this snippet, I have used your second
suggestions. let me know if you have any comments?

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index 8fa90cfc2ac8..91c95e56d870 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -1109,24 +1109,36 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
         return PTR_ERR(dsi->regulator);
     }

-    dsi->regs = devm_regmap_init_mmio_clk(dev, "bus", base,
-                          &sun6i_dsi_regmap_config);
-    if (IS_ERR(dsi->regs)) {
-        dev_err(dev, "Couldn't create the DSI encoder regmap\n");
-        return PTR_ERR(dsi->regs);
-    }
-
     dsi->reset = devm_reset_control_get_shared(dev, NULL);
     if (IS_ERR(dsi->reset)) {
         dev_err(dev, "Couldn't get our reset line\n");
         return PTR_ERR(dsi->reset);
     }

+    dsi->regs = regmap_init_mmio(dev, base, &sun6i_dsi_regmap_config);
+    if (IS_ERR(dsi->regs)) {
+        dev_err(dev, "Couldn't init regmap\n");
+        return PTR_ERR(dsi->regs);
+    }
+
+    dsi->bus_clk = devm_clk_get(dev, NULL);
+    if (IS_ERR(dsi->bus_clk)) {
+        dev_err(dev, "Couldn't get the DSI bus clock\n");
+        ret = PTR_ERR(dsi->bus_clk);
+        goto err_regmap;
+    } else {
+        printk("Jagan.. Got the BUS clock\n");
+        ret = regmap_mmio_attach_clk(dsi->regs, dsi->bus_clk);
+        if (ret)
+            goto err_bus_clk;
+    }
+
     if (dsi->variant->has_mod_clk) {
         dsi->mod_clk = devm_clk_get(dev, "mod");
         if (IS_ERR(dsi->mod_clk)) {
             dev_err(dev, "Couldn't get the DSI mod clock\n");
-            return PTR_ERR(dsi->mod_clk);
+            ret = PTR_ERR(dsi->mod_clk);
+            goto err_attach_clk;
         }
     }

@@ -1167,6 +1179,14 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
 err_unprotect_clk:
     if (dsi->variant->has_mod_clk)
         clk_rate_exclusive_put(dsi->mod_clk);
+err_attach_clk:
+    if (!IS_ERR(dsi->bus_clk))
+        regmap_mmio_detach_clk(dsi->regs);
+err_bus_clk:
+    if (!IS_ERR(dsi->bus_clk))
+        clk_put(dsi->bus_clk);
+err_regmap:
+    regmap_exit(dsi->regs);
     return ret;
 }

@@ -1181,6 +1201,13 @@ static int sun6i_dsi_remove(struct platform_device *pdev)
     if (dsi->variant->has_mod_clk)
         clk_rate_exclusive_put(dsi->mod_clk);

+    if (!IS_ERR(dsi->bus_clk)) {
+        regmap_mmio_detach_clk(dsi->regs);
+        clk_put(dsi->bus_clk);
+    }
+
+    regmap_exit(dsi->regs);
+
     return 0;
 }


Jagan.

Re: [PATCH v11 4/7] drm/sun4i: dsi: Handle bus clock explicitly

[Maxime Ripard]

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

Hi,

On Thu, Nov 21, 2019 at 05:24:47PM +0530, Jagan Teki wrote:
> On Sun, Nov 3, 2019 at 11:02 PM Maxime Ripard <mripard@kernel.org> wrote:
> >
> > On Fri, Nov 01, 2019 at 07:42:55PM +0530, Jagan Teki wrote:
> > > Hi Maxime,
> > >
> > > On Tue, Oct 29, 2019 at 2:24 PM Maxime Ripard <mripard@kernel.org> wrote:
> > > >
> > > > On Tue, Oct 29, 2019 at 04:03:56AM +0530, Jagan Teki wrote:
> > > > > > > explicit handling of common clock would require since the A64
> > > > > > > doesn't need to mention the clock-names explicitly in dts since it
> > > > > > > support only one bus clock.
> > > > > > >
> > > > > > > Also pass clk_id NULL instead "bus" to regmap clock init function
> > > > > > > since the single clock variants no need to mention clock-names
> > > > > > > explicitly.
> > > > > >
> > > > > > You don't need explicit clock handling. Passing NULL as the argument
> > > > > > in regmap_init_mmio_clk will make it use the first clock, which is the
> > > > > > bus clock.
> > > > >
> > > > > Indeed I tried that, since NULL clk_id wouldn't enable the bus clock
> > > > > during regmap_mmio_gen_context code, passing NULL triggering vblank
> > > > > timeout.
> > > >
> > > > There's a bunch of users of NULL in tree, so finding out why NULL
> > > > doesn't work is the way forward.
> > >
> > > I'd have looked the some of the users before checking the code as
> > > well. As I said passing NULL clk_id to devm_regmap_init_mmio_clk =>
> > > __devm_regmap_init_mmio_clk would return before processing the clock.
> > >
> > > Here is the code snippet on the tree just to make sure I'm on the same
> > > page or not.
> > >
> > > static struct regmap_mmio_context *regmap_mmio_gen_context(struct device *dev,
> > >                                         const char *clk_id,
> > >                                         void __iomem *regs,
> > >                                         const struct regmap_config *config)
> > > {
> > >         -----------------------
> > >         --------------
> > >         if (clk_id == NULL)
> > >                 return ctx;
> > >
> > >         ctx->clk = clk_get(dev, clk_id);
> > >         if (IS_ERR(ctx->clk)) {
> > >                 ret = PTR_ERR(ctx->clk);
> > >                 goto err_free;
> > >         }
> > >
> > >         ret = clk_prepare(ctx->clk);
> > >         if (ret < 0) {
> > >                 clk_put(ctx->clk);
> > >                 goto err_free;
> > >         }
> > >         -------------
> > >         ---------------
> > > }
> > >
> > > Yes, I did check on the driver in the tree before committing explicit
> > > clock handle, which make similar requirements like us in [1]. this
> > > imx2 wdt driver is handling the explicit clock as well. I'm sure this
> > > driver is updated as I have seen few changes related to this driver in
> > > ML.
> >
> > I guess we have two ways to go at this then.
> >
> > Either we remove the return, but it might have a few side-effects, or
> > we call clk_get with NULL or bus depending on the case, and then call
> > regmap_mmio_attach_clk.
>
> Thanks for the inputs.
>
> Please have a look at this snippet, I have used your second
> suggestions. let me know if you have any comments?
>
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index 8fa90cfc2ac8..91c95e56d870 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -1109,24 +1109,36 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
>          return PTR_ERR(dsi->regulator);
>      }
>
> -    dsi->regs = devm_regmap_init_mmio_clk(dev, "bus", base,
> -                          &sun6i_dsi_regmap_config);
> -    if (IS_ERR(dsi->regs)) {
> -        dev_err(dev, "Couldn't create the DSI encoder regmap\n");
> -        return PTR_ERR(dsi->regs);
> -    }
> -
>      dsi->reset = devm_reset_control_get_shared(dev, NULL);
>      if (IS_ERR(dsi->reset)) {
>          dev_err(dev, "Couldn't get our reset line\n");
>          return PTR_ERR(dsi->reset);
>      }
>
> +    dsi->regs = regmap_init_mmio(dev, base, &sun6i_dsi_regmap_config);

You should use the devm variant here

> +    if (IS_ERR(dsi->regs)) {
> +        dev_err(dev, "Couldn't init regmap\n");
> +        return PTR_ERR(dsi->regs);
> +    }
> +
> +    dsi->bus_clk = devm_clk_get(dev, NULL);

I guess you still need to pass 'bus' here?

> +    if (IS_ERR(dsi->bus_clk)) {
> +        dev_err(dev, "Couldn't get the DSI bus clock\n");
> +        ret = PTR_ERR(dsi->bus_clk);
> +        goto err_regmap;
> +    } else {
> +        printk("Jagan.. Got the BUS clock\n");
> +        ret = regmap_mmio_attach_clk(dsi->regs, dsi->bus_clk);
> +        if (ret)
> +            goto err_bus_clk;
> +    }
> +
>      if (dsi->variant->has_mod_clk) {
>          dsi->mod_clk = devm_clk_get(dev, "mod");
>          if (IS_ERR(dsi->mod_clk)) {
>              dev_err(dev, "Couldn't get the DSI mod clock\n");
> -            return PTR_ERR(dsi->mod_clk);
> +            ret = PTR_ERR(dsi->mod_clk);
> +            goto err_attach_clk;
>          }
>      }
>
> @@ -1167,6 +1179,14 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
>  err_unprotect_clk:
>      if (dsi->variant->has_mod_clk)
>          clk_rate_exclusive_put(dsi->mod_clk);
> +err_attach_clk:
> +    if (!IS_ERR(dsi->bus_clk))
> +        regmap_mmio_detach_clk(dsi->regs);
> +err_bus_clk:
> +    if (!IS_ERR(dsi->bus_clk))
> +        clk_put(dsi->bus_clk);
> +err_regmap:
> +    regmap_exit(dsi->regs);
>      return ret;
>  }
>
> @@ -1181,6 +1201,13 @@ static int sun6i_dsi_remove(struct platform_device *pdev)
>      if (dsi->variant->has_mod_clk)
>          clk_rate_exclusive_put(dsi->mod_clk);
>
> +    if (!IS_ERR(dsi->bus_clk)) {
> +        regmap_mmio_detach_clk(dsi->regs);
> +        clk_put(dsi->bus_clk);

This will trigger a warning, you put down the reference twice

Maxime

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

Re: [PATCH v11 4/7] drm/sun4i: dsi: Handle bus clock explicitly

[Jagan Teki]

Hi,

On Fri, Nov 22, 2019 at 11:48 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi,
>
> On Thu, Nov 21, 2019 at 05:24:47PM +0530, Jagan Teki wrote:
> > On Sun, Nov 3, 2019 at 11:02 PM Maxime Ripard <mripard@kernel.org> wrote:
> > >
> > > On Fri, Nov 01, 2019 at 07:42:55PM +0530, Jagan Teki wrote:
> > > > Hi Maxime,
> > > >
> > > > On Tue, Oct 29, 2019 at 2:24 PM Maxime Ripard <mripard@kernel.org> wrote:
> > > > >
> > > > > On Tue, Oct 29, 2019 at 04:03:56AM +0530, Jagan Teki wrote:
> > > > > > > > explicit handling of common clock would require since the A64
> > > > > > > > doesn't need to mention the clock-names explicitly in dts since it
> > > > > > > > support only one bus clock.
> > > > > > > >
> > > > > > > > Also pass clk_id NULL instead "bus" to regmap clock init function
> > > > > > > > since the single clock variants no need to mention clock-names
> > > > > > > > explicitly.
> > > > > > >
> > > > > > > You don't need explicit clock handling. Passing NULL as the argument
> > > > > > > in regmap_init_mmio_clk will make it use the first clock, which is the
> > > > > > > bus clock.
> > > > > >
> > > > > > Indeed I tried that, since NULL clk_id wouldn't enable the bus clock
> > > > > > during regmap_mmio_gen_context code, passing NULL triggering vblank
> > > > > > timeout.
> > > > >
> > > > > There's a bunch of users of NULL in tree, so finding out why NULL
> > > > > doesn't work is the way forward.
> > > >
> > > > I'd have looked the some of the users before checking the code as
> > > > well. As I said passing NULL clk_id to devm_regmap_init_mmio_clk =>
> > > > __devm_regmap_init_mmio_clk would return before processing the clock.
> > > >
> > > > Here is the code snippet on the tree just to make sure I'm on the same
> > > > page or not.
> > > >
> > > > static struct regmap_mmio_context *regmap_mmio_gen_context(struct device *dev,
> > > >                                         const char *clk_id,
> > > >                                         void __iomem *regs,
> > > >                                         const struct regmap_config *config)
> > > > {
> > > >         -----------------------
> > > >         --------------
> > > >         if (clk_id == NULL)
> > > >                 return ctx;
> > > >
> > > >         ctx->clk = clk_get(dev, clk_id);
> > > >         if (IS_ERR(ctx->clk)) {
> > > >                 ret = PTR_ERR(ctx->clk);
> > > >                 goto err_free;
> > > >         }
> > > >
> > > >         ret = clk_prepare(ctx->clk);
> > > >         if (ret < 0) {
> > > >                 clk_put(ctx->clk);
> > > >                 goto err_free;
> > > >         }
> > > >         -------------
> > > >         ---------------
> > > > }
> > > >
> > > > Yes, I did check on the driver in the tree before committing explicit
> > > > clock handle, which make similar requirements like us in [1]. this
> > > > imx2 wdt driver is handling the explicit clock as well. I'm sure this
> > > > driver is updated as I have seen few changes related to this driver in
> > > > ML.
> > >
> > > I guess we have two ways to go at this then.
> > >
> > > Either we remove the return, but it might have a few side-effects, or
> > > we call clk_get with NULL or bus depending on the case, and then call
> > > regmap_mmio_attach_clk.
> >
> > Thanks for the inputs.
> >
> > Please have a look at this snippet, I have used your second
> > suggestions. let me know if you have any comments?
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > index 8fa90cfc2ac8..91c95e56d870 100644
> > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > @@ -1109,24 +1109,36 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
> >          return PTR_ERR(dsi->regulator);
> >      }
> >
> > -    dsi->regs = devm_regmap_init_mmio_clk(dev, "bus", base,
> > -                          &sun6i_dsi_regmap_config);
> > -    if (IS_ERR(dsi->regs)) {
> > -        dev_err(dev, "Couldn't create the DSI encoder regmap\n");
> > -        return PTR_ERR(dsi->regs);
> > -    }
> > -
> >      dsi->reset = devm_reset_control_get_shared(dev, NULL);
> >      if (IS_ERR(dsi->reset)) {
> >          dev_err(dev, "Couldn't get our reset line\n");
> >          return PTR_ERR(dsi->reset);
> >      }
> >
> > +    dsi->regs = regmap_init_mmio(dev, base, &sun6i_dsi_regmap_config);
>
> You should use the devm variant here

Sure.

>
> > +    if (IS_ERR(dsi->regs)) {
> > +        dev_err(dev, "Couldn't init regmap\n");
> > +        return PTR_ERR(dsi->regs);
> > +    }
> > +
> > +    dsi->bus_clk = devm_clk_get(dev, NULL);
>
> I guess you still need to pass 'bus' here?

But the idea here is not to specify clock name explicitly to support
A64. otherwise A64 would fail as we are not specifying the clock-names
explicitly on dsi node.

dsi: dsi@1ca0000 {
       compatible = "allwinner,sun50i-a64-mipi-dsi";
       reg = <0x01ca0000 0x1000>;
       interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
       clocks = <&ccu CLK_BUS_MIPI_DSI>;
       resets = <&ccu RST_BUS_MIPI_DSI>;
      phys = <&dphy>;
      phy-names = "dphy";
.....
};

>
> > +    if (IS_ERR(dsi->bus_clk)) {
> > +        dev_err(dev, "Couldn't get the DSI bus clock\n");
> > +        ret = PTR_ERR(dsi->bus_clk);
> > +        goto err_regmap;
> > +    } else {
> > +        printk("Jagan.. Got the BUS clock\n");
> > +        ret = regmap_mmio_attach_clk(dsi->regs, dsi->bus_clk);
> > +        if (ret)
> > +            goto err_bus_clk;
> > +    }
> > +
> >      if (dsi->variant->has_mod_clk) {
> >          dsi->mod_clk = devm_clk_get(dev, "mod");
> >          if (IS_ERR(dsi->mod_clk)) {
> >              dev_err(dev, "Couldn't get the DSI mod clock\n");
> > -            return PTR_ERR(dsi->mod_clk);
> > +            ret = PTR_ERR(dsi->mod_clk);
> > +            goto err_attach_clk;
> >          }
> >      }
> >
> > @@ -1167,6 +1179,14 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
> >  err_unprotect_clk:
> >      if (dsi->variant->has_mod_clk)
> >          clk_rate_exclusive_put(dsi->mod_clk);
> > +err_attach_clk:
> > +    if (!IS_ERR(dsi->bus_clk))
> > +        regmap_mmio_detach_clk(dsi->regs);
> > +err_bus_clk:
> > +    if (!IS_ERR(dsi->bus_clk))
> > +        clk_put(dsi->bus_clk);
> > +err_regmap:
> > +    regmap_exit(dsi->regs);
> >      return ret;
> >  }
> >
> > @@ -1181,6 +1201,13 @@ static int sun6i_dsi_remove(struct platform_device *pdev)
> >      if (dsi->variant->has_mod_clk)
> >          clk_rate_exclusive_put(dsi->mod_clk);
> >
> > +    if (!IS_ERR(dsi->bus_clk)) {
> > +        regmap_mmio_detach_clk(dsi->regs);
> > +        clk_put(dsi->bus_clk);
>
> This will trigger a warning, you put down the reference twice

You mean regmap_mmio_detach_clk will put the clk?

Jagan.

Re: [PATCH v11 4/7] drm/sun4i: dsi: Handle bus clock explicitly

[Maxime Ripard]

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

On Sat, Nov 23, 2019 at 01:20:21AM +0530, Jagan Teki wrote:
> > > Please have a look at this snippet, I have used your second
> > > suggestions. let me know if you have any comments?
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > index 8fa90cfc2ac8..91c95e56d870 100644
> > > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > @@ -1109,24 +1109,36 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
> > >          return PTR_ERR(dsi->regulator);
> > >      }
> > >
> > > -    dsi->regs = devm_regmap_init_mmio_clk(dev, "bus", base,
> > > -                          &sun6i_dsi_regmap_config);
> > > -    if (IS_ERR(dsi->regs)) {
> > > -        dev_err(dev, "Couldn't create the DSI encoder regmap\n");
> > > -        return PTR_ERR(dsi->regs);
> > > -    }
> > > -
> > >      dsi->reset = devm_reset_control_get_shared(dev, NULL);
> > >      if (IS_ERR(dsi->reset)) {
> > >          dev_err(dev, "Couldn't get our reset line\n");
> > >          return PTR_ERR(dsi->reset);
> > >      }
> > >
> > > +    dsi->regs = regmap_init_mmio(dev, base, &sun6i_dsi_regmap_config);
> >
> > You should use the devm variant here
>
> Sure.
>
> >
> > > +    if (IS_ERR(dsi->regs)) {
> > > +        dev_err(dev, "Couldn't init regmap\n");
> > > +        return PTR_ERR(dsi->regs);
> > > +    }
> > > +
> > > +    dsi->bus_clk = devm_clk_get(dev, NULL);
> >
> > I guess you still need to pass 'bus' here?
>
> But the idea here is not to specify clock name explicitly to support
> A64. otherwise A64 would fail as we are not specifying the clock-names
> explicitly on dsi node.

Right. But you have no guarantee that the bus clock is going to be the
first one on the other SoCs either.

What about something like that instead:

char *clk_name = NULL;
if (dsi->has_mod_clk)
    clk_name = "bus";

clk = devm_clk_get(dev, clk_name);
if (IS_ERR(clk))
    return PTR_ERR(clk));

regmap_mmio_attach_clk(regmap, clk);

>
> dsi: dsi@1ca0000 {
>        compatible = "allwinner,sun50i-a64-mipi-dsi";
>        reg = <0x01ca0000 0x1000>;
>        interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
>        clocks = <&ccu CLK_BUS_MIPI_DSI>;
>        resets = <&ccu RST_BUS_MIPI_DSI>;
>       phys = <&dphy>;
>       phy-names = "dphy";
> .....
> };
>
> >
> > > +    if (IS_ERR(dsi->bus_clk)) {
> > > +        dev_err(dev, "Couldn't get the DSI bus clock\n");
> > > +        ret = PTR_ERR(dsi->bus_clk);
> > > +        goto err_regmap;
> > > +    } else {
> > > +        printk("Jagan.. Got the BUS clock\n");
> > > +        ret = regmap_mmio_attach_clk(dsi->regs, dsi->bus_clk);
> > > +        if (ret)
> > > +            goto err_bus_clk;
> > > +    }
> > > +
> > >      if (dsi->variant->has_mod_clk) {
> > >          dsi->mod_clk = devm_clk_get(dev, "mod");
> > >          if (IS_ERR(dsi->mod_clk)) {
> > >              dev_err(dev, "Couldn't get the DSI mod clock\n");
> > > -            return PTR_ERR(dsi->mod_clk);
> > > +            ret = PTR_ERR(dsi->mod_clk);
> > > +            goto err_attach_clk;
> > >          }
> > >      }
> > >
> > > @@ -1167,6 +1179,14 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
> > >  err_unprotect_clk:
> > >      if (dsi->variant->has_mod_clk)
> > >          clk_rate_exclusive_put(dsi->mod_clk);
> > > +err_attach_clk:
> > > +    if (!IS_ERR(dsi->bus_clk))
> > > +        regmap_mmio_detach_clk(dsi->regs);
> > > +err_bus_clk:
> > > +    if (!IS_ERR(dsi->bus_clk))
> > > +        clk_put(dsi->bus_clk);
> > > +err_regmap:
> > > +    regmap_exit(dsi->regs);
> > >      return ret;
> > >  }
> > >
> > > @@ -1181,6 +1201,13 @@ static int sun6i_dsi_remove(struct platform_device *pdev)
> > >      if (dsi->variant->has_mod_clk)
> > >          clk_rate_exclusive_put(dsi->mod_clk);
> > >
> > > +    if (!IS_ERR(dsi->bus_clk)) {
> > > +        regmap_mmio_detach_clk(dsi->regs);
> > > +        clk_put(dsi->bus_clk);
> >
> > This will trigger a warning, you put down the reference twice
>
> You mean regmap_mmio_detach_clk will put the clk?

No, devm_clk_get will.

Maxime

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

Re: [PATCH v11 4/7] drm/sun4i: dsi: Handle bus clock explicitly

[Jagan Teki]

On Thu, Nov 28, 2019 at 11:21 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Sat, Nov 23, 2019 at 01:20:21AM +0530, Jagan Teki wrote:
> > > > Please have a look at this snippet, I have used your second
> > > > suggestions. let me know if you have any comments?
> > > >
> > > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > > b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > > index 8fa90cfc2ac8..91c95e56d870 100644
> > > > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> > > > @@ -1109,24 +1109,36 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
> > > >          return PTR_ERR(dsi->regulator);
> > > >      }
> > > >
> > > > -    dsi->regs = devm_regmap_init_mmio_clk(dev, "bus", base,
> > > > -                          &sun6i_dsi_regmap_config);
> > > > -    if (IS_ERR(dsi->regs)) {
> > > > -        dev_err(dev, "Couldn't create the DSI encoder regmap\n");
> > > > -        return PTR_ERR(dsi->regs);
> > > > -    }
> > > > -
> > > >      dsi->reset = devm_reset_control_get_shared(dev, NULL);
> > > >      if (IS_ERR(dsi->reset)) {
> > > >          dev_err(dev, "Couldn't get our reset line\n");
> > > >          return PTR_ERR(dsi->reset);
> > > >      }
> > > >
> > > > +    dsi->regs = regmap_init_mmio(dev, base, &sun6i_dsi_regmap_config);
> > >
> > > You should use the devm variant here
> >
> > Sure.
> >
> > >
> > > > +    if (IS_ERR(dsi->regs)) {
> > > > +        dev_err(dev, "Couldn't init regmap\n");
> > > > +        return PTR_ERR(dsi->regs);
> > > > +    }
> > > > +
> > > > +    dsi->bus_clk = devm_clk_get(dev, NULL);
> > >
> > > I guess you still need to pass 'bus' here?
> >
> > But the idea here is not to specify clock name explicitly to support
> > A64. otherwise A64 would fail as we are not specifying the clock-names
> > explicitly on dsi node.
>
> Right. But you have no guarantee that the bus clock is going to be the
> first one on the other SoCs either.
>
> What about something like that instead:
>
> char *clk_name = NULL;
> if (dsi->has_mod_clk)
>     clk_name = "bus";
>
> clk = devm_clk_get(dev, clk_name);
> if (IS_ERR(clk))
>     return PTR_ERR(clk));
>
> regmap_mmio_attach_clk(regmap, clk);

This makes sense, thanks for your input. I have tested in A33, A64.

>
> >
> > dsi: dsi@1ca0000 {
> >        compatible = "allwinner,sun50i-a64-mipi-dsi";
> >        reg = <0x01ca0000 0x1000>;
> >        interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
> >        clocks = <&ccu CLK_BUS_MIPI_DSI>;
> >        resets = <&ccu RST_BUS_MIPI_DSI>;
> >       phys = <&dphy>;
> >       phy-names = "dphy";
> > .....
> > };
> >
> > >
> > > > +    if (IS_ERR(dsi->bus_clk)) {
> > > > +        dev_err(dev, "Couldn't get the DSI bus clock\n");
> > > > +        ret = PTR_ERR(dsi->bus_clk);
> > > > +        goto err_regmap;
> > > > +    } else {
> > > > +        printk("Jagan.. Got the BUS clock\n");
> > > > +        ret = regmap_mmio_attach_clk(dsi->regs, dsi->bus_clk);
> > > > +        if (ret)
> > > > +            goto err_bus_clk;
> > > > +    }
> > > > +
> > > >      if (dsi->variant->has_mod_clk) {
> > > >          dsi->mod_clk = devm_clk_get(dev, "mod");
> > > >          if (IS_ERR(dsi->mod_clk)) {
> > > >              dev_err(dev, "Couldn't get the DSI mod clock\n");
> > > > -            return PTR_ERR(dsi->mod_clk);
> > > > +            ret = PTR_ERR(dsi->mod_clk);
> > > > +            goto err_attach_clk;
> > > >          }
> > > >      }
> > > >
> > > > @@ -1167,6 +1179,14 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
> > > >  err_unprotect_clk:
> > > >      if (dsi->variant->has_mod_clk)
> > > >          clk_rate_exclusive_put(dsi->mod_clk);
> > > > +err_attach_clk:
> > > > +    if (!IS_ERR(dsi->bus_clk))
> > > > +        regmap_mmio_detach_clk(dsi->regs);
> > > > +err_bus_clk:
> > > > +    if (!IS_ERR(dsi->bus_clk))
> > > > +        clk_put(dsi->bus_clk);
> > > > +err_regmap:
> > > > +    regmap_exit(dsi->regs);
> > > >      return ret;
> > > >  }
> > > >
> > > > @@ -1181,6 +1201,13 @@ static int sun6i_dsi_remove(struct platform_device *pdev)
> > > >      if (dsi->variant->has_mod_clk)
> > > >          clk_rate_exclusive_put(dsi->mod_clk);
> > > >
> > > > +    if (!IS_ERR(dsi->bus_clk)) {
> > > > +        regmap_mmio_detach_clk(dsi->regs);
> > > > +        clk_put(dsi->bus_clk);
> > >
> > > This will trigger a warning, you put down the reference twice
> >
> > You mean regmap_mmio_detach_clk will put the clk?
>
> No, devm_clk_get will.

Got it. Will update and send v12.

Jagan.