Hi!

(stripping the mail a bit)

On Mon, Sep 24, 2018 at 05:25:26PM +0530, Kishon Vijay Abraham I wrote:

quoted

quoted

quoted

quoted

quoted

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

quoted

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.

quoted

quoted

quoted

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.

quoted

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

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180924/0fabdf38/attachment.sig>