Re: [PATCH] riscv: spacemit: k1: probe dram size during boot phase.

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 2986 bytes --]

On Wed, Jan 08, 2025 at 10:03:08AM -0700, Simon Glass wrote:
> Hi Dan,
> 
> On Wed, 8 Jan 2025 at 05:37, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >
> > On Wed, Jan 08, 2025 at 12:21:18PM +0100, Heinrich Schuchardt wrote:
> > > Am 8. Januar 2025 12:11:05 MEZ schrieb Yixun Lan <dlan@gentoo.org>:
> > > >Hi Huan:
> > > >
> > > >On 16:49 Wed 08 Jan     , Huan Zhou wrote:
> > > >>
> > > >>
> > > >..
> > > >> ---
> > > >remove above "---"? otherwise following commit message will be
> > > >dropped during patch application..
> > > >
> > > >> This patch introduce improvement for get dram size on bananapi BPI-F3,
> > > >> retrieving the dram size dynamically.
> > > >> Have tested on bananapi BPIF3 4G and jupiter 8G.
> > > >>
> > > >> Signed-off-by: Huan Zhou <me@per1cycle.org>
> > > >> ---
> > > >>  arch/riscv/cpu/k1/dram.c | 40 ++++++++++++++++++++++++++++++++++++++--
> > > >>  1 file changed, 38 insertions(+), 2 deletions(-)
> > > >>
> > > >> diff --git a/arch/riscv/cpu/k1/dram.c b/arch/riscv/cpu/k1/dram.c
> > > >> index c477c15cbfb19f0e3a0ee72985b602f5bda352d7..095217f2a4c053f7477d62c0776bcb51e623db47 100644
> > > >> --- a/arch/riscv/cpu/k1/dram.c
> > > >> +++ b/arch/riscv/cpu/k1/dram.c
> > > >> @@ -4,17 +4,53 @@
> > > >>   */
> > > >>
> > > >>  #include <asm/global_data.h>
> > > >> +#include <asm/io.h>
> > > >>  #include <config.h>
> > > >> +#include <bitfield.h>
> > > >>  #include <fdt_support.h>
> > > >>  #include <linux/sizes.h>
> > > >>
> > > >> +#define DDR_BASE 0xC0000000
> > > >>  DECLARE_GLOBAL_DATA_PTR;
> > > >>
> > > >> +static inline u32 map_format_size(u32 val)
> > > >> +{
> > > >> +  u32 tmp;
> > > >> +
> > > >> +  if (val & 0x1 == 0)
> > > >please add brackets explicitly, something like
> > > >     if ((val & 0x1) == 0)
> > >
> > > We tend to avoid == 0 in U-Boot
> > >
> > > if (val & BIT(0))
> > >
> >
> > That's reversed.
> >
> >         if (!(val & BIT(0)) {
> >
> > I have really complicated rules about when to use ! vs == 0.
> > https://staticthinking.wordpress.com/2024/02/20/when-to-use-0/
> 
> I  mostly agree with that and it provides some motivation for the
> conventions which have built up over the years in Linux/U-Boot/etc.
> 
> If you want to send a patch:
> 
> $ git grep 'if (ret != EFI_SUCCESS)' |wc
>     864    4841   51380
> 
> I think the only place I would differ is with strcmp(), where I've got
> used to 0 meaning success, like in much other code.

To be clear here, we have ~900 examples of EFI_SUCCESS and ~250 examples
of other FOO_SUCCESS type tests. And if we're going to make some sort of
change here, we should (a) document it and (b) fix it everywhere.

And at the risk of confusing your intentions Simon, Dan has a well
deserved reputation in the Linux kernel for fixing what I would call "C
is trickier than you think, even if you're been doing it for decades"
bugs so just changing EFI code would be silly.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]