[3/7] usb: dwc3: pci: Store device properties dynamically

[3/7] usb: dwc3: pci: Store device properties dynamically

[Andy Shevchenko]

On Thu, Feb 22, 2018 at 1:57 AM, Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> On 2/21/2018 6:46 AM, Andy Shevchenko wrote:
>> On Tue, Feb 20, 2018 at 11:12 PM, Thinh Nguyen
>> <Thinh.Nguyen@synopsys.com> wrote:
>>> On 2/17/2018 7:29 AM, Andy Shevchenko wrote:
>>>> On Fri, Feb 16, 2018 at 11:55 PM, Thinh Nguyen
>>>> <Thinh.Nguyen@synopsys.com> wrote:
>>>>> Add the ability to add device properties dynamically. Currently, device
>>>>> properties are added to platform device using
>>>>> platform_device_add_properties(). However, this function does not allow
>>>>> adding properties incrementally. It is useful to have this ability when
>>>>> the driver needs to set common device properties across different HW
>>>>
>>>> I'm not sure it's useful anyhow.
>>>>
>>>>> or
>>>>> if IP and FPGA validation test different configurations for different
>>>>> HW.
>>>>
>>>> Shouldn't be a separate stuff for FPGA exclusively?
>>>
>>> Can you clarify/expand your question?
>>
>> FPGA is the one which might have different properties at run time for
>> the same device.
>> So, why do we care on driver / generic level of it?
>>
>> Shouldn't be FPGA manager take care of it (via DT overlays, for example)?
>
> Normally it is handled using DT overlays. But for using FPGA as PCI
> device, I'm not aware of a better solution.

If it's a PCI device, it may utilize PCI (hot plug if you would like
to change IP at run time) with appropriate IDs and stuff, right?

>>>>> Introduce two new functions to do so:
>>>>>    * dwc3_pci_add_one_property() - this function adds one property to
>>>>>      dwc->properties array and increase its size as needed
>>>>>    * dwc3_pci_add_properties() - this function takes a null terminated
>>>>>      array of device properties and add them to dwc->properties
>>>>
>>>> So, why you can't use ACPI / DT here?

>>> dwc3_pci_add_properties() is a convenient function that takes statically
>>> allocated array of (quirks) properties for different HW and store them
>>> to dwc->properties. The idea is to add more properties on top of these
>>> required quirks.
>>
>> Yes, I understand that. What's wrong with DT? The built-in device
>> properties have the same nature as usual properties for DT.
>> Whenever driver calls device_property_read_uXX() or alike it would
>> check all property provides for asked one.
>
> I may not have explained fully in my commit message the purpose of this
> change. That's why I think I misinterpreted your previous question.

> With this new debugging feature, we want to delay adding device
> properties until the user initiates it.

So, see above.
When user wants to enable this IP at run time it will be a PCI hot
plug event which makes device appear behind PCI switch.
When device appears it would have it's own VndrID/DevID + sub pair.

Based on that IDs you may apply hard coded quirks (though I am against
quirks in new hardware) as it's done right now.

> Because the driver cannot use
> platform_device_add_properties() to incrementally add properties to
> platform device, we need a way to store properties so they can be added
> in later time.

So what? You don't need that if you do the right things right.

> That's why I added these 2 new functions.
>
>> Other than that, quirks esp. for FPGA sounds so wrong. Why in the
>> first place not to make non-broken hardware?!
>
> I may have used the term 'quirk' incorrectly since they were placed in
> dwc3_pci_quirk(). There's no quirk for FPGA, but there are some initial
> setup properties for FPGA. To be specific, these properties:
>      PROPERTY_ENTRY_BOOL("snps,usb3_lpm_capable"),
>      PROPERTY_ENTRY_BOOL("snps,has-lpm-erratum"),
>      PROPERTY_ENTRY_BOOL("snps,dis_enblslpm_quirk"),
>      PROPERTY_ENTRY_BOOL("linux,sysdev_is_parent"),

OK.

[3/7] usb: dwc3: pci: Store device properties dynamically

[John Youn]

