Re: [PATCH 1/1] ARM: dma: fix dma_max_pfn()

[Santosh Shilimkar]

On 8/19/2016 12:30 AM, Roger Quadros wrote:

Hi Santosh,




So I'm 99.9% convinced that the proposed change is correct.


I will got with that then :-) and take my objection back. Just
saying that if there other breakages which I can't recollect now,
those drivers needs to be patched as well.


I was able to boot the Keystone2 Edison EVM over NFS with the $subject patch.
Boot log is below. Do you see anything suspicious?

Logs looks ok to me. Probably do some tests where DMA and bounce buffers 
etc gets tested. Running it through your internal regression

suit will be good idea as well if thats possible.

Regards,
Santosh
--
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: [PATCH 1/1] ARM: dma: fix dma_max_pfn()

[Santosh Shilimkar]

Hi Russell,

On 8/18/2016 7:24 AM, Russell King - ARM Linux wrote:

On Wed, Aug 17, 2016 at 03:05:17PM +0300, Roger Quadros wrote:

Since commit 6ce0d2001692 ("ARM: dma: Use dma_pfn_offset for dma address 
translation"),
dma_to_pfn() already returns the PFN with the physical memory start offset
so we don't need to add it again.

This fixes USB mass storage lock-up problem on systems that can't do DMA
over the entire physical memory range (e.g.) Keystone 2 systems with 4GB RAM
can only do DMA over the first 2GB. [K2E-EVM].

What happens there is that without this patch SCSI layer sets a wrong
bounce buffer limit in scsi_calculate_bounce_limit() for the USB mass
storage device. dma_max_pfn() evaluates to 0x8f and bounce_limit
is set to 0x8f000 whereas maximum DMA'ble physical memory on Keystone 2
is 0x87fff. This results in non DMA'ble pages being given to the
USB controller and hence the lock-up.

NOTE: in the above case, USB-SCSI-device's dma_pfn_offset was showing as 0.
This should have really been 0x78 as on K2e, LOWMEM_START is 0x8000
and HIGHMEM_START is 0x8. DMA zone is 2GB so dma_max_pfn should be
0x87. The incorrect dma_pfn_offset for the USB storage device is because
USB devices are not correctly inheriting the dma_pfn_offset from the
USB host controller. This will be fixed by a separate patch.


I'd like to hear from Santosh, as the author of the original change.
The original commit doesn't mention which platform it was intended for
or what the problem was, which would've been helpful.


From what I recollect, we did these changes to make the max pfn behave
same on ARM arch as other archs. This patch was evolved as part of
fixing the max*pfn assumption.

commit 26ba47b18318abe7dadbe9294a611c0e932651d8
Author: Santosh Shilimkar <santosh.shilim...@ti.com>
Date:   Thu Aug 1 03:12:01 2013 +0100

ARM: 7805/1: mm: change max*pfn to include the physical offset of 
memory


Most of the kernel code assumes that max*pfn is maximum pfns because
the physical start of memory is expected to be PFN0. Since this
assumption is not true on ARM architectures, the meaning of max*pfn
is number of memory pages. This is done to keep drivers happy which
are making use of of these variable to calculate the dma bounce limit
using dma_mask.

Now since we have a architecture override possibility for DMAable
maximum pfns, lets make meaning of max*pfns as maximum pnfs on ARM
as well.



Fixes: 6ce0d2001692 ("ARM: dma: Use dma_pfn_offset for dma address translation")
Cc: sta...@vger.kernel.org
Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Cc: Russell King <li...@arm.linux.org.uk>
Cc: Arnd Bergmann <a...@arndb.de>
Cc: Olof Johansson <o...@lixom.net>
Cc: Catalin Marinas <catalin.mari...@arm.com>
Cc: Linus Walleij <linus.wall...@linaro.org>
Reported-by: Grygorii Strashko <grygorii.stras...@ti.com>
Signed-off-by: Roger Quadros <rog...@ti.com>
---
 arch/arm/include/asm/dma-mapping.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/dma-mapping.h 
b/arch/arm/include/asm/dma-mapping.h
index d009f79..bf02dbd 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -111,7 +111,7 @@ static inline dma_addr_t virt_to_dma(struct device *dev, 
void *addr)
 /* The ARM override for dma_max_pfn() */
 static inline unsigned long dma_max_pfn(struct device *dev)
 {
-   return PHYS_PFN_OFFSET + dma_to_pfn(dev, *dev->dma_mask);
+   return dma_to_pfn(dev, *dev->dma_mask);
 }
 #define dma_max_pfn(dev) dma_max_pfn(dev)

By doing this change I hope we don't break other drivers on Keystone so
am not sure about the change.
--
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: [PATCH v3 2/2] usb:dwc3: pass arch data to xhci-hcd child

[santosh shilimkar]

On 4/3/2016 11:28 PM, Felipe Balbi wrote:

santosh shilimkar <santosh.shilim...@oracle.com> writes:

+Arnd, RMK,

On 4/1/2016 4:57 AM, Felipe Balbi wrote:


Hi,

Grygorii Strashko <grygorii.stras...@ti.com> writes:

On 04/01/2016 01:20 PM, Felipe Balbi wrote:


[...]


commit 7ace8fc8219e4cbbfd5b4790390d9a01a2541cdf
Author: Yoshihiro Shimoda <yoshihiro.shimoda...@renesas.com>
Date:   Mon Jul 13 18:10:05 2015 +0900

  usb: gadget: udc: core: Fix argument of dma_map_single for IOMMU

  The dma_map_single and dma_unmap_single should set "gadget->dev.parent"
  instead of ">dev" in the first argument because the parent has
  a udc controller's device pointer.
  Otherwise, iommu functions are not called in ARM environment.

  Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda...@renesas.com>
  Signed-off-by: Felipe Balbi <ba...@ti.com>

Above actually means that DMA configuration code can be dropped from
usb_add_gadget_udc_release() completely. Right?:


true, but now I'm not sure what's better: copy all necessary bits from
parent or just pass the parent device to all DMA API.

Anybody to shed a light here ?


The expectation is drivers should pass the proper dev pointers and let
core DMA code deal with it since it knows the per device dma properties.


okay, so how do you get proper DMA pointers with something like this:

kdwc3_dma_mask = dma_get_mask(dev);
dev->dma_mask = _dma_mask;

This doesn't anything.


Drivers actually needs to touch dma_mask(s) only if the core DMA
code hasn't populated it it. I see Grygorii pointed out couple
of things already.

Reagrds,
Santosh



--
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: [PATCH v3 2/2] usb:dwc3: pass arch data to xhci-hcd child

[santosh shilimkar]

+Arnd, RMK,

On 4/1/2016 4:57 AM, Felipe Balbi wrote:


Hi,

Grygorii Strashko  writes:

On 04/01/2016 01:20 PM, Felipe Balbi wrote:


[...]


commit 7ace8fc8219e4cbbfd5b4790390d9a01a2541cdf
Author: Yoshihiro Shimoda 
Date:   Mon Jul 13 18:10:05 2015 +0900

 usb: gadget: udc: core: Fix argument of dma_map_single for IOMMU

 The dma_map_single and dma_unmap_single should set "gadget->dev.parent"
 instead of ">dev" in the first argument because the parent has
 a udc controller's device pointer.
 Otherwise, iommu functions are not called in ARM environment.

 Signed-off-by: Yoshihiro Shimoda 
 Signed-off-by: Felipe Balbi 

Above actually means that DMA configuration code can be dropped from
usb_add_gadget_udc_release() completely. Right?:


true, but now I'm not sure what's better: copy all necessary bits from
parent or just pass the parent device to all DMA API.

Anybody to shed a light here ?

The expectation is drivers should pass the proper dev pointers and let 
core DMA code deal with it since it knows the per device dma properties.

RMK did massive series of patches to fix many drivers which were not
adhering to dma APIs.

Regrds,
Santosh

--
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: [PATCH] usb: dwc3: core: avoid NULL pointer dereference

[santosh shilimkar]

Felipe,

On 7/15/2015 2:56 PM, Murali Karicheri wrote:

On 07/02/2015 06:34 PM, Felipe Balbi wrote:

commit 3e10a2ce98d1 (usb: dwc3: add hsphy_interface
property) introduced a possible NULL pointer
dereference because dwc-hsphy_interface can be
NULL.

In order to fix it, all we have to do is guard
strncmp() against a NULL argument.

Fixes: 3e10a2ce98d1 (usb: dwc3: add hsphy_interface property)
Tested-by: Murali Karicheri m-kariche...@ti.com
Signed-off-by: Felipe Balbi ba...@ti.com
---
  drivers/usb/dwc3/core.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 5c110d8e293b..ff5773c66b84 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -446,10 +446,12 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
  /* Select the HS PHY interface */
  switch (DWC3_GHWPARAMS3_HSPHY_IFC(dwc-hwparams.hwparams3)) {
  case DWC3_GHWPARAMS3_HSPHY_IFC_UTMI_ULPI:
-if (!strncmp(dwc-hsphy_interface, utmi, 4)) {
+if (dwc-hsphy_interface 
+!strncmp(dwc-hsphy_interface, utmi, 4)) {
  reg = ~DWC3_GUSB2PHYCFG_ULPI_UTMI;
  break;
-} else if (!strncmp(dwc-hsphy_interface, ulpi, 4)) {
+} else if (dwc-hsphy_interface 
+!strncmp(dwc-hsphy_interface, ulpi, 4)) {
  reg |= DWC3_GUSB2PHYCFG_ULPI_UTMI;
  dwc3_writel(dwc-regs, DWC3_GUSB2PHYCFG(0), reg);
  } else {


Dear Maintainer,

This is patch is urgently required to be applied to master for v4.2 for
fixing a crash seen on keystone based boards (K2HK, K2L and K2E EVMs).
Please merge it asap.


I assume you are you sending this one for rc's ?
Feel free to use my ack if you need one.
Acked-by: Santosh Shilimkar ssant...@kernel.org
--
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: [PATCH][next] phy: core: make NULL a valid phy reference if !CONFIG_GENERIC_PHY

[Santosh Shilimkar]

On Thursday 13 March 2014 05:44 PM, Felipe Balbi wrote:
 Hi,
 
 On Thu, Mar 13, 2014 at 10:20:24AM -0500, Felipe Balbi wrote:
 On Thu, Mar 13, 2014 at 01:11:13PM +0200, Grygorii Strashko wrote:
 This fixes a regression on Keystone 2 platforms caused by patch
 57303488cd37da58263e842de134dc65f7c626d5
 usb: dwc3: adapt dwc3 core to use Generic PHY Framework which adds
 optional support of generic phy in DWC3 core.

 On Keystone 2 platforms the USB is not working now because
 CONFIG_GENERIC_PHY isn't set and, as result, Generic PHY APIs stubs
 return -ENOSYS always. The log shows:
  dwc3 269.dwc3: failed to initialize core
  dwc3: probe of 269.dwc3 failed with error -38

 Hence, fix it by making NULL a valid phy reference in Generic PHY
 APIs stubs in the same way as it was done by the patch
 04c2facad8fee66c981a51852806d8923336f362 drivers: phy: Make NULL
 a valid phy reference.

 CC: Kishon Vijay Abraham I kis...@ti.com
 CC: Felipe Balbi ba...@ti.com
 CC: Santosh Shilimkar santosh.shilim...@ti.com
 Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com

 nice :-)

 Acked-by: Felipe Balbi ba...@ti.com
 
 Greg, if your tree isn't closed yet, could you consider this patch still
 for v3.15 merge window ? Grygorii found a regression on Keystone
 platforms which this patch fixes. Let me know if you need the original
 patch and myself or Kishon can send it to you.
 
Just checking whether the fix was picked up for the 3.14 merge window ?

Regards,
Santosh
--
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: [PATCH][next] phy: core: make NULL a valid phy reference if !CONFIG_GENERIC_PHY

[Santosh Shilimkar]

On Wednesday 02 April 2014 02:16 PM, Greg KH wrote:
 On Wed, Apr 02, 2014 at 01:53:19PM -0400, Santosh Shilimkar wrote:
 On Thursday 13 March 2014 05:44 PM, Felipe Balbi wrote:
 Hi,

 On Thu, Mar 13, 2014 at 10:20:24AM -0500, Felipe Balbi wrote:
 On Thu, Mar 13, 2014 at 01:11:13PM +0200, Grygorii Strashko wrote:
 This fixes a regression on Keystone 2 platforms caused by patch
 57303488cd37da58263e842de134dc65f7c626d5
 usb: dwc3: adapt dwc3 core to use Generic PHY Framework which adds
 optional support of generic phy in DWC3 core.

 On Keystone 2 platforms the USB is not working now because
 CONFIG_GENERIC_PHY isn't set and, as result, Generic PHY APIs stubs
 return -ENOSYS always. The log shows:
  dwc3 269.dwc3: failed to initialize core
  dwc3: probe of 269.dwc3 failed with error -38

 Hence, fix it by making NULL a valid phy reference in Generic PHY
 APIs stubs in the same way as it was done by the patch
 04c2facad8fee66c981a51852806d8923336f362 drivers: phy: Make NULL
 a valid phy reference.

 CC: Kishon Vijay Abraham I kis...@ti.com
 CC: Felipe Balbi ba...@ti.com
 CC: Santosh Shilimkar santosh.shilim...@ti.com
 Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com

 nice :-)

 Acked-by: Felipe Balbi ba...@ti.com

 Greg, if your tree isn't closed yet, could you consider this patch still
 for v3.15 merge window ? Grygorii found a regression on Keystone
 platforms which this patch fixes. Let me know if you need the original
 patch and myself or Kishon can send it to you.

 Just checking whether the fix was picked up for the 3.14 merge window ?
 
 3.14 is long released, the merge window for that was months ago.
 
Sorry for the typo. I mean for upcoming v3.15 merge window.


regards,
Santosh


--
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: [PATCH][next] phy: core: make NULL a valid phy reference if !CONFIG_GENERIC_PHY

[Santosh Shilimkar]

On Thursday 13 March 2014 07:11 PM, Strashko, Grygorii wrote:
 This fixes a regression on Keystone 2 platforms caused by patch
 57303488cd37da58263e842de134dc65f7c626d5
 usb: dwc3: adapt dwc3 core to use Generic PHY Framework which adds
 optional support of generic phy in DWC3 core.
 
 On Keystone 2 platforms the USB is not working now because
 CONFIG_GENERIC_PHY isn't set and, as result, Generic PHY APIs stubs
 return -ENOSYS always. The log shows:
  dwc3 269.dwc3: failed to initialize core
  dwc3: probe of 269.dwc3 failed with error -38
 
 Hence, fix it by making NULL a valid phy reference in Generic PHY
 APIs stubs in the same way as it was done by the patch
 04c2facad8fee66c981a51852806d8923336f362 drivers: phy: Make NULL
 a valid phy reference.
 
 CC: Kishon Vijay Abraham I kis...@ti.com
 CC: Felipe Balbi ba...@ti.com
 CC: Santosh Shilimkar santosh.shilim...@ti.com
 Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
 ---
This fixes the regression seen in Linux next and patch seems
reasonable to me.
Acked-by: Santosh Shilimkar santosh.shilim...@ti.com

Felipe, Kishon,
Can you guys pick this fix if you are ok by it. Thanks


  include/linux/phy/phy.h |8 
  1 file changed, 8 insertions(+)
 
 diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
 index e2f5ca9..5a9b193 100644
 --- a/include/linux/phy/phy.h
 +++ b/include/linux/phy/phy.h
 @@ -204,21 +204,29 @@ static inline void phy_pm_runtime_forbid(struct phy 
 *phy)
  
  static inline int phy_init(struct phy *phy)
  {
 + if (!phy)
 + return 0;
   return -ENOSYS;
  }
  
  static inline int phy_exit(struct phy *phy)
  {
 + if (!phy)
 + return 0;
   return -ENOSYS;
  }
  
  static inline int phy_power_on(struct phy *phy)
  {
 + if (!phy)
 + return 0;
   return -ENOSYS;
  }
  
  static inline int phy_power_off(struct phy *phy)
  {
 + if (!phy)
 + return 0;
   return -ENOSYS;
  }
  
 

--
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: [PATCH][next] phy: core: make NULL a valid phy reference if !CONFIG_GENERIC_PHY

[Santosh Shilimkar]

On Thursday 13 March 2014 09:43 PM, Kishon Vijay Abraham I wrote:
 Hi Santosh,
 
 On Thursday 13 March 2014 07:07 PM, Santosh Shilimkar wrote:
 On Thursday 13 March 2014 07:11 PM, Strashko, Grygorii wrote:
 This fixes a regression on Keystone 2 platforms caused by patch
 57303488cd37da58263e842de134dc65f7c626d5
 usb: dwc3: adapt dwc3 core to use Generic PHY Framework which adds
 optional support of generic phy in DWC3 core.

 On Keystone 2 platforms the USB is not working now because
 CONFIG_GENERIC_PHY isn't set and, as result, Generic PHY APIs stubs
 return -ENOSYS always. The log shows:
   dwc3 269.dwc3: failed to initialize core
   dwc3: probe of 269.dwc3 failed with error -38

 Hence, fix it by making NULL a valid phy reference in Generic PHY
 APIs stubs in the same way as it was done by the patch
 04c2facad8fee66c981a51852806d8923336f362 drivers: phy: Make NULL
 a valid phy reference.

 CC: Kishon Vijay Abraham I kis...@ti.com
 CC: Felipe Balbi ba...@ti.com
 CC: Santosh Shilimkar santosh.shilim...@ti.com
 Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
 ---
 This fixes the regression seen in Linux next and patch seems
 reasonable to me.
 Acked-by: Santosh Shilimkar santosh.shilim...@ti.com

 Felipe, Kishon,
 Can you guys pick this fix if you are ok by it. Thanks
 
 I've already given a PULL request to Greg for 3.15. Is it ok to take this in 
 -rc cycle?
 
Am not sure because this is breaking the existing functionality.
May be you can request Greg to pull this fix as well.

Regards,
Santosh
--
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: [PATCH] usb: dwc3: keystone: switch to use runtime pm

[Santosh Shilimkar]

On Friday 31 January 2014 08:20 AM, Grygorii Strashko wrote:
 The Keystone PM management layer has been implemented using PM bus for
 power management clocks. As result, most of Keystone drivers don't need
 to manage clocks directly. They just need to enable runtime PM and use it
 to handle their PM state and clocks.
 
 Hence, remove clock management code and switch to use runtime PM.
 
 Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
 ---
Please capture why now it allowes us to remove the clock code.
Commit 8a6720e {PM / clock_ops: fix up clk prepare/unprepare count}

Without that information, the change log will be miss-leading

  drivers/usb/dwc3/dwc3-keystone.c |   15 ---
  1 file changed, 8 insertions(+), 7 deletions(-)
 
 diff --git a/drivers/usb/dwc3/dwc3-keystone.c 
 b/drivers/usb/dwc3/dwc3-keystone.c
 index 1fad161..a810b41 100644
 --- a/drivers/usb/dwc3/dwc3-keystone.c
 +++ b/drivers/usb/dwc3/dwc3-keystone.c
 @@ -23,6 +23,7 @@
  #include linux/dma-mapping.h
  #include linux/io.h
  #include linux/of_platform.h
 +#include linux/pm_runtime.h
  
  /* USBSS register offsets */
  #define USBSS_REVISION   0x
 @@ -116,12 +117,10 @@ static int kdwc3_probe(struct platform_device *pdev)
   kdwc3_dma_mask = dma_get_mask(dev);
   dev-dma_mask = kdwc3_dma_mask;
  
 - kdwc-clk = devm_clk_get(kdwc-dev, usb);
 -
 - error = clk_prepare_enable(kdwc-clk);
 + pm_runtime_enable(dev);
 + error = pm_runtime_get_sync(dev);
   if (error  0) {
 - dev_dbg(kdwc-dev, unable to enable usb clock, err %d\n,
 - error);
 + dev_dbg(dev, unable to enable usb dev, err %d\n, error);
   return error;
   }
  
 @@ -152,7 +151,8 @@ static int kdwc3_probe(struct platform_device *pdev)
  err_core:
   kdwc3_disable_irqs(kdwc);
  err_irq:
 - clk_disable_unprepare(kdwc-clk);
 + pm_runtime_put_sync(dev);
 + pm_runtime_disable(dev);
  
   return error;
  }
 @@ -172,7 +172,8 @@ static int kdwc3_remove(struct platform_device *pdev)
  
   kdwc3_disable_irqs(kdwc);
   device_for_each_child(pdev-dev, NULL, kdwc3_remove_core);
 - clk_disable_unprepare(kdwc-clk);
 + pm_runtime_put_sync(pdev-dev);
 + pm_runtime_disable(pdev-dev);
   platform_set_drvdata(pdev, NULL);
  
   return 0;
 

--
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: [PATCH] usb: dwc3: keystone: switch to use runtime pm

[Santosh Shilimkar]

On Friday 31 January 2014 10:19 AM, Felipe Balbi wrote:
 Hi,
 
 On Fri, Jan 31, 2014 at 03:20:26PM +0200, Grygorii Strashko wrote:
 The Keystone PM management layer has been implemented using PM bus for
 power management clocks. As result, most of Keystone drivers don't need
 to manage clocks directly. They just need to enable runtime PM and use it
 to handle their PM state and clocks.

 Hence, remove clock management code and switch to use runtime PM.

 Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
 
 quite a few weeks back I sent a series enabling runtime pm for all glue
 layers. I'll use that version instead, sorry.
 
That should be fine but you need to drop clk_*() related code
from that patch. I assume you will send refresh version of it then.

Regards,
Santosh
--
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: [PATCH] usb: dwc3: keystone: switch to use runtime pm

[Santosh Shilimkar]

On Friday 31 January 2014 10:47 AM, Felipe Balbi wrote:
 On Fri, Jan 31, 2014 at 10:43:21AM -0500, Santosh Shilimkar wrote:
 On Friday 31 January 2014 10:19 AM, Felipe Balbi wrote:
 Hi,

 On Fri, Jan 31, 2014 at 03:20:26PM +0200, Grygorii Strashko wrote:
 The Keystone PM management layer has been implemented using PM bus for
 power management clocks. As result, most of Keystone drivers don't need
 to manage clocks directly. They just need to enable runtime PM and use it
 to handle their PM state and clocks.

 Hence, remove clock management code and switch to use runtime PM.

 Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com

 quite a few weeks back I sent a series enabling runtime pm for all glue
 layers. I'll use that version instead, sorry.

 That should be fine but you need to drop clk_*() related code
 from that patch. I assume you will send refresh version of it then.
 
 why ? it makes no difference if you enable twice and disable twice.
 
Sure but why do you want to have the clock node handling code in drivers
if it is not needed. Isn't that better ?
--
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: [PATCH] usb: dwc3: keystone: switch to use runtime pm

[Santosh Shilimkar]

On Friday 31 January 2014 11:45 AM, Felipe Balbi wrote:
 On Fri, Jan 31, 2014 at 10:50:40AM -0500, Santosh Shilimkar wrote:
 On Friday 31 January 2014 10:47 AM, Felipe Balbi wrote:
 On Fri, Jan 31, 2014 at 10:43:21AM -0500, Santosh Shilimkar wrote:
 On Friday 31 January 2014 10:19 AM, Felipe Balbi wrote:
 Hi,

 On Fri, Jan 31, 2014 at 03:20:26PM +0200, Grygorii Strashko wrote:
 The Keystone PM management layer has been implemented using PM bus for
 power management clocks. As result, most of Keystone drivers don't need
 to manage clocks directly. They just need to enable runtime PM and use it
 to handle their PM state and clocks.

 Hence, remove clock management code and switch to use runtime PM.

 Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com

 quite a few weeks back I sent a series enabling runtime pm for all glue
 layers. I'll use that version instead, sorry.

 That should be fine but you need to drop clk_*() related code
 from that patch. I assume you will send refresh version of it then.

 why ? it makes no difference if you enable twice and disable twice.

 Sure but why do you want to have the clock node handling code in drivers
 if it is not needed. Isn't that better ?
 
 It might, but then way that I wanted to see it is so that I can make
 assumptions about the device state. From a driver perspective, what I
 would love to see is the ability to assume that when my probe gets
 called the device is already enabled. So if you can make sure that
 clk_enable() happens before my probe and that you call
 pm_runtime_set_active() before my probe too, then I can more than
 hapilly remove clk_* calls from the driver ;-)
 
 either that or maintain the driver like so:
 
 probe()
 {
   ...
 
   clk_get(dev, fck);
   clk_prepare(clk);
   clk_enable(clk);
   pm_runtime_set_active(dev);
   pm_runtime_enable(dev);
   pm_runtime_get_sync(dev);
   ...
 }
 
 runtime_suspend()
 {
   clk_disable(dev);
 }
 
 runtime_resume()
 {
   clk_enable(dev);
 }
 
 note that because of pm_runtime_set_active() that first
 pm_runtime_get_sync() in probe() will simply increase the reference
 counter without calling my -runtime_resume() callback, which is exactly
 what we want, as that would completely avoid situations of bad context
 being restored because of that initial pm_runtime_get_sync().
 
Thanks for making your point bit clear. 

 Then, we can even make pm_runtime completely async easily, because
 clk_prepare() was called only on probe() (or before it, for that
 matter).
 
 Bottomline is, if you can guarantee me that clk_get(), clk_prepare(),
 clk_enable() and pm_runtime_set_active() will be called properly before
 my probe, i'll be more than happy to comply with your request above as
 that will greatly simplify my driver.
 
Which is the case at least I see on Keystone. And hence the patch from
Grygorii works. I also noticed your proposal for wider platform to
enforce above behavior which seems to be a good idea.

 Just make, also, that if this clock is shared between dwc3-keystone
 wrapper and dwc3 core, you clk_get() on both driver's probe.
 
I understand. In summary, whichever patch you pick(yours) or Grygorii's,
its completely safe to remove the clock handling from Keystone USB driver.

Regards,
Santosh
--
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: [PATCH] usb: dwc3: keystone: switch to use runtime pm

[Santosh Shilimkar]

On Friday 31 January 2014 05:15 PM, Felipe Balbi wrote:
 Hi,
 
 On Fri, Jan 31, 2014 at 02:20:48PM -0500, Santosh Shilimkar wrote:
 
 [ snip ]
 
 note that because of pm_runtime_set_active() that first
 pm_runtime_get_sync() in probe() will simply increase the reference
 counter without calling my -runtime_resume() callback, which is exactly
 what we want, as that would completely avoid situations of bad context
 being restored because of that initial pm_runtime_get_sync().

 Thanks for making your point bit clear. 
 
 no problem.
 
 Then, we can even make pm_runtime completely async easily, because
 clk_prepare() was called only on probe() (or before it, for that
 matter).

 Bottomline is, if you can guarantee me that clk_get(), clk_prepare(),
 clk_enable() and pm_runtime_set_active() will be called properly before
 my probe, i'll be more than happy to comply with your request above as
 that will greatly simplify my driver.

 Which is the case at least I see on Keystone. And hence the patch from
 
 I was going over pm_domain.c and drivers/base/power/clock_ops.c and none
 of them enable pm_runtime or make sure pm_runtime_set_active() is
 called.
 
 Grygorii works. I also noticed your proposal for wider platform to
 enforce above behavior which seems to be a good idea.
 
 it'll take months to stabilize though ;-)
 
 Just make, also, that if this clock is shared between dwc3-keystone
 wrapper and dwc3 core, you clk_get() on both driver's probe.

 I understand. In summary, whichever patch you pick(yours) or Grygorii's,
 its completely safe to remove the clock handling from Keystone USB driver.
 
 alright, since I can't really test, I'll take this as a true statement.
 If there are any regressions I can blame you, hehehe.
 
No problem... :D
Grygorii patch has been working well so all good with that

--
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: [PATCH] usb: host: xhci: Move suspend ops under PM_SLEEP to avoid warning

[Santosh Shilimkar]

On Friday 13 December 2013 12:23 AM, David Cohen wrote:
 On Thu, Dec 12, 2013 at 07:25:55PM -0800, David Cohen wrote:
 On Thu, Dec 12, 2013 at 09:01:04PM -0500, Santosh Shilimkar wrote:
 On Thursday 12 December 2013 08:51 PM, David Cohen wrote:
 On Thu, Dec 12, 2013 at 08:06:24PM -0500, Santosh Shilimkar wrote:
 Otherwise you get below build warnings

 drivers/usb/host/xhci-plat.c:201:12: warning: ‘xhci_plat_suspend’ defined 
 but not used [-Wunused-function]
 drivers/usb/host/xhci-plat.c:209:12: warning: ‘xhci_plat_resume’ defined 
 but not used [-Wunused-function]

 Cc: Sarah Sharp sarah.a.sh...@linux.intel.com
 Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
 Signed-off-by: Santosh Shilimkar santosh.shilim...@ti.com
 ---
  drivers/usb/host/xhci-plat.c |4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

 diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
 index d9c169f..4875be5 100644
 --- a/drivers/usb/host/xhci-plat.c
 +++ b/drivers/usb/host/xhci-plat.c
 @@ -197,7 +197,7 @@ static int xhci_plat_remove(struct platform_device 
 *dev)
   return 0;
  }
  
 -#ifdef CONFIG_PM
 +#ifdef CONFIG_PM_SLEEP

 Can't you just remove these #ifdefs altogether?
 xhci_plat_pm_ops is set using SET_SYSTEM_SLEEP_PM_OPS() macro which
 already handles '#ifdef CONFIG_PM_SLEEP' case.

 It does handle the difference but the hooks implemented would
 show-up un-used warning if you remove the #ifdefs.

 drivers/usb/host/xhci-plat.c:200:12: warning: ‘xhci_plat_suspend’ defined 
 but not used [-Wunused-function]
 drivers/usb/host/xhci-plat.c:208:12: warning: ‘xhci_plat_resume’ defined 
 but not used [-Wunused-function]

 So you need to wrap them under the PM_SLEEP check.

 Yeah... it's not smart enought :)
 But you could still remove the #else condition and the ugly DEV_PM_OPS
 macro.
 
 Since this patch is not urgent, I sent a RFC proposing smarter
 SET_*_PM_OPS(). I included your patch (a bit different) here:
 https://patchwork.kernel.org/patch/3337961/
 
Thats fine by me if you can get your RFC through.

Regards,
Santosh

--
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: [PATCH v2 1/7] usb: dwc3: keystone: add basic PM support

[Santosh Shilimkar]

On Thursday 12 December 2013 07:43 PM, Felipe Balbi wrote:
 On Thu, Dec 12, 2013 at 07:29:24PM -0500, Santosh Shilimkar wrote:
 On Thursday 12 December 2013 04:45 PM, Felipe Balbi wrote:
 A bare-minimum PM implementation which will
 server as building block for more complex
 s/server/serve ;-)
 
 hah! :-) will fix in my branch.
 
 PM implementation in the future.

 At the least will not leave clocks on unnecessarily
 when e.g.  a user write mem to /sys/power/state.

 Signed-off-by: Felipe Balbi ba...@ti.com
 ---

 improve error path a little bit.

 We will test this out. Thanks for the
 patch Felipe.
 
I see Wingman already tested the patch.
Feel free add, my ack if you need one...

Acked-by: Santosh Shilimkar santosh.shilim...@ti.com

--
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: [PATCH v3 1/2] usb: dwc3: Add Keystone specific glue layer

[Santosh Shilimkar]

On Thursday 12 December 2013 02:41 PM, Felipe Balbi wrote:
 On Thu, Dec 12, 2013 at 02:36:01PM -0500, Santosh Shilimkar wrote:
 On Thursday 12 December 2013 12:48 PM, Felipe Balbi wrote:
 Hi,

 On Tue, Dec 10, 2013 at 10:11:36AM -0500, Santosh Shilimkar wrote:
 +static int kdwc3_remove(struct platform_device *pdev)
 +{
 +struct dwc3_keystone *kdwc = platform_get_drvdata(pdev);
 +
 +kdwc3_disable_irqs(kdwc);
 +clk_disable_unprepare(kdwc-clk);

 I hope the clock isn't shared between core and wrapper, otherwise you
 could run into some troubles here. Can you confirm ?

 Yes. the clock isn't shared. Thanks for taking care of other parts.

 so clock for core is always running too ?

 I take that back. The clock is actually common so we should disable
 it after removing the  kdwc3_remove_core() as you suggested.

 You won't see issue since the  kdwc3_remove_core() not doing
 any register access but moving the clock disable after
 the core remove is right thing to do.
 
 the problem is not kdwc3_remove_core() accessing registers, but
 dwc3_remove() _does_ access registers during remove. If you just
 mopdrobe -r dwc3-keystone without removing dwc3.ko first, then
 kdwc3_remove_core() will cause dwc3.ko to be removed (because of
 platform_driver_unregister()) and, since clocks have already been
 disabled, then we'd die :-)
 
Oh yes, you are right. I see Wingman already posted the updated patch.

Regards,
Santosh

--
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: [PATCH v2 1/7] usb: dwc3: keystone: add basic PM support

[Santosh Shilimkar]

On Thursday 12 December 2013 04:45 PM, Felipe Balbi wrote:
 A bare-minimum PM implementation which will
 server as building block for more complex
s/server/serve ;-)
 PM implementation in the future.
 
 At the least will not leave clocks on unnecessarily
 when e.g.  a user write mem to /sys/power/state.
 
 Signed-off-by: Felipe Balbi ba...@ti.com
 ---
 
 improve error path a little bit.
 
We will test this out. Thanks for the
patch Felipe.

Regards,
Santosh
--
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

[PATCH] usb: host: xhci: Move suspend ops under PM_SLEEP to avoid warning

[Santosh Shilimkar]

Otherwise you get below build warnings

drivers/usb/host/xhci-plat.c:201:12: warning: ‘xhci_plat_suspend’ defined but 
not used [-Wunused-function]
drivers/usb/host/xhci-plat.c:209:12: warning: ‘xhci_plat_resume’ defined but 
not used [-Wunused-function]

Cc: Sarah Sharp sarah.a.sh...@linux.intel.com
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
Signed-off-by: Santosh Shilimkar santosh.shilim...@ti.com
---
 drivers/usb/host/xhci-plat.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index d9c169f..4875be5 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -197,7 +197,7 @@ static int xhci_plat_remove(struct platform_device *dev)
return 0;
 }
 
-#ifdef CONFIG_PM
+#ifdef CONFIG_PM_SLEEP
 static int xhci_plat_suspend(struct device *dev)
 {
struct usb_hcd  *hcd = dev_get_drvdata(dev);
@@ -220,7 +220,7 @@ static const struct dev_pm_ops xhci_plat_pm_ops = {
 #define DEV_PM_OPS (xhci_plat_pm_ops)
 #else
 #define DEV_PM_OPS NULL
-#endif /* CONFIG_PM */
+#endif /* CONFIG_PM_SLEEP */
 
 #ifdef CONFIG_OF
 static const struct of_device_id usb_xhci_of_match[] = {
-- 
1.7.9.5

--
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: [PATCH] usb: host: xhci: Move suspend ops under PM_SLEEP to avoid warning

[Santosh Shilimkar]

On Thursday 12 December 2013 08:51 PM, David Cohen wrote:
 On Thu, Dec 12, 2013 at 08:06:24PM -0500, Santosh Shilimkar wrote:
 Otherwise you get below build warnings

 drivers/usb/host/xhci-plat.c:201:12: warning: ‘xhci_plat_suspend’ defined 
 but not used [-Wunused-function]
 drivers/usb/host/xhci-plat.c:209:12: warning: ‘xhci_plat_resume’ defined but 
 not used [-Wunused-function]

 Cc: Sarah Sharp sarah.a.sh...@linux.intel.com
 Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
 Signed-off-by: Santosh Shilimkar santosh.shilim...@ti.com
 ---
  drivers/usb/host/xhci-plat.c |4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

 diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
 index d9c169f..4875be5 100644
 --- a/drivers/usb/host/xhci-plat.c
 +++ b/drivers/usb/host/xhci-plat.c
 @@ -197,7 +197,7 @@ static int xhci_plat_remove(struct platform_device *dev)
  return 0;
  }
  
 -#ifdef CONFIG_PM
 +#ifdef CONFIG_PM_SLEEP
 
 Can't you just remove these #ifdefs altogether?
 xhci_plat_pm_ops is set using SET_SYSTEM_SLEEP_PM_OPS() macro which
 already handles '#ifdef CONFIG_PM_SLEEP' case.
 
It does handle the difference but the hooks implemented would
show-up un-used warning if you remove the #ifdefs.

drivers/usb/host/xhci-plat.c:200:12: warning: ‘xhci_plat_suspend’ defined but 
not used [-Wunused-function]
drivers/usb/host/xhci-plat.c:208:12: warning: ‘xhci_plat_resume’ defined but 
not used [-Wunused-function]

So you need to wrap them under the PM_SLEEP check.

Regards,
Santosh

--
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: [PATCH v3 1/2] usb: dwc3: Add Keystone specific glue layer

[Santosh Shilimkar]

On Monday 09 December 2013 09:51 PM, Balbi, Felipe wrote:
 Hi,
 
 On Mon, Dec 09, 2013 at 05:17:03PM -0500, WingMan Kwok wrote:
 +static void kdwc3_enable_irqs(struct dwc3_keystone *kdwc)
 +{
 +u32 val;
 +
 +val = kdwc3_readl(kdwc-usbss, USBSS_IRQENABLE_SET_0);
 +val = USBSS_IRQ_COREIRQ_EN;
 
 this misses the | in |=. I can fix it up while applying, this time only.
 
 +static int kdwc3_probe(struct platform_device *pdev)
 +{
 +struct device   *dev = pdev-dev;
 +struct device_node  *node = pdev-dev.of_node;
 +struct dwc3_keystone*kdwc;
 +struct resource *res;
 +int error, irq;
 +
 +kdwc = devm_kzalloc(dev, sizeof(*kdwc), GFP_KERNEL);
 +if (!kdwc)
 +return -ENOMEM;
 +
 +platform_set_drvdata(pdev, kdwc);
 +
 +kdwc-dev = dev;
 +
 +res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 +if (!res) {
 +dev_err(dev, missing usbss resource\n);
 +return -EINVAL;
 +}
 +
 +kdwc-usbss = devm_ioremap_resource(dev, res);
 +if (IS_ERR(kdwc-usbss))
 +return PTR_ERR(kdwc-usbss);
 +
 +kdwc3_dma_mask = dma_get_mask(dev);
 +dev-dma_mask = kdwc3_dma_mask;
 +
 +kdwc-clk = devm_clk_get(kdwc-dev, usb);
 +if (IS_ERR_OR_NULL(kdwc-clk)) {
 
 clk_get() will never return NULL. This time, I'll fix it while applying.
 
 +static int kdwc3_remove(struct platform_device *pdev)
 +{
 +struct dwc3_keystone *kdwc = platform_get_drvdata(pdev);
 +
 +kdwc3_disable_irqs(kdwc);
 +clk_disable_unprepare(kdwc-clk);
 
 I hope the clock isn't shared between core and wrapper, otherwise you
 could run into some troubles here. Can you confirm ?
 
Yes. the clock isn't shared. Thanks for taking care of other parts.
--
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: [PATCH v3 2/2] usb: phy: Add keystone usb phy driver

[Santosh Shilimkar]

On Monday 09 December 2013 09:54 PM, Balbi, Felipe wrote:
 Hi,
 
 On Mon, Dec 09, 2013 at 05:17:04PM -0500, WingMan Kwok wrote:
 +ret = usb_add_phy_dev(k_phy-usb_phy_gen.phy);
 +if (ret)
 +return ret;
 +k_phy-usb_phy_gen.phy.init = keystone_usbphy_init;
 +k_phy-usb_phy_gen.phy.shutdown = keystone_usbphy_shutdown;
 
 this *must* be initialized before adding the PHY to the subsystem. So
 these two lines must be moved before usb_add_phy_dev().
 
Make sense. Probably its good idea to repost the $subject patch with
above as well as other delay related comment.

Regards,
Santosh
--
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: [PATCH v3 2/2] usb: phy: Add keystone usb phy driver

[Santosh Shilimkar]

On Monday 09 December 2013 11:47 PM, Felipe Balbi wrote:
 Hi again,
 
 On Mon, Dec 09, 2013 at 05:17:04PM -0500, WingMan Kwok wrote:
 +static int keystone_usbphy_init(struct usb_phy *phy)
 +{
 +struct keystone_usbphy *k_phy = dev_get_drvdata(phy-dev);
 +u32 val;
 +
 +val  = keystone_usbphy_readl(k_phy-phy_ctrl, USB_PHY_CTL_CLOCK);
 +keystone_usbphy_writel(k_phy-phy_ctrl, USB_PHY_CTL_CLOCK,
 +val | PHY_REF_SSP_EN);
 
 you need to enable this device's clock to access its registers right ?
 
Nope.. This clock is always running for CFG block where the phy control
is residing.

 +udelay(20);
 
 why the magic 20 usecs ? Where does that come from ? Empirically found
 or is there a documentation reference ? At least add a comment there.
 
Above probably isn't needed either but good to check why was this added.
In refreshed patch, this can be either removed or a comment can be
added accordingly.

Thanks for spotting that.

Regards,
Santosh
--
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: [PATCH] usb: dwc3: omap: remove unnecessary lock

[Santosh Shilimkar]

On Saturday 07 December 2013 12:55 PM, Balbi, Felipe wrote:
 the lock was only taken inside the hardirq
 handler, which runs with IRQs disabled. There's
 no chance of any race condition happening, even
 on SMP machines. It's safe to remove that
 spinlock.
 
 Signed-off-by: Felipe Balbi ba...@ti.com
 ---
Acked-by: Santosh Shilimkar santosh.shilim...@ti.com

--
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: [PATCH v2 1/5] usb: dwc3: Add Keystone specific glue layer

[Santosh Shilimkar]

On Friday 06 December 2013 03:28 PM, Felipe Balbi wrote:
 Hi,
 
 On Wed, Dec 04, 2013 at 03:10:07PM -0500, WingMan Kwok wrote:
 Add Keystone platform specific glue layer to support
 USB3 Host mode.

 Cc: Santosh Shilimkar santosh.shilim...@ti.com
 Cc: Felipe Balbi ba...@ti.com
 Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
 Signed-off-by: WingMan Kwok w-kw...@ti.com
 ---
  drivers/usb/dwc3/Kconfig |7 ++
  drivers/usb/dwc3/Makefile|1 +
  drivers/usb/dwc3/dwc3-keystone.c |  210 
 ++
  3 files changed, 218 insertions(+)
  create mode 100644 drivers/usb/dwc3/dwc3-keystone.c

[..]

 +static irqreturn_t dwc3_keystone_interrupt(int irq, void *_kdwc)
 +{
 +struct dwc3_keystone*kdwc = _kdwc;
 +
 +spin_lock(kdwc-lock);
 
 Isn't this lock unnecessary ? This handler will run with IRQs disabled
 anyway.
 
Indeed.

 +kdwc3_writel(kdwc-usbss, USBSS_IRQENABLE_CLR_0, USBSS_IRQ_COREIRQ_CLR);
 +kdwc3_writel(kdwc-usbss, USBSS_IRQSTATUS_0, USBSS_IRQ_EVENT_ST);
 +kdwc3_writel(kdwc-usbss, USBSS_IRQENABLE_SET_0, USBSS_IRQ_COREIRQ_EN);
 +kdwc3_writel(kdwc-usbss, USBSS_IRQ_EOI, USBSS_IRQ_EOI_LINE(0));
 +spin_unlock(kdwc-lock);
 +
 +return IRQ_HANDLED;
 +}
 +
 +static int kdwc3_probe(struct platform_device *pdev)
 +{
 +struct device_node  *node = pdev-dev.of_node;
 +struct device   *dev = pdev-dev;
 
 if you invert these lines, it'll look a little nicer:
 
   struct device   *dev = pdev-dev;
   struct device_node  *node = dev-of_node;
 
 everything else looks pretty good, thanks
 
Looks good to me as well. With above update, 
Acked-by: Santosh Shilimkar santosh.shilim...@ti.com


--
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: [PATCH v2 2/5] usb: phy: Add keystone usb phy driver

[Santosh Shilimkar]

On Wednesday 04 December 2013 03:10 PM, WingMan Kwok wrote:
 Add Keystone platform USB PHY driver support. Current main purpose
 of this driver is to enable the PHY reference clock gate on the
 Keystone SoC. Otherwise it is a nop PHY.
 
 Cc: Santosh Shilimkar santosh.shilim...@ti.com
 Cc: Felipe Balbi ba...@ti.com
 Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
 Signed-off-by: WingMan Kwok w-kw...@ti.com
 ---
  drivers/usb/phy/Kconfig|   10 +++
  drivers/usb/phy/Makefile   |1 +
  drivers/usb/phy/phy-keystone.c |  142 
 
  3 files changed, 153 insertions(+)
  create mode 100644 drivers/usb/phy/phy-keystone.c
 
Acked-by: Santosh Shilimkar santosh.shilim...@ti.com

--
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: [PATCH v2 3/5] ARM: dts: keystone: Add usb phy devicetree bindings

[Santosh Shilimkar]

On Friday 06 December 2013 03:30 PM, Felipe Balbi wrote:
 On Wed, Dec 04, 2013 at 03:10:09PM -0500, WingMan Kwok wrote:
 Added device tree support for TI's Keystone USB PHY driver and updated the
 Documentation with device tree binding information.

 Cc: Santosh Shilimkar santosh.shilim...@ti.com
 Signed-off-by: WingMan Kwok w-kw...@ti.com
 ---
  .../devicetree/bindings/usb/keystone-phy.txt   |   19 
 +++
  arch/arm/boot/dts/keystone.dtsi|7 +++
  2 files changed, 26 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/usb/keystone-phy.txt

 diff --git a/Documentation/devicetree/bindings/usb/keystone-phy.txt 
 b/Documentation/devicetree/bindings/usb/keystone-phy.txt
 new file mode 100644
 index 000..300830d
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/usb/keystone-phy.txt
 @@ -0,0 +1,19 @@
 +TI Keystone USB PHY
 +
 +Required properties:
 + - compatible: should be ti,keystone-usbphy.
 + - #address-cells, #size-cells : should be '1' if the device has sub-nodes
 +   with 'reg' property.
 + - reg : Address and length of the usb phy control register set.
 +
 +The main purpose of this PHY driver is to enable the USB PHY reference clock
 +gate on the Keystone SOC for both the USB2 and USB3 PHY. Otherwise it is 
 just
 +an NOP PHY driver.  Hence this node is referenced as both the usb2 and usb3
 +phy node in the USB Glue layer driver node.
 +
 +usb_phy: usb_phy@2620738 {
 +compatible = ti,keystone-usbphy;
 +#address-cells = 1;
 +#size-cells = 1;
 +reg = 0x2620738 32;
 +};
 diff --git a/arch/arm/boot/dts/keystone.dtsi 
 b/arch/arm/boot/dts/keystone.dtsi
 index f6d6d9e..d497d9e 100644
 --- a/arch/arm/boot/dts/keystone.dtsi
 +++ b/arch/arm/boot/dts/keystone.dtsi
 @@ -181,5 +181,12 @@
  interrupts = GIC_SPI 300 IRQ_TYPE_EDGE_RISING;
  clocks = clkspi;
  };
 +
 +usb_phy: usb_phy@2620738 {
 +compatible = ti,keystone-usbphy;
 +#address-cells = 1;
 +#size-cells = 1;
 +reg = 0x2620738 32;
 
 should this one have status = disabled; and let board dts enable the
 PHY ?
 
Currently there is only one board but probably not a bad idea to enable
it from board dts. Lets do that

Regards,
Santosh 
--
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: [PATCH v2 0/5] Kesytone II USB support

[Santosh Shilimkar]

Wingman,

On Wednesday 04 December 2013 03:10 PM, WingMan Kwok wrote:
 Here is the updated version of the series which addresses comments from
 earlier version [1].  The PHY register programming is moved to a separate
 PHY driver.
 
 Series adds USB host support for Keystone SOCs. Keystone SOCs uses dwc3
 hardware IP implementation.  On Keystone II platforms, we use no-op phy 
 driver.
 
 All three patches are posted together just for completeness. Only first patch
 is expected to go via usb tree and other two via linux-keystone tree.

The series looks fine now and probably can be split now.
 
 Patchset are tested on Keystone II EVM with USB2.0 and USB3.0 flash drives.
 
 Cc: Santosh Shilimkar santosh.shilim...@ti.com
 
 WingMan Kwok (5):
   usb: dwc3: Add Keystone specific glue layer
   usb: phy: Add keystone usb phy driver
Please update the minor comments on patch 1 and resubmit above
two patches so that Felipe can pick these up. 

   ARM: dts: keystone: Add usb phy devicetree bindings
   ARM: dts: keystone: Add usb devicetree bindings
   ARM: keystone: defconfig: enable USB support
 
On the dts part just enable the phy,usb3 nodes from
board dts file and have status disabled in common dts file.
With those update, please repost them and I will take
them in my 3.14 queue.

Regards,
Santosh




--
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: [PATCH 1/3] usb: dwc3: Add Keystone specific glue layer

[Santosh Shilimkar]

On Wednesday 27 November 2013 12:41 PM, Felipe Balbi wrote:
 Hi,
 
 On Wed, Nov 27, 2013 at 02:49:54PM +0530, George Cherian wrote:
 +   error = of_platform_populate(node, NULL, NULL, dev);
 +   if (error) {
 +   dev_err(pdev-dev, failed to create dwc3 core\n);
 +   goto err_core;
 +   }
 +
 +   return 0;
 +
 +err_core:
 +   kdwc3_disable_irqs(kdwc);
 +err_irq:
 +   kdwc3_disable(kdwc);
 +
 +   return error;
 +}
 +
 +static int kdwc3_remove(struct platform_device *pdev)
 +{
 +   struct dwc3_keystone *kdwc = platform_get_drvdata(pdev);
 +
 +   if (kdwc) {
 +   kdwc3_disable_irqs(kdwc);
 +   kdwc3_disable(kdwc);
 +   platform_set_drvdata(pdev, NULL);
 +   }
 +   return 0;
 +}
 +

 You need to unregister the child nodes in remove.
 Also why can't the dwc3-omap driver be reused, Felipe??
 I believe the TI wrapper for Keystone is similar to that of AM437x or
 OMAP5.
 
 it is very similar indeed, if it can be easily re-use that glue, I'd
 rather not add another.
 
Initial idea was actually to use same driver but the integration IP
is quite different on Keystone vs OMAP which lead to have a separate
glue layer. They look similar but there are differences in terms of phys,
interrupts, register space. And also non OTG support, runtime PM
vs clock etc for now.

After all the glue is really a very small code describes the integration
details thanks to nice abstracted dwc3 core layer. So as discussed
over irc, keeping the separate glue is preferred but do let us know
if you think otherwise.

Regards,
Santosh

--
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: [PATCH 2/3] ARM: dts: keystone: Add usb devicetree bindings

[Santosh Shilimkar]

On Wednesday 27 November 2013 04:59 AM, George Cherian wrote:
 On 11/26/2013 1:46 AM, WingMan Kwok wrote:
 Added device tree support for TI's Keystone USB driver and updated the
 Documentation with device tree binding information.

 On Keystone II platforms, we use no-op phy driver.

 Cc: Santosh Shilimkar santosh.shilim...@ti.com
 Signed-off-by: WingMan Kwok w-kw...@ti.com
 ---
   .../devicetree/bindings/usb/keystone-usb.txt   |   43 
 
   arch/arm/boot/dts/keystone.dtsi|   27 
   2 files changed, 70 insertions(+)
   create mode 100644 Documentation/devicetree/bindings/usb/keystone-usb.txt

[...]

 diff --git a/arch/arm/boot/dts/keystone.dtsi 
 b/arch/arm/boot/dts/keystone.dtsi
 index f6d6d9e..1e1049c 100644
 --- a/arch/arm/boot/dts/keystone.dtsi
 +++ b/arch/arm/boot/dts/keystone.dtsi
 @@ -181,5 +181,32 @@
   interrupts = GIC_SPI 300 IRQ_TYPE_EDGE_RISING;
   clocks = clkspi;
   };
 +
 +usb2_phy: usb2_phy {
 +compatible = usb-nop-xceiv;
 +};
 +
 +usb3_phy: usb3_phy {
 +compatible = usb-nop-xceiv;
 +};
 +
 +usb: usb@268 {
 +compatible = ti,keystone-dwc3;
 +#address-cells = 1;
 +#size-cells = 1;
 +reg = 0x268 0x1
 +   0x2620738 32;
 +clocks = clkusb;
 +clock-names = usb;
 +interrupts = GIC_SPI 393 IRQ_TYPE_EDGE_RISING;
 
 You don't have seperate interrrupt for wrapper and core?
 Is it the same interrupt shared between XHCI,DWC3 and wrapper?

You don't need actually two seperate interrupts.
DWC3 core actually registers IRQ for XHCI. And in OMAP case, there
is one more IRQ in wrapper. After checking with Felipe, it seems
the OMAP wrapper interrupt was more for debug purpose than any real
use.

On Keystone only one IRQ is used and the handling is managed
through IRQF_SHARED and that is also mainly because the IRQ
ack needs special write to EOI register unlike OMAP.

Regards,
Santosh
 


--
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: [PATCH 1/3] usb: dwc3: Add Keystone specific glue layer

[Santosh Shilimkar]

On Tuesday 26 November 2013 12:16 PM, Felipe Balbi wrote:
 On Mon, Nov 25, 2013 at 07:49:01PM -0500, Santosh Shilimkar wrote:
 On Monday 25 November 2013 03:39 PM, Felipe Balbi wrote:
 Hi,


 [...]


 +  kdwc3_dma_mask = dma_get_mask(dev);
 +  dev-dma_mask = kdwc3_dma_mask;
 +
 +  error = kdwc3_enable(kdwc);

 I would drop this function and just add your clk_prepare() here, then
 move clk_enable()/clk_disable() to -runtime_resume/-runtime_suspend()
 respectively. Then you could just call pm_runtime_get_sync() when you
 need to access your registers and pm_runtime_put() when you want to drop
 the clock reference.

 this will even make PM implementation a lot easier for you going
 forward.

 Just to make the PM runtime part clear, there are few issues with PM
 core clock layer [1], hence drivers is using clock layer. Its trivial
 to update the driver though once the issue is sorted out.

 Meanwhile driver development continue to be with clock calls. 
 
 I don't mind having those clock calls, just suggested a different
 placement for them.
 
I got that part. Just wanted to clear the runtime PM part.

Regards,
Santosh
--
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: [PATCH 2/3] ARM: dts: keystone: Add usb devicetree bindings

[Santosh Shilimkar]

On Monday 25 November 2013 03:42 PM, Felipe Balbi wrote:
 Hi,
 
 On Mon, Nov 25, 2013 at 03:16:20PM -0500, WingMan Kwok wrote:
 Added device tree support for TI's Keystone USB driver and updated the
 Documentation with device tree binding information.

 On Keystone II platforms, we use no-op phy driver.

 Cc: Santosh Shilimkar santosh.shilim...@ti.com
 Signed-off-by: WingMan Kwok w-kw...@ti.com
 ---

[...]

 BTW, some preliminary TRM coming my way would be cool, so I can better
 understand how this HW behaves. A board would also go a long way, so I
 could test this myself (we are part of the same company anyway).
 
TRM documentation is bit different. There is no single TRM which has all
the information but different chapters called user guides. For Keystone
USB, the user guide isn't ready yet but there is an internal version for
which we will send you a link. 

Boards are hard to get for now but we can see next year.

Regards,
Santosh


--
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: [PATCH 1/3] usb: dwc3: Add Keystone specific glue layer

[Santosh Shilimkar]

On Monday 25 November 2013 03:39 PM, Felipe Balbi wrote:
 Hi,
 

[...]

 
 +kdwc3_dma_mask = dma_get_mask(dev);
 +dev-dma_mask = kdwc3_dma_mask;
 +
 +error = kdwc3_enable(kdwc);
 
 I would drop this function and just add your clk_prepare() here, then
 move clk_enable()/clk_disable() to -runtime_resume/-runtime_suspend()
 respectively. Then you could just call pm_runtime_get_sync() when you
 need to access your registers and pm_runtime_put() when you want to drop
 the clock reference.
 
 this will even make PM implementation a lot easier for you going
 forward.
 
Just to make the PM runtime part clear, there are few issues with PM
core clock layer [1], hence drivers is using clock layer. Its trivial
to update the driver though once the issue is sorted out.

Meanwhile driver development continue to be with clock calls. 

Regards,
Santosh

[1] https://groups.google.com/forum/#!msg/linux.kernel/w5gFxFsIK-c/jYcnRET_EdAJ
--
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