usb: chipidea: add one errata for revision 2.40a

PATCH In review Sanchayan Maity 3 participants 20 messages Dec 19, 2014 – Dec 25, 2014

Subsystems (2)

USB Chipidea USB high speed dual role controller

lore

Share & tools

lore

Sanchayan Maity

author

2014-12-19 09:55 UTC #0 lore raw

cover usb: chipidea: add one errata for revision 2.40a

The first two patches add identification register API's. These can be used to get controller's revision.

The third patch implements an errata for revision 2.40a. Not sure which other SOCs implement this version of the Chipidea core but this fixes the usb client issue observed on Vybrids. The patch was tested on a Toradex Colibri VF61 module with the 3.18 kernel. iperf tests ran for three hours plus, with these patches applied have found the USB client connection to be now reliable.

This patchset is based off on Shawn Guo's for-next branch
https://git.kernel.org/cgit/linux/kernel/git/shawnguo/linux.git/tree/?h=for-next

The credit for the patches and fix goes to Matthieu Castet and Peter Chen. First two patches are by Peter Chen and the third patch which fixed the bug we observed was reported by Matthieu Castet.

The discussion of the problem and the relevant testing details can be found at this link: http://www.spinics.net/lists/linux-usb/msg118544.html

The first version of this patchset originally send by Peter Chen can be found at this link: http://www.spinics.net/lists/linux-usb/msg118753.html

Comments for review are welcome :).
Note: I am going on a vacation so will not be able to reply or do any further tests till Monday. Will attend and take care of any comments/requests for further changes/testing from Tuesday. Apologize for the delay in advance.

Sanchayan Maity (3):
usb: chipidea: Add identification registers access APIs
usb: chipidea: Add chipidea revision information
usb: chipidea: Add errata for revision 2.40a

 drivers/usb/chipidea/bits.h |   10 ++++++++
 drivers/usb/chipidea/ci.h   |   53 +++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/chipidea/core.c |   23 +++++++++++++++++--
 drivers/usb/chipidea/udc.c  |   20 ++++++++++++++++
 4 files changed, 104 insertions(+), 2 deletions(-)

Signature · 1 line

1.7.9.5

Sanchayan Maity

author

2014-12-19 09:55 UTC #1 lore raw

PATCH 1/3 usb: chipidea: Add identification registers access APIs

Using hw_write_id_reg and hw_read_id_reg to write and read identification registers contents. This can be used to get controller information, change some system configurations and so on.

Signed-off-by Sanchayan Maity <[email protected]>

---
 drivers/usb/chipidea/ci.h |   53 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

drivers/usb/chipidea/ci.h +53 -0

index ea40626..94db636 100644
@@ -29,6 +29,15 @@
 /******************************************************************************
  * REGISTERS
  *****************************************************************************/
+/* Identification Registers */
+#define ID_ID				0x0
+#define ID_HWGENERAL			0x4
+#define ID_HWHOST			0x8
+#define ID_HWDEVICE			0xc
+#define ID_HWTXBUF			0x10
+#define ID_HWRXBUF			0x14
+#define ID_SBUSCFG			0x90
+
 /* register indices */
 enum ci_hw_regs {
 	CAP_CAPLENGTH,

@@ -97,6 +106,18 @@ enum ci_role {
 	CI_ROLE_END,
 };
 
+enum CI_REVISION {
+	CI_REVISION_1X = 10,	/* Revision 1.x */
+	CI_REVISION_20 = 20, /* Revision 2.0 */
+	CI_REVISION_21, /* Revision 2.1 */
+	CI_REVISION_22, /* Revision 2.2 */
+	CI_REVISION_23, /* Revision 2.3 */
+	CI_REVISION_24, /* Revision 2.4 */
+	CI_REVISION_25, /* Revision 2.5 */
+	CI_REVISION_25_PLUS, /* Revision above than 2.5 */
+	CI_REVISION_UNKNOWN = 99, /* Unknown Revision */
+};
+
 /**
  * struct ci_role_driver - host/gadget role driver
  * @start: start this role

@@ -168,6 +189,7 @@ struct hw_bank {
  * @b_sess_valid_event: indicates there is a vbus event, and handled
  * at ci_otg_work
  * @imx28_write_fix: Freescale imx28 needs swp instruction for writing
+ * @rev: The revision number for controller
  */
 struct ci_hdrc {
 	struct device			*dev;

@@ -207,6 +229,7 @@ struct ci_hdrc {
 	bool				id_event;
 	bool				b_sess_valid_event;
 	bool				imx28_write_fix;
+	enum CI_REVISION		rev;
 };
 
 static inline struct ci_role_driver *ci_role(struct ci_hdrc *ci)

@@ -244,6 +267,36 @@ static inline void ci_role_stop(struct ci_hdrc *ci)
 }
 
 /**
+ * hw_read_id_reg: reads from a identification register
+ * @ci: the controller
+ * @offset: offset from the beginning of identification registers region
+ * @mask: bitfield mask
+ *
+ * This function returns register contents
+ */
+static inline u32 hw_read_id_reg(struct ci_hdrc *ci, u32 offset, u32 mask)
+{
+	return ioread32(ci->hw_bank.abs + offset) & mask;
+}
+
+/**
+ * hw_write_id_reg: writes to a identification register
+ * @ci: the controller
+ * @offset: offset from the beginning of identification registers region
+ * @mask: bitfield mask
+ * @data: new value
+ */
+static inline void hw_write_id_reg(struct ci_hdrc *ci, u32 offset,
+			    u32 mask, u32 data)
+{
+	if (~mask)
+		data = (ioread32(ci->hw_bank.abs + offset) & ~mask)
+			| (data & mask);
+
+	iowrite32(data, ci->hw_bank.abs + offset);
+}
+
+/**
  * hw_read: reads from a hw register
  * @ci: the controller
  * @reg:  register index

Signature · 1 line

1.7.9.5

Sanchayan Maity

author

2014-12-19 09:56 UTC #8 lore raw

PATCH 2/3 usb: chipidea: Add chipidea revision information

In reply to Sanchayan Maity

Define ci_get_revision API to know the controller revision information according to chipidea 1.1a, 2.0a, 2.4 and 2.5a spec. Besides, add one entry in struct ci_hdrc to indicate revision information. This can be used for adding different code for revisions, implementing erratas.

Signed-off-by Sanchayan Maity <[email protected]>

---
 drivers/usb/chipidea/bits.h |   10 ++++++++++
 drivers/usb/chipidea/core.c |   23 +++++++++++++++++++++--
 2 files changed, 31 insertions(+), 2 deletions(-)

drivers/usb/chipidea/bits.h +10 -0

index ca57e3d..e935ccc 100644
@@ -15,6 +15,16 @@
 
 #include <linux/usb/ehci_def.h>
 
+/*
+ * ID
+ * For 1.x revision, bit24 - bit31 are reserved
+ * For 2.x revision, bit25 - bit28 are 0x2
+ */
+#define TAG			(0x1F << 16)
+#define REVISION		(0xF << 21)
+#define VERSION			(0xF << 25)
+#define CIVERSION		(0x7 << 29)
+
 /* HCCPARAMS */
 #define HCCPARAMS_LEN         BIT(17)

drivers/usb/chipidea/core.c +21 -2

index 9bdc6bd..33a8c4a 100644
@@ -136,6 +136,22 @@ static int hw_alloc_regmap(struct ci_hdrc *ci, bool is_lpm)
 	return 0;
 }
 
+static enum CI_REVISION ci_get_revision(struct ci_hdrc *ci)
+{
+	int ver = hw_read_id_reg(ci, ID_ID, VERSION) >> __ffs(VERSION);
+	enum CI_REVISION rev = CI_REVISION_UNKNOWN;
+
+	if (ver == 0x2) {
+		int rev_reg = hw_read_id_reg
+			(ci, ID_ID, REVISION) >> __ffs(REVISION);
+		rev = rev_reg + CI_REVISION_20;
+	} else if (ver == 0x0) {
+		rev = CI_REVISION_1X;
+	}
+
+	return rev;
+}
+
 /**
  * hw_read_intr_enable: returns interrupt enable register
  *

@@ -245,8 +261,11 @@ static int hw_device_init(struct ci_hdrc *ci, void __iomem *base)
 	/* Clear all interrupts status bits*/
 	hw_write(ci, OP_USBSTS, 0xffffffff, 0xffffffff);
 
-	dev_dbg(ci->dev, "ChipIdea HDRC found, lpm: %d; cap: %p op: %p\n",
-		ci->hw_bank.lpm, ci->hw_bank.cap, ci->hw_bank.op);
+	ci->rev = ci_get_revision(ci);
+
+	dev_dbg(ci->dev,
+		"ChipIdea HDRC found, revision: %d, lpm: %d; cap: %p op: %p\n",
+		ci->rev, ci->hw_bank.lpm, ci->hw_bank.cap, ci->hw_bank.op);
 
 	/* setup lock mode ? */

Signature · 1 line

1.7.9.5

Sanchayan Maity

author

2014-12-19 09:56 UTC #11 lore raw

PATCH 3/3 usb: chipidea: Add errata for revision 2.40a

In reply to Sanchayan Maity

At chipidea revision 2.40a, there is a below errata:

9000531823 B2-Medium Adding a dTD to a Primed Endpoint May Not Get Recognized

Title: Adding a dTD to a Primed Endpoint May Not Get Recognized

Impacted Configuration: All device mode configurations.

Description:
There is an issue with the add dTD tripwire semaphore (ATDTW bit in USBCMD register) that can cause the controller to ignore a dTD that is added to a primed endpoint. When this happens, the software can read the tripwire bit and the status bit at '1' even though the endpoint is unprimed.

