<div dir="ltr">Sorry for not thinking this through, but I think I have a more portable solution for handling the included DTB:<div>1. In bsps/shared/start/bsp-fdt.c, include the header as you had originally proposed.</div><div>2. Also bsps/shared/start/bsp-fdt.c, change bsp_fdt_get to return either &bsp_fdt_blob[0] for the u-boot option, or just system_dtb for the DTB option.</div><div>3. in bsps/riscv/shared/start.S - no change is needed for the fdt copy. This is only needed if the device tree is loaded by u-boot and needs to be copied. No need to call it for the embedded DTB.</div><div>To make this work, you would need to have the following in config.ini for the polarfire (and eventually k210) bsp variant:</div><div>BSP_START_COPY_FDT_FROM_U_BOOT=False<br>BSP_DTB_IS_SUPPORTED=True<br></div><div>I'm not sure if the BSP_DTB_IS_SUPPORTED option should be moved to the shared section of the specs, but I think it might - in this case the default might be False.</div><div>Same with the BSP_DTB_HEADER_PATH option - but I'm not sure what a good default would be for that.</div><div><br></div><div>I think this would set up the microblaze_fpga to use the same shared code and options, and we could eliminate the bsps/microblaze/microblaze_fpga/fdt/bsp_fdt.c file later.</div><div><br></div><div>Below are the diffs for bsps/shared/start/bsp-fdt.c that is working on my k210 board. Again, sorry for any confusion, what I have now is just a slight change to your original proposal. (changing bsp_fdt_get instead of bsp_fdt_copy).</div><div>Thanks,</div><div>Alan</div><div>diff --git a/bsps/shared/start/bsp-fdt.c b/bsps/shared/start/bsp-fdt.c<br>index 75a1ea41c9..6e2a0b82db 100644<br>--- a/bsps/shared/start/bsp-fdt.c<br>+++ b/bsps/shared/start/bsp-fdt.c<br>@@ -36,6 +36,10 @@<br> #warning "BSP FDT support indication not defined"<br> #endif<br><br>+#ifdef BSP_DTB_IS_SUPPORTED<br>+ #include BSP_DTB_HEADER_PATH<br>+#endif<br>+<br> #ifndef BSP_FDT_BLOB_SIZE_MAX<br> #define BSP_FDT_BLOB_SIZE_MAX 0<br> #endif<br>@@ -76,5 +80,9 @@ void bsp_fdt_copy(const void *src)<br><br> const void *bsp_fdt_get(void)<br> {<br>+#if BSP_DTB_IS_SUPPORTED<br>+ return system_dtb;<br>+#else<br> return &bsp_fdt_blob[0];<br>+#endif<br> }<br></div><div><br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Sep 14, 2022 at 9:16 AM Alan Cudmore <<a href="mailto:alan.cudmore@gmail.com">alan.cudmore@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>Hi Padmarao,</div><div>Please see my inline comments below</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Sep 14, 2022 at 12:51 AM <<a href="mailto:Padmarao.Begari@microchip.com" target="_blank">Padmarao.Begari@microchip.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Alan,<br>
<br>
> On Tue, 2022-09-13 at 23:57 -0400, Alan Cudmore wrote: <br>
> <br>
> Hi Padmarao,<br>
> I am working on a RISC-V bsp variant for the Kendryte K210 and I am<br>
> also using an included DTB. For my k210 bsp, I changed<br>
> riscv/shared/start/start.S to set the address of the DTB array before<br>
> calling bsp_fdt_copy. If we change start.S to handle the following<br>
> three cases, we would not have to change the bsp_fdt_copy function.<br>
> The three cases are:<br>
> - no FDT, I assume griscv bsp<br>
> - uboot - a0 in contains the address of the u-boot loaded DTB<br>
> - polarfire, k210 and others - set a0 to the address of the included<br>
> DTB array<br>
> I think it's better to keep this function generic and not have to<br>
> conditionally ignore the input parameter.<br>
> <br>
<br>
Yes, Initially I thought same but don't want to include conditional<br>
statements in the startup code so moved to bsp_fdt_copy().<br>
<br>
Can we try like below in the startup code? so that it can support all<br>
three cases.<br>
<br>
#ifdef BSP_DTB_IS_SUPPORTED<br>
#include BSP_DTB_HEADER_PATH<br>
#endif<br></blockquote><div><br></div><div>For including the DTB header, I have a file similar to the microblaze_fpga bsp:</div><div><a href="https://git.rtems.org/rtems/tree/bsps/microblaze/microblaze_fpga/fdt/bsp_fdt.c" target="_blank">https://git.rtems.org/rtems/tree/bsps/microblaze/microblaze_fpga/fdt/bsp_fdt.c</a><br></div><div>Perhaps after your changes are in the tree we can consider promoting this feature and option to bsps/shared for riscv, microblaze and future bsps that need a DTB.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
#ifdef BSP_START_COPY_FDT_FROM_U_BOOT<br>
#ifdef BSP_DTB_IS_SUPPORT<br>
LADDR a0, system_dtb<br>
#else<br>
mv a0, a1<br>
#endif<br>
call bsp_fdt_copy<br>
#endif<br>
<br></blockquote><div>I added your dtb options to my build, and tried this code in start.S. It works for me.</div><div>Once your polarfire BSP is in, then it will be much easier for k210 support - in fact it may just be a few lines of code and selection of options.<br></div><div>Thanks!</div><div>Alan</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Regards<br>
Padmarao<br>
<br>
> Thanks,<br>
> Alan<br>
> <br>
> <br>
> On Thu, Sep 8, 2022 at 11:44 AM Padmarao Begari <<br>
> <a href="mailto:padmarao.begari@microchip.com" target="_blank">padmarao.begari@microchip.com</a>> wrote:<br>
> > If the bsp is integrated and supported a device tree<br>
> > blob(dtb) then use dtb instead of using it from the U-Boot.<br>
> > ---<br>
> > bsps/shared/start/bsp-fdt.c | 11 ++++++++++-<br>
> > 1 file changed, 10 insertions(+), 1 deletion(-)<br>
> > <br>
> > diff --git a/bsps/shared/start/bsp-fdt.c b/bsps/shared/start/bsp-<br>
> > fdt.c<br>
> > index 75a1ea41c9..7e1a698896 100644<br>
> > --- a/bsps/shared/start/bsp-fdt.c<br>
> > +++ b/bsps/shared/start/bsp-fdt.c<br>
> > @@ -32,6 +32,10 @@<br>
> > #include <bsp/fdt.h><br>
> > #include <bsp/linker-symbols.h><br>
> > <br>
> > +#ifdef BSP_DTB_IS_SUPPORTED<br>
> > +#include BSP_DTB_HEADER_PATH<br>
> > +#endif<br>
> > +<br>
> > #ifndef BSP_FDT_IS_SUPPORTED<br>
> > #warning "BSP FDT support indication not defined"<br>
> > #endif<br>
> > @@ -51,7 +55,12 @@ bsp_fdt_blob[BSP_FDT_BLOB_SIZE_MAX /<br>
> > sizeof(uint32_t)];<br>
> > <br>
> > void bsp_fdt_copy(const void *src)<br>
> > {<br>
> > +#ifdef BSP_DTB_IS_SUPPORTED<br>
> > +const volatile uint32_t *s = (const uint32_t *) system_dtb;<br>
> > +#else<br>
> > const volatile uint32_t *s = (const uint32_t *) src;<br>
> > +#endif<br>
> > +<br>
> > #ifdef BSP_FDT_BLOB_COPY_TO_READ_ONLY_LOAD_AREA<br>
> > uint32_t *d = (uint32_t *) ((uintptr_t) &bsp_fdt_blob[0]<br>
> > - (uintptr_t) bsp_section_rodata_begin<br>
> > @@ -61,7 +70,7 @@ void bsp_fdt_copy(const void *src)<br>
> > #endif<br>
> > <br>
> > if (s != d) {<br>
> > - size_t m = MIN(sizeof(bsp_fdt_blob), fdt_totalsize(src));<br>
> > + size_t m = MIN(sizeof(bsp_fdt_blob), fdt_totalsize(s));<br>
> > size_t aligned_size = roundup2(m, CPU_CACHE_LINE_BYTES);<br>
> > size_t n = (m + sizeof(*d) - 1) / sizeof(*d);<br>
> > size_t i;<br>
</blockquote></div></div>
</blockquote></div>