* Re: [RFC PATCH] Kconfig: Enable Kconfig fragments to be used for defconfig
From: Arnd Bergmann @ 2010-07-16 20:01 UTC (permalink / raw)
To: Grant Likely
Cc: Stephen Rothwell, Daniel Walker, linux-kbuild, Tony Lindgren,
Catalin Marinas, Nicolas Pitre, linux-kernel, Linus Torvalds,
Russell King, Uwe Kleine-König, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <AANLkTinyFBTKYaryFANYheTuP-biJUws2x01NRNZPb4M@mail.gmail.com>
On Friday 16 July 2010 19:57:55 Grant Likely wrote:
> On Fri, Jul 16, 2010 at 10:03 AM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > On Wed, 2010-07-14 at 00:04 +0100, Grant Likely wrote:
>
> sfr and I were talking about your patch the other day. Just warning
> on incomplete dependencies is enough to make it actually workable for
> me (without my ugly post-processing step). I was very happy to hear
> that it is in linux-next.
>
> Last missing piece is being able to do "select FOO = n", which Stephen
> is currently working on.
Are there a lot of symbols for which this is needed? If there is only
a handful, you could work around this by selectively adding
config FOO
bool "foo"
default !FOO_DISABLE
config FOO_DISABLE
def_bool "n"
Arnd
^ permalink raw reply
* Re: [RFC PATCH] Kconfig: Enable Kconfig fragments to be used for defconfig
From: Catalin Marinas @ 2010-07-16 20:09 UTC (permalink / raw)
To: Linus Torvalds
Cc: Stephen Rothwell, Daniel Walker, Russell King - ARM Linux,
linux-kbuild, Tony Lindgren, Nicolas Pitre, lkml,
Uwe Kleine-König, linuxppc-dev, linux-arm-kernel
In-Reply-To: <AANLkTikqHnyFVJF0Pzwy2xtKeNSXYAPFdPkAkLiXK8Ls@mail.gmail.com>
On Fri, 2010-07-16 at 19:46 +0100, Linus Torvalds wrote:
> On Fri, Jul 16, 2010 at 11:40 AM, Nicolas Pitre <nico@fluxnic.net> wrote:
> >
> > DOH.
>
> Well, it's possible that the correct approach is a mixture.
>
> Automatically do the trivial cases (recursive selects, dependencies
> that are simple or of the form "x && y" etc), and warn about the cases
> that aren't trivial (where "not trivial" may not necessarily be about
> fundamentally ambiguous ones, but just "complex enough that I won't
> even try").
There is still a risk with this approach when the Kconfig isn't entirely
correct. For example, on ARM we have (I pushed a patch already):
config CPU_32v6K
depends on CPU_V6
config CPU_V7
select CPU_32v6K
In this simple approach, we end up selecting CPU_V6 when we only need
CPU_V7. There other places like this in the kernel.
Of course, kbuild could still warn but if people rely on this feature to
select options automatically I suspect they would ignore the warnings.
--
Catalin
^ permalink raw reply
* Re: [RFC PATCH] Kconfig: Enable Kconfig fragments to be used for defconfig
From: Arnd Bergmann @ 2010-07-16 20:11 UTC (permalink / raw)
To: Linus Torvalds
Cc: Stephen Rothwell, Daniel Walker, Russell King - ARM Linux,
Nicolas Pitre, Tony Lindgren, Catalin Marinas, linux-kbuild, lkml,
Uwe Kleine-König, linuxppc-dev, linux-arm-kernel
In-Reply-To: <AANLkTikqHnyFVJF0Pzwy2xtKeNSXYAPFdPkAkLiXK8Ls@mail.gmail.com>
On Friday 16 July 2010 20:46:17 Linus Torvalds wrote:
> Maybe a full "solver" is unnecessary, for example, but just a simple
> "automatically enable the direct dependencies and scream when it's not
> simple any more" would take care of 99% of the common cases, and then
> warn when it needs some manual help.
I think the recursion should also be limited to cases where the
dependency is a valid selectable option, i.e. not for
# this architecture does not support MMIO
config HAS_IOMEM
def_bool 'n'
config PCI
bool "PCI Device drivers"
depends on HAS_IOMEM
config FOO
tristate "Some device driver"
depends on PCI
In this case, it would be straightforward for the solver to enable PCI
for when something selects CONFIG_FOO, but it should print a warning
if this is attempted while HAS_IOMEM is unconditionally disabled,
since that puts it into the "not simple" category.
Arnd
^ permalink raw reply
* Re: [PATCH v2] edac: mpc85xx: Add support for new MPCxxx/Pxxxx EDAC controllers
From: Scott Wood @ 2010-07-16 20:12 UTC (permalink / raw)
To: Anton Vorontsov
Cc: Peter Tyser, linux-kernel, Dave Jiang, linuxppc-dev,
Doug Thompson, Andrew Morton
In-Reply-To: <20100715182507.GA3482@oksana.dev.rtsoft.ru>
On Thu, 15 Jul 2010 22:25:07 +0400
Anton Vorontsov <avorontsov@mvista.com> wrote:
> Simply add proper IDs into the device table.
>
> Signed-off-by: Anton Vorontsov <avorontsov@mvista.com>
> ---
>
> It appears that the driver has two device ID tables. :-)
> So, my previous attempt enabled only half of the functionality.
>
> Andrew,
>
> Can you please replace
>
> edac-mpc85xx-add-support-for-mpc8569-edac-controllers.patch
>
> with this patch? It also adds some more IDs for the newer chips.
>
> Thanks!
>
> drivers/edac/mpc85xx_edac.c | 8 ++++++++
> 1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/edac/mpc85xx_edac.c b/drivers/edac/mpc85xx_edac.c
> index 52ca09b..3820879 100644
> --- a/drivers/edac/mpc85xx_edac.c
> +++ b/drivers/edac/mpc85xx_edac.c
> @@ -646,8 +646,12 @@ static struct of_device_id mpc85xx_l2_err_of_match[] = {
> { .compatible = "fsl,mpc8555-l2-cache-controller", },
> { .compatible = "fsl,mpc8560-l2-cache-controller", },
> { .compatible = "fsl,mpc8568-l2-cache-controller", },
> + { .compatible = "fsl,mpc8569-l2-cache-controller", },
> { .compatible = "fsl,mpc8572-l2-cache-controller", },
> + { .compatible = "fsl,p1020-l2-cache-controller", },
> + { .compatible = "fsl,p1021-l2-cache-controller", },
> { .compatible = "fsl,p2020-l2-cache-controller", },
> + { .compatible = "fsl,p4080-l2-cache-controller", },
L2 on the p4080 is quite different from those other chips. It's part
of the core, controlled by SPRs.
-Scott
^ permalink raw reply
* Re: [RFC PATCH] Kconfig: Enable Kconfig fragments to be used for defconfig
From: Grant Likely @ 2010-07-16 20:17 UTC (permalink / raw)
To: Catalin Marinas
Cc: Stephen Rothwell, Daniel Walker, Russell King - ARM Linux,
linux-kbuild, Tony Lindgren, Nicolas Pitre, lkml, linuxppc-dev,
Uwe Kleine-König, Linus Torvalds, linux-arm-kernel
In-Reply-To: <1279310976.18579.8.camel@e102109-lin.cambridge.arm.com>
On Fri, Jul 16, 2010 at 2:09 PM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> On Fri, 2010-07-16 at 19:46 +0100, Linus Torvalds wrote:
>> On Fri, Jul 16, 2010 at 11:40 AM, Nicolas Pitre <nico@fluxnic.net> wrote=
:
>> >
>> > DOH.
>>
>> Well, it's possible that the correct approach is a mixture.
>>
>> Automatically do the trivial cases (recursive selects, dependencies
>> that are simple or of the form "x && y" etc), and warn about the cases
>> that aren't trivial (where "not trivial" may not necessarily be about
>> fundamentally ambiguous ones, but just "complex enough that I won't
>> even try").
>
> There is still a risk with this approach when the Kconfig isn't entirely
> correct. For example, on ARM we have (I pushed a patch already):
>
> config CPU_32v6K
> =A0 =A0 =A0 =A0depends on CPU_V6
>
> config CPU_V7
> =A0 =A0 =A0 =A0select CPU_32v6K
>
> In this simple approach, we end up selecting CPU_V6 when we only need
> CPU_V7. There other places like this in the kernel.
>
> Of course, kbuild could still warn but if people rely on this feature to
> select options automatically I suspect they would ignore the warnings.
In my first patch, I made Kconfig problems errors instead of warnings.
That would prevent people from ignoring them.
g.
^ permalink raw reply
* Re: [RFC PATCH] Kconfig: Enable Kconfig fragments to be used for defconfig
From: Nicolas Pitre @ 2010-07-16 20:29 UTC (permalink / raw)
To: Grant Likely
Cc: Stephen Rothwell, Daniel Walker, Russell King - ARM Linux,
linux-kbuild, Tony Lindgren, Catalin Marinas, linuxppc-dev, lkml,
Uwe Kleine-König, Linus Torvalds, linux-arm-kernel
In-Reply-To: <AANLkTimKpmZaGRYfYs8HGDz-_VG8ePXN6bZvl2-YcNdR@mail.gmail.com>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1361 bytes --]
On Fri, 16 Jul 2010, Grant Likely wrote:
> On Fri, Jul 16, 2010 at 2:09 PM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > On Fri, 2010-07-16 at 19:46 +0100, Linus Torvalds wrote:
> >> On Fri, Jul 16, 2010 at 11:40 AM, Nicolas Pitre <nico@fluxnic.net> wrote:
> >> >
> >> > DOH.
> >>
> >> Well, it's possible that the correct approach is a mixture.
> >>
> >> Automatically do the trivial cases (recursive selects, dependencies
> >> that are simple or of the form "x && y" etc), and warn about the cases
> >> that aren't trivial (where "not trivial" may not necessarily be about
> >> fundamentally ambiguous ones, but just "complex enough that I won't
> >> even try").
> >
> > There is still a risk with this approach when the Kconfig isn't entirely
> > correct. For example, on ARM we have (I pushed a patch already):
> >
> > config CPU_32v6K
> > depends on CPU_V6
> >
> > config CPU_V7
> > select CPU_32v6K
> >
> > In this simple approach, we end up selecting CPU_V6 when we only need
> > CPU_V7. There other places like this in the kernel.
> >
> > Of course, kbuild could still warn but if people rely on this feature to
> > select options automatically I suspect they would ignore the warnings.
>
> In my first patch, I made Kconfig problems errors instead of warnings.
> That would prevent people from ignoring them.
ACK.
Nicolas
^ permalink raw reply
* Re: [RFC PATCH] Kconfig: Enable Kconfig fragments to be used for defconfig
From: Grant Likely @ 2010-07-16 20:37 UTC (permalink / raw)
To: Nicolas Pitre
Cc: Stephen Rothwell, Daniel Walker, Russell King - ARM Linux,
linux-kbuild, Tony Lindgren, Catalin Marinas, linuxppc-dev, lkml,
Uwe Kleine-König, Linus Torvalds, linux-arm-kernel
In-Reply-To: <alpine.LFD.2.00.1007161629270.10598@xanadu.home>
On Fri, Jul 16, 2010 at 2:29 PM, Nicolas Pitre <nico@fluxnic.net> wrote:
> On Fri, 16 Jul 2010, Grant Likely wrote:
>
>> On Fri, Jul 16, 2010 at 2:09 PM, Catalin Marinas
>> <catalin.marinas@arm.com> wrote:
>> > On Fri, 2010-07-16 at 19:46 +0100, Linus Torvalds wrote:
>> >> On Fri, Jul 16, 2010 at 11:40 AM, Nicolas Pitre <nico@fluxnic.net> wr=
ote:
>> >> >
>> >> > DOH.
>> >>
>> >> Well, it's possible that the correct approach is a mixture.
>> >>
>> >> Automatically do the trivial cases (recursive selects, dependencies
>> >> that are simple or of the form "x && y" etc), and warn about the case=
s
>> >> that aren't trivial (where "not trivial" may not necessarily be about
>> >> fundamentally ambiguous ones, but just "complex enough that I won't
>> >> even try").
>> >
>> > There is still a risk with this approach when the Kconfig isn't entire=
ly
>> > correct. For example, on ARM we have (I pushed a patch already):
>> >
>> > config CPU_32v6K
>> > =A0 =A0 =A0 =A0depends on CPU_V6
>> >
>> > config CPU_V7
>> > =A0 =A0 =A0 =A0select CPU_32v6K
>> >
>> > In this simple approach, we end up selecting CPU_V6 when we only need
>> > CPU_V7. There other places like this in the kernel.
>> >
>> > Of course, kbuild could still warn but if people rely on this feature =
to
>> > select options automatically I suspect they would ignore the warnings.
>>
>> In my first patch, I made Kconfig problems errors instead of warnings.
>> =A0That would prevent people from ignoring them.
>
> ACK.
It would also flush out any current Kconfig dependency issues.
g.
^ permalink raw reply
* Re: [RFC PATCH] Kconfig: Enable Kconfig fragments to be used for defconfig
From: Catalin Marinas @ 2010-07-16 20:44 UTC (permalink / raw)
To: Grant Likely
Cc: Stephen Rothwell, Daniel Walker, Russell King - ARM Linux,
linux-kbuild, Tony Lindgren, Nicolas Pitre, lkml, linuxppc-dev,
Uwe Kleine-König, Linus Torvalds, linux-arm-kernel
In-Reply-To: <AANLkTimKpmZaGRYfYs8HGDz-_VG8ePXN6bZvl2-YcNdR@mail.gmail.com>
On Fri, 2010-07-16 at 21:17 +0100, Grant Likely wrote:
> On Fri, Jul 16, 2010 at 2:09 PM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > On Fri, 2010-07-16 at 19:46 +0100, Linus Torvalds wrote:
> >> On Fri, Jul 16, 2010 at 11:40 AM, Nicolas Pitre <nico@fluxnic.net> wrote:
> >> >
> >> > DOH.
> >>
> >> Well, it's possible that the correct approach is a mixture.
> >>
> >> Automatically do the trivial cases (recursive selects, dependencies
> >> that are simple or of the form "x && y" etc), and warn about the cases
> >> that aren't trivial (where "not trivial" may not necessarily be about
> >> fundamentally ambiguous ones, but just "complex enough that I won't
> >> even try").
> >
> > There is still a risk with this approach when the Kconfig isn't entirely
> > correct. For example, on ARM we have (I pushed a patch already):
> >
> > config CPU_32v6K
> > depends on CPU_V6
> >
> > config CPU_V7
> > select CPU_32v6K
> >
> > In this simple approach, we end up selecting CPU_V6 when we only need
> > CPU_V7. There other places like this in the kernel.
> >
> > Of course, kbuild could still warn but if people rely on this feature to
> > select options automatically I suspect they would ignore the warnings.
>
> In my first patch, I made Kconfig problems errors instead of warnings.
> That would prevent people from ignoring them.
My point was that if we allow kbuild to select dependencies
automatically (as per Nico's initial suggestion, followed up by Linus),
in the above situation CPU_V7 would trigger the selection of CPU_V6 and
I don't want this. If we rely on such automatic selection of the
"depends on" options, we can't make the warnings be errors.
--
Catalin
^ permalink raw reply
* Re: [PATCH 1/2] Remove REDWOOD_[456] config options and conditional code
From: David Miller @ 2010-07-16 20:45 UTC (permalink / raw)
To: jwboyer
Cc: randy.dunlap, linuxppc-dev, linux, paulus, john.linn, davidb,
ladis, solomon, vamos-dev, vapier, florian, Artem.Bityutskiy,
qy03fugy, nico, netdev, linux-kernel, miltonm, jkosina, joe,
linux-mtd, dwmw2
In-Reply-To: <20100716142055.GA11736@zod.rchland.ibm.com>
From: Josh Boyer <jwboyer@linux.vnet.ibm.com>
Date: Fri, 16 Jul 2010 10:20:55 -0400
> On Fri, Jul 16, 2010 at 02:29:02PM +0200, Christian Dietrich wrote:
>>The config options for REDWOOD_[456] were commented out in the powerpc
>>Kconfig. The ifdefs referencing this options therefore are dead and all
>>references to this can be removed (Also dependencies in other KConfig
>>files).
>>
>>Signed-off-by: Christian Dietrich <qy03fugy@stud.informatik.uni-erlangen.de>
>>Signed-off-by: Christoph Egger <siccegge@cs.fau.de>
>
> This seems fine with me.
>
> The only question is which tree it coms through. I'm happy to take it
> in via mine if the netdev and MTD people are fine with that. Otherwise,
> my ack is below.
>
> Acked-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>
Please take it:
Acked-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply
* Re: hi, i have two flashs, but my kernel can only find one , how can i write the dts?
From: Grant Likely @ 2010-07-16 21:46 UTC (permalink / raw)
To: hacklu; +Cc: linuxppc-dev
In-Reply-To: <201007161634429622975@gmail.com>
T24gRnJpLCBKdWwgMTYsIDIwMTAgYXQgMjozNCBBTSwgaGFja2x1IDxlbWJlZHdheS50ZXN0QGdt
YWlsLmNvbT4gd3JvdGU6Cj4gdGhpcyBpcyBteSBkdHMgZmlsZToKPiBmbGFzaEAwLDCgewo+IKCg
oKCgoKCgoKCgoKCgoKCgoKCgoKCgoCNhZGRyZXNzLWNlbGxzoD2gPDE+Owo+IKCgoKCgoKCgoKCg
oKCgoKCgoKCgoKCgoCNzaXplLWNlbGxzoD2gPDE+Owo+IKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCg
oGNvbXBhdGlibGWgPaAiY2ZpLWZsYXNoIjsKPiCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKBwcm9i
ZS10eXBloD2gIkNGSSI7Cj4goKCgoKCgoKCgoKCgoKCgoKCgIKCgoKCgcmVnoD2gPDCgMKAxMDAw
MDAwPjsKPiCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKBiYW5rLXdpZHRooD2gPDI+Owo+IKCgoKCg
oKCgoKCgoKCgoKCgoKCgoKCgoGRldmljZS13aWR0aKA9oDwxPjsKPiCgoKCgoKCgoKCgoKCgoKCg
oKCgoKCgoKBocmN3QDCgewo+IKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgbGFiZWyg
PaAiaHJjdyI7Cj4goKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKByZWegPaA8MKA0MDAw
MD47Cj4goKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgfTsKPiCgoKCgoKCgoKCgoKCgoKCgoKCgoKCg
oKBqZmZzQDQwMDAwoHsKPiCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoGxhYmVsoD2g
ImpmZnMiOwo+IKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgcmVnoD2gPDQwMDAwoDIw
MDAwMD47Cj4goKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgfTsKPiCgoKCgoKCgoKCgoKCgoKCgoKCg
oKCgoKBqZmZzMkAyNDAwMDCgewo+IKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgbGFi
ZWygPaAidWltYWdlIjsKPiCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoHJlZ6A9oDwy
NDAwMDCgZDgwMDAwPjsKPiCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKB9Owo+IKCgoKCgoKCgoKCg
oKB9Owo+IGZsYXNoQDEsMKB7Cj4goKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgI2FkZHJlc3MtY2Vs
bHOgPaA8MT47Cj4goKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgI3NpemUtY2VsbHOgPaA8MT47Cj4g
oKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgY29tcGF0aWJsZaA9oCJjZmktZmxhc2giOwo+IKCgoKCg
oKCgoKCgoKCgoKCgoKCgoKCgoHByb2JlLXR5cGWgPaAiQ0ZJIjsKPiCgoKCgoKCgoKCgoKCgoKCg
oKCgoKCgoKByZWegPaA8MTAwMDAwMKAwoDEwMDAwMDA+OwoKVGhpcyBsb29rcyB3cm9uZy4gIElm
IHlvdSdyZSBzZWNvbmQgZmxhc2ggaXMgb24gY2hpcCBzZWxlY3QgMSBhcyB0aGUKbm9kZSBuYW1l
IHN1Z2dlc3RzLCB0aGVuIHRoaXMgc2hvdWxkIGJlIChmaXJzdCBjZWxsIGlzIENTIywgc2Vjb25k
IGlzCm9mZnNldCwgYW5kIHRoaXJkIGlzIHNpemUuICBBbG9zIHlvdSdyZSBtaXNzaW5nIHRoZSAw
eCBwcmVmaXgpOgoKcmVnID0gPDEgMCAweDEwMDAwMDA+OwoKSWYgeW91ciBzZWNvbmQgZmxhc2gg
aXMgb24gY2hpcCBzZWxlY3QgMCB3aXRoIHRoZSBmaXJzdCBmbGFzaCwgYnV0Cm9mZnNldCBieSAw
eDEwMDAwMDAsIHRoZW4gcmVnIHNob3VsZCBiZToKCnJlZyA9IDwwIDB4MTAwMDAwMCAweDEwMDAw
MDA+OwoKYW5kIHRoZSBuYW1lIHNob3VsZCBiZToKCmZsYXNoQDAsMTAwMDAwMCB7IC4uLiB9OwoK
Zy4KCi0tIApHcmFudCBMaWtlbHksIEIuU2MuLCBQLkVuZy4KU2VjcmV0IExhYiBUZWNobm9sb2dp
ZXMgTHRkLgo=
^ permalink raw reply
* Re: [RFC PATCH] Kconfig: Enable Kconfig fragments to be used for defconfig
From: Jamie Lokier @ 2010-07-16 23:49 UTC (permalink / raw)
To: Daniel Walker
Cc: linuxppc-dev, linux-kbuild, Tony Lindgren, Nicolas Pitre,
linux-kernel, linux-arm-kernel, Uwe Kleine-König,
Linus Torvalds, Russell King
In-Reply-To: <1279124563.21162.14.camel@c-dwalke-linux.qualcomm.com>
Daniel Walker wrote:
> > But all the rest is arbitrary and could be part of common shared
> > profiles or the like in defconfig format.
>
> I'm sure most people will want to have a config isolated to their
> specific device. That to me seems reasonable because everyone wants the
> smallest possible kernel they can get for their given device.
Indeed, but people who want the smallest possible kernel for their
specific device _in a particular use context_ tend to want:
- To disable support for parts of the device they aren't using.
For example, an SoC with integrated ethernet that isn't actually
wired up on their board, or where they're using an external ethernet
chip instead for some reason.
- To choose what's modular and what isn't, even for integrated
parts. For example to control the bootup sequence, they might
want to delay integrated USB and IDE initialisation, which is done by
making those modular and loading them after bringing up a splash
screen earlier in the boot scripts.
So there is still a need to be able to override the drivers and
settings, but it's still incredibly useful to have defaults which
describe the SoC or board accurately.
-- Jamie
^ permalink raw reply
* Re: cpm_uart_console_write() stuck in waiting for transmitter fifo ready
From: Shawn Jin @ 2010-07-17 0:30 UTC (permalink / raw)
To: Scott Wood; +Cc: ppcdev
In-Reply-To: <20100716131333.0c99a627@schlenkerla.am.freescale.net>
>> > My RCCR=3D0x1, meaning the first 512B is for microcode. So the data an=
d
>> > the TxBD should really be starting at 0xfa202200? Then my muram data's
>> > reg should be <0x200 ?>? What size shall I specify?
>>
>> After the muram data's reg is changed to <0x200 0x1a00>, the cpm_uart
>> driver works properly and the kernel messages are printed on the
>> serial port.
>
> The muram node is supposed to show the portions of DPRAM that are
> usable by the OS. =A0If some portion has been taken up by microcode (or
> anything else not under the OS's control) before the OS has started,
> then it must be excluded from the muram node.
It would be nicer that the initialization code could query the RCCR
value and adjust the base address. It took me quite a while to
understand the design. Without your help it could take much much
longer. So thanks a lot for your help. My project hasn't been done
yet, so I may bother you again. :)
Thanks again,
-Shawn.
^ permalink raw reply
* Re: Badness with the kernel version 2.6.35-rc1-git1 running on P6 box
From: Maciej Rutecki @ 2010-07-17 5:52 UTC (permalink / raw)
To: divya; +Cc: sachinp, linuxppc-dev, LKML
In-Reply-To: <4C401D56.3070108@linux.vnet.ibm.com>
On pi=C4=85tek, 16 lipca 2010 o 10:50:30 divya wrote:
> Hi ,
>=20
> With the latest kernel version 2.6.35-rc5-git1(2f7989efd4398) running on
> power(p6) box came across the following call trace
>=20
I created a Bugzilla entry at=20
https://bugzilla.kernel.org/show_bug.cgi?id=3D16406
for your bug report, please add your address to the CC list in there, thank=
s!
=2D-=20
Maciej Rutecki
http://www.maciek.unixy.pl
^ permalink raw reply
* unsubscribe
From: aiolos.cis90 @ 2010-07-17 11:30 UTC (permalink / raw)
To: linuxppc-dev
unsubscribe
^ permalink raw reply
* Re: [PATCH v2]460EX on-chip SATA driver<resubmisison>
From: Sergei Shtylyov @ 2010-07-17 15:51 UTC (permalink / raw)
To: Rupjyoti Sarmah
Cc: linux-ide, rsarmah, linux-kernel, linuxppc-dev, sr, jgarzik
In-Reply-To: <201007061106.o66B631f013777@amcc.com>
Hello.
Rupjyoti Sarmah wrote:
> This patch enables the on-chip DWC SATA controller of the AppliedMicro processor 460EX.
Too bad thius has already been applied but here's my (mostly stylistic)
comments anyway:
> Signed-off-by: Rupjyoti Sarmah <rsarmah@appliedmicro.com>
> Signed-off-by: Mark Miesfeld <mmiesfeld@appliedmicro.com>
> Signed-off-by: Prodyut Hazarika <phazarika@appliedmicro.com>
[...]
> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> new file mode 100644
> index 0000000..ea24c1e
> --- /dev/null
> +++ b/drivers/ata/sata_dwc_460ex.c
> @@ -0,0 +1,1756 @@
> +/*
> + * drivers/ata/sata_dwc_460ex.c
Filenames in the heading comments have long been frowned upon.
> +#ifdef CONFIG_SATA_DWC_DEBUG
I don't see this option defined anywahere.
> +#define DEBUG
> +#endif
> +
> +#ifdef CONFIG_SATA_DWC_VDEBUG
The same about this one.
> +#define VERBOSE_DEBUG
> +#define DEBUG_NCQ
> +#endif
[...]
> +/* SATA DMA driver Globals */
> +#define DMA_NUM_CHANS 1
> +#define DMA_NUM_CHAN_REGS 8
> +
> +/* SATA DMA Register definitions */
> +#define AHB_DMA_BRST_DFLT 64 /* 16 data items burst length*/
Please put a space before */.
> +struct ahb_dma_regs {
> + struct dma_chan_regs chan_regs[DMA_NUM_CHAN_REGS];
> + struct dma_interrupt_regs interrupt_raw; /* Raw Interrupt */
> + struct dma_interrupt_regs interrupt_status; /* Interrupt Status */
> + struct dma_interrupt_regs interrupt_mask; /* Interrupt Mask */
> + struct dma_interrupt_regs interrupt_clear; /* Interrupt Clear */
> + struct dmareg statusInt; /* Interrupt combined*/
No camelCase please, rename it to status_int.
> +#define DMA_CTL_BLK_TS(size) ((size) & 0x000000FFF) /* Blk Transfer size */
> +#define DMA_CHANNEL(ch) (0x00000001 << (ch)) /* Select channel */
> + /* Enable channel */
> +#define DMA_ENABLE_CHAN(ch) ((0x00000001 << (ch)) | \
> + ((0x000000001 << (ch)) << 8))
> + /* Disable channel */
> +#define DMA_DISABLE_CHAN(ch) (0x00000000 | ((0x000000001 << (ch)) << 8))
What's the point of OR'ing with zero?
> +/*
> + * Commonly used DWC SATA driver Macros
> + */
> +#define HSDEV_FROM_HOST(host) ((struct sata_dwc_device *)\
> + (host)->private_data)
> +#define HSDEV_FROM_AP(ap) ((struct sata_dwc_device *)\
> + (ap)->host->private_data)
> +#define HSDEVP_FROM_AP(ap) ((struct sata_dwc_device_port *)\
> + (ap)->private_data)
> +#define HSDEV_FROM_QC(qc) ((struct sata_dwc_device *)\
> + (qc)->ap->host->private_data)
> +#define HSDEV_FROM_HSDEVP(p) ((struct sata_dwc_device *)\
> + (hsdevp)->hsdev)
Are you sure it's '(hsdevp)', not '(p)'?
> +struct sata_dwc_host_priv {
> + void __iomem *scr_addr_sstatus;
> + u32 sata_dwc_sactive_issued ;
> + u32 sata_dwc_sactive_queued ;
Remove spaces befoer semicolons, please.
> +static void sata_dwc_tf_dump(struct ata_taskfile *tf)
> +{
> + dev_vdbg(host_pvt.dwc_dev, "taskfile cmd: 0x%02x protocol: %s flags:"
> + "0x%lx device: %x\n", tf->command, ata_get_cmd_descript\
There's no need to use \ outside macro defintions.
> +/*
> + * Function: get_burst_length_encode
> + * arguments: datalength: length in bytes of data
> + * returns value to be programmed in register corrresponding to data length
> + * This value is effectively the log(base 2) of the length
> + */
> +static int get_burst_length_encode(int datalength)
> +{
> + int items = datalength >> 2; /* div by 4 to get lword count */
> +
> + if (items >= 64)
> + return 5;
> +
> + if (items >= 32)
> + return 4;
> +
> + if (items >= 16)
> + return 3;
> +
> + if (items >= 8)
> + return 2;
> +
> + if (items >= 4)
> + return 1;
> +
> + return 0;
> +}
Hmm, there should be a function in the kernel to calculate 2^n order from
size, something like get_count_order()...
> +/*
> + * Function: dma_dwc_interrupt
> + * arguments: irq, dev_id, pt_regs
> + * returns channel number if available else -1
> + * Interrupt Handler for DW AHB SATA DMA
> + */
> +static irqreturn_t dma_dwc_interrupt(int irq, void *hsdev_instance)
> +{
> + int chan;
> + u32 tfr_reg, err_reg;
> + unsigned long flags;
> + struct sata_dwc_device *hsdev =
> + (struct sata_dwc_device *)hsdev_instance;
> + struct ata_host *host = (struct ata_host *)hsdev->host;
> + struct ata_port *ap;
> + struct sata_dwc_device_port *hsdevp;
> + u8 tag = 0;
> + unsigned int port = 0;
> +
> + spin_lock_irqsave(&host->lock, flags);
> + ap = host->ports[port];
> + hsdevp = HSDEVP_FROM_AP(ap);
> + tag = ap->link.active_tag;
> +
> + tfr_reg = in_le32(&(host_pvt.sata_dma_regs->interrupt_status.tfr\
There's no need to use \ outside macro defintions.
> + .low));
> + err_reg = in_le32(&(host_pvt.sata_dma_regs->interrupt_status.error\
Same comment here...
> + for (chan = 0; chan < DMA_NUM_CHANS; chan++) {
> + /* Check for end-of-transfer interrupt. */
> + if (tfr_reg & DMA_CHANNEL(chan)) {
> + /*
> + * Each DMA command produces 2 interrupts. Only
> + * complete the command after both interrupts have been
> + * seen. (See sata_dwc_isr())
> + */
> + host_pvt.dma_interrupt_count++;
> + sata_dwc_clear_dmacr(hsdevp, tag);
> +
> + if (hsdevp->dma_pending[tag] ==
> + SATA_DWC_DMA_PENDING_NONE) {
> + dev_err(ap->dev, "DMA not pending eot=0x%08x "
> + "err=0x%08x tag=0x%02x pending=%d\n",
> + tfr_reg, err_reg, tag,
> + hsdevp->dma_pending[tag]);
> + }
> +
> + if ((host_pvt.dma_interrupt_count % 2) == 0)
Prens around % operation not necessary.
> + sata_dwc_dma_xfer_complete(ap, 1);
> +
> + /* Clear the interrupt */
> + out_le32(&(host_pvt.sata_dma_regs->interrupt_clear\
\ not needed.
> + /* Clear the interrupt. */
> + out_le32(&(host_pvt.sata_dma_regs->interrupt_clear\
\ not needed.
> +/*
> + * Function: map_sg_to_lli
> + * The Synopsis driver has a comment proposing that better performance
> + * is possible by only enabling interrupts on the last item in the linked list.
> + * However, it seems that could be a problem if an error happened on one of the
> + * first items. The transfer would halt, but no error interrupt would occur.
> + * Currently this function sets interrupts enabled for each linked list item:
> + * DMA_CTL_INT_EN.
> + */
> +static int map_sg_to_lli(struct scatterlist *sg, int num_elems,
> + struct lli *lli, dma_addr_t dma_lli,
> + void __iomem *dmadr_addr, int dir)
> +{
[...]
> + /* Program the LLI CTL high register */
> + lli[idx].ctl.high = cpu_to_le32(DMA_CTL_BLK_TS\
\ not needed.
> + (len / 4));
> +
> + /* Program the next pointer. The next pointer must be
> + * the physical address, not the virtual address.
> + */
> + next_llp = (dma_lli + ((idx + 1) * sizeof(struct \
Same here...
> +static int dma_dwc_xfer_setup(struct scatterlist *sg, int num_elems,
> + struct lli *lli, dma_addr_t dma_lli,
> + void __iomem *addr, int dir)
> +{
> + int dma_ch;
> + int num_lli;
There should be an empty line here.
> +/*
> + * Function: dma_dwc_init
> + * arguments: hsdev
> + * returns status
> + * This function initializes the SATA DMA driver
> + */
> +static int dma_dwc_init(struct sata_dwc_device *hsdev, int irq)
> +{
> + int err;
> +
> + err = dma_request_interrupts(hsdev, irq);
Could be initilaizer...
> + dev_notice(host_pvt.dwc_dev, "DMA initialized\n");
> + dev_dbg(host_pvt.dwc_dev, "SATA DMA registers=0x%p\n", host_pvt.\
\ not needed.
> +static u32 core_scr_read(unsigned int scr)
> +{
> + return in_le32((void __iomem *)(host_pvt.scr_addr_sstatus) +\
Not needed.
> +static void clear_serror(void)
> +{
> + u32 val;
Empty line needed here.
> + val = core_scr_read(SCR_ERROR);
This could be an initilaizer.
> +/* See ahci.c */
> +static void sata_dwc_error_intr(struct ata_port *ap,
> + struct sata_dwc_device *hsdev, uint intpr)
> +{
> + struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
> + struct ata_eh_info *ehi = &ap->link.eh_info;
> + unsigned int err_mask = 0, action = 0;
> + struct ata_queued_cmd *qc;
> + u32 serror;
> + u8 status, tag;
> + u32 err_reg;
> +
> + ata_ehi_clear_desc(ehi);
> +
> + serror = core_scr_read(SCR_ERROR);
> + status = ap->ops->sff_check_status(ap);
Why not call ata_sff_check_status() directly?
> + err_reg = in_le32(&(host_pvt.sata_dma_regs->interrupt_status.error.\
\ not neeeded.
> +/*
> + * Function : sata_dwc_isr
> + * arguments : irq, void *dev_instance, struct pt_regs *regs
> + * Return value : irqreturn_t - status of IRQ
> + * This Interrupt handler called via port ops registered function.
> + * .irq_handler = sata_dwc_isr
> + */
> +static irqreturn_t sata_dwc_isr(int irq, void *dev_instance)
> +{
> + struct ata_host *host = (struct ata_host *)dev_instance;
> + struct sata_dwc_device *hsdev = HSDEV_FROM_HOST(host);
> + struct ata_port *ap;
> + struct ata_queued_cmd *qc;
> + unsigned long flags;
> + u8 status, tag;
> + int handled, num_processed, port = 0;
> + uint intpr, sactive, sactive2, tag_mask;
> + struct sata_dwc_device_port *hsdevp;
> + host_pvt.sata_dwc_sactive_issued = 0;
[...]
> + sactive = core_scr_read(SCR_ACTIVE);
> + tag_mask = (host_pvt.sata_dwc_sactive_issued | sactive) ^ sactive;
Isn't it the same as 'host_pvt.sata_dwc_sactive_issued & ~sactive'?
> + /* If no sactive issued and tag_mask is zero then this is not NCQ */
> + if (host_pvt.sata_dwc_sactive_issued == 0 && tag_mask == 0) {
> + if (ap->link.active_tag == ATA_TAG_POISON)
> + tag = 0;
> + else
> + tag = ap->link.active_tag;
> + qc = ata_qc_from_tag(ap, tag);
> +
> + /* DEV interrupt w/ no active qc? */
> + if (unlikely(!qc || (qc->tf.flags & ATA_TFLAG_POLLING))) {
> + dev_err(ap->dev, "%s interrupt with no active qc "
> + "qc=%p\n", __func__, qc);
> + ap->ops->sff_check_status(ap);
Call ata_sff_check_status() directly...
> + handled = 1;
> + goto DONE;
> + }
> + status = ap->ops->sff_check_status(ap);
Here too...
> +DRVSTILLBUSY:
> + if (ata_is_dma(qc->tf.protocol)) {
> + /*
> + * Each DMA transaction produces 2 interrupts. The DMAC
> + * transfer complete interrupt and the SATA controller
> + * operation done interrupt. The command should be
> + * completed only after both interrupts are seen.
> + */
> + host_pvt.dma_interrupt_count++;
> + if (hsdevp->dma_pending[tag] == \
> + SATA_DWC_DMA_PENDING_NONE) {
> + dev_err(ap->dev, "%s: DMA not pending "
> + "intpr=0x%08x status=0x%08x pending"
> + "=%d\n", __func__, intpr, status,
> + hsdevp->dma_pending[tag]);
> + }
Curly brace not needed here. I assume you haven't run the patch thru
scripts/checkpatch.pl?
> + /*
> + * This is a NCQ command. At this point we need to figure out for which
> + * tags we have gotten a completion interrupt. One interrupt may serve
> + * as completion for more than one operation when commands are queued
> + * (NCQ). We need to process each completed command.
> + */
> +
> + /* process completed commands */
> + sactive = core_scr_read(SCR_ACTIVE);
> + tag_mask = (host_pvt.sata_dwc_sactive_issued | sactive) ^ sactive;
Isn't it the same as 'host_pvt.sata_dwc_sactive_issued & ~sactive'?
> + if (sactive != 0 || (host_pvt.sata_dwc_sactive_issued) > 1 || \
Useless parens around 'host_pvt.sata_dwc_sactive_issued' + \ not needed.
> + tag_mask > 1) {
> + dev_dbg(ap->dev, "%s NCQ:sactive=0x%08x sactive_issued=0x%08x"
> + "tag_mask=0x%08x\n", __func__, sactive,
> + host_pvt.sata_dwc_sactive_issued, tag_mask);
> + }
Curly braces not needed here.
> + if ((tag_mask | (host_pvt.sata_dwc_sactive_issued)) != \
> + (host_pvt.sata_dwc_sactive_issued)) {
Useless parens around 'host_pvt.sata_dwc_sactive_issued'.
> + dev_warn(ap->dev, "Bad tag mask? sactive=0x%08x "
> + "(host_pvt.sata_dwc_sactive_issued)=0x%08x tag_mask"
> + "=0x%08x\n", sactive, host_pvt.sata_dwc_sactive_issued,
> + tag_mask);
> + }
Curly braces not needed around single statement.
> +
> + /* read just to clear ... not bad if currently still busy */
> + status = ap->ops->sff_check_status(ap);
Call ata_sff_check_status() directly.
> + dev_dbg(ap->dev, "%s ATA status register=0x%x\n", __func__, status);
> +
> + tag = 0;
> + num_processed = 0;
> + while (tag_mask) {
> + num_processed++;
> + while (!(tag_mask & 0x00000001)) {
> + tag++;
> + tag_mask <<= 1;
> + }
> +
> + tag_mask &= (~0x00000001);
Useless parens.
> + /* Process completed command */
> + dev_dbg(ap->dev, "%s NCQ command, protocol: %s\n", __func__,
> + ata_get_cmd_descript(qc->tf.protocol));
> + if (ata_is_dma(qc->tf.protocol)) {
> + host_pvt.dma_interrupt_count++;
> + if (hsdevp->dma_pending[tag] == \
\ not needed.
> + SATA_DWC_DMA_PENDING_NONE)
> + dev_warn(ap->dev, "%s: DMA not pending?\n",
> + __func__);
> + if ((host_pvt.dma_interrupt_count % 2) == 0)
Parens not needed around % operation.
> + sata_dwc_dma_xfer_complete(ap, 1);
> + } else {
> + if (unlikely(sata_dwc_qc_complete(ap, qc, 1)))
> + goto STILLBUSY;
> + }
You should write:
} else if (unlikely(sata_dwc_qc_complete(ap, qc, 1))) {
goto STILLBUSY;
}
> + /*
> + * Check to see if any commands completed while we were processing our
> + * initial set of completed commands (read status clears interrupts,
> + * so we might miss a completed command interrupt if one came in while
> + * we were processing --we read status as part of processing a completed
> + * command).
> + */
> + sactive2 = core_scr_read(SCR_ACTIVE);
> + if (sactive2 != sactive) {
{} not needed here.
> +static void sata_dwc_dma_xfer_complete(struct ata_port *ap, u32 check_status)
> +{
> + struct ata_queued_cmd *qc;
> + struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
> + struct sata_dwc_device *hsdev = HSDEV_FROM_AP(ap);
> + u8 tag = 0;
> +
> + tag = ap->link.active_tag;
> + qc = ata_qc_from_tag(ap, tag);
> + if (!qc) {
> + dev_err(ap->dev, "failed to get qc");
> + return;
> + }
> +
> +#ifdef DEBUG_NCQ
> + if (tag > 0) {
Curly braces not needed around single statement.
> + dev_info(ap->dev, "%s tag=%u cmd=0x%02x dma dir=%s proto=%s "
> + "dmacr=0x%08x\n", __func__, qc->tag, qc->tf.command,
> + ata_get_cmd_descript(qc->dma_dir),
> + ata_get_cmd_descript(qc->tf.protocol),
> + in_le32(&(hsdev->sata_dwc_regs->dmacr)));
> + }
> +#endif
> +
> + if (ata_is_dma(qc->tf.protocol)) {
> + if (hsdevp->dma_pending[tag] == SATA_DWC_DMA_PENDING_NONE) {
Curly braces not needed around single statement.
> +static int sata_dwc_qc_complete(struct ata_port *ap, struct ata_queued_cmd *qc,
> + u32 check_status)
> +{
> + u8 status = 0;
> + u32 mask = 0x0;
> + u8 tag = qc->tag;
> + struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
There should be an empty line here.
> + host_pvt.sata_dwc_sactive_queued = 0;
> + dev_dbg(ap->dev, "%s checkstatus? %x\n", __func__, check_status);
> +
> + if (hsdevp->dma_pending[tag] == SATA_DWC_DMA_PENDING_TX)
> + dev_err(ap->dev, "TX DMA PENDING\n");
> + else if (hsdevp->dma_pending[tag] == SATA_DWC_DMA_PENDING_RX)
> + dev_err(ap->dev, "RX DMA PENDING\n");
> + dev_dbg(ap->dev, "QC complete cmd=0x%02x status=0x%02x ata%u:"
> + " protocol=%d\n", qc->tf.command, status, ap->print_id,
> + qc->tf.protocol);
> +
> + /* clear active bit */
> + mask = (~(qcmd_tag_to_mask(tag)));
Useless parens.
> + host_pvt.sata_dwc_sactive_queued = (host_pvt.sata_dwc_sactive_queued) \
> + & mask;
> + host_pvt.sata_dwc_sactive_issued = (host_pvt.sata_dwc_sactive_issued) \
\ not needed.
> +static void sata_dwc_port_stop(struct ata_port *ap)
> +{
> + int i;
> + struct sata_dwc_device *hsdev = HSDEV_FROM_AP(ap);
> + struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
> +
> + dev_dbg(ap->dev, "%s: ap->id = %d\n", __func__, ap->print_id);
> +
> + if (hsdevp && hsdev) {
> + /* deallocate LLI table */
> + for (i = 0; i < SATA_DWC_QCMD_MAX; i++) {
Curly braces not needed.
> + dma_free_coherent(ap->host->dev,
> + SATA_DWC_DMAC_LLI_TBL_SZ,
> + hsdevp->llit[i], hsdevp->llit_dma[i]);
Should be indented on the same level as above line
> +static void sata_dwc_bmdma_setup(struct ata_queued_cmd *qc)
> +{
> + u8 tag = qc->tag;
> +
> + if (ata_is_ncq(qc->tf.protocol)) {
> + dev_dbg(qc->ap->dev, "%s: ap->link.sactive=0x%08x tag=%d\n",
> + __func__, qc->ap->link.sactive, tag);
> + } else {
> + tag = 0;
> + }
Curly braces not needed around single statements.
> +static void sata_dwc_bmdma_start_by_tag(struct ata_queued_cmd *qc, u8 tag)
> +{
> + int start_dma;
> + u32 reg, dma_chan;
> + struct sata_dwc_device *hsdev = HSDEV_FROM_QC(qc);
> + struct ata_port *ap = qc->ap;
> + struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
> + int dir = qc->dma_dir;
There should be an empty line here.
> + if (start_dma) {
> + reg = core_scr_read(SCR_ERROR);
> + if (reg & SATA_DWC_SERROR_ERR_BITS) {
Curly braces not needed here.
> +static void sata_dwc_bmdma_start(struct ata_queued_cmd *qc)
> +{
> + u8 tag = qc->tag;
> +
> + if (ata_is_ncq(qc->tf.protocol)) {
> + dev_dbg(qc->ap->dev, "%s: ap->link.sactive=0x%08x tag=%d\n",
> + __func__, qc->ap->link.sactive, tag);
> + } else {
> + tag = 0;
> + }
Curly braces not needed around single statements.
> +/*
> + * Function : sata_dwc_qc_prep_by_tag
> + * arguments : ata_queued_cmd *qc, u8 tag
> + * Return value : None
> + * qc_prep for a particular queued command based on tag
> + */
> +static void sata_dwc_qc_prep_by_tag(struct ata_queued_cmd *qc, u8 tag)
> +{
[...]
> + dma_chan = dma_dwc_xfer_setup(sg, qc->n_elem, hsdevp->llit[tag],
> + hsdevp->llit_dma[tag],
> + (void *__iomem)(&hsdev->sata_dwc_regs->\
\ not needed.
> +static unsigned int sata_dwc_qc_issue(struct ata_queued_cmd *qc)
> +{
> + u32 sactive;
> + u8 tag = qc->tag;
> + struct ata_port *ap = qc->ap;
[...]
> +
> + if (ata_is_ncq(qc->tf.protocol)) {
> + sactive = core_scr_read(SCR_ACTIVE);
> + sactive |= (0x00000001 << tag);
Parens not needed.
> + core_scr_write(SCR_ACTIVE, sactive);
> +
> + dev_dbg(qc->ap->dev, "%s: tag=%d ap->link.sactive = 0x%08x "
> + "sactive=0x%08x\n", __func__, tag, qc->ap->link.sactive,
> + sactive);
> +
> + ap->ops->sff_tf_load(ap, &qc->tf);
Why not call ata_sff_tf_load() directly?
> +/*
> + * Function : sata_dwc_qc_prep
> + * arguments : ata_queued_cmd *qc
> + * Return value : None
> + * qc_prep for a particular queued command
> + */
> +
> +static void sata_dwc_qc_prep(struct ata_queued_cmd *qc)
> +{
> + if ((qc->dma_dir == DMA_NONE) || (qc->tf.protocol == ATA_PROT_PIO))
Parens not needed arounf == operation.
> + return;
> +
> +#ifdef DEBUG_NCQ
> + if (qc->tag > 0)
> + dev_info(qc->ap->dev, "%s: qc->tag=%d ap->active_tag=0x%08x\n",
> + __func__, tag, qc->ap->link.active_tag);
> +
> + return ;
Remove spade before semicolon please.
> +static const struct ata_port_info sata_dwc_port_info[] = {
> + {
> + .flags = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
> + ATA_FLAG_MMIO | ATA_FLAG_NCQ,
> + .pio_mask = 0x1f, /* pio 0-4 */
Replace 0x1f with ATA_PIO4, please.
> +static int sata_dwc_probe(struct of_device *ofdev,
> + const struct of_device_id *match)
Shouldn't this function be '__init' or '__devinit'?
> +{
> + struct sata_dwc_device *hsdev;
> + u32 idr, versionr;
> + char *ver = (char *)&versionr;
> + u8 *base = NULL;
> + int err = 0;
> + int irq, rc;
> + struct ata_host *host;
> + struct ata_port_info pi = sata_dwc_port_info[0];
> + const struct ata_port_info *ppi[] = { &pi, NULL };
> +
> + /* Allocate DWC SATA device */
> + hsdev = kmalloc(sizeof(*hsdev), GFP_KERNEL);
> + if (hsdev == NULL) {
> + dev_err(&ofdev->dev, "kmalloc failed for hsdev\n");
> + err = -ENOMEM;
> + goto error_out;
> + }
> + memset(hsdev, 0, sizeof(*hsdev));
Use kzalloc() instead iof kmalloc()/memset().
> + /* Get physical SATA DMA register base address */
> + host_pvt.sata_dma_regs = of_iomap(ofdev->dev.of_node, 1);
> + if (!(host_pvt.sata_dma_regs)) {
Parens not needed around 'host_pvt.sata_dma_regs'.
> + /*
> + * Now, register with libATA core, this will also initiate the
> + * device discovery process, invoking our port_start() handler &
> + * error_handler() to execute a dummy Softreset EH session
> + */
> + rc = ata_host_activate(host, irq, sata_dwc_isr, 0, &sata_dwc_sht);
> +
I think that empty line here is not needed.
> + if (rc != 0)
> + dev_err(&ofdev->dev, "failed to activate host");
> +
> + dev_set_drvdata(&ofdev->dev, host);
> + return 0;
> +
> +error_out:
> + /* Free SATA DMA resources */
> + dma_dwc_exit(hsdev);
> +
> + if (base)
> + iounmap(base);
What about freeing 'hsdev' and 'host' and unmapping
'host_pvt.sata_dma_regs'? And does iounmap() really pair with of_iomap()?
> +static int sata_dwc_remove(struct of_device *ofdev)
Shouldn't this be '__exit' or '__devexit'?
> +{
> + struct device *dev = &ofdev->dev;
> + struct ata_host *host = dev_get_drvdata(dev);
> + struct sata_dwc_device *hsdev = host->private_data;
> +
> + ata_host_detach(host);
> + dev_set_drvdata(dev, NULL);
> +
> + /* Free SATA DMA resources */
> + dma_dwc_exit(hsdev);
> +
> + iounmap(hsdev->reg_base);
What about unmapping 'host_pvt.sata_dma_regs'?
> + kfree(hsdev);
> + kfree(host);
Does kfree() really pair with ata_host_alloc_pinfo()?
MBR, Sergei
^ permalink raw reply
* Re: [PATCH V4] powerpc/prom: Export device tree physical address via proc
From: Segher Boessenkool @ 2010-07-17 16:41 UTC (permalink / raw)
To: Grant Likely
Cc: Kumar Gala, Mitch Bradley, Matthew McClintock, linuxppc-dev,
Timur Tabi, David Gibson
In-Reply-To: <AANLkTilDcYYbsH-6f0_HlX9WwaOwG24w1CfhRLplZKS7@mail.gmail.com>
>>>> Yes. Where would we get a list of memreserve sections?
>>>
>>> I would say the list of reserves that are not under the control of
>>> Linux should be explicitly described in the device tree proper. For
>>> instance, if you have a region that firmware depends on, then have a
>>> node for describing the firmware and a property stating the memory
>>> regions that it depends on. The memreserve regions can be generated
>>> from that.
>>
>> Ok, so we could traverse the tree node-by-bode for a
>> persistent-memreserve property and add them to the /memreserve/
>> list in
>> the kexec user space tools?
>
> I *think* that is okay, but I'd like to hear from Segher, Ben, Mitch,
> David Gibson, and other device tree experts on whether or not that
> exact property naming is a good one.
Let's take a step back: what exactly _are_ "memreserve sections", what
are they used for, who (kernel, firmware, bootloader, etc.) holds what
responsibility wrt them?
Segher
^ permalink raw reply
* Re: [PATCH 1/2] Remove REDWOOD_[456] config options and conditional code
From: Artem Bityutskiy @ 2010-07-18 16:52 UTC (permalink / raw)
To: Josh Boyer
Cc: Randy Dunlap, linuxppc-dev, Alexander Kurz, Paul Mackerras,
John Linn, David Brown, Ladislav Michl, Solomon Peachy, vamos-dev,
Mike Frysinger, Florian Fainelli, Artem Bityutskiy,
Christian Dietrich, Nicolas Pitre, Jiri Kosina, linux-kernel,
Milton Miller, netdev, Joe Perches, linux-mtd, David Woodhouse,
David S. Miller
In-Reply-To: <20100716142055.GA11736@zod.rchland.ibm.com>
On Fri, 2010-07-16 at 10:20 -0400, Josh Boyer wrote:
> On Fri, Jul 16, 2010 at 02:29:02PM +0200, Christian Dietrich wrote:
> >The config options for REDWOOD_[456] were commented out in the powerpc
> >Kconfig. The ifdefs referencing this options therefore are dead and all
> >references to this can be removed (Also dependencies in other KConfig
> >files).
> >
> >Signed-off-by: Christian Dietrich <qy03fugy@stud.informatik.uni-erlangen.de>
> >Signed-off-by: Christoph Egger <siccegge@cs.fau.de>
>
> This seems fine with me.
>
> The only question is which tree it coms through. I'm happy to take it
> in via mine if the netdev and MTD people are fine with that. Otherwise,
> my ack is below.
>
> Acked-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>
You know how slow MTD people may be sometimes, so I'd suggest you to
merge this via whatever tree. David is in CC, he'll complain if he is
unhappy, I think.
--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
^ permalink raw reply
* RE: [PATCH v2]460EX on-chip SATA driver<resubmisison>
From: Rupjyoti Sarmah @ 2010-07-18 17:33 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: linux-ide, sr, jgarzik, linux-kernel, linuxppc-dev
In-Reply-To: <4C41D174.2040907@ru.mvista.com>
Hi Sergei,
Thanks for your suggestions.
I would look into them and submit a patch for style improvement some time
later.
Regards,
Rup
-----Original Message-----
From: Sergei Shtylyov [mailto:sshtylyov@mvista.com]
Sent: Saturday, July 17, 2010 9:21 PM
To: Rupjyoti Sarmah
Cc: linux-ide@vger.kernel.org; linux-kernel@vger.kernel.org;
rsarmah@apm.com; jgarzik@pobox.com; sr@denx.de; linuxppc-dev@ozlabs.org
Subject: Re: [PATCH v2]460EX on-chip SATA driver<resubmisison>
Hello.
Rupjyoti Sarmah wrote:
> This patch enables the on-chip DWC SATA controller of the AppliedMicro
processor 460EX.
Too bad thius has already been applied but here's my (mostly
stylistic)
comments anyway:
> Signed-off-by: Rupjyoti Sarmah <rsarmah@appliedmicro.com>
> Signed-off-by: Mark Miesfeld <mmiesfeld@appliedmicro.com>
> Signed-off-by: Prodyut Hazarika <phazarika@appliedmicro.com>
[...]
> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> new file mode 100644
> index 0000000..ea24c1e
> --- /dev/null
> +++ b/drivers/ata/sata_dwc_460ex.c
> @@ -0,0 +1,1756 @@
> +/*
> + * drivers/ata/sata_dwc_460ex.c
Filenames in the heading comments have long been frowned upon.
> +#ifdef CONFIG_SATA_DWC_DEBUG
I don't see this option defined anywahere.
> +#define DEBUG
> +#endif
> +
> +#ifdef CONFIG_SATA_DWC_VDEBUG
The same about this one.
> +#define VERBOSE_DEBUG
> +#define DEBUG_NCQ
> +#endif
[...]
> +/* SATA DMA driver Globals */
> +#define DMA_NUM_CHANS 1
> +#define DMA_NUM_CHAN_REGS 8
> +
> +/* SATA DMA Register definitions */
> +#define AHB_DMA_BRST_DFLT 64 /* 16 data items burst length*/
Please put a space before */.
> +struct ahb_dma_regs {
> + struct dma_chan_regs chan_regs[DMA_NUM_CHAN_REGS];
> + struct dma_interrupt_regs interrupt_raw; /* Raw Interrupt
*/
> + struct dma_interrupt_regs interrupt_status; /* Interrupt
Status */
> + struct dma_interrupt_regs interrupt_mask; /* Interrupt Mask
*/
> + struct dma_interrupt_regs interrupt_clear; /* Interrupt Clear
*/
> + struct dmareg statusInt; /* Interrupt combined*/
No camelCase please, rename it to status_int.
> +#define DMA_CTL_BLK_TS(size) ((size) & 0x000000FFF) /* Blk
Transfer size */
> +#define DMA_CHANNEL(ch) (0x00000001 << (ch)) /* Select
channel */
> + /* Enable channel */
> +#define DMA_ENABLE_CHAN(ch) ((0x00000001 << (ch)) |
\
> + ((0x000000001 << (ch)) << 8))
> + /* Disable channel */
> +#define DMA_DISABLE_CHAN(ch) (0x00000000 | ((0x000000001 <<
(ch)) << 8))
What's the point of OR'ing with zero?
> +/*
> + * Commonly used DWC SATA driver Macros
> + */
> +#define HSDEV_FROM_HOST(host) ((struct sata_dwc_device *)\
> + (host)->private_data)
> +#define HSDEV_FROM_AP(ap) ((struct sata_dwc_device *)\
> + (ap)->host->private_data)
> +#define HSDEVP_FROM_AP(ap) ((struct sata_dwc_device_port *)\
> + (ap)->private_data)
> +#define HSDEV_FROM_QC(qc) ((struct sata_dwc_device *)\
> + (qc)->ap->host->private_data)
> +#define HSDEV_FROM_HSDEVP(p) ((struct sata_dwc_device *)\
> + (hsdevp)->hsdev)
Are you sure it's '(hsdevp)', not '(p)'?
> +struct sata_dwc_host_priv {
> + void __iomem *scr_addr_sstatus;
> + u32 sata_dwc_sactive_issued ;
> + u32 sata_dwc_sactive_queued ;
Remove spaces befoer semicolons, please.
> +static void sata_dwc_tf_dump(struct ata_taskfile *tf)
> +{
> + dev_vdbg(host_pvt.dwc_dev, "taskfile cmd: 0x%02x protocol: %s
flags:"
> + "0x%lx device: %x\n", tf->command, ata_get_cmd_descript\
There's no need to use \ outside macro defintions.
> +/*
> + * Function: get_burst_length_encode
> + * arguments: datalength: length in bytes of data
> + * returns value to be programmed in register corrresponding to data
length
> + * This value is effectively the log(base 2) of the length
> + */
> +static int get_burst_length_encode(int datalength)
> +{
> + int items = datalength >> 2; /* div by 4 to get lword count */
> +
> + if (items >= 64)
> + return 5;
> +
> + if (items >= 32)
> + return 4;
> +
> + if (items >= 16)
> + return 3;
> +
> + if (items >= 8)
> + return 2;
> +
> + if (items >= 4)
> + return 1;
> +
> + return 0;
> +}
Hmm, there should be a function in the kernel to calculate 2^n order
from
size, something like get_count_order()...
> +/*
> + * Function: dma_dwc_interrupt
> + * arguments: irq, dev_id, pt_regs
> + * returns channel number if available else -1
> + * Interrupt Handler for DW AHB SATA DMA
> + */
> +static irqreturn_t dma_dwc_interrupt(int irq, void *hsdev_instance)
> +{
> + int chan;
> + u32 tfr_reg, err_reg;
> + unsigned long flags;
> + struct sata_dwc_device *hsdev =
> + (struct sata_dwc_device *)hsdev_instance;
> + struct ata_host *host = (struct ata_host *)hsdev->host;
> + struct ata_port *ap;
> + struct sata_dwc_device_port *hsdevp;
> + u8 tag = 0;
> + unsigned int port = 0;
> +
> + spin_lock_irqsave(&host->lock, flags);
> + ap = host->ports[port];
> + hsdevp = HSDEVP_FROM_AP(ap);
> + tag = ap->link.active_tag;
> +
> + tfr_reg = in_le32(&(host_pvt.sata_dma_regs->interrupt_status.tfr\
There's no need to use \ outside macro defintions.
> + .low));
> + err_reg =
in_le32(&(host_pvt.sata_dma_regs->interrupt_status.error\
Same comment here...
> + for (chan = 0; chan < DMA_NUM_CHANS; chan++) {
> + /* Check for end-of-transfer interrupt. */
> + if (tfr_reg & DMA_CHANNEL(chan)) {
> + /*
> + * Each DMA command produces 2 interrupts. Only
> + * complete the command after both interrupts have
been
> + * seen. (See sata_dwc_isr())
> + */
> + host_pvt.dma_interrupt_count++;
> + sata_dwc_clear_dmacr(hsdevp, tag);
> +
> + if (hsdevp->dma_pending[tag] ==
> + SATA_DWC_DMA_PENDING_NONE) {
> + dev_err(ap->dev, "DMA not pending
eot=0x%08x "
> + "err=0x%08x tag=0x%02x
pending=%d\n",
> + tfr_reg, err_reg, tag,
> + hsdevp->dma_pending[tag]);
> + }
> +
> + if ((host_pvt.dma_interrupt_count % 2) == 0)
Prens around % operation not necessary.
> + sata_dwc_dma_xfer_complete(ap, 1);
> +
> + /* Clear the interrupt */
> +
out_le32(&(host_pvt.sata_dma_regs->interrupt_clear\
\ not needed.
> + /* Clear the interrupt. */
> +
out_le32(&(host_pvt.sata_dma_regs->interrupt_clear\
\ not needed.
> +/*
> + * Function: map_sg_to_lli
> + * The Synopsis driver has a comment proposing that better performance
> + * is possible by only enabling interrupts on the last item in the
linked list.
> + * However, it seems that could be a problem if an error happened on
one of the
> + * first items. The transfer would halt, but no error interrupt would
occur.
> + * Currently this function sets interrupts enabled for each linked list
item:
> + * DMA_CTL_INT_EN.
> + */
> +static int map_sg_to_lli(struct scatterlist *sg, int num_elems,
> + struct lli *lli, dma_addr_t dma_lli,
> + void __iomem *dmadr_addr, int dir)
> +{
[...]
> + /* Program the LLI CTL high register */
> + lli[idx].ctl.high = cpu_to_le32(DMA_CTL_BLK_TS\
\ not needed.
> + (len / 4));
> +
> + /* Program the next pointer. The next pointer
must be
> + * the physical address, not the virtual address.
> + */
> + next_llp = (dma_lli + ((idx + 1) * sizeof(struct \
Same here...
> +static int dma_dwc_xfer_setup(struct scatterlist *sg, int num_elems,
> + struct lli *lli, dma_addr_t dma_lli,
> + void __iomem *addr, int dir)
> +{
> + int dma_ch;
> + int num_lli;
There should be an empty line here.
> +/*
> + * Function: dma_dwc_init
> + * arguments: hsdev
> + * returns status
> + * This function initializes the SATA DMA driver
> + */
> +static int dma_dwc_init(struct sata_dwc_device *hsdev, int irq)
> +{
> + int err;
> +
> + err = dma_request_interrupts(hsdev, irq);
Could be initilaizer...
> + dev_notice(host_pvt.dwc_dev, "DMA initialized\n");
> + dev_dbg(host_pvt.dwc_dev, "SATA DMA registers=0x%p\n", host_pvt.\
\ not needed.
> +static u32 core_scr_read(unsigned int scr)
> +{
> + return in_le32((void __iomem *)(host_pvt.scr_addr_sstatus) +\
Not needed.
> +static void clear_serror(void)
> +{
> + u32 val;
Empty line needed here.
> + val = core_scr_read(SCR_ERROR);
This could be an initilaizer.
> +/* See ahci.c */
> +static void sata_dwc_error_intr(struct ata_port *ap,
> + struct sata_dwc_device *hsdev, uint intpr)
> +{
> + struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
> + struct ata_eh_info *ehi = &ap->link.eh_info;
> + unsigned int err_mask = 0, action = 0;
> + struct ata_queued_cmd *qc;
> + u32 serror;
> + u8 status, tag;
> + u32 err_reg;
> +
> + ata_ehi_clear_desc(ehi);
> +
> + serror = core_scr_read(SCR_ERROR);
> + status = ap->ops->sff_check_status(ap);
Why not call ata_sff_check_status() directly?
> + err_reg =
in_le32(&(host_pvt.sata_dma_regs->interrupt_status.error.\
\ not neeeded.
> +/*
> + * Function : sata_dwc_isr
> + * arguments : irq, void *dev_instance, struct pt_regs *regs
> + * Return value : irqreturn_t - status of IRQ
> + * This Interrupt handler called via port ops registered function.
> + * .irq_handler = sata_dwc_isr
> + */
> +static irqreturn_t sata_dwc_isr(int irq, void *dev_instance)
> +{
> + struct ata_host *host = (struct ata_host *)dev_instance;
> + struct sata_dwc_device *hsdev = HSDEV_FROM_HOST(host);
> + struct ata_port *ap;
> + struct ata_queued_cmd *qc;
> + unsigned long flags;
> + u8 status, tag;
> + int handled, num_processed, port = 0;
> + uint intpr, sactive, sactive2, tag_mask;
> + struct sata_dwc_device_port *hsdevp;
> + host_pvt.sata_dwc_sactive_issued = 0;
[...]
> + sactive = core_scr_read(SCR_ACTIVE);
> + tag_mask = (host_pvt.sata_dwc_sactive_issued | sactive) ^ sactive;
Isn't it the same as 'host_pvt.sata_dwc_sactive_issued & ~sactive'?
> + /* If no sactive issued and tag_mask is zero then this is not NCQ
*/
> + if (host_pvt.sata_dwc_sactive_issued == 0 && tag_mask == 0) {
> + if (ap->link.active_tag == ATA_TAG_POISON)
> + tag = 0;
> + else
> + tag = ap->link.active_tag;
> + qc = ata_qc_from_tag(ap, tag);
> +
> + /* DEV interrupt w/ no active qc? */
> + if (unlikely(!qc || (qc->tf.flags & ATA_TFLAG_POLLING))) {
> + dev_err(ap->dev, "%s interrupt with no active qc "
> + "qc=%p\n", __func__, qc);
> + ap->ops->sff_check_status(ap);
Call ata_sff_check_status() directly...
> + handled = 1;
> + goto DONE;
> + }
> + status = ap->ops->sff_check_status(ap);
Here too...
> +DRVSTILLBUSY:
> + if (ata_is_dma(qc->tf.protocol)) {
> + /*
> + * Each DMA transaction produces 2 interrupts. The
DMAC
> + * transfer complete interrupt and the SATA
controller
> + * operation done interrupt. The command should be
> + * completed only after both interrupts are seen.
> + */
> + host_pvt.dma_interrupt_count++;
> + if (hsdevp->dma_pending[tag] == \
> + SATA_DWC_DMA_PENDING_NONE) {
> + dev_err(ap->dev, "%s: DMA not pending "
> + "intpr=0x%08x status=0x%08x
pending"
> + "=%d\n", __func__, intpr, status,
> + hsdevp->dma_pending[tag]);
> + }
Curly brace not needed here. I assume you haven't run the patch thru
scripts/checkpatch.pl?
> + /*
> + * This is a NCQ command. At this point we need to figure out for
which
> + * tags we have gotten a completion interrupt. One interrupt may
serve
> + * as completion for more than one operation when commands are
queued
> + * (NCQ). We need to process each completed command.
> + */
> +
> + /* process completed commands */
> + sactive = core_scr_read(SCR_ACTIVE);
> + tag_mask = (host_pvt.sata_dwc_sactive_issued | sactive) ^ sactive;
Isn't it the same as 'host_pvt.sata_dwc_sactive_issued & ~sactive'?
> + if (sactive != 0 || (host_pvt.sata_dwc_sactive_issued) > 1 || \
Useless parens around 'host_pvt.sata_dwc_sactive_issued' + \ not
needed.
> + tag_mask > 1) {
> + dev_dbg(ap->dev, "%s NCQ:sactive=0x%08x
sactive_issued=0x%08x"
> + "tag_mask=0x%08x\n", __func__, sactive,
> + host_pvt.sata_dwc_sactive_issued, tag_mask);
> + }
Curly braces not needed here.
> + if ((tag_mask | (host_pvt.sata_dwc_sactive_issued)) != \
> +
(host_pvt.sata_dwc_sactive_issued)) {
Useless parens around 'host_pvt.sata_dwc_sactive_issued'.
> + dev_warn(ap->dev, "Bad tag mask? sactive=0x%08x "
> + "(host_pvt.sata_dwc_sactive_issued)=0x%08x
tag_mask"
> + "=0x%08x\n", sactive,
host_pvt.sata_dwc_sactive_issued,
> + tag_mask);
> + }
Curly braces not needed around single statement.
> +
> + /* read just to clear ... not bad if currently still busy */
> + status = ap->ops->sff_check_status(ap);
Call ata_sff_check_status() directly.
> + dev_dbg(ap->dev, "%s ATA status register=0x%x\n", __func__,
status);
> +
> + tag = 0;
> + num_processed = 0;
> + while (tag_mask) {
> + num_processed++;
> + while (!(tag_mask & 0x00000001)) {
> + tag++;
> + tag_mask <<= 1;
> + }
> +
> + tag_mask &= (~0x00000001);
Useless parens.
> + /* Process completed command */
> + dev_dbg(ap->dev, "%s NCQ command, protocol: %s\n",
__func__,
> + ata_get_cmd_descript(qc->tf.protocol));
> + if (ata_is_dma(qc->tf.protocol)) {
> + host_pvt.dma_interrupt_count++;
> + if (hsdevp->dma_pending[tag] == \
\ not needed.
> + SATA_DWC_DMA_PENDING_NONE)
> + dev_warn(ap->dev, "%s: DMA not
pending?\n",
> + __func__);
> + if ((host_pvt.dma_interrupt_count % 2) == 0)
Parens not needed around % operation.
> + sata_dwc_dma_xfer_complete(ap, 1);
> + } else {
> + if (unlikely(sata_dwc_qc_complete(ap, qc, 1)))
> + goto STILLBUSY;
> + }
You should write:
} else if (unlikely(sata_dwc_qc_complete(ap, qc, 1))) {
goto STILLBUSY;
}
> + /*
> + * Check to see if any commands completed while we were processing
our
> + * initial set of completed commands (read status clears
interrupts,
> + * so we might miss a completed command interrupt if one came in
while
> + * we were processing --we read status as part of processing a
completed
> + * command).
> + */
> + sactive2 = core_scr_read(SCR_ACTIVE);
> + if (sactive2 != sactive) {
{} not needed here.
> +static void sata_dwc_dma_xfer_complete(struct ata_port *ap, u32
check_status)
> +{
> + struct ata_queued_cmd *qc;
> + struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
> + struct sata_dwc_device *hsdev = HSDEV_FROM_AP(ap);
> + u8 tag = 0;
> +
> + tag = ap->link.active_tag;
> + qc = ata_qc_from_tag(ap, tag);
> + if (!qc) {
> + dev_err(ap->dev, "failed to get qc");
> + return;
> + }
> +
> +#ifdef DEBUG_NCQ
> + if (tag > 0) {
Curly braces not needed around single statement.
> + dev_info(ap->dev, "%s tag=%u cmd=0x%02x dma dir=%s
proto=%s "
> + "dmacr=0x%08x\n", __func__, qc->tag,
qc->tf.command,
> + ata_get_cmd_descript(qc->dma_dir),
> + ata_get_cmd_descript(qc->tf.protocol),
> + in_le32(&(hsdev->sata_dwc_regs->dmacr)));
> + }
> +#endif
> +
> + if (ata_is_dma(qc->tf.protocol)) {
> + if (hsdevp->dma_pending[tag] == SATA_DWC_DMA_PENDING_NONE)
{
Curly braces not needed around single statement.
> +static int sata_dwc_qc_complete(struct ata_port *ap, struct
ata_queued_cmd *qc,
> + u32 check_status)
> +{
> + u8 status = 0;
> + u32 mask = 0x0;
> + u8 tag = qc->tag;
> + struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
There should be an empty line here.
> + host_pvt.sata_dwc_sactive_queued = 0;
> + dev_dbg(ap->dev, "%s checkstatus? %x\n", __func__, check_status);
> +
> + if (hsdevp->dma_pending[tag] == SATA_DWC_DMA_PENDING_TX)
> + dev_err(ap->dev, "TX DMA PENDING\n");
> + else if (hsdevp->dma_pending[tag] == SATA_DWC_DMA_PENDING_RX)
> + dev_err(ap->dev, "RX DMA PENDING\n");
> + dev_dbg(ap->dev, "QC complete cmd=0x%02x status=0x%02x ata%u:"
> + " protocol=%d\n", qc->tf.command, status, ap->print_id,
> + qc->tf.protocol);
> +
> + /* clear active bit */
> + mask = (~(qcmd_tag_to_mask(tag)));
Useless parens.
> + host_pvt.sata_dwc_sactive_queued =
(host_pvt.sata_dwc_sactive_queued) \
> + & mask;
> + host_pvt.sata_dwc_sactive_issued =
(host_pvt.sata_dwc_sactive_issued) \
\ not needed.
> +static void sata_dwc_port_stop(struct ata_port *ap)
> +{
> + int i;
> + struct sata_dwc_device *hsdev = HSDEV_FROM_AP(ap);
> + struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
> +
> + dev_dbg(ap->dev, "%s: ap->id = %d\n", __func__, ap->print_id);
> +
> + if (hsdevp && hsdev) {
> + /* deallocate LLI table */
> + for (i = 0; i < SATA_DWC_QCMD_MAX; i++) {
Curly braces not needed.
> + dma_free_coherent(ap->host->dev,
> + SATA_DWC_DMAC_LLI_TBL_SZ,
> + hsdevp->llit[i],
hsdevp->llit_dma[i]);
Should be indented on the same level as above line
> +static void sata_dwc_bmdma_setup(struct ata_queued_cmd *qc)
> +{
> + u8 tag = qc->tag;
> +
> + if (ata_is_ncq(qc->tf.protocol)) {
> + dev_dbg(qc->ap->dev, "%s: ap->link.sactive=0x%08x
tag=%d\n",
> + __func__, qc->ap->link.sactive, tag);
> + } else {
> + tag = 0;
> + }
Curly braces not needed around single statements.
> +static void sata_dwc_bmdma_start_by_tag(struct ata_queued_cmd *qc, u8
tag)
> +{
> + int start_dma;
> + u32 reg, dma_chan;
> + struct sata_dwc_device *hsdev = HSDEV_FROM_QC(qc);
> + struct ata_port *ap = qc->ap;
> + struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
> + int dir = qc->dma_dir;
There should be an empty line here.
> + if (start_dma) {
> + reg = core_scr_read(SCR_ERROR);
> + if (reg & SATA_DWC_SERROR_ERR_BITS) {
Curly braces not needed here.
> +static void sata_dwc_bmdma_start(struct ata_queued_cmd *qc)
> +{
> + u8 tag = qc->tag;
> +
> + if (ata_is_ncq(qc->tf.protocol)) {
> + dev_dbg(qc->ap->dev, "%s: ap->link.sactive=0x%08x
tag=%d\n",
> + __func__, qc->ap->link.sactive, tag);
> + } else {
> + tag = 0;
> + }
Curly braces not needed around single statements.
> +/*
> + * Function : sata_dwc_qc_prep_by_tag
> + * arguments : ata_queued_cmd *qc, u8 tag
> + * Return value : None
> + * qc_prep for a particular queued command based on tag
> + */
> +static void sata_dwc_qc_prep_by_tag(struct ata_queued_cmd *qc, u8 tag)
> +{
[...]
> + dma_chan = dma_dwc_xfer_setup(sg, qc->n_elem, hsdevp->llit[tag],
> + hsdevp->llit_dma[tag],
> + (void
*__iomem)(&hsdev->sata_dwc_regs->\
\ not needed.
> +static unsigned int sata_dwc_qc_issue(struct ata_queued_cmd *qc)
> +{
> + u32 sactive;
> + u8 tag = qc->tag;
> + struct ata_port *ap = qc->ap;
[...]
> +
> + if (ata_is_ncq(qc->tf.protocol)) {
> + sactive = core_scr_read(SCR_ACTIVE);
> + sactive |= (0x00000001 << tag);
Parens not needed.
> + core_scr_write(SCR_ACTIVE, sactive);
> +
> + dev_dbg(qc->ap->dev, "%s: tag=%d ap->link.sactive = 0x%08x
"
> + "sactive=0x%08x\n", __func__, tag,
qc->ap->link.sactive,
> + sactive);
> +
> + ap->ops->sff_tf_load(ap, &qc->tf);
Why not call ata_sff_tf_load() directly?
> +/*
> + * Function : sata_dwc_qc_prep
> + * arguments : ata_queued_cmd *qc
> + * Return value : None
> + * qc_prep for a particular queued command
> + */
> +
> +static void sata_dwc_qc_prep(struct ata_queued_cmd *qc)
> +{
> + if ((qc->dma_dir == DMA_NONE) || (qc->tf.protocol ==
ATA_PROT_PIO))
Parens not needed arounf == operation.
> + return;
> +
> +#ifdef DEBUG_NCQ
> + if (qc->tag > 0)
> + dev_info(qc->ap->dev, "%s: qc->tag=%d
ap->active_tag=0x%08x\n",
> + __func__, tag, qc->ap->link.active_tag);
> +
> + return ;
Remove spade before semicolon please.
> +static const struct ata_port_info sata_dwc_port_info[] = {
> + {
> + .flags = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
> + ATA_FLAG_MMIO | ATA_FLAG_NCQ,
> + .pio_mask = 0x1f, /* pio 0-4 */
Replace 0x1f with ATA_PIO4, please.
> +static int sata_dwc_probe(struct of_device *ofdev,
> + const struct of_device_id *match)
Shouldn't this function be '__init' or '__devinit'?
> +{
> + struct sata_dwc_device *hsdev;
> + u32 idr, versionr;
> + char *ver = (char *)&versionr;
> + u8 *base = NULL;
> + int err = 0;
> + int irq, rc;
> + struct ata_host *host;
> + struct ata_port_info pi = sata_dwc_port_info[0];
> + const struct ata_port_info *ppi[] = { &pi, NULL };
> +
> + /* Allocate DWC SATA device */
> + hsdev = kmalloc(sizeof(*hsdev), GFP_KERNEL);
> + if (hsdev == NULL) {
> + dev_err(&ofdev->dev, "kmalloc failed for hsdev\n");
> + err = -ENOMEM;
> + goto error_out;
> + }
> + memset(hsdev, 0, sizeof(*hsdev));
Use kzalloc() instead iof kmalloc()/memset().
> + /* Get physical SATA DMA register base address */
> + host_pvt.sata_dma_regs = of_iomap(ofdev->dev.of_node, 1);
> + if (!(host_pvt.sata_dma_regs)) {
Parens not needed around 'host_pvt.sata_dma_regs'.
> + /*
> + * Now, register with libATA core, this will also initiate the
> + * device discovery process, invoking our port_start() handler &
> + * error_handler() to execute a dummy Softreset EH session
> + */
> + rc = ata_host_activate(host, irq, sata_dwc_isr, 0, &sata_dwc_sht);
> +
I think that empty line here is not needed.
> + if (rc != 0)
> + dev_err(&ofdev->dev, "failed to activate host");
> +
> + dev_set_drvdata(&ofdev->dev, host);
> + return 0;
> +
> +error_out:
> + /* Free SATA DMA resources */
> + dma_dwc_exit(hsdev);
> +
> + if (base)
> + iounmap(base);
What about freeing 'hsdev' and 'host' and unmapping
'host_pvt.sata_dma_regs'? And does iounmap() really pair with of_iomap()?
> +static int sata_dwc_remove(struct of_device *ofdev)
Shouldn't this be '__exit' or '__devexit'?
> +{
> + struct device *dev = &ofdev->dev;
> + struct ata_host *host = dev_get_drvdata(dev);
> + struct sata_dwc_device *hsdev = host->private_data;
> +
> + ata_host_detach(host);
> + dev_set_drvdata(dev, NULL);
> +
> + /* Free SATA DMA resources */
> + dma_dwc_exit(hsdev);
> +
> + iounmap(hsdev->reg_base);
What about unmapping 'host_pvt.sata_dma_regs'?
> + kfree(hsdev);
> + kfree(host);
Does kfree() really pair with ata_host_alloc_pinfo()?
MBR, Sergei
^ permalink raw reply
* Re: Badness with the kernel version 2.6.35-rc1-git1 running on P6 box
From: David Miller @ 2010-07-18 21:51 UTC (permalink / raw)
To: eric.dumazet
Cc: sachinp, netdev, linux-kernel, linuxppc-dev, ossthema, dipraksh
In-Reply-To: <1279282842.2549.16.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 16 Jul 2010 14:20:42 +0200
> Le vendredi 16 juillet 2010 =E0 11:56 +0200, Eric Dumazet a =E9crit :=
> =
>> [PATCH] ehea: ehea_get_stats() should use GFP_KERNEL
>> =
>> ehea_get_stats() is called in process context and should use GFP_KER=
NEL
>> allocation instead of GFP_ATOMIC.
>> =
>> Clearing stats at beginning of ehea_get_stats() is racy in case of
>> concurrent stat readers.
>> =
>> get_stats() can also use netdev net_device_stats, instead of a priva=
te
>> copy.
>> =
>> Reported-by: divya <dipraksh@linux.vnet.ibm.com>
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> ---
>> drivers/net/ehea/ehea.h | 1 -
>> drivers/net/ehea/ehea_main.c | 6 ++----
>> 2 files changed, 2 insertions(+), 5 deletions(-)
>> =
>> =
> =
> Hmm, net-next-2.6 contains following patch :
If people think ehea usage is ubiquitous enough to deserve a backport
of this to net-2.6, fine. But personally I don't think it's worth it.
Can someone close the kernel bugzilla 16406 created for this bug? This=
patch we have already obviously would fix this issue.
^ permalink raw reply
* Re: [PATCH V4] powerpc/prom: Export device tree physical address via proc
From: Benjamin Herrenschmidt @ 2010-07-18 23:41 UTC (permalink / raw)
To: Grant Likely; +Cc: Matthew McClintock, Kumar Gala, linuxppc-dev, Timur Tabi
In-Reply-To: <AANLkTimiM1Ky7aIYoQ3efhivypV90LFG-BpueuaPaiPf@mail.gmail.com>
On Thu, 2010-07-15 at 00:21 -0600, Grant Likely wrote:
> On Wed, Jul 14, 2010 at 9:18 AM, Matthew McClintock <msm@freescale.com> wrote:
> > To build a proper flat device tree for kexec we need to know which
> > memreserve region was used for the device tree for the currently
> > running kernel, so we can remove it and replace it with the new
> > memreserve for the kexec'ed kernel
> >
> > Signed-off-by: Matthew McClintock <msm@freescale.com>
>
> Hi Matthew.
>
> I don't understand. Why does userspace need to know about the old
> memreserve sections? Doesn't kexec tear down all of the old
> allocations anyway? How are they relevant for constructing the dtb
> for the kexec kernel? I'll need a lot more details before I consider
> merging this.
>
> Also, please cc: me and Ben Herrenschmidt on powerpc related device
> tree changes.
I admit to be the victim of a similar misunderstanding...
Care to explain in more details ? (with examples)
Cheers,
Ben.
> Cheers,
> g.
>
> > ---
> > V4: Fixed misspelling
> >
> > V3: Remove unneeded cast, and fixed indentation screw up
> >
> > V2: messed up changes
> >
> > arch/powerpc/kernel/prom.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
> > 1 files changed, 45 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> > index fd9359a..ff3e240 100644
> > --- a/arch/powerpc/kernel/prom.c
> > +++ b/arch/powerpc/kernel/prom.c
> > @@ -32,6 +32,7 @@
> > #include <linux/debugfs.h>
> > #include <linux/irq.h>
> > #include <linux/lmb.h>
> > +#include <linux/bootmem.h>
> >
> > #include <asm/prom.h>
> > #include <asm/rtas.h>
> > @@ -911,3 +912,47 @@ static int __init export_flat_device_tree(void)
> > }
> > __initcall(export_flat_device_tree);
> > #endif
> > +
> > +#ifdef CONFIG_KEXEC
> > +static phys_addr_t flat_dt_start;
> > +static phys_addr_t flat_dt_end;
> > +
> > +static struct property flat_dt_start_prop = {
> > + .name = "linux,devicetree-start",
> > + .length = sizeof(phys_addr_t),
> > + .value = &flat_dt_start,
> > +};
> > +
> > +static struct property flat_dt_end_prop = {
> > + .name = "linux,devicetree-end",
> > + .length = sizeof(phys_addr_t),
> > + .value = &flat_dt_end,
> > +};
> > +
> > +static int __init export_flat_device_tree_phys_addr(void)
> > +{
> > + struct property *prop;
> > + struct device_node *node;
> > +
> > + node = of_find_node_by_path("/chosen");
> > + if (!node)
> > + return -ENOENT;
> > +
> > + prop = of_find_property(node, "linux,devicetree-start", NULL);
> > + if (prop)
> > + prom_remove_property(node, prop);
> > +
> > + prop = of_find_property(node, "linux,devicetree-end", NULL);
> > + if (prop)
> > + prom_remove_property(node, prop);
> > +
> > + flat_dt_start = virt_to_phys(initial_boot_params);
> > + flat_dt_end = virt_to_phys(initial_boot_params)
> > + + initial_boot_params->totalsize;
> > + prom_add_property(node, &flat_dt_start_prop);
> > + prom_add_property(node, &flat_dt_end_prop);
> > +
> > + return 0;
> > +}
> > +__initcall(export_flat_device_tree_phys_addr);
> > +#endif
> > --
> > 1.6.6.1
> >
> >
> > _______________________________________________
> > Linuxppc-dev mailing list
> > Linuxppc-dev@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/linuxppc-dev
> >
>
>
>
^ permalink raw reply
* Re: [PATCH 1/2] Remove REDWOOD_[456] config options and conditional code
From: Benjamin Herrenschmidt @ 2010-07-19 0:00 UTC (permalink / raw)
To: Christian Dietrich
Cc: Randy Dunlap, linuxppc-dev, Alexander Kurz, Paul Mackerras,
John Linn, David Brown, Ladislav Michl, Solomon Peachy, vamos-dev,
Mike Frysinger, Florian Fainelli, Artem Bityutskiy, Nicolas Pitre,
Jiri Kosina, linux-kernel, Milton Miller, netdev, Joe Perches,
linux-mtd, David Woodhouse, David S. Miller
In-Reply-To: <ca1bb25d203618c3548748f5efb6f125a96c89e0.1279282865.git.qy03fugy@stud.informatik.uni-erlangen.de>
On Fri, 2010-07-16 at 14:29 +0200, Christian Dietrich wrote:
> The config options for REDWOOD_[456] were commented out in the powerpc
> Kconfig. The ifdefs referencing this options therefore are dead and all
> references to this can be removed (Also dependencies in other KConfig
> files).
>
> Signed-off-by: Christian Dietrich <qy03fugy@stud.informatik.uni-erlangen.de>
> Signed-off-by: Christoph Egger <siccegge@cs.fau.de>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> arch/powerpc/platforms/40x/Kconfig | 16 -------------
> drivers/mtd/maps/Kconfig | 2 +-
> drivers/mtd/maps/redwood.c | 43 ------------------------------------
> drivers/net/Kconfig | 2 +-
> drivers/net/smc91x.h | 37 -------------------------------
> 5 files changed, 2 insertions(+), 98 deletions(-)
>
> diff --git a/arch/powerpc/platforms/40x/Kconfig b/arch/powerpc/platforms/40x/Kconfig
> index ec64264..b721764 100644
> --- a/arch/powerpc/platforms/40x/Kconfig
> +++ b/arch/powerpc/platforms/40x/Kconfig
> @@ -71,22 +71,6 @@ config MAKALU
> help
> This option enables support for the AMCC PPC405EX board.
>
> -#config REDWOOD_5
> -# bool "Redwood-5"
> -# depends on 40x
> -# default n
> -# select STB03xxx
> -# help
> -# This option enables support for the IBM STB04 evaluation board.
> -
> -#config REDWOOD_6
> -# bool "Redwood-6"
> -# depends on 40x
> -# default n
> -# select STB03xxx
> -# help
> -# This option enables support for the IBM STBx25xx evaluation board.
> -
> #config SYCAMORE
> # bool "Sycamore"
> # depends on 40x
> diff --git a/drivers/mtd/maps/Kconfig b/drivers/mtd/maps/Kconfig
> index f22bc9f..6629d09 100644
> --- a/drivers/mtd/maps/Kconfig
> +++ b/drivers/mtd/maps/Kconfig
> @@ -321,7 +321,7 @@ config MTD_CFI_FLAGADM
>
> config MTD_REDWOOD
> tristate "CFI Flash devices mapped on IBM Redwood"
> - depends on MTD_CFI && ( REDWOOD_4 || REDWOOD_5 || REDWOOD_6 )
> + depends on MTD_CFI
> help
> This enables access routines for the flash chips on the IBM
> Redwood board. If you have one of these boards and would like to
> diff --git a/drivers/mtd/maps/redwood.c b/drivers/mtd/maps/redwood.c
> index 933c0b6..d2c9db0 100644
> --- a/drivers/mtd/maps/redwood.c
> +++ b/drivers/mtd/maps/redwood.c
> @@ -22,8 +22,6 @@
>
> #include <asm/io.h>
>
> -#if !defined (CONFIG_REDWOOD_6)
> -
> #define WINDOW_ADDR 0xffc00000
> #define WINDOW_SIZE 0x00400000
>
> @@ -69,47 +67,6 @@ static struct mtd_partition redwood_flash_partitions[] = {
> }
> };
>
> -#else /* CONFIG_REDWOOD_6 */
> -/* FIXME: the window is bigger - armin */
> -#define WINDOW_ADDR 0xff800000
> -#define WINDOW_SIZE 0x00800000
> -
> -#define RW_PART0_OF 0
> -#define RW_PART0_SZ 0x400000 /* 4 MiB data */
> -#define RW_PART1_OF RW_PART0_OF + RW_PART0_SZ
> -#define RW_PART1_SZ 0x10000 /* 64K VPD */
> -#define RW_PART2_OF RW_PART1_OF + RW_PART1_SZ
> -#define RW_PART2_SZ 0x400000 - (0x10000 + 0x20000)
> -#define RW_PART3_OF RW_PART2_OF + RW_PART2_SZ
> -#define RW_PART3_SZ 0x20000
> -
> -static struct mtd_partition redwood_flash_partitions[] = {
> - {
> - .name = "Redwood filesystem",
> - .offset = RW_PART0_OF,
> - .size = RW_PART0_SZ
> - },
> - {
> - .name = "Redwood OpenBIOS Vital Product Data",
> - .offset = RW_PART1_OF,
> - .size = RW_PART1_SZ,
> - .mask_flags = MTD_WRITEABLE /* force read-only */
> - },
> - {
> - .name = "Redwood kernel",
> - .offset = RW_PART2_OF,
> - .size = RW_PART2_SZ
> - },
> - {
> - .name = "Redwood OpenBIOS",
> - .offset = RW_PART3_OF,
> - .size = RW_PART3_SZ,
> - .mask_flags = MTD_WRITEABLE /* force read-only */
> - }
> -};
> -
> -#endif /* CONFIG_REDWOOD_6 */
> -
> struct map_info redwood_flash_map = {
> .name = "IBM Redwood",
> .size = WINDOW_SIZE,
> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index ce2fcdd..313d306 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -913,7 +913,7 @@ config SMC91X
> tristate "SMC 91C9x/91C1xxx support"
> select CRC32
> select MII
> - depends on ARM || REDWOOD_5 || REDWOOD_6 || M32R || SUPERH || \
> + depends on ARM || M32R || SUPERH || \
> MIPS || BLACKFIN || MN10300 || COLDFIRE
> help
> This is a driver for SMC's 91x series of Ethernet chipsets,
> diff --git a/drivers/net/smc91x.h b/drivers/net/smc91x.h
> index 8d2772c..ee74791 100644
> --- a/drivers/net/smc91x.h
> +++ b/drivers/net/smc91x.h
> @@ -83,43 +83,6 @@ static inline void SMC_outw(u16 val, void __iomem *ioaddr, int reg)
> }
> }
>
> -#elif defined(CONFIG_REDWOOD_5) || defined(CONFIG_REDWOOD_6)
> -
> -/* We can only do 16-bit reads and writes in the static memory space. */
> -#define SMC_CAN_USE_8BIT 0
> -#define SMC_CAN_USE_16BIT 1
> -#define SMC_CAN_USE_32BIT 0
> -#define SMC_NOWAIT 1
> -
> -#define SMC_IO_SHIFT 0
> -
> -#define SMC_inw(a, r) in_be16((volatile u16 *)((a) + (r)))
> -#define SMC_outw(v, a, r) out_be16((volatile u16 *)((a) + (r)), v)
> -#define SMC_insw(a, r, p, l) \
> - do { \
> - unsigned long __port = (a) + (r); \
> - u16 *__p = (u16 *)(p); \
> - int __l = (l); \
> - insw(__port, __p, __l); \
> - while (__l > 0) { \
> - *__p = swab16(*__p); \
> - __p++; \
> - __l--; \
> - } \
> - } while (0)
> -#define SMC_outsw(a, r, p, l) \
> - do { \
> - unsigned long __port = (a) + (r); \
> - u16 *__p = (u16 *)(p); \
> - int __l = (l); \
> - while (__l > 0) { \
> - /* Believe it or not, the swab isn't needed. */ \
> - outw( /* swab16 */ (*__p++), __port); \
> - __l--; \
> - } \
> - } while (0)
> -#define SMC_IRQ_FLAGS (0)
> -
> #elif defined(CONFIG_SA1100_PLEB)
> /* We can only do 16-bit reads and writes in the static memory space. */
> #define SMC_CAN_USE_8BIT 1
^ permalink raw reply
* Re: [PATCH V4] powerpc/prom: Export device tree physical address via proc
From: Benjamin Herrenschmidt @ 2010-07-19 0:09 UTC (permalink / raw)
To: Grant Likely; +Cc: Matthew McClintock, Kumar Gala, linuxppc-dev, Timur Tabi
In-Reply-To: <AANLkTikIGXBNU7NQEL7riZYB-BJU3jhxauzGo6XhUF3t@mail.gmail.com>
On Thu, 2010-07-15 at 10:22 -0600, Grant Likely wrote:
> What is your starting point? Where does the device tree (and
> memreserve list) come from
> that you're passing to kexec? My first impression is that if you have
> to scrub the memreserve list, then the source being used to
> obtain the memreserves is either faulty or unsuitable to the task.
The kernel should ultimately pass the thing to userspace I reckon, with
an appropriate hook for platform code to insert/recover reserved
regions.
Ben.
^ permalink raw reply
* Re: linux-next: build failure after merge of the final tree (powerpc related)
From: Benjamin Herrenschmidt @ 2010-07-19 0:15 UTC (permalink / raw)
To: Stephen Rothwell
Cc: Martyn Welch, linuxppc-dev, linux-next, Paul Mackerras,
linux-kernel
In-Reply-To: <20100716171920.51ce1b88.sfr@canb.auug.org.au>
On Fri, 2010-07-16 at 17:19 +1000, Stephen Rothwell wrote:
> Hi all,
>
> After merging the final tree, today's linux-next build (powerpc
> allmodconfig) failed like this:
>
> ERROR: "of_i8042_kbd_irq" [drivers/input/serio/i8042.ko] undefined!
> ERROR: "of_i8042_aux_irq" [drivers/input/serio/i8042.ko] undefined!
>
> Presumably missing EXPORT_SYMBOLs ..
Thanks. I'll fix that up.
Cheers,
Ben.
^ permalink raw reply
* Re: [PPC64/Power7 - 2.6.35-rc5] Bad relocation warnings while Building a CONFIG_RELOCATABLE kernel with CONFIG_ISERIES enabled
From: Benjamin Herrenschmidt @ 2010-07-19 1:11 UTC (permalink / raw)
To: subrata
Cc: sachinp, Alexander Graf, linux-kernel, Kamalesh Babulal,
Linuxppc-dev, Paul Mackerras, Paul Mackerras, divya.vikas
In-Reply-To: <1279193743.10707.5.camel@subratamodak.linux.ibm.com>
On Thu, 2010-07-15 at 17:05 +0530, Subrata Modak wrote:
> commit e62cee42e66dcca83aae02748535f62e0f564a0c solved the problem for
> 2.6.34-rc6. However some other bad relocation warnings generated against
> 2.6.35-rc5 on Power7/ppc64 below:
>
> MODPOST 2004 modules^M
> WARNING: 2 bad relocations^M
> c000000000008590 R_PPC64_ADDR32 .text+0x4000000000008460^M
> c000000000008594 R_PPC64_ADDR32 .text+0x4000000000008598^M
I think this is KVM + CONFIG_RELOCATABLE. Caused by:
.global kvmppc_trampoline_lowmem
kvmppc_trampoline_lowmem:
.long kvmppc_handler_lowmem_trampoline - CONFIG_KERNEL_START
.global kvmppc_trampoline_enter
kvmppc_trampoline_enter:
.long kvmppc_handler_trampoline_enter - CONFIG_KERNEL_START
Alex, can you turn these into 64-bit on ppc64 so the relocator
can grok them ?
Cheers,
Ben.
> Config file attached.
>
> Regards--
> Subrata
>
> On Fri, 2010-05-07 at 15:40 +1000, Paul Mackerras wrote:
> > On Wed, May 05, 2010 at 05:20:51PM +0530, Subrata Modak wrote:
> >
> > > I built 2.6.34-rc6 with the attached Fedora Config file
> > > (config-2.6.33.1-19.fc13.ppc64) on my P5 Fedora Box and got the
> > > following warning. Is the following expected ?
> > >
> > > CALL arch/powerpc/relocs_check.pl
> > > Building modules, stage 2.
> > > WARNING: 4 bad relocations
> > > c00000000007216e R_PPC64_ADDR16_HIGHEST __ksymtab+0x00000000009dcec8
> > > c000000000072172 R_PPC64_ADDR16_HIGHER __ksymtab+0x00000000009dcec8
> > > c00000000007217a R_PPC64_ADDR16_HI __ksymtab+0x00000000009dcec8
> > > c00000000007217e R_PPC64_ADDR16_LO __ksymtab+0x00000000009dcec8
> >
> > No, it's not expected. It's in iSeries code, so you could avoid it
> > just by disabling CONFIG_ISERIES (I don't think any distro still
> > supports legacy iSeries). I'll post a patch to fix the problem
> > properly.
> >
> > Paul.
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* Re: Re: hi, i have two flashs, but my kernel can only find one , how cani write the dts?
From: hacklu @ 2010-07-19 3:56 UTC (permalink / raw)
To: Grant Likely; +Cc: linuxppc-dev
In-Reply-To: <AANLkTimXGTg41XHChzcnn75Px_pitC8sPKOY-OlPNIe1@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 2122 bytes --]
thanks very much! it works well now~~
but i found in my system,if i add the 0x prefix it will cause a syntax error
2010-07-19
hacklu
发件人： Grant Likely
发送时间： 2010-07-17 05:46:59
收件人： hacklu
抄送： linuxppc-dev
主题： Re: hi, i have two flashs, but my kernel can only find one , how cani write the dts?
On Fri, Jul 16, 2010 at 2:34 AM, hacklu <embedway.test@gmail.com> wrote:
> this is my dts file:
> flash@0,0 {
> #address-cells = <1>;
> #size-cells = <1>;
> compatible = "cfi-flash";
> probe-type = "CFI";
> reg = <0 0 1000000>;
> bank-width = <2>;
> device-width = <1>;
> hrcw@0 {
> label = "hrcw";
> reg = <0 40000>;
> };
> jffs@40000 {
> label = "jffs";
> reg = <40000 200000>;
> };
> jffs2@240000 {
> label = "uimage";
> reg = <240000 d80000>;
> };
> };
> flash@1,0 {
> #address-cells = <1>;
> #size-cells = <1>;
> compatible = "cfi-flash";
> probe-type = "CFI";
> reg = <1000000 0 1000000>;
This looks wrong. If you're second flash is on chip select 1 as the
node name suggests, then this should be (first cell is CS#, second is
offset, and third is size. Alos you're missing the 0x prefix):
reg = <1 0 0x1000000>;
If your second flash is on chip select 0 with the first flash, but
offset by 0x1000000, then reg should be:
reg = <0 0x1000000 0x1000000>;
and the name should be:
flash@0,1000000 { ... };
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
[-- Attachment #1.2: Type: text/html, Size: 9255 bytes --]
[-- Attachment #2: 14.gif --]
[-- Type: image/gif, Size: 1662 bytes --]
^ 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