Currently, when device tree specifies fsl,qe-num-snums = 28 (which a
number of in-tree .dts files do, and which is the default when that
property is missing), qe_snums_init() ends up using the first 28
elements of the snum_init_46[] array.
The situation is quite messy. This patch may break existing setups
that for some reason work with specifying fsl,qe-num-snums = 28 and
using the existing snum_init_46 array. OTOH, the current code
certainly does not work for the MPC8309-based board we're working on,
since the first 14 of the elements in snum_init_46 are "Not available
on MPC8306/MPC8306S/MPC8309" according to the QUICC Engine Reference
Manual (Table 4-30) - and indeed, without this patch (or something to
the same effect), we get
[ 8.895758] ------------[ cut here ]------------
[ 8.895778] NETDEV WATCHDOG: eth0 (ucc_geth): transmit queue 0 timed out
[ 8.895971] WARNING: CPU: 0 PID: 8 at net/sched/sch_generic.c:461 dev_watchdog+0x23c/0x244
[ 8.895977] Modules linked in:
[ 8.895998] CPU: 0 PID: 8 Comm: kworker/u2:1 Not tainted 4.19.18-00012-gd47efbb0119d #183
[ 8.896017] Workqueue: events_unbound call_usermodehelper_exec_work
[ 8.896030] NIP: c037b18c LR: c037b18c CTR: 00000000
[ 8.896042] REGS: cf853b00 TRAP: 0700 Not tainted (4.19.18-00012-gd47efbb0119d)
[ 8.896047] MSR: 00029032 <EE,ME,IR,DR,RI> CR: 42022428 XER: 00000000
[ 8.896080]
[ 8.896080] GPR00: c037b18c cf853bb0 cf82e4c0 0000003c c05f7bc4 000000c4 0000004c 000038dc
[ 8.896080] GPR08: 00000007 00000007 00000001 00000000 22022424 00000000 00000100 c05f3d78
[ 8.896080] GPR16: c05f3d7c 00000001 00000004 cf852000 00000000 c05e0000 fffee3a9 c0496618
[ 8.896080] GPR24: c05241c8 0000000a c05c0000 c05db0f4 cf82b800 c05e0000 00000000 cf82ba74
[ 8.896229] NIP [c037b18c] dev_watchdog+0x23c/0x244
[ 8.896240] LR [c037b18c] dev_watchdog+0x23c/0x244
[ 8.896244] Call Trace:
[ 8.896257] [cf853bb0] [c037b18c] dev_watchdog+0x23c/0x244 (unreliable)
[ 8.896285] [cf853bd0] [c0054eb0] call_timer_fn+0x24/0x84
[ 8.896304] [cf853bf0] [c0055228] expire_timers.isra.4+0x98/0xa8
[ 8.896322] [cf853c10] [c00552cc] run_timer_softirq+0x94/0x190
[ 8.896341] [cf853c60] [c0494738] __do_softirq+0xe0/0x258
[ 8.896357] [cf853cc0] [c001db68] irq_exit+0xfc/0x100
[ 8.896375] [cf853cd0] [c000a2e8] timer_interrupt+0xdc/0x1c0
[ 8.896399] [cf853cf0] [c000f460] ret_from_except+0x0/0x14
[ 8.896432] --- interrupt: 901 at copy_process.isra.8.part.9+0x60c/0x1334
[ 8.896432] LR = copy_process.isra.8.part.9+0x864/0x1334
[ 8.896446] [cf853db0] [c0018fc8] copy_process.isra.8.part.9+0x7d0/0x1334 (unreliable)
[ 8.896466] [cf853e40] [c0019fc0] _do_fork+0xa8/0x2ac
[ 8.896486] [cf853e80] [c002b7e0] call_usermodehelper_exec_work+0x74/0xec
[ 8.896506] [cf853ea0] [c002e520] process_one_work+0x168/0x38c
[ 8.896523] [cf853ec0] [c002e87c] worker_thread+0x138/0x488
[ 8.896541] [cf853f10] [c0033824] kthread+0xe0/0x10c
[ 8.896560] [cf853f40] [c000f1c4] ret_from_kernel_thread+0x14/0x1c
[ 8.896568] Instruction dump:
[ 8.896577] 811ffffc 4bffff70 39200001 7f83e378 9928af9f 4bfd82d5 7fc6f378 7f84e378
[ 8.896615] 7c651b78 3c60c056 3863be98 4bc9f681 <0fe00000> 4bffffb8 9421ffe0 7d800026
[ 8.896655] ---[ end trace cfd3ddfe80d72be2 ]---
along with an endless sequence of
[ 115.716012] ucc_geth e0102000.ethernet eth1: Link is Up - 100Mbps/Full - flow control off
[ 117.744010] ucc_geth e0103000.ethernet eth0: Link is Up - 100Mbps/Full - flow control off
[ 119.295955] ucc_geth e0102000.ethernet eth1: Link is Down
[ 119.712177] ucc_geth e0102000.ethernet eth1: Link is Up - 100Mbps/Full - flow control off
[ 123.295801] ucc_geth e0102000.ethernet eth1: Link is Down
[ 123.295843] ucc_geth e0103000.ethernet eth0: Link is Down
[ 123.712022] ucc_geth e0102000.ethernet eth1: Link is Up - 100Mbps/Full - flow control off
[ 125.744167] ucc_geth e0103000.ethernet eth0: Link is Up - 100Mbps/Full - flow control off
It would seem to make much more sense of the table of snums was either
decided based on some compatible string, or alternatively, if the
array of snums was simply a byte array read directly from DT.
Signed-off-by: Rasmus Villemoes <[email protected]>
---
drivers/soc/fsl/qe/qe.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c
index 2ef6fc6487c1..185da4be3509 100644
--- a/drivers/soc/fsl/qe/qe.c
+++ b/drivers/soc/fsl/qe/qe.c
@@ -306,12 +306,21 @@ static void qe_snums_init(void)
0x28, 0x29, 0x38, 0x39, 0x48, 0x49, 0x58, 0x59,
0x68, 0x69, 0x78, 0x79, 0x80, 0x81,
};
+ static const u8 snum_init_28[] = {
+ 0x88, 0x89, 0x98, 0x99, 0xa8, 0xa9, 0xb8, 0xb9,
+ 0xc8, 0xc9, 0xd8, 0xd9, 0xe8, 0xe9,
+ 0x08, 0x09, 0x18, 0x19, 0x28, 0x29, 0x38, 0x39,
+ 0x48, 0x49, 0x58, 0x59, 0x68, 0x69,
+ };
+
static const u8 *snum_init;
qe_num_of_snum = qe_get_num_of_snums();
if (qe_num_of_snum == 76)
snum_init = snum_init_76;
+ else if (qe_num_of_snum == 28)
+ snum_init = snum_init_28;
else
snum_init = snum_init_46;
--
2.20.1
On Tue, Feb 26, 2019 at 2:49 AM Rasmus Villemoes <[email protected]> wrote:
> -----Original Message-----
> From: Rasmus Villemoes <[email protected]>
> Sent: 2019??2??26?? 16:48
> To: Qiang Zhao <[email protected]>; Leo Li <[email protected]>
> Cc: [email protected]; Valentin Longchamp
> <[email protected]>; Scott Wood <[email protected]>;
> Rasmus Villemoes <[email protected]>
> Subject: [RFC PATCH] soc/fsl/qe: support MPC8309
>
> Currently, when device tree specifies fsl,qe-num-snums = 28 (which a number
> of in-tree .dts files do, and which is the default when that property is missing),
> qe_snums_init() ends up using the first 28 elements of the snum_init_46[]
> array.
>
> The situation is quite messy. This patch may break existing setups that for
> some reason work with specifying fsl,qe-num-snums = 28 and using the
> existing snum_init_46 array. OTOH, the current code certainly does not work
> for the MPC8309-based board we're working on, since the first 14 of the
> elements in snum_init_46 are "Not available on
> MPC8306/MPC8306S/MPC8309" according to the QUICC Engine Reference
> Manual (Table 4-30) - and indeed, without this patch (or something to the
> same effect), we get
According to the QUICC Engine Reference Manual (Table 4-30), the number of snums used for
" MPC8306/MPC8306S/MPC8309" should be 14 instead of 28, so maybe we should assign "fsl,qe-num-snums = 14"
And define a new snum_init_14 array, meanwhile, modify the minimum value of "num_of_snums" to 14 in function "qe_get_num_of_snums"
(I mean: " if ((num_of_snums < 14) || (num_of_snums > QE_NUM_OF_SNUM)) {")
Best Regards
Qiang Zhao
On 28/02/2019 08.14, Qiang Zhao wrote:
> On Tue, Feb 26, 2019 at 2:49 AM Rasmus Villemoes <[email protected]> wrote:
>> -----Original Message-----
>> From: Rasmus Villemoes <[email protected]>
>> Sent: 2019年2月26日 16:48
>> To: Qiang Zhao <[email protected]>; Leo Li <[email protected]>
>> Cc: [email protected]; Valentin Longchamp
>> <[email protected]>; Scott Wood <[email protected]>;
>> Rasmus Villemoes <[email protected]>
>> Subject: [RFC PATCH] soc/fsl/qe: support MPC8309
>>
>> Currently, when device tree specifies fsl,qe-num-snums = 28 (which a number
>> of in-tree .dts files do, and which is the default when that property is missing),
>> qe_snums_init() ends up using the first 28 elements of the snum_init_46[]
>> array.
>>
>> The situation is quite messy. This patch may break existing setups that for
>> some reason work with specifying fsl,qe-num-snums = 28 and using the
>> existing snum_init_46 array. OTOH, the current code certainly does not work
>> for the MPC8309-based board we're working on, since the first 14 of the
>> elements in snum_init_46 are "Not available on
>> MPC8306/MPC8306S/MPC8309" according to the QUICC Engine Reference
>> Manual (Table 4-30) - and indeed, without this patch (or something to the
>> same effect), we get
>
> According to the QUICC Engine Reference Manual (Table 4-30), the number of snums used for
> " MPC8306/MPC8306S/MPC8309" should be 14 instead of 28, so maybe we should assign "fsl,qe-num-snums = 14"
> And define a new snum_init_14 array, meanwhile, modify the minimum value of "num_of_snums" to 14 in function "qe_get_num_of_snums"
> (I mean: " if ((num_of_snums < 14) || (num_of_snums > QE_NUM_OF_SNUM)) {")
Perhaps, but it's completely unclear to me why the code would contain a
comment /* No QE ever has fewer than 28 SNUMs */ when that then appears
to be wrong. It is also far from clear why the snums such as 0x08, 0x09
etc. that appear in table 4-30 without any assigned meaning do appear in
the existing snum_init_46 table - but of course, those elements of that
array don't currently happen to be used when qe_get_num_of_snums==28
(but they do get used for e.g. mpc8569 which specifies fsl,qe-num-snums=46).
Altogether, I think it's a completely wrong approach to pretend that the
_set of valid snums_ is a function of the _number of snums_ alone - the
footnotes to table 4-30 make it abundantly clear that one really cannot
expect such a simple relationship.
So, I suggest deprecating the fsl,qe-num-snums binding, in favor of a
more sensible
fsl,qe-snums = /bits/ 8 <0x88 0x89 0x98 0x99 ...>
which will automatically encode both the number of snums as well as the
concrete values to be used.
Rasmus
This is what I have in mind for a fsl,qe-snums property. The second
patch in the series has the side effect of making it very easy to
introduce that, since the of_property_read_variable_u8_array helper
does exactly what we need in terms of verifying the array length and
copying out the values to the snums array.
This should make it easier to support all of the QE variants out
there, instead of teaching the qe driver some magic mapping from
qe-num-snums to actual snum values.
Rasmus Villemoes (4):
soc/fsl/qe: qe.c: drop useless static qualifier
soc/fsl/qe: qe.c: reduce static memory footprint by 1.7K
soc/fsl/qe: qe.c: introduce qe_get_device_node helper
soc/fsl/qe: qe.c: support fsl,qe-snums property
.../devicetree/bindings/soc/fsl/cpm_qe/qe.txt | 8 +-
drivers/soc/fsl/qe/qe.c | 124 ++++++++----------
2 files changed, 62 insertions(+), 70 deletions(-)
--
2.20.1
The local variable snum_init has no reason to have static storage duration.
Signed-off-by: Rasmus Villemoes <[email protected]>
---
drivers/soc/fsl/qe/qe.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c
index 2ef6fc6487c1..4b6aa6b3b685 100644
--- a/drivers/soc/fsl/qe/qe.c
+++ b/drivers/soc/fsl/qe/qe.c
@@ -306,7 +306,7 @@ static void qe_snums_init(void)
0x28, 0x29, 0x38, 0x39, 0x48, 0x49, 0x58, 0x59,
0x68, 0x69, 0x78, 0x79, 0x80, 0x81,
};
- static const u8 *snum_init;
+ const u8 *snum_init;
qe_num_of_snum = qe_get_num_of_snums();
--
2.20.1
The 'try of_find_compatible_node(NULL, NULL, "fsl,qe"), fall back to
of_find_node_by_type(NULL, "qe")' pattern is repeated five
times. Factor it into a common helper.
Signed-off-by: Rasmus Villemoes <[email protected]>
---
drivers/soc/fsl/qe/qe.c | 71 +++++++++++++++++------------------------
1 file changed, 29 insertions(+), 42 deletions(-)
diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c
index 9b9737a583ef..71beeb72eee4 100644
--- a/drivers/soc/fsl/qe/qe.c
+++ b/drivers/soc/fsl/qe/qe.c
@@ -56,6 +56,20 @@ static unsigned int qe_num_of_snum;
static phys_addr_t qebase = -1;
+static struct device_node *qe_get_device_node(void)
+{
+ struct device_node *qe;
+
+ /*
+ * Newer device trees have an "fsl,qe" compatible property for the QE
+ * node, but we still need to support older device trees.
+ */
+ qe = of_find_compatible_node(NULL, NULL, "fsl,qe");
+ if (qe)
+ return qe;
+ return of_find_node_by_type(NULL, "qe");
+}
+
static phys_addr_t get_qe_base(void)
{
struct device_node *qe;
@@ -65,12 +79,9 @@ static phys_addr_t get_qe_base(void)
if (qebase != -1)
return qebase;
- qe = of_find_compatible_node(NULL, NULL, "fsl,qe");
- if (!qe) {
- qe = of_find_node_by_type(NULL, "qe");
- if (!qe)
- return qebase;
- }
+ qe = qe_get_device_node();
+ if (!qe)
+ return qebase;
ret = of_address_to_resource(qe, 0, &res);
if (!ret)
@@ -164,12 +175,9 @@ unsigned int qe_get_brg_clk(void)
if (brg_clk)
return brg_clk;
- qe = of_find_compatible_node(NULL, NULL, "fsl,qe");
- if (!qe) {
- qe = of_find_node_by_type(NULL, "qe");
- if (!qe)
- return brg_clk;
- }
+ qe = qe_get_device_node();
+ if (!qe)
+ return brg_clk;
prop = of_get_property(qe, "brg-frequency", &size);
if (prop && size == sizeof(*prop))
@@ -563,16 +571,9 @@ struct qe_firmware_info *qe_get_firmware_info(void)
initialized = 1;
- /*
- * Newer device trees have an "fsl,qe" compatible property for the QE
- * node, but we still need to support older device trees.
- */
- qe = of_find_compatible_node(NULL, NULL, "fsl,qe");
- if (!qe) {
- qe = of_find_node_by_type(NULL, "qe");
- if (!qe)
- return NULL;
- }
+ qe = qe_get_device_node();
+ if (!qe)
+ return NULL;
/* Find the 'firmware' child node */
for_each_child_of_node(qe, fw) {
@@ -622,16 +623,9 @@ unsigned int qe_get_num_of_risc(void)
unsigned int num_of_risc = 0;
const u32 *prop;
- qe = of_find_compatible_node(NULL, NULL, "fsl,qe");
- if (!qe) {
- /* Older devices trees did not have an "fsl,qe"
- * compatible property, so we need to look for
- * the QE node by name.
- */
- qe = of_find_node_by_type(NULL, "qe");
- if (!qe)
- return num_of_risc;
- }
+ qe = qe_get_device_node();
+ if (!qe)
+ return num_of_risc;
prop = of_get_property(qe, "fsl,qe-num-riscs", &size);
if (prop && size == sizeof(*prop))
@@ -651,16 +645,9 @@ unsigned int qe_get_num_of_snums(void)
const u32 *prop;
num_of_snums = 28; /* The default number of snum for threads is 28 */
- qe = of_find_compatible_node(NULL, NULL, "fsl,qe");
- if (!qe) {
- /* Older devices trees did not have an "fsl,qe"
- * compatible property, so we need to look for
- * the QE node by name.
- */
- qe = of_find_node_by_type(NULL, "qe");
- if (!qe)
- return num_of_snums;
- }
+ qe = qe_get_device_node();
+ if (!qe)
+ return num_of_snums;
prop = of_get_property(qe, "fsl,qe-num-snums", &size);
if (prop && size == sizeof(*prop)) {
--
2.20.1
The current array of struct qe_snum use 256*4 bytes for just keeping
track of the free/used state of each index, and the struct layout
means there's another 768 bytes of padding. If we just unzip that
structure, the array of snum values just use 256 bytes, while the
free/inuse state can be tracked in a 32 byte bitmap.
So this reduces the .data footprint by 1760 bytes. It also serves as
preparation for introducing another DT binding for specifying the snum
values.
Signed-off-by: Rasmus Villemoes <[email protected]>
---
drivers/soc/fsl/qe/qe.c | 37 ++++++++++++-------------------------
1 file changed, 12 insertions(+), 25 deletions(-)
diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c
index 4b6aa6b3b685..9b9737a583ef 100644
--- a/drivers/soc/fsl/qe/qe.c
+++ b/drivers/soc/fsl/qe/qe.c
@@ -14,6 +14,7 @@
* Free Software Foundation; either version 2 of the License, or (at your
* option) any later version.
*/
+#include <linux/bitmap.h>
#include <linux/errno.h>
#include <linux/sched.h>
#include <linux/kernel.h>
@@ -43,25 +44,14 @@ static DEFINE_SPINLOCK(qe_lock);
DEFINE_SPINLOCK(cmxgcr_lock);
EXPORT_SYMBOL(cmxgcr_lock);
-/* QE snum state */
-enum qe_snum_state {
- QE_SNUM_STATE_USED,
- QE_SNUM_STATE_FREE
-};
-
-/* QE snum */
-struct qe_snum {
- u8 num;
- enum qe_snum_state state;
-};
-
/* We allocate this here because it is used almost exclusively for
* the communication processor devices.
*/
struct qe_immap __iomem *qe_immr;
EXPORT_SYMBOL(qe_immr);
-static struct qe_snum snums[QE_NUM_OF_SNUM]; /* Dynamically allocated SNUMs */
+static u8 snums[QE_NUM_OF_SNUM]; /* Dynamically allocated SNUMs */
+static DECLARE_BITMAP(snum_state, QE_NUM_OF_SNUM);
static unsigned int qe_num_of_snum;
static phys_addr_t qebase = -1;
@@ -308,6 +298,7 @@ static void qe_snums_init(void)
};
const u8 *snum_init;
+ bitmap_zero(snum_state, QE_NUM_OF_SNUM);
qe_num_of_snum = qe_get_num_of_snums();
if (qe_num_of_snum == 76)
@@ -315,10 +306,8 @@ static void qe_snums_init(void)
else
snum_init = snum_init_46;
- for (i = 0; i < qe_num_of_snum; i++) {
- snums[i].num = snum_init[i];
- snums[i].state = QE_SNUM_STATE_FREE;
- }
+ for (i = 0; i < qe_num_of_snum; i++)
+ snums[i] = snum_init[i];
}
int qe_get_snum(void)
@@ -328,12 +317,10 @@ int qe_get_snum(void)
int i;
spin_lock_irqsave(&qe_lock, flags);
- for (i = 0; i < qe_num_of_snum; i++) {
- if (snums[i].state == QE_SNUM_STATE_FREE) {
- snums[i].state = QE_SNUM_STATE_USED;
- snum = snums[i].num;
- break;
- }
+ i = find_first_zero_bit(snum_state, qe_num_of_snum);
+ if (i < qe_num_of_snum) {
+ set_bit(i, snum_state);
+ snum = snums[i];
}
spin_unlock_irqrestore(&qe_lock, flags);
@@ -346,8 +333,8 @@ void qe_put_snum(u8 snum)
int i;
for (i = 0; i < qe_num_of_snum; i++) {
- if (snums[i].num == snum) {
- snums[i].state = QE_SNUM_STATE_FREE;
+ if (snums[i] == snum) {
+ clear_bit(i, snum_state);
break;
}
}
--
2.20.1
The current code assumes that the set of snum _values_ to populate the
snums[] array with is a function of the _number_ of snums
alone. However, reading table 4-30, and its footnotes, of the QUICC
Engine Block Reference Manual shows that that is a bit too naive.
As an alternative, this introduces a new binding fsl,qe-snums, which
automatically encodes both the number of snums and the actual values to
use. Conveniently, of_property_read_variable_u8_array does exactly
what we need.
For example, for the MPC8309, one would specify the property as
fsl,qe-snums = /bits/ 8 <
0x88 0x89 0x98 0x99 0xa8 0xa9 0xb8 0xb9
0xc8 0xc9 0xd8 0xd9 0xe8 0xe9>;
Signed-off-by: Rasmus Villemoes <[email protected]>
---
.../devicetree/bindings/soc/fsl/cpm_qe/qe.txt | 8 +++++++-
drivers/soc/fsl/qe/qe.c | 14 +++++++++++++-
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/soc/fsl/cpm_qe/qe.txt b/Documentation/devicetree/bindings/soc/fsl/cpm_qe/qe.txt
index d7afaff5faff..05f5f485562a 100644
--- a/Documentation/devicetree/bindings/soc/fsl/cpm_qe/qe.txt
+++ b/Documentation/devicetree/bindings/soc/fsl/cpm_qe/qe.txt
@@ -18,7 +18,8 @@ Required properties:
- reg : offset and length of the device registers.
- bus-frequency : the clock frequency for QUICC Engine.
- fsl,qe-num-riscs: define how many RISC engines the QE has.
-- fsl,qe-num-snums: define how many serial number(SNUM) the QE can use for the
+- fsl,qe-snums: This property has to be specified as '/bits/ 8' value,
+ defining the array of serial number (SNUM) values for the virtual
threads.
Optional properties:
@@ -34,6 +35,11 @@ Recommended properties
- brg-frequency : the internal clock source frequency for baud-rate
generators in Hz.
+Deprecated properties
+- fsl,qe-num-snums: define how many serial number(SNUM) the QE can use
+ for the threads. Use fsl,qe-snums instead to not only specify the
+ number of snums, but also their values.
+
Example:
qe@e0100000 {
#address-cells = <1>;
diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c
index 71beeb72eee4..049b36d6aeee 100644
--- a/drivers/soc/fsl/qe/qe.c
+++ b/drivers/soc/fsl/qe/qe.c
@@ -283,7 +283,6 @@ EXPORT_SYMBOL(qe_clock_source);
*/
static void qe_snums_init(void)
{
- int i;
static const u8 snum_init_76[] = {
0x04, 0x05, 0x0C, 0x0D, 0x14, 0x15, 0x1C, 0x1D,
0x24, 0x25, 0x2C, 0x2D, 0x34, 0x35, 0x88, 0x89,
@@ -304,9 +303,22 @@ static void qe_snums_init(void)
0x28, 0x29, 0x38, 0x39, 0x48, 0x49, 0x58, 0x59,
0x68, 0x69, 0x78, 0x79, 0x80, 0x81,
};
+ struct device_node *qe;
const u8 *snum_init;
+ int i;
bitmap_zero(snum_state, QE_NUM_OF_SNUM);
+ qe = qe_get_device_node();
+ if (qe) {
+ i = of_property_read_variable_u8_array(qe, "fsl,qe-snums",
+ snums, 14, QE_NUM_OF_SNUM);
+ of_node_put(qe);
+ if (i > 0) {
+ qe_num_of_snum = i;
+ return;
+ }
+ }
+
qe_num_of_snum = qe_get_num_of_snums();
if (qe_num_of_snum == 76)
--
2.20.1
On 2019??2??28?? 18:31??Rasmus Villemoes wrote:
> -----Original Message-----
> From: Rasmus Villemoes <[email protected]>
> Sent: 2019??2??28?? 18:31
> To: Qiang Zhao <[email protected]>; Leo Li <[email protected]>
> Cc: Scott Wood <[email protected]>; [email protected]; Timur Tabi
> <[email protected]>; Rasmus Villemoes <[email protected]>
> Subject: [PATCH 4/4] soc/fsl/qe: qe.c: support fsl,qe-snums property
>
> The current code assumes that the set of snum _values_ to populate the
> snums[] array with is a function of the _number_ of snums alone. However,
> reading table 4-30, and its footnotes, of the QUICC Engine Block Reference
> Manual shows that that is a bit too naive.
>
> As an alternative, this introduces a new binding fsl,qe-snums, which
> automatically encodes both the number of snums and the actual values to use.
> Conveniently, of_property_read_variable_u8_array does exactly what we need.
>
> For example, for the MPC8309, one would specify the property as
>
> fsl,qe-snums = /bits/ 8 <
> 0x88 0x89 0x98 0x99 0xa8 0xa9 0xb8 0xb9
> 0xc8 0xc9 0xd8 0xd9 0xe8 0xe9>;
>
> Signed-off-by: Rasmus Villemoes <[email protected]>
> ---
> .../devicetree/bindings/soc/fsl/cpm_qe/qe.txt | 8 +++++++-
> drivers/soc/fsl/qe/qe.c | 14 +++++++++++++-
> 2 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/soc/fsl/cpm_qe/qe.txt
> b/Documentation/devicetree/bindings/soc/fsl/cpm_qe/qe.txt
> index d7afaff5faff..05f5f485562a 100644
> --- a/Documentation/devicetree/bindings/soc/fsl/cpm_qe/qe.txt
> +++ b/Documentation/devicetree/bindings/soc/fsl/cpm_qe/qe.txt
> @@ -18,7 +18,8 @@ Required properties:
> - reg : offset and length of the device registers.
> - bus-frequency : the clock frequency for QUICC Engine.
> - fsl,qe-num-riscs: define how many RISC engines the QE has.
> -- fsl,qe-num-snums: define how many serial number(SNUM) the QE can use
> for the
> +- fsl,qe-snums: This property has to be specified as '/bits/ 8' value,
> + defining the array of serial number (SNUM) values for the virtual
> threads.
>
> Optional properties:
> @@ -34,6 +35,11 @@ Recommended properties
> - brg-frequency : the internal clock source frequency for baud-rate
> generators in Hz.
>
> +Deprecated properties
> +- fsl,qe-num-snums: define how many serial number(SNUM) the QE can use
> + for the threads. Use fsl,qe-snums instead to not only specify the
> + number of snums, but also their values.
> +
> Example:
> qe@e0100000 {
> #address-cells = <1>;
> diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c index
> 71beeb72eee4..049b36d6aeee 100644
> --- a/drivers/soc/fsl/qe/qe.c
> +++ b/drivers/soc/fsl/qe/qe.c
> @@ -283,7 +283,6 @@ EXPORT_SYMBOL(qe_clock_source);
> */
> static void qe_snums_init(void)
> {
> - int i;
> static const u8 snum_init_76[] = {
> 0x04, 0x05, 0x0C, 0x0D, 0x14, 0x15, 0x1C, 0x1D,
> 0x24, 0x25, 0x2C, 0x2D, 0x34, 0x35, 0x88, 0x89, @@ -304,9
> +303,22 @@ static void qe_snums_init(void)
> 0x28, 0x29, 0x38, 0x39, 0x48, 0x49, 0x58, 0x59,
> 0x68, 0x69, 0x78, 0x79, 0x80, 0x81,
> };
> + struct device_node *qe;
> const u8 *snum_init;
> + int i;
>
> bitmap_zero(snum_state, QE_NUM_OF_SNUM);
> + qe = qe_get_device_node();
> + if (qe) {
> + i = of_property_read_variable_u8_array(qe, "fsl,qe-snums",
> + snums, 14, QE_NUM_OF_SNUM);
> + of_node_put(qe);
> + if (i > 0) {
> + qe_num_of_snum = i;
> + return;
> + }
> + }
> +
> qe_num_of_snum = qe_get_num_of_snums();
>
> if (qe_num_of_snum == 76)
So you define 14 snums for MPC8309, but there still be the comment "/* No QE ever has fewer than 28 SNUMs */" and it will check if
The num_of_snums "is >28", it will cause confusion, so I suggest to modify 28 to 14.
I read the old version QUICC Engine Block Reference Manual, it said snums table is not available on MPC8306/MPC8306S/MPC8309,
So I think the code it written long before with this version RM, and at that time, the snums is at least 28, and nobody modify the code later.
And now with the new version RM, it support MPC8306/MPC8306S/MPC8309 with snums and have snums fewer then 28, so I think the minimum value should
Be modified to 14.
Thank you!
-Qiang Zhao
On 01/03/2019 04.36, Qiang Zhao wrote:
> On 2019年2月28日 18:31，Rasmus Villemoes wrote:
>
>> -----Original Message-----
>> From: Rasmus Villemoes <[email protected]>
>> Sent: 2019年2月28日 18:31
>> To: Qiang Zhao <[email protected]>; Leo Li <[email protected]>
>> Cc: Scott Wood <[email protected]>; [email protected]; Timur Tabi
>> <[email protected]>; Rasmus Villemoes <[email protected]>
>> Subject: [PATCH 4/4] soc/fsl/qe: qe.c: support fsl,qe-snums property
>>
>> The current code assumes that the set of snum _values_ to populate the
>> snums[] array with is a function of the _number_ of snums alone. However,
>> reading table 4-30, and its footnotes, of the QUICC Engine Block Reference
>> Manual shows that that is a bit too naive.
>>
>> As an alternative, this introduces a new binding fsl,qe-snums, which
>> automatically encodes both the number of snums and the actual values to use.
>> Conveniently, of_property_read_variable_u8_array does exactly what we need.
>>
>> For example, for the MPC8309, one would specify the property as
>>
>> fsl,qe-snums = /bits/ 8 <
>> 0x88 0x89 0x98 0x99 0xa8 0xa9 0xb8 0xb9
>> 0xc8 0xc9 0xd8 0xd9 0xe8 0xe9>;
>>
>> Signed-off-by: Rasmus Villemoes <[email protected]>
>> ---
>> .../devicetree/bindings/soc/fsl/cpm_qe/qe.txt | 8 +++++++-
>> drivers/soc/fsl/qe/qe.c | 14 +++++++++++++-
>> 2 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/soc/fsl/cpm_qe/qe.txt
>> b/Documentation/devicetree/bindings/soc/fsl/cpm_qe/qe.txt
>> index d7afaff5faff..05f5f485562a 100644
>> --- a/Documentation/devicetree/bindings/soc/fsl/cpm_qe/qe.txt
>> +++ b/Documentation/devicetree/bindings/soc/fsl/cpm_qe/qe.txt
>> @@ -18,7 +18,8 @@ Required properties:
>> - reg : offset and length of the device registers.
>> - bus-frequency : the clock frequency for QUICC Engine.
>> - fsl,qe-num-riscs: define how many RISC engines the QE has.
>> -- fsl,qe-num-snums: define how many serial number(SNUM) the QE can use
>> for the
>> +- fsl,qe-snums: This property has to be specified as '/bits/ 8' value,
>> + defining the array of serial number (SNUM) values for the virtual
>> threads.
>>
>> Optional properties:
>> @@ -34,6 +35,11 @@ Recommended properties
>> - brg-frequency : the internal clock source frequency for baud-rate
>> generators in Hz.
>>
>> +Deprecated properties
>> +- fsl,qe-num-snums: define how many serial number(SNUM) the QE can use
>> + for the threads. Use fsl,qe-snums instead to not only specify the
>> + number of snums, but also their values.
>> +
>> Example:
>> qe@e0100000 {
>> #address-cells = <1>;
>> diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c index
>> 71beeb72eee4..049b36d6aeee 100644
>> --- a/drivers/soc/fsl/qe/qe.c
>> +++ b/drivers/soc/fsl/qe/qe.c
>> @@ -283,7 +283,6 @@ EXPORT_SYMBOL(qe_clock_source);
>> */
>> static void qe_snums_init(void)
>> {
>> - int i;
>> static const u8 snum_init_76[] = {
>> 0x04, 0x05, 0x0C, 0x0D, 0x14, 0x15, 0x1C, 0x1D,
>> 0x24, 0x25, 0x2C, 0x2D, 0x34, 0x35, 0x88, 0x89, @@ -304,9
>> +303,22 @@ static void qe_snums_init(void)
>> 0x28, 0x29, 0x38, 0x39, 0x48, 0x49, 0x58, 0x59,
>> 0x68, 0x69, 0x78, 0x79, 0x80, 0x81,
>> };
>> + struct device_node *qe;
>> const u8 *snum_init;
>> + int i;
>>
>> bitmap_zero(snum_state, QE_NUM_OF_SNUM);
>> + qe = qe_get_device_node();
>> + if (qe) {
>> + i = of_property_read_variable_u8_array(qe, "fsl,qe-snums",
>> + snums, 14, QE_NUM_OF_SNUM);
>> + of_node_put(qe);
>> + if (i > 0) {
>> + qe_num_of_snum = i;
>> + return;
>> + }
>> + }
>> +
>> qe_num_of_snum = qe_get_num_of_snums();
>>
>> if (qe_num_of_snum == 76)
>
> So you define 14 snums for MPC8309, but there still be the comment "/* No QE ever has fewer than 28 SNUMs */" and it will check if
> The num_of_snums "is >28", it will cause confusion, so I suggest to modify 28 to 14.
Sure, that needs updating. My thinking was that only legacy DTs would
use the fsl,qe-num-snums, and there would be no need to support lower
values than we used to, since the logic back in qe_snums_init wouldn't
handle such values appropriately anyway.
> I read the old version QUICC Engine Block Reference Manual, it said snums table is not available on MPC8306/MPC8306S/MPC8309,
> So I think the code it written long before with this version RM, and at that time, the snums is at least 28, and nobody modify the code later.
> And now with the new version RM, it support MPC8306/MPC8306S/MPC8309 with snums and have snums fewer then 28, so I think the minimum value should
> Be modified to 14.
Yes. I'll do an extra cleanup patch modifying the code comments
appropriately. But what do you think about the core idea behind this
change (and the preceding cleanup patches)?
Rasmus
On 01/03/2019 15.50，Rasmus Villemoes wrote:
> -----Original Message-----
> From: Rasmus Villemoes <[email protected]>
> Sent: 2019年3月1日 15:50
> To: Qiang Zhao <[email protected]>; Leo Li <[email protected]>
> Cc: Scott Wood <[email protected]>; [email protected]; Timur Tabi
> <[email protected]>; Rasmus Villemoes <[email protected]>
> Subject: Re: [PATCH 4/4] soc/fsl/qe: qe.c: support fsl,qe-snums property
>
> On 01/03/2019 04.36, Qiang Zhao wrote:
> > On 2019年2月28日 18:31，Rasmus Villemoes wrote:
> >
> >> -----Original Message-----
> >> From: Rasmus Villemoes <[email protected]>
> >> Sent: 2019年2月28日 18:31
> >> To: Qiang Zhao <[email protected]>; Leo Li <[email protected]>
> >> Cc: Scott Wood <[email protected]>; [email protected];
> >> Timur Tabi <[email protected]>; Rasmus Villemoes
> >> <[email protected]>
> >> Subject: [PATCH 4/4] soc/fsl/qe: qe.c: support fsl,qe-snums property
> >>
> >
> > So you define 14 snums for MPC8309, but there still be the comment "/*
> > No QE ever has fewer than 28 SNUMs */" and it will check if The
> num_of_snums "is >28", it will cause confusion, so I suggest to modify 28 to
> 14.
>
> Sure, that needs updating. My thinking was that only legacy DTs would use the
> fsl,qe-num-snums, and there would be no need to support lower values than
> we used to, since the logic back in qe_snums_init wouldn't handle such values
> appropriately anyway.
>
> > I read the old version QUICC Engine Block Reference Manual, it said
> > snums table is not available on MPC8306/MPC8306S/MPC8309, So I think
> the code it written long before with this version RM, and at that time, the
> snums is at least 28, and nobody modify the code later.
> > And now with the new version RM, it support
> MPC8306/MPC8306S/MPC8309
> > with snums and have snums fewer then 28, so I think the minimum value
> should Be modified to 14.
>
> Yes. I'll do an extra cleanup patch modifying the code comments appropriately.
> But what do you think about the core idea behind this change (and the
> preceding cleanup patches)?
Maybe we could modify the comments in this patch, Anyway, the MPC8306/MPC8306S/MPC8309
Is supported with snums and the number is 14, In addition, you assign qe_num_of_snum to i as below.
The variable stands for num of snums.
Or we could add comments to explain it clearly why qe_num_of_snum is assign to a value fewer than 28
While it says " No QE ever has fewer than 28 SNUMs ".
+ qe = qe_get_device_node();
+ if (qe) {
+ i = of_property_read_variable_u8_array(qe, "fsl,qe-snums",
+ snums, 14, QE_NUM_OF_SNUM);
+ of_node_put(qe);
+ if (i > 0) {
+ qe_num_of_snum = i;
+ return;
+ }
+ }
Best Regards
Qiang Zhao
On 01/03/2019 10.43, Qiang Zhao wrote:
> On 01/03/2019 15.50，Rasmus Villemoes wrote:
>> -----Original Message-----
>> From: Rasmus Villemoes <[email protected]>
>> Sent: 2019年3月1日 15:50
>> To: Qiang Zhao <[email protected]>; Leo Li <[email protected]>
>> Cc: Scott Wood <[email protected]>; [email protected]; Timur Tabi
>> <[email protected]>; Rasmus Villemoes <[email protected]>
>> Subject: Re: [PATCH 4/4] soc/fsl/qe: qe.c: support fsl,qe-snums property
>>
>> On 01/03/2019 04.36, Qiang Zhao wrote:
>>> On 2019年2月28日 18:31，Rasmus Villemoes wrote:
>>>
>>>> -----Original Message-----
>>>> From: Rasmus Villemoes <[email protected]>
>>>> Sent: 2019年2月28日 18:31
>>>> To: Qiang Zhao <[email protected]>; Leo Li <[email protected]>
>>>> Cc: Scott Wood <[email protected]>; [email protected];
>>>> Timur Tabi <[email protected]>; Rasmus Villemoes
>>>> <[email protected]>
>>>> Subject: [PATCH 4/4] soc/fsl/qe: qe.c: support fsl,qe-snums property
>>>>
>>>
>>> So you define 14 snums for MPC8309, but there still be the comment "/*
>>> No QE ever has fewer than 28 SNUMs */" and it will check if The
>> num_of_snums "is >28", it will cause confusion, so I suggest to modify 28 to
>> 14.
>>
>> Sure, that needs updating. My thinking was that only legacy DTs would use the
>> fsl,qe-num-snums, and there would be no need to support lower values than
>> we used to, since the logic back in qe_snums_init wouldn't handle such values
>> appropriately anyway.
>>
>>> I read the old version QUICC Engine Block Reference Manual, it said
>>> snums table is not available on MPC8306/MPC8306S/MPC8309, So I think
>> the code it written long before with this version RM, and at that time, the
>> snums is at least 28, and nobody modify the code later.
>>> And now with the new version RM, it support
>> MPC8306/MPC8306S/MPC8309
>>> with snums and have snums fewer then 28, so I think the minimum value
>> should Be modified to 14.
>>
>> Yes. I'll do an extra cleanup patch modifying the code comments appropriately.
>> But what do you think about the core idea behind this change (and the
>> preceding cleanup patches)?
>
> Maybe we could modify the comments in this patch, Anyway, the MPC8306/MPC8306S/MPC8309
> Is supported with snums and the number is 14, In addition, you assign qe_num_of_snum to i as below.
> The variable stands for num of snums.
Yes, but I can't assign it directly, because qe_num_of_snum is unsigned,
and I need to test whether the return value is negative.
> Or we could add comments to explain it clearly why qe_num_of_snum is assign to a value fewer than 28
> While it says " No QE ever has fewer than 28 SNUMs ".
OK, I'll fold in some comment updating in a v2 patch.
>
> + qe = qe_get_device_node();
> + if (qe) {
> + i = of_property_read_variable_u8_array(qe, "fsl,qe-snums",
> + snums, 14, QE_NUM_OF_SNUM);
> + of_node_put(qe);
> + if (i > 0) {
> + qe_num_of_snum = i;
> + return;
> + }
> + }
Actually, putting in a lower bound of 14 here is a mistake - it should
just be 1, and then the code would also work for any QE variant that
might be shipped in the future. It's up to the DT author to make sure
the data is correct.
Can you figure out why the old code defaults to 28, and why its ok for
all those variants to just end up using the first 28 elements of the _46
array? What's the purpose of those surplus 18 elements?
Rasmus
> Best Regards
> Qiang Zhao
>
--
Rasmus Villemoes
Software Developer
Prevas A/S
Hedeager 3
DK-8200 Aarhus N
+45 51210274
[email protected]
http://www.prevas.dk
The comment "No QE ever has fewer than 28 SNUMs" is false; e.g. the
MPC8309 has 14. The code path returning -EINVAL is also a recipe for
instant disaster, since the caller (qe_snums_init) uncritically
assigns the return value to the unsigned qe_num_of_snum, and would
thus proceed to attempt to copy 4GB from snum_init_46[] to the snum[]
array.
So fold the handling of the legacy fsl,qe-num-snums into
qe_snums_init, and make sure we do not end up using the snum_init_46
array in cases other than the two where we know it makes sense.
Signed-off-by: Rasmus Villemoes <[email protected]>
---
Qiang: Something like this. I prefer not to fold it into patch 4/4
since it really fixes unrelated issues (the wrong comment and the
bogus handling of unexpected return values from qe_get_num_of_snums),
but I can do so if you wish.
drivers/net/ethernet/freescale/ucc_geth.c | 2 +-
drivers/soc/fsl/qe/qe.c | 54 +++++++----------------
include/soc/fsl/qe/qe.h | 2 +-
3 files changed, 19 insertions(+), 39 deletions(-)
diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
index 22a817da861e..3b2e28614e7b 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.c
+++ b/drivers/net/ethernet/freescale/ucc_geth.c
@@ -3842,7 +3842,7 @@ static int ucc_geth_probe(struct platform_device* ofdev)
}
if (max_speed == SPEED_1000) {
- unsigned int snums = qe_get_num_of_snums();
+ unsigned int snums = qe_num_of_snum;
/* configure muram FIFOs for gigabit operation */
ug_info->uf_info.urfs = UCC_GETH_URFS_GIGA_INIT;
diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c
index 8b915129077a..f8ddbb5b0eb5 100644
--- a/drivers/soc/fsl/qe/qe.c
+++ b/drivers/soc/fsl/qe/qe.c
@@ -52,7 +52,8 @@ EXPORT_SYMBOL(qe_immr);
static u8 snums[QE_NUM_OF_SNUM]; /* Dynamically allocated SNUMs */
static DECLARE_BITMAP(snum_state, QE_NUM_OF_SNUM);
-static unsigned int qe_num_of_snum;
+unsigned int qe_num_of_snum;
+EXPORT_SYMBOL(qe_num_of_snum);
static phys_addr_t qebase = -1;
@@ -308,26 +309,34 @@ static void qe_snums_init(void)
int i;
bitmap_zero(snum_state, QE_NUM_OF_SNUM);
+ qe_num_of_snum = 28; /* The default number of snum for threads is 28 */
qe = qe_get_device_node();
if (qe) {
i = of_property_read_variable_u8_array(qe, "fsl,qe-snums",
snums, 1, QE_NUM_OF_SNUM);
- of_node_put(qe);
if (i > 0) {
+ of_node_put(qe);
qe_num_of_snum = i;
return;
}
+ /*
+ * Fall back to legacy binding of using the value of
+ * fsl,qe-num-snums to choose one of the static arrays
+ * above.
+ */
+ of_property_read_u32(qe, "fsl,qe-num-snums", &qe_num_of_snum);
+ of_node_put(qe);
}
- qe_num_of_snum = qe_get_num_of_snums();
-
if (qe_num_of_snum == 76)
snum_init = snum_init_76;
- else
+ else if (qe_num_of_snum == 28 || qe_num_of_snum == 46)
snum_init = snum_init_46;
-
- for (i = 0; i < qe_num_of_snum; i++)
- snums[i] = snum_init[i];
+ else {
+ pr_err("QE: unsupported value of fsl,qe-num-snums: %u\n", qe_num_of_snum);
+ return;
+ }
+ memcpy(snums, snum_init, qe_num_of_snum);
}
int qe_get_snum(void)
@@ -649,35 +658,6 @@ unsigned int qe_get_num_of_risc(void)
}
EXPORT_SYMBOL(qe_get_num_of_risc);
-unsigned int qe_get_num_of_snums(void)
-{
- struct device_node *qe;
- int size;
- unsigned int num_of_snums;
- const u32 *prop;
-
- num_of_snums = 28; /* The default number of snum for threads is 28 */
- qe = qe_get_device_node();
- if (!qe)
- return num_of_snums;
-
- prop = of_get_property(qe, "fsl,qe-num-snums", &size);
- if (prop && size == sizeof(*prop)) {
- num_of_snums = *prop;
- if ((num_of_snums < 28) || (num_of_snums > QE_NUM_OF_SNUM)) {
- /* No QE ever has fewer than 28 SNUMs */
- pr_err("QE: number of snum is invalid\n");
- of_node_put(qe);
- return -EINVAL;
- }
- }
-
- of_node_put(qe);
-
- return num_of_snums;
-}
-EXPORT_SYMBOL(qe_get_num_of_snums);
-
static int __init qe_init(void)
{
struct device_node *np;
diff --git a/include/soc/fsl/qe/qe.h b/include/soc/fsl/qe/qe.h
index b3d1aff5e8ad..af5739850bf4 100644
--- a/include/soc/fsl/qe/qe.h
+++ b/include/soc/fsl/qe/qe.h
@@ -212,7 +212,7 @@ int qe_setbrg(enum qe_clock brg, unsigned int rate, unsigned int multiplier);
int qe_get_snum(void);
void qe_put_snum(u8 snum);
unsigned int qe_get_num_of_risc(void);
-unsigned int qe_get_num_of_snums(void);
+extern unsigned int qe_num_of_snum;
static inline int qe_alive_during_sleep(void)
{
--
2.20.1