Re: [PATCH] usb: host: ohci-at91: add support to enter suspend using SMC

[Alan Stern <stern@rowland.harvard.edu>]

On Mon, Jun 06, 2022 at 04:18:02PM +0200, Clément Léger wrote:
> When Linux is running under OP-TEE, the SFR is set as secured and thus
> the AT91_OHCIICR_USB_SUSPEND register isn't accessible. Add a SMC to
> do the appropriate call to suspend the controller.
> The SMC id is fetched from the device-tree property
> "microchip,suspend-smc-id". if present, then the syscon regmap is not
> used to enter suspend and a SMC is issued.
> 
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> ---

Acked-by: Alan Stern <stern@rowland.harvard.edu>

However, this is a little weird...  You've written 
usb_hcd_at91_probe() so that the SMC is detected in preference to the 
regmap, but then you wrote ohci_at91_port_suspend() so that the regmap 
is used in preference to the SMC.  It's not wrong, but it is confusing 
to read.

Do you want to rewrite the patch to make the two routines agree on which 
mechanism to use by default?

Alan Stern

>  drivers/usb/host/ohci-at91.c | 69 ++++++++++++++++++++++++------------
>  1 file changed, 46 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c
> index a24aea3d2759..e73fda4af278 100644
> --- a/drivers/usb/host/ohci-at91.c
> +++ b/drivers/usb/host/ohci-at91.c
> @@ -13,6 +13,7 @@
>   * This file is licenced under the GPL.
>   */
>  
> +#include <linux/arm-smccc.h>
>  #include <linux/clk.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/gpio/consumer.h>
> @@ -55,6 +56,7 @@ struct ohci_at91_priv {
>  	bool clocked;
>  	bool wakeup;		/* Saved wake-up state for resume */
>  	struct regmap *sfr_regmap;
> +	u32 smc_id;
>  };
>  /* interface and function clocks; sometimes also an AHB clock */
>  
> @@ -135,6 +137,19 @@ static void at91_stop_hc(struct platform_device *pdev)
>  
>  static void usb_hcd_at91_remove (struct usb_hcd *, struct platform_device *);
>  
> +static u32 at91_dt_suspend_smc(struct device *dev)
> +{
> +	u32 smc_id;
> +
> +	if (!dev->of_node)
> +		return 0;
> +
> +	if (of_property_read_u32(dev->of_node, "microchip,suspend-smc-id", &smc_id))
> +		return 0;
> +
> +	return smc_id;
> +}
> +
>  static struct regmap *at91_dt_syscon_sfr(void)
>  {
>  	struct regmap *regmap;
> @@ -215,9 +230,13 @@ static int usb_hcd_at91_probe(const struct hc_driver *driver,
>  		goto err;
>  	}
>  
> -	ohci_at91->sfr_regmap = at91_dt_syscon_sfr();
> -	if (!ohci_at91->sfr_regmap)
> -		dev_dbg(dev, "failed to find sfr node\n");
> +	ohci_at91->smc_id = at91_dt_suspend_smc(dev);
> +	if (!ohci_at91->smc_id)  {
> +		dev_dbg(dev, "failed to find sfr suspend smc id, using regmap\n");
> +		ohci_at91->sfr_regmap = at91_dt_syscon_sfr();
> +		if (!ohci_at91->sfr_regmap)
> +			dev_dbg(dev, "failed to find sfr node\n");
> +	}
>  
>  	board = hcd->self.controller->platform_data;
>  	ohci = hcd_to_ohci(hcd);
> @@ -303,24 +322,30 @@ static int ohci_at91_hub_status_data(struct usb_hcd *hcd, char *buf)
>  	return length;
>  }
>  
> -static int ohci_at91_port_suspend(struct regmap *regmap, u8 set)
> +static int ohci_at91_port_suspend(struct ohci_at91_priv *ohci_at91, u8 set)
>  {
> +	struct regmap *regmap = ohci_at91->sfr_regmap;
>  	u32 regval;
>  	int ret;
>  
> -	if (!regmap)
> -		return 0;
> +	if (regmap) {
> +		ret = regmap_read(regmap, AT91_SFR_OHCIICR, &regval);
> +		if (ret)
> +			return ret;
>  
> -	ret = regmap_read(regmap, AT91_SFR_OHCIICR, &regval);
> -	if (ret)
> -		return ret;
> +		if (set)
> +			regval |= AT91_OHCIICR_USB_SUSPEND;
> +		else
> +			regval &= ~AT91_OHCIICR_USB_SUSPEND;
>  
> -	if (set)
> -		regval |= AT91_OHCIICR_USB_SUSPEND;
> -	else
> -		regval &= ~AT91_OHCIICR_USB_SUSPEND;
> +		regmap_write(regmap, AT91_SFR_OHCIICR, regval);
> +	} else if (ohci_at91->smc_id) {
> +		struct arm_smccc_res res;
>  
> -	regmap_write(regmap, AT91_SFR_OHCIICR, regval);
> +		arm_smccc_smc(ohci_at91->smc_id, set, 0, 0, 0, 0, 0, 0, &res);
> +		if (res.a0)
> +			return -EINVAL;
> +	}
>  
>  	return 0;
>  }
> @@ -357,9 +382,8 @@ static int ohci_at91_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
>  
>  		case USB_PORT_FEAT_SUSPEND:
>  			dev_dbg(hcd->self.controller, "SetPortFeat: SUSPEND\n");
> -			if (valid_port(wIndex) && ohci_at91->sfr_regmap) {
> -				ohci_at91_port_suspend(ohci_at91->sfr_regmap,
> -						       1);
> +			if (valid_port(wIndex)) {
> +				ohci_at91_port_suspend(ohci_at91, 1);
>  				return 0;
>  			}
>  			break;
> @@ -400,9 +424,8 @@ static int ohci_at91_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
>  
>  		case USB_PORT_FEAT_SUSPEND:
>  			dev_dbg(hcd->self.controller, "ClearPortFeature: SUSPEND\n");
> -			if (valid_port(wIndex) && ohci_at91->sfr_regmap) {
> -				ohci_at91_port_suspend(ohci_at91->sfr_regmap,
> -						       0);
> +			if (valid_port(wIndex)) {
> +				ohci_at91_port_suspend(ohci_at91, 0);
>  				return 0;
>  			}
>  			break;
> @@ -630,10 +653,10 @@ ohci_hcd_at91_drv_suspend(struct device *dev)
>  		/* flush the writes */
>  		(void) ohci_readl (ohci, &ohci->regs->control);
>  		msleep(1);
> -		ohci_at91_port_suspend(ohci_at91->sfr_regmap, 1);
> +		ohci_at91_port_suspend(ohci_at91, 1);
>  		at91_stop_clock(ohci_at91);
>  	} else {
> -		ohci_at91_port_suspend(ohci_at91->sfr_regmap, 1);
> +		ohci_at91_port_suspend(ohci_at91, 1);
>  	}
>  
>  	return ret;
> @@ -645,7 +668,7 @@ ohci_hcd_at91_drv_resume(struct device *dev)
>  	struct usb_hcd	*hcd = dev_get_drvdata(dev);
>  	struct ohci_at91_priv *ohci_at91 = hcd_to_ohci_at91_priv(hcd);
>  
> -	ohci_at91_port_suspend(ohci_at91->sfr_regmap, 0);
> +	ohci_at91_port_suspend(ohci_at91, 0);
>  
>  	if (ohci_at91->wakeup)
>  		disable_irq_wake(hcd->irq);
> -- 
> 2.36.1
>