From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Rini Date: Sun, 23 Aug 2020 10:03:19 -0400 Subject: [PATCH v1 06/24] pci: pci-uclass: Add multi entry support for memory regions In-Reply-To: References: <20200724100856.1482324-1-sr@denx.de> <20200724100856.1482324-7-sr@denx.de>

Message-ID: <20200823140319.GI20605@bill-the-cat> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Sun, Aug 23, 2020 at 11:41:41AM +0200, Stefan Roese wrote: > Hi Simon, > Hi Tom, > > On 22.08.20 17:09, Simon Glass wrote: > > Hi Stefan, > > > > On Fri, 14 Aug 2020 at 05:40, Stefan Roese wrote: > > > > > > Hi Simon, > > > > > > On 04.08.20 17:05, Simon Glass wrote: > > > > > > > > > > > > > > > > > > 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? > > > > Not really. I see these things as features that drivers can enable / > > disable depending on their needs. If that is correct, then sandbox is > > no different form other drivers, except that perhaps it might support > > all combinations rather than just one. > > > > > > > > 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. > > > > How about some flags in struct pci_controller? > > > > > > > > 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. > > > > Kconfig enables and disables the feature by adding the code. But we > > can still have a flag to determine whether it is used by a particular > > driver. That way we can keep our test coverage. > > > > > > > > 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. > > > > Yes but it is not that hard. There are a few PCI emulation devices in > > U-Boot and these form the basis of existing tests. Before this, we > > really had no tests for PCI and even the behaviour was largely > > undefined. Your ability to convert the regions[] array to a > > dynamically allocated array is partly thanks to these tets. > > > > > > > > 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? > > > > I don't want to be silly about it, but in general if we add new > > features, particularly to core features, I think there should be > > tests. > > As mentioned before, I agree in general. But we should be not too > strict here IMHO, to enforce new tests for each and every U-Boot > addition. It's probably not easy to draw the line here to decide, > when and when not to enforce tests. Perhaps this should reflect the > complexitiy of the test code and also the user count of the new > U-Boot code / features (here its solely Octeon TX/TX2). > > > Apart from correctness, they also define the behaviour of the > > code, in many cases. The test you added in one patch looks good, and > > doesn't look too complicated. > > Yes, that one was quite simple. But emulating special PCI device > capabilities to enable such virtual testings will be much more complex, > AFAICT. If it would be "easy", I would not argue with you on this and > just implement these tests and be done with it. ;) But frankly, I have > no real idea (without digging much deeper into this) on how to add these > emulation codes and tests in a way, that they completely test all the > feature additions. Please note that I'm not the original author of these > additions. > > > Of course it is more than zero work. But > > so much of the refactoring we do in U-Boot these days would be much > > harder and error-prone without these tests. > > > > +Tom Rini who might want to weigh in. > > Tom, are you okay with going forward with these PCIe related patches, > without adding test cases for all PCI feature additions? Which would > mean to add very special PCI device emulation and test drivers for > such devices? The PCI extensions are not that intrusive and AFAICT > Simon is okay with all of them in general, only would like to have tests > for all driver extensions. For complex hardware specific things like this and testing I think we'll have the most luck by, when possible and I'm not sure we do enough today, enable features and tests on qemu* machines and get it that way, rather than writing new emulators for sandbox. -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 659 bytes Desc: not available URL: