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

[Pawel Laszczak <pawell@cadence.com>]

>>
>>>
>>> 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 <pawell@cadence.com>
>>>> Reviewed-by: Rob Herring <robh@kernel.org>
>>>>
>>>> ---
>>>>  .../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 <robh@kernel.org> 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