On 03/01/2018 12:25 AM, Felipe Balbi wrote:
> 
> Hi,
> 
> John Youn <John.Youn@synopsys.com> writes:
>> On 02/22/2018 07:20 AM, Andy Shevchenko wrote:
>>> On Thu, Feb 22, 2018 at 1:57 AM, Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>>>> On 2/21/2018 6:46 AM, Andy Shevchenko wrote:
>>>>> On Tue, Feb 20, 2018 at 11:12 PM, Thinh Nguyen
>>>>> <Thinh.Nguyen@synopsys.com> wrote:
>>>>>> On 2/17/2018 7:29 AM, Andy Shevchenko wrote:
>>>>>>> On Fri, Feb 16, 2018 at 11:55 PM, Thinh Nguyen
>>>>>>> <Thinh.Nguyen@synopsys.com> wrote:
>>>>>>>> Add the ability to add device properties dynamically. Currently, device
>>>>>>>> properties are added to platform device using
>>>>>>>> platform_device_add_properties(). However, this function does not allow
>>>>>>>> adding properties incrementally. It is useful to have this ability when
>>>>>>>> the driver needs to set common device properties across different HW
>>>>>>>
>>>>>>> I'm not sure it's useful anyhow.
>>>>>>>
>>>>>>>> or
>>>>>>>> if IP and FPGA validation test different configurations for different
>>>>>>>> HW.
>>>>>>>
>>>>>>> Shouldn't be a separate stuff for FPGA exclusively?
>>>>>>
>>>>>> Can you clarify/expand your question?
>>>>>
>>>>> FPGA is the one which might have different properties at run time for
>>>>> the same device.
>>>>> So, why do we care on driver / generic level of it?
>>>>>
>>>>> Shouldn't be FPGA manager take care of it (via DT overlays, for example)?
>>>>
>>>> Normally it is handled using DT overlays. But for using FPGA as PCI
>>>> device, I'm not aware of a better solution.
>>>
>>> If it's a PCI device, it may utilize PCI (hot plug if you would like
>>> to change IP at run time) with appropriate IDs and stuff, right?
>>>
>>>>>>>> Introduce two new functions to do so:
>>>>>>>>      * dwc3_pci_add_one_property() - this function adds one property to
>>>>>>>>        dwc->properties array and increase its size as needed
>>>>>>>>      * dwc3_pci_add_properties() - this function takes a null terminated
>>>>>>>>        array of device properties and add them to dwc->properties
>>>>>>>
>>>>>>> So, why you can't use ACPI / DT here?
>>>
>>>>>> dwc3_pci_add_properties() is a convenient function that takes statically
>>>>>> allocated array of (quirks) properties for different HW and store them
>>>>>> to dwc->properties. The idea is to add more properties on top of these
>>>>>> required quirks.
>>>>>
>>>>> Yes, I understand that. What's wrong with DT? The built-in device
>>>>> properties have the same nature as usual properties for DT.
>>>>> Whenever driver calls device_property_read_uXX() or alike it would
>>>>> check all property provides for asked one.
>>>>
>>>> I may not have explained fully in my commit message the purpose of this
>>>> change. That's why I think I misinterpreted your previous question.
>>>
>>>> With this new debugging feature, we want to delay adding device
>>>> properties until the user initiates it.
>>>
>>> So, see above.
>>> When user wants to enable this IP at run time it will be a PCI hot
>>> plug event which makes device appear behind PCI switch.
>>> When device appears it would have it's own VndrID/DevID + sub pair.
>>>
>>> Based on that IDs you may apply hard coded quirks (though I am against
>>> quirks in new hardware) as it's done right now.
>>>
>>
>> Hi Andy,
>>
>> Using VID/PID is not really feasible for our use case. If we only had
>> a few concrete devices then it would be ok.
> 
> Agreed. VID/PID would not scale at all :-)
> 
>> But understand that this an IP running on an FPGA validation
>> platform. The IP is very large and flexible with *many* parameters
>> that we must test against.  It is also deployed by our customers with
>> widely varying configurations. So we are constantly testing these as
>> well.
>>
>> One of the last pieces for moving our FPGA validation completely to
>> the in-kernel Linux stack is the ability to configure the driver to
>> set parameters that are not visible to the driver, or with parameters
>> that we want to constrain for testing.
> 
> I'm very much interested in helping you guys validate your IP with
> upstream Linux :-) My concern with this patch is that it makes dwc3-pci
> super complex even for users who are not interested in IP validation.
> 
> So, instead, how about we introduce dwc3-snps.c where you guys can put
> all the controls you need? We remove HAPS from current dwc3-pci.c and
> move everything to dwc3-snps.c. Sure, we'd have a little duplication,
> but I guess that'd be very minor.
> 
> While doing that, we can make dwc3-snps.c depend on EXPERT, so that
> distros don't enable it by default.
> 

Sure, we'll make the changes. I seem to recall we came to this same
conclusion a while back... forgot about that!

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

[3/7] usb: dwc3: pci: Store device properties dynamically

[Felipe Balbi]

Hi,

John Youn <John.Youn@synopsys.com> writes:
> On 02/22/2018 07:20 AM, Andy Shevchenko wrote:
>> On Thu, Feb 22, 2018 at 1:57 AM, Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>>> On 2/21/2018 6:46 AM, Andy Shevchenko wrote:
>>>> On Tue, Feb 20, 2018 at 11:12 PM, Thinh Nguyen
>>>> <Thinh.Nguyen@synopsys.com> wrote:
>>>>> On 2/17/2018 7:29 AM, Andy Shevchenko wrote:
>>>>>> On Fri, Feb 16, 2018 at 11:55 PM, Thinh Nguyen
>>>>>> <Thinh.Nguyen@synopsys.com> wrote:
>>>>>>> Add the ability to add device properties dynamically. Currently, device
>>>>>>> properties are added to platform device using
>>>>>>> platform_device_add_properties(). However, this function does not allow
>>>>>>> adding properties incrementally. It is useful to have this ability when
>>>>>>> the driver needs to set common device properties across different HW
>>>>>>
>>>>>> I'm not sure it's useful anyhow.
>>>>>>
>>>>>>> or
>>>>>>> if IP and FPGA validation test different configurations for different
>>>>>>> HW.
>>>>>>
>>>>>> Shouldn't be a separate stuff for FPGA exclusively?
>>>>>
>>>>> Can you clarify/expand your question?
>>>>
>>>> FPGA is the one which might have different properties at run time for
>>>> the same device.
>>>> So, why do we care on driver / generic level of it?
>>>>
>>>> Shouldn't be FPGA manager take care of it (via DT overlays, for example)?
>>>
>>> Normally it is handled using DT overlays. But for using FPGA as PCI
>>> device, I'm not aware of a better solution.
>> 
>> If it's a PCI device, it may utilize PCI (hot plug if you would like
>> to change IP at run time) with appropriate IDs and stuff, right?
>> 
>>>>>>> Introduce two new functions to do so:
>>>>>>>     * dwc3_pci_add_one_property() - this function adds one property to
>>>>>>>       dwc->properties array and increase its size as needed
>>>>>>>     * dwc3_pci_add_properties() - this function takes a null terminated
>>>>>>>       array of device properties and add them to dwc->properties
>>>>>>
>>>>>> So, why you can't use ACPI / DT here?
>> 
>>>>> dwc3_pci_add_properties() is a convenient function that takes statically
>>>>> allocated array of (quirks) properties for different HW and store them
>>>>> to dwc->properties. The idea is to add more properties on top of these
>>>>> required quirks.
>>>>
>>>> Yes, I understand that. What's wrong with DT? The built-in device
>>>> properties have the same nature as usual properties for DT.
>>>> Whenever driver calls device_property_read_uXX() or alike it would
>>>> check all property provides for asked one.
>>>
>>> I may not have explained fully in my commit message the purpose of this
>>> change. That's why I think I misinterpreted your previous question.
>> 
>>> With this new debugging feature, we want to delay adding device
>>> properties until the user initiates it.
>> 
>> So, see above.
>> When user wants to enable this IP at run time it will be a PCI hot
>> plug event which makes device appear behind PCI switch.
>> When device appears it would have it's own VndrID/DevID + sub pair.
>> 
>> Based on that IDs you may apply hard coded quirks (though I am against
>> quirks in new hardware) as it's done right now.
>> 
>
> Hi Andy,
>
> Using VID/PID is not really feasible for our use case. If we only had
> a few concrete devices then it would be ok.

Agreed. VID/PID would not scale at all :-)

> But understand that this an IP running on an FPGA validation
> platform. The IP is very large and flexible with *many* parameters
> that we must test against.  It is also deployed by our customers with
> widely varying configurations. So we are constantly testing these as
> well.
>
> One of the last pieces for moving our FPGA validation completely to
> the in-kernel Linux stack is the ability to configure the driver to
> set parameters that are not visible to the driver, or with parameters
> that we want to constrain for testing.

I'm very much interested in helping you guys validate your IP with
upstream Linux :-) My concern with this patch is that it makes dwc3-pci
super complex even for users who are not interested in IP validation.

So, instead, how about we introduce dwc3-snps.c where you guys can put
all the controls you need? We remove HAPS from current dwc3-pci.c and
move everything to dwc3-snps.c. Sure, we'd have a little duplication,
but I guess that'd be very minor.

While doing that, we can make dwc3-snps.c depend on EXPERT, so that
distros don't enable it by default.

I don't mind adding a bunch of properties to dwc3 core as long as it has
an actual use. I guess maintaining that is far from being a problem,
however I don't want to have impacts on actual products since this is
only necessary for validation activities :-)

cheers

[3/7] usb: dwc3: pci: Store device properties dynamically

[John Youn]

On 02/22/2018 07:20 AM, Andy Shevchenko wrote:
> On Thu, Feb 22, 2018 at 1:57 AM, Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>> On 2/21/2018 6:46 AM, Andy Shevchenko wrote:
>>> On Tue, Feb 20, 2018 at 11:12 PM, Thinh Nguyen
>>> <Thinh.Nguyen@synopsys.com> wrote:
>>>> On 2/17/2018 7:29 AM, Andy Shevchenko wrote:
>>>>> On Fri, Feb 16, 2018 at 11:55 PM, Thinh Nguyen
>>>>> <Thinh.Nguyen@synopsys.com> wrote:
>>>>>> Add the ability to add device properties dynamically. Currently, device
>>>>>> properties are added to platform device using
>>>>>> platform_device_add_properties(). However, this function does not allow
>>>>>> adding properties incrementally. It is useful to have this ability when
>>>>>> the driver needs to set common device properties across different HW
>>>>>
>>>>> I'm not sure it's useful anyhow.
>>>>>
>>>>>> or
>>>>>> if IP and FPGA validation test different configurations for different
>>>>>> HW.
>>>>>
>>>>> Shouldn't be a separate stuff for FPGA exclusively?
>>>>
>>>> Can you clarify/expand your question?
>>>
>>> FPGA is the one which might have different properties at run time for
>>> the same device.
>>> So, why do we care on driver / generic level of it?
>>>
>>> Shouldn't be FPGA manager take care of it (via DT overlays, for example)?
>>
>> Normally it is handled using DT overlays. But for using FPGA as PCI
>> device, I'm not aware of a better solution.
> 
> If it's a PCI device, it may utilize PCI (hot plug if you would like
> to change IP at run time) with appropriate IDs and stuff, right?
> 
>>>>>> Introduce two new functions to do so:
>>>>>>     * dwc3_pci_add_one_property() - this function adds one property to
>>>>>>       dwc->properties array and increase its size as needed
>>>>>>     * dwc3_pci_add_properties() - this function takes a null terminated
>>>>>>       array of device properties and add them to dwc->properties
>>>>>
>>>>> So, why you can't use ACPI / DT here?
> 
>>>> dwc3_pci_add_properties() is a convenient function that takes statically
>>>> allocated array of (quirks) properties for different HW and store them
>>>> to dwc->properties. The idea is to add more properties on top of these
>>>> required quirks.
>>>
>>> Yes, I understand that. What's wrong with DT? The built-in device
>>> properties have the same nature as usual properties for DT.
>>> Whenever driver calls device_property_read_uXX() or alike it would
>>> check all property provides for asked one.
>>
>> I may not have explained fully in my commit message the purpose of this
>> change. That's why I think I misinterpreted your previous question.
> 
>> With this new debugging feature, we want to delay adding device
>> properties until the user initiates it.
> 
> So, see above.
> When user wants to enable this IP at run time it will be a PCI hot
> plug event which makes device appear behind PCI switch.
> When device appears it would have it's own VndrID/DevID + sub pair.
> 
> Based on that IDs you may apply hard coded quirks (though I am against
> quirks in new hardware) as it's done right now.
> 

