[PATCH 0/6] rapidio: move Kconfig menu definition to subsystem

[PATCH 0/6] rapidio: move Kconfig menu definition to subsystem

[Alexei Colin]

The top-level Kconfig entry for RapidIO subsystem is currently
duplicated in several architecture-specific Kconfig files. This set of
patches does two things:

1. Move the Kconfig menu definition into the RapidIO subsystem and
remove the duplicate definitions from arch Kconfig files.

2. Enable RapidIO Kconfig menu entry for arm and arm64 architectures,
where it was not enabled before. I tested that subsystem and drivers
build successfully for both architectures, and tested that the modules
load on a custom arm64 Qemu model.

For all architectures, RapidIO menu should be offered when either:
(1) The platform has a PCI bus (which host a RapidIO module on the bus).
(2) The platform has a RapidIO IP block (connected to a system bus, e.g.
AXI on ARM). In this case, 'select HAS_RAPIDIO' should be added to the
'config ARCH_*' menu entry for the SoCs that offer the IP block.

Prior to this patchset, different architectures used different criteria:
* powerpc: (1) and (2)
* mips: (1) and (2) after recent commit into next that added (2):
  https://www.linux-mips.org/archives/linux-mips/2018-07/msg00596.html
  fc5d988878942e9b42a4de5204bdd452f3f1ce47
  491ec1553e0075f345fbe476a93775eabcbc40b6
* x86: (1)
* arm,arm64: none (RapidIO menus never offered)

Responses to feedback from prior submission (thanks for the reviews!):
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-July/593347.html
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-July/593349.html

Changelog:
  * Moved Kconfig entry into RapidIO subsystem instead of duplicating

In the current patchset, I took the approach of adding '|| PCI' to the
depends in the subsystem. I did try the alterantive approach mentioned
in the reviews for v1 of this patch, where the subsystem Kconfig does
not add a '|| PCI' and each per-architecture Kconfig has to add a
'select HAS_RAPIDIO if PCI' and SoCs with IP blocks have to also add
'select HAS_RAPIDIO'. This works too but requires each architecture's
Kconfig to add the line for RapidIO (whereas current approach does not
require that involvement) and also may create a false impression that
the dependency on PCI is strict.

We appreciate the suggestion for also selecting the RapdiIO subsystem for
compilation with COMPILE_TEST, but hope to address it in a separate
patchset, localized to the subsystem, since it will need to change
depends on all drivers, not just on the top level, and since this
patch now spans multiple architectures.


Alexei Colin (6):
  rapidio: define top Kconfig menu in driver subtree
  x86: factor out RapidIO Kconfig menu
  powerpc: factor out RapidIO Kconfig menu entry
  mips: factor out RapidIO Kconfig entry
  arm: enable RapidIO menu in Kconfig
  arm64: enable RapidIO menu in Kconfig

 arch/arm/Kconfig        |  2 ++
 arch/arm64/Kconfig      |  2 ++
 arch/mips/Kconfig       | 11 -----------
 arch/powerpc/Kconfig    | 13 +------------
 arch/x86/Kconfig        |  8 --------
 drivers/rapidio/Kconfig | 15 +++++++++++++++
 6 files changed, 20 insertions(+), 31 deletions(-)

-- 
2.18.0

[PATCH 1/6] rapidio: define top Kconfig menu in driver subtree

[Alexei Colin]

The top-level Kconfig entry for RapidIO subsystem is currently
duplicated in several architecture-specific Kconfigs. This
commit re-defines it in the driver subtree, and subsequent
commits, one per architecture, will remove the duplicated
definitions from respective per-architecture Kconfigs.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: John Paul Walters <jwalters@isi.edu>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Alexei Colin <acolin@isi.edu>
---
 drivers/rapidio/Kconfig | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/rapidio/Kconfig b/drivers/rapidio/Kconfig
index d6d2f20c4597..98e301847584 100644
--- a/drivers/rapidio/Kconfig
+++ b/drivers/rapidio/Kconfig
@@ -1,6 +1,21 @@
 #
 # RapidIO configuration
 #
+
+config HAS_RAPIDIO
+	bool
+	default n
+
+config RAPIDIO
+	tristate "RapidIO support"
+	depends on HAS_RAPIDIO || PCI
+	help
+	  This feature enables support for RapidIO high-performance
+	  packet-switched interconnect.
+
+	  If you say Y here, the kernel will include drivers and
+	  infrastructure code to support RapidIO interconnect devices.
+
 source "drivers/rapidio/devices/Kconfig"
 
 config RAPIDIO_DISC_TIMEOUT
-- 
2.18.0

[PATCH 2/6] x86: factor out RapidIO Kconfig menu

[Alexei Colin]

The menu entry is now defined in the rapidio subtree.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: John Paul Walters <jwalters@isi.edu>
Cc: x86@kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Alexei Colin <acolin@isi.edu>
---
 arch/x86/Kconfig | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 27fce7e84357..6f057000e486 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2828,14 +2828,6 @@ config AMD_NB
 
 source "drivers/pcmcia/Kconfig"
 
-config RAPIDIO
-	tristate "RapidIO support"
-	depends on PCI
-	default n
-	help
-	  If enabled this option will include drivers and the core
-	  infrastructure code to support RapidIO interconnect devices.
-
 source "drivers/rapidio/Kconfig"
 
 config X86_SYSFB
-- 
2.18.0

[PATCH 3/6] powerpc: factor out RapidIO Kconfig menu entry

[Alexei Colin]

The menu entry is now defined in the rapidio subtree.  Also, re-order
the bus menu so tha the platform-specific RapidIO controller appears
after the entry for the RapidIO subsystem.

Platforms with a PCI bus will be offered the RapidIO menu since they may
be want support for a RapidIO PCI device. Platforms without a PCI bus
that might include a RapidIO IP block will need to "select HAS_RAPIDIO"
in the platform-/machine-specific "config ARCH_*" Kconfig entry.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: John Paul Walters <jwalters@isi.edu>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Alexei Colin <acolin@isi.edu>
---
 arch/powerpc/Kconfig | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 25d005af0a5b..17ea8a5f90a0 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -993,16 +993,7 @@ source "drivers/pci/Kconfig"
 
 source "drivers/pcmcia/Kconfig"
 
-config HAS_RAPIDIO
-	bool
-	default n
-
-config RAPIDIO
-	tristate "RapidIO support"
-	depends on HAS_RAPIDIO || PCI
-	help
-	  If you say Y here, the kernel will include drivers and
-	  infrastructure code to support RapidIO interconnect devices.
+source "drivers/rapidio/Kconfig"
 
 config FSL_RIO
 	bool "Freescale Embedded SRIO Controller support"
@@ -1012,8 +1003,6 @@ config FSL_RIO
 	  Include support for RapidIO controller on Freescale embedded
 	  processors (MPC8548, MPC8641, etc).
 
-source "drivers/rapidio/Kconfig"
-
 endmenu
 
 config NONSTATIC_KERNEL
-- 
2.18.0

[PATCH 4/6] mips: factor out RapidIO Kconfig entry

[Alexei Colin]

The menu entry is now defined in the rapidio subtree.

Platforms with a PCI bus will be offered the RapidIO menu since they may
be want support for a RapidIO PCI device. Platforms without a PCI bus
that might include a RapidIO IP block will need to "select HAS_RAPIDIO"
in the platform-/machine-specific "config ARCH_*" Kconfig entry.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Alexander Sverdlin <alexander.sverdlin@nokia.com>
Cc: John Paul Walters <jwalters@isi.edu>
Cc: linux-mips@linux-mips.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Alexei Colin <acolin@isi.edu>
---
 arch/mips/Kconfig | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index 10256056647c..92b9262ee731 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -3106,17 +3106,6 @@ config ZONE_DMA32
 
 source "drivers/pcmcia/Kconfig"
 
-config HAS_RAPIDIO
-	bool
-	default n
-
-config RAPIDIO
-	tristate "RapidIO support"
-	depends on HAS_RAPIDIO || PCI
-	help
-	  If you say Y here, the kernel will include drivers and
-	  infrastructure code to support RapidIO interconnect devices.
-
 source "drivers/rapidio/Kconfig"
 
 endmenu
-- 
2.18.0

[PATCH 5/6] arm: enable RapidIO menu in Kconfig

[Alexei Colin]

Platforms with a PCI bus will be offered the RapidIO menu since they may
be want support for a RapidIO PCI device. Platforms without a PCI bus
that might include a RapidIO IP block will need to "select HAS_RAPIDIO"
in the platform-/machine-specific "config ARCH_*" Kconfig entry.

Tested that kernel builds for ARM with the RapidIO subsystem and switch
drivers enabled.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: John Paul Walters <jwalters@isi.edu>
Cc: linux-kernel@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Alexei Colin <acolin@isi.edu>
---
 arch/arm/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index afe350e5e3d9..602a61324890 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1278,6 +1278,8 @@ config PCI_HOST_ITE8152
 
 source "drivers/pci/Kconfig"
 
+source "drivers/rapidio/Kconfig"
+
 source "drivers/pcmcia/Kconfig"
 
 endmenu
-- 
2.18.0

[PATCH 6/6] arm64: enable RapidIO menu in Kconfig

[Alexei Colin]

Platforms with a PCI bus will be offered the RapidIO menu since they may
be want support for a RapidIO PCI device. Platforms without a PCI bus
that might include a RapidIO IP block will need to "select HAS_RAPIDIO"
in the platform-/machine-specific "config ARCH_*" Kconfig entry.

Tested that kernel builds for arm64 with RapidIO subsystem and
switch drivers enabled, also that the modules load successfully
on a custom Aarch64 Qemu model.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Russell King <linux@armlinux.org.uk>
Cc: John Paul Walters <jwalters@isi.edu>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org,
Signed-off-by: Alexei Colin <acolin@isi.edu>
---
 arch/arm64/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index a8f0c74e6f7f..5e8cf90505ec 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -308,6 +308,8 @@ config PCI_SYSCALL
 
 source "drivers/pci/Kconfig"
 
+source "drivers/rapidio/Kconfig"
+
 endmenu
 
 menu "Kernel Features"
-- 
2.18.0

Re: [PATCH 2/6] x86: factor out RapidIO Kconfig menu;

[Thomas Gleixner]

On Mon, 30 Jul 2018, Alexei Colin wrote:

> The menu entry is now defined in the rapidio subtree.

This could be folded into the patch which actually adds the rapidio
specific Kconfig to avoid kconfig complaining about double entries.

But either way is fine for me.

Acked-by: Thomas Gleixner <tglx@linutronix.de>

> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: John Paul Walters <jwalters@isi.edu>
> Cc: x86@kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Alexei Colin <acolin@isi.edu>
> ---
>  arch/x86/Kconfig | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 27fce7e84357..6f057000e486 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -2828,14 +2828,6 @@ config AMD_NB
>  
>  source "drivers/pcmcia/Kconfig"
>  
> -config RAPIDIO
> -	tristate "RapidIO support"
> -	depends on PCI
> -	default n
> -	help
> -	  If enabled this option will include drivers and the core
> -	  infrastructure code to support RapidIO interconnect devices.
> -
>  source "drivers/rapidio/Kconfig"
>  
>  config X86_SYSFB
> -- 
> 2.18.0
> 
> 

Re: [PATCH 0/6] rapidio: move Kconfig menu definition to subsystem

[Russell King - ARM Linux]

I only have this message, and patches 5 and 6.  This is meaningless
for me to review - I can't tell what you've done as far as my comments
on your previous iteration.

