[U-Boot] [PATCH 0/2] common: fitimage support for arbitrary fpga type
The intent of these patches is to modify existing fitimage support for fpga configuration to allow configuration of any fpga type supported by the fpga_load command. Note though, that fitimage support for fpga configuration only allows for configuration of the fpga of devnum 0.
Dalon Westergreen (2): common: image: update boot_get_fpga to support arbitrary fpga image common: bootm: add support for arbitrary fgpa configuration
common/bootm.c | 2 +- common/image.c | 37 ++++++++++++++++++++++--------------- 2 files changed, 23 insertions(+), 16 deletions(-)
The implementation of boot_get_fpga only supported one fpga family. This modification allows for any of the fpga devices supported by fpga_load to be used.
Signed-off-by: Dalon Westergreen dwesterg@gmail.com --- common/image.c | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-)
diff --git a/common/image.c b/common/image.c index 0f88984..792d371 100644 --- a/common/image.c +++ b/common/image.c @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images, uint8_t arch, }
#if IMAGE_ENABLE_FIT -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) +#if defined(CONFIG_FPGA) int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, uint8_t arch, const ulong *ld_start, ulong * const ld_len) { @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, int err; int devnum = 0; /* TODO support multi fpga platforms */ const fpga_desc * const desc = fpga_get_desc(devnum); - xilinx_desc *desc_xilinx = desc->devdesc; + xilinx_desc *desc_xilinx; + bitstream_type bstype;
/* Check to see if the images struct has a FIT configuration */ if (!genimg_has_config(images)) { @@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, return fit_img_result; }
- if (img_len >= desc_xilinx->size) { + switch (desc->devtype) { + case fpga_xilinx: + desc_xilinx = desc->devdesc; + if (img_len >= desc_xilinx->size) { + name = "full"; + bstype = BIT_FULL; + } else { + name = "partial"; + bstype = BIT_PARTIAL; + } + break; + default: name = "full"; - err = fpga_loadbitstream(devnum, (char *)img_data, - img_len, BIT_FULL); - if (err) - err = fpga_load(devnum, (const void *)img_data, - img_len, BIT_FULL); - } else { - name = "partial"; - err = fpga_loadbitstream(devnum, (char *)img_data, - img_len, BIT_PARTIAL); - if (err) - err = fpga_load(devnum, (const void *)img_data, - img_len, BIT_PARTIAL); + bstype = BIT_FULL; }
+ err = fpga_loadbitstream(devnum, (char *)img_data, + img_len, bstype); + if (err) + err = fpga_load(devnum, (const void *)img_data, + img_len, bstype); + if (err) return err;
On 02/19/2017 08:49 PM, Dalon Westergreen wrote:
The implementation of boot_get_fpga only supported one fpga family. This modification allows for any of the fpga devices supported by fpga_load to be used.
Signed-off-by: Dalon Westergreen dwesterg@gmail.com
+CC Xilinx friends :)
common/image.c | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-)
diff --git a/common/image.c b/common/image.c index 0f88984..792d371 100644 --- a/common/image.c +++ b/common/image.c @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images, uint8_t arch, }
#if IMAGE_ENABLE_FIT -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) +#if defined(CONFIG_FPGA) int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, uint8_t arch, const ulong *ld_start, ulong * const ld_len) { @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, int err; int devnum = 0; /* TODO support multi fpga platforms */ const fpga_desc * const desc = fpga_get_desc(devnum);
- xilinx_desc *desc_xilinx = desc->devdesc;
xilinx_desc *desc_xilinx;
bitstream_type bstype;
/* Check to see if the images struct has a FIT configuration */ if (!genimg_has_config(images)) {
@@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, return fit_img_result; }
if (img_len >= desc_xilinx->size) {
switch (desc->devtype) {
Do we need the switch statement at all ? We can have full configuration as a default mode of operation and have something like
if (xilinx) { if (partial reconfiguration) { do_special_setup(); } }
But even better would be to move this platform-dependent stuff into drivers/fpga/ or somewhere there. This is common code, so it shouldn't be here in the first place.
case fpga_xilinx:desc_xilinx = desc->devdesc;if (img_len >= desc_xilinx->size) {name = "full";bstype = BIT_FULL;} else {name = "partial";bstype = BIT_PARTIAL;}break;default: name = "full";
err = fpga_loadbitstream(devnum, (char *)img_data,img_len, BIT_FULL);if (err)err = fpga_load(devnum, (const void *)img_data,img_len, BIT_FULL);} else {name = "partial";err = fpga_loadbitstream(devnum, (char *)img_data,img_len, BIT_PARTIAL);if (err)err = fpga_load(devnum, (const void *)img_data,img_len, BIT_PARTIAL);
bstype = BIT_FULL;}
err = fpga_loadbitstream(devnum, (char *)img_data,img_len, bstype);if (err)err = fpga_load(devnum, (const void *)img_data,img_len, bstype);if (err) return err;
On Sun, 2017-02-19 at 21:07 +0100, Marek Vasut wrote:
On 02/19/2017 08:49 PM, Dalon Westergreen wrote:
The implementation of boot_get_fpga only supported one fpga family. This modification allows for any of the fpga devices supported by fpga_load to be used.
Signed-off-by: Dalon Westergreen dwesterg@gmail.com
+CC Xilinx friends :)
common/image.c | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-)
diff --git a/common/image.c b/common/image.c index 0f88984..792d371 100644 --- a/common/image.c +++ b/common/image.c @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images, uint8_t arch, } #if IMAGE_ENABLE_FIT -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) +#if defined(CONFIG_FPGA) int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, uint8_t arch, const ulong *ld_start, ulong * const ld_len) { @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, int err; int devnum = 0; /* TODO support multi fpga platforms */ const fpga_desc * const desc = fpga_get_desc(devnum);
- xilinx_desc *desc_xilinx = desc->devdesc;
- xilinx_desc *desc_xilinx;
- bitstream_type bstype;
/* Check to see if the images struct has a FIT configuration */ if (!genimg_has_config(images)) { @@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, return fit_img_result; }
if (img_len >= desc_xilinx->size) {
switch (desc->devtype) {Do we need the switch statement at all ? We can have full configuration as a default mode of operation and have something like
if (xilinx) { if (partial reconfiguration) { do_special_setup(); } }
I only did the switch stuff b/c i envisioned a need for partial image support for socfpga. That said, i would suggest, as you mention, moving this to platform specific code and perhaps an indication of the image type in the fitimage.
But even better would be to move this platform-dependent stuff into drivers/fpga/ or somewhere there. This is common code, so it shouldn't be here in the first place.
My preference would be to only call fpga_load and have the platform specific stuff figure out what they want to do. My next comment would be that perhaps it is best to add an fpgap type or some such in the fitimage to specify the image is a partial image rather than looking at the image size?
Also a consideration is that there should be a means of specifying the fpga devnum somehow in the fitimage? it is plausible that a system could have multiple fpgas, no?
--dalon
case fpga_xilinx:desc_xilinx = desc->devdesc;if (img_len >= desc_xilinx->size) {name = "full";bstype = BIT_FULL;} else {name = "partial";bstype = BIT_PARTIAL;}break;default:name = "full";
err = fpga_loadbitstream(devnum, (char *)img_data,img_len, BIT_FULL);if (err)err = fpga_load(devnum, (const void*)img_data,
img_len, BIT_FULL);} else {name = "partial";err = fpga_loadbitstream(devnum, (char *)img_data,img_len, BIT_PARTIAL);if (err)err = fpga_load(devnum, (const void*)img_data,
img_len, BIT_PARTIAL);
bstype = BIT_FULL;}
err = fpga_loadbitstream(devnum, (char *)img_data,img_len, bstype);if (err)err = fpga_load(devnum, (const void *)img_data,img_len, bstype);if (err) return err;
On 02/19/2017 09:43 PM, Dalon Westergreen wrote:
On Sun, 2017-02-19 at 21:07 +0100, Marek Vasut wrote:
On 02/19/2017 08:49 PM, Dalon Westergreen wrote:
The implementation of boot_get_fpga only supported one fpga family. This modification allows for any of the fpga devices supported by fpga_load to be used.
Signed-off-by: Dalon Westergreen dwesterg@gmail.com
+CC Xilinx friends :)
common/image.c | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-)
diff --git a/common/image.c b/common/image.c index 0f88984..792d371 100644 --- a/common/image.c +++ b/common/image.c @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images, uint8_t arch, }
#if IMAGE_ENABLE_FIT -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) +#if defined(CONFIG_FPGA) int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, uint8_t arch, const ulong *ld_start, ulong * const ld_len) { @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, int err; int devnum = 0; /* TODO support multi fpga platforms */ const fpga_desc * const desc = fpga_get_desc(devnum);
- xilinx_desc *desc_xilinx = desc->devdesc;
xilinx_desc *desc_xilinx;
bitstream_type bstype;
/* Check to see if the images struct has a FIT configuration */ if (!genimg_has_config(images)) {
@@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, return fit_img_result; }
if (img_len >= desc_xilinx->size) {
switch (desc->devtype) {Do we need the switch statement at all ? We can have full configuration as a default mode of operation and have something like
if (xilinx) { if (partial reconfiguration) { do_special_setup(); } }
I only did the switch stuff b/c i envisioned a need for partial image support for socfpga.
That'd be seriously cool :)
That said, i would suggest, as you mention, moving this to platform specific code and perhaps an indication of the image type in the fitimage.
driver-specific code . It doesn't need to know the imagetype, just that the blob that you passed in is a partial-reconfiguration blob. I never really worked with P/R though, do you need some other metadata for that or is it contained in that P/R bitstream blob already ?
But even better would be to move this platform-dependent stuff into drivers/fpga/ or somewhere there. This is common code, so it shouldn't be here in the first place.
My preference would be to only call fpga_load and have the platform
s/platform/driver/
specific stuff figure out what they want to do.
Agreed
My next comment would be that perhaps it is best to add an fpgap type or some such in the fitimage to specify the image is a partial image rather than looking at the image size?
Hmmmm, see my question above. If the driver cannot discern it from the blob, maybe a DT-alike property would work. ie. the image entry would also have a "xlnx,partial-reconfiguration" property or somesuch .
Also a consideration is that there should be a means of specifying the fpga devnum somehow in the fitimage? it is plausible that a system could have multiple fpgas, no?
Hmmmm, yeeeeees, system with multiple FPGAs, I like that :-) Probably a similar DT prop in the fitimage indicating where this bitstream goes would do, like loadaddress for kernel, it'd be FPGA devid for bitstream.
--dalon
case fpga_xilinx:desc_xilinx = desc->devdesc;if (img_len >= desc_xilinx->size) {name = "full";bstype = BIT_FULL;} else {name = "partial";bstype = BIT_PARTIAL;}break;default: name = "full";
err = fpga_loadbitstream(devnum, (char *)img_data,img_len, BIT_FULL);if (err)err = fpga_load(devnum, (const void*)img_data,
img_len, BIT_FULL);} else {name = "partial";err = fpga_loadbitstream(devnum, (char *)img_data,img_len, BIT_PARTIAL);if (err)err = fpga_load(devnum, (const void*)img_data,
img_len, BIT_PARTIAL);
bstype = BIT_FULL;}
err = fpga_loadbitstream(devnum, (char *)img_data,img_len, bstype);if (err)err = fpga_load(devnum, (const void *)img_data,img_len, bstype);if (err) return err;
On Sun, 2017-02-19 at 21:49 +0100, Marek Vasut wrote:
On 02/19/2017 09:43 PM, Dalon Westergreen wrote:
On Sun, 2017-02-19 at 21:07 +0100, Marek Vasut wrote:
On 02/19/2017 08:49 PM, Dalon Westergreen wrote:
The implementation of boot_get_fpga only supported one fpga family. This modification allows for any of the fpga devices supported by fpga_load to be used.
Signed-off-by: Dalon Westergreen dwesterg@gmail.com
+CC Xilinx friends :)
common/image.c | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-)
diff --git a/common/image.c b/common/image.c index 0f88984..792d371 100644 --- a/common/image.c +++ b/common/image.c @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images, uint8_t arch, } #if IMAGE_ENABLE_FIT -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) +#if defined(CONFIG_FPGA) int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, uint8_t arch, const ulong *ld_start, ulong * const ld_len) { @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, int err; int devnum = 0; /* TODO support multi fpga platforms */ const fpga_desc * const desc = fpga_get_desc(devnum);
- xilinx_desc *desc_xilinx = desc->devdesc;
- xilinx_desc *desc_xilinx;
- bitstream_type bstype;
/* Check to see if the images struct has a FIT configuration */ if (!genimg_has_config(images)) { @@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, return fit_img_result; }
if (img_len >= desc_xilinx->size) {
switch (desc->devtype) {Do we need the switch statement at all ? We can have full configuration as a default mode of operation and have something like
if (xilinx) { if (partial reconfiguration) { do_special_setup(); } }
I only did the switch stuff b/c i envisioned a need for partial image support for socfpga.
That'd be seriously cool :)
That said, i would suggest, as you mention, moving this to platform specific code and perhaps an indication of the image type in the fitimage.
driver-specific code . It doesn't need to know the imagetype, just that the blob that you passed in is a partial-reconfiguration blob. I never really worked with P/R though, do you need some other metadata for that or is it contained in that P/R bitstream blob already ?
as far as i understand it, it is all in the blob. All that is needed is knowing whether the blob is a full or partial image. X seems to just use the image size to determine this, but that means having a table of all devices and their respective full image size. seems simpler to just specify the image type is partial or not in the fitimage.
But even better would be to move this platform-dependent stuff into drivers/fpga/ or somewhere there. This is common code, so it shouldn't be here in the first place.
My preference would be to only call fpga_load and have the platform
s/platform/driver/
specific stuff figure out what they want to do.
Agreed
My next comment would be that perhaps it is best to add an fpgap type or some such in the fitimage to specify the image is a partial image rather than looking at the image size?
Hmmmm, see my question above. If the driver cannot discern it from the blob, maybe a DT-alike property would work. ie. the image entry would also have a "xlnx,partial-reconfiguration" property or somesuch .
I dont think the property need be fpga vendor specific.
--dalon
Also a consideration is that there should be a means of specifying the fpga devnum somehow in the fitimage? it is plausible that a system could have multiple fpgas, no?
Hmmmm, yeeeeees, system with multiple FPGAs, I like that :-) Probably a similar DT prop in the fitimage indicating where this bitstream goes would do, like loadaddress for kernel, it'd be FPGA devid for bitstream.
--dalon
case fpga_xilinx:desc_xilinx = desc->devdesc;if (img_len >= desc_xilinx->size) {name = "full";bstype = BIT_FULL;} else {name = "partial";bstype = BIT_PARTIAL;}break;default:name = "full";
err = fpga_loadbitstream(devnum, (char*)img_data,
img_len, BIT_FULL);if (err)err = fpga_load(devnum, (const void*)img_data,
img_len, BIT_FULL);} else {name = "partial";err = fpga_loadbitstream(devnum, (char*)img_data,
img_len, BIT_PARTIAL);if (err)err = fpga_load(devnum, (const void*)img_data,
img_len, BIT_PARTIAL);
bstype = BIT_FULL;}
err = fpga_loadbitstream(devnum, (char *)img_data,img_len, bstype);if (err)err = fpga_load(devnum, (const void *)img_data,img_len, bstype);if (err) return err;
On 02/19/2017 09:58 PM, Dalon Westergreen wrote:
On Sun, 2017-02-19 at 21:49 +0100, Marek Vasut wrote:
On 02/19/2017 09:43 PM, Dalon Westergreen wrote:
On Sun, 2017-02-19 at 21:07 +0100, Marek Vasut wrote:
On 02/19/2017 08:49 PM, Dalon Westergreen wrote:
The implementation of boot_get_fpga only supported one fpga family. This modification allows for any of the fpga devices supported by fpga_load to be used.
Signed-off-by: Dalon Westergreen dwesterg@gmail.com
+CC Xilinx friends :)
common/image.c | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-)
diff --git a/common/image.c b/common/image.c index 0f88984..792d371 100644 --- a/common/image.c +++ b/common/image.c @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images, uint8_t arch, }
#if IMAGE_ENABLE_FIT -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) +#if defined(CONFIG_FPGA) int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, uint8_t arch, const ulong *ld_start, ulong * const ld_len) { @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, int err; int devnum = 0; /* TODO support multi fpga platforms */ const fpga_desc * const desc = fpga_get_desc(devnum);
- xilinx_desc *desc_xilinx = desc->devdesc;
xilinx_desc *desc_xilinx;
bitstream_type bstype;
/* Check to see if the images struct has a FIT configuration */ if (!genimg_has_config(images)) {
@@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, return fit_img_result; }
if (img_len >= desc_xilinx->size) {
switch (desc->devtype) {Do we need the switch statement at all ? We can have full configuration as a default mode of operation and have something like
if (xilinx) { if (partial reconfiguration) { do_special_setup(); } }
I only did the switch stuff b/c i envisioned a need for partial image support for socfpga.
That'd be seriously cool :)
That said, i would suggest, as you mention, moving this to platform specific code and perhaps an indication of the image type in the fitimage.
driver-specific code . It doesn't need to know the imagetype, just that the blob that you passed in is a partial-reconfiguration blob. I never really worked with P/R though, do you need some other metadata for that or is it contained in that P/R bitstream blob already ?
as far as i understand it, it is all in the blob. All that is needed is knowing whether the blob is a full or partial image. X seems to just use the image size to determine this, but that means having a table of all devices and their respective full image size. seems simpler to just specify the image type is partial or not in the fitimage.
Can't you extract that info from the RBF though ? But then again, extending the fitImage format for this would be fine IMO.
btw is spelling the X in it's entirety somehow forbidden in A ? :-)
But even better would be to move this platform-dependent stuff into drivers/fpga/ or somewhere there. This is common code, so it shouldn't be here in the first place.
My preference would be to only call fpga_load and have the platform
s/platform/driver/
specific stuff figure out what they want to do.
Agreed
My next comment would be that perhaps it is best to add an fpgap type or some such in the fitimage to specify the image is a partial image rather than looking at the image size?
Hmmmm, see my question above. If the driver cannot discern it from the blob, maybe a DT-alike property would work. ie. the image entry would also have a "xlnx,partial-reconfiguration" property or somesuch .
I dont think the property need be fpga vendor specific.
Indeed.
[...]
On Sun, 2017-02-19 at 22:12 +0100, Marek Vasut wrote:
On 02/19/2017 09:58 PM, Dalon Westergreen wrote:
On Sun, 2017-02-19 at 21:49 +0100, Marek Vasut wrote:
On 02/19/2017 09:43 PM, Dalon Westergreen wrote:
On Sun, 2017-02-19 at 21:07 +0100, Marek Vasut wrote:
On 02/19/2017 08:49 PM, Dalon Westergreen wrote:
The implementation of boot_get_fpga only supported one fpga family. This modification allows for any of the fpga devices supported by fpga_load to be used.
Signed-off-by: Dalon Westergreen dwesterg@gmail.com
+CC Xilinx friends :)
common/image.c | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-)
diff --git a/common/image.c b/common/image.c index 0f88984..792d371 100644 --- a/common/image.c +++ b/common/image.c @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images, uint8_t arch, } #if IMAGE_ENABLE_FIT -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) +#if defined(CONFIG_FPGA) int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, uint8_t arch, const ulong *ld_start, ulong * const ld_len) { @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, int err; int devnum = 0; /* TODO support multi fpga platforms */ const fpga_desc * const desc = fpga_get_desc(devnum);
- xilinx_desc *desc_xilinx = desc->devdesc;
- xilinx_desc *desc_xilinx;
- bitstream_type bstype;
/* Check to see if the images struct has a FIT configuration */ if (!genimg_has_config(images)) { @@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, return fit_img_result; }
if (img_len >= desc_xilinx->size) {
switch (desc->devtype) {Do we need the switch statement at all ? We can have full configuration as a default mode of operation and have something like
if (xilinx) { if (partial reconfiguration) { do_special_setup(); } }
I only did the switch stuff b/c i envisioned a need for partial image support for socfpga.
That'd be seriously cool :)
That said, i would suggest, as you mention, moving this to platform specific code and perhaps an indication of the image type in the fitimage.
driver-specific code . It doesn't need to know the imagetype, just that the blob that you passed in is a partial-reconfiguration blob. I never really worked with P/R though, do you need some other metadata for that or is it contained in that P/R bitstream blob already ?
as far as i understand it, it is all in the blob. All that is needed is knowing whether the blob is a full or partial image. X seems to just use the image size to determine this, but that means having a table of all devices and their respective full image size. seems simpler to just specify the image type is partial or not in the fitimage.
Can't you extract that info from the RBF though ? But then again, extending the fitImage format for this would be fine IMO.
btw is spelling the X in it's entirety somehow forbidden in A ? :-)
Ah, but you forget, we are now I... :)
But even better would be to move this platform-dependent stuff into drivers/fpga/ or somewhere there. This is common code, so it shouldn't be here in the first place.
My preference would be to only call fpga_load and have the platform
s/platform/driver/
specific stuff figure out what they want to do.
Agreed
My next comment would be that perhaps it is best to add an fpgap type or some such in the fitimage to specify the image is a partial image rather than looking at the image size?
Hmmmm, see my question above. If the driver cannot discern it from the blob, maybe a DT-alike property would work. ie. the image entry would also have a "xlnx,partial-reconfiguration" property or somesuch .
I dont think the property need be fpga vendor specific.
Indeed.
[...]
On 02/19/2017 10:21 PM, Dalon Westergreen wrote:
On Sun, 2017-02-19 at 22:12 +0100, Marek Vasut wrote:
On 02/19/2017 09:58 PM, Dalon Westergreen wrote:
On Sun, 2017-02-19 at 21:49 +0100, Marek Vasut wrote:
On 02/19/2017 09:43 PM, Dalon Westergreen wrote:
On Sun, 2017-02-19 at 21:07 +0100, Marek Vasut wrote:
On 02/19/2017 08:49 PM, Dalon Westergreen wrote: > > > > The implementation of boot_get_fpga only supported one fpga family. > This modification allows for any of the fpga devices supported by > fpga_load to be used. > > Signed-off-by: Dalon Westergreen dwesterg@gmail.com
+CC Xilinx friends :)
> > > > --- > common/image.c | 37 ++++++++++++++++++++++--------------- > 1 file changed, 22 insertions(+), 15 deletions(-) > > diff --git a/common/image.c b/common/image.c > index 0f88984..792d371 100644 > --- a/common/image.c > +++ b/common/image.c > @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images, > uint8_t > arch, > } > > #if IMAGE_ENABLE_FIT > -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) > +#if defined(CONFIG_FPGA) > int boot_get_fpga(int argc, char * const argv[], bootm_headers_t > *images, > uint8_t arch, const ulong *ld_start, ulong * > const > ld_len) > { > @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const > argv[], > bootm_headers_t *images, > int err; > int devnum = 0; /* TODO support multi fpga platforms */ > const fpga_desc * const desc = fpga_get_desc(devnum); > - xilinx_desc *desc_xilinx = desc->devdesc; > + xilinx_desc *desc_xilinx; > + bitstream_type bstype; > > /* Check to see if the images struct has a FIT > configuration */ > if (!genimg_has_config(images)) { > @@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const > argv[], > bootm_headers_t *images, > return fit_img_result; > } > > - if (img_len >= desc_xilinx->size) { > + switch (desc->devtype) {
Do we need the switch statement at all ? We can have full configuration as a default mode of operation and have something like
if (xilinx) { if (partial reconfiguration) { do_special_setup(); } }
I only did the switch stuff b/c i envisioned a need for partial image support for socfpga.
That'd be seriously cool :)
That said, i would suggest, as you mention, moving this to platform specific code and perhaps an indication of the image type in the fitimage.
driver-specific code . It doesn't need to know the imagetype, just that the blob that you passed in is a partial-reconfiguration blob. I never really worked with P/R though, do you need some other metadata for that or is it contained in that P/R bitstream blob already ?
as far as i understand it, it is all in the blob. All that is needed is knowing whether the blob is a full or partial image. X seems to just use the image size to determine this, but that means having a table of all devices and their respective full image size. seems simpler to just specify the image type is partial or not in the fitimage.
Can't you extract that info from the RBF though ? But then again, extending the fitImage format for this would be fine IMO.
btw is spelling the X in it's entirety somehow forbidden in A ? :-)
Ah, but you forget, we are now I... :)
So it's now A+I ... 愛 [1] ? :-) [1] http://jisho.org/search/%E6%84%9B
Looking forward to V2 of this , we seem to have some good stuff coming up.
On 19.2.2017 22:26, Marek Vasut wrote:
On 02/19/2017 10:21 PM, Dalon Westergreen wrote:
On Sun, 2017-02-19 at 22:12 +0100, Marek Vasut wrote:
On 02/19/2017 09:58 PM, Dalon Westergreen wrote:
On Sun, 2017-02-19 at 21:49 +0100, Marek Vasut wrote:
On 02/19/2017 09:43 PM, Dalon Westergreen wrote:
On Sun, 2017-02-19 at 21:07 +0100, Marek Vasut wrote: > > > On 02/19/2017 08:49 PM, Dalon Westergreen wrote: >> >> >> >> The implementation of boot_get_fpga only supported one fpga family. >> This modification allows for any of the fpga devices supported by >> fpga_load to be used. >> >> Signed-off-by: Dalon Westergreen dwesterg@gmail.com > > +CC Xilinx friends :) > >> >> >> >> --- >> common/image.c | 37 ++++++++++++++++++++++--------------- >> 1 file changed, 22 insertions(+), 15 deletions(-) >> >> diff --git a/common/image.c b/common/image.c >> index 0f88984..792d371 100644 >> --- a/common/image.c >> +++ b/common/image.c >> @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images, >> uint8_t >> arch, >> } >> >> #if IMAGE_ENABLE_FIT >> -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) >> +#if defined(CONFIG_FPGA) >> int boot_get_fpga(int argc, char * const argv[], bootm_headers_t >> *images, >> uint8_t arch, const ulong *ld_start, ulong * >> const >> ld_len) >> { >> @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const >> argv[], >> bootm_headers_t *images, >> int err; >> int devnum = 0; /* TODO support multi fpga platforms */ >> const fpga_desc * const desc = fpga_get_desc(devnum); >> - xilinx_desc *desc_xilinx = desc->devdesc; >> + xilinx_desc *desc_xilinx; >> + bitstream_type bstype; >> >> /* Check to see if the images struct has a FIT >> configuration */ >> if (!genimg_has_config(images)) { >> @@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const >> argv[], >> bootm_headers_t *images, >> return fit_img_result; >> } >> >> - if (img_len >= desc_xilinx->size) { >> + switch (desc->devtype) { > > Do we need the switch statement at all ? We can have full > configuration > as a default mode of operation and have something like > > if (xilinx) { > if (partial reconfiguration) { > do_special_setup(); > } > }
I only did the switch stuff b/c i envisioned a need for partial image support for socfpga.
That'd be seriously cool :)
That said, i would suggest, as you mention, moving this to platform specific code and perhaps an indication of the image type in the fitimage.
driver-specific code . It doesn't need to know the imagetype, just that the blob that you passed in is a partial-reconfiguration blob. I never really worked with P/R though, do you need some other metadata for that or is it contained in that P/R bitstream blob already ?
as far as i understand it, it is all in the blob. All that is needed is knowing whether the blob is a full or partial image. X seems to just use the image size to determine this, but that means having a table of all devices and their respective full image size. seems simpler to just specify the image type is partial or not in the fitimage.
Can't you extract that info from the RBF though ? But then again, extending the fitImage format for this would be fine IMO.
btw is spelling the X in it's entirety somehow forbidden in A ? :-)
Ah, but you forget, we are now I... :)
So it's now A+I ... 愛 [1] ? :-) [1] http://jisho.org/search/%E6%84%9B
Looking forward to V2 of this , we seem to have some good stuff coming up.
I can only have some thoughts about it but if you work for I you should use that I email. The same in A case and I do something for X that's why I use X email.
Thanks, Michal
On Mon, 2017-02-20 at 10:24 +0100, Michal Simek wrote:
On 19.2.2017 22:26, Marek Vasut wrote:
On 02/19/2017 10:21 PM, Dalon Westergreen wrote:
On Sun, 2017-02-19 at 22:12 +0100, Marek Vasut wrote:
On 02/19/2017 09:58 PM, Dalon Westergreen wrote:
On Sun, 2017-02-19 at 21:49 +0100, Marek Vasut wrote:
On 02/19/2017 09:43 PM, Dalon Westergreen wrote: > > > > On Sun, 2017-02-19 at 21:07 +0100, Marek Vasut wrote: > > > > > > > > On 02/19/2017 08:49 PM, Dalon Westergreen wrote: > > > > > > > > > > > > > > > The implementation of boot_get_fpga only supported one fpga > > > family. > > > This modification allows for any of the fpga devices supported > > > by > > > fpga_load to be used. > > > > > > Signed-off-by: Dalon Westergreen dwesterg@gmail.com > > > > +CC Xilinx friends :) > > > > > > > > > > > > > > > > > --- > > > common/image.c | 37 ++++++++++++++++++++++--------------- > > > 1 file changed, 22 insertions(+), 15 deletions(-) > > > > > > diff --git a/common/image.c b/common/image.c > > > index 0f88984..792d371 100644 > > > --- a/common/image.c > > > +++ b/common/image.c > > > @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t > > > *images, > > > uint8_t > > > arch, > > > } > > > > > > #if IMAGE_ENABLE_FIT > > > -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) > > > +#if defined(CONFIG_FPGA) > > > int boot_get_fpga(int argc, char * const argv[], > > > bootm_headers_t > > > *images, > > > uint8_t arch, const ulong *ld_start, ulong > > > * > > > const > > > ld_len) > > > { > > > @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const > > > argv[], > > > bootm_headers_t *images, > > > int err; > > > int devnum = 0; /* TODO support multi fpga platforms > > > */ > > > const fpga_desc * const desc = fpga_get_desc(devnum); > > > - xilinx_desc *desc_xilinx = desc->devdesc; > > > + xilinx_desc *desc_xilinx; > > > + bitstream_type bstype; > > > > > > /* Check to see if the images struct has a FIT > > > configuration */ > > > if (!genimg_has_config(images)) { > > > @@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * > > > const > > > argv[], > > > bootm_headers_t *images, > > > return fit_img_result; > > > } > > > > > > - if (img_len >= desc_xilinx->size) { > > > + switch (desc->devtype) { > > > > Do we need the switch statement at all ? We can have full > > configuration > > as a default mode of operation and have something like > > > > if (xilinx) { > > if (partial reconfiguration) { > > do_special_setup(); > > } > > } > > I only did the switch stuff b/c i envisioned a need for partial > image > support for socfpga.
That'd be seriously cool :)
> > > > That said, i would suggest, as you mention, moving > this to platform specific code and perhaps an indication of the > image > type > in the fitimage.
driver-specific code . It doesn't need to know the imagetype, just that the blob that you passed in is a partial-reconfiguration blob. I never really worked with P/R though, do you need some other metadata for that or is it contained in that P/R bitstream blob already ?
as far as i understand it, it is all in the blob. All that is needed is knowing whether the blob is a full or partial image. X seems to just use the image size to determine this, but that means having a table of all devices and their respective full image size. seems simpler to just specify the image type is partial or not in the fitimage.
Can't you extract that info from the RBF though ? But then again, extending the fitImage format for this would be fine IMO.
btw is spelling the X in it's entirety somehow forbidden in A ? :-)
Ah, but you forget, we are now I... :)
So it's now A+I ... 愛 [1] ? :-) [1] http://jisho.org/search/%E6%84%9B
Looking forward to V2 of this , we seem to have some good stuff coming up.
I can only have some thoughts about it but if you work for I you should use that I email. The same in A case and I do something for X that's why I use X email.
Thanks for the feedback, although i work for intel, i am doing this purely for interest. As such, and since working on uboot is absolutely not in my job function, i am required to use a personal email.
--dalon
Thanks, Michal
On 19.2.2017 21:58, Dalon Westergreen wrote:
On Sun, 2017-02-19 at 21:49 +0100, Marek Vasut wrote:
On 02/19/2017 09:43 PM, Dalon Westergreen wrote:
On Sun, 2017-02-19 at 21:07 +0100, Marek Vasut wrote:
On 02/19/2017 08:49 PM, Dalon Westergreen wrote:
The implementation of boot_get_fpga only supported one fpga family. This modification allows for any of the fpga devices supported by fpga_load to be used.
Signed-off-by: Dalon Westergreen dwesterg@gmail.com
+CC Xilinx friends :)
common/image.c | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-)
diff --git a/common/image.c b/common/image.c index 0f88984..792d371 100644 --- a/common/image.c +++ b/common/image.c @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images, uint8_t arch, }
#if IMAGE_ENABLE_FIT -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) +#if defined(CONFIG_FPGA) int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, uint8_t arch, const ulong *ld_start, ulong * const ld_len) { @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, int err; int devnum = 0; /* TODO support multi fpga platforms */ const fpga_desc * const desc = fpga_get_desc(devnum);
- xilinx_desc *desc_xilinx = desc->devdesc;
xilinx_desc *desc_xilinx;
bitstream_type bstype;
/* Check to see if the images struct has a FIT configuration */ if (!genimg_has_config(images)) {
@@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, return fit_img_result; }
if (img_len >= desc_xilinx->size) {
switch (desc->devtype) {Do we need the switch statement at all ? We can have full configuration as a default mode of operation and have something like
if (xilinx) { if (partial reconfiguration) { do_special_setup(); } }
I only did the switch stuff b/c i envisioned a need for partial image support for socfpga.
That'd be seriously cool :)
That said, i would suggest, as you mention, moving this to platform specific code and perhaps an indication of the image type in the fitimage.
driver-specific code . It doesn't need to know the imagetype, just that the blob that you passed in is a partial-reconfiguration blob. I never really worked with P/R though, do you need some other metadata for that or is it contained in that P/R bitstream blob already ?
as far as i understand it, it is all in the blob. All that is needed is knowing whether the blob is a full or partial image. X seems to just use the image size to determine this, but that means having a table of all devices and their respective full image size. seems simpler to just specify the image type is partial or not in the fitimage.
We did that for zynq when we did that for the first time. But not for zynqmp. Zynq is maybe still using it but it shouldn't now. It is not 100% reliable way. Definitely having DT property is the best option because you can add sort of "nop" which extend bitstream size and does nothing which breaks that checking.
For full u-boot there is loadb, loadbp, load and loadp to distinguish it.
Thanks, Michal
Thanks, Michal
On Mon, 2017-02-20 at 10:22 +0100, Michal Simek wrote:
On 19.2.2017 21:58, Dalon Westergreen wrote:
On Sun, 2017-02-19 at 21:49 +0100, Marek Vasut wrote:
On 02/19/2017 09:43 PM, Dalon Westergreen wrote:
On Sun, 2017-02-19 at 21:07 +0100, Marek Vasut wrote:
On 02/19/2017 08:49 PM, Dalon Westergreen wrote:
The implementation of boot_get_fpga only supported one fpga family. This modification allows for any of the fpga devices supported by fpga_load to be used.
Signed-off-by: Dalon Westergreen dwesterg@gmail.com
+CC Xilinx friends :)
common/image.c | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-)
diff --git a/common/image.c b/common/image.c index 0f88984..792d371 100644 --- a/common/image.c +++ b/common/image.c @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images, uint8_t arch, } #if IMAGE_ENABLE_FIT -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) +#if defined(CONFIG_FPGA) int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, uint8_t arch, const ulong *ld_start, ulong * const ld_len) { @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, int err; int devnum = 0; /* TODO support multi fpga platforms */ const fpga_desc * const desc = fpga_get_desc(devnum);
- xilinx_desc *desc_xilinx = desc->devdesc;
- xilinx_desc *desc_xilinx;
- bitstream_type bstype;
/* Check to see if the images struct has a FIT configuration */ if (!genimg_has_config(images)) { @@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, return fit_img_result; }
if (img_len >= desc_xilinx->size) {
switch (desc->devtype) {Do we need the switch statement at all ? We can have full configuration as a default mode of operation and have something like
if (xilinx) { if (partial reconfiguration) { do_special_setup(); } }
I only did the switch stuff b/c i envisioned a need for partial image support for socfpga.
That'd be seriously cool :)
That said, i would suggest, as you mention, moving this to platform specific code and perhaps an indication of the image type in the fitimage.
driver-specific code . It doesn't need to know the imagetype, just that the blob that you passed in is a partial-reconfiguration blob. I never really worked with P/R though, do you need some other metadata for that or is it contained in that P/R bitstream blob already ?
as far as i understand it, it is all in the blob. All that is needed is knowing whether the blob is a full or partial image. X seems to just use the image size to determine this, but that means having a table of all devices and their respective full image size. seems simpler to just specify the image type is partial or not in the fitimage.
We did that for zynq when we did that for the first time. But not for zynqmp. Zynq is maybe still using it but it shouldn't now. It is not 100% reliable way. Definitely having DT property is the best option because you can add sort of "nop" which extend bitstream size and does nothing which breaks that checking.
For full u-boot there is loadb, loadbp, load and loadp to distinguish it.
That brings up an interesting point, right now the fpga_loadbitstream doesn't follow the same method as fpga_load for allowing multiple FPGA types to be supported simultaneously. Would it not be prudent to move in that direction? I believe only xilinx implements this right now.
--dalon
Thanks, Michal
Thanks, Michal
On 20.2.2017 15:30, Dalon Westergreen wrote:
On Mon, 2017-02-20 at 10:22 +0100, Michal Simek wrote:
On 19.2.2017 21:58, Dalon Westergreen wrote:
On Sun, 2017-02-19 at 21:49 +0100, Marek Vasut wrote:
On 02/19/2017 09:43 PM, Dalon Westergreen wrote:
On Sun, 2017-02-19 at 21:07 +0100, Marek Vasut wrote:
On 02/19/2017 08:49 PM, Dalon Westergreen wrote: > > > > The implementation of boot_get_fpga only supported one fpga family. > This modification allows for any of the fpga devices supported by > fpga_load to be used. > > Signed-off-by: Dalon Westergreen dwesterg@gmail.com
+CC Xilinx friends :)
> > > > --- > common/image.c | 37 ++++++++++++++++++++++--------------- > 1 file changed, 22 insertions(+), 15 deletions(-) > > diff --git a/common/image.c b/common/image.c > index 0f88984..792d371 100644 > --- a/common/image.c > +++ b/common/image.c > @@ -1306,7 +1306,7 @@ int boot_get_setup(bootm_headers_t *images, > uint8_t > arch, > } > > #if IMAGE_ENABLE_FIT > -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) > +#if defined(CONFIG_FPGA) > int boot_get_fpga(int argc, char * const argv[], bootm_headers_t > *images, > uint8_t arch, const ulong *ld_start, ulong * > const > ld_len) > { > @@ -1318,7 +1318,8 @@ int boot_get_fpga(int argc, char * const > argv[], > bootm_headers_t *images, > int err; > int devnum = 0; /* TODO support multi fpga platforms */ > const fpga_desc * const desc = fpga_get_desc(devnum); > - xilinx_desc *desc_xilinx = desc->devdesc; > + xilinx_desc *desc_xilinx; > + bitstream_type bstype; > > /* Check to see if the images struct has a FIT > configuration */ > if (!genimg_has_config(images)) { > @@ -1365,22 +1366,28 @@ int boot_get_fpga(int argc, char * const > argv[], > bootm_headers_t *images, > return fit_img_result; > } > > - if (img_len >= desc_xilinx->size) { > + switch (desc->devtype) {
Do we need the switch statement at all ? We can have full configuration as a default mode of operation and have something like
if (xilinx) { if (partial reconfiguration) { do_special_setup(); } }
I only did the switch stuff b/c i envisioned a need for partial image support for socfpga.
That'd be seriously cool :)
That said, i would suggest, as you mention, moving this to platform specific code and perhaps an indication of the image type in the fitimage.
driver-specific code . It doesn't need to know the imagetype, just that the blob that you passed in is a partial-reconfiguration blob. I never really worked with P/R though, do you need some other metadata for that or is it contained in that P/R bitstream blob already ?
as far as i understand it, it is all in the blob. All that is needed is knowing whether the blob is a full or partial image. X seems to just use the image size to determine this, but that means having a table of all devices and their respective full image size. seems simpler to just specify the image type is partial or not in the fitimage.
We did that for zynq when we did that for the first time. But not for zynqmp. Zynq is maybe still using it but it shouldn't now. It is not 100% reliable way. Definitely having DT property is the best option because you can add sort of "nop" which extend bitstream size and does nothing which breaks that checking.
For full u-boot there is loadb, loadbp, load and loadp to distinguish it.
That brings up an interesting point, right now the fpga_loadbitstream doesn't follow the same method as fpga_load for allowing multiple FPGA types to be supported simultaneously. Would it not be prudent to move in that direction? I believe only xilinx implements this right now.
loadbitstream is there for a long time which is in general just header bitstream header parser written in pretty bad way. Right now we are adding support for encrypted and authenticated images and for that we are adding fpga loads command where you specify by flag if you want to pass encrypted or authenticated image and keys.
There is also loadmk which should be improved a lot exactly by what you are trying to do. It means we should wrap bitstreams with flags which says what it is inside. On the other hand having separate commands it is easy for testing.
Definitely I am open to improve this subsystem to make it more flexible.
Thanks, Michal
Hi all,
On Mon, Feb 20, 2017 at 6:59 AM, Michal Simek michal.simek@xilinx.com wrote:
Definitely I am open to improve this subsystem to make it more flexible.
Over at linux-fpga ([1]) we're currently discussing the idea of coming up with a header format (vendor neutral) that we stitch onto the fpga image to allow the fpga-mgr to derive certain features of the image from the image itself (i.e. is the image encrypted, compressed, ...).
We're trying to come up with an extensible format that would allow us to add stuff like encryption keys etc.
Cheers,
Moritz
On 02/20/2017 06:35 PM, Moritz Fischer wrote:
Hi all,
On Mon, Feb 20, 2017 at 6:59 AM, Michal Simek michal.simek@xilinx.com wrote:
Definitely I am open to improve this subsystem to make it more flexible.
Over at linux-fpga ([1]) we're currently discussing the idea of coming up with a header format (vendor neutral) that we stitch onto the fpga image to allow the fpga-mgr to derive certain features of the image from the image itself (i.e. is the image encrypted, compressed, ...).
Can you CC me on that discussion ? I think it'd be for the best to use DT for the header format, it's documented and vendor-neutral already.
We're trying to come up with an extensible format that would allow us to add stuff like encryption keys etc.
Nothing like a NIH format :)
Cheers,
Moritz
This adds support for fpga configuration data in fitimages for any fpga device supported by fpga_load. At this point fitimages only support configuration of fpga images for fpga devnum 0.
Signed-off-by: Dalon Westergreen dwesterg@gmail.com --- common/bootm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/bootm.c b/common/bootm.c index b2c0912..4a4b47c 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -248,7 +248,7 @@ int bootm_find_images(int flag, int argc, char * const argv[]) #endif
#if IMAGE_ENABLE_FIT -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) +#if defined(CONFIG_FPGA) /* find bitstreams */ ret = boot_get_fpga(argc, argv, &images, IH_ARCH_DEFAULT, NULL, NULL);
participants (4)
-
Dalon Westergreen -
Marek Vasut -
Michal Simek -
Moritz Fischer