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

[Simon Glass]

Hi Stefan,

On Tue, 4 Aug 2020 at 08:03, Stefan Roese <sr at 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 at 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 at denx.de> wrote:
> >>>>
> >>>> From: Suneel Garapati <sgarapati at 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 at marvell.com>
> >>>> Cc: Simon Glass <sjg at chromium.org>
> >>>> Cc: Bin Meng <bmeng.cn at gmail.com>
> >>>>
> >>>> Signed-off-by: Stefan Roese <sr at 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