Re: [PATCH 2/2] fpga: Add support for Xilinx DFX AXI Shutdown manager

[Moritz Fischer]

On Tue, Jan 19, 2021 at 06:34:54AM +0000, Nava kishore Manne wrote:
> Hi Moritz,
> 
> 	Thanks for the review.
> Please find my response inline.
> 
> > -----Original Message-----
> > From: Moritz Fischer <mdf@xxxxxxxxxx>
> > Sent: Saturday, January 16, 2021 8:28 AM
> > To: Nava kishore Manne <navam@xxxxxxxxxx>
> > Cc: mdf@xxxxxxxxxx; trix@xxxxxxxxxx; robh+dt@xxxxxxxxxx; Michal Simek
> > <michals@xxxxxxxxxx>; linux-fpga@xxxxxxxxxxxxxxx;
> > devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> > kernel@xxxxxxxxxxxxxxx; git <git@xxxxxxxxxx>; chinnikishore369@xxxxxxxxx
> > Subject: Re: [PATCH 2/2] fpga: Add support for Xilinx DFX AXI Shutdown
> > manager
> > 
> > Hi,
> > 
> > On Fri, Jan 15, 2021 at 07:04:31AM +0530, Nava kishore Manne wrote:
> > > This patch adds support for Xilinx Dynamic Function eXchange(DFX) AXI
> > > shutdown manager IP. It can be used to safely handling the AXI traffic
> > > on a Reconfigurable Partition when it is undergoing dynamic
> > > reconfiguration and there by preventing system deadlock that may occur
> > > if AXI transactions are interrupted during reconfiguration.
> > >
> > > PR-Decoupler and AXI shutdown manager are completely different IPs.
> > > But both the IP registers are compatible and also both belong to the
> > > same sub-system (fpga-bridge).So using same driver for both IP's.
> > 
> > I'm a bit confused, the whole goal here is to give the thing a different name?
> 
> 
> Both the PR Decoupler and AXI Shutdown IP manager IP's are follows same register spec.
> Most of the code is common so we thought of reusing  same driver for AXI shutdown manager as well.

What are the differences, though other than it shows a different name?
> 
> > >
> > > Signed-off-by: Nava kishore Manne <nava.manne@xxxxxxxxxx>
> > > ---
> > >  drivers/fpga/xilinx-pr-decoupler.c | 35
> > > ++++++++++++++++++++++++++----
> > >  1 file changed, 31 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/fpga/xilinx-pr-decoupler.c
> > > b/drivers/fpga/xilinx-pr-decoupler.c
> > > index 7d69af230567..c95f3d065ccb 100644
> > > --- a/drivers/fpga/xilinx-pr-decoupler.c
> > > +++ b/drivers/fpga/xilinx-pr-decoupler.c
> > > @@ -19,10 +19,15 @@
> > >  #define CTRL_OFFSET		0
> > >
> > >  struct xlnx_pr_decoupler_data {
> > > +	const struct xlnx_config_data *ipconfig;
> > >  	void __iomem *io_base;
> > >  	struct clk *clk;
> > >  };
> > >
> > > +struct xlnx_config_data {
> > > +	char *name;
> > > +};
> > > +
> > >  static inline void xlnx_pr_decoupler_write(struct xlnx_pr_decoupler_data
> > *d,
> > >  					   u32 offset, u32 val)
> > >  {
> > > @@ -76,15 +81,28 @@ static const struct fpga_bridge_ops
> > xlnx_pr_decoupler_br_ops = {
> > >  	.enable_show = xlnx_pr_decoupler_enable_show,  };
> > >
> > > +static const struct xlnx_config_data decoupler_config = {
> > > +	.name = "Xilinx PR Decoupler",
> > > +};
> > > +
> > > +static const struct xlnx_config_data shutdown_config = {
> > > +	.name = "Xilinx DFX AXI shutdown mgr", };
> > 
> > If it's just the strings, why not store them as is?
> 
> In order to differentiate the IP's at probe time we are using this name filed.
> 
> > > +
> > >  static const struct of_device_id xlnx_pr_decoupler_of_match[] = {
> > > -	{ .compatible = "xlnx,pr-decoupler-1.00", },
> > > -	{ .compatible = "xlnx,pr-decoupler", },
> > > +	{ .compatible = "xlnx,pr-decoupler-1.00", .data = &decoupler_config
> > },
> > > +	{ .compatible = "xlnx,pr-decoupler", .data = &decoupler_config },
> > > +	{ .compatible = "xlnx,dfx-axi-shutdown-manager-1.00",
> > > +					.data = &shutdown_config },
> > > +	{ .compatible = "xlnx,dfx-axi-shutdown-manager",
> > > +					.data = &shutdown_config },
> > >  	{},
> > >  };
> > >  MODULE_DEVICE_TABLE(of, xlnx_pr_decoupler_of_match);
> > >
> > >  static int xlnx_pr_decoupler_probe(struct platform_device *pdev)  {
> > > +	struct device_node *np = pdev->dev.of_node;
> > >  	struct xlnx_pr_decoupler_data *priv;
> > >  	struct fpga_bridge *br;
> > >  	int err;
> > > @@ -94,6 +112,14 @@ static int xlnx_pr_decoupler_probe(struct
> > platform_device *pdev)
> > >  	if (!priv)
> > >  		return -ENOMEM;
> > >
> > > +	if (np) {
> > > +		const struct of_device_id *match;
> > > +
> > > +		match = of_match_node(xlnx_pr_decoupler_of_match, np);
> > > +		if (match && match->data)
> > > +			priv->ipconfig = match->data;
> > > +	}
> > > +
> > >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > >  	priv->io_base = devm_ioremap_resource(&pdev->dev, res);
> > >  	if (IS_ERR(priv->io_base))
> > > @@ -114,7 +140,7 @@ static int xlnx_pr_decoupler_probe(struct
> > > platform_device *pdev)
> > >
> > >  	clk_disable(priv->clk);
> > >
> > > -	br = devm_fpga_bridge_create(&pdev->dev, "Xilinx PR Decoupler",
> > > +	br = devm_fpga_bridge_create(&pdev->dev, priv->ipconfig->name,
> > >  				     &xlnx_pr_decoupler_br_ops, priv);
> > >  	if (!br) {
> > >  		err = -ENOMEM;
> > > @@ -125,7 +151,8 @@ static int xlnx_pr_decoupler_probe(struct
> > > platform_device *pdev)
> > >
> > >  	err = fpga_bridge_register(br);
> > >  	if (err) {
> > > -		dev_err(&pdev->dev, "unable to register Xilinx PR
> > Decoupler");
> > > +		dev_err(&pdev->dev, "unable to register %s",
> > > +			priv->ipconfig->name);
> > >  		goto err_clk;
> > >  	}
> > >
> > > --
> > > 2.18.0
> > >
> > 
> 
> Regards,
> Navakishore.
- Moritz