Re: [PATCH v2 1/2] dt-bindings: phy: add mediatek mipi csi driver v 0.5

[Rob Herring]

On Mon, Jun 05, 2023 at 10:44:22AM +0200, Julien Stephan wrote:
> On Tue, May 30, 2023 at 10:53:31AM +0200, Krzysztof Kozlowski wrote:
> > On 22/05/2023 21:15, Kevin Hilman wrote:
> > > Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> writes:
> > >
> > >> On 16/05/2023 23:31, Kevin Hilman wrote:
> > >>
> > >>>> Third is to use versioned IP blocks.
> > >>>>
> > >>>> The second case also would work, if it is applicable to you (you really
> > >>>> have fallback matching all devices). Third solution depends on your
> > >>>> versioning and Rob expressed dislike about it many times.
> > >>>>
> > >>>> We had many discussions on mailing lists, thus simplifying the review -
> > >>>> I recommend the first choice. For a better recommendation you should say
> > >>>> a bit more about the block in different SoCs.
> > >>>
> > >>> I'll try to say a bit more about the PHY block, but in fact, it's not
> > >>> just about differences between SoCs. On the same SoC, 2 different PHYs
> > >>> may have different features/capabilities.
> > >>>
> > >>> For example, on MT8365, There are 2 PHYs: CSI0 and CSI1.  CSI0 can
> > >>> function as a C-PHY or a D-PHY, but CSI1 can only function as D-PHY
> > >>> (used as the example in the binding patch[1].)  On another related SoC,
> > >>> there are 3 PHYs, where CSI0 is C-D but CSI1 & CSI2 are only D.
> > >>>
> > >>> So that's why it seems (at least to me) that while we need SoC
> > >>> compatible, it's not enough.  We also need properties to describe
> > >>> PHY-specific features (e.g. C-D PHY)
> > >>
> > >> I recall the same or very similar case... It bugs me now, but
> > >> unfortunately I cannot find it.
> > >>
> > >>>
> > >>> Of course, we could rely only on SoC-specific compatibles describe this.
> > >>> But then driver will need an SoC-specific table with the number of PHYs
> > >>> and per-PHY features for each SoC encoded in the driver.  Since the
> > >>> driver otherwise doesn't (and shouldn't, IMHO) need to know how many
> > >>> PHYs are on each SoC, I suggested to Julien that perhaps the additional
> > >>> propery was the better solution.
> > >>
> > >> Phys were modeled as separate device instances, so you would need
> > >> difference in compatible to figure out which phy is it.
> > >>
> > >> Other way could be to create device for all phys and use phy-cells=1.
> > >> Whether it makes sense, depends on the actual datasheet - maybe the
> > >> split phy per device is artificial? There is one PHY block with two
> > >> address ranges for each PHY - CSI0 and CSI1 - but it is actually one
> > >> block? You should carefully check this because once design is chosen,
> > >> you won't be able to go back to other and it might be a problem (e.g.
> > >> there is some top-level block for powering on all CSI instances).
> > >
> > > We're pretty sure these are multiple instances of the IP block as they
> > > can operate completely independently.
> > >
> > >>>
> > >>> To me it seems redundant to have the driver encode PHYs-per-SoC info,
> > >>> when the per-SoC DT is going to have the same info, so my suggestion was
> > >>> to simplify the driver and have this kind of hardware description in the
> > >>> DT, and keep the driver simple, but we are definitely open to learning
> > >>> the "right way" of doing this.
> > >>
> > >> The property then is reasonable. It should not be bool, though, because
> > >> it does not scale. There can be next block which supports only D-PHY on
> > >> CSI0 and C-PHY on CSI1? Maybe some enum or list, depending on possible
> > >> configurations.
> > >
> > > OK, looks like include/dt-bindings/phy/phy.y already has
> > >
> > >   #define PHY_TYPE_DPHY		10
> > >   #define PHY_TYPE_CPHY		11
> > >
> > > we'll add a PHY_TYPE_CDPHY and use that.   Sound reasonable?

No, because these defines are the 1 mode used for a specific connection, 
not the set of supported modes.

> >
> > Yes. Currently it is usually used as phy-cells argument (after the phy
> > number/lane/ID), but cdns,phy-type and intel,phy-mode use it directly as
> > property in provider. In both cases they have a bit different meaning
> > than yours. You want to list all supported modes or narrow/restrict
> > them. Maybe hisilicon,fixed-mode fits your purpose?

There is also 'phy-type' to set the mode/type in the provider.

If we need to list all possible modes, then you need to justify why 
that's needed and then define a property which is a mask of the 
existing type defines.

> >
> 
> Hi Krzysztof,
> 
> Thanks for the suggestion, using something like hisilicon,fixed-node
> looks like a good fit.
> With mediatek,fixed-node, by default CSI node will be considered as
> CD-PHY capable (unless the fixed-mode property is set.) so I won't need
> anymore the new define PHY_TYPE_CDPHY introduced in v3.
> 
> Also introducing mediatek,fixed-mode suggests that PHYs not declaring
> the fixed mode property support mode selection, so I suggest to add a
> phy argument (#phy-cells = <1>) to select the mode (D or C mode).
> Exactly what is done by the hsilicon driver.
> 
> How does that sound to you?

I don't follow the need for fixed-mode, but agree you should use a phy 
arg. Can't you just assume the D-PHY only instance will only have D-PHY 
for the arg?

Rob