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

[Kishon Vijay Abraham I]

Hi,

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?
> 
> 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)
> 
>>> + * @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.
> 
>> Can't the helper functions be directly invoked by the PHY driver for
>> the configuration.
> 
> Not really. The helpers are here to introduce functions that give you
> the defaults provided by the spec for a given configuration, and to
> validate that a given configuration is within the spec boundaries. I
> expect some consumers to need to change the defaults for some more
> suited parameters that are still within the boundaries defined by the
> spec.
> 
> And I'd really want to have that interface being quite generic, and
> applicable to other phy modes as well. The allwinner USB PHY for
> example require at the moment an extra function that could be moved to
> this API:
> https://elixir.bootlin.com/linux/latest/source/drivers/phy/allwinner/phy-sun4i-usb.c#L512
> 
>>> + *
>>> + * Used to change the PHY parameters. phy_init() must have
>>> + * been called on the phy.
>>> + *
>>> + * Returns: 0 if successful, an negative error code otherwise
>>> + */
>>> +int phy_configure(struct phy *phy, enum phy_mode mode,
>>> +		  union phy_configure_opts *opts)
>>> +{> +	int ret;
>>> +
>>> +	if (!phy)
>>> +		return -EINVAL;
>>> +
>>> +	if (!phy->ops->configure)
>>> +		return 0;
>>> +
>>> +	mutex_lock(&phy->mutex);
>>> +	ret = phy->ops->configure(phy, mode, opts);
>>> +	mutex_unlock(&phy->mutex);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +/**
>>> + * 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?

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

Thanks
Kishon