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

[Rasmus Villemoes]

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
>>
>> 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 <rasmus.villemoes@xxxxxxxxx>
>> ---
>>  .../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@e0100000 {
>>  	#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