Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1273517 > unrolled thread

[PATCH v3 0/8] phy: rockchip-usb: correct pll handling and usb-uart

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/8] phy: rockchip-usb: correct pll handling and usb-uart Heiko Stuebner <heiko@sntech.de> - 2015-11-19 22:30 +0100
    [PATCH v3 4/8] phy: rockchip-usb: add compatible values for rk3066a and rk3188 Heiko Stuebner <heiko@sntech.de> - 2015-11-19 22:30 +0100
      Re: [PATCH v3 4/8] phy: rockchip-usb: add compatible values for  rk3066a and rk3188 Doug Anderson <dianders@chromium.org> - 2015-11-20 01:40 +0100
        Re: [PATCH v3 4/8] phy: rockchip-usb: add compatible values for rk3066a and rk3188 Heiko Stuebner <heiko@sntech.de> - 2015-11-22 20:50 +0100
          Re: [PATCH v3 4/8] phy: rockchip-usb: add compatible values for  rk3066a and rk3188 Doug Anderson <dianders@chromium.org> - 2015-11-25 18:10 +0100
            Re: [PATCH v3 4/8] phy: rockchip-usb: add compatible values for rk3066a and rk3188 Heiko Stübner <heiko@sntech.de> - 2015-11-25 19:30 +0100
              Re: [PATCH v3 4/8] phy: rockchip-usb: add compatible values for  rk3066a and rk3188 Doug Anderson <dianders@chromium.org> - 2015-11-25 19:40 +0100
    [PATCH v3 3/8] phy: rockchip-usb: move per-phy init into a separate function Heiko Stuebner <heiko@sntech.de> - 2015-11-19 22:30 +0100
    [PATCH v3 1/8] phy: rockchip-usb: fix clock get-put mismatch Heiko Stuebner <heiko@sntech.de> - 2015-11-19 22:30 +0100

#1273517 — [PATCH v3 0/8] phy: rockchip-usb: correct pll handling and usb-uart

changes in v3:
- rebase on top of Julias of_node_put fix
- address comments from Kishon Vijay Abraham
  - position of the devm_action in the first patch
  - separate compatible-addition into separate patch
  - don't rephrase comment when moving stuff around
- address Doug's comment and keep clk-tree change and assigned-clocks
  setting together
- add Doug's review-tag to patches 5,6,7
changes in v2:
- add Doug's review-tag to patches 1 and 3
- address comment and add the missing transistional rk_phy->base
  assignment in patch2

Patch 1 might be nice to go in as fix together with Julia's patch?

Patches 2-7 fix a long-standing issue with the clock-tree of Rockchip SoCs
namely our ignorance of the usbphy-internal pll that creates the needed
480MHz but is also a supply-clock back to the core clock-controller in
Rockchip SoCs.

Till now that was worked around using a virtual clock in the cru itself,
but that is of course ignorant of other parts then disabling the phy
behind the cru's back, thus breaking potential users of these clocks.


Patch 8, while not associated with the new pll handling, also builds
on the groundwork introduced there and adds support for the function
repurposing one of the phys as passthrough for uart-data. This enables
attaching a ttl converter to the D+ and D- pins of an usb cable to
receive uart data this way, when it is not really possible to attach
a regular serial console to a board.

One point of critique in my first iteration [0] of this was, that
due to when the reconfiguration happens we may miss parts of the logs
when earlycon is enabled. So far early_initcall gets used as the
unflattened devicetree is necessary to set this up. Doing this for
example in the early_param directly would require parsing the flattened
devicetree to get needed nodes and properties.

I still maintain that if you're working on anything before smp-bringup
you should use a real dev-board instead or try to solder uart cables
on hopefully available test-points :-) .


In any case, if patch 8 causes to much headache, it could be dropped
to not hinder the earlier 7 patches.

[0] http://comments.gmane.org/gmane.linux.ports.arm.rockchip/715

