* [PATCH v1 1/3] usb: host: npcm7xx: remove USB EHCI host reset sequence
@ 2022-07-18 12:29 ` Tomer Maimon
0 siblings, 0 replies; 15+ messages in thread
From: Tomer Maimon @ 2022-07-18 12:29 UTC (permalink / raw)
To: avifishman70, tali.perry1, joel, venture, yuenn, benjaminfair,
gregkh, stern, tony, felipe.balbi, jgross, lukas.bulwahn, arnd,
robh+dt, krzysztof.kozlowski+dt
Cc: openbmc, linux-usb, linux-kernel, devicetree, Tomer Maimon
Remove USB EHCI host controller reset sequence from NPCM7XX USB EHCI
host probe function because it is done in the NPCM reset driver.
Due to it, NPCM7XX EHCI driver configuration is dependent on NPCM reset.
Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
---
drivers/usb/host/Kconfig | 2 +-
drivers/usb/host/ehci-npcm7xx.c | 47 ---------------------------------
2 files changed, 1 insertion(+), 48 deletions(-)
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 682b3d2da623..e05e2cf806f8 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -206,7 +206,7 @@ config USB_EHCI_FSL
config USB_EHCI_HCD_NPCM7XX
tristate "Support for Nuvoton NPCM7XX on-chip EHCI USB controller"
- depends on (USB_EHCI_HCD && ARCH_NPCM7XX) || COMPILE_TEST
+ depends on (USB_EHCI_HCD && ARCH_NPCM7XX && RESET_NPCM) || COMPILE_TEST
default y if (USB_EHCI_HCD && ARCH_NPCM7XX)
help
Enables support for the on-chip EHCI controller on
diff --git a/drivers/usb/host/ehci-npcm7xx.c b/drivers/usb/host/ehci-npcm7xx.c
index 6b5a7a873e01..955e7c8f3db8 100644
--- a/drivers/usb/host/ehci-npcm7xx.c
+++ b/drivers/usb/host/ehci-npcm7xx.c
@@ -28,13 +28,6 @@
#define DRIVER_DESC "EHCI npcm7xx driver"
static const char hcd_name[] = "npcm7xx-ehci";
-
-#define USB2PHYCTL_OFFSET 0x144
-
-#define IPSRST2_OFFSET 0x24
-#define IPSRST3_OFFSET 0x34
-
-
static struct hc_driver __read_mostly ehci_npcm7xx_hc_driver;
static int __maybe_unused ehci_npcm7xx_drv_suspend(struct device *dev)
@@ -60,52 +53,12 @@ static int npcm7xx_ehci_hcd_drv_probe(struct platform_device *pdev)
{
struct usb_hcd *hcd;
struct resource *res;
- struct regmap *gcr_regmap;
- struct regmap *rst_regmap;
const struct hc_driver *driver = &ehci_npcm7xx_hc_driver;
int irq;
int retval;
dev_dbg(&pdev->dev, "initializing npcm7xx ehci USB Controller\n");
- gcr_regmap = syscon_regmap_lookup_by_compatible("nuvoton,npcm750-gcr");
- if (IS_ERR(gcr_regmap)) {
- dev_err(&pdev->dev, "%s: failed to find nuvoton,npcm750-gcr\n",
- __func__);
- return PTR_ERR(gcr_regmap);
- }
-
- rst_regmap = syscon_regmap_lookup_by_compatible("nuvoton,npcm750-rst");
- if (IS_ERR(rst_regmap)) {
- dev_err(&pdev->dev, "%s: failed to find nuvoton,npcm750-rst\n",
- __func__);
- return PTR_ERR(rst_regmap);
- }
-
- /********* phy init ******/
- // reset usb host
- regmap_update_bits(rst_regmap, IPSRST2_OFFSET,
- (0x1 << 26), (0x1 << 26));
- regmap_update_bits(rst_regmap, IPSRST3_OFFSET,
- (0x1 << 25), (0x1 << 25));
- regmap_update_bits(gcr_regmap, USB2PHYCTL_OFFSET,
- (0x1 << 28), 0);
-
- udelay(1);
-
- // enable phy
- regmap_update_bits(rst_regmap, IPSRST3_OFFSET,
- (0x1 << 25), 0);
-
- udelay(50); // enable phy
-
- regmap_update_bits(gcr_regmap, USB2PHYCTL_OFFSET,
- (0x1 << 28), (0x1 << 28));
-
- // enable host
- regmap_update_bits(rst_regmap, IPSRST2_OFFSET,
- (0x1 << 26), 0);
-
if (usb_disabled())
return -ENODEV;
--
2.33.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v1 1/3] usb: host: npcm7xx: remove USB EHCI host reset sequence
2022-07-18 12:29 ` Tomer Maimon
@ 2022-07-18 12:38 ` Arnd Bergmann
-1 siblings, 0 replies; 15+ messages in thread
From: Arnd Bergmann @ 2022-07-18 12:38 UTC (permalink / raw)
To: Tomer Maimon
Cc: Avi Fishman, Tali Perry, Joel Stanley, Patrick Venture,
Nancy Yuen, Benjamin Fair, gregkh, Alan Stern, Tony Lindgren,
Felipe Balbi, Juergen Gross, Lukas Bulwahn, Arnd Bergmann,
Rob Herring, Krzysztof Kozlowski, OpenBMC Maillist, USB list,
Linux Kernel Mailing List, DTML
On Mon, Jul 18, 2022 at 2:29 PM Tomer Maimon <tmaimon77@gmail.com> wrote:
>
> config USB_EHCI_HCD_NPCM7XX
> tristate "Support for Nuvoton NPCM7XX on-chip EHCI USB controller"
> - depends on (USB_EHCI_HCD && ARCH_NPCM7XX) || COMPILE_TEST
> + depends on (USB_EHCI_HCD && ARCH_NPCM7XX && RESET_NPCM) || COMPILE_TEST
> default y if (USB_EHCI_HCD && ARCH_NPCM7XX)
> help
> Enables support for the on-chip EHCI controller on
I would leave out this Kconfig change, there is really no need to enforce
this specific dependency. It is expected that a device driver has dependencies
on several other subsystems (irqchip, reset, pinctrl, clock, ....) and will only
work if the required drivers are also built for the same kernel.
Also, forcing the USB driver to be a loadable module when the reset driver
is a module (rather than built-in) does not guarantee that they are initialized
in the correct order. If only the USB driver is built-in and the reset driver is
a module, or both are loadable modules and USB gets loaded first, then
the probe() function should notice this and return -EPROBE_DEFER so
it will be retried after the reset driver is successfully loaded.
Arnd
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v1 1/3] usb: host: npcm7xx: remove USB EHCI host reset sequence
@ 2022-07-18 12:38 ` Arnd Bergmann
0 siblings, 0 replies; 15+ messages in thread
From: Arnd Bergmann @ 2022-07-18 12:38 UTC (permalink / raw)
To: Tomer Maimon
Cc: Juergen Gross, DTML, Benjamin Fair, Felipe Balbi, Avi Fishman,
Patrick Venture, OpenBMC Maillist, USB list,
Linux Kernel Mailing List, Tali Perry, Tony Lindgren,
Rob Herring, Alan Stern, Joel Stanley, Krzysztof Kozlowski,
gregkh, Lukas Bulwahn, Arnd Bergmann
On Mon, Jul 18, 2022 at 2:29 PM Tomer Maimon <tmaimon77@gmail.com> wrote:
>
> config USB_EHCI_HCD_NPCM7XX
> tristate "Support for Nuvoton NPCM7XX on-chip EHCI USB controller"
> - depends on (USB_EHCI_HCD && ARCH_NPCM7XX) || COMPILE_TEST
> + depends on (USB_EHCI_HCD && ARCH_NPCM7XX && RESET_NPCM) || COMPILE_TEST
> default y if (USB_EHCI_HCD && ARCH_NPCM7XX)
> help
> Enables support for the on-chip EHCI controller on
I would leave out this Kconfig change, there is really no need to enforce
this specific dependency. It is expected that a device driver has dependencies
on several other subsystems (irqchip, reset, pinctrl, clock, ....) and will only
work if the required drivers are also built for the same kernel.
Also, forcing the USB driver to be a loadable module when the reset driver
is a module (rather than built-in) does not guarantee that they are initialized
in the correct order. If only the USB driver is built-in and the reset driver is
a module, or both are loadable modules and USB gets loaded first, then
the probe() function should notice this and return -EPROBE_DEFER so
it will be retried after the reset driver is successfully loaded.
Arnd
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v1 1/3] usb: host: npcm7xx: remove USB EHCI host reset sequence
2022-07-18 12:38 ` Arnd Bergmann
@ 2022-07-18 14:29 ` Tomer Maimon
-1 siblings, 0 replies; 15+ messages in thread
From: Tomer Maimon @ 2022-07-18 14:29 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Avi Fishman, Tali Perry, Joel Stanley, Patrick Venture,
Nancy Yuen, Benjamin Fair, gregkh, Alan Stern, Tony Lindgren,
Felipe Balbi, Juergen Gross, Lukas Bulwahn, Rob Herring,
Krzysztof Kozlowski, OpenBMC Maillist, USB list,
Linux Kernel Mailing List, DTML
Hi Arnd,
Thanks for your detailed explanation.
I will remove this modification in the next version.
On Mon, 18 Jul 2022 at 15:38, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Mon, Jul 18, 2022 at 2:29 PM Tomer Maimon <tmaimon77@gmail.com> wrote:
> >
> > config USB_EHCI_HCD_NPCM7XX
> > tristate "Support for Nuvoton NPCM7XX on-chip EHCI USB controller"
> > - depends on (USB_EHCI_HCD && ARCH_NPCM7XX) || COMPILE_TEST
> > + depends on (USB_EHCI_HCD && ARCH_NPCM7XX && RESET_NPCM) || COMPILE_TEST
> > default y if (USB_EHCI_HCD && ARCH_NPCM7XX)
> > help
> > Enables support for the on-chip EHCI controller on
>
> I would leave out this Kconfig change, there is really no need to enforce
> this specific dependency. It is expected that a device driver has dependencies
> on several other subsystems (irqchip, reset, pinctrl, clock, ....) and will only
> work if the required drivers are also built for the same kernel.
>
> Also, forcing the USB driver to be a loadable module when the reset driver
> is a module (rather than built-in) does not guarantee that they are initialized
> in the correct order. If only the USB driver is built-in and the reset driver is
> a module, or both are loadable modules and USB gets loaded first, then
> the probe() function should notice this and return -EPROBE_DEFER so
> it will be retried after the reset driver is successfully loaded.
>
> Arnd
Best regards,
Tomer
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/3] usb: host: npcm7xx: remove USB EHCI host reset sequence
@ 2022-07-18 14:29 ` Tomer Maimon
0 siblings, 0 replies; 15+ messages in thread
From: Tomer Maimon @ 2022-07-18 14:29 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Juergen Gross, DTML, Benjamin Fair, Felipe Balbi, Avi Fishman,
Patrick Venture, OpenBMC Maillist, USB list,
Linux Kernel Mailing List, Tali Perry, Tony Lindgren,
Rob Herring, Alan Stern, Joel Stanley, Krzysztof Kozlowski,
gregkh, Lukas Bulwahn
Hi Arnd,
Thanks for your detailed explanation.
I will remove this modification in the next version.
On Mon, 18 Jul 2022 at 15:38, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Mon, Jul 18, 2022 at 2:29 PM Tomer Maimon <tmaimon77@gmail.com> wrote:
> >
> > config USB_EHCI_HCD_NPCM7XX
> > tristate "Support for Nuvoton NPCM7XX on-chip EHCI USB controller"
> > - depends on (USB_EHCI_HCD && ARCH_NPCM7XX) || COMPILE_TEST
> > + depends on (USB_EHCI_HCD && ARCH_NPCM7XX && RESET_NPCM) || COMPILE_TEST
> > default y if (USB_EHCI_HCD && ARCH_NPCM7XX)
> > help
> > Enables support for the on-chip EHCI controller on
>
> I would leave out this Kconfig change, there is really no need to enforce
> this specific dependency. It is expected that a device driver has dependencies
> on several other subsystems (irqchip, reset, pinctrl, clock, ....) and will only
> work if the required drivers are also built for the same kernel.
>
> Also, forcing the USB driver to be a loadable module when the reset driver
> is a module (rather than built-in) does not guarantee that they are initialized
> in the correct order. If only the USB driver is built-in and the reset driver is
> a module, or both are loadable modules and USB gets loaded first, then
> the probe() function should notice this and return -EPROBE_DEFER so
> it will be retried after the reset driver is successfully loaded.
>
> Arnd
Best regards,
Tomer
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/3] usb: host: npcm7xx: remove USB EHCI host reset sequence
2022-07-18 12:29 ` Tomer Maimon
@ 2022-07-18 14:30 ` Alan Stern
-1 siblings, 0 replies; 15+ messages in thread
From: Alan Stern @ 2022-07-18 14:30 UTC (permalink / raw)
To: Tomer Maimon
Cc: avifishman70, tali.perry1, joel, venture, yuenn, benjaminfair,
gregkh, tony, felipe.balbi, jgross, lukas.bulwahn, arnd, robh+dt,
krzysztof.kozlowski+dt, openbmc, linux-usb, linux-kernel,
devicetree
On Mon, Jul 18, 2022 at 03:29:20PM +0300, Tomer Maimon wrote:
> Remove USB EHCI host controller reset sequence from NPCM7XX USB EHCI
> host probe function because it is done in the NPCM reset driver.
>
> Due to it, NPCM7XX EHCI driver configuration is dependent on NPCM reset.
>
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> ---
Regarding the changes to ehci-npcm7xx.c:
Acked-by: Alan Stern <stern@rowland.harvard.edu>
But you probably should remove the "#include <linux/regmap.h>" line near
the start of the source file.
Alan Stern
> drivers/usb/host/Kconfig | 2 +-
> drivers/usb/host/ehci-npcm7xx.c | 47 ---------------------------------
> 2 files changed, 1 insertion(+), 48 deletions(-)
>
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index 682b3d2da623..e05e2cf806f8 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -206,7 +206,7 @@ config USB_EHCI_FSL
>
> config USB_EHCI_HCD_NPCM7XX
> tristate "Support for Nuvoton NPCM7XX on-chip EHCI USB controller"
> - depends on (USB_EHCI_HCD && ARCH_NPCM7XX) || COMPILE_TEST
> + depends on (USB_EHCI_HCD && ARCH_NPCM7XX && RESET_NPCM) || COMPILE_TEST
> default y if (USB_EHCI_HCD && ARCH_NPCM7XX)
> help
> Enables support for the on-chip EHCI controller on
> diff --git a/drivers/usb/host/ehci-npcm7xx.c b/drivers/usb/host/ehci-npcm7xx.c
> index 6b5a7a873e01..955e7c8f3db8 100644
> --- a/drivers/usb/host/ehci-npcm7xx.c
> +++ b/drivers/usb/host/ehci-npcm7xx.c
> @@ -28,13 +28,6 @@
> #define DRIVER_DESC "EHCI npcm7xx driver"
>
> static const char hcd_name[] = "npcm7xx-ehci";
> -
> -#define USB2PHYCTL_OFFSET 0x144
> -
> -#define IPSRST2_OFFSET 0x24
> -#define IPSRST3_OFFSET 0x34
> -
> -
> static struct hc_driver __read_mostly ehci_npcm7xx_hc_driver;
>
> static int __maybe_unused ehci_npcm7xx_drv_suspend(struct device *dev)
> @@ -60,52 +53,12 @@ static int npcm7xx_ehci_hcd_drv_probe(struct platform_device *pdev)
> {
> struct usb_hcd *hcd;
> struct resource *res;
> - struct regmap *gcr_regmap;
> - struct regmap *rst_regmap;
> const struct hc_driver *driver = &ehci_npcm7xx_hc_driver;
> int irq;
> int retval;
>
> dev_dbg(&pdev->dev, "initializing npcm7xx ehci USB Controller\n");
>
> - gcr_regmap = syscon_regmap_lookup_by_compatible("nuvoton,npcm750-gcr");
> - if (IS_ERR(gcr_regmap)) {
> - dev_err(&pdev->dev, "%s: failed to find nuvoton,npcm750-gcr\n",
> - __func__);
> - return PTR_ERR(gcr_regmap);
> - }
> -
> - rst_regmap = syscon_regmap_lookup_by_compatible("nuvoton,npcm750-rst");
> - if (IS_ERR(rst_regmap)) {
> - dev_err(&pdev->dev, "%s: failed to find nuvoton,npcm750-rst\n",
> - __func__);
> - return PTR_ERR(rst_regmap);
> - }
> -
> - /********* phy init ******/
> - // reset usb host
> - regmap_update_bits(rst_regmap, IPSRST2_OFFSET,
> - (0x1 << 26), (0x1 << 26));
> - regmap_update_bits(rst_regmap, IPSRST3_OFFSET,
> - (0x1 << 25), (0x1 << 25));
> - regmap_update_bits(gcr_regmap, USB2PHYCTL_OFFSET,
> - (0x1 << 28), 0);
> -
> - udelay(1);
> -
> - // enable phy
> - regmap_update_bits(rst_regmap, IPSRST3_OFFSET,
> - (0x1 << 25), 0);
> -
> - udelay(50); // enable phy
> -
> - regmap_update_bits(gcr_regmap, USB2PHYCTL_OFFSET,
> - (0x1 << 28), (0x1 << 28));
> -
> - // enable host
> - regmap_update_bits(rst_regmap, IPSRST2_OFFSET,
> - (0x1 << 26), 0);
> -
> if (usb_disabled())
> return -ENODEV;
>
> --
> 2.33.0
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v1 1/3] usb: host: npcm7xx: remove USB EHCI host reset sequence
@ 2022-07-18 14:30 ` Alan Stern
0 siblings, 0 replies; 15+ messages in thread
From: Alan Stern @ 2022-07-18 14:30 UTC (permalink / raw)
To: Tomer Maimon
Cc: jgross, devicetree, benjaminfair, felipe.balbi, avifishman70,
venture, openbmc, linux-usb, linux-kernel, tali.perry1, tony,
robh+dt, arnd, joel, krzysztof.kozlowski+dt, gregkh,
lukas.bulwahn
On Mon, Jul 18, 2022 at 03:29:20PM +0300, Tomer Maimon wrote:
> Remove USB EHCI host controller reset sequence from NPCM7XX USB EHCI
> host probe function because it is done in the NPCM reset driver.
>
> Due to it, NPCM7XX EHCI driver configuration is dependent on NPCM reset.
>
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> ---
Regarding the changes to ehci-npcm7xx.c:
Acked-by: Alan Stern <stern@rowland.harvard.edu>
But you probably should remove the "#include <linux/regmap.h>" line near
the start of the source file.
Alan Stern
> drivers/usb/host/Kconfig | 2 +-
> drivers/usb/host/ehci-npcm7xx.c | 47 ---------------------------------
> 2 files changed, 1 insertion(+), 48 deletions(-)
>
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index 682b3d2da623..e05e2cf806f8 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -206,7 +206,7 @@ config USB_EHCI_FSL
>
> config USB_EHCI_HCD_NPCM7XX
> tristate "Support for Nuvoton NPCM7XX on-chip EHCI USB controller"
> - depends on (USB_EHCI_HCD && ARCH_NPCM7XX) || COMPILE_TEST
> + depends on (USB_EHCI_HCD && ARCH_NPCM7XX && RESET_NPCM) || COMPILE_TEST
> default y if (USB_EHCI_HCD && ARCH_NPCM7XX)
> help
> Enables support for the on-chip EHCI controller on
> diff --git a/drivers/usb/host/ehci-npcm7xx.c b/drivers/usb/host/ehci-npcm7xx.c
> index 6b5a7a873e01..955e7c8f3db8 100644
> --- a/drivers/usb/host/ehci-npcm7xx.c
> +++ b/drivers/usb/host/ehci-npcm7xx.c
> @@ -28,13 +28,6 @@
> #define DRIVER_DESC "EHCI npcm7xx driver"
>
> static const char hcd_name[] = "npcm7xx-ehci";
> -
> -#define USB2PHYCTL_OFFSET 0x144
> -
> -#define IPSRST2_OFFSET 0x24
> -#define IPSRST3_OFFSET 0x34
> -
> -
> static struct hc_driver __read_mostly ehci_npcm7xx_hc_driver;
>
> static int __maybe_unused ehci_npcm7xx_drv_suspend(struct device *dev)
> @@ -60,52 +53,12 @@ static int npcm7xx_ehci_hcd_drv_probe(struct platform_device *pdev)
> {
> struct usb_hcd *hcd;
> struct resource *res;
> - struct regmap *gcr_regmap;
> - struct regmap *rst_regmap;
> const struct hc_driver *driver = &ehci_npcm7xx_hc_driver;
> int irq;
> int retval;
>
> dev_dbg(&pdev->dev, "initializing npcm7xx ehci USB Controller\n");
>
> - gcr_regmap = syscon_regmap_lookup_by_compatible("nuvoton,npcm750-gcr");
> - if (IS_ERR(gcr_regmap)) {
> - dev_err(&pdev->dev, "%s: failed to find nuvoton,npcm750-gcr\n",
> - __func__);
> - return PTR_ERR(gcr_regmap);
> - }
> -
> - rst_regmap = syscon_regmap_lookup_by_compatible("nuvoton,npcm750-rst");
> - if (IS_ERR(rst_regmap)) {
> - dev_err(&pdev->dev, "%s: failed to find nuvoton,npcm750-rst\n",
> - __func__);
> - return PTR_ERR(rst_regmap);
> - }
> -
> - /********* phy init ******/
> - // reset usb host
> - regmap_update_bits(rst_regmap, IPSRST2_OFFSET,
> - (0x1 << 26), (0x1 << 26));
> - regmap_update_bits(rst_regmap, IPSRST3_OFFSET,
> - (0x1 << 25), (0x1 << 25));
> - regmap_update_bits(gcr_regmap, USB2PHYCTL_OFFSET,
> - (0x1 << 28), 0);
> -
> - udelay(1);
> -
> - // enable phy
> - regmap_update_bits(rst_regmap, IPSRST3_OFFSET,
> - (0x1 << 25), 0);
> -
> - udelay(50); // enable phy
> -
> - regmap_update_bits(gcr_regmap, USB2PHYCTL_OFFSET,
> - (0x1 << 28), (0x1 << 28));
> -
> - // enable host
> - regmap_update_bits(rst_regmap, IPSRST2_OFFSET,
> - (0x1 << 26), 0);
> -
> if (usb_disabled())
> return -ENODEV;
>
> --
> 2.33.0
>
^ permalink raw reply [flat|nested] 15+ messages in thread