| Message ID | 20200806182059.2431-9-krzk@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,01/41] ARM: s3c: Remove unneeded machine header includes | expand |
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?
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
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
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 --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 */