[PATCH 0/4] usb: atmel_usba_udc: Rework errata handling

[PATCH 0/4] usb: atmel_usba_udc: Rework errata handling

[Boris Brezillon]

Hello,

Here is a set of patches porting existing at91sam9rl erratum handling to
DT and adding new code to handle at91sam9g45/9x5 erratum.
It also adds several compatible strings to differentiate those errata.

These patches should be backported to 3.17 and 3.18 stable releases but
they do not apply cleanly on those stable branches.
I'll send a backport of this series once it is merged in mainline.

Regards,

Boris


Boris Brezillon (4):
  usb: atmel_usba_udc: Rework at91sam9rl errata handling
  usb: atmel_usba_udc: Add at91sam9g45 and at91sam9x5 errata handling
  ARM: at91/dt: update udc compatible strings
  usb: atmel_usba_udc: mask status with enabled irqs

 .../devicetree/bindings/usb/atmel-usb.txt          |   5 +-
 arch/arm/boot/dts/at91sam9g45.dtsi                 |   2 +-
 arch/arm/boot/dts/at91sam9x5.dtsi                  |   2 +-
 arch/arm/boot/dts/sama5d3.dtsi                     |   2 +-
 arch/arm/boot/dts/sama5d4.dtsi                     |   2 +-
 drivers/usb/gadget/udc/atmel_usba_udc.c            | 104 ++++++++++++++-------
 drivers/usb/gadget/udc/atmel_usba_udc.h            |   7 ++
 7 files changed, 86 insertions(+), 38 deletions(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/4] usb: atmel_usba_udc: Rework at91sam9rl errata handling

[Boris Brezillon]

at91sam9rl SoC has an erratum forcing us to toggle the BIAS on USB
suspend/resume events.

This specific handling is only activated when CONFIG_ARCH_AT91SAM9RL is
set and this option is only set when building a non-DT kernel, which is
problematic since non-DT support for at91sam9rl SoC has been removed.

Rework the toggle_bias implementation to attach it to the "at91sam9rl-udc"
compatible string.

Add new compatible strings to avoid executing at91sam9rl erratum handling
on other SoCs.

Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 .../devicetree/bindings/usb/atmel-usb.txt          |  5 +-
 drivers/usb/gadget/udc/atmel_usba_udc.c            | 78 ++++++++++++----------
 drivers/usb/gadget/udc/atmel_usba_udc.h            |  5 ++
 3 files changed, 52 insertions(+), 36 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/atmel-usb.txt b/Documentation/devicetree/bindings/usb/atmel-usb.txt
index bc2222c..38fee0f 100644
--- a/Documentation/devicetree/bindings/usb/atmel-usb.txt
+++ b/Documentation/devicetree/bindings/usb/atmel-usb.txt
@@ -51,7 +51,10 @@ usb1: gadget@fffa4000 {
 Atmel High-Speed USB device controller
 
 Required properties:
- - compatible: Should be "atmel,at91sam9rl-udc"
+ - compatible: Should be one of the following
+	       "at91sam9rl-udc"
+	       "at91sam9g45-udc"
+	       "sama5d3-udc"
  - reg: Address and length of the register set for the device
  - interrupts: Should contain usba interrupt
  - ep childnode: To specify the number of endpoints and their properties.
diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
index ce88237..36fd34b 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.c
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
@@ -8,6 +8,7 @@
  * published by the Free Software Foundation.
  */
 #include <linux/clk.h>
+#include <linux/clk/at91_pmc.h>
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
@@ -324,28 +325,12 @@ static int vbus_is_present(struct usba_udc *udc)
 	return 1;
 }
 
-#if defined(CONFIG_ARCH_AT91SAM9RL)
-
-#include <linux/clk/at91_pmc.h>
-
-static void toggle_bias(int is_on)
-{
-	unsigned int uckr = at91_pmc_read(AT91_CKGR_UCKR);
-
-	if (is_on)
-		at91_pmc_write(AT91_CKGR_UCKR, uckr | AT91_PMC_BIASEN);
-	else
-		at91_pmc_write(AT91_CKGR_UCKR, uckr & ~(AT91_PMC_BIASEN));
-}
-
-#else
-
-static void toggle_bias(int is_on)
+static void toggle_bias(struct usba_udc *udc, int is_on)
 {
+	if (udc->errata && udc->errata->toggle_bias)
+		udc->errata->toggle_bias(udc, is_on);
 }
 
