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

[Nava kishore Manne]

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.

> >
> > 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.