Re: [PATCH 02/10] phy: Add configuration interface

[Kishon Vijay Abraham I]

Hi Maxime,

On Wednesday 12 September 2018 02:12 PM, Maxime Ripard wrote:
> Hi!
> 
> On Wed, Sep 12, 2018 at 01:12:31PM +0530, Kishon Vijay Abraham I wrote:
>> On Thursday 06 September 2018 08:26 PM, Maxime Ripard wrote:
>>> Hi Kishon,
>>>
>>> On Thu, Sep 06, 2018 at 02:57:58PM +0530, Kishon Vijay Abraham I wrote:
>>>> On Wednesday 05 September 2018 02:46 PM, Maxime Ripard wrote:
>>>>> The phy framework is only allowing to configure the power state of the PHY
>>>>> using the init and power_on hooks, and their power_off and exit
>>>>> counterparts.
>>>>>
>>>>> While it works for most, simple, PHYs supported so far, some more advanced
>>>>> PHYs need some configuration depending on runtime parameters. These PHYs
>>>>> have been supported by a number of means already, often by using ad-hoc
>>>>> drivers in their consumer drivers.
>>>>>
>>>>> That doesn't work too well however, when a consumer device needs to deal
>>>>> multiple PHYs, or when multiple consumers need to deal with the same PHY (a
>>>>> DSI driver and a CSI driver for example).
>>>>>
>>>>> So we'll add a new interface, through two funtions, phy_validate and
>>>>> phy_configure. The first one will allow to check that a current
>>>>> configuration, for a given mode, is applicable. It will also allow the PHY
>>>>> driver to tune the settings given as parameters as it sees fit.
>>>>>
>>>>> phy_configure will actually apply that configuration in the phy itself.
>>>>>
>>>>> Signed-off-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxx>
>>>>> ---
>>>>>  drivers/phy/phy-core.c  | 62 ++++++++++++++++++++++++++++++++++++++++++-
>>>>>  include/linux/phy/phy.h | 42 ++++++++++++++++++++++++++++-
>>>>>  2 files changed, 104 insertions(+)
>>>>>
>>>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>>>>> index 35fd38c5a4a1..6eaf655e370f 100644
>>>>> --- a/drivers/phy/phy-core.c
>>>>> +++ b/drivers/phy/phy-core.c
>>>>> @@ -408,6 +408,68 @@ int phy_calibrate(struct phy *phy)
>>>>>  EXPORT_SYMBOL_GPL(phy_calibrate);
>>>>>  
>>>>>  /**
>>>>> + * phy_configure() - Changes the phy parameters
>>>>> + * @phy: the phy returned by phy_get()
>>>>> + * @mode: phy_mode the configuration is applicable to.
>>>>
>>>> mode should be used if the same PHY can be configured in multiple modes. But
>>>> with phy_set_mode() and phy_calibrate() we could achieve the same.
>>>
>>> So you would change the prototype to have a configuration applying
>>> only to the current mode set previously through set_mode?
>>
>> yeah.
>> With phy_configure, if the PHY is not in @mode, it should return an error? Or
>> will it set the PHY to @mode and apply the configuration in @opts?
> 
> I wanted to have it return an error either if it was configured in
> another mode or if the mode was unsupported yes.
> 
>>> Can we have PHY that operate in multiple modes at the same time?
>>
>> Not at the same time. But the same PHY can operate in multiple modes (For
>> example we have PHYs that can be used either with PCIe or USB3)
> 
> Ok, that makes sense. I guess we could rely on phy_set_mode then if
> you prefer.
> 
>>>>> + * @opts: New configuration to apply
>>>>
>>>> Should these configuration come from the consumer driver?
>>>
>>> Yes
>>
>> How does the consumer driver get these configurations? Is it from user space or
>> dt associated with consumer device.
> 
> It really depends on multiple factors (and I guess on what mode the
> PHY is actually supposed to support), but in the case covered by this
> serie, the info mostly come from multiple places:
>   - The resolutions supported by the panel
>   - The resolutions supported by the phy consumer (and its
>     integration, for things like the clock rates it can output)
>   - The resolutions and timings supported by the phy itself (once
>     again, the integration is mostly involved here since it really
>     only depends on which clock rates can be achieved)
>   - The timings boundaries that the specification has
>   - The resolution selected by the user
> 
> So we'd have that information coming from multiple places: the
> userspace would select the resolution, drivers would be able to filter
> out unsupported resolutions, and the DT will provide the integration
> details to help them do so.
> 
> But I guess from an API standpoint, it really is expected to be
> assembled by the phy consumer driver.
> 
>>>>> +/**
>>>>> + * 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.

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?

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.
> 
> 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?

Thanks
Kishon