[PATCH v1 0/2] usb: ChipIdea: add Nuvoton NPCM UDC support

[PATCH v1 0/2] usb: ChipIdea: add Nuvoton NPCM UDC support

[Tomer Maimon]

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.

The NPCM UDC is using the CI_HDRC_FORCE_VBUS_ACTIVE_ALWAYS flag
therefor, this patch set should apply after  
usb: chipidea: add CI_HDRC_FORCE_VBUS_ACTIVE_ALWAYS flag commit will
apply.

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

The NPCM UDC were tested on NPCM845 evaluation board.

Tomer Maimon (2):
  dt-binding: usb: ci-hdrc-usb2: document Nuvoton NPCM supprt
  usb: chipidea: Add support for NPCM

 .../devicetree/bindings/usb/ci-hdrc-usb2.yaml |  16 +++
 drivers/usb/chipidea/Kconfig                  |   4 +
 drivers/usb/chipidea/Makefile                 |   1 +
 drivers/usb/chipidea/ci_hdrc_npcm.c           | 126 ++++++++++++++++++
 4 files changed, 147 insertions(+)
 create mode 100644 drivers/usb/chipidea/ci_hdrc_npcm.c

-- 
2.33.0

[PATCH v1 1/2] dt-binding: usb: ci-hdrc-usb2: document Nuvoton NPCM supprt

[Tomer Maimon]

Nuvoton NPCM BMC SoCs use ChipIdea silicon IP for the USB device controller.

Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
---
 .../devicetree/bindings/usb/ci-hdrc-usb2.yaml    | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
index 1394557517b1..9de4dfe004d1 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,npcm-udc
           - nvidia,tegra20-ehci
           - nvidia,tegra20-udc
           - nvidia,tegra30-ehci
@@ -325,6 +326,20 @@ properties:
     type: boolean
     deprecated: true
 
+  nuvoton,sysgcr:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    items:
+      - items:
+          - description: phandle to syscon that configures usb phy mux.
+          - description: offset of usb phy mux selection.
+          - description: mask usb phy mux selection.
+          - description: value usb phy mux selection.
+    description:
+      A phandle to syscon with three arguments that configure usb phy mux.
+      The argument one is the offset of usb phy mux selection, the argument two
+      is the mask usb phy mux selection, the argument three is the mask usb phy
+      mux selection.
+
   port:
     description:
       Any connector to the data bus of this controller should be modelled
@@ -388,6 +403,7 @@ allOf:
             enum:
               - chipidea,usb2
               - lsi,zevio-usb
+              - nuvoton,npcm-udc
               - nvidia,tegra20-udc
               - nvidia,tegra30-udc
               - nvidia,tegra114-udc
-- 
2.33.0

[PATCH v1 2/2] usb: chipidea: Add support for NPCM

[Tomer Maimon]

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 <tmaimon77@gmail.com>
---
 drivers/usb/chipidea/Kconfig        |   4 +
 drivers/usb/chipidea/Makefile       |   1 +
 drivers/usb/chipidea/ci_hdrc_npcm.c | 126 ++++++++++++++++++++++++++++
 3 files changed, 131 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..8214a4d1e418
--- /dev/null
+++ b/drivers/usb/chipidea/ci_hdrc_npcm.c
@@ -0,0 +1,126 @@
+// 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/mfd/syscon.h>
+#include <linux/regmap.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;
+	unsigned int args[3];
+	struct regmap *gcr_regmap;
+	struct npcm_udc_data *ci;
+	struct platform_device *plat_ci;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+
+	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) {
+		dev_err(dev, "failed to enable the clock: %d\n", ret);
+		return 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;
+
+	gcr_regmap = syscon_regmap_lookup_by_phandle_args(np, "nuvoton,sysgcr",
+							  3, args);
+	if (!IS_ERR(gcr_regmap))
+		regmap_update_bits(gcr_regmap, args[0], args[1], args[2]);
+
+	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,npcm-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 <tomer.maimon@nuvoton.com>");
+MODULE_ALIAS("platform:npcm-udc");
+MODULE_LICENSE("GPL v2");
-- 
2.33.0

Re: [PATCH v1 1/2] dt-binding: usb: ci-hdrc-usb2: document Nuvoton NPCM supprt

[Krzysztof Kozlowski]

On 18/09/2023 18:59, Tomer Maimon wrote:
> Nuvoton NPCM BMC SoCs use ChipIdea silicon IP for the USB device controller.
> 
> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. It's dt-bindings.


> ---
>  .../devicetree/bindings/usb/ci-hdrc-usb2.yaml    | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
> index 1394557517b1..9de4dfe004d1 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,npcm-udc

You need SoC specific compatible.

>            - nvidia,tegra20-ehci
>            - nvidia,tegra20-udc
>            - nvidia,tegra30-ehci
> @@ -325,6 +326,20 @@ properties:
>      type: boolean
>      deprecated: true
>  
> +  nuvoton,sysgcr:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      - items:
> +          - description: phandle to syscon that configures usb phy mux.
> +          - description: offset of usb phy mux selection.
> +          - description: mask usb phy mux selection.
> +          - description: value usb phy mux selection.
> +    description:
> +      A phandle to syscon with three arguments that configure usb phy mux.
> +      The argument one is the offset of usb phy mux selection, the argument two
> +      is the mask usb phy mux selection, the argument three is the mask usb phy
> +      mux selection.

Sorry, you miss phy driver. Don't use syscon instead of proper hardware
devices.


Best regards,
Krzysztof

Re: [PATCH v1 1/2] dt-binding: usb: ci-hdrc-usb2: document Nuvoton NPCM supprt

[Tomer Maimon]

Hi Krzysztof

Thanks for you comment

On Mon, 18 Sept 2023 at 23:18, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 18/09/2023 18:59, Tomer Maimon wrote:
> > Nuvoton NPCM BMC SoCs use ChipIdea silicon IP for the USB device controller.
> >
> > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
>
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching. It's dt-bindings.
O.K. Thanks
>
>
> > ---
> >  .../devicetree/bindings/usb/ci-hdrc-usb2.yaml    | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
> > index 1394557517b1..9de4dfe004d1 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,npcm-udc
>
> You need SoC specific compatible.
O.K. Thanks,
>
> >            - nvidia,tegra20-ehci
> >            - nvidia,tegra20-udc
> >            - nvidia,tegra30-ehci
> > @@ -325,6 +326,20 @@ properties:
> >      type: boolean
> >      deprecated: true
> >
> > +  nuvoton,sysgcr:
> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +    items:
> > +      - items:
> > +          - description: phandle to syscon that configures usb phy mux.
> > +          - description: offset of usb phy mux selection.
> > +          - description: mask usb phy mux selection.
> > +          - description: value usb phy mux selection.
> > +    description:
> > +      A phandle to syscon with three arguments that configure usb phy mux.
> > +      The argument one is the offset of usb phy mux selection, the argument two
> > +      is the mask usb phy mux selection, the argument three is the mask usb phy
> > +      mux selection.
>
> Sorry, you miss phy driver. Don't use syscon instead of proper hardware
> devices.
Sorry the role of nuvoton,sysgcr property is to handle a mux between
the different devices and not the handle the phy itself, handle the
mux done in the GCR.
Should we move the nuvoton,sysgcr description to another place in the
ci-hdrc-usb2.yaml
or
Should we use a different driver to handle the mux and call it from
the ci-hdrc-npcm driver, If yes which driver should we use?
>
>
> Best regards,
> Krzysztof
>

Best regards,

Tomer

Re: [PATCH v1 2/2] usb: chipidea: Add support for NPCM

[kernel test robot]

Hi Tomer,

kernel test robot noticed the following build errors:

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on usb/usb-next usb/usb-linus linus/master v6.6-rc2 next-20230918]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Tomer-Maimon/dt-binding-usb-ci-hdrc-usb2-document-Nuvoton-NPCM-supprt/20230919-010147
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/20230918165958.2659-3-tmaimon77%40gmail.com
patch subject: [PATCH v1 2/2] usb: chipidea: Add support for NPCM
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20230919/202309191545.dZXS2Y2p-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230919/202309191545.dZXS2Y2p-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309191545.dZXS2Y2p-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/usb/chipidea/ci_hdrc_npcm.c: In function 'npcm_udc_probe':
>> drivers/usb/chipidea/ci_hdrc_npcm.c:68:17: error: 'CI_HDRC_FORCE_VBUS_ACTIVE_ALWAYS' undeclared (first use in this function)
      68 |                 CI_HDRC_FORCE_VBUS_ACTIVE_ALWAYS;
         |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/usb/chipidea/ci_hdrc_npcm.c:68:17: note: each undeclared identifier is reported only once for each function it appears in


vim +/CI_HDRC_FORCE_VBUS_ACTIVE_ALWAYS +68 drivers/usb/chipidea/ci_hdrc_npcm.c

    39	
    40	static int npcm_udc_probe(struct platform_device *pdev)
    41	{
    42		int ret;
    43		unsigned int args[3];
    44		struct regmap *gcr_regmap;
    45		struct npcm_udc_data *ci;
    46		struct platform_device *plat_ci;
    47		struct device *dev = &pdev->dev;
    48		struct device_node *np = dev->of_node;
    49	
    50		ci = devm_kzalloc(&pdev->dev, sizeof(*ci), GFP_KERNEL);
    51		if (!ci)
    52			return -ENOMEM;
    53		platform_set_drvdata(pdev, ci);
    54	
    55		ci->core_clk = devm_clk_get_optional(dev, NULL);
    56		if (IS_ERR(ci->core_clk))
    57			return PTR_ERR(ci->core_clk);
    58	
    59		ret = clk_prepare_enable(ci->core_clk);
    60		if (ret) {
    61			dev_err(dev, "failed to enable the clock: %d\n", ret);
    62			return ret;
    63		}
    64	
    65		ci->pdata.name = dev_name(dev);
    66		ci->pdata.capoffset = DEF_CAPOFFSET;
    67		ci->pdata.flags	= CI_HDRC_REQUIRES_ALIGNED_DMA |
  > 68			CI_HDRC_FORCE_VBUS_ACTIVE_ALWAYS;
    69		ci->pdata.phy_mode = USBPHY_INTERFACE_MODE_UTMI;
    70		ci->pdata.notify_event = npcm_udc_notify_event;
    71	
    72		gcr_regmap = syscon_regmap_lookup_by_phandle_args(np, "nuvoton,sysgcr",
    73								  3, args);
    74		if (!IS_ERR(gcr_regmap))
    75			regmap_update_bits(gcr_regmap, args[0], args[1], args[2]);
    76	
    77		plat_ci = ci_hdrc_add_device(dev, pdev->resource, pdev->num_resources,
    78					     &ci->pdata);
    79		if (IS_ERR(plat_ci)) {
    80			ret = PTR_ERR(plat_ci);
    81			dev_err(dev, "failed to register HDRC NPCM device: %d\n", ret);
    82			goto clk_err;
    83		}
    84	
    85		pm_runtime_no_callbacks(dev);
    86		pm_runtime_enable(dev);
    87	
    88		return 0;
    89	
    90	clk_err:
    91		clk_disable_unprepare(ci->core_clk);
    92		return ret;
    93	}
    94	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 1/2] dt-binding: usb: ci-hdrc-usb2: document Nuvoton NPCM supprt

[Krzysztof Kozlowski]

