This patch set introduce new Cadence USBSS DRD driver
to linux kernel.
The Cadence USBSS DRD Driver s 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 site of USBSS controller is compliance
with XHCI specification, so it works with
standard XHCI linux driver.
Changes since v2:
- made some text correction in Kconfig file as suggested by Randy Dunlap.
- simplified Makefile as suggested by Peter Chan.
- removed phy-names from dt-binding as suggested Rob Herring.
- changed cdns3-usb.txt to cdns3-usb3.txt as suggested by Rob Herring.
- added checking error code in some function in drd.c file
as suggested by Peter Chan.
- added reg-names to dt-binding documentation as suggested by Chunfeng Yun.
- replaced platform_get_resource with platform_get_resource_byname.
- made other changes suggested by Chunfeng Yun.
- fixed bug in cdns3_get_id. Now function return id instead 1.
- added trace_cdns3_log trace event.
- simplify cdns3_disable_write function.
- create separate patch for work around related with blocking endpoint
issue.
- Fixed issue related with stale data address in TRB.
Issue: At some situations, the controller may get stale data address
in TRB at below sequences:
1. Controller read TRB includes data address.
2. Software updates TRBs includes data address and Cycle bit.
3. Controller read TRB which includes Cycle bit.
4. DMA run with stale data address.
- Fixed issue without transfer. In some cases not all interrupts
disabled in Hard IRQ was enabled in Soft Irq.
- Modified LFPS minimal U1 Exit time for controller revision 0x00024505.
- Fixed issue - in some case selected endpoint was unexpectedly changed.
- Fixed issue - after clearing halted endpoint transfer was not started.
- Fixed issue - in some case driver send ACK instead STALL in status phase.
- Fixed issues related to dequeue request.
- Fixed incorrect operator in cdns3_ep_run_transfer function.
Changes since v1:
- Removed not implemented Suspend/Resume functions.
- Fixed some issues in debugging related functions.
- Added trace_cdns3_request_handled marker.
- Added support for Isochronous transfer.
- Added some additional descriptions.
- Fixed compilation error in cdns3_gadget_ep_disable.
- Added detection of device controller version at runtime.
- Upgraded dt-binding documentation.
- Deleted ENOSYS from phy initialization section. It should be also removed
from generic PHY driver.
- added ep0_stage flag used during enumeration process.
- Fixed issue with TEST MODE.
- Added one common function for finish control transfer.
- Separated some decoding function from dwc3 driver to common library file,
and removed equivalents function from debug.h file as suggested by Felipe.
- replaced function name cdns3_gadget_unconfig with cdns3_hw_reset_eps_config.
- Improved algorithm fixing hardware issue related to blocking endpoints.
This issue is related to on-chip shared FIFO buffers for OUT packets.
Problem was reported by Peter Chan.
- Changed organization of endpoint array in cdns3_device object.
- added ep0 to common eps array
- removed cdns3_free_trb_pool and cdns3_ep_addr_to_bit_pos macros.
- removed ep0_trb_dma, ep0_trb fields from cdns3_device.
- Removed ep0_request and ep_nums fields from cdns3_device.
- Other minor changes according with Felipe suggestion.
---
Pawel Laszczak (6):
dt-bindings: add binding for USBSS-DRD controller.
usb:common Separated decoding functions from dwc3 driver.
usb:common Patch simplify usb_decode_set_clear_feature function.
usb:common Simplify usb_decode_get_set_descriptor function.
usb:cdns3 Add Cadence USB3 DRD Driver
usb:cdns3 Fix for stuck packets in on-chip OUT buffer.
.../devicetree/bindings/usb/cdns-usb3.txt | 33 +
drivers/usb/Kconfig | 2 +
drivers/usb/Makefile | 2 +
drivers/usb/cdns3/Kconfig | 44 +
drivers/usb/cdns3/Makefile | 14 +
drivers/usb/cdns3/cdns3-pci-wrap.c | 155 ++
drivers/usb/cdns3/core.c | 403 +++
drivers/usb/cdns3/core.h | 116 +
drivers/usb/cdns3/debug.h | 168 ++
drivers/usb/cdns3/debugfs.c | 164 ++
drivers/usb/cdns3/drd.c | 365 +++
drivers/usb/cdns3/drd.h | 162 ++
drivers/usb/cdns3/ep0.c | 907 +++++++
drivers/usb/cdns3/gadget-export.h | 28 +
drivers/usb/cdns3/gadget.c | 2254 +++++++++++++++++
drivers/usb/cdns3/gadget.h | 1214 +++++++++
drivers/usb/cdns3/host-export.h | 28 +
drivers/usb/cdns3/host.c | 72 +
drivers/usb/cdns3/trace.c | 23 +
drivers/usb/cdns3/trace.h | 404 +++
drivers/usb/common/Makefile | 2 +-
drivers/usb/common/debug.c | 265 ++
drivers/usb/dwc3/debug.h | 243 --
drivers/usb/dwc3/trace.h | 2 +-
include/linux/usb/ch9.h | 19 +
25 files changed, 6844 insertions(+), 245 deletions(-)
create mode 100644 Documentation/devicetree/bindings/usb/cdns-usb3.txt
create mode 100644 drivers/usb/cdns3/Kconfig
create mode 100644 drivers/usb/cdns3/Makefile
create mode 100644 drivers/usb/cdns3/cdns3-pci-wrap.c
create mode 100644 drivers/usb/cdns3/core.c
create mode 100644 drivers/usb/cdns3/core.h
create mode 100644 drivers/usb/cdns3/debug.h
create mode 100644 drivers/usb/cdns3/debugfs.c
create mode 100644 drivers/usb/cdns3/drd.c
create mode 100644 drivers/usb/cdns3/drd.h
create mode 100644 drivers/usb/cdns3/ep0.c
create mode 100644 drivers/usb/cdns3/gadget-export.h
create mode 100644 drivers/usb/cdns3/gadget.c
create mode 100644 drivers/usb/cdns3/gadget.h
create mode 100644 drivers/usb/cdns3/host-export.h
create mode 100644 drivers/usb/cdns3/host.c
create mode 100644 drivers/usb/cdns3/trace.c
create mode 100644 drivers/usb/cdns3/trace.h
create mode 100644 drivers/usb/common/debug.c
--
2.17.1
Controller for OUT endpoints has shared on-chip buffers for all incoming
packets, including ep0out. It's FIFO buffer, so packets must be handle
by DMA in correct order. If the first packet in the buffer will not be
handled, then the following packets directed for other endpoints and
functions will be blocked.
Additionally the packets directed to one endpoint can block entire on-chip
buffers. In this case transfer to other endpoints also will blocked.
To resolve this issue after raising the descriptor missing interrupt
driver prepares internal usb_request object and use it to arm DMA
transfer.
The problematic situation was observed in case when endpoint has
been enabled but no usb_request were queued. Driver try detects
such endpoints and will use this workaround only for these endpoint.
Driver use limited number of buffer. This number can be set by macro
CDNS_WA2_NUM_BUFFERS.
Such blocking situation was observed on ACM gadget. For this function
host send OUT data packet but ACM function is not prepared for
this packet. It's cause that buffer placed in on chip memory block
transfer to other endpoints.
It's limitation of controller but maybe this issues should be fixed in
function driver.
This work around can be disabled/enabled by means of quirk_internal_buffer
module parameter. By default feature is enabled. It can has impact to
transfer performance and in most case this feature can be disabled.
Signed-off-by: Pawel Laszczak <[email protected]>
---
drivers/usb/cdns3/gadget.c | 273 ++++++++++++++++++++++++++++++++++++-
drivers/usb/cdns3/gadget.h | 7 +
2 files changed, 278 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
index 35985633470f..20e46d8e308e 100644
--- a/drivers/usb/cdns3/gadget.c
+++ b/drivers/usb/cdns3/gadget.c
@@ -27,6 +27,37 @@
* If (((Dequeue Ptr (i.e. EP_TRADDR) == Enqueue Ptr-1) or
* (Dequeue Ptr (i.e. EP_TRADDR) == Enqueue Ptr))
* and (DRBL==1 and (not EP0)))
+ *
+ * Work around 2:
+ * Controller for OUT endpoints has shared on-chip buffers for all incoming
+ * packets, including ep0out. It's FIFO buffer, so packets must be handle by DMA
+ * in correct order. If the first packet in the buffer will not be handled,
+ * then the following packets directed for other endpoints and functions
+ * will be blocked.
+ * Additionally the packets directed to one endpoint can block entire on-chip
+ * buffers. In this case transfer to other endpoints also will blocked.
+ *
+ * To resolve this issue after raising the descriptor missing interrupt
+ * driver prepares internal usb_request object and use it to arm DMA transfer.
+ *
+ * The problematic situation was observed in case when endpoint has been enabled
+ * but no usb_request were queued. Driver try detects such endpoints and will
+ * use this workaround only for these endpoint.
+ *
+ * Driver use limited number of buffer. This number can be set by macro
+ * CDNS_WA2_NUM_BUFFERS.
+ *
+ * Such blocking situation was observed on ACM gadget. For this function
+ * host send OUT data packet but ACM function is not prepared for this packet.
+ * It's cause that buffer placed in on chip memory block transfer to other
+ * endpoints.
+ *
+ * It's limitation of controller but maybe this issues should be fixed in
+ * function driver.
+ *
+ * This work around can be disabled/enabled by means of quirk_internal_buffer
+ * module parameter. By default feature is enabled. It can has impact to
+ * transfer performance and in most case this feature can be disabled.
*/
#include <linux/dma-mapping.h>
@@ -42,6 +73,14 @@ static int __cdns3_gadget_ep_queue(struct usb_ep *ep,
struct usb_request *request,
gfp_t gfp_flags);
+/*
+ * Parameter allows to disable/enable handling of work around 2 feature.
+ * By default this value is enabled.
+ */
+static bool quirk_internal_buffer = 1;
+module_param(quirk_internal_buffer, bool, 0644);
+MODULE_PARM_DESC(quirk_internal_buffer, "Disable/enable WA2 algorithm");
+
/**
* cdns3_handshake - spin reading until handshake completes or fails
* @ptr: address of device controller register to be read
@@ -105,6 +144,17 @@ struct usb_request *cdns3_next_request(struct list_head *list)
return list_first_entry_or_null(list, struct usb_request, list);
}
+/**
+ * cdns3_next_priv_request - returns next request from list
+ * @list: list containing requests
+ *
+ * Returns request or NULL if no requests in list
+ */
+struct cdns3_request *cdns3_next_priv_request(struct list_head *list)
+{
+ return list_first_entry_or_null(list, struct cdns3_request, list);
+}
+
/**
* select_ep - selects endpoint
* @priv_dev: extended gadget object
@@ -384,6 +434,53 @@ static int cdns3_start_all_request(struct cdns3_device *priv_dev,
return ret;
}
+/**
+ * cdns3_descmiss_copy_data copy data from internal requests to request queued
+ * by class driver.
+ * @priv_ep: extended endpoint object
+ * @request: request object
+ */
+static void cdns3_descmiss_copy_data(struct cdns3_endpoint *priv_ep,
+ struct usb_request *request)
+{
+ struct usb_request *descmiss_req;
+ struct cdns3_request *descmiss_priv_req;
+
+ while (!list_empty(&priv_ep->descmiss_req_list)) {
+ int chunk_end;
+ int length;
+
+ descmiss_priv_req =
+ cdns3_next_priv_request(&priv_ep->descmiss_req_list);
+ descmiss_req = &descmiss_priv_req->request;
+
+ /* driver can't touch pending request */
+ if (descmiss_priv_req->flags & REQUEST_PENDING)
+ break;
+
+ chunk_end = descmiss_priv_req->flags & REQUEST_INTERNAL_CH;
+ length = request->actual + descmiss_req->actual;
+
+ if (length <= request->length) {
+ memcpy(&((u8 *)request->buf)[request->actual],
+ descmiss_req->buf,
+ descmiss_req->actual);
+ request->actual = length;
+ } else {
+ /* It should never occures */
+ request->status = -ENOMEM;
+ }
+
+ list_del_init(&descmiss_priv_req->list);
+
+ kfree(descmiss_req->buf);
+ cdns3_gadget_ep_free_request(&priv_ep->endpoint, descmiss_req);
+
+ if (!chunk_end)
+ break;
+ }
+}
+
/**
* cdns3_gadget_giveback - call struct usb_request's ->complete callback
* @priv_ep: The endpoint to whom the request belongs to
@@ -412,6 +509,32 @@ void cdns3_gadget_giveback(struct cdns3_endpoint *priv_ep,
priv_req->flags &= ~REQUEST_PENDING;
trace_cdns3_gadget_giveback(priv_req);
+ /* WA2: */
+ if (priv_ep->flags & EP_QUIRK_EXTRA_BUF_EN &&
+ priv_req->flags & REQUEST_INTERNAL) {
+ struct usb_request *req;
+
+ req = cdns3_next_request(&priv_ep->deferred_req_list);
+ request = req;
+ priv_ep->descmis_req = NULL;
+
+ if (!req)
+ return;
+
+ cdns3_descmiss_copy_data(priv_ep, req);
+ if (!(priv_ep->flags & EP_QUIRK_END_TRANSFER) &&
+ req->length != req->actual) {
+ /* wait for next part of transfer */
+ return;
+ }
+
+ if (req->status == -EINPROGRESS)
+ req->status = 0;
+
+ list_del_init(&req->list);
+ cdns3_start_all_request(priv_dev, priv_ep);
+ }
+
/* Start all not pending request */
if (priv_ep->flags & EP_RING_FULL)
cdns3_start_all_request(priv_dev, priv_ep);
@@ -769,6 +892,59 @@ void cdns3_rearm_transfer(struct cdns3_endpoint *priv_ep, u8 rearm)
}
}
+/**
+ * cdns3_descmissing_packet - handles descriptor missing event.
+ * @priv_dev: extended gadget object
+ *
+ * This function is used only for WA2. For more information see Work around 2
+ * description.
+ */
+static int cdns3_descmissing_packet(struct cdns3_endpoint *priv_ep)
+{
+ struct cdns3_request *priv_req;
+ struct usb_request *request;
+
+ if (priv_ep->flags & EP_QUIRK_EXTRA_BUF_DET) {
+ priv_ep->flags &= ~EP_QUIRK_EXTRA_BUF_DET;
+ priv_ep->flags |= EP_QUIRK_EXTRA_BUF_EN;
+ }
+
+ cdns3_dbg(priv_ep->cdns3_dev, "WA2: Description Missing detected\n");
+
+ request = cdns3_gadget_ep_alloc_request(&priv_ep->endpoint,
+ GFP_ATOMIC);
+ if (!request)
+ return -ENOMEM;
+
+ priv_req = to_cdns3_request(request);
+ priv_req->flags |= REQUEST_INTERNAL;
+
+ /* if this field is still assigned it indicate that transfer related
+ * with this request has not been finished yet. Driver in this
+ * case simply allocate next request and assign flag REQUEST_INTERNAL_CH
+ * flag to previous one. It will indicate that current request is
+ * part of the previous one.
+ */
+ if (priv_ep->descmis_req)
+ priv_ep->descmis_req->flags |= REQUEST_INTERNAL_CH;
+
+ priv_req->request.buf = kzalloc(CDNS3_DESCMIS_BUF_SIZE,
+ GFP_ATOMIC);
+ if (!priv_req) {
+ cdns3_gadget_ep_free_request(&priv_ep->endpoint, request);
+ return -ENOMEM;
+ }
+
+ priv_req->request.length = CDNS3_DESCMIS_BUF_SIZE;
+ priv_ep->descmis_req = priv_req;
+
+ __cdns3_gadget_ep_queue(&priv_ep->endpoint,
+ &priv_ep->descmis_req->request,
+ GFP_ATOMIC);
+
+ return 0;
+}
+
/**
* cdns3_check_ep_interrupt_proceed - Processes interrupt related to endpoint
* @priv_ep: endpoint object
@@ -801,8 +977,31 @@ static int cdns3_check_ep_interrupt_proceed(struct cdns3_endpoint *priv_ep)
cdns3_rearm_transfer(priv_ep, priv_ep->wa1_set);
}
- if ((ep_sts_reg & EP_STS_IOC) || (ep_sts_reg & EP_STS_ISP))
+ if ((ep_sts_reg & EP_STS_IOC) || (ep_sts_reg & EP_STS_ISP)) {
+ if (priv_ep->flags & EP_QUIRK_EXTRA_BUF_EN) {
+ if (ep_sts_reg & EP_STS_ISP)
+ priv_ep->flags |= EP_QUIRK_END_TRANSFER;
+ else
+ priv_ep->flags &= ~EP_QUIRK_END_TRANSFER;
+ }
+
cdns3_transfer_completed(priv_dev, priv_ep);
+ }
+
+ /*
+ * WA2: this condition should only be meet when
+ * priv_ep->flags & EP_QUIRK_EXTRA_BUF_DET or
+ * priv_ep->flags & EP_QUIRK_EXTRA_BUF_EN.
+ * In other cases this interrupt will be disabled/
+ */
+ if (ep_sts_reg & EP_STS_DESCMIS) {
+ int err;
+
+ err = cdns3_descmissing_packet(priv_ep);
+ if (err)
+ dev_err(priv_dev->dev,
+ "Failed: No sufficient memory for DESCMIS\n");
+ }
return 0;
}
@@ -1223,13 +1422,26 @@ static int cdns3_gadget_ep_enable(struct usb_ep *ep,
/* enable interrupt for selected endpoint */
cdns3_set_register_bit(&priv_dev->regs->ep_ien,
BIT(cdns3_ep_addr_to_index(bEndpointAddress)));
+ /*
+ * WA2: Set flag for all not ISOC OUT endpoints. If this flag is set
+ * driver try to detect whether endpoint need additional internal
+ * buffer for unblocking on-chip FIFO buffer. This flag will be cleared
+ * if before first DESCMISS interrupt the DMA will be armed.
+ */
+ if (quirk_internal_buffer) {
+ if (!priv_ep->dir && priv_ep->type != USB_ENDPOINT_XFER_ISOC) {
+ priv_ep->flags |= EP_QUIRK_EXTRA_BUF_DET;
+ reg |= EP_STS_EN_DESCMISEN;
+ }
+ }
writel(reg, &priv_dev->regs->ep_sts_en);
cdns3_set_register_bit(&priv_dev->regs->ep_cfg, EP_CFG_ENABLE);
ep->desc = desc;
- priv_ep->flags &= ~(EP_PENDING_REQUEST | EP_STALL);
+ priv_ep->flags &= ~(EP_PENDING_REQUEST | EP_STALL |
+ EP_QUIRK_EXTRA_BUF_EN);
priv_ep->flags |= EP_ENABLED | EP_UPDATE_EP_TRBADDR;
priv_ep->wa1_set = 0;
priv_ep->enqueue = 0;
@@ -1254,6 +1466,7 @@ static int cdns3_gadget_ep_enable(struct usb_ep *ep,
static int cdns3_gadget_ep_disable(struct usb_ep *ep)
{
struct cdns3_endpoint *priv_ep;
+ struct cdns3_request *priv_req;
struct cdns3_device *priv_dev;
struct usb_request *request;
unsigned long flags;
@@ -1290,6 +1503,14 @@ static int cdns3_gadget_ep_disable(struct usb_ep *ep)
-ESHUTDOWN);
}
+ while (!list_empty(&priv_ep->descmiss_req_list)) {
+ priv_req = cdns3_next_priv_request(&priv_ep->descmiss_req_list);
+
+ kfree(priv_req->request.buf);
+ cdns3_gadget_ep_free_request(&priv_ep->endpoint,
+ &priv_req->request);
+ }
+
while (!list_empty(&priv_ep->deferred_req_list)) {
request = cdns3_next_request(&priv_ep->deferred_req_list);
@@ -1330,6 +1551,53 @@ static int __cdns3_gadget_ep_queue(struct usb_ep *ep,
priv_req = to_cdns3_request(request);
trace_cdns3_ep_queue(priv_req);
+ /*
+ * WA2: if transfer was queued before DESCMISS appear than we
+ * can disable handling of DESCMISS interrupt. Driver assumes that it
+ * can disable special treatment for this endpoint.
+ */
+ if (priv_ep->flags & EP_QUIRK_EXTRA_BUF_DET) {
+ u32 reg;
+
+ cdns3_select_ep(priv_dev, priv_ep->num | priv_ep->dir);
+ priv_ep->flags &= ~EP_QUIRK_EXTRA_BUF_DET;
+ reg = readl(&priv_dev->regs->ep_sts_en);
+ reg &= ~EP_STS_EN_DESCMISEN;
+ writel(reg, &priv_dev->regs->ep_sts_en);
+ }
+
+ /* WA2 */
+ if (priv_ep->flags & EP_QUIRK_EXTRA_BUF_EN) {
+ u8 pending_empty = list_empty(&priv_ep->pending_req_list);
+ u8 descmiss_empty = list_empty(&priv_ep->descmiss_req_list);
+
+ /*
+ * DESCMISS transfer has been finished, so data will be
+ * directly copied from internal allocated usb_request
+ * objects.
+ */
+ if (pending_empty && !descmiss_empty &&
+ !(priv_req->flags & REQUEST_INTERNAL)) {
+ cdns3_descmiss_copy_data(priv_ep, request);
+ list_add_tail(&request->list,
+ &priv_ep->pending_req_list);
+ cdns3_gadget_giveback(priv_ep, priv_req,
+ request->status);
+ return ret;
+ }
+
+ /*
+ * WA2 driver will wait for completion DESCMISS transfer,
+ * before starts new, not DESCMISS transfer.
+ */
+ if (!pending_empty && !descmiss_empty)
+ deferred = 1;
+
+ if (priv_req->flags & REQUEST_INTERNAL)
+ list_add_tail(&priv_req->list,
+ &priv_ep->descmiss_req_list);
+ }
+
ret = usb_gadget_map_request_by_dev(priv_dev->sysdev, request,
usb_endpoint_dir_in(ep->desc));
if (ret)
@@ -1764,6 +2032,7 @@ static int cdns3_init_eps(struct cdns3_device *priv_dev)
INIT_LIST_HEAD(&priv_ep->pending_req_list);
INIT_LIST_HEAD(&priv_ep->deferred_req_list);
+ INIT_LIST_HEAD(&priv_ep->descmiss_req_list);
}
return 0;
diff --git a/drivers/usb/cdns3/gadget.h b/drivers/usb/cdns3/gadget.h
index 0f8fec39917e..386a2ac64331 100644
--- a/drivers/usb/cdns3/gadget.h
+++ b/drivers/usb/cdns3/gadget.h
@@ -1000,6 +1000,7 @@ struct cdns3_device;
* @endpoint: usb endpoint
* @pending_req_list: list of requests queuing on transfer ring.
* @deferred_req_list: list of requests waiting for queuing on transfer ring.
+ * @descmiss_req_list: list of requests internally allocated by driver (WA2).
* @trb_pool: transfer ring - array of transaction buffers
* @trb_pool_dma: dma address of transfer ring
* @cdns3_dev: device associated with this endpoint
@@ -1026,6 +1027,7 @@ struct cdns3_endpoint {
struct usb_ep endpoint;
struct list_head pending_req_list;
struct list_head deferred_req_list;
+ struct list_head descmiss_req_list;
struct cdns3_trb *trb_pool;
dma_addr_t trb_pool_dma;
@@ -1041,6 +1043,9 @@ struct cdns3_endpoint {
#define EP_PENDING_REQUEST BIT(5)
#define EP_RING_FULL BIT(6)
#define EP_CLAIMED BIT(7)
+#define EP_QUIRK_EXTRA_BUF_DET BIT(8)
+#define EP_QUIRK_EXTRA_BUF_EN BIT(9)
+#define EP_QUIRK_END_TRANSFER BIT(10)
u32 flags;
@@ -1074,6 +1079,7 @@ struct cdns3_endpoint {
* @start_trb: number of the first TRB in transfer ring
* @end_trb: number of the last TRB in transfer ring
* @flags: flag specifying special usage of request
+ * @list: used by internally allocated request to add to descmiss_req_list.
*/
struct cdns3_request {
struct usb_request request;
@@ -1086,6 +1092,7 @@ struct cdns3_request {
#define REQUEST_INTERNAL_CH BIT(2)
#define REQUEST_ZLP BIT(3)
u32 flags;
+ struct list_head list;
};
#define to_cdns3_request(r) (container_of(r, struct cdns3_request, request))
--
2.17.1
This patch aim at documenting USB related dt-bindings for the
Cadence USBSS-DRD controller.
Signed-off-by: Pawel Laszczak <[email protected]>
---
.../devicetree/bindings/usb/cdns-usb3.txt | 33 +++++++++++++++++++
1 file changed, 33 insertions(+)
create mode 100644 Documentation/devicetree/bindings/usb/cdns-usb3.txt
diff --git a/Documentation/devicetree/bindings/usb/cdns-usb3.txt b/Documentation/devicetree/bindings/usb/cdns-usb3.txt
new file mode 100644
index 000000000000..6dc38be77f5c
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/cdns-usb3.txt
@@ -0,0 +1,33 @@
+Binding for the Cadence USBSS-DRD controller
+
+Required properties:
+ - reg: Physical base address and size of the controller's register areas.
+ Controller has 3 different regions:
+ region 1 - HOST registers area
+ region 2 - DEVICE registers area
+ region 3 - OTG/DRD registers area
+ - reg-names - register memory area names:
+ "xhci" - for HOST registers space
+ "dev" - for DEVICE registers space
+ "otg" - for OTG/DRD registers space
+ - compatible: Should contain: "cdns,usb3-1.0.0" or "cdns,usb3-1.0.1"
+ - interrupts: Interrupt specifier. Refer to interrupt bindings.
+ Driver supports only single interrupt line.
+ This single interrupt is shared between Device,
+ host and OTG/DRD part of driver.
+
+Optional properties:
+ - maximum-speed : valid arguments are "super-speed", "high-speed" and
+ "full-speed"; refer to usb/generic.txt
+ - dr_mode: Should be one of "host", "peripheral" or "otg".
+ - phys: reference to the USB PHY
+
+Example:
+ usb@f3000000 {
+ compatible = "cdns,usb3";
+ interrupts = <USB_IRQ 7 IRQ_TYPE_LEVEL_HIGH>;
+ reg = <0xf3000000 0x10000 /* memory area for HOST registers */
+ 0xf3010000 0x10000 /* memory area for DEVICE registers */
+ 0xf3020000 0x10000>; /* memory area for OTG/DRD registers */
+ reg-names = "xhci", "dev", "otg";
+ };
--
2.17.1
Patch adds usb_decode_test_mode and usb_decode_device_feature functions,
which allow to make more readable and simplify the
usb_decode_set_clear_feature function.
Signed-off-by: Pawel Laszczak <[email protected]>
---
drivers/usb/common/debug.c | 83 ++++++++++++++++++--------------------
1 file changed, 40 insertions(+), 43 deletions(-)
diff --git a/drivers/usb/common/debug.c b/drivers/usb/common/debug.c
index 824b3d793745..bdd529a686e4 100644
--- a/drivers/usb/common/debug.c
+++ b/drivers/usb/common/debug.c
@@ -29,6 +29,44 @@ static void usb_decode_get_status(__u8 bRequestType, __u16 wIndex,
}
}
+static const char *usb_decode_device_feature(u16 wValue)
+{
+ switch (wValue) {
+ case USB_DEVICE_SELF_POWERED:
+ return "Self Powered";
+ case USB_DEVICE_REMOTE_WAKEUP:
+ return "Remote Wakeup";
+ case USB_DEVICE_TEST_MODE:
+ return "Test Mode";
+ case USB_DEVICE_U1_ENABLE:
+ return "U1 Enable";
+ case USB_DEVICE_U2_ENABLE:
+ return "U2 Enable";
+ case USB_DEVICE_LTM_ENABLE:
+ return "LTM Enable";
+ default:
+ return "UNKNOWN";
+ }
+}
+
+static const char *usb_decode_test_mode(u16 wIndex)
+{
+ switch (wIndex) {
+ case TEST_J:
+ return ": TEST_J";
+ case TEST_K:
+ return ": TEST_K";
+ case TEST_SE0_NAK:
+ return ": TEST_SE0_NAK";
+ case TEST_PACKET:
+ return ": TEST_PACKET";
+ case TEST_FORCE_EN:
+ return ": TEST_FORCE_EN";
+ default:
+ return ": UNKNOWN";
+ }
+}
+
static void usb_decode_set_clear_feature(__u8 bRequestType,
__u8 bRequest, __u16 wValue,
__u16 wIndex, char *str)
@@ -37,50 +75,9 @@ static void usb_decode_set_clear_feature(__u8 bRequestType,
case USB_RECIP_DEVICE:
sprintf(str, "%s Device Feature(%s%s)",
bRequest == USB_REQ_CLEAR_FEATURE ? "Clear" : "Set",
- ({char *s;
- switch (wValue) {
- case USB_DEVICE_SELF_POWERED:
- s = "Self Powered";
- break;
- case USB_DEVICE_REMOTE_WAKEUP:
- s = "Remote Wakeup";
- break;
- case USB_DEVICE_TEST_MODE:
- s = "Test Mode";
- break;
- case USB_DEVICE_U1_ENABLE:
- s = "U1 Enable";
- break;
- case USB_DEVICE_U2_ENABLE:
- s = "U2 Enable";
- break;
- case USB_DEVICE_LTM_ENABLE:
- s = "LTM Enable";
- break;
- default:
- s = "UNKNOWN";
- } s; }),
+ usb_decode_device_feature(wValue),
wValue == USB_DEVICE_TEST_MODE ?
- ({ char *s;
- switch (wIndex) {
- case TEST_J:
- s = ": TEST_J";
- break;
- case TEST_K:
- s = ": TEST_K";
- break;
- case TEST_SE0_NAK:
- s = ": TEST_SE0_NAK";
- break;
- case TEST_PACKET:
- s = ": TEST_PACKET";
- break;
- case TEST_FORCE_EN:
- s = ": TEST_FORCE_EN";
- break;
- default:
- s = ": UNKNOWN";
- } s; }) : "");
+ usb_decode_test_mode(wIndex) : "");
break;
case USB_RECIP_INTERFACE:
sprintf(str, "%s Interface Feature(%s)",
--
2.17.1
Patch moves some decoding functions from driver/usb/dwc3/debug.h driver
to driver/usb/common/debug.c file. These moved functions include:
dwc3_decode_get_status
dwc3_decode_set_clear_feature
dwc3_decode_set_address
dwc3_decode_get_set_descriptor
dwc3_decode_get_configuration
dwc3_decode_set_configuration
dwc3_decode_get_intf
dwc3_decode_set_intf
dwc3_decode_synch_frame
dwc3_decode_set_sel
dwc3_decode_set_isoch_delay
dwc3_decode_ctrl
These functions are used also in inroduced cdns3 driver.
All functions prefixes were changed from dwc3 to usb.
Also, function's parameters has been extended according to the name
of fields in standard SETUP packet.
Additionally, patch adds usb_decode_ctrl function to
include/linux/usb/ch9.h file.
mend
Signed-off-by: Pawel Laszczak <[email protected]>
---
drivers/usb/common/Makefile | 2 +-
drivers/usb/common/debug.c | 265 ++++++++++++++++++++++++++++++++++++
drivers/usb/dwc3/debug.h | 243 ---------------------------------
drivers/usb/dwc3/trace.h | 2 +-
include/linux/usb/ch9.h | 19 +++
5 files changed, 286 insertions(+), 245 deletions(-)
create mode 100644 drivers/usb/common/debug.c
diff --git a/drivers/usb/common/Makefile b/drivers/usb/common/Makefile
index fb4d5ef4165c..3d3d2962ea4b 100644
--- a/drivers/usb/common/Makefile
+++ b/drivers/usb/common/Makefile
@@ -4,7 +4,7 @@
#
obj-$(CONFIG_USB_COMMON) += usb-common.o
-usb-common-y += common.o
+usb-common-y += common.o debug.o
usb-common-$(CONFIG_USB_LED_TRIG) += led.o
obj-$(CONFIG_USB_OTG_FSM) += usb-otg-fsm.o
diff --git a/drivers/usb/common/debug.c b/drivers/usb/common/debug.c
new file mode 100644
index 000000000000..824b3d793745
--- /dev/null
+++ b/drivers/usb/common/debug.c
@@ -0,0 +1,265 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * Common USB debugging functions
+ *
+ * Copyright (C) 2010-2011 Texas Instruments Incorporated - http://www.ti.com
+ *
+ * Authors: Felipe Balbi <[email protected]>,
+ * Sebastian Andrzej Siewior <[email protected]>
+ */
+
+#ifndef __LINUX_USB_COMMON_DEBUG
+#define __LINUX_USB_COMMON_DEBUG
+
+#include <linux/usb/ch9.h>
+
+static void usb_decode_get_status(__u8 bRequestType, __u16 wIndex,
+ __u16 wLength, char *str)
+{
+ switch (bRequestType & USB_RECIP_MASK) {
+ case USB_RECIP_INTERFACE:
+ sprintf(str, "Get Interface Status(Intf = %d, Length = %d)",
+ wIndex, wLength);
+ break;
+ case USB_RECIP_ENDPOINT:
+ sprintf(str, "Get Endpoint Status(ep%d%s)",
+ wIndex & ~USB_DIR_IN,
+ wIndex & USB_DIR_IN ? "in" : "out");
+ break;
+ }
+}
+
+static void usb_decode_set_clear_feature(__u8 bRequestType,
+ __u8 bRequest, __u16 wValue,
+ __u16 wIndex, char *str)
+{
+ switch (bRequestType & USB_RECIP_MASK) {
+ case USB_RECIP_DEVICE:
+ sprintf(str, "%s Device Feature(%s%s)",
+ bRequest == USB_REQ_CLEAR_FEATURE ? "Clear" : "Set",
+ ({char *s;
+ switch (wValue) {
+ case USB_DEVICE_SELF_POWERED:
+ s = "Self Powered";
+ break;
+ case USB_DEVICE_REMOTE_WAKEUP:
+ s = "Remote Wakeup";
+ break;
+ case USB_DEVICE_TEST_MODE:
+ s = "Test Mode";
+ break;
+ case USB_DEVICE_U1_ENABLE:
+ s = "U1 Enable";
+ break;
+ case USB_DEVICE_U2_ENABLE:
+ s = "U2 Enable";
+ break;
+ case USB_DEVICE_LTM_ENABLE:
+ s = "LTM Enable";
+ break;
+ default:
+ s = "UNKNOWN";
+ } s; }),
+ wValue == USB_DEVICE_TEST_MODE ?
+ ({ char *s;
+ switch (wIndex) {
+ case TEST_J:
+ s = ": TEST_J";
+ break;
+ case TEST_K:
+ s = ": TEST_K";
+ break;
+ case TEST_SE0_NAK:
+ s = ": TEST_SE0_NAK";
+ break;
+ case TEST_PACKET:
+ s = ": TEST_PACKET";
+ break;
+ case TEST_FORCE_EN:
+ s = ": TEST_FORCE_EN";
+ break;
+ default:
+ s = ": UNKNOWN";
+ } s; }) : "");
+ break;
+ case USB_RECIP_INTERFACE:
+ sprintf(str, "%s Interface Feature(%s)",
+ bRequest == USB_REQ_CLEAR_FEATURE ? "Clear" : "Set",
+ wValue == USB_INTRF_FUNC_SUSPEND ?
+ "Function Suspend" : "UNKNOWN");
+ break;
+ case USB_RECIP_ENDPOINT:
+ sprintf(str, "%s Endpoint Feature(%s ep%d%s)",
+ bRequest == USB_REQ_CLEAR_FEATURE ? "Clear" : "Set",
+ wValue == USB_ENDPOINT_HALT ? "Halt" : "UNKNOWN",
+ wIndex & ~USB_DIR_IN,
+ wIndex & USB_DIR_IN ? "in" : "out");
+ break;
+ }
+}
+
+static void usb_decode_set_address(__u16 wValue, char *str)
+{
+ sprintf(str, "Set Address(Addr = %02x)", wValue);
+}
+
+static void usb_decode_get_set_descriptor(__u8 bRequestType, __u8 bRequest,
+ __u16 wValue, __u16 wIndex,
+ __u16 wLength, char *str)
+{
+ sprintf(str, "%s %s Descriptor(Index = %d, Length = %d)",
+ bRequest == USB_REQ_GET_DESCRIPTOR ? "Get" : "Set",
+ ({ char *s;
+ switch (wValue >> 8) {
+ case USB_DT_DEVICE:
+ s = "Device";
+ break;
+ case USB_DT_CONFIG:
+ s = "Configuration";
+ break;
+ case USB_DT_STRING:
+ s = "String";
+ break;
+ case USB_DT_INTERFACE:
+ s = "Interface";
+ break;
+ case USB_DT_ENDPOINT:
+ s = "Endpoint";
+ break;
+ case USB_DT_DEVICE_QUALIFIER:
+ s = "Device Qualifier";
+ break;
+ case USB_DT_OTHER_SPEED_CONFIG:
+ s = "Other Speed Config";
+ break;
+ case USB_DT_INTERFACE_POWER:
+ s = "Interface Power";
+ break;
+ case USB_DT_OTG:
+ s = "OTG";
+ break;
+ case USB_DT_DEBUG:
+ s = "Debug";
+ break;
+ case USB_DT_INTERFACE_ASSOCIATION:
+ s = "Interface Association";
+ break;
+ case USB_DT_BOS:
+ s = "BOS";
+ break;
+ case USB_DT_DEVICE_CAPABILITY:
+ s = "Device Capability";
+ break;
+ case USB_DT_PIPE_USAGE:
+ s = "Pipe Usage";
+ break;
+ case USB_DT_SS_ENDPOINT_COMP:
+ s = "SS Endpoint Companion";
+ break;
+ case USB_DT_SSP_ISOC_ENDPOINT_COMP:
+ s = "SSP Isochronous Endpoint Companion";
+ break;
+ default:
+ s = "UNKNOWN";
+ break;
+ } s; }), wValue & 0xff, wLength);
+}
+
+static void usb_decode_get_configuration(__u16 wLength, char *str)
+{
+ sprintf(str, "Get Configuration(Length = %d)", wLength);
+}
+
+static inline void usb_decode_set_configuration(__u8 wValue, char *str)
+{
+ sprintf(str, "Set Configuration(Config = %d)", wValue);
+}
+
+static void usb_decode_get_intf(__u16 wIndex, __u16 wLength, char *str)
+{
+ sprintf(str, "Get Interface(Intf = %d, Length = %d)", wIndex, wLength);
+}
+
+static void usb_decode_set_intf(__u8 wValue, __u16 wIndex, char *str)
+{
+ sprintf(str, "Set Interface(Intf = %d, Alt.Setting = %d)",
+ wIndex, wValue);
+}
+
+static void usb_decode_synch_frame(__u16 wIndex, __u16 wLength,
+ char *str)
+{
+ sprintf(str, "Synch Frame(Endpoint = %d, Length = %d)",
+ wIndex, wLength);
+}
+
+static void usb_decode_set_sel(__u16 wLength, char *str)
+{
+ sprintf(str, "Set SEL(Length = %d)", wLength);
+}
+
+static void usb_decode_set_isoch_delay(__u8 wValue, char *str)
+{
+ sprintf(str, "Set Isochronous Delay(Delay = %d ns)", wValue);
+}
+
+/**
+ * usb_decode_ctrl - returns a string representation of ctrl request
+ */
+const char *usb_decode_ctrl(char *str, __u8 bRequestType, __u8 bRequest,
+ __u16 wValue, __u16 wIndex, __u16 wLength)
+{
+ switch (bRequest) {
+ case USB_REQ_GET_STATUS:
+ usb_decode_get_status(bRequestType, wIndex, wLength, str);
+ break;
+ case USB_REQ_CLEAR_FEATURE:
+ case USB_REQ_SET_FEATURE:
+ usb_decode_set_clear_feature(bRequestType, bRequest, wValue,
+ wIndex, str);
+ break;
+ case USB_REQ_SET_ADDRESS:
+ usb_decode_set_address(wValue, str);
+ break;
+ case USB_REQ_GET_DESCRIPTOR:
+ case USB_REQ_SET_DESCRIPTOR:
+ usb_decode_get_set_descriptor(bRequestType, bRequest, wValue,
+ wIndex, wLength, str);
+ break;
+ case USB_REQ_GET_CONFIGURATION:
+ usb_decode_get_configuration(wLength, str);
+ break;
+ case USB_REQ_SET_CONFIGURATION:
+ usb_decode_set_configuration(wValue, str);
+ break;
+ case USB_REQ_GET_INTERFACE:
+ usb_decode_get_intf(wIndex, wLength, str);
+ break;
+ case USB_REQ_SET_INTERFACE:
+ usb_decode_set_intf(wValue, wIndex, str);
+ break;
+ case USB_REQ_SYNCH_FRAME:
+ usb_decode_synch_frame(wIndex, wLength, str);
+ break;
+ case USB_REQ_SET_SEL:
+ usb_decode_set_sel(wLength, str);
+ break;
+ case USB_REQ_SET_ISOCH_DELAY:
+ usb_decode_set_isoch_delay(wValue, str);
+ break;
+ default:
+ sprintf(str, "%02x %02x %02x %02x %02x %02x %02x %02x",
+ bRequestType, bRequest,
+ (u8)(cpu_to_le16(wValue) & 0xff),
+ (u8)(cpu_to_le16(wValue) >> 8),
+ (u8)(cpu_to_le16(wIndex) & 0xff),
+ (u8)(cpu_to_le16(wIndex) >> 8),
+ (u8)(cpu_to_le16(wLength) & 0xff),
+ (u8)(cpu_to_le16(wLength) >> 8));
+ }
+
+ return str;
+}
+EXPORT_SYMBOL_GPL(usb_decode_ctrl);
+
+#endif /* __LINUX_USB_COMMON_DEBUG */
diff --git a/drivers/usb/dwc3/debug.h b/drivers/usb/dwc3/debug.h
index c66d216dcc30..9db2b0fb8f22 100644
--- a/drivers/usb/dwc3/debug.h
+++ b/drivers/usb/dwc3/debug.h
@@ -214,249 +214,6 @@ dwc3_gadget_event_string(char *str, const struct dwc3_event_devt *event)
return str;
}
-static inline void dwc3_decode_get_status(__u8 t, __u16 i, __u16 l, char *str)
-{
- switch (t & USB_RECIP_MASK) {
- case USB_RECIP_INTERFACE:
- sprintf(str, "Get Interface Status(Intf = %d, Length = %d)",
- i, l);
- break;
- case USB_RECIP_ENDPOINT:
- sprintf(str, "Get Endpoint Status(ep%d%s)",
- i & ~USB_DIR_IN,
- i & USB_DIR_IN ? "in" : "out");
- break;
- }
-}
-
-static inline void dwc3_decode_set_clear_feature(__u8 t, __u8 b, __u16 v,
- __u16 i, char *str)
-{
- switch (t & USB_RECIP_MASK) {
- case USB_RECIP_DEVICE:
- sprintf(str, "%s Device Feature(%s%s)",
- b == USB_REQ_CLEAR_FEATURE ? "Clear" : "Set",
- ({char *s;
- switch (v) {
- case USB_DEVICE_SELF_POWERED:
- s = "Self Powered";
- break;
- case USB_DEVICE_REMOTE_WAKEUP:
- s = "Remote Wakeup";
- break;
- case USB_DEVICE_TEST_MODE:
- s = "Test Mode";
- break;
- case USB_DEVICE_U1_ENABLE:
- s = "U1 Enable";
- break;
- case USB_DEVICE_U2_ENABLE:
- s = "U2 Enable";
- break;
- case USB_DEVICE_LTM_ENABLE:
- s = "LTM Enable";
- break;
- default:
- s = "UNKNOWN";
- } s; }),
- v == USB_DEVICE_TEST_MODE ?
- ({ char *s;
- switch (i) {
- case TEST_J:
- s = ": TEST_J";
- break;
- case TEST_K:
- s = ": TEST_K";
- break;
- case TEST_SE0_NAK:
- s = ": TEST_SE0_NAK";
- break;
- case TEST_PACKET:
- s = ": TEST_PACKET";
- break;
- case TEST_FORCE_EN:
- s = ": TEST_FORCE_EN";
- break;
- default:
- s = ": UNKNOWN";
- } s; }) : "");
- break;
- case USB_RECIP_INTERFACE:
- sprintf(str, "%s Interface Feature(%s)",
- b == USB_REQ_CLEAR_FEATURE ? "Clear" : "Set",
- v == USB_INTRF_FUNC_SUSPEND ?
- "Function Suspend" : "UNKNOWN");
- break;
- case USB_RECIP_ENDPOINT:
- sprintf(str, "%s Endpoint Feature(%s ep%d%s)",
- b == USB_REQ_CLEAR_FEATURE ? "Clear" : "Set",
- v == USB_ENDPOINT_HALT ? "Halt" : "UNKNOWN",
- i & ~USB_DIR_IN,
- i & USB_DIR_IN ? "in" : "out");
- break;
- }
-}
-
-static inline void dwc3_decode_set_address(__u16 v, char *str)
-{
- sprintf(str, "Set Address(Addr = %02x)", v);
-}
-
-static inline void dwc3_decode_get_set_descriptor(__u8 t, __u8 b, __u16 v,
- __u16 i, __u16 l, char *str)
-{
- sprintf(str, "%s %s Descriptor(Index = %d, Length = %d)",
- b == USB_REQ_GET_DESCRIPTOR ? "Get" : "Set",
- ({ char *s;
- switch (v >> 8) {
- case USB_DT_DEVICE:
- s = "Device";
- break;
- case USB_DT_CONFIG:
- s = "Configuration";
- break;
- case USB_DT_STRING:
- s = "String";
- break;
- case USB_DT_INTERFACE:
- s = "Interface";
- break;
- case USB_DT_ENDPOINT:
- s = "Endpoint";
- break;
- case USB_DT_DEVICE_QUALIFIER:
- s = "Device Qualifier";
- break;
- case USB_DT_OTHER_SPEED_CONFIG:
- s = "Other Speed Config";
- break;
- case USB_DT_INTERFACE_POWER:
- s = "Interface Power";
- break;
- case USB_DT_OTG:
- s = "OTG";
- break;
- case USB_DT_DEBUG:
- s = "Debug";
- break;
- case USB_DT_INTERFACE_ASSOCIATION:
- s = "Interface Association";
- break;
- case USB_DT_BOS:
- s = "BOS";
- break;
- case USB_DT_DEVICE_CAPABILITY:
- s = "Device Capability";
- break;
- case USB_DT_PIPE_USAGE:
- s = "Pipe Usage";
- break;
- case USB_DT_SS_ENDPOINT_COMP:
- s = "SS Endpoint Companion";
- break;
- case USB_DT_SSP_ISOC_ENDPOINT_COMP:
- s = "SSP Isochronous Endpoint Companion";
- break;
- default:
- s = "UNKNOWN";
- break;
- } s; }), v & 0xff, l);
-}
-
-
-static inline void dwc3_decode_get_configuration(__u16 l, char *str)
-{
- sprintf(str, "Get Configuration(Length = %d)", l);
-}
-
-static inline void dwc3_decode_set_configuration(__u8 v, char *str)
-{
- sprintf(str, "Set Configuration(Config = %d)", v);
-}
-
-static inline void dwc3_decode_get_intf(__u16 i, __u16 l, char *str)
-{
- sprintf(str, "Get Interface(Intf = %d, Length = %d)", i, l);
-}
-
-static inline void dwc3_decode_set_intf(__u8 v, __u16 i, char *str)
-{
- sprintf(str, "Set Interface(Intf = %d, Alt.Setting = %d)", i, v);
-}
-
-static inline void dwc3_decode_synch_frame(__u16 i, __u16 l, char *str)
-{
- sprintf(str, "Synch Frame(Endpoint = %d, Length = %d)", i, l);
-}
-
-static inline void dwc3_decode_set_sel(__u16 l, char *str)
-{
- sprintf(str, "Set SEL(Length = %d)", l);
-}
-
-static inline void dwc3_decode_set_isoch_delay(__u8 v, char *str)
-{
- sprintf(str, "Set Isochronous Delay(Delay = %d ns)", v);
-}
-
-/**
- * dwc3_decode_ctrl - returns a string represetion of ctrl request
- */
-static inline const char *dwc3_decode_ctrl(char *str, __u8 bRequestType,
- __u8 bRequest, __u16 wValue, __u16 wIndex, __u16 wLength)
-{
- switch (bRequest) {
- case USB_REQ_GET_STATUS:
- dwc3_decode_get_status(bRequestType, wIndex, wLength, str);
- break;
- case USB_REQ_CLEAR_FEATURE:
- case USB_REQ_SET_FEATURE:
- dwc3_decode_set_clear_feature(bRequestType, bRequest, wValue,
- wIndex, str);
- break;
- case USB_REQ_SET_ADDRESS:
- dwc3_decode_set_address(wValue, str);
- break;
- case USB_REQ_GET_DESCRIPTOR:
- case USB_REQ_SET_DESCRIPTOR:
- dwc3_decode_get_set_descriptor(bRequestType, bRequest, wValue,
- wIndex, wLength, str);
- break;
- case USB_REQ_GET_CONFIGURATION:
- dwc3_decode_get_configuration(wLength, str);
- break;
- case USB_REQ_SET_CONFIGURATION:
- dwc3_decode_set_configuration(wValue, str);
- break;
- case USB_REQ_GET_INTERFACE:
- dwc3_decode_get_intf(wIndex, wLength, str);
- break;
- case USB_REQ_SET_INTERFACE:
- dwc3_decode_set_intf(wValue, wIndex, str);
- break;
- case USB_REQ_SYNCH_FRAME:
- dwc3_decode_synch_frame(wIndex, wLength, str);
- break;
- case USB_REQ_SET_SEL:
- dwc3_decode_set_sel(wLength, str);
- break;
- case USB_REQ_SET_ISOCH_DELAY:
- dwc3_decode_set_isoch_delay(wValue, str);
- break;
- default:
- sprintf(str, "%02x %02x %02x %02x %02x %02x %02x %02x",
- bRequestType, bRequest,
- cpu_to_le16(wValue) & 0xff,
- cpu_to_le16(wValue) >> 8,
- cpu_to_le16(wIndex) & 0xff,
- cpu_to_le16(wIndex) >> 8,
- cpu_to_le16(wLength) & 0xff,
- cpu_to_le16(wLength) >> 8);
- }
-
- return str;
-}
-
/**
* dwc3_ep_event_string - returns event name
* @event: then event code
diff --git a/drivers/usb/dwc3/trace.h b/drivers/usb/dwc3/trace.h
index f22714cce070..6236f0657583 100644
--- a/drivers/usb/dwc3/trace.h
+++ b/drivers/usb/dwc3/trace.h
@@ -86,7 +86,7 @@ DECLARE_EVENT_CLASS(dwc3_log_ctrl,
__entry->wIndex = le16_to_cpu(ctrl->wIndex);
__entry->wLength = le16_to_cpu(ctrl->wLength);
),
- TP_printk("%s", dwc3_decode_ctrl(__get_str(str), __entry->bRequestType,
+ TP_printk("%s", usb_decode_ctrl(__get_str(str), __entry->bRequestType,
__entry->bRequest, __entry->wValue,
__entry->wIndex, __entry->wLength)
)
diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h
index 523aa088f6ab..a33cd2b9fbba 100644
--- a/include/linux/usb/ch9.h
+++ b/include/linux/usb/ch9.h
@@ -62,4 +62,23 @@ extern enum usb_device_speed usb_get_maximum_speed(struct device *dev);
*/
extern const char *usb_state_string(enum usb_device_state state);
+/**
+ * usb_decode_ctrl - Returns human readable representation of control request.
+ * @str: buffer to return a human-readable representation of control request.
+ * This buffer should have about 200 bytes.
+ * @bRequestType: matches the USB bmRequestType field
+ * @bRequest: matches the USB bRequest field
+ * @wValue: matches the USB wValue field (CPU byte order)
+ * @wIndex: matches the USB wIndex field (CPU byte order)
+ * @wLength: matches the USB wLength field (CPU byte order)
+ *
+ * Function returns decoded, formatted and human-readable description of
+ * control request packet.
+ *
+ * Important: wValue, wIndex, wLength parameters before invoking this function
+ * should be processed by le16_to_cpu macro.
+ */
+const char *usb_decode_ctrl(char *str, __u8 bRequestType, __u8 bRequest,
+ __u16 wValue, __u16 wIndex, __u16 wLength);
+
#endif /* __LINUX_USB_CH9_H */
--
2.17.1
Patch moves switch responsible for decoding descriptor type
outside snprintf. It's little improves code readability.
Signed-off-by: Pawel Laszczak <[email protected]>
---
drivers/usb/common/debug.c | 111 +++++++++++++++++++------------------
1 file changed, 57 insertions(+), 54 deletions(-)
diff --git a/drivers/usb/common/debug.c b/drivers/usb/common/debug.c
index bdd529a686e4..a2fbcefe1ea0 100644
--- a/drivers/usb/common/debug.c
+++ b/drivers/usb/common/debug.c
@@ -104,62 +104,65 @@ static void usb_decode_get_set_descriptor(__u8 bRequestType, __u8 bRequest,
__u16 wValue, __u16 wIndex,
__u16 wLength, char *str)
{
+ char *s;
+
+ switch (wValue >> 8) {
+ case USB_DT_DEVICE:
+ s = "Device";
+ break;
+ case USB_DT_CONFIG:
+ s = "Configuration";
+ break;
+ case USB_DT_STRING:
+ s = "String";
+ break;
+ case USB_DT_INTERFACE:
+ s = "Interface";
+ break;
+ case USB_DT_ENDPOINT:
+ s = "Endpoint";
+ break;
+ case USB_DT_DEVICE_QUALIFIER:
+ s = "Device Qualifier";
+ break;
+ case USB_DT_OTHER_SPEED_CONFIG:
+ s = "Other Speed Config";
+ break;
+ case USB_DT_INTERFACE_POWER:
+ s = "Interface Power";
+ break;
+ case USB_DT_OTG:
+ s = "OTG";
+ break;
+ case USB_DT_DEBUG:
+ s = "Debug";
+ break;
+ case USB_DT_INTERFACE_ASSOCIATION:
+ s = "Interface Association";
+ break;
+ case USB_DT_BOS:
+ s = "BOS";
+ break;
+ case USB_DT_DEVICE_CAPABILITY:
+ s = "Device Capability";
+ break;
+ case USB_DT_PIPE_USAGE:
+ s = "Pipe Usage";
+ break;
+ case USB_DT_SS_ENDPOINT_COMP:
+ s = "SS Endpoint Companion";
+ break;
+ case USB_DT_SSP_ISOC_ENDPOINT_COMP:
+ s = "SSP Isochronous Endpoint Companion";
+ break;
+ default:
+ s = "UNKNOWN";
+ break;
+ }
+
sprintf(str, "%s %s Descriptor(Index = %d, Length = %d)",
bRequest == USB_REQ_GET_DESCRIPTOR ? "Get" : "Set",
- ({ char *s;
- switch (wValue >> 8) {
- case USB_DT_DEVICE:
- s = "Device";
- break;
- case USB_DT_CONFIG:
- s = "Configuration";
- break;
- case USB_DT_STRING:
- s = "String";
- break;
- case USB_DT_INTERFACE:
- s = "Interface";
- break;
- case USB_DT_ENDPOINT:
- s = "Endpoint";
- break;
- case USB_DT_DEVICE_QUALIFIER:
- s = "Device Qualifier";
- break;
- case USB_DT_OTHER_SPEED_CONFIG:
- s = "Other Speed Config";
- break;
- case USB_DT_INTERFACE_POWER:
- s = "Interface Power";
- break;
- case USB_DT_OTG:
- s = "OTG";
- break;
- case USB_DT_DEBUG:
- s = "Debug";
- break;
- case USB_DT_INTERFACE_ASSOCIATION:
- s = "Interface Association";
- break;
- case USB_DT_BOS:
- s = "BOS";
- break;
- case USB_DT_DEVICE_CAPABILITY:
- s = "Device Capability";
- break;
- case USB_DT_PIPE_USAGE:
- s = "Pipe Usage";
- break;
- case USB_DT_SS_ENDPOINT_COMP:
- s = "SS Endpoint Companion";
- break;
- case USB_DT_SSP_ISOC_ENDPOINT_COMP:
- s = "SSP Isochronous Endpoint Companion";
- break;
- default:
- s = "UNKNOWN";
- break;
- } s; }), wValue & 0xff, wLength);
+ s, wValue & 0xff, wLength);
}
static void usb_decode_get_configuration(__u16 wLength, char *str)
--
2.17.1
On Thu, Jan 31, 2019 at 1:53 PM Pawel Laszczak <[email protected]> wrote:
>
> Patch moves switch responsible for decoding descriptor type
> outside snprintf. It's little improves code readability.
> + char *s;
const ?
--
With Best Regards,
Andy Shevchenko
On Thu, Jan 31, 2019 at 11:52:29AM +0000, Pawel Laszczak wrote:
> Patch moves some decoding functions from driver/usb/dwc3/debug.h driver
> to driver/usb/common/debug.c file. These moved functions include:
> dwc3_decode_get_status
> dwc3_decode_set_clear_feature
> dwc3_decode_set_address
> dwc3_decode_get_set_descriptor
> dwc3_decode_get_configuration
> dwc3_decode_set_configuration
> dwc3_decode_get_intf
> dwc3_decode_set_intf
> dwc3_decode_synch_frame
> dwc3_decode_set_sel
> dwc3_decode_set_isoch_delay
> dwc3_decode_ctrl
>
> These functions are used also in inroduced cdns3 driver.
>
> All functions prefixes were changed from dwc3 to usb.
Ick, why?
> Also, function's parameters has been extended according to the name
> of fields in standard SETUP packet.
> Additionally, patch adds usb_decode_ctrl function to
> include/linux/usb/ch9.h file.
Why ch9.h? It's not something that is specified in the spec, it's a
usb-specific thing :)
Also, the api for that function is not ok. If you are going to make
this something that the whole kernel can call, you have to fix it up:
> +/**
> + * usb_decode_ctrl - Returns human readable representation of control request.
> + * @str: buffer to return a human-readable representation of control request.
> + * This buffer should have about 200 bytes.
"about 200 bytes" is not very specific.
Pass in the length so we know we don't overflow it.
> + * @bRequestType: matches the USB bmRequestType field
> + * @bRequest: matches the USB bRequest field
> + * @wValue: matches the USB wValue field (CPU byte order)
> + * @wIndex: matches the USB wIndex field (CPU byte order)
> + * @wLength: matches the USB wLength field (CPU byte order)
> + *
> + * Function returns decoded, formatted and human-readable description of
> + * control request packet.
> + *
> + * Important: wValue, wIndex, wLength parameters before invoking this function
> + * should be processed by le16_to_cpu macro.
> + */
> +const char *usb_decode_ctrl(char *str, __u8 bRequestType, __u8 bRequest,
> + __u16 wValue, __u16 wIndex, __u16 wLength);
Why are you returning a value, isn't the data stored in str? Why not
just return an int saying if the call succeeded or not?
thanks,
greg k-h
On Thu, Jan 31, 2019 at 11:52:29AM +0000, Pawel Laszczak wrote:
> Patch moves some decoding functions from driver/usb/dwc3/debug.h driver
> to driver/usb/common/debug.c file. These moved functions include:
> dwc3_decode_get_status
> dwc3_decode_set_clear_feature
> dwc3_decode_set_address
> dwc3_decode_get_set_descriptor
> dwc3_decode_get_configuration
> dwc3_decode_set_configuration
> dwc3_decode_get_intf
> dwc3_decode_set_intf
> dwc3_decode_synch_frame
> dwc3_decode_set_sel
> dwc3_decode_set_isoch_delay
> dwc3_decode_ctrl
>
> These functions are used also in inroduced cdns3 driver.
/me hands Pawel a spell checker for the changelogs...
On Thu, Jan 31, 2019 at 11:52:31AM +0000, Pawel Laszczak wrote:
> Patch moves switch responsible for decoding descriptor type
> outside snprintf. It's little improves code readability.
Should that last sentence read: "It improves code readability a little"?
thanks,
greg k-h
Hi,
Greg KH <[email protected]> writes:
> On Thu, Jan 31, 2019 at 11:52:29AM +0000, Pawel Laszczak wrote:
>> Patch moves some decoding functions from driver/usb/dwc3/debug.h driver
>> to driver/usb/common/debug.c file. These moved functions include:
>> dwc3_decode_get_status
>> dwc3_decode_set_clear_feature
>> dwc3_decode_set_address
>> dwc3_decode_get_set_descriptor
>> dwc3_decode_get_configuration
>> dwc3_decode_set_configuration
>> dwc3_decode_get_intf
>> dwc3_decode_set_intf
>> dwc3_decode_synch_frame
>> dwc3_decode_set_sel
>> dwc3_decode_set_isoch_delay
>> dwc3_decode_ctrl
>>
>> These functions are used also in inroduced cdns3 driver.
>>
>> All functions prefixes were changed from dwc3 to usb.
>
> Ick, why?
moving dwc3-specific code into generic code.
>> Also, function's parameters has been extended according to the name
>> of fields in standard SETUP packet.
>> Additionally, patch adds usb_decode_ctrl function to
>> include/linux/usb/ch9.h file.
>
> Why ch9.h? It's not something that is specified in the spec, it's a
> usb-specific thing :)
agree.
>> +/**
>> + * usb_decode_ctrl - Returns human readable representation of control request.
>> + * @str: buffer to return a human-readable representation of control request.
>> + * This buffer should have about 200 bytes.
>
> "about 200 bytes" is not very specific.
>
> Pass in the length so we know we don't overflow it.
agree.
>> + * @bRequestType: matches the USB bmRequestType field
>> + * @bRequest: matches the USB bRequest field
>> + * @wValue: matches the USB wValue field (CPU byte order)
>> + * @wIndex: matches the USB wIndex field (CPU byte order)
>> + * @wLength: matches the USB wLength field (CPU byte order)
>> + *
>> + * Function returns decoded, formatted and human-readable description of
>> + * control request packet.
>> + *
>> + * Important: wValue, wIndex, wLength parameters before invoking this function
>> + * should be processed by le16_to_cpu macro.
>> + */
>> +const char *usb_decode_ctrl(char *str, __u8 bRequestType, __u8 bRequest,
>> + __u16 wValue, __u16 wIndex, __u16 wLength);
>
> Why are you returning a value, isn't the data stored in str? Why not
> just return an int saying if the call succeeded or not?
There is one detail here. The usage scenario for this is for
tracepoints. When dealing with tracepoints we want to delay decoding of
the data into strings until print-time. I guess it's best to illustrate
with an example:
DECLARE_EVENT_CLASS(dwc3_log_ctrl,
TP_PROTO(struct usb_ctrlrequest *ctrl),
TP_ARGS(ctrl),
TP_STRUCT__entry(
__field(__u8, bRequestType)
__field(__u8, bRequest)
__field(__u16, wValue)
__field(__u16, wIndex)
__field(__u16, wLength)
__dynamic_array(char, str, DWC3_MSG_MAX)
),
TP_fast_assign(
__entry->bRequestType = ctrl->bRequestType;
__entry->bRequest = ctrl->bRequest;
__entry->wValue = le16_to_cpu(ctrl->wValue);
__entry->wIndex = le16_to_cpu(ctrl->wIndex);
__entry->wLength = le16_to_cpu(ctrl->wLength);
),
TP_printk("%s", dwc3_decode_ctrl(__get_str(str), DWC3_MSG_MAX,
__entry->bRequestType,
__entry->bRequest, __entry->wValue,
__entry->wIndex, __entry->wLength)
)
);
The above is the code is today (well, I've added buffer size as an
argument). If I make dwc3_decode_ctrl() return an integer, I can't call
it from TP_printk() time. I'd have to move it to TP_fast_assign() time
which is supposed to be, simply, a copy of the necessary data. IOW, I
would have this:
DECLARE_EVENT_CLASS(dwc3_log_ctrl,
TP_PROTO(struct usb_ctrlrequest *ctrl),
TP_ARGS(ctrl),
TP_STRUCT__entry(
__dynamic_array(char, str, DWC3_MSG_MAX)
),
TP_fast_assign(
dwc3_decode_ctrl(__get_str(str), DWC3_MSG_MAX,
ctrl->bRequestType,
ctrl->bRequest,
le16_to_cpu(ctrl->wValue),
le16_to_cpu(ctrl->wIndex),
le16_to_cpu(ctrl->wLength));
),
TP_printk("%s", __get_str(str)
)
);
Essentially, we end up moving decoding of the tracepoint to the time it
is captured; IOW, we reintroduce regular latency of string formatting.
What we could do, is move all functions called by dwc3_decode_ctrl() to
return int, but leave dwc3_decode_ctrl() returning a pointer to str just
so we continue decoding the data at printing time.
--
balbi
On Mon, Feb 04, 2019 at 04:17:00PM +0200, Felipe Balbi wrote:
>
> Hi,
>
> Greg KH <[email protected]> writes:
> > On Thu, Jan 31, 2019 at 11:52:29AM +0000, Pawel Laszczak wrote:
> >> Patch moves some decoding functions from driver/usb/dwc3/debug.h driver
> >> to driver/usb/common/debug.c file. These moved functions include:
> >> dwc3_decode_get_status
> >> dwc3_decode_set_clear_feature
> >> dwc3_decode_set_address
> >> dwc3_decode_get_set_descriptor
> >> dwc3_decode_get_configuration
> >> dwc3_decode_set_configuration
> >> dwc3_decode_get_intf
> >> dwc3_decode_set_intf
> >> dwc3_decode_synch_frame
> >> dwc3_decode_set_sel
> >> dwc3_decode_set_isoch_delay
> >> dwc3_decode_ctrl
> >>
> >> These functions are used also in inroduced cdns3 driver.
> >>
> >> All functions prefixes were changed from dwc3 to usb.
> >
> > Ick, why?
>
> moving dwc3-specific code into generic code.
That says what it does, but not why :)
And if this really was just moving things around, why was only one
symbol needed to be exported and not all of them?
> >> + * @bRequestType: matches the USB bmRequestType field
> >> + * @bRequest: matches the USB bRequest field
> >> + * @wValue: matches the USB wValue field (CPU byte order)
> >> + * @wIndex: matches the USB wIndex field (CPU byte order)
> >> + * @wLength: matches the USB wLength field (CPU byte order)
> >> + *
> >> + * Function returns decoded, formatted and human-readable description of
> >> + * control request packet.
> >> + *
> >> + * Important: wValue, wIndex, wLength parameters before invoking this function
> >> + * should be processed by le16_to_cpu macro.
> >> + */
> >> +const char *usb_decode_ctrl(char *str, __u8 bRequestType, __u8 bRequest,
> >> + __u16 wValue, __u16 wIndex, __u16 wLength);
> >
> > Why are you returning a value, isn't the data stored in str? Why not
> > just return an int saying if the call succeeded or not?
>
> There is one detail here. The usage scenario for this is for
> tracepoints. When dealing with tracepoints we want to delay decoding of
> the data into strings until print-time. I guess it's best to illustrate
> with an example:
>
> DECLARE_EVENT_CLASS(dwc3_log_ctrl,
> TP_PROTO(struct usb_ctrlrequest *ctrl),
> TP_ARGS(ctrl),
> TP_STRUCT__entry(
> __field(__u8, bRequestType)
> __field(__u8, bRequest)
> __field(__u16, wValue)
> __field(__u16, wIndex)
> __field(__u16, wLength)
> __dynamic_array(char, str, DWC3_MSG_MAX)
> ),
> TP_fast_assign(
> __entry->bRequestType = ctrl->bRequestType;
> __entry->bRequest = ctrl->bRequest;
> __entry->wValue = le16_to_cpu(ctrl->wValue);
> __entry->wIndex = le16_to_cpu(ctrl->wIndex);
> __entry->wLength = le16_to_cpu(ctrl->wLength);
> ),
> TP_printk("%s", dwc3_decode_ctrl(__get_str(str), DWC3_MSG_MAX,
> __entry->bRequestType,
> __entry->bRequest, __entry->wValue,
> __entry->wIndex, __entry->wLength)
> )
> );
>
> The above is the code is today (well, I've added buffer size as an
> argument). If I make dwc3_decode_ctrl() return an integer, I can't call
> it from TP_printk() time. I'd have to move it to TP_fast_assign() time
> which is supposed to be, simply, a copy of the necessary data. IOW, I
> would have this:
>
> DECLARE_EVENT_CLASS(dwc3_log_ctrl,
> TP_PROTO(struct usb_ctrlrequest *ctrl),
> TP_ARGS(ctrl),
> TP_STRUCT__entry(
> __dynamic_array(char, str, DWC3_MSG_MAX)
> ),
> TP_fast_assign(
> dwc3_decode_ctrl(__get_str(str), DWC3_MSG_MAX,
> ctrl->bRequestType,
> ctrl->bRequest,
> le16_to_cpu(ctrl->wValue),
> le16_to_cpu(ctrl->wIndex),
> le16_to_cpu(ctrl->wLength));
> ),
> TP_printk("%s", __get_str(str)
> )
> );
>
> Essentially, we end up moving decoding of the tracepoint to the time it
> is captured; IOW, we reintroduce regular latency of string formatting.
>
> What we could do, is move all functions called by dwc3_decode_ctrl() to
> return int, but leave dwc3_decode_ctrl() returning a pointer to str just
> so we continue decoding the data at printing time.
Ok, it wasn't obvious that this was used in a tracepoint like this, that
makes more sense.
So, it should be documented as well :)
thanks,
greg k-h
H Greg
>
>On Thu, Jan 31, 2019 at 11:52:29AM +0000, Pawel Laszczak wrote:
>> Patch moves some decoding functions from driver/usb/dwc3/debug.h driver
>> to driver/usb/common/debug.c file. These moved functions include:
>> dwc3_decode_get_status
>> dwc3_decode_set_clear_feature
>> dwc3_decode_set_address
>> dwc3_decode_get_set_descriptor
>> dwc3_decode_get_configuration
>> dwc3_decode_set_configuration
>> dwc3_decode_get_intf
>> dwc3_decode_set_intf
>> dwc3_decode_synch_frame
>> dwc3_decode_set_sel
>> dwc3_decode_set_isoch_delay
>> dwc3_decode_ctrl
>>
>> These functions are used also in inroduced cdns3 driver.
>>
>> All functions prefixes were changed from dwc3 to usb.
>
>Ick, why?
Because CDNS3 driver in one of the previous version had implemented very similar function as dwc3 and Felipe suggested that we should
make common file with these functions. He also suggested that this function also could be used on host side.
It was the reason why I've took function from DWC driver and put it in separate file in usb/common directory.
I change only the prefix to make this function more common.
>
>> Also, function's parameters has been extended according to the name
>> of fields in standard SETUP packet.
>> Additionally, patch adds usb_decode_ctrl function to
>> include/linux/usb/ch9.h file.
>
>Why ch9.h? It's not something that is specified in the spec, it's a
>usb-specific thing :)
Why not ?
Similar as usb_state_string function from include/linux/usb/ch9.h which
" Returns human readable name for the state", the usb_decode_ctrl function
make the same but for standard USB request.
>
>Also, the api for that function is not ok. If you are going to make
>this something that the whole kernel can call, you have to fix it up:
>
>> +/**
>> + * usb_decode_ctrl - Returns human readable representation of control request.
>> + * @str: buffer to return a human-readable representation of control request.
>> + * This buffer should have about 200 bytes.
>
>"about 200 bytes" is not very specific.
>
>Pass in the length so we know we don't overflow it.
I didn't want to change to much this code, because it's not my code.
I'm not sure if I should ?
>
>> + * @bRequestType: matches the USB bmRequestType field
>> + * @bRequest: matches the USB bRequest field
>> + * @wValue: matches the USB wValue field (CPU byte order)
>> + * @wIndex: matches the USB wIndex field (CPU byte order)
>> + * @wLength: matches the USB wLength field (CPU byte order)
>> + *
>> + * Function returns decoded, formatted and human-readable description of
>> + * control request packet.
>> + *
>> + * Important: wValue, wIndex, wLength parameters before invoking this function
>> + * should be processed by le16_to_cpu macro.
>> + */
>> +const char *usb_decode_ctrl(char *str, __u8 bRequestType, __u8 bRequest,
>> + __u16 wValue, __u16 wIndex, __u16 wLength);
>
>Why are you returning a value, isn't the data stored in str? Why not
>just return an int saying if the call succeeded or not?
Currently this function is called only from trace point, and probably it will be use only
In this way.
If function prototype looks like above, we can simply call it in following way:
TP_printk("%s", usb_decode_ctrl(__get_str(str), __entry->bRequestType,
__entry->bRequest, __entry->wValue,
__entry->wIndex, __entry->wLength)
Thanks
Pawel laszczak
Hi,
Pawel Laszczak <[email protected]> writes:
>>On Thu, Jan 31, 2019 at 11:52:29AM +0000, Pawel Laszczak wrote:
>>> Patch moves some decoding functions from driver/usb/dwc3/debug.h driver
>>> to driver/usb/common/debug.c file. These moved functions include:
>>> dwc3_decode_get_status
>>> dwc3_decode_set_clear_feature
>>> dwc3_decode_set_address
>>> dwc3_decode_get_set_descriptor
>>> dwc3_decode_get_configuration
>>> dwc3_decode_set_configuration
>>> dwc3_decode_get_intf
>>> dwc3_decode_set_intf
>>> dwc3_decode_synch_frame
>>> dwc3_decode_set_sel
>>> dwc3_decode_set_isoch_delay
>>> dwc3_decode_ctrl
>>>
>>> These functions are used also in inroduced cdns3 driver.
>>>
>>> All functions prefixes were changed from dwc3 to usb.
>>
>>Ick, why?
>
> Because CDNS3 driver in one of the previous version had implemented very similar function as dwc3 and Felipe suggested that we should
> make common file with these functions. He also suggested that this function also could be used on host side.
right, host controllers can make use of Control transfer decoding.a
>>> Also, function's parameters has been extended according to the name
>>> of fields in standard SETUP packet.
>>> Additionally, patch adds usb_decode_ctrl function to
>>> include/linux/usb/ch9.h file.
>>
>>Why ch9.h? It's not something that is specified in the spec, it's a
>>usb-specific thing :)
> Why not ?
>
> Similar as usb_state_string function from include/linux/usb/ch9.h which
> " Returns human readable name for the state", the usb_decode_ctrl function
> make the same but for standard USB request.
right, I would say usb_state_string() should be moved elsewhere. ch9.h
is, really, just what's described in chapter 9 of the usb specification.
>>Also, the api for that function is not ok. If you are going to make
>>this something that the whole kernel can call, you have to fix it up:
>>
>>> +/**
>>> + * usb_decode_ctrl - Returns human readable representation of control request.
>>> + * @str: buffer to return a human-readable representation of control request.
>>> + * This buffer should have about 200 bytes.
>>
>>"about 200 bytes" is not very specific.
>>
>>Pass in the length so we know we don't overflow it.
>
> I didn't want to change to much this code, because it's not my code.
> I'm not sure if I should ?
I have a patch for that, then you can start on top of it. I'll send it
soon after testing.
--
balbi
>On Thu, Jan 31, 2019 at 11:52:31AM +0000, Pawel Laszczak wrote:
>> Patch moves switch responsible for decoding descriptor type
>> outside snprintf. It's little improves code readability.
>
>Should that last sentence read: "It improves code readability a little"?
>
Yes, it should.
Thanks
Pawel
Hi,
>
>Greg KH <[email protected]> writes:
>> On Thu, Jan 31, 2019 at 11:52:29AM +0000, Pawel Laszczak wrote:
>>> Patch moves some decoding functions from driver/usb/dwc3/debug.h driver
>>> to driver/usb/common/debug.c file. These moved functions include:
>>> dwc3_decode_get_status
>>> dwc3_decode_set_clear_feature
>>> dwc3_decode_set_address
>>> dwc3_decode_get_set_descriptor
>>> dwc3_decode_get_configuration
>>> dwc3_decode_set_configuration
>>> dwc3_decode_get_intf
>>> dwc3_decode_set_intf
>>> dwc3_decode_synch_frame
>>> dwc3_decode_set_sel
>>> dwc3_decode_set_isoch_delay
>>> dwc3_decode_ctrl
>>>
>>> These functions are used also in inroduced cdns3 driver.
>>>
>>> All functions prefixes were changed from dwc3 to usb.
>>
>> Ick, why?
>
>moving dwc3-specific code into generic code.
>
>>> Also, function's parameters has been extended according to the name
>>> of fields in standard SETUP packet.
>>> Additionally, patch adds usb_decode_ctrl function to
>>> include/linux/usb/ch9.h file.
>>
>> Why ch9.h? It's not something that is specified in the spec, it's a
>> usb-specific thing :)
>
>agree.
Similar as usb_state_string function from include/linux/usb/ch9.h which
"Returns human readable name for the state", the usb_decode_ctrl function
make the same but for standard USB request.
USB Request is usb-specifing thing.
If my idea is not correct, can you suggest where this function declaration should be moved.
>
>>> +/**
>>> + * usb_decode_ctrl - Returns human readable representation of control request.
>>> + * @str: buffer to return a human-readable representation of control request.
>>> + * This buffer should have about 200 bytes.
>>
>> "about 200 bytes" is not very specific.
>>
>> Pass in the length so we know we don't overflow it.
>
>agree.
I also agree and I will add such parameter.
>
>>> + * @bRequestType: matches the USB bmRequestType field
>>> + * @bRequest: matches the USB bRequest field
>>> + * @wValue: matches the USB wValue field (CPU byte order)
>>> + * @wIndex: matches the USB wIndex field (CPU byte order)
>>> + * @wLength: matches the USB wLength field (CPU byte order)
>>> + *
>>> + * Function returns decoded, formatted and human-readable description of
>>> + * control request packet.
>>> + *
>>> + * Important: wValue, wIndex, wLength parameters before invoking this function
>>> + * should be processed by le16_to_cpu macro.
>>> + */
>>> +const char *usb_decode_ctrl(char *str, __u8 bRequestType, __u8 bRequest,
>>> + __u16 wValue, __u16 wIndex, __u16 wLength);
>>
>> Why are you returning a value, isn't the data stored in str? Why not
>> just return an int saying if the call succeeded or not?
>
>There is one detail here. The usage scenario for this is for
>tracepoints. When dealing with tracepoints we want to delay decoding of
>the data into strings until print-time. I guess it's best to illustrate
>with an example:
>
>DECLARE_EVENT_CLASS(dwc3_log_ctrl,
> TP_PROTO(struct usb_ctrlrequest *ctrl),
> TP_ARGS(ctrl),
> TP_STRUCT__entry(
> __field(__u8, bRequestType)
> __field(__u8, bRequest)
> __field(__u16, wValue)
> __field(__u16, wIndex)
> __field(__u16, wLength)
> __dynamic_array(char, str, DWC3_MSG_MAX)
> ),
> TP_fast_assign(
> __entry->bRequestType = ctrl->bRequestType;
> __entry->bRequest = ctrl->bRequest;
> __entry->wValue = le16_to_cpu(ctrl->wValue);
> __entry->wIndex = le16_to_cpu(ctrl->wIndex);
> __entry->wLength = le16_to_cpu(ctrl->wLength);
> ),
> TP_printk("%s", dwc3_decode_ctrl(__get_str(str), DWC3_MSG_MAX,
> __entry->bRequestType,
> __entry->bRequest, __entry->wValue,
> __entry->wIndex, __entry->wLength)
> )
>);
>
>The above is the code is today (well, I've added buffer size as an
>argument). If I make dwc3_decode_ctrl() return an integer, I can't call
>it from TP_printk() time. I'd have to move it to TP_fast_assign() time
>which is supposed to be, simply, a copy of the necessary data. IOW, I
>would have this:
>
>DECLARE_EVENT_CLASS(dwc3_log_ctrl,
> TP_PROTO(struct usb_ctrlrequest *ctrl),
> TP_ARGS(ctrl),
> TP_STRUCT__entry(
> __dynamic_array(char, str, DWC3_MSG_MAX)
> ),
> TP_fast_assign(
> dwc3_decode_ctrl(__get_str(str), DWC3_MSG_MAX,
> ctrl->bRequestType,
> ctrl->bRequest,
> le16_to_cpu(ctrl->wValue),
> le16_to_cpu(ctrl->wIndex),
> le16_to_cpu(ctrl->wLength));
> ),
> TP_printk("%s", __get_str(str)
> )
>);
>
>Essentially, we end up moving decoding of the tracepoint to the time it
>is captured; IOW, we reintroduce regular latency of string formatting.
>
>What we could do, is move all functions called by dwc3_decode_ctrl() to
>return int, but leave dwc3_decode_ctrl() returning a pointer to str just
>so we continue decoding the data at printing time.
I will try to change these functions in this way.
>
>--
Thanks
Pawel
Hi,
Pawel Laszczak <[email protected]> writes:
>>>> +/**
>>>> + * usb_decode_ctrl - Returns human readable representation of control request.
>>>> + * @str: buffer to return a human-readable representation of control request.
>>>> + * This buffer should have about 200 bytes.
>>>
>>> "about 200 bytes" is not very specific.
>>>
>>> Pass in the length so we know we don't overflow it.
>>
>>agree.
> I also agree and I will add such parameter.
https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git/commit/?h=next&id=7790b3556fccc555ae422f1576e97bf34c8ab8b6
--
balbi
>
>Pawel Laszczak <[email protected]> writes:
>>>>> +/**
>>>>> + * usb_decode_ctrl - Returns human readable representation of control request.
>>>>> + * @str: buffer to return a human-readable representation of control request.
>>>>> + * This buffer should have about 200 bytes.
>>>>
>>>> "about 200 bytes" is not very specific.
>>>>
>>>> Pass in the length so we know we don't overflow it.
>>>
>>>agree.
>> I also agree and I will add such parameter.
>
>https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git/commit/?h=next&id=7790b3556fccc555ae422f1576e97bf34c8ab8b6
Cool :). Thanks.
--
Pawel
On Thu, Jan 31, 2019 at 11:52:28AM +0000, Pawel Laszczak wrote:
> This patch aim at documenting USB related dt-bindings for the
> Cadence USBSS-DRD controller.
>
> Signed-off-by: Pawel Laszczak <[email protected]>
> ---
> .../devicetree/bindings/usb/cdns-usb3.txt | 33 +++++++++++++++++++
> 1 file changed, 33 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/usb/cdns-usb3.txt
>
> diff --git a/Documentation/devicetree/bindings/usb/cdns-usb3.txt b/Documentation/devicetree/bindings/usb/cdns-usb3.txt
> new file mode 100644
> index 000000000000..6dc38be77f5c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/cdns-usb3.txt
> @@ -0,0 +1,33 @@
> +Binding for the Cadence USBSS-DRD controller
> +
> +Required properties:
> + - reg: Physical base address and size of the controller's register areas.
> + Controller has 3 different regions:
> + region 1 - HOST registers area
> + region 2 - DEVICE registers area
> + region 3 - OTG/DRD registers area
> + - reg-names - register memory area names:
> + "xhci" - for HOST registers space
> + "dev" - for DEVICE registers space
> + "otg" - for OTG/DRD registers space
> + - compatible: Should contain: "cdns,usb3-1.0.0" or "cdns,usb3-1.0.1"
> + - interrupts: Interrupt specifier. Refer to interrupt bindings.
> + Driver supports only single interrupt line.
Driver supports or h/w only has 1 interrupt?
> + This single interrupt is shared between Device,
> + host and OTG/DRD part of driver.
> +
> +Optional properties:
> + - maximum-speed : valid arguments are "super-speed", "high-speed" and
> + "full-speed"; refer to usb/generic.txt
> + - dr_mode: Should be one of "host", "peripheral" or "otg".
> + - phys: reference to the USB PHY
> +
> +Example:
> + usb@f3000000 {
> + compatible = "cdns,usb3";
Doesn't match above.
> + interrupts = <USB_IRQ 7 IRQ_TYPE_LEVEL_HIGH>;
> + reg = <0xf3000000 0x10000 /* memory area for HOST registers */
> + 0xf3010000 0x10000 /* memory area for DEVICE registers */
> + 0xf3020000 0x10000>; /* memory area for OTG/DRD registers */
Are these really 64KB long? That wastes virtual address space on 32-bit
systems if not.
> + reg-names = "xhci", "dev", "otg";
> + };
> --
> 2.17.1
>
Hi Rob,
>On Thu, Jan 31, 2019 at 11:52:28AM +0000, Pawel Laszczak wrote:
>> This patch aim at documenting USB related dt-bindings for the
>> Cadence USBSS-DRD controller.
>>
>> Signed-off-by: Pawel Laszczak <[email protected]>
>> ---
>> .../devicetree/bindings/usb/cdns-usb3.txt | 33 +++++++++++++++++++
>> 1 file changed, 33 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/usb/cdns-usb3.txt
>>
>> diff --git a/Documentation/devicetree/bindings/usb/cdns-usb3.txt b/Documentation/devicetree/bindings/usb/cdns-usb3.txt
>> new file mode 100644
>> index 000000000000..6dc38be77f5c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/cdns-usb3.txt
>> @@ -0,0 +1,33 @@
>> +Binding for the Cadence USBSS-DRD controller
>> +
>> +Required properties:
>> + - reg: Physical base address and size of the controller's register areas.
>> + Controller has 3 different regions:
>> + region 1 - HOST registers area
>> + region 2 - DEVICE registers area
>> + region 3 - OTG/DRD registers area
>> + - reg-names - register memory area names:
>> + "xhci" - for HOST registers space
>> + "dev" - for DEVICE registers space
>> + "otg" - for OTG/DRD registers space
>> + - compatible: Should contain: "cdns,usb3-1.0.0" or "cdns,usb3-1.0.1"
>> + - interrupts: Interrupt specifier. Refer to interrupt bindings.
>> + Driver supports only single interrupt line.
>
>Driver supports or h/w only has 1 interrupt?
h/w internally has 3 separate interrupt line, but in my testing board
they are ored. So physically driver sees only one h/w interrupt line and
assumes that h/w has single common interrupt line.
>
>> + This single interrupt is shared between Device,
>> + host and OTG/DRD part of driver.
>> +
>> +Optional properties:
>> + - maximum-speed : valid arguments are "super-speed", "high-speed" and
>> + "full-speed"; refer to usb/generic.txt
>> + - dr_mode: Should be one of "host", "peripheral" or "otg".
>> + - phys: reference to the USB PHY
>> +
>> +Example:
>> + usb@f3000000 {
>> + compatible = "cdns,usb3";
>
>Doesn't match above.
Right, should be:
Compatible = " cdns,usb3-1.0.0", "cdns,usb3-1.0.1";
>
>> + interrupts = <USB_IRQ 7 IRQ_TYPE_LEVEL_HIGH>;
>> + reg = <0xf3000000 0x10000 /* memory area for HOST registers */
>> + 0xf3010000 0x10000 /* memory area for DEVICE registers */
>> + 0xf3020000 0x10000>; /* memory area for OTG/DRD registers */
>
>Are these really 64KB long? That wastes virtual address space on 32-bit
>systems if not.
Yes, on my testing platform it takes 64KB. It's only example, and it could be limited.
>
>> + reg-names = "xhci", "dev", "otg";
>> + };
>> --
>> 2.17.1
>>
Thanks
Pawel
On Thu, Feb 14, 2019 at 12:08 AM Pawel Laszczak <[email protected]> wrote:
>
> Hi Rob,
> >On Thu, Jan 31, 2019 at 11:52:28AM +0000, Pawel Laszczak wrote:
> >> This patch aim at documenting USB related dt-bindings for the
> >> Cadence USBSS-DRD controller.
> >>
> >> Signed-off-by: Pawel Laszczak <[email protected]>
> >> ---
> >> .../devicetree/bindings/usb/cdns-usb3.txt | 33 +++++++++++++++++++
> >> 1 file changed, 33 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/usb/cdns-usb3.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/usb/cdns-usb3.txt b/Documentation/devicetree/bindings/usb/cdns-usb3.txt
> >> new file mode 100644
> >> index 000000000000..6dc38be77f5c
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/usb/cdns-usb3.txt
> >> @@ -0,0 +1,33 @@
> >> +Binding for the Cadence USBSS-DRD controller
> >> +
> >> +Required properties:
> >> + - reg: Physical base address and size of the controller's register areas.
> >> + Controller has 3 different regions:
> >> + region 1 - HOST registers area
> >> + region 2 - DEVICE registers area
> >> + region 3 - OTG/DRD registers area
> >> + - reg-names - register memory area names:
> >> + "xhci" - for HOST registers space
> >> + "dev" - for DEVICE registers space
> >> + "otg" - for OTG/DRD registers space
> >> + - compatible: Should contain: "cdns,usb3-1.0.0" or "cdns,usb3-1.0.1"
> >> + - interrupts: Interrupt specifier. Refer to interrupt bindings.
> >> + Driver supports only single interrupt line.
> >
> >Driver supports or h/w only has 1 interrupt?
>
> h/w internally has 3 separate interrupt line, but in my testing board
> they are ored. So physically driver sees only one h/w interrupt line and
> assumes that h/w has single common interrupt line.
If it is possible for others to have 3 separate lines then you should
have 3. Just repeat the connection 3 times. Or the binding can support
1 or 3 interrupts based on the h/w. Either way it's not about what the
driver supports, but what the h/w looks like.
> >> + This single interrupt is shared between Device,
> >> + host and OTG/DRD part of driver.
> >> +
> >> +Optional properties:
> >> + - maximum-speed : valid arguments are "super-speed", "high-speed" and
> >> + "full-speed"; refer to usb/generic.txt
> >> + - dr_mode: Should be one of "host", "peripheral" or "otg".
> >> + - phys: reference to the USB PHY
> >> +
> >> +Example:
> >> + usb@f3000000 {
> >> + compatible = "cdns,usb3";
> >
> >Doesn't match above.
> Right, should be:
> Compatible = " cdns,usb3-1.0.0", "cdns,usb3-1.0.1";
No, that doesn't match either. Above you said "or".
Rob
> >
> > Hi Rob,
> > >On Thu, Jan 31, 2019 at 11:52:28AM +0000, Pawel Laszczak wrote:
> > >> This patch aim at documenting USB related dt-bindings for the
> > >> Cadence USBSS-DRD controller.
> > >>
> > >> Signed-off-by: Pawel Laszczak <[email protected]>
> > >> ---
> > >> .../devicetree/bindings/usb/cdns-usb3.txt | 33 +++++++++++++++++++
> > >> 1 file changed, 33 insertions(+)
> > >> create mode 100644
> > >> Documentation/devicetree/bindings/usb/cdns-usb3.txt
> > >>
> > >> diff --git a/Documentation/devicetree/bindings/usb/cdns-usb3.txt
> > >> b/Documentation/devicetree/bindings/usb/cdns-usb3.txt
> > >> new file mode 100644
> > >> index 000000000000..6dc38be77f5c
> > >> --- /dev/null
> > >> +++ b/Documentation/devicetree/bindings/usb/cdns-usb3.txt
> > >> @@ -0,0 +1,33 @@
> > >> +Binding for the Cadence USBSS-DRD controller
> > >> +
> > >> +Required properties:
> > >> + - reg: Physical base address and size of the controller's register areas.
> > >> + Controller has 3 different regions:
> > >> + region 1 - HOST registers area
> > >> + region 2 - DEVICE registers area
> > >> + region 3 - OTG/DRD registers area
> > >> + - reg-names - register memory area names:
> > >> + "xhci" - for HOST registers space
> > >> + "dev" - for DEVICE registers space
> > >> + "otg" - for OTG/DRD registers space
> > >> + - compatible: Should contain: "cdns,usb3-1.0.0" or "cdns,usb3-1.0.1"
> > >> + - interrupts: Interrupt specifier. Refer to interrupt bindings.
> > >> + Driver supports only single interrupt line.
> > >
> > >Driver supports or h/w only has 1 interrupt?
> >
> > h/w internally has 3 separate interrupt line, but in my testing board
> > they are ored. So physically driver sees only one h/w interrupt line
> > and assumes that h/w has single common interrupt line.
>
> If it is possible for others to have 3 separate lines then you should have 3. Just
> repeat the connection 3 times. Or the binding can support
> 1 or 3 interrupts based on the h/w. Either way it's not about what the driver
> supports, but what the h/w looks like.
>
From IP level, it has three interrupt sources for "otg", "host" and "device". But from
SoC level, it usually only has one USB interrupt line, the device driver internal controls
and knows which sources from USB controller.
Peter