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

[Xu Yilun <yilun.xu@linux.intel.com>]

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
>