Heiko Stuebner (8):
  phy: rockchip-usb: fix clock get-put mismatch
  phy: rockchip-usb: introduce a common data-struct for the device
  phy: rockchip-usb: move per-phy init into a separate function
  phy: rockchip-usb: add compatible values for rk3066a and rk3188
  phy: rockchip-usb: expose the phy-internal PLLs
  ARM: dts: rockchip: add clock-cells for usb phy nodes
  clk: rockchip: fix usbphy-related clocks
  phy: rockchip-usb: add handler for usb-uart functionality

 .../devicetree/bindings/phy/rockchip-usb-phy.txt   |   6 +-
 Documentation/kernel-parameters.txt                |   6 +
 arch/arm/boot/dts/rk3066a.dtsi                     |   2 +
 arch/arm/boot/dts/rk3188.dtsi                      |   2 +
 arch/arm/boot/dts/rk3288-veyron.dtsi               |   2 +-
 arch/arm/boot/dts/rk3288.dtsi                      |   3 +
 drivers/clk/rockchip/clk-rk3188.c                  |  11 +-
 drivers/clk/rockchip/clk-rk3288.c                  |  16 +-
 drivers/phy/phy-rockchip-usb.c                     | 458 ++++++++++++++++++---
 9 files changed, 417 insertions(+), 89 deletions(-)

-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1273518 — [PATCH v3 4/8] phy: rockchip-usb: add compatible values for rk3066a and rk3188

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"
  - 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" },
 	{}
 };
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1273633 — Re: [PATCH v3 4/8] phy: rockchip-usb: add compatible values for rk3066a and rk3188

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1274930 — Re: [PATCH v3 4/8] phy: rockchip-usb: add compatible values for rk3066a and rk3188

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277619 — Re: [PATCH v3 4/8] phy: rockchip-usb: add compatible values for rk3066a and rk3188

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277681 — Re: [PATCH v3 4/8] phy: rockchip-usb: add compatible values for rk3066a and rk3188

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1277688 — Re: [PATCH v3 4/8] phy: rockchip-usb: add compatible values for rk3066a and rk3188

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1273519 — [PATCH v3 3/8] phy: rockchip-usb: move per-phy init into a separate function

This unclutters the loop in probe a lot and makes current (and future)
error handling easier to read.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
 drivers/phy/phy-rockchip-usb.c | 83 ++++++++++++++++++++++--------------------
 1 file changed, 43 insertions(+), 40 deletions(-)

diff --git a/drivers/phy/phy-rockchip-usb.c b/drivers/phy/phy-rockchip-usb.c
index 2b4802a..ff3ac33 100644
--- a/drivers/phy/phy-rockchip-usb.c
+++ b/drivers/phy/phy-rockchip-usb.c
@@ -103,14 +103,52 @@ static void rockchip_usb_phy_action(void *data)
 		clk_put(rk_phy->clk);
 }
 
+static int rockchip_usb_phy_init(struct rockchip_usb_phy_base *base,
+				 struct device_node *child)
+{
+	struct rockchip_usb_phy *rk_phy;
+	unsigned int reg_offset;
+	int err;
+
+	rk_phy = devm_kzalloc(base->dev, sizeof(*rk_phy), GFP_KERNEL);
+	if (!rk_phy)
+		return -ENOMEM;
+
+	rk_phy->base = base;
+
+	if (of_property_read_u32(child, "reg", &reg_offset)) {
+		dev_err(base->dev, "missing reg property in node %s\n",
+			child->name);
+		return -EINVAL;
+	}
+
+	rk_phy->reg_offset = reg_offset;
+
+	err = devm_add_action(base->dev, rockchip_usb_phy_action, rk_phy);
+	if (err)
+		return err;
+
+	rk_phy->clk = of_clk_get_by_name(child, "phyclk");
+	if (IS_ERR(rk_phy->clk))
+		rk_phy->clk = NULL;
+
+	rk_phy->phy = devm_phy_create(base->dev, child, &ops);
+	if (IS_ERR(rk_phy->phy)) {
+		dev_err(base->dev, "failed to create PHY\n");
+		return PTR_ERR(rk_phy->phy);
+	}
+	phy_set_drvdata(rk_phy->phy, rk_phy);
+
+	/* only power up usb phy when it use, so disable it when init*/
+	return rockchip_usb_phy_power(rk_phy, 1);
+}
+
 static int rockchip_usb_phy_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct rockchip_usb_phy_base *phy_base;
