From: Marek Szyprowski <m.szyprowski@samsung.com> To: 'Harald Welte' <laforge@gnumonks.org> Cc: linux-samsung-soc@vger.kernel.org, kyungmin.park@samsung.com, bhmin@samsung.com, ben-linux@fluff.org, linux-arm-kernel@lists.infradead.org, Marek Szyprowski <m.szyprowski@samsung.com> Subject: RE: [PATCH 02/17] ARM: S5PC1XX: registers rename Date: Fri, 06 Nov 2009 16:02:17 +0100 [thread overview] Message-ID: <000401ca5ef2$24a09240$6de1b6c0$%szyprowski@samsung.com> (raw) In-Reply-To: <20091106031845.GG3913@prithivi.gnumonks.org> Hello, On Friday, November 06, 2009 4:19 AM, Harald Welte wrote: > Hi Marek, > > On Tue, Oct 13, 2009 at 10:11:07AM +0200, Marek Szyprowski wrote: > > > S5PC110 and S5PC100 register maps differs in many places, rename all > > defined registers to be S5PC100 specific. System map has been also updated > > to cover more integrated peripherals. > > The general idea of this patch is fine. However, I have some questions: > > > /* System */ > > -#define S5PC100_PA_SYS (0xE0100000) > > -#define S5PC100_PA_CLK (S5PC100_PA_SYS + 0x0) > > -#define S5PC100_PA_PWR (S5PC100_PA_SYS + 0x8000) > > +#define S5PC100_PA_CLK (0xE0100000) > > +#define S5PC100_PA_CLK_OTHER (0xE0200000) > > +#define S5PC100_PA_PWR (0xE0108000) > > this is more like a rename. Why was this done? It would be good to explain in > the commitlog I renamed these registers to better match the chip specification. The 'SYS' register name was borrowed from S3C64XX series and is a bit inadequate in C100. > > +/* GPIO */ > > +#define S5PC100_PA_GPIO (0xE0300000) > > +#define S5PC1XX_PA_GPIO S5PC100_PA_GPIO > > +#define S5PC1XX_VA_GPIO S3C_ADDR(0x00500000) > > If the address is different for c100 and c110: why do we need a S5CP1XX_* > definition? In my personal opinion, all those compile-time defines are a > kludge and we should not introduce more of them. They will bite us in the back > if we ever in the future want to build a kernel that can boot on both c100 and > c110. These C1XX defines were the first step to prepare the code for C110 support. S5PC110 register map differs completely from the S5PC100 one. These two SOCs cannot be easily handled by the same kernel binary image without some hacks and runtime fixups if we place them in the one kernel platform. Creating yet another kernel platform just because of these differences would unnecessarily duplicate a lot of code and would not use the potential of the current kernel infrastructure (separate include directories, resource&device driver model, etc). My idea was to introduce a sub-platforms in plat-s5pc1xx. Such sub-platforms would be exclusive - one for C100 and one for C110. Each of the sub-platforms would have its own include files (in mach-s5pc100 and mach-s5pc110 directories respectively) and most of the chip differences can be handled in compile time by proper macros. Macros for the common resources would use 'C1XX' names. In this approach common resources (GPIO, DMA, io space and so on) can be easily defined with C1XX defines. This also perfectly matches the current convention of common S3C_XXX defines (i.e. S3C_PA_UART, S3C_PA_FB, S3C_VA_VICn, S3C_PA_HSMMCn, and so on), so most of the code from arch/arm/plat-s3c/ can be reused without any modifications. I assume that the S3C prefix would be renamed to SAMSUNG sometime later. Sub-platform can be easily defined by a simple choice command in arch/arm/plat-s5pc1xx/Kconfig: +choice + prompt "S5PC1xx SoC Type" + default ARCH_S5PC100 + +config ARCH_S5PC100 + bool "S5PC100" + +config ARCH_S5PC110 + bool "S5PC110" + +endchoice Then each sub-platform can be easily defined basing on the CONFIG_ARCH_S5PC100 and CONFIG_ARCH_S5PC110 defines. Similar approach is used in OMAP platform (there is only one platform directory, and the chip series is defined by a Kconfig choice option). > > .io_pg_offst = (((u32)S5PC1XX_VA_UART) >> 18) & 0xfffc, > > - .boot_params = S5PC100_PA_SDRAM + 0x100, > > + .boot_params = S5PC1XX_PA_SDRAM + 0x100, > > This is the wrong kind of change, from my point of view. We don't know yet > if all future s5pc1xx products will also have the same address, do we? Right, I missed this one. Best regards -- Marek Szyprowski Samsung Poland R&D Center
WARNING: multiple messages have this Message-ID (diff)
From: m.szyprowski@samsung.com (Marek Szyprowski) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 02/17] ARM: S5PC1XX: registers rename Date: Fri, 06 Nov 2009 16:02:17 +0100 [thread overview] Message-ID: <000401ca5ef2$24a09240$6de1b6c0$%szyprowski@samsung.com> (raw) In-Reply-To: <20091106031845.GG3913@prithivi.gnumonks.org> Hello, On Friday, November 06, 2009 4:19 AM, Harald Welte wrote: > Hi Marek, > > On Tue, Oct 13, 2009 at 10:11:07AM +0200, Marek Szyprowski wrote: > > > S5PC110 and S5PC100 register maps differs in many places, rename all > > defined registers to be S5PC100 specific. System map has been also updated > > to cover more integrated peripherals. > > The general idea of this patch is fine. However, I have some questions: > > > /* System */ > > -#define S5PC100_PA_SYS (0xE0100000) > > -#define S5PC100_PA_CLK (S5PC100_PA_SYS + 0x0) > > -#define S5PC100_PA_PWR (S5PC100_PA_SYS + 0x8000) > > +#define S5PC100_PA_CLK (0xE0100000) > > +#define S5PC100_PA_CLK_OTHER (0xE0200000) > > +#define S5PC100_PA_PWR (0xE0108000) > > this is more like a rename. Why was this done? It would be good to explain in > the commitlog I renamed these registers to better match the chip specification. The 'SYS' register name was borrowed from S3C64XX series and is a bit inadequate in C100. > > +/* GPIO */ > > +#define S5PC100_PA_GPIO (0xE0300000) > > +#define S5PC1XX_PA_GPIO S5PC100_PA_GPIO > > +#define S5PC1XX_VA_GPIO S3C_ADDR(0x00500000) > > If the address is different for c100 and c110: why do we need a S5CP1XX_* > definition? In my personal opinion, all those compile-time defines are a > kludge and we should not introduce more of them. They will bite us in the back > if we ever in the future want to build a kernel that can boot on both c100 and > c110. These C1XX defines were the first step to prepare the code for C110 support. S5PC110 register map differs completely from the S5PC100 one. These two SOCs cannot be easily handled by the same kernel binary image without some hacks and runtime fixups if we place them in the one kernel platform. Creating yet another kernel platform just because of these differences would unnecessarily duplicate a lot of code and would not use the potential of the current kernel infrastructure (separate include directories, resource&device driver model, etc). My idea was to introduce a sub-platforms in plat-s5pc1xx. Such sub-platforms would be exclusive - one for C100 and one for C110. Each of the sub-platforms would have its own include files (in mach-s5pc100 and mach-s5pc110 directories respectively) and most of the chip differences can be handled in compile time by proper macros. Macros for the common resources would use 'C1XX' names. In this approach common resources (GPIO, DMA, io space and so on) can be easily defined with C1XX defines. This also perfectly matches the current convention of common S3C_XXX defines (i.e. S3C_PA_UART, S3C_PA_FB, S3C_VA_VICn, S3C_PA_HSMMCn, and so on), so most of the code from arch/arm/plat-s3c/ can be reused without any modifications. I assume that the S3C prefix would be renamed to SAMSUNG sometime later. Sub-platform can be easily defined by a simple choice command in arch/arm/plat-s5pc1xx/Kconfig: +choice + prompt "S5PC1xx SoC Type" + default ARCH_S5PC100 + +config ARCH_S5PC100 + bool "S5PC100" + +config ARCH_S5PC110 + bool "S5PC110" + +endchoice Then each sub-platform can be easily defined basing on the CONFIG_ARCH_S5PC100 and CONFIG_ARCH_S5PC110 defines. Similar approach is used in OMAP platform (there is only one platform directory, and the chip series is defined by a Kconfig choice option). > > .io_pg_offst = (((u32)S5PC1XX_VA_UART) >> 18) & 0xfffc, > > - .boot_params = S5PC100_PA_SDRAM + 0x100, > > + .boot_params = S5PC1XX_PA_SDRAM + 0x100, > > This is the wrong kind of change, from my point of view. We don't know yet > if all future s5pc1xx products will also have the same address, do we? Right, I missed this one. Best regards -- Marek Szyprowski Samsung Poland R&D Center
next prev parent reply other threads:[~2009-11-06 15:02 UTC|newest] Thread overview: 76+ messages / expand[flat|nested] mbox.gz Atom feed top 2009-10-13 8:11 [PATCH] Update Samsung S5PC100 SoC support Marek Szyprowski 2009-10-13 8:11 ` Marek Szyprowski 2009-10-13 8:11 ` [PATCH 01/17] ARM: MM: use 64bytes of L1 cache on plat S5PC1xx Marek Szyprowski 2009-10-13 8:11 ` Marek Szyprowski 2009-10-13 8:11 ` [PATCH 02/17] ARM: S5PC1XX: registers rename Marek Szyprowski 2009-10-13 8:11 ` Marek Szyprowski 2009-10-13 8:11 ` [PATCH 03/17] ARM: S5PC1XX: clock " Marek Szyprowski 2009-10-13 8:11 ` Marek Szyprowski 2009-10-13 8:11 ` [PATCH 04/17] ARM: S5PC1XX: clocks reimplementation Marek Szyprowski 2009-10-13 8:11 ` Marek Szyprowski 2009-10-13 8:11 ` [PATCH 05/17] ARM: S5PC1XX: add GPIO L banks to register definition Marek Szyprowski 2009-10-13 8:11 ` Marek Szyprowski 2009-10-13 8:11 ` [PATCH 06/17] ARM: S5PC1XX: GPIO registers rename Marek Szyprowski 2009-10-13 8:11 ` Marek Szyprowski 2009-10-13 8:11 ` [PATCH 07/17] ARM: S5PC1xx: add gpiolib and external/gpio interrupt support Marek Szyprowski 2009-10-13 8:11 ` Marek Szyprowski 2009-10-13 8:11 ` [PATCH 08/17] ARM: S5PC1XX: add cpu idle and system reset support Marek Szyprowski 2009-10-13 8:11 ` Marek Szyprowski 2009-10-13 8:11 ` [PATCH 09/17] ARM: S5PC1xx: add platform helpers for s3c-fb device Marek Szyprowski 2009-10-13 8:11 ` Marek Szyprowski 2009-10-13 8:11 ` [PATCH 10/17] SMDKC100: enable S3C FrameBuffer Marek Szyprowski 2009-10-13 8:11 ` Marek Szyprowski 2009-10-13 8:11 ` [PATCH 11/17] drivers: fb: enable S3C FrameBuffer on S5PC1XX platform Marek Szyprowski 2009-10-13 8:11 ` Marek Szyprowski 2009-10-13 8:11 ` [PATCH 12/17] ARM: S5PC1xx: add platform helpers for i2c adapter devices Marek Szyprowski 2009-10-13 8:11 ` Marek Szyprowski 2009-10-13 8:11 ` [PATCH 13/17] SMDKC100: add I2C0 and I2C1 buses support Marek Szyprowski 2009-10-13 8:11 ` Marek Szyprowski 2009-10-13 8:11 ` [PATCH 14/17] drivers: i2c: s3c2410-i2c also on S5PC1XX platform Marek Szyprowski 2009-10-13 8:11 ` Marek Szyprowski 2009-10-13 8:11 ` [PATCH 15/17] ARM: S5PC1xx: add platform helpers for SDHCI host controllers Marek Szyprowski 2009-10-13 8:11 ` Marek Szyprowski 2009-10-13 8:11 ` [PATCH 16/17] SMDKC100: add SDHCI controllers 0, 1 and 2 support Marek Szyprowski 2009-10-13 8:11 ` Marek Szyprowski 2009-10-13 8:11 ` [PATCH 17/17] drivers: MMC: enable SDHCI-S3C on S5PC1XX platform Marek Szyprowski 2009-10-13 8:11 ` Marek Szyprowski 2009-11-06 3:31 ` [PATCH 15/17] ARM: S5PC1xx: add platform helpers for SDHCI host controllers Harald Welte 2009-11-06 3:31 ` Harald Welte 2009-11-06 3:30 ` [PATCH 13/17] SMDKC100: add I2C0 and I2C1 buses support Harald Welte 2009-11-06 3:30 ` Harald Welte 2009-11-06 3:30 ` [PATCH 12/17] ARM: S5PC1xx: add platform helpers for i2c adapter devices Harald Welte 2009-11-06 3:30 ` Harald Welte 2009-11-06 3:29 ` [PATCH 10/17] SMDKC100: enable S3C FrameBuffer Harald Welte 2009-11-06 3:29 ` Harald Welte 2009-11-06 3:29 ` [PATCH 09/17] ARM: S5PC1xx: add platform helpers for s3c-fb device Harald Welte 2009-11-06 3:29 ` Harald Welte 2009-11-09 0:02 ` Ben Dooks 2009-11-09 0:02 ` Ben Dooks 2009-11-06 3:26 ` [PATCH 08/17] ARM: S5PC1XX: add cpu idle and system reset support Harald Welte 2009-11-06 3:26 ` Harald Welte 2009-11-06 3:23 ` [PATCH 07/17] ARM: S5PC1xx: add gpiolib and external/gpio interrupt support Harald Welte 2009-11-06 3:23 ` Harald Welte 2009-11-09 0:00 ` Ben Dooks 2009-11-09 0:00 ` Ben Dooks 2009-11-06 3:20 ` [PATCH 06/17] ARM: S5PC1XX: GPIO registers rename Harald Welte 2009-11-06 3:20 ` Harald Welte 2009-11-06 3:20 ` [PATCH 05/17] ARM: S5PC1XX: add GPIO L banks to register definition Harald Welte 2009-11-06 3:20 ` Harald Welte 2009-11-06 3:19 ` [PATCH 03/17] ARM: S5PC1XX: clock registers rename Harald Welte 2009-11-06 3:19 ` Harald Welte 2009-11-06 3:18 ` [PATCH 02/17] ARM: S5PC1XX: " Harald Welte 2009-11-06 3:18 ` Harald Welte 2009-11-06 15:02 ` Marek Szyprowski [this message] 2009-11-06 15:02 ` Marek Szyprowski 2009-11-09 0:07 ` Ben Dooks 2009-11-09 0:07 ` Ben Dooks 2009-11-06 8:17 ` tommy.hong 2009-11-06 2:32 ` [PATCH 01/17] ARM: MM: use 64bytes of L1 cache on plat S5PC1xx Harald Welte 2009-11-06 2:32 ` Harald Welte 2009-11-08 23:58 ` Ben Dooks 2009-11-08 23:58 ` Ben Dooks 2009-11-09 2:41 ` Kyungmin Park 2009-11-09 2:41 ` Kyungmin Park 2009-11-09 0:05 ` [PATCH] Update Samsung S5PC100 SoC support Ben Dooks 2009-11-09 0:05 ` Ben Dooks -- strict thread matches above, loose matches on Subject: below -- 2009-10-13 7:56 Marek Szyprowski 2009-10-13 7:56 ` [PATCH 01/17] ARM: MM: use 64bytes of L1 cache on plat S5PC1xx Marek Szyprowski 2009-10-13 7:56 ` [PATCH 02/17] ARM: S5PC1XX: registers rename Marek Szyprowski
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='000401ca5ef2$24a09240$6de1b6c0$%szyprowski@samsung.com' \ --to=m.szyprowski@samsung.com \ --cc=ben-linux@fluff.org \ --cc=bhmin@samsung.com \ --cc=kyungmin.park@samsung.com \ --cc=laforge@gnumonks.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-samsung-soc@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.