| Message ID | 20250328141944.119504-1-sam.winchenbach@framepointer.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] dt-bindings: fpga: zynq: Document ICAP on boot | expand |
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
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
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
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
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