[PATCH 00/12] Add MP25 FMC2 support

[PATCH 00/12] Add MP25 FMC2 support

[Christophe Kerello]

Add MP25 SOC support in stm32_fmc2 drivers:
 - Update stm32-fmc2-ebi driver to support FMC2 revision 2 and MP25 SOC.
 - Update stm32_fmc2_nand driver to support FMC2 revision 2 and MP25 SOC.

Christophe Kerello (11):
  dt-bindings: memory-controller: st,stm32: add MP25 support
  memory: stm32-fmc2-ebi: add a platform data structure
  memory: stm32-fmc2-ebi: add MP25 support
  memory: stm32-fmc2-ebi: update the driver to support revision 2
  memory: stm32-fmc2-ebi: add RIF support
  memory: stm32-fmc2-ebi: add runtime PM support
  dt-bindings: mtd: st,stm32: add MP25 support
  mtd: rawnand: stm32_fmc2: use dma_get_slave_caps to get DMA max burst
  mtd: rawnand: stm32_fmc2: add a platform data structure
  mtd: rawnand: stm32_fmc2: add MP25 support
  mtd: rawnand: stm32_fmc2: update the driver to support revision 2

Patrick Delaunay (1):
  dt-bindings: memory-controller: st,stm32: add 'power-domains' property

 .../memory-controllers/st,stm32-fmc2-ebi.yaml |   7 +-
 .../bindings/mtd/st,stm32-fmc2-nand.yaml      |  58 ++-
 drivers/memory/stm32-fmc2-ebi.c               | 445 ++++++++++++++++--
 drivers/mtd/nand/raw/stm32_fmc2_nand.c        | 108 ++++-
 4 files changed, 547 insertions(+), 71 deletions(-)

-- 
2.25.1

[PATCH 01/12] dt-bindings: memory-controller: st,stm32: add MP25 support

[Christophe Kerello]

Add a new compatible string to support MP25 SOC.

Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
---
 .../bindings/memory-controllers/st,stm32-fmc2-ebi.yaml        | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/memory-controllers/st,stm32-fmc2-ebi.yaml b/Documentation/devicetree/bindings/memory-controllers/st,stm32-fmc2-ebi.yaml
index 14f1833d37c9..12e6afeceffd 100644
--- a/Documentation/devicetree/bindings/memory-controllers/st,stm32-fmc2-ebi.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/st,stm32-fmc2-ebi.yaml
@@ -23,7 +23,9 @@ maintainers:
 
 properties:
   compatible:
-    const: st,stm32mp1-fmc2-ebi
+    enum:
+      - st,stm32mp1-fmc2-ebi
+      - st,stm32mp25-fmc2-ebi
 
   reg:
     maxItems: 1
-- 
2.25.1

Re: [PATCH 01/12] dt-bindings: memory-controller: st,stm32: add MP25 support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1074 bytes --]

On Mon, Feb 12, 2024 at 06:48:11PM +0100, Christophe Kerello wrote:
> Add a new compatible string to support MP25 SOC.

You're missing an explanation here as to why this mp25 is not compatible
with the mp1.

Cheers,
Conor.

> 
> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
> ---
>  .../bindings/memory-controllers/st,stm32-fmc2-ebi.yaml        | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/st,stm32-fmc2-ebi.yaml b/Documentation/devicetree/bindings/memory-controllers/st,stm32-fmc2-ebi.yaml
> index 14f1833d37c9..12e6afeceffd 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/st,stm32-fmc2-ebi.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/st,stm32-fmc2-ebi.yaml
> @@ -23,7 +23,9 @@ maintainers:
>  
>  properties:
>    compatible:
> -    const: st,stm32mp1-fmc2-ebi
> +    enum:
> +      - st,stm32mp1-fmc2-ebi
> +      - st,stm32mp25-fmc2-ebi
>  
>    reg:
>      maxItems: 1
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 01/12] dt-bindings: memory-controller: st,stm32: add MP25 support

[Christophe Kerello]

On 2/12/24 19:30, Conor Dooley wrote:
> On Mon, Feb 12, 2024 at 06:48:11PM +0100, Christophe Kerello wrote:
>> Add a new compatible string to support MP25 SOC.
> 
> You're missing an explanation here as to why this mp25 is not compatible
> with the mp1.
> 
> Cheers,
> Conor.
> 

Hi Conor,

On MP1 SoC, RNB signal (NAND controller signal) and NWAIT signal (PSRAM
controler signal) have been integrated together in the SoC. That means
that the NAND controller and the PSRAM controller (if the signal is
used) can not be used at the same time. On MP25 SoC, the 2 signals can
be used outside the SoC, so there is no more restrictions.

MP1 SoC also embeds revision 1.1 of the FMC2 IP when MP25 SoC embeds 
revision 2.0 of the FMC2 IP.

I will add this explanation in the commit message.

Regards,
Christophe Kerello.

>>
>> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
>> ---
>>   .../bindings/memory-controllers/st,stm32-fmc2-ebi.yaml        | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/memory-controllers/st,stm32-fmc2-ebi.yaml b/Documentation/devicetree/bindings/memory-controllers/st,stm32-fmc2-ebi.yaml
>> index 14f1833d37c9..12e6afeceffd 100644
>> --- a/Documentation/devicetree/bindings/memory-controllers/st,stm32-fmc2-ebi.yaml
>> +++ b/Documentation/devicetree/bindings/memory-controllers/st,stm32-fmc2-ebi.yaml
>> @@ -23,7 +23,9 @@ maintainers:
>>   
>>   properties:
>>     compatible:
>> -    const: st,stm32mp1-fmc2-ebi
>> +    enum:
>> +      - st,stm32mp1-fmc2-ebi
>> +      - st,stm32mp25-fmc2-ebi
>>   
>>     reg:
>>       maxItems: 1
>> -- 
>> 2.25.1
>>

[PATCH 02/12] dt-bindings: memory-controller: st,stm32: add 'power-domains' property

[Christophe Kerello]

From: Patrick Delaunay <patrick.delaunay@foss.st.com>

On STM32MP25 SOC, STM32 FMC2 memory controller is in a power domain.
Allow a single 'power-domains' entry for STM32 FMC2.

Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
---
 .../bindings/memory-controllers/st,stm32-fmc2-ebi.yaml         | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/memory-controllers/st,stm32-fmc2-ebi.yaml b/Documentation/devicetree/bindings/memory-controllers/st,stm32-fmc2-ebi.yaml
index 12e6afeceffd..84ac6f50a6fc 100644
--- a/Documentation/devicetree/bindings/memory-controllers/st,stm32-fmc2-ebi.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/st,stm32-fmc2-ebi.yaml
@@ -36,6 +36,9 @@ properties:
   resets:
     maxItems: 1
 
+  power-domains:
+    maxItems: 1
+
   "#address-cells":
     const: 2
 
-- 
2.25.1

Re: [PATCH 02/12] dt-bindings: memory-controller: st,stm32: add 'power-domains' property

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1345 bytes --]

On Mon, Feb 12, 2024 at 06:48:12PM +0100, Christophe Kerello wrote:
> From: Patrick Delaunay <patrick.delaunay@foss.st.com>
> 
> On STM32MP25 SOC, STM32 FMC2 memory controller is in a power domain.
> Allow a single 'power-domains' entry for STM32 FMC2.

This should be squashed with patch 1, since they both modify the same
file and this power-domain is part of the addition of mp25 support.

If the mp1 doesn't have power domains, shouldn't you constrain the
property to mp25 only?

Cheers,
Conor.

> 
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
> ---
>  .../bindings/memory-controllers/st,stm32-fmc2-ebi.yaml         | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/st,stm32-fmc2-ebi.yaml b/Documentation/devicetree/bindings/memory-controllers/st,stm32-fmc2-ebi.yaml
> index 12e6afeceffd..84ac6f50a6fc 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/st,stm32-fmc2-ebi.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/st,stm32-fmc2-ebi.yaml
> @@ -36,6 +36,9 @@ properties:
>    resets:
>      maxItems: 1
>  
> +  power-domains:
> +    maxItems: 1
> +
>    "#address-cells":
>      const: 2
>  
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 02/12] dt-bindings: memory-controller: st,stm32: add 'power-domains' property

[Christophe Kerello]

On 2/12/24 19:33, Conor Dooley wrote:
> On Mon, Feb 12, 2024 at 06:48:12PM +0100, Christophe Kerello wrote:
>> From: Patrick Delaunay <patrick.delaunay@foss.st.com>
>>
>> On STM32MP25 SOC, STM32 FMC2 memory controller is in a power domain.
>> Allow a single 'power-domains' entry for STM32 FMC2.
> 
> This should be squashed with patch 1, since they both modify the same
> file and this power-domain is part of the addition of mp25 support.

Hi Conor,

Ok, I will squash this patch with patch 1.

> 
> If the mp1 doesn't have power domains, shouldn't you constrain the
> property to mp25 only?
> 

As this property is optional, I do not see the need to constrain the
property to MP25 only, but if you think that it should be the case, I
will do it.

Regards,
Christophe Kerello.

> Cheers,
> Conor.
> >>
>> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
>> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
>> ---
>>   .../bindings/memory-controllers/st,stm32-fmc2-ebi.yaml         | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/memory-controllers/st,stm32-fmc2-ebi.yaml b/Documentation/devicetree/bindings/memory-controllers/st,stm32-fmc2-ebi.yaml
>> index 12e6afeceffd..84ac6f50a6fc 100644
>> --- a/Documentation/devicetree/bindings/memory-controllers/st,stm32-fmc2-ebi.yaml
>> +++ b/Documentation/devicetree/bindings/memory-controllers/st,stm32-fmc2-ebi.yaml
>> @@ -36,6 +36,9 @@ properties:
>>     resets:
>>       maxItems: 1
>>   
>> +  power-domains:
>> +    maxItems: 1
>> +
>>     "#address-cells":
>>       const: 2
>>   
>> -- 
>> 2.25.1
>>
>>
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 02/12] dt-bindings: memory-controller: st,stm32: add 'power-domains' property

[Krzysztof Kozlowski]

On 13/02/2024 11:57, Christophe Kerello wrote:
> 
> 
> On 2/12/24 19:33, Conor Dooley wrote:
>> On Mon, Feb 12, 2024 at 06:48:12PM +0100, Christophe Kerello wrote:
>>> From: Patrick Delaunay <patrick.delaunay@foss.st.com>
>>>
>>> On STM32MP25 SOC, STM32 FMC2 memory controller is in a power domain.
>>> Allow a single 'power-domains' entry for STM32 FMC2.
>>
>> This should be squashed with patch 1, since they both modify the same
>> file and this power-domain is part of the addition of mp25 support.
> 
> Hi Conor,
> 
> Ok, I will squash this patch with patch 1.
> 
>>
>> If the mp1 doesn't have power domains, shouldn't you constrain the
>> property to mp25 only?
>>
> 
> As this property is optional, I do not see the need to constrain the
> property to MP25 only, but if you think that it should be the case, I
> will do it.

The question is: is this property valid for the old/existing variant?

Best regards,
Krzysztof

Re: [PATCH 02/12] dt-bindings: memory-controller: st,stm32: add 'power-domains' property

[Christophe Kerello]

On 2/13/24 12:57, Krzysztof Kozlowski wrote:
> On 13/02/2024 11:57, Christophe Kerello wrote:
>>
>>
>> On 2/12/24 19:33, Conor Dooley wrote:
>>> On Mon, Feb 12, 2024 at 06:48:12PM +0100, Christophe Kerello wrote:
>>>> From: Patrick Delaunay <patrick.delaunay@foss.st.com>
>>>>
>>>> On STM32MP25 SOC, STM32 FMC2 memory controller is in a power domain.
>>>> Allow a single 'power-domains' entry for STM32 FMC2.
>>>
>>> This should be squashed with patch 1, since they both modify the same
>>> file and this power-domain is part of the addition of mp25 support.
>>
>> Hi Conor,
>>
>> Ok, I will squash this patch with patch 1.
>>
>>>
>>> If the mp1 doesn't have power domains, shouldn't you constrain the
>>> property to mp25 only?
>>>
>>
>> As this property is optional, I do not see the need to constrain the
>> property to MP25 only, but if you think that it should be the case, I
>> will do it.
> 
> The question is: is this property valid for the old/existing variant?
> 

Hi Krzysztof,

It is not currently valid but there is a plan to move MP1 on PSCI 
OS-initiated.

Regards,
Christophe Kerello.

> Best regards,
> Krzysztof
> 

Re: [PATCH 02/12] dt-bindings: memory-controller: st,stm32: add 'power-domains' property

[Krzysztof Kozlowski]

On 13/02/2024 16:57, Christophe Kerello wrote:
> 
> 
> On 2/13/24 12:57, Krzysztof Kozlowski wrote:
>> On 13/02/2024 11:57, Christophe Kerello wrote:
>>>
>>>
>>> On 2/12/24 19:33, Conor Dooley wrote:
>>>> On Mon, Feb 12, 2024 at 06:48:12PM +0100, Christophe Kerello wrote:
>>>>> From: Patrick Delaunay <patrick.delaunay@foss.st.com>
>>>>>
>>>>> On STM32MP25 SOC, STM32 FMC2 memory controller is in a power domain.
>>>>> Allow a single 'power-domains' entry for STM32 FMC2.
>>>>
>>>> This should be squashed with patch 1, since they both modify the same
>>>> file and this power-domain is part of the addition of mp25 support.
>>>
>>> Hi Conor,
>>>
>>> Ok, I will squash this patch with patch 1.
>>>
>>>>
>>>> If the mp1 doesn't have power domains, shouldn't you constrain the
>>>> property to mp25 only?
>>>>
>>>
>>> As this property is optional, I do not see the need to constrain the
>>> property to MP25 only, but if you think that it should be the case, I
>>> will do it.
>>
>> The question is: is this property valid for the old/existing variant?
>>
> 
> Hi Krzysztof,
> 
> It is not currently valid but there is a plan to move MP1 on PSCI 
> OS-initiated.

OK

Best regards,
Krzysztof

[PATCH 03/12] memory: stm32-fmc2-ebi: add a platform data structure

[Christophe Kerello]

Before the introduction of MP25 support, let's use a platform data
structure for parameters that will differ.

The MP1 SOCs have only one signal to manage all the controllers (NWAIT).
The MP25 SOC has one RNB signal for the NAND controller and one NWAIT
signal for the memory controller.

Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
---
 drivers/memory/stm32-fmc2-ebi.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/memory/stm32-fmc2-ebi.c b/drivers/memory/stm32-fmc2-ebi.c
index 47d0ea5f1616..5f82686689ee 100644
--- a/drivers/memory/stm32-fmc2-ebi.c
+++ b/drivers/memory/stm32-fmc2-ebi.c
@@ -132,10 +132,15 @@ enum stm32_fmc2_ebi_cpsize {
 	FMC2_CPSIZE_1024 = 1024
 };
 
+struct stm32_fmc2_ebi_data {
+	bool rnb_for_nand;
+};
+
 struct stm32_fmc2_ebi {
 	struct device *dev;
 	struct clk *clk;
 	struct regmap *regmap;
+	const struct stm32_fmc2_ebi_data *data;
 	u8 bank_assigned;
 
 	u32 bcr[FMC2_MAX_EBI_CE];
@@ -988,6 +993,9 @@ static bool stm32_fmc2_ebi_nwait_used_by_ctrls(struct stm32_fmc2_ebi *ebi)
 	unsigned int cs;
 	u32 bcr;
 
+	if (ebi->data->rnb_for_nand)
+		return false;
+
 	for (cs = 0; cs < FMC2_MAX_EBI_CE; cs++) {
 		if (!(ebi->bank_assigned & BIT(cs)))
 			continue;
@@ -1108,6 +1116,10 @@ static int stm32_fmc2_ebi_probe(struct platform_device *pdev)
 
 	ebi->dev = dev;
 
+	ebi->data = of_device_get_match_data(dev);
+	if (!ebi->data)
+		return -EINVAL;
+
 	ebi->regmap = device_node_to_regmap(dev->of_node);
 	if (IS_ERR(ebi->regmap))
 		return PTR_ERR(ebi->regmap);
@@ -1187,8 +1199,15 @@ static int __maybe_unused stm32_fmc2_ebi_resume(struct device *dev)
 static SIMPLE_DEV_PM_OPS(stm32_fmc2_ebi_pm_ops, stm32_fmc2_ebi_suspend,
 			 stm32_fmc2_ebi_resume);
 
+static const struct stm32_fmc2_ebi_data stm32_fmc2_ebi_mp1_data = {
+	.rnb_for_nand = false,
+};
+
 static const struct of_device_id stm32_fmc2_ebi_match[] = {
-	{.compatible = "st,stm32mp1-fmc2-ebi"},
+	{
+		.compatible = "st,stm32mp1-fmc2-ebi",
+		.data = &stm32_fmc2_ebi_mp1_data,
+	},
 	{}
 };
 MODULE_DEVICE_TABLE(of, stm32_fmc2_ebi_match);
-- 
2.25.1

[PATCH 04/12] memory: stm32-fmc2-ebi: add MP25 support

[Christophe Kerello]

Add MP25 SOC support. RNB and NWAIT signals are differentiated.

Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
---
 drivers/memory/stm32-fmc2-ebi.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/memory/stm32-fmc2-ebi.c b/drivers/memory/stm32-fmc2-ebi.c
index 5f82686689ee..d79dcb6c239a 100644
--- a/drivers/memory/stm32-fmc2-ebi.c
+++ b/drivers/memory/stm32-fmc2-ebi.c
@@ -1203,11 +1203,19 @@ static const struct stm32_fmc2_ebi_data stm32_fmc2_ebi_mp1_data = {
 	.rnb_for_nand = false,
 };
 
+static const struct stm32_fmc2_ebi_data stm32_fmc2_ebi_mp25_data = {
+	.rnb_for_nand = true,
+};
+
 static const struct of_device_id stm32_fmc2_ebi_match[] = {
 	{
 		.compatible = "st,stm32mp1-fmc2-ebi",
 		.data = &stm32_fmc2_ebi_mp1_data,
 	},
+	{
+		.compatible = "st,stm32mp25-fmc2-ebi",
+		.data = &stm32_fmc2_ebi_mp25_data,
+	},
 	{}
 };
 MODULE_DEVICE_TABLE(of, stm32_fmc2_ebi_match);
-- 
2.25.1

Re: [PATCH 04/12] memory: stm32-fmc2-ebi: add MP25 support

[Krzysztof Kozlowski]

On 12/02/2024 18:48, Christophe Kerello wrote:
> Add MP25 SOC support. RNB and NWAIT signals are differentiated.

s/SOC/SoC/
everywhere

> 

The way you split patches is a bit odd. Shall we understand that this
patch is the complete MP25 SoC support?

Best regards,
Krzysztof

Re: [PATCH 04/12] memory: stm32-fmc2-ebi: add MP25 support

[Christophe Kerello]

On 2/13/24 08:36, Krzysztof Kozlowski wrote:
> On 12/02/2024 18:48, Christophe Kerello wrote:
>> Add MP25 SOC support. RNB and NWAIT signals are differentiated.
>

Hi Krzysztof,

> s/SOC/SoC/
> everywhere

Ok.

> 
>>
> 
> The way you split patches is a bit odd. Shall we understand that this
> patch is the complete MP25 SoC support?
> 

No, it is not the full support but the way the FMC2 IP has been
integrated in this SoC. As patch 5 has to be written, I am going to
merge this patch and patch 5 in one patch. I am going to follow your
recommendation and use the platform data to distinguish between each
variant.

Regards,
Christophe Kerello.

> Best regards,
> Krzysztof
> 

[PATCH 05/12] memory: stm32-fmc2-ebi: update the driver to support revision 2

[Christophe Kerello]

Add the support of the revision 2 of FMC2 IP.
 - PCSCNTR register has been removed,
 - CFGR register has been added,
 - the bit used to enable the IP has moved from BCR1 to CFGR,
 - the timeout for CEx deassertion has moved from PCSCNTR to BCRx,
 - the continuous clock enable has moved from BCR1 to CFGR,
 - the clk divide ratio has moved from BCR1 to CFGR.

Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
---
 drivers/memory/stm32-fmc2-ebi.c | 206 +++++++++++++++++++++++++-------
 1 file changed, 163 insertions(+), 43 deletions(-)

diff --git a/drivers/memory/stm32-fmc2-ebi.c b/drivers/memory/stm32-fmc2-ebi.c
index d79dcb6c239a..066722274a45 100644
--- a/drivers/memory/stm32-fmc2-ebi.c
+++ b/drivers/memory/stm32-fmc2-ebi.c
@@ -20,8 +20,10 @@
 #define FMC2_BCR(x)			((x) * 0x8 + FMC2_BCR1)
 #define FMC2_BTR(x)			((x) * 0x8 + FMC2_BTR1)
 #define FMC2_PCSCNTR			0x20
+#define FMC2_CFGR			0x20
 #define FMC2_BWTR1			0x104
 #define FMC2_BWTR(x)			((x) * 0x8 + FMC2_BWTR1)
+#define FMC2_VERR			0x3f4
 
 /* Register: FMC2_BCR1 */
 #define FMC2_BCR1_CCLKEN		BIT(20)
@@ -42,6 +44,7 @@
 #define FMC2_BCR_ASYNCWAIT		BIT(15)
 #define FMC2_BCR_CPSIZE			GENMASK(18, 16)
 #define FMC2_BCR_CBURSTRW		BIT(19)
+#define FMC2_BCR_CSCOUNT		GENMASK(21, 20)
 #define FMC2_BCR_NBLSET			GENMASK(23, 22)
 
 /* Register: FMC2_BTRx/FMC2_BWTRx */
@@ -58,6 +61,15 @@
 #define FMC2_PCSCNTR_CSCOUNT		GENMASK(15, 0)
 #define FMC2_PCSCNTR_CNTBEN(x)		BIT((x) + 16)
 
+/* Register: FMC2_CFGR */
+#define FMC2_CFGR_CLKDIV		GENMASK(19, 16)
+#define FMC2_CFGR_CCLKEN		BIT(20)
+#define FMC2_CFGR_FMC2EN		BIT(31)
+
+/* Register: FMC2_VERR */
+#define FMC2_VERR_MAJREV		GENMASK(7, 4)
+#define FMC2_VERR_MAJREV_2		2
+
 #define FMC2_MAX_EBI_CE			4
 #define FMC2_MAX_BANKS			5
 
@@ -74,6 +86,11 @@
 #define FMC2_BCR_MTYP_PSRAM		0x1
 #define FMC2_BCR_MTYP_NOR		0x2
 
+#define FMC2_BCR_CSCOUNT_0		0x0
+#define FMC2_BCR_CSCOUNT_1		0x1
+#define FMC2_BCR_CSCOUNT_64		0x2
+#define FMC2_BCR_CSCOUNT_256		0x3
+
 #define FMC2_BXTR_EXTMOD_A		0x0
 #define FMC2_BXTR_EXTMOD_B		0x1
 #define FMC2_BXTR_EXTMOD_C		0x2
@@ -85,7 +102,7 @@
 #define FMC2_BXTR_DATAST_MAX		0xff
 #define FMC2_BXTR_BUSTURN_MAX		0xf
 #define FMC2_BXTR_DATAHLD_MAX		0x3
-#define FMC2_BTR_CLKDIV_MAX		0xf
+#define FMC2_REG_CLKDIV_MAX		0xf
 #define FMC2_BTR_DATLAT_MAX		0xf
 #define FMC2_PCSCNTR_CSCOUNT_MAX	0xff
 
@@ -101,7 +118,8 @@ enum stm32_fmc2_ebi_register_type {
 	FMC2_REG_BCR = 1,
 	FMC2_REG_BTR,
 	FMC2_REG_BWTR,
-	FMC2_REG_PCSCNTR
+	FMC2_REG_PCSCNTR,
+	FMC2_REG_CFGR,
 };
 
 enum stm32_fmc2_ebi_transaction_type {
@@ -132,6 +150,13 @@ enum stm32_fmc2_ebi_cpsize {
 	FMC2_CPSIZE_1024 = 1024
 };
 
+enum stm32_fmc2_ebi_cscount {
+	FMC2_CSCOUNT_0 = 0,
+	FMC2_CSCOUNT_1 = 1,
+	FMC2_CSCOUNT_64 = 64,
+	FMC2_CSCOUNT_256 = 256
+};
+
 struct stm32_fmc2_ebi_data {
 	bool rnb_for_nand;
 };
@@ -142,11 +167,13 @@ struct stm32_fmc2_ebi {
 	struct regmap *regmap;
 	const struct stm32_fmc2_ebi_data *data;
 	u8 bank_assigned;
+	u8 majrev;
 
 	u32 bcr[FMC2_MAX_EBI_CE];
 	u32 btr[FMC2_MAX_EBI_CE];
 	u32 bwtr[FMC2_MAX_EBI_CE];
 	u32 pcscntr;
+	u32 cfgr;
 };
 
 /*
@@ -274,15 +301,29 @@ static int stm32_fmc2_ebi_check_clk_period(struct stm32_fmc2_ebi *ebi,
 					   const struct stm32_fmc2_prop *prop,
 					   int cs)
 {
-	u32 bcr, bcr1;
+	u32 bcr, cfgr;
 
 	regmap_read(ebi->regmap, FMC2_BCR(cs), &bcr);
-	if (cs)
-		regmap_read(ebi->regmap, FMC2_BCR1, &bcr1);
-	else
-		bcr1 = bcr;
 
-	if (bcr & FMC2_BCR_BURSTEN && (!cs || !(bcr1 & FMC2_BCR1_CCLKEN)))
+	if (ebi->majrev < FMC2_VERR_MAJREV_2) {
+		u32 bcr1;
+
+		if (cs)
+			regmap_read(ebi->regmap, FMC2_BCR1, &bcr1);
+		else
+			bcr1 = bcr;
+
+		if (bcr & FMC2_BCR_BURSTEN &&
+		    (!cs || !(bcr1 & FMC2_BCR1_CCLKEN)))
+			return 0;
+
+		return -EINVAL;
+	}
+
+	regmap_read(ebi->regmap, FMC2_CFGR, &cfgr);
+
+	if (bcr & FMC2_BCR_BURSTEN &&
+	    (!cs || !(cfgr & FMC2_CFGR_CCLKEN)))
 		return 0;
 
 	return -EINVAL;
@@ -311,15 +352,29 @@ static u32 stm32_fmc2_ebi_ns_to_clk_period(struct stm32_fmc2_ebi *ebi,
 					   int cs, u32 setup)
 {
 	u32 nb_clk_cycles = stm32_fmc2_ebi_ns_to_clock_cycles(ebi, cs, setup);
-	u32 bcr, btr, clk_period;
+	u32 btr, clk_period;
 
-	regmap_read(ebi->regmap, FMC2_BCR1, &bcr);
-	if (bcr & FMC2_BCR1_CCLKEN || !cs)
-		regmap_read(ebi->regmap, FMC2_BTR1, &btr);
-	else
-		regmap_read(ebi->regmap, FMC2_BTR(cs), &btr);
+	if (ebi->majrev < FMC2_VERR_MAJREV_2) {
+		u32 bcr;
 
-	clk_period = FIELD_GET(FMC2_BTR_CLKDIV, btr) + 1;
+		regmap_read(ebi->regmap, FMC2_BCR1, &bcr);
+		if (bcr & FMC2_BCR1_CCLKEN || !cs)
+			regmap_read(ebi->regmap, FMC2_BTR1, &btr);
+		else
+			regmap_read(ebi->regmap, FMC2_BTR(cs), &btr);
+
+		clk_period = FIELD_GET(FMC2_BTR_CLKDIV, btr) + 1;
+	} else {
+		u32 cfgr;
+
+		regmap_read(ebi->regmap, FMC2_CFGR, &cfgr);
+		if (cfgr & FMC2_CFGR_CCLKEN) {
+			clk_period = FIELD_GET(FMC2_CFGR_CLKDIV, cfgr) + 1;
+		} else {
+			regmap_read(ebi->regmap, FMC2_BTR(cs), &btr);
+			clk_period = FIELD_GET(FMC2_BTR_CLKDIV, btr) + 1;
+		}
+	}
 
 	return DIV_ROUND_UP(nb_clk_cycles, clk_period);
 }
@@ -339,6 +394,9 @@ static int stm32_fmc2_ebi_get_reg(int reg_type, int cs, u32 *reg)
 	case FMC2_REG_PCSCNTR:
 		*reg = FMC2_PCSCNTR;
 		break;
+	case FMC2_REG_CFGR:
+		*reg = FMC2_CFGR;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -672,10 +730,26 @@ static int stm32_fmc2_ebi_set_clk_period(struct stm32_fmc2_ebi *ebi,
 					 int cs, u32 setup)
 {
 	u32 val;
+	u32 reg = FMC2_BTR(cs);
+	u32 mask = FMC2_BTR_CLKDIV;
 
-	val = setup ? clamp_val(setup - 1, 1, FMC2_BTR_CLKDIV_MAX) : 1;
-	val = FIELD_PREP(FMC2_BTR_CLKDIV, val);
-	regmap_update_bits(ebi->regmap, FMC2_BTR(cs), FMC2_BTR_CLKDIV, val);
+	if (ebi->majrev >= FMC2_VERR_MAJREV_2) {
+		u32 cfgr;
+
+		regmap_read(ebi->regmap, FMC2_CFGR, &cfgr);
+
+		if (cfgr & FMC2_CFGR_CCLKEN) {
+			reg = FMC2_CFGR;
+			mask = FMC2_CFGR_CLKDIV;
+		}
+	}
+
+	val = setup ? clamp_val(setup - 1, 1, FMC2_REG_CLKDIV_MAX) : 1;
+	if (reg == FMC2_CFGR)
+		val = FIELD_PREP(FMC2_CFGR_CLKDIV, val);
+	else
+		val = FIELD_PREP(FMC2_BTR_CLKDIV, val);
+	regmap_update_bits(ebi->regmap, reg, mask, val);
 
 	return 0;
 }
@@ -697,27 +771,58 @@ static int stm32_fmc2_ebi_set_max_low_pulse(struct stm32_fmc2_ebi *ebi,
 					    const struct stm32_fmc2_prop *prop,
 					    int cs, u32 setup)
 {
-	u32 old_val, new_val, pcscntr;
+	u32 val;
+	u32 reg = ebi->majrev < FMC2_VERR_MAJREV_2 ? FMC2_PCSCNTR :
+						     FMC2_BCR(cs);
+	u32 mask = ebi->majrev < FMC2_VERR_MAJREV_2 ? FMC2_PCSCNTR_CSCOUNT :
+						      FMC2_BCR_CSCOUNT;
 
-	if (setup < 1)
-		return 0;
+	if (ebi->majrev < FMC2_VERR_MAJREV_2) {
+		u32 old_val, pcscntr;
 
-	regmap_read(ebi->regmap, FMC2_PCSCNTR, &pcscntr);
+		if (setup < 1)
+			return 0;
 
-	/* Enable counter for the bank */
-	regmap_update_bits(ebi->regmap, FMC2_PCSCNTR,
-			   FMC2_PCSCNTR_CNTBEN(cs),
-			   FMC2_PCSCNTR_CNTBEN(cs));
+		regmap_read(ebi->regmap, reg, &pcscntr);
 
-	new_val = min_t(u32, setup - 1, FMC2_PCSCNTR_CSCOUNT_MAX);
-	old_val = FIELD_GET(FMC2_PCSCNTR_CSCOUNT, pcscntr);
-	if (old_val && new_val > old_val)
-		/* Keep current counter value */
-		return 0;
+		/* Enable counter for the bank */
+		regmap_update_bits(ebi->regmap, reg,
+				   FMC2_PCSCNTR_CNTBEN(cs),
+				   FMC2_PCSCNTR_CNTBEN(cs));
+
+		val = min_t(u32, setup - 1, FMC2_PCSCNTR_CSCOUNT_MAX);
+		old_val = FIELD_GET(FMC2_PCSCNTR_CSCOUNT, pcscntr);
+		if (old_val && val > old_val)
+			/* Keep current counter value */
+			return 0;
+
+		val = FIELD_PREP(FMC2_PCSCNTR_CSCOUNT, val);
+	} else {
+		if (setup == FMC2_CSCOUNT_0)
+			val = FIELD_PREP(FMC2_BCR_CSCOUNT, FMC2_BCR_CSCOUNT_0);
+		else if (setup == FMC2_CSCOUNT_1)
+			val = FIELD_PREP(FMC2_BCR_CSCOUNT, FMC2_BCR_CSCOUNT_1);
+		else if (setup <= FMC2_CSCOUNT_64)
+			val = FIELD_PREP(FMC2_BCR_CSCOUNT, FMC2_BCR_CSCOUNT_64);
+		else
+			val = FIELD_PREP(FMC2_BCR_CSCOUNT,
+					 FMC2_BCR_CSCOUNT_256);
+	}
+
+	regmap_update_bits(ebi->regmap, reg, mask, val);
 
-	new_val = FIELD_PREP(FMC2_PCSCNTR_CSCOUNT, new_val);
-	regmap_update_bits(ebi->regmap, FMC2_PCSCNTR,
-			   FMC2_PCSCNTR_CSCOUNT, new_val);
+	return 0;
+}
+
+static int stm32_fmc2_ebi_set_cclk(struct stm32_fmc2_ebi *ebi,
+				   const struct stm32_fmc2_prop *prop,
+				   int cs, u32 setup)
+{
+	u32 reg = ebi->majrev < FMC2_VERR_MAJREV_2 ? FMC2_BCR1 : FMC2_CFGR;
+	u32 mask = ebi->majrev < FMC2_VERR_MAJREV_2 ? FMC2_BCR1_CCLKEN :
+						      FMC2_CFGR_CCLKEN;
+
+	regmap_update_bits(ebi->regmap, reg, mask, setup ? mask : 0);
 
 	return 0;
 }
@@ -732,10 +837,8 @@ static const struct stm32_fmc2_prop stm32_fmc2_child_props[] = {
 	{
 		.name = "st,fmc2-ebi-cs-cclk-enable",
 		.bprop = true,
-		.reg_type = FMC2_REG_BCR,
-		.reg_mask = FMC2_BCR1_CCLKEN,
 		.check = stm32_fmc2_ebi_check_cclk,
-		.set = stm32_fmc2_ebi_set_bit_field,
+		.set = stm32_fmc2_ebi_set_cclk,
 	},
 	{
 		.name = "st,fmc2-ebi-cs-mux-enable",
@@ -831,7 +934,7 @@ static const struct stm32_fmc2_prop stm32_fmc2_child_props[] = {
 	},
 	{
 		.name = "st,fmc2-ebi-cs-clk-period-ns",
-		.reset_val = FMC2_BTR_CLKDIV_MAX + 1,
+		.reset_val = FMC2_REG_CLKDIV_MAX + 1,
 		.check = stm32_fmc2_ebi_check_clk_period,
 		.calculate = stm32_fmc2_ebi_ns_to_clock_cycles,
 		.set = stm32_fmc2_ebi_set_clk_period,
@@ -959,7 +1062,10 @@ static void stm32_fmc2_ebi_save_setup(struct stm32_fmc2_ebi *ebi)
 		regmap_read(ebi->regmap, FMC2_BWTR(cs), &ebi->bwtr[cs]);
 	}
 
-	regmap_read(ebi->regmap, FMC2_PCSCNTR, &ebi->pcscntr);
+	if (ebi->majrev < FMC2_VERR_MAJREV_2)
+		regmap_read(ebi->regmap, FMC2_PCSCNTR, &ebi->pcscntr);
+	else
+		regmap_read(ebi->regmap, FMC2_CFGR, &ebi->cfgr);
 }
 
 static void stm32_fmc2_ebi_set_setup(struct stm32_fmc2_ebi *ebi)
@@ -972,7 +1078,10 @@ static void stm32_fmc2_ebi_set_setup(struct stm32_fmc2_ebi *ebi)
 		regmap_write(ebi->regmap, FMC2_BWTR(cs), ebi->bwtr[cs]);
 	}
 
-	regmap_write(ebi->regmap, FMC2_PCSCNTR, ebi->pcscntr);
+	if (ebi->majrev < FMC2_VERR_MAJREV_2)
+		regmap_write(ebi->regmap, FMC2_PCSCNTR, ebi->pcscntr);
+	else
+		regmap_write(ebi->regmap, FMC2_CFGR, ebi->cfgr);
 }
 
 static void stm32_fmc2_ebi_disable_banks(struct stm32_fmc2_ebi *ebi)
@@ -1011,13 +1120,20 @@ static bool stm32_fmc2_ebi_nwait_used_by_ctrls(struct stm32_fmc2_ebi *ebi)
 
 static void stm32_fmc2_ebi_enable(struct stm32_fmc2_ebi *ebi)
 {
-	regmap_update_bits(ebi->regmap, FMC2_BCR1,
-			   FMC2_BCR1_FMC2EN, FMC2_BCR1_FMC2EN);
+	u32 reg = ebi->majrev < FMC2_VERR_MAJREV_2 ? FMC2_BCR1 : FMC2_CFGR;
+	u32 mask = ebi->majrev < FMC2_VERR_MAJREV_2 ? FMC2_BCR1_FMC2EN :
+						      FMC2_CFGR_FMC2EN;
+
+	regmap_update_bits(ebi->regmap, reg, mask, mask);
 }
 
 static void stm32_fmc2_ebi_disable(struct stm32_fmc2_ebi *ebi)
 {
-	regmap_update_bits(ebi->regmap, FMC2_BCR1, FMC2_BCR1_FMC2EN, 0);
+	u32 reg = ebi->majrev < FMC2_VERR_MAJREV_2 ? FMC2_BCR1 : FMC2_CFGR;
+	u32 mask = ebi->majrev < FMC2_VERR_MAJREV_2 ? FMC2_BCR1_FMC2EN :
+						      FMC2_CFGR_FMC2EN;
+
+	regmap_update_bits(ebi->regmap, reg, mask, 0);
 }
 
 static int stm32_fmc2_ebi_setup_cs(struct stm32_fmc2_ebi *ebi,
@@ -1108,6 +1224,7 @@ static int stm32_fmc2_ebi_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct stm32_fmc2_ebi *ebi;
 	struct reset_control *rstc;
+	u32 verr;
 	int ret;
 
 	ebi = devm_kzalloc(&pdev->dev, sizeof(*ebi), GFP_KERNEL);
@@ -1141,6 +1258,9 @@ static int stm32_fmc2_ebi_probe(struct platform_device *pdev)
 		reset_control_deassert(rstc);
 	}
 
+	regmap_read(ebi->regmap, FMC2_VERR, &verr);
+	ebi->majrev = FIELD_GET(FMC2_VERR_MAJREV, verr);
+
 	ret = stm32_fmc2_ebi_parse_dt(ebi);
 	if (ret)
 		goto err_release;
-- 
2.25.1

Re: [PATCH 05/12] memory: stm32-fmc2-ebi: update the driver to support revision 2

[Krzysztof Kozlowski]

On 12/02/2024 18:48, Christophe Kerello wrote:
> Add the support of the revision 2 of FMC2 IP.
>  - PCSCNTR register has been removed,
>  - CFGR register has been added,
>  - the bit used to enable the IP has moved from BCR1 to CFGR,
>  - the timeout for CEx deassertion has moved from PCSCNTR to BCRx,
>  - the continuous clock enable has moved from BCR1 to CFGR,
>  - the clk divide ratio has moved from BCR1 to CFGR.
> 
> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
> ---
>  drivers/memory/stm32-fmc2-ebi.c | 206 +++++++++++++++++++++++++-------
>  1 file changed, 163 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/memory/stm32-fmc2-ebi.c b/drivers/memory/stm32-fmc2-ebi.c
> index d79dcb6c239a..066722274a45 100644
> --- a/drivers/memory/stm32-fmc2-ebi.c
> +++ b/drivers/memory/stm32-fmc2-ebi.c
> @@ -20,8 +20,10 @@
>  #define FMC2_BCR(x)			((x) * 0x8 + FMC2_BCR1)
>  #define FMC2_BTR(x)			((x) * 0x8 + FMC2_BTR1)
>  #define FMC2_PCSCNTR			0x20
> +#define FMC2_CFGR			0x20
>  #define FMC2_BWTR1			0x104
>  #define FMC2_BWTR(x)			((x) * 0x8 + FMC2_BWTR1)
> +#define FMC2_VERR			0x3f4
>  
>  /* Register: FMC2_BCR1 */
>  #define FMC2_BCR1_CCLKEN		BIT(20)
> @@ -42,6 +44,7 @@
>  #define FMC2_BCR_ASYNCWAIT		BIT(15)
>  #define FMC2_BCR_CPSIZE			GENMASK(18, 16)
>  #define FMC2_BCR_CBURSTRW		BIT(19)
> +#define FMC2_BCR_CSCOUNT		GENMASK(21, 20)
>  #define FMC2_BCR_NBLSET			GENMASK(23, 22)
>  
>  /* Register: FMC2_BTRx/FMC2_BWTRx */
> @@ -58,6 +61,15 @@
>  #define FMC2_PCSCNTR_CSCOUNT		GENMASK(15, 0)
>  #define FMC2_PCSCNTR_CNTBEN(x)		BIT((x) + 16)
>  
> +/* Register: FMC2_CFGR */
> +#define FMC2_CFGR_CLKDIV		GENMASK(19, 16)
> +#define FMC2_CFGR_CCLKEN		BIT(20)
> +#define FMC2_CFGR_FMC2EN		BIT(31)
> +
> +/* Register: FMC2_VERR */
> +#define FMC2_VERR_MAJREV		GENMASK(7, 4)
> +#define FMC2_VERR_MAJREV_2		2
> +
>  #define FMC2_MAX_EBI_CE			4
>  #define FMC2_MAX_BANKS			5
>  
> @@ -74,6 +86,11 @@
>  #define FMC2_BCR_MTYP_PSRAM		0x1
>  #define FMC2_BCR_MTYP_NOR		0x2
>  
> +#define FMC2_BCR_CSCOUNT_0		0x0
> +#define FMC2_BCR_CSCOUNT_1		0x1
> +#define FMC2_BCR_CSCOUNT_64		0x2
> +#define FMC2_BCR_CSCOUNT_256		0x3
> +
>  #define FMC2_BXTR_EXTMOD_A		0x0
>  #define FMC2_BXTR_EXTMOD_B		0x1
>  #define FMC2_BXTR_EXTMOD_C		0x2
> @@ -85,7 +102,7 @@
>  #define FMC2_BXTR_DATAST_MAX		0xff
>  #define FMC2_BXTR_BUSTURN_MAX		0xf
>  #define FMC2_BXTR_DATAHLD_MAX		0x3
> -#define FMC2_BTR_CLKDIV_MAX		0xf
> +#define FMC2_REG_CLKDIV_MAX		0xf

Why?

>  #define FMC2_BTR_DATLAT_MAX		0xf
>  #define FMC2_PCSCNTR_CSCOUNT_MAX	0xff
>  
> @@ -101,7 +118,8 @@ enum stm32_fmc2_ebi_register_type {
>  	FMC2_REG_BCR = 1,
>  	FMC2_REG_BTR,
>  	FMC2_REG_BWTR,
> -	FMC2_REG_PCSCNTR
> +	FMC2_REG_PCSCNTR,
> +	FMC2_REG_CFGR,
>  };
>  
>  enum stm32_fmc2_ebi_transaction_type {
> @@ -132,6 +150,13 @@ enum stm32_fmc2_ebi_cpsize {
>  	FMC2_CPSIZE_1024 = 1024
>  };
>  
> +enum stm32_fmc2_ebi_cscount {
> +	FMC2_CSCOUNT_0 = 0,
> +	FMC2_CSCOUNT_1 = 1,
> +	FMC2_CSCOUNT_64 = 64,
> +	FMC2_CSCOUNT_256 = 256
> +};
> +
>  struct stm32_fmc2_ebi_data {
>  	bool rnb_for_nand;
>  };
> @@ -142,11 +167,13 @@ struct stm32_fmc2_ebi {
>  	struct regmap *regmap;
>  	const struct stm32_fmc2_ebi_data *data;
>  	u8 bank_assigned;
> +	u8 majrev;
>  
>  	u32 bcr[FMC2_MAX_EBI_CE];
>  	u32 btr[FMC2_MAX_EBI_CE];
>  	u32 bwtr[FMC2_MAX_EBI_CE];
>  	u32 pcscntr;
> +	u32 cfgr;
>  };
>  
>  /*
> @@ -274,15 +301,29 @@ static int stm32_fmc2_ebi_check_clk_period(struct stm32_fmc2_ebi *ebi,
>  					   const struct stm32_fmc2_prop *prop,
>  					   int cs)
>  {
> -	u32 bcr, bcr1;
> +	u32 bcr, cfgr;
>  
>  	regmap_read(ebi->regmap, FMC2_BCR(cs), &bcr);
> -	if (cs)
> -		regmap_read(ebi->regmap, FMC2_BCR1, &bcr1);
> -	else
> -		bcr1 = bcr;
>  
> -	if (bcr & FMC2_BCR_BURSTEN && (!cs || !(bcr1 & FMC2_BCR1_CCLKEN)))
> +	if (ebi->majrev < FMC2_VERR_MAJREV_2) {
> +		u32 bcr1;
> +
> +		if (cs)
> +			regmap_read(ebi->regmap, FMC2_BCR1, &bcr1);
> +		else
> +			bcr1 = bcr;
> +
> +		if (bcr & FMC2_BCR_BURSTEN &&
> +		    (!cs || !(bcr1 & FMC2_BCR1_CCLKEN)))
> +			return 0;
> +
> +		return -EINVAL;
> +	}

The function is not really readable anymore. Please split it into three
functions: for v1 (so original code), v2 and wrapper choosing it based
on revision). Or two functions and some sort of ops with function
pointers (so you call ops->check_clk_period). Or just parametrize the
registers with two different "struct reg_field" and use appropriate one
for given revision (the code looks the same!)
Or just two set of stm32_fmc2_child_props...

Anyway the duplicated code just two read different register is it not
the way to go.


> +
> +	regmap_read(ebi->regmap, FMC2_CFGR, &cfgr);
> +
> +	if (bcr & FMC2_BCR_BURSTEN &&
> +	    (!cs || !(cfgr & FMC2_CFGR_CCLKEN)))
>  		return 0;
>  
>  	return -EINVAL;
> @@ -311,15 +352,29 @@ static u32 stm32_fmc2_ebi_ns_to_clk_period(struct stm32_fmc2_ebi *ebi,
>  					   int cs, u32 setup)
>  {
>  	u32 nb_clk_cycles = stm32_fmc2_ebi_ns_to_clock_cycles(ebi, cs, setup);
> -	u32 bcr, btr, clk_period;
> +	u32 btr, clk_period;
>  
> -	regmap_read(ebi->regmap, FMC2_BCR1, &bcr);
> -	if (bcr & FMC2_BCR1_CCLKEN || !cs)
> -		regmap_read(ebi->regmap, FMC2_BTR1, &btr);
> -	else
> -		regmap_read(ebi->regmap, FMC2_BTR(cs), &btr);
> +	if (ebi->majrev < FMC2_VERR_MAJREV_2) {
> +		u32 bcr;
>  
> -	clk_period = FIELD_GET(FMC2_BTR_CLKDIV, btr) + 1;
> +		regmap_read(ebi->regmap, FMC2_BCR1, &bcr);
> +		if (bcr & FMC2_BCR1_CCLKEN || !cs)
> +			regmap_read(ebi->regmap, FMC2_BTR1, &btr);
> +		else
> +			regmap_read(ebi->regmap, FMC2_BTR(cs), &btr);
> +
> +		clk_period = FIELD_GET(FMC2_BTR_CLKDIV, btr) + 1;
> +	} else {
> +		u32 cfgr;
> +
> +		regmap_read(ebi->regmap, FMC2_CFGR, &cfgr);
> +		if (cfgr & FMC2_CFGR_CCLKEN) {
> +			clk_period = FIELD_GET(FMC2_CFGR_CLKDIV, cfgr) + 1;
> +		} else {
> +			regmap_read(ebi->regmap, FMC2_BTR(cs), &btr);
> +			clk_period = FIELD_GET(FMC2_BTR_CLKDIV, btr) + 1;
> +		}
> +	}
>  
>  	return DIV_ROUND_UP(nb_clk_cycles, clk_period);
>  }
> @@ -339,6 +394,9 @@ static int stm32_fmc2_ebi_get_reg(int reg_type, int cs, u32 *reg)
>  	case FMC2_REG_PCSCNTR:
>  		*reg = FMC2_PCSCNTR;
>  		break;
> +	case FMC2_REG_CFGR:
> +		*reg = FMC2_CFGR;
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -672,10 +730,26 @@ static int stm32_fmc2_ebi_set_clk_period(struct stm32_fmc2_ebi *ebi,
>  					 int cs, u32 setup)
>  {
>  	u32 val;
> +	u32 reg = FMC2_BTR(cs);
> +	u32 mask = FMC2_BTR_CLKDIV;
>  
> -	val = setup ? clamp_val(setup - 1, 1, FMC2_BTR_CLKDIV_MAX) : 1;
> -	val = FIELD_PREP(FMC2_BTR_CLKDIV, val);
> -	regmap_update_bits(ebi->regmap, FMC2_BTR(cs), FMC2_BTR_CLKDIV, val);
> +	if (ebi->majrev >= FMC2_VERR_MAJREV_2) {
> +		u32 cfgr;
> +
> +		regmap_read(ebi->regmap, FMC2_CFGR, &cfgr);
> +
> +		if (cfgr & FMC2_CFGR_CCLKEN) {
> +			reg = FMC2_CFGR;
> +			mask = FMC2_CFGR_CLKDIV;
> +		}
> +	}
> +
> +	val = setup ? clamp_val(setup - 1, 1, FMC2_REG_CLKDIV_MAX) : 1;
> +	if (reg == FMC2_CFGR)
> +		val = FIELD_PREP(FMC2_CFGR_CLKDIV, val);
> +	else
> +		val = FIELD_PREP(FMC2_BTR_CLKDIV, val);

This scales poorly for any new revision. You should really start using
reg_field per each version.

> +	regmap_update_bits(ebi->regmap, reg, mask, val);
>  
>  	return 0;
>  }
> @@ -697,27 +771,58 @@ static int stm32_fmc2_ebi_set_max_low_pulse(struct stm32_fmc2_ebi *ebi,
>  					    const struct stm32_fmc2_prop *prop,
>  					    int cs, u32 setup)
>  {
> -	u32 old_val, new_val, pcscntr;
> +	u32 val;
> +	u32 reg = ebi->majrev < FMC2_VERR_MAJREV_2 ? FMC2_PCSCNTR :
> +						     FMC2_BCR(cs);
> +	u32 mask = ebi->majrev < FMC2_VERR_MAJREV_2 ? FMC2_PCSCNTR_CSCOUNT :
> +						      FMC2_BCR_CSCOUNT;

You have such code everywhere... sorry, that's not readable at all. No
conditional assignmnents, that's a clear NAK.



Best regards,
Krzysztof

Re: [PATCH 05/12] memory: stm32-fmc2-ebi: update the driver to support revision 2

[Christophe Kerello]

On 2/13/24 08:46, Krzysztof Kozlowski wrote:
> The function is not really readable anymore. Please split it into three
> functions: for v1 (so original code), v2 and wrapper choosing it based
> on revision). Or two functions and some sort of ops with function
> pointers (so you call ops->check_clk_period). Or just parametrize the
> registers with two different "struct reg_field" and use appropriate one
> for given revision (the code looks the same!)
> Or just two set of stm32_fmc2_child_props...
> 
> Anyway the duplicated code just two read different register is it not
> the way to go.

Hi Krzysztof,

As I said in patch 4, I am going to rewrite this patch and I am going to
use the platform data to distinguish between each variant instead of
checking the IP revision.

Regards,
Christophe Kerello.

[PATCH 06/12] memory: stm32-fmc2-ebi: add RIF support

[Christophe Kerello]

The FMC2 revision 2 supports security and isolation compliant with
the Resource Isolation Framework (RIF). From RIF point of view,
the FMC2 is composed of several independent resources, listed below,
which can be assigned to different security and compartment domains:
  - 0: Common FMC_CFGR register.
  - 1: EBI controller for Chip Select 1.
  - 2: EBI controller for Chip Select 2.
  - 3: EBI controller for Chip Select 3.
  - 4: EBI controller for Chip Select 4.
  - 5: NAND controller.

Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
---
 drivers/memory/stm32-fmc2-ebi.c | 178 +++++++++++++++++++++++++++++++-
 1 file changed, 174 insertions(+), 4 deletions(-)

diff --git a/drivers/memory/stm32-fmc2-ebi.c b/drivers/memory/stm32-fmc2-ebi.c
index 066722274a45..04248c15832f 100644
--- a/drivers/memory/stm32-fmc2-ebi.c
+++ b/drivers/memory/stm32-fmc2-ebi.c
@@ -21,8 +21,14 @@
 #define FMC2_BTR(x)			((x) * 0x8 + FMC2_BTR1)
 #define FMC2_PCSCNTR			0x20
 #define FMC2_CFGR			0x20
+#define FMC2_SR				0x84
 #define FMC2_BWTR1			0x104
 #define FMC2_BWTR(x)			((x) * 0x8 + FMC2_BWTR1)
+#define FMC2_SECCFGR			0x300
+#define FMC2_CIDCFGR0			0x30c
+#define FMC2_CIDCFGR(x)			((x) * 0x8 + FMC2_CIDCFGR0)
+#define FMC2_SEMCR0			0x310
+#define FMC2_SEMCR(x)			((x) * 0x8 + FMC2_SEMCR0)
 #define FMC2_VERR			0x3f4
 
 /* Register: FMC2_BCR1 */
@@ -66,12 +72,27 @@
 #define FMC2_CFGR_CCLKEN		BIT(20)
 #define FMC2_CFGR_FMC2EN		BIT(31)
 
+/* Register: FMC2_SR */
+#define FMC2_SR_ISOST			GENMASK(1, 0)
+
+/* Register: FMC2_CIDCFGR */
+#define FMC2_CIDCFGR_CFEN		BIT(0)
+#define FMC2_CIDCFGR_SEMEN		BIT(1)
+#define FMC2_CIDCFGR_SCID		GENMASK(6, 4)
+#define FMC2_CIDCFGR_SEMWLC1		BIT(17)
+
+/* Register: FMC2_SEMCR */
+#define FMC2_SEMCR_SEM_MUTEX		BIT(0)
+#define FMC2_SEMCR_SEMCID		GENMASK(7, 5)
+
 /* Register: FMC2_VERR */
 #define FMC2_VERR_MAJREV		GENMASK(7, 4)
 #define FMC2_VERR_MAJREV_2		2
 
 #define FMC2_MAX_EBI_CE			4
 #define FMC2_MAX_BANKS			5
+#define FMC2_MAX_RESOURCES		6
+#define FMC2_CID1			1
 
 #define FMC2_BCR_CPSIZE_0		0x0
 #define FMC2_BCR_CPSIZE_128		0x1
@@ -167,7 +188,9 @@ struct stm32_fmc2_ebi {
 	struct regmap *regmap;
 	const struct stm32_fmc2_ebi_data *data;
 	u8 bank_assigned;
+	u8 sem_taken;
 	u8 majrev;
+	bool access_granted;
 
 	u32 bcr[FMC2_MAX_EBI_CE];
 	u32 btr[FMC2_MAX_EBI_CE];
@@ -733,6 +756,11 @@ static int stm32_fmc2_ebi_set_clk_period(struct stm32_fmc2_ebi *ebi,
 	u32 reg = FMC2_BTR(cs);
 	u32 mask = FMC2_BTR_CLKDIV;
 
+	if (!ebi->access_granted) {
+		dev_err(ebi->dev, "CFGR access forbidden\n");
+		return -EACCES;
+	}
+
 	if (ebi->majrev >= FMC2_VERR_MAJREV_2) {
 		u32 cfgr;
 
@@ -822,6 +850,11 @@ static int stm32_fmc2_ebi_set_cclk(struct stm32_fmc2_ebi *ebi,
 	u32 mask = ebi->majrev < FMC2_VERR_MAJREV_2 ? FMC2_BCR1_CCLKEN :
 						      FMC2_CFGR_CCLKEN;
 
+	if (!ebi->access_granted) {
+		dev_err(ebi->dev, "CFGR access forbidden\n");
+		return -EACCES;
+	}
+
 	regmap_update_bits(ebi->regmap, reg, mask, setup ? mask : 0);
 
 	return 0;
@@ -990,6 +1023,107 @@ static const struct stm32_fmc2_prop stm32_fmc2_child_props[] = {
 	},
 };
 
+static int stm32_fmc2_ebi_check_rif(struct stm32_fmc2_ebi *ebi, u32 resource)
+{
+	u32 seccfgr, cidcfgr, semcr;
+	int cid;
+
+	if (ebi->majrev < FMC2_VERR_MAJREV_2)
+		return 0;
+
+	if (resource >= FMC2_MAX_RESOURCES)
+		return -EINVAL;
+
+	regmap_read(ebi->regmap, FMC2_SECCFGR, &seccfgr);
+	if (seccfgr & BIT(resource)) {
+		if (resource)
+			dev_err(ebi->dev, "resource %d is configured as secure\n",
+				resource);
+
+		return -EACCES;
+	}
+
+	regmap_read(ebi->regmap, FMC2_CIDCFGR(resource), &cidcfgr);
+	if (!(cidcfgr & FMC2_CIDCFGR_CFEN))
+		/* CID filtering is turned off: access granted */
+		return 0;
+
+	if (!(cidcfgr & FMC2_CIDCFGR_SEMEN)) {
+		/* Static CID mode */
+		cid = FIELD_GET(FMC2_CIDCFGR_SCID, cidcfgr);
+		if (cid != FMC2_CID1) {
+			if (resource)
+				dev_err(ebi->dev, "static CID%d set for resource %d\n",
+					cid, resource);
+
+			return -EACCES;
+		}
+
+		return 0;
+	}
+
+	/* Pass-list with semaphore mode */
+	if (!(cidcfgr & FMC2_CIDCFGR_SEMWLC1)) {
+		if (resource)
+			dev_err(ebi->dev, "CID1 is block-listed for resource %d\n",
+				resource);
+
+		return -EACCES;
+	}
+
+	regmap_read(ebi->regmap, FMC2_SEMCR(resource), &semcr);
+	if (!(semcr & FMC2_SEMCR_SEM_MUTEX)) {
+		regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource),
+				   FMC2_SEMCR_SEM_MUTEX, FMC2_SEMCR_SEM_MUTEX);
+		regmap_read(ebi->regmap, FMC2_SEMCR(resource), &semcr);
+	}
+
+	cid = FIELD_GET(FMC2_SEMCR_SEMCID, semcr);
+	if (cid != FMC2_CID1) {
+		if (resource)
+			dev_err(ebi->dev, "resource %d is already used by CID%d\n",
+				resource, cid);
+
+		return -EACCES;
+	}
+
+	ebi->sem_taken |= BIT(resource);
+
+	return 0;
+}
+
+static void stm32_fmc2_ebi_put_sems(struct stm32_fmc2_ebi *ebi)
+{
+	unsigned int resource;
+
+	if (ebi->majrev < FMC2_VERR_MAJREV_2)
+		return;
+
+	for (resource = 0; resource < FMC2_MAX_RESOURCES; resource++) {
+		if (!(ebi->sem_taken & BIT(resource)))
+			continue;
+
+		regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource),
+				   FMC2_SEMCR_SEM_MUTEX, 0);
+	}
+}
+
+static void stm32_fmc2_ebi_get_sems(struct stm32_fmc2_ebi *ebi)
+{
+	unsigned int resource;
+
+	if (ebi->majrev < FMC2_VERR_MAJREV_2)
+		return;
+
+	for (resource = 0; resource < FMC2_MAX_RESOURCES; resource++) {
+		if (!(ebi->sem_taken & BIT(resource)))
+			continue;
+
+		regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource),
+				   FMC2_SEMCR_SEM_MUTEX, FMC2_SEMCR_SEM_MUTEX);
+	}
+}
+
 static int stm32_fmc2_ebi_parse_prop(struct stm32_fmc2_ebi *ebi,
 				     struct device_node *dev_node,
 				     const struct stm32_fmc2_prop *prop,
@@ -1057,6 +1191,9 @@ static void stm32_fmc2_ebi_save_setup(struct stm32_fmc2_ebi *ebi)
 	unsigned int cs;
 
 	for (cs = 0; cs < FMC2_MAX_EBI_CE; cs++) {
+		if (!(ebi->bank_assigned & BIT(cs)))
+			continue;
+
 		regmap_read(ebi->regmap, FMC2_BCR(cs), &ebi->bcr[cs]);
 		regmap_read(ebi->regmap, FMC2_BTR(cs), &ebi->btr[cs]);
 		regmap_read(ebi->regmap, FMC2_BWTR(cs), &ebi->bwtr[cs]);
@@ -1064,7 +1201,7 @@ static void stm32_fmc2_ebi_save_setup(struct stm32_fmc2_ebi *ebi)
 
 	if (ebi->majrev < FMC2_VERR_MAJREV_2)
 		regmap_read(ebi->regmap, FMC2_PCSCNTR, &ebi->pcscntr);
-	else
+	else if (ebi->access_granted)
 		regmap_read(ebi->regmap, FMC2_CFGR, &ebi->cfgr);
 }
 
@@ -1073,6 +1210,9 @@ static void stm32_fmc2_ebi_set_setup(struct stm32_fmc2_ebi *ebi)
 	unsigned int cs;
 
 	for (cs = 0; cs < FMC2_MAX_EBI_CE; cs++) {
+		if (!(ebi->bank_assigned & BIT(cs)))
+			continue;
+
 		regmap_write(ebi->regmap, FMC2_BCR(cs), ebi->bcr[cs]);
 		regmap_write(ebi->regmap, FMC2_BTR(cs), ebi->btr[cs]);
 		regmap_write(ebi->regmap, FMC2_BWTR(cs), ebi->bwtr[cs]);
@@ -1080,7 +1220,7 @@ static void stm32_fmc2_ebi_set_setup(struct stm32_fmc2_ebi *ebi)
 
 	if (ebi->majrev < FMC2_VERR_MAJREV_2)
 		regmap_write(ebi->regmap, FMC2_PCSCNTR, ebi->pcscntr);
-	else
+	else if (ebi->access_granted)
 		regmap_write(ebi->regmap, FMC2_CFGR, ebi->cfgr);
 }
 
@@ -1124,7 +1264,8 @@ static void stm32_fmc2_ebi_enable(struct stm32_fmc2_ebi *ebi)
 	u32 mask = ebi->majrev < FMC2_VERR_MAJREV_2 ? FMC2_BCR1_FMC2EN :
 						      FMC2_CFGR_FMC2EN;
 
-	regmap_update_bits(ebi->regmap, reg, mask, mask);
+	if (ebi->access_granted)
+		regmap_update_bits(ebi->regmap, reg, mask, mask);
 }
 
 static void stm32_fmc2_ebi_disable(struct stm32_fmc2_ebi *ebi)
