2019-06-05 10:14:09

by Pawel Laszczak

[permalink] [raw]
Subject: [PATCH v7 1/6] dt-bindings: add binding for USBSS-DRD controller.

This patch aim at documenting USB related dt-bindings for the
Cadence USBSS-DRD controller.

Signed-off-by: Pawel Laszczak <[email protected]>
Reviewed-by: Rob Herring <[email protected]>

---
.../devicetree/bindings/usb/cdns-usb3.txt | 30 +++++++++++++++++++
1 file changed, 30 insertions(+)
create mode 100644 Documentation/devicetree/bindings/usb/cdns-usb3.txt

diff --git a/Documentation/devicetree/bindings/usb/cdns-usb3.txt b/Documentation/devicetree/bindings/usb/cdns-usb3.txt
new file mode 100644
index 000000000000..1d2b449e3cb4
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/cdns-usb3.txt
@@ -0,0 +1,30 @@
+Binding for the Cadence USBSS-DRD controller
+
+Required properties:
+ - reg: Physical base address and size of the controller's register areas.
+ Controller has 3 different regions:
+ region 1 - HOST registers area
+ region 2 - DEVICE registers area
+ region 3 - OTG/DRD registers area
+ - reg-names - register memory area names:
+ "xhci" - for HOST registers space
+ "dev" - for DEVICE registers space
+ "otg" - for OTG/DRD registers space
+ - compatible: Should contain: "cdns,usb3-1.0.0" or "cdns,usb3-1.0.1"
+ - interrupts: Interrupts used by cdns3 controller.
+
+Optional properties:
+ - maximum-speed : valid arguments are "super-speed", "high-speed" and
+ "full-speed"; refer to usb/generic.txt
+ - dr_mode: Should be one of "host", "peripheral" or "otg".
+ - phys: reference to the USB PHY
+
+Example:
+ usb@f3000000 {
+ compatible = "cdns,usb3-1.0.1";
+ interrupts = <USB_IRQ 7 IRQ_TYPE_LEVEL_HIGH>;
+ reg = <0xf3000000 0x10000 /* memory area for HOST registers */
+ 0xf3010000 0x10000 /* memory area for DEVICE registers */
+ 0xf3020000 0x10000>; /* memory area for OTG/DRD registers */
+ reg-names = "xhci", "dev", "otg";
+ };
--
2.17.1


2019-06-06 16:36:34

by Roger Quadros

[permalink] [raw]
Subject: Re: [PATCH v7 1/6] dt-bindings: add binding for USBSS-DRD controller.

Pawel,

On 05/06/2019 13:03, Pawel Laszczak wrote:
> This patch aim at documenting USB related dt-bindings for the
> Cadence USBSS-DRD controller.
>
> Signed-off-by: Pawel Laszczak <[email protected]>
> Reviewed-by: Rob Herring <[email protected]>
>
> ---
> .../devicetree/bindings/usb/cdns-usb3.txt | 30 +++++++++++++++++++
> 1 file changed, 30 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/usb/cdns-usb3.txt
>
> diff --git a/Documentation/devicetree/bindings/usb/cdns-usb3.txt b/Documentation/devicetree/bindings/usb/cdns-usb3.txt
> new file mode 100644
> index 000000000000..1d2b449e3cb4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/cdns-usb3.txt
> @@ -0,0 +1,30 @@
> +Binding for the Cadence USBSS-DRD controller
> +
> +Required properties:
> + - reg: Physical base address and size of the controller's register areas.
> + Controller has 3 different regions:
> + region 1 - HOST registers area
> + region 2 - DEVICE registers area
> + region 3 - OTG/DRD registers area
> + - reg-names - register memory area names:
> + "xhci" - for HOST registers space
> + "dev" - for DEVICE registers space
> + "otg" - for OTG/DRD registers space
> + - compatible: Should contain: "cdns,usb3-1.0.0" or "cdns,usb3-1.0.1"
> + - interrupts: Interrupts used by cdns3 controller.

Since we are requesting 3 separate named interrupts in code we need to
mention them here.

> +
> +Optional properties:
> + - maximum-speed : valid arguments are "super-speed", "high-speed" and
> + "full-speed"; refer to usb/generic.txt
> + - dr_mode: Should be one of "host", "peripheral" or "otg".
> + - phys: reference to the USB PHY
> +
> +Example:
> + usb@f3000000 {
> + compatible = "cdns,usb3-1.0.1";
> + interrupts = <USB_IRQ 7 IRQ_TYPE_LEVEL_HIGH>;

This example won't work as code requests for 3 separate interrupts.

> + reg = <0xf3000000 0x10000 /* memory area for HOST registers */
> + 0xf3010000 0x10000 /* memory area for DEVICE registers */
> + 0xf3020000 0x10000>; /* memory area for OTG/DRD registers */
> + reg-names = "xhci", "dev", "otg";
> + };
>

--
cheers,
-roger

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

2019-06-07 04:48:58

by Pawel Laszczak

[permalink] [raw]
Subject: RE: [PATCH v7 1/6] dt-bindings: add binding for USBSS-DRD controller.


>
>Pawel,
>
>On 05/06/2019 13:03, Pawel Laszczak wrote:
>> This patch aim at documenting USB related dt-bindings for the
>> Cadence USBSS-DRD controller.
>>
>> Signed-off-by: Pawel Laszczak <[email protected]>
>> Reviewed-by: Rob Herring <[email protected]>
>>
>> ---
>> .../devicetree/bindings/usb/cdns-usb3.txt | 30 +++++++++++++++++++
>> 1 file changed, 30 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/usb/cdns-usb3.txt
>>
>> diff --git a/Documentation/devicetree/bindings/usb/cdns-usb3.txt b/Documentation/devicetree/bindings/usb/cdns-usb3.txt
>> new file mode 100644
>> index 000000000000..1d2b449e3cb4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/cdns-usb3.txt
>> @@ -0,0 +1,30 @@
>> +Binding for the Cadence USBSS-DRD controller
>> +
>> +Required properties:
>> + - reg: Physical base address and size of the controller's register areas.
>> + Controller has 3 different regions:
>> + region 1 - HOST registers area
>> + region 2 - DEVICE registers area
>> + region 3 - OTG/DRD registers area
>> + - reg-names - register memory area names:
>> + "xhci" - for HOST registers space
>> + "dev" - for DEVICE registers space
>> + "otg" - for OTG/DRD registers space
>> + - compatible: Should contain: "cdns,usb3-1.0.0" or "cdns,usb3-1.0.1"
>> + - interrupts: Interrupts used by cdns3 controller.
>
>Since we are requesting 3 separate named interrupts in code we need to
>mention them here.

Hi Roger,

Yes, I know, but this code is Reviewed-by: Rob Herring, so I thought to add
this as separate patch after putting this driver into kernel.

I assume that after making some change in this file I should remove statement
Reviewed-by: Rob Herring <[email protected]> from this patch to start reviving
from the beginning.

What do you think about such dt-binding:

Required properties:
- reg: Physical base address and size of the controller's register areas.
Controller has 3 different regions:
region 1 - HOST registers area
region 2 - DEVICE registers area
region 3 - OTG/DRD registers area
- reg-names - register memory area names:
"xhci" - for HOST registers space
"dev" - for DEVICE registers space
"otg" - for OTG/DRD registers space
- compatible: Should contain:
"cdns,usb3-1.0.0" - for 0x00024502 controller version
"cdns,usb3-1.0.1" - for 0x00024509 controller version
"cdns,usb3-1.0.2" - for 0x0002450C controller version
"cdns,usb3-1.0.3" - for 0x0002450d controller version

- interrupts: Interrupts used by cdns3 controller:
"host" - interrupt used by XHCI driver.
"peripheral" - interrupt used by device driver
"otg" - interrupt used by DRD/OTG part of driver

Optional properties:
- maximum-speed : valid arguments are "super-speed", "high-speed" and
"full-speed"; refer to usb/generic.txt
- dr_mode: Should be one of "host", "peripheral" or "otg".
- phys: reference to the USB PHY
- on-chip-buff-size : size of memory intended as internal memory for endpoints
buffers expressed in KB

