[v3,1/2] usb: ohci-at91: Forcibly suspend ports while USB suspend

Commit Message

Wenyou Yang June 8, 2016, 4:15 a.m. UTC

In order to the save power consumption, as a workaround, suspend
forcibly the USB PORTA/B/C via set the SUSPEND_A/B/C bits of OHCI
Interrupt Configuration Register in the SFRs while OHCI USB suspend.

This suspend operation must be done before the USB clock is disabled,
resume after the USB clock is enabled.

Signed-off-by: Wenyou Yang <[e-mail]>
---

Changes in v3:
 - Change the compatible description for more precise.

Changes in v2:
 - Add compatible to support forcibly suspend the ports.
 - Add soc/at91/at91_sfr.h to accommodate the defines.
 - Add error checking for .sfr_regmap.
 - Remove unnecessary regmap_read() statement.

 .../devicetree/bindings/usb/atmel-usb.txt          |  6 +-
 drivers/usb/host/ohci-at91.c                       | 80 +++++++++++++++++++++-
 include/soc/at91/at91_sfr.h                        | 29 ++++++++
 3 files changed, 112 insertions(+), 3 deletions(-)
 create mode 100644 include/soc/at91/at91_sfr.h

Comments

Nicolas Ferre June 8, 2016, 10:04 a.m. UTC | #1

Le 08/06/2016 06:15, Wenyou Yang a écrit :
> In order to the save power consumption, as a workaround, suspend
> forcibly the USB PORTA/B/C via set the SUSPEND_A/B/C bits of OHCI
> Interrupt Configuration Register in the SFRs while OHCI USB suspend.
> 
> This suspend operation must be done before the USB clock is disabled,
> resume after the USB clock is enabled.
> 
> Signed-off-by: Wenyou Yang <[e-mail]>

Little nitpicking below...

> ---
> 
> Changes in v3:
>  - Change the compatible description for more precise.
> 
> Changes in v2:
>  - Add compatible to support forcibly suspend the ports.
>  - Add soc/at91/at91_sfr.h to accommodate the defines.
>  - Add error checking for .sfr_regmap.
>  - Remove unnecessary regmap_read() statement.
> 
>  .../devicetree/bindings/usb/atmel-usb.txt          |  6 +-
>  drivers/usb/host/ohci-at91.c                       | 80 +++++++++++++++++++++-
>  include/soc/at91/at91_sfr.h                        | 29 ++++++++
>  3 files changed, 112 insertions(+), 3 deletions(-)
>  create mode 100644 include/soc/at91/at91_sfr.h
> 
> diff --git a/Documentation/devicetree/bindings/usb/atmel-usb.txt b/Documentation/devicetree/bindings/usb/atmel-usb.txt
> index 5883b73..888deaa 100644
> --- a/Documentation/devicetree/bindings/usb/atmel-usb.txt
> +++ b/Documentation/devicetree/bindings/usb/atmel-usb.txt
> @@ -3,8 +3,10 @@ Atmel SOC USB controllers
>  OHCI
>  
>  Required properties:
> - - compatible: Should be "atmel,at91rm9200-ohci" for USB controllers
> -   used in host mode.
> + - compatible: Should be one of the following
> +	       "atmel,at91rm9200-ohci" for USB controllers used in host mode.
> +	       "atmel,sama5d2-ohci" for USB controllers used in host mode
> +	       on SAMA5D2 which can force to suspend.

What about "...on SAMA5D2 and compatible SoCs that must explicitly force
suspend through the Special Function Register (SFR)."

>   - reg: Address and length of the register set for the device
>   - interrupts: Should contain ehci interrupt
>   - clocks: Should reference the peripheral, host and system clocks
> diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c
> index d177372..54e8feb 100644
> --- a/drivers/usb/host/ohci-at91.c
> +++ b/drivers/usb/host/ohci-at91.c
> @@ -21,8 +21,11 @@
>  #include <linux/io.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
>  #include <linux/usb.h>
>  #include <linux/usb/hcd.h>
> +#include <soc/at91/at91_sfr.h>
>  
>  #include "ohci.h"
>  
> @@ -45,12 +48,18 @@ struct at91_usbh_data {
>  	u8 overcurrent_changed[AT91_MAX_USBH_PORTS];
>  };
>  
> +struct ohci_at91_caps {
> +	bool suspend_ctrl;
> +};
> +
>  struct ohci_at91_priv {
>  	struct clk *iclk;
>  	struct clk *fclk;
>  	struct clk *hclk;
>  	bool clocked;
>  	bool wakeup;		/* Saved wake-up state for resume */
> +	const struct ohci_at91_caps *caps;
> +	struct regmap *sfr_regmap;
>  };
>  /* interface and function clocks; sometimes also an AHB clock */
>  
> @@ -132,6 +141,17 @@ static void at91_stop_hc(struct platform_device *pdev)
>  
>  /*-------------------------------------------------------------------------*/
>  
> +struct regmap *at91_dt_syscon_sfr(void)
> +{
> +	struct regmap *regmap;
> +
> +	regmap = syscon_regmap_lookup_by_compatible("atmel,sama5d2-sfr");
> +	if (IS_ERR(regmap))
> +		regmap = NULL;
> +
> +	return regmap;
> +}
> +
>  static void usb_hcd_at91_remove (struct usb_hcd *, struct platform_device *);
>  
>  /* configure so an HC device and id are always provided */
> @@ -197,6 +217,17 @@ static int usb_hcd_at91_probe(const struct hc_driver *driver,
>  		goto err;
>  	}
>  
> +	ohci_at91->caps = (const struct ohci_at91_caps *)
> +			  of_device_get_match_data(&pdev->dev);
> +	if (!ohci_at91->caps)
> +		return -ENODEV;
> +
> +	if (ohci_at91->caps->suspend_ctrl) {
> +		ohci_at91->sfr_regmap = at91_dt_syscon_sfr();
> +		if (!ohci_at91->sfr_regmap)
> +			dev_warn(dev, "failed to find sfr node\n");
> +	}
> +
>  	board = hcd->self.controller->platform_data;
>  	ohci = hcd_to_ohci(hcd);
>  	ohci->num_ports = board->ports;
> @@ -440,8 +471,17 @@ static irqreturn_t ohci_hcd_at91_overcurrent_irq(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +static const struct ohci_at91_caps at91rm9200_caps = {
> +	.suspend_ctrl = false,
> +};
> +
> +static const struct ohci_at91_caps sama5d2_caps = {
> +	.suspend_ctrl = true,
> +};
> +
>  static const struct of_device_id at91_ohci_dt_ids[] = {
> -	{ .compatible = "atmel,at91rm9200-ohci" },
> +	{ .compatible = "atmel,at91rm9200-ohci", .data = &at91rm9200_caps },
> +	{ .compatible = "atmel,sama5d2-ohci", .data = &sama5d2_caps },
>  	{ /* sentinel */ }
>  };
>  
> @@ -581,6 +621,38 @@ static int ohci_hcd_at91_drv_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static int ohci_at91_port_ctrl(struct regmap *regmap, bool enable)
> +{
> +	u32 regval;
> +	int ret;
> +
> +	if (!regmap)
> +		return -EINVAL;
> +
> +	ret = regmap_read(regmap, SFR_OHCIICR, &regval);
> +	if (ret)
> +		return ret;
> +
> +	if (enable)
> +		regval &= ~SFR_OHCIICR_USB_SUSPEND;
> +	else
> +		regval |= SFR_OHCIICR_USB_SUSPEND;
> +
> +	regmap_write(regmap, SFR_OHCIICR, regval);
> +
> +	return 0;
> +}
> +
> +static int ohci_at91_port_suspend(struct regmap *regmap)
> +{
> +	return ohci_at91_port_ctrl(regmap, false);
> +}
> +
> +static int ohci_at91_port_resume(struct regmap *regmap)
> +{
> +	return ohci_at91_port_ctrl(regmap, true);
> +}
> +
>  static int __maybe_unused
>  ohci_hcd_at91_drv_suspend(struct device *dev)
>  {
> @@ -618,6 +690,9 @@ ohci_hcd_at91_drv_suspend(struct device *dev)
>  		ohci_writel(ohci, ohci->hc_control, &ohci->regs->control);
>  		ohci->rh_state = OHCI_RH_HALTED;
>  
> +		if (ohci_at91->caps->suspend_ctrl)
> +			ohci_at91_port_suspend(ohci_at91->sfr_regmap);
> +
>  		/* flush the writes */
>  		(void) ohci_readl (ohci, &ohci->regs->control);
>  		at91_stop_clock(ohci_at91);
> @@ -637,6 +712,9 @@ ohci_hcd_at91_drv_resume(struct device *dev)
>  
>  	at91_start_clock(ohci_at91);
>  
> +	if (ohci_at91->caps->suspend_ctrl)
> +		ohci_at91_port_resume(ohci_at91->sfr_regmap);
> +
>  	ohci_resume(hcd, false);
>  	return 0;
>  }
> diff --git a/include/soc/at91/at91_sfr.h b/include/soc/at91/at91_sfr.h
> new file mode 100644
> index 0000000..04a3a1e
> --- /dev/null
> +++ b/include/soc/at91/at91_sfr.h
> @@ -0,0 +1,29 @@
> +/*
> + * Header file for the Atmel DDR/SDR SDRAM Controller

It's not for DDR controller, isn't it?


> + *
> + * Copyright (C) 2016 Atmel Corporation
> + *
> + * Author: Wenyou Yang <[e-mail]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +#ifndef __AT91_SFR_H__
> +#define __AT91_SFR_H__
> +
> +#define SFR_DDRCFG		0x04	/* DDR Configuration Register */
> +/* 0x08 ~ 0x0c: Reserved */
> +#define SFR_OHCIICR		0x10	/* OHCI Interrupt Configuration Register */
> +#define SFR_OHCIISR		0x14	/* OHCI Interrupt Status Register */
> +
> +#define SFR_OHCIICR_SUSPEND_A	BIT(8)
> +#define SFR_OHCIICR_SUSPEND_B	BIT(9)
> +#define SFR_OHCIICR_SUSPEND_C	BIT(10)
> +
> +#define SFR_OHCIICR_USB_SUSPEND	(SFR_OHCIICR_SUSPEND_A | \
> +				 SFR_OHCIICR_SUSPEND_B | \
> +				 SFR_OHCIICR_SUSPEND_C)
> +
> +#endif
> 

