Re: [PATCH] Input: twl4030_keypad - Fix missing IRQF_ONESHOT as only threaded handler
zhuguangqin...@gmail.com writes: > From: Guangqing Zhu > > Coccinelle noticed: > drivers/input/keyboard/twl4030_keypad.c:413:9-34: ERROR: Threaded IRQ with > no primary handler requested without IRQF_ONESHOT > > Signed-off-by: Guangqing Zhu Reviewed-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [PATCH V3 2/4] soc: qcom: dcc:Add driver support for Data Capture and Compare unit(DCC)
Hi, Souradeep Chowdhury writes: > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile > index ad675a6..e7f0ccb 100644 > --- a/drivers/soc/qcom/Makefile > +++ b/drivers/soc/qcom/Makefile > @@ -1,19 +1,22 @@ > # SPDX-License-Identifier: GPL-2.0 > CFLAGS_rpmh-rsc.o := -I$(src) > obj-$(CONFIG_QCOM_AOSS_QMP) += qcom_aoss.o > -obj-$(CONFIG_QCOM_GENI_SE) +=qcom-geni-se.o > +obj-$(CONFIG_QCOM_APR) += apr.o > obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o > obj-$(CONFIG_QCOM_CPR) += cpr.o > +obj-$(CONFIG_QCOM_DCC) += dcc.o > +obj-$(CONFIG_QCOM_GENI_SE) += qcom-geni-se.o > obj-$(CONFIG_QCOM_GSBI) += qcom_gsbi.o > +obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) += kryo-l2-accessors.o > +obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o > obj-$(CONFIG_QCOM_MDT_LOADER)+= mdt_loader.o > obj-$(CONFIG_QCOM_OCMEM) += ocmem.o > obj-$(CONFIG_QCOM_PDR_HELPERS) += pdr_interface.o > obj-$(CONFIG_QCOM_QMI_HELPERS) += qmi_helpers.o > -qmi_helpers-y+= qmi_encdec.o qmi_interface.o > obj-$(CONFIG_QCOM_RMTFS_MEM) += rmtfs_mem.o > obj-$(CONFIG_QCOM_RPMH) += qcom_rpmh.o > -qcom_rpmh-y += rpmh-rsc.o > -qcom_rpmh-y += rpmh.o > +obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o > +obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o > obj-$(CONFIG_QCOM_SMD_RPM) += smd-rpm.o > obj-$(CONFIG_QCOM_SMEM) += smem.o > obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o > @@ -21,8 +24,6 @@ obj-$(CONFIG_QCOM_SMP2P)+= smp2p.o > obj-$(CONFIG_QCOM_SMSM) += smsm.o > obj-$(CONFIG_QCOM_SOCINFO) += socinfo.o > obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o > -obj-$(CONFIG_QCOM_APR) += apr.o > -obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o > -obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o > -obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o > -obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) += kryo-l2-accessors.o > +qmi_helpers-y += qmi_encdec.o qmi_interface.o > +qcom_rpmh-y += rpmh-rsc.o > +qcom_rpmh-y += rpmh.o why so many changes? > diff --git a/drivers/soc/qcom/dcc.c b/drivers/soc/qcom/dcc.c > new file mode 100644 > index 000..fcd5580 > --- /dev/null > +++ b/drivers/soc/qcom/dcc.c > @@ -0,0 +1,1539 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2015-2021, The Linux Foundation. All rights reserved. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > + one blank line is enough > +#define TIMEOUT_US 100 > + > +#define dcc_writel(drvdata, val, off) > \ > + writel((val), drvdata->base + dcc_offset_conv(drvdata, off)) > +#define dcc_readl(drvdata, off) > \ > + readl(drvdata->base + dcc_offset_conv(drvdata, off)) > + > +#define dcc_sram_readl(drvdata, off) \ > + readl(drvdata->ram_base + off) this would be probably be better as static inlines. -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc3: gadget: Avoid canceling current request for queuing error
Wesley Cheng writes: > If an error is received when issuing a start or update transfer > command, the error handler will stop all active requests (including > the current USB request), and call dwc3_gadget_giveback() to notify > function drivers of the requests which have been stopped. Avoid > having to cancel the current request which is trying to be queued, as > the function driver will handle the EP queue error accordingly. > Simply unmap the request as it was done before, and allow previously > started transfers to be cleaned up. > > Signed-off-by: Wesley Cheng > --- > drivers/usb/dwc3/gadget.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index e1b04c97..4200775 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -1399,6 +1399,11 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep > *dep) > if (ret == -EAGAIN) > return ret; > > + /* Avoid canceling current request, as it has not been started > */ > + if (req->trb) > + memset(req->trb, 0, sizeof(struct dwc3_trb)); we don't need a full memset. I think ensuring HWO bit is zero is enough. -- balbi signature.asc Description: PGP signature
Re: [PATCH v2 2/3] arm64: dts: qcom: sm8150: add i2c nodes
Caleb Connolly writes: > Tested on the OnePlus 7 Pro (including DMA). > > Signed-off-by: Caleb Connolly > Reviewed-by: Vinod Koul > Reviewed-by: Bhupesh Sharma Tested on Microsoft Surface Duo (DTS will be sent after -rc1) Tested-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [PATCH v2] usb: dwc3: core: Add shutdown callback for dwc3
Sandeep Maheswaram writes: > This patch adds a shutdown callback to USB DWC core driver to ensure that > it is properly shutdown in reboot/shutdown path. This is required > where SMMU address translation is enabled like on SC7180 > SoC and few others. If the hardware is still accessing memory after > SMMU translation is disabled as part of SMMU shutdown callback in > system reboot or shutdown path, then IOVAs(I/O virtual address) > which it was using will go on the bus as the physical addresses which > might result in unknown crashes (NoC/interconnect errors). > > Signed-off-by: Sandeep Maheswaram Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [PATCH] regulator: Use IRQF_ONESHOT
Hi,
Krzysztof Kozlowski writes:
> On 23/03/2021 13:12, Jian Dong wrote:
>> From: Jian Dong
>>
>> Fixes coccicheck error:
>>
>> drivers/regulator/mt6360-regulator.c:388:8-33: ERROR:
>> drivers/regulator/pca9450-regulator.c:781:7-32: ERROR:
>> drivers/regulator/slg51000-regulator.c:480:8-33: ERROR:
>> drivers/regulator/qcom-labibb-regulator.c:364:8-33: ERROR:
>> Threaded IRQ with no primary handler requested without IRQF_ONESHOT
>>
>> Signed-off-by: Jian Dong
>> ---
>> drivers/regulator/mt6360-regulator.c | 4 ++--
>> drivers/regulator/pca9450-regulator.c | 2 +-
>> drivers/regulator/qcom-labibb-regulator.c | 3 ++-
>> drivers/regulator/slg51000-regulator.c| 4 ++--
>> 4 files changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/regulator/mt6360-regulator.c
>> b/drivers/regulator/mt6360-regulator.c
>> index 15308ee..947350d 100644
>> --- a/drivers/regulator/mt6360-regulator.c
>> +++ b/drivers/regulator/mt6360-regulator.c
>> @@ -385,8 +385,8 @@ static int mt6360_regulator_irq_register(struct
>> platform_device *pdev,
>> return irq;
>> }
>>
>> -ret = devm_request_threaded_irq(>dev, irq, NULL,
>> irq_desc->handler, 0,
>> -irq_desc->name, rdev);
>> +ret = devm_request_threaded_irq(>dev, irq, NULL,
>> irq_desc->handler,
>> +IRQF_ONESHOT, irq_desc->name, rdev);
>
> This does not look like trivial rename/replace fix. This should be
> tested but it looks that you just did what coccinelle asked for, without
> testing.
Right, but it must be done. If things work today, they work out of sheer
luck. Also, which evidence is there that $subject wasn't tested?
>> diff --git a/drivers/regulator/pca9450-regulator.c
>> b/drivers/regulator/pca9450-regulator.c
>> index 2f7ee21..d4bc1c3 100644
>> --- a/drivers/regulator/pca9450-regulator.c
>> +++ b/drivers/regulator/pca9450-regulator.c
>> @@ -780,7 +780,7 @@ static int pca9450_i2c_probe(struct i2c_client *i2c,
>>
>> ret = devm_request_threaded_irq(pca9450->dev, pca9450->irq, NULL,
>> pca9450_irq_handler,
>> -(IRQF_TRIGGER_FALLING | IRQF_ONESHOT),
>> +IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>
> How this is related to the missing IRQF_ONESHOT?
agreed.
>> diff --git a/drivers/regulator/slg51000-regulator.c
>> b/drivers/regulator/slg51000-regulator.c
>> index 75a941f..3f310ab 100644
>> --- a/drivers/regulator/slg51000-regulator.c
>> +++ b/drivers/regulator/slg51000-regulator.c
>> @@ -479,8 +479,8 @@ static int slg51000_i2c_probe(struct i2c_client *client)
>> if (chip->chip_irq) {
>> ret = devm_request_threaded_irq(dev, chip->chip_irq, NULL,
>> slg51000_irq_handler,
>> -(IRQF_TRIGGER_HIGH |
>> -IRQF_ONESHOT),
>> +IRQF_TRIGGER_HIGH |
>> +IRQF_ONESHOT,
>> "slg51000-irq", chip);
>
> How this is related to the missing IRQF_ONESHOT?
agreed.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH] usb: dwc3: add cancelled reason for dwc3 requests
Hi, Ray Chi writes: > diff --git a/drivers/usb/dwc3/gadget.h b/drivers/usb/dwc3/gadget.h > index 0cd281949970..a23e85bd3933 100644 > --- a/drivers/usb/dwc3/gadget.h > +++ b/drivers/usb/dwc3/gadget.h > @@ -56,6 +56,12 @@ struct dwc3; > > /* Frame/Microframe Number Mask */ > #define DWC3_FRNUMBER_MASK 0x3fff > + > +/* Cancel reason for dwc3 request */ > +#define DWC3_REQUEST_DEQUEUED-ECONNRESET /* Request get > dequeued */ > +#define DWC3_REQUEST_DISCONNECTED-ESHUTDOWN /* Device is > disconnected/disabled */ > +#define DWC3_REQUEST_STALL -EPIPE /* Bus or protocol error */ this is just obfuscation, pass the errors directly. Also, make sure these are documented in the API. -- balbi signature.asc Description: PGP signature
Re: usb: dwc3: gadget: skip pullup and set_speed after suspend
Hi, Daehwan Jung writes: > Sometimes dwc3_gadget_pullup and dwc3_gadget_set_speed are called after > entering suspend. That's why it needs to check whether suspend > > 1. dwc3 sends disconnect uevent and turn off. (suspend) > 2. Platform side causes pullup or set_speed(e.g., adbd closes ffs node) > 3. It causes unexpected behavior like ITMON error. please collect dwc3 trace events showing this problem. -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: gadget: u_serial: Remove old tasklet comments
Davidlohr Bueso writes: > Update old comments as of 8b4c62aef6f (usb: gadget: u_serial: process RX > in workqueue instead of tasklet). > > Signed-off-by: Davidlohr Bueso Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [PATCH] USB: gadget: udc: Process disconnect synchronously
Davidlohr Bueso writes: > As the comment in usb_disconnect() hints, do not defer the > disconnect processing, and instead just do it directly in > the irq handler. This allows the driver to avoid using a > nowadays deprecated tasklet. > > Signed-off-by: Davidlohr Bueso You may have to deal with VBUS valid, but for now this is good: Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [PATCH 1/7] dt-bindings: usb: qcom,dwc3: Add binding for SDX55
Hi, Manivannan Sadhasivam writes: > Add devicetree binding for SDX55 USB controller based on Qcom designware > IP. > > Cc: Rob Herring > Cc: devicet...@vger.kernel.org > Cc: linux-...@vger.kernel.org > Signed-off-by: Manivannan Sadhasivam Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [PATCH v2 4/4] dt-bindings: usb: qcom,dwc3: Add bindings for SM8150, SM8250, SM8350
Hi, Jack Pham writes: > Add compatible strings for the USB DWC3 controller on QCOM SM8150, > SM8250 and SM8350 SoCs. > > Note the SM8150 & SM8250 compatibles are already being used in the > dts but was missing from the documentation. > > Signed-off-by: Jack Pham > --- > Documentation/devicetree/bindings/usb/qcom,dwc3.yaml | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml > b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml > index 2cf525d21e05..da47f43d6b04 100644 > --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml > +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml > @@ -17,6 +17,9 @@ properties: >- qcom,msm8998-dwc3 >- qcom,sc7180-dwc3 >- qcom,sdm845-dwc3 > + - qcom,sm8150-dwc3 > + - qcom,sm8250-dwc3 > + - qcom,sm8350-dwc3 nicely done! Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [RESEND PATCH v3 2/2] usb: dwc3: Add driver for Xilinx platforms
Hi, Michael Grzeschik writes: > On Tue, Dec 15, 2020 at 12:24:51PM +0530, Manish Narani wrote: >>Add a new driver for supporting Xilinx platforms. This driver is used >>for some sequence of operations required for Xilinx USB controllers. >>This driver is also used to choose between PIPE clock coming from SerDes >>and the Suspend Clock. Before the controller is out of reset, the clock >>selection should be changed to PIPE clock in order to make the USB >>controller work. There is a register added in Xilinx USB controller >>register space for the same. > > I tried out this driver with the vanilla kernel on an zynqmp. Without > this patch the USB-Gadget is already acting buggy. In the gadget mode, > some iterations of plug/unplug results to an stalled gadget which will > never come back without a reboot. > > With the corresponding code of this driver (reset assert, clk modify, > reset deassert) in the downstream kernels phy driver we found out it is > totaly stable. But using this exact glue driver which should do the same > as the downstream code, the gadget still was buggy the way described > above. > > I suspect the difference lays in the different order of operations. > While the downstream code is runing the resets inside the phy driver > which is powered and initialized in the dwc3-core itself. With this glue > layser approach of this patch the whole phy init is done before even > touching dwc3-core in any way. It seems not to have the same effect, > though. > > If really the order of operations is limiting us, we probably need > another solution than this glue layer. Any Ideas? might be a good idea to collect dwc3 trace events. Can you do that? -- balbi
Re: [PATCH] usb: bdc: Remove the BDC PCI driver
Hi, Greg Kroah-Hartman writes: >> Al Cooper writes: >> > The BDC PCI driver was only used for design verification with >> > an PCI/FPGA board. The board no longer exists and is not in use >> > anywhere. All instances of this core now exist as a memory mapped >> > device on the platform bus. >> > >> > NOTE: This only removes the PCI driver and does not remove the >> > platform driver. >> > >> > Signed-off-by: Al Cooper >> >> It sounds like it could be used for pre-silicon verification of newer >> Core Releases, much like Synopsys still uses the HAPS (with mainline >> linux, mind you) for silicon validation. >> >> Why would we delete this small shim if it *could* still be useful? > > It ends up conflicting with the PCI id of a device that is actually in > the wild (a camera on Apple laptops). So it's good to drop this driver > so the wrong driver doesn't get constantly bound to the wrong device. I see. Oh well... -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: bdc: Remove the BDC PCI driver
Hi, Al Cooper writes: > The BDC PCI driver was only used for design verification with > an PCI/FPGA board. The board no longer exists and is not in use > anywhere. All instances of this core now exist as a memory mapped > device on the platform bus. > > NOTE: This only removes the PCI driver and does not remove the > platform driver. > > Signed-off-by: Al Cooper It sounds like it could be used for pre-silicon verification of newer Core Releases, much like Synopsys still uses the HAPS (with mainline linux, mind you) for silicon validation. Why would we delete this small shim if it *could* still be useful? -- balbi signature.asc Description: PGP signature
Re: [PATCH v3 4/4] usb: dwc3: pci: add support for the Intel Alder Lake-P
Heikki Krogerus writes: > This patch adds the necessary PCI ID for Intel Alder Lake-P > devices. > > Signed-off-by: Heikki Krogerus The only missing my ack: Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [PATCH] drivers/usb/gadget/udc: Assign boolean values to a bool variable
Jiapeng Zhong writes: > Fix the following coccicheck warnings: > > ./drivers/usb/gadget/udc/udc-xilinx.c:1957:2-18: WARNING: > Assignment of 0/1 to bool variable. > > Reported-by: Abaci Robot > Signed-off-by: Jiapeng Zhong Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [PATCH 07/10] dwc3: document gadget_max_speed
Mauro Carvalho Chehab writes: > This new field was added to struct dwc3_scratchpad_array, but > a documentation for it was missed: > > ../drivers/usb/dwc3/core.h:1259: warning: Function parameter or member > 'gadget_max_speed' not described in 'dwc3' > > Signed-off-by: Mauro Carvalho Chehab Thanks, Mauro. Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [PATCH RESEND v4 11/11] usb: gadget: bdc: fix checkpatch.pl repeated word warning
Chunfeng Yun writes: > fix the warning: > WARNING:REPEATED_WORD: Possible repeated word: 'and' > > Cc: Florian Fainelli > Signed-off-by: Chunfeng Yun > Acked-by: Florian Fainelli Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [PATCH RESEND v4 07/11] usb: gadget: bdc: avoid precedence issues
Chunfeng Yun writes: > Add () around macro argument to avoid precedence issues > > Cc: Florian Fainelli > Signed-off-by: Chunfeng Yun > Acked-by: Florian Fainelli Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [PATCH RESEND v4 10/11] usb: gadget: bdc: fix checkpatch.pl spacing error
Chunfeng Yun writes: > fix checkpatch.pl error: > ERROR:SPACING: space prohibited before that ',' > > Cc: Florian Fainelli > Signed-off-by: Chunfeng Yun > Acked-by: Florian Fainelli Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [PATCH RESEND v4 09/11] usb: gadget: bdc: fix checkpatch.pl tab warning
Chunfeng Yun writes: > WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements > WARNING:TABSTOP: Statements should start on a tabstop > > Cc: Florian Fainelli > Signed-off-by: Chunfeng Yun > Acked-by: Florian Fainelli Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [PATCH RESEND v4 08/11] usb: gadget: bdc: use the BIT macro to define bit filed
Chunfeng Yun writes: > Prefer using the BIT macro to define bit fileds > > Cc: Florian Fainelli > Signed-off-by: Chunfeng Yun > Acked-by: Florian Fainelli Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [PATCH RESEND v4 06/11] usb: gadget: bdc: add identifier name for function declaraion
Chunfeng Yun writes: > This is used to avoid the warning of function arguments, e.g. > WARNING:FUNCTION_ARGUMENTS: function definition argument 'u32' > should also have an identifier name > > Cc: Florian Fainelli > Signed-off-by: Chunfeng Yun > Acked-by: Florian Fainelli Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [PATCH RESEND v4 05/11] usb: gadget: bdc: fix check warning of block comments alignment
Chunfeng Yun writes: > fix the warning: > WARNING:BLOCK_COMMENT_STYLE: > Block comments should align the * on each line > > Cc: Florian Fainelli > Signed-off-by: Chunfeng Yun > Acked-by: Florian Fainelli Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [PATCH RESEND v4 04/11] usb: gadget: bdc: fix warning of embedded function name
Chunfeng Yun writes: > Use '"%s...", __func__' to replace embedded function name > > Cc: Florian Fainelli > Signed-off-by: Chunfeng Yun > Acked-by: Florian Fainelli Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [PATCH RESEND v4 03/11] usb: gadget: bdc: prefer pointer dereference to pointer type
Chunfeng Yun writes: > Prefer kzalloc(sizeof(*bd_table)...) over > kzalloc(sizeof(struct bd_table) > > Cc: Florian Fainelli > Signed-off-by: Chunfeng Yun > Acked-by: Florian Fainelli Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [PATCH RESEND v4 02/11] usb: gadget: bdc: remove bdc_ep_set_halt() declaration
Chunfeng Yun writes: > No definition for bdc_ep_set_halt(), so remove it. > > Cc: Florian Fainelli > Signed-off-by: Chunfeng Yun > Acked-by: Florian Fainelli Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [PATCH RESEND v4 01/11] usb: gadget: bdc: fix improper SPDX comment style for header file
Chunfeng Yun writes: > For C header files Documentation/process/license-rules.rst > mandates C-like comments (opposed to C source files where > C++ style should be used). > > Cc: Florian Fainelli > Signed-off-by: Chunfeng Yun > Acked-by: Florian Fainelli Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc3: qcom: Add shutdown callback for dwc3
Hi,
Sandeep Maheswaram writes:
> This patch adds a shutdown callback to USB DWC QCOM driver to ensure that
> it is properly shutdown in reboot/shutdown path. This is required
> where SMMU address translation is enabled like on SC7180
> SoC and few others. If the hardware is still accessing memory after
> SMMU translation is disabled as part of SMMU shutdown callback in
> system reboot or shutdown path, then IOVAs(I/O virtual address)
> which it was using will go on the bus as the physical addresses which
> might result in unknown crashes (NoC/interconnect errors).
>
> Signed-off-by: Sandeep Maheswaram
sounds like this is fixing a bug. Do you have a Fixes tag for it? Should
this go to stable?
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index c703d55..a930e06 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -790,13 +790,11 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> return ret;
> }
>
> -static int dwc3_qcom_remove(struct platform_device *pdev)
> +static void __dwc3_qcom_teardown(struct dwc3_qcom *qcom)
> {
> - struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
> - struct device *dev = >dev;
> int i;
>
> - of_platform_depopulate(dev);
> + of_platform_depopulate(qcom->dev);
>
> for (i = qcom->num_clocks - 1; i >= 0; i--) {
> clk_disable_unprepare(qcom->clks[i]);
> @@ -807,12 +805,27 @@ static int dwc3_qcom_remove(struct platform_device
> *pdev)
> dwc3_qcom_interconnect_exit(qcom);
> reset_control_assert(qcom->resets);
>
> - pm_runtime_allow(dev);
> - pm_runtime_disable(dev);
> + pm_runtime_allow(qcom->dev);
> + pm_runtime_disable(qcom->dev);
> +}
you can make the changes smaller by adding:
struct device *dev = qcom->dev;
The nothing else needs to change in this function ;-)
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH v2 0/3] Remove one more platform_device_add_properties() call
Heikki Krogerus writes: > Hi Felipe, Rafael, > > This is the second version of this series. There are no real changes, > but I added the Tiger Lake ID patch to this series in hope that it > will make your life a bit easier, assuming that Rafael will still pick > these. > > > The original over letter: > > I originally introduced these as part of my series where I was > proposing PM ops for software nodes [1], but since that still needs > work, I'm sending these two separately. > > So basically I'm only modifying dwc3-pci.c so it registers a software > node directly at this point. That will remove one more user of > platform_device_add_properties(). > > [1] > https://lore.kernel.org/lkml/20201029105941.63410-1-heikki.kroge...@linux.intel.com/ > > thanks, > > Heikki Krogerus (3): > software node: Introduce device_add_software_node() > usb: dwc3: pci: Register a software node for the dwc3 platform device > usb: dwc3: pci: ID for Tiger Lake CPU Looks good to me. Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb/gadget: f_midi: Replace tasklet with work
Hi, Davidlohr Bueso writes: > Currently a tasklet is used to transmit input substream buffer > data. However, tasklets have long been deprecated as being too > heavy on the system by running in irq context - and this is not > a performance critical path. If a higher priority process wants > to run, it must wait for the tasklet to finish before doing so. > > Deferring work to a workqueue and executing in process context > should be fine considering the callback already does > f_midi_do_transmit() under the transmit_lock and thus changes in > semantics are ok regarding concurrency - tasklets being serialized > against itself. > > Cc: Takashi Iwai > Signed-off-by: Davidlohr Bueso Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc3: core: Replace devm_reset_control_array_get()
Yejune Deng writes: > devm_reset_control_array_get_optional_shared() looks more readable > > Signed-off-by: Yejune Deng Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [PATCH v3 1/2] usb: dwc3: Trigger a GCTL soft reset when switching modes in DRD
Hi, John Stultz writes: > From: Yu Chen > > Just resending this, as discussion died out a bit and I'm not > sure how to make further progress. See here for debug data that > was requested last time around: > > https://lore.kernel.org/lkml/calaqxlxdnaufjkx0an9xwwtfwvjmwigppy2aqsnj56yvnbu...@mail.gmail.com/ > > With the current dwc3 code on the HiKey960 we often see the > COREIDLE flag get stuck off in __dwc3_gadget_start(), which > seems to prevent the reset irq and causes the USB gadget to > fail to initialize. > > We had seen occasional initialization failures with older > kernels but with recent 5.x era kernels it seemed to be becoming > much more common, so I dug back through some older trees and > realized I dropped this quirk from Yu Chen during upstreaming > as I couldn't provide a proper rational for it and it didn't > seem to be necessary. I now realize I was wrong. > > After resubmitting the quirk, Thinh Nguyen pointed out that it > shouldn't be a quirk at all and it is actually mentioned in the > programming guide that it should be done when switching modes > in DRD. > > So, to avoid these !COREIDLE lockups seen on HiKey960, this > patch issues GCTL soft reset when switching modes if the > controller is in DRD mode. > > Cc: Felipe Balbi > Cc: Tejas Joglekar > Cc: Yang Fei > Cc: YongQin Liu > Cc: Andrzej Pietrasiewicz > Cc: Thinh Nguyen > Cc: Jun Li > Cc: Mauro Carvalho Chehab > Cc: Greg Kroah-Hartman > Cc: linux-...@vger.kernel.org > Signed-off-by: Yu Chen > Signed-off-by: John Stultz > --- > v2: > * Rework to always call the GCTL soft reset in DRD mode, > rather then using a quirk as suggested by Thinh Nguyen > > v3: > * Move GCTL soft reset under the spinlock as suggested by > Thinh Nguyen Because this is such an invasive change, I would prefer that we get Tested-By tags from a good fraction of the users before applying these two changes. -- balbi
Re: [PATCH 1/1] usb: gadget: aspeed: fix stop dma register setting.
Hi, Ryan Chen writes: > The vhub engine has two dma mode, one is descriptor list, another > is single stage DMA. Each mode has different stop register setting. > Descriptor list operation (bit2) : 0 disable reset, 1: enable reset > Single mode operation (bit0) : 0 : disable, 1: enable > > Signed-off-by: Ryan Chen I don't have HW, but FWIW: Acked-by: Felipe Balbi -- balbi
Re: [PATCH v2 5/5] usb: gadget: u_audio: clean up locking
Jerome Brunet writes: > snd_pcm_stream_lock() is held when the ALSA .trigger() callback is called. > The lock of 'struct uac_rtd_params' is not necessary since all its locking > operation are done under the snd_pcm_stream_lock() too. > > Also, usb_request .complete() is called with irqs disabled, so saving and > restoring the irqs is not necessary. > > Signed-off-by: Jerome Brunet This is nice! Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [PATCH v2 4/5] usb: gadget: u_audio: remove struct uac_req
Jerome Brunet writes: > 'struct uac_req' purpose is to link 'struct usb_request' to the > corresponding 'struct uac_rtd_params'. However member req is never > used. Using the context of the usb request, we can keep track of the > corresponding 'struct uac_rtd_params' just as well, without allocating > extra memory. > > Signed-off-by: Jerome Brunet Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [PATCH v2 3/5] usb: gadget: u_audio: factorize ssize to alsa fmt conversion
Jerome Brunet writes: > Factorize format related code common to the capture and playback path. > > Signed-off-by: Jerome Brunet It's never a good idea to send fixes and cleanups/refactors in the same series as that can confuse the person applying your changes. In any case: Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [PATCH v2 1/5] usb: gadget: u_audio: Free requests only after callback
Jerome Brunet writes:
> From: Jack Pham
>
> As per the kernel doc for usb_ep_dequeue(), it states that "this
> routine is asynchronous, that is, it may return before the completion
> routine runs". And indeed since v5.0 the dwc3 gadget driver updated
> its behavior to place dequeued requests on to a cancelled list to be
> given back later after the endpoint is stopped.
>
> The free_ep() was incorrectly assuming that a request was ready to
> be freed after calling dequeue which results in a use-after-free
> in dwc3 when it traverses its cancelled list. Fix this by moving
> the usb_ep_free_request() call to the callback itself in case the
> ep is disabled.
>
> Fixes: eb9fecb9e69b0 ("usb: gadget: f_uac2: split out audio core")
> Reported-and-tested-by: Ferry Toth
> Reviewed-and-tested-by: Peter Chen
> Signed-off-by: Jack Pham
> Signed-off-by: Jerome Brunet
Looks good to me, just one comment below:
> @@ -336,8 +341,9 @@ static inline void free_ep(struct uac_rtd_params *prm,
> struct usb_ep *ep)
>
> for (i = 0; i < params->req_number; i++) {
> if (prm->ureq[i].req) {
> - usb_ep_dequeue(ep, prm->ureq[i].req);
> - usb_ep_free_request(ep, prm->ureq[i].req);
> + if (usb_ep_dequeue(ep, prm->ureq[i].req))
> + usb_ep_free_request(ep, prm->ureq[i].req);
do you mind adding a comment here stating that this is coping with a
possible error during usb_ep_dequeue()?
Other than that:
Acked-by: Felipe Balbi
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH v2 2/5] usb: gadget: f_uac2: reset wMaxPacketSize
Jerome Brunet writes:
> With commit 913e4a90b6f9 ("usb: gadget: f_uac2: finalize wMaxPacketSize
> according to bandwidth")
> wMaxPacketSize is computed dynamically but the value is never reset.
>
> Because of this, the actual maximum packet size can only decrease each time
> the audio gadget is instantiated.
>
> Reset the endpoint maximum packet size and mark wMaxPacketSize as dynamic
> to solve the problem.
>
> Fixes: 913e4a90b6f9 ("usb: gadget: f_uac2: finalize wMaxPacketSize according
> to bandwidth")
> Signed-off-by: Jerome Brunet
Acked-by: Felipe Balbi
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH] Revert "usb: gadget: Quieten gadget config message"
Hi, Albert Wang writes: > This reverts commit 1cbfb8c4f62d667f6b8b3948949737edb92992cccd. > > The log of USB enumeration result is a useful log and only occupies > one line especially when USB3 enumeration failed and then downgrade > to USB2. > > Signed-off-by: Albert Wang you can use dynamic debug to enable it whenever you're testing. -- balbi signature.asc Description: PGP signature
Re: [PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect
Hi,
Wesley Cheng writes:
> +void composite_reset(struct usb_gadget *gadget)
> +{
> + /*
> + * Section 1.4.13 Standard Downstream Port of the USB battery charging
> + * specification v1.2 states that a device connected on a SDP shall only
> + * draw at max 100mA while in a connected, but unconfigured state.
The requirements are different, though. I think OTG spec has some extra
requirements where only 8mA can be drawn max. You need to check for the
otg flag. Moreover, USB3+ spec has units of 150mA meaning the device
can't draw 100mA (IIRC).
--
balbi
Re: [PATCH] usb: gadget: select CONFIG_CRC32
Arnd Bergmann writes:
> From: Arnd Bergmann
>
> Without crc32 support, this driver fails to link:
>
> arm-linux-gnueabi-ld: drivers/usb/gadget/function/f_eem.o: in function
> `eem_unwrap':
> f_eem.c:(.text+0x11cc): undefined reference to `crc32_le'
> arm-linux-gnueabi-ld:
> drivers/usb/gadget/function/f_ncm.o:f_ncm.c:(.text+0x1e40):
> more undefined references to `crc32_le' follow
>
> Fixes: 6d3865f9d41f ("usb: gadget: NCM: Add transmit multi-frame.")
> Signed-off-by: Arnd Bergmann
Acked-by: Felipe Balbi
--
balbi
Re: [PATCH v4 1/4] dt-bindings: usb: add rk3328 dwc3 docs
Hi, Lindsey Stanpoor writes: > On Wed, Sep 2, 2020 at 11:12 AM wrote: >> >> From: Cameron Nemo >> >> Document compatible for dwc3 on the Rockchip rk3328 platform. > > Hi all, > > Wanted to give this patch submission a gentle ping. > > Rob Herring acked the documentation changes, but I have not heard > anything > from the USB or Rockchip maintainers. This patchset would facilitate USB3 > support for Rockchip rk3328 devices like the Pine Rock64. > > If there is anything I can do to help move this along, please let me know. Sorry, it had fallen through the cracks. It's now in my testing/next. -- balbi signature.asc Description: PGP signature
Re: [PATCH 5.9 000/391] 5.9.4-rc1 review
Hi, Naresh Kamboju writes: > On Wed, 4 Nov 2020 at 02:07, Greg Kroah-Hartman > wrote: >> >> This is the start of the stable review cycle for the 5.9.4 release. >> There are 391 patches in this series, all will be posted as a response >> to this one. If anyone has any issues with these being applied, please >> let me know. >> >> Responses should be made by Thu, 05 Nov 2020 20:29:58 +. >> Anything received after that time might be too late. >> >> The whole patch series can be found in one patch at: >> >> https://www.kernel.org/pub/linux/kernel/v5.x/stable-review/patch-5.9.4-rc1.gz >> or in the git tree and branch at: >> >> git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable-rc.git >> linux-5.9.y >> and the diffstat can be found below. >> >> thanks, >> >> greg k-h > > Results from Linaro’s test farm. > No regressions on arm64, arm, x86_64, and i386. > > Tested-by: Linux Kernel Functional Testing > > NOTE: > The kernel warning noticed on arm64 nxp ls2088 device with KASAN config > enabled while booting the device. We are not considering this as regression > because this is the first arm64 KASAN config enabled on nxp ls2088 device. > > [3.301882] dwc3 310.usb3: Failed to get clk 'ref': -2 > [3.307433] [ cut here ] > [3.312048] dwc3 310.usb3: request value same as default, ignoring fix your DTS :-) You're requesting to change a register value that shouldn't be changed (it should be properly set during coreConsultant instantiation). Whenever the requested value is the same as the reset value of the register we WARN to let users know that the register shouldn't be touched. -- balbi
Re: [PATCH v2 4/5] usb: dwc3: debugfs: Introduce DEFINE_SHOW_STORE_ATTRIBUTE
Luo Jiaxing writes: > Seq instroduce a new helper marco DEFINE_SHOW_STORE_ATTRIBUTE for ^^ ^ introduce macro > Read-Write file, So we apply it at dwc3 debugfs to reduce some duplicate ^^^ soduplicated > code. to be fair, your commit is doing more than what it claims. Maybe update commit log with a "while at that, also use DEFINE_SHOW_ATTRIBUTE() where possible". > Signed-off-by: Luo Jiaxing other than that, this looks okay. Since it depends on the definition of DEFINE_SHOW_STORE_ATTRIBUTE: Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [PATCH v3 31/56] usb: dwc3: fix kernel-doc markups
Greg Kroah-Hartman writes: > On Tue, Oct 27, 2020 at 08:58:47AM +0200, Felipe Balbi wrote: >> >> Hi Mauro, >> >> Mauro Carvalho Chehab writes: >> > There is a common comment marked, instead, with kernel-doc >> > notation. >> > >> > Also, some identifiers have different names between their >> > prototypes and the kernel-doc markup. >> > >> > Signed-off-by: Mauro Carvalho Chehab >> > --- >> > drivers/usb/dwc3/core.c| 2 +- >> > drivers/usb/dwc3/core.h| 2 +- >> > drivers/usb/gadget/composite.c | 2 +- >> > drivers/usb/typec/mux.c| 2 +- >> > include/linux/usb/composite.h | 2 +- >> >> mind breaking this into 4 commits? One for dwc3, one for >> gadget/composite, one for type/mux, and a final for composite.h. > > I'll just take these all at once, it's easy enough :) Sure thing, in that case: Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
Re: [PATCH v3] usb: dwc3: core: fix a issue about clear connect state
Hi, Dejin Zheng writes: >> Dejin Zheng writes: >> > According to Synopsys Programming Guide chapter 2.2 Register Resets, >> > it cannot reset the DCTL register by setting DCTL.CSFTRST for core soft >> > reset, if DWC3 controller as a slave device and stay connected with a usb >> > host, then, while rebooting linux, it will fail to reinitialize dwc3 as a >> > slave device when the DWC3 controller did not power off. because the >> > connection status is incorrect, so we also need to clear DCTL.RUN_STOP >> > bit for disabling connect when doing core soft reset. There will still >> > be other stale configuration in DCTL, so reset the other fields of DCTL >> > to the default value 0. >> >> This commit log is a bit hard to understand. When does this problem >> actually happen? It seems like it's in the case of, perhaps, kexecing >> into a new kernel, is that right? >> > It happens when entering the kernel for the second time after the reboot > command. > >> At the time dwc3_core_soft_reset() is called, the assumption is that >> we're starting with a clean core, from power up. If we have stale >> configuration from a previous run, we should fix this on the exit >> path. Note that if we're reaching probe with pull up connected, we >> already have issues elsewhere. >> >> I think this is not the right fix for the problem. >> > I think you are right, Thinh also suggested me fix it on the exit path > in the previous patch v2. Do you think I can do these cleanups in the > shutdown hook of this driver? Balbi, is there a more suitable place to > do this by your rich experience? Thanks! I don't think shutdown is called during removal, I'm not sure. I think we had some fixes done in shutdown time, though. Test it out, but make sure there are no issues with a regular modprobe cycle. -- balbi signature.asc Description: PGP signature
Re: [PATCH v2] usb: gadget: configfs: Fix use-after-free issue with udc_name
Hi, Macpaul Lin writes: > From: Eddie Hung > > There is a use-after-free issue, if access udc_name > in function gadget_dev_desc_UDC_store after another context > free udc_name in function unregister_gadget. > > Context 1: > gadget_dev_desc_UDC_store()->unregister_gadget()-> > free udc_name->set udc_name to NULL > > Context 2: > gadget_dev_desc_UDC_show()-> access udc_name > > Call trace: > dump_backtrace+0x0/0x340 > show_stack+0x14/0x1c > dump_stack+0xe4/0x134 > print_address_description+0x78/0x478 > __kasan_report+0x270/0x2ec > kasan_report+0x10/0x18 > __asan_report_load1_noabort+0x18/0x20 > string+0xf4/0x138 > vsnprintf+0x428/0x14d0 > sprintf+0xe4/0x12c > gadget_dev_desc_UDC_show+0x54/0x64 > configfs_read_file+0x210/0x3a0 > __vfs_read+0xf0/0x49c > vfs_read+0x130/0x2b4 > SyS_read+0x114/0x208 > el0_svc_naked+0x34/0x38 > > Add mutex_lock to protect this kind of scenario. > > Signed-off-by: Eddie Hung > Signed-off-by: Macpaul Lin > Reviewed-by: Peter Chen > Cc: sta...@vger.kernel.org patch doesn't apply: $ patch -p1 --dry-run /usr/bin/patch: Only garbage was found in the patch input. Please resend using git send-email and make sure your smtp server sends it as plain text, not base64. -- balbi signature.asc Description: PGP signature
Re: [PATCH 2/3] usb: dwc3: ulpi: Replace CPU-based busyloop with Protocol-based one
Hi,
Serge Semin writes:
> Originally the procedure of the ULPI transaction finish detection has been
> developed as a simple busy-loop with just decrementing counter and no
> delays. It's wrong since on different systems the loop will take a
> different time to complete. So if the system bus and CPU are fast enough
> to overtake the ULPI bus and the companion PHY reaction, then we'll get to
> take a false timeout error. Fix this by converting the busy-loop procedure
> to take the standard bus speed, address value and the registers access
> mode into account for the busy-loop delay calculation.
>
> Here is the way the fix works. It's known that the ULPI bus is clocked
> with 60MHz signal. In accordance with [1] the ULPI bus protocol is created
> so to spend 5 and 6 clock periods for immediate register write and read
> operations respectively, and 6 and 7 clock periods - for the extended
> register writes and reads. Based on that we can easily pre-calculate the
> time which will be needed for the controller to perform a requested IO
> operation. Note we'll still preserve the attempts counter in case if the
> DWC USB3 controller has got some internals delays.
>
> [1] UTMI+ Low Pin Interface (ULPI) Specification, Revision 1.1,
> October 20, 2004, pp. 30 - 36.
>
> Fixes: 88bc9d194ff6 ("usb: dwc3: add ULPI interface support")
> Signed-off-by: Serge Semin
> ---
> drivers/usb/dwc3/ulpi.c | 18 +++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/dwc3/ulpi.c b/drivers/usb/dwc3/ulpi.c
> index 20f5d9aba317..0dbc826355a5 100644
> --- a/drivers/usb/dwc3/ulpi.c
> +++ b/drivers/usb/dwc3/ulpi.c
> @@ -7,6 +7,8 @@
> * Author: Heikki Krogerus
> */
>
> +#include
> +#include
> #include
>
> #include "core.h"
> @@ -17,12 +19,22 @@
> DWC3_GUSB2PHYACC_ADDR(ULPI_ACCESS_EXTENDED) | \
> DWC3_GUSB2PHYACC_EXTEND_ADDR(a) : DWC3_GUSB2PHYACC_ADDR(a))
>
> -static int dwc3_ulpi_busyloop(struct dwc3 *dwc)
> +#define DWC3_ULPI_BASE_DELAY DIV_ROUND_UP(NSEC_PER_SEC, 6000L)
> +
> +static int dwc3_ulpi_busyloop(struct dwc3 *dwc, u8 addr, bool read)
> {
> + unsigned long ns = 5L * DWC3_ULPI_BASE_DELAY;
> unsigned count = 1000;
> u32 reg;
>
> + if (addr >= ULPI_EXT_VENDOR_SPECIFIC)
> + ns += DWC3_ULPI_BASE_DELAY;
> +
> + if (read)
> + ns += DWC3_ULPI_BASE_DELAY;
> +
> while (count--) {
> + ndelay(ns);
could we allow for a sleep here instead of a delay? Also, I wonder if
you need to make this so complex or should we just take the larger
access time of 7 clock cycles.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH 1/3] usb: dwc3: ulpi: Use VStsDone to detect PHY regs access completion
Hi,
Serge Semin writes:
> In accordance with [1] the DWC_usb3 core sets the GUSB2PHYACCn.VStsDone
> bit when the PHY vendor control access is done and clears it when the
> application initiates a new transaction. The doc doesn't say anything
> about the GUSB2PHYACCn.VStsBsy flag serving for the same purpose. Moreover
> we've discovered that the VStsBsy flag can be cleared before the VStsDone
> bit. So using the former as a signal of the PHY control registers
> completion might be dangerous. Let's have the VStsDone flag utilized
> instead then.
>
> [1] Synopsys DesignWare Cores SuperSpeed USB 3.0 xHCI Host Controller
> Databook, 2.70a, December 2013, p.388
>
> Signed-off-by: Serge Semin
> ---
> drivers/usb/dwc3/core.h | 1 +
> drivers/usb/dwc3/ulpi.c | 2 +-
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 2f04b3e42bf1..8d5e5bba1bc2 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -284,6 +284,7 @@
>
> /* Global USB2 PHY Vendor Control Register */
> #define DWC3_GUSB2PHYACC_NEWREGREQ BIT(25)
> +#define DWC3_GUSB2PHYACC_DONEBIT(24)
> #define DWC3_GUSB2PHYACC_BUSYBIT(23)
> #define DWC3_GUSB2PHYACC_WRITE BIT(22)
> #define DWC3_GUSB2PHYACC_ADDR(n) (n << 16)
> diff --git a/drivers/usb/dwc3/ulpi.c b/drivers/usb/dwc3/ulpi.c
> index e6e6176386a4..20f5d9aba317 100644
> --- a/drivers/usb/dwc3/ulpi.c
> +++ b/drivers/usb/dwc3/ulpi.c
> @@ -24,7 +24,7 @@ static int dwc3_ulpi_busyloop(struct dwc3 *dwc)
>
> while (count--) {
> reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYACC(0));
> - if (!(reg & DWC3_GUSB2PHYACC_BUSY))
> + if (reg & DWC3_GUSB2PHYACC_DONE)
are you sure this works in all supported versions of the core?
John, could you confirm this for us?
thanks
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH 0/3] usb: dwc3: ulpi: Fix UPLI registers read/write ops
Serge Semin writes:
> Our Baikal-T1 SoC is equipped with DWC USB3 IP core as a USB2.0 bus
> controller. In general the DWC USB3 driver is working well for it except
> the ULPI-bus part. We've found out that the DWC USB3 ULPI-bus driver detected
> PHY with VID:PID tuple as 0x:0x, which of course wasn't true since
> it was supposed to be 0x0424:0x0006. After a short digging inside the
> ulpi.c code and studying the DWC USB3 documentation, it has been
> discovered that the ULPI bus IO ops didn't work quite correct. The
> busy-loop had stopped waiting before the actual operation was finished. We
> found out that the problem was caused by several bugs hidden in the DWC
> USB3 ULPI-bus IO implementation.
>
> First of all in accordance with the DWC USB3 databook [1] the ULPI IO
> busy-loop is supposed to use the GUSB2PHYACCn.VStsDone flag as an
> indication of the PHY vendor control access completion. Instead it polled
> the GUSB2PHYACCn.VStsBsy flag, which as we discovered can be cleared a
> bit before the VStsDone flag.
>
> Secondly having the simple counter-based loop in the modern kernel is
> really a weak design of the busy-looping pattern especially seeing the
> ULPI operations delay can be easily estimated [2], since the bus clock is
> fixed to 60MHz.
>
> Finally the root cause of the denoted in the prologue problem was due to
> the Suspend PHY DWC USB3 feature perception. The commit e0082698b689
> ("usb: dwc3: ulpi: conditionally resume ULPI PHY") introduced the Suspend
> USB2.0 HS/FS/LS PHY regression as the Low-power consumption mode would be
> disable after a first attempt to read/write from the ULPI PHY control
> registers, and still didn't fix the problem it was originally intended for
> since the very first attempt of the ULPI PHY control registers IO would
> need much more time than the busy-loop provided. So instead of disabling
> the Suspend USB2.0 HS/FS/LS PHY feature we suggest to just extend the
> busy-loop delay in case if the GUSB2PHYCFGn.SusPHY flag set to 1. By doing
> so we'll eliminate the regression and the fix the false busy-loop timeout
> problem.
>
> [1] Synopsys DesignWare Cores SuperSpeed USB 3.0 xHCI Host Controller
> Databook, 2.70a, December 2013, p.388
>
> [1] UTMI+ Low Pin Interface (ULPI) Specification, Revision 1.1,
> October 20, 2004, pp. 30 - 36.
>
> Fixes: e0082698b689 ("usb: dwc3: ulpi: conditionally resume ULPI PHY")
> Fixes: 88bc9d194ff6 ("usb: dwc3: add ULPI interface support")
> Signed-off-by: Serge Semin
> Cc: Alexey Malahov
> Cc: Pavel Parkhomenko
> Cc: linux-...@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
these should go in the relevant commits instead.
> Serge Semin (3):
> usb: dwc3: ulpi: Use VStsDone to detect PHY regs access completion
> usb: dwc3: ulpi: Replace CPU-based busyloop with Protocol-based one
> usb: dwc3: ulpi: Fix USB2.0 HS/FS/LS PHY suspend regression
make sure fixes don't depend on other rework otherwise they can't be
taken during the -rc cycle.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH v1] usb: dwc3: core: fix a issue about clear connect state
Hi,
Dejin Zheng writes:
> According to Synopsys Programming Guide chapter 2.2 Register Resets,
> it cannot reset the DCTL register by set DCTL.CSFTRST for Core Soft Reset,
> if DWC3 controller as a slave device and stay connected with a usb host,
> then, reboot linux, it will fail to reinitialize dwc3 as a slave device
> when the DWC3 controller did not power off. because the connection status
> is incorrect, so we also need clear DCTL.RUN_STOP bit for disable connect
> when do core soft reset.
>
> Fixes: f59dcab176293b6 ("usb: dwc3: core: improve reset sequence")
> Signed-off-by: Dejin Zheng
> ---
> drivers/usb/dwc3/core.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 2eb34c8b4065..239636c454c2 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -256,6 +256,7 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc)
>
> reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> reg |= DWC3_DCTL_CSFTRST;
> + reg &= ~DWC3_DCTL_RUN_STOP;
as I mentioned in the other thread, I would rather figure out why we're
getting to probe() with RUN_STOP already set.
--
balbi
Re: [PATCH v3] usb: dwc3: core: fix a issue about clear connect state
Hi, Dejin Zheng writes: > According to Synopsys Programming Guide chapter 2.2 Register Resets, > it cannot reset the DCTL register by setting DCTL.CSFTRST for core soft > reset, if DWC3 controller as a slave device and stay connected with a usb > host, then, while rebooting linux, it will fail to reinitialize dwc3 as a > slave device when the DWC3 controller did not power off. because the > connection status is incorrect, so we also need to clear DCTL.RUN_STOP > bit for disabling connect when doing core soft reset. There will still > be other stale configuration in DCTL, so reset the other fields of DCTL > to the default value 0. This commit log is a bit hard to understand. When does this problem actually happen? It seems like it's in the case of, perhaps, kexecing into a new kernel, is that right? At the time dwc3_core_soft_reset() is called, the assumption is that we're starting with a clean core, from power up. If we have stale configuration from a previous run, we should fix this on the exit path. Note that if we're reaching probe with pull up connected, we already have issues elsewhere. I think this is not the right fix for the problem. -- balbi signature.asc Description: PGP signature
Re: [PATCH v3 31/56] usb: dwc3: fix kernel-doc markups
Hi Mauro, Mauro Carvalho Chehab writes: > There is a common comment marked, instead, with kernel-doc > notation. > > Also, some identifiers have different names between their > prototypes and the kernel-doc markup. > > Signed-off-by: Mauro Carvalho Chehab > --- > drivers/usb/dwc3/core.c| 2 +- > drivers/usb/dwc3/core.h| 2 +- > drivers/usb/gadget/composite.c | 2 +- > drivers/usb/typec/mux.c| 2 +- > include/linux/usb/composite.h | 2 +- mind breaking this into 4 commits? One for dwc3, one for gadget/composite, one for type/mux, and a final for composite.h. -- balbi signature.asc Description: PGP signature
Re: [PATCH v2] usb: dwc3: Trigger a GCTL soft reset when switching modes in DRD
Hi, John Stultz writes: > On Thu, Oct 22, 2020 at 12:55 AM Felipe Balbi wrote: >> John Stultz writes: >> > From: Yu Chen >> > >> > With the current dwc3 code on the HiKey960 we often see the >> > COREIDLE flag get stuck off in __dwc3_gadget_start(), which >> > seems to prevent the reset irq and causes the USB gadget to >> > fail to initialize. >> > >> > We had seen occasional initialization failures with older >> > kernels but with recent 5.x era kernels it seemed to be becoming >> > much more common, so I dug back through some older trees and >> > realized I dropped this quirk from Yu Chen during upstreaming >> > as I couldn't provide a proper rational for it and it didn't >> > seem to be necessary. I now realize I was wrong. >> >> This keeps coming back every few years. It has never been necessary so >> far. Why is it necessary now? > > Sorry, I'm not totally sure I've got all the context here. If you mean > with regards to the HiKey960, it's because the HiKey960 had a somewhat it's a general DWC3 thing. The databook claims that a soft reset is necessary, but it turns out it isn't :-) > complicated vendor patch stack that others and I had been carrying > along and trying to upstream slowly over the last few years. Since > that process of upstreaming required lots of rework, the patch set > changed over time fixing a number of issues and in this case (by > dropping the quirk) introducing others. > > The usb functionality on the board was never perfect. As I said in > the patch, we saw initialization issues *very* rarely with older > kernels - which I suspected was due to the oddball mux/hub driver that > had to be deeply reworked - so the issue was easy to overlook, except > the frequency of it had grown to be quite noticeable. So now that all > but the dts bits are upstream, I've been trying to spend occasional > free cycles figuring out what's wrong. > > That's when I figured out it was the quirk fix I dropped. But the > good news is so far with it I've not hit any initialization issues > (over a few hundred reboots). That's good :-) >> The only thing we need to do is verify >> which registers are shadowed between host and peripheral roles and cache >> only those registers. > > Sorry, could you explain this a bit more? Again, I don't have access > to the hardware docs, so I'm just working with the source and any > vendor patches I can find. Right, initialize it in gadget mode, then take a register dump (I think our regdump facility in dwc3's debugfs is enough). Then flip to host mode and take the same register dump. Now diff them. You'll see that some registers get overwritten. The reason for that is that physically some host and peripheral registers map to the same block of memory in the IP. In other words, the address decoder in the Register File decodes some addresses to the same physical block of memory. This was done, I believe, to save die area by reducing gate count. >> A full soft reset will take a while and is likely to create other >> issues. > > I'm also fine with going back to the quirk approach if you think that > would be lower risk to other devices? I think the soft reset can have unexpected side effects here. -- balbi signature.asc Description: PGP signature
Re: [PATCH v2] usb: dwc3: Trigger a GCTL soft reset when switching modes in DRD
Hi,
Thinh Nguyen writes:
> John Stultz wrote:
>> static void __dwc3_set_mode(struct work_struct *work)
>> {
>> struct dwc3 *dwc = work_to_dwc(work);
>> unsigned long flags;
>> +int hw_mode;
>> int ret;
>> u32 reg;
>>
>> @@ -154,6 +168,11 @@ static void __dwc3_set_mode(struct work_struct *work)
>> break;
>> }
>>
>> +/* Execute a GCTL Core Soft Reset when switch mode in DRD*/
>> +hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
>> +if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD)
>> +dwc3_gctl_core_soft_reset(dwc);
>> +
>
> I think this should be done inside the spin_lock.
>
>> spin_lock_irqsave(>lock, flags);
>>
>> dwc3_set_prtcap(dwc, dwc->desired_dr_role);
>
> The DRD mode change sequence should be like this if we want to switch
> from host -> device according to the programming guide (for all DRD IPs):
> 1. Reset controller with GCTL.CoreSoftReset
> 2. Set GCTL.PrtCapDir(device)
> 3. Soft reset with DCTL.CSftRst
> 4. Then follow up with the initializing registers sequence
>
> However, from code review, with this patch, it follows this sequence:
> a. Soft reset with DCTL.CSftRst on driver probe
> b. Reset controller with GCTL.CoreSoftReset
> c. Set GCTL.PrtCapDir(device)
> d. < missing DCTL.CSftRst >
> e. Then follow up with initializing registers sequence
>
> It may work, but it doesn't follow the programming guide.
>
> For device -> host, it should be fine because the xHCI driver will do
> USBCMD.HCRST during initialization.
The only reason why this is needed is because SNPS saves some die area
by mapping some host and peripheral register to the same physical area
in the die. I still think a full soft reset is unnecessary as we have
been running this driver without that soft reset for several years now.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH v2] usb: dwc3: Trigger a GCTL soft reset when switching modes in DRD
Hi, John Stultz writes: > From: Yu Chen > > With the current dwc3 code on the HiKey960 we often see the > COREIDLE flag get stuck off in __dwc3_gadget_start(), which > seems to prevent the reset irq and causes the USB gadget to > fail to initialize. > > We had seen occasional initialization failures with older > kernels but with recent 5.x era kernels it seemed to be becoming > much more common, so I dug back through some older trees and > realized I dropped this quirk from Yu Chen during upstreaming > as I couldn't provide a proper rational for it and it didn't > seem to be necessary. I now realize I was wrong. This keeps coming back every few years. It has never been necessary so far. Why is it necessary now? The only thing we need to do is verify which registers are shadowed between host and peripheral roles and cache only those registers. A full soft reset will take a while and is likely to create other issues. -- balbi signature.asc Description: PGP signature
Re: [PATCH 01/29] usb: dwc3: Discard synopsys,dwc3 compatibility string
Hi,
Serge Semin writes:
> Syonpsys IP cores are supposed to be defined with "snps" vendor-prefix.
> Discard a DW USB3 compatible string with the deprecated prefix seeing
> one isn't used by any dts file anymore.
>
> Signed-off-by: Serge Semin
> ---
> drivers/usb/dwc3/core.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 2eb34c8b4065..28440250e798 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1878,9 +1878,6 @@ static const struct of_device_id of_dwc3_match[] = {
> {
> .compatible = "snps,dwc3"
> },
> - {
> - .compatible = "synopsys,dwc3"
> - },
> { },
> };
> MODULE_DEVICE_TABLE(of, of_dwc3_match);
sorry, no. You can't guarantee that there isn't a FW in ROM somewhere
using the old string.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH 20/20] arch: dts: Fix DWC USB3 DT nodes name
Serge Semin writes: > On Wed, Oct 14, 2020 at 05:09:37PM +0300, Felipe Balbi wrote: >> >> Hi Serge, >> >> Serge Semin writes: >> > In accordance with the DWC USB3 bindings the corresponding node name is >> > suppose to comply with Generic USB HCD DT schema, which requires the USB >> > >> DWC3 is not a simple HDC, though. > > Yeah, strictly speaking it is equipped with a lot of vendor-specific stuff, > which are tuned by the DWC USB3 driver in the kernel. But after that the > controller is registered as xhci-hcd device so it's serviced by the xHCI > driver, in Dual-role or host-only builds, that's correct. We can also have peripheral-only builds (both SW or HW versions) which means xhci isn't even in the picture. -- balbi signature.asc Description: PGP signature
Re: [PATCH 20/20] arch: dts: Fix DWC USB3 DT nodes name
Hi Serge,
Serge Semin writes:
> In accordance with the DWC USB3 bindings the corresponding node name is
> suppose to comply with Generic USB HCD DT schema, which requires the USB
DWC3 is not a simple HDC, though.
> nodes to have the name acceptable by the regexp: "^usb(@.*)?" . But a lot
> of the DWC USB3-compatible nodes defined in the ARM/ARM64 DTS files have
> name as "^dwc3@.*" or "^usb[1-3]@.*" or even "^dwusb@.*", which will cause
> the dtbs_check procedure failure. Let's fix the nodes naming to be
> compatible with the DWC USB3 DT schema to make dtbs_check happy.
>
> Note we don't change the DWC USB3-compatible nodes names of
> arch/arm64/boot/dts/apm/{apm-storm.dtsi,apm-shadowcat.dtsi} since the
> in-source comment says that the nodes name need to be preserved as
> "^dwusb@.*" for some backward compatibility.
interesting, compatibility with what? Some debugfs files, perhaps? :-)
In any case, I don't have any problems with this, so I'll let other
folks comment.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH 1/3] ANDROID: usb: gadget: f_accessory: Add Android Accessory function
Hi,
rickyniu writes:
> From: Benoit Goby
missing Signed-off-by for author
> USB accessory mode allows users to connect USB host hardware
> specifically designed for Android-powered devices. The accessories
> must adhere to the Android accessory protocol outlined in the
> http://accessories.android.com documentation. This allows
> Android devices that cannot act as a USB host to still interact with
> USB hardware. When an Android device is in USB accessory mode, the
> attached Android USB accessory acts as the host, provides power
> to the USB bus, and enumerates connected devices.
The protocol is still a HID protocol, is it? Why couldn't you use f_hid.c?
> Signed-off-by: Mike Lockwood
> [AmitP: Folded following android-4.9 commit changes into this patch
> ceb2f0aac624 ("ANDROID: usb: gadget: accessory: Fix section mismatch")
> Parts of e27543931009 ("ANDROID: usb: gadget: Fixes and hacks to make
> android usb gadget compile on 3.8")
> 1b07ec751563 ("ANDROID: drivers: usb: gadget: 64-bit related type
> fixes")]
> Signed-off-by: Amit Pundir
> [astrachan: Folded the following changes into this patch:
> 9d5891d516e2 ("ANDROID: usb: gadget: f_accessory: Add
> ACCESSORY_SET_AUDIO_MODE control request and ioctl")
> dc66cfce9622 ("ANDROID: usb: gadget: f_accessory: Add support for
> HID input devices")
> 5f1ac9c2871b ("ANDROID: usb: gadget: f_accessory: move userspace
> interface to uapi")
> 9a6241722cd8 ("ANDROID: usb: gadget: f_accessory: Enabled Zero
> Length Packet (ZLP) for acc_write")
> 31a0ecd5a825 ("ANDROID: usb: gadget: f_accessory: check for
> accessory device before disconnecting HIDs")
> 580721fa6cbc ("ANDROID: usb: gadget: f_accessory: Migrate to
> USB_FUNCTION API")
> 7f407172fb28 ("ANDROID: usb: gadget: f_accessory: Fix for
> UsbAccessory clean unbind.")
> ebc98ac5a22f ("ANDROID: usb: gadget: f_accessory: fix false
> disconnect due to a signal sent to the reading process")
> 71c6dc5ffdab ("ANDROID: usb: gadget: f_accessory: assign no-op
> request complete callbacks")
> 675047ee68e9 ("ANDROID: usb: gadget: f_accessory: Move gadget
> functions code")
> b2bedaa5c7df ("CHROMIUM: usb: gadget: f_accessory: add
> .raw_request callback")]
> Signed-off-by: Alistair Strachan
> Signed-off-by: rickyniu
We need an actual name here, IIRC.
> diff --git a/drivers/usb/gadget/function/f_accessory.c
> b/drivers/usb/gadget/function/f_accessory.c
> new file mode 100644
> index ..514eadee1793
> --- /dev/null
> +++ b/drivers/usb/gadget/function/f_accessory.c
> @@ -0,0 +1,1352 @@
missing SPDX license identifier comment
> +/*
> + * Gadget Function Driver for Android USB accessories
> + *
> + * Copyright (C) 2011 Google, Inc.
> + * Author: Mike Lockwood
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +/* #define DEBUG */
> +/* #define VERBOSE_DEBUG */
these shouldn't be necessary
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
>
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +
> +#define MAX_INST_NAME_LEN40
> +#define BULK_BUFFER_SIZE16384
> +#define ACC_STRING_SIZE 256
> +
> +#define PROTOCOL_VERSION2
> +
> +/* String IDs */
> +#define INTERFACE_STRING_INDEX 0
> +
> +/* number of tx and rx requests to allocate */
> +#define TX_REQ_MAX 4
> +#define RX_REQ_MAX 2
> +
> +struct acc_hid_dev {
> + struct list_headlist;
> + struct hid_device *hid;
> + struct acc_dev *dev;
> + /* accessory defined ID */
> + int id;
> + /* HID report descriptor */
> + u8 *report_desc;
> + /* length of HID report descriptor */
> + int report_desc_len;
> + /* number of bytes of report_desc we have received so far */
> + int report_desc_offset;
> +};
> +
> +struct acc_dev {
> + struct usb_function function;
> + struct usb_composite_dev *cdev;
> + spinlock_t lock;
> +
> + struct usb_ep *ep_in;
> + struct usb_ep *ep_out;
> +
> + /* online indicates state of function_set_alt & function_unbind
> + * set to 1 when we connect
> + */
wrong multi-line comment style
> + int online:1;
> +
> + /* disconnected indicates state of open & release
> + * Set to 1 when we disconnect.
> + * Not cleared until our file is
Re: [PATCH 0/3] f_accessory upstream
Hi, rickyniu writes: > Below commit is to add log and send uevent: > 0003-ANDROID-usb-f_accessory-send-uevent-for-51-52-reques.patch if you're sending something new... > Benoit Goby (1): > ANDROID: usb: gadget: f_accessory: Add Android Accessory function > > Vijayavardhan Vennapusa (1): > ANDROID: USB: f_accessory: Check dev pointer before decoding ctrl > request > > rickyniu (1): > ANDROID: usb: f_accessory: send uevent for 51,52 requests why do you send me a broken version with two fixes? Send me a single patch. Also, please correct the subject line for the patch to match what's used in the framework. Something like: usb: gadget: function: add Android Accessory function should do the trick -- balbi signature.asc Description: PGP signature
Re: [PATCH v2 0/5] usb: dwc-meson-g12a: Add support for USB on S400 board
Neil Armstrong writes: > Hi Felipe, > > Is there anything to change in this serie ? I've been waiting for Kishon's review of drivers/phy parts. I can take the rest, but without Kishon's ack, drivers/phy will be left out. -- balbi signature.asc Description: PGP signature
Re: [PATCH 2/2] dt-bindings: document a new quirk for dwc3
Mauro Carvalho Chehab writes: > Ping. > > Felipe, > > Em Thu, 17 Sep 2020 08:47:48 -0600 > Rob Herring escreveu: > >> On Thu, Sep 17, 2020 at 1:18 AM Mauro Carvalho Chehab >> wrote: >> > > >> > IMO, adding a new quirk is cleaner, and adopts the same solution >> > that it is currently used by other drivers with Designware IP. >> >> We already have a bunch of quirk properties. What's one more, sigh. So >> if that's what you want, fine. >> >> Rob > > It sounds that this is the last piece for us to have everything > needed at the drivers in order to provide upstream support for > the Hikey 970 USB hub. > > Could you please merge it? Pushed to testing/next. I'll send a pull request to Greg this week, unless something goes terribly wrong on linux-next -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: bdc: Remove duplicate error message in bdc_probe()
Hi,
Tang Bin writes:
> Hi Greg KH:
>
> 在 2020/9/27 21:45, Greg KH 写道:
>> On Sun, Sep 27, 2020 at 09:42:18PM +0800, Tang Bin wrote:
>>> In this function, we don't need dev_err() message because
>>> when something goes wrong, devm_platform_ioremap_resource()
>>> can print an error message itself, so remove the redundant
>>> one.
>>>
>>> Signed-off-by: Zhang Shengju
>>> Signed-off-by: Tang Bin
>>> ---
>>> drivers/usb/gadget/udc/bdc/bdc_core.c | 4 +---
>>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/usb/gadget/udc/bdc/bdc_core.c
>>> b/drivers/usb/gadget/udc/bdc/bdc_core.c
>>> index 02a3a7746..9454f179e 100644
>>> --- a/drivers/usb/gadget/udc/bdc/bdc_core.c
>>> +++ b/drivers/usb/gadget/udc/bdc/bdc_core.c
>>> @@ -508,10 +508,8 @@ static int bdc_probe(struct platform_device *pdev)
>>> bdc->clk = clk;
>>>
>>> bdc->regs = devm_platform_ioremap_resource(pdev, 0);
>>> - if (IS_ERR(bdc->regs)) {
>>> - dev_err(dev, "ioremap error\n");
>>> + if (IS_ERR(bdc->regs))
>>> return -ENOMEM;
>> Why not return the error given to us?
>
> Because when get ioremap failed, devm_platform_ioremap_resource() can
> print error message
>
> "dev_err(dev, "ioremap failed for resource %pR\n", res)" in it's called
> function. So I think this's place's
>
> dev_err(dev, "ioremap error\n") is redundant.
that's not what Greg point you at, though. Greg's concern is valid in
that instead of passing along the error within bdc->regs, you always
return -ENOMEM. OTW, your code should read like so:
if (IS_ERR(bdc->regs))
return PTR_ERR(bdc->regs);
--
balbi
signature.asc
Description: PGP signature
RE: [PATCH v2 1/2] dt-bindings: usb: dwc3-xilinx: Add documentation for Versal DWC3 Controller
Hi,
Manish Narani writes:
> Hi Rob/Felipe,
>
> Thanks for the review.
>
>> -Original Message-----
>> From: Felipe Balbi
>> Sent: Thursday, September 24, 2020 12:47 PM
>> To: Rob Herring ; Manish Narani
>> Cc: gre...@linuxfoundation.org; Michal Simek ;
>> p.za...@pengutronix.de; linux-...@vger.kernel.org;
>> devicet...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; linux-
>> ker...@vger.kernel.org; git
>> Subject: Re: [PATCH v2 1/2] dt-bindings: usb: dwc3-xilinx: Add
>> documentation for Versal DWC3 Controller
>>
>> Rob Herring writes:
>>
>> > On Thu, Sep 10, 2020 at 12:33:04AM +0530, Manish Narani wrote:
>> >> Add documentation for Versal DWC3 controller. Add required property
>> >> 'reg' for the same. Also add optional properties for snps,dwc3.
>> >>
>> >> Signed-off-by: Manish Narani
>> >> ---
>> >> .../devicetree/bindings/usb/dwc3-xilinx.txt | 20 +--
>> >> 1 file changed, 18 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
>> b/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
>> >> index 4aae5b2cef56..219b5780dbee 100644
>> >> --- a/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
>> >> +++ b/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
>> >> @@ -1,7 +1,8 @@
>> >> Xilinx SuperSpeed DWC3 USB SoC controller
>> >>
>> >> Required properties:
>> >> -- compatible:Should contain "xlnx,zynqmp-dwc3"
>> >> +- compatible:May contain "xlnx,zynqmp-dwc3" or "xlnx,versal-
>> dwc3"
>> >> +- reg: Base address and length of the register control block
>> >> - clocks:A list of phandles for the clocks listed in clock-names
>> >> - clock-names: Should contain the following:
>> >>"bus_clk" Master/Core clock, have to be >= 125 MHz for SS
>> >> @@ -13,12 +14,24 @@ Required child node:
>> >> A child node must exist to represent the core DWC3 IP block. The name of
>> >> the node is not important. The content of the node is defined in
>> >> dwc3.txt.
>> >>
>> >> +Optional properties for snps,dwc3:
>> >> +- dma-coherent: Enable this flag if CCI is enabled in design. Adding
>> >> this
>> >> + flag configures Global SoC bus Configuration Register and
>> >> + Xilinx USB 3.0 IP - USB coherency register to enable CCI.
>> >> +- snps,enable-hibernation: Add this flag to enable hibernation support
>> for
>> >> + peripheral mode.
>> >
>> > This belongs in the DWC3 binding. It also implies that hibernation is
>> > not supported by any other DWC3 based platform. Can't this be implied by
>> > the compatible string (in the parent)?
>
> Rob, We can move this to dwc3 bindings. If Felipe is okay with below response.
>
>>
>> hibernation support is detectable in runtime, and we've been using that.
>
> Felipe, Yes, this flag is to control the enable/disable hibernation.
> I did not see has_hibernation flag being set anywhere in the driver.
> Can we control the hibernation enable/disable through DT entry? See below:
> -
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 2eb34c8b4065..1baf44d8d566 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -769,8 +769,15 @@ static void dwc3_core_setup_global_control(struct dwc3
> *dwc)
> reg &= ~DWC3_GCTL_DSBLCLKGTNG;
> break;
> case DWC3_GHWPARAMS1_EN_PWROPT_HIB:
> - /* enable hibernation here */
> - dwc->nr_scratch =
> DWC3_GHWPARAMS4_HIBER_SCRATCHBUFS(hwparams4);
> + if (!device_property_read_bool(dwc->dev,
> + "snps,enable-hibernation")) {
> + dev_dbg(dwc->dev, "Hibernation not enabled\n");
> + } else {
> + /* enable hibernation here */
> + dwc->nr_scratch =
> + DWC3_GHWPARAMS4_HIBER_SCRATCHBUFS(hwparams4);
> + dwc->has_hibernation = 1;
> + }
I left it off because I didn't have HW to validate. Don't add a new
binding for this. Set has_hibernation to true and make sure it
works. Then send me a patch that sets has_hibernation to true whenever
DWC3_GHWPARAMS1_EN_PWROPT_HIB is valid.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH v3] usb: dwc3: Stop active transfers before halting the controller
Hi,
Alan Stern writes:
>> > Hence, the reason if there was already a pending IRQ triggered, the
>> > dwc3_gadget_disable_irq() won't ensure the IRQ is handled. We can do
>> > something like:
>> > if (!is_on)
>> >dwc3_gadget_disable_irq()
>> > synchronize_irq()
>> > spin_lock_irqsave()
>> > if(!is_on) {
>> > ...
>> >
>> > But the logic to only apply this on the pullup removal case is a little
>> > messy. Also, from my understanding, the spin_lock_irqsave() will only
>> > disable the local CPU IRQs, but not the interrupt line on the GIC, which
>> > means other CPUs can handle it, unless we explicitly set the IRQ
>> > affinity to CPUX.
>>
>> Yeah, the way I understand this can't really happen. But I'm open to
>> being educated. Maybe Alan can explain if this is really possibility?
>
> It depends on the details of the hardware, but yes, it is possible in
> general for an interrupt handler to run after you have turned off the
> device's interrupt-request line. For example:
>
> CPU A CPU B
> --- --
> Gets an IRQ from the device
> Calls handler routine spin_lock_irq
> spin_lock_irq Turns off the IRQ line
> ...spins... spin_unlock_irq
> Rest of handler runs
> spin_unlock_irq
>
> That's why we have synchronize_irq(). The usual pattern is something
> like this:
>
> spin_lock_irq(>lock);
> priv->disconnected = true;
> my_disable_irq(priv);
> spin_unlock_irq(>lock);
> synchronize_irq(priv->irq);
>
> And of course this has to be done in a context that can sleep.
>
> Does this answer your question?
It does, thank you Alan. It seems like we don't need a call to
disable_irq(), only synchronize_irq() is enough, however it should be
called with spinlocks released, not held.
Thanks
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH] usb: phy: tegra: Use IS_ERR() to check and simplify code
Thierry Reding writes: > On Thu, Sep 24, 2020 at 10:26:15AM +0300, Felipe Balbi wrote: >> Tang Bin writes: >> >> > Use IS_ERR() and PTR_ERR() instead of PTR_ERR_OR_ZERO() to >> > simplify code, avoid redundant judgements. >> > >> > Signed-off-by: Zhang Shengju >> > Signed-off-by: Tang Bin >> >> Applied for next merge window. Make sure to get this driver out of >> drivers/usb/phy and moved into drivers/phy ASAP. > > Sergei had commented on this patch with valid concerns, see here in case > you don't have his reply in your inbox: > > > http://patchwork.ozlabs.org/project/linux-tegra/patch/20200910115607.11392-1-tang...@cmss.chinamobile.com/#2526208 > > I agree with those concerns. This patch is broken because it will output > the wrong error code on failure. I don't fully agree with Sergei's point > that this patch isn't worth redoing. I do like the idiomatic error > handling better, but I think we shouldn't be breaking the error messages > like this. Sure thing, dropped for now. -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: gadget: bcm63xx_udc: fix up the error of undeclared usb_debug_root
Hi,
Chunfeng Yun writes:
> On Thu, 2020-09-24 at 10:50 +0300, Felipe Balbi wrote:
>> Chunfeng Yun writes:
>>
>> > Fix up the build error caused by undeclared usb_debug_root
>> >
>> > Cc: stable
>> > Fixes: a66ada4f241c("usb: gadget: bcm63xx_udc: create debugfs directory
>> > under usb root")
>> > Reported-by: kernel test robot
>> > Signed-off-by: Chunfeng Yun
>>
>> $ patch -p1 --dry-run p.patch
>> /usr/bin/patch: Only garbage was found in the patch input.
>>
> Please try to apply v2, https://patchwork.kernel.org/patch/11772911/
> I indeed add a line code
that worked, but the problem I have is that your patches always come
base64 encoded. Make sure they come as plain text in the future :-)
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH] usb: gadget: bcm63xx_udc: fix up the error of undeclared usb_debug_root
Chunfeng Yun writes:
> Fix up the build error caused by undeclared usb_debug_root
>
> Cc: stable
> Fixes: a66ada4f241c("usb: gadget: bcm63xx_udc: create debugfs directory under
> usb root")
> Reported-by: kernel test robot
> Signed-off-by: Chunfeng Yun
$ patch -p1 --dry-run p.patch
/usr/bin/patch: Only garbage was found in the patch input.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH v3] usb: dwc3: Stop active transfers before halting the controller
Hi,
Wesley Cheng writes:
> On 9/6/2020 11:20 PM, Felipe Balbi wrote:
>>
>> Hi,
>>
>> Wesley Cheng writes:
>>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>>> index 59f2e8c31bd1..456aa87e8778 100644
>>> --- a/drivers/usb/dwc3/ep0.c
>>> +++ b/drivers/usb/dwc3/ep0.c
>>> @@ -197,7 +197,7 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct
>>> usb_request *request,
>>> int ret;
>>>
>>> spin_lock_irqsave(>lock, flags);
>>> - if (!dep->endpoint.desc) {
>>> + if (!dep->endpoint.desc || !dwc->pullups_connected) {
>>
>> this looks odd. If we don't have pullups connected, we shouldn't have a
>> descriptor, likewise if we don't have a a description, we haven't been
>> enumerated, therefore we shouldn't have pullups connected.
>>
>> What am I missing here?
>>
>
> Hi Felipe,
>
> When we
> echo "" > /sys/kernel/config/usb_gadget/g1/UDC
>
> This triggers the usb_gadget_disconnect() routine to execute.
>
> int usb_gadget_disconnect(struct usb_gadget *gadget)
> {
> ...
> ret = gadget->ops->pullup(gadget, 0);
> if (!ret) {
> gadget->connected = 0;
> gadget->udc->driver->disconnect(gadget);
> }
>
> So it is possible that we've already disabled the pullup before running
> the disable() callbacks in the function drivers. The disable()
we used to have usage counts for those, are they gone? I think they're
still there.
> callbacks usually are the ones responsible for calling usb_ep_disable(),
> where we clear the desc field. This means there is a brief period where
> the pullups_connected = 0, but we still have valid ep desc, as it has
> not been disabled yet.
this is a valid point, though
> Also, for function drivers like mass storage, the fsg_disable() routine
> defers the actual usb_ep_disable() call to the fsg_thread, so its not
> always ensured that the disconnect() execution would result in the
> usb_ep_disable() to occur synchronously.
also a good point.
>>> @@ -1926,6 +1926,21 @@ static int dwc3_gadget_set_selfpowered(struct
>>> usb_gadget *g,
>>> return 0;
>>> }
>>>
>>> +static void dwc3_stop_active_transfers(struct dwc3 *dwc)
>>> +{
>>> + u32 epnum;
>>> +
>>> + for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
>>
>> dwc3 knows the number of endpoints available in the HW. Use dwc->num_eps
>> instead.
>>
>
> Sure, will do.
>
>>> @@ -1971,6 +1986,8 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int
>>> is_on, int suspend)
>>> return 0;
>>> }
>>>
>>> +static void __dwc3_gadget_stop(struct dwc3 *dwc);
>>> +
>>> static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>> {
>>> struct dwc3 *dwc = gadget_to_dwc(g);
>>> @@ -1994,9 +2011,37 @@ static int dwc3_gadget_pullup(struct usb_gadget *g,
>>> int is_on)
>>> }
>>> }
>>>
>>> + /*
>>> +* Synchronize and disable any further event handling while controller
>>> +* is being enabled/disabled.
>>> +*/
>>> + disable_irq(dwc->irq_gadget);
>>
>> why isn't dwc3_gadget_disable_irq() enough?
>>
>>> spin_lock_irqsave(>lock, flags);
>>
>> spin_lock_irqsave() will disable interrupts, why disable_irq() above?
>>
>
> In the discussion I had with Thinh, the concern was that with the newly
> added code to override the lpos here, if the interrupt routine
> (dwc3_check_event_buf()) runs, then it will reference the lpos for
that's running in hardirq context. All interrupts are disabled while
that runs, there's no risk of race, right?
> copying the event buffer contents to the event cache, and potentially
> process events. There is no locking in place, so it could be possible
> to have both run in parallel.
Is this academic or have you actually found a situation where this
could, indeed, happen? The spin_lock_irqsave() should be enough to
synchronize dwc3_gadget_pullup() and the interrupt handler.
> Hence, the reason if there was already a pending IRQ triggered, the
> dwc3_gadget_disable_irq() won't ensure the IRQ is handled. We can do
> something like:
> if (!is_on)
> dwc3_gadget_disable_irq()
> synchronize_irq()
> spin_lock_irqsave()
> if(!is_on) {
> ...
>
> But the logic to only apply this on the pullup removal case is a little
> messy. Also, from my understan
Re: [PATCH] usb: phy: tegra: Use IS_ERR() to check and simplify code
Tang Bin writes: > Use IS_ERR() and PTR_ERR() instead of PTR_ERR_OR_ZERO() to > simplify code, avoid redundant judgements. > > Signed-off-by: Zhang Shengju > Signed-off-by: Tang Bin Applied for next merge window. Make sure to get this driver out of drivers/usb/phy and moved into drivers/phy ASAP. -- balbi signature.asc Description: PGP signature
Re: [PATCH] MAINTAINERS: Add entry for Broadcom BDC driver
Felipe Balbi writes: > Greg KH writes: > >> On Thu, Sep 17, 2020 at 02:49:54PM +0800, Chunfeng Yun wrote: >>> On Sun, 2020-09-06 at 12:55 -0700, Florian Fainelli wrote: >>> > >>> > On 7/9/2020 8:48 PM, Florian Fainelli wrote: >>> > > The Broadcom BDC driver did not have a MAINTAINERS entry which made it >>> > > escape review from Al and myself, add an entry so the relevant mailing >>> > > lists and people are copied. >>> > > >>> > > Signed-off-by: Florian Fainelli >>> > >>> > This patch still does not seem to have been picked up (not seeing it in >>> > linux-next), can this be applied so we have an accurate maintainer >>> > information for this driver? >>> Ping >> >> Felipe should have picked this up. >> >> If not, please resend it again and I can. > > Applied scratch that, it's already in Linus' -- balbi signature.asc Description: PGP signature
Re: [PATCH] MAINTAINERS: Add entry for Broadcom BDC driver
Greg KH writes: > On Thu, Sep 17, 2020 at 02:49:54PM +0800, Chunfeng Yun wrote: >> On Sun, 2020-09-06 at 12:55 -0700, Florian Fainelli wrote: >> > >> > On 7/9/2020 8:48 PM, Florian Fainelli wrote: >> > > The Broadcom BDC driver did not have a MAINTAINERS entry which made it >> > > escape review from Al and myself, add an entry so the relevant mailing >> > > lists and people are copied. >> > > >> > > Signed-off-by: Florian Fainelli >> > >> > This patch still does not seem to have been picked up (not seeing it in >> > linux-next), can this be applied so we have an accurate maintainer >> > information for this driver? >> Ping > > Felipe should have picked this up. > > If not, please resend it again and I can. Applied -- balbi signature.asc Description: PGP signature
Re: [PATCH 1/7] usb: mtu3: convert to devm_platform_ioremap_resource_byname
Chunfeng Yun writes: > Hi Felip, > > > On Mon, 2020-09-07 at 10:42 +0300, Felipe Balbi wrote: >> Hi, >> >> Chunfeng Yun writes: >> > Use devm_platform_ioremap_resource_byname() to simplify code >> > >> > Signed-off-by: Chunfeng Yun >> >> why is it so that your patches always come base64 encoded? They look >> fine on the email client, but when I try to pipe the message to git am >> it always gives me a lot of trouble and I have to manually decode the >> body of your messages and recombine with the patch. >> >> Can you try to send your patches as actual plain text without encoding >> the body with base64? > Missed the email. > > Sorry for inconvenience! > Is only the commit message base64 encoded, or includes the codes? The entire thing :-) -- balbi signature.asc Description: PGP signature
Re: [PATCH v2 1/2] dt-bindings: usb: dwc3-xilinx: Add documentation for Versal DWC3 Controller
Rob Herring writes: > On Thu, Sep 10, 2020 at 12:33:04AM +0530, Manish Narani wrote: >> Add documentation for Versal DWC3 controller. Add required property >> 'reg' for the same. Also add optional properties for snps,dwc3. >> >> Signed-off-by: Manish Narani >> --- >> .../devicetree/bindings/usb/dwc3-xilinx.txt | 20 +-- >> 1 file changed, 18 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt >> b/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt >> index 4aae5b2cef56..219b5780dbee 100644 >> --- a/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt >> +++ b/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt >> @@ -1,7 +1,8 @@ >> Xilinx SuperSpeed DWC3 USB SoC controller >> >> Required properties: >> -- compatible: Should contain "xlnx,zynqmp-dwc3" >> +- compatible: May contain "xlnx,zynqmp-dwc3" or "xlnx,versal-dwc3" >> +- reg: Base address and length of the register control block >> - clocks: A list of phandles for the clocks listed in clock-names >> - clock-names: Should contain the following: >>"bus_clk" Master/Core clock, have to be >= 125 MHz for SS >> @@ -13,12 +14,24 @@ Required child node: >> A child node must exist to represent the core DWC3 IP block. The name of >> the node is not important. The content of the node is defined in dwc3.txt. >> >> +Optional properties for snps,dwc3: >> +- dma-coherent: Enable this flag if CCI is enabled in design. Adding >> this >> +flag configures Global SoC bus Configuration Register and >> +Xilinx USB 3.0 IP - USB coherency register to enable CCI. >> +- snps,enable-hibernation: Add this flag to enable hibernation support for >> +peripheral mode. > > This belongs in the DWC3 binding. It also implies that hibernation is > not supported by any other DWC3 based platform. Can't this be implied by > the compatible string (in the parent)? hibernation support is detectable in runtime, and we've been using that. -- balbi signature.asc Description: PGP signature
Re: [trivial PATCH] treewide: Convert switch/case fallthrough; to break;
Hi, Joe Perches writes: > drivers/usb/dwc3/core.c | 2 +- > drivers/usb/gadget/legacy/inode.c | 2 +- > drivers/usb/gadget/udc/pxa25x_udc.c | 4 ++-- > drivers/usb/phy/phy-fsl-usb.c | 2 +- for the drivers above: Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
RE: [PATCH 2/2] usb: dwc3: Add driver for Xilinx platforms
Hi,
Manish Narani writes:
>> -Original Message-
>> From: Felipe Balbi
>> Sent: Tuesday, September 1, 2020 5:45 PM
>>
>> >> > + goto err;
>> >> > + }
>> >> > +
>> >> > + ret = dwc3_xlnx_rst_assert(priv_data->apbrst);
>> >> > + if (ret < 0) {
>> >> > + dev_err(dev, "%s: %d: Failed to assert reset\n",
>> >> > + __func__, __LINE__);
>> >>
>> >> dev_err(dev, "Failed to assert APB reset\n");
>> >>
>> >> > + goto err;
>> >> > + }
>> >> > +
>> >> > + ret = phy_init(priv_data->usb3_phy);
>> >>
>> >> dwc3 core should be handling this already
>> >
>> > The USB controller used in Xilinx ZynqMP platform uses xilinx GT phy
>> > which has 4 GT lanes and can used by 4 peripherals at a time.
>>
>> At the same time or are they mutually exclusive?
>
> The lanes are mutually exclusive.
Thank you for confirming :-)
> [...]
>> >> > + if (ret < 0) {
>> >> > + dev_err(dev, "%s: %d: Failed to release reset\n",
>> >> > + __func__, __LINE__);
>> >> > + goto err;
>> >> > + }
>> >> > +
>> >> > + /* Set PIPE power present signal */
>> >> > + writel(PIPE_POWER_ON, priv_data->regs + PIPE_POWER_OFFSET);
>> >> > +
>> >> > + /* Clear PIPE CLK signal */
>> >> > + writel(PIPE_CLK_OFF, priv_data->regs + PIPE_CLK_OFFSET);
>> >>
>> >> shouldn't this be hidden under clk_enable()?
>> >
>> > Though its naming suggests something related to clock framework, it is
>> > a register in the Xilinx USB controller space which configures the
>> > PIPE clock coming from Serdes.
>>
>> PIPE clock is a clock. It just so happens that the source is the PHY
>> itself.
>
> This bit is used to choose between PIPE clock coming from SerDes
> and the Suspend Clock. When the controller is out of reset, this bit
> needs to be reset in order to make the USB controller work. This
> register is added in Xilinx USB controller register space. I will
> add more description about the same in v2.
Aha! That clarifies. It's just a clock selection from clocks that are
generated elsewhere :-) I guess a clk driver would be overkill, indeed.
Thanks for explaining. Could you add some of this information to commit
log, then?
cheers
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH v2] dwc3-of-simple: add support for Hikey 970
Hi,
Mauro Carvalho Chehab writes:
>> Mauro Carvalho Chehab writes:
>> > This binding driver is needed for Hikey 970 to work,
>> > as otherwise a Serror is produced:
>>
>> you mentioned Serror doesn't happen anymore...
>>
>> > [1.837458] SError Interrupt on CPU0, code 0xbf02 -- SError
>> > [1.837462] CPU: 0 PID: 74 Comm: kworker/0:1 Not tainted 5.8.0+ #205
>> > [1.837463] Hardware name: HiKey970 (DT)
>> > [1.837465] Workqueue: events deferred_probe_work_func
>> > [1.837467] pstate: 2005 (nzCv daif -PAN -UAO BTYPE=--)
>> > [1.837468] pc : _raw_spin_unlock_irqrestore+0x18/0x50
>> > [1.837469] lr : regmap_unlock_spinlock+0x14/0x20
>> > [1.837470] sp : 8000124dba60
>> > [1.837471] x29: 8000124dba60 x28:
>> > [1.837474] x27: 0001b7e854c8 x26: 80001204ea18
>> > [1.837476] x25: 0005 x24: 800011f918f8
>> > [1.837479] x23: 800011fbb588 x22: 0001b7e40e00
>> > [1.837481] x21: 0100 x20:
>> > [1.837483] x19: 0001b767ec00 x18: ff10c000
>> > [1.837485] x17: 0002 x16: b0740fdb9950
>> > [1.837488] x15: 8000116c1198 x14:
>> > [1.837490] x13: 0030 x12: 0101010101010101
>> > [1.837493] x11: 0020 x10: 0001bf17d130
>> > [1.837495] x9 : x8 : 0001b6938080
>> > [1.837497] x7 : x6 : 003f
>> > [1.837500] x5 : x4 :
>> > [1.837502] x3 : 80001096a880 x2 :
>> > [1.837505] x1 : 0001b7e40e00 x0 : 00010001
>> > [1.837507] Kernel panic - not syncing: Asynchronous SError
>> > Interrupt
>> > [1.837509] CPU: 0 PID: 74 Comm: kworker/0:1 Not tainted 5.8.0+ #205
>> > [1.837510] Hardware name: HiKey970 (DT)
>> > [1.837511] Workqueue: events deferred_probe_work_func
>> > [1.837513] Call trace:
>> > [1.837514] dump_backtrace+0x0/0x1e0
>> > [1.837515] show_stack+0x18/0x24
>> > [1.837516] dump_stack+0xc0/0x11c
>> > [1.837517] panic+0x15c/0x324
>> > [1.837518] nmi_panic+0x8c/0x90
>> > [1.837519] arm64_serror_panic+0x78/0x84
>> > [1.837520] do_serror+0x158/0x15c
>> > [1.837521] el1_error+0x84/0x100
>> > [1.837522] _raw_spin_unlock_irqrestore+0x18/0x50
>> > [1.837523] regmap_write+0x58/0x80
>> > [1.837524] hi3660_reset_deassert+0x28/0x34
>> > [1.837526] reset_control_deassert+0x50/0x260
>> > [1.837527] reset_control_deassert+0xf4/0x260
>> > [1.837528] dwc3_probe+0x5dc/0xe6c
>> > [1.837529] platform_drv_probe+0x54/0xb0
>> > [1.837530] really_probe+0xe0/0x490
>> > [1.837531] driver_probe_device+0xf4/0x160
>> > [1.837532] __device_attach_driver+0x8c/0x114
>> > [1.837533] bus_for_each_drv+0x78/0xcc
>> > [1.837534] __device_attach+0x108/0x1a0
>> > [1.837535] device_initial_probe+0x14/0x20
>> > [1.837537] bus_probe_device+0x98/0xa0
>> > [1.837538] deferred_probe_work_func+0x88/0xe0
>> > [1.837539] process_one_work+0x1cc/0x350
>> > [1.837540] worker_thread+0x2c0/0x470
>> > [1.837541] kthread+0x154/0x160
>> > [1.837542] ret_from_fork+0x10/0x30
>> > [1.837569] SMP: stopping secondary CPUs
>> > [1.837570] Kernel Offset: 0x1d from 0x80001000
>> > [1.837571] PHYS_OFFSET: 0x0
>> > [1.837572] CPU features: 0x240002,20882004
>> > [1.837573] Memory Limit: none
>>
>> is this splat still valid?
>
> What I tried to say, is that, if the dwc3 is described this way at the
> DT bindings:
>
>
> / {
> dwc3: dwc3@ff10 {
> compatible = "snps,dwc3";
> reg = <0x0 0xff10 0x0 0x10>;
> clocks = <_ctrl HI3670_CLK_GATE_ABB_USB>,
> <_ctrl HI3670_HCLK_GATE_USB3OTG>,
> <_ctrl HI3670_CLK_GATE_USB3OTG_REF>,
> <_ctrl HI3670_ACLK_GATE_USB3DVFS>;
> ...
>
> The panic occurs, with the logs posted at the patch.
>
> The fix is to use dwc3-of-simple to initialize the clocks earlier,
> e. g., using this binding:
>
> / {
> usb3: hisi_dwc3 {
> compatible = "hisilicon,kirin970-dwc3";
> #address-cells = <2>;
> #size-cells = <2>;
> ranges;
>
> clocks = <_ctrl HI3670_CLK_GATE_ABB_USB>,
> <_ctrl HI3670_HCLK_GATE_USB3OTG>,
> <_ctrl HI3670_CLK_GATE_USB3OTG_REF>,
> <_ctrl HI3670_ACLK_GATE_USB3DVFS>;
>
>
> dwc3: dwc3@ff10 {
> compatible = "snps,dwc3";
>
Re: [PATCH v2] dwc3-of-simple: add support for Hikey 970
Hi, Mauro Carvalho Chehab writes: > This binding driver is needed for Hikey 970 to work, > as otherwise a Serror is produced: you mentioned Serror doesn't happen anymore... > [1.837458] SError Interrupt on CPU0, code 0xbf02 -- SError > [1.837462] CPU: 0 PID: 74 Comm: kworker/0:1 Not tainted 5.8.0+ #205 > [1.837463] Hardware name: HiKey970 (DT) > [1.837465] Workqueue: events deferred_probe_work_func > [1.837467] pstate: 2005 (nzCv daif -PAN -UAO BTYPE=--) > [1.837468] pc : _raw_spin_unlock_irqrestore+0x18/0x50 > [1.837469] lr : regmap_unlock_spinlock+0x14/0x20 > [1.837470] sp : 8000124dba60 > [1.837471] x29: 8000124dba60 x28: > [1.837474] x27: 0001b7e854c8 x26: 80001204ea18 > [1.837476] x25: 0005 x24: 800011f918f8 > [1.837479] x23: 800011fbb588 x22: 0001b7e40e00 > [1.837481] x21: 0100 x20: > [1.837483] x19: 0001b767ec00 x18: ff10c000 > [1.837485] x17: 0002 x16: b0740fdb9950 > [1.837488] x15: 8000116c1198 x14: > [1.837490] x13: 0030 x12: 0101010101010101 > [1.837493] x11: 0020 x10: 0001bf17d130 > [1.837495] x9 : x8 : 0001b6938080 > [1.837497] x7 : x6 : 003f > [1.837500] x5 : x4 : > [1.837502] x3 : 80001096a880 x2 : > [1.837505] x1 : 0001b7e40e00 x0 : 00010001 > [1.837507] Kernel panic - not syncing: Asynchronous SError Interrupt > [1.837509] CPU: 0 PID: 74 Comm: kworker/0:1 Not tainted 5.8.0+ #205 > [1.837510] Hardware name: HiKey970 (DT) > [1.837511] Workqueue: events deferred_probe_work_func > [1.837513] Call trace: > [1.837514] dump_backtrace+0x0/0x1e0 > [1.837515] show_stack+0x18/0x24 > [1.837516] dump_stack+0xc0/0x11c > [1.837517] panic+0x15c/0x324 > [1.837518] nmi_panic+0x8c/0x90 > [1.837519] arm64_serror_panic+0x78/0x84 > [1.837520] do_serror+0x158/0x15c > [1.837521] el1_error+0x84/0x100 > [1.837522] _raw_spin_unlock_irqrestore+0x18/0x50 > [1.837523] regmap_write+0x58/0x80 > [1.837524] hi3660_reset_deassert+0x28/0x34 > [1.837526] reset_control_deassert+0x50/0x260 > [1.837527] reset_control_deassert+0xf4/0x260 > [1.837528] dwc3_probe+0x5dc/0xe6c > [1.837529] platform_drv_probe+0x54/0xb0 > [1.837530] really_probe+0xe0/0x490 > [1.837531] driver_probe_device+0xf4/0x160 > [1.837532] __device_attach_driver+0x8c/0x114 > [1.837533] bus_for_each_drv+0x78/0xcc > [1.837534] __device_attach+0x108/0x1a0 > [1.837535] device_initial_probe+0x14/0x20 > [1.837537] bus_probe_device+0x98/0xa0 > [1.837538] deferred_probe_work_func+0x88/0xe0 > [1.837539] process_one_work+0x1cc/0x350 > [1.837540] worker_thread+0x2c0/0x470 > [1.837541] kthread+0x154/0x160 > [1.837542] ret_from_fork+0x10/0x30 > [1.837569] SMP: stopping secondary CPUs > [1.837570] Kernel Offset: 0x1d from 0x80001000 > [1.837571] PHYS_OFFSET: 0x0 > [1.837572] CPU features: 0x240002,20882004 > [1.837573] Memory Limit: none is this splat still valid? I can edit commit while applying, just let me know if I should remove this. -- balbi signature.asc Description: PGP signature
Re: [PATCH v2 10/11] dwc3-of-simple: add support for Hikey 970
Mauro Carvalho Chehab writes: > Em Mon, 7 Sep 2020 17:59:34 +0200 > Mauro Carvalho Chehab escreveu: > >> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c >> b/drivers/usb/dwc3/dwc3-of-simple.c >> index 8852fbfdead4..2d497165efe2 100644 >> --- a/drivers/usb/dwc3/dwc3-of-simple.c >> +++ b/drivers/usb/dwc3/dwc3-of-simple.c >> @@ -49,7 +49,8 @@ static int dwc3_of_simple_probe(struct platform_device >> *pdev) >> * Some controllers need to toggle the usb3-otg reset before trying to >> * initialize the PHY, otherwise the PHY times out. >> */ >> -if (of_device_is_compatible(np, "rockchip,rk3399-dwc3")) >> +if (of_device_is_compatible(np, "rockchip,rk3399-dwc3") || >> +of_device_is_compatible(np, "hisilicon,hi3670-dwc3")) >> simple->need_reset = true; > > It turns that this hunk caused Serror during my suspend/resume tests. > > Without this one, the driver works just fine. > > As you already applied this patch, do you prefer a patch dropping it, > or should I send a version 2 without it? Send me a new one, I'll remove the patch. -- balbi signature.asc Description: PGP signature
Re: [PATCH v2 1/3] dt-bindings: usb: Convert cdns-usb3.txt to YAML schema
Hi, Roger Quadros writes: > Converts cdns-usb3.txt to YAML schema cdns,usb3.yaml > > Signed-off-by: Roger Quadros > --- > .../devicetree/bindings/usb/cdns,usb3.yaml| 89 +++ > .../devicetree/bindings/usb/cdns-usb3.txt | 45 -- Rob, should I wait for your Ack on yaml conversions? -- balbi signature.asc Description: PGP signature
Re: [PATCH v6 04/13] usb: dwc3: Add splitdisable quirk for Hisilicon Kirin Soc
Hi,
Mauro Carvalho Chehab writes:
>> > I tested here, together with the Hikey 970 phy RFC patches I sent
>> > last week.
>> >
>> > Without this patch, the USB HID driver receives -EPROTO from
>> > submitted URBs, causing it to enter into an endless reset cycle
>> > on every 500 ms, at the hid_io_error() logic.
>>
>> > Tested-by: Mauro Carvalho Chehab
>> >
>> > If you prefer, I can re-submit this one with my SOB.
>>
>> Please do, but since you're changing device tree, I need Rob's acked-by.
>
> Ok, I'll do that.
thanks
>> > Em Sat, 20 Apr 2019 14:40:10 +0800
>> > Yu Chen escreveu:
>> >
>> >> SPLIT_BOUNDARY_DISABLE should be set for DesignWare USB3 DRD Core
>> >> of Hisilicon Kirin Soc when dwc3 core act as host.
>>
>> is this Kirin-specific or is this something that we should do a revision
>> check?
>
> I've no idea. I don't have any datasheets from this device.
I see
>> Why does it affect only Hikey kirin?
>
> As John Stultz didn't re-submit this one (and looking at the DT
> between Kirin 960 and 970 from the original Kernel 4.9 official
> drivers), I suspect that only Kirin 970 requires this quirk.
>
> It could well be due to some Dwc3 revision, but it could also be due
> to some differences at the USB part of the SoC, as there are a
the reason I ask is that if it's caused by dwc3 revision, then we don't
need the extra dt property, we can rely on a revision check. If it's
something that can't be detected in runtime, then we need a property.
> few other things different between hikey 960 and 970: it has a
> different PHY driver, and there are also some differences at the
> USB HUB which is connected into it.
>
> On both devices, the USB physical ports are actually connected
> into a HUB. In the case of Hikey 970, the hub seems to be a
> TI TUSB8041 4-Port Hub:
>
> $ lsusb
> Bus 002 Device 002: ID 0451:8140 Texas Instruments, Inc. TUSB8041
> 4-Port Hub
> Bus 002 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
> Bus 001 Device 004: ID 090c:1000 Silicon Motion, Inc. - Taiwan
> (formerly Feiya Technology Corp.) Flash Drive
> Bus 001 Device 003: ID 413c:301a Dell Computer Corp. Dell MS116 Optical
> Mouse
> Bus 001 Device 002: ID 0451:8142 Texas Instruments, Inc. TUSB8041
> 4-Port Hub
> Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub
>
>> What's the dwc3 revision on
>> that SoC (grep SNPSID /sys/kernel/debugfs/*dwc3/regdump)?
>
> GSNPSID = 0x33313130
This isn't even listed as a known revision in dwc3/core.h. Thinh, could
the issue being described here caused by a known Erratum with this
particular revision?
>> >> + reg = dwc3_readl(dwc->regs, DWC3_GUCTL3);
>> >> + reg |= DWC3_GUCTL3_SPLITDISABLE;
>> >> + dwc3_writel(dwc->regs, DWC3_GUCTL3, reg);
>> >> + }
>> >> +}
>> >> +#else
>> >> +#define dwc3_complete NULL
>> >> #endif /* CONFIG_PM_SLEEP */
>> >>
>> >> static const struct dev_pm_ops dwc3_dev_pm_ops = {
>> >> SET_SYSTEM_SLEEP_PM_OPS(dwc3_suspend, dwc3_resume)
>> >> + .complete = dwc3_complete,
>>
>> why is this done on complete? Why can't it be done at the end of
>> dwc3_resume()?
>
> Again, no idea. I didn't actually tried to suspend/resume.
>
> Maybe the original author can shed a light on it.
yeah, would be nice :-)
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH v2 10/11] dwc3-of-simple: add support for Hikey 970
Hi, Mauro Carvalho Chehab writes: > This binding driver is needed for Hikey 970 to work, > as otherwise a Serror is produced: > > [1.837458] SError Interrupt on CPU0, code 0xbf02 -- SError > [1.837462] CPU: 0 PID: 74 Comm: kworker/0:1 Not tainted 5.8.0+ #205 > [1.837463] Hardware name: HiKey970 (DT) > [1.837465] Workqueue: events deferred_probe_work_func > [1.837467] pstate: 2005 (nzCv daif -PAN -UAO BTYPE=--) > [1.837468] pc : _raw_spin_unlock_irqrestore+0x18/0x50 > [1.837469] lr : regmap_unlock_spinlock+0x14/0x20 > [1.837470] sp : 8000124dba60 > [1.837471] x29: 8000124dba60 x28: > [1.837474] x27: 0001b7e854c8 x26: 80001204ea18 > [1.837476] x25: 0005 x24: 800011f918f8 > [1.837479] x23: 800011fbb588 x22: 0001b7e40e00 > [1.837481] x21: 0100 x20: > [1.837483] x19: 0001b767ec00 x18: ff10c000 > [1.837485] x17: 0002 x16: b0740fdb9950 > [1.837488] x15: 8000116c1198 x14: > [1.837490] x13: 0030 x12: 0101010101010101 > [1.837493] x11: 0020 x10: 0001bf17d130 > [1.837495] x9 : x8 : 0001b6938080 > [1.837497] x7 : x6 : 003f > [1.837500] x5 : x4 : > [1.837502] x3 : 80001096a880 x2 : > [1.837505] x1 : 0001b7e40e00 x0 : 00010001 > [1.837507] Kernel panic - not syncing: Asynchronous SError Interrupt > [1.837509] CPU: 0 PID: 74 Comm: kworker/0:1 Not tainted 5.8.0+ #205 > [1.837510] Hardware name: HiKey970 (DT) > [1.837511] Workqueue: events deferred_probe_work_func > [1.837513] Call trace: > [1.837514] dump_backtrace+0x0/0x1e0 > [1.837515] show_stack+0x18/0x24 > [1.837516] dump_stack+0xc0/0x11c > [1.837517] panic+0x15c/0x324 > [1.837518] nmi_panic+0x8c/0x90 > [1.837519] arm64_serror_panic+0x78/0x84 > [1.837520] do_serror+0x158/0x15c > [1.837521] el1_error+0x84/0x100 > [1.837522] _raw_spin_unlock_irqrestore+0x18/0x50 > [1.837523] regmap_write+0x58/0x80 > [1.837524] hi3660_reset_deassert+0x28/0x34 > [1.837526] reset_control_deassert+0x50/0x260 > [1.837527] reset_control_deassert+0xf4/0x260 > [1.837528] dwc3_probe+0x5dc/0xe6c > [1.837529] platform_drv_probe+0x54/0xb0 > [1.837530] really_probe+0xe0/0x490 > [1.837531] driver_probe_device+0xf4/0x160 > [1.837532] __device_attach_driver+0x8c/0x114 > [1.837533] bus_for_each_drv+0x78/0xcc > [1.837534] __device_attach+0x108/0x1a0 > [1.837535] device_initial_probe+0x14/0x20 > [1.837537] bus_probe_device+0x98/0xa0 > [1.837538] deferred_probe_work_func+0x88/0xe0 > [1.837539] process_one_work+0x1cc/0x350 > [1.837540] worker_thread+0x2c0/0x470 > [1.837541] kthread+0x154/0x160 > [1.837542] ret_from_fork+0x10/0x30 > [1.837569] SMP: stopping secondary CPUs > [1.837570] Kernel Offset: 0x1d from 0x80001000 > [1.837571] PHYS_OFFSET: 0x0 > [1.837572] CPU features: 0x240002,20882004 > [1.837573] Memory Limit: none > > Signed-off-by: Mauro Carvalho Chehab applied for v5.9-rc -- balbi signature.asc Description: PGP signature
Re: [PATCH v6 04/13] usb: dwc3: Add splitdisable quirk for Hisilicon Kirin Soc
Hi Mauro,
Mauro Carvalho Chehab writes:
> Hi Felipe/Greg,
>
> What's the status of this patch?
to be frank, I don't think I have this in my inbox anymore.
> I tested here, together with the Hikey 970 phy RFC patches I sent
> last week.
>
> Without this patch, the USB HID driver receives -EPROTO from
> submitted URBs, causing it to enter into an endless reset cycle
> on every 500 ms, at the hid_io_error() logic.
> Tested-by: Mauro Carvalho Chehab
>
> If you prefer, I can re-submit this one with my SOB.
Please do, but since you're changing device tree, I need Rob's acked-by.
> Thanks,
> Mauro
>
> Em Sat, 20 Apr 2019 14:40:10 +0800
> Yu Chen escreveu:
>
>> SPLIT_BOUNDARY_DISABLE should be set for DesignWare USB3 DRD Core
>> of Hisilicon Kirin Soc when dwc3 core act as host.
is this Kirin-specific or is this something that we should do a revision
check? Why does it affect only Hikey kirin? What's the dwc3 revision on
that SoC (grep SNPSID /sys/kernel/debugfs/*dwc3/regdump)?
>> @@ -1825,10 +1834,27 @@ static int dwc3_resume(struct device *dev)
>>
>> return 0;
>> }
>> +
>> +static void dwc3_complete(struct device *dev)
>> +{
>> +struct dwc3 *dwc = dev_get_drvdata(dev);
>> +u32 reg;
>> +
>> +if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST &&
>> +dwc->dis_split_quirk) {
>> +dev_dbg(dwc->dev, "set DWC3_GUCTL3_SPLITDISABLE\n");
no more dev_dbg() should be added. This driver relies exclusively on
tracepoints for debugging.
>> +reg = dwc3_readl(dwc->regs, DWC3_GUCTL3);
>> +reg |= DWC3_GUCTL3_SPLITDISABLE;
>> +dwc3_writel(dwc->regs, DWC3_GUCTL3, reg);
>> +}
>> +}
>> +#else
>> +#define dwc3_complete NULL
>> #endif /* CONFIG_PM_SLEEP */
>>
>> static const struct dev_pm_ops dwc3_dev_pm_ops = {
>> SET_SYSTEM_SLEEP_PM_OPS(dwc3_suspend, dwc3_resume)
>> +.complete = dwc3_complete,
why is this done on complete? Why can't it be done at the end of
dwc3_resume()?
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH v3] usb: dwc3: Add support for VBUS power control
Hi, Mike Looijmans writes: > Met vriendelijke groet / kind regards, > > Mike Looijmans > System Expert > > > TOPIC Embedded Products B.V. > Materiaalweg 4, 5681 RJ Best > The Netherlands > > T: +31 (0) 499 33 69 69 > E: mike.looijm...@topicproducts.com > W: www.topicproducts.com > > Please consider the environment before printing this e-mail > On 27-07-2020 12:23, Mark Brown wrote: >> On Sun, Jul 26, 2020 at 09:10:39AM +0200, Mike Looijmans wrote: >>> On 23-07-2020 13:05, Mark Brown wrote: >> Does the device actually support running without power so that's a thing that can happen? _get_optional() should only ever be used for supplies that may be physically absent. >> >>> It's the 5V VBUS power for the USB "plug" that's being controlled here. It >>> must turned on when the controller is in "host" mode. Some boards arrange >>> this in hardware through the PHY, and some just don't have any control at >>> all and have it permanently on or off. On a board where the 5V is controlled >>> using a GPIO line or an I2C chip, this patch is required to make it work. >> >> That sounds like the driver should not be using _get_optional() then. >> > > Making it mandatory would break most (read: all except Topic's) existing > boards as they won't have it in their devicetree. I'm perfectly okay with > that, but others might disagree. you're perfectly okay with break all existing users of the driver? That's a bit harsh -- balbi signature.asc Description: PGP signature
Re: [PATCH 1/7] usb: mtu3: convert to devm_platform_ioremap_resource_byname
Hi, Chunfeng Yun writes: > Use devm_platform_ioremap_resource_byname() to simplify code > > Signed-off-by: Chunfeng Yun why is it so that your patches always come base64 encoded? They look fine on the email client, but when I try to pipe the message to git am it always gives me a lot of trouble and I have to manually decode the body of your messages and recombine with the patch. Can you try to send your patches as actual plain text without encoding the body with base64? -- balbi signature.asc Description: PGP signature
Re: [PATCH v3] usb: dwc3: Stop active transfers before halting the controller
Hi,
Wesley Cheng writes:
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 59f2e8c31bd1..456aa87e8778 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -197,7 +197,7 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct
> usb_request *request,
> int ret;
>
> spin_lock_irqsave(>lock, flags);
> - if (!dep->endpoint.desc) {
> + if (!dep->endpoint.desc || !dwc->pullups_connected) {
this looks odd. If we don't have pullups connected, we shouldn't have a
descriptor, likewise if we don't have a a description, we haven't been
enumerated, therefore we shouldn't have pullups connected.
What am I missing here?
> @@ -1926,6 +1926,21 @@ static int dwc3_gadget_set_selfpowered(struct
> usb_gadget *g,
> return 0;
> }
>
> +static void dwc3_stop_active_transfers(struct dwc3 *dwc)
> +{
> + u32 epnum;
> +
> + for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
dwc3 knows the number of endpoints available in the HW. Use dwc->num_eps
instead.
> @@ -1971,6 +1986,8 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int
> is_on, int suspend)
> return 0;
> }
>
> +static void __dwc3_gadget_stop(struct dwc3 *dwc);
> +
> static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
> {
> struct dwc3 *dwc = gadget_to_dwc(g);
> @@ -1994,9 +2011,37 @@ static int dwc3_gadget_pullup(struct usb_gadget *g,
> int is_on)
> }
> }
>
> + /*
> + * Synchronize and disable any further event handling while controller
> + * is being enabled/disabled.
> + */
> + disable_irq(dwc->irq_gadget);
why isn't dwc3_gadget_disable_irq() enough?
> spin_lock_irqsave(>lock, flags);
spin_lock_irqsave() will disable interrupts, why disable_irq() above?
> + /* Controller is not halted until pending events are acknowledged */
> + if (!is_on) {
> + u32 count;
> +
> + /*
> + * The databook explicitly mentions for a device-initiated
> + * disconnect sequence, the SW needs to ensure that it ends any
> + * active transfers.
> + */
make this a little better by mentioning the version and section of the
databook you're reading. That makes it easier for future
reference. Also, use an actual quote from the databook, along the lines
of:
/*
* Synopsys DesignWare Cores USB3 Databook Revision
* X.YYa states in section W.Z that "device-initiated
* disconnect "
*/
> + dwc3_stop_active_transfers(dwc);
> + __dwc3_gadget_stop(dwc);
> +
> + count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
> + count &= DWC3_GEVNTCOUNT_MASK;
> + if (count > 0) {
> + dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count);
> + dwc->ev_buf->lpos = (dwc->ev_buf->lpos + count) %
> + dwc->ev_buf->length;
> + }
don't duplicate code. Add a patch before this extracting this into
helper and use it for both irq handler and gadget pullup.
> + }
> +
> ret = dwc3_gadget_run_stop(dwc, is_on, false);
> spin_unlock_irqrestore(>lock, flags);
> + enable_irq(dwc->irq_gadget);
>
> return ret;
> }
> @@ -3100,6 +3145,8 @@ static void dwc3_gadget_reset_interrupt(struct dwc3
> *dwc)
> }
>
> dwc3_reset_gadget(dwc);
> + /* Stop any active/pending transfers when receiving bus reset */
unnecessary comment. We're calling a function named "stop active
transfers" from within the "bus reset handler".
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH 14/20] usb/phy: mxs-usb: Use pm_ptr() macro
Paul Cercueil writes: > Use the newly introduced pm_ptr() macro, and mark the suspend/resume > functions __maybe_unused. These functions can then be moved outside the > CONFIG_PM_SUSPEND block, and the compiler can then process them and > detect build failures independently of the config. If unused, they will > simply be discarded by the compiler. > > Signed-off-by: Paul Cercueil > --- Acked-by: Felipe Balbi -- balbi signature.asc Description: PGP signature
RE: [PATCH 2/2] usb: dwc3: Add driver for Xilinx platforms
Hi,
(remember to break your lines at 80-columns)
Manish Narani writes:
>> > + goto err;
>> > + }
>> > +
>> > + ret = dwc3_xlnx_rst_assert(priv_data->apbrst);
>> > + if (ret < 0) {
>> > + dev_err(dev, "%s: %d: Failed to assert reset\n",
>> > + __func__, __LINE__);
>>
>> dev_err(dev, "Failed to assert APB reset\n");
>>
>> > + goto err;
>> > + }
>> > +
>> > + ret = phy_init(priv_data->usb3_phy);
>>
>> dwc3 core should be handling this already
>
> The USB controller used in Xilinx ZynqMP platform uses xilinx GT phy
> which has 4 GT lanes and can used by 4 peripherals at a time.
At the same time or are they mutually exclusive?
> This USB controller uses 1 GT phy lane among the 4 GT lanes. To
> configure the GT lane for USB controller, the below sequence is
> expected.
>
> 1. Assert the USB controller resets.
> 2. Configure the Xilinx GT phy lane for USB controller (phy_init).
> 3. De-assert the USB controller resets and configure PIPE.
> 4. Wait for PLL of the GT lane used by USB to be locked
>(phy_power_on).
it seems like you need to extend the PHY framework and teach it about
lane configuration.
> The dwc3 core by default does the phy_init() and phy_power_on() but
> the default sequence doesn't work with Xilinx platforms. Because of
> this reason, we have introduced this new driver to support the new
> sequence.
Instead of teaching the relevant framework about your new requirements
;-)
>> > + if (ret < 0) {
>> > + dev_err(dev, "%s: %d: Failed to release reset\n",
>> > + __func__, __LINE__);
>> > + goto err;
>> > + }
>> > +
>> > + /* Set PIPE power present signal */
>> > + writel(PIPE_POWER_ON, priv_data->regs + PIPE_POWER_OFFSET);
>> > +
>> > + /* Clear PIPE CLK signal */
>> > + writel(PIPE_CLK_OFF, priv_data->regs + PIPE_CLK_OFFSET);
>>
>> shouldn't this be hidden under clk_enable()?
>
> Though its naming suggests something related to clock framework, it is
> a register in the Xilinx USB controller space which configures the
> PIPE clock coming from Serdes.
PIPE clock is a clock. It just so happens that the source is the PHY
itself.
>> > +static int dwc3_xlnx_resume(struct device *dev)
>> > +{
>> > + struct dwc3_xlnx *priv_data = dev_get_drvdata(dev);
>> > +
>> > + return clk_bulk_enable(priv_data->num_clocks, priv_data->clks);
>> > +}
>>
>> you have the same implementation for both types of suspend/resume. Maybe
>> extract dwc3_xlnx_{suspend,resume}_common() and just call it from both
>> callbacks?
>
> Going forward we have a plan to add Hibernation handling calls in
> dwc3_xlnx_suspend/resume functions.
at that moment and only at that moment, should you be worried about
splitting them.
> For that reason, these APIs are kept separate. If you insist, I can
> make them common for now and separate them later when I add
> hibernation code.
It would be a little better, no?
cheers
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH] usb: gadget: aspeed: fixup vhub port irq handling
Hi,
Tao Ren writes:
>> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/core.c
>> > b/drivers/usb/gadget/udc/aspeed-vhub/core.c
>> > index cdf96911e4b1..be7bb64e3594 100644
>> > --- a/drivers/usb/gadget/udc/aspeed-vhub/core.c
>> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/core.c
>> > @@ -135,13 +135,9 @@ static irqreturn_t ast_vhub_irq(int irq, void *data)
>> >
>> >/* Handle device interrupts */
>> >if (istat & vhub->port_irq_mask) {
>> > - unsigned long bitmap = istat;
>> > - int offset = VHUB_IRQ_DEV1_BIT;
>> > - int size = VHUB_IRQ_DEV1_BIT + vhub->max_ports;
>> > -
>> > - for_each_set_bit_from(offset, , size) {
>> > - i = offset - VHUB_IRQ_DEV1_BIT;
>> > - ast_vhub_dev_irq(>ports[i].dev);
>> > + for (i = 0; i < vhub->max_ports; i++) {
>> > + if (istat & VHUB_DEV_IRQ(i))
>> > + ast_vhub_dev_irq(>ports[i].dev);
>>
>> how have you measured your statement above? for_each_set_bit() does
>> exactly what you did. Unless your architecture has an instruction which
>> helps finds the next set bit (like cls on ARM), which, then, makes it
>> much faster.
>
> I did some testing and result shows for() loop runs faster than
> for_each_set_bit() loop. Please refer to details below (discussion with
> Benjamin in the original patch) and kindly let me know your
> suggestions.
no strong feelings, just surprised you're already worried about 20~40
cycles of cpu time ;-)
patch queued for next merge window
--
balbi
Re: [PATCH] usb: gadget: aspeed: fixup vhub port irq handling
Hi,
Tao Ren writes:
> On Mon, Aug 17, 2020 at 04:49:32PM +0300, Felipe Balbi wrote:
>>
>> Hi,
>>
>> rentao.b...@gmail.com writes:
>> > From: Tao Ren
>> >
>> > This is a follow-on patch for commit a23be4ed8f48 ("usb: gadget: aspeed:
>> > improve vhub port irq handling"): for_each_set_bit() is replaced with
>> > simple for() loop because for() loop runs faster on ASPEED BMC.
>> >
>> > Signed-off-by: Tao Ren
>> > ---
>> > drivers/usb/gadget/udc/aspeed-vhub/core.c | 10 +++---
>> > drivers/usb/gadget/udc/aspeed-vhub/vhub.h | 3 +++
>> > 2 files changed, 6 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/core.c
>> > b/drivers/usb/gadget/udc/aspeed-vhub/core.c
>> > index cdf96911e4b1..be7bb64e3594 100644
>> > --- a/drivers/usb/gadget/udc/aspeed-vhub/core.c
>> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/core.c
>> > @@ -135,13 +135,9 @@ static irqreturn_t ast_vhub_irq(int irq, void *data)
>> >
>> >/* Handle device interrupts */
>> >if (istat & vhub->port_irq_mask) {
>> > - unsigned long bitmap = istat;
>> > - int offset = VHUB_IRQ_DEV1_BIT;
>> > - int size = VHUB_IRQ_DEV1_BIT + vhub->max_ports;
>> > -
>> > - for_each_set_bit_from(offset, , size) {
>> > - i = offset - VHUB_IRQ_DEV1_BIT;
>> > - ast_vhub_dev_irq(>ports[i].dev);
>> > + for (i = 0; i < vhub->max_ports; i++) {
>> > + if (istat & VHUB_DEV_IRQ(i))
>> > + ast_vhub_dev_irq(>ports[i].dev);
>>
>> how have you measured your statement above? for_each_set_bit() does
>> exactly what you did. Unless your architecture has an instruction which
>> helps finds the next set bit (like cls on ARM), which, then, makes it
>> much faster.
>
> I did some testing and result shows for() loop runs faster than
> for_each_set_bit() loop. Please refer to details below (discussion with
> Benjamin in the original patch) and kindly let me know your suggestions.
no strong feelings, just surprised that you're already worried about
20~40 cycles of cpu time ;-)
Patch applied for next merge window
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH] usb: gadget: aspeed: fixup vhub port irq handling
Hi,
Tao Ren writes:
> On Mon, Aug 17, 2020 at 04:49:32PM +0300, Felipe Balbi wrote:
>>
>> Hi,
>>
>> rentao.b...@gmail.com writes:
>> > From: Tao Ren
>> >
>> > This is a follow-on patch for commit a23be4ed8f48 ("usb: gadget: aspeed:
>> > improve vhub port irq handling"): for_each_set_bit() is replaced with
>> > simple for() loop because for() loop runs faster on ASPEED BMC.
>> >
>> > Signed-off-by: Tao Ren
>> > ---
>> > drivers/usb/gadget/udc/aspeed-vhub/core.c | 10 +++---
>> > drivers/usb/gadget/udc/aspeed-vhub/vhub.h | 3 +++
>> > 2 files changed, 6 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/core.c
>> > b/drivers/usb/gadget/udc/aspeed-vhub/core.c
>> > index cdf96911e4b1..be7bb64e3594 100644
>> > --- a/drivers/usb/gadget/udc/aspeed-vhub/core.c
>> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/core.c
>> > @@ -135,13 +135,9 @@ static irqreturn_t ast_vhub_irq(int irq, void *data)
>> >
>> >/* Handle device interrupts */
>> >if (istat & vhub->port_irq_mask) {
>> > - unsigned long bitmap = istat;
>> > - int offset = VHUB_IRQ_DEV1_BIT;
>> > - int size = VHUB_IRQ_DEV1_BIT + vhub->max_ports;
>> > -
>> > - for_each_set_bit_from(offset, , size) {
>> > - i = offset - VHUB_IRQ_DEV1_BIT;
>> > - ast_vhub_dev_irq(>ports[i].dev);
>> > + for (i = 0; i < vhub->max_ports; i++) {
>> > + if (istat & VHUB_DEV_IRQ(i))
>> > + ast_vhub_dev_irq(>ports[i].dev);
>>
>> how have you measured your statement above? for_each_set_bit() does
>> exactly what you did. Unless your architecture has an instruction which
>> helps finds the next set bit (like cls on ARM), which, then, makes it
>> much faster.
>
> I did some testing and result shows for() loop runs faster than
> for_each_set_bit() loop. Please refer to details below (discussion with
> Benjamin in the original patch) and kindly let me know your suggestions.
no strong feelings, just surprised that you're already worried about
20~40 cycles of cpu time ;-)
Patch applied for next merge window
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH 1/1] USB: PHY: JZ4770: Fix static checker warning.
周琰杰 (Zhou Yanjie) writes:
> The commit 2a6c0b82e651 ("USB: PHY: JZ4770: Add support for new
> Ingenic SoCs.") introduced the initialization function for different
> chips, but left the relevant code involved in the resetting process
> in the original function, resulting in uninitialized variable calls.
>
> Fixes: 2a6c0b82e651 ("USB: PHY: JZ4770: Add support for new
> Ingenic SoCs.").
These two lines here, they should be one line :-)
For the Fixes: line, you shouldn't worry about the 72-char limit. Also,
when resending, don't add a blank line between Fixes and Signed-off-by
and since this is a bug fix, it seems like Cc: stable is in order.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH 1/1] USB: PHY: JZ4770: Fix uninitialized value written to HW register
Hi, Paul Cercueil writes: > @@ -246,9 +241,8 @@ static void x1830_usb_phy_init(struct usb_phy > *phy) > USBPCR1_DMPD | USBPCR1_DPPD; > writel(reg, priv->base + REG_USBPCR1_OFFSET); > > - reg = USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT > |USBPCR_TXPREEMPHTUNE | > + return USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT | > USBPCR_TXPREEMPHTUNE | > USBPCR_COMMONONN | USBPCR_POR; > - writel(reg, priv->base + REG_USBPCR_OFFSET); not a bug fix >>> >>> Well, if you don't like my bug fix, next time wait for my >>> Reviewed-by. >> >> why so angry? Take a break every once in a while. Besides, someone >> else >> already sent the oneliner before you ;-) > > I'm just pissed that this patch has not been tested. I don't like > sloppy work. yeah, s**t happens >> In any case, why should I wait for your Reviewed-by? Get maintainer >> doesn't list you as the maintainer for it. Do you want to update >> MAINTAINERS by any chance? > > Yes, I thought I was (I'm maintainer of all Ingenic drivers), that also > explains why I wasn't Cc'd for the oneliner patch you mentioned... > > IIRC Zhou has a patch to move the driver to drivers/phy/, I'll add > myself as maintainer once it's moved there. makes sense -- balbi signature.asc Description: PGP signature
Re: [PATCH 1/2] docs: process: Add cross-link to security-bugs
Hi, On Thu, Aug 27, 2020 at 1:54 PM Krzysztof Kozlowski wrote: > > The submitting patches mentions criteria for a fix to be called > "security fix". Add a link to document explaining the entire process > of handling security bugs. > > Cc: Greg KH > Cc: Marek Szyprowski > Cc: Linus Torvalds > Cc: Kees Cook > Signed-off-by: Krzysztof Kozlowski > --- > Documentation/process/submitting-patches.rst | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/Documentation/process/submitting-patches.rst > b/Documentation/process/submitting-patches.rst > index 5219bf3cddfc..d5b3c5a74d5d 100644 > --- a/Documentation/process/submitting-patches.rst > +++ b/Documentation/process/submitting-patches.rst > @@ -299,7 +299,8 @@ sending him e-mail. > If you have a patch that fixes an exploitable security bug, send that patch > to secur...@kernel.org. For severe bugs, a short embargo may be considered > to allow distributors to get the patch out to users; in such cases, > -obviously, the patch should not be sent to any public lists. > +obviously, the patch should not be sent to any public lists. See also > +:ref:`Documentation/admin-guide/security-bugs.rst `. > > Patches that fix a severe bug in a released kernel should be directed > toward the stable maintainers by putting a line like this:: > -- > 2.17.1 Reviewed-by: Felipe Balbi -- balbi
Re: [PATCH 1/1] USB: PHY: JZ4770: Fix uninitialized value written to HW register
Hi,
Paul Cercueil writes:
>>> @@ -172,7 +172,8 @@ static int ingenic_usb_phy_init(struct usb_phy
>>> *phy)
>>> return err;
>>> }
>>>
>>> - priv->soc_info->usb_phy_init(phy);
>>> + reg = priv->soc_info->usb_phy_init(phy);
>>> + writel(reg, priv->base + REG_USBPCR_OFFSET);
>>
>> not fixing any bug.
>>
>> Looking at the code, the bug follows after this line. It would suffice
>> to read REG_USBPCR_OFFSET in order to initialize reg. This bug fix
>> could
>> have been a one liner.
>
> There's no need to re-read a register when you have the value readily
> available. It just needs to be returned from the usb_phy_init
> callbacks. But yes, it's not a one-liner.
there's a difference between making a bug fix and reworking the behavior
of the driver ;-)
>>> @@ -195,19 +196,15 @@ static void ingenic_usb_phy_remove(void *phy)
>>> usb_remove_phy(phy);
>>> }
>>>
>>> -static void jz4770_usb_phy_init(struct usb_phy *phy)
>>> +static u32 jz4770_usb_phy_init(struct usb_phy *phy)
>>
>> not a bug fix
>>
>>> {
>>> - struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>>> - u32 reg;
>>> -
>>> - reg = USBPCR_AVLD_REG | USBPCR_COMMONONN | USBPCR_IDPULLUP_ALWAYS
>>> |
>>> + return USBPCR_AVLD_REG | USBPCR_COMMONONN |
>>> USBPCR_IDPULLUP_ALWAYS |
>>> USBPCR_COMPDISTUNE_DFT | USBPCR_OTGTUNE_DFT |
>>> USBPCR_SQRXTUNE_DFT |
>>> USBPCR_TXFSLSTUNE_DFT | USBPCR_TXRISETUNE_DFT |
>>> USBPCR_TXVREFTUNE_DFT |
>>> USBPCR_POR;
>>> - writel(reg, priv->base + REG_USBPCR_OFFSET);
>>
>> not a bug fix
>>
>>> }
>>>
>>> -static void jz4780_usb_phy_init(struct usb_phy *phy)
>>> +static u32 jz4780_usb_phy_init(struct usb_phy *phy)
>>
>> not a bug fix
>>
>>> @@ -216,11 +213,10 @@ static void jz4780_usb_phy_init(struct
>>> usb_phy *phy)
>>> USBPCR1_WORD_IF_16BIT;
>>> writel(reg, priv->base + REG_USBPCR1_OFFSET);
>>>
>>> - reg = USBPCR_TXPREEMPHTUNE | USBPCR_COMMONONN | USBPCR_POR;
>>> - writel(reg, priv->base + REG_USBPCR_OFFSET);
>>> + return USBPCR_TXPREEMPHTUNE | USBPCR_COMMONONN | USBPCR_POR;
>>
>> not a bug fix
>>
>>> }
>>>
>>> -static void x1000_usb_phy_init(struct usb_phy *phy)
>>> +static u32 x1000_usb_phy_init(struct usb_phy *phy)
>>
>> not a bug fix
>>
>>> {
>>> struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>>> u32 reg;
>>> @@ -228,13 +224,12 @@ static void x1000_usb_phy_init(struct usb_phy
>>> *phy)
>>> reg = readl(priv->base + REG_USBPCR1_OFFSET) |
>>> USBPCR1_WORD_IF_16BIT;
>>> writel(reg, priv->base + REG_USBPCR1_OFFSET);
>>>
>>> - reg = USBPCR_SQRXTUNE_DCR_20PCT | USBPCR_TXPREEMPHTUNE |
>>> + return USBPCR_SQRXTUNE_DCR_20PCT | USBPCR_TXPREEMPHTUNE |
>>> USBPCR_TXHSXVTUNE_DCR_15MV | USBPCR_TXVREFTUNE_INC_25PPT |
>>> USBPCR_COMMONONN | USBPCR_POR;
>>> - writel(reg, priv->base + REG_USBPCR_OFFSET);
>>
>> not a bug fix
>>
>>> }
>>>
>>> -static void x1830_usb_phy_init(struct usb_phy *phy)
>>> +static u32 x1830_usb_phy_init(struct usb_phy *phy)
>>
>> not a bug fix
>>
>>> {
>>> struct jz4770_phy *priv = phy_to_jz4770_phy(phy);
>>> u32 reg;
>>> @@ -246,9 +241,8 @@ static void x1830_usb_phy_init(struct usb_phy
>>> *phy)
>>> USBPCR1_DMPD | USBPCR1_DPPD;
>>> writel(reg, priv->base + REG_USBPCR1_OFFSET);
>>>
>>> - reg = USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT
>>> | USBPCR_TXPREEMPHTUNE |
>>> + return USBPCR_IDPULLUP_OTG | USBPCR_VBUSVLDEXT |
>>> USBPCR_TXPREEMPHTUNE |
>>> USBPCR_COMMONONN | USBPCR_POR;
>>> - writel(reg, priv->base + REG_USBPCR_OFFSET);
>>
>> not a bug fix
>
> Well, if you don't like my bug fix, next time wait for my Reviewed-by.
why so angry? Take a break every once in a while. Besides, someone else
already sent the oneliner before you ;-)
In any case, why should I wait for your Reviewed-by? Get maintainer
doesn't list you as the maintainer for it. Do you want to update
MAINTAINERS by any chance?
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH v3] usb: mtu3: fix panic in mtu3_gadget_stop()
Macpaul Lin writes: > This patch fixes a possible issue when mtu3_gadget_stop() > already assigned NULL to mtu->gadget_driver during mtu_gadget_disconnect(). > > [] notifier_call_chain+0xa4/0x128 > [] __atomic_notifier_call_chain+0x84/0x138 > [] notify_die+0xb0/0x120 > [] die+0x1f8/0x5d0 > [] __do_kernel_fault+0x19c/0x280 > [] do_bad_area+0x44/0x140 > [] do_translation_fault+0x4c/0x90 > [] do_mem_abort+0xb8/0x258 > [] el1_da+0x24/0x3c > [] mtu3_gadget_disconnect+0xac/0x128 > [] mtu3_irq+0x34c/0xc18 > [] __handle_irq_event_percpu+0x2ac/0xcd0 > [] handle_irq_event_percpu+0x80/0x138 > [] handle_irq_event+0xac/0x148 > [] handle_fasteoi_irq+0x234/0x568 > [] generic_handle_irq+0x48/0x68 > [] __handle_domain_irq+0x264/0x1740 > [] gic_handle_irq+0x14c/0x250 > [] el1_irq+0xec/0x194 > [] dma_pool_alloc+0x6e4/0xae0 > [] cmdq_mbox_pool_alloc_impl+0xb0/0x238 > [] cmdq_pkt_alloc_buf+0x2dc/0x7c0 > [] cmdq_pkt_add_cmd_buffer+0x178/0x270 > [] cmdq_pkt_perf_begin+0x108/0x148 > [] cmdq_pkt_create+0x178/0x1f0 > [] mtk_crtc_config_default_path+0x328/0x7a0 > [] mtk_drm_idlemgr_kick+0xa6c/0x1460 > [] mtk_drm_crtc_atomic_begin+0x1a4/0x1a68 > [] drm_atomic_helper_commit_planes+0x154/0x878 > [] mtk_atomic_complete.isra.16+0xe80/0x19c8 > [] mtk_atomic_commit+0x258/0x898 > [] drm_atomic_commit+0xcc/0x108 > [] drm_mode_atomic_ioctl+0x1c20/0x2580 > [] drm_ioctl_kernel+0x118/0x1b0 > [] drm_ioctl+0x5c0/0x920 > [] do_vfs_ioctl+0x188/0x1820 > [] SyS_ioctl+0x8c/0xa0 > > Signed-off-by: Macpaul Lin > Cc: sta...@vger.kernel.org missing a Fixes: line here -- balbi signature.asc Description: PGP signature
Re: [PATCH 3/3] usb: cdns3: Enable workaround for USB2.0 PHY Rx compliance test PHY lockup
Hi, Roger Quadros writes: > From: Pawel Laszczak > > USB2.0 PHY hangs in Rx Compliance test when the incoming packet > amplitude is varied below and above the Squelch Level of Receiver > during the active packet multiple times. > > Version 1 of the controller allows PHY to be reset when RX fail > condition is detected to work around the above issue. This feature > is disabled by default and needs to be enabled using a bit from > the newly added PHYRST_CFG register. This patch enables the > workaround. > > As there is no way to distinguish between the controller version > before the device controller is started we need to rely on a DT > property to decide when to apply the workaround. Pawel, it could know the controller version at cdns3_gadget_start, but the controller starts when it tries to bind gadget driver, at that time, it has already known the controller version. For me, the device controller starts is using USB_CONF.DEVEN (Device Enable) through usb_gadget_connect, I am not sure if it is the same with yours. >>> >>> Yes in device mode driver knows controller version but this >>> workaround Must be enabled also in host mode. In host mode the >>> controller doesn't have access to device registers. The controller >>> version is placed in device register. >>> >> >> You may suggest your design team adding CHIP_VER register at global >> register region, it will easy the software engineer life. >> > >From what I read, this register is only enabling USB2 PHY reset >> software control, it needs for all chips with rev 0x0002450D, and the >> place you current change is only for 0x0002450D, right? > > Even I could say that this workaround should be enabled only for Specific > USB2 > PHY (only 0x0002450D) > > This bit should not have any impact for Cadence PHY but it can has Impact > for third > party PHYs. > So, it is related to specific PHY, but enable this specific PHY reset bit is at controller region, why don't put this enable bit at PHY region? >>> >>> I think this is related to Controller + PHY combination. >>> The fix for the issue is via a bit in the controller, so it needs to be >>> managed by the >>> controller driver. >>> So, you use controller's device property to know this specific PHY, can controller know this specific PHY dynamically? >>> >>> Still the PHY will have to tell the controller the enable that bit. How to >>> do that? >>> >>> Adding a dt-property that vendors can used was the simplest option. >>> >> >> Ok, does all controllers with ver 0x0002450D need this fix? I just think >> if we introduce a flag stands for ver 0x0002450D in case this ver has >> other issues in future or just using phy reset enable property? >> >> Pawel & Roger, what's your opinion? >> > I think it is best to keep the flags specific to the issue rather than > a one flag for all issues with a specific version. This way you can > re-use the flag irrespective of IP version. I second that. Specially when some SoC-manufacturers may implement ECO fixes and not change IP revision. > But best case is that Cadence put a IP revision register in common area as you > have previously suggested so driver can automatically apply quirks to specific > versions. little too late for that :-) -- balbi signature.asc Description: PGP signature
