| Message ID | 20200724100856.1482324-7-sr@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Stefan Roese |
| Headers | show |
| Series | arm: Introduce Marvell/Cavium OcteonTX/TX2 | expand |
Hi Stefan, On Fri, 24 Jul 2020 at 04:09, Stefan Roese <sr@denx.de> wrote: > > From: Suneel Garapati <sgarapati@marvell.com> > > Enable PCI memory regions in ranges property to be of multiple entry. > This helps to add support for SoC's like OcteonTX/TX2 where every > peripheral is on PCI bus. > > Signed-off-by: Suneel Garapati <sgarapati@marvell.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: Bin Meng <bmeng.cn@gmail.com> > > Signed-off-by: Stefan Roese <sr@denx.de> > --- > > 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.
Hi Simon, On 28.07.20 21:01, Simon Glass wrote: > Hi Stefan, > > On Fri, 24 Jul 2020 at 04:09, Stefan Roese <sr@denx.de> wrote: >> >> From: Suneel Garapati <sgarapati@marvell.com> >> >> Enable PCI memory regions in ranges property to be of multiple entry. >> This helps to add support for SoC's like OcteonTX/TX2 where every >> peripheral is on PCI bus. >> >> Signed-off-by: Suneel Garapati <sgarapati@marvell.com> >> Cc: Simon Glass <sjg@chromium.org> >> Cc: Bin Meng <bmeng.cn@gmail.com> >> >> Signed-off-by: Stefan Roese <sr@denx.de> >> --- >> >> 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. Thanks, Stefan
Hi Stefan, On Thu, 30 Jul 2020 at 09:35, Stefan Roese <sr@denx.de> wrote: > > Hi Simon, > > On 28.07.20 21:01, Simon Glass wrote: > > Hi Stefan, > > > > On Fri, 24 Jul 2020 at 04:09, Stefan Roese <sr@denx.de> wrote: > >> > >> From: Suneel Garapati <sgarapati@marvell.com> > >> > >> Enable PCI memory regions in ranges property to be of multiple entry. > >> This helps to add support for SoC's like OcteonTX/TX2 where every > >> peripheral is on PCI bus. > >> > >> Signed-off-by: Suneel Garapati <sgarapati@marvell.com> > >> Cc: Simon Glass <sjg@chromium.org> > >> Cc: Bin Meng <bmeng.cn@gmail.com> > >> > >> Signed-off-by: Stefan Roese <sr@denx.de> > >> --- > >> > >> 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? Regards, Simon
Hi Simon, On 31.07.20 20:44, Simon Glass wrote: > Hi Stefan, > > On Thu, 30 Jul 2020 at 09:35, Stefan Roese <sr@denx.de> wrote: >> >> Hi Simon, >> >> On 28.07.20 21:01, Simon Glass wrote: >>> Hi Stefan, >>> >>> On Fri, 24 Jul 2020 at 04:09, Stefan Roese <sr@denx.de> wrote: >>>> >>>> From: Suneel Garapati <sgarapati@marvell.com> >>>> >>>> Enable PCI memory regions in ranges property to be of multiple entry. >>>> This helps to add support for SoC's like OcteonTX/TX2 where every >>>> peripheral is on PCI bus. >>>> >>>> Signed-off-by: Suneel Garapati <sgarapati@marvell.com> >>>> Cc: Simon Glass <sjg@chromium.org> >>>> Cc: Bin Meng <bmeng.cn@gmail.com> >>>> >>>> Signed-off-by: Stefan Roese <sr@denx.de> >>>> --- >>>> >>>> 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? Thanks, Stefan
Hi Stefan, On Tue, 4 Aug 2020 at 08:03, Stefan Roese <sr@denx.de> wrote: > > Hi Simon, > > On 31.07.20 20:44, Simon Glass wrote: > > Hi Stefan, > > > > On Thu, 30 Jul 2020 at 09:35, Stefan Roese <sr@denx.de> wrote: > >> > >> Hi Simon, > >> > >> On 28.07.20 21:01, Simon Glass wrote: > >>> Hi Stefan, > >>> > >>> On Fri, 24 Jul 2020 at 04:09, Stefan Roese <sr@denx.de> wrote: > >>>> > >>>> From: Suneel Garapati <sgarapati@marvell.com> > >>>> > >>>> Enable PCI memory regions in ranges property to be of multiple entry. > >>>> This helps to add support for SoC's like OcteonTX/TX2 where every > >>>> peripheral is on PCI bus. > >>>> > >>>> Signed-off-by: Suneel Garapati <sgarapati@marvell.com> > >>>> Cc: Simon Glass <sjg@chromium.org> > >>>> Cc: Bin Meng <bmeng.cn@gmail.com> > >>>> > >>>> Signed-off-by: Stefan Roese <sr@denx.de> > >>>> --- > >>>> > >>>> 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. Regards, SImon
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
Hi Stefan, On Fri, 14 Aug 2020 at 05:40, Stefan Roese <sr@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. 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. 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. Regards, SImon
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@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. Thanks, Stefan
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@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.
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@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? Thanks, Stefan
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@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.
Hi, On Mon, 24 Aug 2020 at 07:09, Tom Rini <trini@konsulko.com> wrote: > > 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@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. That sounds fine, particularly as these are arch-specific and therefore not enabled on any other board. What happens when they are not? It would be interesting to hear if qemu supports this. Having said that, sandbox has the advantage that it can quickly test all the features at once and it is easy for someone to debug and fix (vs. qemu). IMO it is often worth the investment. Regards, Simon
On Tue, Aug 25, 2020 at 09:04:26AM -0600, Simon Glass wrote: > Hi, > > On Mon, 24 Aug 2020 at 07:09, Tom Rini <trini@konsulko.com> wrote: > > > > 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@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. > > That sounds fine, particularly as these are arch-specific and > therefore not enabled on any other board. What happens when they are > not? > > It would be interesting to hear if qemu supports this. Having said > that, sandbox has the advantage that it can quickly test all the > features at once and it is easy for someone to debug and fix (vs. > qemu). IMO it is often worth the investment. There are trade-offs in sandbox vs qemu. There's a lot of stuff I love sandbox for. But when we get in to "emulate some hardware" there's a big community for qemu and just us for sandbox. If we can't test it in qemu, and it's fairly complex, testing it on hardware only is the final stop. So yes, get tests, but if they're only able to be run when we have the hardware, that's fine so long as someone is writing the test and running it on hardware, at least as part of submission. More automated hardware testing is great.
diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig index ff974e5d74..14c96dd108 100644 --- a/drivers/pci/Kconfig +++ b/drivers/pci/Kconfig @@ -43,6 +43,16 @@ config PCI_PNP help Enable PCI memory and I/O space resource allocation and assignment. +config PCI_REGION_MULTI_ENTRY + bool "Enable Multiple entries of region type MEMORY in ranges for PCI" + depends on PCI || DM_PCI + default n + help + Enable PCI memory regions to be of multiple entry. Multiple entry + here refers to allow more than one count of address ranges for MEMORY + region type. This helps to add support for SoC's like OcteonTX/TX2 + where every peripheral is on the PCI bus. + config PCIE_ECAM_GENERIC bool "Generic ECAM-based PCI host controller support" default n diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c index fa2436a0ef..061ac4e943 100644 --- a/drivers/pci/pci-uclass.c +++ b/drivers/pci/pci-uclass.c @@ -936,10 +936,13 @@ static void decode_regions(struct pci_controller *hose, ofnode parent_node, } pos = -1; - for (i = 0; i < hose->region_count; i++) { - if (hose->regions[i].flags == type) - pos = i; + if (!IS_ENABLED(CONFIG_PCI_REGION_MULTI_ENTRY)) { + for (i = 0; i < hose->region_count; i++) { + if (hose->regions[i].flags == type) + pos = i; + } } + if (pos == -1) pos = hose->region_count++; debug(" - type=%d, pos=%d\n", type, pos);