Hi Giau,
On 2026-10-28T11:30:05+11:00, Giau Nguyen <[email protected]> wrote:
> i2c: bcm2835: Add new BCM2835/BCM2711 I2C controller driver
>
> Mainline U-Boot currently lacks an I2C controller driver for the
> Broadcom BCM2711 SoC (Raspberry Pi 4).
>
> This commit adds a new DM-compliant I2C driver for Broadcom
> BCM2835/BCM2711 SoCs based on the Linux kernel's i2c-bcm2835.c.
>
> The driver dynamically queries the actual VPU core clock rate from
> the VideoCore firmware via mailbox property messages to calculate
> the CDIV clock divider, ensuring accurate I2C timing and baud rates
> on BCM2711.
>
> Enable CONFIG_SYS_I2C_BCM2835 in rpi_4_defconfig.
>
> Signed-off-by: Giau Nguyen <[email protected]>
>
> MAINTAINERS | 1 +
> arch/arm/mach-bcm283x/include/mach/msg.h | 9 +
> arch/arm/mach-bcm283x/msg.c | 22 ++
> configs/rpi_4_defconfig | 4 +
> drivers/i2c/Kconfig | 10 +
> drivers/i2c/Makefile | 1 +
> drivers/i2c/i2c-bcm2835.c | 370
> +++++++++++++++++++++++++++++++
> 7 files changed, 417 insertions(+)
> diff --git a/arch/arm/mach-bcm283x/msg.c b/arch/arm/mach-bcm283x/msg.c
> @@ -116,6 +116,28 @@ int bcm2835_get_mmc_clock(u32 clock_id)
> +int bcm2835_get_clock_rate(u32 clock_id, ulong *rate)
> +{
> + ALLOC_CACHE_ALIGN_BUFFER(struct msg_get_clock_rate, msg_clk, 1);
> + int ret;
> +
> + BCM2835_MBOX_INIT_HDR(msg_clk);
> + BCM2835_MBOX_INIT_TAG(&msg_clk->get_clock_rate, GET_CLOCK_RATE);
> + msg_clk->get_clock_rate.body.req.clock_id = clock_id;
This duplicates the first half of bcm2835_get_mmc_clock() just above.
Please can you make bcm2835_get_mmc_clock() call this new function, so
there is only one copy?
> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig
> @@ -171,6 +171,16 @@ config SYS_I2C_AT91
> +config SYS_I2C_BCM2835
> + bool "BCM2835 I2C controller"
> + depends on DM_I2C
The driver uses asm/arch/mbox.h, asm/arch/msg.h and
bcm2835_get_clock_rate(), so it only builds on bcm283x. Please can you
add 'depends on ARCH_BCM283X' so it cannot be enabled on other boards?
> diff --git a/drivers/i2c/i2c-bcm2835.c b/drivers/i2c/i2c-bcm2835.c
> @@ -0,0 +1,370 @@
> + if (msgs[i].flags & I2C_M_IGNORE_NAK)
> + ignore_nak = true;
I don't think this works as intended. If any one message has the flag,
NAKs are ignored for the whole transfer. Also, if the NAK comes before
all the bytes have gone out, the DONE path sets msg_err to
BCM2835_I2C_S_LEN and you return -EIO anyway. The Linux driver doesn't
support I2C_M_IGNORE_NAK and I can't find anything in U-Boot that sets
it for this sort of bus, so I suggest dropping it.
> diff --git a/drivers/i2c/i2c-bcm2835.c b/drivers/i2c/i2c-bcm2835.c
> @@ -0,0 +1,370 @@
> + if (val & BCM2835_I2C_S_DONE) {
> + if (!i2c_dev->curr_msg) {
> + dev_err(dev, "unexpected completion status\n");
In Linux this catches an interrupt that arrives when no transfer is
running. Here we poll inside xfer() with curr_msg always set, so the
check can never trigger and can be removed.
> diff --git a/drivers/i2c/i2c-bcm2835.c b/drivers/i2c/i2c-bcm2835.c
> @@ -0,0 +1,370 @@
> + priv->regs = dev_read_addr_ptr(bus);
> + if (!priv->regs) {
> + printf("bcm2835_i2c: cannot get base address\n");
> + return -EINVAL;
> + }
Please use dev_err() here instead of printf(), as you do elsewhere in
the driver.
> diff --git a/drivers/i2c/i2c-bcm2835.c b/drivers/i2c/i2c-bcm2835.c
> @@ -0,0 +1,370 @@
> + bus_clk_rate = dev_read_u32_default(bus, "clock-frequency",
> + I2C_SPEED_STANDARD_RATE);
> + ret = bcm2835_i2c_set_rate(bus, bus_clk_rate);
> + if (ret)
> + return ret;
The uclass already does this: i2c_post_probe() reads 'clock-frequency'
(with the same default) and calls dm_i2c_set_bus_speed(), which calls
your set_bus_speed() op, so the divider is programmed twice. Please
can you drop this and keep only the mailbox query and the FIFO/status
clear in probe?
Regards,
Simon