-#endif /* CONFIG_ARCH_AT91SAM9RL */
-
 static void next_fifo_transaction(struct usba_ep *ep, struct usba_request *req)
 {
 	unsigned int transaction_len;
@@ -1620,7 +1605,7 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
 	DBG(DBG_INT, "irq, status=%#08x\n", status);
 
 	if (status & USBA_DET_SUSPEND) {
-		toggle_bias(0);
+		toggle_bias(udc, 0);
 		usba_writel(udc, INT_CLR, USBA_DET_SUSPEND);
 		DBG(DBG_BUS, "Suspend detected\n");
 		if (udc->gadget.speed != USB_SPEED_UNKNOWN
@@ -1632,7 +1617,7 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
 	}
 
 	if (status & USBA_WAKE_UP) {
-		toggle_bias(1);
+		toggle_bias(udc, 1);
 		usba_writel(udc, INT_CLR, USBA_WAKE_UP);
 		DBG(DBG_BUS, "Wake Up CPU detected\n");
 	}
@@ -1736,13 +1721,13 @@ static irqreturn_t usba_vbus_irq(int irq, void *devid)
 	vbus = vbus_is_present(udc);
 	if (vbus != udc->vbus_prev) {
 		if (vbus) {
-			toggle_bias(1);
+			toggle_bias(udc, 1);
 			usba_writel(udc, CTRL, USBA_ENABLE_MASK);
 			usba_writel(udc, INT_ENB, USBA_END_OF_RESET);
 		} else {
 			udc->gadget.speed = USB_SPEED_UNKNOWN;
 			reset_all_endpoints(udc);
-			toggle_bias(0);
+			toggle_bias(udc, 0);
 			usba_writel(udc, CTRL, USBA_DISABLE_MASK);
 			if (udc->driver->disconnect) {
 				spin_unlock(&udc->lock);
@@ -1788,7 +1773,7 @@ static int atmel_usba_start(struct usb_gadget *gadget,
 	/* If Vbus is present, enable the controller and wait for reset */
 	spin_lock_irqsave(&udc->lock, flags);
 	if (vbus_is_present(udc) && udc->vbus_prev == 0) {
-		toggle_bias(1);
+		toggle_bias(udc, 1);
 		usba_writel(udc, CTRL, USBA_ENABLE_MASK);
 		usba_writel(udc, INT_ENB, USBA_END_OF_RESET);
 	}
@@ -1811,7 +1796,7 @@ static int atmel_usba_stop(struct usb_gadget *gadget)
 	spin_unlock_irqrestore(&udc->lock, flags);
 
 	/* This will also disable the DP pullup */
-	toggle_bias(0);
+	toggle_bias(udc, 0);
 	usba_writel(udc, CTRL, USBA_DISABLE_MASK);
 
 	clk_disable_unprepare(udc->hclk);
@@ -1823,6 +1808,29 @@ static int atmel_usba_stop(struct usb_gadget *gadget)
 }
 
 #ifdef CONFIG_OF
+static void at91sam9rl_toggle_bias(struct usba_udc *udc, int is_on)
+{
+	unsigned int uckr = at91_pmc_read(AT91_CKGR_UCKR);
+
+	if (is_on)
+		at91_pmc_write(AT91_CKGR_UCKR, uckr | AT91_PMC_BIASEN);
+	else
+		at91_pmc_write(AT91_CKGR_UCKR, uckr & ~(AT91_PMC_BIASEN));
+}
+
+static const struct usba_udc_errata at91sam9rl_errata = {
+	.toggle_bias = at91sam9rl_toggle_bias,
+};
+
+static const struct of_device_id atmel_udc_dt_ids[] = {
+	{ .compatible = "atmel,at91sam9rl-udc", .data = &at91sam9rl_errata },
+	{ .compatible = "atmel,at91sam9g45-udc" },
+	{ .compatible = "atmel,sama5d3-udc" },
+	{ /* sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, atmel_udc_dt_ids);
+
 static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
 						    struct usba_udc *udc)
 {
@@ -1830,10 +1838,17 @@ static struct usba_ep * atmel_udc_of_init(struct platform_device *pdev,
 	const char *name;
 	enum of_gpio_flags flags;
 	struct device_node *np = pdev->dev.of_node;
+	const struct of_device_id *match;
 	struct device_node *pp;
 	int i, ret;
 	struct usba_ep *eps, *ep;
 
+	match = of_match_node(atmel_udc_dt_ids, np);
+	if (!match)
+		return ERR_PTR(-EINVAL);
+
+	udc->errata = match->data;
+
 	udc->num_ep = 0;
 
 	udc->vbus_pin = of_get_named_gpio_flags(np, "atmel,vbus-gpio", 0,
@@ -2024,7 +2039,7 @@ static int usba_udc_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "Unable to enable pclk, aborting.\n");
 		return ret;
 	}
-	toggle_bias(0);
+
 	usba_writel(udc, CTRL, USBA_DISABLE_MASK);
 	clk_disable_unprepare(pclk);
 
@@ -2033,6 +2048,8 @@ static int usba_udc_probe(struct platform_device *pdev)
 	else
 		udc->usba_ep = usba_udc_pdata(pdev, udc);
 
+	toggle_bias(udc, 0);
+
 	if (IS_ERR(udc->usba_ep))
 		return PTR_ERR(udc->usba_ep);
 
@@ -2092,15 +2109,6 @@ static int __exit usba_udc_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#if defined(CONFIG_OF)
-static const struct of_device_id atmel_udc_dt_ids[] = {
-	{ .compatible = "atmel,at91sam9rl-udc" },
-	{ /* sentinel */ }
-};
-
-MODULE_DEVICE_TABLE(of, atmel_udc_dt_ids);
-#endif
-
 static struct platform_driver udc_driver = {
 	.remove		= __exit_p(usba_udc_remove),
 	.driver		= {
diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.h b/drivers/usb/gadget/udc/atmel_usba_udc.h
index a70706e..456899e 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.h
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.h
@@ -304,6 +304,10 @@ struct usba_request {
 	unsigned int				mapped:1;
 };
 
+struct usba_udc_errata {
+	void (*toggle_bias)(struct usba_udc *udc, int is_on);
+};
+
 struct usba_udc {
 	/* Protect hw registers from concurrent modifications */
 	spinlock_t lock;
@@ -314,6 +318,7 @@ struct usba_udc {
 	struct usb_gadget gadget;
 	struct usb_gadget_driver *driver;
 	struct platform_device *pdev;
+	const struct usba_udc_errata *errata;
 	int irq;
 	int vbus_pin;
 	int vbus_pin_inverted;
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] usb: atmel_usba_udc: Add at91sam9g45 and at91sam9x5 errata handling

[Boris Brezillon]

at91sam9g45 and at91sam9x5 SoCs have an hardware bug forcing us to
generate a pulse on the BIAS signal on "USB end of reset” and
“USB end of resume" events.

Reported-by: Patrice VILCHEZ <patrice.vilchez-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/usb/gadget/udc/atmel_usba_udc.c | 28 +++++++++++++++++++++++++++-
 drivers/usb/gadget/udc/atmel_usba_udc.h |  2 ++
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
index 36fd34b..55c8dde 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.c
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
@@ -331,6 +331,17 @@ static void toggle_bias(struct usba_udc *udc, int is_on)
 		udc->errata->toggle_bias(udc, is_on);
 }
 
+static void generate_bias_pulse(struct usba_udc *udc)
+{
+	if (!udc->bias_pulse_needed)
+		return;
+
+	if (udc->errata && udc->errata->pulse_bias)
+		udc->errata->pulse_bias(udc);
+
+	udc->bias_pulse_needed = false;
+}
+
 static void next_fifo_transaction(struct usba_ep *ep, struct usba_request *req)
 {
 	unsigned int transaction_len;
@@ -1607,6 +1618,7 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
 	if (status & USBA_DET_SUSPEND) {
 		toggle_bias(udc, 0);
 		usba_writel(udc, INT_CLR, USBA_DET_SUSPEND);
+		udc->bias_pulse_needed = true;
 		DBG(DBG_BUS, "Suspend detected\n");
 		if (udc->gadget.speed != USB_SPEED_UNKNOWN
 				&& udc->driver && udc->driver->suspend) {
@@ -1624,6 +1636,7 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
 
 	if (status & USBA_END_OF_RESUME) {
 		usba_writel(udc, INT_CLR, USBA_END_OF_RESUME);
+		generate_bias_pulse(udc);
 		DBG(DBG_BUS, "Resume detected\n");
 		if (udc->gadget.speed != USB_SPEED_UNKNOWN
 				&& udc->driver && udc->driver->resume) {
@@ -1659,6 +1672,7 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
 		struct usba_ep *ep0;
 
 		usba_writel(udc, INT_CLR, USBA_END_OF_RESET);
+		generate_bias_pulse(udc);
 		reset_all_endpoints(udc);
 
 		if (udc->gadget.speed != USB_SPEED_UNKNOWN && udc->driver) {
@@ -1818,13 +1832,25 @@ static void at91sam9rl_toggle_bias(struct usba_udc *udc, int is_on)
 		at91_pmc_write(AT91_CKGR_UCKR, uckr & ~(AT91_PMC_BIASEN));
 }
 
+static void at91sam9g45_pulse_bias(struct usba_udc *udc)
+{
+	unsigned int uckr = at91_pmc_read(AT91_CKGR_UCKR);
+
+	at91_pmc_write(AT91_CKGR_UCKR, uckr & ~(AT91_PMC_BIASEN));
+	at91_pmc_write(AT91_CKGR_UCKR, uckr | AT91_PMC_BIASEN);
+}
+
 static const struct usba_udc_errata at91sam9rl_errata = {
 	.toggle_bias = at91sam9rl_toggle_bias,
 };
 
+static const struct usba_udc_errata at91sam9g45_errata = {
+	.pulse_bias = at91sam9g45_pulse_bias,
+};
+
 static const struct of_device_id atmel_udc_dt_ids[] = {
 	{ .compatible = "atmel,at91sam9rl-udc", .data = &at91sam9rl_errata },
-	{ .compatible = "atmel,at91sam9g45-udc" },
+	{ .compatible = "atmel,at91sam9g45-udc", .data = &at91sam9g45_errata },
 	{ .compatible = "atmel,sama5d3-udc" },
 	{ /* sentinel */ }
 };
diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.h b/drivers/usb/gadget/udc/atmel_usba_udc.h
index 456899e..72b3537 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.h
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.h
@@ -306,6 +306,7 @@ struct usba_request {
 
 struct usba_udc_errata {
 	void (*toggle_bias)(struct usba_udc *udc, int is_on);
+	void (*pulse_bias)(struct usba_udc *udc);
 };
 
 struct usba_udc {
@@ -326,6 +327,7 @@ struct usba_udc {
 	struct clk *pclk;
 	struct clk *hclk;
 	struct usba_ep *usba_ep;
+	bool bias_pulse_needed;
 
 	u16 devstatus;
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] ARM: at91/dt: update udc compatible strings

[Boris Brezillon]

at91sam9g45, at91sam9x5 and sama5 SoCs should not use
"atmel,at91sam9rl-udc" for their USB device compatible property since
this compatible is attached to a specific hardware bug fix.

Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/boot/dts/at91sam9g45.dtsi | 2 +-
 arch/arm/boot/dts/at91sam9x5.dtsi  | 2 +-
 arch/arm/boot/dts/sama5d3.dtsi     | 2 +-
 arch/arm/boot/dts/sama5d4.dtsi     | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/at91sam9g45.dtsi b/arch/arm/boot/dts/at91sam9g45.dtsi
index 6c0637a..cf53155 100644
--- a/arch/arm/boot/dts/at91sam9g45.dtsi
+++ b/arch/arm/boot/dts/at91sam9g45.dtsi
@@ -1112,7 +1112,7 @@
 			usb2: gadget@fff78000 {
 				#address-cells = <1>;
 				#size-cells = <0>;
-				compatible = "atmel,at91sam9rl-udc";
+				compatible = "atmel,at91sam9g45-udc";
 				reg = <0x00600000 0x80000
 				       0xfff78000 0x400>;
 				interrupts = <27 IRQ_TYPE_LEVEL_HIGH 0>;
diff --git a/arch/arm/boot/dts/at91sam9x5.dtsi b/arch/arm/boot/dts/at91sam9x5.dtsi
index bbb3ba6..d213147 100644
--- a/arch/arm/boot/dts/at91sam9x5.dtsi
+++ b/arch/arm/boot/dts/at91sam9x5.dtsi
@@ -1057,7 +1057,7 @@
 			usb2: gadget@f803c000 {
 				#address-cells = <1>;
 				#size-cells = <0>;
-				compatible = "atmel,at91sam9rl-udc";
+				compatible = "atmel,at91sam9g45-udc";
 				reg = <0x00500000 0x80000
 				       0xf803c000 0x400>;
 				interrupts = <23 IRQ_TYPE_LEVEL_HIGH 0>;
diff --git a/arch/arm/boot/dts/sama5d3.dtsi b/arch/arm/boot/dts/sama5d3.dtsi
index 5f4144d..2b407a4 100644
--- a/arch/arm/boot/dts/sama5d3.dtsi
+++ b/arch/arm/boot/dts/sama5d3.dtsi
@@ -1264,7 +1264,7 @@
 		usb0: gadget@00500000 {
 			#address-cells = <1>;
 			#size-cells = <0>;
-			compatible = "atmel,at91sam9rl-udc";
+			compatible = "atmel,sama5d3-udc";
 			reg = <0x00500000 0x100000
 			       0xf8030000 0x4000>;
 			interrupts = <33 IRQ_TYPE_LEVEL_HIGH 2>;
diff --git a/arch/arm/boot/dts/sama5d4.dtsi b/arch/arm/boot/dts/sama5d4.dtsi
index e0157b0..0896efe 100644
--- a/arch/arm/boot/dts/sama5d4.dtsi
+++ b/arch/arm/boot/dts/sama5d4.dtsi
@@ -111,7 +111,7 @@
 		usb0: gadget@00400000 {
 			#address-cells = <1>;
 			#size-cells = <0>;
-			compatible = "atmel,at91sam9rl-udc";
+			compatible = "atmel,sama5d3-udc";
 			reg = <0x00400000 0x100000
 			       0xfc02c000 0x4000>;
 			interrupts = <47 IRQ_TYPE_LEVEL_HIGH 2>;
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] usb: atmel_usba_udc: mask status with enabled irqs

[Boris Brezillon]

Avoid interpreting useless status flags when we're not waiting for such
events by masking the status variable with the interrupt enabled register
value.

Reported-by: Patrice VILCHEZ <patrice.vilchez-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/usb/gadget/udc/atmel_usba_udc.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
index 55c8dde..bc3a532 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.c
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
@@ -1612,12 +1612,14 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
 
 	spin_lock(&udc->lock);
 
-	status = usba_readl(udc, INT_STA);
+	status = usba_readl(udc, INT_STA) & usba_readl(udc, INT_ENB);
 	DBG(DBG_INT, "irq, status=%#08x\n", status);
 
 	if (status & USBA_DET_SUSPEND) {
 		toggle_bias(udc, 0);
 		usba_writel(udc, INT_CLR, USBA_DET_SUSPEND);
+		usba_writel(udc, INT_ENB,
+			    usba_readl(udc, INT_ENB) | USBA_WAKE_UP);
 		udc->bias_pulse_needed = true;
 		DBG(DBG_BUS, "Suspend detected\n");
 		if (udc->gadget.speed != USB_SPEED_UNKNOWN
@@ -1631,6 +1633,8 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
 	if (status & USBA_WAKE_UP) {
 		toggle_bias(udc, 1);
 		usba_writel(udc, INT_CLR, USBA_WAKE_UP);
+		usba_writel(udc, INT_ENB,
+			    usba_readl(udc, INT_ENB) & ~USBA_WAKE_UP);
 		DBG(DBG_BUS, "Wake Up CPU detected\n");
 	}
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] usb: atmel_usba_udc: mask status with enabled irqs

[Sergei Shtylyov]

Hello.

On 12/15/2014 4:03 PM, Boris Brezillon wrote:

> Avoid interpreting useless status flags when we're not waiting for such
> events by masking the status variable with the interrupt enabled register
> value.

> Reported-by: Patrice VILCHEZ <patrice.vilchez-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>   drivers/usb/gadget/udc/atmel_usba_udc.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)

> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
> index 55c8dde..bc3a532 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
> @@ -1612,12 +1612,14 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
>
>   	spin_lock(&udc->lock);
>
> -	status = usba_readl(udc, INT_STA);
> +	status = usba_readl(udc, INT_STA) & usba_readl(udc, INT_ENB);
>   	DBG(DBG_INT, "irq, status=%#08x\n", status);
>
>   	if (status & USBA_DET_SUSPEND) {
>   		toggle_bias(udc, 0);
>   		usba_writel(udc, INT_CLR, USBA_DET_SUSPEND);
> +		usba_writel(udc, INT_ENB,
> +			    usba_readl(udc, INT_ENB) | USBA_WAKE_UP);
>   		udc->bias_pulse_needed = true;
>   		DBG(DBG_BUS, "Suspend detected\n");
>   		if (udc->gadget.speed != USB_SPEED_UNKNOWN
> @@ -1631,6 +1633,8 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
>   	if (status & USBA_WAKE_UP) {
>   		toggle_bias(udc, 1);
>   		usba_writel(udc, INT_CLR, USBA_WAKE_UP);
> +		usba_writel(udc, INT_ENB,
> +			    usba_readl(udc, INT_ENB) & ~USBA_WAKE_UP);
>   		DBG(DBG_BUS, "Wake Up CPU detected\n");
>   	}

    Looks like t make sense to read the INT_ENB register into a separate 
variable, to save on extra reads?

WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 4/4] usb: atmel_usba_udc: mask status with enabled irqs

[David Laight]

From: Sergei Shtylyov
> Hello.
> 
> On 12/15/2014 4:03 PM, Boris Brezillon wrote:
> 
> > Avoid interpreting useless status flags when we're not waiting for such
> > events by masking the status variable with the interrupt enabled register
> > value.
> 
> > Reported-by: Patrice VILCHEZ <patrice.vilchez-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> > Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---
> >   drivers/usb/gadget/udc/atmel_usba_udc.c | 6 +++++-
> >   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> > diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
> > index 55c8dde..bc3a532 100644
> > --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
> > +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
> > @@ -1612,12 +1612,14 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
> >
> >   	spin_lock(&udc->lock);
> >
> > -	status = usba_readl(udc, INT_STA);
> > +	status = usba_readl(udc, INT_STA) & usba_readl(udc, INT_ENB);
> >   	DBG(DBG_INT, "irq, status=%#08x\n", status);
> >
> >   	if (status & USBA_DET_SUSPEND) {
> >   		toggle_bias(udc, 0);
> >   		usba_writel(udc, INT_CLR, USBA_DET_SUSPEND);
> > +		usba_writel(udc, INT_ENB,
> > +			    usba_readl(udc, INT_ENB) | USBA_WAKE_UP);
> >   		udc->bias_pulse_needed = true;
> >   		DBG(DBG_BUS, "Suspend detected\n");
> >   		if (udc->gadget.speed != USB_SPEED_UNKNOWN
> > @@ -1631,6 +1633,8 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
> >   	if (status & USBA_WAKE_UP) {
> >   		toggle_bias(udc, 1);
> >   		usba_writel(udc, INT_CLR, USBA_WAKE_UP);
> > +		usba_writel(udc, INT_ENB,
> > +			    usba_readl(udc, INT_ENB) & ~USBA_WAKE_UP);
> >   		DBG(DBG_BUS, "Wake Up CPU detected\n");
> >   	}
> 
>     Looks like t make sense to read the INT_ENB register into a separate
> variable, to save on extra reads?


Better still remember the written value in one of the structures so
that it doesn't have to be read at all.

	David



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] usb: atmel_usba_udc: mask status with enabled irqs

[Boris Brezillon]

On Mon, 15 Dec 2014 13:34:56 +0000
David Laight <David.Laight-ZS65k/vG3HxXrIkS9f7CXA@public.gmane.org> wrote:

> From: Sergei Shtylyov
> > Hello.
> > 
> > On 12/15/2014 4:03 PM, Boris Brezillon wrote:
> > 
> > > Avoid interpreting useless status flags when we're not waiting for such
> > > events by masking the status variable with the interrupt enabled register
> > > value.
> > 
> > > Reported-by: Patrice VILCHEZ <patrice.vilchez-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> > > Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > > ---
> > >   drivers/usb/gadget/udc/atmel_usba_udc.c | 6 +++++-
> > >   1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > > diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
> > > index 55c8dde..bc3a532 100644
> > > --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
> > > +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
> > > @@ -1612,12 +1612,14 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
> > >
> > >   	spin_lock(&udc->lock);
> > >
> > > -	status = usba_readl(udc, INT_STA);
> > > +	status = usba_readl(udc, INT_STA) & usba_readl(udc, INT_ENB);
> > >   	DBG(DBG_INT, "irq, status=%#08x\n", status);
> > >
> > >   	if (status & USBA_DET_SUSPEND) {
> > >   		toggle_bias(udc, 0);
> > >   		usba_writel(udc, INT_CLR, USBA_DET_SUSPEND);
> > > +		usba_writel(udc, INT_ENB,
> > > +			    usba_readl(udc, INT_ENB) | USBA_WAKE_UP);
> > >   		udc->bias_pulse_needed = true;
> > >   		DBG(DBG_BUS, "Suspend detected\n");
> > >   		if (udc->gadget.speed != USB_SPEED_UNKNOWN
> > > @@ -1631,6 +1633,8 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
> > >   	if (status & USBA_WAKE_UP) {
> > >   		toggle_bias(udc, 1);
> > >   		usba_writel(udc, INT_CLR, USBA_WAKE_UP);
> > > +		usba_writel(udc, INT_ENB,
> > > +			    usba_readl(udc, INT_ENB) & ~USBA_WAKE_UP);
> > >   		DBG(DBG_BUS, "Wake Up CPU detected\n");
> > >   	}
> > 
> >     Looks like t make sense to read the INT_ENB register into a separate
> > variable, to save on extra reads?
> 
> 
> Better still remember the written value in one of the structures so
> that it doesn't have to be read at all.

Sure, I'll modify the code accordingly.

Thanks,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] usb: atmel_usba_udc: mask status with enabled irqs

[Boris Brezillon]

Hi David,

On Mon, 15 Dec 2014 13:34:56 +0000
David Laight <David.Laight-ZS65k/vG3HxXrIkS9f7CXA@public.gmane.org> wrote:

> From: Sergei Shtylyov
> > Hello.
> > 
> > On 12/15/2014 4:03 PM, Boris Brezillon wrote:
> > 
> > > Avoid interpreting useless status flags when we're not waiting for such
> > > events by masking the status variable with the interrupt enabled register
> > > value.
> > 
> > > Reported-by: Patrice VILCHEZ <patrice.vilchez-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> > > Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > > ---
> > >   drivers/usb/gadget/udc/atmel_usba_udc.c | 6 +++++-
> > >   1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > > diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
> > > index 55c8dde..bc3a532 100644
> > > --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
> > > +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
> > > @@ -1612,12 +1612,14 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
> > >
> > >   	spin_lock(&udc->lock);
> > >
> > > -	status = usba_readl(udc, INT_STA);
> > > +	status = usba_readl(udc, INT_STA) & usba_readl(udc, INT_ENB);
> > >   	DBG(DBG_INT, "irq, status=%#08x\n", status);
> > >
> > >   	if (status & USBA_DET_SUSPEND) {
> > >   		toggle_bias(udc, 0);
> > >   		usba_writel(udc, INT_CLR, USBA_DET_SUSPEND);
> > > +		usba_writel(udc, INT_ENB,
> > > +			    usba_readl(udc, INT_ENB) | USBA_WAKE_UP);
> > >   		udc->bias_pulse_needed = true;
> > >   		DBG(DBG_BUS, "Suspend detected\n");
> > >   		if (udc->gadget.speed != USB_SPEED_UNKNOWN
> > > @@ -1631,6 +1633,8 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
> > >   	if (status & USBA_WAKE_UP) {
> > >   		toggle_bias(udc, 1);
> > >   		usba_writel(udc, INT_CLR, USBA_WAKE_UP);
> > > +		usba_writel(udc, INT_ENB,
> > > +			    usba_readl(udc, INT_ENB) & ~USBA_WAKE_UP);
> > >   		DBG(DBG_BUS, "Wake Up CPU detected\n");
> > >   	}
> > 
> >     Looks like t make sense to read the INT_ENB register into a separate
> > variable, to save on extra reads?
> 
> 
> Better still remember the written value in one of the structures so
> that it doesn't have to be read at all.

Hmm, I'm getting back to this suggestion.
While I definitely understand why I should use a local variable to
store INT_ENB value in usba_udc_irq, I don't see the point of mirroring
INT_EN status in an udc struct field (after all, INT_EN will always
contain the value we previously set).
Is this a performance concern ?

Regards,

Boris


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 4/4] usb: atmel_usba_udc: mask status with enabled irqs

[David Laight]

From: Boris Brezillon
> Hi David,
> 
> On Mon, 15 Dec 2014 13:34:56 +0000
> David Laight <David.Laight-ZS65k/vG3HxXrIkS9f7CXA@public.gmane.org> wrote:
> 
> > From: Sergei Shtylyov
> > > Hello.
> > >
> > > On 12/15/2014 4:03 PM, Boris Brezillon wrote:
> > >
> > > > Avoid interpreting useless status flags when we're not waiting for such
> > > > events by masking the status variable with the interrupt enabled register
> > > > value.
> > >
> > > > Reported-by: Patrice VILCHEZ <patrice.vilchez-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> > > > Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > > > ---
> > > >   drivers/usb/gadget/udc/atmel_usba_udc.c | 6 +++++-
> > > >   1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > > diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
> > > > index 55c8dde..bc3a532 100644
> > > > --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
> > > > +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
> > > > @@ -1612,12 +1612,14 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
> > > >
> > > >   	spin_lock(&udc->lock);
> > > >
> > > > -	status = usba_readl(udc, INT_STA);
> > > > +	status = usba_readl(udc, INT_STA) & usba_readl(udc, INT_ENB);
...
> > >     Looks like t make sense to read the INT_ENB register into a separate
> > > variable, to save on extra reads?
> >
> >
> > Better still remember the written value in one of the structures so
> > that it doesn't have to be read at all.
> 
> Hmm, I'm getting back to this suggestion.
> While I definitely understand why I should use a local variable to
> store INT_ENB value in usba_udc_irq, I don't see the point of mirroring
> INT_EN status in an udc struct field (after all, INT_EN will always
> contain the value we previously set).

This is exactly why it makes sense to mirror it locally.

> Is this a performance concern ?

Absolutely, you really don't want to know how many cpu cycles it is
likely to take to do a read from an io device.
At best it is a uncached read of a fast on-chip peripheral.
If you are reading from a PCIe device then you are looking at hundreds
(if not thousands) of cpu clock cycles.

	David



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] usb: atmel_usba_udc: mask status with enabled irqs

[Boris Brezillon]

On Mon, 15 Dec 2014 17:22:04 +0000
David Laight <David.Laight-ZS65k/vG3HxXrIkS9f7CXA@public.gmane.org> wrote:

> From: Boris Brezillon
> > Hi David,
> > 
> > On Mon, 15 Dec 2014 13:34:56 +0000
> > David Laight <David.Laight-ZS65k/vG3HxXrIkS9f7CXA@public.gmane.org> wrote:
> > 
> > > From: Sergei Shtylyov
> > > > Hello.
> > > >
> > > > On 12/15/2014 4:03 PM, Boris Brezillon wrote:
> > > >
> > > > > Avoid interpreting useless status flags when we're not waiting for such
> > > > > events by masking the status variable with the interrupt enabled register
> > > > > value.
> > > >
> > > > > Reported-by: Patrice VILCHEZ <patrice.vilchez-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> > > > > Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > > > > ---
> > > > >   drivers/usb/gadget/udc/atmel_usba_udc.c | 6 +++++-
> > > > >   1 file changed, 5 insertions(+), 1 deletion(-)
> > > >
> > > > > diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
> > > > > index 55c8dde..bc3a532 100644
> > > > > --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
> > > > > +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
> > > > > @@ -1612,12 +1612,14 @@ static irqreturn_t usba_udc_irq(int irq, void *devid)
> > > > >
> > > > >   	spin_lock(&udc->lock);
> > > > >
> > > > > -	status = usba_readl(udc, INT_STA);
> > > > > +	status = usba_readl(udc, INT_STA) & usba_readl(udc, INT_ENB);
> ...
> > > >     Looks like t make sense to read the INT_ENB register into a separate
> > > > variable, to save on extra reads?
> > >
> > >
> > > Better still remember the written value in one of the structures so
> > > that it doesn't have to be read at all.
> > 
> > Hmm, I'm getting back to this suggestion.
> > While I definitely understand why I should use a local variable to
> > store INT_ENB value in usba_udc_irq, I don't see the point of mirroring
> > INT_EN status in an udc struct field (after all, INT_EN will always
> > contain the value we previously set).
> 
> This is exactly why it makes sense to mirror it locally.

Absolutely.

> 
> > Is this a performance concern ?
> 
> Absolutely, you really don't want to know how many cpu cycles it is
> likely to take to do a read from an io device.
> At best it is a uncached read of a fast on-chip peripheral.
> If you are reading from a PCIe device then you are looking at hundreds
> (if not thousands) of cpu clock cycles.

I know there is a perf penalty when accessing IO memory regions (in this
case an uncached memory access) compared to standard memory accesses (in
other words a cached accesses), just don't know the exact numbers.
My point was, is the performance improvement worth the addition of this
new field and the code modification (addition of a wrapper function to
modify the interrupt register) ?

I take your answer as a yes ;-).

Thanks,

Boris


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html