lkml.org 
[lkml]   [2018]   [Sep]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 02/10] phy: Add configuration interface
From
Date
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

\
 
 \ /
  Last update: 2018-09-24 10:50    [W:0.070 / U:15.684 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site