On 28/02/2019 08.14, Qiang Zhao wrote:
> On Tue, Feb 26, 2019 at 2:49 AM Rasmus Villemoes <rasmus.villem...@prevas.dk>
> wrote:
>> -----Original Message-----
>> From: Rasmus Villemoes <rasmus.villem...@prevas.dk>
>> Sent: 2019年2月26日 16:48
>> To: Qiang Zhao <qiang.z...@nxp.com>; Leo Li <leoyang...@nxp.com>
>> Cc: linux-kernel@vger.kernel.org; Valentin Longchamp
>> <valentin.longch...@keymile.com>; Scott Wood <o...@buserror.net>;
>> Rasmus Villemoes <rasmus.villem...@prevas.se>
>> 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
