Re: [PATCH 4/4] soc/fsl/qe: qe.c: support fsl,qe-snums property

[Rasmus Villemoes]

On 01/03/2019 10.43, Qiang Zhao wrote:
> On 01/03/2019 15.50ïRasmus Villemoes wrote:
>> -----Original Message-----
>> From: Rasmus Villemoes <rasmus.villemoes@xxxxxxxxx>
>> Sent: 2019å3æ1æ 15:50
>> To: Qiang Zhao <qiang.zhao@xxxxxxx>; Leo Li <leoyang.li@xxxxxxx>
>> Cc: Scott Wood <oss@xxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; Timur Tabi
>> <timur@xxxxxxxxxxxxx>; Rasmus Villemoes <Rasmus.Villemoes@xxxxxxxxx>
>> 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 <rasmus.villemoes@xxxxxxxxx>
>>>> Sent: 2019å2æ28æ 18:31
>>>> To: Qiang Zhao <qiang.zhao@xxxxxxx>; Leo Li <leoyang.li@xxxxxxx>
>>>> Cc: Scott Wood <oss@xxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
>>>> Timur Tabi <timur@xxxxxxxxxxxxx>; Rasmus Villemoes
>>>> <Rasmus.Villemoes@xxxxxxxxx>
>>>> 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.villemoes@xxxxxxxxx
www.prevas.dk