Re: Raspberry Pi 4 support on 32 bit earmv7hf GENERIC kernel

[Izumi Tsutsui <tsutsui%ceres.dti.ne.jp@localhost>]

> > Hi,
> > 
> > Currentl NetBSD/evbarm-earmv7hf GENERIC doesn't work on Raspberry Pi 4B:
> > 
> 
> Seems strange, but "OK"...
> 
> Part of me thinks this is unecessary as it unnecessarily bloats GENERIC
> for 32bit platforms.

I have no idea about non-technical decision.

The only reason I'm trying this is to use mmap(2) against /dev/mem
for GPIO accessses on my Pi 4B.

> > diff --git a/sys/arch/arm/arm32/bus_dma.c b/sys/arch/arm/arm32/bus_dma.c
> > index c41fb1e1a065..a6d693a4dae5 100644
> > --- a/sys/arch/arm/arm32/bus_dma.c
> > +++ b/sys/arch/arm/arm32/bus_dma.c
> > @@ -160,7 +160,7 @@ _bus_dma_paddr_inrange(struct arm32_dma_range *ranges, int nranges,
> >   
> >   	for (i = 0, dr = ranges; i < nranges; i++, dr++) {
> >   		if (pa >= dr->dr_sysbase &&
> > -		    pa < dr->dr_sysbase + dr->dr_len)
> > +		    pa - dr->dr_sysbase < dr->dr_len)
> >   			return dr;
> >   	}
> >   
> > @@ -180,11 +180,11 @@ _bus_dma_busaddr_to_paddr(bus_dma_tag_t t, bus_addr_t curaddr)
> >   		return curaddr;
> >   
> >   	for (i = 0, dr = t->_ranges; i < t->_nranges; i++, dr++) {
> > -		if (dr->dr_busbase <= curaddr
> > -		    && curaddr < dr->dr_busbase + dr->dr_len)
> > +		if (curaddr >= dr->dr_busbase &&
> > +		    curaddr - dr->dr_busbase < dr->dr_len)
> >   			return curaddr - dr->dr_busbase + dr->dr_sysbase;
> >   	}
> > -	panic("%s: curaddr %#" PRIxBUSADDR "not in range", __func__, curaddr);
> > +	panic("%s: curaddr %#" PRIxBUSADDR " not in range", __func__, curaddr);
> >   }
> >   
> 
> Why do you think this is needed?

See GitHub commit logs:
 https://github.com/tsutsui/netbsd-src/commit/db488da3abb090fc0fecf89d97a55a30e1ca7ffe
---
arm32/bus_dma: fix interger overflow on 32 bit kernels

On Raspberry Pi 4B with BCM2711, dr_busbase is 0xc0000000 and
dr_len is 0x40000000, so "dr->dr_busbase + dr->dr_len" causes
wraparound.

Fixes panic on Raspberry Pi 4B during attaching vcmbox0:
> panic: _bus_dma_busaddr_to_paddr: curaddr 0xc5b6c000not in range
---

