Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <a022c0590f0fbf22cc8476b5ef3f1c22746429ac.camel@yadro.com>
Subject: Re: [PATCH 3/3] watchdog/aspeed: add support for dual boot
From:   Ivan Mikhaylov <i.mikhaylov@yadro.com>
To:     Guenter Roeck <linux@roeck-us.net>
CC:     Wim Van Sebroeck <wim@linux-watchdog.org>,
        Joel Stanley <joel@jms.id.au>,
        Andrew Jeffery <andrew@aj.id.au>,
        <linux-watchdog@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-aspeed@lists.ozlabs.org>, <linux-kernel@vger.kernel.org>,
        "Alexander Amelkin" <a.amelkin@yadro.com>
Date:   Thu, 22 Aug 2019 12:15:20 +0300
In-Reply-To: <20190821163220.GA11547@roeck-us.net>
References: <1f2cd155057e5ab0cdb20a9a11614bbb09bb49ad.camel@yadro.com>
         <20190821163220.GA11547@roeck-us.net>
Organization: YADRO
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.30.5 (3.30.5-1.fc29) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, 2019-08-21 at 09:32 -0700, Guenter Roeck wrote:
> 
> > +	writel(WDT_CLEAR_TIMEOUT_AND_BOOT_CODE_SELECTION,
> > +			wdt->base + WDT_CLEAR_TIMEOUT_STATUS);
> > +	wdt->wdd.bootstatus |= WDIOF_EXTERN1;
> 
> The variable reflects the _boot status_. It should not change after booting.

Okay, then perhaps may we set 'status' handler for watchdog device and check 
'status' file? Right now 'bootstatus' and 'status' are same because there is no
handler for 'status'.

> > +
> > +	return size;
> > +}
> > +static DEVICE_ATTR_WO(access_cs0);
> > +
> > +static struct attribute *bswitch_attrs[] = {
> > +	&dev_attr_access_cs0.attr,
> > +	NULL
> > +};
> > +ATTRIBUTE_GROUPS(bswitch);
> > +
> >  static const struct watchdog_ops aspeed_wdt_ops = {
> >  	.start		= aspeed_wdt_start,
> >  	.stop		= aspeed_wdt_stop,
> > @@ -223,6 +248,9 @@ static int aspeed_wdt_probe(struct platform_device
> > *pdev)
> >  
> >  	wdt->ctrl = WDT_CTRL_1MHZ_CLK;
> >  
> > +	if (of_property_read_bool(np, "aspeed,alt-boot"))
> > +		wdt->wdd.groups = bswitch_groups;
> > +
> Why does this have to be separate to the existing evaluation of
> aspeed,alt-boot, and why does the existing code not work ?
> 
> Also, is it guaranteed that this does not interfer with existing
> support for alt-boot ?

It doesn't, it just provides for ast2400 switch to cs0 at side 1(cs1). Problem
is that only one flash chip(side 1/cs1) is accessible on alternate boot, there
is citation from the documentation in commit body. So if by some reason side 0
is corrupted, need to switch into alternate boot to cs1, do the load from it,
drop that bit to make side 0 accessible and do the flash of first side. On
ast2500/2600 this problem is solved already, in alternate boot there both flash
chips are present. It's additional requirement for alternate boot on ast2400, to
make the possibility to access at all side 0 flash chip after we boot to the
alternate side.