[PATCH 1/2] dt-bindings: fpga: zynq: Document ICAP on boot

[PATCH 1/2] dt-bindings: fpga: zynq: Document ICAP on boot

[Sam Winchenbach]

From: Sam Winchenbach <swinchenbach@arka.org>

Documents the ability to enable the ICAP interface on boot.

Signed-off-by: Sam Winchenbach <swinchenbach@arka.org>
---
 .../devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.yaml     | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.yaml b/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.yaml
index 04dcadc2c20e9..bb2781ae126ca 100644
--- a/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.yaml
+++ b/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.yaml
@@ -31,6 +31,13 @@ properties:
     description:
       Phandle to syscon block which provide access to SLCR registers
 
+  enable-icap-on-load:
+    type: boolean
+    description: If present, the ICAP controller will be enabled when
+      the driver probes. This is useful if the fabric is loaded
+      during the boot process and contains a core, such as the SEM,
+      that requires access to ICAP interface to operate properly.
+
 required:
   - compatible
   - reg
-- 
2.49.0

[PATCH 2/2] fpga: zynq-fpga: Allow ICAP enable on probe

[Sam Winchenbach]

From: Sam Winchenbach <swinchenbach@arka.org>

Adds an option to enable the ICAP interface when driver is probed. This
is useful when the fabric is loaded as part of the first or second stage
of the boot process and contains an IP core that requires access to the
ICAP interface, such as Soft Error Mitigation (SEM) core.

Signed-off-by: Sam Winchenbach <swinchenbach@arka.org>
---
 drivers/fpga/zynq-fpga.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
index f7e08f7ea9ef3..4a53a429d659f 100644
--- a/drivers/fpga/zynq-fpga.c
+++ b/drivers/fpga/zynq-fpga.c
@@ -20,6 +20,7 @@
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/pm.h>
+#include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/string.h>
 #include <linux/scatterlist.h>
@@ -482,8 +483,7 @@ static int zynq_fpga_ops_write(struct fpga_manager *mgr, struct sg_table *sgt)
 	return err;
 }
 
-static int zynq_fpga_ops_write_complete(struct fpga_manager *mgr,
-					struct fpga_image_info *info)
+static int zynq_fpga_enable_icap(struct fpga_manager *mgr)
 {
 	struct zynq_fpga_priv *priv = mgr->priv;
 	int err;
@@ -504,6 +504,18 @@ static int zynq_fpga_ops_write_complete(struct fpga_manager *mgr,
 
 	clk_disable(priv->clk);
 
+	return err;
+}
+
+static int zynq_fpga_ops_write_complete(struct fpga_manager *mgr,
+					struct fpga_image_info *info)
+{
+	struct zynq_fpga_priv *priv = mgr->priv;
+	int err;
+	u32 intr_status;
+
+	err = zynq_fpga_enable_icap(mgr);
+
 	if (err)
 		return err;
 
@@ -615,6 +627,16 @@ static int zynq_fpga_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, mgr);
 
+	if (device_property_read_bool(dev, "enable-icap-on-load")) {
+		err = zynq_fpga_enable_icap(mgr);
+		if (err) {
+			dev_err(dev, "unable to enable ICAP interface\n");
+			fpga_mgr_unregister(mgr);
+			clk_unprepare(priv->clk);
+			return err;
+		}
+	}
+
 	return 0;
 }
 
-- 
2.49.0

Re: [PATCH 1/2] dt-bindings: fpga: zynq: Document ICAP on boot

[Krzysztof Kozlowski]

On 28/03/2025 15:19, Sam Winchenbach wrote:
> From: Sam Winchenbach <swinchenbach@arka.org>
> 
> Documents the ability to enable the ICAP interface on boot.
> 
> Signed-off-by: Sam Winchenbach <swinchenbach@arka.org>
> ---
>  .../devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.yaml     | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.yaml b/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.yaml
> index 04dcadc2c20e9..bb2781ae126ca 100644
> --- a/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.yaml
> +++ b/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.yaml
> @@ -31,6 +31,13 @@ properties:
>      description:
>        Phandle to syscon block which provide access to SLCR registers
>  
> +  enable-icap-on-load:

Missing vendor prefix.

> +    type: boolean
> +    description: If present, the ICAP controller will be enabled when
> +      the driver probes. This is useful if the fabric is loaded
> +      during the boot process and contains a core, such as the SEM,

I don't get how this is suitable for DT. If you decide to load the
fabric from driver, that's driver decision so not DT.

> +      that requires access to ICAP interface to operate properly.
> +
>  required:
>    - compatible
>    - reg


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: fpga: zynq: Document ICAP on boot

[Sam Winchenbach]

On Sat, Mar 29, 2025 at 05:59:07AM +0100, Krzysztof Kozlowski wrote:
> On 28/03/2025 15:19, Sam Winchenbach wrote:
> > From: Sam Winchenbach <swinchenbach@arka.org>
> > 
> > Documents the ability to enable the ICAP interface on boot.
> > 
> > Signed-off-by: Sam Winchenbach <swinchenbach@arka.org>
> > ---
> >  .../devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.yaml     | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.yaml b/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.yaml
> > index 04dcadc2c20e9..bb2781ae126ca 100644
> > --- a/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.yaml
> > +++ b/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.yaml
> > @@ -31,6 +31,13 @@ properties:
> >      description:
> >        Phandle to syscon block which provide access to SLCR registers
> >  
> > +  enable-icap-on-load:
> 
> Missing vendor prefix.

I will add this to a v2 patch, assuming we come to an agreement on the
suitability of this approach.

> 
> > +    type: boolean
> > +    description: If present, the ICAP controller will be enabled when
> > +      the driver probes. This is useful if the fabric is loaded
> > +      during the boot process and contains a core, such as the SEM,
> 
> I don't get how this is suitable for DT. If you decide to load the
> fabric from driver, that's driver decision so not DT.

Before writing the fabric to the FPGA the driver disables the ICAP, enabling
the PCAP. Once writing is complete it unconditionally disables the PCAP,
enabling the ICAP. This patch just makes it so, depending on the use case,
the ICAP can be enabled at boot. This will not prevent the system from being
able to load a fabric through the driver. I added in this boolean so existing
behavior would be maintained.

Do you recommend another approach such as writing to a sysfs attribute to
switch from PCAP to ICAP?

> 
> > +      that requires access to ICAP interface to operate properly.
> > +
> >  required:
> >    - compatible
> >    - reg
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 1/2] dt-bindings: fpga: zynq: Document ICAP on boot

[Krzysztof Kozlowski]

On 31/03/2025 14:30, Sam Winchenbach wrote:
>>
>>> +    type: boolean
>>> +    description: If present, the ICAP controller will be enabled when
>>> +      the driver probes. This is useful if the fabric is loaded
>>> +      during the boot process and contains a core, such as the SEM,
>>
>> I don't get how this is suitable for DT. If you decide to load the
>> fabric from driver, that's driver decision so not DT.
> 
> Before writing the fabric to the FPGA the driver disables the ICAP, enabling
> the PCAP. Once writing is complete it unconditionally disables the PCAP,
> enabling the ICAP. This patch just makes it so, depending on the use case,
> the ICAP can be enabled at boot. This will not prevent the system from being
> able to load a fabric through the driver. I added in this boolean so existing
> behavior would be maintained.
> 
> Do you recommend another approach such as writing to a sysfs attribute to
> switch from PCAP to ICAP?
Not sure yet. Can't you check the status of ICAP before programming and
then enable it only if was enabled before?

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: fpga: zynq: Document ICAP on boot

[Sam Winchenbach]

On Mon, Mar 31, 2025 at 02:43:59PM +0200, Krzysztof Kozlowski wrote:
> On 31/03/2025 14:30, Sam Winchenbach wrote:
> >>
> >>> +    type: boolean
> >>> +    description: If present, the ICAP controller will be enabled when
> >>> +      the driver probes. This is useful if the fabric is loaded
> >>> +      during the boot process and contains a core, such as the SEM,
> >>
> >> I don't get how this is suitable for DT. If you decide to load the
> >> fabric from driver, that's driver decision so not DT.
> > 
> > Before writing the fabric to the FPGA the driver disables the ICAP, enabling
> > the PCAP. Once writing is complete it unconditionally disables the PCAP,
> > enabling the ICAP. This patch just makes it so, depending on the use case,
> > the ICAP can be enabled at boot. This will not prevent the system from being
> > able to load a fabric through the driver. I added in this boolean so existing
> > behavior would be maintained.
> > 
> > Do you recommend another approach such as writing to a sysfs attribute to
> > switch from PCAP to ICAP?
> Not sure yet. Can't you check the status of ICAP before programming and
> then enable it only if was enabled before?

