* [PATCH 0/4] usb: atmel_usba_udc: Rework errata handling
@ 2014-12-15 13:03 Boris Brezillon
[not found] ` <1418648588-17872-1-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
0 siblings, 1 reply; 11+ messages in thread
From: Boris Brezillon @ 2014-12-15 13:03 UTC (permalink / raw)
To: Felipe Balbi, Greg Kroah-Hartman,
linux-usb-u79uwXL29TY76Z2rM5mHXA, Nicolas Ferre,
Jean-Christophe Plagniol-Villard, Alexandre Belloni,
Andrew Victor
Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
devicetree-u79uwXL29TY76Z2rM5mHXA, 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
^ permalink raw reply [flat|nested] 11+ messages in thread[parent not found: <1418648588-17872-1-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]
* [PATCH 1/4] usb: atmel_usba_udc: Rework at91sam9rl errata handling [not found] ` <1418648588-17872-1-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> @ 2014-12-15 13:03 ` Boris Brezillon 2014-12-15 13:03 ` [PATCH 2/4] usb: atmel_usba_udc: Add at91sam9g45 and at91sam9x5 " Boris Brezillon ` (2 subsequent siblings) 3 siblings, 0 replies; 11+ messages in thread From: Boris Brezillon @ 2014-12-15 13:03 UTC (permalink / raw) To: Felipe Balbi, Greg Kroah-Hartman, linux-usb-u79uwXL29TY76Z2rM5mHXA, Nicolas Ferre, Jean-Christophe Plagniol-Villard, Alexandre Belloni, Andrew Victor Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/4] usb: atmel_usba_udc: Add at91sam9g45 and at91sam9x5 errata handling [not found] ` <1418648588-17872-1-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 2014-12-15 13:03 ` [PATCH 1/4] usb: atmel_usba_udc: Rework at91sam9rl " Boris Brezillon @ 2014-12-15 13:03 ` Boris Brezillon 2014-12-15 13:03 ` [PATCH 3/4] ARM: at91/dt: update udc compatible strings Boris Brezillon 2014-12-15 13:03 ` [PATCH 4/4] usb: atmel_usba_udc: mask status with enabled irqs Boris Brezillon 3 siblings, 0 replies; 11+ messages in thread From: Boris Brezillon @ 2014-12-15 13:03 UTC (permalink / raw) To: Felipe Balbi, Greg Kroah-Hartman, linux-usb-u79uwXL29TY76Z2rM5mHXA, Nicolas Ferre, Jean-Christophe Plagniol-Villard, Alexandre Belloni, Andrew Victor Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/4] ARM: at91/dt: update udc compatible strings [not found] ` <1418648588-17872-1-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 2014-12-15 13:03 ` [PATCH 1/4] usb: atmel_usba_udc: Rework at91sam9rl " Boris Brezillon 2014-12-15 13:03 ` [PATCH 2/4] usb: atmel_usba_udc: Add at91sam9g45 and at91sam9x5 " Boris Brezillon @ 2014-12-15 13:03 ` Boris Brezillon 2014-12-15 13:03 ` [PATCH 4/4] usb: atmel_usba_udc: mask status with enabled irqs Boris Brezillon 3 siblings, 0 replies; 11+ messages in thread From: Boris Brezillon @ 2014-12-15 13:03 UTC (permalink / raw) To: Felipe Balbi, Greg Kroah-Hartman, linux-usb-u79uwXL29TY76Z2rM5mHXA, Nicolas Ferre, Jean-Christophe Plagniol-Villard, Alexandre Belloni, Andrew Victor Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 4/4] usb: atmel_usba_udc: mask status with enabled irqs [not found] ` <1418648588-17872-1-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> ` (2 preceding siblings ...) 2014-12-15 13:03 ` [PATCH 3/4] ARM: at91/dt: update udc compatible strings Boris Brezillon @ 2014-12-15 13:03 ` Boris Brezillon [not found] ` <1418648588-17872-5-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 3 siblings, 1 reply; 11+ messages in thread From: Boris Brezillon @ 2014-12-15 13:03 UTC (permalink / raw) To: Felipe Balbi, Greg Kroah-Hartman, linux-usb-u79uwXL29TY76Z2rM5mHXA, Nicolas Ferre, Jean-Christophe Plagniol-Villard, Alexandre Belloni, Andrew Victor Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
[parent not found: <1418648588-17872-5-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]
* Re: [PATCH 4/4] usb: atmel_usba_udc: mask status with enabled irqs [not found] ` <1418648588-17872-5-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> @ 2014-12-15 13:32 ` Sergei Shtylyov [not found] ` <548EE2DA.6050408-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org> 0 siblings, 1 reply; 11+ messages in thread From: Sergei Shtylyov @ 2014-12-15 13:32 UTC (permalink / raw) To: Boris Brezillon, Felipe Balbi, Greg Kroah-Hartman, linux-usb-u79uwXL29TY76Z2rM5mHXA, Nicolas Ferre, Jean-Christophe Plagniol-Villard, Alexandre Belloni, Andrew Victor Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <548EE2DA.6050408-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org>]
* RE: [PATCH 4/4] usb: atmel_usba_udc: mask status with enabled irqs [not found] ` <548EE2DA.6050408-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org> @ 2014-12-15 13:34 ` David Laight [not found] ` <063D6719AE5E284EB5DD2968C1650D6D1CA0CE20-VkEWCZq2GCInGFn1LkZF6NBPR1lH4CV8@public.gmane.org> 0 siblings, 1 reply; 11+ messages in thread From: David Laight @ 2014-12-15 13:34 UTC (permalink / raw) To: 'Sergei Shtylyov', Boris Brezillon, Felipe Balbi, Greg Kroah-Hartman, linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Nicolas Ferre, Jean-Christophe Plagniol-Villard, Alexandre Belloni, Andrew Victor Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <063D6719AE5E284EB5DD2968C1650D6D1CA0CE20-VkEWCZq2GCInGFn1LkZF6NBPR1lH4CV8@public.gmane.org>]
* Re: [PATCH 4/4] usb: atmel_usba_udc: mask status with enabled irqs [not found] ` <063D6719AE5E284EB5DD2968C1650D6D1CA0CE20-VkEWCZq2GCInGFn1LkZF6NBPR1lH4CV8@public.gmane.org> @ 2014-12-15 14:01 ` Boris Brezillon 2014-12-15 17:02 ` Boris Brezillon 1 sibling, 0 replies; 11+ messages in thread From: Boris Brezillon @ 2014-12-15 14:01 UTC (permalink / raw) To: David Laight Cc: 'Sergei Shtylyov', Felipe Balbi, Greg Kroah-Hartman, linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Nicolas Ferre, Jean-Christophe Plagniol-Villard, Alexandre Belloni, Andrew Victor, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] usb: atmel_usba_udc: mask status with enabled irqs [not found] ` <063D6719AE5E284EB5DD2968C1650D6D1CA0CE20-VkEWCZq2GCInGFn1LkZF6NBPR1lH4CV8@public.gmane.org> 2014-12-15 14:01 ` Boris Brezillon @ 2014-12-15 17:02 ` Boris Brezillon 2014-12-15 17:22 ` David Laight 1 sibling, 1 reply; 11+ messages in thread From: Boris Brezillon @ 2014-12-15 17:02 UTC (permalink / raw) To: David Laight Cc: 'Sergei Shtylyov', Felipe Balbi, Greg Kroah-Hartman, linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Nicolas Ferre, Jean-Christophe Plagniol-Villard, Alexandre Belloni, Andrew Victor, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 4/4] usb: atmel_usba_udc: mask status with enabled irqs 2014-12-15 17:02 ` Boris Brezillon @ 2014-12-15 17:22 ` David Laight [not found] ` <063D6719AE5E284EB5DD2968C1650D6D1CA0D0EF-VkEWCZq2GCInGFn1LkZF6NBPR1lH4CV8@public.gmane.org> 0 siblings, 1 reply; 11+ messages in thread From: David Laight @ 2014-12-15 17:22 UTC (permalink / raw) To: 'Boris Brezillon' Cc: 'Sergei Shtylyov', Felipe Balbi, Greg Kroah-Hartman, linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Nicolas Ferre, Jean-Christophe Plagniol-Villard, Alexandre Belloni, Andrew Victor, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <063D6719AE5E284EB5DD2968C1650D6D1CA0D0EF-VkEWCZq2GCInGFn1LkZF6NBPR1lH4CV8@public.gmane.org>]
* Re: [PATCH 4/4] usb: atmel_usba_udc: mask status with enabled irqs [not found] ` <063D6719AE5E284EB5DD2968C1650D6D1CA0D0EF-VkEWCZq2GCInGFn1LkZF6NBPR1lH4CV8@public.gmane.org> @ 2014-12-15 18:12 ` Boris Brezillon 0 siblings, 0 replies; 11+ messages in thread From: Boris Brezillon @ 2014-12-15 18:12 UTC (permalink / raw) To: David Laight Cc: 'Sergei Shtylyov', Felipe Balbi, Greg Kroah-Hartman, linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Nicolas Ferre, Jean-Christophe Plagniol-Villard, Alexandre Belloni, Andrew Victor, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-12-15 18:12 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-12-15 13:03 [PATCH 0/4] usb: atmel_usba_udc: Rework errata handling Boris Brezillon
[not found] ` <1418648588-17872-1-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
2014-12-15 13:03 ` [PATCH 1/4] usb: atmel_usba_udc: Rework at91sam9rl " Boris Brezillon
2014-12-15 13:03 ` [PATCH 2/4] usb: atmel_usba_udc: Add at91sam9g45 and at91sam9x5 " Boris Brezillon
2014-12-15 13:03 ` [PATCH 3/4] ARM: at91/dt: update udc compatible strings Boris Brezillon
2014-12-15 13:03 ` [PATCH 4/4] usb: atmel_usba_udc: mask status with enabled irqs Boris Brezillon
[not found] ` <1418648588-17872-5-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
2014-12-15 13:32 ` Sergei Shtylyov
[not found] ` <548EE2DA.6050408-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org>
2014-12-15 13:34 ` David Laight
[not found] ` <063D6719AE5E284EB5DD2968C1650D6D1CA0CE20-VkEWCZq2GCInGFn1LkZF6NBPR1lH4CV8@public.gmane.org>
2014-12-15 14:01 ` Boris Brezillon
2014-12-15 17:02 ` Boris Brezillon
2014-12-15 17:22 ` David Laight
[not found] ` <063D6719AE5E284EB5DD2968C1650D6D1CA0D0EF-VkEWCZq2GCInGFn1LkZF6NBPR1lH4CV8@public.gmane.org>
2014-12-15 18:12 ` Boris Brezillon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox