Re: [PATCHv6 6/8] usb: dwc2: gadget: Do not fail probe if there isn't a clock node

2014-11-03 Thread Felipe Balbi 
Hi,

On Fri, Oct 31, 2014 at 02:31:31PM -0500, Dinh Nguyen wrote:
 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.

why ? Why only for gadget and why can't it work on platforms without
clk? What if Paul wants to run the gadget side on his HAPS-5x platform
configured as a PCIe card ?

You haven't explained why gadget has this hard-dependency on clk and why
*only* gadget has it. This sounds really, really wrong. Why can host
side run without clk but gadget can't ?

Moreover, if you really want to prevent people from using gadget
without clock, fail dwc2_gadget_init() and have the core fallback to
host-only, then print a warning message.

 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.

that's fine, but why the hard-dependency on clk ?

 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.

so what ?

 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?

probably the same thing.

  @@ -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

Re: [PATCHv6 6/8] usb: dwc2: gadget: Do not fail probe if there isn't a clock node

2014-10-31 Thread Felipe Balbi 
On Fri, Oct 31, 2014 at 10:38:28AM +0800, Kever Yang wrote:
 Hi Dinh,
 
 On 10/29/2014 07:25 AM, dingu...@opensource.altera.com wrote:
 From: Dinh Nguyen dingu...@opensource.altera.com
 
 Since the dwc2 hcd driver is currently not looking for a clock node during
 init, we should not completely fail if there isn't a clock provided.
 For dual-role mode, we will only fail init for a non-clock node error. We
 then update the HCD to only call gadget funtions if there is a proper clock
 node.
 We have to add clock management for hcd, and I think it is better to
 do it before more Socs use this driver, isn't it?
 I have do something in my RFC patches, but I think I still do it in a wrong
 way.
 Can we just handle all the clock thing in platform?
 
 Balbi suggested in my patch that we can hide clk_enable()/disable() under
 -runtime_resume()/-runtime_suspend() and linux driver model.
 Can this be in platform driver?

it can and it probably should. Implement
-runtime_resume()/-runtime_suspend()/runtime_idle() in platform.c and
call pm_runtime_enable()/get()/put()/mark_last_busy()/autosuspend()
properly.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCHv6 6/8] usb: dwc2: gadget: Do not fail probe if there isn't a clock node

2014-10-31 Thread Dinh Nguyen 
On 10/30/2014 09:04 AM, Felipe Balbi wrote:
 Hi,
 
 On Tue, Oct 28, 2014 at 06:25:47PM -0500, dingu...@opensource.altera.com 
 wrote:
 From: Dinh Nguyen dingu...@opensource.altera.com

 Since the dwc2 hcd driver is currently not looking for a clock node during
 init, we should not completely fail if there isn't a clock provided.
 For dual-role mode, we will only fail init for a non-clock node error. We
 then update the HCD to only call gadget funtions if there is a proper clock
 node.

 Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com
 ---
 v5: reworked to not access gadget functions from the hcd.
 ---
  drivers/usb/dwc2/core.h  |  3 +--
  drivers/usb/dwc2/core_intr.c |  9 ++---
  drivers/usb/dwc2/hcd.c   |  3 ++-
  drivers/usb/dwc2/platform.c  | 19 +++
  4 files changed, 24 insertions(+), 10 deletions(-)

 diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
 index ec70862..48120c8 100644
 --- a/drivers/usb/dwc2/core.h
 +++ b/drivers/usb/dwc2/core.h
 @@ -660,6 +660,7 @@ struct dwc2_hsotg {
  #endif
  #endif /* CONFIG_USB_DWC2_HOST || CONFIG_USB_DWC2_DUAL_ROLE */
  
 +struct clk *clk;
  #if IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL) || 
 IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)
  /* Gadget structures */
  struct usb_gadget_driver *driver;
 @@ -667,8 +668,6 @@ struct dwc2_hsotg {
  struct usb_phy *uphy;
  struct s3c_hsotg_plat *plat;
  
 -struct clk *clk;
 -
  struct regulator_bulk_data supplies[ARRAY_SIZE(s3c_hsotg_supply_names)];
  
  u32 phyif;
 diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
 index 1240875..1608037 100644
 --- a/drivers/usb/dwc2/core_intr.c
 +++ b/drivers/usb/dwc2/core_intr.c
 @@ -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();

 
 @@ -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 longer needed when I use shared IRQ lines.

 
 diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
 index 44c609f..fa49c72 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 (!IS_ERR(hsotg-clk))
 +s3c_hsotg_core_init(hsotg);
  } else {
  /* A-Device connector (Host Mode) */
  dev_dbg(hsotg-dev, connId A\n);
 diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
 index 72f32f7..77c8417 100644
 --- a/drivers/usb/dwc2/platform.c
 +++ b/drivers/usb/dwc2/platform.c
 @@ -217,8 +217,17 @@ static int dwc2_driver_probe(struct platform_device 
 *dev)
  
  spin_lock_init(hsotg-lock);
  retval = dwc2_gadget_init(hsotg, irq);
 -if (retval)
 -return retval;
 +if (retval) {
 +/*
 + * We will not fail the driver initialization for dual-role
 + * if no clock node is supplied. However, all gadget
 + * functionality will be disabled if a clock node is not
 + * provided. Host functionality will continue.
 + * TO-DO: make clock node a requirement for the HCD.
 + */

Re: [PATCHv6 6/8] usb: dwc2: gadget: Do not fail probe if there isn't a clock node

2014-10-31 Thread Felipe Balbi 
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);

  @@ -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 ?

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCHv6 6/8] usb: dwc2: gadget: Do not fail probe if there isn't a clock node

2014-10-31 Thread Dinh Nguyen 
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

Re: [PATCHv6 6/8] usb: dwc2: gadget: Do not fail probe if there isn't a clock node

2014-10-31 Thread Dinh Nguyen 
On 10/31/2014 02:31 PM, Dinh Nguyen wrote:
 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?
 

Ah...nevermind. I don't need to check for clocks at all because in
dwc2_gadget_init(), the clock node check comes before
usb_add_gadget_udc(). Thus without a clock node, gadget functionality is
disabled already.

Thanks,
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

Re: [PATCHv6 6/8] usb: dwc2: gadget: Do not fail probe if there isn't a clock node

2014-10-30 Thread Felipe Balbi 
Hi,

On Tue, Oct 28, 2014 at 06:25:47PM -0500, dingu...@opensource.altera.com wrote:
 From: Dinh Nguyen dingu...@opensource.altera.com
 
 Since the dwc2 hcd driver is currently not looking for a clock node during
 init, we should not completely fail if there isn't a clock provided.
 For dual-role mode, we will only fail init for a non-clock node error. We
 then update the HCD to only call gadget funtions if there is a proper clock
 node.
 
 Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com
 ---
 v5: reworked to not access gadget functions from the hcd.
 ---
  drivers/usb/dwc2/core.h  |  3 +--
  drivers/usb/dwc2/core_intr.c |  9 ++---
  drivers/usb/dwc2/hcd.c   |  3 ++-
  drivers/usb/dwc2/platform.c  | 19 +++
  4 files changed, 24 insertions(+), 10 deletions(-)
 
 diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
 index ec70862..48120c8 100644
 --- a/drivers/usb/dwc2/core.h
 +++ b/drivers/usb/dwc2/core.h
 @@ -660,6 +660,7 @@ struct dwc2_hsotg {
  #endif
  #endif /* CONFIG_USB_DWC2_HOST || CONFIG_USB_DWC2_DUAL_ROLE */
  
 + struct clk *clk;
  #if IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL) || 
 IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)
   /* Gadget structures */
   struct usb_gadget_driver *driver;
 @@ -667,8 +668,6 @@ struct dwc2_hsotg {
   struct usb_phy *uphy;
   struct s3c_hsotg_plat *plat;
  
 - struct clk *clk;
 -
   struct regulator_bulk_data supplies[ARRAY_SIZE(s3c_hsotg_supply_names)];
  
   u32 phyif;
 diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
 index 1240875..1608037 100644
 --- a/drivers/usb/dwc2/core_intr.c
 +++ b/drivers/usb/dwc2/core_intr.c
 @@ -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.

 @@ -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 ?

 diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
 index 44c609f..fa49c72 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 (!IS_ERR(hsotg-clk))
 + s3c_hsotg_core_init(hsotg);
   } else {
   /* A-Device connector (Host Mode) */
   dev_dbg(hsotg-dev, connId A\n);
 diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
 index 72f32f7..77c8417 100644
 --- a/drivers/usb/dwc2/platform.c
 +++ b/drivers/usb/dwc2/platform.c
 @@ -217,8 +217,17 @@ static int dwc2_driver_probe(struct platform_device *dev)
  
   spin_lock_init(hsotg-lock);
   retval = dwc2_gadget_init(hsotg, irq);
 - if (retval)
 - return retval;
 + if (retval) {
 + /*
 +  * We will not fail the driver initialization for dual-role
 +  * if no clock node is supplied. However, all gadget
 +  * functionality will be disabled if a clock node is not
 +  * provided. Host functionality will continue.
 +  * TO-DO: make clock node a requirement for the HCD.
 +  */
 + if (!IS_ERR(hsotg-clk))
 + return retval;
 + }

no here... this should have been taken care by dwc2_gadget_init()
itself.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCHv6 6/8] usb: dwc2: gadget: Do not fail probe if there isn't a clock node

2014-10-30 Thread Kever Yang 

Hi Dinh,

On 10/29/2014 07:25 AM, dingu...@opensource.altera.com wrote:

From: Dinh Nguyen dingu...@opensource.altera.com

Since the dwc2 hcd driver is currently not looking for a clock node during
init, we should not completely fail if there isn't a clock provided.
For dual-role mode, we will only fail init for a non-clock node error. We
then update the HCD to only call gadget funtions if there is a proper clock
node.

We have to add clock management for hcd, and I think it is better to
do it before more Socs use this driver, isn't it?
I have do something in my RFC patches, but I think I still do it in a 
wrong way.

Can we just handle all the clock thing in platform?

Balbi suggested in my patch that we can hide clk_enable()/disable() under
-runtime_resume()/-runtime_suspend() and linux driver model.
Can this be in platform driver?

- Kever
--
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

RE: [PATCHv6 6/8] usb: dwc2: gadget: Do not fail probe if there isn't a clock node

2014-10-28 Thread Paul Zimmerman 
 From: dingu...@opensource.altera.com [mailto:dingu...@opensource.altera.com]
 Sent: Tuesday, October 28, 2014 4:26 PM
 
 Since the dwc2 hcd driver is currently not looking for a clock node during
 init, we should not completely fail if there isn't a clock provided.
 For dual-role mode, we will only fail init for a non-clock node error. We
 then update the HCD to only call gadget funtions if there is a proper clock
 node.
 
 Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com
 ---
 v5: reworked to not access gadget functions from the hcd.
 ---
  drivers/usb/dwc2/core.h  |  3 +--
  drivers/usb/dwc2/core_intr.c |  9 ++---
  drivers/usb/dwc2/hcd.c   |  3 ++-
  drivers/usb/dwc2/platform.c  | 19 +++
  4 files changed, 24 insertions(+), 10 deletions(-)
 
 diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
 index ec70862..48120c8 100644
 --- a/drivers/usb/dwc2/core.h
 +++ b/drivers/usb/dwc2/core.h
 @@ -660,6 +660,7 @@ struct dwc2_hsotg {
  #endif
  #endif /* CONFIG_USB_DWC2_HOST || CONFIG_USB_DWC2_DUAL_ROLE */
 
 + struct clk *clk;
  #if IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL) || 
 IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)
   /* Gadget structures */
   struct usb_gadget_driver *driver;
 @@ -667,8 +668,6 @@ struct dwc2_hsotg {
   struct usb_phy *uphy;
   struct s3c_hsotg_plat *plat;
 
 - struct clk *clk;
 -
   struct regulator_bulk_data supplies[ARRAY_SIZE(s3c_hsotg_supply_names)];
 
   u32 phyif;
 diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
 index 1240875..1608037 100644
 --- a/drivers/usb/dwc2/core_intr.c
 +++ b/drivers/usb/dwc2/core_intr.c
 @@ -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);
   } else {
   if (hsotg-lx_state != DWC2_L1) {
   u32 pcgcctl = readl(hsotg-regs + PCGCTL);
 @@ -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);
 
   gintsts = dwc2_read_common_intr(hsotg);
   if (gintsts  ~GINTSTS_PRTINT)
 diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
 index 44c609f..fa49c72 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 (!IS_ERR(hsotg-clk))
 + s3c_hsotg_core_init(hsotg);
   } else {
   /* A-Device connector (Host Mode) */
   dev_dbg(hsotg-dev, connId A\n);
 diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
 index 72f32f7..77c8417 100644
 --- a/drivers/usb/dwc2/platform.c
 +++ b/drivers/usb/dwc2/platform.c
 @@ -217,8 +217,17 @@ static int dwc2_driver_probe(struct platform_device *dev)
 
   spin_lock_init(hsotg-lock);
   retval = dwc2_gadget_init(hsotg, irq);
 - if (retval)
 - return retval;
 + if (retval) {
 + /*
 +  * We will not fail the driver initialization for dual-role
 +  * if no clock node is supplied. However, all gadget
 +  * functionality will be disabled if a clock node is not
 +  * provided. Host functionality will continue.
 +  * TO-DO: make clock node a requirement for the HCD.
 +  */
 + if (!IS_ERR(hsotg-clk))
 + return retval;
 + }
   retval = dwc2_hcd_init(hsotg, irq, params);
   if (retval)
   return retval;
 @@ -234,7 +243,8 @@ static int dwc2_suspend(struct device *dev)
   int ret = 0;
 
   if (dwc2_is_device_mode(dwc2))
 - ret = s3c_hsotg_suspend(dwc2);
 + if (!IS_ERR(dwc2-clk))
 + ret = s3c_hsotg_suspend(dwc2);
   return ret;
  }
 
 @@ -244,7 +254,8 @@ static int