| Message ID | 1436278217-20522-7-git-send-email-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Simon Horman |
| Headers | show |
Hi Geert, Thank you for the patches. On Tuesday 07 July 2015 16:10:12 Geert Uytterhoeven wrote: > Add a link from the Clock Pulse Generator node to the Reset Controller > node, so the CPG can read the Mode Monitoring Register (MODEMR) to > obtain the MD pin values.x > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > arch/arm/boot/dts/r8a7790.dtsi | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi > index 08067ae177643b8f..4ee5523fc3e13e12 100644 > --- a/arch/arm/boot/dts/r8a7790.dtsi > +++ b/arch/arm/boot/dts/r8a7790.dtsi > @@ -1115,6 +1115,7 @@ > "lb", "qspi", "sdh", "sd0", "sd1", > "z", "rcan", "adsp"; > #power-domain-cells = <0>; > + renesas,modemr = <&rst 0x60>; I have mixed feelings about this as I don't think it really describes the hardware. Wouldn't it make sense to instead add a standard kernel API to retrieve the boot mode value ? It seems to be a pretty common feature of SoCs across all vendors. > }; > > /* Variable factor clocks */
Hi Laurent, On Tue, Jul 7, 2015 at 4:23 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Tuesday 07 July 2015 16:10:12 Geert Uytterhoeven wrote: >> Add a link from the Clock Pulse Generator node to the Reset Controller >> node, so the CPG can read the Mode Monitoring Register (MODEMR) to >> obtain the MD pin values.x >> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >> --- >> arch/arm/boot/dts/r8a7790.dtsi | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi >> index 08067ae177643b8f..4ee5523fc3e13e12 100644 >> --- a/arch/arm/boot/dts/r8a7790.dtsi >> +++ b/arch/arm/boot/dts/r8a7790.dtsi >> @@ -1115,6 +1115,7 @@ >> "lb", "qspi", "sdh", "sd0", "sd1", >> "z", "rcan", "adsp"; >> #power-domain-cells = <0>; >> + renesas,modemr = <&rst 0x60>; > > I have mixed feelings about this as I don't think it really describes the > hardware. From the R-Car Gen2 manual: 8. Reset (RST) 8.1 Features The following functions are implemented by RST. [...] Latching of the levels on mode pins when PRESET# is negated Mode monitoring register 7. Clock Pulse Generator (CPG) 7.2 Input/Output Pins Table 7.1 lists the CPG pin configuration. Table 7.1 Pin Configuration and Functions of CPG Pin Name Function I/O Description [...] MD0 Mode 0 ... Hence there definitely is a link between the (latched) values in the RST module and CPG configuration. This link is expressed using the "renesas,modemr" property, where the phandle provides the link to the RST block, and the register offset provides a way for software to read the configuration. > Wouldn't it make sense to instead add a standard kernel API to retrieve the > boot mode value ? It seems to be a pretty common feature of SoCs across all > vendors. What format would the boot mode value be in? One u32 word, an array of u32s? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On Tuesday 07 July 2015 16:58:21 Geert Uytterhoeven wrote: > On Tue, Jul 7, 2015 at 4:23 PM, Laurent Pinchart wrote: > > On Tuesday 07 July 2015 16:10:12 Geert Uytterhoeven wrote: > >> Add a link from the Clock Pulse Generator node to the Reset Controller > >> node, so the CPG can read the Mode Monitoring Register (MODEMR) to > >> obtain the MD pin values.x > >> > >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > >> --- > >> > >> arch/arm/boot/dts/r8a7790.dtsi | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/arch/arm/boot/dts/r8a7790.dtsi > >> b/arch/arm/boot/dts/r8a7790.dtsi index > >> 08067ae177643b8f..4ee5523fc3e13e12 100644 > >> --- a/arch/arm/boot/dts/r8a7790.dtsi > >> +++ b/arch/arm/boot/dts/r8a7790.dtsi > >> @@ -1115,6 +1115,7 @@ > >> "lb", "qspi", "sdh", "sd0", > >> "sd1", > >> "z", "rcan", "adsp"; > >> #power-domain-cells = <0>; > >> + renesas,modemr = <&rst 0x60>; > > > > I have mixed feelings about this as I don't think it really describes the > > hardware. > > From the R-Car Gen2 manual: > > 8. Reset (RST) > 8.1 Features > The following functions are implemented by RST. > [...] > Latching of the levels on mode pins when PRESET# is negated > Mode monitoring register > > 7. Clock Pulse Generator (CPG) > 7.2 Input/Output Pins > Table 7.1 lists the CPG pin configuration. > Table 7.1 Pin Configuration and Functions of CPG > Pin Name Function I/O Description > [...] > MD0 Mode 0 ... > > Hence there definitely is a link between the (latched) values in the RST > module and CPG configuration. This link is expressed using the > "renesas,modemr" property, where the phandle provides the link to the RST > block, and the register offset provides a way for software to read the > configuration. The mode bits of course influence the CPG (otherwise we wouldn't be having this discussion :-)), but to me it looks more like a configuration broadcast through the whole SoC than a real link between two IP cores. It's obviously subject to interpretation. > > Wouldn't it make sense to instead add a standard kernel API to retrieve > > the boot mode value ? It seems to be a pretty common feature of SoCs > > across all vendors. > > What format would the boot mode value be in? One u32 word, an array > of u32s? I'd go for a u32 as that's what all the vendors I came across use. It could easily be extended to a u64 or an array of u32/u64 later if needed. The format of the boot mode value will be SoC-specific, but that's also true of the mode register that would be read through syscon.
Hi Laurent, On Tue, Jul 7, 2015 at 6:20 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: >> >> --- a/arch/arm/boot/dts/r8a7790.dtsi >> >> +++ b/arch/arm/boot/dts/r8a7790.dtsi >> >> @@ -1115,6 +1115,7 @@ >> >> "lb", "qspi", "sdh", "sd0", >> >> "sd1", >> >> "z", "rcan", "adsp"; >> >> #power-domain-cells = <0>; >> >> + renesas,modemr = <&rst 0x60>; >> > >> > I have mixed feelings about this as I don't think it really describes the >> > hardware. >> >> From the R-Car Gen2 manual: >> >> 8. Reset (RST) >> 8.1 Features >> The following functions are implemented by RST. >> [...] >> Latching of the levels on mode pins when PRESET# is negated >> Mode monitoring register >> >> 7. Clock Pulse Generator (CPG) >> 7.2 Input/Output Pins >> Table 7.1 lists the CPG pin configuration. >> Table 7.1 Pin Configuration and Functions of CPG >> Pin Name Function I/O Description >> [...] >> MD0 Mode 0 ... >> >> Hence there definitely is a link between the (latched) values in the RST >> module and CPG configuration. This link is expressed using the >> "renesas,modemr" property, where the phandle provides the link to the RST >> block, and the register offset provides a way for software to read the >> configuration. > > The mode bits of course influence the CPG (otherwise we wouldn't be having > this discussion :-)), but to me it looks more like a configuration broadcast > through the whole SoC than a real link between two IP cores. It's obviously > subject to interpretation. Yep. Broadcast? Like a bus clock? For that we also have properties with phandles... Syscon is the Hot New Abstraction for a module that provides a bunch of registers needed by drivers for other modules. Those other modules need some way to refer to the appropriate syscon register. The RST module definitely falls in that category: CPG needs the MODEMR register, watchdog (RWDT/SWDT) needs the Watchdog Timer Reset Control Register, and APMU probably needs the CA15/7 reset control registers. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On Wednesday 08 July 2015 10:29:56 Geert Uytterhoeven wrote: > On Tue, Jul 7, 2015 at 6:20 PM, Laurent Pinchart wrote: > >>>> --- a/arch/arm/boot/dts/r8a7790.dtsi > >>>> +++ b/arch/arm/boot/dts/r8a7790.dtsi > >>>> @@ -1115,6 +1115,7 @@ > >>>> "lb", "qspi", "sdh", "sd0", > >>>> "sd1", > >>>> "z", "rcan", "adsp"; > >>>> #power-domain-cells = <0>; > >>>> + renesas,modemr = <&rst 0x60>; > >>> > >>> I have mixed feelings about this as I don't think it really describes > >>> the hardware. > >> > >> From the R-Car Gen2 manual: > >> > >> 8. Reset (RST) > >> 8.1 Features > >> The following functions are implemented by RST. > >> [...] > >> Latching of the levels on mode pins when PRESET# is negated > >> Mode monitoring register > >> > >> 7. Clock Pulse Generator (CPG) > >> 7.2 Input/Output Pins > >> Table 7.1 lists the CPG pin configuration. > >> Table 7.1 Pin Configuration and Functions of CPG > >> Pin Name Function I/O Description > >> [...] > >> MD0 Mode 0 ... > >> > >> Hence there definitely is a link between the (latched) values in the RST > >> module and CPG configuration. This link is expressed using the > >> "renesas,modemr" property, where the phandle provides the link to the RST > >> block, and the register offset provides a way for software to read the > >> configuration. > > > > The mode bits of course influence the CPG (otherwise we wouldn't be having > > this discussion :-)), but to me it looks more like a configuration > > broadcast through the whole SoC than a real link between two IP cores. > > It's obviously subject to interpretation. > > Yep. > > Broadcast? Like a bus clock? For that we also have properties with > phandles... I knew someone would raise that topic ;-) > Syscon is the Hot New Abstraction for a module that provides a bunch of > registers needed by drivers for other modules. Those other modules need > some way to refer to the appropriate syscon register. > > The RST module definitely falls in that category: CPG needs the MODEMR > register, watchdog (RWDT/SWDT) needs the Watchdog Timer Reset Control > Register, and APMU probably needs the CA15/7 reset control registers. syscon is a quick hack to be used when no clean solution exists. Sometimes implementing a proper API is just overkill, especially when the "system controller" aggregates registers that really belong to individual IP cores. Proper abstract kernel APIs should of course be used wherever possible. Some system controllers are already supported in such a way, using the MFD subsystem and exposing proper APIs for part of their features, and letting drivers access registers directly for other features. In the boot mode case I believe adding a new API would be both useful and quite simple, so I'd like to explore that option. By the way, regarding the CA15/7 reset control registers, shouldn't they be exposed through the reset controller API ?
diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi index 08067ae177643b8f..4ee5523fc3e13e12 100644 --- a/arch/arm/boot/dts/r8a7790.dtsi +++ b/arch/arm/boot/dts/r8a7790.dtsi @@ -1115,6 +1115,7 @@ "lb", "qspi", "sdh", "sd0", "sd1", "z", "rcan", "adsp"; #power-domain-cells = <0>; + renesas,modemr = <&rst 0x60>; }; /* Variable factor clocks */
Add a link from the Clock Pulse Generator node to the Reset Controller node, so the CPG can read the Mode Monitoring Register (MODEMR) to obtain the MD pin values.x Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- arch/arm/boot/dts/r8a7790.dtsi | 1 + 1 file changed, 1 insertion(+)