2023-09-19 00:37:22

by Krzysztof Kozlowski

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

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 <[email protected]>

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


2023-09-19 06:27:39

by Tomer Maimon

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

Hi Krzysztof

Thanks for you comment

On Mon, 18 Sept 2023 at 23:18, Krzysztof Kozlowski
<[email protected]> 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 <[email protected]>
>
> 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

2023-09-19 20:52:19

by Krzysztof Kozlowski

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

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

2023-09-20 00:26:18

by Tomer Maimon

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

On Tue, 19 Sept 2023 at 15:39, Krzysztof Kozlowski
<[email protected]> 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