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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 16 Oct 2019 12:51:07 +0300
From:   Roger Quadros <rogerq@...com>
To:     Rob Herring <robh@...nel.org>
CC:     <felipe.balbi@...ux.intel.com>, <gregkh@...uxfoundation.org>,
        <pawell@...ence.com>, <nsekhar@...com>,
        <linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <devicetree@...r.kernel.org>
Subject: Re: [PATCH 1/2] dt-bindings: usb: Add binding for the TI wrapper for
 Cadence USB3 controller

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@...com>
>> Signed-off-by: Sekhar Nori <nsekhar@...com>
>> ---
>>   .../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@...4000 {
>> +		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@...8000 {
>> +			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@...0000 {
>> +			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