linux-kernel - RE: [upstream-release] [PATCH 1/2] drivers: usb: phy: Add qoriq usb 3.0 phy driver support

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]

Message-ID: <DB5PR0401MB1925E935B4E7F5D1D1E521D5F5BE0@DB5PR0401MB1925.eurprd04.prod.outlook.com>
Date:   Wed, 16 Nov 2016 11:33:11 +0000
From:   Sriram Dash <sriram.dash@....com>
To:     Scott Wood <scott.wood@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
CC:     "mark.rutland@....com" <mark.rutland@....com>,
        "felipe.balbi@...ux.intel.com" <felipe.balbi@...ux.intel.com>,
        "mathias.nyman@...el.com" <mathias.nyman@...el.com>,
        "catalin.marinas@....com" <catalin.marinas@....com>,
        "will.deacon@....com" <will.deacon@....com>,
        "kishon@...com" <kishon@...com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "stern@...land.harvard.edu" <stern@...land.harvard.edu>,
        Suresh Gupta <suresh.gupta@....com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "pku.leo@...il.com" <pku.leo@...il.com>
Subject: RE: [upstream-release] [PATCH 1/2] drivers: usb: phy: Add qoriq usb
 3.0    phy driver support

>From: Scott Wood
>On 11/15/2016 06:39 AM, Sriram Dash wrote:
>>> From: Scott Wood
>>> On 11/13/2016 11:27 PM, Sriram Dash wrote:
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
>>>> b/Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
>>>> new file mode 100644
>>>> index 0000000..d934c80
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
>>>> @@ -0,0 +1,36 @@
>>>> +Driver for Freescale USB 3.0 PHY
>>>> +
>>>> +Required properties:
>>>> +
>>>> +- compatible :	fsl,qoriq-usb3-phy
>>>
>>
>> Hi Scott,
>>
>>> This is a very vague compatible.  Are there versioning registers
>>> within this register block?
>>>
>>
>> There are versioning registers for the phy (1.0 and 1.1). But the
>> current erratum
>> A008751 does not require the mentioning of the version numbers. Was
>> planning to take care of the versioning when there is code diversity
>> on the basis of the version number.
>
>That is not how device tree bindings work.  The describe the hardware, not the
>driver.
>
>That said, is the block version sufficient to tell whether a given chip has this
>erratum?  If so, you don't need a special property for the erratum.  If not, what is
>different about the PHY that is not described by the versioning?
>
>In any case, it would be nice to mention the version register and its offset in the
>binding, just so that it becomes part of the definition of this compatible string, and
>if we come out with some QorIQ chip with a
>USB3 PHY that is totally different and doesn't have that version register, it'll be
>clear that it needs a different compatible.
>

Okay. Will include version number in the next rev for Documentation and dt.

>>>> +static inline u32 qoriq_usb3_phy_readl(void __iomem *addr, u32
>>>> +offset) {
>>>> +	return __raw_readl(addr + offset); }
>>>> +
>>>> +static inline void qoriq_usb3_phy_writel(void __iomem *addr, u32 offset,
>>>> +	u32 data)
>>>> +{
>>>> +	__raw_writel(data, addr + offset); }
>>>
>>> Why raw?  Besides missing barriers, this will cause the accesses to
>>> be native-endian which is not correct.
>>>
>>
>> The only reason for __raw_writel is to make the code faster.
>
>Does that really matter here?
>
>> However, shall I use writel(with both barriers and byte swap) instead
>
>Yes, if the registers are little-endian on all chips.
>

The endianness is not same for all Socs. But for most Socs, it is big-endian.
Is "iowrite32be" better instead? 

>> and then make appropriate changes in the value 32'h27672B2A?
>
>Not sure what you mean here.
>
>> In my knowledge, there are more than 5 errata in pipeline,
>
>Then please get all of these errata described in the device tree ASAP (unless their
>presence can be reliably inferred from the block version, as discussed above).
>

Yes. We will push the errata asap.

>> However, in future, if any other erratum comes up, and it has to be
>> applied at any point other than during init, then the variable has to
>> be added in qoriq_usb3_phy struct and the property has to be read separately.
>
>Or if the erratum is detected by some means other than a device tree property...
>

Yes. For any other case also, it will be handled differently.

>-Scott