Hi Laurent and all,
Thanks for the feedback.
> Subject: Re: [PATCH v6 2/2] drm: rcar-du: Add RZ/G2L DSI driver
>
> Hi Biju,
>
> On Tue, Aug 30, 2022 at 08:22:08AM +0000, Biju Das wrote:
> > Subject: Re: [PATCH v6 2/2] drm: rcar-du: Add RZ/G2L DSI driver
> > > On Mon, Aug 29, 2022 at 10:19:01AM +0100, Biju Das wrote:
> > > > This driver supports the MIPI DSI encoder found in the RZ/G2L SoC.
> > > > It currently supports DSI video mode only.
> > > >
> > > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > > > Acked-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
> > > > ---
> > > > v5->v6:
> > > > * Updated commit description
> > > > * Moved handling of arst and prst from rzg2l_mipi_dsi_startup-
> > > >runtime
> > > > PM suspend/resume handlers.
> > > > * Max lane capability read at probe(), and enforced in
> > > > rzg2l_mipi_dsi_host_attach()
> > > > * Simplified vich1ppsetr setting.
> > > > * Renamed hsclk_running_mode,hsclk_mode->is_clk_cont.
> > > > * Fixed typo in probe error message(arst->rst).
> > > > * Reordered DRM bridge initaization in probe()
> > > > * Updated typo in e-mail address.
> > > > v4->v5:
> > > > * Added Ack from Sam.
> > > > * Added a trivial change, replaced rzg2l_mipi_dsi_parse_dt()
> > > > with drm_of_get_data_lanes_count_ep() in probe.
> > > > v3->v4:
> > > > * Updated error handling in rzg2l_mipi_dsi_startup() and
> > > > rzg2l_mipi_dsi_atomic_enable()
> > > > v2->v3:
> > > > * pass rzg2l_mipi_dsi pointer to {Link,Phy} register rd/wr
> > > > function
> > > instead
> > > > of the memory pointer
> > > > * Fixed the comment in rzg2l_mipi_dsi_startup()
> > > > * Removed unnecessary dbg message from
> > > > rzg2l_mipi_dsi_start_video()
> > > > * DRM bridge parameter initialization moved to probe
> > > > * Replaced dev_dbg->dev_err in rzg2l_mipi_dsi_parse_dt()
> > > > * Inserted the missing blank lane after return in probe()
> > > > * Added missing MODULE_DEVICE_TABLE
> > > > * Added include linux/bits.h in header file
> > > > * Fixed various macros in header file.
> > > > * Reorder the make file for DSI, so that it is no more dependent
> > > > on RZ/G2L DU patch series.
> > > > v1->v2:
> > > > * Rework based on dt-binding change (DSI + D-PHY) as single block
> > > > * Replaced link_mmio and phy_mmio with mmio in struct
> > > > rzg2l_mipi_dsi
> > > > * Replaced rzg2l_mipi_phy_write with rzg2l_mipi_dsi_phy_write
> > > > and rzg2l_mipi_dsi_link_write
> > > > * Replaced rzg2l_mipi_phy_read->rzg2l_mipi_dsi_link_read
> > > > RFC->v1:
> > > > * Added "depends on ARCH_RENESAS || COMPILE_TEST" on KCONFIG
> > > > and dropped DRM as it is implied by DRM_BRIDGE
> > > > * Used devm_reset_control_get_exclusive() for reset handle
> > > > * Removed bool hsclkmode from struct rzg2l_mipi_dsi
> > > > * Added error check for pm, using pm_runtime_resume_and_get()
> > > > instead
> > > of
> > > > pm_runtime_get_sync()
> > > > * Added check for unsupported formats in
> > > > rzg2l_mipi_dsi_host_attach()
> > > > * Avoided read-modify-write stopping hsclock
> > > > * Used devm_platform_ioremap_resource for resource allocation
> > > > * Removed unnecessary assert call from probe and remove.
> > > > * wrap the line after the PTR_ERR() in probe()
> > > > * Updated reset failure messages in probe
> > > > * Fixed the typo arstc->prstc
> > > > * Made hex constants to lower case.
> > > > RFC:
> > > > *
> > > > ---
> > > > drivers/gpu/drm/rcar-du/Kconfig | 8 +
> > > > drivers/gpu/drm/rcar-du/Makefile | 2 +
> > > > drivers/gpu/drm/rcar-du/rzg2l_mipi_dsi.c | 703
> > > ++++++++++++++++++
> > > > drivers/gpu/drm/rcar-du/rzg2l_mipi_dsi_regs.h | 151 ++++
> > > > 4 files changed, 864 insertions(+) create mode 100644
> > > > drivers/gpu/drm/rcar-du/rzg2l_mipi_dsi.c
> > > > create mode 100644 drivers/gpu/drm/rcar-du/rzg2l_mipi_dsi_regs.h
> > > >
> > > > diff --git a/drivers/gpu/drm/rcar-du/Kconfig
> > > > b/drivers/gpu/drm/rcar-du/Kconfig index c959e8c6be7d..58ffb8c2443b
> > > > 100644
> > > > --- a/drivers/gpu/drm/rcar-du/Kconfig
> > > > +++ b/drivers/gpu/drm/rcar-du/Kconfig
> > > > @@ -51,6 +51,14 @@ config DRM_RCAR_MIPI_DSI
> > > > help
> > > > Enable support for the R-Car Display Unit embedded MIPI
> DSI
> > > encoders.
> > > >
> > > > +config DRM_RZG2L_MIPI_DSI
> > > > + tristate "RZ/G2L MIPI DSI Encoder Support"
> > > > + depends on DRM_BRIDGE && OF
> > > > + depends on ARCH_RENESAS || COMPILE_TEST
> > > > + select DRM_MIPI_DSI
> > > > + help
> > > > + Enable support for the RZ/G2L Display Unit embedded MIPI
> DSI
> > > encoders.
> > > > +
> > > > config DRM_RCAR_VSP
> > > > bool "R-Car DU VSP Compositor Support" if ARM
> > > > default y if ARM64
> > > > diff --git a/drivers/gpu/drm/rcar-du/Makefile
> > > > b/drivers/gpu/drm/rcar-du/Makefile
> > > > index 6f132325c8b7..b8f2c82651d9 100644
> > > > --- a/drivers/gpu/drm/rcar-du/Makefile
> > > > +++ b/drivers/gpu/drm/rcar-du/Makefile
> > > > @@ -14,3 +14,5 @@ obj-$(CONFIG_DRM_RCAR_DU) += rcar-du-
> drm.o
> > > > obj-$(CONFIG_DRM_RCAR_DW_HDMI) += rcar_dw_hdmi.o
> > > > obj-$(CONFIG_DRM_RCAR_LVDS) += rcar_lvds.o
> > > > obj-$(CONFIG_DRM_RCAR_MIPI_DSI) += rcar_mipi_dsi.o
> > > > +
> > > > +obj-$(CONFIG_DRM_RZG2L_MIPI_DSI) += rzg2l_mipi_dsi.o
> > > > diff --git a/drivers/gpu/drm/rcar-du/rzg2l_mipi_dsi.c
> > > > b/drivers/gpu/drm/rcar-du/rzg2l_mipi_dsi.c
> > > > new file mode 100644
> > > > index 000000000000..46a71e39e336
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/rcar-du/rzg2l_mipi_dsi.c
> > > > @@ -0,0 +1,703 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * RZ/G2L MIPI DSI Encoder Driver
> > > > + *
> > > > + * Copyright (C) 2022 Renesas Electronics Corporation */
......
> > > This should be goto out_pm_get, and the label should be renamed to
> > > out_pm_disable (or just out_pm). Error labels should be named
> > > according to what cleanup operation they perform, not the error that
> they handle.
> > > There are a few error labels above that should also be renamed
> > > accordingly.
> >
> > Oops. Agreed will fix this in next version.
> >
> > > > +
> > > > + dsi->max_lanes = ((rzg2l_mipi_dsi_link_read(dsi, TXSETR) >>
> 16)
> > > > +&
> > > > +0x3) + 1;
> > >
> > > ... here write
> > >
> > > dsi->max_lanes = min(((rzg2l_mipi_dsi_link_read(dsi, TXSETR) >>
> > > 16) & 3) + 1,
> > > num_data_lanes);
> > >
> > > to drop the num_data_lanes field from the rzg2l_mipi_dsi structure.
> > > You could also use a local variable to store the register value in
> > > order to shorten lines:
> > >
> > > txsetr = rzg2l_mipi_dsi_link_read(dsi, TXSETR);
> > > dsi->max_lanes = min(((txsetr >> 16) & 3) + 1, num_data_lanes);
> > >
> > > Up to you.
> >
> > OK, will fix this in next version.
There are some updates.
1) Our HW team provided new recommended values for the Global Operation Timing Parameter
MIPI DPHY.
The reason is with current settings, if its peer device barely complies
with the min side of the MIPI D-PHY specification, it may not be able to transfer data.
Eg:- Cannot receive SoT when HS transfer starts and may not connect to cameras or displays.
So we need to accommodate this settings in next version of the patch.
2) During testing it was found that the value of read-only register TXSETR.NUMLANECAP
read at probe is not stable and randomly I get a value of 0 or 3.
As per HW manual section 34.4.2.1 Reset
The power on sequence of the DSI-Tx Module is as follows
A) Bring up VDD and VCCQLV18. There are no restrictions on the order to bring up VDD and VCCQLV18.
(B) Set 1 to RE_VCCQLV18_DETVDD.
(C) De-assert preestn and aresetn after after MIPI_DSI_*CLK is stable.
(D) Write 1 to DSIDPHYCTRL0.EN_BGR.
(E) Write 1 to DSIDPHYCTRL0.EN_LDO1200 after waiting for more than 20 usec.
(F) Write DSIDPHYTIMx (x=0 to 3) registers and LINK registers after waiting for more than 10 usec.
(G) De-assert CMN_RSTB.
(H) Wait for more than 1 msec.
(I) The DSI-Tx Module is ready.
Previously ie, when we read TXSETR.NUMLANECAP after H) we get stable values of 3.
Now after moving to probe ie, we read TXSETR.NUMLANECAP after C), we get random values(0 or 3).
So, I asked HW team to provide operational flow for reading TXSETR.NUMLANECAP register.
I don't know how long it will take HW team to provide the answer.
So can we have interim solution?
ie, read TXSETR.NUMLANECAP after H)
and once we get the feedback from HW team, if any change is required, I can send
the follow-up patch, to improve enforcing the lane capability check
during host_attach.
Please let me know your thoughts.
Cheers,
Biju
> >
> > > All those changes are fairly small, once addressed,
> > >
> > > Reviewed-by: Laurent Pinchart
> > > <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > >
