RE: [PATCH v3 3/4] drm: ipk: Add extensions for DW MIPI DSI Host driver
From: Daniel Vetter
Date: Tue, Apr 28, 2020 at 16:28:15
> On Mon, Apr 27, 2020 at 04:00:35PM +0200, Angelo Ribeiro wrote:
> > Add Synopsys DesignWare IPK specific extensions for Synopsys DesignWare
> > MIPI DSI Host driver.
> >
> > Cc: Maarten Lankhorst
> > Cc: Maxime Ripard
> > Cc: David Airlie
> > Cc: Daniel Vetter
> > Cc: Sam Ravnborg
> > Cc: Gustavo Pimentel
> > Cc: Joao Pinto
> > Signed-off-by: Angelo Ribeiro
>
> I've dumped this on a pile of bridge drivers by now, but I don't think the
> dw-mipi-dsi organization makes much sense.
>
> I think what we'd need is:
>
> - drm_encoder is handled by the drm_device driver, not by dw-mipi-dsi
> drm_bridge driver
>
> - the glue code for the various soc specific implementations (like ipk
> here) should be put behind the drm_bridge abstraction. Otherwise I'm not
> really seeing why exactly dw-mipi-dsi is a bridge driver if it doesn't
> work like a bridge driver
>
> - Probably we should put all these files into drm/bridge/dw-mipi-dsi/
>
> - drm_device drivers should get at their bridges with one of the standard
> of helpers we have in drm_bridge, not by directly calling into a bridge
> drivers.
>
> I know that dw-hdmi is using the exact same code pattern, but we got to
> stop this eventually or it becomes an unfixable mess.
> -Daniel
Hi Daniel,
Sorry for the late answer.
I understand what you stated and the conversion of
this driver in a help library could be a good solution since
you can use the DSI as bridge or as encoder, as your pipeline
requires.
Also most of the code implemented by each glue is essential PHY related,
the development of a PHY driver could make this more clear.
However, this needs a lot of work and consensus. Do you think that we
can go ahead with this driver and do the rework later?
I'm available and interested to help on this rework.
Thanks,
Angelo
>
> > ---
> > Changes since v3:
> > - Rearranged headers.
> > ---
> > drivers/gpu/drm/ipk/Kconfig | 9 +
> > drivers/gpu/drm/ipk/Makefile | 2 +
> > drivers/gpu/drm/ipk/dw-mipi-dsi-ipk.c | 557
> > ++
> > 3 files changed, 568 insertions(+)
> > create mode 100644 drivers/gpu/drm/ipk/dw-mipi-dsi-ipk.c
> >
> > diff --git a/drivers/gpu/drm/ipk/Kconfig b/drivers/gpu/drm/ipk/Kconfig
> > index 1f87444..49819e5 100644
> > --- a/drivers/gpu/drm/ipk/Kconfig
> > +++ b/drivers/gpu/drm/ipk/Kconfig
> > @@ -11,3 +11,12 @@ config DRM_IPK
> > Enable support for the Synopsys DesignWare DRM DSI.
> > To compile this driver as a module, choose M here: the module
> > will be called ipk-drm.
> > +
> > +config DRM_IPK_DSI
> > + tristate "Synopsys DesignWare IPK specific extensions for MIPI DSI"
> > + depends on DRM_IPK
> > + select DRM_DW_MIPI_DSI
> > + help
> > + Choose this option for Synopsys DesignWare IPK MIPI DSI support.
> > + To compile this driver as a module, choose M here: the module
> > + will be called dw-mipi-dsi-ipk.
> > diff --git a/drivers/gpu/drm/ipk/Makefile b/drivers/gpu/drm/ipk/Makefile
> > index 6a1a911..f22d590 100644
> > --- a/drivers/gpu/drm/ipk/Makefile
> > +++ b/drivers/gpu/drm/ipk/Makefile
> > @@ -2,3 +2,5 @@
> > ipk-drm-y := dw-drv.o dw-vpg.o
> >
> > obj-$(CONFIG_DRM_IPK) += ipk-drm.o
> > +
> > +obj-$(CONFIG_DRM_IPK_DSI) += dw-mipi-dsi-ipk.o
> > diff --git a/drivers/gpu/drm/ipk/dw-mipi-dsi-ipk.c
> > b/drivers/gpu/drm/ipk/dw-mipi-dsi-ipk.c
> > new file mode 100644
> > index 000..f8ac4ca
> > --- /dev/null
> > +++ b/drivers/gpu/drm/ipk/dw-mipi-dsi-ipk.c
> > @@ -0,0 +1,557 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2019-2020 Synopsys, Inc. and/or its affiliates.
> > + * Synopsys DesignWare MIPI DSI solution driver
> > + *
> > + * Author: Angelo Ribeiro
> > + * Author: Luis Oliveira
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define DW_DPHY_LPCLK_CTRL 0x94
> > +#define DW_DPHY_RSTZ 0xA0
> > +#define DW_DPHY_IF_CFG 0xA4
> > +#define DW_DPHY_ULPS_CTRL 0xA8
> > +#define DW_DPHY_TX_TRIGGERS0xAC
> > +#define DW_DPHY_STATUS 0xB0
> > +#define DW_DPHY_TST_CTRL0 0xB4
> > +#define DW_DPHY_TST_CTRL1 0xB8
> > +#define DW_GEN3_IF_TESTER 0x3c
> > +#define DW_GEN3_IF_SOC_PLL 0x48
> > +#define DW_GEN3_IF_SOC_PLL_EN 0x4C
> > +
> > +#define DW_12BITS_DPHY_RDY_L0 0x507
> > +#define DW_12BITS_DPHY_RDY_L1 0x707
> > +#define DW_12BITS_DPHY_RDY_L2 0x907
> > +#define DW_12BITS_DPHY_RDY_L3 0xB07
> > +
> > +#define DW_LANE_MIN_KBPS 8
> > +#define DW_LANE_MAX_KBPS 25
> > +#define DW_DPHY_DIV_UPPER_LIMIT8000
> > +#define DW_DPHY_DIV_LOWER_LIMIT2000
> > +#define DW_MIN_OUTPUT_FREQ 80
> > +#define DW_LPHS_TIM_TRANSIONS 0x40
> > +
> > +enum dw_glueiftester {
> > + GLUE_LOGIC = 0x4,
> > +
Re: [PATCH v3 3/4] drm: ipk: Add extensions for DW MIPI DSI Host driver
On Wed, May 06, 2020 at 09:56:16AM +, Angelo Ribeiro wrote: > From: Daniel Vetter > Date: Tue, Apr 28, 2020 at 16:28:15 > > > On Mon, Apr 27, 2020 at 04:00:35PM +0200, Angelo Ribeiro wrote: > > > Add Synopsys DesignWare IPK specific extensions for Synopsys DesignWare > > > MIPI DSI Host driver. > > > > > > Cc: Maarten Lankhorst > > > Cc: Maxime Ripard > > > Cc: David Airlie > > > Cc: Daniel Vetter > > > Cc: Sam Ravnborg > > > Cc: Gustavo Pimentel > > > Cc: Joao Pinto > > > Signed-off-by: Angelo Ribeiro > > > > I've dumped this on a pile of bridge drivers by now, but I don't think the > > dw-mipi-dsi organization makes much sense. > > > > I think what we'd need is: > > > > - drm_encoder is handled by the drm_device driver, not by dw-mipi-dsi > > drm_bridge driver > > > > - the glue code for the various soc specific implementations (like ipk > > here) should be put behind the drm_bridge abstraction. Otherwise I'm not > > really seeing why exactly dw-mipi-dsi is a bridge driver if it doesn't > > work like a bridge driver > > > > - Probably we should put all these files into drm/bridge/dw-mipi-dsi/ > > > > - drm_device drivers should get at their bridges with one of the standard > > of helpers we have in drm_bridge, not by directly calling into a bridge > > drivers. > > > > I know that dw-hdmi is using the exact same code pattern, but we got to > > stop this eventually or it becomes an unfixable mess. > > -Daniel > > Hi Daniel, > > Sorry for the late answer. > > I understand what you stated and the conversion of > this driver in a help library could be a good solution since > you can use the DSI as bridge or as encoder, as your pipeline > requires. > > Also most of the code implemented by each glue is essential PHY related, > the development of a PHY driver could make this more clear. > > However, this needs a lot of work and consensus. Do you think that we > can go ahead with this driver and do the rework later? > I'm available and interested to help on this rework. There's a bunch of these in the works right now, so minimally we need to make sure that we do actually have consensus among all stakeholders (all the existing and new drivers plus people working on dw-mipi-dsi drivers). E.g. I'm not clear whether a helper library is a good interface for drm_device drivers, that really should be doable as a standard drm_bridge with no drm_encoder. I think the best way to ensure that consensus is by adding a todo entry to Documentation/gpu/todo.rst for both dw-hdmi and dw-mipi-dsi (same design really), with acks from everyone. Once we have agreement and on how to best get there I'm all happy. -Daniel > > Thanks, > Angelo > > > > > > --- > > > Changes since v3: > > > - Rearranged headers. > > > --- > > > drivers/gpu/drm/ipk/Kconfig | 9 + > > > drivers/gpu/drm/ipk/Makefile | 2 + > > > drivers/gpu/drm/ipk/dw-mipi-dsi-ipk.c | 557 > > > ++ > > > 3 files changed, 568 insertions(+) > > > create mode 100644 drivers/gpu/drm/ipk/dw-mipi-dsi-ipk.c > > > > > > diff --git a/drivers/gpu/drm/ipk/Kconfig b/drivers/gpu/drm/ipk/Kconfig > > > index 1f87444..49819e5 100644 > > > --- a/drivers/gpu/drm/ipk/Kconfig > > > +++ b/drivers/gpu/drm/ipk/Kconfig > > > @@ -11,3 +11,12 @@ config DRM_IPK > > > Enable support for the Synopsys DesignWare DRM DSI. > > > To compile this driver as a module, choose M here: the module > > > will be called ipk-drm. > > > + > > > +config DRM_IPK_DSI > > > + tristate "Synopsys DesignWare IPK specific extensions for MIPI DSI" > > > + depends on DRM_IPK > > > + select DRM_DW_MIPI_DSI > > > + help > > > + Choose this option for Synopsys DesignWare IPK MIPI DSI support. > > > + To compile this driver as a module, choose M here: the module > > > + will be called dw-mipi-dsi-ipk. > > > diff --git a/drivers/gpu/drm/ipk/Makefile b/drivers/gpu/drm/ipk/Makefile > > > index 6a1a911..f22d590 100644 > > > --- a/drivers/gpu/drm/ipk/Makefile > > > +++ b/drivers/gpu/drm/ipk/Makefile > > > @@ -2,3 +2,5 @@ > > > ipk-drm-y := dw-drv.o dw-vpg.o > > > > > > obj-$(CONFIG_DRM_IPK) += ipk-drm.o > > > + > > > +obj-$(CONFIG_DRM_IPK_DSI) += dw-mipi-dsi-ipk.o > > > diff --git a/drivers/gpu/drm/ipk/dw-mipi-dsi-ipk.c > > > b/drivers/gpu/drm/ipk/dw-mipi-dsi-ipk.c > > > new file mode 100644 > > > index 000..f8ac4ca > > > --- /dev/null > > > +++ b/drivers/gpu/drm/ipk/dw-mipi-dsi-ipk.c > > > @@ -0,0 +1,557 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * Copyright (c) 2019-2020 Synopsys, Inc. and/or its affiliates. > > > + * Synopsys DesignWare MIPI DSI solution driver > > > + * > > > + * Author: Angelo Ribeiro > > > + * Author: Luis Oliveira > > > + */ > > > + > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > + > > > +#include > > > + > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > + > > > +#define
Re: [PATCH v3 3/4] drm: ipk: Add extensions for DW MIPI DSI Host driver
On Mon, Apr 27, 2020 at 04:00:35PM +0200, Angelo Ribeiro wrote:
> Add Synopsys DesignWare IPK specific extensions for Synopsys DesignWare
> MIPI DSI Host driver.
>
> Cc: Maarten Lankhorst
> Cc: Maxime Ripard
> Cc: David Airlie
> Cc: Daniel Vetter
> Cc: Sam Ravnborg
> Cc: Gustavo Pimentel
> Cc: Joao Pinto
> Signed-off-by: Angelo Ribeiro
I've dumped this on a pile of bridge drivers by now, but I don't think the
dw-mipi-dsi organization makes much sense.
I think what we'd need is:
- drm_encoder is handled by the drm_device driver, not by dw-mipi-dsi
drm_bridge driver
- the glue code for the various soc specific implementations (like ipk
here) should be put behind the drm_bridge abstraction. Otherwise I'm not
really seeing why exactly dw-mipi-dsi is a bridge driver if it doesn't
work like a bridge driver
- Probably we should put all these files into drm/bridge/dw-mipi-dsi/
- drm_device drivers should get at their bridges with one of the standard
of helpers we have in drm_bridge, not by directly calling into a bridge
drivers.
I know that dw-hdmi is using the exact same code pattern, but we got to
stop this eventually or it becomes an unfixable mess.
-Daniel
> ---
> Changes since v3:
> - Rearranged headers.
> ---
> drivers/gpu/drm/ipk/Kconfig | 9 +
> drivers/gpu/drm/ipk/Makefile | 2 +
> drivers/gpu/drm/ipk/dw-mipi-dsi-ipk.c | 557
> ++
> 3 files changed, 568 insertions(+)
> create mode 100644 drivers/gpu/drm/ipk/dw-mipi-dsi-ipk.c
>
> diff --git a/drivers/gpu/drm/ipk/Kconfig b/drivers/gpu/drm/ipk/Kconfig
> index 1f87444..49819e5 100644
> --- a/drivers/gpu/drm/ipk/Kconfig
> +++ b/drivers/gpu/drm/ipk/Kconfig
> @@ -11,3 +11,12 @@ config DRM_IPK
> Enable support for the Synopsys DesignWare DRM DSI.
> To compile this driver as a module, choose M here: the module
> will be called ipk-drm.
> +
> +config DRM_IPK_DSI
> + tristate "Synopsys DesignWare IPK specific extensions for MIPI DSI"
> + depends on DRM_IPK
> + select DRM_DW_MIPI_DSI
> + help
> + Choose this option for Synopsys DesignWare IPK MIPI DSI support.
> + To compile this driver as a module, choose M here: the module
> + will be called dw-mipi-dsi-ipk.
> diff --git a/drivers/gpu/drm/ipk/Makefile b/drivers/gpu/drm/ipk/Makefile
> index 6a1a911..f22d590 100644
> --- a/drivers/gpu/drm/ipk/Makefile
> +++ b/drivers/gpu/drm/ipk/Makefile
> @@ -2,3 +2,5 @@
> ipk-drm-y := dw-drv.o dw-vpg.o
>
> obj-$(CONFIG_DRM_IPK) += ipk-drm.o
> +
> +obj-$(CONFIG_DRM_IPK_DSI) += dw-mipi-dsi-ipk.o
> diff --git a/drivers/gpu/drm/ipk/dw-mipi-dsi-ipk.c
> b/drivers/gpu/drm/ipk/dw-mipi-dsi-ipk.c
> new file mode 100644
> index 000..f8ac4ca
> --- /dev/null
> +++ b/drivers/gpu/drm/ipk/dw-mipi-dsi-ipk.c
> @@ -0,0 +1,557 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019-2020 Synopsys, Inc. and/or its affiliates.
> + * Synopsys DesignWare MIPI DSI solution driver
> + *
> + * Author: Angelo Ribeiro
> + * Author: Luis Oliveira
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define DW_DPHY_LPCLK_CTRL 0x94
> +#define DW_DPHY_RSTZ 0xA0
> +#define DW_DPHY_IF_CFG 0xA4
> +#define DW_DPHY_ULPS_CTRL0xA8
> +#define DW_DPHY_TX_TRIGGERS 0xAC
> +#define DW_DPHY_STATUS 0xB0
> +#define DW_DPHY_TST_CTRL00xB4
> +#define DW_DPHY_TST_CTRL10xB8
> +#define DW_GEN3_IF_TESTER0x3c
> +#define DW_GEN3_IF_SOC_PLL 0x48
> +#define DW_GEN3_IF_SOC_PLL_EN0x4C
> +
> +#define DW_12BITS_DPHY_RDY_L00x507
> +#define DW_12BITS_DPHY_RDY_L10x707
> +#define DW_12BITS_DPHY_RDY_L20x907
> +#define DW_12BITS_DPHY_RDY_L30xB07
> +
> +#define DW_LANE_MIN_KBPS 8
> +#define DW_LANE_MAX_KBPS 25
> +#define DW_DPHY_DIV_UPPER_LIMIT 8000
> +#define DW_DPHY_DIV_LOWER_LIMIT 2000
> +#define DW_MIN_OUTPUT_FREQ 80
> +#define DW_LPHS_TIM_TRANSIONS0x40
> +
> +enum dw_glueiftester {
> + GLUE_LOGIC = 0x4,
> + RX_PHY = 0x2,
> + TX_PHY = 0x1,
> + RESET = 0x0,
> +};
> +
> +struct dw_range_dphy {
> + u32 freq;
> + u8 hs_freq_range;
> + u32 osc_freq_target;
> +} dw_range_gen3[] = {
> + { 80, 0x00, 0x3f }, { 90, 0x10, 0x3f }, { 100, 0x20, 0x3f },
> + { 110, 0x30, 0x39 }, { 120, 0x01, 0x39 }, { 130, 0x11, 0x39 },
> + { 140, 0x21, 0x39 }, { 150, 0x31, 0x39 }, { 160, 0x02, 0x39 },
> + { 170, 0x12, 0x2f }, { 180, 0x22, 0x2f }, { 190, 0x32, 0x2f },
> + { 205, 0x03, 0x2f }, { 220, 0x13, 0x29 }, { 235, 0x23, 0x29 },
> + { 250, 0x33, 0x29 }, { 275, 0x04, 0x29 }, { 300, 0x14, 0x29 },
> + { 325, 0x25, 0x29 }, { 350, 0x35, 0x1f }, { 400, 0x05, 0x1f },
> + { 450, 0x16, 0x19 }, { 500, 0x26, 0x19 }, { 550, 0x37, 0x19 },
> + { 600, 0x07, 0x19 }, { 650, 0x18, 0x19 }, { 700, 0x28, 0x0f