But you can take my:

Acked-by: Nicolas Ferre <[e-mail]>

with the little corrections listed.

Alan, We plan to take the second patch of this series with AT91 git tree
through arm-soc. Do you agree to take this one through yours?

Bye,

Nicolas Ferre June 8, 2016, 10:45 a.m. UTC | #2

Le 08/06/2016 12:04, Nicolas Ferre a écrit :
> Le 08/06/2016 06:15, Wenyou Yang a écrit :
>> In order to the save power consumption, as a workaround, suspend
>> forcibly the USB PORTA/B/C via set the SUSPEND_A/B/C bits of OHCI
>> Interrupt Configuration Register in the SFRs while OHCI USB suspend.
>>
>> This suspend operation must be done before the USB clock is disabled,
>> resume after the USB clock is enabled.
>>
>> Signed-off-by: Wenyou Yang <[e-mail]>
> 
> Little nitpicking below...
> 
>> ---
>>
>> Changes in v3:
>>  - Change the compatible description for more precise.
>>
>> Changes in v2:
>>  - Add compatible to support forcibly suspend the ports.
>>  - Add soc/at91/at91_sfr.h to accommodate the defines.
>>  - Add error checking for .sfr_regmap.
>>  - Remove unnecessary regmap_read() statement.
>>
>>  .../devicetree/bindings/usb/atmel-usb.txt          |  6 +-
>>  drivers/usb/host/ohci-at91.c                       | 80 +++++++++++++++++++++-
>>  include/soc/at91/at91_sfr.h                        | 29 ++++++++

Oops sorry, additional comment which is not nitpicking, this one:

We already have SFR header file in this patch:

Author: Cyrille Pitchen <[e-mail]>
Date:   Thu Mar 17 17:04:00 2016 +0100

    ARM: dts: at91: sama5d2: add SFR node

    This SFR node is looked up by the I2S controller driver to tune the
    SFR_I2SCLKSEL register.

    Signed-off-by: Cyrille Pitchen <[e-mail]>
    Signed-off-by: Ludovic Desroches <[e-mail]>
    Acked-by: Alexandre Belloni <[e-mail]>
    Acked-by: Rob Herring <[e-mail]>
    Signed-off-by: Nicolas Ferre <[e-mail]>

Which is already accepted by arm-soc guys for 4.7... So my ack
transforms into a nack, sorry...

We will have to coordinate the effort and maybe take the whole series
with us. But for sure, you'll have to use the existing
include/soc/at91/atmel-sfr.h file and build on top of it...

Bye,

>>  3 files changed, 112 insertions(+), 3 deletions(-)
>>  create mode 100644 include/soc/at91/at91_sfr.h

[..]

> 
> But you can take my:
> 
> Acked-by: Nicolas Ferre <[e-mail]>
> 
> with the little corrections listed.
> 
> Alan, We plan to take the second patch of this series with AT91 git tree
> through arm-soc. Do you agree to take this one through yours?

Alan, forget this request, we'll have to coordinate differently.

Sorry for the noise. Bye,

Alan Stern June 8, 2016, 7:13 p.m. UTC | #3

On Wed, 8 Jun 2016, Wenyou Yang wrote:

> In order to the save power consumption, as a workaround, suspend
> forcibly the USB PORTA/B/C via set the SUSPEND_A/B/C bits of OHCI
> Interrupt Configuration Register in the SFRs while OHCI USB suspend.
> 
> This suspend operation must be done before the USB clock is disabled,
> resume after the USB clock is enabled.
> 
> Signed-off-by: Wenyou Yang <[e-mail]>
> ---

You never answered the questions I posted for the first version of this 
patch:

What does this mean?  What does suspending a port do?  Is it the same 
as a normal USB port suspend?

If it is the same, why doesn't the USB_PORT_FEAT_SUSPEND subcase of the 
SetPortFeature case in ohci_hub_control() already take care of this?

Alan Stern

Rob Herring June 8, 2016, 8:26 p.m. UTC | #4

On Wed, Jun 08, 2016 at 12:15:10PM +0800, Wenyou Yang wrote:
> In order to the save power consumption, as a workaround, suspend
> forcibly the USB PORTA/B/C via set the SUSPEND_A/B/C bits of OHCI
> Interrupt Configuration Register in the SFRs while OHCI USB suspend.
> 
> This suspend operation must be done before the USB clock is disabled,
> resume after the USB clock is enabled.
> 
> Signed-off-by: Wenyou Yang <[e-mail]>
> ---
> 
> Changes in v3:
>  - Change the compatible description for more precise.
> 
> Changes in v2:
>  - Add compatible to support forcibly suspend the ports.
>  - Add soc/at91/at91_sfr.h to accommodate the defines.
>  - Add error checking for .sfr_regmap.
>  - Remove unnecessary regmap_read() statement.
> 
>  .../devicetree/bindings/usb/atmel-usb.txt          |  6 +-
>  drivers/usb/host/ohci-at91.c                       | 80 +++++++++++++++++++++-
>  include/soc/at91/at91_sfr.h                        | 29 ++++++++
>  3 files changed, 112 insertions(+), 3 deletions(-)
>  create mode 100644 include/soc/at91/at91_sfr.h
> 
> diff --git a/Documentation/devicetree/bindings/usb/atmel-usb.txt b/Documentation/devicetree/bindings/usb/atmel-usb.txt
> index 5883b73..888deaa 100644
> --- a/Documentation/devicetree/bindings/usb/atmel-usb.txt
> +++ b/Documentation/devicetree/bindings/usb/atmel-usb.txt
> @@ -3,8 +3,10 @@ Atmel SOC USB controllers
>  OHCI
>  
>  Required properties:
> - - compatible: Should be "atmel,at91rm9200-ohci" for USB controllers
> -   used in host mode.
> + - compatible: Should be one of the following
> +	       "atmel,at91rm9200-ohci" for USB controllers used in host mode.
> +	       "atmel,sama5d2-ohci" for USB controllers used in host mode
> +	       on SAMA5D2 which can force to suspend.

Guess I wasn't clear enough before. Drop "which can force to suspend".

