| Message ID | 20190430133615.25721-5-rasmus.villemoes@prevas.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soc/fsl/qe: cleanups and new DT binding | expand |
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) >
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
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)
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(-)