Date:	Thu, 1 Aug 2013 10:05:44 +0200
From:	Matthieu CASTET <matthieu.castet@...rot.com>
To:	Tuomas Tynkkynen <ttynkkynen@...dia.com>
CC:	"balbi@...com" <balbi@...com>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
	"swarren@...dotorg.org" <swarren@...dotorg.org>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"stern@...land.harvard.edu" <stern@...land.harvard.edu>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>
Subject: Re: [PATCH 1/6] usb: host: add has_tdi_phy_lpm capability bit

Hi,

Le Wed, 31 Jul 2013 18:41:57 +0100,
Tuomas Tynkkynen <ttynkkynen@...dia.com> a écrit :

> The has_hostpc capability bit indicates that the host controller has
> the HOSTPC register extensions, but at the same time enables clock
> disabling power saving features with the PHY Low Power Clock Disable
> (PHCD) bit.
> 
> However, some host controllers have the HOSTPC extensions but don't
> support the low-power feature, so the PHCD bit must not be set on
> those controllers. Add a separate capability bit for the low-power
> feature instead, and change all existing users of has_hostpc to use
> this new capability bit.
> 
> The idea for this commit is taken from an old 2012 commit that never
> got merged ("disociate chipidea PHY low power suspend control from
> hostpc")
Note that because of the different register layout (see "add phy low
power suspend for older chipidea core" commit in the same series), we
should not set has_tdi_phy_lpm if has_hostpc == 0 with the current code.

May be you should have change the ehci->has_hostpc to (ehci->has_hostpc
&& ehci->has_tdi_phy_lpm).

BTW Alan make some comment on the commit :
http://marc.info/?l=linux-usb&m=133701342028213&w=2

They may apply to your commit.

> 
> Inspired-by: Matthieu CASTET <matthieu.castet@...rot.com>
> Signed-off-by: Tuomas Tynkkynen <ttynkkynen@...dia.com>
> ---
>  drivers/usb/chipidea/host.c |  1 +
>  drivers/usb/host/ehci-hub.c | 14 +++++++-------
>  drivers/usb/host/ehci.h     |  1 +
>  3 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> index 40d0fda..9b3aaf1 100644
> --- a/drivers/usb/chipidea/host.c
> +++ b/drivers/usb/chipidea/host.c
> @@ -63,6 +63,7 @@ static int host_start(struct ci_hdrc *ci)
>  	ehci = hcd_to_ehci(hcd);
>  	ehci->caps = ci->hw_bank.cap;
>  	ehci->has_hostpc = ci->hw_bank.lpm;
> +	ehci->has_tdi_phy_lpm = ci->hw_bank.lpm;
>  
>  	ret = usb_add_hcd(hcd, 0, 0);
>  	if (ret)
> diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
> index 2b70277..8044a74 100644
> --- a/drivers/usb/host/ehci-hub.c
> +++ b/drivers/usb/host/ehci-hub.c
> @@ -183,7 +183,7 @@ static void ehci_adjust_port_wakeup_flags(struct
> ehci_hcd *ehci, spin_lock_irq(&ehci->lock);
>  
>  	/* clear phy low-power mode before changing wakeup flags */
> -	if (ehci->has_hostpc) {
> +	if (ehci->has_tdi_phy_lpm) {
>  		port = HCS_N_PORTS(ehci->hcs_params);
>  		while (port--) {
>  			u32 __iomem	*hostpc_reg =
> &ehci->regs->hostpc[port]; @@ -217,7 +217,7 @@ static void
> ehci_adjust_port_wakeup_flags(struct ehci_hcd *ehci, }
>  
>  	/* enter phy low-power mode again */
> -	if (ehci->has_hostpc) {
> +	if (ehci->has_tdi_phy_lpm) {
>  		port = HCS_N_PORTS(ehci->hcs_params);
>  		while (port--) {
>  			u32 __iomem	*hostpc_reg =
> &ehci->regs->hostpc[port]; @@ -309,7 +309,7 @@ static int
> ehci_bus_suspend (struct usb_hcd *hcd) }
>  	}
>  
> -	if (changed && ehci->has_hostpc) {
> +	if (changed && ehci->has_tdi_phy_lpm) {
>  		spin_unlock_irq(&ehci->lock);
>  		msleep(5);	/* 5 ms for HCD to enter low-power
> mode */ spin_lock_irq(&ehci->lock);
> @@ -435,7 +435,7 @@ static int ehci_bus_resume (struct usb_hcd *hcd)
>  		goto shutdown;
>  
>  	/* clear phy low-power mode before resume */
> -	if (ehci->bus_suspended && ehci->has_hostpc) {
> +	if (ehci->bus_suspended && ehci->has_tdi_phy_lpm) {
>  		i = HCS_N_PORTS(ehci->hcs_params);
>  		while (i--) {
>  			if (test_bit(i, &ehci->bus_suspended)) {
> @@ -788,7 +788,7 @@ static int ehci_hub_control (
>  				goto error;
>  
>  			/* clear phy low-power mode before resume */
> -			if (ehci->has_hostpc) {
> +			if (ehci->has_tdi_phy_lpm) {
>  				temp1 = ehci_readl(ehci, hostpc_reg);
>  				ehci_writel(ehci, temp1 &
> ~HOSTPC_PHCD, hostpc_reg);
> @@ -1031,12 +1031,12 @@ static int ehci_hub_control (
>  
>  			/* After above check the port must be
> connected.
>  			 * Set appropriate bit thus could put phy
> into low power
> -			 * mode if we have hostpc feature
> +			 * mode if we have tdi_phy_lpm feature
>  			 */
>  			temp &= ~PORT_WKCONN_E;
>  			temp |= PORT_WKDISC_E | PORT_WKOC_E;
>  			ehci_writel(ehci, temp | PORT_SUSPEND,
> status_reg);
> -			if (ehci->has_hostpc) {
> +			if (ehci->has_tdi_phy_lpm) {
>  				spin_unlock_irqrestore(&ehci->lock,
> flags); msleep(5);/* 5ms for HCD enter low pwr mode */
>  				spin_lock_irqsave(&ehci->lock,
> flags); diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
> index 64f9a08..d034d94 100644
> --- a/drivers/usb/host/ehci.h
> +++ b/drivers/usb/host/ehci.h
> @@ -210,6 +210,7 @@ struct ehci_hcd {			/* one
> per controller */ #define OHCI_HCCTRL_LEN         0x4
>  	__hc32			*ohci_hcctrl_reg;
>  	unsigned		has_hostpc:1;
> +	unsigned		has_tdi_phy_lpm:1;
>  	unsigned		has_ppcd:1; /* support per-port
> change bits */ u8			sbrn;		/*
> packed release number */ 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists