[PATCH v2 2/3] fpga: region: Add fpga-region 'power-domains' property

[Xu Yilun]

On 2022-08-16 at 10:22:28 +0000, Manne, Nava kishore wrote:
> Hi Yilun,
> 
> 	Please find my response inline.
> 
> > -----Original Message-----
> > From: Xu Yilun <yilun.xu at intel.com>
> > Sent: Thursday, July 21, 2022 12:25 PM
> > To: Manne, Nava kishore <nava.kishore.manne at amd.com>
> > Cc: Nava kishore Manne <nava.manne at xilinx.com>; mdf at kernel.org;
> > hao.wu at intel.com; trix at redhat.com; robh+dt at kernel.org;
> > krzysztof.kozlowski+dt at linaro.org; michal.simek at xilinx.com; linux-
> > fpga at vger.kernel.org; devicetree at vger.kernel.org; linux-
> > kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> > git at xilinx.com; Rob Herring <robh at kernel.org>
> > Subject: Re: [PATCH v2 2/3] fpga: region: Add fpga-region 'power-domains'
> > property
> > 
> > On Mon, Jul 18, 2022 at 06:46:32AM +0000, Manne, Nava kishore wrote:
> > > Hi Yilun,
> > >
> > > 	Please find my response inline.
> > >
> > > > -----Original Message-----
> > > > From: Xu Yilun <yilun.xu at intel.com>
> > > > Sent: Friday, June 24, 2022 9:58 PM
> > > > To: Nava kishore Manne <nava.manne at xilinx.com>
> > > > Cc: mdf at kernel.org; hao.wu at intel.com; trix at redhat.com;
> > > > robh+dt at kernel.org; krzysztof.kozlowski+dt at linaro.org;
> > > > michal.simek at xilinx.com; linux-fpga at vger.kernel.org;
> > > > devicetree at vger.kernel.org; linux-kernel at vger.kernel.org; linux-arm-
> > > > kernel at lists.infradead.org; git at xilinx.com; Rob Herring
> > > > <robh at kernel.org>
> > > > Subject: Re: [PATCH v2 2/3] fpga: region: Add fpga-region 'power-
> > domains'
> > > > property
> > > >
> > > > CAUTION: This message has originated from an External Source. Please
> > > > use proper judgment and caution when opening attachments, clicking
> > > > links, or responding to this email.
> > > >
> > > >
> > > > On Mon, May 23, 2022 at 07:15:16PM +0530, Nava kishore Manne wrote:
> > > > > Add fpga-region 'power-domains' property to allow to handle the
> > > > > FPGA/PL power domains.
> > > > >
> > > > > Signed-off-by: Nava kishore Manne <nava.manne at xilinx.com>
> > > > > Acked-by: Rob Herring <robh at kernel.org>
> > > > > ---
> > > > > Changes for v2:
> > > > >               - Updated power-domains description.
> > > > >
> > > > >  .../devicetree/bindings/fpga/fpga-region.txt       | 14 ++++++++++++++
> > > > >  1 file changed, 14 insertions(+)
> > > > >
> > > > > diff --git
> > > > > a/Documentation/devicetree/bindings/fpga/fpga-region.txt
> > > > > b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> > > > > index 7d3515264838..f299c3749505 100644
> > > > > --- a/Documentation/devicetree/bindings/fpga/fpga-region.txt
> > > > > +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> > > > > @@ -196,6 +196,20 @@ Optional properties:
> > > > >  - config-complete-timeout-us : The maximum time in microseconds
> > > > > time
> > > > for the
> > > > >       FPGA to go to operating mode after the region has been
> > programmed.
> > > > >  - child nodes : devices in the FPGA after programming.
> > > > > +- power-domains : A phandle and power domain specifier pair to
> > > > > +the
> > > > power domain
> > > > > +     which is responsible for turning on/off the power to the
> > > > > + FPGA/PL
> > > > region.
> > > >
> > > > Could you help explain what is PL?
> > > >
> > > > > +Example:
> > > > > +     fpga_full: fpga-full {
> > > > > +                compatible = "fpga-region";
> > > > > +                fpga-mgr = <&zynqmp_pcap>;
> > > > > +                #address-cells = <2>;
> > > > > +                #size-cells = <2>;
> > > > > +                ranges;
> > > > > +                power-domains = <&zynqmp_firmware PL_PD>;
> > > > > +        };
> > > > > +
> > > > > +     The PL_PD power domain will be turned on before loading the
> > > > > +bitstream and turned off while removing/unloading the bitstream
> > > > > +using
> > > > overlays.
> > > >
> > > > I think the single power-domain may not cover some use cases that
> > > > of-fpga- region driver supports.
> > >
> > > I am not sure which use case you are talking about. Can you please point
> > me the exact use case here?
> > >
> > > > It is possible there are already devices in fpga-region for static
> > > > OF tree, or an overlay with no 'firmware-name' but 'external-fpga-
> > > > config'. In these cases power domains may still be needed, is it?
> > > >
> > >
> > > It's an optional property user can decide whether he needs this
> > > support or not for 'external-fpga-config Use case.
> > 
> > If an external-fpga-config FPGA region needs to enable a power domain
> > before sub devices population, how could it config the DT? I assume in this
> > patch "power-domains" property is only used before & after reconfiguration
> > but external-fpga-config FPGA region may need no reconfiguration.
> > 
> I agree, For 'external-fpga-config' use case this patch is not capable of supporting the dynamic PM.
> I Will explore and get back to you to handle this use case.
> 
> > >
> > > > Another case is the fpga-region may need multiple power domains?
> > > >
> > >
> > > In our use case full region and relevant partial regions have
> > > different power domains and this patch is capable of handle different
> > > power domain regions (full and partial regions)
> > 
> > If a FPGA region needs 2 or more power domains for partial reconfiguration,
> > how could we find out and enable them all?
> > 
> > From the 2 cases, I see as a generic driver, there may be need to enable
> > different power domains at different moments. And I'm afraid a simple
> > implementation of pm_runtime_get before reconfiguration may limit the
> > usage of "power-domains" property for of-fpga-region.
> > 
> 
> Can you please point me any references to handle the multiple power domains?

Some devices may need multiple domains to work properly, for example:

	power-domains = <&pd_disp>, <&pd_pci>;
	power-domain-names = "pcie", "pcie_phy";

I'm not sure if there is need for some fpga region, but think it could be
a valid usecase.

Thanks,
Yilun

> 
> This implementation is region specific which means it can handle both full and partial power domains
> Independently based on the power domains info exists in the DT.
> For example:
> 
> Base DT: For full
> fpga_full: fpga-full {
>                 compatible = "fpga-region";
>                 fpga-mgr = <&zynqmp_pcap>;
>                 #address-cells = <2>;
>                 #size-cells = <2>;
>                 ranges;
>                 power-domains = <&zynqmp_firmware PL_PD>;
> };
> 
> Overlay DT: For Partial
> fragment at 0 {
> 	target = <&fpga_full>;
> 	#address-cells = <2>;
> 	#size-cells = <2>;
> 	 __overlay__ {
> 		#address-cells = <2>;
> 		#size-cells = <2>;
> 		firmware-name = "full.bin";
> 		fpga_PR0: fpga-PR0 {
> 			compatible = "fpga-region";
> 			#address-cells = <2>;
> 			#size-cells = <2>;
> 			ranges;
> 			power-domains = <&zynqmp_firmware PL_PR_PD>;
> 		};
> 	 };
> };
> 
> Regards,
> Navakishore.