Re: [PATCH] omap: usbhs: Fixed the crash during rmmod of ehci and ohci

["Munegowda, Keshava" <keshava_mgowda@xxxxxx>]

On Mon, May 9, 2011 at 3:06 PM, Felipe Balbi <balbi@xxxxxx> wrote:
> Hi,
>
> On Mon, May 09, 2011 at 02:40:03PM +0530, Keshava Munegowda wrote:
>> From: Keshava Munegowda <Keshava_mgowda@xxxxxx>
>>
>> the disabling of clocks and freeing GPIO are changed
>> to fix the occurence of the crash of rmmod of ehci and ohci
>> drivers
>>
>> Signed-off-by: Keshava Munegowda <keshava_mgowda@xxxxxx>
>
> NAK
>
>> ---
>>  drivers/mfd/omap-usb-host.c |   24 ++++++++++++++++--------
>>  1 files changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c
>> index 3ab9ffa..cda0797 100644
>> --- a/drivers/mfd/omap-usb-host.c
>> +++ b/drivers/mfd/omap-usb-host.c
>> @@ -939,6 +939,7 @@ static void usbhs_disable(struct device *dev)
>>       struct usbhs_hcd_omap           *omap = dev_get_drvdata(dev);
>>       struct usbhs_omap_platform_data *pdata = &omap->platdata;
>>       unsigned long                   flags = 0;
>> +     unsigned int                    halt = 0;
>
> you shouldn't need this.
>
>> @@ -994,24 +995,31 @@ static void usbhs_disable(struct device *dev)
>>                       dev_dbg(dev, "operation timed out\n");
>>       }
>>
>> -     if (pdata->ehci_data->phy_reset) {
>> -             if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[0]))
>> -                     gpio_free(pdata->ehci_data->reset_gpio_port[0]);
>> -
>> -             if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[1]))
>> -                     gpio_free(pdata->ehci_data->reset_gpio_port[1]);
>> +     if (is_omap_usbhs_rev2(omap)) {
>> +             if (is_ehci_tll_mode(pdata->port_mode[0]))
>> +                     clk_enable(omap->usbtll_p1_fck);
>> +             if (is_ehci_tll_mode(pdata->port_mode[1]))
>> +                     clk_enable(omap->usbtll_p2_fck);
>> +             clk_disable(omap->utmi_p2_fck);
>> +             clk_disable(omap->utmi_p1_fck);
>>       }
>>
>> -     clk_disable(omap->utmi_p2_fck);
>> -     clk_disable(omap->utmi_p1_fck);
>>       clk_disable(omap->usbtll_ick);
>>       clk_disable(omap->usbtll_fck);
>>       clk_disable(omap->usbhost_fs_fck);
>>       clk_disable(omap->usbhost_hs_fck);
>>       clk_disable(omap->usbhost_ick);
>> +     halt = 1;
>
> at least from this diff, you're always reaching that part of the code
> rendering this halt flag useless.

I you see only the patch ; its looks like variable halt is not needed;

If the code; it will be set only when the clocks are disabled;
Then after restoring irq, you will free the gpio based on this value.

The complete usbhs_disable code is follows:
static void usbhs_disable(struct device *dev)
{
	struct usbhs_hcd_omap		*omap = dev_get_drvdata(dev);
	struct usbhs_omap_platform_data	*pdata = &omap->platdata;
	unsigned long			flags = 0;
	unsigned int			halt = 0;
	unsigned long			timeout;

	dev_dbg(dev, "stopping TI HSUSB Controller\n");

	spin_lock_irqsave(&omap->lock, flags);

	if (omap->count == 0)
		goto end_disble;

	omap->count--;

	if (omap->count != 0)
		goto end_disble;

	/* Reset OMAP modules for insmod/rmmod to work */
	usbhs_write(omap->uhh_base, OMAP_UHH_SYSCONFIG,
			is_omap_usbhs_rev2(omap) ?
			OMAP4_UHH_SYSCONFIG_SOFTRESET :
			OMAP_UHH_SYSCONFIG_SOFTRESET);

	timeout = jiffies + msecs_to_jiffies(100);
	while (!(usbhs_read(omap->uhh_base, OMAP_UHH_SYSSTATUS)
				& (1 << 0))) {
		cpu_relax();

		if (time_after(jiffies, timeout))
			dev_dbg(dev, "operation timed out\n");
	}

	while (!(usbhs_read(omap->uhh_base, OMAP_UHH_SYSSTATUS)
				& (1 << 1))) {
		cpu_relax();

		if (time_after(jiffies, timeout))
			dev_dbg(dev, "operation timed out\n");
	}

	while (!(usbhs_read(omap->uhh_base, OMAP_UHH_SYSSTATUS)
				& (1 << 2))) {
		cpu_relax();

		if (time_after(jiffies, timeout))
			dev_dbg(dev, "operation timed out\n");
	}

	usbhs_write(omap->tll_base, OMAP_USBTLL_SYSCONFIG, (1 << 1));

	while (!(usbhs_read(omap->tll_base, OMAP_USBTLL_SYSSTATUS)
				& (1 << 0))) {
		cpu_relax();

		if (time_after(jiffies, timeout))
			dev_dbg(dev, "operation timed out\n");
	}

	if (is_omap_usbhs_rev2(omap)) {
		if (is_ehci_tll_mode(pdata->port_mode[0]))
			clk_enable(omap->usbtll_p1_fck);
		if (is_ehci_tll_mode(pdata->port_mode[1]))
			clk_enable(omap->usbtll_p2_fck);
		clk_disable(omap->utmi_p2_fck);
		clk_disable(omap->utmi_p1_fck);
	}

	clk_disable(omap->usbtll_ick);
	clk_disable(omap->usbtll_fck);
	clk_disable(omap->usbhost_fs_fck);
	clk_disable(omap->usbhost_hs_fck);
	clk_disable(omap->usbhost_ick);
	halt = 1;

end_disble:
	spin_unlock_irqrestore(&omap->lock, flags);
	if (halt && pdata->ehci_data->phy_reset) {
		if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[0]))
			gpio_free(pdata->ehci_data->reset_gpio_port[0]);

		if (gpio_is_valid(pdata->ehci_data->reset_gpio_port[1]))
			gpio_free(pdata->ehci_data->reset_gpio_port[1]);
	}
}




>
> --
> balbi
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html