RE: [PATCH] usb: gadget: storage: Remove warning message
Thanks for the info. Will remove it in the V2. Regards, EJ -Original Message- From: Greg KH Sent: Wednesday, May 8, 2019 2:40 PM To: EJ Hsu Cc: ba...@kernel.org; linux-usb@vger.kernel.org Subject: Re: [PATCH] usb: gadget: storage: Remove warning message On Wed, May 08, 2019 at 11:24:00AM +0800, EJ Hsu wrote: > --- a/drivers/usb/gadget/function/storage_common.h > +++ b/drivers/usb/gadget/function/storage_common.h > @@ -1,4 +1,7 @@ > /* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (c) 2019, NVIDIA CORPORATION. All rights reserved. > + */ Same here, why are you claiming copyright for this whole file? That is just flat out not acceptable at all, again, go talk to your lawyers. If you insist on this, I will insist on having one of your lawyers on the signed-off-by: line as well so that they know what they are agreeing with. not ok. greg k-h --- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. ---
[V2] usb: gadget: storage: Remove warning message
This change is to fix below warning message in following scenario:
usb_composite_setup_continue: Unexpected call
When system tried to enter suspend, the fsg_disable() will be called to
disable fsg driver and send a signal to fsg_main_thread. However, at
this point, the fsg_main_thread has already been frozen and can not
respond to this signal. So, this signal will be pended until
fsg_main_thread wakes up.
Once system resumes from suspend, fsg_main_thread will detect a signal
pended and do some corresponding action (in handle_exception()). Then,
host will send some setup requests (get descriptor, set configuration...)
to UDC driver trying to enumerate this device. During the handling of "set
configuration" request, it will try to sync up with fsg_main_thread by
sending a signal (which is the same as the signal sent by fsg_disable)
to it. In a similar manner, once the fsg_main_thread receives this
signal, it will call handle_exception() to handle the request.
However, if the fsg_main_thread wakes up from suspend a little late and
"set configuration" request from Host arrives a little earlier,
fsg_main_thread might come across the request from "set configuration"
when it handles the signal from fsg_disable(). In this case, it will
handle this request as well. So, when fsg_main_thread tries to handle
the signal sent from "set configuration" later, there will nothing left
to do and warning message "Unexpected call" is printed.
Signed-off-by: EJ Hsu
---
v2: remove the copyright info
---
drivers/usb/gadget/function/f_mass_storage.c | 17 ++---
drivers/usb/gadget/function/storage_common.h | 1 +
2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/gadget/function/f_mass_storage.c
b/drivers/usb/gadget/function/f_mass_storage.c
index 043f97a..ba1d827 100644
--- a/drivers/usb/gadget/function/f_mass_storage.c
+++ b/drivers/usb/gadget/function/f_mass_storage.c
@@ -2294,7 +2294,7 @@ static void fsg_disable(struct usb_function *f)
{
struct fsg_dev *fsg = fsg_from_func(f);
fsg->common->new_fsg = NULL;
- raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE);
+ raise_exception(fsg->common, FSG_STATE_DISCONNECT);
}
@@ -2307,6 +2307,7 @@ static void handle_exception(struct fsg_common *common)
enum fsg_state old_state;
struct fsg_lun *curlun;
unsigned intexception_req_tag;
+ struct fsg_dev *fsg;
/*
* Clear the existing signals. Anything but SIGUSR1 is converted
@@ -2413,9 +2414,19 @@ static void handle_exception(struct fsg_common *common)
break;
case FSG_STATE_CONFIG_CHANGE:
- do_set_interface(common, common->new_fsg);
- if (common->new_fsg)
+ fsg = common->new_fsg;
+ /*
+* Add a check here to double confirm if a disconnect event
+* occurs and common->new_fsg has been cleared.
+*/
+ if (fsg) {
+ do_set_interface(common, fsg);
usb_composite_setup_continue(common->cdev);
+ }
+ break;
+
+ case FSG_STATE_DISCONNECT:
+ do_set_interface(common, NULL);
break;
case FSG_STATE_EXIT:
diff --git a/drivers/usb/gadget/function/storage_common.h
b/drivers/usb/gadget/function/storage_common.h
index e5e3a25..12687f7 100644
--- a/drivers/usb/gadget/function/storage_common.h
+++ b/drivers/usb/gadget/function/storage_common.h
@@ -161,6 +161,7 @@ enum fsg_state {
FSG_STATE_ABORT_BULK_OUT,
FSG_STATE_PROTOCOL_RESET,
FSG_STATE_CONFIG_CHANGE,
+ FSG_STATE_DISCONNECT,
FSG_STATE_EXIT,
FSG_STATE_TERMINATED
};
--
2.1.4
Re: [PATCH V2 2/8] phy: tegra: xusb: t210: add usb3 port fake support
On 25-04-2019 20:25, Thierry Reding wrote:
> On Mon, Mar 11, 2019 at 04:41:50PM +0530, Nagarjuna Kristam wrote:
>> On Tegra210, usb2 only otg/peripheral ports dont work in device mode.
>> They need an assosciated usb3 port to work in device mode. Identify
>> an unused usb3 port and assign it as a fake USB3 port to USB2 only
>> port whose mode is otg/peripheral
>>
>> Based on work by BH Hsieh .
>>
>> Signed-off-by: Nagarjuna Kristam
>> ---
>> drivers/phy/tegra/xusb-tegra210.c | 40
>> +++
>> drivers/phy/tegra/xusb.c | 29
>> drivers/phy/tegra/xusb.h | 6 ++
>> 3 files changed, 75 insertions(+)
>
> I think this looks a whole lot better than the initial version. One or
> two minor comments below.
>
>> diff --git a/drivers/phy/tegra/xusb-tegra210.c
>> b/drivers/phy/tegra/xusb-tegra210.c
>> index 4beebcc..48478bc4 100644
>> --- a/drivers/phy/tegra/xusb-tegra210.c
>> +++ b/drivers/phy/tegra/xusb-tegra210.c
>> @@ -58,6 +58,7 @@
>> #define XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_SHIFT(x) ((x) * 5)
>> #define XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_MASK(x) (0x7 << ((x) * 5))
>> #define XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP(x, v) (((v) & 0x7) << ((x) * 5))
>> +#define XUSB_PADCTL_SS_PORT_MAP_PORT_DISABLED 0x7
>>
>> #define XUSB_PADCTL_ELPG_PROGRAM1 0x024
>> #define XUSB_PADCTL_ELPG_PROGRAM1_AUX_MUX_LP0_VCORE_DOWN (1 << 31)
>> @@ -952,6 +953,15 @@ static int tegra210_usb2_phy_power_on(struct phy *phy)
>>
>> priv = to_tegra210_xusb_padctl(padctl);
>>
>> +if (port->usb3_port_fake != -1) {
>> +value = padctl_readl(padctl, XUSB_PADCTL_SS_PORT_MAP);
>> +value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_MASK(
>> +port->usb3_port_fake);
>> +value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP(
>> +port->usb3_port_fake, index);
>> +padctl_writel(padctl, value, XUSB_PADCTL_SS_PORT_MAP);
>> +}
>> +
>> value = padctl_readl(padctl, XUSB_PADCTL_USB2_BIAS_PAD_CTL0);
>> value &= ~((XUSB_PADCTL_USB2_BIAS_PAD_CTL0_HS_SQUELCH_LEVEL_MASK <<
>> XUSB_PADCTL_USB2_BIAS_PAD_CTL0_HS_SQUELCH_LEVEL_SHIFT) |
>> @@ -1086,6 +1096,15 @@ static int tegra210_usb2_phy_power_off(struct phy
>> *phy)
>>
>> mutex_lock(&padctl->lock);
>>
>> +if (port->usb3_port_fake != -1) {
>> +value = padctl_readl(padctl, XUSB_PADCTL_SS_PORT_MAP);
>> +value &= ~XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP_MASK(
>> +port->usb3_port_fake);
>> +value |= XUSB_PADCTL_SS_PORT_MAP_PORTX_MAP(port->usb3_port_fake,
>> +XUSB_PADCTL_SS_PORT_MAP_PORT_DISABLED);
>> +padctl_writel(padctl, value, XUSB_PADCTL_SS_PORT_MAP);
>> +}
>> +
>> if (WARN_ON(pad->enable == 0))
>> goto out;
>>
>> @@ -1784,6 +1803,27 @@ static const struct tegra_xusb_pad_soc * const
>> tegra210_pads[] = {
>>
>> static int tegra210_usb2_port_enable(struct tegra_xusb_port *port)
>> {
>> +struct tegra_xusb_usb2_port *usb2 = to_usb2_port(port);
>> +struct tegra_xusb_padctl *padctl = port->padctl;
>> +
>> +/* Disable usb3_port_fake usage by default and assign if needed */
>> +usb2->usb3_port_fake = -1;
>> +if (usb2 && (usb2->mode == USB_DR_MODE_OTG ||
>> +usb2->mode == USB_DR_MODE_PERIPHERAL)) {
>> +if (!tegra_xusb_usb3_port_has_companion(padctl, port->index)) {
>> +int fake = tegra_xusb_find_unused_usb3_port(padctl);
>> +
>> +if (fake >= 0) {
>> +dev_dbg(&port->dev, "Found unused usb3 port:
>> %d\n",
>> + fake);
>> +usb2->usb3_port_fake = fake;
>> +} else {
>> +dev_err(&port->dev, "usb2-%u has neither a
>> companion nor an unused usb3 port to fake it\n",
>> +port->index);
>> +return -ENODEV;
>> +}
>> +}
>> +}
>
> Generally it's preferable to check for errors and deal with success in
> the regular code path. So the above would be:
>
> if (fake < 0) {
> dev_err(...);
> return -ENODEV;
> }
>
> dev_dbg(...);
> ...
>
> Also, the error message is slightly redundant because &port->dev will
> already output the usb2-%u part. Also, perhaps try to make that message
> a little shorter. Something like:
>
> dev_err(&port->dev, "no unused USB3 ports available\n");
>
Will update accordingly
> Also, I don't think this is the right place to do this. There's really
> no way for the ->enable() callbacks to fail. Or at least failures will
> be ignored, so there's no chance fo
Re: [PATCH V2 3/8] phy: tegra: xusb: t210: add vbus override support
On 25-04-2019 20:34, Thierry Reding wrote:
> On Mon, Mar 11, 2019 at 04:41:51PM +0530, Nagarjuna Kristam wrote:
>> Tegra XUSB device control driver needs to control vbus override
>> during its operations, add API for the support
>>
>> Signed-off-by: Nagarjuna Kristam
>> ---
>> drivers/phy/tegra/xusb-tegra210.c | 61
>> +++
>> drivers/phy/tegra/xusb.c | 28 --
>> drivers/phy/tegra/xusb.h | 2 ++
>> include/linux/phy/tegra/xusb.h| 6 ++--
>> 4 files changed, 92 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/phy/tegra/xusb-tegra210.c
>> b/drivers/phy/tegra/xusb-tegra210.c
>> index 48478bc4..be1a870 100644
>> --- a/drivers/phy/tegra/xusb-tegra210.c
>> +++ b/drivers/phy/tegra/xusb-tegra210.c
>> @@ -73,6 +73,10 @@
>> #define XUSB_PADCTL_USB3_PAD_MUX_PCIE_IDDQ_DISABLE(x) (1 << (1 + (x)))
>> #define XUSB_PADCTL_USB3_PAD_MUX_SATA_IDDQ_DISABLE(x) (1 << (8 + (x)))
>>
>> +#define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPADX_CTL0(x) (0x080 + (x) * 0x40)
>> +#define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL0_ZIP (1 << 18)
>> +#define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL0_ZIN (1 << 22)
>> +
>> #define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPADX_CTL1(x) (0x084 + (x) * 0x40)
>> #define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV_SHIFT 7
>> #define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV_MASK 0x3
>> @@ -235,6 +239,12 @@
>> #define XUSB_PADCTL_UPHY_USB3_PADX_ECTL6(x) (0xa74 + (x) * 0x40)
>> #define XUSB_PADCTL_UPHY_USB3_PAD_ECTL6_RX_EQ_CTRL_H_VAL 0xfcf01368
>>
>> +#define XUSB_PADCTL_USB2_VBUS_ID 0xc60
>> +#define XUSB_PADCTL_USB2_VBUS_ID_OVERRIDE_VBUS_ON (1 << 14)
>> +#define XUSB_PADCTL_USB2_VBUS_ID_OVERRIDE_SHIFT 18
>> +#define XUSB_PADCTL_USB2_VBUS_ID_OVERRIDE_MASK 0xf
>> +#define XUSB_PADCTL_USB2_VBUS_ID_OVERRIDE_FLOATING 8
>> +
>> struct tegra210_xusb_fuse_calibration {
>> u32 hs_curr_level[4];
>> u32 hs_term_range_adj;
>> @@ -2009,6 +2019,55 @@ static const struct tegra_xusb_port_ops
>> tegra210_usb3_port_ops = {
>> .map = tegra210_usb3_port_map,
>> };
>>
>> +static int tegra210_xusb_padctl_vbus_override(struct tegra_xusb_padctl
>> *padctl,
>> + bool set)
>
> I think "status" would perhaps be somewhat more meaningful.
>
Will update accordingly
>> +{
>> +u32 reg;
>
> The rest of the driver uses "u32 value". It'd be good to be consistent.
>
Will sync with rest of code as suggested.
>> +
>> +dev_dbg(padctl->dev, "%s vbus override\n", set ? "set" : "clear");
>> +
>> +reg = padctl_readl(padctl, XUSB_PADCTL_USB2_VBUS_ID);
>> +if (set) {
>> +reg |= XUSB_PADCTL_USB2_VBUS_ID_OVERRIDE_VBUS_ON;
>> +reg &= ~(XUSB_PADCTL_USB2_VBUS_ID_OVERRIDE_MASK <<
>> + XUSB_PADCTL_USB2_VBUS_ID_OVERRIDE_SHIFT);
>> +reg |= XUSB_PADCTL_USB2_VBUS_ID_OVERRIDE_FLOATING <<
>> + XUSB_PADCTL_USB2_VBUS_ID_OVERRIDE_SHIFT;
>> +} else
>> +reg &= ~XUSB_PADCTL_USB2_VBUS_ID_OVERRIDE_VBUS_ON;
>
> This could use some blank lines to separate blocks and make it more
> readable.
>
will update accordingly
>> +padctl_writel(padctl, reg, XUSB_PADCTL_USB2_VBUS_ID);
>> +
>> +return 0;
>> +}
>> +
>> +static int tegra210_utmi_port_reset(struct phy *phy)
>> +{
>> +struct tegra_xusb_padctl *padctl;
>> +struct tegra_xusb_lane *lane;
>> +struct device *dev;
>> +u32 reg;
>
> u32 value
>
will do
>> +
>> +if (!phy)
>> +return -ENODEV;
>
> When would this happen?
>
Added this as safety check, if caller class with null.
But in current code flow this won't happen, it can be removed.
>> +
>> +lane = phy_get_drvdata(phy);
>> +padctl = lane->pad->padctl;
>> +dev = padctl->dev;
>> +
>> +reg = padctl_readl(padctl,
>> +XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPADX_CTL0(0));
>
> Usually subsequent lines are indented so that they align with the first
> argument of the first line.
>
will do
>> +dev_dbg(dev, "BATTERY_CHRG_OTGPADX_CTL0(0): 0x%x\n", reg);
>
> You can use %#x to avoid having to explicitly provide the 0x prefix.
> Also, is this really useful for debugging? We could add trace support to
> this driver (to padctl_readl() and padctl_writel() for example) to allow
> for more flexible tracing of register programming sequences.
>
will remove the debug log
>> +
>> +if ((reg & XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL0_ZIP) ||
>> +(reg & XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL0_ZIN)) {
>> +dev_dbg(dev, "Toggle vbus\n");
>
> This one is pretty redundant because the function calls below each
> already output something to that effect.
>
will remove
>> +tegra210_xusb_padctl_vbus_override(padctl, false);
>> +tegra210_xusb_padctl_vbus_override(padctl, true);
>> +return 1;
>> +}
>> +return 0;
>> +}
>> +
>> static int
>> tegra210_xusb_read_fuse_calibra
Re: [PATCH] usb: dwc3: move core validation to be after clks enable
Hi Jun Li,
On 5/8/2019 9:54 AM, Jun Li wrote:
From: Jun Li
Register access in core validation may hang before the bulk
clks are enabled.
Fixes: b873e2d0ea1e ("usb: dwc3: Do core validation early on probe")
Signed-off-by: Jun Li
---
drivers/usb/dwc3/core.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 4aff1d8..0e49ff3 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1423,11 +1423,6 @@ static int dwc3_probe(struct platform_device *pdev)
dwc->regs= regs;
dwc->regs_size = resource_size(&dwc_res);
- if (!dwc3_core_is_valid(dwc)) {
- dev_err(dwc->dev, "this is not a DesignWare USB3 DRD Core\n");
- return -ENODEV;
- }
-
dwc3_get_properties(dwc);
dwc->reset = devm_reset_control_get_optional_shared(dev, NULL);
@@ -1460,6 +1455,11 @@ static int dwc3_probe(struct platform_device *pdev)
if (ret)
goto unprepare_clks;
+ if (!dwc3_core_is_valid(dwc)) {
+ dev_err(dwc->dev, "this is not a DesignWare USB3 DRD Core\n");
Please update "ret" here with -ENODEV, else the probe call will return
success (ret is 0).
+ goto disable_clks;
+ }
+
platform_set_drvdata(pdev, dwc);
dwc3_cache_hwparams(dwc);
@@ -1524,7 +1524,7 @@ static int dwc3_probe(struct platform_device *pdev)
err1:
pm_runtime_put_sync(&pdev->dev);
pm_runtime_disable(&pdev->dev);
-
+disable_clks:
clk_bulk_disable(dwc->num_clks, dwc->clks);
unprepare_clks:
clk_bulk_unprepare(dwc->num_clks, dwc->clks);
Regards,
Sriharsha
RE: [PATCH] usb: dwc3: move core validation to be after clks enable
Hi Sriharsha,
> -Original Message-
> From: Sriharsha Allenki
> Sent: 2019年5月8日 18:26
> To: Jun Li ; ba...@kernel.org; gre...@linuxfoundation.org
> Cc: thi...@synopsys.com; linux-usb@vger.kernel.org; dl-linux-imx
>
> Subject: Re: [PATCH] usb: dwc3: move core validation to be after clks enable
>
> Hi Jun Li,
>
> On 5/8/2019 9:54 AM, Jun Li wrote:
> > From: Jun Li
> >
> > Register access in core validation may hang before the bulk clks are
> > enabled.
> >
> > Fixes: b873e2d0ea1e ("usb: dwc3: Do core validation early on probe")
> > Signed-off-by: Jun Li
> > ---
> > drivers/usb/dwc3/core.c | 12 ++--
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index
> > 4aff1d8..0e49ff3 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -1423,11 +1423,6 @@ static int dwc3_probe(struct platform_device *pdev)
> > dwc->regs = regs;
> > dwc->regs_size = resource_size(&dwc_res);
> >
> > - if (!dwc3_core_is_valid(dwc)) {
> > - dev_err(dwc->dev, "this is not a DesignWare USB3 DRD Core\n");
> > - return -ENODEV;
> > - }
> > -
> > dwc3_get_properties(dwc);
> >
> > dwc->reset = devm_reset_control_get_optional_shared(dev, NULL);
> @@
> > -1460,6 +1455,11 @@ static int dwc3_probe(struct platform_device *pdev)
> > if (ret)
> > goto unprepare_clks;
> >
> > + if (!dwc3_core_is_valid(dwc)) {
> > + dev_err(dwc->dev, "this is not a DesignWare USB3 DRD Core\n");
>
> Please update "ret" here with -ENODEV, else the probe call will return
> success (ret
> is 0).
Correct, I will update and send V2.
Thanks
Jun
>
> > + goto disable_clks;
> > + }
> > +
> > platform_set_drvdata(pdev, dwc);
> > dwc3_cache_hwparams(dwc);
> >
> > @@ -1524,7 +1524,7 @@ static int dwc3_probe(struct platform_device *pdev)
> > err1:
> > pm_runtime_put_sync(&pdev->dev);
> > pm_runtime_disable(&pdev->dev);
> > -
> > +disable_clks:
> > clk_bulk_disable(dwc->num_clks, dwc->clks);
> > unprepare_clks:
> > clk_bulk_unprepare(dwc->num_clks, dwc->clks);
>
> Regards,
>
> Sriharsha
Re: [PATCH V2 4/8] dt-bindings: usb: Add NVIDIA Tegra XUSB device mode controller binding
On 03-05-2019 20:00, Thierry Reding wrote:
> On Thu, Apr 25, 2019 at 05:14:01PM +0200, Thierry Reding wrote:
>> On Mon, Mar 11, 2019 at 04:41:52PM +0530, Nagarjuna Kristam wrote:
>>> Add device-tree binding documentation for the XUSB device mode controller
>>> present on tegra210 SoC. This controller supports USB 3.0 specification
>>
>> Tegra210, please. "... supports the USB 3.0 ...". Also end sentences
>> with a fullstop.
>>
Will update
>>>
>>> Based on work by Andrew Bresticker .
>>>
>>> Signed-off-by: Nagarjuna Kristam
>>> ---
>>> .../devicetree/bindings/usb/nvidia,tegra-xudc.txt | 105
>>> +
>>> 1 file changed, 105 insertions(+)
>>> create mode 100644
>>> Documentation/devicetree/bindings/usb/nvidia,tegra-xudc.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra-xudc.txt
>>> b/Documentation/devicetree/bindings/usb/nvidia,tegra-xudc.txt
>>> new file mode 100644
>>> index 000..990655d
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/usb/nvidia,tegra-xudc.txt
>>> @@ -0,0 +1,105 @@
>>> +Device tree binding for NVIDIA Tegra XUSB device mode controller (XUDC)
>>> +===
>>> +
>>> +The Tegra XUDC controller supports both USB 2.0 HighSpeed/FullSpeed and
>>> +USB 3.0 SuperSpeed protocols.
>>> +
>>> +Required properties:
>>> +
>>> +- compatible: For Tegra210, must contain "nvidia,tegra210-xudc".
>>> +- reg: Must contain the base and length of the XUSB device registers, XUSB
>>> device
>>> + PCI Config registers and XUSB device controller registers.
>>> +- interrupts: Must contain the XUSB device interrupt
>>> +- clocks: Must contain an entry for ell clocks used.
>>
>> s/ell/all/
>>
will do
>>> + See ../clock/clock-bindings.txt for details.
>>> +- clock-names: Must include the following entries:
>>> + - xusb_device
>>> + - xusb_ss
>>> + - xusb_ss_src
>>> + - xusb_hs_src
>>> + - xusb_fs_src
>>
>> It'd be good to explain what each of these are.
>
> Perhaps we should also drop the xusb_ prefix here. That's already
> implied by this being the XUDC controller bindings.
>
xusb_ prefix is added to be in sync with USB host driver. considering that this
file
already for XUSB hardware, it can be removed.
Will add details and remove xusb_ prefix.
>>
>>> +- nvidia,xusb-padctl: phandle to the XUSB pad controller that is used to
>>> + configure the USB pads used by the XUDC controller
>>> +- power-domains: A list of PM domain specifiers that reference each
>>> power-domain
>>> + used by the XUSB device mode controller. This list must comprise of a
>>> specifier
>>> + for the XUSBA and XUSBB power-domains. See ../power/power_domain.txt and
>>> + ../arm/tegra/nvidia,tegra20-pmc.txt for details.
>>> +- power-domain-names: A list of names that represent each of the
>>> specifiers in
>>> + the 'power-domains' property. Must include 'xusb_ss' and 'xusb_device'
>
> Same here, I'd drop the xusb_ prefix. I think the "device" power domain
> is also usually referred to as "XUSB_DEV", so perhaps make that "dev"
> instead?
>
> Thierry
>
will update accordingly. and apply event at clocks naming for "device" to "dev".
>>> +
>>> +For Tegra210:
>>> +- avddio-usb-supply: PCIe/USB3 analog logic power supply. Must supply 1.05
>>> V.
>>> +- hvdd-usb-supply: USB controller power supply. Must supply 3.3 V.
>>> +- avdd-pll-utmip-supply: UTMI PLL power supply. Must supply 1.8 V.
>>
>> My understanding is that this last supply is really needed for the XUSB
>> pad controller to bring up the PLL. In fact, I've just moved the same
>> supply to the XUSB pad controller from the XUSB controller for all of
>> the supported boards because having this in the XUSB controller would
>> fail under some circumstances.
>>
Will remove avdd-pll-utmip-supply from xudc driver, as its now part of padctl
>>> +
>>> +- phys: Must contain an entry for each entry in phy-names.
>>> + See ../phy/phy-bindings.txt for details.
>>> +- extcon-usb: Must contains an extcon-usb entry which detects
>>
>> In the example below, this is simply "extcon".
>>
yes, its extcon only, will update accordingly.
>>> + USB VBUS pin. See ../extcon/extcon-usb-gpio.txt for details.
>>> +
>>> +Optional properties:
>>> +
>>> +- phy-names: Should include an entry for each PHY used by the controller.
>>> + Names must be "usb2", and "usb3" if support SuperSpeed device mode.
>>> + - "usb3" phy, SuperSpeed (SSTX+/SSTX-/SSRX+/SSRX-) data lines
>>> + - "usb2" phy, USB 2.0 (D+/D-) data lines
>>
>> Why are these optional? phys is required and references phy-names
>> explicitly, so I think that effectively makes these phy-names required
>> as well.
>>
Reason being is SS mode is optional, but not usb 2.0 or earlier and hence added
in
optional, but considering phys and phy-names goes together, will move this
section to
required.
>>> +
>>> +Example:
>>> +
>>> + pmc: pmc@7000e400 {
>>> + c
[PATCH v2] usb: dwc3: move core validation to be after clks enable
From: Jun Li
Register access in core validation may hang before the bulk
clks are enabled.
Fixes: b873e2d0ea1e ("usb: dwc3: Do core validation early on probe")
Signed-off-by: Jun Li
---
Change for v2:
- Update ret to be -ENODEV in case dwc3_core_is_valid() fail.
drivers/usb/dwc3/core.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 4aff1d8..93b96e6 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1423,11 +1423,6 @@ static int dwc3_probe(struct platform_device *pdev)
dwc->regs = regs;
dwc->regs_size = resource_size(&dwc_res);
- if (!dwc3_core_is_valid(dwc)) {
- dev_err(dwc->dev, "this is not a DesignWare USB3 DRD Core\n");
- return -ENODEV;
- }
-
dwc3_get_properties(dwc);
dwc->reset = devm_reset_control_get_optional_shared(dev, NULL);
@@ -1460,6 +1455,12 @@ static int dwc3_probe(struct platform_device *pdev)
if (ret)
goto unprepare_clks;
+ if (!dwc3_core_is_valid(dwc)) {
+ dev_err(dwc->dev, "this is not a DesignWare USB3 DRD Core\n");
+ ret = -ENODEV;
+ goto disable_clks;
+ }
+
platform_set_drvdata(pdev, dwc);
dwc3_cache_hwparams(dwc);
@@ -1524,7 +1525,7 @@ static int dwc3_probe(struct platform_device *pdev)
err1:
pm_runtime_put_sync(&pdev->dev);
pm_runtime_disable(&pdev->dev);
-
+disable_clks:
clk_bulk_disable(dwc->num_clks, dwc->clks);
unprepare_clks:
clk_bulk_unprepare(dwc->num_clks, dwc->clks);
--
2.7.4
Re: [V2] usb: gadget: storage: Remove warning message
On Wed, 8 May 2019, EJ Hsu wrote:
> This change is to fix below warning message in following scenario:
> usb_composite_setup_continue: Unexpected call
>
> When system tried to enter suspend, the fsg_disable() will be called to
> disable fsg driver and send a signal to fsg_main_thread. However, at
> this point, the fsg_main_thread has already been frozen and can not
> respond to this signal. So, this signal will be pended until
> fsg_main_thread wakes up.
>
> Once system resumes from suspend, fsg_main_thread will detect a signal
> pended and do some corresponding action (in handle_exception()). Then,
> host will send some setup requests (get descriptor, set configuration...)
> to UDC driver trying to enumerate this device. During the handling of "set
> configuration" request, it will try to sync up with fsg_main_thread by
> sending a signal (which is the same as the signal sent by fsg_disable)
> to it. In a similar manner, once the fsg_main_thread receives this
> signal, it will call handle_exception() to handle the request.
>
> However, if the fsg_main_thread wakes up from suspend a little late and
> "set configuration" request from Host arrives a little earlier,
> fsg_main_thread might come across the request from "set configuration"
> when it handles the signal from fsg_disable(). In this case, it will
> handle this request as well. So, when fsg_main_thread tries to handle
> the signal sent from "set configuration" later, there will nothing left
> to do and warning message "Unexpected call" is printed.
>
> Signed-off-by: EJ Hsu
> ---
> v2: remove the copyright info
> ---
> drivers/usb/gadget/function/f_mass_storage.c | 17 ++---
> drivers/usb/gadget/function/storage_common.h | 1 +
> 2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c
> b/drivers/usb/gadget/function/f_mass_storage.c
> index 043f97a..ba1d827 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -2294,7 +2294,7 @@ static void fsg_disable(struct usb_function *f)
> {
> struct fsg_dev *fsg = fsg_from_func(f);
> fsg->common->new_fsg = NULL;
That line is no longer needed.
> - raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE);
> + raise_exception(fsg->common, FSG_STATE_DISCONNECT);
> }
>
>
> @@ -2307,6 +2307,7 @@ static void handle_exception(struct fsg_common *common)
> enum fsg_state old_state;
> struct fsg_lun *curlun;
> unsigned intexception_req_tag;
> + struct fsg_dev *fsg;
>
> /*
>* Clear the existing signals. Anything but SIGUSR1 is converted
> @@ -2413,9 +2414,19 @@ static void handle_exception(struct fsg_common *common)
> break;
>
> case FSG_STATE_CONFIG_CHANGE:
> - do_set_interface(common, common->new_fsg);
> - if (common->new_fsg)
> + fsg = common->new_fsg;
> + /*
> + * Add a check here to double confirm if a disconnect event
> + * occurs and common->new_fsg has been cleared.
> + */
> + if (fsg) {
> + do_set_interface(common, fsg);
> usb_composite_setup_continue(common->cdev);
> + }
> + break;
> +
> + case FSG_STATE_DISCONNECT:
> + do_set_interface(common, NULL);
> break;
>
> case FSG_STATE_EXIT:
> diff --git a/drivers/usb/gadget/function/storage_common.h
> b/drivers/usb/gadget/function/storage_common.h
> index e5e3a25..12687f7 100644
> --- a/drivers/usb/gadget/function/storage_common.h
> +++ b/drivers/usb/gadget/function/storage_common.h
> @@ -161,6 +161,7 @@ enum fsg_state {
> FSG_STATE_ABORT_BULK_OUT,
> FSG_STATE_PROTOCOL_RESET,
> FSG_STATE_CONFIG_CHANGE,
> + FSG_STATE_DISCONNECT,
> FSG_STATE_EXIT,
> FSG_STATE_TERMINATED
> };
You forgot to change fsg_unbind() to use FSG_STATE_DISCONNECT. And
when that's done, it will no longer need to set common->new_fsg to NULL
either.
This is sort of a band-aid approach. The real problem is that the
original design of the driver never intended for there to be more than
one outstanding CONFIG_CHANGE event, so naturally there are scenarios
where it doesn't work right when that assumption is violated. This
patch addresses one of those scenarios, but there may be others
remaining.
Ultimately the problem boils down to synchronization with the composite
core. Some of the callbacks made by the core take time to fully
process, so what should happen if the core makes a second callback
before the first one is completely processed?
On the other hand, I can't think of anything better at the moment.
Alan Stern
Re: [PATCH v2 1/1] usb: typec: tcpci: Clear the fault status register
On Wed, May 08, 2019 at 07:48:43AM -0600, Angus Ainslie wrote:
> Hi Guenter
>
> On 2019-05-07 23:18, Guenter Roeck wrote:
> >On 5/7/19 7:49 PM, Angus Ainslie wrote:
> >>On 2019-05-07 20:03, Guenter Roeck wrote:
> >>>On 5/7/19 5:27 PM, Angus Ainslie (Purism) wrote:
> If the fault status register doesn't get cleared then
> the ptn5110 interrupt gets stuck on. As the fault register gets
> set everytime the ptn5110 powers on the interrupt is always stuck.
>
> Fixes: fault status register stuck
> Signed-off-by: Angus Ainslie (Purism)
> ---
> drivers/usb/typec/tcpm/tcpci.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/usb/typec/tcpm/tcpci.c
> b/drivers/usb/typec/tcpm/tcpci.c
> index c1f7073a56de..a5746657b190 100644
> --- a/drivers/usb/typec/tcpm/tcpci.c
> +++ b/drivers/usb/typec/tcpm/tcpci.c
> @@ -463,6 +463,17 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
> else if (status & TCPC_ALERT_TX_FAILED)
> tcpm_pd_transmit_complete(tcpci->port, TCPC_TX_FAILED);
> + if (status & TCPC_ALERT_FAULT) {
> >>>
> >>>Wait - the driver doesn't set TCPC_ALERT_FAULT in the alert mask
> >>>register. How can the chip report it if fault alerts are not enabled ?
> >>
> >>Well that I didn't check. But I know this code gets executed so
> >>something must be turning it on.
> >>
> >>Also if I don't clear it I get an unlimited number of interrupts.
> >>
> >>>What am I missing here ?
> >>
> >>Can the power on fault be masked ?
> >>
> >
> >There is a TCPC_ALERT_FAULT mask bit, so I would think so.
> >Can you dump register contents in the irq function and at the end of
> >tcpci_init() ?
> >
>
> Ok so this seems to be related to imx8mq errata e7805:
>
> I2C: When the I2C clock speed is configured for 400 kHz, the SCL low period
> violates the I2C spec of
> 1.3 uS min
>
> The work around suggested by NXP is to set the clock to 384 kHz so that is
> what I did and this is the output:
>
> [4.091512] device: 'tcpm-source-psy-0-0052': device_add
> [4.091581] PM: Adding info for No Bus:tcpm-source-psy-0-0052
> [4.091596] device: 'tcpm-source-psy-0-0052': dev_uevent: class uevent()
> returned -11
> [4.094774] tcpci 0-0052: ALERT MASK 0x7f
> [4.107869] driver: 'tcpci': driver_bound: bound to device '0-0052'
> [4.107935] bus: 'i2c': really_probe: bound device 0-0052 to driver tcpci
> [4.110994] tcpci 0-0052: ALERT MASK 0x7f
> [4.115511] tcpci 0-0052: FAULT ALERT status 0x80
> [4.126332] tcpci 0-0052: ALERT MASK 0x7f
> [4.130784] tcpci 0-0052: FAULT ALERT status 0x0
>
> The first "ALERT MASK" is in the init function immediately after setting
>
> reg = TCPC_ALERT_TX_SUCCESS | TCPC_ALERT_TX_FAILED |
> TCPC_ALERT_TX_DISCARDED | TCPC_ALERT_RX_STATUS |
> TCPC_ALERT_RX_HARD_RST | TCPC_ALERT_CC_STATUS;
> if (tcpci->controls_vbus)
> reg |= TCPC_ALERT_POWER_STATUS;
> ret = tcpci_write16(tcpci, TCPC_ALERT_MASK, reg);
>
>
> So it looks like the register is correct but the fault interrupt still
> fires. At 200 kHz I get the following output.
>
> [4.136845] device: 'tcpm-source-psy-0-0052': device_add
> [4.136943] PM: Adding info for No Bus:tcpm-source-psy-0-0052
> [4.136966] device: 'tcpm-source-psy-0-0052': dev_uevent: class uevent()
> returned -11
> [4.178510] tcpci 0-0052: ALERT MASK 0x7f
> [4.217197] driver: 'tcpci': driver_bound: bound to device '0-0052'
> [4.217371] bus: 'i2c': really_probe: bound device 0-0052 to driver tcpci
>
> So this is what is expected no fault interrupt.
>
> Maybe errata e7805 needs an update.
>
> Sorry for the noise.
>
Let's not jump to conclusions; I don't think this is noise. It is more
likely that the i2c problem uncovers a race condition in tcpci_init().
In tcpci_init(), we first clear TCPC_ALERT by writing 0x into it.
Subsequently, we set TCPC_ALERT_MASK. I suspect what may happen is
that the chip has FAULT_ALERT enabled, and that a fault was logged.
We don't clear the FAULT_STATUS register in tcpci_init(), thus
FAULT_ALERT is immediately set again, before we clear the FAULT_ALERT
mask bit.
The standard says that the alert pin shall not be set if the respective
interrupt is masked, but maybe the chip doesn't follow that. Either case,
the standard does say that masked alerts are still reported in the status
registers, so it is not surprising that the fault status is reported.
What we should probably do in tcpci_init() is to change the register
initialization order, and to clear the fault status register.
- Set TCPC_ALERT_MASK
- Set FAULT_STATUS_MASK (to 0)
- Clear TCPC_FAULT_STATUS
- Clear TCPC_ALERT
I suspect that will fix your problem.
Another question is if TCPC_ALERT_FAULT (together with appropriate
FAULT_STATUS_MASK bits) should be enabled, and if faults should be
logged. But that would be a separate patch or patch series.
Thanks,
Gu
