| Message ID | 20230929064852.16642-2-piyush.mehta@amd.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | usb: phy: Add platform driver support for ULPI phys | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
Hey, On Fri, Sep 29, 2023 at 12:18:50PM +0530, Piyush Mehta wrote: > Create an ulpi-phy binding to read and write PHY registers with explicit > control of the address and data using the usb.VIEWPORT register. > > Signed-off-by: Piyush Mehta <piyush.mehta@amd.com> > --- > This binding patch was created to support generic platforms. This binding > will be modified in accordance with patch [3/3] procedures. One of the > approch may be Create a zynq phy platform driver in "driver/usb/phy" with > driver source "phy-ulpi-zynq-usb.c" and then the binding will be particular > to the Xilinx/AMD zynq platform. I don't understand what the drivers have to do with describing the hardware here. You have a zynq platform with this phy, so the compatible should reflect that. Whether you create some software that supports generic platforms is mostly a separate question IMO. > This binding was built with the Zynq hardware design example in consideration > of as a generic platform. The viewport provide access the Chipidea controller > to interface with the ULPI PHY. > --- > .../devicetree/bindings/usb/ulpi-phy.yaml | 48 +++++++++++++++++++ > 1 file changed, 48 insertions(+) > create mode 100644 Documentation/devicetree/bindings/usb/ulpi-phy.yaml > > diff --git a/Documentation/devicetree/bindings/usb/ulpi-phy.yaml b/Documentation/devicetree/bindings/usb/ulpi-phy.yaml > new file mode 100644 > index 000000000000..490b2f610129 > --- /dev/null > +++ b/Documentation/devicetree/bindings/usb/ulpi-phy.yaml > @@ -0,0 +1,48 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/usb/ulpi-phy.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: ULPI PHY- Generic platform > + > +maintainers: > + - Piyush Mehta <piyush.mehta@amd.com> > + > +properties: > + compatible: > + const: ulpi-phy Please add a device specific compatible here. > + > + reg: > + maxItems: 1 > + > + '#phy-cells': > + const: 0 > + > + external-drv-vbus: > + description: > + If present, configure ulpi-phy external supply to drive 5V on VBus. > + type: boolean > + > + view-port: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + Address to read and write PHY registers with explicit control of > + the address and data using the usb.VIEWPORT register. Why would this not just be a second entry in reg? Cheers, Conor. > + > +required: > + - compatible > + - reg > + - view-port > + > +additionalProperties: false > + > +examples: > + - | > + phy0@e0002000 { > + compatible = "ulpi-phy"; > + #phy-cells = <0x00>; > + reg = <0xe0002000 0x1000>; > + view-port = <0x170>; > + external-drv-vbus; > + }; > -- > 2.17.1 >
On 29/09/2023 08:48, Piyush Mehta wrote: > Create an ulpi-phy binding to read and write PHY registers with explicit > control of the address and data using the usb.VIEWPORT register. > > Signed-off-by: Piyush Mehta <piyush.mehta@amd.com> Subject: dt-bindings, not dt-binding. 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. > --- > This binding patch was created to support generic platforms. This binding > will be modified in accordance with patch [3/3] procedures. I don't understand this. How binding can be updated by further procedures? Your patch 3 is a driver, so how driver can modify a binding? Best regards, Krzysztof
On Fri, Sep 29, 2023 at 12:18:50PM +0530, Piyush Mehta wrote: > Create an ulpi-phy binding to read and write PHY registers with explicit > control of the address and data using the usb.VIEWPORT register. > > Signed-off-by: Piyush Mehta <piyush.mehta@amd.com> > --- > This binding patch was created to support generic platforms. This binding > will be modified in accordance with patch [3/3] procedures. One of the > approch may be Create a zynq phy platform driver in "driver/usb/phy" with > driver source "phy-ulpi-zynq-usb.c" and then the binding will be particular > to the Xilinx/AMD zynq platform. > > This binding was built with the Zynq hardware design example in consideration > of as a generic platform. The viewport provide access the Chipidea controller > to interface with the ULPI PHY. > --- > .../devicetree/bindings/usb/ulpi-phy.yaml | 48 +++++++++++++++++++ > 1 file changed, 48 insertions(+) > create mode 100644 Documentation/devicetree/bindings/usb/ulpi-phy.yaml > > diff --git a/Documentation/devicetree/bindings/usb/ulpi-phy.yaml b/Documentation/devicetree/bindings/usb/ulpi-phy.yaml > new file mode 100644 > index 000000000000..490b2f610129 > --- /dev/null > +++ b/Documentation/devicetree/bindings/usb/ulpi-phy.yaml > @@ -0,0 +1,48 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/usb/ulpi-phy.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: ULPI PHY- Generic platform > + > +maintainers: > + - Piyush Mehta <piyush.mehta@amd.com> > + > +properties: > + compatible: > + const: ulpi-phy > + > + reg: > + maxItems: 1 > + > + '#phy-cells': > + const: 0 > + > + external-drv-vbus: > + description: > + If present, configure ulpi-phy external supply to drive 5V on VBus. > + type: boolean > + > + view-port: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + Address to read and write PHY registers with explicit control of > + the address and data using the usb.VIEWPORT register. > + > +required: > + - compatible > + - reg > + - view-port > + > +additionalProperties: false > + > +examples: > + - | > + phy0@e0002000 { > + compatible = "ulpi-phy"; > + #phy-cells = <0x00>; > + reg = <0xe0002000 0x1000>; > + view-port = <0x170>; I don't understand. Do you have an MMIO address and the VIEWPORT address to the PHY? You need both? There's already a defined binding for ULPI bus: Documentation/devicetree/bindings/usb/ulpi.txt Why can't you use/expand that? Rob
Hi All, > -----Original Message----- > From: Rob Herring <robh@kernel.org> > Sent: Monday, October 2, 2023 10:30 PM > To: Mehta, Piyush <piyush.mehta@amd.com> > Cc: gregkh@linuxfoundation.org; Simek, Michal <michal.simek@amd.com>; > krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; > peter.chen@kernel.org; linus.walleij@linaro.org; paul@crapouillou.net; > arnd@arndb.de; linux-usb@vger.kernel.org; devicetree@vger.kernel.org; linux- > kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com> > Subject: Re: [RFC PATCH 1/3] dt-binding: usb: ulpi-phy: add ulpi-phy binding > > On Fri, Sep 29, 2023 at 12:18:50PM +0530, Piyush Mehta wrote: > > Create an ulpi-phy binding to read and write PHY registers with > > explicit control of the address and data using the usb.VIEWPORT register. > > > > Signed-off-by: Piyush Mehta <piyush.mehta@amd.com> > > --- > > This binding patch was created to support generic platforms. This > > binding will be modified in accordance with patch [3/3] procedures. > > One of the approch may be Create a zynq phy platform driver in > > "driver/usb/phy" with driver source "phy-ulpi-zynq-usb.c" and then the > > binding will be particular to the Xilinx/AMD zynq platform. > > > > This binding was built with the Zynq hardware design example in > > consideration of as a generic platform. The viewport provide access > > the Chipidea controller to interface with the ULPI PHY. > > --- > > .../devicetree/bindings/usb/ulpi-phy.yaml | 48 +++++++++++++++++++ > > 1 file changed, 48 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/usb/ulpi-phy.yaml > > > > diff --git a/Documentation/devicetree/bindings/usb/ulpi-phy.yaml > > b/Documentation/devicetree/bindings/usb/ulpi-phy.yaml > > new file mode 100644 > > index 000000000000..490b2f610129 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/usb/ulpi-phy.yaml > > @@ -0,0 +1,48 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/usb/ulpi-phy.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: ULPI PHY- Generic platform > > + > > +maintainers: > > + - Piyush Mehta <piyush.mehta@amd.com> > > + > > +properties: > > + compatible: > > + const: ulpi-phy > > + > > + reg: > > + maxItems: 1 > > + > > + '#phy-cells': > > + const: 0 > > + > > + external-drv-vbus: > > + description: > > + If present, configure ulpi-phy external supply to drive 5V on VBus. > > + type: boolean > > + > > + view-port: > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: > > + Address to read and write PHY registers with explicit control of > > + the address and data using the usb.VIEWPORT register. > > + > > +required: > > + - compatible > > + - reg > > + - view-port > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + phy0@e0002000 { > > + compatible = "ulpi-phy"; > > + #phy-cells = <0x00>; > > + reg = <0xe0002000 0x1000>; > > + view-port = <0x170>; > > I don't understand. Do you have an MMIO address and the VIEWPORT address > to the PHY? You need both? Yes, we need both. The ULPI Link wrapper passes-through packet data and interprets Rx commands as well as send Tx commands and provide viewport services to the software. The ULPI link wrapper interfaces between the port controller (a bus similar to UTMI+ that connects to the rest of the controller and its registers) and the ULPI interface. Name XUSBPS_ULPIVIEW_OFFSET Address 0x00000170 Description ULPI Viewport The register provides indirect access to the ULPI PHY register set. Although the core performs access to the ULPI PHY register set, there may be extraordinary circumstances where software may need direct access. ULPI PHY Viewport The ULPI viewport provides a mechanism for software to read and write PHY registers with explicit control of the address and data using the usb.VIEWPORT register. An interrupt is generated when a transaction is complete, including when the requested read data is available. > > There's already a defined binding for ULPI bus: > > Documentation/devicetree/bindings/usb/ulpi.txt > > Why can't you use/expand that? > > Rob We need your input to determine the best approach . We did preliminary research and discovered few possibilities: USB Node { ............. ULPI PHY Node { // child node ................. compatible = "ulpi-phy-"; #phy-cells = <0x00>; .............. }; }; https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/phy/qcom%2Cusb-hs-phy.yaml#L100 https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml#L338 https://github.com/torvalds/linux/blob/master/drivers/usb/chipidea/ci_hdrc_msm.c#L245 https://github.com/torvalds/linux/blob/master/drivers/phy/qualcomm/phy-qcom-usb-hs.c#L85 [This implementation is based on ulpi driver: https://github.com/torvalds/linux/blob/master/drivers/phy/qualcomm/phy-qcom-usb-hs.c#L287C1-L287C19] OR https://github.com/torvalds/linux/blob/master/drivers/usb/chipidea/ci_hdrc_imx.c#L81 https://github.com/torvalds/linux/blob/master/drivers/usb/chipidea/ci_hdrc_imx.c#L176 https://github.com/torvalds/linux/blob/master/drivers/usb/chipidea/usbmisc_imx.c#L234 https://github.com/torvalds/linux/blob/master/drivers/usb/phy/phy-mxs-usb.c#L191 [This implementation is based on platform driver: https://github.com/torvalds/linux/blob/master/drivers/usb/phy/phy-mxs-usb.c#L848] Note: Above examples are to show the interface between the Chipidea Controller and PHY. Are these possibilities aligning with your inputs? Regards, Piyush Mehta
> -----Original Message----- > From: Mehta, Piyush > Sent: Wednesday, October 4, 2023 5:15 PM > To: Rob Herring <robh@kernel.org> > Cc: gregkh@linuxfoundation.org; Simek, Michal <michal.simek@amd.com>; > krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; > peter.chen@kernel.org; linus.walleij@linaro.org; paul@crapouillou.net; > arnd@arndb.de; linux-usb@vger.kernel.org; devicetree@vger.kernel.org; > linux-kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com> > Subject: RE: [RFC PATCH 1/3] dt-binding: usb: ulpi-phy: add ulpi-phy binding > > Hi All, > > > -----Original Message----- > > From: Rob Herring <robh@kernel.org> > > Sent: Monday, October 2, 2023 10:30 PM > > To: Mehta, Piyush <piyush.mehta@amd.com> > > Cc: gregkh@linuxfoundation.org; Simek, Michal <michal.simek@amd.com>; > > krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; > > peter.chen@kernel.org; linus.walleij@linaro.org; paul@crapouillou.net; > > arnd@arndb.de; linux-usb@vger.kernel.org; devicetree@vger.kernel.org; > > linux- kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com> > > Subject: Re: [RFC PATCH 1/3] dt-binding: usb: ulpi-phy: add ulpi-phy > > binding > > > > On Fri, Sep 29, 2023 at 12:18:50PM +0530, Piyush Mehta wrote: > > > Create an ulpi-phy binding to read and write PHY registers with > > > explicit control of the address and data using the usb.VIEWPORT register. > > > > > > Signed-off-by: Piyush Mehta <piyush.mehta@amd.com> > > > --- > > > This binding patch was created to support generic platforms. This > > > binding will be modified in accordance with patch [3/3] procedures. > > > One of the approch may be Create a zynq phy platform driver in > > > "driver/usb/phy" with driver source "phy-ulpi-zynq-usb.c" and then > > > the binding will be particular to the Xilinx/AMD zynq platform. > > > > > > This binding was built with the Zynq hardware design example in > > > consideration of as a generic platform. The viewport provide access > > > the Chipidea controller to interface with the ULPI PHY. > > > --- > > > .../devicetree/bindings/usb/ulpi-phy.yaml | 48 +++++++++++++++++++ > > > 1 file changed, 48 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/usb/ulpi-phy.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/usb/ulpi-phy.yaml > > > b/Documentation/devicetree/bindings/usb/ulpi-phy.yaml > > > new file mode 100644 > > > index 000000000000..490b2f610129 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/usb/ulpi-phy.yaml > > > @@ -0,0 +1,48 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/usb/ulpi-phy.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: ULPI PHY- Generic platform > > > + > > > +maintainers: > > > + - Piyush Mehta <piyush.mehta@amd.com> > > > + > > > +properties: > > > + compatible: > > > + const: ulpi-phy > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + '#phy-cells': > > > + const: 0 > > > + > > > + external-drv-vbus: > > > + description: > > > + If present, configure ulpi-phy external supply to drive 5V on VBus. > > > + type: boolean > > > + > > > + view-port: > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + description: > > > + Address to read and write PHY registers with explicit control of > > > + the address and data using the usb.VIEWPORT register. > > > + > > > +required: > > > + - compatible > > > + - reg > > > + - view-port > > > + > > > +additionalProperties: false > > > + > > > +examples: > > > + - | > > > + phy0@e0002000 { > > > + compatible = "ulpi-phy"; > > > + #phy-cells = <0x00>; > > > + reg = <0xe0002000 0x1000>; > > > + view-port = <0x170>; > > > > I don't understand. Do you have an MMIO address and the VIEWPORT > > address to the PHY? You need both? > > Yes, we need both. > > The ULPI Link wrapper passes-through packet data and interprets Rx > commands as well as send Tx commands and provide viewport services to the > software. The ULPI link wrapper interfaces between the port controller (a bus > similar to UTMI+ that connects to the rest of the controller and its registers) > and the ULPI interface. > > Name XUSBPS_ULPIVIEW_OFFSET > Address 0x00000170 > > Description ULPI Viewport > > The register provides indirect access to the ULPI PHY register set. Although the > core performs access to the ULPI PHY register set, there may be extraordinary > circumstances where software may need direct access. > > ULPI PHY Viewport > The ULPI viewport provides a mechanism for software to read and write PHY > registers with explicit control of the address and data using the usb.VIEWPORT > register. An interrupt is generated when a transaction is complete, including > when the requested read data is available. > > > > > There's already a defined binding for ULPI bus: > > > > Documentation/devicetree/bindings/usb/ulpi.txt > > > > Why can't you use/expand that? > > > > Rob > > We need your input to determine the best approach . We did preliminary > research and discovered few possibilities: > > USB Node { > ............. > ULPI PHY Node { // child node > ................. > compatible = "ulpi-phy-"; > #phy-cells = <0x00>; > .............. > }; > }; > > https://github.com/torvalds/linux/blob/master/Documentation/devicetree/b > indings/phy/qcom%2Cusb-hs-phy.yaml#L100 > https://github.com/torvalds/linux/blob/master/Documentation/devicetree/b > indings/usb/ci-hdrc-usb2.yaml#L338 > https://github.com/torvalds/linux/blob/master/drivers/usb/chipidea/ci_hdrc > _msm.c#L245 > https://github.com/torvalds/linux/blob/master/drivers/phy/qualcomm/phy- > qcom-usb-hs.c#L85 > [This implementation is based on ulpi driver: > https://github.com/torvalds/linux/blob/master/drivers/phy/qualcomm/phy- > qcom-usb-hs.c#L287C1-L287C19] > > OR > > https://github.com/torvalds/linux/blob/master/drivers/usb/chipidea/ci_hdrc > _imx.c#L81 > https://github.com/torvalds/linux/blob/master/drivers/usb/chipidea/ci_hdrc > _imx.c#L176 > https://github.com/torvalds/linux/blob/master/drivers/usb/chipidea/usbmis > c_imx.c#L234 > https://github.com/torvalds/linux/blob/master/drivers/usb/phy/phy-mxs- > usb.c#L191 > [This implementation is based on platform driver: > https://github.com/torvalds/linux/blob/master/drivers/usb/phy/phy-mxs- > usb.c#L848] > > Note: > Above examples are to show the interface between the Chipidea Controller > and PHY. > > Are these possibilities aligning with your inputs? > Please share your inputs on the above alternate design approaches. > Regards, > Piyush Mehta BR, PM
diff --git a/Documentation/devicetree/bindings/usb/ulpi-phy.yaml b/Documentation/devicetree/bindings/usb/ulpi-phy.yaml new file mode 100644 index 000000000000..490b2f610129 --- /dev/null +++ b/Documentation/devicetree/bindings/usb/ulpi-phy.yaml @@ -0,0 +1,48 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/usb/ulpi-phy.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: ULPI PHY- Generic platform + +maintainers: + - Piyush Mehta <piyush.mehta@amd.com> + +properties: + compatible: + const: ulpi-phy + + reg: + maxItems: 1 + + '#phy-cells': + const: 0 + + external-drv-vbus: + description: + If present, configure ulpi-phy external supply to drive 5V on VBus. + type: boolean + + view-port: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + Address to read and write PHY registers with explicit control of + the address and data using the usb.VIEWPORT register. + +required: + - compatible + - reg + - view-port + +additionalProperties: false + +examples: + - | + phy0@e0002000 { + compatible = "ulpi-phy"; + #phy-cells = <0x00>; + reg = <0xe0002000 0x1000>; + view-port = <0x170>; + external-drv-vbus; + };
Create an ulpi-phy binding to read and write PHY registers with explicit control of the address and data using the usb.VIEWPORT register. Signed-off-by: Piyush Mehta <piyush.mehta@amd.com> --- This binding patch was created to support generic platforms. This binding will be modified in accordance with patch [3/3] procedures. One of the approch may be Create a zynq phy platform driver in "driver/usb/phy" with driver source "phy-ulpi-zynq-usb.c" and then the binding will be particular to the Xilinx/AMD zynq platform. This binding was built with the Zynq hardware design example in consideration of as a generic platform. The viewport provide access the Chipidea controller to interface with the ULPI PHY. --- .../devicetree/bindings/usb/ulpi-phy.yaml | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 Documentation/devicetree/bindings/usb/ulpi-phy.yaml