For Gen1 isoc-in endpoint on controller before about SSUSB IPM v1.6.0, it
still send out unexpected ACK after receiving a short packet in burst
transfer, this will cause an exception on connected device, specially for
a 4k camera.
Add a quirk property "rx-fifo-depth" to work around this hardware issue,
prefer to use 3k bytes;
The side-effect is that may cause performance drop about 10%, including
bulk transfer.
Signed-off-by: Chunfeng Yun <[email protected]>
---
v3: add fifo depth unit, change the value range from 0-3 to 1-4
v2: change 'mediatek,rxfifo-depth' to 'rx-fifo-depth'
---
.../devicetree/bindings/usb/mediatek,mtk-xhci.yaml | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.yaml b/Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.yaml
index e9644e333d78..9478b7031796 100644
--- a/Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.yaml
+++ b/Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.yaml
@@ -124,6 +124,18 @@ properties:
defined in the xHCI spec on MTK's controller.
default: 5000
+ rx-fifo-depth:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description:
+ It is a quirk used to work around Gen1 isoc-in endpoint transfer issue
+ that still send out unexpected ACK after device finish the burst transfer
+ with a short packet and cause an exception, specially on a 4K camera
+ device, it happens on controller before about IPM v1.6.0; the side-effect
+ is that may cause performance drop about 10%, include bulk transfer,
+ prefer to use 3 here. The unit is 1K bytes.
+ minimum: 1
+ maximum: 4
+
# the following properties are only used for case 1
wakeup-source:
description: enable USB remote wakeup, see power/wakeup-source.txt
--
2.18.0
Add the quirk property "rx-fifo-depth" to work around Gen1 isoc-in
transfer issue which send out unexpected ACK even after device
already finished the burst transfer with a short patcket, specially
for a 4K camera device.
Signed-off-by: Chunfeng Yun <[email protected]>
---
v3: change value according to binding
v2: use 'rx-fifo-depth' property
---
arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi b/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi
index dd5b89b73190..58593348b810 100644
--- a/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi
@@ -1185,6 +1185,7 @@
vusb33-supply = <&mt6359_vusb_ldo_reg>;
vbus-supply = <&usb_vbus>;
+ rx-fifo-depth = <3>;
};
&xhci1 {
@@ -1192,6 +1193,7 @@
vusb33-supply = <&mt6359_vusb_ldo_reg>;
vbus-supply = <&usb_vbus>;
+ rx-fifo-depth = <3>;
};
&xhci2 {
--
2.18.0
For Gen1 isoc-in transfer, host still send out unexpected ACK after device
finish the burst with a short packet, this will cause an exception on the
connected device, such as, a usb 4k camera.
It can be fixed by setting rxfifo depth less than 4k bytes, prefer to use
3k here, the side-effect is that may cause performance drop about 10%,
including bulk transfer.
Fixes: 926d60ae64a6 ("usb: xhci-mtk: modify the SOF/ITP interval for mt8195")
Cc: [email protected]
Signed-off-by: Chunfeng Yun <[email protected]>
---
v3:
add Cc stable
change @rxfifo_depth from int to u32, due to property's value range is changed
v2:
use 'rx-fifo-depth' property;
add header file 'linux/bitfield.h' to avoid building error on some ARCH;
---
drivers/usb/host/xhci-mtk.c | 37 +++++++++++++++++++++++++++++++++++--
drivers/usb/host/xhci-mtk.h | 2 ++
2 files changed, 37 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
index bbdf1b0b7be1..868793b82348 100644
--- a/drivers/usb/host/xhci-mtk.c
+++ b/drivers/usb/host/xhci-mtk.c
@@ -7,6 +7,7 @@
* Chunfeng Yun <[email protected]>
*/
+#include <linux/bitfield.h>
#include <linux/dma-mapping.h>
#include <linux/iopoll.h>
#include <linux/kernel.h>
@@ -73,6 +74,9 @@
#define FRMCNT_LEV1_RANG (0x12b << 8)
#define FRMCNT_LEV1_RANG_MASK GENMASK(19, 8)
+#define HSCH_CFG1 0x960
+#define SCH3_RXFIFO_DEPTH_MASK GENMASK(21, 20)
+
#define SS_GEN2_EOF_CFG 0x990
#define SSG2EOF_OFFSET 0x3c
@@ -165,6 +169,34 @@ static void xhci_mtk_set_frame_interval(struct xhci_hcd_mtk *mtk)
writel(value, hcd->regs + SS_GEN2_EOF_CFG);
}
+/*
+ * workaround: usb3.2 gen1 isoc rx hw issue
+ * host send out unexpected ACK afer device fininsh a burst transfer with
+ * a short packet.
+ */
+static void xhci_mtk_rxfifo_depth_set(struct xhci_hcd_mtk *mtk)
+{
+ struct usb_hcd *hcd = mtk->hcd;
+ u32 value;
+
+ if (!mtk->rxfifo_depth)
+ return;
+
+ value = readl(hcd->regs + HSCH_CFG1);
+ value &= ~SCH3_RXFIFO_DEPTH_MASK;
+ value |= FIELD_PREP(SCH3_RXFIFO_DEPTH_MASK, mtk->rxfifo_depth - 1);
+ writel(value, hcd->regs + HSCH_CFG1);
+}
+
+static void xhci_mtk_init_quirk(struct xhci_hcd_mtk *mtk)
+{
+ /* workaround only for mt8195 */
+ xhci_mtk_set_frame_interval(mtk);
+
+ /* workaround for SoCs using SSUSB about before IPM v1.6.0 */
+ xhci_mtk_rxfifo_depth_set(mtk);
+}
+
static int xhci_mtk_host_enable(struct xhci_hcd_mtk *mtk)
{
struct mu3c_ippc_regs __iomem *ippc = mtk->ippc_regs;
@@ -448,8 +480,7 @@ static int xhci_mtk_setup(struct usb_hcd *hcd)
if (ret)
return ret;
- /* workaround only for mt8195 */
- xhci_mtk_set_frame_interval(mtk);
+ xhci_mtk_init_quirk(mtk);
}
ret = xhci_gen_setup(hcd, xhci_mtk_quirks);
@@ -527,6 +558,8 @@ static int xhci_mtk_probe(struct platform_device *pdev)
of_property_read_u32(node, "mediatek,u2p-dis-msk",
&mtk->u2p_dis_msk);
+ of_property_read_u32(node, "rx-fifo-depth", &mtk->rxfifo_depth);
+
ret = usb_wakeup_of_property_parse(mtk, node);
if (ret) {
dev_err(dev, "failed to parse uwk property\n");
diff --git a/drivers/usb/host/xhci-mtk.h b/drivers/usb/host/xhci-mtk.h
index 39f7ae7d3087..f5e2bd66bb1b 100644
--- a/drivers/usb/host/xhci-mtk.h
+++ b/drivers/usb/host/xhci-mtk.h
@@ -171,6 +171,8 @@ struct xhci_hcd_mtk {
struct regmap *uwk;
u32 uwk_reg_base;
u32 uwk_vers;
+ /* quirk */
+ u32 rxfifo_depth;
};
static inline struct xhci_hcd_mtk *hcd_to_mtk(struct usb_hcd *hcd)
--
2.18.0
On Wed, Dec 20, 2023 at 10:58:41AM +0800, Chunfeng Yun wrote:
> For Gen1 isoc-in transfer, host still send out unexpected ACK after device
> finish the burst with a short packet, this will cause an exception on the
> connected device, such as, a usb 4k camera.
> It can be fixed by setting rxfifo depth less than 4k bytes, prefer to use
> 3k here, the side-effect is that may cause performance drop about 10%,
> including bulk transfer.
>
> Fixes: 926d60ae64a6 ("usb: xhci-mtk: modify the SOF/ITP interval for mt8195")
> Cc: [email protected]
> Signed-off-by: Chunfeng Yun <[email protected]>
> ---
> v3:
> add Cc stable
Why is a patch that you are marking for stable inclusion (and I am
guessing inclusion in 6.7-final) in the middle of other patches that are
not marked as such?
Always split out bugfixes from other things so that they can go through
the two different branches, one for this current release, and one for
the next one.
Otherwise you will have to wait until 6.8-rc1 for this bugfix to land,
which I doubt you intend to have happen.
Please fix up and resend 2 different patch series.
thanks,
greg k-h
On 20/12/2023 03:58, Chunfeng Yun wrote:
> For Gen1 isoc-in endpoint on controller before about SSUSB IPM v1.6.0, it
> still send out unexpected ACK after receiving a short packet in burst
> transfer, this will cause an exception on connected device, specially for
> a 4k camera.
> Add a quirk property "rx-fifo-depth" to work around this hardware issue,
> prefer to use 3k bytes;
> The side-effect is that may cause performance drop about 10%, including
> bulk transfer.
>
> Signed-off-by: Chunfeng Yun <[email protected]>
> ---
> v3: add fifo depth unit, change the value range from 0-3 to 1-4
> v2: change 'mediatek,rxfifo-depth' to 'rx-fifo-depth'
> ---
> .../devicetree/bindings/usb/mediatek,mtk-xhci.yaml | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.yaml b/Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.yaml
> index e9644e333d78..9478b7031796 100644
> --- a/Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.yaml
> +++ b/Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.yaml
> @@ -124,6 +124,18 @@ properties:
> defined in the xHCI spec on MTK's controller.
> default: 5000
>
> + rx-fifo-depth:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description:
> + It is a quirk used to work around Gen1 isoc-in endpoint transfer issue
> + that still send out unexpected ACK after device finish the burst transfer
> + with a short packet and cause an exception, specially on a 4K camera
> + device, it happens on controller before about IPM v1.6.0; the side-effect
> + is that may cause performance drop about 10%, include bulk transfer,
> + prefer to use 3 here. The unit is 1K bytes.
NAK. Read comments on previous submission.
Best regards,
Krzysztof
Il 20/12/23 03:58, Chunfeng Yun ha scritto:
> Add the quirk property "rx-fifo-depth" to work around Gen1 isoc-in
> transfer issue which send out unexpected ACK even after device
> already finished the burst transfer with a short patcket, specially
> for a 4K camera device.
>
> Signed-off-by: Chunfeng Yun <[email protected]>
> ---
> v3: change value according to binding
> v2: use 'rx-fifo-depth' property
> ---
> arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi b/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi
> index dd5b89b73190..58593348b810 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi
> @@ -1185,6 +1185,7 @@
>
Can you please place rx-fifo-depth before power supplies?
after which,
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
Thanks,
Angelo
> vusb33-supply = <&mt6359_vusb_ldo_reg>;
> vbus-supply = <&usb_vbus>;
> + rx-fifo-depth = <3>;
> };
>
> &xhci1 {
> @@ -1192,6 +1193,7 @@
>
> vusb33-supply = <&mt6359_vusb_ldo_reg>;
> vbus-supply = <&usb_vbus>;
> + rx-fifo-depth = <3>;
> };
>
> &xhci2 {
Il 20/12/23 03:58, Chunfeng Yun ha scritto:
> For Gen1 isoc-in transfer, host still send out unexpected ACK after device
> finish the burst with a short packet, this will cause an exception on the
> connected device, such as, a usb 4k camera.
> It can be fixed by setting rxfifo depth less than 4k bytes, prefer to use
> 3k here, the side-effect is that may cause performance drop about 10%,
> including bulk transfer.
>
> Fixes: 926d60ae64a6 ("usb: xhci-mtk: modify the SOF/ITP interval for mt8195")
> Cc: [email protected]
> Signed-off-by: Chunfeng Yun <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
Il 20/12/23 08:38, Krzysztof Kozlowski ha scritto:
> On 20/12/2023 03:58, Chunfeng Yun wrote:
>> For Gen1 isoc-in endpoint on controller before about SSUSB IPM v1.6.0, it
>> still send out unexpected ACK after receiving a short packet in burst
>> transfer, this will cause an exception on connected device, specially for
>> a 4k camera.
>> Add a quirk property "rx-fifo-depth" to work around this hardware issue,
>> prefer to use 3k bytes;
>> The side-effect is that may cause performance drop about 10%, including
>> bulk transfer.
>>
>> Signed-off-by: Chunfeng Yun <[email protected]>
>> ---
>> v3: add fifo depth unit, change the value range from 0-3 to 1-4
>> v2: change 'mediatek,rxfifo-depth' to 'rx-fifo-depth'
>> ---
>> .../devicetree/bindings/usb/mediatek,mtk-xhci.yaml | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.yaml b/Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.yaml
>> index e9644e333d78..9478b7031796 100644
>> --- a/Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.yaml
>> +++ b/Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.yaml
>> @@ -124,6 +124,18 @@ properties:
>> defined in the xHCI spec on MTK's controller.
>> default: 5000
>>
>> + rx-fifo-depth:
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + description:
>> + It is a quirk used to work around Gen1 isoc-in endpoint transfer issue
>> + that still send out unexpected ACK after device finish the burst transfer
>> + with a short packet and cause an exception, specially on a 4K camera
>> + device, it happens on controller before about IPM v1.6.0; the side-effect
>> + is that may cause performance drop about 10%, include bulk transfer,
>> + prefer to use 3 here. The unit is 1K bytes.
>
> NAK. Read comments on previous submission.
>
> Best regards,
> Krzysztof
>
Chunfeng, I think the discussion was not clear for you, so I will try to give
you a different explanation: this should be expressed in bytes, so 1000, or 1024,
2048, 4096, etc, and not 1/2/3/4/5/n.
The driver shall then validate and map your bytes number to hardware register
value and subsequently write to the registers.
Cheers,
Angleo
On Wed, 2023-12-20 at 06:57 +0100, Greg Kroah-Hartman wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> On Wed, Dec 20, 2023 at 10:58:41AM +0800, Chunfeng Yun wrote:
> > For Gen1 isoc-in transfer, host still send out unexpected ACK after
> device
> > finish the burst with a short packet, this will cause an exception
> on the
> > connected device, such as, a usb 4k camera.
> > It can be fixed by setting rxfifo depth less than 4k bytes, prefer
> to use
> > 3k here, the side-effect is that may cause performance drop about
> 10%,
> > including bulk transfer.
> >
> > Fixes: 926d60ae64a6 ("usb: xhci-mtk: modify the SOF/ITP interval
> for mt8195")
> > Cc: [email protected]
> > Signed-off-by: Chunfeng Yun <[email protected]>
> > ---
> > v3:
> > add Cc stable
>
> Why is a patch that you are marking for stable inclusion (and I am
> guessing inclusion in 6.7-final) in the middle of other patches that
> are
> not marked as such?
I'll remove Cc.
>
> Always split out bugfixes from other things so that they can go
> through
> the two different branches, one for this current release, and one for
> the next one.
>
> Otherwise you will have to wait until 6.8-rc1 for this bugfix to
> land,
> which I doubt you intend to have happen.
>
> Please fix up and resend 2 different patch series.
Send v3 today, and modify property;
Thanks a lot
>
> thanks,
>
> greg k-h
On Wed, 2023-12-20 at 11:30 +0100, AngeloGioacchino Del Regno wrote:
> Il 20/12/23 03:58, Chunfeng Yun ha scritto:
> > Add the quirk property "rx-fifo-depth" to work around Gen1 isoc-in
> > transfer issue which send out unexpected ACK even after device
> > already finished the burst transfer with a short patcket, specially
> > for a 4K camera device.
> >
> > Signed-off-by: Chunfeng Yun <[email protected]>
> > ---
> > v3: change value according to binding
> > v2: use 'rx-fifo-depth' property
> > ---
> > arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi
> > b/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi
> > index dd5b89b73190..58593348b810 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi
> > +++ b/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi
> > @@ -1185,6 +1185,7 @@
> >
>
> Can you please place rx-fifo-depth before power supplies?
Ok, thanks
>
> after which,
>
> Reviewed-by: AngeloGioacchino Del Regno <
> [email protected]>
>
> Thanks,
> Angelo
>
> > vusb33-supply = <&mt6359_vusb_ldo_reg>;
> > vbus-supply = <&usb_vbus>;
> > + rx-fifo-depth = <3>;
> > };
> >
> > &xhci1 {
> > @@ -1192,6 +1193,7 @@
> >
> > vusb33-supply = <&mt6359_vusb_ldo_reg>;
> > vbus-supply = <&usb_vbus>;
> > + rx-fifo-depth = <3>;
> > };
> >
> > &xhci2 {
>
>
On Wed, 2023-12-20 at 11:40 +0100, AngeloGioacchino Del Regno wrote:
> Il 20/12/23 08:38, Krzysztof Kozlowski ha scritto:
> > On 20/12/2023 03:58, Chunfeng Yun wrote:
> > > For Gen1 isoc-in endpoint on controller before about SSUSB IPM
> > > v1.6.0, it
> > > still send out unexpected ACK after receiving a short packet in
> > > burst
> > > transfer, this will cause an exception on connected device,
> > > specially for
> > > a 4k camera.
> > > Add a quirk property "rx-fifo-depth" to work around this hardware
> > > issue,
> > > prefer to use 3k bytes;
> > > The side-effect is that may cause performance drop about 10%,
> > > including
> > > bulk transfer.
> > >
> > > Signed-off-by: Chunfeng Yun <[email protected]>
> > > ---
> > > v3: add fifo depth unit, change the value range from 0-3 to 1-4
> > > v2: change 'mediatek,rxfifo-depth' to 'rx-fifo-depth'
> > > ---
> > > .../devicetree/bindings/usb/mediatek,mtk-xhci.yaml | 12
> > > ++++++++++++
> > > 1 file changed, 12 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/usb/mediatek,mtk-
> > > xhci.yaml b/Documentation/devicetree/bindings/usb/mediatek,mtk-
> > > xhci.yaml
> > > index e9644e333d78..9478b7031796 100644
> > > --- a/Documentation/devicetree/bindings/usb/mediatek,mtk-
> > > xhci.yaml
> > > +++ b/Documentation/devicetree/bindings/usb/mediatek,mtk-
> > > xhci.yaml
> > > @@ -124,6 +124,18 @@ properties:
> > > defined in the xHCI spec on MTK's controller.
> > > default: 5000
> > >
> > > + rx-fifo-depth:
> > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > + description:
> > > + It is a quirk used to work around Gen1 isoc-in endpoint
> > > transfer issue
> > > + that still send out unexpected ACK after device finish the
> > > burst transfer
> > > + with a short packet and cause an exception, specially on a
> > > 4K camera
> > > + device, it happens on controller before about IPM v1.6.0;
> > > the side-effect
> > > + is that may cause performance drop about 10%, include bulk
> > > transfer,
> > > + prefer to use 3 here. The unit is 1K bytes.
> >
> > NAK. Read comments on previous submission.
> >
> > Best regards,
> > Krzysztof
> >
>
> Chunfeng, I think the discussion was not clear for you, so I will try
> to give
> you a different explanation: this should be expressed in bytes, so
> 1000, or 1024,
> 2048, 4096, etc, and not 1/2/3/4/5/n.
>
> The driver shall then validate and map your bytes number to hardware
> register
> value and subsequently write to the registers.
Got it, I left out Krzysztof's comments when sent v3 series,
Thanks a lot
>
> Cheers,
> Angleo
On Wed, 2023-12-20 at 08:38 +0100, Krzysztof Kozlowski wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> On 20/12/2023 03:58, Chunfeng Yun wrote:
> > For Gen1 isoc-in endpoint on controller before about SSUSB IPM
> v1.6.0, it
> > still send out unexpected ACK after receiving a short packet in
> burst
> > transfer, this will cause an exception on connected device,
> specially for
> > a 4k camera.
> > Add a quirk property "rx-fifo-depth" to work around this hardware
> issue,
> > prefer to use 3k bytes;
> > The side-effect is that may cause performance drop about 10%,
> including
> > bulk transfer.
> >
> > Signed-off-by: Chunfeng Yun <[email protected]>
> > ---
> > v3: add fifo depth unit, change the value range from 0-3 to 1-4
> > v2: change 'mediatek,rxfifo-depth' to 'rx-fifo-depth'
> > ---
> > .../devicetree/bindings/usb/mediatek,mtk-xhci.yaml | 12
> ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/mediatek,mtk-
> xhci.yaml b/Documentation/devicetree/bindings/usb/mediatek,mtk-
> xhci.yaml
> > index e9644e333d78..9478b7031796 100644
> > --- a/Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.yaml
> > +++ b/Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.yaml
> > @@ -124,6 +124,18 @@ properties:
> > defined in the xHCI spec on MTK's controller.
> > default: 5000
> >
> > + rx-fifo-depth:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + description:
> > + It is a quirk used to work around Gen1 isoc-in endpoint
> transfer issue
> > + that still send out unexpected ACK after device finish the
> burst transfer
> > + with a short packet and cause an exception, specially on a
> 4K camera
> > + device, it happens on controller before about IPM v1.6.0;
> the side-effect
> > + is that may cause performance drop about 10%, include bulk
> transfer,
> > + prefer to use 3 here. The unit is 1K bytes.
>
> NAK. Read comments on previous submission.
Ok, will do it today
Thanks a lot
>
> Best regards,
> Krzysztof
>