@@ -1133,7 +1274,8 @@ static void stm32_fmc2_ebi_disable(struct stm32_fmc2_ebi *ebi)
 	u32 mask = ebi->majrev < FMC2_VERR_MAJREV_2 ? FMC2_BCR1_FMC2EN :
 						      FMC2_CFGR_FMC2EN;
 
-	regmap_update_bits(ebi->regmap, reg, mask, 0);
+	if (ebi->access_granted)
+		regmap_update_bits(ebi->regmap, reg, mask, 0);
 }
 
 static int stm32_fmc2_ebi_setup_cs(struct stm32_fmc2_ebi *ebi,
@@ -1190,6 +1332,13 @@ static int stm32_fmc2_ebi_parse_dt(struct stm32_fmc2_ebi *ebi)
 			return -EINVAL;
 		}
 
+		ret = stm32_fmc2_ebi_check_rif(ebi, bank + 1);
+		if (ret) {
+			dev_err(dev, "bank access failed: %d\n", bank);
+			of_node_put(child);
+			return ret;
+		}
+
 		if (bank < FMC2_MAX_EBI_CE) {
 			ret = stm32_fmc2_ebi_setup_cs(ebi, child, bank);
 			if (ret) {
@@ -1261,6 +1410,23 @@ static int stm32_fmc2_ebi_probe(struct platform_device *pdev)
 	regmap_read(ebi->regmap, FMC2_VERR, &verr);
 	ebi->majrev = FIELD_GET(FMC2_VERR_MAJREV, verr);
 
+	/* Check if CFGR register can be modified */
+	ret = stm32_fmc2_ebi_check_rif(ebi, 0);
+	if (!ret)
+		ebi->access_granted = true;
+
+	/* In case of CFGR is secure, just check that the FMC2 is enabled */
+	if (!ebi->access_granted) {
+		u32 sr;
+
+		regmap_read(ebi->regmap, FMC2_SR, &sr);
+		if (sr & FMC2_SR_ISOST) {
+			dev_err(dev, "FMC2 is not ready to be used.\n");
+			ret = -EACCES;
+			goto err_release;
+		}
+	}
+
 	ret = stm32_fmc2_ebi_parse_dt(ebi);
 	if (ret)
 		goto err_release;
@@ -1273,6 +1439,7 @@ static int stm32_fmc2_ebi_probe(struct platform_device *pdev)
 err_release:
 	stm32_fmc2_ebi_disable_banks(ebi);
 	stm32_fmc2_ebi_disable(ebi);
+	stm32_fmc2_ebi_put_sems(ebi);
 	clk_disable_unprepare(ebi->clk);
 
 	return ret;
@@ -1285,6 +1452,7 @@ static void stm32_fmc2_ebi_remove(struct platform_device *pdev)
 	of_platform_depopulate(&pdev->dev);
 	stm32_fmc2_ebi_disable_banks(ebi);
 	stm32_fmc2_ebi_disable(ebi);
+	stm32_fmc2_ebi_put_sems(ebi);
 	clk_disable_unprepare(ebi->clk);
 }
 
@@ -1293,6 +1461,7 @@ static int __maybe_unused stm32_fmc2_ebi_suspend(struct device *dev)
 	struct stm32_fmc2_ebi *ebi = dev_get_drvdata(dev);
 
 	stm32_fmc2_ebi_disable(ebi);
+	stm32_fmc2_ebi_put_sems(ebi);
 	clk_disable_unprepare(ebi->clk);
 	pinctrl_pm_select_sleep_state(dev);
 
@@ -1310,6 +1479,7 @@ static int __maybe_unused stm32_fmc2_ebi_resume(struct device *dev)
 	if (ret)
 		return ret;
 
+	stm32_fmc2_ebi_get_sems(ebi);
 	stm32_fmc2_ebi_set_setup(ebi);
 	stm32_fmc2_ebi_enable(ebi);
 
-- 
2.25.1

Re: [PATCH 06/12] memory: stm32-fmc2-ebi: add RIF support

[Krzysztof Kozlowski]

On 12/02/2024 18:48, Christophe Kerello wrote:
> The FMC2 revision 2 supports security and isolation compliant with
> the Resource Isolation Framework (RIF). From RIF point of view,
> the FMC2 is composed of several independent resources, listed below,
> which can be assigned to different security and compartment domains:
>   - 0: Common FMC_CFGR register.
>   - 1: EBI controller for Chip Select 1.
>   - 2: EBI controller for Chip Select 2.
>   - 3: EBI controller for Chip Select 3.
>   - 4: EBI controller for Chip Select 4.
>   - 5: NAND controller.
> 


>  	regmap_update_bits(ebi->regmap, reg, mask, setup ? mask : 0);
>  
>  	return 0;
> @@ -990,6 +1023,107 @@ static const struct stm32_fmc2_prop stm32_fmc2_child_props[] = {
>  	},
>  };
>  
> +static int stm32_fmc2_ebi_check_rif(struct stm32_fmc2_ebi *ebi, u32 resource)
> +{
> +	u32 seccfgr, cidcfgr, semcr;
> +	int cid;
> +
> +	if (ebi->majrev < FMC2_VERR_MAJREV_2)
> +		return 0;
> +
> +	if (resource >= FMC2_MAX_RESOURCES)
> +		return -EINVAL;
> +
> +	regmap_read(ebi->regmap, FMC2_SECCFGR, &seccfgr);

No checking of read value?

> +	if (seccfgr & BIT(resource)) {

Then on read failure this is random stack junk.

> +		if (resource)
> +			dev_err(ebi->dev, "resource %d is configured as secure\n",
> +				resource);
> +
> +		return -EACCES;
> +	}
> +
> +	regmap_read(ebi->regmap, FMC2_CIDCFGR(resource), &cidcfgr);
> +	if (!(cidcfgr & FMC2_CIDCFGR_CFEN))
> +		/* CID filtering is turned off: access granted */
> +		return 0;
> +
> +	if (!(cidcfgr & FMC2_CIDCFGR_SEMEN)) {
> +		/* Static CID mode */
> +		cid = FIELD_GET(FMC2_CIDCFGR_SCID, cidcfgr);
> +		if (cid != FMC2_CID1) {
> +			if (resource)
> +				dev_err(ebi->dev, "static CID%d set for resource %d\n",
> +					cid, resource);
> +
> +			return -EACCES;
> +		}
> +
> +		return 0;
> +	}
> +
> +	/* Pass-list with semaphore mode */
> +	if (!(cidcfgr & FMC2_CIDCFGR_SEMWLC1)) {
> +		if (resource)
> +			dev_err(ebi->dev, "CID1 is block-listed for resource %d\n",
> +				resource);
> +
> +		return -EACCES;
> +	}
> +
> +	regmap_read(ebi->regmap, FMC2_SEMCR(resource), &semcr);
> +	if (!(semcr & FMC2_SEMCR_SEM_MUTEX)) {
> +		regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource),
> +				   FMC2_SEMCR_SEM_MUTEX, FMC2_SEMCR_SEM_MUTEX);
> +		regmap_read(ebi->regmap, FMC2_SEMCR(resource), &semcr);
> +	}
> +
> +	cid = FIELD_GET(FMC2_SEMCR_SEMCID, semcr);
> +	if (cid != FMC2_CID1) {
> +		if (resource)
> +			dev_err(ebi->dev, "resource %d is already used by CID%d\n",
> +				resource, cid);
> +
> +		return -EACCES;
> +	}
> +
> +	ebi->sem_taken |= BIT(resource);
> +
> +	return 0;
> +}
> +
> +static void stm32_fmc2_ebi_put_sems(struct stm32_fmc2_ebi *ebi)
> +{
> +	unsigned int resource;
> +
> +	if (ebi->majrev < FMC2_VERR_MAJREV_2)
> +		return;
> +
> +	for (resource = 0; resource < FMC2_MAX_RESOURCES; resource++) {
> +		if (!(ebi->sem_taken & BIT(resource)))
> +			continue;
> +
> +		regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource),
> +				   FMC2_SEMCR_SEM_MUTEX, 0);
> +	}
> +}
> +
> +static void stm32_fmc2_ebi_get_sems(struct stm32_fmc2_ebi *ebi)
> +{
> +	unsigned int resource;
> +
> +	if (ebi->majrev < FMC2_VERR_MAJREV_2)
> +		return;
> +
> +	for (resource = 0; resource < FMC2_MAX_RESOURCES; resource++) {
> +		if (!(ebi->sem_taken & BIT(resource)))
> +			continue;
> +
> +		regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource),
> +				   FMC2_SEMCR_SEM_MUTEX, FMC2_SEMCR_SEM_MUTEX);
> +	}
> +}
> +
>  static int stm32_fmc2_ebi_parse_prop(struct stm32_fmc2_ebi *ebi,
>  				     struct device_node *dev_node,
>  				     const struct stm32_fmc2_prop *prop,
> @@ -1057,6 +1191,9 @@ static void stm32_fmc2_ebi_save_setup(struct stm32_fmc2_ebi *ebi)
>  	unsigned int cs;
>  
>  	for (cs = 0; cs < FMC2_MAX_EBI_CE; cs++) {
> +		if (!(ebi->bank_assigned & BIT(cs)))
> +			continue;
> +
>  		regmap_read(ebi->regmap, FMC2_BCR(cs), &ebi->bcr[cs]);
>  		regmap_read(ebi->regmap, FMC2_BTR(cs), &ebi->btr[cs]);
>  		regmap_read(ebi->regmap, FMC2_BWTR(cs), &ebi->bwtr[cs]);
> @@ -1064,7 +1201,7 @@ static void stm32_fmc2_ebi_save_setup(struct stm32_fmc2_ebi *ebi)
>  
>  	if (ebi->majrev < FMC2_VERR_MAJREV_2)
>  		regmap_read(ebi->regmap, FMC2_PCSCNTR, &ebi->pcscntr);
> -	else
> +	else if (ebi->access_granted)
>  		regmap_read(ebi->regmap, FMC2_CFGR, &ebi->cfgr);
>  }
>  
> @@ -1073,6 +1210,9 @@ static void stm32_fmc2_ebi_set_setup(struct stm32_fmc2_ebi *ebi)
>  	unsigned int cs;
>  
>  	for (cs = 0; cs < FMC2_MAX_EBI_CE; cs++) {
> +		if (!(ebi->bank_assigned & BIT(cs)))
> +			continue;
> +
>  		regmap_write(ebi->regmap, FMC2_BCR(cs), ebi->bcr[cs]);
>  		regmap_write(ebi->regmap, FMC2_BTR(cs), ebi->btr[cs]);
>  		regmap_write(ebi->regmap, FMC2_BWTR(cs), ebi->bwtr[cs]);
> @@ -1080,7 +1220,7 @@ static void stm32_fmc2_ebi_set_setup(struct stm32_fmc2_ebi *ebi)
>  
>  	if (ebi->majrev < FMC2_VERR_MAJREV_2)
>  		regmap_write(ebi->regmap, FMC2_PCSCNTR, ebi->pcscntr);
> -	else
> +	else if (ebi->access_granted)
>  		regmap_write(ebi->regmap, FMC2_CFGR, ebi->cfgr);

So this is kind of half-allowed-half-not. How is it supposed to work
with !access_granted? You configure some registers but some not. So will
it work or not? If yes, why even needing to write to FMC2_CFGR!

>  }
>  
> @@ -1124,7 +1264,8 @@ static void stm32_fmc2_ebi_enable(struct stm32_fmc2_ebi *ebi)
>  	u32 mask = ebi->majrev < FMC2_VERR_MAJREV_2 ? FMC2_BCR1_FMC2EN :
>  						      FMC2_CFGR_FMC2EN;
>  
> -	regmap_update_bits(ebi->regmap, reg, mask, mask);
> +	if (ebi->access_granted)
> +		regmap_update_bits(ebi->regmap, reg, mask, mask);
>  }
>  
>  static void stm32_fmc2_ebi_disable(struct stm32_fmc2_ebi *ebi)
> @@ -1133,7 +1274,8 @@ static void stm32_fmc2_ebi_disable(struct stm32_fmc2_ebi *ebi)
>  	u32 mask = ebi->majrev < FMC2_VERR_MAJREV_2 ? FMC2_BCR1_FMC2EN :
>  						      FMC2_CFGR_FMC2EN;
>  
> -	regmap_update_bits(ebi->regmap, reg, mask, 0);
> +	if (ebi->access_granted)
> +		regmap_update_bits(ebi->regmap, reg, mask, 0);
>  }
>  
>  static int stm32_fmc2_ebi_setup_cs(struct stm32_fmc2_ebi *ebi,
> @@ -1190,6 +1332,13 @@ static int stm32_fmc2_ebi_parse_dt(struct stm32_fmc2_ebi *ebi)
>  			return -EINVAL;
>  		}
>  
> +		ret = stm32_fmc2_ebi_check_rif(ebi, bank + 1);
> +		if (ret) {
> +			dev_err(dev, "bank access failed: %d\n", bank);
> +			of_node_put(child);
> +			return ret;
> +		}
> +
>  		if (bank < FMC2_MAX_EBI_CE) {
>  			ret = stm32_fmc2_ebi_setup_cs(ebi, child, bank);
>  			if (ret) {
> @@ -1261,6 +1410,23 @@ static int stm32_fmc2_ebi_probe(struct platform_device *pdev)
>  	regmap_read(ebi->regmap, FMC2_VERR, &verr);
>  	ebi->majrev = FIELD_GET(FMC2_VERR_MAJREV, verr);
>  
> +	/* Check if CFGR register can be modified */
> +	ret = stm32_fmc2_ebi_check_rif(ebi, 0);
> +	if (!ret)
> +		ebi->access_granted = true;

I don't understand why you need to store it. If access is not granted,
what else is to do for this driver? Why even probing it? Why enabling
clocks and keep everything running if it cannot work?

> +
> +	/* In case of CFGR is secure, just check that the FMC2 is enabled */
> +	if (!ebi->access_granted) {

This is just "else", isn't it?

> +		u32 sr;
> +
> +		regmap_read(ebi->regmap, FMC2_SR, &sr);
> +		if (sr & FMC2_SR_ISOST) {
> +			dev_err(dev, "FMC2 is not ready to be used.\n");
> +			ret = -EACCES;
> +			goto err_release;
> +		}
> +	}
> +
>  	ret = stm32_fmc2_ebi_parse_dt(ebi);

>  

Best regards,
Krzysztof

Re: [PATCH 06/12] memory: stm32-fmc2-ebi: add RIF support

[Christophe Kerello]

On 2/13/24 08:52, Krzysztof Kozlowski wrote:
> On 12/02/2024 18:48, Christophe Kerello wrote:
>> The FMC2 revision 2 supports security and isolation compliant with
>> the Resource Isolation Framework (RIF). From RIF point of view,
>> the FMC2 is composed of several independent resources, listed below,
>> which can be assigned to different security and compartment domains:
>>    - 0: Common FMC_CFGR register.
>>    - 1: EBI controller for Chip Select 1.
>>    - 2: EBI controller for Chip Select 2.
>>    - 3: EBI controller for Chip Select 3.
>>    - 4: EBI controller for Chip Select 4.
>>    - 5: NAND controller.
>>
> 
> 
>>   	regmap_update_bits(ebi->regmap, reg, mask, setup ? mask : 0);
>>   
>>   	return 0;
>> @@ -990,6 +1023,107 @@ static const struct stm32_fmc2_prop stm32_fmc2_child_props[] = {
>>   	},
>>   };
>>   
>> +static int stm32_fmc2_ebi_check_rif(struct stm32_fmc2_ebi *ebi, u32 resource)
>> +{
>> +	u32 seccfgr, cidcfgr, semcr;
>> +	int cid;
>> +
>> +	if (ebi->majrev < FMC2_VERR_MAJREV_2)
>> +		return 0;
>> +
>> +	if (resource >= FMC2_MAX_RESOURCES)
>> +		return -EINVAL;
>> +
>> +	regmap_read(ebi->regmap, FMC2_SECCFGR, &seccfgr);

Hi Krzysztof,

> 
> No checking of read value?
> 

No, it should never failed.

>> +	if (seccfgr & BIT(resource)) {
> 
> Then on read failure this is random stack junk.
> 
>> +		if (resource)
>> +			dev_err(ebi->dev, "resource %d is configured as secure\n",
>> +				resource);
>> +
>> +		return -EACCES;
>> +	}
>> +
>> +	regmap_read(ebi->regmap, FMC2_CIDCFGR(resource), &cidcfgr);
>> +	if (!(cidcfgr & FMC2_CIDCFGR_CFEN))
>> +		/* CID filtering is turned off: access granted */
>> +		return 0;
>> +
>> +	if (!(cidcfgr & FMC2_CIDCFGR_SEMEN)) {
>> +		/* Static CID mode */
>> +		cid = FIELD_GET(FMC2_CIDCFGR_SCID, cidcfgr);
>> +		if (cid != FMC2_CID1) {
>> +			if (resource)
>> +				dev_err(ebi->dev, "static CID%d set for resource %d\n",
>> +					cid, resource);
>> +
>> +			return -EACCES;
>> +		}
>> +
>> +		return 0;
>> +	}
>> +
>> +	/* Pass-list with semaphore mode */
>> +	if (!(cidcfgr & FMC2_CIDCFGR_SEMWLC1)) {
>> +		if (resource)
>> +			dev_err(ebi->dev, "CID1 is block-listed for resource %d\n",
>> +				resource);
>> +
>> +		return -EACCES;
>> +	}
>> +
>> +	regmap_read(ebi->regmap, FMC2_SEMCR(resource), &semcr);
>> +	if (!(semcr & FMC2_SEMCR_SEM_MUTEX)) {
>> +		regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource),
>> +				   FMC2_SEMCR_SEM_MUTEX, FMC2_SEMCR_SEM_MUTEX);
>> +		regmap_read(ebi->regmap, FMC2_SEMCR(resource), &semcr);
>> +	}
>> +
>> +	cid = FIELD_GET(FMC2_SEMCR_SEMCID, semcr);
>> +	if (cid != FMC2_CID1) {
>> +		if (resource)
>> +			dev_err(ebi->dev, "resource %d is already used by CID%d\n",
>> +				resource, cid);
>> +
>> +		return -EACCES;
>> +	}
>> +
>> +	ebi->sem_taken |= BIT(resource);
>> +
>> +	return 0;
>> +}
>> +
>> +static void stm32_fmc2_ebi_put_sems(struct stm32_fmc2_ebi *ebi)
>> +{
>> +	unsigned int resource;
>> +
>> +	if (ebi->majrev < FMC2_VERR_MAJREV_2)
>> +		return;
>> +
>> +	for (resource = 0; resource < FMC2_MAX_RESOURCES; resource++) {
>> +		if (!(ebi->sem_taken & BIT(resource)))
>> +			continue;
>> +
>> +		regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource),
>> +				   FMC2_SEMCR_SEM_MUTEX, 0);
>> +	}
>> +}
>> +
>> +static void stm32_fmc2_ebi_get_sems(struct stm32_fmc2_ebi *ebi)
>> +{
>> +	unsigned int resource;
>> +
>> +	if (ebi->majrev < FMC2_VERR_MAJREV_2)
>> +		return;
>> +
>> +	for (resource = 0; resource < FMC2_MAX_RESOURCES; resource++) {
>> +		if (!(ebi->sem_taken & BIT(resource)))
>> +			continue;
>> +
>> +		regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource),
>> +				   FMC2_SEMCR_SEM_MUTEX, FMC2_SEMCR_SEM_MUTEX);
>> +	}
>> +}
>> +
>>   static int stm32_fmc2_ebi_parse_prop(struct stm32_fmc2_ebi *ebi,
>>   				     struct device_node *dev_node,
>>   				     const struct stm32_fmc2_prop *prop,
>> @@ -1057,6 +1191,9 @@ static void stm32_fmc2_ebi_save_setup(struct stm32_fmc2_ebi *ebi)
>>   	unsigned int cs;
>>   
>>   	for (cs = 0; cs < FMC2_MAX_EBI_CE; cs++) {
>> +		if (!(ebi->bank_assigned & BIT(cs)))
>> +			continue;
>> +
>>   		regmap_read(ebi->regmap, FMC2_BCR(cs), &ebi->bcr[cs]);
>>   		regmap_read(ebi->regmap, FMC2_BTR(cs), &ebi->btr[cs]);
>>   		regmap_read(ebi->regmap, FMC2_BWTR(cs), &ebi->bwtr[cs]);
>> @@ -1064,7 +1201,7 @@ static void stm32_fmc2_ebi_save_setup(struct stm32_fmc2_ebi *ebi)
>>   
>>   	if (ebi->majrev < FMC2_VERR_MAJREV_2)
>>   		regmap_read(ebi->regmap, FMC2_PCSCNTR, &ebi->pcscntr);
>> -	else
>> +	else if (ebi->access_granted)
>>   		regmap_read(ebi->regmap, FMC2_CFGR, &ebi->cfgr);
>>   }
>>   
>> @@ -1073,6 +1210,9 @@ static void stm32_fmc2_ebi_set_setup(struct stm32_fmc2_ebi *ebi)
>>   	unsigned int cs;
>>   
>>   	for (cs = 0; cs < FMC2_MAX_EBI_CE; cs++) {
>> +		if (!(ebi->bank_assigned & BIT(cs)))
>> +			continue;
>> +
>>   		regmap_write(ebi->regmap, FMC2_BCR(cs), ebi->bcr[cs]);
>>   		regmap_write(ebi->regmap, FMC2_BTR(cs), ebi->btr[cs]);
>>   		regmap_write(ebi->regmap, FMC2_BWTR(cs), ebi->bwtr[cs]);
>> @@ -1080,7 +1220,7 @@ static void stm32_fmc2_ebi_set_setup(struct stm32_fmc2_ebi *ebi)
>>   
>>   	if (ebi->majrev < FMC2_VERR_MAJREV_2)
>>   		regmap_write(ebi->regmap, FMC2_PCSCNTR, ebi->pcscntr);
>> -	else
>> +	else if (ebi->access_granted)
>>   		regmap_write(ebi->regmap, FMC2_CFGR, ebi->cfgr);
> 
> So this is kind of half-allowed-half-not. How is it supposed to work
> with !access_granted? You configure some registers but some not. So will
> it work or not? If yes, why even needing to write to FMC2_CFGR!
> 

This register is considered as one resource and can be protected. If a
companion (like optee_os) has configured this resource as secure, it
means that the driver can not write into this register, and this
register will be handled by the companion. If this register is let as
non secure, the driver can handle this ressource.

>>   }
>>   
>> @@ -1124,7 +1264,8 @@ static void stm32_fmc2_ebi_enable(struct stm32_fmc2_ebi *ebi)
>>   	u32 mask = ebi->majrev < FMC2_VERR_MAJREV_2 ? FMC2_BCR1_FMC2EN :
>>   						      FMC2_CFGR_FMC2EN;
>>   
>> -	regmap_update_bits(ebi->regmap, reg, mask, mask);
>> +	if (ebi->access_granted)
>> +		regmap_update_bits(ebi->regmap, reg, mask, mask);
>>   }
>>   
>>   static void stm32_fmc2_ebi_disable(struct stm32_fmc2_ebi *ebi)
>> @@ -1133,7 +1274,8 @@ static void stm32_fmc2_ebi_disable(struct stm32_fmc2_ebi *ebi)
>>   	u32 mask = ebi->majrev < FMC2_VERR_MAJREV_2 ? FMC2_BCR1_FMC2EN :
>>   						      FMC2_CFGR_FMC2EN;
>>   
>> -	regmap_update_bits(ebi->regmap, reg, mask, 0);
>> +	if (ebi->access_granted)
>> +		regmap_update_bits(ebi->regmap, reg, mask, 0);
>>   }
>>   
>>   static int stm32_fmc2_ebi_setup_cs(struct stm32_fmc2_ebi *ebi,
>> @@ -1190,6 +1332,13 @@ static int stm32_fmc2_ebi_parse_dt(struct stm32_fmc2_ebi *ebi)
>>   			return -EINVAL;
>>   		}
>>   
>> +		ret = stm32_fmc2_ebi_check_rif(ebi, bank + 1);
>> +		if (ret) {
>> +			dev_err(dev, "bank access failed: %d\n", bank);
>> +			of_node_put(child);
>> +			return ret;
>> +		}
>> +
>>   		if (bank < FMC2_MAX_EBI_CE) {
>>   			ret = stm32_fmc2_ebi_setup_cs(ebi, child, bank);
>>   			if (ret) {
>> @@ -1261,6 +1410,23 @@ static int stm32_fmc2_ebi_probe(struct platform_device *pdev)
>>   	regmap_read(ebi->regmap, FMC2_VERR, &verr);
>>   	ebi->majrev = FIELD_GET(FMC2_VERR_MAJREV, verr);
>>   
>> +	/* Check if CFGR register can be modified */
>> +	ret = stm32_fmc2_ebi_check_rif(ebi, 0);
>> +	if (!ret)
>> +		ebi->access_granted = true;
> 
> I don't understand why you need to store it. If access is not granted,
> what else is to do for this driver? Why even probing it? Why enabling
> clocks and keep everything running if it cannot work?
> 

CFGR register contains the bit that is enabling the IP. CFGR register
can be set to secure when all the others ressources can be set to non
secure. If CFGR register is secured, then we check that the IP has been
enabled by the companion. If it is the case, PSRAM controller or NAND
controller set as non secure can be used. And, if CFGR register is
secured and the IP is not enabled, the probe of the driver fails.

>> +
>> +	/* In case of CFGR is secure, just check that the FMC2 is enabled */
>> +	if (!ebi->access_granted) {
> 
> This is just "else", isn't it?

Yes, can be "else".

Regards,
Christophe Kerello.

> 
>> +		u32 sr;
>> +
>> +		regmap_read(ebi->regmap, FMC2_SR, &sr);
>> +		if (sr & FMC2_SR_ISOST) {
>> +			dev_err(dev, "FMC2 is not ready to be used.\n");
>> +			ret = -EACCES;
>> +			goto err_release;
>> +		}
>> +	}
>> +
>>   	ret = stm32_fmc2_ebi_parse_dt(ebi);
> 
>>   
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 06/12] memory: stm32-fmc2-ebi: add RIF support

[Krzysztof Kozlowski]

On 13/02/2024 14:15, Christophe Kerello wrote:
>>> +
>>> +	if (ebi->majrev < FMC2_VERR_MAJREV_2)
>>> +		return 0;
>>> +
>>> +	if (resource >= FMC2_MAX_RESOURCES)
>>> +		return -EINVAL;
>>> +
>>> +	regmap_read(ebi->regmap, FMC2_SECCFGR, &seccfgr);
> 
> Hi Krzysztof,
> 
>>
>> No checking of read value?
>>
> 
> No, it should never failed.

And you tested that neither smatch, sparse nor Coverity report here
warnings?

> 
>>> +	if (seccfgr & BIT(resource)) {
>>
>> Then on read failure this is random stack junk.
>>
>>> +		if (resource)
>>> +			dev_err(ebi->dev, "resource %d is configured as secure\n",
>>> +				resource);
>>> +
>>> +		return -EACCES;
>>> +	}
>>> +
>>> +	regmap_read(ebi->regmap, FMC2_CIDCFGR(resource), &cidcfgr);
>>> +	if (!(cidcfgr & FMC2_CIDCFGR_CFEN))
>>> +		/* CID filtering is turned off: access granted */
>>> +		return 0;
>>> +
>>> +	if (!(cidcfgr & FMC2_CIDCFGR_SEMEN)) {
>>> +		/* Static CID mode */
>>> +		cid = FIELD_GET(FMC2_CIDCFGR_SCID, cidcfgr);
>>> +		if (cid != FMC2_CID1) {
>>> +			if (resource)
>>> +				dev_err(ebi->dev, "static CID%d set for resource %d\n",
>>> +					cid, resource);
>>> +
>>> +			return -EACCES;
>>> +		}
>>> +
>>> +		return 0;
>>> +	}
>>> +
>>> +	/* Pass-list with semaphore mode */
>>> +	if (!(cidcfgr & FMC2_CIDCFGR_SEMWLC1)) {
>>> +		if (resource)
>>> +			dev_err(ebi->dev, "CID1 is block-listed for resource %d\n",
>>> +				resource);
>>> +
>>> +		return -EACCES;
>>> +	}
>>> +
>>> +	regmap_read(ebi->regmap, FMC2_SEMCR(resource), &semcr);
>>> +	if (!(semcr & FMC2_SEMCR_SEM_MUTEX)) {
>>> +		regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource),
>>> +				   FMC2_SEMCR_SEM_MUTEX, FMC2_SEMCR_SEM_MUTEX);
>>> +		regmap_read(ebi->regmap, FMC2_SEMCR(resource), &semcr);
>>> +	}
>>> +
>>> +	cid = FIELD_GET(FMC2_SEMCR_SEMCID, semcr);
>>> +	if (cid != FMC2_CID1) {
>>> +		if (resource)
>>> +			dev_err(ebi->dev, "resource %d is already used by CID%d\n",
>>> +				resource, cid);
>>> +
>>> +		return -EACCES;
>>> +	}
>>> +
>>> +	ebi->sem_taken |= BIT(resource);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static void stm32_fmc2_ebi_put_sems(struct stm32_fmc2_ebi *ebi)
>>> +{
>>> +	unsigned int resource;
>>> +
>>> +	if (ebi->majrev < FMC2_VERR_MAJREV_2)
>>> +		return;
>>> +
>>> +	for (resource = 0; resource < FMC2_MAX_RESOURCES; resource++) {
>>> +		if (!(ebi->sem_taken & BIT(resource)))
>>> +			continue;
>>> +
>>> +		regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource),
>>> +				   FMC2_SEMCR_SEM_MUTEX, 0);
>>> +	}
>>> +}
>>> +
>>> +static void stm32_fmc2_ebi_get_sems(struct stm32_fmc2_ebi *ebi)
>>> +{
>>> +	unsigned int resource;
>>> +
>>> +	if (ebi->majrev < FMC2_VERR_MAJREV_2)
>>> +		return;
>>> +
>>> +	for (resource = 0; resource < FMC2_MAX_RESOURCES; resource++) {
>>> +		if (!(ebi->sem_taken & BIT(resource)))
>>> +			continue;
>>> +
>>> +		regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource),
>>> +				   FMC2_SEMCR_SEM_MUTEX, FMC2_SEMCR_SEM_MUTEX);
>>> +	}
>>> +}
>>> +
>>>   static int stm32_fmc2_ebi_parse_prop(struct stm32_fmc2_ebi *ebi,
>>>   				     struct device_node *dev_node,
>>>   				     const struct stm32_fmc2_prop *prop,
>>> @@ -1057,6 +1191,9 @@ static void stm32_fmc2_ebi_save_setup(struct stm32_fmc2_ebi *ebi)
>>>   	unsigned int cs;
>>>   
>>>   	for (cs = 0; cs < FMC2_MAX_EBI_CE; cs++) {
>>> +		if (!(ebi->bank_assigned & BIT(cs)))
>>> +			continue;
>>> +
>>>   		regmap_read(ebi->regmap, FMC2_BCR(cs), &ebi->bcr[cs]);
>>>   		regmap_read(ebi->regmap, FMC2_BTR(cs), &ebi->btr[cs]);
>>>   		regmap_read(ebi->regmap, FMC2_BWTR(cs), &ebi->bwtr[cs]);
>>> @@ -1064,7 +1201,7 @@ static void stm32_fmc2_ebi_save_setup(struct stm32_fmc2_ebi *ebi)
>>>   
>>>   	if (ebi->majrev < FMC2_VERR_MAJREV_2)
>>>   		regmap_read(ebi->regmap, FMC2_PCSCNTR, &ebi->pcscntr);
>>> -	else
>>> +	else if (ebi->access_granted)
>>>   		regmap_read(ebi->regmap, FMC2_CFGR, &ebi->cfgr);
>>>   }
>>>   
>>> @@ -1073,6 +1210,9 @@ static void stm32_fmc2_ebi_set_setup(struct stm32_fmc2_ebi *ebi)
>>>   	unsigned int cs;
>>>   
>>>   	for (cs = 0; cs < FMC2_MAX_EBI_CE; cs++) {
>>> +		if (!(ebi->bank_assigned & BIT(cs)))
>>> +			continue;
>>> +
>>>   		regmap_write(ebi->regmap, FMC2_BCR(cs), ebi->bcr[cs]);
>>>   		regmap_write(ebi->regmap, FMC2_BTR(cs), ebi->btr[cs]);
>>>   		regmap_write(ebi->regmap, FMC2_BWTR(cs), ebi->bwtr[cs]);
>>> @@ -1080,7 +1220,7 @@ static void stm32_fmc2_ebi_set_setup(struct stm32_fmc2_ebi *ebi)
>>>   
>>>   	if (ebi->majrev < FMC2_VERR_MAJREV_2)
>>>   		regmap_write(ebi->regmap, FMC2_PCSCNTR, ebi->pcscntr);
>>> -	else
>>> +	else if (ebi->access_granted)
>>>   		regmap_write(ebi->regmap, FMC2_CFGR, ebi->cfgr);
>>
>> So this is kind of half-allowed-half-not. How is it supposed to work
>> with !access_granted? You configure some registers but some not. So will
>> it work or not? If yes, why even needing to write to FMC2_CFGR!
>>
> 
> This register is considered as one resource and can be protected. If a
> companion (like optee_os) has configured this resource as secure, it
> means that the driver can not write into this register, and this
> register will be handled by the companion. If this register is let as
> non secure, the driver can handle this ressource.

So this is not an error? Other places print errors and return -EACCESS,
so that's a bit confusing me.


Best regards,
Krzysztof

Re: [PATCH 06/12] memory: stm32-fmc2-ebi: add RIF support

[Christophe Kerello]

On 2/14/24 11:07, Krzysztof Kozlowski wrote:
> On 13/02/2024 14:15, Christophe Kerello wrote:
>>>> +
>>>> +	if (ebi->majrev < FMC2_VERR_MAJREV_2)
>>>> +		return 0;
>>>> +
>>>> +	if (resource >= FMC2_MAX_RESOURCES)
>>>> +		return -EINVAL;
>>>> +
>>>> +	regmap_read(ebi->regmap, FMC2_SECCFGR, &seccfgr);
>>
>> Hi Krzysztof,
>>
>>>
>>> No checking of read value?
>>>
>>
>> No, it should never failed.
> 
> And you tested that neither smatch, sparse nor Coverity report here
> warnings?
> 

Hi Krzysztof,

There is a lot of driver in the Kernel that are using same 
implementation, and I am surprised to not have had this comment 4 years 
ago when the driver was introduced.

So, how should I proceed? Shall I initialize all local variables used by 
regmap_read? Or shall I check the return value of regmap_read?
And, as there is a lot of regmap_read call in this driver, shall I fix 
them in a separate patch?

>>
>>>> +	if (seccfgr & BIT(resource)) {
>>>
>>> Then on read failure this is random stack junk.
>>>
>>>> +		if (resource)
>>>> +			dev_err(ebi->dev, "resource %d is configured as secure\n",
>>>> +				resource);
>>>> +
>>>> +		return -EACCES;
>>>> +	}
>>>> +
>>>> +	regmap_read(ebi->regmap, FMC2_CIDCFGR(resource), &cidcfgr);
>>>> +	if (!(cidcfgr & FMC2_CIDCFGR_CFEN))
>>>> +		/* CID filtering is turned off: access granted */
>>>> +		return 0;
>>>> +
>>>> +	if (!(cidcfgr & FMC2_CIDCFGR_SEMEN)) {
>>>> +		/* Static CID mode */
>>>> +		cid = FIELD_GET(FMC2_CIDCFGR_SCID, cidcfgr);
>>>> +		if (cid != FMC2_CID1) {
>>>> +			if (resource)
>>>> +				dev_err(ebi->dev, "static CID%d set for resource %d\n",
>>>> +					cid, resource);
>>>> +
>>>> +			return -EACCES;
>>>> +		}
>>>> +
>>>> +		return 0;
>>>> +	}
>>>> +
>>>> +	/* Pass-list with semaphore mode */
>>>> +	if (!(cidcfgr & FMC2_CIDCFGR_SEMWLC1)) {
>>>> +		if (resource)
>>>> +			dev_err(ebi->dev, "CID1 is block-listed for resource %d\n",
>>>> +				resource);
>>>> +
>>>> +		return -EACCES;
>>>> +	}
>>>> +
>>>> +	regmap_read(ebi->regmap, FMC2_SEMCR(resource), &semcr);
>>>> +	if (!(semcr & FMC2_SEMCR_SEM_MUTEX)) {
>>>> +		regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource),
>>>> +				   FMC2_SEMCR_SEM_MUTEX, FMC2_SEMCR_SEM_MUTEX);
>>>> +		regmap_read(ebi->regmap, FMC2_SEMCR(resource), &semcr);
>>>> +	}
>>>> +
>>>> +	cid = FIELD_GET(FMC2_SEMCR_SEMCID, semcr);
>>>> +	if (cid != FMC2_CID1) {
>>>> +		if (resource)
>>>> +			dev_err(ebi->dev, "resource %d is already used by CID%d\n",
>>>> +				resource, cid);
>>>> +
>>>> +		return -EACCES;
>>>> +	}
>>>> +
>>>> +	ebi->sem_taken |= BIT(resource);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static void stm32_fmc2_ebi_put_sems(struct stm32_fmc2_ebi *ebi)
>>>> +{
>>>> +	unsigned int resource;
>>>> +
>>>> +	if (ebi->majrev < FMC2_VERR_MAJREV_2)
>>>> +		return;
>>>> +
>>>> +	for (resource = 0; resource < FMC2_MAX_RESOURCES; resource++) {
>>>> +		if (!(ebi->sem_taken & BIT(resource)))
>>>> +			continue;
>>>> +
>>>> +		regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource),
>>>> +				   FMC2_SEMCR_SEM_MUTEX, 0);
>>>> +	}
>>>> +}
>>>> +
>>>> +static void stm32_fmc2_ebi_get_sems(struct stm32_fmc2_ebi *ebi)
>>>> +{
>>>> +	unsigned int resource;
>>>> +
>>>> +	if (ebi->majrev < FMC2_VERR_MAJREV_2)
>>>> +		return;
>>>> +
>>>> +	for (resource = 0; resource < FMC2_MAX_RESOURCES; resource++) {
>>>> +		if (!(ebi->sem_taken & BIT(resource)))
>>>> +			continue;
>>>> +
>>>> +		regmap_update_bits(ebi->regmap, FMC2_SEMCR(resource),
>>>> +				   FMC2_SEMCR_SEM_MUTEX, FMC2_SEMCR_SEM_MUTEX);
>>>> +	}
>>>> +}
>>>> +
>>>>    static int stm32_fmc2_ebi_parse_prop(struct stm32_fmc2_ebi *ebi,
>>>>    				     struct device_node *dev_node,
>>>>    				     const struct stm32_fmc2_prop *prop,
>>>> @@ -1057,6 +1191,9 @@ static void stm32_fmc2_ebi_save_setup(struct stm32_fmc2_ebi *ebi)
>>>>    	unsigned int cs;
>>>>    
>>>>    	for (cs = 0; cs < FMC2_MAX_EBI_CE; cs++) {
>>>> +		if (!(ebi->bank_assigned & BIT(cs)))
>>>> +			continue;
>>>> +
>>>>    		regmap_read(ebi->regmap, FMC2_BCR(cs), &ebi->bcr[cs]);
>>>>    		regmap_read(ebi->regmap, FMC2_BTR(cs), &ebi->btr[cs]);
>>>>    		regmap_read(ebi->regmap, FMC2_BWTR(cs), &ebi->bwtr[cs]);
>>>> @@ -1064,7 +1201,7 @@ static void stm32_fmc2_ebi_save_setup(struct stm32_fmc2_ebi *ebi)
>>>>    
>>>>    	if (ebi->majrev < FMC2_VERR_MAJREV_2)
>>>>    		regmap_read(ebi->regmap, FMC2_PCSCNTR, &ebi->pcscntr);
>>>> -	else
>>>> +	else if (ebi->access_granted)
>>>>    		regmap_read(ebi->regmap, FMC2_CFGR, &ebi->cfgr);
>>>>    }
>>>>    
>>>> @@ -1073,6 +1210,9 @@ static void stm32_fmc2_ebi_set_setup(struct stm32_fmc2_ebi *ebi)
>>>>    	unsigned int cs;
>>>>    
>>>>    	for (cs = 0; cs < FMC2_MAX_EBI_CE; cs++) {
>>>> +		if (!(ebi->bank_assigned & BIT(cs)))
>>>> +			continue;
>>>> +
>>>>    		regmap_write(ebi->regmap, FMC2_BCR(cs), ebi->bcr[cs]);
>>>>    		regmap_write(ebi->regmap, FMC2_BTR(cs), ebi->btr[cs]);
>>>>    		regmap_write(ebi->regmap, FMC2_BWTR(cs), ebi->bwtr[cs]);
>>>> @@ -1080,7 +1220,7 @@ static void stm32_fmc2_ebi_set_setup(struct stm32_fmc2_ebi *ebi)
>>>>    
>>>>    	if (ebi->majrev < FMC2_VERR_MAJREV_2)
>>>>    		regmap_write(ebi->regmap, FMC2_PCSCNTR, ebi->pcscntr);
>>>> -	else
>>>> +	else if (ebi->access_granted)
>>>>    		regmap_write(ebi->regmap, FMC2_CFGR, ebi->cfgr);
>>>
>>> So this is kind of half-allowed-half-not. How is it supposed to work
>>> with !access_granted? You configure some registers but some not. So will
>>> it work or not? If yes, why even needing to write to FMC2_CFGR!
>>>
>>
>> This register is considered as one resource and can be protected. If a
>> companion (like optee_os) has configured this resource as secure, it
>> means that the driver can not write into this register, and this
>> register will be handled by the companion. If this register is let as
>> non secure, the driver can handle this ressource.
> 
> So this is not an error? Other places print errors and return -EACCESS,
> so that's a bit confusing me.
> 

It is not an error. We are saving registers values to restore them on 
low power cases. If registers are set as secure, it is the 
responsibility of the companion to restore them when it is the 
responsibility of the non secure driver to restore them if they are 
configured as non secure.

Regards,
Christophe Kerello.

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 06/12] memory: stm32-fmc2-ebi: add RIF support

[Krzysztof Kozlowski]

On 15/02/2024 10:00, Christophe Kerello wrote:
> 
> 
> On 2/14/24 11:07, Krzysztof Kozlowski wrote:
>> On 13/02/2024 14:15, Christophe Kerello wrote:
>>>>> +
>>>>> +	if (ebi->majrev < FMC2_VERR_MAJREV_2)
>>>>> +		return 0;
>>>>> +
>>>>> +	if (resource >= FMC2_MAX_RESOURCES)
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	regmap_read(ebi->regmap, FMC2_SECCFGR, &seccfgr);
>>>
>>> Hi Krzysztof,
>>>
>>>>
>>>> No checking of read value?
>>>>
>>>
>>> No, it should never failed.
>>
>> And you tested that neither smatch, sparse nor Coverity report here
>> warnings?
>>
> 
> Hi Krzysztof,
> 
> There is a lot of driver in the Kernel that are using same 
> implementation, and I am surprised to not have had this comment 4 years 
> ago when the driver was introduced.

Really? Care to give some pointers? Heh, I don't know what to respond to
it. Either you say that my comment is incorrect or you say that it's
okay to sneak poor code if no one notices? We can argue on the first,
whether my comment is reasonable or not. But if you claim that previous
poor choice of code is argument of bringing more of such poor choices,
then we are done here. It's the oldest argument: someone did it that
way, so I can do the same. Nope.

> 
> So, how should I proceed? Shall I initialize all local variables used by 
> regmap_read? Or shall I check the return value of regmap_read?
> And, as there is a lot of regmap_read call in this driver, shall I fix 
> them in a separate patch?

regmap operations, depending on the regmap used, can fail. Most of the
errors are result of static configuration, e.g. alignment, regmap in
cache mode etc. Then certain regmap implementations can produce errors,
which is not a static condition but dynamic.

You have neither error checking nor value initialization. You risk here
to have quite tricky to find, unnoticeable bugs, if there any mistake
leading to regmap errors.

Indeed neither smatch nor sparse report this as error currently, but
maybe that's their limitation?


Best regards,
Krzysztof

Re: [PATCH 06/12] memory: stm32-fmc2-ebi: add RIF support

[Christophe Kerello]

On 2/15/24 19:56, Krzysztof Kozlowski wrote:
> On 15/02/2024 10:00, Christophe Kerello wrote:
>>
>>
>> On 2/14/24 11:07, Krzysztof Kozlowski wrote:
>>> On 13/02/2024 14:15, Christophe Kerello wrote:
>>>>>> +
>>>>>> +	if (ebi->majrev < FMC2_VERR_MAJREV_2)
>>>>>> +		return 0;
>>>>>> +
>>>>>> +	if (resource >= FMC2_MAX_RESOURCES)
>>>>>> +		return -EINVAL;
>>>>>> +
>>>>>> +	regmap_read(ebi->regmap, FMC2_SECCFGR, &seccfgr);
>>>>
>>>> Hi Krzysztof,
>>>>
>>>>>
>>>>> No checking of read value?
>>>>>
>>>>
>>>> No, it should never failed.
>>>
>>> And you tested that neither smatch, sparse nor Coverity report here
>>> warnings?
>>>
>>
>> Hi Krzysztof,
>>
>> There is a lot of driver in the Kernel that are using same
>> implementation, and I am surprised to not have had this comment 4 years
>> ago when the driver was introduced.
> 
> Really? Care to give some pointers? Heh, I don't know what to respond to
> it. Either you say that my comment is incorrect or you say that it's
> okay to sneak poor code if no one notices? We can argue on the first,
> whether my comment is reasonable or not. But if you claim that previous
> poor choice of code is argument of bringing more of such poor choices,
> then we are done here. It's the oldest argument: someone did it that
> way, so I can do the same. Nope.
> 
>>
>> So, how should I proceed? Shall I initialize all local variables used by
>> regmap_read? Or shall I check the return value of regmap_read?
>> And, as there is a lot of regmap_read call in this driver, shall I fix
>> them in a separate patch?
> 
> regmap operations, depending on the regmap used, can fail. Most of the
> errors are result of static configuration, e.g. alignment, regmap in
> cache mode etc. Then certain regmap implementations can produce errors,
> which is not a static condition but dynamic.
> 
> You have neither error checking nor value initialization. You risk here
> to have quite tricky to find, unnoticeable bugs, if there any mistake
> leading to regmap errors.
> 
> Indeed neither smatch nor sparse report this as error currently, but
> maybe that's their limitation?
> 

Hi Krzysztof,

I will check the return value of regmap_read API.

Regards,
Christophe Kerello.

> 
> Best regards,
> Krzysztof
> 

[PATCH 07/12] memory: stm32-fmc2-ebi: add runtime PM support

[Christophe Kerello]

Add runtime PM support in FMC2 ebi driver to be able to manage GENPD
support when it will be enabled.

Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
---
 drivers/memory/stm32-fmc2-ebi.c | 40 ++++++++++++++++++++++++++-------
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/drivers/memory/stm32-fmc2-ebi.c b/drivers/memory/stm32-fmc2-ebi.c
index 04248c15832f..8c30e56be3b0 100644
--- a/drivers/memory/stm32-fmc2-ebi.c
+++ b/drivers/memory/stm32-fmc2-ebi.c
@@ -11,6 +11,7 @@
 #include <linux/of_platform.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/reset.h>
 
@@ -1381,6 +1382,7 @@ static int stm32_fmc2_ebi_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	ebi->dev = dev;
+	platform_set_drvdata(pdev, ebi);
 
 	ebi->data = of_device_get_match_data(dev);
 	if (!ebi->data)
@@ -1398,10 +1400,14 @@ static int stm32_fmc2_ebi_probe(struct platform_device *pdev)
 	if (PTR_ERR(rstc) == -EPROBE_DEFER)
 		return -EPROBE_DEFER;
 
-	ret = clk_prepare_enable(ebi->clk);
+	ret = devm_pm_runtime_enable(dev);
 	if (ret)
 		return ret;
 
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret < 0)
+		return ret;
+
 	if (!IS_ERR(rstc)) {
 		reset_control_assert(rstc);
 		reset_control_deassert(rstc);
@@ -1432,7 +1438,6 @@ static int stm32_fmc2_ebi_probe(struct platform_device *pdev)
 		goto err_release;
 
 	stm32_fmc2_ebi_save_setup(ebi);
-	platform_set_drvdata(pdev, ebi);
 
 	return 0;
 
@@ -1440,7 +1445,7 @@ static int stm32_fmc2_ebi_probe(struct platform_device *pdev)
 	stm32_fmc2_ebi_disable_banks(ebi);
 	stm32_fmc2_ebi_disable(ebi);
 	stm32_fmc2_ebi_put_sems(ebi);
-	clk_disable_unprepare(ebi->clk);
+	pm_runtime_put_sync_suspend(dev);
 
 	return ret;
 }
@@ -1453,7 +1458,23 @@ static void stm32_fmc2_ebi_remove(struct platform_device *pdev)
 	stm32_fmc2_ebi_disable_banks(ebi);
 	stm32_fmc2_ebi_disable(ebi);
 	stm32_fmc2_ebi_put_sems(ebi);
+	pm_runtime_put_sync_suspend(&pdev->dev);
+}
+
+static int __maybe_unused stm32_fmc2_ebi_runtime_suspend(struct device *dev)
+{
+	struct stm32_fmc2_ebi *ebi = dev_get_drvdata(dev);
+
 	clk_disable_unprepare(ebi->clk);
+
+	return 0;
+}
+
+static int __maybe_unused stm32_fmc2_ebi_runtime_resume(struct device *dev)
+{
+	struct stm32_fmc2_ebi *ebi = dev_get_drvdata(dev);
+
+	return clk_prepare_enable(ebi->clk);
 }
 
 static int __maybe_unused stm32_fmc2_ebi_suspend(struct device *dev)
@@ -1462,7 +1483,7 @@ static int __maybe_unused stm32_fmc2_ebi_suspend(struct device *dev)
 
 	stm32_fmc2_ebi_disable(ebi);
 	stm32_fmc2_ebi_put_sems(ebi);
-	clk_disable_unprepare(ebi->clk);
+	pm_runtime_put_sync_suspend(dev);
 	pinctrl_pm_select_sleep_state(dev);
 
 	return 0;
