Re: [PATCH 6/6] arm64: enable RapidIO menu in Kconfig
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 Cc: Russell King Cc: John Paul Walters Cc: linux-arm-ker...@lists.infradead.org Cc: linux-kernel@vger.kernel.org, Signed-off-by: Alexei Colin --- arch/arm64/Kconfig | 2 ++ 1 file changed, 2 insertions(+) Thanks, this looks much cleaner than before: Acked-by: Will Deacon 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.
Re: [PATCH 6/6] arm64: enable RapidIO menu in Kconfig
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 Cc: Russell King Cc: John Paul Walters Cc: linux-arm-ker...@lists.infradead.org Cc: linux-kernel@vger.kernel.org, Signed-off-by: Alexei Colin --- arch/arm64/Kconfig | 2 ++ 1 file changed, 2 insertions(+) Thanks, this looks much cleaner than before: Acked-by: Will Deacon 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
> > 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
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 > >>Cc: Russell King > >>Cc: John Paul Walters > >>Cc: linux-arm-ker...@lists.infradead.org > >>Cc: linux-kernel@vger.kernel.org, > >>Signed-off-by: Alexei Colin > >>--- > >> arch/arm64/Kconfig | 2 ++ > >> 1 file changed, 2 insertions(+) > > > >Thanks, this looks much cleaner than before: > > > >Acked-by: Will Deacon > > > >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. > > > > >>F
Re: [PATCH 6/6] arm64: enable RapidIO menu in Kconfig
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 > >>>Cc: Russell King > >>>Cc: John Paul Walters > >>>Cc: linux-arm-ker...@lists.infradead.org > >>>Cc: linux-kernel@vger.kernel.org, > >>>Signed-off-by: Alexei Colin > >>>--- > >>> arch/arm64/Kconfig | 2 ++ > >>> 1 file changed, 2 insertions(+) > >> > >>Thanks, this looks much cleaner than before: > >> > >>Acked-by: Will Deacon > >> > >>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_ 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
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 > > > > Cc: Russell King > > > > Cc: John Paul Walters > > > > Cc: linux-arm-ker...@lists.infradead.org > > > > Cc: linux-kernel@vger.kernel.org, > > > > Signed-off-by: Alexei Colin > > > > --- > > > > arch/arm64/Kconfig | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > Thanks, this looks much cleaner than before: > > > > > > Acked-by: Will Deacon > > > > > > 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
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 Cc: Russell King Cc: John Paul Walters Cc: linux-arm-ker...@lists.infradead.org Cc: linux-kernel@vger.kernel.org, Signed-off-by: Alexei Colin --- arch/arm64/Kconfig | 2 ++ 1 file changed, 2 insertions(+) Thanks, this looks much cleaner than before: Acked-by: Will Deacon 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
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 Cc: Russell King Cc: John Paul Walters Cc: linux-arm-ker...@lists.infradead.org Cc: linux-kernel@vger.kernel.org, Signed-off-by: Alexei Colin --- arch/arm64/Kconfig | 2 ++ 1 file changed, 2 insertions(+) Thanks, this looks much cleaner than before: Acked-by: Will Deacon 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 th
Re: [PATCH 6/6] arm64: enable RapidIO menu in Kconfig
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 > Cc: Russell King > Cc: John Paul Walters > Cc: linux-arm-ker...@lists.infradead.org > Cc: linux-kernel@vger.kernel.org, > Signed-off-by: Alexei Colin > --- > arch/arm64/Kconfig | 2 ++ > 1 file changed, 2 insertions(+) > >>> > >>>Thanks, this looks much cleaner than before: > >>> > >>>Acked-by: Will Deacon > >>> > >>>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
Re: [PATCH 6/6] arm64: enable RapidIO menu in Kconfig
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 Cc: Russell King Cc: John Paul Walters Cc: linux-arm-ker...@lists.infradead.org Cc: linux-kernel@vger.kernel.org, Signed-off-by: Alexei Colin --- arch/arm64/Kconfig | 2 ++ 1 file changed, 2 insertions(+) Thanks, this looks much cleaner than before: Acked-by: Will Deacon 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 p
Re: [PATCH 6/6] arm64: enable RapidIO menu in Kconfig
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 > >>Cc: Russell King > >>Cc: John Paul Walters > >>Cc: linux-arm-ker...@lists.infradead.org > >>Cc: linux-kernel@vger.kernel.org, > >>Signed-off-by: Alexei Colin > >>--- > >> arch/arm64/Kconfig | 2 ++ > >> 1 file changed, 2 insertions(+) > > > >Thanks, this looks much cleaner than before: > > > >Acked-by: Will Deacon > > > >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
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 Cc: Russell King Cc: John Paul Walters Cc: linux-arm-ker...@lists.infradead.org Cc: linux-kernel@vger.kernel.org, Signed-off-by: Alexei Colin --- arch/arm64/Kconfig | 2 ++ 1 file changed, 2 insertions(+) Thanks, this looks much cleaner than before: Acked-by: Will Deacon 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 6/6] arm64: enable RapidIO menu in Kconfig
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 > Cc: Russell King > Cc: John Paul Walters > Cc: linux-arm-ker...@lists.infradead.org > Cc: linux-kernel@vger.kernel.org, > Signed-off-by: Alexei Colin > --- > arch/arm64/Kconfig | 2 ++ > 1 file changed, 2 insertions(+) Thanks, this looks much cleaner than before: Acked-by: Will Deacon 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 >
[PATCH 6/6] arm64: enable RapidIO menu in Kconfig
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 Cc: Russell King Cc: John Paul Walters Cc: linux-arm-ker...@lists.infradead.org Cc: linux-kernel@vger.kernel.org, Signed-off-by: Alexei Colin --- 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