On 19/09/2023 07:14, Tomer Maimon wrote:
>>>            - nvidia,tegra20-ehci
>>>            - nvidia,tegra20-udc
>>>            - nvidia,tegra30-ehci
>>> @@ -325,6 +326,20 @@ properties:
>>>      type: boolean
>>>      deprecated: true
>>>
>>> +  nuvoton,sysgcr:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>>> +    items:
>>> +      - items:
>>> +          - description: phandle to syscon that configures usb phy mux.
>>> +          - description: offset of usb phy mux selection.
>>> +          - description: mask usb phy mux selection.
>>> +          - description: value usb phy mux selection.
>>> +    description:
>>> +      A phandle to syscon with three arguments that configure usb phy mux.
>>> +      The argument one is the offset of usb phy mux selection, the argument two
>>> +      is the mask usb phy mux selection, the argument three is the mask usb phy
>>> +      mux selection.
>>
>> Sorry, you miss phy driver. Don't use syscon instead of proper hardware
>> devices.
> Sorry the role of nuvoton,sysgcr property is to handle a mux between
> the different devices and not the handle the phy itself, handle the
> mux done in the GCR.
> Should we move the nuvoton,sysgcr description to another place in the
> ci-hdrc-usb2.yaml
> or
> Should we use a different driver to handle the mux and call it from
> the ci-hdrc-npcm driver, If yes which driver should we use?

What is an "usb phy mux"?

Best regards,
Krzysztof

Re: [PATCH v1 1/2] dt-binding: usb: ci-hdrc-usb2: document Nuvoton NPCM supprt

[Tomer Maimon]

On Tue, 19 Sept 2023 at 15:39, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 19/09/2023 07:14, Tomer Maimon wrote:
> >>>            - nvidia,tegra20-ehci
> >>>            - nvidia,tegra20-udc
> >>>            - nvidia,tegra30-ehci
> >>> @@ -325,6 +326,20 @@ properties:
> >>>      type: boolean
> >>>      deprecated: true
> >>>
> >>> +  nuvoton,sysgcr:
> >>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> >>> +    items:
> >>> +      - items:
> >>> +          - description: phandle to syscon that configures usb phy mux.
> >>> +          - description: offset of usb phy mux selection.
> >>> +          - description: mask usb phy mux selection.
> >>> +          - description: value usb phy mux selection.
> >>> +    description:
> >>> +      A phandle to syscon with three arguments that configure usb phy mux.
> >>> +      The argument one is the offset of usb phy mux selection, the argument two
> >>> +      is the mask usb phy mux selection, the argument three is the mask usb phy
> >>> +      mux selection.
> >>
> >> Sorry, you miss phy driver. Don't use syscon instead of proper hardware
> >> devices.
> > Sorry the role of nuvoton,sysgcr property is to handle a mux between
> > the different devices and not the handle the phy itself, handle the
> > mux done in the GCR.
> > Should we move the nuvoton,sysgcr description to another place in the
> > ci-hdrc-usb2.yaml
> > or
> > Should we use a different driver to handle the mux and call it from
> > the ci-hdrc-npcm driver, If yes which driver should we use?
>
> What is an "usb phy mux"?
We have USB phy that could be connected to USB host (different driver)
or it can be connected to the UDC driver(ChipIdea)
> Best regards,
> Krzysztof
>

Best regards,

Tomer

Re: [PATCH v1 1/2] dt-binding: usb: ci-hdrc-usb2: document Nuvoton NPCM supprt

[Rob Herring]

On Tue, Sep 19, 2023 at 04:31:56PM +0300, Tomer Maimon wrote:
> On Tue, 19 Sept 2023 at 15:39, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> >
> > On 19/09/2023 07:14, Tomer Maimon wrote:
> > >>>            - nvidia,tegra20-ehci
> > >>>            - nvidia,tegra20-udc
> > >>>            - nvidia,tegra30-ehci
> > >>> @@ -325,6 +326,20 @@ properties:
> > >>>      type: boolean
> > >>>      deprecated: true
> > >>>
> > >>> +  nuvoton,sysgcr:
> > >>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > >>> +    items:
> > >>> +      - items:
> > >>> +          - description: phandle to syscon that configures usb phy mux.
> > >>> +          - description: offset of usb phy mux selection.
> > >>> +          - description: mask usb phy mux selection.
> > >>> +          - description: value usb phy mux selection.
> > >>> +    description:
> > >>> +      A phandle to syscon with three arguments that configure usb phy mux.
> > >>> +      The argument one is the offset of usb phy mux selection, the argument two
> > >>> +      is the mask usb phy mux selection, the argument three is the mask usb phy
> > >>> +      mux selection.
> > >>
> > >> Sorry, you miss phy driver. Don't use syscon instead of proper hardware
> > >> devices.
> > > Sorry the role of nuvoton,sysgcr property is to handle a mux between
> > > the different devices and not the handle the phy itself, handle the
> > > mux done in the GCR.
> > > Should we move the nuvoton,sysgcr description to another place in the
> > > ci-hdrc-usb2.yaml
> > > or
> > > Should we use a different driver to handle the mux and call it from
> > > the ci-hdrc-npcm driver, If yes which driver should we use?
> >
> > What is an "usb phy mux"?
> We have USB phy that could be connected to USB host (different driver)
> or it can be connected to the UDC driver(ChipIdea)

Isn't that just role switching? There is a driver framework for that in 
drivers/usb/roles/. Though it doesn't seem widely used yet.

Rob

Re: [PATCH v1 1/2] dt-binding: usb: ci-hdrc-usb2: document Nuvoton NPCM supprt

[Tomer Maimon]

Thanks Rob,

I will check drivers/usb/roles

On Tue, 19 Sept 2023 at 19:28, Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Sep 19, 2023 at 04:31:56PM +0300, Tomer Maimon wrote:
> > On Tue, 19 Sept 2023 at 15:39, Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> > >
> > > On 19/09/2023 07:14, Tomer Maimon wrote:
> > > >>>            - nvidia,tegra20-ehci
> > > >>>            - nvidia,tegra20-udc
> > > >>>            - nvidia,tegra30-ehci
> > > >>> @@ -325,6 +326,20 @@ properties:
> > > >>>      type: boolean
> > > >>>      deprecated: true
> > > >>>
> > > >>> +  nuvoton,sysgcr:
> > > >>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > > >>> +    items:
> > > >>> +      - items:
> > > >>> +          - description: phandle to syscon that configures usb phy mux.
> > > >>> +          - description: offset of usb phy mux selection.
> > > >>> +          - description: mask usb phy mux selection.
> > > >>> +          - description: value usb phy mux selection.
> > > >>> +    description:
> > > >>> +      A phandle to syscon with three arguments that configure usb phy mux.
> > > >>> +      The argument one is the offset of usb phy mux selection, the argument two
> > > >>> +      is the mask usb phy mux selection, the argument three is the mask usb phy
> > > >>> +      mux selection.
> > > >>
> > > >> Sorry, you miss phy driver. Don't use syscon instead of proper hardware
> > > >> devices.
> > > > Sorry the role of nuvoton,sysgcr property is to handle a mux between
> > > > the different devices and not the handle the phy itself, handle the
> > > > mux done in the GCR.
> > > > Should we move the nuvoton,sysgcr description to another place in the
> > > > ci-hdrc-usb2.yaml
> > > > or
> > > > Should we use a different driver to handle the mux and call it from
> > > > the ci-hdrc-npcm driver, If yes which driver should we use?
> > >
> > > What is an "usb phy mux"?
> > We have USB phy that could be connected to USB host (different driver)
> > or it can be connected to the UDC driver(ChipIdea)
>
> Isn't that just role switching? There is a driver framework for that in
> drivers/usb/roles/. Though it doesn't seem widely used yet.
>
> Rob

Re: [PATCH v1 1/2] dt-binding: usb: ci-hdrc-usb2: document Nuvoton NPCM supprt

[Tomer Maimon]

Hi Rob,

Again thanks for your suggestion I took a look at the
drivers/usb/roles and I am not sure it answers NPCM ChipIdae case.
For example, in the NPCM845 we have ten UDC ChipIdea modules (UDC0-9).
Only UDC8 and UDC9 are muxed with USB host0 and USB host1.
NPCM UDC and NPCM USB host are different HW modules therefore different drivers.

The ChipIdea driver uses its own internal USB role function to switch
between ChipIdea UDC and ChipIdea USB host and we can't replace it
with a unique NPCM USB role function also we need to set the mux only
at the probe stage.

