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

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

[Jingyi Wang]

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,
-- 
Jingyi Wang <quic_jingyw@quicinc.com>

[PATCH 1/4] dt-bindings: cache: qcom,llcc: add num-banks property

[Jingyi Wang]

Add a property "num-banks" for errata.

Signed-off-by: Jingyi Wang <quic_jingyw@quicinc.com>
---
 Documentation/devicetree/bindings/cache/qcom,llcc.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/cache/qcom,llcc.yaml b/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
index 68ea5f70b75f..03241b719c98 100644
--- a/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
+++ b/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
@@ -56,6 +56,11 @@ properties:
     items:
       - const: multi-chan-ddr
 
+  num-banks:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      The num of llcc banks
+
 required:
   - compatible
   - reg

-- 
2.25.1

[PATCH 2/4] dt-bindings: cache: qcom,llcc: Document the QCS8300 LLCC

[Jingyi Wang]

Document the Last Level Cache Controller on QCS8300 platform.

Signed-off-by: Jingyi Wang <quic_jingyw@quicinc.com>
---
 Documentation/devicetree/bindings/cache/qcom,llcc.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/cache/qcom,llcc.yaml b/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
index 03241b719c98..69d9bcc5614c 100644
--- a/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
+++ b/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
@@ -20,6 +20,7 @@ description: |
 properties:
   compatible:
     enum:
+      - qcom,qcs8300-llcc
       - qcom,qdu1000-llcc
       - qcom,sa8775p-llcc
       - qcom,sc7180-llcc
@@ -170,6 +171,7 @@ allOf:
         compatible:
           contains:
             enum:
+              - qcom,qcs8300-llcc
               - qcom,sdm845-llcc
               - qcom,sm8150-llcc
               - qcom,sm8250-llcc

-- 
2.25.1

[PATCH 3/4] soc: qcom: llcc: add errata to get bank num

[Jingyi Wang]

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;
+	}
 
-	num_banks &= LLCC_LB_CNT_MASK;
-	num_banks >>= LLCC_LB_CNT_SHIFT;
 	drv_data->num_banks = num_banks;
 
 	drv_data->regmaps = devm_kcalloc(dev, num_banks, sizeof(*drv_data->regmaps), GFP_KERNEL);

-- 
2.25.1

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

[Jingyi Wang]

Add llcc configuration support for the QCS8300 platform.

Signed-off-by: Jingyi Wang <quic_jingyw@quicinc.com>
---
 drivers/soc/qcom/llcc-qcom.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
index 3fb45e625d82..571fb6045f25 100644
--- a/drivers/soc/qcom/llcc-qcom.c
+++ b/drivers/soc/qcom/llcc-qcom.c
@@ -422,6 +422,14 @@ static const struct llcc_slice_config sm8650_data[] = {
 	{LLCC_VIDVSP,   28,  256, 3, 1, 0xFFFFFF, 0x0,      0, 0x0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0},
 };
 
