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 <m...@kernel.org>
> > Sent: Saturday, January 16, 2021 8:28 AM
> > To: Nava kishore Manne <na...@xilinx.com>
> > Cc: m...@kernel.org; t...@redhat.com; robh...@kernel.org; Michal Simek
> > <mich...@xilinx.com>; linux-f...@vger.kernel.org;
> > devicet...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; linux-
> > ker...@vger.kernel.org; git <g...@xilinx.com>; chinnikishore...@gmail.com
> > 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.ma...@xilinx.com>
> > > ---
> > > 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
