[PATCH 1/7] usb: mtu3: give back request when rx error happens

[PATCH 1/7] usb: mtu3: give back request when rx error happens

[Chunfeng Yun]

When the Rx enconnter errors, currently, only print error logs, that
may cause class driver's RX halt, shall give back the request with
error status meanwhile.

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/usb/mtu3/mtu3_qmu.c | 39 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/mtu3/mtu3_qmu.c b/drivers/usb/mtu3/mtu3_qmu.c
index a2fdab8b63b2..7be4e4be1a6a 100644
--- a/drivers/usb/mtu3/mtu3_qmu.c
+++ b/drivers/usb/mtu3/mtu3_qmu.c
@@ -466,6 +466,39 @@ static void qmu_tx_zlp_error_handler(struct mtu3 *mtu, u8 epnum)
 	mtu3_qmu_resume(mep);
 }
 
+/*
+ * when rx error happens (except zlperr), QMU will stop, and RQCPR saves
+ * the GPD encountered error, Done irq will arise after resuming QMU again.
+ */
+static void qmu_error_rx(struct mtu3 *mtu, u8 epnum)
+{
+	struct mtu3_ep *mep = mtu->out_eps + epnum;
+	struct mtu3_gpd_ring *ring = &mep->gpd_ring;
+	struct qmu_gpd *gpd_current = NULL;
+	struct usb_request *req = NULL;
+	struct mtu3_request *mreq;
+	dma_addr_t cur_gpd_dma;
+
+	cur_gpd_dma = read_rxq_cur_addr(mtu->mac_base, epnum);
+	gpd_current = gpd_dma_to_virt(ring, cur_gpd_dma);
+
+	mreq = next_request(mep);
+	if (!mreq || mreq->gpd != gpd_current) {
+		dev_err(mtu->dev, "no correct RX req is found\n");
+		return;
+	}
+
+	req = &mreq->request;
+	req->status = -EAGAIN;
+
+	/* by pass the current GDP */
+	gpd_current->dw0_info |= cpu_to_le32(GPD_FLAGS_BPS | GPD_FLAGS_HWO);
+	mtu3_qmu_resume(mep);
+
+	dev_dbg(mtu->dev, "%s EP%d, current=%p, req=%p\n",
+		__func__, epnum, gpd_current, mreq);
+}
+
 /*
  * NOTE: request list maybe is already empty as following case:
  * queue_tx --> qmu_interrupt(clear interrupt pending, schedule tasklet)-->
@@ -571,14 +604,18 @@ static void qmu_exception_isr(struct mtu3 *mtu, u32 qmu_status)
 
 	if ((qmu_status & RXQ_CSERR_INT) || (qmu_status & RXQ_LENERR_INT)) {
 		errval = mtu3_readl(mbase, U3D_RQERRIR0);
+		mtu3_writel(mbase, U3D_RQERRIR0, errval);
+
 		for (i = 1; i < mtu->num_eps; i++) {
 			if (errval & QMU_RX_CS_ERR(i))
 				dev_err(mtu->dev, "Rx %d CS error!\n", i);
 
 			if (errval & QMU_RX_LEN_ERR(i))
 				dev_err(mtu->dev, "RX %d Length error\n", i);
+
+			if (errval & (QMU_RX_CS_ERR(i) | QMU_RX_LEN_ERR(i)))
+				qmu_error_rx(mtu, i);
 		}
-		mtu3_writel(mbase, U3D_RQERRIR0, errval);
 	}
 
 	if (qmu_status & RXQ_ZLPERR_INT) {
-- 
2.18.0

[PATCH 2/7] usb: mtu3: use boolean return value

[Chunfeng Yun]

Prefer to use boolean value due to gpd_ring_empty() return true or false.
See "16) Function return values and names" in coding-style.rst

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/usb/mtu3/mtu3_qmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/mtu3/mtu3_qmu.c b/drivers/usb/mtu3/mtu3_qmu.c
index 7be4e4be1a6a..66639f602a9d 100644
--- a/drivers/usb/mtu3/mtu3_qmu.c
+++ b/drivers/usb/mtu3/mtu3_qmu.c
@@ -221,7 +221,7 @@ static struct qmu_gpd *advance_deq_gpd(struct mtu3_gpd_ring *ring)
 }
 
 /* check if a ring is emtpy */
-static int gpd_ring_empty(struct mtu3_gpd_ring *ring)
+static bool gpd_ring_empty(struct mtu3_gpd_ring *ring)
 {
 	struct qmu_gpd *enq = ring->enqueue;
 	struct qmu_gpd *next;
-- 
2.18.0

Re: [PATCH 2/7] usb: mtu3: use boolean return value

[AngeloGioacchino Del Regno]

Il 03/04/23 04:52, Chunfeng Yun ha scritto:
> Prefer to use boolean value due to gpd_ring_empty() return true or false.
> See "16) Function return values and names" in coding-style.rst
> 
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

[PATCH 3/7] usb: mtu3: fix KE at qmu transfer done irq handler

[Chunfeng Yun]

When handle qmu transfer irq, it will unlock @mtu->lock before give back
request, if another thread hanlde disconnect event at the same time, and
try to disable ep, it may lock @mtu->lock and free qmu ring, then qmu
irq hanlder may get a NULL gpd, avoid the KE by checking gpd's value before
handling it.

e.g.
qmu done irq on cpu0                 thread running on cpu1

qmu_done_tx()
  handle gpd [0]
    mtu3_requ_complete()        mtu3_gadget_ep_disable()
      unlock @mtu->lock
        give back request         lock @mtu->lock
                                    mtu3_ep_disable()
                                      mtu3_gpd_ring_free()
                                   unlock @mtu->lock
      lock @mtu->lock
    get next gpd [1]

[1]: goto [0] to handle next gpd, and next gpd may be NULL.

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/usb/mtu3/mtu3_qmu.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/mtu3/mtu3_qmu.c b/drivers/usb/mtu3/mtu3_qmu.c
index 66639f602a9d..3146a112141f 100644
--- a/drivers/usb/mtu3/mtu3_qmu.c
+++ b/drivers/usb/mtu3/mtu3_qmu.c
@@ -210,6 +210,7 @@ static struct qmu_gpd *advance_enq_gpd(struct mtu3_gpd_ring *ring)
 	return ring->enqueue;
 }
 
