| Message ID | 20231220025842.7082-1-chunfeng.yun@mediatek.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v3,1/3] dt-bindings: usb: mtk-xhci: add a property for Gen1 isoc-in transfer issue | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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: stable@vger.kernel.org > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > --- > 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 <chunfeng.yun@mediatek.com> > --- > 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 <chunfeng.yun@mediatek.com> > --- > 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 <angelogioacchino.delregno@collabora.com> 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: stable@vger.kernel.org > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
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 <chunfeng.yun@mediatek.com> >> --- >> 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: stable@vger.kernel.org > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > > --- > > 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 <chunfeng.yun@mediatek.com> > > --- > > 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 < > angelogioacchino.delregno@collabora.com> > > 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 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 <chunfeng.yun@mediatek.com> > > --- > > 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 >
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 <chunfeng.yun@mediatek.com> > > > --- > > > 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
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
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 <chunfeng.yun@mediatek.com> --- 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(+)