AW: (EXT) Re: [PATCH 1/3] drm/bridge: ti-sn65dsi83: Add vcc supply regulator support
Hi Sam,
On Mon, 11 Oct 2021 22:29:30 +0200, Sam Ravnborg wrote:
> > VCC needs to be enabled before releasing the enable GPIO.
> >
> > Signed-off-by: Alexander Stein
> > ---
> > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 15 ++-
> > 1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > index a32f70bc68ea..5fab0fabcd15 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > @@ -33,6 +33,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > @@ -143,6 +144,7 @@ struct sn65dsi83 {
> > struct mipi_dsi_device *dsi;
> > struct drm_bridge *panel_bridge;
> > struct gpio_desc*enable_gpio;
> > + struct regulator*vcc;
> > int dsi_lanes;
> > boollvds_dual_link;
> > boollvds_dual_link_even_odd_swap;
> > @@ -647,6 +649,12 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx,
> enum sn65dsi83_model model)
> >
> > ctx->panel_bridge = panel_bridge;
> >
> > + ctx->vcc = devm_regulator_get(dev, "vcc");
> In the binding the vcc regulator is required, but devm_regulator_get()
> will create a dummy regulator if not found. Maybe this is on purpose and
> all is good.
Thanks for addressing this. I was slightly unsure myself, but IMHO this is
all good as this makes the driver backward compatible with older DT
which lack the regulator. If there was no vcc regulator necessary and the bridge
was working,then a dummy regulator is fine, as the DT fix would be adding a
always-on, regulator-fixed without any enable/disable possibility anyway.
Best regards,
Alexander
[PATCH v2 1/4] dt-bindings: display: bridge: sn65dsi83: Make enable GPIO optional
From: Laurent Pinchart The SN65DSI8x EN signal may be tied to VCC, or otherwise controlled by means not available to the kernel. Make the GPIO optional. Signed-off-by: Laurent Pinchart Signed-off-by: Alexander Stein --- .../devicetree/bindings/display/bridge/ti,sn65dsi83.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml index 07b20383cbca..a5779bf17849 100644 --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml @@ -93,7 +93,6 @@ properties: required: - compatible - reg - - enable-gpios - ports allOf: -- 2.25.1
[PATCH v2 3/4] dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings
Add a VCC regulator which needs to be enabled before the EN pin is
released.
Reviewed-by: Sam Ravnborg
Signed-off-by: Alexander Stein
---
.../devicetree/bindings/display/bridge/ti,sn65dsi83.yaml | 5 +
1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
index a5779bf17849..49ace6f312d5 100644
--- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
@@ -32,6 +32,9 @@ properties:
maxItems: 1
description: GPIO specifier for bridge_en pin (active high).
+ vcc-supply:
+description: A 1.8V power supply (see regulator/regulator.yaml).
+
ports:
$ref: /schemas/graph.yaml#/properties/ports
@@ -93,6 +96,7 @@ properties:
required:
- compatible
- reg
+ - vcc-supply
- ports
allOf:
@@ -134,6 +138,7 @@ examples:
reg = <0x2d>;
enable-gpios = <&gpio2 1 GPIO_ACTIVE_HIGH>;
+vcc-supply = <®_sn65dsi83_1v8>;
ports {
#address-cells = <1>;
--
2.25.1
[PATCH v2 0/4] ti-sn65dsi83 patches
Changes in V2 of this set: * Add patch from Laurent for fixing the binding regarding optional GPIO * Reorder patches so bindings are changed beforehand * Add small fixes from Sam's review Alexander Stein (3): drm/bridge: ti-sn65dsi83: Make enable GPIO optional dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings drm/bridge: ti-sn65dsi83: Add vcc supply regulator support Laurent Pinchart (1): dt-bindings: display: bridge: sn65dsi83: Make enable GPIO optional .../bindings/display/bridge/ti,sn65dsi83.yaml | 6 +- drivers/gpu/drm/bridge/ti-sn65dsi83.c | 18 -- 2 files changed, 21 insertions(+), 3 deletions(-) -- 2.25.1
[PATCH v2 2/4] drm/bridge: ti-sn65dsi83: Make enable GPIO optional
The enable signal may not be controllable by the kernel. Make it
optional.
This is a similar to commit bbda1704fc15 ("drm/bridge: ti-sn65dsi86: Make
enable GPIO optional")
Reviewed-by: Laurent Pinchart
Reviewed-by: Sam Ravnborg
Signed-off-by: Alexander Stein
---
drivers/gpu/drm/bridge/ti-sn65dsi83.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index a32f70bc68ea..9072342566f3 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -671,7 +671,8 @@ static int sn65dsi83_probe(struct i2c_client *client,
model = id->driver_data;
}
- ctx->enable_gpio = devm_gpiod_get(ctx->dev, "enable", GPIOD_OUT_LOW);
+ ctx->enable_gpio = devm_gpiod_get_optional(ctx->dev, "enable",
+ GPIOD_OUT_LOW);
if (IS_ERR(ctx->enable_gpio))
return PTR_ERR(ctx->enable_gpio);
--
2.25.1
[PATCH v2 4/4] drm/bridge: ti-sn65dsi83: Add vcc supply regulator support
VCC needs to be enabled before releasing the enable GPIO.
Reviewed-by: Sam Ravnborg
Signed-off-by: Alexander Stein
---
drivers/gpu/drm/bridge/ti-sn65dsi83.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 9072342566f3..a6b1fd71dfee 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -33,6 +33,7 @@
#include
#include
#include
+#include
#include
#include
@@ -143,6 +144,7 @@ struct sn65dsi83 {
struct mipi_dsi_device *dsi;
struct drm_bridge *panel_bridge;
struct gpio_desc*enable_gpio;
+ struct regulator*vcc;
int dsi_lanes;
boollvds_dual_link;
boollvds_dual_link_even_odd_swap;
@@ -647,6 +649,12 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum
sn65dsi83_model model)
ctx->panel_bridge = panel_bridge;
+ ctx->vcc = devm_regulator_get(dev, "vcc");
+ if (IS_ERR(ctx->vcc))
+ return dev_err_probe(dev, PTR_ERR(ctx->vcc),
+"Failed to get supply 'vcc': %pe\n",
+ctx->vcc);
+
return 0;
}
@@ -691,7 +699,11 @@ static int sn65dsi83_probe(struct i2c_client *client,
ctx->bridge.of_node = dev->of_node;
drm_bridge_add(&ctx->bridge);
- return 0;
+ ret = regulator_enable(ctx->vcc);
+ if (ret)
+ dev_err(dev, "Failed to enable vcc: %i\n", ret);
+
+ return ret;
}
static int sn65dsi83_remove(struct i2c_client *client)
@@ -702,6 +714,7 @@ static int sn65dsi83_remove(struct i2c_client *client)
mipi_dsi_device_unregister(ctx->dsi);
drm_bridge_remove(&ctx->bridge);
of_node_put(ctx->host_node);
+ regulator_disable(ctx->vcc);
return 0;
}
--
2.25.1
AW: (EXT) Re: [PATCH v2 4/4] drm/bridge: ti-sn65dsi83: Add vcc supply regulator support
Hello Laurent,
On Tue, Oct 12, 2021 at 10:43 +0200, Laurent Pinchart wrote:
> On Tue, Oct 12, 2021 at 08:48:43AM +0200, Alexander Stein wrote:
> > VCC needs to be enabled before releasing the enable GPIO.
> >
> > Reviewed-by: Sam Ravnborg
> > Signed-off-by: Alexander Stein
> > ---
> > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 15 ++-
> > 1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > index 9072342566f3..a6b1fd71dfee 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > @@ -33,6 +33,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > @@ -143,6 +144,7 @@ struct sn65dsi83 {
> > struct mipi_dsi_device *dsi;
> > struct drm_bridge *panel_bridge;
> > struct gpio_desc*enable_gpio;
> > + struct regulator*vcc;
> > int dsi_lanes;
> > boollvds_dual_link;
> > boollvds_dual_link_even_odd_swap;
> > @@ -647,6 +649,12 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx,
> enum sn65dsi83_model model)
> >
> > ctx->panel_bridge = panel_bridge;
> >
> > + ctx->vcc = devm_regulator_get(dev, "vcc");
> > + if (IS_ERR(ctx->vcc))
> > + return dev_err_probe(dev, PTR_ERR(ctx->vcc),
> > +"Failed to get supply 'vcc': %pe\n",
> > +ctx->vcc);
> > +
> > return 0;
> > }
> >
> > @@ -691,7 +699,11 @@ static int sn65dsi83_probe(struct i2c_client *client,
> > ctx->bridge.of_node = dev->of_node;
> > drm_bridge_add(&ctx->bridge);
> >
> > - return 0;
> > + ret = regulator_enable(ctx->vcc);
> > + if (ret)
> > + dev_err(dev, "Failed to enable vcc: %i\n", ret);
>
> I think this should move to sn65dsi83_atomic_pre_enable() (and similarly
> for regulator_disable()) as keeping the regulator enabled at all times
> will cost power.
I get your idea. The thing is that unless 1V8 is provided the bridge is not
even accessible on I2C. So any access to sn65dsi83.regmap without the vcc
regulator enabled will fail. AFAICS this is not an issue right now, as regmap
is only used in sn65dsi83_atomic_enable(), sn65dsi83_atomic_disable() and
sn65dsi83_atomic_pre_enable(), so your sugestion would work, but I'm
hestitating a bit. The driver then has to ensure all regmap uses are done
only when vcc is enabled.
Best regards,
Alexander
[PATCH v3 0/4] ti-sn65dsi83 patches
Changes in V3 of this set: * Do not require vcc-supply in bindings, making it purely optional * Move regulator enable/disable to sn65dsi83_atomic_pre_enable and sn65dsi83_atomic_disable Changes in V2 of this set: * Add patch from Laurent for fixing the binding regarding optional GPIO * Reorder patches so bindings are changed beforehand * Add small fixes from Sam's review Alexander Stein (3): drm/bridge: ti-sn65dsi83: Make enable GPIO optional dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings drm/bridge: ti-sn65dsi83: Add vcc supply regulator support Laurent Pinchart (1): dt-bindings: display: bridge: sn65dsi83: Make enable GPIO optional .../bindings/display/bridge/ti,sn65dsi83.yaml | 5 - drivers/gpu/drm/bridge/ti-sn65dsi83.c | 21 ++- 2 files changed, 24 insertions(+), 2 deletions(-) -- 2.25.1
[PATCH v3 4/4] drm/bridge: ti-sn65dsi83: Add vcc supply regulator support
VCC needs to be enabled before releasing the enable GPIO.
Reviewed-by: Sam Ravnborg
Signed-off-by: Alexander Stein
---
drivers/gpu/drm/bridge/ti-sn65dsi83.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 9072342566f3..c55c45d5d29a 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -33,6 +33,7 @@
#include
#include
#include
+#include
#include
#include
@@ -143,6 +144,7 @@ struct sn65dsi83 {
struct mipi_dsi_device *dsi;
struct drm_bridge *panel_bridge;
struct gpio_desc*enable_gpio;
+ struct regulator*vcc;
int dsi_lanes;
boollvds_dual_link;
boollvds_dual_link_even_odd_swap;
@@ -292,6 +294,11 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge
*bridge,
struct drm_bridge_state
*old_bridge_state)
{
struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
+ int ret;
+
+ ret = regulator_enable(ctx->vcc);
+ if (ret)
+ dev_err(ctx->dev, "Failed to enable vcc: %i\n", ret);
/*
* Reset the chip, pull EN line low for t_reset=10ms,
@@ -536,9 +543,14 @@ static void sn65dsi83_atomic_post_disable(struct
drm_bridge *bridge,
struct drm_bridge_state
*old_bridge_state)
{
struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
+ int ret;
/* Put the chip in reset, pull EN line low. */
gpiod_set_value(ctx->enable_gpio, 0);
+
+ ret = regulator_disable(ctx->vcc);
+ if (ret)
+ dev_err(ctx->dev, "Failed to disable vcc: %i\n", ret);
}
static enum drm_mode_status
@@ -647,6 +659,12 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum
sn65dsi83_model model)
ctx->panel_bridge = panel_bridge;
+ ctx->vcc = devm_regulator_get(dev, "vcc");
+ if (IS_ERR(ctx->vcc))
+ return dev_err_probe(dev, PTR_ERR(ctx->vcc),
+"Failed to get supply 'vcc': %pe\n",
+ctx->vcc);
+
return 0;
}
--
2.25.1
[PATCH v3 2/4] drm/bridge: ti-sn65dsi83: Make enable GPIO optional
The enable signal may not be controllable by the kernel. Make it
optional.
This is a similar to commit bbda1704fc15 ("drm/bridge: ti-sn65dsi86: Make
enable GPIO optional")
Reviewed-by: Laurent Pinchart
Reviewed-by: Sam Ravnborg
Signed-off-by: Alexander Stein
---
drivers/gpu/drm/bridge/ti-sn65dsi83.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index a32f70bc68ea..9072342566f3 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -671,7 +671,8 @@ static int sn65dsi83_probe(struct i2c_client *client,
model = id->driver_data;
}
- ctx->enable_gpio = devm_gpiod_get(ctx->dev, "enable", GPIOD_OUT_LOW);
+ ctx->enable_gpio = devm_gpiod_get_optional(ctx->dev, "enable",
+ GPIOD_OUT_LOW);
if (IS_ERR(ctx->enable_gpio))
return PTR_ERR(ctx->enable_gpio);
--
2.25.1
[PATCH v3 1/4] dt-bindings: display: bridge: sn65dsi83: Make enable GPIO optional
From: Laurent Pinchart The SN65DSI8x EN signal may be tied to VCC, or otherwise controlled by means not available to the kernel. Make the GPIO optional. Signed-off-by: Laurent Pinchart Signed-off-by: Alexander Stein --- .../devicetree/bindings/display/bridge/ti,sn65dsi83.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml index 07b20383cbca..a5779bf17849 100644 --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml @@ -93,7 +93,6 @@ properties: required: - compatible - reg - - enable-gpios - ports allOf: -- 2.25.1
[PATCH v3 3/4] dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings
Add a VCC regulator which needs to be enabled before the EN pin is
released.
Reviewed-by: Sam Ravnborg
Signed-off-by: Alexander Stein
---
.../devicetree/bindings/display/bridge/ti,sn65dsi83.yaml | 4
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
index a5779bf17849..f1e4f149ccb4 100644
--- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
@@ -32,6 +32,9 @@ properties:
maxItems: 1
description: GPIO specifier for bridge_en pin (active high).
+ vcc-supply:
+description: A 1.8V power supply (see regulator/regulator.yaml).
+
ports:
$ref: /schemas/graph.yaml#/properties/ports
@@ -134,6 +137,7 @@ examples:
reg = <0x2d>;
enable-gpios = <&gpio2 1 GPIO_ACTIVE_HIGH>;
+vcc-supply = <®_sn65dsi83_1v8>;
ports {
#address-cells = <1>;
--
2.25.1
[PATCH 1/3] drm/bridge: ti-sn65dsi83: Add vcc supply regulator support
VCC needs to be enabled before releasing the enable GPIO.
Signed-off-by: Alexander Stein
---
drivers/gpu/drm/bridge/ti-sn65dsi83.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index a32f70bc68ea..5fab0fabcd15 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -33,6 +33,7 @@
#include
#include
#include
+#include
#include
#include
@@ -143,6 +144,7 @@ struct sn65dsi83 {
struct mipi_dsi_device *dsi;
struct drm_bridge *panel_bridge;
struct gpio_desc*enable_gpio;
+ struct regulator*vcc;
int dsi_lanes;
boollvds_dual_link;
boollvds_dual_link_even_odd_swap;
@@ -647,6 +649,12 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum
sn65dsi83_model model)
ctx->panel_bridge = panel_bridge;
+ ctx->vcc = devm_regulator_get(dev, "vcc");
+ if (IS_ERR(ctx->vcc))
+ return dev_err_probe(dev, PTR_ERR(ctx->vcc),
+"Failed to get supply 'vcc': %pe\n",
+ERR_PTR(ret));
+
return 0;
}
@@ -690,7 +698,11 @@ static int sn65dsi83_probe(struct i2c_client *client,
ctx->bridge.of_node = dev->of_node;
drm_bridge_add(&ctx->bridge);
- return 0;
+ ret = regulator_enable(ctx->vcc);
+ if (ret)
+ dev_err(dev, "Failed to enable vcc\n");
+
+ return ret;
}
static int sn65dsi83_remove(struct i2c_client *client)
@@ -701,6 +713,7 @@ static int sn65dsi83_remove(struct i2c_client *client)
mipi_dsi_device_unregister(ctx->dsi);
drm_bridge_remove(&ctx->bridge);
of_node_put(ctx->host_node);
+ regulator_disable(ctx->vcc);
return 0;
}
--
2.25.1
[PATCH 2/3] dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings
Add a VCC regulator which needs to be enabled before the EN pin is
released.
Signed-off-by: Alexander Stein
---
.../devicetree/bindings/display/bridge/ti,sn65dsi83.yaml | 5 +
1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
index 07b20383cbca..149cff3233c2 100644
--- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
@@ -32,6 +32,9 @@ properties:
maxItems: 1
description: GPIO specifier for bridge_en pin (active high).
+ vcc-supply:
+description: A 1.8V power supply (see regulator/regulator.yaml).
+
ports:
$ref: /schemas/graph.yaml#/properties/ports
@@ -94,6 +97,7 @@ required:
- compatible
- reg
- enable-gpios
+ - vcc-supply
- ports
allOf:
@@ -135,6 +139,7 @@ examples:
reg = <0x2d>;
enable-gpios = <&gpio2 1 GPIO_ACTIVE_HIGH>;
+vcc-supply = <®_sn65dsi83_1v8>;
ports {
#address-cells = <1>;
--
2.25.1
AW: (EXT) [PATCH] dt-bindings: display: bridge: sn65dsi83: Make enable GPIO optional
> From: Laurent Pinchart > > The SN65DSI8x EN signal may be tied to VCC, or otherwise controlled by > means not available to the kernel. Make the GPIO optional. > > Signed-off-by: Laurent Pinchart > --- > .../devicetree/bindings/display/bridge/ti,sn65dsi83.yaml | 1 - > 1 file changed, 1 deletion(-) > > diff --git > a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml > b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml > index 07b20383cbca..a5779bf17849 100644 > --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml > +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml > @@ -93,7 +93,6 @@ properties: > required: >- compatible >- reg > - - enable-gpios >- ports > > allOf: > > base-commit: 1e3944578b749449bd7fa6bf0bae4c3d3f5f1733 Reviewed-by: Alexander Stein Best regards, Alexander
AW: (EXT) Re: [PATCH 2/3] dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings
On Wed, Oct 6, 2021 at 3:21 PM Rob Herring wrote: > On Wed, Oct 6, 2021 at 2:47 AM Alexander Stein > wrote: > > > > Add a VCC regulator which needs to be enabled before the EN pin is > > released. > > > > Signed-off-by: Alexander Stein > > --- > > .../devicetree/bindings/display/bridge/ti,sn65dsi83.yaml | 5 + > > 1 file changed, 5 insertions(+) > > > > diff --git > > a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml > b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml > > index 07b20383cbca..149cff3233c2 100644 > > --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml > > +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml > > @@ -32,6 +32,9 @@ properties: > > maxItems: 1 > > description: GPIO specifier for bridge_en pin (active high). > > > > + vcc-supply: > > + description: A 1.8V power supply (see regulator/regulator.yaml). > > + > > ports: > > $ref: /schemas/graph.yaml#/properties/ports > > > > @@ -94,6 +97,7 @@ required: > > - compatible > > - reg > > - enable-gpios > > + - vcc-supply > > You generally can't make added properties required unless all DT files > already had this property. It breaks compatibility. I agree, but AFAICS none of the in-kernel DT files uses this binding. I used 'grep -re "ti,sn65dsi8[34]" arch/*/boot/dts' to check. Only ti,sn65dsi86 is used on some qcom boards, but this is a completly different binding and driver. Best regards, Alexander
[PATCH 3/3] drm/bridge: ti-sn65dsi8: Make enable GPIO optional
The enable signal may not be controllable by the kernel. Make it
optional.
This is a similar to commit bbda1704fc15 ("drm/bridge: ti-sn65dsi86: Make
enable GPIO optional")
Signed-off-by: Alexander Stein
---
drivers/gpu/drm/bridge/ti-sn65dsi83.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 5fab0fabcd15..101da29ba698 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -679,7 +679,7 @@ static int sn65dsi83_probe(struct i2c_client *client,
model = id->driver_data;
}
- ctx->enable_gpio = devm_gpiod_get(ctx->dev, "enable", GPIOD_OUT_LOW);
+ ctx->enable_gpio = devm_gpiod_get_optional(ctx->dev, "enable",
GPIOD_OUT_LOW);
if (IS_ERR(ctx->enable_gpio))
return PTR_ERR(ctx->enable_gpio);
--
2.25.1
[PATCH 1/1] drm: mxsfb: Use dev_err_probe() helper
Use the dev_err_probe() helper, instead of open-coding the same
operation. This also adds a nice hint in
/sys/kernel/debug/devices_deferred.
Signed-off-by: Alexander Stein
---
Based on next-20220120
drivers/gpu/drm/mxsfb/mxsfb_drv.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
index 86d78634a979..bc0d766d217b 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
@@ -258,8 +258,7 @@ static int mxsfb_load(struct drm_device *drm,
ret = mxsfb_attach_bridge(mxsfb);
if (ret) {
- if (ret != -EPROBE_DEFER)
- dev_err(drm->dev, "Cannot connect bridge: %d\n", ret);
+ dev_err_probe(drm->dev, ret, "Cannot connect bridge\n");
goto err_vblank;
}
--
2.25.1
[PATCH 1/1] drm: mxsfb: Fix NULL pointer dereference
Do not deference the NULL pointer if the bridge does not return a
bridge state. Assume a fixed format instead.
Fixes: commit b776b0f00f24 ("drm: mxsfb: Use bus_format from the nearest bridge
if present")
Signed-off-by: Alexander Stein
---
This can happen if a "ti,sn75lvds83", "lvds-encoder" bridge is attached
to it. atomic_get_input_bus_fmts is only implemented for the
lvds-decoder case.
drivers/gpu/drm/mxsfb/mxsfb_kms.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
index 0655582ae8ed..4cfb6c001679 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
@@ -361,7 +361,11 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc,
bridge_state =
drm_atomic_get_new_bridge_state(state,
mxsfb->bridge);
- bus_format = bridge_state->input_bus_cfg.format;
+ if (!bridge_state)
+ bus_format = MEDIA_BUS_FMT_FIXED;
+ else
+ bus_format = bridge_state->input_bus_cfg.format;
+
if (bus_format == MEDIA_BUS_FMT_FIXED) {
dev_warn_once(drm->dev,
"Bridge does not provide bus format,
assuming MEDIA_BUS_FMT_RGB888_1X24.\n"
--
2.25.1
Re: (EXT) Re: [PATCH 1/1] drm: mxsfb: Fix NULL pointer dereference
Am Freitag, 21. Januar 2022, 14:14:01 CET schrieb Marek Vasut:
> On 1/21/22 14:12, Alexander Stein wrote:
> > Do not deference the NULL pointer if the bridge does not return a
> > bridge state. Assume a fixed format instead.
> >
> > Fixes: commit b776b0f00f24 ("drm: mxsfb: Use bus_format from the nearest
> > bridge if present") Signed-off-by: Alexander Stein
> >
> > ---
> > This can happen if a "ti,sn75lvds83", "lvds-encoder" bridge is attached
> > to it. atomic_get_input_bus_fmts is only implemented for the
> > lvds-decoder case.
> >
> > drivers/gpu/drm/mxsfb/mxsfb_kms.c | 6 +-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> > b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index 0655582ae8ed..4cfb6c001679
> > 100644
> > --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> > +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> > @@ -361,7 +361,11 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc
> > *crtc,>
> > bridge_state =
> >
> > drm_atomic_get_new_bridge_state(state,
> >
> >
mxsfb->bridge);
> >
> > - bus_format = bridge_state->input_bus_cfg.format;
> > + if (!bridge_state)
> > + bus_format = MEDIA_BUS_FMT_FIXED;
> > + else
> > + bus_format = bridge_state-
>input_bus_cfg.format;
> > +
> >
> > if (bus_format == MEDIA_BUS_FMT_FIXED) {
> >
> > dev_warn_once(drm->dev,
> >
> > "Bridge does not provide bus
format, assuming
> > MEDIA_BUS_FMT_RGB888_1X24.
\n"
>
> Shouldn't this be fixed on the bridge driver side instead ?
>
> Which bridge driver do you use ?
It's drivers/gpu/drm/bridge/lvds-codec.c. I thought naming the compatibles
would suffice. I consider a patch for the bridge driver as a separate issue,
hence the warning from mxsfb. Although I'm unsure how/what to implement.
Similar to the encode case where the bus format is specified in DT?
Anyway, mxsfb should not never dereference the NULL pointer which
drm_atomic_get_new_bridge_state is allowed to return.
Best regards,
Alexander
[PATCH v2 0/2] mxsfb fixes
This v2 collects both single patches from [1] and [2]. Changes in v2: * Added Reviewed-by: Marek Vasut to patch 1 * Updated commit message of patch 2 as suggested by Marek [1] https://patchwork.kernel.org/project/linux-arm-kernel/patch/20220121095644.329256-1-alexander.st...@ew.tq-group.com/ [2] https://patchwork.kernel.org/project/linux-arm-kernel/patch/20220121131238.507567-1-alexander.st...@ew.tq-group.com/ Alexander Stein (2): drm: mxsfb: Use dev_err_probe() helper drm: mxsfb: Fix NULL pointer dereference drivers/gpu/drm/mxsfb/mxsfb_drv.c | 3 +-- drivers/gpu/drm/mxsfb/mxsfb_kms.c | 6 +- 2 files changed, 6 insertions(+), 3 deletions(-) -- 2.25.1
[PATCH v2 1/2] drm: mxsfb: Use dev_err_probe() helper
Use the dev_err_probe() helper, instead of open-coding the same
operation. This also adds a nice hint in
/sys/kernel/debug/devices_deferred.
Reviewed-by: Marek Vasut
Signed-off-by: Alexander Stein
---
drivers/gpu/drm/mxsfb/mxsfb_drv.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
index 375f26d4a417..c4da358f2154 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
@@ -258,8 +258,7 @@ static int mxsfb_load(struct drm_device *drm,
ret = mxsfb_attach_bridge(mxsfb);
if (ret) {
- if (ret != -EPROBE_DEFER)
- dev_err(drm->dev, "Cannot connect bridge: %d\n", ret);
+ dev_err_probe(drm->dev, ret, "Cannot connect bridge\n");
goto err_vblank;
}
--
2.25.1
[PATCH v2 2/2] drm: mxsfb: Fix NULL pointer dereference
mxsfb should not never dereference the NULL pointer which
drm_atomic_get_new_bridge_state is allowed to return.
Assume a fixed format instead.
Fixes: commit b776b0f00f24 ("drm: mxsfb: Use bus_format from the nearest bridge
if present")
Signed-off-by: Alexander Stein
---
drivers/gpu/drm/mxsfb/mxsfb_kms.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
index 0655582ae8ed..4cfb6c001679 100644
--- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
+++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
@@ -361,7 +361,11 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc,
bridge_state =
drm_atomic_get_new_bridge_state(state,
mxsfb->bridge);
- bus_format = bridge_state->input_bus_cfg.format;
+ if (!bridge_state)
+ bus_format = MEDIA_BUS_FMT_FIXED;
+ else
+ bus_format = bridge_state->input_bus_cfg.format;
+
if (bus_format == MEDIA_BUS_FMT_FIXED) {
dev_warn_once(drm->dev,
"Bridge does not provide bus format,
assuming MEDIA_BUS_FMT_RGB888_1X24.\n"
--
2.25.1
Re: (EXT) Re: [PATCH v2 1/2] drm: mxsfb: Use dev_err_probe() helper
Am Mittwoch, 2. Februar 2022, 09:29:20 CET schrieb Marek Vasut: > On 2/2/22 09:17, Alexander Stein wrote: > > Use the dev_err_probe() helper, instead of open-coding the same > > operation. This also adds a nice hint in > > /sys/kernel/debug/devices_deferred. > > > > Reviewed-by: Marek Vasut > > Signed-off-by: Alexander Stein > > --- > > > > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > V2: ... what changed ... ? > > (probably nothing, since the patch still looks fine) I put the changelog into cover letter (0/2). Content hasn't changed, I just added your Reviewed-by. Regards, Alexander
Re: (EXT) Re: [PATCH v2 2/2] drm: mxsfb: Fix NULL pointer dereference
Am Mittwoch, 2. Februar 2022, 09:30:38 CET schrieb Marek Vasut:
> On 2/2/22 09:17, Alexander Stein wrote:
> > mxsfb should not never dereference the NULL pointer which
>
> ... not ever ... but that's really a nitpick.
Doh, I just copied it from my mail...
You want me to send a v2.1? Or will someone fix it when applying?
> > drm_atomic_get_new_bridge_state is allowed to return.
> > Assume a fixed format instead.
> >
> > Fixes: commit b776b0f00f24 ("drm: mxsfb: Use bus_format from the nearest
> > bridge if present") Signed-off-by: Alexander Stein
> >
>
> It's perfect otherwise, thanks !
Thanks,
Alexander
[PATCH v4 0/4] ti-sn65dsi83 patches
Changes in V4 of this set: * Rebased to next-2028 (due to merge-conflict in linux-next) * Added Rob Herring's Ack on Patch 1 & 3 * Reworked patch 4 due to other changes in linux-next * Removed Sam Ravnborg's Reviewed-by for patch4 due to rework Changes in V3 of this set: * Do not require vcc-supply in bindings, making it purely optional * Move regulator enable/disable to sn65dsi83_atomic_pre_enable and sn65dsi83_atomic_disable Changes in V2 of this set: * Add patch from Laurent for fixing the binding regarding optional GPIO * Reorder patches so bindings are changed beforehand * Add small fixes from Sam's review Alexander Stein (3): drm/bridge: ti-sn65dsi83: Make enable GPIO optional dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings drm/bridge: ti-sn65dsi83: Add vcc supply regulator support Laurent Pinchart (1): dt-bindings: display: bridge: sn65dsi83: Make enable GPIO optional .../bindings/display/bridge/ti,sn65dsi83.yaml | 5 - drivers/gpu/drm/bridge/ti-sn65dsi83.c | 22 ++- 2 files changed, 25 insertions(+), 2 deletions(-) -- 2.25.1
[PATCH v4 4/4] drm/bridge: ti-sn65dsi83: Add vcc supply regulator support
VCC needs to be enabled before releasing the enable GPIO.
Signed-off-by: Alexander Stein
---
drivers/gpu/drm/bridge/ti-sn65dsi83.c | 19 +++
1 file changed, 19 insertions(+)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 065610edc37a..54d18e82ed74 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -33,6 +33,7 @@
#include
#include
#include
+#include
#include
#include
@@ -143,6 +144,7 @@ struct sn65dsi83 {
struct mipi_dsi_device *dsi;
struct drm_bridge *panel_bridge;
struct gpio_desc*enable_gpio;
+ struct regulator*vcc;
int dsi_lanes;
boollvds_dual_link;
boollvds_dual_link_even_odd_swap;
@@ -337,6 +339,12 @@ static void sn65dsi83_atomic_enable(struct drm_bridge
*bridge,
u16 val;
int ret;
+ ret = regulator_enable(ctx->vcc);
+ if (ret) {
+ dev_err(ctx->dev, "Failed to enable vcc\n");
+ return;
+ }
+
/* Deassert reset */
gpiod_set_value(ctx->enable_gpio, 1);
usleep_range(1000, 1100);
@@ -486,11 +494,16 @@ static void sn65dsi83_atomic_disable(struct drm_bridge
*bridge,
struct drm_bridge_state *old_bridge_state)
{
struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
+ int ret;
/* Put the chip in reset, pull EN line low, and assure 10ms reset low
timing. */
gpiod_set_value(ctx->enable_gpio, 0);
usleep_range(1, 11000);
+ ret = regulator_disable(ctx->vcc);
+ if (ret)
+ dev_err(ctx->dev, "Failed to disable vcc: %i\n", ret);
+
regcache_mark_dirty(ctx->regmap);
}
@@ -599,6 +612,12 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum
sn65dsi83_model model)
ctx->panel_bridge = panel_bridge;
+ ctx->vcc = devm_regulator_get(dev, "vcc");
+ if (IS_ERR(ctx->vcc))
+ return dev_err_probe(dev, PTR_ERR(ctx->vcc),
+"Failed to get supply 'vcc': %pe\n",
+ERR_PTR(ret));
+
return 0;
}
--
2.25.1
[PATCH v4 2/4] drm/bridge: ti-sn65dsi83: Make enable GPIO optional
The enable signal may not be controllable by the kernel. Make it
optional.
This is a similar to commit bbda1704fc15 ("drm/bridge: ti-sn65dsi86: Make
enable GPIO optional")
Reviewed-by: Laurent Pinchart
Reviewed-by: Sam Ravnborg
Signed-off-by: Alexander Stein
---
drivers/gpu/drm/bridge/ti-sn65dsi83.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 945f08de45f1..065610edc37a 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -662,7 +662,8 @@ static int sn65dsi83_probe(struct i2c_client *client,
}
/* Put the chip in reset, pull EN line low, and assure 10ms reset low
timing. */
- ctx->enable_gpio = devm_gpiod_get(ctx->dev, "enable", GPIOD_OUT_LOW);
+ ctx->enable_gpio = devm_gpiod_get_optional(ctx->dev, "enable",
+ GPIOD_OUT_LOW);
if (IS_ERR(ctx->enable_gpio))
return PTR_ERR(ctx->enable_gpio);
--
2.25.1
[PATCH v4 3/4] dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings
Add a VCC regulator which needs to be enabled before the EN pin is
released.
Reviewed-by: Sam Ravnborg
Acked-by: Rob Herring
Signed-off-by: Alexander Stein
---
.../devicetree/bindings/display/bridge/ti,sn65dsi83.yaml | 4
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
index c3f3e73f740a..48a97bb3e2e0 100644
--- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
@@ -32,6 +32,9 @@ properties:
maxItems: 1
description: GPIO specifier for bridge_en pin (active high).
+ vcc-supply:
+description: A 1.8V power supply (see regulator/regulator.yaml).
+
ports:
$ref: /schemas/graph.yaml#/properties/ports
@@ -132,6 +135,7 @@ examples:
reg = <0x2d>;
enable-gpios = <&gpio2 1 GPIO_ACTIVE_HIGH>;
+vcc-supply = <®_sn65dsi83_1v8>;
ports {
#address-cells = <1>;
--
2.25.1
[PATCH v4 1/4] dt-bindings: display: bridge: sn65dsi83: Make enable GPIO optional
From: Laurent Pinchart The SN65DSI8x EN signal may be tied to VCC, or otherwise controlled by means not available to the kernel. Make the GPIO optional. Signed-off-by: Laurent Pinchart Acked-by: Rob Herring Signed-off-by: Alexander Stein --- .../devicetree/bindings/display/bridge/ti,sn65dsi83.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml index b446d0f0f1b4..c3f3e73f740a 100644 --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml @@ -91,7 +91,6 @@ properties: required: - compatible - reg - - enable-gpios - ports allOf: -- 2.25.1
Re: (EXT) Re: [PATCH v4 1/4] dt-bindings: display: bridge: sn65dsi83: Make enable GPIO optional
Am Donnerstag, dem 09.12.2021 um 12:37 +0530 schrieb Jagan Teki:
> On Thu, Nov 18, 2021 at 2:50 PM Alexander Stein
> <
> alexander.st...@ew.tq-group.com
> > wrote:
> > From: Laurent Pinchart <
> > laurent.pinch...@ideasonboard.com
> > >
> >
> > The SN65DSI8x EN signal may be tied to VCC, or otherwise controlled
> > by
> > means not available to the kernel. Make the GPIO optional.
>
> Sorry, I couldn't understand what it means. Does it mean VCC enabled
> designs no need to enable GPIO? I've a design that do support both EN
> and VCC.
The patches 1 & 2 are only about the "enable" gpio for the bridge, it's
unrelated to the VCC regulator introduced in patch 3 & 4.
Maybe the commit message should say:
> The SN65DSI8x EN signal may be hard-wired to VCC, or otherwise
controlled[...]
But I copied the message from bbda1704fc15 ("drm/bridge: ti-sn65dsi86:
Make enable GPIO optional").
This is for use-cases where there is no GPIO the kernel can use, to
control the EN pad of the bridge. Thus make this gpio optional in
bindings and driver.
HTH
Alexander
[PATCH v5 0/4] ti-sn65dsi83 patches
Changes in V5 of this set: * Rebased to next-20211208 * Fix format string in error message * Remove superfluous error value for dev_err_probe() * Added Reviewed-by: Jagan Teki for patch 3 & 4 Changes in V4 of this set: * Rebased to next-2028 (due to merge-conflict in linux-next) * Added Rob Herring's Ack on Patch 1 & 3 * Reworked patch 4 due to other changes in linux-next * Removed Sam Ravnborg's Reviewed-by for patch4 due to rework Changes in V3 of this set: * Do not require vcc-supply in bindings, making it purely optional * Move regulator enable/disable to sn65dsi83_atomic_pre_enable and sn65dsi83_atomic_disable Changes in V2 of this set: * Add patch from Laurent for fixing the binding regarding optional GPIO * Reorder patches so bindings are changed beforehand * Add small fixes from Sam's review Alexander Stein (3): drm/bridge: ti-sn65dsi83: Make enable GPIO optional dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings drm/bridge: ti-sn65dsi83: Add vcc supply regulator support Laurent Pinchart (1): dt-bindings: display: bridge: sn65dsi83: Make enable GPIO optional .../bindings/display/bridge/ti,sn65dsi83.yaml | 5 - drivers/gpu/drm/bridge/ti-sn65dsi83.c | 21 ++- 2 files changed, 24 insertions(+), 2 deletions(-) -- 2.25.1
[PATCH v5 1/4] dt-bindings: display: bridge: sn65dsi83: Make enable GPIO optional
From: Laurent Pinchart The SN65DSI8x EN signal may be tied to VCC, or otherwise controlled by means not available to the kernel. Make the GPIO optional. Signed-off-by: Laurent Pinchart Acked-by: Rob Herring Signed-off-by: Alexander Stein --- .../devicetree/bindings/display/bridge/ti,sn65dsi83.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml index b446d0f0f1b4..c3f3e73f740a 100644 --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml @@ -91,7 +91,6 @@ properties: required: - compatible - reg - - enable-gpios - ports allOf: -- 2.25.1
[PATCH v5 2/4] drm/bridge: ti-sn65dsi83: Make enable GPIO optional
The enable signal may not be controllable by the kernel. Make it
optional.
This is a similar to commit bbda1704fc15 ("drm/bridge: ti-sn65dsi86: Make
enable GPIO optional")
Reviewed-by: Laurent Pinchart
Reviewed-by: Sam Ravnborg
Signed-off-by: Alexander Stein
---
drivers/gpu/drm/bridge/ti-sn65dsi83.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 945f08de45f1..065610edc37a 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -662,7 +662,8 @@ static int sn65dsi83_probe(struct i2c_client *client,
}
/* Put the chip in reset, pull EN line low, and assure 10ms reset low
timing. */
- ctx->enable_gpio = devm_gpiod_get(ctx->dev, "enable", GPIOD_OUT_LOW);
+ ctx->enable_gpio = devm_gpiod_get_optional(ctx->dev, "enable",
+ GPIOD_OUT_LOW);
if (IS_ERR(ctx->enable_gpio))
return PTR_ERR(ctx->enable_gpio);
--
2.25.1
[PATCH v5 3/4] dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings
Add a VCC regulator which needs to be enabled before the EN pin is
released.
Reviewed-by: Sam Ravnborg
Acked-by: Rob Herring
Reviewed-by: Jagan Teki
Signed-off-by: Alexander Stein
---
.../devicetree/bindings/display/bridge/ti,sn65dsi83.yaml | 4
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
index c3f3e73f740a..48a97bb3e2e0 100644
--- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
@@ -32,6 +32,9 @@ properties:
maxItems: 1
description: GPIO specifier for bridge_en pin (active high).
+ vcc-supply:
+description: A 1.8V power supply (see regulator/regulator.yaml).
+
ports:
$ref: /schemas/graph.yaml#/properties/ports
@@ -132,6 +135,7 @@ examples:
reg = <0x2d>;
enable-gpios = <&gpio2 1 GPIO_ACTIVE_HIGH>;
+vcc-supply = <®_sn65dsi83_1v8>;
ports {
#address-cells = <1>;
--
2.25.1
[PATCH v5 4/4] drm/bridge: ti-sn65dsi83: Add vcc supply regulator support
VCC needs to be enabled before releasing the enable GPIO.
Reviewed-by: Laurent Pinchart
Signed-off-by: Alexander Stein
---
drivers/gpu/drm/bridge/ti-sn65dsi83.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 065610edc37a..5650a793db81 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -33,6 +33,7 @@
#include
#include
#include
+#include
#include
#include
@@ -143,6 +144,7 @@ struct sn65dsi83 {
struct mipi_dsi_device *dsi;
struct drm_bridge *panel_bridge;
struct gpio_desc*enable_gpio;
+ struct regulator*vcc;
int dsi_lanes;
boollvds_dual_link;
boollvds_dual_link_even_odd_swap;
@@ -337,6 +339,12 @@ static void sn65dsi83_atomic_enable(struct drm_bridge
*bridge,
u16 val;
int ret;
+ ret = regulator_enable(ctx->vcc);
+ if (ret) {
+ dev_err(ctx->dev, "Failed to enable vcc: %d\n", ret);
+ return;
+ }
+
/* Deassert reset */
gpiod_set_value(ctx->enable_gpio, 1);
usleep_range(1000, 1100);
@@ -486,11 +494,16 @@ static void sn65dsi83_atomic_disable(struct drm_bridge
*bridge,
struct drm_bridge_state *old_bridge_state)
{
struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
+ int ret;
/* Put the chip in reset, pull EN line low, and assure 10ms reset low
timing. */
gpiod_set_value(ctx->enable_gpio, 0);
usleep_range(1, 11000);
+ ret = regulator_disable(ctx->vcc);
+ if (ret)
+ dev_err(ctx->dev, "Failed to disable vcc: %d\n", ret);
+
regcache_mark_dirty(ctx->regmap);
}
@@ -599,6 +612,11 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum
sn65dsi83_model model)
ctx->panel_bridge = panel_bridge;
+ ctx->vcc = devm_regulator_get(dev, "vcc");
+ if (IS_ERR(ctx->vcc))
+ return dev_err_probe(dev, PTR_ERR(ctx->vcc),
+"Failed to get supply 'vcc'\n");
+
return 0;
}
--
2.25.1
Re: (EXT) [PATCH v8 09/14] drm/bridge: imx: Add LDB driver helper support
Am Donnerstag, 9. Juni 2022, 08:49:26 CEST schrieb Liu Ying:
> This patch adds a helper to support LDB drm bridge drivers for
> i.MX SoCs. Helper functions supported by this helper should
> implement common logics for all LDB modules embedded in i.MX SoCs.
>
> Tested-by: Marcel Ziswiler # Colibri iMX8X,
> LT170410-2WHC, LP156WF1 Reviewed-by: Robert Foss
> Signed-off-by: Liu Ying
> ---
Hi,
reading this I got reminded of fsl-ldb [1], which is accepted already. At a
first glance reading the RM the LDB peripheral are similar, although not
identical. Is it worth merging them into one driver (at some point)?
Best regards,
Alexander
[1]
https://patchwork.freedesktop.org/patch/msgid/20220426193645.244792-2-ma...@denx.de
> Marcel, I add your T-b tag from v6, let me know if you want me to drop it,
> as the checkpatch fix in v7 and the rebase in v8 are trivial.
>
> v7->v8:
> * Use devm_drm_of_get_bridge() due to the rebase upon v5.19-rc1.
>
> v6->v7:
> * Fix below complaints from 'checkpatch.pl --strict'. (Robert)
>- 'Alignment should match open parenthesis'
>- 'Prefer using the BIT macro'
> * Add Marcel's T-b tag.
> * Add Robert's R-b tag.
>
> v5->v6:
> * No change.
>
> v4->v5:
> * Make imx-ldb-helper be a pure object to be linked with i.MX8qxp LDB bridge
> driver and i.MX8qm LDB bridge driver. (Robert)
> * Move 'imx_ldb_helper.h' to 'drivers/gpu/drm/bridge/imx/imx-ldb-helper.h'.
> (Robert)
> * s/__FSL_IMX_LDB__/__IMX_LDB_HELPER__/ for 'imx-ldb-helper.h'.
>
> v3->v4:
> * No change.
>
> v2->v3:
> * Call syscon_node_to_regmap() to get regmap instead of
> syscon_regmap_lookup_by_phandle().
>
> v1->v2:
> * No change.
>
> drivers/gpu/drm/bridge/imx/imx-ldb-helper.c | 220
> drivers/gpu/drm/bridge/imx/imx-ldb-helper.h | 96 +
> 2 files changed, 316 insertions(+)
> create mode 100644 drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
> create mode 100644 drivers/gpu/drm/bridge/imx/imx-ldb-helper.h
>
> diff --git a/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
> b/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c new file mode 100644
> index ..e85eb9ab5947
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
> @@ -0,0 +1,220 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2012 Sascha Hauer, Pengutronix
> + * Copyright 2019,2020,2022 NXP
> + */
> +
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +
> +#include "imx-ldb-helper.h"
> +
> +bool ldb_channel_is_single_link(struct ldb_channel *ldb_ch)
> +{
> + return ldb_ch->link_type == LDB_CH_SINGLE_LINK;
> +}
> +
> +bool ldb_channel_is_split_link(struct ldb_channel *ldb_ch)
> +{
> + return ldb_ch->link_type == LDB_CH_DUAL_LINK_EVEN_ODD_PIXELS ||
> +ldb_ch->link_type == LDB_CH_DUAL_LINK_ODD_EVEN_PIXELS;
> +}
> +
> +int ldb_bridge_atomic_check_helper(struct drm_bridge *bridge,
> +struct drm_bridge_state
*bridge_state,
> +struct drm_crtc_state
*crtc_state,
> +struct drm_connector_state
*conn_state)
> +{
> + struct ldb_channel *ldb_ch = bridge->driver_private;
> +
> + ldb_ch->in_bus_format = bridge_state->input_bus_cfg.format;
> + ldb_ch->out_bus_format = bridge_state->output_bus_cfg.format;
> +
> + return 0;
> +}
> +
> +void ldb_bridge_mode_set_helper(struct drm_bridge *bridge,
> + const struct drm_display_mode
*mode,
> + const struct drm_display_mode
*adjusted_mode)
> +{
> + struct ldb_channel *ldb_ch = bridge->driver_private;
> + struct ldb *ldb = ldb_ch->ldb;
> + bool is_split = ldb_channel_is_split_link(ldb_ch);
> +
> + if (is_split)
> + ldb->ldb_ctrl |= LDB_SPLIT_MODE_EN;
> +
> + switch (ldb_ch->out_bus_format) {
> + case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG:
> + break;
> + case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG:
> + if (ldb_ch->chno == 0 || is_split)
> + ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH0_24;
> + if (ldb_ch->chno == 1 || is_split)
> + ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH1_24;
> + break;
> + case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA:
> + if (ldb_ch->chno == 0 || is_split)
> + ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH0_24 |
> + LDB_BIT_MAP_CH0_JEIDA;
> + if (ldb_ch->chno == 1 || is_split)
> + ldb->ldb_ctrl |= LDB_DATA_WIDTH_CH1_24 |
> + LDB_BIT_MAP_CH1_JEIDA;
> + break;
> + }
> +}
> +
> +void ldb_bridge_enable_helper(struct drm_bridge *bridge)
> +{
> + struct ldb_channel *ldb_ch = bridge->driver_private;
> + struct ldb *ldb = ldb_ch->ldb;
> +
> + /*
> + * Platform specific bridge drivers should set ldb_ctrl properly
> + * for the enablement, so just write the ctrl_reg her
Re: (EXT) Re: (EXT) [PATCH v8 09/14] drm/bridge: imx: Add LDB driver helper support
Hi,
Am Freitag, 10. Juni 2022, 05:01:21 CEST schrieb Liu Ying:
> > reading this I got reminded of fsl-ldb [1], which is accepted
> > already. At a
> > first glance reading the RM the LDB peripheral are similar, although
> > not
> > identical. Is it worth merging them into one driver (at some point)?
>
> fsl-ldb is for i.MX8mp LDB. It couples the lvds phy(LVDS_CTRL register)
> with LDB(LDB_CTRL register) hardly.
>
> Eventually, I think there would be separate LDB bridge drivers for
> i.MX8mp/qxp/qm LDBs, as they are far or less different(LVDS PHY IPs,
> clocks, ways of dual link usage...). So, maybe, the question is that
> can fsl-ldb use this LDB helper driver. AFAICS, the different DT
> bindings between i.MX8mp LDB and i.MX8qxp/qm LDB make this difficult.
> This LDB helper takes each LDB child node(channel node) of i.MX8qxp/qm
> as a bridge, while i.MX8mp LDB bindings put input and output ports in
> 'ports' node. Like i.MX8qxp/qm LDB, i.MX6 LDB
> binding(Documentation/devicetree/bindings/display/imx/ldb.txt) also
> uses 'channel' nodes, though i.MX6 LDB has a separate encoder driver.
> I think the 'channel' node better reflects HW design.
> So, maybe, fsl-ldb for i.MX8mp won't use this LDB helper.
Apparently the hardware is too different to share much common code. Yes,
bindings seem very different as well, so maybe it's ot possible. Thanks for
the explanation though.
Best regards,
Alexander
> Regards,
> Liu Ying
>
> > Best regards,
> > Alexander
> >
> > [1]
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwor
> > k.freedesktop.org%2Fpatch%2Fmsgid%2F20220426193645.244792-2-marex%40denx.d
> > e&data=05%7C01%7Cvictor.liu%40nxp.com%7Ca4ee326b3f314cf48a3408da49ec5a
> > 33%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637903576722519563%7CUnkno
> > wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXV
> > CI6Mn0%3D%7C3000%7C%7C%7C&sdata=yJhovCeB7Dx2eWJqKohZskA7fX3NLwqmW1GxeQ
> > lDe40%3D&reserved=0>
> > > Marcel, I add your T-b tag from v6, let me know if you want me to
> > > drop it,
> > > as the checkpatch fix in v7 and the rebase in v8 are trivial.
> > >
> > > v7->v8:
> > > * Use devm_drm_of_get_bridge() due to the rebase upon v5.19-rc1.
> > >
> > > v6->v7:
> > > * Fix below complaints from 'checkpatch.pl --strict'. (Robert)
> > >
> > >- 'Alignment should match open parenthesis'
> > >- 'Prefer using the BIT macro'
> > >
> > > * Add Marcel's T-b tag.
> > > * Add Robert's R-b tag.
> > >
> > > v5->v6:
> > > * No change.
> > >
> > > v4->v5:
> > > * Make imx-ldb-helper be a pure object to be linked with i.MX8qxp
> > > LDB bridge
> > > driver and i.MX8qm LDB bridge driver. (Robert)
> > > * Move 'imx_ldb_helper.h' to 'drivers/gpu/drm/bridge/imx/imx-ldb-
> > > helper.h'.
> > >
> > > (Robert)
> > >
> > > * s/__FSL_IMX_LDB__/__IMX_LDB_HELPER__/ for 'imx-ldb-helper.h'.
> > >
> > > v3->v4:
> > > * No change.
> > >
> > > v2->v3:
> > > * Call syscon_node_to_regmap() to get regmap instead of
> > >
> > > syscon_regmap_lookup_by_phandle().
> > >
> > > v1->v2:
> > > * No change.
> > >
> > > drivers/gpu/drm/bridge/imx/imx-ldb-helper.c | 220
> > >
> > >
> > >
> > > drivers/gpu/drm/bridge/imx/imx-ldb-helper.h | 96 +
> > > 2 files changed, 316 insertions(+)
> > > create mode 100644 drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
> > > create mode 100644 drivers/gpu/drm/bridge/imx/imx-ldb-helper.h
> > >
> > > diff --git a/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
> > > b/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c new file mode 100644
> > > index ..e85eb9ab5947
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/bridge/imx/imx-ldb-helper.c
> > > @@ -0,0 +1,220 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright (C) 2012 Sascha Hauer, Pengutronix
> > > + * Copyright 2019,2020,2022 NXP
> > > + */
> > > +
> > > +#include
> > > +#include
> > > +#include
> > > +
> > > +#include
> > > +#include
> > > +#include
> > > +
> > > +#include "imx-ldb-helper.h"
> > > +
> > > +bool ldb_channel_is_single_link(struct ldb_channel *ldb_ch)
> > > +{
> > > + return ldb_ch->link_type == LDB_CH_SINGLE_LINK;
> > > +}
> > > +
> > > +bool ldb_channel_is_split_link(struct ldb_channel *ldb_ch)
> > > +{
> > > + return ldb_ch->link_type == LDB_CH_DUAL_LINK_EVEN_ODD_PIXELS ||
> > > +ldb_ch->link_type == LDB_CH_DUAL_LINK_ODD_EVEN_PIXELS;
> > > +}
> > > +
> > > +int ldb_bridge_atomic_check_helper(struct drm_bridge *bridge,
> > > +struct drm_bridge_state
> >
> > *bridge_state,
> >
> > > +struct drm_crtc_state
> >
> > *crtc_state,
> >
> > > +struct drm_connector_state
> >
> > *conn_state)
> >
> > > +{
> > > + struct ldb_channel *ldb_ch = bridge->driver_private;
> > > +
> > > + ldb_ch->in_bus_format = bridge_state->input_bus_cfg.format;
> > > + ldb_ch->out_bus_format = bridge_state->output_bus_cfg.format;
> > > +
>
[PATCH v2 1/2] drm/bridge: ti-sn65dsi83: add more dev_err_probe
Add more warning/debug messages during probe. E.g. a single -EPROBE_DEFER might have several causes, these messages help finding the origin. Signed-off-by: Alexander Stein --- * New in v2 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index b27c0d7c451a..a306150a8027 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c @@ -677,7 +677,7 @@ static int sn65dsi83_probe(struct i2c_client *client, ctx->enable_gpio = devm_gpiod_get_optional(ctx->dev, "enable", GPIOD_OUT_LOW); if (IS_ERR(ctx->enable_gpio)) - return PTR_ERR(ctx->enable_gpio); + return dev_err_probe(dev, PTR_ERR(ctx->enable_gpio), "failed to get enable GPIO\n"); usleep_range(1, 11000); @@ -687,7 +687,7 @@ static int sn65dsi83_probe(struct i2c_client *client, ctx->regmap = devm_regmap_init_i2c(client, &sn65dsi83_regmap_config); if (IS_ERR(ctx->regmap)) - return PTR_ERR(ctx->regmap); + return dev_err_probe(dev, PTR_ERR(ctx->regmap), "failed to get regmap\n"); dev_set_drvdata(dev, ctx); i2c_set_clientdata(client, ctx); -- 2.25.1
[PATCH v2 2/2] drm/bridge: ti-sn65dsi83: Allow GPIO operations to sleep
There is no need to require non-sleeping GPIO access. Silence the WARN_ON() if GPIO is using e.g. I2C expanders. Signed-off-by: Alexander Stein --- Change in v2: * None drivers/gpu/drm/bridge/ti-sn65dsi83.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index a306150a8027..dc26640e7d9b 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c @@ -344,7 +344,7 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge, } /* Deassert reset */ - gpiod_set_value(ctx->enable_gpio, 1); + gpiod_set_value_cansleep(ctx->enable_gpio, 1); usleep_range(1000, 1100); /* Get the LVDS format from the bridge state. */ @@ -500,7 +500,7 @@ static void sn65dsi83_atomic_disable(struct drm_bridge *bridge, int ret; /* Put the chip in reset, pull EN line low, and assure 10ms reset low timing. */ - gpiod_set_value(ctx->enable_gpio, 0); + gpiod_set_value_cansleep(ctx->enable_gpio, 0); usleep_range(1, 11000); ret = regulator_disable(ctx->vcc); -- 2.25.1
[PATCH 1/1] drm/panel: panel-simple: Add dev_err_probe if backlight could not be found
If the backlight node is not enabled, this (silently) returns with
-EPROBE_DEFER. /sys/kernel/debug/devices_deferred also shows nothing
helpful:
$ cat /sys/kernel/debug/devices_deferred
display
With this patch, there is a helpful hint:
$ cat /sys/kernel/debug/devices_deferred
display panel-simple: Could not find backlight
Signed-off-by: Alexander Stein
---
drivers/gpu/drm/panel/panel-simple.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/panel/panel-simple.c
b/drivers/gpu/drm/panel/panel-simple.c
index 4a2e580a2f7b..8fb1c563c96a 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -663,8 +663,10 @@ static int panel_simple_probe(struct device *dev, const
struct panel_desc *desc)
drm_panel_init(&panel->base, dev, &panel_simple_funcs, connector_type);
err = drm_panel_of_backlight(&panel->base);
- if (err)
+ if (err) {
+ dev_err_probe(dev, err, "Could not find backlight\n");
goto disable_pm_runtime;
+ }
drm_panel_add(&panel->base);
--
2.25.1
Re: (EXT) Re: [RFC][PATCH] Revert "drm/panel-simple: drop use of data-mapping property"
Hi Laurent, Am Donnerstag, 3. Februar 2022, 00:45:59 CET schrieb Laurent Pinchart: > [...] > You're right that there's an issue, but a revert isn't the right option. > The commit you're reverting never made it in a stable release, because > it was deemed to not be a good enough option. > > First of all, any attempt to fix this should include an update to the DT > binding. Second, as this is about DPI panels, the LVDS option should be > dropped. Finally, I've shared some initial thoughts in [1], maybe you > can reply to that e-mail to continue the discussion there ? > > https://lore.kernel.org/all/20200303185531.gj11...@pendragon.ideasonboard.co > m/ At first I thought, this is a different issue than the one I currently have, but after reading this post, I think it's somewhat related. > If a panel expects RGB888 and receives RGB666 with the two LSBs of each > component hardwired to GND on the PCB, should DT report RGB888 or RGB666 > on the panel side ? I'm tempted by the former, and specifying the latter > on the transmitting side. My situation is the other way around. My panel (cdtech,s070swv29hg-dc44) has a MEDIA_BUS_FMT_RGB666_1X18 bus format (see panel-simple.c). Unfortunately for one mainboard the connection is like that: i.MX -- Panel (Blue and green is identical) R7 -- R5 R6 -- R4 ... R2 -- R0 R1 dont care R0 dont care So the 8 bpc (imx) and 6 bps (panel) are MSB aligned. The 2 LSB are completely ignored. The fast hacked fix is to use an additional panel description with bus format set to MEDIA_BUS_FMT_RGB888_1X24, keeping everything else the same. But that is cumbersome. IMHO a straight forward solution is to use a, yet to be written, simple bridge which just converts the bus format transparently, assuming the electrical connection is actually correct. This way the panel can set the native bus format, regardless of actual connections. Christoph's problem should disappear as well if going that way, as the bus format is set for the -> connection. Nevertheless the panel bus format should be available in the end. Regards, Alexander
[PATCH 1/2] drm/bridge: ti-sn65dsi83: use dev_err_probe
sn65dsi83_host_attach is called from probe, so silence message upon
deferred probe. This can happen, e.g. if the bridge driver is built-in, but
the host is built as module.
Signed-off-by: Alexander Stein
---
This might look a bit weird in the first place, but the real benefit is
usage of device_set_deferred_probe_reason() inside dev_err_probe().
Having /sys/kernel/debug/devices_deferred providing more information
actually helped me tracking down an issue.
drivers/gpu/drm/bridge/ti-sn65dsi83.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index a88d90f928ce..1f02596d6db4 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -644,8 +644,7 @@ static int sn65dsi83_host_attach(struct sn65dsi83 *ctx)
host = of_find_mipi_dsi_host_by_node(ctx->host_node);
if (!host) {
- dev_err(dev, "failed to find dsi host\n");
- return -EPROBE_DEFER;
+ return dev_err_probe(dev, -EPROBE_DEFER, "failed to find dsi
host\n");
}
dsi = devm_mipi_dsi_device_register_full(dev, host, &info);
--
2.25.1
[PATCH 2/2] drm/bridge: ti-sn65dsi83: Allow GPIO operations to sleep
There is no need to require non-sleeping GPIO access. Silence the WARN_ON() if GPIO is using e.g. I2C expanders. Signed-off-by: Alexander Stein --- If the GPIO is from an expander on I2C, this warning will rise obviously. Straight forward fix. drivers/gpu/drm/bridge/ti-sn65dsi83.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index 1f02596d6db4..2927fa2abd3d 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c @@ -346,7 +346,7 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge, } /* Deassert reset */ - gpiod_set_value(ctx->enable_gpio, 1); + gpiod_set_value_cansleep(ctx->enable_gpio, 1); usleep_range(1000, 1100); /* Get the LVDS format from the bridge state. */ @@ -497,7 +497,7 @@ static void sn65dsi83_atomic_disable(struct drm_bridge *bridge, int ret; /* Put the chip in reset, pull EN line low, and assure 10ms reset low timing. */ - gpiod_set_value(ctx->enable_gpio, 0); + gpiod_set_value_cansleep(ctx->enable_gpio, 0); usleep_range(1, 11000); ret = regulator_disable(ctx->vcc); -- 2.25.1
Re: (EXT) [PATCH] drm: mxsfb: Simplify LCDIF clock handling
Hi Marek,
I like the overall idea. Thanks for the effort.
Am Sonntag, 6. Februar 2022, 19:55:55 CET schrieb Marek Vasut:
> The current clock handling in the LCDIF driver is a convoluted mess.
> Implement runtime PM ops which turn the clock ON and OFF and let the
> pm_runtime_get_sync()/pm_runtime_put_sync() calls in .atomic_enable
> and .atomic_disable callbacks turn the clock ON and OFF at the right
> time.
> Signed-off-by: Marek Vasut
> Cc: Alexander Stein
> Cc: Laurent Pinchart
> Cc: Lucas Stach
> Cc: Peng Fan
> Cc: Robby Cai
> Cc: Sam Ravnborg
> Cc: Stefan Agner
> ---
> drivers/gpu/drm/mxsfb/mxsfb_drv.c | 85 ++-
> drivers/gpu/drm/mxsfb/mxsfb_kms.c | 18 ++-
> 2 files changed, 54 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> b/drivers/gpu/drm/mxsfb/mxsfb_drv.c index 375f26d4a4172..4ff3c6195dd0c
> 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> @@ -72,18 +72,6 @@ static const struct mxsfb_devdata mxsfb_devdata[] = {
> },
> };
>
> -void mxsfb_enable_axi_clk(struct mxsfb_drm_private *mxsfb)
> -{
> - if (mxsfb->clk_axi)
> - clk_prepare_enable(mxsfb->clk_axi);
> -}
> -
> -void mxsfb_disable_axi_clk(struct mxsfb_drm_private *mxsfb)
> -{
> - if (mxsfb->clk_axi)
> - clk_disable_unprepare(mxsfb->clk_axi);
> -}
> -
The declarations for mxsfb_enable_axi_clk() and mxsfb_disable_axi_clk() are
still in drivers/gpu/drm/mxsfb/mxsfb_drv.h. Please remove them as well.
You will then notice that they are still used at some places.
> static struct drm_framebuffer *
> mxsfb_fb_create(struct drm_device *dev, struct drm_file *file_priv,
> const struct drm_mode_fb_cmd2 *mode_cmd)
> @@ -224,33 +212,31 @@ static int mxsfb_load(struct drm_device *drm,
> if (IS_ERR(mxsfb->clk))
> return PTR_ERR(mxsfb->clk);
>
> - mxsfb->clk_axi = devm_clk_get(drm->dev, "axi");
> + mxsfb->clk_axi = devm_clk_get_optional(drm->dev, "axi");
> if (IS_ERR(mxsfb->clk_axi))
> - mxsfb->clk_axi = NULL;
> + return PTR_ERR(mxsfb->clk_axi);
>
> - mxsfb->clk_disp_axi = devm_clk_get(drm->dev, "disp_axi");
> + mxsfb->clk_disp_axi = devm_clk_get_optional(drm->dev, "disp_axi");
> if (IS_ERR(mxsfb->clk_disp_axi))
> - mxsfb->clk_disp_axi = NULL;
> + return PTR_ERR(mxsfb->clk_disp_axi);
>
> ret = dma_set_mask_and_coherent(drm->dev, DMA_BIT_MASK(32));
> if (ret)
> return ret;
>
> - pm_runtime_enable(drm->dev);
> -
> /* Modeset init */
> drm_mode_config_init(drm);
>
> ret = mxsfb_kms_init(mxsfb);
> if (ret < 0) {
> dev_err(drm->dev, "Failed to initialize KMS
pipeline\n");
> - goto err_vblank;
> + return ret;
> }
>
> ret = drm_vblank_init(drm, drm->mode_config.num_crtc);
> if (ret < 0) {
> dev_err(drm->dev, "Failed to initialise vblank\n");
> - goto err_vblank;
> + return ret;
> }
>
> /* Start with vertical blanking interrupt reporting disabled. */
> @@ -260,7 +246,7 @@ static int mxsfb_load(struct drm_device *drm,
> if (ret) {
> if (ret != -EPROBE_DEFER)
> dev_err(drm->dev, "Cannot connect bridge:
%d\n", ret);
> - goto err_vblank;
> + return ret;
> }
>
> drm->mode_config.min_width = MXSFB_MIN_XRES;
> @@ -277,13 +263,10 @@ static int mxsfb_load(struct drm_device *drm,
> goto err_vblank;
You are still using err_vblank here which gets removed below.
Alexander
> mxsfb->irq = ret;
>
> - pm_runtime_get_sync(drm->dev);
> ret = mxsfb_irq_install(drm, mxsfb->irq);
> - pm_runtime_put_sync(drm->dev);
> -
> if (ret < 0) {
> dev_err(drm->dev, "Failed to install IRQ handler\n");
> - goto err_vblank;
> + return ret;
> }
>
> drm_kms_helper_poll_init(drm);
> @@ -292,12 +275,9 @@ static int mxsfb_load(struct drm_device *drm,
>
> drm_helper_hpd_irq_event(drm);
>
> - return 0;
> -
> -err_vblank:
> - pm_runtime_disable(drm->dev);
> + pm_runtime_enable(drm->dev);
>
> - return ret;
> + return 0;
> }
>
> static void mxsfb_unload(struct drm_device *drm)
> @@ -305,9 +285,7 @@ static void mxsfb
Re: (EXT) [PATCH v2 00/12] drm: bridge: Add Samsung MIPI DSIM bridge
Hello Jagan, thanks for the second version of this patchset. Am Mittwoch, 4. Mai 2022, 13:40:09 CEST schrieb Jagan Teki: > This series supports common bridge support for Samsung MIPI DSIM > which is used in Exynos and i.MX8MM SoC's. > > Previous v1 can be available here [1]. > > The final bridge supports both the Exynos and i.MX8MM DSI devices. > > On, summary this patch-set break the entire DSIM driver into > - platform specific glue code for platform ops, component_ops. > - common bridge driver which handle platform glue init and invoke. > > Patch : Samsung DSIM bridge > > Patch 0001: Common lookup code for OF-graph or child > > Patch 0002: platform init flag via driver_data > > Patch 0003/10: bridge fixes, atomic API's > > Patch 0011: document fsl,imx8mm-mipi-dsim > > Patch 0012: add i.MX8MM DSIM support > > Tested in Engicam i.Core MX8M Mini SoM. > > Anyone interested, please have a look on this repo [2] > > [2] https://github.com/openedev/kernel/tree/imx8mm-dsi-v2 > [1] > https://patchwork.kernel.org/project/dri-devel/cover/20220408162108.184583-> > 1-ja...@amarulasolutions.com/ > > Any inputs? I was able to get my LVDS display running using this driver and an LVDS bridge. Actually my setup is similar to yours. My chain is like this: MIPI-DSI -> sn65dsi83 -> LVDS panel I noticed some things though: My setup only works if I use less than 4 lanes. See [1]. When using 4 lanes the image is flickering, but the content is "visible". Your DT has only 2 lanes configured, do you have the possibility to use 4 lanes? I have no idea how to tackle this. It might be the DSIM side or the bridge side. Apparently the downstream kernel from NXP supports 4 lanes, if I can trust the config. I have no way to verify this though. Another thing is I get the following warning > sn65dsi83 2-002d: Unsupported LVDS bus format 0x100a, please check output bridge driver. Falling back to SPWG24. This seems to be caused by a wrong bridge chain. Using commit 81e80429 at [2] I get the following output: > bridge chain: /soc@0/bus@3080/i2c@30a4/dsi-lvds-bridge@2d -> / panel_lvds0 -> /soc@0/bus@32c0/dsi@32e1 -> Which seems weird. I would have expected something like dsi@32e1 -> dsi-lvds-bridge@2d -> panel_lvds0 Do you happen to see somthing similar? But this is completely unrelated to your patchset though. Also unloading the samsung_dsim driver raises a regulator warning: [ cut here ] WARNING: CPU: 2 PID: 381 at drivers/regulator/core.c:2275 _regulator_put.part. 0+0x38/0x40 Modules linked in: caam_jr caamhash_desc caamalg_desc crypto_engine rng_core authenc libdes hantro_vpu(C) v4l2_vp9 v4l2_h264 snd_soc_ fsl_asoc_card crct10dif_ce snd_soc_tlv320aic32x4_spi videobuf2_dma_contig phy_fsl_imx8m_pcie v4l2_mem2mem samsung_dsim(-) snd_soc_tlv 320aic32x4_i2c snd_soc_tlv320aic32x4 caam error imx8mm_thermal imx_sdma pwm_beeper fuse ipv6 CPU: 2 PID: 381 Comm: modprobe Tainted: G C5.18.0-rc5- next-20220504+ #204 03c84d7b1600b734091c3159e797071c8f65061c Hardware name: TQ-Systems GmbH i.MX8MM TQMa8MxML on MBa8Mx (DT) pstate: 8005 (Nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) pc : _regulator_put.part.0+0x38/0x40 lr : regulator_bulk_free+0x58/0x80 sp : 8aeb3af0 x29: 8aeb3af0 x28: 0360bb00 x27: x26: 89bad438 x25: 00276890 x24: 0009 x23: 0360bb00 x22: 03543268 x21: 89b85280 x20: 05587800 x19: 03543238 x18: x17: x16: x15: 6d3d4d4554535953 x14: 42555300302e6973 x13: 4553003338697364 x12: x11: x10: 0ab0 x9 : 8aeb38f0 x8 : 0360c610 x7 : x6 : x5 : 892dc468 x4 : 0360bb00 x3 : x2 : 0360bb00 x1 : 0001 x0 : 0558
Re: (EXT) Re: (EXT) [PATCH v2 00/12] drm: bridge: Add Samsung MIPI DSIM bridge
Hello Jagan, thanks for the quick response. Am Donnerstag, 5. Mai 2022, 09:38:48 CEST schrieb Jagan Teki: > On Thu, May 5, 2022 at 12:57 PM Alexander Stein > > wrote: > > Hello Jagan, > > > > thanks for the second version of this patchset. > > > > Am Mittwoch, 4. Mai 2022, 13:40:09 CEST schrieb Jagan Teki: > > > This series supports common bridge support for Samsung MIPI DSIM > > > which is used in Exynos and i.MX8MM SoC's. > > > > > > Previous v1 can be available here [1]. > > > > > > The final bridge supports both the Exynos and i.MX8MM DSI devices. > > > > > > On, summary this patch-set break the entire DSIM driver into > > > - platform specific glue code for platform ops, component_ops. > > > - common bridge driver which handle platform glue init and invoke. > > > > > > Patch : Samsung DSIM bridge > > > > > > Patch 0001: Common lookup code for OF-graph or child > > > > > > Patch 0002: platform init flag via driver_data > > > > > > Patch 0003/10: bridge fixes, atomic API's > > > > > > Patch 0011: document fsl,imx8mm-mipi-dsim > > > > > > Patch 0012: add i.MX8MM DSIM support > > > > > > Tested in Engicam i.Core MX8M Mini SoM. > > > > > > Anyone interested, please have a look on this repo [2] > > > > > > [2] https://github.com/openedev/kernel/tree/imx8mm-dsi-v2 > > > [1] > > > https://patchwork.kernel.org/project/dri-devel/cover/20220408162108.1845 > > > 83-> 1-ja...@amarulasolutions.com/ > > > > > > Any inputs? > > > > I was able to get my LVDS display running using this driver and an LVDS > > bridge. Actually my setup is similar to yours. My chain is like this: > > MIPI-DSI -> sn65dsi83 -> LVDS panel > > I noticed some things though: > > My setup only works if I use less than 4 lanes. See [1]. When using 4 > > lanes > > the image is flickering, but the content is "visible". Your DT has only 2 > > lanes configured, do you have the possibility to use 4 lanes? I have no > > idea how to tackle this. It might be the DSIM side or the bridge side. > > Apparently the downstream kernel from NXP supports 4 lanes, if I can trust > > the config. I have no way to verify this though. > > What is dsi_lvds_bridge node? have you added your dts changes on top > of imx8mm-dsi-v2 branch I'm pointing it. I cherry-picked your commits and applied them on (currently) next-20220504. Maybe you missed the links at the end of my mail. The branch I am talking about is https://github.com/tq-steina/linux/commits/imx8mm-dsi-lvds This includes your commits as well as my additions. > I will check 4 lanes and let you know. Great, thanks. > > Another thing is I get the following warning > > > > > sn65dsi83 2-002d: Unsupported LVDS bus format 0x100a, please check > > > output > > > > bridge driver. Falling back to SPWG24. > > This couldn't be much affected but will fix it. > > > This seems to be caused by a wrong bridge chain. Using commit 81e80429 at > > [2]> > > I get the following output: > > > bridge chain: /soc@0/bus@3080/i2c@30a4/dsi-lvds-bridge@2d -> / > > > > panel_lvds0 -> /soc@0/bus@32c0/dsi@32e1 -> > > Which seems weird. I would have expected something like > > dsi@32e1 -> dsi-lvds-bridge@2d -> panel_lvds0 > > Do you happen to see somthing similar? But this is completely unrelated to > > your patchset though. > > Can you share the link to the exact commit? This is the commit introducing this output: https://github.com/tq-steina/linux/commit/ 81e80429341cd0a4f119ec9cf50839498915443b Best regards, Alexander
Re: (EXT) Re: (EXT) [PATCH v2 00/12] drm: bridge: Add Samsung MIPI DSIM bridge
Am Donnerstag, 5. Mai 2022, 09:38:48 CEST schrieb Jagan Teki:
> On Thu, May 5, 2022 at 12:57 PM Alexander Stein
>
> wrote:
> > Hello Jagan,
> >
> > thanks for the second version of this patchset.
> >
> > Am Mittwoch, 4. Mai 2022, 13:40:09 CEST schrieb Jagan Teki:
> > > This series supports common bridge support for Samsung MIPI DSIM
> > > which is used in Exynos and i.MX8MM SoC's.
> > >
> > > Previous v1 can be available here [1].
> > >
> > > The final bridge supports both the Exynos and i.MX8MM DSI devices.
> > >
> > > On, summary this patch-set break the entire DSIM driver into
> > > - platform specific glue code for platform ops, component_ops.
> > > - common bridge driver which handle platform glue init and invoke.
> > >
> > > Patch : Samsung DSIM bridge
> > >
> > > Patch 0001: Common lookup code for OF-graph or child
> > >
> > > Patch 0002: platform init flag via driver_data
> > >
> > > Patch 0003/10: bridge fixes, atomic API's
> > >
> > > Patch 0011: document fsl,imx8mm-mipi-dsim
> > >
> > > Patch 0012: add i.MX8MM DSIM support
> > >
> > > Tested in Engicam i.Core MX8M Mini SoM.
> > >
> > > Anyone interested, please have a look on this repo [2]
> > >
> > > [2] https://github.com/openedev/kernel/tree/imx8mm-dsi-v2
> > > [1]
> > > https://patchwork.kernel.org/project/dri-devel/cover/20220408162108.1845
> > > 83-> 1-ja...@amarulasolutions.com/
> > >
> > > Any inputs?
> >
> > I was able to get my LVDS display running using this driver and an LVDS
> > bridge. Actually my setup is similar to yours. My chain is like this:
> > MIPI-DSI -> sn65dsi83 -> LVDS panel
> > I noticed some things though:
> > My setup only works if I use less than 4 lanes. See [1]. When using 4
> > lanes
> > the image is flickering, but the content is "visible". Your DT has only 2
> > lanes configured, do you have the possibility to use 4 lanes? I have no
> > idea how to tackle this. It might be the DSIM side or the bridge side.
> > Apparently the downstream kernel from NXP supports 4 lanes, if I can trust
> > the config. I have no way to verify this though.
>
> What is dsi_lvds_bridge node? have you added your dts changes on top
> of imx8mm-dsi-v2 branch I'm pointing it.
>
> I will check 4 lanes and let you know.
>
> > Another thing is I get the following warning
> >
> > > sn65dsi83 2-002d: Unsupported LVDS bus format 0x100a, please check
> > > output
> >
> > bridge driver. Falling back to SPWG24.
>
> This couldn't be much affected but will fix it.
I found the cause. You need the following diff:
8<-
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/
samsung-dsim.c
index 138323dec0eb..7fb96dc7bb2e 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1427,7 +1427,7 @@ static int samsung_dsim_attach(struct drm_bridge
*bridge,
{
struct samsung_dsim *dsi = bridge_to_dsi(bridge);
- return drm_bridge_attach(bridge->encoder, dsi->out_bridge, NULL,
flags);
+ return drm_bridge_attach(bridge->encoder, dsi->out_bridge, bridge,
flags);
}
static const struct drm_bridge_funcs samsung_dsim_bridge_funcs = {
8<-
Best regards,
Alexander
Re: [PATCH v5 1/4] drm/lcdif: Clean up headers
Hello Marek,
Am Freitag, 19. August 2022, 16:08:49 CEST schrieb Marek Vasut:
> Drop unneeded headers, sort rest alphabetically, no functional change.
>
> Acked-by: Sam Ravnborg
> Reviewed-by: Liu Ying
> Reported-by: Liu Ying
> Tested-by: Martyn Welch
> Fixes: 9db35bb349a0e ("drm: lcdif: Add support for i.MX8MP LCDIF variant")
> Signed-off-by: Marek Vasut
> Cc: Alexander Stein
> Cc: Laurent Pinchart
> Cc: Liu Ying
> Cc: Lucas Stach
> Cc: Marek Vasut
> Cc: Martyn Welch
> Cc: Peng Fan
> Cc: Robby Cai
> Cc: Sam Ravnborg
> Cc: Stefan Agner
> ---
> V2: Add RB from Liu
> V3: Add TB from Martyn from V1
> V4: Add AB from Sam from V2
> V5: Rebase on current drm-misc-next
> ---
> drivers/gpu/drm/mxsfb/lcdif_drv.c | 3 ---
> drivers/gpu/drm/mxsfb/lcdif_drv.h | 1 +
> drivers/gpu/drm/mxsfb/lcdif_kms.c | 2 +-
> 3 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> b/drivers/gpu/drm/mxsfb/lcdif_drv.c index 05db135800db0..4f16947212b60
> 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> @@ -8,7 +8,6 @@
> #include
> #include
> #include
> -#include
> #include
> #include
> #include
> @@ -16,10 +15,8 @@
>
> #include
> #include
> -#include
> #include
> #include
> -#include
> #include
> #include
> #include
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.h
> b/drivers/gpu/drm/mxsfb/lcdif_drv.h index cb916341e8454..6cdba6e20c02b
> 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_drv.h
> +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.h
> @@ -8,6 +8,7 @@
> #ifndef __LCDIF_DRV_H__
> #define __LCDIF_DRV_H__
>
> +#include
> #include
> #include
> #include
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> b/drivers/gpu/drm/mxsfb/lcdif_kms.c index c7efc0d27f0e3..750e7e7ea8e81
> 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> @@ -17,9 +17,9 @@
> #include
> #include
> #include
> -#include
> #include
> #include
> +#include
> #include
> #include
> #include
Reviewed-by: Alexander Stein
Re: [PATCH v5 4/4] drm/lcdif: switch to devm_drm_of_get_bridge
Hello Marek,
Am Freitag, 19. August 2022, 16:08:52 CEST schrieb Marek Vasut:
> The function "drm_of_find_panel_or_bridge" has been deprecated in
> favor of "devm_drm_of_get_bridge".
>
> Switch to the new function and reduce boilerplate.
>
> Acked-by: Sam Ravnborg
> Reviewed-by: Liu Ying
> Reported-by: Liu Ying
> Tested-by: Martyn Welch
> Fixes: 9db35bb349a0e ("drm: lcdif: Add support for i.MX8MP LCDIF variant")
> Signed-off-by: Marek Vasut
> Cc: Alexander Stein
> Cc: Laurent Pinchart
> Cc: Liu Ying
> Cc: Lucas Stach
> Cc: Marek Vasut
> Cc: Martyn Welch
> Cc: Peng Fan
> Cc: Robby Cai
> Cc: Sam Ravnborg
> Cc: Stefan Agner
> ---
> V2: Add RB from Liu
> V3: Add TB from Martyn from V1
> V4: Add AB from Sam from V2
> V5: Rebase on current drm-misc-next
> ---
> drivers/gpu/drm/mxsfb/lcdif_drv.c | 18 +++---
> 1 file changed, 3 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> b/drivers/gpu/drm/mxsfb/lcdif_drv.c index 4f16947212b60..075002ed6fb09
> 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> @@ -42,23 +42,11 @@ static int lcdif_attach_bridge(struct lcdif_drm_private
> *lcdif) {
> struct drm_device *drm = lcdif->drm;
> struct drm_bridge *bridge;
> - struct drm_panel *panel;
> int ret;
>
> - ret = drm_of_find_panel_or_bridge(drm->dev->of_node, 0, 0, &panel,
> - &bridge);
> - if (ret)
> - return ret;
> -
> - if (panel) {
> - bridge = devm_drm_panel_bridge_add_typed(drm->dev,
panel,
> -
DRM_MODE_CONNECTOR_DPI);
> - if (IS_ERR(bridge))
> - return PTR_ERR(bridge);
> - }
> -
> - if (!bridge)
> - return -ENODEV;
> + bridge = devm_drm_of_get_bridge(drm->dev, drm->dev->of_node, 0, 0);
> + if (IS_ERR(bridge))
> + return PTR_ERR(bridge);
>
> ret = drm_bridge_attach(&lcdif->encoder, bridge, NULL, 0);
> if (ret)
Reviewed-by: Alexander Stein
Re: [PATCH v5 3/4] drm/lcdif: Clean up debug prints and comments
Hello Marek,
Am Freitag, 19. August 2022, 16:08:51 CEST schrieb Marek Vasut:
> Update debug print to report bridge timings over connector ones.
> Drop missed comment commit from mxsfb.
>
> Acked-by: Sam Ravnborg
> Reviewed-by: Liu Ying
> Reported-by: Liu Ying
> Tested-by: Martyn Welch
> Fixes: 9db35bb349a0e ("drm: lcdif: Add support for i.MX8MP LCDIF variant")
> Signed-off-by: Marek Vasut
> Cc: Alexander Stein
> Cc: Laurent Pinchart
> Cc: Liu Ying
> Cc: Lucas Stach
> Cc: Marek Vasut
> Cc: Martyn Welch
> Cc: Peng Fan
> Cc: Robby Cai
> Cc: Sam Ravnborg
> Cc: Stefan Agner
> ---
> V2: Add RB from Liu
> V3: Add TB from Martyn from V1
> V4: Add AB from Sam from V2
> V5: Rebase on current drm-misc-next
> ---
> drivers/gpu/drm/mxsfb/lcdif_kms.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> b/drivers/gpu/drm/mxsfb/lcdif_kms.c index db7a90e5497c6..b1092aab14231
> 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> @@ -203,7 +203,7 @@ static void lcdif_crtc_mode_set_nofb(struct
> lcdif_drm_private *lcdif, DRM_DEV_DEBUG_DRIVER(drm->dev, "Pixel clock:
> %dkHz (actual: %dkHz)\n", m->crtc_clock,
>(int)(clk_get_rate(lcdif->clk) / 1000));
> - DRM_DEV_DEBUG_DRIVER(drm->dev, "Connector bus_flags: 0x%08X\n",
> + DRM_DEV_DEBUG_DRIVER(drm->dev, "Bridge bus_flags: 0x%08X\n",
>bus_flags);
> DRM_DEV_DEBUG_DRIVER(drm->dev, "Mode flags: 0x%08X\n", m->flags);
Is there any benefit to explicitly state it is the bridge input flags?
Anyway:
Reviewed-by: Alexander Stein
Re: (EXT) Re: (EXT) [PATCH v2 00/12] drm: bridge: Add Samsung MIPI DSIM bridge
Hi Marek,
Am Freitag, 6. Mai 2022, 10:57:05 CEST schrieb Marek Szyprowski:
> Hi Alexander,
>
> On 05.05.2022 13:55, Alexander Stein wrote:
> > Am Donnerstag, 5. Mai 2022, 09:38:48 CEST schrieb Jagan Teki:
> >> On Thu, May 5, 2022 at 12:57 PM Alexander Stein
> >>
> >> wrote:
> >>> Hello Jagan,
> >>>
> >>> thanks for the second version of this patchset.
> >>>
> >>> Am Mittwoch, 4. Mai 2022, 13:40:09 CEST schrieb Jagan Teki:
> >>>> This series supports common bridge support for Samsung MIPI DSIM
> >>>> which is used in Exynos and i.MX8MM SoC's.
> >>>>
> >>>> Previous v1 can be available here [1].
> >>>>
> >>>> The final bridge supports both the Exynos and i.MX8MM DSI devices.
> >>>>
> >>>> On, summary this patch-set break the entire DSIM driver into
> >>>> - platform specific glue code for platform ops, component_ops.
> >>>> - common bridge driver which handle platform glue init and invoke.
> >>>>
> >>>> Patch : Samsung DSIM bridge
> >>>>
> >>>> Patch 0001: Common lookup code for OF-graph or child
> >>>>
> >>>> Patch 0002: platform init flag via driver_data
> >>>>
> >>>> Patch 0003/10: bridge fixes, atomic API's
> >>>>
> >>>> Patch 0011: document fsl,imx8mm-mipi-dsim
> >>>>
> >>>> Patch 0012: add i.MX8MM DSIM support
> >>>>
> >>>> Tested in Engicam i.Core MX8M Mini SoM.
> >>>>
> >>>> Anyone interested, please have a look on this repo [2]
> >>>>
> >>>> [2]
> >>>> https://protect2.fireeye.com/v1/url?k=569d5207-09066afa-569cd948-000ba
> >>>> bff317b-7f7572918a36c54e&q=1&e=1305c5cc-33c8-467e-a498-6862a854cf94&u=h
> >>>> ttps%3A%2F%2Fgithub.com%2Fopenedev%2Fkernel%2Ftree%2Fimx8mm-dsi-v2 [1]
> >>>> https://patchwork.kernel.org/project/dri-devel/cover/20220408162108.184
> >>>> 5
> >>>> 83-> 1-ja...@amarulasolutions.com/
> >>>>
> >>>> Any inputs?
> >>>
> >>> I was able to get my LVDS display running using this driver and an LVDS
> >>> bridge. Actually my setup is similar to yours. My chain is like this:
> >>> MIPI-DSI -> sn65dsi83 -> LVDS panel
> >>> I noticed some things though:
> >>> My setup only works if I use less than 4 lanes. See [1]. When using 4
> >>> lanes
> >>> the image is flickering, but the content is "visible". Your DT has only
> >>> 2
> >>> lanes configured, do you have the possibility to use 4 lanes? I have no
> >>> idea how to tackle this. It might be the DSIM side or the bridge side.
> >>> Apparently the downstream kernel from NXP supports 4 lanes, if I can
> >>> trust
> >>> the config. I have no way to verify this though.
> >>
> >> What is dsi_lvds_bridge node? have you added your dts changes on top
> >> of imx8mm-dsi-v2 branch I'm pointing it.
> >>
> >> I will check 4 lanes and let you know.
> >>
> >>> Another thing is I get the following warning
> >>>
> >>>> sn65dsi83 2-002d: Unsupported LVDS bus format 0x100a, please check
> >>>> output
> >>>
> >>> bridge driver. Falling back to SPWG24.
> >>
> >> This couldn't be much affected but will fix it.
> >
> > I found the cause. You need the following diff:
> > 8<-
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> > b/drivers/gpu/drm/bridge/ samsung-dsim.c
> > index 138323dec0eb..7fb96dc7bb2e 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -1427,7 +1427,7 @@ static int samsung_dsim_attach(struct drm_bridge
> > *bridge,
> >
> > {
> >
> > struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> >
> > - return drm_bridge_attach(bridge->encoder, dsi->out_bridge, NULL,
> > flags);
> > + return drm_bridge_attach(bridge->encoder, dsi->out_bridge, bridge,
> > flags);
> >
> > }
> >
> > static const struct drm_bridge_funcs samsung_dsim_bridge_funcs = {
> >
> > 8<-
>
> Well, basically, the above change breaks DSI panels. :(
That's too
Re: (EXT) [PATCH v0.5 0/9] i.MX8MP HDMI support
Hi Lucas, Am Freitag, 6. Mai 2022, 20:10:25 CEST schrieb Lucas Stach: > second round of the i.MX8MP HDMI work. Still not split up into proper > parts for merging through the various trees this needs to go into, but > should make it easy for people to test. > > I've worked in the feedback I got from the last round, including fixing > the system hang that could happen when the drivers were built as modules. > > Series is based on linux-next/master, as there are some prerequisite > patches in both the drm and imx tree already. The last patch from [1] > and the patches from [2] need to be applied. Please note that this series > expects the sync polarity from the LCDIF to be set according to the > comments I made in [2]. Please test and provide feedback. Thanks for the 2nd round of HDMI support patches. Sorry I wasn't able to reply to your questions, but the PLL locking seems to be gone on my system. I still get the error > imx-lcdif 32fc6000.display-controller: Unknown media bus format 0x200f To answer the other question on the last patchset > Do have a 4k HDMI display connected that wants to do YUV input? That's > something I have to admit I didn't test yet and would be likely to > cause this bus format selection. This is a FullHD HDMI monitor, ASUS PB238Q. Apparently the color format is YCBCR422. From what I can see is that dw_hdmi_bridge_atomic_get_output_bus_fmts() adds MEDIA_BUS_FMT_UYVY8_1X16 (0x200f) to the output formats. This is then passed to select_bus_fmt_recursive() on the bridge chain. For 0x200f dw_hdmi_bridge_atomic_get_input_bus_fmts() returns 3 input formats with MEDIA_BUS_FMT_UYVY8_1X16 being the 1st. Each entry is then probed on pvi_bridge_get_input_bus_fmts(), which just forwards to dw_hdmi_bridge_atomic_get_input_bus_fmts(). Note: At this point it is only checked whether the input format can be output. As 0x200f is supported by dw_hdmi this format will finally be selected, which is not supported by lcdif_kms, resulting in the error message above. A quick&dirty hack to workaround is the following diff which just changes the order of the format to be tested: ---8<--- --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2816,9 +2816,9 @@ static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; break; case MEDIA_BUS_FMT_UYVY8_1X16: + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; - input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; break; /* 10bit */ ---8<--- With this MEDIA_BUS_FMT_RGB888_1X24 is probed 1st (and selected) which actually is supported by lcdif_kms. For the records, I used this diff for lcdif driver to fix the polarity issue ---8<--- --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c @@ -89,12 +89,12 @@ static void lcdif_set_mode(struct lcdif_drm_private *lcdif, u32 bus_flags) struct drm_display_mode *m = &lcdif->crtc.state->adjusted_mode; u32 ctrl = 0; - if (m->flags & DRM_MODE_FLAG_PHSYNC) + if (m->flags & DRM_MODE_FLAG_NHSYNC) ctrl |= CTRL_INV_HS; - if (m->flags & DRM_MODE_FLAG_PVSYNC) + if (m->flags & DRM_MODE_FLAG_NVSYNC) ctrl |= CTRL_INV_VS; /* Make sure Data Enable is high active by default */ - if (!(bus_flags & DRM_BUS_FLAG_DE_LOW)) + if ((bus_flags & DRM_BUS_FLAG_DE_LOW)) ctrl |= CTRL_INV_DE; if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE) ctrl |= CTRL_INV_PXCK; ---8<--- With both changes from above I can see the weston desktop. Alexander > [1] > https://lore.kernel.org/all/20220406153402.1265474-1-l.st...@pengutronix.de > / [2] https://lore.kernel.org/all/20220322142853.125880-1-ma...@denx.de/ > > Lucas Stach (9): > dt-bindings: display: imx: add binding for i.MX8MP HDMI TX > drm/imx: add bridge wrapper driver for i.MX8MP DWC HDMI > dt-bindings: display: imx: add binding for i.MX8MP HDMI PVI > drm/imx: add driver for HDMI TX Parallel Video Interface > dt-bindings: phy: add binding for the i.MX8MP HDMI PHY > phy: freescale: add Samsung HDMI PHY > arm64: dts: imx8mp: add HDMI irqsteer > arm64: dts: imx8mp: add HDMI display pipeline > arm64: dts: imx8mp-evk: enable HDMI > > .../display/imx/fsl,imx8mp-hdmi-pvi.yaml | 83 ++ > .../bindings/display/imx/fsl,imx8mp-hdmi.yaml | 73 ++ > .../bindings/phy/fsl,imx8mp-hdmi-phy.yaml | 62 + > arch/arm64/boot/dts/freescale/imx8mp-evk.dts | 19 + > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 94 ++ > drivers/gpu/drm/imx/Kconfig |1 + > drivers/gpu/drm/imx/Makefile |2 + > drivers/gpu/drm/imx/bridge/Kconfig| 18 + > drivers/gpu/drm
Re: [PATCH v4 2/2] drm: lcdif: Add support for i.MX8MP LCDIF variant
Hi Marek, Am Donnerstag, 19. Mai 2022, 13:48:49 CEST schrieb Marek Vasut: > Add support for i.MX8MP LCDIF variant. This is called LCDIFv3 and is > completely different from the LCDIFv3 found in i.MX23 in that it has > a completely scrambled register layout compared to all previous LCDIF > variants. The new LCDIFv3 also supports 36bit address space. > > Add a separate driver which is really a fork of MXSFB driver with the > i.MX8MP LCDIF variant handling filled in. > > Signed-off-by: Marek Vasut > Cc: Alexander Stein > Cc: Laurent Pinchart > Cc: Lucas Stach > Cc: Peng Fan > Cc: Robby Cai > Cc: Sam Ravnborg > Cc: Stefan Agner > --- > V2: - Drop the pitch check from lcdif_fb_create() > - Drop connector caching > - Wait for shadow load bit to be cleared in IRQ handler > - Make all clock mandatory and grab them all by name > - Wait for EN to be cleared in lcdif_disable_controller > - Rename to imx-lcdif > - Move shadow load to atomic_flush > V3: - Invert DE polarity to match MX8MPRM datasheet > - Enable CSC in RGB to YUV mode for MEDIA_BUS_FMT_UYVY8_1X16 > V4: - Drop lcdif_overlay_plane_formats, it is unused Thanks for the update. With your change in V3 my HDMI output works now without that hack mentioned. weston screen as well as 'fb-test -p 5' output seems sensible. Unfortunately this isn't the case for LVDS output on LCDIF2. I somehow managed to get the DT nodes for LCDIF and LDB done. Also the necessary addition to imx8m-blk-ctl. So eventually I can see some output. But the screen is cutoff on the right side of about 15-20% and the screen is flickering slighty. This is especially visible in 'fb-test -p 5'. The red bars are only visible to less than 1/3 and the text as well as the diagonal lines are flickering. Colors are correct though. For the record: I am using a 'tianma,tm070jvhg33' panel. > --- > drivers/gpu/drm/mxsfb/Kconfig | 16 + > drivers/gpu/drm/mxsfb/Makefile | 2 + > drivers/gpu/drm/mxsfb/lcdif_drv.c | 361 + > drivers/gpu/drm/mxsfb/lcdif_drv.h | 47 +++ > drivers/gpu/drm/mxsfb/lcdif_kms.c | 497 + > drivers/gpu/drm/mxsfb/lcdif_regs.h | 257 +++ > 6 files changed, 1180 insertions(+) > create mode 100644 drivers/gpu/drm/mxsfb/lcdif_drv.c > create mode 100644 drivers/gpu/drm/mxsfb/lcdif_drv.h > create mode 100644 drivers/gpu/drm/mxsfb/lcdif_kms.c > create mode 100644 drivers/gpu/drm/mxsfb/lcdif_regs.h > > diff --git a/drivers/gpu/drm/mxsfb/Kconfig b/drivers/gpu/drm/mxsfb/Kconfig > index 987170e16ebd6..873551b4552f5 100644 > --- a/drivers/gpu/drm/mxsfb/Kconfig > +++ b/drivers/gpu/drm/mxsfb/Kconfig > @@ -19,3 +19,19 @@ config DRM_MXSFB > i.MX28, i.MX6SX, i.MX7 and i.MX8M). > > If M is selected the module will be called mxsfb. > + > +config DRM_IMX_LCDIF > + tristate "i.MX LCDIFv3 LCD controller" > + depends on DRM && OF > + depends on COMMON_CLK > + select DRM_MXS > + select DRM_KMS_HELPER > + select DRM_GEM_CMA_HELPER > + select DRM_PANEL > + select DRM_PANEL_BRIDGE > + help > + Choose this option if you have an LCDIFv3 LCD controller. > + Those devices are found in various i.MX SoC (i.MX8MP, > + i.MXRT). > + > + If M is selected the module will be called imx-lcdif. > diff --git a/drivers/gpu/drm/mxsfb/Makefile b/drivers/gpu/drm/mxsfb/Makefile > index 26d153896d720..3fa44059b9d85 100644 > --- a/drivers/gpu/drm/mxsfb/Makefile > +++ b/drivers/gpu/drm/mxsfb/Makefile > @@ -1,3 +1,5 @@ > # SPDX-License-Identifier: GPL-2.0-only > mxsfb-y := mxsfb_drv.o mxsfb_kms.o > obj-$(CONFIG_DRM_MXSFB) += mxsfb.o > +imx-lcdif-y := lcdif_drv.o lcdif_kms.o > +obj-$(CONFIG_DRM_IMX_LCDIF) += imx-lcdif.o > diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c > b/drivers/gpu/drm/mxsfb/lcdif_drv.c new file mode 100644 > index 0..3e29c8a768487 > --- /dev/null > +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c > @@ -0,0 +1,361 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (C) 2022 Marek Vasut > + * > + * This code is based on drivers/gpu/drm/mxsfb/mxsfb* > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "lcdif_drv.h" > +#include "lcdif_regs.h" > + > +static struct drm_framebuffer * > +lcdif_fb_create(struct drm_device *
Re: [PATCH v4 2/2] drm: lcdif: Add support for i.MX8MP LCDIF variant
Am Dienstag, 24. Mai 2022, 09:29:43 CEST schrieb Marek Vasut:
> On 5/24/22 09:09, Alexander Stein wrote:
> > Hi Marek,
>
> Hi,
>
> > Am Donnerstag, 19. Mai 2022, 13:48:49 CEST schrieb Marek Vasut:
> >> Add support for i.MX8MP LCDIF variant. This is called LCDIFv3 and is
> >> completely different from the LCDIFv3 found in i.MX23 in that it has
> >> a completely scrambled register layout compared to all previous LCDIF
> >> variants. The new LCDIFv3 also supports 36bit address space.
> >>
> >> Add a separate driver which is really a fork of MXSFB driver with the
> >> i.MX8MP LCDIF variant handling filled in.
> >>
> >> Signed-off-by: Marek Vasut
> >> Cc: Alexander Stein
> >> Cc: Laurent Pinchart
> >> Cc: Lucas Stach
> >> Cc: Peng Fan
> >> Cc: Robby Cai
> >> Cc: Sam Ravnborg
> >> Cc: Stefan Agner
> >> ---
> >> V2: - Drop the pitch check from lcdif_fb_create()
> >>
> >> - Drop connector caching
> >> - Wait for shadow load bit to be cleared in IRQ handler
> >> - Make all clock mandatory and grab them all by name
> >> - Wait for EN to be cleared in lcdif_disable_controller
> >> - Rename to imx-lcdif
> >> - Move shadow load to atomic_flush
> >>
> >> V3: - Invert DE polarity to match MX8MPRM datasheet
> >>
> >> - Enable CSC in RGB to YUV mode for MEDIA_BUS_FMT_UYVY8_1X16
> >>
> >> V4: - Drop lcdif_overlay_plane_formats, it is unused
> >
> > Thanks for the update. With your change in V3 my HDMI output works now
> > without that hack mentioned. weston screen as well as 'fb-test -p 5'
> > output seems sensible.
> > Unfortunately this isn't the case for LVDS output on LCDIF2. I somehow
> > managed to get the DT nodes for LCDIF and LDB done. Also the necessary
> > addition to imx8m-blk-ctl. So eventually I can see some output. But the
> > screen is cutoff on the right side of about 15-20% and the screen is
> > flickering slighty. This is especially visible in 'fb-test -p 5'. The red
> > bars are only visible to less than 1/3 and the text as well as the
> > diagonal lines are flickering. Colors are correct though.
> > For the record: I am using a 'tianma,tm070jvhg33' panel.
>
> Does LDB start working if you apply:
>
> static const struct drm_bridge_funcs funcs = {
> .attach = fsl_ldb_attach,
> - .atomic_check = fsl_ldb_atomic_check,
> .atomic_enable = fsl_ldb_atomic_enable,
> .atomic_disable = fsl_ldb_atomic_disable,
> .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>
> to
>
> drivers/gpu/drm/bridge/fsl-ldb.c
Thanks for the suggestion, but this doesn't change anything. For some reason
bridge_state->output_bus_cfg.flags is 0, rendering this function as a no-op
anyway. Why do we need to invert the DE signal polarity anyway?
I have a hunch this isn't related to data enable, I suspect this would lead to
completly borked colors. But as this is correct, I think something about HSYNC
is borked. VSYNC seems to be correct as the top and bottom lines are fine as
expected.
Best regards,
Alexander
Re: [PATCH v4 2/2] drm: lcdif: Add support for i.MX8MP LCDIF variant
Hi Marek,
Am Dienstag, 24. Mai 2022, 09:29:43 CEST schrieb Marek Vasut:
> On 5/24/22 09:09, Alexander Stein wrote:
> > Hi Marek,
>
> Hi,
>
> > Am Donnerstag, 19. Mai 2022, 13:48:49 CEST schrieb Marek Vasut:
> >> Add support for i.MX8MP LCDIF variant. This is called LCDIFv3 and is
> >> completely different from the LCDIFv3 found in i.MX23 in that it has
> >> a completely scrambled register layout compared to all previous LCDIF
> >> variants. The new LCDIFv3 also supports 36bit address space.
> >>
> >> Add a separate driver which is really a fork of MXSFB driver with the
> >> i.MX8MP LCDIF variant handling filled in.
> >>
> >> Signed-off-by: Marek Vasut
> >> Cc: Alexander Stein
> >> Cc: Laurent Pinchart
> >> Cc: Lucas Stach
> >> Cc: Peng Fan
> >> Cc: Robby Cai
> >> Cc: Sam Ravnborg
> >> Cc: Stefan Agner
> >> ---
> >> V2: - Drop the pitch check from lcdif_fb_create()
> >>
> >> - Drop connector caching
> >> - Wait for shadow load bit to be cleared in IRQ handler
> >> - Make all clock mandatory and grab them all by name
> >> - Wait for EN to be cleared in lcdif_disable_controller
> >> - Rename to imx-lcdif
> >> - Move shadow load to atomic_flush
> >>
> >> V3: - Invert DE polarity to match MX8MPRM datasheet
> >>
> >> - Enable CSC in RGB to YUV mode for MEDIA_BUS_FMT_UYVY8_1X16
> >>
> >> V4: - Drop lcdif_overlay_plane_formats, it is unused
> >
> > Thanks for the update. With your change in V3 my HDMI output works now
> > without that hack mentioned. weston screen as well as 'fb-test -p 5'
> > output seems sensible.
> > Unfortunately this isn't the case for LVDS output on LCDIF2. I somehow
> > managed to get the DT nodes for LCDIF and LDB done. Also the necessary
> > addition to imx8m-blk-ctl. So eventually I can see some output. But the
> > screen is cutoff on the right side of about 15-20% and the screen is
> > flickering slighty. This is especially visible in 'fb-test -p 5'. The red
> > bars are only visible to less than 1/3 and the text as well as the
> > diagonal lines are flickering. Colors are correct though.
> > For the record: I am using a 'tianma,tm070jvhg33' panel.
>
> Does LDB start working if you apply:
>
> static const struct drm_bridge_funcs funcs = {
> .attach = fsl_ldb_attach,
> - .atomic_check = fsl_ldb_atomic_check,
> .atomic_enable = fsl_ldb_atomic_enable,
> .atomic_disable = fsl_ldb_atomic_disable,
> .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>
> to
>
> drivers/gpu/drm/bridge/fsl-ldb.c
I got this working, somehow. The root cause was that the LDB clock was not the
media_disp2_pix_root_clk clock * 7, which is mandatory for LVDS (single link).
excerpt from clk_summary:
video_pll1_out 2 2 0 59400 0 0 5 Y
media_ldb 1 1 0 59400 0 0 5 Y
media_disp2_pix 1 1 06600 0 0 5 Y
media_ldb is too high (should be 46200). I wonder why media_ldb is not a
child from media_disp2_pix (or vice versa) when there is a hard dependency.
There are several solutions:
1.
Set video_pll1 to 103950 and adjust requested pixel clock of the panel
(7425 in this case). Now the dividers match hit the clock rates exactly.
But this renders the display list in panel-simple a bit useless.
2.
Adjust video_pll1_out only (e.g. 478757145). Now the calculated clocks comply
to their mandated ratio. But this might affect other users, e.g. DSI displays
3.
Improve fsl_ldb_atomic_check to set adjusted_mode.clock to an achievable
clock. This way lcdif will pick the new pixelclock to match their ratio.
But there is more work necessary, e.g. ensure the new pixelclock is in the
valid range of the display.
To summarize:
For both HDMI and LVDS using changes unrelated to this lcdif driver:
Tested-by: Alexander Stein
Re: (EXT) [PATCH 5/6] dt-bindings: drm/bridge: ti-sn65dsi83: Add reset controller documentation
Hi Marco, Am Montag, 30. Mai 2022, 17:05:48 CEST schrieb Marco Felsch: > The bridge device can now also be enabled/disabled by an external reset > controller. So the device now supports either enable/disable by simple > GPIO or by an Reset-Controller. > > Signed-off-by: Marco Felsch > --- > .../devicetree/bindings/display/bridge/ti,sn65dsi83.yaml| 6 ++ > 1 file changed, 6 insertions(+) > > diff --git > a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml > b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml index > 7306b9874dc3..eff8360c184e 100644 > --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml > +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml > @@ -35,6 +35,12 @@ properties: >vcc-supply: > description: A 1.8V power supply (see regulator/regulator.yaml). > > + resets: > +maxItems: 1 > +description: | > + Reset specifier for bridge_en pin. This is required only if the > brdige_en + pin is connected to a reset controller. > + >ports: > $ref: /schemas/graph.yaml#/properties/ports Maybe it should be added here, that 'resets' is an alternative to 'enable- gpios' as both are essentially controlling the same pin from the bridge. Best regards Alexander
Re: (EXT) [PATCH v0 09/10] arm64: dts: imx8mp: add HDMI display pipeline
Hi Lucas,
Am Mittwoch, 6. April 2022, 18:01:22 CEST schrieb Lucas Stach:
> This adds the DT nodes for all the peripherals that make up the
> HDMI display pipeline.
>
> Signed-off-by: Lucas Stach
> ---
> arch/arm64/boot/dts/freescale/imx8mp.dtsi | 80 +++
> 1 file changed, 80 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> b/arch/arm64/boot/dts/freescale/imx8mp.dtsi index
> 6b7b5ba32b48..a41da99e9537 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> @@ -1087,6 +1087,86 @@ irqsteer_hdmi: interrupt-controller@32fc2000 {
> clock-names = "ipg";
> power-domains = <&hdmi_blk_ctrl
IMX8MP_HDMIBLK_PD_IRQSTEER>;
> };
> +
> + hdmi_pvi: display-bridge@32fc4000 {
> + compatible = "fsl,imx8mp-hdmi-
pvi";
> + reg = <0x32fc4000 0x40>;
> + power-domains = <&hdmi_blk_ctrl
IMX8MP_HDMIBLK_PD_PVI>;
This should be disabled by default as well. Unless 'hdmi_tx: hdmi@32fd8000' is
enabled, this results in the warning:
imx-hdmi-pvi: probe of 32fc4000.display-bridge failed with error -22
Best regards
Alexander
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
pvi_from_lcdif3: endpoint {
> +
remote-endpoint = <&lcdif3_to_pvi>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> +
pvi_to_hdmi_tx: endpoint {
> +
remote-endpoint = <&hdmi_tx_from_pvi>;
> + };
> + };
> + };
> + };
> +
> + lcdif3: display-controller@32fc6000 {
> + compatible = "fsl,imx8mp-lcdif";
> + reg = <0x32fc6000 0x238>;
> + interrupts = <8
IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-parent =
<&irqsteer_hdmi>;
> + clocks = <&hdmi_tx_phy>,
> + <&clk
IMX8MP_CLK_HDMI_APB>,
> + <&clk
IMX8MP_CLK_HDMI_ROOT>;
> + clock-names = "pix", "axi",
"disp_axi";
> + power-domains = <&hdmi_blk_ctrl
IMX8MP_HDMIBLK_PD_LCDIF>;
> +
> + port {
> + lcdif3_to_pvi: endpoint
{
> + remote-
endpoint = <&pvi_from_lcdif3>;
> + };
> + };
> + };
> +
> + hdmi_tx: hdmi@32fd8000 {
> + compatible = "fsl,imx8mp-hdmi";
> + reg = <0x32fd8000 0x7eff>;
> + interrupts = <0
IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-parent =
<&irqsteer_hdmi>;
> + clocks = <&clk
IMX8MP_CLK_HDMI_APB>,
> + <&clk
IMX8MP_CLK_HDMI_REF_266M>,
> + <&clk IMX8MP_CLK_32K>,
> + <&hdmi_tx_phy>;
> + clock-names = "iahb", "isfr",
"cec", "pix";
> + assigned-clocks = <&clk
IMX8MP_CLK_HDMI_REF_266M>;
> + assigned-clock-parents = <&clk
IMX8MP_SYS_PLL1_266M>;
> + power-domains = <&hdmi_blk_ctrl
IMX8MP_HDMIBLK_PD_HDMI_TX>;
> + reg-io-width = <1>;
> + status = "disabled";
> +
> + port {
> + hdmi_tx_from_pvi:
endpoint {
> + remote-
endpoint = <&pvi_to_hdmi_tx>;
> + };
> + };
> + };
> +
> + hdmi_tx_phy: phy@32fdff00 {
> + compatible = "fsl,imx8mp-hdmi-
phy";
> + reg = <0x32fdff00 0x100>;
> + clocks = <&clk
IMX8MP_CLK_HDMI_APB>,
> + <&clk
IMX8MP_CLK_HDMI_24M>;
> + clock-names = "apb", "ref";
> +
Re: (EXT) [PATCH v5 2/2] drm: lcdif: Add support for i.MX8MP LCDIF variant
Hi,
Am Montag, 13. Juni 2022, 23:31:24 CEST schrieb Marek Vasut:
> Add support for i.MX8MP LCDIF variant. This is called LCDIFv3 and is
> completely different from the LCDIFv3 found in i.MX23 in that it has
> a completely scrambled register layout compared to all previous LCDIF
> variants. The new LCDIFv3 also supports 36bit address space.
>
> Add a separate driver which is really a fork of MXSFB driver with the
> i.MX8MP LCDIF variant handling filled in.
>
> Tested-by: Alexander Stein
> Tested-by: Martyn Welch
> Signed-off-by: Marek Vasut
> Cc: Alexander Stein
> Cc: Laurent Pinchart
> Cc: Lucas Stach
> Cc: Peng Fan
> Cc: Robby Cai
> Cc: Sam Ravnborg
> Cc: Stefan Agner
> ---
> V2: - Drop the pitch check from lcdif_fb_create()
> - Drop connector caching
> - Wait for shadow load bit to be cleared in IRQ handler
> - Make all clock mandatory and grab them all by name
> - Wait for EN to be cleared in lcdif_disable_controller
> - Rename to imx-lcdif
> - Move shadow load to atomic_flush
> V3: - Invert DE polarity to match MX8MPRM datasheet
> - Enable CSC in RGB to YUV mode for MEDIA_BUS_FMT_UYVY8_1X16
> V4: - Drop lcdif_overlay_plane_formats, it is unused
> V5: - Add TB from Martyn
> - Drop lcdif_fb_create in favor of drm_gem_fb_create
> - Pull in mxsfb/ directory from drm top level Makefile
> - Drop busy polling of CTRLDESCL0_5_SHADOW_LOAD_EN in irq handler
> - Use devm_request_irq
> - Drop useless dev.of_node validity check in probe
> - Drop lcdif_*_axi_clk() prototypes
> - Invert HS/VS polarity
> - Drop polling from lcdif_reset_block()
> - Add TB from Alexander
> ---
> drivers/gpu/drm/Makefile | 2 +-
> drivers/gpu/drm/mxsfb/Kconfig | 16 +
> drivers/gpu/drm/mxsfb/Makefile | 2 +
> drivers/gpu/drm/mxsfb/lcdif_drv.c | 342
> drivers/gpu/drm/mxsfb/lcdif_drv.h | 44 +++
> drivers/gpu/drm/mxsfb/lcdif_kms.c | 482 +
> drivers/gpu/drm/mxsfb/lcdif_regs.h | 257 +++
> 7 files changed, 1144 insertions(+), 1 deletion(-)
> create mode 100644 drivers/gpu/drm/mxsfb/lcdif_drv.c
> create mode 100644 drivers/gpu/drm/mxsfb/lcdif_drv.h
> create mode 100644 drivers/gpu/drm/mxsfb/lcdif_kms.c
> create mode 100644 drivers/gpu/drm/mxsfb/lcdif_regs.h
>
>[...]
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> b/drivers/gpu/drm/mxsfb/lcdif_kms.c new file mode 100644
> index 0..d2f65a44889ef
> --- /dev/null
> +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> @@ -0,0 +1,482 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2022 Marek Vasut
> + *
> + * This code is based on drivers/gpu/drm/mxsfb/mxsfb*
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
With the current drm changes in linux-next a
> #include
is needed now.
Best regards,
Alexander
> +#include "lcdif_drv.h"
> +#include "lcdif_regs.h"
> +
> +/* 1 second delay should be plenty of time for block reset */
> +#define RESET_TIMEOUT100
> +
> +/*
> ---
> -- + * CRTC
> + */
> +static void lcdif_set_formats(struct lcdif_drm_private *lcdif,
> + const u32 bus_format)
> +{
> + struct drm_device *drm = lcdif->drm;
> + const u32 format = lcdif->crtc.primary->state->fb->format->format;
> +
> + writel(CSC0_CTRL_BYPASS, lcdif->base + LCDC_V8_CSC0_CTRL);
> +
> + switch (bus_format) {
> + case MEDIA_BUS_FMT_RGB565_1X16:
> + writel(DISP_PARA_LINE_PATTERN_RGB565,
> +lcdif->base + LCDC_V8_DISP_PARA);
> + break;
> + case MEDIA_BUS_FMT_RGB888_1X24:
> + writel(DISP_PARA_LINE_PATTERN_RGB888,
> +lcdif->base + LCDC_V8_DISP_PARA);
> + break;
> + case MEDIA_BUS_FMT_UYVY8_1X16:
> + writel(DISP_PARA_LINE_PATTERN_UYVY_H,
> +lcdif->base + LCDC_V8_DISP_PARA);
> +
> + /* CSC: BT.601 Full Range RGB to YCbCr coefficients. */
> + writel(CSC0_COEF0_A2(0x096) | CSC0_COEF0_A1(0x04c),
> +lcdif->base + LCDC_V8_CSC0_COEF0);
> + writel(CSC0_COEF1_B1(0x7d5) | CSC0_COEF1_A3(0x01d),
> +lcdif->base + LCDC_V8_CSC0_COEF1);
> + writel(CSC0_COEF2_B3(0x080) | CSC0_COEF
Re: [PATCH] drm: mxsfb: Obtain bus flags from bridge state
Am Mittwoch, 6. April 2022, 11:29:29 CEST schrieb Marek Vasut:
> In case the MXSFB is connected to a bridge, attempt to obtain bus flags
> from that bridge state too. The bus flags may specify e.g. the DE signal
> polarity.
>
> Signed-off-by: Marek Vasut
> Cc: Alexander Stein
> Cc: Laurent Pinchart
> Cc: Lucas Stach
> Cc: Peng Fan
> Cc: Robby Cai
> Cc: Sam Ravnborg
> Cc: Stefan Agner
> ---
> drivers/gpu/drm/mxsfb/mxsfb_kms.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index 4baa3db1f3d10..c7f14ef1edc25
> 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> @@ -298,6 +298,7 @@ static int mxsfb_reset_block(struct mxsfb_drm_private
> *mxsfb) }
>
> static void mxsfb_crtc_mode_set_nofb(struct mxsfb_drm_private *mxsfb,
> + struct drm_bridge_state
*bridge_state,
>const u32 bus_format)
> {
> struct drm_device *drm = mxsfb->crtc.dev;
> @@ -307,6 +308,8 @@ static void mxsfb_crtc_mode_set_nofb(struct
> mxsfb_drm_private *mxsfb,
>
> if (mxsfb->bridge && mxsfb->bridge->timings)
> bus_flags = mxsfb->bridge->timings->input_bus_flags;
> + else if (bridge_state)
> + bus_flags = bridge_state->input_bus_cfg.flags;
>
> DRM_DEV_DEBUG_DRIVER(drm->dev, "Pixel clock: %dkHz (actual: %dkHz)
\n",
>m->crtc_clock,
> @@ -365,7 +368,7 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc
> *crtc, {
> struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(crtc->dev);
> struct drm_display_mode *m = &mxsfb->crtc.state->adjusted_mode;
> - struct drm_bridge_state *bridge_state;
> + struct drm_bridge_state *bridge_state = NULL;
> struct drm_device *drm = mxsfb->drm;
> u32 bus_format = 0;
>
> @@ -399,7 +402,7 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc
> *crtc,
>
> pm_runtime_get_sync(drm->dev);
>
> - mxsfb_crtc_mode_set_nofb(mxsfb, bus_format);
> + mxsfb_crtc_mode_set_nofb(mxsfb, bridge_state, bus_format);
>
> /* Write cur_buf as well to avoid an initial corrupt frame */
> mxsfb_update_buffer(mxsfb, crtc->primary, true);
I only have boards with an lvds-encoder bridge connected, which provides the
timings, so I can't test it. Otherwise LGTM.
Acked-by: Alexander Stein
Re: (EXT) [PATCH 00/11] drm: bridge: Add Samsung MIPI DSIM bridge
Hi Jagan,
thanks for picking this up again and sending a new version.
Am Freitag, 8. April 2022, 18:20:57 CEST schrieb Jagan Teki:
> This series supports common bridge support for Samsung MIPI DSIM
> which is used in Exynos and i.MX8MM SoC's.
>
> Previous RFC can be available here [1].
>
> The final bridge supports both the Exynos and i.MX8MM DSI devices.
>
> On, summary this patch-set break the entire DSIM driver into
> - platform specific glue code for platform ops, component_ops.
> - common bridge driver which handle platform glue init and invoke.
>
> Patch : Samsung DSIM bridge
>
> Patch 0001: platform init flag via driver_data
>
> Patch 0002/9: bridge fixes, atomic API's
>
> Patch 0010: document fsl,imx8mm-mipi-dsim
>
> Patch 0011: add i.MX8MM DSIM support
>
> Tested in Engicam i.Core MX8M Mini SoM.
>
> Anyone interested, please have a look on this repo [2]
>
> [2] https://github.com/openedev/kernel/tree/imx8mm-dsi-v1
> [1]
> https://lore.kernel.org/linux-arm-kernel/yp2j9k5srz2%2fo2%...@ravnborg.org/
> T/
>
> Any inputs?
With the following patch I can use LVDS, connected via an LVDS bridge on my
TQMa8MxML + MBa8Mx. Unless I enable 4 MIPI-DSI lanes.
Using "data-lanes = <1 2 3 4>;" instead show a flickering image, but the
"content" seems ok. On the downstream kernel MIPI-DSI is working, apparently
using 4-lanes. On the first glance a bigger difference to the downstream
kernel from NXP is that AFAICS they change the clocks depending on the
currently selected mode [1]. I tried playing with the clocks but I don't fully
grasp which clock has which effect, so I eventually had no results.
Any ideas what might be wrong here?
On a side note, might be completely unrelated to this series, I get the
following warning as well:
> sn65dsi83 2-002d: Unsupported LVDS bus format 0x100a, please check output
bridge driver. Falling back to SPWG24.
0x100a is MEDIA_BUS_FMT_RGB888_1X24 from
samsung_dsim_atomic_get_input_bus_fmts(). For some reason this is propagates
to the output_bus_cfg used in sn65dsi83_atomic_enable(). I would have expected
this is MEDIA_BUS_FMT_RGB888_1X7X4_SPWG from "tianma,tm070jvhg33" display.
Best regards,
Alexander
[1] https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/gpu/drm/
bridge/sec-dsim.c?h=lf-5.10.72-2.2.0#n1255
Here is my patch for the DT
--->8---
diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/
freescale/Makefile
index 52ce0f798657..7dd280b45681 100644
--- a/arch/arm64/boot/dts/freescale/Makefile
+++ b/arch/arm64/boot/dts/freescale/Makefile
@@ -58,6 +58,11 @@ dtb-$(CONFIG_ARCH_MXC) += imx8mm-icore-mx8mm-edimm2.2.dtb
dtb-$(CONFIG_ARCH_MXC) += imx8mm-kontron-n801x-s.dtb
dtb-$(CONFIG_ARCH_MXC) += imx8mm-nitrogen-r2.dtb
dtb-$(CONFIG_ARCH_MXC) += imx8mm-tqma8mqml-mba8mx.dtb
+
+tqma8mqml-mba8mx-imx327-dtbs += imx8mm-tqma8mqml-mba8mx.dtb imx8mm-tqma8mqml-
mba8mx-imx327.dtbo
+tqma8mqml-mba8mx-lvds-dtbs += imx8mm-tqma8mqml-mba8mx.dtb imx8mm-tqma8mqml-
mba8mx-lvds.dtbo
+dtb-$(CONFIG_ARCH_MXC) += tqma8mqml-mba8mx-imx327.dtb tqma8mqml-mba8mx-
lvds.dtb
+
dtb-$(CONFIG_ARCH_MXC) += imx8mm-var-som-symphony.dtb
dtb-$(CONFIG_ARCH_MXC) += imx8mm-venice-gw71xx-0x.dtb
dtb-$(CONFIG_ARCH_MXC) += imx8mm-venice-gw72xx-0x.dtb
diff --git a/arch/arm64/boot/dts/freescale/imx8mm-tqma8mqml-mba8mx-lvds.dts b/
arch/arm64/boot/dts/freescale/imx8mm-tqma8mqml-mba8mx-lvds.dts
new file mode 100644
index ..8c743d291459
--- /dev/null
+++ b/arch/arm64/boot/dts/freescale/imx8mm-tqma8mqml-mba8mx-lvds.dts
@@ -0,0 +1,63 @@
+// SPDX-License-Identifier: (GPL-2.0-or-later OR MIT)
+/*
+ * Copyright 2021-2022 TQ-Systems GmbH
+ */
+
+/dts-v1/;
+/plugin/;
+
+&{/} {
+ compatible = "tq,imx8mm-tqma8mqml-mba8mx", "tq,imx8mm-tqma8mqml",
"fsl,imx8mm";
+};
+
+&backlight_lvds0 {
+ status = "okay";
+};
+
+&dsi {
+ status = "okay";
+
+ ports {
+ port@1 {
+ mipi_dsi_out: endpoint {
+ remote-endpoint =
<&lvds_bridge_in>;
+ };
+ };
+ };
+};
+
+&dsi_lvds_bridge {
+ status = "okay";
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+
+ lvds_bridge_in: endpoint {
+ data-lanes = <1 2 3>;
+ remote-endpoint = <&mipi_dsi_out>;
+ };
+ };
+ };
+};
+
+&expander0 {
+ dsi-mux-oe-hog {
+ gpio-hog;
+ gpios = <10 0>;
+ output-low;
+ line-name = "DSI_MUX_OE#";
+ };
+};
+
+&lcdif {
+ status = "okay";
+};
+
+&panel0 {
+ compatible = "tianma,tm070jvhg33";
+ status = "okay";
+};
diff --git a/arch/arm64/boot/dts/freescale/mba8mx.dtsi b/arch/arm64/boot/dts/
freescale/mba8mx.dtsi
index c2f0f1a1566c..4b2cca3268eb 100644
--- a/arch/
Re: [PATCH v0 00/10] i.MX8MP HDMI support
Hello Lucas, Am Mittwoch, 6. April 2022, 18:01:13 CEST schrieb Lucas Stach: > Hi all, > > this adds support for the HDMI output pipeline on the i.MX8MP. > It currently depends on the i.MX8MP HDMI power domain series [1] > and support for the new LCDIF [2] in the i.MX8MP. I guess the > implementation presented here also still has some warts that > require fixing and the individual patches most likely need to go > through different maintainer trees, so I don't expect this series > to be applied right away. > > However this complete series should allow people to test it more > easily and provide feedback on the implementation with the full > picture available. > > Compared to downstream this implementation actually allows to > power down the separate HDMI PHY power domain when the display > is inactive or no HDMI cable is connected. Thanks for these patches. I tried using them on my imx8mp based board (TQMa8MPxL + MBa8MPxL) but failed to get the display showing anything. I noticed several things though: * For some reason the HDMI PHY PLL does not lock. I get the error > fsl-samsung-hdmi-phy 32fdff00.phy: PLL failed to lock Increasing timeout does not change anything. * The HDMI bridge wants to use bus format 0x200f which is not supported by lcdif. > lcdif 32fc6000.display-controller: Unknown media bus format 0x200f I wonder which part in the DRM chain choses to use this. But even hard limiting to 0x100a the screen stayed in suspend * If the drivers are built as modules I get a hard lockup during boot. Using built-in drivers or 'clk_ignore_unused' workarounds this. * DDC does actually work. The display is detected and EDID can be read. * Sometimes I get the following error: [ cut here ] [CRTC:33:crtc-0] vblank wait timed out WARNING: CPU: 2 PID: 151 at drivers/gpu/drm/drm_atomic_helper.c:1529 drm_atomic_helper_wait_for_vblanks.part.0+0x2ac/0x2fc Modules linked in: caamhash_desc caamalg_desc crypto_engine rng_core mcp320x dw_hdmi_cec authenc libdes dw100 videobuf2_dma_contig lcdif crct10dif_ce phy_fsl_samsung_hdmi v4l2_mem2mem imx_sdma flexcan imx8mm_thermal can_dev caam error pwm_fan fuse ipv6 CPU: 2 PID: 151 Comm: kworker/u8:7 Not tainted 5.18.0-rc2-next-20220412+ #165 d226098cac46ded24901c7090f909ca8f5098eb0 Hardware name: TQ-Systems i.MX8MPlus TQMa8MPxL on MBa8MPxL (DT) Workqueue: events_unbound deferred_probe_work_func pstate: 6005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) pc : drm_atomic_helper_wait_for_vblanks.part.0+0x2ac/0x2fc lr : drm_atomic_helper_wait_for_vblanks.part.0+0x2ac/0x2fc sp : 8a133430 x29: 8a133430 x28: x27: x26: x25: 0001 x24: 8935f030 x23: 0433e000 x22: 029e7000 x21: 0001 x20: 02e7fb48 x19: x18: 0001 x17: 4d554e5145530065 x16: 6c75646f6d3d4d45 x15: 5453595342555300 x14: x13: 0a74756f2064656d x12: 6974207469617720 x11: x10: 003a x9 : 8a133430 x8 : x7 : 6974207469617720 x6 : 6b6e616c6276205d x5 : 7fb91b00 x4 : x3 : 0027 x2 : 0023 x1 : x0 : Call trace: drm_atomic_helper_wait_for_vblanks.part.0+0x2ac/0x2fc drm_atomic_helper_commit_tail_rpm+0x80/0xa0 commit_tail+0xcc/0x1f0 drm_atomic_helper_commit+0x13c/0x370 drm_atomic_commit+0xa4/0xe0 drm_client_modeset_commit_atomic+0x1fc/0x250 drm_client_modeset_commit_locked+0x58/0xa0 drm_client_modeset_commit+0x2c/0x50 __drm_fb_helper_restore_fbdev_mode_unlocked+0xec/0x140 drm_fb_helper_set_par+0x38/0x6c fbcon_init+0x264/0x5e4 visual_init+0xc8/0x15c do_bind_con_driver.isra.0+0x20c/0x470 do_take_over_console+0x44/0x60 do_fbcon_takeover+0x80/0x140 fbcon_fb_registered+0x1c4/0x260 do_register_framebuffer+0x1e0/0x2d0 register_framebuffer+0x2c/0x50 __drm_fb_helper_initial_config_and_unlock+0x9c/0x130 drm_fbdev_client_hotplug+0x1a8/0x20c drm_fbdev_generic_setup+0xc0/0x1d0 lcdif_probe+0x7c/0xa0 [lcdif e756925430e957a7bc9e6376ad5964e4b1cb143e] platform_probe+0x64/0x100 call_driver_probe+0x28/0x130 really_probe+0x178/0x310 __driver_probe_device+0xfc/0x144 driver_probe_device+0x38/0x12c __device_attach_driver+0xd4/0x180 bus_for_each_drv+0x74/0xc4 __device_attach+0xd8/0x1e0 device_initial_probe+0x10/0x20 bus_probe_device+0x90/0xa0 deferred_probe_work_func+0x9c/0xf0 process_one_work+0x1d0/0x330 worker_thread+0x68/0x390 kthread+0xec/0xfc ret_from_fork+0x10/0x20 ---[ end trace ]--- But given that the PLL did not lock I assume this is not too surprising. Best regards, Alexander
Re: [PATCH v3 00/13] drm: bridge: Add Samsung MIPI DSIM bridge
Hi Jagan, thanks for the update. Am Mittwoch, 20. Juli 2022, 17:51:57 CEST schrieb Jagan Teki: > This series supports common bridge support for Samsung MIPI DSIM > which is used in Exynos and i.MX8MM SoC's. > > Previous v2 can be available here [1]. > > The final bridge supports both the Exynos and i.MX8MM DSI devices. > > On, summary this patch-set break the entire DSIM driver into > - platform specific glue code for platform ops, component_ops. > - common bridge driver which handle platform glue init and invoke. > > Patch 0001: Restore proper bridge chain in exynos_dsi > > Patch 0002: Samsung DSIM bridge > > Patch 0003: Common lookup code for OF-graph or child > > Patch 0004: plat_data quirk flag via driver_data > > Patch 0005/11: bridge fixes, atomic API's > > Patch 0012: document fsl,imx8mm-mipi-dsim > > Patch 0013: add i.MX8MM DSIM support > > Tested in Engicam i.Core MX8M Mini SoM. > > Anyone interested, please have a look on this repo [2] > > [2] https://github.com/openedev/kernel/tree/imx8mm-dsi-v2 I suspect you meant https://github.com/openedev/kernel/tree/imx8mm-dsi-v3 Using this v3: Tested-by: Alexander Stein on TQMa8MxML + MBa8Mx. > [1] > https://patchwork.kernel.org/project/dri-devel/cover/20220504114021.33265-1 > -ja...@amarulasolutions.com/ > > Any inputs? > Jagan. > > Jagan Teki (12): > drm: bridge: Add Samsung DSIM bridge driver > drm: bridge: samsung-dsim: Lookup OF-graph or Child node devices > drm: bridge: samsung-dsim: Handle platform init via driver_data > drm: bridge: samsung-dsim: Mark PHY as optional > drm: bridge: samsung-dsim: Add DSI init in bridge pre_enable() > drm: bridge: samsung-dsim: Fix PLL_P (PMS_P) offset > drm: bridge: samsung-dsim: Add module init, exit > drm: bridge: samsung-dsim: Add atomic_check > drm: bridge: samsung-dsim: Add atomic_get_input_bus_fmts > drm: bridge: samsung-dsim: Add input_bus_flags > dt-bindings: display: exynos: dsim: Add NXP i.MX8MM support > drm: bridge: samsung-dsim: Add i.MX8MM support > > Marek Szyprowski (1): > drm: exynos: dsi: Restore proper bridge chain order > > .../bindings/display/exynos/exynos_dsim.txt |1 + > MAINTAINERS |8 + > drivers/gpu/drm/bridge/Kconfig| 12 + > drivers/gpu/drm/bridge/Makefile |1 + > drivers/gpu/drm/bridge/samsung-dsim.c | 1850 + > drivers/gpu/drm/exynos/Kconfig|1 + > drivers/gpu/drm/exynos/exynos_drm_dsi.c | 1717 +-- > include/drm/bridge/samsung-dsim.h | 106 + > 8 files changed, 2042 insertions(+), 1654 deletions(-) > create mode 100644 drivers/gpu/drm/bridge/samsung-dsim.c > create mode 100644 include/drm/bridge/samsung-dsim.h
[PATCH 1/1] drm: panel-simple: add missing bus flags for Tianma tm070jvhg[30/33]
From: Markus Niebel
The DE signal is active high on this display, fill in the missing
bus_flags. This aligns panel_desc with its display_timing.
Fixes: 9a2654c0f62a ("drm/panel: Add and fill drm_panel type field")
Fixes: b3bfcdf8a3b6 ("drm/panel: simple: add Tianma TM070JVHG33")
Signed-off-by: Markus Niebel
Signed-off-by: Alexander Stein
---
drivers/gpu/drm/panel/panel-simple.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/panel/panel-simple.c
b/drivers/gpu/drm/panel/panel-simple.c
index 065f378bba9d..fbccaf1cb6f2 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -3598,6 +3598,7 @@ static const struct panel_desc tianma_tm070jdhg30 = {
},
.bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
.connector_type = DRM_MODE_CONNECTOR_LVDS,
+ .bus_flags = DRM_BUS_FLAG_DE_HIGH,
};
static const struct panel_desc tianma_tm070jvhg33 = {
@@ -3610,6 +3611,7 @@ static const struct panel_desc tianma_tm070jvhg33 = {
},
.bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
.connector_type = DRM_MODE_CONNECTOR_LVDS,
+ .bus_flags = DRM_BUS_FLAG_DE_HIGH,
};
static const struct display_timing tianma_tm070rvhg71_timing = {
--
2.34.1
Re: [PATCH v12 00/18] drm: Add Samsung MIPI DSIM bridge
Hi Rasmus, Am Montag, 30. Januar 2023, 13:45:38 CET schrieb Rasmus Villemoes: > On 27/01/2023 12.30, Marek Vasut wrote: > > On 1/27/23 12:04, Jagan Teki wrote: > >>> Thanks, but that's exactly what I'm doing, and I don't see any > >>> modification of imx8mp.dtsi in that branch. I'm basically looking for > >>> help to do the equivalent of > >>> > >>>88775338cd58 - arm64: dts: imx8mm: Add MIPI DSI pipeline > >>>f964f67dd6ee - arm64: dts: imx8mm: Add eLCDIF node support > >>> > >>> for imx8mp in order to test those patches on our boards (we have two > >>> variants). > >> > >> Marek, any help here, thanks. > > > > Try attached patch. > > Thanks. I removed the lcdif2 and ldb nodes I had added from Alexander's > patch (94e6197dadc9 in linux-next) in order to apply it. I get a couple > of errors during boot: > > clk: /soc@0/bus@32c0/mipi_dsi@32e6: failed to reparent > media_apb to sys_pll1_266m: -22 > > and enabling a pr_debug in clk_core_set_parent_nolock() shows that this > is because > > clk_core_set_parent_nolock: clk sys_pll1_266m can not be parent of clk > media_apb > > Further, the mipi_dsi fails to probe due to > > /soc@0/bus@32c0/mipi_dsi@32e6: failed to get > 'samsung,burst-clock-frequency' property > > All other .dtsi files seem to have those samsung,burst-clock-frequency > and samsung,esc-clock-frequency properties, so I suppose those should > also go into the imx8mp.dtsi and are not something that the board .dts > file should supply(?). > > > [There's also some differences between your patch and Alexander's > regarding the lcdif2 and ldb nodes, so while my lvds display still sorta > works, I get > > fsl-ldb 32ec.blk-ctrl:lvds-ldb: Configured LDB clock (29700 > Hz) does not match requested LVDS clock: 34650 Hz > > and the image is oddly distorted/shifted. But I suppose that's > orthogonal to getting the lcdif1 -> mipi-dsi -> ... pipeline working.] It seems my patch indicates exactly what's your problem here. Your clock rate configuration for LCDIF2 and LDB are not compatible (ratio 1:7). For now manual clock config is necessary in this case. Check the clock tree in debugfs how your root clocks are configured and if it affects lcdif1 and lcdif2. Best regards Alexander
Re: [PATCH 2/2] drm: fsl-dcu: enable PIXCLK on LS1021A
Hi,
any feedback on this?
Best regards
Alexander
Am Dienstag, 17. Januar 2023, 12:08:01 CET schrieb Alexander Stein:
> From: Matthias Schiffer
>
> The PIXCLK needs to be enabled in SCFG before accessing certain DCU
> registers, or the access will hang.
>
> Signed-off-by: Matthias Schiffer
> Signed-off-by: Alexander Stein
> ---
> drivers/gpu/drm/fsl-dcu/Kconfig | 1 +
> drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 14 ++
> drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h | 3 +++
> 3 files changed, 18 insertions(+)
>
> diff --git a/drivers/gpu/drm/fsl-dcu/Kconfig
> b/drivers/gpu/drm/fsl-dcu/Kconfig index 5ca71ef87325..c9ee98693b48 100644
> --- a/drivers/gpu/drm/fsl-dcu/Kconfig
> +++ b/drivers/gpu/drm/fsl-dcu/Kconfig
> @@ -8,6 +8,7 @@ config DRM_FSL_DCU
> select DRM_PANEL
> select REGMAP_MMIO
> select VIDEOMODE_HELPERS
> + select MFD_SYSCON if SOC_LS1021A
> help
> Choose this option if you have an Freescale DCU chipset.
> If M is selected the module will be called fsl-dcu-drm.
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c index
> 418887654bac..314cb8af89ee 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> @@ -100,12 +100,26 @@ static void fsl_dcu_irq_uninstall(struct drm_device
> *dev) static int fsl_dcu_load(struct drm_device *dev, unsigned long flags)
> {
> struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
> + struct regmap *scfg;
> int ret;
>
> ret = fsl_dcu_drm_modeset_init(fsl_dev);
> if (ret < 0)
> return dev_err_probe(dev->dev, ret, "failed to initialize
mode
> setting\n");
>
> + scfg = syscon_regmap_lookup_by_compatible("fsl,ls1021a-scfg");
> + if (PTR_ERR(scfg) != -ENODEV) {
> + /*
> + * For simplicity, enable the PIXCLK unconditionally.
Disabling
> + * the clock in PM or on unload could be implemented as a
future
> + * improvement.
> + */
> + ret = regmap_update_bits(scfg, SCFG_PIXCLKCR,
SCFG_PIXCLKCR_PXCEN,
> + SCFG_PIXCLKCR_PXCEN);
> + if (ret < 0)
> + return dev_err_probe(dev->dev, ret, "failed to
enable pixclk\n");
> + }
> +
> ret = drm_vblank_init(dev, dev->mode_config.num_crtc);
> if (ret < 0) {
> dev_err(dev->dev, "failed to initialize vblank\n");
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h index
> e2049a0e8a92..566396013c04 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
> @@ -160,6 +160,9 @@
> #define FSL_DCU_ARGB 12
> #define FSL_DCU_YUV422 14
>
> +#define SCFG_PIXCLKCR0x28
> +#define SCFG_PIXCLKCR_PXCEN BIT(31)
> +
> #define VF610_LAYER_REG_NUM 9
> #define LS1021A_LAYER_REG_NUM10
Re: [PATCH 1/2] drm: fsl-dcu: Use dev_err_probe
Hi,
any feedback on this series?
Best regards,
Alexander
Am Dienstag, 17. Januar 2023, 12:08:00 CET schrieb Alexander Stein:
> fsl_dcu_drm_modeset_init can return -EPROBE_DEFER, so use dev_err_probe
> to remove an invalid error message and add it to deferral description.
>
> Signed-off-by: Alexander Stein
> ---
> drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c index
> 8579c7629f5e..418887654bac 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> @@ -103,10 +103,8 @@ static int fsl_dcu_load(struct drm_device *dev,
> unsigned long flags) int ret;
>
> ret = fsl_dcu_drm_modeset_init(fsl_dev);
> - if (ret < 0) {
> - dev_err(dev->dev, "failed to initialize mode setting\n");
> - return ret;
> - }
> + if (ret < 0)
> + return dev_err_probe(dev->dev, ret, "failed to initialize
mode
> setting\n");
>
> ret = drm_vblank_init(dev, dev->mode_config.num_crtc);
> if (ret < 0) {
--
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/
Re: [PATCH v3 2/6] drm: lcdif: Drop unnecessary NULL pointer check on lcdif->bridge
Hi Liu,
thanks for the update.
Am Montag, 13. Februar 2023, 09:56:08 CET schrieb Liu Ying:
> A valid bridge is already found in lcdif_attach_bridge() and set
> to lcdif->bridge, so lcdif->bridge cannot be a NULL pointer. Drop
> the unnecessary NULL pointer check in KMS stage.
>
> Signed-off-by: Liu Ying
> ---
> v2->v3:
> * No change.
>
> v1->v2:
> * Split from patch 2/2 in v1. (Marek, Alexander)
>
> drivers/gpu/drm/mxsfb/lcdif_kms.c | 33 +++
> 1 file changed, 12 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> b/drivers/gpu/drm/mxsfb/lcdif_kms.c index 262bc43b1079..e54200a9fcb9 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> @@ -394,7 +394,7 @@ static void lcdif_crtc_mode_set_nofb(struct
> lcdif_drm_private *lcdif, struct drm_display_mode *m =
> &lcdif->crtc.state->adjusted_mode;
> u32 bus_flags = 0;
>
> - if (lcdif->bridge && lcdif->bridge->timings)
> + if (lcdif->bridge->timings)
> bus_flags = lcdif->bridge->timings->input_bus_flags;
> else if (bridge_state)
> bus_flags = bridge_state->input_bus_cfg.flags;
> @@ -463,30 +463,21 @@ static void lcdif_crtc_atomic_enable(struct drm_crtc
> *crtc, struct drm_display_mode *m = &lcdif->crtc.state->adjusted_mode;
> struct drm_bridge_state *bridge_state = NULL;
> struct drm_device *drm = lcdif->drm;
> - u32 bus_format = 0;
> + u32 bus_format;
> dma_addr_t paddr;
>
> - /* If there is a bridge attached to the LCDIF, use its bus format */
> - if (lcdif->bridge) {
> - bridge_state =
> - drm_atomic_get_new_bridge_state(state,
> - lcdif-
>bridge);
> - if (!bridge_state)
> - bus_format = MEDIA_BUS_FMT_FIXED;
> - else
> - bus_format = bridge_state->input_bus_cfg.format;
> -
> - if (bus_format == MEDIA_BUS_FMT_FIXED) {
> - dev_warn_once(drm->dev,
> - "Bridge does not provide bus
format, assuming
> MEDIA_BUS_FMT_RGB888_1X24.\n" -
"Please fix bridge driver by
> handling atomic_get_input_bus_fmts.\n"); -
bus_format =
> MEDIA_BUS_FMT_RGB888_1X24;
> - }
> - }
> + bridge_state = drm_atomic_get_new_bridge_state(state, lcdif-
>bridge);
> + if (!bridge_state)
> + bus_format = MEDIA_BUS_FMT_FIXED;
> + else
> + bus_format = bridge_state->input_bus_cfg.format;
>
> - /* If all else fails, default to RGB888_1X24 */
> - if (!bus_format)
> + if (bus_format == MEDIA_BUS_FMT_FIXED) {
> + dev_warn_once(drm->dev,
> + "Bridge does not provide bus format,
assuming
> MEDIA_BUS_FMT_RGB888_1X24.\n" + "Please fix
bridge driver by
> handling atomic_get_input_bus_fmts.\n"); bus_format =
> MEDIA_BUS_FMT_RGB888_1X24;
> + }
>
> clk_set_rate(lcdif->clk, m->crtc_clock * 1000);
LGTM.
Reviewed-by: Alexander Stein
--
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/
Re: [PATCH v3 3/6] drm: lcdif: Determine bus format and flags in ->atomic_check()
Hi Liu,
thanks for the update.
Am Montag, 13. Februar 2023, 09:56:09 CET schrieb Liu Ying:
> Instead of determining LCDIF output bus format and bus flags in
> ->atomic_enable(), do that in ->atomic_check(). This is a
> preparation for the upcoming patch to check consistent bus format
> and bus flags across all first downstream bridges in ->atomic_check().
> New lcdif_crtc_state structure is introduced to cache bus format
> and bus flags states in ->atomic_check() so that they can be read
> in ->atomic_enable().
>
> Signed-off-by: Liu Ying
> ---
> v2->v3:
> * No change.
>
> v1->v2:
> * Split from patch 2/2 in v1. (Marek, Alexander)
> * Add comment on the 'base' member of lcdif_crtc_state structure to
> note it should always be the first member. (Lothar)
>
> drivers/gpu/drm/mxsfb/lcdif_kms.c | 138 ++
> 1 file changed, 100 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> b/drivers/gpu/drm/mxsfb/lcdif_kms.c index e54200a9fcb9..294cecdf5439 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> @@ -30,6 +30,18 @@
> #include "lcdif_drv.h"
> #include "lcdif_regs.h"
>
> +struct lcdif_crtc_state {
> + struct drm_crtc_state base; /* always be the first
member */
Is it strictly necessary that base is the first member? to_lcdif_crtc_state()
should be able to handle any position within the struct. I mean it's sensible
to put it in first place. But the comment somehow bugs me.
> + u32 bus_format;
> + u32 bus_flags;
> +};
> +
> +static inline struct lcdif_crtc_state *
> +to_lcdif_crtc_state(struct drm_crtc_state *s)
> +{
> + return container_of(s, struct lcdif_crtc_state, base);
> +}
> +
> /*
> ---
> -- * CRTC
> */
> @@ -385,48 +397,72 @@ static void lcdif_reset_block(struct lcdif_drm_private
> *lcdif) readl(lcdif->base + LCDC_V8_CTRL);
> }
>
> -static void lcdif_crtc_mode_set_nofb(struct lcdif_drm_private *lcdif,
> - struct drm_plane_state
*plane_state,
> - struct drm_bridge_state
*bridge_state,
> - const u32 bus_format)
> +static void lcdif_crtc_mode_set_nofb(struct drm_crtc_state *crtc_state,
> + struct drm_plane_state
*plane_state)
> {
> - struct drm_device *drm = lcdif->crtc.dev;
> - struct drm_display_mode *m = &lcdif->crtc.state->adjusted_mode;
> - u32 bus_flags = 0;
> -
> - if (lcdif->bridge->timings)
> - bus_flags = lcdif->bridge->timings->input_bus_flags;
> - else if (bridge_state)
> - bus_flags = bridge_state->input_bus_cfg.flags;
> + struct lcdif_crtc_state *lcdif_crtc_state =
> to_lcdif_crtc_state(crtc_state); +struct drm_device *drm =
> crtc_state->crtc->dev;
> + struct lcdif_drm_private *lcdif = to_lcdif_drm_private(drm);
> + struct drm_display_mode *m = &crtc_state->adjusted_mode;
>
> DRM_DEV_DEBUG_DRIVER(drm->dev, "Pixel clock: %dkHz (actual: %dkHz)
\n",
>m->crtc_clock,
>(int)(clk_get_rate(lcdif->clk) / 1000));
> DRM_DEV_DEBUG_DRIVER(drm->dev, "Bridge bus_flags: 0x%08X\n",
> - bus_flags);
> + lcdif_crtc_state->bus_flags);
> DRM_DEV_DEBUG_DRIVER(drm->dev, "Mode flags: 0x%08X\n", m->flags);
>
> /* Mandatory eLCDIF reset as per the Reference Manual */
> lcdif_reset_block(lcdif);
>
> - lcdif_set_formats(lcdif, plane_state, bus_format);
> + lcdif_set_formats(lcdif, plane_state, lcdif_crtc_state->bus_format);
>
> - lcdif_set_mode(lcdif, bus_flags);
> + lcdif_set_mode(lcdif, lcdif_crtc_state->bus_flags);
> }
>
> static int lcdif_crtc_atomic_check(struct drm_crtc *crtc,
> struct drm_atomic_state *state)
> {
> + struct drm_device *drm = crtc->dev;
> + struct lcdif_drm_private *lcdif = to_lcdif_drm_private(drm);
> struct drm_crtc_state *crtc_state =
drm_atomic_get_new_crtc_state(state,
>
crtc);
> + struct lcdif_crtc_state *lcdif_crtc_state =
> to_lcdif_crtc_state(crtc_state); bool has_primary = crtc_state->plane_mask
> &
> drm_plane_mask(crtc->primary);
> + struct drm_bridge_state *bridge_state;
> + struct drm_bridge *bridge = lcdif->bridge;
> + int ret;
>
> /* The primary plane has to be enabled when the CRTC is active. */
> if (crtc_state->active && !has_primary)
> return -EINVAL;
>
> - return drm_atomic_add_affected_planes(state, crtc);
> + ret = drm_atomic_add_affected_planes(state, crtc);
> + if (ret)
> + return ret;
> +
> + bridge_state = drm_atomic_get_new_bridge_state(state, bridge);
> +
Re: [PATCH v3 1/6] dt-bindings: lcdif: Add i.MX93 LCDIF support
Hi Liu, thanks for the update. Am Montag, 13. Februar 2023, 09:56:07 CET schrieb Liu Ying: > There is one LCDIF embedded in i.MX93 SoC to connect with > MIPI DSI controller through LCDIF cross line pattern(controlled > by mediamix blk-ctrl) or connect with LVDS display bridge(LDB) > directly or connect with a parallel display through parallel > display format(also controlled by mediamix blk-ctrl). i.MX93 > LCDIF IP is essentially the same to i.MX8MP LCDIF IP. Add device > tree binding for i.MX93 LCDIF. > > Acked-by: Krzysztof Kozlowski > Reviewed-by: Marek Vasut > Signed-off-by: Liu Ying > --- > v2->v3: > * No change. > > v1->v2: > * Add Krzysztof's A-b and Marek's R-b tags on patch 1/6. > > Documentation/devicetree/bindings/display/fsl,lcdif.yaml | 7 ++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml > b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml index > 75b4efd70ba8..fc11ab5fc465 100644 > --- a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml > +++ b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml > @@ -21,6 +21,7 @@ properties: >- fsl,imx28-lcdif >- fsl,imx6sx-lcdif >- fsl,imx8mp-lcdif > + - fsl,imx93-lcdif >- items: >- enum: >- fsl,imx6sl-lcdif > @@ -88,7 +89,9 @@ allOf: >properties: > compatible: >contains: > -const: fsl,imx8mp-lcdif > +enum: > + - fsl,imx8mp-lcdif > + - fsl,imx93-lcdif > then: >properties: > clocks: > @@ -107,6 +110,7 @@ allOf: >enum: > - fsl,imx6sx-lcdif > - fsl,imx8mp-lcdif > +- fsl,imx93-lcdif > then: >properties: > clocks: > @@ -123,6 +127,7 @@ allOf: >- fsl,imx8mm-lcdif >- fsl,imx8mn-lcdif >- fsl,imx8mp-lcdif > + - fsl,imx93-lcdif > then: >required: > - power-domains I would have expected that fsl,imx93-lcdif supports up to 3 endpoints (MIPI DSI, LVDS, and parallel) in a 'ports' subnode. But this binding only supports a single 'port' sub-node. Also an example for this case might be very helpful. Best regards, Alexander -- TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany Amtsgericht München, HRB 105018 Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider http://www.tq-group.com/
Re: [PATCH v3 5/6] drm: lcdif: Add multiple encoders and first bridges support
Hi Liu,
thanks for the update.
Am Montag, 13. Februar 2023, 09:56:11 CET schrieb Liu Ying:
> The single LCDIF embedded in i.MX93 SoC may drive multiple displays
> simultaneously. Look at LCDIF output port's remote port parents to
> find all enabled first bridges. Add an encoder for each found bridge
> and attach the bridge to the encoder. This is a preparation for
> adding i.MX93 LCDIF support.
>
> Signed-off-by: Liu Ying
> ---
> v2->v3:
> * No change.
>
> v1->v2:
> * Split from patch 2/2 in v1. (Marek, Alexander)
> * Drop '!remote ||' from lcdif_attach_bridge(). (Lothar)
> * Drop unneeded 'bridges' member from lcdif_drm_private structure.
>
> drivers/gpu/drm/mxsfb/lcdif_drv.c | 68 +++
> drivers/gpu/drm/mxsfb/lcdif_drv.h | 4 +-
> drivers/gpu/drm/mxsfb/lcdif_kms.c | 21 ++
> 3 files changed, 66 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> b/drivers/gpu/drm/mxsfb/lcdif_drv.c index b5b9a8e273c6..eb6c265fa2fe 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> @@ -9,13 +9,16 @@
> #include
> #include
> #include
> +#include
> #include
> +#include
> #include
> #include
>
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -38,19 +41,68 @@ static const struct drm_mode_config_helper_funcs
> lcdif_mode_config_helpers = { .atomic_commit_tail =
> drm_atomic_helper_commit_tail_rpm,
> };
>
> +static const struct drm_encoder_funcs lcdif_encoder_funcs = {
> + .destroy = drm_encoder_cleanup,
> +};
> +
> static int lcdif_attach_bridge(struct lcdif_drm_private *lcdif)
> {
> - struct drm_device *drm = lcdif->drm;
> + struct device *dev = lcdif->drm->dev;
> + struct device_node *ep;
> struct drm_bridge *bridge;
> int ret;
>
> - bridge = devm_drm_of_get_bridge(drm->dev, drm->dev->of_node, 0, 0);
> - if (IS_ERR(bridge))
> - return PTR_ERR(bridge);
> -
> - ret = drm_bridge_attach(&lcdif->encoder, bridge, NULL, 0);
> - if (ret)
> - return dev_err_probe(drm->dev, ret, "Failed to attach
bridge\n");
> + for_each_endpoint_of_node(dev->of_node, ep) {
> + struct device_node *remote;
> + struct of_endpoint of_ep;
> + struct drm_encoder *encoder;
> +
> + remote = of_graph_get_remote_port_parent(ep);
Is it possible for remote to be NULL?
> + if (!of_device_is_available(remote)) {
> + of_node_put(remote);
> + continue;
> + }
> + of_node_put(remote);
> +
> + ret = of_graph_parse_endpoint(ep, &of_ep);
> + if (ret < 0) {
> + dev_err(dev, "Failed to parse endpoint %pOF\n",
ep);
> + of_node_put(ep);
> + return ret;
> + }
> +
> + if (of_ep.id >= MAX_DISPLAYS) {
> + dev_warn(dev, "invalid endpoint id %u\n",
of_ep.id);
I would write
dev_warn(dev, "ignoring invalid endpoint id %u\n", of_ep.id);
just because the parsing continues but this one is skipped.
> + continue;
> + }
> +
> + bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0,
of_ep.id);
> + if (IS_ERR(bridge)) {
> + of_node_put(ep);
> + return dev_err_probe(dev, PTR_ERR(bridge),
> + "Failed to get bridge
for endpoint%u\n",
> + of_ep.id);
> + }
> +
> + encoder = &lcdif->encoders[of_ep.id];
> + encoder->possible_crtcs = drm_crtc_mask(&lcdif->crtc);
> + ret = drm_encoder_init(lcdif->drm, encoder,
&lcdif_encoder_funcs,
> +DRM_MODE_ENCODER_NONE, NULL);
> + if (ret) {
> + dev_err(dev, "Failed to initialize encoder for
endpoint%u: %d\n",
> + of_ep.id, ret);
> + of_node_put(ep);
> + return ret;
> + }
> +
> + ret = drm_bridge_attach(encoder, bridge, NULL, 0);
> + if (ret) {
> + of_node_put(ep);
> + return dev_err_probe(dev, ret,
> + "Failed to attach
bridge for endpoint%u\n",
> + of_ep.id);
> + }
Admittedly I'm not used to the drm API, but do we need to some manual cleanup/
revert if some endpoints is e.g. deferred, but previous endpoints already have
been successfully added? e.g. endpoint 0 is added, but adding endpoint 1
fails.
Best regards,
Alexander
> + }
>
> return 0;
> }
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.h
> b/drivers/gpu/drm/mxsfb/lcdif_drv.h index aa6d099a1897..c7400bd9bbd9 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_drv.h
Re: [PATCH v3 6/6] drm: lcdif: Add i.MX93 LCDIF compatible string
Hi Liu,
thanks for the update.
Am Montag, 13. Februar 2023, 09:56:12 CET schrieb Liu Ying:
> With all previous preparations done to make it possible for the
> single LCDIF embedded in i.MX93 SoC to drive multiple displays
> simultaneously, add i.MX93 LCDIF compatible string as the last
> step of adding i.MX93 LCDIF support.
>
> Signed-off-by: Liu Ying
> ---
> v2->v3:
> * Fix a trivial typo in commit message.
>
> v1->v2:
> * Split from patch 2/2 in v1. (Marek, Alexander)
>
> drivers/gpu/drm/mxsfb/lcdif_drv.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> b/drivers/gpu/drm/mxsfb/lcdif_drv.c index eb6c265fa2fe..48c43b273a4a 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> @@ -249,6 +249,7 @@ static const struct drm_driver lcdif_driver = {
>
> static const struct of_device_id lcdif_dt_ids[] = {
> { .compatible = "fsl,imx8mp-lcdif" },
> + { .compatible = "fsl,imx93-lcdif" },
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, lcdif_dt_ids);
Reviewed-by: Alexander Stein
--
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/
Re: [PATCH v3 4/6] drm: lcdif: Check consistent bus format and flags across first bridges
Hi Liu,
thanks for the update.
Am Montag, 13. Februar 2023, 09:56:10 CET schrieb Liu Ying:
> The single LCDIF embedded in i.MX93 SoC may drive multiple displays
> simultaneously. Check bus format and flags across first bridges in
> ->atomic_check() to ensure they are consistent. This is a preparation
> for adding i.MX93 LCDIF support.
>
> Signed-off-by: Liu Ying
> ---
> v2->v3:
> * No change.
>
> v1->v2:
> * Split from patch 2/2 in v1. (Marek, Alexander)
> * Drop a comment about bridge input bus format from
> lcdif_crtc_atomic_check().
>
> drivers/gpu/drm/mxsfb/lcdif_drv.c | 2 -
> drivers/gpu/drm/mxsfb/lcdif_drv.h | 1 -
> drivers/gpu/drm/mxsfb/lcdif_kms.c | 76 ++-
> 3 files changed, 55 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> b/drivers/gpu/drm/mxsfb/lcdif_drv.c index cc2ceb301b96..b5b9a8e273c6 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> @@ -52,8 +52,6 @@ static int lcdif_attach_bridge(struct lcdif_drm_private
> *lcdif) if (ret)
> return dev_err_probe(drm->dev, ret, "Failed to attach
bridge\n");
>
> - lcdif->bridge = bridge;
> -
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.h
> b/drivers/gpu/drm/mxsfb/lcdif_drv.h index 6cdba6e20c02..aa6d099a1897 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_drv.h
> +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.h
> @@ -31,7 +31,6 @@ struct lcdif_drm_private {
> } planes;
> struct drm_crtc crtc;
> struct drm_encoder encoder;
> - struct drm_bridge *bridge;
> };
>
> static inline struct lcdif_drm_private *
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> b/drivers/gpu/drm/mxsfb/lcdif_kms.c index 294cecdf5439..4ea3d2b2cf61 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> @@ -17,6 +17,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -424,15 +425,19 @@ static int lcdif_crtc_atomic_check(struct drm_crtc
> *crtc, struct drm_atomic_state *state)
> {
> struct drm_device *drm = crtc->dev;
> - struct lcdif_drm_private *lcdif = to_lcdif_drm_private(drm);
> struct drm_crtc_state *crtc_state =
drm_atomic_get_new_crtc_state(state,
>
crtc);
> struct lcdif_crtc_state *lcdif_crtc_state =
> to_lcdif_crtc_state(crtc_state); bool has_primary = crtc_state->plane_mask
> &
> drm_plane_mask(crtc->primary);
> + struct drm_connector_state *connector_state;
> + struct drm_connector *connector;
> + struct drm_encoder *encoder;
> struct drm_bridge_state *bridge_state;
> - struct drm_bridge *bridge = lcdif->bridge;
> - int ret;
> + struct drm_bridge *bridge;
> + u32 bus_format, bus_flags;
> + bool format_set = false, flags_set = false;
> + int ret, i;
>
> /* The primary plane has to be enabled when the CRTC is active. */
> if (crtc_state->active && !has_primary)
> @@ -442,26 +447,55 @@ static int lcdif_crtc_atomic_check(struct drm_crtc
> *crtc, if (ret)
> return ret;
>
> - bridge_state = drm_atomic_get_new_bridge_state(state, bridge);
> - if (!bridge_state)
> - lcdif_crtc_state->bus_format = MEDIA_BUS_FMT_FIXED;
> - else
> - lcdif_crtc_state->bus_format = bridge_state-
>input_bus_cfg.format;
> -
> - if (lcdif_crtc_state->bus_format == MEDIA_BUS_FMT_FIXED) {
> - dev_warn_once(drm->dev,
> - "Bridge does not provide bus format,
assuming
> MEDIA_BUS_FMT_RGB888_1X24.\n" - "Please fix
bridge driver by
> handling atomic_get_input_bus_fmts.\n"); -lcdif_crtc_state-
>bus_format =
> MEDIA_BUS_FMT_RGB888_1X24;
> + /* Try to find consistent bus format and flags across first bridges.
*/
> + for_each_new_connector_in_state(state, connector, connector_state,
i) {
> + if (!connector_state->crtc)
> + continue;
> +
> + encoder = connector_state->best_encoder;
> +
> + bridge = drm_bridge_chain_get_first_bridge(encoder);
> + if (!bridge)
> + continue;
> +
> + bridge_state = drm_atomic_get_new_bridge_state(state,
bridge);
> + if (!bridge_state)
> + bus_format = MEDIA_BUS_FMT_FIXED;
> + else
> + bus_format = bridge_state->input_bus_cfg.format;
> +
> + if (bus_format == MEDIA_BUS_FMT_FIXED) {
> + dev_warn(drm->dev,
> + "[ENCODER:%d:%s]'s bridge does not
provide bus format, assuming
> MEDIA_BUS_FMT_RGB888_1X24.\n" +
"Please fix bridge driver by handling
> atomic_get_input_bus_fmts.\n", +
encoder->bas
Re: [PATCH v3 3/6] drm: lcdif: Determine bus format and flags in ->atomic_check()
Hi Liu,
Am Mittwoch, 15. Februar 2023, 04:44:15 CET schrieb Liu Ying:
> On Tue, 2023-02-14 at 15:12 +0100, Alexander Stein wrote:
> > Hi Liu,
>
> Hi Alexander,
>
> > thanks for the update.
>
> Thanks for your review.
>
> > Am Montag, 13. Februar 2023, 09:56:09 CET schrieb Liu Ying:
> > > Instead of determining LCDIF output bus format and bus flags in
> > > ->atomic_enable(), do that in ->atomic_check(). This is a
> > > preparation for the upcoming patch to check consistent bus format
> > > and bus flags across all first downstream bridges in
> > > ->atomic_check().
> > > New lcdif_crtc_state structure is introduced to cache bus format
> > > and bus flags states in ->atomic_check() so that they can be read
> > > in ->atomic_enable().
> > >
> > > Signed-off-by: Liu Ying
> > > ---
> > > v2->v3:
> > > * No change.
> > >
> > > v1->v2:
> > > * Split from patch 2/2 in v1. (Marek, Alexander)
> > > * Add comment on the 'base' member of lcdif_crtc_state structure to
> > >
> > > note it should always be the first member. (Lothar)
> > >
> > > drivers/gpu/drm/mxsfb/lcdif_kms.c | 138 ++--
> > >
> > > --
> > >
> > > 1 file changed, 100 insertions(+), 38 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > b/drivers/gpu/drm/mxsfb/lcdif_kms.c index
> > > e54200a9fcb9..294cecdf5439 100644
> > > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > @@ -30,6 +30,18 @@
> > >
> > > #include "lcdif_drv.h"
> > > #include "lcdif_regs.h"
> > >
> > > +struct lcdif_crtc_state {
> > > + struct drm_crtc_state base; /* always be the first
> >
> > member */
> >
> > Is it strictly necessary that base is the first member?
> > to_lcdif_crtc_state()
> > should be able to handle any position within the struct. I mean it's
> > sensible
> > to put it in first place. But the comment somehow bugs me.
>
> The comment was added in v2 to make sure to_lcdif_crtc_state() work ok
> when a NULL pointer is handed over to it. The NULL pointer worries
> Lothar a bit as we can see Lothar's comment for v1.
Nice, this makes totally sense. I was just curious about the reasoning.
>
> > > + u32 bus_format;
> > > + u32 bus_flags;
> > > +};
> > > +
> > > +static inline struct lcdif_crtc_state *
> > > +to_lcdif_crtc_state(struct drm_crtc_state *s)
> > > +{
> > > + return container_of(s, struct lcdif_crtc_state, base);
> > > +}
> > > +
> > >
> > > /*
> > >
> > > -
> > > --
> > > -- * CRTC
> > >
> > > */
> > >
> > > @@ -385,48 +397,72 @@ static void lcdif_reset_block(struct
> > > lcdif_drm_private
> > > *lcdif) readl(lcdif->base + LCDC_V8_CTRL);
> > >
> > > }
> > >
> > > -static void lcdif_crtc_mode_set_nofb(struct lcdif_drm_private
> > > *lcdif,
> > > - struct drm_plane_state
> >
> > *plane_state,
> >
> > > - struct drm_bridge_state
> >
> > *bridge_state,
> >
> > > - const u32 bus_format)
> > > +static void lcdif_crtc_mode_set_nofb(struct drm_crtc_state
> > > *crtc_state,
> > > + struct drm_plane_state
> >
> > *plane_state)
> >
> > > {
> > >
> > > - struct drm_device *drm = lcdif->crtc.dev;
> > > - struct drm_display_mode *m = &lcdif->crtc.state->adjusted_mode;
> > > - u32 bus_flags = 0;
> > > -
> > > - if (lcdif->bridge->timings)
> > > - bus_flags = lcdif->bridge->timings->input_bus_flags;
> > > - else if (bridge_state)
> > > - bus_flags = bridge_state->input_bus_cfg.flags;
> > > + struct lcdif_crtc_state *lcdif_crtc_state =
> > > to_lcdif_crtc_state(crtc_state); +struct drm_device *drm =
> > > crtc_state->crtc->dev;
> > > + struct lcdif_drm_private *lcdif = to_lcdif_drm_private(drm);
> > > + struct drm_display_mode *m = &crtc_state->adjusted_mode;
Re: [PATCH v3 1/6] dt-bindings: lcdif: Add i.MX93 LCDIF support
Hi Liu,
Am Mittwoch, 15. Februar 2023, 08:49:56 CET schrieb Liu Ying:
> On Wed, 2023-02-15 at 08:26 +0100, Alexander Stein wrote:
> > Hi Liu,
>
> Hi Alexander,
>
> > thanks for the update.
>
> Thanks for the review.
>
> > Am Montag, 13. Februar 2023, 09:56:07 CET schrieb Liu Ying:
> > > There is one LCDIF embedded in i.MX93 SoC to connect with
> > > MIPI DSI controller through LCDIF cross line pattern(controlled
> > > by mediamix blk-ctrl) or connect with LVDS display bridge(LDB)
> > > directly or connect with a parallel display through parallel
> > > display format(also controlled by mediamix blk-ctrl). i.MX93
> > > LCDIF IP is essentially the same to i.MX8MP LCDIF IP. Add device
> > > tree binding for i.MX93 LCDIF.
> > >
> > > Acked-by: Krzysztof Kozlowski
> > > Reviewed-by: Marek Vasut
> > > Signed-off-by: Liu Ying
> > > ---
> > > v2->v3:
> > > * No change.
> > >
> > > v1->v2:
> > > * Add Krzysztof's A-b and Marek's R-b tags on patch 1/6.
> > >
> > > Documentation/devicetree/bindings/display/fsl,lcdif.yaml | 7
> > >
> > > ++-
> > >
> > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
> > > b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml index
> > > 75b4efd70ba8..fc11ab5fc465 100644
> > > --- a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
> > > +++ b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
> > >
> > > @@ -21,6 +21,7 @@ properties:
> > >- fsl,imx28-lcdif
> > >- fsl,imx6sx-lcdif
> > >- fsl,imx8mp-lcdif
> > >
> > > + - fsl,imx93-lcdif
> > >
> > >- items:
> > >- enum:
> > >- fsl,imx6sl-lcdif
> > >
> > > @@ -88,7 +89,9 @@ allOf:
> > >properties:
> > > compatible:
> > >contains:
> > > -const: fsl,imx8mp-lcdif
> > > +enum:
> > > + - fsl,imx8mp-lcdif
> > > + - fsl,imx93-lcdif
> > >
> > > then:
> > >properties:
> > > clocks:
> > > @@ -107,6 +110,7 @@ allOf:
> > >enum:
> > > - fsl,imx6sx-lcdif
> > > - fsl,imx8mp-lcdif
> > >
> > > +- fsl,imx93-lcdif
> > >
> > > then:
> > >properties:
> > > clocks:
> > > @@ -123,6 +127,7 @@ allOf:
> > >- fsl,imx8mm-lcdif
> > >- fsl,imx8mn-lcdif
> > >- fsl,imx8mp-lcdif
> > >
> > > + - fsl,imx93-lcdif
> > >
> > > then:
> > >required:
> > > - power-domains
> >
> > I would have expected that fsl,imx93-lcdif supports up to 3 endpoints
> > (MIPI
> > DSI, LVDS, and parallel) in a 'ports' subnode. But this binding only
> > supports
> > a single 'port' sub-node. Also an example for this case might be very
> > helpful.
>
> The port node allows multiple endpoints(See graph.yaml[1]). It's
> enough to use the existing port node instead of using ports node.
Ah, I wasn't aware of that possibility for OF graph. Yep, agreed then it's
enough.
> For i.MX93 LCDIF, the port node will be something like this:
> 8<--
> port {
> #address-cells = <1>;
> #size-cells = <0>;
>
> lcdif_to_pdfc: endpoint@0 {
> reg = <0>;
> };
>
> lcdif_to_ldb: endpoint@1 {
> reg = <1>;
> };
>
> lcdif_to_cross_line_pattern: endpoint@2 {
> reg = <2>;
> };
> };
> 8<--
>
> Looks like it's not necessary to add a specifc example for i.MX93
> LCDIF.
Fine by me.
Reviewed-by: Alexander Stein
> [1]
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph
> .yaml#L48
>
> Regards,
> Liu Ying
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
Re: [PATCH v4 3/6] drm: lcdif: Determine bus format and flags in ->atomic_check()
Hi Liu,
thanks for the update.
Am Freitag, 17. Februar 2023, 07:54:04 CET schrieb Liu Ying:
> Instead of determining LCDIF output bus format and bus flags in
> ->atomic_enable(), do that in ->atomic_check(). This is a
> preparation for the upcoming patch to check consistent bus format
> and bus flags across all first downstream bridges in ->atomic_check().
> New lcdif_crtc_state structure is introduced to cache bus format
> and bus flags states in ->atomic_check() so that they can be read
> in ->atomic_enable().
>
> Signed-off-by: Liu Ying
Look good to me.
Reviewed-by: Alexander Stein
> ---
> v3->v4:
> * Use 'new_{c,p}state' instead of 'new_{crtc,plane}_state'. (Alexander)
> * Simplify lcdif_crtc_reset() by calling lcdif_crtc_atomic_destroy_state().
> (Alexander)
> * Add '!crtc->state' check in lcdif_crtc_atomic_duplicate_state().
> (Alexander)
>
> v2->v3:
> * No change.
>
> v1->v2:
> * Split from patch 2/2 in v1. (Marek, Alexander)
> * Add comment on the 'base' member of lcdif_crtc_state structure to
> note it should always be the first member. (Lothar)
>
> drivers/gpu/drm/mxsfb/lcdif_kms.c | 134 ++
> 1 file changed, 99 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> b/drivers/gpu/drm/mxsfb/lcdif_kms.c index e54200a9fcb9..d46de433cd8e 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> @@ -30,6 +30,18 @@
> #include "lcdif_drv.h"
> #include "lcdif_regs.h"
>
> +struct lcdif_crtc_state {
> + struct drm_crtc_state base; /* always be the first
member */
> + u32 bus_format;
> + u32 bus_flags;
> +};
> +
> +static inline struct lcdif_crtc_state *
> +to_lcdif_crtc_state(struct drm_crtc_state *s)
> +{
> + return container_of(s, struct lcdif_crtc_state, base);
> +}
> +
> /*
> ---
> -- * CRTC
> */
> @@ -385,48 +397,72 @@ static void lcdif_reset_block(struct lcdif_drm_private
> *lcdif) readl(lcdif->base + LCDC_V8_CTRL);
> }
>
> -static void lcdif_crtc_mode_set_nofb(struct lcdif_drm_private *lcdif,
> - struct drm_plane_state
*plane_state,
> - struct drm_bridge_state
*bridge_state,
> - const u32 bus_format)
> +static void lcdif_crtc_mode_set_nofb(struct drm_crtc_state *crtc_state,
> + struct drm_plane_state
*plane_state)
> {
> - struct drm_device *drm = lcdif->crtc.dev;
> - struct drm_display_mode *m = &lcdif->crtc.state->adjusted_mode;
> - u32 bus_flags = 0;
> -
> - if (lcdif->bridge->timings)
> - bus_flags = lcdif->bridge->timings->input_bus_flags;
> - else if (bridge_state)
> - bus_flags = bridge_state->input_bus_cfg.flags;
> + struct lcdif_crtc_state *lcdif_crtc_state =
> to_lcdif_crtc_state(crtc_state); +struct drm_device *drm =
> crtc_state->crtc->dev;
> + struct lcdif_drm_private *lcdif = to_lcdif_drm_private(drm);
> + struct drm_display_mode *m = &crtc_state->adjusted_mode;
>
> DRM_DEV_DEBUG_DRIVER(drm->dev, "Pixel clock: %dkHz (actual: %dkHz)
\n",
>m->crtc_clock,
>(int)(clk_get_rate(lcdif->clk) / 1000));
> DRM_DEV_DEBUG_DRIVER(drm->dev, "Bridge bus_flags: 0x%08X\n",
> - bus_flags);
> + lcdif_crtc_state->bus_flags);
> DRM_DEV_DEBUG_DRIVER(drm->dev, "Mode flags: 0x%08X\n", m->flags);
>
> /* Mandatory eLCDIF reset as per the Reference Manual */
> lcdif_reset_block(lcdif);
>
> - lcdif_set_formats(lcdif, plane_state, bus_format);
> + lcdif_set_formats(lcdif, plane_state, lcdif_crtc_state->bus_format);
>
> - lcdif_set_mode(lcdif, bus_flags);
> + lcdif_set_mode(lcdif, lcdif_crtc_state->bus_flags);
> }
>
> static int lcdif_crtc_atomic_check(struct drm_crtc *crtc,
> struct drm_atomic_state *state)
> {
> + struct drm_device *drm = crtc->dev;
> + struct lcdif_drm_private *lcdif = to_lcdif_drm_private(drm);
> struct drm_crtc_state *crtc_state =
drm_atomic_get_new_crtc_state(state,
>
crtc);
> + struct lcdif_crtc_state *lcdif_crtc_state =
> to_lcdif_crtc_state(crtc_state); bool has
Re: [PATCH v4 4/6] drm: lcdif: Check consistent bus format and flags across first bridges
Hi Liu,
thanks for this update.
Am Freitag, 17. Februar 2023, 07:54:05 CET schrieb Liu Ying:
> The single LCDIF embedded in i.MX93 SoC may drive multiple displays
> simultaneously. Check bus format and flags across first bridges in
> ->atomic_check() to ensure they are consistent. This is a preparation
> for adding i.MX93 LCDIF support.
>
> Signed-off-by: Liu Ying
Acked-by: Alexander Stein
> ---
> v3->v4:
> * No change.
>
> v2->v3:
> * No change.
>
> v1->v2:
> * Split from patch 2/2 in v1. (Marek, Alexander)
> * Drop a comment about bridge input bus format from
> lcdif_crtc_atomic_check().
>
> drivers/gpu/drm/mxsfb/lcdif_drv.c | 2 -
> drivers/gpu/drm/mxsfb/lcdif_drv.h | 1 -
> drivers/gpu/drm/mxsfb/lcdif_kms.c | 76 ++-
> 3 files changed, 55 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> b/drivers/gpu/drm/mxsfb/lcdif_drv.c index cc2ceb301b96..b5b9a8e273c6 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> @@ -52,8 +52,6 @@ static int lcdif_attach_bridge(struct lcdif_drm_private
> *lcdif) if (ret)
> return dev_err_probe(drm->dev, ret, "Failed to attach
bridge\n");
>
> - lcdif->bridge = bridge;
> -
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.h
> b/drivers/gpu/drm/mxsfb/lcdif_drv.h index 6cdba6e20c02..aa6d099a1897 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_drv.h
> +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.h
> @@ -31,7 +31,6 @@ struct lcdif_drm_private {
> } planes;
> struct drm_crtc crtc;
> struct drm_encoder encoder;
> - struct drm_bridge *bridge;
> };
>
> static inline struct lcdif_drm_private *
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> b/drivers/gpu/drm/mxsfb/lcdif_kms.c index d46de433cd8e..d6009b353a16 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> @@ -17,6 +17,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -424,15 +425,19 @@ static int lcdif_crtc_atomic_check(struct drm_crtc
> *crtc, struct drm_atomic_state *state)
> {
> struct drm_device *drm = crtc->dev;
> - struct lcdif_drm_private *lcdif = to_lcdif_drm_private(drm);
> struct drm_crtc_state *crtc_state =
drm_atomic_get_new_crtc_state(state,
>
crtc);
> struct lcdif_crtc_state *lcdif_crtc_state =
> to_lcdif_crtc_state(crtc_state); bool has_primary = crtc_state->plane_mask
> &
> drm_plane_mask(crtc->primary);
> + struct drm_connector_state *connector_state;
> + struct drm_connector *connector;
> + struct drm_encoder *encoder;
> struct drm_bridge_state *bridge_state;
> - struct drm_bridge *bridge = lcdif->bridge;
> - int ret;
> + struct drm_bridge *bridge;
> + u32 bus_format, bus_flags;
> + bool format_set = false, flags_set = false;
> + int ret, i;
>
> /* The primary plane has to be enabled when the CRTC is active. */
> if (crtc_state->active && !has_primary)
> @@ -442,26 +447,55 @@ static int lcdif_crtc_atomic_check(struct drm_crtc
> *crtc, if (ret)
> return ret;
>
> - bridge_state = drm_atomic_get_new_bridge_state(state, bridge);
> - if (!bridge_state)
> - lcdif_crtc_state->bus_format = MEDIA_BUS_FMT_FIXED;
> - else
> - lcdif_crtc_state->bus_format = bridge_state-
>input_bus_cfg.format;
> -
> - if (lcdif_crtc_state->bus_format == MEDIA_BUS_FMT_FIXED) {
> - dev_warn_once(drm->dev,
> - "Bridge does not provide bus format,
assuming
> MEDIA_BUS_FMT_RGB888_1X24.\n" - "Please fix
bridge driver by
> handling atomic_get_input_bus_fmts.\n"); -lcdif_crtc_state-
>bus_format =
> MEDIA_BUS_FMT_RGB888_1X24;
> + /* Try to find consistent bus format and flags across first bridges.
*/
> + for_each_new_connector_in_state(state, connector, connector_state,
i) {
> + if (!connector_state->crtc)
> + continue;
> +
> + encoder = connector_state->best_encoder;
> +
> + bridge = drm_bridge_chain_get_first_bridge(encoder);
> + if (!bridge)
> + continue;
> +
> + bridge_state = drm_atomic_get_new_bridge_state(state,
bridge);
> + if (!bridge_state)
> +
Re: [PATCH v4 5/6] drm: lcdif: Add multiple encoders and first bridges support
Hi Liu,
thanks for the update.
Am Freitag, 17. Februar 2023, 07:54:06 CET schrieb Liu Ying:
> The single LCDIF embedded in i.MX93 SoC may drive multiple displays
> simultaneously. Look at LCDIF output port's remote port parents to
> find all enabled first bridges. Add an encoder for each found bridge
> and attach the bridge to the encoder. This is a preparation for
> adding i.MX93 LCDIF support.
>
> Signed-off-by: Liu Ying
Acked-by: Alexander Stein
> ---
> v3->v4:
> * Improve warning message when ignoring invalid LCDIF OF endpoint ids.
> (Alexander)
>
> v2->v3:
> * No change.
>
> v1->v2:
> * Split from patch 2/2 in v1. (Marek, Alexander)
> * Drop '!remote ||' from lcdif_attach_bridge(). (Lothar)
> * Drop unneeded 'bridges' member from lcdif_drm_private structure.
>
> drivers/gpu/drm/mxsfb/lcdif_drv.c | 68 +++
> drivers/gpu/drm/mxsfb/lcdif_drv.h | 4 +-
> drivers/gpu/drm/mxsfb/lcdif_kms.c | 21 ++
> 3 files changed, 66 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> b/drivers/gpu/drm/mxsfb/lcdif_drv.c index b5b9a8e273c6..f1f5caef390a 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_drv.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_drv.c
> @@ -9,13 +9,16 @@
> #include
> #include
> #include
> +#include
> #include
> +#include
> #include
> #include
>
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -38,19 +41,68 @@ static const struct drm_mode_config_helper_funcs
> lcdif_mode_config_helpers = { .atomic_commit_tail =
> drm_atomic_helper_commit_tail_rpm,
> };
>
> +static const struct drm_encoder_funcs lcdif_encoder_funcs = {
> + .destroy = drm_encoder_cleanup,
> +};
> +
> static int lcdif_attach_bridge(struct lcdif_drm_private *lcdif)
> {
> - struct drm_device *drm = lcdif->drm;
> + struct device *dev = lcdif->drm->dev;
> + struct device_node *ep;
> struct drm_bridge *bridge;
> int ret;
>
> - bridge = devm_drm_of_get_bridge(drm->dev, drm->dev->of_node, 0, 0);
> - if (IS_ERR(bridge))
> - return PTR_ERR(bridge);
> -
> - ret = drm_bridge_attach(&lcdif->encoder, bridge, NULL, 0);
> - if (ret)
> - return dev_err_probe(drm->dev, ret, "Failed to attach
bridge\n");
> + for_each_endpoint_of_node(dev->of_node, ep) {
> + struct device_node *remote;
> + struct of_endpoint of_ep;
> + struct drm_encoder *encoder;
> +
> + remote = of_graph_get_remote_port_parent(ep);
> + if (!of_device_is_available(remote)) {
> + of_node_put(remote);
> + continue;
> + }
> + of_node_put(remote);
> +
> + ret = of_graph_parse_endpoint(ep, &of_ep);
> + if (ret < 0) {
> + dev_err(dev, "Failed to parse endpoint %pOF\n",
ep);
> + of_node_put(ep);
> + return ret;
> + }
> +
> + if (of_ep.id >= MAX_DISPLAYS) {
> + dev_warn(dev, "ingoring invalid endpoint id
%u\n", of_ep.id);
> + continue;
> + }
> +
> + bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0,
of_ep.id);
> + if (IS_ERR(bridge)) {
> + of_node_put(ep);
> + return dev_err_probe(dev, PTR_ERR(bridge),
> + "Failed to get bridge
for endpoint%u\n",
> + of_ep.id);
> + }
> +
> + encoder = &lcdif->encoders[of_ep.id];
> + encoder->possible_crtcs = drm_crtc_mask(&lcdif->crtc);
> + ret = drm_encoder_init(lcdif->drm, encoder,
&lcdif_encoder_funcs,
> +DRM_MODE_ENCODER_NONE, NULL);
> + if (ret) {
> + dev_err(dev, "Failed to initialize encoder for
endpoint%u: %d\n",
> + of_ep.id, ret);
> + of_node_put(ep);
> + return ret;
> + }
> +
> + ret = drm_bridge_attach(encoder, bridge, NULL, 0);
> + if (ret) {
> + of_node_put(ep);
> + return dev_err_probe(dev, ret,
> + "Failed to attach
bridge for endpoint%u\n",
> +
Re: [PATCH v4 0/6] drm: lcdif: Add i.MX93 LCDIF support
Hi Liu,
Am Freitag, 17. Februar 2023, 07:54:01 CET schrieb Liu Ying:
> Hi,
>
> This patch set aims to add i.MX93 LCDIF display controller support
> in the existing LCDIF DRM driver. The LCDIF embedded in i.MX93 SoC
> is essentially the same to those embedded in i.MX8mp SoC. Through
> internal bridges, i.MX93 LCDIF may drive a MIPI DSI display or a LVDS
> display or a parallel display.
>
> Patch 1/6 adds device tree binding support for i.MX93 LCDIF in the
> existing fsl,lcdif.yaml.
>
> Patch 2/6 drops lcdif->bridge NULL pointer check as a cleanup patch.
>
> Patch 3/6~5/6 prepare for adding i.MX93 LCDIF support step by step.
>
> Patch 6/6 adds i.MX93 LCDIF compatible string as the last step of
> adding i.MX93 LCDIF support.
Thanks for the series. I could test this on my TQMa93xxLA/MBa93xxCA with a
single LVDS display attached, so no DSI or parallel display. Hence I could not
test the bus format and flags checks, but they look okay.
So you can add
Tested-by: Alexander Stein
to the whole series as well.
One thing I noticed is that, sometimes it seems that before probing lcdif my
system completely freezes. Adding some debug output it seems that's during
powering up the IMX93_MEDIABLK_PD_LCDIF power domain there is some race
condition. But adding more more detailed output made the problem go away.
Did you notice something similar? It might be a red hering though.
Best regards,
Alexander
>
> v3->v4:
> * Improve warning message when ignoring invalid LCDIF OF endpoint ids in
> patch 5/6. (Alexander)
> * Use 'new_{c,p}state' instead of 'new_{crtc,plane}_state' in patch 3/6.
> (Alexander)
> * Simplify lcdif_crtc_reset() by calling lcdif_crtc_atomic_destroy_state()
> in patch 3/6. (Alexander)
> * Add '!crtc->state' check in lcdif_crtc_atomic_duplicate_state() in patch
> 3/6. (Alexander)
> * Collect Alexander's R-b tags on patch 1/6, 2/6 and 6/6.
>
> v2->v3:
> * Fix a trivial typo in patch 6/6's commit message.
>
> v1->v2:
> * Add Krzysztof's A-b and Marek's R-b tags on patch 1/6.
> * Split patch 2/2 in v1 into patch 2/6~6/6 in v2. (Marek, Alexander)
> * Drop '!remote ||' from lcdif_attach_bridge(). (Lothar)
> * Add comment on the 'base' member of lcdif_crtc_state structure to
> note it should always be the first member. (Lothar)
> * Drop unneeded 'bridges' member from lcdif_drm_private structure.
> * Drop a comment about bridge input bus format from
> lcdif_crtc_atomic_check().
>
> Liu Ying (6):
> dt-bindings: lcdif: Add i.MX93 LCDIF support
> drm: lcdif: Drop unnecessary NULL pointer check on lcdif->bridge
> drm: lcdif: Determine bus format and flags in ->atomic_check()
> drm: lcdif: Check consistent bus format and flags across first bridges
> drm: lcdif: Add multiple encoders and first bridges support
> drm: lcdif: Add i.MX93 LCDIF compatible string
>
> .../bindings/display/fsl,lcdif.yaml | 7 +-
> drivers/gpu/drm/mxsfb/lcdif_drv.c | 71 ++-
> drivers/gpu/drm/mxsfb/lcdif_drv.h | 5 +-
> drivers/gpu/drm/mxsfb/lcdif_kms.c | 198 --
> 4 files changed, 206 insertions(+), 75 deletions(-)
--
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/
Re: [PATCH v12 00/18] drm: Add Samsung MIPI DSIM bridge
Am Freitag, 17. Februar 2023, 09:55:22 CET schrieb Rasmus Villemoes: > On 14/02/2023 12.09, Fabio Estevam wrote: > > Hi Rasmus, > > > > On Tue, Feb 14, 2023 at 7:55 AM Rasmus Villemoes > > > > wrote: > >> Well, the data sheet for the dsi86 says up to 750MHz DSI HS clock, and > >> if the value specified in samsung,burst-clock-frequency is twice the DSI > >> HS clk, I suppose I should be good up to 1.5GHz? I have tried many > >> different values, but I never seem to get anything through; I think I'm > >> missing some piece. > >> > >> So now I've tried to use these patches on the imx8mp-evk with the > >> mipi->hdmi accessory from NXP, just to see if I can ever get any > >> graphics through the mipi interface. And there the story is the same: > >> the adv7535 bridge gets probed, and can read out the edid from the > >> monitor over hdmi. And while the mipi block and the bridge seem to > >> attach to each other, I still don't get any output. > >> > >> Do any of you happen to have this working on the imx8mp-evk, and if so, > >> can you share the .dts updates you've done and how exactly you test the > >> graphics? > > > > I don't have access to an imx8mp-evk, but I tested the ADV7535 MIPI to > > HDMI daughter card on an imx8mm-evk. > > > > Some extra ADV7535 patches were needed. Please check patches 0020-0023 > > and see if they help. > > Thanks, but they don't seem to make a difference. > > I've started trying to simply compare registers between the NXP 5.15 > kernel and the imx8mm-dsi-v12 branch with Marek's patch on top. Already > in MEDIA_BLK_CTRL, 0x32ec, there's something interesting: > > ## Media Mix Clock Enable Register > -CLK_EN 00040080e133 > +CLK_EN 000400800133 > ## MIPI PHY Control Register > -MIPI_RESET_DIV 00084003 > +MIPI_RESET_DIV 00080002 > > So with the NXP kernel, there are three bits set in CLK_EN which are not > set with the "mainline", but those bits are marked reserved in the RM, > so I have no idea if they are just some RO bits that get set due to some > other munging. Then there's the MIPI_RESET_DIV register where bits 16 > and 30 do not get set. Of course, there are lots of other differences, > but perhaps this gives somebody an idea. Looking at drivers/soc/imx/imx8m-blk-ctrl.c the bits for MIPI_RESET_DIV are 16: MIPI_CSI1 17: MIPI_DSI 30: MIPI_CSI2 So i think that's okay here. Best regards, Alexander -- TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany Amtsgericht München, HRB 105018 Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider http://www.tq-group.com/
Re: [PATCH v4 0/6] drm: lcdif: Add i.MX93 LCDIF support
Hi Liu,
Am Freitag, 17. Februar 2023, 09:59:14 CET schrieb Liu Ying:
> On Fri, 2023-02-17 at 09:18 +0100, Alexander Stein wrote:
> > Hi Liu,
>
> Hi Alexander,
>
> > Am Freitag, 17. Februar 2023, 07:54:01 CET schrieb Liu Ying:
> > > Hi,
> > >
> > > This patch set aims to add i.MX93 LCDIF display controller support
> > > in the existing LCDIF DRM driver. The LCDIF embedded in i.MX93 SoC
> > > is essentially the same to those embedded in i.MX8mp SoC. Through
> > > internal bridges, i.MX93 LCDIF may drive a MIPI DSI display or a LVDS
> > > display or a parallel display.
> > >
> > > Patch 1/6 adds device tree binding support for i.MX93 LCDIF in the
> > > existing fsl,lcdif.yaml.
> > >
> > > Patch 2/6 drops lcdif->bridge NULL pointer check as a cleanup patch.
> > >
> > > Patch 3/6~5/6 prepare for adding i.MX93 LCDIF support step by step.
> > >
> > > Patch 6/6 adds i.MX93 LCDIF compatible string as the last step of
> > > adding i.MX93 LCDIF support.
> >
> > Thanks for the series. I could test this on my TQMa93xxLA/MBa93xxCA with a
> > single LVDS display attached, so no DSI or parallel display. Hence I could
> > not test the bus format and flags checks, but they look okay.
> > So you can add
> > Tested-by: Alexander Stein
> > to the whole series as well.
>
> Thanks for your test.
>
> > One thing I noticed is that, sometimes it seems that before probing lcdif
> > my system completely freezes. Adding some debug output it seems that's
> > during powering up the IMX93_MEDIABLK_PD_LCDIF power domain there is some
> > race condition. But adding more more detailed output made the problem go
> > away. Did you notice something similar? It might be a red hering though.
> I don't see system freezing with my i.MX93 11x11 EVK when probing
> lcdif. I did try to boot the system several times. All look ok. This is
> a snippet of dmesg when lcdif probes:
>
> --8<--
> [0.753083] Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled
> [0.761669] SuperH (H)SCI(F) driver initialized
> [0.766523] msm_serial: driver initialized
> [0.780523] printk: console [ttyLP0] enabled0x44380010 (irq = 16,
> base_baud = 150) is a FSL_LPUART
> [0.780523] printk: console [ttyLP0] enabled
> [0.788928] printk: bootconsole [lpuart32] disabled
> [0.788928] printk: bootconsole [lpuart32] disabled
> [0.804632] panel-simple lvds_panel: supply power not found, using
> dummy regulator
> [0.814741] [drm] Initialized imx-lcdif 1.0.0 20220417 for
> 4ae3.lcd-controller on minor 0
> [1.195930] Console: switching to colour frame buffer device 160x50
> [1.218385] imx-lcdif 4ae3.lcd-controller: [drm] fb0: imx-
> lcdifdrmfb frame buffer device
> [1.227099] cacheinfo: Unable to detect cache hierarchy for CPU 0
> [1.236725] loop: module loaded
> --8<--
>
> ~300 milliseconds are consumed by the enablement delay required by the
> "boe,ev121wxm-n10-1850" LVDS panel I use.
It seems you have the drivers compiled in. I use modules in my case and
simple-panel as well. But this is unrelated, because lcdif_probe() is yet to
be called. Using the debug diff from below I get the following output:
[ 16.97] imx93-blk-ctrl 4ac1.system-controller:
imx93_blk_ctrl_power_on: 1
[ 16.122491] imx93-blk-ctrl 4ac1.system-controller:
imx93_blk_ctrl_power_on: 2
[ 16.137766] imx93-blk-ctrl 4ac1.system-controller:
imx93_blk_ctrl_power_on: 3
[ 16.154905] imx93-blk-ctrl 4ac1.system-controller:
imx93_blk_ctrl_power_on: 4
It seems setting BLK_CLK_EN blocks the whole system, even reading is not
possible. I don't have any details on the hardware, but it seems that either
some clock or power domain is not enabled. This can also happen if I'm loading
the lcdif module manually after boot. But I can't detect any differences in /
sys/kernel/debug/clk/clk_summary.
---8<---
diff --git a/drivers/soc/imx/imx93-blk-ctrl.c b/drivers/soc/imx/imx93-blk-
ctrl.c
index 2c600329436cf..50aeb20ce90dc 100644
--- a/drivers/soc/imx/imx93-blk-ctrl.c
+++ b/drivers/soc/imx/imx93-blk-ctrl.c
@@ -129,12 +129,14 @@ static int imx93_blk_ctrl_power_on(struct
generic_pm_domain *genpd)
struct imx93_blk_ctrl *bc = domain->bc;
int ret;
+ dev_info(bc->dev, "%s: 1\n", __func__);
ret = clk_bulk_prepare_enable(bc->num_clks, bc->clks);
if (ret) {
dev_err(bc->dev, "failed to enable bus clocks\n");
Re: [PATCH v4 0/6] drm: lcdif: Add i.MX93 LCDIF support
Hi Liu, Am Montag, 20. Februar 2023, 09:55:19 CET schrieb Alexander Stein: > Hi Liu, > > Am Freitag, 17. Februar 2023, 09:59:14 CET schrieb Liu Ying: > > On Fri, 2023-02-17 at 09:18 +0100, Alexander Stein wrote: > > > Hi Liu, > > > > Hi Alexander, > > > > > Am Freitag, 17. Februar 2023, 07:54:01 CET schrieb Liu Ying: > > > > Hi, > > > > > > > > This patch set aims to add i.MX93 LCDIF display controller support > > > > in the existing LCDIF DRM driver. The LCDIF embedded in i.MX93 SoC > > > > is essentially the same to those embedded in i.MX8mp SoC. Through > > > > internal bridges, i.MX93 LCDIF may drive a MIPI DSI display or a LVDS > > > > display or a parallel display. > > > > > > > > Patch 1/6 adds device tree binding support for i.MX93 LCDIF in the > > > > existing fsl,lcdif.yaml. > > > > > > > > Patch 2/6 drops lcdif->bridge NULL pointer check as a cleanup patch. > > > > > > > > Patch 3/6~5/6 prepare for adding i.MX93 LCDIF support step by step. > > > > > > > > Patch 6/6 adds i.MX93 LCDIF compatible string as the last step of > > > > adding i.MX93 LCDIF support. > > > > > > Thanks for the series. I could test this on my TQMa93xxLA/MBa93xxCA with > > > a > > > single LVDS display attached, so no DSI or parallel display. Hence I > > > could > > > not test the bus format and flags checks, but they look okay. > > > So you can add > > > Tested-by: Alexander Stein > > > to the whole series as well. > > > > Thanks for your test. > > > > > One thing I noticed is that, sometimes it seems that before probing > > > lcdif > > > my system completely freezes. Adding some debug output it seems that's > > > during powering up the IMX93_MEDIABLK_PD_LCDIF power domain there is > > > some > > > race condition. But adding more more detailed output made the problem go > > > away. Did you notice something similar? It might be a red hering though. > > > > I don't see system freezing with my i.MX93 11x11 EVK when probing > > lcdif. I did try to boot the system several times. All look ok. This is > > a snippet of dmesg when lcdif probes: > > > > --8<-- > > [0.753083] Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled > > [0.761669] SuperH (H)SCI(F) driver initialized > > [0.766523] msm_serial: driver initialized > > [0.780523] printk: console [ttyLP0] enabled0x44380010 (irq = 16, > > base_baud = 150) is a FSL_LPUART > > [0.780523] printk: console [ttyLP0] enabled > > [0.788928] printk: bootconsole [lpuart32] disabled > > [0.788928] printk: bootconsole [lpuart32] disabled > > [0.804632] panel-simple lvds_panel: supply power not found, using > > dummy regulator > > [0.814741] [drm] Initialized imx-lcdif 1.0.0 20220417 for > > 4ae3.lcd-controller on minor 0 > > [1.195930] Console: switching to colour frame buffer device 160x50 > > [1.218385] imx-lcdif 4ae3.lcd-controller: [drm] fb0: imx- > > lcdifdrmfb frame buffer device > > [1.227099] cacheinfo: Unable to detect cache hierarchy for CPU 0 > > [1.236725] loop: module loaded > > --8<-- > > > > ~300 milliseconds are consumed by the enablement delay required by the > > "boe,ev121wxm-n10-1850" LVDS panel I use. > > It seems you have the drivers compiled in. I use modules in my case and > simple-panel as well. But this is unrelated, because lcdif_probe() is yet to > be called. Using the debug diff from below I get the following output: > > [ 16.97] imx93-blk-ctrl 4ac1.system-controller: > imx93_blk_ctrl_power_on: 1 > [ 16.122491] imx93-blk-ctrl 4ac1.system-controller: > imx93_blk_ctrl_power_on: 2 > [ 16.137766] imx93-blk-ctrl 4ac1.system-controller: > imx93_blk_ctrl_power_on: 3 > [ 16.154905] imx93-blk-ctrl 4ac1.system-controller: > imx93_blk_ctrl_power_on: 4 > > It seems setting BLK_CLK_EN blocks the whole system, even reading is not > possible. I don't have any details on the hardware, but it seems that either > some clock or power domain is not enabled. This can also happen if I'm > loading the lcdif module manually after boot. But I can't detect any > differences in / sys/kernel/debug/clk/clk_summary. I think I found the cause. It's the maximum
Re: [PATCH 1/1] drm: panel-simple: add missing bus flags for Tianma tm070jvhg[30/33]
Hi,
a gentle ping
Best regards,
Alexander
Am Mittwoch, 25. Januar 2023, 15:52:15 CET schrieb Alexander Stein:
> From: Markus Niebel
>
> The DE signal is active high on this display, fill in the missing
> bus_flags. This aligns panel_desc with its display_timing.
>
> Fixes: 9a2654c0f62a ("drm/panel: Add and fill drm_panel type field")
> Fixes: b3bfcdf8a3b6 ("drm/panel: simple: add Tianma TM070JVHG33")
> Signed-off-by: Markus Niebel
> Signed-off-by: Alexander Stein
> ---
> drivers/gpu/drm/panel/panel-simple.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c
> b/drivers/gpu/drm/panel/panel-simple.c index 065f378bba9d..fbccaf1cb6f2
> 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -3598,6 +3598,7 @@ static const struct panel_desc tianma_tm070jdhg30 = {
> },
> .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> .connector_type = DRM_MODE_CONNECTOR_LVDS,
> + .bus_flags = DRM_BUS_FLAG_DE_HIGH,
> };
>
> static const struct panel_desc tianma_tm070jvhg33 = {
> @@ -3610,6 +3611,7 @@ static const struct panel_desc tianma_tm070jvhg33 = {
> },
> .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> .connector_type = DRM_MODE_CONNECTOR_LVDS,
> + .bus_flags = DRM_BUS_FLAG_DE_HIGH,
> };
>
> static const struct display_timing tianma_tm070rvhg71_timing = {
--
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/
[PATCH 1/1] drm: panel: simple: convert LG LB070WV8 fixed mode into display timings
At least the pixelclock has a range which can vary. Convert fixed mode
into display timings so they can be overwritten in DT if necessary.
Signed-off-by: Alexander Stein
---
drivers/gpu/drm/panel/panel-simple.c | 24
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-simple.c
b/drivers/gpu/drm/panel/panel-simple.c
index 065f378bba9d..5048be54ffd9 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -2458,21 +2458,21 @@ static const struct panel_desc lemaker_bl035_rgb_002 = {
.bus_flags = DRM_BUS_FLAG_DE_LOW,
};
-static const struct drm_display_mode lg_lb070wv8_mode = {
- .clock = 33246,
- .hdisplay = 800,
- .hsync_start = 800 + 88,
- .hsync_end = 800 + 88 + 80,
- .htotal = 800 + 88 + 80 + 88,
- .vdisplay = 480,
- .vsync_start = 480 + 10,
- .vsync_end = 480 + 10 + 25,
- .vtotal = 480 + 10 + 25 + 10,
+static const struct display_timing lg_lb070wv8_timing = {
+ .pixelclock = { 3195, 3326, 3460 },
+ .hactive = { 800, 800, 800 },
+ .hfront_porch = { 88, 88, 88 },
+ .hback_porch = { 88, 88, 88 },
+ .hsync_len = { 80, 80, 80 },
+ .vactive = { 480, 480, 480 },
+ .vfront_porch = { 10, 10, 10 },
+ .vback_porch = { 10, 10, 10 },
+ .vsync_len = { 25, 25, 25 },
};
static const struct panel_desc lg_lb070wv8 = {
- .modes = &lg_lb070wv8_mode,
- .num_modes = 1,
+ .timings = &lg_lb070wv8_timing,
+ .num_timings = 1,
.bpc = 8,
.size = {
.width = 151,
--
2.34.1
[PATCH 1/1] drm/bridge: sii902x: Use dev_err_probe
This helps figuring out why the device probe is deferred. Signed-off-by: Alexander Stein --- drivers/gpu/drm/bridge/sii902x.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c index d212ff7f7a87..f4a8f227c41b 100644 --- a/drivers/gpu/drm/bridge/sii902x.c +++ b/drivers/gpu/drm/bridge/sii902x.c @@ -1116,7 +1116,8 @@ static int sii902x_probe(struct i2c_client *client) sii902x->next_bridge = of_drm_find_bridge(remote); of_node_put(remote); if (!sii902x->next_bridge) - return -EPROBE_DEFER; + return dev_err_probe(dev, -EPROBE_DEFER, +"Failed to find remote bridge\n"); } mutex_init(&sii902x->mutex); -- 2.34.1
[PATCH 1/2] drm: fsl-dcu: Use dev_err_probe
fsl_dcu_drm_modeset_init can return -EPROBE_DEFER, so use dev_err_probe
to remove an invalid error message and add it to deferral description.
Signed-off-by: Alexander Stein
---
drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
index 8579c7629f5e..418887654bac 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
@@ -103,10 +103,8 @@ static int fsl_dcu_load(struct drm_device *dev, unsigned
long flags)
int ret;
ret = fsl_dcu_drm_modeset_init(fsl_dev);
- if (ret < 0) {
- dev_err(dev->dev, "failed to initialize mode setting\n");
- return ret;
- }
+ if (ret < 0)
+ return dev_err_probe(dev->dev, ret, "failed to initialize mode
setting\n");
ret = drm_vblank_init(dev, dev->mode_config.num_crtc);
if (ret < 0) {
--
2.34.1
[PATCH 2/2] drm: fsl-dcu: enable PIXCLK on LS1021A
From: Matthias Schiffer
The PIXCLK needs to be enabled in SCFG before accessing certain DCU
registers, or the access will hang.
Signed-off-by: Matthias Schiffer
Signed-off-by: Alexander Stein
---
drivers/gpu/drm/fsl-dcu/Kconfig | 1 +
drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 14 ++
drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h | 3 +++
3 files changed, 18 insertions(+)
diff --git a/drivers/gpu/drm/fsl-dcu/Kconfig b/drivers/gpu/drm/fsl-dcu/Kconfig
index 5ca71ef87325..c9ee98693b48 100644
--- a/drivers/gpu/drm/fsl-dcu/Kconfig
+++ b/drivers/gpu/drm/fsl-dcu/Kconfig
@@ -8,6 +8,7 @@ config DRM_FSL_DCU
select DRM_PANEL
select REGMAP_MMIO
select VIDEOMODE_HELPERS
+ select MFD_SYSCON if SOC_LS1021A
help
Choose this option if you have an Freescale DCU chipset.
If M is selected the module will be called fsl-dcu-drm.
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
index 418887654bac..314cb8af89ee 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
@@ -100,12 +100,26 @@ static void fsl_dcu_irq_uninstall(struct drm_device *dev)
static int fsl_dcu_load(struct drm_device *dev, unsigned long flags)
{
struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
+ struct regmap *scfg;
int ret;
ret = fsl_dcu_drm_modeset_init(fsl_dev);
if (ret < 0)
return dev_err_probe(dev->dev, ret, "failed to initialize mode
setting\n");
+ scfg = syscon_regmap_lookup_by_compatible("fsl,ls1021a-scfg");
+ if (PTR_ERR(scfg) != -ENODEV) {
+ /*
+* For simplicity, enable the PIXCLK unconditionally. Disabling
+* the clock in PM or on unload could be implemented as a future
+* improvement.
+*/
+ ret = regmap_update_bits(scfg, SCFG_PIXCLKCR,
SCFG_PIXCLKCR_PXCEN,
+ SCFG_PIXCLKCR_PXCEN);
+ if (ret < 0)
+ return dev_err_probe(dev->dev, ret, "failed to enable
pixclk\n");
+ }
+
ret = drm_vblank_init(dev, dev->mode_config.num_crtc);
if (ret < 0) {
dev_err(dev->dev, "failed to initialize vblank\n");
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
index e2049a0e8a92..566396013c04 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
@@ -160,6 +160,9 @@
#define FSL_DCU_ARGB 12
#define FSL_DCU_YUV422 14
+#define SCFG_PIXCLKCR 0x28
+#define SCFG_PIXCLKCR_PXCENBIT(31)
+
#define VF610_LAYER_REG_NUM9
#define LS1021A_LAYER_REG_NUM 10
--
2.34.1
Re: [PATCH 1/1] drm: bridge: ldb: Warn if LDB clock does not match requested link frequency
Hi everyone,
Am Donnerstag, 8. Dezember 2022, 07:55:38 CET schrieb Alexander Stein:
> The LDB clock needs to be exactly 7-times the pixel clock used by the
> display.
Any feedback on this?
Thanks
Alexander
> Signed-off-by: Alexander Stein
> ---
> i.MX8MP has a dedicated LDB clock which defines the actual LVDS link
> frequency. This has to be (exactly) the 7-time of the pixel clock.
> Although the clock min/max range is available, panel-simple does not (yet)
> use the range to find a (perfect) frequency which can be used down the
> chain, which is also in range.
> Depending on the pixel clock the exact multiple might not be configured.
> Raise a warning if there is a mismatch, which might cause an invalid display
> image.
>
> drivers/gpu/drm/bridge/fsl-ldb.c | 23 +++
> 1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c
> b/drivers/gpu/drm/bridge/fsl-ldb.c index f9e0f8d992680..9bcba8fc57e74
> 100644
> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
> @@ -66,6 +66,14 @@ static inline struct fsl_ldb *to_fsl_ldb(struct
> drm_bridge *bridge) return container_of(bridge, struct fsl_ldb, bridge);
> }
>
> +static unsigned long fsl_ldb_link_frequency(struct fsl_ldb *fsl_ldb, int
> clock) +{
> + if (fsl_ldb->lvds_dual_link)
> + return clock * 3500;
> + else
> + return clock * 7000;
> +}
> +
> static int fsl_ldb_attach(struct drm_bridge *bridge,
> enum drm_bridge_attach_flags flags)
> {
> @@ -85,6 +93,8 @@ static void fsl_ldb_atomic_enable(struct drm_bridge
> *bridge, const struct drm_display_mode *mode;
> struct drm_connector *connector;
> struct drm_crtc *crtc;
> + unsigned long configured_link_freq;
> + unsigned long requested_link_freq;
> bool lvds_format_24bpp;
> bool lvds_format_jeida;
> u32 reg;
> @@ -128,10 +138,15 @@ static void fsl_ldb_atomic_enable(struct drm_bridge
> *bridge, crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> mode = &crtc_state->adjusted_mode;
>
> - if (fsl_ldb->lvds_dual_link)
> - clk_set_rate(fsl_ldb->clk, mode->clock * 3500);
> - else
> - clk_set_rate(fsl_ldb->clk, mode->clock * 7000);
> + requested_link_freq = fsl_ldb_link_frequency(fsl_ldb, mode->clock);
> + clk_set_rate(fsl_ldb->clk, requested_link_freq);
> +
> + configured_link_freq = clk_get_rate(fsl_ldb->clk);
> + if (configured_link_freq != requested_link_freq)
> + dev_warn(fsl_ldb->dev, "Configured LDB clock (%lu Hz) does
not match
> requested LVDS clock: %lu Hz", +
configured_link_freq,
> + requested_link_freq);
> +
> clk_prepare_enable(fsl_ldb->clk);
>
> /* Program LDB_CTRL */
Re: [PATCH 2/2] drm: lcdif: Add i.MX93 LCDIF support
Hi, Am Dienstag, 24. Januar 2023, 08:59:39 CET schrieb Liu Ying: > On Mon, 2023-01-23 at 16:57 +0100, Marek Vasut wrote: > > On 1/23/23 08:23, Liu Ying wrote: > > > The LCDIF embedded in i.MX93 SoC is essentially the same to those > > > in i.MX8mp SoC. However, i.MX93 LCDIF may connect with MIPI DSI > > > controller through LCDIF cross line pattern(controlled by mediamix > > > blk-ctrl) or connect with LVDS display bridge(LDB) directly or a > > > parallel display(also through mediamix blk-ctrl), so add multiple > > > encoders(with DRM_MODE_ENCODER_NONE encoder type) support in the > > > LCDIF DRM driver and find a bridge to attach the relevant encoder's > > > chain when needed. While at it, derive lcdif_crtc_state structure > > > from drm_crtc_state structure to introduce bus_format and bus_flags > > > states so that the next downstream bridges may use consistent bus > > > format and bus flags. > > > > Would it be possible to split this patch into preparatory clean up > > and > > i.MX93 addition ? It seems like the patch is doing two things > > according > > to the commit message. > > IMHO, all the patch does is for i.MX93 addition, not for clean up. > Note that the single LCDIF embedded in i.MX93 SoC may connect with MIPI > DSI/LVDS/parallel related bridges to drive triple displays > _simultaneously_ in theory, while the three LCDIF instances embedded in > i.MX8mp SoC connect with MIPI DSI/LVDS/HDMI displays respectively(one > LCDIF maps to one display). The multiple encoders addition and the new > checks for consistent bus format and bus flags are only for i.MX93 > LCDIF, not for i.MX8mp LCDIF. Also, I think the multiple encoders > addition and the new checks should be done together - if the new checks > come first, then the new checks do not make sense(no multiple displays > driven by LCDIF); You are right on this one, but on the other hand there are lot of preparing patches already. Even if it is useless by itself, having the bus format & flag checks in a separate patch, it is easier to review, IMHO. > if the new checks come later, then it would be a bug > to allow inconsistent bus format and bus flags across the next > downstream bridges when only adding multiple encoders support(also, I > don't know which encoder's bridge should determine the LCDIF output bus > format and bus flags, since the three encoders come together with the > three next bridges). Agreed, this order is a no-go. Best regards, Alexander > Regards, > Liu Ying
Re: [PATCH v7 00/10] drm: bridge: Add Samsung MIPI DSIM bridge
Hi Jagan, Am Mittwoch, 5. Oktober 2022, 17:12:59 CEST schrieb Jagan Teki: > This series supports common bridge support for Samsung MIPI DSIM > which is used in Exynos and i.MX8MM SoC's. > > The final bridge supports both the Exynos and i.MX8MM DSI devices. > > Changes for v7: > * fix the drm bridge attach chain for exynos drm dsi driver > * fix the hw_type checking logic > > Changes for v6: > * handle previous bridge for exynos dsi while attaching bridge > > Changes for v5: > * bridge changes to support multi-arch > * updated and clear commit messages > * add hw_type via plat data > * removed unneeded quirk > * rebased on linux-next > > Changes for v4: > * include Inki Dae in MAINTAINERS > * remove dsi_driver probe in exynos_drm_drv to support multi-arch build > * update init handling to ensure host init done on first cmd transfer > > Changes for v3: > * fix the mult-arch build > * fix dsi host init > * updated commit messages > > Changes for v2: > * fix bridge handling > * fix dsi host init > * correct the commit messages > > Patch 0001: Samsung DSIM bridge > > Patch 0002: PHY optional > > Patch 0003: OF-graph or Child node lookup > > Patch 0004: DSI host initialization > > Patch 0005: atomic check > > Patch 0006: PMS_P offset via plat data > > Patch 0007: atomic_get_input_bus_fmts > > Patch 0008: input_bus_flags > > Patch 0009: document fsl,imx8mm-mipi-dsim > > Patch 0010: add i.MX8MM DSIM support > > Tested in Engicam i.Core MX8M Mini SoM. Thanks for working on this! This works on TQMa8MQML + MBa8Mx (imx8mm) using a SN65DSI83 DSI-LVDS-Bridge. Tested-by: Alexander Stein Best regards, Alexander > Repo: > https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v7 > > Any inputs? > Jagan. > > Jagan Teki (10): > drm: bridge: Add Samsung DSIM bridge driver > drm: bridge: samsung-dsim: Lookup OF-graph or Child node devices > drm: bridge: samsung-dsim: Mark PHY as optional > drm: bridge: samsung-dsim: Handle proper DSI host initialization > drm: bridge: samsung-dsim: Add atomic_check > drm: bridge: samsung-dsim: Add platform PLL_P (PMS_P) offset > drm: bridge: samsung-dsim: Add atomic_get_input_bus_fmts > drm: bridge: samsung-dsim: Add input_bus_flags > dt-bindings: display: exynos: dsim: Add NXP i.MX8MM support > drm: bridge: samsung-dsim: Add i.MX8MM support > > .../bindings/display/exynos/exynos_dsim.txt |1 + > MAINTAINERS |9 + > drivers/gpu/drm/bridge/Kconfig| 12 + > drivers/gpu/drm/bridge/Makefile |1 + > drivers/gpu/drm/bridge/samsung-dsim.c | 1856 + > drivers/gpu/drm/exynos/Kconfig|1 + > drivers/gpu/drm/exynos/exynos_drm_dsi.c | 1766 +--- > include/drm/bridge/samsung-dsim.h | 115 + > 8 files changed, 2108 insertions(+), 1653 deletions(-) > create mode 100644 drivers/gpu/drm/bridge/samsung-dsim.c > create mode 100644 include/drm/bridge/samsung-dsim.h
Re: [PATCH v4 00/10] Initial support for Cadence MHDP(HDMI/DP) for i.MX8MQ
Hello Sandor, thanks for the updated series. Am Montag, 21. November 2022, 08:23:50 CET schrieb Sandor Yu: > The patch set initial support for Cadence MHDP(HDMI/DP) DRM bridge > drivers and Cadence HDP-TX PHY(HDMI/DP) drivers for iMX8MQ. > > The patch set compose of DRM bridge drivers and PHY drivers. > Both of them need the followed two patches to pass build. > drm: bridge: cadence: convert mailbox functions to macro functions > phy: Add HDMI configuration options > > DRM bridges driver patches: > dts-bingings: display: bridge: Add MHDP HDMI bindings for i.MX8MQ > drm: bridge: cadence: Add MHDP DP driver for i.MX8MQ > dts-bindings: display: bridge: Add MHDP DP bindings for i.MX8MQ > drm: bridge: cadence: Add MHDP HDMI driver for i.MX8MQ > > PHY driver patches: > dts-bindings: phy: Add Cadence HDP-TX DP PHY bindings > phy: cadence: Add driver for HDP-TX DisplyPort PHY > dts-bindings: phy: Add Cadence HDP-TX HDMI PHY bindings > phy: cadence: Add driver for HDP-TX HDMI PHY > > v3->v4: > dt-bindings: > - Correct dt-bindings coding style and address review comments. > - Add apb_clk description. > - Add output port for HDMI/DP connector > PHY: > - Alphabetically sorted in Kconfig and Makefile for DP and HDMI PHY > - Remove unused registers define from HDMI and DP PHY drivers. > - More description in phy_hdmi.h. > - Add apb_clk to HDMI and DP phy driver. > HDMI/DP: > - Use get_unaligned_le32() to replace hardcode type conversion > in HDMI AVI infoframe data fill function. > - Add mailbox mutex lock in HDMI/DP driver for phy functions > to reslove race conditions between HDMI/DP and PHY drivers. > - Add apb_clk to both HDMI and DP driver. > - Rename some function names and add prefix with "cdns_hdmi/cdns_dp". > - Remove bpc 12 and 16 optional that not supported. With the apb_clk enabled now, I can use both HDMI and PHY driver as modules now. Thanks! Best regards, Alexander > v2->v3: > Address comments for dt-bindings files. > - Correct dts-bindings file names > Rename phy-cadence-hdptx-dp.yaml to cdns,mhdp-imx8mq-dp.yaml > Rename phy-cadence-hdptx-hdmi.yaml to cdns,mhdp-imx8mq-hdmi.yaml > - Drop redundant words and descriptions. > - Correct hdmi/dp node name. > > v2 is a completely different version compared to v1. > Previous v1 can be available here [1]. > > v1->v2: > - Reuse Cadence mailbox access functions from mhdp8546 instead of > rockchip DP. > - Mailbox access functions be convert to marco functions > that will be referenced by HDP-TX PHY(HDMI/DP) driver too. > - Plain bridge instead of component driver. > - Standalone Cadence HDP-TX PHY(HDMI/DP) driver. > - Audio driver are removed from the patch set, it will be add in another > patch set later. > > [1] > https://patchwork.kernel.org/project/linux-rockchip/cover/cover.1590982881. > git.sandor...@nxp.com/ > > Sandor Yu (10): > drm: bridge: cadence: convert mailbox functions to macro functions > dt-bindings: display: bridge: Add MHDP DP for i.MX8MQ > drm: bridge: cadence: Add MHDP DP driver for i.MX8MQ > phy: Add HDMI configuration options > dt-bindings: display: bridge: Add MHDP HDMI for i.MX8MQ > drm: bridge: cadence: Add MHDP HDMI driver for i.MX8MQ > dt-bindings: phy: Add Cadence HDP-TX DP PHY > phy: cadence: Add driver for HDP-TX DisplyPort PHY > dt-bindings: phy: Add Cadence HDP-TX HDMI PHY > phy: cadence: Add driver for HDP-TX HDMI PHY > > .../display/bridge/cdns,mhdp-imx8mq-dp.yaml | 93 ++ > .../display/bridge/cdns,mhdp-imx8mq-hdmi.yaml | 93 ++ > .../bindings/phy/cdns,hdptx-dp-phy.yaml | 68 ++ > .../bindings/phy/cdns,hdptx-hdmi-phy.yaml | 52 + > drivers/gpu/drm/bridge/cadence/Kconfig| 25 + > drivers/gpu/drm/bridge/cadence/Makefile |3 + > drivers/gpu/drm/bridge/cadence/cdns-dp-core.c | 1071 + > .../gpu/drm/bridge/cadence/cdns-hdmi-core.c | 1018 > .../gpu/drm/bridge/cadence/cdns-mhdp-common.h | 400 ++ > .../drm/bridge/cadence/cdns-mhdp8546-core.c | 197 +-- > .../drm/bridge/cadence/cdns-mhdp8546-core.h |1 - > drivers/phy/cadence/Kconfig | 16 + > drivers/phy/cadence/Makefile |2 + > drivers/phy/cadence/phy-cadence-hdptx-dp.c| 737 > drivers/phy/cadence/phy-cadence-hdptx-hdmi.c | 891 ++ > include/drm/bridge/cdns-mhdp-mailbox.h| 240 > include/linux/phy/phy-hdmi.h | 38 + > include/linux/phy/phy.h |7 +- > 18 files changed, 4755 insertions(+), 197 deletions(-) > create mode 100644 > Documentation/devicetree/bindings/display/bridge/cdns,mhdp-imx8mq-dp.yaml > create mode 100644 > Documentation/devicetree/bindings/display/bridge/cdns,mhdp-imx8mq-hdmi.yaml > create mode 100644 > Documentation/devicetree/bindings/phy/cdns,hdptx-dp-phy.yaml create mode > 100644 Documentation/devicetree/bindings/phy/cdns,hdptx-hdmi-phy.yaml > create mode 100644 drivers/gpu/drm/br
Re: [PATCH] drm/bridge: ti-sn65dsi83: Fix delay after reset deassert to match spec
Am Dienstag, 22. November 2022, 09:12:18 CET schrieb Frieder Schrempf:
> From: Frieder Schrempf
>
> The datasheet specifies a delay of 10 milliseconds, but the current
> driver only waits for 1 ms. Fix this to make sure the initialization
> sequence meets the spec.
>
> Fixes: ceb515ba29ba ("drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 and
> SN65DSI84 driver") Signed-off-by: Frieder Schrempf
>
> ---
> drivers/gpu/drm/bridge/ti-sn65dsi83.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index 7ba9467fff12..047c14ddbbf1
> 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> @@ -346,7 +346,7 @@ static void sn65dsi83_atomic_enable(struct drm_bridge
> *bridge,
>
> /* Deassert reset */
> gpiod_set_value_cansleep(ctx->enable_gpio, 1);
> - usleep_range(1000, 1100);
> + usleep_range(1, 11000);
>
> /* Get the LVDS format from the bridge state. */
> bridge_state = drm_atomic_get_new_bridge_state(state, bridge);
How about using fsleep?
Either way:
Reviewed-by: Alexander Stein
Re: [PATCH 1/2] drm/ofdrm: Cast PCI IDs to u32 for comparing
Hello Thomas,
Am Donnerstag, 27. Oktober 2022, 13:57:06 CEST schrieb Thomas Zimmermann:
> Properties of 32-bit integers are returned from the OF device tree
> as type __be32. Cast PCI vendor and device IDs from __be32 to u32
> before comparing them to constants. Fixes sparse warnings shown below.
>
> drivers/gpu/drm/tiny/ofdrm.c:237:17: warning: restricted __be32 degrades
> to integer drivers/gpu/drm/tiny/ofdrm.c:238:18: warning: restricted __be32
> degrades to integer drivers/gpu/drm/tiny/ofdrm.c:238:54: warning:
> restricted __be32 degrades to integer
>
> See [1] for the bug report.
>
> Reported-by: kernel test robot
> Signed-off-by: Thomas Zimmermann
> Link: https://lore.kernel.org/dri-devel/202210192208.d888i6x7-...@intel.com/
> # [1] ---
> drivers/gpu/drm/tiny/ofdrm.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/tiny/ofdrm.c b/drivers/gpu/drm/tiny/ofdrm.c
> index 0e1cc2369afcc..0da8b248ccc6e 100644
> --- a/drivers/gpu/drm/tiny/ofdrm.c
> +++ b/drivers/gpu/drm/tiny/ofdrm.c
> @@ -231,8 +231,11 @@ static u64 display_get_address_of(struct drm_device
> *dev, struct device_node *of return address;
> }
>
> -static bool is_avivo(__be32 vendor, __be32 device)
> +static bool is_avivo(__be32 vendor_id, __be32 device_id)
> {
> + u32 vendor = (__force u32)vendor_id;
> + u32 device = (__force u32)device_id;
I don't have much context, but just from reading this, shouldn't this be
be32_to_cpu() instead?
Best regards,
Alexander
> +
> /* This will match most R5xx */
> return (vendor == PCI_VENDOR_ID_ATI) &&
> ((device >= PCI_VENDOR_ID_ATI_R520 && device < 0x7800) ||
Re: [PATCH 1/2] drm/ofdrm: Cast PCI IDs to u32 for comparing
Hi Thomas,
Am Donnerstag, 27. Oktober 2022, 16:04:34 CEST schrieb Thomas Zimmermann:
> * PGP Signed: 10/27/2022 at 04:04:34 PM
>
> Hi
>
> Am 27.10.22 um 15:07 schrieb Alexander Stein:
> > Hello Thomas,
> >
> > Am Donnerstag, 27. Oktober 2022, 13:57:06 CEST schrieb Thomas Zimmermann:
> >> Properties of 32-bit integers are returned from the OF device tree
> >> as type __be32. Cast PCI vendor and device IDs from __be32 to u32
> >> before comparing them to constants. Fixes sparse warnings shown below.
> >>
> >>drivers/gpu/drm/tiny/ofdrm.c:237:17: warning: restricted __be32
> >>degrades
> >>
> >> to integer drivers/gpu/drm/tiny/ofdrm.c:238:18: warning: restricted
> >> __be32
> >> degrades to integer drivers/gpu/drm/tiny/ofdrm.c:238:54: warning:
> >> restricted __be32 degrades to integer
> >>
> >> See [1] for the bug report.
> >>
> >> Reported-by: kernel test robot
> >> Signed-off-by: Thomas Zimmermann
> >> Link:
> >> https://lore.kernel.org/dri-devel/202210192208.d888i6x7-...@intel.com/ #
> >> [1] ---
> >>
> >> drivers/gpu/drm/tiny/ofdrm.c | 5 -
> >> 1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/tiny/ofdrm.c b/drivers/gpu/drm/tiny/ofdrm.c
> >> index 0e1cc2369afcc..0da8b248ccc6e 100644
> >> --- a/drivers/gpu/drm/tiny/ofdrm.c
> >> +++ b/drivers/gpu/drm/tiny/ofdrm.c
> >> @@ -231,8 +231,11 @@ static u64 display_get_address_of(struct drm_device
> >> *dev, struct device_node *of return address;
> >>
> >> }
> >>
> >> -static bool is_avivo(__be32 vendor, __be32 device)
> >> +static bool is_avivo(__be32 vendor_id, __be32 device_id)
> >>
> >> {
> >>
> >> + u32 vendor = (__force u32)vendor_id;
> >> + u32 device = (__force u32)device_id;
> >
> > I don't have much context, but just from reading this, shouldn't this be
> > be32_to_cpu() instead?
>
> I should have explained that in the commit message. The values are
> supposed to be in big endian. We compare to PCI ids. The code originally
> was taken from [1], which does the right thing. The next version will
> add this info to the commit message.
>
> Best regards
> Thomas
>
> [1]
> https://elixir.bootlin.com/linux/v6.0.5/source/drivers/video/fbdev/offb.c#L3
> 57
Thanks for the link. The commit message for that original change [2] indicates
this was a fix on a Powerstation which happens to be a PowerPC aka big-endian.
This this happens to work as intended, but not on little-endian machines.
vendor_id is __be32 (big-endian) and you compare this to PCI_VENDOR_ID_ATI
(0x1002) which is native-endian. I guess you see the problem.
See also [3] where a property is converted properly.
Thinking about it, instead of calling 'is_avivo(*vendor_p, *device_p)'
I'd prefer something like 'is_avivo(be32_to_cpup(vendor_p),
be32_to_cpup(device_p))', so there is no need to pass __be32 around.
Best regards,
Alexander
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
commit/?id=57a20d8fb0d2a05abe40abd6bb29e3f923721f1b
[3]
https://elixir.bootlin.com/linux/v6.0.5/source/drivers/bus/fsl-mc/fsl-mc-bus.c#L1027
> > Best regards,
> > Alexander
> >
> >> +
> >>
> >>/* This will match most R5xx */
> >>return (vendor == PCI_VENDOR_ID_ATI) &&
> >>
> >> ((device >= PCI_VENDOR_ID_ATI_R520 && device < 0x7800) ||
Re: [v2 06/10] drm: bridge: cadence: Add MHDP HDMI driver for i.MX8MQ
Hello,
thanks for working on this and the updated version.
Am Freitag, 4. November 2022, 07:44:56 CET schrieb Sandor Yu:
> Add a new DRM HDMI bridge driver for Candence MHDP used in i.MX8MQ
> SOC. MHDP IP could support HDMI or DisplayPort standards according
> embedded Firmware running in the uCPU.
>
> For iMX8MQ SOC, the HDMI FW was loaded and activated by SOC ROM code.
> Bootload binary included HDMI FW was required for the driver.
>
> Signed-off-by: Sandor Yu
> ---
> drivers/gpu/drm/bridge/cadence/Kconfig| 12 +
> .../gpu/drm/bridge/cadence/cdns-hdmi-core.c | 1038 +
> 2 files changed, 1050 insertions(+)
> create mode 100644 drivers/gpu/drm/bridge/cadence/cdns-hdmi-core.c
>
> diff --git a/drivers/gpu/drm/bridge/cadence/Kconfig
> b/drivers/gpu/drm/bridge/cadence/Kconfig index e79ae1af3765..377452d09992
> 100644
> --- a/drivers/gpu/drm/bridge/cadence/Kconfig
> +++ b/drivers/gpu/drm/bridge/cadence/Kconfig
> @@ -26,6 +26,18 @@ config DRM_CDNS_MHDP8546_J721E
> clock and data muxes.
> endif
>
> +config DRM_CDNS_HDMI
> + tristate "Cadence HDMI DRM driver"
> + select DRM_KMS_HELPER
> + select DRM_PANEL_BRIDGE
> + select DRM_DISPLAY_HELPER
> + select DRM_CDNS_AUDIO
> + depends on OF
> + help
> + Support Cadence MHDP HDMI driver.
> + Cadence MHDP Controller support one or more protocols,
> + HDMI firmware is required for this driver.
> +
> config DRM_CDNS_DP
> tristate "Cadence DP DRM driver"
> select DRM_KMS_HELPER
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-hdmi-core.c
> b/drivers/gpu/drm/bridge/cadence/cdns-hdmi-core.c new file mode 100644
> index ..b392aac015bd
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-hdmi-core.c
> @@ -0,0 +1,1038 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Cadence High-Definition Multimedia Interface (HDMI) driver
> + *
> + * Copyright (C) 2019-2022 NXP Semiconductor, Inc.
> + *
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "cdns-mhdp-common.h"
> +
> +void cdns_mhdp_infoframe_set(struct cdns_mhdp_device *mhdp,
> + u8 entry_id, u8 packet_len,
u8 *packet, u8 packet_type)
> +{
> + u32 *packet32, len32;
> + u32 val, i;
> +
> + /* invalidate entry */
> + val = F_ACTIVE_IDLE_TYPE(1) | F_PKT_ALLOC_ADDRESS(entry_id);
> + writel(val, mhdp->regs + SOURCE_PIF_PKT_ALLOC_REG);
> + writel(F_PKT_ALLOC_WR_EN(1), mhdp->regs +
SOURCE_PIF_PKT_ALLOC_WR_EN);
> +
> + /* flush fifo 1 */
> + writel(F_FIFO1_FLUSH(1), mhdp->regs + SOURCE_PIF_FIFO1_FLUSH);
> +
> + /* write packet into memory */
> + packet32 = (u32 *)packet;
This only works on little-endian machines, no?
> + len32 = packet_len / 4;
> + for (i = 0; i < len32; i++)
> + writel(F_DATA_WR(packet32[i]), mhdp->regs +
SOURCE_PIF_DATA_WR);
> +
> + /* write entry id */
> + writel(F_WR_ADDR(entry_id), mhdp->regs + SOURCE_PIF_WR_ADDR);
> +
> + /* write request */
> + writel(F_HOST_WR(1), mhdp->regs + SOURCE_PIF_WR_REQ);
> +
> + /* update entry */
> + val = F_ACTIVE_IDLE_TYPE(1) | F_TYPE_VALID(1) |
> + F_PACKET_TYPE(packet_type) |
F_PKT_ALLOC_ADDRESS(entry_id);
> + writel(val, mhdp->regs + SOURCE_PIF_PKT_ALLOC_REG);
> +
> + writel(F_PKT_ALLOC_WR_EN(1), mhdp->regs +
SOURCE_PIF_PKT_ALLOC_WR_EN);
> +}
> +
> +static int cdns_hdmi_get_edid_block(void *data, u8 *edid,
> + u32 block, size_t length)
> +{
> + struct cdns_mhdp_device *mhdp = data;
> + u8 msg[2], reg[5], i;
> + int ret;
> +
> + mutex_lock(&mhdp->mbox_mutex);
> +
> + for (i = 0; i < 4; i++) {
What is i? It is not used inside the loop.
> + msg[0] = block / 2;
> + msg[1] = block % 2;
> +
> + ret = cdns_mhdp_mailbox_send(mhdp, MB_MODULE_ID_HDMI_TX,
HDMI_TX_EDID,
> + sizeof(msg), msg);
> + if (ret)
> + continue;
> +
> + ret = cdns_mhdp_mailbox_recv_header(mhdp,
MB_MODULE_ID_HDMI_TX,
> +
HDMI_TX_EDID, sizeof(reg) + length);
> + if (ret)
> + continue;
> +
> + ret = cdns_mhdp_mailbox_recv_data(mhdp, reg, sizeof(reg));
> + if (ret)
> + continue;
> +
> + ret = cdns_mhdp_mailbox_recv_data(mhdp, edid, length);
> + if (ret)
> + continue;
> +
> + if ((reg[3] << 8 | reg[4]) == length)
> + break;
> + }
> +
> + mutex_unlock(&mhdp->mbox_mutex);
> +
> + if (ret)
> + DRM_ERROR("get block[%d] edid fail
RE: [EXT] Re: [v2 06/10] drm: bridge: cadence: Add MHDP HDMI driver for i.MX8MQ
Hi Sandor,
Am Mittwoch, 9. November 2022, 14:26:14 CET schrieb Sandor Yu:
> Thanks for your comments.
>
>
> > -Original Message-
> > From: Alexander Stein
> > Sent: 2022年11月8日 21:17
> > To: jo...@kwiboo.se; Sandor Yu
> > Cc: dri-devel@lists.freedesktop.org; devicet...@vger.kernel.org;
> > linux-arm-ker...@lists.infradead.org; linux-ker...@vger.kernel.org;
> > linux-...@lists.infradead.org; andrzej.ha...@intel.com;
> > neil.armstr...@linaro.org; robert.f...@linaro.org;
> > laurent.pinch...@ideasonboard.com; jernej.skra...@gmail.com;
> > kis...@ti.com; vk...@kernel.org; Oliver Brown ;
> > krzysztof.kozlowski...@linaro.org; s...@ravnborg.org;
> > jani.nik...@intel.com;
tzimmerm...@suse.de; s.ha...@pengutronix.de;
> > javi...@redhat.com;
> > penguin-ker...@i-love.sakura.ne.jp; robh...@kernel.org; dl-linux-imx
> > ; ker...@pengutronix.de; Sandor Yu
> > ; shawn...@kernel.org; p.ya...@ti.com;
> > max...@cerno.tech
> > Subject: [EXT] Re: [v2 06/10] drm: bridge: cadence: Add MHDP HDMI driver
> > for i.MX8MQ
> >
> > Caution: EXT Email
> >
> > Hello,
> >
> > thanks for working on this and the updated version.
> >
> > Am Freitag, 4. November 2022, 07:44:56 CET schrieb Sandor Yu:
> >
> > > Add a new DRM HDMI bridge driver for Candence MHDP used in i.MX8MQ
> > > SOC. MHDP IP could support HDMI or DisplayPort standards according
> > > embedded Firmware running in the uCPU.
> > >
> > >
> > >
> > > For iMX8MQ SOC, the HDMI FW was loaded and activated by SOC ROM
> >
> > code.
> >
> > > Bootload binary included HDMI FW was required for the driver.
> > >
> > >
> > >
> > > Signed-off-by: Sandor Yu
> > > ---
> > >
> > > drivers/gpu/drm/bridge/cadence/Kconfig| 12 +
> > > .../gpu/drm/bridge/cadence/cdns-hdmi-core.c | 1038
> >
> > +
> >
> > > 2 files changed, 1050 insertions(+)
> > > create mode 100644 drivers/gpu/drm/bridge/cadence/cdns-hdmi-core.c
> > >
> > >
> > >
> > > diff --git a/drivers/gpu/drm/bridge/cadence/Kconfig
> > > b/drivers/gpu/drm/bridge/cadence/Kconfig index
> > > e79ae1af3765..377452d09992
> > > 100644
> > > --- a/drivers/gpu/drm/bridge/cadence/Kconfig
> > > +++ b/drivers/gpu/drm/bridge/cadence/Kconfig
[snip]
> > > +static int cdns_hdmi_get_edid_block(void *data, u8 *edid,
> > > + u32 block, size_t length) {
> > > + struct cdns_mhdp_device *mhdp = data;
> > > + u8 msg[2], reg[5], i;
> > > + int ret;
> > > +
> > > + mutex_lock(&mhdp->mbox_mutex);
> > > +
> > > + for (i = 0; i < 4; i++) {
> >
> >
> > What is i? It is not used inside the loop.
>
> EDID data read by HDMI firmware are not guarantee 100% successful.
> Base on experiments, try 4 times if EDID read failed.
Mh, 4 times sounds a bit too arbitrary to me. How about using a timeout in ms,
like 50ms, for retrying to read the EDID?
[snip]
> > > +static int cdns_mhdp_imx_probe(struct platform_device *pdev) {
> > > + struct device *dev = &pdev->dev;
> > > + struct cdns_mhdp_device *mhdp;
> > > + struct platform_device_info pdevinfo;
> > > + struct resource *res;
> > > + u32 reg;
> > > + int ret;
> > > +
> > > + mhdp = devm_kzalloc(dev, sizeof(*mhdp), GFP_KERNEL);
> > > + if (!mhdp)
> > > + return -ENOMEM;
> > > +
> > > + mutex_init(&mhdp->lock);
> > > + mutex_init(&mhdp->mbox_mutex);
> > > +
> > > + INIT_DELAYED_WORK(&mhdp->hotplug_work, hotplug_work_func);
> > > +
> > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > + if (!res)
> > > + return -ENODEV;
> > > + mhdp->regs = devm_ioremap(dev, res->start, resource_size(res));
> > > + if (IS_ERR(mhdp->regs))
> > > + return PTR_ERR(mhdp->regs);
> >
> >
> > Please use devm_platform_get_and_ioremap_resource.
>
> Both HDMI PHY driver and mhdp HDMI driver should access same APB base
> register offset for mailbox.
devm_ioremap_resource could not support such
> feature.
Oh I see, both remap the same range. To be honest I do not like this. Is there
a need to map overlapping ranges in both drivers? How can you avoid race
conditions due to simultaneous ac
[PATCH 1/2] dt-bindings: lcdif: Fix clock constraints for imx8mp
i.MX8MP uses 3 clocks, so soften the restrictions for clocks & clock-names.
Fixes: f5419cb0743f ("dt-bindings: lcdif: Add compatible for i.MX8MP")
Signed-off-by: Alexander Stein
---
Documentation/devicetree/bindings/display/fsl,lcdif.yaml | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
index 876015a44a1e6..793e8eccf8b8b 100644
--- a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
+++ b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
@@ -70,7 +70,9 @@ allOf:
properties:
compatible:
contains:
-const: fsl,imx6sx-lcdif
+enum:
+ - fsl,imx6sx-lcdif
+ - fsl,imx8mp-lcdif
then:
properties:
clocks:
--
2.34.1
[PATCH 2/2] dt-bindings: lcdif: Add optional power-domain
i.MX8MP requires a power-domain for this peripheral to use. Add it as an optional property. Signed-off-by: Alexander Stein --- Documentation/devicetree/bindings/display/fsl,lcdif.yaml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml index 793e8eccf8b8b..9d9fb5ad587c2 100644 --- a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml +++ b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml @@ -52,6 +52,9 @@ properties: interrupts: maxItems: 1 + power-domains: +maxItems: 1 + port: $ref: /schemas/graph.yaml#/properties/port description: The LCDIF output port -- 2.34.1
Re: [PATCH 2/2] dt-bindings: lcdif: Add optional power-domain
Am Mittwoch, 7. Dezember 2022, 17:00:22 CET schrieb Marek Vasut: > On 12/7/22 16:14, Alexander Stein wrote: > > i.MX8MP requires a power-domain for this peripheral to use. Add it as > > an optional property. > > > > Signed-off-by: Alexander Stein > > --- > > > > Documentation/devicetree/bindings/display/fsl,lcdif.yaml | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml > > b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml index > > 793e8eccf8b8b..9d9fb5ad587c2 100644 > > --- a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml > > +++ b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml > > > > @@ -52,6 +52,9 @@ properties: > > interrupts: > > maxItems: 1 > > > > + power-domains: > > +maxItems: 1 > > + > > > > port: > > $ref: /schemas/graph.yaml#/properties/port > > description: The LCDIF output port > > Should this be required on mx8mp then ? I'm hesitating to add a required property later on. But I'm okay with both. Rob, Krzysztof: Any preference here? Shall power-domains be made required for fsl,imx8mp-lcdif only? Best regards, Alexander
Re: [PATCH 1/2] dt-bindings: lcdif: Fix clock constraints for imx8mp
Hello Marek,
Am Mittwoch, 7. Dezember 2022, 16:59:50 CET schrieb Marek Vasut:
> On 12/7/22 16:13, Alexander Stein wrote:
> > i.MX8MP uses 3 clocks, so soften the restrictions for clocks &
> > clock-names.
> >
> > Fixes: f5419cb0743f ("dt-bindings: lcdif: Add compatible for i.MX8MP")
> > Signed-off-by: Alexander Stein
> > ---
> >
> > Documentation/devicetree/bindings/display/fsl,lcdif.yaml | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
> > b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml index
> > 876015a44a1e6..793e8eccf8b8b 100644
> > --- a/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
> > +++ b/Documentation/devicetree/bindings/display/fsl,lcdif.yaml
> >
> > @@ -70,7 +70,9 @@ allOf:
> > properties:
> > compatible:
> > contains:
> > -const: fsl,imx6sx-lcdif
> > +enum:
> > + - fsl,imx6sx-lcdif
> > + - fsl,imx8mp-lcdif
> >
> > then:
> > properties:
> > clocks:
> Reviewed-by: Marek Vasut
Thanks!
> btw you might want to update the clock-names and clock proerty order in
> imx8mp.dtsi to match the clock-names order in these bindings.
The lcdif nodes are not yet in linux-next ;-) So its probably a local commit
on your side. But yes, the upcoming patches will address this.
Best regards,
Alexander