+/* @dequeue may be NULL if ring is unallocated or freed */
 static struct qmu_gpd *advance_deq_gpd(struct mtu3_gpd_ring *ring)
 {
 	if (ring->dequeue < ring->end)
@@ -524,7 +525,7 @@ static void qmu_done_tx(struct mtu3 *mtu, u8 epnum)
 	dev_dbg(mtu->dev, "%s EP%d, last=%p, current=%p, enq=%p\n",
 		__func__, epnum, gpd, gpd_current, ring->enqueue);
 
-	while (gpd != gpd_current && !GET_GPD_HWO(gpd)) {
+	while (gpd && gpd != gpd_current && !GET_GPD_HWO(gpd)) {
 
 		mreq = next_request(mep);
 
@@ -563,7 +564,7 @@ static void qmu_done_rx(struct mtu3 *mtu, u8 epnum)
 	dev_dbg(mtu->dev, "%s EP%d, last=%p, current=%p, enq=%p\n",
 		__func__, epnum, gpd, gpd_current, ring->enqueue);
 
-	while (gpd != gpd_current && !GET_GPD_HWO(gpd)) {
+	while (gpd && gpd != gpd_current && !GET_GPD_HWO(gpd)) {
 
 		mreq = next_request(mep);
 
-- 
2.18.0

Re: [PATCH 3/7] usb: mtu3: fix KE at qmu transfer done irq handler

[AngeloGioacchino Del Regno]

Il 03/04/23 04:52, Chunfeng Yun ha scritto:
> When handle qmu transfer irq, it will unlock @mtu->lock before give back
> request, if another thread hanlde disconnect event at the same time, and
> try to disable ep, it may lock @mtu->lock and free qmu ring, then qmu
> irq hanlder may get a NULL gpd, avoid the KE by checking gpd's value before

KE == Kernel Error? I think you wanted to say KP == Kernel Panic instead.

Also, s/hanlder/handler/g.

> handling it.
> 
> e.g.
> qmu done irq on cpu0                 thread running on cpu1
> 
> qmu_done_tx()
>    handle gpd [0]
>      mtu3_requ_complete()        mtu3_gadget_ep_disable()
>        unlock @mtu->lock
>          give back request         lock @mtu->lock
>                                      mtu3_ep_disable()
>                                        mtu3_gpd_ring_free()
>                                     unlock @mtu->lock
>        lock @mtu->lock
>      get next gpd [1]
> 
> [1]: goto [0] to handle next gpd, and next gpd may be NULL.
 >
 > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>

This is a fix and needs a Fixes tag.

Regards,
Angelo

Re: [PATCH 3/7] usb: mtu3: fix KE at qmu transfer done irq handler

[Chunfeng Yun (云春峰)]

On Mon, 2023-04-03 at 14:31 +0200, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Il 03/04/23 04:52, Chunfeng Yun ha scritto:
> > When handle qmu transfer irq, it will unlock @mtu->lock before give
> > back
> > request, if another thread hanlde disconnect event at the same
> > time, and
> > try to disable ep, it may lock @mtu->lock and free qmu ring, then
> > qmu
> > irq hanlder may get a NULL gpd, avoid the KE by checking gpd's
> > value before
> 
> KE == Kernel Error? I think you wanted to say KP == Kernel Panic
> instead.
> 
> Also, s/hanlder/handler/g.
Ok, will modify it
> 
> > handling it.
> > 
> > e.g.
> > qmu done irq on cpu0                 thread running on cpu1
> > 
> > qmu_done_tx()
> >    handle gpd [0]
> >      mtu3_requ_complete()        mtu3_gadget_ep_disable()
> >        unlock @mtu->lock
> >          give back request         lock @mtu->lock
> >                                      mtu3_ep_disable()
> >                                        mtu3_gpd_ring_free()
> >                                     unlock @mtu->lock
> >        lock @mtu->lock
> >      get next gpd [1]
> > 
> > [1]: goto [0] to handle next gpd, and next gpd may be NULL.
> 
>  >
>  > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> 
> This is a fix and needs a Fixes tag.
I usually add Fixes tag when the issue introduced by a patch except the
original one when the driver applied.

Thanks a lot

> 
> Regards,
> Angelo
> 
> 

Re: [PATCH 3/7] usb: mtu3: fix KE at qmu transfer done irq handler

[AngeloGioacchino Del Regno]

Il 07/04/23 09:59, Chunfeng Yun (云春峰) ha scritto:
> On Mon, 2023-04-03 at 14:31 +0200, AngeloGioacchino Del Regno wrote:
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>
>>
>> Il 03/04/23 04:52, Chunfeng Yun ha scritto:
>>> When handle qmu transfer irq, it will unlock @mtu->lock before give
>>> back
>>> request, if another thread hanlde disconnect event at the same
>>> time, and
>>> try to disable ep, it may lock @mtu->lock and free qmu ring, then
>>> qmu
>>> irq hanlder may get a NULL gpd, avoid the KE by checking gpd's
>>> value before
>>
>> KE == Kernel Error? I think you wanted to say KP == Kernel Panic
>> instead.
>>
>> Also, s/hanlder/handler/g.
> Ok, will modify it
>>
>>> handling it.
>>>
>>> e.g.
>>> qmu done irq on cpu0                 thread running on cpu1
>>>
>>> qmu_done_tx()
>>>     handle gpd [0]
>>>       mtu3_requ_complete()        mtu3_gadget_ep_disable()
>>>         unlock @mtu->lock
>>>           give back request         lock @mtu->lock
>>>                                       mtu3_ep_disable()
>>>                                         mtu3_gpd_ring_free()
>>>                                      unlock @mtu->lock
>>>         lock @mtu->lock
>>>       get next gpd [1]
>>>
>>> [1]: goto [0] to handle next gpd, and next gpd may be NULL.
>>
>>   >
>>   > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
>>
>> This is a fix and needs a Fixes tag.
> I usually add Fixes tag when the issue introduced by a patch except the
> original one when the driver applied.
> 

If this patch is a fix for the "original driver", you shall still add a
Fixes tag which advertises that this fixes the first commit, so the driver
was broken from the very beginning.

Thanks,
Angelo.

[PATCH 4/7] usb: mtu3: unlock @mtu->lock just before giving back request

[Chunfeng Yun]

No need unlock @mtu->lock when unmap request, unlock it just before
giving back request, due to we do not lock this spinlock when map
the request.

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/usb/mtu3/mtu3_gadget.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/mtu3/mtu3_gadget.c b/drivers/usb/mtu3/mtu3_gadget.c
index c0264d5426bf..ad0eeac4332d 100644
--- a/drivers/usb/mtu3/mtu3_gadget.c
+++ b/drivers/usb/mtu3/mtu3_gadget.c
@@ -23,7 +23,6 @@ __acquires(mep->mtu->lock)
 		req->status = status;
 
 	trace_mtu3_req_complete(mreq);
-	spin_unlock(&mtu->lock);
 
 	/* ep0 makes use of PIO, needn't unmap it */
 	if (mep->epnum)
@@ -32,6 +31,7 @@ __acquires(mep->mtu->lock)
 	dev_dbg(mtu->dev, "%s complete req: %p, sts %d, %d/%d\n",
 		mep->name, req, req->status, req->actual, req->length);
 
+	spin_unlock(&mtu->lock);
 	usb_gadget_giveback_request(&mep->ep, req);
 	spin_lock(&mtu->lock);
 }
-- 
2.18.0

Re: [PATCH 4/7] usb: mtu3: unlock @mtu->lock just before giving back request

[AngeloGioacchino Del Regno]

Il 03/04/23 04:52, Chunfeng Yun ha scritto:
> No need unlock @mtu->lock when unmap request, unlock it just before
> giving back request, due to we do not lock this spinlock when map
> the request.
> 
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

[PATCH 5/7] usb: mtu3: expose role-switch control to userspace

[Chunfeng Yun]

The allow_userspace_control flag enables manual role switch from userspace,
turn this feature on like several other USB DRD controller drivers.

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/usb/mtu3/mtu3_dr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/mtu3/mtu3_dr.c b/drivers/usb/mtu3/mtu3_dr.c
index 9b8aded3d95e..8191b7ed3852 100644
--- a/drivers/usb/mtu3/mtu3_dr.c
+++ b/drivers/usb/mtu3/mtu3_dr.c
@@ -294,6 +294,7 @@ static int ssusb_role_sw_register(struct otg_switch_mtk *otg_sx)
 	role_sx_desc.get = ssusb_role_sw_get;
 	role_sx_desc.fwnode = dev_fwnode(dev);
 	role_sx_desc.driver_data = ssusb;
+	role_sx_desc.allow_userspace_control = true;
 	otg_sx->role_sw = usb_role_switch_register(dev, &role_sx_desc);
 	if (IS_ERR(otg_sx->role_sw))
 		return PTR_ERR(otg_sx->role_sw);
-- 
2.18.0

Re: [PATCH 5/7] usb: mtu3: expose role-switch control to userspace

[AngeloGioacchino Del Regno]

Il 03/04/23 04:52, Chunfeng Yun ha scritto:
> The allow_userspace_control flag enables manual role switch from userspace,
> turn this feature on like several other USB DRD controller drivers.
> 
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

[PATCH 6/7] dt-bindings: usb: mtu3: add two optional clocks

[Chunfeng Yun]

Add optional clock 'xhci_ck' and 'frmcnt_ck';
Add optional property "assigned-clock" and "assigned-clock-parents";

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 .../devicetree/bindings/usb/mediatek,mtu3.yaml       | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/mediatek,mtu3.yaml b/Documentation/devicetree/bindings/usb/mediatek,mtu3.yaml
index d2655173e108..50c15f2ce14d 100644
--- a/Documentation/devicetree/bindings/usb/mediatek,mtu3.yaml
+++ b/Documentation/devicetree/bindings/usb/mediatek,mtu3.yaml
@@ -66,6 +66,8 @@ properties:
       - description: Reference clock used by low power mode etc
       - description: Mcu bus clock for register access
       - description: DMA bus clock for data transfer
+      - description: DRD controller clock
+      - description: Frame count clock
 
   clock-names:
     minItems: 1
@@ -74,6 +76,16 @@ properties:
       - const: ref_ck
       - const: mcu_ck
       - const: dma_ck
+      - const: xhci_ck
+      - const: frmcnt_ck
+
+  assigned-clocks:
+    minItems: 1
+    maxItems: 6
+
+  assigned-clock-parents:
+    minItems: 1
+    maxItems: 6
 
   phys:
     description:
-- 
2.18.0

Re: [PATCH 6/7] dt-bindings: usb: mtu3: add two optional clocks

[Rob Herring]

On Mon, Apr 03, 2023 at 10:52:29AM +0800, Chunfeng Yun wrote:
> Add optional clock 'xhci_ck' and 'frmcnt_ck';
> Add optional property "assigned-clock" and "assigned-clock-parents";
> 
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
>  .../devicetree/bindings/usb/mediatek,mtu3.yaml       | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/mediatek,mtu3.yaml b/Documentation/devicetree/bindings/usb/mediatek,mtu3.yaml
> index d2655173e108..50c15f2ce14d 100644
> --- a/Documentation/devicetree/bindings/usb/mediatek,mtu3.yaml
> +++ b/Documentation/devicetree/bindings/usb/mediatek,mtu3.yaml
> @@ -66,6 +66,8 @@ properties:
>        - description: Reference clock used by low power mode etc
>        - description: Mcu bus clock for register access
>        - description: DMA bus clock for data transfer
> +      - description: DRD controller clock
> +      - description: Frame count clock
>  
>    clock-names:
>      minItems: 1
> @@ -74,6 +76,16 @@ properties:
>        - const: ref_ck
>        - const: mcu_ck
>        - const: dma_ck
> +      - const: xhci_ck
> +      - const: frmcnt_ck
> +
> +  assigned-clocks:
> +    minItems: 1
> +    maxItems: 6
> +
> +  assigned-clock-parents:
> +    minItems: 1
> +    maxItems: 6

You don't need to include assigned-clocks properties. They are 
implicitly allowed (when clocks is present).

Rob

Re: [PATCH 6/7] dt-bindings: usb: mtu3: add two optional clocks

[Chunfeng Yun (云春峰)]

On Tue, 2023-04-04 at 08:43 -0500, Rob Herring wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Mon, Apr 03, 2023 at 10:52:29AM +0800, Chunfeng Yun wrote:
> > Add optional clock 'xhci_ck' and 'frmcnt_ck';
> > Add optional property "assigned-clock" and "assigned-clock-
> > parents";
> > 
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > ---
> >  .../devicetree/bindings/usb/mediatek,mtu3.yaml       | 12
> > ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/usb/mediatek,mtu3.yaml
> > b/Documentation/devicetree/bindings/usb/mediatek,mtu3.yaml
> > index d2655173e108..50c15f2ce14d 100644
> > --- a/Documentation/devicetree/bindings/usb/mediatek,mtu3.yaml
> > +++ b/Documentation/devicetree/bindings/usb/mediatek,mtu3.yaml
> > @@ -66,6 +66,8 @@ properties:
> >        - description: Reference clock used by low power mode etc
> >        - description: Mcu bus clock for register access
> >        - description: DMA bus clock for data transfer
> > +      - description: DRD controller clock
> > +      - description: Frame count clock
> > 
> >    clock-names:
> >      minItems: 1
> > @@ -74,6 +76,16 @@ properties:
> >        - const: ref_ck
> >        - const: mcu_ck
> >        - const: dma_ck
> > +      - const: xhci_ck
> > +      - const: frmcnt_ck
> > +
> > +  assigned-clocks:
> > +    minItems: 1
> > +    maxItems: 6
> > +
> > +  assigned-clock-parents:
> > +    minItems: 1
> > +    maxItems: 6
> 
> You don't need to include assigned-clocks properties. They are
> implicitly allowed (when clocks is present).
Got it, will modify it, thanks a lot

> 
> Rob

[PATCH 7/7] usb: mtu3: add optional clock xhci_ck and frmcnt_ck

[Chunfeng Yun]

Add optional clock 'xhci_ck' which is usually the same as sys_ck, but
some SoC use two separated clocks when the controller supports dual
role mode;
Add optional clock 'frmcnt_ck' used on 4nm or advanced process SoC.

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/usb/mtu3/mtu3.h      | 2 +-
 drivers/usb/mtu3/mtu3_plat.c | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/mtu3/mtu3.h b/drivers/usb/mtu3/mtu3.h
index 2d7b57e07eee..b4a7662dded5 100644
--- a/drivers/usb/mtu3/mtu3.h
+++ b/drivers/usb/mtu3/mtu3.h
@@ -90,7 +90,7 @@ struct mtu3_request;
  */
 #define EP0_RESPONSE_BUF  6
 
-#define BULK_CLKS_CNT	4
+#define BULK_CLKS_CNT	6
 
 /* device operated link and speed got from DEVICE_CONF register */
 enum mtu3_speed {
diff --git a/drivers/usb/mtu3/mtu3_plat.c b/drivers/usb/mtu3/mtu3_plat.c
index d78ae52b4e26..6f264b129243 100644
--- a/drivers/usb/mtu3/mtu3_plat.c
+++ b/drivers/usb/mtu3/mtu3_plat.c
@@ -234,6 +234,8 @@ static int get_ssusb_rscs(struct platform_device *pdev, struct ssusb_mtk *ssusb)
 	clks[1].id = "ref_ck";
 	clks[2].id = "mcu_ck";
 	clks[3].id = "dma_ck";
+	clks[4].id = "xhci_ck";
+	clks[5].id = "frmcnt_ck";
 	ret = devm_clk_bulk_get_optional(dev, BULK_CLKS_CNT, clks);
 	if (ret)
 		return ret;
-- 
2.18.0

Re: [PATCH 7/7] usb: mtu3: add optional clock xhci_ck and frmcnt_ck

[AngeloGioacchino Del Regno]

Il 03/04/23 04:52, Chunfeng Yun ha scritto:
> Add optional clock 'xhci_ck' which is usually the same as sys_ck, but
> some SoC use two separated clocks when the controller supports dual
> role mode;
> Add optional clock 'frmcnt_ck' used on 4nm or advanced process SoC.

This needs more details, because from what it seems, `xhci_ck` could be
a children of `sys_ck`, in which case there would be no need to add that
to this driver, as it'd be handled by the clock API instead.

What SoC is this for?

Regards,
Angelo

Re: [PATCH 7/7] usb: mtu3: add optional clock xhci_ck and frmcnt_ck

[Chunfeng Yun (云春峰)]

On Mon, 2023-04-03 at 14:34 +0200, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Il 03/04/23 04:52, Chunfeng Yun ha scritto:
> > Add optional clock 'xhci_ck' which is usually the same as sys_ck,
> > but
> > some SoC use two separated clocks when the controller supports dual
> > role mode;
> > Add optional clock 'frmcnt_ck' used on 4nm or advanced process SoC.
> 
> This needs more details, because from what it seems, `xhci_ck` could
> be
> a children of `sys_ck`,
No, it's not child of 'sys_ck', they are all the 125Mhz clocks.

> in which case there would be no need to add that
> to this driver, as it'd be handled by the clock API instead.
> 
> What SoC is this for?
encounter the issue on mt8195

> 
> Regards,
> Angelo
> 

Re: [PATCH 1/7] usb: mtu3: give back request when rx error happens

[AngeloGioacchino Del Regno]

Il 03/04/23 04:52, Chunfeng Yun ha scritto:
> When the Rx enconnter errors, currently, only print error logs, that
> may cause class driver's RX halt, shall give back the request with
> error status meanwhile.
> 
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>

 From what I understand, this is not a new feature, but a fix for a unwanted QMU
halt.
This means that this commit needs a Fixes tag.

> ---
>   drivers/usb/mtu3/mtu3_qmu.c | 39 ++++++++++++++++++++++++++++++++++++-
>   1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/mtu3/mtu3_qmu.c b/drivers/usb/mtu3/mtu3_qmu.c
> index a2fdab8b63b2..7be4e4be1a6a 100644
> --- a/drivers/usb/mtu3/mtu3_qmu.c
> +++ b/drivers/usb/mtu3/mtu3_qmu.c
> @@ -466,6 +466,39 @@ static void qmu_tx_zlp_error_handler(struct mtu3 *mtu, u8 epnum)
>   	mtu3_qmu_resume(mep);
>   }
>   
> +/*
> + * when rx error happens (except zlperr), QMU will stop, and RQCPR saves
> + * the GPD encountered error, Done irq will arise after resuming QMU again.
> + */
> +static void qmu_error_rx(struct mtu3 *mtu, u8 epnum)
> +{
> +	struct mtu3_ep *mep = mtu->out_eps + epnum;
> +	struct mtu3_gpd_ring *ring = &mep->gpd_ring;
> +	struct qmu_gpd *gpd_current = NULL;
> +	struct usb_request *req = NULL;
> +	struct mtu3_request *mreq;
> +	dma_addr_t cur_gpd_dma;
> +
> +	cur_gpd_dma = read_rxq_cur_addr(mtu->mac_base, epnum);
> +	gpd_current = gpd_dma_to_virt(ring, cur_gpd_dma);
> +
> +	mreq = next_request(mep);
> +	if (!mreq || mreq->gpd != gpd_current) {
> +		dev_err(mtu->dev, "no correct RX req is found\n");
> +		return;
> +	}
> +
> +	req = &mreq->request;
> +	req->status = -EAGAIN;

You don't need a *req pointer for just one simple assignment.

	mreq->request.status = -EAGAIN;

that'll do.

> +
> +	/* by pass the current GDP */
> +	gpd_current->dw0_info |= cpu_to_le32(GPD_FLAGS_BPS | GPD_FLAGS_HWO);
> +	mtu3_qmu_resume(mep);
> +
> +	dev_dbg(mtu->dev, "%s EP%d, current=%p, req=%p\n",
> +		__func__, epnum, gpd_current, mreq);
> +}
> +
>   /*
>    * NOTE: request list maybe is already empty as following case:
>    * queue_tx --> qmu_interrupt(clear interrupt pending, schedule tasklet)-->
> @@ -571,14 +604,18 @@ static void qmu_exception_isr(struct mtu3 *mtu, u32 qmu_status)
>   
>   	if ((qmu_status & RXQ_CSERR_INT) || (qmu_status & RXQ_LENERR_INT)) {
>   		errval = mtu3_readl(mbase, U3D_RQERRIR0);
> +		mtu3_writel(mbase, U3D_RQERRIR0, errval);

Please mention in the commit description the reason why you're moving this register
write here.

Regards,
Angelo

Re: [PATCH 1/7] usb: mtu3: give back request when rx error happens

[Chunfeng Yun (云春峰)]

On Mon, 2023-04-03 at 14:31 +0200, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Il 03/04/23 04:52, Chunfeng Yun ha scritto:
> > When the Rx enconnter errors, currently, only print error logs,
> > that
> > may cause class driver's RX halt, shall give back the request with
> > error status meanwhile.
> > 
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> 
>  From what I understand, this is not a new feature, but a fix for a
> unwanted QMU
> halt.
> This means that this commit needs a Fixes tag.

I did not take into account this cases when write this driver, it
caused the issue by the bug of host driver.

> 
> > ---
> >   drivers/usb/mtu3/mtu3_qmu.c | 39
> > ++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 38 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/mtu3/mtu3_qmu.c
> > b/drivers/usb/mtu3/mtu3_qmu.c
> > index a2fdab8b63b2..7be4e4be1a6a 100644
> > --- a/drivers/usb/mtu3/mtu3_qmu.c
> > +++ b/drivers/usb/mtu3/mtu3_qmu.c
> > @@ -466,6 +466,39 @@ static void qmu_tx_zlp_error_handler(struct
> > mtu3 *mtu, u8 epnum)
> >       mtu3_qmu_resume(mep);
> >   }
> > 
> > +/*
> > + * when rx error happens (except zlperr), QMU will stop, and RQCPR
> > saves
> > + * the GPD encountered error, Done irq will arise after resuming
> > QMU again.
> > + */
> > +static void qmu_error_rx(struct mtu3 *mtu, u8 epnum)
> > +{
> > +     struct mtu3_ep *mep = mtu->out_eps + epnum;
> > +     struct mtu3_gpd_ring *ring = &mep->gpd_ring;
> > +     struct qmu_gpd *gpd_current = NULL;
> > +     struct usb_request *req = NULL;
> > +     struct mtu3_request *mreq;
> > +     dma_addr_t cur_gpd_dma;
> > +
> > +     cur_gpd_dma = read_rxq_cur_addr(mtu->mac_base, epnum);
> > +     gpd_current = gpd_dma_to_virt(ring, cur_gpd_dma);
> > +
> > +     mreq = next_request(mep);
> > +     if (!mreq || mreq->gpd != gpd_current) {
> > +             dev_err(mtu->dev, "no correct RX req is found\n");
> > +             return;
> > +     }
> > +
> > +     req = &mreq->request;
> > +     req->status = -EAGAIN;
> 
> You don't need a *req pointer for just one simple assignment.
> 
>         mreq->request.status = -EAGAIN;
> 
> that'll do.

That's good, I'll modify it, thanks
> 
> > +
> > +     /* by pass the current GDP */
> > +     gpd_current->dw0_info |= cpu_to_le32(GPD_FLAGS_BPS |
> > GPD_FLAGS_HWO);
> > +     mtu3_qmu_resume(mep);
> > +
> > +     dev_dbg(mtu->dev, "%s EP%d, current=%p, req=%p\n",
> > +             __func__, epnum, gpd_current, mreq);
> > +}
> > +
> >   /*
> >    * NOTE: request list maybe is already empty as following case:
> >    * queue_tx --> qmu_interrupt(clear interrupt pending, schedule
> > tasklet)-->
> > @@ -571,14 +604,18 @@ static void qmu_exception_isr(struct mtu3
> > *mtu, u32 qmu_status)
> > 
> >       if ((qmu_status & RXQ_CSERR_INT) || (qmu_status &
> > RXQ_LENERR_INT)) {
> >               errval = mtu3_readl(mbase, U3D_RQERRIR0);
> > +             mtu3_writel(mbase, U3D_RQERRIR0, errval);
> 
> Please mention in the commit description the reason why you're moving
> this register
> write here.

It clears irq status before handling the error;

> 
> Regards,
> Angelo

Re: [PATCH 1/7] usb: mtu3: give back request when rx error happens

[Chunfeng Yun (云春峰)]

On Mon, 2023-04-03 at 14:31 +0200, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Il 03/04/23 04:52, Chunfeng Yun ha scritto:
> > When the Rx enconnter errors, currently, only print error logs,
> > that
> > may cause class driver's RX halt, shall give back the request with
> > error status meanwhile.
> > 
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> 
>  From what I understand, this is not a new feature, but a fix for a
> unwanted QMU
> halt.
> This means that this commit needs a Fixes tag.
I did not take into account this cases when write this driver, it
caused the issue by the bug of host driver.

> 
> > ---
> >   drivers/usb/mtu3/mtu3_qmu.c | 39
> > ++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 38 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/mtu3/mtu3_qmu.c
> > b/drivers/usb/mtu3/mtu3_qmu.c
> > index a2fdab8b63b2..7be4e4be1a6a 100644
> > --- a/drivers/usb/mtu3/mtu3_qmu.c
> > +++ b/drivers/usb/mtu3/mtu3_qmu.c
> > @@ -466,6 +466,39 @@ static void qmu_tx_zlp_error_handler(struct
> > mtu3 *mtu, u8 epnum)
> >       mtu3_qmu_resume(mep);
> >   }
> > 
> > +/*
> > + * when rx error happens (except zlperr), QMU will stop, and RQCPR
> > saves
> > + * the GPD encountered error, Done irq will arise after resuming
> > QMU again.
> > + */
> > +static void qmu_error_rx(struct mtu3 *mtu, u8 epnum)
> > +{
> > +     struct mtu3_ep *mep = mtu->out_eps + epnum;
> > +     struct mtu3_gpd_ring *ring = &mep->gpd_ring;
> > +     struct qmu_gpd *gpd_current = NULL;
> > +     struct usb_request *req = NULL;
> > +     struct mtu3_request *mreq;
> > +     dma_addr_t cur_gpd_dma;
> > +
> > +     cur_gpd_dma = read_rxq_cur_addr(mtu->mac_base, epnum);
> > +     gpd_current = gpd_dma_to_virt(ring, cur_gpd_dma);
> > +
> > +     mreq = next_request(mep);
> > +     if (!mreq || mreq->gpd != gpd_current) {
> > +             dev_err(mtu->dev, "no correct RX req is found\n");
> > +             return;
> > +     }
> > +
> > +     req = &mreq->request;
> > +     req->status = -EAGAIN;
> 
> You don't need a *req pointer for just one simple assignment.
> 
>         mreq->request.status = -EAGAIN;
> 
> that'll do.
That's good, I'll modify it, thanks
> 
> > +
> > +     /* by pass the current GDP */
> > +     gpd_current->dw0_info |= cpu_to_le32(GPD_FLAGS_BPS |
> > GPD_FLAGS_HWO);
> > +     mtu3_qmu_resume(mep);
> > +
> > +     dev_dbg(mtu->dev, "%s EP%d, current=%p, req=%p\n",
> > +             __func__, epnum, gpd_current, mreq);
> > +}
> > +
> >   /*
> >    * NOTE: request list maybe is already empty as following case:
> >    * queue_tx --> qmu_interrupt(clear interrupt pending, schedule
> > tasklet)-->
> > @@ -571,14 +604,18 @@ static void qmu_exception_isr(struct mtu3
> > *mtu, u32 qmu_status)
> > 
> >       if ((qmu_status & RXQ_CSERR_INT) || (qmu_status &
> > RXQ_LENERR_INT)) {
> >               errval = mtu3_readl(mbase, U3D_RQERRIR0);
> > +             mtu3_writel(mbase, U3D_RQERRIR0, errval);
> 
> Please mention in the commit description the reason why you're moving
> this register
> write here.
It clears irq status before handling the error;

> 
> Regards,
> Angelo