[PATCH v1 06/24] pci: pci-uclass: Add multi entry support for memory regions

Tom Rini trini at konsulko.com
Mon Aug 24 15:09:00 CEST 2020


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 <sr at denx.de> wrote:
> > > > > 
> > > > > 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?
> > > > 
> > > > 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: <https://lists.denx.de/pipermail/u-boot/attachments/20200824/bae25aa1/attachment.sig>


More information about the U-Boot mailing list