| Message ID | 1444860627-8258-1-git-send-email-agross@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
On Thu, Oct 15, 2015 at 11:05:27PM +0200, Arnd Bergmann wrote: > On Wednesday 14 October 2015 17:10:27 Andy Gross wrote: > > Qualcomm ARM Based SoC Updates for 4.4 > > > > * Implement id_table driver matching in SMD > > * Avoid NULL pointer exception on remove of SMEM > > * Reorder SMEM/SMD configs > > * Make qcom_smem_get() return a pointer > > * Handle big endian CPUs correctly in SMEM > > * Represent SMD channel layout in structures > > * Use __iowrite32_copy() in SMD > > * Remove use of VLAIs in SMD > > * Handle big endian CPUs correctly in SMD/RPM > > * Handle big endian CPUs corretly in SMD > > * Reject sending SMD packets that are too large > > * Fix endianness issue in SCM __qcom_scm_is_call_available > > * Add missing prototype for qcom_scm_is_available() > > * Correct SMEM items for upper channels > > * Use architecture level to build SCM correctly > > * Delete unneeded of_node_put in SMD > > * Correct active/slep state flagging in SMD/RPM > > * Move RPM message ram out of SMEM DT node > > > > Pulled into next/drivers, but please try to make the changelogs more readable > in the future. No point listing each small patch here, instead say what you've > done overall and what the major things are. Right now, the merge commit > just duplicates the information like this: Fair enough. I'll condense it next time. Thanks for the hwspinlock fix on top.
On 10/15/2015 02:05 PM, Arnd Bergmann wrote: > > This also seemed like the right place > to add a bugfix that I had in my queue: > > commit 73ebb85444b0472d90bb70a1a9e6b5df3f92c14c > Author: Arnd Bergmann <arnd@arndb.de> > Date: Tue Oct 13 17:05:39 2015 +0200 > > soc: qcom/smem: add HWSPINLOCK dependency > > This fixes a build error when smem is enabled without hwspinlock: > > drivers/built-in.o: In function `qcom_smem_alloc': > rockchip-efuse.c:(.text+0x7a3e4): undefined reference to `__hwspin_lock_timeout' > rockchip-efuse.c:(.text+0x7a568): undefined reference to `__hwspin_unlock' > drivers/built-in.o: In function `qcom_smem_remove': > rockchip-efuse.c:(.text+0x7a5cc): undefined reference to `hwspin_lock_free' > drivers/built-in.o: In function `qcom_smem_probe': > rockchip-efuse.c:(.text+0x7a960): undefined reference to `hwspin_lock_request_specific' > rockchip-efuse.c:(.text+0x7a988): undefined reference to `of_hwspin_lock_get_id' > drivers/built-in.o: In function `qcom_smem_get': > rockchip-efuse.c:(.text+0x7aa24): undefined reference to `__hwspin_lock_timeout' > rockchip-efuse.c:(.text+0x7aafc): undefined reference to `__hwspin_unlock' > Can you share your .config? It looks like there are stubs for these, so I'm lost how we got undefined references.
On 10/16, Arnd Bergmann wrote: > On Friday 16 October 2015 09:56:30 Stephen Boyd wrote: > > > > Can you share your .config? It looks like there are stubs for these, so > > I'm lost how we got undefined references. > > > http://pastebin.com/HtrC510p > > The problem is CONFIG_HWSPINLOCK=m && CONFIG_QCOM_SMEM=y. Sorry for sending > an incomplete patch description, I had not noticed those wrapper functions. > > My patch fixes the issue, but does not allow you to build QCOM_SMEM without > HWSPINLOCK. If we want that configuration to be valid, we need one of > the two changes below: > > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig > index eec76141d9b9..cae0ffa19bca 100644 > --- a/drivers/soc/qcom/Kconfig > +++ b/drivers/soc/qcom/Kconfig > @@ -22,7 +22,7 @@ config QCOM_PM > config QCOM_SMEM > tristate "Qualcomm Shared Memory Manager (SMEM)" > depends on ARCH_QCOM > - depends on HWSPINLOCK > + depends on HWSPINLOCK && !HWSPINLOCK What does this mean? Is it missing an =m? > help > Say y here to enable support for the Qualcomm Shared Memory Manager. > The driver provides an interface to items in a heap shared among all > diff --git a/include/linux/hwspinlock.h b/include/linux/hwspinlock.h > index 859d673d98c8..cdfd9fd2ba11 100644 > --- a/include/linux/hwspinlock.h > +++ b/include/linux/hwspinlock.h > @@ -59,7 +59,7 @@ struct hwspinlock_pdata { > int base_id; > }; > > -#if defined(CONFIG_HWSPINLOCK) || defined(CONFIG_HWSPINLOCK_MODULE) > +#if IS_REACHABLE(CONFIG_HWSPINLOCK) > > int hwspin_lock_register(struct hwspinlock_device *bank, struct device *dev, > const struct hwspinlock_ops *ops, int base_id, int num_locks); > I'd prefer this patch. Is there some way to capture that QCOM_SMEM must be a module if HWSPINLOCK is a module unless we're doing a compile test, in which case we don't care? I guess that would just be: depends on HWSPINLOCK || COMPILE_TEST
On Friday 16 October 2015 13:04:17 Stephen Boyd wrote: > On 10/16, Arnd Bergmann wrote: > > On Friday 16 October 2015 09:56:30 Stephen Boyd wrote: > > > > > > Can you share your .config? It looks like there are stubs for these, so > > > I'm lost how we got undefined references. > > > > > http://pastebin.com/HtrC510p > > > > The problem is CONFIG_HWSPINLOCK=m && CONFIG_QCOM_SMEM=y. Sorry for sending > > an incomplete patch description, I had not noticed those wrapper functions. > > > > My patch fixes the issue, but does not allow you to build QCOM_SMEM without > > HWSPINLOCK. If we want that configuration to be valid, we need one of > > the two changes below: > > > > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig > > index eec76141d9b9..cae0ffa19bca 100644 > > --- a/drivers/soc/qcom/Kconfig > > +++ b/drivers/soc/qcom/Kconfig > > @@ -22,7 +22,7 @@ config QCOM_PM > > config QCOM_SMEM > > tristate "Qualcomm Shared Memory Manager (SMEM)" > > depends on ARCH_QCOM > > - depends on HWSPINLOCK > > + depends on HWSPINLOCK && !HWSPINLOCK > > What does this mean? This is the magic way in Kconfig to express what we want ;-) It says it depends on either HWSPINLOCK being disabled (!HWSPINLOCK), or it depends on HWSPINLOCK. The latter implies that QCOM_SMEM cannot be built-in when HWSPINLOCK=m, but they are allowed to both be built-in. > Is it missing an =m? No > > help > > Say y here to enable support for the Qualcomm Shared Memory Manager. > > The driver provides an interface to items in a heap shared among all > > diff --git a/include/linux/hwspinlock.h b/include/linux/hwspinlock.h > > index 859d673d98c8..cdfd9fd2ba11 100644 > > --- a/include/linux/hwspinlock.h > > +++ b/include/linux/hwspinlock.h > > @@ -59,7 +59,7 @@ struct hwspinlock_pdata { > > int base_id; > > }; > > > > -#if defined(CONFIG_HWSPINLOCK) || defined(CONFIG_HWSPINLOCK_MODULE) > > +#if IS_REACHABLE(CONFIG_HWSPINLOCK) > > > > int hwspin_lock_register(struct hwspinlock_device *bank, struct device *dev, > > const struct hwspinlock_ops *ops, int base_id, int num_locks); > > > > I'd prefer this patch. Is there some way to capture that > QCOM_SMEM must be a module if HWSPINLOCK is a module unless we're > doing a compile test, in which case we don't care? The problem here is that while we avoid the build error, we get the counterintuitive behavior that you can have the HWSPINLOCK module loaded but the built-in QCOM_SMEM module does not use it because it wasn't reachable at compile time. > I guess that would just be: > > depends on HWSPINLOCK || COMPILE_TEST This would not solve the randconfig build problem. Arnd
On 10/16, Arnd Bergmann wrote: > On Friday 16 October 2015 13:04:17 Stephen Boyd wrote: > > On 10/16, Arnd Bergmann wrote: > > > On Friday 16 October 2015 09:56:30 Stephen Boyd wrote: > > > > > > > > Can you share your .config? It looks like there are stubs for these, so > > > > I'm lost how we got undefined references. > > > > > > > http://pastebin.com/HtrC510p > > > > > > The problem is CONFIG_HWSPINLOCK=m && CONFIG_QCOM_SMEM=y. Sorry for sending > > > an incomplete patch description, I had not noticed those wrapper functions. > > > > > > My patch fixes the issue, but does not allow you to build QCOM_SMEM without > > > HWSPINLOCK. If we want that configuration to be valid, we need one of > > > the two changes below: > > > > > > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig > > > index eec76141d9b9..cae0ffa19bca 100644 > > > --- a/drivers/soc/qcom/Kconfig > > > +++ b/drivers/soc/qcom/Kconfig > > > @@ -22,7 +22,7 @@ config QCOM_PM > > > config QCOM_SMEM > > > tristate "Qualcomm Shared Memory Manager (SMEM)" > > > depends on ARCH_QCOM > > > - depends on HWSPINLOCK > > > + depends on HWSPINLOCK && !HWSPINLOCK > > > > What does this mean? > > This is the magic way in Kconfig to express what we want ;-) > > It says it depends on either HWSPINLOCK being disabled (!HWSPINLOCK), > or it depends on HWSPINLOCK. The latter implies that QCOM_SMEM cannot > be built-in when HWSPINLOCK=m, but they are allowed to both be built-in. > I think you mean OR then? depends on HWSPINLOCK || !HWSPINLOCK If I have HWSPINLOCK=y and try menuconfig with your patch, the smem config isn't selectable. But if HWSPINLOCK=m then the smem config is visible and smem can only be a module. > > > > help > > > Say y here to enable support for the Qualcomm Shared Memory Manager. > > > The driver provides an interface to items in a heap shared among all > > > diff --git a/include/linux/hwspinlock.h b/include/linux/hwspinlock.h > > > index 859d673d98c8..cdfd9fd2ba11 100644 > > > --- a/include/linux/hwspinlock.h > > > +++ b/include/linux/hwspinlock.h > > > @@ -59,7 +59,7 @@ struct hwspinlock_pdata { > > > int base_id; > > > }; > > > > > > -#if defined(CONFIG_HWSPINLOCK) || defined(CONFIG_HWSPINLOCK_MODULE) > > > +#if IS_REACHABLE(CONFIG_HWSPINLOCK) > > > > > > int hwspin_lock_register(struct hwspinlock_device *bank, struct device *dev, > > > const struct hwspinlock_ops *ops, int base_id, int num_locks); > > > > > > > I'd prefer this patch. Is there some way to capture that > > QCOM_SMEM must be a module if HWSPINLOCK is a module unless we're > > doing a compile test, in which case we don't care? > > The problem here is that while we avoid the build error, we get the > counterintuitive behavior that you can have the HWSPINLOCK module > loaded but the built-in QCOM_SMEM module does not use it because > it wasn't reachable at compile time. > Agreed, which is why I suggested depends on HWSPINLOCK || COMPILE_TEST + the IS_REACHABLE patch. That way if hwspinlock=y, smem can be y or m, if hwspinlock=m, smem can only be m, and if hwspinlock=n then smem can only be y or m if it has COMPILE_TEST=y. In the last case the IS_REACHABLE ifdef will "do the right thing" and make it so that HWSPINLOCK symbols are stubbed out if hwspinlock=m and smem=y or hwspinlock=n and smem=y/m. I don't think we ever care about a kernel that actually runs when COMPILE_TEST=y, so this seems to be ok.
On Friday 16 October 2015 14:26:27 Stephen Boyd wrote: > On 10/16, Arnd Bergmann wrote: > > On Friday 16 October 2015 13:04:17 Stephen Boyd wrote: > > > On 10/16, Arnd Bergmann wrote: > > > > On Friday 16 October 2015 09:56:30 Stephen Boyd wrote: > > > > > > > > > > Can you share your .config? It looks like there are stubs for these, so > > > > > I'm lost how we got undefined references. > > > > > > > > > http://pastebin.com/HtrC510p > > > > > > > > The problem is CONFIG_HWSPINLOCK=m && CONFIG_QCOM_SMEM=y. Sorry for sending > > > > an incomplete patch description, I had not noticed those wrapper functions. > > > > > > > > My patch fixes the issue, but does not allow you to build QCOM_SMEM without > > > > HWSPINLOCK. If we want that configuration to be valid, we need one of > > > > the two changes below: > > > > > > > > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig > > > > index eec76141d9b9..cae0ffa19bca 100644 > > > > --- a/drivers/soc/qcom/Kconfig > > > > +++ b/drivers/soc/qcom/Kconfig > > > > @@ -22,7 +22,7 @@ config QCOM_PM > > > > config QCOM_SMEM > > > > tristate "Qualcomm Shared Memory Manager (SMEM)" > > > > depends on ARCH_QCOM > > > > - depends on HWSPINLOCK > > > > + depends on HWSPINLOCK && !HWSPINLOCK > > > > > > What does this mean? > > > > This is the magic way in Kconfig to express what we want ;-) > > > > It says it depends on either HWSPINLOCK being disabled (!HWSPINLOCK), > > or it depends on HWSPINLOCK. The latter implies that QCOM_SMEM cannot > > be built-in when HWSPINLOCK=m, but they are allowed to both be built-in. > > > > I think you mean OR then? > > depends on HWSPINLOCK || !HWSPINLOCK Yes, sorry about that. > If I have HWSPINLOCK=y and try menuconfig with your patch, the > smem config isn't selectable. But if HWSPINLOCK=m then the smem > config is visible and smem can only be a module. Ok, I have to admit that I don't even know how Kconfig interprets (FOO && !FOO), so that's probably the result of my typo. > > > > > > help > > > > Say y here to enable support for the Qualcomm Shared Memory Manager. > > > > The driver provides an interface to items in a heap shared among all > > > > diff --git a/include/linux/hwspinlock.h b/include/linux/hwspinlock.h > > > > index 859d673d98c8..cdfd9fd2ba11 100644 > > > > --- a/include/linux/hwspinlock.h > > > > +++ b/include/linux/hwspinlock.h > > > > @@ -59,7 +59,7 @@ struct hwspinlock_pdata { > > > > int base_id; > > > > }; > > > > > > > > -#if defined(CONFIG_HWSPINLOCK) || defined(CONFIG_HWSPINLOCK_MODULE) > > > > +#if IS_REACHABLE(CONFIG_HWSPINLOCK) > > > > > > > > int hwspin_lock_register(struct hwspinlock_device *bank, struct device *dev, > > > > const struct hwspinlock_ops *ops, int base_id, int num_locks); > > > > > > > > > > I'd prefer this patch. Is there some way to capture that > > > QCOM_SMEM must be a module if HWSPINLOCK is a module unless we're > > > doing a compile test, in which case we don't care? > > > > The problem here is that while we avoid the build error, we get the > > counterintuitive behavior that you can have the HWSPINLOCK module > > loaded but the built-in QCOM_SMEM module does not use it because > > it wasn't reachable at compile time. > > > > Agreed, which is why I suggested depends on HWSPINLOCK || > COMPILE_TEST + the IS_REACHABLE patch. That way if hwspinlock=y, > smem can be y or m, if hwspinlock=m, smem can only be m, and if > hwspinlock=n then smem can only be y or m if it has > COMPILE_TEST=y. In the last case the IS_REACHABLE ifdef will "do > the right thing" and make it so that HWSPINLOCK symbols are > stubbed out if hwspinlock=m and smem=y or hwspinlock=n and > smem=y/m. Ah, I see. The downside is still that other users of HWSPINLOCK might now accidentally build but not work if HWSPINLOCK=m. > I don't think we ever care about a kernel that actually runs when > COMPILE_TEST=y, so this seems to be ok. Right, for the QCOM_SMEM support that is clearly ok as long as you have the Kconfig logic there. Arnd
The following changes since commit 6ff33f3902c3b1c5d0db6b1e2c70b6d76fba357f: Linux 4.3-rc1 (2015-09-12 16:35:56 -0700) are available in the git repository at: git://codeaurora.org/quic/kernel/agross-msm.git tags/qcom-soc-for-4.4 for you to fetch changes up to d0bfd7c9b162612de55ca2d204403b90dc278db6: soc: qcom: smem: Move RPM message ram out of smem DT node (2015-10-14 14:51:40 -0500) ---------------------------------------------------------------- Qualcomm ARM Based SoC Updates for 4.4 * Implement id_table driver matching in SMD * Avoid NULL pointer exception on remove of SMEM * Reorder SMEM/SMD configs * Make qcom_smem_get() return a pointer * Handle big endian CPUs correctly in SMEM * Represent SMD channel layout in structures * Use __iowrite32_copy() in SMD * Remove use of VLAIs in SMD * Handle big endian CPUs correctly in SMD/RPM * Handle big endian CPUs corretly in SMD * Reject sending SMD packets that are too large * Fix endianness issue in SCM __qcom_scm_is_call_available * Add missing prototype for qcom_scm_is_available() * Correct SMEM items for upper channels * Use architecture level to build SCM correctly * Delete unneeded of_node_put in SMD * Correct active/slep state flagging in SMD/RPM * Move RPM message ram out of SMEM DT node ---------------------------------------------------------------- Andy Gross (2): firmware: qcom: scm: Add function stubs for ARM64 Merge tag 'qcom-fixes-for-4.3-rc1' into base-for-4.4 Arnd Bergmann (1): firmware: qcom-scm: build for correct architecture level Bjorn Andersson (4): soc: qcom: smd: Implement id_table driver matching soc: qcom: smd: Reject send of too big packets soc: qcom: smd: Correct SMEM items for upper channels soc: qcom: smd-rpm: Correct the active vs sleep state flagging Julia Lawall (1): soc: qcom: smd: delete unneeded of_node_put Rob Clark (2): qcom-scm: fix endianess issue in __qcom_scm_is_call_available qcom-scm: add missing prototype for qcom_scm_is_available() Stephen Boyd (10): soc: qcom: smem: Avoid NULL pointer exception on remove soc: qcom: Reorder SMEM/SMD configs soc: qcom: Make qcom_smem_get() return a pointer soc: qcom: smem: Handle big endian CPUs soc: qcom: smd: Represent channel layout in structures soc: qcom: smd: Use __iowrite32_copy() instead of open-coding it soc: qcom: smd: Remove use of VLAIS soc: qcom: smd_rpm: Handle big endian CPUs soc: qcom: smd: Handle big endian CPUs soc: qcom: smem: Move RPM message ram out of smem DT node arch/arm/boot/dts/qcom-msm8974.dtsi | 17 +- drivers/firmware/Kconfig | 8 + drivers/firmware/Makefile | 5 +- drivers/firmware/qcom_scm-32.c | 6 +- drivers/firmware/qcom_scm-64.c | 63 ++++++ drivers/soc/qcom/Kconfig | 16 +- drivers/soc/qcom/smd-rpm.c | 68 ++++--- drivers/soc/qcom/smd.c | 296 +++++++++++++++++----------- drivers/soc/qcom/smem.c | 368 +++++++++++++++++++---------------- include/linux/qcom_scm.h | 2 + include/linux/soc/qcom/smd.h | 11 ++ include/linux/soc/qcom/smem.h | 2 +- 12 files changed, 525 insertions(+), 337 deletions(-) create mode 100644 drivers/firmware/qcom_scm-64.c