Example:
usb@f3000000 {
compatible = "cdns,usb3-1.0.1";
interrupts = <USB_HOST_IRQ 7 IRQ_TYPE_LEVEL_HIGH>
<USB_DEVICE_IRQ 7 IRQ_TYPE_LEVEL_HIGH>
<USB_OTG_IRQ 8 IRQ_TYPE_LEVEL_HIGH>;
interrupt-names = "host", "peripheral", "otg";
reg = <0xf3000000 0x10000 /* memory area for HOST registers */
0xf3010000 0x10000 /* memory area for DEVICE registers */
0xf3020000 0x10000>; /* memory area for OTG/DRD registers */
reg-names = "xhci", "dev", "otg";
};

Is this correct now ?

Maybe I should add something additionally for phy ?

Regards,
Pawel


>
>> +
>> +Optional properties:
>> + - maximum-speed : valid arguments are "super-speed", "high-speed" and
>> + "full-speed"; refer to usb/generic.txt
>> + - dr_mode: Should be one of "host", "peripheral" or "otg".
>> + - phys: reference to the USB PHY
>> +
>> +Example:
>> + usb@f3000000 {
>> + compatible = "cdns,usb3-1.0.1";
>> + interrupts = <USB_IRQ 7 IRQ_TYPE_LEVEL_HIGH>;
>
>This example won't work as code requests for 3 separate interrupts.
>
>> + reg = <0xf3000000 0x10000 /* memory area for HOST registers */
>> + 0xf3010000 0x10000 /* memory area for DEVICE registers */
>> + 0xf3020000 0x10000>; /* memory area for OTG/DRD registers */
>> + reg-names = "xhci", "dev", "otg";
>> + };
>>
>
>--
>cheers,
>-roger
>
>Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

2019-06-07 10:00:23

by Roger Quadros

[permalink] [raw]
Subject: Re: [PATCH v7 1/6] dt-bindings: add binding for USBSS-DRD controller.

On 07/06/2019 07:46, Pawel Laszczak wrote:
>
>>
>> Pawel,
>>
>> On 05/06/2019 13:03, Pawel Laszczak wrote:
>>> This patch aim at documenting USB related dt-bindings for the
>>> Cadence USBSS-DRD controller.
>>>
>>> Signed-off-by: Pawel Laszczak <[email protected]>
>>> Reviewed-by: Rob Herring <[email protected]>
>>>
>>> ---
>>> .../devicetree/bindings/usb/cdns-usb3.txt | 30 +++++++++++++++++++
>>> 1 file changed, 30 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/usb/cdns-usb3.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/cdns-usb3.txt b/Documentation/devicetree/bindings/usb/cdns-usb3.txt
>>> new file mode 100644
>>> index 000000000000..1d2b449e3cb4
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/usb/cdns-usb3.txt
>>> @@ -0,0 +1,30 @@
>>> +Binding for the Cadence USBSS-DRD controller
>>> +
>>> +Required properties:
>>> + - reg: Physical base address and size of the controller's register areas.
>>> + Controller has 3 different regions:
>>> + region 1 - HOST registers area
>>> + region 2 - DEVICE registers area
>>> + region 3 - OTG/DRD registers area
>>> + - reg-names - register memory area names:
>>> + "xhci" - for HOST registers space
>>> + "dev" - for DEVICE registers space
>>> + "otg" - for OTG/DRD registers space
>>> + - compatible: Should contain: "cdns,usb3-1.0.0" or "cdns,usb3-1.0.1"
>>> + - interrupts: Interrupts used by cdns3 controller.
>>
>> Since we are requesting 3 separate named interrupts in code we need to
>> mention them here.
>
> Hi Roger,
>
> Yes, I know, but this code is Reviewed-by: Rob Herring, so I thought to add
> this as separate patch after putting this driver into kernel.
>

Binding document should be updated as code changes.

> I assume that after making some change in this file I should remove statement
> Reviewed-by: Rob Herring <[email protected]> from this patch to start reviving
> from the beginning.

Yes.

>
> What do you think about such dt-binding:
>
> Required properties:
> - reg: Physical base address and size of the controller's register areas.
> Controller has 3 different regions:
> region 1 - HOST registers area
> region 2 - DEVICE registers area
> region 3 - OTG/DRD registers area

Is it so that region 1 is always HOST?
offset 0 seems to be OTG though.

If it is implementation specific then you could get rid of numbering.

> - reg-names - register memory area names:
> "xhci" - for HOST registers space
> "dev" - for DEVICE registers space
> "otg" - for OTG/DRD registers space
> - compatible: Should contain:
> "cdns,usb3-1.0.0" - for 0x00024502 controller version
> "cdns,usb3-1.0.1" - for 0x00024509 controller version
> "cdns,usb3-1.0.2" - for 0x0002450C controller version
> "cdns,usb3-1.0.3" - for 0x0002450d controller version
>
> - interrupts: Interrupts used by cdns3 controller:
> "host" - interrupt used by XHCI driver.
> "peripheral" - interrupt used by device driver
> "otg" - interrupt used by DRD/OTG part of driver
>
> Optional properties:
> - maximum-speed : valid arguments are "super-speed", "high-speed" and
> "full-speed"; refer to usb/generic.txt
> - dr_mode: Should be one of "host", "peripheral" or "otg".
> - phys: reference to the USB PHY

need to add 'phy-names' since you request the PHY with name.

> - on-chip-buff-size : size of memory intended as internal memory for endpoints
> buffers expressed in KB
>
> Example:
> usb@f3000000 {
> compatible = "cdns,usb3-1.0.1";
> interrupts = <USB_HOST_IRQ 7 IRQ_TYPE_LEVEL_HIGH>

coma missing.
> <USB_DEVICE_IRQ 7 IRQ_TYPE_LEVEL_HIGH>
here too.
> <USB_OTG_IRQ 8 IRQ_TYPE_LEVEL_HIGH>;

What is 7 and 8 ?

> interrupt-names = "host", "peripheral", "otg";
> reg = <0xf3000000 0x10000 /* memory area for HOST registers */

should end with >,

> 0xf3010000 0x10000 /* memory area for DEVICE registers */
here too

> 0xf3020000 0x10000>; /* memory area for OTG/DRD registers */
> reg-names = "xhci", "dev", "otg";
> };

Isn't otg at offset 0, xhci at offset 0x10000 and dev at offset 0x20000?

>
> Is this correct now ?
>
> Maybe I should add something additionally for phy ?

e.g.

phys = <&usb3_phy>;
phy-names = "cdns3,usbphy";

>
> Regards,
> Pawel
>
>
>>
>>> +
>>> +Optional properties:
>>> + - maximum-speed : valid arguments are "super-speed", "high-speed" and
>>> + "full-speed"; refer to usb/generic.txt
>>> + - dr_mode: Should be one of "host", "peripheral" or "otg".
>>> + - phys: reference to the USB PHY
>>> +
>>> +Example:
>>> + usb@f3000000 {
>>> + compatible = "cdns,usb3-1.0.1";
>>> + interrupts = <USB_IRQ 7 IRQ_TYPE_LEVEL_HIGH>;
>>
>> This example won't work as code requests for 3 separate interrupts.
>>
>>> + reg = <0xf3000000 0x10000 /* memory area for HOST registers */
>>> + 0xf3010000 0x10000 /* memory area for DEVICE registers */
>>> + 0xf3020000 0x10000>; /* memory area for OTG/DRD registers */
>>> + reg-names = "xhci", "dev", "otg";
>>> + };
>>>
>>


--
cheers,
-roger

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

2019-06-10 05:00:28

by Pawel Laszczak

[permalink] [raw]
Subject: RE: [PATCH v7 1/6] dt-bindings: add binding for USBSS-DRD controller.

>>
>>>
>>> Pawel,
>>>
>>> On 05/06/2019 13:03, Pawel Laszczak wrote:
>>>> This patch aim at documenting USB related dt-bindings for the
>>>> Cadence USBSS-DRD controller.
>>>>
>>>> Signed-off-by: Pawel Laszczak <[email protected]>
>>>> Reviewed-by: Rob Herring <[email protected]>
>>>>
>>>> ---
>>>> .../devicetree/bindings/usb/cdns-usb3.txt | 30 +++++++++++++++++++
>>>> 1 file changed, 30 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/usb/cdns-usb3.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/usb/cdns-usb3.txt b/Documentation/devicetree/bindings/usb/cdns-usb3.txt
>>>> new file mode 100644
>>>> index 000000000000..1d2b449e3cb4
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/usb/cdns-usb3.txt
>>>> @@ -0,0 +1,30 @@
>>>> +Binding for the Cadence USBSS-DRD controller
>>>> +
>>>> +Required properties:
>>>> + - reg: Physical base address and size of the controller's register areas.
>>>> + Controller has 3 different regions:
>>>> + region 1 - HOST registers area
>>>> + region 2 - DEVICE registers area
>>>> + region 3 - OTG/DRD registers area
>>>> + - reg-names - register memory area names:
>>>> + "xhci" - for HOST registers space
>>>> + "dev" - for DEVICE registers space
>>>> + "otg" - for OTG/DRD registers space
>>>> + - compatible: Should contain: "cdns,usb3-1.0.0" or "cdns,usb3-1.0.1"
>>>> + - interrupts: Interrupts used by cdns3 controller.
>>>
>>> Since we are requesting 3 separate named interrupts in code we need to
>>> mention them here.
>>
>> Hi Roger,
>>
>> Yes, I know, but this code is Reviewed-by: Rob Herring, so I thought to add
>> this as separate patch after putting this driver into kernel.
>>
>
>Binding document should be updated as code changes.
>
>> I assume that after making some change in this file I should remove statement
>> Reviewed-by: Rob Herring <[email protected]> from this patch to start reviving
>> from the beginning.
>
>Yes.
>
>>
>> What do you think about such dt-binding:
>>
>> Required properties:
>> - reg: Physical base address and size of the controller's register areas.
>> Controller has 3 different regions:
>> region 1 - HOST registers area
>> region 2 - DEVICE registers area
>> region 3 - OTG/DRD registers area
>
>Is it so that region 1 is always HOST?
>offset 0 seems to be OTG though.
>
>If it is implementation specific then you could get rid of numbering.

Right, I rid of numbering.
>
>> - reg-names - register memory area names:
>> "xhci" - for HOST registers space
>> "dev" - for DEVICE registers space
>> "otg" - for OTG/DRD registers space
>> - compatible: Should contain:
>> "cdns,usb3-1.0.0" - for 0x00024502 controller version
>> "cdns,usb3-1.0.1" - for 0x00024509 controller version
>> "cdns,usb3-1.0.2" - for 0x0002450C controller version
>> "cdns,usb3-1.0.3" - for 0x0002450d controller version
>>
>> - interrupts: Interrupts used by cdns3 controller:
>> "host" - interrupt used by XHCI driver.
>> "peripheral" - interrupt used by device driver
>> "otg" - interrupt used by DRD/OTG part of driver
>>
>> Optional properties:
>> - maximum-speed : valid arguments are "super-speed", "high-speed" and
>> "full-speed"; refer to usb/generic.txt
>> - dr_mode: Should be one of "host", "peripheral" or "otg".
>> - phys: reference to the USB PHY
>
>need to add 'phy-names' since you request the PHY with name.
>
>> - on-chip-buff-size : size of memory intended as internal memory for endpoints
>> buffers expressed in KB
>>
>> Example:
>> usb@f3000000 {
>> compatible = "cdns,usb3-1.0.1";
>> interrupts = <USB_HOST_IRQ 7 IRQ_TYPE_LEVEL_HIGH>
>
>coma missing.
>> <USB_DEVICE_IRQ 7 IRQ_TYPE_LEVEL_HIGH>
>here too.
>> <USB_OTG_IRQ 8 IRQ_TYPE_LEVEL_HIGH>;
>
>What is 7 and 8 ?
Interrupt line number. As I remember device and host are connected to the same interrupt
line.

I'm not sure but first element is probably the number indicating of Interrupt controller,
so rather It should has the same name for all interrupts entries. I will change it to GIC_USB_IRQ.

>
>> interrupt-names = "host", "peripheral", "otg";
>> reg = <0xf3000000 0x10000 /* memory area for HOST registers */
>
>should end with >,
I've checked in other dt-binding, and there use in this form.
I will change it. Checkpatch script doesn't complain for both version.
>
>> 0xf3010000 0x10000 /* memory area for DEVICE registers */
>here too
>
>> 0xf3020000 0x10000>; /* memory area for OTG/DRD registers */
>> reg-names = "xhci", "dev", "otg";
>> };
>
>Isn't otg at offset 0, xhci at offset 0x10000 and dev at offset 0x20000?

