Message ID | c6cfd167838ae8e226dee0820983efe918fa4e1f.1524577064.git.gustavo.pimentel@synopsys.com |
---|---|
State | Superseded |
Delegated to: | Lorenzo Pieralisi |
Headers | show |
Series | Designware EP support and code clean up | expand |
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@synopsys.com> > Acked-by: Kishon Vijay Abraham I <kishon@ti.com> > --- > 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 ? I think I am missing something in the whole scheme of things, please clarify. Thanks, Lorenzo
On 26/04/2018 17:56, 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". > Ok, I will do an overall check to fix the entries. >> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com> >> Acked-by: Kishon Vijay Abraham I <kishon@ti.com> >> --- >> 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. I will add more information about it. > > In pci-test-howto.txt an explanation should be added to the configs > device creation paragraph to clarify it. That's true, it should exist some explanation of that. To be honest I didn't remember of the pci-test-howto.txt file existence. > >> 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 ? > > I think I am missing something in the whole scheme of things, please > clarify. This type of configuration / configuration was suggested by Kishon. I can only assume that it would not be possible (or no one thought of it) to correlate the compatible string of the controller to select the configuration, perhaps Kishon could give his opinion on the feasibility of this and even to provide some example of it. :) If it is possible, it will be quite straightforward. > > Thanks, > Lorenzo >
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@synopsys.com> >> Acked-by: Kishon Vijay Abraham I <kishon@ti.com> >> --- >> 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. Thanks Kishon
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@synopsys.com> > >> Acked-by: Kishon Vijay Abraham I <kishon@ti.com> > >> --- > >> 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. Lorenzo
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@synopsys.com> >>>> Acked-by: Kishon Vijay Abraham I <kishon@ti.com> >>>> --- >>>> 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. 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. Thanks Kishon
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@synopsys.com> > >>>> Acked-by: Kishon Vijay Abraham I <kishon@ti.com> > >>>> --- > >>>> 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. 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. > > Thanks > Kishon
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@synopsys.com> >>>>>> Acked-by: Kishon Vijay Abraham I <kishon@ti.com> >>>>>> --- >>>>>> 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
On Wed, May 02, 2018 at 11:39:00AM +0100, Gustavo Pimentel wrote: > 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@synopsys.com> > >>>>>> Acked-by: Kishon Vijay Abraham I <kishon@ti.com> > >>>>>> --- > >>>>>> 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. What I do not understand is why the workqueue notification can't be called irrespective of the linkup notifier value from pci_epf_test_bind(), what's the point in calling it earlier with pci_epf_linkup() (or the other way around why is it safe to call it a bind() time if there is no notification available - which is the DW case). [...] > 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. They are platform dependent because they depend on the EP controller. That's why I said that those are EP controller parameters. I do not think they are module parameters either - they should be part of HW description in firmware. Lorenzo > I think this option is also a good alternative, simple and clean. What > do you think? > > >> > >> Thanks > >> Kishon > > Regards, > Gustavo >
Hi Lorenzo, On Wednesday 02 May 2018 10:21 PM, Lorenzo Pieralisi wrote: > On Wed, May 02, 2018 at 11:39:00AM +0100, Gustavo Pimentel wrote: >> 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@synopsys.com> >>>>>>>> Acked-by: Kishon Vijay Abraham I <kishon@ti.com> >>>>>>>> --- >>>>>>>> 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. > > What I do not understand is why the workqueue notification can't be called > irrespective of the linkup notifier value from pci_epf_test_bind(), > what's the point in calling it earlier with pci_epf_linkup() (or the > other way around why is it safe to call it a bind() time if there > is no notification available - which is the DW case). The linkup notifier is used just to prevent the system from wasting cpu cycles. Without linkup notifier, pci_epf_test will start checking for commands from host after the pci_epf_test is bound to the EP controller (bind() callback) though the host can issue commands only when after the link is up. So for platforms which supports linkup notification, it's better we poll command register only after link is established. > > [...] > >> 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. > > They are platform dependent because they depend on the EP controller. > That's why I said that those are EP controller parameters. I do not > think they are module parameters either - they should be part of HW > description in firmware. The problem is because pci-epf-test cannot be described in HW. pci-epf-test is also not automatically bound to the EP controller but is bound by the user like below ln -s functions/pci_epf_test/func1 controllers/51000000.pcie_ep/ So given that user anyways has to bind the epf device to the controller, based on the platform the user can use a different configfs entry like below ln -s functions/pci_epf_test_dw/func1 controllers/51000000.pcie_ep/ or ln -s functions/pci_epf_test_k2g/func1 controllers/21800000.pcie-ep/ If the epf can be described in dt, then something like below can be done pcie1_ep: pcie_ep@51000000 { compatible = "ti,dra7-pcie-ep"; interrupts = <0 232 0x4>; num-lanes = <1>; num-ib-windows = <4>; num-ob-windows = <16>; phys = <&pcie1_phy>; phy-names = "pcie-phy0"; pci_epf_test: pci_epf_test@0 { name = "pci_epf_test_dw"; <other properties>; } }; With this pci-dra7xx.c driver should create pci_epf_device using pci_epf_create("pci_epf_test_dw"); Then the driver_data corresponding to "pci_epf_test_dw" will select linkup notifier or BAR index etc. Having different pci_epf_device_id entries and corresponding driver_data for each entry seems fine to me (as done in this patch). We use configfs only since pci_epf_test cannot be described in dt. Thanks Kishon
On Thu, May 03, 2018 at 12:03:15PM +0530, Kishon Vijay Abraham I wrote: [...] > >> 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. > > > > They are platform dependent because they depend on the EP controller. > > That's why I said that those are EP controller parameters. I do not > > think they are module parameters either - they should be part of HW > > description in firmware. > > The problem is because pci-epf-test cannot be described in HW. pci-epf-test is > also not automatically bound to the EP controller but is bound by the user like > below > ln -s functions/pci_epf_test/func1 controllers/51000000.pcie_ep/ > > So given that user anyways has to bind the epf device to the controller, based > on the platform the user can use a different configfs entry like below > ln -s functions/pci_epf_test_dw/func1 controllers/51000000.pcie_ep/ or > ln -s functions/pci_epf_test_k2g/func1 controllers/21800000.pcie-ep/ > > If the epf can be described in dt, then something like below can be done > pcie1_ep: pcie_ep@51000000 { > compatible = "ti,dra7-pcie-ep"; > interrupts = <0 232 0x4>; > num-lanes = <1>; > num-ib-windows = <4>; > num-ob-windows = <16>; > phys = <&pcie1_phy>; > phy-names = "pcie-phy0"; > pci_epf_test: pci_epf_test@0 { > name = "pci_epf_test_dw"; > <other properties>; > } > }; > > With this pci-dra7xx.c driver should create pci_epf_device using > pci_epf_create("pci_epf_test_dw"); > > Then the driver_data corresponding to "pci_epf_test_dw" will select linkup > notifier or BAR index etc. Those two properties are properties of the EP controller (it is not 100% clear to me how the test BAR register is defined), is this correct ? If yes, given that those properties are not useful before an EPF is bound to an EPC, can't they be retrieved at bind time from the EPC controller data (that we can add through DT bindings) ? Thanks, Lorenzo
Hi Lorenzo and Kishon, On 03/05/2018 07:33, Kishon Vijay Abraham I wrote: > Hi Lorenzo, > > On Wednesday 02 May 2018 10:21 PM, Lorenzo Pieralisi wrote: >> On Wed, May 02, 2018 at 11:39:00AM +0100, Gustavo Pimentel wrote: >>> 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@synopsys.com> >>>>>>>>> Acked-by: Kishon Vijay Abraham I <kishon@ti.com> >>>>>>>>> --- >>>>>>>>> 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. >> >> What I do not understand is why the workqueue notification can't be called >> irrespective of the linkup notifier value from pci_epf_test_bind(), >> what's the point in calling it earlier with pci_epf_linkup() (or the >> other way around why is it safe to call it a bind() time if there >> is no notification available - which is the DW case). > > The linkup notifier is used just to prevent the system from wasting cpu cycles. > Without linkup notifier, pci_epf_test will start checking for commands from > host after the pci_epf_test is bound to the EP controller (bind() callback) > though the host can issue commands only when after the link is up. So for > platforms which supports linkup notification, it's better we poll command > register only after link is established. >> >> [...] >> >>> 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. >> >> They are platform dependent because they depend on the EP controller. >> That's why I said that those are EP controller parameters. I do not >> think they are module parameters either - they should be part of HW >> description in firmware. > > The problem is because pci-epf-test cannot be described in HW. pci-epf-test is > also not automatically bound to the EP controller but is bound by the user like > below > ln -s functions/pci_epf_test/func1 controllers/51000000.pcie_ep/ > > So given that user anyways has to bind the epf device to the controller, based > on the platform the user can use a different configfs entry like below > ln -s functions/pci_epf_test_dw/func1 controllers/51000000.pcie_ep/ or > ln -s functions/pci_epf_test_k2g/func1 controllers/21800000.pcie-ep/ > > If the epf can be described in dt, then something like below can be done > pcie1_ep: pcie_ep@51000000 { > compatible = "ti,dra7-pcie-ep"; > interrupts = <0 232 0x4>; > num-lanes = <1>; > num-ib-windows = <4>; > num-ob-windows = <16>; > phys = <&pcie1_phy>; > phy-names = "pcie-phy0"; > pci_epf_test: pci_epf_test@0 { > name = "pci_epf_test_dw"; > <other properties>; > } > }; > > With this pci-dra7xx.c driver should create pci_epf_device using > pci_epf_create("pci_epf_test_dw"); > > Then the driver_data corresponding to "pci_epf_test_dw" will select linkup > notifier or BAR index etc. > > Having different pci_epf_device_id entries and corresponding driver_data for > each entry seems fine to me (as done in this patch). We use configfs only since > pci_epf_test cannot be described in dt. I think I understood your point of view Lorenzo and it makes sense. However I think I have an alternative, since this 2 parameters are platform dependent, but there aren't quite a HW description, so they shouldn't be described on DT, right? So I would propose another alternative. What if each EP driver (in my case pcie-designware-plat) provided a callback that could be used to set the test configuration data, if needed? Similar to the dw_plat_set_num_vectors callback. In fact, something similar mechanism already exists, for example when the pci_epf_test driver tries to get the number of MSI/MSI-X, the driver retrieves the value through callbacks. In this way each driver would be responsible for adapting to their needs, there would be no changes to the DT modification or extra configfs entries. What do you think? Regards, Gustavo > > Thanks > Kishon >
Hi Lorenzo, On Thursday 03 May 2018 07:46 PM, Lorenzo Pieralisi wrote: > On Thu, May 03, 2018 at 12:03:15PM +0530, Kishon Vijay Abraham I wrote: > > [...] > >>>> 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. >>> >>> They are platform dependent because they depend on the EP controller. >>> That's why I said that those are EP controller parameters. I do not >>> think they are module parameters either - they should be part of HW >>> description in firmware. >> >> The problem is because pci-epf-test cannot be described in HW. pci-epf-test is >> also not automatically bound to the EP controller but is bound by the user like >> below >> ln -s functions/pci_epf_test/func1 controllers/51000000.pcie_ep/ >> >> So given that user anyways has to bind the epf device to the controller, based >> on the platform the user can use a different configfs entry like below >> ln -s functions/pci_epf_test_dw/func1 controllers/51000000.pcie_ep/ or >> ln -s functions/pci_epf_test_k2g/func1 controllers/21800000.pcie-ep/ >> >> If the epf can be described in dt, then something like below can be done >> pcie1_ep: pcie_ep@51000000 { >> compatible = "ti,dra7-pcie-ep"; >> interrupts = <0 232 0x4>; >> num-lanes = <1>; >> num-ib-windows = <4>; >> num-ob-windows = <16>; >> phys = <&pcie1_phy>; >> phy-names = "pcie-phy0"; >> pci_epf_test: pci_epf_test@0 { >> name = "pci_epf_test_dw"; >> <other properties>; >> } >> }; >> >> With this pci-dra7xx.c driver should create pci_epf_device using >> pci_epf_create("pci_epf_test_dw"); >> >> Then the driver_data corresponding to "pci_epf_test_dw" will select linkup >> notifier or BAR index etc. > > Those two properties are properties of the EP controller (it is not 100% > clear to me how the test BAR register is defined), is this correct ? Right, these properties are specific to a platform. In some of the platforms like K2G (BAR0 is reserved i.e it is used to map PCIe app registers and cannot be used by pci_epf_test. In such cases we should use a BAR other than BAR0). > > If yes, given that those properties are not useful before an EPF is > bound to an EPC, can't they be retrieved at bind time from the EPC > controller data (that we can add through DT bindings) ? hmm.. We can have quirk in pci_epc, something like below struct pci_epc { . . unsigned int quirks; . . }; #define EPC_QUIRKS_NO_LINKUP_NOTIFIER BIT(0) #define EPC_QUIRKS_BAR0_RESERVED BIT(1) #define EPC_QUIRKS_BAR1_RESERVED BIT(2) #define EPC_QUIRKS_BAR2_RESERVED BIT(3) #define EPC_QUIRKS_BAR3_RESERVED BIT(4) #define EPC_QUIRKS_BAR4_RESERVED BIT(5) #define EPC_QUIRKS_BAR5_RESERVED BIT(6) The controller driver can set the appropriate quirks epc->quirks |= EPC_QUIRKS_NO_LINKUP_NOTIFIER | EPC_QUIRKS_BAR0_RESERVED; Then pci-epf-test driver can checks the quirks to see the supported EPC features. Does something like above looks okay to you? Thanks Kishon
On Fri, May 04, 2018 at 11:28:58AM +0530, Kishon Vijay Abraham I wrote: > Hi Lorenzo, > > On Thursday 03 May 2018 07:46 PM, Lorenzo Pieralisi wrote: > > On Thu, May 03, 2018 at 12:03:15PM +0530, Kishon Vijay Abraham I wrote: > > > > [...] > > > >>>> 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. > >>> > >>> They are platform dependent because they depend on the EP controller. > >>> That's why I said that those are EP controller parameters. I do not > >>> think they are module parameters either - they should be part of HW > >>> description in firmware. > >> > >> The problem is because pci-epf-test cannot be described in HW. pci-epf-test is > >> also not automatically bound to the EP controller but is bound by the user like > >> below > >> ln -s functions/pci_epf_test/func1 controllers/51000000.pcie_ep/ > >> > >> So given that user anyways has to bind the epf device to the controller, based > >> on the platform the user can use a different configfs entry like below > >> ln -s functions/pci_epf_test_dw/func1 controllers/51000000.pcie_ep/ or > >> ln -s functions/pci_epf_test_k2g/func1 controllers/21800000.pcie-ep/ > >> > >> If the epf can be described in dt, then something like below can be done > >> pcie1_ep: pcie_ep@51000000 { > >> compatible = "ti,dra7-pcie-ep"; > >> interrupts = <0 232 0x4>; > >> num-lanes = <1>; > >> num-ib-windows = <4>; > >> num-ob-windows = <16>; > >> phys = <&pcie1_phy>; > >> phy-names = "pcie-phy0"; > >> pci_epf_test: pci_epf_test@0 { > >> name = "pci_epf_test_dw"; > >> <other properties>; > >> } > >> }; > >> > >> With this pci-dra7xx.c driver should create pci_epf_device using > >> pci_epf_create("pci_epf_test_dw"); > >> > >> Then the driver_data corresponding to "pci_epf_test_dw" will select linkup > >> notifier or BAR index etc. > > > > Those two properties are properties of the EP controller (it is not 100% > > clear to me how the test BAR register is defined), is this correct ? > > Right, these properties are specific to a platform. In some of the platforms > like K2G (BAR0 is reserved i.e it is used to map PCIe app registers and cannot > be used by pci_epf_test. In such cases we should use a BAR other than BAR0). I do not think those properties are pci_epf_test specific, that's the whole point I am making. Those are EPC controller features. > > If yes, given that those properties are not useful before an EPF is > > bound to an EPC, can't they be retrieved at bind time from the EPC > > controller data (that we can add through DT bindings) ? > > hmm.. > > We can have quirk in pci_epc, something like below > > struct pci_epc { > . > . > unsigned int quirks; > . > . > }; > > #define EPC_QUIRKS_NO_LINKUP_NOTIFIER BIT(0) > #define EPC_QUIRKS_BAR0_RESERVED BIT(1) > #define EPC_QUIRKS_BAR1_RESERVED BIT(2) > #define EPC_QUIRKS_BAR2_RESERVED BIT(3) > #define EPC_QUIRKS_BAR3_RESERVED BIT(4) > #define EPC_QUIRKS_BAR4_RESERVED BIT(5) > #define EPC_QUIRKS_BAR5_RESERVED BIT(6) > > The controller driver can set the appropriate quirks > epc->quirks |= EPC_QUIRKS_NO_LINKUP_NOTIFIER | EPC_QUIRKS_BAR0_RESERVED; > > Then pci-epf-test driver can checks the quirks to see the supported EPC features. > > Does something like above looks okay to you? Well, it is better than the driver_data approach, I would not call them quirks but features and for the BARs you should define a mask it does not make sense to enumerate them. It is probably a good idea to leave room for additional "features" so please choose the bits placement wisely. Thanks, Lorenzo
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. 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, + }, {}, };