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

2016-11-16 Thread Scott Wood
On 11/16/2016 05:33 AM, Sriram Dash wrote:
>> 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 000..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?

Can you find out the answer to this?

>> 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.

I'm not asking you to put the version number in the dt if it can be read
from a register.  I'm asking you to have the binding describe the
version register, as part of the definition of what "fsl,qoriq-usb3-phy"
means.

>>> 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? 

Then the device tree node needs to say what endian it is, and you need
to choose the appropriate accessor at runtime.

But is it really big endian for most or even any SoCs?  This hardware
isn't present on our PPC chips, right (I checked a couple of the most
recent PPC RMs and it shows USB 2.0 only)?  So it'd just be the few ARM
chips that made almost everything big endian, and even there the couple
RMs I checked (ls1021a and ls1043a) have these registers described as
little-endian.

>>> 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.

My point is that the device tree node should be complete when you submit
it.  Don't wait for the implementation of a workaround to advertise the
existence of an erratum in the device tree.

-Scott

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2016-11-16 Thread Sriram Dash
>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 000..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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2016-11-16 Thread 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 000..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.

>>> +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.

> 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).

> 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...

-Scott

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


RE: [PATCH 1/2] drivers: usb: phy: Add qoriq usb 3.0 phy driver support

2016-11-16 Thread Sriram Dash
>From: Rob Herring [mailto:r...@kernel.org]
>On Mon, Nov 14, 2016 at 10:56:54AM +0530, Sriram Dash wrote:
>> Adds qoriq usb 3.0 phy driver support for LS1043A platform.
>> Describes the qoriq usb 2.0 phy driver binding, currently used for
>> LS1043A platform.
>>
>> Signed-off-by: Sriram Dash 
>> ---
>>  .../devicetree/bindings/phy/phy-qoriq-usb3.txt |  36 
>>  drivers/phy/Kconfig|   8 +
>>  drivers/phy/Makefile   |   1 +
>>  drivers/phy/phy-qoriq-usb3.c   | 202 
>> +
>>  4 files changed, 247 insertions(+)
>>  create mode 100644
>> Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
>>  create mode 100644 drivers/phy/phy-qoriq-usb3.c
>>
>> 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 000..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
>> +- reg : register mappings for Parameter Configuration Register
>> +and Phy base offset.
>> +- reg-names :   "param_ctrl" and "phy_base"
>> +- phy_type :For multi port host USB controllers, should be one of
>> +"ulpi", or "serial". For dual role USB controllers,
>> +should be one of "ulpi", "utmi", "utmi_wide", or "serial".

Hi Rob,

>
>Do any of these really apply to a USB3 PHY?
>

The concerned USB3 phy used is UTMI. I agree to your point somewhat that
all the types are not required now. Anyway, shall I make it an optional
property, with the mention of only UTMI and ULPI?

>Rob
>
>> +
>> +Example:
>> +usbphy0: usb3-phy@084F {
>
>usb-phy@...
>

Ok. Will change in the next rev for Documentation and dts (patch 2/2)

>> +compatible = "fsl,qoriq-usb3-phy";
>> +reg = <0x0 0x01570070 0x0 0xC>, <0x0 0x084F 0x0 
>> 0x5000>;
>> +reg-names = "param_ctrl", "phy_base";
>> +#phy-cells = <0>;
>> +phy_type = "utmi";
>> +};
>> +
>> +usbphy1: usb3-phy@0850 {
>> +compatible = "fsl,qoriq-usb3-phy";
>> +reg = <0x0 0x0157007C 0x0 0xC>, <0x0 0x0850 0x0 
>> 0x5000>;
>> +reg-names = "param_ctrl", "phy_base";
>> +#phy-cells = <0>;
>> +phy_type = "utmi";
>> +};
>> +
>> +usbphy2: usb3-phy@0851 {
>> +compatible = "fsl,qoriq-usb3-phy";
>> +reg = <0x0 0x01570088 0x0 0xC>, <0x0 0x0851 0x0 
>> 0x5000>;
>> +reg-names = "param_ctrl", "phy_base";
>> +#phy-cells = <0>;
>> +phy_type = "utmi";
>> +};
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/2] drivers: usb: phy: Add qoriq usb 3.0 phy driver support

