mbox series

[0/4] soc: qcom: llcc: Add llcc support for the QCS8300 platform

Message ID 20240903-qcs8300_llcc_driver-v1-0-228659bdf067@quicinc.com
Headers show
Series soc: qcom: llcc: Add llcc support for the QCS8300 platform | expand

Message

Jingyi Wang Sept. 3, 2024, 6:21 a.m. UTC 
Add llcc support for QCS8300 platform, there is an errata on the QCS8300
SoC to get bank num, define a property "num-banks" in the devicetree node
for errata.

Signed-off-by: Jingyi Wang <quic_jingyw@quicinc.com>
---
The device tree node of QCS8300 LLCC will be submitted in the following
dts changes.

---
Jingyi Wang (4):
      dt-bindings: cache: qcom,llcc: add num-banks property
      dt-bindings: cache: qcom,llcc: Document the QCS8300 LLCC
      soc: qcom: llcc: add errata to get bank num
      soc: qcom: llcc: Add llcc configuration support for the QCS8300 platform

 .../devicetree/bindings/cache/qcom,llcc.yaml       |  7 ++++
 drivers/soc/qcom/llcc-qcom.c                       | 39 +++++++++++++++++++---
 2 files changed, 41 insertions(+), 5 deletions(-)
---
base-commit: eb8c5ca373cbb018a84eb4db25c863302c9b6314
change-id: 20240903-qcs8300_llcc_driver-aa87a0541821

Best regards,

Comments

Krzysztof Kozlowski Sept. 3, 2024, 7:13 a.m. UTC | #1 
On Tue, Sep 03, 2024 at 02:21:31PM +0800, Jingyi Wang wrote:
> Use "num-banks" property to indicate the actual num of banks for
> errata.
> 
> Signed-off-by: Jingyi Wang <quic_jingyw@quicinc.com>
> ---
>  drivers/soc/qcom/llcc-qcom.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
> index 8fa4ffd3a9b5..3fb45e625d82 100644
> --- a/drivers/soc/qcom/llcc-qcom.c
> +++ b/drivers/soc/qcom/llcc-qcom.c
> @@ -1275,12 +1275,17 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>  		goto err;
>  	cfg = &cfgs->llcc_config[cfg_index];
>  
> -	ret = regmap_read(regmap, cfg->reg_offset[LLCC_COMMON_STATUS0], &num_banks);
> -	if (ret)
> -		goto err;
> +	if (unlikely(!of_property_read_u32(dev->of_node, "num-banks", &num_banks))) {

Drop unlikely.

> +		/* errata: get num of llcc banks. */

Huh? What?

> +	} else {
> +		ret = regmap_read(regmap, cfg->reg_offset[LLCC_COMMON_STATUS0], &num_banks);
> +		if (ret)
> +			goto err;

Sorry, but what? You can read it from hardware, but you add DT property?
No, that's just wrong. Why commit msg explains nothing about reasons and
problem you are solving?

Best regards,
Krzysztof
Krzysztof Kozlowski Sept. 3, 2024, 7:14 a.m. UTC | #2 
On Tue, Sep 03, 2024 at 02:21:32PM +0800, Jingyi Wang wrote:
> Add llcc configuration support for the QCS8300 platform.

It is LLCC. Fix it everywhere and create commits using consisting style.
In some subjects you call it LLCC but here llcc...

> 
> Signed-off-by: Jingyi Wang <quic_jingyw@quicinc.com>

Best regards,
Krzysztof
Jingyi Wang Sept. 3, 2024, 7:17 a.m. UTC | #3 
On 9/3/2024 3:13 PM, Krzysztof Kozlowski wrote:
> On Tue, Sep 03, 2024 at 02:21:31PM +0800, Jingyi Wang wrote:
>> Use "num-banks" property to indicate the actual num of banks for
>> errata.
>>
>> Signed-off-by: Jingyi Wang <quic_jingyw@quicinc.com>
>> ---
>>  drivers/soc/qcom/llcc-qcom.c | 15 ++++++++++-----
>>  1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
>> index 8fa4ffd3a9b5..3fb45e625d82 100644
>> --- a/drivers/soc/qcom/llcc-qcom.c
>> +++ b/drivers/soc/qcom/llcc-qcom.c
>> @@ -1275,12 +1275,17 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>>  		goto err;
>>  	cfg = &cfgs->llcc_config[cfg_index];
>>  
>> -	ret = regmap_read(regmap, cfg->reg_offset[LLCC_COMMON_STATUS0], &num_banks);
>> -	if (ret)
>> -		goto err;
>> +	if (unlikely(!of_property_read_u32(dev->of_node, "num-banks", &num_banks))) {
> 
> Drop unlikely.
> 
>> +		/* errata: get num of llcc banks. */
> 
> Huh? What?
> 
>> +	} else {
>> +		ret = regmap_read(regmap, cfg->reg_offset[LLCC_COMMON_STATUS0], &num_banks);
>> +		if (ret)
>> +			goto err;
> 
> Sorry, but what? You can read it from hardware, but you add DT property?
> No, that's just wrong. Why commit msg explains nothing about reasons and
> problem you are solving?
> 
we need the property because there is hardware errata on this SoC, regmap_read get wrong num.
> Best regards,
> Krzysztof
> 
Thanks,
Jingyi
Jingyi Wang Sept. 3, 2024, 7:18 a.m. UTC | #4 
On 9/3/2024 3:14 PM, Krzysztof Kozlowski wrote:
> On Tue, Sep 03, 2024 at 02:21:32PM +0800, Jingyi Wang wrote:
>> Add llcc configuration support for the QCS8300 platform.
> 
> It is LLCC. Fix it everywhere and create commits using consisting style.
> In some subjects you call it LLCC but here llcc...
> 
well noted, will fix that.
>>
>> Signed-off-by: Jingyi Wang <quic_jingyw@quicinc.com>
> 
> Best regards,
> Krzysztof
> 
Thanks,
Jingyi
Krzysztof Kozlowski Sept. 3, 2024, 7:19 a.m. UTC | #5 
On 03/09/2024 09:17, Jingyi Wang wrote:
> 
> 
> On 9/3/2024 3:13 PM, Krzysztof Kozlowski wrote:
>> On Tue, Sep 03, 2024 at 02:21:31PM +0800, Jingyi Wang wrote:
>>> Use "num-banks" property to indicate the actual num of banks for
>>> errata.
>>>
>>> Signed-off-by: Jingyi Wang <quic_jingyw@quicinc.com>
>>> ---
>>>  drivers/soc/qcom/llcc-qcom.c | 15 ++++++++++-----
>>>  1 file changed, 10 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
>>> index 8fa4ffd3a9b5..3fb45e625d82 100644
>>> --- a/drivers/soc/qcom/llcc-qcom.c
>>> +++ b/drivers/soc/qcom/llcc-qcom.c
>>> @@ -1275,12 +1275,17 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>>>  		goto err;
>>>  	cfg = &cfgs->llcc_config[cfg_index];
>>>  
>>> -	ret = regmap_read(regmap, cfg->reg_offset[LLCC_COMMON_STATUS0], &num_banks);
>>> -	if (ret)
>>> -		goto err;
>>> +	if (unlikely(!of_property_read_u32(dev->of_node, "num-banks", &num_banks))) {
>>
>> Drop unlikely.
>>
>>> +		/* errata: get num of llcc banks. */
>>
>> Huh? What?
>>
>>> +	} else {
>>> +		ret = regmap_read(regmap, cfg->reg_offset[LLCC_COMMON_STATUS0], &num_banks);
>>> +		if (ret)
>>> +			goto err;
>>
>> Sorry, but what? You can read it from hardware, but you add DT property?
>> No, that's just wrong. Why commit msg explains nothing about reasons and
>> problem you are solving?
>>
> we need the property because there is hardware errata on this SoC, regmap_read get wrong num.

That's what compatible is for.

Anyway, you did not explain the problem but just send some code. All
your patches in this and all future submissions must explain why you are
doing this. What you are fixing, why you are introducing something.

Best regards,
Krzysztof
Jingyi Wang Sept. 3, 2024, 7:28 a.m. UTC | #6 
On 9/3/2024 3:19 PM, Krzysztof Kozlowski wrote:
> On 03/09/2024 09:17, Jingyi Wang wrote:
>>
>>
>> On 9/3/2024 3:13 PM, Krzysztof Kozlowski wrote:
>>> On Tue, Sep 03, 2024 at 02:21:31PM +0800, Jingyi Wang wrote:
>>>> Use "num-banks" property to indicate the actual num of banks for
>>>> errata.
>>>>
>>>> Signed-off-by: Jingyi Wang <quic_jingyw@quicinc.com>
>>>> ---
>>>>  drivers/soc/qcom/llcc-qcom.c | 15 ++++++++++-----
>>>>  1 file changed, 10 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
>>>> index 8fa4ffd3a9b5..3fb45e625d82 100644
>>>> --- a/drivers/soc/qcom/llcc-qcom.c
>>>> +++ b/drivers/soc/qcom/llcc-qcom.c
>>>> @@ -1275,12 +1275,17 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>>>>  		goto err;
>>>>  	cfg = &cfgs->llcc_config[cfg_index];
>>>>  
>>>> -	ret = regmap_read(regmap, cfg->reg_offset[LLCC_COMMON_STATUS0], &num_banks);
>>>> -	if (ret)
>>>> -		goto err;
>>>> +	if (unlikely(!of_property_read_u32(dev->of_node, "num-banks", &num_banks))) {
>>>
>>> Drop unlikely.
>>>
>>>> +		/* errata: get num of llcc banks. */
>>>
>>> Huh? What?
>>>
>>>> +	} else {
>>>> +		ret = regmap_read(regmap, cfg->reg_offset[LLCC_COMMON_STATUS0], &num_banks);
>>>> +		if (ret)
>>>> +			goto err;
>>>
>>> Sorry, but what? You can read it from hardware, but you add DT property?
>>> No, that's just wrong. Why commit msg explains nothing about reasons and
>>> problem you are solving?
>>>
>> we need the property because there is hardware errata on this SoC, regmap_read get wrong num.
> 
> That's what compatible is for.
> 
> Anyway, you did not explain the problem but just send some code. All
> your patches in this and all future submissions must explain why you are
> doing this. What you are fixing, why you are introducing something.
> 
Sure, thanks for remind.
> Best regards,
> Krzysztof
> 
Thanks,
Jingyi
Konrad Dybcio Sept. 5, 2024, 1:29 p.m. UTC | #7 
On 3.09.2024 8:21 AM, Jingyi Wang wrote:
> Use "num-banks" property to indicate the actual num of banks for
> errata.
> 
> Signed-off-by: Jingyi Wang <quic_jingyw@quicinc.com>
> ---
>  drivers/soc/qcom/llcc-qcom.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
> index 8fa4ffd3a9b5..3fb45e625d82 100644
> --- a/drivers/soc/qcom/llcc-qcom.c
> +++ b/drivers/soc/qcom/llcc-qcom.c
> @@ -1275,12 +1275,17 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>  		goto err;
>  	cfg = &cfgs->llcc_config[cfg_index];
>  
> -	ret = regmap_read(regmap, cfg->reg_offset[LLCC_COMMON_STATUS0], &num_banks);
> -	if (ret)
> -		goto err;
> +	if (unlikely(!of_property_read_u32(dev->of_node, "num-banks", &num_banks))) {
> +		/* errata: get num of llcc banks. */
> +	} else {
> +		ret = regmap_read(regmap, cfg->reg_offset[LLCC_COMMON_STATUS0], &num_banks);
> +		if (ret)
> +			goto err;
> +
> +		num_banks &= LLCC_LB_CNT_MASK;
> +		num_banks >>= LLCC_LB_CNT_SHIFT;
> +	}

Is num-banks going to be populated by the bootloader, or hardcoded?

If the latter, we may just do so in the driver, hoping no more SoCs
have this erratum..

Konrad
Jingyi Wang Sept. 6, 2024, 5:43 a.m. UTC | #8 
Hi Konrad,

On 9/5/2024 9:29 PM, Konrad Dybcio wrote:
> On 3.09.2024 8:21 AM, Jingyi Wang wrote:
>> Use "num-banks" property to indicate the actual num of banks for
>> errata.
>>
>> Signed-off-by: Jingyi Wang <quic_jingyw@quicinc.com>
>> ---
>>  drivers/soc/qcom/llcc-qcom.c | 15 ++++++++++-----
>>  1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
>> index 8fa4ffd3a9b5..3fb45e625d82 100644
>> --- a/drivers/soc/qcom/llcc-qcom.c
>> +++ b/drivers/soc/qcom/llcc-qcom.c
>> @@ -1275,12 +1275,17 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>>  		goto err;
>>  	cfg = &cfgs->llcc_config[cfg_index];
>>  
>> -	ret = regmap_read(regmap, cfg->reg_offset[LLCC_COMMON_STATUS0], &num_banks);
>> -	if (ret)
>> -		goto err;
>> +	if (unlikely(!of_property_read_u32(dev->of_node, "num-banks", &num_banks))) {
>> +		/* errata: get num of llcc banks. */
>> +	} else {
>> +		ret = regmap_read(regmap, cfg->reg_offset[LLCC_COMMON_STATUS0], &num_banks);
>> +		if (ret)
>> +			goto err;
>> +
>> +		num_banks &= LLCC_LB_CNT_MASK;
>> +		num_banks >>= LLCC_LB_CNT_SHIFT;
>> +	}
> 
> Is num-banks going to be populated by the bootloader, or hardcoded?
> 
> If the latter, we may just do so in the driver, hoping no more SoCs
> have this erratum..
> 
> Konrad

We would like modify that in driver instead of bootloader, so you suggestion
is hardcode it in struct like "qcom_llcc_config" instead of adding property
in devicetree? Please correct my if there is misunderstanding.

Thanks,
Jingyi