[02/10] phy: Add configuration interface

Commit Message

Maxime Ripard Sept. 5, 2018, 9:16 a.m. UTC

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 <[e-mail]>
---
 drivers/phy/phy-core.c  | 62 ++++++++++++++++++++++++++++++++++++++++++-
 include/linux/phy/phy.h | 42 ++++++++++++++++++++++++++++-
 2 files changed, 104 insertions(+)

Comments

Laurent Pinchart Sept. 5, 2018, 1:39 p.m. UTC | #1

Hi Maxime,

Thank you for the patch.

On Wednesday, 5 September 2018 12:16:33 EEST 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

s/deal/deal with/

> 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 <[e-mail]>
> ---
>  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.
> + * @opts: New configuration to apply
> + *
> + * 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;

Shouldn't you report an error to the caller ? If a caller expects the PHY to 
be configurable, I would assume that silently ignoring the requested 
configuration won't work great.

> +	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)
> +{
> +	int ret;
> +
> +	if (!phy)
> +		return -EINVAL;
> +
> +	if (!phy->ops->validate)
> +		return 0;
> +
> +	mutex_lock(&phy->mutex);
> +	ret = phy->ops->validate(phy, mode, opts);
> +	mutex_unlock(&phy->mutex);
> +
> +	return ret;
> +}
> +
> +/**
>   * _of_phy_get() - lookup and obtain a reference to a phy by phandle
>   * @np: device_node for which to get the phy
>   * @index: the index of the phy
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index 9cba7fe16c23..3cc315dcfcd0 100644
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h
> @@ -44,6 +44,12 @@ enum phy_mode {
>  };
> 
>  /**
> + * union phy_configure_opts - Opaque generic phy configuration
> + */
> +union phy_configure_opts {
> +};
> +
> +/**
>   * struct phy_ops - set of function pointers for performing phy operations
>   * @init: operation to be performed for initializing phy
>   * @exit: operation to be performed while exiting
> @@ -60,6 +66,38 @@ struct phy_ops {
>  	int	(*power_on)(struct phy *phy);
>  	int	(*power_off)(struct phy *phy);
>  	int	(*set_mode)(struct phy *phy, enum phy_mode mode);
> +
> +	/**
> +	 * @configure:
> +	 *
> +	 * Optional.
> +	 *
> +	 * 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	(*configure)(struct phy *phy, enum phy_mode mode,
> +			     union phy_configure_opts *opts);

Is this function allowed to modify opts ? If so, to what extent ? If not, the 
pointer should be made const.

> +	/**
> +	 * @validate:
> +	 *
> +	 * Optional.
> +	 *
> +	 * 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 must not change
> +	 * any actual configuration of the PHY, so calling it as many
> +	 * times as deemed fit by the consumer must have no side
> +	 * effect.
> +	 *
> +	 * Returns: 0 if the configuration can be applied, an negative
> +	 * error code otherwise

When should this operation modify the passed parameters, and when should it 
return an error ? I understand that your goal is to implement a negotiation 
mechanism for the PHY parameters, and to be really useful I think we need to 
document it more precisely.

> +	 */
> +	int	(*validate)(struct phy *phy, enum phy_mode mode,
> +			    union phy_configure_opts *opts);
>  	int	(*reset)(struct phy *phy);
>  	int	(*calibrate)(struct phy *phy);
>  	struct module *owner;
> @@ -164,6 +202,10 @@ int phy_exit(struct phy *phy);
>  int phy_power_on(struct phy *phy);
>  int phy_power_off(struct phy *phy);
>  int phy_set_mode(struct phy *phy, enum phy_mode mode);
> +int phy_configure(struct phy *phy, enum phy_mode mode,
> +		  union phy_configure_opts *opts);
> +int phy_validate(struct phy *phy, enum phy_mode mode,
> +		 union phy_configure_opts *opts);
>  static inline enum phy_mode phy_get_mode(struct phy *phy)
>  {
>  	return phy->attrs.mode;

Kishon Vijay Abraham I Sept. 6, 2018, 9:27 a.m. UTC | #2

Hi,

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 <[e-mail]>
> ---
>  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.
> + * @opts: New configuration to apply

Should these configuration come from the consumer driver? Can't the helper
functions be directly invoked by the PHY driver for the configuration.
> + *
> + * 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.


> +{
> +	int ret;
> +
> +	if (!phy)
> +		return -EINVAL;
> +
> +	if (!phy->ops->validate)
> +		return 0;
> +
> +	mutex_lock(&phy->mutex);
> +	ret = phy->ops->validate(phy, mode, opts);
> +	mutex_unlock(&phy->mutex);
> +
> +	return ret;
> +}
> +
> +/**
>   * _of_phy_get() - lookup and obtain a reference to a phy by phandle
>   * @np: device_node for which to get the phy
>   * @index: the index of the phy
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index 9cba7fe16c23..3cc315dcfcd0 100644
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h
> @@ -44,6 +44,12 @@ enum phy_mode {
>  };
>  
>  /**
> + * union phy_configure_opts - Opaque generic phy configuration
> + */
> +union phy_configure_opts {
> +};
> +
> +/**
>   * struct phy_ops - set of function pointers for performing phy operations
>   * @init: operation to be performed for initializing phy
>   * @exit: operation to be performed while exiting
> @@ -60,6 +66,38 @@ struct phy_ops {
>  	int	(*power_on)(struct phy *phy);
>  	int	(*power_off)(struct phy *phy);
>  	int	(*set_mode)(struct phy *phy, enum phy_mode mode);
> +
> +	/**
> +	 * @configure:
> +	 *
> +	 * Optional.
> +	 *
> +	 * 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	(*configure)(struct phy *phy, enum phy_mode mode,
> +			     union phy_configure_opts *opts);
> +
> +	/**
> +	 * @validate:
> +	 *
> +	 * Optional.
> +	 *
> +	 * 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 must not change
> +	 * any actual configuration of the PHY, so calling it as many
> +	 * times as deemed fit by the consumer must have no side
> +	 * effect.
> +	 *
> +	 * Returns: 0 if the configuration can be applied, an negative
> +	 * error code otherwise
> +	 */
> +	int	(*validate)(struct phy *phy, enum phy_mode mode,
> +			    union phy_configure_opts *opts);
>  	int	(*reset)(struct phy *phy);
>  	int	(*calibrate)(struct phy *phy);
>  	struct module *owner;
> @@ -164,6 +202,10 @@ int phy_exit(struct phy *phy);
>  int phy_power_on(struct phy *phy);
>  int phy_power_off(struct phy *phy);
>  int phy_set_mode(struct phy *phy, enum phy_mode mode);
> +int phy_configure(struct phy *phy, enum phy_mode mode,
> +		  union phy_configure_opts *opts);
> +int phy_validate(struct phy *phy, enum phy_mode mode,
> +		 union phy_configure_opts *opts);

Stub function when CONFIG_GENERIC_PHY is not set is also required.

Thanks
Kishon

Maxime Ripard Sept. 6, 2018, 2:48 p.m. UTC | #3

On Wed, Sep 05, 2018 at 04:39:46PM +0300, Laurent Pinchart wrote:
> Hi Maxime,
> 
> Thank you for the patch.
> 
> On Wednesday, 5 September 2018 12:16:33 EEST 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
> 
> s/deal/deal with/
> 
> > 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 <[e-mail]>
> > ---
> >  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.
> > + * @opts: New configuration to apply
> > + *
> > + * 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;
> 
> Shouldn't you report an error to the caller ? If a caller expects the PHY to 
> be configurable, I would assume that silently ignoring the requested 
> configuration won't work great.

I'm not sure. I also expect a device having to interact with multiple
PHYs, some of them needing some configuration while some other do
not. In that scenario, returning 0 seems to be the right thing to do.

> > +	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)
> > +{
> > +	int ret;
> > +
> > +	if (!phy)
> > +		return -EINVAL;
> > +
> > +	if (!phy->ops->validate)
> > +		return 0;
> > +
> > +	mutex_lock(&phy->mutex);
> > +	ret = phy->ops->validate(phy, mode, opts);
> > +	mutex_unlock(&phy->mutex);
> > +
> > +	return ret;
> > +}
> > +
> > +/**
> >   * _of_phy_get() - lookup and obtain a reference to a phy by phandle
> >   * @np: device_node for which to get the phy
> >   * @index: the index of the phy
> > diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> > index 9cba7fe16c23..3cc315dcfcd0 100644
> > --- a/include/linux/phy/phy.h
> > +++ b/include/linux/phy/phy.h
> > @@ -44,6 +44,12 @@ enum phy_mode {
> >  };
> > 
> >  /**
> > + * union phy_configure_opts - Opaque generic phy configuration
> > + */
> > +union phy_configure_opts {
> > +};
> > +
> > +/**
> >   * struct phy_ops - set of function pointers for performing phy operations
> >   * @init: operation to be performed for initializing phy
> >   * @exit: operation to be performed while exiting
> > @@ -60,6 +66,38 @@ struct phy_ops {
> >  	int	(*power_on)(struct phy *phy);
> >  	int	(*power_off)(struct phy *phy);
> >  	int	(*set_mode)(struct phy *phy, enum phy_mode mode);
> > +
> > +	/**
> > +	 * @configure:
> > +	 *
> > +	 * Optional.
> > +	 *
> > +	 * 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	(*configure)(struct phy *phy, enum phy_mode mode,
> > +			     union phy_configure_opts *opts);
> 
> Is this function allowed to modify opts ? If so, to what extent ? If not, the 
> pointer should be made const.

That's a pretty good question. I guess it could modify it to the same
extent than validate could. Would that make sense?

> > +	/**
> > +	 * @validate:
> > +	 *
> > +	 * Optional.
> > +	 *
> > +	 * 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 must not change
> > +	 * any actual configuration of the PHY, so calling it as many
> > +	 * times as deemed fit by the consumer must have no side
> > +	 * effect.
> > +	 *
> > +	 * Returns: 0 if the configuration can be applied, an negative
> > +	 * error code otherwise
> 
> When should this operation modify the passed parameters, and when should it 
> return an error ? I understand that your goal is to implement a negotiation 
> mechanism for the PHY parameters, and to be really useful I think we need to 
> document it more precisely.

My initial idea was to reject a configuration that wouldn't be
achievable by the PHY, ie you're asking something that is outside of
the operating boundaries, while you would be able to change settings
that would be operational, but sub-optimal.

Maxime

Maxime Ripard Sept. 6, 2018, 2:56 p.m. UTC | #4

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 <[e-mail]>
> > ---
> >  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?

Can we have PHY that operate in multiple modes at the same time?

> > + * @opts: New configuration to apply
> 
> Should these configuration come from the consumer driver?

Yes

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

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
exposed to the user, and you don't end up in a situation where the
user select a mode that you knew had zero chance to work.

> > @@ -164,6 +202,10 @@ int phy_exit(struct phy *phy);
> >  int phy_power_on(struct phy *phy);
> >  int phy_power_off(struct phy *phy);
> >  int phy_set_mode(struct phy *phy, enum phy_mode mode);
> > +int phy_configure(struct phy *phy, enum phy_mode mode,
> > +		  union phy_configure_opts *opts);
> > +int phy_validate(struct phy *phy, enum phy_mode mode,
> > +		 union phy_configure_opts *opts);
> 
> Stub function when CONFIG_GENERIC_PHY is not set is also required.

Ok. Thanks!
Maxime

Andrew Lunn Sept. 6, 2018, 4:24 p.m. UTC | #5

> > > +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;
> > 
> > Shouldn't you report an error to the caller ? If a caller expects the PHY to 
> > be configurable, I would assume that silently ignoring the requested 
> > configuration won't work great.
> 
> I'm not sure. I also expect a device having to interact with multiple
> PHYs, some of them needing some configuration while some other do
> not. In that scenario, returning 0 seems to be the right thing to do.

You could return -EOPNOTSUPP. That is common in the network stack. The
caller then has the information to decide if it should keep going, or
return an error.

       Andrew

Laurent Pinchart Sept. 6, 2018, 4:51 p.m. UTC | #6

Hi Maxime,

On Thursday, 6 September 2018 17:48:07 EEST Maxime Ripard wrote:
> On Wed, Sep 05, 2018 at 04:39:46PM +0300, Laurent Pinchart wrote:
> > On Wednesday, 5 September 2018 12:16:33 EEST 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
> > 
> > s/deal/deal with/
> > 
> >> 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 <[e-mail]>
> >> ---
> >> 
> >>  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.
> >> + * @opts: New configuration to apply
> >> + *
> >> + * 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;
> > 
> > Shouldn't you report an error to the caller ? If a caller expects the PHY
> > to be configurable, I would assume that silently ignoring the requested
> > configuration won't work great.
> 
> I'm not sure. I also expect a device having to interact with multiple
> PHYs, some of them needing some configuration while some other do
> not. In that scenario, returning 0 seems to be the right thing to do.

It could be up to the caller to decide whether to ignore the error or not when 
the operation isn't implemented. I expect that a call requiring specific 
configuration parameters for a given PHY might want to bail out if the 
configuration can't be applied. On the other hand that should never happen 
when the system is designed correctly, as vendors are not supposed to ship 
kernels that would be broken by design (as in requiring a configure operation 
but not providing it).

> >> +	mutex_lock(&phy->mutex);
> >> +	ret = phy->ops->configure(phy, mode, opts);
> >> +	mutex_unlock(&phy->mutex);
> >> +
> >> +	return ret;
> >> +}

[snip]

> >> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> >> index 9cba7fe16c23..3cc315dcfcd0 100644
> >> --- a/include/linux/phy/phy.h
> >> +++ b/include/linux/phy/phy.h

[snip]

> >> @@ -60,6 +66,38 @@ struct phy_ops {
> >>  	int	(*power_on)(struct phy *phy);
> >>  	int	(*power_off)(struct phy *phy);
> >>  	int	(*set_mode)(struct phy *phy, enum phy_mode mode);
> >> +
> >> +	/**
> >> +	 * @configure:
> >> +	 *
> >> +	 * Optional.
> >> +	 *
> >> +	 * 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	(*configure)(struct phy *phy, enum phy_mode mode,
> >> +			     union phy_configure_opts *opts);
> > 
> > Is this function allowed to modify opts ? If so, to what extent ? If not,
> > the pointer should be made const.
> 
> That's a pretty good question. I guess it could modify it to the same
> extent than validate could. Would that make sense?

It would, or we could say that PHY users are required to call the validate 
function first, and the the configure function will return an error if the 
passed configuration isn't valid. That would avoid double-validation when the 
PHY user uses .validate().

> >> +	/**
> >> +	 * @validate:
> >> +	 *
> >> +	 * Optional.
> >> +	 *
> >> +	 * 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 must not change
> >> +	 * any actual configuration of the PHY, so calling it as many
> >> +	 * times as deemed fit by the consumer must have no side
> >> +	 * effect.
> >> +	 *
> >> +	 * Returns: 0 if the configuration can be applied, an negative
> >> +	 * error code otherwise
> > 
> > When should this operation modify the passed parameters, and when should
> > it return an error ? I understand that your goal is to implement a
> > negotiation mechanism for the PHY parameters, and to be really useful I
> > think we need to document it more precisely.
> 
> My initial idea was to reject a configuration that wouldn't be
> achievable by the PHY, ie you're asking something that is outside of
> the operating boundaries, while you would be able to change settings
> that would be operational, but sub-optimal.

I'm fine with that, let's document it explicitly.

Maxime Ripard Sept. 7, 2018, 9:01 a.m. UTC | #7

On Thu, Sep 06, 2018 at 06:24:50PM +0200, Andrew Lunn wrote:
> > > > +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;
> > > 
> > > Shouldn't you report an error to the caller ? If a caller expects the PHY to 
> > > be configurable, I would assume that silently ignoring the requested 
> > > configuration won't work great.
> > 
> > I'm not sure. I also expect a device having to interact with multiple
> > PHYs, some of them needing some configuration while some other do
> > not. In that scenario, returning 0 seems to be the right thing to do.
> 
> You could return -EOPNOTSUPP. That is common in the network stack. The
> caller then has the information to decide if it should keep going, or
> return an error.

Ok, that works for me then.

Maxime

Maxime Ripard Sept. 7, 2018, 9:07 a.m. UTC | #8

On Thu, Sep 06, 2018 at 07:51:05PM +0300, Laurent Pinchart wrote:
> On Thursday, 6 September 2018 17:48:07 EEST Maxime Ripard wrote:
> > On Wed, Sep 05, 2018 at 04:39:46PM +0300, Laurent Pinchart wrote:
> > > On Wednesday, 5 September 2018 12:16:33 EEST 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
> > > 
> > > s/deal/deal with/
> > > 
> > >> 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 <[e-mail]>
> > >> ---
> > >> 
> > >>  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.
> > >> + * @opts: New configuration to apply
> > >> + *
> > >> + * 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;
> > > 
> > > Shouldn't you report an error to the caller ? If a caller expects the PHY
> > > to be configurable, I would assume that silently ignoring the requested
> > > configuration won't work great.
> > 
> > I'm not sure. I also expect a device having to interact with multiple
> > PHYs, some of them needing some configuration while some other do
> > not. In that scenario, returning 0 seems to be the right thing to do.
> 
> It could be up to the caller to decide whether to ignore the error or not when 
> the operation isn't implemented. I expect that a call requiring specific 
> configuration parameters for a given PHY might want to bail out if the 
> configuration can't be applied. On the other hand that should never happen 
> when the system is designed correctly, as vendors are not supposed to ship 
> kernels that would be broken by design (as in requiring a configure operation 
> but not providing it).

I'll do as Andrew (and you) suggested then.

> > >> @@ -60,6 +66,38 @@ struct phy_ops {
> > >>  	int	(*power_on)(struct phy *phy);
> > >>  	int	(*power_off)(struct phy *phy);
> > >>  	int	(*set_mode)(struct phy *phy, enum phy_mode mode);
> > >> +
> > >> +	/**
> > >> +	 * @configure:
> > >> +	 *
> > >> +	 * Optional.
> > >> +	 *
> > >> +	 * 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	(*configure)(struct phy *phy, enum phy_mode mode,
> > >> +			     union phy_configure_opts *opts);
> > > 
> > > Is this function allowed to modify opts ? If so, to what extent ? If not,
> > > the pointer should be made const.
> > 
> > That's a pretty good question. I guess it could modify it to the same
> > extent than validate could. Would that make sense?
> 
> It would, or we could say that PHY users are required to call the validate 
> function first, and the the configure function will return an error if the 
> passed configuration isn't valid. That would avoid double-validation when the 
> PHY user uses .validate().

I usually prefer to have a function being able to check its input on
its own. Especially, the sole use case we have right now is DRM, and
DRM would typically call phy_validate X+1 times (X being the number of
modes), once for each mode in mode_valid and once in atomic_check.

> > >> +	/**
> > >> +	 * @validate:
> > >> +	 *
> > >> +	 * Optional.
> > >> +	 *
> > >> +	 * 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 must not change
> > >> +	 * any actual configuration of the PHY, so calling it as many
> > >> +	 * times as deemed fit by the consumer must have no side
> > >> +	 * effect.
> > >> +	 *
> > >> +	 * Returns: 0 if the configuration can be applied, an negative
> > >> +	 * error code otherwise
> > > 
> > > When should this operation modify the passed parameters, and when should
> > > it return an error ? I understand that your goal is to implement a
> > > negotiation mechanism for the PHY parameters, and to be really useful I
> > > think we need to document it more precisely.
> > 
> > My initial idea was to reject a configuration that wouldn't be
> > achievable by the PHY, ie you're asking something that is outside of
> > the operating boundaries, while you would be able to change settings
> > that would be operational, but sub-optimal.
> 
> I'm fine with that, let's document it explicitly.

ACK.

Maxime

Kishon Vijay Abraham I Sept. 12, 2018, 7:42 a.m. UTC | #9

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 <[e-mail]>
>>> ---
>>>  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

Maxime Ripard Sept. 12, 2018, 8:42 a.m. UTC | #10

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 <[e-mail]>
> >>> ---
> >>>  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.

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.

Maxime

Kishon Vijay Abraham I Sept. 14, 2018, 8:48 a.m. UTC | #11

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 <[e-mail]>
>>>>> ---
>>>>>  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

Maxime Ripard Sept. 19, 2018, 12:14 p.m. UTC | #12

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 Sept. 21, 2018, 2:18 p.m. UTC | #13

On Wed, Sep 19, 2018 at 02:14:36PM +0200, Maxime Ripard wrote:
> > 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.

There's also a patch set currently being submitted that uses a phy
driver + custom functions:
https://lore.kernel.org/patchwork/cover/988959/

Maxime

Kishon Vijay Abraham I Sept. 24, 2018, 8:48 a.m. UTC | #14

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

Maxime Ripard Sept. 24, 2018, 9:54 a.m. UTC | #15

Hi,

On Mon, Sep 24, 2018 at 02:18:35PM +0530, Kishon Vijay Abraham I wrote:
> 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.

That wouldn't work for device whose bindings need to remain backward
compatible. And the Allwinner part at least is in that case.

I think we should aim at making it a norm for newer bindings, but we
still have to support the old ones that cannot be changed.

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

As I was saying above, I'm not sure we can do that.

> We could also use phy_set_bus_width() for lanes.

I overlooked this function somehow, it indeed looks like we can remove
the lanes part in favor of this function.

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

I definitely understand you skepticism towards someone coming in and
dropping such a big list of obscure parameters :)

However, those values are actually the whole list of parameters
defined by the MIPI-DPHY standard, so I really don't expect drivers to
need more than that. As you can see, most drivers allow less
parameters to be configured, but all of them are defined within those
parameters. So I'm not sure we need to worry about an ever-expanding
list of parameters: we have a limited set of parameters defined, and
from an authoritative source, so we can also push back if someone
wants to add a parameter that is implementation specific.


Maxime

Kishon Vijay Abraham I Sept. 24, 2018, 11:55 a.m. UTC | #16

Hi Maxime,

On Monday 24 September 2018 03:24 PM, Maxime Ripard wrote:
> Hi,
> 
> On Mon, Sep 24, 2018 at 02:18:35PM +0530, Kishon Vijay Abraham I wrote:
>> 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.
> 
> That wouldn't work for device whose bindings need to remain backward
> compatible. And the Allwinner part at least is in that case.

Er..
> 
> I think we should aim at making it a norm for newer bindings, but we
> still have to support the old ones that cannot be changed.

There are drivers which support both the old and new bindings. Allwinner could
be in that category.
> 
>>>> 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.
> 
> As I was saying above, I'm not sure we can do that.
> 
>> We could also use phy_set_bus_width() for lanes.
> 
> I overlooked this function somehow, it indeed looks like we can remove
> the lanes part in favor of this function.
> 
>>>
>>>> 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).
> 
> I definitely understand you skepticism towards someone coming in and
> dropping such a big list of obscure parameters :)

That's part of my concern. The other concern is consumer driver having to know
so much internal details of the PHY. None of the other consumers (PCIe, USB,
etc) had to know so much internals of the PHY.
> 
> However, those values are actually the whole list of parameters
> defined by the MIPI-DPHY standard, so I really don't expect drivers to
> need more than that. As you can see, most drivers allow less
> parameters to be configured, but all of them are defined within those
> parameters. So I'm not sure we need to worry about an ever-expanding
> list of parameters: we have a limited set of parameters defined, and
> from an authoritative source, so we can also push back if someone
> wants to add a parameter that is implementation specific.

Your commit log mentioned there are a few parameters in addition to what is
specified in the MIPI D-PHY spec :-/

"The parameters added here are the one defined in the MIPI D-PHY spec, plus
some parameters that were used by a number of PHY drivers currently found
in the linux kernel."

Thanks
Kishon

Maxime Ripard Sept. 24, 2018, 12:19 p.m. UTC | #17

Hi!

(stripping the mail a bit)

On Mon, Sep 24, 2018 at 05:25:26PM +0530, Kishon Vijay Abraham I wrote:
> >>>>> 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.
> > 
> > That wouldn't work for device whose bindings need to remain backward
> > compatible. And the Allwinner part at least is in that case.
> 
> Er..
> 
> > I think we should aim at making it a norm for newer bindings, but we
> > still have to support the old ones that cannot be changed.
> 
> There are drivers which support both the old and new bindings. Allwinner could
> be in that category.

Yes, definitely. However, in order to support the old binding, we'd
still need to have a way to give the clock rates to the phy when we
don't have that clock provider. So I don't really see how we could
remove those fields, even if we start introducing new bindings.

> >>> 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).
> > 
> > I definitely understand you skepticism towards someone coming in and
> > dropping such a big list of obscure parameters :)
> 
> That's part of my concern. The other concern is consumer driver having to know
> so much internal details of the PHY. None of the other consumers (PCIe, USB,
> etc) had to know so much internals of the PHY.

I guess that's true to some extent, but it also feels a bit like a
self-realizing prophecy. The phy framework also didn't provide any way
to change the phy configuration based on some runtime values up until
now, and we have a good example with the MIPI-DSI and MIPI-CSI drivers
that given the constructs used in these drivers, if the phy framework
had allowed it, they would have used it.

Also, speaking of USB, we've had some configuration that was done in
some private functions exported by the phy drivers themselves. So
there is a use case for such an interface even for other, less
configurable, phy types.

I even planned for that, since the phy_validate and phy_configure
functions are passed an union, in order to make it extensible to other
phy types easily.

I guess that both the fact that the configuration is simpler for the
phy types supported so far, and that the phy and its consumer are very
integrated, didn't make it necessary to have such an interface so
far. But with MIPI-DPHY, we have both a quite extensive configuration
to make, and the phys and their consumers seem to be a bit more
loosely integrated. HDMI phys seem to be in pretty much the same case
too.

> > However, those values are actually the whole list of parameters
> > defined by the MIPI-DPHY standard, so I really don't expect drivers to
> > need more than that. As you can see, most drivers allow less
> > parameters to be configured, but all of them are defined within those
> > parameters. So I'm not sure we need to worry about an ever-expanding
> > list of parameters: we have a limited set of parameters defined, and
> > from an authoritative source, so we can also push back if someone
> > wants to add a parameter that is implementation specific.
> 
> Your commit log mentioned there are a few parameters in addition to what is
> specified in the MIPI D-PHY spec :-/
> 
> "The parameters added here are the one defined in the MIPI D-PHY spec, plus
> some parameters that were used by a number of PHY drivers currently found
> in the linux kernel."

I should have phrased that better then, sorry. The only additions that
were made were the lanes number (that we agreed to remove), the high
speed and low power clock rates, and the video timings.

Maxime

Patch

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.
+ * @opts: New configuration to apply
+ *
+ * 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)
+{
+	int ret;
+
+	if (!phy)
+		return -EINVAL;
+
+	if (!phy->ops->validate)
+		return 0;
+
+	mutex_lock(&phy->mutex);
+	ret = phy->ops->validate(phy, mode, opts);
+	mutex_unlock(&phy->mutex);
+
+	return ret;
+}
+
+/**
  * _of_phy_get() - lookup and obtain a reference to a phy by phandle
  * @np: device_node for which to get the phy
  * @index: the index of the phy
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index 9cba7fe16c23..3cc315dcfcd0 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -44,6 +44,12 @@  enum phy_mode {
 };
 
 /**
+ * union phy_configure_opts - Opaque generic phy configuration
+ */
+union phy_configure_opts {
+};
+
+/**
  * struct phy_ops - set of function pointers for performing phy operations
  * @init: operation to be performed for initializing phy
  * @exit: operation to be performed while exiting
@@ -60,6 +66,38 @@  struct phy_ops {
 	int	(*power_on)(struct phy *phy);
 	int	(*power_off)(struct phy *phy);
 	int	(*set_mode)(struct phy *phy, enum phy_mode mode);
+
+	/**
+	 * @configure:
+	 *
+	 * Optional.
+	 *
+	 * 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	(*configure)(struct phy *phy, enum phy_mode mode,
+			     union phy_configure_opts *opts);
+
+	/**
+	 * @validate:
+	 *
+	 * Optional.
+	 *
+	 * 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 must not change
+	 * any actual configuration of the PHY, so calling it as many
+	 * times as deemed fit by the consumer must have no side
+	 * effect.
+	 *
+	 * Returns: 0 if the configuration can be applied, an negative
+	 * error code otherwise
+	 */
+	int	(*validate)(struct phy *phy, enum phy_mode mode,
+			    union phy_configure_opts *opts);
 	int	(*reset)(struct phy *phy);
 	int	(*calibrate)(struct phy *phy);
 	struct module *owner;
@@ -164,6 +202,10 @@  int phy_exit(struct phy *phy);
 int phy_power_on(struct phy *phy);
 int phy_power_off(struct phy *phy);
 int phy_set_mode(struct phy *phy, enum phy_mode mode);
+int phy_configure(struct phy *phy, enum phy_mode mode,
+		  union phy_configure_opts *opts);
+int phy_validate(struct phy *phy, enum phy_mode mode,
+		 union phy_configure_opts *opts);
 static inline enum phy_mode phy_get_mode(struct phy *phy)
 {
 	return phy->attrs.mode;