On Tuesday, July 18, 2023 6:51 PM Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx> wrote: > > Hi, Hi, > > On Tue, Jul 18, 2023 at 3:19 PM Ying Liu <victor.liu@xxxxxxx> wrote: > > > > On Tuesday, July 18, 2023 5:35 PM Jagan Teki > <jagan@xxxxxxxxxxxxxxxxxxxx> wrote: > > > > > > > > > > > Hi Jagan, > > > > > > > > On Monday, July 17, 2023 2:44 PM Jagan Teki > > > <jagan@xxxxxxxxxxxxxxxxxxxx> wrote: > > > > > On Mon, Jul 17, 2023 at 11:44 AM Liu Ying <victor.liu@xxxxxxx> > 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. > > > > > > > > > > > > > > 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. Regards, Liu Ying > > Thanks > Jagan.