Please arrange to _at least_ copy all patches the appropriate mailing
lists for the set with your complete patch set if you aren't going to
copy everyone on all patches in the set.

On Mon, Jul 30, 2018 at 06:50:28PM -0400, Alexei Colin wrote:
> In the current patchset, I took the approach of adding '|| PCI' to the
> depends in the subsystem. I did try the alterantive approach mentioned
> in the reviews for v1 of this patch, where the subsystem Kconfig does
> not add a '|| PCI' and each per-architecture Kconfig has to add a
> 'select HAS_RAPIDIO if PCI' and SoCs with IP blocks have to also add
> 'select HAS_RAPIDIO'. This works too but requires each architecture's
> Kconfig to add the line for RapidIO (whereas current approach does not
> require that involvement) and also may create a false impression that
> the dependency on PCI is strict.

... which means, as it stands, I've no idea what you actually came up
with for this.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up

Re: [PATCH 0/6] rapidio: move Kconfig menu definition to subsystem

[Randy Dunlap]

On 07/30/2018 03:50 PM, Alexei Colin wrote:
> The top-level Kconfig entry for RapidIO subsystem is currently
> duplicated in several architecture-specific Kconfig files. This set of
> patches does two things:
> 
> 1. Move the Kconfig menu definition into the RapidIO subsystem and
> remove the duplicate definitions from arch Kconfig files.
> 
> 2. Enable RapidIO Kconfig menu entry for arm and arm64 architectures,
> where it was not enabled before. I tested that subsystem and drivers
> build successfully for both architectures, and tested that the modules
> load on a custom arm64 Qemu model.
> 
> For all architectures, RapidIO menu should be offered when either:
> (1) The platform has a PCI bus (which host a RapidIO module on the bus).
> (2) The platform has a RapidIO IP block (connected to a system bus, e.g.
> AXI on ARM). In this case, 'select HAS_RAPIDIO' should be added to the
> 'config ARCH_*' menu entry for the SoCs that offer the IP block.
> 
> Prior to this patchset, different architectures used different criteria:
> * powerpc: (1) and (2)
> * mips: (1) and (2) after recent commit into next that added (2):
>   https://www.linux-mips.org/archives/linux-mips/2018-07/msg00596.html
>   fc5d988878942e9b42a4de5204bdd452f3f1ce47
>   491ec1553e0075f345fbe476a93775eabcbc40b6
> * x86: (1)
> * arm,arm64: none (RapidIO menus never offered)
> 
> Responses to feedback from prior submission (thanks for the reviews!):
> http://lists.infradead.org/pipermail/linux-arm-kernel/2018-July/593347.html
> http://lists.infradead.org/pipermail/linux-arm-kernel/2018-July/593349.html
> 
> Changelog:
>   * Moved Kconfig entry into RapidIO subsystem instead of duplicating
> 
> In the current patchset, I took the approach of adding '|| PCI' to the
> depends in the subsystem. I did try the alterantive approach mentioned
> in the reviews for v1 of this patch, where the subsystem Kconfig does
> not add a '|| PCI' and each per-architecture Kconfig has to add a
> 'select HAS_RAPIDIO if PCI' and SoCs with IP blocks have to also add
> 'select HAS_RAPIDIO'. This works too but requires each architecture's
> Kconfig to add the line for RapidIO (whereas current approach does not
> require that involvement) and also may create a false impression that
> the dependency on PCI is strict.
> 
> We appreciate the suggestion for also selecting the RapdiIO subsystem for
> compilation with COMPILE_TEST, but hope to address it in a separate
> patchset, localized to the subsystem, since it will need to change
> depends on all drivers, not just on the top level, and since this
> patch now spans multiple architectures.
> 
> 
> Alexei Colin (6):
>   rapidio: define top Kconfig menu in driver subtree
>   x86: factor out RapidIO Kconfig menu
>   powerpc: factor out RapidIO Kconfig menu entry
>   mips: factor out RapidIO Kconfig entry
>   arm: enable RapidIO menu in Kconfig
>   arm64: enable RapidIO menu in Kconfig
> 
>  arch/arm/Kconfig        |  2 ++
>  arch/arm64/Kconfig      |  2 ++
>  arch/mips/Kconfig       | 11 -----------
>  arch/powerpc/Kconfig    | 13 +------------
>  arch/x86/Kconfig        |  8 --------
>  drivers/rapidio/Kconfig | 15 +++++++++++++++
>  6 files changed, 20 insertions(+), 31 deletions(-)
> 

LGTM.

Acked-by: Randy Dunlap <rdunlap@infradead.org> # for the series

thanks,
-- 
~Randy

Re: [PATCH 4/6] mips: factor out RapidIO Kconfig entry

[Alexander Sverdlin]

Hi!

On 31/07/18 00:50, Alexei Colin wrote:
> The menu entry is now defined in the rapidio subtree.
> 
> Platforms with a PCI bus will be offered the RapidIO menu since they may
> be want support for a RapidIO PCI device. Platforms without a PCI bus
> that might include a RapidIO IP block will need to "select HAS_RAPIDIO"
> in the platform-/machine-specific "config ARCH_*" Kconfig entry.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> Cc: John Paul Walters <jwalters@isi.edu>
> Cc: linux-mips@linux-mips.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Alexei Colin <acolin@isi.edu>

With patch [1/6] together this looks fine to me.
Reviewed-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>

> ---
>  arch/mips/Kconfig | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> index 10256056647c..92b9262ee731 100644
> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -3106,17 +3106,6 @@ config ZONE_DMA32
>  
>  source "drivers/pcmcia/Kconfig"
>  
> -config HAS_RAPIDIO
> -	bool
> -	default n
> -
> -config RAPIDIO
> -	tristate "RapidIO support"
> -	depends on HAS_RAPIDIO || PCI
> -	help
> -	  If you say Y here, the kernel will include drivers and
> -	  infrastructure code to support RapidIO interconnect devices.
> -
>  source "drivers/rapidio/Kconfig"
>  
>  endmenu

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH 6/6] arm64: enable RapidIO menu in Kconfig

[Will Deacon]

On Mon, Jul 30, 2018 at 06:50:34PM -0400, Alexei Colin wrote:
> Platforms with a PCI bus will be offered the RapidIO menu since they may
> be want support for a RapidIO PCI device. Platforms without a PCI bus
> that might include a RapidIO IP block will need to "select HAS_RAPIDIO"
> in the platform-/machine-specific "config ARCH_*" Kconfig entry.
> 
> Tested that kernel builds for arm64 with RapidIO subsystem and
> switch drivers enabled, also that the modules load successfully
> on a custom Aarch64 Qemu model.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: John Paul Walters <jwalters@isi.edu>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org,
> Signed-off-by: Alexei Colin <acolin@isi.edu>
> ---
>  arch/arm64/Kconfig | 2 ++
>  1 file changed, 2 insertions(+)

Thanks, this looks much cleaner than before:

Acked-by: Will Deacon <will.deacon@arm.com>

The only thing I'm not sure about is why we don't just select HAS_RAPIDIO
unconditionally in the arm64 Kconfig. Does selecting only that option
actually pull in new code to the build?

Will

> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index a8f0c74e6f7f..5e8cf90505ec 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -308,6 +308,8 @@ config PCI_SYSCALL
>  
>  source "drivers/pci/Kconfig"
>  
> +source "drivers/rapidio/Kconfig"
> +
>  endmenu
>  
>  menu "Kernel Features"
> -- 
> 2.18.0
> 

Re: [PATCH 3/6] powerpc: factor out RapidIO Kconfig menu entry

[Michael Ellerman]

Alexei Colin <acolin@isi.edu> writes:

> The menu entry is now defined in the rapidio subtree.  Also, re-order
> the bus menu so tha the platform-specific RapidIO controller appears
> after the entry for the RapidIO subsystem.
>
> Platforms with a PCI bus will be offered the RapidIO menu since they may
> be want support for a RapidIO PCI device. Platforms without a PCI bus
> that might include a RapidIO IP block will need to "select HAS_RAPIDIO"
> in the platform-/machine-specific "config ARCH_*" Kconfig entry.
>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: John Paul Walters <jwalters@isi.edu>
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Alexei Colin <acolin@isi.edu>
> ---
>  arch/powerpc/Kconfig | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)

Looks good.

Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)

cheers

> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 25d005af0a5b..17ea8a5f90a0 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -993,16 +993,7 @@ source "drivers/pci/Kconfig"
>  
>  source "drivers/pcmcia/Kconfig"
>  
> -config HAS_RAPIDIO
> -	bool
> -	default n
> -
> -config RAPIDIO
> -	tristate "RapidIO support"
> -	depends on HAS_RAPIDIO || PCI
> -	help
> -	  If you say Y here, the kernel will include drivers and
> -	  infrastructure code to support RapidIO interconnect devices.
> +source "drivers/rapidio/Kconfig"
>  
>  config FSL_RIO
>  	bool "Freescale Embedded SRIO Controller support"
> @@ -1012,8 +1003,6 @@ config FSL_RIO
>  	  Include support for RapidIO controller on Freescale embedded
>  	  processors (MPC8548, MPC8641, etc).
>  
> -source "drivers/rapidio/Kconfig"
> -
>  endmenu
>  
>  config NONSTATIC_KERNEL
> -- 
> 2.18.0

Re: [PATCH 5/6] arm: enable RapidIO menu in Kconfig

[Christoph Hellwig]

On Mon, Jul 30, 2018 at 06:50:33PM -0400, Alexei Colin wrote:
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index afe350e5e3d9..602a61324890 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1278,6 +1278,8 @@ config PCI_HOST_ITE8152
>  
>  source "drivers/pci/Kconfig"
>  
> +source "drivers/rapidio/Kconfig"

Please include this from drivers/Kconfig instead of enabling it
for a completely random set of architectures.

Re: [PATCH 5/6] arm: enable RapidIO menu in Kconfig

[Alex Bounine]

On 2018-07-31 08:04 AM, Christoph Hellwig wrote:
> On Mon, Jul 30, 2018 at 06:50:33PM -0400, Alexei Colin wrote:
>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> index afe350e5e3d9..602a61324890 100644
>> --- a/arch/arm/Kconfig
>> +++ b/arch/arm/Kconfig
>> @@ -1278,6 +1278,8 @@ config PCI_HOST_ITE8152
>>   
>>   source "drivers/pci/Kconfig"
>>   
>> +source "drivers/rapidio/Kconfig"
> 
> Please include this from drivers/Kconfig instead of enabling it
> for a completely random set of architectures.
> 
This is not a random set of architectures but only ones that implement 
support for RapidIO as system bus.

On some platforms RapidIO can be the only system bus available replacing 
PCI/PCIe.

As it is done now, RapidIO is configured in "Bus Options" (x86/PPC) or 
"Bus Support" (ARMs) sub-menu and from system configuration option it 
should be kept this way.

Current location of RAPIDIO configuration option is familiar to users of 
PowerPC and x86 platforms, and is similarly available in some ARM 
manufacturers kernel code trees.

Re: [PATCH 5/6] arm: enable RapidIO menu in Kconfig

[Russell King - ARM Linux]

