| Message ID | 20211005111324.19749-3-jorge@foundries.io |
|---|---|
| State | Superseded |
| Delegated to: | Michal Simek |
| Headers | show |
| Series | [RFC,1/2] fpga_load: pass compatible string | expand |
On 10/5/21 1:13 PM, Jorge Ramirez-Ortiz wrote: > Add new compatible string u-boot,zynqmp-fpga-ddrauth to handle this > use case. > > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> > --- > drivers/fpga/xilinx.c | 29 +++++++++++++++++++++++++---- > drivers/fpga/zynqmppl.c | 4 ++-- > include/xilinx.h | 2 +- > 3 files changed, 28 insertions(+), 7 deletions(-) > > diff --git a/drivers/fpga/xilinx.c b/drivers/fpga/xilinx.c > index cbebefb55f..c8035105e2 100644 > --- a/drivers/fpga/xilinx.c > +++ b/drivers/fpga/xilinx.c > @@ -135,23 +135,44 @@ int fpga_loadbitstream(int devnum, char *fpgadata, size_t size, > dataptr += 4; > printf(" bytes in bitstream = %d\n", swapsize); > > - return fpga_load(devnum, dataptr, swapsize, bstype); > + return fpga_load(devnum, dataptr, swapsize, bstype, NULL); > } > > int xilinx_load(xilinx_desc *desc, const void *buf, size_t bsize, > - bitstream_type bstype) > + bitstream_type bstype, const char *compatible) > { > + struct fpga_secure_info info = { 0 }; > + > if (!xilinx_validate (desc, (char *)__FUNCTION__)) { > printf ("%s: Invalid device descriptor\n", __FUNCTION__); > return FPGA_FAIL; > } > > - if (!desc->operations || !desc->operations->load) { > + if (!desc->operations) { > printf("%s: Missing load operation\n", __func__); > return FPGA_FAIL; > } > > - return desc->operations->load(desc, buf, bsize, bstype); > + if (!compatible || !strcmp(compatible, "u-boot,fpga-legacy")) { > + if (!desc->operations->load) { > + printf("%s: Missing load operation\n", __func__); > + return FPGA_FAIL; > + } > + return desc->operations->load(desc, buf, bsize, bstype); > + } > + > + if (!strcmp(compatible, "u-boot,zynqmp-fpga-ddrauth")) { > + if (!desc->operations->loads) { > + printf("%s: Missing load operation\n", __func__); > + return FPGA_FAIL; > + } > + /* DDR authentication */ > + info.authflag = 1; > + return desc->operations->loads(desc, buf, bsize, &info); > + } I am not sure about this solution. First of all it forces SPL to have additional code which just extending size. It means there must be a way to enable/disable it. The next thing is that you have zynqmp string in generic xilinx file which doesn't look right. It would be better to deal with image types directly in driver which is capable to handle it. It means record fit image compatible string in the driver with hooks/flags and determined what should be called. And the same style should work for SPL and also for U-Boot proper. Thanks, Michal
On 05/10/21, Michal Simek wrote: > > > On 10/5/21 1:13 PM, Jorge Ramirez-Ortiz wrote: > > Add new compatible string u-boot,zynqmp-fpga-ddrauth to handle this > > use case. > > > > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> > > --- > > drivers/fpga/xilinx.c | 29 +++++++++++++++++++++++++---- > > drivers/fpga/zynqmppl.c | 4 ++-- > > include/xilinx.h | 2 +- > > 3 files changed, 28 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/fpga/xilinx.c b/drivers/fpga/xilinx.c > > index cbebefb55f..c8035105e2 100644 > > --- a/drivers/fpga/xilinx.c > > +++ b/drivers/fpga/xilinx.c > > @@ -135,23 +135,44 @@ int fpga_loadbitstream(int devnum, char *fpgadata, size_t size, > > dataptr += 4; > > printf(" bytes in bitstream = %d\n", swapsize); > > > > - return fpga_load(devnum, dataptr, swapsize, bstype); > > + return fpga_load(devnum, dataptr, swapsize, bstype, NULL); > > } > > > > int xilinx_load(xilinx_desc *desc, const void *buf, size_t bsize, > > - bitstream_type bstype) > > + bitstream_type bstype, const char *compatible) > > { > > + struct fpga_secure_info info = { 0 }; > > + > > if (!xilinx_validate (desc, (char *)__FUNCTION__)) { > > printf ("%s: Invalid device descriptor\n", __FUNCTION__); > > return FPGA_FAIL; > > } > > > > - if (!desc->operations || !desc->operations->load) { > > + if (!desc->operations) { > > printf("%s: Missing load operation\n", __func__); > > return FPGA_FAIL; > > } > > > > - return desc->operations->load(desc, buf, bsize, bstype); > > + if (!compatible || !strcmp(compatible, "u-boot,fpga-legacy")) { > > + if (!desc->operations->load) { > > + printf("%s: Missing load operation\n", __func__); > > + return FPGA_FAIL; > > + } > > + return desc->operations->load(desc, buf, bsize, bstype); > > + } > > + > > + if (!strcmp(compatible, "u-boot,zynqmp-fpga-ddrauth")) { > > + if (!desc->operations->loads) { > > + printf("%s: Missing load operation\n", __func__); > > + return FPGA_FAIL; > > + } > > + /* DDR authentication */ > > + info.authflag = 1; > > + return desc->operations->loads(desc, buf, bsize, &info); > > + } > > I am not sure about this solution. First of all it forces SPL to have > additional code which just extending size. It means there must be a way > to enable/disable it. that can be contained of course...so not really an issue (ie compile out either fpga_load or fpga_loads depending on needs). > > The next thing is that you have zynqmp string in generic xilinx file > which doesn't look right. It would be better to deal with image types > directly in driver which is capable to handle it. sure that is easy..but the idea is the same > It means record fit image compatible string in the driver with > hooks/flags and determined what should be called. what about the compatible names? will you define those? or should I do it? For our use case, we need DDR authentication so that must be defined. I can add others and someone else can add the support needed? > > And the same style should work for SPL and also for U-Boot proper. sure. thanks for the quick response. > > Thanks, > Michal
On 10/5/21 4:03 PM, Jorge Ramirez-Ortiz, Foundries wrote: > On 05/10/21, Michal Simek wrote: >> >> >> On 10/5/21 1:13 PM, Jorge Ramirez-Ortiz wrote: >>> Add new compatible string u-boot,zynqmp-fpga-ddrauth to handle this >>> use case. >>> >>> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> >>> --- >>> drivers/fpga/xilinx.c | 29 +++++++++++++++++++++++++---- >>> drivers/fpga/zynqmppl.c | 4 ++-- >>> include/xilinx.h | 2 +- >>> 3 files changed, 28 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/fpga/xilinx.c b/drivers/fpga/xilinx.c >>> index cbebefb55f..c8035105e2 100644 >>> --- a/drivers/fpga/xilinx.c >>> +++ b/drivers/fpga/xilinx.c >>> @@ -135,23 +135,44 @@ int fpga_loadbitstream(int devnum, char *fpgadata, size_t size, >>> dataptr += 4; >>> printf(" bytes in bitstream = %d\n", swapsize); >>> >>> - return fpga_load(devnum, dataptr, swapsize, bstype); >>> + return fpga_load(devnum, dataptr, swapsize, bstype, NULL); >>> } >>> >>> int xilinx_load(xilinx_desc *desc, const void *buf, size_t bsize, >>> - bitstream_type bstype) >>> + bitstream_type bstype, const char *compatible) >>> { >>> + struct fpga_secure_info info = { 0 }; >>> + >>> if (!xilinx_validate (desc, (char *)__FUNCTION__)) { >>> printf ("%s: Invalid device descriptor\n", __FUNCTION__); >>> return FPGA_FAIL; >>> } >>> >>> - if (!desc->operations || !desc->operations->load) { >>> + if (!desc->operations) { >>> printf("%s: Missing load operation\n", __func__); >>> return FPGA_FAIL; >>> } >>> >>> - return desc->operations->load(desc, buf, bsize, bstype); >>> + if (!compatible || !strcmp(compatible, "u-boot,fpga-legacy")) { >>> + if (!desc->operations->load) { >>> + printf("%s: Missing load operation\n", __func__); >>> + return FPGA_FAIL; >>> + } >>> + return desc->operations->load(desc, buf, bsize, bstype); >>> + } >>> + >>> + if (!strcmp(compatible, "u-boot,zynqmp-fpga-ddrauth")) { >>> + if (!desc->operations->loads) { >>> + printf("%s: Missing load operation\n", __func__); >>> + return FPGA_FAIL; >>> + } >>> + /* DDR authentication */ >>> + info.authflag = 1; >>> + return desc->operations->loads(desc, buf, bsize, &info); >>> + } >> >> I am not sure about this solution. First of all it forces SPL to have >> additional code which just extending size. It means there must be a way >> to enable/disable it. > > that can be contained of course...so not really an issue (ie compile > out either fpga_load or fpga_loads depending on needs). > >> >> The next thing is that you have zynqmp string in generic xilinx file >> which doesn't look right. It would be better to deal with image types >> directly in driver which is capable to handle it. > > sure that is easy..but the idea is the same We just need to find out how this should be properly propagated and stored. > >> It means record fit image compatible string in the driver with >> hooks/flags and determined what should be called. > > what about the compatible names? will you define those? or should I do > it? Just define it and let's see if this fits. > > For our use case, we need DDR authentication so that must be > defined. I can add others and someone else can add the support needed? > Sure. None is asking you to support everything. Just add support for regular bitstreams and your case. And adding support for something else will be just +1 if interface is right. Thanks, Michal
diff --git a/drivers/fpga/xilinx.c b/drivers/fpga/xilinx.c index cbebefb55f..c8035105e2 100644 --- a/drivers/fpga/xilinx.c +++ b/drivers/fpga/xilinx.c @@ -135,23 +135,44 @@ int fpga_loadbitstream(int devnum, char *fpgadata, size_t size, dataptr += 4; printf(" bytes in bitstream = %d\n", swapsize); - return fpga_load(devnum, dataptr, swapsize, bstype); + return fpga_load(devnum, dataptr, swapsize, bstype, NULL); } int xilinx_load(xilinx_desc *desc, const void *buf, size_t bsize, - bitstream_type bstype) + bitstream_type bstype, const char *compatible) { + struct fpga_secure_info info = { 0 }; + if (!xilinx_validate (desc, (char *)__FUNCTION__)) { printf ("%s: Invalid device descriptor\n", __FUNCTION__); return FPGA_FAIL; } - if (!desc->operations || !desc->operations->load) { + if (!desc->operations) { printf("%s: Missing load operation\n", __func__); return FPGA_FAIL; } - return desc->operations->load(desc, buf, bsize, bstype); + if (!compatible || !strcmp(compatible, "u-boot,fpga-legacy")) { + if (!desc->operations->load) { + printf("%s: Missing load operation\n", __func__); + return FPGA_FAIL; + } + return desc->operations->load(desc, buf, bsize, bstype); + } + + if (!strcmp(compatible, "u-boot,zynqmp-fpga-ddrauth")) { + if (!desc->operations->loads) { + printf("%s: Missing load operation\n", __func__); + return FPGA_FAIL; + } + /* DDR authentication */ + info.authflag = 1; + return desc->operations->loads(desc, buf, bsize, &info); + } + + printf("%s: compatible support not implemented\n", __func__); + return FPGA_FAIL; } #if defined(CONFIG_CMD_FPGA_LOADFS) diff --git a/drivers/fpga/zynqmppl.c b/drivers/fpga/zynqmppl.c index 6b394869db..65a9412123 100644 --- a/drivers/fpga/zynqmppl.c +++ b/drivers/fpga/zynqmppl.c @@ -245,7 +245,7 @@ static int zynqmp_load(xilinx_desc *desc, const void *buf, size_t bsize, return ret; } -#if defined(CONFIG_CMD_FPGA_LOAD_SECURE) && !defined(CONFIG_SPL_BUILD) +#if defined(CONFIG_CMD_FPGA_LOAD_SECURE) static int zynqmp_loads(xilinx_desc *desc, const void *buf, size_t bsize, struct fpga_secure_info *fpga_sec_info) { @@ -306,7 +306,7 @@ static int zynqmp_pcap_info(xilinx_desc *desc) struct xilinx_fpga_op zynqmp_op = { .load = zynqmp_load, -#if defined(CONFIG_CMD_FPGA_LOAD_SECURE) && !defined(CONFIG_SPL_BUILD) +#if defined(CONFIG_CMD_FPGA_LOAD_SECURE) .loads = zynqmp_loads, #endif .info = zynqmp_pcap_info, diff --git a/include/xilinx.h b/include/xilinx.h index ab4537becf..ae78009e6b 100644 --- a/include/xilinx.h +++ b/include/xilinx.h @@ -59,7 +59,7 @@ struct xilinx_fpga_op { /* Generic Xilinx Functions *********************************************************************/ int xilinx_load(xilinx_desc *desc, const void *image, size_t size, - bitstream_type bstype); + bitstream_type bstype, const char *compatible); int xilinx_dump(xilinx_desc *desc, const void *buf, size_t bsize); int xilinx_info(xilinx_desc *desc); int xilinx_loadfs(xilinx_desc *desc, const void *buf, size_t bsize,
Add new compatible string u-boot,zynqmp-fpga-ddrauth to handle this use case. Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io> --- drivers/fpga/xilinx.c | 29 +++++++++++++++++++++++++---- drivers/fpga/zynqmppl.c | 4 ++-- include/xilinx.h | 2 +- 3 files changed, 28 insertions(+), 7 deletions(-)