| 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
| ||
|
Message-ID: <BYAPR07MB470965B3CBCB58E1C7B1253FDD130@BYAPR07MB4709.namprd07.prod.outlook.com>
Date: Mon, 10 Jun 2019 04:58:55 +0000
From: Pawel Laszczak <pawell@...ence.com>
To: Roger Quadros <rogerq@...com>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
CC: "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"felipe.balbi@...ux.intel.com" <felipe.balbi@...ux.intel.com>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"hdegoede@...hat.com" <hdegoede@...hat.com>,
"heikki.krogerus@...ux.intel.com" <heikki.krogerus@...ux.intel.com>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"jbergsagel@...com" <jbergsagel@...com>,
"nsekhar@...com" <nsekhar@...com>, "nm@...com" <nm@...com>,
Suresh Punnoose <sureshp@...ence.com>,
"peter.chen@....com" <peter.chen@....com>,
Jayshri Dajiram Pawar <jpawar@...ence.com>,
Rahul Kumar <kurahul@...ence.com>
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 <pawell@...ence.com>
>>>> Reviewed-by: Rob Herring <robh@...nel.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@...nel.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@...00000 {
>> 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@...00000 {
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@...00000 {
>>>> + 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
Powered by blists - more mailing lists