| Message ID | 1329809787-2788-1-git-send-email-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Tuesday 21 February 2012, Linus Walleij wrote: > > Hi Arnd, Olof, > > here are some accumulated patches for ux500 from the mailing > list, intended for the next (3.4) merge window. > > In order to somewhat make sure that they do not unnecessary > conflict with other stuff in the tree I pulled the whole > shebang into a branch off yesterdays linux-next and it worked > just fine, so it should be orthogonal to other changes. > > The main change is support for a new ASIC variant in the > families named DB9540 and its platform the U9540. Hi Linus, Most of the patches look ok, but since this about a new SoC, I decided to take a much closer look and found a few issues that I think you can still fix: * The handling of the DB9540_ROM adds a number of special cases, but in the end it looks like this is not used anywhere, so I'd recomment just removing it. If you get something that needs the ROM to be mapped, just make that use ioremap. * You add a new irqs-db9540.h file, but the macros defined in there are not actually used. Better not add that file then, especially since the interrupt definitions will soon all move into device tree for ux500. * I assume that we will need to add a few changes to make this actually work together with Lee's series. I would suggest that I put your series into the next/soc branch and merge it into the next/soc2 branch as well, which already has his patches. Then you can add a patch on top that adds the necessary changes, if any. Arnd
On Tue, Feb 21, 2012 at 6:00 PM, Arnd Bergmann <arnd@arndb.de> wrote: > Most of the patches look ok, but since this about a new SoC, > I decided to take a much closer look and found a few issues that > I think you can still fix: Actually it's a sibling to U8500 more than a completely new one, an incremental step. > * The handling of the DB9540_ROM adds a number of special cases, > but in the end it looks like this is not used anywhere, so I'd > recomment just removing it. If you get something that needs the > ROM to be mapped, just make that use ioremap. It is used to read out the ASIC ID, like this: +static unsigned int u9540_read_asicid(phys_addr_t addr) +{ + phys_addr_t base = addr & ~0xfff; + struct map_desc desc = { + .virtual = IO_ADDRESS_DB9540_ROM(base), + .pfn = __phys_to_pfn(base), + .length = SZ_16K, + .type = MT_DEVICE, + }; + + iotable_init(&desc, 1); + + /* As in devicemaps_init() */ + local_flush_tlb_all(); + flush_cache_all(); + + return readl(__io_address_db9540_rom(addr)); +} Notice that ioremap() cannot be used here since it is used at arch init time (as was discussed a while back with Nicolas I think). Most platforms need to use an approach like this to get chip ID:s at early init time. The read-out ASIC ID is then used for the cpu_is_u9540() function which is then used for the cpu_is_u8500_family() to ensure that the same codepath is used for the entire family. So if I remove this part the support patch is moot. > * You add a new irqs-db9540.h file, but the macros defined in there > are not actually used. Better not add that file then, especially > since the interrupt definitions will soon all move into device > tree for ux500. OK that's true, I've deleted it and will respin a v2 without the IRQ file, then iterate the pull request. > * I assume that we will need to add a few changes to make this > actually work together with Lee's series. It actually works just fine. I just merged this patchset into next-20120224, compiled and booted, it JustWorks(TM) because the changes are strangely enough orthogonal. > I would suggest that > I put your series into the next/soc branch and merge it into > the next/soc2 branch as well, which already has his patches. > Then you can add a patch on top that adds the necessary changes, > if any. Hm, not quite following but I think it will just work :-) Yours, Linus Walleij
On Friday 24 February 2012, Linus Walleij wrote: > On Tue, Feb 21, 2012 at 6:00 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > * The handling of the DB9540_ROM adds a number of special cases, > > but in the end it looks like this is not used anywhere, so I'd > > recomment just removing it. If you get something that needs the > > ROM to be mapped, just make that use ioremap. > > It is used to read out the ASIC ID, like this: > > +static unsigned int u9540_read_asicid(phys_addr_t addr) > +{ > + phys_addr_t base = addr & ~0xfff; > + struct map_desc desc = { > + .virtual = IO_ADDRESS_DB9540_ROM(base), > + .pfn = __phys_to_pfn(base), > + .length = SZ_16K, > + .type = MT_DEVICE, > + }; > + > + iotable_init(&desc, 1); > + > + /* As in devicemaps_init() */ > + local_flush_tlb_all(); > + flush_cache_all(); > + > + return readl(__io_address_db9540_rom(addr)); > +} Do I understand this correctly that there is a new chip that is almost the same as the old one, but the main difference is location of the register that tells us which one it is? ;-) > Notice that ioremap() cannot be used here since it is used at > arch init time (as was discussed a while back with Nicolas > I think). Most platforms need to use an approach like this > to get chip ID:s at early init time. > > The read-out ASIC ID is then used for the cpu_is_u9540() function > which is then used for the cpu_is_u8500_family() to ensure that > the same codepath is used for the entire family. > > So if I remove this part the support patch is moot. Ok, I see. How about hardcoding the virtual address for the id register so that you can actually use the same function:? static unsigned int ux500_read_asicid(phys_addr_t addr) { phys_addr_t base = addr & ~PAGE_MASK; unsigned long offset = addr & PAGE_MASK; struct map_desc desc = { .virtual = (unsigned long)UX500_ASICID_ADDR, .pfn = __phys_to_pfn(base), .length = SZ_4K, .type = MT_DEVICE, }; iotable_init(&desc, 1); /* As in devicemaps_init() */ local_flush_tlb_all(); flush_cache_all(); return readl(UX500_ASICID_ADDR + offset); } Then you just need to find a convenient value for UX500_ASICID_VIRT that does not conflict with the other static mappings. > > * I assume that we will need to add a few changes to make this > > actually work together with Lee's series. > > It actually works just fine. I just merged this patchset into > next-20120224, compiled and booted, it JustWorks(TM) because the > changes are strangely enough orthogonal. Ok, good. Arnd
On Fri, Feb 24, 2012 at 4:23 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Friday 24 February 2012, Linus Walleij wrote: >> On Tue, Feb 21, 2012 at 6:00 PM, Arnd Bergmann <arnd@arndb.de> wrote: > >> > * The handling of the DB9540_ROM adds a number of special cases, >> > but in the end it looks like this is not used anywhere, so I'd >> > recomment just removing it. If you get something that needs the >> > ROM to be mapped, just make that use ioremap. >> >> It is used to read out the ASIC ID, like this: >> >> +static unsigned int u9540_read_asicid(phys_addr_t addr) >> +{ >> + phys_addr_t base = addr & ~0xfff; >> + struct map_desc desc = { >> + .virtual = IO_ADDRESS_DB9540_ROM(base), >> + .pfn = __phys_to_pfn(base), >> + .length = SZ_16K, >> + .type = MT_DEVICE, >> + }; >> + >> + iotable_init(&desc, 1); >> + >> + /* As in devicemaps_init() */ >> + local_flush_tlb_all(); >> + flush_cache_all(); >> + >> + return readl(__io_address_db9540_rom(addr)); >> +} > > Do I understand this correctly that there is a new chip that > is almost the same as the old one, but the main difference is > location of the register that tells us which one it is? ;-) Not quite, we already know which main variant it is at this point, but reading this register tells us the sub-variant. They are numbered ED (early drop), v1, v2, etc. This location also tells the manufacturing process and that other stuff that Lee recently added to the sysfs files in the SoC bus. > How about hardcoding the virtual address for the id register so > that you can actually use the same function:? > > static unsigned int ux500_read_asicid(phys_addr_t addr) > { > phys_addr_t base = addr & ~PAGE_MASK; > unsigned long offset = addr & PAGE_MASK; > > struct map_desc desc = { > .virtual = (unsigned long)UX500_ASICID_ADDR, > .pfn = __phys_to_pfn(base), > .length = SZ_4K, > .type = MT_DEVICE, > }; > > iotable_init(&desc, 1); > > /* As in devicemaps_init() */ > local_flush_tlb_all(); > flush_cache_all(); > > return readl(UX500_ASICID_ADDR + offset); > } > > Then you just need to find a convenient value for UX500_ASICID_VIRT > that does not conflict with the other static mappings. But the problem is that the ASIC address is not at all constant across ASIC variants. They are on different places in the ROM for U8500, U5500. U9540... So as described above we already know the main type of ASIC, but we need to read this value to get the subvariant, process etc. So there is no such thing as UX500_ASICID_ADDR. Unless we defines it with #ifdefs and compile a kernel for just U8500, U5500 or U9540 and that's mainly what we want to get away from you know... Yours, Linus Walleij
On Friday 24 February 2012, Linus Walleij wrote: > > How about hardcoding the virtual address for the id register so > > that you can actually use the same function:? > > > > static unsigned int ux500_read_asicid(phys_addr_t addr) > > { > > phys_addr_t base = addr & ~PAGE_MASK; > > unsigned long offset = addr & PAGE_MASK; > > > > struct map_desc desc = { > > .virtual = (unsigned long)UX500_ASICID_ADDR, > > .pfn = __phys_to_pfn(base), > > .length = SZ_4K, > > .type = MT_DEVICE, > > }; > > > > iotable_init(&desc, 1); > > > > /* As in devicemaps_init() */ > > local_flush_tlb_all(); > > flush_cache_all(); > > > > return readl(UX500_ASICID_ADDR + offset); > > } > > > > Then you just need to find a convenient value for UX500_ASICID_VIRT > > that does not conflict with the other static mappings. > > But the problem is that the ASIC address is not > at all constant across ASIC variants. They are on different > places in the ROM for U8500, U5500. U9540... So as described > above we already know the main type of ASIC, but we need to > read this value to get the subvariant, process etc. > > So there is no such thing as UX500_ASICID_ADDR. > > Unless we defines it with #ifdefs and compile a kernel for just > U8500, U5500 or U9540 and that's mainly what we want to > get away from you know... My point was that you use the same virtual address here independent of which physical address is used. The physical address already gets passed into the function as its argument, we only need to map it to a convenient place in order to read it! Arnd
On Fri, Feb 24, 2012 at 5:25 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Friday 24 February 2012, Linus Walleij wrote: >> > Then you just need to find a convenient value for UX500_ASICID_VIRT >> > that does not conflict with the other static mappings. >> >> But the problem is that the ASIC address is not >> at all constant across ASIC variants. They are on different >> places in the ROM for U8500, U5500. U9540... So as described >> above we already know the main type of ASIC, but we need to >> read this value to get the subvariant, process etc. >> >> So there is no such thing as UX500_ASICID_ADDR. >> >> Unless we defines it with #ifdefs and compile a kernel for just >> U8500, U5500 or U9540 and that's mainly what we want to >> get away from you know... > > My point was that you use the same virtual address here independent > of which physical address is used. The physical address already gets > passed into the function as its argument, we only need to map it > to a convenient place in order to read it! Oh sorry I'm slow in the head :-/ Yes that should work of course, I will attempt to make an additional patch on top of these that consolidates the whole thing into one function that does that. Thanks Arnd! Linus Walleij
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hi Arnd, Olof, here are some accumulated patches for ux500 from the mailing list, intended for the next (3.4) merge window. In order to somewhat make sure that they do not unnecessary conflict with other stuff in the tree I pulled the whole shebang into a branch off yesterdays linux-next and it worked just fine, so it should be orthogonal to other changes. The main change is support for a new ASIC variant in the families named DB9540 and its platform the U9540. Thanks, Linus Walleij The following changes since commit b01543dfe67bb1d191998e90d20534dc354de059: Linux 3.3-rc4 (2012-02-18 15:53:33 -0800) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-stericsson.git/ for-arm-soc-20120220 Linus Walleij (1): ARM: ux500: core U9540 support Michel Jaouen (1): ARM: ux500: ioremap differences for DB9540 Ola Lilja (3): ARM: ux500: Add placeholder for clk_set_parent ARM: ux500: Add DMA-channels for MSP ARM: ux500: Add audio-regulators Philippe Langlais (2): ARM: ux500: set ARCH_NR_GPIO to 355 on U8500 platforms ARM: ux500: fix around AB8500 GPIO macro name arch/arm/Kconfig | 2 +- arch/arm/mach-ux500/board-mop500-regulators.c | 28 +++++++++++ arch/arm/mach-ux500/board-mop500-uib.c | 2 +- arch/arm/mach-ux500/board-mop500.c | 2 +- arch/arm/mach-ux500/board-mop500.h | 2 +- arch/arm/mach-ux500/cache-l2x0.c | 14 +++++- arch/arm/mach-ux500/clock.c | 9 +++- arch/arm/mach-ux500/clock.h | 1 + arch/arm/mach-ux500/cpu-db8500.c | 24 ++++++++-- arch/arm/mach-ux500/cpu.c | 4 +- arch/arm/mach-ux500/devices-db8500.c | 6 +++ arch/arm/mach-ux500/id.c | 26 +++++++++++ arch/arm/mach-ux500/include/mach/db8500-regs.h | 6 +++ arch/arm/mach-ux500/include/mach/hardware.h | 12 +++++ arch/arm/mach-ux500/include/mach/id.h | 17 +++++++- arch/arm/mach-ux500/include/mach/irqs-db9540.h | 58 ++++++++++++++++++++++++ arch/arm/mach-ux500/include/mach/irqs.h | 3 +- arch/arm/mach-ux500/include/mach/setup.h | 7 +++ arch/arm/mach-ux500/platsmp.c | 4 +- arch/arm/mach-ux500/timer.c | 2 +- drivers/cpufreq/db8500-cpufreq.c | 2 +- 21 files changed, 212 insertions(+), 19 deletions(-) create mode 100644 arch/arm/mach-ux500/include/mach/irqs-db9540.h -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) iQIcBAEBAgAGBQJPQ0ikAAoJEEEQszewGV1zB0wP/jeRLhLzYPYMOrJZR0ZD2ntR uMukCtAnqaxxOtIv7vNT9Zy6duzmtiUUyfK8wwicLusT2oaiIubFcXgMC0FRERe9 dFlRMxyBU6CXX7piJjgZ6wedZzRqB8yiuMjKUbpMnPdhAr3BZDcxv4UuCnjZYfxd Hhboj0+wZm2qyvFYTGzMyPlTkzoXddHMDC4DsFDUifouPu9ovJeiCJlmXfPT0HYf tlE9Dytp6axtWUVlGduEMEgNU4IUUDxGOBtECSfqHAt0yyI28dGk0lftewiU3uj8 RwEuW88BJXX66aVTBVhsdealEkSjK48UFzCAs8A39LGhz0bT60GmI6GCS+yO89Cf feX8Qejxm3YzHUzWN3ZI2c6SHYrdOtoqlNSuvYdhjfa3vfCstYGRXB7pgVgBqSrS npbRhYkzGYcY8PZKcNBnC/1xtKE11UCb56CujteTAtsKALGZNZv8nmpGcHmRVmX9 YBgwA/c/p+/G5DyMe4V8tWE0hYfyVmkRUBAtCINDje4BRhuDVtE7HhGKEPZtgKvw ZVV3UfXVE5/qEoZ5u5xHS9OwbHZEQQdgUN7QwpBjUSue9QO4rz3iN2Gu0YMWtp2I F+hNBcGelAafL7ohqBa1xnTz7pgwV81VUWzGiqcG2htmLY2KblBUnZIIk0jvNC1u XoXMj6/h2ta0xFrMT3ys =3MW5 -----END PGP SIGNATURE-----