Re: [PATCH v2 5/6] sparc32: move early memory setup to setup_arch

[Magnus Lindholm]

Hi Sam,


Thanks for taking the time to review this, I've put together a v3 to address
your feedback, will post it after some more polishing.

On Sun, Aug 30, 2026 at 8:19 PM Sam Ravnborg <sam@xxxxxxxxxxxx> wrote:

>
> We have several >> PAGE_SHIFT and << PAGE_SHIFT.
>
> It would be more readable to use the corresponding:
> PHYS_PFN() and PFN_PHYS().

I agree this makes sense.

>>  #define PFN(x)           ((x) >> PAGE_SHIFT)
>A cleanup for another day is replacing  PFN() with the standard PFN_DOWN()

I'll make a note of this, maybe we come back to it later than.


> > -extern unsigned long last_valid_pfn;
> > +extern unsigned long max_low_pfn;
> This is already declared in memblock.h:
>
> $ git grep max_low_pfn
> include/linux/memblock.h:extern unsigned long max_low_pfn;
>
>

This is the better solution, thanks


> > +     for_each_mem_pfn_range(i, MAX_NUMNODES, &start_pfn, &end_pfn,
> > +                            NULL) {
> No need to break this up, we are not above 80 columns.
>

Yes, I'll fix this.

> > +     min_low_pfn = PFN_DOWN(memblock_start_of_DRAM());
> > +     pfn_base = phys_base >> PAGE_SHIFT;
> > +     max_low_pfn = PFN_DOWN(memblock_end_of_DRAM());
> > +     highend_pfn = max_low_pfn;
> highend_pfn is only used to set max_zone_pfn[ZONE_HIGHMEM].
> It would be simpler to just use max_low_pfn and get rid of
> highend_pfn.
>
>
> This part should also set max_pfn.
>

Well spotted, that makes sense.

> > -     if (max_low_pfn > pfn_base + (SRMMU_MAXMEM >> PAGE_SHIFT)) {
> > -             highstart_pfn = pfn_base + (SRMMU_MAXMEM >> PAGE_SHIFT);
> > -             max_low_pfn = calc_max_low_pfn();
> > -             high_pages = calc_highpages();
> > -             printk(KERN_NOTICE "%ldMB HIGHMEM available.\n",
> > -                 high_pages >> (20 - PAGE_SHIFT));
> > -     }
> All the above are not needed as the kernel already print HIGHMEM, and
> this patch removes it - good.
> Maybe tell so in the commit message?

Yes. will fix this.

Magnus