Re: [PATCH v4 3/3] reboot-mode: read the boot mode from RTC memory

Simon Glass
2021-07-05T15:29:57Z

Hi Nandor,

On Mon, 28 Jun 2021 at 03:44, Nandor Han <nandor.han@vaisala.com> wrote:
>
> On 6/26/21 9:30 PM, Simon Glass wrote:
> > Hi Nandor,
> >
> > On Thu, 10 Jun 2021 at 07:57, Nandor Han <nandor.han@vaisala.com> wrote:
> >>
> >> RTC devices could provide battery-backed memory that can be used for
> >> storing the reboot mode magic value.
> >>
> >> Add a new reboot-mode back-end that uses RTC to store the reboot-mode
> >> magic value. The driver also supports both endianness modes.
> >>
> >> Signed-off-by: Nandor Han <nandor.han@vaisala.com>
> >> ---
> >> arch/sandbox/dts/test.dts | 10 ++
> >> configs/sandbox_defconfig | 1 +
> >> .../reboot-mode/reboot-mode-rtc.txt | 22 +++
> >> drivers/reboot-mode/Kconfig | 9 ++
> >> drivers/reboot-mode/Makefile | 1 +
> >> drivers/reboot-mode/reboot-mode-rtc.c | 127 ++++++++++++++++++
> >> include/reboot-mode/reboot-mode-rtc.h | 16 +++
> >> test/dm/reboot-mode.c | 29 ++++
> >> 8 files changed, 215 insertions(+)
> >> create mode 100644 doc/device-tree-bindings/reboot-mode/reboot-mode-rtc.txt
> >> create mode 100644 drivers/reboot-mode/reboot-mode-rtc.c
> >> create mode 100644 include/reboot-mode/reboot-mode-rtc.h
> >>
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > But I think it would be better to put this device as a child of the
> > rtc, so it is more obvious that they are related. It also avoids the
> > phandle. What do you think?
> >
>
> Hi Simon and thanks for reviewing the code,
> I think the idea is not bad, however, would be ok to consider the
> recommendation as improvement opportunity and merge this as
> it is for now? I'm a bit busy with some other stuff now and I could
> start implementing this some time later. At least it would help me since
> this are on the mailing list since Dec 2019 :).

Well I don't think you could change this later because it would change
the binding. I suppose you could add it as an option.

I am OK with either way (hence my reviewed-by tag). With my use of RTC
for data storage I have made it a subnode since it makes it easy to
see which parts of the RTC are used for what:

rtc: rtc {
compatible = "motorola,mc146818";
u-boot,dm-pre-proper;
reg = <0x70 2>;
#address-cells = <1>;
#size-cells = <0>;
nvdata {
u-boot,dm-vpl;
compatible = "google,cmos-nvdata";
reg = <0x26>;
nvdata,types = <CROS_NV_DATA>;
};
};

Regards,
Simon