* Re: [PATCH] powerpc: Create "rom" (MTD) device prpmc2800
From: Segher Boessenkool @ 2007-06-07 15:32 UTC (permalink / raw)
To: David Woodhouse; +Cc: ppcdev, linux-mtd, Milton Miller
In-Reply-To: <1181227676.2785.47.camel@pmac.infradead.org>
>> That's the way the cookie crumbles in Linux MTD for now. It's
>> *always* detecting this by probing -- you only can say what [not] to
>> probe.
>
> You could hook it up to the chip drivers directly if you want, and
> bypass the probe code. I'm not sure it's worth it.
In most cases it is probably not worth it. It is good
to hear that the option to bypass the probe if needed
is available though.
Segher
^ permalink raw reply
* Re: [PATCH] powerpc: Create "rom" (MTD) device prpmc2800
From: Segher Boessenkool @ 2007-06-07 15:49 UTC (permalink / raw)
To: David Woodhouse; +Cc: ppcdev, linux-mtd, Milton Miller
In-Reply-To: <1181228038.2785.54.camel@pmac.infradead.org>
>> Put the proper interface informations in the device-tree, maybe some
>> OS
>> smarter than linux will make good use on it and maybe linux will be
>> fixed at one point too (not by you, of course, you gave us that line
>> often enough about not being paid to do the right thing).
>
> In general, Linux is doing the right thing by probing.
Compare to PCI. After Linux has found out how to
drive a PHB from the device tree, it can detect the
whole PCI tree on its own. The device tree can still
be used to find out special things about the devices
(interrupt routing, ...) or to change some of the
probing algorithm (in the case of hardware bugs, for
example).
Now with NOR flash, the situation is analogue. The
device tree tells Linux how to drive the flash bus
controller, and certain things about that bus (width,
for example). It also should tell Linux what kind
of devices are on that bus (CFI, ...). Linux can
do its own probes then, or it can use the device tree
for special cases.
> There are two major command sets for NOR flash -- the Intel/Sharp
> command set, and the AMD/Fujitsu command set.
>
> There are also two major ways to detect flash chips -- the JEDEC probe
> with magic numbers for mfr/chip ident,
A heuristic, but it's been worked out well enough over
the years that it is pretty reliable now.
> and the CFI ('Common Flash
> Interface') probe which gets you tables of information about the chip,
> including what optional command set extensions it has, etc.
But it only is 100% reliable _if_ you already know the
chip is CFI compatible.
> You _can_ bypass the probe and pass straight through to the 'back-end'
> chip driver. But unless you provide the CFI information you won't get
> to
> use any of the optional chip features (or blacklist some of the
> known-broken features). It doesn't make a lot of sense to try using the
> back-end chip drivers directly. Just go through the normal probe
> process, really.
Just don't do any heuristic probes ("just try if something's
there", etc.) -- using the CFI probe algorithm on a flash
device that the device tree tells you is a CFI device should
be just fine, certainly.
> If you really want to bypass the probe, then I suspect you want the
> full
> CFI tables to be present in your OF properties.
I never saw how that kind of info is useful for the OS to
have in the device tree -- if the OS can use the device at
all, it can get that info straight from the device easy
enough.
Segher
^ permalink raw reply
* Re: [PATCH] powerpc: Create "rom" (MTD) device prpmc2800
From: Segher Boessenkool @ 2007-06-07 15:55 UTC (permalink / raw)
To: David Woodhouse; +Cc: linuxppc-dev, linux-mtd, Milton Miller
In-Reply-To: <1181228442.2785.60.camel@pmac.infradead.org>
> Any representation of flash devices in the device-tree should ideally
> have 'bus width' and 'interleave' properties to contain this
> information.
>
> The 'bus width' cannot necessarily be inferred, especially where a
> given
> bus can be configured to allow multiple sizes of access. It's purely a
> function of how the flash chips are wired up. That's why we actually
> call it 'bank width', not 'bus width' in the Linux code.
Ah, "bank width" as in "bus width" per chip select. I see.
So I think the best thing to have would be
compatible "cfi-flash"
bank-width like you said
device-width width of a single flash device
reg complete address range of this thing
and then the Linux OF CFI flash code / MTD code just keeps
on probing devices from that address range until it has it
filled.
Sounds good / comments / anything I missed?
Segher
^ permalink raw reply
* Re: [PATCH] Fix the LPC47M192 SuperIO on the MPC8641 HPCN
From: Wade Farnsworth @ 2007-06-07 16:04 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linuxppc-dev, paulus
In-Reply-To: <200706070039.51541.arnd@arndb.de>
On Thu, 2007-06-07 at 00:39 +0200, Arnd Bergmann wrote:
> On Wednesday 06 June 2007, Wade Farnsworth wrote:
> > + /* LPC47M192 Super I/O configuration */
> > + outb(0x55, 0x4e); /* enter superio config mode */
> > +
> > + /* Enable keyboard and mouse */
> > + outb(0x07, 0x4e); /* device selector register */
> > + outb(0x07, 0x4f); /* select keyboard registers (device 7) */
> > + outb(0x30, 0x4e); /* keyboard activation register */
> > + outb(0x01, 0x4f); /* activate keyboard */
>
> Hardcoded I/O port numbers always worry me a little. I know that this is
> supposed to work in general, but can't you read the I/O port range from
> a device tree property?
I suppose I could create a device node for the Super I/O config
registers and use those instead of hardcoding it here.
Something to the effect of:
superio_cfg@4e {
reg = <1 4e 2>;
compatible = "smsc-lpc47m192-cfg";
};
I'm not sure if the name and compatible properties are appropriate
though. Any recommendations?
--Wade
^ permalink raw reply
* Re: [PATCH] powerpc: Create "rom" (MTD) device prpmc2800
From: David Woodhouse @ 2007-06-07 16:05 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev, linux-mtd, Milton Miller
In-Reply-To: <18fbfbaf113ab76b602d279848d9b405@kernel.crashing.org>
On Thu, 2007-06-07 at 17:55 +0200, Segher Boessenkool wrote:
> > Any representation of flash devices in the device-tree should ideally
> > have 'bus width' and 'interleave' properties to contain this
> > information.
> >
> > The 'bus width' cannot necessarily be inferred, especially where a
> > given
> > bus can be configured to allow multiple sizes of access. It's purely a
> > function of how the flash chips are wired up. That's why we actually
> > call it 'bank width', not 'bus width' in the Linux code.
>
> Ah, "bank width" as in "bus width" per chip select. I see.
>
> So I think the best thing to have would be
>
> compatible "cfi-flash"
> bank-width like you said
> device-width width of a single flash device
> reg complete address range of this thing
>
> and then the Linux OF CFI flash code / MTD code just keeps
> on probing devices from that address range until it has it
> filled.
>
> Sounds good / comments / anything I missed?
That seems reasonable. I would personally have used 'interleave' to give
the number of devices interleaved together into the specified
bank-width, rather than specifying device-width. People might get
confused by device-width because many devices can actually be _either_
x16 or x8. But I don't care much. Linux will only be using the
bank-width anyway.
The other thing that might be seen in the 'compatible' property would be
'jedec-flash', and in that case perhaps we also want properties for the
manufacturer and device ident? Linux wouldn't necessarily care about
them, but other operating systems might.
And we also need partition information to get through somehow, in the
cases where the firmware knows it.
--
dwmw2
^ permalink raw reply
* Re: [PATCH] Fix drivers/rtc/Kconfig for powerpc
From: Jon Loeliger @ 2007-06-07 16:08 UTC (permalink / raw)
To: Wade Farnsworth; +Cc: a.zummo, rtc-linux, linuxppc-dev
In-Reply-To: <1181150240.5674.139.camel@rhino>
On Wed, 2007-06-06 at 12:17, Wade Farnsworth wrote:
> >
> > Did you miss where I ACK'ed it 16-May-2007? :-)
>
> Ah okay, maybe I don't understand what that means. When you say
> "Acked-by" does that mean you'll push it upstream?
I meant that I picked the patch up, applied it to my tree,
tested it, and verified that _I_ thought it was good.
It received my Vote of Confidence, for whatever that is worth.
> What I meant was I haven't seen anybody pick up this patch yet, and
> would like to know it if will be, or if there is something else I need
> to do.
Right. This patch is in "generic" PowerPC related code,
and not in my area to "push upstream". I would expect, say,
Paul to pick it up directly. Perhaps Kumar would pick it up
and stage it for Paul. Perhaps akpm would pick it up. :-)
> --Wade
HTH,
jdl
^ permalink raw reply
* Re: [PATCH] kexec ppc64: fix misaligned cmdline
From: Geoff Levand @ 2007-06-07 16:19 UTC (permalink / raw)
To: Michael Neuling; +Cc: dwmw2, kexec, Milton Miller, linuxppc-dev, horms
In-Reply-To: <10188.1181179162@neuling.org>
Michael Neuling wrote:
> If the cmdline changes between boots, we can get misalignment of the
> bootargs entry, which in turn corrupts our device tree blob and hence
> kills our kexec boot. Also, if there was no /chosen/bootargs
> previously, we'd never add a new one.
>
> This ignores the bootargs while parsing the tree and inserts it later.
>
> Signed-off-by: Michael Neuling <mikey@neuling.org>
Hi.
Acked-by: Geoff Levand <geoffrey.levand@am.sony.com>
I tested this with PS3 and it works as expected. I added it to my
ps3-kexec-tools.git. With this patch mainline kexec will work
with the PS3.
I still have a work-in-progress patch started by David Woodhouse
to support building a 32 bit kexec executable for PPC64 platforms.
I don't have any definite plan to work on it though.
-Geoff
^ permalink raw reply
* Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.
From: Andy Fleming @ 2007-06-07 16:21 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev@ozlabs.org
In-Reply-To: <1181113764.31677.242.camel@localhost.localdomain>
On Jun 6, 2007, at 02:09, Benjamin Herrenschmidt wrote:
> On Mon, 2007-06-04 at 15:31 -0700, Randy Vinson wrote:
>> Actually, that's not strictly true. If a card containing a P2P bridge
>> is
>> installed into PCI slot 4, the i8259 will end up on a bus that is not
>> numbered bus 1. I've often wondered how something like that is
>> handled
>> in the DTS.
>
> The bus number shouldn't matter as long as low IO cycles are properly
> forwarded to it.
Right. Unless there's an interrupt mapping, which has to include the
bus number in the filter. When that happens, parsing the interrupt
map for the i8259's mapping fails because its address doesn't match.
Andy
^ permalink raw reply
* Re: [PATCH] When checking I8042 io port, use of_find_compatible_node() instead of of_find_node_by_type()
From: Wade Farnsworth @ 2007-06-07 16:26 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev, paulus
In-Reply-To: <2a56a9d008269d393c68959f1804c21d@kernel.crashing.org>
On Thu, 2007-06-07 at 15:05 +0200, Segher Boessenkool wrote:
> > In check_legacy_ioport(), instead of using of_find_node_by_type() to
> > find the 8042 node, use of_find_compatible_node() to find either the
> > keyboard or mouse node.
>
> Why?
>
> > switch(base_port) {
> > case I8042_DATA_REG:
> > - np = of_find_node_by_type(NULL, "8042");
> > + np = of_find_compatible_node(NULL, NULL, "pnpPNP,303");
> > + if (!np)
> > + np = of_find_compatible_node(NULL, NULL, "pnpPNP,f03");
> > + if (np) {
> > + parent = of_get_parent(np);
> > + of_node_put(np);
> > + np = parent;
> > + }
>
> This breaks other boards using 8042, if those exist --
> if this code is board-specific, it is in the wrong file.
Perhaps I was a little too bold here.
I guess if this breaks other boards then I should leave the check for
the device type, or perhaps just drop this patch altogether.
In the latter case, the 8042 node would need to have device_type =
"8042". This contradicts what you posted in an earlier conversation we
had regarding the 8641 device tree:
On Thu, 2007-05-17 at 01:40 +0200, Segher Boessenkool wrote:
> >>>> + 8042@60 {
> >>>> + device_type = "8042";
> >>
> >> Drop the device_type. A number as a name isn't
> >> all that great, either.
> >
> > Currently in order for the i8042 devices to be initialized,
> > check_legacy_ioport() must find a node with device_type "8042".
>
> So fix that :-)
This is my attempt to fix it :)
Now if you prefer that the 8042 node on the 8641 has the device_type
"8042", I will gladly add it back and drop this patch.
--Wade
^ permalink raw reply
* Re: [PATCH] Fix drivers/rtc/Kconfig for powerpc
From: Wade Farnsworth @ 2007-06-07 16:33 UTC (permalink / raw)
To: Jon Loeliger; +Cc: a.zummo, rtc-linux, linuxppc-dev
In-Reply-To: <1181232488.31686.19.camel@ld0161-tx32>
On Thu, 2007-06-07 at 11:08 -0500, Jon Loeliger wrote:
> On Wed, 2007-06-06 at 12:17, Wade Farnsworth wrote:
>
> > >
> > > Did you miss where I ACK'ed it 16-May-2007? :-)
> >
> > Ah okay, maybe I don't understand what that means. When you say
> > "Acked-by" does that mean you'll push it upstream?
>
> I meant that I picked the patch up, applied it to my tree,
> tested it, and verified that _I_ thought it was good.
> It received my Vote of Confidence, for whatever that is worth.
Ok. Thanks for acking it by the way.
>
> > What I meant was I haven't seen anybody pick up this patch yet, and
> > would like to know it if will be, or if there is something else I need
> > to do.
>
> Right. This patch is in "generic" PowerPC related code,
> and not in my area to "push upstream". I would expect, say,
> Paul to pick it up directly. Perhaps Kumar would pick it up
> and stage it for Paul. Perhaps akpm would pick it up. :-)
>
But since this is in drivers/rtc/Kconfig, wouldn't the maintainer of
that subsystem be expected to pick this up?
Thanks for the clarifications.
--Wade
^ permalink raw reply
* Re: [PATCH] Fix the LPC47M192 SuperIO on the MPC8641 HPCN
From: Segher Boessenkool @ 2007-06-07 16:35 UTC (permalink / raw)
To: Wade Farnsworth; +Cc: linuxppc-dev, paulus, Arnd Bergmann
In-Reply-To: <1181232276.5674.146.camel@rhino>
>> Hardcoded I/O port numbers always worry me a little. I know that this
>> is
>> supposed to work in general, but can't you read the I/O port range
>> from
>> a device tree property?
>
> I suppose I could create a device node for the Super I/O config
> registers and use those instead of hardcoding it here.
I'd just hide it all, do this setup in the firmware,
where it belongs, and don't expose the superio config
in the device tree.
> superio_cfg@4e {
> reg = <1 4e 2>;
> compatible = "smsc-lpc47m192-cfg";
> };
>
> I'm not sure if the name and compatible properties are appropriate
> though. Any recommendations?
"superio" and "smsc,lpc47m192" I'd say. You also
then should link the logical devices on the superio
to the device nodes that represent those. I'm not
sure this is all worth it, this is low-level setup
the firmware should do and everything else can treat
it as a black box.
Segher
^ permalink raw reply
* Re: [PATCH] Fix drivers/rtc/Kconfig for powerpc
From: Jon Loeliger @ 2007-06-07 16:37 UTC (permalink / raw)
To: Wade Farnsworth; +Cc: a.zummo, rtc-linux, linuxppc-dev
In-Reply-To: <1181233980.5674.174.camel@rhino>
On Thu, 2007-06-07 at 11:33, Wade Farnsworth wrote:
>
> But since this is in drivers/rtc/Kconfig, wouldn't the maintainer of
> that subsystem be expected to pick this up?
Oh, sure. In which case he/she may be waiting on
someone over in PowerPC land to ACK it first. :-)
Dunno.
jdl
^ permalink raw reply
* Re: [PATCH] Fix the LPC47M192 SuperIO on the MPC8641 HPCN
From: Wade Farnsworth @ 2007-06-07 16:42 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev, paulus
In-Reply-To: <af3aa7bd5ff385f5ad94aa7c1d456a35@kernel.crashing.org>
On Thu, 2007-06-07 at 15:01 +0200, Segher Boessenkool wrote:
> > + /* enable superio @ 0x4e and keyboard/mouse address decoding */
> > + pci_write_config_byte(dev, 0x63, 0x90);
>
> I doubt that comment is accurate.
m1575 legacy interface device register 0x63 is the legacy I/O decoding
control.
Writing 0x90 enables ports 0x4e and 0x4f for Super I/O configuration and
ports 0x60 and 0x64 for keyboard/mouse.
>
> > + outb(0x07, 0x4e); /* device selector register */
> > + outb(0x07, 0x4f); /* select keyboard registers (device 7) */
>
> Please abstract out the 4e,4f access sequence, so
> you can just say write_sio(0x30, 1); or similar.
Sure.
>
> > + /* Enable superio runtime registers for gpio in pci i/o space */
> > + outb(0x20, 0x4e); /* device id register */
>
> This write is superfluous as far as I can see.
Yes, it appears so. I'll remove it.
--Wade
^ permalink raw reply
* Re: [PATCH] powerpc: Create "rom" (MTD) device prpmc2800
From: Segher Boessenkool @ 2007-06-07 16:46 UTC (permalink / raw)
To: David Woodhouse; +Cc: linuxppc-dev, linux-mtd, Milton Miller
In-Reply-To: <1181232358.2785.79.camel@pmac.infradead.org>
>> So I think the best thing to have would be
>>
>> compatible "cfi-flash"
>> bank-width like you said
>> device-width width of a single flash device
>> reg complete address range of this thing
>>
>> and then the Linux OF CFI flash code / MTD code just keeps
>> on probing devices from that address range until it has it
>> filled.
>>
>> Sounds good / comments / anything I missed?
>
> That seems reasonable. I would personally have used 'interleave' to
> give
> the number of devices interleaved together into the specified
> bank-width, rather than specifying device-width.
I think "device-width" is much clearer -- from the name
"interleave" you cannot tell that it actually means
"number-of-interleaved-devices-in-this-bank".
> People might get
> confused by device-width because many devices can actually be _either_
> x16 or x8.
Well after it has been soldered down to the board, it's
pretty much fixed what the device's bus width is ;-)
> But I don't care much. Linux will only be using the
> bank-width anyway.
So it can reliably detect interleaving itself?
I'd like "device-width" to be a required property still,
for the benefit of non-Linux device tree users, or just
in case.
> The other thing that might be seen in the 'compatible' property would
> be
> 'jedec-flash',
I don't want to deal with that yet, one thing at a time,
and CFI flash it is now. JEDEC is a bit hairier.
> and in that case perhaps we also want properties for the
> manufacturer and device ident?
Certainly, since they cannot be reliably probed.
> Linux wouldn't necessarily care about
> them, but other operating systems might.
>
> And we also need partition information to get through somehow, in the
> cases where the firmware knows it.
A child node per partition I suppose, dunno.
Segher
^ permalink raw reply
* Re: [PATCH] Fix the LPC47M192 SuperIO on the MPC8641 HPCN
From: Wade Farnsworth @ 2007-06-07 16:51 UTC (permalink / raw)
To: Segher Boessenkool, Jon Loeliger; +Cc: linuxppc-dev, paulus, Arnd Bergmann
In-Reply-To: <63ff9dee6b212cc7908c8e1b73920ea6@kernel.crashing.org>
On Thu, 2007-06-07 at 18:35 +0200, Segher Boessenkool wrote:
> >> Hardcoded I/O port numbers always worry me a little. I know that this
> >> is
> >> supposed to work in general, but can't you read the I/O port range
> >> from
> >> a device tree property?
> >
> > I suppose I could create a device node for the Super I/O config
> > registers and use those instead of hardcoding it here.
>
> I'd just hide it all, do this setup in the firmware,
> where it belongs, and don't expose the superio config
> in the device tree.
That's a valid point. This probably could (should?) be handled by
U-boot.
Jon, or others do you have any opinions on this?
>
> > superio_cfg@4e {
> > reg = <1 4e 2>;
> > compatible = "smsc-lpc47m192-cfg";
> > };
> >
> > I'm not sure if the name and compatible properties are appropriate
> > though. Any recommendations?
>
> "superio" and "smsc,lpc47m192" I'd say. You also
> then should link the logical devices on the superio
> to the device nodes that represent those. I'm not
> sure this is all worth it, this is low-level setup
> the firmware should do and everything else can treat
> it as a black box.
OK, if we decide to keep this, I'll use those instead.
Thanks.
--Wade
^ permalink raw reply
* Re: [PATCH 2/8] Add uli1575 pci-bridge sector to MPC8641HPCN dts file.
From: Segher Boessenkool @ 2007-06-07 16:53 UTC (permalink / raw)
To: Andy Fleming; +Cc: linuxppc-dev@ozlabs.org
In-Reply-To: <FE5191F7-8A1D-428A-B4F0-32AD3A08B08A@freescale.com>
>> The bus number shouldn't matter as long as low IO cycles are properly
>> forwarded to it.
>
> Right. Unless there's an interrupt mapping, which has to include the
> bus number in the filter.
No it doesn't; the PCI device # of the thing that gets
its interrupt mapped is enough. PCI bus # is not a
hardware property (except for bus #0) so it isn't a
structural thing in the device tree at all.
> When that happens, parsing the interrupt
> map for the i8259's mapping fails because its address doesn't match.
That's because you're trying to map interrupts from
two separate interrupt domains in your interrupt-map:
the PCI bus of that node, and some child PCI bus
somewhere deeper down. This isn't designed to work,
and hey, it doesn't ;-)
Segher
^ permalink raw reply
* Re: [PATCH] When checking I8042 io port, use of_find_compatible_node() instead of of_find_node_by_type()
From: Segher Boessenkool @ 2007-06-07 16:59 UTC (permalink / raw)
To: Wade Farnsworth; +Cc: linuxppc-dev, paulus
In-Reply-To: <1181233610.5674.167.camel@rhino>
>>> In check_legacy_ioport(), instead of using of_find_node_by_type() to
>>> find the 8042 node, use of_find_compatible_node() to find either the
>>> keyboard or mouse node.
>>
>> Why?
^^^^^^^^
Why do you need/want this at all?
>>> switch(base_port) {
>>> case I8042_DATA_REG:
>>> - np = of_find_node_by_type(NULL, "8042");
>>> + np = of_find_compatible_node(NULL, NULL, "pnpPNP,303");
>>> + if (!np)
>>> + np = of_find_compatible_node(NULL, NULL, "pnpPNP,f03");
>>> + if (np) {
>>> + parent = of_get_parent(np);
>>> + of_node_put(np);
>>> + np = parent;
>>> + }
>>
>> This breaks other boards using 8042, if those exist --
>> if this code is board-specific, it is in the wrong file.
>
> Perhaps I was a little too bold here.
>
> I guess if this breaks other boards
I don't know, but neither do you ;-)
> then I should leave the check for
> the device type, or perhaps just drop this patch altogether.
Maybe you want to test for _either_ of the three devices?
> In the latter case, the 8042 node would need to have device_type =
> "8042". This contradicts what you posted in an earlier conversation we
> had regarding the 8641 device tree:
>
> On Thu, 2007-05-17 at 01:40 +0200, Segher Boessenkool wrote:
>>>>>> + 8042@60 {
>>>>>> + device_type = "8042";
>>>>
>>>> Drop the device_type. A number as a name isn't
>>>> all that great, either.
>>>
>>> Currently in order for the i8042 devices to be initialized,
>>> check_legacy_ioport() must find a node with device_type "8042".
>>
>> So fix that :-)
>
> This is my attempt to fix it :)
A bit too harsh though... Deprecate first, remove
later.
> Now if you prefer that the 8042 node on the 8641 has the device_type
> "8042", I will gladly add it back and drop this patch.
Never ever device_type. "compatible" perhaps. And
not a bare number, either.
I'm not sure what the Linux code here does exactly --
it seems to me you're allowing a legacy I/O port mapping
when legacy drivers probe for it, right? Instead you
should change those drivers to not probe (perhaps by
refusing the I/O range access here), and be explicitly
instantiated from your device tree parsing code.
Segher
^ permalink raw reply
* [PATCH] powerpc: Don't use long for 32-bit temp variables in spin lock ops
From: Olof Johansson @ 2007-06-07 17:08 UTC (permalink / raw)
To: paulus; +Cc: linuxppc-dev
The spinlock ops have long as their return type (as well as for some
of the temporary types internally). All locks are 32-bit, so it makes
no sense to do 64-bit ops on them.
For example, this is how my compiler built _spin_lock() for me:
c0000000004b2050: li r0,0
c0000000004b2054: stb r0,460(r13)
c0000000004b2058: lwz r0,8(r13)
c0000000004b205c: lwarx r9,0,r3
c0000000004b2060: cmpwi r9,0
c0000000004b2064: bne- c0000000004b2078 <._spin_lock+0x28>
c0000000004b2068: stwcx. r0,0,r3
c0000000004b206c: nop
c0000000004b2070: bne+ c0000000004b205c <._spin_lock+0xc>
c0000000004b2074: isync
c0000000004b2078: cmpdi cr7,r9,0
c0000000004b207c: .long 0x4dfe0020
c0000000004b2080: mr r1,r1
c0000000004b2084: lwz r0,0(r3)
c0000000004b2088: cmpdi cr7,r0,0
c0000000004b208c: bne+ cr7,c0000000004b2080 <._spin_lock+0x30>
c0000000004b2090: mr r2,r2
c0000000004b2094: b c0000000004b2058 <._spin_lock+0x8>
Note the cmpdi at ..78 when r9 was loaded with lwarx.
Unfortunately I haven't found a way to get rid of the duplicate
comparison alltogether.
Signed-off-by: Olof Johansson <olof@lixom.net>
Index: 2.6.21/include/asm-powerpc/spinlock.h
===================================================================
--- 2.6.21.orig/include/asm-powerpc/spinlock.h
+++ 2.6.21/include/asm-powerpc/spinlock.h
@@ -53,9 +53,9 @@
* This returns the old value in the lock, so we succeeded
* in getting the lock if the return value is 0.
*/
-static __inline__ unsigned long __spin_trylock(raw_spinlock_t *lock)
+static __inline__ unsigned int __spin_trylock(raw_spinlock_t *lock)
{
- unsigned long tmp, token;
+ unsigned int tmp, token;
token = LOCK_TOKEN;
__asm__ __volatile__(
@@ -179,9 +179,9 @@ extern void __raw_spin_unlock_wait(raw_s
* This returns the old value in the lock + 1,
* so we got a read lock if the return value is > 0.
*/
-static long __inline__ __read_trylock(raw_rwlock_t *rw)
+static int __inline__ __read_trylock(raw_rwlock_t *rw)
{
- long tmp;
+ int tmp;
__asm__ __volatile__(
"1: lwarx %0,0,%1\n"
@@ -203,9 +203,9 @@ static long __inline__ __read_trylock(ra
* This returns the old value in the lock,
* so we got the write lock if the return value is 0.
*/
-static __inline__ long __write_trylock(raw_rwlock_t *rw)
+static __inline__ int __write_trylock(raw_rwlock_t *rw)
{
- long tmp, token;
+ int tmp, token;
token = WRLOCK_TOKEN;
__asm__ __volatile__(
@@ -263,7 +263,7 @@ static int __inline__ __raw_write_tryloc
static void __inline__ __raw_read_unlock(raw_rwlock_t *rw)
{
- long tmp;
+ int tmp;
__asm__ __volatile__(
"# read_unlock\n\t"
^ permalink raw reply
* Re: [PATCH] Fix the LPC47M192 SuperIO on the MPC8641 HPCN
From: Segher Boessenkool @ 2007-06-07 17:04 UTC (permalink / raw)
To: Wade Farnsworth; +Cc: linuxppc-dev, paulus
In-Reply-To: <1181234565.5674.182.camel@rhino>
>>> + /* enable superio @ 0x4e and keyboard/mouse address decoding */
>>> + pci_write_config_byte(dev, 0x63, 0x90);
>>
>> I doubt that comment is accurate.
>
> m1575 legacy interface device register 0x63 is the legacy I/O decoding
> control.
>
> Writing 0x90 enables ports 0x4e and 0x4f for Super I/O configuration
> and
> ports 0x60 and 0x64 for keyboard/mouse.
Ah I see. Copy this explanation into your next
version of the patch? :-)
Segher
^ permalink raw reply
* Re: [PATCH] Fix the LPC47M192 SuperIO on the MPC8641 HPCN
From: Segher Boessenkool @ 2007-06-07 17:05 UTC (permalink / raw)
To: Wade Farnsworth; +Cc: linuxppc-dev, paulus, Arnd Bergmann
In-Reply-To: <1181235084.5674.190.camel@rhino>
>> I'd just hide it all, do this setup in the firmware,
>> where it belongs, and don't expose the superio config
>> in the device tree.
>
> That's a valid point. This probably could (should?) be handled by
> U-boot.
Yeah. One more argument for this is that _every_
possible OS would need this code executed before
it runs.
Segher
^ permalink raw reply
* Re: [RFC] 85XX: Allow 8259 cascade to share an MPIC interrupt line.
From: Sergei Shtylyov @ 2007-06-07 17:14 UTC (permalink / raw)
To: Randy Vinson; +Cc: linuxppc-dev@ozlabs.org
In-Reply-To: <466755AC.40201@mvista.com>
Hello.
Randy Vinson wrote:
> The Freescale MPC8555CDS and MPC8548CDS reference hardware has a legacy
> 8259 interrupt controller pair contained within a VIA VT82C686B Southbridge
> on the main carrier board. The processor complex plugs into the carrier
> card using a PCI slot which limits the available interrupts to the
> INTA-INTD PCI interrupts. The output of the 8259 cascade pair is routed
> through a gate array and connected to the PCI INTA interrupt line.
> The normal interrupt chaining hook (set_irq_chained_handler) does
> not allow sharing of the chained interrupt which prevents the
> use of PCI INTA by PCI devices. This patch allows the 8259 cascade
> pair to share their interrupt line with PCI devices.
Hmm, I see you've come up with an interesting solution. :-)
> Signed-off-by: Randy Vinson <rvinson@mvista.com>
> ---
> Note that there may very well be a better way of accomplishing this. If someone
> has a better alternative, I'm open to it. This was just the simplest way I could
> get this to work.
>
> Also, the addition of the .end routine for the MPIC is not strictly necessary for
> this patch. It's there so this code will run from within the threaded interrupt
> context used by the Real Time
Hmmm, why would you need that? Where RT is different from the vanilla? :-O
> arch/powerpc/platforms/85xx/mpc85xx_cds.c | 32 +++++++++++++++++++++++++---
> arch/powerpc/sysdev/mpic.c | 1 +
> 2 files changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/platforms/85xx/mpc85xx_cds.c b/arch/powerpc/platforms/85xx/mpc85xx_cds.c
> index 1490eb3..431aaa2 100644
> --- a/arch/powerpc/platforms/85xx/mpc85xx_cds.c
> +++ b/arch/powerpc/platforms/85xx/mpc85xx_cds.c
> @@ -127,16 +127,30 @@ static void __init mpc85xx_cds_pcibios_fixup(void)
> }
>
> #ifdef CONFIG_PPC_I8259
> -#warning The i8259 PIC support is currently broken
> -static void mpc85xx_8259_cascade(unsigned int irq, struct irq_desc *desc)
> +static void mpc85xx_8259_cascade_handler(unsigned int irq,
> + struct irq_desc *desc)
> {
> unsigned int cascade_irq = i8259_irq();
>
> if (cascade_irq != NO_IRQ)
> + /* handle an interrupt from the 8259 */
> generic_handle_irq(cascade_irq);
>
> - desc->chip->eoi(irq);
> + /* check for any interrupts from the shared IRQ line */
> + handle_fasteoi_irq(irq, desc);
> }
> +
> +static irqreturn_t mpc85xx_8259_cascade_action(int irq, void *dev_id)
> +{
> + return IRQ_HANDLED;
> +}
Well, mpc85xx_8259_intr() would probably be more in line with the code
elsewhere... and you could keep the mpc85xx_8259_cascade() name then.
> +static struct irqaction mpc85xxcds_8259_irqaction = {
> + .handler = mpc85xx_8259_cascade_action,
> + .flags = IRQF_SHARED,
> + .mask = CPU_MASK_NONE,
> + .name = "8259 cascade",
> +};
> #endif /* PPC_I8259 */
> #endif /* CONFIG_PCI */
>
> @@ -216,7 +230,17 @@ static void __init mpc85xx_cds_pic_init(void)
> i8259_init(cascade_node, 0);
> of_node_put(cascade_node);
>
> - set_irq_chained_handler(cascade_irq, mpc85xx_8259_cascade);
> + /*
> + * Hook the interrupt to make sure desc->action is never NULL.
> + * This is required to ensure that the interrupt does not get
> + * disabled when the last user of the shared IRQ line frees their
> + * interrupt.
> + */
> + if (setup_irq(cascade_irq, &mpc85xxcds_8259_irqaction))
> + printk(KERN_ERR "Failed to setup cascade interrupt\n");
> + else
> + /* Success. Connect our low-level cascade handler. */
> + set_irq_handler(cascade_irq, mpc85xx_8259_cascade_handler);
> #endif /* CONFIG_PPC_I8259 */
> }
>
> diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
> index 75aad38..14e3d1d 100644
> --- a/arch/powerpc/sysdev/mpic.c
> +++ b/arch/powerpc/sysdev/mpic.c
> @@ -836,6 +836,7 @@ static struct irq_chip mpic_irq_chip = {
> .mask = mpic_mask_irq,
> .unmask = mpic_unmask_irq,
> .eoi = mpic_end_irq,
> + .end = mpic_unmask_irq,
> .set_type = mpic_set_irq_type,
> };
WBR, Sergei
^ permalink raw reply
* Re: write u-boot.bin on mpc8349e-mITX-gp with bdi2000
From: Muruga Ganapathy @ 2007-06-07 16:49 UTC (permalink / raw)
To: Soohyung Cho, linuxppc-embedded
You may want to talk to Abarton to see whether they have the BDI2000
configuration file with 8349 support and use it for flashing the
u-boot.
> Hello.
>
> Now I'm trying to write u-boot.bin on mpc8349e-mITX-gp with bdi2000.
> But I don't have correct bdi2000 configuration file for mpc8349e-mITX-
gp.
>
> How can I write u-boot.bin on that board?
> Unfortunately I erased u-boot area on that board so there no u-boot
anymore
> on that board.
> Is there anyone can show bdi2000 configuration file for mpc8349e-mITX-
gp?
> Or please let me know how to write u-boot on that board with bdi2000.
>
> I'll appreciate any comments.
>
>
>
> _______________________________________________
> Linuxppc-embedded mailing list
> Linuxppc-embedded@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-embedded
>
>
*************************************************************
GDA Technologies, Inc.
1010 Rincon Circle
San Jose CA, 95131
Phone (408) 432-3090
Fax (408) 432-3091
Accelerate Your Innovation
**************************************************************
=====
This message contains information from GDA Technologies Inc and
affiliates, and is intended for the sole use of the individual and
entity to whom it is addressed. It may contain information, including
any attachments, that is privileged, confidential and exempt from
disclosure under applicable law. If you are not the intended addressee,
nor authorized to receive for the intended addressee, you are hereby
notified that you may not use, copy, disclose or distribute to anyone
the message or any information contained in the message. If you have
received this electronic transmission in error, please notify the
sender immediately by a "reply to sender only" message and destroy all
electronic and hard copies of the communication, including attachments.
====
^ permalink raw reply
* Re: [PATCH] When checking I8042 io port, use of_find_compatible_node() instead of of_find_node_by_type()
From: Wade Farnsworth @ 2007-06-07 17:52 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev, paulus
In-Reply-To: <cd1e655423140dbb20af2821d5c3955f@kernel.crashing.org>
On Thu, 2007-06-07 at 18:59 +0200, Segher Boessenkool wrote:
> >>> In check_legacy_ioport(), instead of using of_find_node_by_type() to
> >>> find the 8042 node, use of_find_compatible_node() to find either the
> >>> keyboard or mouse node.
> >>
> >> Why?
>
> ^^^^^^^^
>
> Why do you need/want this at all?
check_legacy_ioport() takes an io port as a parameter, and then matches
it with a device node based on the device type. My patch would instead
match it with the compatible property of one of the 8042 devices, since
AIUI, the device type shouldn't be used.
>
> >>> switch(base_port) {
> >>> case I8042_DATA_REG:
> >>> - np = of_find_node_by_type(NULL, "8042");
> >>> + np = of_find_compatible_node(NULL, NULL, "pnpPNP,303");
> >>> + if (!np)
> >>> + np = of_find_compatible_node(NULL, NULL, "pnpPNP,f03");
> >>> + if (np) {
> >>> + parent = of_get_parent(np);
> >>> + of_node_put(np);
> >>> + np = parent;
> >>> + }
> >>
> >> This breaks other boards using 8042, if those exist --
> >> if this code is board-specific, it is in the wrong file.
> >
> > Perhaps I was a little too bold here.
> >
> > I guess if this breaks other boards
>
> I don't know, but neither do you ;-)
True.
>
> > then I should leave the check for
> > the device type, or perhaps just drop this patch altogether.
>
> Maybe you want to test for _either_ of the three devices?
Ok, I'll add the test for the 8042 device type back in to preserve
compatibility with any existing boards.
>
> > In the latter case, the 8042 node would need to have device_type =
> > "8042". This contradicts what you posted in an earlier conversation we
> > had regarding the 8641 device tree:
> >
> > On Thu, 2007-05-17 at 01:40 +0200, Segher Boessenkool wrote:
> >>>>>> + 8042@60 {
> >>>>>> + device_type = "8042";
> >>>>
> >>>> Drop the device_type. A number as a name isn't
> >>>> all that great, either.
> >>>
> >>> Currently in order for the i8042 devices to be initialized,
> >>> check_legacy_ioport() must find a node with device_type "8042".
> >>
> >> So fix that :-)
> >
> > This is my attempt to fix it :)
>
> A bit too harsh though... Deprecate first, remove
> later.
Fair enough.
>
> > Now if you prefer that the 8042 node on the 8641 has the device_type
> > "8042", I will gladly add it back and drop this patch.
>
> Never ever device_type. "compatible" perhaps. And
> not a bare number, either.
That was my understanding, hence the patch.
>
> I'm not sure what the Linux code here does exactly --
> it seems to me you're allowing a legacy I/O port mapping
> when legacy drivers probe for it, right? Instead you
> should change those drivers to not probe (perhaps by
> refusing the I/O range access here), and be explicitly
> instantiated from your device tree parsing code.
>
The drivers could probably be modified in such a way, but that is not my
goal with these patches. My goal is to enable the 8042 devices on the
8641. Any driver modification should be done separately.
--Wade
^ permalink raw reply
* Re: [RFC] 85XX: Allow 8259 cascade to share an MPIC interrupt line.
From: Randy Vinson @ 2007-06-07 18:04 UTC (permalink / raw)
To: Jon Loeliger; +Cc: linuxppc-dev@ozlabs.org
In-Reply-To: <1181230255.31686.3.camel@ld0161-tx32>
Jon Loeliger wrote:
> On Wed, 2007-06-06 at 19:47, Randy Vinson wrote:
[snippage]
>> Signed-off-by: Randy Vinson <rvinson@mvista.com>
>> ---
>> Note that there may very well be a better way of accomplishing this. If someone
>> has a better alternative, I'm open to it. This was just the simplest way I could
>> get this to work.
>>
>> Also, the addition of the .end routine for the MPIC is not strictly necessary for
>> this patch. It's there so this code will run from within the threaded interrupt
>> context used by the Real Time patch.
>
>
> Hmm. I feel that at least the last paragraph here needs
> to be above the --- line for future patch readers. Otherwise
> it's relationship to the rest of the patch details will be
> intermixed and lost.
OK. That's a good idea. I'll update that for the next round.
Thx,
Randy V.
^ permalink raw reply
* Re: [spi-devel-general] [PATCH] Simple driver for Xilinx SPI controler.
From: Andrei Konovalov @ 2007-06-07 18:39 UTC (permalink / raw)
To: David Brownell; +Cc: spi-devel-general, linuxppc-embedded
In-Reply-To: <200706062209.09731.david-b@pacbell.net>
Hi David,
Thanks for reviewing this stuff!
David Brownell wrote:
> On Wednesday 06 June 2007, Andrei Konovalov wrote:
>> Would be nice to get this driver into mainline.
>> Reviews and comments are welcome.
>
> I'll ignore the Kconfig flamage ... ;)
>
>
>> --- /dev/null
>> +++ b/drivers/spi/xilinx_spi.c
>> @@ -0,0 +1,447 @@
>> +/*
>> + * xilinx_spi.c
>> + *
>> + * Xilinx SPI controler driver (master mode only)
>> + *
>> + * Author: MontaVista Software, Inc.
>> + * source@mvista.com
>> + *
>> + * 2002-2007 (c) MontaVista Software, Inc. This file is licensed under the
>> + * terms of the GNU General Public License version 2. This program is licensed
>> + * "as is" without any warranty of any kind, whether express or implied.
>> + */
>> +
>> +
>> +/* Simple macros to get the code more readable */
>> +#define xspi_in16(addr) in_be16((u16 __iomem *)(addr))
>> +#define xspi_in32(addr) in_be32((u32 __iomem *)(addr))
>> +#define xspi_out16(addr, value) out_be16((u16 __iomem *)(addr), (value))
>> +#define xspi_out32(addr, value) out_be32((u32 __iomem *)(addr), (value))
>
> I'm rather used to seeing I/O addressses passed around as "void __iomem *"
> so those sorts of cast are not needed... :)
I've decided to make the base_address (u8 __iomem *) to make it easier to
add the register offsets to the base. Like that:
static void xspi_init_hw(u8 __iomem *regs_base)
{
/* Reset the SPI device */
xspi_out32(regs_base + XIPIF_V123B_RESETR_OFFSET,
XIPIF_V123B_RESET_MASK);
...
Without the cast, out_be32(regs_base + XIPIF_V123B_RESETR_OFFSET, XIPIF_V123B_RESET_MASK)
produces a warning.
Another solution could be
#define XSPI_REG(base, offset) (void *)((base) + (offset))
and
/* Reset the SPI device */
out_be32(XSPI_REG(regs_base, XIPIF_V123B_RESETR_OFFSET),
XIPIF_V123B_RESET_MASK);
Is it better?
>> +
>> +static void xspi_abort_transfer(u8 __iomem *regs_base)
>> +{
>
> You should not need an abort primitive. This is called only
> in the remove-controller path. By the time it's called,
> every child spi_device on this bus segment should have been
> removed ... which means any spi_driver attached to that
> device has already returned from its remove() method, which
> in turn means that there will be no spi_message objects in
> flight from any of those drivers.
I am paranoid enough not to rely 100% on the spi_device's and the spi_bitbang
doing all that :)
Agreed, xspi_abort_transfer is bad name here. But I still would like to
leave these two writes:
+ /* Deselect the slave on the SPI bus */
+ xspi_out32(regs_base + XSPI_SSR_OFFSET, 0xffff);
+ /* Disable the transmitter */
+ xspi_out16(regs_base + XSPI_CR_OFFSET,
+ XSPI_CR_TRANS_INHIBIT | XSPI_CR_MANUAL_SSELECT);
in xilinx_spi_remove(). Just in case :)
>> +static void xilinx_spi_chipselect(struct spi_device *spi, int is_on)
>> +{
>> + struct xilinx_spi *xspi;
>> + u8 __iomem *regs_base;
>> +
>> + xspi = spi_master_get_devdata(spi->master);
>> + regs_base = xspi->regs;
>> +
>> + if (is_on == BITBANG_CS_INACTIVE) {
>> + /* Deselect the slave on the SPI bus */
>> + xspi_out32(regs_base + XSPI_SSR_OFFSET, 0xffff);
>
> I take it you can't support SPI_CS_HIGH??
There is no clear indication in the SPI controller docs that SPI_CS_HIGH
would work. I suspect it would as we don't use the ability of the controller
to generate the chip selects automatically (the auto mode doesn't support SPI_CS_HIGH).
But it is more safe to advertise SPI_CS_HIGH is not supported.
>> +/* spi_bitbang requires custom setup_transfer() to be defined if there is a
>> + * custom txrx_bufs(). We have nothing to setup here as the SPI IP block
>> + * supports just 8 bits per word, and SPI clock can't be changed in software.
>> + * Check for 8 bits per word; speed_hz checking could be added if the SPI
>> + * clock information is available. Chip select delay calculations could be
>> + * added here as soon as bitbang_work() can be made aware of the delay value.
>> + */
>> +static int xilinx_spi_setup_transfer(struct spi_device *spi,
>> + struct spi_transfer *t)
>> +{
>> + u8 bits_per_word;
>> +
>> + bits_per_word = (t) ? t->bits_per_word : spi->bits_per_word;
>> + if (bits_per_word != 8)
>> + return -EINVAL;
>
> Speed checking *SHOULD* be added; the clock info can be platform data.
>
> It would make trouble if a driver needed to turn the clock down to,
> say, 400 KHz for some operation, and the controller said "yes" but
> kept it at 25 MHz or whatever. It's OK if the driver is told that
> 400 KHz can't happen though -- it's possible to recover from that.
OK
> (Although in practice it's best to have the transfer method do
> the error checking, so that messages that will fail do so before
> they are allowed to enter the I/O queue.)
>
> ISTR you may need to delegate to the default method here too, but
> it's been a while since I poked at that level and the issue might
> not apply to this particular driver config.
>
>
>
>> +
>> + return 0;
>> +}
>> +
>> +
>> +static int xilinx_spi_setup(struct spi_device *spi)
>> +{
>> + struct spi_bitbang *bitbang;
>> + struct xilinx_spi *xspi;
>> + int retval;
>> +
>> + xspi = spi_master_get_devdata(spi->master);
>> + bitbang = &xspi->bitbang;
>
> You need to verify ALL the input parameters. In particular,
> mask spi->mode against all the values this driver recognizes
> and supports. If you don't support SPI_LSB_FIRST it's a bug
> if setup() succeeds after setting that. Same thing with all
> other bits defined today (SPI_3WIRE, SPI_CS_HIGH) and in the
> future...
>
> For an example, see the atmel_spi.c driver and MODEBITS.
> There's a patch in MM that makes all the SPI controller
> drivers have analagous tests ... and for bitbang there's
> a patch adding spi_bitbang.mode flags to declare any
> extra spi->mode flags that should be accepted.
OK. Thanks.
>> + ...
>> +
>> +static irqreturn_t xilinx_spi_irq(int irq, void *dev_id)
>> +{
>> + struct xilinx_spi *xspi;
>> + u8 __iomem *regs_base;
>> + u32 ipif_isr;
>> +
>> + xspi = (struct xilinx_spi *) dev_id;
>> + regs_base = xspi->regs;
>> +
>> + /* Get the IPIF inetrrupts, and clear them immediately */
>
> Spell checkers will tell you this is "interrupts" ... ;)
Oops...
>> + ipif_isr = xspi_in32(regs_base + XIPIF_V123B_IISR_OFFSET);
>> + xspi_out32(regs_base + XIPIF_V123B_IISR_OFFSET, ipif_isr);
>> +
>> + if (ipif_isr & XSPI_INTR_TX_EMPTY) { /* Transmission completed */
>> + u16 cr;
>> + u8 sr;
>> +
>> + /* A transmit has just completed. Process received data and
>> + * check for more data to transmit. Always inhibit the
>> + * transmitter while the Isr refills the transmit register/FIFO,
>> + * or make sure it is stopped if we're done.
>> + */
>> + cr = xspi_in16(regs_base + XSPI_CR_OFFSET);
>> + xspi_out16(regs_base + XSPI_CR_OFFSET,
>> + cr | XSPI_CR_TRANS_INHIBIT);
>> +
>> + /* Read out all the data from the Rx FIFO */
>> + sr = in_8(regs_base + XSPI_SR_OFFSET);
>> + while ((sr & XSPI_SR_RX_EMPTY_MASK) == 0) {
>> + u8 data;
>> +
>> + data = in_8(regs_base + XSPI_RXD_OFFSET);
>> + if (xspi->rx_ptr) {
>> + *xspi->rx_ptr++ = data;
>> + }
>> + sr = in_8(regs_base + XSPI_SR_OFFSET);
>> + }
>> +
>> + /* See if there is more data to send */
>> + if (xspi->remaining_bytes > 0) {
>> + /* sr content is valid here; no need for io_8() */
>> + while ((sr & XSPI_SR_TX_FULL_MASK) == 0
>> + && xspi->remaining_bytes > 0) {
>> + if (xspi->tx_ptr) {
>> + out_8(regs_base + XSPI_TXD_OFFSET,
>> + *xspi->tx_ptr++);
>> + } else {
>> + out_8(regs_base + XSPI_TXD_OFFSET, 0);
>> + }
>
> This duplicates the loop in txrx_bufs(); that's bad style.
> Have one routine holding the shared code.
OK
>> +static int __init xilinx_spi_probe(struct platform_device *dev)
>> +{
>> + int ret = 0;
>> + struct spi_master *master;
>> + struct xilinx_spi *xspi;
>> + struct xspi_platform_data *pdata;
>> + struct resource *r;
>> +
>> + /* Get resources(memory, IRQ) associated with the device */
>> + master = spi_alloc_master(&dev->dev, sizeof(struct xilinx_spi));
>> +
>> + if (master == NULL) {
>> + return -ENOMEM;
>> + }
>> +
>> + platform_set_drvdata(dev, master);
>> + pdata = dev->dev.platform_data;
>> +
>> + if (pdata == NULL) {
>> + ret = -ENODEV;
>> + goto put_master;
>> + }
>> +
>> + r = platform_get_resource(dev, IORESOURCE_MEM, 0);
>> + if (r == NULL) {
>> + ret = -ENODEV;
>> + goto put_master;
>> + }
>> +
>> + xspi = spi_master_get_devdata(master);
>> + xspi->bitbang.master = spi_master_get(master);
>> + xspi->bitbang.chipselect = xilinx_spi_chipselect;
>> + xspi->bitbang.setup_transfer = xilinx_spi_setup_transfer;
>> + xspi->bitbang.txrx_bufs = xilinx_spi_txrx_bufs;
>> + xspi->bitbang.master->setup = xilinx_spi_setup;
>> + init_completion(&xspi->done);
>> +
>> + xspi->regs = ioremap(r->start, r->end - r->start + 1);
>
> Strictly speaking a request_region() should precede the ioremap,
> but a lot of folk don't bother. However, lacking that I'd put
> the request_irq() earlier, since that will be the only resource
> providing any guard against another driver sharing the hardware.
Will add request_region(). I was confused by platform_device_register()
doing something with the resources (contrary to the "plain" device_register()).
Thanks,
Andrei
^ 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