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:

quoted

On Wednesday 05 September 2018 02:46 PM, Maxime Ripard wrote:

quoted

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

quoted

quoted

+ * @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.

quoted

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

quoted

quoted

+ *
+ * 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