After executing a dTD, the device controller endpoint state machine executes a final read of the dTD terminate bit to check if the application added a dTD to the linked list at the last moment. This read is done in the finpkt_read_latest_next_td (44) state. After the read is performed, if the terminate bit is still set, the state machine moves to unprime the endpoint. The decision to unprime the endpoint is done in the checkqh_decision (59) state, based on the value of the terminate bit. Before reaching the checkqh_decision state, the state machine traverses the writeqhtd_status (57), writeqh_status (56), and release_prime_mask (42) states. As shown in the waveform, the ep_addtd_tripwire_clr signal is not set to clear the tripwire bit in these states.

Workaround:
The software must implement a periodic poll cycle, and check for each dTD pending on execution (Active = 1), if the enpoint is primed. It can do this by reading the corresponding bits in the ENDPTPRIME and ENDPTSTAT registers. If these bits are read at 0, the software needs to re-prime the endpoint by writing 1 to the corresponding bit in the ENDPTPRIME register. This can be done for every microframe, every frame or with a larger interval, depending on the urgency of transfer execution for the application.

Signed-off-by Sanchayan Maity <[email protected]>

---
 drivers/usb/chipidea/udc.c |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

drivers/usb/chipidea/udc.c +20 -0

index 0444d3f..551ab37 100644
@@ -522,6 +522,20 @@ static void free_pending_td(struct ci_hw_ep *hwep)
 	kfree(pending);
 }
 
+static int reprime_dtd(struct ci_hdrc *ci, struct ci_hw_ep *hwep,
+					   struct td_node *node)
+{
+	hwep->qh.ptr->td.next = node->dma;
+	hwep->qh.ptr->td.token &=
+		cpu_to_le32(~(TD_STATUS_HALTED | TD_STATUS_ACTIVE));
+
+	/* Synchronize before ep prime */
+	wmb();
+
+	return hw_ep_prime(ci, hwep->num, hwep->dir,
+				hwep->type == USB_ENDPOINT_XFER_CONTROL);
+}
+
 /**
  * _hardware_dequeue: handles a request at hardware level
  * @gadget: gadget

@@ -535,6 +549,7 @@ static int _hardware_dequeue(struct ci_hw_ep *hwep, struct ci_hw_req *hwreq)
 	struct td_node *node, *tmpnode;
 	unsigned remaining_length;
 	unsigned actual = hwreq->req.length;
+	struct ci_hdrc *ci = hwep->ci;
 
 	if (hwreq->req.status != -EALREADY)
 		return -EINVAL;

@@ -544,6 +559,11 @@ static int _hardware_dequeue(struct ci_hw_ep *hwep, struct ci_hw_req *hwreq)
 	list_for_each_entry_safe(node, tmpnode, &hwreq->tds, td) {
 		tmptoken = le32_to_cpu(node->ptr->token);
 		if ((TD_STATUS_ACTIVE & tmptoken) != 0) {
+			int n = hw_ep_bit(hwep->num, hwep->dir);
+
+			if (ci->rev == CI_REVISION_24)
+				if (!hw_read(ci, OP_ENDPTSTAT, BIT(n)))
+					reprime_dtd(ci, hwep, node);
 			hwreq->req.status = -EALREADY;
 			return -EBUSY;
 		}

Signature · 1 line

1.7.9.5

Peter Chen

2014-12-22 02:27 UTC #12 lore raw

In reply to Sanchayan Maity

Quoted 9 lines · Sanchayan Maity

The first two patches add identification register API's. These can be used to get controller's revision.
The third patch implements an errata for revision 2.40a. Not sure which other SOCs implement this version of the Chipidea core but this fixes the usb client issue observed on Vybrids. The patch was tested on a Toradex Colibri VF61 module with the 3.18 kernel. iperf tests ran for three hours plus, with these patches applied have found the USB client connection to be now reliable.

Would you help do a overnight test? It is passed, I will queue them, thanks.

Peter

Quoted 33 lines

This patchset is based off on Shawn Guo's for-next branch
https://git.kernel.org/cgit/linux/kernel/git/shawnguo/linux.git/tree/?h=for-next
The credit for the patches and fix goes to Matthieu Castet and Peter Chen. First two patches are by Peter Chen and the third patch which fixed the bug we observed was reported by Matthieu Castet.
The discussion of the problem and the relevant testing details can be found at this link: http://www.spinics.net/lists/linux-usb/msg118544.html
The first version of this patchset originally send by Peter Chen can be found at this link: http://www.spinics.net/lists/linux-usb/msg118753.html
Comments for review are welcome :).
Note: I am going on a vacation so will not be able to reply or do any further tests till Monday. Will attend and take care of any comments/requests for further changes/testing from Tuesday. Apologize for the delay in advance.
Sanchayan Maity (3):
usb: chipidea: Add identification registers access APIs
usb: chipidea: Add chipidea revision information
usb: chipidea: Add errata for revision 2.40a
 drivers/usb/chipidea/bits.h |   10 ++++++++
 drivers/usb/chipidea/ci.h   |   53 +++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/chipidea/core.c |   23 +++++++++++++++++--
 drivers/usb/chipidea/udc.c  |   20 ++++++++++++++++
 4 files changed, 104 insertions(+), 2 deletions(-)
Signature · 1 line
1.7.9.5

Signature · 2 lines

Best Regards,
Peter Chen

Sanchayan Maity

author

2014-12-22 13:10 UTC #13 lore raw

Quoted 11 lines · Peter Chen

Quoted 9 lines · Sanchayan Maity
The first two patches add identification register API's. These can be used to get controller's revision.
The third patch implements an errata for revision 2.40a. Not sure which other SOCs implement this version of the Chipidea core but this fixes the usb client issue observed on Vybrids. The patch was tested on a Toradex Colibri VF61 module with the 3.18 kernel. iperf tests ran for three hours plus, with these patches applied have found the USB client connection to be now reliable.

Quoted 2 lines

Would you help do a overnight test? It is passed, I will queue them, thanks.

Yes definitely I can help with the testing. Are you looking for iperf tests only or something else? iperf tests running for 12 or 24 hours will do? I will need a bit of time to set things up here, as I am away from work, but, ya I will do them.

-Regards,
Sanchayan.

Quoted 36 lines

Peter
Quoted 33 lines
This patchset is based off on Shawn Guo's for-next branch
https://git.kernel.org/cgit/linux/kernel/git/shawnguo/linux.git/tree/?h=for-next
The credit for the patches and fix goes to Matthieu Castet and Peter Chen. First two patches are by Peter Chen and the third patch which fixed the bug we observed was reported by Matthieu Castet.
The discussion of the problem and the relevant testing details can be found at this link: http://www.spinics.net/lists/linux-usb/msg118544.html
The first version of this patchset originally send by Peter Chen can be found at this link: http://www.spinics.net/lists/linux-usb/msg118753.html
Comments for review are welcome :).
Note: I am going on a vacation so will not be able to reply or do any further tests till Monday. Will attend and take care of any comments/requests for further changes/testing from Tuesday. Apologize for the delay in advance.
Sanchayan Maity (3):
usb: chipidea: Add identification registers access APIs
usb: chipidea: Add chipidea revision information
usb: chipidea: Add errata for revision 2.40a
 drivers/usb/chipidea/bits.h |   10 ++++++++
 drivers/usb/chipidea/ci.h   |   53 +++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/chipidea/core.c |   23 +++++++++++++++++--
 drivers/usb/chipidea/udc.c  |   20 ++++++++++++++++
 4 files changed, 104 insertions(+), 2 deletions(-)
Signature · 1 line
1.7.9.5

Peter Chen

2014-12-23 01:33 UTC #14 lore raw

Quoted 12 lines · Sanchayan Maity

Quoted 11 lines · Peter Chen
Quoted 9 lines · Sanchayan Maity
The first two patches add identification register API's. These can be used to get controller's revision.
The third patch implements an errata for revision 2.40a. Not sure which other SOCs implement this version of the Chipidea core but this fixes the usb client issue observed on Vybrids. The patch was tested on a Toradex Colibri VF61 module with the 3.18 kernel. iperf tests ran for three hours plus, with these patches applied have found the USB client connection to be now reliable.

Quoted 8 lines

Quoted 2 lines
Would you help do a overnight test? It is passed, I will queue them, thanks.
Yes definitely I can help with the testing. Are you looking for iperf tests only or something else? iperf tests running for 12 or 24 hours will do? I will need a bit of time to set things up here, as I am away from work, but, ya I will do them.

iperf for g_ncm or g_ether and bonnie++ for g_mass_storage if you can run, thanks.

Peter

Quoted 38 lines

-Regards,
Sanchayan.
Quoted 36 lines
Peter
Quoted 33 lines
This patchset is based off on Shawn Guo's for-next branch
https://git.kernel.org/cgit/linux/kernel/git/shawnguo/linux.git/tree/?h=for-next
The credit for the patches and fix goes to Matthieu Castet and Peter Chen. First two patches are by Peter Chen and the third patch which fixed the bug we observed was reported by Matthieu Castet.
The discussion of the problem and the relevant testing details can be found at this link: http://www.spinics.net/lists/linux-usb/msg118544.html
The first version of this patchset originally send by Peter Chen can be found at this link: http://www.spinics.net/lists/linux-usb/msg118753.html
Comments for review are welcome :).
Note: I am going on a vacation so will not be able to reply or do any further tests till Monday. Will attend and take care of any comments/requests for further changes/testing from Tuesday. Apologize for the delay in advance.
Sanchayan Maity (3):
usb: chipidea: Add identification registers access APIs
usb: chipidea: Add chipidea revision information
usb: chipidea: Add errata for revision 2.40a
 drivers/usb/chipidea/bits.h |   10 ++++++++
 drivers/usb/chipidea/ci.h   |   53 +++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/chipidea/core.c |   23 +++++++++++++++++--
 drivers/usb/chipidea/udc.c  |   20 ++++++++++++++++
 4 files changed, 104 insertions(+), 2 deletions(-)
Signature · 1 line
1.7.9.5

Signature · 2 lines

Best Regards,
Peter Chen

Sanchayan Maity

author

2014-12-23 04:12 UTC #15 lore raw

Quoted 16 lines · Peter Chen

Quoted 15 lines · Sanchayan Maity
Quoted 13 lines · Peter Chen
Quoted 9 lines · Sanchayan Maity
The first two patches add identification register API's. These can be used to get controller's revision.
The third patch implements an errata for revision 2.40a. Not sure which other SOCs implement this version of the Chipidea core but this fixes the usb client issue observed on Vybrids. The patch was tested on a Toradex Colibri VF61 module with the 3.18 kernel. iperf tests ran for three hours plus, with these patches applied have found the USB client connection to be now reliable.
Would you help do a overnight test? It is passed, I will queue them, thanks.

Quoted 8 lines

Quoted 5 lines
Yes definitely I can help with the testing. Are you looking for iperf tests only or something else? iperf tests running for 12 or 24 hours will do? I will need a bit of time to set things up here, as I am away from work, but, ya I will do them.
iperf for g_ncm or g_ether and bonnie++ for g_mass_storage if you can run, thanks.

Will run 12 hour iperf test for CDC ECM. I am not aware of bonnie++, but, will look into it. Will get back once I have the results.

Thanks.

Regards,
Sanchayan.

Quoted 42 lines

Peter
Quoted 38 lines
-Regards,
Sanchayan.
Quoted 36 lines
Peter
Quoted 33 lines
This patchset is based off on Shawn Guo's for-next branch
https://git.kernel.org/cgit/linux/kernel/git/shawnguo/linux.git/tree/?h=for-next
The credit for the patches and fix goes to Matthieu Castet and Peter Chen. First two patches are by Peter Chen and the third patch which fixed the bug we observed was reported by Matthieu Castet.
The discussion of the problem and the relevant testing details can be found at this link: http://www.spinics.net/lists/linux-usb/msg118544.html
The first version of this patchset originally send by Peter Chen can be found at this link: http://www.spinics.net/lists/linux-usb/msg118753.html
Comments for review are welcome :).
Note: I am going on a vacation so will not be able to reply or do any further tests till Monday. Will attend and take care of any comments/requests for further changes/testing from Tuesday. Apologize for the delay in advance.
Sanchayan Maity (3):
usb: chipidea: Add identification registers access APIs
usb: chipidea: Add chipidea revision information
usb: chipidea: Add errata for revision 2.40a
 drivers/usb/chipidea/bits.h |   10 ++++++++
 drivers/usb/chipidea/ci.h   |   53 +++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/chipidea/core.c |   23 +++++++++++++++++--
 drivers/usb/chipidea/udc.c  |   20 ++++++++++++++++
 4 files changed, 104 insertions(+), 2 deletions(-)
Signature · 1 line
1.7.9.5

Sanchayan Maity

author

2014-12-24 07:51 UTC #16 lore raw

In reply to Peter Chen

Quoted 16 lines · Peter Chen

Quoted 15 lines · Sanchayan Maity
Quoted 13 lines · Peter Chen
Quoted 9 lines · Sanchayan Maity
The first two patches add identification register API's. These can be used to get controller's revision.
The third patch implements an errata for revision 2.40a. Not sure which other SOCs implement this version of the Chipidea core but this fixes the usb client issue observed on Vybrids. The patch was tested on a Toradex Colibri VF61 module with the 3.18 kernel. iperf tests ran for three hours plus, with these patches applied have found the USB client connection to be now reliable.
Would you help do a overnight test? It is passed, I will queue them, thanks.

Quoted 8 lines

Quoted 5 lines
Yes definitely I can help with the testing. Are you looking for iperf tests only or something else? iperf tests running for 12 or 24 hours will do? I will need a bit of time to set things up here, as I am away from work, but, ya I will do them.
iperf for g_ncm or g_ether and bonnie++ for g_mass_storage if you can run, thanks.

The tests were run on a Toradex Colibri Vybrid VF61 module having 256MB RAM with the 3.18 kernel.

The iperf tests ran for around 19 hours before I stopped it. A snip of the iperf tests is below. Used the Ethernet Gadget class for this.

[  4] 70453.0-70453.5 sec  6.89 MBytes   116 Mbits/sec
[  4] 70453.5-70454.0 sec  6.83 MBytes   115 Mbits/sec
[  4] 70454.0-70454.5 sec  6.84 MBytes   115 Mbits/sec
[  4] 70454.5-70455.0 sec  6.89 MBytes   116 Mbits/sec
[  4] 70455.0-70455.5 sec  6.90 MBytes   116 Mbits/sec
[  4] 70455.5-70456.0 sec  6.90 MBytes   116 Mbits/sec
[  4] 70456.0-70456.5 sec  6.82 MBytes   114 Mbits/sec
[  4] 70456.5-70457.0 sec  6.80 MBytes   114 Mbits/sec
[  4] 70457.0-70457.5 sec  6.89 MBytes   116 Mbits/sec
[  4] 70457.5-70458.0 sec  6.85 MBytes   115 Mbits/sec
[  4] 70458.0-70458.5 sec  6.82 MBytes   114 Mbits/sec
[  4] 70458.5-70459.0 sec  6.82 MBytes   114 Mbits/sec
[  4]  0.0-70459.2 sec   946 GBytes   115 Mbits/sec

Ran bonnie++ on gadget mass storage. CPU usage around the time of running this test was mostly around the 90% mark with the minimum at 60% plus. The storage directory was formatted with ext4. bonnie++ version used is 1.97 and was installed from the Arch repositories with pacman.

The size of the file being specified for "lun" storage is 512MB. I have specified 128MB RAM in the below run with the size of file for IO performance as 256MB. Without this bonnie++ was giving me an error around the "Writing intelligently" point. I assume this has to do with the file size bonnie++ uses for testing.

[sanchayan@Sanchayan-Arch ~]$ sudo /usr/bin/bonnie++ -m Vybrid -r 128 -d /var/run/media/sanchayan/Vybrid/ -x 5 -u root -n 0 -s 256
Using uid:0, gid:0. format_version,bonnie_version,name,concurrency,seed,file_size,io_chunk_size,putc,putc_cpu,put_block,put_block_cpu,rewrite,rewrite_cpu,getc,getc_cpu,get_block,get_block_cpu,seeks,seeks_cpu,num_files,max_size,min_size,num_dirs,file_chunk_size,seq_create,seq_create_cpu,seq_stat,seq_stat_cpu,seq_del,seq_del_cpu,ran_create,ran_create_cpu,ran_stat,ran_stat_cpu,ran_del,ran_del_cpu,putc_latency,put_block_latency,rewrite_latency,getc_latency,get_block_latency,seeks_latency,seq_create_latency,seq_stat_latency,seq_del_latency,ran_create_latency,ran_stat_latency,ran_del_latency Writing a byte at a time...done
Writing intelligently...done
Rewriting...done
Reading a byte at a time...done
Reading intelligently...done
start 'em...done...done...done...done...done...
1.97,1.97,Vybrid,1,1419409300,256M,,659,87,8341,1,9401,0,4222,98,+++++,+++,3539,19,,,,,,,,,,,,,,,,,,23042us,66us,59us,4482us,79us,475us,,,,,,
Writing a byte at a time...done
Writing intelligently...done
Rewriting...done
Reading a byte at a time...done
Reading intelligently...done
start 'em...done...done...done...done...done...
1.97,1.97,Vybrid,1,1419409300,256M,,661,90,7689,1,9071,0,4011,99,+++++,+++,3426,20,,,,,,,,,,,,,,,,,,15406us,64us,62us,4667us,23us,10030us,,,,,,
Writing a byte at a time...done
Writing intelligently...done
Rewriting...done
Reading a byte at a time...done
Reading intelligently...done
start 'em...done...done...done...done...done...
1.97,1.97,Vybrid,1,1419409300,256M,,673,89,8117,1,9451,0,3879,98,+++++,+++,3355,22,,,,,,,,,,,,,,,,,,14210us,45us,69us,5069us,21us,10052us,,,,,,
Writing a byte at a time...done
Writing intelligently...done
Rewriting...done
Reading a byte at a time...done
Reading intelligently...done
start 'em...done...done...done...done...done...
1.97,1.97,Vybrid,1,1419409300,256M,,668,89,7801,1,9343,0,4099,98,+++++,+++,3336,16,,,,,,,,,,,,,,,,,,17019us,44us,75us,4920us,20us,10234us,,,,,,
Writing a byte at a time...done
Writing intelligently...done
Rewriting...done
Reading a byte at a time...done
Reading intelligently...done
start 'em...done...done...done...done...done...
1.97,1.97,Vybrid,1,1419409300,256M,,676,89,7953,1,9494,0,3878,98,+++++,+++,3396,22,,,,,,,,,,,,,,,,,,14080us,56us,42us,5177us,23us,12224us,,,,,,

Let me know if this is OK.

Thanks.

-Regards,
Sanchayan.

Quoted 42 lines

Peter
Quoted 38 lines
-Regards,
Sanchayan.
Quoted 36 lines
Peter
Quoted 33 lines
This patchset is based off on Shawn Guo's for-next branch
https://git.kernel.org/cgit/linux/kernel/git/shawnguo/linux.git/tree/?h=for-next
The credit for the patches and fix goes to Matthieu Castet and Peter Chen. First two patches are by Peter Chen and the third patch which fixed the bug we observed was reported by Matthieu Castet.
The discussion of the problem and the relevant testing details can be found at this link: http://www.spinics.net/lists/linux-usb/msg118544.html
The first version of this patchset originally send by Peter Chen can be found at this link: http://www.spinics.net/lists/linux-usb/msg118753.html
Comments for review are welcome :).
Note: I am going on a vacation so will not be able to reply or do any further tests till Monday. Will attend and take care of any comments/requests for further changes/testing from Tuesday. Apologize for the delay in advance.
Sanchayan Maity (3):
usb: chipidea: Add identification registers access APIs
usb: chipidea: Add chipidea revision information
usb: chipidea: Add errata for revision 2.40a
 drivers/usb/chipidea/bits.h |   10 ++++++++
 drivers/usb/chipidea/ci.h   |   53 +++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/chipidea/core.c |   23 +++++++++++++++++--
 drivers/usb/chipidea/udc.c  |   20 ++++++++++++++++
 4 files changed, 104 insertions(+), 2 deletions(-)
Signature · 1 line
1.7.9.5

Peter Chen

2014-12-24 09:00 UTC #17 lore raw

Quoted 53 lines · Peter Chen

Quoted 16 lines · Sanchayan Maity
Quoted 15 lines · Peter Chen
Quoted 13 lines · Sanchayan Maity
Quoted 9 lines
The first two patches add identification register API's. These can be used to get controller's revision.
The third patch implements an errata for revision 2.40a. Not sure which other SOCs implement this version of the Chipidea core but this fixes the usb client issue observed on Vybrids. The patch was tested on a Toradex Colibri VF61 module with the 3.18 kernel. iperf tests ran for three hours plus, with these patches applied have found the USB client connection to be now reliable.
Would you help do a overnight test? It is passed, I will queue them, thanks.
Quoted 8 lines
Quoted 5 lines
Yes definitely I can help with the testing. Are you looking for iperf tests only or something else? iperf tests running for 12 or 24 hours will do? I will need a bit of time to set things up here, as I am away from work, but, ya I will do them.
iperf for g_ncm or g_ether and bonnie++ for g_mass_storage if you can run, thanks.
The tests were run on a Toradex Colibri Vybrid VF61 module having 256MB RAM with the 3.18 kernel.
The iperf tests ran for around 19 hours before I stopped it. A snip of the iperf tests is below. Used the Ethernet Gadget class for this.
[  4] 70453.0-70453.5 sec  6.89 MBytes   116 Mbits/sec
[  4] 70453.5-70454.0 sec  6.83 MBytes   115 Mbits/sec
[  4] 70454.0-70454.5 sec  6.84 MBytes   115 Mbits/sec
[  4] 70454.5-70455.0 sec  6.89 MBytes   116 Mbits/sec
[  4] 70455.0-70455.5 sec  6.90 MBytes   116 Mbits/sec
[  4] 70455.5-70456.0 sec  6.90 MBytes   116 Mbits/sec
[  4] 70456.0-70456.5 sec  6.82 MBytes   114 Mbits/sec
[  4] 70456.5-70457.0 sec  6.80 MBytes   114 Mbits/sec
[  4] 70457.0-70457.5 sec  6.89 MBytes   116 Mbits/sec
[  4] 70457.5-70458.0 sec  6.85 MBytes   115 Mbits/sec
[  4] 70458.0-70458.5 sec  6.82 MBytes   114 Mbits/sec
[  4] 70458.5-70459.0 sec  6.82 MBytes   114 Mbits/sec
[  4]  0.0-70459.2 sec   946 GBytes   115 Mbits/sec
Ran bonnie++ on gadget mass storage. CPU usage around the time of running this test was mostly around the 90% mark with the minimum at 60% plus. The storage directory was formatted with ext4. bonnie++ version used is 1.97 and was installed from the Arch repositories with pacman.

Quoted 5 lines

The size of the file being specified for "lun" storage is 512MB. I have specified 128MB RAM in the below run with the size of file for IO performance as 256MB. Without this bonnie++ was giving me an error around the "Writing intelligently" point. I assume this has to do with the file size bonnie++ uses for testing.

What's your backfile for mass storage, mmc?
You mean without this errata, running bonnie++ will meet error?

Quoted 51 lines

[sanchayan@Sanchayan-Arch ~]$ sudo /usr/bin/bonnie++ -m Vybrid -r 128 -d /var/run/media/sanchayan/Vybrid/ -x 5 -u root -n 0 -s 256 Using uid:0, gid:0. format_version,bonnie_version,name,concurrency,seed,file_size,io_chunk_si ze,putc,putc_cpu,put_block,put_block_cpu,rewrite,rewrite_cpu,getc,getc_cp u,get_block,get_block_cpu,seeks,seeks_cpu,num_files,max_size,min_size,nu m_dirs,file_chunk_size,seq_create,seq_create_cpu,seq_stat,seq_stat_cpu,se q_del,seq_del_cpu,ran_create,ran_create_cpu,ran_stat,ran_stat_cpu,ran_del ,ran_del_cpu,putc_latency,put_block_latency,rewrite_latency,getc_latency,ge t_block_latency,seeks_latency,seq_create_latency,seq_stat_latency,seq_del_ latency,ran_create_latency,ran_stat_latency,ran_del_latency Writing a byte at a time...done
Writing intelligently...done
Rewriting...done
Reading a byte at a time...done
Reading intelligently...done
start 'em...done...done...done...done...done... 1.97,1.97,Vybrid,1,1419409300,256M,,659,87,8341,1,9401,0,4222,98,+++++,+++, 3539,19,,,,,,,,,,,,,,,,,,23042us,66us,59us,4482us,79us,475us,,,,,, Writing a byte at a time...done
Writing intelligently...done
Rewriting...done
Reading a byte at a time...done
Reading intelligently...done
start 'em...done...done...done...done...done... 1.97,1.97,Vybrid,1,1419409300,256M,,661,90,7689,1,9071,0,4011,99,+++++,+++, 3426,20,,,,,,,,,,,,,,,,,,15406us,64us,62us,4667us,23us,10030us,,,,,, Writing a byte at a time...done
Writing intelligently...done
Rewriting...done
Reading a byte at a time...done
Reading intelligently...done
start 'em...done...done...done...done...done... 1.97,1.97,Vybrid,1,1419409300,256M,,673,89,8117,1,9451,0,3879,98,+++++,+++, 3355,22,,,,,,,,,,,,,,,,,,14210us,45us,69us,5069us,21us,10052us,,,,,, Writing a byte at a time...done
Writing intelligently...done
Rewriting...done
Reading a byte at a time...done
Reading intelligently...done
start 'em...done...done...done...done...done... 1.97,1.97,Vybrid,1,1419409300,256M,,668,89,7801,1,9343,0,4099,98,+++++,+++, 3336,16,,,,,,,,,,,,,,,,,,17019us,44us,75us,4920us,20us,10234us,,,,,, Writing a byte at a time...done
Writing intelligently...done
Rewriting...done
Reading a byte at a time...done
Reading intelligently...done
start 'em...done...done...done...done...done... 1.97,1.97,Vybrid,1,1419409300,256M,,676,89,7953,1,9494,0,3878,98,+++++,+++, 3396,22,,,,,,,,,,,,,,,,,,14080us,56us,42us,5177us,23us,12224us,,,,,,

Quoted 1 line

Let me know if this is OK.

Thanks for making and testing the patch, it is really a good job. I will queue this patch in my chipidea tree.

Peter

Sanchayan Maity

author

2014-12-24 09:21 UTC #18 lore raw

Quoted 53 lines · Peter Chen

Quoted 53 lines · Peter Chen
Quoted 16 lines · Sanchayan Maity
Quoted 15 lines · Peter Chen
Quoted 13 lines · Sanchayan Maity
Quoted 9 lines
The first two patches add identification register API's. These can be used to get controller's revision.
The third patch implements an errata for revision 2.40a. Not sure which other SOCs implement this version of the Chipidea core but this fixes the usb client issue observed on Vybrids. The patch was tested on a Toradex Colibri VF61 module with the 3.18 kernel. iperf tests ran for three hours plus, with these patches applied have found the USB client connection to be now reliable.
Would you help do a overnight test? It is passed, I will queue them, thanks.
Quoted 8 lines
Quoted 5 lines
Yes definitely I can help with the testing. Are you looking for iperf tests only or something else? iperf tests running for 12 or 24 hours will do? I will need a bit of time to set things up here, as I am away from work, but, ya I will do them.
iperf for g_ncm or g_ether and bonnie++ for g_mass_storage if you can run, thanks.
The tests were run on a Toradex Colibri Vybrid VF61 module having 256MB RAM with the 3.18 kernel.
The iperf tests ran for around 19 hours before I stopped it. A snip of the iperf tests is below. Used the Ethernet Gadget class for this.
[  4] 70453.0-70453.5 sec  6.89 MBytes   116 Mbits/sec
[  4] 70453.5-70454.0 sec  6.83 MBytes   115 Mbits/sec
[  4] 70454.0-70454.5 sec  6.84 MBytes   115 Mbits/sec
[  4] 70454.5-70455.0 sec  6.89 MBytes   116 Mbits/sec
[  4] 70455.0-70455.5 sec  6.90 MBytes   116 Mbits/sec
[  4] 70455.5-70456.0 sec  6.90 MBytes   116 Mbits/sec
[  4] 70456.0-70456.5 sec  6.82 MBytes   114 Mbits/sec
[  4] 70456.5-70457.0 sec  6.80 MBytes   114 Mbits/sec
[  4] 70457.0-70457.5 sec  6.89 MBytes   116 Mbits/sec
[  4] 70457.5-70458.0 sec  6.85 MBytes   115 Mbits/sec
[  4] 70458.0-70458.5 sec  6.82 MBytes   114 Mbits/sec
[  4] 70458.5-70459.0 sec  6.82 MBytes   114 Mbits/sec
[  4]  0.0-70459.2 sec   946 GBytes   115 Mbits/sec
Ran bonnie++ on gadget mass storage. CPU usage around the time of running this test was mostly around the 90% mark with the minimum at 60% plus. The storage directory was formatted with ext4. bonnie++ version used is 1.97 and was installed from the Arch repositories with pacman.

Quoted 7 lines

Quoted 5 lines
The size of the file being specified for "lun" storage is 512MB. I have specified 128MB RAM in the below run with the size of file for IO performance as 256MB. Without this bonnie++ was giving me an error around the "Writing intelligently" point. I assume this has to do with the file size bonnie++ uses for testing.
What's your backfile for mass storage, mmc?

The backfile for this mass storage is a "file.img" in the /home/root directory created with the dd command as below
dd if=/dev/zero of=file.img bs=1024 count=0 seek=$[1024*512]

The script I use for activating mass storage is as below:

/bin/mount none /mnt -t configfs
/bin/mkdir /mnt/usb_gadget/g1
cd /mnt/usb_gadget/g1
/bin/mkdir configs/c.1
/bin/mkdir functions/mass_storage.0
echo /home/root/file.img > functions/mass_storage.0/lun.0/file /bin/mkdir strings/0x409
/bin/mkdir configs/c.1/strings/0x409
echo 0x006a > idProduct
echo 0x15a2 > idVendor
echo Freescale123 > strings/0x409/serialnumber
echo Freescale > strings/0x409/manufacturer
echo "Mass Storage Gadget" > strings/0x409/product echo "Conf 1" > configs/c.1/strings/0x409/configuration echo 200 > configs/c.1/MaxPower
ln -s functions/mass_storage.0 configs/c.1
echo ci_hdrc.0 > UDC

Quoted 1 line

You mean without this errata, running bonnie++ will meet error?

No, I didn't mean that. I meant if I do something like

sudo /usr/bin/bonnie++ -m Vybrid -r 256 -d /var/run/media/sanchayan/Vybrid/ -x 5 -u root -n 0

then this gives me an error. Not aware of the bonnie++ internals, but, I guess this would be because bonnie++ uses some higher multiple of the block size to write and the storage is much lesser than that. The exact output for this case is as below

[sanchayan@Sanchayan-Arch ~]$ sudo /usr/bin/bonnie++ -m Vybrid -r 256 -d /var/run/media/sanchayan/Vybrid/ -x 5 -u root -n 0
[sudo] password for sanchayan:
Using uid:0, gid:0. format_version,bonnie_version,name,concurrency,seed,file_size,io_chunk_size,putc,putc_cpu,put_block,put_block_cpu,rewrite,rewrite_cpu,getc,getc_cpu,get_block,get_block_cpu,seeks,seeks_cpu,num_files,max_size,min_size,num_dirs,file_chunk_size,seq_create,seq_create_cpu,seq_stat,seq_stat_cpu,seq_del,seq_del_cpu,ran_create,ran_create_cpu,ran_stat,ran_stat_cpu,ran_del,ran_del_cpu,putc_latency,put_block_latency,rewrite_latency,getc_latency,get_block_latency,seeks_latency,seq_create_latency,seq_stat_latency,seq_del_latency,ran_create_latency,ran_stat_latency,ran_del_latency Writing a byte at a time...done
Writing intelligently...Can't write block.: No such file or directory
Can't write block 61508.

Quoted 52 lines

Quoted 52 lines
[sanchayan@Sanchayan-Arch ~]$ sudo /usr/bin/bonnie++ -m Vybrid -r 128 -d /var/run/media/sanchayan/Vybrid/ -x 5 -u root -n 0 -s 256 Using uid:0, gid:0. format_version,bonnie_version,name,concurrency,seed,file_size,io_chunk_si ze,putc,putc_cpu,put_block,put_block_cpu,rewrite,rewrite_cpu,getc,getc_cp u,get_block,get_block_cpu,seeks,seeks_cpu,num_files,max_size,min_size,nu m_dirs,file_chunk_size,seq_create,seq_create_cpu,seq_stat,seq_stat_cpu,se q_del,seq_del_cpu,ran_create,ran_create_cpu,ran_stat,ran_stat_cpu,ran_del ,ran_del_cpu,putc_latency,put_block_latency,rewrite_latency,getc_latency,ge t_block_latency,seeks_latency,seq_create_latency,seq_stat_latency,seq_del_ latency,ran_create_latency,ran_stat_latency,ran_del_latency Writing a byte at a time...done
Writing intelligently...done
Rewriting...done
Reading a byte at a time...done
Reading intelligently...done
start 'em...done...done...done...done...done... 1.97,1.97,Vybrid,1,1419409300,256M,,659,87,8341,1,9401,0,4222,98,+++++,+++, 3539,19,,,,,,,,,,,,,,,,,,23042us,66us,59us,4482us,79us,475us,,,,,, Writing a byte at a time...done
Writing intelligently...done
Rewriting...done
Reading a byte at a time...done
Reading intelligently...done
start 'em...done...done...done...done...done... 1.97,1.97,Vybrid,1,1419409300,256M,,661,90,7689,1,9071,0,4011,99,+++++,+++, 3426,20,,,,,,,,,,,,,,,,,,15406us,64us,62us,4667us,23us,10030us,,,,,, Writing a byte at a time...done
Writing intelligently...done
Rewriting...done
Reading a byte at a time...done
Reading intelligently...done
start 'em...done...done...done...done...done... 1.97,1.97,Vybrid,1,1419409300,256M,,673,89,8117,1,9451,0,3879,98,+++++,+++, 3355,22,,,,,,,,,,,,,,,,,,14210us,45us,69us,5069us,21us,10052us,,,,,, Writing a byte at a time...done
Writing intelligently...done
Rewriting...done
Reading a byte at a time...done
Reading intelligently...done
start 'em...done...done...done...done...done... 1.97,1.97,Vybrid,1,1419409300,256M,,668,89,7801,1,9343,0,4099,98,+++++,+++, 3336,16,,,,,,,,,,,,,,,,,,17019us,44us,75us,4920us,20us,10234us,,,,,, Writing a byte at a time...done
Writing intelligently...done
Rewriting...done
Reading a byte at a time...done
Reading intelligently...done
start 'em...done...done...done...done...done... 1.97,1.97,Vybrid,1,1419409300,256M,,676,89,7953,1,9494,0,3878,98,+++++,+++, 3396,22,,,,,,,,,,,,,,,,,,14080us,56us,42us,5177us,23us,12224us,,,,,,

Quoted 5 lines

Quoted 1 line
Let me know if this is OK.
Thanks for making and testing the patch, it is really a good job. I will queue this patch in my chipidea tree.

Thanks :). Happy to help.

-Sanchayan.

Quoted 2 lines

Peter

Stefan Agner

Tested-by

2014-12-24 15:42 UTC #19 lore raw

In reply to Peter Chen

Quoted 11 lines · Peter Chen

Quoted 9 lines · Sanchayan Maity
The first two patches add identification register API's. These can be used to get controller's revision.
The third patch implements an errata for revision 2.40a. Not sure which other SOCs implement this version of the Chipidea core but this fixes the usb client issue observed on Vybrids. The patch was tested on a Toradex Colibri VF61 module with the 3.18 kernel. iperf tests ran for three hours plus, with these patches applied have found the USB client connection to be now reliable.

Quoted 3 lines

Would you help do a overnight test? It is passed, I will queue them, thanks.

Hi Peter,

Also did a short test on 3.19-rc1 here:

# iperf -s
------------------------------------------------------------
Server listening on TCP port 5001
TCP window size: 85.3 KByte (default)
------------------------------------------------------------
[  4] local 192.168.1.10 port 5001 connected with 192.168.1.1 port 53241
[ ID] Interval       Transfer     Bandwidth
[  4]  0.0-10.0 sec   152 MBytes   127 Mbits/sec
[  5] local 192.168.1.10 port 5001 connected with 192.168.1.1 port 53243
[  8] local 192.168.1.10 port 5001 connected with 192.168.1.1 port 53247
[  7] local 192.168.1.10 port 5001 connected with 192.168.1.1 port 53246
[  6] local 192.168.1.10 port 5001 connected with 192.168.1.1 port 53245
[  4] local 192.168.1.10 port 5001 connected with 192.168.1.1 port 53244
[  6]  0.0-10.1 sec  30.9 MBytes  25.7 Mbits/sec
[  5]  0.0-10.1 sec  30.9 MBytes  25.7 Mbits/sec
[  7]  0.0-10.1 sec  30.9 MBytes  25.7 Mbits/sec
[  8]  0.0-10.1 sec  30.9 MBytes  25.6 Mbits/sec
[  4]  0.0-10.1 sec  30.9 MBytes  25.7 Mbits/sec
[SUM]  0.0-10.1 sec   154 MBytes   128 Mbits/sec

Tried also UDP mode and scp copies as well as adding some kernel level latencies by pluging and unpluging a USB mass storage device. Looks good to me as well:

Tested-by Stefan Agner <[email protected]>

Btw, this was and solves now the error I was mentioning in my patchset enabling Vybrid support:

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/274342.html

@Sanchayan/Peter:
Minor nits as reply to the actual patch emails.

--
Stefan

Quoted 34 lines

Peter
Quoted 33 lines
This patchset is based off on Shawn Guo's for-next branch
https://git.kernel.org/cgit/linux/kernel/git/shawnguo/linux.git/tree/?h=for-next
The credit for the patches and fix goes to Matthieu Castet and Peter Chen. First two patches are by Peter Chen and the third patch which fixed the bug we observed was reported by Matthieu Castet.
The discussion of the problem and the relevant testing details can be found at this link: http://www.spinics.net/lists/linux-usb/msg118544.html
The first version of this patchset originally send by Peter Chen can be found at this link: http://www.spinics.net/lists/linux-usb/msg118753.html
Comments for review are welcome :).
Note: I am going on a vacation so will not be able to reply or do any further tests till Monday. Will attend and take care of any comments/requests for further changes/testing from Tuesday. Apologize for the delay in advance.
Sanchayan Maity (3):
usb: chipidea: Add identification registers access APIs
usb: chipidea: Add chipidea revision information
usb: chipidea: Add errata for revision 2.40a
 drivers/usb/chipidea/bits.h |   10 ++++++++
 drivers/usb/chipidea/ci.h   |   53 +++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/chipidea/core.c |   23 +++++++++++++++++--
 drivers/usb/chipidea/udc.c  |   20 ++++++++++++++++
 4 files changed, 104 insertions(+), 2 deletions(-)
--
1.7.9.5

Stefan Agner

2014-12-24 16:00 UTC #2 lore raw

Re: [PATCH 1/3] usb: chipidea: Add identification registers access APIs

In reply to Sanchayan Maity

Quoted 4 lines · Sanchayan Maity

Using hw_write_id_reg and hw_read_id_reg to write and read identification registers contents. This can be used to get controller information, change some system configurations and so on.

Checkpatch is complaining about DOS line endings and some trailing whitespace. This applies to all three patches.

Quoted 27 lines

Signed-off-by Sanchayan Maity <[email protected]>

---
 drivers/usb/chipidea/ci.h |   53 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

drivers/usb/chipidea/ci.h +9 -0

index ea40626..94db636 100644
@@ -29,6 +29,15 @@
 /******************************************************************************
  * REGISTERS
  *****************************************************************************/
