* Re: [RFC PATCH 11/19] powerpc: gamecube/wii: flipper interrupt controller support
From: Segher Boessenkool @ 2009-11-26 21:52 UTC (permalink / raw)
To: Albert Herranz; +Cc: linuxppc-dev
In-Reply-To: <4B0D65C5.8090304@yahoo.es>
>> Maybe using FLIPPER (or GAMECUBE_FLIPPER) instead of GAMECUBE_COMMON
>> is a good name?
>
> I'd prefer to not use a name that implies a specific hardware to
> describe two (mostly) similar but different hardwares.
Hollywood is 100% compatible to Flipper though.
>>> +/*
>>> + * Each interrupt has a corresponding bit in both
>>> + * the Interrupt Cause (ICR) and Interrupt Mask (IMR) registers.
>>> + *
>>> + * Enabling/disabling an interrupt line involves asserting/clearing
>>> + * the corresponding bit in IMR. ACK'ing a request simply involves
>>> + * asserting the corresponding bit in ICR.
>>> + */
I looked it up in YAGCD; it says that _reading_ the ICR reg already
acks all interrupts (and clears the bits), you never write this reg!
>>> +static void flipper_pic_ack(unsigned int virq)
>>> +{
>>> + int irq = virq_to_hw(virq);
>>> + void __iomem *io_base = get_irq_chip_data(virq);
>>> +
>>> + set_bit(irq, io_base + FLIPPER_ICR);
>>> +}
>>
>> So it should be a simple write instead of an RMW here, right?
>> As it is you are ACKing _all_ IRQs as far as I can see.
>>
>
> No, it acks just a single IRQ.
No it doesn't. Say IRQs 1 and 3 are asserted, so the reg contains 0x0a.
Now you want to ack IRQ1; set_bit() will write 0x0a | 0x02, not just
0x02.
Segher
^ permalink raw reply
* Re: [PATCH 04/11] of/flattree: eliminate cell_t typedef
From: Benjamin Herrenschmidt @ 2009-11-26 21:40 UTC (permalink / raw)
To: Segher Boessenkool
Cc: sfr, microblaze-uclinux, devicetree-discuss, sparclinux,
linuxppc-dev, davem
In-Reply-To: <F307E7FB-0819-47E8-9CF4-CA1433C18DA6@kernel.crashing.org>
On Thu, 2009-11-26 at 22:36 +0100, Segher Boessenkool wrote:
> >> You're right, it's not, but makes merging less complex, and then I
> >> can
> >> refactor properly.
> >
> > Still, make them __be32 at least
>
> There is no alignment guarantee at all either, better make it all u8
> and use accessor functions everywhere.
Well... if you want to force using an accessor, then make it an opaque
type. But __be32 is fine. It doesn't necessarily convey alignment and
besides, there happens to -be- aligned in almost all cases so far :-)
The flat tree format guarantees 32-bit alignment for the start of a
property, so we are good here I think.
Cheers,
Ben.
^ permalink raw reply
* Re: [RFC PATCH 04/19] powerpc: wii: device tree
From: Albert Herranz @ 2009-11-26 21:38 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev
In-Reply-To: <1259269265.18084.14.camel@pasglop>
Benjamin Herrenschmidt wrote:
>>> Good point. I can't even guarantee that the kernel will work reliably
>>> with nobats :-) At least you really want the kernel .text to be fully
>>> covered by an IBAT.
>>>
>> It works with nobats.
>
> But does it work -reliably- ? :-)
>
It does AFAICT. My Wii is a 24x7 linux box although it is not stressed in any way usually.
> Any ways, not a big deal right now, as I said, we really want the BATs
> for performances anyways, so we should probably just add some kind of
> hack in mmu_mapin_ram() for the time being.
>
Yup. The idea is to map the first 16MB of MEM1 with a BAT.
Mapping the whole 24MB with BATs may not be possible because we want the framebuffer in MEM1 for performance reasons.
>> I must say that all the patches posted (and the device drivers, which haven't
>> been posted yet) are tested and working code.
>
> That was my impression too, but in this case, I'm talking about a
> potential very hard to hit problem that you may well have never managed
> to actually trigger.
>
I haven't actually, if that applies :)
>> There you can find the hardware interface that supports the IPC mechanism.
>> It is made up of a pair of registers to pass data between the processors and a
>> pair of control/flags registers.
>> The hardware can interrupt the PowerPC side when there is data available for it.
>
> Ok. So the right way to do that would be to have a node purely
> representing the HW IPC, unrelated to whatever is running on the
> secondary processor.
>
Totally agreed.
> However, it's ok to have -below- that node, a set of device nodes or a
> node with properties or whatever representing the FW in there and the
> function it exposes.
>
> That can be discussed later tho. I'm not that keen on having those info
> be in the .dts coming with the kernel since those functions essentially
> depend on what FW is loaded on the aux. processor.
>
I think we can leave this for later as you said.
> What might do however is to have a way for that FW itself to provide you
> with the nodes and properties for the functions it provides :-) Then you
> can have the boot wrapper or the kernel platform init code use some well
> defined (and as stable as possible) IPC API to identify the FW there and
> expose all that stuff.
>
Segher was playing with an OF implementation...
> Of course that wouldn't work with FW we don't have control on. Can Linux
> run on the wii with the original N. FW on the aux. processor ? Can we
> detect what is running there ? Do we care ?
>
Yes, it can. And it is done. But I think we don't care/need that in mainline.
>> It is what Nintendo calls the "Serial Interface" (SI) which is a proprietary
>> serial hardware to drive the gamepads (and a custom keyboard too, once available
>> for an RPG game).
>
> So I would give it a different name than "serial" then. Make it gpsi
> maybe ? (game pad serial interface ?) :-) Or invent something else...
>
I'd choose "gcnsi" with a compatible like "nintendo,gamecube-si" :)
(gcn is the official short abbreviation for the Nintendo GameCube)
> Cheers,
> Ben.
>
Thanks,
Albert
^ permalink raw reply
* Re: [PATCH 04/11] of/flattree: eliminate cell_t typedef
From: Segher Boessenkool @ 2009-11-26 21:36 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: sfr, microblaze-uclinux, devicetree-discuss, sparclinux,
linuxppc-dev, davem
In-Reply-To: <1259213266.16367.274.camel@pasglop>
>> You're right, it's not, but makes merging less complex, and then I
>> can
>> refactor properly.
>
> Still, make them __be32 at least
There is no alignment guarantee at all either, better make it all u8
and use accessor functions everywhere.
Segher
^ permalink raw reply
* Re: [RFC PATCH 04/19] powerpc: wii: device tree
From: Albert Herranz @ 2009-11-26 21:20 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev
In-Reply-To: <1259268528.18084.2.camel@pasglop>
Benjamin Herrenschmidt wrote:
> On Thu, 2009-11-26 at 16:09 +0100, Albert Herranz wrote:
>
>>> Are the above OHCI and EHCI "special" ? If not, there's an existing
>>> binding for that sort of thing, which btw requires properties to
>>> indicate the endianness of the registers and in-memory data structures
>>> etc...
>>>
>> They are a bit special because registers are in reverse little endian format,
>> must be written in 32-bit chunks, and (all things point to) they have hardware bugs.
>
> Well.. first what is "reverse little endian" ? :-) Big endian ?
>
It's the same case as the register access for the Freescale eSDHC controller, which is already in the existing sdhci-of driver.
> The OHCI driver today has separate flags to control register endianness
> and memory based data structures endianness.
>
> I think we also only use 32-bit reads and writes no ? So that should be
> covered :-)
>
There are 8, 16 and 32 bit accesses. We need the same workarounds as present for the Freescale eSDHC controller.
> As for HW bugs, well, as long as we know them we can define a quirk bit
> and add the necessary workarounds to the driver :-)
>
Yes, that's how it was done.
Although it it currently not integrated into sdhci-of. I think that the specific eSDHC code should be moved to another file, and leave the sdhci-of with the generic code and the sdhci_of_match table. Then adding a similar driver would simply consist in adding another file with the sdhci_data (containing ops and quirks) and adding an entry to the sdhci_of_match table in sdhci-of.
> Do you have a patch somewhere that adds the OCHI and EHCI support btw ?
>
Yes, too. Although it needs review and cleanup too.
I have a git tree with all the stuff, but it contains only the existing code previous to the effort of mainlining it:
http://git.infradead.org/users/herraa1/gc-linux-2.6.git/shortlog/refs/heads/mini/gc-linux-2.6.31
> Cheers,
> Ben.
>
Thanks,
Albert
^ permalink raw reply
* Re: [RFC] powerpc/mm: honor O_SYNC flag for memory map
From: Segher Boessenkool @ 2009-11-26 21:26 UTC (permalink / raw)
To: Li Yang; +Cc: linuxppc-dev, Li Yang-R58472
In-Reply-To: <2a27d3730911250007s62f31673p3358631996e8a451@mail.gmail.com>
> So what you are saying is that if the kernel has mapped a physical
> page as cacheable while user application is trying to map it as
> non-cacheable, there will be machine checks and checkstops rather than
> just performance drop? This is new to me. Could you elaborate a bit?
If some data is in cache at a certain physical address, and you then
do an uncached read from that address, on at least some CPUs both the
bus interface and the cache will reply. Bang, machine check.
Writes are problematic as well.
Segher
^ permalink raw reply
* Re: [RFC PATCH 16/19] powerpc: wii: hollywood interrupt controller support
From: Benjamin Herrenschmidt @ 2009-11-26 21:13 UTC (permalink / raw)
To: Albert Herranz; +Cc: linuxppc-dev
In-Reply-To: <4B0EA1E8.6070603@yahoo.es>
On Thu, 2009-11-26 at 16:42 +0100, Albert Herranz wrote:
> Benjamin Herrenschmidt wrote:
> > On Sun, 2009-11-22 at 23:01 +0100, Albert Herranz wrote:
> >
> >> +static void hlwd_pic_mask_and_ack(unsigned int virq)
> >> +{
> >> + int irq = virq_to_hw(virq);
> >> + void __iomem *io_base = get_irq_chip_data(virq);
> >> +
> >> + clear_bit(irq, io_base + HW_BROADWAY_IMR);
> >> + set_bit(irq, io_base + HW_BROADWAY_ICR);
> >> +}
> >
> > Same comment as with flipper. BTW. It looks really similar, can't you
> > use the same driver ?
> >
>
> I'll look at it. Surely, parts of it would be mergeable.
If it's too much work, we can do that later. No big deal.
Cheers,
Ben.
> > Cheers,
> > Ben.
> >
>
> Thanks,
> Albert
>
> >
> >
^ permalink raw reply
* Re: [RFC PATCH 14/19] powerpc: allow ioremap within reserved fake ram regions
From: Benjamin Herrenschmidt @ 2009-11-26 21:13 UTC (permalink / raw)
To: Albert Herranz; +Cc: linuxppc-dev
In-Reply-To: <4B0EA036.1090901@yahoo.es>
On Thu, 2009-11-26 at 16:35 +0100, Albert Herranz wrote:
> Benjamin Herrenschmidt wrote:
> > On Tue, 2009-11-24 at 18:09 +0100, Albert Herranz wrote:
> >> I could use ppc_md.ioremap to duplicate ioremap except for the ioremap
> >> ram check.
> >> But calling the stock ioremap without modifying it is not possible
> >> because it checks and bails out when ioremapping a region marked as
> >> ram (even if it's not real ram and it's memreserved).
> >>
> >> Is the list of memreserved areas preserved once the kernel is running?
> >> If it is preserved another option would be to unban ioremapping ram if
> >> memreserved.
> >
> > Yes, check lmb's they should still be around.
> >
>
> Good.
> So adding a kconfig option to allow ioremapping memreserved memory marked
> as ram and adding a proper check (under that kconfig) to unban this case
> in ioremap would be an acceptable solution?
Don't even make it Kconfig. Stick it under you GAMECUBE_COMMON for now
or CONFIG_WII or whatever. This is temporary until we sort out the whole
disctontig mem issue.
Cheers,
Ben.
> > Cheers,
> > Ben.
> >
>
> Thanks,
> Albert
^ permalink raw reply
* Re: [RFC PATCH 11/19] powerpc: gamecube/wii: flipper interrupt controller support
From: Benjamin Herrenschmidt @ 2009-11-26 21:12 UTC (permalink / raw)
To: Albert Herranz; +Cc: linuxppc-dev
In-Reply-To: <4B0E9FBC.1090900@yahoo.es>
On Thu, 2009-11-26 at 16:33 +0100, Albert Herranz wrote:
> Benjamin Herrenschmidt wrote:
> > On Sun, 2009-11-22 at 16:28 -0700, Grant Likely wrote:
> >>> +unsigned int flipper_pic_get_irq(void)
> >>> +{
> >>> + void __iomem *io_base = flipper_irq_host->host_data;
> >>> + int irq;
> >>> + u32 irq_status;
> >>> +
> >>> + irq_status = in_be32(io_base + FLIPPER_ICR) &
> >>> + in_be32(io_base + FLIPPER_IMR);
> >>> + if (irq_status == 0)
> >>> + return -1; /* no more IRQs pending */
> >> NO_IRQ_IGNORE
> >
> > Why no just 0 ? (aka NO_IRQ)
> >
>
> I didn't know about 0 (I thought that was another valid interrupt).
0 is a valid hw number. It's not a valid linux "virq" number. In fact,
that's part of the reason why we generalized the whole virq thingy :-)
Because back then, Linus made the statement that 0 should never be a
valid IRQ number in linux. That also allowed us to make sure that 1..15
are never allocated to anything but a legacy 8259 to avoid all sorts of
problems with crackpot legacy x86 originated drivers who hard coded some
of these interrupts.
> I was used to -1 to tell that no IRQs were pending (at least from the
> ARCH=ppc days) :)
Yup, we changed that :-)
> > Or do you know you are getting lots of spurrious that you don't want to
> > account ?
> >
>
> No, this is not the case here.
Cheers,
Ben.
> > Cheers,
> > Ben.
> >
>
> Thanks,
> Albert
^ permalink raw reply
* Re: [RFC PATCH 11/19] powerpc: gamecube/wii: flipper interrupt controller support
From: Benjamin Herrenschmidt @ 2009-11-26 21:10 UTC (permalink / raw)
To: Albert Herranz; +Cc: linuxppc-dev
In-Reply-To: <4B0E9F11.5000605@yahoo.es>
On Thu, 2009-11-26 at 16:30 +0100, Albert Herranz wrote:
> Benjamin Herrenschmidt wrote:
> > On Sun, 2009-11-22 at 23:01 +0100, Albert Herranz wrote:
> >
> >> +static void flipper_pic_mask_and_ack(unsigned int virq)
> >> +{
> >> + int irq = virq_to_hw(virq);
> >> + void __iomem *io_base = get_irq_chip_data(virq);
> >> +
> >> + clear_bit(irq, io_base + FLIPPER_IMR);
> >> + set_bit(irq, io_base + FLIPPER_ICR);
> >> +}
> >
> > Do not use clear_bit and set_bit on IOs. They will do lwarx/stwcx. which
> > is really not what you want. You can use __clear_bit and __set_bit but
> > it's still fishy. Those operate on unsigned long, so the size vary
> > between 32 and 64 bit etc... not something you care that much about, but
> > it's still the wrong tool for the job.
> >
> > Do those guys have more than 32 interrupts ? If not, just hand
> > code the msak & shifts. If they do, then maybe stick with __clear_bit()
> > and __set_bit() which are the non atomic variants.
> >
>
> There can be only 32 interrupt sources per pic.
> I'll build a mask and check if just a simple write works too (it should IMHO), instead of a RWM.
For the ICR just writes should be ok. For the IMR, I wouldn't be
surprised if you need to RMW. In which case you have two options:
- One is you just do RMW :-) As long as you don't do SMP, you don't
have a problem since these callbacks are generally called with local
IRQs off.
- Or you could explicitely add a spinlock_irqsave just out of paranoia
or if you envision SMP type access to these things (from the aux
processor ? But then the spinlock would have to be shared with the aux
ptiocrocessor FW... fun)
- In any case, clear_bit and set_bit are the wrong accessors since
lwarx and stwcx. are definitely the wrong instructions to use for non
cachable accesses. On other processor you probably would have blown up
here in fact. Also those accessors don't provide any memory barriers
which mean you may even have some ordering issues. Use readl_be() and
writel_be() (or the ppc specific in_be32/out_be32) or the _le variants
if you want to count bits backward :-)
- You could also keep in memory cache of what the mask is supposed to
be. You RMW the cache and do a simple store to the register. It won't
fix the concurrency problem (that you probably don't have anyways) but
it will make things faster by avoiding an MMIO load....
- However you probably need that MMIO load anyways :-) Trick is, the
store you do to ack and mask an interrupt may take some time to hit the
PIC before the said PIC actually lowers the CPU EE line. Enough time for
your code to go back, re-enable MSR:EE and start calling into the
handlers. Enough time thus to catch a "spurrious" interrupt. Old pmacs
have a similar PIC and have the same problem. I would recommend that
after a mask or a mask & ack, you also read back the register. The MMIO
accessors (readl, etc...) will use a twi;isync construct to ensure that
the load has been fully completed before execution continues which will
help.
- The construct and flow handler you use assume all interrupts are
level sensitive, this is the case right ?
Cheers,
Ben.
> > Cheers,
> > Ben.
> >
>
> Thanks,
> Albert
^ permalink raw reply
* Re: [RFC PATCH 09/19] powerpc: gamecube/wii: udbg support for usbgecko
From: Benjamin Herrenschmidt @ 2009-11-26 21:02 UTC (permalink / raw)
To: Albert Herranz; +Cc: linuxppc-dev
In-Reply-To: <4B0E9E84.4060400@yahoo.es>
On Thu, 2009-11-26 at 16:28 +0100, Albert Herranz wrote:
> Benjamin Herrenschmidt wrote:
> > On Sun, 2009-11-22 at 23:01 +0100, Albert Herranz wrote:
> >> Add support for using the USB Gecko adapter via the udbg facility on
> >> the Nintendo GameCube and Wii video game consoles.
> >> The USB Gecko is a 3rd party memory card interface adapter that provides
> >> a EXI (External Interface) to USB serial converter.
> >
> > The main issue here is that the gecko is supposed to be something you
> > can plug or unplug no ? Is it hotswap or only at boot ? In the later
> > case, maybe the node should be created by the boot wrapper when
> > detecting the presence of the device. If it's hotplug, then we need
> > something smarter...
> >
>
> The usbgecko is hotplugable and hotswappable.
> But as this is mostly a developer feature, not normaly used by end users,
> I think that we can just let it be as it is: autodetect it on boot (now probing
> for it instead of using information from the device tree).
> If you unplug it later it causes no errors, you just miss whatever data is sent to it.
Ack. We may still went ultimately to have some kind of nice hotplug
driver for that connector and we may even have a usbgecko driver for
that things "after boot" that creates areal tty etc... but for now, just
a hard probe will do just fine.
Of course a real OF implemention is welcome to create a device node for
it if it was indeed present at boot time.
Cheers,
Ben.
> > Cheers,
> > Ben.
> >
>
> Thanks,
> Albert
>
^ permalink raw reply
* Re: [RFC PATCH 04/19] powerpc: wii: device tree
From: Benjamin Herrenschmidt @ 2009-11-26 21:01 UTC (permalink / raw)
To: Albert Herranz; +Cc: linuxppc-dev
In-Reply-To: <4B0E9C5F.50304@yahoo.es>
> > Good point. I can't even guarantee that the kernel will work reliably
> > with nobats :-) At least you really want the kernel .text to be fully
> > covered by an IBAT.
> >
>
> It works with nobats.
But does it work -reliably- ? :-)
I haven't looked at that for years, but we used to have a subtle issue
if you happen to take a hash miss on the kernel text or data in the
wrong time, such as when SRR0/SRR1 are modified and before a subsequent
rfi. This is very very hard to trigger and maybe impossible without SMP
but to keep in mind.
Paulus added some code ages ago to close most of these by using the
MSR:RI bit so that the hash code could detect the situation and branch
to some "recovery" code, but from memory, while that dealt with missing
D-BATs, we still had a potential window of problem if the kernel text
itself wasn't entirely covered.
Any ways, not a big deal right now, as I said, we really want the BATs
for performances anyways, so we should probably just add some kind of
hack in mmu_mapin_ram() for the time being.
> I must say that all the patches posted (and the device drivers, which haven't
> been posted yet) are tested and working code.
That was my impression too, but in this case, I'm talking about a
potential very hard to hit problem that you may well have never managed
to actually trigger.
> There you can find the hardware interface that supports the IPC mechanism.
> It is made up of a pair of registers to pass data between the processors and a
> pair of control/flags registers.
> The hardware can interrupt the PowerPC side when there is data available for it.
Ok. So the right way to do that would be to have a node purely
representing the HW IPC, unrelated to whatever is running on the
secondary processor.
However, it's ok to have -below- that node, a set of device nodes or a
node with properties or whatever representing the FW in there and the
function it exposes.
That can be discussed later tho. I'm not that keen on having those info
be in the .dts coming with the kernel since those functions essentially
depend on what FW is loaded on the aux. processor.
What might do however is to have a way for that FW itself to provide you
with the nodes and properties for the functions it provides :-) Then you
can have the boot wrapper or the kernel platform init code use some well
defined (and as stable as possible) IPC API to identify the FW there and
expose all that stuff.
Of course that wouldn't work with FW we don't have control on. Can Linux
run on the wii with the original N. FW on the aux. processor ? Can we
detect what is running there ? Do we care ?
> It is what Nintendo calls the "Serial Interface" (SI) which is a proprietary
> serial hardware to drive the gamepads (and a custom keyboard too, once available
> for an RPG game).
So I would give it a different name than "serial" then. Make it gpsi
maybe ? (game pad serial interface ?) :-) Or invent something else...
Cheers,
Ben.
> > Cheers,
> > Ben.
> >
>
> Thanks,
> Albert
^ permalink raw reply
* Re: [RFC PATCH 04/19] powerpc: wii: device tree
From: Benjamin Herrenschmidt @ 2009-11-26 20:48 UTC (permalink / raw)
To: Albert Herranz; +Cc: linuxppc-dev
In-Reply-To: <4B0E9A0F.9010203@yahoo.es>
On Thu, 2009-11-26 at 16:09 +0100, Albert Herranz wrote:
> > Are the above OHCI and EHCI "special" ? If not, there's an existing
> > binding for that sort of thing, which btw requires properties to
> > indicate the endianness of the registers and in-memory data structures
> > etc...
> >
>
> They are a bit special because registers are in reverse little endian format,
> must be written in 32-bit chunks, and (all things point to) they have hardware bugs.
Well.. first what is "reverse little endian" ? :-) Big endian ?
The OHCI driver today has separate flags to control register endianness
and memory based data structures endianness.
I think we also only use 32-bit reads and writes no ? So that should be
covered :-)
As for HW bugs, well, as long as we know them we can define a quirk bit
and add the necessary workarounds to the driver :-)
Do you have a patch somewhere that adds the OCHI and EHCI support btw ?
Cheers,
Ben.
^ permalink raw reply
* [PATCH v6 2/2] pseries: Serialize cpu hotplug operations during deactivate Vs deallocate
From: Vaidyanathan Srinivasan @ 2009-11-26 19:59 UTC (permalink / raw)
To: Nathan Fontenot, Benjamin Herrenschmidt
Cc: Peter Zijlstra, Gautham R Shenoy, linux-kernel, Arun R Bharadwaj,
Andrew Morton, linuxppc-dev, Ingo Molnar
In-Reply-To: <20091126195121.29159.66098.stgit@drishya>
From: Gautham R Shenoy <ego@in.ibm.com>
Currently the cpu-allocation/deallocation process comprises of two steps:
- Set the indicators and to update the device tree with DLPAR node
information.
- Online/offline the allocated/deallocated CPU.
This is achieved by writing to the sysfs tunables "probe" during allocation
and "release" during deallocation.
At the sametime, the userspace can independently online/offline the CPUs of
the system using the sysfs tunable "online".
It is quite possible that when a userspace tool offlines a CPU
for the purpose of deallocation and is in the process of updating the device
tree, some other userspace tool could bring the CPU back online by writing to
the "online" sysfs tunable thereby causing the deallocate process to fail.
The solution to this is to serialize writes to the "probe/release" sysfs
tunable with the writes to the "online" sysfs tunable.
This patch employs a mutex to provide this serialization, which is a no-op on
all architectures except PPC_PSERIES
Signed-off-by: Gautham R Shenoy <ego@in.ibm.com>
Acked-by: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
---
arch/powerpc/platforms/pseries/dlpar.c | 45 +++++++++++++++++++++++++-------
drivers/base/cpu.c | 2 +
include/linux/cpu.h | 13 +++++++++
3 files changed, 50 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index 642e1b2..fd2f0af 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -436,6 +436,18 @@ int dlpar_release_drc(u32 drc_index)
#ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
+static DEFINE_MUTEX(pseries_cpu_hotplug_mutex);
+
+void cpu_hotplug_driver_lock()
+{
+ mutex_lock(&pseries_cpu_hotplug_mutex);
+}
+
+void cpu_hotplug_driver_unlock()
+{
+ mutex_unlock(&pseries_cpu_hotplug_mutex);
+}
+
static ssize_t dlpar_cpu_probe(const char *buf, size_t count)
{
struct device_node *dn;
@@ -443,13 +455,18 @@ static ssize_t dlpar_cpu_probe(const char *buf, size_t count)
char *cpu_name;
int rc;
+ cpu_hotplug_driver_lock();
rc = strict_strtoul(buf, 0, &drc_index);
- if (rc)
- return -EINVAL;
+ if (rc) {
+ rc = -EINVAL;
+ goto out;
+ }
dn = dlpar_configure_connector(drc_index);
- if (!dn)
- return -EINVAL;
+ if (!dn) {
+ rc = -EINVAL;
+ goto out;
+ }
/* configure-connector reports cpus as living in the base
* directory of the device tree. CPUs actually live in the
@@ -459,7 +476,8 @@ static ssize_t dlpar_cpu_probe(const char *buf, size_t count)
GFP_KERNEL);
if (!cpu_name) {
dlpar_free_cc_nodes(dn);
- return -ENOMEM;
+ rc = -ENOMEM;
+ goto out;
}
sprintf(cpu_name, "/cpus%s", dn->full_name);
@@ -469,7 +487,8 @@ static ssize_t dlpar_cpu_probe(const char *buf, size_t count)
rc = dlpar_acquire_drc(drc_index);
if (rc) {
dlpar_free_cc_nodes(dn);
- return -EINVAL;
+ rc = -EINVAL;
+ goto out;
}
rc = dlpar_attach_node(dn);
@@ -479,6 +498,8 @@ static ssize_t dlpar_cpu_probe(const char *buf, size_t count)
}
rc = online_node_cpus(dn);
+out:
+ cpu_hotplug_driver_unlock();
return rc ? rc : count;
}
@@ -499,26 +520,30 @@ static ssize_t dlpar_cpu_release(const char *buf, size_t count)
return -EINVAL;
}
+ cpu_hotplug_driver_lock();
rc = offline_node_cpus(dn);
if (rc) {
of_node_put(dn);
- return -EINVAL;
+ rc = -EINVAL;
+ goto out;
}
rc = dlpar_release_drc(*drc_index);
if (rc) {
of_node_put(dn);
- return -EINVAL;
+ goto out;
}
rc = dlpar_detach_node(dn);
if (rc) {
dlpar_acquire_drc(*drc_index);
- return rc;
+ goto out;
}
of_node_put(dn);
- return count;
+out:
+ cpu_hotplug_driver_unlock();
+ return rc ? rc : count;
}
static int __init pseries_dlpar_init(void)
diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index 7c03af7..27fd775 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -35,6 +35,7 @@ static ssize_t __ref store_online(struct sys_device *dev, struct sysdev_attribut
struct cpu *cpu = container_of(dev, struct cpu, sysdev);
ssize_t ret;
+ cpu_hotplug_driver_lock();
switch (buf[0]) {
case '0':
ret = cpu_down(cpu->sysdev.id);
@@ -49,6 +50,7 @@ static ssize_t __ref store_online(struct sys_device *dev, struct sysdev_attribut
default:
ret = -EINVAL;
}
+ cpu_hotplug_driver_unlock();
if (ret >= 0)
ret = count;
diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index c972f7c..e287863 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -117,6 +117,19 @@ extern void put_online_cpus(void);
#define unregister_hotcpu_notifier(nb) unregister_cpu_notifier(nb)
int cpu_down(unsigned int cpu);
+#ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
+extern void cpu_hotplug_driver_lock(void);
+extern void cpu_hotplug_driver_unlock(void);
+#else
+static inline void cpu_hotplug_driver_lock(void)
+{
+}
+
+static inline void cpu_hotplug_driver_unlock(void)
+{
+}
+#endif
+
#else /* CONFIG_HOTPLUG_CPU */
#define get_online_cpus() do { } while (0)
^ permalink raw reply related
* [PATCH v6 1/2] pseries: Add code to online/offline CPUs of a DLPAR node
From: Vaidyanathan Srinivasan @ 2009-11-26 19:58 UTC (permalink / raw)
To: Nathan Fontenot, Benjamin Herrenschmidt
Cc: Peter Zijlstra, Gautham R Shenoy, linux-kernel, Arun R Bharadwaj,
Andrew Morton, linuxppc-dev, Ingo Molnar
In-Reply-To: <20091126195121.29159.66098.stgit@drishya>
From: Gautham R Shenoy <ego@in.ibm.com>
Currently the cpu-allocation/deallocation on pSeries is a
two step process from the Userspace.
- Set the indicators and update the device tree by writing to the sysfs
tunable "probe" during allocation and "release" during deallocation.
- Online / Offline the CPUs of the allocated/would_be_deallocated node by
writing to the sysfs tunable "online".
This patch adds kernel code to online/offline the CPUs soon_after/just_before
they have been allocated/would_be_deallocated. This way, the userspace tool
that performs DLPAR operations would only have to deal with one set of sysfs
tunables namely "probe" and release".
Signed-off-by: Gautham R Shenoy <ego@in.ibm.com>
Signed-off-by: Nathan Fontenot <nfont@austin.ibm.com>
Acked-by: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
---
arch/powerpc/platforms/pseries/dlpar.c | 101 ++++++++++++++++++++++++++++++++
1 files changed, 101 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index fe8d4b3..642e1b2 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -16,6 +16,7 @@
#include <linux/proc_fs.h>
#include <linux/spinlock.h>
#include <linux/cpu.h>
+#include "offline_states.h"
#include <asm/prom.h>
#include <asm/machdep.h>
@@ -287,6 +288,98 @@ int dlpar_detach_node(struct device_node *dn)
return 0;
}
+int online_node_cpus(struct device_node *dn)
+{
+ int rc = 0;
+ unsigned int cpu;
+ int len, nthreads, i;
+ const u32 *intserv;
+
+ intserv = of_get_property(dn, "ibm,ppc-interrupt-server#s", &len);
+ if (!intserv)
+ return -EINVAL;
+
+ nthreads = len / sizeof(u32);
+
+ cpu_maps_update_begin();
+ for (i = 0; i < nthreads; i++) {
+ for_each_present_cpu(cpu) {
+ if (get_hard_smp_processor_id(cpu) != intserv[i])
+ continue;
+ BUG_ON(get_cpu_current_state(cpu)
+ != CPU_STATE_OFFLINE);
+ cpu_maps_update_done();
+ rc = cpu_up(cpu);
+ if (rc)
+ goto out;
+ cpu_maps_update_begin();
+
+ break;
+ }
+ if (cpu == num_possible_cpus())
+ printk(KERN_WARNING "Could not find cpu to online "
+ "with physical id 0x%x\n", intserv[i]);
+ }
+ cpu_maps_update_done();
+
+out:
+ return rc;
+
+}
+
+int offline_node_cpus(struct device_node *dn)
+{
+ int rc = 0;
+ unsigned int cpu;
+ int len, nthreads, i;
+ const u32 *intserv;
+
+ intserv = of_get_property(dn, "ibm,ppc-interrupt-server#s", &len);
+ if (!intserv)
+ return -EINVAL;
+
+ nthreads = len / sizeof(u32);
+
+ cpu_maps_update_begin();
+ for (i = 0; i < nthreads; i++) {
+ for_each_present_cpu(cpu) {
+ if (get_hard_smp_processor_id(cpu) != intserv[i])
+ continue;
+
+ if (get_cpu_current_state(cpu) == CPU_STATE_OFFLINE)
+ break;
+
+ if (get_cpu_current_state(cpu) == CPU_STATE_ONLINE) {
+ cpu_maps_update_done();
+ rc = cpu_down(cpu);
+ if (rc)
+ goto out;
+ cpu_maps_update_begin();
+ break;
+
+ }
+
+ /*
+ * The cpu is in CPU_STATE_INACTIVE.
+ * Upgrade it's state to CPU_STATE_OFFLINE.
+ */
+ set_preferred_offline_state(cpu, CPU_STATE_OFFLINE);
+ BUG_ON(plpar_hcall_norets(H_PROD, intserv[i])
+ != H_SUCCESS);
+ __cpu_die(cpu);
+ break;
+ }
+ if (cpu == num_possible_cpus())
+ printk(KERN_WARNING "Could not find cpu to offline "
+ "with physical id 0x%x\n", intserv[i]);
+ }
+ cpu_maps_update_done();
+
+out:
+ return rc;
+
+}
+
#define DR_ENTITY_SENSE 9003
#define DR_ENTITY_PRESENT 1
#define DR_ENTITY_UNUSABLE 2
@@ -385,6 +478,8 @@ static ssize_t dlpar_cpu_probe(const char *buf, size_t count)
dlpar_free_cc_nodes(dn);
}
+ rc = online_node_cpus(dn);
+
return rc ? rc : count;
}
@@ -404,6 +499,12 @@ static ssize_t dlpar_cpu_release(const char *buf, size_t count)
return -EINVAL;
}
+ rc = offline_node_cpus(dn);
+ if (rc) {
+ of_node_put(dn);
+ return -EINVAL;
+ }
+
rc = dlpar_release_drc(*drc_index);
if (rc) {
of_node_put(dn);
^ permalink raw reply related
* [PATCH v6 0/2] pseries: Add cede support for cpu-offline
From: Vaidyanathan Srinivasan @ 2009-11-26 19:58 UTC (permalink / raw)
To: Nathan Fontenot, Benjamin Herrenschmidt
Cc: Peter Zijlstra, Gautham R Shenoy, linux-kernel, Arun R Bharadwaj,
Andrew Morton, linuxppc-dev, Ingo Molnar
Hi,
This is version 6 of patch series that provides a framework to choose the state
a pseries CPU must be put to when it is offlined.
Previous versions can be found here:
Version 5: http://lkml.org/lkml/2009/10/30/6
Version 4: http://lkml.org/lkml/2009/10/9/59
Version 3: http://lkml.org/lkml/2009/9/15/164
Version 2: http://lkml.org/lkml/2009/8/28/102
Version 1: http://lkml.org/lkml/2009/8/6/236
Changes from the previous version include:
- Built on Nathan Fontenot's v3 of "Kernel handling of Dynamic
Logical Partitioning" http://lkml.org/lkml/2009/11/25/21
- Rebased to powerpc.git tree and hence dropped 1st and 2nd patch in
the stack since they are already in the powerpc.git tree:
With reference to previous version,
Dropped:
1/4 pSeries: extended_cede_processor() helper function
2/4 pSeries: Add hooks to put the CPU into an appropriate offline state
Posting only:
3/4 pseries: Add code to online/offline CPUs of a DLPAR node
4/4 pseries: Serialize cpu hotplug operations during deactivate Vs deallocate
Minor changes in the above patchs due to changes in Nathan's routines.
Also,
- This approach addresses Peter Z's objections regarding layering
violations. The user simply offlines the cpu and doesn't worry about what
state the CPU should be put into. That part is automatically handled by the
kernel.
Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
http://lkml.org/lkml/2009/11/11/328
- It does not add any additional sysfs interface instead uses the existing
sysfs interface to offline CPUs.
- On platforms which do not have support for ceding the vcpu with a
latency specifier value, the offlining mechanism defaults to the current
method of calling rtas_stop_self().
This patchset is based on powerpc.git + Nathan's patches and has been built and
tested on pseries platforms. This series can be applied to powerpc.git after
Nathan's patches.
Thanks,
Vaidy
---
Gautham R Shenoy (2):
pseries: Serialize cpu hotplug operations during deactivate Vs deallocate
pseries: Add code to online/offline CPUs of a DLPAR node
arch/powerpc/platforms/pseries/dlpar.c | 144 ++++++++++++++++++++++++++++++--
drivers/base/cpu.c | 2
include/linux/cpu.h | 13 +++
3 files changed, 150 insertions(+), 9 deletions(-)
--
^ permalink raw reply
* Re: [PATCH] Reserve memory for kdump kernel within RMO region
From: Bernhard Walle @ 2009-11-26 19:26 UTC (permalink / raw)
To: M. Mohan Kumar; +Cc: kexec, ppcdev
In-Reply-To: <4B0E628A.9070009@in.ibm.com>
M. Mohan Kumar schrieb:
> On 11/26/2009 12:22 AM, Bernhard Walle wrote:
>> M. Mohan Kumar schrieb:
>>> Reserve memory for kdump kernel within RMO region
>>>
>>> When the kernel size exceeds 32MB(observed with some distros), memory
>>> for kdump kernel can not be reserved as kdump kernel base is assumed to
>>> be 32MB always. When the kernel has CONFIG_RELOCATABLE option enabled,
>>> provide the feature to reserve the memory for kdump kernel anywhere in
>>> the RMO region.
>
> Hi Bernhard,
>
>> Correct me if I'm wrong, but: CONFIG_RELOCATABLE is for the kernel that
>> gets loaded as crashkernel, not for the kernel that loads the
>> crashkernel. So it would be perfectly fine that a kernel that has not
>> CONFIG_RELOCATABLE set would load another kernel that has
>> CONFIG_RELOCATABLE set on an address != 32 M.
>
> No, with relocatable option, the same kernel is used as both production
> and kdump kernel.
Can be, but it's not strictly necessary. It depends what userland does.
Especially it's possible that a non-relocatable, self-compiled kernel
loads a relocatable distribution kernel as capture kernel.
Also, it would make sense to make the behaviour symmetric across
platforms. Currently we have:
- x86 and ia64: Without offset on command line, use any offset
With offset on command line, use that offset and fail
if no memory is available at that offset.
- ppc64: Always use 32M and ignore the offset.
If your patch gets applied, we have:
- ppc64: With CONFIG_RELOCATABLE, use any offset
With offset on command
I don't see why the behaviour on ppc64 should be completely different.
Having maintained kdump for SUSE for x86, ia64 and partly ppc64 in the
past, I always felt that ppc64 is more different from x86 than ia64 is
from x86. That's one more step into that direction without a technical
reason.
Having that all said: If your patch gets in mainline kernel, than we
should change the behaviour also for x86 and ia64.
Regards,
Bernhard
^ permalink raw reply
* Re: spi_mpc8xxx.c: chip select polarity problem
From: Grant Likely @ 2009-11-26 19:17 UTC (permalink / raw)
To: avorontsov; +Cc: Torsten Fleischer, spi-devel-general, linuxppc-dev
In-Reply-To: <20091126190101.GA19404@oksana.dev.rtsoft.ru>
On Thu, Nov 26, 2009 at 12:01 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Thu, Nov 26, 2009 at 11:50:05AM -0700, Grant Likely wrote:
>> On Thu, Nov 26, 2009 at 11:41 AM, Anton Vorontsov
>> <avorontsov@ru.mvista.com> wrote:
>> > On Thu, Nov 26, 2009 at 11:16:34AM -0700, Grant Likely wrote:
>> > [...]
>> >> The spi-cs-high property is defined in
>> >> Documentation/powerpc/dts-bindings/spi-bus.txt, but it definitely was
>> >> a mistake
>> >
>> > Yup.
>> >
>> >> Currently the spi-cs-high property is parsed in the
>> >> of_register_spi_devices() function, but the CS polarity needs to be
>> >> known before registering devices. =A0It needs to be factored out into
>> >> another utility function callable by spi bus drivers so that it can
>> >> get polarity data at probe time.
>> >
>> > Untill we have this, Torsten's patch is a real improvement, and
>> > works for non-broken hw/fw.
>> >
>> > So I think it should be applied.
>>
>> I disagree since it only band-aids the problem and uglifies the driver
>> in the process. =A0In the immediate term the driver needs to be changed
>> to read the spi-cs-high property out of the child nodes before
>> registering the devices.
>
> Hm. I thought we agreed that spi-cs-high is not good? Why do you
> encourage using it then? We'll have to uglify the driver with
> legacy device-tree handling code.
spi-cs-high is definitely not a complete solution, but it isn't
actively evil either. Plus it is documented and (presumably) in
active use. so support for it should not be dropped.
Regardless, there needs to be a library function for parsing all the
SPI child nodes and returning the active state for each GPIO chip
select. All the code for parsing the old spi-cs-high properties can
be contained in the same place as a new yet-to-be-defined bus node cs
polarity property. The rework to the driver itself is not ugly.
g.
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: spi_mpc8xxx.c: chip select polarity problem
From: Anton Vorontsov @ 2009-11-26 19:01 UTC (permalink / raw)
To: Grant Likely; +Cc: Torsten Fleischer, spi-devel-general, linuxppc-dev
In-Reply-To: <fa686aa40911261050g3f7ae231mc82c41f058ff8530@mail.gmail.com>
On Thu, Nov 26, 2009 at 11:50:05AM -0700, Grant Likely wrote:
> On Thu, Nov 26, 2009 at 11:41 AM, Anton Vorontsov
> <avorontsov@ru.mvista.com> wrote:
> > On Thu, Nov 26, 2009 at 11:16:34AM -0700, Grant Likely wrote:
> > [...]
> >> The spi-cs-high property is defined in
> >> Documentation/powerpc/dts-bindings/spi-bus.txt, but it definitely was
> >> a mistake
> >
> > Yup.
> >
> >> Currently the spi-cs-high property is parsed in the
> >> of_register_spi_devices() function, but the CS polarity needs to be
> >> known before registering devices. It needs to be factored out into
> >> another utility function callable by spi bus drivers so that it can
> >> get polarity data at probe time.
> >
> > Untill we have this, Torsten's patch is a real improvement, and
> > works for non-broken hw/fw.
> >
> > So I think it should be applied.
>
> I disagree since it only band-aids the problem and uglifies the driver
> in the process. In the immediate term the driver needs to be changed
> to read the spi-cs-high property out of the child nodes before
> registering the devices.
Hm. I thought we agreed that spi-cs-high is not good? Why do you
encourage using it then? We'll have to uglify the driver with
legacy device-tree handling code.
--
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
^ permalink raw reply
* Re: spi_mpc8xxx.c: chip select polarity problem
From: Grant Likely @ 2009-11-26 18:50 UTC (permalink / raw)
To: avorontsov; +Cc: Torsten Fleischer, spi-devel-general, linuxppc-dev
In-Reply-To: <20091126184157.GA16607@oksana.dev.rtsoft.ru>
On Thu, Nov 26, 2009 at 11:41 AM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Thu, Nov 26, 2009 at 11:16:34AM -0700, Grant Likely wrote:
> [...]
>> The spi-cs-high property is defined in
>> Documentation/powerpc/dts-bindings/spi-bus.txt, but it definitely was
>> a mistake
>
> Yup.
>
>> Currently the spi-cs-high property is parsed in the
>> of_register_spi_devices() function, but the CS polarity needs to be
>> known before registering devices. =A0It needs to be factored out into
>> another utility function callable by spi bus drivers so that it can
>> get polarity data at probe time.
>
> Untill we have this, Torsten's patch is a real improvement, and
> works for non-broken hw/fw.
>
> So I think it should be applied.
I disagree since it only band-aids the problem and uglifies the driver
in the process. In the immediate term the driver needs to be changed
to read the spi-cs-high property out of the child nodes before
registering the devices. I'm not going to apply this patch.
g.
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: spi_mpc8xxx.c: chip select polarity problem
From: Anton Vorontsov @ 2009-11-26 18:41 UTC (permalink / raw)
To: Grant Likely; +Cc: Torsten Fleischer, spi-devel-general, linuxppc-dev
In-Reply-To: <fa686aa40911261016p489981a2g1fac393264c4d1c7@mail.gmail.com>
On Thu, Nov 26, 2009 at 11:16:34AM -0700, Grant Likely wrote:
[...]
> The spi-cs-high property is defined in
> Documentation/powerpc/dts-bindings/spi-bus.txt, but it definitely was
> a mistake
Yup.
> Currently the spi-cs-high property is parsed in the
> of_register_spi_devices() function, but the CS polarity needs to be
> known before registering devices. It needs to be factored out into
> another utility function callable by spi bus drivers so that it can
> get polarity data at probe time.
Untill we have this, Torsten's patch is a real improvement, and
works for non-broken hw/fw.
So I think it should be applied.
Thanks,
--
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
^ permalink raw reply
* Re: spi_mpc8xxx.c: chip select polarity problem
From: Grant Likely @ 2009-11-26 18:18 UTC (permalink / raw)
To: Torsten Fleischer; +Cc: spi-devel-general, linuxppc-dev
In-Reply-To: <200911261827.38405.to-fleischer@t-online.de>
On Thu, Nov 26, 2009 at 10:27 AM, Torsten Fleischer
<to-fleischer@t-online.de> wrote:
> On Thu, Nov 26, 2009 at 13:12:04 Anton Vorontsov wrote:
> [...]
>> > Ah. =A0I understand what you're doing now. =A0 Hmmm. =A0This approach
>> > concerns me because it relies on firmware or platform code to get CS
>> > gpios set up properly before the driver is probed.
>>
>> Yes, that was said at the very beginning of this thread.
>>
>> > =A0Firmware doesn't
>> > always get it right, and I prefer to avoid platform specific setups as
>> > much as possible. =A0Why can't the CS polarity be encoded into the
>> > device tree so the driver *does* have the polarity data at probe time?
>>
>> We have the spi-cs-high property, but it duplicates compatible
>> property. 'compatible' is enough to tell whether some device has
>> cs-high or cs-low (device's driver knows that already).
>>
>> The problem is that SPI bus master doesn't know all the devices,
>> so it can't extract that information from the compatible string.
>> To workaround that we can use 'spi-cs-high', but that's ugly
>> workaround.
>>
>> SPI modes (0,1,2,3) is another matter, some devices can work in
>> several modes, so 'spi-cpol' and 'spi-cpha' are actually useful.
>>
> To get a sane initial state the needed GPIOs can be set to be inputs duri=
ng
> the driver's initialization.
> This requires pull-up/pull-down resistors connected to the chip select
> lines. I think we can assume that they exist, because the GPIOs are all i=
nputs
> after the controller's hardware reset and the resistors are needed to hav=
e a
> well-defined voltage level on the chip select lines. Normally the level i=
s set
> so that the devices are disabled.
>
> Therefore, it doesn't matter if the firmware sets the GPIOs wrong.
No, that's just shifting responsibility from firmware to hardware.
There is just as much broken hardware out there as broken firmware.
The assumption cannot be made that the initial state of the pin is the
inactive state of the CS line. Plus, some GPIO pins are output only
and the inital state cannot be read.
g.
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: spi_mpc8xxx.c: chip select polarity problem
From: Grant Likely @ 2009-11-26 18:16 UTC (permalink / raw)
To: avorontsov; +Cc: Torsten Fleischer, spi-devel-general, linuxppc-dev
In-Reply-To: <20091126121204.GA3558@oksana.dev.rtsoft.ru>
On Thu, Nov 26, 2009 at 5:12 AM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Wed, Nov 25, 2009 at 03:11:57PM -0700, Grant Likely wrote:
>> On Wed, Nov 25, 2009 at 1:41 PM, Torsten Fleischer
>> <to-fleischer@t-online.de> wrote:
>> > On Wen, Nov 25, 2009 at 01:33:57 Grant Likely wrote:
>> >> Thanks. =A0However, there needs to be a proper description of what th=
is
>> >> patch does to go in the commit header. =A0Can you please write one?
>> >>
>> >> Thanks,
>> >> g.
>> >>
>> > [...]
>> >
>> > The initialization of the chip selects is removed from the probe() fun=
ction of
>> > the spi_mpc8xxx driver, because the driver doesn't know the polarity o=
f the
>> > chip selects of the SPI devices at the time of its initialization.
>> >
>> > For this reason the initialization of the several chip selects is post=
poned
>> > to the point of time when the very first SPI transfer to the associate=
d device
>> > occurs.
>> >
>> >
>> > Signed-off-by: Torsten Fleischer <to-fleischer@t-online.de>
>>
>> Ah. =A0I understand what you're doing now. =A0 Hmmm. =A0This approach
>> concerns me because it relies on firmware or platform code to get CS
>> gpios set up properly before the driver is probed.
>
> Yes, that was said at the very beginning of this thread.
I also came in part way through as I wasn't an SPI maintainer when
this thread started. :-)
>> =A0Firmware doesn't
>> always get it right, and I prefer to avoid platform specific setups as
>> much as possible. =A0Why can't the CS polarity be encoded into the
>> device tree so the driver *does* have the polarity data at probe time?
>
> We have the spi-cs-high property, but it duplicates compatible
> property. 'compatible' is enough to tell whether some device has
> cs-high or cs-low (device's driver knows that already).
But the device's driver isn't controlling the CS line, the SPI bus
driver is. Besides, there is no guarantee that all drivers will
actualy be loaded before something starts using SPI. The bus driver
*must* know what the active state of each CS line is before activating
any devices.
The spi bus binding is deficient in this case. A property (can be
optional) need to be added to the spi bus node to explicitly state the
CS polarities. It's not entirely sane to look for a "spi-cs-high'
property in the spi device nodes because the SPI bus may not be fully
populated (ie. if a device happens to be sitting on the bus, but isn't
in the device tree yet). Before any SPI transactions go out, it is
the responsibility of the bus driver to ensure all CS lines are in the
correct state.
> The problem is that SPI bus master doesn't know all the devices,
> so it can't extract that information from the compatible string.
> To workaround that we can use 'spi-cs-high', but that's ugly
> workaround.
It doesn't need to know about the devices, but is must know how all of
its CS lines behave. So it isn't an really an ugly workaround, but I
do think the binding is insufficient for the SPI bus driver (see
below)
> SPI modes (0,1,2,3) is another matter, some devices can work in
> several modes, so 'spi-cpol' and 'spi-cpha' are actually useful.
yes. spi-cpol and spi-cpha are actually properties of the device, and
belong in the spi device node, not the spi bus node.
The spi-cs-high property is defined in
Documentation/powerpc/dts-bindings/spi-bus.txt, but it definitely was
a mistake for the reasons I described above. It does work in some
cases, but a property at the bus node would be better. The driver can
still fallback to looking for spi-cs-high properties in child nodes.
Currently the spi-cs-high property is parsed in the
of_register_spi_devices() function, but the CS polarity needs to be
known before registering devices. It needs to be factored out into
another utility function callable by spi bus drivers so that it can
get polarity data at probe time.
Cheers,
g.
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: spi_mpc8xxx.c: chip select polarity problem
From: Torsten Fleischer @ 2009-11-26 17:27 UTC (permalink / raw)
To: avorontsov; +Cc: spi-devel-general, linuxppc-dev
In-Reply-To: <20091126121204.GA3558@oksana.dev.rtsoft.ru>
On Thu, Nov 26, 2009 at 13:12:04 Anton Vorontsov wrote:
[...]
> > Ah. I understand what you're doing now. Hmmm. This approach
> > concerns me because it relies on firmware or platform code to get CS
> > gpios set up properly before the driver is probed.
>
> Yes, that was said at the very beginning of this thread.
>
> > Firmware doesn't
> > always get it right, and I prefer to avoid platform specific setups as
> > much as possible. Why can't the CS polarity be encoded into the
> > device tree so the driver *does* have the polarity data at probe time?
>
> We have the spi-cs-high property, but it duplicates compatible
> property. 'compatible' is enough to tell whether some device has
> cs-high or cs-low (device's driver knows that already).
>
> The problem is that SPI bus master doesn't know all the devices,
> so it can't extract that information from the compatible string.
> To workaround that we can use 'spi-cs-high', but that's ugly
> workaround.
>
> SPI modes (0,1,2,3) is another matter, some devices can work in
> several modes, so 'spi-cpol' and 'spi-cpha' are actually useful.
>
To get a sane initial state the needed GPIOs can be set to be inputs during
the driver's initialization.
This requires pull-up/pull-down resistors connected to the chip select
lines. I think we can assume that they exist, because the GPIOs are all inputs
after the controller's hardware reset and the resistors are needed to have a
well-defined voltage level on the chip select lines. Normally the level is set
so that the devices are disabled.
Therefore, it doesn't matter if the firmware sets the GPIOs wrong.
^ permalink raw reply
* Re: [RFC PATCH 16/19] powerpc: wii: hollywood interrupt controller support
From: Albert Herranz @ 2009-11-26 15:42 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev
In-Reply-To: <1259213022.16367.273.camel@pasglop>
Benjamin Herrenschmidt wrote:
> On Sun, 2009-11-22 at 23:01 +0100, Albert Herranz wrote:
>
>> +static void hlwd_pic_mask_and_ack(unsigned int virq)
>> +{
>> + int irq = virq_to_hw(virq);
>> + void __iomem *io_base = get_irq_chip_data(virq);
>> +
>> + clear_bit(irq, io_base + HW_BROADWAY_IMR);
>> + set_bit(irq, io_base + HW_BROADWAY_ICR);
>> +}
>
> Same comment as with flipper. BTW. It looks really similar, can't you
> use the same driver ?
>
I'll look at it. Surely, parts of it would be mergeable.
> Cheers,
> Ben.
>
Thanks,
Albert
>
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox