[PATCH 9/9] drm/bridge: imx: Add i.MX93 MIPI DSI support

[Jagan Teki]

On Wed, Jul 19, 2023 at 2:05 PM Ying Liu <victor.liu at nxp.com> wrote:
>
> On Tuesday, July 18, 2023 6:51 PM Jagan Teki <jagan at amarulasolutions.com> wrote:
> >
> > Hi,
>
> Hi,
>
> >
> > On Tue, Jul 18, 2023 at 3:19 PM Ying Liu <victor.liu at nxp.com> wrote:
> > >
> > > On Tuesday, July 18, 2023 5:35 PM Jagan Teki
> > <jagan at amarulasolutions.com> wrote:
> > > >
> > > > >
> > > > > Hi Jagan,
> > > > >
> > > > > On Monday, July 17, 2023 2:44 PM Jagan Teki
> > > > <jagan at amarulasolutions.com> wrote:
> > > > > > On Mon, Jul 17, 2023 at 11:44 AM Liu Ying <victor.liu at nxp.com>
> > wrote:
> > > > > > >
> > > > > > > Freescale i.MX93 SoC embeds a Synopsys Designware MIPI DSI host
> > > > > > > controller and a Synopsys Designware MIPI DPHY.  Some
> > configurations
> > > > > > > and extensions to them are controlled by i.MX93 media blk-ctrl.
> > > > > > >
> > > > > > > Add a DRM bridge for i.MX93 MIPI DSI by using existing DW MIPI DSI
> > > > > > > bridge helpers and implementing i.MX93 MIPI DSI specific
> > extensions.
> > > > > >
> > > > > > I think the better way would add compatibility to be part of existing
> > > > > > dw-mipi-dsi.c with specific driver data. This way it avoids all the
> > > > > > platform-related helpers(extensions) and makes the driver generic to
> > > > > > all SoCs which use DW DSI IP. It would be a straightforward change as
> > > > > > the imx93 drm pipeline already supports bridge topology.
> > > > >
> > > > > The platform-related stuff is handed over to dw-mipi-dsi.c via struct
> > > > > dw_mipi_dsi_plat_data as an argument of dw_mipi_dsi_probe().  It
> > looks
> > > > > ok for vendor drivers to call dw_mipi_dsi_probe() to set the platform-
> > > > related
> > > > > information(rockchip, meson and stm do that), like pdata.phy_ops and
> > > > > pdata.host_ops.
> > > >
> > > > I understand this topology of having soc-platform drivers with
> > > > dw-mipi-dsi bridge. What I'm suggesting is to not add a soc-platform
> > > > driver for imx93 instead add add support directly on dw-mipi-dsi
> > > > bridge.
> > >
> > > It seems that directly supporting i.MX93 MIPI DSI in dw-mipi-dsi.c is
> > > not feasible.  The main reason is that struct dw_mipi_dsi_plat_data
> > > contains phy_ops and each vendor driver provides very different
> > > methods for phy, while...
> >
> > Cannot this phy_ops goes to PHY core somewhere around and even it is
> > possible to add via driver data for imx93 by untouching existing
> > handling? I know it is not as direct as I describe but it is worth
> > maintaining the driver this way to keep control of the future
> > soc-drivers to include in dw-mipi-dsi instead of handling platform
> > code separately.
>
> Like I said, each vendor driver provides very different methods for phy.
> The reason is that phy IPs are different and SoC integration things are
> handled via struct dw_mipi_dsi_phy_ops.  Meson calls devm_phy_get()
> to get a phy.  Rockchip calls devm_phy_create() to create a phy.  Meson,
> rockchip and stm have their own struct dw_mipi_dsi_phy_ops
> implementations, same to i.MX93.  Different pixel format control is
> implemented in meson's and i.MX93's ->init() callback. Meson additionally
> handles clocks in ->init() callback.
>
> Generally speaking, it's a no-go to put phy_ops into PHY core for all the
> vendors, IMHO.
>
> In particular, i.MX93 MIPI DPHY's PLL can be fully controlled by media
> blk-ctrl(as a syscon) thru the PLL's SoC control interface, while PHY
> registers can be controlled by DW MIPI DSI testdin/out control interface
> alternatively including a part of the PLL registers.  So, adding a PHY
> driver for i.MX93 MIPI DPHY doesn't make sense since the PLL controlled
> by media blk-ctrl doesn't constitute a PHY.  Instead, dw_mipi_dsi_phy_ops
> may cover all the PHY control well.
>
> From my PoV, w/wo this series, dw-mipi-dsi.c looks fine to keep being
> generic and easy to maintain.  All vendor drivers do is to handle platform
> specific stuff, which is good.

Thanks for the explanation. I agreed with on you most of the points
from the perspective of PoV which are difficult to FIT into a common
bridge. However, I'm still believing that the having bridge does only
the bridge job, and keeping the PHY or other PoV-specific stuff on
respective driver subsystems would be great synergy for the bridge
drivers or any common IP driver's point-of-view.

Anyway, I might not control i.MX93 PoV stuff, but I can try on it for
the new DSI which uses this IP, and keep you in CC if it is a possible
land in the near future.

>
> >
> > >
> > > >
> > > > >
> > > > > dw-mipi-dsi.c is generic w/wo this patch series.
> > > > >
> > > > > Can you elaborate more about adding compatibility to be part of
> > existing
> > > > > dw-mipi-dsi.c with specific driver data?  I don't see clear approach to do
> > > > > that.
> > > >
> > > > Please check the above comments, an example of samsung-dsim.c
> > >
> > > ... it seems that samsung-dsim.c uses struct samsung_dsim_driver_data to
> > > differential platform information and it doesn't contain any callback, which
> > > means comparing to DW MIPI DSI, Samsung DSIM shows more consistency
> > > across vendor SoCs from HW IP/SoC integration PoV.
> >
> > Yes, but is it possible to adjust struct according to DW MIPI DSI.
>
> Looking at the vendor drivers, they show a lot diversity, which cannot be
> parameterized into a struct like the samsung dsim driver does.
>
> struct dw_mipi_dsi_plat_data hides all platform specific information from
> dw-mipi-dsi.c well, IMHO.

Okay.

Thanks,
Jagan.