Hi, On Wed, Nov 25, 2015 at 10:24 AM, Heiko St?bner <heiko at sntech.de> wrote: > Am Mittwoch, 25. November 2015, 09:04:19 schrieb Doug Anderson: >> Hi, >> >> On Sun, Nov 22, 2015 at 11:49 AM, Heiko Stuebner <heiko at sntech.de> wrote: >> > Am Donnerstag, 19. November 2015, 16:32:23 schrieb Doug Anderson: >> >> Heiko, >> >> >> >> On Thu, Nov 19, 2015 at 1:22 PM, Heiko Stuebner <heiko at sntech.de> wrote: >> >> > We need custom handling for these two socs in the driver shortly, >> >> > so add the necessary compatible values to binding and driver. >> >> > >> >> > Signed-off-by: Heiko Stuebner <heiko at sntech.de> >> >> > --- >> >> > >> >> > Documentation/devicetree/bindings/phy/rockchip-usb-phy.txt | 5 ++++- >> >> > drivers/phy/phy-rockchip-usb.c | 2 ++ >> >> > 2 files changed, 6 insertions(+), 1 deletion(-) >> >> > >> >> > diff --git a/Documentation/devicetree/bindings/phy/rockchip-usb-phy.txt >> > >> > b/Documentation/devicetree/bindings/phy/rockchip-usb-phy.txt >> > >> >> > index 826454a..9b37242 100644 >> >> > --- a/Documentation/devicetree/bindings/phy/rockchip-usb-phy.txt >> >> > +++ b/Documentation/devicetree/bindings/phy/rockchip-usb-phy.txt >> >> > @@ -1,7 +1,10 @@ >> >> > >> >> > ROCKCHIP USB2 PHY >> >> > >> >> > Required properties: >> >> > - - compatible: rockchip,rk3288-usb-phy >> >> > + - compatible: matching the soc type, one of >> >> > + "rockchip,rk3066a-usb-phy" >> >> > + "rockchip,rk3188-usb-phy" >> >> > + "rockchip,rk3288-usb-phy" >> >> >> >> I can never quite keep it straight how this is supposed to work, but >> >> since previously only "rockchip,rk3288-usb-phy" was supported and now >> >> we have these new compatible strings, I would have expected the new >> >> strings to specify the old ones as fallback. That would mean your >> >> choices would be: >> >> >> >> - "rockchip,rk3288-usb-phy" - A real rk3288 >> >> - "rockchip,rk3188-usb-phy", "rockchip,rk3288-usb-phy" - A rk3188 with >> >> fallback to 3288 driver. >> >> - "rockchip,rk3066a-usb-phy", "rockchip,rk3288-usb-phy" - A rk3066a >> >> with fallback to 3288 driver. >> > >> > How this is supposed to be done also is sometimes confusing for me :-) >> > >> > But I don't think that specifying the "fallbacks" is part of the binding >> > at >> > all, when the binding really is done in a soc-specific way. For example >> > following the suggestion of the dt-maintainers at the time we're >> > specifying >> > the uarts as >> > >> > compatible = "rockchip,rk3288-uart", "snps,dw-apb-uart" >> > >> > as a measure to use a more-special driver if there is ever the need for >> > it. >> > But here the "snps,dw-apb-uart" actually is a superset (a more generic >> > implementation), while in the usb-uart-case >> >> Hrm, this gets into the whole issue of coming up with generic names. >> It's not always easy, especially when marketing gets involved. If >> Synopsis comes up with a new APB UART that's not compatible, I guess >> you call it a v2 and people just need to figure out which one they >> have? I remember it being terribly confusing with exynos since >> Samsung called things "exynos" that were very different, and I think >> even "exynos5" devices were pretty different form each other. Anyway, >> that's getting pretty far afield. >> >> The general way of doing things in Linux is that the first driver >> there becomes the generic, right? So if the first supported SoC using >> this PHY was rk3288 then it gets the name and becomes the generic. If >> rk3066a and 3188 are 90% the same and initially can actually use the >> same driver, then they specify the specific "3188" and the generic >> "3288" as a fallback. It sounds like that was what was actually done >> in the DTS files anyway, which is right as far as I'm concerned. >> >> ...but personally I'd love to see it documented. ...someone reading >> the binding should be able to create a DTS and it's not obvious from >> the DTS that "rk3288" is the generic as far as this binding is >> concerned. > > I'd like to disagree here :-) > > Generic is actually currently "rockchip-usb-phy", the platform-driver name, > but thankfully that hasn't leaked into the dts, as even that name + filename > should change in the future. What rk3288 (and before) uses is a Designware > picophy with custom registers in the grf, so it should actually be > "rockchip-usb-picophy" or so. Following socs use a different IP (Innosilicon > if I remember correctly), with different clock/pll handling and a whole > different set of registers, so will get a new driver. > > In terms of hardware compatibility, the phys aren't actually compatible, it's > only per chance that the used SIDDQ bit has the same position on all socs :-) > Everything else seems to move around quite happily in the registers. > > > As it stands now, the rk3188 dts has a compatible of > "rockchip,rk3188-usb-phy", "rockchip,rk3288-usb-phy", matching the usable > [refraining from calling it generic] driver on old kernels, while now it is > supposed to match the actually correct rk3188 variant. So that combination > works with the same dts on both old and new kernels fullfilling the ABI- > stability promise. > > With the new matching code (clock-names etc) you actually get issues if you > try to match against "rockchip,rk3288-usb-phy" on a rk3188, so there is no > "generic" part. And that difference will widen if we need to control other > parts of the usb-phy as well. > > So essentially that second property should go completely, I just didn't wanted > to create the impression of changing the ABI here ;-) OK, fair enough. I'm not terribly familiar with rk3066a and rk3188, so didn't realize how different they were. I guess I assumed that they were more like 90% compatible, but perhaps not... I also think some of these corner cases of device tree still haven't seeped into my consciousness yet and become instinct. Maybe in another few years. ;) -Doug
