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

[Felipe Balbi <balbi@kernel.org>]

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