@@ -1475,8 +1496,8 @@ static int __maybe_unused stm32_fmc2_ebi_resume(struct device *dev)
 
 	pinctrl_pm_select_default_state(dev);
 
-	ret = clk_prepare_enable(ebi->clk);
-	if (ret)
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret < 0)
 		return ret;
 
 	stm32_fmc2_ebi_get_sems(ebi);
@@ -1486,8 +1507,11 @@ static int __maybe_unused stm32_fmc2_ebi_resume(struct device *dev)
 	return 0;
 }
 
-static SIMPLE_DEV_PM_OPS(stm32_fmc2_ebi_pm_ops, stm32_fmc2_ebi_suspend,
-			 stm32_fmc2_ebi_resume);
+static const struct dev_pm_ops stm32_fmc2_ebi_pm_ops = {
+	SET_RUNTIME_PM_OPS(stm32_fmc2_ebi_runtime_suspend,
+			   stm32_fmc2_ebi_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(stm32_fmc2_ebi_suspend, stm32_fmc2_ebi_resume)
+};
 
 static const struct stm32_fmc2_ebi_data stm32_fmc2_ebi_mp1_data = {
 	.rnb_for_nand = false,
-- 
2.25.1

Re: [PATCH 07/12] memory: stm32-fmc2-ebi: add runtime PM support

[Krzysztof Kozlowski]

On 12/02/2024 18:48, Christophe Kerello wrote:
> Add runtime PM support in FMC2 ebi driver to be able to manage GENPD
> support when it will be enabled.

You don't do any real runtime PM here (turning on PM domain on probe is
not real PM), so please explain what is the goal of it and say that it
is basic RPM for keeping domain on.

> 
> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
> ---
>  drivers/memory/stm32-fmc2-ebi.c | 40 ++++++++++++++++++++++++++-------
>  1 file changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/memory/stm32-fmc2-ebi.c b/drivers/memory/stm32-fmc2-ebi.c
> index 04248c15832f..8c30e56be3b0 100644
> --- a/drivers/memory/stm32-fmc2-ebi.c
> +++ b/drivers/memory/stm32-fmc2-ebi.c
> @@ -11,6 +11,7 @@
>  #include <linux/of_platform.h>
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/regmap.h>
>  #include <linux/reset.h>
>  
> @@ -1381,6 +1382,7 @@ static int stm32_fmc2_ebi_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	ebi->dev = dev;
> +	platform_set_drvdata(pdev, ebi);

Does not look related.


Best regards,
Krzysztof

Re: [PATCH 07/12] memory: stm32-fmc2-ebi: add runtime PM support

[Christophe Kerello]

On 2/13/24 08:59, Krzysztof Kozlowski wrote:
> On 12/02/2024 18:48, Christophe Kerello wrote:
>> Add runtime PM support in FMC2 ebi driver to be able to manage GENPD
>> support when it will be enabled.
> 

Hi Krzysztof,

> You don't do any real runtime PM here (turning on PM domain on probe is
> not real PM), so please explain what is the goal of it and say that it
> is basic RPM for keeping domain on.
> 

Yes, you are true, I will modify the commit message.

>>
>> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
>> ---
>>   drivers/memory/stm32-fmc2-ebi.c | 40 ++++++++++++++++++++++++++-------
>>   1 file changed, 32 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/memory/stm32-fmc2-ebi.c b/drivers/memory/stm32-fmc2-ebi.c
>> index 04248c15832f..8c30e56be3b0 100644
>> --- a/drivers/memory/stm32-fmc2-ebi.c
>> +++ b/drivers/memory/stm32-fmc2-ebi.c
>> @@ -11,6 +11,7 @@
>>   #include <linux/of_platform.h>
>>   #include <linux/pinctrl/consumer.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>>   #include <linux/regmap.h>
>>   #include <linux/reset.h>
>>   
>> @@ -1381,6 +1382,7 @@ static int stm32_fmc2_ebi_probe(struct platform_device *pdev)
>>   		return -ENOMEM;
>>   
>>   	ebi->dev = dev;
>> +	platform_set_drvdata(pdev, ebi);
> 
> Does not look related.
> 

With pm_runtime_resume_and_get API now called, 
stm32_fmc2_ebi_runtime_resume needs ebi data to enable the clock. It
means that the platform data has to be set before this call.

Regards,
Christophe Kerello.

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 07/12] memory: stm32-fmc2-ebi: add runtime PM support

[Krzysztof Kozlowski]

On 13/02/2024 14:31, Christophe Kerello wrote:
>>>   
>>> @@ -1381,6 +1382,7 @@ static int stm32_fmc2_ebi_probe(struct platform_device *pdev)
>>>   		return -ENOMEM;
>>>   
>>>   	ebi->dev = dev;
>>> +	platform_set_drvdata(pdev, ebi);
>>
>> Does not look related.
>>
> 
> With pm_runtime_resume_and_get API now called, 
> stm32_fmc2_ebi_runtime_resume needs ebi data to enable the clock. It
> means that the platform data has to be set before this call.
> 

Ah, OK.

Best regards,
Krzysztof

[PATCH 08/12] dt-bindings: mtd: st,stm32: add MP25 support

[Christophe Kerello]

Add 2 new compatible strings to support MP25 SOC.
MP25 SOC supports up to 4 chip select.

Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
---
 .../bindings/mtd/st,stm32-fmc2-nand.yaml      | 58 ++++++++++++++++++-
 1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mtd/st,stm32-fmc2-nand.yaml b/Documentation/devicetree/bindings/mtd/st,stm32-fmc2-nand.yaml
index e72cb5bacaf0..33a753c8877b 100644
--- a/Documentation/devicetree/bindings/mtd/st,stm32-fmc2-nand.yaml
+++ b/Documentation/devicetree/bindings/mtd/st,stm32-fmc2-nand.yaml
@@ -14,10 +14,12 @@ properties:
     enum:
       - st,stm32mp15-fmc2
       - st,stm32mp1-fmc2-nfc
+      - st,stm32mp25-fmc2
+      - st,stm32mp25-fmc2-nfc
 
   reg:
     minItems: 6
-    maxItems: 7
+    maxItems: 13
 
   interrupts:
     maxItems: 1
@@ -92,6 +94,60 @@ allOf:
             - description: Chip select 1 command
             - description: Chip select 1 address space
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: st,stm32mp25-fmc2
+    then:
+      properties:
+        reg:
+          items:
+            - description: Registers
+            - description: Chip select 0 data
+            - description: Chip select 0 command
+            - description: Chip select 0 address space
+            - description: Chip select 1 data
+            - description: Chip select 1 command
+            - description: Chip select 1 address space
+            - description: Chip select 2 data
+            - description: Chip select 2 command
+            - description: Chip select 2 address space
+            - description: Chip select 3 data
+            - description: Chip select 3 command
+            - description: Chip select 3 address space
+
+        clocks:
+          maxItems: 1
+
+        resets:
+          maxItems: 1
+
+      required:
+        - clocks
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: st,stm32mp25-fmc2-nfc
+    then:
+      properties:
+        reg:
+          items:
+            - description: Chip select 0 data
+            - description: Chip select 0 command
+            - description: Chip select 0 address space
+            - description: Chip select 1 data
+            - description: Chip select 1 command
+            - description: Chip select 1 address space
+            - description: Chip select 2 data
+            - description: Chip select 2 command
+            - description: Chip select 2 address space
+            - description: Chip select 3 data
+            - description: Chip select 3 command
+            - description: Chip select 3 address space
+
 required:
   - compatible
   - reg
-- 
2.25.1

Re: [PATCH 08/12] dt-bindings: mtd: st,stm32: add MP25 support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3281 bytes --]

On Mon, Feb 12, 2024 at 06:48:18PM +0100, Christophe Kerello wrote:
> Add 2 new compatible strings to support MP25 SOC.
> MP25 SOC supports up to 4 chip select.

Again, please explain why the new device is not compatible with the
existing ones. Also, please explain why two compatibles are required for
the mp25.

Thanks,
Conor.

> 
> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
> ---
>  .../bindings/mtd/st,stm32-fmc2-nand.yaml      | 58 ++++++++++++++++++-
>  1 file changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/st,stm32-fmc2-nand.yaml b/Documentation/devicetree/bindings/mtd/st,stm32-fmc2-nand.yaml
> index e72cb5bacaf0..33a753c8877b 100644
> --- a/Documentation/devicetree/bindings/mtd/st,stm32-fmc2-nand.yaml
> +++ b/Documentation/devicetree/bindings/mtd/st,stm32-fmc2-nand.yaml
> @@ -14,10 +14,12 @@ properties:
>      enum:
>        - st,stm32mp15-fmc2
>        - st,stm32mp1-fmc2-nfc
> +      - st,stm32mp25-fmc2
> +      - st,stm32mp25-fmc2-nfc
>  
>    reg:
>      minItems: 6
> -    maxItems: 7
> +    maxItems: 13
>  
>    interrupts:
>      maxItems: 1
> @@ -92,6 +94,60 @@ allOf:
>              - description: Chip select 1 command
>              - description: Chip select 1 address space
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: st,stm32mp25-fmc2
> +    then:
> +      properties:
> +        reg:
> +          items:
> +            - description: Registers
> +            - description: Chip select 0 data
> +            - description: Chip select 0 command
> +            - description: Chip select 0 address space
> +            - description: Chip select 1 data
> +            - description: Chip select 1 command
> +            - description: Chip select 1 address space
> +            - description: Chip select 2 data
> +            - description: Chip select 2 command
> +            - description: Chip select 2 address space
> +            - description: Chip select 3 data
> +            - description: Chip select 3 command
> +            - description: Chip select 3 address space
> +
> +        clocks:
> +          maxItems: 1
> +
> +        resets:
> +          maxItems: 1
> +
> +      required:
> +        - clocks
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: st,stm32mp25-fmc2-nfc
> +    then:
> +      properties:
> +        reg:
> +          items:
> +            - description: Chip select 0 data
> +            - description: Chip select 0 command
> +            - description: Chip select 0 address space
> +            - description: Chip select 1 data
> +            - description: Chip select 1 command
> +            - description: Chip select 1 address space
> +            - description: Chip select 2 data
> +            - description: Chip select 2 command
> +            - description: Chip select 2 address space
> +            - description: Chip select 3 data
> +            - description: Chip select 3 command
> +            - description: Chip select 3 address space
> +
>  required:
>    - compatible
>    - reg
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 08/12] dt-bindings: mtd: st,stm32: add MP25 support

[Christophe Kerello]

On 2/12/24 19:38, Conor Dooley wrote:
> On Mon, Feb 12, 2024 at 06:48:18PM +0100, Christophe Kerello wrote:
>> Add 2 new compatible strings to support MP25 SOC.
>> MP25 SOC supports up to 4 chip select.
> 
> Again, please explain why the new device is not compatible with the
> existing ones. Also, please explain why two compatibles are required for
> the mp25.
> 

Hi Conor,

FMC2 IP supports up to 4 chip select. On MP1 SoC, only 2 of them are 
available when on MP25 SoC, the 4 chip select are available.

MP1 SoC also embeds revision 1.1 of the FMC2 IP when MP25 SoC embeds 
revision 2.0 of the FMC2 IP.

I will add this explanation in the commit message.

Regards,
Christophe Kerello.

> Thanks,
> Conor.
> 
>>
>> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
>> ---
>>   .../bindings/mtd/st,stm32-fmc2-nand.yaml      | 58 ++++++++++++++++++-
>>   1 file changed, 57 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/st,stm32-fmc2-nand.yaml b/Documentation/devicetree/bindings/mtd/st,stm32-fmc2-nand.yaml
>> index e72cb5bacaf0..33a753c8877b 100644
>> --- a/Documentation/devicetree/bindings/mtd/st,stm32-fmc2-nand.yaml
>> +++ b/Documentation/devicetree/bindings/mtd/st,stm32-fmc2-nand.yaml
>> @@ -14,10 +14,12 @@ properties:
>>       enum:
>>         - st,stm32mp15-fmc2
>>         - st,stm32mp1-fmc2-nfc
>> +      - st,stm32mp25-fmc2
>> +      - st,stm32mp25-fmc2-nfc
>>   
>>     reg:
>>       minItems: 6
>> -    maxItems: 7
>> +    maxItems: 13
>>   
>>     interrupts:
>>       maxItems: 1
>> @@ -92,6 +94,60 @@ allOf:
>>               - description: Chip select 1 command
>>               - description: Chip select 1 address space
>>   
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: st,stm32mp25-fmc2
>> +    then:
>> +      properties:
>> +        reg:
>> +          items:
>> +            - description: Registers
>> +            - description: Chip select 0 data
>> +            - description: Chip select 0 command
>> +            - description: Chip select 0 address space
>> +            - description: Chip select 1 data
>> +            - description: Chip select 1 command
>> +            - description: Chip select 1 address space
>> +            - description: Chip select 2 data
>> +            - description: Chip select 2 command
>> +            - description: Chip select 2 address space
>> +            - description: Chip select 3 data
>> +            - description: Chip select 3 command
>> +            - description: Chip select 3 address space
>> +
>> +        clocks:
>> +          maxItems: 1
>> +
>> +        resets:
>> +          maxItems: 1
>> +
>> +      required:
>> +        - clocks
>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: st,stm32mp25-fmc2-nfc
>> +    then:
>> +      properties:
>> +        reg:
>> +          items:
>> +            - description: Chip select 0 data
>> +            - description: Chip select 0 command
>> +            - description: Chip select 0 address space
>> +            - description: Chip select 1 data
>> +            - description: Chip select 1 command
>> +            - description: Chip select 1 address space
>> +            - description: Chip select 2 data
>> +            - description: Chip select 2 command
>> +            - description: Chip select 2 address space
>> +            - description: Chip select 3 data
>> +            - description: Chip select 3 command
>> +            - description: Chip select 3 address space
>> +
>>   required:
>>     - compatible
>>     - reg
>> -- 
>> 2.25.1
>>

[PATCH 09/12] mtd: rawnand: stm32_fmc2: use dma_get_slave_caps to get DMA max burst

[Christophe Kerello]

use dma_get_slave_caps API to get the max burst size of a DMA channel.

For MP1 SOCs, MDMA is used and the max burst size is 128.
For MP25 SOC, DMA3 is used and the max burst size is 64.

Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
---
 drivers/mtd/nand/raw/stm32_fmc2_nand.c | 29 +++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/raw/stm32_fmc2_nand.c b/drivers/mtd/nand/raw/stm32_fmc2_nand.c
