| Message ID | 1386763029-8516-2-git-send-email-a.kesavan@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Abhilash, On Wed, Dec 11, 2013 at 3:57 AM, Abhilash Kesavan <a.kesavan@samsung.com> wrote: > The sysreg (system register) generates control signals for various blocks > like disp1blk, i2c, mipi etc. However, it gets disabled as an unused clock > at boot-up. > > Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com> > --- > drivers/clk/samsung/clk-exynos5250.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/samsung/clk-exynos5250.c b/drivers/clk/samsung/clk-exynos5250.c > index adf3234..0833501 100644 > --- a/drivers/clk/samsung/clk-exynos5250.c > +++ b/drivers/clk/samsung/clk-exynos5250.c > @@ -406,7 +406,8 @@ static struct samsung_gate_clock exynos5250_gate_clks[] __initdata = { > GATE(hsi2c2, "hsi2c2", "aclk66", GATE_IP_PERIC, 30, 0, 0), > GATE(hsi2c3, "hsi2c3", "aclk66", GATE_IP_PERIC, 31, 0, 0), > GATE(chipid, "chipid", "aclk66", GATE_IP_PERIS, 0, 0, 0), > - GATE(sysreg, "sysreg", "aclk66", GATE_IP_PERIS, 1, 0, 0), > + GATE(sysreg, "sysreg", "aclk66", > + GATE_IP_PERIS, 1, CLK_IGNORE_UNUSED, 0), > GATE(pmu, "pmu", "aclk66", GATE_IP_PERIS, 2, CLK_IGNORE_UNUSED, 0), > GATE(tzpc0, "tzpc0", "aclk66", GATE_IP_PERIS, 6, 0, 0), > GATE(tzpc1, "tzpc1", "aclk66", GATE_IP_PERIS, 7, 0, 0), This does match what's done in exynos4 and exynos5420 and it's not terrible. I'm always a fan of actually specifying clocks properly and that's more possible now using the syscon stuff (see Leela Krishna's watchdog patches). You'd have to extend that to add a clock, but that wouldn't be too hard. That being said, I'd personally be OK with this solution for now (to match the other exynos products), so: Reviewed-by: Doug Anderson <dianders@chromium.org>
2013/12/12 Doug Anderson <dianders@chromium.org>: > Abhilash, > > On Wed, Dec 11, 2013 at 3:57 AM, Abhilash Kesavan <a.kesavan@samsung.com> wrote: >> The sysreg (system register) generates control signals for various blocks >> like disp1blk, i2c, mipi etc. However, it gets disabled as an unused clock >> at boot-up. >> >> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com> >> --- >> drivers/clk/samsung/clk-exynos5250.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/clk/samsung/clk-exynos5250.c b/drivers/clk/samsung/clk-exynos5250.c >> index adf3234..0833501 100644 >> --- a/drivers/clk/samsung/clk-exynos5250.c >> +++ b/drivers/clk/samsung/clk-exynos5250.c >> @@ -406,7 +406,8 @@ static struct samsung_gate_clock exynos5250_gate_clks[] __initdata = { >> GATE(hsi2c2, "hsi2c2", "aclk66", GATE_IP_PERIC, 30, 0, 0), >> GATE(hsi2c3, "hsi2c3", "aclk66", GATE_IP_PERIC, 31, 0, 0), >> GATE(chipid, "chipid", "aclk66", GATE_IP_PERIS, 0, 0, 0), >> - GATE(sysreg, "sysreg", "aclk66", GATE_IP_PERIS, 1, 0, 0), >> + GATE(sysreg, "sysreg", "aclk66", >> + GATE_IP_PERIS, 1, CLK_IGNORE_UNUSED, 0), >> GATE(pmu, "pmu", "aclk66", GATE_IP_PERIS, 2, CLK_IGNORE_UNUSED, 0), >> GATE(tzpc0, "tzpc0", "aclk66", GATE_IP_PERIS, 6, 0, 0), >> GATE(tzpc1, "tzpc1", "aclk66", GATE_IP_PERIS, 7, 0, 0), > > This does match what's done in exynos4 and exynos5420 and it's not > terrible. I'm always a fan of actually specifying clocks properly and > that's more possible now using the syscon stuff (see Leela Krishna's > watchdog patches). You'd have to extend that to add a clock, but that > wouldn't be too hard. Leela's patches are about PMU, not sysreg, but that's not an issue, I guess, as it's about adding a clock to the generic syscon driver. Still, I discussed about such cases as this with Sylwester a bit today and maybe a bit different approach would be better. There is a number of clocks that need to be always on, such as PMU (but also a lot of currently undefined ones). IMHO it would be nice to make sure they are enabled at boot time and do one of following: 1) claim and enable them directly from the clock controller driver 2) define them with CLK_IGNORE_UNUSED flag and enable them directly from the clock controller driver (without increasing the refcount, so users could possibly disable them later), 3) add a generic flag, such as CLK_BOOT_ENABLE (or something), that would make the CCF enable such clock at bootup (in addition to implying CLK_IGNORE_UNUSED). For me, the most sensible option would be 2) as it doesn't bloat the CCF with yet another flag and doesn't encourage people to leave clocks always on just because of laziness stopping them from implementing proper clock support in drivers. Still, for now, it's an OK from me, as we don't have the infrastructure yet. Reviewed-by: Tomasz Figa <t.figa@samsung.com> Best regards, Tomasz
Tomasz, On Wed, Dec 11, 2013 at 4:13 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > 2013/12/12 Doug Anderson <dianders@chromium.org>: >> This does match what's done in exynos4 and exynos5420 and it's not >> terrible. I'm always a fan of actually specifying clocks properly and >> that's more possible now using the syscon stuff (see Leela Krishna's >> watchdog patches). You'd have to extend that to add a clock, but that >> wouldn't be too hard. > > Leela's patches are about PMU, not sysreg, but that's not an issue, I guess, > as it's about adding a clock to the generic syscon driver. Right. I was proposing doing something similar to his, but for the separate "sysreg" address range. ...then adding a clock to the generic syscon driver. I _think_ the syscon clock doesn't really need to be always on--it only needs to be on during the access of these registers, right? I make this statement based on the fact that exynos5250 boards currently bootup and are very functional, but this clock is currently off. > Still, I discussed about such cases as this with Sylwester a bit today and > maybe a bit different approach would be better. There is a number of clocks > that need to be always on, such as PMU (but also a lot of currently undefined > ones). IMHO it would be nice to make sure they are enabled at boot time > and do one of following: > 1) claim and enable them directly from the clock controller driver > 2) define them with CLK_IGNORE_UNUSED flag and enable them directly from > the clock controller driver (without increasing the refcount, so users could > possibly disable them later), > 3) add a generic flag, such as CLK_BOOT_ENABLE (or something), that would > make the CCF enable such clock at bootup (in addition to implying > CLK_IGNORE_UNUSED). > > For me, the most sensible option would be 2) as it doesn't bloat the CCF with > yet another flag and doesn't encourage people to leave clocks always on > just because of laziness stopping them from implementing proper clock > support in drivers. Right, we're using #2 for this now, but one problem is that it's possible that the firmware may turn off one of these misc-type clocks. On exynos5250-snow we ran into this. The firmware actually gates the clock needed for accessing the chip_id, though perhaps that's not one of the clocks that needs to be on all the time. -Doug
Hi Tomasz and Doug, Thanks for the review. On Thu, Dec 12, 2013 at 6:00 AM, Doug Anderson <dianders@chromium.org> wrote: > Tomasz, > > On Wed, Dec 11, 2013 at 4:13 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: >> 2013/12/12 Doug Anderson <dianders@chromium.org>: >>> This does match what's done in exynos4 and exynos5420 and it's not >>> terrible. I'm always a fan of actually specifying clocks properly and >>> that's more possible now using the syscon stuff (see Leela Krishna's >>> watchdog patches). You'd have to extend that to add a clock, but that >>> wouldn't be too hard. >> >> Leela's patches are about PMU, not sysreg, but that's not an issue, I guess, >> as it's about adding a clock to the generic syscon driver. > > Right. I was proposing doing something similar to his, but for the > separate "sysreg" address range. ...then adding a clock to the > generic syscon driver. > > I _think_ the syscon clock doesn't really need to be always on--it > only needs to be on during the access of these registers, right? I > make this statement based on the fact that exynos5250 boards currently > bootup and are very functional, but this clock is currently off. I see USB2 currently using sysreg at boot-up, so I would assume that it would fail if we have this clock turned off by the firmware. Have not tested yet. > > >> Still, I discussed about such cases as this with Sylwester a bit today and >> maybe a bit different approach would be better. There is a number of clocks >> that need to be always on, such as PMU (but also a lot of currently undefined >> ones). IMHO it would be nice to make sure they are enabled at boot time >> and do one of following: >> 1) claim and enable them directly from the clock controller driver >> 2) define them with CLK_IGNORE_UNUSED flag and enable them directly from >> the clock controller driver (without increasing the refcount, so users could >> possibly disable them later), >> 3) add a generic flag, such as CLK_BOOT_ENABLE (or something), that would >> make the CCF enable such clock at bootup (in addition to implying >> CLK_IGNORE_UNUSED). >> >> For me, the most sensible option would be 2) as it doesn't bloat the CCF with >> yet another flag and doesn't encourage people to leave clocks always on >> just because of laziness stopping them from implementing proper clock >> support in drivers. > > Right, we're using #2 for this now, but one problem is that it's > possible that the firmware may turn off one of these misc-type clocks. > On exynos5250-snow we ran into this. The firmware actually gates the > clock needed for accessing the chip_id, though perhaps that's not one > of the clocks that needs to be on all the time. Yes, If the firmware gates sysreg then we should see failures at boot-up or later. Is this patch OK for now ? How would you guys like me to proceed on this ? > > -Doug Regards, Abhilash > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thursday 12 of December 2013 10:06:36 Abhilash Kesavan wrote: > Hi Tomasz and Doug, > > Thanks for the review. > On Thu, Dec 12, 2013 at 6:00 AM, Doug Anderson <dianders@chromium.org> wrote: > > Tomasz, > > > > On Wed, Dec 11, 2013 at 4:13 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: [snip] > >> Still, I discussed about such cases as this with Sylwester a bit today and > >> maybe a bit different approach would be better. There is a number of clocks > >> that need to be always on, such as PMU (but also a lot of currently undefined > >> ones). IMHO it would be nice to make sure they are enabled at boot time > >> and do one of following: > >> 1) claim and enable them directly from the clock controller driver > >> 2) define them with CLK_IGNORE_UNUSED flag and enable them directly from > >> the clock controller driver (without increasing the refcount, so users could > >> possibly disable them later), > >> 3) add a generic flag, such as CLK_BOOT_ENABLE (or something), that would > >> make the CCF enable such clock at bootup (in addition to implying > >> CLK_IGNORE_UNUSED). > >> > >> For me, the most sensible option would be 2) as it doesn't bloat the CCF with > >> yet another flag and doesn't encourage people to leave clocks always on > >> just because of laziness stopping them from implementing proper clock > >> support in drivers. > > > > Right, we're using #2 for this now, but one problem is that it's > > possible that the firmware may turn off one of these misc-type clocks. > > On exynos5250-snow we ran into this. The firmware actually gates the > > clock needed for accessing the chip_id, though perhaps that's not one > > of the clocks that needs to be on all the time. > > Yes, If the firmware gates sysreg then we should see failures at > boot-up or later. > Is this patch OK for now ? How would you guys like me to proceed on this ? Yes, as I wrote in my original reply, for now your patch is fine as we don't have the infrastructure for handling such clocks in a stricter way yet. Best regards, Tomasz
On Wednesday 11 of December 2013 17:27:05 Abhilash Kesavan wrote: > The sysreg (system register) generates control signals for various blocks > like disp1blk, i2c, mipi etc. However, it gets disabled as an unused clock > at boot-up. > > Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com> > --- > drivers/clk/samsung/clk-exynos5250.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/samsung/clk-exynos5250.c b/drivers/clk/samsung/clk-exynos5250.c > index adf3234..0833501 100644 > --- a/drivers/clk/samsung/clk-exynos5250.c > +++ b/drivers/clk/samsung/clk-exynos5250.c > @@ -406,7 +406,8 @@ static struct samsung_gate_clock exynos5250_gate_clks[] __initdata = { > GATE(hsi2c2, "hsi2c2", "aclk66", GATE_IP_PERIC, 30, 0, 0), > GATE(hsi2c3, "hsi2c3", "aclk66", GATE_IP_PERIC, 31, 0, 0), > GATE(chipid, "chipid", "aclk66", GATE_IP_PERIS, 0, 0, 0), > - GATE(sysreg, "sysreg", "aclk66", GATE_IP_PERIS, 1, 0, 0), > + GATE(sysreg, "sysreg", "aclk66", > + GATE_IP_PERIS, 1, CLK_IGNORE_UNUSED, 0), > GATE(pmu, "pmu", "aclk66", GATE_IP_PERIS, 2, CLK_IGNORE_UNUSED, 0), > GATE(tzpc0, "tzpc0", "aclk66", GATE_IP_PERIS, 6, 0, 0), > GATE(tzpc1, "tzpc1", "aclk66", GATE_IP_PERIS, 7, 0, 0), > Applied to samsung-clk tree, since this patch seems not to depend or be a dependency of other patches. Best regards, Tomasz
diff --git a/drivers/clk/samsung/clk-exynos5250.c b/drivers/clk/samsung/clk-exynos5250.c index adf3234..0833501 100644 --- a/drivers/clk/samsung/clk-exynos5250.c +++ b/drivers/clk/samsung/clk-exynos5250.c @@ -406,7 +406,8 @@ static struct samsung_gate_clock exynos5250_gate_clks[] __initdata = { GATE(hsi2c2, "hsi2c2", "aclk66", GATE_IP_PERIC, 30, 0, 0), GATE(hsi2c3, "hsi2c3", "aclk66", GATE_IP_PERIC, 31, 0, 0), GATE(chipid, "chipid", "aclk66", GATE_IP_PERIS, 0, 0, 0), - GATE(sysreg, "sysreg", "aclk66", GATE_IP_PERIS, 1, 0, 0), + GATE(sysreg, "sysreg", "aclk66", + GATE_IP_PERIS, 1, CLK_IGNORE_UNUSED, 0), GATE(pmu, "pmu", "aclk66", GATE_IP_PERIS, 2, CLK_IGNORE_UNUSED, 0), GATE(tzpc0, "tzpc0", "aclk66", GATE_IP_PERIS, 6, 0, 0), GATE(tzpc1, "tzpc1", "aclk66", GATE_IP_PERIS, 7, 0, 0),
The sysreg (system register) generates control signals for various blocks like disp1blk, i2c, mipi etc. However, it gets disabled as an unused clock at boot-up. Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com> --- drivers/clk/samsung/clk-exynos5250.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)