Hi Andy,

Using VID/PID is not really feasible for our use case. If we only had
a few concrete devices then it would be ok.

But understand that this an IP running on an FPGA validation
platform. The IP is very large and flexible with *many* parameters
that we must test against.  It is also deployed by our customers with
widely varying configurations. So we are constantly testing these as
well.

One of the last pieces for moving our FPGA validation completely to
the in-kernel Linux stack is the ability to configure the driver to
set parameters that are not visible to the driver, or with parameters
that we want to constrain for testing.

For example, consider the maximum_speed property. We could create a
PID for each speed. But what if we want test a configuration with
maximum_speed in combination with some other parameters, say nump,
burst size, fifo size, fifo thresholding, etc.

The amount of PIDs we would need for our validation would get out of
hand quickly and would be a pain to maintain. And it wouldn't help for
testing with fine variations, like increasing the thresholding value
by 1.

The other part of this is that we don't necessarily need an entirely
new device to perform all testing with different parameters. So we
wouldn't even have an opportunity to change the VID/PID.

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

[3/7] usb: dwc3: pci: Store device properties dynamically

[Thinh Nguyen]

Hi Andy,

On 2/21/2018 6:46 AM, Andy Shevchenko wrote:
> On Tue, Feb 20, 2018 at 11:12 PM, Thinh Nguyen
> <Thinh.Nguyen@synopsys.com> wrote:
>> On 2/17/2018 7:29 AM, Andy Shevchenko wrote:
>>> On Fri, Feb 16, 2018 at 11:55 PM, Thinh Nguyen
>>> <Thinh.Nguyen@synopsys.com> wrote:
>>>> Add the ability to add device properties dynamically. Currently, device
>>>> properties are added to platform device using
>>>> platform_device_add_properties(). However, this function does not allow
>>>> adding properties incrementally. It is useful to have this ability when
>>>> the driver needs to set common device properties across different HW
>>>
>>> I'm not sure it's useful anyhow.
>>>
>>>> or
>>>> if IP and FPGA validation test different configurations for different
>>>> HW.
>>>
>>> Shouldn't be a separate stuff for FPGA exclusively?
>>
>> Can you clarify/expand your question?
> 
> FPGA is the one which might have different properties at run time for
> the same device.
> So, why do we care on driver / generic level of it?
> 
> Shouldn't be FPGA manager take care of it (via DT overlays, for example)?

Normally it is handled using DT overlays. But for using FPGA as PCI 
device, I'm not aware of a better solution.

> 
>>>> To address this issue, update dwc3_pci to store device properties to
>>>> an array and dynamically manage them here.
>>>>
>>>> Introduce two new functions to do so:
>>>>    * dwc3_pci_add_one_property() - this function adds one property to
>>>>      dwc->properties array and increase its size as needed
>>>>    * dwc3_pci_add_properties() - this function takes a null terminated
>>>>      array of device properties and add them to dwc->properties
>>>
>>> So, why you can't use ACPI / DT here?
>>>
>>
>> dwc3_pci_add_properties() is a convenient function that takes statically
>> allocated array of (quirks) properties for different HW and store them
>> to dwc->properties. The idea is to add more properties on top of these
>> required quirks.
> 
> Yes, I understand that. What's wrong with DT? The built-in device
> properties have the same nature as usual properties for DT.
> Whenever driver calls device_property_read_uXX() or alike it would
> check all property provides for asked one.

I may not have explained fully in my commit message the purpose of this 
change. That's why I think I misinterpreted your previous question.

