Re: [PATCH 1/2] dt-bindings: usb: Add binding for the TI wrapper for Cadence USB3 controller

[Roger Quadros]

Hi,

On 16/10/2019 00:29, Rob Herring wrote:
> On Mon, Oct 07, 2019 at 02:41:41PM +0300, Roger Quadros wrote:
> > TI platforms have a wrapper module around the Cadence USB3
> > controller. Add binding information for that.
> >
> > Signed-off-by: Roger Quadros <rogerq@xxxxxx>
> > Signed-off-by: Sekhar Nori <nsekhar@xxxxxx>
> > ---
> >   .../devicetree/bindings/usb/cdns-usb3-ti.txt  | 59 +++++++++++++++++++
> >   1 file changed, 59 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/usb/cdns-usb3-ti.txt
>
> Please convert to DT schema.
>
> > diff --git a/Documentation/devicetree/bindings/usb/cdns-usb3-ti.txt b/Documentation/devicetree/bindings/usb/cdns-usb3-ti.txt
> > new file mode 100644
> > index 000000000000..12c7c903e6da
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/cdns-usb3-ti.txt
> > @@ -0,0 +1,59 @@
> > +Binding for the TI specific wrapper for the Cadence USBSS-DRD controller
> > +
> > +Required properties:
> > +  - compatible: Should contain "ti,j721e-usb"
> > +  - reg: Physical base address and size of the wrappers register area.
> > +  - power-domains: Should contain a phandle to a PM domain provider node
> > +                   and an args specifier containing the USB device id
> > +                   value. This property is as per the binding documentation:
> > +                   Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
> > +  - clocks: Clock phandles to usb2_refclk and lpm_clk
> > +  - clock-names: Should contain "usb2_refclk" and "lpm_clk"
>
> _clk is redundant. 'ref' amd 'lpm' is sufficient.
>
> > +
> > +Optional properties:
> > + - ti,usb2-only: If present, it restricts the controller to USB2.0 mode of
> > +		 operation. Must be present if USB3 PHY is not available
> > +		 for USB.
>
> Seems like this should be discoverable based on describing the phy
> connections.

I don't think so. the PHY connections are in the child node.

> > + - ti,modestrap-host: Set controller modestrap to HOST mode.
> > + - ti,modestrap-peripheral: Set controller modestrap to PERIPHERAL mode.
>
> What does modestrap mean? Fixed to the mode or that's the default? For
> default, John Stultz sent a similar binding. Seems we need something
> common.

It means if the controller needs to be hard-wired (using a register bit)
to be either host or peripheral.

I think that this will not really be used so I'll get rid of it. It was mostly
used for initial debug.

The controller is dual-role and the role can be set to either host or peripheral
using the operational registers.

> > + - ti,vbus-divider: Should be present if USB VBUS line is connected to the
> > +		 VBUS pin of the SoC via a 1/3 voltage divider.
> > +
> > +Sub-nodes:
> > +The USB2 PHY and the Cadence USB3 controller should be the sub-nodes.
> > +
> > +Example:
> > +
> > +	ti_usb0: cdns_usb@4104000 {
> > +		compatible = "ti,j721e-usb";
> > +		reg = <0x00 0x4104000 0x00 0x100>;
> > +		power-domains = <&k3_pds 288 TI_SCI_PD_EXCLUSIVE>;
> > +		clocks = <&k3_clks 288 15>, <&k3_clks 288 3>;
> > +		clock-names = "usb2_refclk", "lpm_clk";
> > +		assigned-clocks = <&k3_clks 288 15>;	/* USB2_REFCLK */
> > +		assigned-clock-parents = <&k3_clks 288 16>; /* HFOSC0 */
> > +		#address-cells = <2>;
> > +		#size-cells = <2>;
> > +		ranges;
> > +
> > +		phy@4108000 {
> > +			compatible = "ti,j721e-usb2-phy";
> > +			reg = <0x00 0x4108000 0x00 0x400>;
> > +		};
>
> Why is this a child node? Use the phy binding.

This is a USB2 PHY that physically sits within the ti_usb0 wrapper module that
controls power and clock to it.

There isn't a device driver required for it so should I just get rid of it?
Or can it just live there? what can the compatible be?
Or we can add it later if at all a driver is needed.

> > +
> > +		usb0: usb@6000000 {
> > +			compatible = "cdns,usb3-1.0.1";
>
> Not documented.

should be
	compatible = "cdns,usb3";
> > +			reg = <0x00 0x6000000 0x00 0x10000>,
> > +			      <0x00 0x6010000 0x00 0x10000>,
> > +			      <0x00 0x6020000 0x00 0x10000>;
> > +			reg-names = "otg", "xhci", "dev";
> > +			interrupts = <GIC_SPI 96 IRQ_TYPE_LEVEL_HIGH>,	/* irq.0 */
> > +				     <GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>,	/* irq.6 */
> > +				     <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;	/* otgirq.0 */
> > +			interrupt-names = "host",
> > +					  "peripheral",
> > +					  "otg";
> > +			maximum-speed = "super-speed";
> > +			dr_mode = "otg";
> > +		};
> > +	};

--
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki