* [PATCH RESEND 0/5] soc/fsl/qe: cleanups and new DT binding @ 2019-04-30 13:36 Rasmus Villemoes 2019-04-30 13:36 ` [PATCH 1/5] soc/fsl/qe: qe.c: drop useless static qualifier Rasmus Villemoes ` (5 more replies) 0 siblings, 6 replies; 40+ messages in thread From: Rasmus Villemoes @ 2019-04-30 13:36 UTC (permalink / raw) To: Qiang Zhao, Li Yang Cc: linuxppc-dev, linux-arm-kernel, linux-kernel, Rob Herring, Valentin Longchamp, Scott Wood, Rasmus Villemoes This small series consists of some small cleanups and simplifications of the QUICC engine driver, and introduces a new DT binding that makes it much easier to support other variants of the QUICC engine IP block that appears in the wild: There's no reason to expect in general that the number of valid SNUMs uniquely determines the set of such, so it's better to simply let the device tree specify the values (and, implicitly via the array length, also the count). I sent these two months ago, but mostly as POC inside another thread. Resending as proper patch series. Rasmus Villemoes (5): 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 soc/fsl/qe: qe.c: fold qe_get_num_of_snums into qe_snums_init .../devicetree/bindings/soc/fsl/cpm_qe/qe.txt | 8 +- drivers/net/ethernet/freescale/ucc_geth.c | 2 +- drivers/soc/fsl/qe/qe.c | 162 +++++++----------- include/soc/fsl/qe/qe.h | 2 +- 4 files changed, 73 insertions(+), 101 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH 1/5] soc/fsl/qe: qe.c: drop useless static qualifier 2019-04-30 13:36 [PATCH RESEND 0/5] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes @ 2019-04-30 13:36 ` Rasmus Villemoes 2019-04-30 17:03 ` Christophe Leroy 2019-04-30 13:36 ` [PATCH 2/5] soc/fsl/qe: qe.c: reduce static memory footprint by 1.7K Rasmus Villemoes ` (4 subsequent siblings) 5 siblings, 1 reply; 40+ messages in thread From: Rasmus Villemoes @ 2019-04-30 13:36 UTC (permalink / raw) To: Qiang Zhao, Li Yang Cc: linuxppc-dev, linux-arm-kernel, linux-kernel, Rob Herring, Valentin Longchamp, Scott Wood, Rasmus Villemoes The local variable snum_init has no reason to have static storage duration. Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- 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 612d9c551be5..855373deb746 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 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH 1/5] soc/fsl/qe: qe.c: drop useless static qualifier 2019-04-30 13:36 ` [PATCH 1/5] soc/fsl/qe: qe.c: drop useless static qualifier Rasmus Villemoes @ 2019-04-30 17:03 ` Christophe Leroy 0 siblings, 0 replies; 40+ messages in thread From: Christophe Leroy @ 2019-04-30 17:03 UTC (permalink / raw) To: Rasmus Villemoes, Qiang Zhao, Li Yang Cc: Valentin Longchamp, linux-kernel, Scott Wood, Rasmus Villemoes, Rob Herring, linuxppc-dev, linux-arm-kernel Le 30/04/2019 à 15:36, Rasmus Villemoes a écrit : > The local variable snum_init has no reason to have static storage duration. > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr> > --- > 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 612d9c551be5..855373deb746 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(); > > ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH 2/5] soc/fsl/qe: qe.c: reduce static memory footprint by 1.7K 2019-04-30 13:36 [PATCH RESEND 0/5] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes 2019-04-30 13:36 ` [PATCH 1/5] soc/fsl/qe: qe.c: drop useless static qualifier Rasmus Villemoes @ 2019-04-30 13:36 ` Rasmus Villemoes 2019-04-30 17:12 ` Christophe Leroy 2019-04-30 13:36 ` [PATCH 3/5] soc/fsl/qe: qe.c: introduce qe_get_device_node helper Rasmus Villemoes ` (3 subsequent siblings) 5 siblings, 1 reply; 40+ messages in thread From: Rasmus Villemoes @ 2019-04-30 13:36 UTC (permalink / raw) To: Qiang Zhao, Li Yang Cc: linuxppc-dev, linux-arm-kernel, linux-kernel, Rob Herring, Valentin Longchamp, Scott Wood, Rasmus Villemoes 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 <rasmus.villemoes@prevas.dk> --- 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 855373deb746..d0393f83145c 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 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH 2/5] soc/fsl/qe: qe.c: reduce static memory footprint by 1.7K 2019-04-30 13:36 ` [PATCH 2/5] soc/fsl/qe: qe.c: reduce static memory footprint by 1.7K Rasmus Villemoes @ 2019-04-30 17:12 ` Christophe Leroy 2019-05-01 5:51 ` Rasmus Villemoes 0 siblings, 1 reply; 40+ messages in thread From: Christophe Leroy @ 2019-04-30 17:12 UTC (permalink / raw) To: Rasmus Villemoes, Qiang Zhao, Li Yang Cc: Valentin Longchamp, linux-kernel, Scott Wood, Rasmus Villemoes, Rob Herring, linuxppc-dev, linux-arm-kernel Le 30/04/2019 à 15:36, Rasmus Villemoes a écrit : > 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 <rasmus.villemoes@prevas.dk> > --- > 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 855373deb746..d0393f83145c 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); Doesn't make much importance, but wouldn't it be more logical to add this line where the setting of .state = QE_SNUM_STATE_FREE was done previously, ie around the for() loop below ? > 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]; Could use memcpy() instead ? > } > > 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; > } > } Can we replace this loop by memchr() ? Christophe ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/5] soc/fsl/qe: qe.c: reduce static memory footprint by 1.7K 2019-04-30 17:12 ` Christophe Leroy @ 2019-05-01 5:51 ` Rasmus Villemoes 0 siblings, 0 replies; 40+ messages in thread From: Rasmus Villemoes @ 2019-05-01 5:51 UTC (permalink / raw) To: Christophe Leroy, Qiang Zhao, Li Yang Cc: linux-kernel, Scott Wood, Rasmus Villemoes, Rob Herring, linuxppc-dev, linux-arm-kernel On 30/04/2019 19.12, Christophe Leroy wrote: > > Le 30/04/2019 à 15:36, Rasmus Villemoes a écrit : >> 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 <rasmus.villemoes@prevas.dk> >> --- >> - >> /* 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); > > Doesn't make much importance, but wouldn't it be more logical to add > this line where the setting of .state = QE_SNUM_STATE_FREE was done > previously, ie around the for() loop below ? This was on purpose, to avoid having to move it up in patch 4, where we don't necessarily reach the for loop. >> 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]; > > Could use memcpy() instead ? Yes, I switch to that in 5/5. Sure, I could do it here already, but I did it this way to keep close to the current style. I don't care either way, so if you prefer introducing memcpy here, fine by me. >> 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; >> } >> } > > Can we replace this loop by memchr() ? Hm, yes. So that would be const u8 *p = memchr(snums, snum, qe_num_of_snum) if (p) clear_bit(p - snums, snum_state); I guess. Let me fold that in and see how it looks. Thanks, Rasmus ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH 3/5] soc/fsl/qe: qe.c: introduce qe_get_device_node helper 2019-04-30 13:36 [PATCH RESEND 0/5] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes 2019-04-30 13:36 ` [PATCH 1/5] soc/fsl/qe: qe.c: drop useless static qualifier Rasmus Villemoes 2019-04-30 13:36 ` [PATCH 2/5] soc/fsl/qe: qe.c: reduce static memory footprint by 1.7K Rasmus Villemoes @ 2019-04-30 13:36 ` Rasmus Villemoes 2019-04-30 17:14 ` Christophe Leroy 2019-04-30 13:36 ` [PATCH 4/5] soc/fsl/qe: qe.c: support fsl,qe-snums property Rasmus Villemoes ` (2 subsequent siblings) 5 siblings, 1 reply; 40+ messages in thread From: Rasmus Villemoes @ 2019-04-30 13:36 UTC (permalink / raw) To: Qiang Zhao, Li Yang Cc: linuxppc-dev, linux-arm-kernel, linux-kernel, Rob Herring, Valentin Longchamp, Scott Wood, Rasmus Villemoes 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 <rasmus.villemoes@prevas.dk> --- 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 d0393f83145c..aff9d1373529 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 */ fw = of_get_child_by_name(qe, "firmware"); @@ -618,16 +619,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)) @@ -647,16 +641,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 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH 3/5] soc/fsl/qe: qe.c: introduce qe_get_device_node helper 2019-04-30 13:36 ` [PATCH 3/5] soc/fsl/qe: qe.c: introduce qe_get_device_node helper Rasmus Villemoes @ 2019-04-30 17:14 ` Christophe Leroy 0 siblings, 0 replies; 40+ messages in thread From: Christophe Leroy @ 2019-04-30 17:14 UTC (permalink / raw) To: Rasmus Villemoes, Qiang Zhao, Li Yang Cc: Valentin Longchamp, linux-kernel, Scott Wood, Rasmus Villemoes, Rob Herring, linuxppc-dev, linux-arm-kernel Le 30/04/2019 à 15:36, Rasmus Villemoes a écrit : > 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 <rasmus.villemoes@prevas.dk> Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr> > --- > 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 d0393f83145c..aff9d1373529 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 */ > fw = of_get_child_by_name(qe, "firmware"); > @@ -618,16 +619,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)) > @@ -647,16 +641,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)) { > ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH 4/5] soc/fsl/qe: qe.c: support fsl,qe-snums property 2019-04-30 13:36 [PATCH RESEND 0/5] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes ` (2 preceding siblings ...) 2019-04-30 13:36 ` [PATCH 3/5] soc/fsl/qe: qe.c: introduce qe_get_device_node helper Rasmus Villemoes @ 2019-04-30 13:36 ` Rasmus Villemoes 2019-04-30 17:19 ` Christophe Leroy 2019-04-30 13:36 ` [PATCH 5/5] soc/fsl/qe: qe.c: fold qe_get_num_of_snums into qe_snums_init Rasmus Villemoes 2019-05-01 9:29 ` [PATCH v2 0/6] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes 5 siblings, 1 reply; 40+ messages in thread From: Rasmus Villemoes @ 2019-04-30 13:36 UTC (permalink / raw) To: Qiang Zhao, Li Yang Cc: linuxppc-dev, linux-arm-kernel, linux-kernel, Rob Herring, Valentin Longchamp, Scott Wood, Rasmus Villemoes 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 <rasmus.villemoes@prevas.dk> --- .../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 aff9d1373529..af3c2b2b268f 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, 1, 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 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH 4/5] soc/fsl/qe: qe.c: support fsl,qe-snums property 2019-04-30 13:36 ` [PATCH 4/5] soc/fsl/qe: qe.c: support fsl,qe-snums property Rasmus Villemoes @ 2019-04-30 17:19 ` Christophe Leroy 2019-05-01 6:00 ` Rasmus Villemoes 0 siblings, 1 reply; 40+ messages in thread From: Christophe Leroy @ 2019-04-30 17:19 UTC (permalink / raw) To: Rasmus Villemoes, Qiang Zhao, Li Yang Cc: Valentin Longchamp, linux-kernel, Scott Wood, Rasmus Villemoes, Rob Herring, linuxppc-dev, linux-arm-kernel Le 30/04/2019 à 15:36, Rasmus Villemoes a écrit : > 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 <rasmus.villemoes@prevas.dk> > --- > .../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 aff9d1373529..af3c2b2b268f 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; Why do you move this one ? > 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, 1, QE_NUM_OF_SNUM); > + of_node_put(qe); > + if (i > 0) { > + qe_num_of_snum = i; > + return; In that case you skip the rest of the init ? Can you explain ? Christophe > + } > + } > + > qe_num_of_snum = qe_get_num_of_snums(); > > if (qe_num_of_snum == 76) > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 4/5] soc/fsl/qe: qe.c: support fsl,qe-snums property 2019-04-30 17:19 ` Christophe Leroy @ 2019-05-01 6:00 ` Rasmus Villemoes 0 siblings, 0 replies; 40+ messages in thread From: Rasmus Villemoes @ 2019-05-01 6:00 UTC (permalink / raw) To: Christophe Leroy, Qiang Zhao, Li Yang Cc: linux-kernel, Scott Wood, Rasmus Villemoes, Rob Herring, linuxppc-dev, linux-arm-kernel On 30/04/2019 19.19, Christophe Leroy wrote: > > > Le 30/04/2019 à 15:36, Rasmus Villemoes a écrit : >> 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 <rasmus.villemoes@prevas.dk> >> --- >> .../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 aff9d1373529..af3c2b2b268f 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; > > Why do you move this one ? To keep the declarations of the auto variables together. When reading the code and needing to know the type of i, it's much harder to find its declaration if one has to skip back over the two tables, and it's unnatural to have it separate from the others. >> 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, 1, QE_NUM_OF_SNUM); >> + of_node_put(qe); >> + if (i > 0) { >> + qe_num_of_snum = i; >> + return; > > In that case you skip the rest of the init ? Can you explain ? If of_property_read_variable_u8_array is succesful, it has already stored the values into the snums array, so there's no copying left to do, and the return value is the length of the array (which we save for later in qe_num_of_snum). So there's really nothing more to do. This was what I tried to hint at with "Conveniently, of_property_read_variable_u8_array does exactly what we need.", but I can see that that might need elaborating a little. Rasmus ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH 5/5] soc/fsl/qe: qe.c: fold qe_get_num_of_snums into qe_snums_init 2019-04-30 13:36 [PATCH RESEND 0/5] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes ` (3 preceding siblings ...) 2019-04-30 13:36 ` [PATCH 4/5] soc/fsl/qe: qe.c: support fsl,qe-snums property Rasmus Villemoes @ 2019-04-30 13:36 ` Rasmus Villemoes 2019-04-30 17:27 ` Christophe Leroy 2019-05-01 9:29 ` [PATCH v2 0/6] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes 5 siblings, 1 reply; 40+ messages in thread From: Rasmus Villemoes @ 2019-04-30 13:36 UTC (permalink / raw) To: Qiang Zhao, Li Yang Cc: linuxppc-dev, linux-arm-kernel, linux-kernel, Rob Herring, Valentin Longchamp, Scott Wood, Rasmus Villemoes 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 <rasmus.villemoes@prevas.dk> --- 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 eb3e65e8868f..5748eb8464d0 100644 --- a/drivers/net/ethernet/freescale/ucc_geth.c +++ b/drivers/net/ethernet/freescale/ucc_geth.c @@ -3837,7 +3837,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 af3c2b2b268f..8c3b3c62d81b 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) @@ -645,35 +654,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 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH 5/5] soc/fsl/qe: qe.c: fold qe_get_num_of_snums into qe_snums_init 2019-04-30 13:36 ` [PATCH 5/5] soc/fsl/qe: qe.c: fold qe_get_num_of_snums into qe_snums_init Rasmus Villemoes @ 2019-04-30 17:27 ` Christophe Leroy 0 siblings, 0 replies; 40+ messages in thread From: Christophe Leroy @ 2019-04-30 17:27 UTC (permalink / raw) To: Rasmus Villemoes, Qiang Zhao, Li Yang Cc: Valentin Longchamp, linux-kernel, Scott Wood, Rasmus Villemoes, Rob Herring, linuxppc-dev, linux-arm-kernel Le 30/04/2019 à 15:36, Rasmus Villemoes a écrit : > 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 <rasmus.villemoes@prevas.dk> > --- > 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 eb3e65e8868f..5748eb8464d0 100644 > --- a/drivers/net/ethernet/freescale/ucc_geth.c > +++ b/drivers/net/ethernet/freescale/ucc_geth.c > @@ -3837,7 +3837,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 af3c2b2b268f..8c3b3c62d81b 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); By exporting the object you allow other drivers to modify it. Is that really what we want ? Why not keep qe_get_num_of_snums() as a helper that simply returns 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; > + } The first leg of the if/else must have {} too when the second leg has them. > + memcpy(snums, snum_init, qe_num_of_snum); > } > > int qe_get_snum(void) > @@ -645,35 +654,6 @@ unsigned int qe_get_num_of_risc(void) > } > EXPORT_SYMBOL(qe_get_num_of_risc); > > -unsigned int qe_get_num_of_snums(void) I think this function should remain and just return num_of_snums, see my other comment above. Christophe > -{ > - 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) > { > ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 0/6] soc/fsl/qe: cleanups and new DT binding 2019-04-30 13:36 [PATCH RESEND 0/5] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes ` (4 preceding siblings ...) 2019-04-30 13:36 ` [PATCH 5/5] soc/fsl/qe: qe.c: fold qe_get_num_of_snums into qe_snums_init Rasmus Villemoes @ 2019-05-01 9:29 ` Rasmus Villemoes 2019-05-01 9:29 ` [PATCH v2 1/6] soc/fsl/qe: qe.c: drop useless static qualifier Rasmus Villemoes ` (6 more replies) 5 siblings, 7 replies; 40+ messages in thread From: Rasmus Villemoes @ 2019-05-01 9:29 UTC (permalink / raw) To: devicetree, Qiang Zhao, Li Yang Cc: linuxppc-dev, linux-arm-kernel, linux-kernel, Rob Herring, Scott Wood, Christophe Leroy, Mark Rutland, Rasmus Villemoes This small series consists of some small cleanups and simplifications of the QUICC engine driver, and introduces a new DT binding that makes it much easier to support other variants of the QUICC engine IP block that appears in the wild: There's no reason to expect in general that the number of valid SNUMs uniquely determines the set of such, so it's better to simply let the device tree specify the values (and, implicitly via the array length, also the count). v2: - Address comments from Christophe Leroy - Add his Reviewed-by to 1/6 and 3/6 - Split DT binding update to separate patch as per Documentation/devicetree/bindings/submitting-patches.txt Rasmus Villemoes (6): 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 dt-bindings: soc/fsl: qe: document new fsl,qe-snums binding soc/fsl/qe: qe.c: support fsl,qe-snums property soc/fsl/qe: qe.c: fold qe_get_num_of_snums into qe_snums_init .../devicetree/bindings/soc/fsl/cpm_qe/qe.txt | 8 +- drivers/soc/fsl/qe/qe.c | 164 +++++++----------- 2 files changed, 72 insertions(+), 100 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 1/6] soc/fsl/qe: qe.c: drop useless static qualifier 2019-05-01 9:29 ` [PATCH v2 0/6] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes @ 2019-05-01 9:29 ` Rasmus Villemoes 2019-05-01 9:29 ` [PATCH v2 2/6] soc/fsl/qe: qe.c: reduce static memory footprint by 1.7K Rasmus Villemoes ` (5 subsequent siblings) 6 siblings, 0 replies; 40+ messages in thread From: Rasmus Villemoes @ 2019-05-01 9:29 UTC (permalink / raw) To: devicetree, Qiang Zhao, Li Yang Cc: linuxppc-dev, linux-arm-kernel, linux-kernel, Rob Herring, Scott Wood, Christophe Leroy, Mark Rutland, Rasmus Villemoes The local variable snum_init has no reason to have static storage duration. Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- 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 612d9c551be5..855373deb746 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 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v2 2/6] soc/fsl/qe: qe.c: reduce static memory footprint by 1.7K 2019-05-01 9:29 ` [PATCH v2 0/6] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes 2019-05-01 9:29 ` [PATCH v2 1/6] soc/fsl/qe: qe.c: drop useless static qualifier Rasmus Villemoes @ 2019-05-01 9:29 ` Rasmus Villemoes 2019-05-02 4:51 ` Christophe Leroy 2019-05-01 9:29 ` [PATCH v2 3/6] soc/fsl/qe: qe.c: introduce qe_get_device_node helper Rasmus Villemoes ` (4 subsequent siblings) 6 siblings, 1 reply; 40+ messages in thread From: Rasmus Villemoes @ 2019-05-01 9:29 UTC (permalink / raw) To: devicetree, Qiang Zhao, Li Yang Cc: linuxppc-dev, linux-arm-kernel, linux-kernel, Rob Herring, Scott Wood, Christophe Leroy, Mark Rutland, Rasmus Villemoes 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 <rasmus.villemoes@prevas.dk> --- drivers/soc/fsl/qe/qe.c | 43 ++++++++++++----------------------------- 1 file changed, 12 insertions(+), 31 deletions(-) diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c index 855373deb746..303aa29cb27d 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; @@ -315,10 +305,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; - } + bitmap_zero(snum_state, QE_NUM_OF_SNUM); + memcpy(snums, snum_init, qe_num_of_snum); } int qe_get_snum(void) @@ -328,12 +316,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); @@ -343,14 +329,9 @@ EXPORT_SYMBOL(qe_get_snum); 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; - break; - } - } + const u8 *p = memchr(snums, snum, qe_num_of_snum); + if (p) + clear_bit(p - snums, snum_state); } EXPORT_SYMBOL(qe_put_snum); -- 2.20.1 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v2 2/6] soc/fsl/qe: qe.c: reduce static memory footprint by 1.7K 2019-05-01 9:29 ` [PATCH v2 2/6] soc/fsl/qe: qe.c: reduce static memory footprint by 1.7K Rasmus Villemoes @ 2019-05-02 4:51 ` Christophe Leroy 0 siblings, 0 replies; 40+ messages in thread From: Christophe Leroy @ 2019-05-02 4:51 UTC (permalink / raw) To: Rasmus Villemoes, devicetree, Qiang Zhao, Li Yang Cc: linuxppc-dev, linux-arm-kernel, linux-kernel, Rob Herring, Scott Wood, Mark Rutland, Rasmus Villemoes Le 01/05/2019 à 11:29, Rasmus Villemoes a écrit : > 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 <rasmus.villemoes@prevas.dk> Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr> Trivial comment below > --- > drivers/soc/fsl/qe/qe.c | 43 ++++++++++++----------------------------- > 1 file changed, 12 insertions(+), 31 deletions(-) > > diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c > index 855373deb746..303aa29cb27d 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; > @@ -315,10 +305,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; > - } > + bitmap_zero(snum_state, QE_NUM_OF_SNUM); > + memcpy(snums, snum_init, qe_num_of_snum); > } > > int qe_get_snum(void) > @@ -328,12 +316,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); > > @@ -343,14 +329,9 @@ EXPORT_SYMBOL(qe_get_snum); > > 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; > - break; > - } > - } > + const u8 *p = memchr(snums, snum, qe_num_of_snum); A blank line is expected here. Christophe > + if (p) > + clear_bit(p - snums, snum_state); > } > EXPORT_SYMBOL(qe_put_snum); > > ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 3/6] soc/fsl/qe: qe.c: introduce qe_get_device_node helper 2019-05-01 9:29 ` [PATCH v2 0/6] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes 2019-05-01 9:29 ` [PATCH v2 1/6] soc/fsl/qe: qe.c: drop useless static qualifier Rasmus Villemoes 2019-05-01 9:29 ` [PATCH v2 2/6] soc/fsl/qe: qe.c: reduce static memory footprint by 1.7K Rasmus Villemoes @ 2019-05-01 9:29 ` Rasmus Villemoes 2019-05-01 9:29 ` [PATCH v2 4/6] dt-bindings: soc/fsl: qe: document new fsl,qe-snums binding Rasmus Villemoes ` (3 subsequent siblings) 6 siblings, 0 replies; 40+ messages in thread From: Rasmus Villemoes @ 2019-05-01 9:29 UTC (permalink / raw) To: devicetree, Qiang Zhao, Li Yang Cc: linuxppc-dev, linux-arm-kernel, linux-kernel, Rob Herring, Scott Wood, Christophe Leroy, Mark Rutland, Rasmus Villemoes 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. Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- 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 303aa29cb27d..0fb8b59f61ad 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)) @@ -557,16 +565,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 */ fw = of_get_child_by_name(qe, "firmware"); @@ -612,16 +613,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)) @@ -641,16 +635,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 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v2 4/6] dt-bindings: soc/fsl: qe: document new fsl,qe-snums binding 2019-05-01 9:29 ` [PATCH v2 0/6] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes ` (2 preceding siblings ...) 2019-05-01 9:29 ` [PATCH v2 3/6] soc/fsl/qe: qe.c: introduce qe_get_device_node helper Rasmus Villemoes @ 2019-05-01 9:29 ` Rasmus Villemoes 2019-05-01 15:12 ` Joakim Tjernlund 2019-05-01 21:00 ` Rob Herring 2019-05-01 9:29 ` [PATCH v2 5/6] soc/fsl/qe: qe.c: support fsl,qe-snums property Rasmus Villemoes ` (2 subsequent siblings) 6 siblings, 2 replies; 40+ messages in thread From: Rasmus Villemoes @ 2019-05-01 9:29 UTC (permalink / raw) To: devicetree, Qiang Zhao, Li Yang Cc: linuxppc-dev, linux-arm-kernel, linux-kernel, Rob Herring, Scott Wood, Christophe Leroy, Mark Rutland, Rasmus Villemoes Reading table 4-30, and its footnotes, of the QUICC Engine Block Reference Manual shows that the set of snum _values_ is not necessarily just a function of the _number_ of snums, as given in the fsl,qe-num-snums property. As an alternative, to make it easier to add support for other variants of the QUICC engine IP, this introduces a new binding fsl,qe-snums, which automatically encodes both the number of snums and the actual values to use. 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 <rasmus.villemoes@prevas.dk> --- Documentation/devicetree/bindings/soc/fsl/cpm_qe/qe.txt | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) 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>; -- 2.20.1 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v2 4/6] dt-bindings: soc/fsl: qe: document new fsl,qe-snums binding 2019-05-01 9:29 ` [PATCH v2 4/6] dt-bindings: soc/fsl: qe: document new fsl,qe-snums binding Rasmus Villemoes @ 2019-05-01 15:12 ` Joakim Tjernlund 2019-05-01 18:47 ` Rasmus Villemoes 2019-05-01 21:00 ` Rob Herring 1 sibling, 1 reply; 40+ messages in thread From: Joakim Tjernlund @ 2019-05-01 15:12 UTC (permalink / raw) To: rasmus.villemoes, leoyang.li, qiang.zhao, devicetree Cc: linuxppc-dev, mark.rutland, linux-kernel, oss, Rasmus.Villemoes, robh+dt, linux-arm-kernel On Wed, 2019-05-01 at 09:29 +0000, Rasmus Villemoes wrote: > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. > > > Reading table 4-30, and its footnotes, of the QUICC Engine Block > Reference Manual shows that the set of snum _values_ is not > necessarily just a function of the _number_ of snums, as given in the > fsl,qe-num-snums property. > > As an alternative, to make it easier to add support for other variants > of the QUICC engine IP, this introduces a new binding fsl,qe-snums, > which automatically encodes both the number of snums and the actual > values to use. > > 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>; I think you need add this example to the qe.txt doc itselft. BTW, what is /bits/ ? > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > --- > Documentation/devicetree/bindings/soc/fsl/cpm_qe/qe.txt | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > 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>; > -- > 2.20.1 > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 4/6] dt-bindings: soc/fsl: qe: document new fsl,qe-snums binding 2019-05-01 15:12 ` Joakim Tjernlund @ 2019-05-01 18:47 ` Rasmus Villemoes 0 siblings, 0 replies; 40+ messages in thread From: Rasmus Villemoes @ 2019-05-01 18:47 UTC (permalink / raw) To: Joakim Tjernlund, leoyang.li, qiang.zhao, devicetree Cc: linuxppc-dev, mark.rutland, linux-kernel, oss, Rasmus Villemoes, robh+dt, linux-arm-kernel On 01/05/2019 17.12, Joakim Tjernlund wrote: > On Wed, 2019-05-01 at 09:29 +0000, Rasmus Villemoes wrote: >> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. >> >> >> Reading table 4-30, and its footnotes, of the QUICC Engine Block >> Reference Manual shows that the set of snum _values_ is not >> necessarily just a function of the _number_ of snums, as given in the >> fsl,qe-num-snums property. >> >> As an alternative, to make it easier to add support for other variants >> of the QUICC engine IP, this introduces a new binding fsl,qe-snums, >> which automatically encodes both the number of snums and the actual >> values to use. >> >> 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>; > > I think you need add this example to the qe.txt doc itselft. Sure, can do. > BTW, what is /bits/ ? That indicates that the numbers should be stored as an array of u8, and not as by default an array of (big-endian) 32-bit numbers. See https://git.kernel.org/pub/scm/utils/dtc/dtc.git/tree/Documentation/dts-format.txt#n46 This is already used in some bindings and existing .dts (e.g. hwmon/aspeed-pwm-tacho.txt, but git grep shows many more). Rasmus ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 4/6] dt-bindings: soc/fsl: qe: document new fsl,qe-snums binding 2019-05-01 9:29 ` [PATCH v2 4/6] dt-bindings: soc/fsl: qe: document new fsl,qe-snums binding Rasmus Villemoes 2019-05-01 15:12 ` Joakim Tjernlund @ 2019-05-01 21:00 ` Rob Herring 1 sibling, 0 replies; 40+ messages in thread From: Rob Herring @ 2019-05-01 21:00 UTC (permalink / raw) To: Rasmus Villemoes Cc: devicetree, Qiang Zhao, Li Yang, linuxppc-dev, linux-arm-kernel, linux-kernel, Scott Wood, Christophe Leroy, Mark Rutland, Rasmus Villemoes On Wed, 1 May 2019 09:29:08 +0000, Rasmus Villemoes wrote: > Reading table 4-30, and its footnotes, of the QUICC Engine Block > Reference Manual shows that the set of snum _values_ is not > necessarily just a function of the _number_ of snums, as given in the > fsl,qe-num-snums property. > > As an alternative, to make it easier to add support for other variants > of the QUICC engine IP, this introduces a new binding fsl,qe-snums, > which automatically encodes both the number of snums and the actual > values to use. > > 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 <rasmus.villemoes@prevas.dk> > --- > Documentation/devicetree/bindings/soc/fsl/cpm_qe/qe.txt | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > Reviewed-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 5/6] soc/fsl/qe: qe.c: support fsl,qe-snums property 2019-05-01 9:29 ` [PATCH v2 0/6] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes ` (3 preceding siblings ...) 2019-05-01 9:29 ` [PATCH v2 4/6] dt-bindings: soc/fsl: qe: document new fsl,qe-snums binding Rasmus Villemoes @ 2019-05-01 9:29 ` Rasmus Villemoes 2019-05-02 4:52 ` Christophe Leroy 2019-05-01 9:29 ` [PATCH v2 6/6] soc/fsl/qe: qe.c: fold qe_get_num_of_snums into qe_snums_init Rasmus Villemoes 2019-05-13 11:14 ` [PATCH v3 0/6] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes 6 siblings, 1 reply; 40+ messages in thread From: Rasmus Villemoes @ 2019-05-01 9:29 UTC (permalink / raw) To: devicetree, Qiang Zhao, Li Yang Cc: linuxppc-dev, linux-arm-kernel, linux-kernel, Rob Herring, Scott Wood, Christophe Leroy, Mark Rutland, Rasmus Villemoes Add driver support for the newly introduced fsl,qe-snums property. Conveniently, of_property_read_variable_u8_array does exactly what we need: If the property fsl,qe-snums is found (and has an allowed size), the array of values get copied to snums, and the return value is the number of snums - we cannot assign directly to num_of_snums, since we need to check whether the return value is negative. Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- drivers/soc/fsl/qe/qe.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c index 0fb8b59f61ad..325d689cbf5c 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,7 +303,21 @@ 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, 1, 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(); @@ -313,7 +326,6 @@ static void qe_snums_init(void) else snum_init = snum_init_46; - bitmap_zero(snum_state, QE_NUM_OF_SNUM); memcpy(snums, snum_init, qe_num_of_snum); } -- 2.20.1 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v2 5/6] soc/fsl/qe: qe.c: support fsl,qe-snums property 2019-05-01 9:29 ` [PATCH v2 5/6] soc/fsl/qe: qe.c: support fsl,qe-snums property Rasmus Villemoes @ 2019-05-02 4:52 ` Christophe Leroy 0 siblings, 0 replies; 40+ messages in thread From: Christophe Leroy @ 2019-05-02 4:52 UTC (permalink / raw) To: Rasmus Villemoes, devicetree, Qiang Zhao, Li Yang Cc: linuxppc-dev, linux-arm-kernel, linux-kernel, Rob Herring, Scott Wood, Mark Rutland, Rasmus Villemoes Le 01/05/2019 à 11:29, Rasmus Villemoes a écrit : > Add driver support for the newly introduced fsl,qe-snums property. > > Conveniently, of_property_read_variable_u8_array does exactly what we > need: If the property fsl,qe-snums is found (and has an allowed size), > the array of values get copied to snums, and the return value is the > number of snums - we cannot assign directly to num_of_snums, since we > need to check whether the return value is negative. > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr> > --- > drivers/soc/fsl/qe/qe.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c > index 0fb8b59f61ad..325d689cbf5c 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,7 +303,21 @@ 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, 1, 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(); > > @@ -313,7 +326,6 @@ static void qe_snums_init(void) > else > snum_init = snum_init_46; > > - bitmap_zero(snum_state, QE_NUM_OF_SNUM); > memcpy(snums, snum_init, qe_num_of_snum); > } > > ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 6/6] soc/fsl/qe: qe.c: fold qe_get_num_of_snums into qe_snums_init 2019-05-01 9:29 ` [PATCH v2 0/6] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes ` (4 preceding siblings ...) 2019-05-01 9:29 ` [PATCH v2 5/6] soc/fsl/qe: qe.c: support fsl,qe-snums property Rasmus Villemoes @ 2019-05-01 9:29 ` Rasmus Villemoes 2019-05-02 4:54 ` Christophe Leroy 2019-05-09 2:35 ` [EXT] " Qiang Zhao 2019-05-13 11:14 ` [PATCH v3 0/6] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes 6 siblings, 2 replies; 40+ messages in thread From: Rasmus Villemoes @ 2019-05-01 9:29 UTC (permalink / raw) To: devicetree, Qiang Zhao, Li Yang Cc: linuxppc-dev, linux-arm-kernel, linux-kernel, Rob Herring, Scott Wood, Christophe Leroy, Mark Rutland, Rasmus Villemoes 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 <rasmus.villemoes@prevas.dk> --- drivers/soc/fsl/qe/qe.c | 46 ++++++++++++++--------------------------- 1 file changed, 16 insertions(+), 30 deletions(-) diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c index 325d689cbf5c..276d7d78ebfc 100644 --- a/drivers/soc/fsl/qe/qe.c +++ b/drivers/soc/fsl/qe/qe.c @@ -308,24 +308,33 @@ 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) + 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; - + } 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); } @@ -641,30 +650,7 @@ 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; + return qe_num_of_snum; } EXPORT_SYMBOL(qe_get_num_of_snums); -- 2.20.1 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v2 6/6] soc/fsl/qe: qe.c: fold qe_get_num_of_snums into qe_snums_init 2019-05-01 9:29 ` [PATCH v2 6/6] soc/fsl/qe: qe.c: fold qe_get_num_of_snums into qe_snums_init Rasmus Villemoes @ 2019-05-02 4:54 ` Christophe Leroy 2019-05-09 2:35 ` [EXT] " Qiang Zhao 1 sibling, 0 replies; 40+ messages in thread From: Christophe Leroy @ 2019-05-02 4:54 UTC (permalink / raw) To: Rasmus Villemoes, devicetree, Qiang Zhao, Li Yang Cc: linuxppc-dev, linux-arm-kernel, linux-kernel, Rob Herring, Scott Wood, Mark Rutland, Rasmus Villemoes Le 01/05/2019 à 11:29, Rasmus Villemoes a écrit : > 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 <rasmus.villemoes@prevas.dk> Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr> > --- > drivers/soc/fsl/qe/qe.c | 46 ++++++++++++++--------------------------- > 1 file changed, 16 insertions(+), 30 deletions(-) > > diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c > index 325d689cbf5c..276d7d78ebfc 100644 > --- a/drivers/soc/fsl/qe/qe.c > +++ b/drivers/soc/fsl/qe/qe.c > @@ -308,24 +308,33 @@ 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) > + 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; > - > + } 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); > } > > @@ -641,30 +650,7 @@ 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; > + return qe_num_of_snum; > } > EXPORT_SYMBOL(qe_get_num_of_snums); > > ^ permalink raw reply [flat|nested] 40+ messages in thread
* RE: [EXT] [PATCH v2 6/6] soc/fsl/qe: qe.c: fold qe_get_num_of_snums into qe_snums_init 2019-05-01 9:29 ` [PATCH v2 6/6] soc/fsl/qe: qe.c: fold qe_get_num_of_snums into qe_snums_init Rasmus Villemoes 2019-05-02 4:54 ` Christophe Leroy @ 2019-05-09 2:35 ` Qiang Zhao 1 sibling, 0 replies; 40+ messages in thread From: Qiang Zhao @ 2019-05-09 2:35 UTC (permalink / raw) To: Rasmus Villemoes, devicetree, Leo Li Cc: linuxppc-dev, linux-arm-kernel, linux-kernel, Rob Herring, Scott Wood, Christophe Leroy, Mark Rutland, Rasmus Villemoes On 2019/5/1 17:29, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > -----Original Message----- > From: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > Sent: 2019年5月1日 17:29 > To: devicetree@vger.kernel.org; Qiang Zhao <qiang.zhao@nxp.com>; Leo Li > <leoyang.li@nxp.com> > Cc: linuxppc-dev@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org; > linux-kernel@vger.kernel.org; Rob Herring <robh+dt@kernel.org>; Scott Wood > <oss@buserror.net>; Christophe Leroy <christophe.leroy@c-s.fr>; Mark > Rutland <mark.rutland@arm.com>; Rasmus Villemoes > <Rasmus.Villemoes@prevas.se> > Subject: [PATCH v2 6/6] soc/fsl/qe: qe.c: fold qe_get_num_of_snums into > qe_snums_init > > 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 <rasmus.villemoes@prevas.dk> > --- Reviewed-by: Qiang Zhao <qiang.zhao@nxp.com> > drivers/soc/fsl/qe/qe.c | 46 ++++++++++++++--------------------------- > 1 file changed, 16 insertions(+), 30 deletions(-) > > diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c index > 325d689cbf5c..276d7d78ebfc 100644 > --- a/drivers/soc/fsl/qe/qe.c > +++ b/drivers/soc/fsl/qe/qe.c > @@ -308,24 +308,33 @@ 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) > + 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; > - > + } 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); } > > @@ -641,30 +650,7 @@ 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; > + return qe_num_of_snum; > } > EXPORT_SYMBOL(qe_get_num_of_snums); > > -- > 2.20.1 Best Regards Qiang Zhao ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v3 0/6] soc/fsl/qe: cleanups and new DT binding 2019-05-01 9:29 ` [PATCH v2 0/6] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes ` (5 preceding siblings ...) 2019-05-01 9:29 ` [PATCH v2 6/6] soc/fsl/qe: qe.c: fold qe_get_num_of_snums into qe_snums_init Rasmus Villemoes @ 2019-05-13 11:14 ` Rasmus Villemoes 2019-05-13 11:14 ` [PATCH v3 1/6] soc/fsl/qe: qe.c: drop useless static qualifier Rasmus Villemoes ` (7 more replies) 6 siblings, 8 replies; 40+ messages in thread From: Rasmus Villemoes @ 2019-05-13 11:14 UTC (permalink / raw) To: devicetree, Qiang Zhao, Li Yang Cc: linuxppc-dev, linux-arm-kernel, linux-kernel, Rob Herring, Scott Wood, Christophe Leroy, Mark Rutland, Joakim Tjernlund, Rasmus Villemoes This small series consists of some small cleanups and simplifications of the QUICC engine driver, and introduces a new DT binding that makes it much easier to support other variants of the QUICC engine IP block that appears in the wild: There's no reason to expect in general that the number of valid SNUMs uniquely determines the set of such, so it's better to simply let the device tree specify the values (and, implicitly via the array length, also the count). Which tree should this go through? v3: - Move example from commit log into binding document (adapting to MPC8360 which the existing example pertains to). - Add more review tags. - Fix minor style issue. v2: - Address comments from Christophe Leroy - Add his Reviewed-by to 1/6 and 3/6 - Split DT binding update to separate patch as per Documentation/devicetree/bindings/submitting-patches.txt Rasmus Villemoes (6): 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 dt-bindings: soc/fsl: qe: document new fsl,qe-snums binding soc/fsl/qe: qe.c: support fsl,qe-snums property soc/fsl/qe: qe.c: fold qe_get_num_of_snums into qe_snums_init .../devicetree/bindings/soc/fsl/cpm_qe/qe.txt | 13 +- drivers/soc/fsl/qe/qe.c | 163 +++++++----------- 2 files changed, 77 insertions(+), 99 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v3 1/6] soc/fsl/qe: qe.c: drop useless static qualifier 2019-05-13 11:14 ` [PATCH v3 0/6] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes @ 2019-05-13 11:14 ` Rasmus Villemoes 2019-05-13 11:14 ` [PATCH v3 2/6] soc/fsl/qe: qe.c: reduce static memory footprint by 1.7K Rasmus Villemoes ` (6 subsequent siblings) 7 siblings, 0 replies; 40+ messages in thread From: Rasmus Villemoes @ 2019-05-13 11:14 UTC (permalink / raw) To: devicetree, Qiang Zhao, Li Yang Cc: linuxppc-dev, linux-arm-kernel, linux-kernel, Rob Herring, Scott Wood, Christophe Leroy, Mark Rutland, Joakim Tjernlund, Rasmus Villemoes The local variable snum_init has no reason to have static storage duration. Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr> Reviewed-by: Qiang Zhao <qiang.zhao@nxp.com> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- 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 612d9c551be5..855373deb746 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 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v3 2/6] soc/fsl/qe: qe.c: reduce static memory footprint by 1.7K 2019-05-13 11:14 ` [PATCH v3 0/6] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes 2019-05-13 11:14 ` [PATCH v3 1/6] soc/fsl/qe: qe.c: drop useless static qualifier Rasmus Villemoes @ 2019-05-13 11:14 ` Rasmus Villemoes 2019-05-13 11:14 ` [PATCH v3 3/6] soc/fsl/qe: qe.c: introduce qe_get_device_node helper Rasmus Villemoes ` (5 subsequent siblings) 7 siblings, 0 replies; 40+ messages in thread From: Rasmus Villemoes @ 2019-05-13 11:14 UTC (permalink / raw) To: devicetree, Qiang Zhao, Li Yang Cc: linuxppc-dev, linux-arm-kernel, linux-kernel, Rob Herring, Scott Wood, Christophe Leroy, Mark Rutland, Joakim Tjernlund, Rasmus Villemoes 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. Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr> Reviewed-by: Qiang Zhao <qiang.zhao@nxp.com> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- drivers/soc/fsl/qe/qe.c | 42 ++++++++++++----------------------------- 1 file changed, 12 insertions(+), 30 deletions(-) diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c index 855373deb746..4b59109df22b 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; @@ -315,10 +305,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; - } + bitmap_zero(snum_state, QE_NUM_OF_SNUM); + memcpy(snums, snum_init, qe_num_of_snum); } int qe_get_snum(void) @@ -328,12 +316,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); @@ -343,14 +329,10 @@ EXPORT_SYMBOL(qe_get_snum); void qe_put_snum(u8 snum) { - int i; + const u8 *p = memchr(snums, snum, qe_num_of_snum); - for (i = 0; i < qe_num_of_snum; i++) { - if (snums[i].num == snum) { - snums[i].state = QE_SNUM_STATE_FREE; - break; - } - } + if (p) + clear_bit(p - snums, snum_state); } EXPORT_SYMBOL(qe_put_snum); -- 2.20.1 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v3 3/6] soc/fsl/qe: qe.c: introduce qe_get_device_node helper 2019-05-13 11:14 ` [PATCH v3 0/6] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes 2019-05-13 11:14 ` [PATCH v3 1/6] soc/fsl/qe: qe.c: drop useless static qualifier Rasmus Villemoes 2019-05-13 11:14 ` [PATCH v3 2/6] soc/fsl/qe: qe.c: reduce static memory footprint by 1.7K Rasmus Villemoes @ 2019-05-13 11:14 ` Rasmus Villemoes 2019-05-13 11:14 ` [PATCH v3 4/6] dt-bindings: soc/fsl: qe: document new fsl,qe-snums binding Rasmus Villemoes ` (4 subsequent siblings) 7 siblings, 0 replies; 40+ messages in thread From: Rasmus Villemoes @ 2019-05-13 11:14 UTC (permalink / raw) To: devicetree, Qiang Zhao, Li Yang Cc: linuxppc-dev, linux-arm-kernel, linux-kernel, Rob Herring, Scott Wood, Christophe Leroy, Mark Rutland, Joakim Tjernlund, Rasmus Villemoes 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. Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr> Reviewed-by: Qiang Zhao <qiang.zhao@nxp.com> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- 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 4b59109df22b..4b444846d590 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)) @@ -558,16 +566,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 */ fw = of_get_child_by_name(qe, "firmware"); @@ -613,16 +614,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)) @@ -642,16 +636,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 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v3 4/6] dt-bindings: soc/fsl: qe: document new fsl,qe-snums binding 2019-05-13 11:14 ` [PATCH v3 0/6] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes ` (2 preceding siblings ...) 2019-05-13 11:14 ` [PATCH v3 3/6] soc/fsl/qe: qe.c: introduce qe_get_device_node helper Rasmus Villemoes @ 2019-05-13 11:14 ` Rasmus Villemoes 2019-05-13 11:39 ` Joakim Tjernlund 2019-05-13 17:51 ` Rob Herring 2019-05-13 11:15 ` [PATCH v3 5/6] soc/fsl/qe: qe.c: support fsl,qe-snums property Rasmus Villemoes ` (3 subsequent siblings) 7 siblings, 2 replies; 40+ messages in thread From: Rasmus Villemoes @ 2019-05-13 11:14 UTC (permalink / raw) To: devicetree, Qiang Zhao, Li Yang Cc: linuxppc-dev, linux-arm-kernel, linux-kernel, Rob Herring, Scott Wood, Christophe Leroy, Mark Rutland, Joakim Tjernlund, Rasmus Villemoes Reading table 4-30, and its footnotes, of the QUICC Engine Block Reference Manual shows that the set of snum _values_ is not necessarily just a function of the _number_ of snums, as given in the fsl,qe-num-snums property. As an alternative, to make it easier to add support for other variants of the QUICC engine IP, this introduces a new binding fsl,qe-snums, which automatically encodes both the number of snums and the actual values to use. Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- Rob, thanks for the review of v2. However, since I moved the example from the commit log to the binding (per Joakim's request), I didn't add a Reviewed-by tag for this revision. .../devicetree/bindings/soc/fsl/cpm_qe/qe.txt | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/soc/fsl/cpm_qe/qe.txt b/Documentation/devicetree/bindings/soc/fsl/cpm_qe/qe.txt index d7afaff5faff..05ec2a838c54 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>; @@ -44,6 +50,11 @@ Example: reg = <e0100000 480>; brg-frequency = <0>; bus-frequency = <179A7B00>; + fsl,qe-snums = /bits/ 8 < + 0x04 0x05 0x0C 0x0D 0x14 0x15 0x1C 0x1D + 0x24 0x25 0x2C 0x2D 0x34 0x35 0x88 0x89 + 0x98 0x99 0xA8 0xA9 0xB8 0xB9 0xC8 0xC9 + 0xD8 0xD9 0xE8 0xE9>; } * Multi-User RAM (MURAM) -- 2.20.1 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v3 4/6] dt-bindings: soc/fsl: qe: document new fsl,qe-snums binding 2019-05-13 11:14 ` [PATCH v3 4/6] dt-bindings: soc/fsl: qe: document new fsl,qe-snums binding Rasmus Villemoes @ 2019-05-13 11:39 ` Joakim Tjernlund 2019-05-13 17:51 ` Rob Herring 1 sibling, 0 replies; 40+ messages in thread From: Joakim Tjernlund @ 2019-05-13 11:39 UTC (permalink / raw) To: rasmus.villemoes, leoyang.li, qiang.zhao, devicetree Cc: linux-kernel, robh+dt, linuxppc-dev, Rasmus.Villemoes, mark.rutland, christophe.leroy, linux-arm-kernel, oss On Mon, 2019-05-13 at 11:14 +0000, Rasmus Villemoes wrote: > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. > > > Reading table 4-30, and its footnotes, of the QUICC Engine Block > Reference Manual shows that the set of snum _values_ is not > necessarily just a function of the _number_ of snums, as given in the > fsl,qe-num-snums property. > > As an alternative, to make it easier to add support for other variants > of the QUICC engine IP, this introduces a new binding fsl,qe-snums, > which automatically encodes both the number of snums and the actual > values to use. > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > --- > Rob, thanks for the review of v2. However, since I moved the example > from the commit log to the binding (per Joakim's request), I didn't Thanks, looks good now. > add a Reviewed-by tag for this revision. > > .../devicetree/bindings/soc/fsl/cpm_qe/qe.txt | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/soc/fsl/cpm_qe/qe.txt b/Documentation/devicetree/bindings/soc/fsl/cpm_qe/qe.txt > index d7afaff5faff..05ec2a838c54 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>; > @@ -44,6 +50,11 @@ Example: > reg = <e0100000 480>; > brg-frequency = <0>; > bus-frequency = <179A7B00>; > + fsl,qe-snums = /bits/ 8 < > + 0x04 0x05 0x0C 0x0D 0x14 0x15 0x1C 0x1D > + 0x24 0x25 0x2C 0x2D 0x34 0x35 0x88 0x89 > + 0x98 0x99 0xA8 0xA9 0xB8 0xB9 0xC8 0xC9 > + 0xD8 0xD9 0xE8 0xE9>; > } > > * Multi-User RAM (MURAM) > -- > 2.20.1 > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 4/6] dt-bindings: soc/fsl: qe: document new fsl,qe-snums binding 2019-05-13 11:14 ` [PATCH v3 4/6] dt-bindings: soc/fsl: qe: document new fsl,qe-snums binding Rasmus Villemoes 2019-05-13 11:39 ` Joakim Tjernlund @ 2019-05-13 17:51 ` Rob Herring 1 sibling, 0 replies; 40+ messages in thread From: Rob Herring @ 2019-05-13 17:51 UTC (permalink / raw) To: Rasmus Villemoes Cc: devicetree, Qiang Zhao, Li Yang, linuxppc-dev, linux-arm-kernel, linux-kernel, Scott Wood, Christophe Leroy, Mark Rutland, Joakim Tjernlund, Rasmus Villemoes On Mon, 13 May 2019 11:14:58 +0000, Rasmus Villemoes wrote: > Reading table 4-30, and its footnotes, of the QUICC Engine Block > Reference Manual shows that the set of snum _values_ is not > necessarily just a function of the _number_ of snums, as given in the > fsl,qe-num-snums property. > > As an alternative, to make it easier to add support for other variants > of the QUICC engine IP, this introduces a new binding fsl,qe-snums, > which automatically encodes both the number of snums and the actual > values to use. > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > --- > Rob, thanks for the review of v2. However, since I moved the example > from the commit log to the binding (per Joakim's request), I didn't > add a Reviewed-by tag for this revision. > > .../devicetree/bindings/soc/fsl/cpm_qe/qe.txt | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > Reviewed-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v3 5/6] soc/fsl/qe: qe.c: support fsl,qe-snums property 2019-05-13 11:14 ` [PATCH v3 0/6] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes ` (3 preceding siblings ...) 2019-05-13 11:14 ` [PATCH v3 4/6] dt-bindings: soc/fsl: qe: document new fsl,qe-snums binding Rasmus Villemoes @ 2019-05-13 11:15 ` Rasmus Villemoes 2019-05-13 11:15 ` [PATCH v3 6/6] soc/fsl/qe: qe.c: fold qe_get_num_of_snums into qe_snums_init Rasmus Villemoes ` (2 subsequent siblings) 7 siblings, 0 replies; 40+ messages in thread From: Rasmus Villemoes @ 2019-05-13 11:15 UTC (permalink / raw) To: devicetree, Qiang Zhao, Li Yang Cc: linuxppc-dev, linux-arm-kernel, linux-kernel, Rob Herring, Scott Wood, Christophe Leroy, Mark Rutland, Joakim Tjernlund, Rasmus Villemoes Add driver support for the newly introduced fsl,qe-snums property. Conveniently, of_property_read_variable_u8_array does exactly what we need: If the property fsl,qe-snums is found (and has an allowed size), the array of values get copied to snums, and the return value is the number of snums - we cannot assign directly to num_of_snums, since we need to check whether the return value is negative. Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr> Reviewed-by: Qiang Zhao <qiang.zhao@nxp.com> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- drivers/soc/fsl/qe/qe.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c index 4b444846d590..1d27187b251c 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,7 +303,21 @@ 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, 1, 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(); @@ -313,7 +326,6 @@ static void qe_snums_init(void) else snum_init = snum_init_46; - bitmap_zero(snum_state, QE_NUM_OF_SNUM); memcpy(snums, snum_init, qe_num_of_snum); } -- 2.20.1 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [PATCH v3 6/6] soc/fsl/qe: qe.c: fold qe_get_num_of_snums into qe_snums_init 2019-05-13 11:14 ` [PATCH v3 0/6] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes ` (4 preceding siblings ...) 2019-05-13 11:15 ` [PATCH v3 5/6] soc/fsl/qe: qe.c: support fsl,qe-snums property Rasmus Villemoes @ 2019-05-13 11:15 ` Rasmus Villemoes 2019-06-03 19:53 ` [PATCH v3 0/6] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes 2019-06-04 20:29 ` Li Yang 7 siblings, 0 replies; 40+ messages in thread From: Rasmus Villemoes @ 2019-05-13 11:15 UTC (permalink / raw) To: devicetree, Qiang Zhao, Li Yang Cc: linuxppc-dev, linux-arm-kernel, linux-kernel, Rob Herring, Scott Wood, Christophe Leroy, Mark Rutland, Joakim Tjernlund, Rasmus Villemoes 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. Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr> Reviewed-by: Qiang Zhao <qiang.zhao@nxp.com> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- drivers/soc/fsl/qe/qe.c | 46 ++++++++++++++--------------------------- 1 file changed, 16 insertions(+), 30 deletions(-) diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c index 1d27187b251c..852060caff24 100644 --- a/drivers/soc/fsl/qe/qe.c +++ b/drivers/soc/fsl/qe/qe.c @@ -308,24 +308,33 @@ 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) + 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; - + } 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); } @@ -642,30 +651,7 @@ 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; + return qe_num_of_snum; } EXPORT_SYMBOL(qe_get_num_of_snums); -- 2.20.1 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v3 0/6] soc/fsl/qe: cleanups and new DT binding 2019-05-13 11:14 ` [PATCH v3 0/6] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes ` (5 preceding siblings ...) 2019-05-13 11:15 ` [PATCH v3 6/6] soc/fsl/qe: qe.c: fold qe_get_num_of_snums into qe_snums_init Rasmus Villemoes @ 2019-06-03 19:53 ` Rasmus Villemoes 2019-06-03 19:55 ` Leo Li 2019-06-04 20:29 ` Li Yang 7 siblings, 1 reply; 40+ messages in thread From: Rasmus Villemoes @ 2019-06-03 19:53 UTC (permalink / raw) To: devicetree, Qiang Zhao, Li Yang Cc: linuxppc-dev, linux-arm-kernel, linux-kernel, Rob Herring, Scott Wood, Christophe Leroy, Mark Rutland, Joakim Tjernlund On 13/05/2019 13.14, Rasmus Villemoes wrote: > This small series consists of some small cleanups and simplifications > of the QUICC engine driver, and introduces a new DT binding that makes > it much easier to support other variants of the QUICC engine IP block > that appears in the wild: There's no reason to expect in general that > the number of valid SNUMs uniquely determines the set of such, so it's > better to simply let the device tree specify the values (and, > implicitly via the array length, also the count). > > Which tree should this go through? Ping? These patches should be ready to go in, but I don't know who is supposed to pick them up. Thanks, Rasmus ^ permalink raw reply [flat|nested] 40+ messages in thread
* RE: [PATCH v3 0/6] soc/fsl/qe: cleanups and new DT binding 2019-06-03 19:53 ` [PATCH v3 0/6] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes @ 2019-06-03 19:55 ` Leo Li 0 siblings, 0 replies; 40+ messages in thread From: Leo Li @ 2019-06-03 19:55 UTC (permalink / raw) To: Rasmus Villemoes, devicetree, Qiang Zhao Cc: linuxppc-dev, linux-arm-kernel, linux-kernel, Rob Herring, Scott Wood, Christophe Leroy, Mark Rutland, jocke@infinera.com > -----Original Message----- > From: Rasmus Villemoes <Rasmus.Villemoes@prevas.se> > Sent: Monday, June 3, 2019 2:54 PM > To: devicetree@vger.kernel.org; Qiang Zhao <qiang.zhao@nxp.com>; Leo Li > <leoyang.li@nxp.com> > Cc: linuxppc-dev@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org; > linux-kernel@vger.kernel.org; Rob Herring <robh+dt@kernel.org>; Scott > Wood <oss@buserror.net>; Christophe Leroy <christophe.leroy@c-s.fr>; > Mark Rutland <mark.rutland@arm.com>; jocke@infinera.com > <joakim.tjernlund@infinera.com> > Subject: Re: [PATCH v3 0/6] soc/fsl/qe: cleanups and new DT binding > > On 13/05/2019 13.14, Rasmus Villemoes wrote: > > This small series consists of some small cleanups and simplifications > > of the QUICC engine driver, and introduces a new DT binding that makes > > it much easier to support other variants of the QUICC engine IP block > > that appears in the wild: There's no reason to expect in general that > > the number of valid SNUMs uniquely determines the set of such, so it's > > better to simply let the device tree specify the values (and, > > implicitly via the array length, also the count). > > > > Which tree should this go through? > > Ping? These patches should be ready to go in, but I don't know who is > supposed to pick them up. I can pick them up through the soc/fsl tree. Regards, Leo ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v3 0/6] soc/fsl/qe: cleanups and new DT binding 2019-05-13 11:14 ` [PATCH v3 0/6] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes ` (6 preceding siblings ...) 2019-06-03 19:53 ` [PATCH v3 0/6] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes @ 2019-06-04 20:29 ` Li Yang 7 siblings, 0 replies; 40+ messages in thread From: Li Yang @ 2019-06-04 20:29 UTC (permalink / raw) To: Rasmus Villemoes Cc: devicetree, Qiang Zhao, Mark Rutland, linux-kernel, Scott Wood, Rasmus Villemoes, Rob Herring, linuxppc-dev, linux-arm-kernel On Mon, May 13, 2019 at 6:17 AM Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > > This small series consists of some small cleanups and simplifications > of the QUICC engine driver, and introduces a new DT binding that makes > it much easier to support other variants of the QUICC engine IP block > that appears in the wild: There's no reason to expect in general that > the number of valid SNUMs uniquely determines the set of such, so it's > better to simply let the device tree specify the values (and, > implicitly via the array length, also the count). > > Which tree should this go through? > > v3: > - Move example from commit log into binding document (adapting to > MPC8360 which the existing example pertains to). > - Add more review tags. > - Fix minor style issue. > > v2: > - Address comments from Christophe Leroy > - Add his Reviewed-by to 1/6 and 3/6 > - Split DT binding update to separate patch as per > Documentation/devicetree/bindings/submitting-patches.txt > > Rasmus Villemoes (6): > 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 > dt-bindings: soc/fsl: qe: document new fsl,qe-snums binding > soc/fsl/qe: qe.c: support fsl,qe-snums property > soc/fsl/qe: qe.c: fold qe_get_num_of_snums into qe_snums_init Series applied to soc/fsl for-next. Thanks! Regards, Leo > > .../devicetree/bindings/soc/fsl/cpm_qe/qe.txt | 13 +- > drivers/soc/fsl/qe/qe.c | 163 +++++++----------- > 2 files changed, 77 insertions(+), 99 deletions(-) > > -- > 2.20.1 > ^ permalink raw reply [flat|nested] 40+ messages in thread
* RE: [PATCH v2 3/6] soc/fsl/qe: qe.c: introduce qe_get_device_node helper @ 2019-05-09 2:31 Qiang Zhao 0 siblings, 0 replies; 40+ messages in thread From: Qiang Zhao @ 2019-05-09 2:31 UTC (permalink / raw) To: Rasmus Villemoes, devicetree, Leo Li Cc: linuxppc-dev, linux-arm-kernel, linux-kernel, Rob Herring, Scott Wood, Christophe Leroy, Mark Rutland, Rasmus Villemoes On 2019/5/1 17:29, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > -----Original Message----- > From: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > Sent: 2019年5月1日 17:29 > To: devicetree@vger.kernel.org; Qiang Zhao <qiang.zhao@nxp.com>; Leo Li > <leoyang.li@nxp.com> > Cc: linuxppc-dev@lists.ozlabs.org; linux-arm-kernel@lists.infradead.org; > linux-kernel@vger.kernel.org; Rob Herring <robh+dt@kernel.org>; Scott Wood > <oss@buserror.net>; Christophe Leroy <christophe.leroy@c-s.fr>; Mark > Rutland <mark.rutland@arm.com>; Rasmus Villemoes > <Rasmus.Villemoes@prevas.se> > Subject: [PATCH v2 3/6] soc/fsl/qe: qe.c: introduce qe_get_device_node > helper > > 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. > > Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr> > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > --- Reviewed-by: Qiang Zhao <qiang.zhao@nxp.com> > 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 > 303aa29cb27d..0fb8b59f61ad 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)) @@ -557,16 +565,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 */ > fw = of_get_child_by_name(qe, "firmware"); @@ -612,16 +613,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)) @@ -641,16 +635,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 Best Regards Qiang Zhao ^ permalink raw reply [flat|nested] 40+ messages in thread
end of thread, other threads:[~2019-06-04 20:29 UTC | newest] Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-30 13:36 [PATCH RESEND 0/5] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes 2019-04-30 13:36 ` [PATCH 1/5] soc/fsl/qe: qe.c: drop useless static qualifier Rasmus Villemoes 2019-04-30 17:03 ` Christophe Leroy 2019-04-30 13:36 ` [PATCH 2/5] soc/fsl/qe: qe.c: reduce static memory footprint by 1.7K Rasmus Villemoes 2019-04-30 17:12 ` Christophe Leroy 2019-05-01 5:51 ` Rasmus Villemoes 2019-04-30 13:36 ` [PATCH 3/5] soc/fsl/qe: qe.c: introduce qe_get_device_node helper Rasmus Villemoes 2019-04-30 17:14 ` Christophe Leroy 2019-04-30 13:36 ` [PATCH 4/5] soc/fsl/qe: qe.c: support fsl,qe-snums property Rasmus Villemoes 2019-04-30 17:19 ` Christophe Leroy 2019-05-01 6:00 ` Rasmus Villemoes 2019-04-30 13:36 ` [PATCH 5/5] soc/fsl/qe: qe.c: fold qe_get_num_of_snums into qe_snums_init Rasmus Villemoes 2019-04-30 17:27 ` Christophe Leroy 2019-05-01 9:29 ` [PATCH v2 0/6] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes 2019-05-01 9:29 ` [PATCH v2 1/6] soc/fsl/qe: qe.c: drop useless static qualifier Rasmus Villemoes 2019-05-01 9:29 ` [PATCH v2 2/6] soc/fsl/qe: qe.c: reduce static memory footprint by 1.7K Rasmus Villemoes 2019-05-02 4:51 ` Christophe Leroy 2019-05-01 9:29 ` [PATCH v2 3/6] soc/fsl/qe: qe.c: introduce qe_get_device_node helper Rasmus Villemoes 2019-05-01 9:29 ` [PATCH v2 4/6] dt-bindings: soc/fsl: qe: document new fsl,qe-snums binding Rasmus Villemoes 2019-05-01 15:12 ` Joakim Tjernlund 2019-05-01 18:47 ` Rasmus Villemoes 2019-05-01 21:00 ` Rob Herring 2019-05-01 9:29 ` [PATCH v2 5/6] soc/fsl/qe: qe.c: support fsl,qe-snums property Rasmus Villemoes 2019-05-02 4:52 ` Christophe Leroy 2019-05-01 9:29 ` [PATCH v2 6/6] soc/fsl/qe: qe.c: fold qe_get_num_of_snums into qe_snums_init Rasmus Villemoes 2019-05-02 4:54 ` Christophe Leroy 2019-05-09 2:35 ` [EXT] " Qiang Zhao 2019-05-13 11:14 ` [PATCH v3 0/6] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes 2019-05-13 11:14 ` [PATCH v3 1/6] soc/fsl/qe: qe.c: drop useless static qualifier Rasmus Villemoes 2019-05-13 11:14 ` [PATCH v3 2/6] soc/fsl/qe: qe.c: reduce static memory footprint by 1.7K Rasmus Villemoes 2019-05-13 11:14 ` [PATCH v3 3/6] soc/fsl/qe: qe.c: introduce qe_get_device_node helper Rasmus Villemoes 2019-05-13 11:14 ` [PATCH v3 4/6] dt-bindings: soc/fsl: qe: document new fsl,qe-snums binding Rasmus Villemoes 2019-05-13 11:39 ` Joakim Tjernlund 2019-05-13 17:51 ` Rob Herring 2019-05-13 11:15 ` [PATCH v3 5/6] soc/fsl/qe: qe.c: support fsl,qe-snums property Rasmus Villemoes 2019-05-13 11:15 ` [PATCH v3 6/6] soc/fsl/qe: qe.c: fold qe_get_num_of_snums into qe_snums_init Rasmus Villemoes 2019-06-03 19:53 ` [PATCH v3 0/6] soc/fsl/qe: cleanups and new DT binding Rasmus Villemoes 2019-06-03 19:55 ` Leo Li 2019-06-04 20:29 ` Li Yang 2019-05-09 2:31 [PATCH v2 3/6] soc/fsl/qe: qe.c: introduce qe_get_device_node helper Qiang Zhao
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).