| Message ID | 20211102152207.11891-1-zajec5@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | reset: syscon-reboot: add "reg" property support | expand |
On 02.11.2021 16:22, Rafał Miłecki wrote: > During my work on MFD binding for Broadcom's TWD block I received > comment from Rob saying that "syscon-reboot" should use "reg" property. > I'm not sure if my understanding & implementation are correct so I'm > sending this RFC. Reference: Re: [PATCH 2/2] dt-bindings: mfd: add Broadcom's timer MFD block Link: https://www.spinics.net/lists/linux-watchdog/msg21255.html
On Tue, Nov 2, 2021 at 10:22 AM Rafał Miłecki <zajec5@gmail.com> wrote: > > From: Rafał Miłecki <rafal@milecki.pl> > > During my work on MFD binding for Broadcom's TWD block I received > comment from Rob saying that "syscon-reboot" should use "reg" property. > I'm not sure if my understanding & implementation are correct so I'm > sending this RFC. > > What bothers me is non-standard "reg" property usage. Treating it as a > simple (unsigned) integer number means different logic when it comes to > ranges. It shouldn't be. The idea is that 'reg' works like normal. See below. > Consider this example: > > timer@400 { > compatible = "simple-mfd", "syscon"; > reg = <0x400 0x3c>; > ranges; ranges = <0 0x400 0x100>; // Just guessing for size > > #address-cells = <1>; > #size-cells = <1>; > > reboot@434 { reboot@34 Just reading 'reg' is fine, but really, Linux should be either getting the translated address or have a function to get the offset from the parent base. IOW, it should also work if you just changed 'reg' to '<0x434 0x4>'. > compatible = "syscon-reboot"; > reg = <0x34 0x4>; > mask = <0x1>; > }; > }; > > I've reboot@434 node with reg 0x34. Also 0x4 is ignored but must be > present because of of MFD addressing. > > Please review this idea / binding / implementation. > > Rafał Miłecki (3): > dt-bindings: power: reset: syscon-reboot: use non-deprecated example > dt-bindings: power: reset: syscon-reboot: add "reg" property > power: reset: syscon-reboot: support "reg" property > > .../bindings/power/reset/syscon-reboot.yaml | 28 +++++++++++++------ > drivers/power/reset/syscon-reboot.c | 9 ++++-- > 2 files changed, 26 insertions(+), 11 deletions(-) > > -- > 2.31.1 >
On 12.11.2021 23:18, Rob Herring wrote: > On Tue, Nov 2, 2021 at 10:22 AM Rafał Miłecki <zajec5@gmail.com> wrote: >> >> From: Rafał Miłecki <rafal@milecki.pl> >> >> During my work on MFD binding for Broadcom's TWD block I received >> comment from Rob saying that "syscon-reboot" should use "reg" property. >> I'm not sure if my understanding & implementation are correct so I'm >> sending this RFC. >> >> What bothers me is non-standard "reg" property usage. Treating it as a >> simple (unsigned) integer number means different logic when it comes to >> ranges. > > It shouldn't be. The idea is that 'reg' works like normal. See below. > >> Consider this example: >> >> timer@400 { >> compatible = "simple-mfd", "syscon"; >> reg = <0x400 0x3c>; >> ranges; > > ranges = <0 0x400 0x100>; // Just guessing for size > >> >> #address-cells = <1>; >> #size-cells = <1>; >> >> reboot@434 { > > reboot@34 > > Just reading 'reg' is fine, but really, Linux should be either getting > the translated address or have a function to get the offset from the > parent base. IOW, it should also work if you just changed 'reg' to > '<0x434 0x4>'. Are you aware of anyone working on support for getting translated address? Do you recall any efforts on implementing such a helper? Thanks a lot for commenting on this concern explicitly.
On Fri, Nov 12, 2021 at 4:23 PM Rafał Miłecki <zajec5@gmail.com> wrote: > > On 12.11.2021 23:18, Rob Herring wrote: > > On Tue, Nov 2, 2021 at 10:22 AM Rafał Miłecki <zajec5@gmail.com> wrote: > >> > >> From: Rafał Miłecki <rafal@milecki.pl> > >> > >> During my work on MFD binding for Broadcom's TWD block I received > >> comment from Rob saying that "syscon-reboot" should use "reg" property. > >> I'm not sure if my understanding & implementation are correct so I'm > >> sending this RFC. > >> > >> What bothers me is non-standard "reg" property usage. Treating it as a > >> simple (unsigned) integer number means different logic when it comes to > >> ranges. > > > > It shouldn't be. The idea is that 'reg' works like normal. See below. > > > >> Consider this example: > >> > >> timer@400 { > >> compatible = "simple-mfd", "syscon"; > >> reg = <0x400 0x3c>; > >> ranges; > > > > ranges = <0 0x400 0x100>; // Just guessing for size > > > >> > >> #address-cells = <1>; > >> #size-cells = <1>; > >> > >> reboot@434 { > > > > reboot@34 > > > > Just reading 'reg' is fine, but really, Linux should be either getting > > the translated address or have a function to get the offset from the > > parent base. IOW, it should also work if you just changed 'reg' to > > '<0x434 0x4>'. > > Are you aware of anyone working on support for getting translated > address? Do you recall any efforts on implementing such a helper? All the DT address functions give you translated addresses. It's the latter that doesn't exist that I'm aware of offhand. It's just of_address_to_resource() on the child and parent nodes and then calculate the offset. Rob
Hi, On Fri, Nov 12, 2021 at 04:32:48PM -0600, Rob Herring wrote: > On Fri, Nov 12, 2021 at 4:23 PM Rafał Miłecki <zajec5@gmail.com> wrote: > > > > On 12.11.2021 23:18, Rob Herring wrote: > > > On Tue, Nov 2, 2021 at 10:22 AM Rafał Miłecki <zajec5@gmail.com> wrote: > > >> > > >> From: Rafał Miłecki <rafal@milecki.pl> > > >> > > >> During my work on MFD binding for Broadcom's TWD block I received > > >> comment from Rob saying that "syscon-reboot" should use "reg" property. > > >> I'm not sure if my understanding & implementation are correct so I'm > > >> sending this RFC. > > >> > > >> What bothers me is non-standard "reg" property usage. Treating it as a > > >> simple (unsigned) integer number means different logic when it comes to > > >> ranges. > > > > > > It shouldn't be. The idea is that 'reg' works like normal. See below. > > > > > >> Consider this example: > > >> > > >> timer@400 { > > >> compatible = "simple-mfd", "syscon"; > > >> reg = <0x400 0x3c>; > > >> ranges; > > > > > > ranges = <0 0x400 0x100>; // Just guessing for size > > > > > >> > > >> #address-cells = <1>; > > >> #size-cells = <1>; > > >> > > >> reboot@434 { > > > > > > reboot@34 > > > > > > Just reading 'reg' is fine, but really, Linux should be either getting > > > the translated address or have a function to get the offset from the > > > parent base. IOW, it should also work if you just changed 'reg' to > > > '<0x434 0x4>'. > > > > Are you aware of anyone working on support for getting translated > > address? Do you recall any efforts on implementing such a helper? > > All the DT address functions give you translated addresses. It's the > latter that doesn't exist that I'm aware of offhand. It's just > of_address_to_resource() on the child and parent nodes and then > calculate the offset. Has a new version beent sent with this change, that I missed? -- Sebastian
Hi, On 02.12.2021 18:21, Sebastian Reichel wrote: > On Fri, Nov 12, 2021 at 04:32:48PM -0600, Rob Herring wrote: >> On Fri, Nov 12, 2021 at 4:23 PM Rafał Miłecki <zajec5@gmail.com> wrote: >>> >>> On 12.11.2021 23:18, Rob Herring wrote: >>>> On Tue, Nov 2, 2021 at 10:22 AM Rafał Miłecki <zajec5@gmail.com> wrote: >>>>> >>>>> From: Rafał Miłecki <rafal@milecki.pl> >>>>> >>>>> During my work on MFD binding for Broadcom's TWD block I received >>>>> comment from Rob saying that "syscon-reboot" should use "reg" property. >>>>> I'm not sure if my understanding & implementation are correct so I'm >>>>> sending this RFC. >>>>> >>>>> What bothers me is non-standard "reg" property usage. Treating it as a >>>>> simple (unsigned) integer number means different logic when it comes to >>>>> ranges. >>>> >>>> It shouldn't be. The idea is that 'reg' works like normal. See below. >>>> >>>>> Consider this example: >>>>> >>>>> timer@400 { >>>>> compatible = "simple-mfd", "syscon"; >>>>> reg = <0x400 0x3c>; >>>>> ranges; >>>> >>>> ranges = <0 0x400 0x100>; // Just guessing for size >>>> >>>>> >>>>> #address-cells = <1>; >>>>> #size-cells = <1>; >>>>> >>>>> reboot@434 { >>>> >>>> reboot@34 >>>> >>>> Just reading 'reg' is fine, but really, Linux should be either getting >>>> the translated address or have a function to get the offset from the >>>> parent base. IOW, it should also work if you just changed 'reg' to >>>> '<0x434 0x4>'. >>> >>> Are you aware of anyone working on support for getting translated >>> address? Do you recall any efforts on implementing such a helper? >> >> All the DT address functions give you translated addresses. It's the >> latter that doesn't exist that I'm aware of offhand. It's just >> of_address_to_resource() on the child and parent nodes and then >> calculate the offset. > > Has a new version beent sent with this change, that I missed? Not yet, I was sidetracked by pinctrl & OpenWrt stuff.
From: Rafał Miłecki <rafal@milecki.pl> During my work on MFD binding for Broadcom's TWD block I received comment from Rob saying that "syscon-reboot" should use "reg" property. I'm not sure if my understanding & implementation are correct so I'm sending this RFC. What bothers me is non-standard "reg" property usage. Treating it as a simple (unsigned) integer number means different logic when it comes to ranges. Consider this example: timer@400 { compatible = "simple-mfd", "syscon"; reg = <0x400 0x3c>; ranges; #address-cells = <1>; #size-cells = <1>; reboot@434 { compatible = "syscon-reboot"; reg = <0x34 0x4>; mask = <0x1>; }; }; I've reboot@434 node with reg 0x34. Also 0x4 is ignored but must be present because of of MFD addressing. Please review this idea / binding / implementation. Rafał Miłecki (3): dt-bindings: power: reset: syscon-reboot: use non-deprecated example dt-bindings: power: reset: syscon-reboot: add "reg" property power: reset: syscon-reboot: support "reg" property .../bindings/power/reset/syscon-reboot.yaml | 28 +++++++++++++------ drivers/power/reset/syscon-reboot.c | 9 ++++-- 2 files changed, 26 insertions(+), 11 deletions(-)