Re: [PATCH] drivers, char: add U-Boot bootcount driver

Re: [PATCH] drivers, char: add U-Boot bootcount driver

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2308 bytes --]

On Sun, Dec 04, 2011 at 10:45:21AM +0100, Heiko Schocher wrote:
> This driver implements the Linux kernel half of the boot count feature -
> the boot counter can only be reset after it is clear that the
> application has been started and is running correctly, which usually
> can only be determined by the application code itself. Thus the reset
> of the boot counter must be done by application code, which thus needs
> an appropriate driver.

An appropriate mechanism, not necessarily a driver, see below.

> Required feature by the Carrier Grade Linux Requirements Definition;
> see for example document "Carrier Grade Linux Requirements Definition
> Overview V3.0" at
> 
> http://www.linuxfoundation.org/collaborate/workgroups/cgl/requirements#SMM.6.0_Boot_Cycle_Detection
> 
>       Description: OSDL CGL specifies that carrier grade Linux
>       shall provide support for detecting a repeating reboot cycle
> 	due to recurring failures. This detection should happen in
> 	user space before system services are started.

So, technically, a flag would be enough, not necessarily a counter? Although a
counter probably has more advantages...

> This driver provides read/write access to the U-Boot bootcounter
> through PROC FS and/or sysFS file.

Why ProcFS? Why ProcFS and/or SysFS? Which has priority? Why not /dev?

> The bootcountregister gets configured via DTS.
> for example on the enbw_cmc board:
> 
> bootcount@0x23060 {
>                   compatible = "uboot,bootcount";

No. I assume you are not the vendor of what is at 0x23060, the actual device.
Only the device must be encoded in the compatible-entry which then implies the
bootcount functionality. Also, keep in mind that your solution should be
generic for bootloaders.

>                   reg = <0x23060 0x20>;

I assume that non-volatile memory would qualify as a boot-counter, so those
could be tied to I2C busses etc? reg would not fit then.

I do wonder if it makes more sense to add such functionality to the
watchdog-core to save the additional device (CCed). Needs a second thought,
though...

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] drivers, char: add U-Boot bootcount driver

[Wolfgang Denk]

Dear Wolfram,

in message <20111204114741.GA5788@pengutronix.de> you wrote:
> 
> >       Description: OSDL CGL specifies that carrier grade Linux
> >       shall provide support for detecting a repeating reboot cycle
> > 	due to recurring failures. This detection should happen in
> > 	user space before system services are started.
>
> So, technically, a flag would be enough, not necessarily a counter? Although a
> counter probably has more advantages...

The real-life applications we have seen so far all required a counter.
They all required to switch to a recovery mode or other alternative
boot sequence after N failed boot attempts, with N > 1 in all cases.

> >                   reg = <0x23060 0x20>;
>
> I assume that non-volatile memory would qualify as a boot-counter, so those
> could be tied to I2C busses etc? reg would not fit then.

Actually all kind of non-volatile storage can be used - it depends
on specific hardware roperties.  Some SoCs have registers that are
guaranteed not to change theier value during a reset; on other systems
we have storage or NVRAM in RTCs or similar, or we can use SRAM, or
I2C or SPI attached EEPROM, or even storage on SDCard, USB or other
storage devices.

This binding covers the memory type only.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
"There is nothing new under the sun, but there are lots of old things
we don't know yet."                                  - Ambrose Bierce
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drivers, char: add U-Boot bootcount driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 420 bytes --]

* Wolfram Sang wrote:
> On Sun, Dec 04, 2011 at 10:45:21AM +0100, Heiko Schocher wrote:
[...]
> >                   reg = <0x23060 0x20>;
> 
> I assume that non-volatile memory would qualify as a boot-counter, so those
> could be tied to I2C busses etc? reg would not fit then.

This is not really on-topic, but in the device-tree the "reg" property is
used to encode an I2C device's slave address.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] drivers, char: add U-Boot bootcount driver

[Heiko Schocher]

Hello Wolfram,

Wolfram Sang wrote:
> On Sun, Dec 04, 2011 at 10:45:21AM +0100, Heiko Schocher wrote:
>> This driver implements the Linux kernel half of the boot count feature -
>> the boot counter can only be reset after it is clear that the
>> application has been started and is running correctly, which usually
>> can only be determined by the application code itself. Thus the reset
>> of the boot counter must be done by application code, which thus needs
>> an appropriate driver.
> 
> An appropriate mechanism, not necessarily a driver, see below.
> 
>> Required feature by the Carrier Grade Linux Requirements Definition;
>> see for example document "Carrier Grade Linux Requirements Definition
>> Overview V3.0" at
>>
>> http://www.linuxfoundation.org/collaborate/workgroups/cgl/requirements#SMM.6.0_Boot_Cycle_Detection
>>
>>       Description: OSDL CGL specifies that carrier grade Linux
>>       shall provide support for detecting a repeating reboot cycle
>> 	due to recurring failures. This detection should happen in
>> 	user space before system services are started.
> 
> So, technically, a flag would be enough, not necessarily a counter? Although a
> counter probably has more advantages...
> 
>> This driver provides read/write access to the U-Boot bootcounter
>> through PROC FS and/or sysFS file.
> 
> Why ProcFS? Why ProcFS and/or SysFS? Which has priority? Why not /dev?

I drop the ProcFS support for v2.

>> The bootcountregister gets configured via DTS.
>> for example on the enbw_cmc board:
>>
>> bootcount@0x23060 {
>>                   compatible = "uboot,bootcount";
> 
> No. I assume you are not the vendor of what is at 0x23060, the actual device.
> Only the device must be encoded in the compatible-entry which then implies the
> bootcount functionality. Also, keep in mind that your solution should be
> generic for bootloaders.

So I should call it compatible = "generic, bootcount" ?

>>                   reg = <0x23060 0x20>;
> 
> I assume that non-volatile memory would qualify as a boot-counter, so those
> could be tied to I2C busses etc? reg would not fit then.

Currently, mem only supported, add this to the Documentation and log
message.

> I do wonder if it makes more sense to add such functionality to the
> watchdog-core to save the additional device (CCed). Needs a second thought,
> though...

Thanks!

bye,
heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Re: [PATCH] drivers, char: add U-Boot bootcount driver

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1464 bytes --]

Hi Heiko,

> >> This driver provides read/write access to the U-Boot bootcounter
> >> through PROC FS and/or sysFS file.
> > 
> > Why ProcFS? Why ProcFS and/or SysFS? Which has priority? Why not /dev?
> 
> I drop the ProcFS support for v2.

Don't bother. This approach starts from the wrong side.

> 
> >> The bootcountregister gets configured via DTS.
> >> for example on the enbw_cmc board:
> >>
> >> bootcount@0x23060 {
> >>                   compatible = "uboot,bootcount";
> > 
> > No. I assume you are not the vendor of what is at 0x23060, the actual device.
> > Only the device must be encoded in the compatible-entry which then implies the
> > bootcount functionality. Also, keep in mind that your solution should be
> > generic for bootloaders.
> 
> So I should call it compatible = "generic, bootcount" ?

Nope, you should give it the name of the device. Remember that 'compatible' is
no 1:1 replacement for platform_driver-binding. Check
http://devicetree.org/Device_Tree_Usage, especially the sections about the
compatible-property.

bootcount itself is not a device. It is a feature of certain devices. And that
needs to be implemented; possibly generic enough that it can work for register
based, i2c based, and so forth, accesses.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] drivers, char: add U-Boot bootcount driver

[Wolfgang Denk]

Dear Wolfram,

In message <20111206215056.GD14154@pengutronix.de> you wrote:
> 
> bootcount itself is not a device. It is a feature of certain devices. And that
> needs to be implemented; possibly generic enough that it can work for register
> based, i2c based, and so forth, accesses.

If "boot counter" is not a good name for such a device, then what name
would you suggest?

Or do you think a counter (which can be implemented in a number of
different ways, depending on hardware specifics) is not a device?
What would be such a device, then?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
"How is this place run - is it an anarchy?"
"No, I wouldn't say so; it is not that well organised..."
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drivers, char: add U-Boot bootcount driver

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 807 bytes --]


> > bootcount itself is not a device. It is a feature of certain devices. And that
> > needs to be implemented; possibly generic enough that it can work for register
> > based, i2c based, and so forth, accesses.
> 
> If "boot counter" is not a good name for such a device, then what name
> would you suggest?

None.

> Or do you think a counter (which can be implemented in a number of
> different ways, depending on hardware specifics) is not a device?

Yes.

> What would be such a device, then?

"maxim,ds1338"

Please have a look at the devicetree.org-wiki-page I just mentioned:

http://devicetree.org/Device_Tree_Usage

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] drivers, char: add U-Boot bootcount driver

[Rob Herring]

On 12/06/2011 04:06 PM, Wolfram Sang wrote:
> 
>>> bootcount itself is not a device. It is a feature of certain devices. And that
>>> needs to be implemented; possibly generic enough that it can work for register
>>> based, i2c based, and so forth, accesses.
>>
>> If "boot counter" is not a good name for such a device, then what name
>> would you suggest?
> 
> None.
> 
>> Or do you think a counter (which can be implemented in a number of
>> different ways, depending on hardware specifics) is not a device?
> 
> Yes.
> 
>> What would be such a device, then?
> 
> "maxim,ds1338"
> 
> Please have a look at the devicetree.org-wiki-page I just mentioned:
> 
> http://devicetree.org/Device_Tree_Usage
> 

Perhaps fs/pstore would be a good choice for the user space interface
(defining a new file bootcount). This can support any arbitrary backing
device although pretty much only ACPI is implemented.

Rob

Re: [PATCH] drivers, char: add U-Boot bootcount driver

[Heiko Schocher]

Hello Rob,

Sorry for the late reply ...

Rob Herring wrote:
> On 12/06/2011 04:06 PM, Wolfram Sang wrote:
>>>> bootcount itself is not a device. It is a feature of certain devices. And that
>>>> needs to be implemented; possibly generic enough that it can work for register
>>>> based, i2c based, and so forth, accesses.
>>> If "boot counter" is not a good name for such a device, then what name
>>> would you suggest?
>> None.
>>
>>> Or do you think a counter (which can be implemented in a number of
>>> different ways, depending on hardware specifics) is not a device?
>> Yes.
>>
>>> What would be such a device, then?
>> "maxim,ds1338"
>>
>> Please have a look at the devicetree.org-wiki-page I just mentioned:
>>
>> http://devicetree.org/Device_Tree_Usage
>>
> 
> Perhaps fs/pstore would be a good choice for the user space interface
> (defining a new file bootcount). This can support any arbitrary backing
> device although pretty much only ACPI is implemented.

I tried to use fs/pstore for the bootcount feature, and I can mount
and read the bootcount value from the file, I created. But I could not
write to that file ... I only see the callback from
include/linux/pstore.h

struct pstore_info {
[...]
        int             (*write)(enum pstore_type_id type,
                        enum kmsg_dump_reason reason, u64 *id,
                        unsigned int part, size_t size, struct pstore_info *psi);

called, if I reboot ... do I miss something? Or is it not possible
to write to the files created through fs/pstore?

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany