On Thu, Feb 21, 2019 at 6:51 AM Marek Vasut <ma...@denx.de> wrote: > > On 2/21/19 1:13 PM, Michal Simek wrote: > > On 21. 02. 19 10:04, Marek Vasut wrote: > >> On 2/21/19 9:55 AM, Alexander Graf wrote: > >>> > >>> > >>> On 21.02.19 09:49, Marek Vasut wrote: > >>>> On 2/21/19 9:44 AM, Alexander Graf wrote: > >>>>> > >>>>> > >>>>> On 21.02.19 09:41, Marek Vasut wrote: > >>>>>> On 2/21/19 9:40 AM, Chee, Tien Fong wrote: > >>>>>>> On Thu, 2019-02-21 at 09:29 +0100, Alexander Graf wrote: > >>>>>>>> > >>>>>>>> On 21.02.19 09:23, Chee, Tien Fong wrote: > >>>>>>>>> > >>>>>>>>> On Thu, 2019-02-21 at 08:45 +0100, Michal Simek wrote: > >>>>>>>>>> > >>>>>>>>>> Hi Tom, > >>>>>>>>>> > >>>>>>>>>> On 20. 02. 19 2:58, Tom Rini wrote: > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> On Mon, Feb 11, 2019 at 02:56:19PM +0800, tien.fong.chee@intel. > >>>>>>>>>>> com > >>>>>>>>>>> wrote: > >>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> From: Tien Fong Chee <tien.fong.c...@intel.com> > >>>>>>>>>>>> > >>>>>>>>>>>> Drop the statically allocated get_contents_vfatname_block and > >>>>>>>>>>>> dynamically allocate a buffer only if required. This saves > >>>>>>>>>>>> 64KiB of memory. > >>>>>>>>>>>> > >>>>>>>>>>>> Signed-off-by: Stefan Agner <stefan.ag...@toradex.com> > >>>>>>>>>>>> Signed-off-by: Tien Fong Chee <tien.fong.c...@intel.com> > >>>>>>>>>>> Applied to u-boot/master, thanks! > >>>>>>>>>> please remove this patch (better both of them because they were > >>>>>>>>>> in > >>>>>>>>>> series) > >>>>>>>>> I think patch 2/2 should be safe, because no memory size is > >>>>>>>>> changed. > >>>>>>>>> Basically, it just to release the allocated memory immediately when > >>>>>>>>> it's not required, so other can re-use it. > >>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> because they are breaking at least ZynqMP SPL. It is also too > >>>>>>>>>> late in cycle to create random fix. > >>>>>>>>>> > >>>>>>>>>> You can't simply move 64KB from code to malloc without reflecting > >>>>>>>>>> this > >>>>>>>>>> by changing MALLOC space size. > >>>>>>>>>> > >>>>>>>>>> Other boards with SPL fat could be also affected by this if they > >>>>>>>>>> don't > >>>>>>>>>> allocate big malloc space. > >>>>>>>>> So, any suggestion to get the patch 1/2 accepted? inform all board > >>>>>>>>> maintainers to test it out? > >>>>>>>> You already received feedback that it does break ZynqMP, so the > >>>>>>>> current > >>>>>>>> approach won't work. > >>>>>>>> > >>>>>>>> How about you create a new kconfig option that allows you to say > >>>>>>>> whether > >>>>>>>> you want to use malloc or .bss for temporary data in the FAT driver. > >>>>>>>> You > >>>>>>>> can then have an _SPL_ version of that kconfig and check for it with > >>>>>>>> IS_ENABLED() which should automatically tell you the right answer > >>>>>>>> depending on whether you're in an SPL build or not. > >>>>>>>> > >>>>>>>> Then you can set the SPL version to default malloc and the non-SPL > >>>>>>>> version to default .bss. > >>>>>>> Marek and Tom rini, > >>>>>>> > >>>>>>> Are you guys okay with Alex's suggestion? > >>>>>> > >>>>>> I'm not a big fan of adding more and more ifdeffery. > >>>>>> Is there some other option ? > >>>>> > >>>>> Is RAM up already at this point? Maybe we could improve the SPL malloc > >>>>> mechanism to move allocations into DRAM once it's available. > >>>> > >>>> Well, the FAT buffers waste some 64kiB of bss, so we can use that area > >>>> for malloc instead, no ? > >>> > >>> Yes, but that means you need to review every single board that uses FAT > >>> in SPL today and adjust its malloc region size.
This happened to me for a different board where the SPI-NOR flash driver was updated, but it required me to increase my malloc size. I don't think it would have been appropriate for me to ask the maintainer to reject a patch when where was a simple solution, and the author helped me identify how to make the updated infrastructure work. I think the benefits outweigh the cons if (and only if) the solution is only to increase the malloc size. Many of us have very restricted SPL space. > >> > >> That's quite likely ... I still think this patch is beneficial, it's > >> much better to dynamically allocate the cluster size than have this > >> 64kiB chunk of BSS carved out. > >> > > > > ok. I have played with it a little bit and the patch exposed different > > problem with one of my out of tree patch I am working on. > > > > Anyway that's being said I still think that patches like this shouldn't > > come to the tree at this stage because it requires checking on other > > boards. IIRC similar patch was around in past and there was also any > > issue with it. > > > > Tom: up2you if you want to keep it in the tree or not. > > This shouldn't have come in after RC2, so revert and let's fix it for > next release. > > -- > Best regards, > Marek Vasut > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot