| Message ID | 20181127092824.22756-5-peter.chen@nxp.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | usb: chipidea: imx: add HSIC support | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
Hi Peter, On Tue, Nov 27, 2018 at 7:31 AM PETER CHEN <peter.chen@nxp.com> wrote: > > For USB HSIC, the data and strobe pin needs to be pulled down > at default, we consider it as "idle" state. When the USB host > is ready to be used, the strobe pin needs to be pulled up, > we consider it as "active" state. > > Signed-off-by: Peter Chen <peter.chen@nxp.com> > --- > Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > index 529e51879fb2..1e5e7ddfb1a5 100644 > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > @@ -80,7 +80,10 @@ Optional properties: > controller. It's expected that a mux state of 0 indicates device mode and a > mux state of 1 indicates host mode. > - mux-control-names: Shall be "usb_switch" if mux-controls is specified. > -- pinctrl-names: Names for optional pin modes in "default", "host", "device" > +- pinctrl-names: Names for optional pin modes in "default", "host", "device". > + In case of HSIC-mode, "idle" and "active" pin modes are mandatory. In this > + case, the "idle" state needs to pull down the data and strobe pin > + and the "active" state needs to pull up the strobe pin. > - pinctrl-n: alternate pin modes > > i.mx specific properties > @@ -110,4 +113,7 @@ Example: > phy-clkgate-delay-us = <400>; > mux-controls = <&usb_switch>; > mux-control-names = "usb_switch"; > + pinctrl-names = "idle", "active"; > + pinctrl-0 = <&pinctrl_usbh2_1>; > + pinctrl-1 = <&pinctrl_usbh2_2>; I would put the "idle" and "active" entries inside a explicit section for HSIC. Otherwise, it may confuse people using non-HSIC bindings.
> > On Tue, Nov 27, 2018 at 7:31 AM PETER CHEN <peter.chen@nxp.com> wrote: > > > > For USB HSIC, the data and strobe pin needs to be pulled down at > > default, we consider it as "idle" state. When the USB host is ready to > > be used, the strobe pin needs to be pulled up, we consider it as > > "active" state. > > > > Signed-off-by: Peter Chen <peter.chen@nxp.com> > > --- > > Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > > b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > > index 529e51879fb2..1e5e7ddfb1a5 100644 > > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > > @@ -80,7 +80,10 @@ Optional properties: > > controller. It's expected that a mux state of 0 indicates device mode and a > > mux state of 1 indicates host mode. > > - mux-control-names: Shall be "usb_switch" if mux-controls is specified. > > -- pinctrl-names: Names for optional pin modes in "default", "host", "device" > > +- pinctrl-names: Names for optional pin modes in "default", "host", "device". > > + In case of HSIC-mode, "idle" and "active" pin modes are mandatory. > > +In this > > + case, the "idle" state needs to pull down the data and strobe pin > > + and the "active" state needs to pull up the strobe pin. > > - pinctrl-n: alternate pin modes > > > > i.mx specific properties > > @@ -110,4 +113,7 @@ Example: > > phy-clkgate-delay-us = <400>; > > mux-controls = <&usb_switch>; > > mux-control-names = "usb_switch"; > > + pinctrl-names = "idle", "active"; > > + pinctrl-0 = <&pinctrl_usbh2_1>; > > + pinctrl-1 = <&pinctrl_usbh2_2>; > > I would put the "idle" and "active" entries inside a explicit section for HSIC. > > Otherwise, it may confuse people using non-HSIC bindings. It is just an example; the user should check the meaning for optional properties before using it, like the property "phy-clkgate-delay-us", only imx7d/7s needs it. Peter
Hi Peter, hi Fabio, On 30.11.18 03:33, PETER CHEN wrote: > >> >> On Tue, Nov 27, 2018 at 7:31 AM PETER CHEN <peter.chen@nxp.com> wrote: >>> >>> For USB HSIC, the data and strobe pin needs to be pulled down at >>> default, we consider it as "idle" state. When the USB host is ready to >>> be used, the strobe pin needs to be pulled up, we consider it as >>> "active" state. >>> >>> Signed-off-by: Peter Chen <peter.chen@nxp.com> >>> --- >>> Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt | 8 +++++++- >>> 1 file changed, 7 insertions(+), 1 deletion(-) >>> >>> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt >>> b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt >>> index 529e51879fb2..1e5e7ddfb1a5 100644 >>> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt >>> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt >>> @@ -80,7 +80,10 @@ Optional properties: >>> controller. It's expected that a mux state of 0 indicates device mode and a >>> mux state of 1 indicates host mode. >>> - mux-control-names: Shall be "usb_switch" if mux-controls is specified. >>> -- pinctrl-names: Names for optional pin modes in "default", "host", "device" >>> +- pinctrl-names: Names for optional pin modes in "default", "host", "device". >>> + In case of HSIC-mode, "idle" and "active" pin modes are mandatory. >>> +In this >>> + case, the "idle" state needs to pull down the data and strobe pin >>> + and the "active" state needs to pull up the strobe pin. >>> - pinctrl-n: alternate pin modes >>> >>> i.mx specific properties >>> @@ -110,4 +113,7 @@ Example: >>> phy-clkgate-delay-us = <400>; >>> mux-controls = <&usb_switch>; >>> mux-control-names = "usb_switch"; >>> + pinctrl-names = "idle", "active"; >>> + pinctrl-0 = <&pinctrl_usbh2_1>; >>> + pinctrl-1 = <&pinctrl_usbh2_2>; >> >> I would put the "idle" and "active" entries inside a explicit section for HSIC. >> >> Otherwise, it may confuse people using non-HSIC bindings. > > It is just an example; the user should check the meaning for optional properties before > using it, like the property "phy-clkgate-delay-us", only imx7d/7s needs it. There are many other optional properties for this driver and a lot of them are not in the given example. Maybe we should just keep the pinctrls for HSIC-mode out of the example, too? Regards, Frieder
Hi Frieder, On Tue, Dec 4, 2018 at 12:31 PM Schrempf Frieder <frieder.schrempf@kontron.de> wrote: > There are many other optional properties for this driver and a lot of > them are not in the given example. Maybe we should just keep the > pinctrls for HSIC-mode out of the example, too? I am just trying to make life easier for those who want to use HSIC support with chipidea. Can we just add a real dts snippet example of your board into the binding document?
Hi Fabio, On 04.12.18 21:01, Fabio Estevam wrote: > Hi Frieder, > > On Tue, Dec 4, 2018 at 12:31 PM Schrempf Frieder > <frieder.schrempf@kontron.de> wrote: > >> There are many other optional properties for this driver and a lot of >> them are not in the given example. Maybe we should just keep the >> pinctrls for HSIC-mode out of the example, too? > > I am just trying to make life easier for those who want to use HSIC > support with chipidea. > > Can we just add a real dts snippet example of your board into the > binding document? Sure, here is what I have in my dts: &usbh2 { pinctrl-names = "idle", "active"; pinctrl-0 = <&pinctrl_usbh2_idle>; pinctrl-1 = <&pinctrl_usbh2_active>; status = "okay"; #address-cells = <1>; #size-cells = <0>; usbnet: smsc@1 { compatible = "usb424,9730"; reg = <1>; }; }; @Peter: Can you add this as a second example to the binding documentation? Thanks, Frieder
> On 04.12.18 21:01, Fabio Estevam wrote: > > Hi Frieder, > > > > On Tue, Dec 4, 2018 at 12:31 PM Schrempf Frieder > > <frieder.schrempf@kontron.de> wrote: > > > >> There are many other optional properties for this driver and a lot of > >> them are not in the given example. Maybe we should just keep the > >> pinctrls for HSIC-mode out of the example, too? > > > > I am just trying to make life easier for those who want to use HSIC > > support with chipidea. > > > > Can we just add a real dts snippet example of your board into the > > binding document? > > Sure, here is what I have in my dts: > > &usbh2 { > pinctrl-names = "idle", "active"; > pinctrl-0 = <&pinctrl_usbh2_idle>; > pinctrl-1 = <&pinctrl_usbh2_active>; > status = "okay"; > #address-cells = <1>; > #size-cells = <0>; > > usbnet: smsc@1 { > compatible = "usb424,9730"; > reg = <1>; > }; > }; > > @Peter: Can you add this as a second example to the binding documentation? > So, there are two examples at binding-doc, one for normal, one for HSIC? Fabio, do you mean that? If DT maintainer agrees it too, I will add it. Peter
On 05.12.18 08:45, Schrempf Frieder wrote: > Hi Fabio, > > On 04.12.18 21:01, Fabio Estevam wrote: >> Hi Frieder, >> >> On Tue, Dec 4, 2018 at 12:31 PM Schrempf Frieder >> <frieder.schrempf@kontron.de> wrote: >> >>> There are many other optional properties for this driver and a lot of >>> them are not in the given example. Maybe we should just keep the >>> pinctrls for HSIC-mode out of the example, too? >> >> I am just trying to make life easier for those who want to use HSIC >> support with chipidea. >> >> Can we just add a real dts snippet example of your board into the >> binding document? > > Sure, here is what I have in my dts: > > &usbh2 { > pinctrl-names = "idle", "active"; > pinctrl-0 = <&pinctrl_usbh2_idle>; > pinctrl-1 = <&pinctrl_usbh2_active>; > status = "okay"; > #address-cells = <1>; > #size-cells = <0>; > > usbnet: smsc@1 { > compatible = "usb424,9730"; > reg = <1>; > }; > }; Or merged with the settings from imx6qdl.dtsi this will look like below. Maybe this is better as it is the complete node without depending on imx6qdl.dtsi: usbh2: usb@2184400 { compatible = "fsl,imx6q-usb", "fsl,imx27-usb"; reg = <0x02184400 0x200>; interrupts = <0 41 IRQ_TYPE_LEVEL_HIGH>; clocks = <&clks IMX6QDL_CLK_USBOH3>; fsl,usbphy = <&usbphynop1>; fsl,usbmisc = <&usbmisc 2>; phy_type = "hsic"; dr_mode = "host"; ahb-burst-config = <0x0>; tx-burst-size-dword = <0x10>; rx-burst-size-dword = <0x10>; pinctrl-names = "idle", "active"; pinctrl-0 = <&pinctrl_usbh2_idle>; pinctrl-1 = <&pinctrl_usbh2_active>; #address-cells = <1>; #size-cells = <0>; usbnet: smsc@1 { compatible = "usb424,9730"; reg = <1>; }; }; > > @Peter: Can you add this as a second example to the binding documentation? > > Thanks, > Frieder >
On Wed, Dec 5, 2018 at 5:57 AM PETER CHEN <peter.chen@nxp.com> wrote: > So, there are two examples at binding-doc, one for normal, one for HSIC? Fabio, do you > mean that? If DT maintainer agrees it too, I will add it. Yes, I think it will makes things clearer. Thanks
On Wed, Dec 05, 2018 at 07:57:37AM +0000, PETER CHEN wrote: > > > On 04.12.18 21:01, Fabio Estevam wrote: > > > Hi Frieder, > > > > > > On Tue, Dec 4, 2018 at 12:31 PM Schrempf Frieder > > > <frieder.schrempf@kontron.de> wrote: > > > > > >> There are many other optional properties for this driver and a lot of > > >> them are not in the given example. Maybe we should just keep the > > >> pinctrls for HSIC-mode out of the example, too? > > > > > > I am just trying to make life easier for those who want to use HSIC > > > support with chipidea. > > > > > > Can we just add a real dts snippet example of your board into the > > > binding document? > > > > Sure, here is what I have in my dts: > > > > &usbh2 { > > pinctrl-names = "idle", "active"; > > pinctrl-0 = <&pinctrl_usbh2_idle>; > > pinctrl-1 = <&pinctrl_usbh2_active>; > > status = "okay"; > > #address-cells = <1>; > > #size-cells = <0>; > > > > usbnet: smsc@1 { > > compatible = "usb424,9730"; > > reg = <1>; > > }; > > }; > > > > @Peter: Can you add this as a second example to the binding documentation? > > > > So, there are two examples at binding-doc, one for normal, one for HSIC? Fabio, do you > mean that? If DT maintainer agrees it too, I will add it. Okay.
diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt index 529e51879fb2..1e5e7ddfb1a5 100644 --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt @@ -80,7 +80,10 @@ Optional properties: controller. It's expected that a mux state of 0 indicates device mode and a mux state of 1 indicates host mode. - mux-control-names: Shall be "usb_switch" if mux-controls is specified. -- pinctrl-names: Names for optional pin modes in "default", "host", "device" +- pinctrl-names: Names for optional pin modes in "default", "host", "device". + In case of HSIC-mode, "idle" and "active" pin modes are mandatory. In this + case, the "idle" state needs to pull down the data and strobe pin + and the "active" state needs to pull up the strobe pin. - pinctrl-n: alternate pin modes i.mx specific properties @@ -110,4 +113,7 @@ Example: phy-clkgate-delay-us = <400>; mux-controls = <&usb_switch>; mux-control-names = "usb_switch"; + pinctrl-names = "idle", "active"; + pinctrl-0 = <&pinctrl_usbh2_1>; + pinctrl-1 = <&pinctrl_usbh2_2>; };
For USB HSIC, the data and strobe pin needs to be pulled down at default, we consider it as "idle" state. When the USB host is ready to be used, the strobe pin needs to be pulled up, we consider it as "active" state. Signed-off-by: Peter Chen <peter.chen@nxp.com> --- Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)