+/* Identification Registers */
+#define ID_ID				0x0
+#define ID_HWGENERAL			0x4
+#define ID_HWHOST			0x8
+#define ID_HWDEVICE			0xc
+#define ID_HWTXBUF			0x10
+#define ID_HWRXBUF			0x14
+#define ID_SBUSCFG			0x90
+
 /* register indices */
 enum ci_hw_regs {
 	CAP_CAPLENGTH,

drivers/usb/chipidea/ci.h enum ci_role { · line 109

Quoted 4 lines

drivers/usb/chipidea/ci.h +1 -0

@@ -97,6 +106,18 @@ enum ci_role {
 	CI_ROLE_END,
 };
 
+enum CI_REVISION {

Usually the enum names are small caps, only the labels are capitalized. I would suggest use ci_revision here (similar to the enum above).

Quoted 52 lines

drivers/usb/chipidea/ci.h +31 -0

+	CI_REVISION_1X = 10,	/* Revision 1.x */
+	CI_REVISION_20 = 20, /* Revision 2.0 */
+	CI_REVISION_21, /* Revision 2.1 */
+	CI_REVISION_22, /* Revision 2.2 */
+	CI_REVISION_23, /* Revision 2.3 */
+	CI_REVISION_24, /* Revision 2.4 */
+	CI_REVISION_25, /* Revision 2.5 */
+	CI_REVISION_25_PLUS, /* Revision above than 2.5 */
+	CI_REVISION_UNKNOWN = 99, /* Unknown Revision */
+};
+
 /**
  * struct ci_role_driver - host/gadget role driver
  * @start: start this role

@@ -168,6 +189,7 @@ struct hw_bank {
  * @b_sess_valid_event: indicates there is a vbus event, and handled
  * at ci_otg_work
  * @imx28_write_fix: Freescale imx28 needs swp instruction for writing
+ * @rev: The revision number for controller
  */
 struct ci_hdrc {
 	struct device			*dev;

@@ -207,6 +229,7 @@ struct ci_hdrc {
 	bool				id_event;
 	bool				b_sess_valid_event;
 	bool				imx28_write_fix;
+	enum CI_REVISION		rev;
 };
 
 static inline struct ci_role_driver *ci_role(struct ci_hdrc *ci)

@@ -244,6 +267,36 @@ static inline void ci_role_stop(struct ci_hdrc *ci)
 }
 
 /**
+ * hw_read_id_reg: reads from a identification register
+ * @ci: the controller
+ * @offset: offset from the beginning of identification registers region
+ * @mask: bitfield mask
+ *
+ * This function returns register contents
+ */
+static inline u32 hw_read_id_reg(struct ci_hdrc *ci, u32 offset, u32 mask)
+{
+	return ioread32(ci->hw_bank.abs + offset) & mask;
+}
+
+/**
+ * hw_write_id_reg: writes to a identification register
+ * @ci: the controller
+ * @offset: offset from the beginning of identification registers region
+ * @mask: bitfield mask
+ * @data: new value

drivers/usb/chipidea/ci.h static inline void ci_role_stop(struct ci_hdrc *ci) · line 297

Quoted 10 lines

drivers/usb/chipidea/ci.h +10 -0

+ */
+static inline void hw_write_id_reg(struct ci_hdrc *ci, u32 offset,
+			    u32 mask, u32 data)
+{
+	if (~mask)
+		data = (ioread32(ci->hw_bank.abs + offset) & ~mask)
+			| (data & mask);
+
+	iowrite32(data, ci->hw_bank.abs + offset);
+}

This function isn't used anywhere, does it make sense to write an ID register? The ones I see in Vybrid's RM are read-only anyway..

--
Stefan

Quoted 5 lines

drivers/usb/chipidea/ci.h +2 -0

+
+/**
  * hw_read: reads from a hw register
  * @ci: the controller
  * @reg:  register index

Sanchayan Maity

author

2014-12-24 16:16 UTC #3 lore raw

Re: [PATCH 1/3] usb: chipidea: Add identification registers access APIs

Quoted 8 lines · Stefan Agner

Quoted 4 lines · Sanchayan Maity
Using hw_write_id_reg and hw_read_id_reg to write and read identification registers contents. This can be used to get controller information, change some system configurations and so on.
Checkpatch is complaining about DOS line endings and some trailing whitespace. This applies to all three patches.

Hmm... that's surprising. I did run checkpatch at my end before sending. I do not have the original patchset on my laptop which I have right now but downloading diffs back from lkml and running checkpatch on them back, only gives me signed off errors as it should.

I will resend if somehow errors crept in with my earlier patches.

-Sanchayan

Quoted 111 lines

Quoted 28 lines

Signed-off-by Sanchayan Maity <[email protected]>

---
 drivers/usb/chipidea/ci.h |   53 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

drivers/usb/chipidea/ci.h +9 -0

index ea40626..94db636 100644
@@ -29,6 +29,15 @@
 /******************************************************************************
  * REGISTERS
  *****************************************************************************/
+/* Identification Registers */
+#define ID_ID				0x0
+#define ID_HWGENERAL			0x4
+#define ID_HWHOST			0x8
+#define ID_HWDEVICE			0xc
+#define ID_HWTXBUF			0x10
+#define ID_HWRXBUF			0x14
+#define ID_SBUSCFG			0x90
+
 /* register indices */
 enum ci_hw_regs {
 	CAP_CAPLENGTH,

drivers/usb/chipidea/ci.h enum ci_role { · line 109

Quoted 4 lines

drivers/usb/chipidea/ci.h +1 -0

@@ -97,6 +106,18 @@ enum ci_role {
 	CI_ROLE_END,
 };
 
+enum CI_REVISION {

Usually the enum names are small caps, only the labels are capitalized. I would suggest use ci_revision here (similar to the enum above).

Quoted 52 lines

drivers/usb/chipidea/ci.h +31 -0

+	CI_REVISION_1X = 10,	/* Revision 1.x */
+	CI_REVISION_20 = 20, /* Revision 2.0 */
+	CI_REVISION_21, /* Revision 2.1 */
+	CI_REVISION_22, /* Revision 2.2 */
+	CI_REVISION_23, /* Revision 2.3 */
+	CI_REVISION_24, /* Revision 2.4 */
+	CI_REVISION_25, /* Revision 2.5 */
+	CI_REVISION_25_PLUS, /* Revision above than 2.5 */
+	CI_REVISION_UNKNOWN = 99, /* Unknown Revision */
+};
+
 /**
  * struct ci_role_driver - host/gadget role driver
  * @start: start this role

@@ -168,6 +189,7 @@ struct hw_bank {
  * @b_sess_valid_event: indicates there is a vbus event, and handled
  * at ci_otg_work
  * @imx28_write_fix: Freescale imx28 needs swp instruction for writing
+ * @rev: The revision number for controller
  */
 struct ci_hdrc {
 	struct device			*dev;

@@ -207,6 +229,7 @@ struct ci_hdrc {
 	bool				id_event;
 	bool				b_sess_valid_event;
 	bool				imx28_write_fix;
+	enum CI_REVISION		rev;
 };
 
 static inline struct ci_role_driver *ci_role(struct ci_hdrc *ci)

@@ -244,6 +267,36 @@ static inline void ci_role_stop(struct ci_hdrc *ci)
 }
 
 /**
+ * hw_read_id_reg: reads from a identification register
+ * @ci: the controller
+ * @offset: offset from the beginning of identification registers region
+ * @mask: bitfield mask
+ *
+ * This function returns register contents
+ */
+static inline u32 hw_read_id_reg(struct ci_hdrc *ci, u32 offset, u32 mask)
+{
+	return ioread32(ci->hw_bank.abs + offset) & mask;
+}
+
+/**
+ * hw_write_id_reg: writes to a identification register
+ * @ci: the controller
+ * @offset: offset from the beginning of identification registers region
+ * @mask: bitfield mask
+ * @data: new value

drivers/usb/chipidea/ci.h static inline void ci_role_stop(struct ci_hdrc *ci) · line 297

Quoted 10 lines

drivers/usb/chipidea/ci.h +10 -0

+ */
+static inline void hw_write_id_reg(struct ci_hdrc *ci, u32 offset,
+			    u32 mask, u32 data)
+{
+	if (~mask)
+		data = (ioread32(ci->hw_bank.abs + offset) & ~mask)
+			| (data & mask);
+
+	iowrite32(data, ci->hw_bank.abs + offset);
+}

This function isn't used anywhere, does it make sense to write an ID register? The ones I see in Vybrid's RM are read-only anyway..

--
Stefan

Quoted 5 lines

drivers/usb/chipidea/ci.h +2 -0

+
+/**
  * hw_read: reads from a hw register
  * @ci: the controller
  * @reg:  register index

Stefan Agner

2014-12-24 16:23 UTC #9 lore raw

Re: [PATCH 2/3] usb: chipidea: Add chipidea revision information

In reply to Sanchayan Maity

Quoted 20 lines · Sanchayan Maity

Define ci_get_revision API to know the controller revision information according to chipidea 1.1a, 2.0a, 2.4 and 2.5a spec. Besides, add one entry in struct ci_hdrc to indicate revision information. This can be used for adding different code for revisions, implementing erratas.
Signed-off-by Sanchayan Maity <[email protected]>
---
 drivers/usb/chipidea/bits.h |   10 ++++++++++
 drivers/usb/chipidea/core.c |   23 +++++++++++++++++++++--
 2 files changed, 31 insertions(+), 2 deletions(-)
drivers/usb/chipidea/bits.h +0 -0
index ca57e3d..e935ccc 100644
@@ -15,6 +15,16 @@
 
 #include <linux/usb/ehci_def.h>
 

drivers/usb/chipidea/bits.h line 27

Quoted 10 lines

drivers/usb/chipidea/bits.h +10 -0

+/*
+ * ID
+ * For 1.x revision, bit24 - bit31 are reserved
+ * For 2.x revision, bit25 - bit28 are 0x2
+ */
+#define TAG			(0x1F << 16)
+#define REVISION		(0xF << 21)
+#define VERSION			(0xF << 25)
+#define CIVERSION		(0x7 << 29)
+

Hm, the other defines use spaces in this file, I like tabs more too, but I guess uniformity wins here.. Peter?

Quoted 9 lines

drivers/usb/chipidea/bits.h +0 -0

 /* HCCPARAMS */
 #define HCCPARAMS_LEN         BIT(17)

drivers/usb/chipidea/core.c +0 -0

index 9bdc6bd..33a8c4a 100644
@@ -136,6 +136,22 @@ static int hw_alloc_regmap(struct ci_hdrc *ci, bool is_lpm)
 	return 0;

drivers/usb/chipidea/core.c static int hw_alloc_regmap(struct ci_hdrc *ci, bool is_lpm) · line 146

Quoted 10 lines

drivers/usb/chipidea/core.c +8 -0

 }
 
+static enum CI_REVISION ci_get_revision(struct ci_hdrc *ci)
+{
+	int ver = hw_read_id_reg(ci, ID_ID, VERSION) >> __ffs(VERSION);
+	enum CI_REVISION rev = CI_REVISION_UNKNOWN;
+
+	if (ver == 0x2) {
+		int rev_reg = hw_read_id_reg
+			(ci, ID_ID, REVISION) >> __ffs(REVISION);

This line break is somewhat awkward, doesn't help for good readability.

Is the local integer variable necessary at all? Enums are arithmetic types, so afaik this should be fine too:

		rev = CI_REVISION_20;
		rev += hw_read_id_reg(ci, ID_ID, REVISION) >> __ffs(REVISION);

Quoted 25 lines

drivers/usb/chipidea/core.c +13 -2

+		rev = rev_reg + CI_REVISION_20;
+	} else if (ver == 0x0) {
+		rev = CI_REVISION_1X;
+	}
+
+	return rev;
+}
+
 /**
  * hw_read_intr_enable: returns interrupt enable register
  *

@@ -245,8 +261,11 @@ static int hw_device_init(struct ci_hdrc *ci,
 void __iomem *base)
 	/* Clear all interrupts status bits*/
 	hw_write(ci, OP_USBSTS, 0xffffffff, 0xffffffff);
 
-	dev_dbg(ci->dev, "ChipIdea HDRC found, lpm: %d; cap: %p op: %p\n",
-		ci->hw_bank.lpm, ci->hw_bank.cap, ci->hw_bank.op);
+	ci->rev = ci_get_revision(ci);
+
+	dev_dbg(ci->dev,
+		"ChipIdea HDRC found, revision: %d, lpm: %d; cap: %p op: %p\n",
+		ci->rev, ci->hw_bank.lpm, ci->hw_bank.cap, ci->hw_bank.op);
 
 	/* setup lock mode ? */

Stefan Agner

2014-12-24 16:41 UTC #4 lore raw

Re: [PATCH 1/3] usb: chipidea: Add identification registers access APIs

In reply to Sanchayan Maity

Quoted 7 lines · Sanchayan Maity

Quoted 6 lines · Stefan Agner
Quoted 4 lines · Sanchayan Maity
Using hw_write_id_reg and hw_read_id_reg to write and read identification registers contents. This can be used to get controller information, change some system configurations and so on.

Quoted 8 lines

Quoted 2 lines
Checkpatch is complaining about DOS line endings and some trailing whitespace. This applies to all three patches.
Hmm... that's surprising. I did run checkpatch at my end before sending. I do not have the original patchset on my laptop which I have right now but downloading diffs back from lkml and running checkpatch on them back, only gives me signed off errors as it should.

You are right, it's on my side. My webmailer (Roundcube) seem to introduce those line endings when using "Download (.eml)". Using "Show Source", and then save the e-mail makes checkpatch happy... Sorry about that.

--
Stefan

Quoted 115 lines

I will resend if somehow errors crept in with my earlier patches.

-Sanchayan

Quoted 111 lines

Quoted 28 lines

Signed-off-by Sanchayan Maity <[email protected]>

---
 drivers/usb/chipidea/ci.h |   53 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

drivers/usb/chipidea/ci.h +9 -0

index ea40626..94db636 100644
@@ -29,6 +29,15 @@
 /******************************************************************************
  * REGISTERS
  *****************************************************************************/
+/* Identification Registers */
+#define ID_ID				0x0
+#define ID_HWGENERAL			0x4
+#define ID_HWHOST			0x8
+#define ID_HWDEVICE			0xc
+#define ID_HWTXBUF			0x10
+#define ID_HWRXBUF			0x14
+#define ID_SBUSCFG			0x90
+
 /* register indices */
 enum ci_hw_regs {
 	CAP_CAPLENGTH,

drivers/usb/chipidea/ci.h enum ci_role { · line 109

Quoted 4 lines

drivers/usb/chipidea/ci.h +1 -0

@@ -97,6 +106,18 @@ enum ci_role {
 	CI_ROLE_END,
 };
 
+enum CI_REVISION {

Usually the enum names are small caps, only the labels are capitalized. I would suggest use ci_revision here (similar to the enum above).

Quoted 52 lines

drivers/usb/chipidea/ci.h +31 -0

+	CI_REVISION_1X = 10,	/* Revision 1.x */
+	CI_REVISION_20 = 20, /* Revision 2.0 */
+	CI_REVISION_21, /* Revision 2.1 */
+	CI_REVISION_22, /* Revision 2.2 */
+	CI_REVISION_23, /* Revision 2.3 */
+	CI_REVISION_24, /* Revision 2.4 */
+	CI_REVISION_25, /* Revision 2.5 */
+	CI_REVISION_25_PLUS, /* Revision above than 2.5 */
+	CI_REVISION_UNKNOWN = 99, /* Unknown Revision */
+};
+
 /**
  * struct ci_role_driver - host/gadget role driver
  * @start: start this role

@@ -168,6 +189,7 @@ struct hw_bank {
  * @b_sess_valid_event: indicates there is a vbus event, and handled
  * at ci_otg_work
  * @imx28_write_fix: Freescale imx28 needs swp instruction for writing
+ * @rev: The revision number for controller
  */
 struct ci_hdrc {
 	struct device			*dev;

@@ -207,6 +229,7 @@ struct ci_hdrc {
 	bool				id_event;
 	bool				b_sess_valid_event;
 	bool				imx28_write_fix;
+	enum CI_REVISION		rev;
 };
 
 static inline struct ci_role_driver *ci_role(struct ci_hdrc *ci)

@@ -244,6 +267,36 @@ static inline void ci_role_stop(struct ci_hdrc *ci)
 }
 
 /**
+ * hw_read_id_reg: reads from a identification register
+ * @ci: the controller
+ * @offset: offset from the beginning of identification registers region
+ * @mask: bitfield mask
+ *
+ * This function returns register contents
+ */
+static inline u32 hw_read_id_reg(struct ci_hdrc *ci, u32 offset, u32 mask)
+{
+	return ioread32(ci->hw_bank.abs + offset) & mask;
+}
+
+/**
+ * hw_write_id_reg: writes to a identification register
+ * @ci: the controller
+ * @offset: offset from the beginning of identification registers region
+ * @mask: bitfield mask
+ * @data: new value

drivers/usb/chipidea/ci.h static inline void ci_role_stop(struct ci_hdrc *ci) · line 297

Quoted 10 lines

drivers/usb/chipidea/ci.h +10 -0

+ */
+static inline void hw_write_id_reg(struct ci_hdrc *ci, u32 offset,
+			    u32 mask, u32 data)
+{
+	if (~mask)
+		data = (ioread32(ci->hw_bank.abs + offset) & ~mask)
+			| (data & mask);
+
+	iowrite32(data, ci->hw_bank.abs + offset);
+}

This function isn't used anywhere, does it make sense to write an ID register? The ones I see in Vybrid's RM are read-only anyway..

--
Stefan

Quoted 5 lines

drivers/usb/chipidea/ci.h +2 -0

+
+/**
  * hw_read: reads from a hw register
  * @ci: the controller
  * @reg:  register index

Peter Chen

2014-12-25 03:12 UTC #7 lore raw

Re: [PATCH 1/3] usb: chipidea: Add identification registers access APIs

In reply to Stefan Agner

Quoted 39 lines · Stefan Agner

Quoted 4 lines · Sanchayan Maity
Using hw_write_id_reg and hw_read_id_reg to write and read identification registers contents. This can be used to get controller information, change some system configurations and so on.
Checkpatch is complaining about DOS line endings and some trailing whitespace. This applies to all three patches.
Quoted 30 lines
Signed-off-by Sanchayan Maity <[email protected]>
---
 drivers/usb/chipidea/ci.h |   53 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)
drivers/usb/chipidea/ci.h +9 -0
index ea40626..94db636 100644
@@ -29,6 +29,15 @@
 /******************************************************************************
  * REGISTERS
  *****************************************************************************/
+/* Identification Registers */
+#define ID_ID				0x0
+#define ID_HWGENERAL			0x4
+#define ID_HWHOST			0x8
+#define ID_HWDEVICE			0xc
+#define ID_HWTXBUF			0x10
+#define ID_HWRXBUF			0x14
+#define ID_SBUSCFG			0x90
+
 /* register indices */
 enum ci_hw_regs {
 	CAP_CAPLENGTH,
@@ -97,6 +106,18 @@ enum ci_role {
 	CI_ROLE_END,
 };
 

Quoted 4 lines

drivers/usb/chipidea/ci.h enum ci_role { · line 109
Quoted 1 line
drivers/usb/chipidea/ci.h +1 -0
+enum CI_REVISION {
Usually the enum names are small caps, only the labels are capitalized. I would suggest use ci_revision here (similar to the enum above).

I will change it.

Quoted 58 lines

drivers/usb/chipidea/ci.h +36 -0

+	CI_REVISION_1X = 10,	/* Revision 1.x */
+	CI_REVISION_20 = 20, /* Revision 2.0 */
+	CI_REVISION_21, /* Revision 2.1 */
+	CI_REVISION_22, /* Revision 2.2 */
+	CI_REVISION_23, /* Revision 2.3 */
+	CI_REVISION_24, /* Revision 2.4 */
+	CI_REVISION_25, /* Revision 2.5 */
+	CI_REVISION_25_PLUS, /* Revision above than 2.5 */
+	CI_REVISION_UNKNOWN = 99, /* Unknown Revision */
+};
+
 /**
  * struct ci_role_driver - host/gadget role driver
  * @start: start this role

@@ -168,6 +189,7 @@ struct hw_bank {
  * @b_sess_valid_event: indicates there is a vbus event, and handled
  * at ci_otg_work
  * @imx28_write_fix: Freescale imx28 needs swp instruction for writing
+ * @rev: The revision number for controller
  */
 struct ci_hdrc {
 	struct device			*dev;

@@ -207,6 +229,7 @@ struct ci_hdrc {
 	bool				id_event;
 	bool				b_sess_valid_event;
 	bool				imx28_write_fix;
+	enum CI_REVISION		rev;
 };
 
 static inline struct ci_role_driver *ci_role(struct ci_hdrc *ci)

@@ -244,6 +267,36 @@ static inline void ci_role_stop(struct ci_hdrc *ci)
 }
 
 /**
+ * hw_read_id_reg: reads from a identification register
+ * @ci: the controller
+ * @offset: offset from the beginning of identification registers region
+ * @mask: bitfield mask
+ *
+ * This function returns register contents
+ */
+static inline u32 hw_read_id_reg(struct ci_hdrc *ci, u32 offset, u32 mask)
+{
+	return ioread32(ci->hw_bank.abs + offset) & mask;
+}
+
+/**
+ * hw_write_id_reg: writes to a identification register
+ * @ci: the controller
+ * @offset: offset from the beginning of identification registers region
+ * @mask: bitfield mask
+ * @data: new value
+ */
+static inline void hw_write_id_reg(struct ci_hdrc *ci, u32 offset,
+			    u32 mask, u32 data)
+{
+	if (~mask)

Quoted 9 lines

drivers/usb/chipidea/ci.h static inline void ci_role_stop(struct ci_hdrc *ci) · line 297
Quoted 5 lines
drivers/usb/chipidea/ci.h +5 -0
+		data = (ioread32(ci->hw_bank.abs + offset) & ~mask)
+			| (data & mask);
+
+	iowrite32(data, ci->hw_bank.abs + offset);
+}
This function isn't used anywhere, does it make sense to write an ID register? The ones I see in Vybrid's RM are read-only anyway..

This API is used to write identification registers (offset, 0x00 - 0x90), not only the ID register, the SBUSCFG (offset 0x90) attribute is read/write.

Signature · 2 lines

Best Regards,
Peter Chen

Peter Chen

2014-12-25 03:22 UTC #5 lore raw

Re: [PATCH 1/3] usb: chipidea: Add identification registers access APIs

In reply to Sanchayan Maity

Quoted 10 lines · Sanchayan Maity

Quoted 8 lines · Stefan Agner
Quoted 4 lines · Sanchayan Maity
Using hw_write_id_reg and hw_read_id_reg to write and read identification registers contents. This can be used to get controller information, change some system configurations and so on.
Checkpatch is complaining about DOS line endings and some trailing whitespace. This applies to all three patches.

Quoted 9 lines

Hmm... that's surprising. I did run checkpatch at my end before sending. I do not have the original patchset on my laptop which I have right now but downloading diffs back from lkml and running checkpatch on them back, only gives me signed off errors as it should.
I will resend if somehow errors crept in with my earlier patches.
-Sanchayan

Hi Sanchayan,

I will take care the first two, you are the third if there is any comments, thanks.

Signature · 2 lines

Best Regards,
Peter Chen

Sanchayan Maity

author

2014-12-25 03:34 UTC #6 lore raw

Re: [PATCH 1/3] usb: chipidea: Add identification registers access APIs

Quoted 18 lines · Peter Chen

Quoted 17 lines · Sanchayan Maity
Quoted 8 lines · Stefan Agner
Quoted 4 lines · Sanchayan Maity
Using hw_write_id_reg and hw_read_id_reg to write and read identification registers contents. This can be used to get controller information, change some system configurations and so on.
Checkpatch is complaining about DOS line endings and some trailing whitespace. This applies to all three patches.
Hmm... that's surprising. I did run checkpatch at my end before sending. I do not have the original patchset on my laptop which I have right now but downloading diffs back from lkml and running checkpatch on them back, only gives me signed off errors as it should.
I will resend if somehow errors crept in with my earlier patches.

Quoted 8 lines

Quoted 2 lines
-Sanchayan
Hi Sanchayan,
I will take care the first two, you are the third if there is any comments, thanks.

Hi Peter,

OK with me. I can resend all the three patches with Stefan's recommended changes if you did like.

-Regards,
Sanchayan.

Peter Chen

2014-12-25 03:39 UTC #10 lore raw

Re: [PATCH 2/3] usb: chipidea: Add chipidea revision information

In reply to Stefan Agner

Quoted 21 lines · Stefan Agner

Quoted 20 lines · Sanchayan Maity
Define ci_get_revision API to know the controller revision information according to chipidea 1.1a, 2.0a, 2.4 and 2.5a spec. Besides, add one entry in struct ci_hdrc to indicate revision information. This can be used for adding different code for revisions, implementing erratas.
Signed-off-by Sanchayan Maity <[email protected]>
---
 drivers/usb/chipidea/bits.h |   10 ++++++++++
 drivers/usb/chipidea/core.c |   23 +++++++++++++++++++++--
 2 files changed, 31 insertions(+), 2 deletions(-)
drivers/usb/chipidea/bits.h +0 -0
index ca57e3d..e935ccc 100644
@@ -15,6 +15,16 @@
 
 #include <linux/usb/ehci_def.h>
 

Quoted 13 lines

drivers/usb/chipidea/bits.h line 27
Quoted 10 lines
drivers/usb/chipidea/bits.h +10 -0
+/*
+ * ID
+ * For 1.x revision, bit24 - bit31 are reserved
+ * For 2.x revision, bit25 - bit28 are 0x2
+ */
+#define TAG			(0x1F << 16)
+#define REVISION		(0xF << 21)
+#define VERSION			(0xF << 25)
+#define CIVERSION		(0x7 << 29)
+
Hm, the other defines use spaces in this file, I like tabs more too, but I guess uniformity wins here.. Peter?

I will change to use spaces.

Quoted 12 lines

drivers/usb/chipidea/bits.h +0 -0

 /* HCCPARAMS */
 #define HCCPARAMS_LEN         BIT(17)

drivers/usb/chipidea/core.c +0 -0

index 9bdc6bd..33a8c4a 100644
@@ -136,6 +136,22 @@ static int hw_alloc_regmap(struct ci_hdrc *ci, bool is_lpm)
 	return 0;
 }

Quoted 11 lines

drivers/usb/chipidea/core.c static int hw_alloc_regmap(struct ci_hdrc *ci, bool is_lpm) · line 146
Quoted 8 lines
drivers/usb/chipidea/core.c +8 -0
+static enum CI_REVISION ci_get_revision(struct ci_hdrc *ci)
+{
+	int ver = hw_read_id_reg(ci, ID_ID, VERSION) >> __ffs(VERSION);
+	enum CI_REVISION rev = CI_REVISION_UNKNOWN;
+
+	if (ver == 0x2) {
+		int rev_reg = hw_read_id_reg
+			(ci, ID_ID, REVISION) >> __ffs(REVISION);
This line break is somewhat awkward, doesn't help for good readability.

But it exceeds 80 characters, maybe I change like below:

rev += hw_read_id_reg(ci, ID_ID, REVISION)

	       		>> __ffs(REVISION);

Quoted 4 lines

Is the local integer variable necessary at all? Enums are arithmetic types, so afaik this should be fine too:
		rev = CI_REVISION_20;
		rev += hw_read_id_reg(ci, ID_ID, REVISION) >> __ffs(REVISION);

Will change, thanks.

Signature · 2 lines

Best Regards,
Peter Chen