Re: [RFC PATCH] soc/fsl/qe: support MPC8309

[Rasmus Villemoes]

On 28/02/2019 08.14, Qiang Zhao wrote:
> On Tue, Feb 26, 2019 at 2:49 AM Rasmus Villemoes <rasmus.villemoes@xxxxxxxxx> wrote:
>> -----Original Message-----
>> From: Rasmus Villemoes <rasmus.villemoes@xxxxxxxxx>
>> Sent: 2019å2æ26æ 16:48
>> To: Qiang Zhao <qiang.zhao@xxxxxxx>; Leo Li <leoyang.li@xxxxxxx>
>> Cc: linux-kernel@xxxxxxxxxxxxxxx; Valentin Longchamp
>> <valentin.longchamp@xxxxxxxxxxx>; Scott Wood <oss@xxxxxxxxxxxx>;
>> Rasmus Villemoes <Rasmus.Villemoes@xxxxxxxxx>
>> Subject: [RFC PATCH] soc/fsl/qe: support MPC8309
>>
>> Currently, when device tree specifies fsl,qe-num-snums = 28 (which a number
>> of in-tree .dts files do, and which is the default when that property is missing),
>> qe_snums_init() ends up using the first 28 elements of the snum_init_46[]
>> array.
>>
>> The situation is quite messy. This patch may break existing setups that for
>> some reason work with specifying fsl,qe-num-snums = 28 and using the
>> existing snum_init_46 array. OTOH, the current code certainly does not work
>> for the MPC8309-based board we're working on, since the first 14 of the
>> elements in snum_init_46 are "Not available on
>> MPC8306/MPC8306S/MPC8309" according to the QUICC Engine Reference
>> Manual (Table 4-30) - and indeed, without this patch (or something to the
>> same effect), we get
> 
> According to the QUICC Engine Reference Manual (Table 4-30), the number of snums used for 
> " MPC8306/MPC8306S/MPC8309" should be 14 instead of 28, so maybe we should assign "fsl,qe-num-snums = 14"
> And define a new snum_init_14 array, meanwhile, modify the minimum value of "num_of_snums" to 14 in function "qe_get_num_of_snums"
> (I mean: " if ((num_of_snums < 14) || (num_of_snums > QE_NUM_OF_SNUM)) {")

Perhaps, but it's completely unclear to me why the code would contain a
comment /* No QE ever has fewer than 28 SNUMs */ when that then appears
to be wrong. It is also far from clear why the snums such as 0x08, 0x09
etc. that appear in table 4-30 without any assigned meaning do appear in
the existing snum_init_46 table - but of course, those elements of that
array don't currently happen to be used when qe_get_num_of_snums==28
(but they do get used for e.g. mpc8569 which specifies fsl,qe-num-snums=46).

Altogether, I think it's a completely wrong approach to pretend that the
_set of valid snums_ is a function of the _number of snums_ alone - the
footnotes to table 4-30 make it abundantly clear that one really cannot
expect such a simple relationship.

So, I suggest deprecating the fsl,qe-num-snums binding, in favor of a
more sensible

fsl,qe-snums = /bits/ 8 <0x88 0x89 0x98 0x99 ...>

which will automatically encode both the number of snums as well as the
concrete values to be used.

Rasmus