diff mbox series

[1/4] dt-bindings: usb: musb: Add support for MediaTek musb controller

Message ID 1545896066-897-2-git-send-email-min.guo@mediatek.com (mailing list archive)
State Superseded
Headers show
Series Add MediaTek MUSB Controller Driver | expand

Commit Message

Min Guo Dec. 27, 2018, 7:34 a.m. UTC 
From: Min Guo <min.guo@mediatek.com>

This adds support for MediaTek musb controller in
host, peripheral and otg mode

Signed-off-by: Min Guo <min.guo@mediatek.com>
---
 .../devicetree/bindings/usb/mediatek,musb.txt      | 49 ++++++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/mediatek,musb.txt

Comments

Rob Herring (Arm) Jan. 3, 2019, 10:14 p.m. UTC | #1 
On Thu, Dec 27, 2018 at 03:34:23PM +0800, min.guo@mediatek.com wrote:
> From: Min Guo <min.guo@mediatek.com>
> 
> This adds support for MediaTek musb controller in
> host, peripheral and otg mode
> 
> Signed-off-by: Min Guo <min.guo@mediatek.com>
> ---
>  .../devicetree/bindings/usb/mediatek,musb.txt      | 49 ++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/mediatek,musb.txt
> 
> diff --git a/Documentation/devicetree/bindings/usb/mediatek,musb.txt b/Documentation/devicetree/bindings/usb/mediatek,musb.txt
> new file mode 100644
> index 0000000..e899c9b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/mediatek,musb.txt
> @@ -0,0 +1,49 @@
> +MediaTek musb DRC/OTG controller
> +-------------------------------------------
> +
> +Required properties:
> + - compatible      : should be "mediatek,<soc-model>-musb",
> +   "mediatek,mtk-musb", soc-model is the name of SoC, such as
> +   mt2701, when using "mediatek,mtk-musb" compatible string, you
> +   need SoC specific ones in addition, one of:
> +   - "mediatek,mt2701-musb"

This isn't very clear. Just drop the <soc-model and enumerate the SoCs:

compatible: should be one of:
    "mediatek,mt2701-musb"
    ...
  followed by "mediatek,mtk-musb"

> + - reg             : specifies physical base address and size of
> +   the registers
> + - interrupts      : interrupt used by musb controller
> + - interrupt-names : must be "mc"

-names is pointless when there is only one.

> + - phys            : PHY specifier for the OTG phy
> + - phy-names       : should be "usb2-phy"

Same here.

> + - dr_mode         : should be one of "host", "peripheral" or "otg",
> +   refer to usb/generic.txt
> + - clocks          : a list of phandle + clock-specifier pairs, one for
> +   each entry in clock-names
> + - clock-names     : must contain "main","mcu","univpll"

space needed after each comma.

> +   for clocks of controller
> +
> +Optional properties:
> + - extcon : external connector for VBUS and IDPIN changes detection,
> +   needed when supports dual-role mode.

Don't use extcon for new bindings. The usb-connector binding should be 
used instead.

> + - vbus-supply : reference to the VBUS regulator, needed when supports
> +   dual-role mode.

The controller is powered from Vbus? Probably not. This belongs in the 
connector or maybe the phy (if the phy is powered from Vbus).

> + - power-domains   : a phandle to USB power domain node to control USB's
> +   MTCMOS
> +
> +Example:
> +
> +usb2: usb@11200000 {
> +	compatible = "mediatek,mt2701-musb";
> +		"mediatek,mtk-musb";
> +	reg = <0 0x11200000 0 0x1000>;
> +	interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_LOW>;
> +	interrupt-names = "mc";
> +	phys = <&u2port2 PHY_TYPE_USB2>;
> +	phy-names = "usb2-phy";
> +	vbus-supply = <&usb_vbus>;
> +	extcon = <&extcon_usb>;
> +	dr_mode = "otg";
> +	clocks = <&pericfg CLK_PERI_USB0>,
> +		 <&pericfg CLK_PERI_USB0_MCU>,
> +		 <&pericfg CLK_PERI_USB_SLV>;
> +	clock-names = "main","mcu","univpll";
> +	power-domains = <&scpsys MT2701_POWER_DOMAIN_IFR_MSC>;
> +};
> -- 
> 1.9.1
>
Min Guo Jan. 4, 2019, 3 a.m. UTC | #2 
On Thu, 2019-01-03 at 16:14 -0600, Rob Herring wrote:
> On Thu, Dec 27, 2018 at 03:34:23PM +0800, min.guo@mediatek.com wrote:
> > From: Min Guo <min.guo@mediatek.com>
> > 
> > This adds support for MediaTek musb controller in
> > host, peripheral and otg mode
> > 
> > Signed-off-by: Min Guo <min.guo@mediatek.com>
> > ---
> >  .../devicetree/bindings/usb/mediatek,musb.txt      | 49 ++++++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/usb/mediatek,musb.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/mediatek,musb.txt b/Documentation/devicetree/bindings/usb/mediatek,musb.txt
> > new file mode 100644
> > index 0000000..e899c9b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/mediatek,musb.txt
> > @@ -0,0 +1,49 @@
> > +MediaTek musb DRC/OTG controller
> > +-------------------------------------------
> > +
> > +Required properties:
> > + - compatible      : should be "mediatek,<soc-model>-musb",
> > +   "mediatek,mtk-musb", soc-model is the name of SoC, such as
> > +   mt2701, when using "mediatek,mtk-musb" compatible string, you
> > +   need SoC specific ones in addition, one of:
> > +   - "mediatek,mt2701-musb"
> 
> This isn't very clear. Just drop the <soc-model and enumerate the SoCs:
> 
> compatible: should be one of:
>     "mediatek,mt2701-musb"
>     ...
>   followed by "mediatek,mtk-musb"
I will modify it in the next patch.

