| Message ID | 1381780051-1826-1-git-send-email-dinguyen@altera.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Dinh, On 10/15/2013 04:47 AM, dinguyen@altera.com wrote: > From: Dinh Nguyen <dinguyen@altera.com> > > Add functionality in the System Manager to set the SDR settings for the > SD/MMC IP. > > Signed-off-by: Dinh Nguyen <dinguyen@altera.com> > Cc: Pavel Machek <pavel@denx.de> > CC: Arnd Bergmann <arnd@arndb.de> > CC: Olof Johansson <olof@lixom.net> > Cc: Rob Herring <rob.herring@calxeda.com> > Cc: Pawel Moll <pawel.moll@arm.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Stephen Warren <swarren@wwwdotorg.org> > Cc: Ian Campbell <ian.campbell@citrix.com> > Cc: Chris Ball <cjb@laptop.org> > Cc: Jaehoon Chung <jh80.chung@samsung.com> > Cc: Seungwon Jeon <tgih.jun@samsung.com> > Cc: devicetree@vger.kernel.org > Cc: linux-mmc@vger.kernel.org > CC: linux-arm-kernel@lists.infradead.org > > --- > v2: > - Have socfpga_sysmgr_set_dwmmc_drvsel_smpsel() take in the SDR settings > directly so that it does not have to walk the DTB. > --- > arch/arm/mach-socfpga/Makefile | 2 +- > arch/arm/mach-socfpga/core.h | 6 ++++++ > arch/arm/mach-socfpga/system_mgr.c | 28 ++++++++++++++++++++++++++++ > 3 files changed, 35 insertions(+), 1 deletion(-) > create mode 100644 arch/arm/mach-socfpga/system_mgr.c > > diff --git a/arch/arm/mach-socfpga/Makefile b/arch/arm/mach-socfpga/Makefile > index 6dd7a93..e4ff8b9 100644 > --- a/arch/arm/mach-socfpga/Makefile > +++ b/arch/arm/mach-socfpga/Makefile > @@ -2,5 +2,5 @@ > # Makefile for the linux kernel. > # > > -obj-y := socfpga.o > +obj-y := socfpga.o system_mgr.o > obj-$(CONFIG_SMP) += headsmp.o platsmp.o > diff --git a/arch/arm/mach-socfpga/core.h b/arch/arm/mach-socfpga/core.h > index 572b8f7..b05fa6a 100644 > --- a/arch/arm/mach-socfpga/core.h > +++ b/arch/arm/mach-socfpga/core.h > @@ -44,4 +44,10 @@ extern unsigned long cpu1start_addr; > > #define SOCFPGA_SCU_VIRT_BASE 0xfffec000 > > +/* SDMMC Group for System Manager defines */ > +#define SYSMGR_SDMMCGRP_CTRL_OFFSET 0x108 > +#define DRV_CLK_PHASE_SHIFT_SEL_MASK 0x7 I didn't see where this define is used. When do this use? Best Regards, Jaehoon Chung > +#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel) \ > + ((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0)) > + > #endif > diff --git a/arch/arm/mach-socfpga/system_mgr.c b/arch/arm/mach-socfpga/system_mgr.c > new file mode 100644 > index 0000000..9fdce1d > --- /dev/null > +++ b/arch/arm/mach-socfpga/system_mgr.c > @@ -0,0 +1,28 @@ > +/* > + * Copyright Altera Corporation (C) 2013. All Rights Reserved. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + * You should have received a copy of the GNU General Public License along with > + * this program. If not, see <http://www.gnu.org/licenses/>. > + */ > +#include <linux/of_platform.h> > +#include <asm/mach/map.h> > + > +#include "core.h" > + > +void socfpga_sysmgr_set_dwmmc_drvsel_smpsel(u32 drvsel, u32 smplsel) > +{ > + u32 hs_timing; > + > + hs_timing = SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel); > + writel(hs_timing, sys_manager_base_addr + SYSMGR_SDMMCGRP_CTRL_OFFSET); > +} > +EXPORT_SYMBOL(socfpga_sysmgr_set_dwmmc_drvsel_smpsel); >
Hi Jaehoo, On 10/15/13 1:51 AM, Jaehoon Chung wrote: > Hi Dinh, > > On 10/15/2013 04:47 AM, dinguyen@altera.com wrote: >> From: Dinh Nguyen <dinguyen@altera.com> >> >> Add functionality in the System Manager to set the SDR settings for the >> SD/MMC IP. >> >> Signed-off-by: Dinh Nguyen <dinguyen@altera.com> >> Cc: Pavel Machek <pavel@denx.de> >> CC: Arnd Bergmann <arnd@arndb.de> >> CC: Olof Johansson <olof@lixom.net> >> Cc: Rob Herring <rob.herring@calxeda.com> >> Cc: Pawel Moll <pawel.moll@arm.com> >> Cc: Mark Rutland <mark.rutland@arm.com> >> Cc: Stephen Warren <swarren@wwwdotorg.org> >> Cc: Ian Campbell <ian.campbell@citrix.com> >> Cc: Chris Ball <cjb@laptop.org> >> Cc: Jaehoon Chung <jh80.chung@samsung.com> >> Cc: Seungwon Jeon <tgih.jun@samsung.com> >> Cc: devicetree@vger.kernel.org >> Cc: linux-mmc@vger.kernel.org >> CC: linux-arm-kernel@lists.infradead.org >> >> --- >> v2: >> - Have socfpga_sysmgr_set_dwmmc_drvsel_smpsel() take in the SDR settings >> directly so that it does not have to walk the DTB. >> --- >> arch/arm/mach-socfpga/Makefile | 2 +- >> arch/arm/mach-socfpga/core.h | 6 ++++++ >> arch/arm/mach-socfpga/system_mgr.c | 28 ++++++++++++++++++++++++++++ >> 3 files changed, 35 insertions(+), 1 deletion(-) >> create mode 100644 arch/arm/mach-socfpga/system_mgr.c >> >> diff --git a/arch/arm/mach-socfpga/Makefile b/arch/arm/mach-socfpga/Makefile >> index 6dd7a93..e4ff8b9 100644 >> --- a/arch/arm/mach-socfpga/Makefile >> +++ b/arch/arm/mach-socfpga/Makefile >> @@ -2,5 +2,5 @@ >> # Makefile for the linux kernel. >> # >> >> -obj-y := socfpga.o >> +obj-y := socfpga.o system_mgr.o >> obj-$(CONFIG_SMP) += headsmp.o platsmp.o >> diff --git a/arch/arm/mach-socfpga/core.h b/arch/arm/mach-socfpga/core.h >> index 572b8f7..b05fa6a 100644 >> --- a/arch/arm/mach-socfpga/core.h >> +++ b/arch/arm/mach-socfpga/core.h >> @@ -44,4 +44,10 @@ extern unsigned long cpu1start_addr; >> >> #define SOCFPGA_SCU_VIRT_BASE 0xfffec000 >> >> +/* SDMMC Group for System Manager defines */ >> +#define SYSMGR_SDMMCGRP_CTRL_OFFSET 0x108 >> +#define DRV_CLK_PHASE_SHIFT_SEL_MASK 0x7 > I didn't see where this define is used. When do this use? It's not used at all. I will remove. Thanks for the review. Dinh > > Best Regards, > Jaehoon Chung > >> +#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel) \ >> + ((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0)) >> + >> #endif >> diff --git a/arch/arm/mach-socfpga/system_mgr.c b/arch/arm/mach-socfpga/system_mgr.c >> new file mode 100644 >> index 0000000..9fdce1d >> --- /dev/null >> +++ b/arch/arm/mach-socfpga/system_mgr.c >> @@ -0,0 +1,28 @@ >> +/* >> + * Copyright Altera Corporation (C) 2013. All Rights Reserved. >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms and conditions of the GNU General Public License, >> + * version 2, as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope it will be useful, but WITHOUT >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or >> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for >> + * more details. >> + * >> + * You should have received a copy of the GNU General Public License along with >> + * this program. If not, see <http://www.gnu.org/licenses/>. >> + */ >> +#include <linux/of_platform.h> >> +#include <asm/mach/map.h> >> + >> +#include "core.h" >> + >> +void socfpga_sysmgr_set_dwmmc_drvsel_smpsel(u32 drvsel, u32 smplsel) >> +{ >> + u32 hs_timing; >> + >> + hs_timing = SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel); >> + writel(hs_timing, sys_manager_base_addr + SYSMGR_SDMMCGRP_CTRL_OFFSET); >> +} >> +EXPORT_SYMBOL(socfpga_sysmgr_set_dwmmc_drvsel_smpsel); >>
On Monday 14 October 2013, dinguyen@altera.com wrote: > +void socfpga_sysmgr_set_dwmmc_drvsel_smpsel(u32 drvsel, u32 smplsel) > +{ > + u32 hs_timing; > + > + hs_timing = SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel); > + writel(hs_timing, sys_manager_base_addr + SYSMGR_SDMMCGRP_CTRL_OFFSET); > +} > +EXPORT_SYMBOL(socfpga_sysmgr_set_dwmmc_drvsel_smpsel); This looks like the wrong approach. What are you trying to do? If you want to set a clock, please use the clk API. Arnd
Hi Arnd, On 10/15/13 7:50 AM, Arnd Bergmann wrote: > On Monday 14 October 2013, dinguyen@altera.com wrote: >> +void socfpga_sysmgr_set_dwmmc_drvsel_smpsel(u32 drvsel, u32 smplsel) >> +{ >> + u32 hs_timing; >> + >> + hs_timing = SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel); >> + writel(hs_timing, sys_manager_base_addr + SYSMGR_SDMMCGRP_CTRL_OFFSET); >> +} >> +EXPORT_SYMBOL(socfpga_sysmgr_set_dwmmc_drvsel_smpsel); > This looks like the wrong approach. What are you trying to do? If you want to > set a clock, please use the clk API. I can't use the clk API because this function is setting up a clock phase bit for the SD/MMC clock that is used to clock the card, not the IP. This register is located outside the SD/MMC and the clock manager. Just to refresh your memory on this topic: I tried to use the syscon approach that you suggested: http://lists.infradead.org/pipermail/linux-arm-kernel/2013-May/168470.html http://lists.infradead.org/pipermail/linux-arm-kernel/2013-May/170423.html But this approach was rejected by Stephen Warren because we wanted to the SD driver to be automonous of registers outside its IP: http://lists.infradead.org/pipermail/linux-arm-kernel/2013-August/194014.html So I went with the approach of exposing a platform API so that the SD/MMC platform specific code can call it. The system manager has a plethora of registers that controls other IPs on the SOC, so I kinda thought syscon was the way to go with this. A driver for this IP did not make sense to me. Please advise if you know of another approach? Thanks, Dinh > > Arnd
On Tuesday 15 October 2013, Dinh Nguyen wrote: > Hi Arnd, > > On 10/15/13 7:50 AM, Arnd Bergmann wrote: > > On Monday 14 October 2013, dinguyen@altera.com wrote: > >> +void socfpga_sysmgr_set_dwmmc_drvsel_smpsel(u32 drvsel, u32 smplsel) > >> +{ > >> + u32 hs_timing; > >> + > >> + hs_timing = SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel); > >> + writel(hs_timing, sys_manager_base_addr + SYSMGR_SDMMCGRP_CTRL_OFFSET); > >> +} > >> +EXPORT_SYMBOL(socfpga_sysmgr_set_dwmmc_drvsel_smpsel); > > This looks like the wrong approach. What are you trying to do? If you want to > > set a clock, please use the clk API. > I can't use the clk API because this function is setting up a clock > phase bit for the SD/MMC > clock that is used to clock the card, not the IP. This register is > located outside the SD/MMC > and the clock manager. > > Just to refresh your memory on this topic: > I tried to use the syscon approach that you suggested: > > http://lists.infradead.org/pipermail/linux-arm-kernel/2013-May/168470.html > http://lists.infradead.org/pipermail/linux-arm-kernel/2013-May/170423.html Ah, thanks. I knew this problem had come up before, I just didn't remember it was for socfpga. > But this approach was rejected by Stephen Warren because we wanted to > the SD driver to be automonous > of registers outside its IP: > > http://lists.infradead.org/pipermail/linux-arm-kernel/2013-August/194014.html > > So I went with the approach of exposing a platform API so that the > SD/MMC platform specific > code can call it. > > The system manager has a plethora of registers that controls other IPs > on the SOC, so I kinda thought > syscon was the way to go with this. A driver for this IP did not make > sense to me. > > Please advise if you know of another approach? I don't remember the details of what we have gone through before, but I think this should still work: 1 Create a "syscon" backend driver to control your "system manager", which lets other drivers hook into it without calling a private API. 2 Create a trivial clock driver that is independent of your existing clock driver and independent of the other drivers using the system manager, by using syscon as the low-level interface. 3 Make the sdmmc driver use the normal clock API and link its clock to the driver from step 2 in the device tree. Is this what you have tried before? Arnd
Hi Arnd, On 10/15/13 2:01 PM, Arnd Bergmann wrote: > On Tuesday 15 October 2013, Dinh Nguyen wrote: >> Hi Arnd, >> >> On 10/15/13 7:50 AM, Arnd Bergmann wrote: >>> On Monday 14 October 2013, dinguyen@altera.com wrote: >>>> +void socfpga_sysmgr_set_dwmmc_drvsel_smpsel(u32 drvsel, u32 smplsel) >>>> +{ >>>> + u32 hs_timing; >>>> + >>>> + hs_timing = SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel); >>>> + writel(hs_timing, sys_manager_base_addr + SYSMGR_SDMMCGRP_CTRL_OFFSET); >>>> +} >>>> +EXPORT_SYMBOL(socfpga_sysmgr_set_dwmmc_drvsel_smpsel); >>> This looks like the wrong approach. What are you trying to do? If you want to >>> set a clock, please use the clk API. >> I can't use the clk API because this function is setting up a clock >> phase bit for the SD/MMC >> clock that is used to clock the card, not the IP. This register is >> located outside the SD/MMC >> and the clock manager. >> >> Just to refresh your memory on this topic: >> I tried to use the syscon approach that you suggested: >> >> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-May/168470.html >> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-May/170423.html > Ah, thanks. I knew this problem had come up before, I just didn't remember > it was for socfpga. > >> But this approach was rejected by Stephen Warren because we wanted to >> the SD driver to be automonous >> of registers outside its IP: >> >> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-August/194014.html >> >> So I went with the approach of exposing a platform API so that the >> SD/MMC platform specific >> code can call it. >> >> The system manager has a plethora of registers that controls other IPs >> on the SOC, so I kinda thought >> syscon was the way to go with this. A driver for this IP did not make >> sense to me. >> >> Please advise if you know of another approach? > I don't remember the details of what we have gone through before, but > I think this should still work: > > 1 Create a "syscon" backend driver to control your "system manager", which > lets other drivers hook into it without calling a private API. Yes, if you look at drivers/mmc/host/dw_mmc-socfpga.c that is in the mainline, it is hooking into the "system manager" through "syscon". Is this what you mean here? The problem is because of the SYSMGR_SDMMCGRP_CTRL_OFFSET define in this file. This means the SD/MMC driver needs information that is outside of its IP. Dinh > 2 Create a trivial clock driver that is independent of your existing > clock driver and independent of the other drivers using the system > manager, by using syscon as the low-level interface. > 3 Make the sdmmc driver use the normal clock API and link its clock to the > driver from step 2 in the device tree. > > Is this what you have tried before? > > Arnd
On Tuesday 15 October 2013, Dinh Nguyen wrote: > > > > 1 Create a "syscon" backend driver to control your "system manager", which > > lets other drivers hook into it without calling a private API. > Yes, if you look at drivers/mmc/host/dw_mmc-socfpga.c that is in the > mainline, > it is hooking into the "system manager" through "syscon". Is this what you > mean here? No, because you directly hook into the syscon driver, rather than using a clock driver as the middle-man, see steps 2 and 3 below. > The problem is because of the SYSMGR_SDMMCGRP_CTRL_OFFSET define > in this file. This means the SD/MMC driver needs information that is > outside of its IP. Yes, this is not ideal because you don't really want that information in the sd/mmc driver. That driver should only know about the fact that it talks to a clock controller, not how it's implemented. Arnd > > 2 Create a trivial clock driver that is independent of your existing > > clock driver and independent of the other drivers using the system > > manager, by using syscon as the low-level interface. > > 3 Make the sdmmc driver use the normal clock API and link its clock to the > > driver from step 2 in the device tree. > > > > Is this what you have tried before?
On 10/15/13 2:47 PM, Arnd Bergmann wrote: > On Tuesday 15 October 2013, Dinh Nguyen wrote: >>> 1 Create a "syscon" backend driver to control your "system manager", which >>> lets other drivers hook into it without calling a private API. >> Yes, if you look at drivers/mmc/host/dw_mmc-socfpga.c that is in the >> mainline, >> it is hooking into the "system manager" through "syscon". Is this what you >> mean here? > No, because you directly hook into the syscon driver, rather than using > a clock driver as the middle-man, see steps 2 and 3 below. > Ok, now I understand. >> The problem is because of the SYSMGR_SDMMCGRP_CTRL_OFFSET define >> in this file. This means the SD/MMC driver needs information that is >> outside of its IP. > Yes, this is not ideal because you don't really want that information > in the sd/mmc driver. That driver should only know about the fact > that it talks to a clock controller, not how it's implemented. > > Arnd > >>> 2 Create a trivial clock driver that is independent of your existing >>> clock driver and independent of the other drivers using the system >>> manager, by using syscon as the low-level interface. >>> 3 Make the sdmmc driver use the normal clock API and link its clock to the >>> driver from step 2 in the device tree. This makes sense for the SD/MMC driver, but do you think I can use the same approach for other drivers that this system manager touches? i.e. The ethernet IP's PHY setting is controlled by a register that is in the system manager as well. I have submitted this patch for enabling ethernet. It is making use of the driver platform specific driver calls to touch the system manager. http://lists.infradead.org/pipermail/linux-arm-kernel/2013-September/200173.html Just wondering if that is right approach for the ethernet driver? If not, then I think I may have to come up with a generic system manager driver so that it can be used for other IPs. Thanks, Dinh >>> >>> Is this what you have tried before?
On Tuesday 15 October 2013, Dinh Nguyen wrote: > This makes sense for the SD/MMC driver, but do you think I can use the > same approach for > other drivers that this system manager touches? It might not be as straightforward for every driver, but something like this should work at least for some of the more interesting ones. For the rest, you can have a system manager driver that exports functions. > i.e. The ethernet IP's > PHY setting is controlled > by a register that is in the system manager as well. > > I have submitted this patch for enabling ethernet. It is making use of > the driver platform specific > driver calls to touch the system manager. > > http://lists.infradead.org/pipermail/linux-arm-kernel/2013-September/200173.html I definitely don't want to see auxdata getting used for stuff like this. In case of the ethernet driver, I suspect an SoC specific PHY driver would be the right approach. > Just wondering if that is right approach for the ethernet driver? If > not, then I think I may have > to come up with a generic system manager driver so that it can be used > for other IPs. The general way of handling this should be to see if there is a subsystem available for handling a particular feature. In case of clk and phy, we have those subsystems, so it's a matter of writing an appropriate driver that communicates with the high-level drivers using that. I don't know what other features are provided by the system manager that might need a different solution, but if you can't come up with a nice solution, you can either have an soc specific portion in the device driver (as you suggested for sdmmc first), or you can have that platform specific mfd driver exporting some functions. If the functionality is actually something common but doesn't have a subsystem in Linux yet, the "right" approach might also be to introduce a new subsystem, which I realize can be a lot of work. Arnd
diff --git a/arch/arm/mach-socfpga/Makefile b/arch/arm/mach-socfpga/Makefile index 6dd7a93..e4ff8b9 100644 --- a/arch/arm/mach-socfpga/Makefile +++ b/arch/arm/mach-socfpga/Makefile @@ -2,5 +2,5 @@ # Makefile for the linux kernel. # -obj-y := socfpga.o +obj-y := socfpga.o system_mgr.o obj-$(CONFIG_SMP) += headsmp.o platsmp.o diff --git a/arch/arm/mach-socfpga/core.h b/arch/arm/mach-socfpga/core.h index 572b8f7..b05fa6a 100644 --- a/arch/arm/mach-socfpga/core.h +++ b/arch/arm/mach-socfpga/core.h @@ -44,4 +44,10 @@ extern unsigned long cpu1start_addr; #define SOCFPGA_SCU_VIRT_BASE 0xfffec000 +/* SDMMC Group for System Manager defines */ +#define SYSMGR_SDMMCGRP_CTRL_OFFSET 0x108 +#define DRV_CLK_PHASE_SHIFT_SEL_MASK 0x7 +#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel) \ + ((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0)) + #endif diff --git a/arch/arm/mach-socfpga/system_mgr.c b/arch/arm/mach-socfpga/system_mgr.c new file mode 100644 index 0000000..9fdce1d --- /dev/null +++ b/arch/arm/mach-socfpga/system_mgr.c @@ -0,0 +1,28 @@ +/* + * Copyright Altera Corporation (C) 2013. All Rights Reserved. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along with + * this program. If not, see <http://www.gnu.org/licenses/>. + */ +#include <linux/of_platform.h> +#include <asm/mach/map.h> + +#include "core.h" + +void socfpga_sysmgr_set_dwmmc_drvsel_smpsel(u32 drvsel, u32 smplsel) +{ + u32 hs_timing; + + hs_timing = SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel); + writel(hs_timing, sys_manager_base_addr + SYSMGR_SDMMCGRP_CTRL_OFFSET); +} +EXPORT_SYMBOL(socfpga_sysmgr_set_dwmmc_drvsel_smpsel);