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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 03/01/2018 12:25 AM, Felipe Balbi wrote:

Hi,

John Youn <John.Youn@xxxxxxxxxxxx> writes:
On 02/22/2018 07:20 AM, Andy Shevchenko wrote:
On Thu, Feb 22, 2018 at 1:57 AM, Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> wrote:
On 2/21/2018 6:46 AM, Andy Shevchenko wrote:
On Tue, Feb 20, 2018 at 11:12 PM, Thinh Nguyen
<Thinh.Nguyen@xxxxxxxxxxxx> wrote:
On 2/17/2018 7:29 AM, Andy Shevchenko wrote:
On Fri, Feb 16, 2018 at 11:55 PM, Thinh Nguyen
<Thinh.Nguyen@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux