Hi Felipe,
Thank you for the review.
On Monday 06 October 2014 11:47:31 Felipe Balbi wrote:
> On Mon, Oct 06, 2014 at 06:55:05PM +0300, Laurent Pinchart wrote:
> > The ISP1761 is a dual-mode host and device controller backward
> > compatible on the host side with the ISP1760. Add support for the device
> > controller.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>
> finally my part :-)
>
> Unfortunately I don't have HW to verify all this, so I'd really like to
> see some test results. It would be nice if you had access to a windows
> box to run USB20CV on this UDC too, but I guess that's too much to ask
>
> :-p
>
> Anyway, a few comments below. I might come up with more as I digest this
> code.
>
> > diff --git a/drivers/usb/gadget/udc/Kconfig
> > b/drivers/usb/gadget/udc/Kconfig index 34ebaa6..7bdfdf0 100644
> > --- a/drivers/usb/gadget/udc/Kconfig
> > +++ b/drivers/usb/gadget/udc/Kconfig
> > @@ -109,6 +109,13 @@ config USB_GR_UDC
> >
> > Select this to support Aeroflex Gaisler GRUSBDC cores from the
> > GRLIB
> >
> > VHDL IP core library.
> >
> > +config USB_ISP1761_UDC
> > + boolean "NXP ISP1761 USB Device Controller"
>
> not tristate ?
No, this isn't a separate driver, but an option to compile UDC support in the
existing isp1760 host driver. The isp1760 module itself has a tristate Kconfig
option.
> > diff --git a/drivers/usb/host/isp1760-core.c
> > b/drivers/usb/host/isp1760-core.c index 595f234..54aedc2 100644
> > --- a/drivers/usb/host/isp1760-core.c
> > +++ b/drivers/usb/host/isp1760-core.c
[snip]
> > @@ -72,11 +85,35 @@ static void isp1760_init_core(struct isp1760_device
> > *isp)>
> > isp1760_write32(isp->regs, HC_HW_MODE_CTRL, hwmode);
> > isp1760_write32(isp->regs, HC_HW_MODE_CTRL, hwmode);
> >
> > + /*
> > + * PORT 1 Control register of the ISP1760 is the OTG control register
> > + * on ISP1761.
> > + *
> > + * TODO: Really support OTG. For now we configure port 1 in device
> > mode
> > + * when OTG is requested.
> > + */
> > + if ((isp->devflags & ISP1760_FLAG_ISP1761) &&
> > + (isp->devflags & ISP1760_FLAG_OTG_EN))
> > + otgctrl = ((HW_DM_PULLDOWN | HW_DP_PULLDOWN) << 16)
> > + | HW_OTG_DISABLE;
> > + else
> > + otgctrl = (HW_SW_SEL_HC_DC << 16)
> > + | (HW_VBUS_DRV | HW_SEL_CP_EXT);
> > +
> > + isp1760_write32(isp->regs, HC_PORT1_CTRL, otgctrl);
> > + usleep_range(10000, 11000);
>
> do you need a comment on this usleep_range() ? Perhaps pointing to a
> section of documentation which state this is necessary ? 10ms is quite a
> bit of wait.
I've just moved the delay from isp1760_hc_setup() to isp1760_init_core(),
without testing whether it's really needed. I'll check that.
> > +
> >
> > dev_info(isp->dev, "bus width: %u, oc: %s\n",
> > isp->devflags & ISP1760_FLAG_BUS_WIDTH_16 ? 16 : 32,
> > isp->devflags & ISP1760_FLAG_ANALOG_OC ? "analog" : "digital");
> > }
[snip]
> > @@ -162,6 +209,11 @@ void isp1760_shutdown(struct device *dev)
> > {
> > struct isp1760_device *isp = dev_get_drvdata(dev);
> >
> > + if (isp->devflags & ISP1760_FLAG_ISP1761) {
> > + isp1760_write32(isp->regs, DC_MODE, DC_SFRESET);
> > + isp1760_write32(isp->regs, DC_MODE, 0);
> > + }
>
> looks like you need to fix commit log of previous patch where you state
> you'd maybe reset device side too, but that code only comes in here.
Will do.
> > +
> > isp1760_write32(isp->regs, HC_RESET_REG, SW_RESET_RESET_HC);
> > }
[snip]
> > diff --git a/drivers/usb/host/isp1760-hcd.c
> > b/drivers/usb/host/isp1760-hcd.c index a6081d5..1e3d1d9 100644
> > --- a/drivers/usb/host/isp1760-hcd.c
> > +++ b/drivers/usb/host/isp1760-hcd.c
> > @@ -502,15 +502,6 @@ static int isp1760_hc_setup(struct usb_hcd *hcd)
> >
> > reg_write32(hcd->regs, HC_INTERRUPT_ENABLE, INTERRUPT_ENABLE_MASK);
> >
> > - /*
> > - * PORT 1 Control register of the ISP1760 is the OTG control
> > - * register on ISP1761. Since there is no OTG or device controller
> > - * support in this driver, we use port 1 as a "normal" USB host port
> > on
> > - * both chips.
> > - */
> > - reg_write32(hcd->regs, HC_PORT1_CTRL, PORT1_POWER | PORT1_INIT2);
> > - mdelay(10);
> > -
>
> looks like moving this code should be part of its own patch.
Agreed.
> > priv->hcs_params = reg_read32(hcd->regs, HC_HCSPARAMS);
> >
> > return priv_init(hcd);
[snip]
> > diff --git a/drivers/usb/host/isp1760-udc.c
> > b/drivers/usb/host/isp1760-udc.c new file mode 100644
> > index 0000000..2f447ed
> > --- /dev/null
> > +++ b/drivers/usb/host/isp1760-udc.c
> > @@ -0,0 +1,1418 @@
[snip]
> > +static int isp1760_udc_set_address(struct isp1760_udc *udc, u16 addr)
> > +{
> > + if (addr > 127) {
> > + dev_dbg(udc->isp->dev, "invalid device address %u\n", addr);
> > + return -EINVAL;
> > + }
>
> if you want to be pedantic, you could add a check here to stall
> SetAdress when gadget is already in CONFIGURED state.
OK, I'll do that.
> > +
> > + usb_gadget_set_state(&udc->gadget, addr ? USB_STATE_ADDRESS :
> > + USB_STATE_DEFAULT);
> > +
> > + isp1760_udc_write(udc, DC_ADDRESS, DC_DEVEN | addr);
> > +
> > + spin_lock(&udc->lock);
> > + isp1760_udc_ctrl_send_status(&udc->ep[0], USB_DIR_OUT);
> > + spin_unlock(&udc->lock);
> > +
> > + return 0;
> > +}
[snip]
> > +static void isp1760_ep0_setup(struct isp1760_udc *udc)
> > +{
> > + union {
> > + struct usb_ctrlrequest r;
> > + u32 data[2];
> > + } req;
> > + unsigned int count;
> > + bool stall = false;
> > +
> > + spin_lock(&udc->lock);
> > +
> > + isp1760_udc_write(udc, DC_EPINDEX, DC_EP0SETUP);
> > +
> > + count = isp1760_udc_read(udc, DC_BUFLEN) & DC_DATACOUNT_MASK;
> > + if (count != sizeof(req)) {
> > + spin_unlock(&udc->lock);
> > +
> > + dev_err(udc->isp->dev, "invalid length %u for setup packet\n",
> > + count);
> > +
> > + isp1760_udc_ctrl_send_stall(&udc->ep[0]);
> > + return;
> > + }
> > +
> > + req.data[0] = isp1760_udc_read(udc, DC_DATAPORT);
> > + req.data[1] = isp1760_udc_read(udc, DC_DATAPORT);
> > +
> > + if (udc->ep0_state != ISP1760_CTRL_SETUP) {
> > + spin_unlock(&udc->lock);
> > + dev_dbg(udc->isp->dev, "unexpected SETUP packet\n");
> > + return;
> > + }
> > +
> > + /* Move to the data stage. */
> > + if (req.r.bRequestType & USB_DIR_IN)
> > + udc->ep0_state = ISP1760_CTRL_DATA_IN;
> > + else
> > + udc->ep0_state = ISP1760_CTRL_DATA_OUT;
>
> data phase is optional. This should be something like:
>
> if (req.r.wLength > 0) {
> if (req.r.bRequestType & USB_DIR_IN)
> udc->ep0_state = ISP1760_CTRL_DATA_IN;
> else
> udc->ep0_state = ISP1760_CTRL_DATA_OUT;
> } else {
> udc->ep0_state = ISP1760_CTRL_STATUS;
> }
Except that there's no ISP1760_CTRL_STATUS :-) The device handles the status
stage and completes it automatically, moving back to the setup stage without
notifying the driver. I thus use the ep0_state variable to store both whether
we're in a setup or non-setup stage and the transfer direction. Even if the
result works fine I agree that the implementation is confusing. I'll fix it by
storing the direction in a separate variable.
> > + spin_unlock(&udc->lock);
> > +
> > + dev_dbg(udc->isp->dev,
> > + "%s: bRequestType 0x%02x bRequest 0x%02x wValue 0x%04x wIndex
> > 0x%04x
> > wLength 0x%04x\n", + __func__, req.r.bRequestType, req.r.bRequest,
> > + le16_to_cpu(req.r.wValue), le16_to_cpu(req.r.wIndex),
> > + le16_to_cpu(req.r.wLength));
> > +
> > + if ((req.r.bRequestType & USB_TYPE_MASK) == USB_TYPE_STANDARD)
> > + stall = isp1760_ep0_setup_standard(udc, &req.r);
> > + else
> > + stall = udc->driver->setup(&udc->gadget, &req.r) < 0;
> > +
> > + if (stall)
> > + isp1760_udc_ctrl_send_stall(&udc->ep[0]);
> > +}
[snip]
> > +static int __isp1760_ep_set_halt(struct isp1760_ep *uep, bool stall, bool
> > wedge)
> > +{
> > + struct isp1760_udc *udc = uep->udc;
> > + int ret;
> > +
> > + if (!uep->addr) {
> > + /*
> > + * Halting the control endpoint is only valid as a delayed error
> > + * response to a SETUP packet. Make sure EP0 is in the right
> > + * stage and that the gadget isn't trying to clear the halt
> > + * condition.
> > + */
> > + if (WARN_ON((udc->ep0_state != ISP1760_CTRL_DATA_IN &&
> > + udc->ep0_state != ISP1760_CTRL_DATA_OUT) ||
>
> no, this isn't really correct. You can very well stall a CTRL request.
> Imagine you get a ctrl request which isn't valid at all, you can stall
> right there. A data phase can also be stalled if gadget expected for
> e.g. DATA IN but host tried to move DATA OUT.
Sure, but that's what the call already does. It only rejects stalling the
control endpoint when no control request is in progress. Note that the
ISP1760_CTRL_SETUP state means that we're waiting for a setup packet, not that
the setup packet has been received and is being processed by the driver.
I'll rewrite the condition as
if (WARN_ON(udc->ep0_state == ISP1760_CTRL_SETUP || !stall || wedge))
to make is clearer.
> > + !stall || wedge)) {
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + if (uep->addr && !uep->desc) {
> > + dev_dbg(udc->isp->dev, "%s: ep%02x is disabled\n", __func__,
> > + uep->addr);
> > + return -EINVAL;
> > + }
> > +
> > + if (uep->addr & USB_DIR_IN) {
> > + /* Refuse to halt IN endpoints with active transfers. */
> > + if (!list_empty(&uep->queue)) {
> > + dev_dbg(udc->isp->dev,
> > + "%s: ep%02x has request pending\n", __func__,
> > + uep->addr);
> > + return -EAGAIN;
> > + }
> > + }
> > +
> > + ret = __isp1760_udc_set_halt(uep, stall);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (!uep->addr) {
> > + /*
> > + * Stalling EP0 completes the control transaction, move back to
> > + * the SETUP state.
> > + */
> > + udc->ep0_state = ISP1760_CTRL_SETUP;
> > + return 0;
> > + }
> > +
> > + if (wedge)
> > + uep->wedged = true;
> > + else if (!stall)
> > + uep->wedged = false;
> > +
> > + return 0;
> > +}
[snip]
> > +static void isp1760_udc_disconnect(struct isp1760_udc *udc)
> > +{
> > + if (udc->gadget.state < USB_STATE_POWERED)
> > + return;
> > +
> > + dev_dbg(udc->isp->dev, "Device disconnected in state %u\n",
> > + udc->gadget.state);
> > +
> > + udc->gadget.speed = USB_SPEED_UNKNOWN;
> > + usb_gadget_set_state(&udc->gadget, USB_STATE_ATTACHED);
> > +
> > + if (udc->driver->disconnect)
> > + udc->driver->disconnect(&udc->gadget);
>
> alright, so this can be called from your reset IRQ handler. You need a
> little more inteligence on this branch because if you're coming from
> reset IRQ you only want to call gadget driver's disconnect in case speed
> != UNKNOWN.
>
> That has become easier, however, because of a recent patch adding
> ->reset() method which should be called from reset unconditionally and
> ->disconnect() will only be called from disconnect IRQ.
OK, I'll fix that.
> > +
> > + /* TODO Reset all endpoints ? */
> > +}
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
