Hi Simon, Thanks for the in-depth review! Your points make perfect sense, especially regarding the U-Boot specific polling behavior versus Linux's interrupt-driven logic.
I will incorporate all your suggestions into the next version: - Refactoring bcm2835_get_mmc_clock() to avoid duplication. - Adding the 'depends on ARCH_BCM283X' in Kconfig. - Dropping the I2C_M_IGNORE_NAK flag and the curr_msg NULL check. - Using dev_err() instead of printf(). - Removing the redundant set_bus_speed() call in probe(). Hi Peter, Noted on the threading rule! I will make sure to send the next version as a fresh, unthreaded post. I will also split the changes into a 2-patch series (one for the driver, one for the defconfigs) as requested. I will hold off on sending v4 for a few days to see if there is any further feedback. Best regards, Giau Nguyen -------------------------------------------------------- Phone: (+84) 387 915 996 <+84%20387%20915%20996> On Sat, Oct 31, 2026 at 8:59 PM Simon Glass <[email protected]> wrote: > 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 >
