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.
> 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.
> 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
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.
Note that at least cdns-dsi, exynos4-is
(drivers/media/platform/exynos4-is/mipi-csis.c), kirin, sun4i, msm,
mtk, omap4iss, plus the v4l2 drivers cdns-csi2tx and cdns-csi2rx I
want to convert, have already either a driver for their DPHY using the
phy framework plus a configuration function, or a design very similar
that could be migrated to such an API.
> > Obviously, the consumer driver shouldn't care about the phy
> > integration details, especially since some of those consumer drivers
> > need to interact with multiple phy designs (or the same phy design can
> > be used by multiple consumers).
> >
> > So knowing that a feature is supported is really not enough.
> >
> > With MIPI-DPHY at least, the API is generic enough so that another
> > mode where the features would make sense could implement a feature
> > flag if that makes sense.
> >
> >>> For example, DRM requires this to filter out display modes (ie,
> >>> resolutions) that wouldn't be achievable by the PHY so that it's never
> >>
> >> Can't the consumer driver just tell the required resolution to the PHY and PHY
> >> figuring out all the parameters for the resolution or an error if that
> >> resolution cannot be supported?
> >
> > Not really either. With MIPI D-PHY, the phy is fed a clock that is
> > generated by the phy consumer, which might or might not be an exact
> > fit for the resolution. There's so many resolutions that in most case,
> > the clock factors don't allow you to have a perfect match. And
> > obviously, this imprecision should be taken into account by the PHY as
> > well.
> >
> > And then, there's also the matter than due to design constraints, some
> > consumers would have fixed timings that are not at the spec default
> > value, but still within the acceptable range. We need to communicate
> > that to the PHY.
>
> Here do you mean videomode timings?
No, I mean the DPHY timings.
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
