From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Rini Date: Mon, 24 Aug 2020 09:09:00 -0400 Subject: [PATCH v1 06/24] pci: pci-uclass: Add multi entry support for memory regions In-Reply-To: <75dc6398-3d16-ec0c-9053-58a9429169d4@denx.de> References:

<20200823140319.GI20605@bill-the-cat> <75dc6398-3d16-ec0c-9053-58a9429169d4@denx.de> Message-ID: <20200824130900.GW20605@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 Mon, Aug 24, 2020 at 09:36:13AM +0200, Stefan Roese wrote: > Hi Tom, > > On 23.08.20 16:03, Tom Rini wrote: > > 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. > > I agree. As was proved by the QEMU Nokia test a few days ago with the > bi_memstart (etc) cleanup, I'm working on. I also had the idea that such > an QEMU machine emulation would be much better. Let me check, if any > such machine emulation is possible for Octeon TX/TX2. I'll get back to > this, once I have more informations here. > > In the meantime, how do we proceeed with this current Octeon TX/TX2 > patchset? I would really like to get at least some of it included > into mainline soon. The first RFC version has been posted to the list > by Suneel end of October last year, so more than 3/4 of a year ago. > My proposal would be (if nobody objects), to push the PCI patches > and Octeon TX/TX2 base port into mainline, dropping the really big > drivers (nand & ethernet) for now, which need a bit more review IMHO. > > Should I continue this way? I think that's reasonable, yes. -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 659 bytes Desc: not available URL: