| Message ID | 20230918165958.2659-2-tmaimon77@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | usb: ChipIdea: add Nuvoton NPCM UDC support | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 1 warnings, 34 lines checked |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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
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
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
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
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
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
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
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
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
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(+)