Re: [PATCH v4 4/4] usb: dwc2: refactor common low-level hw code to platform.c

[Felipe Balbi]

John Youn <john.y...@synopsys.com> writes:

Hi,

> On 10/2/2015 12:45 AM, Marek Szyprowski wrote:
>> DWC2 module on some platforms needs three additional hardware
>> resources: phy controller, clock and power supply. All of them must be
>> enabled/activated to properly initialize and operate. This was initially
>> handled in s3c-hsotg driver, which has been converted to 'gadget' part
>> of dwc2 driver. Unfortunately, not all of this code got moved to common
>> platform code, what resulted in accessing DWC2 registers without
>> enabling low-level hardware resources. This fails for example on Exynos
>> SoCs. This patch moves all the code for managing those resources to
>> common platform.c file and provides convenient wrappers for controlling
>> them.
>> 
>> Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
>> ---
>> Changelog:
>> v4:
>> - fixed broken conditional compilation and adjusted comments in dwc2_hsotg
>>   structure documentation
>> 
>> v3:
>> - rebased onto latest 'testing/next' from Felipe Balbi (includes
>>   s3c_hsotg -> dwc2 rename)
>> 
>> v2:
>> - moved setting of ll_hw_enabled flag to enable/disable functions,
>>   as suggested by John Youn
>> - moved setting of phy width to dwc2_lowlevel_init function
>> ---
>>  drivers/usb/dwc2/core.h |  24 +++--
>>  drivers/usb/dwc2/gadget.c   | 193 
>>  drivers/usb/dwc2/platform.c | 234 
>> +---
>>  3 files changed, 228 insertions(+), 223 deletions(-)
>> 
>
> Hi Marek,
>
> I still see lockdep warnings.
>
> Any ideas about these?
>
>
> [ 1618.179611] ==
> [ 1618.179612] [ INFO: possible circular locking dependency detected ]
> [ 1618.179613] 4.3.0-rc3-snps-00125-g744fd93 #28 Not tainted
> [ 1618.179614] ---
> [ 1618.179615] modprobe/2658 is trying to acquire lock:
> [ 1618.179616]  (>init_mutex){+.+.+.}, at: [] 
> dwc2_hsotg_udc_start+0x5c/0x200 [dwc2]
> [ 1618.179622] 
> [ 1618.179622] but task is already holding lock:
> [ 1618.179623]  (udc_lock){+.+.+.}, at: [] 
> usb_gadget_probe_driver+0x3a/0xd0 [udc_core]
> [ 1618.179627] 
> [ 1618.179627] which lock already depends on the new lock.
> [ 1618.179627] 
> [ 1618.179628] 
> [ 1618.179628] the existing dependency chain (in reverse order) is:
> [ 1618.179629] 
> [ 1618.179629] -> #1 (udc_lock){+.+.+.}:
> [ 1618.179631][] lock_acquire+0xdd/0x1f0
> [ 1618.179635][] mutex_lock_nested+0x76/0x3e0
> [ 1618.179638][] 
> usb_add_gadget_udc_release+0x187/0x240 [udc_core]
> [ 1618.179640][] usb_add_gadget_udc+0x10/0x20 
> [udc_core]
> [ 1618.179642][] dwc2_gadget_init+0x47c/0x580 [dwc2]
> [ 1618.179645][] dwc2_driver_probe+0x422/0x4b0 
> [dwc2]
> [ 1618.179648][] platform_drv_probe+0x34/0x90
> [ 1618.179650][] driver_probe_device+0x224/0x480
> [ 1618.179652][] __device_attach_driver+0x71/0xa0
> [ 1618.179654][] bus_for_each_drv+0x5d/0x90
> [ 1618.179655][] __device_attach+0xbf/0x140
> [ 1618.179657][] device_initial_probe+0x13/0x20
> [ 1618.179658][] bus_probe_device+0xa3/0xb0
> [ 1618.179660][] device_add+0x40d/0x690
> [ 1618.179661][] platform_device_add+0x111/0x270
> [ 1618.179663][] dwc2_pci_probe+0xe8/0x1d2 
> [dwc2_pci]
> [ 1618.179665][] local_pci_probe+0x45/0xa0
> [ 1618.179668][] pci_device_probe+0xe1/0x130
> [ 1618.179669][] driver_probe_device+0x224/0x480
> [ 1618.179671][] __driver_attach+0x88/0x90
> [ 1618.179672][] bus_for_each_dev+0x66/0xa0
> [ 1618.179674][] driver_attach+0x1e/0x20
> [ 1618.179675][] bus_add_driver+0x1ee/0x280
> [ 1618.179677][] driver_register+0x60/0xe0
> [ 1618.179678][] __pci_register_driver+0x60/0x70
> [ 1618.179680][] 0xc000601e
> [ 1618.179681][] do_one_initcall+0xb3/0x200
> [ 1618.179684][] do_init_module+0x5f/0x1e7
> [ 1618.179687][] load_module+0x21a8/0x2840
> [ 1618.179689][] SyS_finit_module+0x9a/0xc0
> [ 1618.179691][] entry_SYSCALL_64_fastpath+0x16/0x7a
> [ 1618.179693] 
> [ 1618.179693] -> #0 (>init_mutex){+.+.+.}:
> [ 1618.179695][] __lock_acquire+0x1d35/0x1db0
> [ 1618.179697][] lock_acquire+0xdd/0x1f0
> [ 1618.179698][] mutex_lock_nested+0x76/0x3e0
> [ 1618.179700][] dwc2_hsotg_udc_start+0x5c/0x200 
> [dwc2]
> [ 1618.179703][] udc_bind_to_driver+0xa4/0x100 

Re: [PATCH v3 4/4] usb: dwc2: refactor common low-level hw code to platform.c

[Felipe Balbi]

On Mon, Sep 21, 2015 at 12:16:12PM +0200, Marek Szyprowski wrote:
> DWC2 module on some platforms needs three additional hardware
> resources: phy controller, clock and power supply. All of them must be
> enabled/activated to properly initialize and operate. This was initially
> handled in s3c-hsotg driver, which has been converted to 'gadget' part
> of dwc2 driver. Unfortunately, not all of this code got moved to common
> platform code, what resulted in accessing DWC2 registers without
> enabling low-level hardware resources. This fails for example on Exynos
> SoCs. This patch moves all the code for managing those resources to
> common platform.c file and provides convenient wrappers for controlling
> them.
> 
> Signed-off-by: Marek Szyprowski 

I just caught several build errors which this patch. I hope you can
send me a follow-up fix (which I can amend to $subject) otherwise
I'll have to drop this series

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v3 4/4] usb: dwc2: refactor common low-level hw code to platform.c

[Felipe Balbi]

On Thu, Oct 01, 2015 at 09:04:59PM +, John Youn wrote:
> On 10/1/2015 8:50 AM, Felipe Balbi wrote:
> > On Mon, Sep 21, 2015 at 12:16:12PM +0200, Marek Szyprowski wrote:
> >> DWC2 module on some platforms needs three additional hardware
> >> resources: phy controller, clock and power supply. All of them must be
> >> enabled/activated to properly initialize and operate. This was initially
> >> handled in s3c-hsotg driver, which has been converted to 'gadget' part
> >> of dwc2 driver. Unfortunately, not all of this code got moved to common
> >> platform code, what resulted in accessing DWC2 registers without
> >> enabling low-level hardware resources. This fails for example on Exynos
> >> SoCs. This patch moves all the code for managing those resources to
> >> common platform.c file and provides convenient wrappers for controlling
> >> them.
> >>
> >> Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
> > 
> > I just caught several build errors which this patch. I hope you can
> > send me a follow-up fix (which I can amend to $subject) otherwise
> > I'll have to drop this series
> > 
> 
> I forgot that this was initially part of a larger
> patch-set. Maybe that is causing issues? If this wasn't intended
> to go through Felipe's tree then my bad.
> 
> Also, I noticed this patch causes deadlock warnings with lockdep
> enabled.
> 
> Can you look into that also while you investigate this?

John, do you want me to drop the series meanwhile ? I can do that no problems.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v3 4/4] usb: dwc2: refactor common low-level hw code to platform.c

[Felipe Balbi]

On Thu, Oct 01, 2015 at 10:21:22PM +, John Youn wrote:
> On 10/1/2015 3:04 PM, Felipe Balbi wrote:
> > On Thu, Oct 01, 2015 at 09:04:59PM +, John Youn wrote:
> >> On 10/1/2015 8:50 AM, Felipe Balbi wrote:
> >>> On Mon, Sep 21, 2015 at 12:16:12PM +0200, Marek Szyprowski wrote:
> >>>> DWC2 module on some platforms needs three additional hardware
> >>>> resources: phy controller, clock and power supply. All of them must be
> >>>> enabled/activated to properly initialize and operate. This was initially
> >>>> handled in s3c-hsotg driver, which has been converted to 'gadget' part
> >>>> of dwc2 driver. Unfortunately, not all of this code got moved to common
> >>>> platform code, what resulted in accessing DWC2 registers without
> >>>> enabling low-level hardware resources. This fails for example on Exynos
> >>>> SoCs. This patch moves all the code for managing those resources to
> >>>> common platform.c file and provides convenient wrappers for controlling
> >>>> them.
> >>>>
> >>>> Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com>
> >>>
> >>> I just caught several build errors which this patch. I hope you can
> >>> send me a follow-up fix (which I can amend to $subject) otherwise
> >>> I'll have to drop this series
> >>>
> >>
> >> I forgot that this was initially part of a larger
> >> patch-set. Maybe that is causing issues? If this wasn't intended
> >> to go through Felipe's tree then my bad.
> >>
> >> Also, I noticed this patch causes deadlock warnings with lockdep
> >> enabled.
> >>
> >> Can you look into that also while you investigate this?
> > 
> > John, do you want me to drop the series meanwhile ? I can do that no 
> > problems.
> > 
> 
> You can drop this one. I think leaving in patch 1-3 is fine as
> they are small cleanup patches. Unless you or Marek prefer to
> drop all of them.

dropped this one for now. Thanks

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCHv3 1/4] phy: phy-core: Make GENERIC_PHY an invisible option

[Felipe Balbi]

On Mon, Jun 01, 2015 at 06:22:41PM +0530, Kishon Vijay Abraham I wrote:
 Hi,
 
 On Friday 29 May 2015 08:34 PM, Felipe Balbi wrote:
 Hi,
 
 On Fri, May 29, 2015 at 05:04:38PM +0530, Kishon Vijay Abraham I wrote:
 Hi Felipe,
 
 On Wednesday 27 May 2015 12:09 AM, Felipe Balbi wrote:
 On Tue, May 26, 2015 at 11:37:17AM -0700, Arun Ramamurthy wrote:
 Hi
 
 On 15-05-26 07:19 AM, Felipe Balbi wrote:
 HI,
 
 On Mon, May 25, 2015 at 02:19:58PM -0700, Arun Ramamurthy wrote:
 
 
 On 15-05-14 05:52 PM, Felipe Balbi wrote:
 Hi,
 
 On Wed, Apr 22, 2015 at 04:04:10PM -0700, Arun Ramamurthy wrote:
 Most of the phy providers use select to enable GENERIC_PHY. Since 
 select
 is only recommended when the config is not visible, GENERIC_PHY is 
 changed
 an invisible option. To maintain consistency, all phy providers are 
 changed
 to select GENERIC_PHY and all non-phy drivers use depends on when 
 the
 phy framework is explicity required. USB_MUSB_OMAP2PLUS has a cyclic
 dependency, so it is left as select.
 
 Signed-off-by: Arun Ramamurthy arun.ramamur...@broadcom.com
 ---
   drivers/ata/Kconfig   | 1 -
   drivers/media/platform/exynos4-is/Kconfig | 2 +-
   drivers/phy/Kconfig   | 4 ++--
   drivers/usb/host/Kconfig  | 4 ++--
   drivers/video/fbdev/exynos/Kconfig| 2 +-
   5 files changed, 6 insertions(+), 7 deletions(-)
 
 diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
 index 5f60155..6d2e881 100644
 --- a/drivers/ata/Kconfig
 +++ b/drivers/ata/Kconfig
 @@ -301,7 +301,6 @@ config SATA_MV
   tristate Marvell SATA support
   depends on PCI || ARCH_DOVE || ARCH_MV78XX0 || \
  ARCH_MVEBU || ARCH_ORION5X || COMPILE_TEST
 - select GENERIC_PHY
   help
 This option enables support for the Marvell Serial ATA family.
 Currently supports 88SX[56]0[48][01] PCI(-X) chips,
 diff --git a/drivers/media/platform/exynos4-is/Kconfig 
 b/drivers/media/platform/exynos4-is/Kconfig
 index b7b2e47..b6f3eaa 100644
 --- a/drivers/media/platform/exynos4-is/Kconfig
 +++ b/drivers/media/platform/exynos4-is/Kconfig
 @@ -31,7 +31,7 @@ config VIDEO_S5P_FIMC
   config VIDEO_S5P_MIPI_CSIS
   tristate S5P/EXYNOS MIPI-CSI2 receiver (MIPI-CSIS) driver
   depends on REGULATOR
 - select GENERIC_PHY
 + depends on GENERIC_PHY
   help
 This is a V4L2 driver for Samsung S5P and EXYNOS4 SoC 
  MIPI-CSI2
 receiver (MIPI-CSIS) devices.
 diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
 index 2962de2..edecdb1 100644
 --- a/drivers/phy/Kconfig
 +++ b/drivers/phy/Kconfig
 @@ -5,7 +5,7 @@
   menu PHY Subsystem
 
   config GENERIC_PHY
 - bool PHY Core
 + bool
   help
 Generic PHY support.
 
 @@ -72,7 +72,7 @@ config PHY_MIPHY365X
   config PHY_RCAR_GEN2
   tristate Renesas R-Car generation 2 USB PHY driver
   depends on ARCH_SHMOBILE
 - depends on GENERIC_PHY
 + select GENERIC_PHY
 
 so some you changed from depends to select...
 
   help
 Support for USB PHY found on Renesas R-Car generation 2 SoCs.
 
 diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
 index 5ad60e4..e2197e2 100644
 --- a/drivers/usb/host/Kconfig
 +++ b/drivers/usb/host/Kconfig
 @@ -182,7 +182,7 @@ config USB_EHCI_HCD_SPEAR
   config USB_EHCI_HCD_STI
   tristate Support for ST STiHxxx on-chip EHCI USB controller
   depends on ARCH_STI  OF
 - select GENERIC_PHY
 + depends on GENERIC_PHY
 
 while others you changed from select to depends.
 
 NAK.
 
 Felipe, I dont understand your concern, could you please explain it more
 detail?  The logic behind the changes is that in cases where there was 
 an
 explicit dependency, I changed it to depends on and in other cases I
 changed it to selects. Thanks
 
 Since GENERIC_PHY is visible from Kconfig, it would be much nicer to
 avoid select altogether.
 
 Felipe, after discussion with the maintainers, I have made GENERIC_PHY an
 invisible option as part of this change. Thanks
 
 Then, if the option is invisible, how can you depend on it ? It can
 never be selected by poking around in Kconfig. IMO, it's
 counterintuitive that you need to enable a PHY driver before you can see
 your EHCI/OHCI/whatever controller listed in Kconfig.
 
 If the controller requires PHY for it to be functional, it is okay to make
 the controller depend on PHY IMHO. We want to try and minimize the usage of
 'select' wherever possible or else 'select' is the most intuitive way. The
 other option is just to leave the 'depends on' and let the user select PHY.
 
 How can you 'depend' on something that the user can't select by
 navigating through Kconfig ?
 
 hmm... Actually it's selected when the user selects the PHY driver.

that's my point, don't you think it's a little counter-intuitive ?

 Maybe we should directly depend on the PHY driver instead of Generic
 PHY?

maybe... But then what do you do when you have different boards using
different PHYs

Re: [PATCHv3 1/4] phy: phy-core: Make GENERIC_PHY an invisible option

[Felipe Balbi]

Hi,

On Fri, May 29, 2015 at 05:04:38PM +0530, Kishon Vijay Abraham I wrote:
 Hi Felipe,
 
 On Wednesday 27 May 2015 12:09 AM, Felipe Balbi wrote:
 On Tue, May 26, 2015 at 11:37:17AM -0700, Arun Ramamurthy wrote:
 Hi
 
 On 15-05-26 07:19 AM, Felipe Balbi wrote:
 HI,
 
 On Mon, May 25, 2015 at 02:19:58PM -0700, Arun Ramamurthy wrote:
 
 
 On 15-05-14 05:52 PM, Felipe Balbi wrote:
 Hi,
 
 On Wed, Apr 22, 2015 at 04:04:10PM -0700, Arun Ramamurthy wrote:
 Most of the phy providers use select to enable GENERIC_PHY. Since 
 select
 is only recommended when the config is not visible, GENERIC_PHY is 
 changed
 an invisible option. To maintain consistency, all phy providers are 
 changed
 to select GENERIC_PHY and all non-phy drivers use depends on when 
 the
 phy framework is explicity required. USB_MUSB_OMAP2PLUS has a cyclic
 dependency, so it is left as select.
 
 Signed-off-by: Arun Ramamurthy arun.ramamur...@broadcom.com
 ---
   drivers/ata/Kconfig   | 1 -
   drivers/media/platform/exynos4-is/Kconfig | 2 +-
   drivers/phy/Kconfig   | 4 ++--
   drivers/usb/host/Kconfig  | 4 ++--
   drivers/video/fbdev/exynos/Kconfig| 2 +-
   5 files changed, 6 insertions(+), 7 deletions(-)
 
 diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
 index 5f60155..6d2e881 100644
 --- a/drivers/ata/Kconfig
 +++ b/drivers/ata/Kconfig
 @@ -301,7 +301,6 @@ config SATA_MV
 tristate Marvell SATA support
 depends on PCI || ARCH_DOVE || ARCH_MV78XX0 || \
ARCH_MVEBU || ARCH_ORION5X || COMPILE_TEST
 -   select GENERIC_PHY
 help
   This option enables support for the Marvell Serial ATA family.
   Currently supports 88SX[56]0[48][01] PCI(-X) chips,
 diff --git a/drivers/media/platform/exynos4-is/Kconfig 
 b/drivers/media/platform/exynos4-is/Kconfig
 index b7b2e47..b6f3eaa 100644
 --- a/drivers/media/platform/exynos4-is/Kconfig
 +++ b/drivers/media/platform/exynos4-is/Kconfig
 @@ -31,7 +31,7 @@ config VIDEO_S5P_FIMC
   config VIDEO_S5P_MIPI_CSIS
 tristate S5P/EXYNOS MIPI-CSI2 receiver (MIPI-CSIS) driver
 depends on REGULATOR
 -   select GENERIC_PHY
 +   depends on GENERIC_PHY
 help
   This is a V4L2 driver for Samsung S5P and EXYNOS4 SoC 
  MIPI-CSI2
   receiver (MIPI-CSIS) devices.
 diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
 index 2962de2..edecdb1 100644
 --- a/drivers/phy/Kconfig
 +++ b/drivers/phy/Kconfig
 @@ -5,7 +5,7 @@
   menu PHY Subsystem
 
   config GENERIC_PHY
 -   bool PHY Core
 +   bool
 help
   Generic PHY support.
 
 @@ -72,7 +72,7 @@ config PHY_MIPHY365X
   config PHY_RCAR_GEN2
 tristate Renesas R-Car generation 2 USB PHY driver
 depends on ARCH_SHMOBILE
 -   depends on GENERIC_PHY
 +   select GENERIC_PHY
 
 so some you changed from depends to select...
 
 help
   Support for USB PHY found on Renesas R-Car generation 2 SoCs.
 
 diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
 index 5ad60e4..e2197e2 100644
 --- a/drivers/usb/host/Kconfig
 +++ b/drivers/usb/host/Kconfig
 @@ -182,7 +182,7 @@ config USB_EHCI_HCD_SPEAR
   config USB_EHCI_HCD_STI
 tristate Support for ST STiHxxx on-chip EHCI USB controller
 depends on ARCH_STI  OF
 -   select GENERIC_PHY
 +   depends on GENERIC_PHY
 
 while others you changed from select to depends.
 
 NAK.
 
 Felipe, I dont understand your concern, could you please explain it more
 detail?  The logic behind the changes is that in cases where there was an
 explicit dependency, I changed it to depends on and in other cases I
 changed it to selects. Thanks
 
 Since GENERIC_PHY is visible from Kconfig, it would be much nicer to
 avoid select altogether.
 
 Felipe, after discussion with the maintainers, I have made GENERIC_PHY an
 invisible option as part of this change. Thanks
 
 Then, if the option is invisible, how can you depend on it ? It can
 never be selected by poking around in Kconfig. IMO, it's
 counterintuitive that you need to enable a PHY driver before you can see
 your EHCI/OHCI/whatever controller listed in Kconfig.
 
 If the controller requires PHY for it to be functional, it is okay to make
 the controller depend on PHY IMHO. We want to try and minimize the usage of
 'select' wherever possible or else 'select' is the most intuitive way. The
 other option is just to leave the 'depends on' and let the user select PHY.

How can you 'depend' on something that the user can't select by
navigating through Kconfig ?

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCHv3 1/4] phy: phy-core: Make GENERIC_PHY an invisible option

[Felipe Balbi]

On Tue, May 26, 2015 at 11:37:17AM -0700, Arun Ramamurthy wrote:
 Hi
 
 On 15-05-26 07:19 AM, Felipe Balbi wrote:
 HI,
 
 On Mon, May 25, 2015 at 02:19:58PM -0700, Arun Ramamurthy wrote:
 
 
 On 15-05-14 05:52 PM, Felipe Balbi wrote:
 Hi,
 
 On Wed, Apr 22, 2015 at 04:04:10PM -0700, Arun Ramamurthy wrote:
 Most of the phy providers use select to enable GENERIC_PHY. Since select
 is only recommended when the config is not visible, GENERIC_PHY is changed
 an invisible option. To maintain consistency, all phy providers are 
 changed
 to select GENERIC_PHY and all non-phy drivers use depends on when the
 phy framework is explicity required. USB_MUSB_OMAP2PLUS has a cyclic
 dependency, so it is left as select.
 
 Signed-off-by: Arun Ramamurthy arun.ramamur...@broadcom.com
 ---
   drivers/ata/Kconfig   | 1 -
   drivers/media/platform/exynos4-is/Kconfig | 2 +-
   drivers/phy/Kconfig   | 4 ++--
   drivers/usb/host/Kconfig  | 4 ++--
   drivers/video/fbdev/exynos/Kconfig| 2 +-
   5 files changed, 6 insertions(+), 7 deletions(-)
 
 diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
 index 5f60155..6d2e881 100644
 --- a/drivers/ata/Kconfig
 +++ b/drivers/ata/Kconfig
 @@ -301,7 +301,6 @@ config SATA_MV
   tristate Marvell SATA support
   depends on PCI || ARCH_DOVE || ARCH_MV78XX0 || \
  ARCH_MVEBU || ARCH_ORION5X || COMPILE_TEST
 - select GENERIC_PHY
   help
 This option enables support for the Marvell Serial ATA family.
 Currently supports 88SX[56]0[48][01] PCI(-X) chips,
 diff --git a/drivers/media/platform/exynos4-is/Kconfig 
 b/drivers/media/platform/exynos4-is/Kconfig
 index b7b2e47..b6f3eaa 100644
 --- a/drivers/media/platform/exynos4-is/Kconfig
 +++ b/drivers/media/platform/exynos4-is/Kconfig
 @@ -31,7 +31,7 @@ config VIDEO_S5P_FIMC
   config VIDEO_S5P_MIPI_CSIS
   tristate S5P/EXYNOS MIPI-CSI2 receiver (MIPI-CSIS) driver
   depends on REGULATOR
 - select GENERIC_PHY
 + depends on GENERIC_PHY
   help
 This is a V4L2 driver for Samsung S5P and EXYNOS4 SoC 
  MIPI-CSI2
 receiver (MIPI-CSIS) devices.
 diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
 index 2962de2..edecdb1 100644
 --- a/drivers/phy/Kconfig
 +++ b/drivers/phy/Kconfig
 @@ -5,7 +5,7 @@
   menu PHY Subsystem
 
   config GENERIC_PHY
 - bool PHY Core
 + bool
   help
 Generic PHY support.
 
 @@ -72,7 +72,7 @@ config PHY_MIPHY365X
   config PHY_RCAR_GEN2
   tristate Renesas R-Car generation 2 USB PHY driver
   depends on ARCH_SHMOBILE
 - depends on GENERIC_PHY
 + select GENERIC_PHY
 
 so some you changed from depends to select...
 
   help
 Support for USB PHY found on Renesas R-Car generation 2 SoCs.
 
 diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
 index 5ad60e4..e2197e2 100644
 --- a/drivers/usb/host/Kconfig
 +++ b/drivers/usb/host/Kconfig
 @@ -182,7 +182,7 @@ config USB_EHCI_HCD_SPEAR
   config USB_EHCI_HCD_STI
   tristate Support for ST STiHxxx on-chip EHCI USB controller
   depends on ARCH_STI  OF
 - select GENERIC_PHY
 + depends on GENERIC_PHY
 
 while others you changed from select to depends.
 
 NAK.
 
 Felipe, I dont understand your concern, could you please explain it more
 detail?  The logic behind the changes is that in cases where there was an
 explicit dependency, I changed it to depends on and in other cases I
 changed it to selects. Thanks
 
 Since GENERIC_PHY is visible from Kconfig, it would be much nicer to
 avoid select altogether.
 
 Felipe, after discussion with the maintainers, I have made GENERIC_PHY an
 invisible option as part of this change. Thanks

Then, if the option is invisible, how can you depend on it ? It can
never be selected by poking around in Kconfig. IMO, it's
counterintuitive that you need to enable a PHY driver before you can see
your EHCI/OHCI/whatever controller listed in Kconfig.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCHv3 1/4] phy: phy-core: Make GENERIC_PHY an invisible option

[Felipe Balbi]

HI,

On Mon, May 25, 2015 at 02:19:58PM -0700, Arun Ramamurthy wrote:
 
 
 On 15-05-14 05:52 PM, Felipe Balbi wrote:
 Hi,
 
 On Wed, Apr 22, 2015 at 04:04:10PM -0700, Arun Ramamurthy wrote:
 Most of the phy providers use select to enable GENERIC_PHY. Since select
 is only recommended when the config is not visible, GENERIC_PHY is changed
 an invisible option. To maintain consistency, all phy providers are changed
 to select GENERIC_PHY and all non-phy drivers use depends on when the
 phy framework is explicity required. USB_MUSB_OMAP2PLUS has a cyclic
 dependency, so it is left as select.
 
 Signed-off-by: Arun Ramamurthy arun.ramamur...@broadcom.com
 ---
   drivers/ata/Kconfig   | 1 -
   drivers/media/platform/exynos4-is/Kconfig | 2 +-
   drivers/phy/Kconfig   | 4 ++--
   drivers/usb/host/Kconfig  | 4 ++--
   drivers/video/fbdev/exynos/Kconfig| 2 +-
   5 files changed, 6 insertions(+), 7 deletions(-)
 
 diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
 index 5f60155..6d2e881 100644
 --- a/drivers/ata/Kconfig
 +++ b/drivers/ata/Kconfig
 @@ -301,7 +301,6 @@ config SATA_MV
 tristate Marvell SATA support
 depends on PCI || ARCH_DOVE || ARCH_MV78XX0 || \
ARCH_MVEBU || ARCH_ORION5X || COMPILE_TEST
 -   select GENERIC_PHY
 help
   This option enables support for the Marvell Serial ATA family.
   Currently supports 88SX[56]0[48][01] PCI(-X) chips,
 diff --git a/drivers/media/platform/exynos4-is/Kconfig 
 b/drivers/media/platform/exynos4-is/Kconfig
 index b7b2e47..b6f3eaa 100644
 --- a/drivers/media/platform/exynos4-is/Kconfig
 +++ b/drivers/media/platform/exynos4-is/Kconfig
 @@ -31,7 +31,7 @@ config VIDEO_S5P_FIMC
   config VIDEO_S5P_MIPI_CSIS
 tristate S5P/EXYNOS MIPI-CSI2 receiver (MIPI-CSIS) driver
 depends on REGULATOR
 -   select GENERIC_PHY
 +   depends on GENERIC_PHY
 help
   This is a V4L2 driver for Samsung S5P and EXYNOS4 SoC MIPI-CSI2
   receiver (MIPI-CSIS) devices.
 diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
 index 2962de2..edecdb1 100644
 --- a/drivers/phy/Kconfig
 +++ b/drivers/phy/Kconfig
 @@ -5,7 +5,7 @@
   menu PHY Subsystem
 
   config GENERIC_PHY
 -   bool PHY Core
 +   bool
 help
   Generic PHY support.
 
 @@ -72,7 +72,7 @@ config PHY_MIPHY365X
   config PHY_RCAR_GEN2
 tristate Renesas R-Car generation 2 USB PHY driver
 depends on ARCH_SHMOBILE
 -   depends on GENERIC_PHY
 +   select GENERIC_PHY
 
 so some you changed from depends to select...
 
 help
   Support for USB PHY found on Renesas R-Car generation 2 SoCs.
 
 diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
 index 5ad60e4..e2197e2 100644
 --- a/drivers/usb/host/Kconfig
 +++ b/drivers/usb/host/Kconfig
 @@ -182,7 +182,7 @@ config USB_EHCI_HCD_SPEAR
   config USB_EHCI_HCD_STI
 tristate Support for ST STiHxxx on-chip EHCI USB controller
 depends on ARCH_STI  OF
 -   select GENERIC_PHY
 +   depends on GENERIC_PHY
 
 while others you changed from select to depends.
 
 NAK.
 
 Felipe, I dont understand your concern, could you please explain it more
 detail?  The logic behind the changes is that in cases where there was an
 explicit dependency, I changed it to depends on and in other cases I
 changed it to selects. Thanks

Since GENERIC_PHY is visible from Kconfig, it would be much nicer to
avoid select altogether.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 00/11] Exynos7: Adding USB 3.0 support

[Felipe Balbi]

On Fri, Nov 21, 2014 at 07:05:43PM +0530, Vivek Gautam wrote:
 The series has dependency on
 a) [PATCH v7 0/7] Enable support for Samsung Exynos7 SoC
http://www.spinics.net/lists/linux-samsung-soc/msg38734.html
 b) [GIT PULL] Samsung clock changes for 3.19 - specifically the clock dt
bindings header.
http://comments.gmane.org/gmane.linux.kernel.samsung-soc/39142
 c) tty: serial: samsung: Clean-up selection of number of available UARTs
http://www.spinics.net/lists/linux-samsung-soc/msg37418.html
 d) dts, kbuild: Implement support for dtb vendor subdirs(merged in 
 linux-next)
https://lkml.org/lkml/2014/10/21/654
 e) Samsung pinctrl patches for v3.19
http://www.spinics.net/lists/linux-samsung-soc/msg38744.html
 
 Tested on Exynos7-espresso board with 3.18-rc5 and above dependencies.
 
 Clubbing the pinctrl, clk, and usb driver changes alongwith the dt changes
 together in this series only so as to avoid having 'n' number of dependencies.
 
 The USB driver patches in this series were part of [1] sent earlier.
 [1] [PATCH v2 0/4] usb: dwc3/phy-exynos5-usbdrd: Extend support to Exynos7
 https://lkml.org/lkml/2014/10/7/191

I took dwc3 driver patches.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls

[Felipe Balbi]

On Fri, Oct 31, 2014 at 11:12:33AM +0100, Marek Szyprowski wrote:
 This patch adds mutex, which protects initialization and
 deinitialization procedures against suspend/resume methods.
 
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com

doesn't apply either:

checking file drivers/usb/dwc2/core.h
Hunk #1 FAILED at 187.
1 out of 1 hunk FAILED
checking file drivers/usb/dwc2/gadget.c
Hunk #2 succeeded at 2863 (offset -46 lines).
Hunk #3 succeeded at 2889 (offset -46 lines).
Hunk #4 succeeded at 2915 (offset -47 lines).
Hunk #5 succeeded at 2934 (offset -47 lines).
Hunk #6 succeeded at 2964 (offset -47 lines).
Hunk #7 succeeded at 2976 (offset -47 lines).
Hunk #8 FAILED at 3518.
Hunk #9 succeeded at 3579 (offset -84 lines).
Hunk #10 succeeded at 3603 with fuzz 1 (offset -84 lines).
Hunk #11 succeeded at 3614 (offset -84 lines).
Hunk #12 succeeded at 3632 with fuzz 1 (offset -84 lines).
1 out of 12 hunks FAILED

please rebase on testing/next. Also, add Paul's Ack when doing so ;-)

thanks

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v5] usb: dwc2/gadget: rework disconnect event handling

[Felipe Balbi]

On Mon, Nov 17, 2014 at 09:59:42AM +0100, Marek Szyprowski wrote:
 This patch adds a call to s3c_hsotg_disconnect() from 'end session'
 interrupt (GOTGINT_SES_END_DET) to correctly notify gadget subsystem
 about unplugged usb cable. DISCONNINT interrupt cannot be used for this
 purpose, because it is asserted only in host mode.
 
 To avoid reporting disconnect event more than once, a disconnect call has
 been moved from USB_REQ_SET_ADDRESS handling function to SESSREQINT
 interrupt. This way driver ensures that disconnect event is reported
 either when usb cable is unplugged or every time the host starts a new
 session. To handle devices which has been synthesized without
 SRP support, connected state is set in ENUMDONE interrupt.
 
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com

doesn't apply, please rebase on my testing/next:

checking file drivers/usb/dwc2/core.h
Hunk #1 FAILED at 210.
1 out of 1 hunk FAILED
checking file drivers/usb/dwc2/gadget.c
Hunk #1 FAILED at 1029.
Hunk #2 succeeded at 1108 (offset 1 line).
Hunk #3 succeeded at 2031 (offset 1 line).
Hunk #4 FAILED at 2293.
2 out of 4 hunks FAILED

thanks

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v4] usb: dwc2/gadget: rework disconnect event handling

[Felipe Balbi]

On Fri, Nov 14, 2014 at 07:01:37PM +, Paul Zimmerman wrote:
  -Original Message-
  From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
  Sent: Friday, November 14, 2014 4:20 AM
  
  This patch adds a call to s3c_hsotg_disconnect() from 'end session'
  interrupt (GOTGINT_SES_END_DET) to correctly notify gadget subsystem
  about unplugged usb cable. DISCONNINT interrupt cannot be used for this
  purpose, because it is asserted only in host mode.
  
  To avoid reporting disconnect event more than once, a disconnect call has
  been moved from USB_REQ_SET_ADDRESS handling function to SESSREQINT
  interrupt. This way driver ensures that disconnect event is reported
  either when usb cable is unplugged or every time the host starts a new
  session.
  
  Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
  ---
   drivers/usb/dwc2/core.h   |  1 +
   drivers/usb/dwc2/gadget.c | 13 +++--
   2 files changed, 12 insertions(+), 2 deletions(-)
  
  diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
  index 55c90c53f2d6..78b9090ebf71 100644
  --- a/drivers/usb/dwc2/core.h
  +++ b/drivers/usb/dwc2/core.h
  @@ -210,6 +210,7 @@ struct s3c_hsotg {
  u8  ctrl_buff[8];
  
  struct usb_gadget   gadget;
  +   unsigned intsession:1;
  unsigned intsetup;
  unsigned long   last_rst;
  struct s3c_hsotg_ep *eps;
  diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
  index fcd2bb55ccca..c7f68dc1cf6b 100644
  --- a/drivers/usb/dwc2/gadget.c
  +++ b/drivers/usb/dwc2/gadget.c
  @@ -1029,7 +1029,6 @@ static int s3c_hsotg_process_req_feature(struct 
  s3c_hsotg *hsotg,
   }
  
   static void s3c_hsotg_enqueue_setup(struct s3c_hsotg *hsotg);
  -static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg);
  
   /**
* s3c_hsotg_stall_ep0 - stall ep0
  @@ -1107,7 +1106,6 @@ static void s3c_hsotg_process_control(struct 
  s3c_hsotg *hsotg,
  if ((ctrl-bRequestType  USB_TYPE_MASK) == USB_TYPE_STANDARD) {
  switch (ctrl-bRequest) {
  case USB_REQ_SET_ADDRESS:
  -   s3c_hsotg_disconnect(hsotg);
  dcfg = readl(hsotg-regs + DCFG);
  dcfg = ~DCFG_DEVADDR_MASK;
  dcfg |= (le16_to_cpu(ctrl-wValue) 
  @@ -2031,6 +2029,10 @@ static void s3c_hsotg_disconnect(struct s3c_hsotg 
  *hsotg)
   {
  unsigned ep;
  
  +   if (!hsotg-session)
  +   return;
  +
  +   hsotg-session = 0;
  for (ep = 0; ep  hsotg-num_of_eps; ep++)
  kill_all_requests(hsotg, hsotg-eps[ep], -ESHUTDOWN, true);
  
  @@ -2290,11 +2292,18 @@ irq_retry:
  dev_info(hsotg-dev, OTGInt: %08x\n, otgint);
  
  writel(otgint, hsotg-regs + GOTGINT);
  +
  +   if (otgint  GOTGINT_SES_END_DET) {
  +   s3c_hsotg_disconnect(hsotg);
 
 I think you should clear hsotg-session here, shouldn't you?
 Otherwise I think s3c_hsotg_disconnect() will be called twice, once
 here and once when the next GINTSTS_SESSREQINT comes.

the best way to avoid that would be fiddle with hsotg-session inside
s3c_hsotg_disconnect() only.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v3 1/2] usb: dwc2/gadget: add mutex to serialize init/deinit calls

[Felipe Balbi]

Hi,

On Fri, Nov 14, 2014 at 07:43:23PM +, Paul Zimmerman wrote:
   @@ -3699,6 +3717,8 @@ static int s3c_hsotg_resume(struct 
   platform_device *pdev)
  s3c_hsotg_core_connect(hsotg);
  spin_unlock_irqrestore(hsotg-lock, flags);
  
   +  mutex_unlock(hsotg-init_mutex);
   +
  return ret;
  }
  
   Hmm. I can't find any other UDC driver that uses a mutex in its
   suspend/resume functions. Can you explain why this is needed only
   for dwc2?
   I've posted this version because I thought you were not convinced that
   the patch
   usb: dwc2/gadget: rework suspend/resume code to correctly restore
   gadget state
   can add code for initialization and deinitialization in suspend/resume
   paths.
   My problem with that patch was that you were checking the -enabled
   flag outside of the spinlock. To address that, you only need to move
   the check inside of the spinlock. I don't see why a mutex is needed.
  
  It is not that simple. I can add spin_lock() before checking enabled,
  but then
  I would need to spin_unlock() to call regulator_bulk_enable() and
  phy_enable(),
  because both cannot be called from atomic context. This means that the
  spinlock
  in such case will not protect anything and is simply useless.
 
 Ah, OK. So you're using the mutex instead of the -enabled flag that you
 proposed in the rework suspend/resume code patch. So this patch is a
 replacement for that one. Somehow I was thinking this patch was on top
 of that one.
 
 So I guess this is OK, but I would like to get Felipe's opinion about
 it before we apply this.
 
 Felipe?

I can't think of a better way, I'm afraid :-(

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v2 03/10] usb: dwc2/gadget: fix gadget unregistration in udc_stop() function

[Felipe Balbi]

On Mon, Oct 20, 2014 at 12:45:33PM +0200, Marek Szyprowski wrote:
 udc_stop() should clear -driver pointer unconditionally to let the UDC
 framework to work correctly with both registering/unregistering gadgets
 and enabling/disabling gadgets by writing to
 /sys/class/udc/*hsotg/soft_connect interface.
 
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com

Also here. In fact, this is much more important :-)

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v2 04/10] usb: dwc2/gadget: disable phy before turning off power regulators

[Felipe Balbi]

On Mon, Oct 20, 2014 at 12:45:34PM +0200, Marek Szyprowski wrote:
 This patch fixes probe function to match the pattern used elsewhere in
 the driver, where power regulators are turned off as the last element in
 the device shutdown procedure.
 
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com

Paul, can I have your Acked-by here so I can send this to v3.18-rc1 ?

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v2 03/10] usb: dwc2/gadget: fix gadget unregistration in udc_stop() function

[Felipe Balbi]

On Thu, Oct 23, 2014 at 06:18:51PM +, Paul Zimmerman wrote:
  From: Felipe Balbi [mailto:ba...@ti.com]
  Sent: Thursday, October 23, 2014 8:02 AM
  
  On Mon, Oct 20, 2014 at 12:45:33PM +0200, Marek Szyprowski wrote:
   udc_stop() should clear -driver pointer unconditionally to let the UDC
   framework to work correctly with both registering/unregistering gadgets
   and enabling/disabling gadgets by writing to
   /sys/class/udc/*hsotg/soft_connect interface.
  
   Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
  
  Also here. In fact, this is much more important :-)
 
 Acked-by: Paul Zimmerman pa...@synopsys.com
 
 Are there any more patches in this series that need immediate attention?
 Otherwise I plan to finish reviewing the series on Friday or so.

no, this is the most important one. Everything else can wait until
Friday, thanks.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v2 04/10] usb: dwc2/gadget: disable phy before turning off power regulators

[Felipe Balbi]

On Thu, Oct 23, 2014 at 06:15:57PM +, Paul Zimmerman wrote:
  From: Felipe Balbi [mailto:ba...@ti.com]
  Sent: Thursday, October 23, 2014 8:01 AM
  
  On Mon, Oct 20, 2014 at 12:45:34PM +0200, Marek Szyprowski wrote:
   This patch fixes probe function to match the pattern used elsewhere in
   the driver, where power regulators are turned off as the last element in
   the device shutdown procedure.
  
   Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
  
  Paul, can I have your Acked-by here so I can send this to v3.18-rc1 ?
 
 Sorry, I've been working on a really hot issue at $work and didn't have
 time yet to review this series.

don't worry, we've all been there :-)

thanks

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 3/9] usb: dwc2/gadget: fix support for soft_connect udc framework feature

[Felipe Balbi]

On Fri, Oct 17, 2014 at 12:43:48PM +0200, Marek Szyprowski wrote:
 Hello,
 
 On 2014-10-16 15:36, Felipe Balbi wrote:
 On Thu, Oct 16, 2014 at 02:57:59PM +0200, Marek Szyprowski wrote:
 Enabling and disabling usb gadget by writing to
 /sys/class/udc/*hsotg/soft_connect results in calling udc_start/udc_stop
 functions with the same usb gadget driver, so the driver should not WARN
 about such case.
 
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
 ---
   drivers/usb/dwc2/gadget.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index 8870e38c1d82..37fda4c03397 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -2892,7 +2892,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget 
 *gadget,
 return -EINVAL;
 }
 -   WARN_ON(hsotg-driver);
 +   WARN_ON(hsotg-driver  hsotg-driver != driver);
 the bug is in your -udc_stop(). You should clear hsotg-driver to NULL
 there.
 
 Ok, I will change udc_stop() to always zero hsotg-driver, like other udc
 drivers. I was a bit confused by the fact that udc core passes driver to
 udc_stop(), when called from soft_connect and NULL on gadget removal.

That can probably be cleaned up, I'll go have a look on all UDCs and
make sure I won't break anything.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 3/9] usb: dwc2/gadget: fix support for soft_connect udc framework feature

[Felipe Balbi]

Hi,

On Fri, Oct 17, 2014 at 10:44:35AM -0500, Felipe Balbi wrote:
 On Fri, Oct 17, 2014 at 12:43:48PM +0200, Marek Szyprowski wrote:
  Hello,
  
  On 2014-10-16 15:36, Felipe Balbi wrote:
  On Thu, Oct 16, 2014 at 02:57:59PM +0200, Marek Szyprowski wrote:
  Enabling and disabling usb gadget by writing to
  /sys/class/udc/*hsotg/soft_connect results in calling udc_start/udc_stop
  functions with the same usb gadget driver, so the driver should not WARN
  about such case.
  
  Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
  ---
drivers/usb/dwc2/gadget.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
  
  diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
  index 8870e38c1d82..37fda4c03397 100644
  --- a/drivers/usb/dwc2/gadget.c
  +++ b/drivers/usb/dwc2/gadget.c
  @@ -2892,7 +2892,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget 
  *gadget,
return -EINVAL;
}
  - WARN_ON(hsotg-driver);
  + WARN_ON(hsotg-driver  hsotg-driver != driver);
  the bug is in your -udc_stop(). You should clear hsotg-driver to NULL
  there.
  
  Ok, I will change udc_stop() to always zero hsotg-driver, like other udc
  drivers. I was a bit confused by the fact that udc core passes driver to
  udc_stop(), when called from soft_connect and NULL on gadget removal.
 
 That can probably be cleaned up, I'll go have a look on all UDCs and
 make sure I won't break anything.

looks like chipidea is the only one still using that argument, if you
make your patch look like below:

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 7b5856f..ac14328 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2934,8 +2934,7 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
 
spin_lock_irqsave(hsotg-lock, flags);
 
-   if (!driver)
-   hsotg-driver = NULL;
+   hsotg-driver = NULL;
 
hsotg-gadget.speed = USB_SPEED_UNKNOWN;
 

I'll remove the argument.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 1/9] usb: dwc2/gadget: report disconnect event from 'end session' irq

[Felipe Balbi]

On Fri, Oct 17, 2014 at 12:35:39PM +0200, Marek Szyprowski wrote:
 Hello,
 
 On 2014-10-16 15:33, Felipe Balbi wrote:
 On Thu, Oct 16, 2014 at 02:57:57PM +0200, Marek Szyprowski wrote:
 This patch adds a call to s3c_hsotg_disconnect() from 'end session'
 interrupt to correctly notify gadget subsystem about unplugged usb
 cable.
 
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
 ---
   drivers/usb/dwc2/gadget.c | 6 ++
   1 file changed, 6 insertions(+)
 
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index 7b5856fadd93..119c8a3effc2 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -2279,6 +2279,12 @@ irq_retry:
 dev_info(hsotg-dev, OTGInt: %08x\n, otgint);
 writel(otgint, hsotg-regs + GOTGINT);
 +
 +   if (otgint  GOTGINT_SES_END_DET) {
 looks like this should be done for GINTSTS_DISCONNINT.
 
 I also would like to report it from that interrupt, but according to
 DWC2 databook (version 2.81a) and my observations on Samsung Exynos
 SoCs, DISCONNINT interrupt is asserted only in host mode, so in device
 mode we need to use something else.

If that's the case, then I withdraw my comments. I don't have access to
HW or docs about this IP, it just looked suspicious :-)

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 1/9] usb: dwc2/gadget: report disconnect event from 'end session' irq

[Felipe Balbi]

On Thu, Oct 16, 2014 at 02:57:57PM +0200, Marek Szyprowski wrote:
 This patch adds a call to s3c_hsotg_disconnect() from 'end session'
 interrupt to correctly notify gadget subsystem about unplugged usb
 cable.
 
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
 ---
  drivers/usb/dwc2/gadget.c | 6 ++
  1 file changed, 6 insertions(+)
 
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index 7b5856fadd93..119c8a3effc2 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -2279,6 +2279,12 @@ irq_retry:
   dev_info(hsotg-dev, OTGInt: %08x\n, otgint);
  
   writel(otgint, hsotg-regs + GOTGINT);
 +
 + if (otgint  GOTGINT_SES_END_DET) {

looks like this should be done for GINTSTS_DISCONNINT.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 3/9] usb: dwc2/gadget: fix support for soft_connect udc framework feature

[Felipe Balbi]

Hi,

On Thu, Oct 16, 2014 at 02:57:59PM +0200, Marek Szyprowski wrote:
 Enabling and disabling usb gadget by writing to
 /sys/class/udc/*hsotg/soft_connect results in calling udc_start/udc_stop
 functions with the same usb gadget driver, so the driver should not WARN
 about such case.
 
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
 ---
  drivers/usb/dwc2/gadget.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index 8870e38c1d82..37fda4c03397 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -2892,7 +2892,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget 
 *gadget,
   return -EINVAL;
   }
  
 - WARN_ON(hsotg-driver);
 + WARN_ON(hsotg-driver  hsotg-driver != driver);

the bug is in your -udc_stop(). You should clear hsotg-driver to NULL
there.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 6/9] usb: dwc2/gadget: decouple setting soft disconnect from s3c_hsotg_core_init

[Felipe Balbi]

On Thu, Oct 16, 2014 at 02:58:02PM +0200, Marek Szyprowski wrote:
 This patch changes s3c_hsotg_core_init function to leave hardware in
 soft disconnect mode, so the actual moment of coupling the hardware to
 the usb bus can be later controlled by the driver in the more accurate

what is this more accurate way you talk about ? Why is it more
accurate ? Perhaps you have failed some USB Certification test ? Which
test id was that ? Why did it fail ? and why does this patch solve the
issue ?

 way. For this purpose, separate functions for enabling and disabling
 soft disconnect mode have been added.
 
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
 ---
  drivers/usb/dwc2/gadget.c | 22 +-
  1 file changed, 17 insertions(+), 5 deletions(-)
 
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index 1ba0682fb252..d039334967d7 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -2124,7 +2124,7 @@ static int s3c_hsotg_corereset(struct s3c_hsotg *hsotg)
   *
   * Issue a soft reset to the core, and await the core finishing it.
   */
 -static void s3c_hsotg_core_init(struct s3c_hsotg *hsotg)
 +static void s3c_hsotg_core_init_disconnected(struct s3c_hsotg *hsotg)
  {
   s3c_hsotg_corereset(hsotg);
  
 @@ -2241,14 +2241,23 @@ static void s3c_hsotg_core_init(struct s3c_hsotg 
 *hsotg)
   readl(hsotg-regs + DOEPCTL0));
  
   /* clear global NAKs */
 - writel(DCTL_CGOUTNAK | DCTL_CGNPINNAK,
 + writel(DCTL_CGOUTNAK | DCTL_CGNPINNAK | DCTL_SFTDISCON,
  hsotg-regs + DCTL);
  
   /* must be at-least 3ms to allow bus to see disconnect */
   mdelay(3);
  
   hsotg-last_rst = jiffies;
 +}
 +
 +static void s3c_hsotg_core_disconnect(struct s3c_hsotg *hsotg)
 +{
 + /* set the soft-disconnect bit */
 + __orr32(hsotg-regs + DCTL, DCTL_SFTDISCON);
 +}
  
 +static void s3c_hsotg_core_connect(struct s3c_hsotg *hsotg)
 +{
   /* remove the soft-disconnect and let's go */
   __bic32(hsotg-regs + DCTL, DCTL_SFTDISCON);
  }
 @@ -2348,7 +2357,8 @@ irq_retry:
   kill_all_requests(hsotg, hsotg-eps[0],
 -ECONNRESET, true);
  
 - s3c_hsotg_core_init(hsotg);
 + s3c_hsotg_core_init_disconnected(hsotg);
 + s3c_hsotg_core_connect(hsotg);
   }
   }
   }
 @@ -2983,7 +2993,8 @@ static int s3c_hsotg_pullup(struct usb_gadget *gadget, 
 int is_on)
   if (is_on) {
   s3c_hsotg_phy_enable(hsotg);
   clk_enable(hsotg-clk);
 - s3c_hsotg_core_init(hsotg);
 + s3c_hsotg_core_init_disconnected(hsotg);
 + s3c_hsotg_core_connect(hsotg);
   } else {
   clk_disable(hsotg-clk);
   s3c_hsotg_phy_disable(hsotg);
 @@ -3670,7 +3681,8 @@ static int s3c_hsotg_resume(struct platform_device 
 *pdev)
  
   spin_lock_irqsave(hsotg-lock, flags);
   s3c_hsotg_phy_enable(hsotg);
 - s3c_hsotg_core_init(hsotg);
 + s3c_hsotg_core_init_disconnect(hsotg);
 + s3c_hsotg_core_connect(hsotg);
   spin_unlock_irqrestore(hsotg-lock, flags);
  
   return ret;
 -- 
 1.9.2
 
 --
 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

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 7/9] usb: dwc2/gadget: use soft disconnect mode for implementing pullup control

[Felipe Balbi]

Hi,

your subject line is wrong. -pullup() is already implemented, you're
moving unnecessary code from -pullup() to other places.

On Thu, Oct 16, 2014 at 02:58:03PM +0200, Marek Szyprowski wrote:
 This patch moves PHY enable and disable calls from pullup method to
 udc_start/stop functions and adds calls to recently introduces soft
 disconnect mode in pullup method. This improves dwc2 gadget driver
 compatibility with gadget API requirements (now pullup method really
 forces soft disconnect mode instead of shutting down the whole hardware)
 and as a side effect also solves the issue related to limited caller
 context for PHY related functions (they cannot be called from
 non-sleeping context).

you're doing two things in one patch. See below

 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
 ---
  drivers/usb/dwc2/gadget.c | 16 +---
  1 file changed, 13 insertions(+), 3 deletions(-)
 
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index d039334967d7..cdf417a7ae63 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -2883,6 +2883,7 @@ static int s3c_hsotg_udc_start(struct usb_gadget 
 *gadget,
  struct usb_gadget_driver *driver)
  {
   struct s3c_hsotg *hsotg = to_hsotg(gadget);
 + unsigned long flags;
   int ret;
  
   if (!hsotg) {
 @@ -2919,7 +2920,15 @@ static int s3c_hsotg_udc_start(struct usb_gadget 
 *gadget,
   goto err;
   }
  
 + s3c_hsotg_phy_enable(hsotg);

you moved phy_enable to udc_start/udc_stop (one patch)

 +
 + spin_lock_irqsave(hsotg-lock, flags);
 + s3c_hsotg_init(hsotg);
 + s3c_hsotg_core_init_disconnected(hsotg);

you moved core init here (another patch).

 + spin_unlock_irqrestore(hsotg-lock, flags);
 +
   dev_info(hsotg-dev, bound driver %s\n, driver-driver.name);
 +
   return 0;
  
  err:
 @@ -2957,6 +2966,8 @@ static int s3c_hsotg_udc_stop(struct usb_gadget *gadget,
  
   spin_unlock_irqrestore(hsotg-lock, flags);
  
 + s3c_hsotg_phy_disable(hsotg);
 +
   regulator_bulk_disable(ARRAY_SIZE(hsotg-supplies), hsotg-supplies);
  
   clk_disable(hsotg-clk);
 @@ -2990,14 +3001,13 @@ static int s3c_hsotg_pullup(struct usb_gadget 
 *gadget, int is_on)
   dev_dbg(hsotg-dev, %s: is_on: %d\n, __func__, is_on);
  
   spin_lock_irqsave(hsotg-lock, flags);
 +
   if (is_on) {
 - s3c_hsotg_phy_enable(hsotg);
   clk_enable(hsotg-clk);
 - s3c_hsotg_core_init_disconnected(hsotg);
   s3c_hsotg_core_connect(hsotg);
   } else {
 + s3c_hsotg_core_disconnect(hsotg);
   clk_disable(hsotg-clk);
 - s3c_hsotg_phy_disable(hsotg);
   }
  
   hsotg-gadget.speed = USB_SPEED_UNKNOWN;
 -- 
 1.9.2
 
 --
 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

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 2/9] usb: dwc2/gadget: fix enumeration issues

[Felipe Balbi]

On Thu, Oct 16, 2014 at 02:57:58PM +0200, Marek Szyprowski wrote:
 Excessive debug messages might cause timing issues that prevent correct
 usb enumeration. This patch hides information about USB bus reset to let
 driver enumerate fast enough to avoid making host angry. This fixes
 endless enumeration and usb reset loop observed with some Linux hosts.
 
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
 ---
  drivers/usb/dwc2/gadget.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index 119c8a3effc2..8870e38c1d82 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -2333,7 +2333,7 @@ irq_retry:
  
   u32 usb_status = readl(hsotg-regs + GOTGCTL);
  
 - dev_info(hsotg-dev, %s: USBRst\n, __func__);
 + dev_dbg(hsotg-dev, %s: USBRst\n, __func__);

considering this is inside an IRQ handler, I'd rather use dev_vdbg() but
no strong feelings:

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

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 8/9] usb: dwc2/gadget: fix calls to phy control functions in suspend/resume code

[Felipe Balbi]

Hi,

On Thu, Oct 16, 2014 at 02:58:04PM +0200, Marek Szyprowski wrote:
 This patch moves calls to phy enable/disable out of spinlock protected
 blocks in device suspend/resume to fix incorrect caller context. Phy
 related functions must not be called from atomic context.
 
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
 ---
  drivers/usb/dwc2/gadget.c | 9 ++---
  1 file changed, 6 insertions(+), 3 deletions(-)
 
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index cdf417a7ae63..052b1a857291 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -3656,11 +3656,13 @@ static int s3c_hsotg_suspend(struct platform_device 
 *pdev, pm_message_t state)
hsotg-driver-driver.name);
  
   spin_lock_irqsave(hsotg-lock, flags);
 + s3c_hsotg_core_disconnect(hsotg);
   s3c_hsotg_disconnect(hsotg);
 - s3c_hsotg_phy_disable(hsotg);
   hsotg-gadget.speed = USB_SPEED_UNKNOWN;
   spin_unlock_irqrestore(hsotg-lock, flags);
  
 + s3c_hsotg_phy_disable(hsotg);

this is aching to have a locked version as well as an unlocked version.
Look at what you do here. There's a minor race when you release that
spinlock. By the time -suspend() is called, IRQs are not yet disabled.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v2 1/4] dwc3: exynos: Add support for SCLK present on Exynos7

[Felipe Balbi]

Hi,

On Tue, Oct 14, 2014 at 10:25:00AM +0530, Vivek Gautam wrote:
 Hi Felipe,
 
 
 On Tue, Oct 14, 2014 at 4:14 AM, Felipe Balbi ba...@ti.com wrote:
  Hi,
 
  On Mon, Oct 13, 2014 at 01:54:59PM +0900, Anton Tikhomirov wrote:
  Hi Vivek,
 
   Exynos7 also has a separate special gate clock going to the IP
   apart from the usual AHB clock. So add support for the same.
 
  As we discussed before, Exynos7 SoCs have 7 clocks to be controlled
  by the driver. Adding only sclk is not enough.
 
  
   Signed-off-by: Vivek Gautam gautam.vi...@samsung.com
   ---
drivers/usb/dwc3/dwc3-exynos.c |   16 
1 file changed, 16 insertions(+)
  
   diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-
   exynos.c
   index 3951a65..7dc6a98 100644
   --- a/drivers/usb/dwc3/dwc3-exynos.c
   +++ b/drivers/usb/dwc3/dwc3-exynos.c
   @@ -35,6 +35,7 @@ struct dwc3_exynos {
   struct device   *dev;
  
   struct clk  *clk;
 
  The clock clk in Exynos5 just gated all that above 7 clocks, which
  we should control separately now in Exynos7.
 
 
  should I drop this patch for now ?
 
 Yes, better to hold this for some time till we get more clarity
 from our h/w team.

now dropped. Please a new one if needed.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v2 1/4] dwc3: exynos: Add support for SCLK present on Exynos7

[Felipe Balbi]

Hi,

On Mon, Oct 13, 2014 at 01:54:59PM +0900, Anton Tikhomirov wrote:
 Hi Vivek,
 
  Exynos7 also has a separate special gate clock going to the IP
  apart from the usual AHB clock. So add support for the same.
 
 As we discussed before, Exynos7 SoCs have 7 clocks to be controlled
 by the driver. Adding only sclk is not enough. 
 
  
  Signed-off-by: Vivek Gautam gautam.vi...@samsung.com
  ---
   drivers/usb/dwc3/dwc3-exynos.c |   16 
   1 file changed, 16 insertions(+)
  
  diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-
  exynos.c
  index 3951a65..7dc6a98 100644
  --- a/drivers/usb/dwc3/dwc3-exynos.c
  +++ b/drivers/usb/dwc3/dwc3-exynos.c
  @@ -35,6 +35,7 @@ struct dwc3_exynos {
  struct device   *dev;
  
  struct clk  *clk;
 
 The clock clk in Exynos5 just gated all that above 7 clocks, which
 we should control separately now in Exynos7.
 

should I drop this patch for now ?

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v2 1/4] dwc3: exynos: Add support for SCLK present on Exynos7

[Felipe Balbi]

On Tue, Oct 07, 2014 at 03:49:33PM +0530, Vivek Gautam wrote:
 Exynos7 also has a separate special gate clock going to the IP
 apart from the usual AHB clock. So add support for the same.
 
 Signed-off-by: Vivek Gautam gautam.vi...@samsung.com

I'll take this one once -rc1 is tagged. The others have no direct
dependency on this so I'll leave them to Kishon.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v6 4/4] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

[Felipe Balbi]

Hi,

On Thu, Sep 11, 2014 at 09:10:21PM +0530, Kishon Vijay Abraham I wrote:
 Hi,
 
 On Wednesday 10 September 2014 01:26 PM, Vivek Gautam wrote:
  On Wed, Sep 10, 2014 at 10:53 AM, Vivek Gautam gautam.vi...@samsung.com 
  wrote:
  On Wed, Sep 10, 2014 at 10:23 AM, Felipe Balbi ba...@ti.com wrote:
  On Wed, Sep 10, 2014 at 09:09:57AM +0530, Vivek Gautam wrote:
  On Wed, Sep 10, 2014 at 9:07 AM, Vivek Gautam gautam.vi...@samsung.com 
  wrote:
  adding Julius here,
 
  i think i had missed adding Julius for this entire series :-(
  I should be more careful with the CC list in future.
  Added his chromium id, since that seems to be more active.
 
 
 
  On Tue, Sep 9, 2014 at 8:12 PM, Felipe Balbi ba...@ti.com wrote:
  On Tue, Sep 09, 2014 at 07:19:50AM +0530, Vivek Gautam wrote:
  Hi,
 
 
  On Mon, Sep 8, 2014 at 7:14 PM, Felipe Balbi ba...@ti.com wrote:
  Hi,
 
  On Mon, Sep 08, 2014 at 09:53:09AM +0530, Vivek Gautam wrote:
  On Fri, Sep 5, 2014 at 11:26 PM, Felipe Balbi ba...@ti.com wrote:
  On Thu, Sep 04, 2014 at 12:01:19PM +0530, Vivek Gautam wrote:
  Don't we have phy_power_on()
  for that ? It looks like you could just as well do this from
  phy_power_on() ?
 
  No, unfortunately keeping these calibration settings in 
  phy_power_on()
  doesn't help, since we need to do this after XHCI reset has 
  happened.
 
  teach xHCI about PHYs ?
 
  sorry i couldn't understand you here.
  Aren't we trying to do the same with Heikki's patch about dwc3 :
  [PATCH 6/6] usb: dwc3: host: convey the PHYs to xhci
 
  and the 2nd patch in this series :
  [PATCH v6 2/4] usb: host: xhci-plat: Get PHYs for xhci's hcds
 
  Is there something else that is expected ?
 
  right, use that to call phy_init() at the right time, then you need 
  to
  add a new -calibrate() method which, likely, will only be used by 
  you
  ;-)
 
  so you mean, the xhci should itself call phy_init() at a time 
  suitable,
  so that -calibrate() is not required at all ?
  
  but wait, dwc3 does a phy_init() already, then how xhci will be able to
  do that again. We can't do phy_init() multiple times right ?
 
 right. I think we should split and do phy ops separately for dwc3 host
 and gadget?

no, don't do that. We need a better way of handling this. As of now we
don't support dual-role, so we can just reinitialize the PHY once we
reach xhci.

Once we start supporting dual-role, we will need more inteligence in the
algorithm.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v6 4/4] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

[Felipe Balbi]

On Tue, Sep 09, 2014 at 07:19:50AM +0530, Vivek Gautam wrote:
 Hi,
 
 
 On Mon, Sep 8, 2014 at 7:14 PM, Felipe Balbi ba...@ti.com wrote:
  Hi,
 
  On Mon, Sep 08, 2014 at 09:53:09AM +0530, Vivek Gautam wrote:
  On Fri, Sep 5, 2014 at 11:26 PM, Felipe Balbi ba...@ti.com wrote:
   On Thu, Sep 04, 2014 at 12:01:19PM +0530, Vivek Gautam wrote:
Don't we have phy_power_on()
for that ? It looks like you could just as well do this from
phy_power_on() ?
  
   No, unfortunately keeping these calibration settings in phy_power_on()
   doesn't help, since we need to do this after XHCI reset has happened.
  
   teach xHCI about PHYs ?
 
  sorry i couldn't understand you here.
  Aren't we trying to do the same with Heikki's patch about dwc3 :
  [PATCH 6/6] usb: dwc3: host: convey the PHYs to xhci
 
  and the 2nd patch in this series :
  [PATCH v6 2/4] usb: host: xhci-plat: Get PHYs for xhci's hcds
 
  Is there something else that is expected ?
 
  right, use that to call phy_init() at the right time, then you need to
  add a new -calibrate() method which, likely, will only be used by you
  ;-)
 
 so you mean, the xhci should itself call phy_init() at a time suitable,
 so that -calibrate() is not required at all ?
 
 i think you meant there - then you __do not__ need to

right :-)

  add a new -calibrate() method which, likely,
 will only be used by you, is it ?

yup

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v6 4/4] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

[Felipe Balbi]

On Wed, Sep 10, 2014 at 09:09:57AM +0530, Vivek Gautam wrote:
 On Wed, Sep 10, 2014 at 9:07 AM, Vivek Gautam gautam.vi...@samsung.com 
 wrote:
  adding Julius here,
 
 i think i had missed adding Julius for this entire series :-(
 I should be more careful with the CC list in future.
 Added his chromium id, since that seems to be more active.
 
 
 
  On Tue, Sep 9, 2014 at 8:12 PM, Felipe Balbi ba...@ti.com wrote:
  On Tue, Sep 09, 2014 at 07:19:50AM +0530, Vivek Gautam wrote:
  Hi,
 
 
  On Mon, Sep 8, 2014 at 7:14 PM, Felipe Balbi ba...@ti.com wrote:
   Hi,
  
   On Mon, Sep 08, 2014 at 09:53:09AM +0530, Vivek Gautam wrote:
   On Fri, Sep 5, 2014 at 11:26 PM, Felipe Balbi ba...@ti.com wrote:
On Thu, Sep 04, 2014 at 12:01:19PM +0530, Vivek Gautam wrote:
 Don't we have phy_power_on()
 for that ? It looks like you could just as well do this from
 phy_power_on() ?
   
No, unfortunately keeping these calibration settings in 
phy_power_on()
doesn't help, since we need to do this after XHCI reset has 
happened.
   
teach xHCI about PHYs ?
  
   sorry i couldn't understand you here.
   Aren't we trying to do the same with Heikki's patch about dwc3 :
   [PATCH 6/6] usb: dwc3: host: convey the PHYs to xhci
  
   and the 2nd patch in this series :
   [PATCH v6 2/4] usb: host: xhci-plat: Get PHYs for xhci's hcds
  
   Is there something else that is expected ?
  
   right, use that to call phy_init() at the right time, then you need to
   add a new -calibrate() method which, likely, will only be used by you
   ;-)
 
  so you mean, the xhci should itself call phy_init() at a time suitable,
  so that -calibrate() is not required at all ?
 
  i think you meant there - then you __do not__ need to
 
  right :-)
 
  alright, i will prepare a patch for the suggested change.
 
  But AFAI remember we had discussion for this patch in earlier
  version, and Julius suggested to use a generic approach for such
  change wherein other users in future may be able to use the
  facility.

right, and what's more generic than adding the support for PHYs straight
into xHCI ?

What I fear is that we end up opening the doors for every odd
platform-specific operation to be added to the framework without really
considering what needs to be done. That would defeat the idea of having
a generic framework altogether.

cheers

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v6 4/4] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

[Felipe Balbi]

Hi,

On Mon, Sep 08, 2014 at 09:53:09AM +0530, Vivek Gautam wrote:
 On Fri, Sep 5, 2014 at 11:26 PM, Felipe Balbi ba...@ti.com wrote:
  On Thu, Sep 04, 2014 at 12:01:19PM +0530, Vivek Gautam wrote:
   Don't we have phy_power_on()
   for that ? It looks like you could just as well do this from
   phy_power_on() ?
 
  No, unfortunately keeping these calibration settings in phy_power_on()
  doesn't help, since we need to do this after XHCI reset has happened.
 
  teach xHCI about PHYs ?
 
 sorry i couldn't understand you here.
 Aren't we trying to do the same with Heikki's patch about dwc3 :
 [PATCH 6/6] usb: dwc3: host: convey the PHYs to xhci
 
 and the 2nd patch in this series :
 [PATCH v6 2/4] usb: host: xhci-plat: Get PHYs for xhci's hcds
 
 Is there something else that is expected ?

right, use that to call phy_init() at the right time, then you need to
add a new -calibrate() method which, likely, will only be used by you
;-)

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v6 4/4] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

[Felipe Balbi]

On Thu, Sep 04, 2014 at 12:01:19PM +0530, Vivek Gautam wrote:
  Don't we have phy_power_on()
  for that ? It looks like you could just as well do this from
  phy_power_on() ?
 
 No, unfortunately keeping these calibration settings in phy_power_on()
 doesn't help, since we need to do this after XHCI reset has happened.

teach xHCI about PHYs ?

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v6 4/4] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

[Felipe Balbi]

Hi,

On Wed, Sep 03, 2014 at 12:59:27PM +0530, Vivek Gautam wrote:
  On Tue, Sep 02, 2014 at 04:42:18PM +0530, Vivek Gautam wrote:
  Adding phy calibrate callback, which facilitates setting certain
  PHY settings post initialization of the PHY controller.
  Exynos5420 and Exynos5800 have 28nm USB 3.0 DRD PHY for which
  the Loss-of-Signal (LOS) Detector Threshold Level as well as
  Tx-Vboost-Level should be controlled for Super-Speed operations.
 
  Additionally set proper time to wait for RxDetect measurement,
  for desired PHY reference clock, so as to solve issue with enumeration
  of few USB 3.0 devices, like Samsung SUM-TSB16S 3.0 USB drive
  on the controller.
  We are using CR_port for this purpose to send required data
  to override the LOS values.
 
  On testing with USB 3.0 devices on USB 3.0 port present on
  SMDK5420, and peach-pit boards should see following message:
  usb 2-1: new SuperSpeed USB device number 2 using xhci-hcd
 
  and without this patch, should see below shown message:
  usb 1-1: new high-speed USB device number 2 using xhci-hcd
 
  [Also removed unnecessary extra lines in the register macro definitions]
 
  Signed-off-by: Vivek Gautam gautam.vi...@samsung.com
  ---
   drivers/phy/phy-exynos5-usbdrd.c |  185 
  ++
   1 file changed, 170 insertions(+), 15 deletions(-)
 
  diff --git a/drivers/phy/phy-exynos5-usbdrd.c 
  b/drivers/phy/phy-exynos5-usbdrd.c
  index 47f47fe..85742b0 100644
  --- a/drivers/phy/phy-exynos5-usbdrd.c
  +++ b/drivers/phy/phy-exynos5-usbdrd.c
  @@ -37,13 +37,11 @@
 
   /* EXYNOS5: USB 3.0 DRD PHY registers */
   #define EXYNOS5_DRD_LINKSYSTEM   0x04
  -
   #define LINKSYSTEM_FLADJ_MASK(0x3f  1)
   #define LINKSYSTEM_FLADJ(_x) ((_x)  1)
   #define LINKSYSTEM_XHCI_VERSION_CONTROL  BIT(27)
 
   #define EXYNOS5_DRD_PHYUTMI  0x08
  -
   #define PHYUTMI_OTGDISABLE   BIT(6)
   #define PHYUTMI_FORCESUSPEND BIT(1)
   #define PHYUTMI_FORCESLEEP   BIT(0)
  @@ -51,26 +49,20 @@
   #define EXYNOS5_DRD_PHYPIPE  0x0c
 
   #define EXYNOS5_DRD_PHYCLKRST0x10
  -
   #define PHYCLKRST_EN_UTMISUSPEND BIT(31)
  -
   #define PHYCLKRST_SSC_REFCLKSEL_MASK (0xff  23)
   #define PHYCLKRST_SSC_REFCLKSEL(_x)  ((_x)  23)
  -
   #define PHYCLKRST_SSC_RANGE_MASK (0x03  21)
   #define PHYCLKRST_SSC_RANGE(_x)  ((_x)  21)
  -
   #define PHYCLKRST_SSC_EN BIT(20)
   #define PHYCLKRST_REF_SSP_EN BIT(19)
   #define PHYCLKRST_REF_CLKDIV2BIT(18)
  -
   #define PHYCLKRST_MPLL_MULTIPLIER_MASK   (0x7f  11)
   #define PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF (0x19  11)
   #define PHYCLKRST_MPLL_MULTIPLIER_50M_REF(0x32  11)
   #define PHYCLKRST_MPLL_MULTIPLIER_24MHZ_REF  (0x68  11)
   #define PHYCLKRST_MPLL_MULTIPLIER_20MHZ_REF  (0x7d  11)
   #define PHYCLKRST_MPLL_MULTIPLIER_19200KHZ_REF   (0x02  11)
  -
   #define PHYCLKRST_FSEL_UTMI_MASK (0x7  5)
   #define PHYCLKRST_FSEL_PIPE_MASK (0x7  8)
   #define PHYCLKRST_FSEL(_x)   ((_x)  5)
  @@ -78,46 +70,68 @@
   #define PHYCLKRST_FSEL_PAD_24MHZ (0x2a  5)
   #define PHYCLKRST_FSEL_PAD_20MHZ (0x31  5)
   #define PHYCLKRST_FSEL_PAD_19_2MHZ   (0x38  5)
  -
   #define PHYCLKRST_RETENABLEN BIT(4)
  -
   #define PHYCLKRST_REFCLKSEL_MASK (0x03  2)
   #define PHYCLKRST_REFCLKSEL_PAD_REFCLK   (0x2  2)
   #define PHYCLKRST_REFCLKSEL_EXT_REFCLK   (0x3  2)
  -
   #define PHYCLKRST_PORTRESET  BIT(1)
   #define PHYCLKRST_COMMONONN  BIT(0)
 
   #define EXYNOS5_DRD_PHYREG0  0x14
  +#define PHYREG0_SSC_REF_CLK_SEL  BIT(21)
  +#define PHYREG0_SSC_RANGEBIT(20)
  +#define PHYREG0_CR_WRITE BIT(19)
  +#define PHYREG0_CR_READ  BIT(18)
  +#define PHYREG0_CR_DATA_IN(_x)   ((_x)  2)
  +#define PHYREG0_CR_CAP_DATA  BIT(1)
  +#define PHYREG0_CR_CAP_ADDR  BIT(0)
  +
   #define EXYNOS5_DRD_PHYREG1  0x18
  +#define PHYREG1_CR_DATA_OUT(_x)  ((_x)  1)
  +#define PHYREG1_CR_ACK   BIT(0)
 
   #define EXYNOS5_DRD_PHYPARAM00x1c
  -
   #define PHYPARAM0_REF_USE_PADBIT(31)
   #define PHYPARAM0_REF_LOSLEVEL_MASK  (0x1f  26)
   #define PHYPARAM0_REF_LOSLEVEL   (0x9  26)
 
   #define EXYNOS5_DRD_PHYPARAM10x20
  -
   #define PHYPARAM1_PCS_TXDEEMPH_MASK  (0x1f  0)
   #define PHYPARAM1_PCS_TXDEEMPH   (0x1c)
 
   #define EXYNOS5_DRD_PHYTERM  0x24
 

Re: [PATCH 5/5] phy: exynos5-usbdrd: Adding Kconfig dependency for Exynos7

[Felipe Balbi]

On Wed, Sep 03, 2014 at 09:32:14AM +0530, Vivek Gautam wrote:
 On Tue, Sep 2, 2014 at 8:07 PM, Felipe Balbi ba...@ti.com wrote:
  On Mon, Sep 01, 2014 at 01:30:21PM +0530, Vivek Gautam wrote:
  On Thu, Aug 28, 2014 at 8:36 PM, Daniele Forsi dfo...@gmail.com wrote:
   2014-08-28 10:02 GMT+02:00 Vivek Gautam:
  
   This USB 3.0 PHY controller is also present on Exynos7
   platform, so adding the dependency on ARCH_EXYNOS7 for this driver.
  
   +++ b/drivers/phy/Kconfig
   @@ -186,7 +186,7 @@ config PHY_EXYNOS5250_USB2
  
config PHY_EXYNOS5_USBDRD
   tristate Exynos5 SoC series USB DRD PHY driver
   -   depends on ARCH_EXYNOS5  OF
   +   depends on (ARCH_EXYNOS5 || ARCH_EXYNOS7)  OF
  
   shouldn't that prompt and its help text be updated to mention also 
   Exynos7?
 
  Right, even that has to be updated accordingly. Will update the same in 
  next
  version of the patch. Thanks for pointing this.
 
  I would rather change that to ARCH_EXYNOS, unless Kishon doesn't like
  that idea. The thing is that this will likely need to be patches for
  exynos8, 9, 10, 11...
 
 Yes, after we have the 2nd version of Exynos7 support patches, it makes
 more sense to keep dependency on ARCH_EXYNOS.

thank you, that'll help new silicon wakeup; one less thing to patch :-p

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v6 4/4] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

[Felipe Balbi]

Hi,

On Tue, Sep 02, 2014 at 04:42:18PM +0530, Vivek Gautam wrote:
 Adding phy calibrate callback, which facilitates setting certain
 PHY settings post initialization of the PHY controller.
 Exynos5420 and Exynos5800 have 28nm USB 3.0 DRD PHY for which
 the Loss-of-Signal (LOS) Detector Threshold Level as well as
 Tx-Vboost-Level should be controlled for Super-Speed operations.
 
 Additionally set proper time to wait for RxDetect measurement,
 for desired PHY reference clock, so as to solve issue with enumeration
 of few USB 3.0 devices, like Samsung SUM-TSB16S 3.0 USB drive
 on the controller.
 We are using CR_port for this purpose to send required data
 to override the LOS values.
 
 On testing with USB 3.0 devices on USB 3.0 port present on
 SMDK5420, and peach-pit boards should see following message:
 usb 2-1: new SuperSpeed USB device number 2 using xhci-hcd
 
 and without this patch, should see below shown message:
 usb 1-1: new high-speed USB device number 2 using xhci-hcd
 
 [Also removed unnecessary extra lines in the register macro definitions]
 
 Signed-off-by: Vivek Gautam gautam.vi...@samsung.com
 ---
  drivers/phy/phy-exynos5-usbdrd.c |  185 
 ++
  1 file changed, 170 insertions(+), 15 deletions(-)
 
 diff --git a/drivers/phy/phy-exynos5-usbdrd.c 
 b/drivers/phy/phy-exynos5-usbdrd.c
 index 47f47fe..85742b0 100644
 --- a/drivers/phy/phy-exynos5-usbdrd.c
 +++ b/drivers/phy/phy-exynos5-usbdrd.c
 @@ -37,13 +37,11 @@
  
  /* EXYNOS5: USB 3.0 DRD PHY registers */
  #define EXYNOS5_DRD_LINKSYSTEM   0x04
 -
  #define LINKSYSTEM_FLADJ_MASK(0x3f  1)
  #define LINKSYSTEM_FLADJ(_x) ((_x)  1)
  #define LINKSYSTEM_XHCI_VERSION_CONTROL  BIT(27)
  
  #define EXYNOS5_DRD_PHYUTMI  0x08
 -
  #define PHYUTMI_OTGDISABLE   BIT(6)
  #define PHYUTMI_FORCESUSPEND BIT(1)
  #define PHYUTMI_FORCESLEEP   BIT(0)
 @@ -51,26 +49,20 @@
  #define EXYNOS5_DRD_PHYPIPE  0x0c
  
  #define EXYNOS5_DRD_PHYCLKRST0x10
 -
  #define PHYCLKRST_EN_UTMISUSPEND BIT(31)
 -
  #define PHYCLKRST_SSC_REFCLKSEL_MASK (0xff  23)
  #define PHYCLKRST_SSC_REFCLKSEL(_x)  ((_x)  23)
 -
  #define PHYCLKRST_SSC_RANGE_MASK (0x03  21)
  #define PHYCLKRST_SSC_RANGE(_x)  ((_x)  21)
 -
  #define PHYCLKRST_SSC_EN BIT(20)
  #define PHYCLKRST_REF_SSP_EN BIT(19)
  #define PHYCLKRST_REF_CLKDIV2BIT(18)
 -
  #define PHYCLKRST_MPLL_MULTIPLIER_MASK   (0x7f  11)
  #define PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF (0x19  11)
  #define PHYCLKRST_MPLL_MULTIPLIER_50M_REF(0x32  11)
  #define PHYCLKRST_MPLL_MULTIPLIER_24MHZ_REF  (0x68  11)
  #define PHYCLKRST_MPLL_MULTIPLIER_20MHZ_REF  (0x7d  11)
  #define PHYCLKRST_MPLL_MULTIPLIER_19200KHZ_REF   (0x02  11)
 -
  #define PHYCLKRST_FSEL_UTMI_MASK (0x7  5)
  #define PHYCLKRST_FSEL_PIPE_MASK (0x7  8)
  #define PHYCLKRST_FSEL(_x)   ((_x)  5)
 @@ -78,46 +70,68 @@
  #define PHYCLKRST_FSEL_PAD_24MHZ (0x2a  5)
  #define PHYCLKRST_FSEL_PAD_20MHZ (0x31  5)
  #define PHYCLKRST_FSEL_PAD_19_2MHZ   (0x38  5)
 -
  #define PHYCLKRST_RETENABLEN BIT(4)
 -
  #define PHYCLKRST_REFCLKSEL_MASK (0x03  2)
  #define PHYCLKRST_REFCLKSEL_PAD_REFCLK   (0x2  2)
  #define PHYCLKRST_REFCLKSEL_EXT_REFCLK   (0x3  2)
 -
  #define PHYCLKRST_PORTRESET  BIT(1)
  #define PHYCLKRST_COMMONONN  BIT(0)
  
  #define EXYNOS5_DRD_PHYREG0  0x14
 +#define PHYREG0_SSC_REF_CLK_SEL  BIT(21)
 +#define PHYREG0_SSC_RANGEBIT(20)
 +#define PHYREG0_CR_WRITE BIT(19)
 +#define PHYREG0_CR_READ  BIT(18)
 +#define PHYREG0_CR_DATA_IN(_x)   ((_x)  2)
 +#define PHYREG0_CR_CAP_DATA  BIT(1)
 +#define PHYREG0_CR_CAP_ADDR  BIT(0)
 +
  #define EXYNOS5_DRD_PHYREG1  0x18
 +#define PHYREG1_CR_DATA_OUT(_x)  ((_x)  1)
 +#define PHYREG1_CR_ACK   BIT(0)
  
  #define EXYNOS5_DRD_PHYPARAM00x1c
 -
  #define PHYPARAM0_REF_USE_PADBIT(31)
  #define PHYPARAM0_REF_LOSLEVEL_MASK  (0x1f  26)
  #define PHYPARAM0_REF_LOSLEVEL   (0x9  26)
  
  #define EXYNOS5_DRD_PHYPARAM10x20
 -
  #define PHYPARAM1_PCS_TXDEEMPH_MASK  (0x1f  0)
  #define PHYPARAM1_PCS_TXDEEMPH   (0x1c)
  
  #define EXYNOS5_DRD_PHYTERM  0x24
  
  #define EXYNOS5_DRD_PHYTEST  0x28
 -
  #define PHYTEST_POWERDOWN_SSPBIT(3)
  #define PHYTEST_POWERDOWN_HSP   

Re: [PATCH 1/5] usb: dwc3: exynos: Add support for SCLK present on Exynos7

[Felipe Balbi]

On Tue, Sep 02, 2014 at 04:09:08PM +0530, Vivek Gautam wrote:
 Hi,
 
 
 On Fri, Aug 29, 2014 at 12:18 AM, Mark Rutland mark.rutl...@arm.com wrote:
  On Thu, Aug 28, 2014 at 09:01:56AM +0100, Vivek Gautam wrote:
  Exynos7 also has a separate special gate clock going to the IP
  apart from the usual AHB clock. So add support for the same.
 
  Signed-off-by: Vivek Gautam gautam.vi...@samsung.com
  ---
   drivers/usb/dwc3/dwc3-exynos.c |   16 
   1 file changed, 16 insertions(+)
 
  diff --git a/drivers/usb/dwc3/dwc3-exynos.c 
  b/drivers/usb/dwc3/dwc3-exynos.c
  index f9fb8ad..bab6395 100644
  --- a/drivers/usb/dwc3/dwc3-exynos.c
  +++ b/drivers/usb/dwc3/dwc3-exynos.c
  @@ -35,6 +35,7 @@ struct dwc3_exynos {
struct device   *dev;
 
struct clk  *clk;
  + struct clk  *sclk;
struct regulator*vdd33;
struct regulator*vdd10;
   };
  @@ -141,10 +142,17 @@ static int dwc3_exynos_probe(struct platform_device 
  *pdev)
return -EINVAL;
}
 
  + /* Exynos7 has a special gate clock going to this IP */
  + exynos-sclk = devm_clk_get(dev, usbdrd30_sclk);
  + if (IS_ERR(exynos-sclk))
  + dev_warn(dev, couldn't get sclk\n);
 
  Doesn't this introduce a pointless warning for Exynos SoCs other than
  Exynos7?
 
 True, it will introduce an unnecessary warning for non-Exynos7 systems.
 I initially thought of introducing a compatible check for Exynos7-dwc3, but 
 that
 way we may end up adding such checks for future SoCs which have similar
 controller but have some clock difference or some other small change, no ?

maybe dev_dbg() is what you want ? :-)

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 3/5] phy: exynos5-usbdrd: Add facility for VBUS-BOOST-5V supply

[Felipe Balbi]

hi,

On Thu, Aug 28, 2014 at 01:31:58PM +0530, Vivek Gautam wrote:
 @@ -457,11 +458,19 @@ static int exynos5_usbdrd_phy_power_on(struct phy *phy)
   clk_prepare_enable(phy_drd-ref_clk);
  
   /* Enable VBUS supply */
 + if (phy_drd-vbus_boost) {
 + ret = regulator_enable(phy_drd-vbus_boost);
 + if (ret) {
 + dev_err(phy_drd-dev,
 + Failed to enable VBUS boost supply\n);
 + goto fail_vbus;
 + }
 + }

really this is nitpicking, but can you add a blank line here just make
my inner child happy ? :-)

 @@ -470,6 +479,9 @@ static int exynos5_usbdrd_phy_power_on(struct phy *phy)
  
   return 0;
  
 +fail_vbus_boost:
 + if (phy_drd-vbus_boost)
 + regulator_disable(phy_drd-vbus_boost);

same here

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 4/5] usb: dwc3: Adding Kconfig dependency for Exynos7

[Felipe Balbi]

On Thu, Aug 28, 2014 at 01:31:59PM +0530, Vivek Gautam wrote:
 The Exynos-DWC3 USB 3.0 DRD controller is also present on
 Exynos7 platform, so adding the dependency on ARCH_EXYNOS7
 for this driver.
 
 Signed-off-by: Vivek Gautam gautam.vi...@samsung.com
 ---
  drivers/usb/dwc3/Kconfig |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
 index 785510a..e235894 100644
 --- a/drivers/usb/dwc3/Kconfig
 +++ b/drivers/usb/dwc3/Kconfig
 @@ -55,7 +55,7 @@ config USB_DWC3_OMAP
  
  config USB_DWC3_EXYNOS
   tristate Samsung Exynos Platform
 - depends on ARCH_EXYNOS || COMPILE_TEST
 + depends on (ARCH_EXYNOS || ARCH_EXYNOS7) || COMPILE_TEST

wait when building for ARCH_EXYNOS7 you don't get ARCH_EXYNOS ?

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v2 0/5] usb: phy: samsung: remove old USB PHY code

[Felipe Balbi]

Hi,

On Fri, Aug 22, 2014 at 06:59:00PM +0200, Bartlomiej Zolnierkiewicz wrote:
 Hi,
 
 This patch series removes the old Samsung USB PHY drivers that
 got replaced by the new ones using the generic PHY layer.
 
 Depends on:
 - v3.17-rc1 branch of Linus' kernel
 
 Changes since v1 (https://lkml.org/lkml/2014/8/14/241):
 - rebased on v3.17-rc1 kernel
 - added Acked-by-s from Kishon Vijay Abraham I
 - added Reviewed-by-s from Vivek Gautam and Jingoo Han

So, everybody agrees that there will be no regressions, right ? I'll
merge this on my testing/next.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v2 5/5] usb: phy: samsung: remove old common USB PHY code

[Felipe Balbi]

Hi,

On Fri, Aug 22, 2014 at 06:59:05PM +0200, Bartlomiej Zolnierkiewicz wrote:
 drivers/usb/phy/phy-samsung-usb[2,3] drivers got replaced by
 drivers/phy/phy-samsung-usb[2,3] ones and the old common Samsung
 USB PHY code is no longer used.
 
 Signed-off-by: Bartlomiej Zolnierkiewicz b.zolnier...@samsung.com
 Acked-by: Kyungmin Park kyungmin.p...@samsung.com
 Reviewed-by: Vivek Gautam gautam.vi...@samsung.com
 Reviewed-by: Jingoo Han jg1@samsung.com
 Acked-by: Kishon Vijay Abraham I kis...@ti.com
 Cc: Kamil Debski k.deb...@samsung.com

this patch doesn't apply. Please rebase on my testing/next

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH] usb: dwc3: exynos: remove usb_phy_generic support

[Felipe Balbi]

Hi,

On Wed, Aug 27, 2014 at 11:42:25PM +0530, Vivek Gautam wrote:
 On Wed, Aug 27, 2014 at 1:22 PM, Bartlomiej Zolnierkiewicz
 b.zolnier...@samsung.com wrote:
  dwc3 driver is using the new Exynos5 SoC series USB DRD PHY driver
  (PHY_EXYNOS5_USBDRD which selects GENERIC_PHY) as can be seen by
  looking at the following commits:
 
7a4cf0fde054 (ARM: dts: Update DWC3 usb controller to use new
phy driver for exynos5250)
 
f070267b5fc1 (ARM: dts: Enable support for DWC3 controller for
exynos5420)
 
  Thus remove unused usb_phy_generic support from dwc3 Exynos glue
  layer.
 
  [ The code that is being removed is harmful in the context of
multi_v7_defconfig and enabling EHCI support for Samsung
S5P/EXYNOS SoC Series (USB_EHCI_EXYNOS) + OHCI support for
Samsung S5P/EXYNOS SoC Series (USB_OHCI_EXYNOS) because EHCI
support for OMAP3 and later chips (USB_EHCI_HCD_OMAP) selects
NOP USB Transceiver Driver (NOP_USB_XCEIV).  NOP USB driver
attaches itself to usb_phy_generic platform devices created by
dwc3 Exynos glue layer and later causes Exynos EHCI driver to
fail probe and Exynos OHCI driver to hang on probe (as observed
on Exynos5250 Arndale board). ]
 
 The issue with EHCI and OHCI on exynos platforms is that until now
 they also request
 usb-phy and only later if they don't find one, they go ahead and get a
 'phy' generic.
 
 Fortunately we missed this issue with exynos_defconfig, and as you rightly
 mentioned with multi_v7_defconfig, the NOP_USB_XCEIV gets enabled and
 EHCI and OHCI exynos get no-op usb-phy, which actually blocks EHCI/OHCI from
 initializing the real PHYs.

right, when I added PHY support to dwc3 I expected people to remote NOP
transceivers from their glue layers after they had proper PHY drivers
available.

 This issue is resolved with patches:
 [PATCH v2 1/2] usb: host: ehci-exynos: Remove unnecessary usb-phy support
 [PATCH v2 2/2] usb: host: ohci-exynos: Remove unnecessary usb-phy support
 wherein we are completely removing the usb-phy support from ehci/ohci-exynos.
 So with these patches we should not see the issue mentioned by you.
 
 Now for the DWC3 part, the usb-phy part was added to use register two
 separate no-op-xceivers of USB2_PHY type and USB3_PHY type,
 so that platforms with no separate PHY can still be able to use dwc3.

correct.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 0/5] usb: phy: samsung: remove old USB PHY code

[Felipe Balbi]

Hi,

On Thu, Aug 14, 2014 at 04:25:22PM +0200, Bartlomiej Zolnierkiewicz wrote:
 Hi,
 
 This patch series removes the old Samsung USB PHY drivers that
 got replaced by the new ones using the generic PHY layer.
 
 Depends on:
 - next-20140813 branch of linux-next kernel

this thread seems to have died, what do I need to do with these patches?
Are we deleting the phy drivers or not ?

Please rebase on v3.17-rc1 and resend with all Acks in place.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v4 2/4] usb: host: xhci-plat: Get PHYs for xhci's hcds

[Felipe Balbi]

On Wed, Jul 16, 2014 at 01:51:40PM +0530, Vivek Gautam wrote:
 The host controller by itself may sometimes need to handle PHY
 and/or calibrate some of the PHY settings to get full support out
 of the PHY controller. The PHY core provides a calibration
 funtionality now to do so.
 Therefore, facilitate getting the two possible PHYs, viz.
 USB 2.0 type (UTMI+) and USB 3.0 type (PIPE3).
 
 Signed-off-by: Vivek Gautam gautam.vi...@samsung.com
 ---
 
 Changes from v3:
  - Modified error message as per review comments from Julius.
 
  drivers/usb/host/xhci-plat.c |   19 +++
  1 file changed, 19 insertions(+)
 
 diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
 index 1a0cf9f..b1c0364 100644
 --- a/drivers/usb/host/xhci-plat.c
 +++ b/drivers/usb/host/xhci-plat.c
 @@ -16,6 +16,7 @@
  #include linux/module.h
  #include linux/of.h
  #include linux/platform_device.h
 +#include linux/phy/phy.h
  #include linux/slab.h
  #include linux/usb/xhci_pdriver.h
  
 @@ -180,6 +181,15 @@ static int xhci_plat_probe(struct platform_device *pdev)
   goto put_hcd;
   }
  
 + /* Get possile USB 2.0 type PHY (UTMI+) available with xhci */
 + hcd-gen_phy = devm_phy_get(pdev-dev, usb2-phy);
 + if (IS_ERR(hcd-gen_phy)) {
 + ret = PTR_ERR(hcd-gen_phy);
 + if (ret != -ENOSYS  ret != -ENODEV)
 + dev_warn(pdev-dev,
 +  Error retrieving usb2 phy: %d\n, ret);
 + }

should you treat -EPROBE_DEFER differently here ?

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 1/2] usb: host: Kconfig: Select PHY drivers for Exynos EHCI/OHCI

[Felipe Balbi]

On Thu, Jun 26, 2014 at 11:09:37AM +0530, Sachin Kamat wrote:
 EHCI and OHCI drivers on Exynos platforms do not work without their
 corresponding SoC specific phy drivers. Hence it makes no sense to
 keep these phy drivers as user selectable. Instead select them from
 the respective USB configs to make things easier for the end user.
 While at it enable 5250 phy for Exynos 5420 SoC too.
 
 Signed-off-by: Sachin Kamat sachin.ka...@samsung.com
 Cc: Kishon Vijay Abraham I kis...@ti.com

no more selects, please. We've already had way too many issues because
of misused selects.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 1/2] usb: host: Kconfig: Select PHY drivers for Exynos EHCI/OHCI

[Felipe Balbi]

On Fri, Jun 27, 2014 at 08:55:31AM -0700, Doug Anderson wrote:
 Felipe,
 
 On Fri, Jun 27, 2014 at 8:46 AM, Felipe Balbi ba...@ti.com wrote:
  On Thu, Jun 26, 2014 at 11:09:37AM +0530, Sachin Kamat wrote:
  EHCI and OHCI drivers on Exynos platforms do not work without their
  corresponding SoC specific phy drivers. Hence it makes no sense to
  keep these phy drivers as user selectable. Instead select them from
  the respective USB configs to make things easier for the end user.
  While at it enable 5250 phy for Exynos 5420 SoC too.
 
  Signed-off-by: Sachin Kamat sachin.ka...@samsung.com
  Cc: Kishon Vijay Abraham I kis...@ti.com
 
  no more selects, please. We've already had way too many issues because
  of misused selects.
 
 I'll admit to not having been involved with the previous discussions,
 but this seems strange to me.  Are we throwing in the towel and
 deciding that it's too hard to get the Kconfigs right and that we'll
 just rely on individual users to figure out the right answer for
 themselves?

no. select prevents a driver from be built as a dynamically linked
module and distro-kernels might want to enable everything as modules.

 Certainly the Exynos USB driver is not useful without the Exynos USB
 Phy driver, so it seems awfully strange to allow the user to select
 one without getting the other...  ...and if including an extra USB Phy
 driver will break something then it seems like we have bigger problems
 (aren't we supposed to have one kernel that works across a wide
 variety of boards?)

yeah, but for the kernel to work it doesn't depend on the PHY driver,
does it ? The USB parts of the SoC depend on the PHY, but even PHY
drivers should be allowed to be built as modules. Find another way

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH] phy: exynos5-usbdrd: Add facility to toggle vbus gpio on/off

[Felipe Balbi]

On Wed, Apr 09, 2014 at 05:24:45PM +0530, Vivek Gautam wrote:
 Adding support to enable/disable VBUS hooked to a gpio
 to enable vbus supply on the port.
 
 Signed-off-by: Vivek Gautam gautam.vi...@samsung.com
 ---
 
 Based on 'phy-exynos5-usbdrd' patches:
 [PATCH V4 0/5] Add Exynos5 USB 3.0 phy driver based on generic PHY framework
 http://www.spinics.net/lists/linux-usb/msg105507.html
 
  drivers/phy/phy-exynos5-usbdrd.c |   18 ++
  1 file changed, 18 insertions(+)
 
 diff --git a/drivers/phy/phy-exynos5-usbdrd.c 
 b/drivers/phy/phy-exynos5-usbdrd.c
 index ff54a7c..5ca7485 100644
 --- a/drivers/phy/phy-exynos5-usbdrd.c
 +++ b/drivers/phy/phy-exynos5-usbdrd.c
 @@ -18,6 +18,7 @@
  #include linux/module.h
  #include linux/of.h
  #include linux/of_address.h
 +#include linux/of_gpio.h
  #include linux/phy/phy.h
  #include linux/platform_device.h
  #include linux/mutex.h
 @@ -176,6 +177,7 @@ struct exynos5_usbdrd_phy {
   struct clk *ref_clk;
   unsigned long ref_rate;
   unsigned int refclk_reg;
 + int gpio;
  };
  
  #define to_usbdrd_phy(inst) \
 @@ -460,6 +462,9 @@ static int exynos5_usbdrd_phy_power_on(struct phy *phy)
   if (!IS_ERR(phy_drd-usb30_sclk))
   clk_prepare_enable(phy_drd-usb30_sclk);
  
 + /* Toggle VBUS gpio - on */
 + gpio_set_value(phy_drd-gpio, 1);

It seems like you'd be better off using a regulator_enable() call for
this.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 0/7] usb: dwc3: pm_runtime implementation

[Felipe Balbi]

Hi,

On Tue, Dec 17, 2013 at 03:35:54PM -0800, David Cohen wrote:
 On Tue, Dec 17, 2013 at 03:31:40PM -0800, David Cohen wrote:
  On Thu, Dec 12, 2013 at 03:38:38PM -0600, Felipe Balbi wrote:
   hi all,
   
   these patches add pm_runtime support for all glue layers.
   
   I plan to add pm_runtime support for dwc3 after these
   patches are merged upstream.
   
   Please test.
  
  At first time I failed to notice you were removing #ifdef's around pm
  callback functions. Instead of saying DWC3 will start to have warnings
  when CONFIG_PM is not selected, I'd say your patch set is now a
  dependence of my RFC :)
 
 I guess I said it in wrong order :P
 Your patch set depends on my RFC.

Or I just modify my patchset a little bit for now ;-)

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 0/7] usb: dwc3: pm_runtime implementation

[Felipe Balbi]

On Wed, Dec 18, 2013 at 09:36:14AM -0600, Felipe Balbi wrote:
 Hi,
 
 On Tue, Dec 17, 2013 at 03:35:54PM -0800, David Cohen wrote:
  On Tue, Dec 17, 2013 at 03:31:40PM -0800, David Cohen wrote:
   On Thu, Dec 12, 2013 at 03:38:38PM -0600, Felipe Balbi wrote:
hi all,

these patches add pm_runtime support for all glue layers.

I plan to add pm_runtime support for dwc3 after these
patches are merged upstream.

Please test.
   
   At first time I failed to notice you were removing #ifdef's around pm
   callback functions. Instead of saying DWC3 will start to have warnings
   when CONFIG_PM is not selected, I'd say your patch set is now a
   dependence of my RFC :)
  
  I guess I said it in wrong order :P
  Your patch set depends on my RFC.
 
 Or I just modify my patchset a little bit for now ;-)

nah, it looks horrible with all those ifdefs around. I'll delay my
patch series.

-- 
balbi


signature.asc
Description: Digital signature

Re: [RFC 5/5] usb: dwc3: enable async suspend/resume

[Felipe Balbi]

On Wed, Dec 18, 2013 at 04:09:34PM +0530, Yuvaraj Kumar C D wrote:
 From: Andrew Bresticker abres...@chromium.org
 
 In addition to enabling async suspend/resume on the xhci-plat device,
 we must enable it for the dwc3 device (the parent of xhci-plat) in order
 to make the full USB stack resume asynchronously.  Like the xhci-plat,
 ehci-s5p, and ohci-exynos drivers, there are no outside dependencies
 which would make resuming the dwc3 driver asynchronously unsafe.
 
 Signed-off-by: Andrew Bresticker abres...@chromium.org
 Reviewed-by: Julius Werner jwer...@chromium.org
 Signed-off-by: Yuvaraj Kumar C D yuvaraj...@samsung.com
 ---
  drivers/usb/dwc3/core.c |2 ++
  1 file changed, 2 insertions(+)
 
 diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
 index 59bb8d2..9c8a273 100644
 --- a/drivers/usb/dwc3/core.c
 +++ b/drivers/usb/dwc3/core.c
 @@ -586,6 +586,8 @@ static int dwc3_probe(struct platform_device *pdev)
  
   pm_runtime_allow(dev);
  
 + device_enable_async_suspend(dev);

how has this series been tested ? Let me rephrase this: was this series
tested at all ? Note that if we are in gadget mode, we will disable
physical endpoints 0 and 1, which will break USB communication requiring
a new enumeration later. On top of that, for versions of the core which
are configured without hibernation support - in fact for all cores
currently, since we don't have hibernation support implemented in this
driver -, we will loose communication with the far end (be it a host or
a device).

You mention there are no external dependencies to make async suspend
work on this driver, but that's far from the truth. As it is today, this
driver needs a lot of work to learn about all the details about all
different versions of this IP when it comes to supporting async PM.

I suppose this was tested with 500 other out-of-tree patches and you
simply cherry-picked this patch to send upstream ? Am I right ? It
certainly looks like it.

Please let us know how has this been tested ? Did you run USB30CV ? Did
you make sure to run through USB Certification interoperability tests ?

Did you leave some stress test running for weeks before sending this
patch out ? Is Exynos 5 working fine out of mainline ?

cheers

-- 
balbi


signature.asc
Description: Digital signature

Re: [RFC 4/5] usb: dwc3-exynos: enable async suspend/resume

[Felipe Balbi]

On Wed, Dec 18, 2013 at 04:09:33PM +0530, Yuvaraj Kumar C D wrote:
 From: Andrew Bresticker abres...@chromium.org
 
 In addition to enabling async suspend/resume on the xhci-plat device,
 we must enable it for the dwc3-exynos platform device in order to make
 the full USB stack resume asynchronously.  Like the xhci-plat, ehci-s5p,
 and ohci-exynos drivers, there are no outside dependencies which would
 make resuming the dwc3-exynos driver asynchronously unsafe.
 
 Signed-off-by: Andrew Bresticker abres...@chromium.org
 Reviewed-by: Julius Werner jwer...@chromium.org
 Signed-off-by: Yuvaraj Kumar C D yuvaraj...@samsung.com
 ---
  drivers/usb/dwc3/dwc3-exynos.c |2 ++
  1 file changed, 2 insertions(+)
 
 diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
 index 8b20c70..57431b7 100644
 --- a/drivers/usb/dwc3/dwc3-exynos.c
 +++ b/drivers/usb/dwc3/dwc3-exynos.c
 @@ -155,6 +155,8 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
   goto err2;
   }
  
 + device_enable_async_suspend(dev);

sure that clk_disable() in your -suspend() callback will cause no
issues at all ?

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v2 1/7] usb: dwc3: keystone: add basic PM support

[Felipe Balbi]

On Fri, Dec 13, 2013 at 10:04:38AM -0600, Kwok, WingMan wrote:
 Hello
 
  -Original Message-
  From: Shilimkar, Santosh
  Sent: Thursday, December 12, 2013 7:29 PM
  To: Balbi, Felipe
  Cc: Linux USB Mailing List; kgene@samsung.com; Linux ARM Kernel
  Mailing List; linux-samsung-soc@vger.kernel.org; Linux OMAP Mailing List;
  Kwok, WingMan
  Subject: Re: [PATCH v2 1/7] usb: dwc3: keystone: add basic PM support
  
  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.
  
 
 I have tested the patch and the keystone usb driver continues to function,
 though I can't test suspend at this time as the rest of the system does
 not that functionality yet.

Thanks, should I add your Tested-by ?

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 7/7] usb: dwc3: exynos: add pm_runtime support

[Felipe Balbi]

On Fri, Dec 13, 2013 at 02:01:32PM +0900, Anton Tikhomirov wrote:
 Hi Felipe,
 
  -static int dwc3_exynos_suspend(struct device *dev)
  +static int __dwc3_exynos_suspend(struct dwc3_exynos *exynos)
   {
  -   struct dwc3_exynos *exynos = dev_get_drvdata(dev);
  -
  clk_disable(exynos-clk);
  
  return 0;
   }
  
  +static int __dwc3_exynos_resume(struct dwc3_exynos *exynos)
  +{
  +   return clk_enable(exynos-clk);
  +}
  +
  +static int dwc3_exynos_suspend(struct device *dev)
  +{
  +   struct dwc3_exynos *exynos = dev_get_drvdata(dev);
  +
  +   return __dwc3_exynos_suspend(exynos);
 
 If dwc3-exynos is runtime suspended, the clock will be disabled
 second time here (unbalanced clk_enable/clk_disable).

I don't get what you mean but there is something that probably needs
fixing, I guess below makes it better:

diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
index c93919a..1e5720a 100644
--- a/drivers/usb/dwc3/dwc3-exynos.c
+++ b/drivers/usb/dwc3/dwc3-exynos.c
@@ -218,6 +218,9 @@ static int dwc3_exynos_suspend(struct device *dev)
 {
struct dwc3_exynos *exynos = dev_get_drvdata(dev);
 
+   if (pm_runtime_suspended(dev))
+   return 0;
+
return __dwc3_exynos_suspend(exynos);
 }
 

Is that what you meant ?

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 7/7] usb: dwc3: exynos: add pm_runtime support

[Felipe Balbi]

On Fri, Dec 13, 2013 at 01:56:18PM -0600, Felipe Balbi wrote:
 On Fri, Dec 13, 2013 at 02:01:32PM +0900, Anton Tikhomirov wrote:
  Hi Felipe,
  
   -static int dwc3_exynos_suspend(struct device *dev)
   +static int __dwc3_exynos_suspend(struct dwc3_exynos *exynos)
{
   - struct dwc3_exynos *exynos = dev_get_drvdata(dev);
   -
 clk_disable(exynos-clk);
   
 return 0;
}
   
   +static int __dwc3_exynos_resume(struct dwc3_exynos *exynos)
   +{
   + return clk_enable(exynos-clk);
   +}
   +
   +static int dwc3_exynos_suspend(struct device *dev)
   +{
   + struct dwc3_exynos *exynos = dev_get_drvdata(dev);
   +
   + return __dwc3_exynos_suspend(exynos);
  
  If dwc3-exynos is runtime suspended, the clock will be disabled
  second time here (unbalanced clk_enable/clk_disable).
 
 I don't get what you mean but there is something that probably needs
 fixing, I guess below makes it better:
 
 diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
 index c93919a..1e5720a 100644
 --- a/drivers/usb/dwc3/dwc3-exynos.c
 +++ b/drivers/usb/dwc3/dwc3-exynos.c
 @@ -218,6 +218,9 @@ static int dwc3_exynos_suspend(struct device *dev)
  {
   struct dwc3_exynos *exynos = dev_get_drvdata(dev);
  
 + if (pm_runtime_suspended(dev))
 + return 0;
 +
   return __dwc3_exynos_suspend(exynos);
  }
  
 
 Is that what you meant ?

note, however, that this is *not* a case where we would fall today. See
that we pm_runtime_get() in probe and only pm_runtime_put() during
remove. So there would never be a case where we would try system suspend
while device was already runtime suspended.

I have fixed all patches in my testing/next branch anyway, just to make
sure we're idiot-proof when it comes to implementing real runtime pm
later on :-)

cheers

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v2 1/7] usb: dwc3: keystone: add basic PM support

[Felipe Balbi]

On Fri, Dec 13, 2013 at 02:18:42PM -0600, Kwok, WingMan wrote:
 
  -Original Message-
  From: Balbi, Felipe
  Sent: Friday, December 13, 2013 2:55 PM
  To: Kwok, WingMan
  Cc: Shilimkar, Santosh; Balbi, Felipe; Linux USB Mailing List;
  kgene@samsung.com; Linux ARM Kernel Mailing List; linux-samsung-
  s...@vger.kernel.org; Linux OMAP Mailing List
  Subject: Re: [PATCH v2 1/7] usb: dwc3: keystone: add basic PM support
  
  On Fri, Dec 13, 2013 at 10:04:38AM -0600, Kwok, WingMan wrote:
   Hello
  
-Original Message-
From: Shilimkar, Santosh
Sent: Thursday, December 12, 2013 7:29 PM
To: Balbi, Felipe
Cc: Linux USB Mailing List; kgene@samsung.com; Linux ARM Kernel
Mailing List; linux-samsung-soc@vger.kernel.org; Linux OMAP Mailing
List; Kwok, WingMan
Subject: Re: [PATCH v2 1/7] usb: dwc3: keystone: add basic PM
support
   
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.
   
  
   I have tested the patch and the keystone usb driver continues to
   function, though I can't test suspend at this time as the rest of the
   system does not that functionality yet.
  
  Thanks, should I add your Tested-by ?
 
 Yes please.

you need to reply with Tested-by: Your Name your.em...@domain.com just
to make it all official. Sorry

-- 
balbi


signature.asc
Description: Digital signature

[PATCH 7/7] usb: dwc3: exynos: add pm_runtime support

[Felipe Balbi]

teach Exynos glue about pm_runtime so that
it's easier to teach dwc3 core about it
later.

No functional changes otherwise.

Signed-off-by: Felipe Balbi ba...@ti.com
---
 drivers/usb/dwc3/dwc3-exynos.c | 65 --
 1 file changed, 56 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
index 6ccfc64..c93919a 100644
--- a/drivers/usb/dwc3/dwc3-exynos.c
+++ b/drivers/usb/dwc3/dwc3-exynos.c
@@ -141,24 +141,40 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
exynos-dev = dev;
exynos-clk = clk;
 
-   clk_prepare_enable(exynos-clk);
+   ret = clk_prepare(exynos-clk);
+   if (ret) {
+   dev_err(dev, failed to prepare clock\n);
+   goto err2;
+   }
+
+   pm_runtime_enable(dev);
+   ret = pm_runtime_get_sync(dev);
+   if (ret) {
+   dev_err(dev, failed to enable dwc3 device\n);
+   goto err3;
+   }
 
if (node) {
ret = of_platform_populate(node, NULL, NULL, dev);
if (ret) {
dev_err(dev, failed to add dwc3 core\n);
-   goto err2;
+   goto err3;
}
} else {
dev_err(dev, no device node, failed to add dwc3 core\n);
ret = -ENODEV;
-   goto err2;
+   goto err3;
}
 
return 0;
 
+err3:
+   pm_runtime_put_sync(dev);
+   pm_runtime_disable(dev);
+
 err2:
-   clk_disable_unprepare(clk);
+   clk_unprepare(clk);
+
 err1:
return ret;
 }
@@ -171,7 +187,9 @@ static int dwc3_exynos_remove(struct platform_device *pdev)
platform_device_unregister(exynos-usb2_phy);
platform_device_unregister(exynos-usb3_phy);
 
-   clk_disable_unprepare(exynos-clk);
+   pm_runtime_put_sync(exynos-dev);
+   pm_runtime_disable(exynos-dev);
+   clk_unprepare(exynos-clk);
 
return 0;
 }
@@ -184,20 +202,33 @@ static const struct of_device_id exynos_dwc3_match[] = {
 MODULE_DEVICE_TABLE(of, exynos_dwc3_match);
 #endif
 
-static int dwc3_exynos_suspend(struct device *dev)
+static int __dwc3_exynos_suspend(struct dwc3_exynos *exynos)
 {
-   struct dwc3_exynos *exynos = dev_get_drvdata(dev);
-
clk_disable(exynos-clk);
 
return 0;
 }
 
+static int __dwc3_exynos_resume(struct dwc3_exynos *exynos)
+{
+   return clk_enable(exynos-clk);
+}
+
+static int dwc3_exynos_suspend(struct device *dev)
+{
+   struct dwc3_exynos *exynos = dev_get_drvdata(dev);
+
+   return __dwc3_exynos_suspend(exynos);
+}
+
 static int dwc3_exynos_resume(struct device *dev)
 {
struct dwc3_exynos *exynos = dev_get_drvdata(dev);
+   int ret;
 
-   clk_enable(exynos-clk);
+   ret = __dwc3_exynos_resume(exynos);
+   if (ret)
+   return ret;
 
/* runtime set active to reflect active state. */
pm_runtime_disable(dev);
@@ -207,8 +238,24 @@ static int dwc3_exynos_resume(struct device *dev)
return 0;
 }
 
+static int dwc3_exynos_runtime_suspend(struct device *dev)
+{
+   struct dwc3_exynos *exynos = dev_get_drvdata(dev);
+
+   return __dwc3_exynos_suspend(exynos);
+}
+
+static int dwc3_exynos_runtime_resume(struct device *dev)
+{
+   struct dwc3_exynos *exynos = dev_get_drvdata(dev);
+
+   return __dwc3_exynos_resume(exynos);
+}
+
 static const struct dev_pm_ops dwc3_exynos_dev_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(dwc3_exynos_suspend, dwc3_exynos_resume)
+   SET_RUNTIME_PM_OPS(dwc3_exynos_runtime_suspend,
+   dwc3_exynos_runtime_resume, NULL)
 };
 
 static struct platform_driver dwc3_exynos_driver = {
-- 
1.8.4.GIT

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/7] usb: dwc3: omap: add basic pm_runtime support

[Felipe Balbi]

If we want to suspend/runtime in runtime, we
can do so, in OMAP's case at least, with the
same implementation we use for system pm.

This patch adds basic pm_runtime support with
that in mind.

Signed-off-by: Felipe Balbi ba...@ti.com
---
 drivers/usb/dwc3/dwc3-omap.c | 39 +++
 1 file changed, 35 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
index b269dbd..aea305d 100644
--- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -617,20 +617,35 @@ static void dwc3_omap_complete(struct device *dev)
dwc3_omap_enable_irqs(omap);
 }
 
-static int dwc3_omap_suspend(struct device *dev)
+static int __dwc3_omap_suspend(struct dwc3_omap *omap)
 {
-   struct dwc3_omap*omap = dev_get_drvdata(dev);
-
omap-utmi_otg_status = dwc3_omap_read_utmi_status(omap);
 
return 0;
 }
 
+static int __dwc3_omap_resume(struct dwc3_omap *omap)
+{
+   dwc3_omap_write_utmi_status(omap, omap-utmi_otg_status);
+
+   return 0;
+}
+
+static int dwc3_omap_suspend(struct device *dev)
+{
+   struct dwc3_omap*omap = dev_get_drvdata(dev);
+
+   return __dwc3_omap_suspend(omap);
+}
+
 static int dwc3_omap_resume(struct device *dev)
 {
struct dwc3_omap*omap = dev_get_drvdata(dev);
+   int ret;
 
-   dwc3_omap_write_utmi_status(omap, omap-utmi_otg_status);
+   ret = __dwc3_omap_resume(omap);
+   if (ret)
+   return ret;
 
pm_runtime_disable(dev);
pm_runtime_set_active(dev);
@@ -639,11 +654,27 @@ static int dwc3_omap_resume(struct device *dev)
return 0;
 }
 
+static int dwc3_omap_runtime_suspend(struct device *dev)
+{
+   struct dwc3_omap*omap = dev_get_drvdata(dev);
+
+   return __dwc3_omap_suspend(omap);
+}
+
+static int dwc3_omap_runtime_resume(struct device *dev)
+{
+   struct dwc3_omap*omap = dev_get_drvdata(dev);
+
+   return __dwc3_omap_resume(omap);
+}
+
 static const struct dev_pm_ops dwc3_omap_dev_pm_ops = {
.prepare= dwc3_omap_prepare,
.complete   = dwc3_omap_complete,
 
SET_SYSTEM_SLEEP_PM_OPS(dwc3_omap_suspend, dwc3_omap_resume)
+   SET_RUNTIME_PM_OPS(dwc3_omap_runtime_suspend, dwc3_omap_runtime_resume,
+   NULL)
 };
 
 #define DEV_PM_OPS (dwc3_omap_dev_pm_ops)
-- 
1.8.4.GIT

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/7] usb: dwc3: omap: fix pm_runtime usage

[Felipe Balbi]

even if pm_runtime_get*() fails, it still increments
pm usage counter, so we must pm_runtime_put*()
in that case too. Fix that observation in dwc3-omap.c.

Signed-off-by: Felipe Balbi ba...@ti.com
---
 drivers/usb/dwc3/dwc3-omap.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
index aea305d..076bb28 100644
--- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -451,7 +451,7 @@ static int dwc3_omap_probe(struct platform_device *pdev)
ret = pm_runtime_get_sync(dev);
if (ret  0) {
dev_err(dev, get_sync failed with err %d\n, ret);
-   goto err0;
+   goto err1;
}
 
reg = dwc3_omap_readl(omap-base, USBOTGSS_REVISION);
@@ -566,8 +566,6 @@ err2:
 
 err1:
pm_runtime_put_sync(dev);
-
-err0:
pm_runtime_disable(dev);
 
return ret;
-- 
1.8.4.GIT

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/7] usb: dwc3: pci: add pm_runtime support

[Felipe Balbi]

teach the PCI glue about pm_runtime so that
it's easier to teach dwc3 core about it
later.

No functional changes otherwise.

Signed-off-by: Felipe Balbi ba...@ti.com
---
 drivers/usb/dwc3/dwc3-pci.c | 66 ++---
 1 file changed, 51 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
index 665686e..78cd0b0 100644
--- a/drivers/usb/dwc3/dwc3-pci.c
+++ b/drivers/usb/dwc3/dwc3-pci.c
@@ -109,18 +109,17 @@ static int dwc3_pci_probe(struct pci_dev *pci,
 
glue-dev = dev;
 
-   ret = pci_enable_device(pci);
+   pm_runtime_enable(dev);
+   ret = pm_runtime_get_sync(dev);
if (ret) {
dev_err(dev, failed to enable pci device\n);
return -ENODEV;
}
 
-   pci_set_master(pci);
-
ret = dwc3_pci_register_phys(glue);
if (ret) {
dev_err(dev, couldn't register PHYs\n);
-   return ret;
+   goto err1;
}
 
dwc3 = platform_device_alloc(dwc3, PLATFORM_DEVID_AUTO);
@@ -167,7 +166,8 @@ static int dwc3_pci_probe(struct pci_dev *pci,
 err3:
platform_device_put(dwc3);
 err1:
-   pci_disable_device(pci);
+   pm_runtime_put_sync(dev);
+   pm_runtime_disable(dev);
 
return ret;
 }
@@ -175,11 +175,13 @@ err1:
 static void dwc3_pci_remove(struct pci_dev *pci)
 {
struct dwc3_pci *glue = pci_get_drvdata(pci);
+   struct device *dev = pci-dev;
 
platform_device_unregister(glue-dwc3);
platform_device_unregister(glue-usb2_phy);
platform_device_unregister(glue-usb3_phy);
-   pci_disable_device(pci);
+   pm_runtime_put_sync(dev);
+   pm_runtime_disable(dev);
 }
 
 static const struct pci_device_id dwc3_pci_id_table[] = {
@@ -193,24 +195,20 @@ static const struct pci_device_id dwc3_pci_id_table[] = {
 };
 MODULE_DEVICE_TABLE(pci, dwc3_pci_id_table);
 
-#ifdef CONFIG_PM_SLEEP
-static int dwc3_pci_suspend(struct device *dev)
+static int __dwc3_pci_suspend(struct pci_dev *pci)
 {
-   struct pci_dev  *pci = to_pci_dev(dev);
-
pci_disable_device(pci);
 
return 0;
 }
 
-static int dwc3_pci_resume(struct device *dev)
+static int __dwc3_pci_resume(struct pci_dev *pci)
 {
-   struct pci_dev  *pci = to_pci_dev(dev);
-   int ret;
+   int ret;
 
ret = pci_enable_device(pci);
if (ret) {
-   dev_err(dev, can't re-enable device -- %d\n, ret);
+   dev_err(pci-dev, can't re-enable device -- %d\n, ret);
return ret;
}
 
@@ -218,10 +216,48 @@ static int dwc3_pci_resume(struct device *dev)
 
return 0;
 }
-#endif /* CONFIG_PM_SLEEP */
+
+static int dwc3_pci_suspend(struct device *dev)
+{
+   struct pci_dev  *pci = to_pci_dev(dev);
+
+   return __dwc3_pci_suspend(pci);
+}
+
+static int dwc3_pci_resume(struct device *dev)
+{
+   struct pci_dev  *pci = to_pci_dev(dev);
+   int ret;
+
+   ret = __dwc3_pci_resume(pci);
+   if (ret)
+   return ret;
+
+   pm_runtime_disable(dev);
+   pm_runtime_set_active(dev);
+   pm_runtime_disable(dev);
+
+   return 0;
+}
+
+static int dwc3_pci_runtime_suspend(struct device *dev)
+{
+   struct pci_dev  *pci = to_pci_dev(dev);
+
+   return __dwc3_pci_suspend(pci);
+}
+
+static int dwc3_pci_runtime_resume(struct device *dev)
+{
+   struct pci_dev  *pci = to_pci_dev(dev);
+
+   return __dwc3_pci_resume(pci);
+}
 
 static const struct dev_pm_ops dwc3_pci_dev_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(dwc3_pci_suspend, dwc3_pci_resume)
+   SET_RUNTIME_PM_OPS(dwc3_pci_runtime_suspend, dwc3_pci_runtime_resume,
+   NULL)
 };
 
 static struct pci_driver dwc3_pci_driver = {
-- 
1.8.4.GIT

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/7] usb: dwc3: omap: fix order of pm_runtime vs child removal

[Felipe Balbi]

pm_runtime_put_sync() will kill dwc3's clocks and,
since dwc3 core accesses registers during removal,
we must make sure to unregister core before
disabling clocks and pm_runtime.

Signed-off-by: Felipe Balbi ba...@ti.com
---
 drivers/usb/dwc3/dwc3-omap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
index 076bb28..a9090a1 100644
--- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -580,9 +580,9 @@ static int dwc3_omap_remove(struct platform_device *pdev)
if (omap-extcon_id_dev.edev)
extcon_unregister_interest(omap-extcon_id_dev);
dwc3_omap_disable_irqs(omap);
+   device_for_each_child(pdev-dev, NULL, dwc3_omap_remove_core);
pm_runtime_put_sync(pdev-dev);
pm_runtime_disable(pdev-dev);
-   device_for_each_child(pdev-dev, NULL, dwc3_omap_remove_core);
 
return 0;
 }
-- 
1.8.4.GIT

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/7] usb: dwc3: pm_runtime implementation

[Felipe Balbi]

hi all,

these patches add pm_runtime support for all glue layers.

I plan to add pm_runtime support for dwc3 after these
patches are merged upstream.

Please test.

Felipe Balbi (7):
  usb: dwc3: keystone: add basic PM support
  usb: dwc3: omap: add basic pm_runtime support
  usb: dwc3: pci: add pm_runtime support
  usb: dwc3: omap: fix pm_runtime usage
  usb: dwc3: omap: fix order of pm_runtime vs child removal
  usb: dwc3: exynos: remove DEV_PM_OPS hackery
  usb: dwc3: exynos: add pm_runtime support

 drivers/usb/dwc3/dwc3-exynos.c   | 73 +++---
 drivers/usb/dwc3/dwc3-keystone.c | 97 ++--
 drivers/usb/dwc3/dwc3-omap.c | 45 +++
 drivers/usb/dwc3/dwc3-pci.c  | 66 ---
 4 files changed, 239 insertions(+), 42 deletions(-)

-- 
1.8.4.GIT

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/7] usb: dwc3: exynos: remove DEV_PM_OPS hackery

[Felipe Balbi]

it's best to just remove all ifdefs and always
define our dev_pm_ops structure.

Signed-off-by: Felipe Balbi ba...@ti.com
---
 drivers/usb/dwc3/dwc3-exynos.c | 8 +---
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
index 8b20c70..6ccfc64 100644
--- a/drivers/usb/dwc3/dwc3-exynos.c
+++ b/drivers/usb/dwc3/dwc3-exynos.c
@@ -184,7 +184,6 @@ static const struct of_device_id exynos_dwc3_match[] = {
 MODULE_DEVICE_TABLE(of, exynos_dwc3_match);
 #endif
 
-#ifdef CONFIG_PM_SLEEP
 static int dwc3_exynos_suspend(struct device *dev)
 {
struct dwc3_exynos *exynos = dev_get_drvdata(dev);
@@ -212,18 +211,13 @@ static const struct dev_pm_ops dwc3_exynos_dev_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(dwc3_exynos_suspend, dwc3_exynos_resume)
 };
 
-#define DEV_PM_OPS (dwc3_exynos_dev_pm_ops)
-#else
-#define DEV_PM_OPS NULL
-#endif /* CONFIG_PM_SLEEP */
-
 static struct platform_driver dwc3_exynos_driver = {
.probe  = dwc3_exynos_probe,
.remove = dwc3_exynos_remove,
.driver = {
.name   = exynos-dwc3,
.of_match_table = of_match_ptr(exynos_dwc3_match),
-   .pm = DEV_PM_OPS,
+   .pm = dwc3_exynos_dev_pm_ops,
},
 };
 
-- 
1.8.4.GIT

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/7] usb: dwc3: keystone: add basic PM support

[Felipe Balbi]

A bare-minimum PM implementation which will
server as building block for more complex
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
---
 drivers/usb/dwc3/dwc3-keystone.c | 97 ++--
 1 file changed, 94 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-keystone.c b/drivers/usb/dwc3/dwc3-keystone.c
index 1fad161..361437f 100644
--- a/drivers/usb/dwc3/dwc3-keystone.c
+++ b/drivers/usb/dwc3/dwc3-keystone.c
@@ -21,6 +21,7 @@
 #include linux/interrupt.h
 #include linux/platform_device.h
 #include linux/dma-mapping.h
+#include linux/pm_runtime.h
 #include linux/io.h
 #include linux/of_platform.h
 
@@ -118,13 +119,23 @@ static int kdwc3_probe(struct platform_device *pdev)
 
kdwc-clk = devm_clk_get(kdwc-dev, usb);
 
-   error = clk_prepare_enable(kdwc-clk);
+   error = clk_prepare(kdwc-clk);
if (error  0) {
dev_dbg(kdwc-dev, unable to enable usb clock, err %d\n,
error);
return error;
}
 
+   pm_runtime_enable(dev);
+
+   error = pm_runtime_get_sync(dev);
+   if (error  0) {
+   dev_dbg(dev, unable to pm_runtime_get_sync(), err %d\n,
+   error);
+   pm_runtime_put_sync(dev);
+   goto err_runtime_get;
+   }
+
irq = platform_get_irq(pdev, 0);
if (irq  0) {
dev_err(pdev-dev, missing irq\n);
@@ -151,8 +162,13 @@ 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);
+
+err_runtime_get:
+   pm_runtime_disable(dev);
+   clk_unprepare(kdwc-clk);
 
return error;
 }
@@ -172,7 +188,9 @@ 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);
+   clk_unprepare(kdwc-clk);
platform_set_drvdata(pdev, NULL);
 
return 0;
@@ -184,6 +202,79 @@ static const struct of_device_id kdwc3_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, kdwc3_of_match);
 
+static int __kdwc3_suspend(struct dwc3_keystone *kdwc)
+{
+   clk_disable(kdwc-clk);
+
+   return 0;
+}
+
+static int __kdwc3_resume(struct dwc3_keystone *kdwc)
+{
+   return clk_enable(kdwc-clk);
+}
+
+static int kdwc3_prepare(struct device *dev)
+{
+   struct dwc3_keystone*kdwc = dev_get_drvdata(dev);
+
+   kdwc3_disable_irqs(kdwc);
+
+   return 0;
+}
+
+static void kdwc3_complete(struct device *dev)
+{
+   struct dwc3_keystone*kdwc = dev_get_drvdata(dev);
+
+   kdwc3_enable_irqs(kdwc);
+}
+
+static int kdwc3_suspend(struct device *dev)
+{
+   struct dwc3_keystone*kdwc = dev_get_drvdata(dev);
+
+   return __kdwc3_suspend(kdwc);
+}
+
+static int kdwc3_resume(struct device *dev)
+{
+   struct dwc3_keystone*kdwc = dev_get_drvdata(dev);
+   int ret;
+
+   ret = __kdwc3_resume(kdwc);
+   if (ret)
+   return ret;
+
+   pm_runtime_disable(dev);
+   pm_runtime_set_active(dev);
+   pm_runtime_enable(dev);
+
+   return 0;
+}
+
+static int kdwc3_runtime_suspend(struct device *dev)
+{
+   struct dwc3_keystone *kdwc = dev_get_drvdata(dev);
+
+   return __kdwc3_suspend(kdwc);
+}
+
+static int kdwc3_runtime_resume(struct device *dev)
+{
+   struct dwc3_keystone *kdwc = dev_get_drvdata(dev);
+
+   return __kdwc3_resume(kdwc);
+}
+
+static const struct dev_pm_ops kdwc3_dev_pm_ops = {
+   .prepare= kdwc3_prepare,
+   .complete   = kdwc3_complete,
+
+   SET_SYSTEM_SLEEP_PM_OPS(kdwc3_suspend, kdwc3_resume)
+   SET_RUNTIME_PM_OPS(kdwc3_runtime_suspend, kdwc3_runtime_resume, NULL)
+};
+
 static struct platform_driver kdwc3_driver = {
.probe  = kdwc3_probe,
.remove = kdwc3_remove,
-- 
1.8.4.GIT

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc 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/7] usb: dwc3: keystone: add basic PM support

[Felipe Balbi]

On Thu, Dec 12, 2013 at 03:38:39PM -0600, Felipe Balbi wrote:
 A bare-minimum PM implementation which will
 server as building block for more complex
 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
 ---
  drivers/usb/dwc3/dwc3-keystone.c | 97 
 ++--
  1 file changed, 94 insertions(+), 3 deletions(-)
 
 diff --git a/drivers/usb/dwc3/dwc3-keystone.c 
 b/drivers/usb/dwc3/dwc3-keystone.c
 index 1fad161..361437f 100644
 --- a/drivers/usb/dwc3/dwc3-keystone.c
 +++ b/drivers/usb/dwc3/dwc3-keystone.c
 @@ -21,6 +21,7 @@
  #include linux/interrupt.h
  #include linux/platform_device.h
  #include linux/dma-mapping.h
 +#include linux/pm_runtime.h
  #include linux/io.h
  #include linux/of_platform.h
  
 @@ -118,13 +119,23 @@ static int kdwc3_probe(struct platform_device *pdev)
  
   kdwc-clk = devm_clk_get(kdwc-dev, usb);
  
 - error = clk_prepare_enable(kdwc-clk);
 + error = clk_prepare(kdwc-clk);
   if (error  0) {
   dev_dbg(kdwc-dev, unable to enable usb clock, err %d\n,
   error);
   return error;
   }
  
 + pm_runtime_enable(dev);
 +
 + error = pm_runtime_get_sync(dev);
 + if (error  0) {
 + dev_dbg(dev, unable to pm_runtime_get_sync(), err %d\n,
 + error);
 + pm_runtime_put_sync(dev);

I can move this pm_runtime_sync() to error path, will refresh this
patch.

-- 
balbi


signature.asc
Description: Digital signature

[PATCH v2 1/7] usb: dwc3: keystone: add basic PM support

[Felipe Balbi]

A bare-minimum PM implementation which will
server as building block for more complex
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.

 drivers/usb/dwc3/dwc3-keystone.c | 94 ++--
 1 file changed, 91 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-keystone.c b/drivers/usb/dwc3/dwc3-keystone.c
index 1fad161..3b3761c 100644
--- a/drivers/usb/dwc3/dwc3-keystone.c
+++ b/drivers/usb/dwc3/dwc3-keystone.c
@@ -21,6 +21,7 @@
 #include linux/interrupt.h
 #include linux/platform_device.h
 #include linux/dma-mapping.h
+#include linux/pm_runtime.h
 #include linux/io.h
 #include linux/of_platform.h
 
@@ -118,13 +119,22 @@ static int kdwc3_probe(struct platform_device *pdev)
 
kdwc-clk = devm_clk_get(kdwc-dev, usb);
 
-   error = clk_prepare_enable(kdwc-clk);
+   error = clk_prepare(kdwc-clk);
if (error  0) {
dev_dbg(kdwc-dev, unable to enable usb clock, err %d\n,
error);
return error;
}
 
+   pm_runtime_enable(dev);
+
+   error = pm_runtime_get_sync(dev);
+   if (error  0) {
+   dev_dbg(dev, unable to pm_runtime_get_sync(), err %d\n,
+   error);
+   goto err_irq;
+   }
+
irq = platform_get_irq(pdev, 0);
if (irq  0) {
dev_err(pdev-dev, missing irq\n);
@@ -151,8 +161,11 @@ 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);
+   clk_unprepare(kdwc-clk);
 
return error;
 }
@@ -172,7 +185,9 @@ 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);
+   clk_unprepare(kdwc-clk);
platform_set_drvdata(pdev, NULL);
 
return 0;
@@ -184,6 +199,79 @@ static const struct of_device_id kdwc3_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, kdwc3_of_match);
 
+static int __kdwc3_suspend(struct dwc3_keystone *kdwc)
+{
+   clk_disable(kdwc-clk);
+
+   return 0;
+}
+
+static int __kdwc3_resume(struct dwc3_keystone *kdwc)
+{
+   return clk_enable(kdwc-clk);
+}
+
+static int kdwc3_prepare(struct device *dev)
+{
+   struct dwc3_keystone*kdwc = dev_get_drvdata(dev);
+
+   kdwc3_disable_irqs(kdwc);
+
+   return 0;
+}
+
+static void kdwc3_complete(struct device *dev)
+{
+   struct dwc3_keystone*kdwc = dev_get_drvdata(dev);
+
+   kdwc3_enable_irqs(kdwc);
+}
+
+static int kdwc3_suspend(struct device *dev)
+{
+   struct dwc3_keystone*kdwc = dev_get_drvdata(dev);
+
+   return __kdwc3_suspend(kdwc);
+}
+
+static int kdwc3_resume(struct device *dev)
+{
+   struct dwc3_keystone*kdwc = dev_get_drvdata(dev);
+   int ret;
+
+   ret = __kdwc3_resume(kdwc);
+   if (ret)
+   return ret;
+
+   pm_runtime_disable(dev);
+   pm_runtime_set_active(dev);
+   pm_runtime_enable(dev);
+
+   return 0;
+}
+
+static int kdwc3_runtime_suspend(struct device *dev)
+{
+   struct dwc3_keystone *kdwc = dev_get_drvdata(dev);
+
+   return __kdwc3_suspend(kdwc);
+}
+
+static int kdwc3_runtime_resume(struct device *dev)
+{
+   struct dwc3_keystone *kdwc = dev_get_drvdata(dev);
+
+   return __kdwc3_resume(kdwc);
+}
+
+static const struct dev_pm_ops kdwc3_dev_pm_ops = {
+   .prepare= kdwc3_prepare,
+   .complete   = kdwc3_complete,
+
+   SET_SYSTEM_SLEEP_PM_OPS(kdwc3_suspend, kdwc3_resume)
+   SET_RUNTIME_PM_OPS(kdwc3_runtime_suspend, kdwc3_runtime_resume, NULL)
+};
+
 static struct platform_driver kdwc3_driver = {
.probe  = kdwc3_probe,
.remove = kdwc3_remove,
-- 
1.8.4.GIT

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc 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

[Felipe Balbi]

Hi,

On Thu, Dec 12, 2013 at 03:45:55PM -0600, Felipe Balbi wrote:
 +static const struct dev_pm_ops kdwc3_dev_pm_ops = {
 + .prepare= kdwc3_prepare,
 + .complete   = kdwc3_complete,
 +
 + SET_SYSTEM_SLEEP_PM_OPS(kdwc3_suspend, kdwc3_resume)
 + SET_RUNTIME_PM_OPS(kdwc3_runtime_suspend, kdwc3_runtime_resume, NULL)
 +};

did I really forget to use this ? Wonder why compiler didn't annoy me
:-(

there I go to v3. Will fix in my tree only to avoid the extra traffic in
the mailing list.

-- 
balbi


signature.asc
Description: Digital signature

[PATCH] usb: dwc3: omap: remove DEV_PM_OPS trickery

[Felipe Balbi]

it's best to just remove all ifdefs and always
define our dev_pm_ops structure.

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

one more patch

 drivers/usb/dwc3/dwc3-omap.c | 8 +---
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
index a9090a1..14a53ba 100644
--- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -598,7 +598,6 @@ static const struct of_device_id of_dwc3_match[] = {
 };
 MODULE_DEVICE_TABLE(of, of_dwc3_match);
 
-#ifdef CONFIG_PM_SLEEP
 static int dwc3_omap_prepare(struct device *dev)
 {
struct dwc3_omap*omap = dev_get_drvdata(dev);
@@ -675,18 +674,13 @@ static const struct dev_pm_ops dwc3_omap_dev_pm_ops = {
NULL)
 };
 
-#define DEV_PM_OPS (dwc3_omap_dev_pm_ops)
-#else
-#define DEV_PM_OPS NULL
-#endif /* CONFIG_PM_SLEEP */
-
 static struct platform_driver dwc3_omap_driver = {
.probe  = dwc3_omap_probe,
.remove = dwc3_omap_remove,
.driver = {
.name   = omap-dwc3,
.of_match_table = of_dwc3_match,
-   .pm = DEV_PM_OPS,
+   .pm = dwc3_omap_dev_pm_ops,
},
 };
 
-- 
1.8.4.GIT

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc 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

[Felipe Balbi]

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.

thanks.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 3/7] usb: dwc3: pci: add pm_runtime support

[Felipe Balbi]

On Thu, Dec 12, 2013 at 05:56:05PM -0800, David Cohen wrote:
 On Thu, Dec 12, 2013 at 03:38:41PM -0600, Felipe Balbi wrote:
  teach the PCI glue about pm_runtime so that
  it's easier to teach dwc3 core about it
  later.
  
  No functional changes otherwise.
  
  Signed-off-by: Felipe Balbi ba...@ti.com
 
 I was able to test this one with Intel Merrifield:
 Acked-by: David Cohen david.a.co...@linux.intel.com

cool, thanks. Although that'd be a Tested-by. Is it ok to add your name
as Tested-by instead ?

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH] drivers: video: exynos: Fix compiler Warning

[Felipe Balbi]

On Thu, Oct 24, 2013 at 08:57:34PM +0530, Kishon Vijay Abraham I wrote:
 Fixed the following compilation warning:
 ../../drivers/video/exynos/exynos_mipi_dsi.c: In function
 'exynos_mipi_dsi_blank_mode':
 ../../drivers/video/exynos/exynos_mipi_dsi.c:144:26: warning: unused
 variable 'pdev' [-Wunused-variable]
   struct platform_device *pdev = to_platform_device(dsim-dev);
 
 Reported-by: Olof Johansson o...@lixom.net
 Cc: Sylwester Nawrocki s.nawro...@samsung.com
 Signed-off-by: Kishon Vijay Abraham I kis...@ti.com

pretty obvious patch:

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

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 42/51] DMA-API: usb: musb: use platform_device_register_full() to avoid directly messing with dma masks

[Felipe Balbi]

Hi,

On Fri, Sep 20, 2013 at 12:14:38AM +0100, Russell King wrote:
 Use platform_device_register_full() for those drivers which can, to
 avoid messing directly with DMA masks.  This can only be done when
 the driver does not need to access the allocated musb platform device
 from within its callbacks, which may be called during the musb
 device probing.
 
 Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk

you want me to carry this one through my tree or you prefer getting my
Acked-by ? Either way works for me:

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

there's also the third option of me setting up a branch with only this
patch and we both merge it, that'd also work.

cheers

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 42/51] DMA-API: usb: musb: use platform_device_register_full() to avoid directly messing with dma masks

[Felipe Balbi]

Hi,

On Fri, Sep 20, 2013 at 02:49:38PM +0100, Russell King - ARM Linux wrote:
 On Fri, Sep 20, 2013 at 08:11:25AM -0500, Felipe Balbi wrote:
  Hi,
  
  On Fri, Sep 20, 2013 at 12:14:38AM +0100, Russell King wrote:
   Use platform_device_register_full() for those drivers which can, to
   avoid messing directly with DMA masks.  This can only be done when
   the driver does not need to access the allocated musb platform device
   from within its callbacks, which may be called during the musb
   device probing.
   
   Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
  
  you want me to carry this one through my tree or you prefer getting my
  Acked-by ? Either way works for me:
  
  Acked-by: Felipe Balbi ba...@ti.com
  
  there's also the third option of me setting up a branch with only this
  patch and we both merge it, that'd also work.
 
 I think this patch is sufficiently stand-alone that it should be fine
 if you want to take it through your tree.  That may be better in the
 long run to avoid conflicts with this patch and any future work in
 this area during this cycle.

awesome, i'll take this one early next week.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Felipe Balbi]

Hi,

On Tue, Aug 27, 2013 at 01:27:48PM -0700, Julius Werner wrote:
 *Ping!*
 
 Are there still unanswered concerns left with this patch? I hope my
 prior mails could clear up why I think that the PMU register
 description in the device tree for a specific PHY is represents the
 hardware more accurately after my patch, and my analysis of the
 Exynos4 situation currently not being implemented (and therefore not
 needing to be addressed by this patch) was correct. Please let me know
 if you have further objections... and if not, could we agree to have
 this picked up somewhere?

I need acks for DTS part...

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v11 0/8] PHY framework

[Felipe Balbi]

On Wed, Sep 04, 2013 at 02:27:06PM +0530, Kishon Vijay Abraham I wrote:
 On Tuesday 03 September 2013 09:20 PM, Greg KH wrote:
  On Tue, Sep 03, 2013 at 08:55:23PM +0530, Kishon Vijay Abraham I wrote:
  Hi Greg,
 
  On Wednesday 28 August 2013 12:50 AM, Felipe Balbi wrote:
  Hi,
 
  On Mon, Aug 26, 2013 at 01:44:49PM +0530, Kishon Vijay Abraham I wrote:
  On Wednesday 21 August 2013 11:16 AM, Kishon Vijay Abraham I wrote:
  Added a generic PHY framework that provides a set of APIs for the PHY 
  drivers
  to create/destroy a PHY and APIs for the PHY users to obtain a 
  reference to
  the PHY with or without using phandle.
 
  This framework will be of use only to devices that uses external PHY 
  (PHY
  functionality is not embedded within the controller).
 
  The intention of creating this framework is to bring the phy drivers 
  spread
  all over the Linux kernel to drivers/phy to increase code re-use and to
  increase code maintainability.
 
  Comments to make PHY as bus wasn't done because PHY devices can be part 
  of
  other bus and making a same device attached to multiple bus leads to bad
  design.
 
  If the PHY driver has to send notification on connect/disconnect, the 
  PHY
  driver should make use of the extcon framework. Using this susbsystem
  to use extcon framwork will have to be analysed.
 
  You can find this patch series @
  git://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git 
  testing
 
  Looks like there are not further comments on this series. Can you take 
  this in
  your misc tree?
 
  Do you want me to queue these for you ? There are quite a few users for
  this framework already and I know of at least 2 others which will show
  up for v3.13.
 
  Can you queue this patch series? There are quite a few users already for 
  this
  framework.
  
  It will have to wait for 3.13 as the merge window for new features has
  been closed for a week or so.  Sorry, I'll queue this up after 3.12-rc1
  is out.
 
 Alright, thanks.

Just a gentle ping on this one...

cheers

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Felipe Balbi]

On Tue, Sep 17, 2013 at 05:53:31PM +0200, Tomasz Figa wrote:
 Hi Felipe,
 
 On Tuesday 17 of September 2013 10:36:16 Felipe Balbi wrote:
  Hi,
  
  On Tue, Aug 27, 2013 at 01:27:48PM -0700, Julius Werner wrote:
   *Ping!*
   
   Are there still unanswered concerns left with this patch? I hope my
   prior mails could clear up why I think that the PMU register
   description in the device tree for a specific PHY is represents the
   hardware more accurately after my patch, and my analysis of the
   Exynos4 situation currently not being implemented (and therefore not
   needing to be addressed by this patch) was correct. Please let me know
   if you have further objections... and if not, could we agree to have
   this picked up somewhere?
  
  I need acks for DTS part...
 
 This patch breaks Exynos 4, so it's a NAK from me.
 
 Anyway, a new, completely redesigned PHY driver supporting most of the PHY 
 features (as opposed to only the basic subset currently) developed by Kamil 
 Debski should show up soon, so I don't think there is any reason to apply 
 any patches to this old driver.

awesome, then I espect to see a patch deleting the old driver shortly
:-)

cheers

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH] usb: phy: samsung-usb2: Toggle HSIC GPIO from device tree

[Felipe Balbi]

On Tue, Aug 13, 2013 at 02:11:27PM +0530, Tushar Behera wrote:
 On 12 July 2013 12:27, Felipe Balbi ba...@ti.com wrote:
  Hi,
 
  On Wed, Jul 10, 2013 at 10:42:27AM -0700, Julius Werner wrote:
  Hi Felipe,
 
  This is intended to pull down a reset signal line, not to switch power
  to the device. I could implement that with the regulator framework
  too, but I think that would just be confusing and harder to understand
  without providing any benefit. It's really just a plain old GPIO.
 
  alright, in that case how about using drivers/reset/ ?
 
 
 IIUC, reset-gpio driver only provides APIs for reset controls (reset,
 assert, deassert). We still need to find out the location from where
 we would be calling the reset function.

that's fair, but at least you reuse a bunch of boilerplate code to claim
the GPIO, set proper direction and value. No need to duplicate that in
your driver.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v11 0/8] PHY framework

[Felipe Balbi]

Hi,

On Mon, Aug 26, 2013 at 01:44:49PM +0530, Kishon Vijay Abraham I wrote:
 On Wednesday 21 August 2013 11:16 AM, Kishon Vijay Abraham I wrote:
  Added a generic PHY framework that provides a set of APIs for the PHY 
  drivers
  to create/destroy a PHY and APIs for the PHY users to obtain a reference to
  the PHY with or without using phandle.
  
  This framework will be of use only to devices that uses external PHY (PHY
  functionality is not embedded within the controller).
  
  The intention of creating this framework is to bring the phy drivers spread
  all over the Linux kernel to drivers/phy to increase code re-use and to
  increase code maintainability.
  
  Comments to make PHY as bus wasn't done because PHY devices can be part of
  other bus and making a same device attached to multiple bus leads to bad
  design.
  
  If the PHY driver has to send notification on connect/disconnect, the PHY
  driver should make use of the extcon framework. Using this susbsystem
  to use extcon framwork will have to be analysed.
  
  You can find this patch series @
  git://git.kernel.org/pub/scm/linux/kernel/git/kishon/linux-phy.git testing
 
 Looks like there are not further comments on this series. Can you take this in
 your misc tree?

Do you want me to queue these for you ? There are quite a few users for
this framework already and I know of at least 2 others which will show
up for v3.13.

Let me know.

cheers

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 01/15] drivers: phy: add generic PHY framework

[Felipe Balbi]

Hi,

On Mon, Aug 19, 2013 at 10:58:09AM +0530, Kishon Vijay Abraham I wrote:
  So maybe let's stop solving an already solved problem and just state that 
  you need to explicitly assign device ID to use this framework?
  
  Felipe,
  Can we have it the way I had in my v10 patch series till we find a better 
  way?
  I think this *non-dt* stuff shouldn't be blocking as most of the users are 
  dt only?

I don't have a lot of things against it, but preventing driver authors
to use PLATFORM_DEVID_AUTO just to use the framework is likely going to
piss some people off.

Perhaps we can start with this approach and fix things later ? At least
it ungates all the PHY drivers which are depending on this framework
(quite a few already). If everybody agrees with this approach, I'd be ok
with it too.

cheers

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH RESEND] i2c: move of helpers into the core

[Felipe Balbi]

On Mon, Aug 19, 2013 at 07:59:40PM +0200, Wolfram Sang wrote:
 I2C of helpers used to live in of_i2c.c but experience (from SPI) shows
 that it is much cleaner to have this in the core. This also removes a
 circular dependency between the helpers and the core, and so we can
 finally register child nodes in the core instead of doing this manually
 in each driver. So, fix the drivers and documentation, too.
 
 Signed-off-by: Wolfram Sang w...@the-dreams.de

for i2c-omap.c:

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

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 1/3] usb: phy-samsung-usb: Simplify PMU register handling

[Felipe Balbi]

Hi,

On Thu, Aug 01, 2013 at 05:52:04PM -0700, Julius Werner wrote:
 This patch simplifies the way the phy-samsung-usb code finds the correct
 power management register to enable PHY clock gating. Previously, the
 code would calculate the register address from a device tree supplied
 base address and add an offset based on the PHY type.
 
 Since every PHY has its own device tree entry and needs only one
 register, we can just encode the address itself in the device tree and
 remove the diffentiation in the code. The bitmask needed to specify the
 bit within that register stays in place, allowing support for platforms
 like s3c64xx that use different bits within the same register.
 
 Signed-off-by: Julius Werner jwer...@chromium.org

I need an Acked-by for the dtsi part. Ideally from Exynos 5 maintainer
and at least one of the DT maintainers.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 01/15] drivers: phy: add generic PHY framework

[Felipe Balbi]

Hi,

On Wed, Jul 31, 2013 at 11:14:32AM +0530, Kishon Vijay Abraham I wrote:
  IMHO we need a lookup method for PHYs, just like for clocks,
  regulators, PWMs or even i2c busses because there are complex cases
  when passing just a name using platform data will not work. I would
  second what Stephen said [1] and define a structure doing things in a
  DT-like way.
 
  Example;
 
  [platform code]
 
  static const struct phy_lookup my_phy_lookup[] = {
 
  PHY_LOOKUP(s3c-hsotg.0, otg, samsung-usbphy.1, phy.2),
 
  The only problem here is that if *PLATFORM_DEVID_AUTO* is used while
  creating the device, the ids in the device name would change and
  PHY_LOOKUP wont be useful.
 
  I don't think this is a problem. All the existing lookup methods already 
  use ID to identify devices (see regulators, clkdev, PWMs, i2c, ...). You 
  can simply add a requirement that the ID must be assigned manually, 
  without using PLATFORM_DEVID_AUTO to use PHY lookup.
 
  And I'm saying that this idea, of using a specific name and id, is
  frought with fragility and will break in the future in various ways when
  devices get added to systems, making these strings constantly have to be
  kept up to date with different board configurations.
 
  People, NEVER, hardcode something like an id.  The fact that this
  happens today with the clock code, doesn't make it right, it makes the
  clock code wrong.  Others have already said that this is wrong there as
  well, as systems change and dynamic ids get used more and more.
 
  Let's not repeat the same mistakes of the past just because we refuse to
  learn from them...
 
  So again, the find a phy by a string functions should be removed, the
  device id should be automatically created by the phy core just to make
  things unique in sysfs, and no driver code should _ever_ be reliant on
  the number that is being created, and the pointer to the phy structure
  should be used everywhere instead.
 
  With those types of changes, I will consider merging this subsystem, but
  without them, sorry, I will not.
  
  I'll agree with Greg here, the very fact that we see people trying to
  add a requirement of *NOT* using PLATFORM_DEVID_AUTO already points to a
  big problem in the framework.
  
  The fact is that if we don't allow PLATFORM_DEVID_AUTO we will end up
  adding similar infrastructure to the driver themselves to make sure we
  don't end up with duplicate names in sysfs in case we have multiple
  instances of the same IP in the SoC (or several of the same PCIe card).
  I really don't want to go back to that.
 
 If we are using PLATFORM_DEVID_AUTO, then I dont see any way we can give the
 correct binding information to the PHY framework. I think we can drop having
 this non-dt support in PHY framework? I see only one platform (OMAP3) going to
 be needing this non-dt support and we can use the USB PHY library for it.

you shouldn't drop support for non-DT platform, in any case we lived
without DT (and still do) for years. Gotta find a better way ;-)

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 01/15] drivers: phy: add generic PHY framework

[Felipe Balbi]

On Sun, Jul 21, 2013 at 08:46:53AM -0700, Greg KH wrote:
 On Sun, Jul 21, 2013 at 01:12:07PM +0200, Tomasz Figa wrote:
  On Sunday 21 of July 2013 16:37:33 Kishon Vijay Abraham I wrote:
   Hi,
   
   On Sunday 21 July 2013 04:01 PM, Tomasz Figa wrote:
Hi,

On Saturday 20 of July 2013 19:59:10 Greg KH wrote:
On Sat, Jul 20, 2013 at 10:32:26PM -0400, Alan Stern wrote:
On Sat, 20 Jul 2013, Greg KH wrote:
That should be passed using platform data.

Ick, don't pass strings around, pass pointers.  If you have
platform
data you can get to, then put the pointer there, don't use a
name.

I don't think I understood you here :-s We wont have phy pointer
when we create the device for the controller no?(it'll be done in
board file). Probably I'm missing something.

Why will you not have that pointer?  You can't rely on the name
as
the device id will not match up, so you should be able to rely on
the pointer being in the structure that the board sets up, right?

Don't use names, especially as ids can, and will, change, that is
going
to cause big problems.  Use pointers, this is C, we are supposed to
be
doing that :)

Kishon, I think what Greg means is this:  The name you are using
must
be stored somewhere in a data structure constructed by the board
file,
right?  Or at least, associated with some data structure somehow.
Otherwise the platform code wouldn't know which PHY hardware
corresponded to a particular name.

Greg's suggestion is that you store the address of that data
structure
in the platform data instead of storing the name string.  Have the
consumer pass the data structure's address when it calls phy_create,
instead of passing the name.  Then you don't have to worry about two
PHYs accidentally ending up with the same name or any other similar
problems.

Close, but the issue is that whatever returns from phy_create()
should
then be used, no need to call any find functions, as you can just
use
the pointer that phy_create() returns.  Much like all other class api
functions in the kernel work.

I think there is a confusion here about who registers the PHYs.

All platform code does is registering a platform/i2c/whatever device,
which causes a driver (located in drivers/phy/) to be instantiated.
Such drivers call phy_create(), usually in their probe() callbacks,
so platform_code has no way (and should have no way, for the sake of
layering) to get what phy_create() returns.
 
 Why not put pointers in the platform data structure that can hold these
 pointers?  I thought that is why we created those structures in the
 first place.  If not, what are they there for?

heh, IMO we shouldn't pass pointers of any kind through platform_data,
we want to pass data :-)

Allowing to pass pointers through that, is one of the reasons which got
us in such a big mess in ARM land, well it was much easier for a
board-file/driver writer to pass a function pointer then to create a
generic framework :-)

IMHO we need a lookup method for PHYs, just like for clocks,
regulators, PWMs or even i2c busses because there are complex cases
when passing just a name using platform data will not work. I would
second what Stephen said [1] and define a structure doing things in a
DT-like way.

Example;

[platform code]

static const struct phy_lookup my_phy_lookup[] = {

PHY_LOOKUP(s3c-hsotg.0, otg, samsung-usbphy.1, phy.2),
   
   The only problem here is that if *PLATFORM_DEVID_AUTO* is used while
   creating the device, the ids in the device name would change and
   PHY_LOOKUP wont be useful.
  
  I don't think this is a problem. All the existing lookup methods already 
  use ID to identify devices (see regulators, clkdev, PWMs, i2c, ...). You 
  can simply add a requirement that the ID must be assigned manually, 
  without using PLATFORM_DEVID_AUTO to use PHY lookup.
 
 And I'm saying that this idea, of using a specific name and id, is
 frought with fragility and will break in the future in various ways when
 devices get added to systems, making these strings constantly have to be
 kept up to date with different board configurations.
 
 People, NEVER, hardcode something like an id.  The fact that this
 happens today with the clock code, doesn't make it right, it makes the
 clock code wrong.  Others have already said that this is wrong there as
 well, as systems change and dynamic ids get used more and more.
 
 Let's not repeat the same mistakes of the past just because we refuse to
 learn from them...
 
 So again, the find a phy by a string functions should be removed, the
 device id should be automatically created by the phy core just to make
 things unique in sysfs, and no driver code should _ever_ be reliant on
 the number that is being created, and the pointer 

Re: [PATCH v2] usb: phy-samsung-usb: Simplify PMU register handling

[Felipe Balbi]

On Mon, Jul 29, 2013 at 02:17:56PM -0700, Julius Werner wrote:
 This patch simplifies the way the phy-samsung-usb code finds the correct
 power management register to enable PHY clock gating. Previously, the
 code would calculate the register address from a device tree supplied
 base address and add an offset based on the PHY type.
 
 Since every PHY has its own device tree entry and needs only one
 register, we can just encode the address itself in the device tree and
 remove the diffentiation in the code. The bitmask needed to specify the
 bit within that register stays in place, allowing support for platforms
 like s3c64xx that use different bits within the same register. Due to
 this simplification, the whole complication of a Samsung-specific USB
 PHY type can be removed from the PHY driver.
 
 Change-Id: Id823f04bbf1942f307bd1d24ec9d8d55a69b0e56

remove this Gerrit Change-Id, please. Also resend with
linux-...@vger.kernel.org in Cc.

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 04/11] spi/omap2: Covert to core runtime PM

[Felipe Balbi]

Hi,

On Sun, Jul 28, 2013 at 03:43:19PM +0100, Mark Brown wrote:
 From: Mark Brown broo...@linaro.org
 
 Signed-off-by: Mark Brown broo...@linaro.org

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

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH] usb: phy: samsung-usb2: Toggle HSIC GPIO from device tree

[Felipe Balbi]

Hi,

On Wed, Jul 10, 2013 at 10:42:27AM -0700, Julius Werner wrote:
 Hi Felipe,
 
 This is intended to pull down a reset signal line, not to switch power
 to the device. I could implement that with the regulator framework
 too, but I think that would just be confusing and harder to understand
 without providing any benefit. It's really just a plain old GPIO.

alright, in that case how about using drivers/reset/ ?

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH] usb: phy: samsung-usb2: Toggle HSIC GPIO from device tree

[Felipe Balbi]

On Tue, Jul 09, 2013 at 05:34:15PM -0700, Julius Werner wrote:
 This patch adds support for a new 'samsung,hsic-reset-gpio' in the
 device tree, which will be interpreted as an active-low reset pin during
 PHY initialization when it exists. Useful for intergrated HSIC devices
 like an SMSC 3503 hub. It is necessary to add this directly to the PHY
 initialization to get the timing right, since resetting a HSIC device
 after it has already been enumerated can confuse the USB stack.
 
 Also fixes PHY semaphore code to make sure we always go through the
 setup at least once, even if it was already turned on (e.g. by
 firmware), and changes a spinlock to a mutex to allow sleeping in the
 critical section.
 
 Change-Id: Ieecac52c27daa7a17a7ed3b2863ddba3aeb8d16f
 Signed-off-by: Julius Werner jwer...@chromium.org
 ---
  .../devicetree/bindings/usb/samsung-usbphy.txt | 10 ++
  drivers/usb/phy/phy-samsung-usb.c  | 17 ++
  drivers/usb/phy/phy-samsung-usb.h  |  7 ++--
  drivers/usb/phy/phy-samsung-usb2.c | 38 
 ++
  drivers/usb/phy/phy-samsung-usb3.c | 12 +++
  5 files changed, 55 insertions(+), 29 deletions(-)
 
 diff --git a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt 
 b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
 index 33fd354..82e2e16 100644
 --- a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
 +++ b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
 @@ -31,6 +31,12 @@ Optional properties:
  - ranges: allows valid translation between child's address space and parent's
 address space.
  
 +- samsung,hsic-reset-gpio: an active low GPIO pin that resets a device
 + connected to the HSIC port. Useful for things like
 + an on-board SMSC3503 hub.
 +- pinctrl-0: Pin control group containing the HSIC reset GPIO pin.
 +- pinctrl-names: Should contain only one value - default.
 +
  - The child node 'usbphy-sys' to the node 'usbphy' is for the system 
 controller
interface for usb-phy. It should provide the following information 
 required by
usb-phy controller to control phy.
 @@ -56,6 +62,10 @@ Example:
   clocks = clock 2, clock 305;
   clock-names = xusbxti, otg;
  
 + samsung,hsic-reset-gpio = gpx2 4 1;

looks like this should be modeled as a fixed-regulator ?

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH 1/4] usb: gadget: s3c-hsotg: Allow driver instantiation using device tree

[Felipe Balbi]

Hi,

On Tue, Jun 25, 2013 at 05:38:23PM +0200, Tomasz Figa wrote:
 This patch adds OF match table to the driver to allow instantiating it
 using device tree.
 
 Signed-off-by: Tomasz Figa t.f...@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com

I will take this one only, the others have no dependency on this.

cheers

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH V2 1/3] phy: Add driver for Exynos DP PHY

[Felipe Balbi]

On Fri, Jun 28, 2013 at 04:15:32PM +0900, Jingoo Han wrote:
 Add a PHY provider driver for the Samsung Exynos SoC DP PHY.
 
 Signed-off-by: Jingoo Han jg1@samsung.com

Now that you fixed Kishon's concerns, this looks pretty good:

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

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH V2 2/3] ARM: dts: Add DP PHY node to exynos5250.dtsi

[Felipe Balbi]

On Fri, Jun 28, 2013 at 04:16:44PM +0900, Jingoo Han wrote:
 Add PHY provider node for the DP PHY.
 
 Signed-off-by: Jingoo Han jg1@samsung.com
 ---
  arch/arm/boot/dts/exynos5250.dtsi |   13 -
  1 file changed, 8 insertions(+), 5 deletions(-)
 
 diff --git a/arch/arm/boot/dts/exynos5250.dtsi 
 b/arch/arm/boot/dts/exynos5250.dtsi
 index 41cd625..f7bac75 100644
 --- a/arch/arm/boot/dts/exynos5250.dtsi
 +++ b/arch/arm/boot/dts/exynos5250.dtsi
 @@ -614,6 +614,12 @@
   interrupts = 0 94 0;
   };
  
 + dp_phy: video-phy@10040720 {
 + compatible = samsung,exynos5250-dp-video-phy;
 + reg = 0x10040720 4;
 + #phy-cells = 0;
 + };
 +
   dp-controller {
   compatible = samsung,exynos5-dp;
   reg = 0x145b 0x1000;
 @@ -623,11 +629,8 @@
   clock-names = dp;
   #address-cells = 1;
   #size-cells = 0;
 -
 - dptx-phy {
 - reg = 0x10040720;
 - samsung,enable-mask = 1;
 - };
 + phys = dp_phy 0;

phy-cells being 0, means that this would become:

phys = dp_phy;

 + phy-names = dp;

for the label, I would use something more descriptive such as
'display-port'.

other than that:

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

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH V2 3/3] video: exynos_dp: Use the generic PHY driver

[Felipe Balbi]

On Fri, Jun 28, 2013 at 04:18:23PM +0900, Jingoo Han wrote:
 Use the generic PHY API instead of the platform callback to control
 the DP PHY. The 'phy_label' field is added to the platform data
 structure to allow PHY lookup on non-dt platforms.
 
 Signed-off-by: Jingoo Han jg1@samsung.com
 ---
  .../devicetree/bindings/video/exynos_dp.txt|   17 ---
  drivers/video/exynos/exynos_dp_core.c  |  118 
 ++--
  drivers/video/exynos/exynos_dp_core.h  |2 +
  include/video/exynos_dp.h  |6 +-
  4 files changed, 15 insertions(+), 128 deletions(-)
 
 diff --git a/Documentation/devicetree/bindings/video/exynos_dp.txt 
 b/Documentation/devicetree/bindings/video/exynos_dp.txt
 index 84f10c1..a8320e3 100644
 --- a/Documentation/devicetree/bindings/video/exynos_dp.txt
 +++ b/Documentation/devicetree/bindings/video/exynos_dp.txt
 @@ -1,17 +1,6 @@
  The Exynos display port interface should be configured based on
  the type of panel connected to it.
  
 -We use two nodes:
 - -dp-controller node
 - -dptx-phy node(defined inside dp-controller node)
 -
 -For the DP-PHY initialization, we use the dptx-phy node.
 -Required properties for dptx-phy:
 - -reg:
 - Base address of DP PHY register.
 - -samsung,enable-mask:
 - The bit-mask used to enable/disable DP PHY.
 -
  For the Panel initialization, we read data from dp-controller node.
  Required properties for dp-controller:
   -compatible:
 @@ -67,12 +56,6 @@ SOC specific portion:
   interrupt-parent = combiner;
   clocks = clock 342;
   clock-names = dp;
 -
 - dptx-phy {
 - reg = 0x10040720;
 - samsung,enable-mask = 1;
 - };
 -
   };
  
  Board Specific portion:
 diff --git a/drivers/video/exynos/exynos_dp_core.c 
 b/drivers/video/exynos/exynos_dp_core.c
 index 12bbede..bac515b 100644
 --- a/drivers/video/exynos/exynos_dp_core.c
 +++ b/drivers/video/exynos/exynos_dp_core.c
 @@ -19,6 +19,7 @@
  #include linux/interrupt.h
  #include linux/delay.h
  #include linux/of.h
 +#include linux/phy/phy.h
  
  #include video/exynos_dp.h
  
 @@ -960,84 +961,15 @@ static struct exynos_dp_platdata 
 *exynos_dp_dt_parse_pdata(struct device *dev)
   return ERR_PTR(-EINVAL);
   }
  
 - return pd;
 -}
 -
 -static int exynos_dp_dt_parse_phydata(struct exynos_dp_device *dp)
 -{
 - struct device_node *dp_phy_node = of_node_get(dp-dev-of_node);
 - u32 phy_base;
 - int ret = 0;
 -
 - dp_phy_node = of_find_node_by_name(dp_phy_node, dptx-phy);
 - if (!dp_phy_node) {
 - dev_err(dp-dev, could not find dptx-phy node\n);
 - return -ENODEV;
 - }
 -
 - if (of_property_read_u32(dp_phy_node, reg, phy_base)) {
 - dev_err(dp-dev, failed to get reg for dptx-phy\n);
 - ret = -EINVAL;
 - goto err;
 - }
 -
 - if (of_property_read_u32(dp_phy_node, samsung,enable-mask,
 - dp-enable_mask)) {
 - dev_err(dp-dev, failed to get enable-mask for dptx-phy\n);
 - ret = -EINVAL;
 - goto err;
 - }
 -
 - dp-phy_addr = ioremap(phy_base, SZ_4);
 - if (!dp-phy_addr) {
 - dev_err(dp-dev, failed to ioremap dp-phy\n);
 - ret = -ENOMEM;
 - goto err;
 - }
 -
 -err:
 - of_node_put(dp_phy_node);
 -
 - return ret;
 -}
 -
 -static void exynos_dp_phy_init(struct exynos_dp_device *dp)
 -{
 - u32 reg;
 -
 - reg = __raw_readl(dp-phy_addr);
 - reg |= dp-enable_mask;
 - __raw_writel(reg, dp-phy_addr);
 -}
 -
 -static void exynos_dp_phy_exit(struct exynos_dp_device *dp)
 -{
 - u32 reg;
 + pd-phy_label = dp;

only the label, which I would use 'display-port'. Other than that:

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

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs

[Felipe Balbi]

On Wed, Jun 26, 2013 at 05:00:34PM +0200, Sylwester Nawrocki wrote:
 Hi,
 
 On 06/25/2013 05:06 PM, Felipe Balbi wrote:
  +static struct platform_driver exynos_video_phy_driver = {
   +.probe  = exynos_video_phy_probe,
 
  you *must* provide a remove method. drivers with NULL remove are
  non-removable :-)
 
 Actually the remove() callback can be NULL, it's just missing module_exit
 function that makes a module not unloadable.

look at the implementation of platform_drv_remove():

 499 static int platform_drv_remove(struct device *_dev)
 500 {
 501 struct platform_driver *drv = to_platform_driver(_dev-driver);
 502 struct platform_device *dev = to_platform_device(_dev);
 503 int ret;
 504 
 505 ret = drv-remove(dev);
 506 if (ACPI_HANDLE(_dev))
 507 acpi_dev_pm_detach(_dev, true);
 508 
 509 return ret;
 510 }

that's not a conditional call right :-)

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs

[Felipe Balbi]

On Thu, Jun 27, 2013 at 09:47:47AM +0200, Andrzej Hajda wrote:
 Hi Felipe,
 
 On 06/27/2013 08:17 AM, Felipe Balbi wrote:
  On Wed, Jun 26, 2013 at 05:00:34PM +0200, Sylwester Nawrocki wrote:
  Hi,
 
  On 06/25/2013 05:06 PM, Felipe Balbi wrote:
  +static struct platform_driver exynos_video_phy_driver = {
  +   .probe  = exynos_video_phy_probe,
 
  you *must* provide a remove method. drivers with NULL remove are
  non-removable :-)
 
  Actually the remove() callback can be NULL, it's just missing module_exit
  function that makes a module not unloadable.
  
  look at the implementation of platform_drv_remove():
  
   499 static int platform_drv_remove(struct device *_dev)
   500 {
   501 struct platform_driver *drv = to_platform_driver(_dev-driver);
   502 struct platform_device *dev = to_platform_device(_dev);
   503 int ret;
   504 
   505 ret = drv-remove(dev);
   506 if (ACPI_HANDLE(_dev))
   507 acpi_dev_pm_detach(_dev, true);
   508 
   509 return ret;
   510 }
  
  that's not a conditional call right :-)
 
 It is conditional, just condition check is in different place:
 
 int platform_driver_register(struct platform_driver *drv)
 {
   (...)
   if (drv-remove)
   drv-driver.remove = platform_drv_remove;
   (...)
 }

good point :-) thanks. I'll go ack your driver now

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v3 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs

[Felipe Balbi]

On Wed, Jun 26, 2013 at 05:02:22PM +0200, Sylwester Nawrocki wrote:
 Add a PHY provider driver for the Samsung S5P/Exynos SoC MIPI CSI-2
 receiver and MIPI DSI transmitter DPHYs.
 
 Signed-off-by: Sylwester Nawrocki s.nawro...@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com

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

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v3 2/5] ARM: dts: Add MIPI PHY node to exynos4.dtsi

[Felipe Balbi]

On Wed, Jun 26, 2013 at 05:02:23PM +0200, Sylwester Nawrocki wrote:
 Add PHY provider node for the MIPI CSIS and MIPI DSIM PHYs.
 
 Signed-off-by: Sylwester Nawrocki s.nawro...@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com

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

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v3 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs

[Felipe Balbi]

On Thu, Jun 27, 2013 at 10:37:12AM +0200, Sylwester Nawrocki wrote:
 On 06/27/2013 09:52 AM, Felipe Balbi wrote:
  On Wed, Jun 26, 2013 at 05:02:22PM +0200, Sylwester Nawrocki wrote:
  Add a PHY provider driver for the Samsung S5P/Exynos SoC MIPI CSI-2
  receiver and MIPI DSI transmitter DPHYs.
 
  Signed-off-by: Sylwester Nawrocki s.nawro...@samsung.com
  Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
  
  Acked-by: Felipe Balbi ba...@ti.com
 
 Thank you for your review and ack!
 
 Just for the record, I have tested this driver as a loadable
 module on Exynos4412 TRATS2 board and it all worked fine for
 both the camera and display side.

Awesome dude :-) very cool, let's hope more users convert to Kishon's
generic phy layer :-)

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs

[Felipe Balbi]

On Wed, Jun 26, 2013 at 02:03:42PM +0200, Sylwester Nawrocki wrote:
 On 06/26/2013 01:21 PM, Kishon Vijay Abraham I wrote:
  +static int exynos_video_phy_probe(struct platform_device *pdev)
   +{
   +  struct exynos_video_phy *state;
   +  struct device *dev = pdev-dev;
   +  struct resource *res;
   +  struct phy_provider *phy_provider;
   +  int i;
   +
   +  state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
   +  if (!state)
   +  return -ENOMEM;
   +
   +  res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
   +
   +  state-regs = devm_ioremap_resource(dev, res);
   +  if (IS_ERR(state-regs))
   +  return PTR_ERR(state-regs);
   +
   +  dev_set_drvdata(dev, state);
  
   you can use platform_set_drvdata(pdev, state);
  
   I had it in the previous version, but changed for symmetry with
   dev_set_drvdata(). I guess those could be replaced with
   phy_{get, set}_drvdata as you suggested.
 
  right. currently I was setting dev_set_drvdata of phy (core) device
  in phy-core.c and the corresponding dev_get_drvdata in phy provider driver
  which is little confusing.
  So I'll add phy_set_drvdata and phy_get_drvdata in phy.h (as suggested by
  Felipe) to be used by phy provider drivers. So after creating the PHY, the
  phy provider should use phy_set_drvdata and in phy_ops, it can use
  phy_get_drvdata. (I'll remove the dev_set_drvdata in phy_create).
  
  This also means _void *priv_ in phy_create is useless. So I'll be removing
  _priv_ from phy_create.
 
 Yeah, sounds good. Then in the phy ops phy_get_drvdata(phy-dev) would

phy_get_drvdata(phy);

accessing the dev pointer will be done inside the helper :-)

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v2 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs

[Felipe Balbi]

Hi,

On Tue, Jun 25, 2013 at 04:21:46PM +0200, Sylwester Nawrocki wrote:
 +enum phy_id {
 + PHY_CSIS0,
 + PHY_DSIM0,
 + PHY_CSIS1,
 + PHY_DSIM1,
 + NUM_PHYS

please prepend these with EXYNOS_PHY_ or EXYNOS_MIPI_PHY_

 +struct exynos_video_phy {
 + spinlock_t slock;
 + struct phy *phys[NUM_PHYS];

more than one phy ? This means you should instantiate driver multiple
drivers. Each phy id should call probe again.

 +static int __set_phy_state(struct exynos_video_phy *state,
 + enum phy_id id, unsigned int on)
 +{
 + void __iomem *addr;
 + unsigned long flags;
 + u32 reg, reset;
 +
 + if (WARN_ON(id  NUM_PHYS))
 + return -EINVAL;

you don't want to do this, actually. It'll bug you everytime you want to
add another phy ID :-)

 + addr = state-regs + EXYNOS_MIPI_PHY_CONTROL(id / 2);
 +
 + if (id == PHY_DSIM0 || id == PHY_DSIM1)
 + reset = EXYNOS_MIPI_PHY_MRESETN;
 + else
 + reset = EXYNOS_MIPI_PHY_SRESETN;
 +
 + spin_lock_irqsave(state-slock, flags);
 + reg = readl(addr);
 + if (on)
 + reg |= reset;
 + else
 + reg = ~reset;
 + writel(reg, addr);
 +
 + /* Clear ENABLE bit only if MRESETN, SRESETN bits are not set. */
 + if (on)
 + reg |= EXYNOS_MIPI_PHY_ENABLE;
 + else if (!(reg  EXYNOS_MIPI_PHY_RESET_MASK))
 + reg = ~EXYNOS_MIPI_PHY_ENABLE;
 +
 + writel(reg, addr);
 + spin_unlock_irqrestore(state-slock, flags);
 +
 + pr_debug(%s(): id: %d, on: %d, addr: %#x, base: %#x\n,
 +  __func__, id, on, (u32)addr, (u32)state-regs);

use dev_dbg() instead.

 +
 + return 0;
 +}
 +
 +static int exynos_video_phy_power_on(struct phy *phy)
 +{
 + struct exynos_video_phy *state = dev_get_drvdata(phy-dev);

looks like we should have phy_get_drvdata() helper :-) Kishon ?

 +static struct phy *exynos_video_phy_xlate(struct device *dev,
 + struct of_phandle_args *args)
 +{
 + struct exynos_video_phy *state = dev_get_drvdata(dev);
 +
 + if (WARN_ON(args-args[0]  NUM_PHYS))
 + return NULL;

please remove this check.

 + return state-phys[args-args[0]];

and your xlate is 'wrong'.

 +}
 +
 +static struct phy_ops exynos_video_phy_ops = {
 + .power_on   = exynos_video_phy_power_on,
 + .power_off  = exynos_video_phy_power_off,
 + .owner  = THIS_MODULE,
 +};
 +
 +static int exynos_video_phy_probe(struct platform_device *pdev)
 +{
 + struct exynos_video_phy *state;
 + struct device *dev = pdev-dev;
 + struct resource *res;
 + struct phy_provider *phy_provider;
 + int i;
 +
 + state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
 + if (!state)
 + return -ENOMEM;
 +
 + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 +
 + state-regs = devm_ioremap_resource(dev, res);
 + if (IS_ERR(state-regs))
 + return PTR_ERR(state-regs);
 +
 + dev_set_drvdata(dev, state);

you can use platform_set_drvdata(pdev, state);

same thing though, no strong feelings.

 + phy_provider = devm_of_phy_provider_register(dev,
 + exynos_video_phy_xlate);
 + if (IS_ERR(phy_provider))
 + return PTR_ERR(phy_provider);
 +
 + for (i = 0; i  NUM_PHYS; i++) {
 + char label[8];
 +
 + snprintf(label, sizeof(label), %s.%d,
 + i == PHY_DSIM0 || i == PHY_DSIM1 ?
 + dsim : csis, i / 2);
 +
 + state-phys[i] = devm_phy_create(dev, i, exynos_video_phy_ops,
 + label, state);
 + if (IS_ERR(state-phys[i])) {
 + dev_err(dev, failed to create PHY %s\n, label);
 + return PTR_ERR(state-phys[i]);
 + }
 + }

this really doesn't look correct to me. It looks like you have multiple
PHYs, one for each ID. So your probe should be called for each PHY ID
and you have several phy_providers too.

 +static const struct of_device_id exynos_video_phy_of_match[] = {
 + { .compatible = samsung,s5pv210-mipi-video-phy },

and this should contain all PHY IDs:

{ .compatible = samsung,s5pv210-mipi-video-dsim0-phy,
.data = (const void *) DSIM0, },
{ .compatible = samsung,s5pv210-mipi-video-dsim1-phy,
.data = (const void *) DSIM1, },
{ .compatible = samsung,s5pv210-mipi-video-csi0-phy
.data = (const void *) CSI0, },
{ .compatible = samsung,s5pv210-mipi-video-csi1-phy
.data = (const void *) CSI1, },

then on your probe you can fetch that data field and use it as phy-id.

 +static struct platform_driver exynos_video_phy_driver = {
 + .probe  = exynos_video_phy_probe,

you *must* provide a remove method. drivers with NULL remove are
non-removable :-)

-- 

Re: [PATCH v2 3/5] video: exynos_mipi_dsim: Use generic PHY driver

[Felipe Balbi]

Hi,

On Tue, Jun 25, 2013 at 04:21:48PM +0200, Sylwester Nawrocki wrote:
 Use the generic PHY API instead of the platform callback to control
 the MIPI DSIM DPHY. The 'phy_label' field is added to the platform
 data structure to allow PHY lookup on non-dt platforms.
 
 Signed-off-by: Sylwester Nawrocki s.nawro...@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com

this is awesome :-)

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

-- 
balbi


signature.asc
Description: Digital signature

Re: [PATCH v2 4/5] exynos4-is: Use generic MIPI CSIS PHY driver

[Felipe Balbi]

On Tue, Jun 25, 2013 at 04:21:49PM +0200, Sylwester Nawrocki wrote:
 Use the generic PHY API instead of the platform callback to control
 the MIPI CSIS DPHY. The 'phy_label' field is added to the platform
 data structure to allow PHY lookup on non-dt platforms
 
 Signed-off-by: Sylwester Nawrocki s.nawro...@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com

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

-- 
balbi


signature.asc
Description: Digital signature