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.
Regards,
Simon
