RE: [PATCH v3 4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups
> On 04.12.18 21:01, Fabio Estevam wrote:
> > Hi Frieder,
> >
> > On Tue, Dec 4, 2018 at 12:31 PM Schrempf Frieder
> > 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
Re: [PATCH v3 4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups
Hi Fabio,
On 04.12.18 21:01, Fabio Estevam wrote:
> Hi Frieder,
>
> On Tue, Dec 4, 2018 at 12:31 PM Schrempf Frieder
> 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
RE: [RFC PATCH v2 04/15] usb:cdns3: Driver initialization code.
Hi,
>> >>
>> >> Patch adds core.c and core.h file that implements initialization
>> >> of platform driver and adds function responsible for selecting,
>> >> switching and running appropriate Device/Host mode.
>> >>
>> >> Signed-off-by: Pawel Laszczak
>> >> ---
>> >> drivers/usb/cdns3/Makefile | 2 +
>> >> drivers/usb/cdns3/core.c | 413 +
>> >> drivers/usb/cdns3/core.h | 100 +
>> >> 3 files changed, 515 insertions(+)
>> >> create mode 100644 drivers/usb/cdns3/core.c
>> >> create mode 100644 drivers/usb/cdns3/core.h
>> >>
>> >> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
>> >> index dcdd62003c6a..02d25b23c5d3 100644
>> >> --- a/drivers/usb/cdns3/Makefile
>> >> +++ b/drivers/usb/cdns3/Makefile
>> >> @@ -1,3 +1,5 @@
>> >> +obj-$(CONFIG_USB_CDNS3)+= cdns3.o
>> >> obj-$(CONFIG_USB_CDNS3_PCI_WRAP) += cdns3-pci.o
>> >>
>> >> +cdns3-y:= core.o
>> >> cdns3-pci-y:= cdns3-pci-wrap.o
>> >> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
>> >> new file mode 100644
>> >> index ..f9055d4da67f
>> >> --- /dev/null
>> >> +++ b/drivers/usb/cdns3/core.c
>> >> @@ -0,0 +1,413 @@
>> >> +// SPDX-License-Identifier: GPL-2.0
>> >> +/*
>> >> + * Cadence USBSS DRD Driver.
>> >> + *
>> >> + * Copyright (C) 2018 Cadence.
>> >> + *
>> >
>> >Please add NXP copyright too.
>>
>> Ok, I don't know why I omitted this.
>> I know that you are the main author of this file
>> Sorry for that.
>>
>> One additional question. What year I should add in Copyright for NXP?.
>> The original year 2017 or I should modified all to 2018.
>>
>Please use below copyright, thanks.
>
>Copyright 2017-2018 NXP
I add this in all files modified or created by you.
>
>
>
>> >> + mutex_init(&cdns->mutex);
>> >> +
>> >> + cdns->phy = devm_phy_get(dev, "cdns3,usbphy");
>> >> + if (IS_ERR(cdns->phy)) {
>> >> + dev_info(dev, "no generic phy found\n");
>> >> + cdns->phy = NULL;
>> >> + /*
>> >> +* fall through here!
>> >> +* if no generic phy found, phy init
>> >> +* should be done under boot!
>> >> +*/
>> >
>> >If the phy driver is defer-probed, it will be here, it is not an error.
>> >I think you could have a generic phy driver or usb generic phy driver
>> >(drivers/usb/phy/phy-generic.c) even you don't need any operations for
>> >PHY. It will be easy for other platforms.
>>
>> Yes, Roger ask me to modify this fragment. In next version it will look like:
>> cdns->phy = devm_phy_get(dev, "cdns3,usbphy");
>> if (IS_ERR(cdns->phy)) {
>> ret = PTR_ERR(cdns->phy);
>> if (ret == -ENOSYS || ret == -ENODEV) {
>> cdns->phy = NULL;
>> } else if (ret == -EPROBE_DEFER) {
>> return ret;
>> } else {
>> dev_err(dev, "no phy found\n");
>> goto err0;
>> }
>> }
>>
>> phy_init(cdns->phy);
>>
>> We are going to use phy driver. I don't know if it correct.
>> I don't have experience in this filed.
>> We need phy initialization but I don't have testing platform now.
>> In most usb drivers I see that there are used usb phy driverd instead phy
>> dirverd.
>>
>
>At my CDNS3 platform, there are some USB PHY initialization for register
>setting
>and clock enable. You could add generic PHY driver under: drivers/phy/cadence/.
>
>Above PHY initialization code is OK for me.
It will be added as separate driver.
I think that Allan Douglas working on it.
I ask him to add you to -cc in patch for phy.
>
>
>> >> +static void __exit cdns3_driver_platform_unregister(void)
>> >> +{
>> >> + platform_driver_unregister(&cdns3_driver);
>> >> +}
>> >> +module_exit(cdns3_driver_platform_unregister);
>> >> +
>> >> +MODULE_ALIAS("platform:cdns3");
>> >> +MODULE_AUTHOR("Pawel Laszczak ");
>> >> +MODULE_LICENSE("GPL v2");
>> >> +MODULE_DESCRIPTION("Cadence USB3 DRD Controller Driver");
>> >> diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
>> >> new file mode 100644
>> >> index ..7c8204fe4d3d
>> >> --- /dev/null
>> >> +++ b/drivers/usb/cdns3/core.h
>> >> @@ -0,0 +1,100 @@
>> >> +/* SPDX-License-Identifier: GPL-2.0 */
>> >> +/*
>> >> + * Cadence USBSS DRD Driver.
>> >> + *
>> >
>> >Header file
>> I don't understand. What is wrong ?
>> >
>
>The comment for this file
>
>Cadence USBSS DRD Core Header File
Ok, thanks
Cheers,
Pawel
Re: [PATCH] USB: qcaux: Add Motorola modem UARTs
On Sun, Dec 02, 2018 at 05:34:24PM -0800, Tony Lindgren wrote: > On Motorola Mapphone devices such as Droid 4 there are five USB ports > that do not use the same layout as Gobi 1K/2K/etc devices listed in > qcserial.c. So we should use qcaux.c or option.c as noted by > Dan Williams . > > The ff/ff/ff interfaces seem to always be UARTs on Motorola devices. > And we should not add interfaces with 0x0a class (CDC Data) as they > are part of a multi-interface function like for example interface > 0x22b8:0x4281 as noted by Bjørn Mork . Can you post the output of usb-devices (or lsusb -v) for these three devices (PIDs)? Thanks, Johan
Re: [PATCH 1/2 v4] USB: serial: mos7840: Adjust port settings for read and write registers
On Fri, Nov 30, 2018 at 02:31:21PM +0800, Jackychou wrote:
> From: JackyChou
>
> In the read/write function, set port 2 independently in the 2-port case.
>
> When setting the offset of port registers, the offset between port 1 and
> other ports is different, so port 1 is set independently.
> Then in the rest of ports, the port 2 between 2-ports case and 4-ports case
> is different, so port 2 in 2-ports case is set independently.
>
> Signed-off-by: JackyChou
> ---
Thanks for the update.
> + } else {
> + u8 port_offset;
> +
> + if ((mos7840_port->port_num == 2) && (serial->num_ports == 2))
> + port_offset = 1;
> + else
> + port_offset = mos7840_port->port_num - 2;
> + mos7840_port->SpRegOffset = 0x8 + (2 * port_offset);
> + mos7840_port->ControlRegOffset = 0x9 + (2 * port_offset);
> + mos7840_port->DcrRegOffset = 0x16 + (3 * port_offset);
I simplified this further as:
} else {
u8 phy_num = mos7840_port->port_num;
/* Port 2 in the 2-port case uses registers of port 3 */
if (serial->num_ports == 2)
phy_num = 3;
mos7840_port->SpRegOffset = 0x8 + 2 * (phy_num - 2);
mos7840_port->ControlRegOffset = 0x9 + 2 * (phy_num - 2);
mos7840_port->DcrRegOffset = 0x16 + 3 * (phy_num - 2);
before applying.
Johan
Re: [PATCH v1 10/12] hikey960: Support usb functionality of Hikey960
Hi, On 2018/12/5 1:47, Andy Shevchenko wrote: > On Tue, Dec 4, 2018 at 3:40 AM Chen Yu wrote: >> On 2018/12/3 17:23, Andy Shevchenko wrote: >>> On Mon, Dec 3, 2018 at 5:47 AM Yu Chen wrote: > + .of_match_table = of_match_ptr(id_table_hisi_hikey_usb), >>> >>> Does it compiles for non-OF case? Why macro is in use? > >> OK. I will add the "CONFIG_OF". > Why? Is this driver supposed to work without DT? > No. This driver should depend on OF. Can I solve this by add "dpends on OF" in Kconfig?
Re: [PATCH v1 09/12] usb: dwc3: Registering a role switch in the DRD code.
On 2018/12/4 18:54, Heikki Krogerus wrote: > On Mon, Dec 03, 2018 at 11:45:12AM +0800, Yu Chen wrote: >> The Type-C drivers use USB role switch API to inform the >> system about the negotiated data role, so registering a role >> switch in the DRD code in order to support platforms with >> USB Type-C connectors. >> >> Cc: Felipe Balbi >> Cc: Greg Kroah-Hartman >> Signed-off-by: Yu Chen >> Signed-off-by: Heikki Krogerus >> >> -- >> v0: >> The patch is provided by Heikki Krogerus. I modified and test it >> on Hikey960 platform. >> -- >> --- >> drivers/usb/dwc3/core.h | 2 ++ >> drivers/usb/dwc3/drd.c | 49 >> + >> 2 files changed, 51 insertions(+) > > You need to select USB_ROLE_SWITCH in Kconfig: > > diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig > index 1a0404fda596..3a0cb9f1f38a 100644 > --- a/drivers/usb/dwc3/Kconfig > +++ b/drivers/usb/dwc3/Kconfig > @@ -42,6 +42,7 @@ config USB_DWC3_DUAL_ROLE > bool "Dual Role mode" > depends on ((USB=y || USB=USB_DWC3) && (USB_GADGET=y || > USB_GADGET=USB_DWC3)) > depends on (EXTCON=y || EXTCON=USB_DWC3) > + select USB_ROLE_SWITCH > help > This is the default mode of working of DWC3 controller where > both host and gadget features are enabled. > OK. Thanks!
[PATCH 2/3] usb: appledisplay: Set urb transfer_flags to URB_NO_TRANSFER_DMA_MAP
From: Alexander Theissen
The driver does allocate a DMA address with usb_alloc_coherent but did
not set the appropriate flag to signal that transfer_dma is set to a
valid value.
Signed-off-by: Alexander Theissen
---
drivers/usb/misc/appledisplay.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/misc/appledisplay.c b/drivers/usb/misc/appledisplay.c
index 39ca31b4de46..2f3c4769238d 100644
--- a/drivers/usb/misc/appledisplay.c
+++ b/drivers/usb/misc/appledisplay.c
@@ -261,6 +261,7 @@ static int appledisplay_probe(struct usb_interface *iface,
usb_rcvintpipe(udev, int_in_endpointAddr),
pdata->urbdata, ACD_URB_BUFFER_LEN, appledisplay_complete,
pdata, 1);
+ pdata->urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
if (usb_submit_urb(pdata->urb, GFP_KERNEL)) {
retval = -EIO;
dev_err(&iface->dev, "Submitting URB failed\n");
--
2.19.1
[PATCH 3/3] usb: appledisplay: Remove unnecessary spinlock
From: Alexander Theissen
The spinlock was inside the urb completion function which is only
called once per display and is then resubmitted from this function.
There was no other place where this lock was used.
Signed-off-by: Alexander Theissen
---
drivers/usb/misc/appledisplay.c | 7 ---
1 file changed, 7 deletions(-)
diff --git a/drivers/usb/misc/appledisplay.c b/drivers/usb/misc/appledisplay.c
index 2f3c4769238d..ac92725458b5 100644
--- a/drivers/usb/misc/appledisplay.c
+++ b/drivers/usb/misc/appledisplay.c
@@ -69,7 +69,6 @@ struct appledisplay {
struct delayed_work work;
int button_pressed;
- spinlock_t lock;
struct mutex sysfslock; /* concurrent read and write */
};
@@ -79,7 +78,6 @@ static void appledisplay_complete(struct urb *urb)
{
struct appledisplay *pdata = urb->context;
struct device *dev = &pdata->udev->dev;
- unsigned long flags;
int status = urb->status;
int retval;
@@ -105,8 +103,6 @@ static void appledisplay_complete(struct urb *urb)
goto exit;
}
- spin_lock_irqsave(&pdata->lock, flags);
-
switch(pdata->urbdata[1]) {
case ACD_BTN_BRIGHT_UP:
case ACD_BTN_BRIGHT_DOWN:
@@ -119,8 +115,6 @@ static void appledisplay_complete(struct urb *urb)
break;
}
- spin_unlock_irqrestore(&pdata->lock, flags);
-
exit:
retval = usb_submit_urb(pdata->urb, GFP_ATOMIC);
if (retval) {
@@ -229,7 +223,6 @@ static int appledisplay_probe(struct usb_interface *iface,
pdata->udev = udev;
- spin_lock_init(&pdata->lock);
INIT_DELAYED_WORK(&pdata->work, appledisplay_work);
mutex_init(&pdata->sysfslock);
--
2.19.1
[PATCH 0/3] Small improvements to the appledisplay driver
With the exception of the first patch I am not entirely sure if my changes are correct and justified. This is my first contribution and I am equally satisfied if someone could point out why my changes are not correct. The added display is my own and works well with the driver. drivers/usb/misc/appledisplay.c | 9 ++--- 1 file changed, 2 insertions(+), 7 deletions(-)
[PATCH 1/3] usb: appledisplay: Add 27" Apple Cinema Display
From: Alexander Theissen
Add another Apple Cinema Display to the list of supported displays.
Signed-off-by: Alexander Theissen
---
drivers/usb/misc/appledisplay.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/misc/appledisplay.c b/drivers/usb/misc/appledisplay.c
index 85b48c6ddc7e..39ca31b4de46 100644
--- a/drivers/usb/misc/appledisplay.c
+++ b/drivers/usb/misc/appledisplay.c
@@ -51,6 +51,7 @@ static const struct usb_device_id appledisplay_table[] = {
{ APPLEDISPLAY_DEVICE(0x921c) },
{ APPLEDISPLAY_DEVICE(0x921d) },
{ APPLEDISPLAY_DEVICE(0x9222) },
+ { APPLEDISPLAY_DEVICE(0x9226) },
{ APPLEDISPLAY_DEVICE(0x9236) },
/* Terminating entry */
--
2.19.1
Re: [RFC PATCH v2 03/15] dt-bindings: add binding for USBSS-DRD controller.
On Sun, Nov 18, 2018 at 10:08:59AM +, Pawel Laszczak wrote:
> Thsi patch aim at documenting USB related dt-bindings for the
typo
> Cadence USBSS-DRD controller.
>
> Signed-off-by: Pawel Laszczak
> ---
> .../devicetree/bindings/usb/cdns3-usb.txt | 17 +
> 1 file changed, 17 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/usb/cdns3-usb.txt
>
> diff --git a/Documentation/devicetree/bindings/usb/cdns3-usb.txt
> b/Documentation/devicetree/bindings/usb/cdns3-usb.txt
> new file mode 100644
> index ..f9e953f32d47
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/cdns3-usb.txt
> @@ -0,0 +1,17 @@
> +Binding for the Cadence USBSS-DRD controller
> +
> +Required properties:
> + - reg: Physical base address and size of the controller's register area.
1 or 3 regions? If 3, what is each one and the order?
> + - compatible: Should contain: "cdns,usb3"
Only one version and one set of bugs?
> + - interrupts: Interrupt specifier. Refer to interrupt bindings.
How many interrupts?
> +
> +
> +Example:
> + cdns3@f300 {
usb@...
> + compatible = "cdns,usb3";
> + interrupts = ;
> + reg = <0xf300 0x1 //memory area for OTG/DRD
> registers
> + 0xf301 0x1 //memory area for HOST registers
> + 0xf302 0x1>;//memory area for Device
> registers
> + };
> +
> --
> 2.17.1
>
Re: [PATCH] USB: quirks: add NO_LPM quirk for Logitech Flare|Meetup|Brio|Rally
On Tue, 4 Dec 2018, Kyle Williams wrote:
> Description: Some USB device / host controller combinations seem to have
> problems with Link Power management. In particular it is described that
> the combination of certain Logitech devices and other powered media
> devices such as the Atrus device causes 'not enough bandwidth for
> new device state'error.
>
> This patch creates quirk entries for the tested Logitech device
> indicating LPM should remain disabled for the device.
>
> Signed-off-by: Kyle Williams
> ---
> drivers/usb/core/quirks.c | 16
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
> index 0690fcff0ea2..9403edee4797 100644
> --- a/drivers/usb/core/quirks.c
> +++ b/drivers/usb/core/quirks.c
> @@ -246,6 +246,22 @@ static const struct usb_device_id usb_quirk_list[] = {
> /* Logitech Harmony 700-series */
> { USB_DEVICE(0x046d, 0xc122), .driver_info = USB_QUIRK_DELAY_INIT },
>
> + /* Logitech Flare */
> + { USB_DEVICE(0x046d, 0x0876), .driver_info = USB_QUIRK_NO_LPM },
This entry is out of order with the preceding entry. And some of the
new entries below are out of order with each other (entries are
supposed to be sorted by Vendor ID, then Product ID).
Also, perhaps instead of adding all these new entries, we should set
the NO_LPM quirk flag for all Logitech devices?
Alan Stern
> +
> + /* Logitech Rally Camera */
> + { USB_DEVICE(0x046d, 0x0881), .driver_info = USB_QUIRK_NO_LPM },
> + { USB_DEVICE(0x046d, 0x0888), .driver_info = USB_QUIRK_NO_LPM },
> + { USB_DEVICE(0x046d, 0x0889), .driver_info = USB_QUIRK_NO_LPM },
> +
> + /* Logitech Meetup */
> + { USB_DEVICE(0x046d, 0x0867), .driver_info = USB_QUIRK_NO_LPM },
> + { USB_DEVICE(0x046d, 0x0866), .driver_info = USB_QUIRK_NO_LPM },
> + { USB_DEVICE(0x046d, 0x086a), .driver_info = USB_QUIRK_NO_LPM },
> +
> + /* Logitech Brio */
> + { USB_DEVICE(0x046d, 0x085e), .driver_info = USB_QUIRK_NO_LPM },
> +
> /* Philips PSC805 audio device */
> { USB_DEVICE(0x0471, 0x0155), .driver_info = USB_QUIRK_RESET_RESUME
> },
>
>
Re: [PATCH v3 4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups
Hi Frieder, On Tue, Dec 4, 2018 at 12:31 PM Schrempf Frieder 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?
RE: [PATCH v7 01/10] usb: gadget: udc: Add timer support for usb requests
On Tue, 4 Dec 2018, Anurag Kumar Vulisha wrote: > >Is there any way for the device controller driver to detect the problem > >without relying > >on a timeout? > > > >In fact, is this problem a known bug in the existing device controller > >hardware? Have > >the controller manufacturers published a suggested scheme for working around > >it? > > > > Yes, this problem is mentioned in dwc3 controller data book and the workaround > mentioned is the same that we are doing , to implement timeout and if no > valid > stream event is found , after timeout issue end transfer followed by start > transfer. Okay. But this implies that the problem is confined to DWC3 and doesn't affect other types of controllers, which would mean modifying the UDC core would be inappropriate. Does the data book suggest a value for the timeout? > >At this point, it seems that the generic approach will be messier than > >having every > >controller driver implement its own fix. At least, that's how it appears to > >me. (Especially if dwc3 is the only driver affected.) > With the dequeue approach there is an advantage(not related to this issue) > that the > class driver can have control of the timed out transfer whether to requeue it > or consider > it as an error (based on implementation). This approach gives more > flexibility to the class > drivers. This may be out of context of this issue but wanted to point out > that we may lose > this advantage on switching to older implementation. Class drivers can cancel and requeue requests at any time. There's no extra flexibility. > >Ideally it would not be necessary to rely on a timeout at all. > > > >Also, maintainers dislike module parameters. It would be better not to add > >one. > > Okay. I would be happy if any alternative for this issue is present but > unfortunately > I am not able to figure out any alternative other than timers. If not > module_params() > we can add an configfs entry in stream gadget to update the timeout. Please > provide > your opinion on this approach. Since the purpose of the timeout is to detect a deadlock caused by a hardware bug, I suggest a fixed and relatively short timeout value such as one second. Cancelling and requeuing a few requests at 1-second intervals shouldn't add very much overhead. Alan Stern
RE: [PATCH v7 01/10] usb: gadget: udc: Add timer support for usb requests
Hi Alan, >-Original Message- >From: Alan Stern [mailto:st...@rowland.harvard.edu] >Sent: Tuesday, December 04, 2018 10:17 PM >To: Anurag Kumar Vulisha >Cc: Felipe Balbi ; Greg Kroah-Hartman >; Shuah Khan ; Johan Hovold >; Jaejoong Kim ; Benjamin >Herrenschmidt ; Roger Quadros ; Manu >Gautam ; martin.peter...@oracle.com; Bart Van >Assche ; Mike Christie ; Matthew >Wilcox ; Colin Ian King ; linux- >u...@vger.kernel.org; linux-ker...@vger.kernel.org; v.anuragku...@gmail.com; >Thinh Nguyen ; Tejas Joglekar >; Ajay Yugalkishore Pandey >Subject: RE: [PATCH v7 01/10] usb: gadget: udc: Add timer support for usb >requests > >On Tue, 4 Dec 2018, Anurag Kumar Vulisha wrote: > >> >> Yes the host can cancel the transfer. This issue originated from >> >> the endpoints using >> >bulk >> >> streaming protocol and may not occur with normal endpoints. AFAIK >> >> bulk streaming >> >is >> >> gadget driven, where the gadget is allowed to select which stream >> >> id transfer the >> >host >> >> should work on . Since the host doesn't have control on when the >> >> transfer would be selected by gadget, it may wait for longer timeouts >> >> before >cancelling the transfer. >> > >> >You're missing the point. Although the device selects which stream >> >ID gets transferred, the _host_ decides whether a stream transfer >> >should occur in the first place. No matter how many ERDY packets the >> >device controller tries to send, no transfer will occur until the >> >host wants to do it. >> > >> >In this sense, stream transfers (like all other USB interactions >> >except wakeup requests) are host-driven. >> > >> >> I agree with you that without host willing to transfer, the transfer >> wouldn't have started and also agree the host always has the >> capability of detecting the hang. But we are not sure on what host >> platform and operating system the gadget would be tested and also not sure >whether the host software is capable of handling the deadlock. >> Even if the host has a timer , it would be very long and waiting for >> the timer to get timedout would reduce the performance. In this patch >> we are giving the user the flexibility to opt the timeout value, so >> that the deadlock can be avoided without effecting the performance. So >> I think adding the timer in gadget is more easier solution than handling in >> host. I >have seen this workaround working for both linux & windows. > >Is there any way for the device controller driver to detect the problem >without relying >on a timeout? > >In fact, is this problem a known bug in the existing device controller >hardware? Have >the controller manufacturers published a suggested scheme for working around >it? > Yes, this problem is mentioned in dwc3 controller data book and the workaround mentioned is the same that we are doing , to implement timeout and if no valid stream event is found , after timeout issue end transfer followed by start transfer. >> >> >> Since the gadget >> >> >> controller driver is aware that the controller is stuck , makes >> >> >> it responsible to recover the controller from hang condition by >> >> >> restarting the transfer (which triggers the controller FSM to issue >> >> >> ERDY to >host). >> >> > >> >> >Isn't there a cleaner way to recover than by cancelling the >> >> >request and resubmitting it? >> >> > >> >> >> >> dequeuing the request issues the stop transfer command to the >> >> controller, which cleans all the hardware resource allocated for >> >> that endpoint. This also resets the hardware FSMs for that endpoint >> >> . So, when re-queuing of the transfer happens the controller starts >> >> allocating hardware resources again, thus avoiding the >> >probability >> >> of entering into the issue. I am not sure of other controllers, but >> >> for dwc3, issuing the stop transfer is the only way to handle this issue. >> > >> >Again you're missing the point. Can't the controller driver issue >> >the Stop Transfer command but still consider the request to be in >> >progress (i.e., don't dequeue the request) so that the gadget >> >driver's completion callback isn't invoked and the request does not >> >need to be explicitly resubmitted? >> > >> >> Yes , what you are saying is correct. My initial patches were >> following the same approach that you have suggested. We switched to >> the dequeue approach because there can be different gadgets which may >> enter into this issue and it would be better to follow a generic >> approach rather than having every controller driver implementing their own >workaround. >> Please find the conservation in this link >> https://patchwork.kernel.org/patch/10640145/ > >At this point, it seems that the generic approach will be messier than having >every >controller driver implement its own fix. At least, that's how it appears to >me. With the dequeue approach there is an advantage(not related to this issue) that the class driver can have control of the timed out transfer whether to requeue it or
dwc2 isochronous transfers issues
Hi All, I have recently noticed a problem with dwc2 and audio gadget. The expected behavior: The uac2 function acts as a kind of a pass-through. For example to play audio from USB host on an Odroid U3 the following command is issued at the host: aplay -D plughw:CARD=Gadget,DEV=0 /usr/share/sounds/alsa/Front_Center.wav while the following command is used at the device: arecord -f dat -t wav -D plughw:CARD=UAC2Gadget,DEV=0 \| aplay -D default:CARD=OdroidU3 At the device we "record" from the audio device provided by the usb gadget and pipe the resulting stream to the actual hardware device present in the chip on board. What actually happens: On recent kernels this does not work in a stable manner and sometimes the kernel panics. I traced the problem back and it seems that the last kernel without the below problems is 4.7. I am using a compiled-in, legacy audio gadget with only uac2 function selected in Kconfig. 4.7 works 4.8 kernel crashes at boot time (bisect identifies 381fc8f8228923026b3d75c8230fa2ee4d688f32 "usb: dwc2: gadget: Add Incomplete ISO IN/OUT Interrupt handlers"), compiling as a module does not help (crash at module load time), composing the gagdet with configfs does not help either. 4.9 kernel crashes at boot time 4.10kernel crashes at boot time 4.11kernel crashes at boot time 4.12kernel crashes at boot time 4.13kernel crashes at boot time 4.14kernel usually crashes at boot time, but sometimes it boots, if it boots, it crashes the same way as at boot time when the gadget is actually used (aplay at host and arecord | aplay at the device) 4.15kernel sometimes crashes at boot time, but otherwise it boots, if it boots, it crashes the same way as at boot time when the gadget is actually used (aplay at host and arecord | aplay at the device) 4.16kernel crashes at boot time 4.17kernel sometimes crashes at boot time, but otherwise it boots, if it boots, it crashes the same way as at boot time when the gadget is actually used (aplay at host and arecord | aplay at the device) 4.18kernel sometimes crashes at boot time or even silently freezes, but otherwise it boots, if it boots, it crashes the same way as at boot time when the gadget is actually used (aplay at host and arecord | aplay at the device) 4.19 and 4.20-rc4 kernel boots, but after some time the gadget stops working: at the device with 4.19: # arecord -f dat -t wav -D plughw:CARD=UAC2Gadget,DEV=0 | aplay -D default:CARD=OdroidU3 Recording WAVE 'stdin' : Signed 16 bit Little Endian, Rate 48000 Hz, Stereo Playing WAVE 'stdin' : Signed 16 bit Little Endian, Rate 48000 Hz, Stereo [ 26.245056] max98090 1-0010: PLL unlocked underrun!!! (at least 1256.128 ms long) [ 29.363400] max98090 1-0010: PLL unlocked underrun!!! (at least 930.612 ms long) underrun!!! (at least 447.733 ms long) [ 34.475390] max98090 1-0010: PLL unlocked underrun!!! (at least 319.622 ms long) [ 36.678554] max98090 1-0010: PLL unlocked underrun!!! (at least 611.690 ms long) [ 39.153660] max98090 1-0010: PLL unlocked underrun!!! (at least 724.196 ms long) [ 41.741488] max98090 1-0010: PLL unlocked underrun!!! (at least 695.816 ms long) [ 44.428490] max98090 1-0010: PLL unlocked underrun!!! (at least 795.061 ms long) [ 47.106276] max98090 1-0010: PLL unlocked underrun!!! (at least 760.757 ms long) underrun!!! (at least 2685.446 ms long) [ 54.288479] max98090 1-0010: PLL unlocked underrun!!! (at least 1591.909 ms long) [ 57.764445] max98090 1-0010: PLL unlocked underrun!!! (at least 223.942 ms long) underrun!!! (at least 237.751 ms long) [ 61.974963] max98090 1-0010: PLL unlocked underrun!!! (at least 677.925 ms long) [ 64.645406] max98090 1-0010: PLL unlocked underrun!!! (at least 550.651 ms long) [ 67.079379] max98090 1-0010: PLL unlocked underrun!!! (at least 1575.582 ms long) [ 70.521155] max98090 1-0010: PLL unlocked underrun!!! (at least 832.604 ms long) [ 73.219814] max98090 1-0010: PLL unlocked underrun!!! (at least 721.516 ms long) while at the host: $ aplay -D plughw:CARD=Gadget,DEV=0 /usr/share/sounds/alsa/Front_Center.wav Playing WAVE '/usr/share/sounds/alsa/Front_Center.wav' : Signed 16 bit Little Endian, Rate 48000 Hz, Mono several times ok $ aplay -D plughw:CARD=Gadget,DEV=0 /usr/share/sounds/alsa/Front_Center.wav Playing WAVE '/usr/share/sounds/alsa/Front_Center.wav' : Signed 16 bit Little Endian, Rate 48000 Hz, Mono aplay: set_params:1297: Unable to install hw params: ACCESS: RW_INTERLEAVED FORMAT: S16_LE SUBFORMAT: STD SAMPLE_BITS: 16 FRAME_BITS: 16 CHANNELS: 1 RATE: 48000 PERIOD_TIME: 125000 PERIOD_SIZE: 6000 PERIOD_BYTES: 12000 PERIODS: 4 BUFFER_TIME: 50 BUFFER_SIZE: 24000 BUFFER_BYTES: 48000 TICK_TIME: 0 Any ideas on how to test it further to identify the problem? Definitely the commit identified with bisect was a problem at least until 4.13. Per
Re: [PATCH v1 10/12] hikey960: Support usb functionality of Hikey960
On Tue, Dec 4, 2018 at 3:40 AM Chen Yu wrote: > On 2018/12/3 17:23, Andy Shevchenko wrote: > > On Mon, Dec 3, 2018 at 5:47 AM Yu Chen wrote: > >> + .of_match_table = of_match_ptr(id_table_hisi_hikey_usb), > > > > Does it compiles for non-OF case? Why macro is in use? > OK. I will add the "CONFIG_OF". Why? Is this driver supposed to work without DT? -- With Best Regards, Andy Shevchenko
RE: [PATCH v7 01/10] usb: gadget: udc: Add timer support for usb requests
On Tue, 4 Dec 2018, Anurag Kumar Vulisha wrote: > >> Yes the host can cancel the transfer. This issue originated from the > >> endpoints using > >bulk > >> streaming protocol and may not occur with normal endpoints. AFAIK bulk > >> streaming > >is > >> gadget driven, where the gadget is allowed to select which stream id > >> transfer the > >host > >> should work on . Since the host doesn't have control on when the transfer > >> would be > >> selected by gadget, it may wait for longer timeouts before cancelling the > >> transfer. > > > >You're missing the point. Although the device selects which stream ID > >gets transferred, the _host_ decides whether a stream transfer should > >occur in the first place. No matter how many ERDY packets the device > >controller tries to send, no transfer will occur until the host wants > >to do it. > > > >In this sense, stream transfers (like all other USB interactions except > >wakeup requests) are host-driven. > > > > I agree with you that without host willing to transfer, the transfer wouldn't > have > started and also agree the host always has the capability of detecting the > hang. But > we are not sure on what host platform and operating system the gadget would be > tested and also not sure whether the host software is capable of handling the > deadlock. > Even if the host has a timer , it would be very long and waiting for the > timer to get > timedout would reduce the performance. In this patch we are giving the user > the > flexibility to opt the timeout value, so that the deadlock can be avoided > without > effecting the performance. So I think adding the timer in gadget is more > easier solution > than handling in host. I have seen this workaround working for both linux & > windows. Is there any way for the device controller driver to detect the problem without relying on a timeout? In fact, is this problem a known bug in the existing device controller hardware? Have the controller manufacturers published a suggested scheme for working around it? > >> >> Since the gadget > >> >> controller driver is aware that the controller is stuck , makes it > >> >> responsible > >> >> to recover the controller from hang condition by restarting the > >> >> transfer (which > >> >> triggers the controller FSM to issue ERDY to host). > >> > > >> >Isn't there a cleaner way to recover than by cancelling the request and > >> >resubmitting it? > >> > > >> > >> dequeuing the request issues the stop transfer command to the controller, > >> which > >> cleans all the hardware resource allocated for that endpoint. This also > >> resets the > >> hardware FSMs for that endpoint . So, when re-queuing of the transfer > >> happens > >> the controller starts allocating hardware resources again, thus avoiding > >> the > >probability > >> of entering into the issue. I am not sure of other controllers, but for > >> dwc3, issuing > >> the stop transfer is the only way to handle this issue. > > > >Again you're missing the point. Can't the controller driver issue the > >Stop Transfer command but still consider the request to be in progress > >(i.e., don't dequeue the request) so that the gadget driver's > >completion callback isn't invoked and the request does not need to be > >explicitly resubmitted? > > > > Yes , what you are saying is correct. My initial patches were following the > same approach that you have suggested. We switched to the dequeue > approach because there can be different gadgets which may enter into > this issue and it would be better to follow a generic approach rather than > having every controller driver implementing their own workaround. > Please find the conservation in this link > https://patchwork.kernel.org/patch/10640145/ At this point, it seems that the generic approach will be messier than having every controller driver implement its own fix. At least, that's how it appears to me. > >> @Felipe: Can you please provide your suggestion on this. > > > >> >How can the gadget driver know what timeout to use? The host is > >> >allowed to be as slow as it wants; the gadget driver doesn't have any > >> >way to tell when the host wants to start the transfer. > >> > >> Yes , I agree with you that the timeout may vary from usage to usage. This > >> timeout > >> should be decided by the class driver which queues the request. As > >> discussed above > >> this issue was observed in streaming protocol , which is very much faster > >> than > >normal > >> BOT modes and it works on super speed . > > > >Although USB mass storage is currently the only user of the stream > >protocol, that may not be true in the future. You should think in more > >general terms. A timeout which is appropriate for mass storage may not > >be appropriate for some other application. > > > > You are correct , this timeout may not be ideal. Since I was not able to > reproduce this issue > on non-stream capable transfers , at present the only user of > usb_ep_queu
RE: scsi_set_medium_removal timeout issue
On Thu, 29 Nov 2018, Zengtao (B) wrote:
> Ping?
>
> >-Original Message-
> >From: Alan Stern [mailto:st...@rowland.harvard.edu]
> >Sent: Wednesday, November 14, 2018 11:35 PM
> >To: Martin Petersen ; Zengtao (B)
> >
> >Cc: j...@linux.vnet.ibm.com; gre...@linuxfoundation.org;
> >linux-s...@vger.kernel.org; linux-ker...@vger.kernel.org;
> >linux-usb@vger.kernel.org; usb-stor...@lists.one-eyed-alien.net
> >Subject: RE: scsi_set_medium_removal timeout issue
> >
> >On Wed, 14 Nov 2018, Zengtao (B) wrote:
> >
> >> I just enabled the scsi log in the middle of the umount operation,
> >> otherwise I can't reproduce the issue when the scsi log is enabled.
> >>
> >> >from the beginning. In any case, it wasn't what I wanted. I asked
> >> >you to post the dmesg log, not the SCSI log.
> >>
> >> Please refer to the new attachment for dmesg log.
> >
> >Heh, yes, I see now.
> >
> >Martin, shouldn't sd_release() call sd_sync_cache() in the same way that
> >sd_shutdown() does, before it calls scsi_set_medium_removal()?
> >
> >Alan Stern
I don't know if this is the right thing to do, but you can try out the
following patch to see if it helps.
Alan Stern
Index: usb-4.x/drivers/scsi/sd.c
===
--- usb-4.x.orig/drivers/scsi/sd.c
+++ usb-4.x/drivers/scsi/sd.c
@@ -113,6 +113,7 @@ static void sd_shutdown(struct device *)
static int sd_suspend_system(struct device *);
static int sd_suspend_runtime(struct device *);
static int sd_resume(struct device *);
+static int sd_sync_cache(struct scsi_disk *sdkp, struct scsi_sense_hdr *sshdr);
static void sd_rescan(struct device *);
static int sd_init_command(struct scsi_cmnd *SCpnt);
static void sd_uninit_command(struct scsi_cmnd *SCpnt);
@@ -1393,8 +1394,14 @@ static void sd_release(struct gendisk *d
SCSI_LOG_HLQUEUE(3, sd_printk(KERN_INFO, sdkp, "sd_release\n"));
if (atomic_dec_return(&sdkp->openers) == 0 && sdev->removable) {
- if (scsi_block_when_processing_errors(sdev))
+ if (scsi_block_when_processing_errors(sdev)) {
+ if (sdkp->WCE && sdkp->media_present) {
+ sd_printk(KERN_NOTICE, sdkp,
+ "Synchronizing SCSI cache\n");
+ sd_sync_cache(sdkp, NULL);
+ }
scsi_set_medium_removal(sdev, SCSI_REMOVAL_ALLOW);
+ }
}
/*
Re: [PATCH] USB: serial: console: fix reported terminal settings
On Tue, Dec 04, 2018 at 05:15:18PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Dec 04, 2018 at 05:00:36PM +0100, Johan Hovold wrote:
> > The USB-serial console implementation has never reported the actual
> > terminal settings used. Despite storing the corresponding cflags in its
> > struct console, this was never honoured on later tty open() where the
> > tty termios would be left initialised to the driver defaults.
> >
> > Unlike the serial console implementation, the USB-serial code calls
> > subdriver open() already at console setup. While calling set_termios()
> > before open() looks like it could work for some USB-serial drivers,
> > others definitely do not expect this, so modelling this after serial
> > core is going to be intrusive, if at all possible.
> >
> > Instead, use a (renamed) tty helper to save the termios data used at
> > console setup, so that the tty termios reflects the actual terminal
> > settings after a subsequent tty open().
> >
> > Note that the calls to tty_init_termios() (tty_driver_install()) and
> > tty_save_termios() are serialised using the disconnect mutex.
> >
> > This specifically fixes a regression that was triggered by a recent
> > change adding software flow control to the pl2303 driver: a getty trying
> > to disable flow control while leaving the baud rate unchanged would now
> > also set the baud rate to the driver default (prior to the flow-control
> > change this had been a noop).
> >
> > Fixes: 7041d9c3f01b ("USB: serial: pl2303: add support for tx xon/xoff flow
> > control")
> > Cc: stable # 4.18
> > Reported-by: Jarkko Nikula
> > Cc: Florian Zumbiehl
> > Signed-off-by: Johan Hovold
> > ---
> > drivers/tty/tty_io.c | 11 +--
> > drivers/usb/serial/console.c | 2 +-
> > include/linux/tty.h | 1 +
> > 3 files changed, 11 insertions(+), 3 deletions(-)
>
> Ah, messy :)
>
> Want me to take this through my tty tree?
If you prefer. I was planning on including this in a USB-serial pull
request for -rc6 since it fixes a user-reported regression, but perhaps
taking this through your tty-linus branch (which already holds a console
fix) is easier/faster.
We should wait for Jarkko to confirm that this fixes the problem he
reported first, though.
Thanks,
Johan
RE: [PATCH v7 01/10] usb: gadget: udc: Add timer support for usb requests
Hi Alan, >-Original Message- >From: Alan Stern [mailto:st...@rowland.harvard.edu] >Sent: Tuesday, December 04, 2018 4:38 AM >To: Anurag Kumar Vulisha >Cc: Felipe Balbi ; Greg Kroah-Hartman >; Shuah Khan ; Johan Hovold >; Jaejoong Kim ; Benjamin >Herrenschmidt ; Roger Quadros ; Manu >Gautam ; martin.peter...@oracle.com; Bart Van >Assche ; Mike Christie ; Matthew >Wilcox ; Colin Ian King ; linux- >u...@vger.kernel.org; linux-ker...@vger.kernel.org; v.anuragku...@gmail.com; >Thinh Nguyen ; Tejas Joglekar >; Ajay Yugalkishore Pandey >Subject: RE: [PATCH v7 01/10] usb: gadget: udc: Add timer support for usb >requests > >On Mon, 3 Dec 2018, Anurag Kumar Vulisha wrote: > >> >On Mon, 3 Dec 2018, Anurag Kumar Vulisha wrote: >> > >> >> >First of all, if some sort of deadlock causes a transfer to fail to >> >> >complete, the host is expected to cancel and restart it. Not the >> >> >gadget. >> >> > >> >> >> >> Thanks for spending your time in reviewing this patch. The deadlock >> >> is a very rare case scenario and is happening because both the gadget >> >> controller & host controllers get out of sync and are stuck waiting for >> >> the >> >> relevant event. For example this issue is observed in stream protocol >> >> where >> >> the gadget controller is waiting on Host controller to issue PRIME >> >> transaction >> >> and Host controller is waiting on gadget to issue ERDY transaction. Since >> >> the stream protocol is gadget driven, the host may not proceed further >> >> until it >> >> receives a valid Start Stream (ERDY) transaction from gadget. >> > >> >That's not entirely true. Can't the host cancel the transfer and then >> >restart it? >> > >> >> Yes the host can cancel the transfer. This issue originated from the >> endpoints using >bulk >> streaming protocol and may not occur with normal endpoints. AFAIK bulk >> streaming >is >> gadget driven, where the gadget is allowed to select which stream id >> transfer the >host >> should work on . Since the host doesn't have control on when the transfer >> would be >> selected by gadget, it may wait for longer timeouts before cancelling the >> transfer. > >You're missing the point. Although the device selects which stream ID >gets transferred, the _host_ decides whether a stream transfer should >occur in the first place. No matter how many ERDY packets the device >controller tries to send, no transfer will occur until the host wants >to do it. > >In this sense, stream transfers (like all other USB interactions except >wakeup requests) are host-driven. > I agree with you that without host willing to transfer, the transfer wouldn't have started and also agree the host always has the capability of detecting the hang. But we are not sure on what host platform and operating system the gadget would be tested and also not sure whether the host software is capable of handling the deadlock. Even if the host has a timer , it would be very long and waiting for the timer to get timedout would reduce the performance. In this patch we are giving the user the flexibility to opt the timeout value, so that the deadlock can be avoided without effecting the performance. So I think adding the timer in gadget is more easier solution than handling in host. I have seen this workaround working for both linux & windows. >> >> Since the gadget >> >> controller driver is aware that the controller is stuck , makes it >> >> responsible >> >> to recover the controller from hang condition by restarting the transfer >> >> (which >> >> triggers the controller FSM to issue ERDY to host). >> > >> >Isn't there a cleaner way to recover than by cancelling the request and >> >resubmitting it? >> > >> >> dequeuing the request issues the stop transfer command to the controller, >> which >> cleans all the hardware resource allocated for that endpoint. This also >> resets the >> hardware FSMs for that endpoint . So, when re-queuing of the transfer happens >> the controller starts allocating hardware resources again, thus avoiding the >probability >> of entering into the issue. I am not sure of other controllers, but for >> dwc3, issuing >> the stop transfer is the only way to handle this issue. > >Again you're missing the point. Can't the controller driver issue the >Stop Transfer command but still consider the request to be in progress >(i.e., don't dequeue the request) so that the gadget driver's >completion callback isn't invoked and the request does not need to be >explicitly resubmitted? > Yes , what you are saying is correct. My initial patches were following the same approach that you have suggested. We switched to the dequeue approach because there can be different gadgets which may enter into this issue and it would be better to follow a generic approach rather than having every controller driver implementing their own workaround. Please find the conservation in this link https://patchwork.kernel.org/patch/10640145/ >> @Felipe: Can you please provide y
Re: [PATCH] USB: serial: console: fix reported terminal settings
On Tue, Dec 04, 2018 at 05:00:36PM +0100, Johan Hovold wrote:
> The USB-serial console implementation has never reported the actual
> terminal settings used. Despite storing the corresponding cflags in its
> struct console, this was never honoured on later tty open() where the
> tty termios would be left initialised to the driver defaults.
>
> Unlike the serial console implementation, the USB-serial code calls
> subdriver open() already at console setup. While calling set_termios()
> before open() looks like it could work for some USB-serial drivers,
> others definitely do not expect this, so modelling this after serial
> core is going to be intrusive, if at all possible.
>
> Instead, use a (renamed) tty helper to save the termios data used at
> console setup, so that the tty termios reflects the actual terminal
> settings after a subsequent tty open().
>
> Note that the calls to tty_init_termios() (tty_driver_install()) and
> tty_save_termios() are serialised using the disconnect mutex.
>
> This specifically fixes a regression that was triggered by a recent
> change adding software flow control to the pl2303 driver: a getty trying
> to disable flow control while leaving the baud rate unchanged would now
> also set the baud rate to the driver default (prior to the flow-control
> change this had been a noop).
>
> Fixes: 7041d9c3f01b ("USB: serial: pl2303: add support for tx xon/xoff flow
> control")
> Cc: stable# 4.18
> Reported-by: Jarkko Nikula
> Cc: Florian Zumbiehl
> Signed-off-by: Johan Hovold
> ---
> drivers/tty/tty_io.c | 11 +--
> drivers/usb/serial/console.c | 2 +-
> include/linux/tty.h | 1 +
> 3 files changed, 11 insertions(+), 3 deletions(-)
Ah, messy :)
Want me to take this through my tty tree?
thanks,
greg k-h
[PATCH] USB: serial: console: fix reported terminal settings
The USB-serial console implementation has never reported the actual
terminal settings used. Despite storing the corresponding cflags in its
struct console, this was never honoured on later tty open() where the
tty termios would be left initialised to the driver defaults.
Unlike the serial console implementation, the USB-serial code calls
subdriver open() already at console setup. While calling set_termios()
before open() looks like it could work for some USB-serial drivers,
others definitely do not expect this, so modelling this after serial
core is going to be intrusive, if at all possible.
Instead, use a (renamed) tty helper to save the termios data used at
console setup, so that the tty termios reflects the actual terminal
settings after a subsequent tty open().
Note that the calls to tty_init_termios() (tty_driver_install()) and
tty_save_termios() are serialised using the disconnect mutex.
This specifically fixes a regression that was triggered by a recent
change adding software flow control to the pl2303 driver: a getty trying
to disable flow control while leaving the baud rate unchanged would now
also set the baud rate to the driver default (prior to the flow-control
change this had been a noop).
Fixes: 7041d9c3f01b ("USB: serial: pl2303: add support for tx xon/xoff flow
control")
Cc: stable # 4.18
Reported-by: Jarkko Nikula
Cc: Florian Zumbiehl
Signed-off-by: Johan Hovold
---
drivers/tty/tty_io.c | 11 +--
drivers/usb/serial/console.c | 2 +-
include/linux/tty.h | 1 +
3 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index ee80dfbd5442..687250ec8032 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1373,7 +1373,13 @@ struct tty_struct *tty_init_dev(struct tty_driver
*driver, int idx)
return ERR_PTR(retval);
}
-static void tty_free_termios(struct tty_struct *tty)
+/**
+ * tty_save_termios() - save tty termios data in driver table
+ * @tty: tty whose termios data to save
+ *
+ * Locking: Caller guarantees serialisation with tty_init_termios().
+ */
+void tty_save_termios(struct tty_struct *tty)
{
struct ktermios *tp;
int idx = tty->index;
@@ -1392,6 +1398,7 @@ static void tty_free_termios(struct tty_struct *tty)
}
*tp = tty->termios;
}
+EXPORT_SYMBOL_GPL(tty_save_termios);
/**
* tty_flush_works - flush all works of a tty/pty pair
@@ -1491,7 +1498,7 @@ static void release_tty(struct tty_struct *tty, int idx)
WARN_ON(!mutex_is_locked(&tty_mutex));
if (tty->ops->shutdown)
tty->ops->shutdown(tty);
- tty_free_termios(tty);
+ tty_save_termios(tty);
tty_driver_remove_tty(tty->driver, tty);
tty->port->itty = NULL;
if (tty->link)
diff --git a/drivers/usb/serial/console.c b/drivers/usb/serial/console.c
index 17940589c647..7d289302ff6c 100644
--- a/drivers/usb/serial/console.c
+++ b/drivers/usb/serial/console.c
@@ -101,7 +101,6 @@ static int usb_console_setup(struct console *co, char
*options)
cflag |= PARENB;
break;
}
- co->cflag = cflag;
/*
* no need to check the index here: if the index is wrong, console
@@ -164,6 +163,7 @@ static int usb_console_setup(struct console *co, char
*options)
serial->type->set_termios(tty, port, &dummy);
tty_port_tty_set(&port->port, NULL);
+ tty_save_termios(tty);
tty_kref_put(tty);
}
tty_port_set_initialized(&port->port, 1);
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 414db2bce715..392138fe59b6 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -556,6 +556,7 @@ extern struct tty_struct *tty_init_dev(struct tty_driver
*driver, int idx);
extern void tty_release_struct(struct tty_struct *tty, int idx);
extern int tty_release(struct inode *inode, struct file *filp);
extern void tty_init_termios(struct tty_struct *tty);
+extern void tty_save_termios(struct tty_struct *tty);
extern int tty_standard_install(struct tty_driver *driver,
struct tty_struct *tty);
--
2.19.2
Re: pl2303 regression after v4.17. Bisected to 7041d9c3f01b
On Mon, Nov 26, 2018 at 02:29:53PM +0200, Jarkko Nikula wrote: > On 11/24/18 7:32 PM, Florian Zumbiehl wrote: > > Hi, > > > >> Requested baud setting looks odd to me. Maybe related to this > >> --keep-baud flag in "/sbin/agetty -o -p -- \u --keep-baud > >> 115200,38400,9600 ttyUSB0 vt220"? That indeed seems to be the case. > > Well, what is the baud rate of the other side? It seems a bit strange > > that ... > > > 115200. > > >> 1. serial-getty@ttyUSB0.service disabled > >> > >> speed 9600 baud; rows 0; columns 0; line = 0; > > > > ... this works ... > > > >> 3. serial-getty@ttyUSB0.service disabled && picocom -b 115200 /dev/ttyUSB0 > >> > >> speed 115200 baud; rows 0; columns 0; line = 0; > > > > ... and this works as well?! > > > > I think before debugging a potential problem with the new flow control > > feature, it would be a good idea to first make sure the baud rate is > > configured correctly. Maybe agetty's baud rate selection is somehow > > influenced by the new flow control support? I don't see how it would, and > > maybe that's still a bug somehow, but I suspect that there is no flow > > control problem at all, but rather it's just a baud rate mismatch that > > prevents communication. > > > I went debugging the commit and pl2303_set_termios() a bit. I noticed > that the tty_termios_hw_change() returns zero when starting the getty > service that keeps the existing baud. Same goes with the plain "picocom > /dev/ttyUSB0" without specifying the baud which also works before commit > 7041d9c3f01b. > > For both cases ixon_change is true so the pl2303_termios_change() > returns true causing the pl2303_set_termios() to go forward doing the > baudrate (incorrectly 9600 instead of 115200) etc. changes which doesn't > happen before commit 7041d9c3f01b. > > This incorrect 9600 baud made me thinking could it be possible that > driver doesn't initialize some structure with the current settings the > kernel console is using? Would this also explain also why > "stty -a -F /dev/ttyUSB0" shows 9600 when kernel console is certainly > using 115200 (after boot before running neither agetty or picocom)? Correct. The USB-serial console code has never reported the actual settings used, but with the pl2303 flow control change this now obviously broke your setup. Judging from your reports, including the stty outputs, it seems like the getty is trying to disable flow control while keeping the baud rate unchanged. Prior to the pl2303 change this was a noop which left the baud rate unchanged, but now you end up with the driver default 9600 baud. > I verified the same setup using legacy 8250 uart and with it the > "stty -a -F /dev/ttyS0" shows correctly 115200 baud. Right, the serial console correctly updates the terminal settings, unlike the USB-serial console code. I've come up with a non-intrusive fix which fixes the incorrectly reported terminal settings. Would you mind giving it a try? Thanks, Johan
Re: USB driver resets
On Tue, Dec 04, 2018 at 04:45:33PM +0200, Mathias Nyman wrote: > On 03.12.2018 12:36, Richard van der Hoff wrote: > > Does anybody have any suggestions as to how I could set about debugging > > this? I'm seeing the same symptoms on a 4.19 kernel. > > > > > > On 19/11/2018 15:27, Richard van der Hoff wrote: > > > I have a USB-3.1 dock, which I use for video (via displayport alt mode) > > > and power delivery, as well as keyboard, mouse, etc. > > > > > > From time to time, all connectivity with the dock drops out for a few > > > seconds, before resetting itself. I'm trying to pin down if this is a > > > problem with the usb drivers, pci drivers, laptop hardware, or the dock. > > > It looks to me from the dmesg output like it could be a problem at the > > > PCI layer, but I am only speculating really. > > > > > > Attached are the dmesg output during the problem, lsusb -v, and lspci -v. > > > They are all recorded with a 4.15.0 kernel; I've seen exactly the same > > > symptoms with a number of kernels between 4.8 and 4.15. > > > > > > Advice appreciated! > > > > > Mika (cc) would be the right person to contact about these kind of > thunderbolt related issues. > I'm sure he would appreciate logs with a 4.19 - 4.20-rcx kernel. Right, those would help and also some knowledge about which system and dock we are dealing with :) Also if you have dual boot or similar, do you experience similar issue in Windows side?
Re: USB driver resets
On 03.12.2018 12:36, Richard van der Hoff wrote: Does anybody have any suggestions as to how I could set about debugging this? I'm seeing the same symptoms on a 4.19 kernel. On 19/11/2018 15:27, Richard van der Hoff wrote: I have a USB-3.1 dock, which I use for video (via displayport alt mode) and power delivery, as well as keyboard, mouse, etc. From time to time, all connectivity with the dock drops out for a few seconds, before resetting itself. I'm trying to pin down if this is a problem with the usb drivers, pci drivers, laptop hardware, or the dock. It looks to me from the dmesg output like it could be a problem at the PCI layer, but I am only speculating really. Attached are the dmesg output during the problem, lsusb -v, and lspci -v. They are all recorded with a 4.15.0 kernel; I've seen exactly the same symptoms with a number of kernels between 4.8 and 4.15. Advice appreciated! Mika (cc) would be the right person to contact about these kind of thunderbolt related issues. I'm sure he would appreciate logs with a 4.19 - 4.20-rcx kernel. -Mathias
[PATCH] usb: core: Remove unnecessary memset()
register_root_hub() calls memset() setting usb_dev->bus->devmap. devicemap to 0 during hcd probe function (usb_hcd_pci_probe). But in previous function which is also the procedure of usb_hcd_pci_probe(), usb_bus_init() already initialized bus->devmap calling memset(). Furthermore, register_root_hub() is called only once in kernel. So, calling memset() which resets usb_bus->devmap.devicemap in register_root_hub() is redundant. Signed-off-by: Suwan Kim --- drivers/usb/core/hcd.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c index 487025d31d44..015b126ce455 100644 --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -1074,8 +1074,6 @@ static int register_root_hub(struct usb_hcd *hcd) usb_dev->devnum = devnum; usb_dev->bus->devnum_next = devnum + 1; - memset (&usb_dev->bus->devmap.devicemap, 0, - sizeof usb_dev->bus->devmap.devicemap); set_bit (devnum, usb_dev->bus->devmap.devicemap); usb_set_device_state(usb_dev, USB_STATE_ADDRESS); -- 2.19.2
Re: [PATCH v3 4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups
Hi Peter, hi Fabio, On 30.11.18 03:33, PETER CHEN wrote: > >> >> On Tue, Nov 27, 2018 at 7:31 AM PETER CHEN 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 >>> --- >>> 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
Re: [PATCH] usb: dwc2: Disable power down feature on Samsung SoCs
Hi Marek,
On 11/20/2018 19:38, Marek Szyprowski wrote:
> Power down feature of DWC2 module integrated in Samsung SoCs doesn't work
> properly or needs some additional handling in PHY or SoC glue layer, so
> disable it for now. Without disabling power down, DWC2 causes random memory
> trashes and fails enumeration if there is no USB link to host on driver
> probe.
>
> Fixes: 03ea6d6e9e1ff1 ("usb: dwc2: Enable power down")
> Signed-off-by: Marek Szyprowski
> ---
> drivers/usb/dwc2/params.c | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
> index 7c1b6938f212..266157ae179a 100644
> --- a/drivers/usb/dwc2/params.c
> +++ b/drivers/usb/dwc2/params.c
> @@ -71,6 +71,13 @@ static void dwc2_set_his_params(struct dwc2_hsotg *hsotg)
> p->power_down = false;
> }
>
> +static void dwc2_set_s3c6400_params(struct dwc2_hsotg *hsotg)
> +{
> + struct dwc2_core_params *p = &hsotg->params;
> +
> + p->power_down = 0;
> +}
> +
> static void dwc2_set_rk_params(struct dwc2_hsotg *hsotg)
> {
> struct dwc2_core_params *p = &hsotg->params;
> @@ -151,7 +158,8 @@ const struct of_device_id dwc2_of_match_table[] = {
> { .compatible = "lantiq,arx100-usb", .data = dwc2_set_ltq_params },
> { .compatible = "lantiq,xrx200-usb", .data = dwc2_set_ltq_params },
> { .compatible = "snps,dwc2" },
> - { .compatible = "samsung,s3c6400-hsotg" },
> + { .compatible = "samsung,s3c6400-hsotg",
> + .data = dwc2_set_s3c6400_params },
> { .compatible = "amlogic,meson8-usb",
> .data = dwc2_set_amlogic_params },
> { .compatible = "amlogic,meson8b-usb",
>
Could you please provide dmesg logs with verbose Debug enabled
configuration and the register dump.
So that we can see what the issue is related to.
Regards,
Artur
Re: [PATCH v3 1/3] usb: chipidea: imx: support configuring for active low oc signal
Hello, On Tue, Dec 04, 2018 at 12:43:21PM +0100, Stefan Wahren wrote: > Am 04.12.18 um 09:52 schrieb Uwe Kleine-König: > > But for the review you are right, I added the dt people to Cc for them > > to comment. In v2 Matthew also noted that he would prefer to handle the > > situation when both over-current-active-low and over-current-active-high > > were given differently. I think we don't need that as this is a case of > > "broken dt" and it doesn't matter much what is done then. (With my patch > > we're configuring for active high in that case.) A feedback here would > > be great, too. > > AFAIR such invalid settings should be catched with a proper error > message and abort of the probing. I remember a review commend from Rob[1]: "It is not really the kernel's job to validate crap in bindings. If you put [strange things] in your DT, then the kernel may or may not handle that." If this applies here, too, the current patches are fine. Best regards Uwe [1] https://patchwork.kernel.org/patch/8776441/ -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"
On Tue, Dec 04, 2018 at 12:34:08PM +, Minas Harutyunyan wrote:
> @@ -3185,12 +3183,13 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
> hsotg->connected = 0;
> hsotg->test_mode = 0;
>
> - /* all endpoints should be shutdown */
> for (ep = 0; ep < hsotg->num_of_eps; ep++) {
> if (hsotg->eps_in[ep])
> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> + kill_all_requests(hsotg, hsotg->eps_in[ep],
> + -ESHUTDOWN);
> if (hsotg->eps_out[ep])
> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> + kill_all_requests(hsotg, hsotg->eps_out[ep],
> + -ESHUTDOWN);
Should this part be in a separate patch?
I'm not trying to be rhetorical at all. I literally don't know the
code very well. Hopefully the full commit message will explain it.
> }
>
> call_gadget(hsotg, disconnect);
> @@ -3234,6 +3233,8 @@ static void dwc2_hsotg_irq_fifoempty(struct
> dwc2_hsotg *hsotg, bool periodic)
> GINTSTS_PTXFEMP | \
> GINTSTS_RXFLVL)
>
> +static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
> +
> /**
>* dwc2_hsotg_core_init - issue softreset to the core
>* @hsotg: The device state
> @@ -3258,12 +3259,14 @@ void dwc2_hsotg_core_init_disconnected(struct
> dwc2_hsotg *hsotg,
> return;
> } else {
> /* all endpoints should be shutdown */
> + spin_unlock(&hsotg->lock);
> for (ep = 1; ep < hsotg->num_of_eps; ep++) {
> if (hsotg->eps_in[ep])
>
> dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> if (hsotg->eps_out[ep])
>
> dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> }
> + spin_lock(&hsotg->lock);
> }
>
> /*
The idea here is that this is the only caller which is holding the
lock and we drop it here and take it again inside dwc2_hsotg_ep_disable().
I don't know the code very well and can't totally swear that this
doesn't introduce a small race condition...
Another option would be to introduce a new function which takes the lock
and change all the other callers instead. To me that would be easier to
review... See below for how it might look:
regards,
dan carpenter
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 94f3ba995580..b17a5dbefd5f 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3166,6 +3166,7 @@ static void kill_all_requests(struct dwc2_hsotg *hsotg,
}
static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
+static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep);
/**
* dwc2_hsotg_disconnect - disconnect service
@@ -3188,9 +3189,9 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
/* all endpoints should be shutdown */
for (ep = 0; ep < hsotg->num_of_eps; ep++) {
if (hsotg->eps_in[ep])
- dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
+ dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
if (hsotg->eps_out[ep])
- dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
+ dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
}
call_gadget(hsotg, disconnect);
@@ -4069,10 +4070,8 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
struct dwc2_hsotg *hsotg = hs_ep->parent;
int dir_in = hs_ep->dir_in;
int index = hs_ep->index;
- unsigned long flags;
u32 epctrl_reg;
u32 ctrl;
- int locked;
dev_dbg(hsotg->dev, "%s(ep %p)\n", __func__, ep);
@@ -4088,10 +4087,6 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
- locked = spin_is_locked(&hsotg->lock);
- if (!locked)
- spin_lock_irqsave(&hsotg->lock, flags);
-
ctrl = dwc2_readl(hsotg, epctrl_reg);
if (ctrl & DXEPCTL_EPENA)
@@ -4114,12 +4109,23 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
hs_ep->fifo_index = 0;
hs_ep->fifo_size = 0;
- if (!locked)
- spin_unlock_irqrestore(&hsotg->lock, flags);
-
return 0;
}
+static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep)
+{
+ struct dwc2_hsotg_ep *hs_ep = our_ep(ep);
+ struct dwc2_hsotg *hsotg = hs_ep->parent;
+ unsigned long flags;
+ int ret;
+
+ spin_lock_irqsave(&hsotg->lock, flags);
+ ret = dwc2_hsotg_ep_disable(ep);
+ spin_unlock_irqrestore(&hsotg->lock, flags);
+
+ return ret;
+}
+
/**
* on_list - check request is on the given endpoint
* @ep: The endpoint to check.
@@ -4267,7 +4273,7 @@ static int dwc2_hso
Re: [PATCH v1 12/12] dts: hi3660: Add support for usb on Hikey960
On Mon, Dec 03, 2018 at 11:45:15AM +0800, Yu Chen wrote:
> This patch adds support for usb on Hikey960.
>
> Cc: Wei Xu
> Cc: Rob Herring
> Cc: Mark Rutland
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: John Stultz
> Cc: Binghui Wang
> Signed-off-by: Yu Chen
> ---
> arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts | 56 ++
> arch/arm64/boot/dts/hisilicon/hi3660.dtsi | 72
> +++
> 2 files changed, 128 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts
> b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts
> index c98bcbc8dfba..066c558154ea 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts
> +++ b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts
> @@ -13,6 +13,7 @@
> #include
> #include
> #include
> +#include
>
> / {
> model = "HiKey960";
> @@ -196,6 +197,28 @@
> method = "smc";
> };
> };
> +
> + hisi_hikey_usb: hisi_hikey_usb {
> + compatible = "hisilicon,hikey960_usb";
> + typec-vbus-gpios = <&gpio25 2 0>;
> + typc-vbus-enable-val = <1>;
> + otg-switch-gpios = <&gpio25 6 0>;
> + hub-vdd33-en-gpios = <&gpio5 6 0>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&usbhub5734_pmx_func>;
> +
> + port {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + hikey_usb_ep: endpoint@0 {
> + reg = <0>;
> + device_type = "usb-role-switch";
> + remote-endpoint = <&dwc3_role_switch_notify>;
> + };
> + };
> + };
> +
> };
>
> /*
> @@ -526,6 +549,39 @@
> &i2c1 {
> status = "okay";
>
> + rt1711h: rt1711h@4e {
> + compatible = "richtek,rt1711h";
> + reg = <0x4e>;
> + status = "ok";
> + interrupt-parent = <&gpio27>;
> + interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&usb_cfg_func>;
> +
> + usb_con: connector {
> + compatible = "usb-c-connector";
> + label = "USB-C";
> + data-role = "dual";
> + power-role = "dual";
> + try-power-role = "sink";
> + source-pdos = PDO_FIXED_USB_COMM)>;
> + sink-pdos = + PDO_VAR(5000, 5000, 1000)>;
> + op-sink-microwatt = <1000>;
> + };
> +
> + port {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + rt1711h_ep: endpoint@0 {
> + reg = <0>;
> + device_type = "usb-role-switch";
device_type is deprecated, no?
In any case, I don't think we can use the endpoint subnode for
matching. To play it safe, I think we should use the remote port
parent for matching. See below..
> + remote-endpoint = <&dwc3_role_switch>;
> + };
> + };
> + };
> +
> adv7533: adv7533@39 {
> status = "ok";
> compatible = "adi,adv7533";
> diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> index f432b0a88c65..e6583bd7efdb 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> @@ -349,6 +349,12 @@
> #clock-cells = <1>;
> };
>
> + pmctrl: pmctrl@fff31000 {
> + compatible = "hisilicon,hi3660-pmctrl", "syscon";
> + reg = <0x0 0xfff31000 0x0 0x1000>;
> + #clock-cells = <1>;
> + };
> +
> pmuctrl: crg_ctrl@fff34000 {
> compatible = "hisilicon,hi3660-pmuctrl", "syscon";
> reg = <0x0 0xfff34000 0x0 0x1000>;
> @@ -1122,5 +1128,71 @@
> };
> };
> };
> +
> + usb3_otg_bc: usb3_otg_bc@ff20 {
> + compatible = "syscon";
> + reg = <0x0 0xff20 0x0 0x1000>;
> + };
> +
> + usb_phy: usb-phy {
> + compatible = "hisilicon,hi3660-usb-phy";
> + #phy-cells = <0>;
> + hisilicon,pericrg-syscon = <&crg_ctrl>;
> + hisilicon,pctrl-syscon = <&pctrl>;
> + hisilicon,usb3-otg-bc-syscon = <&usb3_otg_bc>;
> + hisilicon,eye-diagram-param = <0x22466e4>;
> + };
> +
> + usb3: hisi_dwc3 {
> + compatible = "hisilicon,hi3660-dwc3";
> + #address-cells =
Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"
On 11/23/2018 6:43 PM, Dan Carpenter wrote:
> Ugh... We also had a long thread about the v2 patch but it turns out
> the list was not CC'd. I should have checked for that.
>
> You have to pass a flag to say if the caller holds the lock or not...
>
> regards,
> dan carpenter
>
Hi Dan, Marek, Maynard,
Could you please apply bellow patch over follow patches:
dccf1bad4be7 usb: dwc2: Disable all EP's on disconnect
6f774b501928 usb: dwc2: Fix ep disable spinlock flow.
Please review and test. Feedback is appreciated :-)
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 8eb25e3597b0..db438d13c36a 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3165,8 +3165,6 @@ static void kill_all_requests(struct dwc2_hsotg
*hsotg,
dwc2_hsotg_txfifo_flush(hsotg, ep->fifo_index);
}
-static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
-
/**
* dwc2_hsotg_disconnect - disconnect service
* @hsotg: The device state.
@@ -3185,12 +3183,13 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
hsotg->connected = 0;
hsotg->test_mode = 0;
- /* all endpoints should be shutdown */
for (ep = 0; ep < hsotg->num_of_eps; ep++) {
if (hsotg->eps_in[ep])
- dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
+ kill_all_requests(hsotg, hsotg->eps_in[ep],
+ -ESHUTDOWN);
if (hsotg->eps_out[ep])
- dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
+ kill_all_requests(hsotg, hsotg->eps_out[ep],
+ -ESHUTDOWN);
}
call_gadget(hsotg, disconnect);
@@ -3234,6 +3233,8 @@ static void dwc2_hsotg_irq_fifoempty(struct
dwc2_hsotg *hsotg, bool periodic)
GINTSTS_PTXFEMP | \
GINTSTS_RXFLVL)
+static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
+
/**
* dwc2_hsotg_core_init - issue softreset to the core
* @hsotg: The device state
@@ -3258,12 +3259,14 @@ void dwc2_hsotg_core_init_disconnected(struct
dwc2_hsotg *hsotg,
return;
} else {
/* all endpoints should be shutdown */
+ spin_unlock(&hsotg->lock);
for (ep = 1; ep < hsotg->num_of_eps; ep++) {
if (hsotg->eps_in[ep])
dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
if (hsotg->eps_out[ep])
dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
}
+ spin_lock(&hsotg->lock);
}
/*
@@ -4072,7 +4075,6 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
unsigned long flags;
u32 epctrl_reg;
u32 ctrl;
- int locked;
dev_dbg(hsotg->dev, "%s(ep %p)\n", __func__, ep);
@@ -4088,7 +4090,7 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
- locked = spin_trylock_irqsave(&hsotg->lock, flags);
+ spin_lock_irqsave(&hsotg->lock, flags);
ctrl = dwc2_readl(hsotg, epctrl_reg);
@@ -4112,8 +4114,7 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
hs_ep->fifo_index = 0;
hs_ep->fifo_size = 0;
- if (locked)
- spin_unlock_irqrestore(&hsotg->lock, flags);
+ spin_unlock_irqrestore(&hsotg->lock, flags);
return 0;
}
Thanks,
Minas
FROM MISS DIANA HOLLAND
Attention, Good day to you, how are you? My name is Diana Holland. I am a U.S. Army Specialist, serving here in Afghanistan. I found a box containing the sum of $10,000,000.00 USD (Ten Million United States Dollars) due to my status as a U.S. Army Specialist, i can’t be able to transfer these funds to my account in United States. I need your help to evacuate this funds and safe keep it until i finish my service here and come to collect my share of it. The box will be addressed as family valuables from me to you. If this is possible for you to do, i need your full assurance that it will be safe in your care. You may be wondering why i decided to communicate with you, I have no one to discuss this with. I am an orphan, please assist me to secure a brighter future. Trusting someone is very difficult, but i give you my trust, also i need your trust to make this a success. Your acceptance to this would really encourage me. Kindly let me know your conclusion to my request so we can proceed to get this completed soon. With a sincere heart, I am willing to give you 35% from the total sum for your assistance and willingness to help me. Please do not betray my trust and confidence in you. If you truly desire to help me, please reply me quickly via my Personal email: (diana.hollan...@yahoo.com ) Regards, Diana Holland. Personal Email: (diana.hollan...@yahoo.com)
Re: [PATCH v3 1/3] usb: chipidea: imx: support configuring for active low oc signal
Am 04.12.18 um 09:52 schrieb Uwe Kleine-König: > Hello Stefan, > > On Tue, Dec 04, 2018 at 09:40:26AM +0100, Stefan Wahren wrote: >> Am 04.12.18 um 09:31 schrieb Uwe Kleine-König: >>> The status quo on i.MX6 is that if "over-current-active-high" is >>> specified in the device tree this is configured as expected. If >>> the property is missing polarity isn't changed and so the >>> polarity is kept as setup by the bootloader. Reset default is >>> active high, so active low can only be used with help by the >>> bootloader. On i.MX7 it is similar, but there disabling of >>> over current detection has a similar inconsistency. >>> >>> This patch introduces a new property that allows to explicitly >>> configure for active low over current detection and consistently >>> sets this up. In the absence of an explicit configuration the >>> bit is kept as is. On i.MX7 over current detection is used unless >>> disabled in the device tree. >>> >>> Signed-off-by: Uwe Kleine-König >>> --- >>> .../devicetree/bindings/usb/ci-hdrc-usb2.txt | 5 ++-- >>> drivers/usb/chipidea/ci_hdrc_imx.c| 16 --- >>> drivers/usb/chipidea/ci_hdrc_imx.h| 8 +- >>> drivers/usb/chipidea/usbmisc_imx.c| 28 ++- >>> 4 files changed, 43 insertions(+), 14 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt >>> b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt >>> index 529e51879fb2..c32f6e983cf6 100644 >>> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt >>> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt >>> @@ -87,8 +87,9 @@ i.mx specific properties >>> - fsl,usbmisc: phandler of non-core register device, with one >>>argument that indicate usb controller index >>> - disable-over-current: disable over current detect >>> -- over-current-active-high: over current signal polarity is high active, >>> - typically over current signal polarity is low active. >>> +- over-current-active-low: over current signal polarity is active low. >>> +- over-current-active-high: over current signal polarity is active high. >>> + It's recommended to specify the over current polarity. >>> - external-vbus-divider: enables off-chip resistor divider for Vbus >>> >>> Example: >> thanks for making this configurable. But shouldn't this be a separate >> patch and reviewed by the devicetree guys? > I'm not sure about the separate patch part. My impression is it depends > on the maintainer if a change to the binding (opposed to the > introduction of a binding) should be separate or not. > > But for the review you are right, I added the dt people to Cc for them > to comment. In v2 Matthew also noted that he would prefer to handle the > situation when both over-current-active-low and over-current-active-high > were given differently. I think we don't need that as this is a case of > "broken dt" and it doesn't matter much what is done then. (With my patch > we're configuring for active high in that case.) A feedback here would > be great, too. AFAIR such invalid settings should be catched with a proper error message and abort of the probing. Regards Stefan > > Best regards > Uwe >
Re: [PATCH v1 09/12] usb: dwc3: Registering a role switch in the DRD code.
On Mon, Dec 03, 2018 at 11:45:12AM +0800, Yu Chen wrote: > The Type-C drivers use USB role switch API to inform the > system about the negotiated data role, so registering a role > switch in the DRD code in order to support platforms with > USB Type-C connectors. > > Cc: Felipe Balbi > Cc: Greg Kroah-Hartman > Signed-off-by: Yu Chen > Signed-off-by: Heikki Krogerus > > -- > v0: > The patch is provided by Heikki Krogerus. I modified and test it > on Hikey960 platform. > -- > --- > drivers/usb/dwc3/core.h | 2 ++ > drivers/usb/dwc3/drd.c | 49 > + > 2 files changed, 51 insertions(+) You need to select USB_ROLE_SWITCH in Kconfig: diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig index 1a0404fda596..3a0cb9f1f38a 100644 --- a/drivers/usb/dwc3/Kconfig +++ b/drivers/usb/dwc3/Kconfig @@ -42,6 +42,7 @@ config USB_DWC3_DUAL_ROLE bool "Dual Role mode" depends on ((USB=y || USB=USB_DWC3) && (USB_GADGET=y || USB_GADGET=USB_DWC3)) depends on (EXTCON=y || EXTCON=USB_DWC3) + select USB_ROLE_SWITCH help This is the default mode of working of DWC3 controller where both host and gadget features are enabled. -- heikki
Re: [RFC PATCH v2 04/15] usb:cdns3: Driver initialization code.
On 04/12/18 10:50, Peter Chen wrote:
>>> + * Cadence USBSS DRD Driver.
>>> + *
>>> + * Copyright (C) 2018 Cadence.
>>> + *
>>> + * Author: Peter Chen
>>> + * Pawel Laszczak
>>> + */
>>> +
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +
>>> +#include "gadget.h"
>>> +#include "core.h"
>>> +
>>> +static inline struct cdns3_role_driver
>>> *cdns3_get_current_role_driver(struct cdns3 *cdns)
>>> +{
>>> + WARN_ON(cdns->role >= CDNS3_ROLE_END || !cdns->roles[cdns->role]);
>>> + return cdns->roles[cdns->role];
>>> +}
>>> +
>>> +static inline int cdns3_role_start(struct cdns3 *cdns, enum cdns3_roles
>>> role)
>>> +{
>>> + int ret;
>>> +
>>> + if (role >= CDNS3_ROLE_END)
>>
>> WARN_ON()?
>>
>>> + return 0;
>>> +
>>> + if (!cdns->roles[role])
>>> + return -ENXIO;
>>> +
>>> + mutex_lock(&cdns->mutex);
>>> + cdns->role = role;
>>> + ret = cdns->roles[role]->start(cdns);
>>> + mutex_unlock(&cdns->mutex);
>>> + return ret;
>>> +}
>>> +
>>> +static inline void cdns3_role_stop(struct cdns3 *cdns)
>>> +{
>>> + enum cdns3_roles role = cdns->role;
>>> +
>>> + if (role == CDNS3_ROLE_END)
>>
>> WARN_ON(role >= CNDS3_ROLE_END) ?
>>
>>> + return;
>>> +
>>> + mutex_lock(&cdns->mutex);
>>> + cdns->roles[role]->stop(cdns);
>>> + cdns->role = CDNS3_ROLE_END;
>>
>> Why change the role here? You are just stopping the role not changing it.
>> I think cdns->role should remain unchanged, so we can call cdns3_role_start()
>> if required without error.
>>
>
> The current version of this IP has some issues to detect vbus status
> correctly,
> we have to force vbus status accordingly, so we need a status to indicate
> vbus disconnection, and add some code to let controller know vbus
> removal, in that case, the controller's state machine can be correct.
> So, we increase one role 'CDNS3_ROLE_END' to for this purpose.
>
> CDNS3_ROLE_GADGET: gadget mode and VBUS on
> CDNS3_ROLE_HOST: host mode and VBUS on
> CDNS3_ROLE_END: VBUS off, eg either host or device cable on the port.
>
> So, we may start role from CDNS3_ROLE_END at probe when nothing is connected,
> and need to set role as CDNS3_ROLE_END at ->stop for further handling at
> role switch routine.
OK. but still this (changing to ROLE_END) must be moved to the role switch
routine
and the explanation you just mentioned must be added there.
>
>>> + mutex_unlock(&cdns->mutex);
>>> +}
>>> +
>>> +static enum cdns3_roles cdns3_get_role(struct cdns3 *cdns)
>>> +{
>>> + if (cdns->roles[CDNS3_ROLE_HOST] && cdns->roles[CDNS3_ROLE_GADGET]) {
>>> + //TODO: implements selecting device/host mode
>>> + return CDNS3_ROLE_HOST;
>>> + }
>>> + return cdns->roles[CDNS3_ROLE_HOST]
>>> + ? CDNS3_ROLE_HOST
>>> + : CDNS3_ROLE_GADGET;
>>
>> Why not just
>> return cdns->role;
>>
>> I'm wondering if we really need this function.
>
> cdns->role gets from cdns3_get_role, and this API tells role at the runtime.
> If both roles are supported, the role is decided by external
> conditions, eg, vbus/id
> or external connector. If only single role is supported, only one role
> structure
> is allocated, cdns->roles[CDNS3_ROLE_HOST] or cdns->roles[CDNS3_ROLE_GADGET]
>
How about adding this description in function documentation.
>>> +}
>>
>>> +
>>> +/**
>>> + * cdns3_core_init_role - initialize role of operation
>>> + * @cdns: Pointer to cdns3 structure
>>> + *
>>> + * Returns 0 on success otherwise negative errno
>>> + */
>>> +static int cdns3_core_init_role(struct cdns3 *cdns)
>>> +{
>>> + struct device *dev = cdns->dev;
>>> + enum usb_dr_mode dr_mode;
>>> +
>>> + dr_mode = usb_get_dr_mode(dev);
>>> + cdns->role = CDNS3_ROLE_END;
>>> +
>>> + /*
>>> + * If driver can't read mode by means of usb_get_dr_mdoe function then
>>> + * chooses mode according with Kernel configuration. This setting
>>> + * can be restricted later depending on strap pin configuration.
>>> + */
>>> + if (dr_mode == USB_DR_MODE_UNKNOWN) {
>>> + if (IS_ENABLED(CONFIG_USB_CDNS3_HOST) &&
>>> + IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
>>> + dr_mode = USB_DR_MODE_OTG;
>>> + else if (IS_ENABLED(CONFIG_USB_CDNS3_HOST))
>>> + dr_mode = USB_DR_MODE_HOST;
>>> + else if (IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
>>> + dr_mode = USB_DR_MODE_PERIPHERAL;
>>> + }
>>> +
>>> + if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
>>> + //TODO: implements host initialization
>>
>> /* TODO: Add host role */ ?
>>
>>> + }
>>> +
>>> + if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_PERIPHERAL) {
>>> + //TODO: implements device initialization
>>
>> /* TODO: Add device role */ ?
>>
>
> Yes, it needs to al
Re: [RFC PATCH v2 05/15] usb:cdns3: Added DRD support
On Sun, Nov 18, 2018 at 6:13 PM Pawel Laszczak wrote:
>
> Patch adds supports for detecting Host/Device mode.
> Controller has additional OTG register that allow
> implement even whole OTG functionality.
> At this moment patch adds support only for detecting
> the appropriate mode based on strap pins and ID pin.
>
> Signed-off-by: Pawel Laszczak
> ---
> drivers/usb/cdns3/Makefile | 2 +-
> drivers/usb/cdns3/core.c | 27 +++--
> drivers/usb/cdns3/drd.c| 229 +
> drivers/usb/cdns3/drd.h| 122
> 4 files changed, 372 insertions(+), 8 deletions(-)
> create mode 100644 drivers/usb/cdns3/drd.c
> create mode 100644 drivers/usb/cdns3/drd.h
>
> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
> index 02d25b23c5d3..e779b2a2f8eb 100644
> --- a/drivers/usb/cdns3/Makefile
> +++ b/drivers/usb/cdns3/Makefile
> @@ -1,5 +1,5 @@
> obj-$(CONFIG_USB_CDNS3)+= cdns3.o
> obj-$(CONFIG_USB_CDNS3_PCI_WRAP) += cdns3-pci.o
>
> -cdns3-y:= core.o
> +cdns3-y:= core.o drd.o
> cdns3-pci-y:= cdns3-pci-wrap.o
> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> index f9055d4da67f..dbee4325da7f 100644
> --- a/drivers/usb/cdns3/core.c
> +++ b/drivers/usb/cdns3/core.c
> @@ -17,6 +17,7 @@
>
> #include "gadget.h"
> #include "core.h"
> +#include "drd.h"
>
> static inline struct cdns3_role_driver *cdns3_get_current_role_driver(struct
> cdns3 *cdns)
> {
> @@ -57,8 +58,10 @@ static inline void cdns3_role_stop(struct cdns3 *cdns)
> static enum cdns3_roles cdns3_get_role(struct cdns3 *cdns)
> {
> if (cdns->roles[CDNS3_ROLE_HOST] && cdns->roles[CDNS3_ROLE_GADGET]) {
> - //TODO: implements selecting device/host mode
> - return CDNS3_ROLE_HOST;
> + if (cdns3_is_host(cdns))
> + return CDNS3_ROLE_HOST;
> + if (cdns3_is_device(cdns))
> + return CDNS3_ROLE_GADGET;
> }
> return cdns->roles[CDNS3_ROLE_HOST]
> ? CDNS3_ROLE_HOST
> @@ -124,6 +127,12 @@ static irqreturn_t cdns3_irq(int irq, void *data)
> struct cdns3 *cdns = data;
> irqreturn_t ret = IRQ_NONE;
>
> + if (cdns->dr_mode == USB_DR_MODE_OTG) {
> + ret = cdns3_drd_irq(cdns);
> + if (ret == IRQ_HANDLED)
> + return ret;
> + }
> +
> /* Handle device/host interrupt */
> if (cdns->role != CDNS3_ROLE_END)
> ret = cdns3_get_current_role_driver(cdns)->irq(cdns);
> @@ -176,11 +185,8 @@ static void cdns3_role_switch(struct work_struct *work)
>
> cdns = container_of(work, struct cdns3, role_switch_wq);
>
> - //TODO: implements this functions.
> - //host = cdns3_is_host(cdns);
> - //device = cdns3_is_device(cdns);
> - host = 1;
> - device = 0;
> + host = cdns3_is_host(cdns);
> + device = cdns3_is_device(cdns);
>
> if (host)
> role = CDNS3_ROLE_HOST;
> @@ -194,6 +200,12 @@ static void cdns3_role_switch(struct work_struct *work)
> pm_runtime_get_sync(cdns->dev);
> cdns3_role_stop(cdns);
>
> + if (cdns->desired_dr_mode != cdns->current_dr_mode) {
> + cdns3_drd_update_mode(cdns);
> + host = cdns3_is_host(cdns);
> + device = cdns3_is_device(cdns);
> + }
> +
> if (host) {
> if (cdns->roles[CDNS3_ROLE_HOST])
> cdns3_do_role_switch(cdns, CDNS3_ROLE_HOST);
> @@ -287,6 +299,7 @@ static int cdns3_probe(struct platform_device *pdev)
> if (ret)
> goto err2;
>
> + ret = cdns3_drd_init(cdns);
> if (ret)
> goto err2;
>
> diff --git a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c
> new file mode 100644
> index ..ac741c80e776
> --- /dev/null
> +++ b/drivers/usb/cdns3/drd.c
> @@ -0,0 +1,229 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Cadence USBSS DRD Driver.
> + *
> + * Copyright (C) 2018 Cadence.
> + *
> + * Author: Pawel Laszczak + *
> + */
> +#include
> +#include
> +#include
> +#include
> +
> +#include "gadget.h"
> +#include "drd.h"
> +
> +/**
> + * cdns3_set_mode - change mode of OTG Core
> + * @cdns: pointer to context structure
> + * @mode: selected mode from cdns_role
> + */
> +void cdns3_set_mode(struct cdns3 *cdns, enum usb_dr_mode mode)
> +{
> + u32 reg;
> +
> + cdns->current_dr_mode = mode;
> + switch (mode) {
> + case USB_DR_MODE_PERIPHERAL:
> + dev_info(cdns->dev, "Set controller to Gadget mode\n");
> + writel(OTGCMD_DEV_BUS_REQ | OTGCMD_OTG_DIS,
> + &cdns->otg_regs->cmd);
> + break;
> + case USB_DR_MODE_HOST:
> + dev_info(cdns->dev, "Set contro
Re: [RFC PATCH v2 04/15] usb:cdns3: Driver initialization code.
> Hi Roger
>
> >On 18/11/18 12:09, Pawel Laszczak wrote:
> >> Patch adds core.c and core.h file that implements initialization
> >> of platform driver and adds function responsible for selecting,
> >> switching and running appropriate Device/Host mode.
> >>
> >> Signed-off-by: Pawel Laszczak
> >> ---
> >> drivers/usb/cdns3/Makefile | 2 +
> >> drivers/usb/cdns3/core.c | 413 +
> >> drivers/usb/cdns3/core.h | 100 +
> >> 3 files changed, 515 insertions(+)
> >> create mode 100644 drivers/usb/cdns3/core.c
> >> create mode 100644 drivers/usb/cdns3/core.h
> >>
> >> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
> >> index dcdd62003c6a..02d25b23c5d3 100644
> >> --- a/drivers/usb/cdns3/Makefile
> >> +++ b/drivers/usb/cdns3/Makefile
> >> @@ -1,3 +1,5 @@
> >> +obj-$(CONFIG_USB_CDNS3) += cdns3.o
> >> obj-$(CONFIG_USB_CDNS3_PCI_WRAP)+= cdns3-pci.o
> >>
> >> +cdns3-y := core.o
> >> cdns3-pci-y := cdns3-pci-wrap.o
> >> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> >> new file mode 100644
> >> index ..f9055d4da67f
> >> --- /dev/null
> >> +++ b/drivers/usb/cdns3/core.c
> >> @@ -0,0 +1,413 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Cadence USBSS DRD Driver.
> >> + *
> >> + * Copyright (C) 2018 Cadence.
> >> + *
> >> + * Author: Peter Chen
> >> + * Pawel Laszczak
> >> + */
> >> +
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +
> >> +#include "gadget.h"
> >> +#include "core.h"
> >> +
> >> +static inline struct cdns3_role_driver
> >> *cdns3_get_current_role_driver(struct cdns3 *cdns)
> >> +{
> >> +WARN_ON(cdns->role >= CDNS3_ROLE_END || !cdns->roles[cdns->role]);
> >> +return cdns->roles[cdns->role];
> >> +}
> >> +
> >> +static inline int cdns3_role_start(struct cdns3 *cdns, enum cdns3_roles
> >> role)
> >> +{
> >> +int ret;
> >> +
> >> +if (role >= CDNS3_ROLE_END)
> >
> >WARN_ON()?
> I agree.
> >
> >> +return 0;
> >> +
> >> +if (!cdns->roles[role])
> >> +return -ENXIO;
> >> +
> >> +mutex_lock(&cdns->mutex);
> >> +cdns->role = role;
> >> +ret = cdns->roles[role]->start(cdns);
> >> +mutex_unlock(&cdns->mutex);
> >> +return ret;
> >> +}
> >> +
> >> +static inline void cdns3_role_stop(struct cdns3 *cdns)
> >> +{
> >> +enum cdns3_roles role = cdns->role;
> >> +
> >> +if (role == CDNS3_ROLE_END)
> >
> >WARN_ON(role >= CNDS3_ROLE_END) ?
> I agree
> >
> >> +return;
> >> +
> >> +mutex_lock(&cdns->mutex);
> >> +cdns->roles[role]->stop(cdns);
> >> +cdns->role = CDNS3_ROLE_END;
> >
> >Why change the role here? You are just stopping the role not changing it.
> >I think cdns->role should remain unchanged, so we can call cdns3_role_start()
> >if required without error.
>
> This line is unnecessary.
>
> >
> >> +mutex_unlock(&cdns->mutex);
> >> +}
> >> +
> >> +static enum cdns3_roles cdns3_get_role(struct cdns3 *cdns)
> >> +{
> >> +if (cdns->roles[CDNS3_ROLE_HOST] && cdns->roles[CDNS3_ROLE_GADGET]) {
> >> +//TODO: implements selecting device/host mode
> >> +return CDNS3_ROLE_HOST;
> >> +}
> >> +return cdns->roles[CDNS3_ROLE_HOST]
> >> +? CDNS3_ROLE_HOST
> >> +: CDNS3_ROLE_GADGET;
> >
> >Why not just
> > return cdns->role;
> >
> >I'm wondering if we really need this function
>
> TODO will look likie:
> if (cdns3_is_host(cdns))
> return CDNS3_ROLE_HOST;
> if (cdns3_is_device(cdns))
> return CDNS3_ROLE_GADGET;
>
> Function selects initial role. Before invoking it the role is unknown.
> I think that function name should be changed because current name can be
> misleading.
>
> I will change it to cdns3_get_initial_role.
> .
> >> +}
> >
> >> +
> >> +/**
> >> + * cdns3_core_init_role - initialize role of operation
> >> + * @cdns: Pointer to cdns3 structure
> >> + *
> >> + * Returns 0 on success otherwise negative errno
> >> + */
> >> +static int cdns3_core_init_role(struct cdns3 *cdns)
> >> +{
> >> +struct device *dev = cdns->dev;
> >> +enum usb_dr_mode dr_mode;
> >> +
> >> +dr_mode = usb_get_dr_mode(dev);
> >> +cdns->role = CDNS3_ROLE_END;
> >> +
> >> +/*
> >> + * If driver can't read mode by means of usb_get_dr_mdoe function then
> >> + * chooses mode according with Kernel configuration. This setting
> >> + * can be restricted later depending on strap pin configuration.
> >> + */
> >> +if (dr_mode == USB_DR_MODE_UNKNOWN) {
> >> +if (IS_ENABLED(CONFIG_USB_CDNS3_HOST) &&
> >> +IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
> >> +dr_mode = USB_DR_MODE_OTG;
> >> +else if (IS_ENABLED(CONFIG_USB_CDNS3_HOST))
> >> +dr_mode =
Re: [PATCH v3 1/3] usb: chipidea: imx: support configuring for active low oc signal
Hello Stefan, On Tue, Dec 04, 2018 at 09:40:26AM +0100, Stefan Wahren wrote: > Am 04.12.18 um 09:31 schrieb Uwe Kleine-König: > > The status quo on i.MX6 is that if "over-current-active-high" is > > specified in the device tree this is configured as expected. If > > the property is missing polarity isn't changed and so the > > polarity is kept as setup by the bootloader. Reset default is > > active high, so active low can only be used with help by the > > bootloader. On i.MX7 it is similar, but there disabling of > > over current detection has a similar inconsistency. > > > > This patch introduces a new property that allows to explicitly > > configure for active low over current detection and consistently > > sets this up. In the absence of an explicit configuration the > > bit is kept as is. On i.MX7 over current detection is used unless > > disabled in the device tree. > > > > Signed-off-by: Uwe Kleine-König > > --- > > .../devicetree/bindings/usb/ci-hdrc-usb2.txt | 5 ++-- > > drivers/usb/chipidea/ci_hdrc_imx.c| 16 --- > > drivers/usb/chipidea/ci_hdrc_imx.h| 8 +- > > drivers/usb/chipidea/usbmisc_imx.c| 28 ++- > > 4 files changed, 43 insertions(+), 14 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > > b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > > index 529e51879fb2..c32f6e983cf6 100644 > > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > > @@ -87,8 +87,9 @@ i.mx specific properties > > - fsl,usbmisc: phandler of non-core register device, with one > >argument that indicate usb controller index > > - disable-over-current: disable over current detect > > -- over-current-active-high: over current signal polarity is high active, > > - typically over current signal polarity is low active. > > +- over-current-active-low: over current signal polarity is active low. > > +- over-current-active-high: over current signal polarity is active high. > > + It's recommended to specify the over current polarity. > > - external-vbus-divider: enables off-chip resistor divider for Vbus > > > > Example: > > thanks for making this configurable. But shouldn't this be a separate > patch and reviewed by the devicetree guys? I'm not sure about the separate patch part. My impression is it depends on the maintainer if a change to the binding (opposed to the introduction of a binding) should be separate or not. But for the review you are right, I added the dt people to Cc for them to comment. In v2 Matthew also noted that he would prefer to handle the situation when both over-current-active-low and over-current-active-high were given differently. I think we don't need that as this is a case of "broken dt" and it doesn't matter much what is done then. (With my patch we're configuring for active high in that case.) A feedback here would be great, too. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: [RFC PATCH v2 04/15] usb:cdns3: Driver initialization code.
> > + * Cadence USBSS DRD Driver.
> > + *
> > + * Copyright (C) 2018 Cadence.
> > + *
> > + * Author: Peter Chen
> > + * Pawel Laszczak
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include "gadget.h"
> > +#include "core.h"
> > +
> > +static inline struct cdns3_role_driver
> > *cdns3_get_current_role_driver(struct cdns3 *cdns)
> > +{
> > + WARN_ON(cdns->role >= CDNS3_ROLE_END || !cdns->roles[cdns->role]);
> > + return cdns->roles[cdns->role];
> > +}
> > +
> > +static inline int cdns3_role_start(struct cdns3 *cdns, enum cdns3_roles
> > role)
> > +{
> > + int ret;
> > +
> > + if (role >= CDNS3_ROLE_END)
>
> WARN_ON()?
>
> > + return 0;
> > +
> > + if (!cdns->roles[role])
> > + return -ENXIO;
> > +
> > + mutex_lock(&cdns->mutex);
> > + cdns->role = role;
> > + ret = cdns->roles[role]->start(cdns);
> > + mutex_unlock(&cdns->mutex);
> > + return ret;
> > +}
> > +
> > +static inline void cdns3_role_stop(struct cdns3 *cdns)
> > +{
> > + enum cdns3_roles role = cdns->role;
> > +
> > + if (role == CDNS3_ROLE_END)
>
> WARN_ON(role >= CNDS3_ROLE_END) ?
>
> > + return;
> > +
> > + mutex_lock(&cdns->mutex);
> > + cdns->roles[role]->stop(cdns);
> > + cdns->role = CDNS3_ROLE_END;
>
> Why change the role here? You are just stopping the role not changing it.
> I think cdns->role should remain unchanged, so we can call cdns3_role_start()
> if required without error.
>
The current version of this IP has some issues to detect vbus status correctly,
we have to force vbus status accordingly, so we need a status to indicate
vbus disconnection, and add some code to let controller know vbus
removal, in that case, the controller's state machine can be correct.
So, we increase one role 'CDNS3_ROLE_END' to for this purpose.
CDNS3_ROLE_GADGET: gadget mode and VBUS on
CDNS3_ROLE_HOST: host mode and VBUS on
CDNS3_ROLE_END: VBUS off, eg either host or device cable on the port.
So, we may start role from CDNS3_ROLE_END at probe when nothing is connected,
and need to set role as CDNS3_ROLE_END at ->stop for further handling at
role switch routine.
> > + mutex_unlock(&cdns->mutex);
> > +}
> > +
> > +static enum cdns3_roles cdns3_get_role(struct cdns3 *cdns)
> > +{
> > + if (cdns->roles[CDNS3_ROLE_HOST] && cdns->roles[CDNS3_ROLE_GADGET]) {
> > + //TODO: implements selecting device/host mode
> > + return CDNS3_ROLE_HOST;
> > + }
> > + return cdns->roles[CDNS3_ROLE_HOST]
> > + ? CDNS3_ROLE_HOST
> > + : CDNS3_ROLE_GADGET;
>
> Why not just
> return cdns->role;
>
> I'm wondering if we really need this function.
cdns->role gets from cdns3_get_role, and this API tells role at the runtime.
If both roles are supported, the role is decided by external
conditions, eg, vbus/id
or external connector. If only single role is supported, only one role structure
is allocated, cdns->roles[CDNS3_ROLE_HOST] or cdns->roles[CDNS3_ROLE_GADGET]
> > +}
>
> > +
> > +/**
> > + * cdns3_core_init_role - initialize role of operation
> > + * @cdns: Pointer to cdns3 structure
> > + *
> > + * Returns 0 on success otherwise negative errno
> > + */
> > +static int cdns3_core_init_role(struct cdns3 *cdns)
> > +{
> > + struct device *dev = cdns->dev;
> > + enum usb_dr_mode dr_mode;
> > +
> > + dr_mode = usb_get_dr_mode(dev);
> > + cdns->role = CDNS3_ROLE_END;
> > +
> > + /*
> > + * If driver can't read mode by means of usb_get_dr_mdoe function then
> > + * chooses mode according with Kernel configuration. This setting
> > + * can be restricted later depending on strap pin configuration.
> > + */
> > + if (dr_mode == USB_DR_MODE_UNKNOWN) {
> > + if (IS_ENABLED(CONFIG_USB_CDNS3_HOST) &&
> > + IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
> > + dr_mode = USB_DR_MODE_OTG;
> > + else if (IS_ENABLED(CONFIG_USB_CDNS3_HOST))
> > + dr_mode = USB_DR_MODE_HOST;
> > + else if (IS_ENABLED(CONFIG_USB_CDNS3_GADGET))
> > + dr_mode = USB_DR_MODE_PERIPHERAL;
> > + }
> > +
> > + if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
> > + //TODO: implements host initialization
>
> /* TODO: Add host role */ ?
>
> > + }
> > +
> > + if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_PERIPHERAL) {
> > + //TODO: implements device initialization
>
> /* TODO: Add device role */ ?
>
Yes, it needs to allocate cdns->roles[CDNS3_ROLE_HOST] and
cdns->roles[CDNS3_ROLE_GADGET].
> > + }
> > +
> > + if (!cdns->roles[CDNS3_ROLE_HOST] && !cdns->roles[CDNS3_ROLE_GADGET])
> > {
> > + dev_err(dev, "no supported roles\n");
> > + return -ENODEV;
> > + }
> > +
> > + cdns->dr_mode = dr_mode;
Pawe
Re: [PATCH v3 1/3] usb: chipidea: imx: support configuring for active low oc signal
Hi Uwe, Am 04.12.18 um 09:31 schrieb Uwe Kleine-König: > The status quo on i.MX6 is that if "over-current-active-high" is > specified in the device tree this is configured as expected. If > the property is missing polarity isn't changed and so the > polarity is kept as setup by the bootloader. Reset default is > active high, so active low can only be used with help by the > bootloader. On i.MX7 it is similar, but there disabling of > over current detection has a similar inconsistency. > > This patch introduces a new property that allows to explicitly > configure for active low over current detection and consistently > sets this up. In the absence of an explicit configuration the > bit is kept as is. On i.MX7 over current detection is used unless > disabled in the device tree. > > Signed-off-by: Uwe Kleine-König > --- > .../devicetree/bindings/usb/ci-hdrc-usb2.txt | 5 ++-- > drivers/usb/chipidea/ci_hdrc_imx.c| 16 --- > drivers/usb/chipidea/ci_hdrc_imx.h| 8 +- > drivers/usb/chipidea/usbmisc_imx.c| 28 ++- > 4 files changed, 43 insertions(+), 14 deletions(-) > > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > index 529e51879fb2..c32f6e983cf6 100644 > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt > @@ -87,8 +87,9 @@ i.mx specific properties > - fsl,usbmisc: phandler of non-core register device, with one >argument that indicate usb controller index > - disable-over-current: disable over current detect > -- over-current-active-high: over current signal polarity is high active, > - typically over current signal polarity is low active. > +- over-current-active-low: over current signal polarity is active low. > +- over-current-active-high: over current signal polarity is active high. > + It's recommended to specify the over current polarity. > - external-vbus-divider: enables off-chip resistor divider for Vbus > > Example: thanks for making this configurable. But shouldn't this be a separate patch and reviewed by the devicetree guys?
[PATCH v3 2/3] usb: chipidea: imx: Warn if oc polarity isn't specified
The polarity of the over current detection pin isn't configured on i.MX6/7
if it's unspecified in the device tree. So the actual configuration depends
on bootloader behavior which is bad.
So encourage users to fix their device tree by issuing a warning in this
case.
Signed-off-by: Uwe Kleine-König
---
drivers/usb/chipidea/ci_hdrc_imx.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
b/drivers/usb/chipidea/ci_hdrc_imx.c
index 18b0ca87799c..1e7182272fb5 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -144,6 +144,8 @@ static struct imx_usbmisc_data
*usbmisc_get_init_data(struct device *dev)
} else if (of_find_property(np, "over-current-active-low", NULL)) {
data->oc_pol_active_low = 1;
data->oc_pol_configured = 1;
+ } else {
+ dev_warn(dev, "No over current polarity defined\n");
}
if (of_find_property(np, "external-vbus-divider", NULL))
--
2.19.2
[PATCH v3 3/3] usb: chipidea: imx: allow to configure oc polarity on i.MX25
Up to now the polarity of the over current pin was hard coded to active high. Use the already defined device tree properties to configure polarity on i.MX25, too. In difference to i.MX6/7 use active high behavior if the polarity is unspecified to keep compatibility to existing device trees. Signed-off-by: Uwe Kleine-König --- drivers/usb/chipidea/usbmisc_imx.c | 15 +++ 1 file changed, 15 insertions(+) diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c index 4a8de1b37f67..47eee5cade84 100644 --- a/drivers/usb/chipidea/usbmisc_imx.c +++ b/drivers/usb/chipidea/usbmisc_imx.c @@ -120,6 +120,14 @@ static int usbmisc_imx25_init(struct imx_usbmisc_data *data) val &= ~(MX25_OTG_SIC_MASK | MX25_OTG_PP_BIT); val |= (MX25_EHCI_INTERFACE_DIFF_UNI & MX25_EHCI_INTERFACE_MASK) << MX25_OTG_SIC_SHIFT; val |= (MX25_OTG_PM_BIT | MX25_OTG_OCPOL_BIT); + + /* +* If the polarity is not configured assume active high for +* historical reasons. +*/ + if (data->oc_pol_configured && data->oc_pol_active_low) + val &= ~MX25_OTG_OCPOL_BIT; + writel(val, usbmisc->base); break; case 1: @@ -129,6 +137,13 @@ static int usbmisc_imx25_init(struct imx_usbmisc_data *data) val |= (MX25_H1_PM_BIT | MX25_H1_OCPOL_BIT | MX25_H1_TLL_BIT | MX25_H1_USBTE_BIT | MX25_H1_IPPUE_DOWN_BIT); + /* +* If the polarity is not configured assume active high for +* historical reasons. +*/ + if (data->oc_pol_configured && data->oc_pol_active_low) + val &= ~MX25_H1_OCPOL_BIT; + writel(val, usbmisc->base); break; -- 2.19.2
[PATCH v3 1/3] usb: chipidea: imx: support configuring for active low oc signal
The status quo on i.MX6 is that if "over-current-active-high" is
specified in the device tree this is configured as expected. If
the property is missing polarity isn't changed and so the
polarity is kept as setup by the bootloader. Reset default is
active high, so active low can only be used with help by the
bootloader. On i.MX7 it is similar, but there disabling of
over current detection has a similar inconsistency.
This patch introduces a new property that allows to explicitly
configure for active low over current detection and consistently
sets this up. In the absence of an explicit configuration the
bit is kept as is. On i.MX7 over current detection is used unless
disabled in the device tree.
Signed-off-by: Uwe Kleine-König
---
.../devicetree/bindings/usb/ci-hdrc-usb2.txt | 5 ++--
drivers/usb/chipidea/ci_hdrc_imx.c| 16 ---
drivers/usb/chipidea/ci_hdrc_imx.h| 8 +-
drivers/usb/chipidea/usbmisc_imx.c| 28 ++-
4 files changed, 43 insertions(+), 14 deletions(-)
diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
index 529e51879fb2..c32f6e983cf6 100644
--- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
+++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
@@ -87,8 +87,9 @@ i.mx specific properties
- fsl,usbmisc: phandler of non-core register device, with one
argument that indicate usb controller index
- disable-over-current: disable over current detect
-- over-current-active-high: over current signal polarity is high active,
- typically over current signal polarity is low active.
+- over-current-active-low: over current signal polarity is active low.
+- over-current-active-high: over current signal polarity is active high.
+ It's recommended to specify the over current polarity.
- external-vbus-divider: enables off-chip resistor divider for Vbus
Example:
diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
b/drivers/usb/chipidea/ci_hdrc_imx.c
index 09b37c0d075d..18b0ca87799c 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -132,11 +132,19 @@ static struct imx_usbmisc_data
*usbmisc_get_init_data(struct device *dev)
data->dev = &misc_pdev->dev;
- if (of_find_property(np, "disable-over-current", NULL))
+ /*
+* Check the various over current related properties. If over current
+* detection is disabled we're not interested in the polarity.
+*/
+ if (of_find_property(np, "disable-over-current", NULL)) {
data->disable_oc = 1;
-
- if (of_find_property(np, "over-current-active-high", NULL))
- data->oc_polarity = 1;
+ } else if (of_find_property(np, "over-current-active-high", NULL)) {
+ data->oc_pol_active_low = 0;
+ data->oc_pol_configured = 1;
+ } else if (of_find_property(np, "over-current-active-low", NULL)) {
+ data->oc_pol_active_low = 1;
+ data->oc_pol_configured = 1;
+ }
if (of_find_property(np, "external-vbus-divider", NULL))
data->evdo = 1;
diff --git a/drivers/usb/chipidea/ci_hdrc_imx.h
b/drivers/usb/chipidea/ci_hdrc_imx.h
index 204275f47573..640721fef0d7 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.h
+++ b/drivers/usb/chipidea/ci_hdrc_imx.h
@@ -11,7 +11,13 @@ struct imx_usbmisc_data {
int index;
unsigned int disable_oc:1; /* over current detect disabled */
- unsigned int oc_polarity:1; /* over current polarity if oc enabled */
+
+ /* true if over-current polarity is active low */
+ unsigned int oc_pol_active_low:1;
+
+ /* true if dt specifies polarity */
+ unsigned int oc_pol_configured:1;
+
unsigned int evdo:1; /* set external vbus divider option */
unsigned int ulpi:1; /* connected to an ULPI phy */
};
diff --git a/drivers/usb/chipidea/usbmisc_imx.c
b/drivers/usb/chipidea/usbmisc_imx.c
index def80ff547e4..4a8de1b37f67 100644
--- a/drivers/usb/chipidea/usbmisc_imx.c
+++ b/drivers/usb/chipidea/usbmisc_imx.c
@@ -340,11 +340,17 @@ static int usbmisc_imx6q_init(struct imx_usbmisc_data
*data)
reg = readl(usbmisc->base + data->index * 4);
if (data->disable_oc) {
reg |= MX6_BM_OVER_CUR_DIS;
- } else if (data->oc_polarity == 1) {
- /* High active */
- reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
} else {
- reg &= ~(MX6_BM_OVER_CUR_DIS);
+ reg &= ~MX6_BM_OVER_CUR_DIS;
+
+ /*
+* If the polarity is not configured keep it as setup by the
+* bootloader.
+*/
+ if (data->oc_pol_configured && data->oc_pol_active_low)
+ reg |= MX6_BM_OVER_CUR_POLARITY;
+ else if (data->oc_pol_configured)
+ reg &= ~MX6_BM_OVER_
[PATCH v3 0/3] usb: chipidea: imx: improve oc handling
Hello, compared to v2 (which starts with Message-Id: <20181202213325.26017-1-u.kleine-koe...@pengutronix.de>) I adapted patch 1 that the oc polarity is only determined if oc isn't disabled. This results in the warning that is introduced in patch 2 not to be emitted in this case. Best regards Uwe
Re: [PATCH v2 2/3] usb: chipidea: imx: Warn if oc polarity isn't specified
Hello Peter,
On Tue, Dec 04, 2018 at 06:19:11AM +, PETER CHEN wrote:
> > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
> > b/drivers/usb/chipidea/ci_hdrc_imx.c
> > index 80b4e4ef9b68..3dcfd0d97f94 100644
> > --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> > @@ -141,6 +141,8 @@ static struct imx_usbmisc_data
> > *usbmisc_get_init_data(struct device *dev)
> > } else if (of_find_property(np, "over-current-active-low", NULL)) {
> > data->oc_pol_active_low = 1;
> > data->oc_pol_configured = 1;
> > + } else {
> > + dev_warn(dev, "No over current polarity defined\n");
> > }
>
> If the platform doesn't support OC, the polarity setting is not needed. You
> could consider
> getting polarity property only "disable-over-current" is not found at patch 1.
Right, just fixed that up and will send out v3 in a moment.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
