From: Sriram Dash <sriram.dash@nxp.com>
To: Scott Wood <scott.wood@nxp.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>
CC: "mark.rutland@arm.com" <mark.rutland@arm.com>,
        "felipe.balbi@linux.intel.com" <felipe.balbi@linux.intel.com>,
        "mathias.nyman@intel.com" <mathias.nyman@intel.com>,
        "catalin.marinas@arm.com" <catalin.marinas@arm.com>,
        "will.deacon@arm.com" <will.deacon@arm.com>,
        "kishon@ti.com" <kishon@ti.com>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "stern@rowland.harvard.edu" <stern@rowland.harvard.edu>,
        Suresh Gupta <suresh.gupta@nxp.com>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "pku.leo@gmail.com" <pku.leo@gmail.com>
Subject: RE: [upstream-release] [PATCH 1/2] drivers: usb: phy: Add qoriq usb
 3.0    phy driver support
Thread-Topic: [upstream-release] [PATCH 1/2] drivers: usb: phy: Add qoriq usb
 3.0    phy driver support
Thread-Index: AQHSPpFDCWlUkKh88UW4QNNVDxolJKDbYJJw
Date: Wed, 16 Nov 2016 11:33:11 +0000
Message-ID: <DB5PR0401MB1925E935B4E7F5D1D1E521D5F5BE0@DB5PR0401MB1925.eurprd04.prod.outlook.com>
References: <1479101215-26954-1-git-send-email-sriram.dash@nxp.com>
 <1479101215-26954-2-git-send-email-sriram.dash@nxp.com>
 <DB5PR0401MB1928CA058CC01243457F124791BC0@DB5PR0401MB1928.eurprd04.prod.outlook.com>
 <DB5PR0401MB1925FB1FE45C15B0671E46D1F5BF0@DB5PR0401MB1925.eurprd04.prod.outlook.com>
 <HE1PR0401MB1931C92287A8E92BC6B52B1991BE0@HE1PR0401MB1931.eurprd04.prod.outlook.com>
In-Reply-To: <HE1PR0401MB1931C92287A8E92BC6B52B1991BE0@HE1PR0401MB1931.eurprd04.prod.outlook.com>
Accept-Language: en-US
Content-Language: en-US
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Nov 2016 11:33:11.0896
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB5PR0401MB1928
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id uAGC6p1B002232
Content-Length: 3219
Lines: 95

>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