Re: [PATCH v4 2/2] usb: gadget: Add xilinx usb2 device support
Hi Felipe, On Thu, Aug 21, 2014 at 7:30 PM, Felipe Balbi ba...@ti.com wrote: On Thu, Aug 21, 2014 at 12:19:03PM +0530, sundeep subbaraya wrote: Hi Daniel, On Tue, Aug 19, 2014 at 3:26 PM, Daniel Mack dan...@zonque.org wrote: Hi, On 07/22/2014 11:08 AM, Subbaraya Sundeep Bhatta wrote: This patch adds xilinx usb2 device driver support Add some more information here, please. Copying the text from the Kconfig option is already a good start. drivers/usb/gadget/Kconfig | 14 + drivers/usb/gadget/Makefile |1 + drivers/usb/gadget/udc-xilinx.c | 2261 +++ 3 files changed, 2276 insertions(+), 0 deletions(-) create mode 100644 drivers/usb/gadget/udc-xilinx.c As your driver has a DT binding, you have to describe it in Documentation/devicetree/bindings. --- a/drivers/usb/gadget/Kconfig +++ b/drivers/usb/gadget/Kconfig @@ -459,6 +459,20 @@ config USB_EG20T ML7213/ML7831 is companion chip for Intel Atom E6xx series. ML7213/ML7831 is completely compatible for Intel EG20T PCH. +config USB_GADGET_XILINX + tristate Xilinx USB Driver + depends on COMPILE_TEST Why would you depend on that? Felipe asked to make this since this is USB soft IP driver and its dependencies have tendency to grow. that's not exactly what I asked :-) Usually you only add COMPILE_TEST when you have an ARCH dependency. So something like: depends on ARCH_ARM || COMPILE_TEST would make it clear that this driver is only available on ARM, but when doing my build tests, I'd still be able to compile it on x86. Ok got it :) Also, your code uses device tree functions unconditionally, which is fine, but it must hence depend on 'OF'. Ok will add OF along with COMPILE_TEST so this would be: depends on OF || COMPILE_TEST Ok Thanks, Sundeep -- balbi -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/2] usb: gadget: Add xilinx usb2 device support
On Wednesday 10 September 2014 19:25:11 sundeep subbaraya wrote: that's not exactly what I asked Usually you only add COMPILE_TEST when you have an ARCH dependency. So something like: depends on ARCH_ARM || COMPILE_TEST would make it clear that this driver is only available on ARM, but when doing my build tests, I'd still be able to compile it on x86. Ok got it Most of the xilinx stuff also applies to MICROBLAZE however, so it may need to be ARM || MICROBLAZE || COMPILE_TEST (not ARCH_ARM, btw). Also, your code uses device tree functions unconditionally, which is fine, but it must hence depend on 'OF'. Ok will add OF along with COMPILE_TEST so this would be: depends on OF || COMPILE_TEST If it requires OF to build, you can use 'OF (ARM || COMPILE_TEST)', but if there is no compile-time dependency, you can just leave out the 'depends on OF' completely. You can't even build a Zynq machine without OF enabled. Arnd -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/2] usb: gadget: Add xilinx usb2 device support
On Thu, Aug 21, 2014 at 12:19:03PM +0530, sundeep subbaraya wrote: Hi Daniel, On Tue, Aug 19, 2014 at 3:26 PM, Daniel Mack dan...@zonque.org wrote: Hi, On 07/22/2014 11:08 AM, Subbaraya Sundeep Bhatta wrote: This patch adds xilinx usb2 device driver support Add some more information here, please. Copying the text from the Kconfig option is already a good start. drivers/usb/gadget/Kconfig | 14 + drivers/usb/gadget/Makefile |1 + drivers/usb/gadget/udc-xilinx.c | 2261 +++ 3 files changed, 2276 insertions(+), 0 deletions(-) create mode 100644 drivers/usb/gadget/udc-xilinx.c As your driver has a DT binding, you have to describe it in Documentation/devicetree/bindings. --- a/drivers/usb/gadget/Kconfig +++ b/drivers/usb/gadget/Kconfig @@ -459,6 +459,20 @@ config USB_EG20T ML7213/ML7831 is companion chip for Intel Atom E6xx series. ML7213/ML7831 is completely compatible for Intel EG20T PCH. +config USB_GADGET_XILINX + tristate Xilinx USB Driver + depends on COMPILE_TEST Why would you depend on that? Felipe asked to make this since this is USB soft IP driver and its dependencies have tendency to grow. that's not exactly what I asked :-) Usually you only add COMPILE_TEST when you have an ARCH dependency. So something like: depends on ARCH_ARM || COMPILE_TEST would make it clear that this driver is only available on ARM, but when doing my build tests, I'd still be able to compile it on x86. Also, your code uses device tree functions unconditionally, which is fine, but it must hence depend on 'OF'. Ok will add OF along with COMPILE_TEST so this would be: depends on OF || COMPILE_TEST -- balbi signature.asc Description: Digital signature
Re: [PATCH v4 2/2] usb: gadget: Add xilinx usb2 device support
Hi,
On Tue, Jul 22, 2014 at 3:32 PM, Varka Bhadram varkabhad...@gmail.com wrote:
On 07/22/2014 02:38 PM, Subbaraya Sundeep Bhatta wrote:
+#include linux/delay.h
+#include linux/device.h
+#include linux/dma-mapping.h
+#include gadget_chips.h
+#include linux/interrupt.h
+#include linux/io.h
+#include linux/module.h
+#include linux/of_address.h
+#include linux/of_device.h
+#include linux/of_platform.h
+#include linux/of_irq.h
+#include linux/prefetch.h
+#include linux/usb/ch9.h
+#include linux/usb/gadget.h
+
Normally we will put the includes in alphabetical because it looks good
and readable.
But we have to include the local headers separately after all the main
includes...
#include linux/delay.h
#include linux/device.h
#include linux/usb/gadget.h
#include gadget_chips.h
(...)
Ok
+ switch (udc-setupseqtx) {
+ case STATUS_PHASE:
+ switch (udc-setup.bRequest) {
+ case USB_REQ_SET_ADDRESS:
+ /* Set the address of the device.*/
+ udc-write_fn(udc-base_address,
+ XUSB_ADDRESS_OFFSET,
+ udc-setup.wValue);
Should match open parenthesis
not necessary, as per coding style spaces should not be used --
Outside of comments, documentation and except in Kconfig, spaces are never
used for indentation.
udc-write_fn(udc-base_address,
XUSB_ADDRESS_OFFSET,
udc-setup.wValue);
+ break;
+ case USB_REQ_SET_FEATURE:
+ if (udc-setup.bRequestType ==
+ USB_RECIP_DEVICE) {
+ if (udc-setup.wValue ==
+ USB_DEVICE_TEST_MODE)
+ udc-write_fn(udc-base_address,
+
XUSB_TESTMODE_OFFSET,
+ test_mode);
Dto
not necessary
+ }
+ break;
+ }
+ req-usb_req.actual = req-usb_req.length;
+ xudc_done(ep0, req, 0);
+ break;
+ case DATA_PHASE:
+ if (!bytes_to_tx) {
+ /*
+* We're done with data transfer, next
+* will be zero length OUT with data toggle of
+* 1. Setup data_toggle.
+*/
+ epcfgreg = udc-read_fn(udc-base_address +
+ ep0-offset);
+ epcfgreg |= XUSB_EP_CFG_DATA_TOGGLE_MASK;
+ udc-write_fn(udc-base_address, ep0-offset,
epcfgreg);
+ udc-setupseqtx = STATUS_PHASE;
+ } else {
+ length = count = min_t(u32, bytes_to_tx,
+ EP0_MAX_PACKET);
+ /* Copy the data to be transmitted into the DPRAM.
*/
+ ep0rambase = (u8 __force *) (udc-base_address +
+ (ep0-rambase 2));
+
+ buffer = req-usb_req.buf + req-usb_req.actual;
+ req-usb_req.actual = req-usb_req.actual +
length;
+ memcpy((void *)ep0rambase, buffer, length);
+ }
+ udc-write_fn(udc-base_address, XUSB_EP_BUF0COUNT_OFFSET,
+ count);
+ udc-write_fn(udc-base_address, XUSB_BUFFREADY_OFFSET,
1);
+ break;
+ default:
+ break;
+ }
+}
+
+/**
+ * xudc_ctrl_ep_handler - Endpoint 0 interrupt handler.
+ * @udc: pointer to the udc structure.
+ * @intrstatus:It's the mask value for the interrupt sources on
endpoint 0.
+ *
+ * Processes the commands received during enumeration phase.
+ */
+static void xudc_ctrl_ep_handler(struct xusb_udc *udc, u32 intrstatus)
+{
+
+ if (intrstatus XUSB_STATUS_SETUP_PACKET_MASK) {
+ xudc_handle_setup(udc);
+ } else {
+ if (intrstatus XUSB_STATUS_FIFO_BUFF_RDY_MASK)
+ xudc_ep0_out(udc);
+ else if (intrstatus XUSB_STATUS_FIFO_BUFF_FREE_MASK)
+ xudc_ep0_in(udc);
+ }
+}
+
+/**
+ * xudc_nonctrl_ep_handler - Non control endpoint interrupt handler.
+ * @udc: pointer to the udc structure.
+ * @epnum: End point number for which the interrupt is to be processed
+ * @intrstatus:mask value for interrupt sources of endpoints
other
+ * than endpoint 0.
+ *
+ * Processes the buffer completion interrupts.
+ */
+static void xudc_nonctrl_ep_handler(struct xusb_udc *udc, u8 epnum,
+ u32 intrstatus)
Should match open parenthesis:
not necessary
static void
Re: [PATCH v4 2/2] usb: gadget: Add xilinx usb2 device support
Hi,
On 07/22/2014 11:08 AM, Subbaraya Sundeep Bhatta wrote:
This patch adds xilinx usb2 device driver support
Add some more information here, please. Copying the text from the
Kconfig option is already a good start.
drivers/usb/gadget/Kconfig | 14 +
drivers/usb/gadget/Makefile |1 +
drivers/usb/gadget/udc-xilinx.c | 2261
+++
3 files changed, 2276 insertions(+), 0 deletions(-)
create mode 100644 drivers/usb/gadget/udc-xilinx.c
As your driver has a DT binding, you have to describe it in
Documentation/devicetree/bindings.
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -459,6 +459,20 @@ config USB_EG20T
ML7213/ML7831 is companion chip for Intel Atom E6xx series.
ML7213/ML7831 is completely compatible for Intel EG20T PCH.
+config USB_GADGET_XILINX
+ tristate Xilinx USB Driver
+ depends on COMPILE_TEST
Why would you depend on that?
Also, your code uses device tree functions unconditionally, which is
fine, but it must hence depend on 'OF'.
+struct xusb_ep {
+ struct usb_ep ep_usb;
+ struct list_head queue;
+ struct xusb_udc *udc;
+ const struct usb_endpoint_descriptor *desc;
+ u32 rambase;
+ u32 offset;
+ char name[4];
+ u16 epnumber;
+ u16 maxpacket;
+ u16 buffer0count;
+ u16 buffer1count;
+ u8 buffer0ready;
+ u8 buffer1ready;
+ u8 eptype;
+ u8 curbufnum;
+ u8 is_in;
+ u8 is_iso;
Some of those could probably become booleans.
+struct xusb_udc {
+ struct usb_gadget gadget;
+ struct xusb_ep ep[8];
+ struct usb_gadget_driver *driver;
+ struct usb_ctrlrequest setup;
+ struct xusb_req *req;
+ struct device *dev;
+ u32 usb_state;
+ u32 remote_wkp;
+ u32 setupseqtx;
+ u32 setupseqrx;
+ void __iomem *base_address;
+ spinlock_t lock;
+ bool dma_enabled;
+
+ unsigned int (*read_fn)(void __iomem *);
+ void (*write_fn)(void __iomem *, u32, u32);
+};
+
+/* Endpoint buffer start addresses in the core */
+static u32 rambase[8] = { 0x22, 0x1000, 0x1100, 0x1200, 0x1300, 0x1400,
0x1500,
+ 0x1600 };
As stated by Peter, indenting such lines to match the position of the
line before makes such code much prettier and more readable. It's also
done in the majority of drivers.
This counts for many locations in your code.
+/* Control endpoint configuration.*/
+static const struct usb_endpoint_descriptor config_bulk_out_desc = {
+ .bLength= USB_DT_ENDPOINT_SIZE,
+ .bDescriptorType= USB_DT_ENDPOINT,
+ .bEndpointAddress = USB_DIR_OUT,
+ .bmAttributes = USB_ENDPOINT_XFER_BULK,
+ .wMaxPacketSize = cpu_to_le16(64),
Why not use EP0_MAX_PACKET here?
+static void xudc_wrstatus(struct xusb_udc *udc)
+{
+ struct xusb_ep *ep0 = udc-ep[XUSB_EP_NUMBER_ZERO];
+ u32 epcfgreg;
+
+ epcfgreg = udc-read_fn(udc-base_address + ep0-offset)|
+ XUSB_EP_CFG_DATA_TOGGLE_MASK;
+ udc-write_fn(udc-base_address, ep0-offset, epcfgreg);
+ udc-write_fn(udc-base_address, ep0-offset + XUSB_EP_BUF0COUNT_OFFSET,
+ 0);
Just a nit, but renaming 'base_address' to something like 'base' or
'addr' would help you get around or maximum line constraint here and in
some other places.
+static int xudc_dma_send(struct xusb_ep *ep, struct xusb_req *req,
+ u8 *buffer, u32 length)
+{
+ u32 *eprambase;
+ dma_addr_t src;
+ dma_addr_t dst;
+ int ret;
+ struct xusb_udc *udc = ep-udc;
+
+ src = req-usb_req.dma + req-usb_req.actual;
+ if (req-usb_req.length)
+ dma_sync_single_for_device(udc-dev, src,
+ length, DMA_TO_DEVICE);
+ if (!ep-curbufnum !ep-buffer0ready) {
+ /* Get the Buffer address and copy the transmit data.*/
+ eprambase = (u32 __force *)(udc-base_address +
+ ep-rambase);
+ dst = virt_to_phys(eprambase);
+ udc-write_fn(udc-base_address, ep-offset +
+ XUSB_EP_BUF0COUNT_OFFSET, length);
+ udc-write_fn(udc-base_address, XUSB_DMA_CONTROL_OFFSET,
+ XUSB_DMA_BRR_CTRL | (1 ep-epnumber));
+ ep-buffer0ready = 1;
+ ep-curbufnum = 1;
+ } else if (ep-curbufnum !ep-buffer1ready) {
+ /* Get the Buffer address and copy the transmit data.*/
+ eprambase = (u32 __force *)(udc-base_address +
+ ep-rambase + ep-ep_usb.maxpacket);
+ dst = virt_to_phys(eprambase);
+ udc-write_fn(udc-base_address, ep-offset +
+ XUSB_EP_BUF1COUNT_OFFSET, length);
+ udc-write_fn(udc-base_address, XUSB_DMA_CONTROL_OFFSET,
+
Re: [PATCH v4 2/2] usb: gadget: Add xilinx usb2 device support
On 08/19/2014 11:56 AM, Daniel Mack wrote: On 07/22/2014 11:08 AM, Subbaraya Sundeep Bhatta wrote: drivers/usb/gadget/Kconfig | 14 + drivers/usb/gadget/Makefile |1 + drivers/usb/gadget/udc-xilinx.c | 2261 +++ 3 files changed, 2276 insertions(+), 0 deletions(-) create mode 100644 drivers/usb/gadget/udc-xilinx.c As your driver has a DT binding, you have to describe it in Documentation/devicetree/bindings. Ah, sorry. I missed patch 1/2 which does that. Thanks, Daniel -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/2] usb: gadget: Add xilinx usb2 device support
On 07/22/2014 02:38 PM, Subbaraya Sundeep Bhatta wrote:
+#include linux/delay.h
+#include linux/device.h
+#include linux/dma-mapping.h
+#include gadget_chips.h
+#include linux/interrupt.h
+#include linux/io.h
+#include linux/module.h
+#include linux/of_address.h
+#include linux/of_device.h
+#include linux/of_platform.h
+#include linux/of_irq.h
+#include linux/prefetch.h
+#include linux/usb/ch9.h
+#include linux/usb/gadget.h
+
Normally we will put the includes in alphabetical because it looks good
and readable.
But we have to include the local headers separately after all the main
includes...
#include linux/delay.h
#include linux/device.h
#include linux/usb/gadget.h
#include gadget_chips.h
(...)
+ switch (udc-setupseqtx) {
+ case STATUS_PHASE:
+ switch (udc-setup.bRequest) {
+ case USB_REQ_SET_ADDRESS:
+ /* Set the address of the device.*/
+ udc-write_fn(udc-base_address,
+ XUSB_ADDRESS_OFFSET,
+ udc-setup.wValue);
Should match open parenthesis
udc-write_fn(udc-base_address,
XUSB_ADDRESS_OFFSET,
udc-setup.wValue);
+ break;
+ case USB_REQ_SET_FEATURE:
+ if (udc-setup.bRequestType ==
+ USB_RECIP_DEVICE) {
+ if (udc-setup.wValue ==
+ USB_DEVICE_TEST_MODE)
+ udc-write_fn(udc-base_address,
+ XUSB_TESTMODE_OFFSET,
+ test_mode);
Dto
+ }
+ break;
+ }
+ req-usb_req.actual = req-usb_req.length;
+ xudc_done(ep0, req, 0);
+ break;
+ case DATA_PHASE:
+ if (!bytes_to_tx) {
+ /*
+* We're done with data transfer, next
+* will be zero length OUT with data toggle of
+* 1. Setup data_toggle.
+*/
+ epcfgreg = udc-read_fn(udc-base_address +
+ ep0-offset);
+ epcfgreg |= XUSB_EP_CFG_DATA_TOGGLE_MASK;
+ udc-write_fn(udc-base_address, ep0-offset, epcfgreg);
+ udc-setupseqtx = STATUS_PHASE;
+ } else {
+ length = count = min_t(u32, bytes_to_tx,
+ EP0_MAX_PACKET);
+ /* Copy the data to be transmitted into the DPRAM. */
+ ep0rambase = (u8 __force *) (udc-base_address +
+ (ep0-rambase 2));
+
+ buffer = req-usb_req.buf + req-usb_req.actual;
+ req-usb_req.actual = req-usb_req.actual + length;
+ memcpy((void *)ep0rambase, buffer, length);
+ }
+ udc-write_fn(udc-base_address, XUSB_EP_BUF0COUNT_OFFSET,
+ count);
+ udc-write_fn(udc-base_address, XUSB_BUFFREADY_OFFSET, 1);
+ break;
+ default:
+ break;
+ }
+}
+
+/**
+ * xudc_ctrl_ep_handler - Endpoint 0 interrupt handler.
+ * @udc: pointer to the udc structure.
+ * @intrstatus:It's the mask value for the interrupt sources on
endpoint 0.
+ *
+ * Processes the commands received during enumeration phase.
+ */
+static void xudc_ctrl_ep_handler(struct xusb_udc *udc, u32 intrstatus)
+{
+
+ if (intrstatus XUSB_STATUS_SETUP_PACKET_MASK) {
+ xudc_handle_setup(udc);
+ } else {
+ if (intrstatus XUSB_STATUS_FIFO_BUFF_RDY_MASK)
+ xudc_ep0_out(udc);
+ else if (intrstatus XUSB_STATUS_FIFO_BUFF_FREE_MASK)
+ xudc_ep0_in(udc);
+ }
+}
+
+/**
+ * xudc_nonctrl_ep_handler - Non control endpoint interrupt handler.
+ * @udc: pointer to the udc structure.
+ * @epnum: End point number for which the interrupt is to be processed
+ * @intrstatus:mask value for interrupt sources of endpoints other
+ * than endpoint 0.
+ *
+ * Processes the buffer completion interrupts.
+ */
+static void xudc_nonctrl_ep_handler(struct xusb_udc *udc, u8 epnum,
+ u32 intrstatus)
Should match open parenthesis:
static void xudc_nonctrl_ep_handler(struct xusb_udc *udc, u8 epnum,
u32 intrstatus)
+{
+
+ struct xusb_req *req;
+ struct xusb_ep *ep;
+
+ ep = udc-ep[epnum];
+ /* Process the End point interrupts.*/
+ if (intrstatus (XUSB_STATUS_EP0_BUFF1_COMP_MASK epnum))
+ ep-buffer0ready = 0;
+ if
