All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [RFC] Add a parser in fpga_region to decode the tlv meta data suggested by Sundar
       [not found]                 ` <CANk1AXRMGTrk5JUU_tKuZFVBiMbn4-1kQ0SajMYuPFYnjvMWVA@mail.gmail.com>
@ 2017-03-22 21:17                   ` Moritz Fischer
  2017-03-23 15:30                     ` Alan Tull
  0 siblings, 1 reply; 5+ messages in thread
From: Moritz Fischer @ 2017-03-22 21:17 UTC (permalink / raw)
  To: Alan Tull
  Cc: Moritz Fischer, Li, Yi, Nadathur, Sundar, linux-fpga, Jason Gunthorpe

[-- Attachment #1: Type: text/plain, Size: 1662 bytes --]

On Wed, Mar 22, 2017 at 03:13:39PM -0500, Alan Tull wrote:
> On Wed, Mar 22, 2017 at 1:33 AM, Moritz Fischer
> <moritz.fischer@ettus.com> wrote:

> > It doesn't have to be exactly FIT, but it serves a very similar purpose,
> > so I'll definitely spend some more time on studying how that works.
> > What you describe above seems to 'fit' (pun intended) very well with
> > what the U-Boot guys do with FIT already (multiple images, configurations
> > etc) there's definitely code there that we can reuse.
> 
> Bringing all of FIT to the kernel would be a large project (and
> wouldn't belong in drivers/fpga).  But as your  experimentation
> demonstrated, parsing our own thing with existing support in the
> kernel or libfdt would be pretty easy.

Possibly, yeah.

> > The one thing that would be interesting for our use case might be the
> > non-contiguous header case which I assume 1. and 2. are meant to deal with?
> 
> Yes non-contiguous cases and streaming cases.
> 
> 1. is the file magic (4 bytes) 2. is header size (4 bytes).  The
> parser code will check the magic, get the header size, then gather
> enough additional non-contiguous/streaming header into a buffer to
> parse it.  Having parsed it, it will have offsets to the bitfiles and
> other header info.

I think again fdt got us covered ([1]) there :-) There's a magic and a
totalsize member:

<snip>
struct fdt_header {
        fdt32_t magic;                   /* magic word FDT_MAGIC */
        fdt32_t totalsize;               /* total size of DT block
</snip>

Cheers,

Moritz

[1] http://lxr.free-electrons.com/source/scripts/dtc/libfdt/fdt.h#L57

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC] Add a parser in fpga_region to decode the tlv meta data suggested by Sundar
  2017-03-22 21:17                   ` [RFC] Add a parser in fpga_region to decode the tlv meta data suggested by Sundar Moritz Fischer
@ 2017-03-23 15:30                     ` Alan Tull
  0 siblings, 0 replies; 5+ messages in thread
From: Alan Tull @ 2017-03-23 15:30 UTC (permalink / raw)
  To: Moritz Fischer
  Cc: Moritz Fischer, Li, Yi, Nadathur, Sundar, linux-fpga, Jason Gunthorpe

On Wed, Mar 22, 2017 at 4:17 PM, Moritz Fischer <mdf@kernel.org> wrote:
> On Wed, Mar 22, 2017 at 03:13:39PM -0500, Alan Tull wrote:
>> On Wed, Mar 22, 2017 at 1:33 AM, Moritz Fischer
>> <moritz.fischer@ettus.com> wrote:
>
>> > It doesn't have to be exactly FIT, but it serves a very similar purpose,
>> > so I'll definitely spend some more time on studying how that works.
>> > What you describe above seems to 'fit' (pun intended) very well with
>> > what the U-Boot guys do with FIT already (multiple images, configurations
>> > etc) there's definitely code there that we can reuse.
>>
>> Bringing all of FIT to the kernel would be a large project (and
>> wouldn't belong in drivers/fpga).  But as your  experimentation
>> demonstrated, parsing our own thing with existing support in the
>> kernel or libfdt would be pretty easy.
>
> Possibly, yeah.
>
>> > The one thing that would be interesting for our use case might be the
>> > non-contiguous header case which I assume 1. and 2. are meant to deal with?
>>
>> Yes non-contiguous cases and streaming cases.
>>
>> 1. is the file magic (4 bytes) 2. is header size (4 bytes).  The
>> parser code will check the magic, get the header size, then gather
>> enough additional non-contiguous/streaming header into a buffer to
>> parse it.  Having parsed it, it will have offsets to the bitfiles and
>> other header info.
>
> I think again fdt got us covered ([1]) there :-) There's a magic and a
> totalsize member:
>
> <snip>
> struct fdt_header {
>         fdt32_t magic;                   /* magic word FDT_MAGIC */
>         fdt32_t totalsize;               /* total size of DT block
> </snip>

0xd00dfeed !

Alan

>
> Cheers,
>
> Moritz
>
> [1] http://lxr.free-electrons.com/source/scripts/dtc/libfdt/fdt.h#L57

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFC] Add a parser in fpga_region to decode the tlv meta data suggested by Sundar
  2017-02-24 21:41               ` Nadathur, Sundar
