| Message ID | 1396577891-2713-3-git-send-email-elder@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 04/03/2014 09:18 PM, Alex Elder wrote: > This patch adds SMP support for BCM281XX and BCM21664 family SoCs. > > This feature is controlled with a distinct config option such that a > SMP-enabled multi-v7 binary can be configured to run these SoCs in > uniprocessor mode. Since this SMP functionality is used for > multiple Broadcom mobile chip families the config option is called > ARCH_BCM_MOBILE_SMP (for lack of a better name). > > On SoCs of this type, the secondary core is not held in reset on > power-on. Instead it loops in a ROM-based holding pen. To release > it, one must write into a special register a jump address whose > low-order bits have been replaced with a secondary core's id, then > trigger an event with SEV. On receipt of an event, the ROM code > will examine the register's contents, and if the low-order bits > match its cpu id, it will clear them and write the value back to the > register just prior to jumping to the address specified. > > The location of the special register is defined in the device tree > using a "secondary-boot-reg" property in a node whose "enable-method" > matches. > > Derived from code originally provided by Ray Jui <rjui@broadcom.com> > > Signed-off-by: Alex Elder <elder@linaro.org> > --- . . . > diff --git a/arch/arm/mach-bcm/Makefile b/arch/arm/mach-bcm/Makefile > index b2279e3..929579f 100644 > --- a/arch/arm/mach-bcm/Makefile > +++ b/arch/arm/mach-bcm/Makefile > @@ -15,7 +15,10 @@ obj-$(CONFIG_ARCH_BCM_281XX) += board_bcm281xx.o > plus_sec := $(call as-instr,.arch_extension sec,+sec) > > # BCM21664 > -obj-$(CONFIG_ARCH_BCM_21664) += board_bcm21664.o > +obj-$(CONFIG_ARCH_BCM_21664) := board_bcm21664.o The above was a mistake, it should still be +=. (I'll fix it.) . . . -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 3, 2014 at 7:18 PM, Alex Elder <elder@linaro.org> wrote: > diff --git a/arch/arm/mach-bcm/platsmp.c b/arch/arm/mach-bcm/platsmp.c > new file mode 100644 > index 0000000..46a64f2 > --- /dev/null > +++ b/arch/arm/mach-bcm/platsmp.c > +/* Size of mapped Cortex A9 SCU address space */ > +#define SCU_SIZE 0x58 > +/* > + * Enable the Cortex A9 Snoop Control Unit > + * > + * By the time this is called we already know there are multiple > + * cores present. We assume we're running on a Cortex A9 processor, > + * so any trouble getting the base address register or getting the > + * SCU base is a problem. > + * > + * Return 0 if successful or an error code otherwise. > + */ > +static int __init scu_a9_enable(void) > +{ > + unsigned long config_base; > + void __iomem *scu_base; > + > + if (!scu_a9_has_base()) { > + pr_err("no configuration base address register!\n"); > + return -ENXIO; > + } > + > + /* Config base address register value is zero for uniprocessor */ > + config_base = scu_a9_get_base(); > + if (!config_base) { > + pr_err("hardware reports only one core; disabling SMP\n"); > + return -ENOENT; > + } > + > + scu_base = ioremap((phys_addr_t)config_base, SCU_SIZE); > + if (!scu_base) { > + pr_err("failed to remap config base (%lu/%u) for SCU\n", > + config_base, SCU_SIZE); > + return -ENOMEM; > + } > + > + scu_enable(scu_base); > + > + iounmap(scu_base); /* That's the last we'll need of this */ > + > + return 0; > +} This function seems useful for Cortex A9 MPCore in general. While you gave it a generic name, you put it in a Broadcom file. Is there a better location for this code? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/03/14 19:18, Alex Elder wrote: > + > +/* > + * Secondary startup method setup routine to extract the location of > + * the secondary boot register from a "cpu" or "cpus" device tree > + * node. Only the first seen secondary boot register value is used; > + * any others are ignored. The secondary boot register value must be > + * non-zero. > + * > + * Returns 0 if successful or an error code otherwise. > + */ > +static int __init of_enable_method_setup(struct device_node *node) > +{ > + int ret; > + > + /* Ignore all but the first one specified */ > + if (secondary_boot) > + return 0; > + > + ret = of_property_read_u32(node, OF_SECONDARY_BOOT, &secondary_boot); > + if (ret) > + pr_err("%s: missing/invalid " OF_SECONDARY_BOOT " property\n", > + node->name); > + > + return ret; > +} I don't understand why we need this. Why can't we get the secondary boot address from the /cpus node in the smp_prepare_cpus op. It isn't that hard to get access to the cpus node there via of_find_node_by_path(). Then we don't need patch 1 at all. If it turns out to be common stuff, we can always have the common function live in arm common code or maybe even be a devicetree API.
On 04/04/2014 10:30 AM, Tim Kryger wrote: > On Thu, Apr 3, 2014 at 7:18 PM, Alex Elder <elder@linaro.org> wrote: > >> diff --git a/arch/arm/mach-bcm/platsmp.c b/arch/arm/mach-bcm/platsmp.c >> new file mode 100644 >> index 0000000..46a64f2 >> --- /dev/null >> +++ b/arch/arm/mach-bcm/platsmp.c > >> +/* Size of mapped Cortex A9 SCU address space */ >> +#define SCU_SIZE 0x58 > >> +/* >> + * Enable the Cortex A9 Snoop Control Unit >> + * >> + * By the time this is called we already know there are multiple >> + * cores present. We assume we're running on a Cortex A9 processor, >> + * so any trouble getting the base address register or getting the >> + * SCU base is a problem. >> + * >> + * Return 0 if successful or an error code otherwise. >> + */ >> +static int __init scu_a9_enable(void) >> +{ >> + unsigned long config_base; >> + void __iomem *scu_base; >> + >> + if (!scu_a9_has_base()) { >> + pr_err("no configuration base address register!\n"); >> + return -ENXIO; >> + } >> + >> + /* Config base address register value is zero for uniprocessor */ >> + config_base = scu_a9_get_base(); >> + if (!config_base) { >> + pr_err("hardware reports only one core; disabling SMP\n"); >> + return -ENOENT; >> + } >> + >> + scu_base = ioremap((phys_addr_t)config_base, SCU_SIZE); >> + if (!scu_base) { >> + pr_err("failed to remap config base (%lu/%u) for SCU\n", >> + config_base, SCU_SIZE); >> + return -ENOMEM; >> + } >> + >> + scu_enable(scu_base); >> + >> + iounmap(scu_base); /* That's the last we'll need of this */ >> + >> + return 0; >> +} > > This function seems useful for Cortex A9 MPCore in general. > > While you gave it a generic name, you put it in a Broadcom file. > > Is there a better location for this code? I think it belongs in arch/arm/kernel/smp_scu.c. I was thinking it might be generally useful when I wrote it (hence the more complete header comment, for example). And I'll gladly move it there, I just didn't want anybody to get hung up on that. -Alex -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/04/2014 12:55 PM, Stephen Boyd wrote: > On 04/03/14 19:18, Alex Elder wrote: >> + >> +/* >> + * Secondary startup method setup routine to extract the location of >> + * the secondary boot register from a "cpu" or "cpus" device tree >> + * node. Only the first seen secondary boot register value is used; >> + * any others are ignored. The secondary boot register value must be >> + * non-zero. >> + * >> + * Returns 0 if successful or an error code otherwise. >> + */ >> +static int __init of_enable_method_setup(struct device_node *node) >> +{ >> + int ret; >> + >> + /* Ignore all but the first one specified */ >> + if (secondary_boot) >> + return 0; >> + >> + ret = of_property_read_u32(node, OF_SECONDARY_BOOT, &secondary_boot); >> + if (ret) >> + pr_err("%s: missing/invalid " OF_SECONDARY_BOOT " property\n", >> + node->name); >> + >> + return ret; >> +} > > I don't understand why we need this. Why can't we get the secondary boot > address from the /cpus node in the smp_prepare_cpus op. It isn't that > hard to get access to the cpus node there via of_find_node_by_path(). > Then we don't need patch 1 at all. If it turns out to be common stuff, > we can always have the common function live in arm common code or maybe > even be a devicetree API. You're right, that is one of several ways this could have also been done. Part of my thinking about this was affected by seeing how arm_dt_init_cpu_maps() works. (Well, at least how it's structured.) It looks for an enable method in each "cpu" node, then falls back to looking for one in the "cpus" node if none was seen. It doesn't assume the extra properties are in the "cpus" node (or a "cpu" node for that matter). It directly supplies a node that's known to have a matching "enable-method" property--at the time that match is found--to make it possible to extract other relevant information from that matching node. (As an aside, the documentation doesn't mention that "cpus" nodes will contain "enable-method" properties, even though it seems that's more a function of "cpus" than each individual "cpu".) I think it offers useful flexibility to do it this way, and I like that it is unambiguous which node should be searched for the additional information. -Alex -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/04/2014 01:56 PM, Alex Elder wrote: > On 04/04/2014 10:30 AM, Tim Kryger wrote: >> On Thu, Apr 3, 2014 at 7:18 PM, Alex Elder <elder@linaro.org> wrote: >> >>> diff --git a/arch/arm/mach-bcm/platsmp.c b/arch/arm/mach-bcm/platsmp.c >>> new file mode 100644 >>> index 0000000..46a64f2 >>> --- /dev/null >>> +++ b/arch/arm/mach-bcm/platsmp.c >> >>> +/* Size of mapped Cortex A9 SCU address space */ >>> +#define SCU_SIZE 0x58 >> >>> +/* >>> + * Enable the Cortex A9 Snoop Control Unit >>> + * >>> + * By the time this is called we already know there are multiple >>> + * cores present. We assume we're running on a Cortex A9 processor, >>> + * so any trouble getting the base address register or getting the >>> + * SCU base is a problem. >>> + * >>> + * Return 0 if successful or an error code otherwise. >>> + */ >>> +static int __init scu_a9_enable(void) >>> +{ >>> + unsigned long config_base; >>> + void __iomem *scu_base; >>> + >>> + if (!scu_a9_has_base()) { >>> + pr_err("no configuration base address register!\n"); >>> + return -ENXIO; >>> + } >>> + >>> + /* Config base address register value is zero for uniprocessor */ >>> + config_base = scu_a9_get_base(); >>> + if (!config_base) { >>> + pr_err("hardware reports only one core; disabling SMP\n"); >>> + return -ENOENT; >>> + } >>> + >>> + scu_base = ioremap((phys_addr_t)config_base, SCU_SIZE); >>> + if (!scu_base) { >>> + pr_err("failed to remap config base (%lu/%u) for SCU\n", >>> + config_base, SCU_SIZE); >>> + return -ENOMEM; >>> + } >>> + >>> + scu_enable(scu_base); >>> + >>> + iounmap(scu_base); /* That's the last we'll need of this */ >>> + >>> + return 0; >>> +} >> >> This function seems useful for Cortex A9 MPCore in general. >> >> While you gave it a generic name, you put it in a Broadcom file. >> >> Is there a better location for this code? > > I think it belongs in arch/arm/kernel/smp_scu.c. I was thinking > it might be generally useful when I wrote it (hence the more > complete header comment, for example). And I'll gladly move > it there, I just didn't want anybody to get hung up on that. I'm going to re-submit this series this morning. I looked at where this function might be used, and it looks like only one other platform could use it (in hi3xxx_smp_prepare_cpus()). Man of the others define get their scu base address some other way (using a fixed constant or using device tree). For now I'm going to keep it where it is. If you or someone else reiterate the suggestion I'll move it to arch/arm/kernel/smp_scu.c. -Alex -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/04/2014 12:55 PM, Stephen Boyd wrote: > On 04/03/14 19:18, Alex Elder wrote: >> + >> +/* >> + * Secondary startup method setup routine to extract the location of >> + * the secondary boot register from a "cpu" or "cpus" device tree >> + * node. Only the first seen secondary boot register value is used; >> + * any others are ignored. The secondary boot register value must be >> + * non-zero. >> + * >> + * Returns 0 if successful or an error code otherwise. >> + */ >> +static int __init of_enable_method_setup(struct device_node *node) >> +{ >> + int ret; >> + >> + /* Ignore all but the first one specified */ >> + if (secondary_boot) >> + return 0; >> + >> + ret = of_property_read_u32(node, OF_SECONDARY_BOOT, &secondary_boot); >> + if (ret) >> + pr_err("%s: missing/invalid " OF_SECONDARY_BOOT " property\n", >> + node->name); >> + >> + return ret; >> +} > > I don't understand why we need this. Why can't we get the secondary boot > address from the /cpus node in the smp_prepare_cpus op. It isn't that > hard to get access to the cpus node there via of_find_node_by_path(). > Then we don't need patch 1 at all. If it turns out to be common stuff, > we can always have the common function live in arm common code or maybe > even be a devicetree API. I already responded to this, but never got any response. I was preparing to re-send this series and wanted to try to pull the added feature (patch 1) out and not be dependent on it. But I think it's a bit ugly so I'm hoping to get a blessing to proceed with what I originally proposed. For reference, here's the thread: https://lkml.org/lkml/2014/4/3/421 What I'm trying to do is get the value of a "secondary-boot-reg" property from a node known to have an "enable-method" property that matches the method name supplied in CPU_METHOD_OF_DECLARE(). Using the callback function as I originally proposed, this is very easy. When arm_dt_init_cpu_maps() parses the "cpus" portion of the device tree it calls set_smp_ops_by_method() for a matching "cpu" or "cpus" node, and that function supplies the node to the callback function. The callback can extract additional property values if needed. If I hold off until smp_prepare_cpus() is called, I have to re-parse the device tree to find the "cpus" node (this is in itself trivial). I then need to re-parse that node to verify the matching "enable-method" property is found before looking for the parameter information I need for that enable method. I would really prefer not to re-do this parsing step. It's imprecise and a little inefficient, and it duplicates (but not exactly) logic that's already performed by arm_dt_init_cpu_maps(). One more point of clarification. This "secondary-boot-reg" value is *not* the secondary boot address--that is, it's not the address secondary cores jump to when they are activated. Instead, this is the address of a register that's used to request the ROM code release a core from its ROM-implemented holding pen. For this machine, control jumps at that point to secondary_startup(), defined in arch/arm/kernel/head.S. So... Stephen, I'd like to hear from you whether my explanation is adequate, and whether you think my addition and use of CPU_METHOD_OF_DECLARE_SETUP() is reasonable. (If you have a suggestion for a better name, I'm open.) If you still don't like it, I'll follow up with a new version of the patches, this time parsing the device tree in the smp_prepare_cpus() method as you suggested. I don't want this to hold up getting this SMP support into the kernel. Thanks. -Alex -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 05/05/14 15:02, Alex Elder wrote: > On 04/04/2014 12:55 PM, Stephen Boyd wrote: >> On 04/03/14 19:18, Alex Elder wrote: >>> + >>> +/* >>> + * Secondary startup method setup routine to extract the location of >>> + * the secondary boot register from a "cpu" or "cpus" device tree >>> + * node. Only the first seen secondary boot register value is used; >>> + * any others are ignored. The secondary boot register value must be >>> + * non-zero. >>> + * >>> + * Returns 0 if successful or an error code otherwise. >>> + */ >>> +static int __init of_enable_method_setup(struct device_node *node) >>> +{ >>> + int ret; >>> + >>> + /* Ignore all but the first one specified */ >>> + if (secondary_boot) >>> + return 0; >>> + >>> + ret = of_property_read_u32(node, OF_SECONDARY_BOOT, &secondary_boot); >>> + if (ret) >>> + pr_err("%s: missing/invalid " OF_SECONDARY_BOOT " property\n", >>> + node->name); >>> + >>> + return ret; >>> +} >> I don't understand why we need this. Why can't we get the secondary boot >> address from the /cpus node in the smp_prepare_cpus op. It isn't that >> hard to get access to the cpus node there via of_find_node_by_path(). >> Then we don't need patch 1 at all. If it turns out to be common stuff, >> we can always have the common function live in arm common code or maybe >> even be a devicetree API. > I already responded to this, but never got any response. I > was preparing to re-send this series and wanted to try to > pull the added feature (patch 1) out and not be dependent on > it. But I think it's a bit ugly so I'm hoping to get a > blessing to proceed with what I originally proposed. For > reference, here's the thread: > https://lkml.org/lkml/2014/4/3/421 > > What I'm trying to do is get the value of a "secondary-boot-reg" > property from a node known to have an "enable-method" property > that matches the method name supplied in CPU_METHOD_OF_DECLARE(). I don't recall seeing any documentation about the secondary-boot-reg property. Please make sure it is documented in the next patch series. > > Using the callback function as I originally proposed, this is > very easy. When arm_dt_init_cpu_maps() parses the "cpus" portion > of the device tree it calls set_smp_ops_by_method() for a > matching "cpu" or "cpus" node, and that function supplies > the node to the callback function. The callback can extract > additional property values if needed. > > If I hold off until smp_prepare_cpus() is called, I have to > re-parse the device tree to find the "cpus" node (this is > in itself trivial). I then need to re-parse that node to > verify the matching "enable-method" property is found before > looking for the parameter information I need for that enable > method. I would really prefer not to re-do this parsing > step. It's imprecise and a little inefficient, and it > duplicates (but not exactly) logic that's already performed > by arm_dt_init_cpu_maps(). Do you have any devices where the enable-method and secondary-boot-reg isn't the same across all CPUs? A lot of the complexity comes from broken DTs that don't specify a secondary-boot-reg along with the enable-method. From the description and the code it seems that we should just always put these two properties in the cpus node to make things simple and precise. I agree it's a minor duplication to read the DT again to get the /cpus node and read a property out of it, but I doubt you could even measure the difference. > > One more point of clarification. This "secondary-boot-reg" > value is *not* the secondary boot address--that is, it's > not the address secondary cores jump to when they are > activated. Instead, this is the address of a register > that's used to request the ROM code release a core from > its ROM-implemented holding pen. For this machine, > control jumps at that point to secondary_startup(), > defined in arch/arm/kernel/head.S. Yes it wouldn't be possible to specify the entry address in DT (depends on compile time factors). How is this different from cpu-release-addr though? It looks like it describes something similar to what ePAPR describes and how arm64 uses it (although those two slightly differ). Assuming it's paired with a different enable-method than spin-table I don't see a problem reusing the same name. > > So... > > Stephen, I'd like to hear from you whether my explanation > is adequate, and whether you think my addition and use of > CPU_METHOD_OF_DECLARE_SETUP() is reasonable. (If you have > a suggestion for a better name, I'm open.) > > If you still don't like it, I'll follow up with a > new version of the patches, this time parsing the > device tree in the smp_prepare_cpus() method as > you suggested. I don't want this to hold up getting > this SMP support into the kernel. > This was my train of thought. It annoys me that we have smp ops at two levels. We already have SMP ops that deal with SMP things and now we have another level that is associated with the enable-method. If we really need this callback why couldn't we just collapse it with the smp_ops we already have? When we do that we realize that smp_prepare_cpus() is a fairly similar place to do this sort of thing, but changing that callback to take the node is annoying to propagate to all users. But do we really need that node? It seems simple enough to just get the cpus node and then read out the property we care about and we can do all that in smp_prepare_cpus() one time (and keep the mapping around forever) without requiring any core ARM changes. Do you have any more complicated use cases for this setup hook? RIght now I'm not convinced that we need to add all this framework for something that looks like it could be done in a couple lines in the prepare_cpus() step. cpus = of_find_node_by_path("/cpus") ret = of_property_read_u32(cpus, "secondary-boot-reg", &addr);
On 05/05/2014 08:43 PM, Stephen Boyd wrote: > On 05/05/14 15:02, Alex Elder wrote: >> On 04/04/2014 12:55 PM, Stephen Boyd wrote: >>> On 04/03/14 19:18, Alex Elder wrote: >>>> + >>>> +/* >>>> + * Secondary startup method setup routine to extract the location of >>>> + * the secondary boot register from a "cpu" or "cpus" device tree >>>> + * node. Only the first seen secondary boot register value is used; >>>> + * any others are ignored. The secondary boot register value must be >>>> + * non-zero. >>>> + * >>>> + * Returns 0 if successful or an error code otherwise. >>>> + */ >>>> +static int __init of_enable_method_setup(struct device_node *node) >>>> +{ >>>> + int ret; >>>> + >>>> + /* Ignore all but the first one specified */ >>>> + if (secondary_boot) >>>> + return 0; >>>> + >>>> + ret = of_property_read_u32(node, OF_SECONDARY_BOOT, &secondary_boot); >>>> + if (ret) >>>> + pr_err("%s: missing/invalid " OF_SECONDARY_BOOT " property\n", >>>> + node->name); >>>> + >>>> + return ret; >>>> +} >>> I don't understand why we need this. Why can't we get the secondary boot >>> address from the /cpus node in the smp_prepare_cpus op. It isn't that >>> hard to get access to the cpus node there via of_find_node_by_path(). >>> Then we don't need patch 1 at all. If it turns out to be common stuff, >>> we can always have the common function live in arm common code or maybe >>> even be a devicetree API. >> I already responded to this, but never got any response. I >> was preparing to re-send this series and wanted to try to >> pull the added feature (patch 1) out and not be dependent on >> it. But I think it's a bit ugly so I'm hoping to get a >> blessing to proceed with what I originally proposed. For >> reference, here's the thread: >> https://lkml.org/lkml/2014/4/3/421 >> >> What I'm trying to do is get the value of a "secondary-boot-reg" >> property from a node known to have an "enable-method" property >> that matches the method name supplied in CPU_METHOD_OF_DECLARE(). > > I don't recall seeing any documentation about the secondary-boot-reg > property. Please make sure it is documented in the next patch series. I omitted the documentation, hoping to get some input about how best to go about it. I talked with Mark Rutland last week and agreed to split off the "enable-method" descriptions into a file separate from "Documentation/devicetree/bindings/arm/cpus.txt" because it seemed like there might be a proliferation of them. I will definitely include it this time around. >> Using the callback function as I originally proposed, this is >> very easy. When arm_dt_init_cpu_maps() parses the "cpus" portion >> of the device tree it calls set_smp_ops_by_method() for a >> matching "cpu" or "cpus" node, and that function supplies >> the node to the callback function. The callback can extract >> additional property values if needed. >> >> If I hold off until smp_prepare_cpus() is called, I have to >> re-parse the device tree to find the "cpus" node (this is >> in itself trivial). I then need to re-parse that node to >> verify the matching "enable-method" property is found before >> looking for the parameter information I need for that enable >> method. I would really prefer not to re-do this parsing >> step. It's imprecise and a little inefficient, and it >> duplicates (but not exactly) logic that's already performed >> by arm_dt_init_cpu_maps(). > > Do you have any devices where the enable-method and secondary-boot-reg > isn't the same across all CPUs? A lot of the complexity comes from To be clear, my discussion here is thinking more broadly than this specific case--beyond the Broadcom SoCs I'm adding support for. But to answer your question, I am not aware of any such devices. > broken DTs that don't specify a secondary-boot-reg along with the > enable-method. From the description and the code it seems that we should > just always put these two properties in the cpus node to make things > simple and precise. I agree it's a minor duplication to read the DT Yes, I agree that specifying it only in "cpus" is preferable in general, and specifically that was how I was going to describe this binding. I could imagine a big.LITTLE machine could have different methods but I have no experience with that. It seems we should just dictate using the "cpus" node (not "cpu" nodes) unless/until there comes along a real machine that requires multiple methods. (But as the code now stands, only the first matching "cpu" node is ever used--any others are ignored--so it wouldn't support such a theoretical case anyway.) > again to get the /cpus node and read a property out of it, but I doubt > you could even measure the difference. No, that's why I said "a little inefficient." My bigger issue has to do with the fact that arm_dt_init_cpu_maps() already does a comprehensive job of parsing device tree nodes and finding matching "enable-method" entries. I view this extra "secondary-boot-reg" property as a *parameter* of this particular enable method. With this view I think the parameter properties (if any) should be parsed in the same context that the "enable-method" was found. Having it get parsed much later, in a separate pass through the device tree just seems like splitting up something that should be logically grouped. A different enable method might require a different parameter. So for a specific example, I might define a setup function to get the "cpu-release-address" property for a "spin-table" enable method, if one were to be defined. If we were restricted to the single "cpus" node this would be much simpler, but I'd still argue the enable method and its related properties should be treated together. >> One more point of clarification. This "secondary-boot-reg" >> value is *not* the secondary boot address--that is, it's >> not the address secondary cores jump to when they are >> activated. Instead, this is the address of a register >> that's used to request the ROM code release a core from >> its ROM-implemented holding pen. For this machine, >> control jumps at that point to secondary_startup(), >> defined in arch/arm/kernel/head.S. > > Yes it wouldn't be possible to specify the entry address in DT (depends > on compile time factors). How is this different from cpu-release-addr Sorry, I misstated that. I meant it's not the cpu-release-addr (which is the location of the register into which the secondary boot address should be written). > though? It looks like it describes something similar to what ePAPR > describes and how arm64 uses it (although those two slightly differ). > Assuming it's paired with a different enable-method than spin-table I > don't see a problem reusing the same name. I suppose. I hate to use the same name for something whose use is semantically different. I'm not sure why there's value to reusing the name. To be honest I don't think the set_smp_ops_by_method() call(s) necessarily belongs in arm_dt_init_cpu_maps(). It seems like it could just as easily be done inside smp_init_cpus(). I don't think the smp_operations vector needs to be set at that early stage, and it was done where it is as more a matter of convenience. Note that arm64 uses cpu_operations rather than smp_operations and there are some differences. The cpu_init() method gets called from smp_init_cpus(), right after the "enable-method" property is parsed. That method passes the matching device node, so it really does something very much like what I'm proposing. >> >> So... >> >> Stephen, I'd like to hear from you whether my explanation >> is adequate, and whether you think my addition and use of >> CPU_METHOD_OF_DECLARE_SETUP() is reasonable. (If you have >> a suggestion for a better name, I'm open.) >> >> If you still don't like it, I'll follow up with a >> new version of the patches, this time parsing the >> device tree in the smp_prepare_cpus() method as >> you suggested. I don't want this to hold up getting >> this SMP support into the kernel. >> > > This was my train of thought. It annoys me that we have smp ops at two > levels. We already have SMP ops that deal with SMP things and now we > have another level that is associated with the enable-method. If we I could add the callback as a new member of the smp_operations struct but it doesn't really fit there (like you said, it's tied to the enable method more than SMP). > really need this callback why couldn't we just collapse it with the > smp_ops we already have? When we do that we realize that > smp_prepare_cpus() is a fairly similar place to do this sort of thing, > but changing that callback to take the node is annoying to propagate to I have no problem making such a change if it's the right thing to do. I found the existing interfaces didn't support what I wanted to do so I tried to make them more flexible. > all users. But do we really need that node? It seems simple enough to > just get the cpus node and then read out the property we care about and > we can do all that in smp_prepare_cpus() one time (and keep the mapping > around forever) without requiring any core ARM changes. (I don't really like leaking mappings either, trivial as that may seem.) > Do you have any more complicated use cases for this setup hook? RIght > now I'm not convinced that we need to add all this framework for > something that looks like it could be done in a couple lines in the > prepare_cpus() step. > > cpus = of_find_node_by_path("/cpus") > ret = of_property_read_u32(cpus, "secondary-boot-reg", &addr); For the case of these Broadcom SoCs, I think this is sufficient. I'll put together a new version of the series in the morning. I think I can do what you suggest or very similar. I think there's more improvement that can be made in this SMP setup code though, so I think this is good discussion. I appreciate your responses. -Alex -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig index 183fdef..863c623 100644 --- a/arch/arm/mach-bcm/Kconfig +++ b/arch/arm/mach-bcm/Kconfig @@ -14,7 +14,6 @@ config ARCH_BCM_MOBILE depends on MMU select ARCH_REQUIRE_GPIOLIB select ARM_ERRATA_754322 - select ARM_ERRATA_764369 if SMP select ARM_GIC select GPIO_BCM_KONA select TICK_ONESHOT @@ -31,18 +30,31 @@ menu "Broadcom Mobile SoC Selection" config ARCH_BCM_281XX bool "Broadcom BCM281XX SoC family" default y + select HAVE_SMP help - Enable support for the the BCM281XX family, which includes + Enable support for the BCM281XX family, which includes BCM11130, BCM11140, BCM11351, BCM28145 and BCM28155 variants. config ARCH_BCM_21664 bool "Broadcom BCM21664 SoC family" default y + select HAVE_SMP help - Enable support for the the BCM21664 family, which includes + Enable support for the BCM21664 family, which includes BCM21663 and BCM21664 variants. +config ARCH_BCM_MOBILE_SMP + bool "Broadcom mobile SoC SMP support" + depends on (ARCH_BCM_281XX || ARCH_BCM_21664) && SMP + default y + select HAVE_ARM_SCU + select ARM_ERRATA_764369 + help + SMP support for the BCM281XX and BCM21664 SoC families. + Provided as an option so SMP support for SoCs of this type + can be disabled for an SMP-enabled kernel. + endmenu endif diff --git a/arch/arm/mach-bcm/Makefile b/arch/arm/mach-bcm/Makefile index b2279e3..929579f 100644 --- a/arch/arm/mach-bcm/Makefile +++ b/arch/arm/mach-bcm/Makefile @@ -15,7 +15,10 @@ obj-$(CONFIG_ARCH_BCM_281XX) += board_bcm281xx.o plus_sec := $(call as-instr,.arch_extension sec,+sec) # BCM21664 -obj-$(CONFIG_ARCH_BCM_21664) += board_bcm21664.o +obj-$(CONFIG_ARCH_BCM_21664) := board_bcm21664.o + +# BCM281XX and BCM21664 SMP support +obj-$(CONFIG_ARCH_BCM_MOBILE_SMP) += platsmp.o # BCM281XX and BCM21664 L2 cache control obj-$(CONFIG_ARCH_BCM_MOBILE) += bcm_kona_smc.o bcm_kona_smc_asm.o kona.o diff --git a/arch/arm/mach-bcm/platsmp.c b/arch/arm/mach-bcm/platsmp.c new file mode 100644 index 0000000..46a64f2 --- /dev/null +++ b/arch/arm/mach-bcm/platsmp.c @@ -0,0 +1,194 @@ +/* + * Copyright (C) 2014 Broadcom Corporation + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation version 2. + * + * This program is distributed "as is" WITHOUT ANY WARRANTY of any + * kind, whether express or implied; without even the implied warranty + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/init.h> +#include <linux/errno.h> +#include <linux/io.h> +#include <linux/of.h> +#include <linux/sched.h> + +#include <asm/smp.h> +#include <asm/smp_plat.h> +#include <asm/smp_scu.h> + +/* Size of mapped Cortex A9 SCU address space */ +#define SCU_SIZE 0x58 + +#define SECONDARY_TIMEOUT_NS NSEC_PER_MSEC /* 1 msec (in nanoseconds) */ +#define BOOT_ADDR_CPUID_MASK 0x3 + +/* Name of device node property defining secondary boot register location */ +#define OF_SECONDARY_BOOT "secondary-boot-reg" + +/* I/O address of register used to coordinate secondary core startup */ +static u32 secondary_boot; + +/* + * Enable the Cortex A9 Snoop Control Unit + * + * By the time this is called we already know there are multiple + * cores present. We assume we're running on a Cortex A9 processor, + * so any trouble getting the base address register or getting the + * SCU base is a problem. + * + * Return 0 if successful or an error code otherwise. + */ +static int __init scu_a9_enable(void) +{ + unsigned long config_base; + void __iomem *scu_base; + + if (!scu_a9_has_base()) { + pr_err("no configuration base address register!\n"); + return -ENXIO; + } + + /* Config base address register value is zero for uniprocessor */ + config_base = scu_a9_get_base(); + if (!config_base) { + pr_err("hardware reports only one core; disabling SMP\n"); + return -ENOENT; + } + + scu_base = ioremap((phys_addr_t)config_base, SCU_SIZE); + if (!scu_base) { + pr_err("failed to remap config base (%lu/%u) for SCU\n", + config_base, SCU_SIZE); + return -ENOMEM; + } + + scu_enable(scu_base); + + iounmap(scu_base); /* That's the last we'll need of this */ + + return 0; +} + +static void __init bcm_smp_prepare_cpus(unsigned int max_cpus) +{ + static cpumask_t only_cpu_0 = { CPU_BITS_CPU0 }; + int ret; + + /* Enable SCU on Cortex A9 based SoCs */ + ret = scu_a9_enable(); + if (!ret) + return; + + /* + * If we have a uniprocessor, make sure the CPU present map + * reflects that. Bail on any other failure to enable the SCU. + */ + if (ret == -ENOENT) + init_cpu_present(&only_cpu_0); + else + BUG(); +} + +/* + * Secondary startup method setup routine to extract the location of + * the secondary boot register from a "cpu" or "cpus" device tree + * node. Only the first seen secondary boot register value is used; + * any others are ignored. The secondary boot register value must be + * non-zero. + * + * Returns 0 if successful or an error code otherwise. + */ +static int __init of_enable_method_setup(struct device_node *node) +{ + int ret; + + /* Ignore all but the first one specified */ + if (secondary_boot) + return 0; + + ret = of_property_read_u32(node, OF_SECONDARY_BOOT, &secondary_boot); + if (ret) + pr_err("%s: missing/invalid " OF_SECONDARY_BOOT " property\n", + node->name); + + return ret; +} + +/* + * The ROM code has the secondary cores looping, waiting for an event. + * When an event occurs each core examines the bottom two bits of the + * secondary boot register. When a core finds those bits contain its + * own core id, it performs initialization, including computing its boot + * address by clearing the boot register value's bottom two bits. The + * core signals that it is beginning its execution by writing its boot + * address back to the secondary boot register, and finally jumps to + * that address. + * + * So to start a core executing we need to: + * - Encode the (hardware) CPU id with the bottom bits of the secondary + * start address. + * - Write that value into the secondary boot register. + * - Generate an event to wake up the secondary CPU(s). + * - Wait for the secondary boot register to be re-written, which + * indicates the secondary core has started. + */ +static int bcm_boot_secondary(unsigned int cpu, struct task_struct *idle) +{ + void __iomem *boot_reg; + phys_addr_t boot_func; + u64 start_clock; + u32 cpu_id; + u32 boot_val; + bool timeout = false; + + cpu_id = cpu_logical_map(cpu); + if (cpu_id & ~BOOT_ADDR_CPUID_MASK) { + pr_err("bad cpu id (%u > %u)\n", cpu_id, BOOT_ADDR_CPUID_MASK); + return -EINVAL; + } + + if (!secondary_boot) { + pr_err("required secondary boot register not specified\n"); + return -EINVAL; + } + + boot_reg = ioremap_nocache((phys_addr_t)secondary_boot, sizeof(u32)); + if (!boot_reg) { + pr_err("unable to map boot register for cpu %u\n", cpu_id); + return -ENOSYS; + } + + boot_func = virt_to_phys(secondary_startup); + BUG_ON(boot_func & BOOT_ADDR_CPUID_MASK); + BUG_ON(boot_func > (phys_addr_t)U32_MAX); + + boot_val = (u32)boot_func | cpu_id; + writel_relaxed(boot_val, boot_reg); + + sev(); + + start_clock = local_clock(); + while (!timeout && readl_relaxed(boot_reg) == boot_val) + timeout = local_clock() - start_clock > SECONDARY_TIMEOUT_NS; + + iounmap(boot_reg); + + if (!timeout) + return 0; + + pr_err("timeout waiting for cpu %u to start\n", cpu_id); + + return -ENOSYS; +} + +static struct smp_operations bcm_smp_ops __initdata = { + .smp_prepare_cpus = bcm_smp_prepare_cpus, + .smp_boot_secondary = bcm_boot_secondary, +}; +CPU_METHOD_OF_DECLARE_SETUP(bcm_smp_bcm281xx, "brcm,bcm11351-cpu-method", + &of_enable_method_setup, &bcm_smp_ops);
This patch adds SMP support for BCM281XX and BCM21664 family SoCs. This feature is controlled with a distinct config option such that a SMP-enabled multi-v7 binary can be configured to run these SoCs in uniprocessor mode. Since this SMP functionality is used for multiple Broadcom mobile chip families the config option is called ARCH_BCM_MOBILE_SMP (for lack of a better name). On SoCs of this type, the secondary core is not held in reset on power-on. Instead it loops in a ROM-based holding pen. To release it, one must write into a special register a jump address whose low-order bits have been replaced with a secondary core's id, then trigger an event with SEV. On receipt of an event, the ROM code will examine the register's contents, and if the low-order bits match its cpu id, it will clear them and write the value back to the register just prior to jumping to the address specified. The location of the special register is defined in the device tree using a "secondary-boot-reg" property in a node whose "enable-method" matches. Derived from code originally provided by Ray Jui <rjui@broadcom.com> Signed-off-by: Alex Elder <elder@linaro.org> --- arch/arm/mach-bcm/Kconfig | 18 +++- arch/arm/mach-bcm/Makefile | 5 +- arch/arm/mach-bcm/platsmp.c | 194 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 213 insertions(+), 4 deletions(-) create mode 100644 arch/arm/mach-bcm/platsmp.c