On 10/31/2014 12:42 PM, Felipe Balbi wrote:
> Hi,
>
> On Fri, Oct 31, 2014 at 10:20:06AM -0500, Dinh Nguyen wrote:
>>>> @@ -339,7 +339,8 @@ static void dwc2_handle_wakeup_detected_intr(struct
>>>> dwc2_hsotg *hsotg)
>>>> }
>>>> /* Change to L0 state */
>>>> hsotg->lx_state = DWC2_L0;
>>>> - call_gadget(hsotg, resume);
>>>> + if (!IS_ERR(hsotg->clk))
>>>> + call_gadget(hsotg, resume);
>>>
>>> instead of exposing the clock detail to the entire driver, add IS_ERR()
>>> checks to resume and suspend instead. In fact, NULL is a valid clock, so
>>> you might as well:
>>>
>>> clk = clk_get(foo, bar);
>>> if (IS_ERR(clk))
>>> dwc->clk = NULL;
>>> else
>>> dwc->clk = clk;
>>>
>>> Then you don't need any IS_ERR() checks sprinkled around the driver.
>>
>> But we would still need to check for the clock before accessing gadget
>> functionality right?
>>
>> if (dwc2->clk)
>> call_gadget();
>
> Read my comment again. "NULL is a valid clock". Look at what
> clk_enable() does when a NULL pointer is passed:
>
> static int __clk_enable(struct clk *clk)
> {
> int ret = 0;
>
> if (!clk)
> return 0;
>
> if (WARN_ON(clk->prepare_count == 0))
> return -ESHUTDOWN;
>
> if (clk->enable_count == 0) {
> ret = __clk_enable(clk->parent);
>
> if (ret)
> return ret;
>
> if (clk->ops->enable) {
> ret = clk->ops->enable(clk->hw);
> if (ret) {
> __clk_disable(clk->parent);
> return ret;
> }
> }
> }
>
> clk->enable_count++;
> return 0;
> }
>
> int clk_enable(struct clk *clk)
> {
> unsigned long flags;
> int ret;
>
> flags = clk_enable_lock();
> ret = __clk_enable(clk);
> clk_enable_unlock(flags);
>
> return ret;
> }
> EXPORT_SYMBOL_GPL(clk_enable);
Ah yes, thanks for the explanation. So if clk=NULL, it just return 0.
But what I'm saying is that if the driver is configured for dual-role
mode, and no clock is specified, then the driver should not be accessing
any gadget functionality.
So as the patch series stands right now, if I swap out an A connector to
a B-connector, then I get a connect_id_status change interrupt. The
status would show a device and I would initialize the gadget portion of
the driver.
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 44c609f..96810f7 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -1371,7 +1371,8 @@ static void dwc2_conn_id_status_change(struct
work_struct *work)
hsotg->op_state = OTG_STATE_B_PERIPHERAL;
dwc2_core_init(hsotg, false, -1);
dwc2_enable_global_interrupts(hsotg);
- s3c_hsotg_core_init(hsotg);
+ if (hsotg->clk)
+ s3c_hsotg_core_init(hsotg);
So if I don't have a valid clock, I'll be accessing the peripheral
portion of the IP.
But I guess not having the check for the valid clock here should be fine
as I don't see a case where there can be 2 different clocks for host and
peripheral?
>
>>>> @@ -400,7 +401,8 @@ static void dwc2_handle_usb_suspend_intr(struct
>>>> dwc2_hsotg *hsotg)
>>>> "DSTS.Suspend Status=%d HWCFG4.Power Optimize=%d\n",
>>>> !!(dsts & DSTS_SUSPSTS),
>>>> hsotg->hw_params.power_optimized);
>>>> - call_gadget(hsotg, suspend);
>>>> + if (!IS_ERR(hsotg->clk))
>>>> + call_gadget(hsotg, suspend);
>>>> } else {
>>>> if (hsotg->op_state == OTG_STATE_A_PERIPHERAL) {
>>>> dev_dbg(hsotg->dev, "a_peripheral->a_host\n");
>>>> @@ -477,7 +479,8 @@ irqreturn_t dwc2_handle_common_intr(int irq, void *dev)
>>>> spin_lock(&hsotg->lock);
>>>>
>>>> if (dwc2_is_device_mode(hsotg))
>>>> - retval = s3c_hsotg_irq(irq, dev);
>>>> + if (!IS_ERR(hsotg->clk))
>>>> + retval = s3c_hsotg_irq(irq, dev);
>>>
>>> wait a minute, if there is no clock we don't call the gadget interrupt
>>> handler ? Why ? Who will disable the IRQ line ?
>>
>> This portion is no static int __clk_enable(struct clk *clk)
>
> huh ? What I mean is that this has the potential of leaving that IRQ
> line enabled. Imagine you don't have a clock and s3c_hsotg_irq() isn't
> called, then a peripheral IRQ fires, since the handler isn't called, who
> will clear the interrupt ?
>
Yes, right. This portion of the code is no longer needed when I switched
to use a shared IRQ.
Dinh
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