On Tue, Jul 31, 2018 at 08:43:02AM -0400, Alex Bounine wrote:
> On 2018-07-31 08:04 AM, Christoph Hellwig wrote:
> >On Mon, Jul 30, 2018 at 06:50:33PM -0400, Alexei Colin wrote:
> >>diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> >>index afe350e5e3d9..602a61324890 100644
> >>--- a/arch/arm/Kconfig
> >>+++ b/arch/arm/Kconfig
> >>@@ -1278,6 +1278,8 @@ config PCI_HOST_ITE8152
> >>  source "drivers/pci/Kconfig"
> >>+source "drivers/rapidio/Kconfig"
> >
> >Please include this from drivers/Kconfig instead of enabling it
> >for a completely random set of architectures.
> >
> This is not a random set of architectures but only ones that implement
> support for RapidIO as system bus.
> 
> On some platforms RapidIO can be the only system bus available replacing
> PCI/PCIe.
> 
> As it is done now, RapidIO is configured in "Bus Options" (x86/PPC) or "Bus
> Support" (ARMs) sub-menu and from system configuration option it should be
> kept this way.
> 
> Current location of RAPIDIO configuration option is familiar to users of
> PowerPC and x86 platforms, and is similarly available in some ARM
> manufacturers kernel code trees.

... which is why you have a HAS_RAPIDIO thing and select it from
arch/*/Kconfig as I suggested in my original review of your patch
set.

As I've also said, I'm unable to review what you've sent this time
around because I don't have all your patches.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up

Re: [PATCH 6/6] arm64: enable RapidIO menu in Kconfig

[Alex Bounine]

On 2018-07-31 04:41 AM, Will Deacon wrote:
> On Mon, Jul 30, 2018 at 06:50:34PM -0400, Alexei Colin wrote:
>> Platforms with a PCI bus will be offered the RapidIO menu since they may
>> be want support for a RapidIO PCI device. Platforms without a PCI bus
>> that might include a RapidIO IP block will need to "select HAS_RAPIDIO"
>> in the platform-/machine-specific "config ARCH_*" Kconfig entry.
>>
>> Tested that kernel builds for arm64 with RapidIO subsystem and
>> switch drivers enabled, also that the modules load successfully
>> on a custom Aarch64 Qemu model.
>>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Russell King <linux@armlinux.org.uk>
>> Cc: John Paul Walters <jwalters@isi.edu>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org,
>> Signed-off-by: Alexei Colin <acolin@isi.edu>
>> ---
>>   arch/arm64/Kconfig | 2 ++
>>   1 file changed, 2 insertions(+)
> 
> Thanks, this looks much cleaner than before:
> 
> Acked-by: Will Deacon <will.deacon@arm.com>
> 
> The only thing I'm not sure about is why we don't just select HAS_RAPIDIO
> unconditionally in the arm64 Kconfig. Does selecting only that option
> actually pull in new code to the build?
> 
HAS_RAPIDIO option is intended for SOCs that have built in SRIO 
controllers, like TI KeyStoneII or FPGAs. Because RapidIO subsystem core 
is required during RapidIO port driver initialization, having separate 
option allows us to control available build options for RapidIO core and 
port driver (bool vs. tristate) and disable module option if port driver 
is configured as built-in.

> Will
> 
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index a8f0c74e6f7f..5e8cf90505ec 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -308,6 +308,8 @@ config PCI_SYSCALL
>>   
>>   source "drivers/pci/Kconfig"
>>   
>> +source "drivers/rapidio/Kconfig"
>> +
>>   endmenu
>>   
>>   menu "Kernel Features"
>> -- 
>> 2.18.0
>>

Re: [PATCH 5/6] arm: enable RapidIO menu in Kconfig

[Alex Bounine]

On 2018-07-31 08:48 AM, Russell King - ARM Linux wrote:
> On Tue, Jul 31, 2018 at 08:43:02AM -0400, Alex Bounine wrote:
>> On 2018-07-31 08:04 AM, Christoph Hellwig wrote:
>>> On Mon, Jul 30, 2018 at 06:50:33PM -0400, Alexei Colin wrote:
>>>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>>>> index afe350e5e3d9..602a61324890 100644
>>>> --- a/arch/arm/Kconfig
>>>> +++ b/arch/arm/Kconfig
>>>> @@ -1278,6 +1278,8 @@ config PCI_HOST_ITE8152
>>>>   source "drivers/pci/Kconfig"
>>>> +source "drivers/rapidio/Kconfig"
>>>
>>> Please include this from drivers/Kconfig instead of enabling it
>>> for a completely random set of architectures.
>>>
>> This is not a random set of architectures but only ones that implement
>> support for RapidIO as system bus.
>>
>> On some platforms RapidIO can be the only system bus available replacing
>> PCI/PCIe.
>>
>> As it is done now, RapidIO is configured in "Bus Options" (x86/PPC) or "Bus
>> Support" (ARMs) sub-menu and from system configuration option it should be
>> kept this way.
>>
>> Current location of RAPIDIO configuration option is familiar to users of
>> PowerPC and x86 platforms, and is similarly available in some ARM
>> manufacturers kernel code trees.
> 
> ... which is why you have a HAS_RAPIDIO thing and select it from
> arch/*/Kconfig as I suggested in my original review of your patch
> set.

Correct, this option will be selected as soon as board that supports it 
is added.

> 
> As I've also said, I'm unable to review what you've sent this time
> around because I don't have all your patches.
> 
I think Alexei will resend full set soon.

Re: [PATCH 0/6] rapidio: move Kconfig menu definition to subsystem

[Alex Bounine]

Acked-by: Alexandre Bounine <alex.bou9@gmail.com>

On 2018-07-30 06:50 PM, Alexei Colin wrote:
> The top-level Kconfig entry for RapidIO subsystem is currently
> duplicated in several architecture-specific Kconfig files. This set of
> patches does two things:
> 
> 1. Move the Kconfig menu definition into the RapidIO subsystem and
> remove the duplicate definitions from arch Kconfig files.
> 
> 2. Enable RapidIO Kconfig menu entry for arm and arm64 architectures,
> where it was not enabled before. I tested that subsystem and drivers
> build successfully for both architectures, and tested that the modules
> load on a custom arm64 Qemu model.
> 
> For all architectures, RapidIO menu should be offered when either:
> (1) The platform has a PCI bus (which host a RapidIO module on the bus).
> (2) The platform has a RapidIO IP block (connected to a system bus, e.g.
> AXI on ARM). In this case, 'select HAS_RAPIDIO' should be added to the
> 'config ARCH_*' menu entry for the SoCs that offer the IP block.
> 
> Prior to this patchset, different architectures used different criteria:
> * powerpc: (1) and (2)
> * mips: (1) and (2) after recent commit into next that added (2):
>    https://www.linux-mips.org/archives/linux-mips/2018-07/msg00596.html
>    fc5d988878942e9b42a4de5204bdd452f3f1ce47
>    491ec1553e0075f345fbe476a93775eabcbc40b6
> * x86: (1)
> * arm,arm64: none (RapidIO menus never offered)
> 
> Responses to feedback from prior submission (thanks for the reviews!):
> http://lists.infradead.org/pipermail/linux-arm-kernel/2018-July/593347.html
> http://lists.infradead.org/pipermail/linux-arm-kernel/2018-July/593349.html
> 
> Changelog:
>    * Moved Kconfig entry into RapidIO subsystem instead of duplicating
> 
> In the current patchset, I took the approach of adding '|| PCI' to the
> depends in the subsystem. I did try the alterantive approach mentioned
> in the reviews for v1 of this patch, where the subsystem Kconfig does
> not add a '|| PCI' and each per-architecture Kconfig has to add a
> 'select HAS_RAPIDIO if PCI' and SoCs with IP blocks have to also add
> 'select HAS_RAPIDIO'. This works too but requires each architecture's
> Kconfig to add the line for RapidIO (whereas current approach does not
> require that involvement) and also may create a false impression that
> the dependency on PCI is strict.
> 
> We appreciate the suggestion for also selecting the RapdiIO subsystem for
> compilation with COMPILE_TEST, but hope to address it in a separate
> patchset, localized to the subsystem, since it will need to change
> depends on all drivers, not just on the top level, and since this
> patch now spans multiple architectures.
> 
> 
> Alexei Colin (6):
>    rapidio: define top Kconfig menu in driver subtree
>    x86: factor out RapidIO Kconfig menu
>    powerpc: factor out RapidIO Kconfig menu entry
>    mips: factor out RapidIO Kconfig entry
>    arm: enable RapidIO menu in Kconfig
>    arm64: enable RapidIO menu in Kconfig
> 
>   arch/arm/Kconfig        |  2 ++
>   arch/arm64/Kconfig      |  2 ++
>   arch/mips/Kconfig       | 11 -----------
>   arch/powerpc/Kconfig    | 13 +------------
>   arch/x86/Kconfig        |  8 --------
>   drivers/rapidio/Kconfig | 15 +++++++++++++++
>   6 files changed, 20 insertions(+), 31 deletions(-)
> 

Re: [PATCH 2/6] x86: factor out RapidIO Kconfig menu;

[Alex Bounine]

On 2018-07-30 06:55 PM, Thomas Gleixner wrote:
> On Mon, 30 Jul 2018, Alexei Colin wrote:
> 
>> The menu entry is now defined in the rapidio subtree.
> 
> This could be folded into the patch which actually adds the rapidio
> specific Kconfig to avoid kconfig complaining about double entries.
>
Please share details how to reproduce double entries warning.

> But either way is fine for me.
> 
> Acked-by: Thomas Gleixner <tglx@linutronix.de>
> 
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: John Paul Walters <jwalters@isi.edu>
>> Cc: x86@kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Alexei Colin <acolin@isi.edu>
>> ---
>>   arch/x86/Kconfig | 8 --------
>>   1 file changed, 8 deletions(-)
>>
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index 27fce7e84357..6f057000e486 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -2828,14 +2828,6 @@ config AMD_NB
>>   
>>   source "drivers/pcmcia/Kconfig"
>>   
>> -config RAPIDIO
>> -	tristate "RapidIO support"
>> -	depends on PCI
>> -	default n
>> -	help
>> -	  If enabled this option will include drivers and the core
>> -	  infrastructure code to support RapidIO interconnect devices.
>> -
>>   source "drivers/rapidio/Kconfig"
>>   
>>   config X86_SYSFB
>> -- 
>> 2.18.0
>>
>>

Re: [PATCH 6/6] arm64: enable RapidIO menu in Kconfig

[Russell King - ARM Linux]

On Tue, Jul 31, 2018 at 08:54:14AM -0400, Alex Bounine wrote:
> On 2018-07-31 04:41 AM, Will Deacon wrote:
> >On Mon, Jul 30, 2018 at 06:50:34PM -0400, Alexei Colin wrote:
> >>Platforms with a PCI bus will be offered the RapidIO menu since they may
> >>be want support for a RapidIO PCI device. Platforms without a PCI bus
> >>that might include a RapidIO IP block will need to "select HAS_RAPIDIO"
> >>in the platform-/machine-specific "config ARCH_*" Kconfig entry.
> >>
> >>Tested that kernel builds for arm64 with RapidIO subsystem and
> >>switch drivers enabled, also that the modules load successfully
> >>on a custom Aarch64 Qemu model.
> >>
> >>Cc: Andrew Morton <akpm@linux-foundation.org>
> >>Cc: Russell King <linux@armlinux.org.uk>
> >>Cc: John Paul Walters <jwalters@isi.edu>
> >>Cc: linux-arm-kernel@lists.infradead.org
> >>Cc: linux-kernel@vger.kernel.org,
> >>Signed-off-by: Alexei Colin <acolin@isi.edu>
> >>---
> >>  arch/arm64/Kconfig | 2 ++
> >>  1 file changed, 2 insertions(+)
> >
> >Thanks, this looks much cleaner than before:
> >
> >Acked-by: Will Deacon <will.deacon@arm.com>
> >
> >The only thing I'm not sure about is why we don't just select HAS_RAPIDIO
> >unconditionally in the arm64 Kconfig. Does selecting only that option
> >actually pull in new code to the build?
> >
> HAS_RAPIDIO option is intended for SOCs that have built in SRIO controllers,
> like TI KeyStoneII or FPGAs. Because RapidIO subsystem core is required
> during RapidIO port driver initialization, having separate option allows us
> to control available build options for RapidIO core and port driver (bool
> vs. tristate) and disable module option if port driver is configured as
> built-in.

