2023-10-12 23:01:32

by Tomer Maimon

[permalink] [raw]
Subject: [PATCH v6 0/3] usb: ChipIdea: add Nuvoton NPCM UDC support

This patch set add USB device controller for the NPCM Baseboard
Management Controllers (BMC).

NPCM UDC driver is a part of the USB ChipIdea driver.

Adding CI_HDRC_FORCE_VBUS_ACTIVE_ALWAYS flag to modify the vbus_active
parameter to active in case the ChipIdea USB IP role is device-only and
there is no otgsc register.

BMC NPCM7XX and BMC NPCM8XX has ten identical NPCM UDC modules,

The NPCM UDC were tested on NPCM845 evaluation board.

Addressed comments from:
- Krzysztof Kozlowski : https://www.spinics.net/lists/devicetree/msg639032.html

Changes since version 5:
- Remove unnecessary npcm845-udc compatible.

Changes since version 4:
- Modify npcm845-udc compatible.

Changes since version 3:
- Add Acked-by Peter Chen.

Changes since version 2:
- Use dev_err_probe.
- Remove MODULE_ALIAS.

Changes since version 1:
- Add SoC specific compatible.
- Remove USB phy mux property from dt-binding, will be handled differently.
- Add CI_HDRC_FORCE_VBUS_ACTIVE_ALWAYS commit to this patch set.

Tomer Maimon (3):
usb: chipidea: add CI_HDRC_FORCE_VBUS_ACTIVE_ALWAYS flag
dt-bindings: usb: ci-hdrc-usb2: add npcm750 and npcm845 compatible
usb: chipidea: Add support for NPCM

.../devicetree/bindings/usb/ci-hdrc-usb2.yaml | 6 +
drivers/usb/chipidea/Kconfig | 4 +
drivers/usb/chipidea/Makefile | 1 +
drivers/usb/chipidea/ci_hdrc_npcm.c | 114 ++++++++++++++++++
drivers/usb/chipidea/otg.c | 5 +-
include/linux/usb/chipidea.h | 1 +
6 files changed, 130 insertions(+), 1 deletion(-)
create mode 100644 drivers/usb/chipidea/ci_hdrc_npcm.c

--
2.33.0


2023-10-12 23:01:32

by Tomer Maimon

[permalink] [raw]
Subject: [PATCH v6 1/3] usb: chipidea: add CI_HDRC_FORCE_VBUS_ACTIVE_ALWAYS flag

Adding CI_HDRC_FORCE_VBUS_ACTIVE_ALWAYS flag to modify the vbus_active
parameter to active in case the ChipIdea USB IP role is device-only and
there is no otgsc register.

Signed-off-by: Tomer Maimon <[email protected]>
Acked-by: Peter Chen <[email protected]>
---
drivers/usb/chipidea/otg.c | 5 ++++-
include/linux/usb/chipidea.h | 1 +
2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
index f5490f2a5b6b..647e98f4e351 100644
--- a/drivers/usb/chipidea/otg.c
+++ b/drivers/usb/chipidea/otg.c
@@ -130,8 +130,11 @@ enum ci_role ci_otg_role(struct ci_hdrc *ci)

void ci_handle_vbus_change(struct ci_hdrc *ci)
{
- if (!ci->is_otg)
+ if (!ci->is_otg) {
+ if (ci->platdata->flags & CI_HDRC_FORCE_VBUS_ACTIVE_ALWAYS)
+ usb_gadget_vbus_connect(&ci->gadget);
return;
+ }

if (hw_read_otgsc(ci, OTGSC_BSV) && !ci->vbus_active)
usb_gadget_vbus_connect(&ci->gadget);
diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
index 0b4f2d5faa08..5a7f96684ea2 100644
--- a/include/linux/usb/chipidea.h
+++ b/include/linux/usb/chipidea.h
@@ -64,6 +64,7 @@ struct ci_hdrc_platform_data {
#define CI_HDRC_PMQOS BIT(15)
#define CI_HDRC_PHY_VBUS_CONTROL BIT(16)
#define CI_HDRC_HAS_PORTSC_PEC_MISSED BIT(17)
+#define CI_HDRC_FORCE_VBUS_ACTIVE_ALWAYS BIT(18)
enum usb_dr_mode dr_mode;
#define CI_HDRC_CONTROLLER_RESET_EVENT 0
#define CI_HDRC_CONTROLLER_STOPPED_EVENT 1
--
2.33.0

2023-10-12 23:02:24

by Tomer Maimon

[permalink] [raw]
Subject: [PATCH v6 3/3] usb: chipidea: Add support for NPCM

Add Nuvoton NPCM BMC SoCs support to USB ChipIdea driver.
NPCM SoC include ChipIdea IP block that used for USB device controller
mode.

Signed-off-by: Tomer Maimon <[email protected]>
Acked-by: Peter Chen <[email protected]>
---
drivers/usb/chipidea/Kconfig | 4 +
drivers/usb/chipidea/Makefile | 1 +
drivers/usb/chipidea/ci_hdrc_npcm.c | 114 ++++++++++++++++++++++++++++
3 files changed, 119 insertions(+)
create mode 100644 drivers/usb/chipidea/ci_hdrc_npcm.c

diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig
index c815824a0b2d..bab45bc62361 100644
--- a/drivers/usb/chipidea/Kconfig
+++ b/drivers/usb/chipidea/Kconfig
@@ -43,6 +43,10 @@ config USB_CHIPIDEA_MSM
tristate "Enable MSM hsusb glue driver" if EXPERT
default USB_CHIPIDEA

+config USB_CHIPIDEA_NPCM
+ tristate "Enable NPCM hsusb glue driver" if EXPERT
+ default USB_CHIPIDEA
+
config USB_CHIPIDEA_IMX
tristate "Enable i.MX USB glue driver" if EXPERT
depends on OF
diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
index 71afeab97e83..718cb24603dd 100644
--- a/drivers/usb/chipidea/Makefile
+++ b/drivers/usb/chipidea/Makefile
@@ -13,6 +13,7 @@ ci_hdrc-$(CONFIG_USB_OTG_FSM) += otg_fsm.o

obj-$(CONFIG_USB_CHIPIDEA_GENERIC) += ci_hdrc_usb2.o
obj-$(CONFIG_USB_CHIPIDEA_MSM) += ci_hdrc_msm.o
+obj-$(CONFIG_USB_CHIPIDEA_NPCM) += ci_hdrc_npcm.o
obj-$(CONFIG_USB_CHIPIDEA_PCI) += ci_hdrc_pci.o
obj-$(CONFIG_USB_CHIPIDEA_IMX) += usbmisc_imx.o ci_hdrc_imx.o
obj-$(CONFIG_USB_CHIPIDEA_TEGRA) += ci_hdrc_tegra.o
diff --git a/drivers/usb/chipidea/ci_hdrc_npcm.c b/drivers/usb/chipidea/ci_hdrc_npcm.c
new file mode 100644
index 000000000000..37b64a3dbd96
--- /dev/null
+++ b/drivers/usb/chipidea/ci_hdrc_npcm.c
@@ -0,0 +1,114 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2023 Nuvoton Technology corporation.
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/usb/chipidea.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/reset-controller.h>
+#include <linux/of.h>
+
+#include "ci.h"
+
+struct npcm_udc_data {
+ struct platform_device *ci;
+ struct clk *core_clk;
+ struct ci_hdrc_platform_data pdata;
+};
+
+static int npcm_udc_notify_event(struct ci_hdrc *ci, unsigned event)
+{
+ struct device *dev = ci->dev->parent;
+
+ switch (event) {
+ case CI_HDRC_CONTROLLER_RESET_EVENT:
+ /* clear all mode bits */
+ hw_write(ci, OP_USBMODE, 0xffffffff, 0x0);
+ break;
+ default:
+ dev_dbg(dev, "unknown ci_hdrc event\n");
+ break;
+ }
+
+ return 0;
+}
+
+static int npcm_udc_probe(struct platform_device *pdev)
+{
+ int ret;
+ struct npcm_udc_data *ci;
+ struct platform_device *plat_ci;
+ struct device *dev = &pdev->dev;
+
+ ci = devm_kzalloc(&pdev->dev, sizeof(*ci), GFP_KERNEL);
+ if (!ci)
+ return -ENOMEM;
+ platform_set_drvdata(pdev, ci);
+
+ ci->core_clk = devm_clk_get_optional(dev, NULL);
+ if (IS_ERR(ci->core_clk))
+ return PTR_ERR(ci->core_clk);
+
+ ret = clk_prepare_enable(ci->core_clk);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to enable the clock: %d\n", ret);
+
+ ci->pdata.name = dev_name(dev);
+ ci->pdata.capoffset = DEF_CAPOFFSET;
+ ci->pdata.flags = CI_HDRC_REQUIRES_ALIGNED_DMA |
+ CI_HDRC_FORCE_VBUS_ACTIVE_ALWAYS;
+ ci->pdata.phy_mode = USBPHY_INTERFACE_MODE_UTMI;
+ ci->pdata.notify_event = npcm_udc_notify_event;
+
+ plat_ci = ci_hdrc_add_device(dev, pdev->resource, pdev->num_resources,
+ &ci->pdata);
+ if (IS_ERR(plat_ci)) {
+ ret = PTR_ERR(plat_ci);
+ dev_err(dev, "failed to register HDRC NPCM device: %d\n", ret);
+ goto clk_err;
+ }
+
+ pm_runtime_no_callbacks(dev);
+ pm_runtime_enable(dev);
+
+ return 0;
+
+clk_err:
+ clk_disable_unprepare(ci->core_clk);
+ return ret;
+}
+
+static int npcm_udc_remove(struct platform_device *pdev)
+{
+ struct npcm_udc_data *ci = platform_get_drvdata(pdev);
+
+ pm_runtime_disable(&pdev->dev);
+ ci_hdrc_remove_device(ci->ci);
+ clk_disable_unprepare(ci->core_clk);
+
+ return 0;
+}
+
+static const struct of_device_id npcm_udc_dt_match[] = {
+ { .compatible = "nuvoton,npcm750-udc", },
+ { .compatible = "nuvoton,npcm845-udc", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, npcm_udc_dt_match);
+
+static struct platform_driver npcm_udc_driver = {
+ .probe = npcm_udc_probe,
+ .remove = npcm_udc_remove,
+ .driver = {
+ .name = "npcm_udc",
+ .of_match_table = npcm_udc_dt_match,
+ },
+};
+
+module_platform_driver(npcm_udc_driver);
+
+MODULE_DESCRIPTION("NPCM USB device controller driver");
+MODULE_AUTHOR("Tomer Maimon <[email protected]>");
+MODULE_LICENSE("GPL v2");
--
2.33.0

2023-10-12 23:02:25

by Tomer Maimon

[permalink] [raw]
Subject: [PATCH v6 2/3] dt-bindings: usb: ci-hdrc-usb2: add npcm750 and npcm845 compatible

Add a compatible string for Nuvoton BMC NPCM750 and Nuvoton BMC NPCM845.

Signed-off-by: Tomer Maimon <[email protected]>
---
Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
index 1394557517b1..f5ec1aef839e 100644
--- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
+++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
@@ -16,6 +16,7 @@ properties:
- enum:
- chipidea,usb2
- lsi,zevio-usb
+ - nuvoton,npcm750-udc
- nvidia,tegra20-ehci
- nvidia,tegra20-udc
- nvidia,tegra30-ehci
@@ -66,6 +67,10 @@ properties:
- items:
- const: xlnx,zynq-usb-2.20a
- const: chipidea,usb2
+ - items:
+ - enum:
+ - nuvoton,npcm845-udc
+ - const: nuvoton,npcm750-udc

reg:
minItems: 1
@@ -388,6 +393,7 @@ allOf:
enum:
- chipidea,usb2
- lsi,zevio-usb
+ - nuvoton,npcm750-udc
- nvidia,tegra20-udc
- nvidia,tegra30-udc
- nvidia,tegra114-udc
--
2.33.0

2023-10-13 13:19:36

by Paul Menzel

[permalink] [raw]
Subject: Re: [PATCH v6 3/3] usb: chipidea: Add support for NPCM

Dear Tomer,


Thank you for your patch.

Am 13.10.23 um 01:00 schrieb Tomer Maimon:
> Add Nuvoton NPCM BMC SoCs support to USB ChipIdea driver.
> NPCM SoC include ChipIdea IP block that used for USB device controller

include*s*
“that *is* used” or just “… used for”

> mode.

Please add a line, how you tested this patch.

> Signed-off-by: Tomer Maimon <[email protected]>
> Acked-by: Peter Chen <[email protected]>
> ---
> drivers/usb/chipidea/Kconfig | 4 +
> drivers/usb/chipidea/Makefile | 1 +
> drivers/usb/chipidea/ci_hdrc_npcm.c | 114 ++++++++++++++++++++++++++++
> 3 files changed, 119 insertions(+)
> create mode 100644 drivers/usb/chipidea/ci_hdrc_npcm.c
>
> diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig
> index c815824a0b2d..bab45bc62361 100644
> --- a/drivers/usb/chipidea/Kconfig
> +++ b/drivers/usb/chipidea/Kconfig
> @@ -43,6 +43,10 @@ config USB_CHIPIDEA_MSM
> tristate "Enable MSM hsusb glue driver" if EXPERT
> default USB_CHIPIDEA
>
> +config USB_CHIPIDEA_NPCM
> + tristate "Enable NPCM hsusb glue driver" if EXPERT
> + default USB_CHIPIDEA
> +
> config USB_CHIPIDEA_IMX
> tristate "Enable i.MX USB glue driver" if EXPERT
> depends on OF
> diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
> index 71afeab97e83..718cb24603dd 100644
> --- a/drivers/usb/chipidea/Makefile
> +++ b/drivers/usb/chipidea/Makefile
> @@ -13,6 +13,7 @@ ci_hdrc-$(CONFIG_USB_OTG_FSM) += otg_fsm.o
>
> obj-$(CONFIG_USB_CHIPIDEA_GENERIC) += ci_hdrc_usb2.o
> obj-$(CONFIG_USB_CHIPIDEA_MSM) += ci_hdrc_msm.o
> +obj-$(CONFIG_USB_CHIPIDEA_NPCM) += ci_hdrc_npcm.o
> obj-$(CONFIG_USB_CHIPIDEA_PCI) += ci_hdrc_pci.o
> obj-$(CONFIG_USB_CHIPIDEA_IMX) += usbmisc_imx.o ci_hdrc_imx.o
> obj-$(CONFIG_USB_CHIPIDEA_TEGRA) += ci_hdrc_tegra.o
> diff --git a/drivers/usb/chipidea/ci_hdrc_npcm.c b/drivers/usb/chipidea/ci_hdrc_npcm.c
> new file mode 100644
> index 000000000000..37b64a3dbd96
> --- /dev/null
> +++ b/drivers/usb/chipidea/ci_hdrc_npcm.c
> @@ -0,0 +1,114 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2023 Nuvoton Technology corporation.
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/usb/chipidea.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/reset-controller.h>
> +#include <linux/of.h>
> +
> +#include "ci.h"
> +
> +struct npcm_udc_data {
> + struct platform_device *ci;
> + struct clk *core_clk;
> + struct ci_hdrc_platform_data pdata;
> +};
> +
> +static int npcm_udc_notify_event(struct ci_hdrc *ci, unsigned event)
> +{
> + struct device *dev = ci->dev->parent;
> +
> + switch (event) {
> + case CI_HDRC_CONTROLLER_RESET_EVENT:
> + /* clear all mode bits */
> + hw_write(ci, OP_USBMODE, 0xffffffff, 0x0);
> + break;
> + default:
> + dev_dbg(dev, "unknown ci_hdrc event\n");

Please print it out.

> + break;
> + }
> +
> + return 0;
> +}
> +
> +static int npcm_udc_probe(struct platform_device *pdev)
> +{
> + int ret;
> + struct npcm_udc_data *ci;
> + struct platform_device *plat_ci;
> + struct device *dev = &pdev->dev;
> +
> + ci = devm_kzalloc(&pdev->dev, sizeof(*ci), GFP_KERNEL);
> + if (!ci)
> + return -ENOMEM;
> + platform_set_drvdata(pdev, ci);
> +
> + ci->core_clk = devm_clk_get_optional(dev, NULL);
> + if (IS_ERR(ci->core_clk))
> + return PTR_ERR(ci->core_clk);
> +
> + ret = clk_prepare_enable(ci->core_clk);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to enable the clock: %d\n", ret);
> +
> + ci->pdata.name = dev_name(dev);
> + ci->pdata.capoffset = DEF_CAPOFFSET;
> + ci->pdata.flags = CI_HDRC_REQUIRES_ALIGNED_DMA |
> + CI_HDRC_FORCE_VBUS_ACTIVE_ALWAYS;
> + ci->pdata.phy_mode = USBPHY_INTERFACE_MODE_UTMI;
> + ci->pdata.notify_event = npcm_udc_notify_event;
> +
> + plat_ci = ci_hdrc_add_device(dev, pdev->resource, pdev->num_resources,
> + &ci->pdata);
> + if (IS_ERR(plat_ci)) {
> + ret = PTR_ERR(plat_ci);
> + dev_err(dev, "failed to register HDRC NPCM device: %d\n", ret);
> + goto clk_err;
> + }
> +
> + pm_runtime_no_callbacks(dev);
> + pm_runtime_enable(dev);
> +
> + return 0;
> +
> +clk_err:
> + clk_disable_unprepare(ci->core_clk);
> + return ret;
> +}
> +
> +static int npcm_udc_remove(struct platform_device *pdev)
> +{
> + struct npcm_udc_data *ci = platform_get_drvdata(pdev);
> +
> + pm_runtime_disable(&pdev->dev);
> + ci_hdrc_remove_device(ci->ci);
> + clk_disable_unprepare(ci->core_clk);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id npcm_udc_dt_match[] = {
> + { .compatible = "nuvoton,npcm750-udc", },
> + { .compatible = "nuvoton,npcm845-udc", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, npcm_udc_dt_match);
> +
> +static struct platform_driver npcm_udc_driver = {
> + .probe = npcm_udc_probe,
> + .remove = npcm_udc_remove,
> + .driver = {
> + .name = "npcm_udc",
> + .of_match_table = npcm_udc_dt_match,
> + },
> +};
> +
> +module_platform_driver(npcm_udc_driver);
> +
> +MODULE_DESCRIPTION("NPCM USB device controller driver");
> +MODULE_AUTHOR("Tomer Maimon <[email protected]>");

Should that address also be recorded as the patch author?

> +MODULE_LICENSE("GPL v2");


Kind regards,

Paul

2023-10-13 13:32:50

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v6 2/3] dt-bindings: usb: ci-hdrc-usb2: add npcm750 and npcm845 compatible

On 13/10/2023 01:00, Tomer Maimon wrote:
> Add a compatible string for Nuvoton BMC NPCM750 and Nuvoton BMC NPCM845.
>
> Signed-off-by: Tomer Maimon <[email protected]>
> ---

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time, thus I will skip this patch entirely till you follow
the process allowing the patch to be tested.

Please kindly resend and include all necessary To/Cc entries.

Best regards,
Krzysztof

2023-10-15 09:01:24

by Tomer Maimon

[permalink] [raw]
Subject: Re: [PATCH v6 2/3] dt-bindings: usb: ci-hdrc-usb2: add npcm750 and npcm845 compatible

Hi Krzysztof,

Thanks a lot for your comments, I really appreciate it!

I will run scripts/get_maintainers.pl to the latest kernel.

After running scripts/get_maintainers.pl do I need to send the
patchset with resend on V6 or do I need to send V7 with the new
maintainers list

Thanks,

Tomer

On Fri, 13 Oct 2023 at 16:32, Krzysztof Kozlowski <[email protected]> wrote:
>
> On 13/10/2023 01:00, Tomer Maimon wrote:
> > Add a compatible string for Nuvoton BMC NPCM750 and Nuvoton BMC NPCM845.
> >
> > Signed-off-by: Tomer Maimon <[email protected]>
> > ---
>
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC. It might happen, that command when run on an older
> kernel, gives you outdated entries. Therefore please be sure you base
> your patches on recent Linux kernel.
>
> You missed at least devicetree list (maybe more), so this won't be
> tested by automated tooling. Performing review on untested code might be
> a waste of time, thus I will skip this patch entirely till you follow
> the process allowing the patch to be tested.
>
> Please kindly resend and include all necessary To/Cc entries.
>
> Best regards,
> Krzysztof
>

2023-10-15 17:22:23

by Tomer Maimon

[permalink] [raw]
Subject: Re: [PATCH v6 3/3] usb: chipidea: Add support for NPCM

Hi Paul,

Thanks for your comments

On Fri, 13 Oct 2023 at 16:18, Paul Menzel <[email protected]> wrote:
>
> Dear Tomer,
>
>
> Thank you for your patch.
>
> Am 13.10.23 um 01:00 schrieb Tomer Maimon:
> > Add Nuvoton NPCM BMC SoCs support to USB ChipIdea driver.
> > NPCM SoC include ChipIdea IP block that used for USB device controller
>
> include*s*
> “that *is* used” or just “… used for”
Will do
>
> > mode.
>
> Please add a line, how you tested this patch.
I don't recall "how you tested the patch" done in the commit message,
I can add it in the cover letter
>
> > Signed-off-by: Tomer Maimon <[email protected]>
> > Acked-by: Peter Chen <[email protected]>
> > ---
> > drivers/usb/chipidea/Kconfig | 4 +
> > drivers/usb/chipidea/Makefile | 1 +
> > drivers/usb/chipidea/ci_hdrc_npcm.c | 114 ++++++++++++++++++++++++++++
> > 3 files changed, 119 insertions(+)
> > create mode 100644 drivers/usb/chipidea/ci_hdrc_npcm.c
> >
> > diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig
> > index c815824a0b2d..bab45bc62361 100644
> > --- a/drivers/usb/chipidea/Kconfig
> > +++ b/drivers/usb/chipidea/Kconfig
> > @@ -43,6 +43,10 @@ config USB_CHIPIDEA_MSM
> > tristate "Enable MSM hsusb glue driver" if EXPERT
> > default USB_CHIPIDEA
> >
> > +config USB_CHIPIDEA_NPCM
> > + tristate "Enable NPCM hsusb glue driver" if EXPERT
> > + default USB_CHIPIDEA
> > +
> > config USB_CHIPIDEA_IMX
> > tristate "Enable i.MX USB glue driver" if EXPERT
> > depends on OF
> > diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
> > index 71afeab97e83..718cb24603dd 100644
> > --- a/drivers/usb/chipidea/Makefile
> > +++ b/drivers/usb/chipidea/Makefile
> > @@ -13,6 +13,7 @@ ci_hdrc-$(CONFIG_USB_OTG_FSM) += otg_fsm.o
> >
> > obj-$(CONFIG_USB_CHIPIDEA_GENERIC) += ci_hdrc_usb2.o
> > obj-$(CONFIG_USB_CHIPIDEA_MSM) += ci_hdrc_msm.o
> > +obj-$(CONFIG_USB_CHIPIDEA_NPCM) += ci_hdrc_npcm.o
> > obj-$(CONFIG_USB_CHIPIDEA_PCI) += ci_hdrc_pci.o
> > obj-$(CONFIG_USB_CHIPIDEA_IMX) += usbmisc_imx.o ci_hdrc_imx.o
> > obj-$(CONFIG_USB_CHIPIDEA_TEGRA) += ci_hdrc_tegra.o
> > diff --git a/drivers/usb/chipidea/ci_hdrc_npcm.c b/drivers/usb/chipidea/ci_hdrc_npcm.c
> > new file mode 100644
> > index 000000000000..37b64a3dbd96
> > --- /dev/null
> > +++ b/drivers/usb/chipidea/ci_hdrc_npcm.c
> > @@ -0,0 +1,114 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (c) 2023 Nuvoton Technology corporation.
> > +
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/usb/chipidea.h>
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/reset-controller.h>
> > +#include <linux/of.h>
> > +
> > +#include "ci.h"
> > +
> > +struct npcm_udc_data {
> > + struct platform_device *ci;
> > + struct clk *core_clk;
> > + struct ci_hdrc_platform_data pdata;
> > +};
> > +
> > +static int npcm_udc_notify_event(struct ci_hdrc *ci, unsigned event)
> > +{
> > + struct device *dev = ci->dev->parent;
> > +
> > + switch (event) {
> > + case CI_HDRC_CONTROLLER_RESET_EVENT:
> > + /* clear all mode bits */
> > + hw_write(ci, OP_USBMODE, 0xffffffff, 0x0);
> > + break;
> > + default:
> > + dev_dbg(dev, "unknown ci_hdrc event\n");
>
> Please print it out.
Do you mean instead dev_dbg to use dev_info? in that case, each event
that didnt handled will cause a print.
>
> > + break;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int npcm_udc_probe(struct platform_device *pdev)
> > +{
> > + int ret;
> > + struct npcm_udc_data *ci;
> > + struct platform_device *plat_ci;
> > + struct device *dev = &pdev->dev;
> > +
> > + ci = devm_kzalloc(&pdev->dev, sizeof(*ci), GFP_KERNEL);
> > + if (!ci)
> > + return -ENOMEM;
> > + platform_set_drvdata(pdev, ci);
> > +
> > + ci->core_clk = devm_clk_get_optional(dev, NULL);
> > + if (IS_ERR(ci->core_clk))
> > + return PTR_ERR(ci->core_clk);
> > +
> > + ret = clk_prepare_enable(ci->core_clk);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to enable the clock: %d\n", ret);
> > +
> > + ci->pdata.name = dev_name(dev);
> > + ci->pdata.capoffset = DEF_CAPOFFSET;
> > + ci->pdata.flags = CI_HDRC_REQUIRES_ALIGNED_DMA |
> > + CI_HDRC_FORCE_VBUS_ACTIVE_ALWAYS;
> > + ci->pdata.phy_mode = USBPHY_INTERFACE_MODE_UTMI;
> > + ci->pdata.notify_event = npcm_udc_notify_event;
> > +
> > + plat_ci = ci_hdrc_add_device(dev, pdev->resource, pdev->num_resources,
> > + &ci->pdata);
> > + if (IS_ERR(plat_ci)) {
> > + ret = PTR_ERR(plat_ci);
> > + dev_err(dev, "failed to register HDRC NPCM device: %d\n", ret);
> > + goto clk_err;
> > + }
> > +
> > + pm_runtime_no_callbacks(dev);
> > + pm_runtime_enable(dev);
> > +
> > + return 0;
> > +
> > +clk_err:
> > + clk_disable_unprepare(ci->core_clk);
> > + return ret;
> > +}
> > +
> > +static int npcm_udc_remove(struct platform_device *pdev)
> > +{
> > + struct npcm_udc_data *ci = platform_get_drvdata(pdev);
> > +
> > + pm_runtime_disable(&pdev->dev);
> > + ci_hdrc_remove_device(ci->ci);
> > + clk_disable_unprepare(ci->core_clk);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id npcm_udc_dt_match[] = {
> > + { .compatible = "nuvoton,npcm750-udc", },
> > + { .compatible = "nuvoton,npcm845-udc", },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, npcm_udc_dt_match);
> > +
> > +static struct platform_driver npcm_udc_driver = {
> > + .probe = npcm_udc_probe,
> > + .remove = npcm_udc_remove,
> > + .driver = {
> > + .name = "npcm_udc",
> > + .of_match_table = npcm_udc_dt_match,
> > + },
> > +};
> > +
> > +module_platform_driver(npcm_udc_driver);
> > +
> > +MODULE_DESCRIPTION("NPCM USB device controller driver");
> > +MODULE_AUTHOR("Tomer Maimon <[email protected]>");
>
> Should that address also be recorded as the patch author?
I prefer to use this e-mail since this is my main e-mail.
>
> > +MODULE_LICENSE("GPL v2");
>
>
> Kind regards,
>
> Paul

Best regards,

Tomer

2023-10-15 19:19:14

by Paul Menzel

[permalink] [raw]
Subject: Re: [PATCH v6 3/3] usb: chipidea: Add support for NPCM

Dear Tomer,


Am 15.10.23 um 19:21 schrieb Tomer Maimon:

> On Fri, 13 Oct 2023 at 16:18, Paul Menzel <[email protected]> wrote:

>> Am 13.10.23 um 01:00 schrieb Tomer Maimon:
>>> Add Nuvoton NPCM BMC SoCs support to USB ChipIdea driver.
>>> NPCM SoC include ChipIdea IP block that used for USB device controller
>>
>> include*s*
>> “that *is* used” or just “… used for”
> Will do
>>
>>> mode.
>>
>> Please add a line, how you tested this patch.
> I don't recall "how you tested the patch" done in the commit message,
> I can add it in the cover letter

In my opinion, adding a separate paragraph to the commit message would
be best.

>>> Signed-off-by: Tomer Maimon <[email protected]>
>>> Acked-by: Peter Chen <[email protected]>
>>> ---
>>> drivers/usb/chipidea/Kconfig | 4 +
>>> drivers/usb/chipidea/Makefile | 1 +
>>> drivers/usb/chipidea/ci_hdrc_npcm.c | 114 ++++++++++++++++++++++++++++
>>> 3 files changed, 119 insertions(+)
>>> create mode 100644 drivers/usb/chipidea/ci_hdrc_npcm.c
>>>
>>> diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig
>>> index c815824a0b2d..bab45bc62361 100644
>>> --- a/drivers/usb/chipidea/Kconfig
>>> +++ b/drivers/usb/chipidea/Kconfig
>>> @@ -43,6 +43,10 @@ config USB_CHIPIDEA_MSM
>>> tristate "Enable MSM hsusb glue driver" if EXPERT
>>> default USB_CHIPIDEA
>>>
>>> +config USB_CHIPIDEA_NPCM
>>> + tristate "Enable NPCM hsusb glue driver" if EXPERT
>>> + default USB_CHIPIDEA
>>> +
>>> config USB_CHIPIDEA_IMX
>>> tristate "Enable i.MX USB glue driver" if EXPERT
>>> depends on OF
>>> diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
>>> index 71afeab97e83..718cb24603dd 100644
>>> --- a/drivers/usb/chipidea/Makefile
>>> +++ b/drivers/usb/chipidea/Makefile
>>> @@ -13,6 +13,7 @@ ci_hdrc-$(CONFIG_USB_OTG_FSM) += otg_fsm.o
>>>
>>> obj-$(CONFIG_USB_CHIPIDEA_GENERIC) += ci_hdrc_usb2.o
>>> obj-$(CONFIG_USB_CHIPIDEA_MSM) += ci_hdrc_msm.o
>>> +obj-$(CONFIG_USB_CHIPIDEA_NPCM) += ci_hdrc_npcm.o
>>> obj-$(CONFIG_USB_CHIPIDEA_PCI) += ci_hdrc_pci.o
>>> obj-$(CONFIG_USB_CHIPIDEA_IMX) += usbmisc_imx.o ci_hdrc_imx.o
>>> obj-$(CONFIG_USB_CHIPIDEA_TEGRA) += ci_hdrc_tegra.o
>>> diff --git a/drivers/usb/chipidea/ci_hdrc_npcm.c b/drivers/usb/chipidea/ci_hdrc_npcm.c
>>> new file mode 100644
>>> index 000000000000..37b64a3dbd96
>>> --- /dev/null
>>> +++ b/drivers/usb/chipidea/ci_hdrc_npcm.c
>>> @@ -0,0 +1,114 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +// Copyright (c) 2023 Nuvoton Technology corporation.
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/pm_runtime.h>
>>> +#include <linux/usb/chipidea.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/io.h>
>>> +#include <linux/reset-controller.h>
>>> +#include <linux/of.h>
>>> +
>>> +#include "ci.h"
>>> +
>>> +struct npcm_udc_data {
>>> + struct platform_device *ci;
>>> + struct clk *core_clk;
>>> + struct ci_hdrc_platform_data pdata;
>>> +};
>>> +
>>> +static int npcm_udc_notify_event(struct ci_hdrc *ci, unsigned event)
>>> +{
>>> + struct device *dev = ci->dev->parent;
>>> +
>>> + switch (event) {
>>> + case CI_HDRC_CONTROLLER_RESET_EVENT:
>>> + /* clear all mode bits */
>>> + hw_write(ci, OP_USBMODE, 0xffffffff, 0x0);
>>> + break;
>>> + default:
>>> + dev_dbg(dev, "unknown ci_hdrc event\n");
>>
>> Please print it out.
> Do you mean instead dev_dbg to use dev_info? in that case, each event
> that didnt handled will cause a print.

Sorry for not being clear. I meant print out `event`.

>>> + break;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int npcm_udc_probe(struct platform_device *pdev)
>>> +{
>>> + int ret;
>>> + struct npcm_udc_data *ci;
>>> + struct platform_device *plat_ci;
>>> + struct device *dev = &pdev->dev;
>>> +
>>> + ci = devm_kzalloc(&pdev->dev, sizeof(*ci), GFP_KERNEL);
>>> + if (!ci)
>>> + return -ENOMEM;
>>> + platform_set_drvdata(pdev, ci);
>>> +
>>> + ci->core_clk = devm_clk_get_optional(dev, NULL);
>>> + if (IS_ERR(ci->core_clk))
>>> + return PTR_ERR(ci->core_clk);
>>> +
>>> + ret = clk_prepare_enable(ci->core_clk);
>>> + if (ret)
>>> + return dev_err_probe(dev, ret, "failed to enable the clock: %d\n", ret);
>>> +
>>> + ci->pdata.name = dev_name(dev);
>>> + ci->pdata.capoffset = DEF_CAPOFFSET;
>>> + ci->pdata.flags = CI_HDRC_REQUIRES_ALIGNED_DMA |
>>> + CI_HDRC_FORCE_VBUS_ACTIVE_ALWAYS;
>>> + ci->pdata.phy_mode = USBPHY_INTERFACE_MODE_UTMI;
>>> + ci->pdata.notify_event = npcm_udc_notify_event;
>>> +
>>> + plat_ci = ci_hdrc_add_device(dev, pdev->resource, pdev->num_resources,
>>> + &ci->pdata);
>>> + if (IS_ERR(plat_ci)) {
>>> + ret = PTR_ERR(plat_ci);
>>> + dev_err(dev, "failed to register HDRC NPCM device: %d\n", ret);
>>> + goto clk_err;
>>> + }
>>> +
>>> + pm_runtime_no_callbacks(dev);
>>> + pm_runtime_enable(dev);
>>> +
>>> + return 0;
>>> +
>>> +clk_err:
>>> + clk_disable_unprepare(ci->core_clk);
>>> + return ret;
>>> +}
>>> +
>>> +static int npcm_udc_remove(struct platform_device *pdev)
>>> +{
>>> + struct npcm_udc_data *ci = platform_get_drvdata(pdev);
>>> +
>>> + pm_runtime_disable(&pdev->dev);
>>> + ci_hdrc_remove_device(ci->ci);
>>> + clk_disable_unprepare(ci->core_clk);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct of_device_id npcm_udc_dt_match[] = {
>>> + { .compatible = "nuvoton,npcm750-udc", },
>>> + { .compatible = "nuvoton,npcm845-udc", },
>>> + { }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, npcm_udc_dt_match);
>>> +
>>> +static struct platform_driver npcm_udc_driver = {
>>> + .probe = npcm_udc_probe,
>>> + .remove = npcm_udc_remove,
>>> + .driver = {
>>> + .name = "npcm_udc",
>>> + .of_match_table = npcm_udc_dt_match,
>>> + },
>>> +};
>>> +
>>> +module_platform_driver(npcm_udc_driver);
>>> +
>>> +MODULE_DESCRIPTION("NPCM USB device controller driver");
>>> +MODULE_AUTHOR("Tomer Maimon <[email protected]>");
>>
>> Should that address also be recorded as the patch author?
> I prefer to use this e-mail since this is my main e-mail.

Alright, if it’s fine with your employer that’s great.

>>> +MODULE_LICENSE("GPL v2");


Kind regards,

Paul