| Message ID | 20181108070449.23572-2-shawn.guo@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Synopsys High-Speed USB PHY driver for Qualcomm SoCs | expand |
On 08-11-18, 15:04, Shawn Guo wrote: > From: Sriharsha Allenki <sallenki@codeaurora.org> > > It adds bindings for Synopsys 28nm femto phy controller that supports > LS/FS/HS usb connectivity on Qualcomm chipsets. > > Signed-off-by: Sriharsha Allenki <sallenki@codeaurora.org> > Signed-off-by: Anu Ramanathan <anur@codeaurora.org> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > --- > .../phy/qcom,snps-28nm-usb-hs-phy.txt | 101 ++++++++++++++++++ > 1 file changed, 101 insertions(+) > create mode 100644 Documentation/devicetree/bindings/phy/qcom,snps-28nm-usb-hs-phy.txt > > diff --git a/Documentation/devicetree/bindings/phy/qcom,snps-28nm-usb-hs-phy.txt b/Documentation/devicetree/bindings/phy/qcom,snps-28nm-usb-hs-phy.txt > new file mode 100644 > index 000000000000..75e7a09dd558 > --- /dev/null > +++ b/Documentation/devicetree/bindings/phy/qcom,snps-28nm-usb-hs-phy.txt > @@ -0,0 +1,101 @@ > +Qualcomm Synopsys 28nm Femto phy controller > +=========================================== > + > +Synopsys 28nm femto phy controller supports LS/FS/HS usb connectivity on > +Qualcomm chipsets. > + > +Required properties: > + > +- compatible: > + Value type: <string> > + Definition: Should contain "qcom,usb-snps-hsphy". > + > +- reg: > + Value type: <prop-encoded-array> > + Definition: USB PHY base address and length of the register map. > + > +- #phy-cells: > + Value type: <u32> > + Definition: Should be 0. I dont quite understand the definition that it should be 0, maybe you mean allowed value is 0, if so why have this property? -- ~Vinod
On 09-11-18, 14:31, Shawn Guo wrote: > On Fri, Nov 09, 2018 at 10:38:19AM +0530, Vinod Koul wrote: > > On 08-11-18, 15:04, Shawn Guo wrote: > > > + > > > +- #phy-cells: > > > + Value type: <u32> > > > + Definition: Should be 0. > > > > I dont quite understand the definition that it should be 0, maybe you > > mean allowed value is 0, if so why have this property? > > The property is defined by generic phy bindings phy/phy-bindings.txt. > I can add a pointer to it if you think that's necessary. The property > should be 0 for our device, because there is zero number cell in phy > specifier from dwc3 node as shown in the example. That makes sense, also does it make sense it mention the properties in phy/phy-bindings.txt, why not refer that here for the properties we use and vlaues. > > dwc3@78c0000 { > ... > phys = <&usb2_phy_prim>; > phy-names = "usb2-phy"; > } > > And for that reason, we can use the generic .of_xlate implementation > of_phy_simple_xlate() provided by phy core. There are some comments > in kernel doc of of_phy_simple_xlate() which might be helpful. > > Shawn -- ~Vinod
Hi Rob, On Mon, Nov 12, 2018 at 01:24:51PM -0600, Rob Herring wrote: > On Thu, Nov 08, 2018 at 03:04:48PM +0800, Shawn Guo wrote: > > From: Sriharsha Allenki <sallenki@codeaurora.org> > > > > It adds bindings for Synopsys 28nm femto phy controller that supports > > LS/FS/HS usb connectivity on Qualcomm chipsets. > > > > Signed-off-by: Sriharsha Allenki <sallenki@codeaurora.org> > > Signed-off-by: Anu Ramanathan <anur@codeaurora.org> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > > --- > > .../phy/qcom,snps-28nm-usb-hs-phy.txt | 101 ++++++++++++++++++ > > 1 file changed, 101 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/phy/qcom,snps-28nm-usb-hs-phy.txt > > > > diff --git a/Documentation/devicetree/bindings/phy/qcom,snps-28nm-usb-hs-phy.txt b/Documentation/devicetree/bindings/phy/qcom,snps-28nm-usb-hs-phy.txt > > new file mode 100644 > > index 000000000000..75e7a09dd558 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/phy/qcom,snps-28nm-usb-hs-phy.txt > > @@ -0,0 +1,101 @@ > > +Qualcomm Synopsys 28nm Femto phy controller > > +=========================================== > > + > > +Synopsys 28nm femto phy controller supports LS/FS/HS usb connectivity on > > +Qualcomm chipsets. > > + > > +Required properties: > > + > > +- compatible: > > + Value type: <string> > > + Definition: Should contain "qcom,usb-snps-hsphy". > > SoC specific compatible? Agreed. A SoC prefixed compatible would be more specific and scalable for handling different programming model of the same IP. I will use "qcom,qcs404-usb-hsphy" in v3. > > > + > > +- reg: > > + Value type: <prop-encoded-array> > > + Definition: USB PHY base address and length of the register map. > > + > > +- #phy-cells: > > + Value type: <u32> > > + Definition: Should be 0. > > + > > +- clocks: > > + Value type: <prop-encoded-array> > > + Definition: See clock-bindings.txt section "consumers". List of > > + three clock specifiers for reference, phy core and > > + sleep clocks. > > + > > +- clock-names: > > + Value type: <string> > > + Definition: Names of the clocks in 1-1 correspondence with the "clocks" > > + property. Must contain "ref", "phy" and "sleep". > > + > > +- resets: > > + Value type: <prop-encoded-array> > > + Definition: See reset.txt section "consumers". PHY reset specifiers > > + for phy core and POR resets. > > + > > +- reset-names: > > + Value type: <string> > > + Definition: Names of the resets in 1-1 correspondence with the "resets" > > + property. Must contain "phy" and "por". > > + > > +- vdd-supply: > > + Value type: <phandle> > > + Definition: phandle to the regulator VDD supply node. > > + > > +- vdda1p8-supply: > > + Value type: <phandle> > > + Definition: phandle to the regulator 1.8V supply node. > > + > > +- vdda3p3-supply: > > + Value type: <phandle> > > + Definition: phandle to the regulator 3.3V supply node. > > + > > +- qcom,vdd-voltage-level: > > + Value type: <prop-array> > > + Definition: This is a list of three integer values <no min max> where > > + each value corresponding to voltage corner in uV. > > + > > +Optional properties: > > + > > +- extcon: > > + Value type: <prop-encoded-array> > > + Definition: Should contain the vbus extcon. > > Don't use extcon for new bindings. Use usb-connector binding. Okay, I just did a bit of research and found that 'extcon' is becoming a deprecated DT property recently and we should OF graph bindings to specify the connector. Will do in v3. > > > + > > +- qcom,init-seq: > > + Value type: <u32 array> > > + Definition: Should contain a sequence of <offset value delay> tuples to > > + program 'value' into phy register at 'offset' with 'delay' > > + in us afterwards. > > If we wanted this type of thing in DT, we'd have a generic binding (or > forth). Right now, this is a qualcomm usb phy specific bindings - first used in qcom,usb-hs-phy.txt and I extended it a bit for my phy. As this is not a so good hardware description, I'm a little hesitated to make it generic for other platforms to use in general. What about we put off it a little bit until we see more platforms need the same thing? Shawn > This should probably be split between SoC specific settings in > the driver and board properties in DT.
Hi Sriharsha, On Tue, Nov 13, 2018 at 11:42 AM Shawn Guo <shawn.guo@linaro.org> wrote: <snip> > > > +- qcom,init-seq: > > > + Value type: <u32 array> > > > + Definition: Should contain a sequence of <offset value delay> tuples to > > > + program 'value' into phy register at 'offset' with 'delay' > > > + in us afterwards. > > > > If we wanted this type of thing in DT, we'd have a generic binding (or > > forth). > > Right now, this is a qualcomm usb phy specific bindings - first used in > qcom,usb-hs-phy.txt and I extended it a bit for my phy. As this is not > a so good hardware description, I'm a little hesitated to make it > generic for other platforms to use in general. What about we put off it > a little bit until we see more platforms need the same thing? Are those register write sequences really required here? At least, from the test I do, it still works with this property dropped. Shawn
On Mon, Nov 12, 2018 at 9:42 PM Shawn Guo <shawn.guo@linaro.org> wrote: > > Hi Rob, > > On Mon, Nov 12, 2018 at 01:24:51PM -0600, Rob Herring wrote: > > On Thu, Nov 08, 2018 at 03:04:48PM +0800, Shawn Guo wrote: > > > From: Sriharsha Allenki <sallenki@codeaurora.org> > > > > > > It adds bindings for Synopsys 28nm femto phy controller that supports > > > LS/FS/HS usb connectivity on Qualcomm chipsets. > > > > > > Signed-off-by: Sriharsha Allenki <sallenki@codeaurora.org> > > > Signed-off-by: Anu Ramanathan <anur@codeaurora.org> > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > > > --- > > > .../phy/qcom,snps-28nm-usb-hs-phy.txt | 101 ++++++++++++++++++ > > > 1 file changed, 101 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/phy/qcom,snps-28nm-usb-hs-phy.txt > > > > > > diff --git a/Documentation/devicetree/bindings/phy/qcom,snps-28nm-usb-hs-phy.txt b/Documentation/devicetree/bindings/phy/qcom,snps-28nm-usb-hs-phy.txt > > > new file mode 100644 > > > index 000000000000..75e7a09dd558 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/phy/qcom,snps-28nm-usb-hs-phy.txt > > > @@ -0,0 +1,101 @@ > > > +Qualcomm Synopsys 28nm Femto phy controller > > > +=========================================== > > > + > > > +Synopsys 28nm femto phy controller supports LS/FS/HS usb connectivity on > > > +Qualcomm chipsets. > > > + > > > +Required properties: > > > + > > > +- compatible: > > > + Value type: <string> > > > + Definition: Should contain "qcom,usb-snps-hsphy". > > > > SoC specific compatible? > > Agreed. A SoC prefixed compatible would be more specific and scalable > for handling different programming model of the same IP. I will use > "qcom,qcs404-usb-hsphy" in v3. > > > > > > + > > > +- reg: > > > + Value type: <prop-encoded-array> > > > + Definition: USB PHY base address and length of the register map. > > > + > > > +- #phy-cells: > > > + Value type: <u32> > > > + Definition: Should be 0. > > > + > > > +- clocks: > > > + Value type: <prop-encoded-array> > > > + Definition: See clock-bindings.txt section "consumers". List of > > > + three clock specifiers for reference, phy core and > > > + sleep clocks. > > > + > > > +- clock-names: > > > + Value type: <string> > > > + Definition: Names of the clocks in 1-1 correspondence with the "clocks" > > > + property. Must contain "ref", "phy" and "sleep". > > > + > > > +- resets: > > > + Value type: <prop-encoded-array> > > > + Definition: See reset.txt section "consumers". PHY reset specifiers > > > + for phy core and POR resets. > > > + > > > +- reset-names: > > > + Value type: <string> > > > + Definition: Names of the resets in 1-1 correspondence with the "resets" > > > + property. Must contain "phy" and "por". > > > + > > > +- vdd-supply: > > > + Value type: <phandle> > > > + Definition: phandle to the regulator VDD supply node. > > > + > > > +- vdda1p8-supply: > > > + Value type: <phandle> > > > + Definition: phandle to the regulator 1.8V supply node. > > > + > > > +- vdda3p3-supply: > > > + Value type: <phandle> > > > + Definition: phandle to the regulator 3.3V supply node. > > > + > > > +- qcom,vdd-voltage-level: > > > + Value type: <prop-array> > > > + Definition: This is a list of three integer values <no min max> where > > > + each value corresponding to voltage corner in uV. > > > + > > > +Optional properties: > > > + > > > +- extcon: > > > + Value type: <prop-encoded-array> > > > + Definition: Should contain the vbus extcon. > > > > Don't use extcon for new bindings. Use usb-connector binding. > > Okay, I just did a bit of research and found that 'extcon' is becoming > a deprecated DT property recently and we should OF graph bindings to > specify the connector. Will do in v3. > > > > > > + > > > +- qcom,init-seq: > > > + Value type: <u32 array> > > > + Definition: Should contain a sequence of <offset value delay> tuples to > > > + program 'value' into phy register at 'offset' with 'delay' > > > + in us afterwards. > > > > If we wanted this type of thing in DT, we'd have a generic binding (or > > forth). > > Right now, this is a qualcomm usb phy specific bindings - first used in > qcom,usb-hs-phy.txt and I extended it a bit for my phy. As this is not > a so good hardware description, I'm a little hesitated to make it > generic for other platforms to use in general. What about we put off it > a little bit until we see more platforms need the same thing? I'm not saying I want it generic. Quite the opposite. I don't think we should have it either generically or vendor specific. The main thing I have a problem with is the timing information because then we're more that just data. Without that we're talking about a bunch of properties for register fields or just raw register values in DT. That becomes more of a judgement call. There's not too much value in making a driver translate a bunch of properties just to stuff them into registers on init. But then just allowing any raw register value to be in DT could be easily abused. Rob
On Sat, Nov 17, 2018 at 09:13:38AM -0600, Rob Herring wrote: <snip> > > > > +- qcom,init-seq: > > > > + Value type: <u32 array> > > > > + Definition: Should contain a sequence of <offset value delay> tuples to > > > > + program 'value' into phy register at 'offset' with 'delay' > > > > + in us afterwards. > > > > > > If we wanted this type of thing in DT, we'd have a generic binding (or > > > forth). > > > > Right now, this is a qualcomm usb phy specific bindings - first used in > > qcom,usb-hs-phy.txt and I extended it a bit for my phy. As this is not > > a so good hardware description, I'm a little hesitated to make it > > generic for other platforms to use in general. What about we put off it > > a little bit until we see more platforms need the same thing? > > I'm not saying I want it generic. Quite the opposite. I don't think we > should have it either generically or vendor specific. The main thing I > have a problem with is the timing information because then we're more > that just data. Without that we're talking about a bunch of properties > for register fields or just raw register values in DT. That becomes > more of a judgement call. There's not too much value in making a > driver translate a bunch of properties just to stuff them into > registers on init. But then just allowing any raw register value to be > in DT could be easily abused. Rob, I agree with your comments. Honestly, I'm not comfortable with this 'qcom,init-seq' thing in the first impression. The similar existence in mainline qcom,usb-hs-phy.txt makes me think it might be acceptable with the timing data added. Okay, I know your position on this now. @Sriharsha, Seeing that 'qcom,init-seq' is being configured with the exactly same values for both HS phys in SoC level dts file (qcs404.dtsi), I think such settings can be moved into driver code as SoC specific data. Unless you have a different view on this, I will do it with v4. Shawn
On Mon, Nov 19, 2018 at 12:29 PM Shawn Guo <shawn.guo@linaro.org> wrote: > > On Sat, Nov 17, 2018 at 09:13:38AM -0600, Rob Herring wrote: > <snip> > > > > > +- qcom,init-seq: > > > > > + Value type: <u32 array> > > > > > + Definition: Should contain a sequence of <offset value delay> tuples to > > > > > + program 'value' into phy register at 'offset' with 'delay' > > > > > + in us afterwards. > > > > > > > > If we wanted this type of thing in DT, we'd have a generic binding (or > > > > forth). > > > > > > Right now, this is a qualcomm usb phy specific bindings - first used in > > > qcom,usb-hs-phy.txt and I extended it a bit for my phy. As this is not > > > a so good hardware description, I'm a little hesitated to make it > > > generic for other platforms to use in general. What about we put off it > > > a little bit until we see more platforms need the same thing? > > > > I'm not saying I want it generic. Quite the opposite. I don't think we > > should have it either generically or vendor specific. The main thing I > > have a problem with is the timing information because then we're more > > that just data. Without that we're talking about a bunch of properties > > for register fields or just raw register values in DT. That becomes > > more of a judgement call. There's not too much value in making a > > driver translate a bunch of properties just to stuff them into > > registers on init. But then just allowing any raw register value to be > > in DT could be easily abused. > > Rob, > > I agree with your comments. Honestly, I'm not comfortable with this > 'qcom,init-seq' thing in the first impression. The similar existence in > mainline qcom,usb-hs-phy.txt makes me think it might be acceptable with > the timing data added. Okay, I know your position on this now. > > @Sriharsha, > > Seeing that 'qcom,init-seq' is being configured with the exactly same > values for both HS phys in SoC level dts file (qcs404.dtsi), I think > such settings can be moved into driver code as SoC specific data. > Unless you have a different view on this, I will do it with v4. phy-qcom-qmp and phy-qcom-qusb2 have been maintaining such SoC specific init sequences in the drivers if you would like to have pointers from them. Thanks Vivek > > Shawn -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
diff --git a/Documentation/devicetree/bindings/phy/qcom,snps-28nm-usb-hs-phy.txt b/Documentation/devicetree/bindings/phy/qcom,snps-28nm-usb-hs-phy.txt new file mode 100644 index 000000000000..75e7a09dd558 --- /dev/null +++ b/Documentation/devicetree/bindings/phy/qcom,snps-28nm-usb-hs-phy.txt @@ -0,0 +1,101 @@ +Qualcomm Synopsys 28nm Femto phy controller +=========================================== + +Synopsys 28nm femto phy controller supports LS/FS/HS usb connectivity on +Qualcomm chipsets. + +Required properties: + +- compatible: + Value type: <string> + Definition: Should contain "qcom,usb-snps-hsphy". + +- reg: + Value type: <prop-encoded-array> + Definition: USB PHY base address and length of the register map. + +- #phy-cells: + Value type: <u32> + Definition: Should be 0. + +- clocks: + Value type: <prop-encoded-array> + Definition: See clock-bindings.txt section "consumers". List of + three clock specifiers for reference, phy core and + sleep clocks. + +- clock-names: + Value type: <string> + Definition: Names of the clocks in 1-1 correspondence with the "clocks" + property. Must contain "ref", "phy" and "sleep". + +- resets: + Value type: <prop-encoded-array> + Definition: See reset.txt section "consumers". PHY reset specifiers + for phy core and POR resets. + +- reset-names: + Value type: <string> + Definition: Names of the resets in 1-1 correspondence with the "resets" + property. Must contain "phy" and "por". + +- vdd-supply: + Value type: <phandle> + Definition: phandle to the regulator VDD supply node. + +- vdda1p8-supply: + Value type: <phandle> + Definition: phandle to the regulator 1.8V supply node. + +- vdda3p3-supply: + Value type: <phandle> + Definition: phandle to the regulator 3.3V supply node. + +- qcom,vdd-voltage-level: + Value type: <prop-array> + Definition: This is a list of three integer values <no min max> where + each value corresponding to voltage corner in uV. + +Optional properties: + +- extcon: + Value type: <prop-encoded-array> + Definition: Should contain the vbus extcon. + +- qcom,init-seq: + Value type: <u32 array> + Definition: Should contain a sequence of <offset value delay> tuples to + program 'value' into phy register at 'offset' with 'delay' + in us afterwards. + +Example: + + phy@7a000 { + compatible = "qcom,usb-snps-hsphy"; + reg = <0x7a000 0x200>; + #phy-cells = <0>; + clocks = <&rpmcc RPM_SMD_LN_BB_CLK>, + <&gcc GCC_USB_HS_PHY_CFG_AHB_CLK>, + <&gcc GCC_USB2A_PHY_SLEEP_CLK>; + clock-names = "ref", "phy", "sleep"; + resets = <&gcc GCC_USB_HS_PHY_CFG_AHB_BCR>, + <&gcc GCC_USB2A_PHY_BCR>; + reset-names = "phy", "por"; + vdd-supply = <&vreg_l4_1p2>; + vdda1p8-supply = <&vreg_l5_1p8>; + vdda3p3-supply = <&vreg_l12_3p3>; + qcom,vdd-voltage-level = <0 1144000 1200000>; + qcom,init-seq = <0xc0 0x01 0>, + <0xe8 0x0d 0>, + <0x74 0x12 0>, + <0x98 0x63 0>, + <0x9c 0x03 0>, + <0xa0 0x1d 0>, + <0xa4 0x03 0>, + <0x8c 0x23 0>, + <0x78 0x08 0>, + <0x7c 0xdc 0>, + <0x90 0xe0 20>, + <0x74 0x10 0>, + <0x90 0x60 0>; + };