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 -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Attachment:
signature.asc
Description: PGP signature