With this new debugging feature, we want to delay adding device 
properties until the user initiates it. Because the driver cannot use 
platform_device_add_properties() to incrementally add properties to 
platform device, we need a way to store properties so they can be added 
in later time. That's why I added these 2 new functions.

> Other than that, quirks esp. for FPGA sounds so wrong. Why in the
> first place not to make non-broken hardware?!

I may have used the term 'quirk' incorrectly since they were placed in 
dwc3_pci_quirk(). There's no quirk for FPGA, but there are some initial 
setup properties for FPGA. To be specific, these properties:
     PROPERTY_ENTRY_BOOL("snps,usb3_lpm_capable"),
     PROPERTY_ENTRY_BOOL("snps,has-lpm-erratum"),
     PROPERTY_ENTRY_BOOL("snps,dis_enblslpm_quirk"),
     PROPERTY_ENTRY_BOOL("linux,sysdev_is_parent"),

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

[3/7] usb: dwc3: pci: Store device properties dynamically

[Andy Shevchenko]

On Tue, Feb 20, 2018 at 11:12 PM, Thinh Nguyen
<Thinh.Nguyen@synopsys.com> wrote:
> On 2/17/2018 7:29 AM, Andy Shevchenko wrote:
>> On Fri, Feb 16, 2018 at 11:55 PM, Thinh Nguyen
>> <Thinh.Nguyen@synopsys.com> wrote:
>>> Add the ability to add device properties dynamically. Currently, device
>>> properties are added to platform device using
>>> platform_device_add_properties(). However, this function does not allow
>>> adding properties incrementally. It is useful to have this ability when
>>> the driver needs to set common device properties across different HW
>>
>> I'm not sure it's useful anyhow.
>>
>>> or
>>> if IP and FPGA validation test different configurations for different
>>> HW.
>>
>> Shouldn't be a separate stuff for FPGA exclusively?
>
> Can you clarify/expand your question?

FPGA is the one which might have different properties at run time for
the same device.
So, why do we care on driver / generic level of it?

Shouldn't be FPGA manager take care of it (via DT overlays, for example)?

>>> To address this issue, update dwc3_pci to store device properties to
>>> an array and dynamically manage them here.
>>>
>>> Introduce two new functions to do so:
>>>   * dwc3_pci_add_one_property() - this function adds one property to
>>>     dwc->properties array and increase its size as needed
>>>   * dwc3_pci_add_properties() - this function takes a null terminated
>>>     array of device properties and add them to dwc->properties
>>
>> So, why you can't use ACPI / DT here?
>>
>
> dwc3_pci_add_properties() is a convenient function that takes statically
> allocated array of (quirks) properties for different HW and store them
> to dwc->properties. The idea is to add more properties on top of these
> required quirks.

Yes, I understand that. What's wrong with DT? The built-in device
properties have the same nature as usual properties for DT.
Whenever driver calls device_property_read_uXX() or alike it would
check all property provides for asked one.

Other than that, quirks esp. for FPGA sounds so wrong. Why in the
first place not to make non-broken hardware?!

[3/7] usb: dwc3: pci: Store device properties dynamically

[Thinh Nguyen]

On 2/17/2018 7:29 AM, Andy Shevchenko wrote:
> On Fri, Feb 16, 2018 at 11:55 PM, Thinh Nguyen
> <Thinh.Nguyen@synopsys.com> wrote:
>> Add the ability to add device properties dynamically. Currently, device
>> properties are added to platform device using
>> platform_device_add_properties(). However, this function does not allow
>> adding properties incrementally. It is useful to have this ability when
>> the driver needs to set common device properties across different HW
> 
> I'm not sure it's useful anyhow.
> 
>> or
>> if IP and FPGA validation test different configurations for different
>> HW.
> 
> Shouldn't be a separate stuff for FPGA exclusively?

Can you clarify/expand your question?

