Re: [PATCH 2/2] fpga: reset bridge: Add the reset bridge

[Alan Tull <atull@kernel.org>]

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 = &region->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(&region->bridge_list);
>         fpga_mgr_put(mgr);
>         fpga_region_put(region);