RE: [RFC PATCH v2 11/15] usb:cdns3: Implements ISR functionality.
From: Pawel Laszczak
Date: Sun Dec 02 2018 - 06:50:00 EST
Hi,
>> Patch adds set of generic functions used for handling interrupts
>> generated by controller. Interrupt related functions are divided
>> into three groups. The first is related to ep0 and is placed in ep0.c.
>> The second is responsible for non-default endpoints and is
>> implemented in gadget.c file. The last group is not related to
>> endpoints interrupts and is placed in gadget.c.
>> All groups have common entry point in cdns3_irq_handler_thread function.
>>
>> Signed-off-by: Pawel Laszczak <pawell@xxxxxxxxxxx>
>> ---
>> drivers/usb/cdns3/ep0.c | 63 +++++++++++
>> drivers/usb/cdns3/gadget.c | 224 ++++++++++++++++++++++++++++++++++++-
>> drivers/usb/cdns3/gadget.h | 1 +
>> 3 files changed, 287 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/cdns3/ep0.c b/drivers/usb/cdns3/ep0.c
>> index d05169e73631..eb92fd234bd7 100644
>> --- a/drivers/usb/cdns3/ep0.c
>> +++ b/drivers/usb/cdns3/ep0.c
>> @@ -90,6 +90,69 @@ static void cdns3_set_hw_configuration(struct cdns3_device *priv_dev)
>> }
>> }
>>
>> +static void __pending_setup_status_handler(struct cdns3_device *priv_dev)
>> +{
>> + //TODO: Implements this function
>> +}
>> +
>> +/**
>> + * cdns3_ep0_setup_phase - Handling setup USB requests
>> + * @priv_dev: extended gadget object
>> + */
>> +static void cdns3_ep0_setup_phase(struct cdns3_device *priv_dev)
>> +{
>> + //TODO: Implements this function.
>> +}
>> +
>> +static void cdns3_transfer_completed(struct cdns3_device *priv_dev)
>> +{
>> + //TODO: Implements this function
>> +}
>> +
>> +/**
>> + * cdns3_check_ep0_interrupt_proceed - Processes interrupt related to endpoint 0
>> + * @priv_dev: extended gadget object
>> + * @dir: 1 for IN direction, 0 for OUT direction
>> + */
>> +void cdns3_check_ep0_interrupt_proceed(struct cdns3_device *priv_dev, int dir)
>> +{
>> + struct cdns3_usb_regs __iomem *regs = priv_dev->regs;
>> + u32 ep_sts_reg;
>> +
>> + cdns3_select_ep(priv_dev, 0 | (dir ? USB_DIR_IN : USB_DIR_OUT));
>> + ep_sts_reg = readl(®s->ep_sts);
>> +
>> + __pending_setup_status_handler(priv_dev);
>> +
>> + if ((ep_sts_reg & EP_STS_SETUP) && dir == 0) {
>> + struct usb_ctrlrequest *setup = priv_dev->setup;
>> +
>> + writel(EP_STS_SETUP | EP_STS_IOC | EP_STS_ISP, ®s->ep_sts);
>
>instead you can just clear all events at the end of this function by
> writel(ep_sts_reg, ®s->ep_sts);
But I can delete events that can appear during processing this function.
I think that safer is to add this fragment at the beginning, below line
ep_sts_reg = readl(®s->ep_sts);
>> +
>> + priv_dev->ep0_data_dir = setup->bRequestType & USB_DIR_IN;
>> + cdns3_ep0_setup_phase(priv_dev);
>> + ep_sts_reg &= ~(EP_STS_SETUP | EP_STS_IOC | EP_STS_ISP);
>
>Not required.
Why not ? I must clear at least EP_STS_IOC | EP_STS_ISP.
I don't want to handle the last condition in this function, of course if SETUP was reported.
Of course I can also change the construction of function and then I could delete this statement.
>
>> + }
>> +
>> + if (ep_sts_reg & EP_STS_TRBERR)
>> + writel(EP_STS_TRBERR, &priv_dev->regs->ep_sts);
>
>Can be omitted.
>> +
>> + if (ep_sts_reg & EP_STS_DESCMIS) {
>> + writel(EP_STS_DESCMIS, &priv_dev->regs->ep_sts);
>
>This as well.
>> +
>> + if (dir == 0 && !priv_dev->setup_pending) {
>> + priv_dev->ep0_data_dir = 0;
>> + cdns3_ep0_run_transfer(priv_dev, priv_dev->setup_dma,
>> + 8, 0);
>> + }
>> + }
>> +
>> + if ((ep_sts_reg & EP_STS_IOC) || (ep_sts_reg & EP_STS_ISP)) {
>> + writel(EP_STS_IOC, &priv_dev->regs->ep_sts);
>
>this write can be omitted.
>> + cdns3_transfer_completed(priv_dev);
>> + }
>
>here you can do
> writel(ep_sts_reg, ®s->ep_sts);
>> +}
>> +
>> /**
>> * cdns3_gadget_ep0_enable
>> * Function shouldn't be called by gadget driver,
>> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
>> index c965da16c0c8..309202474e57 100644
>> --- a/drivers/usb/cdns3/gadget.c
>> +++ b/drivers/usb/cdns3/gadget.c
>> @@ -58,6 +58,18 @@ void cdns3_set_register_bit(void __iomem *ptr, u32 mask)
>> writel(mask, ptr);
>> }
>>
>> +/**
>> + * cdns3_ep_reg_pos_to_index - Macro converts bit position of ep_ists register
>> + * to index of endpoint object in cdns3_device.eps[] container
>> + * @i: bit position of endpoint for which endpoint object is required
>> + *
>> + * Remember that endpoint container doesn't contain default endpoint
>> + */
>> +static u8 cdns3_ep_reg_pos_to_index(int i)
>> +{
>> + return ((i / 16) + (((i % 16) - 2) * 2));
>> +}
>> +
>> /**
>> * cdns3_next_request - returns next request from list
>> * @list: list containing requests
>> @@ -188,6 +200,21 @@ static void cdns3_ep_stall_flush(struct cdns3_endpoint *priv_ep)
>> priv_ep->flags |= EP_STALL;
>> }
>>
>> +/**
>> + * cdns3_gadget_unconfig - reset device configuration
>> + * @priv_dev: extended gadget object
>> + */
>> +void cdns3_gadget_unconfig(struct cdns3_device *priv_dev)
>> +{
>> + /* RESET CONFIGURATION */
>> + writel(USB_CONF_CFGRST, &priv_dev->regs->usb_conf);
>> +
>> + cdns3_enable_l1(priv_dev, 0);
>> + priv_dev->hw_configured_flag = 0;
>> + priv_dev->onchip_mem_allocated_size = 0;
>> + priv_dev->out_mem_is_allocated = 0;
>> +}
>> +
>> void cdns3_enable_l1(struct cdns3_device *priv_dev, int enable)
>> {
>> if (enable)
>> @@ -196,6 +223,23 @@ void cdns3_enable_l1(struct cdns3_device *priv_dev, int enable)
>> writel(USB_CONF_L1DS, &priv_dev->regs->usb_conf);
>> }
>>
>> +static enum usb_device_speed cdns3_get_speed(struct cdns3_device *priv_dev)
>> +{
>> + u32 reg;
>> +
>> + reg = readl(&priv_dev->regs->usb_sts);
>> +
>> + if (DEV_SUPERSPEED(reg))
>> + return USB_SPEED_SUPER;
>> + else if (DEV_HIGHSPEED(reg))
>> + return USB_SPEED_HIGH;
>> + else if (DEV_FULLSPEED(reg))
>> + return USB_SPEED_FULL;
>> + else if (DEV_LOWSPEED(reg))
>> + return USB_SPEED_LOW;
>> + return USB_SPEED_UNKNOWN;
>> +}
>> +
>> /**
>> * cdns3_gadget_giveback - call struct usb_request's ->complete callback
>> * @priv_ep: The endpoint to whom the request belongs to
>> @@ -221,12 +265,136 @@ void cdns3_gadget_giveback(struct cdns3_endpoint *priv_ep,
>> */
>> int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
>> struct usb_request *request)
>> +{
>> + return 0;
>> +}
>> +
>> +static void cdns3_transfer_completed(struct cdns3_device *priv_dev,
>> + struct cdns3_endpoint *priv_ep)
>> {
>> //TODO: Implements this function.
>> +}
>> +
>> +/**
>> + * cdns3_check_ep_interrupt_proceed - Processes interrupt related to endpoint
>> + * @priv_ep: endpoint object
>> + *
>> + * Returns 0
>> + */
>> +static int cdns3_check_ep_interrupt_proceed(struct cdns3_endpoint *priv_ep)
>> +{
>> + struct cdns3_device *priv_dev = priv_ep->cdns3_dev;
>> + struct cdns3_usb_regs __iomem *regs;
>> + u32 ep_sts_reg;
>> +
>> + regs = priv_dev->regs;
>> +
>> + cdns3_select_ep(priv_dev, priv_ep->endpoint.address);
>> + ep_sts_reg = readl(®s->ep_sts);
>> +
>> + if (ep_sts_reg & EP_STS_TRBERR)
>> + writel(EP_STS_TRBERR, ®s->ep_sts);
>> +
>> + if (ep_sts_reg & EP_STS_ISOERR)
>> + writel(EP_STS_ISOERR, ®s->ep_sts);
>> +
>> + if (ep_sts_reg & EP_STS_OUTSMM)
>> + writel(EP_STS_OUTSMM, ®s->ep_sts);
>> +
>> + if (ep_sts_reg & EP_STS_NRDY)
>> + writel(EP_STS_NRDY, ®s->ep_sts);
>
>Why check for each bit when you are not doing anything. Instead at the end
>you could just do
> writel(ep_sts_reg, ®s->ep_sts)
>to clear all pending events.
Some time ago I had in these conditions debug messages, but then
I created the cdns3_decode_epx_irq and I removed them from there.
I will remove them.
>> +
>> + if ((ep_sts_reg & EP_STS_IOC) || (ep_sts_reg & EP_STS_ISP)) {
>> + writel(EP_STS_IOC | EP_STS_ISP, ®s->ep_sts);
>> + cdns3_transfer_completed(priv_dev, priv_ep);
>> + }
>> +
>> + if (ep_sts_reg & EP_STS_DESCMIS)
>> + writel(EP_STS_DESCMIS, ®s->ep_sts);
>>
>> return 0;
>> }
>>
>> +/**
>> + * cdns3_check_usb_interrupt_proceed - Processes interrupt related to device
>> + * @priv_dev: extended gadget object
>> + * @usb_ists: bitmap representation of device's reported interrupts
>> + * (usb_ists register value)
>> + */
>> +static void cdns3_check_usb_interrupt_proceed(struct cdns3_device *priv_dev,
>> + u32 usb_ists)
>> +{
>> + struct cdns3_usb_regs __iomem *regs;
>> + int speed = 0;
>> +
>> + regs = priv_dev->regs;
>> +
>> + /* Connection detected */
>> + if (usb_ists & (USB_ISTS_CON2I | USB_ISTS_CONI)) {
>> + writel(USB_ISTS_CON2I | USB_ISTS_CONI, ®s->usb_ists);
>> + speed = cdns3_get_speed(priv_dev);
>> +
>> + dev_dbg(&priv_dev->dev, "Connection detected at speed: %s %d\n",
>> + usb_speed_string(speed), speed);
>> +
>> + priv_dev->gadget.speed = speed;
>> + priv_dev->is_connected = 1;
>> + usb_gadget_set_state(&priv_dev->gadget, USB_STATE_POWERED);
>> + cdns3_ep0_config(priv_dev);
>> + }
>> +
>> + /* SS Disconnection detected */
>> + if (usb_ists & (USB_ISTS_DIS2I | USB_ISTS_DISI)) {
>> + dev_dbg(&priv_dev->dev, "Disconnection detected\n");
>> +
>> + writel(USB_ISTS_DIS2I | USB_ISTS_DISI, ®s->usb_ists);
>> + if (priv_dev->gadget_driver &&
>> + priv_dev->gadget_driver->disconnect) {
>> + spin_unlock(&priv_dev->lock);
>> + priv_dev->gadget_driver->disconnect(&priv_dev->gadget);
>> + spin_lock(&priv_dev->lock);
>> + }
>> + priv_dev->gadget.speed = USB_SPEED_UNKNOWN;
>> + usb_gadget_set_state(&priv_dev->gadget, USB_STATE_NOTATTACHED);
>> + priv_dev->is_connected = 0;
>> + cdns3_gadget_unconfig(priv_dev);
>> + }
>
>What about non Super-Speed disconnects?
I connect 2 conditions and I forgot to change the comment.
USB_ISTS_DIS2I - HS/FS disconnection
USB_ISTS_DISI) - SS disconnection
>
>> +
>> + if (usb_ists & USB_ISTS_L2ENTI) {
>> + dev_dbg(&priv_dev->dev, "Device suspended\n");
>> + writel(USB_ISTS_L2ENTI, ®s->usb_ists);
>> + }
>> +
>> + /* Exit from standby mode on L2 exit (Suspend in HS/FS or SS) */
>> + if (usb_ists & USB_ISTS_L2EXTI) {
>> + dev_dbg(&priv_dev->dev, "[Interrupt] L2 exit detected\n");
>> + writel(USB_ISTS_L2EXTI, ®s->usb_ists);
>> + }
>> +
>> + /* Exit from standby mode on U3 exit (Suspend in HS/FS or SS). */
>> + if (usb_ists & USB_ISTS_U3EXTI) {
>> + dev_dbg(&priv_dev->dev, "U3 exit detected\n");
>> + writel(USB_ISTS_U3EXTI, ®s->usb_ists);
>> + }
>> +
>> + /* resets cases */
>> + if (usb_ists & (USB_ISTS_UWRESI | USB_ISTS_UHRESI | USB_ISTS_U2RESI)) {
>> + writel(USB_ISTS_U2RESI | USB_ISTS_UWRESI | USB_ISTS_UHRESI,
>> + ®s->usb_ists);
>> +
>> + /*read again to check the actuall speed*/
>> + speed = cdns3_get_speed(priv_dev);
>> +
>> + dev_dbg(&priv_dev->dev, "Reset detected at speed: %s %d\n",
>> + usb_speed_string(speed), speed);
>> +
>> + usb_gadget_set_state(&priv_dev->gadget, USB_STATE_DEFAULT);
>> + priv_dev->gadget.speed = speed;
>> + cdns3_gadget_unconfig(priv_dev);
>> + cdns3_ep0_config(priv_dev);
>> + }
>> +}
>> +
>> /**
>> * cdns3_irq_handler - irq line interrupt handler
>> * @cdns: cdns3 instance
>> @@ -237,8 +405,62 @@ int cdns3_ep_run_transfer(struct cdns3_endpoint *priv_ep,
>> */
>> static irqreturn_t cdns3_irq_handler_thread(struct cdns3 *cdns)
>> {
>> + struct cdns3_device *priv_dev;
>> irqreturn_t ret = IRQ_NONE;
>> - //TODO: implements this function
>> + unsigned long flags;
>> + u32 reg;
>> +
>> + priv_dev = container_of(cdns->gadget_dev, struct cdns3_device, dev);
>> + spin_lock_irqsave(&priv_dev->lock, flags);
>> +
>> + /* check USB device interrupt */
>> + reg = readl(&priv_dev->regs->usb_ists);
>> + if (reg) {
>> + dev_dbg(&priv_dev->dev, "IRQ: usb_ists: %08X\n", reg);
>
>dev_dbg will be terribly slow to be useful. Tracepoints?
It will be moved to Tracepoint.
>
>> + cdns3_check_usb_interrupt_proceed(priv_dev, reg);
>> + ret = IRQ_HANDLED;
>> + }
>> +
>> + /* check endpoint interrupt */
>> + reg = readl(&priv_dev->regs->ep_ists);
>> + if (reg != 0) {
>> + dev_dbg(&priv_dev->dev, "IRQ ep_ists: %08X\n", reg);
>> + } else {
>> + if (USB_STS_CFGSTS(readl(&priv_dev->regs->usb_sts)))
>> + ret = IRQ_HANDLED;
>
>Why is this done. We don't seem to be handling anything here.
>Don't we need to clear the usb_sts?
This check CFGSTS bit. From controller spec:
Configuration status -
1 - device is in the configured state
0 - device is not configured
This bit set during SET_CONFIGURATION
request means that status stage of this
request was finished successfully, thus device
configuration was finished successfully
And again, the setting related with setting endpoint configuration.
The author did not mention anything about setting alternate setting
that require reconfiguring endpoints.
I think that this fragment protects against handling epx before setting configuration.
I think that I can omit this fragment, but I need to test it with Command Verifier.
Regarding clearing usb_sts, it's cleared but in other functions invoked from this one.
Maybe it could be better to pass to these other function content of usb_ists and do
clearing in one place. It could limit access to registers and make the code more readable
>
>> + goto irqend;
>> + }
>> +
>> + /* handle default endpoint OUT */
>> + if (reg & EP_ISTS_EP_OUT0) {
>> + cdns3_check_ep0_interrupt_proceed(priv_dev, 0);
>> + ret = IRQ_HANDLED;
>> + }
>> +
>> + /* handle default endpoint IN */
>> + if (reg & EP_ISTS_EP_IN0) {
>> + cdns3_check_ep0_interrupt_proceed(priv_dev, 1);
>> + ret = IRQ_HANDLED;
>> + }
>> +
>> + /* check if interrupt from non default endpoint, if no exit */
>> + reg &= ~(EP_ISTS_EP_OUT0 | EP_ISTS_EP_IN0);
>> + if (!reg)
>> + goto irqend;
>> +
>> + do {
>> + unsigned int bit_pos = ffs(reg);
>> + u32 bit_mask = 1 << (bit_pos - 1);
>> + int index;
>> +
>> + index = cdns3_ep_reg_pos_to_index(bit_pos);
>> + cdns3_check_ep_interrupt_proceed(priv_dev->eps[index]);
>> + reg &= ~bit_mask;
>> + ret = IRQ_HANDLED;
>> + } while (reg);
>> +
>> +irqend:
>> + spin_unlock_irqrestore(&priv_dev->lock, flags);
>> return ret;
>> }
>>
>> diff --git a/drivers/usb/cdns3/gadget.h b/drivers/usb/cdns3/gadget.h
>> index 224f6b830bc9..8c2f363f9340 100644
>> --- a/drivers/usb/cdns3/gadget.h
>> +++ b/drivers/usb/cdns3/gadget.h
>> @@ -1072,6 +1072,7 @@ int cdns3_handshake(void __iomem *ptr, u32 mask, u32 done, int usec);
>> void cdns3_set_register_bit(void __iomem *ptr, u32 mask);
>> int cdns3_init_ep0(struct cdns3_device *priv_dev);
>> void cdns3_ep0_config(struct cdns3_device *priv_dev);
>> +void cdns3_check_ep0_interrupt_proceed(struct cdns3_device *priv_dev, int dir);
>> void cdns3_select_ep(struct cdns3_device *priv_dev, u32 ep);
>> void cdns3_enable_l1(struct cdns3_device *priv_dev, int enable);
>> struct usb_request *cdns3_next_request(struct list_head *list);
>>
Thanks
Cheers,
Pawel