Re: [PATCH u-boot v2 1/2] drivers: spi: Add support for disabling FMC_WDT2 for aspeed

Marc Olberding
2025-12-17T01:15:50Z

On Wed, Dec 17, 2025 at 09:53:33AM +1030, Andrew Jeffery wrote:
> On Tue, 2025-12-02 at 15:52 -0800, Marc Olberding wrote:
> > Adds support for disabling the ast2600 FMC_WDT2 through
> > a device tree entry in the fmc node.
> > Set `aspeed,watchdog-disable` in your device tree to have
> > the driver disable it.
> >
> > Signed-off-by: Marc Olberding <molberding@nvidia.com>
> > ---
> >  drivers/spi/aspeed_spi.c | 19 +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/spi/aspeed_spi.c b/drivers/spi/aspeed_spi.c
> > index 54520122f1c48c8b2052b4b1e47445a9b990d25e..de954e477aa15e6d1be042a2aee47f5a501178da 100644
> > --- a/drivers/spi/aspeed_spi.c
> > +++ b/drivers/spi/aspeed_spi.c
> > @@ -30,14 +30,16 @@ struct aspeed_spi_regs {
> >   /* 0x30 .. 0x38 Segment Address */
> >   u32 _reserved1[5]; /* .. */
> >   u32 soft_rst_cmd_ctrl; /* 0x50 Auto Soft-Reset Command Control */
> > - u32 _reserved2[11]; /* .. */
> > + u32 _reserved2[4]; /* .. */
> > + u32 wdt2_ctrl; /* 0x64 FMC_WDT2 control */
> > + u32 _reserved3[6]; /* .. */
>
> Ugh (passing commentary, not your fault).
>
Yeah this sucks, if you have a better idea, let me know.

> >   /*
> >   * Set safe default settings for each device. These will be
> >   * tuned after the SPI flash devices are probed.
> > @@ -1907,6 +1914,10 @@ static int aspeed_spi_probe(struct udevice *bus)
> >   * SPI controllers
> >   */
> >   priv->is_fmc = dev_get_driver_data(bus);
> > + if (device_is_compatible(bus, "aspeed,ast2600-fmc") &&
> > +     dev_read_bool(bus, "aspeed,watchdog-disable"))
> > + priv->disable_wdt = true;
>
> We're not setting it to false, just declaring it above, which means if
> this branch isn't taken then its value is undefined.
>
> Perhaps initialise it to false by default.

Ah, I figured that priv would've been callocd by the driver framework
before being passed to probe.
I'll change this to
priv->disable_wdt = <boolean expression>

in the next rev. Thanks.

> > +
>
> Unnecessary new-line?
>
> >
> >   ret = aspeed_spi_controller_init(priv);
> >   if (ret)
ack, will fix.

Thanks for the review.