Patch modifies the TD_SIZE in TRB before ZLP TRB.
The TD_SIZE in TRB before ZLP TRB must be set to 1 to force
processing ZLP TRB by controller.
cc: <[email protected]>
Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver")
Signed-off-by: Pawel Laszczak <[email protected]>
---
Changelog:
v2:
- returned value for last TRB must be 0
drivers/usb/cdns3/cdnsp-ring.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c
index 04dfcaa08dc4..aa79bce89d8a 100644
--- a/drivers/usb/cdns3/cdnsp-ring.c
+++ b/drivers/usb/cdns3/cdnsp-ring.c
@@ -1769,8 +1769,13 @@ static u32 cdnsp_td_remainder(struct cdnsp_device *pdev,
/* One TRB with a zero-length data packet. */
if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
- trb_buff_len == td_total_len)
+ trb_buff_len == td_total_len) {
+ /* Before ZLP driver needs set TD_SIZE=1. */
+ if (more_trbs_coming)
+ return 1;
+
return 0;
+ }
maxp = usb_endpoint_maxp(preq->pep->endpoint.desc);
total_packet_count = DIV_ROUND_UP(td_total_len, maxp);
--
2.25.1
On 22-10-24 10:04:35, Pawel Laszczak wrote:
> Patch modifies the TD_SIZE in TRB before ZLP TRB.
> The TD_SIZE in TRB before ZLP TRB must be set to 1 to force
> processing ZLP TRB by controller.
>
> cc: <[email protected]>
> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver")
> Signed-off-by: Pawel Laszczak <[email protected]>
>
> ---
> Changelog:
> v2:
> - returned value for last TRB must be 0
>
> drivers/usb/cdns3/cdnsp-ring.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c
> index 04dfcaa08dc4..aa79bce89d8a 100644
> --- a/drivers/usb/cdns3/cdnsp-ring.c
> +++ b/drivers/usb/cdns3/cdnsp-ring.c
> @@ -1769,8 +1769,13 @@ static u32 cdnsp_td_remainder(struct cdnsp_device *pdev,
>
> /* One TRB with a zero-length data packet. */
> if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
> - trb_buff_len == td_total_len)
> + trb_buff_len == td_total_len) {
> + /* Before ZLP driver needs set TD_SIZE=1. */
> + if (more_trbs_coming)
> + return 1;
> +
> return 0;
> + }
Does that fix the issue you want at bulk transfer, which has zero-length
packet at the last packet? It seems not align with your previous fix.
Would you mind explaining more?
>
> maxp = usb_endpoint_maxp(preq->pep->endpoint.desc);
> total_packet_count = DIV_ROUND_UP(td_total_len, maxp);
> --
> 2.25.1
>
--
Thanks,
Peter Chen
>
>On 22-10-24 10:04:35, Pawel Laszczak wrote:
>> Patch modifies the TD_SIZE in TRB before ZLP TRB.
>> The TD_SIZE in TRB before ZLP TRB must be set to 1 to force processing
>> ZLP TRB by controller.
>>
>> cc: <[email protected]>
>> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence
>> USBSSP DRD Driver")
>> Signed-off-by: Pawel Laszczak <[email protected]>
>>
>> ---
>> Changelog:
>> v2:
>> - returned value for last TRB must be 0
>>
>> drivers/usb/cdns3/cdnsp-ring.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/cdns3/cdnsp-ring.c
>> b/drivers/usb/cdns3/cdnsp-ring.c index 04dfcaa08dc4..aa79bce89d8a
>> 100644
>> --- a/drivers/usb/cdns3/cdnsp-ring.c
>> +++ b/drivers/usb/cdns3/cdnsp-ring.c
>> @@ -1769,8 +1769,13 @@ static u32 cdnsp_td_remainder(struct
>> cdnsp_device *pdev,
>>
>> /* One TRB with a zero-length data packet. */
>> if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
>> - trb_buff_len == td_total_len)
>> + trb_buff_len == td_total_len) {
>> + /* Before ZLP driver needs set TD_SIZE=1. */
>> + if (more_trbs_coming)
>> + return 1;
>> +
>> return 0;
>> + }
>
>Does that fix the issue you want at bulk transfer, which has zero-length packet
>at the last packet? It seems not align with your previous fix.
>Would you mind explaining more?
Value returned by function cdnsp_td_remainder is used
as TD_SIZE in TRB.
The last TRB in TD should have TD_SIZE=0, so trb for ZLP should have
set also TD_SIZE=0. If driver set TD_SIZE=0 on before the last one
TRB then the controller stops the transfer and ignore trb for ZLP packet.
To fix this, the driver in such case must set TD_SIZE = 1
before the last TRB.
e.g.
TD -> TRB1 transfer_length = 64KB, TD_SIZE =0
TRB2 transfer_length =0, TD_SIZE = 0 - controller will
ignore this transfer and stop transfer on previous one
TD -> TRB1 transfer_length = 64KB, TD_SIZE =1
TRB2 transfer_length =0, TD_SIZE = 0 - controller will
execute this trb and send ZLP
As you noticed previously, previous fix for last TRB returned
TD_SIZE = 1 in some cases.
Previous fix was working correct but was not compliance with
controller specification.
>
>>
>> maxp = usb_endpoint_maxp(preq->pep->endpoint.desc);
>> total_packet_count = DIV_ROUND_UP(td_total_len, maxp);
>> --
>> 2.25.1
>>
>
>--
>
Thanks,
Pawel Laszczak
On 22-10-27 08:46:17, Pawel Laszczak wrote:
>
> >
> >On 22-10-24 10:04:35, Pawel Laszczak wrote:
> >> Patch modifies the TD_SIZE in TRB before ZLP TRB.
> >> The TD_SIZE in TRB before ZLP TRB must be set to 1 to force processing
> >> ZLP TRB by controller.
> >>
> >> cc: <[email protected]>
> >> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence
> >> USBSSP DRD Driver")
> >> Signed-off-by: Pawel Laszczak <[email protected]>
> >>
> >> ---
> >> Changelog:
> >> v2:
> >> - returned value for last TRB must be 0
> >>
> >> drivers/usb/cdns3/cdnsp-ring.c | 7 ++++++-
> >> 1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/usb/cdns3/cdnsp-ring.c
> >> b/drivers/usb/cdns3/cdnsp-ring.c index 04dfcaa08dc4..aa79bce89d8a
> >> 100644
> >> --- a/drivers/usb/cdns3/cdnsp-ring.c
> >> +++ b/drivers/usb/cdns3/cdnsp-ring.c
> >> @@ -1769,8 +1769,13 @@ static u32 cdnsp_td_remainder(struct
> >> cdnsp_device *pdev,
> >>
> >> /* One TRB with a zero-length data packet. */
> >> if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
> >> - trb_buff_len == td_total_len)
> >> + trb_buff_len == td_total_len) {
> >> + /* Before ZLP driver needs set TD_SIZE=1. */
> >> + if (more_trbs_coming)
> >> + return 1;
> >> +
> >> return 0;
> >> + }
> >
> >Does that fix the issue you want at bulk transfer, which has zero-length packet
> >at the last packet? It seems not align with your previous fix.
> >Would you mind explaining more?
>
> Value returned by function cdnsp_td_remainder is used
> as TD_SIZE in TRB.
>
> The last TRB in TD should have TD_SIZE=0, so trb for ZLP should have
> set also TD_SIZE=0. If driver set TD_SIZE=0 on before the last one
> TRB then the controller stops the transfer and ignore trb for ZLP packet.
>
> To fix this, the driver in such case must set TD_SIZE = 1
> before the last TRB.
if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
- trb_buff_len == td_total_len)
+ trb_buff_len == td_total_len) {
+ /* Before ZLP driver needs set TD_SIZE=1. */
+ if (more_trbs_coming)
+ return 1;
+
return 0;
+ }
How your above fix could return TD_SIZE as 1 for last non-ZLP TRB?
Which conditions are satisfied?
Peter
> e.g.
>
> TD -> TRB1 transfer_length = 64KB, TD_SIZE =0
> TRB2 transfer_length =0, TD_SIZE = 0 - controller will
> ignore this transfer and stop transfer on previous one
>
> TD -> TRB1 transfer_length = 64KB, TD_SIZE =1
> TRB2 transfer_length =0, TD_SIZE = 0 - controller will
> execute this trb and send ZLP
>
> As you noticed previously, previous fix for last TRB returned
> TD_SIZE = 1 in some cases.
> Previous fix was working correct but was not compliance with
> controller specification.
>
> >
> >>
> >> maxp = usb_endpoint_maxp(preq->pep->endpoint.desc);
> >> total_packet_count = DIV_ROUND_UP(td_total_len, maxp);
> >> --
> >> 2.25.1
> >>
> >
> >--
> >
>
> Thanks,
> Pawel Laszczak
--
Thanks,
Peter Chen
>
>
>On 22-10-27 08:46:17, Pawel Laszczak wrote:
>>
>> >
>> >On 22-10-24 10:04:35, Pawel Laszczak wrote:
>> >> Patch modifies the TD_SIZE in TRB before ZLP TRB.
>> >> The TD_SIZE in TRB before ZLP TRB must be set to 1 to force
>> >> processing ZLP TRB by controller.
>> >>
>> >> cc: <[email protected]>
>> >> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence
>> >> USBSSP DRD Driver")
>> >> Signed-off-by: Pawel Laszczak <[email protected]>
>> >>
>> >> ---
>> >> Changelog:
>> >> v2:
>> >> - returned value for last TRB must be 0
>> >>
>> >> drivers/usb/cdns3/cdnsp-ring.c | 7 ++++++-
>> >> 1 file changed, 6 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/usb/cdns3/cdnsp-ring.c
>> >> b/drivers/usb/cdns3/cdnsp-ring.c index 04dfcaa08dc4..aa79bce89d8a
>> >> 100644
>> >> --- a/drivers/usb/cdns3/cdnsp-ring.c
>> >> +++ b/drivers/usb/cdns3/cdnsp-ring.c
>> >> @@ -1769,8 +1769,13 @@ static u32 cdnsp_td_remainder(struct
>> >> cdnsp_device *pdev,
>> >>
>> >> /* One TRB with a zero-length data packet. */
>> >> if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
>> >> - trb_buff_len == td_total_len)
>> >> + trb_buff_len == td_total_len) {
>> >> + /* Before ZLP driver needs set TD_SIZE=1. */
>> >> + if (more_trbs_coming)
>> >> + return 1;
>> >> +
>> >> return 0;
>> >> + }
>> >
>> >Does that fix the issue you want at bulk transfer, which has
>> >zero-length packet at the last packet? It seems not align with your previous
>fix.
>> >Would you mind explaining more?
>>
>> Value returned by function cdnsp_td_remainder is used as TD_SIZE in
>> TRB.
>>
>> The last TRB in TD should have TD_SIZE=0, so trb for ZLP should have
>> set also TD_SIZE=0. If driver set TD_SIZE=0 on before the last one TRB
>> then the controller stops the transfer and ignore trb for ZLP packet.
>>
>> To fix this, the driver in such case must set TD_SIZE = 1 before the
>> last TRB.
>
> if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
> - trb_buff_len == td_total_len)
> + trb_buff_len == td_total_len) {
> + /* Before ZLP driver needs set TD_SIZE=1. */
> + if (more_trbs_coming)
> + return 1;
> +
> return 0;
> + }
>
>How your above fix could return TD_SIZE as 1 for last non-ZLP TRB?
>Which conditions are satisfied?
For last non-ZLP TRB TD_SIZE should be 0 or 1.
We have three casess:
1.
TRB1 - length > 1
TRb2 - ZLP
In this case TRB1 should have set TD_SIZE = 1. In this case the condition
if (more_trbs_coming)
return 1;
returns TD_SIZE=1. In this case more_trb_comming for TRB1 is 1 and for TRB2 is 0
2.
TRB1 - length >1 and we don't except ZLP
In this case TD_SIZE should be set to 0 for TRB1 and function returns 0, more_trbs_comming for TRB1 will be set to 0.
3 More TRBs without ZLP:
e.g.
TRB1 - length > 0, more_trbs_comming = 1 - TD_SIZE > 0
TRB2 - length > 0, more_trbs_comming = 1 - TD_SIZE > 0
TRB3 - length >= 0, more_trbs_comming = 0 - TD_SIZE = 0
Pawel
>
>Peter
>
>> e.g.
>>
>> TD -> TRB1 transfer_length = 64KB, TD_SIZE =0
>> TRB2 transfer_length =0, TD_SIZE = 0 - controller will
>> ignore this transfer and stop transfer on previous one
>>
>> TD -> TRB1 transfer_length = 64KB, TD_SIZE =1
>> TRB2 transfer_length =0, TD_SIZE = 0 - controller will
>> execute this trb and send ZLP
>>
>> As you noticed previously, previous fix for last TRB returned TD_SIZE
>> = 1 in some cases.
>> Previous fix was working correct but was not compliance with
>> controller specification.
>>
>> >
>> >>
>> >> maxp = usb_endpoint_maxp(preq->pep->endpoint.desc);
>> >> total_packet_count = DIV_ROUND_UP(td_total_len, maxp);
>> >> --
>> >> 2.25.1
>> >>
>> >
>> >--
>> >
>>
>> Thanks,
>> Pawel Laszczak
>
>--
>
>Thanks,
>Peter Chen
Regards,
Pawel Laszczak
On Mon, Nov 7, 2022 at 1:39 PM Pawel Laszczak <[email protected]> wrote:
>
> >
> >
> >On 22-10-27 08:46:17, Pawel Laszczak wrote:
> >>
> >> >
> >> >On 22-10-24 10:04:35, Pawel Laszczak wrote:
> >> >> Patch modifies the TD_SIZE in TRB before ZLP TRB.
> >> >> The TD_SIZE in TRB before ZLP TRB must be set to 1 to force
> >> >> processing ZLP TRB by controller.
> >> >>
> >> >> cc: <[email protected]>
> >> >> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence
> >> >> USBSSP DRD Driver")
> >> >> Signed-off-by: Pawel Laszczak <[email protected]>
> >> >>
> >> >> ---
> >> >> Changelog:
> >> >> v2:
> >> >> - returned value for last TRB must be 0
> >> >>
> >> >> drivers/usb/cdns3/cdnsp-ring.c | 7 ++++++-
> >> >> 1 file changed, 6 insertions(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/drivers/usb/cdns3/cdnsp-ring.c
> >> >> b/drivers/usb/cdns3/cdnsp-ring.c index 04dfcaa08dc4..aa79bce89d8a
> >> >> 100644
> >> >> --- a/drivers/usb/cdns3/cdnsp-ring.c
> >> >> +++ b/drivers/usb/cdns3/cdnsp-ring.c
> >> >> @@ -1769,8 +1769,13 @@ static u32 cdnsp_td_remainder(struct
> >> >> cdnsp_device *pdev,
> >> >>
> >> >> /* One TRB with a zero-length data packet. */
> >> >> if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
> >> >> - trb_buff_len == td_total_len)
> >> >> + trb_buff_len == td_total_len) {
> >> >> + /* Before ZLP driver needs set TD_SIZE=1. */
> >> >> + if (more_trbs_coming)
> >> >> + return 1;
> >> >> +
> >> >> return 0;
> >> >> + }
> >> >
> >> >Does that fix the issue you want at bulk transfer, which has
> >> >zero-length packet at the last packet? It seems not align with your previous
> >fix.
> >> >Would you mind explaining more?
> >>
> >> Value returned by function cdnsp_td_remainder is used as TD_SIZE in
> >> TRB.
> >>
> >> The last TRB in TD should have TD_SIZE=0, so trb for ZLP should have
> >> set also TD_SIZE=0. If driver set TD_SIZE=0 on before the last one TRB
> >> then the controller stops the transfer and ignore trb for ZLP packet.
> >>
> >> To fix this, the driver in such case must set TD_SIZE = 1 before the
> >> last TRB.
> >
> > if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
> > - trb_buff_len == td_total_len)
> > + trb_buff_len == td_total_len) {
> > + /* Before ZLP driver needs set TD_SIZE=1. */
> > + if (more_trbs_coming)
> > + return 1;
> > +
> > return 0;
> > + }
> >
> >How your above fix could return TD_SIZE as 1 for last non-ZLP TRB?
> >Which conditions are satisfied?
>
> For last non-ZLP TRB TD_SIZE should be 0 or 1.
>
> We have three casess:
> 1.
> TRB1 - length > 1
> TRb2 - ZLP
>
> In this case TRB1 should have set TD_SIZE = 1. In this case the condition
> if (more_trbs_coming)
> return 1;
>
> returns TD_SIZE=1. In this case more_trb_comming for TRB1 is 1 and for TRB2 is 0
>
This one is my question. How below condition is true for your case 1:
if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
trb_buff_len == td_total_len)
Peter
>
> 2.
> TRB1 - length >1 and we don't except ZLP
>
> In this case TD_SIZE should be set to 0 for TRB1 and function returns 0, more_trbs_comming for TRB1 will be set to 0.
>
> 3 More TRBs without ZLP:
> e.g.
> TRB1 - length > 0, more_trbs_comming = 1 - TD_SIZE > 0
> TRB2 - length > 0, more_trbs_comming = 1 - TD_SIZE > 0
> TRB3 - length >= 0, more_trbs_comming = 0 - TD_SIZE = 0
>
> Pawel
>
> >
> >Peter
> >
> >> e.g.
> >>
> >> TD -> TRB1 transfer_length = 64KB, TD_SIZE =0
> >> TRB2 transfer_length =0, TD_SIZE = 0 - controller will
> >> ignore this transfer and stop transfer on previous one
> >>
> >> TD -> TRB1 transfer_length = 64KB, TD_SIZE =1
> >> TRB2 transfer_length =0, TD_SIZE = 0 - controller will
> >> execute this trb and send ZLP
> >>
> >> As you noticed previously, previous fix for last TRB returned TD_SIZE
> >> = 1 in some cases.
> >> Previous fix was working correct but was not compliance with
> >> controller specification.
> >>
> >> >
> >> >>
> >> >> maxp = usb_endpoint_maxp(preq->pep->endpoint.desc);
> >> >> total_packet_count = DIV_ROUND_UP(td_total_len, maxp);
> >> >> --
> >> >> 2.25.1
> >> >>
> >> >
> >> >--
> >> >
> >>
> >> Thanks,
> >> Pawel Laszczak
> >
> >--
> >
> >Thanks,
> >Peter Chen
>
> Regards,
> Pawel Laszczak
>On Mon, Nov 7, 2022 at 1:39 PM Pawel Laszczak <[email protected]>
>wrote:
>>
>> >
>> >
>> >On 22-10-27 08:46:17, Pawel Laszczak wrote:
>> >>
>> >> >
>> >> >On 22-10-24 10:04:35, Pawel Laszczak wrote:
>> >> >> Patch modifies the TD_SIZE in TRB before ZLP TRB.
>> >> >> The TD_SIZE in TRB before ZLP TRB must be set to 1 to force
>> >> >> processing ZLP TRB by controller.
>> >> >>
>> >> >> cc: <[email protected]>
>> >> >> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence
>> >> >> USBSSP DRD Driver")
>> >> >> Signed-off-by: Pawel Laszczak <[email protected]>
>> >> >>
>> >> >> ---
>> >> >> Changelog:
>> >> >> v2:
>> >> >> - returned value for last TRB must be 0
>> >> >>
>> >> >> drivers/usb/cdns3/cdnsp-ring.c | 7 ++++++-
>> >> >> 1 file changed, 6 insertions(+), 1 deletion(-)
>> >> >>
>> >> >> diff --git a/drivers/usb/cdns3/cdnsp-ring.c
>> >> >> b/drivers/usb/cdns3/cdnsp-ring.c index
>> >> >> 04dfcaa08dc4..aa79bce89d8a
>> >> >> 100644
>> >> >> --- a/drivers/usb/cdns3/cdnsp-ring.c
>> >> >> +++ b/drivers/usb/cdns3/cdnsp-ring.c
>> >> >> @@ -1769,8 +1769,13 @@ static u32 cdnsp_td_remainder(struct
>> >> >> cdnsp_device *pdev,
>> >> >>
>> >> >> /* One TRB with a zero-length data packet. */
>> >> >> if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
>> >> >> - trb_buff_len == td_total_len)
>> >> >> + trb_buff_len == td_total_len) {
>> >> >> + /* Before ZLP driver needs set TD_SIZE=1. */
>> >> >> + if (more_trbs_coming)
>> >> >> + return 1;
>> >> >> +
>> >> >> return 0;
>> >> >> + }
>> >> >
>> >> >Does that fix the issue you want at bulk transfer, which has
>> >> >zero-length packet at the last packet? It seems not align with
>> >> >your previous
>> >fix.
>> >> >Would you mind explaining more?
>> >>
>> >> Value returned by function cdnsp_td_remainder is used as TD_SIZE in
>> >> TRB.
>> >>
>> >> The last TRB in TD should have TD_SIZE=0, so trb for ZLP should
>> >> have set also TD_SIZE=0. If driver set TD_SIZE=0 on before the last
>> >> one TRB then the controller stops the transfer and ignore trb for ZLP
>packet.
>> >>
>> >> To fix this, the driver in such case must set TD_SIZE = 1 before
>> >> the last TRB.
>> >
>> > if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
>> > - trb_buff_len == td_total_len)
>> > + trb_buff_len == td_total_len) {
>> > + /* Before ZLP driver needs set TD_SIZE=1. */
>> > + if (more_trbs_coming)
>> > + return 1;
>> > +
>> > return 0;
>> > + }
>> >
>> >How your above fix could return TD_SIZE as 1 for last non-ZLP TRB?
>> >Which conditions are satisfied?
>>
>> For last non-ZLP TRB TD_SIZE should be 0 or 1.
>>
>> We have three casess:
>> 1.
>> TRB1 - length > 1
>> TRb2 - ZLP
>>
>> In this case TRB1 should have set TD_SIZE = 1. In this case the condition
>> if (more_trbs_coming)
>> return 1;
>>
>> returns TD_SIZE=1. In this case more_trb_comming for TRB1 is 1 and for
>> TRB2 is 0
>>
>
>This one is my question. How below condition is true for your case 1:
>
> if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
> trb_buff_len == td_total_len)
For TRB1:
more_trbs_coming = true
transferred == 0 && trb_buff_len == 0 - false - it does not matter in this case
trb_buff_len == td_total_len - true
so whole condition is true.
Because more_trb_comming = true so:
if (more_trbs_coming)
return 1;
returns TD_SIZE = 1
For TRB2 - ZLP:
more_trbs_coming = false
transferred == 0 && trb_buff_len == 0 - false - it does not matter in this case
trb_buff_len == td_total_len - true
Because more_trb_comming = false so function returns TD_SIZE = 0 for last ZLP trb.
Pawel
>
>Peter
>
>
>
>>
>> 2.
>> TRB1 - length >1 and we don't except ZLP
>>
>> In this case TD_SIZE should be set to 0 for TRB1 and function returns 0,
>more_trbs_comming for TRB1 will be set to 0.
>>
>> 3 More TRBs without ZLP:
>> e.g.
>> TRB1 - length > 0, more_trbs_comming = 1 - TD_SIZE > 0
>> TRB2 - length > 0, more_trbs_comming = 1 - TD_SIZE > 0
>> TRB3 - length >= 0, more_trbs_comming = 0 - TD_SIZE = 0
>>
>> Pawel
>>
>> >
>> >Peter
>> >
>> >> e.g.
>> >>
>> >> TD -> TRB1 transfer_length = 64KB, TD_SIZE =0
>> >> TRB2 transfer_length =0, TD_SIZE = 0 - controller will
>> >> ignore this transfer and stop transfer on previous
>> >> one
>> >>
>> >> TD -> TRB1 transfer_length = 64KB, TD_SIZE =1
>> >> TRB2 transfer_length =0, TD_SIZE = 0 - controller will
>> >> execute this trb and send ZLP
>> >>
>> >> As you noticed previously, previous fix for last TRB returned
>> >> TD_SIZE = 1 in some cases.
>> >> Previous fix was working correct but was not compliance with
>> >> controller specification.
>> >>
>> >> >
>> >> >>
>> >> >> maxp = usb_endpoint_maxp(preq->pep->endpoint.desc);
>> >> >> total_packet_count = DIV_ROUND_UP(td_total_len, maxp);
>> >> >> --
>> >> >> 2.25.1
>> >> >>
>> >> >
>> >> >--
>> >> >
>> >>
>> >> Thanks,
>> >> Pawel Laszczak
>> >
>> >--
>> >
>> >Thanks,
>> >Peter Chen
>>
>> Regards,
>> Pawel Laszczak
On Thu, Nov 10, 2022 at 1:38 PM Pawel Laszczak <[email protected]> wrote:
>
> >On Mon, Nov 7, 2022 at 1:39 PM Pawel Laszczak <[email protected]>
> >wrote:
> >>
> >> >
> >> >
> >> >On 22-10-27 08:46:17, Pawel Laszczak wrote:
> >> >>
> >> >> >
> >> >> >On 22-10-24 10:04:35, Pawel Laszczak wrote:
> >> >> >> Patch modifies the TD_SIZE in TRB before ZLP TRB.
> >> >> >> The TD_SIZE in TRB before ZLP TRB must be set to 1 to force
> >> >> >> processing ZLP TRB by controller.
> >> >> >>
> >> >> >> cc: <[email protected]>
> >> >> >> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence
> >> >> >> USBSSP DRD Driver")
> >> >> >> Signed-off-by: Pawel Laszczak <[email protected]>
> >> >> >>
> >> >> >> ---
> >> >> >> Changelog:
> >> >> >> v2:
> >> >> >> - returned value for last TRB must be 0
> >> >> >>
> >> >> >> drivers/usb/cdns3/cdnsp-ring.c | 7 ++++++-
> >> >> >> 1 file changed, 6 insertions(+), 1 deletion(-)
> >> >> >>
> >> >> >> diff --git a/drivers/usb/cdns3/cdnsp-ring.c
> >> >> >> b/drivers/usb/cdns3/cdnsp-ring.c index
> >> >> >> 04dfcaa08dc4..aa79bce89d8a
> >> >> >> 100644
> >> >> >> --- a/drivers/usb/cdns3/cdnsp-ring.c
> >> >> >> +++ b/drivers/usb/cdns3/cdnsp-ring.c
> >> >> >> @@ -1769,8 +1769,13 @@ static u32 cdnsp_td_remainder(struct
> >> >> >> cdnsp_device *pdev,
> >> >> >>
> >> >> >> /* One TRB with a zero-length data packet. */
> >> >> >> if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
> >> >> >> - trb_buff_len == td_total_len)
> >> >> >> + trb_buff_len == td_total_len) {
> >> >> >> + /* Before ZLP driver needs set TD_SIZE=1. */
> >> >> >> + if (more_trbs_coming)
> >> >> >> + return 1;
> >> >> >> +
> >> >> >> return 0;
> >> >> >> + }
> >> >> >
> >> >> >Does that fix the issue you want at bulk transfer, which has
> >> >> >zero-length packet at the last packet? It seems not align with
> >> >> >your previous
> >> >fix.
> >> >> >Would you mind explaining more?
> >> >>
> >> >> Value returned by function cdnsp_td_remainder is used as TD_SIZE in
> >> >> TRB.
> >> >>
> >> >> The last TRB in TD should have TD_SIZE=0, so trb for ZLP should
> >> >> have set also TD_SIZE=0. If driver set TD_SIZE=0 on before the last
> >> >> one TRB then the controller stops the transfer and ignore trb for ZLP
> >packet.
> >> >>
> >> >> To fix this, the driver in such case must set TD_SIZE = 1 before
> >> >> the last TRB.
> >> >
> >> > if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
> >> > - trb_buff_len == td_total_len)
> >> > + trb_buff_len == td_total_len) {
> >> > + /* Before ZLP driver needs set TD_SIZE=1. */
> >> > + if (more_trbs_coming)
> >> > + return 1;
> >> > +
> >> > return 0;
> >> > + }
> >> >
> >> >How your above fix could return TD_SIZE as 1 for last non-ZLP TRB?
> >> >Which conditions are satisfied?
> >>
> >> For last non-ZLP TRB TD_SIZE should be 0 or 1.
> >>
> >> We have three casess:
> >> 1.
> >> TRB1 - length > 1
> >> TRb2 - ZLP
> >>
> >> In this case TRB1 should have set TD_SIZE = 1. In this case the condition
> >> if (more_trbs_coming)
> >> return 1;
> >>
> >> returns TD_SIZE=1. In this case more_trb_comming for TRB1 is 1 and for
> >> TRB2 is 0
> >>
> >
> >This one is my question. How below condition is true for your case 1:
> >
> > if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
> > trb_buff_len == td_total_len)
>
> For TRB1:
> more_trbs_coming = true
> transferred == 0 && trb_buff_len == 0 - false - it does not matter in this case
> trb_buff_len == td_total_len - true
Why trb_buff_len equals to td_total_length? Considering the bulk
transfer with request length
larger than 64KB.
Peter
> so whole condition is true.
>
> Because more_trb_comming = true so:
> if (more_trbs_coming)
> return 1;
> returns TD_SIZE = 1
>
> For TRB2 - ZLP:
> more_trbs_coming = false
> transferred == 0 && trb_buff_len == 0 - false - it does not matter in this case
> trb_buff_len == td_total_len - true
>
> Because more_trb_comming = false so function returns TD_SIZE = 0 for last ZLP trb.
>
> Pawel
>On Thu, Nov 10, 2022 at 1:38 PM Pawel Laszczak <[email protected]>
>wrote:
>>
>> >On Mon, Nov 7, 2022 at 1:39 PM Pawel Laszczak <[email protected]>
>> >wrote:
>> >>
>> >> >
>> >> >
>> >> >On 22-10-27 08:46:17, Pawel Laszczak wrote:
>> >> >>
>> >> >> >
>> >> >> >On 22-10-24 10:04:35, Pawel Laszczak wrote:
>> >> >> >> Patch modifies the TD_SIZE in TRB before ZLP TRB.
>> >> >> >> The TD_SIZE in TRB before ZLP TRB must be set to 1 to force
>> >> >> >> processing ZLP TRB by controller.
>> >> >> >>
>> >> >> >> cc: <[email protected]>
>> >> >> >> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of
>> >> >> >> Cadence USBSSP DRD Driver")
>> >> >> >> Signed-off-by: Pawel Laszczak <[email protected]>
>> >> >> >>
>> >> >> >> ---
>> >> >> >> Changelog:
>> >> >> >> v2:
>> >> >> >> - returned value for last TRB must be 0
>> >> >> >>
>> >> >> >> drivers/usb/cdns3/cdnsp-ring.c | 7 ++++++-
>> >> >> >> 1 file changed, 6 insertions(+), 1 deletion(-)
>> >> >> >>
>> >> >> >> diff --git a/drivers/usb/cdns3/cdnsp-ring.c
>> >> >> >> b/drivers/usb/cdns3/cdnsp-ring.c index
>> >> >> >> 04dfcaa08dc4..aa79bce89d8a
>> >> >> >> 100644
>> >> >> >> --- a/drivers/usb/cdns3/cdnsp-ring.c
>> >> >> >> +++ b/drivers/usb/cdns3/cdnsp-ring.c
>> >> >> >> @@ -1769,8 +1769,13 @@ static u32 cdnsp_td_remainder(struct
>> >> >> >> cdnsp_device *pdev,
>> >> >> >>
>> >> >> >> /* One TRB with a zero-length data packet. */
>> >> >> >> if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0)
>||
>> >> >> >> - trb_buff_len == td_total_len)
>> >> >> >> + trb_buff_len == td_total_len) {
>> >> >> >> + /* Before ZLP driver needs set TD_SIZE=1. */
>> >> >> >> + if (more_trbs_coming)
>> >> >> >> + return 1;
>> >> >> >> +
>> >> >> >> return 0;
>> >> >> >> + }
>> >> >> >
>> >> >> >Does that fix the issue you want at bulk transfer, which has
>> >> >> >zero-length packet at the last packet? It seems not align with
>> >> >> >your previous
>> >> >fix.
>> >> >> >Would you mind explaining more?
>> >> >>
>> >> >> Value returned by function cdnsp_td_remainder is used as TD_SIZE
>> >> >> in TRB.
>> >> >>
>> >> >> The last TRB in TD should have TD_SIZE=0, so trb for ZLP should
>> >> >> have set also TD_SIZE=0. If driver set TD_SIZE=0 on before the
>> >> >> last one TRB then the controller stops the transfer and ignore
>> >> >> trb for ZLP
>> >packet.
>> >> >>
>> >> >> To fix this, the driver in such case must set TD_SIZE = 1 before
>> >> >> the last TRB.
>> >> >
>> >> > if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0)
>||
>> >> > - trb_buff_len == td_total_len)
>> >> > + trb_buff_len == td_total_len) {
>> >> > + /* Before ZLP driver needs set TD_SIZE=1. */
>> >> > + if (more_trbs_coming)
>> >> > + return 1;
>> >> > +
>> >> > return 0;
>> >> > + }
>> >> >
>> >> >How your above fix could return TD_SIZE as 1 for last non-ZLP TRB?
>> >> >Which conditions are satisfied?
>> >>
>> >> For last non-ZLP TRB TD_SIZE should be 0 or 1.
>> >>
>> >> We have three casess:
>> >> 1.
>> >> TRB1 - length > 1
>> >> TRb2 - ZLP
>> >>
>> >> In this case TRB1 should have set TD_SIZE = 1. In this case the condition
>> >> if (more_trbs_coming)
>> >> return 1;
>> >>
>> >> returns TD_SIZE=1. In this case more_trb_comming for TRB1 is 1 and
>> >> for
>> >> TRB2 is 0
>> >>
>> >
>> >This one is my question. How below condition is true for your case 1:
>> >
>> > if (!more_trbs_coming || (transferred == 0 && trb_buff_len == 0) ||
>> > trb_buff_len == td_total_len)
>>
>> For TRB1:
>> more_trbs_coming = true
>> transferred == 0 && trb_buff_len == 0 - false - it does not matter in this
>case
>> trb_buff_len == td_total_len - true
>
>Why trb_buff_len equals to td_total_length? Considering the bulk transfer
>with request length larger than 64KB.
>
You have right, there might still be a problem with ZLP.
I've posted the v3 implemented a little differently.
Thanks
Pawel
>
>> so whole condition is true.
>>
>> Because more_trb_comming = true so:
>> if (more_trbs_coming)
>> return 1;
>> returns TD_SIZE = 1
>>
>> For TRB2 - ZLP:
>> more_trbs_coming = false
>> transferred == 0 && trb_buff_len == 0 - false - it does not matter in this
>case
>> trb_buff_len == td_total_len - true
>>
>> Because more_trb_comming = false so function returns TD_SIZE = 0 for
>last ZLP trb.
>>
>> Pawel