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

[Qiang Zhao]

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.
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