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

[Simon Glass]

Hi Dan,

On Wed, 8 Jan 2025 at 10:18, Dan Carpenter <dan.carpenter at linaro.org> wrote:
>
> 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 at 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 at 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 at 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.
> >
>
> Huh...  I've never thought about it as a success/fail thing.

I have evolved on that one.

>
> I feel like I've seens quite a few reversed strcmp() functions but it's
> hard to catch these sorts of bugs with static analysis because it
> totally depends on the context if you want the strings to be equal or
> not equal.  I can't imagine what a static checker rule would look like.

There's no substitute for tests and naming is hard. We could have a
streq() function which just returns 0 or 1, but it would presumably
have to return 1/true when equal, thus creating confusion with
strcmp(). Perhaps strcheckeq() would be a better name for a function
that returns '0' on success?

Regards,
Simon