@ 2017-02-24 22:00                 ` Jason Gunthorpe
  0 siblings, 0 replies; 5+ messages in thread
From: Jason Gunthorpe @ 2017-02-24 22:00 UTC (permalink / raw)
  To: Nadathur, Sundar
  Cc: Li, Yi, Moritz Fischer, Alan Tull, linux-fpga, linux-kernel

On Fri, Feb 24, 2017 at 09:41:12PM +0000, Nadathur, Sundar wrote:

> I have talked about the need for structs and arrays, potentially
> nested, without really explaining why we may need them
> eventually. I'll get to that in a moment. But, can we agree that, if
> nested structs, arrays and general extensibility is needed, FDT or
> TLVs would be the way to go? If we agree, it becomes a FDT vs. TLV
> discussion.
> 
> Here's why I think we may need extensibility. (Pardon me for
> starting from the basics -- just trying to set the background.) A
> FPGA can be programmed as a whole, or can have Partial
> Reconfiguration (PR) regions, which can be programmed
> independently. The image programmed into a PR region may have
> components and sub-units in general. These components may have their
> own properties. For example, if the FPGA is exposed as a PCIe device
> to the host, the components may have their own registers in MMIO,
> DMA attributes and/or interrupts. They may also have identifiers
> describing their function. In general, we may want these attributes
> and properties, on a per-component basis, in the metadata. Even if
> we don't need them tomorrow, as data center and IOT needs grow, we
> are likely to need that going forward.

So, you are imagining that the FPGA header would include more than
just information relative to programming it, but also something to do
with operating it?

If that is sensible, then I would go with fdt - it is proven to be
able to handle such complex and open-ended needs and has supporting
tooling.

But.. it is crossing over into the DT overlay stuff, if a complex FPGA
like that is a DT overlay + bitfile then we do not need such data in
the bitfile header.

Jason

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [RFC] Add a parser in fpga_region to decode the tlv meta data suggested by Sundar
       [not found]             ` <20170224211655.GC28016@obsidianresearch.com>
@ 2017-02-24 21:41               ` Nadathur, Sundar
  2017-02-24 22:00                 ` Jason Gunthorpe
  0 siblings, 1 reply; 5+ messages in thread
From: Nadathur, Sundar @ 2017-02-24 21:41 UTC (permalink / raw)
  To: Jason Gunthorpe, Li, Yi
  Cc: Moritz Fischer, Alan Tull, linux-fpga, linux-kernel

On Friday, February 24, 2017 1:17 PM, Jason Gunthorpe wrote:
> Do we need binary data in the header?
> 
> Jason

I have talked about the need for structs and arrays, potentially nested, without really explaining why we may need them eventually. I'll get to that in a moment. But, can we agree that, if nested structs, arrays and general extensibility is needed, FDT or TLVs would be the way to go? If we agree, it becomes a FDT vs. TLV discussion.

Here's why I think we may need extensibility. (Pardon me for starting from the basics -- just trying to set the background.) A FPGA can be programmed as a whole, or can have Partial Reconfiguration (PR) regions, which can be programmed independently. The image programmed into a PR region may have components and sub-units in general. These components may have their own properties. For example, if the FPGA is exposed as a PCIe device to the host, the components may have their own registers in MMIO, DMA attributes and/or interrupts. They may also have identifiers describing their function. In general, we may want these attributes and properties, on  a per-component basis, in the metadata. Even if we don't need them tomorrow, as data center and IOT needs grow, we are likely to need that going forward. 

We could then model the properties of a component as a struct, and the set of components then becomes either an array of structs or a struct of structs. I am not trying to pick a specific object model here-- just mentioning the possibilities. 

IMHO, KVPs are good for scalar quantities. But, when we get to nested arrays/structs, we would need a tree-structured data model, such as TLVs or FDTs. 

Please let me know what you think.

Regards,
Sundar

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [RFC] Add a parser in fpga_region to decode the tlv meta data suggested by Sundar
       [not found] <1487957340-26578-1-git-send-email-yi1.li@linux.intel.com>
@ 2017-02-24 18:08 ` Nadathur, Sundar
       [not found] ` <20170224180848.GB22491@obsidianresearch.com>
  1 sibling, 0 replies; 5+ messages in thread
From: Nadathur, Sundar @ 2017-02-24 18:08 UTC (permalink / raw)
  To: yi1.li, atull, moritz.fischer, jgunthorpe; +Cc: linux-fpga, linux-kernel

[Adding linux-kernel mailing list]

> -----Original Message-----
> From: yi1.li@linux.intel.com [mailto:yi1.li@linux.intel.com]
> Sent: Friday, February 24, 2017 9:29 AM
> To: atull@kernel.org; moritz.fischer@ettus.com;
> jgunthorpe@obsidianresearch.com
> Cc: linux-fpga@vger.kernel.org; Nadathur, Sundar
> <sundar.nadathur@intel.com>; Yi Li <yi1.li@linux.intel.com>
> Subject: [RFC] Add a parser in fpga_region to decode the tlv meta data
> suggested by Sundar
> 
> From: Yi Li <yi1.li@linux.intel.com>
> 
> The TLV contains 3 fields
> Type  --- 4 bytes long,  pre-defined
> Length  --- 4 bytes long, Length refers to the length in bytes of the Value field
> Value --- variable length, TLV payload
> 
> In the patch below, there are 5 types, which can be extended.
> TYPE_MDHEADER --- Meta Data Header TLV, which can contain sub TLV types
> TYPE_ENABLETIMEOUT --- Variable type TLV, with 4 bytes long of Value
> TYPE_DISABLETIMEOUT --- Variable type TLV, with 4 bytes long of Value
> TYPE_PARTIAL_RECONFIG --- Flag type TLV, with Zero Length.
> TYPE_BITSTREAM --- Raw bit stream type TLV.
> 
> Example:
> 0x00000001 0x00000014 --- Mata Data header, includes 3 sub-TLVs below
> 0x00000002 0x00000004 0x00000004 --- fpga_image_info.enable_timeout_us
> = 0x4
> 0x00000003 0x00000004 0x00000004 ---
> fpga_image_info.disable_timeout_us = 0x4
> 0x00000005 0x00000000 --- Partial Reconfig Flag
> 0x00000004 0x00100000 --- Bitstream with 1MB long at the end
> 
> Signed-off-by: Yi Li <yi1.li@linux.intel.com>
> ---
>  drivers/fpga/fpga-mgr.c    | 10 ++++-
>  drivers/fpga/fpga-region.c | 93
> +++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 100 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index
> 3206a53..8d762d328 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -311,12 +311,18 @@ EXPORT_SYMBOL_GPL(fpga_mgr_firmware_load);
> 
>  int fpga_mgr_load(struct fpga_manager *mgr, struct fpga_image_info *info)
> {
> +	int ret;
> +
>  	if (info->firmware_name)
>  		return fpga_mgr_firmware_load(mgr, info, info-
> >firmware_name);
>  	if (info->sgt)
>  		return fpga_mgr_buf_load_sg(mgr, info, info->sgt);
> -	if (info->buf && info->count)
> -		return fpga_mgr_buf_load(mgr, info, info->buf, info->count);
> +	if (info->buf && info->count) {
> +		ret = fpga_mgr_buf_load(mgr, info, info->buf, info->count);
> +		info->buf = NULL;
> +		info->count = 0;
> +		return ret;
> +	}
>  	return -EINVAL;
>  }
>  EXPORT_SYMBOL_GPL(fpga_mgr_load);
> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c index
> a63bc6c..6d8bf47 100644
> --- a/drivers/fpga/fpga-region.c
> +++ b/drivers/fpga/fpga-region.c
> @@ -15,7 +15,7 @@
>   * You should have received a copy of the GNU General Public License along
> with
>   * this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
> -
> +#include <linux/firmware.h>
>  #include <linux/fpga/fpga-bridge.h>
>  #include <linux/fpga/fpga-mgr.h>
>  #include <linux/idr.h>
> @@ -389,6 +389,83 @@ static void fpga_region_put(struct fpga_region
> *region)
>  	mutex_unlock(&region->mutex);
>  }
> 
> +#define TYPE_MDHEADER 0x00000001
> +#define TYPE_ENABLETIMEOUT 0x00000002
> +#define TYPE_DISABLETIMEOUT 0x00000003
> +#define TYPE_BITSTREAM 0x00000004
> +#define TYPE_PARTIAL_RECONFIG 0x00000005
> +
> +/**
> + * fpga_region_parse_header - parser FPGA file header
> + * @data: file header start
> + * @size: file size
> + * @fpga_image_info
> + * @buf: raw bitstream start
> + * @count: raw bitstream size
> + *
> + * Parse the FPGA stream header.
> + *
> + * Return: 0 for success or negative error code.
> + */
> +static int fpga_region_parser_header(const char* data, size_t size,
> +				    struct fpga_image_info *image_info) {
> +	int ret = 0;
> +	u32 tlv_type;
> +	u32 tlv_len;
> +	u32 offset = 0;
> +
> +	while (size >= 8) {
> +		tlv_type = *((u32 *) (data + offset));
> +		tlv_len = *((u32 *) (data + offset + 4));
> +		offset += 8; size -= 8;
> +
> +		if (size < tlv_len)
> +			goto tlv_err;
> +
> +		switch (tlv_type) {
> +			case TYPE_MDHEADER:
> +				break;
> +			case TYPE_ENABLETIMEOUT:
> +				if (tlv_len != sizeof(u32))
> +					goto tlv_err;
> +				image_info->enable_timeout_us = *((u32 *)
> (data + offset));
> +				break;
> +			case TYPE_DISABLETIMEOUT:
> +				if (tlv_len != sizeof(u32))
> +					goto tlv_err;
> +				image_info->disable_timeout_us = *((u32 *)
> (data + offset));
> +				break;
> +			case TYPE_PARTIAL_RECONFIG:
> +				if (tlv_len != 0)
> +					goto tlv_err;
> +				image_info->flags |=
> FPGA_MGR_PARTIAL_RECONFIG;
> +				break;
> +			case TYPE_BITSTREAM:
> +				image_info->buf = data + offset;
> +				image_info->count = tlv_len;
> +				break;
> +			default:
> +				pr_err("unknown type\n");
> +				ret = -EINVAL;
> +		}
> +
> +		/* TYPE_STRUCT is a super type, decode types inside of this
> struct */
> +		if (tlv_type != TYPE_MDHEADER) {
> +			offset += tlv_len;
> +			size -= tlv_len;
> +		}
> +	}
> +
> +	image_info->firmware_name = NULL;
> +	image_info->sgt = NULL;
> +	return ret;
> +
> +tlv_err:
> +	pr_err("Error parsing tlv at type = %d offset = 0x%x\n", tlv_type,
> offset);
> +	return -EINVAL;
> +}
> +
>  /**
>   * fpga_region_program_fpga - program FPGA
>   * @region: FPGA region that is receiving an overlay @@ -401,6 +478,8 @@
> static void fpga_region_put(struct fpga_region *region)  int
> fpga_region_program_fpga(struct fpga_region *region,
>  			     struct fpga_image_info *image_info)  {
> +	struct device *dev = &region->dev;
> +	const struct firmware *fw;
>  	int ret;
> 
>  	region = fpga_region_get(region);
> @@ -415,6 +494,17 @@ int fpga_region_program_fpga(struct fpga_region
> *region,
>  		goto err_put_region;
>  	}
> 
> +	ret = request_firmware(&fw, image_info->firmware_name, dev);
> +	if (ret) {
> +		dev_err(dev, "Error requesting firmware %s\n", image_info-
> >firmware_name);
> +		goto err_put_region;
> +	}
> +
> +	/* todo: parse the header and leave results in image_info */
> +	ret = fpga_region_parser_header(fw->data, fw->size, image_info);
> +	if (ret)
> +		goto err_put_region;
> +
>  	/*
>  	 * In some cases, we already have a list of bridges in the
>  	 * fpga region struct.  Or we don't have any bridges.
> @@ -434,6 +524,7 @@ int fpga_region_program_fpga(struct fpga_region
> *region,
>  	}
> 
>  	ret = fpga_mgr_load(region->mgr, image_info);
> +	release_firmware(fw);
>  	if (ret) {
>  		pr_err("failed to load fpga image\n");
>  		goto err_put_br;
> --
> 2.1.4

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2017-03-23 15:30 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20170320225514.GA22059@obsidianresearch.com>
     [not found] ` <CAAtXAHcFCXzOXMTyAkQQCAdTnoJXOsF1nDectdC10o1KSXtscQ@mail.gmail.com>
     [not found]   ` <CANk1AXSJ_ije8cebi88ULkonRhtGi+75=x1DxwCbMcLZZcdLww@mail.gmail.com>
     [not found]     ` <CAAtXAHe8paePE+kkOPv-pWWQzUTVeihQSMzdECUWi1FN6XGytw@mail.gmail.com>
     [not found]       ` <CANk1AXQHeyM74DiY3Dz4ex0rB6BBUgRFLdPmxrKh_=GTSL+xkg@mail.gmail.com>
     [not found]         ` <CANk1AXQC_F4dSSBO1Prkui=e7zRONtyDaUx6_8CQ3-C7cgWESA@mail.gmail.com>
     [not found]           ` <CANk1AXR_Q9dFRAPn_+EuxFT8m8+Q1jsezYpvMeLBEc-g33betw@mail.gmail.com>
     [not found]             ` <CANk1AXSo8fbtbSWVhnneZcGML9+Sus76_SVMSDMmeL+EHAhppA@mail.gmail.com>
     [not found]               ` <CAAtXAHcwfUiQseLgt287vBvtuKnbCnTOJxWd_ZwJ7vmN1KP17A@mail.gmail.com>
     [not found]                 ` <CANk1AXRMGTrk5JUU_tKuZFVBiMbn4-1kQ0SajMYuPFYnjvMWVA@mail.gmail.com>
2017-03-22 21:17                   ` [RFC] Add a parser in fpga_region to decode the tlv meta data suggested by Sundar Moritz Fischer
2017-03-23 15:30                     ` Alan Tull
     [not found] <1487957340-26578-1-git-send-email-yi1.li@linux.intel.com>
2017-02-24 18:08 ` Nadathur, Sundar
     [not found] ` <20170224180848.GB22491@obsidianresearch.com>
     [not found]   ` <CAAtXAHeViHdXJ3w2Feef4Gj9txKyha6pE0empemRMKB9tCMr-w@mail.gmail.com>
     [not found]     ` <20170224184320.GA21794@obsidianresearch.com>
     [not found]       ` <190c92e5-8ea9-c19f-d40d-894d99c7305e@linux.intel.com>
     [not found]         ` <20170224210209.GA28016@obsidianresearch.com>
     [not found]           ` <bce373a4-37db-cce5-7990-5afab41eceff@linux.intel.com>
     [not found]             ` <20170224211655.GC28016@obsidianresearch.com>
2017-02-24 21:41               ` Nadathur, Sundar
2017-02-24 22:00                 ` Jason Gunthorpe

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.