RE: [RFC PATCH 1/3] dt-binding: usb: ulpi-phy: add ulpi-phy binding

[Mehta, Piyush]

Hi All,

> -----Original Message-----
> From: Rob Herring <robh@xxxxxxxxxx>
> Sent: Monday, October 2, 2023 10:30 PM
> To: Mehta, Piyush <piyush.mehta@xxxxxxx>
> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; Simek, Michal <michal.simek@xxxxxxx>;
> krzysztof.kozlowski+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx;
> peter.chen@xxxxxxxxxx; linus.walleij@xxxxxxxxxx; paul@xxxxxxxxxxxxxxx;
> arnd@xxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; git (AMD-Xilinx) <git@xxxxxxx>
> 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@xxxxxxx>
> > ---
> > 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@xxxxxxx>
> > +
> > +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