>   - reg: Address and length of the register set for the device
>   - interrupts: Should contain ehci interrupt
>   - clocks: Should reference the peripheral, host and system clocks
> diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c
> index d177372..54e8feb 100644
> --- a/drivers/usb/host/ohci-at91.c
> +++ b/drivers/usb/host/ohci-at91.c
> @@ -21,8 +21,11 @@
>  #include <linux/io.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
>  #include <linux/usb.h>
>  #include <linux/usb/hcd.h>
> +#include <soc/at91/at91_sfr.h>
>  
>  #include "ohci.h"
>  
> @@ -45,12 +48,18 @@ struct at91_usbh_data {
>  	u8 overcurrent_changed[AT91_MAX_USBH_PORTS];
>  };
>  
> +struct ohci_at91_caps {
> +	bool suspend_ctrl;
> +};
> +
>  struct ohci_at91_priv {
>  	struct clk *iclk;
>  	struct clk *fclk;
>  	struct clk *hclk;
>  	bool clocked;
>  	bool wakeup;		/* Saved wake-up state for resume */
> +	const struct ohci_at91_caps *caps;
> +	struct regmap *sfr_regmap;
>  };
>  /* interface and function clocks; sometimes also an AHB clock */
>  
> @@ -132,6 +141,17 @@ static void at91_stop_hc(struct platform_device *pdev)
>  
>  /*-------------------------------------------------------------------------*/
>  
> +struct regmap *at91_dt_syscon_sfr(void)
> +{
> +	struct regmap *regmap;
> +
> +	regmap = syscon_regmap_lookup_by_compatible("atmel,sama5d2-sfr");
> +	if (IS_ERR(regmap))
> +		regmap = NULL;
> +
> +	return regmap;
> +}
> +
>  static void usb_hcd_at91_remove (struct usb_hcd *, struct platform_device *);
>  
>  /* configure so an HC device and id are always provided */
> @@ -197,6 +217,17 @@ static int usb_hcd_at91_probe(const struct hc_driver *driver,
>  		goto err;
>  	}
>  
> +	ohci_at91->caps = (const struct ohci_at91_caps *)
> +			  of_device_get_match_data(&pdev->dev);
> +	if (!ohci_at91->caps)
> +		return -ENODEV;
> +
> +	if (ohci_at91->caps->suspend_ctrl) {
> +		ohci_at91->sfr_regmap = at91_dt_syscon_sfr();
> +		if (!ohci_at91->sfr_regmap)
> +			dev_warn(dev, "failed to find sfr node\n");
> +	}
> +
>  	board = hcd->self.controller->platform_data;
>  	ohci = hcd_to_ohci(hcd);
>  	ohci->num_ports = board->ports;
> @@ -440,8 +471,17 @@ static irqreturn_t ohci_hcd_at91_overcurrent_irq(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +static const struct ohci_at91_caps at91rm9200_caps = {
> +	.suspend_ctrl = false,
> +};
> +
> +static const struct ohci_at91_caps sama5d2_caps = {
> +	.suspend_ctrl = true,
> +};
> +
>  static const struct of_device_id at91_ohci_dt_ids[] = {
> -	{ .compatible = "atmel,at91rm9200-ohci" },
> +	{ .compatible = "atmel,at91rm9200-ohci", .data = &at91rm9200_caps },
> +	{ .compatible = "atmel,sama5d2-ohci", .data = &sama5d2_caps },
>  	{ /* sentinel */ }
>  };
>  
> @@ -581,6 +621,38 @@ static int ohci_hcd_at91_drv_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static int ohci_at91_port_ctrl(struct regmap *regmap, bool enable)
> +{
> +	u32 regval;
> +	int ret;
> +
> +	if (!regmap)
> +		return -EINVAL;
> +
> +	ret = regmap_read(regmap, SFR_OHCIICR, &regval);
> +	if (ret)
> +		return ret;
> +
> +	if (enable)
> +		regval &= ~SFR_OHCIICR_USB_SUSPEND;
> +	else
> +		regval |= SFR_OHCIICR_USB_SUSPEND;
> +
> +	regmap_write(regmap, SFR_OHCIICR, regval);
> +
> +	return 0;
> +}
> +
> +static int ohci_at91_port_suspend(struct regmap *regmap)
> +{
> +	return ohci_at91_port_ctrl(regmap, false);
> +}
> +
> +static int ohci_at91_port_resume(struct regmap *regmap)
> +{
> +	return ohci_at91_port_ctrl(regmap, true);
> +}
> +
>  static int __maybe_unused
>  ohci_hcd_at91_drv_suspend(struct device *dev)
>  {
> @@ -618,6 +690,9 @@ ohci_hcd_at91_drv_suspend(struct device *dev)
>  		ohci_writel(ohci, ohci->hc_control, &ohci->regs->control);
>  		ohci->rh_state = OHCI_RH_HALTED;
>  
> +		if (ohci_at91->caps->suspend_ctrl)
> +			ohci_at91_port_suspend(ohci_at91->sfr_regmap);
> +
>  		/* flush the writes */
>  		(void) ohci_readl (ohci, &ohci->regs->control);
>  		at91_stop_clock(ohci_at91);
> @@ -637,6 +712,9 @@ ohci_hcd_at91_drv_resume(struct device *dev)
>  
>  	at91_start_clock(ohci_at91);
>  
> +	if (ohci_at91->caps->suspend_ctrl)
> +		ohci_at91_port_resume(ohci_at91->sfr_regmap);
> +
>  	ohci_resume(hcd, false);
>  	return 0;
>  }
> diff --git a/include/soc/at91/at91_sfr.h b/include/soc/at91/at91_sfr.h
> new file mode 100644
> index 0000000..04a3a1e
> --- /dev/null
> +++ b/include/soc/at91/at91_sfr.h
> @@ -0,0 +1,29 @@
> +/*
> + * Header file for the Atmel DDR/SDR SDRAM Controller
> + *
> + * Copyright (C) 2016 Atmel Corporation
> + *
> + * Author: Wenyou Yang <[e-mail]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +#ifndef __AT91_SFR_H__
> +#define __AT91_SFR_H__
> +
> +#define SFR_DDRCFG		0x04	/* DDR Configuration Register */
> +/* 0x08 ~ 0x0c: Reserved */
> +#define SFR_OHCIICR		0x10	/* OHCI Interrupt Configuration Register */
> +#define SFR_OHCIISR		0x14	/* OHCI Interrupt Status Register */
> +
> +#define SFR_OHCIICR_SUSPEND_A	BIT(8)
> +#define SFR_OHCIICR_SUSPEND_B	BIT(9)
> +#define SFR_OHCIICR_SUSPEND_C	BIT(10)
> +
> +#define SFR_OHCIICR_USB_SUSPEND	(SFR_OHCIICR_SUSPEND_A | \
> +				 SFR_OHCIICR_SUSPEND_B | \
> +				 SFR_OHCIICR_SUSPEND_C)
> +
> +#endif
> -- 
> 2.7.4
>

Alexandre Belloni June 8, 2016, 8:38 p.m. UTC | #5

On 08/06/2016 at 15:26:51 -0500, Rob Herring wrote :
> On Wed, Jun 08, 2016 at 12:15:10PM +0800, Wenyou Yang wrote:
> > In order to the save power consumption, as a workaround, suspend
> > forcibly the USB PORTA/B/C via set the SUSPEND_A/B/C bits of OHCI
> > Interrupt Configuration Register in the SFRs while OHCI USB suspend.
> > 
> > This suspend operation must be done before the USB clock is disabled,
> > resume after the USB clock is enabled.
> > 
> > Signed-off-by: Wenyou Yang <[e-mail]>
> > ---
> > 
> > Changes in v3:
> >  - Change the compatible description for more precise.
> > 
> > Changes in v2:
> >  - Add compatible to support forcibly suspend the ports.
> >  - Add soc/at91/at91_sfr.h to accommodate the defines.
> >  - Add error checking for .sfr_regmap.
> >  - Remove unnecessary regmap_read() statement.
> > 
> >  .../devicetree/bindings/usb/atmel-usb.txt          |  6 +-
> >  drivers/usb/host/ohci-at91.c                       | 80 +++++++++++++++++++++-
> >  include/soc/at91/at91_sfr.h                        | 29 ++++++++
> >  3 files changed, 112 insertions(+), 3 deletions(-)
> >  create mode 100644 include/soc/at91/at91_sfr.h
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/atmel-usb.txt b/Documentation/devicetree/bindings/usb/atmel-usb.txt
> > index 5883b73..888deaa 100644
> > --- a/Documentation/devicetree/bindings/usb/atmel-usb.txt
> > +++ b/Documentation/devicetree/bindings/usb/atmel-usb.txt
> > @@ -3,8 +3,10 @@ Atmel SOC USB controllers
> >  OHCI
> >  
> >  Required properties:
> > - - compatible: Should be "atmel,at91rm9200-ohci" for USB controllers
> > -   used in host mode.
> > + - compatible: Should be one of the following
> > +	       "atmel,at91rm9200-ohci" for USB controllers used in host mode.
> > +	       "atmel,sama5d2-ohci" for USB controllers used in host mode
> > +	       on SAMA5D2 which can force to suspend.
> 
> Guess I wasn't clear enough before. Drop "which can force to suspend".
> 

Well, my point is that we don't need a new compatible anyway.

Wenyou Yang June 10, 2016, 8:58 a.m. UTC | #6

Hi Alan, 

> -----Original Message-----

> From: Alan Stern [mailto:[e-mail]]

> Sent: 2016年6月9日 3:14

> To: Yang, Wenyou <[e-mail]>

> Cc: Greg Kroah-Hartman <[e-mail]>; Ferre, Nicolas

> <[e-mail]>; Rob Herring <[e-mail]>; Pawel Moll

> <[e-mail]>; Mark Brown <[e-mail]>; Ian Campbell

> <[e-mail]>; Kumar Gala <[e-mail]>;

> Alexandre Belloni <[e-mail]>; Kernel development

> list <[e-mail]>; [e-mail]; linux-arm-

> [e-mail]; USB list <[e-mail]>

