| Message ID | 1519188826-2047-1-git-send-email-shubhrajyoti.datta@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [1/2] doc: fpga: Add reset bridge support | expand |
On Wed, Feb 21, 2018 at 10:23:45AM +0530, shubhrajyoti.datta@gmail.com wrote: > From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > > Add reset bridge support. Once this bridge is enabled. > The reset line(s) will be toggled. Generally it will be > called after the bitstream load to reset the PL. > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > --- > .../devicetree/bindings/fpga/xlnx,rst-bridge.txt | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > create mode 100644 Documentation/devicetree/bindings/fpga/xlnx,rst-bridge.txt > > diff --git a/Documentation/devicetree/bindings/fpga/xlnx,rst-bridge.txt b/Documentation/devicetree/bindings/fpga/xlnx,rst-bridge.txt > new file mode 100644 > index 0000000..6f1bfc2 > --- /dev/null > +++ b/Documentation/devicetree/bindings/fpga/xlnx,rst-bridge.txt > @@ -0,0 +1,22 @@ > +Xilinx fpga reset bridge > + > +The Xilinx reset bridge toggles the reset line to the PL > +in Zynqmp Ultrascale plus. Out of curiosity do you have a reference in the TRM where this is explained? > + > + > +Required properties: > +- compatible : Should contain "xlnx,rst-bridge" > +- reset : reset phandles > + > +Optional properties: > +- bridge-enable : 0 if driver should disable bridge at startup > + 1 if driver should enable bridge at startup > + Default is to leave bridge in current state. > + > +See Documentation/devicetree/bindings/fpga/fpga-region.txt for generic bindings. Since this would be the 5th? time of replicating this I sent out a patch [1] to consolidate this paragraph into a single file. Feel free to add this to your series and replace the above with See Documentation/devicetree/bindings/fpga/fpga-bridge.txt for generic bindings. Thanks Moritz [1] https://lkml.org/lkml/2018/2/21/1099
Hi Moritz, On Wed, Feb 21, 2018 at 11:14 PM, Moritz Fischer <mdf@kernel.org> wrote: > On Wed, Feb 21, 2018 at 10:23:45AM +0530, shubhrajyoti.datta@gmail.com wrote: >> From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> >> >> Add reset bridge support. Once this bridge is enabled. >> The reset line(s) will be toggled. Generally it will be >> called after the bitstream load to reset the PL. >> >> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> >> --- >> .../devicetree/bindings/fpga/xlnx,rst-bridge.txt | 22 ++++++++++++++++++++++ >> 1 file changed, 22 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/fpga/xlnx,rst-bridge.txt >> >> diff --git a/Documentation/devicetree/bindings/fpga/xlnx,rst-bridge.txt b/Documentation/devicetree/bindings/fpga/xlnx,rst-bridge.txt >> new file mode 100644 >> index 0000000..6f1bfc2 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/fpga/xlnx,rst-bridge.txt >> @@ -0,0 +1,22 @@ >> +Xilinx fpga reset bridge >> + >> +The Xilinx reset bridge toggles the reset line to the PL >> +in Zynqmp Ultrascale plus. > > Out of curiosity do you have a reference in the TRM where this is > explained? https://www.xilinx.com/support/documentation/ip_documentation/zynq_ultra_ps_e/v2_0/pg201-zynq-ultrascale-plus-processing-system.pdf section :Fabric Reset Enable >> + >> + >> +Required properties: >> +- compatible : Should contain "xlnx,rst-bridge" >> +- reset : reset phandles >> + >> +Optional properties: >> +- bridge-enable : 0 if driver should disable bridge at startup >> + 1 if driver should enable bridge at startup >> + Default is to leave bridge in current state. >> + >> +See Documentation/devicetree/bindings/fpga/fpga-region.txt for generic bindings. > > Since this would be the 5th? time of replicating this I sent out a patch > [1] to consolidate this paragraph into a single file. > > Feel free to add this to your series and replace the above with > > See Documentation/devicetree/bindings/fpga/fpga-bridge.txt for generic bindings. Will do > > Thanks > > Moritz > > [1] https://lkml.org/lkml/2018/2/21/1099 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 20, 2018 at 10:53 PM, <shubhrajyoti.datta@gmail.com> wrote: > From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > > Adds the reset bridge. After the bitstream load the reset > bridge helps in reseting the programable logic. The > reset lines depends on the design. Hi Shubhrajyoti, OK so adding this 'bridge' will get a reset line to be toggled after programming is done. It looks like it might be generally usable. This doesn't look very xilinx specific (i.e. no specific device registers, just using a reset), so maybe it is just a 'rst-brige'. How is it used? To hit a reset line on a whole FPGA? Alan > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > --- > drivers/fpga/Kconfig | 8 ++++ > drivers/fpga/Makefile | 1 + > drivers/fpga/xilinx-rst-bridge.c | 100 +++++++++++++++++++++++++++++++++++++++ > 3 files changed, 109 insertions(+) > create mode 100644 drivers/fpga/xilinx-rst-bridge.c > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig > index ad5448f..752a907 100644 > --- a/drivers/fpga/Kconfig > +++ b/drivers/fpga/Kconfig > @@ -117,4 +117,12 @@ config XILINX_PR_DECOUPLER > region of the FPGA from the busses while that region is > being reprogrammed during partial reconfig. > > +config XILINX_RST_BRIDGE > + tristate "Xilinx Reset bridge" > + depends on FPGA_BRIDGE > + help > + Say Y to enable drivers for Xilinx Reset bridge. > + After writing the bitstream there has to be a reset. > + The reset lines are design specific. > + > endif # FPGA > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile > index f98dcf1..c1b0d13 100644 > --- a/drivers/fpga/Makefile > +++ b/drivers/fpga/Makefile > @@ -23,6 +23,7 @@ obj-$(CONFIG_FPGA_BRIDGE) += fpga-bridge.o > obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE) += altera-hps2fpga.o altera-fpga2sdram.o > obj-$(CONFIG_ALTERA_FREEZE_BRIDGE) += altera-freeze-bridge.o > obj-$(CONFIG_XILINX_PR_DECOUPLER) += xilinx-pr-decoupler.o > +obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA) += xilinx-rst-bridge.o > > # High Level Interfaces > obj-$(CONFIG_FPGA_REGION) += fpga-region.o > diff --git a/drivers/fpga/xilinx-rst-bridge.c b/drivers/fpga/xilinx-rst-bridge.c > new file mode 100644 > index 0000000..8062283 > --- /dev/null > +++ b/drivers/fpga/xilinx-rst-bridge.c > @@ -0,0 +1,100 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Xilinx FPGA reset bridge. > + * Copyright (c) 2018 Xilinx Inc. > + * > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/fpga/fpga-bridge.h> > +#include <linux/reset.h> > + > +struct xlnx_rst_bridge_priv { > + struct device *dev; > + bool enable; > +}; > + > +static int xlnx_rst_bridge_enable_set(struct fpga_bridge *bridge, bool enable) > +{ > + struct xlnx_rst_bridge_priv *priv = bridge->priv; > + struct device *dev = priv->dev; > + struct reset_control *rstc; > + int ret = 0; > + > + if (enable) { > + rstc = of_reset_control_array_get(dev->of_node, false, false); > + if (IS_ERR(rstc)) > + return PTR_ERR(rstc); > + > + ret = reset_control_reset(rstc); > + > + reset_control_put(rstc); > + > + if (ret) > + dev_err(dev, "Reset failed\n"); > + } else { > + dev_dbg(dev, "Bridge disabled\n"); > + } > + > + if (!ret) > + priv->enable = enable; > + > + return ret; > +} > + > +static int xlnx_rst_bridge_enable_show(struct fpga_bridge *bridge) > +{ > + struct xlnx_rst_bridge_priv *priv = bridge->priv; > + > + return priv->enable; > +} > + > +static struct fpga_bridge_ops xlnx_rst_bridge_ops = { > + .enable_set = xlnx_rst_bridge_enable_set, > + .enable_show = xlnx_rst_bridge_enable_show, > +}; > + > +static int xlnx_rst_bridge_probe(struct platform_device *pdev) > +{ > + struct xlnx_rst_bridge_priv *priv; > + struct device *dev = &pdev->dev; > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->dev = dev; > + > + return fpga_bridge_register(dev, "xlnx_rst_bridge", > + &xlnx_rst_bridge_ops, priv); > +} > + > +static int xlnx_rst_bridge_remove(struct platform_device *pdev) > +{ > + fpga_bridge_unregister(&pdev->dev); > + > + return 0; > +} > + > +static const struct of_device_id xlnx_rst_bridge_of_match[] = { > + { .compatible = "xlnx,rst-bridge", }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, xlnx_rst_bridge_of_match); > + > +static struct platform_driver xlnx_rst_bridge_driver = { > + .probe = xlnx_rst_bridge_probe, > + .remove = xlnx_rst_bridge_remove, > + .driver = { > + .name = "xlnx_rst_bridge", > + .of_match_table = of_match_ptr(xlnx_rst_bridge_of_match), > + }, > +}; > + > +module_platform_driver(xlnx_rst_bridge_driver); > + > +MODULE_DESCRIPTION("Xilinx reset Bridge"); > +MODULE_AUTHOR("Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>"); > +MODULE_LICENSE("GPL v2"); > -- > 2.1.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-fpga" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Shubhrajyoti, Alan On Mon, Feb 26, 2018 at 04:53:59PM -0600, Alan Tull wrote: > On Tue, Feb 20, 2018 at 10:53 PM, <shubhrajyoti.datta@gmail.com> wrote: > > From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > > > > Adds the reset bridge. After the bitstream load the reset > > bridge helps in reseting the programable logic. The > > reset lines depends on the design. > > Hi Shubhrajyoti, > > OK so adding this 'bridge' will get a reset line to be toggled after > programming is done. It looks like it might be generally usable. > This doesn't look very xilinx specific (i.e. no specific device > registers, just using a reset), so maybe it is just a 'rst-brige'. > How is it used? To hit a reset line on a whole FPGA? In zynq-fpga we have the fpga-mgr hit on the reset, which to some extend I suppose makes sense, since after reload you wanna reset the entire FPGA space (unless you do PR, where you don't hit on the resets). Shubhrajyoti's solution seems more modular I guess, however I don't really see a good usecase for only hitting a single reset. IP that's in the FPGA and needs reset should have their dedicated resets via normal reset API bindings imho and not rely on bridges to do the right thing. The ZynqMP is fairly complex and I might be missing something here, so maybe you (Shubhrajyoti) can elaborate a bit more. The paragraph in the TRM seemed fairly short, and didn't enlighten me as to why it is required. Cheers, Moritz > > Alan > > > > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > > --- > > drivers/fpga/Kconfig | 8 ++++ > > drivers/fpga/Makefile | 1 + > > drivers/fpga/xilinx-rst-bridge.c | 100 +++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 109 insertions(+) > > create mode 100644 drivers/fpga/xilinx-rst-bridge.c > > > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig > > index ad5448f..752a907 100644 > > --- a/drivers/fpga/Kconfig > > +++ b/drivers/fpga/Kconfig > > @@ -117,4 +117,12 @@ config XILINX_PR_DECOUPLER > > region of the FPGA from the busses while that region is > > being reprogrammed during partial reconfig. > > > > +config XILINX_RST_BRIDGE > > + tristate "Xilinx Reset bridge" > > + depends on FPGA_BRIDGE > > + help > > + Say Y to enable drivers for Xilinx Reset bridge. > > + After writing the bitstream there has to be a reset. > > + The reset lines are design specific. > > + > > endif # FPGA > > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile > > index f98dcf1..c1b0d13 100644 > > --- a/drivers/fpga/Makefile > > +++ b/drivers/fpga/Makefile > > @@ -23,6 +23,7 @@ obj-$(CONFIG_FPGA_BRIDGE) += fpga-bridge.o > > obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE) += altera-hps2fpga.o altera-fpga2sdram.o > > obj-$(CONFIG_ALTERA_FREEZE_BRIDGE) += altera-freeze-bridge.o > > obj-$(CONFIG_XILINX_PR_DECOUPLER) += xilinx-pr-decoupler.o > > +obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA) += xilinx-rst-bridge.o > > > > # High Level Interfaces > > obj-$(CONFIG_FPGA_REGION) += fpga-region.o > > diff --git a/drivers/fpga/xilinx-rst-bridge.c b/drivers/fpga/xilinx-rst-bridge.c > > new file mode 100644 > > index 0000000..8062283 > > --- /dev/null > > +++ b/drivers/fpga/xilinx-rst-bridge.c > > @@ -0,0 +1,100 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Xilinx FPGA reset bridge. > > + * Copyright (c) 2018 Xilinx Inc. > > + * > > + */ > > + > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/of_device.h> > > +#include <linux/fpga/fpga-bridge.h> > > +#include <linux/reset.h> > > + > > +struct xlnx_rst_bridge_priv { > > + struct device *dev; > > + bool enable; > > +}; > > + > > +static int xlnx_rst_bridge_enable_set(struct fpga_bridge *bridge, bool enable) > > +{ > > + struct xlnx_rst_bridge_priv *priv = bridge->priv; > > + struct device *dev = priv->dev; > > + struct reset_control *rstc; > > + int ret = 0; > > + > > + if (enable) { > > + rstc = of_reset_control_array_get(dev->of_node, false, false); > > + if (IS_ERR(rstc)) > > + return PTR_ERR(rstc); > > + > > + ret = reset_control_reset(rstc); > > + > > + reset_control_put(rstc); > > + > > + if (ret) > > + dev_err(dev, "Reset failed\n"); > > + } else { > > + dev_dbg(dev, "Bridge disabled\n"); > > + } > > + > > + if (!ret) > > + priv->enable = enable; > > + > > + return ret; > > +} > > + > > +static int xlnx_rst_bridge_enable_show(struct fpga_bridge *bridge) > > +{ > > + struct xlnx_rst_bridge_priv *priv = bridge->priv; > > + > > + return priv->enable; > > +} > > + > > +static struct fpga_bridge_ops xlnx_rst_bridge_ops = { > > + .enable_set = xlnx_rst_bridge_enable_set, > > + .enable_show = xlnx_rst_bridge_enable_show, > > +}; > > + > > +static int xlnx_rst_bridge_probe(struct platform_device *pdev) > > +{ > > + struct xlnx_rst_bridge_priv *priv; > > + struct device *dev = &pdev->dev; > > + > > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + priv->dev = dev; > > + > > + return fpga_bridge_register(dev, "xlnx_rst_bridge", > > + &xlnx_rst_bridge_ops, priv); > > +} > > + > > +static int xlnx_rst_bridge_remove(struct platform_device *pdev) > > +{ > > + fpga_bridge_unregister(&pdev->dev); > > + > > + return 0; > > +} > > + > > +static const struct of_device_id xlnx_rst_bridge_of_match[] = { > > + { .compatible = "xlnx,rst-bridge", }, > > + {}, > > +}; > > +MODULE_DEVICE_TABLE(of, xlnx_rst_bridge_of_match); > > + > > +static struct platform_driver xlnx_rst_bridge_driver = { > > + .probe = xlnx_rst_bridge_probe, > > + .remove = xlnx_rst_bridge_remove, > > + .driver = { > > + .name = "xlnx_rst_bridge", > > + .of_match_table = of_match_ptr(xlnx_rst_bridge_of_match), > > + }, > > +}; > > + > > +module_platform_driver(xlnx_rst_bridge_driver); > > + > > +MODULE_DESCRIPTION("Xilinx reset Bridge"); > > +MODULE_AUTHOR("Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>"); > > +MODULE_LICENSE("GPL v2"); > > -- > > 2.1.1 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-fpga" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Moritz, On Tue, Feb 27, 2018 at 5:58 AM, Moritz Fischer <mdf@kernel.org> wrote: > Hi Shubhrajyoti, Alan > > On Mon, Feb 26, 2018 at 04:53:59PM -0600, Alan Tull wrote: >> On Tue, Feb 20, 2018 at 10:53 PM, <shubhrajyoti.datta@gmail.com> wrote: >> > From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> >> > >> > Adds the reset bridge. After the bitstream load the reset >> > bridge helps in reseting the programable logic. The >> > reset lines depends on the design. >> >> Hi Shubhrajyoti, >> >> OK so adding this 'bridge' will get a reset line to be toggled after >> programming is done. It looks like it might be generally usable. >> This doesn't look very xilinx specific (i.e. no specific device >> registers, just using a reset), so maybe it is just a 'rst-brige'. >> How is it used? To hit a reset line on a whole FPGA? > > In zynq-fpga we have the fpga-mgr hit on the reset, which to some > extend I suppose makes sense, since after reload you wanna reset the > entire FPGA space (unless you do PR, where you don't hit on the resets). > > Shubhrajyoti's solution seems more modular I guess, however I don't > really see a good usecase for only hitting a single reset. > > IP that's in the FPGA and needs reset should have their dedicated resets > via normal reset API bindings imho and not rely on bridges to do the > right thing. > > The ZynqMP is fairly complex and I might be missing something here, > so maybe you (Shubhrajyoti) can elaborate a bit more. The paragraph in > the TRM seemed fairly short, and didn't enlighten me as to why it is > required. The FPGA supports both the PR case and independent execution Like a design can have 2 parts that can have independent execution. In that case they have to have independent resets. There are 4 PS-PL lines that could be used by designs. So in that case we should have multiple resets. > > Cheers, > > Moritz >> >> Alan >> >> > >> > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> >> > --- >> > drivers/fpga/Kconfig | 8 ++++ >> > drivers/fpga/Makefile | 1 + >> > drivers/fpga/xilinx-rst-bridge.c | 100 +++++++++++++++++++++++++++++++++++++++ >> > 3 files changed, 109 insertions(+) >> > create mode 100644 drivers/fpga/xilinx-rst-bridge.c >> > >> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig >> > index ad5448f..752a907 100644 >> > --- a/drivers/fpga/Kconfig >> > +++ b/drivers/fpga/Kconfig >> > @@ -117,4 +117,12 @@ config XILINX_PR_DECOUPLER >> > region of the FPGA from the busses while that region is >> > being reprogrammed during partial reconfig. >> > >> > +config XILINX_RST_BRIDGE >> > + tristate "Xilinx Reset bridge" >> > + depends on FPGA_BRIDGE >> > + help >> > + Say Y to enable drivers for Xilinx Reset bridge. >> > + After writing the bitstream there has to be a reset. >> > + The reset lines are design specific. >> > + >> > endif # FPGA >> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile >> > index f98dcf1..c1b0d13 100644 >> > --- a/drivers/fpga/Makefile >> > +++ b/drivers/fpga/Makefile >> > @@ -23,6 +23,7 @@ obj-$(CONFIG_FPGA_BRIDGE) += fpga-bridge.o >> > obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE) += altera-hps2fpga.o altera-fpga2sdram.o >> > obj-$(CONFIG_ALTERA_FREEZE_BRIDGE) += altera-freeze-bridge.o >> > obj-$(CONFIG_XILINX_PR_DECOUPLER) += xilinx-pr-decoupler.o >> > +obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA) += xilinx-rst-bridge.o >> > >> > # High Level Interfaces >> > obj-$(CONFIG_FPGA_REGION) += fpga-region.o >> > diff --git a/drivers/fpga/xilinx-rst-bridge.c b/drivers/fpga/xilinx-rst-bridge.c >> > new file mode 100644 >> > index 0000000..8062283 >> > --- /dev/null >> > +++ b/drivers/fpga/xilinx-rst-bridge.c >> > @@ -0,0 +1,100 @@ >> > +// SPDX-License-Identifier: GPL-2.0 >> > +/* >> > + * Xilinx FPGA reset bridge. >> > + * Copyright (c) 2018 Xilinx Inc. >> > + * >> > + */ >> > + >> > +#include <linux/kernel.h> >> > +#include <linux/module.h> >> > +#include <linux/of_device.h> >> > +#include <linux/fpga/fpga-bridge.h> >> > +#include <linux/reset.h> >> > + >> > +struct xlnx_rst_bridge_priv { >> > + struct device *dev; >> > + bool enable; >> > +}; >> > + >> > +static int xlnx_rst_bridge_enable_set(struct fpga_bridge *bridge, bool enable) >> > +{ >> > + struct xlnx_rst_bridge_priv *priv = bridge->priv; >> > + struct device *dev = priv->dev; >> > + struct reset_control *rstc; >> > + int ret = 0; >> > + >> > + if (enable) { >> > + rstc = of_reset_control_array_get(dev->of_node, false, false); >> > + if (IS_ERR(rstc)) >> > + return PTR_ERR(rstc); >> > + >> > + ret = reset_control_reset(rstc); >> > + >> > + reset_control_put(rstc); >> > + >> > + if (ret) >> > + dev_err(dev, "Reset failed\n"); >> > + } else { >> > + dev_dbg(dev, "Bridge disabled\n"); >> > + } >> > + >> > + if (!ret) >> > + priv->enable = enable; >> > + >> > + return ret; >> > +} >> > + >> > +static int xlnx_rst_bridge_enable_show(struct fpga_bridge *bridge) >> > +{ >> > + struct xlnx_rst_bridge_priv *priv = bridge->priv; >> > + >> > + return priv->enable; >> > +} >> > + >> > +static struct fpga_bridge_ops xlnx_rst_bridge_ops = { >> > + .enable_set = xlnx_rst_bridge_enable_set, >> > + .enable_show = xlnx_rst_bridge_enable_show, >> > +}; >> > + >> > +static int xlnx_rst_bridge_probe(struct platform_device *pdev) >> > +{ >> > + struct xlnx_rst_bridge_priv *priv; >> > + struct device *dev = &pdev->dev; >> > + >> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >> > + if (!priv) >> > + return -ENOMEM; >> > + >> > + priv->dev = dev; >> > + >> > + return fpga_bridge_register(dev, "xlnx_rst_bridge", >> > + &xlnx_rst_bridge_ops, priv); >> > +} >> > + >> > +static int xlnx_rst_bridge_remove(struct platform_device *pdev) >> > +{ >> > + fpga_bridge_unregister(&pdev->dev); >> > + >> > + return 0; >> > +} >> > + >> > +static const struct of_device_id xlnx_rst_bridge_of_match[] = { >> > + { .compatible = "xlnx,rst-bridge", }, >> > + {}, >> > +}; >> > +MODULE_DEVICE_TABLE(of, xlnx_rst_bridge_of_match); >> > + >> > +static struct platform_driver xlnx_rst_bridge_driver = { >> > + .probe = xlnx_rst_bridge_probe, >> > + .remove = xlnx_rst_bridge_remove, >> > + .driver = { >> > + .name = "xlnx_rst_bridge", >> > + .of_match_table = of_match_ptr(xlnx_rst_bridge_of_match), >> > + }, >> > +}; >> > + >> > +module_platform_driver(xlnx_rst_bridge_driver); >> > + >> > +MODULE_DESCRIPTION("Xilinx reset Bridge"); >> > +MODULE_AUTHOR("Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>"); >> > +MODULE_LICENSE("GPL v2"); >> > -- >> > 2.1.1 >> > >> > -- >> > To unsubscribe from this list: send the line "unsubscribe linux-fpga" in >> > the body of a message to majordomo@vger.kernel.org >> > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Alan, On Tue, Feb 27, 2018 at 4:23 AM, Alan Tull <atull@kernel.org> wrote: > On Tue, Feb 20, 2018 at 10:53 PM, <shubhrajyoti.datta@gmail.com> wrote: >> From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> >> >> Adds the reset bridge. After the bitstream load the reset >> bridge helps in reseting the programable logic. The >> reset lines depends on the design. > > Hi Shubhrajyoti, > > OK so adding this 'bridge' will get a reset line to be toggled after > programming is done. It looks like it might be generally usable. > This doesn't look very xilinx specific (i.e. no specific device > registers, just using a reset), I agree. However if there the reset line in some cases is customizable in design. > so maybe it is just a 'rst-brige'. > How is it used? To hit a reset line on a whole FPGA? There are reset lines (configurable max 4 ) from PS_PL . In most cases there is 1 reset line for the design. However our hardware supports independent design (executed independently) in that case we may need multiple resets. [1] AR describes the reset lines. [1] https://www.xilinx.com/support/answers/68962.html > > Alan > >> >> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> >> --- >> drivers/fpga/Kconfig | 8 ++++ >> drivers/fpga/Makefile | 1 + >> drivers/fpga/xilinx-rst-bridge.c | 100 +++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 109 insertions(+) >> create mode 100644 drivers/fpga/xilinx-rst-bridge.c >> >> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig >> index ad5448f..752a907 100644 >> --- a/drivers/fpga/Kconfig >> +++ b/drivers/fpga/Kconfig >> @@ -117,4 +117,12 @@ config XILINX_PR_DECOUPLER >> region of the FPGA from the busses while that region is >> being reprogrammed during partial reconfig. >> >> +config XILINX_RST_BRIDGE >> + tristate "Xilinx Reset bridge" >> + depends on FPGA_BRIDGE >> + help >> + Say Y to enable drivers for Xilinx Reset bridge. >> + After writing the bitstream there has to be a reset. >> + The reset lines are design specific. >> + >> endif # FPGA >> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile >> index f98dcf1..c1b0d13 100644 >> --- a/drivers/fpga/Makefile >> +++ b/drivers/fpga/Makefile >> @@ -23,6 +23,7 @@ obj-$(CONFIG_FPGA_BRIDGE) += fpga-bridge.o >> obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE) += altera-hps2fpga.o altera-fpga2sdram.o >> obj-$(CONFIG_ALTERA_FREEZE_BRIDGE) += altera-freeze-bridge.o >> obj-$(CONFIG_XILINX_PR_DECOUPLER) += xilinx-pr-decoupler.o >> +obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA) += xilinx-rst-bridge.o >> >> # High Level Interfaces >> obj-$(CONFIG_FPGA_REGION) += fpga-region.o >> diff --git a/drivers/fpga/xilinx-rst-bridge.c b/drivers/fpga/xilinx-rst-bridge.c >> new file mode 100644 >> index 0000000..8062283 >> --- /dev/null >> +++ b/drivers/fpga/xilinx-rst-bridge.c >> @@ -0,0 +1,100 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Xilinx FPGA reset bridge. >> + * Copyright (c) 2018 Xilinx Inc. >> + * >> + */ >> + >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/of_device.h> >> +#include <linux/fpga/fpga-bridge.h> >> +#include <linux/reset.h> >> + >> +struct xlnx_rst_bridge_priv { >> + struct device *dev; >> + bool enable; >> +}; >> + >> +static int xlnx_rst_bridge_enable_set(struct fpga_bridge *bridge, bool enable) >> +{ >> + struct xlnx_rst_bridge_priv *priv = bridge->priv; >> + struct device *dev = priv->dev; >> + struct reset_control *rstc; >> + int ret = 0; >> + >> + if (enable) { >> + rstc = of_reset_control_array_get(dev->of_node, false, false); >> + if (IS_ERR(rstc)) >> + return PTR_ERR(rstc); >> + >> + ret = reset_control_reset(rstc); >> + >> + reset_control_put(rstc); >> + >> + if (ret) >> + dev_err(dev, "Reset failed\n"); >> + } else { >> + dev_dbg(dev, "Bridge disabled\n"); >> + } >> + >> + if (!ret) >> + priv->enable = enable; >> + >> + return ret; >> +} >> + >> +static int xlnx_rst_bridge_enable_show(struct fpga_bridge *bridge) >> +{ >> + struct xlnx_rst_bridge_priv *priv = bridge->priv; >> + >> + return priv->enable; >> +} >> + >> +static struct fpga_bridge_ops xlnx_rst_bridge_ops = { >> + .enable_set = xlnx_rst_bridge_enable_set, >> + .enable_show = xlnx_rst_bridge_enable_show, >> +}; >> + >> +static int xlnx_rst_bridge_probe(struct platform_device *pdev) >> +{ >> + struct xlnx_rst_bridge_priv *priv; >> + struct device *dev = &pdev->dev; >> + >> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >> + if (!priv) >> + return -ENOMEM; >> + >> + priv->dev = dev; >> + >> + return fpga_bridge_register(dev, "xlnx_rst_bridge", >> + &xlnx_rst_bridge_ops, priv); >> +} >> + >> +static int xlnx_rst_bridge_remove(struct platform_device *pdev) >> +{ >> + fpga_bridge_unregister(&pdev->dev); >> + >> + return 0; >> +} >> + >> +static const struct of_device_id xlnx_rst_bridge_of_match[] = { >> + { .compatible = "xlnx,rst-bridge", }, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, xlnx_rst_bridge_of_match); >> + >> +static struct platform_driver xlnx_rst_bridge_driver = { >> + .probe = xlnx_rst_bridge_probe, >> + .remove = xlnx_rst_bridge_remove, >> + .driver = { >> + .name = "xlnx_rst_bridge", >> + .of_match_table = of_match_ptr(xlnx_rst_bridge_of_match), >> + }, >> +}; >> + >> +module_platform_driver(xlnx_rst_bridge_driver); >> + >> +MODULE_DESCRIPTION("Xilinx reset Bridge"); >> +MODULE_AUTHOR("Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>"); >> +MODULE_LICENSE("GPL v2"); >> -- >> 2.1.1 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 26, 2018 at 10:46 PM, Shubhrajyoti Datta <shubhrajyoti.datta@gmail.com> wrote: > Hi Moritz, > > On Tue, Feb 27, 2018 at 5:58 AM, Moritz Fischer <mdf@kernel.org> wrote: >> Hi Shubhrajyoti, Alan >> >> On Mon, Feb 26, 2018 at 04:53:59PM -0600, Alan Tull wrote: >>> On Tue, Feb 20, 2018 at 10:53 PM, <shubhrajyoti.datta@gmail.com> wrote: >>> > From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> >>> > >>> > Adds the reset bridge. After the bitstream load the reset >>> > bridge helps in reseting the programable logic. The >>> > reset lines depends on the design. >>> >>> Hi Shubhrajyoti, >>> >>> OK so adding this 'bridge' will get a reset line to be toggled after >>> programming is done. It looks like it might be generally usable. >>> This doesn't look very xilinx specific (i.e. no specific device >>> registers, just using a reset), so maybe it is just a 'rst-brige'. >>> How is it used? To hit a reset line on a whole FPGA? >> >> In zynq-fpga we have the fpga-mgr hit on the reset, which to some >> extend I suppose makes sense, since after reload you wanna reset the >> entire FPGA space (unless you do PR, where you don't hit on the resets). >> >> Shubhrajyoti's solution seems more modular I guess, however I don't >> really see a good usecase for only hitting a single reset. >> >> IP that's in the FPGA and needs reset should have their dedicated resets >> via normal reset API bindings imho and not rely on bridges to do the >> right thing. >> >> The ZynqMP is fairly complex and I might be missing something here, >> so maybe you (Shubhrajyoti) can elaborate a bit more. The paragraph in >> the TRM seemed fairly short, and didn't enlighten me as to why it is >> required. > > The FPGA supports both the PR case and independent execution > Like a design can have 2 parts that can have independent execution. I want to understand you better here. What do you mean by 'independent execution'? Like 2 partial reconfiguration regions that each have a single reset to reset the hardware in their region? Or something else? > In that case they have to have independent resets. > > There are 4 PS-PL lines that could be used by designs. > So in that case we should have multiple resets. Normally, as Moritz is saying, the reset would be handled by the driver for the fabric-based hardware. Alan > > >> >> Cheers, >> >> Moritz >>> >>> Alan >>> >>> > >>> > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> >>> > --- >>> > drivers/fpga/Kconfig | 8 ++++ >>> > drivers/fpga/Makefile | 1 + >>> > drivers/fpga/xilinx-rst-bridge.c | 100 +++++++++++++++++++++++++++++++++++++++ >>> > 3 files changed, 109 insertions(+) >>> > create mode 100644 drivers/fpga/xilinx-rst-bridge.c >>> > >>> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig >>> > index ad5448f..752a907 100644 >>> > --- a/drivers/fpga/Kconfig >>> > +++ b/drivers/fpga/Kconfig >>> > @@ -117,4 +117,12 @@ config XILINX_PR_DECOUPLER >>> > region of the FPGA from the busses while that region is >>> > being reprogrammed during partial reconfig. >>> > >>> > +config XILINX_RST_BRIDGE >>> > + tristate "Xilinx Reset bridge" >>> > + depends on FPGA_BRIDGE >>> > + help >>> > + Say Y to enable drivers for Xilinx Reset bridge. >>> > + After writing the bitstream there has to be a reset. >>> > + The reset lines are design specific. >>> > + >>> > endif # FPGA >>> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile >>> > index f98dcf1..c1b0d13 100644 >>> > --- a/drivers/fpga/Makefile >>> > +++ b/drivers/fpga/Makefile >>> > @@ -23,6 +23,7 @@ obj-$(CONFIG_FPGA_BRIDGE) += fpga-bridge.o >>> > obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE) += altera-hps2fpga.o altera-fpga2sdram.o >>> > obj-$(CONFIG_ALTERA_FREEZE_BRIDGE) += altera-freeze-bridge.o >>> > obj-$(CONFIG_XILINX_PR_DECOUPLER) += xilinx-pr-decoupler.o >>> > +obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA) += xilinx-rst-bridge.o >>> > >>> > # High Level Interfaces >>> > obj-$(CONFIG_FPGA_REGION) += fpga-region.o >>> > diff --git a/drivers/fpga/xilinx-rst-bridge.c b/drivers/fpga/xilinx-rst-bridge.c >>> > new file mode 100644 >>> > index 0000000..8062283 >>> > --- /dev/null >>> > +++ b/drivers/fpga/xilinx-rst-bridge.c >>> > @@ -0,0 +1,100 @@ >>> > +// SPDX-License-Identifier: GPL-2.0 >>> > +/* >>> > + * Xilinx FPGA reset bridge. >>> > + * Copyright (c) 2018 Xilinx Inc. >>> > + * >>> > + */ >>> > + >>> > +#include <linux/kernel.h> >>> > +#include <linux/module.h> >>> > +#include <linux/of_device.h> >>> > +#include <linux/fpga/fpga-bridge.h> >>> > +#include <linux/reset.h> >>> > + >>> > +struct xlnx_rst_bridge_priv { >>> > + struct device *dev; >>> > + bool enable; >>> > +}; >>> > + >>> > +static int xlnx_rst_bridge_enable_set(struct fpga_bridge *bridge, bool enable) >>> > +{ >>> > + struct xlnx_rst_bridge_priv *priv = bridge->priv; >>> > + struct device *dev = priv->dev; >>> > + struct reset_control *rstc; >>> > + int ret = 0; >>> > + >>> > + if (enable) { >>> > + rstc = of_reset_control_array_get(dev->of_node, false, false); >>> > + if (IS_ERR(rstc)) >>> > + return PTR_ERR(rstc); >>> > + >>> > + ret = reset_control_reset(rstc); >>> > + >>> > + reset_control_put(rstc); >>> > + >>> > + if (ret) >>> > + dev_err(dev, "Reset failed\n"); >>> > + } else { >>> > + dev_dbg(dev, "Bridge disabled\n"); >>> > + } >>> > + >>> > + if (!ret) >>> > + priv->enable = enable; >>> > + >>> > + return ret; >>> > +} >>> > + >>> > +static int xlnx_rst_bridge_enable_show(struct fpga_bridge *bridge) >>> > +{ >>> > + struct xlnx_rst_bridge_priv *priv = bridge->priv; >>> > + >>> > + return priv->enable; >>> > +} >>> > + >>> > +static struct fpga_bridge_ops xlnx_rst_bridge_ops = { >>> > + .enable_set = xlnx_rst_bridge_enable_set, >>> > + .enable_show = xlnx_rst_bridge_enable_show, >>> > +}; >>> > + >>> > +static int xlnx_rst_bridge_probe(struct platform_device *pdev) >>> > +{ >>> > + struct xlnx_rst_bridge_priv *priv; >>> > + struct device *dev = &pdev->dev; >>> > + >>> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >>> > + if (!priv) >>> > + return -ENOMEM; >>> > + >>> > + priv->dev = dev; >>> > + >>> > + return fpga_bridge_register(dev, "xlnx_rst_bridge", >>> > + &xlnx_rst_bridge_ops, priv); >>> > +} >>> > + >>> > +static int xlnx_rst_bridge_remove(struct platform_device *pdev) >>> > +{ >>> > + fpga_bridge_unregister(&pdev->dev); >>> > + >>> > + return 0; >>> > +} >>> > + >>> > +static const struct of_device_id xlnx_rst_bridge_of_match[] = { >>> > + { .compatible = "xlnx,rst-bridge", }, >>> > + {}, >>> > +}; >>> > +MODULE_DEVICE_TABLE(of, xlnx_rst_bridge_of_match); >>> > + >>> > +static struct platform_driver xlnx_rst_bridge_driver = { >>> > + .probe = xlnx_rst_bridge_probe, >>> > + .remove = xlnx_rst_bridge_remove, >>> > + .driver = { >>> > + .name = "xlnx_rst_bridge", >>> > + .of_match_table = of_match_ptr(xlnx_rst_bridge_of_match), >>> > + }, >>> > +}; >>> > + >>> > +module_platform_driver(xlnx_rst_bridge_driver); >>> > + >>> > +MODULE_DESCRIPTION("Xilinx reset Bridge"); >>> > +MODULE_AUTHOR("Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>"); >>> > +MODULE_LICENSE("GPL v2"); >>> > -- >>> > 2.1.1 >>> > >>> > -- >>> > To unsubscribe from this list: send the line "unsubscribe linux-fpga" in >>> > the body of a message to majordomo@vger.kernel.org >>> > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 27, 2018 at 10:30 PM, Alan Tull <atull@kernel.org> wrote: > On Mon, Feb 26, 2018 at 10:46 PM, Shubhrajyoti Datta > <shubhrajyoti.datta@gmail.com> wrote: >> Hi Moritz, >> >> On Tue, Feb 27, 2018 at 5:58 AM, Moritz Fischer <mdf@kernel.org> wrote: >>> Hi Shubhrajyoti, Alan >>> >>> On Mon, Feb 26, 2018 at 04:53:59PM -0600, Alan Tull wrote: >>>> On Tue, Feb 20, 2018 at 10:53 PM, <shubhrajyoti.datta@gmail.com> wrote: >>>> > From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> >>>> > >>>> > Adds the reset bridge. After the bitstream load the reset >>>> > bridge helps in reseting the programable logic. The >>>> > reset lines depends on the design. >>>> >>>> Hi Shubhrajyoti, >>>> >>>> OK so adding this 'bridge' will get a reset line to be toggled after >>>> programming is done. It looks like it might be generally usable. >>>> This doesn't look very xilinx specific (i.e. no specific device >>>> registers, just using a reset), so maybe it is just a 'rst-brige'. >>>> How is it used? To hit a reset line on a whole FPGA? >>> >>> In zynq-fpga we have the fpga-mgr hit on the reset, which to some >>> extend I suppose makes sense, since after reload you wanna reset the >>> entire FPGA space (unless you do PR, where you don't hit on the resets). >>> >>> Shubhrajyoti's solution seems more modular I guess, however I don't >>> really see a good usecase for only hitting a single reset. >>> >>> IP that's in the FPGA and needs reset should have their dedicated resets >>> via normal reset API bindings imho and not rely on bridges to do the >>> right thing. >>> >>> The ZynqMP is fairly complex and I might be missing something here, >>> so maybe you (Shubhrajyoti) can elaborate a bit more. The paragraph in >>> the TRM seemed fairly short, and didn't enlighten me as to why it is >>> required. >> >> The FPGA supports both the PR case and independent execution >> Like a design can have 2 parts that can have independent execution. > > I want to understand you better here. What do you mean by > 'independent execution'? Like 2 partial reconfiguration regions that > each have a single reset to reset the hardware in their region? Or > something else? Yes. > >> In that case they have to have independent resets. >> >> There are 4 PS-PL lines that could be used by designs. >> So in that case we should have multiple resets. > > Normally, as Moritz is saying, the reset would be handled by the > driver for the fabric-based hardware. I didnt understand you mean the platform-fpga.c ? > > Alan > >> >> >>> >>> Cheers, >>> >>> Moritz >>>> >>>> Alan >>>> >>>> > >>>> > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> >>>> > --- >>>> > drivers/fpga/Kconfig | 8 ++++ >>>> > drivers/fpga/Makefile | 1 + >>>> > drivers/fpga/xilinx-rst-bridge.c | 100 +++++++++++++++++++++++++++++++++++++++ >>>> > 3 files changed, 109 insertions(+) >>>> > create mode 100644 drivers/fpga/xilinx-rst-bridge.c >>>> > >>>> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig >>>> > index ad5448f..752a907 100644 >>>> > --- a/drivers/fpga/Kconfig >>>> > +++ b/drivers/fpga/Kconfig >>>> > @@ -117,4 +117,12 @@ config XILINX_PR_DECOUPLER >>>> > region of the FPGA from the busses while that region is >>>> > being reprogrammed during partial reconfig. >>>> > >>>> > +config XILINX_RST_BRIDGE >>>> > + tristate "Xilinx Reset bridge" >>>> > + depends on FPGA_BRIDGE >>>> > + help >>>> > + Say Y to enable drivers for Xilinx Reset bridge. >>>> > + After writing the bitstream there has to be a reset. >>>> > + The reset lines are design specific. >>>> > + >>>> > endif # FPGA >>>> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile >>>> > index f98dcf1..c1b0d13 100644 >>>> > --- a/drivers/fpga/Makefile >>>> > +++ b/drivers/fpga/Makefile >>>> > @@ -23,6 +23,7 @@ obj-$(CONFIG_FPGA_BRIDGE) += fpga-bridge.o >>>> > obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE) += altera-hps2fpga.o altera-fpga2sdram.o >>>> > obj-$(CONFIG_ALTERA_FREEZE_BRIDGE) += altera-freeze-bridge.o >>>> > obj-$(CONFIG_XILINX_PR_DECOUPLER) += xilinx-pr-decoupler.o >>>> > +obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA) += xilinx-rst-bridge.o >>>> > >>>> > # High Level Interfaces >>>> > obj-$(CONFIG_FPGA_REGION) += fpga-region.o >>>> > diff --git a/drivers/fpga/xilinx-rst-bridge.c b/drivers/fpga/xilinx-rst-bridge.c >>>> > new file mode 100644 >>>> > index 0000000..8062283 >>>> > --- /dev/null >>>> > +++ b/drivers/fpga/xilinx-rst-bridge.c >>>> > @@ -0,0 +1,100 @@ >>>> > +// SPDX-License-Identifier: GPL-2.0 >>>> > +/* >>>> > + * Xilinx FPGA reset bridge. >>>> > + * Copyright (c) 2018 Xilinx Inc. >>>> > + * >>>> > + */ >>>> > + >>>> > +#include <linux/kernel.h> >>>> > +#include <linux/module.h> >>>> > +#include <linux/of_device.h> >>>> > +#include <linux/fpga/fpga-bridge.h> >>>> > +#include <linux/reset.h> >>>> > + >>>> > +struct xlnx_rst_bridge_priv { >>>> > + struct device *dev; >>>> > + bool enable; >>>> > +}; >>>> > + >>>> > +static int xlnx_rst_bridge_enable_set(struct fpga_bridge *bridge, bool enable) >>>> > +{ >>>> > + struct xlnx_rst_bridge_priv *priv = bridge->priv; >>>> > + struct device *dev = priv->dev; >>>> > + struct reset_control *rstc; >>>> > + int ret = 0; >>>> > + >>>> > + if (enable) { >>>> > + rstc = of_reset_control_array_get(dev->of_node, false, false); >>>> > + if (IS_ERR(rstc)) >>>> > + return PTR_ERR(rstc); >>>> > + >>>> > + ret = reset_control_reset(rstc); >>>> > + >>>> > + reset_control_put(rstc); >>>> > + >>>> > + if (ret) >>>> > + dev_err(dev, "Reset failed\n"); >>>> > + } else { >>>> > + dev_dbg(dev, "Bridge disabled\n"); >>>> > + } >>>> > + >>>> > + if (!ret) >>>> > + priv->enable = enable; >>>> > + >>>> > + return ret; >>>> > +} >>>> > + >>>> > +static int xlnx_rst_bridge_enable_show(struct fpga_bridge *bridge) >>>> > +{ >>>> > + struct xlnx_rst_bridge_priv *priv = bridge->priv; >>>> > + >>>> > + return priv->enable; >>>> > +} >>>> > + >>>> > +static struct fpga_bridge_ops xlnx_rst_bridge_ops = { >>>> > + .enable_set = xlnx_rst_bridge_enable_set, >>>> > + .enable_show = xlnx_rst_bridge_enable_show, >>>> > +}; >>>> > + >>>> > +static int xlnx_rst_bridge_probe(struct platform_device *pdev) >>>> > +{ >>>> > + struct xlnx_rst_bridge_priv *priv; >>>> > + struct device *dev = &pdev->dev; >>>> > + >>>> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >>>> > + if (!priv) >>>> > + return -ENOMEM; >>>> > + >>>> > + priv->dev = dev; >>>> > + >>>> > + return fpga_bridge_register(dev, "xlnx_rst_bridge", >>>> > + &xlnx_rst_bridge_ops, priv); >>>> > +} >>>> > + >>>> > +static int xlnx_rst_bridge_remove(struct platform_device *pdev) >>>> > +{ >>>> > + fpga_bridge_unregister(&pdev->dev); >>>> > + >>>> > + return 0; >>>> > +} >>>> > + >>>> > +static const struct of_device_id xlnx_rst_bridge_of_match[] = { >>>> > + { .compatible = "xlnx,rst-bridge", }, >>>> > + {}, >>>> > +}; >>>> > +MODULE_DEVICE_TABLE(of, xlnx_rst_bridge_of_match); >>>> > + >>>> > +static struct platform_driver xlnx_rst_bridge_driver = { >>>> > + .probe = xlnx_rst_bridge_probe, >>>> > + .remove = xlnx_rst_bridge_remove, >>>> > + .driver = { >>>> > + .name = "xlnx_rst_bridge", >>>> > + .of_match_table = of_match_ptr(xlnx_rst_bridge_of_match), >>>> > + }, >>>> > +}; >>>> > + >>>> > +module_platform_driver(xlnx_rst_bridge_driver); >>>> > + >>>> > +MODULE_DESCRIPTION("Xilinx reset Bridge"); >>>> > +MODULE_AUTHOR("Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>"); >>>> > +MODULE_LICENSE("GPL v2"); >>>> > -- >>>> > 2.1.1 >>>> > >>>> > -- >>>> > To unsubscribe from this list: send the line "unsubscribe linux-fpga" in >>>> > the body of a message to majordomo@vger.kernel.org >>>> > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Shubhrajyoti, On Thu, Mar 01, 2018 at 04:19:48PM +0530, Shubhrajyoti Datta wrote: > On Tue, Feb 27, 2018 at 10:30 PM, Alan Tull <atull@kernel.org> wrote: > > On Mon, Feb 26, 2018 at 10:46 PM, Shubhrajyoti Datta > > <shubhrajyoti.datta@gmail.com> wrote: > >> Hi Moritz, > >> > >> On Tue, Feb 27, 2018 at 5:58 AM, Moritz Fischer <mdf@kernel.org> wrote: > >>> Hi Shubhrajyoti, Alan > >>> > >>> On Mon, Feb 26, 2018 at 04:53:59PM -0600, Alan Tull wrote: > >>>> On Tue, Feb 20, 2018 at 10:53 PM, <shubhrajyoti.datta@gmail.com> wrote: > >>>> > From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > >>>> > > >>>> > Adds the reset bridge. After the bitstream load the reset > >>>> > bridge helps in reseting the programable logic. The > >>>> > reset lines depends on the design. > >>>> > >>>> Hi Shubhrajyoti, > >>>> > >>>> OK so adding this 'bridge' will get a reset line to be toggled after > >>>> programming is done. It looks like it might be generally usable. > >>>> This doesn't look very xilinx specific (i.e. no specific device > >>>> registers, just using a reset), so maybe it is just a 'rst-brige'. > >>>> How is it used? To hit a reset line on a whole FPGA? > >>> > >>> In zynq-fpga we have the fpga-mgr hit on the reset, which to some > >>> extend I suppose makes sense, since after reload you wanna reset the > >>> entire FPGA space (unless you do PR, where you don't hit on the resets). > >>> > >>> Shubhrajyoti's solution seems more modular I guess, however I don't > >>> really see a good usecase for only hitting a single reset. > >>> > >>> IP that's in the FPGA and needs reset should have their dedicated resets > >>> via normal reset API bindings imho and not rely on bridges to do the > >>> right thing. > >>> > >>> The ZynqMP is fairly complex and I might be missing something here, > >>> so maybe you (Shubhrajyoti) can elaborate a bit more. The paragraph in > >>> the TRM seemed fairly short, and didn't enlighten me as to why it is > >>> required. > >> > >> The FPGA supports both the PR case and independent execution > >> Like a design can have 2 parts that can have independent execution. > > > > I want to understand you better here. What do you mean by > > 'independent execution'? Like 2 partial reconfiguration regions that > > each have a single reset to reset the hardware in their region? Or > > something else? > Yes. > > > > >> In that case they have to have independent resets. > >> > >> There are 4 PS-PL lines that could be used by designs. > >> So in that case we should have multiple resets. > > > > Normally, as Moritz is saying, the reset would be handled by the > > driver for the fabric-based hardware. > I didnt understand you mean the platform-fpga.c ? I don't understand this sentence ;-) I'll try to re-explain: Say you have an AXI DMA engine in there that needs a reset to be toggled after programming the FPGA then you are in either one of these cases: a) You're doing a full reprogram of the entire fabric, at which point you can reset everything by asserting them in the driver like in drivers/fpga/zynq-fpga.c b) You're doing a partial reconfiguration in which case the regions that are being reconfigured contain some peripherals you want to selectively reset. If you need a software reset, the driver for this peripheral can request a reset through the normal reset API. Am I missing somehting here? Why do you need the bridge to do the reset? - Moritz -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Moritz, <snip> >> > >> > Normally, as Moritz is saying, the reset would be handled by the >> > driver for the fabric-based hardware. >> I didnt understand you mean the platform-fpga.c ? > > I don't understand this sentence ;-) I'll try to re-explain: > > Say you have an AXI DMA engine in there that needs a reset to be toggled > after programming the FPGA then you are in either one of these cases: > > a) You're doing a full reprogram of the entire fabric, at which point > you can reset everything by asserting them in the driver like in > drivers/fpga/zynq-fpga.c That would work however I was thinking the reset technically should be in the region and not the fpga manager as it is more related to the region than the manager. Ofcourse manager could proxy for the region. how about [1] > > b) You're doing a partial reconfiguration in which case the regions that > are being reconfigured contain some peripherals you want to selectively > reset. If you need a software reset, the driver for this peripheral can > request a reset through the normal reset API. So if the ip was written in full bitstream case the fpga manager does the request. In PRR case the driver would do it ? I would have preferred to keep that(full or PRR case) agnostic to the driver. > > Am I missing somehting here? Why do you need the bridge to do the reset? > > - Moritz [1] diff --git a/Documentation/devicetree/bindings/fpga/fpga-region.txt b/Documentation/devicetree/bindings/fpga/fpga-region.txt index 6db8aed..955a863 100644 --- a/Documentation/devicetree/bindings/fpga/fpga-region.txt +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt @@ -196,6 +196,7 @@ 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. +- resets : Phandle and reset specifier for this region In the example below, when an overlay is applied targeting fpga-region0, fpga_mgr is used to program the FPGA. Two bridges are controlled during diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c index 58789b9..8c87a6b 100644 --- a/drivers/fpga/fpga-region.c +++ b/drivers/fpga/fpga-region.c @@ -25,6 +25,7 @@ #include <linux/of_platform.h> #include <linux/slab.h> #include <linux/spinlock.h> +#include <linux/reset.h> /** * struct fpga_region - FPGA Region structure @@ -235,6 +236,8 @@ static int fpga_region_program_fpga(struct fpga_region *region, { struct fpga_manager *mgr; int ret; + struct device *dev = ®ion->dev; + struct reset_control *rstc; region = fpga_region_get(region); if (IS_ERR(region)) { @@ -273,6 +276,15 @@ static int fpga_region_program_fpga(struct fpga_region *region, goto err_put_br; } + rstc = of_reset_control_array_get(dev->of_node, false, true); + if (IS_ERR(rstc)) { + goto err_put_br; + } + + reset_control_reset(rstc); + reset_control_put(rstc); + fpga_bridges_put(®ion->bridge_list); fpga_mgr_put(mgr); fpga_region_put(region); -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 2, 2018 at 1:24 AM, Shubhrajyoti Datta <shubhrajyoti.datta@gmail.com> wrote: > Hi Moritz, > > <snip> >>> > >>> > Normally, as Moritz is saying, the reset would be handled by the >>> > driver for the fabric-based hardware. >>> I didnt understand you mean the platform-fpga.c ? >> >> I don't understand this sentence ;-) I'll try to re-explain: >> >> Say you have an AXI DMA engine in there that needs a reset to be toggled >> after programming the FPGA then you are in either one of these cases: >> >> a) You're doing a full reprogram of the entire fabric, at which point >> you can reset everything by asserting them in the driver like in >> drivers/fpga/zynq-fpga.c > That would work however I was thinking the reset technically should be in > the region and not the fpga manager as it is more related to the region than > the manager. OK now I understand that this is supporting a reset that is resetting all the hardware in a region of fabric. The region could contain multiple devices. This change isn't meant to support toggling a single fpga-based device's reset (that would be handled in the device's driver). > Ofcourse manager could proxy for the region. Mapping the reset to the region is the more general case. It will support resets that either apply to a single PR region or resetting a whole fabric of a FPGA that is doing full reconfig, i > > how about [1] [1] makes sense to me now. That is a better solution than making this be a bridge. > >> >> b) You're doing a partial reconfiguration in which case the regions that >> are being reconfigured contain some peripherals you want to selectively >> reset. If you need a software reset, the driver for this peripheral can >> request a reset through the normal reset API. > > So if the ip was written in full bitstream case the fpga manager does > the request. > In PRR case the driver would do it ? > > I would have preferred to keep that(full or PRR case) agnostic to the driver. >> >> Am I missing somehting here? Why do you need the bridge to do the reset? >> >> - Moritz > [1] > diff --git a/Documentation/devicetree/bindings/fpga/fpga-region.txt > b/Documentation/devicetree/bindings/fpga/fpga-region.txt > index 6db8aed..955a863 100644 > --- a/Documentation/devicetree/bindings/fpga/fpga-region.txt > +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt > @@ -196,6 +196,7 @@ 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. > +- resets : Phandle and reset specifier for this region > > In the example below, when an overlay is applied targeting fpga-region0, > fpga_mgr is used to program the FPGA. Two bridges are controlled during > diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c > index 58789b9..8c87a6b 100644 > --- a/drivers/fpga/fpga-region.c > +++ b/drivers/fpga/fpga-region.c > @@ -25,6 +25,7 @@ > #include <linux/of_platform.h> > #include <linux/slab.h> > #include <linux/spinlock.h> > +#include <linux/reset.h> > > /** > * struct fpga_region - FPGA Region structure > @@ -235,6 +236,8 @@ static int fpga_region_program_fpga(struct > fpga_region *region, > { > struct fpga_manager *mgr; > int ret; > + struct device *dev = ®ion->dev; > + struct reset_control *rstc; > > region = fpga_region_get(region); > if (IS_ERR(region)) { > @@ -273,6 +276,15 @@ static int fpga_region_program_fpga(struct > fpga_region *region, > goto err_put_br; > } > > + rstc = of_reset_control_array_get(dev->of_node, false, true); > + if (IS_ERR(rstc)) { > + goto err_put_br; > + } > + > + reset_control_reset(rstc); > + reset_control_put(rstc); > + This looks good to me. This allows there to be a reset that's not dedicated to a specific device, but really is resetting a whole PR region. Or resetting the whole FPGA, it handles both cases. And we get that pretty pain-free, just add the reset to the fpga region in the device tree, if this device tree is being used, for example. Alan > fpga_bridges_put(®ion->bridge_list); > fpga_mgr_put(mgr); > fpga_region_put(region); -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Shubhrajyoti, Alan, On Fri, Mar 02, 2018 at 08:43:19PM -0600, Alan Tull wrote: > On Fri, Mar 2, 2018 at 1:24 AM, Shubhrajyoti Datta > <shubhrajyoti.datta@gmail.com> wrote: > > Hi Moritz, > > > > <snip> > >>> > > >>> > Normally, as Moritz is saying, the reset would be handled by the > >>> > driver for the fabric-based hardware. > >>> I didnt understand you mean the platform-fpga.c ? > >> > >> I don't understand this sentence ;-) I'll try to re-explain: > >> > >> Say you have an AXI DMA engine in there that needs a reset to be toggled > >> after programming the FPGA then you are in either one of these cases: > >> > >> a) You're doing a full reprogram of the entire fabric, at which point > >> you can reset everything by asserting them in the driver like in > >> drivers/fpga/zynq-fpga.c > > That would work however I was thinking the reset technically should be in > > the region and not the fpga manager as it is more related to the region than > > the manager. > > OK now I understand that this is supporting a reset that is resetting > all the hardware in a region of fabric. The region could contain > multiple devices. This change isn't meant to support toggling a > single fpga-based device's reset (that would be handled in the > device's driver). I could see how that could happen in a design, yes. > > > Ofcourse manager could proxy for the region. > > Mapping the reset to the region is the more general case. It will > support resets that either apply to a single PR region or resetting a > whole fabric of a FPGA that is doing full reconfig, i > > > > > how about [1] > > [1] makes sense to me now. That is a better solution than making this > be a bridge. Yeah I think this is the direction to go for what he's trying to achieve. > > > > >> > >> b) You're doing a partial reconfiguration in which case the regions that > >> are being reconfigured contain some peripherals you want to selectively > >> reset. If you need a software reset, the driver for this peripheral can > >> request a reset through the normal reset API. > > > > So if the ip was written in full bitstream case the fpga manager does > > the request. > > > In PRR case the driver would do it ? > > > > I would have preferred to keep that(full or PRR case) agnostic to the driver. Yeah, the device driver should not have to worry about whether it's in partial reconfiguration or full reconfiguration. What I do feel strongly about is the region reset shouldn't remove the reset handling from the drivers if they do need it. > >> > >> Am I missing somehting here? Why do you need the bridge to do the reset? > >> > >> - Moritz > > [1] > > diff --git a/Documentation/devicetree/bindings/fpga/fpga-region.txt > > b/Documentation/devicetree/bindings/fpga/fpga-region.txt > > index 6db8aed..955a863 100644 > > --- a/Documentation/devicetree/bindings/fpga/fpga-region.txt > > +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt > > @@ -196,6 +196,7 @@ 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. > > +- resets : Phandle and reset specifier for this region > > > > In the example below, when an overlay is applied targeting fpga-region0, > > fpga_mgr is used to program the FPGA. Two bridges are controlled during > > diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c > > index 58789b9..8c87a6b 100644 > > --- a/drivers/fpga/fpga-region.c > > +++ b/drivers/fpga/fpga-region.c > > @@ -25,6 +25,7 @@ > > #include <linux/of_platform.h> > > #include <linux/slab.h> > > #include <linux/spinlock.h> > > +#include <linux/reset.h> > > > > /** > > * struct fpga_region - FPGA Region structure > > @@ -235,6 +236,8 @@ static int fpga_region_program_fpga(struct > > fpga_region *region, > > { > > struct fpga_manager *mgr; > > int ret; > > + struct device *dev = ®ion->dev; > > + struct reset_control *rstc; > > > > region = fpga_region_get(region); > > if (IS_ERR(region)) { > > @@ -273,6 +276,15 @@ static int fpga_region_program_fpga(struct > > fpga_region *region, > > goto err_put_br; > > } > > > > + rstc = of_reset_control_array_get(dev->of_node, false, true); > > + if (IS_ERR(rstc)) { > > + goto err_put_br; > > + } > > + > > + reset_control_reset(rstc); > > + reset_control_put(rstc); > > + > > This looks good to me. This allows there to be a reset that's not > dedicated to a specific device, but really is resetting a whole PR > region. Or resetting the whole FPGA, it handles both cases. And we > get that pretty pain-free, just add the reset to the fpga region in > the device tree, if this device tree is being used, for example. One thing to watch out for is to make sure to not re-introduce an DT dependency into the fpga-region code. Thanks, Moritz -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 2, 2018 at 9:23 PM, Moritz Fischer <mdf@kernel.org> wrote: > Hi Shubhrajyoti, Alan, > > On Fri, Mar 02, 2018 at 08:43:19PM -0600, Alan Tull wrote: >> On Fri, Mar 2, 2018 at 1:24 AM, Shubhrajyoti Datta >> <shubhrajyoti.datta@gmail.com> wrote: >> > Hi Moritz, >> > >> > <snip> >> >>> > >> >>> > Normally, as Moritz is saying, the reset would be handled by the >> >>> > driver for the fabric-based hardware. >> >>> I didnt understand you mean the platform-fpga.c ? >> >> >> >> I don't understand this sentence ;-) I'll try to re-explain: >> >> >> >> Say you have an AXI DMA engine in there that needs a reset to be toggled >> >> after programming the FPGA then you are in either one of these cases: >> >> >> >> a) You're doing a full reprogram of the entire fabric, at which point >> >> you can reset everything by asserting them in the driver like in >> >> drivers/fpga/zynq-fpga.c >> > That would work however I was thinking the reset technically should be in >> > the region and not the fpga manager as it is more related to the region than >> > the manager. >> >> OK now I understand that this is supporting a reset that is resetting >> all the hardware in a region of fabric. The region could contain >> multiple devices. This change isn't meant to support toggling a >> single fpga-based device's reset (that would be handled in the >> device's driver). > > I could see how that could happen in a design, yes. > >> >> > Ofcourse manager could proxy for the region. >> >> Mapping the reset to the region is the more general case. It will >> support resets that either apply to a single PR region or resetting a >> whole fabric of a FPGA that is doing full reconfig, i >> >> > >> > how about [1] >> >> [1] makes sense to me now. That is a better solution than making this >> be a bridge. > > Yeah I think this is the direction to go for what he's trying to achieve. >> >> > >> >> >> >> b) You're doing a partial reconfiguration in which case the regions that >> >> are being reconfigured contain some peripherals you want to selectively >> >> reset. If you need a software reset, the driver for this peripheral can >> >> request a reset through the normal reset API. >> > >> > So if the ip was written in full bitstream case the fpga manager does >> > the request. >> >> > In PRR case the driver would do it ? >> > >> > I would have preferred to keep that(full or PRR case) agnostic to the driver. > > Yeah, the device driver should not have to worry about whether it's in partial > reconfiguration or full reconfiguration. What I do feel strongly about > is the region reset shouldn't remove the reset handling from the drivers > if they do need it. Yes, I agree totally. > >> >> >> >> Am I missing somehting here? Why do you need the bridge to do the reset? >> >> >> >> - Moritz >> > [1] >> > diff --git a/Documentation/devicetree/bindings/fpga/fpga-region.txt >> > b/Documentation/devicetree/bindings/fpga/fpga-region.txt >> > index 6db8aed..955a863 100644 >> > --- a/Documentation/devicetree/bindings/fpga/fpga-region.txt >> > +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt >> > @@ -196,6 +196,7 @@ 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. >> > +- resets : Phandle and reset specifier for this region >> > >> > In the example below, when an overlay is applied targeting fpga-region0, >> > fpga_mgr is used to program the FPGA. Two bridges are controlled during >> > diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c >> > index 58789b9..8c87a6b 100644 >> > --- a/drivers/fpga/fpga-region.c >> > +++ b/drivers/fpga/fpga-region.c >> > @@ -25,6 +25,7 @@ >> > #include <linux/of_platform.h> >> > #include <linux/slab.h> >> > #include <linux/spinlock.h> >> > +#include <linux/reset.h> >> > >> > /** >> > * struct fpga_region - FPGA Region structure >> > @@ -235,6 +236,8 @@ static int fpga_region_program_fpga(struct >> > fpga_region *region, >> > { >> > struct fpga_manager *mgr; >> > int ret; >> > + struct device *dev = ®ion->dev; >> > + struct reset_control *rstc; >> > >> > region = fpga_region_get(region); >> > if (IS_ERR(region)) { >> > @@ -273,6 +276,15 @@ static int fpga_region_program_fpga(struct >> > fpga_region *region, >> > goto err_put_br; >> > } >> > >> > + rstc = of_reset_control_array_get(dev->of_node, false, true); >> > + if (IS_ERR(rstc)) { >> > + goto err_put_br; >> > + } >> > + >> > + reset_control_reset(rstc); >> > + reset_control_put(rstc); >> > + >> >> This looks good to me. This allows there to be a reset that's not >> dedicated to a specific device, but really is resetting a whole PR >> region. Or resetting the whole FPGA, it handles both cases. And we >> get that pretty pain-free, just add the reset to the fpga region in >> the device tree, if this device tree is being used, for example. > > One thing to watch out for is to make sure to not re-introduce an DT > dependency into the fpga-region code. Good catch Moritz! It looks like this example is based on v4.15? Shubhrajyoti, please look at the linux-next repo [2] as I've separated the DT parts of fpga region to a separate file of-fpga-region.c. Alan [2] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/fpga > > Thanks, > > Moritz -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 2, 2018 at 9:54 PM, Alan Tull <atull@kernel.org> wrote: > On Fri, Mar 2, 2018 at 9:23 PM, Moritz Fischer <mdf@kernel.org> wrote: >> Hi Shubhrajyoti, Alan, >> >> On Fri, Mar 02, 2018 at 08:43:19PM -0600, Alan Tull wrote: >>> On Fri, Mar 2, 2018 at 1:24 AM, Shubhrajyoti Datta >>> <shubhrajyoti.datta@gmail.com> wrote: >>> > Hi Moritz, >>> > >>> > <snip> >>> >>> > >>> >>> > Normally, as Moritz is saying, the reset would be handled by the >>> >>> > driver for the fabric-based hardware. >>> >>> I didnt understand you mean the platform-fpga.c ? >>> >> >>> >> I don't understand this sentence ;-) I'll try to re-explain: >>> >> >>> >> Say you have an AXI DMA engine in there that needs a reset to be toggled >>> >> after programming the FPGA then you are in either one of these cases: >>> >> >>> >> a) You're doing a full reprogram of the entire fabric, at which point >>> >> you can reset everything by asserting them in the driver like in >>> >> drivers/fpga/zynq-fpga.c >>> > That would work however I was thinking the reset technically should be in >>> > the region and not the fpga manager as it is more related to the region than >>> > the manager. >>> >>> OK now I understand that this is supporting a reset that is resetting >>> all the hardware in a region of fabric. The region could contain >>> multiple devices. This change isn't meant to support toggling a >>> single fpga-based device's reset (that would be handled in the >>> device's driver). >> >> I could see how that could happen in a design, yes. >> >>> >>> > Ofcourse manager could proxy for the region. >>> >>> Mapping the reset to the region is the more general case. It will >>> support resets that either apply to a single PR region or resetting a >>> whole fabric of a FPGA that is doing full reconfig, i >>> >>> > >>> > how about [1] >>> >>> [1] makes sense to me now. That is a better solution than making this >>> be a bridge. >> >> Yeah I think this is the direction to go for what he's trying to achieve. >>> >>> > >>> >> >>> >> b) You're doing a partial reconfiguration in which case the regions that >>> >> are being reconfigured contain some peripherals you want to selectively >>> >> reset. If you need a software reset, the driver for this peripheral can >>> >> request a reset through the normal reset API. >>> > >>> > So if the ip was written in full bitstream case the fpga manager does >>> > the request. >>> >>> > In PRR case the driver would do it ? >>> > >>> > I would have preferred to keep that(full or PRR case) agnostic to the driver. >> >> Yeah, the device driver should not have to worry about whether it's in partial >> reconfiguration or full reconfiguration. What I do feel strongly about >> is the region reset shouldn't remove the reset handling from the drivers >> if they do need it. > > Yes, I agree totally. > >> >>> >> >>> >> Am I missing somehting here? Why do you need the bridge to do the reset? >>> >> >>> >> - Moritz >>> > [1] >>> > diff --git a/Documentation/devicetree/bindings/fpga/fpga-region.txt >>> > b/Documentation/devicetree/bindings/fpga/fpga-region.txt >>> > index 6db8aed..955a863 100644 >>> > --- a/Documentation/devicetree/bindings/fpga/fpga-region.txt >>> > +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt >>> > @@ -196,6 +196,7 @@ 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. >>> > +- resets : Phandle and reset specifier for this region >>> > >>> > In the example below, when an overlay is applied targeting fpga-region0, >>> > fpga_mgr is used to program the FPGA. Two bridges are controlled during >>> > diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c >>> > index 58789b9..8c87a6b 100644 >>> > --- a/drivers/fpga/fpga-region.c >>> > +++ b/drivers/fpga/fpga-region.c >>> > @@ -25,6 +25,7 @@ >>> > #include <linux/of_platform.h> >>> > #include <linux/slab.h> >>> > #include <linux/spinlock.h> >>> > +#include <linux/reset.h> >>> > >>> > /** >>> > * struct fpga_region - FPGA Region structure >>> > @@ -235,6 +236,8 @@ static int fpga_region_program_fpga(struct >>> > fpga_region *region, >>> > { >>> > struct fpga_manager *mgr; >>> > int ret; >>> > + struct device *dev = ®ion->dev; >>> > + struct reset_control *rstc; >>> > >>> > region = fpga_region_get(region); >>> > if (IS_ERR(region)) { >>> > @@ -273,6 +276,15 @@ static int fpga_region_program_fpga(struct >>> > fpga_region *region, >>> > goto err_put_br; >>> > } >>> > >>> > + rstc = of_reset_control_array_get(dev->of_node, false, true); >>> > + if (IS_ERR(rstc)) { >>> > + goto err_put_br; >>> > + } >>> > + >>> > + reset_control_reset(rstc); >>> > + reset_control_put(rstc); >>> > + >>> >>> This looks good to me. This allows there to be a reset that's not >>> dedicated to a specific device, but really is resetting a whole PR >>> region. Or resetting the whole FPGA, it handles both cases. And we >>> get that pretty pain-free, just add the reset to the fpga region in >>> the device tree, if this device tree is being used, for example. >> >> One thing to watch out for is to make sure to not re-introduce an DT >> dependency into the fpga-region code. > > Good catch Moritz! It looks like this example is based on v4.15? > Shubhrajyoti, please look at the linux-next repo [2] as I've separated > the DT parts of fpga region to a separate file of-fpga-region.c. The rstc can be added to struct fpga_region in fpga-region.h. of_reset_control_array_get can be in of-fpga-region.c. Toggling the reset can still happen in fpga_region_program_fpga. Resetting the contents of a FPGA region is something that will be useful in general (currently there are now two users with patches on this mailing list wanting to do region reset). The cool thing is that reset support for non-DT platforms has recently been added [3]. Alan > > Alan > > [2] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/fpga [3] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/reset/core.c?id=940821732839622c22db89b57d3969e76c863e44 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/fpga/xlnx,rst-bridge.txt b/Documentation/devicetree/bindings/fpga/xlnx,rst-bridge.txt new file mode 100644 index 0000000..6f1bfc2 --- /dev/null +++ b/Documentation/devicetree/bindings/fpga/xlnx,rst-bridge.txt @@ -0,0 +1,22 @@ +Xilinx fpga reset bridge + +The Xilinx reset bridge toggles the reset line to the PL +in Zynqmp Ultrascale plus. + + +Required properties: +- compatible : Should contain "xlnx,rst-bridge" +- reset : reset phandles + +Optional properties: +- bridge-enable : 0 if driver should disable bridge at startup + 1 if driver should enable bridge at startup + Default is to leave bridge in current state. + +See Documentation/devicetree/bindings/fpga/fpga-region.txt for generic bindings. + +Example: +fpga_rst_bridge: fpga_rst_bridge { + compatible = "xlnx,rst-bridge"; + resets = <&rst 115>; +};