Re: [PATCH v3 7/8] PCI: vmd: Add support for second rootbus under VMD

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Nov 22, 2024 at 09:52:14AM +0100, Szymon Durawa wrote:
> Starting from Intel Arrow Lake VMD enhancement introduces second rootbus
> support with fixed root bus number (0x80). It means that all 3 MMIO BARs
> exposed by VMD are shared now between both buses (current BUS0 and
> new BUS1).
> 
> Add new BUS1 enumeration and divide MMIO space to be shared between
> both rootbuses. Due to enumeration issues with rootbus hardwired to a
> fixed non-zero value, this patch will work with a workaround proposed
> in next patch. Without workaround user won't see attached devices for BUS1
> rootbus.

s/rootbus/root bus/

> Suggested-by: Nirmal Patel <nirmal.patel@linux.intel.com>
> Reviewed-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
> Signed-off-by: Szymon Durawa <szymon.durawa@linux.intel.com>
> ---
>  drivers/pci/controller/vmd.c | 208 ++++++++++++++++++++++++++++++-----
>  1 file changed, 180 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index 6d8397b5ebee..6cd14c28fd4e 100755
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -26,6 +26,7 @@
>  #define VMD_RESTRICT_0_BUS_START 0
>  #define VMD_RESTRICT_1_BUS_START 128
>  #define VMD_RESTRICT_2_BUS_START 224
> +#define VMD_RESTRICT_3_BUS_START 225

You're just following the pattern here, which makes sense.  But these
are apparently bus numbers, which are typically written in hex, so it
would be nice to convert them all so we don't have to convert.

>  #define PCI_REG_VMCAP		0x40
>  #define BUS_RESTRICT_CAP(vmcap)	(vmcap & 0x1)
> @@ -38,15 +39,33 @@
>  #define MB2_SHADOW_OFFSET	0x2000
>  #define MB2_SHADOW_SIZE		16
>  
> +#define VMD_PRIMARY_BUS0 0x00
> +#define VMD_PRIMARY_BUS1 0x80

The above are bus numbers; the below are register offsets.  Would be
nice to separate them with a blank line since they are semantically
different.

I don't understand the difference between VMD_RESTRICT_3_BUS_START and
VMD_PRIMARY_BUS1.  Maybe one is the default Primary Bus Number of the
Root Ports after a reset?

> +#define VMD_BUSRANGE0 0xc8
> +#define VMD_BUSRANGE1 0xcc
> +#define VMD_MEMBAR1_OFFSET 0xd0
> +#define VMD_MEMBAR2_OFFSET1 0xd8
> +#define VMD_MEMBAR2_OFFSET2 0xdc
> +#define VMD_BUS_END(busr) ((busr >> 8) & 0xff)
> +#define VMD_BUS_START(busr) (busr & 0x00ff)

Would be nice if VMD_BUS_END/VMD_BUS_START were defined with
GENMASK(); then we could use FIELD_GET() below to extract them.

Nit: indent these bus numbers and offsets so the values line up like
the other #defines.

> +	 * Starting from Intel Arrow Lake, VMD devices have their VMD rootports
> +	 * connected to additional BUS1 rootport.

This doesn't quite make sense.  Root Ports can't be connected to
another Root Port.  I think you mean "Root Ports on the additional
root bus".