This is why I have added nuvoton,sysgcr property to the NPCM ChipIdea driver.

With the above do you think I should do it differently?

On Wed, 20 Sept 2023 at 12:11, Tomer Maimon <tmaimon77@gmail.com> wrote:
>
> Thanks Rob,
>
> I will check drivers/usb/roles
>
> On Tue, 19 Sept 2023 at 19:28, Rob Herring <robh@kernel.org> wrote:
> >
> > On Tue, Sep 19, 2023 at 04:31:56PM +0300, Tomer Maimon wrote:
> > > On Tue, 19 Sept 2023 at 15:39, Krzysztof Kozlowski
> > > <krzysztof.kozlowski@linaro.org> wrote:
> > > >
> > > > On 19/09/2023 07:14, Tomer Maimon wrote:
> > > > >>>            - nvidia,tegra20-ehci
> > > > >>>            - nvidia,tegra20-udc
> > > > >>>            - nvidia,tegra30-ehci
> > > > >>> @@ -325,6 +326,20 @@ properties:
> > > > >>>      type: boolean
> > > > >>>      deprecated: true
> > > > >>>
> > > > >>> +  nuvoton,sysgcr:
> > > > >>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > > > >>> +    items:
> > > > >>> +      - items:
> > > > >>> +          - description: phandle to syscon that configures usb phy mux.
> > > > >>> +          - description: offset of usb phy mux selection.
> > > > >>> +          - description: mask usb phy mux selection.
> > > > >>> +          - description: value usb phy mux selection.
> > > > >>> +    description:
> > > > >>> +      A phandle to syscon with three arguments that configure usb phy mux.
> > > > >>> +      The argument one is the offset of usb phy mux selection, the argument two
> > > > >>> +      is the mask usb phy mux selection, the argument three is the mask usb phy
> > > > >>> +      mux selection.
> > > > >>
> > > > >> Sorry, you miss phy driver. Don't use syscon instead of proper hardware
> > > > >> devices.
> > > > > Sorry the role of nuvoton,sysgcr property is to handle a mux between
> > > > > the different devices and not the handle the phy itself, handle the
> > > > > mux done in the GCR.
> > > > > Should we move the nuvoton,sysgcr description to another place in the
> > > > > ci-hdrc-usb2.yaml
> > > > > or
> > > > > Should we use a different driver to handle the mux and call it from
> > > > > the ci-hdrc-npcm driver, If yes which driver should we use?
> > > >
> > > > What is an "usb phy mux"?
> > > We have USB phy that could be connected to USB host (different driver)
> > > or it can be connected to the UDC driver(ChipIdea)
> >
> > Isn't that just role switching? There is a driver framework for that in
> > drivers/usb/roles/. Though it doesn't seem widely used yet.
> >
> > Rob

Best regards,

Tomer

Re: [PATCH v1 1/2] dt-binding: usb: ci-hdrc-usb2: document Nuvoton NPCM supprt

[Krzysztof Kozlowski]

On 21/09/2023 15:17, Tomer Maimon wrote:
> Hi Rob,
> 
> Again thanks for your suggestion I took a look at the
> drivers/usb/roles and I am not sure it answers NPCM ChipIdae case.
> For example, in the NPCM845 we have ten UDC ChipIdea modules (UDC0-9).
> Only UDC8 and UDC9 are muxed with USB host0 and USB host1.
> NPCM UDC and NPCM USB host are different HW modules therefore different drivers.
> 
> The ChipIdea driver uses its own internal USB role function to switch
> between ChipIdea UDC and ChipIdea USB host and we can't replace it
> with a unique NPCM USB role function also we need to set the mux only
> at the probe stage.
> 
> This is why I have added nuvoton,sysgcr property to the NPCM ChipIdea driver.
> 
> With the above do you think I should do it differently?

Your entire rationale above explains drivers, so it is not proper
justification for bindings. How your probe stage has anything to do with
bindings? If drivers do not work or miss something, change them.

Best regards,
Krzysztof