Hi,
On Sat, Nov 3, 2018 at 7:40 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
>
> Quoting Evan Green (2018-10-26 10:35:40)
> > (or)
> > @@ -150,3 +153,54 @@ Example:
> > ...
> > ...
> > };
> > +
> > + phy@88eb000 {
> > + compatible = "qcom,sdm845-qmp-usb3-uni-phy";
> > + reg = <0x88eb000 0x18c>;
> > + #clock-cells = <1>;
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + ranges;
> > +
> > + clocks = <&gcc GCC_USB3_SEC_PHY_AUX_CLK>,
> > + <&gcc GCC_USB_PHY_CFG_AHB2PHY_CLK>,
> > + <&gcc GCC_USB3_SEC_CLKREF_CLK>,
> > + <&gcc GCC_USB3_SEC_PHY_COM_AUX_CLK>;
> > + clock-names = "aux", "cfg_ahb", "ref", "com_aux";
> > +
> > + resets = <&gcc GCC_USB3PHY_PHY_SEC_BCR>,
> > + <&gcc GCC_USB3_PHY_SEC_BCR>;
> > + reset-names = "phy", "common";
> > +
> > + lane@88eb200 {
> > + reg = <0x88eb200 0x128>,
> > + <0x88eb400 0x1fc>,
> > + <0x88eb800 0x218>,
> > + <0x88eb600 0x70>;
> > + #phy-cells = <0>;
> > + clocks = <&gcc GCC_USB3_SEC_PHY_PIPE_CLK>;
> > + clock-names = "pipe0";
> > + clock-output-names = "usb3_uni_phy_pipe_clk_src";
>
> If this has clock-output-names then I would expect to see a #clock-cells
> property, but that isn't here in this node. Are we relying on the same
> property in the parent node?
If I had to guess, I believe it's yet more confusing than that. I
believe you actually point to the parent phandle if you want to use
the clock. I notice that the parent has #clock-cells as 1 so
presumably this is how you point to one child or the other? ...but I
don't think it's documented how this works. The lane nodes don't have
any sort of ID as far as I can tell. ...and in any case having
#clock-cells of 1 makes no sense for USB 3 PHYs which are supposed to
only have one child?
Let's look at the code, maybe? Hrm, phy_pipe_clk_register() takes no
ID or anything. Huh? OK, so as far as I can tell
of_clk_add_provider() is never called on this clock...
So I think the answer is that #clock-cells should be <0> and should
move to the child node to match with clock-output-names. Then I guess
(if anyone references this clock from the device tree rather than
relying on the global clock-output-names) we should add the
of_clk_add_provider() into the code?
Maybe we can add that as a patch to the end of this series? There are
so many crazy / random things wrong with these bindings that it makes
sense to make smaller / incremental changes?
-Doug