> > + - reg             : specifies physical base address and size of
> > +   the registers
> > + - interrupts      : interrupt used by musb controller
> > + - interrupt-names : must be "mc"
> 
> -names is pointless when there is only one.
The MUSB core driver has two interrupts, one is for MAC, another for DMA,
but on MTK platform, there is only a MAC interrupt, here following the binding
of MUSB core driver.

> > + - phys            : PHY specifier for the OTG phy
> > + - phy-names       : should be "usb2-phy"
> 
> Same here.
I will modify it in the next patch.

> > + - dr_mode         : should be one of "host", "peripheral" or "otg",
> > +   refer to usb/generic.txt
> > + - clocks          : a list of phandle + clock-specifier pairs, one for
> > +   each entry in clock-names
> > + - clock-names     : must contain "main","mcu","univpll"
> 
> space needed after each comma.
I will modify it in the next patch.

> > +   for clocks of controller
> > +
> > +Optional properties:
> > + - extcon : external connector for VBUS and IDPIN changes detection,
> > +   needed when supports dual-role mode.
> 
> Don't use extcon for new bindings. The usb-connector binding should be 
> used instead.
This is used to detect the changes of the IDPIN and VBUS, the change
events are provided by other drivers, such as extcon-usb-gpio.c, and
then switch MUSB controller to host or device mode, but the
usb-connector can't detect these changes.

> > + - vbus-supply : reference to the VBUS regulator, needed when supports
> > +   dual-role mode.
> 
> The controller is powered from Vbus? Probably not. This belongs in the 
> connector or maybe the phy (if the phy is powered from Vbus).
The Vbus is used to provide 5V voltage to the connected device when the
controller works as host mode.

> > + - power-domains   : a phandle to USB power domain node to control USB's
> > +   MTCMOS
> > +
> > +Example:
> > +
> > +usb2: usb@11200000 {
> > +	compatible = "mediatek,mt2701-musb";
> > +		"mediatek,mtk-musb";
> > +	reg = <0 0x11200000 0 0x1000>;
> > +	interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_LOW>;
> > +	interrupt-names = "mc";
> > +	phys = <&u2port2 PHY_TYPE_USB2>;
> > +	phy-names = "usb2-phy";
> > +	vbus-supply = <&usb_vbus>;
> > +	extcon = <&extcon_usb>;
> > +	dr_mode = "otg";
> > +	clocks = <&pericfg CLK_PERI_USB0>,
> > +		 <&pericfg CLK_PERI_USB0_MCU>,
> > +		 <&pericfg CLK_PERI_USB_SLV>;
> > +	clock-names = "main","mcu","univpll";
> > +	power-domains = <&scpsys MT2701_POWER_DOMAIN_IFR_MSC>;
> > +};
> > -- 
> > 1.9.1
> >
Rob Herring (Arm) Jan. 4, 2019, 4:10 p.m. UTC | #3 
On Thu, Jan 3, 2019 at 9:00 PM Min Guo <min.guo@mediatek.com> wrote:
>
> On Thu, 2019-01-03 at 16:14 -0600, Rob Herring wrote:
> > On Thu, Dec 27, 2018 at 03:34:23PM +0800, min.guo@mediatek.com wrote:
> > > From: Min Guo <min.guo@mediatek.com>
> > >
> > > This adds support for MediaTek musb controller in
> > > host, peripheral and otg mode

[...]