> Subject: Re: [PATCH v3 1/2] usb: ohci-at91: Forcibly suspend ports while USB

> suspend

> 

> On Wed, 8 Jun 2016, Wenyou Yang wrote:

> 

> > In order to the save power consumption, as a workaround, suspend

> > forcibly the USB PORTA/B/C via set the SUSPEND_A/B/C bits of OHCI

> > Interrupt Configuration Register in the SFRs while OHCI USB suspend.

> >

> > This suspend operation must be done before the USB clock is disabled,

> > resume after the USB clock is enabled.

> >

> > Signed-off-by: Wenyou Yang <[e-mail]>

> > ---

> 

> You never answered the questions I posted for the first version of this

> patch:

> 

> What does this mean?  What does suspending a port do?  Is it the same as a

> normal USB port suspend?

> 

> If it is the same, why doesn't the USB_PORT_FEAT_SUSPEND subcase of the

> SetPortFeature case in ohci_hub_control() already take care of this?


I remembered I answered your questions, http://lists.infradead.org/pipermail/linux-arm-kernel/2016-May/429245.html

Maybe not very clear.

> 

> Alan Stern



Best Regards,
Wenyou Yang

Wenyou Yang June 17, 2016, 1:44 p.m. UTC | #7

Hi Alexandre,

> -----Original Message-----

> From: Alexandre Belloni [mailto:[e-mail]]

> Sent: 2016年6月9日 4:38

> To: Rob Herring <[e-mail]>

> Cc: Yang, Wenyou <[e-mail]>; Alan Stern

> <[e-mail]>; Greg Kroah-Hartman

> <[e-mail]>; Ferre, Nicolas <[e-mail]>;

> Pawel Moll <[e-mail]>; Mark Brown <[e-mail]>; Ian

> Campbell <[e-mail]>; Kumar Gala <[e-mail]>;

> [e-mail]; [e-mail]; linux-arm-

> [e-mail]; [e-mail]

> Subject: Re: [PATCH v3 1/2] usb: ohci-at91: Forcibly suspend ports while USB

> suspend

> 

> On 08/06/2016 at 15:26:51 -0500, Rob Herring wrote :

> > On Wed, Jun 08, 2016 at 12:15:10PM +0800, Wenyou Yang wrote:

> > > In order to the save power consumption, as a workaround, suspend

> > > forcibly the USB PORTA/B/C via set the SUSPEND_A/B/C bits of OHCI

> > > Interrupt Configuration Register in the SFRs while OHCI USB suspend.

> > >

> > > This suspend operation must be done before the USB clock is

> > > disabled, resume after the USB clock is enabled.

> > >

> > > Signed-off-by: Wenyou Yang <[e-mail]>

> > > ---

> > >

> > > Changes in v3:

> > >  - Change the compatible description for more precise.

> > >

> > > Changes in v2:

> > >  - Add compatible to support forcibly suspend the ports.

> > >  - Add soc/at91/at91_sfr.h to accommodate the defines.

> > >  - Add error checking for .sfr_regmap.

> > >  - Remove unnecessary regmap_read() statement.

> > >

> > >  .../devicetree/bindings/usb/atmel-usb.txt          |  6 +-

> > >  drivers/usb/host/ohci-at91.c                       | 80 +++++++++++++++++++++-

> > >  include/soc/at91/at91_sfr.h                        | 29 ++++++++

> > >  3 files changed, 112 insertions(+), 3 deletions(-)  create mode

> > > 100644 include/soc/at91/at91_sfr.h

> > >

> > > diff --git a/Documentation/devicetree/bindings/usb/atmel-usb.txt

> > > b/Documentation/devicetree/bindings/usb/atmel-usb.txt

> > > index 5883b73..888deaa 100644

> > > --- a/Documentation/devicetree/bindings/usb/atmel-usb.txt

> > > +++ b/Documentation/devicetree/bindings/usb/atmel-usb.txt

> > > @@ -3,8 +3,10 @@ Atmel SOC USB controllers  OHCI

> > >

> > >  Required properties:

> > > - - compatible: Should be "atmel,at91rm9200-ohci" for USB controllers

> > > -   used in host mode.

> > > + - compatible: Should be one of the following

> > > +	       "atmel,at91rm9200-ohci" for USB controllers used in host mode.

> > > +	       "atmel,sama5d2-ohci" for USB controllers used in host mode

> > > +	       on SAMA5D2 which can force to suspend.

> >

> > Guess I wasn't clear enough before. Drop "which can force to suspend".

> >

> 

> Well, my point is that we don't need a new compatible anyway.


Could you give some advice?.

> 

> --

> Alexandre Belloni, Free Electrons

> Embedded Linux, Kernel and Android engineering http://free-electrons.com



Best Regards,
Wenyou Yang

Alexandre Belloni June 17, 2016, 1:54 p.m. UTC | #8

On 17/06/2016 at 13:44:22 +0000, Yang, Wenyou wrote :
> Hi Alexandre,
> 
> > -----Original Message-----
> > From: Alexandre Belloni [mailto:[e-mail]]
> > Sent: 2016年6月9日 4:38
> > To: Rob Herring <[e-mail]>
> > Cc: Yang, Wenyou <[e-mail]>; Alan Stern
> > <[e-mail]>; Greg Kroah-Hartman
> > <[e-mail]>; Ferre, Nicolas <[e-mail]>;
> > Pawel Moll <[e-mail]>; Mark Brown <[e-mail]>; Ian
> > Campbell <[e-mail]>; Kumar Gala <[e-mail]>;
> > [e-mail]; [e-mail]; linux-arm-
> > [e-mail]; [e-mail]
> > Subject: Re: [PATCH v3 1/2] usb: ohci-at91: Forcibly suspend ports while USB
> > suspend
> > 
> > On 08/06/2016 at 15:26:51 -0500, Rob Herring wrote :
> > > On Wed, Jun 08, 2016 at 12:15:10PM +0800, Wenyou Yang wrote:
> > > > In order to the save power consumption, as a workaround, suspend
> > > > forcibly the USB PORTA/B/C via set the SUSPEND_A/B/C bits of OHCI
> > > > Interrupt Configuration Register in the SFRs while OHCI USB suspend.
> > > >
> > > > This suspend operation must be done before the USB clock is
> > > > disabled, resume after the USB clock is enabled.
> > > >
> > > > Signed-off-by: Wenyou Yang <[e-mail]>
> > > > ---
> > > >
> > > > Changes in v3:
> > > >  - Change the compatible description for more precise.
> > > >
> > > > Changes in v2:
> > > >  - Add compatible to support forcibly suspend the ports.
> > > >  - Add soc/at91/at91_sfr.h to accommodate the defines.
> > > >  - Add error checking for .sfr_regmap.
> > > >  - Remove unnecessary regmap_read() statement.
> > > >
> > > >  .../devicetree/bindings/usb/atmel-usb.txt          |  6 +-
> > > >  drivers/usb/host/ohci-at91.c                       | 80 +++++++++++++++++++++-
> > > >  include/soc/at91/at91_sfr.h                        | 29 ++++++++
> > > >  3 files changed, 112 insertions(+), 3 deletions(-)  create mode
> > > > 100644 include/soc/at91/at91_sfr.h
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/usb/atmel-usb.txt
> > > > b/Documentation/devicetree/bindings/usb/atmel-usb.txt
> > > > index 5883b73..888deaa 100644
> > > > --- a/Documentation/devicetree/bindings/usb/atmel-usb.txt
> > > > +++ b/Documentation/devicetree/bindings/usb/atmel-usb.txt
> > > > @@ -3,8 +3,10 @@ Atmel SOC USB controllers  OHCI
> > > >
> > > >  Required properties:
> > > > - - compatible: Should be "atmel,at91rm9200-ohci" for USB controllers
> > > > -   used in host mode.
> > > > + - compatible: Should be one of the following
> > > > +	       "atmel,at91rm9200-ohci" for USB controllers used in host mode.
> > > > +	       "atmel,sama5d2-ohci" for USB controllers used in host mode
> > > > +	       on SAMA5D2 which can force to suspend.
> > >
> > > Guess I wasn't clear enough before. Drop "which can force to suspend".
> > >
> > 
> > Well, my point is that we don't need a new compatible anyway.
> 
> Could you give some advice?.
> 

Sure, what I mean is that you can try to get the regmap for the SFR in
every case. Depending on whether you were able to get it, you can decide
to call ohci_at91_port_suspend/resume or not (just test for
sfr_regmap != NULL).

Wenyou Yang June 20, 2016, 3:16 a.m. UTC | #9

Hi Aleandre,

> -----Original Message-----

> From: Alexandre Belloni [mailto:[e-mail]]

> Sent: 2016年6月17日 21:55

> To: Yang, Wenyou <[e-mail]>

> Cc: Rob Herring <[e-mail]>; Alan Stern <[e-mail]>;

> Greg Kroah-Hartman <[e-mail]>; Ferre, Nicolas

