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

[Marc Olberding <molberding@nvidia.com>]

On Wed, Dec 17, 2025 at 11:51:23AM +1030, Andrew Jeffery wrote:
> On Wed, 2025-12-17 at 11:48 +1030, Andrew Jeffery wrote:
> > On Tue, 2025-12-16 at 17:15 -0800, Marc Olberding wrote:
> > > 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..de954e477aa15e6d1be04
> > > > > 2a2aee47f5a501178da 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.
> > 
> > Oh, wait, the stack variable is just redundant? My eyes glazed over
> > that.
> 
> Nope, ignore that too, I misinterpreted the diff.
> 
> Ugh, sorry for the noise.

No worries, I think the unconditional assignment is cleaner
and I've already made the change. I had to go cleanup your device tree
comments anyways, I'll send out v3 once I flash to hardware.

Thanks,
Marc