I am having a bit of difficulty understanding this so let's talk about cases
where the ICAP is enabled/disabled -

1. When writing the fabric from the driver
   In this situation it might make sense to read the state of the ICAP
   interface when preparing the fabric, before enabling PCAP. When the write
   completes you could re-enable the ICAP if it was previously enabled.

   This might be outside the scope of this change - and I am not comfortable
   enough with this use-case to understand potential side effects from doing
   this. Logically it makes sense, but there may be a very specific reason that
   the ICAP must be enabled after doing a fabric load or partial
   reconfiguration.

2. When the FPGA driver loads and is probed by the DTS
   In this situation, which is covered by this patch, the FPGA is loaded by
   BootROM/FSBL but contains functionality that requires the ICAP. Unless the
   user has made modifications to the FSBL or 3rd stage bootloader there is no
   clear way to enable the ICAP interface. Checking to see if it had been
   enabled prior to loading this driver does not (in my opinion) make a lot of
   sense here.

   Perhaps the name of the DTS is confusing? The suffix '-on-load' was meant to
   indicate when the driver was loaded, not the fabric. Would the suffix
   '-on-probe' be more clear?

Let me know your thoughts,
-Sam

>
> Best regards,
> Krzysztof

Re: [PATCH 1/2] dt-bindings: fpga: zynq: Document ICAP on boot

[Krzysztof Kozlowski]

On Mon, Mar 31, 2025 at 09:07:03AM -0400, Sam Winchenbach wrote:
> On Mon, Mar 31, 2025 at 02:43:59PM +0200, Krzysztof Kozlowski wrote:
> > Not sure yet. Can't you check the status of ICAP before programming and
> > then enable it only if was enabled before?
> 
> I am having a bit of difficulty understanding this so let's talk about cases
> where the ICAP is enabled/disabled -
> 
> 1. When writing the fabric from the driver
>    In this situation it might make sense to read the state of the ICAP
>    interface when preparing the fabric, before enabling PCAP. When the write
>    completes you could re-enable the ICAP if it was previously enabled.
> 
>    This might be outside the scope of this change - and I am not comfortable
>    enough with this use-case to understand potential side effects from doing
>    this. Logically it makes sense, but there may be a very specific reason that
>    the ICAP must be enabled after doing a fabric load or partial
>    reconfiguration.
> 
> 2. When the FPGA driver loads and is probed by the DTS
>    In this situation, which is covered by this patch, the FPGA is loaded by
>    BootROM/FSBL but contains functionality that requires the ICAP. Unless the
>    user has made modifications to the FSBL or 3rd stage bootloader there is no
>    clear way to enable the ICAP interface. Checking to see if it had been
>    enabled prior to loading this driver does not (in my opinion) make a lot of
>    sense here.
> 
>    Perhaps the name of the DTS is confusing? The suffix '-on-load' was meant to
>    indicate when the driver was loaded, not the fabric. Would the suffix
>    '-on-probe' be more clear?

Neither on-load nor on-probe, because again you instruct the OS what it
should do. You should instead describe the hardware (or other parts of
software stack). Describe the condition, the hardware feature, the
characteristic observed. With proper phrasing the property should be
fine, but I still do not see that its name and description match actual
hardware.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: fpga: zynq: Document ICAP on boot

[Krzysztof Kozlowski]

