Re: [PATCH v2] soc: qcom: ocmem: Add OCMEM hardware version print

[Konrad Dybcio]

On 29.05.2023 10:38, Luca Weiss wrote:
> On Freitag, 26. Mai 2023 21:39:44 CEST Konrad Dybcio wrote:
>> On 23.05.2023 22:32, Luca Weiss wrote:
>>> It might be useful to know what hardware version of the OCMEM block the
>>> SoC contains. Add a debug print for that.
>>>
>>> Signed-off-by: Luca Weiss <luca@xxxxxxxxx>
>>> ---
>>> This patch is depends on [0] but could also be applied in the other
>>> order, if conflicts are resolved.
>>>
>>> [0]
>>> https://lore.kernel.org/linux-arm-msm/20230506-msm8226-ocmem-v1-1-3e24e27
>>> 24f01@xxxxxxxxx/ ---
>>> Changes in v2:
>>> - Use FIELD_GET macros for getting correct bits from register (take
>>>
>>>   wording from mdp5: major, minor, step which hopefully is sort of
>>>   accurate)
>>
>> Qualcomm uses that (or that-ish) nomenclature for many hw blocks, so
>> it's likely spot on.
>>
>>> - Link to v1:
>>> https://lore.kernel.org/r/20230509-ocmem-hwver-v1-1-b83ece3f2168@xxxxxxxx
>>> z ---
>>>
>>>  drivers/soc/qcom/ocmem.c | 9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/soc/qcom/ocmem.c b/drivers/soc/qcom/ocmem.c
>>> index c3e78411c637..9f7c3348cbb9 100644
>>> --- a/drivers/soc/qcom/ocmem.c
>>> +++ b/drivers/soc/qcom/ocmem.c
>>> @@ -76,6 +76,10 @@ struct ocmem {
>>>
>>>  #define OCMEM_REG_GFX_MPU_START			0x00001004
>>>  #define OCMEM_REG_GFX_MPU_END			0x00001008
>>>
>>> +#define OCMEM_HW_VERSION_MAJOR(val)		FIELD_GET(GENMASK(31, 28), val)
>>> +#define OCMEM_HW_VERSION_MINOR(val)		FIELD_GET(GENMASK(27, 16), val)
>>> +#define OCMEM_HW_VERSION_STEP(val)		FIELD_GET(GENMASK(15, 0), val)
>>> +
>>>
>>>  #define OCMEM_HW_PROFILE_NUM_PORTS(val)		FIELD_GET(0x0000000f, (val))
>>>  #define OCMEM_HW_PROFILE_NUM_MACROS(val)	FIELD_GET(0x00003f00, (val))
>>>
>>> @@ -355,6 +359,11 @@ static int ocmem_dev_probe(struct platform_device
>>> *pdev)> 
>>>  		}
>>>  	
>>>  	}
>>>
>>> +	reg = ocmem_read(ocmem, OCMEM_REG_HW_VERSION);
>>> +	dev_dbg(dev, "OCMEM hardware version: %ld.%ld.%ld\n",
>>
>> reg is an unsigned long (u32), this should probably be %u
> 
> I can make it %lu, just %u gives me this warning
Right, grumpy compilers care about types that have been meaningless for
longer than I've been alive.. 

Konrad
> 
> drivers/soc/qcom/ocmem.c:363:22: warning: format '%u' expects argument of type 'unsigned int', but argument 6 has type 'long unsigned int' [-Wformat=]
>   363 |         dev_dbg(dev, "OCMEM hardware version: %u.%u.%u\n",
>       |                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> 
> Will send a v3 with that.
> 
>>
>>> +		OCMEM_HW_VERSION_MAJOR(reg), OCMEM_HW_VERSION_MINOR(reg),
>>> +		OCMEM_HW_VERSION_STEP(reg));
>>
>> Nit: one per line would make this the tiniest bit easier to read
> 
> ack
> 
> Regards
> Luca
> 
>>
>> Konrad
>>
>>> +
>>>
>>>  	reg = ocmem_read(ocmem, OCMEM_REG_HW_PROFILE);
>>>  	ocmem->num_ports = OCMEM_HW_PROFILE_NUM_PORTS(reg);
>>>  	ocmem->num_macros = OCMEM_HW_PROFILE_NUM_MACROS(reg);
>>>
>>> ---
>>> base-commit: 8705151771af822ac794b44504cd72eebc423499
>>> change-id: 20230509-ocmem-hwver-99bcb33b028b
>>>
>>> Best regards,
> 
> 
> 
>