Re: [PATCH 2/2] usb: cdnsp: Fix lack of removing request from pending list.
On 21-04-19 09:53:11, Pawel Laszczak wrote:
> From: Pawel Laszczak
>
> Patch fixes lack of removing request from ep->pending_list on failure
> of the stop endpoint command. Driver even after failing this command
> must remove request from ep->pending_list.
> Without this fix driver can stuck in cdnsp_gadget_ep_disable function
> in loop:
> while (!list_empty(>pending_list)) {
> preq = next_request(>pending_list);
> cdnsp_ep_dequeue(pep, preq);
> }
>
> Signed-off-by: Pawel Laszczak
> ---
> drivers/usb/cdns3/cdnsp-gadget.c | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/cdns3/cdnsp-gadget.c
> b/drivers/usb/cdns3/cdnsp-gadget.c
> index 6182c9bc65de..1ca8c1777a5c 100644
> --- a/drivers/usb/cdns3/cdnsp-gadget.c
> +++ b/drivers/usb/cdns3/cdnsp-gadget.c
> @@ -424,17 +424,17 @@ int cdnsp_ep_enqueue(struct cdnsp_ep *pep, struct
> cdnsp_request *preq)
> int cdnsp_ep_dequeue(struct cdnsp_ep *pep, struct cdnsp_request *preq)
> {
> struct cdnsp_device *pdev = pep->pdev;
> - int ret;
> + int ret_stop = 0;
> + int ret_rem;
>
> trace_cdnsp_request_dequeue(preq);
>
> - if (GET_EP_CTX_STATE(pep->out_ctx) == EP_STATE_RUNNING) {
> - ret = cdnsp_cmd_stop_ep(pdev, pep);
> - if (ret)
> - return ret;
> - }
> + if (GET_EP_CTX_STATE(pep->out_ctx) == EP_STATE_RUNNING)
> + ret_stop = cdnsp_cmd_stop_ep(pdev, pep);
> +
> + ret_rem = cdnsp_remove_request(pdev, preq, pep);
One more blank space above, and add Fixed-by tag.
>
> - return cdnsp_remove_request(pdev, preq, pep);
> + return ret_rem ? ret_rem : ret_stop;
> }
>
> static void cdnsp_zero_in_ctx(struct cdnsp_device *pdev)
> --
> 2.25.1
>
--
Thanks,
Peter Chen
Re: [PATCH 1/2] usb: gadget: f_uac2: Stop endpoint before enabling it.
On 21-04-19 09:50:53, Pawel Laszczak wrote:
> From: Pawel Laszczak
>
> Patch adds disabling endpoint before enabling it during changing
> alternate setting. Lack of this functionality causes that in some
> cases uac2 queue the same request multiple time.
> Such situation can occur when host send set interface with
> alternate setting 1 twice.
>
> Signed-off-by: Pawel Laszczak
> ---
> drivers/usb/gadget/function/f_uac2.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_uac2.c
> b/drivers/usb/gadget/function/f_uac2.c
> index 9cc5c512a5cd..7d20a9d8a1b4 100644
> --- a/drivers/usb/gadget/function/f_uac2.c
> +++ b/drivers/usb/gadget/function/f_uac2.c
> @@ -890,17 +890,17 @@ afunc_set_alt(struct usb_function *fn, unsigned intf,
> unsigned alt)
> if (intf == uac2->as_out_intf) {
> uac2->as_out_alt = alt;
>
> + u_audio_stop_capture(>g_audio);
> +
> if (alt)
> ret = u_audio_start_capture(>g_audio);
> - else
> - u_audio_stop_capture(>g_audio);
> } else if (intf == uac2->as_in_intf) {
> uac2->as_in_alt = alt;
>
> + u_audio_stop_playback(>g_audio);
> +
> if (alt)
> ret = u_audio_start_playback(>g_audio);
> - else
> - u_audio_stop_playback(>g_audio);
> } else {
> dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
> return -EINVAL;
To avoid this, you may use prm->ep_enabled to judge if the endpoint has
already enabled.
--
Thanks,
Peter Chen
Re: [PATCH] usb: gadget: Fix double free of device descriptor pointers
On 21-04-19 12:57:20, Wesley Cheng wrote:
> From: Hemant Kumar
>
> Upon driver unbind usb_free_all_descriptors() function frees all
> speed descriptor pointers without setting them to NULL. In case
> gadget speed changes (i.e from super speed plus to super speed)
> after driver unbind only upto super speed descriptor pointers get
> populated. Super speed plus desc still holds the stale (already
> freed) pointer. Fix this issue by setting all descriptor pointers
> to NULL after freeing them in usb_free_all_descriptors().
>
> Signed-off-by: Hemant Kumar
> Signed-off-by: Wesley Cheng
> ---
> drivers/usb/gadget/config.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/usb/gadget/config.c b/drivers/usb/gadget/config.c
> index 2d11535..8bb2577 100644
> --- a/drivers/usb/gadget/config.c
> +++ b/drivers/usb/gadget/config.c
> @@ -194,9 +194,13 @@ EXPORT_SYMBOL_GPL(usb_assign_descriptors);
> void usb_free_all_descriptors(struct usb_function *f)
> {
> usb_free_descriptors(f->fs_descriptors);
> + f->fs_descriptors = NULL;
> usb_free_descriptors(f->hs_descriptors);
> + f->hs_descriptors = NULL;
> usb_free_descriptors(f->ss_descriptors);
> + f->ss_descriptors = NULL;
> usb_free_descriptors(f->ssp_descriptors);
> + f->ssp_descriptors = NULL;
> }
> EXPORT_SYMBOL_GPL(usb_free_all_descriptors);
>
Reviewed-by: Peter Chen
You may add Fixed-by tag, and cc to stable tree.
--
Thanks,
Peter Chen
Re: [PATCH] [v2] usb: cdns3: Fix runtime PM imbalance on error
On 21-04-12 13:49:07, Dinghao Liu wrote:
> When cdns3_gadget_start() fails, a pairing PM usage counter
> decrement is needed to keep the counter balanced.
>
> Signed-off-by: Dinghao Liu
Applied, thanks.
Peter
> ---
>
> Changelog:
>
> v2: - Use pm_runtime_put_sync() to decrease refcount.
> ---
> drivers/usb/cdns3/cdns3-gadget.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/cdns3/cdns3-gadget.c
> b/drivers/usb/cdns3/cdns3-gadget.c
> index 582bfeceedb4..a49fc68ec2ef 100644
> --- a/drivers/usb/cdns3/cdns3-gadget.c
> +++ b/drivers/usb/cdns3/cdns3-gadget.c
> @@ -3255,8 +3255,10 @@ static int __cdns3_gadget_init(struct cdns *cdns)
> pm_runtime_get_sync(cdns->dev);
>
> ret = cdns3_gadget_start(cdns);
> - if (ret)
> + if (ret) {
> + pm_runtime_put_sync(cdns->dev);
> return ret;
> + }
>
> /*
>* Because interrupt line can be shared with other components in
> --
> 2.17.1
>
--
Thanks,
Peter Chen
Re: [PATCH v2] usb: cdnsp: Fixes issue with Configure Endpoint command
On 21-04-07 08:36:29, Pawel Laszczak wrote:
> From: Pawel Laszczak
>
> Patch adds flag EP_UNCONFIGURED to detect whether endpoint was
> unconfigured. This flag is set in cdnsp_reset_device after Reset Device
> command. Among others this command disables all non control endpoints.
> Flag is used in cdnsp_gadget_ep_disable to protect controller against
> invoking Configure Endpoint command on disabled endpoint. Lack of this
> protection in some cases caused that Configure Endpoint command completed
> with Context State Error code completion.
>
> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD
> Driver")
> Signed-off-by: Pawel Laszczak
Pawel, it is a little late for v5.12, I apply it to v5.13-rc1 if you
don't mind.
Peter
>
> ---
> Changelog:
> v2:
> - removed useless blank line
> - changed the EP_UNCONFIGURED to limit changes in patch
>
> drivers/usb/cdns3/cdnsp-gadget.c | 17 -
> drivers/usb/cdns3/cdnsp-gadget.h | 1 +
> 2 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/cdns3/cdnsp-gadget.c
> b/drivers/usb/cdns3/cdnsp-gadget.c
> index d7d4bdd57f46..56707b6b0f57 100644
> --- a/drivers/usb/cdns3/cdnsp-gadget.c
> +++ b/drivers/usb/cdns3/cdnsp-gadget.c
> @@ -727,7 +727,7 @@ int cdnsp_reset_device(struct cdnsp_device *pdev)
>* are in Disabled state.
>*/
> for (i = 1; i < CDNSP_ENDPOINTS_NUM; ++i)
> - pdev->eps[i].ep_state |= EP_STOPPED;
> + pdev->eps[i].ep_state |= EP_STOPPED | EP_UNCONFIGURED;
>
> trace_cdnsp_handle_cmd_reset_dev(slot_ctx);
>
> @@ -942,6 +942,7 @@ static int cdnsp_gadget_ep_enable(struct usb_ep *ep,
>
> pep = to_cdnsp_ep(ep);
> pdev = pep->pdev;
> + pep->ep_state &= ~EP_UNCONFIGURED;
>
> if (dev_WARN_ONCE(pdev->dev, pep->ep_state & EP_ENABLED,
> "%s is already enabled\n", pep->name))
> @@ -1023,9 +1024,13 @@ static int cdnsp_gadget_ep_disable(struct usb_ep *ep)
> goto finish;
> }
>
> - cdnsp_cmd_stop_ep(pdev, pep);
> pep->ep_state |= EP_DIS_IN_RROGRESS;
> - cdnsp_cmd_flush_ep(pdev, pep);
> +
> + /* Endpoint was unconfigured by Reset Device command. */
> + if (!(pep->ep_state & EP_UNCONFIGURED)) {
> + cdnsp_cmd_stop_ep(pdev, pep);
> + cdnsp_cmd_flush_ep(pdev, pep);
> + }
>
> /* Remove all queued USB requests. */
> while (!list_empty(>pending_list)) {
> @@ -1043,10 +1048,12 @@ static int cdnsp_gadget_ep_disable(struct usb_ep *ep)
>
> cdnsp_endpoint_zero(pdev, pep);
>
> - ret = cdnsp_update_eps_configuration(pdev, pep);
> + if (!(pep->ep_state & EP_UNCONFIGURED))
> + ret = cdnsp_update_eps_configuration(pdev, pep);
> +
> cdnsp_free_endpoint_rings(pdev, pep);
>
> - pep->ep_state &= ~EP_ENABLED;
> + pep->ep_state &= ~(EP_ENABLED | EP_UNCONFIGURED);
> pep->ep_state |= EP_STOPPED;
>
> finish:
> diff --git a/drivers/usb/cdns3/cdnsp-gadget.h
> b/drivers/usb/cdns3/cdnsp-gadget.h
> index 6bbb26548c04..783ca8ffde00 100644
> --- a/drivers/usb/cdns3/cdnsp-gadget.h
> +++ b/drivers/usb/cdns3/cdnsp-gadget.h
> @@ -835,6 +835,7 @@ struct cdnsp_ep {
> #define EP_WEDGE BIT(4)
> #define EP0_HALTED_STATUSBIT(5)
> #define EP_HAS_STREAMS BIT(6)
> +#define EP_UNCONFIGURED BIT(7)
>
> bool skip;
> };
> --
> 2.25.1
>
--
Thanks,
Peter Chen
Re: [PATCH v2] usb: cdnsp: Fixes issue with Configure Endpoint command
On 21-04-07 08:36:29, Pawel Laszczak wrote:
> From: Pawel Laszczak
>
> Patch adds flag EP_UNCONFIGURED to detect whether endpoint was
> unconfigured. This flag is set in cdnsp_reset_device after Reset Device
> command. Among others this command disables all non control endpoints.
> Flag is used in cdnsp_gadget_ep_disable to protect controller against
> invoking Configure Endpoint command on disabled endpoint. Lack of this
> protection in some cases caused that Configure Endpoint command completed
> with Context State Error code completion.
>
> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD
> Driver")
> Signed-off-by: Pawel Laszczak
Pawel, it is a little later for v5.12. I queue it to v5.13-rc1 if you
don't mind.
Peter
>
> ---
> Changelog:
> v2:
> - removed useless blank line
> - changed the EP_UNCONFIGURED to limit changes in patch
>
> drivers/usb/cdns3/cdnsp-gadget.c | 17 -
> drivers/usb/cdns3/cdnsp-gadget.h | 1 +
> 2 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/cdns3/cdnsp-gadget.c
> b/drivers/usb/cdns3/cdnsp-gadget.c
> index d7d4bdd57f46..56707b6b0f57 100644
> --- a/drivers/usb/cdns3/cdnsp-gadget.c
> +++ b/drivers/usb/cdns3/cdnsp-gadget.c
> @@ -727,7 +727,7 @@ int cdnsp_reset_device(struct cdnsp_device *pdev)
>* are in Disabled state.
>*/
> for (i = 1; i < CDNSP_ENDPOINTS_NUM; ++i)
> - pdev->eps[i].ep_state |= EP_STOPPED;
> + pdev->eps[i].ep_state |= EP_STOPPED | EP_UNCONFIGURED;
>
> trace_cdnsp_handle_cmd_reset_dev(slot_ctx);
>
> @@ -942,6 +942,7 @@ static int cdnsp_gadget_ep_enable(struct usb_ep *ep,
>
> pep = to_cdnsp_ep(ep);
> pdev = pep->pdev;
> + pep->ep_state &= ~EP_UNCONFIGURED;
>
> if (dev_WARN_ONCE(pdev->dev, pep->ep_state & EP_ENABLED,
> "%s is already enabled\n", pep->name))
> @@ -1023,9 +1024,13 @@ static int cdnsp_gadget_ep_disable(struct usb_ep *ep)
> goto finish;
> }
>
> - cdnsp_cmd_stop_ep(pdev, pep);
> pep->ep_state |= EP_DIS_IN_RROGRESS;
> - cdnsp_cmd_flush_ep(pdev, pep);
> +
> + /* Endpoint was unconfigured by Reset Device command. */
> + if (!(pep->ep_state & EP_UNCONFIGURED)) {
> + cdnsp_cmd_stop_ep(pdev, pep);
> + cdnsp_cmd_flush_ep(pdev, pep);
> + }
>
> /* Remove all queued USB requests. */
> while (!list_empty(>pending_list)) {
> @@ -1043,10 +1048,12 @@ static int cdnsp_gadget_ep_disable(struct usb_ep *ep)
>
> cdnsp_endpoint_zero(pdev, pep);
>
> - ret = cdnsp_update_eps_configuration(pdev, pep);
> + if (!(pep->ep_state & EP_UNCONFIGURED))
> + ret = cdnsp_update_eps_configuration(pdev, pep);
> +
> cdnsp_free_endpoint_rings(pdev, pep);
>
> - pep->ep_state &= ~EP_ENABLED;
> + pep->ep_state &= ~(EP_ENABLED | EP_UNCONFIGURED);
> pep->ep_state |= EP_STOPPED;
>
> finish:
> diff --git a/drivers/usb/cdns3/cdnsp-gadget.h
> b/drivers/usb/cdns3/cdnsp-gadget.h
> index 6bbb26548c04..783ca8ffde00 100644
> --- a/drivers/usb/cdns3/cdnsp-gadget.h
> +++ b/drivers/usb/cdns3/cdnsp-gadget.h
> @@ -835,6 +835,7 @@ struct cdnsp_ep {
> #define EP_WEDGE BIT(4)
> #define EP0_HALTED_STATUSBIT(5)
> #define EP_HAS_STREAMS BIT(6)
> +#define EP_UNCONFIGURED BIT(7)
>
> bool skip;
> };
> --
> 2.25.1
>
--
Thanks,
Peter Chen
Re: [PATCH] usb: cdns3: Fix rumtime PM imbalance on error
On 21-04-07 13:22:26, Dinghao Liu wrote:
> When cdns3_gadget_start() fails, a pairing PM usage counter
> decrement is needed to keep the counter balanced.
>
> Signed-off-by: Dinghao Liu
> ---
> drivers/usb/cdns3/cdns3-gadget.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/cdns3/cdns3-gadget.c
> b/drivers/usb/cdns3/cdns3-gadget.c
> index 582bfeceedb4..ad891a108aed 100644
> --- a/drivers/usb/cdns3/cdns3-gadget.c
> +++ b/drivers/usb/cdns3/cdns3-gadget.c
> @@ -3255,8 +3255,11 @@ static int __cdns3_gadget_init(struct cdns *cdns)
> pm_runtime_get_sync(cdns->dev);
>
> ret = cdns3_gadget_start(cdns);
> - if (ret)
> + if (ret) {
> + pm_runtime_mark_last_busy(cdns->dev);
> + pm_runtime_put_autosuspend(cdns->dev);
> return ret;
It doesn't need to delay entering runtime suspend, I prefer using
pm_runtime_put_sync directly.
--
Thanks,
Peter Chen
Re: [PATCH] usb: cdnsp: Fixes issue with Configure Endpoint command
On 21-03-30 04:26:10, Pawel Laszczak wrote:
> Hi Peter,
>
> >
> >On 21-03-22 07:09:02, Pawel Laszczak wrote:
> >> From: Pawel Laszczak
> >>
> >> Patch adds flag EP_UNCONFIGURED to detect whether endpoint was
> >> unconfigured. This flag is set in cdnsp_reset_device after Reset Device
> >> command. Among others this command disables all non control endpoints.
> >> Flag is used in cdnsp_gadget_ep_disable to protect controller against
> >> invoking Configure Endpoint command on disabled endpoint. Lack of this
> >> protection in some cases caused that Configure Endpoint command completed
> >> with Context State Error code completion.
> >>
> >> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP
> >> DRD Driver")
> >> Signed-off-by: Pawel Laszczak
> >> ---
> >> drivers/usb/cdns3/cdnsp-gadget.c | 18 +-
> >> drivers/usb/cdns3/cdnsp-gadget.h | 11 ++-
> >> 2 files changed, 19 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/usb/cdns3/cdnsp-gadget.c
> >> b/drivers/usb/cdns3/cdnsp-gadget.c
> >> index d7d4bdd57f46..de17cc4ad91a 100644
> >> --- a/drivers/usb/cdns3/cdnsp-gadget.c
> >> +++ b/drivers/usb/cdns3/cdnsp-gadget.c
> >> @@ -727,7 +727,7 @@ int cdnsp_reset_device(struct cdnsp_device *pdev)
> >> * are in Disabled state.
> >> */
> >>for (i = 1; i < CDNSP_ENDPOINTS_NUM; ++i)
> >> - pdev->eps[i].ep_state |= EP_STOPPED;
> >> + pdev->eps[i].ep_state |= EP_STOPPED | EP_UNCONFIGURED;
> >>
> >>trace_cdnsp_handle_cmd_reset_dev(slot_ctx);
> >>
> >> @@ -942,6 +942,7 @@ static int cdnsp_gadget_ep_enable(struct usb_ep *ep,
> >>
> >>pep = to_cdnsp_ep(ep);
> >>pdev = pep->pdev;
> >> + pep->ep_state &= ~EP_UNCONFIGURED;
> >>
> >>if (dev_WARN_ONCE(pdev->dev, pep->ep_state & EP_ENABLED,
> >> "%s is already enabled\n", pep->name))
> >> @@ -1023,9 +1024,13 @@ static int cdnsp_gadget_ep_disable(struct usb_ep
> >> *ep)
> >>goto finish;
> >>}
> >>
> >> - cdnsp_cmd_stop_ep(pdev, pep);
> >>pep->ep_state |= EP_DIS_IN_RROGRESS;
> >> - cdnsp_cmd_flush_ep(pdev, pep);
> >> +
> >> + /* Endpoint was unconfigured by Reset Device command. */
> >> + if (!(pep->ep_state & EP_UNCONFIGURED)) {
> >> + cdnsp_cmd_stop_ep(pdev, pep);
> >> + cdnsp_cmd_flush_ep(pdev, pep);
> >> + }
> >>
> >>/* Remove all queued USB requests. */
> >>while (!list_empty(>pending_list)) {
> >> @@ -1036,6 +1041,7 @@ static int cdnsp_gadget_ep_disable(struct usb_ep *ep)
> >>cdnsp_invalidate_ep_events(pdev, pep);
> >>
> >>pep->ep_state &= ~EP_DIS_IN_RROGRESS;
> >> +
> >
> >Useless blank line
> >
> >>drop_flag = cdnsp_get_endpoint_flag(pep->endpoint.desc);
> >>ctrl_ctx = cdnsp_get_input_control_ctx(>in_ctx);
> >>ctrl_ctx->drop_flags = cpu_to_le32(drop_flag);
> >> @@ -1043,10 +1049,12 @@ static int cdnsp_gadget_ep_disable(struct usb_ep
> >> *ep)
> >>
> >>cdnsp_endpoint_zero(pdev, pep);
> >>
> >> - ret = cdnsp_update_eps_configuration(pdev, pep);
> >> + if (!(pep->ep_state & EP_UNCONFIGURED))
> >> + ret = cdnsp_update_eps_configuration(pdev, pep);
> >> +
> >>cdnsp_free_endpoint_rings(pdev, pep);
> >>
> >> - pep->ep_state &= ~EP_ENABLED;
> >> + pep->ep_state &= ~(EP_ENABLED | EP_UNCONFIGURED);
> >>pep->ep_state |= EP_STOPPED;
> >>
> >> finish:
> >> diff --git a/drivers/usb/cdns3/cdnsp-gadget.h
> >> b/drivers/usb/cdns3/cdnsp-gadget.h
> >> index 6bbb26548c04..e628bd539e23 100644
> >> --- a/drivers/usb/cdns3/cdnsp-gadget.h
> >> +++ b/drivers/usb/cdns3/cdnsp-gadget.h
> >> @@ -830,11 +830,12 @@ struct cdnsp_ep {
> >>unsigned int ep_state;
> >> #define EP_ENABLEDBIT(0)
> >> #define EP_DIS_IN_RROGRESS BIT(1)
> >> -#define EP_HALTED BIT(2)
> >> -#define EP_STOPPEDBIT(3)
> >> -#define EP_WEDGE BIT(4)
> >> -#define EP0_HALTED_STATUS BIT(5)
> >> -#define EP_HAS_STREAMSBIT(6)
> >> +#define EP_UNCONFIGURED BIT(2)
> >
> >Why add new flag as BIT(2), it causes many changes in this patch?
>
> In my feeling, EP_UNCONFIGURED is more associates with the first 2 flags, so
> I've decided
> put it after BIT(1).
No, you may not add such relationship, each flag has its own meaning,
otherwise, the sequence of flag bitmap may be changed again.
Peter
>
> >
> >> +#define EP_HALTED BIT(3)
> >> +#define EP_STOPPEDBIT(4)
> >> +#define EP_WEDGE BIT(5)
> >> +#define EP0_HALTED_STATUS BIT(6)
> >> +#define EP_HAS_STREAMSBIT(7)
> >>
> >>bool skip;
> >> };
> >> --
> >> 2.25.1
> >>
> >
> >--
> >
> >Thanks,
> >Peter Chen
>
--
Thanks,
Peter Chen
Re: [PATCH] usb: cdns3: delete repeated clear operations
On 21-03-22 07:19:46, Pawel Laszczak wrote: > Hi Peter, > > Can you add this patch to for-usb-next branch. > Feel free add your ACK base on this patch. Peter > Thanks. > > > > > > >dma_alloc_coherent already zeroes out memory, so memset is not needed. > > > >Signed-off-by: Wang Qing > > Reviewed-by: Pawel Laszczak > > >--- > > drivers/usb/cdns3/cdnsp-mem.c | 1 - > > 1 file changed, 1 deletion(-) > > > >diff --git a/drivers/usb/cdns3/cdnsp-mem.c b/drivers/usb/cdns3/cdnsp-mem.c > >index 7a84e92..1d1b9a4 > >--- a/drivers/usb/cdns3/cdnsp-mem.c > >+++ b/drivers/usb/cdns3/cdnsp-mem.c > >@@ -1231,7 +1231,6 @@ int cdnsp_mem_init(struct cdnsp_device *pdev) > > if (!pdev->dcbaa) > > return -ENOMEM; > > > >-memset(pdev->dcbaa, 0, sizeof(*pdev->dcbaa)); > > pdev->dcbaa->dma = dma; > > > > cdnsp_write_64(dma, >op_regs->dcbaa_ptr); > >-- > >2.7.4 > > Regards, > Pawel Laszczak -- Thanks, Peter Chen
Re: [PATCH] usb: cdnsp: Fixes issue with Configure Endpoint command
On 21-03-22 07:09:02, Pawel Laszczak wrote:
> From: Pawel Laszczak
>
> Patch adds flag EP_UNCONFIGURED to detect whether endpoint was
> unconfigured. This flag is set in cdnsp_reset_device after Reset Device
> command. Among others this command disables all non control endpoints.
> Flag is used in cdnsp_gadget_ep_disable to protect controller against
> invoking Configure Endpoint command on disabled endpoint. Lack of this
> protection in some cases caused that Configure Endpoint command completed
> with Context State Error code completion.
>
> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD
> Driver")
> Signed-off-by: Pawel Laszczak
> ---
> drivers/usb/cdns3/cdnsp-gadget.c | 18 +-
> drivers/usb/cdns3/cdnsp-gadget.h | 11 ++-
> 2 files changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/usb/cdns3/cdnsp-gadget.c
> b/drivers/usb/cdns3/cdnsp-gadget.c
> index d7d4bdd57f46..de17cc4ad91a 100644
> --- a/drivers/usb/cdns3/cdnsp-gadget.c
> +++ b/drivers/usb/cdns3/cdnsp-gadget.c
> @@ -727,7 +727,7 @@ int cdnsp_reset_device(struct cdnsp_device *pdev)
>* are in Disabled state.
>*/
> for (i = 1; i < CDNSP_ENDPOINTS_NUM; ++i)
> - pdev->eps[i].ep_state |= EP_STOPPED;
> + pdev->eps[i].ep_state |= EP_STOPPED | EP_UNCONFIGURED;
>
> trace_cdnsp_handle_cmd_reset_dev(slot_ctx);
>
> @@ -942,6 +942,7 @@ static int cdnsp_gadget_ep_enable(struct usb_ep *ep,
>
> pep = to_cdnsp_ep(ep);
> pdev = pep->pdev;
> + pep->ep_state &= ~EP_UNCONFIGURED;
>
> if (dev_WARN_ONCE(pdev->dev, pep->ep_state & EP_ENABLED,
> "%s is already enabled\n", pep->name))
> @@ -1023,9 +1024,13 @@ static int cdnsp_gadget_ep_disable(struct usb_ep *ep)
> goto finish;
> }
>
> - cdnsp_cmd_stop_ep(pdev, pep);
> pep->ep_state |= EP_DIS_IN_RROGRESS;
> - cdnsp_cmd_flush_ep(pdev, pep);
> +
> + /* Endpoint was unconfigured by Reset Device command. */
> + if (!(pep->ep_state & EP_UNCONFIGURED)) {
> + cdnsp_cmd_stop_ep(pdev, pep);
> + cdnsp_cmd_flush_ep(pdev, pep);
> + }
>
> /* Remove all queued USB requests. */
> while (!list_empty(>pending_list)) {
> @@ -1036,6 +1041,7 @@ static int cdnsp_gadget_ep_disable(struct usb_ep *ep)
> cdnsp_invalidate_ep_events(pdev, pep);
>
> pep->ep_state &= ~EP_DIS_IN_RROGRESS;
> +
Useless blank line
> drop_flag = cdnsp_get_endpoint_flag(pep->endpoint.desc);
> ctrl_ctx = cdnsp_get_input_control_ctx(>in_ctx);
> ctrl_ctx->drop_flags = cpu_to_le32(drop_flag);
> @@ -1043,10 +1049,12 @@ static int cdnsp_gadget_ep_disable(struct usb_ep *ep)
>
> cdnsp_endpoint_zero(pdev, pep);
>
> - ret = cdnsp_update_eps_configuration(pdev, pep);
> + if (!(pep->ep_state & EP_UNCONFIGURED))
> + ret = cdnsp_update_eps_configuration(pdev, pep);
> +
> cdnsp_free_endpoint_rings(pdev, pep);
>
> - pep->ep_state &= ~EP_ENABLED;
> + pep->ep_state &= ~(EP_ENABLED | EP_UNCONFIGURED);
> pep->ep_state |= EP_STOPPED;
>
> finish:
> diff --git a/drivers/usb/cdns3/cdnsp-gadget.h
> b/drivers/usb/cdns3/cdnsp-gadget.h
> index 6bbb26548c04..e628bd539e23 100644
> --- a/drivers/usb/cdns3/cdnsp-gadget.h
> +++ b/drivers/usb/cdns3/cdnsp-gadget.h
> @@ -830,11 +830,12 @@ struct cdnsp_ep {
> unsigned int ep_state;
> #define EP_ENABLED BIT(0)
> #define EP_DIS_IN_RROGRESS BIT(1)
> -#define EP_HALTEDBIT(2)
> -#define EP_STOPPED BIT(3)
> -#define EP_WEDGE BIT(4)
> -#define EP0_HALTED_STATUSBIT(5)
> -#define EP_HAS_STREAMS BIT(6)
> +#define EP_UNCONFIGURED BIT(2)
Why add new flag as BIT(2), it causes many changes in this patch?
> +#define EP_HALTEDBIT(3)
> +#define EP_STOPPED BIT(4)
> +#define EP_WEDGE BIT(5)
> +#define EP0_HALTED_STATUSBIT(6)
> +#define EP_HAS_STREAMS BIT(7)
>
> bool skip;
> };
> --
> 2.25.1
>
--
Thanks,
Peter Chen
Re: [PATCH v2] usb: cdnsp: Fixes issue with dequeuing requests after disabling endpoint
On 21-03-22 06:47:14, Pawel Laszczak wrote:
> From: Pawel Laszczak
>
> Patch fixes the bug:
> BUG: kernel NULL pointer dereference, address: 0050
> PGD 0 P4D 0
> Oops: 0002 [#1] SMP PTI
> CPU: 0 PID: 4137 Comm: uvc-gadget Tainted: G OE
> 5.10.0-next-20201214+ #3
> Hardware name: ASUS All Series/Q87T, BIOS 0908 07/22/2014
> RIP: 0010:cdnsp_remove_request+0xe9/0x530 [cdnsp_udc_pci]
> Code: 01 00 00 31 f6 48 89 df e8 64 d4 ff ff 48 8b 43 08 48 8b 13 45 31 f6 48
> 89 42 08 48 89 10 b8 98 ff ff ff 48 89 1b 48 89 5b 08 <41> 83 6d 50 01 41 83
> af d0 00 00 00 01 41 f6 84 24 78 20 00 00 08
> RSP: 0018:b68d00d07b60 EFLAGS: 00010046
> RAX: ff98 RBX: 9d29c57fbf00 RCX: 1400
> RDX: 9d29c57fbf00 RSI: RDI: 9d29c57fbf00
> RBP: b68d00d07bb0 R08: 9d2ad9510a00 R09: 9d2ac011c000
> R10: 9d2a12b6e760 R11: R12: 9d29d3fb8000
> R13: R14: R15: 9d29d3fb88c0
> FS: () GS:9d2adba0() knlGS:
> CS: 0010 DS: ES: CR0: 80050033
> CR2: 0050 CR3: 000102164005 CR4: 001706f0
> Call Trace:
> cdnsp_ep_dequeue+0x3c/0x90 [cdnsp_udc_pci]
> cdnsp_gadget_ep_dequeue+0x3f/0x80 [cdnsp_udc_pci]
> usb_ep_dequeue+0x21/0x70 [udc_core]
> uvcg_video_enable+0x19d/0x220 [usb_f_uvc]
> uvc_v4l2_release+0x49/0x90 [usb_f_uvc]
> v4l2_release+0xa5/0x100 [videodev]
> __fput+0x99/0x250
> fput+0xe/0x10
> task_work_run+0x75/0xb0
> do_exit+0x370/0xb80
> do_group_exit+0x43/0xa0
> get_signal+0x12d/0x820
> arch_do_signal_or_restart+0xb2/0x870
> ? __switch_to_asm+0x36/0x70
> ? kern_select+0xc6/0x100
> exit_to_user_mode_prepare+0xfc/0x170
> syscall_exit_to_user_mode+0x2a/0x40
> do_syscall_64+0x43/0x80
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
> RIP: 0033:0x7fe969cf5dd7
> Code: Unable to access opcode bytes at RIP 0x7fe969cf5dad.
>
> Problem occurs for UVC class. During disconnecting the UVC class disable
> endpoints and then start dequeuing all requests. This leads to situation
> where requests are removed twice. The first one in
> cdnsp_gadget_ep_disable and the second in cdnsp_gadget_ep_dequeue
> function.
> Patch adds condition in cdnsp_gadget_ep_dequeue function which allows
> dequeue requests only from enabled endpoint.
>
> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD
> Driver")
> Signed-off-by: Pawel Laszczak
>
> ---
> Changelog:
> v2:
> - removed unexpected 'commit' word from fixes tag
Acked-by: Peter Chen
Greg, would you help queue it to your usb-linus branch?
>
> drivers/usb/cdns3/cdnsp-gadget.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/usb/cdns3/cdnsp-gadget.c
> b/drivers/usb/cdns3/cdnsp-gadget.c
> index f2ebbacd932e..d7d4bdd57f46 100644
> --- a/drivers/usb/cdns3/cdnsp-gadget.c
> +++ b/drivers/usb/cdns3/cdnsp-gadget.c
> @@ -1128,6 +1128,10 @@ static int cdnsp_gadget_ep_dequeue(struct usb_ep *ep,
> return -ESHUTDOWN;
> }
>
> + /* Requests has been dequeued during disabling endpoint. */
> + if (!(pep->ep_state & EP_ENABLED))
> + return 0;
> +
> spin_lock_irqsave(>lock, flags);
> ret = cdnsp_ep_dequeue(pep, to_cdnsp_request(request));
> spin_unlock_irqrestore(>lock, flags);
> --
> 2.25.1
>
--
Thanks,
Peter Chen
Re: [PATCH v2] usb: cdns3: Optimize DMA request buffer allocation
dns3-gadget.h
> b/drivers/usb/cdns3/cdns3-gadget.h
> index ecf9b91..c5660f2 100644
> --- a/drivers/usb/cdns3/cdns3-gadget.h
> +++ b/drivers/usb/cdns3/cdns3-gadget.h
> @@ -12,6 +12,7 @@
> #ifndef __LINUX_CDNS3_GADGET
> #define __LINUX_CDNS3_GADGET
> #include
> +#include
>
> /*
> * USBSS-DEV register interface.
> @@ -1205,6 +1206,7 @@ struct cdns3_aligned_buf {
> void*buf;
> dma_addr_t dma;
> u32 size;
> + enum dma_data_direction dir;
> unsignedin_use:1;
> struct list_headlist;
> };
> --
> 2.4.5
>
--
Thanks,
Peter Chen
Re: [PATCH v2] usb: cdns3: Optimize DMA request buffer allocation
On 21-03-18 07:32:45, Christoph Hellwig wrote: > On Wed, Mar 17, 2021 at 08:13:59PM +0100, Sanket Parmar wrote: > > dma_alloc_coherent() might fail on the platform with a small > > DMA region. > > > > To avoid such failure in cdns3_prepare_aligned_request_buf(), > > dma_alloc_coherent() is replaced with dma_alloc_noncoherent() > > to allocate aligned request buffer of dynamic length. > > > > Reported-by: Aswath Govindraju > > Signed-off-by: Sanket Parmar > > Looks good to me: > > Reviewed-by: Christoph Hellwig Hi Christoph, I would like to confirm the dma_alloc_noncoherent allocates the memory less than PAGE_SIZE if buffer size it would like to allocate is small (eg, 64 bytes)? -- Thanks, Peter Chen
Re: [PATCH v2 1/2] usb: gadget: uvc: Updating bcdUVC field to 0x0110
On 21-03-15 07:59:25, Pawel Laszczak wrote:
> From: Pawel Laszczak
>
> Command Verifier during UVC Descriptor Tests (Class Video Control
> Interface Descriptor Test Video) complains about:
>
> Video Control Interface Header bcdUVC is 0x0100. USB Video Class
> specification 1.0 has been replaced by 1.1 specification
> (UVC: 6.2.26) Class Video Control Interface Descriptor bcdUVC is not 1.1
>
> Reviewed-by: Laurent Pinchart
> Signed-off-by: Pawel Laszczak
Reviewed-by: Peter Chen
>
> ---
> Changlog:
> v2:
> - fixed typo in commit message
>
> drivers/usb/gadget/function/uvc_configfs.c | 2 +-
> drivers/usb/gadget/legacy/webcam.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c
> b/drivers/usb/gadget/function/uvc_configfs.c
> index 00fb58e50a15..cd28dec837dd 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -231,7 +231,7 @@ static struct config_item
> *uvcg_control_header_make(struct config_group *group,
> h->desc.bLength = UVC_DT_HEADER_SIZE(1);
> h->desc.bDescriptorType = USB_DT_CS_INTERFACE;
> h->desc.bDescriptorSubType = UVC_VC_HEADER;
> - h->desc.bcdUVC = cpu_to_le16(0x0100);
> + h->desc.bcdUVC = cpu_to_le16(0x0110);
> h->desc.dwClockFrequency= cpu_to_le32(4800);
>
> config_item_init_type_name(>item, name, _control_header_type);
> diff --git a/drivers/usb/gadget/legacy/webcam.c
> b/drivers/usb/gadget/legacy/webcam.c
> index a9f8eb8e1c76..3a61de4bb2b1 100644
> --- a/drivers/usb/gadget/legacy/webcam.c
> +++ b/drivers/usb/gadget/legacy/webcam.c
> @@ -90,7 +90,7 @@ static const struct UVC_HEADER_DESCRIPTOR(1)
> uvc_control_header = {
> .bLength= UVC_DT_HEADER_SIZE(1),
> .bDescriptorType= USB_DT_CS_INTERFACE,
> .bDescriptorSubType = UVC_VC_HEADER,
> - .bcdUVC = cpu_to_le16(0x0100),
> + .bcdUVC = cpu_to_le16(0x0110),
> .wTotalLength = 0, /* dynamic */
> .dwClockFrequency = cpu_to_le32(4800),
> .bInCollection = 0, /* dynamic */
> --
> 2.25.1
>
--
Thanks,
Peter Chen
Re: [PATCH 2/2] usb: cdns3: Optimize DMA request buffer allocation
On 21-03-15 15:51:04, Sanket Parmar wrote: > > > + > > > priv_req->flags |= REQUEST_UNALIGNED; > > > trace_cdns3_prepare_aligned_request(priv_req); > > > > > > @@ -3088,11 +3113,11 @@ static void cdns3_gadget_exit(struct cdns > > *cdns) > > > struct cdns3_aligned_buf *buf; > > > > > > buf = cdns3_next_align_buf(_dev->aligned_buf_list); > > > - dma_free_coherent(priv_dev->sysdev, buf->size, > > > - buf->buf, > > > - buf->dma); > > > + dma_unmap_single(priv_dev->sysdev, buf->dma, buf->size, > > > + buf->dir); > > > > It only needs to DMA unmap after DMA has completed, this buf will not be > > used, otherwise, the kfree below will cause issue. > > This part is not clear. Aligned DMA buffer is allocated and mapped in > cdns3_prepare_aligned_request_buf() > and put into aligned_buf_list. While unloading the gadget, We need to undo > the same if aligned_buf_list is not > empty. Am I missing something here? My point is this unmap operation is useless since there is no user for aligned buf, and it calls kfree afterwards. You could also keep it as it has no harm. > > Also, I will post v2 of this patch which uses dma_*_noncoherent APIs > suggested by Christoph Hellwig. -- Thanks, Peter Chen
Re: [PATCH v4 2/2] usb: webcam: Invalid size of Processing Unit Descriptor
On 21-03-15 08:17:48, Pawel Laszczak wrote:
> From: Pawel Laszczak
>
> According with USB Device Class Definition for Video Device the
> Processing Unit Descriptor bLength should be 12 (10 + bmControlSize),
> but it has 11.
>
> Invalid length caused that Processing Unit Descriptor Test Video form
> CV tool failed. To fix this issue patch adds bmVideoStandards into
> uvc_processing_unit_descriptor structure.
>
> The bmVideoStandards field was added in UVC 1.1 and it wasn't part of
> UVC 1.0a.
>
> Reviewed-by: Laurent Pinchart
> Signed-off-by: Pawel Laszczak
>
Reviewed-by: Peter Chen
> ---
> Changelog:
> v4:
> - fixed compilation error caused by v2
> v3:
> - updated the commit message
> - added bmVideoStandard field to UVC gadget driver
> v2:
> - updated UVC_DT_PROCESSING_UNIT_SIZE macro
>
> drivers/usb/gadget/function/f_uvc.c | 1 +
> drivers/usb/gadget/legacy/webcam.c | 1 +
> include/uapi/linux/usb/video.h | 3 ++-
> 3 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/f_uvc.c
> b/drivers/usb/gadget/function/f_uvc.c
> index 5d62720bb9e1..e3b0a79c8f01 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -823,6 +823,7 @@ static struct usb_function_instance *uvc_alloc_inst(void)
> pd->bmControls[0] = 1;
> pd->bmControls[1] = 0;
> pd->iProcessing = 0;
> + pd->bmVideoStandards= 0;
>
> od = >uvc_output_terminal;
> od->bLength = UVC_DT_OUTPUT_TERMINAL_SIZE;
> diff --git a/drivers/usb/gadget/legacy/webcam.c
> b/drivers/usb/gadget/legacy/webcam.c
> index 3a61de4bb2b1..accb4dacf715 100644
> --- a/drivers/usb/gadget/legacy/webcam.c
> +++ b/drivers/usb/gadget/legacy/webcam.c
> @@ -125,6 +125,7 @@ static const struct uvc_processing_unit_descriptor
> uvc_processing = {
> .bmControls[0] = 1,
> .bmControls[1] = 0,
> .iProcessing= 0,
> + .bmVideoStandards = 0,
> };
>
> static const struct uvc_output_terminal_descriptor uvc_output_terminal = {
> diff --git a/include/uapi/linux/usb/video.h b/include/uapi/linux/usb/video.h
> index d854cb19c42c..bfdae12cdacf 100644
> --- a/include/uapi/linux/usb/video.h
> +++ b/include/uapi/linux/usb/video.h
> @@ -302,9 +302,10 @@ struct uvc_processing_unit_descriptor {
> __u8 bControlSize;
> __u8 bmControls[2];
> __u8 iProcessing;
> + __u8 bmVideoStandards;
> } __attribute__((__packed__));
>
> -#define UVC_DT_PROCESSING_UNIT_SIZE(n) (9+(n))
> +#define UVC_DT_PROCESSING_UNIT_SIZE(n) (10+(n))
>
> /* 3.7.2.6. Extension Unit Descriptor */
> struct uvc_extension_unit_descriptor {
> --
> 2.25.1
>
--
Thanks,
Peter Chen
Re: [PATCH 2/2] usb: cdns3: Optimize DMA request buffer allocation
iv_req->aligned_buf->in_use = 0;
> + queue_work(system_freezable_wq,
> +_dev->aligned_buf_wq);
@Pawel, do you remember when this condition is met?
> + }
> +
> + buf->dir = priv_ep->dir ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
> + buf->in_use = 1;
> + priv_req->aligned_buf = buf;
> +
> + buf->dma = dma_map_single(priv_dev->sysdev, buf->buf, buf->size,
> + buf->dir);
> +
> + if (dma_mapping_error(priv_dev->sysdev, buf->dma)) {
> + dev_err(priv_dev->dev, "Failed to map buffer\n");
> + kfree(buf->buf);
> + kfree(buf);
> + return -EFAULT;
> + }
> +
> priv_req->flags |= REQUEST_UNALIGNED;
> trace_cdns3_prepare_aligned_request(priv_req);
>
> @@ -3088,11 +3113,11 @@ static void cdns3_gadget_exit(struct cdns *cdns)
> struct cdns3_aligned_buf *buf;
>
> buf = cdns3_next_align_buf(_dev->aligned_buf_list);
> - dma_free_coherent(priv_dev->sysdev, buf->size,
> - buf->buf,
> - buf->dma);
> + dma_unmap_single(priv_dev->sysdev, buf->dma, buf->size,
> + buf->dir);
It only needs to DMA unmap after DMA has completed, this buf will not be
used, otherwise, the kfree below will cause issue.
>
> list_del(>list);
> + kfree(buf->buf);
> kfree(buf);
> }
>
> diff --git a/drivers/usb/cdns3/cdns3-gadget.h
> b/drivers/usb/cdns3/cdns3-gadget.h
> index ecf9b91..c5660f2 100644
> --- a/drivers/usb/cdns3/cdns3-gadget.h
> +++ b/drivers/usb/cdns3/cdns3-gadget.h
> @@ -12,6 +12,7 @@
> #ifndef __LINUX_CDNS3_GADGET
> #define __LINUX_CDNS3_GADGET
> #include
> +#include
>
> /*
> * USBSS-DEV register interface.
> @@ -1205,6 +1206,7 @@ struct cdns3_aligned_buf {
> void*buf;
> dma_addr_t dma;
> u32 size;
> + enum dma_data_direction dir;
> unsignedin_use:1;
> struct list_headlist;
> };
> --
> 1.7.1
>
--
Thanks,
Peter Chen
Re: [PATCH 1/2] usb: cdns3: Use dma_pool_* api to alloc trb pool
err2:
> cdns3_free_all_eps(priv_dev);
> err1:
> + dma_pool_destroy(priv_dev->eps_dma_pool);
> +
> usb_put_gadget(_dev->gadget);
> cdns->gadget_dev = NULL;
> return ret;
> diff --git a/drivers/usb/cdns3/cdns3-gadget.h
> b/drivers/usb/cdns3/cdns3-gadget.h
> index 21fa461..ecf9b91 100644
> --- a/drivers/usb/cdns3/cdns3-gadget.h
> +++ b/drivers/usb/cdns3/cdns3-gadget.h
> @@ -1298,6 +1298,7 @@ struct cdns3_device {
>
> struct cdns3_usb_regs __iomem *regs;
>
> + struct dma_pool *eps_dma_pool;
> struct usb_ctrlrequest *setup_buf;
> dma_addr_t setup_dma;
> void*zlp_buf;
> --
> 1.7.1
>
--
Thanks,
Peter Chen
Re: [PATCH v2] usb: gadget: uvc: add bInterval checking for HS mode
On 21-03-08 13:53:38, Pawel Laszczak wrote: > From: Pawel Laszczak > > Patch adds extra checking for bInterval passed by configfs. > The 5.6.4 chapter of USB Specification (rev. 2.0) say: > "A high-bandwidth endpoint must specify a period of 1x125 µs > (i.e., a bInterval value of 1)." > > The issue was observed during testing UVC class on CV. > I treat this change as improvement because we can control > bInterval by configfs. > > Signed-off-by: Pawel Laszczak > > --- > Changlog: > v2: > - removed duplicated assignment > > drivers/usb/gadget/function/f_uvc.c | 7 ++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/gadget/function/f_uvc.c > b/drivers/usb/gadget/function/f_uvc.c > index 44b4352a2676..ed77a126a74f 100644 > --- a/drivers/usb/gadget/function/f_uvc.c > +++ b/drivers/usb/gadget/function/f_uvc.c > @@ -633,7 +633,12 @@ uvc_function_bind(struct usb_configuration *c, struct > usb_function *f) > > uvc_hs_streaming_ep.wMaxPacketSize = > cpu_to_le16(max_packet_size | ((max_packet_mult - 1) << 11)); > - uvc_hs_streaming_ep.bInterval = opts->streaming_interval; > + > + /* A high-bandwidth endpoint must specify a bInterval value of 1 */ > + if (max_packet_mult > 1) > + uvc_hs_streaming_ep.bInterval = 1; > + else > + uvc_hs_streaming_ep.bInterval = opts->streaming_interval; > > uvc_ss_streaming_ep.wMaxPacketSize = cpu_to_le16(max_packet_size); > uvc_ss_streaming_ep.bInterval = opts->streaming_interval; > -- > 2.25.1 > Reviewed-by: Peter Chen -- Thanks, Peter Chen
Re: [PATCH 1/2] usb: gadget: uvc: Updating bcdUVC field to 0x0110
On 21-03-08 11:27:34, Pawel Laszczak wrote:
> From: Pawel Laszczak
>
> Command Verifier during UVC Descriptor Tests (Class Video Control
> Interface Descriptor Test Video) compleins about:
%s/compleins/complains
>
> Video Control Interface Header bcdUVC is 0x0100. USB Video Class
> specification 1.0 has been replaced by 1.1 specification
> (UVC: 6.2.26) Class Video Control Interface Descriptor bcdUVC is not 1.1
What does this (UVC: 6.2.26) mean? There are only 4 chapters for this
spec, Am I something wrong?
>
> Signed-off-by: Pawel Laszczak
> ---
> drivers/usb/gadget/function/uvc_configfs.c | 2 +-
> drivers/usb/gadget/legacy/webcam.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c
> b/drivers/usb/gadget/function/uvc_configfs.c
> index 00fb58e50a15..cd28dec837dd 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -231,7 +231,7 @@ static struct config_item
> *uvcg_control_header_make(struct config_group *group,
> h->desc.bLength = UVC_DT_HEADER_SIZE(1);
> h->desc.bDescriptorType = USB_DT_CS_INTERFACE;
> h->desc.bDescriptorSubType = UVC_VC_HEADER;
> - h->desc.bcdUVC = cpu_to_le16(0x0100);
> + h->desc.bcdUVC = cpu_to_le16(0x0110);
> h->desc.dwClockFrequency= cpu_to_le32(4800);
>
> config_item_init_type_name(>item, name, _control_header_type);
> diff --git a/drivers/usb/gadget/legacy/webcam.c
> b/drivers/usb/gadget/legacy/webcam.c
> index a9f8eb8e1c76..3a61de4bb2b1 100644
> --- a/drivers/usb/gadget/legacy/webcam.c
> +++ b/drivers/usb/gadget/legacy/webcam.c
> @@ -90,7 +90,7 @@ static const struct UVC_HEADER_DESCRIPTOR(1)
> uvc_control_header = {
> .bLength= UVC_DT_HEADER_SIZE(1),
> .bDescriptorType= USB_DT_CS_INTERFACE,
> .bDescriptorSubType = UVC_VC_HEADER,
> - .bcdUVC = cpu_to_le16(0x0100),
> + .bcdUVC = cpu_to_le16(0x0110),
> .wTotalLength = 0, /* dynamic */
> .dwClockFrequency = cpu_to_le32(4800),
> .bInCollection = 0, /* dynamic */
> --
> 2.25.1
>
--
Thanks,
Peter Chen
Re: [PATCH 2/2] usb: webcam: Invalid size of Processing Unit Descriptor
On 21-03-08 11:27:35, Pawel Laszczak wrote:
> From: Pawel Laszczak
>
> According with USB Device Class Definition for Video Device the
> Processing Unit Descriptor bLength should be 12 (10 + bmControlSize),
> but it has 11.
Does the reason forget filling bmVideoStandards entry?
Peter
>
> Invalid length caused that Processing Unit Descriptor Test Video form
> CV tool failed. To fix this issue patch adds bmVideoStandards into
> uvc_processing_unit_descriptor structure.
>
> Signed-off-by: Pawel Laszczak
> ---
> include/uapi/linux/usb/video.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/include/uapi/linux/usb/video.h b/include/uapi/linux/usb/video.h
> index d854cb19c42c..2a54e8fdd341 100644
> --- a/include/uapi/linux/usb/video.h
> +++ b/include/uapi/linux/usb/video.h
> @@ -302,6 +302,7 @@ struct uvc_processing_unit_descriptor {
> __u8 bControlSize;
> __u8 bmControls[2];
> __u8 iProcessing;
> + __u8 bmVideoStandards;
> } __attribute__((__packed__));
>
> #define UVC_DT_PROCESSING_UNIT_SIZE(n) (9+(n))
> --
> 2.25.1
>
--
Thanks,
Peter Chen
Re: [PATCH 0/2] tracing: Detect unsafe dereferencing of pointers from trace events
On 21-03-02 09:56:05, Steven Rostedt wrote: > On Tue, 2 Mar 2021 16:23:55 +0800 > Peter Chen wrote: > > s it looks like it uses %pa which IIUC from the printk code, it > > > >> dereferences the pointer to find it's virtual address. The event has > > > >> this as the field: > > > >> > > > >> __field(struct cdns3_trb *, start_trb_addr) > > > >> > > > >> Assigns it with: > > > >> > > > >> __entry->start_trb_addr = req->trb; > > > >> > > > >> And prints that with %pa, which will dereference pointer at the time of > > > >> reading, where the address in question may no longer be around. That > > > >> looks to me as a potential bug. > > > > Steven, thanks for reporting. Do you mind sending patch to fix it? > > If you have no time to do it, I will do it later. > > > > I would have already fixed it, but I wasn't exactly sure how this is used. > > In Documentation/core-api/printk-formats.rst we have: > >Physical address types phys_addr_t >-- > >:: > >%pa[p] 0x01234567 or 0x0123456789abcdef > >For printing a phys_addr_t type (and its derivatives, such as >resource_size_t) which can vary based on build options, regardless of the >width of the CPU data path. > > So it only looks like it is used to for the size of the pointer. > > I guess something like this might work: > > diff --git a/drivers/usb/cdns3/cdns3-trace.h b/drivers/usb/cdns3/cdns3-trace.h > index 8648c7a7a9dd..d3b8624fc427 100644 > --- a/drivers/usb/cdns3/cdns3-trace.h > +++ b/drivers/usb/cdns3/cdns3-trace.h > @@ -214,7 +214,7 @@ DECLARE_EVENT_CLASS(cdns3_log_request, > __field(int, no_interrupt) > __field(int, start_trb) > __field(int, end_trb) > - __field(struct cdns3_trb *, start_trb_addr) > + __field(phys_addr_t, start_trb_addr) > __field(int, flags) > __field(unsigned int, stream_id) > ), > @@ -230,7 +230,7 @@ DECLARE_EVENT_CLASS(cdns3_log_request, > __entry->no_interrupt = req->request.no_interrupt; > __entry->start_trb = req->start_trb; > __entry->end_trb = req->end_trb; > - __entry->start_trb_addr = req->trb; > + __entry->start_trb_addr = *(const phys_addr_t *)req->trb; > __entry->flags = req->flags; > __entry->stream_id = req->request.stream_id; > ), > @@ -244,7 +244,7 @@ DECLARE_EVENT_CLASS(cdns3_log_request, > __entry->status, > __entry->start_trb, > __entry->end_trb, > - __entry->start_trb_addr, > + /* %pa dereferences */ &__entry->start_trb_addr, > __entry->flags, > __entry->stream_id > ) > > > Can you please test it? I don't have the hardware, but I also want to make > sure I don't break anything. > > Thanks, > Since the virtual address for req->trb is NULL before using it. It will trigger below oops using your change. There is already index (start_trb/end_trb) for which TRB it has handled, it is not necessary to trace information for its physical address. I decide to delete this trace entry, thanks for reporting it. [ 61.695160] Unable to handle kernel NULL pointer dereference at virtual address [ 61.704066] Mem abort info: [ 61.706910] ESR = 0x9606 [ 61.71] EC = 0x25: DABT (current EL), IL = 32 bits [ 61.715339] SET = 0, FnV = 0 [ 61.718416] EA = 0, S1PTW = 0 [ 61.721575] Data abort info: [ 61.724482] ISV = 0, ISS = 0x0006 [ 61.728323] CM = 0, WnR = 0 [ 61.731324] user pgtable: 4k pages, 48-bit VAs, pgdp=0008856dd000 [ 61.737816] [] pgd=00088577a003, p4d=00088577a003, pud=00088477c003, pmd= [ 61.748532] Internal error: Oops: 9606 [#1] PREEMPT SMP [ 61.754113] Modules linked in: fsl_jr_uio caam_jr caamkeyblob_desc caamhash_desc caamalg_desc crypto_engine rng_core authenc libdes crct10dif_ce mxc_jpeg_encdec imx8_media_dev(C) caam error Message from syslogd@imx8qmmek at Fri Jul 10 06:52:44 2020 ... imx8qmmek kernel: [ 61.748532] Internal error: Oops: 9606 [#1] PREEMPT SMP [ 61.784245] CPU: 3 PID: 188 Comm: 1-0050 Tainted: G C 5.10.0-rc7-04451-gfcfe23a5424-dirty #3 [ 61.793993] Hardware name: Freescale i.MX8QXP MEK (DT) [ 61.799139] pstate: 8005 (Nzcv daif -PAN -UAO -TCO BTYPE=--) [ 61.805162] pc : trace_event
Re: [PATCH] usb: cdns3: Coherent memory allocation optimization
usb_put_gadget(_dev->gadget);
> @@ -3185,6 +3196,14 @@ static int cdns3_gadget_start(struct cdns *cdns)
> /* initialize endpoint container */
> INIT_LIST_HEAD(_dev->gadget.ep_list);
> INIT_LIST_HEAD(_dev->aligned_buf_list);
> + priv_dev->eps_dma_pool = dma_pool_create("cdns3_eps_dma_pool",
> + priv_dev->sysdev,
> + TRB_RING_SIZE, 8, 0);
> + if (!priv_dev->eps_dma_pool) {
> + dev_err(priv_dev->dev, "Failed to create TRB dma pool\n");
> + ret = -ENOMEM;
> + goto err1;
> + }
>
> ret = cdns3_init_eps(priv_dev);
> if (ret) {
> @@ -3235,6 +3254,8 @@ static int cdns3_gadget_start(struct cdns *cdns)
> err2:
> cdns3_free_all_eps(priv_dev);
> err1:
> + dma_pool_destroy(priv_dev->eps_dma_pool);
> +
> usb_put_gadget(_dev->gadget);
> cdns->gadget_dev = NULL;
> return ret;
> diff --git a/drivers/usb/cdns3/cdns3-gadget.h
> b/drivers/usb/cdns3/cdns3-gadget.h
> index 21fa461..c5660f2 100644
> --- a/drivers/usb/cdns3/cdns3-gadget.h
> +++ b/drivers/usb/cdns3/cdns3-gadget.h
> @@ -12,6 +12,7 @@
> #ifndef __LINUX_CDNS3_GADGET
> #define __LINUX_CDNS3_GADGET
> #include
> +#include
>
> /*
> * USBSS-DEV register interface.
> @@ -1205,6 +1206,7 @@ struct cdns3_aligned_buf {
> void*buf;
> dma_addr_t dma;
> u32 size;
> + enum dma_data_direction dir;
> unsignedin_use:1;
> struct list_headlist;
> };
> @@ -1298,6 +1300,7 @@ struct cdns3_device {
>
> struct cdns3_usb_regs __iomem *regs;
>
> + struct dma_pool *eps_dma_pool;
> struct usb_ctrlrequest *setup_buf;
> dma_addr_t setup_dma;
> void*zlp_buf;
> --
> 1.7.1
>
I guess this issue may due to the size for DMA region is too small,
try to enlarge the it (eg, CMA size).
--
Thanks,
Peter Chen
Re: [PATCH] usb: cdnsp: Fixes incorrect value in ISOC TRB
On 21-03-05 06:10:59, Pawel Laszczak wrote:
> From: Pawel Laszczak
>
> The value "start_cycle ? 0 : 1" in assignment caused
> implicit truncation whole value to 1 byte.
> To fix the issue, an explicit casting has been added.
The root cause for this issue should be operator "|" priority higher
than "? :", I think just add () for start_cycle ? 0 : 1 could fix it.
Please double confirm it, and change the commit log if necessary
Peter
>
> Fixes: commit 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence
> USBSSP DRD Driver")
> Signed-off-by: Pawel Laszczak
> ---
> drivers/usb/cdns3/cdnsp-ring.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c
> index f9170d177a89..d35bc4490216 100644
> --- a/drivers/usb/cdns3/cdnsp-ring.c
> +++ b/drivers/usb/cdns3/cdnsp-ring.c
> @@ -2197,7 +2197,7 @@ static int cdnsp_queue_isoc_tx(struct cdnsp_device
> *pdev,
>* inverted in the first TDs isoc TRB.
>*/
> field = TRB_TYPE(TRB_ISOC) | TRB_TLBPC(last_burst_pkt) |
> - start_cycle ? 0 : 1 | TRB_SIA | TRB_TBC(burst_count);
> + (u32)(start_cycle ? 0 : 1) | TRB_SIA | TRB_TBC(burst_count);
>
> /* Fill the rest of the TRB fields, and remaining normal TRBs. */
> for (i = 0; i < trbs_per_td; i++) {
> --
> 2.25.1
>
--
Thanks,
Peter Chen
Re: [RFC PATCH] USB:XHCI:Modify XHCI driver for USB2.0 controller
On 21-02-27 11:31:00, liulongfang wrote: > On 2021/2/26 16:38, Greg KH wrote: > > On Fri, Feb 26, 2021 at 04:21:37PM +0800, Longfang Liu wrote: > >> Our current XHCI hardware controller has been customized to only > >> support USB 2.0 ports. > > > > That sounds like a spec violation, right? Why do you want to do this? > > > > greg k-h > > . > > > I hope to support a USB2.0-only mode on the XHCI controller > through software configuration. > Thanks. If your hardware has disabled USB3 logic, when the USB3 device is plugged, since there are no RX signal on the bus, the device will not enable USB3 logic, and only USB2 signals will be on the bus, there are only USB devices on USB2 roothub later. So, any issues you have met? -- Thanks, Peter Chen
Re: [PATCH 0/2] tracing: Detect unsafe dereferencing of pointers from trace events
On 21-03-02 09:56:05, Steven Rostedt wrote: > On Tue, 2 Mar 2021 16:23:55 +0800 > Peter Chen wrote: > > s it looks like it uses %pa which IIUC from the printk code, it > > > >> dereferences the pointer to find it's virtual address. The event has > > > >> this as the field: > > > >> > > > >> __field(struct cdns3_trb *, start_trb_addr) > > > >> > > > >> Assigns it with: > > > >> > > > >> __entry->start_trb_addr = req->trb; > > > >> > > > >> And prints that with %pa, which will dereference pointer at the time of > > > >> reading, where the address in question may no longer be around. That > > > >> looks to me as a potential bug. > > > > Steven, thanks for reporting. Do you mind sending patch to fix it? > > If you have no time to do it, I will do it later. > > > > I would have already fixed it, but I wasn't exactly sure how this is used. > > In Documentation/core-api/printk-formats.rst we have: > >Physical address types phys_addr_t >-- > >:: > >%pa[p] 0x01234567 or 0x0123456789abcdef > >For printing a phys_addr_t type (and its derivatives, such as >resource_size_t) which can vary based on build options, regardless of the >width of the CPU data path. > > So it only looks like it is used to for the size of the pointer. > > I guess something like this might work: > > diff --git a/drivers/usb/cdns3/cdns3-trace.h b/drivers/usb/cdns3/cdns3-trace.h > index 8648c7a7a9dd..d3b8624fc427 100644 > --- a/drivers/usb/cdns3/cdns3-trace.h > +++ b/drivers/usb/cdns3/cdns3-trace.h > @@ -214,7 +214,7 @@ DECLARE_EVENT_CLASS(cdns3_log_request, > __field(int, no_interrupt) > __field(int, start_trb) > __field(int, end_trb) > - __field(struct cdns3_trb *, start_trb_addr) > + __field(phys_addr_t, start_trb_addr) > __field(int, flags) > __field(unsigned int, stream_id) > ), > @@ -230,7 +230,7 @@ DECLARE_EVENT_CLASS(cdns3_log_request, > __entry->no_interrupt = req->request.no_interrupt; > __entry->start_trb = req->start_trb; > __entry->end_trb = req->end_trb; > - __entry->start_trb_addr = req->trb; > + __entry->start_trb_addr = *(const phys_addr_t *)req->trb; > __entry->flags = req->flags; > __entry->stream_id = req->request.stream_id; > ), > @@ -244,7 +244,7 @@ DECLARE_EVENT_CLASS(cdns3_log_request, > __entry->status, > __entry->start_trb, > __entry->end_trb, > - __entry->start_trb_addr, > + /* %pa dereferences */ &__entry->start_trb_addr, > __entry->flags, > __entry->stream_id > ) > > > Can you please test it? I don't have the hardware, but I also want to make > sure I don't break anything. > Hi Steve, Regarding this issue, I have one question: - If the virtual address is got from dma_alloc_coherent, can't we print this address using %pa to get its physical address (the same with DMA address), or its DMA address using %pad? req->trb is the virtual address got from dma_alloc_coherent. And what's the logic for this "unsafe dereference" warning? Thanks. -- Thanks, Peter Chen
Re: [PATCH 0/2] tracing: Detect unsafe dereferencing of pointers from trace events
On 21-03-01 05:27:04, Pawel Laszczak wrote: > > + Peter Chen - Maintainer of CDNS3 driver > > > > >[ Resending with an address that should work for Felipe ] > > > >On Sat, 27 Feb 2021 14:18:02 -0500 > >Steven Rostedt wrote: > > > >> On Fri, 26 Feb 2021 14:21:00 -0800 > >> Linus Torvalds wrote: > >> > >> > On Fri, Feb 26, 2021 at 11:07 AM Steven Rostedt > >> > wrote: > >> > > > >> > > The first patch scans the print fmts of the trace events looking for > >> > > dereferencing pointers from %p*, and making sure that they refer back > >> > > to the trace event itself. > >> > > > >> > > The second patch handles strings "%s" [..] > >> > > >> > Doing this at runtime really feels like the wrong thing to do. > >> > > >> > It won't even protect us from what happened - people like me and > >> > Andrew won't even run those tracepoints in the first place, so we > >> > won't notice. > >> > > >> > It really would be much better in every respect to have this done by > >> > checkpatch, I think. > >> > >> And after fixing the parsing to not trigger false positives, an > >> allyesconfig boot found this: > >> > >> event cdns3_gadget_giveback has unsafe dereference of argument 11 > >> print_fmt: "%s: req: %p, req buff %p, length: %u/%u %s%s%s, status: %d, > >> trb: [start:%d, end:%d: virt addr %pa], flags:%x SID: %u", > >__get_str(name), REC->req, REC->buf, > >> REC->actual, REC->length, REC->zero ? "Z" : "z", REC->short_not_ok ? "S" > >> : "s", REC->no_interrupt ? "I" : "i", REC->status, REC- > >>start_trb, REC->end_trb, REC->start_trb_addr, REC->flags, RE > >> C->stream_id > >> > >> (as the above is from a trace event class, it triggered for every event > >> in that class). > >> > >> As it looks like it uses %pa which IIUC from the printk code, it > >> dereferences the pointer to find it's virtual address. The event has > >> this as the field: > >> > >> __field(struct cdns3_trb *, start_trb_addr) > >> > >> Assigns it with: > >> > >> __entry->start_trb_addr = req->trb; > >> > >> And prints that with %pa, which will dereference pointer at the time of > >> reading, where the address in question may no longer be around. That > >> looks to me as a potential bug. Steven, thanks for reporting. Do you mind sending patch to fix it? If you have no time to do it, I will do it later. -- Thanks, Peter Chen
Re: [PATCH] usb: cdnsp: Removes some useless trace events
On 21-02-04 11:27:28, Greg KH wrote: > On Thu, Feb 04, 2021 at 10:20:35AM +0100, Pawel Laszczak wrote: > > Patch removes some useless trace events that can > > be replaced by ftrace. > > > > Reported-by: Dan Carpenter > > Signed-off-by: Pawel Laszczak > > --- > > drivers/usb/cdns3/cdnsp-ep0.c| 5 - > > drivers/usb/cdns3/cdnsp-gadget.c | 2 -- > > drivers/usb/cdns3/cdnsp-ring.c | 1 - > > drivers/usb/cdns3/cdnsp-trace.h | 10 -- > > 4 files changed, 18 deletions(-) > > Acked-by: Greg Kroah-Hartman Applied, thanks Pawel. -- Thanks, Peter Chen
Re: [PATCH] usb: cdns3: Add support for TI's AM64 SoC
On 21-01-19 10:58:10, Aswath Govindraju wrote:
> Add support for USB controller present on the AM64x SoC.
>
> Signed-off-by: Aswath Govindraju
> ---
> drivers/usb/cdns3/cdns3-ti.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> index 90e246601537..eccb1c766bba 100644
> --- a/drivers/usb/cdns3/cdns3-ti.c
> +++ b/drivers/usb/cdns3/cdns3-ti.c
> @@ -214,6 +214,7 @@ static int cdns_ti_remove(struct platform_device *pdev)
>
> static const struct of_device_id cdns_ti_of_match[] = {
> { .compatible = "ti,j721e-usb", },
> + { .compatible = "ti,am64-usb", },
> {},
> };
> MODULE_DEVICE_TABLE(of, cdns_ti_of_match);
> --
> 2.17.1
>
Applied, thanks.
--
Thanks,
Peter Chen
Re: [PATCH 1/1] usb: xhci: setup packets don't need DMA mapping
On 21-01-14 13:00:21, Alan Stern wrote: > On Thu, Jan 14, 2021 at 01:04:02PM +0800, Peter Chen wrote: > > On 21-01-14 11:59:07, Daewoong Kim wrote: > > > DMA mapping of urb->setup_packet is not necessary for xHCI host > > > controllers. The xHCI specification says that Setup Stage TRB includes > > > whole Setup Data; therefore, urb->setup_dma will not be used in the xhci > > > HCD code. > > > > > > > How about bypass map/unmap operation for xHCI control transfer directly? > > > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > > index 91ab81c3fc79..0a0ab14b7638 100644 > > --- a/drivers/usb/host/xhci.c > > +++ b/drivers/usb/host/xhci.c > > @@ -1374,7 +1374,8 @@ static int xhci_map_urb_for_dma(struct usb_hcd *hcd, > > struct urb *urb, > > > > xhci = hcd_to_xhci(hcd); > > > > - if (xhci_urb_suitable_for_idt(urb)) > > + if (xhci_urb_suitable_for_idt(urb) || > > + (usb_endpoint_xfer_control(>ep->desc))) > > return 0; > > Would this affect the map/unmap operations for the DATA packets in a > control transfer, along with the SETUP packet? > Oh, you are right, Alan. We do need map/unmap operation for DATA packet in control transfer. -- Thanks, Peter Chen
Re: [PATCH 1/1] usb: xhci: setup packets don't need DMA mapping
On 21-01-14 11:59:07, Daewoong Kim wrote:
> DMA mapping of urb->setup_packet is not necessary for xHCI host
> controllers. The xHCI specification says that Setup Stage TRB includes
> whole Setup Data; therefore, urb->setup_dma will not be used in the xhci
> HCD code.
>
How about bypass map/unmap operation for xHCI control transfer directly?
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 91ab81c3fc79..0a0ab14b7638 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1374,7 +1374,8 @@ static int xhci_map_urb_for_dma(struct usb_hcd *hcd,
struct urb *urb,
xhci = hcd_to_xhci(hcd);
- if (xhci_urb_suitable_for_idt(urb))
+ if (xhci_urb_suitable_for_idt(urb) ||
+ (usb_endpoint_xfer_control(>ep->desc)))
return 0;
if (xhci->quirks & XHCI_SG_TRB_CACHE_SIZE_QUIRK) {
@@ -1389,6 +1390,9 @@ static void xhci_unmap_urb_for_dma(struct usb_hcd *hcd,
struct urb *urb)
struct xhci_hcd *xhci;
bool unmap_temp_buf = false;
+ if (usb_endpoint_xfer_control(>ep->desc))
+ return;
+
xhci = hcd_to_xhci(hcd);
if (urb->num_sgs && (urb->transfer_flags & URB_DMA_MAP_SINGLE))
> Signed-off-by: Daewoong Kim
> ---
> drivers/usb/core/hcd.c | 4 +++-
> drivers/usb/host/xhci.c | 1 +
> include/linux/usb.h | 4
> 3 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index ad5a0f405a75..b1f9eac93f0d 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1411,7 +1411,9 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct
> urb *urb,
> if (usb_endpoint_xfer_control(>ep->desc)) {
> if (hcd->self.uses_pio_for_control)
> return ret;
> - if (hcd->localmem_pool) {
> + if (hcd->self.uses_pio_for_setup_pkt) {
> + ; /* do nothing */
> + } else if (hcd->localmem_pool) {
> ret = hcd_alloc_coherent(
> urb->dev->bus, mem_flags,
> >setup_dma,
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index e86940571b4c..c263aee82dc0 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -643,6 +643,7 @@ int xhci_run(struct usb_hcd *hcd)
>*/
>
> hcd->uses_new_polling = 1;
> + hcd->self.uses_pio_for_setup_pkt = 1;
> if (!usb_hcd_is_primary_hcd(hcd))
> return xhci_run_finished(xhci);
>
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index 7d72c4e0713c..76600e8de414 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -430,6 +430,10 @@ struct usb_bus {
>* Does the host controller use PIO
>* for control transfers?
>*/
> + u8 uses_pio_for_setup_pkt; /*
> + * Does the host controller use PIO
> + * for setup packets?
> + */
> u8 otg_port; /* 0, or number of OTG/HNP port */
> unsigned is_b_host:1; /* true during some HNP roleswitches */
> unsigned b_hnp_enable:1;/* OTG: did A-Host enable HNP? */
> --
> 2.17.1
>
--
Thanks,
Peter Chen
Re: Infinite recursion in device_reorder_to_tail() due to circular device links
On 21-01-13 12:18:35, Stephan Gerhold wrote: > > Also, on a completely different note I looked again at the chipidea USB > driver that produces this situation. To request the PHY (which ends up > in the circular device link) it does: > > /* Look for a generic PHY first */ > ci->phy = devm_phy_get(dev->parent, "usb-phy"); > > To me it doesn't really seem great to use the devm_* helpers on the > parent device either, so I will check if I can refactor this somehow. > Perhaps this situation can be prevented entirely. > Hi Stephan, You could try to get the PHY at parent driver (drivers/usb/chipidea/ci_hdrc_msm.c) to see the difference. -- Thanks, Peter Chen
RE: [PATCH v2] usb: cdnsp: fixes undefined reference to cdns_remove
> > > > Issue occurs for USB/CDNS3/CDNSP kernel configuration: > > CONFIG_USB=m > > CONFIG_USB_CDNS_SUPPORT=y > > CONFIG_USB_CDNS3=m > > CONFIG_USB_CDNS3_PCI_WRAP=m > > CONFIG_USB_CDNSP_PCI=y > > > > Reported-by: Randy Dunlap > > Signed-off-by: Pawel Laszczak > > After removing the v1 patch and applying this one, my build errors are gone. > Thanks. > > Acked-by: Randy Dunlap > Force updated, thanks. Peter > > --- > > changelog: > > v2 > > - added missing condition > > > > drivers/usb/cdns3/Makefile | 8 > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile > > index 3f9b7fa8a594..61edb2f89276 100644 > > --- a/drivers/usb/cdns3/Makefile > > +++ b/drivers/usb/cdns3/Makefile > > @@ -26,7 +26,15 @@ obj-$(CONFIG_USB_CDNS3_TI) += > cdns3-ti.o > > obj-$(CONFIG_USB_CDNS3_IMX)+= cdns3-imx.o > > > > cdnsp-udc-pci-y:= cdnsp-pci.o > > + > > +ifdef CONFIG_USB_CDNSP_PCI > > +ifeq ($(CONFIG_USB),m) > > +obj-m += cdnsp-udc-pci.o > > +else > > obj-$(CONFIG_USB_CDNSP_PCI)+= cdnsp-udc-pci.o > > +endif > > +endif > > + > > cdnsp-udc-pci-$(CONFIG_USB_CDNSP_GADGET) += cdnsp-ring.o > cdnsp-gadget.o \ > >cdnsp-mem.o cdnsp-ep0.o > > > > > > > -- > ~Randy > You can't do anything without having to do something else first. > -- Belefant's Law
Re: [PATCH v3 0/9] Support Runtime PM and host mode by Tegra ChipIdea USB driver
On 21-01-12 09:56:37, Dmitry Osipenko wrote: > 29.12.2020 17:26, Dmitry Osipenko пишет: > > 29.12.2020 08:16, Peter Chen пишет: > >> On 20-12-18 15:02:37, Dmitry Osipenko wrote: > >>> This series implements Runtime PM support for the Tegra ChipIdea USB > >>> driver. > >>> It also squashes the older ehci-tegra driver into the ChipIdea driver, > >>> hence > >>> the RPM is supported by both UDC and host controllers, secondly this opens > >>> opportunity for implementing OTG support in the future. > >>> > >>> Patchset was tested on various Tegra20, Tegra30 and Tegra124 devices. > >>> Thanks to Peter Geis, Matt Merhar, Nicolas Chauvet and Ion Agorria for > >>> helping with the extensive and productive testing! > >>> > >>> Changelog: > >>> > >>> v3: - Replaced "goto" with if-statements as was suggested by Thierry > >>> Reding. > >>> > >>> - Improved wording of the deprecated Kconfig entry as was suggested > >>> by Alan Stern. > >>> > >>> - Added ACKs from Thierry Reding and Alan Stern. > >>> > >>> - Added a new minor patch "Specify TX FIFO threshold in UDC SoC info" > >>> just for completeness, since we can now switch OTG to host mode in > >>> the ChipIdea driver. Although, OTG support remains a > >>> work-in-progress > >>> for now. > >>> > >>> v2: - Improved comments in the code as it was suggested by Peter Chen and > >>> Sergei Shtylyov for v1. > >>> > >>> - Replaced mdelay() with fsleep() and made ci->hdc to reset to NULL in > >>> a error code path, like it was suggested by Peter Chen. > >>> > >>> - Redirected deprecated USB_EHCI_TEGRA Kconfig entry to > >>> USB_CHIPIDEA_TEGRA > >>> as was suggested by Alan Stern. > >>> > >>> - Improved commit message and added ACK from Thierry Reding to the > >>> patch > >>> that removes MODULE_ALIAS. > >>> > >>> - Fixed UDC PHY waking up on ASUS TF201 tablet device by utilizing > >>> additional VBUS sensor. This was reported and tested by Ion Agorria. > >>> > >>> - Added t-b from Ion Agorria. > >>> > >>> Dmitry Osipenko (8): > >>> usb: phy: tegra: Add delay after power up > >>> usb: phy: tegra: Support waking up from a low power mode > >>> usb: chipidea: tegra: Remove MODULE_ALIAS > >>> usb: chipidea: tegra: Rename UDC to USB > >>> usb: chipidea: tegra: Support runtime PM > >>> usb: chipidea: tegra: Specify TX FIFO threshold in UDC SoC info > >>> usb: host: ehci-tegra: Remove the driver > >>> ARM: tegra_defconfig: Enable USB_CHIPIDEA_HOST and remove > >>> USB_EHCI_TEGRA > >>> > >>> Peter Geis (1): > >>> usb: chipidea: tegra: Support host mode > >> > >> Chipidea related (patch 3-7) are applied, thanks. > > > > Hello Peter, > > > > Thank you for applying the patches. > > > > Who will apply the remaining patches? > > > > The Chipidea patch #6 depends on the PHY changes, otherwise USB will > > suspend and never resume. > > > > Peter, could you please apply the PHY and defconfig patches along with > the CI patches to -next? I.e. the whole series. Preferentially in > original ordering of patches should be preserved. > Hi Dmitry, Usually, Greg could apply all USB patches, if I apply other USB related patches, it may cause conflict with other patches in other's tree. Greg, free feel to apply this series with Acked-by: Peter Chen for chipidea part. For ARM defconfig patch, I think it should go ARM's tree. -- Thanks, Peter Chen
RE: [PATCH] usb: cdnsp: fixes undefined reference to cdns_remove
> > On 1/11/21 6:42 AM, Pawel Laszczak wrote: > > Patch fixes the following errors: > > ld: drivers/usb/cdns3/cdnsp-pci.o: in function `cdnsp_pci_remove': > > cdnsp-pci.c:(.text+0x80): undefined reference to `cdns_remove' > > ld: drivers/usb/cdns3/cdnsp-pci.o: in function `cdnsp_pci_probe': > > cdnsp-pci.c:(.text+0x34c): undefined reference to `cdns_init' > > > > Issue occurs for USB/CDNS3/CDNSP kernel configuration: > > CONFIG_USB=m > > CONFIG_USB_CDNS_SUPPORT=y > > CONFIG_USB_CDNS3=m > > CONFIG_USB_CDNS3_PCI_WRAP=m > > CONFIG_USB_CDNSP_PCI=y > > > > Reported-by: Randy Dunlap > > Signed-off-by: Pawel Laszczak > > Yep, that works. Thanks. > > Acked-by: Randy Dunlap # build-tested > Applied it, thanks. Peter > > > --- > > drivers/usb/cdns3/Makefile | 4 > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile > > index 3f9b7fa8a594..be906910f98b 100644 > > --- a/drivers/usb/cdns3/Makefile > > +++ b/drivers/usb/cdns3/Makefile > > @@ -26,7 +26,11 @@ obj-$(CONFIG_USB_CDNS3_TI) += > cdns3-ti.o > > obj-$(CONFIG_USB_CDNS3_IMX)+= cdns3-imx.o > > > > cdnsp-udc-pci-y:= cdnsp-pci.o > > +ifeq ($(CONFIG_USB),m) > > +obj-m += cdnsp-udc-pci.o > > +else > > obj-$(CONFIG_USB_CDNSP_PCI)+= cdnsp-udc-pci.o > > +endif > > cdnsp-udc-pci-$(CONFIG_USB_CDNSP_GADGET) += cdnsp-ring.o > cdnsp-gadget.o \ > >cdnsp-mem.o cdnsp-ep0.o > > > > > > > -- > ~Randy
Re: [PATCH] usb: cdns3: Adds missing __iomem markers
On 21-01-11 09:20:31, Pawel Laszczak wrote:
> Hi Peter,
>
> What about this patch, can you apply it into for-usb-next ?
Done.
Peter
>
> Thanks
> Pawel
>
> >
> >Patch adds missing __iomem markers in core.h file
> >and makes some changes in drd.c file related with
> >these markers.
> >
> >The lack of __iomem has reported by sparse checker
> >on parsic architecture.
> >
> >Signed-off-by: Pawel Laszczak
> >Reported-by: kernel test robot
> >---
> > drivers/usb/cdns3/core.h | 12 ++--
> > drivers/usb/cdns3/drd.c | 12 ++--
> > 2 files changed, 12 insertions(+), 12 deletions(-)
> >
> >diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
> >index f8e350cef699..bfa39795208e 100644
> >--- a/drivers/usb/cdns3/core.h
> >+++ b/drivers/usb/cdns3/core.h
> >@@ -86,12 +86,12 @@ struct cdns {
> > struct resource xhci_res[CDNS_XHCI_RESOURCES_NUM];
> > struct cdns3_usb_regs __iomem *dev_regs;
> >
> >-struct resource otg_res;
> >-struct cdns3_otg_legacy_regs*otg_v0_regs;
> >-struct cdns3_otg_regs *otg_v1_regs;
> >-struct cdnsp_otg_regs *otg_cdnsp_regs;
> >-struct cdns_otg_common_regs *otg_regs;
> >-struct cdns_otg_irq_regs*otg_irq_regs;
> >+struct resource otg_res;
> >+struct cdns3_otg_legacy_regs __iomem*otg_v0_regs;
> >+struct cdns3_otg_regs __iomem *otg_v1_regs;
> >+struct cdnsp_otg_regs __iomem *otg_cdnsp_regs;
> >+struct cdns_otg_common_regs __iomem *otg_regs;
> >+struct cdns_otg_irq_reg __iomem *otg_irq_regs;
> > #define CDNS3_CONTROLLER_V0 0
> > #define CDNS3_CONTROLLER_V1 1
> > #define CDNSP_CONTROLLER_V2 2
> >diff --git a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c
> >index 605a413db727..0701853b501c 100644
> >--- a/drivers/usb/cdns3/drd.c
> >+++ b/drivers/usb/cdns3/drd.c
> >@@ -27,7 +27,7 @@
> > */
> > static int cdns_set_mode(struct cdns *cdns, enum usb_dr_mode mode)
> > {
> >-u32 __iomem *override_reg;
> >+void __iomem *override_reg;
> > u32 reg;
> >
> > switch (mode) {
> >@@ -406,7 +406,7 @@ int cdns_drd_init(struct cdns *cdns)
> > cdns->otg_v1_regs = NULL;
> > cdns->otg_cdnsp_regs = NULL;
> > cdns->otg_regs = regs;
> >-cdns->otg_irq_regs = (struct cdns_otg_irq_regs *)
> >+cdns->otg_irq_regs = (struct cdns_otg_irq_regs __iomem *)
> > >otg_v0_regs->ien;
> > writel(1, >otg_v0_regs->simulate);
> > dev_dbg(cdns->dev, "DRD version v0 (%08x)\n",
> >@@ -416,14 +416,14 @@ int cdns_drd_init(struct cdns *cdns)
> > cdns->otg_v1_regs = regs;
> > cdns->otg_cdnsp_regs = regs;
> >
> >-cdns->otg_regs = (void *)>otg_v1_regs->cmd;
> >+cdns->otg_regs = (void __iomem *)>otg_v1_regs->cmd;
> >
> >-if (cdns->otg_cdnsp_regs->did == OTG_CDNSP_DID) {
> >-cdns->otg_irq_regs = (struct cdns_otg_irq_regs *)
> >+if (readl(cdns->otg_cdnsp_regs->did) == OTG_CDNSP_DID) {
> >+cdns->otg_irq_regs = (struct cdns_otg_irq_regs __iomem
> >*)
> > >otg_cdnsp_regs->ien;
> > cdns->version = CDNSP_CONTROLLER_V2;
> > } else {
> >-cdns->otg_irq_regs = (struct cdns_otg_irq_regs *)
> >+cdns->otg_irq_regs = (struct cdns_otg_irq_regs __iomem
> >*)
> > >otg_v1_regs->ien;
> > writel(1, >otg_v1_regs->simulate);
> > cdns->version = CDNS3_CONTROLLER_V1;
> >--
> >2.17.1
>
--
Thanks,
Peter Chen
Re: [PATCH] Revert "usb: gadget: Quieten gadget config message"
On 21-01-07 10:50:38, Greg KH wrote: > On Thu, Jan 07, 2021 at 09:06:04AM +, Albert Wang wrote: > > This reverts commit 1cbfb8c4f62d667f6b8b3948949737edb92992cc. > > > > The log of USB enumeration result is a useful log and only occupies > > one line especially when USB3 enumeration failed and then downgrade > > to USB2. > > > > Signed-off-by: Albert Wang > > --- > > drivers/usb/gadget/composite.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c > > index c6d455f2bb92..5b0d6103a63d 100644 > > --- a/drivers/usb/gadget/composite.c > > +++ b/drivers/usb/gadget/composite.c > > @@ -840,9 +840,9 @@ static int set_config(struct usb_composite_dev *cdev, > > result = 0; > > } > > > > - DBG(cdev, "%s config #%d: %s\n", > > - usb_speed_string(gadget->speed), > > - number, c ? c->label : "unconfigured"); > > + INFO(cdev, "%s config #%d: %s\n", > > +usb_speed_string(gadget->speed), > > +number, c ? c->label : "unconfigured"); > > When everything is working properly, the kernel should be quiet. If you > have to see this message, you can turn it on at runtime, as Felipe > pointed out, to enable it for your system. But it's not a good default > thing to have. For most production kernel (or kernel ready for customers), it may not enable CONFIG_DYNAMIC_DEBUG. > > What do you need to see this message for? What tool relies on it? Who > reads it? > Usually, the developer him(her)self wants to read it to know if current USB gadget enumerated and what speed is recognized, reading from the console by eyes is the quickest way. Without this message, there is no any messages when the USB gadget connects to host. If enable debug message, there are too many messages, but I think most developers may only need one information message to know if enumeration is established correctly. -- Thanks, Peter Chen
Re: [PATCH v3 0/9] Support Runtime PM and host mode by Tegra ChipIdea USB driver
On 20-12-18 15:02:37, Dmitry Osipenko wrote: > This series implements Runtime PM support for the Tegra ChipIdea USB driver. > It also squashes the older ehci-tegra driver into the ChipIdea driver, hence > the RPM is supported by both UDC and host controllers, secondly this opens > opportunity for implementing OTG support in the future. > > Patchset was tested on various Tegra20, Tegra30 and Tegra124 devices. > Thanks to Peter Geis, Matt Merhar, Nicolas Chauvet and Ion Agorria for > helping with the extensive and productive testing! > > Changelog: > > v3: - Replaced "goto" with if-statements as was suggested by Thierry Reding. > > - Improved wording of the deprecated Kconfig entry as was suggested > by Alan Stern. > > - Added ACKs from Thierry Reding and Alan Stern. > > - Added a new minor patch "Specify TX FIFO threshold in UDC SoC info" > just for completeness, since we can now switch OTG to host mode in > the ChipIdea driver. Although, OTG support remains a work-in-progress > for now. > > v2: - Improved comments in the code as it was suggested by Peter Chen and > Sergei Shtylyov for v1. > > - Replaced mdelay() with fsleep() and made ci->hdc to reset to NULL in > a error code path, like it was suggested by Peter Chen. > > - Redirected deprecated USB_EHCI_TEGRA Kconfig entry to USB_CHIPIDEA_TEGRA > as was suggested by Alan Stern. > > - Improved commit message and added ACK from Thierry Reding to the patch > that removes MODULE_ALIAS. > > - Fixed UDC PHY waking up on ASUS TF201 tablet device by utilizing > additional VBUS sensor. This was reported and tested by Ion Agorria. > > - Added t-b from Ion Agorria. > > Dmitry Osipenko (8): > usb: phy: tegra: Add delay after power up > usb: phy: tegra: Support waking up from a low power mode > usb: chipidea: tegra: Remove MODULE_ALIAS > usb: chipidea: tegra: Rename UDC to USB > usb: chipidea: tegra: Support runtime PM > usb: chipidea: tegra: Specify TX FIFO threshold in UDC SoC info > usb: host: ehci-tegra: Remove the driver > ARM: tegra_defconfig: Enable USB_CHIPIDEA_HOST and remove > USB_EHCI_TEGRA > > Peter Geis (1): > usb: chipidea: tegra: Support host mode Chipidea related (patch 3-7) are applied, thanks. Peter > > arch/arm/configs/tegra_defconfig | 3 +- > drivers/usb/chipidea/Kconfig | 3 +- > drivers/usb/chipidea/ci_hdrc_tegra.c | 344 --- > drivers/usb/chipidea/core.c | 10 +- > drivers/usb/chipidea/host.c | 104 - > drivers/usb/host/Kconfig | 10 +- > drivers/usb/host/Makefile| 1 - > drivers/usb/host/ehci-tegra.c| 604 --- > drivers/usb/phy/phy-tegra-usb.c | 103 - > include/linux/usb/chipidea.h | 6 + > include/linux/usb/tegra_usb_phy.h| 2 + > 11 files changed, 518 insertions(+), 672 deletions(-) > delete mode 100644 drivers/usb/host/ehci-tegra.c > > -- > 2.29.2 > -- Thanks, Peter Chen
RE: [PATCH] usb: host: xhci: mvebu: make USB 3.0 PHY optional for Armada 3720
> > > > > > If not supported by ATF, then where to power on and off PHY since no other > place calls PHY APIs? Is it always on? > > > > Yes, in this case (when -EOPNOTSUPP is returned) SMC API is not > > supported by ATF, and PHY is always on. > > To make it clear, core/hcd.c function usb_add_hcd() when > hcd->skip_phy_initialization is false is calling > usb_phy_roothub_power_on() which calls phy_power_on(). If this call fails then > error is propagated back to the usb_add_hcd() and this function fails too. > > But on boards with older ATF (which do not support PHY power on SMC API) is > phy_power_on() returning error -EOPNOTSUPP and therefore whole USB > 3.0 initialization fails. > > This patch is adding init hook to detect if ATF supports PHY power on/off > functions and in case it does not support it, code sets XHCI_SKIP_PHY_INIT > flag > to instruct xhci-plat code to set > hcd->skip_phy_initialization flag to instruct core/hcd.c to skip calling > usb_phy_roothub_power_on() function as it is know that it would fail. Thanks for clarity, clear now. Peter
RE: [PATCH] usb: host: xhci: mvebu: make USB 3.0 PHY optional for Armada 3720
> > > + /* Old bindings miss the PHY handle */
> > > + phy = of_phy_get(dev->of_node, "usb3-phy");
> > > + if (IS_ERR(phy) && PTR_ERR(phy) == -EPROBE_DEFER)
> > > + return -EPROBE_DEFER;
> >
> > Doesn't need to judge IS_ERR(phy).
>
> Ok, I can remove it. I used same condition which is already in SATA and PCIe
> phy code.
>
> > > + else if (IS_ERR(phy))
> > > + goto phy_out;
> > > +
> > > + ret = phy_init(phy);
> > > + if (ret)
> > > + goto phy_put;
> > > +
> > > + ret = phy_set_mode(phy, PHY_MODE_USB_HOST_SS);
> > > + if (ret)
> > > + goto phy_exit;
> > > +
> > > + ret = phy_power_on(phy);
> > > + if (ret == -EOPNOTSUPP) {
> > > + /* Skip initializatin of XHCI PHY when it is unsupported by
> firmware */
> > > + dev_warn(dev, "PHY unsupported by firmware\n");
> > > + xhci->quirks |= XHCI_SKIP_PHY_INIT;
> > > + }
> > > + if (ret)
> > > + goto phy_exit;
> > > +
> > > + phy_power_off(phy);
> > > +phy_exit:
> > > + phy_exit(phy);
> > > +phy_put:
> > > + of_phy_put(phy);
> > > +phy_out:
> > > +
> >
> > You do power on and off again only want to know if PHY has already
> > powered at ATF, right?
>
> I need to know if power on/off procedure is supported by ATF. And if not
> (indicated by -EOPNOTSUPP) then I need to ensure that usb hdc code would not
> try to call phy_power_on() as it would cause failure as described in the
> commit
> message. You can look at those other two commits for PCIe and SATA. Same
> thing is needed for USB.
If not supported by ATF, then where to power on and off PHY since no other
place calls PHY APIs? Is it always on?
Peter
Re: [PATCH v2] usb: host: xhci-plat: fix support for XHCI_SKIP_PHY_INIT quirk
On 20-12-23 17:18:47, Pali Rohár wrote:
> Currently init_quirk callbacks for xhci platform drivers are called
> xhci_plat_setup() function which is called after chip reset completes.
> It happens in the middle of the usb_add_hcd() function.
>
> But XHCI_SKIP_PHY_INIT quirk is checked in the xhci_plat_probe() function
> prior calling usb_add_hcd() function. Therefore this XHCI_SKIP_PHY_INIT
> currently does nothing as prior xhci_plat_setup() it is not set.
>
> Quirk XHCI_SKIP_PHY_INIT is only setting hcd->skip_phy_initialization value
> which really needs to be set prior calling usb_add_hcd() as this function
> at its beginning skips PHY init if this member is set.
>
> This patch fixes implementation of the XHCI_SKIP_PHY_INIT quirk by calling
> init_quirk callbacks (via xhci_priv_init_quirk()) prior checking if
> XHCI_SKIP_PHY_INIT is set. Also checking if either xhci->quirks or
> priv->quirks contains this XHCI_SKIP_PHY_INIT quirk.
>
> Signed-off-by: Pali Rohár
>
> ---
> Changes in v2:
> * Check also xhci->quirks as xhci_priv_init_quirk() callbacks are setting
> xhci->quirks
> * Tested with "usb: host: xhci: mvebu: make USB 3.0 PHY optional for Armada
> 3720" patch
> * Removed Fixes: line
> ---
> drivers/usb/host/xhci-plat.c | 16
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 4d34f6005381..0eab7cb5a767 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -89,13 +89,6 @@ static void xhci_plat_quirks(struct device *dev, struct
> xhci_hcd *xhci)
> /* called during probe() after chip reset completes */
> static int xhci_plat_setup(struct usb_hcd *hcd)
> {
> - int ret;
> -
> -
> - ret = xhci_priv_init_quirk(hcd);
> - if (ret)
> - return ret;
> -
> return xhci_gen_setup(hcd, xhci_plat_quirks);
> }
>
> @@ -330,7 +323,14 @@ static int xhci_plat_probe(struct platform_device *pdev)
>
> hcd->tpl_support = of_usb_host_tpl_support(sysdev->of_node);
> xhci->shared_hcd->tpl_support = hcd->tpl_support;
> - if (priv && (priv->quirks & XHCI_SKIP_PHY_INIT))
> +
> + if (priv) {
> + ret = xhci_priv_init_quirk(hcd);
> + if (ret)
> + goto disable_usb_phy;
> + }
> +
> + if ((xhci->quirks & XHCI_SKIP_PHY_INIT) || (priv && (priv->quirks &
> XHCI_SKIP_PHY_INIT)))
> hcd->skip_phy_initialization = 1;
I am not sure if others agree with you move the position of
xhci_priv_init_quirk, Let's see Mathias opinion.
--
Thanks,
Peter Chen
Re: [PATCH] usb: host: xhci: mvebu: make USB 3.0 PHY optional for Armada 3720
BE_DEFER)
> + return -EPROBE_DEFER;
Doesn't need to judge IS_ERR(phy).
> + else if (IS_ERR(phy))
> + goto phy_out;
> +
> + ret = phy_init(phy);
> + if (ret)
> + goto phy_put;
> +
> + ret = phy_set_mode(phy, PHY_MODE_USB_HOST_SS);
> + if (ret)
> + goto phy_exit;
> +
> + ret = phy_power_on(phy);
> + if (ret == -EOPNOTSUPP) {
> + /* Skip initializatin of XHCI PHY when it is unsupported by
> firmware */
> + dev_warn(dev, "PHY unsupported by firmware\n");
> + xhci->quirks |= XHCI_SKIP_PHY_INIT;
> + }
> + if (ret)
> + goto phy_exit;
> +
> + phy_power_off(phy);
> +phy_exit:
> + phy_exit(phy);
> +phy_put:
> + of_phy_put(phy);
> +phy_out:
> +
You do power on and off again only want to know if PHY has already powered at
ATF, right?
--
Thanks,
Peter Chen
Re: [PATCH] usb: host: xhci-plat: fix support for XHCI_SKIP_PHY_INIT quirk
On 20-12-22 14:30:51, Pali Rohár wrote:
> On Tuesday 22 December 2020 10:23:27 Pali Rohár wrote:
> > On Tuesday 22 December 2020 02:14:45 Peter Chen wrote:
> > > On 20-12-21 16:09:03, Pali Rohár wrote:
> > > > Currently init_quirk callbacks for xhci platform drivers are called
> > > > xhci_plat_setup() function which is called after chip reset completes.
> > > > It happens in the middle of the usb_add_hcd() function.
> > > >
> > > > But XHCI_SKIP_PHY_INIT quirk is checked in the xhci_plat_probe()
> > > > function
> > > > prior calling usb_add_hcd() function. Therefore this XHCI_SKIP_PHY_INIT
> > > > currently does nothing as prior xhci_plat_setup() it is not set.
> > > >
> > > > Quirk XHCI_SKIP_PHY_INIT is only setting hcd->skip_phy_initialization
> > > > value
> > > > which really needs to be set prior calling usb_add_hcd() as this
> > > > function
> > > > at its beginning skips PHY init if this member is set.
> > > >
> > > > This patch fixes implementation of the XHCI_SKIP_PHY_INIT quirk by
> > > > calling
> > > > init_quirk callbacks (via xhci_priv_init_quirk()) prior checking if
> > > > XHCI_SKIP_PHY_INIT is set.
> > > >
> > > > Fixes: f768e718911e0 ("usb: host: xhci-plat: add priv quirk for skip
> > > > PHY initialization")
> > > > Signed-off-by: Pali Rohár
Hi Pali,
I know your case, you need to choose XHCI_SKIP_PHY_INIT quirk
conditionally, but you may can't add Fixes tag at your patch
since your issue was existed before my patch.
Peter
> > > > ---
> > > > drivers/usb/host/xhci-plat.c | 14 +++---
> > > > 1 file changed, 7 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > > > index 4d34f6005381..58704c5b002b 100644
> > > > --- a/drivers/usb/host/xhci-plat.c
> > > > +++ b/drivers/usb/host/xhci-plat.c
> > > > @@ -89,13 +89,6 @@ static void xhci_plat_quirks(struct device *dev,
> > > > struct xhci_hcd *xhci)
> > > > /* called during probe() after chip reset completes */
> > > > static int xhci_plat_setup(struct usb_hcd *hcd)
> > > > {
> > > > - int ret;
> > > > -
> > > > -
> > > > - ret = xhci_priv_init_quirk(hcd);
> > > > - if (ret)
> > > > - return ret;
> > > > -
> > > > return xhci_gen_setup(hcd, xhci_plat_quirks);
> > > > }
> > > >
> > > > @@ -330,6 +323,13 @@ static int xhci_plat_probe(struct platform_device
> > > > *pdev)
> > > >
> > > > hcd->tpl_support = of_usb_host_tpl_support(sysdev->of_node);
> > > > xhci->shared_hcd->tpl_support = hcd->tpl_support;
> > > > +
> > > > + if (priv) {
> > > > + ret = xhci_priv_init_quirk(hcd);
> > > > + if (ret)
> > > > + goto disable_usb_phy;
> > > > + }
> > > > +
> > > > if (priv && (priv->quirks & XHCI_SKIP_PHY_INIT))
> > > > hcd->skip_phy_initialization = 1;
> > > >
> > >
> > > Hi Pali,
> > >
> > > What's problem you have met? In structure xhci_plat_priv, the quirks are
> > > defined at .quirks entry which is got at below code. .init_quirk is the
> > > routine if special initializations are needed.
> >
> > Hello!
> >
> > I'm talking about .init_quirk. And if XHCI_SKIP_PHY_INIT quirk is set in
> > this function then has no effect.
>
> Ok, this patch is not enough, I will send V2.
>
> > I'm working currently on patch for xhci mvebu which conditionally enable
> > or disable XHCI_SKIP_PHY_INIT quirk (it is going to fix existing
> > regression since v5.1 kernel) and without this patch XHCI_SKIP_PHY_INIT
> > quirk from the init_quirk does not work.
>
> And now I have tested V2 with my mvebu regression fix. I will send it to
> mailing list for review.
>
> > > if (pdev->dev.of_node)
> > > priv_match = of_device_get_match_data(>dev);
> > > else
> > > priv_match = dev_get_platdata(>dev);
> > >
> > > if (priv_match) {
> > > priv = hcd_to_xhci_priv(hcd);
> > > /* Just copy data for now */
> > > *priv = *priv_match;
> > > }
> > > --
> > >
> > > Thanks,
> > > Peter Chen
--
Thanks,
Peter Chen
Re: [PATCH] usb: host: xhci-plat: fix support for XHCI_SKIP_PHY_INIT quirk
On 20-12-21 16:09:03, Pali Rohár wrote:
> Currently init_quirk callbacks for xhci platform drivers are called
> xhci_plat_setup() function which is called after chip reset completes.
> It happens in the middle of the usb_add_hcd() function.
>
> But XHCI_SKIP_PHY_INIT quirk is checked in the xhci_plat_probe() function
> prior calling usb_add_hcd() function. Therefore this XHCI_SKIP_PHY_INIT
> currently does nothing as prior xhci_plat_setup() it is not set.
>
> Quirk XHCI_SKIP_PHY_INIT is only setting hcd->skip_phy_initialization value
> which really needs to be set prior calling usb_add_hcd() as this function
> at its beginning skips PHY init if this member is set.
>
> This patch fixes implementation of the XHCI_SKIP_PHY_INIT quirk by calling
> init_quirk callbacks (via xhci_priv_init_quirk()) prior checking if
> XHCI_SKIP_PHY_INIT is set.
>
> Fixes: f768e718911e0 ("usb: host: xhci-plat: add priv quirk for skip PHY
> initialization")
> Signed-off-by: Pali Rohár
> ---
> drivers/usb/host/xhci-plat.c | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 4d34f6005381..58704c5b002b 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -89,13 +89,6 @@ static void xhci_plat_quirks(struct device *dev, struct
> xhci_hcd *xhci)
> /* called during probe() after chip reset completes */
> static int xhci_plat_setup(struct usb_hcd *hcd)
> {
> - int ret;
> -
> -
> - ret = xhci_priv_init_quirk(hcd);
> - if (ret)
> - return ret;
> -
> return xhci_gen_setup(hcd, xhci_plat_quirks);
> }
>
> @@ -330,6 +323,13 @@ static int xhci_plat_probe(struct platform_device *pdev)
>
> hcd->tpl_support = of_usb_host_tpl_support(sysdev->of_node);
> xhci->shared_hcd->tpl_support = hcd->tpl_support;
> +
> + if (priv) {
> + ret = xhci_priv_init_quirk(hcd);
> + if (ret)
> + goto disable_usb_phy;
> + }
> +
> if (priv && (priv->quirks & XHCI_SKIP_PHY_INIT))
> hcd->skip_phy_initialization = 1;
>
Hi Pali,
What's problem you have met? In structure xhci_plat_priv, the quirks are
defined at .quirks entry which is got at below code. .init_quirk is the
routine if special initializations are needed.
if (pdev->dev.of_node)
priv_match = of_device_get_match_data(>dev);
else
priv_match = dev_get_platdata(>dev);
if (priv_match) {
priv = hcd_to_xhci_priv(hcd);
/* Just copy data for now */
*priv = *priv_match;
}
--
Thanks,
Peter Chen
RE: [PATCH v1 5/8] usb: chipidea: tegra: Support host mode
> >>
> >> struct tegra_usb_soc_info {
> >>unsigned long flags;
> >> + unsigned int txfifothresh;
> >> + enum usb_dr_mode dr_mode;
> >> +};
> >> +
> >> +static const struct tegra_usb_soc_info tegra20_ehci_soc_info = {
> >> + .flags = CI_HDRC_REQUIRES_ALIGNED_DMA |
> >> + CI_HDRC_OVERRIDE_PHY_CONTROL,
> >> + .dr_mode = USB_DR_MODE_HOST,
> >> + .txfifothresh = 10,
> >> +};
> >> +
> >> +static const struct tegra_usb_soc_info tegra30_ehci_soc_info = {
> >> + .flags = CI_HDRC_REQUIRES_ALIGNED_DMA |
> >> + CI_HDRC_OVERRIDE_PHY_CONTROL
> >> + .dr_mode = USB_DR_MODE_HOST,
> >> + .txfifothresh = 16,
> >> };
> >>
> >> static const struct tegra_usb_soc_info tegra_udc_soc_info = {
> >> - .flags = CI_HDRC_REQUIRES_ALIGNED_DMA,
> >> + .flags = CI_HDRC_REQUIRES_ALIGNED_DMA |
> >> + CI_HDRC_OVERRIDE_PHY_CONTROL,
> >> + .dr_mode = USB_DR_MODE_UNKNOWN,
> >> };
> >
> > What's the usage for this dr_mode? Does these three SoCs only supports
> > single mode, it usually sets dr_mode at board dts file to indicate USB
> > role for this board.
>
> All Tegra SoCs have three USB controllers. Only one of these three
> controllers
> supports peripheral / OTG modes, the other two controllers are fixed to the
> host mode and device trees do not specify the dr_mode for the host
> controllers.
>
> Hence we need to enforce the dr_mode=host for the host-mode controllers.
>
> For UDC the default mode is "unknown" because actual mode comes from a
> board's device tree.
>
You could add dr_mode property at usb node of soc.dtsi to fulfill that.
> >> static const struct of_device_id tegra_usb_of_match[] = {
> >>{
> >> + .compatible = "nvidia,tegra20-ehci",
> >> + .data = _ehci_soc_info,
> >> + }, {
> >> + .compatible = "nvidia,tegra30-ehci",
> >> + .data = _ehci_soc_info,
> >> + }, {
> >>.compatible = "nvidia,tegra20-udc",
> >>.data = _udc_soc_info,
> >
> > Your compatible indicates this controller is single USB role, is it on
> > purpose?
>
> Yes, the "tegra20/30-ehci" controllers are fixed to the host-mode in hardware.
>
> ...
> >> +static int tegra_usb_internal_port_reset(struct ehci_hcd *ehci,
> >> + u32 __iomem *portsc_reg,
> >> + unsigned long *flags)
> >> +{
> >> + u32 saved_usbintr, temp;
> >> + unsigned int i, tries;
> >> + int retval = 0;
> >> +
> >> + saved_usbintr = ehci_readl(ehci, >regs->intr_enable);
> >> + /* disable USB interrupt */
> >> + ehci_writel(ehci, 0, >regs->intr_enable);
> >> + spin_unlock_irqrestore(>lock, *flags);
> >> +
> >> + /*
> >> + * Here we have to do Port Reset at most twice for
> >> + * Port Enable bit to be set.
> >> + */
> >> + for (i = 0; i < 2; i++) {
> >> + temp = ehci_readl(ehci, portsc_reg);
> >> + temp |= PORT_RESET;
> >> + ehci_writel(ehci, temp, portsc_reg);
> >> + mdelay(10);
> >
> > Needs accurate delay? How about usleep_range?
>
> To be hones I don't know for sure whether hub_control() could be used from
> interrupt context. This mdelay is borrowed from the older ehci-tegra driver.
>
> Are you suggesting that it should be safe to sleep here?
>
I see msleep is called at tegra_ehci_hub_control at ehci-tegra.c.
.hub_control is not called at interrupt context.
> ...
> >> static int tegra_usb_probe(struct platform_device *pdev) {
> >>const struct tegra_usb_soc_info *soc; @@ -83,24 +292,49 @@
> static
> >> int tegra_usb_probe(struct platform_device *pdev)
> >>return err;
> >>}
> >>
> >> + if (device_property_present(>dev, "nvidia,needs-double-reset"))
> >> + usb->needs_double_reset = true;
> >> +
> >> + err = tegra_usb_reset_controller(>dev);
> >> + if (err) {
> >> + dev_err(>dev, "failed to reset controller: %d\n", err);
> >> + goto fail_power_off;
> >> + }
> >> +
> >> + /*
> >> + * USB controller registers shan't be touched before PHY is
> >
> > %s/shan't/shouldn't
>
> ok
>
> ...
> >> static int ehci_ci_portpower(struct usb_hcd *hcd, int portnum, bool
> >> enable) {
> >>struct ehci_hcd *ehci = hcd_to_ehci(hcd); @@ -160,14 +166,14 @@
> >> static int host_start(struct ci_hdrc *ci)
> >>pinctrl_select_state(ci->platdata->pctl,
> >> ci->platdata->pins_host);
> >>
> >> + ci->hcd = hcd;
> >> +
> >>ret = usb_add_hcd(hcd, 0, 0);
> >>if (ret) {
> >>goto disable_reg;
> >>} else {
> >>struct usb_otg *otg = >otg;
> >>
> >> - ci->hcd = hcd;
> >> -
> >
> > Why this changed?
>
> The ci->hcd is used by tegra_usb_notify_event() to handle reset event and the
> reset happens once usb_add_hcd() is invoked. Hence we need to pre-assign
> the hcd pointer, otherwise there will be a NULL dereference.
Get it, please set ci->hcd as NULL at error path.
Peter
Re: [PATCH v1 5/8] usb: chipidea: tegra: Support host mode
uffer *temp;
> + size_t length;
> +
> + if (!(urb->transfer_flags & URB_ALIGNED_TEMP_BUFFER))
> + return;
> +
> + temp = container_of(urb->transfer_buffer,
> + struct ci_hdrc_dma_aligned_buffer, data);
> +
> + if (usb_urb_dir_in(urb)) {
> + if (usb_pipeisoc(urb->pipe))
> + length = urb->transfer_buffer_length;
> + else
> + length = urb->actual_length;
> +
> + memcpy(temp->old_xfer_buffer, temp->data, length);
> + }
> + urb->transfer_buffer = temp->old_xfer_buffer;
> + kfree(temp->kmalloc_ptr);
> +
> + urb->transfer_flags &= ~URB_ALIGNED_TEMP_BUFFER;
> +}
> +
> +static int ci_hdrc_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags)
> +{
> + struct ci_hdrc_dma_aligned_buffer *temp, *kmalloc_ptr;
> + const unsigned int ci_hdrc_usb_dma_align = 32;
> + size_t kmalloc_size;
> +
> + if (urb->num_sgs || urb->sg || urb->transfer_buffer_length == 0 ||
> + !((uintptr_t)urb->transfer_buffer & (ci_hdrc_usb_dma_align - 1)))
> + return 0;
> +
> + /* Allocate a buffer with enough padding for alignment */
> + kmalloc_size = urb->transfer_buffer_length +
> +sizeof(struct ci_hdrc_dma_aligned_buffer) +
> +ci_hdrc_usb_dma_align - 1;
> +
> + kmalloc_ptr = kmalloc(kmalloc_size, mem_flags);
> + if (!kmalloc_ptr)
> + return -ENOMEM;
> +
> + /* Position our struct dma_aligned_buffer such that data is aligned */
> + temp = PTR_ALIGN(kmalloc_ptr + 1, ci_hdrc_usb_dma_align) - 1;
> + temp->kmalloc_ptr = kmalloc_ptr;
> + temp->old_xfer_buffer = urb->transfer_buffer;
> + if (usb_urb_dir_out(urb))
> + memcpy(temp->data, urb->transfer_buffer,
> +urb->transfer_buffer_length);
> + urb->transfer_buffer = temp->data;
> +
> + urb->transfer_flags |= URB_ALIGNED_TEMP_BUFFER;
> +
> + return 0;
> +}
> +
> +static int ci_hdrc_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
> +gfp_t mem_flags)
> +{
> + int ret;
> +
> + ret = ci_hdrc_alloc_dma_aligned_buffer(urb, mem_flags);
> + if (ret)
> + return ret;
> +
> + ret = usb_hcd_map_urb_for_dma(hcd, urb, mem_flags);
> + if (ret)
> + ci_hdrc_free_dma_aligned_buffer(urb);
> +
> + return ret;
> +}
> +
> +static void ci_hdrc_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
> +{
> + usb_hcd_unmap_urb_for_dma(hcd, urb);
> + ci_hdrc_free_dma_aligned_buffer(urb);
> +}
> +
> int ci_hdrc_host_init(struct ci_hdrc *ci)
> {
> struct ci_role_driver *rdrv;
> @@ -366,6 +460,11 @@ int ci_hdrc_host_init(struct ci_hdrc *ci)
> rdrv->name = "host";
> ci->roles[CI_ROLE_HOST] = rdrv;
>
> + if (ci->platdata->flags & CI_HDRC_REQUIRES_ALIGNED_DMA) {
> + ci_ehci_hc_driver.map_urb_for_dma = ci_hdrc_map_urb_for_dma;
> + ci_ehci_hc_driver.unmap_urb_for_dma = ci_hdrc_unmap_urb_for_dma;
> + }
> +
> return 0;
> }
>
> diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
> index 025b41687ce9..edf3342507f1 100644
> --- a/include/linux/usb/chipidea.h
> +++ b/include/linux/usb/chipidea.h
> @@ -88,6 +88,12 @@ struct ci_hdrc_platform_data {
> struct pinctrl_state *pins_default;
> struct pinctrl_state *pins_host;
> struct pinctrl_state *pins_device;
> +
> + /* platform-specific hooks */
> + int (*hub_control)(struct ci_hdrc *ci, u16 typeReq, u16 wValue,
> +u16 wIndex, char *buf, u16 wLength,
> +bool *done, unsigned long *flags);
> + void (*enter_lpm)(struct ci_hdrc *ci, bool enable);
> };
>
> /* Default offset of capability registers */
> --
> 2.29.2
>
--
Thanks,
Peter Chen
Re: [PATCH 1/2] usb: cdnsp: Fixes for sparse warnings
On 20-12-15 06:14:07, Pawel Laszczak wrote: > >On 20-12-15 05:27:38, Pawel Laszczak wrote: > >> > > >> > > >> >On 20-12-14 13:03:44, Pawel Laszczak wrote: > >> >> Patch fixes all sparse warnings in cdsnp driver. > >> >> > >> >> It fixes the following warnings: > >> >> cdnsp-ring.c:1441: warning: incorrect type in assignment > >> >> cdnsp-ring.c:1444: warning: restricted __le32 degrades to integer > >> >> cdnsp-ring.c:2200: warning: dubious: x | !y > >> >> cdnsp-gadget.c:501: warning: incorrect type in assignment > >> >> cdnsp-gadget.c:504: warning: restricted __le32 degrades to integer > >> >> cdnsp-gadget.c:507: warning: restricted __le32 degrades to integer > >> >> cdnsp-gadget.c:508: warning: restricted __le32 degrades to integer > >> >> cdnsp-gadget.c:509: warning: invalid assignment: |= > >> >> cdnsp-gadget.c:510: warning: cast from restricted __le32 > >> >> cdnsp-gadget.c:558: warning: incorrect type in assignment > >> >> cdnsp-gadget.c:561: warning: restricted __le32 degrades to integer > >> >> cdnsp-gadget.c:570: warning: restricted __le32 degrades to integer > >> >> cdnsp-gadget.c:1571: warning: incorrect type in argument 1 > >> >> cdnsp-gadget.c:1602: warning: restricted __le32 degrades to integer > >> >> cdnsp-gadget.c:1760: warning: incorrect type in assignment > >> >> cdnsp-gadget.c:1762: warning: incorrect type in assignment > >> >> cdnsp-gadget.c:1763: warning: incorrect type in assignment > >> >> cdnsp-gadget.c:1764: warning: incorrect type in assignment > >> >> cdnsp-gadget.c:1765: warning: incorrect type in assignment > >> >> cdnsp-gadget.c:1766: warning: incorrect type in assignment > >> >> cdnsp-gadget.c:1767: warning: incorrect type in assignment > >> >> cdnsp-gadget.c:458: warning: cast truncates bits from constant value > >> >> (07ff becomes 7ff) > >> >> cdnsp-gadget.c:666: warning: cast truncates bits from constant value > >> >> (07ff becomes 7ff) > >> >> cdnsp-mem.c:762: warning: incorrect type in assignment > >> >> cdnsp-mem.c:763: warning: incorrect type in assignment > >> >> cdnsp-mem.c:928: warning: cast from restricted __le16 > >> >> cdnsp-mem.c:1187: warning: incorrect type in assignment > >> >> cdnsp-mem.c:1191: warning: incorrect type in assignment > >> >> cdnsp-ep0.c:142: warning: incorrect type in assignment > >> >> cdnsp-ep0.c:144: warning: restricted __le32 degrades to integer > >> >> cdnsp-ep0.c:147: warning: restricted __le32 degrades to integer > >> >> cdnsp-ep0.c:148: warning: restricted __le32 degrades to integer > >> >> cdnsp-ep0.c:179: warning: incorrect type in argument 1 > >> >> cdnsp-ep0.c:311: warning: incorrect type in argument 1 > >> >> cdnsp-ep0.c:469: warning: incorrect type in assignment > >> >> cdnsp-trace.h:611:1: warning: cast from restricted __le32 > >> >> > >> >> Signed-off-by: Pawel Laszczak > >> >> Reported-by: kernel test robot > >> > > >> >Hi Pawel, > >> > > >> >The Reported-by tag should be above your Sob tag, I will change it. > >> >Except the patch reported build error by kernel test robot, I will apply > >> >your other four patches after finishing the compile test. > >> > > >> >Peter > >> > >> Hi Peter, > >> > >> I'm going to fix the "usb: cdns3: Adds missing __iomem markers" today. > >> I haven't seen any issue on ARCH=parisc. Maybe it's some specific riscv > >> arch issue. > >> > >> I believe that: > >> [auto build test WARNING on next-20201211] > >> [cannot apply to peter.chen-usb/ci-for-usb-next v5.10 v5.10-rc7 v5.10-rc6 > >> v5.10] > >> > >> is not the problem. I based on peter.chen-usb/for-usb-next. > >> > >> Also I can't open the url from kernel test robot report. > >> Maybe there is some temporary issue with server. > >> > > > >Thanks for checking it, I have already pushed your other four patches. > >Besides, there is still a build error issue for new cdns3 driver. > > > >https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Furldefense.com%2Fv3%2F__https%3A%2F%2Fwww.spinics.net%2Flists%2Flinux-data=04%7C01%7Cpeter.chen%40nxp.com%7Cf036cd7630664c9e0c5c08d8a0c0a637%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637436096594708469%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=DLBFVB2px5GgA6Y%2FTU4DrfVru6z3P4RXz2x7BSpdE4o%3Dreserved=0 > >usb/msg206073.html__;!!EHscmS1ygiU1lA!X6rYk64ILtzjyHW903LAhBRjMKi9C2eyJWEXVlEZm0ly2BiNzY2wK46Ulq7q5w$ > > > > Did you applied: [PATCH] usb: cdnsp: Fix for undefined reference to > `usb_hcd_is_primary_hcd' ? > Applied now. -- Thanks, Peter Chen
Re: [PATCH 1/2] usb: cdnsp: Fixes for sparse warnings
t_ctx->dev_state = (pdev->device_address & DEV_ADDR_MASK);
> >> - ep0_ctx->tx_info = EP_AVG_TRB_LENGTH(0x8);
> >> + slot_ctx->dev_state = cpu_to_le32((pdev->device_address &
> >> + DEV_ADDR_MASK));
> >> + ep0_ctx->tx_info = cpu_to_le32(EP_AVG_TRB_LENGTH(0x8));
> >>ep0_ctx->ep_info2 = cpu_to_le32(EP_TYPE(CTRL_EP));
> >>ep0_ctx->ep_info2 |= cpu_to_le32(MAX_BURST(0) | ERROR_COUNT(3) |
> >> max_packets);
> >> @@ -925,7 +926,7 @@ static u32 cdnsp_get_max_esit_payload(struct
> >> usb_gadget *g,
> >>/* SuperSpeedPlus Isoc ep sending over 48k per EIST. */
> >>if (g->speed >= USB_SPEED_SUPER_PLUS &&
> >>USB_SS_SSP_ISOC_COMP(pep->endpoint.desc->bmAttributes))
> >> - return le32_to_cpu(pep->endpoint.comp_desc->wBytesPerInterval);
> >> + return le16_to_cpu(pep->endpoint.comp_desc->wBytesPerInterval);
> >>/* SuperSpeed or SuperSpeedPlus Isoc ep with less than 48k per esit */
> >>else if (g->speed >= USB_SPEED_SUPER)
> >>return le16_to_cpu(pep->endpoint.comp_desc->wBytesPerInterval);
> >> @@ -1184,11 +1185,11 @@ static int cdnsp_setup_port_arrays(struct
> >> cdnsp_device *pdev)
> >>
> >>trace_cdnsp_init("Found USB 2.0 ports and USB 3.0 ports.");
> >>
> >> - pdev->usb2_port.regs = (struct cdnsp_port_regs *)
> >> + pdev->usb2_port.regs = (struct cdnsp_port_regs __iomem *)
> >> (>op_regs->port_reg_base + NUM_PORT_REGS *
> >>(pdev->usb2_port.port_num - 1));
> >>
> >> - pdev->usb3_port.regs = (struct cdnsp_port_regs *)
> >> + pdev->usb3_port.regs = (struct cdnsp_port_regs __iomem *)
> >> (>op_regs->port_reg_base + NUM_PORT_REGS *
> >>(pdev->usb3_port.port_num - 1));
> >>
> >> diff --git a/drivers/usb/cdns3/cdnsp-ring.c
> >> b/drivers/usb/cdns3/cdnsp-ring.c
> >> index 874d9ff5406c..e15e13ba27dc 100644
> >> --- a/drivers/usb/cdns3/cdnsp-ring.c
> >> +++ b/drivers/usb/cdns3/cdnsp-ring.c
> >> @@ -1432,7 +1432,7 @@ static bool cdnsp_handle_event(struct cdnsp_device
> >> *pdev)
> >>unsigned int comp_code;
> >>union cdnsp_trb *event;
> >>bool update_ptrs = true;
> >> - __le32 cycle_bit;
> >> + u32 cycle_bit;
> >>int ret = 0;
> >>u32 flags;
> >>
> >> @@ -2198,7 +2198,7 @@ static int cdnsp_queue_isoc_tx(struct cdnsp_device
> >> *pdev,
> >> * inverted in the first TDs isoc TRB.
> >> */
> >>field = TRB_TYPE(TRB_ISOC) | TRB_TLBPC(last_burst_pkt) |
> >> - !start_cycle | TRB_SIA | TRB_TBC(burst_count);
> >> + start_cycle ? 0 : 1 | TRB_SIA | TRB_TBC(burst_count);
> >>
> >>/* Fill the rest of the TRB fields, and remaining normal TRBs. */
> >>for (i = 0; i < trbs_per_td; i++) {
> >> diff --git a/drivers/usb/cdns3/cdnsp-trace.h
> >> b/drivers/usb/cdns3/cdnsp-trace.h
> >> index b68e282464d2..a9de1daadf07 100644
> >> --- a/drivers/usb/cdns3/cdnsp-trace.h
> >> +++ b/drivers/usb/cdns3/cdnsp-trace.h
> >> @@ -620,7 +620,7 @@ DECLARE_EVENT_CLASS(cdnsp_log_slot_ctx,
> >>TP_fast_assign(
> >>__entry->info = le32_to_cpu(ctx->dev_info);
> >>__entry->info2 = le32_to_cpu(ctx->dev_port);
> >> - __entry->int_target = le64_to_cpu(ctx->int_target);
> >> + __entry->int_target = le32_to_cpu(ctx->int_target);
> >>__entry->state = le32_to_cpu(ctx->dev_state);
> >>),
> >>TP_printk("%s", cdnsp_decode_slot_context(__entry->info,
> >> --
> >> 2.17.1
> >>
>
> --
>
> Regards
> Pawel Laszcak
--
Thanks,
Peter Chen
Re: [PATCH] usb: cdns3: Adds missing __iomem markers
g_irq_regs = (struct cdns_otg_irq_regs
> __iomem *)
>410 >otg_v0_regs->ien;
>411writel(1, >otg_v0_regs->simulate);
>412dev_dbg(cdns->dev, "DRD version v0 (%08x)\n",
>413 readl(>otg_v0_regs->version));
>414} else {
>415cdns->otg_v0_regs = NULL;
>416cdns->otg_v1_regs = regs;
>417cdns->otg_cdnsp_regs = regs;
>418
>419cdns->otg_regs = (void __iomem
> *)>otg_v1_regs->cmd;
>420
> > 421if (readl(cdns->otg_cdnsp_regs->did) ==
> OTG_CDNSP_DID) {
>422cdns->otg_irq_regs = (struct
> cdns_otg_irq_regs __iomem *)
>423
> >otg_cdnsp_regs->ien;
>424cdns->version = CDNSP_CONTROLLER_V2;
>425} else {
>426cdns->otg_irq_regs = (struct
> cdns_otg_irq_regs __iomem *)
>427
> >otg_v1_regs->ien;
>428writel(1, >otg_v1_regs->simulate);
>429cdns->version = CDNS3_CONTROLLER_V1;
>430}
>431
>432dev_dbg(cdns->dev, "DRD version v1 (ID: %08x,
> rev: %08x)\n",
>433 readl(>otg_v1_regs->did),
>434 readl(>otg_v1_regs->rid));
>435}
>436
>437state = OTGSTS_STRAP(readl(>otg_regs->sts));
>438
>439/* Update dr_mode according to STRAP configuration. */
>440cdns->dr_mode = USB_DR_MODE_OTG;
>441
>442if ((cdns->version == CDNSP_CONTROLLER_V2 &&
>443 state == OTGSTS_CDNSP_STRAP_HOST) ||
>444(cdns->version != CDNSP_CONTROLLER_V2 &&
>445 state == OTGSTS_STRAP_HOST)) {
>446dev_dbg(cdns->dev, "Controller strapped to
> HOST\n");
>447cdns->dr_mode = USB_DR_MODE_HOST;
>448} else if ((cdns->version == CDNSP_CONTROLLER_V2 &&
>449state == OTGSTS_CDNSP_STRAP_GADGET) ||
>450 (cdns->version != CDNSP_CONTROLLER_V2 &&
>451state == OTGSTS_STRAP_GADGET)) {
>452dev_dbg(cdns->dev, "Controller strapped to
> PERIPHERAL\n");
>453cdns->dr_mode = USB_DR_MODE_PERIPHERAL;
>454}
>455
>456ret = devm_request_threaded_irq(cdns->dev,
> cdns->otg_irq,
>457cdns_drd_irq,
>458cdns_drd_thread_irq,
>459IRQF_SHARED,
>460dev_name(cdns->dev),
> cdns);
>461if (ret) {
>462dev_err(cdns->dev, "couldn't get otg_irq\n");
> 463 return ret;
>464}
>465
>466state = readl(>otg_regs->sts);
>467if (OTGSTS_OTG_NRDY(state)) {
>468dev_err(cdns->dev, "Cadence USB3 OTG device not
> ready\n");
>469return -ENODEV;
>470}
>471
>472return 0;
>473}
>474
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.01.org%2Fhyperkitty%2Flist%2Fkbuild-all%40lists.01.orgdata=04%7C01%7Cpeter.chen%40nxp.com%7C6ce79474794448ae12b008d8a045f9ce%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637435572341553421%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=v9fQGKZKobtIysXu43lCekV%2FoXCc2EZZXIxoTtQpSdw%3Dreserved=0
--
Thanks,
Peter Chen
Re: [PATCH -next] usb: phy: convert comma to semicolon
On 20-12-11 16:54:28, Zheng Yongjun wrote: > Replace a comma between expression statements by a semicolon. > > Signed-off-by: Zheng Yongjun Reviewed-by: Peter Chen Peter > --- > drivers/usb/phy/phy-isp1301-omap.c | 10 +- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/usb/phy/phy-isp1301-omap.c > b/drivers/usb/phy/phy-isp1301-omap.c > index 4a6462c92ef2..6f4f74e6ba51 100644 > --- a/drivers/usb/phy/phy-isp1301-omap.c > +++ b/drivers/usb/phy/phy-isp1301-omap.c > @@ -1566,13 +1566,13 @@ isp1301_probe(struct i2c_client *i2c, const struct > i2c_device_id *id) > > isp->phy.dev = >dev; > isp->phy.label = DRIVER_NAME; > - isp->phy.set_power = isp1301_set_power, > + isp->phy.set_power = isp1301_set_power; > > isp->phy.otg->usb_phy = >phy; > - isp->phy.otg->set_host = isp1301_set_host, > - isp->phy.otg->set_peripheral = isp1301_set_peripheral, > - isp->phy.otg->start_srp = isp1301_start_srp, > - isp->phy.otg->start_hnp = isp1301_start_hnp, > + isp->phy.otg->set_host = isp1301_set_host; > + isp->phy.otg->set_peripheral = isp1301_set_peripheral; > + isp->phy.otg->start_srp = isp1301_start_srp; > + isp->phy.otg->start_hnp = isp1301_start_hnp; > > enable_vbus_draw(isp, 0); > power_down(isp); > -- > 2.22.0 > -- Thanks, Peter Chen
Re: [PATCH 1/2] usb: cdnsp: Fixes for sparse warnings
SPEED) >> 20)
> /* Index of the last valid endpoint context in this device context - 27:31.
> */
> -#define LAST_CTX_MASKGENMASK(31, 27)
> +#define LAST_CTX_MASK((unsigned int)GENMASK(31, 27))
> #define LAST_CTX(p) ((p) << 27)
> #define LAST_CTX_TO_EP_NUM(p)(((p) >> 27) - 1)
> #define SLOT_FLAGBIT(0)
> @@ -1351,9 +1352,9 @@ struct cdnsp_port {
> * @ir_set: Current interrupter register set.
> * @port20_regs: Port 2.0 Peripheral Configuration Registers.
> * @port3x_regs: USB3.x Port Peripheral Configuration Registers.
> + * @rev_cap: Controller Capabilities Registers.
> * @hcs_params1: Cached register copies of read-only HCSPARAMS1
> * @hcc_params: Cached register copies of read-only HCCPARAMS1
> - * @rev_cap: Controller capability.
> * @setup: Temporary buffer for setup packet.
> * @ep0_preq: Internal allocated request used during enumeration.
> * @ep0_stage: ep0 stage during enumeration process.
> @@ -1402,12 +1403,12 @@ struct cdnsp_device {
> struct cdnsp_intr_reg __iomem *ir_set;
> struct cdnsp_20port_cap __iomem *port20_regs;
> struct cdnsp_3xport_cap __iomem *port3x_regs;
> + struct cdnsp_rev_cap __iomem *rev_cap;
>
> /* Cached register copies of read-only CDNSP data */
> __u32 hcs_params1;
> __u32 hcs_params3;
> __u32 hcc_params;
> - struct cdnsp_rev_cap rev_cap;
> /* Lock used in interrupt thread context. */
> spinlock_t lock;
> struct usb_ctrlrequest setup;
> diff --git a/drivers/usb/cdns3/cdnsp-mem.c b/drivers/usb/cdns3/cdnsp-mem.c
> index 4c7d77fb097e..7a84e928710e 100644
> --- a/drivers/usb/cdns3/cdnsp-mem.c
> +++ b/drivers/usb/cdns3/cdnsp-mem.c
> @@ -759,8 +759,9 @@ int cdnsp_setup_addressable_priv_dev(struct cdnsp_device
> *pdev)
>
> port = DEV_PORT(pdev->active_port->port_num);
> slot_ctx->dev_port |= cpu_to_le32(port);
> - slot_ctx->dev_state = (pdev->device_address & DEV_ADDR_MASK);
> - ep0_ctx->tx_info = EP_AVG_TRB_LENGTH(0x8);
> + slot_ctx->dev_state = cpu_to_le32((pdev->device_address &
> +DEV_ADDR_MASK));
> + ep0_ctx->tx_info = cpu_to_le32(EP_AVG_TRB_LENGTH(0x8));
> ep0_ctx->ep_info2 = cpu_to_le32(EP_TYPE(CTRL_EP));
> ep0_ctx->ep_info2 |= cpu_to_le32(MAX_BURST(0) | ERROR_COUNT(3) |
>max_packets);
> @@ -925,7 +926,7 @@ static u32 cdnsp_get_max_esit_payload(struct usb_gadget
> *g,
> /* SuperSpeedPlus Isoc ep sending over 48k per EIST. */
> if (g->speed >= USB_SPEED_SUPER_PLUS &&
> USB_SS_SSP_ISOC_COMP(pep->endpoint.desc->bmAttributes))
> - return le32_to_cpu(pep->endpoint.comp_desc->wBytesPerInterval);
> + return le16_to_cpu(pep->endpoint.comp_desc->wBytesPerInterval);
> /* SuperSpeed or SuperSpeedPlus Isoc ep with less than 48k per esit */
> else if (g->speed >= USB_SPEED_SUPER)
> return le16_to_cpu(pep->endpoint.comp_desc->wBytesPerInterval);
> @@ -1184,11 +1185,11 @@ static int cdnsp_setup_port_arrays(struct
> cdnsp_device *pdev)
>
> trace_cdnsp_init("Found USB 2.0 ports and USB 3.0 ports.");
>
> - pdev->usb2_port.regs = (struct cdnsp_port_regs *)
> + pdev->usb2_port.regs = (struct cdnsp_port_regs __iomem *)
> (>op_regs->port_reg_base + NUM_PORT_REGS *
> (pdev->usb2_port.port_num - 1));
>
> - pdev->usb3_port.regs = (struct cdnsp_port_regs *)
> + pdev->usb3_port.regs = (struct cdnsp_port_regs __iomem *)
> (>op_regs->port_reg_base + NUM_PORT_REGS *
> (pdev->usb3_port.port_num - 1));
>
> diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c
> index 874d9ff5406c..e15e13ba27dc 100644
> --- a/drivers/usb/cdns3/cdnsp-ring.c
> +++ b/drivers/usb/cdns3/cdnsp-ring.c
> @@ -1432,7 +1432,7 @@ static bool cdnsp_handle_event(struct cdnsp_device
> *pdev)
> unsigned int comp_code;
> union cdnsp_trb *event;
> bool update_ptrs = true;
> - __le32 cycle_bit;
> + u32 cycle_bit;
> int ret = 0;
> u32 flags;
>
> @@ -2198,7 +2198,7 @@ static int cdnsp_queue_isoc_tx(struct cdnsp_device
> *pdev,
>* inverted in the first TDs isoc TRB.
>*/
> field = TRB_TYPE(TRB_ISOC) | TRB_TLBPC(last_burst_pkt) |
> - !start_cycle | TRB_SIA | TRB_TBC(burst_count);
> + start_cycle ? 0 : 1 | TRB_SIA | TRB_TBC(burst_count);
>
> /* Fill the rest of the TRB fields, and remaining normal TRBs. */
> for (i = 0; i < trbs_per_td; i++) {
> diff --git a/drivers/usb/cdns3/cdnsp-trace.h b/drivers/usb/cdns3/cdnsp-trace.h
> index b68e282464d2..a9de1daadf07 100644
> --- a/drivers/usb/cdns3/cdnsp-trace.h
> +++ b/drivers/usb/cdns3/cdnsp-trace.h
> @@ -620,7 +620,7 @@ DECLARE_EVENT_CLASS(cdnsp_log_slot_ctx,
> TP_fast_assign(
> __entry->info = le32_to_cpu(ctx->dev_info);
> __entry->info2 = le32_to_cpu(ctx->dev_port);
> - __entry->int_target = le64_to_cpu(ctx->int_target);
> + __entry->int_target = le32_to_cpu(ctx->int_target);
> __entry->state = le32_to_cpu(ctx->dev_state);
> ),
> TP_printk("%s", cdnsp_decode_slot_context(__entry->info,
> --
> 2.17.1
>
--
Thanks,
Peter Chen
Re: linux-next: Tree for Dec 9 (usb/cdns3)
On 20-12-09 16:58:16, Randy Dunlap wrote:
> On 12/9/20 2:44 AM, Stephen Rothwell wrote:
> > Hi all,
> >
> > Changes since 20201208:
> >
>
> (I don't know what to do about this one -- seeking help.)
Add Pawel.
Hi Pawel,
Add old cdns3 logic, when the CONFIG_USB=m
If CONFIG_USB_CDNS3 is M, the host will be built as module
If CONFIG_USB_CDNS3 is build-in, the host will not built due to
USB=m, so USB!= USB_CDNS3 at below dependency.
config USB_CDNS3_HOST
bool "Cadence USB3 host controller"
depends on USB=y || USB=USB_CDNS3
So, it has no such issue.
But after adding CDNSSP support, the configuration relationship is
much complicated, both CDNS3 and CDNSSP could choose host file,
would you have a check for this issue?
Peter
>
>
> on x86_64:
>
> ld: drivers/usb/cdns3/host.o: in function `xhci_cdns3_suspend_quirk':
> host.c:(.text+0x9): undefined reference to `usb_hcd_is_primary_hcd'
>
> This reference to 'usb_hdc_is_primary_hcd' is from hcd_to_xhci(),
> which is being built as a loadable module:
>
> int xhci_cdns3_suspend_quirk(struct usb_hcd *hcd)
> {
> struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>
>
>
>
> CONFIG_USB_GADGET=y
> CONFIG_USB_SUPPORT=y
> CONFIG_USB_COMMON=y
> # CONFIG_USB_CONN_GPIO is not set
> CONFIG_USB_ARCH_HAS_HCD=y
> CONFIG_USB=m
>
> CONFIG_USB_CDNS_SUPPORT=y
> CONFIG_USB_CDNS_HOST=y
> CONFIG_USB_CDNS3=m
> CONFIG_USB_CDNS3_GADGET=y
> CONFIG_USB_CDNS3_HOST=y
>
> Problem is mostly that CONFIG_USB=m and CONFIG_USB_GADGET=y.
>
>
> Full randconfig file is attached.
>
>
> thanks.
> --
> ~Randy
> Reported-by: Randy Dunlap
--
Thanks,
Peter Chen
Re: [PATCH v5 00/10] Introduced new Cadence USBSSP DRD Driver.
On 20-12-07 11:32:17, Pawel Laszczak wrote:
> This patch introduce new Cadence USBSS DRD driver to linux kernel.
>
> The Cadence USBSS DRD Controller is a highly configurable IP Core which
> can be instantiated as Dual-Role Device (DRD), Peripheral Only and
> Host Only (XHCI)configurations.
>
> The current driver has been validated with FPGA burned. We have support
> for PCIe bus, which is used on FPGA prototyping.
>
> The host side of USBSS-DRD controller is compliance with XHCI
> specification, so it works with standard XHCI Linux driver.
>
> The device side of USBSS DRD controller is compliant with XHCI.
> The architecture for device side is almost the same as for host side,
> and most of the XHCI specification can be used to understand how
> this controller operates.
>
> This controller and driver support Full Speed, Hight Speed, Supper Speed
> and Supper Speed Plus USB protocol.
>
> The prefix cdnsp used in driver has chosen by analogy to cdn3 driver.
> The last letter of this acronym means PLUS. The formal name of controller
> is USBSSP but it's to generic so I've decided to use CDNSP.
>
> The patch 1: adds support for DRD CDNSP.
> The patch 2: separates common code that can be reusable by cdnsp driver.
> The patch 3: moves reusable code to separate module.
> The patch 4: changes prefixes in reusable code from cdns3 to common cdns.
> The patch 5: adopts gadget_dev pointer in cdns structure to make possible
> use it in both drivers.
> The patches 6-8: add the main part of driver and has been intentionally
> split into 3 part. In my opinion such division should not
> affect understanding and reviewing the driver, and cause that
> main patch (7/8) is little smaller. Patch 6 introduces main
> header file for driver, 7 is the main part that implements all
> functionality of driver and 8 introduces tracepoints.
> The patch 9: Adds cdns3 prefixes to files related with USBSS driver.
> the patch 10: Adds USBSSP DRD IP driver entry to MAINTAINERS file.
Pawel, there are several issues are reported for this patch set after I add
it to my -next tree, and the merge windows is near. I drop this set for
v5.11-rc1, but keep it at my -next tree.
Peter
>
> Changlog from v4:
> - fixed bug in cdns3_plat_runtime_resume as suggested by Peter Chen
> - fixed bug in cdns3_plat_suspend as suggested by Peter Chen
> - some typos have been removed
>
> Changlog from v3:
> - added 'T' to MAINTAINERS file for CDNSP entry
> - updated common code with latest cdns3 fixes
>
> Changlog from v2:
> - removed not used pdev parameter from cdnsp_read/wite_64 functions
> - fixed incorrect value assigned to CDNSP_ENDPOINTS_NUM (32 -> 31)
> - replaced some constant value with CDNSP_ENDPOINTS_NUM macro
> - replaced 'true' with '1' in bits description in cdnsp-gadget.h file
> - fixed some typos
> - some other less important changes suggested by Peter Chen
>
> Changlog from v1:
> - updated common code to latest cdns3 driver
> - moved cdnsp driver files to cdns3 as suggested by Peter Chen
> - removed duplicate code from cdnsp_ep0_set_config function
> - added cdns3 prefixes to file related with USBSS driver
> - updated MAINTAINERS file
> - fixed issue with U1
> - fixed issue with L1
> - some less improtant changes suggested by Chunfeng Yun
> ---
>
> Pawel Laszczak (10):
> usb: cdns3: Add support for DRD CDNSP
> usb: cdns3: Split core.c into cdns3-plat and core.c file
> usb: cdns3: Moves reusable code to separate module
> usb: cdns3: Refactoring names in reusable code
> usb: cdns3: Changed type of gadget_dev in cdns structure
> usb: cdnsp: Device side header file for CDNSP driver
> usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver
> usb: cdnsp: Add tracepoints for CDNSP driver
> usb: cdns3: Change file names for cdns3 driver.
> MAINTAINERS: add Cadence USBSSP DRD IP driver entry
>
> MAINTAINERS |9 +
> drivers/usb/Makefile |2 +
> drivers/usb/cdns3/Kconfig | 61 +-
> drivers/usb/cdns3/Makefile| 30 +-
> drivers/usb/cdns3/{debug.h => cdns3-debug.h} |0
> drivers/usb/cdns3/{ep0.c => cdns3-ep0.c} |4 +-
> .../usb/cdns3/{gadget.c => cdns3-gadget.c}| 28 +-
> .../usb/cdns3/{gadget.h => cdns3-gadget.h}|0
> drivers/usb/cdns3/cdns3-imx.c |2 +-
> drivers/usb/cdns3/cdns3-plat.c| 315 +++
> drivers/usb/cdns3/{trace.c => cdns3-trace.c} |2 +-
> drivers/usb/cdns3/{trace.h => cdns3-trace.h} |6 +-
> drivers/usb/cdns3/cdnsp-debug.h | 583
&
Re: [PATCH] PCI: Remove pci_try_set_mwi
On 20-12-09 09:31:21, Heiner Kallweit wrote: > drivers/usb/chipidea/ci_hdrc_pci.c| 2 +- For chipidea changes: Acked-by: Peter Chen Peter > drivers/usb/gadget/udc/amd5536udc_pci.c | 2 +- > drivers/usb/gadget/udc/net2280.c | 2 +- > drivers/usb/gadget/udc/pch_udc.c | 2 +- > include/linux/pci.h | 5 ++--- > 27 files changed, 33 insertions(+), 60 deletions(-) > > diff --git a/Documentation/PCI/pci.rst b/Documentation/PCI/pci.rst > index 814b40f83..120362cc9 100644 > --- a/Documentation/PCI/pci.rst > +++ b/Documentation/PCI/pci.rst > @@ -226,10 +226,7 @@ If the PCI device can use the PCI > Memory-Write-Invalidate transaction, > call pci_set_mwi(). This enables the PCI_COMMAND bit for Mem-Wr-Inval > and also ensures that the cache line size register is set correctly. > Check the return value of pci_set_mwi() as not all architectures > -or chip-sets may support Memory-Write-Invalidate. Alternatively, > -if Mem-Wr-Inval would be nice to have but is not required, call > -pci_try_set_mwi() to have the system do its best effort at enabling > -Mem-Wr-Inval. > +or chip-sets may support Memory-Write-Invalidate. > > > Request MMIO/IOP resources > diff --git a/drivers/ata/pata_cs5530.c b/drivers/ata/pata_cs5530.c > index ad75d02b6..8654b3ae1 100644 > --- a/drivers/ata/pata_cs5530.c > +++ b/drivers/ata/pata_cs5530.c > @@ -214,7 +214,7 @@ static int cs5530_init_chip(void) > } > > pci_set_master(cs5530_0); > - pci_try_set_mwi(cs5530_0); > + pci_set_mwi(cs5530_0); > > /* >* Set PCI CacheLineSize to 16-bytes: > diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c > index 664ef658a..ee37755ea 100644 > --- a/drivers/ata/sata_mv.c > +++ b/drivers/ata/sata_mv.c > @@ -4432,7 +4432,7 @@ static int mv_pci_init_one(struct pci_dev *pdev, > mv_print_info(host); > > pci_set_master(pdev); > - pci_try_set_mwi(pdev); > + pci_set_mwi(pdev); > return ata_host_activate(host, pdev->irq, mv_interrupt, IRQF_SHARED, >IS_GEN_I(hpriv) ? _sht : _sht); > } > diff --git a/drivers/dma/dw/pci.c b/drivers/dma/dw/pci.c > index 1142aa6f8..1c20b7485 100644 > --- a/drivers/dma/dw/pci.c > +++ b/drivers/dma/dw/pci.c > @@ -30,7 +30,7 @@ static int dw_pci_probe(struct pci_dev *pdev, const struct > pci_device_id *pid) > } > > pci_set_master(pdev); > - pci_try_set_mwi(pdev); > + pci_set_mwi(pdev); > > ret = pci_set_dma_mask(pdev, DMA_BIT_MASK(32)); > if (ret) > diff --git a/drivers/dma/hsu/pci.c b/drivers/dma/hsu/pci.c > index 07cc7320a..420dd3706 100644 > --- a/drivers/dma/hsu/pci.c > +++ b/drivers/dma/hsu/pci.c > @@ -73,7 +73,7 @@ static int hsu_pci_probe(struct pci_dev *pdev, const struct > pci_device_id *id) > } > > pci_set_master(pdev); > - pci_try_set_mwi(pdev); > + pci_set_mwi(pdev); > > ret = pci_set_dma_mask(pdev, DMA_BIT_MASK(32)); > if (ret) > diff --git a/drivers/ide/cs5530.c b/drivers/ide/cs5530.c > index 5bb46e713..5d2c421ab 100644 > --- a/drivers/ide/cs5530.c > +++ b/drivers/ide/cs5530.c > @@ -168,7 +168,7 @@ static int init_chipset_cs5530(struct pci_dev *dev) >*/ > > pci_set_master(cs5530_0); > - pci_try_set_mwi(cs5530_0); > + pci_set_mwi(cs5530_0); > > /* >* Set PCI CacheLineSize to 16-bytes: > diff --git a/drivers/mfd/intel-lpss-pci.c b/drivers/mfd/intel-lpss-pci.c > index 2d7c588ef..a0c3be750 100644 > --- a/drivers/mfd/intel-lpss-pci.c > +++ b/drivers/mfd/intel-lpss-pci.c > @@ -39,7 +39,7 @@ static int intel_lpss_pci_probe(struct pci_dev *pdev, > > /* Probably it is enough to set this for iDMA capable devices only */ > pci_set_master(pdev); > - pci_try_set_mwi(pdev); > + pci_set_mwi(pdev); > > ret = intel_lpss_probe(>dev, info); > if (ret) > diff --git a/drivers/net/ethernet/adaptec/starfire.c > b/drivers/net/ethernet/adaptec/starfire.c > index 555299737..1dbff34c4 100644 > --- a/drivers/net/ethernet/adaptec/starfire.c > +++ b/drivers/net/ethernet/adaptec/starfire.c > @@ -679,7 +679,7 @@ static int starfire_init_one(struct pci_dev *pdev, > pci_set_master(pdev); > > /* enable MWI -- it vastly improves Rx performance on sparc64 */ > - pci_try_set_mwi(pdev); > + pci_set_mwi(pdev); > > #ifdef ZEROCOPY > /* Starfire can do TCP/UDP checksumming */ > diff --git a/drivers/net/ethernet/alacritech/slicoss.c > b/drivers/net/ethernet/alacritech/slicoss.c > index 696517eae..544510f57 100644 > --- a/drivers/net/e
RE: [PATCH -next] usb: cdns3: fix warning when USB_CDNS_HOST is not set
> In file included from ../drivers/usb/cdns3/core.c:23:0: > ../drivers/usb/cdns3/host-export.h:27:51: warning: ‘struct usb_hcd’ declared > inside parameter list will not be visible outside of this definition or > declaration > static inline int xhci_cdns3_suspend_quirk(struct usb_hcd *hcd) >^~~ > Applied both, thanks. Peter > Signed-off-by: Randy Dunlap > Cc: Peter Chen > Cc: Pawel Laszczak > Cc: Roger Quadros > Cc: linux-...@vger.kernel.org > Cc: Greg Kroah-Hartman > --- > drivers/usb/cdns3/host-export.h |4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > --- linux-next-20201208.orig/drivers/usb/cdns3/host-export.h > +++ linux-next-20201208/drivers/usb/cdns3/host-export.h > @@ -9,10 +9,10 @@ > #ifndef __LINUX_CDNS3_HOST_EXPORT > #define __LINUX_CDNS3_HOST_EXPORT > > -#if IS_ENABLED(CONFIG_USB_CDNS_HOST) > - > struct usb_hcd; > > +#if IS_ENABLED(CONFIG_USB_CDNS_HOST) > + > int cdns_host_init(struct cdns *cdns); > int xhci_cdns3_suspend_quirk(struct usb_hcd *hcd); >
Re: [PATCH -next] usb: cdnsp: Mark cdnsp_gadget_ops with static keyword
On 20-12-08 20:30:49, Zou Wei wrote:
> Fix the following sparse warning:
>
> drivers/usb/cdns3/cdnsp-gadget.c:1546:29: warning: symbol 'cdnsp_gadget_ops'
> was not declared. Should it be static?
>
> Signed-off-by: Zou Wei
> ---
> drivers/usb/cdns3/cdnsp-gadget.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/cdns3/cdnsp-gadget.c
> b/drivers/usb/cdns3/cdnsp-gadget.c
> index 9716273..c7c5d0a 100644
> --- a/drivers/usb/cdns3/cdnsp-gadget.c
> +++ b/drivers/usb/cdns3/cdnsp-gadget.c
> @@ -1543,7 +1543,7 @@ static int cdnsp_gadget_pullup(struct usb_gadget
> *gadget, int is_on)
> return 0;
> }
>
> -const struct usb_gadget_ops cdnsp_gadget_ops = {
> +static const struct usb_gadget_ops cdnsp_gadget_ops = {
> .get_frame = cdnsp_gadget_get_frame,
> .wakeup = cdnsp_gadget_wakeup,
> .set_selfpowered = cdnsp_gadget_set_selfpowered,
> --
Applied, thanks.
--
Thanks,
Peter Chen
Re: [PATCH] drivers: usb: gadget: prefer pr_*() functions over raw printk()
- printk(KERN_INFO "FUSB300_IGR1_U1_ENTRY_INT\n");
> + pr_info("FUSB300_IGR1_U1_ENTRY_INT\n");
> }
>
> if (int_grp1 & FUSB300_IGR1_RESM_INT) {
> fusb300_clear_int(fusb300, FUSB300_OFFSET_IGR1,
> FUSB300_IGR1_RESM_INT);
> - printk(KERN_INFO "fusb300_resume\n");
> + pr_info("fusb300_resume\n");
> }
>
> if (int_grp1 & FUSB300_IGR1_SUSP_INT) {
> fusb300_clear_int(fusb300, FUSB300_OFFSET_IGR1,
> FUSB300_IGR1_SUSP_INT);
> - printk(KERN_INFO "fusb300_suspend\n");
> + pr_info("fusb300_suspend\n");
> }
>
> if (int_grp1 & FUSB300_IGR1_HS_LPM_INT) {
> fusb300_clear_int(fusb300, FUSB300_OFFSET_IGR1,
> FUSB300_IGR1_HS_LPM_INT);
> - printk(KERN_INFO "fusb300_HS_LPM_INT\n");
> + pr_info("fusb300_HS_LPM_INT\n");
> }
>
> if (int_grp1 & FUSB300_IGR1_DEV_MODE_CHG_INT) {
> @@ -1195,11 +1195,11 @@ static irqreturn_t fusb300_irq(int irq, void
> *_fusb300)
>
> if (int_grp1 & FUSB300_IGR1_CX_COMFAIL_INT) {
> fusb300_set_cxstall(fusb300);
> - printk(KERN_INFO "fusb300_ep0fail\n");
> + pr_info("fusb300_ep0fail\n");
> }
>
> if (int_grp1 & FUSB300_IGR1_CX_SETUP_INT) {
> - printk(KERN_INFO "fusb300_ep0setup\n");
> + pr_info("fusb300_ep0setup\n");
> if (setup_packet(fusb300, )) {
> spin_unlock(>lock);
> if (fusb300->driver->setup(>gadget, ) < 0)
> @@ -1209,16 +1209,16 @@ static irqreturn_t fusb300_irq(int irq, void
> *_fusb300)
> }
>
> if (int_grp1 & FUSB300_IGR1_CX_CMDEND_INT)
> - printk(KERN_INFO "fusb300_cmdend\n");
> + pr_info("fusb300_cmdend\n");
>
>
> if (int_grp1 & FUSB300_IGR1_CX_OUT_INT) {
> - printk(KERN_INFO "fusb300_cxout\n");
> + pr_info("fusb300_cxout\n");
> fusb300_ep0out(fusb300);
> }
>
> if (int_grp1 & FUSB300_IGR1_CX_IN_INT) {
> - printk(KERN_INFO "fusb300_cxin\n");
> + pr_info("fusb300_cxin\n");
> fusb300_ep0in(fusb300);
> }
>
> diff --git a/drivers/usb/gadget/udc/goku_udc.c
> b/drivers/usb/gadget/udc/goku_udc.c
> index 3e1267d38774..4f225552861a 100644
> --- a/drivers/usb/gadget/udc/goku_udc.c
> +++ b/drivers/usb/gadget/udc/goku_udc.c
> @@ -1748,7 +1748,7 @@ static int goku_probe(struct pci_dev *pdev, const
> struct pci_device_id *id)
> int retval;
>
> if (!pdev->irq) {
> - printk(KERN_ERR "Check PCI %s IRQ setup!\n", pci_name(pdev));
> + pr_err("Check PCI %s IRQ setup!\n", pci_name(pdev));
> retval = -ENODEV;
> goto err;
> }
> diff --git a/drivers/usb/gadget/udc/r8a66597-udc.h
> b/drivers/usb/gadget/udc/r8a66597-udc.h
> index 9a115caba661..fa4d62c32ea1 100644
> --- a/drivers/usb/gadget/udc/r8a66597-udc.h
> +++ b/drivers/usb/gadget/udc/r8a66597-udc.h
> @@ -247,7 +247,7 @@ static inline u16 get_xtal_from_pdata(struct
> r8a66597_platdata *pdata)
> clock = XTAL48;
> break;
> default:
> - printk(KERN_ERR "r8a66597: platdata clock is wrong.\n");
> + pr_err("r8a66597: platdata clock is wrong.\n");
> break;
> }
>
> --
> 2.11.0
>
--
Thanks,
Peter Chen
RE: [RESEND PATCH] MAINTAINERS: Add myself as a reviewer for CADENCE USB3 DRD IP DRIVER
> + Peter, Pawel and Roger for their acks. Applied, thanks. Peter > > On 08/12/20 10:38 AM, Aswath Govindraju wrote: > > I would like to help in reviewing CADENCE USB3 DRD IP DRIVER patches > > > > Signed-off-by: Aswath Govindraju > > --- > > > > Resending the patch to add more viewers. > > > > MAINTAINERS | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/MAINTAINERS b/MAINTAINERS index > > 6aac0f845f34..ff9bd7d18d94 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -3861,6 +3861,7 @@ CADENCE USB3 DRD IP DRIVER > > M: Peter Chen > > M: Pawel Laszczak > > M: Roger Quadros > > +R: Aswath Govindraju > > L: linux-...@vger.kernel.org > > S: Maintained > > T: git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git > > -- > > 2.17.1 > >
RE: [PATCH v5 00/10] Introduced new Cadence USBSSP DRD Driver.
>
> The Cadence USBSS DRD Controller is a highly configurable IP Core which can
> be instantiated as Dual-Role Device (DRD), Peripheral Only and Host Only
> (XHCI)configurations.
>
> The current driver has been validated with FPGA burned. We have support for
> PCIe bus, which is used on FPGA prototyping.
>
> The host side of USBSS-DRD controller is compliance with XHCI specification,
> so
> it works with standard XHCI Linux driver.
>
> The device side of USBSS DRD controller is compliant with XHCI.
> The architecture for device side is almost the same as for host side, and most
> of the XHCI specification can be used to understand how this controller
> operates.
>
> This controller and driver support Full Speed, Hight Speed, Supper Speed and
> Supper Speed Plus USB protocol.
>
> The prefix cdnsp used in driver has chosen by analogy to cdn3 driver.
> The last letter of this acronym means PLUS. The formal name of controller is
> USBSSP but it's to generic so I've decided to use CDNSP.
>
> The patch 1: adds support for DRD CDNSP.
> The patch 2: separates common code that can be reusable by cdnsp driver.
> The patch 3: moves reusable code to separate module.
> The patch 4: changes prefixes in reusable code from cdns3 to common cdns.
> The patch 5: adopts gadget_dev pointer in cdns structure to make possible
> use it in both drivers.
> The patches 6-8: add the main part of driver and has been intentionally
> split into 3 part. In my opinion such division should not
> affect understanding and reviewing the driver, and cause that
> main patch (7/8) is little smaller. Patch 6 introduces main
> header file for driver, 7 is the main part that implements all
> functionality of driver and 8 introduces tracepoints.
> The patch 9: Adds cdns3 prefixes to files related with USBSS driver.
> the patch 10: Adds USBSSP DRD IP driver entry to MAINTAINERS file.
>
Applied, thanks.
Peter
> Changlog from v4:
> - fixed bug in cdns3_plat_runtime_resume as suggested by Peter Chen
> - fixed bug in cdns3_plat_suspend as suggested by Peter Chen
> - some typos have been removed
>
> Changlog from v3:
> - added 'T' to MAINTAINERS file for CDNSP entry
> - updated common code with latest cdns3 fixes
>
> Changlog from v2:
> - removed not used pdev parameter from cdnsp_read/wite_64 functions
> - fixed incorrect value assigned to CDNSP_ENDPOINTS_NUM (32 -> 31)
> - replaced some constant value with CDNSP_ENDPOINTS_NUM macro
> - replaced 'true' with '1' in bits description in cdnsp-gadget.h file
> - fixed some typos
> - some other less important changes suggested by Peter Chen
>
> Changlog from v1:
> - updated common code to latest cdns3 driver
> - moved cdnsp driver files to cdns3 as suggested by Peter Chen
> - removed duplicate code from cdnsp_ep0_set_config function
> - added cdns3 prefixes to file related with USBSS driver
> - updated MAINTAINERS file
> - fixed issue with U1
> - fixed issue with L1
> - some less improtant changes suggested by Chunfeng Yun
> ---
>
> Pawel Laszczak (10):
> usb: cdns3: Add support for DRD CDNSP
> usb: cdns3: Split core.c into cdns3-plat and core.c file
> usb: cdns3: Moves reusable code to separate module
> usb: cdns3: Refactoring names in reusable code
> usb: cdns3: Changed type of gadget_dev in cdns structure
> usb: cdnsp: Device side header file for CDNSP driver
> usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver
> usb: cdnsp: Add tracepoints for CDNSP driver
> usb: cdns3: Change file names for cdns3 driver.
> MAINTAINERS: add Cadence USBSSP DRD IP driver entry
>
> MAINTAINERS |9 +
> drivers/usb/Makefile |2 +
> drivers/usb/cdns3/Kconfig | 61 +-
> drivers/usb/cdns3/Makefile| 30 +-
> drivers/usb/cdns3/{debug.h => cdns3-debug.h} |0
> drivers/usb/cdns3/{ep0.c => cdns3-ep0.c} |4 +-
> .../usb/cdns3/{gadget.c => cdns3-gadget.c}| 28 +-
> .../usb/cdns3/{gadget.h => cdns3-gadget.h}|0
> drivers/usb/cdns3/cdns3-imx.c |2 +-
> drivers/usb/cdns3/cdns3-plat.c| 315 +++
> drivers/usb/cdns3/{trace.c => cdns3-trace.c} |2 +-
> drivers/usb/cdns3/{trace.h => cdns3-trace.h} |6 +-
> drivers/usb/cdns3/cdnsp-debug.h | 583
> drivers/usb/cdns3/cdnsp-ep0.c | 495
> drivers/usb/cdns3/cdnsp-gadget.c | 2017 ++
> drivers/usb/cdns3/cdnsp-gadget.h | 1600 +++
> drivers/usb/cdns3/cdnsp-mem.c | 1325 +
&g
RE: [PATCH 04/15] usb: misc: ehset: update to use the usb_control_msg_{send|recv}() API
>
> The newer usb_control_msg_{send|recv}() API are an improvement on the
> existing usb_control_msg() as it ensures that a short read/write is treated as
> an error, data can be used off the stack, and raw usb pipes need not be
> created
> in the calling functions.
> For this reason, instances of usb_control_msg() have been replaced with
> usb_control_msg_{recv|send}() appropriately.
>
> Signed-off-by: Anant Thazhemadam
Reviewed-by: Peter Chen
Peter
> ---
> drivers/usb/misc/ehset.c | 70 ++--
> 1 file changed, 31 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/usb/misc/ehset.c b/drivers/usb/misc/ehset.c index
> 2752e1f4f4d0..068f4fae2965 100644
> --- a/drivers/usb/misc/ehset.c
> +++ b/drivers/usb/misc/ehset.c
> @@ -30,48 +30,42 @@ static int ehset_probe(struct usb_interface *intf,
>
> switch (test_pid) {
> case TEST_SE0_NAK_PID:
> - ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
> - USB_REQ_SET_FEATURE, USB_RT_PORT,
> - USB_PORT_FEAT_TEST,
> - (USB_TEST_SE0_NAK << 8) | portnum,
> - NULL, 0, 1000);
> + ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
> +USB_RT_PORT, USB_PORT_FEAT_TEST,
> +(USB_TEST_SE0_NAK << 8) | portnum,
> +NULL, 0, 1000, GFP_KERNEL);
> break;
> case TEST_J_PID:
> - ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
> - USB_REQ_SET_FEATURE, USB_RT_PORT,
> - USB_PORT_FEAT_TEST,
> - (USB_TEST_J << 8) | portnum,
> - NULL, 0, 1000);
> + ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
> +USB_RT_PORT, USB_PORT_FEAT_TEST,
> +(USB_TEST_J << 8) | portnum, NULL, 0,
> +1000, GFP_KERNEL);
> break;
> case TEST_K_PID:
> - ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
> - USB_REQ_SET_FEATURE, USB_RT_PORT,
> - USB_PORT_FEAT_TEST,
> - (USB_TEST_K << 8) | portnum,
> - NULL, 0, 1000);
> + ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
> +USB_RT_PORT, USB_PORT_FEAT_TEST,
> +(USB_TEST_K << 8) | portnum, NULL, 0,
> +1000, GFP_KERNEL);
> break;
> case TEST_PACKET_PID:
> - ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
> - USB_REQ_SET_FEATURE, USB_RT_PORT,
> - USB_PORT_FEAT_TEST,
> - (USB_TEST_PACKET << 8) | portnum,
> - NULL, 0, 1000);
> + ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
> +USB_RT_PORT, USB_PORT_FEAT_TEST,
> +(USB_TEST_PACKET << 8) | portnum,
> +NULL, 0, 1000, GFP_KERNEL);
> break;
> case TEST_HS_HOST_PORT_SUSPEND_RESUME:
> /* Test: wait for 15secs -> suspend -> 15secs delay -> resume */
> msleep(15 * 1000);
> - ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
> - USB_REQ_SET_FEATURE, USB_RT_PORT,
> - USB_PORT_FEAT_SUSPEND, portnum,
> - NULL, 0, 1000);
> + ret = usb_control_msg_send(hub_udev, 0, USB_REQ_SET_FEATURE,
> +USB_RT_PORT, USB_PORT_FEAT_SUSPEND,
> +portnum, NULL, 0, 1000, GFP_KERNEL);
> if (ret < 0)
> break;
>
> msleep(15 * 1000);
> - ret = usb_control_msg(hub_udev, usb_sndctrlpipe(hub_udev, 0),
> - USB_REQ_CLEAR_FEATURE, USB_RT_PORT,
> - USB_PORT_FEAT_SUSPEND, portnum,
> -
Re: [PATCH v4 00/10] Introduced new Cadence USBSSP DRD Driver.
On 20-12-05 11:12:44, Aswath Govindraju wrote: > Hi, > On 04/12/20 6:49 am, Peter Chen wrote: > > > >> This patch introduce new Cadence USBSS DRD driver to linux kernel. > >> > >> The Cadence USBSS DRD Controller is a highly configurable IP Core which can > >> be instantiated as Dual-Role Device (DRD), Peripheral Only and Host Only > >> (XHCI)configurations. > >> > >> The current driver has been validated with FPGA burned. We have support for > >> PCIe bus, which is used on FPGA prototyping. > >> > >> The host side of USBSS-DRD controller is compliance with XHCI > >> specification, so > >> it works with standard XHCI Linux driver. > >> > >> The device side of USBSS DRD controller is compliant with XHCI. > >> The architecture for device side is almost the same as for host side, and > >> most > >> of the XHCI specification can be used to understand how this controller > >> operates. > >> > >> This controller and driver support Full Speed, Hight Speed, Supper Speed > >> and > >> Supper Speed Plus USB protocol. > >> > >> The prefix cdnsp used in driver has chosen by analogy to cdn3 driver. > >> The last letter of this acronym means PLUS. The formal name of controller > >> is > >> USBSSP but it's to generic so I've decided to use CDNSP. > >> > >> The patch 1: adds support for DRD CDNSP. > >> The patch 2: separates common code that can be reusable by cdnsp driver. > >> The patch 3: moves reusable code to separate module. > >> The patch 4: changes prefixes in reusable code from cdns3 to common cdns. > >> The patch 5: adopts gadget_dev pointer in cdns structure to make possible > >> use it in both drivers. > >> The patches 6-8: add the main part of driver and has been intentionally > >> split into 3 part. In my opinion such division should not > >> affect understanding and reviewing the driver, and cause that > >> main patch (7/8) is little smaller. Patch 6 introduces main > >> header file for driver, 7 is the main part that implements all > >> functionality of driver and 8 introduces tracepoints. > >> The patch 9: Adds cdns3 prefixes to files related with USBSS driver. > >> the patch 10: Adds USBSSP DRD IP driver entry to MAINTAINERS file. > >> > > > > Hi Roger & Sekhar, > > > > Would you please test this patch set and see if it works well at TI > > platforms? > > > > Tested this patch series on TI SOC J7200. All given below tests were > performed and they passed, > > Host mode: > - Connected a mass storage device (USB flash stick) and performed read >and write tests > - Connected mouse and keyboard to check enumeration > > Device mode: > - Tested g_mass_storage module by performing read and write > - Tested g_ether module by the pinging host and device from either sides > > OTG: > - Switching between host and device mode based on the device connected. > > > Tested-by: Aswath Govindraju Thanks Aswath. Basic functions at NXP also work. Pawel, you could post your v5 with my last comments. Peter > > Thanks, > Aswath > > > Peter > > > >> Changlog from v3: > >> - added 'T' to MAINTAINERS file for CDNSP entry > >> - updated common code with latest cdns3 fixes > >> > >> Changlog from v2: > >> - removed not used pdev parameter from cdnsp_read/wite_64 functions > >> - fixed incorrect value assigned to CDNSP_ENDPOINTS_NUM (32 -> 31) > >> - replaced some constant value with CDNSP_ENDPOINTS_NUM macro > >> - replaced 'true' with '1' in bits description in cdnsp-gadget.h file > >> - fixed some typos > >> - some other less important changes suggested by Peter Chen > >> > >> Changlog from v1: > >> - updated common code to latest cdns3 driver > >> - moved cdnsp driver files to cdns3 as sugested by Peter Chan > >> - removed duplicate code from cdnsp_ep0_set_config function > >> - added cdns3 prefixes to file related with USBSS driver > >> - updated MAINTAINERS file > >> - fixed issue with U1 > >> - fixed issue with L1 > >> - some less improtant changes sugested by Chunfeng Yun > >> --- > >> > >> Pawel Laszczak (10): > >> usb: cdns3: Add support for DRD CDNSP > >> usb: cdns3: Split core.c into cdns3-plat and core.c file > >> usb: cdns3: Moves reusable code to sep
Re: [PATCH v4 00/10] Introduced new Cadence USBSSP DRD Driver.
On 20-12-02 14:25:38, Pawel Laszczak wrote:
> This patch introduce new Cadence USBSS DRD driver to linux kernel.
>
> The Cadence USBSS DRD Controller is a highly configurable IP Core which
> can be instantiated as Dual-Role Device (DRD), Peripheral Only and
> Host Only (XHCI)configurations.
>
> The current driver has been validated with FPGA burned. We have support
> for PCIe bus, which is used on FPGA prototyping.
>
> The host side of USBSS-DRD controller is compliance with XHCI
> specification, so it works with standard XHCI Linux driver.
>
> The device side of USBSS DRD controller is compliant with XHCI.
> The architecture for device side is almost the same as for host side,
> and most of the XHCI specification can be used to understand how
> this controller operates.
>
> This controller and driver support Full Speed, Hight Speed, Supper Speed
> and Supper Speed Plus USB protocol.
>
> The prefix cdnsp used in driver has chosen by analogy to cdn3 driver.
> The last letter of this acronym means PLUS. The formal name of controller
> is USBSSP but it's to generic so I've decided to use CDNSP.
>
> The patch 1: adds support for DRD CDNSP.
> The patch 2: separates common code that can be reusable by cdnsp driver.
> The patch 3: moves reusable code to separate module.
> The patch 4: changes prefixes in reusable code from cdns3 to common cdns.
> The patch 5: adopts gadget_dev pointer in cdns structure to make possible
> use it in both drivers.
> The patches 6-8: add the main part of driver and has been intentionally
> split into 3 part. In my opinion such division should not
> affect understanding and reviewing the driver, and cause that
> main patch (7/8) is little smaller. Patch 6 introduces main
> header file for driver, 7 is the main part that implements all
> functionality of driver and 8 introduces tracepoints.
> The patch 9: Adds cdns3 prefixes to files related with USBSS driver.
> the patch 10: Adds USBSSP DRD IP driver entry to MAINTAINERS file.
Hi Pawel,
You may need to fix below:
diff --git a/drivers/usb/cdns3/cdns3-plat.c b/drivers/usb/cdns3/cdns3-plat.c
index 04bccf6daaba..30d69b639492 100644
--- a/drivers/usb/cdns3/cdns3-plat.c
+++ b/drivers/usb/cdns3/cdns3-plat.c
@@ -4,7 +4,7 @@
*
* Copyright (C) 2018-2020 Cadence.
* Copyright (C) 2017-2018 NXP
- * Copyright (C) 2019 Texas Instrumentsq
+ * Copyright (C) 2019 Texas Instruments
*
*
* Author: Peter Chen
static int cdns3_plat_runtime_resume(struct device *dev)
{
- return cdns3_controller_resume(dev, PMSG_SUSPEND);
+ return cdns3_controller_resume(dev, PMSG_AUTO_RESUME);
}
@@ -273,7 +273,14 @@ static int cdns3_plat_suspend(struct device *dev)
cdns_suspend(cdns);
- return cdns3_controller_suspend(dev, PMSG_AUTO_SUSPEND);
+ return cdns3_controller_suspend(dev, PMSG_SUSPEND);
I am porting and testing your patches at NXP platforms.
Peter
>
> Changlog from v3:
> - added 'T' to MAINTAINERS file for CDNSP entry
> - updated common code with latest cdns3 fixes
>
> Changlog from v2:
> - removed not used pdev parameter from cdnsp_read/wite_64 functions
> - fixed incorrect value assigned to CDNSP_ENDPOINTS_NUM (32 -> 31)
> - replaced some constant value with CDNSP_ENDPOINTS_NUM macro
> - replaced 'true' with '1' in bits description in cdnsp-gadget.h file
> - fixed some typos
> - some other less important changes suggested by Peter Chen
>
> Changlog from v1:
> - updated common code to latest cdns3 driver
> - moved cdnsp driver files to cdns3 as sugested by Peter Chan
> - removed duplicate code from cdnsp_ep0_set_config function
> - added cdns3 prefixes to file related with USBSS driver
> - updated MAINTAINERS file
> - fixed issue with U1
> - fixed issue with L1
> - some less improtant changes sugested by Chunfeng Yun
> ---
>
> Pawel Laszczak (10):
> usb: cdns3: Add support for DRD CDNSP
> usb: cdns3: Split core.c into cdns3-plat and core.c file
> usb: cdns3: Moves reusable code to separate module
> usb: cdns3: Refactoring names in reusable code
> usb: cdns3: Changed type of gadget_dev in cdns structure
> usb: cdnsp: Device side header file for CDNSP driver
> usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver
> usb: cdnsp: Add tracepoints for CDNSP driver
> usb: cdns3: Change file names for cdns3 driver.
> MAINTAINERS: add Cadence USBSSP DRD IP driver entry
>
> MAINTAINERS |9 +
> drivers/usb/Makefile |2 +
> drivers/usb/cdns3/Kconfig | 61 +-
> drivers/usb/cdns3/Makefile| 30 +-
> drivers/usb/cdns3/{debug.h => cdns3-debug.h} |0
>
RE: [PATCH v4 00/10] Introduced new Cadence USBSSP DRD Driver.
> This patch introduce new Cadence USBSS DRD driver to linux kernel.
>
> The Cadence USBSS DRD Controller is a highly configurable IP Core which can
> be instantiated as Dual-Role Device (DRD), Peripheral Only and Host Only
> (XHCI)configurations.
>
> The current driver has been validated with FPGA burned. We have support for
> PCIe bus, which is used on FPGA prototyping.
>
> The host side of USBSS-DRD controller is compliance with XHCI specification,
> so
> it works with standard XHCI Linux driver.
>
> The device side of USBSS DRD controller is compliant with XHCI.
> The architecture for device side is almost the same as for host side, and most
> of the XHCI specification can be used to understand how this controller
> operates.
>
> This controller and driver support Full Speed, Hight Speed, Supper Speed and
> Supper Speed Plus USB protocol.
>
> The prefix cdnsp used in driver has chosen by analogy to cdn3 driver.
> The last letter of this acronym means PLUS. The formal name of controller is
> USBSSP but it's to generic so I've decided to use CDNSP.
>
> The patch 1: adds support for DRD CDNSP.
> The patch 2: separates common code that can be reusable by cdnsp driver.
> The patch 3: moves reusable code to separate module.
> The patch 4: changes prefixes in reusable code from cdns3 to common cdns.
> The patch 5: adopts gadget_dev pointer in cdns structure to make possible
> use it in both drivers.
> The patches 6-8: add the main part of driver and has been intentionally
> split into 3 part. In my opinion such division should not
> affect understanding and reviewing the driver, and cause that
> main patch (7/8) is little smaller. Patch 6 introduces main
> header file for driver, 7 is the main part that implements all
> functionality of driver and 8 introduces tracepoints.
> The patch 9: Adds cdns3 prefixes to files related with USBSS driver.
> the patch 10: Adds USBSSP DRD IP driver entry to MAINTAINERS file.
>
Hi Roger & Sekhar,
Would you please test this patch set and see if it works well at TI platforms?
Peter
> Changlog from v3:
> - added 'T' to MAINTAINERS file for CDNSP entry
> - updated common code with latest cdns3 fixes
>
> Changlog from v2:
> - removed not used pdev parameter from cdnsp_read/wite_64 functions
> - fixed incorrect value assigned to CDNSP_ENDPOINTS_NUM (32 -> 31)
> - replaced some constant value with CDNSP_ENDPOINTS_NUM macro
> - replaced 'true' with '1' in bits description in cdnsp-gadget.h file
> - fixed some typos
> - some other less important changes suggested by Peter Chen
>
> Changlog from v1:
> - updated common code to latest cdns3 driver
> - moved cdnsp driver files to cdns3 as sugested by Peter Chan
> - removed duplicate code from cdnsp_ep0_set_config function
> - added cdns3 prefixes to file related with USBSS driver
> - updated MAINTAINERS file
> - fixed issue with U1
> - fixed issue with L1
> - some less improtant changes sugested by Chunfeng Yun
> ---
>
> Pawel Laszczak (10):
> usb: cdns3: Add support for DRD CDNSP
> usb: cdns3: Split core.c into cdns3-plat and core.c file
> usb: cdns3: Moves reusable code to separate module
> usb: cdns3: Refactoring names in reusable code
> usb: cdns3: Changed type of gadget_dev in cdns structure
> usb: cdnsp: Device side header file for CDNSP driver
> usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver
> usb: cdnsp: Add tracepoints for CDNSP driver
> usb: cdns3: Change file names for cdns3 driver.
> MAINTAINERS: add Cadence USBSSP DRD IP driver entry
>
> MAINTAINERS |9 +
> drivers/usb/Makefile |2 +
> drivers/usb/cdns3/Kconfig | 61 +-
> drivers/usb/cdns3/Makefile| 30 +-
> drivers/usb/cdns3/{debug.h => cdns3-debug.h} |0
> drivers/usb/cdns3/{ep0.c => cdns3-ep0.c} |4 +-
> .../usb/cdns3/{gadget.c => cdns3-gadget.c}| 28 +-
> .../usb/cdns3/{gadget.h => cdns3-gadget.h}|0
> drivers/usb/cdns3/cdns3-imx.c |2 +-
> drivers/usb/cdns3/cdns3-plat.c| 315 +++
> drivers/usb/cdns3/{trace.c => cdns3-trace.c} |2 +-
> drivers/usb/cdns3/{trace.h => cdns3-trace.h} |6 +-
> drivers/usb/cdns3/cdnsp-debug.h | 583
> drivers/usb/cdns3/cdnsp-ep0.c | 495
> drivers/usb/cdns3/cdnsp-gadget.c | 2017 ++
> drivers/usb/cdns3/cdnsp-gadget.h | 1600 +++
> drivers/usb/cdns3/cdnsp-mem.c | 1325 +
> drivers/usb/cdns3/cdnsp-pci.c
RE: linux-next: Signed-off-by missing for commit in the usb-chipidea-next tree
>
> Hi all,
>
> Commits
>
> b140b354d127 ("usb: typec: Add type sysfs attribute file for partners")
> a67ad71c6468 ("usb: common: ulpi: Constify static attribute_group struct")
> 61336e5db8f9 ("usb: typec: Constify static attribute_group structs")
> f18890ead25d ("USB: core: Constify static attribute_group structs")
> 137bd0e04f46 ("usb: typec: tcpm: Stay in
> SNK_TRY_WAIT_DEBOUNCE_CHECK_VBUS till Rp is seen")
> 2413711487c3 ("usb: typec: tcpm: Disregard vbus off while in
> PR_SWAP_SNK_SRC_SOURCE_ON")
> 38707ad78e16 ("usb: typec: Expose Product Type VDOs via sysfs")
> 581140741224 ("usb: typec: Consolidate sysfs ABI documentation")
> 5b63ba7f3f62 ("usb: pd: DFP product types")
> b5325e0017d7 ("usb: isp1301-omap: Convert to use GPIO descriptors")
> 9e7290cb419d ("usb: Fix fall-through warnings for Clang")
> cf8314bb9f26 ("usb: typec: Fix num_altmodes kernel-doc error")
> 442fa9c05cab ("usb: typec: Add plug num_altmodes sysfs attr")
> 1193103c1076 ("usb: typec: tcpci_maxim: Fix the compatible string")
> 13c53c00e81f ("dt-bindings: usb: Maxim type-c controller device tree binding
> document")
> f5a98cc36225 ("usb: typec: tcpci_maxim: Fix uninitialized return variable")
> 203fd830a193 ("usb: typec: tcpci_maxim: Enable auto discharge
> disconnect")
> ebcf6c255080 ("usb: typec: tcpci: Implement Auto discharge disconnect
> callbacks")
> 428804eed4cd ("usb: typec: tcpm: Implement enabling Auto Discharge
> disconnect support")
> cece16630a9c ("usb: typec: tcpci_maxim: Fix vbus stuck on upon
> diconnecting sink")
> 294a8e667ad9 ("usb: typec: tcpci: frs sourcing vbus callback")
> 1c109ebc0134 ("usb: typec: tcpm: frs sourcing vbus callback")
> 85c16dc068ed ("usb: typec: tcpm: Refactor logic for
> new-source-frs-typec-current")
> 66d004aff708 ("usb: typec: Add number of altmodes partner attr")
> 259fcae90f7e ("usb: pd: Add captive Type C cable type")
> 1090d1e366ea ("USB: apple-mfi-fastcharge: Fix kfree after failed kzalloc")
> 44e0bf239206 ("usb/max3421: fix return error code in max3421_probe()")
> ff5e0488d7fe ("usb: typec: Remove one bit support for the Thunderbolt
> rounded/non-rounded cable")
> bb328b790edb ("usb: typec: intel_pmc_mux: Configure Thunderbolt cable
> generation bits")
> 235920c3c80e ("platform/chrome: cros_ec_typec: Correct the Thunderbolt
> rounded/non-rounded cable support")
> 9d11b7220dff ("usb: typec: Correct the bit values for the Thunderbolt
> rounded/non-rounded cable support")
> dd07e75a143a ("usb: hcd.h: Remove RUN_CONTEXT")
> 997d2edc58ed ("usb: host: ehci-mxc: Remove the driver")
> 1261e818b439 ("usb: host: imx21-hcd: Remove the driver")
> 170408f6e69a ("USB: storage: avoid use of uninitialized values in error
> path")
> 2de743b08132 ("usb: fix a few cases of -Wfallthrough")
> 69f7421c6160 ("usb: misc: brcmstb-usb-pinmap: Make sync_all_pins static")
> 0f399ba93d3e ("usb: typec: ucsi: Work around PPM losing change
> information")
> c1a6cb538802 ("usb: typec: ucsi: acpi: Always decode connector change
> information")
> dcb4ce0e4492 ("usb: misc: brcmstb-usb-pinmap: Fix an IS_ERR() vs NULL
> check")
> 68d6f2bcbfd0 ("kcov, usb: only collect coverage from
> __usb_hcd_giveback_urb in softirq")
> a7d43316e47c ("USB: host: isp1362: delete isp1362_show_regs()")
> 74bfec445b03 ("dt-bindings: connector: Add property to set initial current
> cap for FRS")
> 922a8008fe76 ("usb: xhci: Remove in_interrupt() checks")
> 9b28508e2a86 ("usbip: Remove in_interrupt() check")
> b3cbc18f80c6 ("usb: gadget: pxa27x_udc: Replace in_interrupt() usage in
> comments")
> ff6f2b42ea3a ("usb: core: Replace in_interrupt() in comments")
> b6d8ac49953b ("usb: gadget: udc: Remove in_interrupt()/in_irq() from
> comments")
> 399452b463eb ("USB: host: ehci-pmcmsp: Cleanup
> usb_hcd_msp_remove()")
> 87d64cb083fa ("usb: hosts: Remove in_interrupt() from comments")
> 6a700d8af537 ("usb: atm: Replace in_interrupt() usage in comment")
> 9573d69a7657 ("USB: sisusbvga: Make console support depend on
> BROKEN")
> 3d30c6e0bb36 ("usb: Add driver to allow any GPIO to be used for 7211 USB
> signals")
> e7ed6422dd9c ("dt-bindings: Add support for Broadcom USB pin map driver")
> 7f08972e12f0 ("usb: host: xhci-mem: remove unneeded break")
> 8a508616ef64 ("usb: storage: freecom: remove unneeded break")
> 597335205373 ("usb: misc: iowarrior: remove unneeded break")
> c902d58d8b24 ("usb: host: ehci-sched: add comment about find_tt() not
> returning error")
>
> are missing a Signed-off-by from their committers.
>
> It look like you have rebased (part of) Greg's usb tree into your tree :-(
Yes, you are right. It seems I could only rebase Greg's usb-next tree which I
tracked,
but I can't rebase Greg's usb-linus tree which contains bug fixes.
Ok, I will fix it, sorry about it.
Peter
Re: [PATCH v2 1/5] USB: gadget: f_rndis: fix bitrate for SuperSpeed and above
>
> From: Will McVicker
>
Reviewed-by: Peter Chen
Peter
> Align the SuperSpeed Plus bitrate for f_rndis to match f_ncm's ncm_bitrate
> defined by commit 1650113888fe ("usb: gadget: f_ncm: add SuperSpeed
> descriptors
> for CDC NCM").
>
> Cc: Felipe Balbi
> Cc: EJ Hsu
> Cc: Peter Chen
> Cc: stable
> Signed-off-by: Will McVicker
> Signed-off-by: Greg Kroah-Hartman
> ---
> drivers/usb/gadget/function/f_rndis.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/f_rndis.c
> b/drivers/usb/gadget/function/f_rndis.c
> index 9534c8ab62a8..0739b05a0ef7 100644
> --- a/drivers/usb/gadget/function/f_rndis.c
> +++ b/drivers/usb/gadget/function/f_rndis.c
> @@ -87,8 +87,10 @@ static inline struct f_rndis *func_to_rndis(struct
> usb_function *f)
> /* peak (theoretical) bulk transfer rate in bits-per-second */
> static unsigned int bitrate(struct usb_gadget *g)
> {
> + if (gadget_is_superspeed(g) && g->speed >= USB_SPEED_SUPER_PLUS)
> + return 425000U;
> if (gadget_is_superspeed(g) && g->speed == USB_SPEED_SUPER)
> - return 13 * 1024 * 8 * 1000 * 8;
> + return 375000U;
> else if (gadget_is_dualspeed(g) && g->speed == USB_SPEED_HIGH)
> return 13 * 512 * 8 * 1000 * 8;
> else
> --
> 2.29.2
>
Re: [PATCH v2 1/5] USB: gadget: f_rndis: fix bitrate for SuperSpeed and above
On 20-11-27 15:05:55, Greg Kroah-Hartman wrote:
> From: Will McVicker
>
> Align the SuperSpeed Plus bitrate for f_rndis to match f_ncm's ncm_bitrate
> defined by commit 1650113888fe ("usb: gadget: f_ncm: add SuperSpeed
> descriptors
> for CDC NCM").
>
> Cc: Felipe Balbi
> Cc: EJ Hsu
> Cc: Peter Chen
> Cc: stable
> Signed-off-by: Will McVicker
> Signed-off-by: Greg Kroah-Hartman
> ---
> drivers/usb/gadget/function/f_rndis.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/f_rndis.c
> b/drivers/usb/gadget/function/f_rndis.c
> index 9534c8ab62a8..0739b05a0ef7 100644
> --- a/drivers/usb/gadget/function/f_rndis.c
> +++ b/drivers/usb/gadget/function/f_rndis.c
> @@ -87,8 +87,10 @@ static inline struct f_rndis *func_to_rndis(struct
> usb_function *f)
> /* peak (theoretical) bulk transfer rate in bits-per-second */
> static unsigned int bitrate(struct usb_gadget *g)
> {
> + if (gadget_is_superspeed(g) && g->speed >= USB_SPEED_SUPER_PLUS)
> + return 425000U;
Is tested value or spec defined value?
> if (gadget_is_superspeed(g) && g->speed == USB_SPEED_SUPER)
> - return 13 * 1024 * 8 * 1000 * 8;
> + return 375000U;
13 * 1024 * 8 * 1000 * 8 = 851,968,000, how 375000U is calculated?
> else if (gadget_is_dualspeed(g) && g->speed == USB_SPEED_HIGH)
> return 13 * 512 * 8 * 1000 * 8;
> else
> --
> 2.29.2
>
--
Thanks,
Peter Chen
Re: [PATCH v2 5/5] USB: gadget: f_fs: remove likely/unlikely
On 20-11-27 15:05:59, Greg Kroah-Hartman wrote:
> They are used way too often in this file, in some ways that are actually
> wrong. Almost all of these are already known by the compiler and CPU so
> just remove them all as none of these should be on any "hot paths" where
> it actually matters.
>
> Cc: Felipe Balbi
> Reported-by: Peter Chen
> Signed-off-by: Greg Kroah-Hartman
Reviewed-by: Peter Chen
> ---
> drivers/usb/gadget/function/f_fs.c | 179 ++---
> 1 file changed, 89 insertions(+), 90 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c
> b/drivers/usb/gadget/function/f_fs.c
> index a34a7c96a1ab..9047b20b6715 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -296,11 +296,11 @@ static int __ffs_ep0_queue_wait(struct ffs_data *ffs,
> char *data, size_t len)
> reinit_completion(>ep0req_completion);
>
> ret = usb_ep_queue(ffs->gadget->ep0, req, GFP_ATOMIC);
> - if (unlikely(ret < 0))
> + if (ret < 0)
> return ret;
>
> ret = wait_for_completion_interruptible(>ep0req_completion);
> - if (unlikely(ret)) {
> + if (ret) {
> usb_ep_dequeue(ffs->gadget->ep0, req);
> return -EINTR;
> }
> @@ -337,7 +337,7 @@ static ssize_t ffs_ep0_write(struct file *file, const
> char __user *buf,
>
> /* Acquire mutex */
> ret = ffs_mutex_lock(>mutex, file->f_flags & O_NONBLOCK);
> - if (unlikely(ret < 0))
> + if (ret < 0)
> return ret;
>
> /* Check state */
> @@ -345,7 +345,7 @@ static ssize_t ffs_ep0_write(struct file *file, const
> char __user *buf,
> case FFS_READ_DESCRIPTORS:
> case FFS_READ_STRINGS:
> /* Copy data */
> - if (unlikely(len < 16)) {
> + if (len < 16) {
> ret = -EINVAL;
> break;
> }
> @@ -360,7 +360,7 @@ static ssize_t ffs_ep0_write(struct file *file, const
> char __user *buf,
> if (ffs->state == FFS_READ_DESCRIPTORS) {
> pr_info("read descriptors\n");
> ret = __ffs_data_got_descs(ffs, data, len);
> - if (unlikely(ret < 0))
> + if (ret < 0)
> break;
>
> ffs->state = FFS_READ_STRINGS;
> @@ -368,11 +368,11 @@ static ssize_t ffs_ep0_write(struct file *file, const
> char __user *buf,
> } else {
> pr_info("read strings\n");
> ret = __ffs_data_got_strings(ffs, data, len);
> - if (unlikely(ret < 0))
> + if (ret < 0)
> break;
>
> ret = ffs_epfiles_create(ffs);
> - if (unlikely(ret)) {
> + if (ret) {
> ffs->state = FFS_CLOSING;
> break;
> }
> @@ -381,7 +381,7 @@ static ssize_t ffs_ep0_write(struct file *file, const
> char __user *buf,
> mutex_unlock(>mutex);
>
> ret = ffs_ready(ffs);
> - if (unlikely(ret < 0)) {
> + if (ret < 0) {
> ffs->state = FFS_CLOSING;
> return ret;
> }
> @@ -495,7 +495,7 @@ static ssize_t __ffs_ep0_read_events(struct ffs_data
> *ffs, char __user *buf,
> spin_unlock_irq(>ev.waitq.lock);
> mutex_unlock(>mutex);
>
> - return unlikely(copy_to_user(buf, events, size)) ? -EFAULT : size;
> + return copy_to_user(buf, events, size) ? -EFAULT : size;
> }
>
> static ssize_t ffs_ep0_read(struct file *file, char __user *buf,
> @@ -514,7 +514,7 @@ static ssize_t ffs_ep0_read(struct file *file, char
> __user *buf,
>
> /* Acquire mutex */
> ret = ffs_mutex_lock(>mutex, file->f_flags & O_NONBLOCK);
> - if (unlikely(ret < 0))
> + if (ret < 0)
> return ret;
>
> /* Check state */
> @@ -536,7 +536,7 @@ static ssize_t ffs_ep0_read(struct file *file, char
> __user *buf,
>
> case FFS_NO_SETUP:
> n = len / sizeof(struct usb_functionfs_event);
> - if (unlikely(!n)) {
> + if (!n) {
> ret = -EINVAL;
> break;
> }
> @@ -567,9 +567,9 @@ static ssize_t ffs_ep
Re: [PATCH 4/4] USB: gadget: f_midi: setup SuperSpeed Plus descriptors
On 20-11-26 19:09:37, Greg Kroah-Hartman wrote:
> From: Will McVicker
>
> Needed for SuperSpeed Plus support for f_midi. This allows the
> gadget to work properly without crashing at SuperSpeed rates.
>
> Cc: Felipe Balbi
> Cc: stable
> Signed-off-by: Will McVicker
> Signed-off-by: Greg Kroah-Hartman
> ---
> drivers/usb/gadget/function/f_midi.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/usb/gadget/function/f_midi.c
> b/drivers/usb/gadget/function/f_midi.c
> index 85cb15734aa8..ceb67651de4f 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -1048,6 +1048,11 @@ static int f_midi_bind(struct usb_configuration *c,
> struct usb_function *f)
> f->ss_descriptors = usb_copy_descriptors(midi_function);
> if (!f->ss_descriptors)
> goto fail_f_midi;
Add one blank line, otherwise:
Reviewed-by: Peter Chen
Peter
> + if (gadget_is_superspeed_plus(c->cdev->gadget)) {
> + f->ssp_descriptors =
> usb_copy_descriptors(midi_function);
> + if (!f->ssp_descriptors)
> + goto fail_f_midi;
> + }
> }
>
> kfree(midi_function);
> --
> 2.29.2
>
--
Thanks,
Peter Chen
RE: [PATCH] usb: cdns3: Fix hardware based role switch
> Hi Peter,
>
> On 25/11/2020 14:49, Roger Quadros wrote:
> > Hardware based role switch is broken as the driver always skips it.
> > Fix this by registering for SW role switch only if 'usb-role-switch'
> > property is present in the device tree.
> >
> > Fixes: 50642709f659 ("usb: cdns3: core: quit if it uses role switch
> > class")
> > Signed-off-by: Roger Quadros
>
> Can you please pick this up for -rc cycle, else otg will be broken for us in
> v5.10
> release.
> Thanks.
>
It is at my -fixes tree along with my two fixes[1][2]. If there is no report
issue,
I will send it to Greg next Monday.
[1]
https://patchwork.kernel.org/project/linux-usb/patch/20201126065409.7533-1-peter.c...@kernel.org/
[2]
https://patchwork.kernel.org/project/linux-usb/patch/20201126065409.7533-2-peter.c...@kernel.org/
Peter
RE: [PATCH v3 00/10] Introduced new Cadence USBSSP DRD Driver.
> >>> > >>> The device side of USBSS DRD controller is compliant with XHCI. > >>> The architecture for device side is almost the same as for host > >>> side, and most of the XHCI specification can be used to understand > >>> how this controller operates. > >>> > >>> This controller and driver support Full Speed, Hight Speed, Supper > >>> Speed and Supper Speed Plus USB protocol. > >>> > >>> The prefix cdnsp used in driver has chosen by analogy to cdn3 driver. > >>> The last letter of this acronym means PLUS. The formal name of > >>> controller is USBSSP but it's to generic so I've decided to use CDNSP. > >>> > >>> The patch 1: adds support for DRD CDNSP. > >>> The patch 2: separates common code that can be reusable by cdnsp driver. > >>> The patch 3: moves reusable code to separate module. > >>> The patch 4: changes prefixes in reusable code from cdns3 to common > cdns. > >>> The patch 5: adopts gadget_dev pointer in cdns structure to make possible > >>> use it in both drivers. > >>> The patches 6-8: add the main part of driver and has been intentionally > >>> split into 3 part. In my opinion such division should not > >>> affect understanding and reviewing the driver, and cause > that > >>> main patch (7/8) is little smaller. Patch 6 introduces main > >>> header file for driver, 7 is the main part that implements > all > >>> functionality of driver and 8 introduces tracepoints. > >>> The patch 9: Adds cdns3 prefixes to files related with USBSS driver. > >>> the patch 10: Adds USBSSP DRD IP driver entry to MAINTAINERS file. > >>> > >>> Changlog from v2: > >>> - removed not used pdev parameter from cdnsp_read/wite_64 functions > >>> - fixed incorrect value assigned to CDNSP_ENDPOINTS_NUM (32 -> 31) > >>> - replaced some constant value with CDNSP_ENDPOINTS_NUM macro > >>> - replaced 'true' with '1' in bits description in cdnsp-gadget.h > >>> file > >>> - fixed some typos > >>> - some other less important changes suggested by Peter Chen > >> > >>Hi Pawel, > >> > >>I have updated my -next tree as the latest usb-next tree which > >>v5.10-rc4 is included, would you please rebase my tree and send again, > >>I could apply your patches and test, if test could pass, I will apply it to > >>my > -next tree. > >>You don't need to rebase again since it is a huge patch set, will take > >>some efforts for rebase. > >> > > > >I'll try to post it tomorrow. > > Can I send the new version CDNSP or I should wait for completion > 'Re: [PATCH] Revert "usb: cdns3: core: quit if it uses role switch class"' and > applying the appropriate fix to your repo ? > Please wait that fix, thanks. Peter
Re: [PATCH v3 10/10] MAINTAINERS: add Cadence USBSSP DRD IP driver entry
On 20-11-19 15:13:07, Pawel Laszczak wrote: > Patch adds entry for USBSSP (CDNSP) driver into MAINTARNERS file. > > Signed-off-by: Pawel Laszczak > --- > MAINTAINERS | 8 > 1 file changed, 8 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 842fef329119..70c31fd2cd61 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -3867,6 +3867,14 @@ S: Maintained > T: git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git > F: Documentation/devicetree/bindings/usb/cdns,usb3.yaml > F: drivers/usb/cdns3/ > +X: drivers/usb/cdns3/cdnsp* > + > +CADENCE USBSSP DRD IP DRIVER > +M: Pawel Laszczak > +L: linux-...@vger.kernel.org > +S: Maintained > +F: drivers/usb/cdns3/ > +X: drivers/usb/cdns3/cdns3* > Hi Pawel, You may add "T" for which tree for cdns3 ssp driver. -- Thanks, Peter Chen
Re: [PATCH] Revert "usb: cdns3: core: quit if it uses role switch class"
On 20-11-24 14:22:25, Roger Quadros wrote:
> Peter,
>
> On 24/11/2020 13:47, Peter Chen wrote:
> > On 20-11-24 12:33:34, Roger Quadros wrote:
> > > > > >
> > > > > > I am sorry about that. Do you use role switch /sys entry, if you
> > > > > > have
> > > > > > used, I prefer using "usb-role-switch" property at dts to judge if
> > > > > > SoC
> > > > > > OTG signals or external signals for role switch. If you have not
> > > > > > used
> > > > > > it, I prefer only setting cdns->role_sw for role switch use cases.
> > > > > >
> > > > >
> > > > > We use both hardware role switch and /sys entries for manually
> > > > > forcing a
> > > > > certain role.
> > > > >
> > > > > We do not set any "usb-role-switch" property at DTS.
> > > > >
> > > > > Currently cdns->role_sw is being always set by driver irrespective of
> > > > > any DT
> > > > > property, so this patch is clearly wrong and needs to be reverted.
> > > > >
> > > > > What do you think?
> > > > >
> > > >
> > > > Could you accept below fix?
> > > >
> > > > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> > > > index 2e469139769f..fdd52e87a7b2 100644
> > > > --- a/drivers/usb/cdns3/core.c
> > > > +++ b/drivers/usb/cdns3/core.c
> > > > @@ -280,8 +280,8 @@ int cdns3_hw_role_switch(struct cdns3 *cdns)
> > > > enum usb_role real_role, current_role;
> > > > int ret = 0;
> > > >
> > > > - /* Depends on role switch class */
> > > > - if (cdns->role_sw)
> > > > + /* quit if switch role through external signals */
> > > > + if (device_property_read_bool(cdns->dev, "usb-role-switch"))
> > > > return 0;
> > > >
> > > > pm_runtime_get_sync(cdns->dev);
> > >
> > > Although this will fix the issue I don't think this is making the driver
> > > to behave
> > > as expected with usb-role-switch property.
> > >
> > > Now, even if usb-role-switch property is not present the driver will
> > > still register
> > > the role switch driver.
> > >
> > > I think we need to register the role switch driver only if
> > > usb-role-switch property
> > > is present. We would also need to set the default role if
> > > role-switch-default-mode is present.
> > >
> > > How about the following? It still doesn't handle role-switch-default-mode
> > > property though.
> > >
> >
> > Roger, you said you also use /sys entries (I suppose it means through role
> > switch class) to do role switch, with your change, there will be no /sys
> > entry for role switch.
>
> Sorry for the confusion. Although we do need both features (SW role switch +
> HW role switch)
> I don't think it is required to operate simultaneously. If users need SW
> control they can set the DT flag.
>
I see. I prefer embracing all things related to role switch under the
firmware entry condition. Besides, I find another issue that devm_request_irq
for wakeup_irq does not call usb_role_switch_unregister if it has
failed. So, probably, two patches are needed. I am OK you send the
patches to fix both.
diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
index 2e469139769f..fc6a8152406c 100644
--- a/drivers/usb/cdns3/core.c
+++ b/drivers/usb/cdns3/core.c
@@ -427,7 +427,6 @@ static irqreturn_t cdns3_wakeup_irq(int irq, void *data)
*/
static int cdns3_probe(struct platform_device *pdev)
{
- struct usb_role_switch_desc sw_desc = { };
struct device *dev = >dev;
struct resource *res;
struct cdns3 *cdns;
@@ -529,18 +528,21 @@ static int cdns3_probe(struct platform_device *pdev)
if (ret)
goto err2;
- sw_desc.set = cdns3_role_set;
- sw_desc.get = cdns3_role_get;
- sw_desc.allow_userspace_control = true;
- sw_desc.driver_data = cdns;
- if (device_property_read_bool(dev, "usb-role-switch"))
+ if (device_property_read_bool(dev, "usb-role-switch")) {
+ struct usb_role_switch_desc sw_desc = { };
+
+ sw_desc.set = cdns3_role_set;
+ sw_desc.get = cdns3_role_get;
+ sw_desc.allow_userspace_control = true;
+ sw_desc.d
Re: [PATCH] Revert "usb: cdns3: core: quit if it uses role switch class"
On 20-11-24 12:33:34, Roger Quadros wrote:
> > > >
> > > > I am sorry about that. Do you use role switch /sys entry, if you have
> > > > used, I prefer using "usb-role-switch" property at dts to judge if SoC
> > > > OTG signals or external signals for role switch. If you have not used
> > > > it, I prefer only setting cdns->role_sw for role switch use cases.
> > > >
> > >
> > > We use both hardware role switch and /sys entries for manually forcing a
> > > certain role.
> > >
> > > We do not set any "usb-role-switch" property at DTS.
> > >
> > > Currently cdns->role_sw is being always set by driver irrespective of any
> > > DT
> > > property, so this patch is clearly wrong and needs to be reverted.
> > >
> > > What do you think?
> > >
> >
> > Could you accept below fix?
> >
> > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> > index 2e469139769f..fdd52e87a7b2 100644
> > --- a/drivers/usb/cdns3/core.c
> > +++ b/drivers/usb/cdns3/core.c
> > @@ -280,8 +280,8 @@ int cdns3_hw_role_switch(struct cdns3 *cdns)
> > enum usb_role real_role, current_role;
> > int ret = 0;
> >
> > - /* Depends on role switch class */
> > - if (cdns->role_sw)
> > + /* quit if switch role through external signals */
> > + if (device_property_read_bool(cdns->dev, "usb-role-switch"))
> > return 0;
> >
> > pm_runtime_get_sync(cdns->dev);
>
> Although this will fix the issue I don't think this is making the driver to
> behave
> as expected with usb-role-switch property.
>
> Now, even if usb-role-switch property is not present the driver will still
> register
> the role switch driver.
>
> I think we need to register the role switch driver only if usb-role-switch
> property
> is present. We would also need to set the default role if
> role-switch-default-mode is present.
>
> How about the following? It still doesn't handle role-switch-default-mode
> property though.
>
Roger, you said you also use /sys entries (I suppose it means through role
switch class) to do role switch, with your change, there will be no /sys
entry for role switch.
Peter
> > > We use both hardware role switch and /sys entries for manually forcing a
> > > certain role.
> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> index 4c1445cf2ad0..986b56a9940c 100644
> --- a/drivers/usb/cdns3/core.c
> +++ b/drivers/usb/cdns3/core.c
> @@ -532,11 +532,13 @@ static int cdns3_probe(struct platform_device *pdev)
> if (device_property_read_bool(dev, "usb-role-switch"))
> sw_desc.fwnode = dev->fwnode;
> - cdns->role_sw = usb_role_switch_register(dev, _desc);
> - if (IS_ERR(cdns->role_sw)) {
> - ret = PTR_ERR(cdns->role_sw);
> - dev_warn(dev, "Unable to register Role Switch\n");
> - goto err3;
> + if (device_property_read_bool(cdns->dev, "usb-role-switch")) {
> + cdns->role_sw = usb_role_switch_register(dev, _desc);
> + if (IS_ERR(cdns->role_sw)) {
> + ret = PTR_ERR(cdns->role_sw);
> + dev_warn(dev, "Unable to register Role Switch\n");
> + goto err3;
> + }
> }
> if (cdns->wakeup_irq) {
>
>
>
> cheers,
> -roger
> --
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
--
Thanks,
Peter Chen
RE: [PATCH] Revert "usb: cdns3: core: quit if it uses role switch class"
Best regards, Peter Chen > -Original Message- > From: Roger Quadros > Sent: 2020年11月24日 17:39 > To: Peter Chen > Cc: paw...@cadence.com; gre...@linuxfoundation.org; ba...@kernel.org; > linux-...@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] Revert "usb: cdns3: core: quit if it uses role switch > class" > > Peter, > > On 24/11/2020 08:43, Peter Chen wrote: > > On 20-11-23 13:50:51, Roger Quadros wrote: > >> This reverts commit 50642709f6590fe40afa6d22c32f23f5b842aed5. > >> > >> This commit breaks hardware based role switching on TI platforms. > >> cdns->role_sw is always going to be non-zero as it is a pointer > >> to the usb_role_switch instance. Some other means needs to be used if > >> hardware based role switching is not required by the platform. > >> > >> Signed-off-by: Roger Quadros > >> --- > >> drivers/usb/cdns3/core.c | 4 > >> 1 file changed, 4 deletions(-) > >> > >> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c > >> index a0f73d4711ae..4c1445cf2ad0 100644 > >> --- a/drivers/usb/cdns3/core.c > >> +++ b/drivers/usb/cdns3/core.c > >> @@ -280,10 +280,6 @@ int cdns3_hw_role_switch(struct cdns3 *cdns) > >>enum usb_role real_role, current_role; > >>int ret = 0; > >> > >> - /* Depends on role switch class */ > >> - if (cdns->role_sw) > >> - return 0; > >> - > >>pm_runtime_get_sync(cdns->dev); > >> > >>current_role = cdns->role; > >> -- > > > > Hi Roger, > > > > I am sorry about that. Do you use role switch /sys entry, if you have > > used, I prefer using "usb-role-switch" property at dts to judge if SoC > > OTG signals or external signals for role switch. If you have not used > > it, I prefer only setting cdns->role_sw for role switch use cases. > > > > We use both hardware role switch and /sys entries for manually forcing a > certain role. > > We do not set any "usb-role-switch" property at DTS. > > Currently cdns->role_sw is being always set by driver irrespective of any DT > property, so this patch is clearly wrong and needs to be reverted. > > What do you think? > Could you accept below fix? diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c index 2e469139769f..fdd52e87a7b2 100644 --- a/drivers/usb/cdns3/core.c +++ b/drivers/usb/cdns3/core.c @@ -280,8 +280,8 @@ int cdns3_hw_role_switch(struct cdns3 *cdns) enum usb_role real_role, current_role; int ret = 0; - /* Depends on role switch class */ - if (cdns->role_sw) + /* quit if switch role through external signals */ + if (device_property_read_bool(cdns->dev, "usb-role-switch")) return 0; pm_runtime_get_sync(cdns->dev); Peter
Re: [PATCH v3 00/10] Introduced new Cadence USBSSP DRD Driver.
On 20-11-19 15:12:57, Pawel Laszczak wrote:
> This patch introduce new Cadence USBSS DRD driver to linux kernel.
>
> The Cadence USBSS DRD Controller is a highly configurable IP Core which
> can be instantiated as Dual-Role Device (DRD), Peripheral Only and
> Host Only (XHCI)configurations.
>
> The current driver has been validated with FPGA burned. We have support
> for PCIe bus, which is used on FPGA prototyping.
>
> The host side of USBSS-DRD controller is compliance with XHCI
> specification, so it works with standard XHCI Linux driver.
>
> The device side of USBSS DRD controller is compliant with XHCI.
> The architecture for device side is almost the same as for host side,
> and most of the XHCI specification can be used to understand how
> this controller operates.
>
> This controller and driver support Full Speed, Hight Speed, Supper Speed
> and Supper Speed Plus USB protocol.
>
> The prefix cdnsp used in driver has chosen by analogy to cdn3 driver.
> The last letter of this acronym means PLUS. The formal name of controller
> is USBSSP but it's to generic so I've decided to use CDNSP.
>
> The patch 1: adds support for DRD CDNSP.
> The patch 2: separates common code that can be reusable by cdnsp driver.
> The patch 3: moves reusable code to separate module.
> The patch 4: changes prefixes in reusable code from cdns3 to common cdns.
> The patch 5: adopts gadget_dev pointer in cdns structure to make possible
> use it in both drivers.
> The patches 6-8: add the main part of driver and has been intentionally
> split into 3 part. In my opinion such division should not
> affect understanding and reviewing the driver, and cause that
> main patch (7/8) is little smaller. Patch 6 introduces main
> header file for driver, 7 is the main part that implements all
> functionality of driver and 8 introduces tracepoints.
> The patch 9: Adds cdns3 prefixes to files related with USBSS driver.
> the patch 10: Adds USBSSP DRD IP driver entry to MAINTAINERS file.
>
> Changlog from v2:
> - removed not used pdev parameter from cdnsp_read/wite_64 functions
> - fixed incorrect value assigned to CDNSP_ENDPOINTS_NUM (32 -> 31)
> - replaced some constant value with CDNSP_ENDPOINTS_NUM macro
> - replaced 'true' with '1' in bits description in cdnsp-gadget.h file
> - fixed some typos
> - some other less important changes suggested by Peter Chen
Hi Pawel,
I have updated my -next tree as the latest usb-next tree which v5.10-rc4
is included, would you please rebase my tree and send again, I could apply your
patches and test, if test could pass, I will apply it to my -next tree.
You don't need to rebase again since it is a huge patch set, will take some
efforts for rebase.
Peter
>
> Changlog from v1:
> - updated common code to latest cdns3 driver
> - moved cdnsp driver files to cdns3 as sugested by Peter Chan
> - removed duplicate code from cdnsp_ep0_set_config function
> - added cdns3 prefixes to file related with USBSS driver
> - updated MAINTAINERS file
> - fixed issue with U1
> - fixed issue with L1
> - some less improtant changes sugested by Chunfeng Yun
> ---
>
> Pawel Laszczak (10):
> usb: cdns3: Add support for DRD CDNSP
> usb: cdns3: Split core.c into cdns3-plat and core.c file
> usb: cdns3: Moves reusable code to separate module
> usb: cdns3: Refactoring names in reusable code
> usb: cdns3: Changed type of gadget_dev in cdns structure
> usb: cdnsp: Device side header file for CDNSP driver
> usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver
> usb: cdnsp: Add tracepoints for CDNSP driver
> usb: cdns3: Change file names for cdns3 driver.
> MAINTAINERS: add Cadence USBSSP DRD IP driver entry
>
> MAINTAINERS |8 +
> drivers/usb/Makefile |2 +
> drivers/usb/cdns3/Kconfig | 61 +-
> drivers/usb/cdns3/Makefile| 30 +-
> drivers/usb/cdns3/{debug.h => cdns3-debug.h} |0
> drivers/usb/cdns3/{ep0.c => cdns3-ep0.c} |4 +-
> .../usb/cdns3/{gadget.c => cdns3-gadget.c}| 28 +-
> .../usb/cdns3/{gadget.h => cdns3-gadget.h}|0
> drivers/usb/cdns3/cdns3-imx.c |2 +-
> drivers/usb/cdns3/cdns3-plat.c| 315 +++
> drivers/usb/cdns3/{trace.c => cdns3-trace.c} |2 +-
> drivers/usb/cdns3/{trace.h => cdns3-trace.h} |6 +-
> drivers/usb/cdns3/cdnsp-debug.h | 583
> drivers/usb/cdns3/cdnsp-ep0.c | 495
> drivers/usb/cdns3/cdnsp-gadget.c | 2017 ++
> drivers/usb/cdns3/cdnsp-gadget.h | 1600
Re: [PATCH] usb: cdns3: fix NULL pointer dereference on no platform data
On 20-11-23 12:49:31, Roger Quadros wrote:
> Some platforms (e.g. TI) will not have any platform data which will
> lead to NULL pointer dereference if we don't check for NULL pdata.
>
> Fixes: a284b7fd1b8f ("usb: cdns3: add quirk for enable runtime pm by default")
> Reported-by: Nishanth Menon
> Signed-off-by: Roger Quadros
> ---
> drivers/usb/cdns3/core.c | 2 +-
> drivers/usb/cdns3/host.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> index 54d841aa626f..0f08aebce86d 100644
> --- a/drivers/usb/cdns3/core.c
> +++ b/drivers/usb/cdns3/core.c
> @@ -559,7 +559,7 @@ static int cdns3_probe(struct platform_device *pdev)
> device_set_wakeup_capable(dev, true);
> pm_runtime_set_active(dev);
> pm_runtime_enable(dev);
> - if (!(cdns->pdata->quirks & CDNS3_DEFAULT_PM_RUNTIME_ALLOW))
> + if (!(cdns->pdata && (cdns->pdata->quirks &
> CDNS3_DEFAULT_PM_RUNTIME_ALLOW)))
> pm_runtime_forbid(dev);
>
> /*
> diff --git a/drivers/usb/cdns3/host.c b/drivers/usb/cdns3/host.c
> index 08103785a17a..ec89f2e5430f 100644
> --- a/drivers/usb/cdns3/host.c
> +++ b/drivers/usb/cdns3/host.c
> @@ -59,7 +59,7 @@ static int __cdns3_host_init(struct cdns3 *cdns)
> goto err1;
> }
>
> - if (cdns->pdata->quirks & CDNS3_DEFAULT_PM_RUNTIME_ALLOW)
> + if (cdns->pdata && (cdns->pdata->quirks &
> CDNS3_DEFAULT_PM_RUNTIME_ALLOW))
> cdns->xhci_plat_data->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW;
>
> ret = platform_device_add_data(xhci, cdns->xhci_plat_data,
Thanks for fixing it, already applied to -next tree.
--
Thanks,
Peter Chen
Re: [PATCH] Revert "usb: cdns3: core: quit if it uses role switch class"
On 20-11-23 13:50:51, Roger Quadros wrote: > This reverts commit 50642709f6590fe40afa6d22c32f23f5b842aed5. > > This commit breaks hardware based role switching on TI platforms. > cdns->role_sw is always going to be non-zero as it is a pointer > to the usb_role_switch instance. Some other means needs to be used > if hardware based role switching is not required by the platform. > > Signed-off-by: Roger Quadros > --- > drivers/usb/cdns3/core.c | 4 > 1 file changed, 4 deletions(-) > > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c > index a0f73d4711ae..4c1445cf2ad0 100644 > --- a/drivers/usb/cdns3/core.c > +++ b/drivers/usb/cdns3/core.c > @@ -280,10 +280,6 @@ int cdns3_hw_role_switch(struct cdns3 *cdns) > enum usb_role real_role, current_role; > int ret = 0; > > - /* Depends on role switch class */ > - if (cdns->role_sw) > - return 0; > - > pm_runtime_get_sync(cdns->dev); > > current_role = cdns->role; > -- Hi Roger, I am sorry about that. Do you use role switch /sys entry, if you have used, I prefer using "usb-role-switch" property at dts to judge if SoC OTG signals or external signals for role switch. If you have not used it, I prefer only setting cdns->role_sw for role switch use cases. -- Thanks, Peter Chen
Re: [PATCH v2] usb: gadget: mass_storage: fix error return code in msg_bind()
On 20-11-12 21:54:23, Chen Zhou wrote:
> Fix to return a negative error code from the error handling case
> instead of 0 in function msg_bind(), as done elsewhere in this
> function.
>
> Fixes: d86788979761 ("usb: gadget: mass_storage: allocate and init otg
> descriptor by otg capabilities")
> Reported-by: Hulk Robot
> Signed-off-by: Chen Zhou
Reviewed-by: Peter Chen
Peter
> ---
> drivers/usb/gadget/legacy/mass_storage.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/legacy/mass_storage.c
> b/drivers/usb/gadget/legacy/mass_storage.c
> index 9ed22c5fb7fe..ac1741126619 100644
> --- a/drivers/usb/gadget/legacy/mass_storage.c
> +++ b/drivers/usb/gadget/legacy/mass_storage.c
> @@ -175,8 +175,10 @@ static int msg_bind(struct usb_composite_dev *cdev)
> struct usb_descriptor_header *usb_desc;
>
> usb_desc = usb_otg_descriptor_alloc(cdev->gadget);
> - if (!usb_desc)
> + if (!usb_desc) {
> + status = -ENOMEM;
> goto fail_string_ids;
> + }
> usb_otg_descriptor_init(cdev->gadget, usb_desc);
> otg_desc[0] = usb_desc;
> otg_desc[1] = NULL;
> --
> 2.20.1
>
--
Thanks,
Peter Chen
Re: [PATCH 3/3] usb: gadget: configfs: Add a specific configFS reset callback
On 20-11-14 00:12:47, Wesley Cheng wrote:
> In order for configFS based USB gadgets to set the proper charge current
> for bus reset scenarios, expose a separate reset callback to set the
> current to 100mA based on the USB battery charging specification.
>
> Signed-off-by: Wesley Cheng
Reviewed-by: Peter Chen
> ---
> drivers/usb/gadget/configfs.c | 24 +++-
> 1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> index 56051bb97349..80ca7ff2fb97 100644
> --- a/drivers/usb/gadget/configfs.c
> +++ b/drivers/usb/gadget/configfs.c
> @@ -1481,6 +1481,28 @@ static void configfs_composite_disconnect(struct
> usb_gadget *gadget)
> spin_unlock_irqrestore(>spinlock, flags);
> }
>
> +static void configfs_composite_reset(struct usb_gadget *gadget)
> +{
> + struct usb_composite_dev *cdev;
> + struct gadget_info *gi;
> + unsigned long flags;
> +
> + cdev = get_gadget_data(gadget);
> + if (!cdev)
> + return;
> +
> + gi = container_of(cdev, struct gadget_info, cdev);
> + spin_lock_irqsave(>spinlock, flags);
> + cdev = get_gadget_data(gadget);
> + if (!cdev || gi->unbind) {
> + spin_unlock_irqrestore(>spinlock, flags);
> + return;
> + }
> +
> + composite_reset(gadget);
> + spin_unlock_irqrestore(>spinlock, flags);
> +}
> +
> static void configfs_composite_suspend(struct usb_gadget *gadget)
> {
> struct usb_composite_dev *cdev;
> @@ -1530,7 +1552,7 @@ static const struct usb_gadget_driver
> configfs_driver_template = {
> .unbind = configfs_composite_unbind,
>
> .setup = configfs_composite_setup,
> - .reset = configfs_composite_disconnect,
> + .reset = configfs_composite_reset,
> .disconnect = configfs_composite_disconnect,
>
> .suspend = configfs_composite_suspend,
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
--
Thanks,
Peter Chen
Re: [PATCH 2/3] usb: gadget: composite: Split composite reset and disconnect
On 20-11-14 00:12:46, Wesley Cheng wrote:
> Add a specific composite reset API to differentiate between disconnect and
> reset events. This is needed for adjusting the current draw accordingly
> based on the USB battery charging specification. The device is only allowed
> to draw the 500/900 mA (HS/SS) while in the CONFIGURED state, and only 100 mA
> in the connected and UNCONFIGURED state.
>
> Signed-off-by: Wesley Cheng
Reviewed-by: Peter Chen
Peter
> ---
> drivers/usb/gadget/composite.c | 21 +++--
> include/linux/usb/composite.h | 2 ++
> 2 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 05b176c82cc5..a41f7fe4b518 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -2036,7 +2036,7 @@ composite_setup(struct usb_gadget *gadget, const struct
> usb_ctrlrequest *ctrl)
> return value;
> }
>
> -void composite_disconnect(struct usb_gadget *gadget)
> +static void __composite_disconnect(struct usb_gadget *gadget)
> {
> struct usb_composite_dev*cdev = get_gadget_data(gadget);
> unsigned long flags;
> @@ -2053,6 +2053,23 @@ void composite_disconnect(struct usb_gadget *gadget)
> spin_unlock_irqrestore(>lock, flags);
> }
>
> +void composite_disconnect(struct usb_gadget *gadget)
> +{
> + usb_gadget_vbus_draw(gadget, 0);
> + __composite_disconnect(gadget);
> +}
> +
> +void composite_reset(struct usb_gadget *gadget)
> +{
> + /*
> + * Section 1.4.13 Standard Downstream Port of the USB battery charging
> + * specification v1.2 states that a device connected on a SDP shall only
> + * draw at max 100mA while in a connected, but unconfigured state.
> + */
> + usb_gadget_vbus_draw(gadget, 100);
> + __composite_disconnect(gadget);
> +}
> +
> /*-*/
>
> static ssize_t suspended_show(struct device *dev, struct device_attribute
> *attr,
> @@ -2373,7 +2390,7 @@ static const struct usb_gadget_driver
> composite_driver_template = {
> .unbind = composite_unbind,
>
> .setup = composite_setup,
> - .reset = composite_disconnect,
> + .reset = composite_reset,
> .disconnect = composite_disconnect,
>
> .suspend= composite_suspend,
> diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
> index 2040696d75b6..0d8a71471512 100644
> --- a/include/linux/usb/composite.h
> +++ b/include/linux/usb/composite.h
> @@ -525,6 +525,8 @@ extern struct usb_string *usb_gstrings_attach(struct
> usb_composite_dev *cdev,
> extern int usb_string_ids_n(struct usb_composite_dev *c, unsigned n);
>
> extern void composite_disconnect(struct usb_gadget *gadget);
> +extern void composite_reset(struct usb_gadget *gadget);
> +
> extern int composite_setup(struct usb_gadget *gadget,
> const struct usb_ctrlrequest *ctrl);
> extern void composite_suspend(struct usb_gadget *gadget);
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
--
Thanks,
Peter Chen
Re: [PATCH 1/3] usb: dwc3: gadget: Introduce a DWC3 VBUS draw callback
On 20-11-14 00:12:45, Wesley Cheng wrote:
> Some devices support charging while in device mode. In these situations,
> the USB gadget will notify the DWC3 gadget driver to modify the current
> based on the enumeration and device state. The usb_phy_set_power() API
> will allow external charger entities to adjust the charge current through
> the notifier block.
>
> Signed-off-by: Wesley Cheng
> ---
> drivers/usb/dwc3/gadget.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index c145da1d8ba5..69122f66978e 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2387,6 +2387,16 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g,
> spin_unlock_irqrestore(>lock, flags);
> }
>
> +static int dwc3_gadget_vbus_draw(struct usb_gadget *g, unsigned int mA)
> +{
> + struct dwc3 *dwc = gadget_to_dwc(g);
> +
> + if (dwc->usb2_phy)
> + return usb_phy_set_power(dwc->usb2_phy, mA);
> +
> + return 0;
> +}
> +
> static const struct usb_gadget_ops dwc3_gadget_ops = {
> .get_frame = dwc3_gadget_get_frame,
> .wakeup = dwc3_gadget_wakeup,
> @@ -2396,6 +2406,7 @@ static const struct usb_gadget_ops dwc3_gadget_ops = {
> .udc_stop = dwc3_gadget_stop,
> .udc_set_speed = dwc3_gadget_set_speed,
> .get_config_params = dwc3_gadget_config_params,
> + .vbus_draw = dwc3_gadget_vbus_draw,
> };
>
Reviewed-by: Peter Chen
--
Thanks,
Peter Chen
RE: linux-next: build failure after merge of the usb-chipidea-next tree
> > After merging the usb-chipidea-next tree, today's linux-next build (powerpc > allyesconfig) failed like this: > > In file included from drivers/usb/chipidea/trace.h:18, > from drivers/usb/chipidea/trace.c:11: > drivers/usb/chipidea/ci.h: In function 'ci_otg_is_fsm_mode': > drivers/usb/chipidea/ci.h:440:47: error: invalid use of undefined type 'struct > ci_hdrc_platform_data' > 440 | struct usb_otg_caps *otg_caps = >platdata->ci_otg_caps; > | ^~ > Thanks, I fixed it today. Peter
RE: linux-next: Fixes tag needs some work in the usb-chipidea-fixes tree
>
> In commit
>
> 3d4ee0b42f65 ("usb: cdns3: gadget: initialize link_trb as NULL")
>
> Fixes tag
>
> Fixes: 4e218882eb5a ("usb: cdns3: gadget: improve the dump TRB operation
>
> has these problem(s):
>
> - Subject has leading but no trailing parentheses
> - Subject has leading but no trailing quotes
>
> Please do not split Fixes tags over more than one line.
>
> --
Thanks, Stephen. I fixed it.
I wonder if checkpatch.pl could report this issue?
Peter
Re: [PATCH v2 08/10] usb: cdnsp: Add tracepoints for CDNSP driver
On 20-11-06 12:42:58, Pawel Laszczak wrote:
> Patch adds the series of tracepoints that can be used for
> debugging issues detected in driver.
>
> Signed-off-by: Pawel Laszczak
Reviewed-by: Peter Chen
> ---
> drivers/usb/cdns3/Makefile | 5 +
> drivers/usb/cdns3/cdnsp-debug.h | 583 +
> drivers/usb/cdns3/cdnsp-ep0.c| 22 +-
> drivers/usb/cdns3/cdnsp-gadget.c | 75 ++-
> drivers/usb/cdns3/cdnsp-mem.c| 18 +-
> drivers/usb/cdns3/cdnsp-ring.c | 75 ++-
> drivers/usb/cdns3/cdnsp-trace.c | 12 +
> drivers/usb/cdns3/cdnsp-trace.h | 840 +++
> 8 files changed, 1614 insertions(+), 16 deletions(-)
> create mode 100644 drivers/usb/cdns3/cdnsp-debug.h
> create mode 100644 drivers/usb/cdns3/cdnsp-trace.c
> create mode 100644 drivers/usb/cdns3/cdnsp-trace.h
>
> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
> index a84b129f14b8..a4fdaabdbe18 100644
> --- a/drivers/usb/cdns3/Makefile
> +++ b/drivers/usb/cdns3/Makefile
> @@ -1,6 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0
> # define_trace.h needs to know how to find our header
> CFLAGS_trace.o := -I$(src)
> +CFLAGS_cdnsp-trace.o := -I$(src)
>
> cdns-usb-common-y:= core.o drd.o
> cdns3-y := cdns3-plat.o
> @@ -23,3 +24,7 @@ cdnsp-udc-pci-y :=
> cdnsp-pci.o
> obj-$(CONFIG_USB_CDNSP_PCI) += cdnsp-udc-pci.o
> cdnsp-udc-pci-$(CONFIG_USB_CDNSP_GADGET) += cdnsp-ring.o cdnsp-gadget.o \
> cdnsp-mem.o cdnsp-ep0.o
> +
> +ifneq ($(CONFIG_USB_CDNSP_GADGET),)
> +cdnsp-udc-pci-$(CONFIG_TRACING) += cdnsp-trace.o
> +endif
> diff --git a/drivers/usb/cdns3/cdnsp-debug.h b/drivers/usb/cdns3/cdnsp-debug.h
> new file mode 100644
> index ..d6345d4d2911
> --- /dev/null
> +++ b/drivers/usb/cdns3/cdnsp-debug.h
> @@ -0,0 +1,583 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Cadence CDNSP DRD Driver.
> + *
> + * Copyright (C) 2020 Cadence.
> + *
> + * Author: Pawel Laszczak
> + *
> + */
> +#ifndef __LINUX_CDNSP_DEBUG
> +#define __LINUX_CDNSP_DEBUG
> +
> +static inline const char *cdnsp_trb_comp_code_string(u8 status)
> +{
> + switch (status) {
> + case COMP_INVALID:
> + return "Invalid";
> + case COMP_SUCCESS:
> + return "Success";
> + case COMP_DATA_BUFFER_ERROR:
> + return "Data Buffer Error";
> + case COMP_BABBLE_DETECTED_ERROR:
> + return "Babble Detected";
> + case COMP_TRB_ERROR:
> + return "TRB Error";
> + case COMP_RESOURCE_ERROR:
> + return "Resource Error";
> + case COMP_NO_SLOTS_AVAILABLE_ERROR:
> + return "No Slots Available Error";
> + case COMP_INVALID_STREAM_TYPE_ERROR:
> + return "Invalid Stream Type Error";
> + case COMP_SLOT_NOT_ENABLED_ERROR:
> + return "Slot Not Enabled Error";
> + case COMP_ENDPOINT_NOT_ENABLED_ERROR:
> + return "Endpoint Not Enabled Error";
> + case COMP_SHORT_PACKET:
> + return "Short Packet";
> + case COMP_RING_UNDERRUN:
> + return "Ring Underrun";
> + case COMP_RING_OVERRUN:
> + return "Ring Overrun";
> + case COMP_VF_EVENT_RING_FULL_ERROR:
> + return "VF Event Ring Full Error";
> + case COMP_PARAMETER_ERROR:
> + return "Parameter Error";
> + case COMP_CONTEXT_STATE_ERROR:
> + return "Context State Error";
> + case COMP_EVENT_RING_FULL_ERROR:
> + return "Event Ring Full Error";
> + case COMP_INCOMPATIBLE_DEVICE_ERROR:
> + return "Incompatible Device Error";
> + case COMP_MISSED_SERVICE_ERROR:
> + return "Missed Service Error";
> + case COMP_COMMAND_RING_STOPPED:
> + return "Command Ring Stopped";
> + case COMP_COMMAND_ABORTED:
> + return "Command Aborted";
> + case COMP_STOPPED:
> + return "Stopped";
> + case COMP_STOPPED_LENGTH_INVALID:
> + return "Stopped - Length Invalid";
> + case COMP_STOPPED_SHORT_PACKET:
> + return "Stopped - Short Packet";
> + case COMP_MAX_EXIT_LATENCY_TOO_LARGE_ERROR:
> +
Re: [PATCH v2 09/10] usb: cdns3: Change file names for cdns3 driver.
On 20-11-06 12:42:59, Pawel Laszczak wrote:
> Patch adds prefix cdns3- to all file names related only to
> cdsn3 driver.
%s/cdsn3/cdns3
Peter
>
> Signed-off-by: Pawel Laszczak
> ---
> drivers/usb/cdns3/Makefile | 6 +++---
> drivers/usb/cdns3/{debug.h => cdns3-debug.h} | 0
> drivers/usb/cdns3/{ep0.c => cdns3-ep0.c} | 4 ++--
> drivers/usb/cdns3/{gadget.c => cdns3-gadget.c} | 4 ++--
> drivers/usb/cdns3/{gadget.h => cdns3-gadget.h} | 0
> drivers/usb/cdns3/{trace.c => cdns3-trace.c} | 2 +-
> drivers/usb/cdns3/{trace.h => cdns3-trace.h} | 6 +++---
> 7 files changed, 11 insertions(+), 11 deletions(-)
> rename drivers/usb/cdns3/{debug.h => cdns3-debug.h} (100%)
> rename drivers/usb/cdns3/{ep0.c => cdns3-ep0.c} (99%)
> rename drivers/usb/cdns3/{gadget.c => cdns3-gadget.c} (99%)
> rename drivers/usb/cdns3/{gadget.h => cdns3-gadget.h} (100%)
> rename drivers/usb/cdns3/{trace.c => cdns3-trace.c} (89%)
> rename drivers/usb/cdns3/{trace.h => cdns3-trace.h} (99%)
>
> diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
> index a4fdaabdbe18..01a9a9620044 100644
> --- a/drivers/usb/cdns3/Makefile
> +++ b/drivers/usb/cdns3/Makefile
> @@ -1,6 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0
> # define_trace.h needs to know how to find our header
> -CFLAGS_trace.o := -I$(src)
> +CFLAGS_cdns3-trace.o := -I$(src)
> CFLAGS_cdnsp-trace.o := -I$(src)
>
> cdns-usb-common-y:= core.o drd.o
> @@ -10,10 +10,10 @@ obj-$(CONFIG_USB_CDNS3) +=
> cdns3.o
> obj-$(CONFIG_USB_CDNS_SUPPORT) += cdns-usb-common.o
>
> cdns-usb-common-$(CONFIG_USB_CDNS_HOST) += host.o
> -cdns3-$(CONFIG_USB_CDNS3_GADGET) += gadget.o ep0.o
> +cdns3-$(CONFIG_USB_CDNS3_GADGET) += cdns3-gadget.o cdns3-ep0.o
>
> ifneq ($(CONFIG_USB_CDNS3_GADGET),)
> -cdns3-$(CONFIG_TRACING) += trace.o
> +cdns3-$(CONFIG_TRACING) += cdns3-trace.o
> endif
>
> obj-$(CONFIG_USB_CDNS3_PCI_WRAP) += cdns3-pci-wrap.o
> diff --git a/drivers/usb/cdns3/debug.h b/drivers/usb/cdns3/cdns3-debug.h
> similarity index 100%
> rename from drivers/usb/cdns3/debug.h
> rename to drivers/usb/cdns3/cdns3-debug.h
> diff --git a/drivers/usb/cdns3/ep0.c b/drivers/usb/cdns3/cdns3-ep0.c
> similarity index 99%
> rename from drivers/usb/cdns3/ep0.c
> rename to drivers/usb/cdns3/cdns3-ep0.c
> index d3121a32cc68..b0390fe9a396 100644
> --- a/drivers/usb/cdns3/ep0.c
> +++ b/drivers/usb/cdns3/cdns3-ep0.c
> @@ -13,8 +13,8 @@
> #include
> #include
>
> -#include "gadget.h"
> -#include "trace.h"
> +#include "cdns3-gadget.h"
> +#include "cdns3-trace.h"
>
> static struct usb_endpoint_descriptor cdns3_gadget_ep0_desc = {
> .bLength = USB_DT_ENDPOINT_SIZE,
> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/cdns3-gadget.c
> similarity index 99%
> rename from drivers/usb/cdns3/gadget.c
> rename to drivers/usb/cdns3/cdns3-gadget.c
> index 9e0a82063873..d507a23c7047 100644
> --- a/drivers/usb/cdns3/gadget.c
> +++ b/drivers/usb/cdns3/cdns3-gadget.c
> @@ -63,8 +63,8 @@
>
> #include "core.h"
> #include "gadget-export.h"
> -#include "gadget.h"
> -#include "trace.h"
> +#include "cdns3-gadget.h"
> +#include "cdns3-trace.h"
> #include "drd.h"
>
> static int __cdns3_gadget_ep_queue(struct usb_ep *ep,
> diff --git a/drivers/usb/cdns3/gadget.h b/drivers/usb/cdns3/cdns3-gadget.h
> similarity index 100%
> rename from drivers/usb/cdns3/gadget.h
> rename to drivers/usb/cdns3/cdns3-gadget.h
> diff --git a/drivers/usb/cdns3/trace.c b/drivers/usb/cdns3/cdns3-trace.c
> similarity index 89%
> rename from drivers/usb/cdns3/trace.c
> rename to drivers/usb/cdns3/cdns3-trace.c
> index 459fa72d9c74..b9858acaef02 100644
> --- a/drivers/usb/cdns3/trace.c
> +++ b/drivers/usb/cdns3/cdns3-trace.c
> @@ -8,4 +8,4 @@
> */
>
> #define CREATE_TRACE_POINTS
> -#include "trace.h"
> +#include "cdns3-trace.h"
> diff --git a/drivers/usb/cdns3/trace.h b/drivers/usb/cdns3/cdns3-trace.h
> similarity index 99%
> rename from drivers/usb/cdns3/trace.h
> rename to drivers/usb/cdns3/cdns3-trace.h
> index 0a2a3269bfac..8648c7a7a9dd 100644
> --- a/drivers/usb/cdns3/trace.h
> +++ b/drivers/usb/cdns3/cdns3-trace.h
> @@ -19,8 +19,8 @@
> #include
> #include
> #include "core.h"
> -#include "gadget.h"
> -#include "debug.h"
> +#include "cdns3-gadget.h"
> +#include "cdns3-debug.h"
>
> #define CDNS3_MSG_MAX500
>
> @@ -565,6 +565,6 @@ DEFINE_EVENT(cdns3_log_request_handled,
> cdns3_request_handled,
> #define TRACE_INCLUDE_PATH .
>
> #undef TRACE_INCLUDE_FILE
> -#define TRACE_INCLUDE_FILE trace
> +#define TRACE_INCLUDE_FILE cdns3-trace
>
> #include
> --
> 2.17.1
>
--
Thanks,
Peter Chen
Re: [PATCH v2 06/10] usb: cdnsp: Device side header file for CDNSP driver
On 20-11-06 12:42:56, Pawel Laszczak wrote:
> Patch defines macros, registers and structures used by
> Device side driver.
>
> Because the size of main patch is very big, I’ve decided to create
> separate patch for cdnsp-gadget.h. It should simplify reviewing the code.
>
> Signed-off-by: Pawel Laszczak
> ---
> drivers/usb/cdns3/cdnsp-gadget.h | 1463 ++
> 1 file changed, 1463 insertions(+)
> create mode 100644 drivers/usb/cdns3/cdnsp-gadget.h
>
> diff --git a/drivers/usb/cdns3/cdnsp-gadget.h
> b/drivers/usb/cdns3/cdnsp-gadget.h
> new file mode 100644
> index ..29d5e2d00879
> --- /dev/null
> +++ b/drivers/usb/cdns3/cdnsp-gadget.h
> @@ -0,0 +1,1463 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Cadence CDNSP DRD Driver.
> + *
> + * Copyright (C) 2020 Cadence.
> + *
> + * Author: Pawel Laszczak
> + *
> + * Code based on Linux XHCI driver.
> + * Origin: Copyright (C) 2008 Intel Corp.
> + */
> +#ifndef __LINUX_CDNSP_GADGET_H
> +#define __LINUX_CDNSP_GADGET_H
> +
> +#include
> +#include
> +#include
> +
> +/* Max number slots - only 1 is allowed. */
> +#define CDNSP_DEV_MAX_SLOTS 1
> +
> +#define CDNSP_EP0_SETUP_SIZE 512
> +
> +/*16 for in and 16 for out. */
> +#define CDNSP_ENDPOINTS_NUM 32
> +
> +/* Best Effort Service Latency. */
> +#define CDNSP_DEFAULT_BESL 0
> +
> +/* Device Controller command default timeout value in us */
> +#define CDNSP_CMD_TIMEOUT(15 * 1000)
> +
> +/* Up to 16 ms to halt an device controller */
> +#define CDNSP_MAX_HALT_USEC (16 * 1000)
> +
> +#define CDNSP_CTX_SIZE 2112
> +
> +/*
> + * Controller register interface.
> + */
> +
> +/**
> + * struct cdnsp_cap_regs - CDNSP Registers.
> + * @hc_capbase: Length of the capabilities register and controller
> + * version number
> + * @hcs_params1: HCSPARAMS1 - Structural Parameters 1
> + * @hcs_params2: HCSPARAMS2 - Structural Parameters 2
> + * @hcs_params3: HCSPARAMS3 - Structural Parameters 3
> + * @hcc_params: HCCPARAMS - Capability Parameters
> + * @db_off: DBOFF - Doorbell array offset
> + * @run_regs_off: RTSOFF - Runtime register space offset
> + * @hcc_params2: HCCPARAMS2 Capability Parameters 2,
> + */
> +struct cdnsp_cap_regs {
> + __le32 hc_capbase;
> + __le32 hcs_params1;
> + __le32 hcs_params2;
> + __le32 hcs_params3;
> + __le32 hcc_params;
> + __le32 db_off;
> + __le32 run_regs_off;
> + __le32 hcc_params2;
> + /* Reserved up to (CAPLENGTH - 0x1C) */
> +};
> +
> +/* hc_capbase bitmasks. */
> +/* bits 7:0 - how long is the Capabilities register. */
> +#define HC_LENGTH(p) (((p) >> 00) & GENMASK(7, 0))
> +/* bits 31:16*/
> +#define HC_VERSION(p)(((p) >> 16) & GENMASK(15, 1))
> +
> +/* HCSPARAMS1 - hcs_params1 - bitmasks */
> +/* bits 0:7, Max Device Endpoints */
> +#define HCS_ENDPOINTS_MASK GENMASK(7, 0)
> +#define HCS_ENDPOINTS(p) (((p) & HCS_ENDPOINTS_MASK) >> 0)
> +
> +/* HCCPARAMS offset from PCI base address */
> +#define HCC_PARAMS_OFFSET0x10
> +
> +/* HCCPARAMS - hcc_params - bitmasks */
> +/* true: device controller can use 64-bit address pointers. */
What does "true" mean at above comment?
> +#define HCC_64BIT_ADDR(p)((p) & BIT(0))
> +/* true: device controller uses 64-byte Device Context structures. */
ditto
> +#define HCC_64BYTE_CONTEXT(p)((p) & BIT(2))
> +/* Max size for Primary Stream Arrays - 2^(n+1), where n is bits 12:15. */
> +#define HCC_MAX_PSA(p) p) >> 12) & 0xf) + 1)
> +/* Extended Capabilities pointer from PCI base. */
> +#define HCC_EXT_CAPS(p) (((p) & GENMASK(31, 16)) >> 16)
> +
> +#define CTX_SIZE(_hcc) (HCC_64BYTE_CONTEXT(_hcc) ? 64 : 32)
> +
> +/* db_off bitmask - bits 0:1 reserved. */
> +#define DBOFF_MASK GENMASK(31, 2)
> +
> +/* run_regs_off bitmask - bits 0:4 reserved. */
> +#define RTSOFF_MASK GENMASK(31, 5)
> +
> +/**
> + * struct cdnsp_op_regs - Device Controller Operational Registers.
> + * @command: USBCMD - Controller command register.
> + * @status: USBSTS - Controller status register.
> + * @page_size: This indicates the page size that the device controller
> supports.
> + * If bit n is set, the controller supports a page size of
> 2^(n+12),
> + * up to a 128MB page size. 4K is the minimum page size.
> + * @dnctrl: DNCTRL - Device notification control register.
> + * @cmd_ring: CRP - 64-bit Command Ring Pointer.
> + * @dcbaa_ptr: DCBAAP - 64-bit Device Context Base Address Array Pointer.
> + * @config_reg: CONFIG - Configure Register
> + * @port_reg_base: PORTSCn - base address for Port Status and Control
> + * Each port has a Port Status and Control register,
> + * followed by a Port Power Management Status and Control
> + * register, a Port Link Info register, and a reserved
> + * register.
> + */
> +struct cdnsp_op_regs {
> + __le32 command;
> + __le32 status;
Re: [PATCH v2 03/10] usb: cdns3: Moves reusable code to separate module
On 20-11-10 09:20:54, Pawel Laszczak wrote: > Hi, > > >> > >> int cdns3_hw_role_switch(struct cdns3 *cdns); > >> -int cdns3_init(struct cdns3 *cdns); > >> -int cdns3_remove(struct cdns3 *cdns); > >> +extern int cdns3_init(struct cdns3 *cdns); > >> +extern int cdns3_remove(struct cdns3 *cdns); > > > >Why add "extern" here and below? > > > > These functions are the API between cdnsp and cdns3 modules. > It's looks like a common approach in kernel. > Many or even most of API function in kernel has "extern". > Even you have not written "extern" keyword, the "extern" is added implicitly by compiler. Usually, we use "extern" for variable or the function is defined at assembly. You could see some "extern" keyword use cases at include/linux/device.h. Never mind, it is not a issue. Peter > Of course, here we have little different situation because these API functions > are limited only to cdns3 directory. > > was not sure about that, but I think that this extern is the > information that these functions are used, or can be used > by other modules. > > Am I right ? > > >> > >> #ifdef CONFIG_PM_SLEEP > >> -int cdns3_resume(struct cdns3 *cdns, u8 set_active); > >> -int cdns3_suspend(struct cdns3 *cdns); > >> +extern int cdns3_resume(struct cdns3 *cdns, u8 set_active); > >> +extern int cdns3_suspend(struct cdns3 *cdns); > >> #endif /* CONFIG_PM_SLEEP */ > >> #endif /* __LINUX_CDNS3_CORE_H */ > >> diff --git a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c > >> index ed8cde91a02c..1874dc6018f0 100644 > >> --- a/drivers/usb/cdns3/drd.c > >> +++ b/drivers/usb/cdns3/drd.c > >> @@ -15,7 +15,6 @@ > >> #include > >> #include > >> > >> -#include "gadget.h" > >> #include "drd.h" > >> #include "core.h" > >> > >> @@ -226,6 +225,7 @@ int cdns3_drd_gadget_on(struct cdns3 *cdns) > >>phy_set_mode(cdns->usb3_phy, PHY_MODE_USB_DEVICE); > >>return 0; > >> } > >> +EXPORT_SYMBOL_GPL(cdns3_drd_gadget_on); > >> > >> /** > >> * cdns3_drd_gadget_off - stop gadget. > >> @@ -249,6 +249,7 @@ void cdns3_drd_gadget_off(struct cdns3 *cdns) > >> 1, 200); > >>phy_set_mode(cdns->usb3_phy, PHY_MODE_INVALID); > >> } > >> +EXPORT_SYMBOL_GPL(cdns3_drd_gadget_off); > >> > >> /** > >> * cdns3_init_otg_mode - initialize drd controller > >> diff --git a/drivers/usb/cdns3/drd.h b/drivers/usb/cdns3/drd.h > >> index d752d8806a38..972aba8a40b6 100644 > >> --- a/drivers/usb/cdns3/drd.h > >> +++ b/drivers/usb/cdns3/drd.h > >> @@ -209,8 +209,8 @@ int cdns3_get_vbus(struct cdns3 *cdns); > >> int cdns3_drd_init(struct cdns3 *cdns); > >> int cdns3_drd_exit(struct cdns3 *cdns); > >> int cdns3_drd_update_mode(struct cdns3 *cdns); > >> -int cdns3_drd_gadget_on(struct cdns3 *cdns); > >> -void cdns3_drd_gadget_off(struct cdns3 *cdns); > >> +extern int cdns3_drd_gadget_on(struct cdns3 *cdns); > >> +extern void cdns3_drd_gadget_off(struct cdns3 *cdns); > >> int cdns3_drd_host_on(struct cdns3 *cdns); > >> void cdns3_drd_host_off(struct cdns3 *cdns); > >> > >> -- > >> 2.17.1 > >> > > -- > Thanks > Pawel Laszczak -- Thanks, Peter Chen
Re: [PATCH v2 03/10] usb: cdns3: Moves reusable code to separate module
int cdns3_remove(struct cdns3 *cdns)
>
> return 0;
> }
> +EXPORT_SYMBOL_GPL(cdns3_remove);
>
> #ifdef CONFIG_PM_SLEEP
> int cdns3_suspend(struct cdns3 *cdns)
> @@ -505,6 +509,7 @@ int cdns3_suspend(struct cdns3 *cdns)
>
> return 0;
> }
> +EXPORT_SYMBOL_GPL(cdns3_suspend);
>
> int cdns3_resume(struct cdns3 *cdns, u8 set_active)
> {
> @@ -521,4 +526,11 @@ int cdns3_resume(struct cdns3 *cdns, u8 set_active)
>
> return 0;
> }
> +EXPORT_SYMBOL_GPL(cdns3_resume);
> #endif /* CONFIG_PM_SLEEP */
> +
> +MODULE_AUTHOR("Peter Chen ");
> +MODULE_AUTHOR("Pawel Laszczak ");
> +MODULE_AUTHOR("Roger Quadros ");
> +MODULE_DESCRIPTION("Cadence USBSS and USBSSP DRD Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
> index 7e5d9a344a53..96bdab7e8357 100644
> --- a/drivers/usb/cdns3/core.h
> +++ b/drivers/usb/cdns3/core.h
> @@ -75,6 +75,7 @@ struct cdns3_platform_data {
> * @wakeup_pending: wakeup interrupt pending
> * @pdata: platform data from glue layer
> * @lock: spinlock structure
> + * @gadget_init: pointer to gadget initialization function
> */
> struct cdns3 {
> struct device *dev;
> @@ -111,14 +112,16 @@ struct cdns3 {
> boolwakeup_pending;
> struct cdns3_platform_data *pdata;
> spinlock_t lock;
> +
> + int (*gadget_init)(struct cdns3 *cdns);
> };
>
> int cdns3_hw_role_switch(struct cdns3 *cdns);
> -int cdns3_init(struct cdns3 *cdns);
> -int cdns3_remove(struct cdns3 *cdns);
> +extern int cdns3_init(struct cdns3 *cdns);
> +extern int cdns3_remove(struct cdns3 *cdns);
Why add "extern" here and below?
Peter
>
> #ifdef CONFIG_PM_SLEEP
> -int cdns3_resume(struct cdns3 *cdns, u8 set_active);
> -int cdns3_suspend(struct cdns3 *cdns);
> +extern int cdns3_resume(struct cdns3 *cdns, u8 set_active);
> +extern int cdns3_suspend(struct cdns3 *cdns);
> #endif /* CONFIG_PM_SLEEP */
> #endif /* __LINUX_CDNS3_CORE_H */
> diff --git a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c
> index ed8cde91a02c..1874dc6018f0 100644
> --- a/drivers/usb/cdns3/drd.c
> +++ b/drivers/usb/cdns3/drd.c
> @@ -15,7 +15,6 @@
> #include
> #include
>
> -#include "gadget.h"
> #include "drd.h"
> #include "core.h"
>
> @@ -226,6 +225,7 @@ int cdns3_drd_gadget_on(struct cdns3 *cdns)
> phy_set_mode(cdns->usb3_phy, PHY_MODE_USB_DEVICE);
> return 0;
> }
> +EXPORT_SYMBOL_GPL(cdns3_drd_gadget_on);
>
> /**
> * cdns3_drd_gadget_off - stop gadget.
> @@ -249,6 +249,7 @@ void cdns3_drd_gadget_off(struct cdns3 *cdns)
> 1, 200);
> phy_set_mode(cdns->usb3_phy, PHY_MODE_INVALID);
> }
> +EXPORT_SYMBOL_GPL(cdns3_drd_gadget_off);
>
> /**
> * cdns3_init_otg_mode - initialize drd controller
> diff --git a/drivers/usb/cdns3/drd.h b/drivers/usb/cdns3/drd.h
> index d752d8806a38..972aba8a40b6 100644
> --- a/drivers/usb/cdns3/drd.h
> +++ b/drivers/usb/cdns3/drd.h
> @@ -209,8 +209,8 @@ int cdns3_get_vbus(struct cdns3 *cdns);
> int cdns3_drd_init(struct cdns3 *cdns);
> int cdns3_drd_exit(struct cdns3 *cdns);
> int cdns3_drd_update_mode(struct cdns3 *cdns);
> -int cdns3_drd_gadget_on(struct cdns3 *cdns);
> -void cdns3_drd_gadget_off(struct cdns3 *cdns);
> +extern int cdns3_drd_gadget_on(struct cdns3 *cdns);
> +extern void cdns3_drd_gadget_off(struct cdns3 *cdns);
> int cdns3_drd_host_on(struct cdns3 *cdns);
> void cdns3_drd_host_off(struct cdns3 *cdns);
>
> --
> 2.17.1
>
--
Thanks,
Peter Chen
Re: [PATCH v2 01/10] usb: cdns3: Add support for DRD CDNSP
On 20-11-06 12:42:51, Pawel Laszczak wrote:
> - * Value of the strap pins.
> + * Value of the strap pins for:
> + * CDN3:
%/CDN3/CDNS3
Peter
> * 000 - no default configuration
> * 010 - Controller initiall configured as Host
> * 100 - Controller initially configured as Device
> + * CDNSP:
> + * 000 - No default configuration.
> + * 010 - Controller initiall configured as Host.
> + * 100 - Controller initially configured as Device.
> */
> #define OTGSTS_STRAP(p) (((p) & GENMASK(14, 12)) >> 12)
> #define OTGSTS_STRAP_NO_DEFAULT_CFG 0x00
> #define OTGSTS_STRAP_HOST_OTG0x01
> #define OTGSTS_STRAP_HOST0x02
> #define OTGSTS_STRAP_GADGET 0x04
> +#define OTGSTS_CDNSP_STRAP_HOST 0x01
> +#define OTGSTS_CDNSP_STRAP_GADGET0x02
> +
> /* Host mode is turned on. */
> -#define OTGSTS_XHCI_READYBIT(26)
> +#define OTGSTS_CDNS3_XHCI_READY BIT(26)
> +#define OTGSTS_CDNSP_XHCI_READY BIT(27)
> +
> /* "Device mode is turned on .*/
> -#define OTGSTS_DEV_READY BIT(27)
> +#define OTGSTS_CDNS3_DEV_READY BIT(27)
> +#define OTGSTS_CDNSP_DEV_READY BIT(26)
>
> /* OTGSTATE- bitmasks */
> #define OTGSTATE_DEV_STATE_MASK GENMASK(2, 0)
> @@ -152,6 +194,8 @@ struct cdns3_otg_common_regs {
> #define OVERRIDE_IDPULLUPBIT(0)
> /* Only for CDNS3_CONTROLLER_V0 version */
> #define OVERRIDE_IDPULLUP_V0 BIT(24)
> +/* Vbusvalid/Sesvalid override select. */
> +#define OVERRIDE_SESS_VLD_SELBIT(10)
>
> /* PHYRST_CFG - bitmasks */
> #define PHYRST_CFG_PHYRST_A_ENABLE BIT(0)
> @@ -170,6 +214,5 @@ int cdns3_drd_gadget_on(struct cdns3 *cdns);
> void cdns3_drd_gadget_off(struct cdns3 *cdns);
> int cdns3_drd_host_on(struct cdns3 *cdns);
> void cdns3_drd_host_off(struct cdns3 *cdns);
> -int cdns3_set_mode(struct cdns3 *cdns, enum usb_dr_mode mode);
>
> #endif /* __LINUX_CDNS3_DRD */
> --
> 2.17.1
>
--
Thanks,
Peter Chen
Re: [PATCH] Fix default q_len for usb_f_printer gadget driver
On 20-10-30 18:34:19, Michael R Sweet wrote:
> The usb_f_printer gadget driver uses a default q_len value of *0* which
> prevents
> any IO from occurring. Moreover, once the driver is instantiated it is
> impossible to change the q_len value.
>
> The following patch uses a default q_len value of 10 which matches the legacy
> g_printer gadget driver. This minimizes the possibility that you end up with
> a
> non-working printer gadget. It is still possible to set the q_len to a
> different value using the configfs path of the same name.
>
> Signed-off-by: Michael R Sweet
Reviewed-by: Peter Chen
Peter
> ---
> drivers/usb/gadget/function/f_printer.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/usb/gadget/function/f_printer.c
> b/drivers/usb/gadget/function/f_printer.c
> index 9c7ed2539ff7..4f3161005e4f 100644
> --- a/drivers/usb/gadget/function/f_printer.c
> +++ b/drivers/usb/gadget/function/f_printer.c
> @@ -50,6 +50,8 @@
> #define GET_PORT_STATUS 1
> #define SOFT_RESET 2
>
> +#define DEFAULT_Q_LEN10 /* same as legacy g_printer gadget */
> +
> static int major, minors;
> static struct class *usb_gadget_class;
> static DEFINE_IDA(printer_ida);
> @@ -1317,6 +1319,9 @@ static struct usb_function_instance
> *gprinter_alloc_inst(void)
> opts->func_inst.free_func_inst = gprinter_free_inst;
> ret = >func_inst;
>
> + /* Make sure q_len is initialized, otherwise the bound device can't
> support read/write! */
> + opts->q_len = DEFAULT_Q_LEN;
> +
> mutex_lock(_ida_lock);
>
> if (ida_is_empty(_ida)) {
> --
> 2.17.1
>
--
Thanks,
Peter Chen
RE: [PATCH v2 31/39] docs: ABI: cleanup several ABI documents
Acked-by: Peter Chen For: Documentation/ABI/testing/usb-charger-uevent Peter
Re: [PATCH 05/17] phy: freescale: convert to devm_platform_ioremap_resource
On 20-10-29 10:54:27, Chunfeng Yun wrote: > Use devm_platform_ioremap_resource to simplify code > > Signed-off-by: Chunfeng Yun > --- > drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c | 4 +--- > drivers/phy/freescale/phy-fsl-imx8mq-usb.c | 4 +--- > 2 files changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c > b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c > index 9f2c1da14f5a..a95572b397ca 100644 > --- a/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c > +++ b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c > @@ -434,7 +434,6 @@ static int mixel_dphy_probe(struct platform_device *pdev) > struct device_node *np = dev->of_node; > struct phy_provider *phy_provider; > struct mixel_dphy_priv *priv; > - struct resource *res; > struct phy *phy; > void __iomem *base; > > @@ -449,8 +448,7 @@ static int mixel_dphy_probe(struct platform_device *pdev) > if (!priv->devdata) > return -EINVAL; > > - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - base = devm_ioremap_resource(dev, res); > + base = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(base)) > return PTR_ERR(base); > > diff --git a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c > b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c > index 62d6d6849ad6..0b9ee2b1716f 100644 > --- a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c > +++ b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c > @@ -152,7 +152,6 @@ static int imx8mq_usb_phy_probe(struct platform_device > *pdev) > struct phy_provider *phy_provider; > struct device *dev = >dev; > struct imx8mq_usb_phy *imx_phy; > - struct resource *res; > const struct phy_ops *phy_ops; > > imx_phy = devm_kzalloc(dev, sizeof(*imx_phy), GFP_KERNEL); > @@ -165,8 +164,7 @@ static int imx8mq_usb_phy_probe(struct platform_device > *pdev) > return PTR_ERR(imx_phy->clk); > } > > - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - imx_phy->base = devm_ioremap_resource(dev, res); > + imx_phy->base = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(imx_phy->base)) > return PTR_ERR(imx_phy->base); > Reviewed-by: Peter Chen -- Thanks, Peter Chen
Re: [PATCH 04/17] phy: cadence: convert to devm_platform_ioremap_resource
On 20-10-29 10:54:26, Chunfeng Yun wrote:
> Use devm_platform_ioremap_resource to simplify code
>
> Signed-off-by: Chunfeng Yun
> ---
> drivers/phy/cadence/cdns-dphy.c | 4 +---
> drivers/phy/cadence/phy-cadence-salvo.c | 4 +---
> drivers/phy/cadence/phy-cadence-sierra.c | 4 +---
> 3 files changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c
> index 90c4e9b5aac8..ba042e39cfaf 100644
> --- a/drivers/phy/cadence/cdns-dphy.c
> +++ b/drivers/phy/cadence/cdns-dphy.c
> @@ -314,7 +314,6 @@ static int cdns_dphy_probe(struct platform_device *pdev)
> {
> struct phy_provider *phy_provider;
> struct cdns_dphy *dphy;
> - struct resource *res;
> int ret;
>
> dphy = devm_kzalloc(>dev, sizeof(*dphy), GFP_KERNEL);
> @@ -326,8 +325,7 @@ static int cdns_dphy_probe(struct platform_device *pdev)
> if (!dphy->ops)
> return -EINVAL;
>
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - dphy->regs = devm_ioremap_resource(>dev, res);
> + dphy->regs = devm_platform_ioremap_resource(pdev, 0);
> if (IS_ERR(dphy->regs))
> return PTR_ERR(dphy->regs);
>
> diff --git a/drivers/phy/cadence/phy-cadence-salvo.c
> b/drivers/phy/cadence/phy-cadence-salvo.c
> index 88e239adc3b8..51c0b98f5fd7 100644
> --- a/drivers/phy/cadence/phy-cadence-salvo.c
> +++ b/drivers/phy/cadence/phy-cadence-salvo.c
> @@ -263,7 +263,6 @@ static int cdns_salvo_phy_probe(struct platform_device
> *pdev)
> struct phy_provider *phy_provider;
> struct device *dev = >dev;
> struct cdns_salvo_phy *salvo_phy;
> - struct resource *res;
> const struct of_device_id *match;
> struct cdns_salvo_data *data;
>
> @@ -281,8 +280,7 @@ static int cdns_salvo_phy_probe(struct platform_device
> *pdev)
> if (IS_ERR(salvo_phy->clk))
> return PTR_ERR(salvo_phy->clk);
>
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - salvo_phy->base = devm_ioremap_resource(dev, res);
> + salvo_phy->base = devm_platform_ioremap_resource(pdev, 0);
> if (IS_ERR(salvo_phy->base))
> return PTR_ERR(salvo_phy->base);
>
> diff --git a/drivers/phy/cadence/phy-cadence-sierra.c
> b/drivers/phy/cadence/phy-cadence-sierra.c
> index 453ef26fa1c7..26a0badabe38 100644
> --- a/drivers/phy/cadence/phy-cadence-sierra.c
> +++ b/drivers/phy/cadence/phy-cadence-sierra.c
> @@ -479,7 +479,6 @@ static int cdns_sierra_phy_probe(struct platform_device
> *pdev)
> const struct of_device_id *match;
> struct cdns_sierra_data *data;
> unsigned int id_value;
> - struct resource *res;
> int i, ret, node = 0;
> void __iomem *base;
> struct clk *clk;
> @@ -502,8 +501,7 @@ static int cdns_sierra_phy_probe(struct platform_device
> *pdev)
> sp->dev = dev;
> sp->init_data = data;
>
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - base = devm_ioremap_resource(dev, res);
> + base = devm_platform_ioremap_resource(pdev, 0);
> if (IS_ERR(base)) {
> dev_err(dev, "missing \"reg\"\n");
> return PTR_ERR(base);
> --
> 2.18.0
Acked-by: Peter Chen for phy-cadence-salvo.c.
--
Thanks,
Peter Chen
RE: [PATCH v3] usb: cdns3: Variable 'length' set but not used
> Peter, > > It looks like you missed the " [PATCH v3] usb: cdns3: Variable 'length' set > but > not used" > > It's quite important because compiler complains for this when I use W=1. > Pawel, it is the bug-fix, and located at branch: for-usb-fixes. > Thanks, > Pawel > > >> > > >> > A gentle ping. > >> > > >> > I assume that you should add this and the rest overdue cdsn3 > >> > patches as first to you ci-for-usb-next branch. > >> > Am I right? > >> > > >> > >> Hi Pawel, > >> > >> I queued them locally, and I waited for v5.10-rc1 which was out > >> yesterday, then I will apply them, and add cdns3 patches to my > >> kernel.org branch. Will update you these two days. > >> > >> Peter > > > >Hi Pawel, > > > >The cdns3 -next patches pushed to: for-usb-next; cdns3 -fixes patches pushed > to: for-usb-fixes. > >The git is: > >git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git > > > >Currently, I only pushed three of your patches, would you please review my > patches, thanks. > > > >Peter
RE: [PATCH v3] usb: cdns3: Variable 'length' set but not used
> > > > >> Peter, > >> > >> It looks like you missed the " [PATCH v3] usb: cdns3: Variable > >> 'length' set but not used" > >> > >> It's quite important because compiler complains for this when I use W=1. > >> > > > >Pawel, it is the bug-fix, and located at branch: for-usb-fixes. > > But I can't see it in this branch: > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kern > el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fpeter.chen%2Fusb.git%2Flo > g%2F%3Fh%3Dfor-usb-fixes%26qt%3Dgrep%26q%3Dcdns3data=04%7C > 01%7Cpeter.chen%40nxp.com%7Cc49c80ded3ec4bae70aa08d87b120467%7C > 686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637394664604902329%7 > CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBT > iI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=jtcT0U8ZptlBSwST00%2B > bMtCQDZDr%2B3PNMm69RrZWgDs%3Dreserved=0 > > I can see there only: usb: cdns3: Rids of duplicate error message. > Thanks for notifying it, I have updated it. Peter > > > >> Thanks, > >> Pawel > >> > >> >> > > >> >> > A gentle ping. > >> >> > > >> >> > I assume that you should add this and the rest overdue cdsn3 > >> >> > patches as first to you ci-for-usb-next branch. > >> >> > Am I right? > >> >> > > >> >> > >> >> Hi Pawel, > >> >> > >> >> I queued them locally, and I waited for v5.10-rc1 which was out > >> >> yesterday, then I will apply them, and add cdns3 patches to my > >> >> kernel.org branch. Will update you these two days. > >> >> > >> >> Peter > >> > > >> >Hi Pawel, > >> > > >> >The cdns3 -next patches pushed to: for-usb-next; cdns3 -fixes > >> >patches pushed > >> to: for-usb-fixes. > >> >The git is: > >> >git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git > >> > > >> >Currently, I only pushed three of your patches, would you please > >> >review my > >> patches, thanks. > >> > > >> >Peter
RE: [PATCH v3] usb: cdns3: Variable 'length' set but not used
> > > > A gentle ping. > > > > I assume that you should add this and the rest overdue cdsn3 patches > > as first to you ci-for-usb-next branch. > > Am I right? > > > > Hi Pawel, > > I queued them locally, and I waited for v5.10-rc1 which was out yesterday, > then > I will apply them, and add cdns3 patches to my kernel.org branch. Will update > you these two days. > > Peter Hi Pawel, The cdns3 -next patches pushed to: for-usb-next; cdns3 -fixes patches pushed to: for-usb-fixes. The git is: git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git Currently, I only pushed three of your patches, would you please review my patches, thanks. Peter
RE: [PATCH v3] usb: cdns3: Variable 'length' set but not used
> > A gentle ping. > > I assume that you should add this and the rest overdue cdsn3 patches as first > to you ci-for-usb-next branch. > Am I right? > Hi Pawel, I queued them locally, and I waited for v5.10-rc1 which was out yesterday, then I will apply them, and add cdns3 patches to my kernel.org branch. Will update you these two days. Peter
RE: [PATCH] usb: gadget: fsl: fix null pointer checking
> > Fixes: 75eaa498c99e (“usb: gadget: Correct NULL pointer checking in fsl > gadget”) > Signed-off-by: Ran Wang > --- > drivers/usb/gadget/udc/fsl_udc_core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/gadget/udc/fsl_udc_core.c > b/drivers/usb/gadget/udc/fsl_udc_core.c > index de528e3..ad6ff9c 100644 > --- a/drivers/usb/gadget/udc/fsl_udc_core.c > +++ b/drivers/usb/gadget/udc/fsl_udc_core.c > @@ -1051,7 +1051,7 @@ static int fsl_ep_fifo_status(struct usb_ep *_ep) > u32 bitmask; > struct ep_queue_head *qh; > > - if (!_ep || _ep->desc || !(_ep->desc->bEndpointAddress&0xF)) > + if (!_ep || !_ep->desc || !(_ep->desc->bEndpointAddress&0xF)) > return -ENODEV; > Reviewed-by: Peter Chen Peter
RE: [PATCH v3] usb: cdns3: Rids of duplicate error message
> On failure, the platform_get_irq_byname prints an error message so, patch
typo, "message, so patch..." Otherwise:
Acked-by: Peter Chen
Peter
> removes error message related to this function from core.c file.
>
> A change was suggested during reviewing CDNSP driver by Chunfeng Yun.
>
> Signed-off-by: Pawel Laszczak
> ---
> Changelog:
> v3
> - changed error condition checking for dev_irq.
> v2
> - simplified code as sugested by Roger Quadros.
>
> drivers/usb/cdns3/core.c | 12 ++--
> 1 file changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c index
> a0f73d4711ae..f2dedce3a40e 100644
> --- a/drivers/usb/cdns3/core.c
> +++ b/drivers/usb/cdns3/core.c
> @@ -466,11 +466,8 @@ static int cdns3_probe(struct platform_device *pdev)
> cdns->xhci_res[1] = *res;
>
> cdns->dev_irq = platform_get_irq_byname(pdev, "peripheral");
> - if (cdns->dev_irq == -EPROBE_DEFER)
> - return cdns->dev_irq;
> -
> if (cdns->dev_irq < 0)
> - dev_err(dev, "couldn't get peripheral irq\n");
> + return cdns->dev_irq;
>
> regs = devm_platform_ioremap_resource_byname(pdev, "dev");
> if (IS_ERR(regs))
> @@ -478,14 +475,9 @@ static int cdns3_probe(struct platform_device *pdev)
> cdns->dev_regs = regs;
>
> cdns->otg_irq = platform_get_irq_byname(pdev, "otg");
> - if (cdns->otg_irq == -EPROBE_DEFER)
> + if (cdns->otg_irq < 0)
> return cdns->otg_irq;
>
> - if (cdns->otg_irq < 0) {
> - dev_err(dev, "couldn't get otg irq\n");
> - return cdns->otg_irq;
> - }
> -
> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "otg");
> if (!res) {
> dev_err(dev, "couldn't get otg resource\n");
> --
> 2.17.1
Re: [PATCH v2] usb: cdns3: Rids of duplicate error message
On 20-10-12 14:38:16, Roger Quadros wrote:
> Pawel,
>
> On 12/10/2020 14:12, Pawel Laszczak wrote:
> > Hi Roger,
> >
> > On 12/10/2020 09:42, Pawel Laszczak wrote:
> > > On failure, the platform_get_irq_byname prints an error message
> > > so, patch removes error message related to this function from
> > > core.c file.
> > >
> > > A change was suggested during reviewing CDNSP driver by Chunfeng Yun.
> > >
> > > Signed-off-by: Pawel Laszczak
> > > ---
> > > Changelog:
> > > v2
> > > - simplified code as sugested by Roger Quadros.
> > >
> > >drivers/usb/cdns3/core.c | 10 +-
> > >1 file changed, 1 insertion(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> > > index a0f73d4711ae..85ef3025b293 100644
> > > --- a/drivers/usb/cdns3/core.c
> > > +++ b/drivers/usb/cdns3/core.c
> > > @@ -469,22 +469,14 @@ static int cdns3_probe(struct platform_device *pdev)
> > > if (cdns->dev_irq == -EPROBE_DEFER)
> >
> > Shouldn't this be
> > if (cdns->dev_irq < 0)
> > ?
> >
> > No, such line will change the original behavior of driver.
> >
> > Current patch allows to run driver when we support only host.
> > In such case the dev_irq can be < 0 and we still can use host side.
>
> In that case should we check if we need dev_irq and if so then error out.
> i.e. if mode is "peripheral" or "otg"
>
> Also DT binding document says all 3 IRQs are mandatory. Do we need to update
> that?
>
I agree with Roger, or you change the driver behaviour that return error
if the dev_irq is < 0.
Peter
>
> >
> > Regards,
> > Pawel Laszczak
> >
> > > return cdns->dev_irq;
> > > - if (cdns->dev_irq < 0)
> > > - dev_err(dev, "couldn't get peripheral irq\n");
> > > -
> > > regs = devm_platform_ioremap_resource_byname(pdev, "dev");
> > > if (IS_ERR(regs))
> > > return PTR_ERR(regs);
> > > cdns->dev_regs = regs;
> > > cdns->otg_irq = platform_get_irq_byname(pdev, "otg");
> > > - if (cdns->otg_irq == -EPROBE_DEFER)
> > > - return cdns->otg_irq;
> > > -
> > > - if (cdns->otg_irq < 0) {
> > > - dev_err(dev, "couldn't get otg irq\n");
> > > + if (cdns->otg_irq < 0)
> > > return cdns->otg_irq;
> > > - }
> > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "otg");
> > > if (!res) {
> > >
> >
>
> --
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
--
Thanks,
Peter Chen
