| Message ID | 1c2f9dadbfabb9532aa56c2fe5925240854ff90a.1544369598.git.chunkeey@gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [OpenWrt-Devel,1/8] kernel: add DT binding support to the fit parser | expand |
Hi Christian, On 09.12.2018 16:34, Christian Lamparter wrote: > It allows specifying default and Netgear parsers directly in the DT. > > Signed-off-by: Christian Lamparter <chunkeey@gmail.com> > --- > .../linux/generic/files/drivers/mtd/mtdsplit/mtdsplit_fit.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/target/linux/generic/files/drivers/mtd/mtdsplit/mtdsplit_fit.c b/target/linux/generic/files/drivers/mtd/mtdsplit/mtdsplit_fit.c > index f356adcd4e..b7e56fc083 100644 > --- a/target/linux/generic/files/drivers/mtd/mtdsplit/mtdsplit_fit.c > +++ b/target/linux/generic/files/drivers/mtd/mtdsplit/mtdsplit_fit.c > @@ -120,9 +120,15 @@ mtdsplit_fit_parse(struct mtd_info *mtd, > return 2; > } > > +static const struct of_device_id mtdsplit_fit_of_match_table[] = { > + { .compatible = "openwrt,fit-firmware" }, Shouldn't that be "denx,fit", same as in case of "denx,uimage"? AFAIK, FIT format also comes from U-Boot and it's not OpenWrt specific (please, correct me if I'm wrong here).
On 10/12/2018 16:47, Piotr Dymacz wrote: > Hi Christian, > > On 09.12.2018 16:34, Christian Lamparter wrote: >> It allows specifying default and Netgear parsers directly in the DT. >> >> Signed-off-by: Christian Lamparter <chunkeey@gmail.com> >> --- >> .../linux/generic/files/drivers/mtd/mtdsplit/mtdsplit_fit.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git >> a/target/linux/generic/files/drivers/mtd/mtdsplit/mtdsplit_fit.c >> b/target/linux/generic/files/drivers/mtd/mtdsplit/mtdsplit_fit.c >> index f356adcd4e..b7e56fc083 100644 >> --- a/target/linux/generic/files/drivers/mtd/mtdsplit/mtdsplit_fit.c >> +++ b/target/linux/generic/files/drivers/mtd/mtdsplit/mtdsplit_fit.c >> @@ -120,9 +120,15 @@ mtdsplit_fit_parse(struct mtd_info *mtd, >> return 2; >> } >> +static const struct of_device_id mtdsplit_fit_of_match_table[] = { >> + { .compatible = "openwrt,fit-firmware" }, > > Shouldn't that be "denx,fit", same as in case of "denx,uimage"? AFAIK, > FIT format also comes from U-Boot and it's not OpenWrt specific > (please, correct me if I'm wrong here). > i was about to ask the same. a owrt prefix means we have to worry about it in the future and we dont want that.... John
On Monday, December 10, 2018 10:28:22 PM CET John Crispin wrote: > > On 10/12/2018 16:47, Piotr Dymacz wrote: > > Hi Christian, > > > > On 09.12.2018 16:34, Christian Lamparter wrote: > >> It allows specifying default and Netgear parsers directly in the DT. > >> > >> Signed-off-by: Christian Lamparter <chunkeey@gmail.com> > >> --- > >> .../linux/generic/files/drivers/mtd/mtdsplit/mtdsplit_fit.c | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > >> diff --git > >> a/target/linux/generic/files/drivers/mtd/mtdsplit/mtdsplit_fit.c > >> b/target/linux/generic/files/drivers/mtd/mtdsplit/mtdsplit_fit.c > >> index f356adcd4e..b7e56fc083 100644 > >> --- a/target/linux/generic/files/drivers/mtd/mtdsplit/mtdsplit_fit.c > >> +++ b/target/linux/generic/files/drivers/mtd/mtdsplit/mtdsplit_fit.c > >> @@ -120,9 +120,15 @@ mtdsplit_fit_parse(struct mtd_info *mtd, > >> return 2; > >> } > >> +static const struct of_device_id mtdsplit_fit_of_match_table[] = { > >> + { .compatible = "openwrt,fit-firmware" }, > > > > Shouldn't that be "denx,fit", same as in case of "denx,uimage"? AFAIK, > > FIT format also comes from U-Boot and it's not OpenWrt specific > > (please, correct me if I'm wrong here). > > I didn't know that DENX is fine with calling this mtdsplit binding their own creation, since their u-boot does not contain any bindings for this (yet?). So I went for openwrt,fit-firmware because OpenWrt puts so much more than just the FIT in that partition. The FIT, rootfs, optional padding and rootfs_data(aligned JFFS2 marker) are concatenated together in that specific order so the firmware can be flashed in one go. On boot The u-boot only loads the first thing it finds at a specified offset of the flash (Note: the FIT image does have support to store a rootfs in it too! But from what I can tell, no official OpenWrt image is using this feature). Only the kernel uses a combination of mtdsplit_fit/uimage and mtdsplit_squashfs.c to create the dynamic mtd partitions for the kernel, rootfs and rootfs_data. That said, I'm fine with calling it "denx,fit" and I hope DENX is as well :-D. > i was about to ask the same. a owrt prefix means we have to worry about > it in the future and we dont want that.... Done.
Hi Christian, On 11.12.2018 02:10, Christian Lamparter wrote: > On Monday, December 10, 2018 10:28:22 PM CET John Crispin wrote: >> >> On 10/12/2018 16:47, Piotr Dymacz wrote: >> > Hi Christian, >> > >> > On 09.12.2018 16:34, Christian Lamparter wrote: >> >> It allows specifying default and Netgear parsers directly in the DT. >> >> >> >> Signed-off-by: Christian Lamparter <chunkeey@gmail.com> >> >> --- >> >> .../linux/generic/files/drivers/mtd/mtdsplit/mtdsplit_fit.c | 6 ++++++ >> >> 1 file changed, 6 insertions(+) >> >> >> >> diff --git >> >> a/target/linux/generic/files/drivers/mtd/mtdsplit/mtdsplit_fit.c >> >> b/target/linux/generic/files/drivers/mtd/mtdsplit/mtdsplit_fit.c >> >> index f356adcd4e..b7e56fc083 100644 >> >> --- a/target/linux/generic/files/drivers/mtd/mtdsplit/mtdsplit_fit.c >> >> +++ b/target/linux/generic/files/drivers/mtd/mtdsplit/mtdsplit_fit.c >> >> @@ -120,9 +120,15 @@ mtdsplit_fit_parse(struct mtd_info *mtd, >> >> return 2; >> >> } >> >> +static const struct of_device_id mtdsplit_fit_of_match_table[] = { >> >> + { .compatible = "openwrt,fit-firmware" }, >> > >> > Shouldn't that be "denx,fit", same as in case of "denx,uimage"? AFAIK, >> > FIT format also comes from U-Boot and it's not OpenWrt specific >> > (please, correct me if I'm wrong here). >> > > > I didn't know that DENX is fine with calling this mtdsplit binding their own > creation, since their u-boot does not contain any bindings for this (yet?). > So I went for openwrt,fit-firmware because OpenWrt puts so much more than > just the FIT in that partition. The FIT, rootfs, optional padding and > rootfs_data(aligned JFFS2 marker) are concatenated together in that specific > order so the firmware can be flashed in one go. > On boot The u-boot only loads the first thing it finds at a specified > offset of the flash (Note: the FIT image does have support to store a > rootfs in it too! But from what I can tell, no official OpenWrt image > is using this feature). Only the kernel uses a combination of > mtdsplit_fit/uimage and mtdsplit_squashfs.c to create the dynamic mtd > partitions for the kernel, rootfs and rootfs_data. And this is also true for the legacy uImage format. OpenWrt already uses "denx,uimage" binding for mtd parser which does much more than just finding and creating kernel partition based on the uImage header. > That said, I'm fine with calling it "denx,fit" and I hope DENX is as well :-D. My suggestion was more about keeping consistency here. So if at some point OpenWrt (or upstream) decides it should be "openwrt,xyz" I won't complain. But I think we should at least be consistent with the naming - so if "openwrt,fit" then also "openwrt,uimage". PS. Sorry for the late reply, I know that changes are already in tree.
diff --git a/target/linux/generic/files/drivers/mtd/mtdsplit/mtdsplit_fit.c b/target/linux/generic/files/drivers/mtd/mtdsplit/mtdsplit_fit.c index f356adcd4e..b7e56fc083 100644 --- a/target/linux/generic/files/drivers/mtd/mtdsplit/mtdsplit_fit.c +++ b/target/linux/generic/files/drivers/mtd/mtdsplit/mtdsplit_fit.c @@ -120,9 +120,15 @@ mtdsplit_fit_parse(struct mtd_info *mtd, return 2; } +static const struct of_device_id mtdsplit_fit_of_match_table[] = { + { .compatible = "openwrt,fit-firmware" }, + {}, +}; + static struct mtd_part_parser uimage_parser = { .owner = THIS_MODULE, .name = "fit-fw", + .of_match_table = mtdsplit_fit_of_match_table, .parse_fn = mtdsplit_fit_parse, .type = MTD_PARSER_TYPE_FIRMWARE, };
It allows specifying default and Netgear parsers directly in the DT. Signed-off-by: Christian Lamparter <chunkeey@gmail.com> --- .../linux/generic/files/drivers/mtd/mtdsplit/mtdsplit_fit.c | 6 ++++++ 1 file changed, 6 insertions(+)