Re: [PATCH v7 3/9] PCI: endpoint: functions/pci-epf-test: Add second entry

[Gustavo Pimentel]

Hi Lorenzo,

On 01/05/2018 15:26, Lorenzo Pieralisi wrote:
> On Tue, May 01, 2018 at 05:53:59PM +0530, Kishon Vijay Abraham I wrote:
>> Hi Lorenzo,
>>
>> On Tuesday 01 May 2018 05:24 PM, Lorenzo Pieralisi wrote:
>>> On Tue, May 01, 2018 at 03:37:47PM +0530, Kishon Vijay Abraham I wrote:
>>>> Hi Lorenzo,
>>>>
>>>> On Thursday 26 April 2018 10:26 PM, Lorenzo Pieralisi wrote:
>>>>> On Tue, Apr 24, 2018 at 02:44:40PM +0100, Gustavo Pimentel wrote:
>>>>>> Adds a seconds entry on the pci_epf_test_ids structure that disables the
>>>>>
>>>>> "Add a second entry to..."
>>>>>
>>>>>> linkup_notifier parameter on driver for the designware EP.
>>>>>>
>>>>>> This allows designware EPs that doesn't have linkup notification signal
>>>>>> to work with pcitest.
>>>>>>
>>>>>> Updates the binding documentation accordingly.
>>>>>
>>>>> Valid for all the series: use imperative sentences.
>>>>>
>>>>> eg:
>>>>>
>>>>> "Update the binding documentation accordingly".
>>>>>
>>>>> not
>>>>>
>>>>> "Updates the binding documentation accordingly".
>>>>>
>>>>>> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@xxxxxxxxxxxx>
>>>>>> Acked-by: Kishon Vijay Abraham I <kishon@xxxxxx>
>>>>>> ---
>>>>>> Change v2->v3:
>>>>>>  - Added second entry in pci_epf_test_ids structure.
>>>>>>  - Remove test_reg_bar field assignment on second entry.
>>>>>> Changes v3->v4:
>>>>>>  - Nothing changed, just to follow the patch set version.
>>>>>> Changes v4->v5:
>>>>>>  - Changed pci_epf_test_cfg2 to pci_epf_test_designware.
>>>>>> Changes v5->v6:
>>>>>>  - Changed name field from pci_epf_test_designware to pci_epf_test_dw.
>>>>>> Changes v6->v7:
>>>>>>  - Changed variable name from data_cfg2 to data_linkup_notifier_disabled.
>>>>>>
>>>>>>  Documentation/PCI/endpoint/function/binding/pci-test.txt | 2 ++
>>>>>>  drivers/pci/endpoint/functions/pci-epf-test.c            | 8 ++++++++
>>>>>>  2 files changed, 10 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/PCI/endpoint/function/binding/pci-test.txt b/Documentation/PCI/endpoint/function/binding/pci-test.txt
>>>>>> index 3b68b95..dc39f47 100644
>>>>>> --- a/Documentation/PCI/endpoint/function/binding/pci-test.txt
>>>>>> +++ b/Documentation/PCI/endpoint/function/binding/pci-test.txt
>>>>>> @@ -1,6 +1,8 @@
>>>>>>  PCI TEST ENDPOINT FUNCTION
>>>>>>  
>>>>>>  name: Should be "pci_epf_test" to bind to the pci_epf_test driver.
>>>>>> +name: Should be "pci_epf_test_dw" to bind to the pci_epf_test driver
>>>>>> +      with a custom configuration for the designware EP.
>>>>>
>>>>> The link between the "name" and the device created is quite obscure and
>>>>> reading pci-test-howto.txt certainly does not clarify it.
>>>>>
>>>>> In pci-test-howto.txt an explanation should be added to the configs
>>>>> device creation paragraph to clarify it.
>>>>>
>>>>>>  Configurable Fields:
>>>>>>  vendorid	 : should be 0x104c
>>>>>> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
>>>>>> index 7cef851..4ab463b 100644
>>>>>> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
>>>>>> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
>>>>>> @@ -459,10 +459,18 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>>>>>>  	return 0;
>>>>>>  }
>>>>>>  
>>>>>> +static const struct pci_epf_test_data data_linkup_notifier_disabled = {
>>>>>> +	.linkup_notifier = false
>>>>>> +};
>>>>>> +
>>>>>>  static const struct pci_epf_device_id pci_epf_test_ids[] = {
>>>>>>  	{
>>>>>>  		.name = "pci_epf_test",
>>>>>>  	},
>>>>>> +	{
>>>>>> +		.name = "pci_epf_test_dw",
>>>>>> +		.driver_data = (kernel_ulong_t)&data_linkup_notifier_disabled,
>>>>>> +	},
>>>>>>  	{},
>>>>>
>>>>> Should not this be a property derived from the controller compatible
>>>>> property instead of the test device name written in configfs ?
>>>>
>>>> pci_epf_test is an independent driver on its own that operates in a layer above
>>>> the controller driver. So it does not get the controller compatible (which is
>>>> used in controller drivers like pcie-designware-plat.c). pci_epf_test uses
>>>> "pci_epf_device_id" which is _similar_ to "of_device_id" used by platform drivers.
>>>
>>> I understand that, the problem is that the independent driver depends on
>>> features of the related controller driver as this patch shows. This
>>> patch basically says that if you use a specific path in configfs (that
>>> includes pci_epf_test_dw) your device has specific HW features (eg
>>> linkup notifier above), that obviously depends on the platform HW not on
>>> the string you use in configfs.
>>>
>>> What I am questioning is a) if I understand this right and b) whether
>>> this is the right approach.
>>
>> Your understanding is right. Ideally pci-epf-test driver shouldn't have any HW
>> specific configuration. But different HW have different configurations and
>> pci-epf-test should be informed of the configuration the HW supports.
> 
> I am honestly very confused. First off, I do not understand what this
> patch really does (or better what DW can't do so that it needs a
> specific configfs string and therefore a specific configuration).
> 
> What's the purpose of the linkup notifier ? What does it mean that
> the DW HW can't handle it ?
> 
> Are we referring to the pci_epf_linkup() function ?
> 
> If it is a HW configuration (ie the DW HW does not have a signal to
> report that the link is up ?) its enablement must depend on HW
> controller properties not configfs entries, I do not like what this
> patch does (probably because I am confused and I do not understand it).
> 
> Please let me know your thoughts on this, thanks.

On my setup, the EP is unable generate a signal which is responsible for notify
that the link is established, being therefore necessary to emulate it, this is
done by setting the linkup_notifier variable to false.

This signal allows the pci-epf-test driver on the EP side to startup with a
cyclic task defined on pci_epf_test_cmd_handler() that checks a command register
located on the BAR (by default on BAR 0) settled by pci_endpoint_test driver on
the RC side that defines, by his turn defines which type of command to perform.

> 
> Lorenzo
> 
>>
>> configfs is just one way of creating epf_device and it was mainly added since
>> pci-epf-test cannot have a dt entry because it doesn't represent anything in
>> the HW.
>>
>> The other option was to have a callback to EPC driver to get the features it
>> supports. But a particular feature that is required might be specific to a EPF
>> driver.
>>
>> I find the driver_data approach in pci_epf_device_id to be more clean.

Since the linkup notifier and BAR index (where auxiliary registers are located)
may be configurable and is something platform dependent, perhaps the
configuration of this variables should be done by module parameter and not by
configfs, leaving this configuration responsibility in charge of each platform.

I think this option is also a good alternative, simple and clean. What do you think?

>>
>> Thanks
>> Kishon

Regards,
Gustavo