[PATCH 1/2] cdx: Enable COMPILE_TEST

[PATCH 1/2] cdx: Enable COMPILE_TEST

[Rob Herring]

There is no reason CDX needs to depend on ARM64 other than limiting
visibility. So let's also enable building with COMPILE_TEST.

The CONFIG_OF dependency is redundant as ARM64 always enables it and all
the DT functions have empty stubs.

Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/cdx/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cdx/Kconfig b/drivers/cdx/Kconfig
index a08958485e31..7cdb7c414453 100644
--- a/drivers/cdx/Kconfig
+++ b/drivers/cdx/Kconfig
@@ -7,7 +7,7 @@
 
 config CDX_BUS
 	bool "CDX Bus driver"
-	depends on OF && ARM64
+	depends on ARM64 || COMPILE_TEST
 	help
 	  Driver to enable Composable DMA Transfer(CDX) Bus. CDX bus
 	  exposes Fabric devices which uses composable DMA IP to the
-- 
2.42.0

[PATCH 2/2] cdx: Explicitly include correct DT includes, again

[Rob Herring]

The DT of_device.h and of_platform.h date back to the separate
of_platform_bus_type before it was merged into the regular platform bus.
As part of that merge prepping Arm DT support 13 years ago, they
"temporarily" include each other. They also include platform_device.h
and of.h. As a result, there's a pretty much random mix of those include
files used throughout the tree. In order to detangle these headers and
replace the implicit includes with struct declarations, users need to
explicitly include the correct includes.

CDX was fixed once, but commit ("cdx: Remove cdx controller list from cdx
bus system") added another occurrence.

Fixes: 54b406e10f03 ("cdx: Remove cdx controller list from cdx bus system")
Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/cdx/cdx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c
index 4461c6c9313f..d84d153078d7 100644
--- a/drivers/cdx/cdx.c
+++ b/drivers/cdx/cdx.c
@@ -57,7 +57,10 @@
 
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/mm.h>
 #include <linux/idr.h>
-- 
2.42.0

RE: [PATCH 1/2] cdx: Enable COMPILE_TEST

[Agarwal, Nikhil]

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Thursday, December 7, 2023 10:01 PM
> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Gupta, Nipun
> <Nipun.Gupta@amd.com>; Agarwal, Nikhil <nikhil.agarwal@amd.com>
> Cc: linux-kernel@vger.kernel.org
> Subject: [PATCH 1/2] cdx: Enable COMPILE_TEST
> 
> There is no reason CDX needs to depend on ARM64 other than limiting
> visibility. So let's also enable building with COMPILE_TEST.
> 
> The CONFIG_OF dependency is redundant as ARM64 always enables it and all
> the DT functions have empty stubs.
> 
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
>  drivers/cdx/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cdx/Kconfig b/drivers/cdx/Kconfig index
> a08958485e31..7cdb7c414453 100644
> --- a/drivers/cdx/Kconfig
> +++ b/drivers/cdx/Kconfig
> @@ -7,7 +7,7 @@
> 
>  config CDX_BUS
>  	bool "CDX Bus driver"
> -	depends on OF && ARM64
> +	depends on ARM64 || COMPILE_TEST
Hi Rob,

There is a CDX MSI support patch
https://lore.kernel.org/lkml/20231116125609.245206-1-nipun.gupta@amd.com/ which is in
review and is dependent on ARM64( msi_alloc_info_t definition differs on x86). So, the
COMPILE_TEST would break once the MSI changes are added.

Regards
Nikhil

RE: [PATCH 2/2] cdx: Explicitly include correct DT includes, again

[Agarwal, Nikhil]

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Thursday, December 7, 2023 10:01 PM
> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Gupta, Nipun
> <Nipun.Gupta@amd.com>; Agarwal, Nikhil <nikhil.agarwal@amd.com>;
> Gangurde, Abhijit <abhijit.gangurde@amd.com>
> Cc: linux-kernel@vger.kernel.org
> Subject: [PATCH 2/2] cdx: Explicitly include correct DT includes, again
> 
> The DT of_device.h and of_platform.h date back to the separate
> of_platform_bus_type before it was merged into the regular platform bus.
> As part of that merge prepping Arm DT support 13 years ago, they
> "temporarily" include each other. They also include platform_device.h and
> of.h. As a result, there's a pretty much random mix of those include files used
> throughout the tree. In order to detangle these headers and replace the
> implicit includes with struct declarations, users need to explicitly include the
> correct includes.
> 
> CDX was fixed once, but commit ("cdx: Remove cdx controller list from cdx bus
> system") added another occurrence.
> 
> Fixes: 54b406e10f03 ("cdx: Remove cdx controller list from cdx bus system")
> Signed-off-by: Rob Herring <robh@kernel.org>

Acked-by: Nikhil Agarwal <Nikhil.agarwal@amd.com>

Re: [PATCH 1/2] cdx: Enable COMPILE_TEST

[Rob Herring]

On Mon, Dec 11, 2023 at 10:40 PM Agarwal, Nikhil <nikhil.agarwal@amd.com> wrote:
>
> > -----Original Message-----
> > From: Rob Herring <robh@kernel.org>
> > Sent: Thursday, December 7, 2023 10:01 PM
> > To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Gupta, Nipun
> > <Nipun.Gupta@amd.com>; Agarwal, Nikhil <nikhil.agarwal@amd.com>
> > Cc: linux-kernel@vger.kernel.org
> > Subject: [PATCH 1/2] cdx: Enable COMPILE_TEST
> >
> > There is no reason CDX needs to depend on ARM64 other than limiting
> > visibility. So let's also enable building with COMPILE_TEST.
> >
> > The CONFIG_OF dependency is redundant as ARM64 always enables it and all
> > the DT functions have empty stubs.
> >
> > Signed-off-by: Rob Herring <robh@kernel.org>
> > ---
> >  drivers/cdx/Kconfig | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/cdx/Kconfig b/drivers/cdx/Kconfig index
> > a08958485e31..7cdb7c414453 100644
> > --- a/drivers/cdx/Kconfig
> > +++ b/drivers/cdx/Kconfig
> > @@ -7,7 +7,7 @@
> >
> >  config CDX_BUS
> >       bool "CDX Bus driver"
> > -     depends on OF && ARM64
> > +     depends on ARM64 || COMPILE_TEST
> Hi Rob,
>
> There is a CDX MSI support patch
> https://lore.kernel.org/lkml/20231116125609.245206-1-nipun.gupta@amd.com/ which is in
> review and is dependent on ARM64( msi_alloc_info_t definition differs on x86). So, the
> COMPILE_TEST would break once the MSI changes are added.

An ifdef around 'scratchpad' should fix that. It is worthwhile to get
all this to build on x86 allyesconfig builds at least because that is
frequently built by the various CI systems. arm64 is getting there,
but x86 is first for many.

Rob

Re: [PATCH 1/2] cdx: Enable COMPILE_TEST

[Greg Kroah-Hartman]

On Thu, Dec 07, 2023 at 10:31:26AM -0600, Rob Herring wrote:
> There is no reason CDX needs to depend on ARM64 other than limiting
> visibility. So let's also enable building with COMPILE_TEST.
> 
> The CONFIG_OF dependency is redundant as ARM64 always enables it and all
> the DT functions have empty stubs.
> 
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
>  drivers/cdx/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cdx/Kconfig b/drivers/cdx/Kconfig
> index a08958485e31..7cdb7c414453 100644
> --- a/drivers/cdx/Kconfig
> +++ b/drivers/cdx/Kconfig
> @@ -7,7 +7,7 @@
>  
>  config CDX_BUS
>  	bool "CDX Bus driver"
> -	depends on OF && ARM64
> +	depends on ARM64 || COMPILE_TEST

Ok, good start, now we need to turn this into a module, what's keeping
it from being able to be built as a tristate?

thanks,

greg k-h

Re: [PATCH 1/2] cdx: Enable COMPILE_TEST

[Greg Kroah-Hartman]

On Fri, Dec 15, 2023 at 05:09:15PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Dec 07, 2023 at 10:31:26AM -0600, Rob Herring wrote:
> > There is no reason CDX needs to depend on ARM64 other than limiting
> > visibility. So let's also enable building with COMPILE_TEST.
> > 
> > The CONFIG_OF dependency is redundant as ARM64 always enables it and all
> > the DT functions have empty stubs.
> > 
> > Signed-off-by: Rob Herring <robh@kernel.org>
> > ---
> >  drivers/cdx/Kconfig | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/cdx/Kconfig b/drivers/cdx/Kconfig
> > index a08958485e31..7cdb7c414453 100644
> > --- a/drivers/cdx/Kconfig
> > +++ b/drivers/cdx/Kconfig
> > @@ -7,7 +7,7 @@
> >  
> >  config CDX_BUS
> >  	bool "CDX Bus driver"
> > -	depends on OF && ARM64
> > +	depends on ARM64 || COMPILE_TEST
> 
> Ok, good start, now we need to turn this into a module, what's keeping
> it from being able to be built as a tristate?

To answer my own question, the following errors:

ERROR: modpost: missing MODULE_LICENSE() in drivers/cdx/cdx.o
ERROR: modpost: "iommu_device_unuse_default_domain" [drivers/cdx/cdx.ko] undefined!
ERROR: modpost: "iommu_device_use_default_domain" [drivers/cdx/cdx.ko] undefined!

Would be great for someone to fix this up...

thanks,

greg k-h

RE: [PATCH 1/2] cdx: Enable COMPILE_TEST

[Agarwal, Nikhil]

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Tuesday, December 12, 2023 7:56 PM
> To: Agarwal, Nikhil <nikhil.agarwal@amd.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Gupta, Nipun
> <Nipun.Gupta@amd.com>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 1/2] cdx: Enable COMPILE_TEST
> 
> On Mon, Dec 11, 2023 at 10:40 PM Agarwal, Nikhil
> <nikhil.agarwal@amd.com> wrote:
> >
> > > -----Original Message-----
> > > From: Rob Herring <robh@kernel.org>
> > > Sent: Thursday, December 7, 2023 10:01 PM
> > > To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Gupta, Nipun
> > > <Nipun.Gupta@amd.com>; Agarwal, Nikhil <nikhil.agarwal@amd.com>
> > > Cc: linux-kernel@vger.kernel.org
> > > Subject: [PATCH 1/2] cdx: Enable COMPILE_TEST
> > >
> > > There is no reason CDX needs to depend on ARM64 other than limiting
> > > visibility. So let's also enable building with COMPILE_TEST.
> > >
> > > The CONFIG_OF dependency is redundant as ARM64 always enables it and
> > > all the DT functions have empty stubs.
> > >
> > > Signed-off-by: Rob Herring <robh@kernel.org>
> > > ---
> > >  drivers/cdx/Kconfig | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/cdx/Kconfig b/drivers/cdx/Kconfig index
> > > a08958485e31..7cdb7c414453 100644
> > > --- a/drivers/cdx/Kconfig
> > > +++ b/drivers/cdx/Kconfig
> > > @@ -7,7 +7,7 @@
> > >
> > >  config CDX_BUS
> > >       bool "CDX Bus driver"
> > > -     depends on OF && ARM64
> > > +     depends on ARM64 || COMPILE_TEST
> > Hi Rob,
> >
> > There is a CDX MSI support patch
> > https://lore.kernel.org/lkml/20231116125609.245206-1-
> nipun.gupta@amd.c
> > om/ which is in review and is dependent on ARM64( msi_alloc_info_t
> > definition differs on x86). So, the COMPILE_TEST would break once the MSI
> changes are added.
> 
> An ifdef around 'scratchpad' should fix that. It is worthwhile to get all this to
> build on x86 allyesconfig builds at least because that is frequently built by the
> various CI systems. arm64 is getting there, but x86 is first for many.
> 
> Rob

Thanks for the suggestion, Rob. We will change the MSI patch accordingly.

RE: [PATCH 1/2] cdx: Enable COMPILE_TEST

[Agarwal, Nikhil]

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Thursday, December 7, 2023 10:01 PM
> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Gupta, Nipun
> <Nipun.Gupta@amd.com>; Agarwal, Nikhil <nikhil.agarwal@amd.com>
> Cc: linux-kernel@vger.kernel.org
> Subject: [PATCH 1/2] cdx: Enable COMPILE_TEST
> 
> There is no reason CDX needs to depend on ARM64 other than limiting
> visibility. So let's also enable building with COMPILE_TEST.
> 
> The CONFIG_OF dependency is redundant as ARM64 always enables it and all
> the DT functions have empty stubs.
> 
> Signed-off-by: Rob Herring <robh@kernel.org>

Acked-by: Nikhil Agarwal <Nikhil.agarwal@amd.com>

RE: [PATCH 1/2] cdx: Enable COMPILE_TEST

[Gangurde, Abhijit]

> > On Thu, Dec 07, 2023 at 10:31:26AM -0600, Rob Herring wrote:
> > > There is no reason CDX needs to depend on ARM64 other than limiting
> > > visibility. So let's also enable building with COMPILE_TEST.
> > >
> > > The CONFIG_OF dependency is redundant as ARM64 always enables it and
> all
> > > the DT functions have empty stubs.
> > >
> > > Signed-off-by: Rob Herring <robh@kernel.org>
> > > ---
> > >  drivers/cdx/Kconfig | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/cdx/Kconfig b/drivers/cdx/Kconfig
> > > index a08958485e31..7cdb7c414453 100644
> > > --- a/drivers/cdx/Kconfig
> > > +++ b/drivers/cdx/Kconfig
> > > @@ -7,7 +7,7 @@
> > >
> > >  config CDX_BUS
> > >  	bool "CDX Bus driver"
> > > -	depends on OF && ARM64
> > > +	depends on ARM64 || COMPILE_TEST
> >
> > Ok, good start, now we need to turn this into a module, what's keeping
> > it from being able to be built as a tristate?
> 
> To answer my own question, the following errors:
> 
> ERROR: modpost: missing MODULE_LICENSE() in drivers/cdx/cdx.o
> ERROR: modpost: "iommu_device_unuse_default_domain"
> [drivers/cdx/cdx.ko] undefined!
> ERROR: modpost: "iommu_device_use_default_domain" [drivers/cdx/cdx.ko]
> undefined!
> 
> Would be great for someone to fix this up...

I did look at this code. There are 2 issues here for it to be a module.
1. There are many symbols in iommu, msi and other module which are not exported.
Most of other busses like amba, fslmc, pci are bool only.
2. As of now, iommu has static list on bus types (static struct bus_type * const iommu_buses[])
 which is initializes the notifier at init time. So, if we change cdx bus to be a module
cdx devices would miss these mappings.

Thanks,
Abhijit

Re: [PATCH 1/2] cdx: Enable COMPILE_TEST

[Greg Kroah-Hartman]

On Thu, Dec 21, 2023 at 06:42:29AM +0000, Gangurde, Abhijit wrote:
> > > On Thu, Dec 07, 2023 at 10:31:26AM -0600, Rob Herring wrote:
> > > > There is no reason CDX needs to depend on ARM64 other than limiting
> > > > visibility. So let's also enable building with COMPILE_TEST.
> > > >
> > > > The CONFIG_OF dependency is redundant as ARM64 always enables it and
> > all
> > > > the DT functions have empty stubs.
> > > >
> > > > Signed-off-by: Rob Herring <robh@kernel.org>
> > > > ---
> > > >  drivers/cdx/Kconfig | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/cdx/Kconfig b/drivers/cdx/Kconfig
> > > > index a08958485e31..7cdb7c414453 100644
> > > > --- a/drivers/cdx/Kconfig
> > > > +++ b/drivers/cdx/Kconfig
> > > > @@ -7,7 +7,7 @@
> > > >
> > > >  config CDX_BUS
> > > >  	bool "CDX Bus driver"
> > > > -	depends on OF && ARM64
> > > > +	depends on ARM64 || COMPILE_TEST
> > >
> > > Ok, good start, now we need to turn this into a module, what's keeping
> > > it from being able to be built as a tristate?
> > 
> > To answer my own question, the following errors:
> > 
> > ERROR: modpost: missing MODULE_LICENSE() in drivers/cdx/cdx.o
> > ERROR: modpost: "iommu_device_unuse_default_domain"
> > [drivers/cdx/cdx.ko] undefined!
> > ERROR: modpost: "iommu_device_use_default_domain" [drivers/cdx/cdx.ko]
> > undefined!
> > 
> > Would be great for someone to fix this up...
> 
> I did look at this code. There are 2 issues here for it to be a module.
> 1. There are many symbols in iommu, msi and other module which are not exported.
> Most of other busses like amba, fslmc, pci are bool only.

I only see 2 symbols here, what other ones do you see?

> 2. As of now, iommu has static list on bus types (static struct bus_type * const iommu_buses[])
>  which is initializes the notifier at init time. So, if we change cdx bus to be a module
> cdx devices would miss these mappings.

That static bus list needs to be fixed up eventually anyway as we are
moving all bus structures to read-only-memory.  So it's on my todo
list...

Anyway, for now this is ok, but ideally if possible, and if you all want
more build testing, it should be made a module.

thanks,

greg k-h

RE: [PATCH 1/2] cdx: Enable COMPILE_TEST

[Gangurde, Abhijit]

> > > > On Thu, Dec 07, 2023 at 10:31:26AM -0600, Rob Herring wrote:
> > > > > There is no reason CDX needs to depend on ARM64 other than limiting
> > > > > visibility. So let's also enable building with COMPILE_TEST.
> > > > >
> > > > > The CONFIG_OF dependency is redundant as ARM64 always enables it
> and
> > > all
> > > > > the DT functions have empty stubs.
> > > > >
> > > > > Signed-off-by: Rob Herring <robh@kernel.org>
> > > > > ---
> > > > >  drivers/cdx/Kconfig | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/cdx/Kconfig b/drivers/cdx/Kconfig
> > > > > index a08958485e31..7cdb7c414453 100644
> > > > > --- a/drivers/cdx/Kconfig
> > > > > +++ b/drivers/cdx/Kconfig
> > > > > @@ -7,7 +7,7 @@
> > > > >
> > > > >  config CDX_BUS
> > > > >  	bool "CDX Bus driver"
> > > > > -	depends on OF && ARM64
> > > > > +	depends on ARM64 || COMPILE_TEST
> > > >
> > > > Ok, good start, now we need to turn this into a module, what's keeping
> > > > it from being able to be built as a tristate?
> > >
> > > To answer my own question, the following errors:
> > >
> > > ERROR: modpost: missing MODULE_LICENSE() in drivers/cdx/cdx.o
> > > ERROR: modpost: "iommu_device_unuse_default_domain"
> > > [drivers/cdx/cdx.ko] undefined!
> > > ERROR: modpost: "iommu_device_use_default_domain"
> [drivers/cdx/cdx.ko]
> > > undefined!
> > >
> > > Would be great for someone to fix this up...
> >
> > I did look at this code. There are 2 issues here for it to be a module.
> > 1. There are many symbols in iommu, msi and other module which are not
> exported.
> > Most of other busses like amba, fslmc, pci are bool only.
> 
> I only see 2 symbols here, what other ones do you see?

There are ~5 symbols from cdx msi patch

Thanks,
Abhijit

Re: [PATCH 1/2] cdx: Enable COMPILE_TEST

[Greg Kroah-Hartman]

On Thu, Dec 21, 2023 at 08:40:05AM +0000, Gangurde, Abhijit wrote:
> > > > > On Thu, Dec 07, 2023 at 10:31:26AM -0600, Rob Herring wrote:
> > > > > > There is no reason CDX needs to depend on ARM64 other than limiting
> > > > > > visibility. So let's also enable building with COMPILE_TEST.
> > > > > >
> > > > > > The CONFIG_OF dependency is redundant as ARM64 always enables it
> > and
> > > > all
> > > > > > the DT functions have empty stubs.
> > > > > >
> > > > > > Signed-off-by: Rob Herring <robh@kernel.org>
> > > > > > ---
> > > > > >  drivers/cdx/Kconfig | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/cdx/Kconfig b/drivers/cdx/Kconfig
> > > > > > index a08958485e31..7cdb7c414453 100644
> > > > > > --- a/drivers/cdx/Kconfig
> > > > > > +++ b/drivers/cdx/Kconfig
> > > > > > @@ -7,7 +7,7 @@
> > > > > >
> > > > > >  config CDX_BUS
> > > > > >  	bool "CDX Bus driver"
> > > > > > -	depends on OF && ARM64
> > > > > > +	depends on ARM64 || COMPILE_TEST
> > > > >
> > > > > Ok, good start, now we need to turn this into a module, what's keeping
> > > > > it from being able to be built as a tristate?
> > > >
> > > > To answer my own question, the following errors:
> > > >
> > > > ERROR: modpost: missing MODULE_LICENSE() in drivers/cdx/cdx.o
> > > > ERROR: modpost: "iommu_device_unuse_default_domain"
> > > > [drivers/cdx/cdx.ko] undefined!
> > > > ERROR: modpost: "iommu_device_use_default_domain"
> > [drivers/cdx/cdx.ko]
> > > > undefined!
> > > >
> > > > Would be great for someone to fix this up...
> > >
> > > I did look at this code. There are 2 issues here for it to be a module.
> > > 1. There are many symbols in iommu, msi and other module which are not
> > exported.
> > > Most of other busses like amba, fslmc, pci are bool only.
> > 
> > I only see 2 symbols here, what other ones do you see?
> 
> There are ~5 symbols from cdx msi patch

That hasn't been merged yet :)