Hi Simon,
On 04.08.20 17:05, Simon Glass wrote:
<snip>
Changes in v1:
- Change patch subject
- Enhance Kconfig help descrition
- Use if() instead of #if
drivers/pci/Kconfig | 10 ++++++++++
drivers/pci/pci-uclass.c | 9 ++++++---
2 files changed, 16 insertions(+), 3 deletions(-)
This needs an update to a sandbox test to handle this behaviour.
Okay. But how should I handle all these defconfig changes with regard
to the other patches in this series, introducing multiple new PCI
related Kconfig options. With 3 new Kconfig options, all permutations
would lead to 8 (2 ^ 3) different defconfig files. This does not
scale.
I might be missing something here though - perhaps this is easier to
achieve.
For sandbox, turn on all options and then add a new PCI bus that uses
this functionality. If there are lots of combinations you could add 8
new buses, but I'm hoping that isn't necessary?
If I turn on all new options, sandbox will run with these new options
enabled. I don't know with with implications, as it usually runs with
the "normal" PCI related Kconfig options. Also the "normal" PCI
defconfig (e.g. CONFIG_PCI_REGION_MULTI_ENTRY etc disabled) will not
be tested any more via the sandbox tests. So you get either a test for
the new Kconfig option enabled or disabled this way.
Do you really want me to do this?
So the Kconfig completely changes the implementation of PCI? That
doesn't make it very testable, as you say.
Instead, I think the Kconfig should enable the option, then use one of
three ways to select the option:
- a device tree property (on sandbox particularly)
- compatible string (where the property is not appropriate
- setting a flag in PCI bus (where a driver requires the option be selected)
That way you can write a test for the new feature in sandbox, without
deleting all the other tests.
Coming back to this issue after some time - sorry for the delay.
I'm not sure, if I understand this correctly. Do you suggest that the
driver code (in this case pci-uclass.c) should be extended to support
this (sandbox) testing support?
If yes, I really think that this is counterproductive. As we added (at
least some of) the Kconfig options explicitly, to not add code to
pci-uclass.c in the "normal case". So adding code to e.g. check a device
tree property or a compatible string would increase the code size again.
If not, I'm still unsure how you would like to test the "normal case",
e.g. with CONFIG_PCI_REGION_MULTI_ENTRY disabled, and with it enabled
without adding more sandbox build targets, with all the Kconfig options
permutations. As the extra code (in pci-uclass) is either included or
not in the sandbox binary.
But after adding one test for the first of these pci-uclass related
patches, I do have a general comment on this. I find it quite complex
and time consuming to add these tests. Don't get me wrong, I agree in
general, that having tests in U-Boot is very good. But enforcing tests
for each and every new feature addition in drivers (layers) like PCI
seems a bit too much to me. For example new features like the "pci:
pci-uclass: Add support for Single-Root I/O Virtualization" would mean
AFAIU, that I need to write some emulation code for such a PCI device
and also some testing driver matching such a device, since we have no
real hardware like this in sandbox. This would result in much more
complex code for this test & emulation compared to the driver change /
extension.
To sum it up, I'm asking if you still think that adding tests for all
those PCI driver extensions is really necessary for upstream acceptance?
What's your opinion on this? Do you understand my position on this?
Thanks,
Stefan
