| Message ID | 54882576.8020609@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, (adding linux-omap back to the loop) On Wed, Dec 10, 2014 at 04:20:30PM +0530, Lokesh Vutla wrote: > Hi Felipe, > > On Wednesday 10 December 2014 03:57 AM, Felipe Balbi wrote: > > Before this patch, HWMOD requires the existence > > of a struct omap_hwmod_class very early. > Yes, hwmod code looks for omap_hwmod_class entry before registering any hwmod. > > With the patch 4/7 omap_hwmod_class gets populated from dt very late after registration of the hwmod. > So all the hwmod which gets class data from dt never gets registered and state is always UNKNOWN. > Mostly making it unusable. > IMO this patch just masks the problem. > > In order to register hwmod we need to populate class data very early. > We can populate at the same place as how reg property is being parsed. > Call omap_hwmod_init_sysc() in _init() of the particular hwmod: > The below diff will help: > > diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c > index cbb908d..05ecf8a 100644 > --- a/arch/arm/mach-omap2/omap_hwmod.c > +++ b/arch/arm/mach-omap2/omap_hwmod.c > @@ -2415,6 +2415,116 @@ static int __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data, > return 0; > } > > +static int omap_hwmod_has_sysc_bindings(struct device_node *node) > +{ > + char *properties[] = { > + "ti,rev_offs", > + "ti,sysc_offs", > + "ti,syss_offs", > + "ti,sysc_flags", > + "ti,srst_udelay", > + "ti,idlemodes", > + "ti,clockact", > + "ti,sysc_type", > + NULL > + }; > + char **tmp = properties; > + > + while (*tmp) { > + if (of_property_read_bool(node, *tmp)) { > + return true; > + } > + tmp++; > + } > + > + return 0; > +} > + > +static int omap_hwmod_init_sysc(struct device_node *node, > + struct omap_hwmod *oh, int index) > +{ > + struct omap_hwmod_class *class = oh->class; > + struct omap_hwmod_class_sysconfig *sysc; > + int ret; > + int i; > + char name[128]; > + const char *tmp = oh->name; > + u32 prop; > + > + /* if data isn't provided by DT, skip */ > + if ((class && class->sysc) || !omap_hwmod_has_sysc_bindings(node)) > + return 0; > + > + class = kzalloc(sizeof(*class), GFP_KERNEL); > + if (!class) > + return -ENOMEM; > + > + i = 0; > + while (*tmp) { > + if (isalpha(*tmp)) > + name[i++] = *tmp; > + tmp++; > + } > + name[i] = '\0'; > + > + class->name = kzalloc(sizeof(name), GFP_KERNEL); > + if (!class->name) > + return -ENOMEM; > + strncpy(class->name, name, sizeof(name) - 1); > + > + sysc = kzalloc(sizeof(*sysc), GFP_KERNEL); > + if (!sysc) > + return -ENOMEM; > + > + ret = of_property_read_u32_index(node, "ti,rev_offs", index, &prop); > + if (!ret) > + sysc->rev_offs = prop; > + > + ret = of_property_read_u32_index(node, "ti,sysc_offs", index, &prop); > + if (!ret) > + sysc->sysc_offs = prop; > + > + ret = of_property_read_u32_index(node, "ti,syss_offs", index, &prop); > + if (!ret) > + sysc->syss_offs = prop; > + > + ret = of_property_read_u32_index(node, "ti,sysc_flags", index, &prop); > + if (!ret) > + sysc->sysc_flags = prop & 0xffff; > + > + ret = of_property_read_u32_index(node, "ti,srst_udelay", index, &prop); > + if (!ret) > + sysc->srst_udelay = prop & 0xff; > + > + ret = of_property_read_u32_index(node, "ti,idlemodes", index, &prop); > + if (!ret) > + sysc->idlemodes = prop & 0xff; > + > + ret = of_property_read_u32_index(node, "ti,clockact", index, &prop); > + if (!ret) > + sysc->clockact = prop & 0xff; > + > + ret = of_property_read_u32_index(node, "ti,sysc_type", index, &prop); > + if (ret) > + prop = 1; > + > + switch (prop) { > + case 2: > + sysc->sysc_fields = &omap_hwmod_sysc_type2; > + break; > + case 3: > + sysc->sysc_fields = &omap_hwmod_sysc_type3; > + break; > + case 1: > + default: > + sysc->sysc_fields = &omap_hwmod_sysc_type1; > + } > + class->sysc = sysc; > + oh->class = class; > + > + return 0; > +} > + > /** > * _init - initialize internal data for the hwmod @oh > * @oh: struct omap_hwmod * > @@ -2449,6 +2559,12 @@ static int __init _init(struct omap_hwmod *oh, void *data) > else if (np && index) > pr_warn("omap_hwmod: %s using broken dt data from %s\n", > oh->name, np->name); > + > + if (np) { > + r = omap_hwmod_init_sysc(np, oh, 0); this won't handle any binding which lists more than one hwmod on its ti,hwmods property.
On Wed, Dec 10, 2014 at 08:54:33AM -0600, Felipe Balbi wrote: > Hi, > > (adding linux-omap back to the loop) > > On Wed, Dec 10, 2014 at 04:20:30PM +0530, Lokesh Vutla wrote: > > Hi Felipe, > > > > On Wednesday 10 December 2014 03:57 AM, Felipe Balbi wrote: > > > Before this patch, HWMOD requires the existence > > > of a struct omap_hwmod_class very early. > > Yes, hwmod code looks for omap_hwmod_class entry before registering any hwmod. > > > > With the patch 4/7 omap_hwmod_class gets populated from dt very late after registration of the hwmod. > > So all the hwmod which gets class data from dt never gets registered and state is always UNKNOWN. > > Mostly making it unusable. > > IMO this patch just masks the problem. > > > > In order to register hwmod we need to populate class data very early. > > We can populate at the same place as how reg property is being parsed. > > Call omap_hwmod_init_sysc() in _init() of the particular hwmod: > > The below diff will help: > > > > diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c > > index cbb908d..05ecf8a 100644 > > --- a/arch/arm/mach-omap2/omap_hwmod.c > > +++ b/arch/arm/mach-omap2/omap_hwmod.c > > @@ -2415,6 +2415,116 @@ static int __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data, > > return 0; > > } > > > > +static int omap_hwmod_has_sysc_bindings(struct device_node *node) > > +{ > > + char *properties[] = { > > + "ti,rev_offs", > > + "ti,sysc_offs", > > + "ti,syss_offs", > > + "ti,sysc_flags", > > + "ti,srst_udelay", > > + "ti,idlemodes", > > + "ti,clockact", > > + "ti,sysc_type", > > + NULL > > + }; > > + char **tmp = properties; > > + > > + while (*tmp) { > > + if (of_property_read_bool(node, *tmp)) { > > + return true; > > + } > > + tmp++; > > + } > > + > > + return 0; > > +} > > + > > +static int omap_hwmod_init_sysc(struct device_node *node, > > + struct omap_hwmod *oh, int index) > > +{ > > + struct omap_hwmod_class *class = oh->class; > > + struct omap_hwmod_class_sysconfig *sysc; > > + int ret; > > + int i; > > + char name[128]; > > + const char *tmp = oh->name; > > + u32 prop; > > + > > + /* if data isn't provided by DT, skip */ > > + if ((class && class->sysc) || !omap_hwmod_has_sysc_bindings(node)) > > + return 0; > > + > > + class = kzalloc(sizeof(*class), GFP_KERNEL); > > + if (!class) > > + return -ENOMEM; > > + > > + i = 0; > > + while (*tmp) { > > + if (isalpha(*tmp)) > > + name[i++] = *tmp; > > + tmp++; > > + } > > + name[i] = '\0'; > > + > > + class->name = kzalloc(sizeof(name), GFP_KERNEL); > > + if (!class->name) > > + return -ENOMEM; > > + strncpy(class->name, name, sizeof(name) - 1); > > + > > + sysc = kzalloc(sizeof(*sysc), GFP_KERNEL); > > + if (!sysc) > > + return -ENOMEM; > > + > > + ret = of_property_read_u32_index(node, "ti,rev_offs", index, &prop); > > + if (!ret) > > + sysc->rev_offs = prop; > > + > > + ret = of_property_read_u32_index(node, "ti,sysc_offs", index, &prop); > > + if (!ret) > > + sysc->sysc_offs = prop; > > + > > + ret = of_property_read_u32_index(node, "ti,syss_offs", index, &prop); > > + if (!ret) > > + sysc->syss_offs = prop; > > + > > + ret = of_property_read_u32_index(node, "ti,sysc_flags", index, &prop); > > + if (!ret) > > + sysc->sysc_flags = prop & 0xffff; > > + > > + ret = of_property_read_u32_index(node, "ti,srst_udelay", index, &prop); > > + if (!ret) > > + sysc->srst_udelay = prop & 0xff; > > + > > + ret = of_property_read_u32_index(node, "ti,idlemodes", index, &prop); > > + if (!ret) > > + sysc->idlemodes = prop & 0xff; > > + > > + ret = of_property_read_u32_index(node, "ti,clockact", index, &prop); > > + if (!ret) > > + sysc->clockact = prop & 0xff; > > + > > + ret = of_property_read_u32_index(node, "ti,sysc_type", index, &prop); > > + if (ret) > > + prop = 1; > > + > > + switch (prop) { > > + case 2: > > + sysc->sysc_fields = &omap_hwmod_sysc_type2; > > + break; > > + case 3: > > + sysc->sysc_fields = &omap_hwmod_sysc_type3; > > + break; > > + case 1: > > + default: > > + sysc->sysc_fields = &omap_hwmod_sysc_type1; > > + } > > + class->sysc = sysc; > > + oh->class = class; > > + > > + return 0; > > +} > > + > > /** > > * _init - initialize internal data for the hwmod @oh > > * @oh: struct omap_hwmod * > > @@ -2449,6 +2559,12 @@ static int __init _init(struct omap_hwmod *oh, void *data) > > else if (np && index) > > pr_warn("omap_hwmod: %s using broken dt data from %s\n", > > oh->name, np->name); > > + > > + if (np) { > > + r = omap_hwmod_init_sysc(np, oh, 0); > > this won't handle any binding which lists more than one hwmod on its > ti,hwmods property. I think Tony planned to remove this kind of multi hwmod references. IMHO instead of DT referencing the hwmod stuff using ti,hwmods the hwmod database should reference the compatible values. This depends on omap3 being DT only of course. -- Sebastian
Hi, On Thu, Dec 11, 2014 at 01:52:38AM +0100, Sebastian Reichel wrote: > On Wed, Dec 10, 2014 at 08:54:33AM -0600, Felipe Balbi wrote: > > Hi, > > > > (adding linux-omap back to the loop) > > > > On Wed, Dec 10, 2014 at 04:20:30PM +0530, Lokesh Vutla wrote: > > > Hi Felipe, > > > > > > On Wednesday 10 December 2014 03:57 AM, Felipe Balbi wrote: > > > > Before this patch, HWMOD requires the existence > > > > of a struct omap_hwmod_class very early. > > > Yes, hwmod code looks for omap_hwmod_class entry before registering any hwmod. > > > > > > With the patch 4/7 omap_hwmod_class gets populated from dt very late after registration of the hwmod. > > > So all the hwmod which gets class data from dt never gets registered and state is always UNKNOWN. > > > Mostly making it unusable. > > > IMO this patch just masks the problem. > > > > > > In order to register hwmod we need to populate class data very early. > > > We can populate at the same place as how reg property is being parsed. > > > Call omap_hwmod_init_sysc() in _init() of the particular hwmod: > > > The below diff will help: > > > > > > diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c > > > index cbb908d..05ecf8a 100644 > > > --- a/arch/arm/mach-omap2/omap_hwmod.c > > > +++ b/arch/arm/mach-omap2/omap_hwmod.c > > > @@ -2415,6 +2415,116 @@ static int __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data, > > > return 0; > > > } > > > > > > +static int omap_hwmod_has_sysc_bindings(struct device_node *node) > > > +{ > > > + char *properties[] = { > > > + "ti,rev_offs", > > > + "ti,sysc_offs", > > > + "ti,syss_offs", > > > + "ti,sysc_flags", > > > + "ti,srst_udelay", > > > + "ti,idlemodes", > > > + "ti,clockact", > > > + "ti,sysc_type", > > > + NULL > > > + }; > > > + char **tmp = properties; > > > + > > > + while (*tmp) { > > > + if (of_property_read_bool(node, *tmp)) { > > > + return true; > > > + } > > > + tmp++; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static int omap_hwmod_init_sysc(struct device_node *node, > > > + struct omap_hwmod *oh, int index) > > > +{ > > > + struct omap_hwmod_class *class = oh->class; > > > + struct omap_hwmod_class_sysconfig *sysc; > > > + int ret; > > > + int i; > > > + char name[128]; > > > + const char *tmp = oh->name; > > > + u32 prop; > > > + > > > + /* if data isn't provided by DT, skip */ > > > + if ((class && class->sysc) || !omap_hwmod_has_sysc_bindings(node)) > > > + return 0; > > > + > > > + class = kzalloc(sizeof(*class), GFP_KERNEL); > > > + if (!class) > > > + return -ENOMEM; > > > + > > > + i = 0; > > > + while (*tmp) { > > > + if (isalpha(*tmp)) > > > + name[i++] = *tmp; > > > + tmp++; > > > + } > > > + name[i] = '\0'; > > > + > > > + class->name = kzalloc(sizeof(name), GFP_KERNEL); > > > + if (!class->name) > > > + return -ENOMEM; > > > + strncpy(class->name, name, sizeof(name) - 1); > > > + > > > + sysc = kzalloc(sizeof(*sysc), GFP_KERNEL); > > > + if (!sysc) > > > + return -ENOMEM; > > > + > > > + ret = of_property_read_u32_index(node, "ti,rev_offs", index, &prop); > > > + if (!ret) > > > + sysc->rev_offs = prop; > > > + > > > + ret = of_property_read_u32_index(node, "ti,sysc_offs", index, &prop); > > > + if (!ret) > > > + sysc->sysc_offs = prop; > > > + > > > + ret = of_property_read_u32_index(node, "ti,syss_offs", index, &prop); > > > + if (!ret) > > > + sysc->syss_offs = prop; > > > + > > > + ret = of_property_read_u32_index(node, "ti,sysc_flags", index, &prop); > > > + if (!ret) > > > + sysc->sysc_flags = prop & 0xffff; > > > + > > > + ret = of_property_read_u32_index(node, "ti,srst_udelay", index, &prop); > > > + if (!ret) > > > + sysc->srst_udelay = prop & 0xff; > > > + > > > + ret = of_property_read_u32_index(node, "ti,idlemodes", index, &prop); > > > + if (!ret) > > > + sysc->idlemodes = prop & 0xff; > > > + > > > + ret = of_property_read_u32_index(node, "ti,clockact", index, &prop); > > > + if (!ret) > > > + sysc->clockact = prop & 0xff; > > > + > > > + ret = of_property_read_u32_index(node, "ti,sysc_type", index, &prop); > > > + if (ret) > > > + prop = 1; > > > + > > > + switch (prop) { > > > + case 2: > > > + sysc->sysc_fields = &omap_hwmod_sysc_type2; > > > + break; > > > + case 3: > > > + sysc->sysc_fields = &omap_hwmod_sysc_type3; > > > + break; > > > + case 1: > > > + default: > > > + sysc->sysc_fields = &omap_hwmod_sysc_type1; > > > + } > > > + class->sysc = sysc; > > > + oh->class = class; > > > + > > > + return 0; > > > +} > > > + > > > /** > > > * _init - initialize internal data for the hwmod @oh > > > * @oh: struct omap_hwmod * > > > @@ -2449,6 +2559,12 @@ static int __init _init(struct omap_hwmod *oh, void *data) > > > else if (np && index) > > > pr_warn("omap_hwmod: %s using broken dt data from %s\n", > > > oh->name, np->name); > > > + > > > + if (np) { > > > + r = omap_hwmod_init_sysc(np, oh, 0); > > > > this won't handle any binding which lists more than one hwmod on its > > ti,hwmods property. > > I think Tony planned to remove this kind of multi hwmod references. true, but I'm still a bit iffy wrt that. ABI compatibility breaks and all > IMHO instead of DT referencing the hwmod stuff using ti,hwmods the > hwmod database should reference the compatible values. This depends > on omap3 being DT only of course. don't you think it's too late for that ? We need to support the current form of dts files forever. It's an ABI afterall.
* Sebastian Reichel <sre@kernel.org> [141210 16:55]: > On Wed, Dec 10, 2014 at 08:54:33AM -0600, Felipe Balbi wrote: > > > @@ -2449,6 +2559,12 @@ static int __init _init(struct omap_hwmod *oh, void *data) > > > else if (np && index) > > > pr_warn("omap_hwmod: %s using broken dt data from %s\n", > > > oh->name, np->name); > > > + > > > + if (np) { > > > + r = omap_hwmod_init_sysc(np, oh, 0); > > > > this won't handle any binding which lists more than one hwmod on its > > ti,hwmods property. > > I think Tony planned to remove this kind of multi hwmod references. Yes we really should do that. But that involves repairs in the drivers, dts files, and hwmod. We really should aim for 1 to 1 to 1 mapping of the dts compatible property to device hwmod to device sysc registers. Once all the devices are mapped that way, we can deprecate ti,hwmods in the dts files and set it up automatically based on the compatible and the instance number of the device. For an example of how to fix the interlaced device dts entries, let's look at the EDMA. Instead of the current setup in am33xx.dtsi: edma: edma@49000000 { compatible = "ti,edma3"; ti,hwmods = "tpcc", "tptc0", "tptc1", "tptc2"; reg = <0x49000000 0x10000>, <0x44e10f90 0x40>; interrupts = <12 13 14>; #dma-cells = <1>; }; It really should be set up in the following way (eventually without ti,hwmods): edma: edma@49000000 { compatible = "ti,edma3"; reg = <0x49000000 0x10000>, <0x44e10f90 0x40>; interrupts = <12>; #dma-cells = <1>; }; tptc0 { compatible = "ti,edma3-tptc"; reg = <0x49800000 0x100000>; interrupts = <13>; }; tptc1 { compatible = "ti,edma3-tptc"; reg = <0x49900000 0x100000>; interrupts = <14>; }; tptc2 { compatible = "ti,edma3-tptc"; reg = <0x49900000 0x100000>; interrupts = <15>; }; This is because each tptc entry has it's own sysc register and can idle on it's own. With the above we can have 1 to 1 to 1 mapping of compatible property to hwmod to sysc registers for each device. > IMHO instead of DT referencing the hwmod stuff using ti,hwmods the > hwmod database should reference the compatible values. This depends > on omap3 being DT only of course. Yes agreed. Regards, Tony
On Thu, Dec 11, 2014 at 08:23:33AM -0600, Felipe Balbi wrote: > Hi, > > On Thu, Dec 11, 2014 at 01:52:38AM +0100, Sebastian Reichel wrote: > > On Wed, Dec 10, 2014 at 08:54:33AM -0600, Felipe Balbi wrote: > > > Hi, > > > > > > (adding linux-omap back to the loop) > > > > > > On Wed, Dec 10, 2014 at 04:20:30PM +0530, Lokesh Vutla wrote: > > > > Hi Felipe, > > > > > > > > On Wednesday 10 December 2014 03:57 AM, Felipe Balbi wrote: > > > > > Before this patch, HWMOD requires the existence > > > > > of a struct omap_hwmod_class very early. > > > > Yes, hwmod code looks for omap_hwmod_class entry before registering any hwmod. > > > > > > > > With the patch 4/7 omap_hwmod_class gets populated from dt very late after registration of the hwmod. > > > > So all the hwmod which gets class data from dt never gets registered and state is always UNKNOWN. > > > > Mostly making it unusable. > > > > IMO this patch just masks the problem. > > > > > > > > In order to register hwmod we need to populate class data very early. > > > > We can populate at the same place as how reg property is being parsed. > > > > Call omap_hwmod_init_sysc() in _init() of the particular hwmod: > > > > The below diff will help: > > > > > > > > diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c > > > > index cbb908d..05ecf8a 100644 > > > > --- a/arch/arm/mach-omap2/omap_hwmod.c > > > > +++ b/arch/arm/mach-omap2/omap_hwmod.c > > > > @@ -2415,6 +2415,116 @@ static int __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data, > > > > return 0; > > > > } > > > > > > > > +static int omap_hwmod_has_sysc_bindings(struct device_node *node) > > > > +{ > > > > + char *properties[] = { > > > > + "ti,rev_offs", > > > > + "ti,sysc_offs", > > > > + "ti,syss_offs", > > > > + "ti,sysc_flags", > > > > + "ti,srst_udelay", > > > > + "ti,idlemodes", > > > > + "ti,clockact", > > > > + "ti,sysc_type", > > > > + NULL > > > > + }; > > > > + char **tmp = properties; > > > > + > > > > + while (*tmp) { > > > > + if (of_property_read_bool(node, *tmp)) { > > > > + return true; > > > > + } > > > > + tmp++; > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int omap_hwmod_init_sysc(struct device_node *node, > > > > + struct omap_hwmod *oh, int index) > > > > +{ > > > > + struct omap_hwmod_class *class = oh->class; > > > > + struct omap_hwmod_class_sysconfig *sysc; > > > > + int ret; > > > > + int i; > > > > + char name[128]; > > > > + const char *tmp = oh->name; > > > > + u32 prop; > > > > + > > > > + /* if data isn't provided by DT, skip */ > > > > + if ((class && class->sysc) || !omap_hwmod_has_sysc_bindings(node)) > > > > + return 0; > > > > + > > > > + class = kzalloc(sizeof(*class), GFP_KERNEL); > > > > + if (!class) > > > > + return -ENOMEM; > > > > + > > > > + i = 0; > > > > + while (*tmp) { > > > > + if (isalpha(*tmp)) > > > > + name[i++] = *tmp; > > > > + tmp++; > > > > + } > > > > + name[i] = '\0'; > > > > + > > > > + class->name = kzalloc(sizeof(name), GFP_KERNEL); > > > > + if (!class->name) > > > > + return -ENOMEM; > > > > + strncpy(class->name, name, sizeof(name) - 1); > > > > + > > > > + sysc = kzalloc(sizeof(*sysc), GFP_KERNEL); > > > > + if (!sysc) > > > > + return -ENOMEM; > > > > + > > > > + ret = of_property_read_u32_index(node, "ti,rev_offs", index, &prop); > > > > + if (!ret) > > > > + sysc->rev_offs = prop; > > > > + > > > > + ret = of_property_read_u32_index(node, "ti,sysc_offs", index, &prop); > > > > + if (!ret) > > > > + sysc->sysc_offs = prop; > > > > + > > > > + ret = of_property_read_u32_index(node, "ti,syss_offs", index, &prop); > > > > + if (!ret) > > > > + sysc->syss_offs = prop; > > > > + > > > > + ret = of_property_read_u32_index(node, "ti,sysc_flags", index, &prop); > > > > + if (!ret) > > > > + sysc->sysc_flags = prop & 0xffff; > > > > + > > > > + ret = of_property_read_u32_index(node, "ti,srst_udelay", index, &prop); > > > > + if (!ret) > > > > + sysc->srst_udelay = prop & 0xff; > > > > + > > > > + ret = of_property_read_u32_index(node, "ti,idlemodes", index, &prop); > > > > + if (!ret) > > > > + sysc->idlemodes = prop & 0xff; > > > > + > > > > + ret = of_property_read_u32_index(node, "ti,clockact", index, &prop); > > > > + if (!ret) > > > > + sysc->clockact = prop & 0xff; > > > > + > > > > + ret = of_property_read_u32_index(node, "ti,sysc_type", index, &prop); > > > > + if (ret) > > > > + prop = 1; > > > > + > > > > + switch (prop) { > > > > + case 2: > > > > + sysc->sysc_fields = &omap_hwmod_sysc_type2; > > > > + break; > > > > + case 3: > > > > + sysc->sysc_fields = &omap_hwmod_sysc_type3; > > > > + break; > > > > + case 1: > > > > + default: > > > > + sysc->sysc_fields = &omap_hwmod_sysc_type1; > > > > + } > > > > + class->sysc = sysc; > > > > + oh->class = class; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > /** > > > > * _init - initialize internal data for the hwmod @oh > > > > * @oh: struct omap_hwmod * > > > > @@ -2449,6 +2559,12 @@ static int __init _init(struct omap_hwmod *oh, void *data) > > > > else if (np && index) > > > > pr_warn("omap_hwmod: %s using broken dt data from %s\n", > > > > oh->name, np->name); > > > > + > > > > + if (np) { > > > > + r = omap_hwmod_init_sysc(np, oh, 0); > > > > > > this won't handle any binding which lists more than one hwmod on its > > > ti,hwmods property. > > > > I think Tony planned to remove this kind of multi hwmod references. > > true, but I'm still a bit iffy wrt that. ABI compatibility breaks and > all Moving the hwmod data to device tree will break the ABI compatibility anyways. You wont be able to use an old DT with the new kernel, since then you are missing the hwmod data (assuming there won't be a fallback hwmod data kept in the kernel source). > > IMHO instead of DT referencing the hwmod stuff using ti,hwmods the > > hwmod database should reference the compatible values. This depends > > on omap3 being DT only of course. > > don't you think it's too late for that ? We need to support the current > form of dts files forever. It's an ABI afterall. As I described above the ABI *will* break if hwmod data is removed from the kernel. OTOH where is the ABI breakage when hwmod database in kernel is built from the compatible value? The compatible values are already part of the ABI, so there are no new properties needed. The ti,hwmod property can be marked as deprecated (and maybe removed after some years). -- Sebastian
* Sebastian Reichel <sre@kernel.org> [141211 09:46]: > On Thu, Dec 11, 2014 at 08:23:33AM -0600, Felipe Balbi wrote: > > true, but I'm still a bit iffy wrt that. ABI compatibility breaks and > > all > > Moving the hwmod data to device tree will break the ABI > compatibility anyways. You wont be able to use an old DT with the > new kernel, since then you are missing the hwmod data (assuming > there won't be a fallback hwmod data kept in the kernel source). Right, the way to deal with that is to do the following: 1. Once we have the binding in place, start printing out warnings and deprecate the old built in data. 2. For missing sysc data, we just keep printing warnings and won't idle or enable the devices. This keeps the optional boot console UART working based on the bootloader settings. 3. We may want to keep the UART and system timer data around forever to be able to print sane error messages. > > > IMHO instead of DT referencing the hwmod stuff using ti,hwmods the > > > hwmod database should reference the compatible values. This depends > > > on omap3 being DT only of course. > > > > don't you think it's too late for that ? We need to support the current > > form of dts files forever. It's an ABI afterall. > > As I described above the ABI *will* break if hwmod data is removed from > the kernel. Right, there is no way we can support incomplete device tree bindings except for devices that are enabled by the bootloader already. But we can take our time removing the built-in device data, there's no need to do it all at once. > OTOH where is the ABI breakage when hwmod database in kernel is > built from the compatible value? The compatible values are already > part of the ABI, so there are no new properties needed. The ti,hwmod > property can be marked as deprecated (and maybe removed after some > years). Yes we can keep it around but don't have to do anything with it eventually. For the legacy systems, we can have built in mapping of compatible values to ti,hwmods along the clock aliases. Regards, Tony
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c index cbb908d..05ecf8a 100644 --- a/arch/arm/mach-omap2/omap_hwmod.c +++ b/arch/arm/mach-omap2/omap_hwmod.c @@ -2415,6 +2415,116 @@ static int __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data, return 0; } +static int omap_hwmod_has_sysc_bindings(struct device_node *node) +{ + char *properties[] = { + "ti,rev_offs", + "ti,sysc_offs", + "ti,syss_offs", + "ti,sysc_flags", + "ti,srst_udelay", + "ti,idlemodes", + "ti,clockact", + "ti,sysc_type", + NULL + }; + char **tmp = properties; + + while (*tmp) { + if (of_property_read_bool(node, *tmp)) { + return true; + } + tmp++; + } + + return 0; +} + +static int omap_hwmod_init_sysc(struct device_node *node, + struct omap_hwmod *oh, int index) +{ + struct omap_hwmod_class *class = oh->class; + struct omap_hwmod_class_sysconfig *sysc; + int ret; + int i; + char name[128]; + const char *tmp = oh->name; + u32 prop; + + /* if data isn't provided by DT, skip */ + if ((class && class->sysc) || !omap_hwmod_has_sysc_bindings(node)) + return 0; + + class = kzalloc(sizeof(*class), GFP_KERNEL); + if (!class) + return -ENOMEM; + + i = 0; + while (*tmp) { + if (isalpha(*tmp)) + name[i++] = *tmp; + tmp++; + } + name[i] = '\0'; + + class->name = kzalloc(sizeof(name), GFP_KERNEL); + if (!class->name) + return -ENOMEM; + strncpy(class->name, name, sizeof(name) - 1); + + sysc = kzalloc(sizeof(*sysc), GFP_KERNEL); + if (!sysc) + return -ENOMEM; + + ret = of_property_read_u32_index(node, "ti,rev_offs", index, &prop); + if (!ret) + sysc->rev_offs = prop; + + ret = of_property_read_u32_index(node, "ti,sysc_offs", index, &prop); + if (!ret) + sysc->sysc_offs = prop; + + ret = of_property_read_u32_index(node, "ti,syss_offs", index, &prop); + if (!ret) + sysc->syss_offs = prop; + + ret = of_property_read_u32_index(node, "ti,sysc_flags", index, &prop); + if (!ret) + sysc->sysc_flags = prop & 0xffff; + + ret = of_property_read_u32_index(node, "ti,srst_udelay", index, &prop); + if (!ret) + sysc->srst_udelay = prop & 0xff; + + ret = of_property_read_u32_index(node, "ti,idlemodes", index, &prop); + if (!ret) + sysc->idlemodes = prop & 0xff; + + ret = of_property_read_u32_index(node, "ti,clockact", index, &prop); + if (!ret) + sysc->clockact = prop & 0xff; + + ret = of_property_read_u32_index(node, "ti,sysc_type", index, &prop); + if (ret) + prop = 1; + + switch (prop) { + case 2: + sysc->sysc_fields = &omap_hwmod_sysc_type2; + break; + case 3: + sysc->sysc_fields = &omap_hwmod_sysc_type3; + break; + case 1: + default: + sysc->sysc_fields = &omap_hwmod_sysc_type1; + } + class->sysc = sysc; + oh->class = class; + + return 0; +} + /** * _init - initialize internal data for the hwmod @oh * @oh: struct omap_hwmod * @@ -2449,6 +2559,12 @@ static int __init _init(struct omap_hwmod *oh, void *data) else if (np && index) pr_warn("omap_hwmod: %s using broken dt data from %s\n", oh->name, np->name); + + if (np) { + r = omap_hwmod_init_sysc(np, oh, 0); + if (r) + pr_warn("omap_hwmod: %s missing sysc dt data\n", oh->name); + } } if (oh->class->sysc) { @@ -2673,8 +2789,7 @@ static int __init _setup(struct omap_hwmod *oh, void *data) * already has been registered by the same name; -EINVAL if the * omap_hwmod is in the wrong state, if @oh is NULL, if the * omap_hwmod's class field is NULL; if the omap_hwmod is missing a - * name, or if the omap_hwmod's class is missing a name; or 0 upon - * success. + * name, or 0 upon success. * * XXX The data should be copied into bootmem, so the original data * should be marked __initdata and freed after init. This would allow @@ -2684,8 +2799,7 @@ static int __init _setup(struct omap_hwmod *oh, void *data) */ static int __init _register(struct omap_hwmod *oh) { - if (!oh || !oh->name || !oh->class || !oh->class->name || - (oh->_state != _HWMOD_STATE_UNKNOWN)) + if (!oh || !oh->name || oh->_state != _HWMOD_STATE_UNKNOWN) return -EINVAL; pr_debug("omap_hwmod: %s: registering\n", oh->name);