> > > + - interrupts      : interrupt used by musb controller
> > > + - interrupt-names : must be "mc"
> >
> > -names is pointless when there is only one.
> The MUSB core driver has two interrupts, one is for MAC, another for DMA,
> but on MTK platform, there is only a MAC interrupt, here following the binding
> of MUSB core driver.

You should probably be listing the same interrupt number twice if 2
interrupts are combined.

> > > +Optional properties:
> > > + - extcon : external connector for VBUS and IDPIN changes detection,
> > > +   needed when supports dual-role mode.
> >
> > Don't use extcon for new bindings. The usb-connector binding should be
> > used instead.
> This is used to detect the changes of the IDPIN and VBUS, the change
> events are provided by other drivers, such as extcon-usb-gpio.c, and
> then switch MUSB controller to host or device mode, but the
> usb-connector can't detect these changes.

To repeat, do not use extcon binding for new bindings. It is poorly
designed as it reflects extcon driver needs, not a description of the
hardware. If you have ID on GPIO, then that belongs in a usb-connector
node because that GPIO goes to the connector. For Vbus, you should
have a vbus-supply in the connector and use a gpio-regulator if it is
GPIO controlled.

> > > + - vbus-supply : reference to the VBUS regulator, needed when supports
> > > +   dual-role mode.
> >
> > The controller is powered from Vbus? Probably not. This belongs in the
> > connector or maybe the phy (if the phy is powered from Vbus).
> The Vbus is used to provide 5V voltage to the connected device when the
> controller works as host mode.

I know what Vbus is. Unless Vbus is providing power to the host
controller, putting the Vbus supply in the controller node is not a
accurate representation of the hardware.

Rob
Min Guo Jan. 7, 2019, 7:31 a.m. UTC | #4 
On Fri, 2019-01-04 at 10:10 -0600, Rob Herring wrote:
> On Thu, Jan 3, 2019 at 9:00 PM Min Guo <min.guo@mediatek.com> wrote:
> >
> > On Thu, 2019-01-03 at 16:14 -0600, Rob Herring wrote:
> > > On Thu, Dec 27, 2018 at 03:34:23PM +0800, min.guo@mediatek.com wrote:
> > > > From: Min Guo <min.guo@mediatek.com>
> > > >
> > > > This adds support for MediaTek musb controller in
> > > > host, peripheral and otg mode
> 
> [...]
> 
> > > > + - interrupts      : interrupt used by musb controller
> > > > + - interrupt-names : must be "mc"
> > >
> > > -names is pointless when there is only one.
> > The MUSB core driver has two interrupts, one is for MAC, another for DMA,
> > but on MTK platform, there is only a MAC interrupt, here following the binding
> > of MUSB core driver.
> 
> You should probably be listing the same interrupt number twice if 2
> interrupts are combined.

If only one interrupt is regisetred in driver, dose still need list the
same interrupt number twice in dtsi?

> > > > +Optional properties:
> > > > + - extcon : external connector for VBUS and IDPIN changes detection,
> > > > +   needed when supports dual-role mode.
> > >
> > > Don't use extcon for new bindings. The usb-connector binding should be
> > > used instead.
> > This is used to detect the changes of the IDPIN and VBUS, the change
> > events are provided by other drivers, such as extcon-usb-gpio.c, and
> > then switch MUSB controller to host or device mode, but the
> > usb-connector can't detect these changes.
> 
> To repeat, do not use extcon binding for new bindings. It is poorly
> designed as it reflects extcon driver needs, not a description of the
> hardware. If you have ID on GPIO, then that belongs in a usb-connector
> node because that GPIO goes to the connector. For Vbus, you should
> have a vbus-supply in the connector and use a gpio-regulator if it is
> GPIO controlled.

Sorry, I didn't find a common driver describing the usb-connector. Is
there any driver that I can refer to, specially the way to switch MUSB
controller between host and device mode?

> > > > + - vbus-supply : reference to the VBUS regulator, needed when supports
> > > > +   dual-role mode.
> > >
> > > The controller is powered from Vbus? Probably not. This belongs in the
> > > connector or maybe the phy (if the phy is powered from Vbus).
> > The Vbus is used to provide 5V voltage to the connected device when the
> > controller works as host mode.
> 
> I know what Vbus is. Unless Vbus is providing power to the host
> controller, putting the Vbus supply in the controller node is not a
> accurate representation of the hardware.

I will put vbus-supply in usb-connector after implement it.

> Rob
Bin Liu Jan. 7, 2019, 8:40 p.m. UTC | #5 
Hi,

On Thu, Dec 27, 2018 at 03:34:23PM +0800, min.guo@mediatek.com wrote:
> From: Min Guo <min.guo@mediatek.com>
> 
> This adds support for MediaTek musb controller in
> host, peripheral and otg mode
> 
> Signed-off-by: Min Guo <min.guo@mediatek.com>
> ---
>  .../devicetree/bindings/usb/mediatek,musb.txt      | 49 ++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/mediatek,musb.txt
> 
> diff --git a/Documentation/devicetree/bindings/usb/mediatek,musb.txt b/Documentation/devicetree/bindings/usb/mediatek,musb.txt
> new file mode 100644
> index 0000000..e899c9b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/mediatek,musb.txt
> @@ -0,0 +1,49 @@
> +MediaTek musb DRC/OTG controller

s/DRC/DRD/

> +-------------------------------------------
> +
> +Required properties:

[snip]

> +Example:
> +
> +usb2: usb@11200000 {
> +	compatible = "mediatek,mt2701-musb";

s/;/,/

> +		"mediatek,mtk-musb";

Regards,
-Bin.
Min Guo Jan. 8, 2019, 1:30 a.m. UTC | #6 
On Mon, 2019-01-07 at 14:40 -0600, Bin Liu wrote:
> Hi,
> 
> On Thu, Dec 27, 2018 at 03:34:23PM +0800, min.guo@mediatek.com wrote:
> > From: Min Guo <min.guo@mediatek.com>
> > 
> > This adds support for MediaTek musb controller in
> > host, peripheral and otg mode
> > 
> > Signed-off-by: Min Guo <min.guo@mediatek.com>
> > ---
> >  .../devicetree/bindings/usb/mediatek,musb.txt      | 49 ++++++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/usb/mediatek,musb.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/mediatek,musb.txt b/Documentation/devicetree/bindings/usb/mediatek,musb.txt
> > new file mode 100644
> > index 0000000..e899c9b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/mediatek,musb.txt
> > @@ -0,0 +1,49 @@
> > +MediaTek musb DRC/OTG controller
> 
> s/DRC/DRD/

I will modify it in next patch.

> > +-------------------------------------------
> > +
> > +Required properties:
> 
> [snip]
> 
> > +Example:
> > +
> > +usb2: usb@11200000 {
> > +	compatible = "mediatek,mt2701-musb";
> 
> s/;/,/

I will modify it in next patch.

> > +		"mediatek,mtk-musb";
> 
> Regards,
> -Bin.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/usb/mediatek,musb.txt b/Documentation/devicetree/bindings/usb/mediatek,musb.txt
new file mode 100644
index 0000000..e899c9b
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/mediatek,musb.txt
@@ -0,0 +1,49 @@ 
+MediaTek musb DRC/OTG controller
+-------------------------------------------
+
+Required properties:
+ - compatible      : should be "mediatek,<soc-model>-musb",
+   "mediatek,mtk-musb", soc-model is the name of SoC, such as
+   mt2701, when using "mediatek,mtk-musb" compatible string, you
+   need SoC specific ones in addition, one of:
+   - "mediatek,mt2701-musb"
+ - reg             : specifies physical base address and size of
+   the registers
+ - interrupts      : interrupt used by musb controller
+ - interrupt-names : must be "mc"
+ - phys            : PHY specifier for the OTG phy
+ - phy-names       : should be "usb2-phy"
+ - dr_mode         : should be one of "host", "peripheral" or "otg",
+   refer to usb/generic.txt
+ - clocks          : a list of phandle + clock-specifier pairs, one for
+   each entry in clock-names
+ - clock-names     : must contain "main","mcu","univpll"
+   for clocks of controller
+
+Optional properties:
+ - extcon : external connector for VBUS and IDPIN changes detection,
+   needed when supports dual-role mode.
+ - vbus-supply : reference to the VBUS regulator, needed when supports
+   dual-role mode.
+ - power-domains   : a phandle to USB power domain node to control USB's
+   MTCMOS
+
+Example:
+
+usb2: usb@11200000 {
+	compatible = "mediatek,mt2701-musb";
+		"mediatek,mtk-musb";
+	reg = <0 0x11200000 0 0x1000>;
+	interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_LOW>;
+	interrupt-names = "mc";
+	phys = <&u2port2 PHY_TYPE_USB2>;
+	phy-names = "usb2-phy";
+	vbus-supply = <&usb_vbus>;
+	extcon = <&extcon_usb>;
+	dr_mode = "otg";
+	clocks = <&pericfg CLK_PERI_USB0>,
+		 <&pericfg CLK_PERI_USB0_MCU>,
+		 <&pericfg CLK_PERI_USB_SLV>;
+	clock-names = "main","mcu","univpll";
+	power-domains = <&scpsys MT2701_POWER_DOMAIN_IFR_MSC>;
+};