2016-11-15 Thread Rob Herring
On Mon, Nov 14, 2016 at 10:56:54AM +0530, Sriram Dash wrote:
> Adds qoriq usb 3.0 phy driver support for LS1043A platform.
> Describes the qoriq usb 2.0 phy driver binding, currently used
> for LS1043A platform.
> 
> Signed-off-by: Sriram Dash 
> ---
>  .../devicetree/bindings/phy/phy-qoriq-usb3.txt |  36 
>  drivers/phy/Kconfig|   8 +
>  drivers/phy/Makefile   |   1 +
>  drivers/phy/phy-qoriq-usb3.c   | 202 
> +
>  4 files changed, 247 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
>  create mode 100644 drivers/phy/phy-qoriq-usb3.c
> 
> 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 000..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
> +- reg :  register mappings for Parameter Configuration Register
> + and Phy base offset.
> +- reg-names :"param_ctrl" and "phy_base"
> +- phy_type : For multi port host USB controllers, should be one of
> + "ulpi", or "serial". For dual role USB controllers,
> + should be one of "ulpi", "utmi", "utmi_wide", or "serial".

Do any of these really apply to a USB3 PHY?

Rob

> +
> +Example:
> + usbphy0: usb3-phy@084F {

usb-phy@...

> +compatible = "fsl,qoriq-usb3-phy";
> +reg = <0x0 0x01570070 0x0 0xC>, <0x0 0x084F 0x0 
> 0x5000>;
> +reg-names = "param_ctrl", "phy_base";
> +#phy-cells = <0>;
> +phy_type = "utmi";
> +};
> +
> +usbphy1: usb3-phy@0850 {
> +compatible = "fsl,qoriq-usb3-phy";
> +reg = <0x0 0x0157007C 0x0 0xC>, <0x0 0x0850 0x0 
> 0x5000>;
> +reg-names = "param_ctrl", "phy_base";
> +#phy-cells = <0>;
> +phy_type = "utmi";
> +};
> +
> +usbphy2: usb3-phy@0851 {
> +compatible = "fsl,qoriq-usb3-phy";
> +reg = <0x0 0x01570088 0x0 0xC>, <0x0 0x0851 0x0 
> 0x5000>;
> +reg-names = "param_ctrl", "phy_base";
> +#phy-cells = <0>;
> +phy_type = "utmi";
> +};
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2016-11-15 Thread Sriram Dash
>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 000..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.
Shall I include the versioning in the documentation now for 1.0 and 1.1?

>> +/* Parameter control */
>> +#define USB3PRM1CR  0x000
>> +#define USB3PRM1CR_VAL  0x27672b2a
>> +
>> +/*
>> + * struct qoriq_usb3_phy - driver data for USB 3.0 PHY
>> + * @dev: pointer to device instance of this platform device
>> + * @param_ctrl: usb3 phy parameter control register base
>> + * @phy_base: usb3 phy register memory base
>> + * @has_erratum_flag: keeps track of erratum applicable on device  */
>> +struct qoriq_usb3_phy {
>> +struct device *dev;
>> +void __iomem *param_ctrl;
>> +void __iomem *phy_base;
>> +u32 has_erratum_flag;
>> +};
>> +
>> +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. However, shall I 
use
writel(with both barriers and byte swap) instead and then make appropriate 
changes
in the value 32'h27672B2A?

>> +/*
>> + * Erratum A008751
>> + * SCFG USB3PRM1CR has incorrect default value
>> + * SCFG USB3PRM1CR reset value should be 32'h27672B2A instead of
>32'h25E72B2A.
>
>When documenting C code, can you stick with C-style numeric constants?
>
>For that matter, just put the constant in the code instead of hiding it in an 
>overly-
>generically-named USB3PRM1CR_VAL and then you won't need to redundantly
>state the value in a comment.  Normally putting magic numbers in symbolic
>constants is a good thing, but in this case it's not actually describing 
>anything and
>the number is of no meaning outside of this one erratum workaround (it might 
>even
>be a different value if another chip has a similar erratum).
>

The POR value of the USB3PRM1CR  should be 32'h27672B2A. So, I had named it as
such. Thanks for the info. Will remove the USB3PRM1CR_VAL and during writel,
will use 0x27672b2a directly. I will take care the next time.

>> + */
>> +static void erratum_a008751(struct qoriq_usb3_phy *phy) {
>> +qoriq_usb3_phy_writel(phy->param_ctrl, USB3PRM1CR,
>> +USB3PRM1CR_VAL);
>> +}
>> +
>> +/*
>> + * qoriq_usb3_phy_erratum - List of phy erratum
>> + * @qoriq_phy_erratum - erratum application
>> + * @compat - comapt string for erratum  */
>> +
>> +struct qoriq_usb3_phy_erratum {
>> +void (*qoriq_phy_erratum)(struct qoriq_usb3_phy *phy);
>> +char *compat;
>> +};
>> +
>> +/* Erratum list */
>> +struct qoriq_usb3_phy_erratum  phy_erratum_tbl[] = {
>> +{&erratum_a008751, "fsl,usb-erratum-a008751"},
>> +/* Add init time erratum here */
>> +};
>
>This needs to be static.
>

Ok. Will take care next time.

>Unnecessary & on the function pointer.
>

Ok. Will change in next version.

>> +static int qoriq_usb3_phy_init(struct phy *x) {
>> +struct qoriq_usb3_phy *phy = phy_get_drvdata(x);
>> +int i;
>> +
>> +for (i = 0; i < ARRAY_SIZE(phy_erratum_tbl); i++)
>> +if (phy->has_erratum_flag & 1 << i)
>> +phy_erratum_tbl[i].qoriq_phy_erratum(phy);
>> +return 0;
>> +}
>> +
>> +static const struct phy_ops ops = {
>> +.init   = qoriq_usb3_phy_init,
>> +.owner  = THIS_MODULE,
>> +};
>> +
>> +static int qoriq_usb3_phy_probe(struct platform_device *pdev) {
>> +struct qoriq_usb3_phy *phy;
>> +struct phy *generic_phy;
>> +struct phy_provider *phy_provider;
>> +const struct of_device_id *of_id;
>> +struct device *dev = &pdev->dev;
>> +struct resource *res;
>> +int i, ret;
>> +
>> +phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL);
>> +if (!phy)
>> +return -ENOMEM;
>> +phy->dev = dev;
>> +
>> +of_id = of_match_device(dev->driver->of_match_table, dev);
>> +if (!of_id) {
>> +dev_err(dev, "failed to get device match\n");
>> +ret = -EINVAL;
>> +goto err_out;
>> +}
>> +

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

2016-11-14 Thread 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 000..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

This is a very vague compatible.  Are there versioning registers within
this register block?

> +/* Parameter control */
> +#define USB3PRM1CR   0x000
> +#define USB3PRM1CR_VAL   0x27672b2a
> +
> +/*
> + * struct qoriq_usb3_phy - driver data for USB 3.0 PHY
> + * @dev: pointer to device instance of this platform device
> + * @param_ctrl: usb3 phy parameter control register base
> + * @phy_base: usb3 phy register memory base
> + * @has_erratum_flag: keeps track of erratum applicable on device
> + */
> +struct qoriq_usb3_phy {
> + struct device *dev;
> + void __iomem *param_ctrl;
> + void __iomem *phy_base;
> + u32 has_erratum_flag;
> +};
> +
> +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.

