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
> > edata=05%7C01%7Cvictor.liu%40nxp.com%7Ca4ee326b3f314cf48a3408da49ec5a
> > 33%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637903576722519563%7CUnkno
> > wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXV
> > CI6Mn0%3D%7C3000%7C%7C%7Csdata=yJhovCeB7Dx2eWJqKohZskA7fX3NLwqmW1GxeQ
> > lDe40%3Dreserved=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;
> > > +
> >
Re: (EXT) [PATCH v8 09/14] drm/bridge: imx: Add LDB driver helper support
On Thu, 2022-06-09 at 09:47 +0200, Alexander Stein wrote:
> 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 <
> > robert.f...@linaro.org>
> > Signed-off-by: Liu Ying
> > ---
>
> Hi,
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)?
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.
Regards,
Liu Ying
>
> Best regards,
> Alexander
>
> [1]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.freedesktop.org%2Fpatch%2Fmsgid%2F20220426193645.244792-2-marex%40denx.dedata=05%7C01%7Cvictor.liu%40nxp.com%7Ca4ee326b3f314cf48a3408da49ec5a33%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637903576722519563%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7Csdata=yJhovCeB7Dx2eWJqKohZskA7fX3NLwqmW1GxeQlDe40%3Dreserved=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;
> > +
> > + return 0;
> > +}
> > +
> > +void
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
