On Sun, Mar 21, 2021 at 7:46 PM Krzysztof Kozlowski <krzysztof.kozlowski(a)canonical.com&gt; wrote: > > The Stratix10 service layer and RCU drivers are useful only on > Stratix10, so on ARMv8. Compile testing the RCU driver on 32-bit ARM > fails: > > drivers/firmware/stratix10-rsu.c: In function 'rsu_status_callback': > include/linux/compiler_types.h:320:38: error: call to '__compiletime_assert_179' > declared with attribute error: FIELD_GET: type of reg too small for mask > _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) > ... > drivers/firmware/stratix10-rsu.c:96:26: note: in expansion of macro 'FIELD_GET' > priv->status.version = FIELD_GET(RSU_VERSION_MASK, > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski(a)canonical.com&gt; > Reported-by: kernel test robot <lkp(a)intel.com&gt; While I agree that one shouldn't run 32-bit kernels on this, we should also try to write drivers portably, and in theory any SoC that can run a 64-bit Arm kernel should also be able to run a 32-bit kernel if you include the same drivers. It seems that the problem here is in the smccc definition struct arm_smccc_res { unsigned long a0; unsigned long a1; unsigned long a2; unsigned long a3; }; so the result of #define RSU_VERSION_MASK GENMASK_ULL(63, 32) priv->status.version = FIELD_GET(RSU_VERSION_MASK, res->a2); tries to access bits that are just not returned by the firmware here, which indicates that it probably won't work in this case. What I'm not entirely sure about is whether this is a problem in the Intel firmware implementation requiring the smccc caller to run in a 64-bit context, or if it's a mistake in the way the driver extracts the information if the firmware can actually pass it down correctly.

On 21/03/2021 22:09, Arnd Bergmann wrote: > On Sun, Mar 21, 2021 at 7:46 PM Krzysztof Kozlowski > <krzysztof.kozlowski(a)canonical.com&gt; wrote: >> >> The Stratix10 service layer and RCU drivers are useful only on >> Stratix10, so on ARMv8. Compile testing the RCU driver on 32-bit ARM >> fails: >> >> drivers/firmware/stratix10-rsu.c: In function 'rsu_status_callback': >> include/linux/compiler_types.h:320:38: error: call to '__compiletime_assert_179' >> declared with attribute error: FIELD_GET: type of reg too small for mask >> _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) >> ... >> drivers/firmware/stratix10-rsu.c:96:26: note: in expansion of macro 'FIELD_GET' >> priv->status.version = FIELD_GET(RSU_VERSION_MASK, >> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski(a)canonical.com&gt; >> Reported-by: kernel test robot <lkp(a)intel.com&gt; > > While I agree that one shouldn't run 32-bit kernels on this, we should also try > to write drivers portably, and in theory any SoC that can run a 64-bit > Arm kernel > should also be able to run a 32-bit kernel if you include the same drivers. > > It seems that the problem here is in the smccc definition > > struct arm_smccc_res { > unsigned long a0; > unsigned long a1; > unsigned long a2; > unsigned long a3; > }; > > so the result of > > #define RSU_VERSION_MASK GENMASK_ULL(63, 32) > priv->status.version = FIELD_GET(RSU_VERSION_MASK, res->a2); > > tries to access bits that are just not returned by the firmware here, > which indicates that it probably won't work in this case. > > What I'm not entirely sure about is whether this is a problem in > the Intel firmware implementation requiring the smccc caller to > run in a 64-bit context, or if it's a mistake in the way the driver > extracts the information if the firmware can actually pass it down > correctly. The SMC has two calling conventions - SMC32/HVC32 and SMC64/HVC64. The Stratix 10 driver uses the 64-bit calling convention (see INTEL_SIP_SMC_FAST_CALL_VAL in include/linux/firmware/intel/stratix10-smc.h), so it should not run in aarch32 (regardless of type of hardware). I think that my patch limiting the support to 64-bit makes sense.

On 22/03/2021 13:58, Richard Gong wrote: > > > On 3/22/21 3:26 AM, Krzysztof Kozlowski wrote: >> >> On 21/03/2021 22:09, Arnd Bergmann wrote: >>> On Sun, Mar 21, 2021 at 7:46 PM Krzysztof Kozlowski >>> <krzysztof.kozlowski(a)canonical.com&gt; wrote: >>>> >>>> The Stratix10 service layer and RCU drivers are useful only on >>>> Stratix10, so on ARMv8. Compile testing the RCU driver on 32-bit ARM >>>> fails: >>>> >>>> drivers/firmware/stratix10-rsu.c: In function 'rsu_status_callback': >>>> include/linux/compiler_types.h:320:38: error: call to '__compiletime_assert_179' >>>> declared with attribute error: FIELD_GET: type of reg too small for mask >>>> _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) >>>> ... >>>> drivers/firmware/stratix10-rsu.c:96:26: note: in expansion of macro 'FIELD_GET' >>>> priv->status.version = FIELD_GET(RSU_VERSION_MASK, >>>> >>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski(a)canonical.com&gt; >>>> Reported-by: kernel test robot <lkp(a)intel.com&gt; >>> >>> While I agree that one shouldn't run 32-bit kernels on this, we should also try >>> to write drivers portably, and in theory any SoC that can run a 64-bit >>> Arm kernel >>> should also be able to run a 32-bit kernel if you include the same drivers. >>> >>> It seems that the problem here is in the smccc definition >>> >>> struct arm_smccc_res { >>> unsigned long a0; >>> unsigned long a1; >>> unsigned long a2; >>> unsigned long a3; >>> }; >>> >>> so the result of >>> >>> #define RSU_VERSION_MASK GENMASK_ULL(63, 32) >>> priv->status.version = FIELD_GET(RSU_VERSION_MASK, res->a2); >>> >>> tries to access bits that are just not returned by the firmware here, >>> which indicates that it probably won't work in this case. >>> >>> What I'm not entirely sure about is whether this is a problem in >>> the Intel firmware implementation requiring the smccc caller to >>> run in a 64-bit context, or if it's a mistake in the way the driver >>> extracts the information if the firmware can actually pass it down >>> correctly. >> >> The SMC has two calling conventions - SMC32/HVC32 and SMC64/HVC64. The >> Stratix 10 driver uses the 64-bit calling convention (see >> INTEL_SIP_SMC_FAST_CALL_VAL in >> include/linux/firmware/intel/stratix10-smc.h), so it should not run in >> aarch32 (regardless of type of hardware). >> >> I think that my patch limiting the support to 64-bit makes sense. >> > > The stratix10 service layer and RSU driver are only used in Intel 64-bit > SoCFPGA platforms. This we know, however the questions were: 1. Why the driver cannot be made portable? Why it cannot be developed in a way it allows building on different platforms?

2. Does the actual firmware support 32-bit SMC convention call?

Best regards, Krzysztof