From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pankaj Dubey Subject: Re: [PATCH 0/4] Introducing Exynos ChipId driver Date: Tue, 06 May 2014 15:57:24 +0900 Message-ID: <536887D4.90808@samsung.com> References: <1399097500-4052-1-git-send-email-pankaj.dubey@samsung.com> <13946837.RfnRDPQdge@wuerfel> <536758AB.2060503@samsung.com> <4239261.HFHm7SU1iH@wuerfel> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mailout2.samsung.com ([203.254.224.25]:20818 "EHLO mailout2.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933977AbaEFGjP (ORCPT ); Tue, 6 May 2014 02:39:15 -0400 In-reply-to: <4239261.HFHm7SU1iH@wuerfel> Sender: linux-samsung-soc-owner@vger.kernel.org List-Id: linux-samsung-soc@vger.kernel.org To: Arnd Bergmann Cc: linux-arm-kernel@lists.infradead.org, kgene.kim@samsung.com, linux@arm.linux.org.uk, t.figa@samsung.com, linux-kernel@vger.kernel.org, linux-samsung-soc@vger.kernel.org On 05/05/2014 11:58 PM, Arnd Bergmann wrote: > On Monday 05 May 2014 18:23:55 Pankaj Dubey wrote: >> On 05/04/2014 12:02 AM, Arnd Bergmann wrote: >>> Ideally this should be done by slightly restructuring the DT >>> source to make all on-chip devices appear below the soc node. >> Currently I can't see soc nodes in exynos4 and exynos5 DT files. >> So isn't it should be a separate patch first to modify all exynos4 >> exynos5 DT files to move all devices under soc node? >> In that case existing chipid node will be also moved under soc node. > Yes, that would be good. In fact the soc node could be identical > to the chipid node, effectively moving everything under chipid. OK, in that case I would like to keep this as separate patch once I do all other modifications. >>> We'd have to think a bit about how to best do this while >>> preserving compatibility with existing dts files. >> Is it necessary in this case? >> As I have mentioned there is difference in bit-mask among exynos4 >> and exynos5's chipid. So is this reason not sufficient to keep separate >> compatible for both? > Having two "compatible" values for exynos4 and exynos5 is not a problem, > and it absolutely makes sense to have more specific values in there > as well: > > compatible = "samsung,exynos4210-chipid", "samsung,exynos4-chipid"; OK, will keep compatible as you suggested. > >> Also even if we get some way to preserve existing compatibility, I afraid >> in chipid driver that implementation will not look good, at least I am not >> able to think of any good way. Any suggestions? > The compatibility I mean is to ensure everything keeps working if > the node is not present. > >>> Regarding patch 4, this is not what I meant when I asked for >>> removing the soc_is_exynos* macros. You basically do a 1:1 replacement >>> using a different interface, but you still have code that does >>> things differently based on a global identification. >> I agree with what you are trying to say. But if you see recently we had some >> patches (cpu_idle.c: [2], pmu.c: [3]) to remove usage of such macros from >> exynos machine files. So only leftover files using these macros are exynos.c >> platsmp.c and pm.c. >> >> For exynos.c I have tried to remove soc_is_exynos4/exynos5 by matching with >> compatible string in patch 1 of this series. Please let me know if that is OK? > I've taken a closer look at that file now. My preferred solution > would be to go back to having two machine descriptors as it > was before Sachin Kamat's "ARM: EXYNOS: Consolidate exynos4 and > exynos5 machine files", but keep it all in one file and consolidated > as much as possible, e.g. Yes, that case I do not need to add another function to compare compatible strings. So if there is no issues in having two separate machine descriptor I will do this modification in next version of patch. > > static void __init exynos_dt_machine_init(void) > { > exynos_cpuidle_init(); > exynos_cpufreq_init(); > > of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); > } > > static void __init exynos5_dt_machine_init(void) > { > /* > * Exynos5's legacy i2c controller and new high speed i2c > * controller have muxed interrupt sources. By default the > * interrupts for 4-channel HS-I2C controller are enabled. > * If node for first four channels of legacy i2c controller > * are available then re-configure the interrupts via the > * system register. > */ > struct device_node *i2c_np; > const char *i2c_compat = "samsung,s3c2440-i2c"; > unsigned int tmp; > int id; > > for_each_compatible_node(i2c_np, NULL, i2c_compat) { > if (of_device_is_available(i2c_np)) { > id = of_alias_get_id(i2c_np, "i2c"); > if (id < 4) { > tmp = readl(EXYNOS5_SYS_I2C_CFG); > writel(tmp & ~(0x1 << id), EXYNOS5_SYS_I2C_CFG); > } > } > } > > exynos_dt_machine_init(); > } > > This way you can avoid having another check of the compatible node. > In the long run, all of the this code should go away: The cpuidle > and cpufreq drivers should become normal platform drivers that > get probed when the devices are present (just like it's required > for arm64 anyway), and the EXYNOS5_SYS_I2C_CFG register should > get set up by an appropriate driver, e.g. the i2c driver through > syscon, or a pinmux driver that changes the mux between the > sources based on DT information, whatever fits best. OK, will move this in i2c driver and will use sysreg as syscon phandle. > > Similarly for exynos_map_io(), with the sysram out of the picture, > it can be > > void __init exynos4_init_io(void) > { > debug_ll_io_init(); > iotable_init(exynos4_iodesc, ARRAY_SIZE(exynos4_iodesc)); > } > > void __init exynos5_init_io(void) > { > debug_ll_io_init(); > iotable_init(exynos5_iodesc, ARRAY_SIZE(exynos4_iodesc)); > } > > but in the long run, it would be nicer to completely eliminate > exynos4_iodesc and exynos5_iodesc as well, and remove the init_io > functions. Note that debug_ll_io_init() is already called when > you have a NULL .map_io callback. Agreed. > >> Also for platsmp.c and pm.c I can think of following approaches >> 1: Keep these macros till we get generic solution? >> 2: Allow chipid driver to expose APIs to check SoC id and SoC revisions >> till we get >> generic solution. So that at least we can remove #ifdef based macros >> as soc_is_exynosXYZ. >> 3: Use of "of_flat_dt_is_compatible" or similar APIs in these machine files >> till we get >> generic solution. For some cases where we want to know SoC revision let us >> map chipid register and get revision there itself. >> >> Please let me know what approach you think will be good? > I think 1 or 2 would be better than 3. Between those two, I'm undecided, > but I think either way the SoC specific values would be better kept in the > mach-samsung directory than in plat/cpu.h or linux/exynos-chipid.h. OK, let me introduce this driver via "drivers/soc" in second revision, there also if we think it's not proper to expose such APIs or variable outside of the driver, I will be think to move it in under machine directory itself. > Arnd > -- Best Regards, Pankaj Dubey From mboxrd@z Thu Jan 1 00:00:00 1970 From: pankaj.dubey@samsung.com (Pankaj Dubey) Date: Tue, 06 May 2014 15:57:24 +0900 Subject: [PATCH 0/4] Introducing Exynos ChipId driver In-Reply-To: <4239261.HFHm7SU1iH@wuerfel> References: <1399097500-4052-1-git-send-email-pankaj.dubey@samsung.com> <13946837.RfnRDPQdge@wuerfel> <536758AB.2060503@samsung.com> <4239261.HFHm7SU1iH@wuerfel> Message-ID: <536887D4.90808@samsung.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 05/05/2014 11:58 PM, Arnd Bergmann wrote: > On Monday 05 May 2014 18:23:55 Pankaj Dubey wrote: >> On 05/04/2014 12:02 AM, Arnd Bergmann wrote: >>> Ideally this should be done by slightly restructuring the DT >>> source to make all on-chip devices appear below the soc node. >> Currently I can't see soc nodes in exynos4 and exynos5 DT files. >> So isn't it should be a separate patch first to modify all exynos4 >> exynos5 DT files to move all devices under soc node? >> In that case existing chipid node will be also moved under soc node. > Yes, that would be good. In fact the soc node could be identical > to the chipid node, effectively moving everything under chipid. OK, in that case I would like to keep this as separate patch once I do all other modifications. >>> We'd have to think a bit about how to best do this while >>> preserving compatibility with existing dts files. >> Is it necessary in this case? >> As I have mentioned there is difference in bit-mask among exynos4 >> and exynos5's chipid. So is this reason not sufficient to keep separate >> compatible for both? > Having two "compatible" values for exynos4 and exynos5 is not a problem, > and it absolutely makes sense to have more specific values in there > as well: > > compatible = "samsung,exynos4210-chipid", "samsung,exynos4-chipid"; OK, will keep compatible as you suggested. > >> Also even if we get some way to preserve existing compatibility, I afraid >> in chipid driver that implementation will not look good, at least I am not >> able to think of any good way. Any suggestions? > The compatibility I mean is to ensure everything keeps working if > the node is not present. > >>> Regarding patch 4, this is not what I meant when I asked for >>> removing the soc_is_exynos* macros. You basically do a 1:1 replacement >>> using a different interface, but you still have code that does >>> things differently based on a global identification. >> I agree with what you are trying to say. But if you see recently we had some >> patches (cpu_idle.c: [2], pmu.c: [3]) to remove usage of such macros from >> exynos machine files. So only leftover files using these macros are exynos.c >> platsmp.c and pm.c. >> >> For exynos.c I have tried to remove soc_is_exynos4/exynos5 by matching with >> compatible string in patch 1 of this series. Please let me know if that is OK? > I've taken a closer look at that file now. My preferred solution > would be to go back to having two machine descriptors as it > was before Sachin Kamat's "ARM: EXYNOS: Consolidate exynos4 and > exynos5 machine files", but keep it all in one file and consolidated > as much as possible, e.g. Yes, that case I do not need to add another function to compare compatible strings. So if there is no issues in having two separate machine descriptor I will do this modification in next version of patch. > > static void __init exynos_dt_machine_init(void) > { > exynos_cpuidle_init(); > exynos_cpufreq_init(); > > of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); > } > > static void __init exynos5_dt_machine_init(void) > { > /* > * Exynos5's legacy i2c controller and new high speed i2c > * controller have muxed interrupt sources. By default the > * interrupts for 4-channel HS-I2C controller are enabled. > * If node for first four channels of legacy i2c controller > * are available then re-configure the interrupts via the > * system register. > */ > struct device_node *i2c_np; > const char *i2c_compat = "samsung,s3c2440-i2c"; > unsigned int tmp; > int id; > > for_each_compatible_node(i2c_np, NULL, i2c_compat) { > if (of_device_is_available(i2c_np)) { > id = of_alias_get_id(i2c_np, "i2c"); > if (id < 4) { > tmp = readl(EXYNOS5_SYS_I2C_CFG); > writel(tmp & ~(0x1 << id), EXYNOS5_SYS_I2C_CFG); > } > } > } > > exynos_dt_machine_init(); > } > > This way you can avoid having another check of the compatible node. > In the long run, all of the this code should go away: The cpuidle > and cpufreq drivers should become normal platform drivers that > get probed when the devices are present (just like it's required > for arm64 anyway), and the EXYNOS5_SYS_I2C_CFG register should > get set up by an appropriate driver, e.g. the i2c driver through > syscon, or a pinmux driver that changes the mux between the > sources based on DT information, whatever fits best. OK, will move this in i2c driver and will use sysreg as syscon phandle. > > Similarly for exynos_map_io(), with the sysram out of the picture, > it can be > > void __init exynos4_init_io(void) > { > debug_ll_io_init(); > iotable_init(exynos4_iodesc, ARRAY_SIZE(exynos4_iodesc)); > } > > void __init exynos5_init_io(void) > { > debug_ll_io_init(); > iotable_init(exynos5_iodesc, ARRAY_SIZE(exynos4_iodesc)); > } > > but in the long run, it would be nicer to completely eliminate > exynos4_iodesc and exynos5_iodesc as well, and remove the init_io > functions. Note that debug_ll_io_init() is already called when > you have a NULL .map_io callback. Agreed. > >> Also for platsmp.c and pm.c I can think of following approaches >> 1: Keep these macros till we get generic solution? >> 2: Allow chipid driver to expose APIs to check SoC id and SoC revisions >> till we get >> generic solution. So that at least we can remove #ifdef based macros >> as soc_is_exynosXYZ. >> 3: Use of "of_flat_dt_is_compatible" or similar APIs in these machine files >> till we get >> generic solution. For some cases where we want to know SoC revision let us >> map chipid register and get revision there itself. >> >> Please let me know what approach you think will be good? > I think 1 or 2 would be better than 3. Between those two, I'm undecided, > but I think either way the SoC specific values would be better kept in the > mach-samsung directory than in plat/cpu.h or linux/exynos-chipid.h. OK, let me introduce this driver via "drivers/soc" in second revision, there also if we think it's not proper to expose such APIs or variable outside of the driver, I will be think to move it in under machine directory itself. > Arnd > -- Best Regards, Pankaj Dubey