| Message ID | 20170112181149.29035-1-chris.brandt@renesas.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Simon Horman |
| Headers | show |
Hi Chris, On Thu, Jan 12, 2017 at 7:11 PM, Chris Brandt <chris.brandt@renesas.com> wrote: > Now that all the clocks in the boot loader are disabled before booting > the kernel, and the mstp driver has been fixed for RZ/A1, here is a typo > that was missed during original testing. > > Fixes: 7c8522b7047c ("ARM: dts: r7s72100: add sdhi clock to device tree") > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> > --- > include/dt-bindings/clock/r7s72100-clock.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/dt-bindings/clock/r7s72100-clock.h b/include/dt-bindings/clock/r7s72100-clock.h > index 29e01ed..318ab14 100644 > --- a/include/dt-bindings/clock/r7s72100-clock.h > +++ b/include/dt-bindings/clock/r7s72100-clock.h > @@ -46,6 +46,6 @@ > > /* MSTP12 */ > #define R7S72100_CLK_SDHI0 3 > -#define R7S72100_CLK_SDHI1 2 > +#define R7S72100_CLK_SDHI1 1 This is strange. There are two SDHI channels, but the STBCR12 documentation (all versions up to rev. 3.00) says the register has MSTP bits for four SD host interfaces? Can you please enlighten me? Thanks! 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
Hi Geert, On Thursday, January 12, 2017, Geert Uytterhoeven wrote: > This is strange. There are two SDHI channels, but the STBCR12 > documentation (all versions up to rev. 3.00) says the register has MSTP > bits for four SD host interfaces? > > Can you please enlighten me? Thanks! Ya, I saw that. There are 2 bits per SDHI channel. I did check and just enabling the one works fine. Honestly, I'm not sure why there are two clock enables. I'll go back and ask the design team if they can tell me why there are 2. As I said, I just re-tested and it works fine, but you can hold off on the patch if you want until I come up with a real explanation. Chris
On Thu, Jan 12, 2017 at 08:34:26PM +0000, Chris Brandt wrote: > Hi Geert, > > On Thursday, January 12, 2017, Geert Uytterhoeven wrote: > > This is strange. There are two SDHI channels, but the STBCR12 > > documentation (all versions up to rev. 3.00) says the register has MSTP > > bits for four SD host interfaces? > > > > Can you please enlighten me? Thanks! > > Ya, I saw that. There are 2 bits per SDHI channel. I did check and just > enabling the one works fine. > > Honestly, I'm not sure why there are two clock enables. > > I'll go back and ask the design team if they can tell me why there are 2. > > As I said, I just re-tested and it works fine, but you can hold off on the > patch if you want until I come up with a real explanation. I'd prefer to hold off on this for now.
Hi Geert, On Thursday, January 12, 2017, Geert Uytterhoeven wrote: > This is strange. There are two SDHI channels, but the STBCR12 > documentation (all versions up to rev. 3.00) says the register has MSTP > bits for four SD host interfaces? > > Can you please enlighten me? Thanks! If you look in the rev 3.00 manual for RZ/A1H, in section "50.3.2 Card Detect/Write Protect", it says: * Power-Down mode at Card removal SD Host Interface module is halted by the MSTP123 bit to MSTP120 bit in Standby Control Register 12 (STBCR12). If these bits of each channel in STBCR12 are set to 10, low power consumption is achieved at Card removal. See section 55, Power-Down Modes So, there are 2 clock sources for each SDHI channel, and the setting Options are: SD Host Interface 0 Module Stop 00: SD Host Interface 0 Module runs. 01: Setting prohibited. 10: Only card detect block in SD Host Interface 0 Module runs. 11: Clock supply to SD Host Interface 0 Module is halted Previously I've been running with '00' because I was turning EVERYTHING on in u-boot. Yesterday, I tested with just '10' and things seem to work OK for me. Since the sh_mobile_sdhi/tmio driver only allows for 1 clock per channel, '10' is my only choice for the DT. However, looking at a previous BSP for RZ/A1, the driver arch/arm/mach-shmobile/clock-r7s72100.c had this: static struct clk mstp_clks[MSTP_NR] = { [MSTP123] = SH_CLK_MSTP8(&peripheral1_clk, STBCR12, 3, 0), /* SDHI00 */ [MSTP122] = SH_CLK_MSTP8(&peripheral1_clk, STBCR12, 2, CLK_ENABLE_ON_INIT), /* SDHI01 */ [MSTP121] = SH_CLK_MSTP8(&peripheral1_clk, STBCR12, 1, 0), /* SDHI10 */ [MSTP120] = SH_CLK_MSTP8(&peripheral1_clk, STBCR12, 0, CLK_ENABLE_ON_INIT), /* SDHI11 */ But...that would make me think on boot it would be set to '01' (setting prohibited). I'm going to try and find if "setting prohibited" really just means: "you can set it this way...but nothing is really going to work unless you enable the other clock". If that is the case, is there a DT equivalent to "CLK_ENABLE_ON_INIT" that I can do for MSTP120(SDHI11) and MSTP120(SDHI01) so they are both cleared on boot??? Thanks, Chris
Hi Chris, On Fri, Jan 13, 2017 at 3:44 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote: > On Thursday, January 12, 2017, Geert Uytterhoeven wrote: >> This is strange. There are two SDHI channels, but the STBCR12 >> documentation (all versions up to rev. 3.00) says the register has MSTP >> bits for four SD host interfaces? >> >> Can you please enlighten me? Thanks! > > If you look in the rev 3.00 manual for RZ/A1H, in section > "50.3.2 Card Detect/Write Protect", it says: > > * Power-Down mode at Card removal > SD Host Interface module is halted by the MSTP123 bit to MSTP120 bit in Standby Control Register 12 > (STBCR12). If these bits of each channel in STBCR12 are set to 10, low power consumption is achieved at Card > removal. See section 55, Power-Down Modes > > > So, there are 2 clock sources for each SDHI channel, and the setting > Options are: > SD Host Interface 0 Module Stop > 00: SD Host Interface 0 Module runs. > 01: Setting prohibited. > 10: Only card detect block in SD Host Interface 0 Module runs. > 11: Clock supply to SD Host Interface 0 Module is halted So typically you want to use 10 when idle, and 00 when active. > Previously I've been running with '00' because I was turning EVERYTHING on > in u-boot. > Yesterday, I tested with just '10' and things seem to work OK > for me. > > Since the sh_mobile_sdhi/tmio driver only allows for 1 clock per channel, > '10' is my only choice for the DT. > > However, looking at a previous BSP for RZ/A1, the driver > arch/arm/mach-shmobile/clock-r7s72100.c had this: > > > static struct clk mstp_clks[MSTP_NR] = { > [MSTP123] = SH_CLK_MSTP8(&peripheral1_clk, STBCR12, 3, 0), /* SDHI00 */ > [MSTP122] = SH_CLK_MSTP8(&peripheral1_clk, STBCR12, 2, > CLK_ENABLE_ON_INIT), /* SDHI01 */ > [MSTP121] = SH_CLK_MSTP8(&peripheral1_clk, STBCR12, 1, 0), /* SDHI10 */ > [MSTP120] = SH_CLK_MSTP8(&peripheral1_clk, STBCR12, 0, > CLK_ENABLE_ON_INIT), /* SDHI11 */ > > > But...that would make me think on boot it would be set to '01' (setting prohibited). Yeah, running with enabled SDHI core and disabled card detect sounds silly. > I'm going to try and find if "setting prohibited" really just means: > "you can set it this way...but nothing is really going to work unless you > enable the other clock". > > If that is the case, is there a DT equivalent to "CLK_ENABLE_ON_INIT" that > I can do for MSTP120(SDHI11) and MSTP120(SDHI01) so they are both cleared > on boot??? No there isn't. That's another reason why a full-fledged clock driver with tables in C is a better idea than trying to describe all clocks in DT. The new CPG/MSSR based driver (renesas-cpg-mssr.c) supports "critical module clocks" through CLK_ENABLE_HAND_OFF. Unfortunately that flag hasn't made it upstream, so I really should convert the driver to use the new CLK_IS_CRITICAL instead... 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
Hi Geert, > > But...that would make me think on boot it would be set to '01' (setting prohibited). > > Yeah, running with enabled SDHI core and disabled card detect sounds silly. I just did some testing and with only 1 clock enabled ('01'), the core works but card detect doesn't work. If you boot with the card in, you can read it fine, but if you pull it...no removal is detected. As soon as I turn the other clock on ('00'), card detect magically starts working. So from my experiments: 00: SD Host Interface 1 Module runs. >> everything works 01: Setting prohibited. >> core runs, but no card detect 10: Only card detect block in SD Host Interface 1 Module runs. >> SDHI address space all 00s (core not running) 11: Clock supply to SD Host Interface 1 Module is halted >> nothing works (of course) > So typically you want to use 10 when idle, and 00 when active. I wonder if that would mean the system would get a CD interrupt. Of course if no ISR registered (SDHI not enabled), the kernel would throw it away. > No there isn't. That's another reason why a full-fledged clock driver with > tables in C is a better idea than trying to describe all clocks in DT. > The new CPG/MSSR based driver (renesas-cpg-mssr.c) supports "critical > module clocks" through CLK_ENABLE_HAND_OFF. Unfortunately that flag hasn't > made it upstream, so I really should convert the driver to use the new > CLK_IS_CRITICAL instead... So basically at the moment you're tell me it's going to stay broke (unless it's enabled in u-boot). In sh_mobile_sdhi.c, can we change sh_mobile_sdhi_probe() so that if there are 2 clocks specified (in DT or platform data), it automatically enables the 2nd clock (forever) and just uses the 1st clock as the on/off clock? Chris
Hi Chris, On Fri, Jan 13, 2017 at 6:16 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote: >> > But...that would make me think on boot it would be set to '01' (setting prohibited). >> >> Yeah, running with enabled SDHI core and disabled card detect sounds silly. > > I just did some testing and with only 1 clock enabled ('01'), the core works but > card detect doesn't work. If you boot with the card in, you can read it fine, but > if you pull it...no removal is detected. > As soon as I turn the other clock on ('00'), card detect magically starts working. > > So from my experiments: > > 00: SD Host Interface 1 Module runs. > >> everything works > > 01: Setting prohibited. > >> core runs, but no card detect > > 10: Only card detect block in SD Host Interface 1 Module runs. > >> SDHI address space all 00s (core not running) > > 11: Clock supply to SD Host Interface 1 Module is halted > >> nothing works (of course) > > >> So typically you want to use 10 when idle, and 00 when active. > > I wonder if that would mean the system would get a CD interrupt. Of course > if no ISR registered (SDHI not enabled), the kernel would throw it away. > > >> No there isn't. That's another reason why a full-fledged clock driver with >> tables in C is a better idea than trying to describe all clocks in DT. >> The new CPG/MSSR based driver (renesas-cpg-mssr.c) supports "critical >> module clocks" through CLK_ENABLE_HAND_OFF. Unfortunately that flag hasn't >> made it upstream, so I really should convert the driver to use the new >> CLK_IS_CRITICAL instead... > > So basically at the moment you're tell me it's going to stay broke (unless it's > enabled in u-boot). > > In sh_mobile_sdhi.c, can we change sh_mobile_sdhi_probe() so that if there > are 2 clocks specified (in DT or platform data), it automatically enables > the 2nd clock (forever) and just uses the 1st clock as the on/off clock? Of course the driver can handle the second interrupt, if you update the binding, and add support code for that... 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
Hi Geert, On Friday, January 13, 2017, Geert Uytterhoeven wrote: > > In sh_mobile_sdhi.c, can we change sh_mobile_sdhi_probe() so that if > > there are 2 clocks specified (in DT or platform data), it > > automatically enables the 2nd clock (forever) and just uses the 1st > clock as the on/off clock? > > Of course the driver can handle the second interrupt, if you update the > binding, and add support code for that... Of course my idea is that I would only have to update the bindings for RZ/A1...not any other device. My only question is, today sh_mobile_sdhi.c uses this: priv->clk = devm_clk_get(&pdev->dev, NULL); to get the clock. But if there is a 2nd clock...how do I know the string id name to look to replace NULL with?? Or...for the RZ/A1 dtsi, should I just give the 2 clocks names: clocks = <&mstp12_clks R7S72100_CLK_SDHI00, &mstp12_clks R7S72100_CLK_SDHI01>; clock-names = "core", "cd"; and then in the code do: struct *cd_clk; cd_clk = devm_clk_get(&pdev->dev, "cd"); if (cd_clk) { clk_prepare_enable(cd_clk); } (this simple 1-line fix patch is getting a lot more complicated) Thanks, Chris
Hi Chris, CC Woflram On Fri, Jan 13, 2017 at 6:56 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote: > On Friday, January 13, 2017, Geert Uytterhoeven wrote: >> > In sh_mobile_sdhi.c, can we change sh_mobile_sdhi_probe() so that if >> > there are 2 clocks specified (in DT or platform data), it >> > automatically enables the 2nd clock (forever) and just uses the 1st >> clock as the on/off clock? >> >> Of course the driver can handle the second interrupt, if you update the >> binding, and add support code for that... > > Of course my idea is that I would only have to update the bindings for > RZ/A1...not any other device. > > My only question is, today sh_mobile_sdhi.c uses this: > > priv->clk = devm_clk_get(&pdev->dev, NULL); > > to get the clock. But if there is a 2nd clock...how do I know the > string id name to look to replace NULL with?? That should be specified in the bindings. "NULL" will get you the first clock. > Or...for the RZ/A1 dtsi, should I just give the 2 clocks names: > > clocks = <&mstp12_clks R7S72100_CLK_SDHI00, > &mstp12_clks R7S72100_CLK_SDHI01>; > clock-names = "core", "cd"; Something like that. If devm_clk_get(&pdev->dev, "core") fails, you can assume a single clock and retry with NULL. > and then in the code do: > > struct *cd_clk; > cd_clk = devm_clk_get(&pdev->dev, "cd"); > if (cd_clk) { > clk_prepare_enable(cd_clk); > } > > (this simple 1-line fix patch is getting a lot more complicated) Disclaimer: I don't know how/if the SDHI core manages clocks, and may interfere. Adding Wolfram. 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
> > and then in the code do: > > > > struct *cd_clk; > > cd_clk = devm_clk_get(&pdev->dev, "cd"); > > if (cd_clk) { > > clk_prepare_enable(cd_clk); > > } > > > > (this simple 1-line fix patch is getting a lot more complicated) > > Disclaimer: I don't know how/if the SDHI core manages clocks, and may > interfere. Adding Wolfram. Thanks for the heads up. We have special callbacks for en-/disabling clocks: sh_mobile_sdhi_clk_enable() and sh_mobile_sdhi_clk_disable(). I think those functions should get the above if-blocks (without curly braces) to ensure we always have consistent 00 or 11 settings.
Hi Wolfram and Geert, On Monday, January 16, 2017, Wolfram Sang: > > > and then in the code do: > > > > > > struct *cd_clk; > > > cd_clk = devm_clk_get(&pdev->dev, "cd"); > > > if (cd_clk) { > > > clk_prepare_enable(cd_clk); > > > } > > > > > > (this simple 1-line fix patch is getting a lot more complicated) > > > > Disclaimer: I don't know how/if the SDHI core manages clocks, and may > > interfere. Adding Wolfram. > > Thanks for the heads up. > > We have special callbacks for en-/disabling clocks: > sh_mobile_sdhi_clk_enable() and sh_mobile_sdhi_clk_disable(). > > I think those functions should get the above if-blocks (without curly > braces) to ensure we always have consistent 00 or 11 settings. Well.... I was going to try and cheat and in the probe, enable both the core and card-detect clocks, but, not save the card-detect clock pointer. Basically, once it is turned on, it is never turned off (since it was intended to be used for low power operation anyway). The reason is that would then keep me from having to modify the existing functions sh_mobile_sdhi_clk_enable/disable. Is anyone going to have an issue if I turn the card-detect clock on but never turn it off???? That was the patch that I was going to test out and submit. Chris
> The reason is that would then keep me from having to modify the existing > functions sh_mobile_sdhi_clk_enable/disable. Why do you prefer this? I may be missing something but a small if-block per function are not expensive IMO. > Is anyone going to have an issue if I turn the card-detect clock on but never > turn it off???? That was the patch that I was going to test out and submit. It smells a bit hacky to me. And while hacky things are sometimes needed, IMO this doesn't hold true here. With just 3 more lines (an if-block in disable and a variable for the new clock) we can have it proper.
Hi Wolfram, On Tue, Jan 17, 2017 at 9:21 AM, Wolfram Sang <wsa@the-dreams.de> wrote: >> The reason is that would then keep me from having to modify the existing >> functions sh_mobile_sdhi_clk_enable/disable. > > Why do you prefer this? I may be missing something but a small if-block > per function are not expensive IMO. > >> Is anyone going to have an issue if I turn the card-detect clock on but never >> turn it off???? That was the patch that I was going to test out and submit. > > It smells a bit hacky to me. And while hacky things are sometimes > needed, IMO this doesn't hold true here. With just 3 more lines (an > if-block in disable and a variable for the new clock) we can have it > proper. I have no idea if the SDHI driver disables the module clock when it is idle, but shouldn't the card detect clock be running all the time when the driver is bound to the device? 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
> I have no idea if the SDHI driver disables the module clock when it is > idle, but shouldn't the card detect clock be running all the time when the > driver is bound to the device? Yes, it should. And for all instances with just one clock, this means this main clock must be running. So, en-/disable functions are all about suspend/resume and bind/unbind. (Huh, looks like the unbind part is missing, though. Need to look closer). My take is: either we implement the power saving and handle the cd clock seperately. Or we ignore the power saving for now and handle both clocks as virtually one, i.e. en-/disable them at the same time. Doesn't make sense?
Hi Wolfram, On Tue, Jan 17, 2017 at 10:45 AM, Wolfram Sang <wsa@the-dreams.de> wrote: >> I have no idea if the SDHI driver disables the module clock when it is >> idle, but shouldn't the card detect clock be running all the time when the >> driver is bound to the device? > > Yes, it should. And for all instances with just one clock, this means > this main clock must be running. So, en-/disable functions are all about > suspend/resume and bind/unbind. (Huh, looks like the unbind part is > missing, though. Need to look closer). > > My take is: either we implement the power saving and handle the cd clock > seperately. Or we ignore the power saving for now and handle both clocks > as virtually one, i.e. en-/disable them at the same time. > > Doesn't make sense? If we handle them as one, won't we miss card detect events due to the card detect clock being disabled while SDHI is idle? 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
> If we handle them as one, won't we miss card detect events due to the > card detect clock being disabled while SDHI is idle? You mean this? 1208 /* 1209 * While using internal tmio hardware logic for card detection, we need 1210 * to ensure it stays powered for it to work. 1211 */ 1212 if (_host->native_hotplug) 1213 pm_runtime_get_noresume(&pdev->dev);
Hi Wolfram, On Tue, Jan 17, 2017 at 10:57 AM, Wolfram Sang <wsa@the-dreams.de> wrote: >> If we handle them as one, won't we miss card detect events due to the >> card detect clock being disabled while SDHI is idle? > > You mean this? > > 1208 /* > 1209 * While using internal tmio hardware logic for card detection, we need > 1210 * to ensure it stays powered for it to work. > 1211 */ > 1212 if (_host->native_hotplug) > 1213 pm_runtime_get_noresume(&pdev->dev); OK. So that will keep the core module clock running. 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
Hi Wolfram, On Tuesday, January 17, 2017, Wolfram Sang wrote: > > I have no idea if the SDHI driver disables the module clock when it is > > idle, but shouldn't the card detect clock be running all the time when > > the driver is bound to the device? > > Yes, it should. And for all instances with just one clock, this means this > main clock must be running. So, en-/disable functions are all about > suspend/resume and bind/unbind. (Huh, looks like the unbind part is > missing, though. Need to look closer). > > My take is: either we implement the power saving and handle the cd clock > seperately. Or we ignore the power saving for now and handle both clocks > as virtually one, i.e. en-/disable them at the same time. > > Doesn't make sense? I'll go with "ignore the power saving for now and handle both clocks as virtually one". I'll go give it a try and see how it works. Thanks, Chris
Hi Wolfram, On Tuesday, January 17, 2017, Wolfram Sang wrote: > > I have no idea if the SDHI driver disables the module clock when it is > > idle, but shouldn't the card detect clock be running all the time when > > the driver is bound to the device? > > Yes, it should. And for all instances with just one clock, this means this > main clock must be running. So, en-/disable functions are all about > suspend/resume and bind/unbind. (Huh, looks like the unbind part is > missing, though. Need to look closer). I just did an unbind on my system: $ echo e804e800.sd > /sys/bus/platform/drivers/sh_mobile_sdhi/unbind Looks like sh_mobile_sdhi_remove() is called, but sh_mobile_sdhi_clk_disable() is not. So, should sh_mobile_sdhi_remove() be changed to call sh_mobile_sdhi_clk_disable()? Chris
> So, should sh_mobile_sdhi_remove() be changed to call sh_mobile_sdhi_clk_disable()?
Give me a minute, I already did a patch for this :)
diff --git a/include/dt-bindings/clock/r7s72100-clock.h b/include/dt-bindings/clock/r7s72100-clock.h index 29e01ed..318ab14 100644 --- a/include/dt-bindings/clock/r7s72100-clock.h +++ b/include/dt-bindings/clock/r7s72100-clock.h @@ -46,6 +46,6 @@ /* MSTP12 */ #define R7S72100_CLK_SDHI0 3 -#define R7S72100_CLK_SDHI1 2 +#define R7S72100_CLK_SDHI1 1 #endif /* __DT_BINDINGS_CLOCK_R7S72100_H__ */
Now that all the clocks in the boot loader are disabled before booting the kernel, and the mstp driver has been fixed for RZ/A1, here is a typo that was missed during original testing. Fixes: 7c8522b7047c ("ARM: dts: r7s72100: add sdhi clock to device tree") Signed-off-by: Chris Brandt <chris.brandt@renesas.com> --- include/dt-bindings/clock/r7s72100-clock.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)