Your explanation doesn't make much sense to me.

RAPIDIO is the bus-level support, right?  So drivers that depend on
the bus-level support should depend on RAPIDIO, and so, if RAPIDIO
is configured as a module, they will also be allowed to be disabled
or a module, but not built-in if tristate.  If it is boolean, and
causes the driver to be built-in to the kernel, then you need to use
"RAPIDIO=y" so that it's dependency is only satisfied when the core
is built-in.

HAS_RAPIDIO gives the impression that it defines whether or not
the rapidio core code is allowable or not - it doesn't suggest that
it has anything to do with drivers.  However, reading the PowerPC
Kconfig files, it seems to be used that way.  That's confusing, and
ought to be fixed.  From what I can tell, it's only used for FSL_RIO,
so I suggest that gets converted to:

config HAS_RAPIDIO
	bool PCI

config RAPIDIO
	tristate "RapidIO support"
	depends on HAS_RAPIDIO

config HAS_FSL_RIO
	bool
	select HAS_RAPIDIO

config FSL_RIO
	bool "Freescale Embedded SRIO Controller support"
	depends on RAPIDIO = y && HAS_FSL_RIO

This frees up HAS_RAPIDIO to operate as one would expect - to define
whether or not RAPIDIO should be offered.  This also allows:

config ARM
	select HAS_RAPIDIO if PCI

to be added to arch/arm/Kconfig if appropriate.  However, I'm not yet
convinced that _just because_ we have PCI does not mean that RAPIDIO
should be offered.  I stated a series of questions about that last
Tuesday in response to an individual patch adding rapidio to arch/arm,
and that email seems to have been ignored - at least as far as the
questions go.

Please ensure that you respond to your reviewers questions, otherwise
you will start receiving plain NAKs to your patches instead (since
it becomes a waste of time for reviewers to put any further effort
in to explain why they don't like the patch.)

Thanks.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up

Re: [PATCH 6/6] arm64: enable RapidIO menu in Kconfig

[Alex Bounine]

On 2018-07-31 11:52 AM, Russell King - ARM Linux wrote:
> On Tue, Jul 31, 2018 at 08:54:14AM -0400, Alex Bounine wrote:
>> On 2018-07-31 04:41 AM, Will Deacon wrote:
>>> On Mon, Jul 30, 2018 at 06:50:34PM -0400, Alexei Colin wrote:
>>>> Platforms with a PCI bus will be offered the RapidIO menu since they may
>>>> be want support for a RapidIO PCI device. Platforms without a PCI bus
>>>> that might include a RapidIO IP block will need to "select HAS_RAPIDIO"
>>>> in the platform-/machine-specific "config ARCH_*" Kconfig entry.
>>>>
>>>> Tested that kernel builds for arm64 with RapidIO subsystem and
>>>> switch drivers enabled, also that the modules load successfully
>>>> on a custom Aarch64 Qemu model.
>>>>
>>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>>> Cc: Russell King <linux@armlinux.org.uk>
>>>> Cc: John Paul Walters <jwalters@isi.edu>
>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>> Cc: linux-kernel@vger.kernel.org,
>>>> Signed-off-by: Alexei Colin <acolin@isi.edu>
>>>> ---
>>>>   arch/arm64/Kconfig | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>
>>> Thanks, this looks much cleaner than before:
>>>
>>> Acked-by: Will Deacon <will.deacon@arm.com>
>>>
>>> The only thing I'm not sure about is why we don't just select HAS_RAPIDIO
>>> unconditionally in the arm64 Kconfig. Does selecting only that option
>>> actually pull in new code to the build?
>>>
>> HAS_RAPIDIO option is intended for SOCs that have built in SRIO controllers,
>> like TI KeyStoneII or FPGAs. Because RapidIO subsystem core is required
>> during RapidIO port driver initialization, having separate option allows us
>> to control available build options for RapidIO core and port driver (bool
>> vs. tristate) and disable module option if port driver is configured as
>> built-in.
> 
> Your explanation doesn't make much sense to me.
> 
> RAPIDIO is the bus-level support, right?  So drivers that depend on
> the bus-level support should depend on RAPIDIO, and so, if RAPIDIO
> is configured as a module, they will also be allowed to be disabled
> or a module, but not built-in if tristate.  If it is boolean, and
> causes the driver to be built-in to the kernel, then you need to use
> "RAPIDIO=y" so that it's dependency is only satisfied when the core
> is built-in.
> 

RapidIO host controllers (on local bus like SoC internal or PCIe) are 
serviced by MPORT device drivers that are subsystem specific and 
communicate with RapidIO core using set of callbacks. Depending on HW 
architecture these drivers may be defined as built-in or module.
Built-in driver will require presence of the RapidIO core during its 
initialization time and therefore we have to set CONFIG_RAPIDIO=y.
Also we have PCIe based host controllers and their drivers are OK to be 
built as module like for any other PCI device.

Based on requirements and available resources/HW_configuration the 
platform can have on-chip host controller enabled or disabled (or simply 
not implemented in case of FPGA). This leads us to combination of 
on-chip and PCIe host controllers. For example, if PCIe bus is required 
for other devices, we may have to use PCIe-to_SRIO bridge because all 
available on-chip SerDes lines are assigned to PCIe.

If we use CONFIG_RAPIDIO as a single indicator of possible 
configuration, we can make visible config menu entry for on-chip 
controller that is not available on given platform due to HW setup. For 
example on KeystoneII or MPC85xx/86xx. The option HAS_RAPIDIO tells us 
that given platform really uses on-chip RapidIO host controller. This is 
why FSL_RIO depends on HAS_RAPIDIO.

Also having PCIe enabled in any form is not a good option to control 
support for on-chip controller.

For peripheral devices attached to the RapidIO fabric such dependency on 
local mport implementation does not exist and therefore they all can be 
treated as tristate.

> HAS_RAPIDIO gives the impression that it defines whether or not
> the rapidio core code is allowable or not - it doesn't suggest that
> it has anything to do with drivers. However, reading the PowerPC
> Kconfig files, it seems to be used that way.  That's confusing, and
> ought to be fixed.  From what I can tell, it's only used for FSL_RIO,
> so I suggest that gets converted to:
> 
> config HAS_RAPIDIO
> 	bool PCI
> 
> config RAPIDIO
> 	tristate "RapidIO support"
> 	depends on HAS_RAPIDIO
> 
> config HAS_FSL_RIO
> 	bool
> 	select HAS_RAPIDIO
> 
> config FSL_RIO
> 	bool "Freescale Embedded SRIO Controller support"
> 	depends on RAPIDIO = y && HAS_FSL_RIO
> 
> This frees up HAS_RAPIDIO to operate as one would expect - to define
> whether or not RAPIDIO should be offered.  This also allows:
> 
> config ARM
> 	select HAS_RAPIDIO if PCI
> 
> to be added to arch/arm/Kconfig if appropriate.  However, I'm not yet
> convinced that _just because_ we have PCI does not mean that RAPIDIO
> should be offered.  I stated a series of questions about that last
> Tuesday in response to an individual patch adding rapidio to arch/arm,
> and that email seems to have been ignored - at least as far as the
> questions go.
> 
> Please ensure that you respond to your reviewers questions, otherwise
> you will start receiving plain NAKs to your patches instead (since
> it becomes a waste of time for reviewers to put any further effort
> in to explain why they don't like the patch.)
> 
> Thanks.
> 

Re: [PATCH 6/6] arm64: enable RapidIO menu in Kconfig

[Russell King - ARM Linux]

On Tue, Jul 31, 2018 at 01:59:27PM -0400, Alex Bounine wrote:
> On 2018-07-31 11:52 AM, Russell King - ARM Linux wrote:
> >On Tue, Jul 31, 2018 at 08:54:14AM -0400, Alex Bounine wrote:
> >>On 2018-07-31 04:41 AM, Will Deacon wrote:
> >>>On Mon, Jul 30, 2018 at 06:50:34PM -0400, Alexei Colin wrote:
> >>>>Platforms with a PCI bus will be offered the RapidIO menu since they may
> >>>>be want support for a RapidIO PCI device. Platforms without a PCI bus
> >>>>that might include a RapidIO IP block will need to "select HAS_RAPIDIO"
> >>>>in the platform-/machine-specific "config ARCH_*" Kconfig entry.
> >>>>
> >>>>Tested that kernel builds for arm64 with RapidIO subsystem and
> >>>>switch drivers enabled, also that the modules load successfully
> >>>>on a custom Aarch64 Qemu model.
> >>>>
> >>>>Cc: Andrew Morton <akpm@linux-foundation.org>
> >>>>Cc: Russell King <linux@armlinux.org.uk>
> >>>>Cc: John Paul Walters <jwalters@isi.edu>
> >>>>Cc: linux-arm-kernel@lists.infradead.org
> >>>>Cc: linux-kernel@vger.kernel.org,
> >>>>Signed-off-by: Alexei Colin <acolin@isi.edu>
> >>>>---
> >>>>  arch/arm64/Kconfig | 2 ++
> >>>>  1 file changed, 2 insertions(+)
> >>>
> >>>Thanks, this looks much cleaner than before:
> >>>
> >>>Acked-by: Will Deacon <will.deacon@arm.com>
> >>>
> >>>The only thing I'm not sure about is why we don't just select HAS_RAPIDIO
> >>>unconditionally in the arm64 Kconfig. Does selecting only that option
> >>>actually pull in new code to the build?
> >>>
> >>HAS_RAPIDIO option is intended for SOCs that have built in SRIO controllers,
> >>like TI KeyStoneII or FPGAs. Because RapidIO subsystem core is required
> >>during RapidIO port driver initialization, having separate option allows us
> >>to control available build options for RapidIO core and port driver (bool
> >>vs. tristate) and disable module option if port driver is configured as
> >>built-in.
> >
> >Your explanation doesn't make much sense to me.
> >
> >RAPIDIO is the bus-level support, right?  So drivers that depend on
> >the bus-level support should depend on RAPIDIO, and so, if RAPIDIO
> >is configured as a module, they will also be allowed to be disabled
> >or a module, but not built-in if tristate.  If it is boolean, and
> >causes the driver to be built-in to the kernel, then you need to use
> >"RAPIDIO=y" so that it's dependency is only satisfied when the core
> >is built-in.
> >
> 
> RapidIO host controllers (on local bus like SoC internal or PCIe) are
> serviced by MPORT device drivers that are subsystem specific and communicate
> with RapidIO core using set of callbacks. Depending on HW architecture these
> drivers may be defined as built-in or module.

Why does hardware architecture define whether something has to be built
in or can be modular?

It is the case today that (eg) on-SoC hardware _can_ be built as a module
if desired - just because it's on the SoC does not mean it has to be
built in to the kernel.  Why is RapidIO any different?

> Built-in driver will require presence of the RapidIO core during its
> initialization time and therefore we have to set CONFIG_RAPIDIO=y.
> Also we have PCIe based host controllers and their drivers are OK to be
> built as module like for any other PCI device.
> 
> Based on requirements and available resources/HW_configuration the platform
> can have on-chip host controller enabled or disabled (or simply not
> implemented in case of FPGA). This leads us to combination of on-chip and
> PCIe host controllers. For example, if PCIe bus is required for other
> devices, we may have to use PCIe-to_SRIO bridge because all available
> on-chip SerDes lines are assigned to PCIe.
> 
> If we use CONFIG_RAPIDIO as a single indicator of possible configuration, we
> can make visible config menu entry for on-chip controller that is not
> available on given platform due to HW setup. For example on KeystoneII or
> MPC85xx/86xx. The option HAS_RAPIDIO tells us that given platform really
> uses on-chip RapidIO host controller. This is why FSL_RIO depends on
> HAS_RAPIDIO.
> 
> Also having PCIe enabled in any form is not a good option to control support
> for on-chip controller.

I'm not saying that - can you please read my suggestion below and
respond to that.  I'm giving you technical feedback, but it seems
all I'm getting back is "this is how we're doing it" rather than a
constructive discussion.

For example, can you point out why my idea I present below would not
work?

> For peripheral devices attached to the RapidIO fabric such dependency on
> local mport implementation does not exist and therefore they all can be
> treated as tristate.
> 
> >HAS_RAPIDIO gives the impression that it defines whether or not
> >the rapidio core code is allowable or not - it doesn't suggest that
> >it has anything to do with drivers. However, reading the PowerPC
> >Kconfig files, it seems to be used that way.  That's confusing, and
> >ought to be fixed.  From what I can tell, it's only used for FSL_RIO,
> >so I suggest that gets converted to:
> >
> >config HAS_RAPIDIO
> >	bool PCI
> >
> >config RAPIDIO
> >	tristate "RapidIO support"
> >	depends on HAS_RAPIDIO
> >
> >config HAS_FSL_RIO
> >	bool
> >	select HAS_RAPIDIO
> >
> >config FSL_RIO
> >	bool "Freescale Embedded SRIO Controller support"
> >	depends on RAPIDIO = y && HAS_FSL_RIO
> >
> >This frees up HAS_RAPIDIO to operate as one would expect - to define
> >whether or not RAPIDIO should be offered.  This also allows:
> >
> >config ARM
> >	select HAS_RAPIDIO if PCI
> >
> >to be added to arch/arm/Kconfig if appropriate.  However, I'm not yet
> >convinced that _just because_ we have PCI does not mean that RAPIDIO
> >should be offered.  I stated a series of questions about that last
> >Tuesday in response to an individual patch adding rapidio to arch/arm,
> >and that email seems to have been ignored - at least as far as the
> >questions go.
> >
> >Please ensure that you respond to your reviewers questions, otherwise
> >you will start receiving plain NAKs to your patches instead (since
> >it becomes a waste of time for reviewers to put any further effort
> >in to explain why they don't like the patch.)
> >
> >Thanks.
> >
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up

Re: [PATCH 6/6] arm64: enable RapidIO menu in Kconfig

[Alex Bounine]

On 2018-07-31 02:18 PM, Russell King - ARM Linux wrote:
> On Tue, Jul 31, 2018 at 01:59:27PM -0400, Alex Bounine wrote:
>> On 2018-07-31 11:52 AM, Russell King - ARM Linux wrote:
>>> On Tue, Jul 31, 2018 at 08:54:14AM -0400, Alex Bounine wrote:
>>>> On 2018-07-31 04:41 AM, Will Deacon wrote:
>>>>> On Mon, Jul 30, 2018 at 06:50:34PM -0400, Alexei Colin wrote:
>>>>>> Platforms with a PCI bus will be offered the RapidIO menu since they may
>>>>>> be want support for a RapidIO PCI device. Platforms without a PCI bus
>>>>>> that might include a RapidIO IP block will need to "select HAS_RAPIDIO"
>>>>>> in the platform-/machine-specific "config ARCH_*" Kconfig entry.
>>>>>>
>>>>>> Tested that kernel builds for arm64 with RapidIO subsystem and
>>>>>> switch drivers enabled, also that the modules load successfully
>>>>>> on a custom Aarch64 Qemu model.
>>>>>>
>>>>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>>>>> Cc: Russell King <linux@armlinux.org.uk>
>>>>>> Cc: John Paul Walters <jwalters@isi.edu>
>>>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>>>> Cc: linux-kernel@vger.kernel.org,
>>>>>> Signed-off-by: Alexei Colin <acolin@isi.edu>
>>>>>> ---
>>>>>>   arch/arm64/Kconfig | 2 ++
>>>>>>   1 file changed, 2 insertions(+)
>>>>>
>>>>> Thanks, this looks much cleaner than before:
>>>>>
>>>>> Acked-by: Will Deacon <will.deacon@arm.com>
>>>>>
>>>>> The only thing I'm not sure about is why we don't just select HAS_RAPIDIO
>>>>> unconditionally in the arm64 Kconfig. Does selecting only that option
>>>>> actually pull in new code to the build?
>>>>>
>>>> HAS_RAPIDIO option is intended for SOCs that have built in SRIO controllers,
>>>> like TI KeyStoneII or FPGAs. Because RapidIO subsystem core is required
>>>> during RapidIO port driver initialization, having separate option allows us
>>>> to control available build options for RapidIO core and port driver (bool
>>>> vs. tristate) and disable module option if port driver is configured as
>>>> built-in.
>>>
>>> Your explanation doesn't make much sense to me.
>>>
>>> RAPIDIO is the bus-level support, right?  So drivers that depend on
>>> the bus-level support should depend on RAPIDIO, and so, if RAPIDIO
>>> is configured as a module, they will also be allowed to be disabled
>>> or a module, but not built-in if tristate.  If it is boolean, and
>>> causes the driver to be built-in to the kernel, then you need to use
>>> "RAPIDIO=y" so that it's dependency is only satisfied when the core
>>> is built-in.
>>>
>>
>> RapidIO host controllers (on local bus like SoC internal or PCIe) are
>> serviced by MPORT device drivers that are subsystem specific and communicate
>> with RapidIO core using set of callbacks. Depending on HW architecture these
>> drivers may be defined as built-in or module.
> 
> Why does hardware architecture define whether something has to be built
> in or can be modular?
> 
> It is the case today that (eg) on-SoC hardware _can_ be built as a module
> if desired - just because it's on the SoC does not mean it has to be
> built in to the kernel.  Why is RapidIO any different?
>

Not HW architecture - legacy can be blamed as well. Freescale's FSL_RIO 
driver still exist as built-in so far. Intent of this patch set is to 
allow RapidIO support in more architectures without reworking old stuff.
I do not think that anyone will be updating FSL_RIO driver soon.

Also we cannot dictate to developers of future drivers which method to 
use - they may have built-in option only for the first release.

Unfortunately we have on hands dependency between mport driver build 
mode and  RapidIO core which we need to respect.


>> Built-in driver will require presence of the RapidIO core during its
>> initialization time and therefore we have to set CONFIG_RAPIDIO=y.
>> Also we have PCIe based host controllers and their drivers are OK to be
>> built as module like for any other PCI device.
>>
>> Based on requirements and available resources/HW_configuration the platform
>> can have on-chip host controller enabled or disabled (or simply not
>> implemented in case of FPGA). This leads us to combination of on-chip and
>> PCIe host controllers. For example, if PCIe bus is required for other
>> devices, we may have to use PCIe-to_SRIO bridge because all available
>> on-chip SerDes lines are assigned to PCIe.
>>
>> If we use CONFIG_RAPIDIO as a single indicator of possible configuration, we
>> can make visible config menu entry for on-chip controller that is not
>> available on given platform due to HW setup. For example on KeystoneII or
>> MPC85xx/86xx. The option HAS_RAPIDIO tells us that given platform really
>> uses on-chip RapidIO host controller. This is why FSL_RIO depends on
>> HAS_RAPIDIO.
>>
>> Also having PCIe enabled in any form is not a good option to control support
>> for on-chip controller.
> 
> I'm not saying that - can you please read my suggestion below and
> respond to that.  I'm giving you technical feedback, but it seems
> all I'm getting back is "this is how we're doing it" rather than a
> constructive discussion.

I am trying to explain why we are doing it this way, not how.

> 
> For example, can you point out why my idea I present below would not
> work?

See below.

> 
>> For peripheral devices attached to the RapidIO fabric such dependency on
>> local mport implementation does not exist and therefore they all can be
>> treated as tristate.
>>
>>> HAS_RAPIDIO gives the impression that it defines whether or not
>>> the rapidio core code is allowable or not - it doesn't suggest that
>>> it has anything to do with drivers. However, reading the PowerPC
>>> Kconfig files, it seems to be used that way.  That's confusing, and
>>> ought to be fixed.  From what I can tell, it's only used for FSL_RIO,
>>> so I suggest that gets converted to:
>>>
>>> config HAS_RAPIDIO
>>> 	bool PCI

PCI and RAPIDIO can be mutually exclusive.

>>>
>>> config RAPIDIO
>>> 	tristate "RapidIO support"
>>> 	depends on HAS_RAPIDIO
>>>
>>> config HAS_FSL_RIO
>>> 	bool
>>> 	select HAS_RAPIDIO

Introducing new variable HAS_FSL_RIO here. Do you suggest having one for 
each ARM-based board that has on-chip RIO?

>>>
>>> config FSL_RIO
>>> 	bool "Freescale Embedded SRIO Controller support"
>>> 	depends on RAPIDIO = y && HAS_FSL_RIO
>>>
>>> This frees up HAS_RAPIDIO to operate as one would expect - to define
>>> whether or not RAPIDIO should be offered.  This also allows:
>>>
>>> config ARM
>>> 	select HAS_RAPIDIO if PCI

Some SOCs can be configured without PCI. We have confusing action here - 
we have to enable PCI on platform that does not have it.

Above, you had to introduce HAS_FSL_RIO. The same will be needed for 
each ARM-based platform that supports RapidIO. E.g. HAS_KS2_RIO or 
HAS_XZINQ_RIO.

How this improves things? Right now HAS_RAPIDIO is single option that 
has to be selected for given board (plus enabling specific mport option).

Why we cannot use "select HAS_RAPIDIO" HW-specific Kconfig file 
(mach-*/Kconfig)? And have on-chip port selection in the same 
board-specific place.

This is the right place because chips like Keystone have an internal 
RapidIO controller specific to them. If you got with any type of global

The same for Xilinx. Other platforms unlock RAPIDIO selection if they 
have PCI.

Having HAS_RAPIDIO directly in the board-specific Kconfig looks to me 
more descriptive for end user as well.

>>>
>>> to be added to arch/arm/Kconfig if appropriate.  However, I'm not yet
>>> convinced that _just because_ we have PCI does not mean that RAPIDIO
>>> should be offered.  I stated a series of questions about that last
>>> Tuesday in response to an individual patch adding rapidio to arch/arm,
>>> and that email seems to have been ignored - at least as far as the
>>> questions go.
>>>
>>> Please ensure that you respond to your reviewers questions, otherwise
>>> you will start receiving plain NAKs to your patches instead (since
>>> it becomes a waste of time for reviewers to put any further effort
>>> in to explain why they don't like the patch.)
>>>
>>> Thanks.
>>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH 6/6] arm64: enable RapidIO menu in Kconfig

[Alex Bounine]

On 2018-07-31 08:54 AM, Alex Bounine wrote:
> On 2018-07-31 04:41 AM, Will Deacon wrote:
>> On Mon, Jul 30, 2018 at 06:50:34PM -0400, Alexei Colin wrote:
>>> Platforms with a PCI bus will be offered the RapidIO menu since they may
>>> be want support for a RapidIO PCI device. Platforms without a PCI bus
>>> that might include a RapidIO IP block will need to "select HAS_RAPIDIO"
>>> in the platform-/machine-specific "config ARCH_*" Kconfig entry.
>>>
>>> Tested that kernel builds for arm64 with RapidIO subsystem and
>>> switch drivers enabled, also that the modules load successfully
>>> on a custom Aarch64 Qemu model.
>>>
>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>> Cc: Russell King <linux@armlinux.org.uk>
>>> Cc: John Paul Walters <jwalters@isi.edu>
>>> Cc: linux-arm-kernel@lists.infradead.org
>>> Cc: linux-kernel@vger.kernel.org,
>>> Signed-off-by: Alexei Colin <acolin@isi.edu>
>>> ---
>>>   arch/arm64/Kconfig | 2 ++
>>>   1 file changed, 2 insertions(+)
>>
>> Thanks, this looks much cleaner than before:
>>
>> Acked-by: Will Deacon <will.deacon@arm.com>
>>
>> The only thing I'm not sure about is why we don't just select HAS_RAPIDIO
>> unconditionally in the arm64 Kconfig. Does selecting only that option
>> actually pull in new code to the build?
>>
> HAS_RAPIDIO option is intended for SOCs that have built in SRIO 
> controllers, like TI KeyStoneII or FPGAs. Because RapidIO subsystem core 
> is required during RapidIO port driver initialization, having separate 
> option allows us to control available build options for RapidIO core and 
> port driver (bool vs. tristate) and disable module option if port driver 
> is configured as built-in.

I am thinking about where HAS_RAPIDIO option can be set for arm64 
branch. Having it set globally is too broad. For example we have Xilinx 
Zinq US board with SRIO IP on it. Having it globally in arm64 branch - 
bad. Probably having it set in drivers/soc/... is the best place.

Will, Alexei what do you think?


> 
>> Will
>>
>>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>>> index a8f0c74e6f7f..5e8cf90505ec 100644
>>> --- a/arch/arm64/Kconfig
>>> +++ b/arch/arm64/Kconfig
>>> @@ -308,6 +308,8 @@ config PCI_SYSCALL
>>>   source "drivers/pci/Kconfig"
>>> +source "drivers/rapidio/Kconfig"
>>> +
>>>   endmenu
>>>   menu "Kernel Features"
>>> -- 
>>> 2.18.0
>>>

Re: [PATCH 6/6] arm64: enable RapidIO menu in Kconfig

[Alexei Colin]

On Tue, Jul 31, 2018 at 04:29:56PM -0400, Alex Bounine wrote:
> On 2018-07-31 08:54 AM, Alex Bounine wrote:
> > On 2018-07-31 04:41 AM, Will Deacon wrote:
> > > On Mon, Jul 30, 2018 at 06:50:34PM -0400, Alexei Colin wrote:
> > > > Platforms with a PCI bus will be offered the RapidIO menu since they may
> > > > be want support for a RapidIO PCI device. Platforms without a PCI bus
> > > > that might include a RapidIO IP block will need to "select HAS_RAPIDIO"
> > > > in the platform-/machine-specific "config ARCH_*" Kconfig entry.
> > > > 
> > > > Tested that kernel builds for arm64 with RapidIO subsystem and
> > > > switch drivers enabled, also that the modules load successfully
> > > > on a custom Aarch64 Qemu model.
> > > > 
> > > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > > Cc: Russell King <linux@armlinux.org.uk>
> > > > Cc: John Paul Walters <jwalters@isi.edu>
> > > > Cc: linux-arm-kernel@lists.infradead.org
> > > > Cc: linux-kernel@vger.kernel.org,
> > > > Signed-off-by: Alexei Colin <acolin@isi.edu>
> > > > ---
> > > >   arch/arm64/Kconfig | 2 ++
> > > >   1 file changed, 2 insertions(+)
> > > 
> > > Thanks, this looks much cleaner than before:
> > > 
> > > Acked-by: Will Deacon <will.deacon@arm.com>
> > > 
> > > The only thing I'm not sure about is why we don't just select HAS_RAPIDIO
> > > unconditionally in the arm64 Kconfig. Does selecting only that option
> > > actually pull in new code to the build?
> > > 
> > HAS_RAPIDIO option is intended for SOCs that have built in SRIO
> > controllers, like TI KeyStoneII or FPGAs. Because RapidIO subsystem core
> > is required during RapidIO port driver initialization, having separate
> > option allows us to control available build options for RapidIO core and
> > port driver (bool vs. tristate) and disable module option if port driver
> > is configured as built-in.
> 
> I am thinking about where HAS_RAPIDIO option can be set for arm64 branch.
> Having it set globally is too broad. For example we have Xilinx Zinq US
> board with SRIO IP on it. Having it globally in arm64 branch - bad. Probably
> having it set in drivers/soc/... is the best place.
> 
> Will, Alexei what do you think?

Since the HAS_RAPIODIO flag adds meta info about SoC, maybe the line
that 'select's it can go where the rest of the meta info is, which
differs across architectures:
* For ARM64, in arch/arm64/Kconfig.platforms under each config ARCH_*
for each SoC that includes RapidIO.
* For ARM, in arch/arm/mach-*/Kconfig

But, if we want the flag to be automatically selected for some larger
set of ARM64 SoCs without explicitly adding it to each one, then the
above is not going to do that.

> > > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > > > index a8f0c74e6f7f..5e8cf90505ec 100644
> > > > --- a/arch/arm64/Kconfig
> > > > +++ b/arch/arm64/Kconfig
> > > > @@ -308,6 +308,8 @@ config PCI_SYSCALL
> > > >   source "drivers/pci/Kconfig"
> > > > +source "drivers/rapidio/Kconfig"
> > > > +
> > > >   endmenu
> > > >   menu "Kernel Features"
> > > > -- 
> > > > 2.18.0
> > > > 

Re: [PATCH 6/6] arm64: enable RapidIO menu in Kconfig

[Will Deacon]

On Tue, Jul 31, 2018 at 04:29:56PM -0400, Alex Bounine wrote:
> On 2018-07-31 08:54 AM, Alex Bounine wrote:
> >On 2018-07-31 04:41 AM, Will Deacon wrote:
> >>On Mon, Jul 30, 2018 at 06:50:34PM -0400, Alexei Colin wrote:
> >>>Platforms with a PCI bus will be offered the RapidIO menu since they may
> >>>be want support for a RapidIO PCI device. Platforms without a PCI bus
> >>>that might include a RapidIO IP block will need to "select HAS_RAPIDIO"
> >>>in the platform-/machine-specific "config ARCH_*" Kconfig entry.
> >>>
> >>>Tested that kernel builds for arm64 with RapidIO subsystem and
> >>>switch drivers enabled, also that the modules load successfully
> >>>on a custom Aarch64 Qemu model.
> >>>
> >>>Cc: Andrew Morton <akpm@linux-foundation.org>
> >>>Cc: Russell King <linux@armlinux.org.uk>
> >>>Cc: John Paul Walters <jwalters@isi.edu>
> >>>Cc: linux-arm-kernel@lists.infradead.org
> >>>Cc: linux-kernel@vger.kernel.org,
> >>>Signed-off-by: Alexei Colin <acolin@isi.edu>
> >>>---
> >>>  arch/arm64/Kconfig | 2 ++
> >>>  1 file changed, 2 insertions(+)
> >>
> >>Thanks, this looks much cleaner than before:
> >>
> >>Acked-by: Will Deacon <will.deacon@arm.com>
> >>
> >>The only thing I'm not sure about is why we don't just select HAS_RAPIDIO
> >>unconditionally in the arm64 Kconfig. Does selecting only that option
> >>actually pull in new code to the build?
> >>
> >HAS_RAPIDIO option is intended for SOCs that have built in SRIO
> >controllers, like TI KeyStoneII or FPGAs. Because RapidIO subsystem core
> >is required during RapidIO port driver initialization, having separate
> >option allows us to control available build options for RapidIO core and
> >port driver (bool vs. tristate) and disable module option if port driver
> >is configured as built-in.
> 
> I am thinking about where HAS_RAPIDIO option can be set for arm64 branch.
> Having it set globally is too broad. For example we have Xilinx Zinq US
> board with SRIO IP on it. Having it globally in arm64 branch - bad. Probably
> having it set in drivers/soc/... is the best place.

Why is selecting HAS_RAPIDIO globally a bad thing to do? The way these
normally work is, if some subsystem requires arch support, then there's
an ARCH_HAS_xxxx option which the architecture selects when it implements
that support. Once you've enabled that, then that allows other sub-options
to be selected, such as specific drivers or what-not. Look at the Kconfig
files under drivers/soc/ -- you don't see anybody selecting ARCH_HAS_*
options.

Now, if HAS_RAPIDIO alone is pulling in a whole load of code to the build,
then it sounds like a misnomer.

Confused.

Will

Re: [PATCH 6/6] arm64: enable RapidIO menu in Kconfig

[Russell King - ARM Linux]

On Tue, Jul 31, 2018 at 04:01:27PM -0400, Alex Bounine wrote:
> On 2018-07-31 02:18 PM, Russell King - ARM Linux wrote:
> >On Tue, Jul 31, 2018 at 01:59:27PM -0400, Alex Bounine wrote:
> >>On 2018-07-31 11:52 AM, Russell King - ARM Linux wrote:
> >>>On Tue, Jul 31, 2018 at 08:54:14AM -0400, Alex Bounine wrote:
> >>>>On 2018-07-31 04:41 AM, Will Deacon wrote:
> >>>>>On Mon, Jul 30, 2018 at 06:50:34PM -0400, Alexei Colin wrote:
> >>>>>>Platforms with a PCI bus will be offered the RapidIO menu since they may
> >>>>>>be want support for a RapidIO PCI device. Platforms without a PCI bus
> >>>>>>that might include a RapidIO IP block will need to "select HAS_RAPIDIO"
> >>>>>>in the platform-/machine-specific "config ARCH_*" Kconfig entry.
> >>>>>>
> >>>>>>Tested that kernel builds for arm64 with RapidIO subsystem and
> >>>>>>switch drivers enabled, also that the modules load successfully
> >>>>>>on a custom Aarch64 Qemu model.
> >>>>>>
> >>>>>>Cc: Andrew Morton <akpm@linux-foundation.org>
> >>>>>>Cc: Russell King <linux@armlinux.org.uk>
> >>>>>>Cc: John Paul Walters <jwalters@isi.edu>
> >>>>>>Cc: linux-arm-kernel@lists.infradead.org
> >>>>>>Cc: linux-kernel@vger.kernel.org,
> >>>>>>Signed-off-by: Alexei Colin <acolin@isi.edu>
> >>>>>>---
> >>>>>>  arch/arm64/Kconfig | 2 ++
> >>>>>>  1 file changed, 2 insertions(+)
> >>>>>
> >>>>>Thanks, this looks much cleaner than before:
> >>>>>
> >>>>>Acked-by: Will Deacon <will.deacon@arm.com>
> >>>>>
> >>>>>The only thing I'm not sure about is why we don't just select HAS_RAPIDIO
> >>>>>unconditionally in the arm64 Kconfig. Does selecting only that option
> >>>>>actually pull in new code to the build?
> >>>>>
> >>>>HAS_RAPIDIO option is intended for SOCs that have built in SRIO controllers,
> >>>>like TI KeyStoneII or FPGAs. Because RapidIO subsystem core is required
> >>>>during RapidIO port driver initialization, having separate option allows us
> >>>>to control available build options for RapidIO core and port driver (bool
> >>>>vs. tristate) and disable module option if port driver is configured as
> >>>>built-in.
> >>>
> >>>Your explanation doesn't make much sense to me.
> >>>
> >>>RAPIDIO is the bus-level support, right?  So drivers that depend on
> >>>the bus-level support should depend on RAPIDIO, and so, if RAPIDIO
> >>>is configured as a module, they will also be allowed to be disabled
> >>>or a module, but not built-in if tristate.  If it is boolean, and
> >>>causes the driver to be built-in to the kernel, then you need to use
> >>>"RAPIDIO=y" so that it's dependency is only satisfied when the core
> >>>is built-in.
> >>>
> >>
> >>RapidIO host controllers (on local bus like SoC internal or PCIe) are
> >>serviced by MPORT device drivers that are subsystem specific and communicate
> >>with RapidIO core using set of callbacks. Depending on HW architecture these
> >>drivers may be defined as built-in or module.
> >
> >Why does hardware architecture define whether something has to be built
> >in or can be modular?
> >
> >It is the case today that (eg) on-SoC hardware _can_ be built as a module
> >if desired - just because it's on the SoC does not mean it has to be
> >built in to the kernel.  Why is RapidIO any different?
> >
> 
> Not HW architecture - legacy can be blamed as well. Freescale's FSL_RIO
> driver still exist as built-in so far. Intent of this patch set is to allow
> RapidIO support in more architectures without reworking old stuff.
> I do not think that anyone will be updating FSL_RIO driver soon.

Sorry, but I'm even more confused.  If it's not hardware architecture,
then why did you say previously "Depending on HW architecture these
drivers may be defined as built-in or module." ?

If it's that the driver isn't written to be a module, then that is not
"HW architecture".  Are you just trying to muddy the water?

> Also we cannot dictate to developers of future drivers which method to use -
> they may have built-in option only for the first release.

How is that any different from all the other drivers that we have?

If a driver can only be built-in, then we do this in the Kconfig:

config DRIVERFOO
	bool "Support driverfoo"
	depends on SUBSYSTEM=y

which ensures that the driver can only be built when it's dependent
subsystem is also built-in.

There is another alternative way, which is:

config SUBSYSTEM
	tristate

config DRIVERFOO
	bool "Support driverfoo"
	select SUBSYSTEM

config DRIVERBAR
	tristate "Support driverbar"
	select SUBSYSTEM

and "SUBSYSTEM" will automatically adopt either 'm' or 'y' correctly
depending on whether any drivers are built-in or not.

> Unfortunately we have on hands dependency between mport driver build mode
> and  RapidIO core which we need to respect.

How is that any different from (eg) hundreds of network drivers and the
networking core code that we already have?  That doesn't have such a
convoluted configuration system, and what you have is much simpler.

> >For example, can you point out why my idea I present below would not
> >work?
> 
> See below.
> 
> >
> >>For peripheral devices attached to the RapidIO fabric such dependency on
> >>local mport implementation does not exist and therefore they all can be
> >>treated as tristate.
> >>
> >>>HAS_RAPIDIO gives the impression that it defines whether or not
> >>>the rapidio core code is allowable or not - it doesn't suggest that
> >>>it has anything to do with drivers. However, reading the PowerPC
> >>>Kconfig files, it seems to be used that way.  That's confusing, and
> >>>ought to be fixed.  From what I can tell, it's only used for FSL_RIO,
> >>>so I suggest that gets converted to:
> >>>
> >>>config HAS_RAPIDIO
> >>>	bool PCI
> 
> PCI and RAPIDIO can be mutually exclusive.

Please explain this in light of your patches which contain:

config RAPIDIO
	tristate "RapidIO support"
	depends on HAS_RAPIDIO || PCI

This allows RAPIDIO can be selected when PCI is enabled.  Therefore,
PCI and RAPIDIO *are not* mutually exclusive as your comment above
states, therefore, I have to assume that your comment is wrong.

> >>>
> >>>config RAPIDIO
> >>>	tristate "RapidIO support"
> >>>	depends on HAS_RAPIDIO
> >>>
> >>>config HAS_FSL_RIO
> >>>	bool
> >>>	select HAS_RAPIDIO
> 
> Introducing new variable HAS_FSL_RIO here. Do you suggest having one for
> each ARM-based board that has on-chip RIO?

No, I'm suggesting a solution for what I see in the kernel tree today,
which is frankly a mess for the reasons I've already outlined.

HAS_FSL_RIO is based on the _only_ SoC driver apparently in the tree
based on what I could find in the arch/*/Kconfig files.  If you don't
think having individual HAS_* options in this way is appropriate,
then maybe instead having per-SoC hidden HAS_* config options are
more appropriate.

> >>>
> >>>config FSL_RIO
> >>>	bool "Freescale Embedded SRIO Controller support"
> >>>	depends on RAPIDIO = y && HAS_FSL_RIO
> >>>
> >>>This frees up HAS_RAPIDIO to operate as one would expect - to define
> >>>whether or not RAPIDIO should be offered.  This also allows:
> >>>
> >>>config ARM
> >>>	select HAS_RAPIDIO if PCI
> 
> Some SOCs can be configured without PCI. We have confusing action here - we
> have to enable PCI on platform that does not have it.

Again, you said above "PCI and RAPIDIO can be mutually exclusive."  This
comment self-conflicts with your previous comment.

What you now seem to be saying goes against the patches and the current
Kconfig.  You seem to be saying that RAPIDIO _requires_ PCI.

So, we now have three completely different statements from you:
- "we have to enable PCI on platform that does not have it."
- "PCI and RAPIDIO can be mutually exclusive."
- RAPIDIO does not require PCI (since HAS_RAPIDIO=y RAPIDIO=y PCI=n is
  a permissible configuration as things stand with PowerPC.)

Which is it?

It seems to be that your replies are just muddying the water - I can
only assume that this is to make us give up.  Please don't do that,
it's not in your interest.

> Why we cannot use "select HAS_RAPIDIO" HW-specific Kconfig file
> (mach-*/Kconfig)? And have on-chip port selection in the same board-specific
> place.

As I've already explained, HAS_RAPIDIO has the expectation that it
controls the availability of the RAPIDIO option, not of drivers.
It is HAS_*RAPIDIO*, the clue is in the name.  Using it as you are
(basically, to mean that on-SoC rapidio hardware is present) and
allowing such configurations as HAS_RAPIDIO=n RAPIDIO=y PCI=y is
completely counter-intuitive.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up

Re: [PATCH 6/6] arm64: enable RapidIO menu in Kconfig

[Alexei Colin]

> > Why we cannot use "select HAS_RAPIDIO" HW-specific Kconfig file
> > (mach-*/Kconfig)? And have on-chip port selection in the same board-specific
> > place.
> 
> As I've already explained, HAS_RAPIDIO has the expectation that it
> controls the availability of the RAPIDIO option, not of drivers.
> It is HAS_*RAPIDIO*, the clue is in the name.  Using it as you are
> (basically, to mean that on-SoC rapidio hardware is present) and
> allowing such configurations as HAS_RAPIDIO=n RAPIDIO=y PCI=y is
> completely counter-intuitive.

The intention in the patch was for HAS_RAPIDIO to mean exactly "on-SoC
RapidIO hardware is present". Since the name does not reflect that well,
I'll change it in the next version: would HAS_RAPIDIO_ONCHIP reflect the
meaning well? 

HAS_RAPIDIO=n RAPIDIO=y PCI=y should be an intuitive configuration,
for example for MIPS until last week it effectively was the only option
https://www.linux-mips.org/archives/linux-mips/2018-07/msg00584.html
If we have to rename to make this configuration intuitive again, then
I'll rename and resubmit.

There was never the intention to assign any other meaning to
HAS_RAPIDIO, specifically that it controls the availability of the
RAPIDIO menu option. I think interpreting it this way is is behind a lot
of the issues raised. The intention is that availability of the menu
options is controlled by (1) whether the architecture sources the
rapidio/Kconfig and (2) whether dependencies of RapidIO are satisfied
(either having a PCI bus and enabling it, or having the on-SoC RapidIO
hardware.

The patch does not indend to change the current meaning and usage of the
config options and behavior for X86, PPC, MIPS -- the patch only
refactors there (because it was requested). For ARM and ARM64, we are
adding the same behavior in same way as for those three architectures. I
assume there is nothing that sets ARM and ARM64 apart from the others in
this narrow context. This is the indended scope of this patch. Are we
now discussing a change in the current meaning/usage/behavior?  If so,
could we do one piece at a time -- first, add ARM and ARM64 in the way
consistent with the way other architectures currently do it?

Re: [PATCH 6/6] arm64: enable RapidIO menu in Kconfig

[Alex Bounine]

On 2018-07-31 04:46 PM, Alexei Colin wrote:
> On Tue, Jul 31, 2018 at 04:29:56PM -0400, Alex Bounine wrote:
... skip
>>>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>>>> Cc: Russell King <linux@armlinux.org.uk>
>>>>> Cc: John Paul Walters <jwalters@isi.edu>
>>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>>> Cc: linux-kernel@vger.kernel.org,
>>>>> Signed-off-by: Alexei Colin <acolin@isi.edu>
>>>>> ---
>>>>>    arch/arm64/Kconfig | 2 ++
>>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> Thanks, this looks much cleaner than before:
>>>>
>>>> Acked-by: Will Deacon <will.deacon@arm.com>
>>>>
>>>> The only thing I'm not sure about is why we don't just select HAS_RAPIDIO
>>>> unconditionally in the arm64 Kconfig. Does selecting only that option
>>>> actually pull in new code to the build?
>>>>
>>> HAS_RAPIDIO option is intended for SOCs that have built in SRIO
>>> controllers, like TI KeyStoneII or FPGAs. Because RapidIO subsystem core
>>> is required during RapidIO port driver initialization, having separate
>>> option allows us to control available build options for RapidIO core and
>>> port driver (bool vs. tristate) and disable module option if port driver
>>> is configured as built-in.
>>
>> I am thinking about where HAS_RAPIDIO option can be set for arm64 branch.
>> Having it set globally is too broad. For example we have Xilinx Zinq US
>> board with SRIO IP on it. Having it globally in arm64 branch - bad. Probably
>> having it set in drivers/soc/... is the best place.
>>
>> Will, Alexei what do you think?
> 
> Since the HAS_RAPIODIO flag adds meta info about SoC, maybe the line
> that 'select's it can go where the rest of the meta info is, which
> differs across architectures:
> * For ARM64, in arch/arm64/Kconfig.platforms under each config ARCH_*
> for each SoC that includes RapidIO.
> * For ARM, in arch/arm/mach-*/Kconfig
> 
> But, if we want the flag to be automatically selected for some larger
> set of ARM64 SoCs without explicitly adding it to each one, then the
> above is not going to do that.
>

Thank you for clarification. I am leaning towards fine control of 
available configuration options, on per-board basis.
As Russell mentioned in some of his comments, RapidIO is relatively rare 
thing to ordinary user. Because of that I would prefer not to pollute 
config menu with selection options unrelated to most of boards supported 
in given arch.

Also it looks like proposed patches introduce minimal changes to the 
generic part of code.

>>>>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>>>>> index a8f0c74e6f7f..5e8cf90505ec 100644
>>>>> --- a/arch/arm64/Kconfig
>>>>> +++ b/arch/arm64/Kconfig
>>>>> @@ -308,6 +308,8 @@ config PCI_SYSCALL
>>>>>    source "drivers/pci/Kconfig"
>>>>> +source "drivers/rapidio/Kconfig"
>>>>> +
>>>>>    endmenu
>>>>>    menu "Kernel Features"
>>>>> -- 
>>>>> 2.18.0
>>>>>

Re: [PATCH 6/6] arm64: enable RapidIO menu in Kconfig

[Alex Bounine]

On 2018-08-01 06:38 AM, Russell King - ARM Linux wrote:
> On Tue, Jul 31, 2018 at 04:01:27PM -0400, Alex Bounine wrote:
>> On 2018-07-31 02:18 PM, Russell King - ARM Linux wrote:
>>> On Tue, Jul 31, 2018 at 01:59:27PM -0400, Alex Bounine wrote:
>>>> On 2018-07-31 11:52 AM, Russell King - ARM Linux wrote:
>>>>> On Tue, Jul 31, 2018 at 08:54:14AM -0400, Alex Bounine wrote:
>>>>>> On 2018-07-31 04:41 AM, Will Deacon wrote:
>>>>>>> On Mon, Jul 30, 2018 at 06:50:34PM -0400, Alexei Colin wrote:
>>>>>>>> Platforms with a PCI bus will be offered the RapidIO menu since they may
>>>>>>>> be want support for a RapidIO PCI device. Platforms without a PCI bus
>>>>>>>> that might include a RapidIO IP block will need to "select HAS_RAPIDIO"
>>>>>>>> in the platform-/machine-specific "config ARCH_*" Kconfig entry.
>>>>>>>>
>>>>>>>> Tested that kernel builds for arm64 with RapidIO subsystem and
>>>>>>>> switch drivers enabled, also that the modules load successfully
>>>>>>>> on a custom Aarch64 Qemu model.
>>>>>>>>
>>>>>>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>>>>>>> Cc: Russell King <linux@armlinux.org.uk>
>>>>>>>> Cc: John Paul Walters <jwalters@isi.edu>
>>>>>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>>>>>> Cc: linux-kernel@vger.kernel.org,
>>>>>>>> Signed-off-by: Alexei Colin <acolin@isi.edu>
>>>>>>>> ---
>>>>>>>>   arch/arm64/Kconfig | 2 ++
>>>>>>>>   1 file changed, 2 insertions(+)
>>>>>>>
>>>>>>> Thanks, this looks much cleaner than before:
>>>>>>>
>>>>>>> Acked-by: Will Deacon <will.deacon@arm.com>
>>>>>>>
>>>>>>> The only thing I'm not sure about is why we don't just select HAS_RAPIDIO
>>>>>>> unconditionally in the arm64 Kconfig. Does selecting only that option
>>>>>>> actually pull in new code to the build?
>>>>>>>
>>>>>> HAS_RAPIDIO option is intended for SOCs that have built in SRIO controllers,
>>>>>> like TI KeyStoneII or FPGAs. Because RapidIO subsystem core is required
>>>>>> during RapidIO port driver initialization, having separate option allows us
>>>>>> to control available build options for RapidIO core and port driver (bool
>>>>>> vs. tristate) and disable module option if port driver is configured as
>>>>>> built-in.
>>>>>
>>>>> Your explanation doesn't make much sense to me.
>>>>>
>>>>> RAPIDIO is the bus-level support, right?  So drivers that depend on
>>>>> the bus-level support should depend on RAPIDIO, and so, if RAPIDIO
>>>>> is configured as a module, they will also be allowed to be disabled
>>>>> or a module, but not built-in if tristate.  If it is boolean, and
>>>>> causes the driver to be built-in to the kernel, then you need to use
>>>>> "RAPIDIO=y" so that it's dependency is only satisfied when the core
>>>>> is built-in.
>>>>>
>>>>
>>>> RapidIO host controllers (on local bus like SoC internal or PCIe) are
>>>> serviced by MPORT device drivers that are subsystem specific and communicate
>>>> with RapidIO core using set of callbacks. Depending on HW architecture these
>>>> drivers may be defined as built-in or module.
>>>
>>> Why does hardware architecture define whether something has to be built
>>> in or can be modular?
>>>
>>> It is the case today that (eg) on-SoC hardware _can_ be built as a module
>>> if desired - just because it's on the SoC does not mean it has to be
>>> built in to the kernel.  Why is RapidIO any different?
>>>
>>
>> Not HW architecture - legacy can be blamed as well. Freescale's FSL_RIO
>> driver still exist as built-in so far. Intent of this patch set is to allow
>> RapidIO support in more architectures without reworking old stuff.
>> I do not think that anyone will be updating FSL_RIO driver soon.
> 
> Sorry, but I'm even more confused.  If it's not hardware architecture,
> then why did you say previously "Depending on HW architecture these
> drivers may be defined as built-in or module." ?

My fault, I took some shortcuts in writing here and that made it not 
clear enough. In this case I was talking about legacy code existing in 
PowerPC architecture branch. FSL_RIO host driver can be built only as 
built-in what forces RIO core to be built-in as well.

> If it's that the driver isn't written to be a module, then that is not
> "HW architecture".  Are you just trying to muddy the water?

Not at all :) See above. Driver within architectural branch.

>> Also we cannot dictate to developers of future drivers which method to use -
>> they may have built-in option only for the first release.
> 
> How is that any different from all the other drivers that we have?
> 
> If a driver can only be built-in, then we do this in the Kconfig:
> 
> config DRIVERFOO
> 	bool "Support driverfoo"
> 	depends on SUBSYSTEM=y
> 
> which ensures that the driver can only be built when it's dependent
> subsystem is also built-in.
> 
> There is another alternative way, which is:
> 
> config SUBSYSTEM
> 	tristate
> 
> config DRIVERFOO
> 	bool "Support driverfoo"
> 	select SUBSYSTEM
> 
> config DRIVERBAR
> 	tristate "Support driverbar"
> 	select SUBSYSTEM
> 
> and "SUBSYSTEM" will automatically adopt either 'm' or 'y' correctly
> depending on whether any drivers are built-in or not.
> 
Agree.

>> Unfortunately we have on hands dependency between mport driver build mode
>> and  RapidIO core which we need to respect.
> 
> How is that any different from (eg) hundreds of network drivers and the
> networking core code that we already have?  That doesn't have such a
> convoluted configuration system, and what you have is much simpler.
> 
>>> For example, can you point out why my idea I present below would not
>>> work?
>>
>> See below.
>>
>>>
>>>> For peripheral devices attached to the RapidIO fabric such dependency on
>>>> local mport implementation does not exist and therefore they all can be
>>>> treated as tristate.
>>>>
>>>>> HAS_RAPIDIO gives the impression that it defines whether or not
>>>>> the rapidio core code is allowable or not - it doesn't suggest that
>>>>> it has anything to do with drivers. However, reading the PowerPC
>>>>> Kconfig files, it seems to be used that way.  That's confusing, and
>>>>> ought to be fixed.  From what I can tell, it's only used for FSL_RIO,
>>>>> so I suggest that gets converted to:
>>>>>
>>>>> config HAS_RAPIDIO
>>>>> 	bool PCI
>>
>> PCI and RAPIDIO can be mutually exclusive.
> 
> Please explain this in light of your patches which contain:
> 

Availability of PCIe and RapidIO can be mutually exclusive on specific 
boards.
On oter boards we may have both buses at the same time.
Any platform with PCIe can add RapidIO support using PCIe-to-SRIO host 
bridge.

> config RAPIDIO
> 	tristate "RapidIO support"
> 	depends on HAS_RAPIDIO || PCI
> 
> This allows RAPIDIO can be selected when PCI is enabled.  Therefore,
> PCI and RAPIDIO *are not* mutually exclusive as your comment above
> states, therefore, I have to assume that your comment is wrong.
> 
>>>>>
>>>>> config RAPIDIO
>>>>> 	tristate "RapidIO support"
>>>>> 	depends on HAS_RAPIDIO
>>>>>
>>>>> config HAS_FSL_RIO
>>>>> 	bool
>>>>> 	select HAS_RAPIDIO
>>
>> Introducing new variable HAS_FSL_RIO here. Do you suggest having one for
>> each ARM-based board that has on-chip RIO?
> 
> No, I'm suggesting a solution for what I see in the kernel tree today,
> which is frankly a mess for the reasons I've already outlined.
> 
> HAS_FSL_RIO is based on the _only_ SoC driver apparently in the tree
> based on what I could find in the arch/*/Kconfig files.  If you don't
> think having individual HAS_* options in this way is appropriate,
> then maybe instead having per-SoC hidden HAS_* config options are
> more appropriate.
> 
This is what we are trying to use here: per-SoC option HAS_RAPIDIO. 
While the name does not have an individual designation (like 
HAS_FSL_RIO) it will be only the SoC in the given system that selects 
it. This will enable RapidIO configuration menu, allowing user to go 
ahead with remaining RIO configuration. Enabling on-chip port stays as 
per-SoC option.

Having PCIe bus brings another twist because PCIe-to-SRIO bridge can be 
used when HAS_RAPIDIO=n. As result every platform that has PCIe should 
enable RapidIO (except in architectures that do not support RIO at this 
moment).

>>>>>
>>>>> config FSL_RIO
>>>>> 	bool "Freescale Embedded SRIO Controller support"
>>>>> 	depends on RAPIDIO = y && HAS_FSL_RIO
>>>>>
>>>>> This frees up HAS_RAPIDIO to operate as one would expect - to define
>>>>> whether or not RAPIDIO should be offered.  This also allows:
>>>>>
>>>>> config ARM
>>>>> 	select HAS_RAPIDIO if PCI
>>
>> Some SOCs can be configured without PCI. We have confusing action here - we
>> have to enable PCI on platform that does not have it.
> 
> Again, you said above "PCI and RAPIDIO can be mutually exclusive."  This
> comment self-conflicts with your previous comment.

I explained "mutually exclusive" above.

> 
> What you now seem to be saying goes against the patches and the current
> Kconfig.  You seem to be saying that RAPIDIO _requires_ PCI.
> 
> So, we now have three completely different statements from you:
> - "we have to enable PCI on platform that does not have it."
> - "PCI and RAPIDIO can be mutually exclusive."
> - RAPIDIO does not require PCI (since HAS_RAPIDIO=y RAPIDIO=y PCI=n is
>    a permissible configuration as things stand with PowerPC.)
> 
> Which is it?
> 
> It seems to be that your replies are just muddying the water - I can
> only assume that this is to make us give up.  Please don't do that,
> it's not in your interest.
> 
>> Why we cannot use "select HAS_RAPIDIO" HW-specific Kconfig file
>> (mach-*/Kconfig)? And have on-chip port selection in the same board-specific
>> place.
> 
> As I've already explained, HAS_RAPIDIO has the expectation that it
> controls the availability of the RAPIDIO option, not of drivers.
> It is HAS_*RAPIDIO*, the clue is in the name.  Using it as you are
> (basically, to mean that on-SoC rapidio hardware is present) and
> allowing such configurations as HAS_RAPIDIO=n RAPIDIO=y PCI=y is
> completely counter-intuitive.
>