Hi Maxime, On Wednesday 19 September 2018 05:44 PM, Maxime Ripard wrote: > Hi, > > On Fri, Sep 14, 2018 at 02:18:37PM +0530, Kishon Vijay Abraham I wrote: >>>>>>> +/** >>>>>>> + * phy_validate() - Checks the phy parameters >>>>>>> + * @phy: the phy returned by phy_get() >>>>>>> + * @mode: phy_mode the configuration is applicable to. >>>>>>> + * @opts: Configuration to check >>>>>>> + * >>>>>>> + * Used to check that the current set of parameters can be handled by >>>>>>> + * the phy. Implementations are free to tune the parameters passed as >>>>>>> + * arguments if needed by some implementation detail or >>>>>>> + * constraints. It will not change any actual configuration of the >>>>>>> + * PHY, so calling it as many times as deemed fit will have no side >>>>>>> + * effect. >>>>>>> + * >>>>>>> + * Returns: 0 if successful, an negative error code otherwise >>>>>>> + */ >>>>>>> +int phy_validate(struct phy *phy, enum phy_mode mode, >>>>>>> + union phy_configure_opts *opts) >>>>>> >>>>>> IIUC the consumer driver will pass configuration options (or PHY parameters) >>>>>> which will be validated by the PHY driver and in some cases the PHY driver can >>>>>> modify the configuration options? And these modified configuration options will >>>>>> again be given to phy_configure? >>>>>> >>>>>> Looks like it's a round about way of doing the same thing. >>>>> >>>>> Not really. The validate callback allows to check whether a particular >>>>> configuration would work, and try to negotiate a set of configurations >>>>> that both the consumer and the PHY could work with. >>>> >>>> Maybe the PHY should provide the list of supported features to the consumer >>>> driver and the consumer should select a supported feature? >>> >>> It's not really about the features it supports, but the boundaries it >>> might have on those features. For example, the same phy integrated in >>> two different SoCs will probably have some limit on the clock rate it >>> can output because of the phy design itself, but also because of the >>> clock that is fed into that phy, and that will be different from one >>> SoC to the other. >>> >>> This integration will prevent us to use some clock rates on the first >>> SoC, while the second one would be totally fine with it. >> >> If there's a clock that is fed to the PHY from the consumer, then the consumer >> driver should model a clock provider and the PHY can get a reference to it >> using clk_get(). Rockchip and Arasan eMMC PHYs has already used something like >> that. > > That would be doable, but no current driver has had this in their > binding. So that would prevent any further rework, and make that whole > series moot. And while I could live without the Allwinner part, the > Cadence one is really needed. We could add a binding and modify the driver to to register a clock provider. That could be included in this series itself. > >> Assuming the PHY can get a reference to the clock provided by the consumer, >> what are the parameters we'll be able to get rid of in struct >> phy_configure_opts_mipi_dphy? > > hs_clock_rate and lp_clock_rate. All the other ones are needed. For a start we could use that and get rid of hs_clock_rate and lp_clock_rate in phy_configure_opts_mipi_dphy. We could also use phy_set_bus_width() for lanes. > >> I'm sorry but I'm not convinced a consumer driver should have all the details >> that are added in phy_configure_opts_mipi_dphy. > > If it can convince you, here is the parameters that are needed by all > the MIPI-DSI drivers currently in Linux to configure their PHY: > > - cdns-dsi (drivers/gpu/drm/bridge/cdns-dsi.c) > - hs_clk_rate > - lanes > - videomode > > - kirin (drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c) > - hs_exit > - hs_prepare > - hs_trail > - hs_zero > - lpx > - ta_get > - ta_go > - wakeup > > - msm (drivers/gpu/drm/msm/dsi/*) > - clk_post > - clk_pre > - clk_prepare > - clk_trail > - clk_zero > - hs_clk_rate > - hs_exit > - hs_prepare > - hs_trail > - hs_zero > - lp_clk_rate > - ta_get > - ta_go > - ta_sure > > - mtk (drivers/gpu/drm/mediatek/mtk_dsi.c) > - hs_clk_rate > - hs_exit > - hs_prepare > - hs_trail > - hs_zero > - lpx > > - sun4i (drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c) > - clk_post > - clk_pre > - clk_prepare > - clk_zero > - hs_prepare > - hs_trail > - lanes > - lp_clk_rate > > - tegra (drivers/gpu/drm/tegra/dsi.c) > - clk_post > - clk_pre > - clk_prepare > - clk_trail > - clk_zero > - hs_exit > - hs_prepare > - hs_trail > - hs_zero > - lpx > - ta_get > - ta_go > - ta_sure > > - vc4 (drivers/gpu/drm/vc4/vc4_dsi.c) > - hs_clk_rate > - lanes > > Now, for MIPI-CSI receivers: > > - marvell-ccic (drivers/media/platform/marvell-ccic/mcam-core.c) > - clk_term_en > - clk_settle > - d_term_en > - hs_settle > - lp_clk_rate > > - omap4iss (drivers/staging/media/omap4iss/iss_csiphy.c) > - clk_miss > - clk_settle > - clk_term > - hs_settle > - hs_term > - lanes > > - rcar-vin (drivers/media/platform/rcar-vin/rcar-csi2.c) > - hs_clk_rate > - lanes > > - ti-vpe (drivers/media/platform/ti-vpe/cal.c) > - clk_term_en > - d_term_en > - hs_settle > - hs_term Thank you for providing the exhaustive list. > > So the timings expressed in the structure are the set of all the ones > currently used in the tree by DSI and CSI drivers. I would consider > that a good proof that it would be useful. The problem I see here is each platform (PHY) will have it's own set of parameters and we have to keep adding members to phy_configure_opts which is not scalable. We should try to find a correlation between generic PHY modes and these parameters (at-least for a subset). Thanks Kishon