On 31/03/2025 15:07, Sam Winchenbach wrote:
>>> Before writing the fabric to the FPGA the driver disables the ICAP, enabling
>>> the PCAP. Once writing is complete it unconditionally disables the PCAP,
>>> enabling the ICAP. This patch just makes it so, depending on the use case,
>>> the ICAP can be enabled at boot. This will not prevent the system from being
>>> able to load a fabric through the driver. I added in this boolean so existing
>>> behavior would be maintained.
>>>
>>> Do you recommend another approach such as writing to a sysfs attribute to
>>> switch from PCAP to ICAP?
>> Not sure yet. Can't you check the status of ICAP before programming and
>> then enable it only if was enabled before?
> 
> I am having a bit of difficulty understanding this so let's talk about cases
> where the ICAP is enabled/disabled -
> 
> 1. When writing the fabric from the driver
>    In this situation it might make sense to read the state of the ICAP
>    interface when preparing the fabric, before enabling PCAP. When the write
>    completes you could re-enable the ICAP if it was previously enabled.
> 
>    This might be outside the scope of this change - and I am not comfortable
>    enough with this use-case to understand potential side effects from doing
>    this. Logically it makes sense, but there may be a very specific reason that
>    the ICAP must be enabled after doing a fabric load or partial
>    reconfiguration.
> 
> 2. When the FPGA driver loads and is probed by the DTS
>    In this situation, which is covered by this patch, the FPGA is loaded by
>    BootROM/FSBL but contains functionality that requires the ICAP. Unless the
>    user has made modifications to the FSBL or 3rd stage bootloader there is no
>    clear way to enable the ICAP interface. Checking to see if it had been
>    enabled prior to loading this driver does not (in my opinion) make a lot of
>    sense here.
> 
>    Perhaps the name of the DTS is confusing? The suffix '-on-load' was meant to
>    indicate when the driver was loaded, not the fabric. Would the suffix
>    '-on-probe' be more clear?
None of these two, because you refer to software. Property is fine but
you need to describe the actual state of hardware or system or entire
stack, e.g. "fpga-with-sem".
Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: fpga: zynq: Document ICAP on boot

[Xu Yilun]

On Mon, Mar 31, 2025 at 09:07:03AM -0400, Sam Winchenbach wrote:
> On Mon, Mar 31, 2025 at 02:43:59PM +0200, Krzysztof Kozlowski wrote:
> > On 31/03/2025 14:30, Sam Winchenbach wrote:
> > >>
> > >>> +    type: boolean
> > >>> +    description: If present, the ICAP controller will be enabled when
> > >>> +      the driver probes. This is useful if the fabric is loaded
> > >>> +      during the boot process and contains a core, such as the SEM,
> > >>
> > >> I don't get how this is suitable for DT. If you decide to load the
> > >> fabric from driver, that's driver decision so not DT.
> > > 
> > > Before writing the fabric to the FPGA the driver disables the ICAP, enabling
> > > the PCAP. Once writing is complete it unconditionally disables the PCAP,
> > > enabling the ICAP. This patch just makes it so, depending on the use case,
> > > the ICAP can be enabled at boot. This will not prevent the system from being
> > > able to load a fabric through the driver. I added in this boolean so existing
> > > behavior would be maintained.
> > > 
> > > Do you recommend another approach such as writing to a sysfs attribute to
> > > switch from PCAP to ICAP?
> > Not sure yet. Can't you check the status of ICAP before programming and
> > then enable it only if was enabled before?
> 
> I am having a bit of difficulty understanding this so let's talk about cases
> where the ICAP is enabled/disabled -
> 
> 1. When writing the fabric from the driver
>    In this situation it might make sense to read the state of the ICAP
>    interface when preparing the fabric, before enabling PCAP. When the write
>    completes you could re-enable the ICAP if it was previously enabled.
> 
>    This might be outside the scope of this change - and I am not comfortable
>    enough with this use-case to understand potential side effects from doing
>    this. Logically it makes sense, but there may be a very specific reason that
>    the ICAP must be enabled after doing a fabric load or partial
>    reconfiguration.
> 
> 2. When the FPGA driver loads and is probed by the DTS
>    In this situation, which is covered by this patch, the FPGA is loaded by
>    BootROM/FSBL but contains functionality that requires the ICAP. Unless the
>    user has made modifications to the FSBL or 3rd stage bootloader there is no
>    clear way to enable the ICAP interface. Checking to see if it had been

I don't think this should be a property for fpga_mgr device. It is for
FPGA reprogramming. You insmod the reprograming driver not for
reprogramming, just to enable the already programmed functionality.

My idea is, to load the fpga_region with an image tagged "external-fpga-config".

Thanks,
Yilun

>    enabled prior to loading this driver does not (in my opinion) make a lot of
>    sense here.
> 
>    Perhaps the name of the DTS is confusing? The suffix '-on-load' was meant to
>    indicate when the driver was loaded, not the fabric. Would the suffix
>    '-on-probe' be more clear?
> 
> Let me know your thoughts,
> -Sam
> 
> >
> > Best regards,
> > Krzysztof
>