Re: [PATCH 1/4] dt-bindings: usb: musb: Add support for MediaTek musb controller
On Thu, 2019-01-03 at 16:14 -0600, Rob Herring wrote:
> On Thu, Dec 27, 2018 at 03:34:23PM +0800, min@mediatek.com wrote:
> > From: Min Guo
> >
> > This adds support for MediaTek musb controller in
> > host, peripheral and otg mode
> >
> > Signed-off-by: Min Guo
> > ---
> > .../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 000..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,-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
> 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@1120 {
> > + compatible = "mediatek,mt2701-musb";
> > + "mediatek,mtk-musb";
> > + reg = <0 0x1120 0 0x1000>;
> > + interrupts = ;
> > + 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
> >
Re: [PATCH 1/4] dt-bindings: usb: musb: Add support for MediaTek musb controller
On Fri, 2019-01-04 at 10:10 -0600, Rob Herring wrote: > On Thu, Jan 3, 2019 at 9:00 PM Min Guo wrote: > > > > On Thu, 2019-01-03 at 16:14 -0600, Rob Herring wrote: > > > On Thu, Dec 27, 2018 at 03:34:23PM +0800, min@mediatek.com wrote: > > > > From: Min Guo > > > > > > > > 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
Re: [PATCH 1/4] dt-bindings: usb: musb: Add support for MediaTek musb controller
On Mon, 2019-01-07 at 14:40 -0600, Bin Liu wrote:
> Hi,
>
> On Thu, Dec 27, 2018 at 03:34:23PM +0800, min@mediatek.com wrote:
> > From: Min Guo
> >
> > This adds support for MediaTek musb controller in
> > host, peripheral and otg mode
> >
> > Signed-off-by: Min Guo
> > ---
> > .../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 000..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@1120 {
> > + compatible = "mediatek,mt2701-musb";
>
> s/;/,/
I will modify it in next patch.
> > + "mediatek,mtk-musb";
>
> Regards,
> -Bin.
Re: [PATCH 4/4] usb: musb: Add support for MediaTek musb controller
Hi Bin,
On Tue, 2019-01-08 at 09:44 -0600, Bin Liu wrote:
> Hi,
>
> On Thu, Dec 27, 2018 at 03:34:26PM +0800, min@mediatek.com wrote:
> > From: Min Guo
> >
> > This adds support for MediaTek musb controller in
> > host, peripheral and otg mode
> >
> > Signed-off-by: Min Guo
> > Signed-off-by: Yonglong Wu
> > ---
> > drivers/usb/musb/Kconfig | 8 +-
> > drivers/usb/musb/Makefile| 1 +
> > drivers/usb/musb/mediatek.c | 562
> > +++
> > drivers/usb/musb/musb_core.c | 10 +
> > drivers/usb/musb/musb_core.h | 1 +
> > drivers/usb/musb/musb_dma.h | 1 +
> > drivers/usb/musb/musb_host.c | 79 --
> > drivers/usb/musb/musb_regs.h | 6 +
> > drivers/usb/musb/musbhsdma.c | 34 ++-
> > 9 files changed, 671 insertions(+), 31 deletions(-)
> > create mode 100644 drivers/usb/musb/mediatek.c
> >
> > diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
> > index ad08895..540fc9f 100644
> > --- a/drivers/usb/musb/Kconfig
> > +++ b/drivers/usb/musb/Kconfig
> > @@ -115,6 +115,12 @@ config USB_MUSB_JZ4740
> > depends on USB_MUSB_GADGET
> > depends on USB_OTG_BLACKLIST_HUB
> >
> > +config USB_MUSB_MEDIATEK
> > + tristate "MediaTek platforms"
> > + depends on ARCH_MEDIATEK
>
> Please also add '|| COMPILE_TEST' to make testing easier.
Ok
> > + depends on NOP_USB_XCEIV
> > + depends on GENERIC_PHY
> > +
> > config USB_MUSB_AM335X_CHILD
> > tristate
> >
> > @@ -141,7 +147,7 @@ config USB_UX500_DMA
> >
> > config USB_INVENTRA_DMA
> > bool 'Inventra'
> > - depends on USB_MUSB_OMAP2PLUS
> > + depends on USB_MUSB_OMAP2PLUS || USB_MUSB_MEDIATEK
> > help
> > Enable DMA transfers using Mentor's engine.
> >
> > diff --git a/drivers/usb/musb/Makefile b/drivers/usb/musb/Makefile
> > index 3a88c79..63d82d0 100644
> > --- a/drivers/usb/musb/Makefile
> > +++ b/drivers/usb/musb/Makefile
> > @@ -24,6 +24,7 @@ obj-$(CONFIG_USB_MUSB_DA8XX) +=
> > da8xx.o
> > obj-$(CONFIG_USB_MUSB_UX500) += ux500.o
> > obj-$(CONFIG_USB_MUSB_JZ4740) += jz4740.o
> > obj-$(CONFIG_USB_MUSB_SUNXI) += sunxi.o
> > +obj-$(CONFIG_USB_MUSB_MEDIATEK)+= mediatek.o
> >
> >
> > obj-$(CONFIG_USB_MUSB_AM335X_CHILD)+= musb_am335x.o
> > diff --git a/drivers/usb/musb/mediatek.c b/drivers/usb/musb/mediatek.c
> > new file mode 100644
> > index 000..15a6460
> > --- /dev/null
> > +++ b/drivers/usb/musb/mediatek.c
>
> [snip]
> I will review this section later after we sorted out other things.
>
> > diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> > index b7d5627..d60f76f 100644
> > --- a/drivers/usb/musb/musb_core.c
> > +++ b/drivers/usb/musb/musb_core.c
> > @@ -1028,6 +1028,16 @@ static void musb_disable_interrupts(struct musb
> > *musb)
> > temp = musb_readb(mbase, MUSB_INTRUSB);
> > temp = musb_readw(mbase, MUSB_INTRTX);
> > temp = musb_readw(mbase, MUSB_INTRRX);
> > +
> > + /* MediaTek controller interrupt status is W1C */
>
> This W1C doesn't match to the MUSB Programming Guide that I have. Those
> registers are read-only.
> Is the difference due to the IP intergration in the mtk platforms? or is
> it a new version of the MUSB controller? If latter, what is the version?
This is difference due to the IP intergration in mtk platforms. W1C is
easy for CpdeViser debug.
> > + if (musb->ops->quirks & MUSB_MTK_QUIRKS) {
>
> Basically we don't want to use this type of platform specific quirks if
> possible, so let's try to not use it.
I will try my best to avoid using it.
> > + musb_writeb(mbase, MUSB_INTRUSB,
> > + musb_readb(mbase, MUSB_INTRUSB));
>
> For this clearing register bit operation, please create platform hooks
> musb_clearb() and musb_clearw() in struct musb_platform_ops instead,
> then follow how musb_readb() pointer is assigned in
> musb_init_controller() to use the W1C version for mtk platform.
I have tried implementing musb_readb(), musb_readw() interface with
interrupt status W1C function in struct musb_platform_ops. But this
interface will require a global variable to hold MAC basic address for
judgment, and then special handling of the interrupt state. A global
variable will make the driver work with only a single instanc
Re: [PATCH 4/4] usb: musb: Add support for MediaTek musb controller
Hi Bin, On Wed, 2019-01-09 at 08:01 -0600, Bin Liu wrote: > Hi Min, > > On Wed, Jan 09, 2019 at 08:31:08PM +0800, Min Guo wrote: > > Hi Bin, > > On Tue, 2019-01-08 at 09:44 -0600, Bin Liu wrote: > > > Hi, > > > > > > On Thu, Dec 27, 2018 at 03:34:26PM +0800, min@mediatek.com wrote: > > > > From: Min Guo > > > > > > > > This adds support for MediaTek musb controller in > > > > host, peripheral and otg mode > > > > > > > > Signed-off-by: Min Guo > > > > Signed-off-by: Yonglong Wu > > > > --- > > > > drivers/usb/musb/Kconfig | 8 +- > > > > drivers/usb/musb/Makefile| 1 + > > > > drivers/usb/musb/mediatek.c | 562 > > > > +++ > > > > drivers/usb/musb/musb_core.c | 10 + > > > > drivers/usb/musb/musb_core.h | 1 + > > > > drivers/usb/musb/musb_dma.h | 1 + > > > > drivers/usb/musb/musb_host.c | 79 -- > > > > drivers/usb/musb/musb_regs.h | 6 + > > > > drivers/usb/musb/musbhsdma.c | 34 ++- > > > > 9 files changed, 671 insertions(+), 31 deletions(-) > > > > create mode 100644 drivers/usb/musb/mediatek.c > > > > > > > > diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig > > > > index ad08895..540fc9f 100644 > > > > --- a/drivers/usb/musb/Kconfig > > > > +++ b/drivers/usb/musb/Kconfig > > > > @@ -115,6 +115,12 @@ config USB_MUSB_JZ4740 > > > > depends on USB_MUSB_GADGET > > > > depends on USB_OTG_BLACKLIST_HUB > > > > > > > > +config USB_MUSB_MEDIATEK > > > > + tristate "MediaTek platforms" > > > > + depends on ARCH_MEDIATEK > > > > > > Please also add '|| COMPILE_TEST' to make testing easier. > > > > Ok > > > > > > + depends on NOP_USB_XCEIV > > > > + depends on GENERIC_PHY > > > > + > > > > config USB_MUSB_AM335X_CHILD > > > > tristate > > > > > > > > @@ -141,7 +147,7 @@ config USB_UX500_DMA > > > > > > > > config USB_INVENTRA_DMA > > > > bool 'Inventra' > > > > - depends on USB_MUSB_OMAP2PLUS > > > > + depends on USB_MUSB_OMAP2PLUS || USB_MUSB_MEDIATEK > > > > help > > > > Enable DMA transfers using Mentor's engine. > > > > > > > > diff --git a/drivers/usb/musb/Makefile b/drivers/usb/musb/Makefile > > > > index 3a88c79..63d82d0 100644 > > > > --- a/drivers/usb/musb/Makefile > > > > +++ b/drivers/usb/musb/Makefile > > > > @@ -24,6 +24,7 @@ obj-$(CONFIG_USB_MUSB_DA8XX) += > > > > da8xx.o > > > > obj-$(CONFIG_USB_MUSB_UX500) += ux500.o > > > > obj-$(CONFIG_USB_MUSB_JZ4740) += jz4740.o > > > > obj-$(CONFIG_USB_MUSB_SUNXI) += sunxi.o > > > > +obj-$(CONFIG_USB_MUSB_MEDIATEK)+= mediatek.o > > > > > > > > > > > > obj-$(CONFIG_USB_MUSB_AM335X_CHILD)+= musb_am335x.o > > > > diff --git a/drivers/usb/musb/mediatek.c b/drivers/usb/musb/mediatek.c > > > > new file mode 100644 > > > > index 000..15a6460 > > > > --- /dev/null > > > > +++ b/drivers/usb/musb/mediatek.c > > > > > > [snip] > > > I will review this section later after we sorted out other things. > > > > > > > diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c > > > > index b7d5627..d60f76f 100644 > > > > --- a/drivers/usb/musb/musb_core.c > > > > +++ b/drivers/usb/musb/musb_core.c > > > > @@ -1028,6 +1028,16 @@ static void musb_disable_interrupts(struct musb > > > > *musb) > > > > temp = musb_readb(mbase, MUSB_INTRUSB); > > > > temp = musb_readw(mbase, MUSB_INTRTX); > > > > temp = musb_readw(mbase, MUSB_INTRRX); > > > > + > > > > + /* MediaTek controller interrupt status is W1C */ > > > > > > This W1C doesn't match to the MUSB Programming Guide that I have. Those > > > registers are read-only. > > > Is the difference due to the IP intergration in the mtk platforms? or is > > > it a new version of the MUSB cont
Re: [PATCH 4/4] usb: musb: Add support for MediaTek musb controller
Hi Bin,
On Thu, 2019-01-10 at 08:18 -0600, Bin Liu wrote:
> Hi Min,
>
> Please briefly summarize the controller differences in the commit log,
> such as
>
> - WIC interrupt registers;
> - data toggle bit;
> - no dedicated DMA interrupt line;
>
> so that we can quickly understand the core driver is modified
> accordingly to handle the differences.
Okay, tkanks.
> On Thu, Jan 10, 2019 at 03:24:22PM +0800, Min Guo wrote:
> > Hi Bin,
>
> [snip]
>
> > > > > > + musb_writeb(mbase, MUSB_INTRUSB,
> > > > > > + musb_readb(mbase, MUSB_INTRUSB));
> > > > >
> > > > > For this clearing register bit operation, please create platform hooks
> > > > > musb_clearb() and musb_clearw() in struct musb_platform_ops instead,
> > > > > then follow how musb_readb() pointer is assigned in
> > > > > musb_init_controller() to use the W1C version for mtk platform.
> > > >
> > > > I have tried implementing musb_readb(), musb_readw() interface with
> > > > interrupt status W1C function in struct musb_platform_ops. But this
> > > > interface will require a global variable to hold MAC basic address for
> > > > judgment, and then special handling of the interrupt state. A global
> > > > variable will make the driver work with only a single instance, so it
> > > > can't work on some MTK platforms which have two instances.
> > >
> > > I didn't mean to modify musb_read*(), but
> > >
> > > > How about creating musb_clearb/w() as following:
> > > > void (*clearb)(void __iomem *addr, unsigned offset, u8 data);
> > > > void (*clearw)(void __iomem *addr, unsigned offset, u16 data);
> > >
> > > this is what I was asking for, similar to what musb_readb/w() is
> > > implemented.
> >
> > I will prepare a patch for musb_clearb/w().
>
> This doesn't have to be a separate patch.
Okay.
> > > > > > + musb_writew(mbase, MUSB_INTRRX,
> > > > > > + musb_readw(mbase, MUSB_INTRRX));
> > > > > > + musb_writew(mbase, MUSB_INTRTX,
> > > > > > + musb_readw(mbase, MUSB_INTRTX));
> > > > > > + }
>
> [snip]
>
> > > > > > + /* MediaTek controller has private toggle register */
> > > > >
> > > > > only one toggle register for all endpoints? how does it handle
> > > > > difference toggle values for different endpoints?
> > > >
> > > > MediaTek controller has separate registers to describe TX/RX toggle.
> > >
> > > Is it one register per endpoint?
> >
> > MUSB_RXTOG/MUSB_TXTOG is common register, each bit reflects the toggle
> > state of an endpoint. bit[0] not used,bit[1~8] corresponds to ep[1~8]
> >
> > > >
> > > > > > + if (musb->ops->quirks & MUSB_MTK_QUIRKS) {
> > > > > > + u16 toggle;
> > > > > > + u8 epnum = qh->hw_ep->epnum;
> > > > > > +
> > > > > > + if (is_in)
> > > > > > + toggle = musb_readl(musb->mregs, MUSB_RXTOG);
> > >
> > > this line seems telling there is just *one* register for all endpoints.
> >
> > Yes, all endpoint share this register, endpoint and bit are one-to-one
> > correspondence.
>
> Okay, thanks. Sorry I missed the bit operation in the code below.
>
> >
> > > > >
> > > > > should use musb_readw() instead? MUSB_RXTOG seems to be 16bit.
> > > >
> > > > Ok
> > > >
> > > > > > + else
> > > > > > + toggle = musb_readl(musb->mregs, MUSB_TXTOG);
> > > > > > +
> > > > > > + csr = toggle & (1 << epnum);
>
> Regards,
> -Bin.
Re: [PATCH 4/4] usb: musb: Add support for MediaTek musb controller
Hi Bin,
I have some questions about
musbhs_dma_controller_destroy/create_noirq().
1,Because of controller->irq=0 in noirq case, destroy_noirq can reuse
musbhs_dma_controller_destroy. Is it necessary to write a destroy_noirq
function?
2, How about creating a common function for create musb dma controller
as following:
static struct musb_dma_controller *dma_controller_alloc(struct musb
*musb, void __iomem *base)
then musbhs_dma_controller_create() and
musbhs_dma_controller_create_noirq() call it to alloc common resource.
On Tue, 2019-01-08 at 09:44 -0600, Bin Liu wrote:
> Hi,
>
> On Thu, Dec 27, 2018 at 03:34:26PM +0800, min@mediatek.com wrote:
> > From: Min Guo
> >
> > This adds support for MediaTek musb controller in
> > host, peripheral and otg mode
> >
> > diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c
> > index 824adcb..c545475 100644
> > --- a/drivers/usb/musb/musbhsdma.c
> > +++ b/drivers/usb/musb/musbhsdma.c
> >
> > void musbhs_dma_controller_destroy(struct dma_controller *c)
> > {
> > struct musb_dma_controller *controller = container_of(c,
> > struct musb_dma_controller, controller);
> > + struct musb *musb = controller->private_data;
> >
> > dma_controller_stop(controller);
> >
> > - if (controller->irq)
> > + if (!(musb->ops->quirks & MUSB_MTK_QUIRKS) && controller->irq)
> > free_irq(controller->irq, c);
> >
> > kfree(controller);
> > @@ -398,11 +402,15 @@ struct dma_controller
> > *musbhs_dma_controller_create(struct musb *musb,
> > struct musb_dma_controller *controller;
> > struct device *dev = musb->controller;
> > struct platform_device *pdev = to_platform_device(dev);
> > - int irq = platform_get_irq_byname(pdev, "dma");
> > + int irq = -1;
> >
> > - if (irq <= 0) {
> > - dev_err(dev, "No DMA interrupt line!\n");
> > - return NULL;
> > + if (!(musb->ops->quirks & MUSB_MTK_QUIRKS)) {
> > + irq = platform_get_irq_byname(pdev, "dma");
> > +
> > + if (irq < 0) {
> > + dev_err(dev, "No DMA interrupt line!\n");
> > + return NULL;
> > + }
> > }
>
> Please create musbhs_dma_controller_destroy_noirq() for your platform to
> not use the quirk.
>
> >
> > controller = kzalloc(sizeof(*controller), GFP_KERNEL);
> > @@ -418,15 +426,17 @@ struct dma_controller
> > *musbhs_dma_controller_create(struct musb *musb,
> > controller->controller.channel_program = dma_channel_program;
> > controller->controller.channel_abort = dma_channel_abort;
> >
> > - if (request_irq(irq, dma_controller_irq, 0,
> > + if (!(musb->ops->quirks & MUSB_MTK_QUIRKS)) {
> > + if (request_irq(irq, dma_controller_irq, 0,
> > dev_name(musb->controller), &controller->controller)) {
> > - dev_err(dev, "request_irq %d failed!\n", irq);
> > - musb_dma_controller_destroy(&controller->controller);
> > + dev_err(dev, "request_irq %d failed!\n", irq);
> > + musb_dma_controller_destroy(&controller->controller);
> >
> > - return NULL;
> > - }
> > + return NULL;
> > + }
> >
> > - controller->irq = irq;
> > + controller->irq = irq;
> > + }
> >
> > return &controller->controller;
> > }
>
> Same here, create musbhs_dma_controller_create_noirq(). Then use both
> new API for the mtk glue driver.
> Regards,
> -Bin.
Re: [PATCH v2 3/4] usb: musb: Extract set toggle as a separate interface
Hi Matthias,
On Tue, 2019-01-15 at 16:19 +0100, Matthias Brugger wrote:
>
> On 15/01/2019 02:43, min@mediatek.com wrote:
> > From: Min Guo
> >
> > Add a common interface for set data toggle
> >
> > Signed-off-by: Min Guo
> > ---
> > drivers/usb/musb/musb_host.c | 37 +++--
> > 1 file changed, 23 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> > index b59ce9a..16d0ba4 100644
> > --- a/drivers/usb/musb/musb_host.c
> > +++ b/drivers/usb/musb/musb_host.c
> > @@ -306,6 +306,25 @@ static inline void musb_save_toggle(struct musb_qh
> > *qh, int is_in,
> > usb_settoggle(urb->dev, qh->epnum, !is_in, csr ? 1 : 0);
> > }
> >
> > +static inline u16 musb_set_toggle(struct musb_qh *qh, int is_in,
> > + struct urb *urb)
> > +{
> > + u16 csr = 0;
> > + u16 toggle = 0;
> > +
> > + toggle = usb_gettoggle(urb->dev, qh->epnum, !is_in);
> > +
> > + if (is_in)
> > + csr = toggle ? (MUSB_RXCSR_H_WR_DATATOGGLE
> > + | MUSB_RXCSR_H_DATATOGGLE) : 0;
> > + else
> > + csr = toggle ? (MUSB_TXCSR_H_WR_DATATOGGLE
> > + | MUSB_TXCSR_H_DATATOGGLE)
> > + : MUSB_TXCSR_CLRDATATOG;
>
> Can we switch the if and use is_out logic as function parameter. This would
> make
> the code easier to understand.
Okay.
> Regards,
> Matthias
>
> > +
> > + return csr;
> > +}
> > +
> > /*
> > * Advance this hardware endpoint's queue, completing the specified URB and
> > * advancing to either the next URB queued to that qh, or else invalidating
> > @@ -772,13 +791,8 @@ static void musb_ep_program(struct musb *musb, u8
> > epnum,
> > );
> > csr |= MUSB_TXCSR_MODE;
> >
> > - if (!hw_ep->tx_double_buffered) {
> > - if (usb_gettoggle(urb->dev, qh->epnum, 1))
> > - csr |= MUSB_TXCSR_H_WR_DATATOGGLE
> > - | MUSB_TXCSR_H_DATATOGGLE;
> > - else
> > - csr |= MUSB_TXCSR_CLRDATATOG;
> > - }
> > + if (!hw_ep->tx_double_buffered)
> > + csr |= musb_set_toggle(qh, !is_out, urb);
> >
> > musb_writew(epio, MUSB_TXCSR, csr);
> > /* REVISIT may need to clear FLUSHFIFO ... */
> > @@ -860,17 +874,12 @@ static void musb_ep_program(struct musb *musb, u8
> > epnum,
> >
> > /* IN/receive */
> > } else {
> > - u16 csr;
> > + u16 csr = 0;
> >
> > if (hw_ep->rx_reinit) {
> > musb_rx_reinit(musb, qh, epnum);
> > + csr |= musb_set_toggle(qh, !is_out, urb);
> >
> > - /* init new state: toggle and NYET, maybe DMA later */
> > - if (usb_gettoggle(urb->dev, qh->epnum, 0))
> > - csr = MUSB_RXCSR_H_WR_DATATOGGLE
> > - | MUSB_RXCSR_H_DATATOGGLE;
> > - else
> > - csr = 0;
> > if (qh->type == USB_ENDPOINT_XFER_INT)
> > csr |= MUSB_RXCSR_DISNYET;
> >
> >
Re: [PATCH v2 4/4] usb: musb: Add support for MediaTek musb controller
Hi Bin,
On Tue, 2019-01-15 at 14:38 -0600, Bin Liu wrote:
> Hi Min,
>
> very close, thanks.
> Below I tried to explain a further cleanup in musb_clearb/w() and
> musb_get/set_toggle() implementation. Please let me know if it is not
> clear.
>
> Basically, we don't need musb_default_clearb/w(), just assign the
> musb_io function pointers to musb_readb/w().
>
> Then the mtk platform musb_clearb/w() calls musb_readb/w() and
> musb_writeb/w() to handle W1C.
Okay.
> On Tue, Jan 15, 2019 at 09:43:46AM +0800, min@mediatek.com wrote:
> > From: Min Guo
> >
> > This adds support for MediaTek musb controller in
> > host, peripheral and otg mode.
> > There are some quirk of MediaTek musb controller, such as:
> > -W1C interrupt status registers
> > -Private data toggle registers
> > -No dedicated DMA interrupt line
> >
> > Signed-off-by: Min Guo
> > Signed-off-by: Yonglong Wu
> > ---
> > drivers/usb/musb/Kconfig | 8 +-
> > drivers/usb/musb/Makefile| 1 +
> > drivers/usb/musb/mediatek.c | 617
> > +++
> > drivers/usb/musb/musb_core.c | 69 +
> > drivers/usb/musb/musb_core.h | 9 +
> > drivers/usb/musb/musb_dma.h | 9 +
> > drivers/usb/musb/musb_host.c | 26 +-
> > drivers/usb/musb/musb_io.h | 6 +
> > drivers/usb/musb/musbhsdma.c | 55 ++--
> > 9 files changed, 762 insertions(+), 38 deletions(-)
> > create mode 100644 drivers/usb/musb/mediatek.c
> >
> > diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
> > index ad08895..b72b7c1 100644
> > --- a/drivers/usb/musb/Kconfig
> > +++ b/drivers/usb/musb/Kconfig
> > @@ -115,6 +115,12 @@ config USB_MUSB_JZ4740
> > depends on USB_MUSB_GADGET
> > depends on USB_OTG_BLACKLIST_HUB
> >
> > +config USB_MUSB_MEDIATEK
> > + tristate "MediaTek platforms"
> > + depends on ARCH_MEDIATEK || COMPILE_TEST
> > + depends on NOP_USB_XCEIV
> > + depends on GENERIC_PHY
> > +
> > config USB_MUSB_AM335X_CHILD
> > tristate
> >
> > @@ -141,7 +147,7 @@ config USB_UX500_DMA
> >
> > config USB_INVENTRA_DMA
> > bool 'Inventra'
> > - depends on USB_MUSB_OMAP2PLUS
> > + depends on USB_MUSB_OMAP2PLUS || USB_MUSB_MEDIATEK
> > help
> > Enable DMA transfers using Mentor's engine.
> >
> > diff --git a/drivers/usb/musb/Makefile b/drivers/usb/musb/Makefile
> > index 3a88c79..63d82d0 100644
> > --- a/drivers/usb/musb/Makefile
> > +++ b/drivers/usb/musb/Makefile
> > @@ -24,6 +24,7 @@ obj-$(CONFIG_USB_MUSB_DA8XX) +=
> > da8xx.o
> > obj-$(CONFIG_USB_MUSB_UX500) += ux500.o
> > obj-$(CONFIG_USB_MUSB_JZ4740) += jz4740.o
> > obj-$(CONFIG_USB_MUSB_SUNXI) += sunxi.o
> > +obj-$(CONFIG_USB_MUSB_MEDIATEK) += mediatek.o
> >
> >
> > obj-$(CONFIG_USB_MUSB_AM335X_CHILD)+= musb_am335x.o
> > diff --git a/drivers/usb/musb/mediatek.c b/drivers/usb/musb/mediatek.c
> > new file mode 100644
> > index 000..7221989
> > --- /dev/null
> > +++ b/drivers/usb/musb/mediatek.c
> > @@ -0,0 +1,617 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2018 MediaTek Inc.
> > + *
> > + * Author:
> > + * Min Guo
> > + * Yonglong Wu
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include "musb_core.h"
> > +#include "musb_dma.h"
> > +
> > +#define USB_L1INTS 0x00a0
> > +#define USB_L1INTM 0x00a4
> > +#define MTK_MUSB_TXFUNCADDR0x0480
> > +
> > +/* MediaTek controller toggle enable and status reg */
> > +#define MUSB_RXTOG 0x80
> > +#define MUSB_RXTOGEN 0x82
> > +#define MUSB_TXTOG 0x84
> > +#define MUSB_TXTOGEN 0x86
> > +
> > +#define TX_INT_STATUS BIT(0)
> > +#define RX_INT_STATUS BIT(1)
> > +#define USBCOM_INT_STATUS BIT(2)
>
> missing a TAB for the alignment?
Okay.
> > +#define DMA_INT_STATUS BIT(3)
> > +
> > +#define DMA_INTR_STATUS_MSKGENMASK(7, 0)
> > +#define DMA_INTR_UNMASK_SET_MSKGENMASK(31, 24)
> > +
> > +enum mtk_vbus_id_state {
> > + MTK_ID_FLOAT = 1,
> > + MTK_ID_GROUND,
> > + MTK_VBUS_OFF,
> > + MTK_VB
Re: [PATCH v2 3/4] usb: musb: Extract set toggle as a separate interface
Hi Bin,
On Tue, 2019-01-15 at 14:40 -0600, Bin Liu wrote:
> Hi Min,
>
> On Tue, Jan 15, 2019 at 04:19:42PM +0100, Matthias Brugger wrote:
> >
> >
> > On 15/01/2019 02:43, min@mediatek.com wrote:
> > > From: Min Guo
> > >
> > > Add a common interface for set data toggle
> > >
> > > Signed-off-by: Min Guo
> > > ---
> > > drivers/usb/musb/musb_host.c | 37 +++--
> > > 1 file changed, 23 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> > > index b59ce9a..16d0ba4 100644
> > > --- a/drivers/usb/musb/musb_host.c
> > > +++ b/drivers/usb/musb/musb_host.c
> > > @@ -306,6 +306,25 @@ static inline void musb_save_toggle(struct musb_qh
> > > *qh, int is_in,
> > > usb_settoggle(urb->dev, qh->epnum, !is_in, csr ? 1 : 0);
> > > }
> > >
> > > +static inline u16 musb_set_toggle(struct musb_qh *qh, int is_in,
> > > + struct urb *urb)
> > > +{
> > > + u16 csr = 0;
> > > + u16 toggle = 0;
> > > +
> > > + toggle = usb_gettoggle(urb->dev, qh->epnum, !is_in);
> > > +
> > > + if (is_in)
> > > + csr = toggle ? (MUSB_RXCSR_H_WR_DATATOGGLE
> > > + | MUSB_RXCSR_H_DATATOGGLE) : 0;
> > > + else
> > > + csr = toggle ? (MUSB_TXCSR_H_WR_DATATOGGLE
> > > + | MUSB_TXCSR_H_DATATOGGLE)
> > > + : MUSB_TXCSR_CLRDATATOG;
> >
> > Can we switch the if and use is_out logic as function parameter. This would
> > make
> > the code easier to understand.
>
> based on the current implementation in patch 4/4, I don't think we need
> this separate patch any more, but Matthias' comments still apply.
Okay.
> Regards,
> -Bin.
Re: [PATCH v2 4/4] usb: musb: Add support for MediaTek musb controller
Hi Bin, On Tue, 2019-01-15 at 14:38 -0600, Bin Liu wrote: > Hi Min, > > very close, thanks. > Below I tried to explain a further cleanup in musb_clearb/w() and > musb_get/set_toggle() implementation. Please let me know if it is not > clear. > > Basically, we don't need musb_default_clearb/w(), just assign the > musb_io function pointers to musb_readb/w(). > > Then the mtk platform musb_clearb/w() calls musb_readb/w() and > musb_writeb/w() to handle W1C. Sorry to bother you again, I encounter a problem when prepare the patch. The define of musb_clearb/w and musb_readb/w are difference as follow, and cannot be directly assigned: u8/u16 (*readb/w)(const void __iomem *addr, unsigned offset) void (*clearb/w)(void __iomem *addr, unsigned int offset)) if modify clearb/w as: u8/u16 (*clearb/w)(const void __iomem *addr, unsigned int offset)) then musb_clear needs writeb/w the const addr. Can I delete const in (*readb/w)? > regards, > -Bin.
Re: [PATCH v2 4/4] usb: musb: Add support for MediaTek musb controller
On Wed, 2019-01-16 at 07:59 -0600, Bin Liu wrote: > On Wed, Jan 16, 2019 at 05:39:02PM +0800, Min Guo wrote: > > Hi Bin, > > > > On Tue, 2019-01-15 at 14:38 -0600, Bin Liu wrote: > > > Hi Min, > > > > > > very close, thanks. > > > Below I tried to explain a further cleanup in musb_clearb/w() and > > > musb_get/set_toggle() implementation. Please let me know if it is not > > > clear. > > > > > > Basically, we don't need musb_default_clearb/w(), just assign the > > > musb_io function pointers to musb_readb/w(). > > > > > > Then the mtk platform musb_clearb/w() calls musb_readb/w() and > > > musb_writeb/w() to handle W1C. > > > > Sorry to bother you again, I encounter a problem when prepare the patch. > > no problem at all. Thanks. > > The define of musb_clearb/w and musb_readb/w are difference as follow, > > and cannot be directly assigned: > > u8/u16 (*readb/w)(const void __iomem *addr, unsigned offset) > > void (*clearb/w)(void __iomem *addr, unsigned int offset)) > > > > if modify clearb/w as: > > u8/u16 (*clearb/w)(const void __iomem *addr, unsigned int offset)) > > then musb_clear needs writeb/w the const addr. > > Can I delete const in (*readb/w)? > > yes, please create a separate patch for it, and for readl() as well, > and stating it is for implementing clearing W1C registers. Okay. > Regards, > -Bin.
Re: [PATCH v3 1/4] dt-bindings: usb: musb: Add support for MediaTek musb controller
Hello! On Thu, 2019-01-17 at 14:06 +0300, Sergei Shtylyov wrote: > Hello! > > On 01/17/2019 10:15 AM, min@mediatek.com wrote: > > > From: Min Guo > > > > This adds support for MediaTek musb controller in > > host, peripheral and otg mode. > > > > Signed-off-by: Min Guo > > --- > > .../devicetree/bindings/usb/mediatek,musb.txt | 43 > > ++ > > 1 file changed, 43 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 000..3e3f671 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/usb/mediatek,musb.txt > > @@ -0,0 +1,43 @@ > > +MediaTek musb DRD/OTG controller > > +--- > > + > > +Required properties: > > + - compatible : should be "mediatek,mtk-musb" > > + - reg : specifies physical base address and size of > > + the registers > >I'd indent it more to align with "specifies", else it looks weird. Okay. > > + - interrupts : interrupt used by musb controller > > + - interrupt-names : must be "mc" > > + - phys: PHY specifier for the OTG phy > > + - dr_mode : should be one of "host", "peripheral" or "otg", > > + refer to usb/generic.txt > > Same here... Okay. > > + - clocks : a list of phandle + clock-specifier pairs, one for > > + each entry in clock-names > >Here... Okay. > > + - clock-names : must contain "main", "mcu", "univpll" > > + for clocks of controller > > And here. Okay. > > + > > +Optional properties: > > + - extcon : external connector for VBUS and IDPIN changes detection, > >ID pin, maybe? Okay. > > + 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 > [...] > > MBR, Sergei
Re: [PATCH v3 0/4] Add MediaTek MUSB Controller Driver
Hi Bin, On Thu, 2019-01-17 at 09:00 -0600, Bin Liu wrote: > On Thu, Jan 17, 2019 at 03:15:44PM +0800, min@mediatek.com wrote: > > From: Min Guo > > > > These patches introduce the MediaTek MUSB controller driver. > > > > The driver can be configured as Dual-Role Device (DRD), > > Peripheral Only and Host Only modes. This has beed tested on > > MT2701 with a variety of devices in host mode and with the > > f_mass gadget driver in peripheral mode, plugging otg cables > > in/out a lot of times in all possible imaginable plug orders. > > > > changes in v3: > > Please put the change log in each individual patch, which helps in > viewing patches. Okay. > Regards, > -Bin.
Re: [PATCH v3 4/4] usb: musb: Add support for MediaTek musb controller
On Thu, 2019-01-17 at 06:33 -0800, Tony Lindgren wrote: > Hi, > > * min@mediatek.com [190117 07:16]: > > There are some quirk of MediaTek musb controller, such as: > > -W1C interrupt status registers > > -Private data toggle registers > > -No dedicated DMA interrupt line > > Can you please separate the musb generic changes listed above > into separate individual patches in preparation for adding > support for new hardware? > > Otherwise we'll have hard time finding out with git bisect what > exactly breaks things if we run into trouble. Thanks for your suggestion. I prepared to divide these changes into separate patches. Later, Mr.Bin suggested not to do this. The detailed information can be referred to: https://patchwork.kernel.org/patch/10743561/ https://patchwork.kernel.org/patch/10763737/ https://patchwork.kernel.org/patch/107463741/ > Regards, > > Tony
Re: [PATCH v4 5/6] usb: musb: Add musb_clearb/w() interface
Hi Bin, On Mon, 2019-01-21 at 09:59 -0600, Bin Liu wrote: > Hi Min, > > On Mon, Jan 21, 2019 at 08:22:30PM +0800, min@mediatek.com wrote: > > From: Min Guo > > > > Delete the const attribute of addr parameter in readb/w/l hooks, these > > changes are for implementing clearing W1C registers. > > Replace musb_readb/w with musb_clearb/w to clear the interrupt status. > > > > Signed-off-by: Min Guo > > --- > > new patch based on v3: > > --- > > drivers/usb/musb/musb_core.c | 32 +++- > > drivers/usb/musb/musb_core.h | 8 ++-- > > drivers/usb/musb/musb_io.h | 8 +--- > > drivers/usb/musb/musbhsdma.c | 3 +++ > > drivers/usb/musb/sunxi.c | 4 ++-- > > drivers/usb/musb/tusb6010.c | 2 +- > > 6 files changed, 40 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c > > index 2fe5225..5ef8848 100644 > > --- a/drivers/usb/musb/musb_core.c > > +++ b/drivers/usb/musb/musb_core.c > > [snip] > > > diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c > > index b2394a7..7bf91cb 100644 > > --- a/drivers/usb/musb/musbhsdma.c > > +++ b/drivers/usb/musb/musbhsdma.c > > @@ -286,6 +286,9 @@ irqreturn_t dma_controller_irq(int irq, void > > *private_data) > > > > int_hsdma = musb_readb(mbase, MUSB_HSDMA_INTR); > > > > + /* clear pending interrupts by manual */ > > + musb_clearb(musb->mregs, MUSB_HSDMA_INTR); > > + > > Make musb_clearb/w() return the value of the _read() instead of > _write(), so that we don't have to read twice here. The value of > _write() is not used anyway. > > The change above can be: > > int_hsdma = musb_clearb(mbase, MUSB_HSDMA_INTR); > > Please see my comments in 6/6 for details. Okay. > Regards, > -Bin.
Re: [PATCH v4 6/6] usb: musb: Add support for MediaTek musb controller
Hi Bin,
On Mon, 2019-01-21 at 10:07 -0600, Bin Liu wrote:
> Hi Min,
>
> On Mon, Jan 21, 2019 at 08:22:31PM +0800, min@mediatek.com wrote:
> > From: Min Guo
> >
> > This adds support for MediaTek musb controller in
> > host, peripheral and otg mode.
> > There are some quirk of MediaTek musb controller, such as:
> > -W1C interrupt status registers
> > -Private data toggle registers
> > -No dedicated DMA interrupt line
> >
> > Signed-off-by: Min Guo
> > Signed-off-by: Yonglong Wu
> > ---
> > changes in v4:
> > 1. no changes
> >
> > changes in v3:
> > suggested by Bin:
> > 1. Remove 'u8/u16 data' parameter in clearb/w() hooks
> > 2. Replace musb_readb/w() with musb_clearb/w() to clear interrupts status
> >
> > changes in v2:
> > suggested by Bin:
> > 1. Add summarize of MediaTek musb controller differences in the commit log
> > 2. Add "|| COMPILE_TEST" in Kconfig
> > 3. Move MediaTek's private toggle registers from musb_regs.h to mediatek.c
> > 4. Replace musb_readl() with musb_readw() to read 16bit toggle register
> > ---
> > drivers/usb/musb/Kconfig| 8 +-
> > drivers/usb/musb/Makefile | 1 +
> > drivers/usb/musb/mediatek.c | 624
> >
> > 3 files changed, 632 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/usb/musb/mediatek.c
> >
>
> [snip]
>
> > +static irqreturn_t generic_interrupt(int irq, void *__hci)
> > +{
> > + unsigned long flags;
> > + irqreturn_t retval = IRQ_NONE;
> > + struct musb *musb = __hci;
> > +
> > + spin_lock_irqsave(&musb->lock, flags);
> > + musb->int_usb = musb_readb(musb->mregs, MUSB_INTRUSB) &
> > + musb_readb(musb->mregs, MUSB_INTRUSBE);
> > + musb->int_tx = musb_readw(musb->mregs, MUSB_INTRTX)
> > + & musb_readw(musb->mregs, MUSB_INTRTXE);
> > + musb->int_rx = musb_readw(musb->mregs, MUSB_INTRRX)
> > + & musb_readw(musb->mregs, MUSB_INTRRXE);
>
> Based on my comment in 5/6, the above 3 musb_readb/w() can be changed to
>
> > + /* MediaTek controller interrupt status is W1C */
> > + musb_clearw(musb->mregs, MUSB_INTRRX);
> > + musb_clearw(musb->mregs, MUSB_INTRTX);
> > + musb_clearb(musb->mregs, MUSB_INTRUSB);
>
> musb->int_usb = musb_clearb(musb->mregs, MUSB_INTRUSB)
> musb->int_tx = musb_clearw(musb->mregs, MUSB_INTRTX);
> musb->int_rx = musb_clearw(musb->mregs, MUSB_INTRRX);
Okay.
> > +
> > + if (musb->int_usb || musb->int_tx || musb->int_rx)
> > + retval = musb_interrupt(musb);
> > +
> > + spin_unlock_irqrestore(&musb->lock, flags);
> > +
> > + return retval;
> > +}
> > +
>
> [snip]
>
> > +
> > +static u8 mtk_musb_clearb(void __iomem *addr, unsigned int offset)
> > +{
> > + u8 data;
> > +
> > + /* W1C */
> > + data = musb_readb(addr, offset);
> > + musb_writeb(addr, offset, data);
> > + return musb_readb(addr, offset);
>
> return data;
Okay.
> > +}
> > +
> > +static u16 mtk_musb_clearw(void __iomem *addr, unsigned int offset)
> > +{
> > + u16 data;
> > +
> > + /* W1C */
> > + data = musb_readw(addr, offset);
> > + musb_writew(addr, offset, data);
> > + return musb_readw(addr, offset);
>
> return data;
Okay.
>
> Regards,
> -Bin.
Re: [PATCH v4 1/6] dt-bindings: usb: musb: Add support for MediaTek musb controller
Hi Bin,
Sorry to bother you again, I encounter a problem about the extcon
property.
I don'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?
If it needs to implement by myself, is it possible to emulate an
usb-connector driver by extcon-usb-gpio, and also use the notifier
mechanism or can you give me some advices?
Hi Rob,
On Mon, 2019-01-21 at 09:14 -0600, Rob Herring wrote:
> On Mon, Jan 21, 2019 at 08:22:26PM +0800, min@mediatek.com wrote:
> > From: Min Guo
> >
> > This adds support for MediaTek musb controller in
> > host, peripheral and otg mode.
> >
> > Signed-off-by: Min Guo
> > ---
> > changes in v4:
> > suggested by Sergei:
> > 1. String alignment
> >
> > changes in v3:
> > 1. no changes
> >
> > changes in v2:
> > suggested by Bin:
> > 1. Modify DRC to DRD
> > suggested by Rob:
> > 2. Drop the "-musb" in compatible
>
> This is not what I said. I gave you the exact text to put.
Sorry to misunderstood your meaning last time, I will modify it in next
patch.
compatible: should be one of "mediatek,mt2701-musb"...,followed by
"mediatek,mtk-musb"
> > 3. Remove phy-names
> > 4. Add space after comma in clock-names
> > ---
> > .../devicetree/bindings/usb/mediatek,musb.txt | 43
> > ++
> > 1 file changed, 43 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 000..4305770
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/mediatek,musb.txt
> > @@ -0,0 +1,43 @@
> > +MediaTek musb DRD/OTG controller
> > +---
> > +
> > +Required properties:
> > + - compatible : should be "mediatek,mtk-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
> > + - 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 ID pin changes
> > detection,
> > + needed when supports dual-role mode
>
> Again, do not use extcon in new bindings.
Thanks, ok.
> > + - 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@1120 {
> > + compatible = "mediatek,mt2701-musb",
> > +"mediatek,mtk-musb";
> > + reg = <0 0x1120 0 0x1000>;
> > + interrupts = ;
> > + interrupt-names = "mc";
> > + phys = <&u2port2 PHY_TYPE_USB2>;
> > + 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
> >
> >
> > ___
> > linux-arm-kernel mailing list
> > linux-arm-ker...@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Regards,
Min.
Re: [PATCH v4 1/6] dt-bindings: usb: musb: Add support for MediaTek musb controller
Hi Bin,
Thanks for your help.
Hi Rob,
I find that Samsung describes the usb-connector attribute in DTS, and
uses a private driver.
And try to write DTS as following:
usb-connector node:
musb_con: musb_connector{
compatible = "linux,extcon-usb-gpio","usb-b-connector";
lable = "micro-USB";
type = "micro";
id-gpio = <&pio 44 GPIO_ACTIVE_HIGH>
vbus-supply = <&usb_vbus>;
port {
usb_to_connector: endpoint {
remote-endpoint = <&connector_to_usb>;
};
};
};
usb node:
&usb2{
status = "okay";
port {
connector_to_usb: endpoint {
remote-endpoint = <&usb_to_connector>;
};
};
}
Can I describe usb-connector like this? Or can you give me some advices?
Regards,
Min.
On Tue, 2019-01-22 at 08:33 -0600, Bin Liu wrote:
> Hi Min,
>
> On Tue, Jan 22, 2019 at 05:36:13PM +0800, Min Guo wrote:
> > Hi Bin,
> >
> > Sorry to bother you again, I encounter a problem about the extcon
> > property.
> >
> > I don'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?
> > If it needs to implement by myself, is it possible to emulate an
> > usb-connector driver by extcon-usb-gpio, and also use the notifier
> > mechanism or can you give me some advices?
>
> I am afraid I am unable to help you on this. I wasn't really pay
> attention when usb-connector was introduced and not sure how it can
> replace extcon. Now after read usb-connector.txt, it seems the binding
> only defines a/b/c-connector, but not ab-connector, and there is no
> enough information (at least for me) explaining how VBUS and ID fix into
> this usb-connector binding.
>
> Maybe Rob can provide some hint.
>
> Regards,
> -Bin.
Re: [PATCH v4 1/6] dt-bindings: usb: musb: Add support for MediaTek musb controller
Hi Rob,
Sorry to bother you again.
Can I describe usb-connector in usb node like this?
usb2: usb@1120 {
compatible = "mediatek,mt2701-musb";
"mediatek,mtk-musb";
reg = <0 0x1120 0 0x1000>;
...
usb_connector: musb_connector {
compatible = "usb-b-connector";
label = "micro-USB";
type = "micro";
extcon = <&extcon_usb>;
vbus-supply = <&usb_vbus>;
};
};
Regards,
Min.
On Fri, 2019-01-25 at 10:07 +0800, Min Guo wrote:
> Hi Bin,
>
> Thanks for your help.
>
> Hi Rob,
>
> I find that Samsung describes the usb-connector attribute in DTS, and
> uses a private driver.
> And try to write DTS as following:
>
> usb-connector node:
> musb_con: musb_connector{
> compatible = "linux,extcon-usb-gpio","usb-b-connector";
> lable = "micro-USB";
> type = "micro";
> id-gpio = <&pio 44 GPIO_ACTIVE_HIGH>
> vbus-supply = <&usb_vbus>;
> port {
> usb_to_connector: endpoint {
> remote-endpoint = <&connector_to_usb>;
> };
> };
> };
>
> usb node:
> &usb2{
> status = "okay";
> port {
> connector_to_usb: endpoint {
> remote-endpoint = <&usb_to_connector>;
> };
> };
> }
>
> Can I describe usb-connector like this? Or can you give me some advices?
>
> Regards,
> Min.
>
> On Tue, 2019-01-22 at 08:33 -0600, Bin Liu wrote:
> > Hi Min,
> >
> > On Tue, Jan 22, 2019 at 05:36:13PM +0800, Min Guo wrote:
> > > Hi Bin,
> > >
> > > Sorry to bother you again, I encounter a problem about the extcon
> > > property.
> > >
> > > I don'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?
> > > If it needs to implement by myself, is it possible to emulate an
> > > usb-connector driver by extcon-usb-gpio, and also use the notifier
> > > mechanism or can you give me some advices?
> >
> > I am afraid I am unable to help you on this. I wasn't really pay
> > attention when usb-connector was introduced and not sure how it can
> > replace extcon. Now after read usb-connector.txt, it seems the binding
> > only defines a/b/c-connector, but not ab-connector, and there is no
> > enough information (at least for me) explaining how VBUS and ID fix into
> > this usb-connector binding.
> >
> > Maybe Rob can provide some hint.
> >
> > Regards,
> > -Bin.
>
Re: [PATCH v5 1/6] dt-bindings: usb: musb: Add support for MediaTek musb controller
Hi Rob,
On Fri, 2019-02-22 at 10:49 -0600, Rob Herring wrote:
> On Tue, Feb 19, 2019 at 03:36:30PM +0800, min@mediatek.com wrote:
> > From: Min Guo
> >
> > This adds support for MediaTek musb controller in
> > host, peripheral and otg mode.
> >
> > Signed-off-by: Min Guo
> > ---
> > changes in v5:
> > suggested by Rob:
> > 1. Modify compatible as
> > - compatible : should be one of:
> >"mediatek,mt-2701"
> >...
> >followed by "mediatek,mtk-musb"
> > 2. Add usb connector child node
> >
> > changes in v4:
> > suggested by Sergei:
> > 1. String alignment
> >
> > changes in v3:
> > 1. no changes
> >
> > changes in v2:
> > suggested by Bin:
> > 1. Modify DRC to DRD
> > suggested by Rob:
> > 2. Drop the "-musb" in compatible
> > 3. Remove phy-names
> > 4. Add space after comma in clock-names
> > ---
> > .../devicetree/bindings/usb/mediatek,musb.txt | 54
> > ++
> > 1 file changed, 54 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 000..0632e6e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/mediatek,musb.txt
> > @@ -0,0 +1,54 @@
> > +MediaTek musb DRD/OTG controller
> > +---
> > +
> > +Required properties:
> > + - compatible : should be one of:
> > + "mediatek,mt-2701"
>
> That looks like a top-level SoC compatible and doesn't match the
> example.
"mediatek,mt-2701" is an example, just to describe a specific SOC.
May I modify compatibel like:
compatible : should be "mediatek,mtk-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"
>
> Not much point in a name when there is only 1.
The MUSB core driver has two interrupts, one is for MAC, another for
DMA, but on MTK platform, there is only a MAC interrupt, musb core
driver uses platform_get_irq_byname(pdev, "mc") to get this interrupt
number.
> > + - phys: PHY specifier for the OTG 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:
> > + - power-domains : a phandle to USB power domain node to control USB's
> > + MTCMOS
> > +
> > +Required child nodes:
> > + usb connector node as defined in bindings/connector/usb-connector.txt
> > + - extcon : for VBUS and ID pin changes detection, needed when
> > + supports dual-role mode
> > + - vbus-supply : reference to the VBUS regulator, needed when supports
> > + dual-role mode
> > +
> > +Example:
> > +
> > +usb2: usb@1120 {
> > + compatible = "mediatek,mt2701-musb",
> > +"mediatek,mtk-musb";
> > + reg = <0 0x1120 0 0x1000>;
> > + interrupts = ;
> > + interrupt-names = "mc";
> > + phys = <&u2port2 PHY_TYPE_USB2>;
> > + 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>;
> > + usb_con: connector{
> > + compatible = "usb-b-connector";
> > + label = "micro-USB";
> > + type = "micro";
> > + extcon = <&extcon_usb>;
>
> The connector node should replace 'extcon'. Add what you need here
> directly (e.g. id-gpios). What you add needs to be defined in
> usb-connector.txt.
OK.
> > + vbus-supply = <&usb_vbus>;
> > + };
> > +};
> > --
> > 1.9.1
> >
Regards,
Min.
Re: [PATCH v5 1/6] dt-bindings: usb: musb: Add support for MediaTek musb controller
Hi Rob, On Mon, 2019-03-04 at 11:42 -0600, Rob Herring wrote: > On Sun, Feb 24, 2019 at 7:36 PM Min Guo wrote: > > > > Hi Rob, > > On Fri, 2019-02-22 at 10:49 -0600, Rob Herring wrote: > > > On Tue, Feb 19, 2019 at 03:36:30PM +0800, min@mediatek.com wrote: > > > > From: Min Guo > > > > > > > > This adds support for MediaTek musb controller in > > > > host, peripheral and otg mode. > > > > > > > > Signed-off-by: Min Guo > > > > --- > > > > changes in v5: > > > > suggested by Rob: > > > > 1. Modify compatible as > > > > - compatible : should be one of: > > > >"mediatek,mt-2701" > > > >... > > > >followed by "mediatek,mtk-musb" > > > > 2. Add usb connector child node > > > > > > > > changes in v4: > > > > suggested by Sergei: > > > > 1. String alignment > > > > > > > > changes in v3: > > > > 1. no changes > > > > > > > > changes in v2: > > > > suggested by Bin: > > > > 1. Modify DRC to DRD > > > > suggested by Rob: > > > > 2. Drop the "-musb" in compatible > > > > 3. Remove phy-names > > > > 4. Add space after comma in clock-names > > > > --- > > > > .../devicetree/bindings/usb/mediatek,musb.txt | 54 > > > > ++ > > > > 1 file changed, 54 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 000..0632e6e > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/usb/mediatek,musb.txt > > > > @@ -0,0 +1,54 @@ > > > > +MediaTek musb DRD/OTG controller > > > > +--- > > > > + > > > > +Required properties: > > > > + - compatible : should be one of: > > > > + "mediatek,mt-2701" > > > > > > That looks like a top-level SoC compatible and doesn't match the > > > example. > > "mediatek,mt-2701" is an example, just to describe a specific SOC. > > May I modify compatibel like: > > compatible : should be "mediatek,mtk-musb" > > No, you should fix the example adding "mediatek,mtk-musb". OK. > Rob Regards, Min.
