From: Guenter Roeck <linux@roeck-us.net> To: Ivan Mikhaylov <i.mikhaylov@yadro.com> 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> Subject: Re: [PATCH 3/3] watchdog/aspeed: add support for dual boot Date: Thu, 22 Aug 2019 06:55:28 -0700 [thread overview] Message-ID: <20190822135528.GB8144@roeck-us.net> (raw) In-Reply-To: <a022c0590f0fbf22cc8476b5ef3f1c22746429ac.camel@yadro.com> On Thu, Aug 22, 2019 at 12:15:20PM +0300, Ivan Mikhaylov wrote: > 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'. > You would still have to redefine one of the status bits to mean something driver specific. You would also still have two different flags to read and control cs0 - to read the status, you would read an ioctl (or the status sysfs attribute), to write it you would write into access_cs0. I guess I must be missing something. What is the problem with using access_cs0 for both ? Guenter > > > + > > > + 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. >
WARNING: multiple messages have this Message-ID (diff)
From: Guenter Roeck <linux@roeck-us.net> To: Ivan Mikhaylov <i.mikhaylov@yadro.com> Cc: linux-watchdog@vger.kernel.org, linux-aspeed@lists.ozlabs.org, Andrew Jeffery <andrew@aj.id.au>, Alexander Amelkin <a.amelkin@yadro.com>, linux-kernel@vger.kernel.org, Joel Stanley <joel@jms.id.au>, Wim Van Sebroeck <wim@linux-watchdog.org>, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 3/3] watchdog/aspeed: add support for dual boot Date: Thu, 22 Aug 2019 06:55:28 -0700 [thread overview] Message-ID: <20190822135528.GB8144@roeck-us.net> (raw) In-Reply-To: <a022c0590f0fbf22cc8476b5ef3f1c22746429ac.camel@yadro.com> On Thu, Aug 22, 2019 at 12:15:20PM +0300, Ivan Mikhaylov wrote: > 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'. > You would still have to redefine one of the status bits to mean something driver specific. You would also still have two different flags to read and control cs0 - to read the status, you would read an ioctl (or the status sysfs attribute), to write it you would write into access_cs0. I guess I must be missing something. What is the problem with using access_cs0 for both ? Guenter > > > + > > > + 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. > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-08-22 13:55 UTC|newest] Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-08-21 15:57 [PATCH 3/3] watchdog/aspeed: add support for dual boot Ivan Mikhaylov 2019-08-21 15:57 ` Ivan Mikhaylov 2019-08-21 16:32 ` Guenter Roeck 2019-08-21 16:32 ` Guenter Roeck 2019-08-21 17:42 ` Alexander Amelkin 2019-08-21 17:42 ` Alexander Amelkin 2019-08-21 18:10 ` Guenter Roeck 2019-08-21 18:10 ` Guenter Roeck 2019-08-22 14:36 ` Alexander Amelkin 2019-08-22 14:36 ` Alexander Amelkin 2019-08-22 16:01 ` Guenter Roeck 2019-08-22 16:01 ` Guenter Roeck 2019-08-22 16:45 ` Alexander Amelkin 2019-08-22 16:45 ` Alexander Amelkin 2019-08-22 9:15 ` Ivan Mikhaylov 2019-08-22 9:15 ` Ivan Mikhaylov 2019-08-22 13:55 ` Guenter Roeck [this message] 2019-08-22 13:55 ` Guenter Roeck 2019-08-22 14:24 ` Ivan Mikhaylov 2019-08-22 14:24 ` Ivan Mikhaylov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190822135528.GB8144@roeck-us.net \ --to=linux@roeck-us.net \ --cc=a.amelkin@yadro.com \ --cc=andrew@aj.id.au \ --cc=i.mikhaylov@yadro.com \ --cc=joel@jms.id.au \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-aspeed@lists.ozlabs.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-watchdog@vger.kernel.org \ --cc=wim@linux-watchdog.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.