2022-08-12 07:45:21

by Samuel Holland

[permalink] [raw]
Subject: [PATCH 0/4] drm/sun4i: dsi: Support the A100/D1 controller variant

This series adds support for the digital part of the DSI controller
found in the A100 and D1 SoCs (plus T7, which is not supported by
mainline Linux). There are two changes to the hardware integration:
1) the module clock routes through the TCON TOP, and
2) the separate I/O domain is removed.

The actual register interface appears to be the same as before. The
register definitions in the D1 BSP exactly match the A64 BSP.

The BSP describes this as the "40nm" DSI controller variant. There is
also a "28nm" variant with a different register interface; that one is
found in a different subset of SoCs (V5 and A50).

A100/D1 also come with an updated DPHY, described by the BSP as a
"combo" PHY, which is now also used for LVDS channel 0. (LVDS and DSI
share the same pins on Port D.) Since that is a different subsystem,
I am sending that as a separate series.


Samuel Holland (4):
dt-bindings: display: sun6i-dsi: Fix clock conditional
dt-bindings: display: sun6i-dsi: Add the A100 variant
drm/sun4i: dsi: Add a variant structure
drm/sun4i: dsi: Add the A100 variant

.../display/allwinner,sun6i-a31-mipi-dsi.yaml | 30 +++++++---
drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 58 +++++++++++++------
drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 7 +++
3 files changed, 69 insertions(+), 26 deletions(-)

--
2.35.1


2022-08-12 07:55:04

by Samuel Holland

[permalink] [raw]
Subject: [PATCH 1/4] dt-bindings: display: sun6i-dsi: Fix clock conditional

The A64 case should have limited maxItems, instead of duplicating the
minItems value from the main binding. While here, simplify the binding
by making this an "else" case of the two-clock conditional block.

Fixes: fe5040f2843a ("dt-bindings: sun6i-dsi: Document A64 MIPI-DSI controller")
Signed-off-by: Samuel Holland <[email protected]>
---

.../bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml | 10 ++--------
1 file changed, 2 insertions(+), 8 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 bf0bdf54e5f9..ae55ef3fb1fe 100644
--- a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml
+++ b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml
@@ -78,16 +78,10 @@ allOf:
required:
- clock-names

- - if:
- properties:
- compatible:
- contains:
- const: allwinner,sun50i-a64-mipi-dsi
-
- then:
+ else:
properties:
clocks:
- minItems: 1
+ maxItems: 1

unevaluatedProperties: false

--
2.35.1

2022-08-12 07:56:55

by Samuel Holland

[permalink] [raw]
Subject: [PATCH 2/4] dt-bindings: display: sun6i-dsi: Add the A100 variant

The "40nm" MIPI DSI controller found in the A100 and D1 SoCs has the
same register layout as previous SoC integrations. However, its module
clock now comes from the TCON, which means it no longer runs at a fixed
rate, so this needs to be distinguished in the driver.

The controller also now uses pins on Port D instead of dedicated pins,
so it drops the separate power domain.

Signed-off-by: Samuel Holland <[email protected]>
---
Removal of the vcc-dsi-supply is maybe a bit questionable. Since there
is no "VCC-DSI" pin anymore, it's not obvious which pin actually does
power the DSI controller/PHY. Possibly power comes from VCC-PD or VCC-IO
or VCC-LVDS. So far, all boards have all of these as always-on supplies,
so it is hard to test.

.../display/allwinner,sun6i-a31-mipi-dsi.yaml | 28 +++++++++++++++----
1 file changed, 23 insertions(+), 5 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 ae55ef3fb1fe..c53c25b87bd4 100644
--- a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml
+++ b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml
@@ -12,9 +12,14 @@ maintainers:

properties:
compatible:
- enum:
- - allwinner,sun6i-a31-mipi-dsi
- - allwinner,sun50i-a64-mipi-dsi
+ oneOf:
+ - enum:
+ - allwinner,sun6i-a31-mipi-dsi
+ - allwinner,sun50i-a64-mipi-dsi
+ - allwinner,sun50i-a100-mipi-dsi
+ - items:
+ - const: allwinner,sun20i-d1-mipi-dsi
+ - const: allwinner,sun50i-a100-mipi-dsi

reg:
maxItems: 1
@@ -59,7 +64,6 @@ required:
- phys
- phy-names
- resets
- - vcc-dsi-supply
- port

allOf:
@@ -68,7 +72,9 @@ allOf:
properties:
compatible:
contains:
- const: allwinner,sun6i-a31-mipi-dsi
+ enum:
+ - allwinner,sun6i-a31-mipi-dsi
+ - allwinner,sun50i-a100-mipi-dsi

then:
properties:
@@ -83,6 +89,18 @@ allOf:
clocks:
maxItems: 1

+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - allwinner,sun6i-a31-mipi-dsi
+ - allwinner,sun50i-a64-mipi-dsi
+
+ then:
+ required:
+ - vcc-dsi-supply
+
unevaluatedProperties: false

examples:
--
2.35.1

2022-08-12 08:08:01

by Samuel Holland

[permalink] [raw]
Subject: [PATCH 4/4] drm/sun4i: dsi: Add the A100 variant

The A100 variant of the MIPI DSI controller now gets its module clock
from the TCON via the TCON TOP, so the clock rate cannot be set to a
fixed value. Otherwise, it appears to be the same as the A31 variant.

Signed-off-by: Samuel Holland <[email protected]>
---

drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index 6479ade416b9..5db5ecdc2fc6 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -1222,6 +1222,10 @@ static const struct sun6i_dsi_variant sun6i_a31_mipi_dsi_variant = {
static const struct sun6i_dsi_variant sun50i_a64_mipi_dsi_variant = {
};

+static const struct sun6i_dsi_variant sun50i_a100_mipi_dsi_variant = {
+ .has_mod_clk = true,
+};
+
static const struct of_device_id sun6i_dsi_of_table[] = {
{
.compatible = "allwinner,sun6i-a31-mipi-dsi",
@@ -1231,6 +1235,10 @@ static const struct of_device_id sun6i_dsi_of_table[] = {
.compatible = "allwinner,sun50i-a64-mipi-dsi",
.data = &sun50i_a64_mipi_dsi_variant,
},
+ {
+ .compatible = "allwinner,sun50i-a100-mipi-dsi",
+ .data = &sun50i_a100_mipi_dsi_variant,
+ },
{ }
};
MODULE_DEVICE_TABLE(of, sun6i_dsi_of_table);
--
2.35.1

2022-08-12 08:12:00

by Samuel Holland

[permalink] [raw]
Subject: [PATCH 3/4] drm/sun4i: dsi: Add a variant structure

Replace the ad-hoc calls to of_device_is_compatible() with a structure
describing the differences between variants. This is in preparation for
adding more variants to the driver.

Signed-off-by: Samuel Holland <[email protected]>
---

drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 50 +++++++++++++++++---------
drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 7 ++++
2 files changed, 40 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index b4dfa166eccd..6479ade416b9 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -1101,12 +1101,16 @@ static const struct component_ops sun6i_dsi_ops = {

static int sun6i_dsi_probe(struct platform_device *pdev)
{
+ const struct sun6i_dsi_variant *variant;
struct device *dev = &pdev->dev;
- const char *bus_clk_name = NULL;
struct sun6i_dsi *dsi;
void __iomem *base;
int ret;

+ variant = device_get_match_data(dev);
+ if (!variant)
+ return -EINVAL;
+
dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
if (!dsi)
return -ENOMEM;
@@ -1114,10 +1118,7 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
dsi->dev = dev;
dsi->host.ops = &sun6i_dsi_host_ops;
dsi->host.dev = dev;
-
- if (of_device_is_compatible(dev->of_node,
- "allwinner,sun6i-a31-mipi-dsi"))
- bus_clk_name = "bus";
+ dsi->variant = variant;

base = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(base)) {
@@ -1142,7 +1143,7 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
return PTR_ERR(dsi->regs);
}

- dsi->bus_clk = devm_clk_get(dev, bus_clk_name);
+ dsi->bus_clk = devm_clk_get(dev, variant->has_mod_clk ? "bus" : NULL);
if (IS_ERR(dsi->bus_clk))
return dev_err_probe(dev, PTR_ERR(dsi->bus_clk),
"Couldn't get the DSI bus clock\n");
@@ -1151,21 +1152,21 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
if (ret)
return ret;

- if (of_device_is_compatible(dev->of_node,
- "allwinner,sun6i-a31-mipi-dsi")) {
+ if (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");
ret = PTR_ERR(dsi->mod_clk);
goto err_attach_clk;
}
- }

- /*
- * In order to operate properly, that clock seems to be always
- * set to 297MHz.
- */
- clk_set_rate_exclusive(dsi->mod_clk, 297000000);
+ /*
+ * In order to operate properly, the module clock on the
+ * A31 variant always seems to be set to 297MHz.
+ */
+ if (variant->set_mod_clk)
+ clk_set_rate_exclusive(dsi->mod_clk, 297000000);
+ }

dsi->dphy = devm_phy_get(dev, "dphy");
if (IS_ERR(dsi->dphy)) {
@@ -1205,16 +1206,31 @@ static int sun6i_dsi_remove(struct platform_device *pdev)

component_del(&pdev->dev, &sun6i_dsi_ops);
mipi_dsi_host_unregister(&dsi->host);
- clk_rate_exclusive_put(dsi->mod_clk);
+ if (dsi->variant->has_mod_clk && dsi->variant->set_mod_clk)
+ clk_rate_exclusive_put(dsi->mod_clk);

regmap_mmio_detach_clk(dsi->regs);

return 0;
}

+static const struct sun6i_dsi_variant sun6i_a31_mipi_dsi_variant = {
+ .has_mod_clk = true,
+ .set_mod_clk = true,
+};
+
+static const struct sun6i_dsi_variant sun50i_a64_mipi_dsi_variant = {
+};
+
static const struct of_device_id sun6i_dsi_of_table[] = {
- { .compatible = "allwinner,sun6i-a31-mipi-dsi" },
- { .compatible = "allwinner,sun50i-a64-mipi-dsi" },
+ {
+ .compatible = "allwinner,sun6i-a31-mipi-dsi",
+ .data = &sun6i_a31_mipi_dsi_variant,
+ },
+ {
+ .compatible = "allwinner,sun50i-a64-mipi-dsi",
+ .data = &sun50i_a64_mipi_dsi_variant,
+ },
{ }
};
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 c863900ae3b4..f1ddefe0f554 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h
@@ -15,6 +15,11 @@

#define SUN6I_DSI_TCON_DIV 4

+struct sun6i_dsi_variant {
+ bool has_mod_clk;
+ bool set_mod_clk;
+};
+
struct sun6i_dsi {
struct drm_connector connector;
struct drm_encoder encoder;
@@ -31,6 +36,8 @@ struct sun6i_dsi {
struct mipi_dsi_device *device;
struct drm_device *drm;
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.35.1

2022-08-12 11:14:40

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 1/4] dt-bindings: display: sun6i-dsi: Fix clock conditional

On 12/08/2022 10:42, Samuel Holland wrote:
> The A64 case should have limited maxItems, instead of duplicating the
> minItems value from the main binding. While here, simplify the binding
> by making this an "else" case of the two-clock conditional block.
>
> Fixes: fe5040f2843a ("dt-bindings: sun6i-dsi: Document A64 MIPI-DSI controller")
> Signed-off-by: Samuel Holland <[email protected]>


Acked-by: Krzysztof Kozlowski <[email protected]>


Best regards,
Krzysztof

2022-08-12 11:19:24

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 2/4] dt-bindings: display: sun6i-dsi: Add the A100 variant

On 12/08/2022 10:42, Samuel Holland wrote:
> The "40nm" MIPI DSI controller found in the A100 and D1 SoCs has the
> same register layout as previous SoC integrations. However, its module
> clock now comes from the TCON, which means it no longer runs at a fixed
> rate, so this needs to be distinguished in the driver.
>
> The controller also now uses pins on Port D instead of dedicated pins,
> so it drops the separate power domain.
>
> Signed-off-by: Samuel Holland <[email protected]>
> ---
> Removal of the vcc-dsi-supply is maybe a bit questionable. Since there
> is no "VCC-DSI" pin anymore, it's not obvious which pin actually does
> power the DSI controller/PHY. Possibly power comes from VCC-PD or VCC-IO
> or VCC-LVDS. So far, all boards have all of these as always-on supplies,
> so it is hard to test.
>
> .../display/allwinner,sun6i-a31-mipi-dsi.yaml | 28 +++++++++++++++----
> 1 file changed, 23 insertions(+), 5 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 ae55ef3fb1fe..c53c25b87bd4 100644
> --- a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml
> +++ b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml
> @@ -12,9 +12,14 @@ maintainers:
>
> properties:
> compatible:
> - enum:
> - - allwinner,sun6i-a31-mipi-dsi
> - - allwinner,sun50i-a64-mipi-dsi
> + oneOf:
> + - enum:
> + - allwinner,sun6i-a31-mipi-dsi
> + - allwinner,sun50i-a64-mipi-dsi
> + - allwinner,sun50i-a100-mipi-dsi

While you are moving code, how about bringing alphabetical order?

> + - items:
> + - const: allwinner,sun20i-d1-mipi-dsi
> + - const: allwinner,sun50i-a100-mipi-dsi
>
> reg:
> maxItems: 1
> @@ -59,7 +64,6 @@ required:
> - phys
> - phy-names
> - resets
> - - vcc-dsi-supply
> - port
>
> allOf:
> @@ -68,7 +72,9 @@ allOf:
> properties:
> compatible:
> contains:
> - const: allwinner,sun6i-a31-mipi-dsi
> + enum:
> + - allwinner,sun6i-a31-mipi-dsi
> + - allwinner,sun50i-a100-mipi-dsi

Here as well

>
> then:
> properties:
> @@ -83,6 +89,18 @@ allOf:
> clocks:
> maxItems: 1
>
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - allwinner,sun6i-a31-mipi-dsi
> + - allwinner,sun50i-a64-mipi-dsi

and here


Best regards,
Krzysztof

2022-08-12 23:12:39

by Samuel Holland

[permalink] [raw]
Subject: Re: [PATCH 2/4] dt-bindings: display: sun6i-dsi: Add the A100 variant

Hi Krzysztof,

On 8/12/22 5:49 AM, Krzysztof Kozlowski wrote:
> On 12/08/2022 10:42, Samuel Holland wrote:
>> The "40nm" MIPI DSI controller found in the A100 and D1 SoCs has the
>> same register layout as previous SoC integrations. However, its module
>> clock now comes from the TCON, which means it no longer runs at a fixed
>> rate, so this needs to be distinguished in the driver.
>>
>> The controller also now uses pins on Port D instead of dedicated pins,
>> so it drops the separate power domain.
>>
>> Signed-off-by: Samuel Holland <[email protected]>
>> ---
>> Removal of the vcc-dsi-supply is maybe a bit questionable. Since there
>> is no "VCC-DSI" pin anymore, it's not obvious which pin actually does
>> power the DSI controller/PHY. Possibly power comes from VCC-PD or VCC-IO
>> or VCC-LVDS. So far, all boards have all of these as always-on supplies,
>> so it is hard to test.
>>
>> .../display/allwinner,sun6i-a31-mipi-dsi.yaml | 28 +++++++++++++++----
>> 1 file changed, 23 insertions(+), 5 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 ae55ef3fb1fe..c53c25b87bd4 100644
>> --- a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml
>> +++ b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml
>> @@ -12,9 +12,14 @@ maintainers:
>>
>> properties:
>> compatible:
>> - enum:
>> - - allwinner,sun6i-a31-mipi-dsi
>> - - allwinner,sun50i-a64-mipi-dsi
>> + oneOf:
>> + - enum:
>> + - allwinner,sun6i-a31-mipi-dsi
>> + - allwinner,sun50i-a64-mipi-dsi
>> + - allwinner,sun50i-a100-mipi-dsi
>
> While you are moving code, how about bringing alphabetical order?

I have put the sun*i prefix in numeric order, which matches (almost) all of our
other bindings. It roughly corresponds to chronological order as well. It
doesn't make much sense to me to sort sun50i (ARM64 SoCs) between sun5i and
sun6i (early ARMv7 SoCs).

Regards,
Samuel

2022-08-14 08:05:23

by Jernej Škrabec

[permalink] [raw]
Subject: Re: [PATCH 3/4] drm/sun4i: dsi: Add a variant structure

Dne petek, 12. avgust 2022 ob 09:42:55 CEST je Samuel Holland napisal(a):
> Replace the ad-hoc calls to of_device_is_compatible() with a structure
> describing the differences between variants. This is in preparation for
> adding more variants to the driver.
>
> Signed-off-by: Samuel Holland <[email protected]>

Reviewed-by: Jernej Skrabec <[email protected]>

Best regards,
Jernej


2022-08-14 08:39:29

by Jernej Škrabec

[permalink] [raw]
Subject: Re: [PATCH 4/4] drm/sun4i: dsi: Add the A100 variant

Dne petek, 12. avgust 2022 ob 09:42:56 CEST je Samuel Holland napisal(a):
> The A100 variant of the MIPI DSI controller now gets its module clock
> from the TCON via the TCON TOP, so the clock rate cannot be set to a
> fixed value. Otherwise, it appears to be the same as the A31 variant.
>
> Signed-off-by: Samuel Holland <[email protected]>

Reviewed-by: Jernej Skrabec <[email protected]>

Best regards,
Jernej


2022-08-15 07:03:22

by Maxime Ripard

[permalink] [raw]
Subject: Re: [PATCH 3/4] drm/sun4i: dsi: Add a variant structure

Hi Samuel,

On Fri, Aug 12, 2022 at 02:42:55AM -0500, Samuel Holland wrote:
> Replace the ad-hoc calls to of_device_is_compatible() with a structure
> describing the differences between variants. This is in preparation for
> adding more variants to the driver.
>
> Signed-off-by: Samuel Holland <[email protected]>
> ---
>
> drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 50 +++++++++++++++++---------
> drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 7 ++++
> 2 files changed, 40 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> index b4dfa166eccd..6479ade416b9 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -1101,12 +1101,16 @@ static const struct component_ops sun6i_dsi_ops = {
>
> static int sun6i_dsi_probe(struct platform_device *pdev)
> {
> + const struct sun6i_dsi_variant *variant;
> struct device *dev = &pdev->dev;
> - const char *bus_clk_name = NULL;
> struct sun6i_dsi *dsi;
> void __iomem *base;
> int ret;
>
> + variant = device_get_match_data(dev);
> + if (!variant)
> + return -EINVAL;
> +
> dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
> if (!dsi)
> return -ENOMEM;
> @@ -1114,10 +1118,7 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
> dsi->dev = dev;
> dsi->host.ops = &sun6i_dsi_host_ops;
> dsi->host.dev = dev;
> -
> - if (of_device_is_compatible(dev->of_node,
> - "allwinner,sun6i-a31-mipi-dsi"))
> - bus_clk_name = "bus";
> + dsi->variant = variant;
>
> base = devm_platform_ioremap_resource(pdev, 0);
> if (IS_ERR(base)) {
> @@ -1142,7 +1143,7 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
> return PTR_ERR(dsi->regs);
> }
>
> - dsi->bus_clk = devm_clk_get(dev, bus_clk_name);
> + dsi->bus_clk = devm_clk_get(dev, variant->has_mod_clk ? "bus" : NULL);
> if (IS_ERR(dsi->bus_clk))
> return dev_err_probe(dev, PTR_ERR(dsi->bus_clk),
> "Couldn't get the DSI bus clock\n");
> @@ -1151,21 +1152,21 @@ static int sun6i_dsi_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> - if (of_device_is_compatible(dev->of_node,
> - "allwinner,sun6i-a31-mipi-dsi")) {
> + if (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");
> ret = PTR_ERR(dsi->mod_clk);
> goto err_attach_clk;
> }
> - }
>
> - /*
> - * In order to operate properly, that clock seems to be always
> - * set to 297MHz.
> - */
> - clk_set_rate_exclusive(dsi->mod_clk, 297000000);
> + /*
> + * In order to operate properly, the module clock on the
> + * A31 variant always seems to be set to 297MHz.
> + */
> + if (variant->set_mod_clk)
> + clk_set_rate_exclusive(dsi->mod_clk, 297000000);
> + }
>
>
> dsi->dphy = devm_phy_get(dev, "dphy");
> if (IS_ERR(dsi->dphy)) {
> @@ -1205,16 +1206,31 @@ static int sun6i_dsi_remove(struct platform_device *pdev)
>
> component_del(&pdev->dev, &sun6i_dsi_ops);
> mipi_dsi_host_unregister(&dsi->host);
> - clk_rate_exclusive_put(dsi->mod_clk);
> + if (dsi->variant->has_mod_clk && dsi->variant->set_mod_clk)
> + clk_rate_exclusive_put(dsi->mod_clk);

There's also a clk_rate_exclusive_put call in the bind error path

Maxime


Attachments:
(No filename) (3.36 kB)
signature.asc (235.00 B)
Download all attachments

2022-08-16 11:28:11

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 2/4] dt-bindings: display: sun6i-dsi: Add the A100 variant

On 13/08/2022 01:58, Samuel Holland wrote:
> Hi Krzysztof,
>
> On 8/12/22 5:49 AM, Krzysztof Kozlowski wrote:
>> On 12/08/2022 10:42, Samuel Holland wrote:
>>> The "40nm" MIPI DSI controller found in the A100 and D1 SoCs has the
>>> same register layout as previous SoC integrations. However, its module
>>> clock now comes from the TCON, which means it no longer runs at a fixed
>>> rate, so this needs to be distinguished in the driver.
>>>
>>> The controller also now uses pins on Port D instead of dedicated pins,
>>> so it drops the separate power domain.
>>>
>>> Signed-off-by: Samuel Holland <[email protected]>
>>> ---
>>> Removal of the vcc-dsi-supply is maybe a bit questionable. Since there
>>> is no "VCC-DSI" pin anymore, it's not obvious which pin actually does
>>> power the DSI controller/PHY. Possibly power comes from VCC-PD or VCC-IO
>>> or VCC-LVDS. So far, all boards have all of these as always-on supplies,
>>> so it is hard to test.
>>>
>>> .../display/allwinner,sun6i-a31-mipi-dsi.yaml | 28 +++++++++++++++----
>>> 1 file changed, 23 insertions(+), 5 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 ae55ef3fb1fe..c53c25b87bd4 100644
>>> --- a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml
>>> +++ b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml
>>> @@ -12,9 +12,14 @@ maintainers:
>>>
>>> properties:
>>> compatible:
>>> - enum:
>>> - - allwinner,sun6i-a31-mipi-dsi
>>> - - allwinner,sun50i-a64-mipi-dsi
>>> + oneOf:
>>> + - enum:
>>> + - allwinner,sun6i-a31-mipi-dsi
>>> + - allwinner,sun50i-a64-mipi-dsi
>>> + - allwinner,sun50i-a100-mipi-dsi
>>
>> While you are moving code, how about bringing alphabetical order?
>
> I have put the sun*i prefix in numeric order, which matches (almost) all of our

5 is before 6, so strictly numerical order would be:
allwinner,sun50i-a64-mipi-dsi
allwinner,sun50i-a100-mipi-dsi
allwinner,sun6i-a31-mipi-dsi

> other bindings. It roughly corresponds to chronological order as well. It
> doesn't make much sense to me to sort sun50i (ARM64 SoCs) between sun5i and
> sun6i (early ARMv7 SoCs).

However if you say you already implemented some order (obvious for
Allwinner folks), then of course it is fine with me. I just hope other
people will get figure out this order, so they can maintain it.

So assuming there is some order:

Acked-by: Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof