diff mbox series

[v2,09/41] usb: gadget: s3c-hsudc: remove platform header dependency

Message ID 20200806182059.2431-9-krzk@kernel.org (mailing list archive)
State New, archived
Headers show
Series None | expand

Commit Message

Krzysztof Kozlowski Aug. 6, 2020, 6:20 p.m. UTC 
From: Arnd Bergmann <arnd@arndb.de>

There is no real phy driver, so s3c-hsudc just pokes the registers
itself. Improve this a little by making it a platform data callback
like we do for gpios.

There is only one board using this driver, and it's unlikely
that another would be added, so this is a minimal workaround.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
[krzk: Include regs-s3c2443-clock.h in ifdef to fixup build on s3c6400]
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

---

Changes since v1:
1. Include regs-s3c2443-clock.h in ifdef to fixup build on s3c640
---
 .../include/mach/regs-s3c2443-clock.h         | 49 +++++++++++++++++
 arch/arm/plat-samsung/devs.c                  |  6 ++
 drivers/usb/gadget/udc/s3c-hsudc.c            | 55 ++-----------------
 include/linux/platform_data/s3c-hsudc.h       |  2 +
 4 files changed, 61 insertions(+), 51 deletions(-)

Comments

Felipe Balbi Aug. 7, 2020, 1:59 p.m. UTC | #1 
Krzysztof Kozlowski <krzk@kernel.org> writes:

> From: Arnd Bergmann <arnd@arndb.de>
>
> There is no real phy driver, so s3c-hsudc just pokes the registers
> itself. Improve this a little by making it a platform data callback
> like we do for gpios.
>
> There is only one board using this driver, and it's unlikely
> that another would be added, so this is a minimal workaround.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> [krzk: Include regs-s3c2443-clock.h in ifdef to fixup build on s3c6400]
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
>
> ---
>
> Changes since v1:
> 1. Include regs-s3c2443-clock.h in ifdef to fixup build on s3c640
> ---
>  .../include/mach/regs-s3c2443-clock.h         | 49 +++++++++++++++++
>  arch/arm/plat-samsung/devs.c                  |  6 ++
>  drivers/usb/gadget/udc/s3c-hsudc.c            | 55 ++-----------------
>  include/linux/platform_data/s3c-hsudc.h       |  2 +
>  4 files changed, 61 insertions(+), 51 deletions(-)
>
> diff --git a/arch/arm/mach-s3c24xx/include/mach/regs-s3c2443-clock.h b/arch/arm/mach-s3c24xx/include/mach/regs-s3c2443-clock.h
> index 6bf924612b06..682759549e63 100644
> --- a/arch/arm/mach-s3c24xx/include/mach/regs-s3c2443-clock.h
> +++ b/arch/arm/mach-s3c24xx/include/mach/regs-s3c2443-clock.h
> @@ -10,6 +10,8 @@
>  #ifndef __ASM_ARM_REGS_S3C2443_CLOCK
>  #define __ASM_ARM_REGS_S3C2443_CLOCK
>  
> +#include <linux/delay.h>
> +
>  #define S3C2443_CLKREG(x)		((x) + S3C24XX_VA_CLKPWR)
>  
>  #define S3C2443_PLLCON_MDIVSHIFT	16
> @@ -184,5 +186,52 @@ s3c2443_get_epll(unsigned int pllval, unsigned int baseclk)
>  	return (unsigned int)fvco;
>  }
>  
> +static inline void s3c_hsudc_init_phy(void)

This should, really, be a PHY driver under drivers/phy, since the goal
is to make this platform-independent, might as well take the opportunity
to introduce a proper driver, no?
Arnd Bergmann Aug. 7, 2020, 5:42 p.m. UTC | #2 
On Fri, Aug 7, 2020 at 3:59 PM Felipe Balbi <balbi@kernel.org> wrote:
> Krzysztof Kozlowski <krzk@kernel.org> writes:

> > +#include <linux/delay.h>
> > +
> >  #define S3C2443_CLKREG(x)            ((x) + S3C24XX_VA_CLKPWR)
> >
> >  #define S3C2443_PLLCON_MDIVSHIFT     16
> > @@ -184,5 +186,52 @@ s3c2443_get_epll(unsigned int pllval, unsigned int baseclk)
> >       return (unsigned int)fvco;
> >  }
> >
> > +static inline void s3c_hsudc_init_phy(void)
>
> This should, really, be a PHY driver under drivers/phy, since the goal
> is to make this platform-independent, might as well take the opportunity
> to introduce a proper driver, no?

In theory, this is absolutely correct. I fear it will be hard to find anyone
to make a larger scale cleanup of the file however. As my old changelog
text says, there is only one board (smdk2416) in the kernel that registers
the device. My change was the minimum to keep it working during the
move to a multiplatform configuration, but if there is someone who has
the hardware and volunteers to make a proper phy driver, that would also
work.

As the board only exists as a reference for other boards, but none of those
made it into the kernel, we could alternatively just decide to drop the
driver. There is also a .dts file for the board, which is lacking a device node
for the udc (and the driver lacks DT support), so that board file could also
be dropped then, leaving only the DT version as a reference for the SoC.

       Arnd
Krzysztof Kozlowski Aug. 9, 2020, 8:44 a.m. UTC | #3 
On Fri, Aug 07, 2020 at 07:42:30PM +0200, Arnd Bergmann wrote:
> On Fri, Aug 7, 2020 at 3:59 PM Felipe Balbi <balbi@kernel.org> wrote:
> > Krzysztof Kozlowski <krzk@kernel.org> writes:
> 
> > > +#include <linux/delay.h>
> > > +
> > >  #define S3C2443_CLKREG(x)            ((x) + S3C24XX_VA_CLKPWR)
> > >
> > >  #define S3C2443_PLLCON_MDIVSHIFT     16
> > > @@ -184,5 +186,52 @@ s3c2443_get_epll(unsigned int pllval, unsigned int baseclk)
> > >       return (unsigned int)fvco;
> > >  }
> > >
> > > +static inline void s3c_hsudc_init_phy(void)
> >
> > This should, really, be a PHY driver under drivers/phy, since the goal
> > is to make this platform-independent, might as well take the opportunity
> > to introduce a proper driver, no?
> 
> In theory, this is absolutely correct. I fear it will be hard to find anyone
> to make a larger scale cleanup of the file however. As my old changelog
> text says, there is only one board (smdk2416) in the kernel that registers
> the device. My change was the minimum to keep it working during the
> move to a multiplatform configuration, but if there is someone who has
> the hardware and volunteers to make a proper phy driver, that would also
> work.
> 
> As the board only exists as a reference for other boards, but none of those
> made it into the kernel, we could alternatively just decide to drop the
> driver. There is also a .dts file for the board, which is lacking a device node
> for the udc (and the driver lacks DT support), so that board file could also
> be dropped then, leaving only the DT version as a reference for the SoC.

All this is a work on a very old SoC with an unknown number of
current/real users. I don't have the hardware to test so I would prefer
to skip any big changes. As Arnd suggests, we could just drop this one
board and the driver... or if it does not harm/bother to much we could
keep it as is, just without platform header dependency. Someone still
might be using it.

Best regards,
Krzysztof
Felipe Balbi Aug. 10, 2020, 12:51 p.m. UTC | #4 
Arnd Bergmann <arnd@arndb.de> writes:

> On Fri, Aug 7, 2020 at 3:59 PM Felipe Balbi <balbi@kernel.org> wrote:
>> Krzysztof Kozlowski <krzk@kernel.org> writes:
>
>> > +#include <linux/delay.h>
>> > +
>> >  #define S3C2443_CLKREG(x)            ((x) + S3C24XX_VA_CLKPWR)
>> >
>> >  #define S3C2443_PLLCON_MDIVSHIFT     16
>> > @@ -184,5 +186,52 @@ s3c2443_get_epll(unsigned int pllval, unsigned int baseclk)
>> >       return (unsigned int)fvco;
>> >  }
>> >
>> > +static inline void s3c_hsudc_init_phy(void)
>>
>> This should, really, be a PHY driver under drivers/phy, since the goal
>> is to make this platform-independent, might as well take the opportunity
>> to introduce a proper driver, no?
>
> In theory, this is absolutely correct. I fear it will be hard to find anyone
> to make a larger scale cleanup of the file however. As my old changelog
> text says, there is only one board (smdk2416) in the kernel that registers
> the device. My change was the minimum to keep it working during the
> move to a multiplatform configuration, but if there is someone who has
> the hardware and volunteers to make a proper phy driver, that would also
> work.
>
> As the board only exists as a reference for other boards, but none of those
> made it into the kernel, we could alternatively just decide to drop the
> driver. There is also a .dts file for the board, which is lacking a device node
> for the udc (and the driver lacks DT support), so that board file could also
> be dropped then, leaving only the DT version as a reference for the SoC.

I don't mind deleting the entire thing if nobody is using it. The entire
history of the driver consists of only 65 commits and if you look at the
actual commits, there has been no real maintenance work on it for a long
time, most commits are regular janitorial work and updates due to
framework changes:

6e1591947304 udc: s3c-hsudc: Silence warning about supplies during deferred probe
237b668c1c5d usb: gadget: s3c-hsudc: use devm_platform_ioremap_resource() to simplify code
b33f37064b74 usb: Remove dev_err() usage after platform_get_irq()
229e3682393c USB: gadget: udc: Remove redundant license text
5fd54ace4721 USB: add SPDX identifiers to all remaining files in drivers/usb/
977ac789507a usb: gadget: udc: constify usb_ep_ops structures
ca0709946023 usb: gadget: s3c-hsudc: remove redundant condition
bc1b9f300ae0 usb: gadget: s3c-hsudc: add ep capabilities support
22835b807e7c usb: gadget: remove unnecessary 'driver' argument
6ce372fed2cb usb: gadget: udc: s3c-hsudc: remove bind/unbind messages
82891b959cbb usb: gadget: udc: s3c-hsudc: do not rely on 'driver' argument
3dc3b4e15e09 usb: gadget: s3c-hsudc: delete unnecessary 'out of memory' messages
6d3f5f2d895b usb: gadget: udc: drop owner assignment from platform_drivers
304f7e5e1d08 usb: gadget: Refactor request completion
90fccb529d24 usb: gadget: Gadget directory cleanup - group UDC drivers
236064c25358 usb: gadget: s3c-hsudc: remove unused label
e117e742d310 usb: gadget: add "maxpacket_limit" field to struct usb_ep
e01ee9f509a9 usb: gadget: use dev_get_platdata()
38678f25689c usb: gadget: s3c-hsudc: delete outdated comment
492a39022ad5 usb: gadget: s3c-hsudc: don't touch gadget.dev.driver
4c422049bd0f usb: gadget: s3c-hsudc: remove unnecessary initializations
7bce401cc6db usb: gadget: drop now unnecessary flag
40ed30cff595 usb: gadget: s3c-hsudc: let udc-core manage gadget->dev
eeef45876631 (tag: gadget-for-v3.9) usb: gadget: constify all struct usb_gadget_ops
148e11349b0c usb: Convert to devm_ioremap_resource()
32b8666589d5 usb: gadget: remove u32 castings of address passed to readl()
924d2532ab18 usb: gadget: s3c-hsudc: Use devm_regulator_bulk_get
41ac7b3ab7fe usb: remove use of __devinit
dc2cdcaf4caa usb: gadget: s3c-hsudc: Replace 0 with NULL for pointers
affaab4c58d8 usb: gadget: s3c-hsudc: Add missing braces around sizeof
78f0c53ef856 usb: gadget: s3c-hsudc: Use devm_* functions
ded017ee6c7b usb: phy: fix return value check of usb_get_phy
662dca54ca67 usb: otg: support for multiple transceivers by a single controller
721002ec1dd5 usb: otg: utils: rename function name in OTG utils
109f0f718375 usb: gadget: s3c-hsudc.c: Remove unneeded condition
955846a60a9d usb: gadget: Update s3c-hsudc to use usb_endpoint_descriptor inside the struct usb_ep
6e13c6505cdf (tag: xceiv-for-v3.4) usb: otg: Convert all users to pass struct usb_otg for OTG functions
b96d3b08365f usb: Convert all users to new usb_phy
f9c56cdd3905 usb: gadget: Clear usb_endpoint_descriptor inside the struct usb_ep on disable
8675381109b0 usb: otg: Rename otg_transceiver to usb_phy
294f78ec493e usb: s3c-hsudc: add basic runtime_pm calls
2d4172c93874 usb: s3c-hsudc: Use helper functions instead of generic container_of
922be95a3f26 usb: gadget: s3c-hsudc: remove the_controller global
dee19be7d8ed usb: gadget: s3c-hsudc: use release_mem_region instead of release_resource
bab7d037c84f usb: gadget: s3c-hsudc: Add regulator handling
d93e2600d80f usb: gadget: s3c-hsudc: use udc_start and udc_stop functions
103495aaf0e7 usb: gadget: s3c-hsudc: move device registration to probe
e9bcb9e5feb0 usb: gadget: s3c-hsudc: add missing otg_put_transceiver in probe
a1977562718f usb: gadget: s3c-hsudc: add __devinit to probe function
715a3e41e78a usb: gadget: s3c-hsudc: move platform_data struct to global header
7177aed44f51 usb: gadget: rename usb_gadget_driver::speed to max_speed
d327ab5b6d66 usb: gadget: replace usb_gadget::is_dualspeed with max_speed
bfe0658b402d usb: udc: Fix gadget driver's speed check in various UDC drivers
cc27c96c2bee usb: convert drivers/usb/* to use module_platform_driver()
fba9e546eac9 s3c-hsudc: implement vbus_draw hook
29cc88979a88 USB: use usb_endpoint_maxp() instead of le16_to_cpu()
938fbe54f33e s3c-hsudc: Add basic otg transceiver handling
da4fc14c9955 s3c-hsudc: Fix possible nullpointer dereference during probe
86081d7be34e usb: gadget: add platform module alias where it is missing
0f91349b89f3 usb: gadget: convert all users to the new udc infrastructure
6bc129532176 usb/s3c-hsudc: fix error path
b38b03b363a5 usb: gadget: include <linux/prefetch.h> to fix compiling error
d6167660b284 USB: s3c-hsudc: fix checkpatch error and warning
004c127ef071 USB: s3c-hsudc: use IS_ERR() instead of NULL check
a9df304cf78d USB: Gadget: Add Samsung S3C24XX USB High-Speed controller driver

If there are no objections, I'm okay deleting the driver.
diff mbox series

Patch

diff --git a/arch/arm/mach-s3c24xx/include/mach/regs-s3c2443-clock.h b/arch/arm/mach-s3c24xx/include/mach/regs-s3c2443-clock.h
index 6bf924612b06..682759549e63 100644
--- a/arch/arm/mach-s3c24xx/include/mach/regs-s3c2443-clock.h
+++ b/arch/arm/mach-s3c24xx/include/mach/regs-s3c2443-clock.h
@@ -10,6 +10,8 @@ 
 #ifndef __ASM_ARM_REGS_S3C2443_CLOCK
 #define __ASM_ARM_REGS_S3C2443_CLOCK
 
+#include <linux/delay.h>
+
 #define S3C2443_CLKREG(x)		((x) + S3C24XX_VA_CLKPWR)
 
 #define S3C2443_PLLCON_MDIVSHIFT	16
@@ -184,5 +186,52 @@  s3c2443_get_epll(unsigned int pllval, unsigned int baseclk)
 	return (unsigned int)fvco;
 }
 
+static inline void s3c_hsudc_init_phy(void)
+{
+	u32 cfg;
+
+	cfg = readl(S3C2443_PWRCFG) | S3C2443_PWRCFG_USBPHY;
+	writel(cfg, S3C2443_PWRCFG);
+
+	cfg = readl(S3C2443_URSTCON);
+	cfg |= (S3C2443_URSTCON_FUNCRST | S3C2443_URSTCON_PHYRST);
+	writel(cfg, S3C2443_URSTCON);
+	mdelay(1);
+
+	cfg = readl(S3C2443_URSTCON);
+	cfg &= ~(S3C2443_URSTCON_FUNCRST | S3C2443_URSTCON_PHYRST);
+	writel(cfg, S3C2443_URSTCON);
+
+	cfg = readl(S3C2443_PHYCTRL);
+	cfg &= ~(S3C2443_PHYCTRL_CLKSEL | S3C2443_PHYCTRL_DSPORT);
+	cfg |= (S3C2443_PHYCTRL_EXTCLK | S3C2443_PHYCTRL_PLLSEL);
+	writel(cfg, S3C2443_PHYCTRL);
+
+	cfg = readl(S3C2443_PHYPWR);
+	cfg &= ~(S3C2443_PHYPWR_FSUSPEND | S3C2443_PHYPWR_PLL_PWRDN |
+		S3C2443_PHYPWR_XO_ON | S3C2443_PHYPWR_PLL_REFCLK |
+		S3C2443_PHYPWR_ANALOG_PD);
+	cfg |= S3C2443_PHYPWR_COMMON_ON;
+	writel(cfg, S3C2443_PHYPWR);
+
+	cfg = readl(S3C2443_UCLKCON);
+	cfg |= (S3C2443_UCLKCON_DETECT_VBUS | S3C2443_UCLKCON_FUNC_CLKEN |
+		S3C2443_UCLKCON_TCLKEN);
+	writel(cfg, S3C2443_UCLKCON);
+}
+
+static inline void s3c_hsudc_uninit_phy(void)
+{
+	u32 cfg;
+
+	cfg = readl(S3C2443_PWRCFG) & ~S3C2443_PWRCFG_USBPHY;
+	writel(cfg, S3C2443_PWRCFG);
+
+	writel(S3C2443_PHYPWR_FSUSPEND, S3C2443_PHYPWR);
+
+	cfg = readl(S3C2443_UCLKCON) & ~S3C2443_UCLKCON_FUNC_CLKEN;
+	writel(cfg, S3C2443_UCLKCON);
+}
+
 #endif /*  __ASM_ARM_REGS_S3C2443_CLOCK */
 
diff --git a/arch/arm/plat-samsung/devs.c b/arch/arm/plat-samsung/devs.c
index 2ed3ef604a25..0607d2984841 100644
--- a/arch/arm/plat-samsung/devs.c
+++ b/arch/arm/plat-samsung/devs.c
@@ -40,6 +40,10 @@ 
 #include <mach/irqs.h>
 #include <mach/map.h>
 
+#ifdef CONFIG_PLAT_S3C24XX
+#include <mach/regs-s3c2443-clock.h>
+#endif /* CONFIG_PLAT_S3C24XX */
+
 #include <plat/cpu.h>
 #include <plat/devs.h>
 #include <plat/adc.h>
@@ -1037,6 +1041,8 @@  struct platform_device s3c_device_usb_hsudc = {
 void __init s3c24xx_hsudc_set_platdata(struct s3c24xx_hsudc_platdata *pd)
 {
 	s3c_set_platdata(pd, sizeof(*pd), &s3c_device_usb_hsudc);
+	pd->phy_init = s3c_hsudc_init_phy;
+	pd->phy_uninit = s3c_hsudc_uninit_phy;
 }
 #endif /* CONFIG_PLAT_S3C24XX */
 
diff --git a/drivers/usb/gadget/udc/s3c-hsudc.c b/drivers/usb/gadget/udc/s3c-hsudc.c
index aaca1b0a2f59..7bd5182ce3ef 100644
--- a/drivers/usb/gadget/udc/s3c-hsudc.c
+++ b/drivers/usb/gadget/udc/s3c-hsudc.c
@@ -30,8 +30,6 @@ 
 #include <linux/regulator/consumer.h>
 #include <linux/pm_runtime.h>
 
-#include <mach/regs-s3c2443-clock.h>
-
 #define S3C_HSUDC_REG(x)	(x)
 
 /* Non-Indexed Registers */
@@ -186,53 +184,6 @@  static inline void __orr32(void __iomem *ptr, u32 val)
 	writel(readl(ptr) | val, ptr);
 }
 
-static void s3c_hsudc_init_phy(void)
-{
-	u32 cfg;
-
-	cfg = readl(S3C2443_PWRCFG) | S3C2443_PWRCFG_USBPHY;
-	writel(cfg, S3C2443_PWRCFG);
-
-	cfg = readl(S3C2443_URSTCON);
-	cfg |= (S3C2443_URSTCON_FUNCRST | S3C2443_URSTCON_PHYRST);
-	writel(cfg, S3C2443_URSTCON);
-	mdelay(1);
-
-	cfg = readl(S3C2443_URSTCON);
-	cfg &= ~(S3C2443_URSTCON_FUNCRST | S3C2443_URSTCON_PHYRST);
-	writel(cfg, S3C2443_URSTCON);
-
-	cfg = readl(S3C2443_PHYCTRL);
-	cfg &= ~(S3C2443_PHYCTRL_CLKSEL | S3C2443_PHYCTRL_DSPORT);
-	cfg |= (S3C2443_PHYCTRL_EXTCLK | S3C2443_PHYCTRL_PLLSEL);
-	writel(cfg, S3C2443_PHYCTRL);
-
-	cfg = readl(S3C2443_PHYPWR);
-	cfg &= ~(S3C2443_PHYPWR_FSUSPEND | S3C2443_PHYPWR_PLL_PWRDN |
-		S3C2443_PHYPWR_XO_ON | S3C2443_PHYPWR_PLL_REFCLK |
-		S3C2443_PHYPWR_ANALOG_PD);
-	cfg |= S3C2443_PHYPWR_COMMON_ON;
-	writel(cfg, S3C2443_PHYPWR);
-
-	cfg = readl(S3C2443_UCLKCON);
-	cfg |= (S3C2443_UCLKCON_DETECT_VBUS | S3C2443_UCLKCON_FUNC_CLKEN |
-		S3C2443_UCLKCON_TCLKEN);
-	writel(cfg, S3C2443_UCLKCON);
-}
-
-static void s3c_hsudc_uninit_phy(void)
-{
-	u32 cfg;
-
-	cfg = readl(S3C2443_PWRCFG) & ~S3C2443_PWRCFG_USBPHY;
-	writel(cfg, S3C2443_PWRCFG);
-
-	writel(S3C2443_PHYPWR_FSUSPEND, S3C2443_PHYPWR);
-
-	cfg = readl(S3C2443_UCLKCON) & ~S3C2443_UCLKCON_FUNC_CLKEN;
-	writel(cfg, S3C2443_UCLKCON);
-}
-
 /**
  * s3c_hsudc_complete_request - Complete a transfer request.
  * @hsep: Endpoint to which the request belongs.
@@ -1188,7 +1139,8 @@  static int s3c_hsudc_start(struct usb_gadget *gadget,
 
 	pm_runtime_get_sync(hsudc->dev);
 
-	s3c_hsudc_init_phy();
+	if (hsudc->pd->phy_init)
+		hsudc->pd->phy_init();
 	if (hsudc->pd->gpio_init)
 		hsudc->pd->gpio_init();
 
@@ -1210,7 +1162,8 @@  static int s3c_hsudc_stop(struct usb_gadget *gadget)
 
 	spin_lock_irqsave(&hsudc->lock, flags);
 	hsudc->gadget.speed = USB_SPEED_UNKNOWN;
-	s3c_hsudc_uninit_phy();
+	if (hsudc->pd->phy_uninit)
+		hsudc->pd->phy_uninit();
 
 	pm_runtime_put(hsudc->dev);
 
diff --git a/include/linux/platform_data/s3c-hsudc.h b/include/linux/platform_data/s3c-hsudc.h
index 4dc9b8760166..a170939832d5 100644
--- a/include/linux/platform_data/s3c-hsudc.h
+++ b/include/linux/platform_data/s3c-hsudc.h
@@ -26,6 +26,8 @@  struct s3c24xx_hsudc_platdata {
 	unsigned int	epnum;
 	void		(*gpio_init)(void);
 	void		(*gpio_uninit)(void);
+	void		(*phy_init)(void);
+	void		(*phy_uninit)(void);
 };
 
 #endif	/* __LINUX_USB_S3C_HSUDC_H */