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