> > --- a/sys/arch/arm/broadcom/bcm283x_platform.c
> > +++ b/sys/arch/arm/broadcom/bcm283x_platform.c
> > @@ -74,6 +74,8 @@ __KERNEL_RCSID(0, "$NetBSD: bcm283x_platform.c,v 1.54 2026/01/08 00:51:20 christ
> >   
> >   #include <libfdt.h>
> >   
> > +#include <dev/ofw/openfirm.h>
> > +
> >   #include <arm/broadcom/bcm2835reg.h>
> >   #include <arm/broadcom/bcm2835var.h>
> >   #include <arm/broadcom/bcm283x_platform.h>
> > @@ -856,12 +858,42 @@ bcm2711_bootparams(void)
> >   }
> >   
> >   #if defined(MULTIPROCESSOR)
> > +static bool
> > +bcm283x_is_2711(void)
> > +{
> > +	static const struct device_compatible_entry compat_data[] = {
> > +		{ .compat = "brcm,bcm2711" },
> > +		DEVICE_COMPAT_EOL
> > +	};
> > +	const int root = OF_finddevice("/");
> > +
> > +	if (root < 0) {
> > +		return false;
> > +	}
> > +
> > +	if (of_compatible_match(root, compat_data) > 0) {
> > +		return true;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> >   static int
> >   cpu_enable_bcm2836(int phandle)
> >   {
> > -	bus_space_tag_t iot = &bcm2836_bs_tag;
> > -	bus_space_handle_t ioh = BCM2836_ARM_LOCAL_VBASE;
> > +	bus_space_tag_t iot;
> > +	bus_space_handle_t ioh;
> >   	uint64_t mpidr;
> > +	bool is2711;
> > +
> > +	is2711 = bcm283x_is_2711();
> > +	if (is2711) {
> > +		iot = &bcm2711_bs_tag;
> > +		ioh = BCM2711_ARM_LOCAL_VBASE;
> > +	} else {
> > +		iot = &bcm2836_bs_tag;
> > +		ioh = BCM2836_ARM_LOCAL_VBASE;
> > +	}
> >   
> >   	fdtbus_get_reg64(phandle, 0, &mpidr, NULL);
> >   
> 
> 
> This seems completely wrong.
> 
> Perhaps the DTS need patching with
> 
> bcm2711.dtsi:           enable-method = "brcm,bcm2836-smp"; // for ARM 32-bit
> 
> s/brcm,bcm2836-smp/brcm,bcm2711-smp/

I have no idea if we should patch DTS for maintainability.

> > diff --git a/sys/dev/fdt/fdtbus.c b/sys/dev/fdt/fdtbus.c
> > index e557b0c6479a..fc244ef1e88a 100644
> > --- a/sys/dev/fdt/fdtbus.c
> > +++ b/sys/dev/fdt/fdtbus.c
> > @@ -90,6 +90,7 @@ static void	fdt_post_attach(struct fdt_node *);
> >   static const struct device_compatible_entry compat_data[] = {
> >   	{ .compat = "simple-bus" },
> >   	{ .compat = "simple-pm-bus" },
> > +	{ .compat = "simple_bus" },
> >   	DEVICE_COMPAT_EOL
> >   };
> 
> Are you using vendor DTS? Please don't.

I'm not sure what "vendor DTS" means, but I'm using armv7.img.gz
from NetBSD 11.0_RC2 and only the kernel is built on -current tree.
(I have not tried -current image)

> > diff --git a/sys/dev/ic/bcmgenet.c b/sys/dev/ic/bcmgenet.c
> > index 36c0cbfe1358..0705db0e948c 100644
> > --- a/sys/dev/ic/bcmgenet.c
> > +++ b/sys/dev/ic/bcmgenet.c
> > @@ -189,7 +189,7 @@ genet_setup_txdesc(struct genet_softc *sc, int index, int flags,
> >   	status = flags | __SHIFTIN(len, GENET_TX_DESC_STATUS_BUFLEN);
> >   
> >   	WR4(sc, GENET_TX_DESC_ADDRESS_LO(index), (uint32_t)paddr);
> > -	WR4(sc, GENET_TX_DESC_ADDRESS_HI(index), (uint32_t)(paddr >> 32));
> > +	WR4(sc, GENET_TX_DESC_ADDRESS_HI(index), (uint32_t)((uint64_t)paddr >> 32));
> >   	WR4(sc, GENET_TX_DESC_STATUS(index), status);
> >   }
> >   
> > @@ -259,7 +259,7 @@ genet_setup_rxdesc(struct genet_softc *sc, int index,
> >       bus_addr_t paddr, bus_size_t len)
> >   {
> >   	WR4(sc, GENET_RX_DESC_ADDRESS_LO(index), (uint32_t)paddr);
> > -	WR4(sc, GENET_RX_DESC_ADDRESS_HI(index), (uint32_t)(paddr >> 32));
> > +	WR4(sc, GENET_RX_DESC_ADDRESS_HI(index), (uint32_t)((uint64_t)paddr >> 32));
> >   }
> >   
> >   static int
> > 
> 
> 
> Just use BUS_ADDR_{LO,HI}32

I see.
(Note several other drivers like sys/dev/ic/dwc_eqos.c also use
 "(uint32_t)((uint64_t)paddr >> 32)" formats)

---
Izumi Tsutsui