* [PATCH 0/2] add optional cache params from DT
@ 2020-09-16 7:19 Gilad Ben-Yossef
2020-09-16 7:19 ` [PATCH 1/2] dt-bindings: crypto: update ccree optional params Gilad Ben-Yossef
2020-09-16 7:19 ` [PATCH 2/2] crypto: ccree - add custom cache params from DT file Gilad Ben-Yossef
0 siblings, 2 replies; 16+ messages in thread
From: Gilad Ben-Yossef @ 2020-09-16 7:19 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, Rob Herring
Cc: Ofir Drang, linux-crypto, devicetree, linux-kernel
Rework the setting of AXI bus cache parameters, including
optionally allowing setting them from device tree
Gilad Ben-Yossef (2):
dt-bindings: crypto: update ccree optional params
crypto: ccree - add custom cache params from DT file
.../bindings/crypto/arm-cryptocell.txt | 4 +
drivers/crypto/ccree/cc_driver.c | 89 +++++++++++++++----
drivers/crypto/ccree/cc_driver.h | 4 +-
drivers/crypto/ccree/cc_pm.c | 2 +-
4 files changed, 81 insertions(+), 18 deletions(-)
--
2.27.0
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/2] dt-bindings: crypto: update ccree optional params
2020-09-16 7:19 [PATCH 0/2] add optional cache params from DT Gilad Ben-Yossef
@ 2020-09-16 7:19 ` Gilad Ben-Yossef
2020-09-23 1:57 ` Rob Herring
2020-09-16 7:19 ` [PATCH 2/2] crypto: ccree - add custom cache params from DT file Gilad Ben-Yossef
1 sibling, 1 reply; 16+ messages in thread
From: Gilad Ben-Yossef @ 2020-09-16 7:19 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, Rob Herring
Cc: Ofir Drang, linux-crypto, devicetree, linux-kernel
Document ccree driver supporting new optional parameters allowing to
customize the DMA transactions cache parameters and ACE bus sharability
properties.
Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
---
Documentation/devicetree/bindings/crypto/arm-cryptocell.txt | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt b/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt
index 6130e6eb4af8..1a1603e457a8 100644
--- a/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt
+++ b/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt
@@ -13,6 +13,10 @@ Required properties:
Optional properties:
- clocks: Reference to the crypto engine clock.
- dma-coherent: Present if dma operations are coherent.
+- awcache: Set write transactions cache attributes
+- arcache: Set read transactions cache attributes
+- awdomain: Set write transactions ACE sharability domain (712, 703, 713 only)
+- ardomain: Set read transactions ACE sharability domain (712, 703, 713 only)
Examples:
--
2.27.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/2] crypto: ccree - add custom cache params from DT file
2020-09-16 7:19 [PATCH 0/2] add optional cache params from DT Gilad Ben-Yossef
2020-09-16 7:19 ` [PATCH 1/2] dt-bindings: crypto: update ccree optional params Gilad Ben-Yossef
@ 2020-09-16 7:19 ` Gilad Ben-Yossef
2020-09-16 13:47 ` kernel test robot
1 sibling, 1 reply; 16+ messages in thread
From: Gilad Ben-Yossef @ 2020-09-16 7:19 UTC (permalink / raw)
To: Herbert Xu, David S. Miller, Rob Herring
Cc: Ofir Drang, linux-crypto, devicetree, linux-kernel
Add optinal ability to customize DMA transactions cache parameters and
ACE bus sharability properties and set new defaults.
Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
---
drivers/crypto/ccree/cc_driver.c | 89 ++++++++++++++++++++++++++------
drivers/crypto/ccree/cc_driver.h | 4 +-
drivers/crypto/ccree/cc_pm.c | 2 +-
3 files changed, 77 insertions(+), 18 deletions(-)
diff --git a/drivers/crypto/ccree/cc_driver.c b/drivers/crypto/ccree/cc_driver.c
index 6f519d3e896c..db497bc065d4 100644
--- a/drivers/crypto/ccree/cc_driver.c
+++ b/drivers/crypto/ccree/cc_driver.c
@@ -100,6 +100,71 @@ static const struct of_device_id arm_ccree_dev_of_match[] = {
};
MODULE_DEVICE_TABLE(of, arm_ccree_dev_of_match);
+static void init_cc_dt_params(struct cc_drvdata *drvdata)
+{
+ struct device *dev = drvdata_to_dev(drvdata);
+ struct device_node *np = dev->of_node;
+ u32 cache_params, ace_const, val, mask;
+ int rc;
+
+ /* register CC_AXIM_CACHE_PARAMS */
+ cache_params = cc_ioread(drvdata, CC_REG(AXIM_CACHE_PARAMS));
+ dev_dbg(dev, "Cache params previous: 0x%08X\n", cache_params);
+
+ rc = of_property_read_u32(np, "awcache", &val);
+ if (rc)
+ val = (drvdata->coherent ? 0xb : 0x2);
+
+ mask = CC_GENMASK(CC_AXIM_CACHE_PARAMS_AWCACHE);
+ cache_params &= ~mask;
+ cache_params |= FIELD_PREP(mask, val);
+
+ /* write AWCACHE also to AWCACHE_LAST */
+ mask = CC_GENMASK(CC_AXIM_CACHE_PARAMS_AWCACHE_LAST);
+ cache_params &= ~mask;
+ cache_params |= FIELD_PREP(mask, val);
+
+ rc = of_property_read_u32(np, "arcache", &val);
+ if (rc)
+ val = (drvdata->coherent ? 0xb : 0x2);
+
+ mask = CC_GENMASK(CC_AXIM_CACHE_PARAMS_ARCACHE);
+ cache_params &= ~mask;
+ cache_params |= FIELD_PREP(mask, val);
+
+ drvdata->cache_params = cache_params;
+
+ dev_dbg(dev, "Cache params current: 0x%08X\n", cache_params);
+
+ if (drvdata->hw_rev <= CC_HW_REV_710)
+ return;
+
+ /* register CC_AXIM_ACE_CONST */
+ ace_const = cc_ioread(drvdata, CC_REG(AXIM_ACE_CONST));
+ dev_dbg(dev, "ACE-const previous: 0x%08X\n", ace_const);
+
+ rc = of_property_read_u32(np, "ardomain", &val);
+ ace_const = cc_ioread(drvdata, CC_REG(AXIM_ACE_CONST));
+ if (rc)
+ val = (drvdata->coherent ? 0x2 : 0x3);
+
+ mask = CC_GENMASK(CC_AXIM_ACE_CONST_ARDOMAIN);
+ ace_const &= ~mask;
+ ace_const |= FIELD_PREP(mask, val);
+
+ rc = of_property_read_u32(np, "awdomain", &val);
+ if (rc)
+ val = (drvdata->coherent ? 0x2 : 0x3);
+
+ mask = CC_GENMASK(CC_AXIM_ACE_CONST_AWDOMAIN);
+ ace_const &= ~mask;
+ ace_const |= FIELD_PREP(mask, val);
+
+ dev_dbg(dev, "ACE-const current: 0x%08X\n", ace_const);
+
+ drvdata->ace_const = ace_const;
+}
+
static u32 cc_read_idr(struct cc_drvdata *drvdata, const u32 *idr_offsets)
{
int i;
@@ -218,9 +283,9 @@ bool cc_wait_for_reset_completion(struct cc_drvdata *drvdata)
return false;
}
-int init_cc_regs(struct cc_drvdata *drvdata, bool is_probe)
+int init_cc_regs(struct cc_drvdata *drvdata)
{
- unsigned int val, cache_params;
+ unsigned int val;
struct device *dev = drvdata_to_dev(drvdata);
/* Unmask all AXI interrupt sources AXI_CFG1 register */
@@ -245,19 +310,9 @@ int init_cc_regs(struct cc_drvdata *drvdata, bool is_probe)
cc_iowrite(drvdata, CC_REG(HOST_IMR), ~val);
- cache_params = (drvdata->coherent ? CC_COHERENT_CACHE_PARAMS : 0x0);
-
- val = cc_ioread(drvdata, CC_REG(AXIM_CACHE_PARAMS));
-
- if (is_probe)
- dev_dbg(dev, "Cache params previous: 0x%08X\n", val);
-
- cc_iowrite(drvdata, CC_REG(AXIM_CACHE_PARAMS), cache_params);
- val = cc_ioread(drvdata, CC_REG(AXIM_CACHE_PARAMS));
-
- if (is_probe)
- dev_dbg(dev, "Cache params current: 0x%08X (expect: 0x%08X)\n",
- val, cache_params);
+ cc_iowrite(drvdata, CC_REG(AXIM_CACHE_PARAMS), drvdata->cache_params);
+ if (drvdata->hw_rev >= CC_HW_REV_712)
+ cc_iowrite(drvdata, CC_REG(AXIM_ACE_CONST), drvdata->ace_const);
return 0;
}
@@ -445,7 +500,9 @@ static int init_cc_resources(struct platform_device *plat_dev)
}
dev_dbg(dev, "Registered to IRQ: %d\n", irq);
- rc = init_cc_regs(new_drvdata, true);
+ init_cc_dt_params(new_drvdata);
+
+ rc = init_cc_regs(new_drvdata);
if (rc) {
dev_err(dev, "init_cc_regs failed\n");
goto post_pm_err;
diff --git a/drivers/crypto/ccree/cc_driver.h b/drivers/crypto/ccree/cc_driver.h
index af77b2020350..cd5a51e8a281 100644
--- a/drivers/crypto/ccree/cc_driver.h
+++ b/drivers/crypto/ccree/cc_driver.h
@@ -155,6 +155,8 @@ struct cc_drvdata {
int std_bodies;
bool sec_disabled;
u32 comp_mask;
+ u32 cache_params;
+ u32 ace_const;
};
struct cc_crypto_alg {
@@ -205,7 +207,7 @@ static inline void dump_byte_array(const char *name, const u8 *the_array,
}
bool cc_wait_for_reset_completion(struct cc_drvdata *drvdata);
-int init_cc_regs(struct cc_drvdata *drvdata, bool is_probe);
+int init_cc_regs(struct cc_drvdata *drvdata);
void fini_cc_regs(struct cc_drvdata *drvdata);
unsigned int cc_get_default_hash_len(struct cc_drvdata *drvdata);
diff --git a/drivers/crypto/ccree/cc_pm.c b/drivers/crypto/ccree/cc_pm.c
index 3c65bf070c90..d5421b0c6831 100644
--- a/drivers/crypto/ccree/cc_pm.c
+++ b/drivers/crypto/ccree/cc_pm.c
@@ -45,7 +45,7 @@ static int cc_pm_resume(struct device *dev)
}
cc_iowrite(drvdata, CC_REG(HOST_POWER_DOWN_EN), POWER_DOWN_DISABLE);
- rc = init_cc_regs(drvdata, false);
+ rc = init_cc_regs(drvdata);
if (rc) {
dev_err(dev, "init_cc_regs (%x)\n", rc);
return rc;
--
2.27.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] crypto: ccree - add custom cache params from DT file
2020-09-16 7:19 ` [PATCH 2/2] crypto: ccree - add custom cache params from DT file Gilad Ben-Yossef
@ 2020-09-16 13:47 ` kernel test robot
2020-09-17 7:19 ` Gilad Ben-Yossef
0 siblings, 1 reply; 16+ messages in thread
From: kernel test robot @ 2020-09-16 13:47 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 12374 bytes --]
Hi Gilad,
I love your patch! Perhaps something to improve:
[auto build test WARNING on cryptodev/master]
[also build test WARNING on crypto/master robh/for-next v5.9-rc5 next-20200916]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Gilad-Ben-Yossef/add-optional-cache-params-from-DT/20200916-152151
base: https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
config: arm64-randconfig-r015-20200916 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 9e3842d60351f986d77dfe0a94f76e4fd895f188)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/crypto/ccree/cc_driver.c:120:18: warning: result of comparison of constant 18446744073709551615 with expression of type 'u32' (aka 'unsigned int') is always false [-Wtautological-constant-out-of-range-compare]
cache_params |= FIELD_PREP(mask, val);
^~~~~~~~~~~~~~~~~~~~~
include/linux/bitfield.h:94:3: note: expanded from macro 'FIELD_PREP'
__BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/bitfield.h:52:28: note: expanded from macro '__BF_FIELD_CHECK'
BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ull, \
~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:39:58: note: expanded from macro 'BUILD_BUG_ON_MSG'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
include/linux/compiler_types.h:319:22: note: expanded from macro 'compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/compiler_types.h:307:23: note: expanded from macro '_compiletime_assert'
__compiletime_assert(condition, msg, prefix, suffix)
~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/compiler_types.h:299:9: note: expanded from macro '__compiletime_assert'
if (!(condition)) \
^~~~~~~~~
drivers/crypto/ccree/cc_driver.c:125:18: warning: result of comparison of constant 18446744073709551615 with expression of type 'u32' (aka 'unsigned int') is always false [-Wtautological-constant-out-of-range-compare]
cache_params |= FIELD_PREP(mask, val);
^~~~~~~~~~~~~~~~~~~~~
include/linux/bitfield.h:94:3: note: expanded from macro 'FIELD_PREP'
__BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/bitfield.h:52:28: note: expanded from macro '__BF_FIELD_CHECK'
BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ull, \
~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:39:58: note: expanded from macro 'BUILD_BUG_ON_MSG'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
include/linux/compiler_types.h:319:22: note: expanded from macro 'compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/compiler_types.h:307:23: note: expanded from macro '_compiletime_assert'
__compiletime_assert(condition, msg, prefix, suffix)
~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/compiler_types.h:299:9: note: expanded from macro '__compiletime_assert'
if (!(condition)) \
^~~~~~~~~
drivers/crypto/ccree/cc_driver.c:133:18: warning: result of comparison of constant 18446744073709551615 with expression of type 'u32' (aka 'unsigned int') is always false [-Wtautological-constant-out-of-range-compare]
cache_params |= FIELD_PREP(mask, val);
^~~~~~~~~~~~~~~~~~~~~
include/linux/bitfield.h:94:3: note: expanded from macro 'FIELD_PREP'
__BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/bitfield.h:52:28: note: expanded from macro '__BF_FIELD_CHECK'
BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ull, \
~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:39:58: note: expanded from macro 'BUILD_BUG_ON_MSG'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
include/linux/compiler_types.h:319:22: note: expanded from macro 'compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/compiler_types.h:307:23: note: expanded from macro '_compiletime_assert'
__compiletime_assert(condition, msg, prefix, suffix)
~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/compiler_types.h:299:9: note: expanded from macro '__compiletime_assert'
if (!(condition)) \
^~~~~~~~~
drivers/crypto/ccree/cc_driver.c:153:15: warning: result of comparison of constant 18446744073709551615 with expression of type 'u32' (aka 'unsigned int') is always false [-Wtautological-constant-out-of-range-compare]
ace_const |= FIELD_PREP(mask, val);
^~~~~~~~~~~~~~~~~~~~~
include/linux/bitfield.h:94:3: note: expanded from macro 'FIELD_PREP'
__BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/bitfield.h:52:28: note: expanded from macro '__BF_FIELD_CHECK'
BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ull, \
~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:39:58: note: expanded from macro 'BUILD_BUG_ON_MSG'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
include/linux/compiler_types.h:319:22: note: expanded from macro 'compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/compiler_types.h:307:23: note: expanded from macro '_compiletime_assert'
__compiletime_assert(condition, msg, prefix, suffix)
~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/compiler_types.h:299:9: note: expanded from macro '__compiletime_assert'
if (!(condition)) \
^~~~~~~~~
drivers/crypto/ccree/cc_driver.c:161:15: warning: result of comparison of constant 18446744073709551615 with expression of type 'u32' (aka 'unsigned int') is always false [-Wtautological-constant-out-of-range-compare]
ace_const |= FIELD_PREP(mask, val);
^~~~~~~~~~~~~~~~~~~~~
include/linux/bitfield.h:94:3: note: expanded from macro 'FIELD_PREP'
__BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/bitfield.h:52:28: note: expanded from macro '__BF_FIELD_CHECK'
BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ull, \
~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:39:58: note: expanded from macro 'BUILD_BUG_ON_MSG'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
include/linux/compiler_types.h:319:22: note: expanded from macro 'compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/compiler_types.h:307:23: note: expanded from macro '_compiletime_assert'
__compiletime_assert(condition, msg, prefix, suffix)
# https://github.com/0day-ci/linux/commit/3c84102e6f9ba61d7efbadd1ed836eab947106c1
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Gilad-Ben-Yossef/add-optional-cache-params-from-DT/20200916-152151
git checkout 3c84102e6f9ba61d7efbadd1ed836eab947106c1
vim +120 drivers/crypto/ccree/cc_driver.c
102
103 static void init_cc_dt_params(struct cc_drvdata *drvdata)
104 {
105 struct device *dev = drvdata_to_dev(drvdata);
106 struct device_node *np = dev->of_node;
107 u32 cache_params, ace_const, val, mask;
108 int rc;
109
110 /* register CC_AXIM_CACHE_PARAMS */
111 cache_params = cc_ioread(drvdata, CC_REG(AXIM_CACHE_PARAMS));
112 dev_dbg(dev, "Cache params previous: 0x%08X\n", cache_params);
113
114 rc = of_property_read_u32(np, "awcache", &val);
115 if (rc)
116 val = (drvdata->coherent ? 0xb : 0x2);
117
118 mask = CC_GENMASK(CC_AXIM_CACHE_PARAMS_AWCACHE);
119 cache_params &= ~mask;
> 120 cache_params |= FIELD_PREP(mask, val);
121
122 /* write AWCACHE also to AWCACHE_LAST */
123 mask = CC_GENMASK(CC_AXIM_CACHE_PARAMS_AWCACHE_LAST);
124 cache_params &= ~mask;
125 cache_params |= FIELD_PREP(mask, val);
126
127 rc = of_property_read_u32(np, "arcache", &val);
128 if (rc)
129 val = (drvdata->coherent ? 0xb : 0x2);
130
131 mask = CC_GENMASK(CC_AXIM_CACHE_PARAMS_ARCACHE);
132 cache_params &= ~mask;
133 cache_params |= FIELD_PREP(mask, val);
134
135 drvdata->cache_params = cache_params;
136
137 dev_dbg(dev, "Cache params current: 0x%08X\n", cache_params);
138
139 if (drvdata->hw_rev <= CC_HW_REV_710)
140 return;
141
142 /* register CC_AXIM_ACE_CONST */
143 ace_const = cc_ioread(drvdata, CC_REG(AXIM_ACE_CONST));
144 dev_dbg(dev, "ACE-const previous: 0x%08X\n", ace_const);
145
146 rc = of_property_read_u32(np, "ardomain", &val);
147 ace_const = cc_ioread(drvdata, CC_REG(AXIM_ACE_CONST));
148 if (rc)
149 val = (drvdata->coherent ? 0x2 : 0x3);
150
151 mask = CC_GENMASK(CC_AXIM_ACE_CONST_ARDOMAIN);
152 ace_const &= ~mask;
153 ace_const |= FIELD_PREP(mask, val);
154
155 rc = of_property_read_u32(np, "awdomain", &val);
156 if (rc)
157 val = (drvdata->coherent ? 0x2 : 0x3);
158
159 mask = CC_GENMASK(CC_AXIM_ACE_CONST_AWDOMAIN);
160 ace_const &= ~mask;
161 ace_const |= FIELD_PREP(mask, val);
162
163 dev_dbg(dev, "ACE-const current: 0x%08X\n", ace_const);
164
165 drvdata->ace_const = ace_const;
166 }
167
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 38931 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] crypto: ccree - add custom cache params from DT file
2020-09-16 13:47 ` kernel test robot
@ 2020-09-17 7:19 ` Gilad Ben-Yossef
2020-09-18 19:39 ` Nick Desaulniers
0 siblings, 1 reply; 16+ messages in thread
From: Gilad Ben-Yossef @ 2020-09-17 7:19 UTC (permalink / raw)
To: kernel test robot
Cc: Herbert Xu, David S. Miller, Rob Herring, kbuild-all,
clang-built-linux, netdev, Ofir Drang, Linux Crypto Mailing List,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux kernel mailing list
hmm...
On Wed, Sep 16, 2020 at 4:48 PM kernel test robot <lkp@intel.com> wrote:
>
> url: https://github.com/0day-ci/linux/commits/Gilad-Ben-Yossef/add-optional-cache-params-from-DT/20200916-152151
> base: https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
> config: arm64-randconfig-r015-20200916 (attached as .config)
> compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 9e3842d60351f986d77dfe0a94f76e4fd895f188)
> reproduce (this is a W=1 build):
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # install arm64 cross compiling tool for clang build
> # apt-get install binutils-aarch64-linux-gnu
> # save the attached .config to linux build tree
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
>
> All warnings (new ones prefixed by >>):
>
> >> drivers/crypto/ccree/cc_driver.c:120:18: warning: result of comparison of constant 18446744073709551615 with expression of type 'u32' (aka 'unsigned int') is always false [-Wtautological-constant-out-of-range-compare]
> cache_params |= FIELD_PREP(mask, val);
> ^~~~~~~~~~~~~~~~~~~~~
> include/linux/bitfield.h:94:3: note: expanded from macro 'FIELD_PREP'
> __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> include/linux/bitfield.h:52:28: note: expanded from macro '__BF_FIELD_CHECK'
> BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ull, \
> ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> include/linux/build_bug.h:39:58: note: expanded from macro 'BUILD_BUG_ON_MSG'
> #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
> ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
> include/linux/compiler_types.h:319:22: note: expanded from macro 'compiletime_assert'
> _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> include/linux/compiler_types.h:307:23: note: expanded from macro '_compiletime_assert'
> __compiletime_assert(condition, msg, prefix, suffix)
> ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> include/linux/compiler_types.h:299:9: note: expanded from macro '__compiletime_assert'
> if (!(condition)) \
> ^~~~~~~~~
I am unable to understand this warning. It looks like it is
complaining about a FIELD_GET sanity check that is always false, which
makes sense since we're using a constant.
Anyone can enlighten me if I've missed something?
Thanks,
Gilad
--
Gilad Ben-Yossef
Chief Coffee Drinker
values of β will give rise to dom!
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] crypto: ccree - add custom cache params from DT file
2020-09-17 7:19 ` Gilad Ben-Yossef
@ 2020-09-18 19:39 ` Nick Desaulniers
0 siblings, 0 replies; 16+ messages in thread
From: Nick Desaulniers @ 2020-09-18 19:39 UTC (permalink / raw)
To: Gilad Ben-Yossef
Cc: kernel test robot, Herbert Xu, David S. Miller, Rob Herring,
kbuild-all, clang-built-linux, Network Development, Ofir Drang,
Linux Crypto Mailing List,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux kernel mailing list, Sami Tolvanen, Alex Elder,
Jakub Kicinski
On Thu, Sep 17, 2020 at 12:20 AM Gilad Ben-Yossef <gilad@benyossef.com> wrote:
>
> hmm...
>
> On Wed, Sep 16, 2020 at 4:48 PM kernel test robot <lkp@intel.com> wrote:
> >
> > url: https://github.com/0day-ci/linux/commits/Gilad-Ben-Yossef/add-optional-cache-params-from-DT/20200916-152151
> > base: https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
> > config: arm64-randconfig-r015-20200916 (attached as .config)
> > compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 9e3842d60351f986d77dfe0a94f76e4fd895f188)
> > reproduce (this is a W=1 build):
> > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> > chmod +x ~/bin/make.cross
> > # install arm64 cross compiling tool for clang build
> > # apt-get install binutils-aarch64-linux-gnu
> > # save the attached .config to linux build tree
> > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64
> >
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <lkp@intel.com>
> >
> > All warnings (new ones prefixed by >>):
> >
> > >> drivers/crypto/ccree/cc_driver.c:120:18: warning: result of comparison of constant 18446744073709551615 with expression of type 'u32' (aka 'unsigned int') is always false [-Wtautological-constant-out-of-range-compare]
> > cache_params |= FIELD_PREP(mask, val);
> > ^~~~~~~~~~~~~~~~~~~~~
> > include/linux/bitfield.h:94:3: note: expanded from macro 'FIELD_PREP'
> > __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > include/linux/bitfield.h:52:28: note: expanded from macro '__BF_FIELD_CHECK'
> > BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ull, \
> > ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > include/linux/build_bug.h:39:58: note: expanded from macro 'BUILD_BUG_ON_MSG'
> > #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
> > ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
> > include/linux/compiler_types.h:319:22: note: expanded from macro 'compiletime_assert'
> > _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> > ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > include/linux/compiler_types.h:307:23: note: expanded from macro '_compiletime_assert'
> > __compiletime_assert(condition, msg, prefix, suffix)
> > ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > include/linux/compiler_types.h:299:9: note: expanded from macro '__compiletime_assert'
> > if (!(condition)) \
> > ^~~~~~~~~
>
> I am unable to understand this warning. It looks like it is
> complaining about a FIELD_GET sanity check that is always false, which
> makes sense since we're using a constant.
>
> Anyone can enlighten me if I've missed something?
Looked at some of this code recently. I think it may have an issue
for masks where sizeof(mask) < sizeof(unsigned long long).
In your code, via 0day bot:
107 u32 cache_params, ace_const, val, mask;
...
> 120 cache_params |= FIELD_PREP(mask, val);
then in include/linux/bitfield.h, we have:
92 #define FIELD_PREP(_mask, _val) \
93 ({ \
94 __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
44 #define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \
...
52 BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ull, \
53 _pfx "type of reg too small for mask"); \
so the 0ULL in FIELD_PREP is important. In __BF_FIELD_CHECK, the
typeof(_reg) is unsigned long long (because 0ULL was passed). So we
have a comparison between a u32 and a u64; indeed any u32 can never be
greater than a u64 that we know has the value of ULLONG_MAX.
I did send a series splitting these up, I wonder if they'd help here:
https://lore.kernel.org/lkml/20200708230402.1644819-3-ndesaulniers@google.com/
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] crypto: ccree - add custom cache params from DT file
@ 2020-09-18 19:39 ` Nick Desaulniers
0 siblings, 0 replies; 16+ messages in thread
From: Nick Desaulniers @ 2020-09-18 19:39 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 4344 bytes --]
On Thu, Sep 17, 2020 at 12:20 AM Gilad Ben-Yossef <gilad@benyossef.com> wrote:
>
> hmm...
>
> On Wed, Sep 16, 2020 at 4:48 PM kernel test robot <lkp@intel.com> wrote:
> >
> > url: https://github.com/0day-ci/linux/commits/Gilad-Ben-Yossef/add-optional-cache-params-from-DT/20200916-152151
> > base: https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
> > config: arm64-randconfig-r015-20200916 (attached as .config)
> > compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 9e3842d60351f986d77dfe0a94f76e4fd895f188)
> > reproduce (this is a W=1 build):
> > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> > chmod +x ~/bin/make.cross
> > # install arm64 cross compiling tool for clang build
> > # apt-get install binutils-aarch64-linux-gnu
> > # save the attached .config to linux build tree
> > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64
> >
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <lkp@intel.com>
> >
> > All warnings (new ones prefixed by >>):
> >
> > >> drivers/crypto/ccree/cc_driver.c:120:18: warning: result of comparison of constant 18446744073709551615 with expression of type 'u32' (aka 'unsigned int') is always false [-Wtautological-constant-out-of-range-compare]
> > cache_params |= FIELD_PREP(mask, val);
> > ^~~~~~~~~~~~~~~~~~~~~
> > include/linux/bitfield.h:94:3: note: expanded from macro 'FIELD_PREP'
> > __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > include/linux/bitfield.h:52:28: note: expanded from macro '__BF_FIELD_CHECK'
> > BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ull, \
> > ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > include/linux/build_bug.h:39:58: note: expanded from macro 'BUILD_BUG_ON_MSG'
> > #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
> > ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
> > include/linux/compiler_types.h:319:22: note: expanded from macro 'compiletime_assert'
> > _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> > ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > include/linux/compiler_types.h:307:23: note: expanded from macro '_compiletime_assert'
> > __compiletime_assert(condition, msg, prefix, suffix)
> > ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > include/linux/compiler_types.h:299:9: note: expanded from macro '__compiletime_assert'
> > if (!(condition)) \
> > ^~~~~~~~~
>
> I am unable to understand this warning. It looks like it is
> complaining about a FIELD_GET sanity check that is always false, which
> makes sense since we're using a constant.
>
> Anyone can enlighten me if I've missed something?
Looked at some of this code recently. I think it may have an issue
for masks where sizeof(mask) < sizeof(unsigned long long).
In your code, via 0day bot:
107 u32 cache_params, ace_const, val, mask;
...
> 120 cache_params |= FIELD_PREP(mask, val);
then in include/linux/bitfield.h, we have:
92 #define FIELD_PREP(_mask, _val) \
93 ({ \
94 __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
44 #define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \
...
52 BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ull, \
53 _pfx "type of reg too small for mask"); \
so the 0ULL in FIELD_PREP is important. In __BF_FIELD_CHECK, the
typeof(_reg) is unsigned long long (because 0ULL was passed). So we
have a comparison between a u32 and a u64; indeed any u32 can never be
greater than a u64 that we know has the value of ULLONG_MAX.
I did send a series splitting these up, I wonder if they'd help here:
https://lore.kernel.org/lkml/20200708230402.1644819-3-ndesaulniers(a)google.com/
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] crypto: ccree - add custom cache params from DT file
2020-09-18 19:39 ` Nick Desaulniers
(?)
@ 2020-09-21 7:46 ` Gilad Ben-Yossef
-1 siblings, 0 replies; 16+ messages in thread
From: Gilad Ben-Yossef @ 2020-09-21 7:46 UTC (permalink / raw)
To: Nick Desaulniers
Cc: kernel test robot, Herbert Xu, David S. Miller, Rob Herring,
kbuild-all, clang-built-linux, Network Development, Ofir Drang,
Linux Crypto Mailing List,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux kernel mailing list, Sami Tolvanen, Alex Elder,
Jakub Kicinski
Hi,
On Fri, Sep 18, 2020 at 10:39 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Thu, Sep 17, 2020 at 12:20 AM Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> >
...
> >
> > I am unable to understand this warning. It looks like it is
> > complaining about a FIELD_GET sanity check that is always false, which
> > makes sense since we're using a constant.
> >
> > Anyone can enlighten me if I've missed something?
>
> Looked at some of this code recently. I think it may have an issue
> for masks where sizeof(mask) < sizeof(unsigned long long).
>
> In your code, via 0day bot:
>
> 107 u32 cache_params, ace_const, val, mask;
> ...
> > 120 cache_params |= FIELD_PREP(mask, val);
>
> then in include/linux/bitfield.h, we have:
>
> 92 #define FIELD_PREP(_mask, _val) \
> 93 ({ \
> 94 __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
>
> 44 #define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \
> ...
> 52 BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ull, \
> 53 _pfx "type of reg too small for mask"); \
>
> so the 0ULL in FIELD_PREP is important. In __BF_FIELD_CHECK, the
> typeof(_reg) is unsigned long long (because 0ULL was passed). So we
> have a comparison between a u32 and a u64; indeed any u32 can never be
> greater than a u64 that we know has the value of ULLONG_MAX.
>
> I did send a series splitting these up, I wonder if they'd help here:
> https://lore.kernel.org/lkml/20200708230402.1644819-3-ndesaulniers@google.com/
> --
Thanks!
This indeed explains this. It seems there is nothing for me to do
about it in the driver code though, as it seems the issue is in the
macro and you have already posted a fix for it.
Thanks again,
Gilad
--
Gilad Ben-Yossef
Chief Coffee Drinker
values of β will give rise to dom!
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] dt-bindings: crypto: update ccree optional params
2020-09-16 7:19 ` [PATCH 1/2] dt-bindings: crypto: update ccree optional params Gilad Ben-Yossef
@ 2020-09-23 1:57 ` Rob Herring
2020-09-29 18:08 ` Gilad Ben-Yossef
0 siblings, 1 reply; 16+ messages in thread
From: Rob Herring @ 2020-09-23 1:57 UTC (permalink / raw)
To: Gilad Ben-Yossef
Cc: Herbert Xu, David S. Miller, Ofir Drang, linux-crypto,
devicetree, linux-kernel
On Wed, Sep 16, 2020 at 10:19:49AM +0300, Gilad Ben-Yossef wrote:
> Document ccree driver supporting new optional parameters allowing to
> customize the DMA transactions cache parameters and ACE bus sharability
> properties.
>
> Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
> ---
> Documentation/devicetree/bindings/crypto/arm-cryptocell.txt | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt b/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt
> index 6130e6eb4af8..1a1603e457a8 100644
> --- a/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt
> +++ b/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt
> @@ -13,6 +13,10 @@ Required properties:
> Optional properties:
> - clocks: Reference to the crypto engine clock.
> - dma-coherent: Present if dma operations are coherent.
> +- awcache: Set write transactions cache attributes
> +- arcache: Set read transactions cache attributes
dma-coherent already implies these are 011x, 101x or 111x. In my limited
experience configuring these (Calxeda SATA and ethernet), writeback,
write-allocate was pretty much always optimal.
> +- awdomain: Set write transactions ACE sharability domain (712, 703, 713 only)
> +- ardomain: Set read transactions ACE sharability domain (712, 703, 713 only)
This probably needs something common. We may need something for Mali,
too. I don't think different settings for read and write makes much
sense nor does anything beyond IS or OS.
These could also just be implied by the compatible string (and requiring
an SoC specific one).
Rob
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] dt-bindings: crypto: update ccree optional params
2020-09-23 1:57 ` Rob Herring
@ 2020-09-29 18:08 ` Gilad Ben-Yossef
[not found] ` <CAOtvUMfAKnodo+7EYx2M4yAvxu_VmxwXNRmgOW=KFWi3Wy7msQ@mail.gmail.com>
0 siblings, 1 reply; 16+ messages in thread
From: Gilad Ben-Yossef @ 2020-09-29 18:08 UTC (permalink / raw)
To: Rob Herring
Cc: Herbert Xu, David S. Miller, Ofir Drang,
Linux Crypto Mailing List,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux kernel mailing list
Hןת
On Wed, Sep 23, 2020 at 4:57 AM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Sep 16, 2020 at 10:19:49AM +0300, Gilad Ben-Yossef wrote:
> > Document ccree driver supporting new optional parameters allowing to
> > customize the DMA transactions cache parameters and ACE bus sharability
> > properties.
> >
> > Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
> > ---
> > Documentation/devicetree/bindings/crypto/arm-cryptocell.txt | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt b/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt
> > index 6130e6eb4af8..1a1603e457a8 100644
> > --- a/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt
> > +++ b/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt
> > @@ -13,6 +13,10 @@ Required properties:
> > Optional properties:
> > - clocks: Reference to the crypto engine clock.
> > - dma-coherent: Present if dma operations are coherent.
> > +- awcache: Set write transactions cache attributes
> > +- arcache: Set read transactions cache attributes
>
> dma-coherent already implies these are 011x, 101x or 111x. In my limited
> experience configuring these (Calxeda SATA and ethernet), writeback,
> write-allocate was pretty much always optimal.
Indeed and these are the default. But not all SoC are born equal and
we got a request to allow setting these.
Maybe instead of numerical values have three possible verbal setting
would be better?
> > +- awdomain: Set write transactions ACE sharability domain (712, 703, 713 only)
> > +- ardomain: Set read transactions ACE sharability domain (712, 703, 713 only)
>
> This probably needs something common. We may need something for Mali,
> too. I don't think different settings for read and write makes much
> sense nor does anything beyond IS or OS.
I agree. Maybe
sharability_domain: either "IS" or "OS"?
>
> These could also just be implied by the compatible string (and requiring
> an SoC specific one).
hm... we could do it but this will require us to know (and publicly
acknowledge) of every SoC making use of this piece of hardware design.
There is currently no other part of the driver that needs this.
Gilad
--
Gilad Ben-Yossef
Chief Coffee Drinker
values of β will give rise to dom!
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] dt-bindings: crypto: update ccree optional params
[not found] ` <CAOtvUMfAKnodo+7EYx2M4yAvxu_VmxwXNRmgOW=KFWi3Wy7msQ@mail.gmail.com>
@ 2020-11-16 18:54 ` Rob Herring
2020-11-17 7:39 ` Gilad Ben-Yossef
2020-11-17 14:07 ` Robin Murphy
0 siblings, 2 replies; 16+ messages in thread
From: Rob Herring @ 2020-11-16 18:54 UTC (permalink / raw)
To: Gilad Ben-Yossef
Cc: Herbert Xu, David S. Miller, Ofir Drang,
Linux Crypto Mailing List,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux kernel mailing list, Robin Murphy, Steven Price
On Thu, Oct 22, 2020 at 1:18 AM Gilad Ben-Yossef <gilad@benyossef.com> wrote:
>
>
> Hi again,
>
> Any opinion on the suggested below?
Sorry, lost in the pile...
> Thanks!
> Gilad
>
>
> On Tue, Sep 29, 2020 at 9:08 PM Gilad Ben-Yossef <gilad@benyossef.com> wrote:
>>
>>
>> On Wed, Sep 23, 2020 at 4:57 AM Rob Herring <robh@kernel.org> wrote:
>> >
>> > On Wed, Sep 16, 2020 at 10:19:49AM +0300, Gilad Ben-Yossef wrote:
>> > > Document ccree driver supporting new optional parameters allowing to
>> > > customize the DMA transactions cache parameters and ACE bus sharability
>> > > properties.
>> > >
>> > > Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
>> > > ---
>> > > Documentation/devicetree/bindings/crypto/arm-cryptocell.txt | 4 ++++
>> > > 1 file changed, 4 insertions(+)
>> > >
>> > > diff --git a/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt b/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt
>> > > index 6130e6eb4af8..1a1603e457a8 100644
>> > > --- a/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt
>> > > +++ b/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt
>> > > @@ -13,6 +13,10 @@ Required properties:
>> > > Optional properties:
>> > > - clocks: Reference to the crypto engine clock.
>> > > - dma-coherent: Present if dma operations are coherent.
>> > > +- awcache: Set write transactions cache attributes
>> > > +- arcache: Set read transactions cache attributes
>> >
>> > dma-coherent already implies these are 011x, 101x or 111x. In my limited
>> > experience configuring these (Calxeda SATA and ethernet), writeback,
>> > write-allocate was pretty much always optimal.
>>
>> Indeed and these are the default. But not all SoC are born equal and
>> we got a request to allow setting these.
>>
>> Maybe instead of numerical values have three possible verbal setting
>> would be better?
>>
>>
>> > > +- awdomain: Set write transactions ACE sharability domain (712, 703, 713 only)
>> > > +- ardomain: Set read transactions ACE sharability domain (712, 703, 713 only)
>> >
>> > This probably needs something common. We may need something for Mali,
>> > too. I don't think different settings for read and write makes much
>> > sense nor does anything beyond IS or OS.
>>
>> I agree. Maybe
>>
>> sharability_domain: either "IS" or "OS"?
It's still an Arm thing, so it would need at least an 'arm,' prefix.
But ideally it wouldn't be Arm specific though I'm not sure if any
such thing is needed for other arches. If common either for Arm or
across arches, then it needs to be documented in a common doc with
some wider agreement than what a device specific property needs.
>> > These could also just be implied by the compatible string (and requiring
>> > an SoC specific one).
>>
>> hm... we could do it but this will require us to know (and publicly
>> acknowledge) of every SoC making use of this piece of hardware design.
That's already a requirement in general. Sometimes we can avoid it,
but that's cases of getting lucky.
>> There is currently no other part of the driver that needs this.
If your DT is part of firmware, then waiting until adding some driver
feature or quirk based on a new DT property is too late. Whereas with
a SoC specific compatible, you can handle any new feature or quirk
without a DT change (e.g. just a stable kernel update). Some platforms
may not care about that model, but in general that's the policy we
follow. Not doing that, we end up with the DWC3 binding.
A fallback compatible is how we avoid updating drivers for every
single SoC unless needed.
Rob
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] dt-bindings: crypto: update ccree optional params
2020-11-16 18:54 ` Rob Herring
@ 2020-11-17 7:39 ` Gilad Ben-Yossef
2020-11-17 14:58 ` Rob Herring
2020-11-17 14:07 ` Robin Murphy
1 sibling, 1 reply; 16+ messages in thread
From: Gilad Ben-Yossef @ 2020-11-17 7:39 UTC (permalink / raw)
To: Rob Herring
Cc: Herbert Xu, David S. Miller, Ofir Drang,
Linux Crypto Mailing List,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux kernel mailing list, Robin Murphy, Steven Price
On Mon, Nov 16, 2020 at 8:54 PM Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Oct 22, 2020 at 1:18 AM Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> >
> >
> > Hi again,
> >
> > Any opinion on the suggested below?
>
> Sorry, lost in the pile...
No problem at all. I know how it is...
> >
> >
> > On Tue, Sep 29, 2020 at 9:08 PM Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> >>
> >>
> >> On Wed, Sep 23, 2020 at 4:57 AM Rob Herring <robh@kernel.org> wrote:
> >> >
> >> > On Wed, Sep 16, 2020 at 10:19:49AM +0300, Gilad Ben-Yossef wrote:
> >> > > Document ccree driver supporting new optional parameters allowing to
> >> > > customize the DMA transactions cache parameters and ACE bus sharability
> >> > > properties.
> >> > >
> >> > > Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
> >> > > ---
> >> > > Documentation/devicetree/bindings/crypto/arm-cryptocell.txt | 4 ++++
> >> > > 1 file changed, 4 insertions(+)
> >> > >
> >> > > diff --git a/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt b/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt
> >> > > index 6130e6eb4af8..1a1603e457a8 100644
> >> > > --- a/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt
> >> > > +++ b/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt
> >> > > @@ -13,6 +13,10 @@ Required properties:
> >> > > Optional properties:
> >> > > - clocks: Reference to the crypto engine clock.
> >> > > - dma-coherent: Present if dma operations are coherent.
> >> > > +- awcache: Set write transactions cache attributes
> >> > > +- arcache: Set read transactions cache attributes
> >> >
<snip>
> >> > These could also just be implied by the compatible string (and requiring
> >> > an SoC specific one).
> >>
> >> hm... we could do it but this will require us to know (and publicly
> >> acknowledge) of every SoC making use of this piece of hardware design.
>
> That's already a requirement in general. Sometimes we can avoid it,
> but that's cases of getting lucky.
>
> >> There is currently no other part of the driver that needs this.
>
> If your DT is part of firmware, then waiting until adding some driver
> feature or quirk based on a new DT property is too late. Whereas with
> a SoC specific compatible, you can handle any new feature or quirk
> without a DT change (e.g. just a stable kernel update). Some platforms
> may not care about that model, but in general that's the policy we
> follow. Not doing that, we end up with the DWC3 binding.
>
> A fallback compatible is how we avoid updating drivers for every
> single SoC unless needed.
OK, I now better understand what you meant and that does make some
sense and I will defer to your better judgment about the proper way
to do this.
Having said that, there is still something that bugs me about this,
even just at the level of better understanding why we do things:
Way back when, before DT, we had SoC specific code that identified the
SoC somehow and set things up in a SoC specific way.
Then we introduced DT as a way to say - "hey look, this is how my
busses looks like, these are the devices I have, deal with it" and I
always assumed that this was meant as a way to release us from having
SoC specific setup code.
It seems now potentially every SoC vendor needs to modify not just the
device tree source (which makes sense of course) but also the driver
supporting their platform.
It now looks like we've come a full circle to me :-)
Thanks!
Gilad
--
Gilad Ben-Yossef
Chief Coffee Drinker
values of β will give rise to dom!
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] dt-bindings: crypto: update ccree optional params
2020-11-16 18:54 ` Rob Herring
2020-11-17 7:39 ` Gilad Ben-Yossef
@ 2020-11-17 14:07 ` Robin Murphy
2020-11-19 11:44 ` Gilad Ben-Yossef
1 sibling, 1 reply; 16+ messages in thread
From: Robin Murphy @ 2020-11-17 14:07 UTC (permalink / raw)
To: Rob Herring, Gilad Ben-Yossef
Cc: Herbert Xu, David S. Miller, Ofir Drang,
Linux Crypto Mailing List,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux kernel mailing list, Steven Price
On 2020-11-16 18:54, Rob Herring wrote:
> On Thu, Oct 22, 2020 at 1:18 AM Gilad Ben-Yossef <gilad@benyossef.com> wrote:
>>
>>
>> Hi again,
>>
>> Any opinion on the suggested below?
>
> Sorry, lost in the pile...
>
>> Thanks!
>> Gilad
>>
>>
>> On Tue, Sep 29, 2020 at 9:08 PM Gilad Ben-Yossef <gilad@benyossef.com> wrote:
>>>
>>>
>>> On Wed, Sep 23, 2020 at 4:57 AM Rob Herring <robh@kernel.org> wrote:
>>>>
>>>> On Wed, Sep 16, 2020 at 10:19:49AM +0300, Gilad Ben-Yossef wrote:
>>>>> Document ccree driver supporting new optional parameters allowing to
>>>>> customize the DMA transactions cache parameters and ACE bus sharability
>>>>> properties.
>>>>>
>>>>> Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
>>>>> ---
>>>>> Documentation/devicetree/bindings/crypto/arm-cryptocell.txt | 4 ++++
>>>>> 1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt b/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt
>>>>> index 6130e6eb4af8..1a1603e457a8 100644
>>>>> --- a/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt
>>>>> +++ b/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt
>>>>> @@ -13,6 +13,10 @@ Required properties:
>>>>> Optional properties:
>>>>> - clocks: Reference to the crypto engine clock.
>>>>> - dma-coherent: Present if dma operations are coherent.
>>>>> +- awcache: Set write transactions cache attributes
>>>>> +- arcache: Set read transactions cache attributes
>>>>
>>>> dma-coherent already implies these are 011x, 101x or 111x. In my limited
>>>> experience configuring these (Calxeda SATA and ethernet), writeback,
>>>> write-allocate was pretty much always optimal.
>>>
>>> Indeed and these are the default. But not all SoC are born equal and
>>> we got a request to allow setting these.
>>>
>>> Maybe instead of numerical values have three possible verbal setting
>>> would be better?
>>>
>>>
>>>>> +- awdomain: Set write transactions ACE sharability domain (712, 703, 713 only)
>>>>> +- ardomain: Set read transactions ACE sharability domain (712, 703, 713 only)
>>>>
>>>> This probably needs something common. We may need something for Mali,
>>>> too. I don't think different settings for read and write makes much
>>>> sense nor does anything beyond IS or OS.
>>>
>>> I agree. Maybe
>>>
>>> sharability_domain: either "IS" or "OS"?
>
> It's still an Arm thing, so it would need at least an 'arm,' prefix.
> But ideally it wouldn't be Arm specific though I'm not sure if any
> such thing is needed for other arches. If common either for Arm or
> across arches, then it needs to be documented in a common doc with
> some wider agreement than what a device specific property needs.
"dma-coherent" already encapsulates the shareability. To claim coherency
you need to be able to access memory with inner shareable inner-outer
write back attributes, because that's how regular kernel memory is
mapped. If you don't claim coherency, then you should be using
non-cacheable (and thus implicitly outer shareable) attributes, because
there may still be cacheable aliases hanging around and if you
inadvertently snoop those instead of going direct to DRAM as everything
else expects, you're going to have a bad time.
Essentially, Linux only uses two sets of memory attributes for DMA
memory, so any perceived need for more than that is already something
sufficiently esoteric that it probably doesn't need a generic binding.
I'm aware that it's against type for me to be arguing Linux details in a
DT binding, but I checked the TRMs for some of these devices and they
essentially say "you don't program the hardware directly, you use the
Linux driver we give you", so it seems like this binding is specifically
intended never to be consumed by anything else.
>>>> These could also just be implied by the compatible string (and requiring
>>>> an SoC specific one).
>>>
>>> hm... we could do it but this will require us to know (and publicly
>>> acknowledge) of every SoC making use of this piece of hardware design.
>
> That's already a requirement in general. Sometimes we can avoid it,
> but that's cases of getting lucky.
>
>>> There is currently no other part of the driver that needs this.
>
> If your DT is part of firmware, then waiting until adding some driver
> feature or quirk based on a new DT property is too late. Whereas with
> a SoC specific compatible, you can handle any new feature or quirk
> without a DT change (e.g. just a stable kernel update). Some platforms
> may not care about that model, but in general that's the policy we
> follow. Not doing that, we end up with the DWC3 binding.
>
> A fallback compatible is how we avoid updating drivers for every
> single SoC unless needed.
IMO if this is like PL330 where you just stick some raw AXI attributes
in a register and that's what goes out on transactions then it's not
really part of the platform description anyway, it's just driver policy.
If folks want to tweak the driver's behaviour for secret SoC-specific
performance optimisation, then give them some module parameters or a
sysfs interface. I'm assuming this has to be purely a performance thing,
because I can't see how two different integrations could reasonably
depend on different baseline attributes for correctness, and even if
they did then you'd be able to determine that from the compatible
strings, because they would be different, because those integrations
would be fundamentally *not* compatible with each other.
Robin.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] dt-bindings: crypto: update ccree optional params
2020-11-17 7:39 ` Gilad Ben-Yossef
@ 2020-11-17 14:58 ` Rob Herring
2020-11-19 11:41 ` Gilad Ben-Yossef
0 siblings, 1 reply; 16+ messages in thread
From: Rob Herring @ 2020-11-17 14:58 UTC (permalink / raw)
To: Gilad Ben-Yossef
Cc: Herbert Xu, David S. Miller, Ofir Drang,
Linux Crypto Mailing List,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux kernel mailing list, Robin Murphy, Steven Price
On Tue, Nov 17, 2020 at 1:39 AM Gilad Ben-Yossef <gilad@benyossef.com> wrote:
>
> On Mon, Nov 16, 2020 at 8:54 PM Rob Herring <robh@kernel.org> wrote:
> >
> > On Thu, Oct 22, 2020 at 1:18 AM Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> > >
> > >
> > > Hi again,
> > >
> > > Any opinion on the suggested below?
> >
> > Sorry, lost in the pile...
>
> No problem at all. I know how it is...
>
>
> > >
> > >
> > > On Tue, Sep 29, 2020 at 9:08 PM Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> > >>
> > >>
> > >> On Wed, Sep 23, 2020 at 4:57 AM Rob Herring <robh@kernel.org> wrote:
> > >> >
> > >> > On Wed, Sep 16, 2020 at 10:19:49AM +0300, Gilad Ben-Yossef wrote:
> > >> > > Document ccree driver supporting new optional parameters allowing to
> > >> > > customize the DMA transactions cache parameters and ACE bus sharability
> > >> > > properties.
> > >> > >
> > >> > > Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
> > >> > > ---
> > >> > > Documentation/devicetree/bindings/crypto/arm-cryptocell.txt | 4 ++++
> > >> > > 1 file changed, 4 insertions(+)
> > >> > >
> > >> > > diff --git a/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt b/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt
> > >> > > index 6130e6eb4af8..1a1603e457a8 100644
> > >> > > --- a/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt
> > >> > > +++ b/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt
> > >> > > @@ -13,6 +13,10 @@ Required properties:
> > >> > > Optional properties:
> > >> > > - clocks: Reference to the crypto engine clock.
> > >> > > - dma-coherent: Present if dma operations are coherent.
> > >> > > +- awcache: Set write transactions cache attributes
> > >> > > +- arcache: Set read transactions cache attributes
> > >> >
>
> <snip>
>
> > >> > These could also just be implied by the compatible string (and requiring
> > >> > an SoC specific one).
> > >>
> > >> hm... we could do it but this will require us to know (and publicly
> > >> acknowledge) of every SoC making use of this piece of hardware design.
> >
> > That's already a requirement in general. Sometimes we can avoid it,
> > but that's cases of getting lucky.
> >
> > >> There is currently no other part of the driver that needs this.
> >
> > If your DT is part of firmware, then waiting until adding some driver
> > feature or quirk based on a new DT property is too late. Whereas with
> > a SoC specific compatible, you can handle any new feature or quirk
> > without a DT change (e.g. just a stable kernel update). Some platforms
> > may not care about that model, but in general that's the policy we
> > follow. Not doing that, we end up with the DWC3 binding.
> >
> > A fallback compatible is how we avoid updating drivers for every
> > single SoC unless needed.
>
> OK, I now better understand what you meant and that does make some
> sense and I will defer to your better judgment about the proper way
> to do this.
>
> Having said that, there is still something that bugs me about this,
> even just at the level of better understanding why we do things:
>
> Way back when, before DT, we had SoC specific code that identified the
> SoC somehow and set things up in a SoC specific way.
> Then we introduced DT as a way to say - "hey look, this is how my
> busses looks like, these are the devices I have, deal with it" and I
> always assumed that this was meant as a way to release us from having
> SoC specific setup code.
Yes, but in the end it's a judgement call as to what the boundary is.
Take clocks for example, in the beginning we were trying to describe
clocks on a mux/divider/gate level in DT. We realized this would
result in hundreds to thousands of DT nodes and it would never be
completely correct. So we model only the leaf clocks for the most part
and there's lots of SoC code for clock controller blocks still.
The questions for having properties or not to ask is:
Is this board specific? Yes, then use a property.
Who needs to change this and when? Should/would you want to control
this in your PC BIOS or via a BIOS update?
Zero SoC code was never a goal. It was about a standard way to define
SoCs and having common frameworks (we have a common clock api, but not
implementation at the time). We have *way* less SoC code per SoC than
we used to. At the start of the DT conversion for Arm, we had ~400
boards and now we're at ~1400 last I checked.
> It seems now potentially every SoC vendor needs to modify not just the
> device tree source (which makes sense of course) but also the driver
> supporting their platform.
> It now looks like we've come a full circle to me :-)
As I said, if the h/w is 'exactly the same' (hint: it rarely is), then
use a fallback compatible. Then the new SoC specific compatible is
there just in case.
Think of compatible just as a VID/PID in PCI and USB land (though the
closest thing to a fallback there is class codes). They are the only
way we can uniquely identify h/w.
Rob
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] dt-bindings: crypto: update ccree optional params
2020-11-17 14:58 ` Rob Herring
@ 2020-11-19 11:41 ` Gilad Ben-Yossef
0 siblings, 0 replies; 16+ messages in thread
From: Gilad Ben-Yossef @ 2020-11-19 11:41 UTC (permalink / raw)
To: Rob Herring
Cc: Herbert Xu, David S. Miller, Ofir Drang,
Linux Crypto Mailing List,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux kernel mailing list, Robin Murphy, Steven Price
On Tue, Nov 17, 2020 at 4:58 PM Rob Herring <robh@kernel.org> wrote:
> >
> > > >> > These could also just be implied by the compatible string (and requiring
> > > >> > an SoC specific one).
> > > >>
> > > >> hm... we could do it but this will require us to know (and publicly
> > > >> acknowledge) of every SoC making use of this piece of hardware design.
> > >
> > > That's already a requirement in general. Sometimes we can avoid it,
> > > but that's cases of getting lucky.
> > >
> > > >> There is currently no other part of the driver that needs this.
> > >
> > > If your DT is part of firmware, then waiting until adding some driver
> > > feature or quirk based on a new DT property is too late. Whereas with
> > > a SoC specific compatible, you can handle any new feature or quirk
> > > without a DT change (e.g. just a stable kernel update). Some platforms
> > > may not care about that model, but in general that's the policy we
> > > follow. Not doing that, we end up with the DWC3 binding.
> > >
> > > A fallback compatible is how we avoid updating drivers for every
> > > single SoC unless needed.
> >
> > OK, I now better understand what you meant and that does make some
> > sense and I will defer to your better judgment about the proper way
> > to do this.
> >
> > Having said that, there is still something that bugs me about this,
> > even just at the level of better understanding why we do things:
> >
> > Way back when, before DT, we had SoC specific code that identified the
> > SoC somehow and set things up in a SoC specific way.
> > Then we introduced DT as a way to say - "hey look, this is how my
> > busses looks like, these are the devices I have, deal with it" and I
> > always assumed that this was meant as a way to release us from having
> > SoC specific setup code.
>
> Yes, but in the end it's a judgement call as to what the boundary is.
> Take clocks for example, in the beginning we were trying to describe
> clocks on a mux/divider/gate level in DT. We realized this would
> result in hundreds to thousands of DT nodes and it would never be
> completely correct. So we model only the leaf clocks for the most part
> and there's lots of SoC code for clock controller blocks still.
>
> The questions for having properties or not to ask is:
>
> Is this board specific? Yes, then use a property.
>
> Who needs to change this and when? Should/would you want to control
> this in your PC BIOS or via a BIOS update?
>
>
> Zero SoC code was never a goal. It was about a standard way to define
> SoCs and having common frameworks (we have a common clock api, but not
> implementation at the time). We have *way* less SoC code per SoC than
> we used to. At the start of the DT conversion for Arm, we had ~400
> boards and now we're at ~1400 last I checked.
>
> > It seems now potentially every SoC vendor needs to modify not just the
> > device tree source (which makes sense of course) but also the driver
> > supporting their platform.
> > It now looks like we've come a full circle to me :-)
>
> As I said, if the h/w is 'exactly the same' (hint: it rarely is), then
> use a fallback compatible. Then the new SoC specific compatible is
> there just in case.
>
> Think of compatible just as a VID/PID in PCI and USB land (though the
> closest thing to a fallback there is class codes). They are the only
> way we can uniquely identify h/w.
Thanks Rob, this makes sense.
Gilad
--
Gilad Ben-Yossef
Chief Coffee Drinker
values of β will give rise to dom!
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] dt-bindings: crypto: update ccree optional params
2020-11-17 14:07 ` Robin Murphy
@ 2020-11-19 11:44 ` Gilad Ben-Yossef
0 siblings, 0 replies; 16+ messages in thread
From: Gilad Ben-Yossef @ 2020-11-19 11:44 UTC (permalink / raw)
To: Robin Murphy
Cc: Rob Herring, Herbert Xu, David S. Miller, Ofir Drang,
Linux Crypto Mailing List,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux kernel mailing list, Steven Price
Hi,
On Tue, Nov 17, 2020 at 4:07 PM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2020-11-16 18:54, Rob Herring wrote:
> > On Thu, Oct 22, 2020 at 1:18 AM Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> >>
...
>
> IMO if this is like PL330 where you just stick some raw AXI attributes
> in a register and that's what goes out on transactions then it's not
> really part of the platform description anyway, it's just driver policy.
> If folks want to tweak the driver's behaviour for secret SoC-specific
> performance optimisation, then give them some module parameters or a
> sysfs interface. I'm assuming this has to be purely a performance thing,
> because I can't see how two different integrations could reasonably
> depend on different baseline attributes for correctness, and even if
> they did then you'd be able to determine that from the compatible
> strings, because they would be different, because those integrations
> would be fundamentally *not* compatible with each other.
>
So, I checked internally where we get this requirement and it seems
you are right.
I'm dropping the Dt bindings and in fact the ability to customize
those attributes beyond
the basic coherent/non coherent configuration which we already support
and will just
update the setting to what we think is best.
Thanks for clearing this up.
Gilad
--
Gilad Ben-Yossef
Chief Coffee Drinker
values of β will give rise to dom!
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-11-19 11:45 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-16 7:19 [PATCH 0/2] add optional cache params from DT Gilad Ben-Yossef
2020-09-16 7:19 ` [PATCH 1/2] dt-bindings: crypto: update ccree optional params Gilad Ben-Yossef
2020-09-23 1:57 ` Rob Herring
2020-09-29 18:08 ` Gilad Ben-Yossef
[not found] ` <CAOtvUMfAKnodo+7EYx2M4yAvxu_VmxwXNRmgOW=KFWi3Wy7msQ@mail.gmail.com>
2020-11-16 18:54 ` Rob Herring
2020-11-17 7:39 ` Gilad Ben-Yossef
2020-11-17 14:58 ` Rob Herring
2020-11-19 11:41 ` Gilad Ben-Yossef
2020-11-17 14:07 ` Robin Murphy
2020-11-19 11:44 ` Gilad Ben-Yossef
2020-09-16 7:19 ` [PATCH 2/2] crypto: ccree - add custom cache params from DT file Gilad Ben-Yossef
2020-09-16 13:47 ` kernel test robot
2020-09-17 7:19 ` Gilad Ben-Yossef
2020-09-18 19:39 ` Nick Desaulniers
2020-09-18 19:39 ` Nick Desaulniers
2020-09-21 7:46 ` Gilad Ben-Yossef
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.