> <[e-mail]>; Pawel Moll <[e-mail]>; Mark Brown

> <[e-mail]>; Ian Campbell <[e-mail]>; Kumar

> Gala <[e-mail]>; [e-mail];

> [e-mail]; [e-mail]; linux-

> [e-mail]

> Subject: Re: [PATCH v3 1/2] usb: ohci-at91: Forcibly suspend ports while USB

> suspend

> 

> On 17/06/2016 at 13:44:22 +0000, Yang, Wenyou wrote :

> > Hi Alexandre,

> >

> > > -----Original Message-----

> > > From: Alexandre Belloni

> > > [mailto:[e-mail]]

> > > Sent: 2016年6月9日 4:38

> > > To: Rob Herring <[e-mail]>

> > > Cc: Yang, Wenyou <[e-mail]>; Alan Stern

> > > <[e-mail]>; Greg Kroah-Hartman

> > > <[e-mail]>; Ferre, Nicolas

> > > <[e-mail]>; Pawel Moll <[e-mail]>; Mark

> > > Brown <[e-mail]>; Ian Campbell

> > > <[e-mail]>; Kumar Gala <[e-mail]>;

> > > [e-mail]; [e-mail]; linux-arm-

> > > [e-mail]; [e-mail]

> > > Subject: Re: [PATCH v3 1/2] usb: ohci-at91: Forcibly suspend ports

> > > while USB suspend

> > >

> > > On 08/06/2016 at 15:26:51 -0500, Rob Herring wrote :

> > > > On Wed, Jun 08, 2016 at 12:15:10PM +0800, Wenyou Yang wrote:

> > > > > In order to the save power consumption, as a workaround, suspend

> > > > > forcibly the USB PORTA/B/C via set the SUSPEND_A/B/C bits of

> > > > > OHCI Interrupt Configuration Register in the SFRs while OHCI USB

> suspend.

> > > > >

> > > > > This suspend operation must be done before the USB clock is

> > > > > disabled, resume after the USB clock is enabled.

> > > > >

> > > > > Signed-off-by: Wenyou Yang <[e-mail]>

> > > > > ---

> > > > >

> > > > > Changes in v3:

> > > > >  - Change the compatible description for more precise.

> > > > >

> > > > > Changes in v2:

> > > > >  - Add compatible to support forcibly suspend the ports.

> > > > >  - Add soc/at91/at91_sfr.h to accommodate the defines.

> > > > >  - Add error checking for .sfr_regmap.

> > > > >  - Remove unnecessary regmap_read() statement.

> > > > >

> > > > >  .../devicetree/bindings/usb/atmel-usb.txt          |  6 +-

> > > > >  drivers/usb/host/ohci-at91.c                       | 80

> +++++++++++++++++++++-

> > > > >  include/soc/at91/at91_sfr.h                        | 29 ++++++++

> > > > >  3 files changed, 112 insertions(+), 3 deletions(-)  create mode

> > > > > 100644 include/soc/at91/at91_sfr.h

> > > > >

> > > > > diff --git a/Documentation/devicetree/bindings/usb/atmel-usb.txt

> > > > > b/Documentation/devicetree/bindings/usb/atmel-usb.txt

> > > > > index 5883b73..888deaa 100644

> > > > > --- a/Documentation/devicetree/bindings/usb/atmel-usb.txt

> > > > > +++ b/Documentation/devicetree/bindings/usb/atmel-usb.txt

> > > > > @@ -3,8 +3,10 @@ Atmel SOC USB controllers  OHCI

> > > > >

> > > > >  Required properties:

> > > > > - - compatible: Should be "atmel,at91rm9200-ohci" for USB controllers

> > > > > -   used in host mode.

> > > > > + - compatible: Should be one of the following

> > > > > +	       "atmel,at91rm9200-ohci" for USB controllers used in host

> mode.

> > > > > +	       "atmel,sama5d2-ohci" for USB controllers used in host mode

> > > > > +	       on SAMA5D2 which can force to suspend.

> > > >

> > > > Guess I wasn't clear enough before. Drop "which can force to suspend".

> > > >

> > >

> > > Well, my point is that we don't need a new compatible anyway.

> >

> > Could you give some advice?.

> >

> 

> Sure, what I mean is that you can try to get the regmap for the SFR in every case.

> Depending on whether you were able to get it, you can decide to call

> ohci_at91_port_suspend/resume or not (just test for sfr_regmap != NULL).


I don't think so. The SFR includes a lot of miscellaneous functions, more than this one.

> 

> --

> Alexandre Belloni, Free Electrons

> Embedded Linux, Kernel and Android engineering http://free-electrons.com



Best Regards,
Wenyou Yang

Wenyou Yang June 20, 2016, 7:42 a.m. UTC | #10

Hi Rob,

> -----Original Message-----

> From: Rob Herring [mailto:[e-mail]]

> Sent: 2016年6月9日 4:27

> To: Yang, Wenyou <[e-mail]>

> Cc: Alan Stern <[e-mail]>; Greg Kroah-Hartman

> <[e-mail]>; Ferre, Nicolas <[e-mail]>;

> Pawel Moll <[e-mail]>; Mark Brown <[e-mail]>; Ian

> Campbell <[e-mail]>; Kumar Gala <[e-mail]>;

> Alexandre Belloni <[e-mail]>; linux-

> [e-mail]; [e-mail]; linux-arm-

> [e-mail]; [e-mail]

> Subject: Re: [PATCH v3 1/2] usb: ohci-at91: Forcibly suspend ports while USB

> suspend

> 

> On Wed, Jun 08, 2016 at 12:15:10PM +0800, Wenyou Yang wrote:

> > In order to the save power consumption, as a workaround, suspend

> > forcibly the USB PORTA/B/C via set the SUSPEND_A/B/C bits of OHCI

> > Interrupt Configuration Register in the SFRs while OHCI USB suspend.

> >

> > This suspend operation must be done before the USB clock is disabled,

> > resume after the USB clock is enabled.

> >

> > Signed-off-by: Wenyou Yang <[e-mail]>

> > ---

> >

> > Changes in v3:

> >  - Change the compatible description for more precise.

> >

> > Changes in v2:

> >  - Add compatible to support forcibly suspend the ports.

> >  - Add soc/at91/at91_sfr.h to accommodate the defines.

> >  - Add error checking for .sfr_regmap.

> >  - Remove unnecessary regmap_read() statement.

> >

> >  .../devicetree/bindings/usb/atmel-usb.txt          |  6 +-

> >  drivers/usb/host/ohci-at91.c                       | 80 +++++++++++++++++++++-

> >  include/soc/at91/at91_sfr.h                        | 29 ++++++++

> >  3 files changed, 112 insertions(+), 3 deletions(-)  create mode

> > 100644 include/soc/at91/at91_sfr.h

> >

> > diff --git a/Documentation/devicetree/bindings/usb/atmel-usb.txt

> > b/Documentation/devicetree/bindings/usb/atmel-usb.txt

> > index 5883b73..888deaa 100644

> > --- a/Documentation/devicetree/bindings/usb/atmel-usb.txt

> > +++ b/Documentation/devicetree/bindings/usb/atmel-usb.txt

> > @@ -3,8 +3,10 @@ Atmel SOC USB controllers  OHCI

> >

> >  Required properties:

> > - - compatible: Should be "atmel,at91rm9200-ohci" for USB controllers

> > -   used in host mode.

> > + - compatible: Should be one of the following

> > +	       "atmel,at91rm9200-ohci" for USB controllers used in host mode.

> > +	       "atmel,sama5d2-ohci" for USB controllers used in host mode

> > +	       on SAMA5D2 which can force to suspend.

> 

> Guess I wasn't clear enough before. Drop "which can force to suspend".


In previous mail, Nicolas gave a suggestion with "...on SAMA5D2 and compatible SoCs that must explicitly force suspend through the Special Function Register (SFR)."

I think it is more clear, what is your opinion?

> 

> >   - reg: Address and length of the register set for the device

> >   - interrupts: Should contain ehci interrupt

> >   - clocks: Should reference the peripheral, host and system clocks

> > diff --git a/drivers/usb/host/ohci-at91.c

> > b/drivers/usb/host/ohci-at91.c index d177372..54e8feb 100644

> > --- a/drivers/usb/host/ohci-at91.c

> > +++ b/drivers/usb/host/ohci-at91.c

> > @@ -21,8 +21,11 @@

> >  #include <linux/io.h>

> >  #include <linux/kernel.h>

> >  #include <linux/module.h>

> > +#include <linux/mfd/syscon.h>

> > +#include <linux/regmap.h>

> >  #include <linux/usb.h>

> >  #include <linux/usb/hcd.h>

> > +#include <soc/at91/at91_sfr.h>

> >

> >  #include "ohci.h"

> >

> > @@ -45,12 +48,18 @@ struct at91_usbh_data {

> >  	u8 overcurrent_changed[AT91_MAX_USBH_PORTS];

> >  };

