Message ID | 1447968149-10979-5-git-send-email-heiko@sntech.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Heiko, On Thu, Nov 19, 2015 at 1:22 PM, Heiko Stuebner <heiko@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@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. That means that if you land the dts changes without the driver changes that things still work OK. -Doug
Am Donnerstag, 19. November 2015, 16:32:23 schrieb Doug Anderson: > Heiko, > > On Thu, Nov 19, 2015 at 1:22 PM, Heiko Stuebner <heiko@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@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 > That means that if you land the dts changes without the driver changes > that things still work OK. We already have the alternative for the usb-phys in the devicetree, but I still don't think that this alternative is part of the binding itself :-) Heiko
Hi, On Sun, Nov 22, 2015 at 11:49 AM, Heiko Stuebner <heiko@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@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@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. Quite honestly, though, it's not terribly important to me and definitely not something I feel like I can make a call on. If you feel strongly about keeping it the way you have it and nobody else has any objections, I won't yell. -Doug
Am Mittwoch, 25. November 2015, 09:04:19 schrieb Doug Anderson: > Hi, > > On Sun, Nov 22, 2015 at 11:49 AM, Heiko Stuebner <heiko@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@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@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 ;-) Sorry for that wall of text. Heiko
Hi, On Wed, Nov 25, 2015 at 10:24 AM, Heiko Stübner <heiko@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@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@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@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
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" - rockchip,grf : phandle to the syscon managing the "general register files" - #address-cells: should be 1 diff --git a/drivers/phy/phy-rockchip-usb.c b/drivers/phy/phy-rockchip-usb.c index ff3ac33..16cd533 100644 --- a/drivers/phy/phy-rockchip-usb.c +++ b/drivers/phy/phy-rockchip-usb.c @@ -176,6 +176,8 @@ static int rockchip_usb_phy_probe(struct platform_device *pdev) } static const struct of_device_id rockchip_usb_phy_dt_ids[] = { + { .compatible = "rockchip,rk3066a-usb-phy" }, + { .compatible = "rockchip,rk3188-usb-phy" }, { .compatible = "rockchip,rk3288-usb-phy" }, {} };
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@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(-)