Re: [PATCH v15 5/6] fpga: fpga-area and fpga-bus: device tree control for FPGA

[atull]

On Mon, 25 Jan 2016, Rob Herring wrote:

> On Fri, Jan 22, 2016 at 6:07 PM, Moritz Fischer
> <moritz.fischer@xxxxxxxxx> wrote:
> > On Fri, Jan 22, 2016 at 5:37 PM, atull <atull@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >> On Fri, 22 Jan 2016, Moritz Fischer wrote:
> >>
> >>> Alan,
> >>>
> >>> On Wed, Jan 20, 2016 at 8:24 PM,  <atull@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >>>
> >>> > +static int fpga_area_probe(struct platform_device *pdev)
> >>> > +{
> >>> > +       struct device *dev = &pdev->dev;
> >>> > +       struct device_node *np = dev->of_node;
> >>> > +       struct fpga_area *area;
> >>> > +       int ret;
> >>> > +
> >>> > +       area = devm_kzalloc(dev, sizeof(*area), GFP_KERNEL);
> >>> > +       if (!area)
> >>> > +               return -ENOMEM;
> >>> > +
> >>> > +       INIT_LIST_HEAD(&area->bridge_list);
> >>> > +
> >>> > +       ret = fpga_bridge_register(dev, "FPGA Area", NULL, area);
> >>> > +       if (ret)
> >>> > +               return ret;
> >>> > +       area->br = dev_get_drvdata(dev);
> >>> > +
> >>> > +       if (of_property_read_string(np, "firmware-name",
> >>> > +                                   &area->firmware_name)) {
> >>> > +               of_platform_populate(np, of_default_bus_match_table, NULL, dev);
> >>> > +               return 0;
> >>> > +       }
> >>>
> >>> This is the use case where the bootloader loaded the fpga, and you
> >>> just want to populate
> >>> the devices in the fabric, right?
> >>
> >> Hi Moritz,
> >>
> >> Yes
> 
> That is very strange logic. It should be fine to just call
> of_platform_populate unconditionally. If there are no children of np,
> then it will be a nop.

That's what it is doing.  It's coded this way to reduce indentation.  If there
is no FPGA image to load, call of_platform_populate() and return.  Otherwise do
a bunch of steps to load the FPGA image and handle the bridges, then call
of_platform_populate() and return.  If there's no children, no problem.

> 
> >>> > +       if (of_property_read_bool(np, "partial-reconfig"))
> >>> > +               area->flags |= FPGA_MGR_PARTIAL_RECONFIG;
> >>> > +
> >>> > +       ret = fpga_area_get_bus(area);
> >>> > +       if (ret) {
> >>> > +               dev_dbg(dev, "Should be child of a FPGA Bus");
> >>> > +               goto err_unreg;
> >>> > +       }
> >>>
> >>> Looking at socfpga.dtsi, would that mean that the fpgamgr0 node would
> >>> need to become a subnode of fpgabus@0 at the same place?
> >>>
> >>> i.e. /soc/fpgamgr@ff706000 -> /soc/fpgabus@0/fpgamgr@ff706000
> >>>
> >>> and the ranges property would be used to translate to the fpga memory
> >>> mapped space?
> >>>
> >>> I know we're going back and forth on this. I think Rob brought up a
> >>> similar question:
> >>> "Does the bus really go thru the fpgamgr and then the bridge as this
> >>> implies? Or fpgamgr is a sideband controller?"
> >>>
> >>> To which I think the answer is 'sideband' controller, yet with the new
> >>> bindings it looks like
> >>> the bus goes through the fpgamgr.
> >>
> >> Yeah, let's get this right.  First, let's be clear on the reason for FPGA Bus to
> >> exist.  There may be >1 FPGA in a system.  I want the FPGA Bus bring together
> >> the bridges and manager that are associated with a certain FPGA.  This allows
> >> the system designer to specify which FPGA is getting programmed with which
> >> image/hardware.  So at minimum, we need some way of associating a FPGA Bus with
> >> a FPGA Manager.
> >
> > I see your argument for the FPGA bus. I agree that we need to
> > distinguish different FPGAs,
> > and we need a way to associate an area with a manager (and potentially bridges).
> >
> >> As far as the target path is concerned, in the case of no bridges, we could have
> >> the overlay target the FPGA Bus instead of the FPGA Manager.  That may be more
> >> logical.  This would just be a documentation change; I think fpga-area.c will
> >> work OK if you specify the FPGA bus as your target (the manager still has to
> >> be a child of the bus so the bus knows what manager to use).
> >
> > Could the bus not just use a phandle to the manager? Or the area a
> > phandle to the bus?
> 
> That may be better as it would avoid the virtual fpga-bus which is a
> bit questionable. I'm okay with allowing it, but will happily take any
> examples where it doesn't work. However, I'm not sure this case is
> one.

I have no problem with specifying FPGA manager with a phandle, seems
like it will be better in some cases.

I'm proposing FPGA Bus to specify all the things (manager and bridges) that are
needed to do a reprogramming cycle on a specific FPGA. The FPGA Bus may seem
less necessary in Moritz' case where there are no FPGA Bridges in the DT.

I'll discuss this more on the other thread.

> 
> > Like that one could have potentially disjunct groups. Say I have a SPI
> > device that is in an FPGA area.
> > With our current system, I'd have a FPGA Manager that needs to be a
> > child of the bus as child of the SPI controller
> > with the memory addresses being addresses on the SOC's memory bus:
> >
> > spi_ctrl@deadbeef {
> >         fpga_bus@0 {
> >                 fpgamgr@f8007000 {
> >                     mgr regs etc...
> >                 ... and now the SPI slaves ...
> >                     slave@42 {
> >                     };
> >                 };
> >         };
> > };
> 
> I think Alan's assumption is there's always a memory mapped control
> interface and the DT hierarchy follows that. While I see you could
> have other interfaces like SPI, that would not be the control
> interface related to FPGA programming AIUI. You could also have GPIOs,
> I2C, or any other bus/interface we describe in DT in theory.
> 
> How to deal with additional connections like this is similar to
> overlays for daughterboards (e.g. capes, hats, shields) where the need
> for mapping slave devices on a connector to host controllers/buses
> which can vary. In that case, you don't want to have to create a
> different overlay for every host board just to change the parent
> devices of each child. If there is a SPI connection to FPGA (or area),
> then we need a way to map the host SPI bus to the FPGA. In this
> example, the overlay should only say "slave@42" is connected to the
> spi bus of the FPGA area and it would not have direct reference to
> "spi_ctrl@deadbeef." There's some ideas around how to support these
> usecases probably involving a connector node with mappings of
> connector i/o's to host i/o's (paging Pantelis, where's your
> proposal?). I think the same would work here, so I would suggest we
> not try to address it immediately other than decide the requirements
> here are similar enough to what is needed for connectors.
> 
> Rob
>