Re: [PATCH v12 2/6] fpga: add bindings document for simple fpga bus

[atull]

On Fri, 30 Oct 2015, Rob Herring wrote:

> On Thu, Oct 29, 2015 at 11:02 AM, atull <atull@xxxxxxxxxxxxxxxxxxxxx> wrote:
> > On Wed, 28 Oct 2015, Rob Herring wrote:
> >
> >> On Tue, Oct 27, 2015 at 5:09 PM,  <atull@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >> > From: Alan Tull <atull@xxxxxxxxxxxxxxxxxxxxx>
> >> >
> >> > New bindings document for simple fpga bus.
> >> >
> >> > Signed-off-by: Alan Tull <atull@xxxxxxxxxxxxxxxxxxxxx>
> >> > ---
> >> > v9:  initial version added to this patchset
> >> > v10: s/fpga/FPGA/g
> >> >      replace DT overlay example with slightly more complicated example
> >> >      move to staging/simple-fpga-bus
> >> > v11: No change in this patch for v11 of the patch set
> >> > v12: Moved out of staging.
> >> >      Changed to use FPGA bridges framework instead of resets
> >> >      for bridges.
> >> > ---
> >> >  .../devicetree/bindings/fpga/simple-fpga-bus.txt   |   81 ++++++++++++++++++++
> >> >  1 file changed, 81 insertions(+)
> >> >  create mode 100644 Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt b/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
> >> > new file mode 100644
> >> > index 0000000..2e742f7
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/fpga/simple-fpga-bus.txt
> >> > @@ -0,0 +1,81 @@
> >> > +Simple FPGA Bus
> >> > +===============
> >> > +
> >> > +A Simple FPGA Bus is a bus that handles configuring an FPGA and its bridges
> >> > +before populating the devices below its node.  All this happens when a device
> >> > +tree overlay is added to the live tree.  This document describes that device
> >> > +tree overlay.
> >> > +
> >> > +Required properties:
> >> > +- compatible : should contain "simple-fpga-bus"
> >> > +- #address-cells, #size-cells, ranges: must be present to handle address space
> >> > +  mapping for children.
> >> > +
> >> > +Optional properties:
> >> > +- fpga-mgr : should contain a phandle to a FPGA manager.
> >> > +- fpga-firmware : should contain the name of a FPGA image file located on the
> >> > +  firmware search path.
> >>
> >> Putting firmware filename in DT has come up in other cases recently[1]
> >> and we concluded it should not be in the DT. Maybe the conclusion
> >> would be different here, and if so we should have a common property
> >> here.
> >
> > Interesting discussion.
> >
> > One FPGA image will almost always have multiple hardware devices in it.
> > The device blocks will be reused in various combinations in different
> > FPGA images.  So hardwiring the image name in some specific driver won't
> > be possible.  Also many of the devices that could appear on a FPGA also
> > could appear in normal hardware.  Same driver for both cases, just
> > different address.
> >
> > I won't have control of the name of the firmware file.  It will be something
> > that makes sense to a FPGA hardware guy so translating the node name to an
> > image name would be brittle.
> >
> > Renaming this as a generic property "firmware-name" or "firmware" would be
> > an improvement on what I proposed.
> >
> > If it is acceptible to have this in the DT, it means FPGA hardware development
> > workflow does not require a kernel rebuild.  The DT can collect together the
> > image name, the bridges to the FPGA (or to that area of the FPGA), and a list
> > of the devices/drivers in that image.
> 
> You can deal with this purely in userspace.

We have.  It's ugly.  That means we have to also deal with bridges from
userspace since they have to be disable during FPGA programming.  And the
drivers have to be modules that we modprobe after FPGA programming.

> The firmware userspace
> helper can load any file you like. If the name is frequently changing,
> then I agree it should not be in the kernel. But neither should it be
> in the base DT. However, this would be in the overlay DT? In that use,
> I think having the firmware name in DT is fine. The other option is
> just put the firmware itself into the DT overlay (u-boot FIT images
> are actually DTs with binary blobs). Either way please just create and
> use a generic binding here.

Planning to use overlays.  I'll use "firmware-name."

> 
> >> > +- partial-reconfig : boolean property should be defined if partial
> >> > +  reconfiguration of the FPGA is to be done, otherwise full reconfiguration
> >> > +  is done.
> >> > +- fpga-bridges : should contain a list of bridges that the bus will disable
> >> > +  before   programming the FPGA and then enable after the FPGA has been
> >> > +
> >> > +Example:
> >> > +
> >> > +/dts-v1/;
> >> > +/plugin/;
> >> > +/ {
> >> > +       fragment@0 {
> >> > +               target-path="/soc";
> >> > +               __overlay__ {
> >> > +                       #address-cells = <1>;
> >> > +                       #size-cells = <1>;
> >> > +
> >> > +                       bridge@0xff200000 {
> >> > +                               compatible = "simple-fpga-bus";
> >> > +                               reg = <0xc0000000 0x20000000>,
> >> > +                                     <0xff200000 0x00200000>;
> >>
> >> You have registers for the bus, so therefore it is not simple. I think
> >> the bus or bridge needs a specific compatible
> >>
> >
> > The reg here is cruft from device tree generation.  I don't use it and will
> > clean it out.  After I've done that, does that become simple again?
> >
> > What I need in a bus is:
> >  - Handles 'ranges'
> >  - Controls enabling/disabling bridges and programs the FPGA
> 
> Where are these controls?

The Simple FPGA Bus calls FPGA Bridge Framework functions to
enable/disable the bridges.

> 
> >  - Populates the child devices (and there will probably be many)
> 
> simple-bus handles at least the 1st and 3rd item. I suppose you don't
> want the bus to probe before the bridge driver. If you want the bridge
> driver to control that, you don't actually need a bus name.
> of_platform_populate() will create all child devices. It is only if
> you want to create the grandchildren that you need a bus match on the
> child nodes.
> 
> 
> > This raises another issue: each area of the FPGA is likely to have multiple
> > bridges. That's why I had a list of phandles to bridges rather than
> > different bus for each type of bridge.  Is that acceptible-ish?
> 
> Hard to say. A bridge tends to mean a parent bus to child bus
> translator and the DT hierarchy generally follows the bus hierarchy,
> so really it is better if DT follows the physical bus structure. Of
> course, we really only do that with outbound bus and not the inbound
> side.
> 
> I also worry that all this looks like it may be somewhat Altera specific.

I don't think it is Altera specific.  If you want to reprogram an
FPGA with devices that need drivers, you will likely need to:
 1. Disable some bridges to prevent spurious stuff on the bus
 2. Load the FPGA
 3. Enable the bridges
 4. Probe some drivers

In the case where the whole FPGA gets rewritten, it is possible to
combine the bridge control with the FPGA manager (then it *is* manufacturer
specific).  In the case where part of the FPGA gets rewritten (partial
reconfiguration), it is likely that some sort of bridge will exist on
the FPGA to protect the bus.  And there will be that kind of bridge for
each different area of the FPGA that needs to get rewritten.

The device tree overlay seems a very natural way of keeping all that
information together in one place.  Otherwise we have to invent
something in userspace.

Alan

> 
> Rob
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/