| Message ID | 20210303005526.15056-6-phil@nwl.cc |
|---|---|
| State | Superseded |
| Delegated to: | Stefan Roese |
| Headers | show |
| Series | Synology DS414 integration mini-review | expand |
On 03.03.21 01:55, Phil Sutter wrote: > Signed-off-by: Phil Sutter <phil@nwl.cc> It's common practice to add some minimal text in the commit text, even for simple patches. One short question below... > --- > include/configs/ds414.h | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/include/configs/ds414.h b/include/configs/ds414.h > index a2248cf75ad72..c8b45066cc75a 100644 > --- a/include/configs/ds414.h > +++ b/include/configs/ds414.h > @@ -88,7 +88,12 @@ > "initrd_high=0xffffffff\0" \ > "ramdisk_addr_r=0x8000000\0" \ > "usb0Mode=host\0usb1Mode=host\0usb2Mode=device\0" \ > - "ethmtu=1500\0eth1mtu=1500\0" > + "ethmtu=1500\0eth1mtu=1500\0" \ > + "update_uboot=sf probe; dhcp; " \ > + "mw.b ${loadaddr} 0x0 0xd0000; " \ > + "tftpboot ${loadaddr} u-boot-spl.kwb; " \ > + "sf update ${loadaddr} 0x0 0xd0000\0" > + Wouldn't it be better to use ${filesize} instead of 0xd0000 here? Other than that: Reviewed-by: Stefan Roese <sr@denx.de> Thanks, Stefan
On Thu, Mar 04, 2021 at 02:09:34PM +0100, Stefan Roese wrote: > On 03.03.21 01:55, Phil Sutter wrote: > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > It's common practice to add some minimal text in the commit text, > even for simple patches. I should learn to put less meaningful subjects in those cases so I have an easier time finding something to write about in the description. Then again, your question indicates I just didn't think hard enough. [...] > > diff --git a/include/configs/ds414.h b/include/configs/ds414.h > > index a2248cf75ad72..c8b45066cc75a 100644 > > --- a/include/configs/ds414.h > > +++ b/include/configs/ds414.h > > @@ -88,7 +88,12 @@ > > "initrd_high=0xffffffff\0" \ > > "ramdisk_addr_r=0x8000000\0" \ > > "usb0Mode=host\0usb1Mode=host\0usb2Mode=device\0" \ > > - "ethmtu=1500\0eth1mtu=1500\0" > > + "ethmtu=1500\0eth1mtu=1500\0" \ > > + "update_uboot=sf probe; dhcp; " \ > > + "mw.b ${loadaddr} 0x0 0xd0000; " \ > > + "tftpboot ${loadaddr} u-boot-spl.kwb; " \ > > + "sf update ${loadaddr} 0x0 0xd0000\0" > > + > > Wouldn't it be better to use ${filesize} instead of 0xd0000 here? U-Boot's "partition" size is fixed and hard-coded in the DTB at least. Putting the destination size as a limit at least avoids the unlikely chance of overwriting data past that partition. BTW: I spent a while trying to make the DTB-defined partitions available in sf command, but eventually gave up. Seems I need to have this SPI->MTD mapping and still define partitions in environment. Not worth spending cycles on though, I was just curious because 'help sf' mentions partitions as offset alternatives. Cheers, Phil
On 04.03.21 14:28, Phil Sutter wrote: > On Thu, Mar 04, 2021 at 02:09:34PM +0100, Stefan Roese wrote: >> On 03.03.21 01:55, Phil Sutter wrote: >>> Signed-off-by: Phil Sutter <phil@nwl.cc> >> >> It's common practice to add some minimal text in the commit text, >> even for simple patches. > > I should learn to put less meaningful subjects in those cases so I have > an easier time finding something to write about in the description. ;) > Then > again, your question indicates I just didn't think hard enough. I agree that this "rule" is sometimes hard to understand. I myself am sometimes finding it stupid to write the same sentence twice. > [...] >>> diff --git a/include/configs/ds414.h b/include/configs/ds414.h >>> index a2248cf75ad72..c8b45066cc75a 100644 >>> --- a/include/configs/ds414.h >>> +++ b/include/configs/ds414.h >>> @@ -88,7 +88,12 @@ >>> "initrd_high=0xffffffff\0" \ >>> "ramdisk_addr_r=0x8000000\0" \ >>> "usb0Mode=host\0usb1Mode=host\0usb2Mode=device\0" \ >>> - "ethmtu=1500\0eth1mtu=1500\0" >>> + "ethmtu=1500\0eth1mtu=1500\0" \ >>> + "update_uboot=sf probe; dhcp; " \ >>> + "mw.b ${loadaddr} 0x0 0xd0000; " \ >>> + "tftpboot ${loadaddr} u-boot-spl.kwb; " \ >>> + "sf update ${loadaddr} 0x0 0xd0000\0" >>> + >> >> Wouldn't it be better to use ${filesize} instead of 0xd0000 here? > > U-Boot's "partition" size is fixed and hard-coded in the DTB at least. > Putting the destination size as a limit at least avoids the unlikely > chance of overwriting data past that partition. Ok, understood. > BTW: I spent a while trying to make the DTB-defined partitions available > in sf command, but eventually gave up. Seems I need to have this > SPI->MTD mapping and still define partitions in environment. Not worth > spending cycles on though, I was just curious because 'help sf' mentions > partitions as offset alternatives. Not looking to hard into your issue here, but did you take a look at the "mtd" command? It's newer than the other flash related commands and can manage all kind of flash types (SPI NOR, NAND etc) in one place. And also integrated better into the MTD partitions IIRC. Thanks, Stefan
On Thu, Mar 04, 2021 at 02:34:53PM +0100, Stefan Roese wrote: > On 04.03.21 14:28, Phil Sutter wrote: > > On Thu, Mar 04, 2021 at 02:09:34PM +0100, Stefan Roese wrote: > >> On 03.03.21 01:55, Phil Sutter wrote: > >>> Signed-off-by: Phil Sutter <phil@nwl.cc> > >> > >> It's common practice to add some minimal text in the commit text, > >> even for simple patches. > > > > I should learn to put less meaningful subjects in those cases so I have > > an easier time finding something to write about in the description. > > ;) > > > Then > > again, your question indicates I just didn't think hard enough. > > I agree that this "rule" is sometimes hard to understand. I myself > am sometimes finding it stupid to write the same sentence twice. I've seen enough projects where one-liners are absolutely acceptable, I'd rather write redundant texts than accepting that coding "style". Nothing worse than confusing code garnished with no description at all. :) [...] > > BTW: I spent a while trying to make the DTB-defined partitions available > > in sf command, but eventually gave up. Seems I need to have this > > SPI->MTD mapping and still define partitions in environment. Not worth > > spending cycles on though, I was just curious because 'help sf' mentions > > partitions as offset alternatives. > > Not looking to hard into your issue here, but did you take a look at > the "mtd" command? It's newer than the other flash related commands > and can manage all kind of flash types (SPI NOR, NAND etc) in one > place. And also integrated better into the MTD partitions IIRC. Hmm, for some reason I assumed 'sf' is the optimal tool for SPI flashes. I'll give it a try, thanks for the hint! Thanks, Phil
diff --git a/include/configs/ds414.h b/include/configs/ds414.h index a2248cf75ad72..c8b45066cc75a 100644 --- a/include/configs/ds414.h +++ b/include/configs/ds414.h @@ -88,7 +88,12 @@ "initrd_high=0xffffffff\0" \ "ramdisk_addr_r=0x8000000\0" \ "usb0Mode=host\0usb1Mode=host\0usb2Mode=device\0" \ - "ethmtu=1500\0eth1mtu=1500\0" + "ethmtu=1500\0eth1mtu=1500\0" \ + "update_uboot=sf probe; dhcp; " \ + "mw.b ${loadaddr} 0x0 0xd0000; " \ + "tftpboot ${loadaddr} u-boot-spl.kwb; " \ + "sf update ${loadaddr} 0x0 0xd0000\0" + /* increase autoneg timeout, my NIC sucks */ #define PHY_ANEG_TIMEOUT 16000
Signed-off-by: Phil Sutter <phil@nwl.cc> --- include/configs/ds414.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)