I think that it's FPGA implementation specific. I have connected registers in this way.
In register specification OTG offset is 0.

>
>>
>> Is this correct now ?
>>
>> Maybe I should add something additionally for phy ?
>
>e.g.
>
> phys = <&usb3_phy>;
> phy-names = "cdns3,usbphy";
>
Ok, I will add it, but they are optional parameter.

New version will looks like:
Binding for the Cadence USBSS-DRD controller

Required properties:
- reg: Physical base address and size of the controller's register areas.
Controller has 3 different regions:
- HOST registers area
- DEVICE registers area
- OTG/DRD registers area
- reg-names - register memory area names:
"xhci" - for HOST registers space
"dev" - for DEVICE registers space
"otg" - for OTG/DRD registers space
- compatible: Should contain:
"cdns,usb3-1.0.0" - for 0x00024502 controller version
"cdns,usb3-1.0.1" - for 0x00024509 controller version
"cdns,usb3-1.0.2" - for 0x0002450C controller version
"cdns,usb3-1.0.3" - for 0x0002450d controller version
- interrupts: Interrupts used by cdns3 controller:
"host" - interrupt used by XHCI driver.
"peripheral" - interrupt used by device driver
"otg" - interrupt used by DRD/OTG part of driver

Optional properties:
- maximum-speed : valid arguments are "super-speed", "high-speed" and
"full-speed"; refer to usb/generic.txt
- dr_mode: Should be one of "host", "peripheral" or "otg".
- phys: reference to the USB PHY
- phy-names: from the *Generic PHY* bindings;
- on-chip-buff-size : size of memory intended as internal memory for endpoints
buffers expressed in KB

Example:
usb@f3000000 {
compatible = "cdns,usb3-1.0.1";
interrupts = <GIC_USB_IRQ 7 IRQ_TYPE_LEVEL_HIGH>,
<GIC_USB_IRQ 7 IRQ_TYPE_LEVEL_HIGH>,
<GIC_USB_IRQ 8 IRQ_TYPE_LEVEL_HIGH>;
interrupt-names = "host", "peripheral", "otg";
reg = <0xf3000000 0x10000>, /* memory area for HOST registers */
<0xf3010000 0x10000>, /* memory area for DEVICE registers */
<0xf3020000 0x10000>; /* memory area for OTG/DRD registers */
reg-names = "xhci", "dev", "otg";
phys = <&usb3_phy>;
phy-names = "cdns3,usbphy";


Maybe I should add also something for usb2_phy in dt-binding and in driver ?

Thanks,
Regards,
Pawel

>>
>> Regards,
>> Pawel
>>
>>
>>>
>>>> +
>>>> +Optional properties:
>>>> + - maximum-speed : valid arguments are "super-speed", "high-speed" and
>>>> + "full-speed"; refer to usb/generic.txt
>>>> + - dr_mode: Should be one of "host", "peripheral" or "otg".
>>>> + - phys: reference to the USB PHY
>>>> +
>>>> +Example:
>>>> + usb@f3000000 {
>>>> + compatible = "cdns,usb3-1.0.1";
>>>> + interrupts = <USB_IRQ 7 IRQ_TYPE_LEVEL_HIGH>;
>>>
>>> This example won't work as code requests for 3 separate interrupts.
>>>
>>>> + reg = <0xf3000000 0x10000 /* memory area for HOST registers */
>>>> + 0xf3010000 0x10000 /* memory area for DEVICE registers */
>>>> + 0xf3020000 0x10000>; /* memory area for OTG/DRD registers */
>>>> + reg-names = "xhci", "dev", "otg";
>>>> + };
>>>>
>>>
>
>
>--
>cheers,
>-roger
>
>Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

2019-06-10 06:17:18

by Peter Chen

[permalink] [raw]
Subject: RE: [PATCH v7 1/6] dt-bindings: add binding for USBSS-DRD controller.


> - reg-names - register memory area names:
> "xhci" - for HOST registers space
> "dev" - for DEVICE registers space
> "otg" - for OTG/DRD registers space
> - compatible: Should contain:
> "cdns,usb3-1.0.0" - for 0x00024502 controller version
> "cdns,usb3-1.0.1" - for 0x00024509 controller version
> "cdns,usb3-1.0.2" - for 0x0002450C controller version
> "cdns,usb3-1.0.3" - for 0x0002450d controller version

If the value can be read from register, why we still need different compatibles?

> Example:
> usb@f3000000 {
> compatible = "cdns,usb3-1.0.1";
> interrupts = <GIC_USB_IRQ 7 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_USB_IRQ 7 IRQ_TYPE_LEVEL_HIGH>,
> <GIC_USB_IRQ 8 IRQ_TYPE_LEVEL_HIGH>;
> interrupt-names = "host", "peripheral", "otg";
> reg = <0xf3000000 0x10000>, /* memory area for HOST registers */
> <0xf3010000 0x10000>, /* memory area for DEVICE registers */
> <0xf3020000 0x10000>; /* memory area for OTG/DRD registers
> */
> reg-names = "xhci", "dev", "otg";
> phys = <&usb3_phy>;
> phy-names = "cdns3,usbphy";
>
>
> Maybe I should add also something for usb2_phy in dt-binding and in driver ?
>

The PHY driver is located at: drivers/phy/cadence/phy-cadence-sierra.c.
Will we need another one for USB2 PHY?

Peter


> Thanks,
> Regards,
> Pawel
>
> >>
> >> Regards,
> >> Pawel
> >>
> >>
> >>>
> >>>> +
> >>>> +Optional properties:
> >>>> + - maximum-speed : valid arguments are "super-speed", "high-speed" and
> >>>> + "full-speed"; refer to usb/generic.txt
> >>>> + - dr_mode: Should be one of "host", "peripheral" or "otg".
> >>>> + - phys: reference to the USB PHY
> >>>> +
> >>>> +Example:
> >>>> + usb@f3000000 {
> >>>> + compatible = "cdns,usb3-1.0.1";
> >>>> + interrupts = <USB_IRQ 7 IRQ_TYPE_LEVEL_HIGH>;
> >>>
> >>> This example won't work as code requests for 3 separate interrupts.
> >>>
> >>>> + reg = <0xf3000000 0x10000 /* memory area for HOST
> registers */
> >>>> + 0xf3010000 0x10000 /* memory area for DEVICE
> registers */
> >>>> + 0xf3020000 0x10000>; /* memory area for
> OTG/DRD registers */
> >>>> + reg-names = "xhci", "dev", "otg";
> >>>> + };
> >>>>
> >>>
> >
> >
> >--
> >cheers,
> >-roger
> >
> >Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> >Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

2019-06-10 06:47:11

by Pawel Laszczak

[permalink] [raw]
Subject: RE: [PATCH v7 1/6] dt-bindings: add binding for USBSS-DRD controller.

>
>> - reg-names - register memory area names:
>> "xhci" - for HOST registers space
>> "dev" - for DEVICE registers space
>> "otg" - for OTG/DRD registers space
>> - compatible: Should contain:
>> "cdns,usb3-1.0.0" - for 0x00024502 controller version
>> "cdns,usb3-1.0.1" - for 0x00024509 controller version
>> "cdns,usb3-1.0.2" - for 0x0002450C controller version
>> "cdns,usb3-1.0.3" - for 0x0002450d controller version
>
>If the value can be read from register, why we still need different compatibles?
We don't need it. I left it only as information that cdns3 controller exist in 4 different version.

>
>> Example:
>> usb@f3000000 {
>> compatible = "cdns,usb3-1.0.1";
>> interrupts = <GIC_USB_IRQ 7 IRQ_TYPE_LEVEL_HIGH>,
>> <GIC_USB_IRQ 7 IRQ_TYPE_LEVEL_HIGH>,
>> <GIC_USB_IRQ 8 IRQ_TYPE_LEVEL_HIGH>;
>> interrupt-names = "host", "peripheral", "otg";
>> reg = <0xf3000000 0x10000>, /* memory area for HOST registers */
>> <0xf3010000 0x10000>, /* memory area for DEVICE registers */
>> <0xf3020000 0x10000>; /* memory area for OTG/DRD registers
>> */
>> reg-names = "xhci", "dev", "otg";
>> phys = <&usb3_phy>;
>> phy-names = "cdns3,usbphy";
>>
>>
>> Maybe I should add also something for usb2_phy in dt-binding and in driver ?
>>
>
>The PHY driver is located at: drivers/phy/cadence/phy-cadence-sierra.c.
>Will we need another one for USB2 PHY?

As I know sierra phy is USB3.0 phy.
Also I know that we have torent phy driver for 2.0. It's ready but is has not yet in kernel.
So I assume that I should change
phys = <&usb3_phy>;
phy-names = "cdns3,usbphy";

with:
phys = <&usb2_phy>, <&usb3_phy>;
phy-names = "cdns3,usbphy2", "cdns3,usbphy3";

Pawel
>
>Peter
>
>
>> Thanks,
>> Regards,
>> Pawel
>>
>> >>
>> >> Regards,
>> >> Pawel
>> >>
>> >>
>> >>>
>> >>>> +
>> >>>> +Optional properties:
>> >>>> + - maximum-speed : valid arguments are "super-speed", "high-speed" and
>> >>>> + "full-speed"; refer to usb/generic.txt
>> >>>> + - dr_mode: Should be one of "host", "peripheral" or "otg".
>> >>>> + - phys: reference to the USB PHY
>> >>>> +
>> >>>> +Example:
>> >>>> + usb@f3000000 {
>> >>>> + compatible = "cdns,usb3-1.0.1";
>> >>>> + interrupts = <USB_IRQ 7 IRQ_TYPE_LEVEL_HIGH>;
>> >>>
>> >>> This example won't work as code requests for 3 separate interrupts.
>> >>>
>> >>>> + reg = <0xf3000000 0x10000 /* memory area for HOST
>> registers */
>> >>>> + 0xf3010000 0x10000 /* memory area for DEVICE
>> registers */
>> >>>> + 0xf3020000 0x10000>; /* memory area for
>> OTG/DRD registers */
>> >>>> + reg-names = "xhci", "dev", "otg";
>> >>>> + };
>> >>>>
>> >>>
>> >
>> >
>> >--
>> >cheers,
>> >-roger
>> >
>> >Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>> >Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

2019-06-10 06:53:41

by Peter Chen

[permalink] [raw]
Subject: RE: [PATCH v7 1/6] dt-bindings: add binding for USBSS-DRD controller.



> >> - reg-names - register memory area names:
> >> "xhci" - for HOST registers space
> >> "dev" - for DEVICE registers space
> >> "otg" - for OTG/DRD registers space
> >> - compatible: Should contain:
> >> "cdns,usb3-1.0.0" - for 0x00024502 controller version
> >> "cdns,usb3-1.0.1" - for 0x00024509 controller version
> >> "cdns,usb3-1.0.2" - for 0x0002450C controller version
> >> "cdns,usb3-1.0.3" - for 0x0002450d controller version
> >
> >If the value can be read from register, why we still need different compatibles?
> We don't need it. I left it only as information that cdns3 controller exist in 4 different
> version.
>

So, you will have only one compatible at both driver and binding doc, eg "cdns, usb3"?
And you may move the controller information into the driver, and have a MACRO for it?

> >> Maybe I should add also something for usb2_phy in dt-binding and in driver ?
> >>
> >
> >The PHY driver is located at: drivers/phy/cadence/phy-cadence-sierra.c.
> >Will we need another one for USB2 PHY?
>
> As I know sierra phy is USB3.0 phy.
> Also I know that we have torent phy driver for 2.0. It's ready but is has not yet in
> kernel.
> So I assume that I should change
> phys = <&usb3_phy>;
> phy-names = "cdns3,usbphy";
>
> with:
> phys = <&usb2_phy>, <&usb3_phy>;
> phy-names = "cdns3,usbphy2", "cdns3,usbphy3";
>

Yes, I think so.

Peter