| Message ID | 1487957340-26578-1-git-send-email-yi1.li@linux.intel.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
[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(®ion->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 = ®ion->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 -- 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
On Fri, Feb 24, 2017 at 11:29:00AM -0600, yi1.li@linux.intel.com wrote: > 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; > + } Tthis cannot go there, it must also work with the SGL path. > + while (size >= 8) { > + tlv_type = *((u32 *) (data + offset)); > + tlv_len = *((u32 *) (data + offset + 4)); Use a struct Missing endian annoations and swaps > + switch (tlv_type) { [..] > + default: > + pr_err("unknown type\n"); > + ret = -EINVAL; And this is my biggest complaint with this idea.. The use of a 32 bit tag and the hard failure if the tag is not understood makes the image format useless for any local information such as contained in the examples I provided. > + image_info->firmware_name = NULL; > + image_info->sgt = NULL; Wha? > > + 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; It doesn't really belong here either.. Jason -- 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
On Fri, Feb 24, 2017 at 10:08 AM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: > On Fri, Feb 24, 2017 at 11:29:00AM -0600, yi1.li@linux.intel.com wrote: >> 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; >> + } > > Tthis cannot go there, it must also work with the SGL path. Agreed. Needs handling of the SGL case, header might be scattered. > >> + while (size >= 8) { >> + tlv_type = *((u32 *) (data + offset)); >> + tlv_len = *((u32 *) (data + offset + 4)); > > Use a struct > > Missing endian annoations and swaps Which one gets built-in the fdt parsers... > >> + switch (tlv_type) { > [..] >> + default: >> + pr_err("unknown type\n"); >> + ret = -EINVAL; > > And this is my biggest complaint with this idea.. The use of a 32 bit > tag and the hard failure if the tag is not understood makes the image > format useless for any local information such as contained in the > examples I provided. To clarify, who would use that info? You're talking about the githash, etc, right? Personally I'm very interested in the githash for example and a way to encode the device the bitfile works for. Would the in-kernel consumer of that info be a custom driver? Cheers, Moritz -- 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
On Fri, Feb 24, 2017 at 10:22:08AM -0800, Moritz Fischer wrote: > >> + default: > >> + pr_err("unknown type\n"); > >> + ret = -EINVAL; > > > > And this is my biggest complaint with this idea.. The use of a 32 bit > > tag and the hard failure if the tag is not understood makes the image > > format useless for any local information such as contained in the > > examples I provided. > > To clarify, who would use that info? You're talking about the githash, > etc, right? In my world userspace makes use of some header information, and some is just for human use to track bitfile lineage. eg knowing which speedfile/software was used to build the design is really important when working chips through the ES cycle. We track this outside the bitfile as well, but stamping a header on makes it very reliable. This is why I prefer strings as keys. In the past we've also validated the package/speed grade.. > Personally I'm very interested in the githash for example and a way > to encode the device the bitfile works for. Not sure I follow that.. You mean for partial reconfiguration? Oh, Yi, I also forgot that your TLV header needs to support alignment - eg the bitfile data must be able to start on a 64 byte boundary. Jason -- 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 Jason, On Fri, Feb 24, 2017 at 10:43 AM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: > On Fri, Feb 24, 2017 at 10:22:08AM -0800, Moritz Fischer wrote: >> >> + default: >> >> + pr_err("unknown type\n"); >> >> + ret = -EINVAL; >> > >> > And this is my biggest complaint with this idea.. The use of a 32 bit >> > tag and the hard failure if the tag is not understood makes the image >> > format useless for any local information such as contained in the >> > examples I provided. >> >> To clarify, who would use that info? You're talking about the githash, >> etc, right? > > In my world userspace makes use of some header information, and some > is just for human use to track bitfile lineage. > > eg knowing which speedfile/software was used to build the design is > really important when working chips through the ES cycle. We track > this outside the bitfile as well, but stamping a header on makes it > very reliable. This is why I prefer strings as keys. Ok, understood. That makes perfect sense. > In the past we've also validated the package/speed grade.. Yeah, makes sense. >> Personally I'm very interested in the githash for example and a way >> to encode the device the bitfile works for. > > Not sure I follow that.. You mean for partial reconfiguration? Well device type, speed grade, and yes also partial reconfiguration. What I meant it's stuff we're also tracking in our designs in userland. (i.e. they're the ones I care about most, sorry for being unclear) > Oh, Yi, I also forgot that your TLV header needs to support alignment > - eg the bitfile data must be able to start on a 64 byte boundary. Which fdt makes sure of by definition... Cheers, Moritz -- 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
> -----Original Message----- > From: Moritz Fischer [mailto:moritz.fischer@ettus.com] > Sent: Friday, February 24, 2017 10:22 AM > To: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > Cc: Li, Yi <yi1.li@linux.intel.com>; Alan Tull <atull@kernel.org>; linux- > fpga@vger.kernel.org; Nadathur, Sundar <sundar.nadathur@intel.com> > Subject: Re: [RFC] Add a parser in fpga_region to decode the tlv meta data > suggested by Sundar On Friday, February 24, 2017 10:22 AM, Moritz Fischer wrote: > On Fri, Feb 24, 2017 at 10:08 AM, Jason Gunthorpe > <jgunthorpe@obsidianresearch.com> wrote: > > On Fri, Feb 24, 2017 at 11:29:00AM -0600, yi1.li@linux.intel.com wrote: > > > >> + switch (tlv_type) { > > [..] > >> + default: > >> + pr_err("unknown type\n"); > >> + ret = -EINVAL; > > > > And this is my biggest complaint with this idea.. The use of a 32 bit > > tag and the hard failure if the tag is not understood makes the image > > format useless for any local information such as contained in the > > examples I provided. It should have been clarified that this is illustrative code. It is easy to change this to skip unknown types or, better still, check a bit in the type field to decide whether the unknown type should be skipped or errored out. Regards, Sundar
On 2/24/2017 12:08 PM, Jason Gunthorpe wrote: > On Fri, Feb 24, 2017 at 11:29:00AM -0600, yi1.li@linux.intel.com wrote: >> 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; >> + } > Tthis cannot go there, it must also work with the SGL path. Okay > >> + while (size >= 8) { >> + tlv_type = *((u32 *) (data + offset)); >> + tlv_len = *((u32 *) (data + offset + 4)); > Use a struct > > Missing endian annoations and swaps Thanks for the suggestion, will do. > >> + switch (tlv_type) { > [..] >> + default: >> + pr_err("unknown type\n"); >> + ret = -EINVAL; > And this is my biggest complaint with this idea.. The use of a 32 bit > tag and the hard failure if the tag is not understood makes the image > format useless for any local information such as contained in the > examples I provided. Actually the parser continue/skip to the next TLV when hit unknown type/tag. > >> + image_info->firmware_name = NULL; >> + image_info->sgt = NULL; > Wha? > >> >> + 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; > It doesn't really belong here either.. Agree, will move the parser into lower level load functions like fpga_mgr_firmware_load. > > Jason > -- 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
On 2/24/2017 12:43 PM, Jason Gunthorpe wrote: > On Fri, Feb 24, 2017 at 10:22:08AM -0800, Moritz Fischer wrote: >>>> + default: >>>> + pr_err("unknown type\n"); >>>> + ret = -EINVAL; >>> And this is my biggest complaint with this idea.. The use of a 32 bit >>> tag and the hard failure if the tag is not understood makes the image >>> format useless for any local information such as contained in the >>> examples I provided. >> To clarify, who would use that info? You're talking about the githash, >> etc, right? > In my world userspace makes use of some header information, and some > is just for human use to track bitfile lineage. > > eg knowing which speedfile/software was used to build the design is > really important when working chips through the ES cycle. We track > this outside the bitfile as well, but stamping a header on makes it > very reliable. This is why I prefer strings as keys. > > In the past we've also validated the package/speed grade.. You can still add TLV for human readable mark or user space usage. The Kernel parser just ignore those TLV it does not understand. > >> Personally I'm very interested in the githash for example and a way >> to encode the device the bitfile works for. > Not sure I follow that.. You mean for partial reconfiguration? > > Oh, Yi, I also forgot that your TLV header needs to support alignment > - eg the bitfile data must be able to start on a 64 byte boundary. > > Jason > -- > 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 linux-fpga" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> >eg knowing which speedfile/software was used to build the design is > >really important when working chips through the ES cycle. We track > >this outside the bitfile as well, but stamping a header on makes it > >very reliable. This is why I prefer strings as keys. > > > >In the past we've also validated the package/speed grade.. > > You can still add TLV for human readable mark or user space usage. I can always hack something together with any of the approaches :) My point continues to be this isn't substantially simpler than the other two ideas and gives up string based tags in the process.. > The Kernel parser just ignore those TLV it does not understand. You need to take out the errno set then: > + default: > + pr_err("unknown type\n"); > + ret = -EINVAL; Jason -- 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
On 2/24/2017 3:02 PM, Jason Gunthorpe wrote: >>> eg knowing which speedfile/software was used to build the design is >>> really important when working chips through the ES cycle. We track >>> this outside the bitfile as well, but stamping a header on makes it >>> very reliable. This is why I prefer strings as keys. >>> >>> In the past we've also validated the package/speed grade.. >> You can still add TLV for human readable mark or user space usage. > I can always hack something together with any of the approaches :) > > My point continues to be this isn't substantially simpler than the > other two ideas and gives up string based tags in the process.. That's true :-). The advantage of the TLV in my opinion is it can handle binary info better, but does not prevent ASCII codes. > >> The Kernel parser just ignore those TLV it does not understand. > You need to take out the errno set then: > >> + default: >> + pr_err("unknown type\n"); >> + ret = -EINVAL; Yes, you are right! > Jason -- 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
On Fri, Feb 24, 2017 at 03:08:31PM -0600, Li, Yi wrote: > On 2/24/2017 3:02 PM, Jason Gunthorpe wrote: > >>>eg knowing which speedfile/software was used to build the design is > >>>really important when working chips through the ES cycle. We track > >>>this outside the bitfile as well, but stamping a header on makes it > >>>very reliable. This is why I prefer strings as keys. > >>> > >>>In the past we've also validated the package/speed grade.. > >>You can still add TLV for human readable mark or user space usage. > >I can always hack something together with any of the approaches :) > > > >My point continues to be this isn't substantially simpler than the > >other two ideas and gives up string based tags in the process.. > > That's true :-). The advantage of the TLV in my opinion is it can handle > binary info better, but does not prevent ASCII codes. Do we need binary data in the header? Jason -- 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
On 2/24/2017 3:16 PM, Jason Gunthorpe wrote: > On Fri, Feb 24, 2017 at 03:08:31PM -0600, Li, Yi wrote: > >> On 2/24/2017 3:02 PM, Jason Gunthorpe wrote: >>>>> eg knowing which speedfile/software was used to build the design is >>>>> really important when working chips through the ES cycle. We track >>>>> this outside the bitfile as well, but stamping a header on makes it >>>>> very reliable. This is why I prefer strings as keys. >>>>> >>>>> In the past we've also validated the package/speed grade.. >>>> You can still add TLV for human readable mark or user space usage. >>> I can always hack something together with any of the approaches :) >>> >>> My point continues to be this isn't substantially simpler than the >>> other two ideas and gives up string based tags in the process.. >> That's true :-). The advantage of the TLV in my opinion is it can handle >> binary info better, but does not prevent ASCII codes. > Do we need binary data in the header? Like you mentioned, people might put stamp (can be human readable or machine readable) on the file. Sundar will be a better person to explain this since the TLV requirement is from his team. :-) > > Jason -- 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
On Fri, Feb 24, 2017 at 1:22 PM, Li, Yi <yi1.li@linux.intel.com> wrote: > Like you mentioned, people might put stamp (can be human readable or machine > readable) on the file. Sundar will be a better person to explain this since > the TLV requirement is from his team. :-) Requirement? ... This almost sounds like someone's trying to push some internal format, not for technical reasons but because it's already there ... If those kinds of arguments now count we should use Jason's plaintext headers since he's been using them for a while, and it would be a shame if he'd have to write new code. So far I haven't heard any *technical* arguments why TLVs are better than fdt or plaintext. Cheers, Moritz -- 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
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 -- 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
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 -- 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
On Friday, February 24, 2017 1:39 PM, Moritz Fischer wrote: > On Fri, Feb 24, 2017 at 1:22 PM, Li, Yi <yi1.li@linux.intel.com> wrote: > > > Like you mentioned, people might put stamp (can be human readable or > > machine > > readable) on the file. Sundar will be a better person to explain this > > since the TLV requirement is from his team. :-) > > Requirement? ... This almost sounds like someone's trying to push some > internal format, not for technical reasons but because it's already there ... > > If those kinds of arguments now count we should use Jason's plaintext > headers since he's been using them for a while, and it would be a shame if > he'd have to write new code. > > So far I haven't heard any *technical* arguments why TLVs are better than > fdt or plaintext. > > Cheers, > > Moritz Hi Moritz, I suppose it is easy for the words to get misconstrued. I did request Yi and team to help with the coding, since they have ready access to the code base and the build environment. That is the 'TLV requirement'. I took my time to reply to this thread, so Yi is right to ask me to get back in again. At the end of the day, our discussion will be technical of course. Regards, Sundar
Sundar, On Fri, Feb 24, 2017 at 2:23 PM, Nadathur, Sundar <sundar.nadathur@intel.com> wrote: > I suppose it is easy for the words to get misconstrued. I did request Yi and team to help with the coding, since they have ready access to the code base and the build environment. That is the 'TLV requirement'. I took my time to reply to this thread, so Yi is right to ask me to get back in again. Yeah, I saw your response. All good. As Jason pointed out I think enumeration and flags for low-level drivers are separate problems (with different requirements). > At the end of the day, our discussion will be technical of course. Thanks! Moritz -- 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
On Fri, Feb 24, 2017 at 2:19 PM, Li, Yi <yi1.li@linux.intel.com> wrote: > > > On 2/24/2017 12:08 PM, Jason Gunthorpe wrote: >> >> On Fri, Feb 24, 2017 at 11:29:00AM -0600, yi1.li@linux.intel.com wrote: >>> >>> 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; >>> + } >> >> Tthis cannot go there, it must also work with the SGL path. > > > Okay > >> >>> + while (size >= 8) { >>> + tlv_type = *((u32 *) (data + offset)); >>> + tlv_len = *((u32 *) (data + offset + 4)); >> >> Use a struct >> >> Missing endian annoations and swaps > > > Thanks for the suggestion, will do. > >> >>> + switch (tlv_type) { >> >> [..] >>> >>> + default: >>> + pr_err("unknown type\n"); >>> + ret = -EINVAL; >> >> And this is my biggest complaint with this idea.. The use of a 32 bit >> tag and the hard failure if the tag is not understood makes the image >> format useless for any local information such as contained in the >> examples I provided. > > > Actually the parser continue/skip to the next TLV when hit unknown type/tag. > >> >>> + image_info->firmware_name = NULL; >>> + image_info->sgt = NULL; >> >> Wha? >> >>> + 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; >> >> It doesn't really belong here either.. > > > Agree, will move the parser into lower level load functions like > fpga_mgr_firmware_load. That won't work either ;) The header may have info needed by fpga-mgr or by fpga-bridge. We need to parse the header in fpga-region.c. But that is true regardless of which of the three header proposals we go with and not specific to TLV. Looks like I will need to rework fpga-region.c to be able to get the header here whether it has been given a contiguous buffer, sg buffer, or firmware. Alan > >> >> Jason >> > -- 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
On Fri, Feb 24, 2017 at 4:42 PM, Moritz Fischer <moritz.fischer@ettus.com> wrote: >> At the end of the day, our discussion will be technical of course. One thing I want support is a single file containing several bitstreams. Some FPGAs such as Arria 10 already require programming the ioring first and then the core; it would be helpful to keep track of them together in one file. I also want to support optional enumeration data in the header. We should require putting the bitstream at the end of the file, after the header, to support situations where the system has very little memory such as a bootloader running out of on-chip memory. To support > 1 bitstream, the header would need entries that tell what the file offset to each bitstream is. One downside of TLV is that there is this ever-increasing list of #defines for the types. Also it loses the ease of reading the ascii header. I'd like to propose something combining the strengths of KVP and TLV. Call it Key/Length/Value (KLV). Each entry: 1. NULL-terminated ascii string for key 2. 32 bit length of the value 3. value (payload) This eliminates needing to have a header with list of #defines for the types. This allows binary or text data to be represented as well as entries that include other entries. It will be easy to parse. Adding length to the scripts for KVP wouldn't be hard, just need to change the header type so code receiving the header would know that the length would be there. Either TLV or KLV enables a multi-bitstream file header which would include the following entries or sets of entries: * Header format specifier comes first * Header length * other optional stuff about who/how/when the file was built * bitstream 0 info * Entry specifying FPGA info for bitstream 0 * entry specifying the offset of the raw bitstream 0 * enumeration to apply after bitstream 0 is programmed * other optional stuff * bitstream 1 info * Entry specifying FPGA info for bitstream 1 * entry specifying the offset of the raw bitstream 1 * enumeration to apply after bitstream 1 is programmed * other optional stuff ... other bitstream info sections if needed * raw bitstream 0 * raw bitstream 1 ... other raw bitstreams The entries in the 'bistream 0 info' and 'bitstream 1 info' can be in any order, just contained in that section. Besides that, here is a list of the requirements for the header as I understand from our discussion plus a few I'm adding. The things listed as optional are things we should require that the header can support, but they can be present or not present in a bitstream. * header size. * info needed for programming the bitstream. * offset to raw bitstream. * optional non-hardware info (who built the bitstream image, what tool/version was used, date built, etc). * optional FPGA device info. * optional enumeration info (could be FDT or something else depending on the OS). * padding to get the right alignment for the raw bitstream * (anything else?) * All added header info has to go *before* the raw bitstream. In the case where only one bitstream is in the file we could eliminate the offset to the bitstream and assume the header size is the offset. I don't think FDT is a good fit for the requirements of the header. We would have to lose the optional non-hardware info since device tree is supposed to describe hardware. So adding who built the bitstreamm, when it was built, and the tool version that built it wouldn't be allowed in FDT headers. I don't want to lose that, it's too handy. Also for FDT we would have to go to the device tree mailing list every time we, want to add another compatible string, which a piece of rigidity I'd like to avoid. Alan Tull -- 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
On Mon, Mar 20, 2017 at 05:45:15PM -0500, Alan Tull wrote: > One downside of TLV is that there is this ever-increasing list of > #defines for the types. Also it loses the ease of reading the ascii > header. I'd like to propose something combining the strengths of KVP > and TLV. Call it Key/Length/Value (KLV). Each entry: > > 1. NULL-terminated ascii string for key > 2. 32 bit length of the value > 3. value (payload) I think you more or less just described fdt.. With this sort of complexity, fdt is a reasonable choice. > I don't think FDT is a good fit for the requirements of the header. > We would have to lose the optional non-hardware info since device tree > is supposed to describe hardware. If this used FDT it would be a totally different world from the firmware interface use of FDT, much like that uboot stuff. It is just using the same binary format and parser, but a totally unique schema. So, FPGA gets to have its own rules and the 'describe hardware' business is not applicable, nor is the DT mailing list process.. Jason -- 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
Jason, On Mon, Mar 20, 2017 at 3:55 PM, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote: > On Mon, Mar 20, 2017 at 05:45:15PM -0500, Alan Tull wrote: > >> One downside of TLV is that there is this ever-increasing list of >> #defines for the types. Also it loses the ease of reading the ascii >> header. I'd like to propose something combining the strengths of KVP >> and TLV. Call it Key/Length/Value (KLV). Each entry: >> >> 1. NULL-terminated ascii string for key >> 2. 32 bit length of the value >> 3. value (payload) > > I think you more or less just described fdt.. > > With this sort of complexity, fdt is a reasonable choice. > >> I don't think FDT is a good fit for the requirements of the header. >> We would have to lose the optional non-hardware info since device tree >> is supposed to describe hardware. > > If this used FDT it would be a totally different world from the > firmware interface use of FDT, much like that uboot stuff. It is just > using the same binary format and parser, but a totally unique schema. > > So, FPGA gets to have its own rules and the 'describe hardware' > business is not applicable, nor is the DT mailing list process.. You beat me to it ;-) I was just typing up my response. I totally agree with you: FDT = Binary Format which is NOT the same as OF device-tree usage, and therefore does not need to obey the 'describe hardware' rule. Reusing the parser and encoding was the idea, not reusing device-tree as a concept in the OF sense. Cheers, Moritz -- 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
On Mon, Mar 20, 2017 at 6:04 PM, Moritz Fischer <moritz.fischer@ettus.com> wrote: > Jason, > > On Mon, Mar 20, 2017 at 3:55 PM, Jason Gunthorpe > <jgunthorpe@obsidianresearch.com> wrote: >> On Mon, Mar 20, 2017 at 05:45:15PM -0500, Alan Tull wrote: >> >>> One downside of TLV is that there is this ever-increasing list of >>> #defines for the types. Also it loses the ease of reading the ascii >>> header. I'd like to propose something combining the strengths of KVP >>> and TLV. Call it Key/Length/Value (KLV). Each entry: >>> >>> 1. NULL-terminated ascii string for key >>> 2. 32 bit length of the value >>> 3. value (payload) >> >> I think you more or less just described fdt.. >> >> With this sort of complexity, fdt is a reasonable choice. >> >>> I don't think FDT is a good fit for the requirements of the header. >>> We would have to lose the optional non-hardware info since device tree >>> is supposed to describe hardware. >> >> If this used FDT it would be a totally different world from the >> firmware interface use of FDT, much like that uboot stuff. It is just >> using the same binary format and parser, but a totally unique schema. >> >> So, FPGA gets to have its own rules and the 'describe hardware' >> business is not applicable, nor is the DT mailing list process.. > > You beat me to it ;-) I was just typing up my response. I totally agree with > you: > > FDT = Binary Format which is NOT the same as OF device-tree usage, > and therefore does not need to obey the 'describe hardware' rule. > Reusing the parser and encoding was the idea, not reusing device-tree > as a concept in the OF sense. > > Cheers, > > Moritz Wow, I totally didn't see that coming! I understand your points. I'll play with it. If we do that, it will be necessary to include the version of libfdt to use in the header. Alan -- 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
Alan, On Tue, Mar 21, 2017 at 8:10 AM, Alan Tull <delicious.quinoa@gmail.com> wrote: > On Mon, Mar 20, 2017 at 6:04 PM, Moritz Fischer > <moritz.fischer@ettus.com> wrote: >> Jason, >> >> On Mon, Mar 20, 2017 at 3:55 PM, Jason Gunthorpe >> <jgunthorpe@obsidianresearch.com> wrote: >>> On Mon, Mar 20, 2017 at 05:45:15PM -0500, Alan Tull wrote: >>> >>>> One downside of TLV is that there is this ever-increasing list of >>>> #defines for the types. Also it loses the ease of reading the ascii >>>> header. I'd like to propose something combining the strengths of KVP >>>> and TLV. Call it Key/Length/Value (KLV). Each entry: >>>> >>>> 1. NULL-terminated ascii string for key >>>> 2. 32 bit length of the value >>>> 3. value (payload) >>> >>> I think you more or less just described fdt.. >>> >>> With this sort of complexity, fdt is a reasonable choice. >>> >>>> I don't think FDT is a good fit for the requirements of the header. >>>> We would have to lose the optional non-hardware info since device tree >>>> is supposed to describe hardware. >>> >>> If this used FDT it would be a totally different world from the >>> firmware interface use of FDT, much like that uboot stuff. It is just >>> using the same binary format and parser, but a totally unique schema. >>> >>> So, FPGA gets to have its own rules and the 'describe hardware' >>> business is not applicable, nor is the DT mailing list process.. >> >> You beat me to it ;-) I was just typing up my response. I totally agree with >> you: >> >> FDT = Binary Format which is NOT the same as OF device-tree usage, >> and therefore does not need to obey the 'describe hardware' rule. >> Reusing the parser and encoding was the idea, not reusing device-tree >> as a concept in the OF sense. >> >> Cheers, >> >> Moritz > > Wow, I totally didn't see that coming! I understand your points. > I'll play with it. Cool :) > If we do that, it will be necessary to include the version of libfdt > to use in the header. I think fdt has provisioning to do that / or something similar already [1], I'm happy to help out writing the code since I spent time researching it for my example that I had sent earlier. Cheers, Moritz [1] http://lxr.free-electrons.com/source/scripts/dtc/libfdt/fdt.c#L58 -- 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
On Tue, Mar 21, 2017 at 11:24 AM, Moritz Fischer <moritz.fischer@ettus.com> wrote: > Alan, > > On Tue, Mar 21, 2017 at 8:10 AM, Alan Tull <delicious.quinoa@gmail.com> wrote: >> On Mon, Mar 20, 2017 at 6:04 PM, Moritz Fischer >> <moritz.fischer@ettus.com> wrote: >>> Jason, >>> >>> On Mon, Mar 20, 2017 at 3:55 PM, Jason Gunthorpe >>> <jgunthorpe@obsidianresearch.com> wrote: >>>> On Mon, Mar 20, 2017 at 05:45:15PM -0500, Alan Tull wrote: >>>> >>>>> One downside of TLV is that there is this ever-increasing list of >>>>> #defines for the types. Also it loses the ease of reading the ascii >>>>> header. I'd like to propose something combining the strengths of KVP >>>>> and TLV. Call it Key/Length/Value (KLV). Each entry: >>>>> >>>>> 1. NULL-terminated ascii string for key >>>>> 2. 32 bit length of the value >>>>> 3. value (payload) >>>> >>>> I think you more or less just described fdt.. >>>> >>>> With this sort of complexity, fdt is a reasonable choice. >>>> >>>>> I don't think FDT is a good fit for the requirements of the header. >>>>> We would have to lose the optional non-hardware info since device tree >>>>> is supposed to describe hardware. >>>> >>>> If this used FDT it would be a totally different world from the >>>> firmware interface use of FDT, much like that uboot stuff. It is just >>>> using the same binary format and parser, but a totally unique schema. >>>> >>>> So, FPGA gets to have its own rules and the 'describe hardware' >>>> business is not applicable, nor is the DT mailing list process.. >>> >>> You beat me to it ;-) I was just typing up my response. I totally agree with >>> you: >>> >>> FDT = Binary Format which is NOT the same as OF device-tree usage, >>> and therefore does not need to obey the 'describe hardware' rule. >>> Reusing the parser and encoding was the idea, not reusing device-tree >>> as a concept in the OF sense. >>> >>> Cheers, >>> >>> Moritz >> >> Wow, I totally didn't see that coming! I understand your points. >> I'll play with it. > > Cool :) > >> If we do that, it will be necessary to include the version of libfdt >> to use in the header. > > I think fdt has provisioning to do that / or something similar already [1], > I'm happy to help out writing the code since I spent time researching > it for my example that I had sent earlier. That would be totally cool and very helpful! I suggest the following file layout: 1. specify file type (what should that look like?) 2. specify size of header including enumeration if it's present, but not including the (potentially huge) bitstream. (4 bytes?) 3. FDT header including file offsets for bitstream(s) and FDT overlay(s) if present 4. FDT overlay(s) if present 5. padding 6. bitstream(s) For the kernel part, I have patch (applies on top of my current patchset) where I was experimenting with TLV including enumeration by applying DTO entries from the header. So I just need to gut the functions that get the size of the header from the first chunk of the header and the function that parses the header. It turned out that implementing this wasn't super straightforward as I also needed to implement applying an DT overlay from the header, getting the header from a sg buffer, and some other fpga-mgr.c fixups that were needed to allow fpga-mgr.c to not assume raw bitstreams. My changes for sg aren't tested yet. How do you feel about writing the tool that creates the header? I haven't started on that at all yet and was starting to try to gather the pieces for it. Alan > > Cheers, > > Moritz > > [1] http://lxr.free-electrons.com/source/scripts/dtc/libfdt/fdt.c#L58 -- 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
On Tue, Mar 21, 2017 at 3:30 PM, Alan Tull <delicious.quinoa@gmail.com> wrote: > I suggest the following file layout: > > 1. specify file type (what should that look like?) > 2. specify size of header including enumeration if it's present, but > not including the (potentially huge) bitstream. (4 bytes?) > 3. FDT header including file offsets for bitstream(s) and FDT > overlay(s) if present > 4. FDT overlay(s) if present > 5. padding > 6. bitstream(s) > > > Or are we talking specifically about FIT? 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. 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? > For the kernel part, I have patch (applies on top of my current > patchset) where I was experimenting with TLV including enumeration by > applying DTO entries from the header. So I just need to gut the > functions that get the size of the header from the first chunk of the > header and the function that parses the header. It turned out that > implementing this wasn't super straightforward as I also needed to > implement applying an DT overlay from the header, getting the header > from a sg buffer, and some other fpga-mgr.c fixups that were needed to > allow fpga-mgr.c to not assume raw bitstreams. My changes for sg > aren't tested yet. > > How do you feel about writing the tool that creates the header? I > haven't started on that at all yet and was starting to try to gather > the pieces for it. Sounds good, I'll take another look at what the u-boot guys do for FIT and might get around to write some code next week. Cheers, Moritz -- 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
Moritz, On Tue, 21 Mar 2017, Moritz Fischer wrote: >>On Tue, Mar 21, 2017 at 3:30 PM, Alan Tull <delicious.quinoa@gmail.com> wrote: >> >>> I suggest the following file layout: >>> >>> 1. specify file type (what should that look like?) >>> 2. specify size of header including enumeration if it's present, but >>> not including the (potentially huge) bitstream. (4 bytes?) >>> 3. FDT header including file offsets for bitstream(s) and FDT >>> overlay(s) if present >>> 4. FDT overlay(s) if present >>> 5. padding >>> 6. bitstream(s) >>> >>> >>> Or are we talking specifically about FIT? >> >>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. >> >>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? >> >>> For the kernel part, I have patch (applies on top of my current >>> patchset) where I was experimenting with TLV including enumeration by >>> applying DTO entries from the header. So I just need to gut the >>> functions that get the size of the header from the first chunk of the >>> header and the function that parses the header. It turned out that >>> implementing this wasn't super straightforward as I also needed to >>> implement applying an DT overlay from the header, getting the header >>> from a sg buffer, and some other fpga-mgr.c fixups that were needed to >>> allow fpga-mgr.c to not assume raw bitstreams. My changes for sg >>> aren't tested yet. >>> >>> How do you feel about writing the tool that creates the header? I >>> haven't started on that at all yet and was starting to try to gather >>> the pieces for it. >> >>Sounds good, I'll take another look at what the u-boot guys do for FIT >>and might get around to write some code next week. Great! Since we want this header to be OS agnostic, it's important to use licenses that allow for the use of the code on multiple OS'es, along the lines of libfdt. I take that is what you have in mind, correct? Cheers, Yves >> >>Cheers, >> >>Moritz >>-- >>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 linux-fpga" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 22, 2017 at 1:33 AM, Moritz Fischer <moritz.fischer@ettus.com> wrote: > On Tue, Mar 21, 2017 at 3:30 PM, Alan Tull <delicious.quinoa@gmail.com> wrote: > >> I suggest the following file layout: >> >> 1. specify file type (what should that look like?) >> 2. specify size of header including enumeration if it's present, but >> not including the (potentially huge) bitstream. (4 bytes?) >> 3. FDT header including file offsets for bitstream(s) and FDT >> overlay(s) if present >> 4. FDT overlay(s) if present >> 5. padding >> 6. bitstream(s) >> >> >> Or are we talking specifically about FIT? > > 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. > > 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. Alan > >> For the kernel part, I have patch (applies on top of my current >> patchset) where I was experimenting with TLV including enumeration by >> applying DTO entries from the header. So I just need to gut the >> functions that get the size of the header from the first chunk of the >> header and the function that parses the header. It turned out that >> implementing this wasn't super straightforward as I also needed to >> implement applying an DT overlay from the header, getting the header >> from a sg buffer, and some other fpga-mgr.c fixups that were needed to >> allow fpga-mgr.c to not assume raw bitstreams. My changes for sg >> aren't tested yet. >> >> How do you feel about writing the tool that creates the header? I >> haven't started on that at all yet and was starting to try to gather >> the pieces for it. > > Sounds good, I'll take another look at what the u-boot guys do for FIT > and might get around to write some code next week. > > Cheers, > > Moritz -- 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
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
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 -- 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
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(®ion->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 = ®ion->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;