Re: [PATCH 4/4] soc/fsl/qe: qe.c: support fsl,qe-snums property
On 01/03/2019 10.43, Qiang Zhao wrote:
> On 01/03/2019 15.50，Rasmus Villemoes wrote:
>> -Original Message-
>> From: Rasmus Villemoes
>> Sent: 2019年3月1日 15:50
>> To: Qiang Zhao ; Leo Li
>> Cc: Scott Wood ; linux-kernel@vger.kernel.org; Timur Tabi
>> ; Rasmus Villemoes
>> Subject: Re: [PATCH 4/4] soc/fsl/qe: qe.c: support fsl,qe-snums property
>>
>> On 01/03/2019 04.36, Qiang Zhao wrote:
>>> On 2019年2月28日 18:31，Rasmus Villemoes wrote:
>>>
>>>> -Original Message-
>>>> From: Rasmus Villemoes
>>>> Sent: 2019年2月28日 18:31
>>>> To: Qiang Zhao ; Leo Li
>>>> Cc: Scott Wood ; linux-kernel@vger.kernel.org;
>>>> Timur Tabi ; Rasmus Villemoes
>>>>
>>>> Subject: [PATCH 4/4] soc/fsl/qe: qe.c: support fsl,qe-snums property
>>>>
>>>
>>> So you define 14 snums for MPC8309, but there still be the comment "/*
>>> No QE ever has fewer than 28 SNUMs */" and it will check if The
>> num_of_snums "is >28", it will cause confusion, so I suggest to modify 28 to
>> 14.
>>
>> Sure, that needs updating. My thinking was that only legacy DTs would use the
>> fsl,qe-num-snums, and there would be no need to support lower values than
>> we used to, since the logic back in qe_snums_init wouldn't handle such values
>> appropriately anyway.
>>
>>> I read the old version QUICC Engine Block Reference Manual, it said
>>> snums table is not available on MPC8306/MPC8306S/MPC8309, So I think
>> the code it written long before with this version RM, and at that time, the
>> snums is at least 28, and nobody modify the code later.
>>> And now with the new version RM, it support
>> MPC8306/MPC8306S/MPC8309
>>> with snums and have snums fewer then 28, so I think the minimum value
>> should Be modified to 14.
>>
>> Yes. I'll do an extra cleanup patch modifying the code comments
>> appropriately.
>> But what do you think about the core idea behind this change (and the
>> preceding cleanup patches)?
>
> Maybe we could modify the comments in this patch, Anyway, the
> MPC8306/MPC8306S/MPC8309
> Is supported with snums and the number is 14, In addition, you assign
> qe_num_of_snum to i as below.
> The variable stands for num of snums.
Yes, but I can't assign it directly, because qe_num_of_snum is unsigned,
and I need to test whether the return value is negative.
> Or we could add comments to explain it clearly why qe_num_of_snum is assign
> to a value fewer than 28
> While it says " No QE ever has fewer than 28 SNUMs ".
OK, I'll fold in some comment updating in a v2 patch.
>
> + qe = qe_get_device_node();
> + if (qe) {
> + i = of_property_read_variable_u8_array(qe, "fsl,qe-snums",
> +snums, 14,
> QE_NUM_OF_SNUM);
> + of_node_put(qe);
> + if (i > 0) {
> + qe_num_of_snum = i;
> + return;
> + }
> + }
Actually, putting in a lower bound of 14 here is a mistake - it should
just be 1, and then the code would also work for any QE variant that
might be shipped in the future. It's up to the DT author to make sure
the data is correct.
Can you figure out why the old code defaults to 28, and why its ok for
all those variants to just end up using the first 28 elements of the _46
array? What's the purpose of those surplus 18 elements?
Rasmus
> Best Regards
> Qiang Zhao
>
--
Rasmus Villemoes
Software Developer
Prevas A/S
Hedeager 3
DK-8200 Aarhus N
+45 51210274
rasmus.villem...@prevas.dk
www.prevas.dk
RE: [PATCH 4/4] soc/fsl/qe: qe.c: support fsl,qe-snums property
On 01/03/2019 15.50，Rasmus Villemoes wrote:
> -Original Message-
> From: Rasmus Villemoes
> Sent: 2019年3月1日 15:50
> To: Qiang Zhao ; Leo Li
> Cc: Scott Wood ; linux-kernel@vger.kernel.org; Timur Tabi
> ; Rasmus Villemoes
> Subject: Re: [PATCH 4/4] soc/fsl/qe: qe.c: support fsl,qe-snums property
>
> On 01/03/2019 04.36, Qiang Zhao wrote:
> > On 2019年2月28日 18:31，Rasmus Villemoes wrote:
> >
> >> -Original Message-
> >> From: Rasmus Villemoes
> >> Sent: 2019年2月28日 18:31
> >> To: Qiang Zhao ; Leo Li
> >> Cc: Scott Wood ; linux-kernel@vger.kernel.org;
> >> Timur Tabi ; Rasmus Villemoes
> >>
> >> Subject: [PATCH 4/4] soc/fsl/qe: qe.c: support fsl,qe-snums property
> >>
> >
> > So you define 14 snums for MPC8309, but there still be the comment "/*
> > No QE ever has fewer than 28 SNUMs */" and it will check if The
> num_of_snums "is >28", it will cause confusion, so I suggest to modify 28 to
> 14.
>
> Sure, that needs updating. My thinking was that only legacy DTs would use the
> fsl,qe-num-snums, and there would be no need to support lower values than
> we used to, since the logic back in qe_snums_init wouldn't handle such values
> appropriately anyway.
>
> > I read the old version QUICC Engine Block Reference Manual, it said
> > snums table is not available on MPC8306/MPC8306S/MPC8309, So I think
> the code it written long before with this version RM, and at that time, the
> snums is at least 28, and nobody modify the code later.
> > And now with the new version RM, it support
> MPC8306/MPC8306S/MPC8309
> > with snums and have snums fewer then 28, so I think the minimum value
> should Be modified to 14.
>
> Yes. I'll do an extra cleanup patch modifying the code comments appropriately.
> But what do you think about the core idea behind this change (and the
> preceding cleanup patches)?
Maybe we could modify the comments in this patch, Anyway, the
MPC8306/MPC8306S/MPC8309
Is supported with snums and the number is 14, In addition, you assign
qe_num_of_snum to i as below.
The variable stands for num of snums.
Or we could add comments to explain it clearly why qe_num_of_snum is assign to
a value fewer than 28
While it says " No QE ever has fewer than 28 SNUMs ".
+ qe = qe_get_device_node();
+ if (qe) {
+ i = of_property_read_variable_u8_array(qe, "fsl,qe-snums",
+ snums, 14,
QE_NUM_OF_SNUM);
+ of_node_put(qe);
+ if (i > 0) {
+ qe_num_of_snum = i;
+ return;
+ }
+ }
Best Regards
Qiang Zhao
Re: [PATCH 4/4] soc/fsl/qe: qe.c: support fsl,qe-snums property
On 01/03/2019 04.36, Qiang Zhao wrote:
> On 2019年2月28日 18:31，Rasmus Villemoes wrote:
>
>> -Original Message-
>> From: Rasmus Villemoes
>> Sent: 2019年2月28日 18:31
>> To: Qiang Zhao ; Leo Li
>> Cc: Scott Wood ; linux-kernel@vger.kernel.org; Timur Tabi
>> ; Rasmus Villemoes
>> Subject: [PATCH 4/4] soc/fsl/qe: qe.c: support fsl,qe-snums property
>>
>> The current code assumes that the set of snum _values_ to populate the
>> snums[] array with is a function of the _number_ of snums alone. However,
>> reading table 4-30, and its footnotes, of the QUICC Engine Block Reference
>> Manual shows that that is a bit too naive.
>>
>> As an alternative, this introduces a new binding fsl,qe-snums, which
>> automatically encodes both the number of snums and the actual values to use.
>> Conveniently, of_property_read_variable_u8_array does exactly what we need.
>>
>> For example, for the MPC8309, one would specify the property as
>>
>>fsl,qe-snums = /bits/ 8 <
>>0x88 0x89 0x98 0x99 0xa8 0xa9 0xb8 0xb9
>>0xc8 0xc9 0xd8 0xd9 0xe8 0xe9>;
>>
>> Signed-off-by: Rasmus Villemoes
>> ---
>> .../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@e010 {
>> #address-cells = <1>;
>> diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c index
>> 71beeb72eee4..049b36d6aeee 100644
>> --- a/drivers/soc/fsl/qe/qe.c
>> +++ b/drivers/soc/fsl/qe/qe.c
>> @@ -283,7 +283,6 @@ EXPORT_SYMBOL(qe_clock_source);
>> */
>> static void qe_snums_init(void)
>> {
>> -int i;
>> static const u8 snum_init_76[] = {
>> 0x04, 0x05, 0x0C, 0x0D, 0x14, 0x15, 0x1C, 0x1D,
>> 0x24, 0x25, 0x2C, 0x2D, 0x34, 0x35, 0x88, 0x89, @@ -304,9
>> +303,22 @@ static void qe_snums_init(void)
>> 0x28, 0x29, 0x38, 0x39, 0x48, 0x49, 0x58, 0x59,
>> 0x68, 0x69, 0x78, 0x79, 0x80, 0x81,
>> };
>> +struct device_node *qe;
>> const u8 *snum_init;
>> +int i;
>>
>> bitmap_zero(snum_state, QE_NUM_OF_SNUM);
>> +qe = qe_get_device_node();
>> +if (qe) {
>> +i = of_property_read_variable_u8_array(qe, "fsl,qe-snums",
>> + snums, 14,
>> QE_NUM_OF_SNUM);
>> +of_node_put(qe);
>> +if (i > 0) {
>> +qe_num_of_snum = i;
>> +return;
>> +}
>> +}
>> +
>> qe_num_of_snum = qe_get_num_of_snums();
>>
>> if (qe_num_of_snum == 76)
>
> So you define 14 snums for MPC8309, but there still be the comment "/* No QE
> ever has fewer than 28 SNUMs */" and it will check if
> The num_of_snums "is >28", it will cause confusion, so I suggest to modify 28
> to 14.
Sure, that needs updating. My thinking was that only legacy DTs would
use the fsl,qe-num-snums, and there would be no need to support lower
values than we used to, since the logic back in qe_snums_init wouldn't
handle such values appropriately anyway.
> I read the old version QUICC Engine Block Reference Manual, it said snums
> table is not available on MPC8306/MPC8306S/MPC8309,
> So I think the code it written long before with this version RM, and at that
> time, the snums is at least 28, and nobody modify the code later.
> And now with the new version RM, it support MPC8306/MPC8306S/MPC8309 with
> snums and have snums fewer then 28, so I think the minimum value should
> Be modified to 14.
Yes. I'll do an extra cleanup patch modifying the code comments
appropriately. But what do you think about the core idea behind this
change (and the preceding cleanup patches)?
Rasmus
RE: [PATCH 4/4] soc/fsl/qe: qe.c: support fsl,qe-snums property
On 2019年2月28日 18:31，Rasmus Villemoes wrote:
> -Original Message-
> From: Rasmus Villemoes
> Sent: 2019年2月28日 18:31
> To: Qiang Zhao ; Leo Li
> Cc: Scott Wood ; linux-kernel@vger.kernel.org; Timur Tabi
> ; Rasmus Villemoes
> Subject: [PATCH 4/4] soc/fsl/qe: qe.c: support fsl,qe-snums property
>
> The current code assumes that the set of snum _values_ to populate the
> snums[] array with is a function of the _number_ of snums alone. However,
> reading table 4-30, and its footnotes, of the QUICC Engine Block Reference
> Manual shows that that is a bit too naive.
>
> As an alternative, this introduces a new binding fsl,qe-snums, which
> automatically encodes both the number of snums and the actual values to use.
> Conveniently, of_property_read_variable_u8_array does exactly what we need.
>
> For example, for the MPC8309, one would specify the property as
>
>fsl,qe-snums = /bits/ 8 <
>0x88 0x89 0x98 0x99 0xa8 0xa9 0xb8 0xb9
>0xc8 0xc9 0xd8 0xd9 0xe8 0xe9>;
>
> Signed-off-by: Rasmus Villemoes
> ---
> .../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@e010 {
> #address-cells = <1>;
> diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c index
> 71beeb72eee4..049b36d6aeee 100644
> --- a/drivers/soc/fsl/qe/qe.c
> +++ b/drivers/soc/fsl/qe/qe.c
> @@ -283,7 +283,6 @@ EXPORT_SYMBOL(qe_clock_source);
> */
> static void qe_snums_init(void)
> {
> - int i;
> static const u8 snum_init_76[] = {
> 0x04, 0x05, 0x0C, 0x0D, 0x14, 0x15, 0x1C, 0x1D,
> 0x24, 0x25, 0x2C, 0x2D, 0x34, 0x35, 0x88, 0x89, @@ -304,9
> +303,22 @@ static void qe_snums_init(void)
> 0x28, 0x29, 0x38, 0x39, 0x48, 0x49, 0x58, 0x59,
> 0x68, 0x69, 0x78, 0x79, 0x80, 0x81,
> };
> + struct device_node *qe;
> const u8 *snum_init;
> + int i;
>
> bitmap_zero(snum_state, QE_NUM_OF_SNUM);
> + qe = qe_get_device_node();
> + if (qe) {
> + i = of_property_read_variable_u8_array(qe, "fsl,qe-snums",
> +snums, 14,
> QE_NUM_OF_SNUM);
> + of_node_put(qe);
> + if (i > 0) {
> + qe_num_of_snum = i;
> + return;
> + }
> + }
> +
> qe_num_of_snum = qe_get_num_of_snums();
>
> if (qe_num_of_snum == 76)
So you define 14 snums for MPC8309, but there still be the comment "/* No QE
ever has fewer than 28 SNUMs */" and it will check if
The num_of_snums "is >28", it will cause confusion, so I suggest to modify 28
to 14.
I read the old version QUICC Engine Block Reference Manual, it said snums table
is not available on MPC8306/MPC8306S/MPC8309,
So I think the code it written long before with this version RM, and at that
time, the snums is at least 28, and nobody modify the code later.
And now with the new version RM, it support MPC8306/MPC8306S/MPC8309 with snums
and have snums fewer then 28, so I think the minimum value should
Be modified to 14.
Thank you!
-Qiang Zhao