> 
>> To address this issue, update dwc3_pci to store device properties to
>> an array and dynamically manage them here.
>>
>> Introduce two new functions to do so:
>>   * dwc3_pci_add_one_property() - this function adds one property to
>>     dwc->properties array and increase its size as needed
>>   * dwc3_pci_add_properties() - this function takes a null terminated
>>     array of device properties and add them to dwc->properties
> 
> So, why you can't use ACPI / DT here?
> 

dwc3_pci_add_properties() is a convenient function that takes statically 
allocated array of (quirks) properties for different HW and store them 
to dwc->properties. The idea is to add more properties on top of these 
required quirks.

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

[3/7] usb: dwc3: pci: Store device properties dynamically

[Thinh Nguyen]

Add the ability to add device properties dynamically. Currently, device
properties are added to platform device using
platform_device_add_properties(). However, this function does not allow
adding properties incrementally. It is useful to have this ability when
the driver needs to set common device properties across different HW or
if IP and FPGA validation test different configurations for different
HW. To address this issue, update dwc3_pci to store device properties to
an array and dynamically manage them here.

Introduce two new functions to do so:
 * dwc3_pci_add_one_property() - this function adds one property to
   dwc->properties array and increase its size as needed
 * dwc3_pci_add_properties() - this function takes a null terminated
   array of device properties and add them to dwc->properties

Now platform_device_add_properties() can simply use dwc->properties for
all the selected properties to add to a platform device.

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
 drivers/usb/dwc3/dwc3-pci.c | 107 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 98 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
index 11b1eb1e2cda..bab8c6c1da38 100644
--- a/drivers/usb/dwc3/dwc3-pci.c
+++ b/drivers/usb/dwc3/dwc3-pci.c
@@ -40,12 +40,16 @@
 #define PCI_INTEL_BXT_STATE_D0		0
 #define PCI_INTEL_BXT_STATE_D3		3
 
+#define PROPERTY_ARRAY_INITIAL_SIZE	32
+
 /**
  * struct dwc3_pci - Driver private structure
  * @dwc3: child dwc3 platform_device
  * @pci: our link to PCI bus
  * @guid: _DSM GUID
  * @has_dsm_for_pm: true for devices which need to run _DSM on runtime PM
+ * @properties: null terminated array of device properties
+ * @property_array_size: property array size
  */
 struct dwc3_pci {
 	struct platform_device *dwc3;
@@ -55,6 +59,9 @@ struct dwc3_pci {
 
 	unsigned int has_dsm_for_pm:1;
 	struct work_struct wakeup_work;
+
+	struct property_entry *properties;
+	int property_array_size;
 };
 
 static const struct acpi_gpio_params reset_gpios = { 0, 0, false };
@@ -66,10 +73,76 @@ static const struct acpi_gpio_mapping acpi_dwc3_byt_gpios[] = {
 	{ },
 };
 
+/**
+ * dwc3_pci_add_one_property - Add one device property to dwc->properties
+ * @dwc: dwc3 pci private structure
+ * @property: device property
+ *
+ * Returns 0 on success otherwise negative errno.
+ */
+static int dwc3_pci_add_one_property(struct dwc3_pci *dwc,
+				     struct property_entry property)
+{
+	int idx = 0;
+	int count;
+
+	while (dwc->properties[idx].name)
+		idx++;
+
+	/* Account for null terminate element in array */
+	count = idx + 1;
+
+	/* Increase the array size if not large enough */
+	if (count == dwc->property_array_size) {
+		struct property_entry *tmp;
+
+		tmp = kcalloc(dwc->property_array_size * 2,
+			      sizeof(*tmp), GFP_KERNEL);
+		if (!tmp)
+			return -ENOMEM;
+
+		memcpy(tmp, dwc->properties,
+		       dwc->property_array_size * sizeof(*tmp));
+
+		kfree(dwc->properties);
+		dwc->properties = tmp;
+		dwc->property_array_size *= 2;
+	}
+
+	dwc->properties[idx] = property;
+
+	return 0;
+}
+
+/**
+ * dwc3_pci_add_properties - Add multiple properties to dwc->properties
+ * @dwc: dwc3 pci private structure
+ * @properties: null terminated device property array
+ *
+ * Returns 0 on success otherwise negative errno.
+ */
+static int dwc3_pci_add_properties(struct dwc3_pci *dwc,
+				   struct property_entry *properties)
+{
+	int ret;
+	int idx;
+
+	if (!properties)
+		return -EINVAL;
+
+	for (idx = 0; properties[idx].name; idx++) {
+		ret = dwc3_pci_add_one_property(dwc, properties[idx]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int dwc3_pci_quirks(struct dwc3_pci *dwc)
 {
-	struct platform_device		*dwc3 = dwc->dwc3;
 	struct pci_dev			*pdev = dwc->pci;
+	int				ret;
 
 	if (pdev->vendor == PCI_VENDOR_ID_AMD &&
 	    pdev->device == PCI_DEVICE_ID_AMD_NL_USB) {
@@ -96,22 +169,18 @@ static int dwc3_pci_quirks(struct dwc3_pci *dwc)
 			{ },
 		};
 
-		return platform_device_add_properties(dwc3, properties);
+		ret = dwc3_pci_add_properties(dwc, properties);
+		if (ret)
+			return ret;
 	}
 
 	if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
-		int ret;
-
 		struct property_entry properties[] = {
 			PROPERTY_ENTRY_STRING("dr_mode", "peripheral"),
 			PROPERTY_ENTRY_BOOL("linux,sysdev_is_parent"),
 			{ }
 		};
 
-		ret = platform_device_add_properties(dwc3, properties);
-		if (ret < 0)
-			return ret;
-
 		if (pdev->device == PCI_DEVICE_ID_INTEL_BXT ||
 				pdev->device == PCI_DEVICE_ID_INTEL_BXT_M) {
 			guid_parse(PCI_INTEL_BXT_DSM_GUID, &dwc->guid);
@@ -148,6 +217,10 @@ static int dwc3_pci_quirks(struct dwc3_pci *dwc)
 				usleep_range(10000, 11000);
 			}
 		}
+
+		ret = dwc3_pci_add_properties(dwc, properties);
+		if (ret)
+			return ret;
 	}
 
 	if (pdev->vendor == PCI_VENDOR_ID_SYNOPSYS &&
@@ -162,7 +235,9 @@ static int dwc3_pci_quirks(struct dwc3_pci *dwc)
 			{ },
 		};
 
-		return platform_device_add_properties(dwc3, properties);
+		ret = dwc3_pci_add_properties(dwc, properties);
+		if (ret)
+			return ret;
 	}
 
 	return 0;
@@ -229,6 +304,14 @@ static int dwc3_pci_probe(struct pci_dev *pci,
 	dwc->dwc3->dev.parent = dev;
 	ACPI_COMPANION_SET(&dwc->dwc3->dev, ACPI_COMPANION(dev));
 
+	dwc->property_array_size = PROPERTY_ARRAY_INITIAL_SIZE;
+	dwc->properties = kcalloc(dwc->property_array_size,
+				  sizeof(*dwc->properties), GFP_KERNEL);
+	if (!dwc->properties) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
 	dev_set_name(dev, "dwc3-pci.%02x:%02x.%d", pci->bus->number,
 		     PCI_SLOT(pci->devfn), PCI_FUNC(pci->devfn));
 
@@ -236,6 +319,10 @@ static int dwc3_pci_probe(struct pci_dev *pci,
 	if (ret)
 		goto err;
 
+	ret = platform_device_add_properties(dwc->dwc3, dwc->properties);
+	if (ret)
+		goto err;
+
 	ret = platform_device_add(dwc->dwc3);
 	if (ret) {
 		dev_err(dev, "failed to register dwc3 device\n");
@@ -251,6 +338,7 @@ static int dwc3_pci_probe(struct pci_dev *pci,
 
 	return 0;
 err:
+	kfree(dwc->properties);
 	platform_device_put(dwc->dwc3);
 	return ret;
 }
@@ -264,6 +352,7 @@ static void dwc3_pci_remove(struct pci_dev *pci)
 #endif
 	device_init_wakeup(&pci->dev, false);
 	pm_runtime_get(&pci->dev);
+	kfree(dwc->properties);
 	platform_device_unregister(dwc->dwc3);
 }