Hi,
The Allwinner LVDS encoder allows to change the polarity of clocks and data lanes, so let's add the needed bus flags to DRM, panel-lvds and our encoder driver.
Let me know what you think, Maxime
Maxime Ripard (4): drm/connector: Add data polarity flags dt-bindings: panel: lvds: Add properties for clock and data polarities drm/panel: lvds: Support data and clock polarity flags drm/sun4i: lvds: Support data and clock polarity flags
Documentation/devicetree/bindings/display/panel/lvds.yaml | 10 +++- drivers/gpu/drm/panel/panel-lvds.c | 25 +++++++- drivers/gpu/drm/sun4i/sun4i_tcon.c | 16 ++++- include/drm/drm_connector.h | 4 +- 4 files changed, 49 insertions(+), 6 deletions(-)
base-commit: bb6d3fb354c5ee8d6bde2d576eb7220ea09862b9
Some LVDS encoders can change the polarity of the data signals being sent. Add a DRM bus flag to reflect this.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- include/drm/drm_connector.h | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 221910948b37..9a08fe6ab7c2 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -330,6 +330,8 @@ enum drm_panel_orientation { * edge of the pixel clock * @DRM_BUS_FLAG_SHARP_SIGNALS: Set if the Sharp-specific signals * (SPL, CLS, PS, REV) must be used + * @DRM_BUS_FLAG_DATA_LOW: The Data signals are active low + * @DRM_BUS_FLAG_DATA_HIGH: The Data signals are active high */ enum drm_bus_flags { DRM_BUS_FLAG_DE_LOW = BIT(0), @@ -349,6 +351,8 @@ enum drm_bus_flags { DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE = DRM_BUS_FLAG_SYNC_NEGEDGE, DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE = DRM_BUS_FLAG_SYNC_POSEDGE, DRM_BUS_FLAG_SHARP_SIGNALS = BIT(8), + DRM_BUS_FLAG_DATA_LOW = BIT(9), + DRM_BUS_FLAG_DATA_HIGH = BIT(10), };
/**
Hi Maxime.
On Fri, Feb 14, 2020 at 01:24:38PM +0100, Maxime Ripard wrote:
Some LVDS encoders can change the polarity of the data signals being sent. Add a DRM bus flag to reflect this.
Signed-off-by: Maxime Ripard maxime@cerno.tech
include/drm/drm_connector.h | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 221910948b37..9a08fe6ab7c2 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -330,6 +330,8 @@ enum drm_panel_orientation {
edge of the pixel clock- @DRM_BUS_FLAG_SHARP_SIGNALS: Set if the Sharp-specific signals
(SPL, CLS, PS, REV) must be used
- @DRM_BUS_FLAG_DATA_LOW: The Data signals are active low
- @DRM_BUS_FLAG_DATA_HIGH: The Data signals are active high
Reading the description of these falgs always confuses me. In this case if neither bit 9 nor bit 10 is set then the data signals are netiher active low nor active high. So what can I then expect?
We have the same logic loophole for DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE and DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE. So it is not new, but can we do better here?
Sam
*/ enum drm_bus_flags { DRM_BUS_FLAG_DE_LOW = BIT(0), @@ -349,6 +351,8 @@ enum drm_bus_flags { DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE = DRM_BUS_FLAG_SYNC_NEGEDGE, DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE = DRM_BUS_FLAG_SYNC_POSEDGE, DRM_BUS_FLAG_SHARP_SIGNALS = BIT(8),
- DRM_BUS_FLAG_DATA_LOW = BIT(9),
- DRM_BUS_FLAG_DATA_HIGH = BIT(10),
};
/**
git-series 0.9.1
On Fri, Feb 14, 2020 at 05:13:59PM +0100, Sam Ravnborg wrote:
Hi Maxime.
On Fri, Feb 14, 2020 at 01:24:38PM +0100, Maxime Ripard wrote:
Some LVDS encoders can change the polarity of the data signals being sent. Add a DRM bus flag to reflect this.
Signed-off-by: Maxime Ripard maxime@cerno.tech
include/drm/drm_connector.h | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 221910948b37..9a08fe6ab7c2 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -330,6 +330,8 @@ enum drm_panel_orientation {
edge of the pixel clock- @DRM_BUS_FLAG_SHARP_SIGNALS: Set if the Sharp-specific signals
(SPL, CLS, PS, REV) must be used
- @DRM_BUS_FLAG_DATA_LOW: The Data signals are active low
- @DRM_BUS_FLAG_DATA_HIGH: The Data signals are active high
Reading the description of these falgs always confuses me. In this case if neither bit 9 nor bit 10 is set then the data signals are netiher active low nor active high. So what can I then expect?
We have the same logic loophole for DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE and DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE. So it is not new, but can we do better here?
Honestly, I don't really get it either. I *think* this is to handle the sampling / output inversion properly which wouldn't be possible if this was only a bit set or not.
Maxime
Some LVDS encoders can support multiple polarities on the data and clock lanes, and similarly some panels require a given polarity on their inputs. Add a property on the panel to configure the encoder properly.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- Documentation/devicetree/bindings/display/panel/lvds.yaml | 10 ++++++++- 1 file changed, 10 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/panel/lvds.yaml b/Documentation/devicetree/bindings/display/panel/lvds.yaml index d0083301acbe..4a1111a1ab38 100644 --- a/Documentation/devicetree/bindings/display/panel/lvds.yaml +++ b/Documentation/devicetree/bindings/display/panel/lvds.yaml @@ -90,6 +90,16 @@ properties: CTL2: Data Enable CTL3: 0
+ clock-active-low: + type: boolean + description: > + If set, reverse the clock polarity on the clock lane. + + data-active-low: + type: boolean + description: > + If set, reverse the bit polarity on all data lanes. + data-mirror: type: boolean description:
Hi Maxime.
On Fri, Feb 14, 2020 at 01:24:39PM +0100, Maxime Ripard wrote:
Some LVDS encoders can support multiple polarities on the data and clock lanes, and similarly some panels require a given polarity on their inputs. Add a property on the panel to configure the encoder properly.
Signed-off-by: Maxime Ripard maxime@cerno.tech
Not a fan of this binding... In display-timing.txt we have a specification/description of the panel-timing node.
The panel-timing node already include information such as: - hsync-active: - vsync-active: - de-active: - pixelclk-active: - syncclk-active:
But clock-active-low and data-active-low refer to the bus more than an individual timing. So maybe OK not to have it in a panel-timing node. But then it would IMO be better to include this in the display-timing node - so we make this available and standard for all users of the display-timing node.
I will dig up my patchset to make proper bindings for panel-timing and display-timing this weeked and resend them. Then we can discuss if this goes on top or this is specific for the lvds binding.
Documentation/devicetree/bindings/display/panel/lvds.yaml | 10 ++++++++- 1 file changed, 10 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/panel/lvds.yaml b/Documentation/devicetree/bindings/display/panel/lvds.yaml index d0083301acbe..4a1111a1ab38 100644 --- a/Documentation/devicetree/bindings/display/panel/lvds.yaml +++ b/Documentation/devicetree/bindings/display/panel/lvds.yaml @@ -90,6 +90,16 @@ properties: CTL2: Data Enable CTL3: 0
- clock-active-low:
- type: boolean
- description: >
Should this be "|" and not ">"? Did this pass dt_binding_check?
If set, reverse the clock polarity on the clock lane.
This text could be a bit more specific. If this is set then what? And it seems strange that a clock is active low. For a clock we often talk about raising or falling edge.
- data-active-low:
- type: boolean
- description: >
Same comment with ">"
If set, reverse the bit polarity on all data lanes.
Same comment about a more explicit description.
Sam
data-mirror: type: boolean description: -- git-series 0.9.1
On Fri, Feb 14, 2020 at 05:11:56PM +0100, Sam Ravnborg wrote:
Hi Maxime.
On Fri, Feb 14, 2020 at 01:24:39PM +0100, Maxime Ripard wrote:
Some LVDS encoders can support multiple polarities on the data and clock lanes, and similarly some panels require a given polarity on their inputs. Add a property on the panel to configure the encoder properly.
Signed-off-by: Maxime Ripard maxime@cerno.tech
Not a fan of this binding... In display-timing.txt we have a specification/description of the panel-timing node.
The panel-timing node already include information such as:
- hsync-active:
- vsync-active:
- de-active:
- pixelclk-active:
- syncclk-active:
But clock-active-low and data-active-low refer to the bus more than an individual timing. So maybe OK not to have it in a panel-timing node. But then it would IMO be better to include this in the display-timing node - so we make this available and standard for all users of the display-timing node.
I will dig up my patchset to make proper bindings for panel-timing and display-timing this weeked and resend them. Then we can discuss if this goes on top or this is specific for the lvds binding.
That makes sense, I'll wait for them to be merged then :)
Documentation/devicetree/bindings/display/panel/lvds.yaml | 10 ++++++++- 1 file changed, 10 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/panel/lvds.yaml b/Documentation/devicetree/bindings/display/panel/lvds.yaml index d0083301acbe..4a1111a1ab38 100644 --- a/Documentation/devicetree/bindings/display/panel/lvds.yaml +++ b/Documentation/devicetree/bindings/display/panel/lvds.yaml @@ -90,6 +90,16 @@ properties: CTL2: Data Enable CTL3: 0
- clock-active-low:
- type: boolean
- description: >
Should this be "|" and not ">"? Did this pass dt_binding_check?
Yes. > means that this is a multi-line string that will drop the \n between each line, while | will keep it
For a string like this, I believe it makes more sense to let whatever is using to handle the wrapping, but I don't really have a strong opinion :)
If set, reverse the clock polarity on the clock lane.This text could be a bit more specific. If this is set then what? And it seems strange that a clock is active low. For a clock we often talk about raising or falling edge.
- data-active-low:
- type: boolean
- description: >
Same comment with ">"
If set, reverse the bit polarity on all data lanes.Same comment about a more explicit description.
I'll try to come up with something better. Thanks! Maxime
On Fri, Feb 14, 2020 at 01:24:39PM +0100, Maxime Ripard wrote:
Some LVDS encoders can support multiple polarities on the data and clock lanes, and similarly some panels require a given polarity on their inputs. Add a property on the panel to configure the encoder properly.
If the panel requires a specific setting, then that can be implied by the panel's compatible. Does Boris' format constraint solving series help here?
Rob
Add device tree properties to the panel-lvds driver to set the bus flags properly.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/panel/panel-lvds.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-lvds.c b/drivers/gpu/drm/panel/panel-lvds.c index 5ce3f4a2b7a1..c0d6dcd9e9fc 100644 --- a/drivers/gpu/drm/panel/panel-lvds.c +++ b/drivers/gpu/drm/panel/panel-lvds.c @@ -31,6 +31,8 @@ struct panel_lvds { unsigned int height; struct videomode video_mode; unsigned int bus_format; + bool clk_active_low; + bool data_active_low; bool data_mirror;
struct regulator *supply; @@ -83,6 +85,7 @@ static int panel_lvds_get_modes(struct drm_panel *panel, { struct panel_lvds *lvds = to_panel_lvds(panel); struct drm_display_mode *mode; + unsigned int flags = 0;
mode = drm_mode_create(connector->dev); if (!mode) @@ -96,9 +99,23 @@ static int panel_lvds_get_modes(struct drm_panel *panel, connector->display_info.height_mm = lvds->height; drm_display_info_set_bus_formats(&connector->display_info, &lvds->bus_format, 1); - connector->display_info.bus_flags = lvds->data_mirror - ? DRM_BUS_FLAG_DATA_LSB_TO_MSB - : DRM_BUS_FLAG_DATA_MSB_TO_LSB; + + if (lvds->data_mirror) + flags |= DRM_BUS_FLAG_DATA_LSB_TO_MSB; + else + flags |= DRM_BUS_FLAG_DATA_MSB_TO_LSB; + + if (lvds->clk_active_low) + flags |= DRM_BUS_FLAG_PIXDATA_NEGEDGE; + else + flags |= DRM_BUS_FLAG_PIXDATA_POSEDGE; + + if (lvds->data_active_low) + flags |= DRM_BUS_FLAG_DATA_LOW; + else + flags |= DRM_BUS_FLAG_DATA_HIGH; + + connector->display_info.bus_flags = flags;
return 1; } @@ -159,6 +176,8 @@ static int panel_lvds_parse_dt(struct panel_lvds *lvds) return -EINVAL; }
+ lvds->clk_active_low = of_property_read_bool(np, "clock-active-low"); + lvds->data_active_low = of_property_read_bool(np, "data-active-low"); lvds->data_mirror = of_property_read_bool(np, "data-mirror");
return 0;
Our LVDS encoder can change the polarity of data and clock signals on the LVDS link. Make sure we don't ignore the matching bus flags.
Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/sun4i/sun4i_tcon.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c index c81cdce6ed55..fdf143042f83 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c @@ -404,6 +404,8 @@ static void sun4i_tcon0_mode_set_lvds(struct sun4i_tcon *tcon, const struct drm_encoder *encoder, const struct drm_display_mode *mode) { + struct drm_connector *connector = sun4i_tcon_get_connector(encoder); + const struct drm_display_info *info = &connector->display_info; unsigned int bp; u8 clk_delay; u32 reg, val = 0; @@ -449,9 +451,17 @@ static void sun4i_tcon0_mode_set_lvds(struct sun4i_tcon *tcon, SUN4I_TCON0_BASIC2_V_TOTAL(mode->crtc_vtotal * 2) | SUN4I_TCON0_BASIC2_V_BACKPORCH(bp));
- reg = SUN4I_TCON0_LVDS_IF_CLK_SEL_TCON0 | - SUN4I_TCON0_LVDS_IF_DATA_POL_NORMAL | - SUN4I_TCON0_LVDS_IF_CLK_POL_NORMAL; + reg = SUN4I_TCON0_LVDS_IF_CLK_SEL_TCON0; + if (info->bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE) + reg |= SUN4I_TCON0_LVDS_IF_CLK_POL_INV; + else + reg |= SUN4I_TCON0_LVDS_IF_CLK_POL_NORMAL; + + if (info->bus_flags & DRM_BUS_FLAG_DATA_LOW) + reg |= SUN4I_TCON0_LVDS_IF_DATA_POL_INV; + else + reg |= SUN4I_TCON0_LVDS_IF_DATA_POL_NORMAL; + if (sun4i_tcon_get_pixel_depth(encoder) == 24) reg |= SUN4I_TCON0_LVDS_IF_BITWIDTH_24BITS; else
dri-devel@lists.freedesktop.org
-
Maxime Ripard -
Rob Herring -
Sam Ravnborg