Re: [PATCH 0/4] Introducing Exynos ChipId driver
On Sunday 11 May 2014 18:47:28 Olof Johansson wrote: > > > 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. > > The generic solution is already there: of_machine_is_compatible() is perfectly > sensible to use for _some_ of these inits. Cpufreq is one of the few that > comes > to mind, and maybe some of the platsmp and pm stuff. > > Note that none of them should be used in runtime, i.e. you should only use > them > at probe/setup time and maybe have a local state in the driver if needed. > > I'd rather get people used to that format than everybody needing to implement > a chipid driver now too, especially on platforms that might not even have a > suitable chipid block to base a driver around -- not to mention having to > fill the namespace with is_soc_*() stuff. I was coming from the other angle: exynos is already using soc_is_*() in too many places. I'd like to first see the ones cleaned up that really should be doing something else because they have a device-local ID to look at. If we end up with a couple of instances that don't have a good alternative, we can use of_machine_is_compatible() for those, but I'd like to avoid doing a blind conversion because that would likely lead to more instances in the future, not fewer. I agree that we should have to introduce new chip ID drivers on other platforms for the purpose of finding out the SoC version, especially not with private APIs. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] Introducing Exynos ChipId driver
(Taking the discussion here since Panjak pointed me to this thread when
I commented on the latest patch set)
On Mon, May 05, 2014 at 04:58:14PM +0200, 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.
>
> > > 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";
Actually, "generic" compatibles for a device isn't the right thing to do
here. For machine-level compat it makes sense, but not for devices. There
it should instead use the first version of the IP block.
> > 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.
>
> 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.
>
> 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_i
Re: [PATCH 0/4] Introducing Exynos ChipId driver
On Tuesday 06 May 2014 15:57:24 Pankaj Dubey wrote:
> 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.
Yes, makes sense. Let's see if we can convince Rob first though, he
has some reservations.
> >> 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.
ok.
> > 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.
Ok, cool.
> >> 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 revi
Re: [PATCH 0/4] Introducing Exynos ChipId driver
On Monday 05 May 2014 10:34:02 Rob Herring wrote: > > > Ideally this should be done by slightly restructuring the DT > > source to make all on-chip devices appear below the soc node. > > We'd have to think a bit about how to best do this while > > preserving compatibility with existing dts files. > > I don't agree. How is a block with chip ID info the parent of all the > other devices? > > In doing some work to move default of_platform_populate out of > platforms, I noticed that most platforms using the soc device are > making it the parent of platform devices. I think this is either wrong > or all platforms should have a default soc device. It makes little > sense for some platforms to have a devices under a soc sysfs directory > while others do not. Or the location changes when a platform latter > adds the soc device. We had a long discussion about this when we introduced the drivers/soc framework. The intention is that the /sys/devices/soc* node is meant to describe the SoC in its entirety, the same way you have a pci:00 node as the root of all PCI devices of the first pci host bridge on a PC system. This also reflects the reality of a SoC, which normally has one bus that the CPU is connected to and that has all the other devices as children. Having the chipid registers as part of the top-level bus should not be interpreted as having the other devices as children of the chipid device, but rather the chipid registers as a property of the soc itself as opposed to a random device that happens to be part of the soc. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 exy
Re: [PATCH 0/4] Introducing Exynos ChipId driver
On Sat, May 3, 2014 at 10:02 AM, Arnd Bergmann wrote: > On Saturday 03 May 2014 15:11:36 Pankaj Dubey wrote: >> This patch series attempts to get rid of soc_is_exynos macros >> and eventually with the help of this series we can probably get >> rid of CONFIG_SOC_EXYNOS in near future. >> Each Exynos SoC has ChipID block which can give information about >> SoC's product Id and revision number. Currently we have single >> DT binding information for this as "samsung,exynos4210-chipid". >> But Exynos4 and Exynos5 SoC series have one small difference in >> chip Id, with resepect to product id bit-masks. So it means we >> should have separate compatible string for these different series >> of SoCs. So I have created new binding information for handling >> this difference. Also currently I can think of putting this driver >> code under "drivers/misc/" but suggestions are welcome. >> Also current form of driver is missing platfrom driver and needs >> init function to be called from machine file (either exynos.c or >> platsmp.c). I hope lot of suggestions and comments to improve this >> further. >> >> This patch series is based on Kukjin Kim's for-next (3.14_rc1 tag) >> and prepared on top of following patch series and it's dependent >> patch series. > > I think putting it into drivers/soc would be most appropriate. > We already have a few drivers lined up that we want in there, > although the directory currently doesn't exist. > > However, I would ask that you use the infrastructure provided by > drivers/base/soc.c when you add this driver, to also make the > information available to user space using a standard API. Agreed. > Ideally this should be done by slightly restructuring the DT > source to make all on-chip devices appear below the soc node. > We'd have to think a bit about how to best do this while > preserving compatibility with existing dts files. I don't agree. How is a block with chip ID info the parent of all the other devices? In doing some work to move default of_platform_populate out of platforms, I noticed that most platforms using the soc device are making it the parent of platform devices. I think this is either wrong or all platforms should have a default soc device. It makes little sense for some platforms to have a devices under a soc sysfs directory while others do not. Or the location changes when a platform latter adds the soc device. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] Introducing Exynos ChipId driver
On Monday 05 May 2014 16:58:14 Arnd Bergmann wrote: > > 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. Actually, a good compromise for now would be to add the chipid driver to mach-exynos instead of drivers/bus. This way you can keep the uses of the ID local to the exynos platform code until it's no longer needed. Then it can get moved out to drivers/soc to be shared with arm64. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] Introducing Exynos ChipId driver
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.
> > 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";
> 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.
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.
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.
> 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_exyn
Re: [PATCH 0/4] Introducing Exynos ChipId driver
Hi Arnd,
Thanks for review and suggestions.
On 05/04/2014 12:02 AM, Arnd Bergmann wrote:
On Saturday 03 May 2014 15:11:36 Pankaj Dubey wrote:
This patch series attempts to get rid of soc_is_exynos macros
and eventually with the help of this series we can probably get
rid of CONFIG_SOC_EXYNOS in near future.
Each Exynos SoC has ChipID block which can give information about
SoC's product Id and revision number. Currently we have single
DT binding information for this as "samsung,exynos4210-chipid".
But Exynos4 and Exynos5 SoC series have one small difference in
chip Id, with resepect to product id bit-masks. So it means we
should have separate compatible string for these different series
of SoCs. So I have created new binding information for handling
this difference. Also currently I can think of putting this driver
code under "drivers/misc/" but suggestions are welcome.
Also current form of driver is missing platfrom driver and needs
init function to be called from machine file (either exynos.c or
platsmp.c). I hope lot of suggestions and comments to improve this
further.
This patch series is based on Kukjin Kim's for-next (3.14_rc1 tag)
and prepared on top of following patch series and it's dependent
patch series.
I think putting it into drivers/soc would be most appropriate.
We already have a few drivers lined up that we want in there,
although the directory currently doesn't exist.
OK. Will move to "drivers/soc". I can see already some patches
have been posted [1] by Andy Gross for creating directory
"drivers/soc" I will use those patches for adding samsung folder
under "drivers/soc".
[1]: https://lkml.org/lkml/2014/4/21/46
However, I would ask that you use the infrastructure provided by
drivers/base/soc.c when you add this driver, to also make the
information available to user space using a standard API.
OK. Will update in next version.
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.
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?
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?
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?
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?
[2]: http://thread.gmane.org/gmane.linux.kernel.samsung-soc/29085
[3]: https://lkml.org/lkml/2014/5/2/612
The only user left in device drivers is now the cpufreq driver,
which is going to be replaced anyway, so that is ok. Having
a global variable that is accessible to random device drivers
is probably not a good idea though, it will just lead to
bad coding in drivers again.
To give an example of how I think it should really be restructured,
let's look at one function:
static const struct exynos_wkup_irq exynos4_wkup_irq[] = {
{ 76, BIT(1) }, /* RTC alarm */
{ 77, BIT(2) }, /* RTC tick */
{ /* sentinel */ },
};
static const struct exynos_wkup_irq exynos5250_wkup_irq[] = {
{ 75, BIT(1) }, /* RTC alarm */
{ 76, BIT(2) }, /* RTC tick */
{ /* sentinel */ },
};
static int exynos_irq_set_wake(struct irq_data *data, unsigned int state)
{
const struct exynos_wkup_irq *wkup_irq;
if (soc_is_exynos5250())
Re: [PATCH 0/4] Introducing Exynos ChipId driver
On Saturday 03 May 2014 15:11:36 Pankaj Dubey wrote:
> This patch series attempts to get rid of soc_is_exynos macros
> and eventually with the help of this series we can probably get
> rid of CONFIG_SOC_EXYNOS in near future.
> Each Exynos SoC has ChipID block which can give information about
> SoC's product Id and revision number. Currently we have single
> DT binding information for this as "samsung,exynos4210-chipid".
> But Exynos4 and Exynos5 SoC series have one small difference in
> chip Id, with resepect to product id bit-masks. So it means we
> should have separate compatible string for these different series
> of SoCs. So I have created new binding information for handling
> this difference. Also currently I can think of putting this driver
> code under "drivers/misc/" but suggestions are welcome.
> Also current form of driver is missing platfrom driver and needs
> init function to be called from machine file (either exynos.c or
> platsmp.c). I hope lot of suggestions and comments to improve this
> further.
>
> This patch series is based on Kukjin Kim's for-next (3.14_rc1 tag)
> and prepared on top of following patch series and it's dependent
> patch series.
I think putting it into drivers/soc would be most appropriate.
We already have a few drivers lined up that we want in there,
although the directory currently doesn't exist.
However, I would ask that you use the infrastructure provided by
drivers/base/soc.c when you add this driver, to also make the
information available to user space using a standard API.
Ideally this should be done by slightly restructuring the DT
source to make all on-chip devices appear below the soc node.
We'd have to think a bit about how to best do this while
preserving compatibility with existing dts files.
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.
The only user left in device drivers is now the cpufreq driver,
which is going to be replaced anyway, so that is ok. Having
a global variable that is accessible to random device drivers
is probably not a good idea though, it will just lead to
bad coding in drivers again.
To give an example of how I think it should really be restructured,
let's look at one function:
static const struct exynos_wkup_irq exynos4_wkup_irq[] = {
{ 76, BIT(1) }, /* RTC alarm */
{ 77, BIT(2) }, /* RTC tick */
{ /* sentinel */ },
};
static const struct exynos_wkup_irq exynos5250_wkup_irq[] = {
{ 75, BIT(1) }, /* RTC alarm */
{ 76, BIT(2) }, /* RTC tick */
{ /* sentinel */ },
};
static int exynos_irq_set_wake(struct irq_data *data, unsigned int state)
{
const struct exynos_wkup_irq *wkup_irq;
if (soc_is_exynos5250())
wkup_irq = exynos5250_wkup_irq;
else
wkup_irq = exynos4_wkup_irq;
...
}
There are multiple problems with this code:
- As mentioned, you depend on a specific SoC identification for
something that could be done completely generic.
- The knowledge about what is a wakeup source or not doesn't
really belong here. We don't have a DT binding for wakeups
as far as I'm aware of, but this should probably be handled
locally in the RTC device node, possibly in the node that
contains the S5P_WAKEUP_MASK register.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/4] Introducing Exynos ChipId driver
This patch series attempts to get rid of soc_is_exynos macros and eventually with the help of this series we can probably get rid of CONFIG_SOC_EXYNOS in near future. Each Exynos SoC has ChipID block which can give information about SoC's product Id and revision number. Currently we have single DT binding information for this as "samsung,exynos4210-chipid". But Exynos4 and Exynos5 SoC series have one small difference in chip Id, with resepect to product id bit-masks. So it means we should have separate compatible string for these different series of SoCs. So I have created new binding information for handling this difference. Also currently I can think of putting this driver code under "drivers/misc/" but suggestions are welcome. Also current form of driver is missing platfrom driver and needs init function to be called from machine file (either exynos.c or platsmp.c). I hope lot of suggestions and comments to improve this further. This patch series is based on Kukjin Kim's for-next (3.14_rc1 tag) and prepared on top of following patch series and it's dependent patch series. [1]: Map SYSRAM through generic SRAM bindings. http://www.spinics.net/lists/arm-kernel/msg327677.html [2]: Exynos PMU cleanup and refactoring. https://lkml.org/lkml/2014/4/30/44 Pankaj Dubey (4): ARM: EXYNOS: remove soc_is_exynos4/5 from exynos.c ARM: EXYNOS: remove unused header inclusion from hotplug.c misc: exynos-chipid: Add Exynos Chipid driver support ARM: EXYNOS: Refactoring to remove soc_is_exynos macros from exynos .../bindings/arm/samsung/exynos-chipid.txt | 15 arch/arm/Kconfig |1 + arch/arm/boot/dts/exynos4.dtsi |2 +- arch/arm/boot/dts/exynos5.dtsi |2 +- arch/arm/mach-exynos/exynos.c | 66 arch/arm/mach-exynos/hotplug.c |2 - arch/arm/mach-exynos/platsmp.c | 10 ++- arch/arm/mach-exynos/pm.c | 28 +++ arch/arm/plat-samsung/include/plat/cpu.h | 60 -- drivers/clk/samsung/clk-exynos4.c |2 +- drivers/cpufreq/exynos-cpufreq.c |9 +-- drivers/cpufreq/exynos-cpufreq.h |1 - drivers/cpufreq/exynos4x12-cpufreq.c |5 +- drivers/misc/Kconfig |7 ++ drivers/misc/Makefile |1 + drivers/misc/exynos-chipid.c | 83 include/linux/exynos-soc.h | 46 +++ 17 files changed, 215 insertions(+), 125 deletions(-) create mode 100644 Documentation/devicetree/bindings/arm/samsung/exynos-chipid.txt create mode 100644 drivers/misc/exynos-chipid.c create mode 100644 include/linux/exynos-soc.h -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
