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

Groups > linux.kernel > #1265122 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/8] phy: rockchip-usb: correct pll handling and usb-uart Heiko Stuebner <heiko@sntech.de> - 2015-11-08 17:10 +0100
    [PATCH v2 1/8] phy: rockchip-usb: fix clock get-put mismatch Heiko Stuebner <heiko@sntech.de> - 2015-11-08 17:10 +0100
    Re: [PATCH v2 0/8] phy: rockchip-usb: correct pll handling and usb-uart Doug Anderson <dianders@chromium.org> - 2015-11-09 22:20 +0100
      Re: [PATCH v2 0/8] phy: rockchip-usb: correct pll handling and usb-uart Heiko Stuebner <heiko@sntech.de> - 2015-11-09 22:30 +0100
        Re: [PATCH v2 0/8] phy: rockchip-usb: correct pll handling and usb-uart Doug Anderson <dianders@chromium.org> - 2015-11-09 22:40 +0100
          Re: [PATCH v2 0/8] phy: rockchip-usb: correct pll handling and usb-uart Heiko Stuebner <heiko@sntech.de> - 2015-11-09 22:50 +0100

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

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


Patches 1-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: expose the phy-internal PLLs
  clk: rockchip: fix usbphy-related clocks
  ARM: dts: rockchip: add clock-cells for usb phy nodes
  ARM: dts: rockchip: assign usbphy480m_src to the new usbphy pll on
    veyron
  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                     | 449 ++++++++++++++++++---
 9 files changed, 416 insertions(+), 81 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]

#1265123 — [PATCH v2 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 | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/phy/phy-rockchip-usb.c b/drivers/phy/phy-rockchip-usb.c
index 91d6f34..dfc056b 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;
@@ -124,6 +132,13 @@ static int rockchip_usb_phy_probe(struct platform_device *pdev)
 		if (IS_ERR(rk_phy->clk))
 			rk_phy->clk = NULL;
 
+		err = devm_add_action(dev, rockchip_usb_phy_action, rk_phy);
+		if (err) {
+			if (rk_phy->clk)
+				clk_put(rk_phy->clk);
+			return err;
+		}
+
 		rk_phy->phy = devm_phy_create(dev, child, &ops);
 		if (IS_ERR(rk_phy->phy)) {
 			dev_err(dev, "failed to create PHY\n");
-- 
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]

#1266015

Heiko,

On Sun, Nov 8, 2015 at 8:04 AM, Heiko Stuebner <heiko@sntech.de> wrote:
> 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
>
>
> Patches 1-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: expose the phy-internal PLLs
>   clk: rockchip: fix usbphy-related clocks
>   ARM: dts: rockchip: add clock-cells for usb phy nodes
>   ARM: dts: rockchip: assign usbphy480m_src to the new usbphy pll on
>     veyron
>   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                     | 449 ++++++++++++++++++---
>  9 files changed, 416 insertions(+), 81 deletions(-)

If you happened to be in the mood for cleaning up this PHY and wanted
to fix up one more thing that I noticed...

...you could actually increase the range of registers managed by the
PHYs.  For instance, in rk3288, the "host1" port isn't just managed by
1 register, but by 4 (GRF_UOC2_CON0 - GRF_UOC2_CON3).  I think there
are 5 for the OTG port.

Obviously not required for this series and there's no (current) reason
to do anything with the rest of those registers, but it might be
interesting for the future...

-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]

#1266028

Am Montag, 9. November 2015, 13:11:18 schrieb Doug Anderson:
> Heiko,
> 
> On Sun, Nov 8, 2015 at 8:04 AM, Heiko Stuebner <heiko@sntech.de> wrote:
> > 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
> >
> >
> > Patches 1-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: expose the phy-internal PLLs
> >   clk: rockchip: fix usbphy-related clocks
> >   ARM: dts: rockchip: add clock-cells for usb phy nodes
> >   ARM: dts: rockchip: assign usbphy480m_src to the new usbphy pll on
> >     veyron
> >   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                     | 449 ++++++++++++++++++---
> >  9 files changed, 416 insertions(+), 81 deletions(-)
> 
> If you happened to be in the mood for cleaning up this PHY and wanted
> to fix up one more thing that I noticed...
> 
> ...you could actually increase the range of registers managed by the
> PHYs.  For instance, in rk3288, the "host1" port isn't just managed by
> 1 register, but by 4 (GRF_UOC2_CON0 - GRF_UOC2_CON3).  I think there
> are 5 for the OTG port.
> 
> Obviously not required for this series and there's no (current) reason
> to do anything with the rest of those registers, but it might be
> interesting for the future...

I'm not sure what change you're proposing :-) .

I currently see the reg-property both in the dts and in the code as
"offset" - the start-register, because it points to uoc_con0 for each phy.
So if needed I was just planning on doing reg+x , as they're coming from
the GRF anyway.

One interesting point would be to move that under the GRF node, similar
to what we're doing with the power-domains under the PMU.

In retrospect I think exposing the phys individually was not the best
decision - especially as when you dive deeper, they are not that similar
anymore and individual functions change. But we'll have to live with that.

That is by the way, also one of the reasons for having these per-phy
structs, so that we have the possility to identify phys again and describe
individual phy-functions but still keep the current binding.
The usb-uart is the first user for that :-)

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]

#1266042

Heiko,

On Mon, Nov 9, 2015 at 1:27 PM, Heiko Stuebner <heiko@sntech.de> wrote:
>> If you happened to be in the mood for cleaning up this PHY and wanted
>> to fix up one more thing that I noticed...
>>
>> ...you could actually increase the range of registers managed by the
>> PHYs.  For instance, in rk3288, the "host1" port isn't just managed by
>> 1 register, but by 4 (GRF_UOC2_CON0 - GRF_UOC2_CON3).  I think there
>> are 5 for the OTG port.
>>
>> Obviously not required for this series and there's no (current) reason
>> to do anything with the rest of those registers, but it might be
>> interesting for the future...
>
> I'm not sure what change you're proposing :-) .

I was proposing changing the PHY to look like:

usbphy: phy {
  compatible = "rockchip,rk3288-usb-phy";
  rockchip,grf = <&grf>;
  #address-cells = <1>;
  #size-cells = <1>;
  status = "disabled";

  usbphy0: usb-phy0 {
    #phy-cells = <0>;
    reg = <0x320 0x14>;
    clocks = <&cru SCLK_OTGPHY0>;
    clock-names = "phyclk";
  };
  ...
};

...in other words change the "size-cells" for the main PHY to 1 and
add a length to the registers.


> I currently see the reg-property both in the dts and in the code as
> "offset" - the start-register, because it points to uoc_con0 for each phy.
> So if needed I was just planning on doing reg+x , as they're coming from
> the GRF anyway.

Yeah, that would work.  I was just trying to make it more obvious in
the DTS that there was actually a range of registers managed here...


> One interesting point would be to move that under the GRF node, similar
> to what we're doing with the power-domains under the PMU.
>
> In retrospect I think exposing the phys individually was not the best
> decision - especially as when you dive deeper, they are not that similar
> anymore and individual functions change. But we'll have to live with that.

Now it's my turn: I'm not sure I understand...  ;)
--
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]

#1266047

Am Montag, 9. November 2015, 13:32:28 schrieb Doug Anderson:
> Heiko,
> 
> On Mon, Nov 9, 2015 at 1:27 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> >> If you happened to be in the mood for cleaning up this PHY and wanted
> >> to fix up one more thing that I noticed...
> >>
> >> ...you could actually increase the range of registers managed by the
> >> PHYs.  For instance, in rk3288, the "host1" port isn't just managed by
> >> 1 register, but by 4 (GRF_UOC2_CON0 - GRF_UOC2_CON3).  I think there
> >> are 5 for the OTG port.
> >>
> >> Obviously not required for this series and there's no (current) reason
> >> to do anything with the rest of those registers, but it might be
> >> interesting for the future...
> >
> > I'm not sure what change you're proposing :-) .
> 
> I was proposing changing the PHY to look like:
> 
> usbphy: phy {
>   compatible = "rockchip,rk3288-usb-phy";
>   rockchip,grf = <&grf>;
>   #address-cells = <1>;
>   #size-cells = <1>;
>   status = "disabled";
> 
>   usbphy0: usb-phy0 {
>     #phy-cells = <0>;
>     reg = <0x320 0x14>;
>     clocks = <&cru SCLK_OTGPHY0>;
>     clock-names = "phyclk";
>   };
>   ...
> };
> 
> ...in other words change the "size-cells" for the main PHY to 1 and
> add a length to the registers.
> 
> 
> > I currently see the reg-property both in the dts and in the code as
> > "offset" - the start-register, because it points to uoc_con0 for each phy.
> > So if needed I was just planning on doing reg+x , as they're coming from
> > the GRF anyway.
> 
> Yeah, that would work.  I was just trying to make it more obvious in
> the DTS that there was actually a range of registers managed here...

ok, that would make sense to make that a bit more visible, and it
would even require functional changes ;-)


> > One interesting point would be to move that under the GRF node, similar
> > to what we're doing with the power-domains under the PMU.
> >
> > In retrospect I think exposing the phys individually was not the best
> > decision - especially as when you dive deeper, they are not that similar
> > anymore and individual functions change. But we'll have to live with that.
> 
> Now it's my turn: I'm not sure I understand...  ;)

as the driver currently stands (without my changes) every phy is seen as
the same function block - it looks like phy0...phy2 have the same
functionality and the driver cannot distinguish between them. The
similarity is true for SIDDQ and some other generic bits (even across
different Rockchip SoCs), but for more special functionality bits move
their location between phys. This is even more visisble if you compare
rk3066/rk3188/rk3288 phys.

What I mean, and would like to have for the new phy-IP for rk3368 etc
would be to have one node, then do the reference via an index like 
<&usbphy 1> and hide the internals in the driver instead of having
that in the dts.

That's why it's nice to be able to reidentify the phy in the driver again
via the register/structs introduced, because now you can again know that
phyX is actually the OTG phy etc.


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] | [standalone]

Back to top | Article view | linux.kernel

csiph-web