Re: [PATCH] of: Fix address decoding on Bimini and js2x machines
On Thu, 2013-07-04 at 17:18 +0100, Grant Likely wrote: > > I'll include this in my 3.11 pull request for Linus > > Oops. Ben, I misread what you wrote. It would have been just fine to > include it in your powerpc -next branch. Sorry for the confusion. > > Anyway, I saw your powerpc pull req and that this patch wasn't in it, so > I've picked it up and will send it to Linus as soon as the test build > completes. Yeah, I was about to add it to a subsequent "fixes" branch but since you picked it up I'll just leave it. Thanks ! Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
[PATCH] of: Fix address decoding on Bimini and js2x machines
Commit:
e38c0a1fbc5803cbacdaac0557c70ac8ca5152e7
of/address: Handle #address-cells > 2 specially
broke real time clock access on Bimini, js2x, and similar powerpc
machines using the "maple" platform. That code was indirectly relying
on the old (broken) behaviour of the translation for the hypertransport
to ISA bridge.
This fixes it by treating hypertransport as a PCI bus
Signed-off-by: Benjamin Herrenschmidt
CC: [v3.6+]
---
Rob, if you have no objection I will put that in powerpc -next
diff --git a/drivers/of/address.c b/drivers/of/address.c
index 04da786..7c8221d 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -106,8 +106,12 @@ static unsigned int of_bus_default_get_flags(const __be32 *
static int of_bus_pci_match(struct device_node *np)
{
- /* "vci" is for the /chaos bridge on 1st-gen PCI powermacs */
- return !strcmp(np->type, "pci") || !strcmp(np->type, "vci");
+ /*
+* "vci" is for the /chaos bridge on 1st-gen PCI powermacs
+* "ht" is hypertransport
+*/
+ return !strcmp(np->type, "pci") || !strcmp(np->type, "vci") ||
+ !strcmp(np->type, "ht");
}
static void of_bus_pci_count_cells(struct device_node *np,
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [GIT PULL] devicetree bugfixes for v3.10
On Thu, 2013-06-13 at 23:22 +0100, Grant Likely wrote: > Grant Likely (2): > dtc: Update generated files to output from Bison 2.5 > dtc: ensure #line directives don't consume data from the next Question. Are those fixes also in the upstream dtc from jdl ? Or have we effectively forked it ? Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] of: Fix locking vs. interrupts
On Wed, 2013-06-12 at 10:25 +0200, Thomas Gleixner wrote: > On Wed, 12 Jun 2013, Benjamin Herrenschmidt wrote: > > > The OF code uses irqsafe locks everywhere except in a handful of functions > > for no obvious reasons. Since the conversion from the old rwlocks, this > > now triggers lockdep warnings when used at interrupt time. At least one > > driver (ibmvscsi) seems to be doing that from softirq context. > > > > This converts the few non-irqsafe locks into irqsafe ones, making them > > consistent with the rest of the code. > > Fun. https://lkml.org/lkml/2013/2/4/416 seems to have got lost > > > Signed-off-by: Benjamin Herrenschmidt > > CC: [v3.9+] > > Acked-by: Thomas Gleixner > > > --- > > > > Note: It's silly to access the device-tree at interrupt time in most cases, > > and we should probably fix ibmvscsi, but for the time being, let's fix the > > Right. > > > obvious bug. Thomas, this can probably still go into 3.10... If not, I've > > CCed stable. > > Should go through Grant I think. Right, thinko. Sent to you due to the bug being exposed by your conversion to spinlocks. Anyway, Grant got it now. Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
[PATCH] of: Fix locking vs. interrupts
The OF code uses irqsafe locks everywhere except in a handful of functions
for no obvious reasons. Since the conversion from the old rwlocks, this
now triggers lockdep warnings when used at interrupt time. At least one
driver (ibmvscsi) seems to be doing that from softirq context.
This converts the few non-irqsafe locks into irqsafe ones, making them
consistent with the rest of the code.
Signed-off-by: Benjamin Herrenschmidt
CC: [v3.9+]
---
Note: It's silly to access the device-tree at interrupt time in most cases,
and we should probably fix ibmvscsi, but for the time being, let's fix the
obvious bug. Thomas, this can probably still go into 3.10... If not, I've
CCed stable.
diff --git a/arch/sparc/kernel/prom_common.c b/arch/sparc/kernel/prom_common.c
index 9f20566..79cc0d1 100644
--- a/arch/sparc/kernel/prom_common.c
+++ b/arch/sparc/kernel/prom_common.c
@@ -54,6 +54,7 @@ EXPORT_SYMBOL(of_set_property_mutex);
int of_set_property(struct device_node *dp, const char *name, void *val, int
len)
{
struct property **prevp;
+ unsigned long flags;
void *new_val;
int err;
@@ -64,7 +65,7 @@ int of_set_property(struct device_node *dp, const char *name,
void *val, int len
err = -ENODEV;
mutex_lock(&of_set_property_mutex);
- raw_spin_lock(&devtree_lock);
+ raw_spin_lock_irqsave(&devtree_lock, flags);
prevp = &dp->properties;
while (*prevp) {
struct property *prop = *prevp;
@@ -91,7 +92,7 @@ int of_set_property(struct device_node *dp, const char *name,
void *val, int len
}
prevp = &(*prevp)->next;
}
- raw_spin_unlock(&devtree_lock);
+ raw_spin_unlock_irqrestore(&devtree_lock, flags);
mutex_unlock(&of_set_property_mutex);
/* XXX Upate procfs if necessary... */
diff --git a/drivers/of/base.c b/drivers/of/base.c
index f53b992..951452c 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -192,14 +192,15 @@ EXPORT_SYMBOL(of_find_property);
struct device_node *of_find_all_nodes(struct device_node *prev)
{
struct device_node *np;
+ unsigned long flags;
- raw_spin_lock(&devtree_lock);
+ raw_spin_lock_irqsave(&devtree_lock, flags);
np = prev ? prev->allnext : of_allnodes;
for (; np != NULL; np = np->allnext)
if (of_node_get(np))
break;
of_node_put(prev);
- raw_spin_unlock(&devtree_lock);
+ raw_spin_unlock_irqrestore(&devtree_lock, flags);
return np;
}
EXPORT_SYMBOL(of_find_all_nodes);
@@ -421,8 +422,9 @@ struct device_node *of_get_next_available_child(const
struct device_node *node,
struct device_node *prev)
{
struct device_node *next;
+ unsigned long flags;
- raw_spin_lock(&devtree_lock);
+ raw_spin_lock_irqsave(&devtree_lock, flags);
next = prev ? prev->sibling : node->child;
for (; next; next = next->sibling) {
if (!__of_device_is_available(next))
@@ -431,7 +433,7 @@ struct device_node *of_get_next_available_child(const
struct device_node *node,
break;
}
of_node_put(prev);
- raw_spin_unlock(&devtree_lock);
+ raw_spin_unlock_irqrestore(&devtree_lock, flags);
return next;
}
EXPORT_SYMBOL(of_get_next_available_child);
@@ -735,13 +737,14 @@ EXPORT_SYMBOL_GPL(of_modalias_node);
struct device_node *of_find_node_by_phandle(phandle handle)
{
struct device_node *np;
+ unsigned long flags;
- raw_spin_lock(&devtree_lock);
+ raw_spin_lock_irqsave(&devtree_lock, flags);
for (np = of_allnodes; np; np = np->allnext)
if (np->phandle == handle)
break;
of_node_get(np);
- raw_spin_unlock(&devtree_lock);
+ raw_spin_unlock_irqrestore(&devtree_lock, flags);
return np;
}
EXPORT_SYMBOL(of_find_node_by_phandle);
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 1/5] ibmebus: convert of_platform_driver to platform_driver
On Wed, 2013-05-22 at 07:26 -0500, Rob Herring wrote: > > git://sources.calxeda.com/kernel/linux.git of-platform-removal > > Ben, > > Did you have a chance to test this? I want to get this into -next. I tested the one in for-next (sorry for the high latency). Works fine. Ack. Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 1/5] ibmebus: convert of_platform_driver to platform_driver
On Wed, 2013-05-22 at 07:26 -0500, Rob Herring wrote: > Did you have a chance to test this? I want to get this into -next. Ah sorry, skipped out of my mind, I'll get to it asap... Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] bestcomm: no need to free when kzalloc fail
On Wed, 2013-05-22 at 12:49 +0800, Libo Chen wrote: > ping... This is pointless. We routinely avoid adding such crap by having the various free(...) routines cope with NULL. You just need to make sure you are indeed NULL in the error case. Ben. > On 2013/5/5 16:38, chenlib...@gmail.com wrote: > > From: Libo Chen > > > > There is no need to free bcom_eng if kzalloc fail > > > > Signed-off-by: Libo Chen > > --- > > drivers/dma/bestcomm/bestcomm.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/dma/bestcomm/bestcomm.c > > b/drivers/dma/bestcomm/bestcomm.c > > index a8c2e29..300ee2d 100644 > > --- a/drivers/dma/bestcomm/bestcomm.c > > +++ b/drivers/dma/bestcomm/bestcomm.c > > @@ -400,7 +400,7 @@ static int mpc52xx_bcom_probe(struct platform_device > > *op) > > printk(KERN_ERR DRIVER_NAME ": " > > "Can't allocate state structure\n"); > > rv = -ENOMEM; > > - goto error_sramclean; > > + goto error_kzalloc; > > } > > > > /* Save the node */ > > @@ -449,6 +449,7 @@ error_release: > > release_mem_region(res_bcom.start, sizeof(struct mpc52xx_sdma)); > > error_sramclean: > > kfree(bcom_eng); > > +error_kzalloc: > > bcom_sram_cleanup(); > > error_ofput: > > of_node_put(op->dev.of_node); > > > ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [RFC PATCH v3] drivers: bus: add ARM CCI support
On Tue, 2013-05-21 at 10:21 +0100, Lorenzo Pieralisi wrote: > The node name, not type, but point is taken. I prefer to add a > compatible property to slave-if nodes (eg "arm,cci-400-control-if"), > I do not think that checking the node name is the standard way of > doing > things in DT world. > > Thoughts ? Agreed. Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v8 1/3] of/pci: Unify pci_process_bridge_OF_ranges from Microblaze and PowerPC
On Tue, 2013-05-07 at 09:01 +0100, Andrew Murray wrote: > > There were no objections to this latest revision until now and it is > currently sitting with Jason Cooper (mvebu-next/pcie). [1] Ok, well I've just sent Linus a pull request for my changes so at least drop the powerpc changes from your tree for the time being. > This is a view that was also shared by Bjorn [2] when I attempted to > submit a patchset which moves struct pci_controller to asm-generic. Right, it's the logical way to go > The motativation for my patchsets were to give a way for ARM PCI host > bridge drivers to parse the DT ranges property, but this snow-balled > into unifying pci_process_bridge_OF_ranges. Which I understand, I would probably have done the same thing in your shoes :-) > My v8 patchset provides a of_pci_range_parser which is used directly > by a few ARM PCI DT host bridge drivers, this has been generally > accepted and tested. I don't see why this can't remain and so I'd > really like to keep this around. Sure, no objection, in fact I should/could probably update my new code to use it as well. > Grant, Benjamin would you be happy for me to resubmit this series > which provides the of_pci_range_parser which will be used by the > separate implementations of pci_process_bridge_OF_ranges in > PowerPC/Microblaze? Sure, in fact feel free to update my new code if you have more bandwidth than I do, it should hit Linus tree soon hopefully unless he objects to me having a second pull request this merge window... > Benjamin are you able to still use of_pci_range_parser in your > 'Support per-aperture memory offset' patch? I see no reason why not. I just haven't looked into it much, I admit, being bogged down with a pile of new HW bringup in the lab etc... Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v8 1/3] of/pci: Unify pci_process_bridge_OF_ranges from Microblaze and PowerPC
On Mon, 2013-04-22 at 11:41 +0100, Andrew Murray wrote: > The pci_process_bridge_OF_ranges function, used to parse the "ranges" > property of a PCI host device, is found in both Microblaze and PowerPC > architectures. These implementations are nearly identical. This patch > moves this common code to a common place. What's happening with this ? I'd like to avoid that patch for now as I'm doing some changes to pci_process_bridge_OF_ranges which are fairly urgent (I might even stick them in the current merge window) to deal with memory windows having separate offsets. There's also a few hacks in there that are really ppc specific... I think the right long term approach is to change the way powerpc (and microblaze ?) initializes PCI host bridges. Move it away from setup_arch() (which is a PITA anyway since it's way too early) to an early init call of some sort, and encapsulate the new struct pci_host_bridge. We can then directly configure the host bridge windows rather than having this "intermediary" set of resources in our pci_controller and in fact move most of the fields from pci_controller to pci_host_bridge to the point where the former can remain as a simple platform specific wrapper if needed. So for new stuff (hint: DT based ARM PCI) or stuff that has to deal with a lot less archaic platforms (hint: Microblaze), I'd recommend going straight for that approach rather than perpetuating the PowerPC code which I'll try to deal with in the next few monthes. Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 1/5] ibmebus: convert of_platform_driver to platform_driver
On Thu, 2013-04-25 at 14:14 -0500, Rob Herring wrote: > On 04/25/2013 12:35 PM, Benjamin Herrenschmidt wrote: > > On Thu, 2013-04-25 at 10:23 -0500, Rob Herring wrote: > >> Ben, Can I have your Ack for this? The change is straightforward and > >> neither of the 2 drivers used the id parameter that is removed. > > > > Didn't you get my mail about a compile failure caused by this patch ? > > No, and I can't find any evidence of a mail in my inbox or in the list > archives. Odd... > > .../... > You need patch 2 of this series to fix this: > > driver core: move to_platform_driver to platform_device.h > > which as Arnd pointed out needs to come first. I've fixed that in my > tree, but I don't think it warrants another post. Can I pull you tree from somewhere to test ? Cheers, Ben. > Rob > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 1/5] ibmebus: convert of_platform_driver to platform_driver
On Thu, 2013-04-25 at 10:23 -0500, Rob Herring wrote: > Ben, Can I have your Ack for this? The change is straightforward and > neither of the 2 drivers used the id parameter that is removed. Didn't you get my mail about a compile failure caused by this patch ? Or did you send an update that I missed ? (Copy of the original email below) Cheers, Ben. On Sun, 2013-04-21 at 21:13 -0500, Rob Herring wrote: > From: Rob Herring > > ibmebus is the last remaining user of of_platform_driver and the > conversion to a regular platform driver is trivial. A quick build test leads to: /home/benh/linux-powerpc-test/arch/powerpc/kernel/ibmebus.c: In function 'ibmebus_bus_device_probe': /home/benh/linux-powerpc-test/arch/powerpc/kernel/ibmebus.c:344:2: error: implicit declaration of function 'to_platform_driver' [-Werror=implicit-function-declaration] /home/benh/linux-powerpc-test/arch/powerpc/kernel/ibmebus.c:344:6: error: assignment makes pointer from integer without a cast [-Werror] /home/benh/linux-powerpc-test/arch/powerpc/kernel/ibmebus.c: In function 'ibmebus_bus_device_remove': /home/benh/linux-powerpc-test/arch/powerpc/kernel/ibmebus.c:363:32: error: initialization makes pointer from integer without a cast [-Werror] /home/benh/linux-powerpc-test/arch/powerpc/kernel/ibmebus.c: In function 'ibmebus_bus_device_shutdown': /home/benh/linux-powerpc-test/arch/powerpc/kernel/ibmebus.c:373:32: error: initialization makes pointer from integer without a cast [-Werror] /home/benh/linux-powerpc-test/arch/powerpc/kernel/ibmebus.c: In function 'ibmebus_bus_legacy_suspend': /home/benh/linux-powerpc-test/arch/powerpc/kernel/ibmebus.c:420:32: error: initialization makes pointer from integer without a cast [-Werror] /home/benh/linux-powerpc-test/arch/powerpc/kernel/ibmebus.c: In function 'ibmebus_bus_legacy_resume': /home/benh/linux-powerpc-test/arch/powerpc/kernel/ibmebus.c:431:32: error: initialization makes pointer from integer without a cast [-Werror] cc1: all warnings being treated as errors make[2]: *** [arch/powerpc/kernel/ibmebus.o] Error 1 make[2]: *** Waiting for unfinished jobs Haven't had a chance to look too closely today -EJETLAG :-) ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [RFC PATCH 2/2] ARM: DT: kernel: DT cpu node bindings update
On Wed, 2013-04-17 at 09:14 -0600, Stephen Warren wrote: > On 04/17/2013 03:14 AM, Mark Rutland wrote: > > Hi Stephen, > > > >>> + - enable-method > >>> + Usage: required on ARM 64-bit systems, optional on ARM 32-bit > >>> +systems > >>> + Value type: > >>> + Definition: On ARM 64-bit systems must be "spin-table" [1]. > >> > >> Can that be an integer instead? with dtc+cpp support, that shouldn't > >> hurt the eyes too much any more. > > > > The "enable-method" property is described as a stringlist by ePAPR, and is > > currently in use on arm64 as such. It *must* remain a string(list) for > > arm64. > > > > Having it as an integer for arm is only going to cause us additional work, > > makes it impossible to share a common dt between 64bit and 32bit, and goes > > against the standard. I think it should be a stringlist for arm. > > OK, that's a great reason for this case. > > I hope we don't introduce any more standards that use strings, but that > may just be my personal preference... I happen to have the exact opposite opinion :-) Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [RFC PATCH 2/2] ARM: DT: kernel: DT cpu node bindings update
On Tue, 2013-04-16 at 09:57 -0600, Stephen Warren wrote: > > Mmm, I need to read more on dtc+cpp, I do not think that leaving it > > as a string would hurt though, am I wrong ? Can we assume that all > dts > > are preprocessed before being compiled and passed to the kernel ? > > It wouldn't hurt too much, but representing enums as strings when > there's a capability to simply represent it as a integer just feels > pretty gross to me. It's the other way around... Unless there's a direct semantic meaning of the numbers, things like that (ie. "methods") should be represented as strings. Else we fall back to the realm of "magic numbers" which sucks and causes all sort of pains when several new methods get added by different people, etc... The tree is meant to be generally human readable and we should stick to that. The tiny memory usage is irrelevant on any modern setup. Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v6 0/3] of/pci: Provide common support for PCI DT parsing
On Mon, 2013-04-15 at 20:29 +0200, Linus Walleij wrote: > > As agreed with Rob Herring, series applied to mvebu/drivers to > support > > mvebu pcie driver. > > Will this hit ARM SoC soon-ish so I can base a pull request for the > Integrator on this stuff as well? Do not send this series upstream (or even in -next) until it has been at least build-tested and acked by the affected architectures. AFAIK it breaks microblaze build (simple missing #include) and isn't yet tested on powerpc (I'm on vacation, maybe somebody else on linuxppc-dev can do it though). Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v6 1/3] of/pci: Unify pci_process_bridge_OF_ranges from Microblaze and PowerPC
On Mon, 2013-04-15 at 14:57 +0200, Thomas Petazzoni wrote: > Michal, Ben, > > Would you have some time to look at this patch and give your comments > and/or ACK ? Since it touches the PowerPC and Microblaze core code, we > need your agreement to merge this code, and quite a bit of code pending > for 3.10 depends on this patch. I'm currently still on vacation. I will be able to look at this after I'm back in about a week. > Rob, alternatively, could we imagine doing a different version of the > 'of/pci: Provide support for parsing PCI DT ranges property' that > introduces the new API only, leaving the PowerPC and Microblaze rework > as follow-up efforts, so that all the PCIe drivers that depend on this > patch can get in for 3.10 ? I'd find it pretty sad if the Marvell PCIe > driver that has been worked on since 4+ months does not get into 3.10 > just because this patch cannot be merged. Cheers, Ben. > Thanks! > > Thomas > > On Thu, 11 Apr 2013 16:26:07 +0100, Andrew Murray wrote: > > The pci_process_bridge_OF_ranges function, used to parse the "ranges" > > property of a PCI host device, is found in both Microblaze and PowerPC > > architectures. These implementations are nearly identical. This patch > > moves this common code to a common place. > > > > Signed-off-by: Andrew Murray > > Signed-off-by: Liviu Dudau > > Reviewed-by: Rob Herring > > Tested-by: Thomas Petazzoni > > --- > > arch/microblaze/include/asm/pci-bridge.h |5 +- > > arch/microblaze/pci/pci-common.c | 192 > > > > arch/powerpc/include/asm/pci-bridge.h|5 +- > > arch/powerpc/kernel/pci-common.c | 192 > > > > drivers/of/of_pci.c | 200 > > ++ > > include/linux/of_pci.h |4 + > > 6 files changed, 206 insertions(+), 392 deletions(-) > > > > diff --git a/arch/microblaze/include/asm/pci-bridge.h > > b/arch/microblaze/include/asm/pci-bridge.h > > index cb5d397..5783cd6 100644 > > --- a/arch/microblaze/include/asm/pci-bridge.h > > +++ b/arch/microblaze/include/asm/pci-bridge.h > > @@ -10,6 +10,7 @@ > > #include > > #include > > #include > > +#include > > > > struct device_node; > > > > @@ -132,10 +133,6 @@ extern void setup_indirect_pci(struct pci_controller > > *hose, > > extern struct pci_controller *pci_find_hose_for_OF_device( > > struct device_node *node); > > > > -/* Fill up host controller resources from the OF node */ > > -extern void pci_process_bridge_OF_ranges(struct pci_controller *hose, > > - struct device_node *dev, int primary); > > - > > /* Allocate & free a PCI host bridge structure */ > > extern struct pci_controller *pcibios_alloc_controller(struct device_node > > *dev); > > extern void pcibios_free_controller(struct pci_controller *phb); > > diff --git a/arch/microblaze/pci/pci-common.c > > b/arch/microblaze/pci/pci-common.c > > index 9ea521e..2735ad9 100644 > > --- a/arch/microblaze/pci/pci-common.c > > +++ b/arch/microblaze/pci/pci-common.c > > @@ -622,198 +622,6 @@ void pci_resource_to_user(const struct pci_dev *dev, > > int bar, > > *end = rsrc->end - offset; > > } > > > > -/** > > - * pci_process_bridge_OF_ranges - Parse PCI bridge resources from device > > tree > > - * @hose: newly allocated pci_controller to be setup > > - * @dev: device node of the host bridge > > - * @primary: set if primary bus (32 bits only, soon to be deprecated) > > - * > > - * This function will parse the "ranges" property of a PCI host bridge > > device > > - * node and setup the resource mapping of a pci controller based on its > > - * content. > > - * > > - * Life would be boring if it wasn't for a few issues that we have to deal > > - * with here: > > - * > > - * - We can only cope with one IO space range and up to 3 Memory space > > - * ranges. However, some machines (thanks Apple !) tend to split their > > - * space into lots of small contiguous ranges. So we have to coalesce. > > - * > > - * - We can only cope with all memory ranges having the same offset > > - * between CPU addresses and PCI addresses. Unfortunately, some bridges > > - * are setup for a large 1:1 mapping along with a small "window" which > > - * maps PCI address 0 to some arbitrary high address of the CPU space > > in > > - * order to give access to the ISA memory hole. > > - * The way out of here that I've chosen for now is to always set the > > - * offset based on the first resource found, then override it if we > > - * have a different offset and the previous was set by an ISA hole. > > - * > > - * - Some busses have IO space not starting at 0, which causes trouble > > with > > - * the way we do our IO resource renumbering. The code somewhat deals > > with > > - * it for 64 bits but I would expect problems on 32 bits. > > - * > > - * - Some 32 bits platforms such as 4xx can
Re: [PATCH V2 2/2] of: remove /proc/device-tree
On Fri, 2013-03-22 at 13:03 -0500, Nathan Fontenot wrote: > We don't ever free old property values, mainly I assume since we don't keep > reference counts and can't know when it is safe to do so. The problem I > am starting to see on pseries is that we are getting very large properties. > One of the biggest culprits is the property on pseries systems to describe > the memory on the system in the device tree. These are big and getting > bigger as memory increases, additionally this property is update every > time memory is DLPAR added or removed from the system which can lead to > leaving a bunch of memory that should be free'ed. > > Given that, is there (or has there been) any discussion on adding reference > counts to properties in the device tree? With the myriad ways to get at > the value of a property this may not be feasible but I would like to hear > any thoughts from the community. My assumption was always that the lifetime of property values is tied the the lifetime of the node they are in. IE, we wouldn't free a property removed from a node but we could free all properties when the node goes away... Not the best but would do... refcount of properties, well ... Grant, do we get kobjects for them with the sysfs stuff ? That could do the trick... Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH V2 2/2] of: remove /proc/device-tree
On Thu, 2013-03-21 at 11:24 +, Grant Likely wrote:
> The same data is now available in sysfs, so we can remove the code
> that exports it in /proc and replace it with a symlink to the sysfs
> version.
>
> Tested on versatile qemu model and mpc5200 eval board. More testing
> would be appreciated.
This should be delayed until we are 100% confident that the sysfs
variant is totally backward compatible with anything that messes around
with /proc/device-tree.
kexec comes to mind (all 4 variants of fs2dt.c (yuck !)), dtc, various
powerpc-utils (bootloader configuration etc...), and more.
We also need to test the new code with hotplug, I'll see if I can get
somebody at IBM to give it a spin.
Cheers,
Ben.
> Signed-off-by: Grant Likely
> Cc: Rob Herring
> Cc: Greg Kroah-Hartman
> Cc: Benjamin Herrenschmidt
> Cc: David S. Miller
> ---
> drivers/of/Kconfig |8 --
> drivers/of/base.c | 60
> fs/proc/Makefile|1 -
> fs/proc/proc_devtree.c | 243
> ---
> fs/proc/root.c |3 -
> include/linux/of.h |1 -
> include/linux/proc_fs.h | 16
> 7 files changed, 332 deletions(-)
> delete mode 100644 fs/proc/proc_devtree.c
>
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index d2d7be9..edb97e2 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -7,14 +7,6 @@ config OF
> menu "Device Tree and Open Firmware support"
> depends on OF
>
> -config PROC_DEVICETREE
> - bool "Support for device tree in /proc"
> - depends on PROC_FS && !SPARC
> - help
> - This option adds a device-tree directory under /proc which contains
> - an image of the device tree that the kernel copies from Open
> - Firmware or other boot firmware. If unsure, say Y here.
> -
> config OF_SELFTEST
> bool "Device Tree Runtime self tests"
> help
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index f1298cf..3c8c74a 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -177,11 +177,8 @@ static int __of_node_add(struct device_node *np)
> if (!np->parent) {
> /* Nodes without parents are new top level trees */
> rc = kobject_add(&np->kobj, NULL, "device-tree-%i", extra++);
> -#if !defined(CONFIG_PROC_DEVICETREE)
> - /* Symlink to the new tree when PROC_DEVICETREE is disabled */
> if (!rc && extra == 1)
> proc_symlink("device-tree", NULL,
> "/sys/firmware/ofw/device-tree-0");
> -#endif /* CONFIG_PROC_DEVICETREE */
> } else {
> name = kbasename(np->full_name);
> if (!name || !name[0])
> @@ -1370,12 +1367,6 @@ int of_add_property(struct device_node *np, struct
> property *prop)
> *next = prop;
> raw_spin_unlock_irqrestore(&devtree_lock, flags);
>
> -#ifdef CONFIG_PROC_DEVICETREE
> - /* try to add to proc as well if it was initialized */
> - if (np->pde)
> - proc_device_tree_add_prop(np->pde, prop);
> -#endif /* CONFIG_PROC_DEVICETREE */
> -
> return 0;
> }
>
> @@ -1416,12 +1407,6 @@ int of_remove_property(struct device_node *np, struct
> property *prop)
> if (!found)
> return -ENODEV;
>
> -#ifdef CONFIG_PROC_DEVICETREE
> - /* try to remove the proc node as well */
> - if (np->pde)
> - proc_device_tree_remove_prop(np->pde, prop);
> -#endif /* CONFIG_PROC_DEVICETREE */
> -
> return 0;
> }
>
> @@ -1470,12 +1455,6 @@ int of_update_property(struct device_node *np, struct
> property *newprop)
> if (!found)
> return -ENODEV;
>
> -#ifdef CONFIG_PROC_DEVICETREE
> - /* try to add to proc as well if it was initialized */
> - if (np->pde)
> - proc_device_tree_update_prop(np->pde, newprop, oldprop);
> -#endif /* CONFIG_PROC_DEVICETREE */
> -
> return 0;
> }
>
> @@ -1510,22 +1489,6 @@ int of_reconfig_notify(unsigned long action, void *p)
> return notifier_to_errno(rc);
> }
>
> -#ifdef CONFIG_PROC_DEVICETREE
> -static void of_add_proc_dt_entry(struct device_node *dn)
> -{
> - struct proc_dir_entry *ent;
> -
> - ent = proc_mkdir(strrchr(dn->full_name, '/') + 1, dn->parent->pde);
> - if (ent)
> - proc_device_tree_add_node(dn, ent);
> -}
> -#else
> -static void of_add_proc_dt_entry(struct device_node *dn)
> -{
> - return;
> -}
> -#endif
> -
> /**
> * of_atta
Re: [PATCH V2 1/2] of: Make device nodes kobjects so they show up in sysfs
On Thu, 2013-03-21 at 11:24 +, Grant Likely wrote:
> Device tree nodes are already treated as objects, and we already want to
> expose them to userspace which is done using the /proc filesystem today.
> Right now the kernel has to do a lot of work to keep the /proc view in
> sync with the in-kernel representation. If device_nodes are switched to
> be kobjects then the device tree code can be a whole lot simpler. It
> also turns out that switching to using /sysfs from /proc results in
> smaller code and data size, and the userspace ABI won't change if
> /proc/device-tree symlinks to /sys/device-tree
Here you say /sys/device-tree
> +What:/sys/firmware/ofw/../device-tree/
Here you say /sys/firmware/../device-tree/ ... (wtf are those .. ?)
And further down:
proc_symlink("device-tree", NULL, "/sys/firmware/ofw/device-tree-0");
Some confusion here ... at least _I_ am confused :-)
Then, you do this:
> +static bool of_init_complete = false;
The above requires some explanations
> +static int __of_node_add(struct device_node *np)
> +{
> +
> + const char *name;
> + struct property *pp;
> + static int extra = 0;
> + int rc;
> +
> + np->kobj.kset = of_kset;
> + if (!np->parent) {
> + /* Nodes without parents are new top level trees */
> + rc = kobject_add(&np->kobj, NULL, "device-tree-%i", extra++);
> +#if !defined(CONFIG_PROC_DEVICETREE)
> + /* Symlink to the new tree when PROC_DEVICETREE is disabled */
> + if (!rc && extra == 1)
> + proc_symlink("device-tree", NULL,
> "/sys/firmware/ofw/device-tree-0");
> +#endif /* CONFIG_PROC_DEVICETREE */
WTF is this business of having multiple top level trees ? Also that
local static extra is gross. What is this all about ?
> + } else {
> + name = kbasename(np->full_name);
> + if (!name || !name[0])
> + return -EINVAL;
> + rc = kobject_add(&np->kobj, &np->parent->kobj, "%s", name);
> + }
> + if (rc)
> + return rc;
> +
> + for_each_property_of_node(np, pp) {
> + /* Important: Don't leak passwords */
> + bool secure = strncmp(pp->name, "security-", 9) == 0;
> +
> + pp->attr.attr.name = pp->name;
> + pp->attr.attr.mode = secure ? S_IRUSR : S_IRUGO;
> + pp->attr.size = secure ? 0 : pp->length;
> + pp->attr.read = of_node_property_read;
> + rc = sysfs_create_bin_file(&np->kobj, &pp->attr);
> + WARN(rc, "error creating device node attribute\n");
Might want some better message (attribute name, node path, ...)
We have mechanisms to deal with collisions in proc devicetree that you
don't seem to have here (or am I missing something ?). The main source
of pain is a property and a child node having the same name (happens
regulary with l2-cache on macs for example).
Cheers,
Ben.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: Kobjectify device tree structures
On Thu, 2013-03-21 at 11:24 +, Grant Likely wrote: > 11 files changed, 147 insertions(+), 342 deletions(-) Nice :-) Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 2/2] of: remove /proc/device-tree
On Thu, 2013-03-21 at 08:16 +, Grant Likely wrote: > On Thu, Mar 21, 2013 at 7:43 AM, Benjamin Herrenschmidt > wrote: > > On Thu, 2013-03-21 at 07:35 +, Grant Likely wrote: > >> > Shouldn't we have the symlink just be a config option itself ? > >> > Eventually distros might want get rid of it completely .. > >> > >> Why? It is the cheapest thing in the world and it means the ABI > >> doesn't change at all. > > > > It's also gross and forces sysfs to remain in /sys which isn't a kernel > > enforced policy afaik. > > Documentation/sysfs-rules.txt, Line 30 Whatever... it's still gross :-) Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 2/2] of: remove /proc/device-tree
On Thu, 2013-03-21 at 07:35 +, Grant Likely wrote: > > Shouldn't we have the symlink just be a config option itself ? > > Eventually distros might want get rid of it completely .. > > Why? It is the cheapest thing in the world and it means the ABI > doesn't change at all. It's also gross and forces sysfs to remain in /sys which isn't a kernel enforced policy afaik. Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 2/2] of: remove /proc/device-tree
On Wed, 2013-03-20 at 21:38 +, Grant Likely wrote: > > NAK. It should at the very least be a CONFIG option for a while > before > > completely switching over. > > I'll modify patch 1 to create the symlink if CONFIG_PROC_DEVICETREE is > not set. After the first patch can be applied we can leave it for a > release or two before applying the second. Shouldn't we have the symlink just be a config option itself ? Eventually distros might want get rid of it completely .. Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 1/2] of: Make device nodes kobjects so they show up in sysfs
On Wed, 2013-03-20 at 09:56 -0700, Greg Kroah-Hartman wrote: > > Unfortunately they occasionally are... VPDs can be pretty big for > > example. > > If the attributes are binary blobs, use the binary file capability of > sysfs to properly handle them. Except that we don't know that ... we have properties which comes all as "blobs", only the consumers can interpret what they contain. Tools like lsprop or dtc do have some built-in smarts to differentiate for example strings, simple numbers and blobs based roughly on heuristics of content & size but that's purely for the sake of pretty printing. Something like /proc/device-tree (or a sysfs equivalent) has no way really to know what's in there. So basically they should *all* be binary blobs. > > > Second, all normal sysfs attributes report their size as 4096 bytes > > > instead of the actual property size reported in /proc/device-tree. It is > > > possible that this change will cause some userspace tools to break. > > > > This is btw a complete idiocy of sysfs and should/could be fixed. > > How can sysfs change this? It doesn't know the size of the attribute > ahead of time, and it can change depending on what happens in the > system. So we default to a page size, which is the largest size an > attribute can be. That's true for some attributes I suppose and I while I do understand the difficulty there would be in calculating all the sizes on every stat, it's still gross, especially for those plenty of attributes who do have a fixed size. In our case, we do know the size, we should expose it. Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 2/2] of: remove /proc/device-tree
On Wed, 2013-03-20 at 14:51 +, Grant Likely wrote:
> The same data is now available in sysfs, so we can remove the code
> that exports it in /proc and replace it with a symlink to the sysfs
> version.
>
> Tested on versatile qemu model and mpc5200 eval board. More testing
> would be appreciated.
NAK. It should at the very least be a CONFIG option for a while before
completely switching over.
Cheers,
Ben.
> Signed-off-by: Grant Likely
> Cc: Rob Herring
> Cc: Greg Kroah-Hartman
> Cc: Benjamin Herrenschmidt
> Cc: David S. Miller
> ---
> drivers/of/Kconfig |8 --
> drivers/of/base.c | 59 +---
> fs/proc/Makefile|1 -
> fs/proc/proc_devtree.c | 243
> ---
> fs/proc/root.c |3 -
> include/linux/of.h |1 -
> include/linux/proc_fs.h | 16
> 7 files changed, 2 insertions(+), 329 deletions(-)
> delete mode 100644 fs/proc/proc_devtree.c
>
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index d2d7be9..edb97e2 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -7,14 +7,6 @@ config OF
> menu "Device Tree and Open Firmware support"
> depends on OF
>
> -config PROC_DEVICETREE
> - bool "Support for device tree in /proc"
> - depends on PROC_FS && !SPARC
> - help
> - This option adds a device-tree directory under /proc which contains
> - an image of the device tree that the kernel copies from Open
> - Firmware or other boot firmware. If unsure, say Y here.
> -
> config OF_SELFTEST
> bool "Device Tree Runtime self tests"
> help
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 363e98b..6714114 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -184,6 +184,8 @@ static int __of_node_add(struct device_node *np)
> np->kobj.kset = of_kset;
> if (!np->parent) {
> /* Nodes without parents are new top level trees */
> + if (extra == 0)
> + proc_symlink("device-tree", NULL,
> "/sys/firmware/ofw/device-tree-0");
> rc = kobject_add(&np->kobj, NULL, "device-tree-%i", extra++);
> } else {
> name = kbasename(np->full_name);
> @@ -1375,12 +1377,6 @@ int of_add_property(struct device_node *np, struct
> property *prop)
> *next = prop;
> raw_spin_unlock_irqrestore(&devtree_lock, flags);
>
> -#ifdef CONFIG_PROC_DEVICETREE
> - /* try to add to proc as well if it was initialized */
> - if (np->pde)
> - proc_device_tree_add_prop(np->pde, prop);
> -#endif /* CONFIG_PROC_DEVICETREE */
> -
> return 0;
> }
>
> @@ -1421,12 +1417,6 @@ int of_remove_property(struct device_node *np, struct
> property *prop)
> if (!found)
> return -ENODEV;
>
> -#ifdef CONFIG_PROC_DEVICETREE
> - /* try to remove the proc node as well */
> - if (np->pde)
> - proc_device_tree_remove_prop(np->pde, prop);
> -#endif /* CONFIG_PROC_DEVICETREE */
> -
> return 0;
> }
>
> @@ -1475,12 +1465,6 @@ int of_update_property(struct device_node *np, struct
> property *newprop)
> if (!found)
> return -ENODEV;
>
> -#ifdef CONFIG_PROC_DEVICETREE
> - /* try to add to proc as well if it was initialized */
> - if (np->pde)
> - proc_device_tree_update_prop(np->pde, newprop, oldprop);
> -#endif /* CONFIG_PROC_DEVICETREE */
> -
> return 0;
> }
>
> @@ -1515,22 +1499,6 @@ int of_reconfig_notify(unsigned long action, void *p)
> return notifier_to_errno(rc);
> }
>
> -#ifdef CONFIG_PROC_DEVICETREE
> -static void of_add_proc_dt_entry(struct device_node *dn)
> -{
> - struct proc_dir_entry *ent;
> -
> - ent = proc_mkdir(strrchr(dn->full_name, '/') + 1, dn->parent->pde);
> - if (ent)
> - proc_device_tree_add_node(dn, ent);
> -}
> -#else
> -static void of_add_proc_dt_entry(struct device_node *dn)
> -{
> - return;
> -}
> -#endif
> -
> /**
> * of_attach_node - Plug a device node into the tree and global list.
> */
> @@ -1551,31 +1519,9 @@ int of_attach_node(struct device_node *np)
> raw_spin_unlock_irqrestore(&devtree_lock, flags);
>
> of_node_add(np);
> - of_add_proc_dt_entry(np);
> return 0;
> }
>
> -#ifdef CONFIG_PROC_DEVICETREE
> -static void of_remove_proc_dt_entry(struct device_node *dn)
> -{
> - struct device_node *parent = dn->parent;
> - stru
Re: [PATCH 1/2] of: Make device nodes kobjects so they show up in sysfs
On Wed, 2013-03-20 at 14:51 +, Grant Likely wrote: > Device tree nodes are already treated as objects, and we already want to > expose them to userspace which is done using the /proc filesystem today. > Right now the kernel has to do a lot of work to keep the /proc view in > sync with the in-kernel representation. If device_nodes are switched to > be kobjects then the device tree code can be a whole lot simpler. It > also turns out that switching to using /sysfs from /proc results in > smaller code and data size, and the userspace ABI won't change if > /proc/device-tree symlinks to /sys/device-tree > > Switching to sysfs does introduce two limitations however. First, normal > sysfs attributes have a maximum size of PAGE_SIZE. Any properties larger > than 4k will still show up in sysfs, but attempting to read them will > result in truncated data. Practically speaking this should not be an > issue assuming large binary blobs aren't encoded into the device tree. Unfortunately they occasionally are... VPDs can be pretty big for example. > Second, all normal sysfs attributes report their size as 4096 bytes > instead of the actual property size reported in /proc/device-tree. It is > possible that this change will cause some userspace tools to break. This is btw a complete idiocy of sysfs and should/could be fixed. > Both of the above problems can be worked around by using > sysfs_create_bin_file() instead of sysfs_create_file(), but doing so > adds an additional 20 to 40 bytes of overhead per property. A device > tree has a lot of properties in it. That overhead will very quickly add > up. The other option is to create a new custom sysfs attribute type > would solve the problem without additional overhead, but at the cost of > more complexity in the sysfs support code. However, I'm hopeful that > these problems are just imaginary and we can stick with normal sysfs > attributes. Disagreed. It would break stuff, especially the incorrect file size. Cheers, Ben. > Signed-off-by: Grant Likely > Cc: Rob Herring > Cc: Greg Kroah-Hartman > Cc: Benjamin Herrenschmidt > Cc: David S. Miller > --- > Documentation/ABI/testing/sysfs-firmware-ofw | 28 ++ > drivers/of/Kconfig |2 +- > drivers/of/base.c| 121 > -- > drivers/of/fdt.c |3 +- > drivers/of/pdt.c |4 +- > include/linux/of.h |9 +- > 6 files changed, 154 insertions(+), 13 deletions(-) > create mode 100644 Documentation/ABI/testing/sysfs-firmware-ofw > > diff --git a/Documentation/ABI/testing/sysfs-firmware-ofw > b/Documentation/ABI/testing/sysfs-firmware-ofw > new file mode 100644 > index 000..5ef9a77 > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-firmware-ofw > @@ -0,0 +1,28 @@ > +What:/sys/firmware/ofw/../device-tree/ > +Date:March 2013 > +Contact: Grant Likely > +Description: > + When using OpenFirmware or a Flattened Device Tree to enumerate > + hardware, the device tree structure will be exposed in this > + directory. > + > + It is possible for multiple device-tree directories to exist. > + Some device drivers use a separate detached device tree which > + have no attachment to the system tree and will appear in a > + different subdirectory under /sys/firmware/ofw. > + > + Userspace must not use the /sys/firmware/ofw/../device-tree > + path directly, but instead should follow /proc/device-tree > + symlink. It is possible that the absolute path will change > + in the future, but the symlink is the stable ABI. > + > + The /proc/device-tree symlink replaces the devicetree /proc > + filesystem support, and has largely the same semantics and > + should be compatible with existing userspace. > + > + The contents of /sys/firmware/ofw/../device-tree is a > + hierarchy of directories, one per device tree node. The > + directory name is the resolved path component name (node > + name plus address). Properties are represented as files > + in the directory. The contents of each file is the exact > + binary data from the device tree. > diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig > index d37bfcf..d2d7be9 100644 > --- a/drivers/of/Kconfig > +++ b/drivers/of/Kconfig > @@ -38,7 +38,7 @@ config OF_PROMTREE > # Hardly any platforms need this. It is safe to select, but only do so if > you > # need it. > co
Re: pci and pcie device-tree binding - range No cells
On Mon, 2012-12-10 at 21:43 +, Grant Likely wrote: > > Sorry for my pci ignorance (have never got hw for mb/zynq) > > I just want to get better overview how we should we our drivers to > be compatible. > > > > Does it mean that pci is supposed be always 64 bit wide? > > And there is no option to have just 32bit values. > > Yes, PCIe addressing is always 64 bits wide. Even on 32bit PCI systems > we use 64 bit PCI addressing in the device tree. Right. The size & format of an address cell for PCI is specified in the OF PCI bindings and we follow that binding. It's always 3 cells. Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: Deprecating reserve-map in favor of properties
On Thu, 2012-11-01 at 15:21 +0100, Grant Likely wrote:
> I think this makes sense. Cyril and I are just talking about what he
> needs. He wants to set aside per-device reserved regions and would
> like to have the ability to reference a particular reserved region
> from a device node, probably with a phandle. I like the look of the
> reserved-{ranges,names} properties in the root, but I see the argument
> that it isn't very flexible. What about something like this:
>
> reserved-memory {
> reserved@0x1000 { reg = <0x1000 0x0800>; };
> reserved@0x0100 { reg = <0x0100 0x0020>; };
> }
>
> The node name of the child nodes could be different of course.
I'm not that fan of different nodes, especially nodes with nodes in them
for that purpose. Seems overkill.
Can't he reference reserved entries as , pairs ?
I still think a single property would do fine. We could mandate those be
in the respective "memory" nodes but them you have potentially to break
up reserved regions if you have multiple memory nodes (NUMA) etc...
I'd vote for the simpler approach here...
> > The idea here is that we could have well defined names (using a similar
> > prefix we use for properties) such as linux,initrd, which indicates
> > clearly to the kernel that the only reason that range is reserved is
> > because it contains an initrd (ie, it can be freed once that's been
> > extracted).
> >
> > It would also be generally handy in case a reserved area is meant to be
> > used by a specific driver, such as an in-memory framebuffer
> > pre-initialized by the firmware. The generic memory management code
> > doesn't need to know, but later on, the gfx driver can pick it up easily
> > provided the name is part of the binding for that device.
>
> Right, that would work also even though I prefer phandle references in
> general. Is it conceivable that additional data would want to be
> attached to a particular reserved region?
phandle references and names aren't exclusive from each other. The name
remains a useful diagnostic tool.
If you want additional data, make a node somewhere to represent that
region along with its additional data, that node can have a reference to
the reserved map entry.
I'd keep the reserve map itself simple. It's a "synthetic property" a
bit like the memory nodes. IE. The "memory" nodes don't have to
represent physical memory controllers & DIMMs. They represent the
overall view of memory by the CPUs. You can represent the MCs and the
DIMMs elsewhere in the SoC node.
Cheers,
Ben.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
[PATCH] of/fdt: Don't copy garbage after "/" in root node path
The root node path must be internally converted to "/", or various
pieces of code looking for it that way will fail. The code to do
that however had a bug where we might incorrectly append pieces
of the original path from the fdt to the "/".
We should probably add a proper dedicated accessor for the root node
but in the meantime this patch should fix it.
Signed-off-by: Benjamin Herrenschmidt
---
drivers/of/fdt.c |1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 91a375f..c2b08dc 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -186,6 +186,7 @@ static unsigned long unflatten_dt_node(struct
boot_param_header *blob,
*/
fpsize = 1;
allocl = 2;
+ l = 0;
} else {
/* account for '/' and path size minus terminal 0
* already in 'l'
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: adding OF_DYNAMIC proc interface
On Fri, 2012-09-28 at 11:46 -0500, Alan Tull wrote: > Hello, > > The following patch adds a /proc/ofdt interface to add or remove device tree > nodes dynamically. > > Based on earlier feedback, I've changed my driver to use /proc instead of > creating a new ioctl (the old thread is at > http://www.mail-archive.com/devicetree-discuss@lists.ozlabs.org/msg17333.html) > > > I was hoping to get some early feedback from others who might be interested > who were discussing this on an earlier thread about OF_DYNAMIC usage. > > This code doesn't do any notification for drivers yet. It can add multiple > nodes and they will show up properly under /proc/device-tree. It has an > issue that shows up when removing nodes (it appears that the memory used by > proc gets corrupted after the add). (Adding Arnd here) Have you guys considered whether a better approach would be a file system ? IE, create a node by creating a directory, add files for properties etc... ? It might need some trick to make the node "active" (in order to not internally in the kernel start exposing unfinished nodes), maybe a special file, maybe a permission trick ... It should be at least considered rather than a new interface that tries to re-implement semantics (such as notification) that are pretty much already provided by a fs. Also, what about the existing /proc/device-tree ? Finally, if we are doing something new and for some reason not a fs, what about using sysfs rather than /proc ? Really /proc is not a great idea here. Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: Deprecating reserve-map in favor of properties
On Thu, 2012-09-20 at 20:35 -0500, Kumar Gala wrote: > If you do this, please update the code in dtc/libfdt to construct the > new nodes. We use this in u-boot to reserve kernel, dtb, initrd, etc > regions. So would be nice to have drop in replacement code that could > use same APIs if possible. The kernel would of course still understand the reserve map and I don't intend to remove it from the header immediately, so I think that can stay unless we make it a function of the version. It's non-trivial to make the same (stateless) API in libfdt deal with setting properties. I'd rather avoid that problem initially by not changing the blob format, keeping the reserve map around for "legacy" purposes and simply making the kernel capable of understanding the properties. Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Deprecating reserve-map in favor of properties
Hi folks ! The reserve map is, imho, my biggest mistake when coming up with the FDT format. The main problem is that it doesn't survive the transition via a real Open Firmware interface. There is no practical way to indicate reserved regions of memory accross in that case, unless you have an OS that is nice enough to try to keep OF alive and accomodate its advertised "available" properties, but that's typically not the case of Linux on ppc. So I would like to propose that we add a new "better" way to convey reserved memory information, via properties in the tree. I originally though of having these in the "memory" nodes themselves but this can be tricky on machines that have multiple nodes (ie, NUMA generally means a memory node per NUMA node) since the reserved regions can spawn accross nodes and I don't want to complicate FW life too much by requiring breaking them up in that case. So what about something at the root of the tree: reserved-ranges: An array of ranges of reserved memory reserved-names: A list of zero terminated strings, one for each entry in the reserved-ranges array, providing optional "names" for the reserved ranges. The idea here is that we could have well defined names (using a similar prefix we use for properties) such as linux,initrd, which indicates clearly to the kernel that the only reason that range is reserved is because it contains an initrd (ie, it can be freed once that's been extracted). It would also be generally handy in case a reserved area is meant to be used by a specific driver, such as an in-memory framebuffer pre-initialized by the firmware. The generic memory management code doesn't need to know, but later on, the gfx driver can pick it up easily provided the name is part of the binding for that device. Any objection ? If none, I'll cook up a patch to add support for it (at least on powerpc :-) Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.
On Mon, 2012-09-10 at 14:22 +, Arnd Bergmann wrote: > Following up on the old discussion, I talked briefly about this > issue with BenH at the kernel summit. The outcome basically is that > it's a bit sad to have incompatible bindings, but it's not the end > of the world,and it's more important to do it right this time. > > Just make sure that you use different values for the 'compatible' > strings and then do what you need to get the ARM hardware working. > > Ideally, the new binding should be written in a way that powerpc > machines can use the same one, but the existing ones all use > an version of Open Firmware that is not going to get updated > and it's also not too likely that we are going to see new > powerpc machines based on this chip. Right, mostly these machines where the Pegasos. Those came with a fairly busted variant of Open Firmware which generated a pretty gross device-tree. For some reason, the manufacturer of those things was never willing to fix anything in their firmware (despite the distributor providing patches etc...), seemingly on the assumption that whatever they were doing was perfect and operating system people like us didn't matter one little bit :-) So I don't care much about it. It would be nice to keep them working since people in the community still have them but if it goes through some "compat" code that detects old/broken device-trees and eventually disappears when we finally drop support, then so be it. Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] powerpc/powernv: move the dereference below the NULL test
On Fri, 2012-09-07 at 14:45 +0800, Wei Yongjun wrote:
> From: Wei Yongjun
>
> The dereference should be moved below the NULL test.
>
> spatch with a semantic match is used to found this.
> (http://coccinelle.lip6.fr/)
I haven't applied this patch yet (there was a similar one recently from
another semantic checker I believe) because that code is about to be
deeply reworked (waiting for some dependencies to get in), so this will
just make the patch harder to apply, and the stuff should never be NULL
in the first place anyway.
So let's leave that aside for a bit.
Cheers,
Ben.
> Signed-off-by: Wei Yongjun
> ---
> arch/powerpc/platforms/powernv/pci.c | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/pci.c
> b/arch/powerpc/platforms/powernv/pci.c
> index be3cfc5..4ba89c1 100644
> --- a/arch/powerpc/platforms/powernv/pci.c
> +++ b/arch/powerpc/platforms/powernv/pci.c
> @@ -287,13 +287,15 @@ static int pnv_pci_read_config(struct pci_bus *bus,
> int where, int size, u32 *val)
> {
> struct pci_controller *hose = pci_bus_to_host(bus);
> - struct pnv_phb *phb = hose->private_data;
> + struct pnv_phb *phb;
> u32 bdfn = (((uint64_t)bus->number) << 8) | devfn;
> s64 rc;
>
> if (hose == NULL)
> return PCIBIOS_DEVICE_NOT_FOUND;
>
> + phb = hose->private_data;
> +
> switch (size) {
> case 1: {
> u8 v8;
> @@ -331,12 +333,14 @@ static int pnv_pci_write_config(struct pci_bus *bus,
> int where, int size, u32 val)
> {
> struct pci_controller *hose = pci_bus_to_host(bus);
> - struct pnv_phb *phb = hose->private_data;
> + struct pnv_phb *phb;
> u32 bdfn = (((uint64_t)bus->number) << 8) | devfn;
>
> if (hose == NULL)
> return PCIBIOS_DEVICE_NOT_FOUND;
>
> + phb = hose->private_data;
> +
> cfg_dbg("pnv_pci_write_config bus: %x devfn: %x +%x/%x -> %08x\n",
> bus->number, devfn, where, size, val);
> switch (size) {
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: OF_DYNAMIC usage
On Fri, 2012-07-06 at 10:02 +0200, Michal Simek wrote:
> ok. How that FDT blob segment should look like?
> It can't be just one node because it also requires path where it is connected.
>
> It means at least the part like below for injecting.
No, my idea was to pass 3 arguments:
- action (enum)
- path (string)
- segment (blob)
action would be typically add/remove
* add: add node under whose content is in
* remove: remove node and all children
(or we could require children removal to be done explicitly).
Cheers,
Ben.
> /dts-v1/;
> / {
> #address-cells = <1>;
> #size-cells = <1>;
> compatible = "xlnx,microblaze";
>
> mb_plb: plb@0 {
> #address-cells = <1>;
> #size-cells = <1>;
> compatible = "xlnx,plb-v46-1.00.a", "simple-bus";
> DIP_Switches_4Bit: gpio@8144 {
> #gpio-cells = <2>;
> compatible = "xlnx,xps-gpio-1.00.a";
> gpio-controller ;
> reg = < 0x8144 0x1 >;
> /* ... */
> } ;
> } ;
> };
>
> Not sure if this can be used for removing. I mean if you want to remove node
> if make sense to pass the whole node.
>
>
> Thanks,
> Michal
>
>
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: OF_DYNAMIC usage
On Fri, 2012-07-06 at 08:51 +0200, Michal Simek wrote: > On 07/06/2012 08:19 AM, Benjamin Herrenschmidt wrote: > > On Fri, 2012-07-06 at 08:03 +0200, Michal Simek wrote: > >>> > >>> The way it works at the moment is that when something new is plugged in, > >>> the hypervisor talks to a proprietary crap daemon in userspace which > >>> talks to a special tool (that one we have the source code) which then > >>> obtains via some FW interfaces a "blob" of bits of device-tree to add > >>> (or to remove), using a phyp specific format, and echo that stuff > >>> into /proc/ppc64/ofdt. > >> > >> Where is that source code for the special tool? > > > > I can dig that for you, however ... > > > >> Can you point me to the "phyp specific format"? > > > > Same deal, I don't think there's a public doc, however.. > > Why is it in the kernel something which does not have any public > documentation? > It seems like that it is more proprietary code. Don't ask me, it's been there since day 1 of the ppc64 port afaik, before I was involved in it. I've been wanting to deprecate that interface for a while, but haven't got to it yet. > > > >>From reconfig.c it looks like that there are some key words like > >> add_node/remove_node/add_property... follow by space and node name + > >> options which lookes like dtb format. > > > > Right, I would just recommend you don't do that. > > > > The need to "hotplug" bits of device-tree is going to be generic enough > > that we should come up with something better and more generic than that > > pseries stuff. > > > > IE. Some way to pass bits of ftb blob rather than this specific format > > to begin with, etc... > > Can we discuss this a little bit more? Sure :-) > From my partial reconfiguration understanding is that you have partial > bitstream > which you pass through pcap(IP and driver which can do it) to specific > location > which they are known. > > It means you have to unplug device which is in the same location, > load partial bitstream via pcap > register driver and probe it. > > I expect that pcap driver could be aware which driver is at that location > and is able to plug and unplug it based on description. > > I will ask Xilinx how this is exactly done and I hope I will get any example > to be able to do some experiments. > > > > > So I'd say just ignore the pseries stuff, I can dig the tool etc... if > > you -really- need them but I'd rather you don't base your stuff on it, > > just make up something better& more generic for you. It will be useful > > to others. > > My point here is just to try to understand current working version > to get the better overview how it is done and probably working somehow. > > Of course some ideas how this can/should be done will be helpful. Well, I think the right way is to have some mechanism (TBD) to be able to inject into the kernel a bit of device-tree (or remove a bit of device-tree). Ideally by passing it an FDT blob segment which is a well known & documented format. The kernel would then expand it and call some notifiers which we can use to create platform devices if needed etc... Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: OF_DYNAMIC usage
On Fri, 2012-07-06 at 08:03 +0200, Michal Simek wrote: > > > > The way it works at the moment is that when something new is plugged in, > > the hypervisor talks to a proprietary crap daemon in userspace which > > talks to a special tool (that one we have the source code) which then > > obtains via some FW interfaces a "blob" of bits of device-tree to add > > (or to remove), using a phyp specific format, and echo that stuff > > into /proc/ppc64/ofdt. > > Where is that source code for the special tool? I can dig that for you, however ... > Can you point me to the "phyp specific format"? Same deal, I don't think there's a public doc, however.. > From reconfig.c it looks like that there are some key words like > add_node/remove_node/add_property... follow by space and node name + > options which lookes like dtb format. Right, I would just recommend you don't do that. The need to "hotplug" bits of device-tree is going to be generic enough that we should come up with something better and more generic than that pseries stuff. IE. Some way to pass bits of ftb blob rather than this specific format to begin with, etc... So I'd say just ignore the pseries stuff, I can dig the tool etc... if you -really- need them but I'd rather you don't base your stuff on it, just make up something better & more generic for you. It will be useful to others. Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: OF_DYNAMIC usage
On Thu, 2012-07-05 at 08:20 -0500, Rob Herring wrote: > On 07/05/2012 07:21 AM, Michal Simek wrote: > > Hi, > > > > can someone tell me usage of OF_DYNAMIC? > > How this can be used by user? > > > > Is it possible to exchange device node? > > Add new one, delete one, etc? > > > > Any user guide/log will be helpful to see what I can do with it. > > 2nd question on OF_DYNAMIC in a week... I haven't really looked at it. I > would suggest looking at the existing users (only powerpc iseries and > pseries). My concern using would be that the reference counting is not > correct for many drivers as of_node_get/put is a nop without OF_DYNAMIC. > > You can also activate/deactivate nodes with the status property. > No doc that I know of. It's used by pseries only (iseries is gone btw) for "DLPAR" (aka Dynamic Reconfiguration aka hotplug). The way it works at the moment is that when something new is plugged in, the hypervisor talks to a proprietary crap daemon in userspace which talks to a special tool (that one we have the source code) which then obtains via some FW interfaces a "blob" of bits of device-tree to add (or to remove), using a phyp specific format, and echo that stuff into /proc/ppc64/ofdt. It's pretty fugly... Because nodes can come and go, we thus add refcounting. There's various places where the refcounting is not quite right but mostly things that don't get plugged/unplugged, actual vio devices or PCI devices tend to get it right. Also we don't have good rules for properties lifetime, as it is, it's assumed to be the same as the node and there's no proper way to add/remove/modify a property "live". I would recommend at least something RCUish if you're going to try to be safe to delay the freeing of the old value in the dynamic case. There's definitely room for improvements in that area... Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v2 1/1] of: reform prom_update_property function
On Mon, 2012-06-25 at 14:28 +0800, Dong Aisheng wrote:
> From: Dong Aisheng
>
> prom_update_property() currently fails if the property doesn't
> actually exist yet which isn't what we want. Change to add-or-update
> instead of update-only, then we can remove a lot duplicated lines.
>
> Suggested-by: Grant Likely
> Signed-off-by: Dong Aisheng
> ---
Grand, should I merge that via the powerpc tree or will you take care of
it ?
Acked-by: Benjamin Herrenschmidt
Cheers,
Ben.
> ChangeLog v1->v2:
> * keep reconfig.c behavior the same as before after changes
> ---
> arch/powerpc/platforms/85xx/p1022_ds.c|8 +---
> arch/powerpc/platforms/pseries/mobility.c |8 +---
> arch/powerpc/platforms/pseries/reconfig.c | 16 ++--
> drivers/of/base.c | 15 +++
> fs/proc/proc_devtree.c|5 +
> include/linux/of.h|3 +--
> 6 files changed, 25 insertions(+), 30 deletions(-)
>
> diff --git a/arch/powerpc/platforms/85xx/p1022_ds.c
> b/arch/powerpc/platforms/85xx/p1022_ds.c
> index f700c81..d66631c 100644
> --- a/arch/powerpc/platforms/85xx/p1022_ds.c
> +++ b/arch/powerpc/platforms/85xx/p1022_ds.c
> @@ -348,13 +348,7 @@ void __init p1022_ds_pic_init(void)
> */
> static void __init disable_one_node(struct device_node *np, struct property
> *new)
> {
> - struct property *old;
> -
> - old = of_find_property(np, new->name, NULL);
> - if (old)
> - prom_update_property(np, new, old);
> - else
> - prom_add_property(np, new);
> + prom_update_property(np, new);
> }
>
> /* TRUE if there is a "video=fslfb" command-line parameter. */
> diff --git a/arch/powerpc/platforms/pseries/mobility.c
> b/arch/powerpc/platforms/pseries/mobility.c
> index 029a562..dd30b12 100644
> --- a/arch/powerpc/platforms/pseries/mobility.c
> +++ b/arch/powerpc/platforms/pseries/mobility.c
> @@ -67,7 +67,6 @@ static int update_dt_property(struct device_node *dn,
> struct property **prop,
> const char *name, u32 vd, char *value)
> {
> struct property *new_prop = *prop;
> - struct property *old_prop;
> int more = 0;
>
> /* A negative 'vd' value indicates that only part of the new property
> @@ -117,12 +116,7 @@ static int update_dt_property(struct device_node *dn,
> struct property **prop,
> }
>
> if (!more) {
> - old_prop = of_find_property(dn, new_prop->name, NULL);
> - if (old_prop)
> - prom_update_property(dn, new_prop, old_prop);
> - else
> - prom_add_property(dn, new_prop);
> -
> + prom_update_property(dn, new_prop);
> new_prop = NULL;
> }
>
> diff --git a/arch/powerpc/platforms/pseries/reconfig.c
> b/arch/powerpc/platforms/pseries/reconfig.c
> index 7b3bf76..db1b7b1 100644
> --- a/arch/powerpc/platforms/pseries/reconfig.c
> +++ b/arch/powerpc/platforms/pseries/reconfig.c
> @@ -432,7 +432,7 @@ static int do_update_property(char *buf, size_t bufsize)
> unsigned char *value;
> char *name, *end, *next_prop;
> int rc, length;
> - struct property *newprop, *oldprop;
> + struct property *newprop;
> buf = parse_node(buf, bufsize, &np);
> end = buf + bufsize;
>
> @@ -443,6 +443,9 @@ static int do_update_property(char *buf, size_t bufsize)
> if (!next_prop)
> return -EINVAL;
>
> + if (!strlen(name)
> + return -ENODEV;
> +
> newprop = new_property(name, length, value, NULL);
> if (!newprop)
> return -ENOMEM;
> @@ -450,18 +453,11 @@ static int do_update_property(char *buf, size_t bufsize)
> if (!strcmp(name, "slb-size") || !strcmp(name, "ibm,slb-size"))
> slb_set_size(*(int *)value);
>
> - oldprop = of_find_property(np, name,NULL);
> - if (!oldprop) {
> - if (strlen(name))
> - return prom_add_property(np, newprop);
> - return -ENODEV;
> - }
> -
> upd_value.node = np;
> upd_value.property = newprop;
> pSeries_reconfig_notify(PSERIES_UPDATE_PROPERTY, &upd_value);
>
> - rc = prom_update_property(np, newprop, oldprop);
> + rc = prom_update_property(np, newprop);
> if (rc)
> return rc;
>
> @@ -486,7 +482,7 @@ static int do_update_property(char *buf, size_t bufsize)
>
> rc = pSeries_reconfig_notify(action, value);
> if (rc) {
>
Re: [PATCH 1/1] of: reform prom_update_property function
On Fri, 2012-06-22 at 13:25 +0800, Dong Aisheng wrote: > Which seems the same behavior as the new prop_update_property api. > The only different is if no name, return -EINVAL; > Am i wrong? No, you are right. Sorry for the noise :-) Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 1/1] of: reform prom_update_property function
On Thu, 2012-06-21 at 17:37 +0800, Dong Aisheng wrote:
> Maybe we could change it as as follows.
> It looks then the code follow is the same as before.
> Do you think if it's ok?
>
> diff --git a/arch/powerpc/platforms/pseries/reconfig.c
> b/arch/powerpc/platforms/pseries/reconfig.c
> index 7b3bf76..4c237f4 100644
> --- a/arch/powerpc/platforms/pseries/reconfig.c
> +++ b/arch/powerpc/platforms/pseries/reconfig.c
> @@ -443,6 +443,9 @@ static int do_update_property(char *buf, size_t bufsize)
> if (!next_prop)
> return -EINVAL;
>
> + if (!strlen(name)
> + return -ENODEV;
> +
> newprop = new_property(name, length, value, NULL);
> if (!newprop)
> return -ENOMEM;
> @@ -450,13 +453,6 @@ static int do_update_property(char *buf, size_t bufsize)
> if (!strcmp(name, "slb-size") || !strcmp(name, "ibm,slb-size"))
> slb_set_size(*(int *)value);
>
> - oldprop = of_find_property(np, name,NULL);
> - if (!oldprop) {
> - if (strlen(name))
> - return prom_add_property(np, newprop);
> - return -ENODEV;
> - }
No:
IE. Old code did:
if (property doesn't exist) {
if (has a name)
create_it()
return -ENODEV;
}
What you propose is:
if (!has_a_name)
return -ENODEV;
Not at all the same semantic.
.../...
> > IE. The allocation of the "old" property isn't disposed of. It can't
> > because today we don't know whether any of those pointers was
> > dynamically allocated or not. IE they could point to the fdt
> Hmm, i did not see static allocated property before.
> Where can we see an exist case?
Almost all your property names and values. They are pointers to the
original fdt block, so you can't free them. But dynamically added
propreties will have kmalloc'ed pointers which should be freed. We need
to add flags to indicate that if we want to avoid leaking memory in very
dynamic environments.
> If we really have this issue, it seems of_node_release also has the same
> issue,
> since it frees the property without distinguish whether the property is
> allocated
> dynamically.
Well, actually we do have a flag:
if (!of_node_check_flag(node, OF_DYNAMIC))
return;
So we use that. Good. So if update property uses that flag it should be
able to know when to free or not. I forgot we had that :-)
> > string list, data block, or could be bootmem ... or could be
> > actual kernel memory.
> >
> > We might want to extend the struct property to contain indications of
> > the allocation type so we can kfree dynamic properties properly.
> >
> I wonder the simplest way may be not allow static allocated property, like dt
> node does i guess.
No way. We generate the device-tree way before we have an allocator
available.
> > But then there's the question of the lifetime of a property... since
> > they aren't reference counted like nodes are.
> >
> Yes, that's a real exist problem.
>
> Anyway, i guess we could do that work of this problem in another patch
> rather than have to in this patch series.
Cheers,
Ben.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 1/1] of: reform prom_update_property function
On Wed, 2012-06-20 at 08:07 -0500, Rob Herring wrote:
> On 06/20/2012 12:54 AM, Dong Aisheng wrote:
> > From: Dong Aisheng
> >
> > prom_update_property() currently fails if the property doesn't
> > actually exist yet which isn't what we want. Change to add-or-update
> > instead of update-only, then we can remove a lot duplicated lines.
> >
> > Suggested-by: Grant Likely
> > Signed-off-by: Dong Aisheng
> > ---
>
> Looks fine, but you need to cc powerpc maintainers.
Generally fine, just one issue I spotted:
> > diff --git a/arch/powerpc/platforms/pseries/reconfig.c
> > b/arch/powerpc/platforms/pseries/reconfig.c
> > index 7b3bf76..4c92f1c 100644
> > --- a/arch/powerpc/platforms/pseries/reconfig.c
> > +++ b/arch/powerpc/platforms/pseries/reconfig.c
> > @@ -432,7 +432,7 @@ static int do_update_property(char *buf, size_t bufsize)
> > unsigned char *value;
> > char *name, *end, *next_prop;
> > int rc, length;
> > - struct property *newprop, *oldprop;
> > + struct property *newprop;
> > buf = parse_node(buf, bufsize, &np);
> > end = buf + bufsize;
> >
> > @@ -450,18 +450,11 @@ static int do_update_property(char *buf, size_t
> > bufsize)
> > if (!strcmp(name, "slb-size") || !strcmp(name, "ibm,slb-size"))
> > slb_set_size(*(int *)value);
> >
> > - oldprop = of_find_property(np, name,NULL);
> > - if (!oldprop) {
> > - if (strlen(name))
> > - return prom_add_property(np, newprop);
> > - return -ENODEV;
> > - }
> > -
Here the code would exit the function with an error if the property
didn't already exist, you are losing that semantic.
Additionally there's that oddball test for strlen(name) ... you might
want to check that somewhere... and the strange fact that it would
actually add the new property anyway despite returning an error which is
very very odd semantics but I'd rather not break them since this is
exposed to userspace, so we may have tools relying on them.
> > upd_value.node = np;
> > upd_value.property = newprop;
> > pSeries_reconfig_notify(PSERIES_UPDATE_PROPERTY, &upd_value);
> >
> > - rc = prom_update_property(np, newprop, oldprop);
> > + rc = prom_update_property(np, newprop);
> > if (rc)
> > return rc;
> >
> > @@ -486,7 +479,7 @@ static int do_update_property(char *buf, size_t bufsize)
> >
> > rc = pSeries_reconfig_notify(action, value);
> > if (rc) {
> > - prom_update_property(np, oldprop, newprop);
> > + prom_update_property(np, newprop);
> > return rc;
> > }
> > }
> > diff --git a/drivers/of/base.c b/drivers/of/base.c
> > index d9bfd49..a14f109 100644
> > --- a/drivers/of/base.c
> > +++ b/drivers/of/base.c
> > @@ -1051,7 +1051,8 @@ int prom_remove_property(struct device_node *np,
> > struct property *prop)
> > }
> >
> > /*
> > - * prom_update_property - Update a property in a node.
> > + * prom_update_property - Update a property in a node, if the property does
> > + * not exist, add it.
> > *
> > * Note that we don't actually remove it, since we have given out
> > * who-knows-how-many pointers to the data using get-property.
> > @@ -1059,13 +1060,19 @@ int prom_remove_property(struct device_node *np,
> > struct property *prop)
> > * and add the new property to the property list
> > */
> > int prom_update_property(struct device_node *np,
> > -struct property *newprop,
> > -struct property *oldprop)
> > +struct property *newprop)
> > {
> > - struct property **next;
> > + struct property **next, *oldprop;
> > unsigned long flags;
> > int found = 0;
> >
> > + if (!newprop->name)
> > + return -EINVAL;
> > +
> > + oldprop = of_find_property(np, newprop->name, NULL);
> > + if (!oldprop)
> > + return prom_add_property(np, newprop);
> > +
> > write_lock_irqsave(&devtree_lock, flags);
> > next = &np->properties;
> > while (*next) {
> > diff --git a/fs/proc/proc_devtree.c b/fs/proc/proc_devtree.c
> > index 927cbd1..df7dd08 100644
> > --- a/fs/proc/proc_devtree.c
> > +++ b/fs/proc/proc_devtree.c
> > @@ -101,6 +101,11 @@ void proc_device_tree_update_prop(struct
> > proc_dir_entry *pde,
> > {
> > struct proc_dir_entry *ent;
> >
> > + if (!oldprop) {
> > + proc_device_tree_add_prop(pde, newprop);
> > + return;
> > + }
> > +
> > for (ent = pde->subdir; ent != NULL; ent = ent->next)
> > if (ent->data == oldprop)
> > break;
> > diff --git a/include/linux/of.h b/include/linux/of.h
> > index 2ec1083..b27c871 100644
> > --- a/include/linux/of.h
> > +++ b/include/linux/of.h
> > @@ -260,8 +260,7 @@ extern int of_machine_is_compatible(const char *compat);
> > extern int prom_add_property(struct device_node* np, struct property*
> > prop);
> > extern int prom_remove_property(struct device_node *
Re: [PATCH] devicetree: add helper inline for retrieving a node's full name
On Fri, 2012-06-15 at 22:00 -0600, Grant Likely wrote: > On Sat, 16 Jun 2012 08:57:50 +1000, Benjamin Herrenschmidt > wrote: > > On Fri, 2012-06-15 at 11:50 -0600, Grant Likely wrote: > > > The pattern (np ? np->full_name : "") is rather common in the > > > kernel, but can also make for quite long lines. This patch adds a new > > > inline function, of_node_full_name() so that the test for a valid node > > > pointer doesn't need to be open coded at all call sites. > > > > s/of_node_full_name/of_node_path ? > > It would be nicer to have the shorter name, but the data member is > already named full_name, so I'd rather use the same for the function > name (unless you argue strongly against, it's not something I'm going > to fight about). Nah, it's just that full_name was never a very good choice to begin with ... Do as you wish. Renaming the data member might be a bit annoying I can immagine :-) Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] devicetree: add helper inline for retrieving a node's full name
On Fri, 2012-06-15 at 11:50 -0600, Grant Likely wrote:
> The pattern (np ? np->full_name : "") is rather common in the
> kernel, but can also make for quite long lines. This patch adds a new
> inline function, of_node_full_name() so that the test for a valid node
> pointer doesn't need to be open coded at all call sites.
s/of_node_full_name/of_node_path ?
Cheers,
Ben.
> Signed-off-by: Grant Likely
> Cc: Paul Mundt
> Cc: Benjamin Herrenschmidt
> Cc: Thomas Gleixner
> ---
> arch/microblaze/pci/pci-common.c |6 ++
> arch/powerpc/kernel/pci-common.c |6 ++
> arch/powerpc/kernel/vio.c |5 ++---
> arch/powerpc/platforms/cell/iommu.c|3 +--
> arch/powerpc/platforms/pseries/iommu.c |2 +-
> arch/sparc/kernel/of_device_64.c |2 +-
> drivers/of/base.c |2 +-
> drivers/of/irq.c |2 +-
> include/linux/of.h | 10 ++
> kernel/irq/irqdomain.c |8
> 10 files changed, 25 insertions(+), 21 deletions(-)
>
> diff --git a/arch/microblaze/pci/pci-common.c
> b/arch/microblaze/pci/pci-common.c
> index ed22bfc..ca8f6e7 100644
> --- a/arch/microblaze/pci/pci-common.c
> +++ b/arch/microblaze/pci/pci-common.c
> @@ -249,8 +249,7 @@ int pci_read_irq_line(struct pci_dev *pci_dev)
> } else {
> pr_debug(" Got one, spec %d cells (0x%08x 0x%08x...) on %s\n",
>oirq.size, oirq.specifier[0], oirq.specifier[1],
> - oirq.controller ? oirq.controller->full_name :
> - "");
> + of_node_full_name(oirq.controller));
>
> virq = irq_create_of_mapping(oirq.controller, oirq.specifier,
>oirq.size);
> @@ -1493,8 +1492,7 @@ static void __devinit pcibios_scan_phb(struct
> pci_controller *hose)
> struct pci_bus *bus;
> struct device_node *node = hose->dn;
>
> - pr_debug("PCI: Scanning PHB %s\n",
> - node ? node->full_name : "");
> + pr_debug("PCI: Scanning PHB %s\n", of_node_full_name(node));
>
> pcibios_setup_phb_resources(hose, &resources);
>
> diff --git a/arch/powerpc/kernel/pci-common.c
> b/arch/powerpc/kernel/pci-common.c
> index 8e78e93..886c254 100644
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -248,8 +248,7 @@ static int pci_read_irq_line(struct pci_dev *pci_dev)
> } else {
> pr_debug(" Got one, spec %d cells (0x%08x 0x%08x...) on %s\n",
>oirq.size, oirq.specifier[0], oirq.specifier[1],
> - oirq.controller ? oirq.controller->full_name :
> - "");
> + of_node_full_name(oirq.controller));
>
> virq = irq_create_of_mapping(oirq.controller, oirq.specifier,
>oirq.size);
> @@ -1628,8 +1627,7 @@ void __devinit pcibios_scan_phb(struct pci_controller
> *hose)
> struct device_node *node = hose->dn;
> int mode;
>
> - pr_debug("PCI: Scanning PHB %s\n",
> - node ? node->full_name : "");
> + pr_debug("PCI: Scanning PHB %s\n", of_node_full_name(node));
>
> /* Get some IO space for the new PHB */
> pcibios_setup_phb_io_space(hose);
> diff --git a/arch/powerpc/kernel/vio.c b/arch/powerpc/kernel/vio.c
> index cb87301..63f72ed 100644
> --- a/arch/powerpc/kernel/vio.c
> +++ b/arch/powerpc/kernel/vio.c
> @@ -1296,8 +1296,7 @@ static void __devinit vio_dev_release(struct device
> *dev)
> struct iommu_table *tbl = get_iommu_table_base(dev);
>
> if (tbl)
> - iommu_free_table(tbl, dev->of_node ?
> - dev->of_node->full_name : dev_name(dev));
> + iommu_free_table(tbl, of_node_full_name(dev->of_node));
> of_node_put(dev->of_node);
> kfree(to_vio_dev(dev));
> }
> @@ -1509,7 +1508,7 @@ static ssize_t devspec_show(struct device *dev,
> {
> struct device_node *of_node = dev->of_node;
>
> - return sprintf(buf, "%s\n", of_node ? of_node->full_name : "none");
> + return sprintf(buf, "%s\n", of_node_full_name(of_node));
> }
>
> static ssize_t modalias_show(struct device *dev, struct device_attribute
> *attr,
> diff --git a/arch/powerpc/platforms/cell/iommu.c
> b/arch/powerpc/platforms/cell/iommu.c
> index b9f509a..b673200 100644
> --- a/arch/powerpc/plat
Re: [PATCH v5 06/27] irq_domain/powerpc: eliminate irq_map; use irq_alloc_desc() instead
On Wed, 2012-04-11 at 14:57 -0600, Grant Likely wrote: > > Yeah, I've got a different way to fix it though. There is exactly one > user of irq_virq_count in-tree right now: PS3. Also, irq_virq_count > is only useful for the NOMAP mapping. So, instead of having a single > global irq_virq_count values, I've dropped it entirely and added a > max_irq argument to irq_domain_add_nomap(). That makes it a property > of an individual nomap irq domain instead of a global system settting. > > Hopefully I'll have a draft patch ready today. That works for me. I'll send patches for cleanup MPIC as well. One thing tho (Thomas, Russell) is that I like using set_irq_trigger to establish the "defaults" in mpic, ie, it does the descriptor locking etc... for me, I'd rather avoid open coding all of that. What I need is a "variant" that doesn't actually change the trigger but instead initializes the irq_desc with whatever settings the HW currently has (ie, I need to make sure things are properly in sync) though other implementations may want to use that for defaults. Any objection to defining something like IRQ_TYPE_DEFAULT ? I was thinking about making it equal to IRQ_TYPE_SENSE_MASK since that can obviously not be a valid trigger value and is distinct from IRQ_TYPE_NONE. Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v5 06/27] irq_domain/powerpc: eliminate irq_map; use irq_alloc_desc() instead
On Wed, 2012-04-11 at 11:33 +1000, Benjamin Herrenschmidt wrote: > On Wed, 2012-04-11 at 11:13 +1000, Benjamin Herrenschmidt wrote: > > On Sat, 2012-04-07 at 14:27 +0200, Andreas Schwab wrote: > > > Benjamin Herrenschmidt writes: > > > > > > > It's arguable that this irq_set_irq_type(,NONE) shouln't be there but > > > > still ... it's been around for ever and things worked :-) So something > > > > -else- is causing the problem and I'd like to understand what exactly. > > > > > > AFAICS before a09b659cd68c10ec6a30cb91ebd2c327fcd5bfe5 > > > irq_set_irq_type(,NONE) was actually a no-op. > > > > So I'm still a bit baffled... ie, I understand some of what's happening > > but not why it breaks things, I haven't yet managed to reproduce but I > > haven't tried too hard just yet (was away from the HW) : > > Allright, I have a repro-case, I'll dig. Ok, so it's Grant's fault :-) So basically, it's quite subtle and I'm only 99% sure of the details but I believe what happens is: - The audio interrupts get virq 64 and 65 (so above NR_IRQS) - The reverse map isn't pre-filled at map time (we should probably do it nowadays), we do it lazily so ... - The audio interrupt happens, it tries to pre-fill the reverse map and ... fails (see below) - This cause irq_linear_revmap() to returns 0 - This hits another bug in mpic where when that happens it doesn't EOI the interrupt, which means the priority remains stuck high and we don't get any other interrupt. There's thus two bugs here: - We don't properly establish the reverse mapping for 64 and 65 (well, not always, again see below) - We don't EOI an interrupt we can't reverse map (we should warn & EOI, I'll send a patch for that). The second problem is a bug we shouldn't hit if the first one didn't happen, the first one comes from way irq_find_mapping() works. Basically when the revmap entry is 0, we call it to find the mapping & populate the revmap... and that fails. That fails because it will only search up to irq_virq_count which is statically initialized to NR_IRQS, which in our case is 64 so it won't find our interrupts 64 and 65... The reason it works if you don't do the set_type(NONE) is a fluke, it will crash as soon as you actually do audio: The set_type(NONE) has the effect in mpic to configure the interrupt to level low (default). This is also the idle state of the audio interrupt (which is positive edge), and the MPIC appears to be latching it (yeah odd, it does seem to latch level interrupts). So as soon as the audio driver enables it, it fires, triggering the bug. Without the set_type(NONE) the occurence of the bug is delayed to the fist time you get a real audio interrupt. Now what is the solution ? Well, there's several things I think we need to do: - MPIC should properly EOI interrupts it can't revmap (I'll fix that) - MPIC should probably not do the set_type(NONE) on map, it's not useful - However, we do have a risk of discrepency between the default trigger type in the irq_desc vs. the default HW value at startup/map time, so we do need to do something to reconcile them (that was the intend of NONE I think)... without that, we risk having irq_create_of_mapping() not setting the trigger because it thinks it thinks it's right... What do you think is best here ? We can probably read the HW config and sync the irq desc appropriately ... - The whole business with irq_virq_count needs fixing. Basically the default value shouldn't be NR_IRQ. I suggest making it 0 and have all the use sites do something like: max = irq_virq_count ? irq_virq_count : nr_irqs; (Grant, can you take care of that ?) - We still need to clear up all the NR_IRQS occurences in arch/powerpc Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v5 06/27] irq_domain/powerpc: eliminate irq_map; use irq_alloc_desc() instead
On Wed, 2012-04-11 at 11:13 +1000, Benjamin Herrenschmidt wrote: > On Sat, 2012-04-07 at 14:27 +0200, Andreas Schwab wrote: > > Benjamin Herrenschmidt writes: > > > > > It's arguable that this irq_set_irq_type(,NONE) shouln't be there but > > > still ... it's been around for ever and things worked :-) So something > > > -else- is causing the problem and I'd like to understand what exactly. > > > > AFAICS before a09b659cd68c10ec6a30cb91ebd2c327fcd5bfe5 > > irq_set_irq_type(,NONE) was actually a no-op. > > So I'm still a bit baffled... ie, I understand some of what's happening > but not why it breaks things, I haven't yet managed to reproduce but I > haven't tried too hard just yet (was away from the HW) : Allright, I have a repro-case, I'll dig. Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v5 06/27] irq_domain/powerpc: eliminate irq_map; use irq_alloc_desc() instead
On Sat, 2012-04-07 at 14:27 +0200, Andreas Schwab wrote: > Benjamin Herrenschmidt writes: > > > It's arguable that this irq_set_irq_type(,NONE) shouln't be there but > > still ... it's been around for ever and things worked :-) So something > > -else- is causing the problem and I'd like to understand what exactly. > > AFAICS before a09b659cd68c10ec6a30cb91ebd2c327fcd5bfe5 > irq_set_irq_type(,NONE) was actually a no-op. So I'm still a bit baffled... ie, I understand some of what's happening but not why it breaks things, I haven't yet managed to reproduce but I haven't tried too hard just yet (was away from the HW) : Basically, we end up setting the mpic to IRQ_TYPE_LEVEL_LOW as a result of a request to set it to IRQ_TYPE_NONE, ie, we apply a "default" setting instead of just setting it to "none" (in part because we don't have a way in HW to disable the trigger completely. We then write that into the irq_desc with irqd_set_trigger_type(). Then we return IRQ_SET_MASK_OK_NOCOPY, which means that the generic __irq_set_trigger() code will read back what we've set in the desc and adjust things accordingly, rather than writing the original value in, ie, our descriptor ends up being effectively set to IRQ_TYPE_LEVEL_LOW rather than IRQ_TYPE_NONE. Later, when we actually try to set it to IRQ_TYPE_LEVEL_LOW as a result of parsing the device-tree, we then hit the code irq_create_of_mapping(), at the bottom, which will skip the call to irq_set_irq_type() if we are trying to set it to the same type that it's already set to But I don't see why that breaks anything, ie we -have- set things to LEVEL_LOW as a result of IRQ_TYPE_NONE, which is the right setting, so things should work despite the fact that we skip the second call. Out of curiosity, can you try removing the check in irqdomain and always apply the setting regardless of the previous value (ie, remove the check of type against irqd_get_trigger at the end of irq_create_of_mapping), see if that makes a difference ? Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v5 06/27] irq_domain/powerpc: eliminate irq_map; use irq_alloc_desc() instead
On Fri, 2012-04-06 at 13:51 +0200, Andreas Schwab wrote: > Benjamin Herrenschmidt writes: > > > Ok. Doesn't matter anyway, this shouldn't be the problem in this > > specific case. IE. we shouldn't be setting that interrupt to NONE. > > After removinge the call to irq_set_irq_type in mpic_host_map the irq > problem disappears. > > Instead of the irq_set_irq_type(,NONE) calls from mpic_host_map I see > corresponding irq_set_irq_type(,LEVEL_LOW) calls now. It's arguable that this irq_set_irq_type(,NONE) shouln't be there but still ... it's been around for ever and things worked :-) So something -else- is causing the problem and I'd like to understand what exactly. (When I say arguable, I do mean it. There are some reasons to keep it even if we agree that it's not "setting a default" but marking the interrupt as somewhat disabled as Russell wants, that's fine with me, an MPIC external interrupt should -always- have a proper type set before being used). First we need to fix that business with NR_IRQS/nr_irqs everywhere, then if the problem persists, check why we aren't calling the proper irq_set_irq_type() after the mapping is established (we should be). Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v5 06/27] irq_domain/powerpc: eliminate irq_map; use irq_alloc_desc() instead
On Tue, 2012-04-03 at 14:11 +0200, Andreas Schwab wrote: > > When reconfiguring with NR_IRQS=128 interrupts are working again, but I > still see a lot of spurious interrupts, and the X server is still broken > (no input works, but I still don't know whether that is an unrelated > bug). I have an idea or two about the spurrious interrupt business, I'll have a look later today hopefully. As for the X server, I don't think it's related, but of course it's hard to tell. Do you see a relevant difference in the X log ? Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v5 06/27] irq_domain/powerpc: eliminate irq_map; use irq_alloc_desc() instead
On Mon, 2012-04-02 at 18:29 +0200, Andreas Schwab wrote:
> > This breaks irqs on PowerMac G5. I see lost irq errors from the sata
> > driver.
>
> When I revert a09b659cd68c10ec6a30cb91ebd2c327fcd5bfe5 ("genirq: Fix
> long-term regression in genirq irq_set_irq_type() handling") on top of
> 3.4-rc1 the sata irq errors disappear, but I see a lot of spurious
> interrupts. Also the X server is broken somehow, though I don't know
> whether that is related or a different bug.
I can't explain why but it works for me with your config on a similar
dual G5.
It would be interesting to figure out what causes the call to
set_irq_type(NONE)...
The SATA driver isn't doing anything. What should be happening
is that:
- pci_read_irq_line() in arch/powerpc/kernel/pci-common.c
This will call of_irq_map_pci() and then call irq_of_create_mapping()
with the result if it's successful (it should be).
- irq_of_create_mapping in kernel/irq/irqdomain.c
That should then call domain->ops->xlate() where domain is the domain
of the first MPIC (the one in K2), which should return your IRQ number
for the SATA controller (btw, it's 0, in case that matters, ie, HW irq 0
on MPIC 1 iirc) and the interrupt type. This is where you should
get IRQ_TYPE_LEVEL_LOW (i verified and that's what my device-tree
contains).
You can add printk's in there to see what's happening in your case ?
Cheers,
Ben.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v5 06/27] irq_domain/powerpc: eliminate irq_map; use irq_alloc_desc() instead
On Mon, 2012-04-02 at 23:52 +0100, Russell King - ARM Linux wrote: > If we want to fix it a better way, then sure, that'll be good. But > what > we shouldn't do is re-introduce one regression to fix a different > regression. > > So, Thomas, what do you think about providing a way that a disabled > interrupt could have its pending status cleared such that when it's > enabled, any queued events are ignored? Maybe an > enable_and_clear_irq() ? Ok. Doesn't matter anyway, this shouldn't be the problem in this specific case. IE. we shouldn't be setting that interrupt to NONE. We should have parsed the device-tree and set the trigger appropriately. Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v5 06/27] irq_domain/powerpc: eliminate irq_map; use irq_alloc_desc() instead
On Mon, 2012-04-02 at 22:55 +0100, Russell King - ARM Linux wrote: > Well, presumably someone is calling irq_set_irq_type() asking explicitly > for IRQ_TYPE_NONE. The code will now (as it always used to before David's > change) do exactly what you ask this to: it will ask the type to be set > to none. > > If you don't want to set the type to none, don't call the interface asking > for that to happen. I think part of the idea with NONE is to use it as "reset that interrupt to the "default" setting, whatever that means ... Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v5 06/27] irq_domain/powerpc: eliminate irq_map; use irq_alloc_desc() instead
On Mon, 2012-04-02 at 23:27 +0200, Thomas Gleixner wrote: > So it's covered by this section: > > mpic_set_irq_type() > > if (flow_type == IRQ_TYPE_NONE) > if (mpic->senses && src < mpic->senses_count) > flow_type = mpic->senses[src]; > if (flow_type == IRQ_TYPE_NONE) > flow_type = IRQ_TYPE_LEVEL_LOW; > > And with IRQ_TYPE_NONE this is always going to end up with > IRQ_TYPE_LEVEL_LOW simply because the only function which might set > mpic->senses is mpic_set_default_senses(). And this function does not > have a single caller . > > /me scratches head Something must have broken ... when converting from the device-tree we should be calling xlate which should extract the polarity/sense from the device-tree. Now, afaik, IRQ_LEVEL_LOW is correct for PCI unless the interrupt is inverted inside Apple ASIC which is possible... (it's not an external PCI, it's a cell inside K2). Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v5 06/27] irq_domain/powerpc: eliminate irq_map; use irq_alloc_desc() instead
On Mon, 2012-04-02 at 22:52 +0200, Thomas Gleixner wrote:
> > When I revert a09b659cd68c10ec6a30cb91ebd2c327fcd5bfe5 ("genirq: Fix
> > long-term regression in genirq irq_set_irq_type() handling") on top
> of
> > 3.4-rc1 the sata irq errors disappear, but I see a lot of spurious
>
> Hmm. Which irq chip is handling the interrupt for that sata irq ?
MPIC. The irq is a PCI IRQ, and so should be level-low, it should have
been mapped by the PCI code using the of_* calls.
Cheers,
Ben.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v5 06/27] irq_domain/powerpc: eliminate irq_map; use irq_alloc_desc() instead
On Sun, 2012-04-01 at 23:27 +0200, Andreas Schwab wrote: > Grant Likely writes: > > > This patch drops the powerpc-specific irq_map table and replaces it with > > directly using the irq_alloc_desc()/irq_free_desc() interfaces for > > allocating > > and freeing irq_desc structures. > > This breaks irqs on PowerMac G5. I see lost irq errors from the sata > driver. > > Note that the breakage only started after the series was merged into > mainline. But when transplanted upon the parent of the merge this is > the commit that was blamed by bisect. So something changed between > v3.3-rc3 and the merge of this series that broke it. can you tell me more ? (What machine precisely, what .config etc...) ? I tested here on two G5's: - PowerMac7,3 aka Dual G5 AGP. This ones has U3 + K2 and uses cascaded MPICs - PowerMac11,2 aka Quad G5. This one has U4 + K2 and uses a single MPIC in the northbridge And both appear to work fine with today upstream (basically -rc1). Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: issues calling of_platform_bus_probe() twice
On Sat, 2012-03-17 at 13:35 +, Tabi Timur-B04825 wrote: > >>> > > Are you aware of any reason that we can't call > of_platform_bus_probe() > >>> > > or multiple times. Timur's run into an issue in which all > devices > >>> > > don't get registered properly if we call > of_platform_bus_probe() times > >>> > > with different of_device_id struct's. > >> > > >> > Nothing comes to mind... Grant ? > > Neither for me. Should work. > > I posted a work-around patch here: > > http://patchwork.ozlabs.org/patch/128533/ > > Without this patch, drivers cannot probe on DMA *channels*, or any > other > grandchildren of the root node. Why don't you track down the actual bug instead ? Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: issues calling of_platform_bus_probe() twice
On Fri, 2012-03-16 at 16:21 -0500, Kumar Gala wrote: > Guys, > > Are you aware of any reason that we can't call of_platform_bus_probe() > or multiple times. Timur's run into an issue in which all devices > don't get registered properly if we call of_platform_bus_probe() times > with different of_device_id struct's. Nothing comes to mind... Grant ? Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 1/1] of: add dma-mask binding
On Thu, 2012-03-08 at 17:59 -0700, Grant Likely wrote: > > > This would need some documentation. There is already "dma-ranges" > > > defined for OF which nay do what's needed. > > what is dma-ranges I see no doc about it > > but I don't think it could be used for the dma mask > > As Rob says, it's documented in ePAPR. dma-ranges is generally defined for bridges to indicate the dma window to system memory, it's not been by actual devices so far. It provides the opposite of "ranges", ie a translation from children to parent address space. So it's not quite the same thing. Now the fact that we are limited to power-of-2 masks is a Linux implementation detail. We might want to make the device-tree a bit more flexible and use something like dma-limit instead. However hardware limits tend to be in term of number of bits anyways so it's not a big issue. I don't see a need to keep a "ranges" type of semantic. If we ever do that we can do a new binding for it or add a dma-base... > We could merely add an empty dma_mask u64 to the containing struct > platform_device and then assign the pointer to dev.dma_mask if it is > needed. That would eliminate the malloc. Agreed. > Or we could by default > use dev->dev.dma_mask = &dev->dev.coherent_dma_mask; Drivers or > notifier code could override whatever we setup here as the default. > > dma-ranges really is the property that should be used for this, but > dma-ranges can describe a different set of permutations than dma_mask. I don't think so. As I said, dma-ranges is about a bridge/bus exposing DMA windows & translations. It has nothing to do with the device intrinsic DMA'ing capabilities. Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] of: Only compile OF_DYNAMIC on PowerPC pseries and iseries
On Tue, 2012-02-14 at 10:25 -0700, Grant Likely wrote: > Only two architectures use the OF node reference counting and reclaim bits. > There is no need to compile it for the rest of the PowerPC platforms or for > any of the other architectures. This patch makes iseries and pseries > select CONFIG_OF_DYNAMIC, and makes it default to off for everything else. > > It is still safe to turn on CONFIG_OF_DYNAMIC on all architectures, it just > isn't necessary. > > Signed-off-by: Grant Likely > Cc: "David S. Miller" Acked-by: Benjamin Herrenschmidt Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 00/25] irq_domain generalization and refinement
On Sat, 2012-02-04 at 22:17 +, Russell King - ARM Linux wrote: > On Fri, Jan 27, 2012 at 02:35:54PM -0700, Grant Likely wrote: > > Hey everyone, > > > > This patch series is ready for much wider consumption now. I'd like > > to get it into linux-next ASAP because there will be ARM board support > > depending on it. I'll wait a few days before I ask Stephen to pull > > this in. > > Grant, > > Can you answer me this: does this irqdomain support require DT? The original powerpc code this is based on didn't require it. It was an explicit design decision and I remember insisting that Grant follows it, but I haven't yet reviewed his last batch. DT is orthogonal. You have "helpers" that use the DT to resolve the domain of an interrupt source and do the mapping for you, but I made sure that you call always still create domains and map interrupts using explicit domain pointers & hw numbers. (And I need them to deal with ancient broken device-tree's on some platforms such as oldworld PowerMacs). > The question comes up because OMAP has converted some of their support > to require irq domain support for their PMICs, and it seems irq domain > support requires DT. This seems to have made the whole of OMAP > essentially become a DT-only platform. .../... > Now, here's the thing: I believe that IRQ domains - at least as far as > the hwirq stuff - should be available irrespective of whether we have > the rest of the IRQ domain support code in place, so that IRQ support > code doesn't have to keep playing games to decode from the global > space to the per-controller number space. > > I believe that would certainly help the current OMAP problems, where > the current lack of CONFIG_IRQ_DOMAIN basically makes the kernel oops > on boot. > > How we fix this regression for 3.4 I've no idea at present, I'm trying > to work out what the real dependencies are for OMAP on this stuff. > > Finally, do we need asm/irq.h in our asm/prom.h ? That's causing > fragility between DT and non-DT builds, because people are finding > that their DT builds work without their mach/irqs.h includes but > fail when built with non-DT. The only thing which DT might need - > at the most - is NR_IRQS, but I'd hope with things like irq domains > it doesn't actually require it. Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [RFCv2 00/14]
On Thu, 2012-01-26 at 14:33 -0700, Grant Likely wrote: > I've got the x86 fix in my tree now. It will be part of the next > merge. MIPS, Microblaze and OpenRISC cannot turn on CONFIG_IRQ_DOMAIN > without rework. I just hacked together the microblaze version, but > Michal will have to verify that it is correct. I just posted it. It > will be similar for the other two. > > The real problem is sparc which does something entirely different for > irqs. Rather than resolving irqs on-demand, it calculates the Linux > irq numbers at boot time for every node in the tree. The irq_domains > will need to be set up for all interrupt controllers before sparc > begins it's big walk of the tree to resolve interrupts. I haven't dug > into everything that needs to be done to support this. > > I don't think you can count on turning on IRQ_DOMAIN on all > architectures just yet. Adding irq_domain support directly to > irq_generic_chip is going to be difficult for that reason. However, > it would be useful to have an irq_domain+irq_generic_chip wrapper that > can be enabled only when IRQ_DOMAIN is enabled. Beware also that there are plenty of cases where 1 irq domain != 1 irq chip, for example on cell or xics where a single domain can encompass multiple chips. I don't know whether x86 APICs are the same, they could be tho :-) Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] of: Support CONFIG_CMDLINE_EXTEND config option
On Tue, 2012-01-10 at 20:38 -0600, Rob Herring wrote:
> On 01/09/2012 06:54 PM, Doug Anderson wrote:
> > The old logic assumes CMDLINE_FROM_BOOTLOADER vs. CMDLINE_FORCE and
> > ignores CMDLINE_EXTEND. Here's the old logic:
> >
> > - CONFIG_CMDLINE_FORCE=true
> > CONFIG_CMDLINE
> > - dt bootargs=non-empty:
> > dt bootargs
> > - dt bootargs=empty, @data is non-empty string
> > @data is left unchanged
> > - dt bootargs=empty, @data is empty string
> > CONFIG_CMDLINE (or "" if that's not defined)
> >
> > The old logic would also not honor CONFIG_CMDLINE_FORCE if there was no
> > "chosen" attribute in the device tree.
> >
> > The new logic is now documented in of_fdt.h and is copied here for
> > reference:
> >
> > - CONFIG_CMDLINE_FORCE=true
> > CONFIG_CMDLINE
> > - CONFIG_CMDLINE_EXTEND=true
> > CONFIG_CMDLINE + dt bootargs (even if dt bootargs are empty)
> > - CMDLINE_FROM_BOOTLOADER=true, dt bootargs=non-empty:
> > dt bootargs
> > - CMDLINE_FROM_BOOTLOADER=true, dt bootargs=empty, @data is non-empty string
> > @data is left unchanged
> > - CMDLINE_FROM_BOOTLOADER=true, dt bootargs=empty, @data is empty string
> > CONFIG_CMDLINE (or "" if that's not defined)
> >
So if CONFIG_CMDLINE_EXTEND=true, dt_bootargs is empty, and @data is non
empty, I want CONFIG_CMDLINE + @data..
Now the logic...
> > +/*
> > + * Convert configs to something easy to use in C code
> > + */
> > +#if defined(CONFIG_CMDLINE_FORCE)
> > +static const int overwrite_incoming_cmdline = 1;
> > +static const int read_dt_cmdline;
> > +static const int concat_cmdline;
> > +#elif defined(CONFIG_CMDLINE_EXTEND)
> > +static const int overwrite_incoming_cmdline = 1;
> > +static const int read_dt_cmdline = 1;
> > +static const int concat_cmdline = 1;
> > +#else /* CMDLINE_FROM_BOOTLOADER */
> > +static const int overwrite_incoming_cmdline;
> > +static const int read_dt_cmdline = 1;
> > +static const int concat_cmdline;
> > +#endif
> > +
> > +#ifdef CONFIG_CMDLINE
> > +static const char *config_cmdline = CONFIG_CMDLINE;
> > +#else
> > +static const char *config_cmdline = "";
> > +#endif
> > +
> > int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
> > int depth, void *data)
> > {
> > @@ -672,28 +695,29 @@ int __init early_init_dt_scan_chosen(unsigned long
> > node, const char *uname,
> >
> > pr_debug("search \"chosen\", depth: %d, uname: %s\n", depth, uname);
> >
> > + /* Make sure cmdline default is set early to handle case of no chosen */
> > + if (data && (overwrite_incoming_cmdline || !((char *)data)[0]))
> > + strlcpy(data, config_cmdline, COMMAND_LINE_SIZE);
> > +
That means that if CONFIG_CMDLINE_EXTEND is set, it will overwrite
whatever is in data. So in my case of user command line coming via
"data" in the first place, I'm not going to get the expected behaviour.
You can probably fix that by doing
if (data && ((overwrite_incoming_cmdline && !concat_cmdline) || ...
But we are losing the point of having those variables instead of
CONFIG_* in the first place.
Additionally...
> > if (depth != 1 || !data ||
> > (strcmp(uname, "chosen") != 0 && strcmp(uname, "chosen@0") != 0))
> > return 0;
> >
> > early_init_dt_check_for_initrd(node);
> >
> > - /* Retrieve command line */
> > - p = of_get_flat_dt_prop(node, "bootargs", &l);
> > - if (p != NULL && l > 0)
> > - strlcpy(data, p, min((int)l, COMMAND_LINE_SIZE));
> > -
> > - /*
> > -* CONFIG_CMDLINE is meant to be a default in case nothing else
> > -* managed to set the command line, unless CONFIG_CMDLINE_FORCE
> > -* is set in which case we override whatever was found earlier.
> > -*/
> > -#ifdef CONFIG_CMDLINE
> > -#ifndef CONFIG_CMDLINE_FORCE
> > - if (!((char *)data)[0])
> > -#endif
> > - strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
> > -#endif /* CONFIG_CMDLINE */
> > + /* Retrieve command line unless forcing */
> > + if (read_dt_cmdline) {
> > + p = of_get_flat_dt_prop(node, "bootargs", &l);
> > + if (p != NULL && l > 0) {
> > + if (concat_cmdline) {
> > + strlcat(data, " ", COMMAND_LINE_SIZE);
> > + strlcat(data, p, min_t(int, (int)l,
> > + COMMAND_LINE_SIZE));
> > + } else
> > + strlcpy(data, p, min_t(int, (int)l,
> > + COMMAND_LINE_SIZE));
> > + }
> > + }
So if you have CONFIG_CMDLINE_EXTEND, and have a command line in the
device-tree, you just read it, concat with what's in data but ...
the next node in the device-tree will hit the initial code above again
which will overwrite what you just did with the content of
CONFIG_CMDLINE (unless /chosen is the last node in the device-tree,
probably why it might have worked for you once).
I
Re: [PATCH v2] of: Change logic to overwrite cmd_line with CONFIG_CMDLINE
On Fri, 2012-01-06 at 16:48 -0800, Doug Anderson wrote: > I know this is a long-dead thread, but I was a little curious about > the motivation here. Hi ! Sorry, I planned to reply earlier and then forgot about it... > I'm looking at trying to support CONFIG_CMDLINE_EXTEND (an ARM > Kconfig) in this function and don't know in which cases I should look > at the CONFIG_CMDLINE and in which cases I should use whatever > happened to be in data before the function was called. I'll have a look later (gotta run soon) but basically, the reason I did that logic change is that in some specific circumstances and firmware version, I end up writing the user-specified command line in the global prior to actually booting the kernel :-) So in that case, I really don't want CONFIG_CMDLINE to take over because there's nothing in the device-tree, the user -did- specify something but not via the device-tree. Now, that's a bit of an oddball scenario but I felt that the logic change was harmless. For those interested in gory details, it's when doing the OPAL takeover on some machines with the version 1 of OPAL firmware. We basically boot under pHyp normally (pSeries hypervisor) and do a magic hcall which relocates the kernel to contiguous physical memory and re-starts it with a flat device-tree. The takeover mechanism didn't provide me with a way for passing a command line in that fdt. So I had to do it from prom_init (still running under pHyp context), change the kernel global before it gets relocated. Cheers, Ben. > Here's the definition in the KConfig: > <http://git.kernel.org/?p=linux/kernel/git/next/linux-next.git;a=blob;f=arch/arm/Kconfig;h=24626b0419ee97e963e68329a8eb6769360b46ea;hb=HEAD#l1984> > > Which case do you have CONFIG_CMDLINE defined but not CMDLINE_FORCE? > In those cases, do you happen to have CONFIG_CMDLINE_EXTEND or > CMDLINE_FROM_BOOTLOADER defined? > > Thanks much! > > -Doug > > --- > > On Mon, Sep 19, 2011 at 9:55 PM, Grant Likely > wrote: > > > > On Tue, Sep 20, 2011 at 02:50:15PM +1000, Benjamin Herrenschmidt wrote: > > > We used to overwrite with CONFIG_CMDLINE if we found a chosen > > > node but failed to get bootargs out of it or they were empty, > > > unless CONFIG_CMDLINE_FORCE is set. > > > > > > Instead change that to overwrite if "data" is non empty after > > > the bootargs check. It allows arch code to have other mechanisms > > > to retrieve the command line prior to parsing the device-tree. > > > > > > Note: CONFIG_CMDLINE_FORCE case should ideally be handled elsewhere > > > as it won't work as it-is if the device-tree has no /chosen node > > > > > > Signed-off-by: Benjamin Herrenschmidt > > > CC: devicetree-disc...@lists-ozlabs.org > > > CC: Grant Likely > > > > Looks okay to me. > > > > Acked-by: Grant Likely > > > > > --- > > > drivers/of/fdt.c |7 ++- > > > 1 files changed, 6 insertions(+), 1 deletions(-) > > > > > > v2. Use "data" instead of "cmd_line" so it works on archs like > > > mips who don't pass cmd_line to that function to start with, also > > > add a comment explaining the mechanism. > > > > > > (resent with right list address as well while at it) > > > > > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c > > > index 65200af..323b722 100644 > > > --- a/drivers/of/fdt.c > > > +++ b/drivers/of/fdt.c > > > @@ -681,9 +681,14 @@ int __init early_init_dt_scan_chosen(unsigned long > > > node, const char *uname, > > > if (p != NULL && l > 0) > > > strlcpy(data, p, min((int)l, COMMAND_LINE_SIZE)); > > > > > > + /* > > > + * CONFIG_CMDLINE is meant to be a default in case nothing else > > > + * managed to set the command line, unless CONFIG_CMDLINE_FORCE > > > + * is set in which case we override whatever was found earlier. > > > + */ > > > #ifdef CONFIG_CMDLINE > > > #ifndef CONFIG_CMDLINE_FORCE > > > - if (p == NULL || l == 0 || (l == 1 && (*p) == 0)) > > > + if (!data[0]) > > > #endif > > > strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE); > > > #endif /* CONFIG_CMDLINE */ > > > -- > > > 1.7.4.1 > > > > > > > > > > > > > > ___ > > devicetree-discuss mailing list > > devicetree-discuss@lists.ozlabs.org > > https://lists.ozlabs.org/listinfo/devicetree-discuss ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] fsl/mpic: Document and use the "big-endian" device-tree flag
On Wed, 2012-01-04 at 15:22 -0800, Randy Dunlap wrote:
> On 12/22/2011 08:25 AM, Kyle Moffett wrote:
> > The MPIC code checks for a "big-endian" property and sets the flag
> > MPIC_BIG_ENDIAN if one is present. Unfortunately, the PowerQUICC-III
> > compatible device-tree does not specify it, so all of the board ports
> > need to manually set that flag when calling mpic_alloc().
> >
> > Document the flag and add it to the pq3 device tree. Existing code
> > will still need to pass the MPIC_BIG_ENDIAN flag because their dtb may
> > not have this property, but new platforms shouldn't need to do so.
> >
> > Signed-off-by: Kyle Moffett
>
> Grant, are you merging this patch?
> I don't think I should merge the patch to
> arch/powerpc/boot/dts/fsl/pq3-mpic.dtsi.
Best is to leave the whole lot to me. It's not like mpic is used
anywhere else ...
Cheers,
Ben.
> > --
> > .../devicetree/bindings/powerpc/fsl/mpic.txt |9 -
> > arch/powerpc/boot/dts/fsl/pq3-mpic.dtsi|1 +
> > 2 files changed, 9 insertions(+), 1 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/powerpc/fsl/mpic.txt
> > b/Documentation/devicetree/bindings/powerpc/fsl/mpic.txt
> > index 2cf38bd..ebafba2 100644
> > --- a/Documentation/devicetree/bindings/powerpc/fsl/mpic.txt
> > +++ b/Documentation/devicetree/bindings/powerpc/fsl/mpic.txt
> > @@ -56,7 +56,14 @@ PROPERTIES
> >to the client. The presence of this property also mandates
> >that any initialization related to interrupt sources shall
> >be limited to sources explicitly referenced in the device tree.
> > -
> > +
> > + - big-endian
> > + Usage: optional
> > + Value type:
> > + If present the MPIC will be assumed to be big-endian. Some
> > + device-trees omit this property on MPIC nodes even when the MPIC
> > is
> > + in fact big-endian, so certain boards override this property.
> > +
> > INTERRUPT SPECIFIER DEFINITION
> >
> >Interrupt specifiers consists of 4 cells encoded as
> > diff --git a/arch/powerpc/boot/dts/fsl/pq3-mpic.dtsi
> > b/arch/powerpc/boot/dts/fsl/pq3-mpic.dtsi
> > index 5c80460..47f2b67 100644
> > --- a/arch/powerpc/boot/dts/fsl/pq3-mpic.dtsi
> > +++ b/arch/powerpc/boot/dts/fsl/pq3-mpic.dtsi
> > @@ -39,6 +39,7 @@ mpic: pic@4 {
> > reg = <0x4 0x4>;
> > compatible = "fsl,mpic";
> > device_type = "open-pic";
> > + big-endian;
> > };
> >
> > timer@41100 {
>
>
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH net-next v6 4/4] powerpc: tqm8548/tqm8xx: add and update CAN device nodes
On Wed, 2011-12-07 at 09:25 +0100, Wolfgang Grandegger wrote: > > Also there have been at least 3 versions in a couple of days already > > without comments nor indication of what was changed... > > Unfortunately, no response from those sub-system guys. > > > Can you clarify things a bit please ? It looks like they really should > > go to linuxppc-dev (and you can probably drop a bunch of other lists) or > > am I missing an important piece of the puzzle ? (Such as patch 1/4 and > > 2/4 ...) > > I have not sent the whole series. The changes are documented in the > cover-letter, which I have not sent for those patches. Well, I think > it's better to sent the whole series to all parties instead? Well at least for linuxppc-dev, don't bother now that I know what this is about :-) > > Let me know if I should just remove them from powerpc patchwork. > > Dave has already applied all patches. > > Sorry for the confusion. Any advice on how to handle multi subsystem > series of patches properly is welcome. No specific advice. Ideally, if patchwork could track cover letters it would help but I don't see a non-nasty way to do it so ... :-) Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH net-next v6 4/4] powerpc: tqm8548/tqm8xx: add and update CAN device nodes
On Wed, 2011-12-07 at 02:39 -0500, David Miller wrote: > From: Benjamin Herrenschmidt > Date: Wed, 07 Dec 2011 18:34:28 +1100 > > > Let me know if I should just remove them from powerpc patchwork. > > These are DT entries for CAN devices for which drivers only exist in > the net-next tree, I already included this patch into the net-next > tree so you do not have to. Thanks. Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH net-next v6 4/4] powerpc: tqm8548/tqm8xx: add and update CAN device nodes
On Thu, 2011-12-01 at 10:41 +0100, Wolfgang Grandegger wrote:
> This patch enables or updates support for the CC770 and AN82527
> CAN controller on the TQM8548 and TQM8xx boards.
I'm a bit confused by the net-next prefix here. Those patches seem to
be only touching arch/powerpc and seem to be sent primarily toward
netdev with a net-next prefix.
Also there have been at least 3 versions in a couple of days already
without comments nor indication of what was changed...
Can you clarify things a bit please ? It looks like they really should
go to linuxppc-dev (and you can probably drop a bunch of other lists) or
am I missing an important piece of the puzzle ? (Such as patch 1/4 and
2/4 ...)
Let me know if I should just remove them from powerpc patchwork.
Cheers,
Ben.
> CC: devicetree-discuss@lists.ozlabs.org
> CC: linuxppc-...@ozlabs.org
> CC: Kumar Gala
> Signed-off-by: Wolfgang Grandegger
> ---
> arch/powerpc/boot/dts/tqm8548-bigflash.dts | 19 ++-
> arch/powerpc/boot/dts/tqm8548.dts | 19 ++-
> arch/powerpc/boot/dts/tqm8xx.dts | 25 +
> 3 files changed, 53 insertions(+), 10 deletions(-)
>
> diff --git a/arch/powerpc/boot/dts/tqm8548-bigflash.dts
> b/arch/powerpc/boot/dts/tqm8548-bigflash.dts
> index 9452c3c..d918752 100644
> --- a/arch/powerpc/boot/dts/tqm8548-bigflash.dts
> +++ b/arch/powerpc/boot/dts/tqm8548-bigflash.dts
> @@ -352,7 +352,7 @@
> ranges = <
> 0 0x0 0xfc00 0x0400 // NOR FLASH bank 1
> 1 0x0 0xf800 0x0800 // NOR FLASH bank 0
> - 2 0x0 0xa300 0x8000 // CAN (2 x i82527)
> + 2 0x0 0xa300 0x8000 // CAN (2 x CC770)
> 3 0x0 0xa301 0x8000 // NAND FLASH
>
> >;
> @@ -393,18 +393,27 @@
> };
>
> /* Note: CAN support needs be enabled in U-Boot */
> - can0@2,0 {
> - compatible = "intel,82527"; // Bosch CC770
> + can@2,0 {
> + compatible = "bosch,cc770"; // Bosch CC770
> reg = <2 0x0 0x100>;
> interrupts = <4 1>;
> interrupt-parent = <&mpic>;
> + bosch,external-clock-frequency = <1600>;
> + bosch,disconnect-rx1-input;
> + bosch,disconnect-tx1-output;
> + bosch,iso-low-speed-mux;
> + bosch,clock-out-frequency = <1600>;
> };
>
> - can1@2,100 {
> - compatible = "intel,82527"; // Bosch CC770
> + can@2,100 {
> + compatible = "bosch,cc770"; // Bosch CC770
> reg = <2 0x100 0x100>;
> interrupts = <4 1>;
> interrupt-parent = <&mpic>;
> + bosch,external-clock-frequency = <1600>;
> + bosch,disconnect-rx1-input;
> + bosch,disconnect-tx1-output;
> + bosch,iso-low-speed-mux;
> };
>
> /* Note: NAND support needs to be enabled in U-Boot */
> diff --git a/arch/powerpc/boot/dts/tqm8548.dts
> b/arch/powerpc/boot/dts/tqm8548.dts
> index 619776f..988d887 100644
> --- a/arch/powerpc/boot/dts/tqm8548.dts
> +++ b/arch/powerpc/boot/dts/tqm8548.dts
> @@ -352,7 +352,7 @@
> ranges = <
> 0 0x0 0xfc00 0x0400 // NOR FLASH bank 1
> 1 0x0 0xf800 0x0800 // NOR FLASH bank 0
> - 2 0x0 0xe300 0x8000 // CAN (2 x i82527)
> + 2 0x0 0xe300 0x8000 // CAN (2 x CC770)
> 3 0x0 0xe301 0x8000 // NAND FLASH
>
> >;
> @@ -393,18 +393,27 @@
> };
>
> /* Note: CAN support needs be enabled in U-Boot */
> - can0@2,0 {
> - compatible = "intel,82527"; // Bosch CC770
> + can@2,0 {
> + compatible = "bosch,cc770"; // Bosch CC770
> reg = <2 0x0 0x100>;
> interrupts = <4 1>;
> interrupt-parent = <&mpic>;
> + bosch,external-clock-frequency = <1600>;
> + bosch,disconnect-rx1-input;
> + bosch,disconnect-tx1-output;
> + bosch,iso-low-speed-mux;
> + bosch,clock-out-frequency = <1600>;
> };
>
> - can1@2,100 {
> - compatible = "intel,82527"; // Bosch CC770
> + can@2,100 {
> + compatible = "bosch,cc770"; // Bosch CC770
> reg = <2 0x100 0x100>;
> interrupts = <4 1>;
> interrupt
Re: [PATCH] ata: Don't use NO_IRQ in pata_of_platform driver
On Fri, 2011-12-02 at 11:28 -0800, Linus Torvalds wrote: > On Fri, Dec 2, 2011 at 11:26 AM, Dave Martin wrote: > > > > This is now broken on ARM where, for good or bad, NO_IRQ currently is > > used and is -1. > > > > How do we resolve it? If we are ready to eliminate NO_IRQ from > > drivers/of/irq.c (or indeed, all code that uses it) and just use 0 for > > that case, we should surely just do it... but I'm not confident I can > > judge on that. > > Just stop using NO_IRQ. First in drivers/of/irq.c, then in any drivers > as you notice breakage. Agreed. In fact the whole hack in drivers/of/irq.c was to accomodate ARM which still uses -1, powerpc changed to 0 a long time ago. Now that we have a generic remapper between HW and "linux" IRQ numbers, there is no reason to stick to -1 even on ARM. > Don't *change* NO_IRQ to zero (that whole #define is broken - leave it > around as a marker of brokenness), just start removing it from all the > ARM drivers that use the OF infrastructure. Which is presumably not > all that many yet. > > So whenever you find breakage, the fix now is to just remove NO_IRQ > tests, and replace them with "!irq". Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Please revert commit dc9372808412edbc653a675a526c2ee6c0c14a91
Hi Linus ! Please, revert commit dc9372808412edbc653a675a526c2ee6c0c14a91 "of/irq: of_irq_find_parent: check for parent equal to child" This breaks some powerpc platforms at least. The practice of having a node provide an explicit "interrupt-parent" property pointing to itself is an old trick that we've used in the past to allow a device-node to have interrupts routed to different controllers. In that case, the node also contains an interrupt-map, so the node is its own parent, the interrupt resolution hits the map, which then can route each individual interrupt to a different parent. Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCHv2 3/3] picoxcell: add the DTS for the PC7302 board
On Fri, 2011-09-23 at 10:31 +0800, Barry Song wrote:
> 2011/9/15 Jamie Iles :
> > On Thu, Sep 15, 2011 at 08:32:02AM -0300, Benjamin Herrenschmidt wrote:
> >> On Wed, 2011-08-24 at 15:41 +0100, Jamie Iles wrote:
> >>
> >> > + chosen {
> >> > + bootargs = "console=ttyS0,115200 earlyprintk loglevel=9
> >> > root=ubi0:rootfs rw rootfstype=ubifs ubi.mtd=5,2048";
> >> > + linux,stdout-path = &uart0;
> >> > + };
> >>
> >> Hrm... we don't normally put the bootargs in the device-tree.
> >>
> >> Either you have a way to pass it from a previous firmware (which
> >> can then slap it into the device-tree at runtime), or you
> >> can have a way to compile it in the kernel image but the device-tree
> >> isn't the right place for it.
> >
> > OK, that's fair enough. A few other ARM platforms (tegra, prima2 and
> > zynq) have bootargs in the chosen node and that's where I got it from,
> > but our bootloader has fdt support so this can easily be removed.
>
> some powerpc platforms also do that:
Right, I never said I was perfect :-)
> asp834x-redboot.dts: bootargs = "console=ttyS0,38400
> root=/dev/mtdblock3 rootfstype=jffs2";
> gamecube.dts: bootargs = "root=/dev/gcnsda2 rootwait udbg-immortal";
> prpmc2800.dts:bootargs = "ip=on";
> rainier.dts: bootargs = "console=ttyS0,115200";
> sequoia.dts: bootargs = "console=ttyS0,115200";
> virtex440-ml507.dts: bootargs = "console=ttyS0 root=/dev/ram";
> virtex440-ml510.dts: bootargs = "console=ttyS0 root=/dev/ram";
> wii.dts: bootargs = "root=/dev/mmcblk0p2 rootwait udbg-immortal";
>
> if we have no bootargs in bootloader, kernel will use that one in DT.
Cheers,
Ben.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
[PATCH v2] of: Change logic to overwrite cmd_line with CONFIG_CMDLINE
We used to overwrite with CONFIG_CMDLINE if we found a chosen node but failed to get bootargs out of it or they were empty, unless CONFIG_CMDLINE_FORCE is set. Instead change that to overwrite if "data" is non empty after the bootargs check. It allows arch code to have other mechanisms to retrieve the command line prior to parsing the device-tree. Note: CONFIG_CMDLINE_FORCE case should ideally be handled elsewhere as it won't work as it-is if the device-tree has no /chosen node Signed-off-by: Benjamin Herrenschmidt CC: devicetree-disc...@lists-ozlabs.org CC: Grant Likely --- drivers/of/fdt.c |7 ++- 1 files changed, 6 insertions(+), 1 deletions(-) v2. Use "data" instead of "cmd_line" so it works on archs like mips who don't pass cmd_line to that function to start with, also add a comment explaining the mechanism. (resent with right list address as well while at it) diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c index 65200af..323b722 100644 --- a/drivers/of/fdt.c +++ b/drivers/of/fdt.c @@ -681,9 +681,14 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname, if (p != NULL && l > 0) strlcpy(data, p, min((int)l, COMMAND_LINE_SIZE)); + /* +* CONFIG_CMDLINE is meant to be a default in case nothing else +* managed to set the command line, unless CONFIG_CMDLINE_FORCE +* is set in which case we override whatever was found earlier. +*/ #ifdef CONFIG_CMDLINE #ifndef CONFIG_CMDLINE_FORCE - if (p == NULL || l == 0 || (l == 1 && (*p) == 0)) + if (!data[0]) #endif strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE); #endif /* CONFIG_CMDLINE */ -- 1.7.4.1 ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCHv2 3/3] picoxcell: add the DTS for the PC7302 board
On Wed, 2011-08-24 at 15:41 +0100, Jamie Iles wrote:
> + chosen {
> + bootargs = "console=ttyS0,115200 earlyprintk loglevel=9
> root=ubi0:rootfs rw rootfstype=ubifs ubi.mtd=5,2048";
> + linux,stdout-path = &uart0;
> + };
Hrm... we don't normally put the bootargs in the device-tree.
Either you have a way to pass it from a previous firmware (which
can then slap it into the device-tree at runtime), or you
can have a way to compile it in the kernel image but the device-tree
isn't the right place for it.
Cheers,
Ben.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v2 02/19] OpenRISC: Device tree
On Sun, 2011-07-03 at 14:51 -0600, Grant Likely wrote: > > + > > +#ifdef CONFIG_PCI > > +/* > > + * PCI <-> OF matching functions > > + * (XXX should these be here?) > > + */ > > +struct pci_bus; > > +struct pci_dev; > > +extern int pci_device_from_OF_node(struct device_node *node, > > + u8 *bus, u8 *devfn); > > +extern struct device_node *pci_busdev_to_OF_node(struct pci_bus > *bus, > > + int devfn); > > +extern struct device_node *pci_device_to_OF_node(struct pci_dev > *dev); > > +extern void pci_create_OF_bus_map(void); > > +#endif Don't copy these from powermac or microblaze. the OF_bus_map is something that should not spread :-) You get all you need of the above from generic code with my patches to PCI <-> OF matching, which should be in linux-next and are going to be merged in the next merge window. Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [openmcapi-dev] Re: [PATCH v3 2/2] powerpc: add support for MPIC message register API
On Wed, 2011-06-29 at 22:04 -0500, Meador Inge wrote: > I posted a more detailed response a few days back: > http://patchwork.ozlabs.org/patch/98075/. In > that response, I tried to put forth the rationale > for allocating the registers statically due to > the AMP use case. With that in mind, do you > still disagree with the design? If so, do > you have any suggestions for how it could be > better? No, not really, please repost with my other comments addressed. Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [RFC PATCH] irq: add irq_domain translation infrastructure
On Mon, 2011-06-27 at 11:57 +0200, Sebastian Andrzej Siewior wrote: > I see. No, actually I don't. xics is pseries where I don't see the .dts. > So you are saying that we have one irq_domain but 2+ different > interrupt-parents nodes? Yes. Basically the various PCI bridges and other interrupt sources like HEA contain what are called "source controllers" that control the targetting of interrupts (toward a CPU thread), priority, masking etc... Those act as device-tree parents as well, there's pretty much one per PCI host bridge for example since that's where all of the configuration/masking/etc... happens. However, they then turn interrupts into a special bus message that reaches eventually a presentation controller (there's one per HW thread). Those messages are basically the "HW interrupt numbers" (along with priority info etc...) and that number encodes a "BUID" which identifies the source controller that shot the message. Thus the "interrupt numbers" are unique accross the fabric and live in a unique number space. It's one domain for all intend and purposes. But several device-nodes. Now as to whether it's several irq_chip or not .. well, it depends :-) On pHyp and old style pseries, it's a single set of FW call, it's abstracted, so it's also basically one chip. On WSP, the separate source controllers (ICS) are exposed as individual chips. > How do you distinguish then between two different controllers lets say > xics and a gpio based controller? This implementation calls ->dt_translate > until one controller returns 0 which looks like brute force. It's a bit brute force but would work if the xics implementation of that translate call checks that the device-node is indeed a XICS source controller (which can be identified by its compatible property). Now I have lost track a bit with what Grant is doing, is this the old DT stuff I objected to ? I basically asked him to make the remapping orthogonal from the DT matching. > xics_host_xlate() returns always zero so you would have to go for > the compatible and check it. > Every device has an interrupt-parent node. Shouldn't the code call exact > this irq controller xlate function instead of trying them all? Well, my powerpc code iterates the domains with "match" to check which one claims to own the parent device-node, then calls xlate for that one, but I see why one could collapse those two action after all. Cheers, Ben. > > > > Cheers, > > Ben. > > > Sebastian ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [RFC PATCH] irq: add irq_domain translation infrastructure
On Mon, 2011-06-27 at 11:00 +0200, Sebastian Andrzej Siewior wrote:
> * Grant Likely | 2011-05-26 00:54:38 [-0600]:
>
> >diff --git a/arch/x86/kernel/devicetree.c b/arch/x86/kernel/devicetree.c
> >index 39645b6..9891cd4 100644
> >--- a/arch/x86/kernel/devicetree.c
> >+++ b/arch/x86/kernel/devicetree.c
> >@@ -371,36 +321,49 @@ static struct of_ioapic_type of_ioapic_type[] =
> > },
> > };
> >
> >-static int ioapic_xlate(struct irq_domain *id, const u32 *intspec, u32
> >intsize,
> >-u32 *out_hwirq, u32 *out_type)
> >+static int ioapic_dt_translate(struct irq_domain *domain,
> >+struct device_node *controller,
> >+const u32 *intspec, u32 intsize,
> >+irq_hw_number_t *out_hwirq, u32 *out_type)
> > {
> >-struct mp_ioapic_gsi *gsi_cfg;
> > struct io_apic_irq_attr attr;
> > struct of_ioapic_type *it;
> > u32 line, idx, type;
> >+int rc;
> >
> >-if (intsize < 2)
> >+if (controller != domain->of_node)
> > return -EINVAL;
>
> Is there a reason not havining the (controller != domain->of_node) check
> in irq_create_of_mapping()?
Not all domains are associated with a single OF node.
Take xics, where there can be quite a few "source controllers" which act
as device-tree interrupt parents but there's a single global domain.
Cheers,
Ben.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 2/2] powerpc: add support for MPIC message register API
On Mon, 2011-06-20 at 10:59 -0500, Scott Wood wrote: > On Sat, 18 Jun 2011 08:58:53 +1000 > Benjamin Herrenschmidt wrote: > > > On Fri, 2011-06-17 at 11:58 -0500, Scott Wood wrote: > > > When did this change from "considered an internal implementation > > > issue, and not really an interface" to "all new interfaces"? > > > > Interesting blurb... that's not everybody's opinion and I would argue > > that supporting AMP kernels isn't something we want to do with closed > > source crap. > > I'm not advocating "closed source crap", just that if something is > "policy" (as opposed to opinion), it'd be nice if the documentation > actually matched. Well, in Linux, the line between opinion and policy is quite blurred. I don't know for sure what Linus himself thinks here and various other maintainers have expressed various opinions as well. As far as I'm concerned, I don't see the point in encouraging binary junk, especially for low level interfaces like this one. Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 2/2] powerpc: add support for MPIC message register API
On Fri, 2011-06-17 at 11:58 -0500, Scott Wood wrote: > When did this change from "considered an internal implementation > issue, and not really an interface" to "all new interfaces"? Interesting blurb... that's not everybody's opinion and I would argue that supporting AMP kernels isn't something we want to do with closed source crap. Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 2/2] powerpc: add support for MPIC message register API
On Tue, 2011-05-31 at 14:19 -0500, Meador Inge wrote:
> Some MPIC implementations contain one or more blocks of message registers
> that are used to send messages between cores via IPIs. A simple API has
> been added to access (get/put, read, write, etc ...) these message registers.
> The available message registers are initially discovered via nodes in the
> device tree. A separate commit contains a binding for the message register
> nodes.
Ok, so I finally got to look at that in a bit more details...
> +#ifndef _ASM_MPIC_MSGR_H
> +#define _ASM_MPIC_MSGR_H
> +
> +#include
> +
> +struct mpic_msgr {
> + u32 __iomem *addr;
> + u32 __iomem *mer;
> + u32 __iomem *msr;
> + int irq;
> + atomic_t in_use;
> + int num;
> +};
General comment... I'm really not fan of "msgr", I'd rather see
"mpic_message_*", it's a tad more verbose but looks a lot better, no ?
Also do you need those 3 iomem pointers ? Not just one with fixed
offsets ? Or do they come from vastly different sources ?
atomic_t in_use looks fishy, but let's see how you use it...
> +extern struct mpic_msgr* mpic_msgr_get(unsigned int reg_num);
> +extern void mpic_msgr_put(struct mpic_msgr* msgr);
> +extern void mpic_msgr_enable(struct mpic_msgr *msgr);
> +extern void mpic_msgr_disable(struct mpic_msgr *msgr);
> +extern void mpic_msgr_write(struct mpic_msgr *msgr, u32 message);
> +extern u32 mpic_msgr_read(struct mpic_msgr *msgr);
> +extern void mpic_msgr_clear(struct mpic_msgr *msgr);
> +extern void mpic_msgr_set_destination(struct mpic_msgr *msgr, u32 cpu_num);
> +extern int mpic_msgr_get_irq(struct mpic_msgr *msgr);
Documentation of the API please.
> +#endif
> diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig
> index f7b0772..4d65593 100644
> --- a/arch/powerpc/platforms/Kconfig
> +++ b/arch/powerpc/platforms/Kconfig
> @@ -78,6 +78,14 @@ config MPIC_WEIRD
> bool
> default n
>
> +config MPIC_MSGR
> + bool "MPIC message register support"
> + depends on MPIC
> + default n
> + help
> + Enables support for the MPIC message registers. These
> + registers are used for inter-processor communication.
> +
> config PPC_I8259
> bool
> default n
> diff --git a/arch/powerpc/sysdev/Makefile b/arch/powerpc/sysdev/Makefile
> index 1e0c933..6d40185 100644
> --- a/arch/powerpc/sysdev/Makefile
> +++ b/arch/powerpc/sysdev/Makefile
> @@ -3,7 +3,8 @@ subdir-ccflags-$(CONFIG_PPC_WERROR) := -Werror
> ccflags-$(CONFIG_PPC64) := -mno-minimal-toc
>
> mpic-msi-obj-$(CONFIG_PCI_MSI) += mpic_msi.o mpic_u3msi.o
> mpic_pasemi_msi.o
> -obj-$(CONFIG_MPIC) += mpic.o $(mpic-msi-obj-y)
> +mpic-msgr-obj-$(CONFIG_MPIC_MSGR)+= mpic_msgr.o
> +obj-$(CONFIG_MPIC) += mpic.o $(mpic-msi-obj-y) $(mpic-msgr-obj-y)
> fsl-msi-obj-$(CONFIG_PCI_MSI)+= fsl_msi.o
> obj-$(CONFIG_PPC_MSI_BITMAP) += msi_bitmap.o
>
> diff --git a/arch/powerpc/sysdev/mpic_msgr.c b/arch/powerpc/sysdev/mpic_msgr.c
> new file mode 100644
> index 000..bfa0612
> --- /dev/null
> +++ b/arch/powerpc/sysdev/mpic_msgr.c
> @@ -0,0 +1,279 @@
> +/*
> + * Copyright 2011-2012, Meador Inge, Mentor Graphics Corporation.
> + *
> + * Some ideas based on un-pushed work done by Vivek Mahajan, Jason Jin, and
> + * Mingkai Hu from Freescale Semiconductor, Inc.
> + *
> + * 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 of the
> + * License.
> + *
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define MPIC_MSGR_REGISTERS_PER_BLOCK 4
> +#define MSGR_INUSE 0
> +#define MSGR_FREE 1
> +
> +/* Internal structure used *only* for IO mapping register blocks. */
> +struct mpic_msgr_block {
> + struct msgr {
> + u32 msgr;
> + u8 res[12];
> + } msgrs[MPIC_MSGR_REGISTERS_PER_BLOCK];
> + u8 res0[192];
> + u32 mer;
> + u8 res1[12];
> + u32 msr;
> +};
So this represent HW registers ? Please make it clear in the comment.
I'm not a terrible fan of using structures to map HW especially with so
few registers.
> +static struct mpic_msgr **mpic_msgrs = 0;
> +static unsigned int mpic_msgr_count = 0;
> +
> +struct mpic_msgr* mpic_msgr_get(unsigned int reg_num)
> +{
> + struct mpic_msgr* msgr;
> +
> + if (reg_num >= mpic_msgr_count)
> + return ERR_PTR(-ENODEV);
> +
> + msgr = mpic_msgrs[reg_num];
No locking on the array access, might be ok if those things are never
plugged in/out I suppose...
> + if (atomic_cmpxchg(&msgr->in_use, MSGR_FREE, MSGR_INUSE) == MSGR_FREE)
> + return msgr;
> +
> + return ERR_PTR(-EBUSY);
> +}
> +EXPORT_SYMBOL(mpic_msgr_get);
So how are those things intended to be used ? Clients get a fixed
"register" number to use ? It looks like this stuff would have been
better off using
Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.
On Mon, 2011-05-30 at 15:01 +0800, Mark Brown wrote: > On Sun, May 29, 2011 at 08:18:09PM -1000, Mitch Bradley wrote: > > > I'm currently dealing with an SoC that has over a hundred GPIOs. > > Whatever we choose, I think it should be able to handle an insane > > number of GPIOs without getting any more cumbersome that is > > necessary. > > This is *consumer* side GPIOs, not bindings for the device providing the > GPIOs. If a single device needs to use hundreds of GPIOs I'd expect > many of them will be block functions so you'd have a binding with an > array for things like "databus" and "addrbus". Yes. Agreed. The producer remains identified by phandle/gpio#, so it's just a naming thing on the consumer side. So what are the options ? - gpio- = < ... > properties - existing binding, along with an optional gpio-names string list - anything else ? Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [RFC 2/2] ARM:Tegra: Device Tree Support: Initialize audio card gpio's from the device tree.
On Mon, 2011-05-30 at 00:11 -0600, Grant Likely wrote: > > Interesting... what was the reasoning behind this? It's a definite > > step backwards but it does explain my major concern with the new batch > > of device tree patches. > > The binding for gpios was defined a few years ago and it is in fairly > wide use within the powerpc sphere. The design followed the pattern > established for specifying irqs, and in that regard satisfied the > principle of least surprise. > > That said, it isn't a very large leap to go from a single 'gpios' > property to allowing multiple named gpios properties with meaningful > names, particularly if they are fully specified by the device > binding, and they follow exactly the same binding semantics as the > existing 'gpios' proprety (phandle + gpio specifier). > > Personally, I'm /cautious/ about saying okay to extending the binding, > simply because once the extension is in use it is really hard to go > back on it, but I cannot think of any reason why this particular case > wouldn't be a good idea. Anyone have thoughts on this? Ben? Mitch? So first, I wasn't involved in the definition of the GPIO binding much if at all :-) Second, I can see advantages in both the numbered and the named approaches, it's not totally clear cut and it's definitely not a "definite step backward" which explains "major concerns" since, Mark, you just discovered it ;-) You -do- write like you just found the excuse for your general hostility :-) Now more seriously, referencing GPIOs as a list of phandle/number in an array has the advantage of being compact. It works well for many setups, and GPIOs in practice are -often- referenced by number within a GPIO block. I think it's probably the right approach to "target" a given GPIO -provider-. The question is how do you relate the entries in that array with basically wires on the "client" chip. This is the same problem faced by the clock binding btw, tho there's usually a bit less clocks than GPIOs (but still more than interrupts :-) The approach apple uses is interesting, where they essentially use device-specific properties that contain a GPIO binding. For example, a reset-gpio property, that points to the GPIO doing the reset, etc... We -could- do just that. For 'busses' such a property can easily contain more than one entry. Or we could keep a gpio "map" and have a parallel property to name the entries like I was originally thinking for the clocks. In any case, it's something that can reasonably easily be added on top of the existing binding, and it would be much more productive Mark if you actually proposed solutions rather than just opposing things :-) Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 5/8] powerpc: override dma_get_required_mask by platform hook and ops
On Wed, 2011-05-11 at 15:25 -0700, Nishanth Aravamudan wrote: > From: Milton Miller > > The hook dma_get_required_mask is supposed to return the mask required > by the platform to operate efficently. The generic version of > dma_get_required_mask in driver/base/platform.c returns a mask based > only on max_pfn. However, this is likely too big for iommu systems > and could be too small for platforms that require a dma offset or have > a secondary window at a high offset. The result of those 3 patches doesn't build on top of my current tree, the generic dma_ops lacks the dma_get_required_mask hook. I'll have a look again after the merge window. Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 2/6] powerpc: make irq_{alloc, free}_virt private and remove count argument
On Wed, 2011-05-04 at 09:59 -0600, Grant Likely wrote: > > Heh, I wondered about that, but there hasn't been any users for at > least 5 years (as evidenced by commit e12514650b, "Fix loop logic in > irq_alloc_virt()") so it was looking pretty dead, and it made the > patch to switch to using irq_alloc_desc*() quite a bit simpler if it > was removed. :-) > > I'm not particularly attached to this patch, so I can drop it from the > series. Well, Willy recently added support for the MSI block allocation so ... Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 4/6] dt: generalize irq_of_create_mapping()
On Wed, 2011-05-04 at 10:05 -0600, Grant Likely wrote: > > I think you are going the wrong way around. > > > > First thing first, is to make the irq domain / mapping API generic > > without the OF bits. > > > > IE. move the IRQ domain generically, get rid of irq_map by putting > the > > domain ptr & hw numbers in the irq desc/data etc... > > > > Then you can move over the OF specific bits which are optional and > > orthogonal to a large extent. > > As discussed in my other reply, I disagree. There isn't an immediate > need for the mapping interface in common code. It would be useful, > sure, for some interrupt controllers, but for many of them > irq_alloc_descs() and an irq_base value is all the functionality that > is needed, and irq_host doesn't gain anything. No but the concept of domain is a pre-requisite. Even if it's an opaque data structure. And I don't want to have it be some "of" specific thing. > The OF translation on the other hand is needed immediately by several > architectures and are very much non-optional in that regard. But it relies on having an underlying mapping. I don't see how you can do one without the other, even if your mapping in effect is just an offset. And it is -not- related to OF. Whether you obtain that your interrupt you are interested it is interrupt 5 of your PIC "foo" via the device-tree or any other way (an arch quirk for example), you need the mechanism to associate them together, whether it's a simple offset as you propose (I'm ok with that for simple things, I just didn't think it was the right approach for powerpc but it's perfectly valid as an option generically) or a more complex radix-tree style mapping. Thus sort out the mapping interfaces first. _Then_ layout the device-tree bits on top. The basic parsing for the DT is already there. It will return a parent node and a __be32* pointer. Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 0/6] General device tree irq domain infrastructure
> I completely agree that irq domains are a generically useful feature > for architectures, and it should be made available. I also completely > agree that it is orthogonal to device tree translations, which in a > large part is why I've structured this series and the new code the > way I have. > > In this series I'm specifically addressing device tree translation, > which is the one bit that is DT specific, and is important regardless > of the backend translation mechanism. In fact, the more I looked at > it, the more it seems that the DT api really is orthogonal to the > backend irq mapping and I don't think that the way irq_host ties them > together is necessarily the best way to do it. Many of the interrupt > controllers which need to gain dt irq parsing have already been > converted to using the irq_alloc_desc*() apis and have all the mapping > mechanism they need, but lack a method of exposing it for DT > translation. But irq_alloc_desc() is purely about allocating the linux side irq descriptors. Nothing to do with HW numbers here. You still need a mapping mechanism, regardless of the device-tree, to map those linux number to your HW numbers. This is simple and eventually even 1:1 on things like x86, but the minute you start having cascaded IRQ controllers, multiple IRQ domains, and/or very large HW numbers that encode node IDs etc... this falls appart. And this is still completely orthogonal to the device-tree. Mapping is the important functionality. Whatever the actual allocator is is indeed irrelevant. > As for the mapping, I agree that the functionality is generally > useful, I'm just not fond of the current implementation. I think it > is more complex than it needs to be and I'm not excited about bring it > over to the other architectures as-is. Nobody cares about the current implementation. What is important is indeed the functionality. The basic thing I think everybody agrees is that you need to extend the irq_desc (or data, whatever tglx prefers) with two bits of information: Some identifier of the domain and some identifier of the interrupt number within that domain. In addition, PIC code will need a way to perform efficient reverse mapping. You may decide that for simple IRQ controllers that handle a small linear range of interrupts, it's kosher to simply reserve a linear range of descriptors and use a simple offset, I'm find with that now that we no longer live in a world constrained by NR_IRQ. But the need for the radix tree remains for things that have massively large potential HW numbers such as we have on powerpc. > For the majority of fixed hw interrupt controllers it is overkill. > There is no need for a map table when all the irq descs (<=32 of them) > get allocated when the irq controller is instantiated and the mapping > consists of 'virq = hw_irq + virq_base'. For instance, with the arm > irq controllers, it's be more than sufficient to use irq_alloc_descs > to obtain a range of irq numbers and then a simple of_irq_domain > registration to handle the parsing. That's true if and only if you make NR_IRQ a non issue and if you accept the general wastage due to unused interrupts. The main problem has always been that hard limit which made me chose a more efficient mechanisms. Take a mac with 2 cascaded MPICs with 256 sources each which are mostly never used. I would need an NR_IRQs of 512 with your scheme (plus 16 because I do want to continue avoiding the "ISA" numbers), which is a waste of space, even with SPARSE_IRQ. Now I hope eventually NR_IRQ will go away and we'll have a more efficient mechanism to allocate descriptors and so it will become less of an issue. > For the cases where an interrupt controller isn't able to alloc all > the irq descs at once, like for MSI, then yes the LINEAR and RADIX > mappings are important. What bothers me though is the way irq_host > needs to use unions and the ->revmap_type value to implement multiple > behaviours into a single type. That's the sort of thing that can be > broken out into a library and instantiated only by the interrupt > controllers that actually need it. But that's what it is really. You'll notice that on the fast path the interrupt controller code calls directly into the "right" type of revmap routine. You may want to refactor things a bit if you want, but the union served me well simply because I didnt have to bother doing lots of different alloc_bootmem back then. Nowadays, kmalloc is available much earlier so it might have become a non issue too. > Similarly, it bothers me that that > radix mapping makes up a significant portion of the code, yet it has > only one user. "significant" ? Seriously ? Like 3 function calls ? It's nothing. We use an existing radix tree facility, and the amount of code in our irq.c is actually very small. Originally it was living in xics in fact, but I moved it out specifically because I wanted a common interface to remapping, so for example, I can expose the linux -
Re: Login prompt on a video console instead of serial port?
On Mon, 2011-05-02 at 15:02 -0500, Timur Tabi wrote: > McClintock Matthew-B29882 wrote: > > Don't you have to spawn a terminal on the framebuffer for the login? > > I suppose, but I don't know how to do that. And although that would > technically > answer the question in the subject of this thread, I still would have *some* > boot output on the serial port. It would be nice if I could get all of stdout > on the video display, and all of stdin from the serial port. I don't think we have a way to do that unless you can make a serial "keyboard" device in the input layer. The VT layer will only get its input from such a thing, maybe that does exist in the depth of legacy code in there but it has nothing to do with your device-tree. > > Right now getty spawns the login on the serial port via /etc/inittab. > > Something similiar is probably needed for the framebuffer. > > getty appears to work only with serial devices, since it insists on a baud > rate > as one of the parameters. No, or no existing distro would work :-) Just fake a baudrate for the tty's Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 3/6] powerpc: Make struct irq_host semi-private by moving into irqhost.h
On Thu, 2011-04-28 at 14:01 -0600, Grant Likely wrote:
> Very few files actually need direct access to struct irq_host members.
> This patch moves the irq_host definition into another file so that it
> isn't brought in by default, and to prepare for the addition of
> struct of_irq_domain, which will factor some of the irq_host behaviour
> out into common code. This needs to be done because of_irq_domain
> will be embedded inside struct irq_host, and to do that it needs to be
> guaranteed that struct of_irq_domain gets fully defined first.
Why do you want to separate the irq "host" as used by powerpc with the
generic irq domain ? They should be one single thing...
Cheers,
Ben.
> Signed-off-by: Grant Likely
> ---
> arch/powerpc/include/asm/irq.h | 20 +--
> arch/powerpc/include/asm/irqhost.h | 29
> ++
> arch/powerpc/kernel/irq.c|1 +
> arch/powerpc/platforms/512x/mpc5121_ads_cpld.c |1 +
> arch/powerpc/platforms/52xx/media5200.c |1 +
> arch/powerpc/platforms/52xx/mpc52xx_gpt.c|1 +
> arch/powerpc/platforms/52xx/mpc52xx_pic.c|1 +
> arch/powerpc/platforms/82xx/pq2ads-pci-pic.c |1 +
> arch/powerpc/platforms/cell/axon_msi.c |1 +
> arch/powerpc/platforms/cell/spider-pic.c |1 +
> arch/powerpc/platforms/embedded6xx/flipper-pic.c |1 +
> arch/powerpc/platforms/embedded6xx/hlwd-pic.c|1 +
> arch/powerpc/platforms/embedded6xx/wii.c |6 +++--
> arch/powerpc/sysdev/fsl_msi.c|1 +
> arch/powerpc/sysdev/i8259.c |1 +
> arch/powerpc/sysdev/ipic.c |1 +
> arch/powerpc/sysdev/mpc8xxx_gpio.c |1 +
> arch/powerpc/sysdev/mpic.c |1 +
> arch/powerpc/sysdev/mpic_msi.c |1 +
> arch/powerpc/sysdev/mpic_pasemi_msi.c|1 +
> arch/powerpc/sysdev/qe_lib/qe_ic.c |1 +
> arch/powerpc/sysdev/uic.c|1 +
> arch/powerpc/sysdev/xilinx_intc.c|1 +
> 23 files changed, 54 insertions(+), 21 deletions(-)
> create mode 100644 arch/powerpc/include/asm/irqhost.h
>
> diff --git a/arch/powerpc/include/asm/irq.h b/arch/powerpc/include/asm/irq.h
> index 4d2cc6f..a44be93 100644
> --- a/arch/powerpc/include/asm/irq.h
> +++ b/arch/powerpc/include/asm/irq.h
> @@ -104,29 +104,11 @@ struct irq_host_ops {
>irq_hw_number_t *out_hwirq, unsigned int *out_type);
> };
>
> -struct irq_host {
> - struct list_headlink;
> -
> - /* type of reverse mapping technique */
> - unsigned intrevmap_type;
> +/* Reverse map types; pass into irq_alloc_host revmap_type argument */
> #define IRQ_HOST_MAP_LEGACY 0 /* legacy 8259, gets irqs 1..15 */
> #define IRQ_HOST_MAP_NOMAP 1 /* no fast reverse mapping */
> #define IRQ_HOST_MAP_LINEAR 2 /* linear map of interrupts */
> #define IRQ_HOST_MAP_TREE3 /* radix tree */
> - union {
> - struct {
> - unsigned int size;
> - unsigned int *revmap;
> - } linear;
> - struct radix_tree_root tree;
> - } revmap_data;
> - struct irq_host_ops *ops;
> - void*host_data;
> - irq_hw_number_t inval_irq;
> -
> - /* Optional device node pointer */
> - struct device_node *of_node;
> -};
>
> struct irq_data;
> extern irq_hw_number_t irqd_to_hwirq(struct irq_data *d);
> diff --git a/arch/powerpc/include/asm/irqhost.h
> b/arch/powerpc/include/asm/irqhost.h
> new file mode 100644
> index 000..958e6c1
> --- /dev/null
> +++ b/arch/powerpc/include/asm/irqhost.h
> @@ -0,0 +1,29 @@
> +#ifndef _ASM_POWERPC_IRQHOST_H
> +#define _ASM_POWERPC_IRQHOST_H
> +
> +#include
> +#include
> +#include
> +#include
> +
> +struct irq_host {
> + struct list_headlink;
> +
> + /* type of reverse mapping technique */
> + unsigned intrevmap_type;
> + union {
> + struct {
> + unsigned int size;
> + unsigned int *revmap;
> + } linear;
> + struct radix_tree_root tree;
> + } revmap_data;
> + struct irq_host_ops *ops;
> + void*host_data;
> + irq_hw_number_t inval_irq;
> +
> + /* Optional device node pointer */
> + struct device_node *of_node;
> +};
> +
> +#endif /* _ASM_POWERPC_IRQHOST_H */
> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> index 5ccf38f..b961b19 100644
> --- a/arch/powerpc/kernel/irq.c
> +++ b/arch/powerpc/kernel/irq.c
> @@ -56,6 +56,7 @@
> #include
> #include
>
> +#include
> #include
> #include
> #include
> diff --git a/arch/powerpc/platforms/512x/mpc5121_ads_cpld.c
> b/arch/powerpc/platforms/512x/mpc5121_ads_cpld.c
> index a8b
Re: [PATCH 4/6] dt: generalize irq_of_create_mapping()
On Thu, 2011-04-28 at 14:02 -0600, Grant Likely wrote:
> This patch creates a common implementation of irq_of_create_mapping()
> and factors out the interrupt domain translation code from powerpc to
> make it available for all architectures.
I think you are going the wrong way around.
First thing first, is to make the irq domain / mapping API generic
without the OF bits.
IE. move the IRQ domain generically, get rid of irq_map by putting the
domain ptr & hw numbers in the irq desc/data etc...
Then you can move over the OF specific bits which are optional and
orthogonal to a large extent.
Cheers,
Ben.
> It creates a new structure, struct of_irq_domain, which can be
> embedded into the private data structure of an interrupt controller.
> Any interrupt controller can call of_irq_domain_add() to register a
> structure with a .map() hook that can translate irq specifiers from
> the device tree into linux irq numbers.
>
> The patch also modifies the powerpc irq_host to embed the
> of_irq_domain structure and use the new common infrastructure for
> registering domains. This separates the reverse mapping and irq
> allocation infrastructure of irq_host from the domain registration
> infrastructure. I elected to split the functionality this way for
> several reasons. First, with the major irq cleanup done by Thomas
> Gleixner, dynamic allocation of irqs can be handled gracefully with
> irq_alloc_desc*() and interrupt controllers may not need or want an
> irq_host layer to manage it for them. For example, the new
> irq_chip_generic() already has a method for mapping between hwirq and
> Linux irq.
>
> Second, the irq_host code currently has a lot of complexity to handle
> all the different reverse mapping types from a single structure. The
> radix mapping in particular has a lot of support code, but only one
> user. I didn't want to bring that complexity into the common code
> as-is. Instead, I'd prefer to create a separate encapsulating
> structure for each revmap, and each type would have a separate .map
> hook.
>
> For now, the mapping code remains powerpc-specific and further
> generalization will happen in subsequent patches.
>
> Signed-off-by: Grant Likely
> ---
> arch/microblaze/kernel/irq.c |7 --
> arch/mips/kernel/prom.c | 14
> arch/powerpc/include/asm/irq.h | 21 +++--
> arch/powerpc/include/asm/irqhost.h |4 -
> arch/powerpc/kernel/irq.c| 99 ++---
> arch/powerpc/platforms/cell/axon_msi.c | 12 ++-
> arch/powerpc/platforms/cell/spider-pic.c |8 +-
> arch/powerpc/sysdev/fsl_msi.c|2 -
> arch/powerpc/sysdev/i8259.c |2 -
> arch/powerpc/sysdev/ipic.c |2 -
> arch/powerpc/sysdev/mpic.c |4 +
> arch/powerpc/sysdev/mpic_msi.c |2 -
> arch/powerpc/sysdev/mpic_pasemi_msi.c|4 +
> arch/powerpc/sysdev/qe_lib/qe_ic.c |2 -
> arch/x86/include/asm/irq_controller.h| 12 ---
> arch/x86/include/asm/prom.h |1
> arch/x86/kernel/devicetree.c | 77
> drivers/of/irq.c | 118
> ++
> include/linux/of_irq.h | 31
> 19 files changed, 248 insertions(+), 174 deletions(-)
> delete mode 100644 arch/x86/include/asm/irq_controller.h
>
> diff --git a/arch/microblaze/kernel/irq.c b/arch/microblaze/kernel/irq.c
> index ce7ac84..59bb560 100644
> --- a/arch/microblaze/kernel/irq.c
> +++ b/arch/microblaze/kernel/irq.c
> @@ -54,10 +54,3 @@ unsigned int irq_create_mapping(struct irq_host *host,
> irq_hw_number_t hwirq)
> return hwirq;
> }
> EXPORT_SYMBOL_GPL(irq_create_mapping);
> -
> -unsigned int irq_create_of_mapping(struct device_node *controller,
> -const u32 *intspec, unsigned int intsize)
> -{
> - return intspec[0];
> -}
> -EXPORT_SYMBOL_GPL(irq_create_of_mapping);
> diff --git a/arch/mips/kernel/prom.c b/arch/mips/kernel/prom.c
> index a19811e9..0b82f98 100644
> --- a/arch/mips/kernel/prom.c
> +++ b/arch/mips/kernel/prom.c
> @@ -60,20 +60,6 @@ void __init early_init_dt_setup_initrd_arch(unsigned long
> start,
> }
> #endif
>
> -/*
> - * irq_create_of_mapping - Hook to resolve OF irq specifier into a Linux irq#
> - *
> - * Currently the mapping mechanism is trivial; simple flat hwirq numbers are
> - * mapped 1:1 onto Linux irq numbers. Cascaded irq controllers are not
> - * supported.
> - */
> -unsigned int irq_create_of_mapping(struct device_node *controller,
> -const u32 *intspec, unsigned int intsize)
> -{
> - return intspec[0];
> -}
> -EXPORT_SYMBOL_GPL(irq_create_of_mapping);
> -
> void __init early_init_devtree(void *params)
> {
> /* Setup flat device-tree pointer */
> diff --git a/arch/powerpc/include/asm/irq.h b/arch/powerpc/include/asm/irq.h
> index a44be93..ccefc8c 1006
Re: [PATCH 2/6] powerpc: make irq_{alloc, free}_virt private and remove count argument
On Thu, 2011-04-28 at 14:01 -0600, Grant Likely wrote:
> irq_alloc_virt() and irq_free_virt() aren't called anywhere but from
> arch/powerpc/kernel/irq.c, and they are only ever called with count=1.
> This patch removes the prototypes from the header file, removes the
> count arguments, and cuts out the dead code.
>
> Also removes obsolete references to irq_early_init()
Nack.
The count was intended to be able to allocate blocks of interrupts. This
was not used so far because we didn't support MSI blocks (for non-X
MSIs) but that is coming, and unfortunately, the API that was designed
for that is crap and requires contiguous IRQ numbers on the linux side
as well as on the device side (well device side is a HW requirement but
we could have been smarter on the Linux side).
So the ability to allocate blocks will be needed. In fact it's not clear
yet whether we'll also need them to be naturally aligned powers of two
or not at this stage. It depends how the bloody API is going to be used
by drivers.
Cheers,
Ben.
> Signed-off-by: Grant Likely
> ---
> arch/microblaze/kernel/setup.c |2 -
> arch/powerpc/include/asm/irq.h | 32
> arch/powerpc/kernel/irq.c | 82
>
> 3 files changed, 40 insertions(+), 76 deletions(-)
>
> diff --git a/arch/microblaze/kernel/setup.c b/arch/microblaze/kernel/setup.c
> index 8e2c09b..19d3ab7 100644
> --- a/arch/microblaze/kernel/setup.c
> +++ b/arch/microblaze/kernel/setup.c
> @@ -51,8 +51,6 @@ void __init setup_arch(char **cmdline_p)
>
> unflatten_device_tree();
>
> - /* NOTE I think that this function is not necessary to call */
> - /* irq_early_init(); */
> setup_cpuinfo();
>
> microblaze_cache_init();
> diff --git a/arch/powerpc/include/asm/irq.h b/arch/powerpc/include/asm/irq.h
> index e1983d5..4d2cc6f 100644
> --- a/arch/powerpc/include/asm/irq.h
> +++ b/arch/powerpc/include/asm/irq.h
> @@ -264,38 +264,6 @@ extern unsigned int irq_linear_revmap(struct irq_host
> *host,
> irq_hw_number_t hwirq);
>
>
> -
> -/**
> - * irq_alloc_virt - Allocate virtual irq numbers
> - * @host: host owning these new virtual irqs
> - * @count: number of consecutive numbers to allocate
> - * @hint: pass a hint number, the allocator will try to use a 1:1 mapping
> - *
> - * This is a low level function that is used internally by
> irq_create_mapping()
> - * and that can be used by some irq controllers implementations for things
> - * like allocating ranges of numbers for MSIs. The revmaps are left
> untouched.
> - */
> -extern unsigned int irq_alloc_virt(struct irq_host *host,
> -unsigned int count,
> -unsigned int hint);
> -
> -/**
> - * irq_free_virt - Free virtual irq numbers
> - * @virq: virtual irq number of the first interrupt to free
> - * @count: number of interrupts to free
> - *
> - * This function is the opposite of irq_alloc_virt. It will not clear reverse
> - * maps, this should be done previously by unmap'ing the interrupt. In fact,
> - * all interrupts covered by the range being freed should have been unmapped
> - * prior to calling this.
> - */
> -extern void irq_free_virt(unsigned int virq, unsigned int count);
> -
> -/**
> - * irq_early_init - Init irq remapping subsystem
> - */
> -extern void irq_early_init(void);
> -
> static __inline__ int irq_canonicalize(int irq)
> {
> return irq;
> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> index 422672b..5ccf38f 100644
> --- a/arch/powerpc/kernel/irq.c
> +++ b/arch/powerpc/kernel/irq.c
> @@ -647,6 +647,9 @@ void irq_set_virq_count(unsigned int count)
> irq_virq_count = count;
> }
>
> +static unsigned int irq_alloc_virt(struct irq_host *host, unsigned int hint);
> +static void irq_free_virt(unsigned int virq);
> +
> static int irq_setup_virq(struct irq_host *host, unsigned int virq,
> irq_hw_number_t hwirq)
> {
> @@ -675,7 +678,7 @@ static int irq_setup_virq(struct irq_host *host, unsigned
> int virq,
> errdesc:
> irq_free_descs(virq, 1);
> error:
> - irq_free_virt(virq, 1);
> + irq_free_virt(virq);
> return -1;
> }
>
> @@ -689,7 +692,7 @@ unsigned int irq_create_direct_mapping(struct irq_host
> *host)
> BUG_ON(host == NULL);
> WARN_ON(host->revmap_type != IRQ_HOST_MAP_NOMAP);
>
> - virq = irq_alloc_virt(host, 1, 0);
> + virq = irq_alloc_virt(host, 0);
> if (virq == NO_IRQ) {
> pr_debug("irq: create_direct virq allocation failed\n");
> return NO_IRQ;
> @@ -742,7 +745,7 @@ unsigned int irq_create_mapping(struct irq_host *host,
> } else {
> /* Allocate a virtual interrupt number */
> hint = hwirq % irq_virq_count;
> - virq = irq_alloc_virt(host, 1, hint);
> + virq = irq_alloc_virt(host, hint);
> if (virq ==
Re: [PATCH 1/6] powerpc: stop exporting irq_map
On Thu, 2011-04-28 at 14:01 -0600, Grant Likely wrote: > First step in eliminating irq_map[] table entirely > > Signed-off-by: Grant Likely > --- > Ack in principle, needs scrutiny and testing of course :-) Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 0/6] General device tree irq domain infrastructure
On Thu, 2011-04-28 at 14:01 -0600, Grant Likely wrote:
> A lot of this series ends up being fixups to powerpc code; but the 4th
> patch is of importance to every architecture using CONFIG_OF (except
> SPARC, which has its own solution).
>
> This series (finally!) factors out device tree irq domain decoding
> from arch/powerpc and makes it generic for all architectures. The
> infrastructure is quite simple. Any interrupt controller can embed
> the of_irq_domain structure and register it with the core code. After
> that, device nodes referencing interrupts on that device tree node
> will trigger a call to the domain's map function.
This leads to an immediate reaction from me : why "of_irq_domain" ?
The concept of interrupt domains is completely orthogonal to "OF" and
whether you use the device-tree or not.
Having a remapping mechanism allowing to deal with multiple interrupt
domains without playing stupid numbering tricks is generally useful for
architectures, regardless of their adoption of the device-tree.
The irq domain has one and -only one- op that is related to OF which
allows to map a device node to a domain, but that's 'optional' and only
use if you use the OF resolving process. The whole mechanism can be (and
is under some circumstances on ppc) without a device-tree relationship.
We instanciate IRQs within domains manually in some cases, either
because we lack proper DT informations or bcs the IRQs come from the
firmware or as "created" out of the blue (device-tree). A domain pointer
(or NULL for the default domain) is all is needed.
Thus I object to tying this infrastructure generically to "OF" even if
it's just a mater of naming of the domain structure and location of the
code in the kernel tree.
It should basically all go into kernel/irq/domains.c or something like
that.
Cheers,
Ben.
> PowerPC and x86 have been converted to use of_irq_domain. MIPS and
> Microblaze have it enabled, but nothing actually registers domains
> yet, so a workaround is in place to preserve the current behaviour
> until it is fixed.
>
> I'd really like to get patches 1-4 merged into 2.6.40. Please test.
> I'm also running through build testing here, and when it's complete
> I'll push it out to a 'devicetree/irq-domain' branch on
> git://git.secretlab.ca/git/linux-2.6
>
> It needs testing. I've booted it on a powerpc board here without any
> apparent regressions, but that isn't a very big sample. I've also
> build tested on everything I think is affected.
>
> I'd also like to get it into linux-next. Ben, if things checkout okay
> over the next few days, would you be okay with me adding it to
> linux-next, say around Wednesday next week? As for merging, I think
> this should probably go via your powerpc tree since the that's where
> the bulk of the changes are, but I'm open to other suggestions).
>
> Patches 5 & 6 are follow-on cleanup work to powerpc, but patch 6 is
> RFC only since there is a locking problem that I haven't fixed yet.
>
> Cheers,
> g.
>
> ---
>
> Grant Likely (6):
> powerpc: stop exporting irq_map
> powerpc: make irq_{alloc,free}_virt private and remove count argument
> powerpc: Make struct irq_host semi-private by moving into irqhost.h
> dt: generalize of_irq_parse_and_map()
> powerpc: move irq_alloc_descs_at() call into irq_alloc_virt()
> powerpc: use irq_alloc_desc() to manage irq allocations
>
>
> arch/microblaze/kernel/irq.c |7 -
> arch/microblaze/kernel/setup.c |2
> arch/mips/kernel/prom.c | 14 -
> arch/powerpc/include/asm/irq.h | 88 +--
> arch/powerpc/include/asm/irqhost.h | 27 ++
> arch/powerpc/kernel/irq.c| 260
> --
> arch/powerpc/platforms/512x/mpc5121_ads_cpld.c |5
> arch/powerpc/platforms/52xx/media5200.c |5
> arch/powerpc/platforms/52xx/mpc52xx_gpt.c|1
> arch/powerpc/platforms/52xx/mpc52xx_pic.c| 80 +--
> arch/powerpc/platforms/82xx/pq2ads-pci-pic.c |5
> arch/powerpc/platforms/85xx/socrates_fpga_pic.c | 26 +-
> arch/powerpc/platforms/86xx/gef_pic.c| 10 -
> arch/powerpc/platforms/8xx/m8xx_setup.c |2
> arch/powerpc/platforms/cell/axon_msi.c | 15 +
> arch/powerpc/platforms/cell/spider-pic.c | 19 +-
> arch/powerpc/platforms/embedded6xx/flipper-pic.c |9 -
> arch/powerpc/platforms/embedded6xx/hlwd-pic.c|9 -
> arch/powerpc/platforms/embedded6xx/wii.c |6 -
> arch/powerpc/platforms/iseries/irq.c | 10 -
> arch/powerpc/platforms/powermac/pic.c| 12 +
> arch/powerpc/platforms/pseries/ras.c |4
> arch/powerpc/platforms/pseries/xics.c| 14 +
> arch/powerpc/sysdev/cpm1.c |8 -
> arch/powerpc/sysdev/cpm2_pic.c | 10 -
> arch/powerpc/s
Re: checking status semantics with compatible functions
On Wed, 2011-03-30 at 08:31 -0600, Grant Likely wrote: > Yes, of_device_is_available() should be checked, but it should not be > added directly to of_device_is_compatible(). I'm okay with adding > a helper variant that does the of_device_is_compatible() check. > > In that particular case, I'd also suggest using > for_each_matching_node(). Agreed. A device might be unavailable due to how FW configured the machine, but made later on available by the kernel, that shouldn't impact the interface compatibility test. Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: RFC: Platform data for onboard USB assets
On Fri, 2011-03-18 at 07:42 +, Andy Green wrote: > > No, if you read Arnd's post you will find Device Tree does not support > targeting USB devices yet, and if you further read the 1998 document he > points to as the basis of actually implementing it, it seems to me at > least it'll be a little project yet to do that on Linux side. You are getting seriously full of sh*t here. Read again what Arnd said rather than twisting it to try to make your point. The age of the OF binding document for USB has no relevance. The fact that it's a very simple binding however does. As Arnd mention, nobody says that somebody/something has to generate a full and complete representation of USB for your board, that would be beyond the scope. However, it should be trivial to layout in the fixed part of the board device-tree nodes representing on-board (fixed) devices with the right amount of properties for allowing the linkage to happen between those nodes and the Linux usb_device object. Basically it boils down to one property afaik, "reg". That's about it (oh and possibly #address-cells and compatible for hubs that are on the way to the device if any). About 15mn of work if you are familiar with the .dts syntax, maybe twice as much if you are not. Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: RFC: Platform data for onboard USB assets
On Fri, 2011-03-18 at 21:06 +0100, Arnd Bergmann wrote: > > In a perfect world, we would need neither the device tree nor > any platform data at all, because we'd be able to ask the hardware > or the fictionary correct firmware about what the properties > of the hardware are. This works to a surprisingly large extent > on server hardware, but much less so on typical embedded systems. Properties of the HW per-se but also binding information, ie, what is connected to what outside of the main bus path (think clock/power control etc...). Even server / desktop is affected here, and nobody sane thinks ACPI is a -good- solution here tho it works mostly on x86 :-) Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: RFC: Platform data for onboard USB assets
On Thu, 2011-03-17 at 23:22 +, Andy Green wrote: > > Is there actually a need for sort of not platform_data but universal > vid_pid_specific_usb_device_option_data coming from the board > definition > file or bootloader for *pluggable* usb devices? udev seems to be > well > established doing that already in a generic, not-platform-specific > way > that can go in all distros and so on nicely. Maybe this is just my > impoverished imagination and people do want, say, some kinds of USB > mice > to operate at higher DPI or whatever when plugged in a specific board > because it is that board. Except that vid/pid are the -WRONG- way to do that. Your device is on-board, the proper way to identify it uniquely is it's topological location, ie path of HW port numbers. Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v4 2/2] powerpc: make MPIC honor the "pic-no-reset" device tree property
On Thu, 2011-03-10 at 11:23 -0600, Meador Inge wrote:
> AFAIK, we can't rely on 'set_affinity' always being called. I don't
> think it is called at all when !defined(CONFIG_SMP) and if it was,
> then that would be an error:
>
> /* include/linux/irq.h */
>
> #else /* CONFIG_SMP */
>
> static inline int irq_set_affinity(unsigned int irq,
> const struct cpumask *m)
> {
> return -EINVAL;
> }
You are right. We do need to set a sane default then.
> > partially initialized in set_type, I'd say just modify set_type to
> > initialize the source as well, and problem solved, no ?
>
> The priority has to be initialized as well. They could both be done
> in
> 'mpic_set_irq_type', but that seems like a weird place since it has
> nothing to do with actually setting the type.
>
> Since we already have 'mpic_irq_set_priority' and 'mpic_set_vector',
> perhaps a better option is to add 'mpic_set_destination' and put the
> following in 'mpic_host_map' (using the cpuid helper function
> suggested
> above):
>
> /* Lazy source init when MPIC_NO_RESET */
> if (!mpic_is_ipi(mpic, hw) && (mpic->flags & MPIC_NO_RESET)) {
> mpic_set_vector(virq, hw);
> mpic_set_destination(virq, mpic_cpuid(mpic));
> mpic_irq_set_priority(virq, 8);
> }
>
> It is more overhead, but it reads well. Thoughts?
No objection.
Cheers,
Ben.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v4 2/2] powerpc: make MPIC honor the "pic-no-reset" device tree property
On Thu, 2011-03-10 at 11:23 -0600, Meador Inge wrote: > I agree. I shouldn't have cached that. We should probably introduce a > helper function to get the cpuid, though. The: > > unsigned int cpu = 0; > > if (mpic->flags & MPIC_PRIMARY) > cpu = hard_smp_processor_id(); > > code is scattered in three places: '_mpic_cpu_write', '_mpic_cpu_read', > and 'mpic_init'. Right, but that code must act on the current CPU, not a cached per-MPIC variant. Doing a helper to factor the above 2 lines is fine if you wish to do so but then make it a separate patch. Cheers, Ben. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v4 2/2] powerpc: make MPIC honor the "pic-no-reset" device tree property
> diff --git a/arch/powerpc/include/asm/mpic.h b/arch/powerpc/include/asm/mpic.h
> index e000cce..7e1be12 100644
> --- a/arch/powerpc/include/asm/mpic.h
> +++ b/arch/powerpc/include/asm/mpic.h
> @@ -325,6 +325,8 @@ struct mpic
> #ifdef CONFIG_PM
> struct mpic_irq_save*save_data;
> #endif
> +
> + int cpu;
> };
Why ? The MPIC isn't specifically associated with a CPU and whatever we
pick as default during boot isn't relevant later on, so I don't see why
we should have global permanent state here.
>
> +static inline void mpic_init_vector(struct mpic *mpic, int source)
> +{
> + /* start with vector = source number, and masked */
> + u32 vecpri = MPIC_VECPRI_MASK | source | (8 <<
> MPIC_VECPRI_PRIORITY_SHIFT);
> +
> + /* init hw */
> + mpic_irq_write(source, MPIC_INFO(IRQ_VECTOR_PRI), vecpri);
> + mpic_irq_write(source, MPIC_INFO(IRQ_DESTINATION), 1 << mpic->cpu);
> +}
Just pass the CPU as an argument... but better... just don't do that,
put the code back where it was and ... see below :-)
> /* Check if we have one of those nice broken MPICs with a flipped endian on
> @@ -622,6 +631,14 @@ static unsigned int mpic_is_ipi(struct mpic *mpic,
> unsigned int irq)
> return (src >= mpic->ipi_vecs[0] && src <= mpic->ipi_vecs[3]);
> }
>
> +/* Determine if the linux irq is a timer interrupt */
> +static unsigned int mpic_is_timer_interrupt(struct mpic *mpic, unsigned int
> irq)
> +{
> + unsigned int src = mpic_irq_to_hw(irq);
> +
> + return (src >= mpic->timer_vecs[0] && src <= mpic->timer_vecs[3]);
> +}
> +
>
> /* Convert a cpu mask from logical to physical cpu numbers. */
> static inline u32 mpic_physmask(u32 cpumask)
> @@ -967,6 +984,15 @@ static int mpic_host_map(struct irq_host *h, unsigned
> int virq,
> if (hw >= mpic->irq_count)
> return -EINVAL;
>
> + /* If the MPIC was reset, then all vectors have already been
> + * initialized. Otherwise, the appropriate vector needs to be
> + * initialized here to ensure that only used sources are setup with
> + * a vector.
> + */
> + if (mpic->flags & MPIC_NO_RESET)
> + if (!(mpic_is_ipi(mpic, hw) || mpic_is_timer_interrupt(mpic,
> hw)))
> + mpic_init_vector(mpic, hw);
> +
The above isn't useful. Of those two registers you want to initialize,
afaik, one (the destination) will be initialized by the core calling
into set_affinity when the interrupt is requested, and the other one is
partially initialized in set_type, I'd say just modify set_type to
initialize the source as well, and problem solved, no ? Or is there a
chance that the interrupt was left unmasked ? I think it would be kosher
to mask it in set_type unconditionally, I don't think it's ever supposed
to be called for an enabled interrupt.
> mpic_msi_reserve_hwirq(mpic, hw);
>
> /* Default chip */
> @@ -1033,6 +1059,11 @@ static struct irq_host_ops mpic_host_ops = {
> .xlate = mpic_host_xlate,
> };
>
> +static int mpic_reset_prohibited(struct device_node *node)
> +{
> + return node && of_get_property(node, "pic-no-reset", NULL);
> +}
> +
> /*
> * Exported functions
> */
> @@ -1153,7 +1184,16 @@ struct mpic * __init mpic_alloc(struct device_node
> *node,
> mpic_map(mpic, node, paddr, &mpic->tmregs, MPIC_INFO(TIMER_BASE),
> 0x1000);
>
> /* Reset */
> - if (flags & MPIC_WANTS_RESET) {
> +
> + /* When using a device-node, reset requests are only honored if the MPIC
> + * is allowed to reset.
> + */
> + if (mpic_reset_prohibited(node)) {
> + mpic->flags |= MPIC_NO_RESET;
> + }
No { } for single line nested statements
> + if ((flags & MPIC_WANTS_RESET) && !(mpic->flags & MPIC_NO_RESET)) {
> + printk(KERN_DEBUG "mpic: Resetting\n");
> mpic_write(mpic->gregs, MPIC_INFO(GREG_GLOBAL_CONF_0),
> mpic_read(mpic->gregs, MPIC_INFO(GREG_GLOBAL_CONF_0))
> | MPIC_GREG_GCONF_RESET);
> @@ -1270,7 +1310,6 @@ void __init mpic_set_default_senses(struct mpic *mpic,
> u8 *senses, int count)
> void __init mpic_init(struct mpic *mpic)
> {
> int i;
> - int cpu;
>
> BUG_ON(mpic->num_sources == 0);
>
> @@ -1314,21 +1353,17 @@ void __init mpic_init(struct mpic *mpic)
> mpic_pasemi_msi_init(mpic);
>
> if (mpic->flags & MPIC_PRIMARY)
> - cpu = hard_smp_processor_id();
> + mpic->cpu = hard_smp_processor_id();
> else
> - cpu = 0;
> + mpic->cpu = 0;
Get rid of all that.
> - for (i = 0; i < mpic->num_sources; i++) {
> - /* start with vector = source number, and masked */
> - u32 vecpri = MPIC_VECPRI_MASK | i |
> - (8 << MPIC_VECPRI_PRIORITY_SHIFT);
> -
> - /* check if protected */
> - if (mpic->protected && test_bit(i, mpic->protected))
> - co