index 88811139aaf5..a7db7b675514 100644
--- a/drivers/mtd/nand/raw/stm32_fmc2_nand.c
+++ b/drivers/mtd/nand/raw/stm32_fmc2_nand.c
@@ -264,6 +264,8 @@ struct stm32_fmc2_nfc {
 	struct sg_table dma_ecc_sg;
 	u8 *ecc_buf;
 	int dma_ecc_len;
+	u32 tx_dma_max_burst;
+	u32 rx_dma_max_burst;
 
 	struct completion complete;
 	struct completion dma_data_complete;
@@ -347,20 +349,26 @@ static int stm32_fmc2_nfc_select_chip(struct nand_chip *chip, int chipnr)
 	stm32_fmc2_nfc_setup(chip);
 	stm32_fmc2_nfc_timings_init(chip);
 
-	if (nfc->dma_tx_ch && nfc->dma_rx_ch) {
+	if (nfc->dma_tx_ch) {
 		memset(&dma_cfg, 0, sizeof(dma_cfg));
-		dma_cfg.src_addr = nfc->data_phys_addr[nfc->cs_sel];
 		dma_cfg.dst_addr = nfc->data_phys_addr[nfc->cs_sel];
-		dma_cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
 		dma_cfg.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
-		dma_cfg.src_maxburst = 32;
-		dma_cfg.dst_maxburst = 32;
+		dma_cfg.dst_maxburst = nfc->tx_dma_max_burst /
+				       dma_cfg.dst_addr_width;
 
 		ret = dmaengine_slave_config(nfc->dma_tx_ch, &dma_cfg);
 		if (ret) {
 			dev_err(nfc->dev, "tx DMA engine slave config failed\n");
 			return ret;
 		}
+	}
+
+	if (nfc->dma_rx_ch) {
+		memset(&dma_cfg, 0, sizeof(dma_cfg));
+		dma_cfg.src_addr = nfc->data_phys_addr[nfc->cs_sel];
+		dma_cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+		dma_cfg.src_maxburst = nfc->rx_dma_max_burst /
+				       dma_cfg.src_addr_width;
 
 		ret = dmaengine_slave_config(nfc->dma_rx_ch, &dma_cfg);
 		if (ret) {
@@ -1545,6 +1553,7 @@ static int stm32_fmc2_nfc_setup_interface(struct nand_chip *chip, int chipnr,
 
 static int stm32_fmc2_nfc_dma_setup(struct stm32_fmc2_nfc *nfc)
 {
+	struct dma_slave_caps caps;
 	int ret = 0;
 
 	nfc->dma_tx_ch = dma_request_chan(nfc->dev, "tx");
@@ -1557,6 +1566,11 @@ static int stm32_fmc2_nfc_dma_setup(struct stm32_fmc2_nfc *nfc)
 		goto err_dma;
 	}
 
+	ret = dma_get_slave_caps(nfc->dma_tx_ch, &caps);
+	if (ret)
+		return ret;
+	nfc->tx_dma_max_burst = caps.max_burst;
+
 	nfc->dma_rx_ch = dma_request_chan(nfc->dev, "rx");
 	if (IS_ERR(nfc->dma_rx_ch)) {
 		ret = PTR_ERR(nfc->dma_rx_ch);
@@ -1567,6 +1581,11 @@ static int stm32_fmc2_nfc_dma_setup(struct stm32_fmc2_nfc *nfc)
 		goto err_dma;
 	}
 
+	ret = dma_get_slave_caps(nfc->dma_rx_ch, &caps);
+	if (ret)
+		return ret;
+	nfc->rx_dma_max_burst = caps.max_burst;
+
 	nfc->dma_ecc_ch = dma_request_chan(nfc->dev, "ecc");
 	if (IS_ERR(nfc->dma_ecc_ch)) {
 		ret = PTR_ERR(nfc->dma_ecc_ch);
-- 
2.25.1

[PATCH 10/12] mtd: rawnand: stm32_fmc2: add a platform data structure

[Christophe Kerello]

Before the introduction of MP25 SOC, let's use a platform data
structure for parameters that will differ (number of chip select).

The FMC2 NAND can support up to 4 chips select. On MP1 SOCs, only 2
chip select are available.

Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
---
 drivers/mtd/nand/raw/stm32_fmc2_nand.c | 32 +++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/nand/raw/stm32_fmc2_nand.c b/drivers/mtd/nand/raw/stm32_fmc2_nand.c
index a7db7b675514..c5bdb43f7221 100644
--- a/drivers/mtd/nand/raw/stm32_fmc2_nand.c
+++ b/drivers/mtd/nand/raw/stm32_fmc2_nand.c
@@ -16,6 +16,7 @@
 #include <linux/module.h>
 #include <linux/mtd/rawnand.h>
 #include <linux/of_address.h>
+#include <linux/of_device.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
@@ -37,7 +38,7 @@
 #define FMC2_MAX_SG			16
 
 /* Max chip enable */
-#define FMC2_MAX_CE			2
+#define FMC2_MAX_CE			4
 
 /* Max ECC buffer length */
 #define FMC2_MAX_ECC_BUF_LEN		(FMC2_BCHDSRS_LEN * FMC2_MAX_SG)
@@ -243,6 +244,10 @@ static inline struct stm32_fmc2_nand *to_fmc2_nand(struct nand_chip *chip)
 	return container_of(chip, struct stm32_fmc2_nand, chip);
 }
 
+struct stm32_fmc2_nfc_data {
+	int max_ncs;
+};
+
 struct stm32_fmc2_nfc {
 	struct nand_controller base;
 	struct stm32_fmc2_nand nand;
@@ -256,6 +261,7 @@ struct stm32_fmc2_nfc {
 	phys_addr_t data_phys_addr[FMC2_MAX_CE];
 	struct clk *clk;
 	u8 irq_state;
+	const struct stm32_fmc2_nfc_data *data;
 
 	struct dma_chan *dma_tx_ch;
 	struct dma_chan *dma_rx_ch;
@@ -1809,7 +1815,7 @@ static int stm32_fmc2_nfc_parse_child(struct stm32_fmc2_nfc *nfc,
 			return ret;
 		}
 
-		if (cs >= FMC2_MAX_CE) {
+		if (cs >= nfc->data->max_ncs) {
 			dev_err(nfc->dev, "invalid reg value: %d\n", cs);
 			return -EINVAL;
 		}
@@ -1915,6 +1921,10 @@ static int stm32_fmc2_nfc_probe(struct platform_device *pdev)
 	nand_controller_init(&nfc->base);
 	nfc->base.ops = &stm32_fmc2_nfc_controller_ops;
 
+	nfc->data = of_device_get_match_data(dev);
+	if (!nfc->data)
+		return -EINVAL;
+
 	ret = stm32_fmc2_nfc_set_cdev(nfc);
 	if (ret)
 		return ret;
@@ -1936,7 +1946,7 @@ static int stm32_fmc2_nfc_probe(struct platform_device *pdev)
 	if (nfc->dev == nfc->cdev)
 		start_region = 1;
 
-	for (chip_cs = 0, mem_region = start_region; chip_cs < FMC2_MAX_CE;
+	for (chip_cs = 0, mem_region = start_region; chip_cs < nfc->data->max_ncs;
 	     chip_cs++, mem_region += 3) {
 		if (!(nfc->cs_assigned & BIT(chip_cs)))
 			continue;
@@ -2092,7 +2102,7 @@ static int __maybe_unused stm32_fmc2_nfc_resume(struct device *dev)
 
 	stm32_fmc2_nfc_wp_disable(nand);
 
-	for (chip_cs = 0; chip_cs < FMC2_MAX_CE; chip_cs++) {
+	for (chip_cs = 0; chip_cs < nfc->data->max_ncs; chip_cs++) {
 		if (!(nfc->cs_assigned & BIT(chip_cs)))
 			continue;
 
@@ -2105,9 +2115,19 @@ static int __maybe_unused stm32_fmc2_nfc_resume(struct device *dev)
 static SIMPLE_DEV_PM_OPS(stm32_fmc2_nfc_pm_ops, stm32_fmc2_nfc_suspend,
 			 stm32_fmc2_nfc_resume);
 
+static const struct stm32_fmc2_nfc_data stm32_fmc2_nfc_mp1_data = {
+	.max_ncs = 2,
+};
+
 static const struct of_device_id stm32_fmc2_nfc_match[] = {
-	{.compatible = "st,stm32mp15-fmc2"},
-	{.compatible = "st,stm32mp1-fmc2-nfc"},
+	{
+		.compatible = "st,stm32mp15-fmc2",
+		.data = &stm32_fmc2_nfc_mp1_data,
+	},
+	{
+		.compatible = "st,stm32mp1-fmc2-nfc",
+		.data = &stm32_fmc2_nfc_mp1_data,
+	},
 	{}
 };
 MODULE_DEVICE_TABLE(of, stm32_fmc2_nfc_match);
-- 
2.25.1

[PATCH 11/12] mtd: rawnand: stm32_fmc2: add MP25 support

[Christophe Kerello]

Add MP25 SOC support (4 chip select are available).

Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
---
 drivers/mtd/nand/raw/stm32_fmc2_nand.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/raw/stm32_fmc2_nand.c b/drivers/mtd/nand/raw/stm32_fmc2_nand.c
index c5bdb43f7221..d71ec12cd5b1 100644
--- a/drivers/mtd/nand/raw/stm32_fmc2_nand.c
+++ b/drivers/mtd/nand/raw/stm32_fmc2_nand.c
@@ -1878,11 +1878,14 @@ static int stm32_fmc2_nfc_set_cdev(struct stm32_fmc2_nfc *nfc)
 	struct device *dev = nfc->dev;
 	bool ebi_found = false;
 
-	if (dev->parent && of_device_is_compatible(dev->parent->of_node,
-						   "st,stm32mp1-fmc2-ebi"))
+	if (dev->parent && (of_device_is_compatible(dev->parent->of_node,
+						    "st,stm32mp1-fmc2-ebi") ||
+			    of_device_is_compatible(dev->parent->of_node,
+						    "st,stm32mp25-fmc2-ebi")))
 		ebi_found = true;
 
-	if (of_device_is_compatible(dev->of_node, "st,stm32mp1-fmc2-nfc")) {
+	if (of_device_is_compatible(dev->of_node, "st,stm32mp1-fmc2-nfc") ||
+	    of_device_is_compatible(dev->of_node, "st,stm32mp25-fmc2-nfc")) {
 		if (ebi_found) {
 			nfc->cdev = dev->parent;
 
@@ -2119,6 +2122,10 @@ static const struct stm32_fmc2_nfc_data stm32_fmc2_nfc_mp1_data = {
 	.max_ncs = 2,
 };
 
+static const struct stm32_fmc2_nfc_data stm32_fmc2_nfc_mp25_data = {
+	.max_ncs = 4,
+};
+
 static const struct of_device_id stm32_fmc2_nfc_match[] = {
 	{
 		.compatible = "st,stm32mp15-fmc2",
@@ -2128,6 +2135,14 @@ static const struct of_device_id stm32_fmc2_nfc_match[] = {
 		.compatible = "st,stm32mp1-fmc2-nfc",
 		.data = &stm32_fmc2_nfc_mp1_data,
 	},
+	{
+		.compatible = "st,stm32mp25-fmc2",
+		.data = &stm32_fmc2_nfc_mp25_data,
+	},
+	{
+		.compatible = "st,stm32mp25-fmc2-nfc",
+		.data = &stm32_fmc2_nfc_mp25_data,
+	},
 	{}
 };
 MODULE_DEVICE_TABLE(of, stm32_fmc2_nfc_match);
-- 
2.25.1

[PATCH 12/12] mtd: rawnand: stm32_fmc2: update the driver to support revision 2

[Christophe Kerello]

Add the support of the revision 2 of FMC2 IP.
For the NAND controller, the bit used to enable the IP has moved.

Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
---
 drivers/mtd/nand/raw/stm32_fmc2_nand.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/raw/stm32_fmc2_nand.c b/drivers/mtd/nand/raw/stm32_fmc2_nand.c
index d71ec12cd5b1..877255b0d0fc 100644
--- a/drivers/mtd/nand/raw/stm32_fmc2_nand.c
+++ b/drivers/mtd/nand/raw/stm32_fmc2_nand.c
@@ -54,6 +54,7 @@
 
 /* FMC2 Controller Registers */
 #define FMC2_BCR1			0x0
+#define FMC2_CFGR			0x20
 #define FMC2_PCR			0x80
 #define FMC2_SR				0x84
 #define FMC2_PMEM			0x88
@@ -83,10 +84,14 @@
 #define FMC2_BCHDSR2			0x284
 #define FMC2_BCHDSR3			0x288
 #define FMC2_BCHDSR4			0x28c
+#define FMC2_VERR			0x3f4
 
 /* Register: FMC2_BCR1 */
 #define FMC2_BCR1_FMC2EN		BIT(31)
 
+/* Register: FMC2_CFGR */
+#define FMC2_CFGR_FMC2EN		BIT(31)
+
 /* Register: FMC2_PCR */
 #define FMC2_PCR_PWAITEN		BIT(1)
 #define FMC2_PCR_PBKEN			BIT(2)
@@ -208,6 +213,10 @@
 #define FMC2_BCHDSR4_EBP7		GENMASK(12, 0)
 #define FMC2_BCHDSR4_EBP8		GENMASK(28, 16)
 
+/* Register: FMC2_VERR */
+#define FMC2_VERR_MAJREV		GENMASK(7, 4)
+#define FMC2_VERR_MAJREV_2		2
+
 enum stm32_fmc2_ecc {
 	FMC2_ECC_HAM = 1,
 	FMC2_ECC_BCH4 = 4,
@@ -1397,9 +1406,20 @@ static void stm32_fmc2_nfc_init(struct stm32_fmc2_nfc *nfc)
 	pcr |= FIELD_PREP(FMC2_PCR_TAR, FMC2_PCR_TAR_DEFAULT);
 
 	/* Enable FMC2 controller */
-	if (nfc->dev == nfc->cdev)
-		regmap_update_bits(nfc->regmap, FMC2_BCR1,
-				   FMC2_BCR1_FMC2EN, FMC2_BCR1_FMC2EN);
+	if (nfc->dev == nfc->cdev) {
+		u32 verr;
+		u8 majrev;
+
+		regmap_read(nfc->regmap, FMC2_VERR, &verr);
+		majrev = FIELD_GET(FMC2_VERR_MAJREV, verr);
+
+		if (majrev < FMC2_VERR_MAJREV_2)
+			regmap_update_bits(nfc->regmap, FMC2_BCR1,
+					   FMC2_BCR1_FMC2EN, FMC2_BCR1_FMC2EN);
+		else
+			regmap_update_bits(nfc->regmap, FMC2_CFGR,
+					   FMC2_CFGR_FMC2EN, FMC2_CFGR_FMC2EN);
+	}
 
 	regmap_write(nfc->regmap, FMC2_PCR, pcr);
 	regmap_write(nfc->regmap, FMC2_PMEM, FMC2_PMEM_DEFAULT);
-- 
2.25.1

Re: [PATCH 00/12] Add MP25 FMC2 support

[Krzysztof Kozlowski]

On 12/02/2024 18:48, Christophe Kerello wrote:
> Add MP25 SOC support in stm32_fmc2 drivers:
>  - Update stm32-fmc2-ebi driver to support FMC2 revision 2 and MP25 SOC.
>  - Update stm32_fmc2_nand driver to support FMC2 revision 2 and MP25 SOC

Why do you combine memory controller driver and NAND in one patchset if
there is no dependency? On any further submissions, please split
independent works.

Best regards,
Krzysztof

Re: [PATCH 00/12] Add MP25 FMC2 support

[Christophe Kerello]

On 2/13/24 08:34, Krzysztof Kozlowski wrote:
> On 12/02/2024 18:48, Christophe Kerello wrote:
>> Add MP25 SOC support in stm32_fmc2 drivers:
>>   - Update stm32-fmc2-ebi driver to support FMC2 revision 2 and MP25 SOC.
>>   - Update stm32_fmc2_nand driver to support FMC2 revision 2 and MP25 SOC
> 
> Why do you combine memory controller driver and NAND in one patchset if
> there is no dependency? On any further submissions, please split
> independent works.

Hi Krzysztof,

NAND driver patch 11 refers to the compatible described for the memory
controller (so there is a dependency), but anyway, I am going to split
this patchet.

Regards,
Christophe Kerello.

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 00/12] Add MP25 FMC2 support

[Krzysztof Kozlowski]

On 13/02/2024 13:09, Christophe Kerello wrote:
> 
> 
> On 2/13/24 08:34, Krzysztof Kozlowski wrote:
>> On 12/02/2024 18:48, Christophe Kerello wrote:
>>> Add MP25 SOC support in stm32_fmc2 drivers:
>>>   - Update stm32-fmc2-ebi driver to support FMC2 revision 2 and MP25 SOC.
>>>   - Update stm32_fmc2_nand driver to support FMC2 revision 2 and MP25 SOC
>>
>> Why do you combine memory controller driver and NAND in one patchset if
>> there is no dependency? On any further submissions, please split
>> independent works.
> 
> Hi Krzysztof,
> 
> NAND driver patch 11 refers to the compatible described for the memory

Eh, it shouldn't really. This does not scale - you will keep growing
that 'if' clause? And other drivers should not include other device
compatibles.

But anyway that's not a real subsystem dependency. Just mention in patch
changelog (so ---) that compatible is documented somewhere at URL xyz.

Best regards,
Krzysztof

Re: [PATCH 00/12] Add MP25 FMC2 support

[Christophe Kerello]

On 2/12/24 18:48, Christophe Kerello wrote:
> Add MP25 SOC support in stm32_fmc2 drivers:
>   - Update stm32-fmc2-ebi driver to support FMC2 revision 2 and MP25 SOC.
>   - Update stm32_fmc2_nand driver to support FMC2 revision 2 and MP25 SOC.
> 

Hi Miquel,

Don't waste time reviewing this first patchset because I rewrote the 
NAND part.
Patch V2 will be sent next week.

Regards,
Christophe Kerello.


> Christophe Kerello (11):
>    dt-bindings: memory-controller: st,stm32: add MP25 support
>    memory: stm32-fmc2-ebi: add a platform data structure
>    memory: stm32-fmc2-ebi: add MP25 support
>    memory: stm32-fmc2-ebi: update the driver to support revision 2
>    memory: stm32-fmc2-ebi: add RIF support
>    memory: stm32-fmc2-ebi: add runtime PM support
>    dt-bindings: mtd: st,stm32: add MP25 support
>    mtd: rawnand: stm32_fmc2: use dma_get_slave_caps to get DMA max burst
>    mtd: rawnand: stm32_fmc2: add a platform data structure
>    mtd: rawnand: stm32_fmc2: add MP25 support
>    mtd: rawnand: stm32_fmc2: update the driver to support revision 2
> 
> Patrick Delaunay (1):
>    dt-bindings: memory-controller: st,stm32: add 'power-domains' property
> 
>   .../memory-controllers/st,stm32-fmc2-ebi.yaml |   7 +-
>   .../bindings/mtd/st,stm32-fmc2-nand.yaml      |  58 ++-
>   drivers/memory/stm32-fmc2-ebi.c               | 445 ++++++++++++++++--
>   drivers/mtd/nand/raw/stm32_fmc2_nand.c        | 108 ++++-
>   4 files changed, 547 insertions(+), 71 deletions(-)
>