-	struct rockchip_usb_phy *rk_phy;
 	struct phy_provider *phy_provider;
 	struct device_node *child;
-	unsigned int reg_offset;
 	int err;
 
 	phy_base = devm_kzalloc(dev, sizeof(*phy_base), GFP_KERNEL);
@@ -126,50 +164,15 @@ static int rockchip_usb_phy_probe(struct platform_device *pdev)
 	}
 
 	for_each_available_child_of_node(dev->of_node, child) {
-		rk_phy = devm_kzalloc(dev, sizeof(*rk_phy), GFP_KERNEL);
-		if (!rk_phy) {
-			err = -ENOMEM;
-			goto put_child;
-		}
-
-		rk_phy->base = phy_base;
-
-		if (of_property_read_u32(child, "reg", &reg_offset)) {
-			dev_err(dev, "missing reg property in node %s\n",
-				child->name);
-			err = -EINVAL;
-			goto put_child;
-		}
-
-		rk_phy->reg_offset = reg_offset;
-
-		err = devm_add_action(dev, rockchip_usb_phy_action, rk_phy);
-		if (err)
+		err = rockchip_usb_phy_init(phy_base, child);
+		if (err) {
+			of_node_put(child);
 			return err;
-
-		rk_phy->clk = of_clk_get_by_name(child, "phyclk");
-		if (IS_ERR(rk_phy->clk))
-			rk_phy->clk = NULL;
-
-		rk_phy->phy = devm_phy_create(dev, child, &ops);
-		if (IS_ERR(rk_phy->phy)) {
-			dev_err(dev, "failed to create PHY\n");
-			err = PTR_ERR(rk_phy->phy);
-			goto put_child;
 		}
-		phy_set_drvdata(rk_phy->phy, rk_phy);
-
-		/* only power up usb phy when it use, so disable it when init*/
-		err = rockchip_usb_phy_power(rk_phy, 1);
-		if (err)
-			goto put_child;
 	}
 
 	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
 	return PTR_ERR_OR_ZERO(phy_provider);
-put_child:
-	of_node_put(child);
-	return err;
 }
 
 static const struct of_device_id rockchip_usb_phy_dt_ids[] = {
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1273520 — [PATCH v3 1/8] phy: rockchip-usb: fix clock get-put mismatch

Currently the phy driver only gets the optional clock reference but
never puts it again, neither during error handling nor on remove.
Fix that by moving the clk_put to a devm-action that gets called at
the right time when all other devm actions are done.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
 drivers/phy/phy-rockchip-usb.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/phy/phy-rockchip-usb.c b/drivers/phy/phy-rockchip-usb.c
index 62c43c4..e941444 100644
--- a/drivers/phy/phy-rockchip-usb.c
+++ b/drivers/phy/phy-rockchip-usb.c
@@ -90,6 +90,14 @@ static const struct phy_ops ops = {
 	.owner		= THIS_MODULE,
 };
 
+static void rockchip_usb_phy_action(void *data)
+{
+	struct rockchip_usb_phy *rk_phy = data;
+
+	if (rk_phy->clk)
+		clk_put(rk_phy->clk);
+}
+
 static int rockchip_usb_phy_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -123,6 +131,10 @@ static int rockchip_usb_phy_probe(struct platform_device *pdev)
 		rk_phy->reg_offset = reg_offset;
 		rk_phy->reg_base = grf;
 
+		err = devm_add_action(dev, rockchip_usb_phy_action, rk_phy);
+		if (err)
+			return err;
+
 		rk_phy->clk = of_clk_get_by_name(child, "phyclk");
 		if (IS_ERR(rk_phy->clk))
 			rk_phy->clk = NULL;
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web