> >

> > +struct ohci_at91_caps {

> > +	bool suspend_ctrl;

> > +};

> > +

> >  struct ohci_at91_priv {

> >  	struct clk *iclk;

> >  	struct clk *fclk;

> >  	struct clk *hclk;

> >  	bool clocked;

> >  	bool wakeup;		/* Saved wake-up state for resume */

> > +	const struct ohci_at91_caps *caps;

> > +	struct regmap *sfr_regmap;

> >  };

> >  /* interface and function clocks; sometimes also an AHB clock */

> >

> > @@ -132,6 +141,17 @@ static void at91_stop_hc(struct platform_device

> > *pdev)

> >

> >

> > /*--------------------------------------------------------------------

> > -----*/

> >

> > +struct regmap *at91_dt_syscon_sfr(void) {

> > +	struct regmap *regmap;

> > +

> > +	regmap = syscon_regmap_lookup_by_compatible("atmel,sama5d2-sfr");

> > +	if (IS_ERR(regmap))

> > +		regmap = NULL;

> > +

> > +	return regmap;

> > +}

> > +

> >  static void usb_hcd_at91_remove (struct usb_hcd *, struct

> > platform_device *);

> >

> >  /* configure so an HC device and id are always provided */ @@ -197,6

> > +217,17 @@ static int usb_hcd_at91_probe(const struct hc_driver *driver,

> >  		goto err;

> >  	}

> >

> > +	ohci_at91->caps = (const struct ohci_at91_caps *)

> > +			  of_device_get_match_data(&pdev->dev);

> > +	if (!ohci_at91->caps)

> > +		return -ENODEV;

> > +

> > +	if (ohci_at91->caps->suspend_ctrl) {

> > +		ohci_at91->sfr_regmap = at91_dt_syscon_sfr();

> > +		if (!ohci_at91->sfr_regmap)

> > +			dev_warn(dev, "failed to find sfr node\n");

> > +	}

> > +

> >  	board = hcd->self.controller->platform_data;

> >  	ohci = hcd_to_ohci(hcd);

> >  	ohci->num_ports = board->ports;

> > @@ -440,8 +471,17 @@ static irqreturn_t ohci_hcd_at91_overcurrent_irq(int

> irq, void *data)

> >  	return IRQ_HANDLED;

> >  }

> >

> > +static const struct ohci_at91_caps at91rm9200_caps = {

> > +	.suspend_ctrl = false,

> > +};

> > +

> > +static const struct ohci_at91_caps sama5d2_caps = {

> > +	.suspend_ctrl = true,

> > +};

> > +

> >  static const struct of_device_id at91_ohci_dt_ids[] = {

> > -	{ .compatible = "atmel,at91rm9200-ohci" },

> > +	{ .compatible = "atmel,at91rm9200-ohci", .data = &at91rm9200_caps },

> > +	{ .compatible = "atmel,sama5d2-ohci", .data = &sama5d2_caps },

> >  	{ /* sentinel */ }

> >  };

> >

> > @@ -581,6 +621,38 @@ static int ohci_hcd_at91_drv_remove(struct

> platform_device *pdev)

> >  	return 0;

> >  }

> >

> > +static int ohci_at91_port_ctrl(struct regmap *regmap, bool enable) {

> > +	u32 regval;

> > +	int ret;

> > +

> > +	if (!regmap)

> > +		return -EINVAL;

> > +

> > +	ret = regmap_read(regmap, SFR_OHCIICR, &regval);

> > +	if (ret)

> > +		return ret;

> > +

> > +	if (enable)

> > +		regval &= ~SFR_OHCIICR_USB_SUSPEND;

> > +	else

> > +		regval |= SFR_OHCIICR_USB_SUSPEND;

> > +

> > +	regmap_write(regmap, SFR_OHCIICR, regval);

> > +

> > +	return 0;

> > +}

> > +

> > +static int ohci_at91_port_suspend(struct regmap *regmap) {

> > +	return ohci_at91_port_ctrl(regmap, false); }

> > +

> > +static int ohci_at91_port_resume(struct regmap *regmap) {

> > +	return ohci_at91_port_ctrl(regmap, true); }

> > +

> >  static int __maybe_unused

> >  ohci_hcd_at91_drv_suspend(struct device *dev)  { @@ -618,6 +690,9 @@

> > ohci_hcd_at91_drv_suspend(struct device *dev)

> >  		ohci_writel(ohci, ohci->hc_control, &ohci->regs->control);

> >  		ohci->rh_state = OHCI_RH_HALTED;

> >

> > +		if (ohci_at91->caps->suspend_ctrl)

> > +			ohci_at91_port_suspend(ohci_at91->sfr_regmap);

> > +

> >  		/* flush the writes */

> >  		(void) ohci_readl (ohci, &ohci->regs->control);

> >  		at91_stop_clock(ohci_at91);

> > @@ -637,6 +712,9 @@ ohci_hcd_at91_drv_resume(struct device *dev)

> >

> >  	at91_start_clock(ohci_at91);

> >

> > +	if (ohci_at91->caps->suspend_ctrl)

> > +		ohci_at91_port_resume(ohci_at91->sfr_regmap);

> > +

> >  	ohci_resume(hcd, false);

> >  	return 0;

> >  }

> > diff --git a/include/soc/at91/at91_sfr.h b/include/soc/at91/at91_sfr.h

> > new file mode 100644 index 0000000..04a3a1e

> > --- /dev/null

> > +++ b/include/soc/at91/at91_sfr.h

> > @@ -0,0 +1,29 @@

> > +/*

> > + * Header file for the Atmel DDR/SDR SDRAM Controller

> > + *

> > + * Copyright (C) 2016 Atmel Corporation

> > + *

> > + * Author: Wenyou Yang <[e-mail]>

> > + *

> > + * This program is free software; you can redistribute it and/or

> > +modify

> > + * it under the terms of the GNU General Public License version 2 as

> > + * published by the Free Software Foundation.

> > + *

> > + */

> > +#ifndef __AT91_SFR_H__

> > +#define __AT91_SFR_H__

> > +

> > +#define SFR_DDRCFG		0x04	/* DDR Configuration Register */

> > +/* 0x08 ~ 0x0c: Reserved */

> > +#define SFR_OHCIICR		0x10	/* OHCI Interrupt Configuration

> Register */

> > +#define SFR_OHCIISR		0x14	/* OHCI Interrupt Status Register

> */

> > +

> > +#define SFR_OHCIICR_SUSPEND_A	BIT(8)

> > +#define SFR_OHCIICR_SUSPEND_B	BIT(9)

> > +#define SFR_OHCIICR_SUSPEND_C	BIT(10)

> > +

> > +#define SFR_OHCIICR_USB_SUSPEND	(SFR_OHCIICR_SUSPEND_A | \

> > +				 SFR_OHCIICR_SUSPEND_B | \

> > +				 SFR_OHCIICR_SUSPEND_C)

> > +

> > +#endif

> > --

> > 2.7.4

> >



Best Regards,
Wenyou Yang

Wenyou Yang June 20, 2016, 7:49 a.m. UTC | #11

> -----Original Message-----

> From: Ferre, Nicolas

> Sent: 2016年6月8日 18:46

> To: Yang, Wenyou <[e-mail]>; Alan Stern

> <[e-mail]>; Greg Kroah-Hartman

> <[e-mail]>; Rob Herring <[e-mail]>; Alexandre

> Belloni <[e-mail]>

> Cc: Pawel Moll <[e-mail]>; Mark Brown <[e-mail]>; Ian

> Campbell <[e-mail]>; Kumar Gala <[e-mail]>;

> [e-mail]; [e-mail]; linux-arm-

> [e-mail]; [e-mail]

> Subject: Re: [PATCH v3 1/2] usb: ohci-at91: Forcibly suspend ports while USB

> suspend

> 

> Le 08/06/2016 12:04, Nicolas Ferre a écrit :

> > Le 08/06/2016 06:15, Wenyou Yang a écrit :

> >> In order to the save power consumption, as a workaround, suspend

> >> forcibly the USB PORTA/B/C via set the SUSPEND_A/B/C bits of OHCI

> >> Interrupt Configuration Register in the SFRs while OHCI USB suspend.

> >>

> >> This suspend operation must be done before the USB clock is disabled,

> >> resume after the USB clock is enabled.

> >>

> >> Signed-off-by: Wenyou Yang <[e-mail]>

> >

> > Little nitpicking below...

> >

> >> ---

> >>

> >> Changes in v3:

> >>  - Change the compatible description for more precise.

> >>

> >> Changes in v2:

> >>  - Add compatible to support forcibly suspend the ports.

> >>  - Add soc/at91/at91_sfr.h to accommodate the defines.

> >>  - Add error checking for .sfr_regmap.

> >>  - Remove unnecessary regmap_read() statement.

