Re: [PATCH] Fix clk->enabled counter

Hi,

Dmitry Baryshkov wrote:

What's the status of this patch? Can I submit it to patch-tracker?
Didn't receive any comments.

> Hi,
> 
> eric miao wrote:
> 
>> isn't it better to extract clk_{enable, disable} from udc_disable()?
>> this can be treated as two different things, one is for the clock, the
>> other controls the udc register bit, and better be divided.
> 
> Good idea. What about this patch:
> 
> Fix the pxa2xx_udc to balance calls to clk_enable/clk_disable
> 
> Signed-off-by: Dmitry Baryshkov [email protected]
> 
> diff --git a/drivers/usb/gadget/pxa2xx_udc.c
> b/drivers/usb/gadget/pxa2xx_udc.c index 2900556..f4484e4 100644
> --- a/drivers/usb/gadget/pxa2xx_udc.c +++
> b/drivers/usb/gadget/pxa2xx_udc.c @@ -934,20 +934,36 @@ static void
> udc_disable(struct pxa2xx_udc *);
>  /* We disable the UDC -- and its 48 MHz clock -- whenever it's not
>   * in active use.
>   */
> -static int pullup(struct pxa2xx_udc *udc, int is_active) +static int
> pullup(struct pxa2xx_udc *udc)
>  {
> -	is_active = is_active && udc->vbus && udc->pullup; +	int is_active =
> udc->vbus && udc->pullup && +			! udc->suspended;
>  	DMSG("%s\n", is_active ? "active" : "inactive");
> -	if (is_active)
> -		udc_enable(udc);
> -	else {
> -		if (udc->gadget.speed != USB_SPEED_UNKNOWN) { -			DMSG("disconnect
> %s\n", udc->driver -				? udc->driver->driver.name
> -				: "(no driver)");
> -			stop_activity(udc, udc->driver);
> +	if (is_active) {
> +		if (!udc->active) {
> +			udc->active = 1;
> +#ifdef	CONFIG_ARCH_PXA
> +			/* Enable clock for USB device */ +			clk_enable(udc->clk);
> +#endif
> +			udc_enable(udc);
> +		}
> +	} else {
> +		if (udc->active) {
> +			if (udc->gadget.speed != USB_SPEED_UNKNOWN) { +				DMSG("disconnect
> %s\n", udc->driver +					? udc->driver->driver.name
> +					: "(no driver)");
> +				stop_activity(udc, udc->driver); +			}
> +			udc_disable(udc);
> +#ifdef	CONFIG_ARCH_PXA
> +			/* Disable clock for USB device */ +			clk_disable(udc->clk);
> +#endif
> +			udc->active = 0;
>  		}
> -		udc_disable(udc);
> +
>  	}
>  	return 0;
>  }
> @@ -958,9 +974,9 @@ static int pxa2xx_udc_vbus_session(struct usb_gadget
> *_gadget, int is_active)
>  	struct pxa2xx_udc	*udc;
>  
>  	udc = container_of(_gadget, struct pxa2xx_udc, gadget);
> -	udc->vbus = is_active = (is_active != 0); +	udc->vbus = (is_active !=
> 0);
>  	DMSG("vbus %s\n", is_active ? "supplied" : "inactive");
> -	pullup(udc, is_active);
> +	pullup(udc);
>  	return 0;
>  }
>  
> @@ -975,9 +991,8 @@ static int pxa2xx_udc_pullup(struct usb_gadget
> *_gadget, int is_active)
>  	if (!udc->mach->gpio_pullup && !udc->mach->udc_command)
>  		return -EOPNOTSUPP;
>  
> -	is_active = (is_active != 0);
> -	udc->pullup = is_active;
> -	pullup(udc, is_active);
> +	udc->pullup = (is_active != 0);
> +	pullup(udc);
>  	return 0;
>  }
>  
> @@ -1145,11 +1160,6 @@ static void udc_disable(struct pxa2xx_udc *dev)
>  
>  	udc_clear_mask_UDCCR(UDCCR_UDE);
>  
> -#ifdef	CONFIG_ARCH_PXA
> -        /* Disable clock for USB device */ -	clk_disable(dev->clk);
> -#endif
> -
>  	ep0_idle (dev);
>  	dev->gadget.speed = USB_SPEED_UNKNOWN;
>  }
> @@ -1190,11 +1200,6 @@ static void udc_enable (struct pxa2xx_udc *dev)
>  {
>  	udc_clear_mask_UDCCR(UDCCR_UDE);
>  
> -#ifdef	CONFIG_ARCH_PXA
> -        /* Enable clock for USB device */ -	clk_enable(dev->clk);
> -#endif
> -
>  	/* try to clear these bits before we enable the udc */
>  	udc_ack_int_UDCCR(UDCCR_SUSIR|/*UDCCR_RSTIR|*/UDCCR_RESIR);
>  
> @@ -1285,7 +1290,7 @@ fail:
>  	 * for set_configuration as well as eventual disconnect. */
>  	DMSG("registered gadget driver '%s'\n", driver->driver.name);
> -	pullup(dev, 1);
> +	pullup(dev);
>  	dump_state(dev);
>  	return 0;
>  }
> @@ -1328,7 +1333,8 @@ int usb_gadget_unregister_driver(struct
> usb_gadget_driver *driver)
>  		return -EINVAL;
>  
>  	local_irq_disable();
> -	pullup(dev, 0);
> +	dev->pullup = 0;
> +	pullup(dev);
>  	stop_activity(dev, driver);
>  	local_irq_enable();
>  
> @@ -2267,7 +2273,9 @@ static int __exit pxa2xx_udc_remove(struct
> platform_device *pdev)
>  	if (dev->driver)
>  		return -EBUSY;
>  
> -	udc_disable(dev);
> +	dev->pullup = 0;
> +	pullup(dev);
> +
>  	remove_debug_files(dev);
>  
>  	if (dev->got_irq) {
> @@ -2318,7 +2326,8 @@ static int pxa2xx_udc_suspend(struct
> platform_device *dev, pm_message_t state)
>  
>  	if (!udc->mach->gpio_pullup && !udc->mach->udc_command)
>  		WARN("USB host won't detect disconnect!\n");
> -	pullup(udc, 0);
> +	udc->suspended = 1;
> +	pullup(udc);
>  
>  	return 0;
>  }
> @@ -2327,7 +2336,8 @@ static int pxa2xx_udc_resume(struct
> platform_device *dev)
>  {
>  	struct pxa2xx_udc	*udc = platform_get_drvdata(dev);
>  
> -	pullup(udc, 1);
> +	udc->suspended = 0;
> +	pullup(udc);
>  
>  	return 0;
>  }
> diff --git a/drivers/usb/gadget/pxa2xx_udc.h
> b/drivers/usb/gadget/pxa2xx_udc.h index c08b1a2..26c0347 100644
> --- a/drivers/usb/gadget/pxa2xx_udc.h +++
> b/drivers/usb/gadget/pxa2xx_udc.h @@ -119,13 +119,17 @@ struct
> pxa2xx_udc {
>  						has_cfr : 1,
>  						req_pending : 1,
>  						req_std : 1,
> -						req_config : 1;
> +						req_config : 1,
> +						suspended : 1,
> +						active : 1;
>  
>  #define start_watchdog(dev) mod_timer(&dev->timer, jiffies + (HZ/200))
>  	struct timer_list			timer;
>  
>  	struct device				*dev;
> +#ifdef	CONFIG_ARCH_PXA
>  	struct clk				*clk;
> +#endif
>  	struct pxa2xx_udc_mach_info		*mach;
>  	u64					dma_mask;
>  	struct pxa2xx_ep			ep [PXA_UDC_NUM_ENDPOINTS];
> 
> 
>> On Jan 10, 2008 4:12 AM, Dmitry Baryshkov <[email protected]> wrote:
>>> Hi,
>>>
>>> Russell King - ARM Linux wrote:
>>>
>>> > On Wed, Jan 09, 2008 at 10:02:20PM +0300, Dmitry wrote:
>>> >> 2008/1/9, Russell King - ARM Linux <[email protected]>:
>>> >> > On Wed, Jan 09, 2008 at 07:36:47PM +0300, Dmitry Baryshkov wrote:
>>> >> > > Fix clk->enabled counter so that drivers with unpaired
>>> >> > > clock_disable calls (pxa2xx_udc :) can work.
>>> >> >
>>> >> > NAK.  It's already been discussed why this kind of approach is a
>>> >> > bad idea.
>>> >> >
>>> >> >
>>> >> Then the pxa2xx_udc should be fixed. Because currently it's broken.
>>> >
>>> > Yes.  It would be nice if someone who uses it could look into coming
>>> > up with a tested fix.
>>>
>>> Yes, it was more like a todo item for me. Please consider this patch:
>>>





-- 
With best wishes
Dmitry



-------------------------------------------------------------------
List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php