> +/*
> + * Erratum A008751
> + * SCFG USB3PRM1CR has incorrect default value
> + * SCFG USB3PRM1CR reset value should be 32'h27672B2A instead of 
> 32'h25E72B2A.

When documenting C code, can you stick with C-style numeric constants?

For that matter, just put the constant in the code instead of hiding it
in an overly-generically-named USB3PRM1CR_VAL and then you won't need to
redundantly state the value in a comment.  Normally putting magic
numbers in symbolic constants is a good thing, but in this case it's not
actually describing anything and the number is of no meaning outside of
this one erratum workaround (it might even be a different value if
another chip has a similar erratum).

> + */
> +static void erratum_a008751(struct qoriq_usb3_phy *phy)
> +{
> + qoriq_usb3_phy_writel(phy->param_ctrl, USB3PRM1CR,
> + USB3PRM1CR_VAL);
> +}
> +
> +/*
> + * qoriq_usb3_phy_erratum - List of phy erratum
> + * @qoriq_phy_erratum - erratum application
> + * @compat - comapt string for erratum
> + */
> +
> +struct qoriq_usb3_phy_erratum {
> + void (*qoriq_phy_erratum)(struct qoriq_usb3_phy *phy);
> + char *compat;
> +};
> +
> +/* Erratum list */
> +struct qoriq_usb3_phy_erratum  phy_erratum_tbl[] = {
> + {&erratum_a008751, "fsl,usb-erratum-a008751"},
> + /* Add init time erratum here */
> +};

This needs to be static.

Unnecessary & on the function pointer.

> +static int qoriq_usb3_phy_init(struct phy *x)
> +{
> + struct qoriq_usb3_phy *phy = phy_get_drvdata(x);
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(phy_erratum_tbl); i++)
> + if (phy->has_erratum_flag & 1 << i)
> + phy_erratum_tbl[i].qoriq_phy_erratum(phy);
> + return 0;
> +}
> +
> +static const struct phy_ops ops = {
> + .init   = qoriq_usb3_phy_init,
> + .owner  = THIS_MODULE,
> +};
> +
> +static int qoriq_usb3_phy_probe(struct platform_device *pdev)
> +{
> + struct qoriq_usb3_phy *phy;
> + struct phy *generic_phy;
> + struct phy_provider *phy_provider;
> + const struct of_device_id *of_id;
> + struct device *dev = &pdev->dev;
> + struct resource *res;
> + int i, ret;
> +
> + phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL);
> + if (!phy)
> + return -ENOMEM;
> + phy->dev = dev;
> +
> + of_id = of_match_device(dev->driver->of_match_table, dev);
> + if (!of_id) {
> + dev_err(dev, "failed to get device match\n");
> + ret = -EINVAL;
> + goto err_out;
> + }
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "param_ctrl");
> + if (!res) {
> + dev_err(dev, "failed to get param_ctrl memory\n");
> + ret = -ENOENT;
> + goto err_out;
> + }
> +
> + phy->param_ctrl = devm_ioremap_resource(dev, res);
> + if (!phy->param_ctrl) {
> + dev_err(dev, "failed to remap param_ctrl memory\n");
> + ret = -ENOMEM;
> + goto err_out;
> + }
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "phy_base");
> + if (!res) {
> + dev_err(dev, "failed to get phy_base memory\n");
> + ret = -ENOENT;
> + goto err_out;
> + }
> +
> + phy->phy_base = devm_ioremap_resource(dev, res);
> + if (!phy->phy_base) {
> + dev_err(dev, "failed to remap phy_base memory\n");
> + ret = -ENOMEM;
> + goto

[PATCH 1/2] drivers: usb: phy: Add qoriq usb 3.0 phy driver support

2016-11-13 Thread Sriram Dash
Adds qoriq usb 3.0 phy driver support for LS1043A platform.
Describes the qoriq usb 2.0 phy driver binding, currently used
for LS1043A platform.

Signed-off-by: Sriram Dash 
---
 .../devicetree/bindings/phy/phy-qoriq-usb3.txt |  36 
 drivers/phy/Kconfig|   8 +
 drivers/phy/Makefile   |   1 +
 drivers/phy/phy-qoriq-usb3.c   | 202 +
 4 files changed, 247 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/phy-qoriq-usb3.txt
 create mode 100644 drivers/phy/phy-qoriq-usb3.c

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 000..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
+- reg :register mappings for Parameter Configuration Register
+   and Phy base offset.
+- reg-names :  "param_ctrl" and "phy_base"
+- phy_type :   For multi port host USB controllers, should be one of
+   "ulpi", or "serial". For dual role USB controllers,
+   should be one of "ulpi", "utmi", "utmi_wide", or "serial".
+
+Example:
+   usbphy0: usb3-phy@084F {
+compatible = "fsl,qoriq-usb3-phy";
+reg = <0x0 0x01570070 0x0 0xC>, <0x0 0x084F 0x0 
0x5000>;
+reg-names = "param_ctrl", "phy_base";
+#phy-cells = <0>;
+phy_type = "utmi";
+};
+
+usbphy1: usb3-phy@0850 {
+compatible = "fsl,qoriq-usb3-phy";
+reg = <0x0 0x0157007C 0x0 0xC>, <0x0 0x0850 0x0 
0x5000>;
+reg-names = "param_ctrl", "phy_base";
+#phy-cells = <0>;
+phy_type = "utmi";
+};
+
+usbphy2: usb3-phy@0851 {
+compatible = "fsl,qoriq-usb3-phy";
+reg = <0x0 0x01570088 0x0 0xC>, <0x0 0x0851 0x0 
0x5000>;
+reg-names = "param_ctrl", "phy_base";
+#phy-cells = <0>;
+phy_type = "utmi";
+};
diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index fe00f91..4caa91c 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -489,4 +489,12 @@ config PHY_NS2_PCIE
help
  Enable this to support the Broadcom Northstar2 PCIe PHY.
  If unsure, say N.
+
+config PHY_QORIQ_USB3
+   tristate "Freescale QorIQ USB 3.0 PHY driver"
+   depends on ARCH_LAYERSCAPE
+   depends on OF
+   select GENERIC_PHY
+   help
+ Enable this to support the USB3.0 PHY on the QorIQ SoC.
 endmenu
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index a534cf5..a47ee36b 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -60,3 +60,4 @@ obj-$(CONFIG_PHY_PISTACHIO_USB)   += 
phy-pistachio-usb.o
 obj-$(CONFIG_PHY_CYGNUS_PCIE)  += phy-bcm-cygnus-pcie.o
 obj-$(CONFIG_ARCH_TEGRA) += tegra/
 obj-$(CONFIG_PHY_NS2_PCIE) += phy-bcm-ns2-pcie.o
+obj-$(CONFIG_PHY_QORIQ_USB3)+= phy-qoriq-usb3.o
diff --git a/drivers/phy/phy-qoriq-usb3.c b/drivers/phy/phy-qoriq-usb3.c
new file mode 100644
index 000..5255089
--- /dev/null
+++ b/drivers/phy/phy-qoriq-usb3.c
@@ -0,0 +1,202 @@
+/*
+ * Freescale QorIQ USB3 phy driver
+ *
+ * Copyright 2016 Freescale Semiconductor, Inc.
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * Author: Sriram Dash 
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+
+/* Parameter control */
+#define USB3PRM1CR 0x000
+#define USB3PRM1CR_VAL 0x27672b2a
+
+/*
+ * struct qoriq_usb3_phy - driver data for USB 3.0 PHY
+ * @dev: pointer to device instance of this platform device
+ * @param_ctrl: usb3 phy parameter control register base
+ * @phy_base: usb3 phy register memory base
+ * @has_erratum_flag: keeps track of erratum applicable on device
+ */
+struct qoriq_usb3_phy {
+   struct device *dev;
+   void __iomem *param_ctrl;
+   void __iomem *phy_base;
+   u32 has_erratum_flag;
+};
+
+static i