> >>

> >>  .../devicetree/bindings/usb/atmel-usb.txt          |  6 +-

> >>  drivers/usb/host/ohci-at91.c                       | 80 +++++++++++++++++++++-

> >>  include/soc/at91/at91_sfr.h                        | 29 ++++++++

> 

> Oops sorry, additional comment which is not nitpicking, this one:

> 

> We already have SFR header file in this patch:

> 

> Author: Cyrille Pitchen <[e-mail]>

> Date:   Thu Mar 17 17:04:00 2016 +0100

> 

>     ARM: dts: at91: sama5d2: add SFR node

> 

>     This SFR node is looked up by the I2S controller driver to tune the

>     SFR_I2SCLKSEL register.

> 

>     Signed-off-by: Cyrille Pitchen <[e-mail]>

>     Signed-off-by: Ludovic Desroches <[e-mail]>

>     Acked-by: Alexandre Belloni <[e-mail]>

>     Acked-by: Rob Herring <[e-mail]>

>     Signed-off-by: Nicolas Ferre <[e-mail]>

> 

> Which is already accepted by arm-soc guys for 4.7... So my ack transforms into a

> nack, sorry...

> 

> We will have to coordinate the effort and maybe take the whole series with us. But

> for sure, you'll have to use the existing include/soc/at91/atmel-sfr.h file and build

> on top of it...


Sorry, not notice this file.

I will built it on this file.

> 

> Bye,

> 

> >>  3 files changed, 112 insertions(+), 3 deletions(-)  create mode

> >> 100644 include/soc/at91/at91_sfr.h

> 

> [..]

> 

> >

> > But you can take my:

> >

> > Acked-by: Nicolas Ferre <[e-mail]>

> >

> > with the little corrections listed.

> >

> > Alan, We plan to take the second patch of this series with AT91 git

> > tree through arm-soc. Do you agree to take this one through yours?

> 

> Alan, forget this request, we'll have to coordinate differently.

> 

> Sorry for the noise. Bye,

> --

> Nicolas Ferre



Best Regards,
Wenyou Yang

Alexandre Belloni June 20, 2016, 8:03 a.m. UTC | #12

On 20/06/2016 at 03:16:35 +0000, Yang, Wenyou wrote :
> > Sure, what I mean is that you can try to get the regmap for the SFR in every case.
> > Depending on whether you were able to get it, you can decide to call
> > ohci_at91_port_suspend/resume or not (just test for sfr_regmap != NULL).
> 
> I don't think so. The SFR includes a lot of miscellaneous functions, more than this one.
> 

I know but this is irrelevant to this discussion. If you need to use the
SFR from another driver you will simply get it from that other driver.

I that case, you will try to get "atmel,sama5d2-sfr". It is only present
on sama5d2 so you have enough information to know whether or not you can
use it to suspend/resume.

Wenyou Yang June 20, 2016, 8:46 a.m. UTC | #13

Hi Alexandre & Nicolas,

> -----Original Message-----

> From: Alexandre Belloni [mailto:[e-mail]]

> Sent: 2016年6月20日 16:04

> To: Yang, Wenyou <[e-mail]>

> Cc: Rob Herring <[e-mail]>; Alan Stern <[e-mail]>;

> Greg Kroah-Hartman <[e-mail]>; Ferre, Nicolas

> <[e-mail]>; Pawel Moll <[e-mail]>; Mark Brown

> <[e-mail]>; Ian Campbell <[e-mail]>; Kumar

> Gala <[e-mail]>; [e-mail];

> [e-mail]; [e-mail]; linux-

> [e-mail]

> Subject: Re: [PATCH v3 1/2] usb: ohci-at91: Forcibly suspend ports while USB

> suspend

> 

> On 20/06/2016 at 03:16:35 +0000, Yang, Wenyou wrote :

> > > Sure, what I mean is that you can try to get the regmap for the SFR in every

> case.

> > > Depending on whether you were able to get it, you can decide to call

> > > ohci_at91_port_suspend/resume or not (just test for sfr_regmap != NULL).

> >

> > I don't think so. The SFR includes a lot of miscellaneous functions, more than

> this one.

> >

> 

> I know but this is irrelevant to this discussion. If you need to use the SFR from

> another driver you will simply get it from that other driver.

> 

> I that case, you will try to get "atmel,sama5d2-sfr". It is only present on sama5d2

> so you have enough information to know whether or not you can use it to

> suspend/resume.


I understand what your meaning :).
Use "atmel,sama5d2-sfr" compatible to distinguish whether forcibly suspend USB port via SFR or not.

I am not sure if it is a better solution.

Nicolas, could you give your opinion?

> 

> --

> Alexandre Belloni, Free Electrons

> Embedded Linux, Kernel and Android engineering http://free-electrons.com



Best Regards,
Wenyou Yang

Alexandre Belloni June 20, 2016, 8:52 a.m. UTC | #14

On 20/06/2016 at 08:46:02 +0000, Yang, Wenyou wrote :
> Hi Alexandre & Nicolas,
> 
> > -----Original Message-----
> > From: Alexandre Belloni [mailto:[e-mail]]
> > Sent: 2016年6月20日 16:04
> > To: Yang, Wenyou <[e-mail]>
> > Cc: Rob Herring <[e-mail]>; Alan Stern <[e-mail]>;
> > Greg Kroah-Hartman <[e-mail]>; Ferre, Nicolas
> > <[e-mail]>; Pawel Moll <[e-mail]>; Mark Brown
> > <[e-mail]>; Ian Campbell <[e-mail]>; Kumar
> > Gala <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail]; linux-
> > [e-mail]
> > Subject: Re: [PATCH v3 1/2] usb: ohci-at91: Forcibly suspend ports while USB
> > suspend
> > 
> > On 20/06/2016 at 03:16:35 +0000, Yang, Wenyou wrote :
> > > > Sure, what I mean is that you can try to get the regmap for the SFR in every
> > case.
> > > > Depending on whether you were able to get it, you can decide to call
> > > > ohci_at91_port_suspend/resume or not (just test for sfr_regmap != NULL).
> > >
> > > I don't think so. The SFR includes a lot of miscellaneous functions, more than
> > this one.
> > >
> > 
> > I know but this is irrelevant to this discussion. If you need to use the SFR from
> > another driver you will simply get it from that other driver.
> > 
> > I that case, you will try to get "atmel,sama5d2-sfr". It is only present on sama5d2
> > so you have enough information to know whether or not you can use it to
> > suspend/resume.
> 
> I understand what your meaning :).
> Use "atmel,sama5d2-sfr" compatible to distinguish whether forcibly suspend USB port via SFR or not.
> 
> I am not sure if it is a better solution.
> 

It is definitively superior. There is only one lookup in the device tree
instead of two because whatever happens, you will have to get the SFR
regmap and you don't need to add a compatible string for an IP that
didn't change.

> Nicolas, could you give your opinion?
> 
> > 
> > --
> > Alexandre Belloni, Free Electrons
> > Embedded Linux, Kernel and Android engineering http://free-electrons.com
> 
> 
> Best Regards,
> Wenyou Yang

Nicolas Ferre June 20, 2016, 9:21 a.m. UTC | #15

Le 20/06/2016 10:52, Alexandre Belloni a écrit :
> On 20/06/2016 at 08:46:02 +0000, Yang, Wenyou wrote :
>> Hi Alexandre & Nicolas,
>>
>>> -----Original Message-----
>>> From: Alexandre Belloni [mailto:[e-mail]]
>>> Sent: 2016年6月20日 16:04
>>> To: Yang, Wenyou <[e-mail]>
>>> Cc: Rob Herring <[e-mail]>; Alan Stern <[e-mail]>;
>>> Greg Kroah-Hartman <[e-mail]>; Ferre, Nicolas
>>> <[e-mail]>; Pawel Moll <[e-mail]>; Mark Brown
>>> <[e-mail]>; Ian Campbell <[e-mail]>; Kumar
>>> Gala <[e-mail]>; [e-mail];
>>> [e-mail]; [e-mail]; linux-
>>> [e-mail]
>>> Subject: Re: [PATCH v3 1/2] usb: ohci-at91: Forcibly suspend ports while USB
>>> suspend
>>>
>>> On 20/06/2016 at 03:16:35 +0000, Yang, Wenyou wrote :
>>>>> Sure, what I mean is that you can try to get the regmap for the SFR in every
>>> case.
>>>>> Depending on whether you were able to get it, you can decide to call
>>>>> ohci_at91_port_suspend/resume or not (just test for sfr_regmap != NULL).
>>>>
>>>> I don't think so. The SFR includes a lot of miscellaneous functions, more than
>>> this one.
>>>>
>>>
>>> I know but this is irrelevant to this discussion. If you need to use the SFR from
>>> another driver you will simply get it from that other driver.
>>>
>>> I that case, you will try to get "atmel,sama5d2-sfr". It is only present on sama5d2
>>> so you have enough information to know whether or not you can use it to
>>> suspend/resume.
>>
>> I understand what your meaning :).
>> Use "atmel,sama5d2-sfr" compatible to distinguish whether forcibly suspend USB port via SFR or not.
>>
>> I am not sure if it is a better solution.
>>
> 
> It is definitively superior. There is only one lookup in the device tree
> instead of two because whatever happens, you will have to get the SFR
> regmap and you don't need to add a compatible string for an IP that
> didn't change.
> 
>> Nicolas, could you give your opinion?

