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? Kevin
