diff mbox series

[v2,5/5] firmware: QCOM_SCM: Allow qcom_scm driver to be loadable as a permenent module

Message ID 20200625001039.56174-6-john.stultz@linaro.org
State New
Headers show
Series Allow for qcom-pdc, pinctrl-msm and qcom-scm drivers to be loadable as modules | expand

Commit Message

John Stultz June 25, 2020, 12:10 a.m. UTC
Allow the qcom_scm driver to be loadable as a
permenent module.

Cc: Andy Gross <agross@kernel.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Maulik Shah <mkshah@codeaurora.org>
Cc: Lina Iyer <ilina@codeaurora.org>
Cc: Saravana Kannan <saravanak@google.com>
Cc: Todd Kjos <tkjos@google.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-arm-msm@vger.kernel.org
Cc: iommu@lists.linux-foundation.org
Cc: linux-gpio@vger.kernel.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/firmware/Kconfig    | 2 +-
 drivers/firmware/Makefile   | 3 ++-
 drivers/firmware/qcom_scm.c | 4 ++++
 drivers/iommu/Kconfig       | 2 ++
 4 files changed, 9 insertions(+), 2 deletions(-)

Comments

Greg KH July 2, 2020, 12:47 p.m. UTC | #1
On Thu, Jun 25, 2020 at 12:10:39AM +0000, John Stultz wrote:
> Allow the qcom_scm driver to be loadable as a
> permenent module.
> 
> Cc: Andy Gross <agross@kernel.org>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Maulik Shah <mkshah@codeaurora.org>
> Cc: Lina Iyer <ilina@codeaurora.org>
> Cc: Saravana Kannan <saravanak@google.com>
> Cc: Todd Kjos <tkjos@google.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-arm-msm@vger.kernel.org
> Cc: iommu@lists.linux-foundation.org
> Cc: linux-gpio@vger.kernel.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>


Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Will Deacon July 2, 2020, 2:18 p.m. UTC | #2
On Thu, Jun 25, 2020 at 12:10:39AM +0000, John Stultz wrote:
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index b510f67dfa49..714893535dd2 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -381,6 +381,7 @@ config SPAPR_TCE_IOMMU
>  config ARM_SMMU
>  	tristate "ARM Ltd. System MMU (SMMU) Support"
>  	depends on (ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)) && MMU
> +	depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
>  	select IOMMU_API
>  	select IOMMU_IO_PGTABLE_LPAE
>  	select ARM_DMA_USE_IOMMU if ARM

This looks like a giant hack. Is there another way to handle this?

Will
John Stultz July 10, 2020, 3:28 a.m. UTC | #3
On Thu, Jul 2, 2020 at 7:18 AM Will Deacon <will@kernel.org> wrote:
> On Thu, Jun 25, 2020 at 12:10:39AM +0000, John Stultz wrote:
> > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > index b510f67dfa49..714893535dd2 100644
> > --- a/drivers/iommu/Kconfig
> > +++ b/drivers/iommu/Kconfig
> > @@ -381,6 +381,7 @@ config SPAPR_TCE_IOMMU
> >  config ARM_SMMU
> >       tristate "ARM Ltd. System MMU (SMMU) Support"
> >       depends on (ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)) && MMU
> > +     depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
> >       select IOMMU_API
> >       select IOMMU_IO_PGTABLE_LPAE
> >       select ARM_DMA_USE_IOMMU if ARM
>
> This looks like a giant hack. Is there another way to handle this?

Sorry for the slow response here.

So, I agree the syntax looks strange (requiring a comment obviously
isn't a good sign), but it's a fairly common way to ensure drivers
don't get built in if they optionally depend on another driver that
can be built as a module.
  See "RFKILL || !RFKILL", "EXTCON || !EXTCON", or "USB_GADGET ||
!USB_GADGET" in various Kconfig files.

I'm open to using a different method, and in a different thread you
suggested using something like symbol_get(). I need to look into it
more, but that approach looks even more messy and prone to runtime
failures. Blocking the unwanted case at build time seems a bit cleaner
to me, even if the syntax is odd.

thanks
-john
Will Deacon July 10, 2020, 7:54 a.m. UTC | #4
On Thu, Jul 09, 2020 at 08:28:45PM -0700, John Stultz wrote:
> On Thu, Jul 2, 2020 at 7:18 AM Will Deacon <will@kernel.org> wrote:
> > On Thu, Jun 25, 2020 at 12:10:39AM +0000, John Stultz wrote:
> > > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > > index b510f67dfa49..714893535dd2 100644
> > > --- a/drivers/iommu/Kconfig
> > > +++ b/drivers/iommu/Kconfig
> > > @@ -381,6 +381,7 @@ config SPAPR_TCE_IOMMU
> > >  config ARM_SMMU
> > >       tristate "ARM Ltd. System MMU (SMMU) Support"
> > >       depends on (ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)) && MMU
> > > +     depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
> > >       select IOMMU_API
> > >       select IOMMU_IO_PGTABLE_LPAE
> > >       select ARM_DMA_USE_IOMMU if ARM
> >
> > This looks like a giant hack. Is there another way to handle this?
> 
> Sorry for the slow response here.
> 
> So, I agree the syntax looks strange (requiring a comment obviously
> isn't a good sign), but it's a fairly common way to ensure drivers
> don't get built in if they optionally depend on another driver that
> can be built as a module.
>   See "RFKILL || !RFKILL", "EXTCON || !EXTCON", or "USB_GADGET ||
> !USB_GADGET" in various Kconfig files.
> 
> I'm open to using a different method, and in a different thread you
> suggested using something like symbol_get(). I need to look into it
> more, but that approach looks even more messy and prone to runtime
> failures. Blocking the unwanted case at build time seems a bit cleaner
> to me, even if the syntax is odd.

Maybe just split it out then, so that the ARM_SMMU entry doesn't have this,
as that driver _really_ doesn't care about SoC details like this. In other
words, add a new entry along the lines of:

	config ARM_SMMU_QCOM_IMPL
	default y
	#if QCOM_SCM=m this can't be =y
	depends on ARM_SMMU & (QCOM_SCM || !QCOM_SCM)

and then have arm-smmu.h provide a static inline qcom_smmu_impl_init()
which returns -ENODEV if CONFIG_ARM_SMMU_QCOM_IMPL=n and hack the Makefile
so that we don't bother to compile arm-smmu-qcom.o in that case.

Would that work?

Will
John Stultz July 10, 2020, 10:21 p.m. UTC | #5
On Fri, Jul 10, 2020 at 12:54 AM Will Deacon <will@kernel.org> wrote:
> On Thu, Jul 09, 2020 at 08:28:45PM -0700, John Stultz wrote:
> > On Thu, Jul 2, 2020 at 7:18 AM Will Deacon <will@kernel.org> wrote:
> > > On Thu, Jun 25, 2020 at 12:10:39AM +0000, John Stultz wrote:
> > > > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > > > index b510f67dfa49..714893535dd2 100644
> > > > --- a/drivers/iommu/Kconfig
> > > > +++ b/drivers/iommu/Kconfig
> > > > @@ -381,6 +381,7 @@ config SPAPR_TCE_IOMMU
> > > >  config ARM_SMMU
> > > >       tristate "ARM Ltd. System MMU (SMMU) Support"
> > > >       depends on (ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)) && MMU
> > > > +     depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
> > > >       select IOMMU_API
> > > >       select IOMMU_IO_PGTABLE_LPAE
> > > >       select ARM_DMA_USE_IOMMU if ARM
> > >
> > > This looks like a giant hack. Is there another way to handle this?
> >
> > Sorry for the slow response here.
> >
> > So, I agree the syntax looks strange (requiring a comment obviously
> > isn't a good sign), but it's a fairly common way to ensure drivers
> > don't get built in if they optionally depend on another driver that
> > can be built as a module.
> >   See "RFKILL || !RFKILL", "EXTCON || !EXTCON", or "USB_GADGET ||
> > !USB_GADGET" in various Kconfig files.
> >
> > I'm open to using a different method, and in a different thread you
> > suggested using something like symbol_get(). I need to look into it
> > more, but that approach looks even more messy and prone to runtime
> > failures. Blocking the unwanted case at build time seems a bit cleaner
> > to me, even if the syntax is odd.
>
> Maybe just split it out then, so that the ARM_SMMU entry doesn't have this,
> as that driver _really_ doesn't care about SoC details like this. In other
> words, add a new entry along the lines of:
>
>         config ARM_SMMU_QCOM_IMPL
>         default y
>         #if QCOM_SCM=m this can't be =y
>         depends on ARM_SMMU & (QCOM_SCM || !QCOM_SCM)
>
> and then have arm-smmu.h provide a static inline qcom_smmu_impl_init()
> which returns -ENODEV if CONFIG_ARM_SMMU_QCOM_IMPL=n and hack the Makefile
> so that we don't bother to compile arm-smmu-qcom.o in that case.
>
> Would that work?

I think this proposal still has problems with the directionality of the call.

The arm-smmu-impl.o calls to arm-smmu-qcom.o which calls qcom_scm.o
So if qcom_scm.o is part of a module, the calling code in
arm-smmu-qcom.o also needs to be a module, which means CONFIG_ARM_SMMU
needs to be a module.

I know you said the arm-smmu driver doesn't care about SoC details,
but the trouble is that currently the arm-smmu driver does directly
call the qcom-scm code. So it is a real dependency. However, if
QCOM_SCM is not configured, it calls stubs and that's ok.  In that
way, the "depends on QCOM_SCM || !QCOM_SCM" line actually makes sense.
It looks terrible because we're used to boolean logic, but it's
ternary.

Maybe can have the ARM_SMMU_QCOM_IMPL approach you suggest above, but
that just holds the issue out at arms length, because we're still
going to need to have:
  depends on ARM_SMMU_QCOM_IMPL || !ARM_SMMU_QCOM_IMPL
in the ARM_SMMU definition, which I suspect you're wanting to avoid.

Otherwise the only thing I can think of is a deeper reworking of the
arm-smmu-impl code so that the arm-smmu-qcom code probes itself and
registers its hooks with the arm-smmu core.
That way the arm-smmu driver would not directly call any SoC specific
code (and thus have no dependencies outward). But it's probably a fair
amount of churn vs the extra depends string.

thanks
-john
Will Deacon July 13, 2020, 8:41 p.m. UTC | #6
On Fri, Jul 10, 2020 at 03:21:53PM -0700, John Stultz wrote:
> On Fri, Jul 10, 2020 at 12:54 AM Will Deacon <will@kernel.org> wrote:
> > On Thu, Jul 09, 2020 at 08:28:45PM -0700, John Stultz wrote:
> > > On Thu, Jul 2, 2020 at 7:18 AM Will Deacon <will@kernel.org> wrote:
> > > > On Thu, Jun 25, 2020 at 12:10:39AM +0000, John Stultz wrote:
> > > > > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > > > > index b510f67dfa49..714893535dd2 100644
> > > > > --- a/drivers/iommu/Kconfig
> > > > > +++ b/drivers/iommu/Kconfig
> > > > > @@ -381,6 +381,7 @@ config SPAPR_TCE_IOMMU
> > > > >  config ARM_SMMU
> > > > >       tristate "ARM Ltd. System MMU (SMMU) Support"
> > > > >       depends on (ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)) && MMU
> > > > > +     depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
> > > > >       select IOMMU_API
> > > > >       select IOMMU_IO_PGTABLE_LPAE
> > > > >       select ARM_DMA_USE_IOMMU if ARM
> > > >
> > > > This looks like a giant hack. Is there another way to handle this?
> > >
> > > Sorry for the slow response here.
> > >
> > > So, I agree the syntax looks strange (requiring a comment obviously
> > > isn't a good sign), but it's a fairly common way to ensure drivers
> > > don't get built in if they optionally depend on another driver that
> > > can be built as a module.
> > >   See "RFKILL || !RFKILL", "EXTCON || !EXTCON", or "USB_GADGET ||
> > > !USB_GADGET" in various Kconfig files.
> > >
> > > I'm open to using a different method, and in a different thread you
> > > suggested using something like symbol_get(). I need to look into it
> > > more, but that approach looks even more messy and prone to runtime
> > > failures. Blocking the unwanted case at build time seems a bit cleaner
> > > to me, even if the syntax is odd.
> >
> > Maybe just split it out then, so that the ARM_SMMU entry doesn't have this,
> > as that driver _really_ doesn't care about SoC details like this. In other
> > words, add a new entry along the lines of:
> >
> >         config ARM_SMMU_QCOM_IMPL
> >         default y
> >         #if QCOM_SCM=m this can't be =y
> >         depends on ARM_SMMU & (QCOM_SCM || !QCOM_SCM)
> >
> > and then have arm-smmu.h provide a static inline qcom_smmu_impl_init()
> > which returns -ENODEV if CONFIG_ARM_SMMU_QCOM_IMPL=n and hack the Makefile
> > so that we don't bother to compile arm-smmu-qcom.o in that case.
> >
> > Would that work?
> 
> I think this proposal still has problems with the directionality of the call.
> 
> The arm-smmu-impl.o calls to arm-smmu-qcom.o which calls qcom_scm.o
> So if qcom_scm.o is part of a module, the calling code in
> arm-smmu-qcom.o also needs to be a module, which means CONFIG_ARM_SMMU
> needs to be a module.
> 
> I know you said the arm-smmu driver doesn't care about SoC details,
> but the trouble is that currently the arm-smmu driver does directly
> call the qcom-scm code. So it is a real dependency. However, if
> QCOM_SCM is not configured, it calls stubs and that's ok.  In that
> way, the "depends on QCOM_SCM || !QCOM_SCM" line actually makes sense.
> It looks terrible because we're used to boolean logic, but it's
> ternary.

Yes, it looks ugly, but the part I really have issues with is that building
QCOM_SCM=m and ARM_SMMU=y is perfectly fine if you don't run on an SoC
with the qcom implementation. I don't see why we need to enforce things
here beyond making sure that all selectable permutations _build_ and
fail gracefully at runtime on the qcom SoC if SCM isn't available.

Will
John Stultz July 13, 2020, 8:48 p.m. UTC | #7
On Mon, Jul 13, 2020 at 1:41 PM Will Deacon <will@kernel.org> wrote:
>
> On Fri, Jul 10, 2020 at 03:21:53PM -0700, John Stultz wrote:
> > On Fri, Jul 10, 2020 at 12:54 AM Will Deacon <will@kernel.org> wrote:
> > > On Thu, Jul 09, 2020 at 08:28:45PM -0700, John Stultz wrote:
> > > > On Thu, Jul 2, 2020 at 7:18 AM Will Deacon <will@kernel.org> wrote:
> > > > > On Thu, Jun 25, 2020 at 12:10:39AM +0000, John Stultz wrote:
> > > > > > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > > > > > index b510f67dfa49..714893535dd2 100644
> > > > > > --- a/drivers/iommu/Kconfig
> > > > > > +++ b/drivers/iommu/Kconfig
> > > > > > @@ -381,6 +381,7 @@ config SPAPR_TCE_IOMMU
> > > > > >  config ARM_SMMU
> > > > > >       tristate "ARM Ltd. System MMU (SMMU) Support"
> > > > > >       depends on (ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)) && MMU
> > > > > > +     depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
> > > > > >       select IOMMU_API
> > > > > >       select IOMMU_IO_PGTABLE_LPAE
> > > > > >       select ARM_DMA_USE_IOMMU if ARM
> > > > >
> > > > > This looks like a giant hack. Is there another way to handle this?
> > > >
> > > > Sorry for the slow response here.
> > > >
> > > > So, I agree the syntax looks strange (requiring a comment obviously
> > > > isn't a good sign), but it's a fairly common way to ensure drivers
> > > > don't get built in if they optionally depend on another driver that
> > > > can be built as a module.
> > > >   See "RFKILL || !RFKILL", "EXTCON || !EXTCON", or "USB_GADGET ||
> > > > !USB_GADGET" in various Kconfig files.
> > > >
> > > > I'm open to using a different method, and in a different thread you
> > > > suggested using something like symbol_get(). I need to look into it
> > > > more, but that approach looks even more messy and prone to runtime
> > > > failures. Blocking the unwanted case at build time seems a bit cleaner
> > > > to me, even if the syntax is odd.
> > >
> > > Maybe just split it out then, so that the ARM_SMMU entry doesn't have this,
> > > as that driver _really_ doesn't care about SoC details like this. In other
> > > words, add a new entry along the lines of:
> > >
> > >         config ARM_SMMU_QCOM_IMPL
> > >         default y
> > >         #if QCOM_SCM=m this can't be =y
> > >         depends on ARM_SMMU & (QCOM_SCM || !QCOM_SCM)
> > >
> > > and then have arm-smmu.h provide a static inline qcom_smmu_impl_init()
> > > which returns -ENODEV if CONFIG_ARM_SMMU_QCOM_IMPL=n and hack the Makefile
> > > so that we don't bother to compile arm-smmu-qcom.o in that case.
> > >
> > > Would that work?
> >
> > I think this proposal still has problems with the directionality of the call.
> >
> > The arm-smmu-impl.o calls to arm-smmu-qcom.o which calls qcom_scm.o
> > So if qcom_scm.o is part of a module, the calling code in
> > arm-smmu-qcom.o also needs to be a module, which means CONFIG_ARM_SMMU
> > needs to be a module.
> >
> > I know you said the arm-smmu driver doesn't care about SoC details,
> > but the trouble is that currently the arm-smmu driver does directly
> > call the qcom-scm code. So it is a real dependency. However, if
> > QCOM_SCM is not configured, it calls stubs and that's ok.  In that
> > way, the "depends on QCOM_SCM || !QCOM_SCM" line actually makes sense.
> > It looks terrible because we're used to boolean logic, but it's
> > ternary.
>
> Yes, it looks ugly, but the part I really have issues with is that building
> QCOM_SCM=m and ARM_SMMU=y is perfectly fine if you don't run on an SoC
> with the qcom implementation. I don't see why we need to enforce things
> here beyond making sure that all selectable permutations _build_ and
> fail gracefully at runtime on the qcom SoC if SCM isn't available.

Ok. Thanks, that context/rationale makes sense to me now!  I'll dig in
and see if I can figure out the runtime get_symbol handling you
suggested for the scm callout.

Thanks again for the feedback!
-john
Will Deacon July 14, 2020, 7:56 a.m. UTC | #8
On Mon, Jul 13, 2020 at 01:48:29PM -0700, John Stultz wrote:
> On Mon, Jul 13, 2020 at 1:41 PM Will Deacon <will@kernel.org> wrote:
> > On Fri, Jul 10, 2020 at 03:21:53PM -0700, John Stultz wrote:
> > > On Fri, Jul 10, 2020 at 12:54 AM Will Deacon <will@kernel.org> wrote:
> > > > On Thu, Jul 09, 2020 at 08:28:45PM -0700, John Stultz wrote:
> > > > > On Thu, Jul 2, 2020 at 7:18 AM Will Deacon <will@kernel.org> wrote:
> > > > > > On Thu, Jun 25, 2020 at 12:10:39AM +0000, John Stultz wrote:
> > > > > > > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > > > > > > index b510f67dfa49..714893535dd2 100644
> > > > > > > --- a/drivers/iommu/Kconfig
> > > > > > > +++ b/drivers/iommu/Kconfig
> > > > > > > @@ -381,6 +381,7 @@ config SPAPR_TCE_IOMMU
> > > > > > >  config ARM_SMMU
> > > > > > >       tristate "ARM Ltd. System MMU (SMMU) Support"
> > > > > > >       depends on (ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)) && MMU
> > > > > > > +     depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
> > > > > > >       select IOMMU_API
> > > > > > >       select IOMMU_IO_PGTABLE_LPAE
> > > > > > >       select ARM_DMA_USE_IOMMU if ARM
> > > > > >
> > > > > > This looks like a giant hack. Is there another way to handle this?
> > > > >
> > > > > Sorry for the slow response here.
> > > > >
> > > > > So, I agree the syntax looks strange (requiring a comment obviously
> > > > > isn't a good sign), but it's a fairly common way to ensure drivers
> > > > > don't get built in if they optionally depend on another driver that
> > > > > can be built as a module.
> > > > >   See "RFKILL || !RFKILL", "EXTCON || !EXTCON", or "USB_GADGET ||
> > > > > !USB_GADGET" in various Kconfig files.
> > > > >
> > > > > I'm open to using a different method, and in a different thread you
> > > > > suggested using something like symbol_get(). I need to look into it
> > > > > more, but that approach looks even more messy and prone to runtime
> > > > > failures. Blocking the unwanted case at build time seems a bit cleaner
> > > > > to me, even if the syntax is odd.
> > > >
> > > > Maybe just split it out then, so that the ARM_SMMU entry doesn't have this,
> > > > as that driver _really_ doesn't care about SoC details like this. In other
> > > > words, add a new entry along the lines of:
> > > >
> > > >         config ARM_SMMU_QCOM_IMPL
> > > >         default y
> > > >         #if QCOM_SCM=m this can't be =y
> > > >         depends on ARM_SMMU & (QCOM_SCM || !QCOM_SCM)
> > > >
> > > > and then have arm-smmu.h provide a static inline qcom_smmu_impl_init()
> > > > which returns -ENODEV if CONFIG_ARM_SMMU_QCOM_IMPL=n and hack the Makefile
> > > > so that we don't bother to compile arm-smmu-qcom.o in that case.
> > > >
> > > > Would that work?
> > >
> > > I think this proposal still has problems with the directionality of the call.
> > >
> > > The arm-smmu-impl.o calls to arm-smmu-qcom.o which calls qcom_scm.o
> > > So if qcom_scm.o is part of a module, the calling code in
> > > arm-smmu-qcom.o also needs to be a module, which means CONFIG_ARM_SMMU
> > > needs to be a module.
> > >
> > > I know you said the arm-smmu driver doesn't care about SoC details,
> > > but the trouble is that currently the arm-smmu driver does directly
> > > call the qcom-scm code. So it is a real dependency. However, if
> > > QCOM_SCM is not configured, it calls stubs and that's ok.  In that
> > > way, the "depends on QCOM_SCM || !QCOM_SCM" line actually makes sense.
> > > It looks terrible because we're used to boolean logic, but it's
> > > ternary.
> >
> > Yes, it looks ugly, but the part I really have issues with is that building
> > QCOM_SCM=m and ARM_SMMU=y is perfectly fine if you don't run on an SoC
> > with the qcom implementation. I don't see why we need to enforce things
> > here beyond making sure that all selectable permutations _build_ and
> > fail gracefully at runtime on the qcom SoC if SCM isn't available.
> 
> Ok. Thanks, that context/rationale makes sense to me now!  I'll dig in
> and see if I can figure out the runtime get_symbol handling you
> suggested for the scm callout.

Cheers, but in the interest of not being a massive pain in the bum, please
yell if it ends up being tonnes of work and I'll close my eyes while
applying your original patch.

Will
diff mbox series

Patch

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index fbd785dd0513..9e533a462bf4 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -236,7 +236,7 @@  config INTEL_STRATIX10_RSU
 	  Say Y here if you want Intel RSU support.
 
 config QCOM_SCM
-	bool
+	tristate "Qcom SCM driver"
 	depends on ARM || ARM64
 	select RESET_CONTROLLER
 
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 99510be9f5ed..cf24d674216b 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -17,7 +17,8 @@  obj-$(CONFIG_ISCSI_IBFT)	+= iscsi_ibft.o
 obj-$(CONFIG_FIRMWARE_MEMMAP)	+= memmap.o
 obj-$(CONFIG_RASPBERRYPI_FIRMWARE) += raspberrypi.o
 obj-$(CONFIG_FW_CFG_SYSFS)	+= qemu_fw_cfg.o
-obj-$(CONFIG_QCOM_SCM)		+= qcom_scm.o qcom_scm-smc.o qcom_scm-legacy.o
+obj-$(CONFIG_QCOM_SCM)		+= qcom-scm.o
+qcom-scm-objs += qcom_scm.o qcom_scm-smc.o qcom_scm-legacy.o
 obj-$(CONFIG_TI_SCI_PROTOCOL)	+= ti_sci.o
 obj-$(CONFIG_TRUSTED_FOUNDATIONS) += trusted_foundations.o
 obj-$(CONFIG_TURRIS_MOX_RWTM)	+= turris-mox-rwtm.o
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 0e7233a20f34..b5e88bf66975 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -1155,6 +1155,7 @@  static const struct of_device_id qcom_scm_dt_match[] = {
 	{ .compatible = "qcom,scm" },
 	{}
 };
+MODULE_DEVICE_TABLE(of, qcom_scm_dt_match);
 
 static struct platform_driver qcom_scm_driver = {
 	.driver = {
@@ -1170,3 +1171,6 @@  static int __init qcom_scm_init(void)
 	return platform_driver_register(&qcom_scm_driver);
 }
 subsys_initcall(qcom_scm_init);
+
+MODULE_DESCRIPTION("Qualcomm Technologies, Inc. SCM driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index b510f67dfa49..714893535dd2 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -381,6 +381,7 @@  config SPAPR_TCE_IOMMU
 config ARM_SMMU
 	tristate "ARM Ltd. System MMU (SMMU) Support"
 	depends on (ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)) && MMU
+	depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
 	select IOMMU_API
 	select IOMMU_IO_PGTABLE_LPAE
 	select ARM_DMA_USE_IOMMU if ARM
@@ -500,6 +501,7 @@  config QCOM_IOMMU
 	# Note: iommu drivers cannot (yet?) be built as modules
 	bool "Qualcomm IOMMU Support"
 	depends on ARCH_QCOM || (COMPILE_TEST && !GENERIC_ATOMIC64)
+	depends on QCOM_SCM=y
 	select IOMMU_API
 	select IOMMU_IO_PGTABLE_LPAE
 	select ARM_DMA_USE_IOMMU