I'll paraphrase Alexandre but this is what I understood:

Having the information in one place and not having to managed the
synchronization with 2 potential sources of information is clearly an
advantage of Alexandre's solution.

If the next SoC has the same workaround/feature, we will anyway have a
different SFR string to cling to...
So it won't change much and we won't have the confusion of having the
same sama5d2 compatible string on the OHCI side (same behavior) and
different compatible string on the SFR side (probably a new SFR for a
new SoC...).

If the next SoC doesn't have this workaround/feature... well, it's
simple, we don't look for the SFR, we don't use the bits, and we come
back to the situation that we've always experienced ; with the same
compatibility sting for OHCI as the IP never actually changed...

In conclusion: try Alexandre's solution and we'll certainly find that
it's actually simpler.

Bonus point: it voids the discussion on the OHCI compatible string
descriptions!

Bye,

>>> Alexandre Belloni, Free Electrons
>>> Embedded Linux, Kernel and Android engineering http://free-electrons.com
>>
>>
>> Best Regards,
>> Wenyou Yang
>

Patch

diff --git a/Documentation/devicetree/bindings/usb/atmel-usb.txt b/Documentation/devicetree/bindings/usb/atmel-usb.txt
index 5883b73..888deaa 100644
--- a/Documentation/devicetree/bindings/usb/atmel-usb.txt
+++ b/Documentation/devicetree/bindings/usb/atmel-usb.txt
@@ -3,8 +3,10 @@  Atmel SOC USB controllers
 OHCI
 
 Required properties:
- - compatible: Should be "atmel,at91rm9200-ohci" for USB controllers
-   used in host mode.
+ - compatible: Should be one of the following
+	       "atmel,at91rm9200-ohci" for USB controllers used in host mode.
+	       "atmel,sama5d2-ohci" for USB controllers used in host mode
+	       on SAMA5D2 which can force to suspend.
  - reg: Address and length of the register set for the device
  - interrupts: Should contain ehci interrupt
  - clocks: Should reference the peripheral, host and system clocks
diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c
index d177372..54e8feb 100644
--- a/drivers/usb/host/ohci-at91.c
+++ b/drivers/usb/host/ohci-at91.c
@@ -21,8 +21,11 @@ 
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
 #include <linux/usb.h>
 #include <linux/usb/hcd.h>
+#include <soc/at91/at91_sfr.h>
 
 #include "ohci.h"
 
@@ -45,12 +48,18 @@  struct at91_usbh_data {
 	u8 overcurrent_changed[AT91_MAX_USBH_PORTS];
 };
 
+struct ohci_at91_caps {
+	bool suspend_ctrl;
+};
+
 struct ohci_at91_priv {
 	struct clk *iclk;
 	struct clk *fclk;
 	struct clk *hclk;
 	bool clocked;
 	bool wakeup;		/* Saved wake-up state for resume */
+	const struct ohci_at91_caps *caps;
+	struct regmap *sfr_regmap;
 };
 /* interface and function clocks; sometimes also an AHB clock */
 
@@ -132,6 +141,17 @@  static void at91_stop_hc(struct platform_device *pdev)
 
 /*-------------------------------------------------------------------------*/
 
+struct regmap *at91_dt_syscon_sfr(void)
+{
+	struct regmap *regmap;
+
+	regmap = syscon_regmap_lookup_by_compatible("atmel,sama5d2-sfr");
+	if (IS_ERR(regmap))
+		regmap = NULL;
+
+	return regmap;
+}
+
 static void usb_hcd_at91_remove (struct usb_hcd *, struct platform_device *);
 
 /* configure so an HC device and id are always provided */
@@ -197,6 +217,17 @@  static int usb_hcd_at91_probe(const struct hc_driver *driver,
 		goto err;
 	}
 
+	ohci_at91->caps = (const struct ohci_at91_caps *)
+			  of_device_get_match_data(&pdev->dev);
+	if (!ohci_at91->caps)
+		return -ENODEV;
+
+	if (ohci_at91->caps->suspend_ctrl) {
+		ohci_at91->sfr_regmap = at91_dt_syscon_sfr();
+		if (!ohci_at91->sfr_regmap)
+			dev_warn(dev, "failed to find sfr node\n");
+	}
+
 	board = hcd->self.controller->platform_data;
 	ohci = hcd_to_ohci(hcd);
 	ohci->num_ports = board->ports;
@@ -440,8 +471,17 @@  static irqreturn_t ohci_hcd_at91_overcurrent_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static const struct ohci_at91_caps at91rm9200_caps = {
+	.suspend_ctrl = false,
+};
+
+static const struct ohci_at91_caps sama5d2_caps = {
+	.suspend_ctrl = true,
+};
+
 static const struct of_device_id at91_ohci_dt_ids[] = {
-	{ .compatible = "atmel,at91rm9200-ohci" },
+	{ .compatible = "atmel,at91rm9200-ohci", .data = &at91rm9200_caps },
+	{ .compatible = "atmel,sama5d2-ohci", .data = &sama5d2_caps },
 	{ /* sentinel */ }
 };
 
@@ -581,6 +621,38 @@  static int ohci_hcd_at91_drv_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int ohci_at91_port_ctrl(struct regmap *regmap, bool enable)
+{
+	u32 regval;
+	int ret;
+
+	if (!regmap)
+		return -EINVAL;
+
+	ret = regmap_read(regmap, SFR_OHCIICR, &regval);
+	if (ret)
+		return ret;
+
+	if (enable)
+		regval &= ~SFR_OHCIICR_USB_SUSPEND;
+	else
+		regval |= SFR_OHCIICR_USB_SUSPEND;
+
+	regmap_write(regmap, SFR_OHCIICR, regval);
+
+	return 0;
+}
+
+static int ohci_at91_port_suspend(struct regmap *regmap)
+{
+	return ohci_at91_port_ctrl(regmap, false);
+}
+
+static int ohci_at91_port_resume(struct regmap *regmap)
+{
+	return ohci_at91_port_ctrl(regmap, true);
+}
+
 static int __maybe_unused
 ohci_hcd_at91_drv_suspend(struct device *dev)
 {
@@ -618,6 +690,9 @@  ohci_hcd_at91_drv_suspend(struct device *dev)
 		ohci_writel(ohci, ohci->hc_control, &ohci->regs->control);
 		ohci->rh_state = OHCI_RH_HALTED;
 
+		if (ohci_at91->caps->suspend_ctrl)
+			ohci_at91_port_suspend(ohci_at91->sfr_regmap);
+
 		/* flush the writes */
 		(void) ohci_readl (ohci, &ohci->regs->control);
 		at91_stop_clock(ohci_at91);
@@ -637,6 +712,9 @@  ohci_hcd_at91_drv_resume(struct device *dev)
 
 	at91_start_clock(ohci_at91);
 
+	if (ohci_at91->caps->suspend_ctrl)
+		ohci_at91_port_resume(ohci_at91->sfr_regmap);
+
 	ohci_resume(hcd, false);
 	return 0;
 }
diff --git a/include/soc/at91/at91_sfr.h b/include/soc/at91/at91_sfr.h
new file mode 100644
index 0000000..04a3a1e
--- /dev/null
+++ b/include/soc/at91/at91_sfr.h
@@ -0,0 +1,29 @@ 
+/*
+ * Header file for the Atmel DDR/SDR SDRAM Controller
+ *
+ * Copyright (C) 2016 Atmel Corporation
+ *
+ * Author: Wenyou Yang <[e-mail]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+#ifndef __AT91_SFR_H__
+#define __AT91_SFR_H__
+
+#define SFR_DDRCFG		0x04	/* DDR Configuration Register */
+/* 0x08 ~ 0x0c: Reserved */
+#define SFR_OHCIICR		0x10	/* OHCI Interrupt Configuration Register */
+#define SFR_OHCIISR		0x14	/* OHCI Interrupt Status Register */
+
+#define SFR_OHCIICR_SUSPEND_A	BIT(8)
+#define SFR_OHCIICR_SUSPEND_B	BIT(9)
+#define SFR_OHCIICR_SUSPEND_C	BIT(10)
+
+#define SFR_OHCIICR_USB_SUSPEND	(SFR_OHCIICR_SUSPEND_A | \
+				 SFR_OHCIICR_SUSPEND_B | \
+				 SFR_OHCIICR_SUSPEND_C)
+
+#endif