On Fri, 15 May 2026 at 14:07, Simon Glass <[email protected]> wrote: > > Hi Aidan, > > On 2026-05-13T00:26:04, Aidan Garske <[email protected]> wrote: > > spi: add BCM2835/BCM2711 hardware SPI controller driver > > > > The BCM2835/BCM2711 SoC on Raspberry Pi 3/4 has no in-tree U-Boot > > driver for its hardware SPI0 controller; existing U-Boot work with > > SPI peripherals on these boards has to fall back to GPIO bit-banging > > via spi-gpio. That fallback is functional but slow (a few hundred > > kHz versus the 32 MHz the controller supports), and for protocols > > like the Infineon SLB9670 / SLB9672 TPM 2.0 TIS interface it makes > > early-boot operations (startup, self-test, PCR extend) noticeably > > sluggish. > > > > Port the Linux kernel driver drivers/spi/spi-bcm2835.c into U-Boot > > so existing device-tree nodes using the 'brcm,bcm2835-spi' / > > 'brcm,bcm2711-spi' compatible work out of the box. The port keeps > > the same register layout and software-GPIO chip-select scheme the > > Linux driver already uses, which is necessary to work around the > > BCM2835 SPI block's CS auto-deassert behaviour - that behaviour > > corrupts multi-byte TPM TIS transactions when hardware CS is left > > enabled. > > > > [...] > > > > drivers/spi/Kconfig | 9 + > > drivers/spi/Makefile | 1 + > > drivers/spi/bcm2835_spi.c | 440 > > ++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 450 insertions(+) > > > diff --git a/drivers/spi/bcm2835_spi.c b/drivers/spi/bcm2835_spi.c > > @@ -0,0 +1,440 @@ > > +/* TPM Reset via GPIO4 and GPIO24 */ > > +static void bcm2835_spi_tpm_reset(void) > > +{ > > + void __iomem *gpio_base = (void __iomem *)0xFE200000; > > + u32 val; > > + > > + /* Set GPIO4 as output (GPFSEL0, bits 14:12) */ > > The GPIO base should come from the devicetree and more generally, > access to it should be via a GPIO driver. Please use lower-case hex. > > This should not be in a generic SPI controller driver, and it > contradicts the commit message claim that the driver is not > TPM-specific. Any board that probes this driver currently pulses GPIO4 > and GPIO24 low for 100 ms - what if something is wired to those pins? > TPM reset belongs in the TPM driver (perhaps with a gpio-reset in the > DT node for the TPM). > > > diff --git a/drivers/spi/bcm2835_spi.c b/drivers/spi/bcm2835_spi.c > > @@ -0,0 +1,440 @@ > > +/* GPIO base for software CS control */ > > +static void __iomem *g_gpio_base = (void __iomem *)0xFE200000; > > This is BCM2711-only, so BCM2835 (Pi 3, which the Kconfig help and > commit subject both advertise) has its GPIO block at 0x3f200000, so > the driver will silently corrupt unrelated memory there. Anyway, > U-Boot already has pinctrl (pinctrl-bcm283x.c) and GPIO > (bcm2835_gpio.c) drivers for this SoC; please go through those. > > > diff --git a/drivers/spi/bcm2835_spi.c b/drivers/spi/bcm2835_spi.c > > @@ -0,0 +1,440 @@ > > + /* Try to get CS GPIO from device tree */ > > + ret = gpio_request_by_name(bus, 'cs-gpios', 0, &priv->cs_gpio, > > + GPIOD_IS_OUT | GPIOD_ACTIVE_LOW); > > + if (!ret) { > > + priv->cs_gpio_valid = 1; > > + /* Deassert CS initially */ > > + dm_gpio_set_value(&priv->cs_gpio, 1); > > + } else { > > + priv->cs_gpio_valid = 0; > > + } > > priv->cs_gpio and priv->cs_gpio_valid are written here but never read > - bcm2835_spi_xfer() instead does > > int cs_pin = (cs == 0) ? 8 : 7; > > and pokes GPSET0/GPCLR0 directly, ignoring the DT. Either honour > cs-gpios via dm_gpio_set_value() (the normal U-Boot pattern, which > DM_SPI_GPIO already handles generically), or drop the cs_gpio request > and document that the driver hardcodes the Pi SPI0 CE0/CE1 pin > assignment. As written it's half of each. > > > diff --git a/drivers/spi/bcm2835_spi.c b/drivers/spi/bcm2835_spi.c > > @@ -0,0 +1,440 @@ > > +struct bcm2835_spi_priv { > > + void __iomem *regs; > > + u32 clk_hz; > > + u32 cs_reg; /* Cached CS register value */ > > + u32 speed_hz; > > + u8 mode; > > + struct gpio_desc cs_gpio; > > + int cs_gpio_valid; > > + int cs_asserted; /* Track if CS should stay asserted between > > transfers */ > > +}; > > cs_asserted is assigned in three places in xfer() but never read. > Please remove it (and the comment that promises behaviour it does not > implement). > > > diff --git a/drivers/spi/bcm2835_spi.c b/drivers/spi/bcm2835_spi.c > > @@ -0,0 +1,440 @@ > > +/* Default clock rate - 250 MHz for Pi 4 */ > > +#define BCM2835_SPI_DEFAULT_CLK 250000000 > > This is the SPI core/source clock, not a default transfer rate - the > default transfer speed is 1 MHz below. The name is confusing. More > importantly, on BCM2835 vs BCM2711 vs different VPU clock settings > this number is not constant; please pull it from the clk uclass (or > from clock-frequency in DT, which you already read, with no hard-coded > fallback). > > > diff --git a/drivers/spi/bcm2835_spi.c b/drivers/spi/bcm2835_spi.c > > @@ -0,0 +1,440 @@ > > +static inline void bcm2835_spi_writel(struct bcm2835_spi_priv *priv, > > + > > u32 reg, u32 val) > > +{ > > + writel(val, priv->regs + reg); > > +} > > Please check indent for tab-alignment. Patman or checkpatch will flag these. > > > diff --git a/drivers/spi/bcm2835_spi.c b/drivers/spi/bcm2835_spi.c > > @@ -0,0 +1,440 @@ > > + /* Poll for completion - transfer byte by byte */ > > + timeout = 100000; > > + while ((tx_count < len || rx_count < len) && timeout > 0) { > > The 100000 and 10000 timeouts are bare loop counters, not time bounds > so at 1 MHz a 4 KB transfer takes ~32 ms and the loop body has no > fixed cost, so these mean nothing except 'don't hang forever'. Please > convert to get_timer() / timer_get_us(), the way other DM_SPI drivers > do, so the timeout is independent of CPU speed and transfer length. > > > diff --git a/drivers/spi/bcm2835_spi.c b/drivers/spi/bcm2835_spi.c > > @@ -0,0 +1,440 @@ > > +config BCM2835_SPI > > + bool "BCM2835/BCM2711 SPI driver" > > + depends on ARCH_BCM283X > > Given the hard-coded 0xfe200000 GPIO base and the BCM2711-only fix-up > in of_to_plat(), the BCM2835 half of this option is not actually > supported. Either fix the driver to derive the GPIO base from DT / > from the existing bcm2835_gpio driver and exercise it on a Pi 3, or > narrow the Kconfig text and commit subject to say BCM2711 only.
This needs to support all, if it's an option people will try to use it and get a bad experience.
