Messages in this thread | | | Date | Tue, 06 May 2014 15:57:24 +0900 | From | Pankaj Dubey <> | Subject | Re: [PATCH 0/4] Introducing Exynos ChipId driver |
| |
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
| |