+static struct llcc_slice_config qcs8300_data[] =  {
+	{LLCC_GPUHTW,   11, 128, 1, 1, 0x00F, 0x0, 0, 0, 0, 1, 0, 0, 0},
+	{LLCC_GPU,      12, 512, 1, 1, 0x00F, 0x0, 0, 0, 0, 1, 0, 1, 0},
+	{LLCC_MMUHWT,   13, 128, 1, 1, 0x00F, 0x0, 0, 0, 0, 0, 1, 0, 0},
+	{LLCC_ECC,      26, 256, 3, 1, 0x00F, 0x0, 0, 0, 0, 0, 1, 0, 0},
+	{LLCC_WRCACHE,	31, 128, 1, 1, 0x00F, 0x0, 0, 0, 0, 0, 1, 0, 0},
+};
+
 static const struct llcc_slice_config qdu1000_data_2ch[] = {
 	{ LLCC_MDMHPGRW, 7, 512, 1, 1, 0xfff, 0x0, 0, 0, 0, 1, 0, 0, 0 },
 	{ LLCC_MODHW,    9, 256, 1, 1, 0xfff, 0x0, 0, 0, 0, 1, 0, 0, 0 },
@@ -539,6 +547,16 @@ static const u32 llcc_v2_1_reg_offset[] = {
 	[LLCC_COMMON_STATUS0]	= 0x0003400c,
 };
 
+static const struct qcom_llcc_config qcs8300_cfg[] = {
+	{
+		.sct_data	= qcs8300_data,
+		.size		= ARRAY_SIZE(qcs8300_data),
+		.need_llcc_cfg	= true,
+		.reg_offset	= llcc_v2_1_reg_offset,
+		.edac_reg_offset = &llcc_v2_1_edac_reg_offset,
+	},
+};
+
 static const struct qcom_llcc_config qdu1000_cfg[] = {
 	{
 		.sct_data       = qdu1000_data_8ch,
@@ -721,6 +739,11 @@ static const struct qcom_llcc_config x1e80100_cfg[] = {
 	},
 };
 
+static const struct qcom_sct_config qcs8300_cfgs = {
+	.llcc_config	= qcs8300_cfg,
+	.num_config	= ARRAY_SIZE(qcs8300_cfg),
+};
+
 static const struct qcom_sct_config qdu1000_cfgs = {
 	.llcc_config	= qdu1000_cfg,
 	.num_config	= ARRAY_SIZE(qdu1000_cfg),
@@ -1380,6 +1403,7 @@ static int qcom_llcc_probe(struct platform_device *pdev)
 }
 
 static const struct of_device_id qcom_llcc_of_match[] = {
+	{ .compatible = "qcom,qcs8300-llcc", .data = &qcs8300_cfgs},
 	{ .compatible = "qcom,qdu1000-llcc", .data = &qdu1000_cfgs},
 	{ .compatible = "qcom,sa8775p-llcc", .data = &sa8775p_cfgs },
 	{ .compatible = "qcom,sc7180-llcc", .data = &sc7180_cfgs },

-- 
2.25.1

Re: [PATCH 1/4] dt-bindings: cache: qcom,llcc: add num-banks property

[Krzysztof Kozlowski]

On Tue, Sep 03, 2024 at 02:21:29PM +0800, Jingyi Wang wrote:
> Add a property "num-banks" for errata.

This you said in commit subject and we see in the diff. You *MUST*
explain why.

> 
> Signed-off-by: Jingyi Wang <quic_jingyw@quicinc.com>
> ---
>  Documentation/devicetree/bindings/cache/qcom,llcc.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/cache/qcom,llcc.yaml b/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
> index 68ea5f70b75f..03241b719c98 100644
> --- a/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
> +++ b/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
> @@ -56,6 +56,11 @@ properties:
>      items:
>        - const: multi-chan-ddr
>  
> +  num-banks:

No vendor prefix? So this is generic property? Then add to some common
schema with proper explanation WHY.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      The num of llcc banks

And what are llcc (or LLCC?) banks?


Best regards,
Krzysztof

Re: [PATCH 2/4] dt-bindings: cache: qcom,llcc: Document the QCS8300 LLCC

[Krzysztof Kozlowski]

On Tue, Sep 03, 2024 at 02:21:30PM +0800, Jingyi Wang wrote:
> Document the Last Level Cache Controller on QCS8300 platform.
> 
> Signed-off-by: Jingyi Wang <quic_jingyw@quicinc.com>
> ---
>  Documentation/devicetree/bindings/cache/qcom,llcc.yaml | 2 ++
>  1 file changed, 2 insertions(+)

Although this looks incomplete, considering you need other properties
for your variant.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 3/4] soc: qcom: llcc: add errata to get bank num

[Krzysztof Kozlowski]

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

Re: [PATCH 4/4] soc: qcom: llcc: Add llcc configuration support for the QCS8300 platform

[Krzysztof Kozlowski]

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

Re: [PATCH 3/4] soc: qcom: llcc: add errata to get bank num

[Jingyi Wang]

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

Re: [PATCH 4/4] soc: qcom: llcc: Add llcc configuration support for the QCS8300 platform

[Jingyi Wang]

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

Re: [PATCH 3/4] soc: qcom: llcc: add errata to get bank num

[Krzysztof Kozlowski]

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

Re: [PATCH 3/4] soc: qcom: llcc: add errata to get bank num

[Jingyi Wang]

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

Re: [PATCH 1/4] dt-bindings: cache: qcom,llcc: add num-banks property

[Jingyi Wang]

On 9/3/2024 3:10 PM, Krzysztof Kozlowski wrote:
> On Tue, Sep 03, 2024 at 02:21:29PM +0800, Jingyi Wang wrote:
>> Add a property "num-banks" for errata.
> 
> This you said in commit subject and we see in the diff. You *MUST*
> explain why.
> 
>>
>> Signed-off-by: Jingyi Wang <quic_jingyw@quicinc.com>
>> ---
>>  Documentation/devicetree/bindings/cache/qcom,llcc.yaml | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/cache/qcom,llcc.yaml b/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
>> index 68ea5f70b75f..03241b719c98 100644
>> --- a/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
>> +++ b/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
>> @@ -56,6 +56,11 @@ properties:
>>      items:
>>        - const: multi-chan-ddr
>>  
>> +  num-banks:
> 
> No vendor prefix? So this is generic property? Then add to some common
> schema with proper explanation WHY.
> 
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      The num of llcc banks
> 
> And what are llcc (or LLCC?) banks?
> 
> 
Will add the vendor prefix and description in the next series.
> Best regards,
> Krzysztof
> 
Thanks,
Jingyi

Re: [PATCH 1/4] dt-bindings: cache: qcom,llcc: add num-banks property

[Krzysztof Kozlowski]

On 03/09/2024 09:30, Jingyi Wang wrote:
> 
> 
> On 9/3/2024 3:10 PM, Krzysztof Kozlowski wrote:
>> On Tue, Sep 03, 2024 at 02:21:29PM +0800, Jingyi Wang wrote:
>>> Add a property "num-banks" for errata.
>>
>> This you said in commit subject and we see in the diff. You *MUST*
>> explain why.
>>
>>>
>>> Signed-off-by: Jingyi Wang <quic_jingyw@quicinc.com>
>>> ---
>>>  Documentation/devicetree/bindings/cache/qcom,llcc.yaml | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/cache/qcom,llcc.yaml b/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
>>> index 68ea5f70b75f..03241b719c98 100644
>>> --- a/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
>>> +++ b/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
>>> @@ -56,6 +56,11 @@ properties:
>>>      items:
>>>        - const: multi-chan-ddr
>>>  
>>> +  num-banks:
>>
>> No vendor prefix? So this is generic property? Then add to some common
>> schema with proper explanation WHY.
>>
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    description:
>>> +      The num of llcc banks
>>
>> And what are llcc (or LLCC?) banks?
>>
>>
> Will add the vendor prefix and description in the next series.

You did not provide rationale nor answer to concerns so far.

Best regards,
Krzysztof

Re: [PATCH 1/4] dt-bindings: cache: qcom,llcc: add num-banks property

[Jingyi Wang]

On 9/3/2024 4:00 PM, Krzysztof Kozlowski wrote:
> On 03/09/2024 09:30, Jingyi Wang wrote:
>>
>>
>> On 9/3/2024 3:10 PM, Krzysztof Kozlowski wrote:
>>> On Tue, Sep 03, 2024 at 02:21:29PM +0800, Jingyi Wang wrote:
>>>> Add a property "num-banks" for errata.
>>>
>>> This you said in commit subject and we see in the diff. You *MUST*
>>> explain why.
>>>
Usually the num of LLCC banks is read from hardware, but there is errata
on some SoCs to get the wrong bank num from LLCC_COMMON_STATUS0. Add a
property "num-banks" to indicate the accurate data.
>>>>
>>>> Signed-off-by: Jingyi Wang <quic_jingyw@quicinc.com>
>>>> ---
>>>>  Documentation/devicetree/bindings/cache/qcom,llcc.yaml | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/cache/qcom,llcc.yaml b/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
>>>> index 68ea5f70b75f..03241b719c98 100644
>>>> --- a/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
>>>> +++ b/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
>>>> @@ -56,6 +56,11 @@ properties:
>>>>      items:
>>>>        - const: multi-chan-ddr
>>>>  
>>>> +  num-banks:
>>>
>>> No vendor prefix? So this is generic property? Then add to some common
>>> schema with proper explanation WHY.
>>>

will qcom,num-banks be okay?

>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    description:
>>>> +      The num of llcc banks
>>>
>>> And what are llcc (or LLCC?) banks?
>>>
>>>

LLCC banks means LLCC register regions with same memory size and reg offset
and different memory base for LLCC configuration.

>> Will add the vendor prefix and description in the next series.
> 
> You did not provide rationale nor answer to concerns so far.
> 
> Best regards,
> Krzysztof
> 
Thanks,
Jingyi

Re: [PATCH 1/4] dt-bindings: cache: qcom,llcc: add num-banks property

[Krzysztof Kozlowski]

On 03/09/2024 10:38, Jingyi Wang wrote:
> 
> 
> On 9/3/2024 4:00 PM, Krzysztof Kozlowski wrote:
>> On 03/09/2024 09:30, Jingyi Wang wrote:
>>>
>>>
>>> On 9/3/2024 3:10 PM, Krzysztof Kozlowski wrote:
>>>> On Tue, Sep 03, 2024 at 02:21:29PM +0800, Jingyi Wang wrote:
>>>>> Add a property "num-banks" for errata.
>>>>
>>>> This you said in commit subject and we see in the diff. You *MUST*
>>>> explain why.
>>>>
> Usually the num of LLCC banks is read from hardware, but there is errata
> on some SoCs to get the wrong bank num from LLCC_COMMON_STATUS0. Add a
> property "num-banks" to indicate the accurate data.

You have compatible for that. Anyway, your future patch must clearly
explain the problem in details.

Best regards,
Krzysztof

Re: [PATCH 3/4] soc: qcom: llcc: add errata to get bank num

[Konrad Dybcio]

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

Re: [PATCH 3/4] soc: qcom: llcc: add errata to get bank num

[Jingyi Wang]

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