* Re: [alsa-devel] [PATCH 1/2] powerpc: add platform registration for ALSA SoC drivers
From: Benjamin Herrenschmidt @ 2010-04-29 0:50 UTC (permalink / raw)
To: Timur Tabi; +Cc: alsa-devel, kumar.gala, Mark Brown, linuxppc-dev, lrg
In-Reply-To: <j2qed82fe3e1004281335h4076b050jc2894d4c4d6eac65@mail.gmail.com>
On Wed, 2010-04-28 at 15:35 -0500, Timur Tabi wrote:
> Second, how about this binding for the virtual sound node? It would
> be a root-level node.
>
> sound-devices {
> sound0 {
> ssi = &ssi0;
> playback-dma = &dma00;
> capture-dma = &dma01;
> codec = &cs4270;
> }
> };
Make sure you also have a "compatible" property to uniquely identify the
design. You could use the toplevel board one but I'd rather keep a
separate one here. I've seen case where the exact same base board may
have different sound components (because they are dautherboards for
example, but there's a few other cases).
Cheers,
Ben.
^ permalink raw reply
* Re: [alsa-devel] [PATCH 1/2] powerpc: add platform registration for ALSA SoC drivers
From: Benjamin Herrenschmidt @ 2010-04-29 0:42 UTC (permalink / raw)
To: Mark Brown
Cc: alsa-devel, kumar.gala, linuxppc-dev, Timur Tabi, Liam Girdwood
In-Reply-To: <20100428130037.GF31400@opensource.wolfsonmicro.com>
On Wed, 2010-04-28 at 14:00 +0100, Mark Brown wrote:
> > The whole thing is a matter of common sense and a bit of taste :-)
>
> The impression that has been created in the past is that there are
> inflexible device tree rules which can't be varied.
I'm a bit sad this is how things have been perceived since that's
clearly not the policy I've applied to the powerpc architecture.
Or rather, there are -some- inflexible rules yes, which are to:
- Have a device-tree :-)
- Have a /compatible property at the toplevel to identify your board
- Have the /cpus nodes for representing the CPUs.
That's pretty much the only absolute requirements from a code
perspective.
Now I -do- require people to also have nodes for things like PCI host
bridge, since that allows using a ton of existing code for handling most
aspects of PCI, and I -do- complain if people just hard wire platform
devices everywhere or interrupt numbers without even trying to consider
using the device-tree appropriately.
However, I've always been against the one-bsp-fits-all approach, and
it's always been my clear policy that there should be a per-machine .c
file. I did bend when folks pushed the "simple" platform but with the
understanding that it must contain an -explicit- list of boards it
supports.
You'll also notice that all of my virtual interrupt handling stuff is
such that you -can- use it without device-tree nodes, the DT just makes
it easier. Same goes with PCI devices (only the PHB requires a DT node
at this stage) etc...
Cheers,
Ben.
^ permalink raw reply
* Re: [alsa-devel] [PATCH 1/2] powerpc: add platform registration for ALSA SoC drivers
From: Benjamin Herrenschmidt @ 2010-04-29 0:36 UTC (permalink / raw)
To: Mark Brown
Cc: alsa-devel, kumar.gala, linuxppc-dev, Timur Tabi, Liam Girdwood
In-Reply-To: <20100428120719.GE31400@opensource.wolfsonmicro.com>
On Wed, 2010-04-28 at 13:07 +0100, Mark Brown wrote:
> > The device-tree helps keep the platform .c file simple and devoid of too
> > horrible hacks, it allows to easily pass various configuration data to
> > leaf drivers such as i2c thingies, PHY devices etc... without gross
> > hooks between these and the platform, but the platform code still has
> > the upper hand for doing ad-hoc bits and pieces (or overwriting the
> > device-tree based behaviour) if necessary.
>
> Once again, if you can get the device tree guys to buy into this and
> stick with it that sounds good but my experience has been that this
> isn't where any of these discussions end up.
Well, as the person who came up with the flattened device-tree format in
the first place I suppose I qualify as a "device-tree" guy here :-)
At the moment, I'd say Grant (and to some extent Jeremy Kerr) are the
guys in charge though, but yes, I agree with you, there's a tendency to
be too over-exited and to want to do "too much" with the DT and that is
counter productive. It's a good tool but it's not going to solve world
hunger and in some places an ad-hoc bit of C code is a better option :)
Now, I don't think Grant is totally off the tracks here but I must admit
I haven't taken the time to ensure I understand perfectly everybody's
position in that debate. At least I made mine clear, hope this helps :-)
Cheers,
Ben.
^ permalink raw reply
* [PATCH 2/2] powerpc: only call start-cpu when a CPU is stopped
From: Michael Neuling @ 2010-04-28 23:39 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, ankita
Currently we always call start-cpu irrespective of if the CPU is
stopped or not. Unfortunatley on POWER7, firmware seems to not like
start-cpu being called when a cpu already been started. This was not
the case on POWER6 and earlier.
This patch checks to see if the CPU is stopped or not via an
query-cpu-stopped-state call, and only calls start-cpu on CPUs which
are stopped.
This fixes a bug with kexec on POWER7 on PHYP where only the primary
thread would make it to the second kernel.
Reported-by: Ankita Garg <ankita@linux.vnet.ibm.com>
Cc: <stable@kernel.org>
Signed-off-by: Michael Neuling <mikey@neuling.org>
---
arch/powerpc/platforms/pseries/smp.c | 6 ++++++
1 file changed, 6 insertions(+)
Index: linux-2.6-ozlabs/arch/powerpc/platforms/pseries/smp.c
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/platforms/pseries/smp.c
+++ linux-2.6-ozlabs/arch/powerpc/platforms/pseries/smp.c
@@ -103,6 +103,12 @@ static inline int __devinit smp_startup_
return 1;
pcpu = get_hard_smp_processor_id(lcpu);
+
+ /* Check to see if the CPU out of FW already for kexec */
+ if (smp_query_cpu_stopped(pcpu) == QCSS_NOT_STOPPED){
+ cpu_set(lcpu, of_spin_map);
+ return 1;
+ }
/* Fixup atomic count: it exited inside IRQ handler. */
task_thread_info(paca[lcpu].__current)->preempt_count = 0;
^ permalink raw reply
* [PATCH 1/2] powerpc: make query_cpu_stopped callable outside hotplug cpu
From: Michael Neuling @ 2010-04-28 23:39 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, ankita
This moves query_cpu_stopped() out of the hotplug cpu code and into
smp.c so it can called in other places and renames it to
smp_query_cpu_stopped().
It also cleans up the return values by adding some #defines
Cc: <stable@kernel.org>
Signed-off-by: Michael Neuling <mikey@neuling.org>
---
arch/powerpc/platforms/pseries/hotplug-cpu.c | 30 +++---------------------
arch/powerpc/platforms/pseries/plpar_wrappers.h | 8 ++++++
arch/powerpc/platforms/pseries/smp.c | 22 +++++++++++++++++
3 files changed, 34 insertions(+), 26 deletions(-)
Index: linux-2.6-ozlabs/arch/powerpc/platforms/pseries/hotplug-cpu.c
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ linux-2.6-ozlabs/arch/powerpc/platforms/pseries/hotplug-cpu.c
@@ -154,30 +154,6 @@ static void pseries_mach_cpu_die(void)
for(;;);
}
-static int qcss_tok; /* query-cpu-stopped-state token */
-
-/* Get state of physical CPU.
- * Return codes:
- * 0 - The processor is in the RTAS stopped state
- * 1 - stop-self is in progress
- * 2 - The processor is not in the RTAS stopped state
- * -1 - Hardware Error
- * -2 - Hardware Busy, Try again later.
- */
-static int query_cpu_stopped(unsigned int pcpu)
-{
- int cpu_status, status;
-
- status = rtas_call(qcss_tok, 1, 2, &cpu_status, pcpu);
- if (status != 0) {
- printk(KERN_ERR
- "RTAS query-cpu-stopped-state failed: %i\n", status);
- return status;
- }
-
- return cpu_status;
-}
-
static int pseries_cpu_disable(void)
{
int cpu = smp_processor_id();
@@ -224,8 +200,9 @@ static void pseries_cpu_die(unsigned int
} else if (get_preferred_offline_state(cpu) == CPU_STATE_OFFLINE) {
for (tries = 0; tries < 25; tries++) {
- cpu_status = query_cpu_stopped(pcpu);
- if (cpu_status == 0 || cpu_status == -1)
+ cpu_status = smp_query_cpu_stopped(pcpu);
+ if (cpu_status == QCSS_STOPPED ||
+ cpu_status == QCSS_HARDWARE_ERROR)
break;
cpu_relax();
}
@@ -388,6 +365,7 @@ static int __init pseries_cpu_hotplug_in
struct device_node *np;
const char *typep;
int cpu;
+ int qcss_tok;
for_each_node_by_name(np, "interrupt-controller") {
typep = of_get_property(np, "compatible", NULL);
Index: linux-2.6-ozlabs/arch/powerpc/platforms/pseries/plpar_wrappers.h
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/platforms/pseries/plpar_wrappers.h
+++ linux-2.6-ozlabs/arch/powerpc/platforms/pseries/plpar_wrappers.h
@@ -4,6 +4,14 @@
#include <asm/hvcall.h>
#include <asm/page.h>
+/* Get state of physical CPU from query_cpu_stopped */
+int smp_query_cpu_stopped(unsigned int pcpu);
+#define QCSS_STOPPED 0
+#define QCSS_STOPPING 1
+#define QCSS_NOT_STOPPED 2
+#define QCSS_HARDWARE_ERROR -1
+#define QCSS_HARDWARE_BUSY -2
+
static inline long poll_pending(void)
{
return plpar_hcall_norets(H_POLL_PENDING);
Index: linux-2.6-ozlabs/arch/powerpc/platforms/pseries/smp.c
===================================================================
--- linux-2.6-ozlabs.orig/arch/powerpc/platforms/pseries/smp.c
+++ linux-2.6-ozlabs/arch/powerpc/platforms/pseries/smp.c
@@ -57,6 +57,28 @@
*/
static cpumask_t of_spin_map;
+/* Query where a cpu is now. Return codes #defined in plpar_wrappers.h */
+int smp_query_cpu_stopped(unsigned int pcpu)
+{
+ int cpu_status, status;
+ int qcss_tok = rtas_token("query-cpu-stopped-state");
+
+ if (qcss_tok == RTAS_UNKNOWN_SERVICE) {
+ printk(KERN_INFO "Firmware doesn't support "
+ "query-cpu-stopped-state\n");
+ return QCSS_HARDWARE_ERROR;
+ }
+
+ status = rtas_call(qcss_tok, 1, 2, &cpu_status, pcpu);
+ if (status != 0) {
+ printk(KERN_ERR
+ "RTAS query-cpu-stopped-state failed: %i\n", status);
+ return status;
+ }
+
+ return cpu_status;
+}
+
/**
* smp_startup_cpu() - start the given cpu
*
^ permalink raw reply
* [PATCH] mpc5121_nfc: Changed SVR check to allow MPC5123.
From: Steve Deiters @ 2010-04-28 22:33 UTC (permalink / raw)
To: linux-mtd, linuxppc-dev
The revision in SVR for MPC5123 is 3. The NFC is the same as MPC5121
revision 2.
---
drivers/mtd/nand/mpc5121_nfc.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/nand/mpc5121_nfc.c
b/drivers/mtd/nand/mpc5121_nfc.c
index 191bf99..dc04d74 100644
--- a/drivers/mtd/nand/mpc5121_nfc.c
+++ b/drivers/mtd/nand/mpc5121_nfc.c
@@ -666,10 +666,10 @@ static int __devinit mpc5121_nfc_probe(struct
of_device *op,
=20
/*
* Check SoC revision. This driver supports only NFC
- * in MPC5121 revision 2.
+ * in MPC5121 revision 2 and MPC5123 revision 3.
*/
rev =3D (mfspr(SPRN_SVR) >> 4) & 0xF;
- if (rev !=3D 2) {
+ if ((rev !=3D 2) && (rev !=3D 3)) {
dev_err(dev, "SoC revision %u is not supported!\n",
rev);
return -ENXIO;
}
--=20
1.5.4.3
^ permalink raw reply related
* Re: [PATCH] Fix DEBUG_PAGEALLOC on 603/e300
From: Benjamin Herrenschmidt @ 2010-04-28 22:29 UTC (permalink / raw)
To: Xianghua Xiao; +Cc: linuxppc-dev
In-Reply-To: <y2te6bf505b1004281215n9dcc6190j9915aa27ae91f75d@mail.gmail.com>
On Wed, 2010-04-28 at 14:15 -0500, Xianghua Xiao wrote:
> This change works me on a 834x(e300) platform, tested with lmbench and
> a production-ready application with 2.6.33.3.
But have you tested that DEBUG_PAGEALLOC actually works ? :-)
A way to do that is to
- get_free_pages a page
- read from it
- free it
- write to it
It should oops on the write, and I suspect that without my patch it
doesn't.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 1/2] genirq: reliably replay pending edge-triggered irq
From: Thomas Gleixner @ 2010-04-28 22:28 UTC (permalink / raw)
To: Guillaume Knispel
Cc: Lars-Peter Clausen, Russell King, Haavard Skinnemoen,
Bartlomiej Zolnierkiewicz, Peter Zijlstra, linux-kernel,
Linus Torvalds, Michael Buesch, Ingo Molnar, Linuxppc-dev
In-Reply-To: <20100428235657.7120edd9.gknispel@proformatique.com>
On Wed, 28 Apr 2010, Guillaume Knispel wrote:
> On Tue, 27 Apr 2010 15:42:11 +0200 (CEST)
> Thomas Gleixner <tglx@linutronix.de> wrote:
>
> > On Thu, 22 Apr 2010, Guillaume Knispel wrote:
> [snip]
> > > acked and masked at controller level and IRQ_PENDING is set.
> > > ---
> > > arch/arm/Kconfig | 4 ----
> > > arch/arm/configs/acs5k_defconfig | 1 -
> > > arch/arm/configs/acs5k_tiny_defconfig | 1 -
> >
> > Changing the defconfigs should be a separate patch and I'm not
> > going to push such a thing through the genirq tree. The stale
> > entries do not hurt.
>
> Ok
> In this separate patch, to I put "genirq" in the title?
> Do I put it in the same series?
Just keep them out for now and send them to the arch maintainers once
the change hits linus tree.
Thanks,
tglx
^ permalink raw reply
* Re: [alsa-devel] [PATCH 1/2] powerpc: add platform registration for ALSA SoC drivers
From: Grant Likely @ 2010-04-28 22:23 UTC (permalink / raw)
To: Timur Tabi
Cc: alsa-devel, kumar.gala, Mark Brown, linuxppc-dev,
devicetree-discuss, lrg
In-Reply-To: <r2oed82fe3e1004281513k23b54b56v7904a4a34750c90b@mail.gmail.com>
On Wed, Apr 28, 2010 at 4:13 PM, Timur Tabi <timur.tabi@gmail.com> wrote:
> On Wed, Apr 28, 2010 at 4:58 PM, Grant Likely <grant.likely@secretlab.ca>=
wrote:
>
>> The sound0 node needs a compatible value,
>
> I knew I was forgetting something
:-)
>
>> the sound-device node should
>> probably have one too.
>
> The aliases, cpus, and memory node don't have a compatible property,
> and I was modeling the design after the aliases node.
Well, there are typically three ways to find a node; by name, by
device_type and by compatible. device_type is meaningless for the
flattened tree, so that's out. Matching by name could potentially
have namespace collisions, but I'm not sure. I'll defer to Ben &
Mitch's judgment here.
The difference with aliases, cpus and memory nodes is that the
conventions around them were defined and agreed on a very long time
ago. We could get consensus to do the same here, but I cannot make
that call.
>> The sound0 node should have something board specific like
>> "fsl,mpc8610hpcd-sound" to make it clear that the binding really only
>> applies to this particular board. =A0It would also be a good idea to
>> prefix all of the property names with 'fsl,' to avoid conflicting with
>> any future common bindings or conventions. =A0Other boards can use the
>> same binding, but they would get a different compatible value (the
>> driver could bind on both).
>
> The aliases node doesn't have an fsl, prefix. =A0I understand the need
> for the prefix, but I wonder why we don't do that for the aliases
> node.
aliases is not a vendor-specific or limited scope convention.
g.
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [alsa-devel] [PATCH 1/2] powerpc: add platform registration for ALSA SoC drivers
From: Timur Tabi @ 2010-04-28 22:13 UTC (permalink / raw)
To: Grant Likely
Cc: alsa-devel, kumar.gala, Mark Brown, linuxppc-dev,
devicetree-discuss, lrg
In-Reply-To: <z2xfa686aa41004281458u3a3048feh9d762e07f8052348@mail.gmail.com>
On Wed, Apr 28, 2010 at 4:58 PM, Grant Likely <grant.likely@secretlab.ca> w=
rote:
> The sound0 node needs a compatible value,
I knew I was forgetting something
> the sound-device node should
> probably have one too.
The aliases, cpus, and memory node don't have a compatible property,
and I was modeling the design after the aliases node.
> The sound0 node should have something board specific like
> "fsl,mpc8610hpcd-sound" to make it clear that the binding really only
> applies to this particular board. =A0It would also be a good idea to
> prefix all of the property names with 'fsl,' to avoid conflicting with
> any future common bindings or conventions. =A0Other boards can use the
> same binding, but they would get a different compatible value (the
> driver could bind on both).
The aliases node doesn't have an fsl, prefix. I understand the need
for the prefix, but I wonder why we don't do that for the aliases
node.
> I'm not a huge fan of the name "sound-devices" for the parent node.
> There are other sorts of things that we need 'virtual' device nodes to
> describe. =A0It would be nice to have a single place for collecting
> nodes for stuff like this. =A0Perhaps this:
>
> system {
> =A0 =A0 =A0 =A0compatible =3D "system-devices";
> =A0 =A0 =A0 =A0sound0 {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0compatible =3D "fsl,mpc8610hpcd-sound";
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0fsl,ssi =3D &ssi0;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0fsl,playback-dma =3D &dma00;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0fsl,capture-dma =3D &dma01;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0fsl,codec =3D &cs4270;
> =A0 =A0 =A0 =A0};
> };
I like that.
--=20
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: [microblaze-uclinux] [PATCHv2] [RFC] Xilinx MPMC SDMA subsystem
From: Sergey Temerkhanov @ 2010-04-28 21:58 UTC (permalink / raw)
To: Grant Likely
Cc: microblaze-uclinux, linuxppc-dev, Linux Kernel Mailing List,
steve
In-Reply-To: <n2vfa686aa41004272213wceeef998tadc483ec1de7b792@mail.gmail.com>
On Wednesday 28 April 2010 09:13:06 you wrote:
> Hi Sergey and Steven,
>
> On Tue, Apr 27, 2010 at 8:29 PM, Steven J. Magnani
>
> <steve@digidescorp.com> wrote:
> > On Wed, 2010-04-28 at 02:06 +0400, Sergey Temerkhanov wrote:
> >> This is the 2nd version of Xilinx MPMC LocalLink SDMA subsystem
> >>
> >> Changelog v2:
> >> * Changed the functions and struct definition prefix from sdma_ to
> >> xllsdma_ * Platform bus bindings and various changes by Steven J.
> >> Magnani. * Moved source files from arch/powerpc/sysdev to global
> >> locations and added CONFIG_XLLSDMA option.
> >>
> >> Regards, Sergey Temerkhanov,
> >> Cifronic ZAO
> >>
> >> diff -r baced9e29ab5 drivers/dma/Kconfig
> >> --- a/drivers/dma/Kconfig Tue Apr 27 20:48:50 2010 +0400
> >> +++ b/drivers/dma/Kconfig Wed Apr 28 02:00:51 2010 +0400
> >> @@ -97,6 +97,14 @@
> >> Support the TXx9 SoC internal DMA controller. This can be
> >> integrated in chips such as the Toshiba TX4927/38/39.
> >>
> >> +config XLLSDMA
>
> I'd prefer XILINX_LLSDMA. XLLSDMA could be ambiguous in the global
> namespace (for a human reader), particularly as it is something that
> few people will actually see.
I've changed it to XILINX_SDMA in the current version.
>
> >> + bool "Xilinx MPMC DMA support"
> >> + depends on XILINX_VIRTEX || MICROBLAZE
> >> + select DMA_ENGINE
> >> + help
> >> + Support fot Xilinx MPMC LocalLink SDMA. Virtex FPGA family
> >> + has it integrated or fabric-based.
> >> +
> >
> > fot --> for
> >
> > Since the xllsdma driver provides services to other drivers - not to
> > userland - I think this would be better as a "silent" option, selected
> > automatically when something like ll_temac or the forthcoming Xilinx DMA
> > engine is selected. If we do it that way, note that XLLSDMA is
> > independent of DMA_ENGINE so drivers/Makefile will need to be patched so
> > that the dma subdirectory is always "y".
>
> Agreed. However, looking at this code, I don't see anything that
> actually uses DMA_ENGINE here. Am I missing something?
It's because the appropriate line in drivers/Makefile is
obj-$(CONFIG_DMA_ENGINE) += dma/
instead of
obj-$(CONFIG_DMADEVICES) += dma/
>
> >> diff -r baced9e29ab5 drivers/dma/Makefile
> >> --- a/drivers/dma/Makefile Tue Apr 27 20:48:50 2010 +0400
> >> +++ b/drivers/dma/Makefile Wed Apr 28 02:00:51 2010 +0400
> >> @@ -10,3 +10,4 @@
> >> obj-$(CONFIG_AT_HDMAC) += at_hdmac.o
> >> obj-$(CONFIG_MX3_IPU) += ipu/
> >> obj-$(CONFIG_TXX9_DMAC) += txx9dmac.o
> >> +obj-$(CONFIG_XLLSDMA) += xllsdma.o
> >> diff -r baced9e29ab5 drivers/dma/xllsdma.c
> >> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> >> +++ b/drivers/dma/xllsdma.c Wed Apr 28 02:00:51 2010 +0400
> >> @@ -0,0 +1,887 @@
> >> +/*
> >> + * SDMA subsystem support for Xilinx MPMC.
> >> + *
> >> + * Author: Sergey Temerkhanov
> >> + * Platform Bus by Steven J. Magnani
> >> + *
> >> + * Copyright (c) 2008-2010 Cifronic ZAO
> >> + *
> >> + * This program is free software; you can redistribute it and/or
> >> modify it + * under the terms of the GNU General Public License as
> >> published by the + * Free Software Foundation; either version 2 of the
> >> License, or (at your + * option) any later version.
> >> + *
> >> + */
> >> +
> >> +#include <linux/delay.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/init.h>
> >> +#include <linux/module.h>
> >> +#include <linux/init.h>
> >> +#include <linux/errno.h>
> >> +#include <linux/interrupt.h>
> >> +#include <linux/mutex.h>
> >> +#include <linux/wait.h>
> >> +#include <linux/list.h>
> >> +#include <linux/io.h>
> >> +#include <linux/xllsdma.h>
> >> +
> >> +#include <linux/of_device.h>
> >> +#include <linux/of_platform.h>
> >> +
> >> +#define DRV_VERSION "0.1.0"
>
> Irrelevant, can be dropped
>
> >> +#define DRV_NAME "sdma"
>
> Used only once, drop.
>
> >> +
> >> +MODULE_AUTHOR("Sergey Temerkhanov <temerkhanov@cifronik.ru>");
> >> +MODULE_DESCRIPTION("Xilinx SDMA driver");
> >> +MODULE_LICENSE("GPL");
> >> +MODULE_VERSION(DRV_VERSION);
> >> +
> >> +LIST_HEAD(mpmc_devs);
> >> +DEFINE_MUTEX(mpmc_devs_lock);
> >> +
> >> +void xllsdma_tx_irq_enable(struct xllsdma_device *sdma)
> >> +{
> >> + u32 tx_cr;
> >> + unsigned long flags;
> >> +
> >> + BUG_ON(sdma->tx_irq == NO_IRQ);
> >> +
> >> + spin_lock_irqsave(&sdma->lock, flags);
> >> + tx_cr = xllsdma_tx_in32(sdma, XLLSDMA_CR);
> >> + xllsdma_tx_out32(sdma, XLLSDMA_CR, tx_cr | XLLSDMA_CR_IRQ_EN);
> >> + spin_unlock_irqrestore(&sdma->lock, flags);
>
> This pattern is used a lot. Might be worth while to implement
> xllsdma_tx_* variants of setbits32, clrbits32 and clrsetbits32.
>
> Also, there are a lot of these little functions; really trivial and
> small. Would it be better to have them as static inlines in the
> header file instead of exported globals?
Well, I can do this but it will require moving of register definitions to
xllsdma.h
>
> >> +}
> >> +EXPORT_SYMBOL_GPL(xllsdma_tx_irq_enable);
> >> +
> >> +void xllsdma_rx_irq_enable(struct xllsdma_device *sdma)
> >> +{
> >> + u32 rx_cr;
> >> + unsigned long flags;
> >> +
> >> + BUG_ON(sdma->rx_irq == NO_IRQ);
> >> +
> >> + spin_lock_irqsave(&sdma->lock, flags);
> >> + rx_cr = xllsdma_rx_in32(sdma, XLLSDMA_CR);
> >> + xllsdma_rx_out32(sdma, XLLSDMA_CR, rx_cr | XLLSDMA_CR_IRQ_EN);
> >> + spin_unlock_irqrestore(&sdma->lock, flags);
> >> +}
> >> +EXPORT_SYMBOL_GPL(xllsdma_rx_irq_enable);
> >> +
> >> +void xllsdma_tx_irq_disable(struct xllsdma_device *sdma)
> >> +{
> >> + u32 tx_cr;
> >> + unsigned long flags;
> >> +
> >> + spin_lock_irqsave(&sdma->lock, flags);
> >> + tx_cr = xllsdma_tx_in32(sdma, XLLSDMA_CR);
> >> + xllsdma_tx_out32(sdma, XLLSDMA_CR, tx_cr & ~XLLSDMA_CR_IRQ_EN);
> >> + spin_unlock_irqrestore(&sdma->lock, flags);
> >> +}
> >> +EXPORT_SYMBOL_GPL(xllsdma_tx_irq_disable);
> >> +
> >> +void xllsdma_rx_irq_disable(struct xllsdma_device *sdma)
> >> +{
> >> + u32 rx_cr;
> >> + unsigned long flags;
> >> +
> >> + spin_lock_irqsave(&sdma->lock, flags);
> >> + rx_cr = xllsdma_rx_in32(sdma, XLLSDMA_CR);
> >> + xllsdma_rx_out32(sdma, XLLSDMA_CR, rx_cr & ~XLLSDMA_CR_IRQ_EN);
> >> + spin_unlock_irqrestore(&sdma->lock, flags);
> >> +}
> >> +EXPORT_SYMBOL_GPL(xllsdma_rx_irq_disable);
> >> +
> >> +void xllsdma_tx_irq_ack(struct xllsdma_device *sdma)
> >> +{
> >> + u32 irq_stat;
> >> + unsigned long flags;
> >> +
> >> + spin_lock_irqsave(&sdma->lock, flags);
> >> + irq_stat = xllsdma_tx_in32(sdma, XLLSDMA_IRQ);
> >> + xllsdma_tx_out32(sdma, XLLSDMA_IRQ, irq_stat &
> >> XLLSDMA_IRQ_ALL_DONE); + spin_unlock_irqrestore(&sdma->lock, flags);
> >> +}
> >> +EXPORT_SYMBOL_GPL(xllsdma_tx_irq_ack);
> >> +
> >> +void xllsdma_rx_irq_ack(struct xllsdma_device *sdma)
> >> +{
> >> + u32 irq_stat;
> >> + unsigned long flags;
> >> +
> >> + spin_lock_irqsave(&sdma->lock, flags);
> >> + irq_stat = xllsdma_rx_in32(sdma, XLLSDMA_IRQ);
> >> + xllsdma_rx_out32(sdma, XLLSDMA_IRQ, irq_stat &
> >> XLLSDMA_IRQ_ALL_DONE); + spin_unlock_irqrestore(&sdma->lock, flags);
> >> +}
> >> +EXPORT_SYMBOL_GPL(xllsdma_rx_irq_ack);
> >> +
> >> +void xllsdma_pause(struct xllsdma_device *sdma)
> >> +{
> >> + u32 dmacr;
> >> + unsigned long flags;
> >> +
> >> + spin_lock_irqsave(&sdma->lock, flags);
> >> + dmacr = xllsdma_read_cr(sdma);
> >> + dmacr |= XLLSDMA_DMACR_TX_PAUSE | XLLSDMA_DMACR_RX_PAUSE;
> >> + xllsdma_write_cr(sdma, dmacr);
> >> + spin_unlock_irqrestore(&sdma->lock, flags);
> >> +}
> >> +EXPORT_SYMBOL_GPL(xllsdma_pause);
> >> +
> >> +void xllsdma_resume(struct xllsdma_device *sdma)
> >> +{
> >> + u32 dmacr;
> >> + unsigned long flags;
> >> +
> >> + spin_lock_irqsave(&sdma->lock, flags);
> >> + dmacr = xllsdma_read_cr(sdma);
> >> + dmacr &= ~(XLLSDMA_DMACR_TX_PAUSE | XLLSDMA_DMACR_RX_PAUSE);
> >> + xllsdma_write_cr(sdma, dmacr);
> >> + spin_unlock_irqrestore(&sdma->lock, flags);
> >> +}
> >> +EXPORT_SYMBOL_GPL(xllsdma_resume);
> >> +
> >> +int xllsdma_set_coalesce(struct xllsdma_device *sdma, struct
> >> xllsdma_coalesce *coal) +{
> >> + u32 tx_cr, rx_cr;
> >> + unsigned long flags;
> >> +
> >> + if (coal->tx_timeout > 255 ||
> >> + coal->rx_timeout > 255 ||
> >> + coal->tx_threshold > 255 ||
> >> + coal->rx_threshold > 255)
> >> + return -EINVAL;
> >> +
> >> + spin_lock_irqsave(&sdma->lock, flags);
> >> +
> >> + if (sdma->rx_irq != NO_IRQ) {
> >> + rx_cr = xllsdma_rx_in32(sdma, XLLSDMA_CR);
> >> +
> >> + if (coal->rx_timeout == 0) {
> >> + coal->rx_timeout = 1;
> >> + rx_cr &= ~XLLSDMA_CR_IRQ_TIMEOUT;
> >> + } else {
> >> + rx_cr |= XLLSDMA_CR_IRQ_TIMEOUT;
> >> + }
> >> +
> >> + rx_cr &= ~(XLLSDMA_CR_IRQ_THRESHOLD_MSK |
> >> XLLSDMA_CR_IRQ_TIMEOUT_SH); + rx_cr |= (coal->rx_threshold
> >> << XLLSDMA_CR_IRQ_THRESHOLD_SH) + &
> >> XLLSDMA_CR_IRQ_THRESHOLD_MSK;
> >> + rx_cr |= (coal->rx_timeout << XLLSDMA_CR_IRQ_TIMEOUT_SH)
> >> + & XLLSDMA_CR_IRQ_TIMEOUT_MSK;
> >> + rx_cr |= XLLSDMA_CR_LD_IRQ_CNT;
> >> +
> >> + xllsdma_rx_out32(sdma, XLLSDMA_CR, rx_cr);
> >> + }
> >> +
> >> + if (sdma->tx_irq != NO_IRQ) {
> >> + tx_cr = xllsdma_tx_in32(sdma, XLLSDMA_CR);
> >> +
> >> + if (coal->tx_timeout == 0) {
> >> + coal->tx_timeout = 1;
> >> + tx_cr &= ~XLLSDMA_CR_IRQ_TIMEOUT;
> >> + } else {
> >> + tx_cr |= XLLSDMA_CR_IRQ_TIMEOUT;
> >> + }
> >> +
> >> + tx_cr &= ~(XLLSDMA_CR_IRQ_THRESHOLD_MSK |
> >> XLLSDMA_CR_IRQ_TIMEOUT_SH); + tx_cr |= (coal->tx_threshold
> >> << XLLSDMA_CR_IRQ_THRESHOLD_SH) + &
> >> XLLSDMA_CR_IRQ_THRESHOLD_MSK;
> >> + tx_cr |= (coal->tx_timeout << XLLSDMA_CR_IRQ_TIMEOUT_SH)
> >> + & XLLSDMA_CR_IRQ_TIMEOUT_MSK;
> >> + tx_cr |= XLLSDMA_CR_LD_IRQ_CNT;
> >> +
> >> + xllsdma_tx_out32(sdma, XLLSDMA_CR, tx_cr);
> >> + }
> >> +
> >> + spin_unlock_irqrestore(&sdma->lock, flags);
> >> +
> >> + return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(xllsdma_set_coalesce);
> >> +
> >> +int xllsdma_get_coalesce(struct xllsdma_device *sdma, struct
> >> xllsdma_coalesce *coal) +{
> >> + u32 tx_cr, rx_cr;
> >> + unsigned long flags;
> >> +
> >> + spin_lock_irqsave(&sdma->lock, flags);
> >> +
> >> + tx_cr = xllsdma_tx_in32(sdma, XLLSDMA_CR);
> >> + rx_cr = xllsdma_rx_in32(sdma, XLLSDMA_CR);
> >> +
> >> + coal->tx_threshold = (tx_cr & XLLSDMA_CR_IRQ_THRESHOLD_MSK)
> >> + >> XLLSDMA_CR_IRQ_THRESHOLD_SH;
> >> + coal->tx_timeout = (tx_cr & XLLSDMA_CR_IRQ_TIMEOUT_MSK)
> >> + >> XLLSDMA_CR_IRQ_TIMEOUT_SH;
> >> +
> >> + coal->rx_threshold = (rx_cr & XLLSDMA_CR_IRQ_THRESHOLD_MSK)
> >> + >> XLLSDMA_CR_IRQ_THRESHOLD_SH;
> >> + coal->rx_timeout = (rx_cr & XLLSDMA_CR_IRQ_TIMEOUT_MSK)
> >> + >> XLLSDMA_CR_IRQ_TIMEOUT_SH;
> >> +
> >> + if (!(tx_cr & XLLSDMA_CR_IRQ_TIMEOUT))
> >> + coal->tx_timeout = 0;
> >> +
> >> + if (!(rx_cr & XLLSDMA_CR_IRQ_TIMEOUT))
> >> + coal->rx_timeout = 0;
> >> +
> >> + spin_unlock_irqrestore(&sdma->lock, flags);
> >> +
> >> + return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(xllsdma_get_coalesce);
> >> +
> >> +int xllsdma_tx_submit(struct xllsdma_device *sdma, dma_addr_t desc)
> >> +{
> >> + xllsdma_tx_out32(sdma, XLLSDMA_TDESCR, desc);
> >> + return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(xllsdma_tx_submit);
> >
> > Invariant return value can be dropped.
>
> Also are more tiny functions that could be considered for static inlines.
>
> >> +int xllsdma_rx_submit(struct xllsdma_device *sdma, dma_addr_t desc)
> >> +{
> >> + xllsdma_rx_out32(sdma, XLLSDMA_TDESCR, desc);
> >> + return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(xllsdma_rx_submit);
> >
> > Invariant return value can be dropped.
> >
> >> +
> >> +void xllsdma_tx_init(struct xllsdma_device *sdma, dma_addr_t desc)
> >> +{
> >> + xllsdma_tx_out32(sdma, XLLSDMA_CDESCR, desc);
> >> +}
> >> +EXPORT_SYMBOL_GPL(xllsdma_tx_init);
> >> +
> >> +void xllsdma_rx_init(struct xllsdma_device *sdma, dma_addr_t desc)
> >> +{
> >> + xllsdma_rx_out32(sdma, XLLSDMA_CDESCR, desc);
> >> +}
> >> +EXPORT_SYMBOL_GPL(xllsdma_rx_init);
> >> +
> >> +struct xllsdma_device *xllsdma_find_device(int phandle)
> >> +{
> >> + struct mpmc_device *mpmc;
> >> + struct xllsdma_device *sdma = NULL;
> >> + int found = 0;
> >> + mutex_lock(&mpmc_devs_lock);
> >> + list_for_each_entry(mpmc, &mpmc_devs, item) {
> >> + mutex_lock(&mpmc->devs_lock);
> >> + list_for_each_entry(sdma, &mpmc->xllsdma_devs, item) {
> >> + if (sdma->phandle == phandle) {
> >> + found = 1;
> >> + break;
> >> + }
> >> + }
> >> + mutex_unlock(&mpmc->devs_lock);
> >> + if (found)
> >> + break;
> >> + else
> >> + sdma = NULL;
> >> + }
> >> + mutex_unlock(&mpmc_devs_lock);
>
> Why is the lock getting dropped on each iteration of the loop? It
> doesn't look necessary to me at all.
There can be more than 1 MPMC and the outer loops iterates over MPMSs and the
inner one iterates over SDMA LocalLinks belonging to the appropriate MPMC
(SDMA channels only have access to memory connected to their MPMC).
>
> >> + return sdma;
> >> +}
> >> +EXPORT_SYMBOL_GPL(xllsdma_find_device);
> >
> > I'm still concerned that there is no concept of "allocating" or
> > "reserving" a channel. This seems to invite accidental concurrent use of
> > a channel, if not in the field, then during development.
>
> In the device tree use-case I doubt it will be a practical problem.
> To get multiple users would require multiple nodes to reference the
> same dma device node.
>
> However, I've got concerns about the device model used here.... need
> to read the rest of this driver first to figure out what is bothering
> me....
>
> >> +static void xllsdma_dev_register(struct mpmc_device *mpmc,
> >> + struct xllsdma_device *sdma)
> >> +{
> >> + mutex_lock(&mpmc->devs_lock);
> >> + list_add(&sdma->item, &mpmc->xllsdma_devs);
> >> + mutex_unlock(&mpmc->devs_lock);
> >> +}
>
> This driver could use some documentation on what the data model is
> here. Is it that the system can have one or more mpmc_devices, and
> each mpmc device can contain one or more xllsdma_device?
Yes, it is.
>
> How will the ll_temac driver plug into this model?
>
Well, practically I've never seen a working configuration with more than 1
MPMC (I think, it would need some pseudo-NUMA as well but with a hard binding
of physical address ranges to SDMA devices).
However, I can drop the multiple MPMC support.
> >> +
> >> +static void xllsdma_dev_unregister(struct xllsdma_device *sdma)
> >> +{
> >> + struct mpmc_device *mpmc = sdma->parent;
> >> +
> >> + mutex_lock(&mpmc->devs_lock);
> >> + list_del(&sdma->item);
> >> + mutex_unlock(&mpmc->devs_lock);
> >> +}
> >> +
> >> +static void xllsdma_cleanup(struct device *dev)
> >> +{
> >> + struct xllsdma_device *sdma = dev_get_drvdata(dev);
> >> +
> >> + if (sdma->tx_irq)
> >> + free_irq(sdma->tx_irq, sdma);
> >> +
> >> + if (sdma->rx_irq)
> >> + free_irq(sdma->rx_irq, sdma);
> >> +
> >> + if (sdma->memregion.start)
> >> + release_mem_region(sdma->memregion.start,
> >> + sdma->memregion.end - sdma->memregion.start + 1);
> >> +
> >> + if (sdma->ioaddr)
> >> + iounmap(sdma->ioaddr);
> >> +
> >> + xllsdma_dev_unregister(sdma);
> >> + kfree(sdma);
> >> + dev_set_drvdata(dev, NULL);
> >> +}
> >> +
> >> +static void mpmc_dev_register(struct mpmc_device *mpmc)
> >> +{
> >> + mutex_lock(&mpmc_devs_lock);
> >> + list_add_tail(&mpmc->item, &mpmc_devs);
> >> + mutex_unlock(&mpmc_devs_lock);
> >> +}
> >> +
> >> +static void mpmc_dev_unregister(struct mpmc_device *mpmc)
> >> +{
> >> + mutex_lock(&mpmc_devs_lock);
> >> + list_del(&mpmc->item);
> >> + mutex_unlock(&mpmc_devs_lock);
> >> +}
> >> +
> >> +static void mpmc_cleanup(struct device *dev)
> >> +{
> >> + struct mpmc_device *mpmc = dev_get_drvdata(dev);
> >> +
> >> + if (mpmc->registered)
> >> + mpmc_dev_unregister(mpmc);
>
> Under what condition would the mpmc not be registered when this
> function is called? I don't see any code path where this would be the
> case, or any other users of the .registered data member.
It was intended for error handling. I'll look into that.
>
> >> +
> >> + kfree(mpmc);
> >> + dev_set_drvdata(dev, NULL);
> >> +}
> >> +
> >> +static int __devinit xllsdma_init(struct device *dev, struct resource
> >> *rx_irq, + struct resource *tx_irq, struct
> >> resource *mem, + int phandle)
> >> +{
> >> + struct xllsdma_device *sdma;
> >> + struct mpmc_device *mpmc;
> >> +
> >> + resource_size_t region_size;
> >> + int res;
> >> +
> >> + mpmc = dev_get_drvdata(dev->parent);
> >> +
> >> + sdma = kzalloc(sizeof(struct xllsdma_device), GFP_KERNEL);
> >> + if (!sdma) {
> >> + dev_err(dev, "Cannot allocate SDMA device\n");
> >> + return -ENOMEM;
> >> + }
> >> + dev_set_drvdata(dev, sdma);
> >> + sdma->dev = dev;
> >> +
> >> + spin_lock_init(&sdma->lock);
> >> + INIT_LIST_HEAD(&sdma->clients);
> >> + mutex_init(&sdma->clients_lock);
> >> + sdma->parent = mpmc;
> >> + sdma->phandle = phandle;
> >> +
> >> + region_size = mem->end - mem->start + 1;
> >> + if (!request_mem_region(mem->start, region_size, DRV_NAME)) {
> >> + dev_err(dev, "I/O memory region at %p is busy\n",
> >> + (void *)mem->start);
> >> + return -EBUSY;
>
> Error paths need to unwind allocations and setups. ie. freeing
> memory, releasing irqs, etc.
>
> >> + }
> >> + sdma->memregion = *mem;
> >> +
> >> + sdma->ioaddr = ioremap(mem->start, region_size);
> >> + if (!sdma->ioaddr) {
> >> + dev_err(dev, "Cannot ioremap() I/O memory %p\n",
> >> + (void *)mem->start);
> >> + return -ENOMEM;
> >> + }
> >> +
> >> + xllsdma_reset(sdma);
> >> +
> >> + sdma->rx_irq = NO_IRQ;
> >> + if (rx_irq) {
> >> + res = request_irq(rx_irq->start, xllsdma_rx_intr,
> >> + IRQF_SHARED, "SDMA RX", sdma);
> >> + if (res) {
> >> + dev_err(dev, "Could not allocate RX interrupt
> >> %d.\n", + rx_irq->start);
> >> + return res;
> >> + }
> >> + sdma->rx_irq = rx_irq->start;
> >> + }
> >> +
> >> + sdma->tx_irq = NO_IRQ;
> >> + if (tx_irq) {
> >> + res = request_irq(tx_irq->start, xllsdma_tx_intr,
> >> + IRQF_SHARED, "SDMA TX", sdma);
> >> + if (res) {
> >> + dev_err(dev, "Could not allocate TX interrupt
> >> %d.\n", + tx_irq->start);
> >> + return res;
> >> + }
> >> + sdma->tx_irq = tx_irq->start;
> >> + }
> >> +
> >> + sdma->rx_ack = 1;
> >> + sdma->tx_ack = 1;
> >> + xllsdma_dev_register(mpmc, sdma);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int __devinit mpmc_init(struct device *dev)
> >> +{
> >> + struct mpmc_device *mpmc;
> >> +
> >> + mpmc = kzalloc(sizeof(struct mpmc_device), GFP_KERNEL);
> >> +
> >> + if (!mpmc) {
> >> + dev_err(dev, "Cannot allocate MPMC device\n");
> >> + return -ENOMEM;
> >> + }
> >> +
> >> + dev_set_drvdata(dev, mpmc);
> >> +
> >> + INIT_LIST_HEAD(&mpmc->xllsdma_devs);
> >> + mutex_init(&mpmc->devs_lock);
> >> +
> >> + mpmc_dev_register(mpmc);
> >> + mpmc->registered = 1;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +#ifdef CONFIG_OF
> >> +static int xllsdma_of_remove(struct of_device *op)
>
> +static int __devexit xllsdma_of_remove(struct of_device *op)
>
> >> +{
> >> + xllsdma_cleanup(&op->dev);
> >> + return 0;
> >> +}
> >> +
> >> +/* Match table for of_platform binding */
> >> +static struct of_device_id xllsdma_of_match[] = {
> >> + { .compatible = "xlnx,ll-dma-1.00.a" },
> >> + {},
> >> +};
> >> +
> >> +static int __devinit xllsdma_of_probe(struct of_device *op,
> >> + const struct of_device_id *match)
> >> +{
> >> + const int *prop;
> >> + int phandle;
> >> + struct resource rx_irq, tx_irq, mem;
> >> + struct resource *tx_irq_res = NULL;
> >> + struct resource *rx_irq_res = NULL;
> >> + int res;
> >> +
> >> + res = of_address_to_resource(op->node, 0, &mem);
> >> + if (res) {
> >> + dev_err(&op->dev, "invalid address\n");
> >> + return res;
> >> + }
> >> +
> >> + /* IRQ */
> >> + res = of_irq_to_resource(op->node, 0, &rx_irq);
> >> + if (res != NO_IRQ)
> >> + rx_irq_res = &rx_irq;
> >> +
> >> + res = of_irq_to_resource(op->node, 1, &tx_irq);
> >> + if (res != NO_IRQ)
> >> + tx_irq_res = &tx_irq;
> >> +
> >> + prop = of_get_property(op->node, "linux,phandle", NULL);
> >> + phandle = (prop) ? *prop : -1;
>
> Don't use phandles here. Just store the node pointer directly. When
> looking up a phandle, first convert it to a node pointer, and then use
> the node pointer to search.
>
> >> +
> >> + res = xllsdma_init(&op->dev, rx_irq_res, tx_irq_res, &mem,
> >> phandle); + if (res)
> >> + xllsdma_of_remove(op);
>
> This looks odd. If init fails, there is nothing in this function that
> needs to be deallocated or unwound. You can simply do:
>
> +return xllsdma_init(&op->dev, rx_irq_res, tx_irq_res, &mem, phandle);
>
xllsdma_init() tries to allocate some resources etc. xllsdma_of_remove() calls
xllsdma_cleanup() which cleans the things up depending on what had been really
done.
> >> +
> >> + return res;
> >> +}
> >> +
> >> +static struct of_platform_driver xllsdma_of_driver = {
> >> + .name = "xilinx-sdma",
> >> + .match_table = xllsdma_of_match,
> >> + .probe = xllsdma_of_probe,
> >> + .remove = xllsdma_of_remove,
> >> +};
>
> Should be:
> +static struct of_platform_driver xllsdma_of_driver = {
> + .driver = {
> + .name = "xilinx-sdma",
> + .owner = THIS_MODULE,
> + }
> + .match_table = xllsdma_of_match,
> + .probe = xllsdma_of_probe,
> + .remove = __devexit_p(xllsdma_of_remove),
> +};
>
This code is rather aged, so I'll update this.
> >> +
> >> +int __init xllsdma_of_init(void)
> >> +{
> >> + int ret;
> >> +
> >> + ret = of_register_platform_driver(&xllsdma_of_driver);
> >> + if (ret) {
> >> + of_unregister_platform_driver(&xllsdma_of_driver);
> >> + printk(KERN_ERR "registering driver failed: err=%i", ret);
> >> + return ret;
> >> + }
> >> +
> >> + return 0;
> >> +}
>
> If registering fails, unregistering is not needed. This function
> should simply be:
>
> +int __init xllsdma_of_init(void)
> +{
> + return of_register_platform_driver(&xllsdma_of_driver);
> +}
>
> >> +
> >> +void xllsdma_of_exit(void)
> >> +{
> >> + of_unregister_platform_driver(&xllsdma_of_driver);
> >> +}
> >> +
> >> +static int mpmc_of_remove(struct of_device *op)
>
> +static int __devexit mpmc_of_remove(struct of_device *op)
>
> >> +{
> >> + struct device_node *node;
> >> + struct of_device *ofdev;
> >> +
> >> + for_each_child_of_node(op->node, node) {
> >> + ofdev = of_find_device_by_node(node);
> >> + of_device_unregister(ofdev);
> >> + of_device_free(ofdev);
> >> + }
> >> +
> >> + mpmc_cleanup(&op->dev);
> >> + return 0;
> >> +}
> >> +
> >> +static int __devinit mpmc_of_probe(struct of_device *op,
> >> + const struct of_device_id *match)
> >> +{
> >> + int err = mpmc_init(&op->dev);
> >> + if (err)
> >> + return err;
> >> +
> >> + of_platform_bus_probe(op->node, xllsdma_of_match, &op->dev);
> >> + return 0;
> >> +}
>
> Okay, I think I've figured out what is bothering me....
>
> While this *does* work; it really is the long way to go about things.
> Doing it this way requires going back out to the driver model to tell
> it things and trigger a probe on things that *this* driver needs, and
> *this* driver already knows about. It doesn't need to be this
> complex.
>
> Rather than register a bunch more of_platform devices, do something
> like this instead:
>
> +static int __devinit mpmc_of_probe(struct of_device *op,
> + const struct of_device_id *match)
> +{
> + struct mpmc_device *mpmc;
> + struct device_node *node;
> +
> + mpmc = mpmc_init(&op->dev);
> + if (!mpmc)
> + return -ENODEV;
> +
> + for_each_child_of_node(op->node, node) {
> + xllsdma_of_init(mpmc, node);
> + }
> + return 0;
> +}
>
> You do *not* need to register a separate struct device for each DMA
> channel sub node (at least with regard to the driver model; I don't
> know about the dma subsystem). If you *want* a struct device, then
> xllsdma_of_init() is free to register one, but it does not need to be
> on the of_platform_bus, and this driver should not require a probe
> step for each DMA channel.
>
It will simplify the code, I think.
> >> +
> >> +static struct of_device_id __devinitdata mpmc_of_match[] = {
> >> + { .compatible = "xlnx,mpmc-4.01.a" },
> >> + { .compatible = "xlnx,mpmc-4.03.a" },
> >> + {},
> >> +};
> >> +
> >> +static struct of_platform_driver mpmc_of_driver = {
> >> + .name = "xilinx-mpmc",
> >> + .match_table = mpmc_of_match,
> >> + .probe = mpmc_of_probe,
> >> + .remove = mpmc_of_remove,
> >> +};
> >> +
> >> +int __init mpmc_of_init(void)
> >> +{
> >> + return of_register_platform_driver(&mpmc_of_driver);
> >> +}
> >> +
> >> +void mpmc_of_exit(void)
> >> +{
> >> + of_unregister_platform_driver(&mpmc_of_driver);
> >> +}
>
> Missing the module exit hook?
>
> >> +
> >> +subsys_initcall(mpmc_of_init);
> >> +subsys_initcall(xllsdma_of_init);
>
> Typically initcall statements are placed immediately after the init
> function they reference.
>
> >> +#else /* CONFIG_OF */
>
> Why else? It is perfectly valid to have both of_platform and platform
> bus bindings. That being said, this split will become unnecessary in
> the very near future. I've eliminated of_platform_bus_type, and
> automatically moved all users of it over to the platform bus (without
> driver changes).
>
> However, current powerpc and microblaze code makes CONFIG_OF
> manditory. What condition will compile in the platform bus
> attachment?
>
> >> +/*---------------------------------------------------------------------
> >>------ + * Platform bus attachment
> >> + */
> >> +
> >> +static __devexit int xllsdma_plat_remove(struct platform_device *pdev)
> >> +{
> >> + xllsdma_cleanup(&pdev->dev);
> >> + return 0;
> >> +}
> >> +
> >> +static int __devinit xllsdma_plat_probe(struct platform_device *pdev)
> >> +{
> >> + struct resource *rx_irq, *tx_irq, *mem;
> >> + int err = 0;
> >> +
> >> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> + if (!mem) {
> >> + dev_err(&pdev->dev, "invalid address\n");
> >> + err = -EINVAL;
> >> + goto fail;
> >> + }
> >> +
> >> + /* RX interrupt is optional, and first */
> >> + rx_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> >> +
> >> + /* TX interrupt is optional, and second */
> >> + tx_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 1);
> >> +
> >
> > If it is impossible to create duplicate phandles in Device Tree,
> > there should be a check here that no device with pdev->id already exists
> > (i.e., it's NOT impossible with platform bus). It might be just as well
> > to put the check in xllsdma_init() since that's not a 'hot' code path.
>
> Question: microblaze and powerpc both use the device tree. What is
> the use-case for the non-dts version?
Maybe old kernel versions?
>
> >> + err = xllsdma_init(&pdev->dev, rx_irq, tx_irq, mem, pdev->id);
> >> + if (err)
> >> + xllsdma_plat_remove(pdev);
> >> +fail:
> >> + return err;
> >> +}
> >> +
> >> +static struct platform_driver xllsdma_plat_driver = {
> >> + .probe = xllsdma_plat_probe,
> >> + .remove = __devexit_p(xllsdma_plat_remove),
> >> + .driver = {
> >> + .owner = THIS_MODULE,
> >> + .name = "xilinx-sdma",
> >> + },
> >> +};
> >> +
> >> +int __init xllsdma_plat_init(void)
> >> +{
> >> + int err = platform_driver_register(&xllsdma_plat_driver);
> >> + if (err) {
> >> + platform_driver_unregister(&xllsdma_plat_driver);
> >> + printk(KERN_ERR "registering driver failed: err=%i", err);
> >> + return err;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +subsys_initcall(xllsdma_plat_init);
>
> Again, if driver registration fails, then the driver doesn't need to
> be unregistered. Same as with the of_platform_bus binding.
>
> >> +
> >> +void xllsdma_plat_exit(void)
> >> +{
> >> + platform_driver_unregister(&xllsdma_plat_driver);
> >> +}
> >> +
> >> +static int mpmc_plat_probe(struct platform_device *pdev)
> >> +{
> >> + return mpmc_init(&pdev->dev);
> >> +}
> >> +
> >> +static int __devexit mpmc_plat_remove(struct platform_device *pdev)
> >> +{
> >> + mpmc_cleanup(&pdev->dev);
> >> + return 0;
> >> +}
> >> +
> >> +static struct platform_driver mpmc_plat_driver = {
> >> + .probe = mpmc_plat_probe,
> >> + .remove = __devexit_p(mpmc_plat_remove),
> >> + .driver = {
> >> + .owner = THIS_MODULE,
> >> + .name = "xilinx-mpmc",
> >> + },
> >> +};
>
> I'll make the same argument here. The multiple registrations in this
> driver is weird. This driver already knows about all of it's dma
> channels, so why depend on the core driver model to probe a driver
> instance for each?
>
> >> +
> >> +int __init mpmc_plat_init(void)
> >> +{
> >> + return platform_driver_register(&mpmc_plat_driver);
> >> +}
> >> +subsys_initcall(mpmc_plat_init);
> >> +
> >> +void mpmc_plat_exit(void)
> >> +{
> >> + platform_driver_unregister(&mpmc_plat_driver);
> >> +}
> >> +#endif /* CONFIG_OF */
> >> diff -r baced9e29ab5 include/linux/xllsdma.h
> >> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> >> +++ b/include/linux/xllsdma.h Wed Apr 28 02:00:51 2010 +0400
> >> @@ -0,0 +1,187 @@
> >> +/*
> >> + * SDMA subsystem support for Xilinx MPMC.
> >> + *
> >> + * Author: Sergey Temerkhanov
> >> + *
> >> + * Copyright (c) 2008-2010 Cifronic ZAO
> >> + *
> >> + * This program is free software; you can redistribute it and/or
> >> modify it + * under the terms of the GNU General Public License as
> >> published by the + * Free Software Foundation; either version 2 of the
> >> License, or (at your + * option) any later version.
> >> + *
> >> + */
> >> +
> >> +#ifndef __XLLSDMA_H__
> >> +#define __XLLSDMA_H__
> >> +
> >> +#include <linux/types.h>
> >> +#include <linux/dma-mapping.h>
> >> +
> >> +#define XLLSDMA_ALIGNMENT 0x40
> >> +
> >> +struct xllsdma_desc {
> >> + __be32 next;
> >> + __be32 address;
> >> + __be32 length;
> >> + __be32 stat_ctl;
> >> + __be32 app1;
> >> + __be32 app2;
> >> + __be32 app3;
> >> + __be32 app4;
> >> + void *virt;
> >> + u32 flags;
> >> +} __attribute__((aligned(XLLSDMA_ALIGNMENT)));
> >
> > 'virt' and 'flags' are not used by this driver. Putting them in this
> > structure, and using this structure in the API, forces them on all
> > clients, which can lead to inefficiencies (this will certainly be true in
> > the DMA engine driver, perhaps others). I think a better choice would be
> > to have the API use a structure without these two fields. For the
> > convenience of other clients you could define a 'superstructure' _not_
> > used in the API, like:
> >
> > struct xllsdma_fancy_desc {
> > struct xllsdma_desc basic_desc;
> > void *virt;
> > u32 flags;
> > }
Looks reasonable.
^ permalink raw reply
* Re: [alsa-devel] [PATCH 1/2] powerpc: add platform registration for ALSA SoC drivers
From: Grant Likely @ 2010-04-28 21:58 UTC (permalink / raw)
To: Timur Tabi
Cc: alsa-devel, kumar.gala, Mark Brown, linuxppc-dev,
devicetree-discuss, lrg
In-Reply-To: <j2qed82fe3e1004281335h4076b050jc2894d4c4d6eac65@mail.gmail.com>
On Wed, Apr 28, 2010 at 2:35 PM, Timur Tabi <timur.tabi@gmail.com> wrote:
> On Tue, Apr 27, 2010 at 5:09 AM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
>
>> Keep in mind that it's perfectly kosher to create nodes for "virtual"
>> devices. IE. We could imagine a node for the "sound subsystem" that
>> doesn't actually correspond to any physical device but contain the
>> necessary properties that binds everything together. You could even have
>> multiple of these if you have separate set of sound HW that aren't
>> directly dependant.
>
> First, I want to officially retract this patch. =A0I've talked with
> Grant, and we've come up with a different approach to this problem.
>
> Second, how about this binding for the virtual sound node? =A0It would
> be a root-level node.
>
> =A0 =A0 =A0 =A0sound-devices {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0sound0 {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ssi =3D &ssi0;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0playback-dma =3D &dma00;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0capture-dma =3D &dma01;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0codec =3D &cs4270;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0};
The sound0 node needs a compatible value, the sound-device node should
probably have one too.
The sound0 node should have something board specific like
"fsl,mpc8610hpcd-sound" to make it clear that the binding really only
applies to this particular board. It would also be a good idea to
prefix all of the property names with 'fsl,' to avoid conflicting with
any future common bindings or conventions. Other boards can use the
same binding, but they would get a different compatible value (the
driver could bind on both).
I'm not a huge fan of the name "sound-devices" for the parent node.
There are other sorts of things that we need 'virtual' device nodes to
describe. It would be nice to have a single place for collecting
nodes for stuff like this. Perhaps this:
system {
compatible =3D "system-devices";
sound0 {
compatible =3D "fsl,mpc8610hpcd-sound";
fsl,ssi =3D &ssi0;
fsl,playback-dma =3D &dma00;
fsl,capture-dma =3D &dma01;
fsl,codec =3D &cs4270;
};
};
But I really don't have any knowledge of what has been done previously
in this regard or if any conventions have been established. Ben, any
thoughts?
g.
^ permalink raw reply
* Re: [PATCH 1/2] genirq: reliably replay pending edge-triggered irq
From: Guillaume Knispel @ 2010-04-28 21:56 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Lars-Peter Clausen, Russell King, Haavard Skinnemoen,
Bartlomiej Zolnierkiewicz, Peter Zijlstra, linux-kernel,
Linus Torvalds, Michael Buesch, Ingo Molnar, Linuxppc-dev
In-Reply-To: <alpine.LFD.2.00.1004271537130.2951@localhost.localdomain>
On Tue, 27 Apr 2010 15:42:11 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 22 Apr 2010, Guillaume Knispel wrote:
[snip]
> > acked and masked at controller level and IRQ_PENDING is set.
> > ---
> > arch/arm/Kconfig | 4 ----
> > arch/arm/configs/acs5k_defconfig | 1 -
> > arch/arm/configs/acs5k_tiny_defconfig | 1 -
>
> Changing the defconfigs should be a separate patch and I'm not
> going to push such a thing through the genirq tree. The stale
> entries do not hurt.
Ok
In this separate patch, to I put "genirq" in the title?
Do I put it in the same series?
Cheers,
Guillaume Knispel
^ permalink raw reply
* Re: [alsa-devel] [PATCH 1/2] powerpc: add platform registration for ALSA SoC drivers
From: Timur Tabi @ 2010-04-28 20:35 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: alsa-devel, kumar.gala, Mark Brown, linuxppc-dev, lrg
In-Reply-To: <1272362955.24542.24.camel@pasglop>
On Tue, Apr 27, 2010 at 5:09 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> Keep in mind that it's perfectly kosher to create nodes for "virtual"
> devices. IE. We could imagine a node for the "sound subsystem" that
> doesn't actually correspond to any physical device but contain the
> necessary properties that binds everything together. You could even have
> multiple of these if you have separate set of sound HW that aren't
> directly dependant.
First, I want to officially retract this patch. I've talked with
Grant, and we've come up with a different approach to this problem.
Second, how about this binding for the virtual sound node? It would
be a root-level node.
sound-devices {
sound0 {
ssi = &ssi0;
playback-dma = &dma00;
capture-dma = &dma01;
codec = &cs4270;
}
};
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: [PATCH 3/3] powerpc/mpc5121: shared DIU framebuffer support
From: Anatolij Gustschin @ 2010-04-28 20:28 UTC (permalink / raw)
To: Grant Likely; +Cc: linuxppc-dev, linux-fbdev, yorksun, dzu, wd
In-Reply-To: <fa686aa41002272250j64b2d691vb41f73e1eb335c8a@mail.gmail.com>
Hi Grant,
Thanks for review and comments. I'm back to this now and hope
to address your comments soon.
On Sat, 27 Feb 2010 23:50:09 -0700
Grant Likely <grant.likely@secretlab.ca> wrote:
> On Sat, Feb 27, 2010 at 2:58 PM, Anatolij Gustschin <agust@denx.de> wrote:
> > MPC5121 DIU configuration/setup as initialized by the boot
> > loader currently will get lost while booting Linux. As a
> > result displaying the boot splash is not possible through
> > the boot process.
> >
> > To prevent this we reserve configured DIU frame buffer
> > address range while booting and preserve AOI descriptor
> > and gamma table so that DIU continues displaying through
> > the whole boot process. On first open from user space
> > DIU frame buffer driver releases the reserved frame
> > buffer area and continues to operate as usual.
> >
> > The patch also moves drivers/video/fsl-diu-fb.h file to
> > include/linux as we use some DIU structures in platform
> > code.
> >
> > 'diu_ops' callbacks in platform code borrowed from John's
> > DIU code.
> >
> > Signed-off-by: John Rigby <jrigby@gmail.com>
> > Signed-off-by: Anatolij Gustschin <agust@denx.de>
> > Cc: Grant Likely <grant.likely@secretlab.ca>
>=20
> On quick glance this patch seems mostly okay, but this patch should
> probably be broken up a bit to simplify review and dissociate
> unrelated changes. For example, the move of fsl-diu-fb.h is a
> discrete change that should be split off. Some more comments
> below....
I will split off fsl-diu-fb.h move to separate patch.
...
> > diff --git a/arch/powerpc/platforms/512x/mpc5121_generic.c b/arch/power=
pc/platforms/512x/mpc5121_generic.c
> > index a6c0e3a..3aaa281 100644
> > --- a/arch/powerpc/platforms/512x/mpc5121_generic.c
> > +++ b/arch/powerpc/platforms/512x/mpc5121_generic.c
> > @@ -28,6 +28,7 @@
> > =C2=A0*/
> > =C2=A0static char *board[] __initdata =3D {
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0"prt,prtlvt",
> > + =C2=A0 =C2=A0 =C2=A0 "ifm,pdm360ng",
>=20
> You're adding a new board here. This is completely unrelated.
Yes, it is unrelated. This hunk sneaked in by accident, will drop it.
...
> > diff --git a/arch/powerpc/platforms/512x/mpc512x.h b/arch/powerpc/platf=
orms/512x/mpc512x.h
> > index d72b2c7..1cfe9d5 100644
> > --- a/arch/powerpc/platforms/512x/mpc512x.h
> > +++ b/arch/powerpc/platforms/512x/mpc512x.h
> > @@ -17,4 +17,7 @@ extern int __init mpc5121_clk_init(void);
> > =C2=A0void __init mpc512x_declare_of_platform_devices(void);
> > =C2=A0extern void mpc512x_restart(char *cmd);
> > =C2=A0extern void __init mpc5121_usb_init(void);
> > +extern void __init mpc512x_init_diu(void);
> > +extern void __init mpc512x_setup_diu(void);
>=20
> __init annotations do not belong in header files.
Ok, I will remove them.
> > +extern struct fsl_diu_shared_fb diu_shared_fb;
>=20
> Hmmmm. I'm not fond of the global data structure. Especially
> considering that the struct fsl_diu_shared_fb is defined in
> mpc512x_shared.c, so nothing outside of that .c file can do anything
> with the structure.
This is a remainder from early debugging code, will remove it.
> > =C2=A0#endif =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* __MPC512X_H__ */
> > diff --git a/arch/powerpc/platforms/512x/mpc512x_shared.c b/arch/powerp=
c/platforms/512x/mpc512x_shared.c
> > index fbdf65f..a8c50a6 100644
> > --- a/arch/powerpc/platforms/512x/mpc512x_shared.c
> > +++ b/arch/powerpc/platforms/512x/mpc512x_shared.c
> > @@ -16,7 +16,11 @@
> > =C2=A0#include <linux/io.h>
> > =C2=A0#include <linux/irq.h>
> > =C2=A0#include <linux/of_platform.h>
> > +#include <linux/fsl-diu-fb.h>
> > +#include <linux/bootmem.h>
> > +#include <sysdev/fsl_soc.h>
> >
> > +#include <asm/cacheflush.h>
> > =C2=A0#include <asm/machdep.h>
> > =C2=A0#include <asm/ipic.h>
> > =C2=A0#include <asm/prom.h>
> > @@ -53,6 +57,284 @@ void mpc512x_restart(char *cmd)
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0;
> > =C2=A0}
> >
> > +struct fsl_diu_shared_fb {
> > + =C2=A0 =C2=A0 =C2=A0 char =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ga=
mma[0x300]; =C2=A0 /* 32-bit aligned! */
> > + =C2=A0 =C2=A0 =C2=A0 struct diu_ad =C2=A0 ad0; =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0/* 32-bit aligned! */
> > + =C2=A0 =C2=A0 =C2=A0 phys_addr_t =C2=A0 =C2=A0 fb_phys;
> > + =C2=A0 =C2=A0 =C2=A0 size_t =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0fb_len;
> > + =C2=A0 =C2=A0 =C2=A0 bool =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0in=
_use;
> > +};
> > +
> > +unsigned int mpc512x_get_pixel_format(unsigned int bits_per_pixel,
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 int monitor_port)
> > +{
> > + =C2=A0 =C2=A0 =C2=A0 unsigned int pix_fmt;
> > +
> > + =C2=A0 =C2=A0 =C2=A0 switch (bits_per_pixel) {
> > + =C2=A0 =C2=A0 =C2=A0 case 32:
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 pix_fmt =3D 0x888833=
16;
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
> > + =C2=A0 =C2=A0 =C2=A0 case 24:
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 pix_fmt =3D 0x880822=
19;
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
> > + =C2=A0 =C2=A0 =C2=A0 case 16:
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 pix_fmt =3D 0x650531=
18;
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
> > + =C2=A0 =C2=A0 =C2=A0 default:
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 pix_fmt =3D 0x000004=
00;
> > + =C2=A0 =C2=A0 =C2=A0 }
> > + =C2=A0 =C2=A0 =C2=A0 return pix_fmt;
> > +}
> > +
> > +void mpc512x_set_gamma_table(int monitor_port, char *gamma_table_base)
> > +{
> > +}
> > +
> > +void mpc512x_set_monitor_port(int monitor_port)
> > +{
> > +}
> > +
> > +#define CCM_SCFR1 =C2=A0 =C2=A0 =C2=A00x0000000c
> > +#define DIU_DIV_MASK =C2=A0 0x000000ff
> > +void mpc512x_set_pixel_clock(unsigned int pixclock)
> > +{
> > + =C2=A0 =C2=A0 =C2=A0 unsigned long bestval, bestfreq, speed_ccb, busf=
req;
> > + =C2=A0 =C2=A0 =C2=A0 unsigned long minpixclock, maxpixclock, pixval;
> > + =C2=A0 =C2=A0 =C2=A0 struct device_node *np;
> > + =C2=A0 =C2=A0 =C2=A0 void __iomem *ccm;
> > + =C2=A0 =C2=A0 =C2=A0 u32 temp;
> > + =C2=A0 =C2=A0 =C2=A0 long err;
> > + =C2=A0 =C2=A0 =C2=A0 int i;
> > +
> > + =C2=A0 =C2=A0 =C2=A0 np =3D of_find_compatible_node(NULL, NULL, "fsl,=
mpc5121-clock");
> > + =C2=A0 =C2=A0 =C2=A0 if (!np) {
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 pr_err("Can't find c=
lock control module.\n");
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return;
> > + =C2=A0 =C2=A0 =C2=A0 }
> > +
> > + =C2=A0 =C2=A0 =C2=A0 ccm =3D of_iomap(np, 0);
> > + =C2=A0 =C2=A0 =C2=A0 if (!ccm) {
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 pr_err("Can't map cl=
ock control module reg.\n");
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 of_node_put(np);
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return;
> > + =C2=A0 =C2=A0 =C2=A0 }
> > + =C2=A0 =C2=A0 =C2=A0 of_node_put(np);
> > +
> > + =C2=A0 =C2=A0 =C2=A0 busfreq =3D 200000000;
>=20
> Instead of some hard coding some bogus defalt busfreq, you should
> error out if the real frequency cannot be determined. Force users to
> supply a valid tree.
Ok, will fix.
...
> > +
> > + =C2=A0 =C2=A0 =C2=A0 /* Calculate the pixel clock with the smallest e=
rror */
> > + =C2=A0 =C2=A0 =C2=A0 /* calculate the following in steps to avoid ove=
rflow */
> > + =C2=A0 =C2=A0 =C2=A0 pr_debug("DIU pixclock in ps - %d\n", pixclock);
> > + =C2=A0 =C2=A0 =C2=A0 temp =3D 1;
> > + =C2=A0 =C2=A0 =C2=A0 temp *=3D 1000000000;
> > + =C2=A0 =C2=A0 =C2=A0 temp /=3D pixclock;
> > + =C2=A0 =C2=A0 =C2=A0 temp *=3D 1000;
> > + =C2=A0 =C2=A0 =C2=A0 pixclock =3D temp;
>=20
> Really? I think you can simplify this.
Yes, will do it in the next patch.
...
> > diff --git a/drivers/video/fsl-diu-fb.c b/drivers/video/fsl-diu-fb.c
> > index 19ca1da..263f7da 100644
> > --- a/drivers/video/fsl-diu-fb.c
> > +++ b/drivers/video/fsl-diu-fb.c
> > @@ -34,7 +34,7 @@
> > =C2=A0#include <linux/of_platform.h>
> >
> > =C2=A0#include <sysdev/fsl_soc.h>
> > -#include "fsl-diu-fb.h"
> > +#include <linux/fsl-diu-fb.h>
> >
> > =C2=A0#include "ofmode.h"
> >
> > @@ -331,8 +331,11 @@ static int fsl_diu_enable_panel(struct fb_info *in=
fo)
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0if (mfbi->type !=3D MFB_TYPE_OFF) {
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0switch (mfbi->in=
dex) {
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0case 0: =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 /* plane 0 */
> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
=C2=A0 if (hw->desc[0] !=3D ad->paddr)
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
=C2=A0 if (in_be32(&hw->desc[0]) !=3D ad->paddr) {
>=20
> Unrelated bugfix? If so, please split into separate patch.
>=20
>=20
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 out_be32(&dr.diu_reg->diu_mode, 0);
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0out_be32(&hw->desc[0], ad->paddr);
>=20
> This line also looks like it needs fixing.
Will re-check it an fix.
>=20
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 out_be32(&dr.diu_reg->diu_mode, 1);
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
=C2=A0 }
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0break;
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0case 1: =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 /* plane 1 AOI 0 */
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0cmfbi =3D machine_data->fsl_diu_info[2]->par;
> > @@ -391,7 +394,7 @@ static int fsl_diu_disable_panel(struct fb_info *in=
fo)
> >
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0switch (mfbi->index) {
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0case 0: =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 /* plane 0 */
> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (hw->desc[0] !=3D=
machine_data->dummy_ad->paddr)
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (in_be32(&hw->des=
c[0]) !=3D machine_data->dummy_ad->paddr)
>=20
> Same bugfix?
Will re-check it, too.
>=20
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0out_be32(&hw->desc[0],
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0machine_data->dummy_ad->paddr);
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break;
> > @@ -1102,6 +1105,10 @@ static int fsl_diu_open(struct fb_info *info, in=
t user)
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0struct mfb_info *mfbi =3D info->par;
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0int res =3D 0;
> >
> > + =C2=A0 =C2=A0 =C2=A0 /* free boot splash memory on first /dev/fb0 ope=
n */
> > + =C2=A0 =C2=A0 =C2=A0 if (!mfbi->index && diu_ops.release_bootmem)
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 diu_ops.release_boot=
mem();
> > +
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0spin_lock(&diu_lock);
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0mfbi->count++;
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0if (mfbi->count =3D=3D 1) {
> > @@ -1436,6 +1443,7 @@ static int __devinit fsl_diu_probe(struct of_devi=
ce *ofdev,
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0int ret, i, error =3D 0;
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0struct resource res;
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0struct fsl_diu_data *machine_data;
> > + =C2=A0 =C2=A0 =C2=A0 int diu_mode;
> >
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0machine_data =3D kzalloc(sizeof(struct fsl_d=
iu_data), GFP_KERNEL);
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!machine_data)
> > @@ -1472,7 +1480,9 @@ static int __devinit fsl_diu_probe(struct of_devi=
ce *ofdev,
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto error2;
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> >
> > - =C2=A0 =C2=A0 =C2=A0 out_be32(&dr.diu_reg->diu_mode, 0); =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* disable DIU anyway*/
> > + =C2=A0 =C2=A0 =C2=A0 diu_mode =3D in_be32(&dr.diu_reg->diu_mode);
> > + =C2=A0 =C2=A0 =C2=A0 if (diu_mode !=3D MFB_MODE1)
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 out_be32(&dr.diu_reg=
->diu_mode, 0); =C2=A0 =C2=A0 /* disable DIU */
>=20
> Is this the best approach? Would it be better to make this decision
> based on a property in the device tree?
I don't think it is worth it. The driver disables the DIU
regardless of it is enabled or not and enables it later
using a dummy descriptor. We just prevent this disabling
if the DIU is pre-initialized and already displaying.
> >
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Get the IRQ of the DIU */
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0machine_data->irq =3D irq_of_parse_and_map(n=
p, 0);
> > @@ -1520,7 +1530,12 @@ static int __devinit fsl_diu_probe(struct of_dev=
ice *ofdev,
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0machine_data->dummy_ad->offset_xyd =3D 0;
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0machine_data->dummy_ad->next_ad =3D 0;
> >
> > - =C2=A0 =C2=A0 =C2=A0 out_be32(&dr.diu_reg->desc[0], machine_data->dum=
my_ad->paddr);
> > + =C2=A0 =C2=A0 =C2=A0 /* Let DIU display splash screen if it was pre-i=
nitialized
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0* by the bootloader, set dummy area descri=
ptor otherwise.
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
> > + =C2=A0 =C2=A0 =C2=A0 if (diu_mode !=3D MFB_MODE1)
> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 out_be32(&dr.diu_reg=
->desc[0], machine_data->dummy_ad->paddr);
> > +
>=20
> Same as above.
>=20
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0out_be32(&dr.diu_reg->desc[1], machine_data-=
>dummy_ad->paddr);
> > =C2=A0 =C2=A0 =C2=A0 =C2=A0out_be32(&dr.diu_reg->desc[2], machine_data-=
>dummy_ad->paddr);
> >
>=20
>=20
>=20
Thanks,
Anatolij
^ permalink raw reply
* Re: [PATCH] Fix DEBUG_PAGEALLOC on 603/e300
From: Kim Phillips @ 2010-04-28 19:46 UTC (permalink / raw)
To: Xianghua Xiao; +Cc: linuxppc-dev
In-Reply-To: <y2te6bf505b1004281215n9dcc6190j9915aa27ae91f75d@mail.gmail.com>
On Wed, 28 Apr 2010 14:15:50 -0500
Xianghua Xiao <xiaoxianghua@gmail.com> wrote:
> On Wed, Apr 28, 2010 at 2:22 AM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > So we tried to speed things up a bit using flush_hash_pages() directly
> > but that falls over on 603 of course meaning we fail to flush the TLB
> > properly and we may even end up having it corrupt memory randomly by
> > accessing a hash table that doesn't exist.
> >
> > This removes the "optimization" by always going through flush_tlb_page()
> > for now at least.
> >
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > ---
> > Somebody with a 603 or e300 core based FSL SoC to try this out for me ?
> >
> This change works me on a 834x(e300) platform, tested with lmbench and
> a production-ready application with 2.6.33.3.
moi aussi, Ben: booted this on an 8377 and started a kernel build.
It seems to be progressing normally, I'm just not going to wait for it
to complete before hitting send :).
Kim
^ permalink raw reply
* Re: [PATCH] Fix DEBUG_PAGEALLOC on 603/e300
From: Xianghua Xiao @ 2010-04-28 19:15 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev
In-Reply-To: <1272439375.24542.94.camel@pasglop>
On Wed, Apr 28, 2010 at 2:22 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> So we tried to speed things up a bit using flush_hash_pages() directly
> but that falls over on 603 of course meaning we fail to flush the TLB
> properly and we may even end up having it corrupt memory randomly by
> accessing a hash table that doesn't exist.
>
> This removes the "optimization" by always going through flush_tlb_page()
> for now at least.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>
> Somebody with a 603 or e300 core based FSL SoC to try this out for me ?
>
> It's obviously completely untested :-)
>
> Cheers,
> Ben.
>
> diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
> index b9243e7..95774b4 100644
> --- a/arch/powerpc/mm/pgtable_32.c
> +++ b/arch/powerpc/mm/pgtable_32.c
> @@ -385,11 +385,7 @@ static int __change_page_attr(struct page *page, pgp=
rot_t prot)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -EINVAL;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0__set_pte_at(&init_mm, address, kpte, mk_pte(p=
age, prot), 0);
> =C2=A0 =C2=A0 =C2=A0 =C2=A0wmb();
> -#ifdef CONFIG_PPC_STD_MMU
> - =C2=A0 =C2=A0 =C2=A0 flush_hash_pages(0, address, pmd_val(*kpmd), 1);
> -#else
> =C2=A0 =C2=A0 =C2=A0 =C2=A0flush_tlb_page(NULL, address);
> -#endif
> =C2=A0 =C2=A0 =C2=A0 =C2=A0pte_unmap(kpte);
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;
>
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>
This change works me on a 834x(e300) platform, tested with lmbench and
a production-ready application with 2.6.33.3.
Xianghua
^ permalink raw reply
* Re: [microblaze-uclinux] [PATCHv2] [RFC] Xilinx MPMC SDMA subsystem
From: Steven J. Magnani @ 2010-04-28 17:35 UTC (permalink / raw)
To: microblaze-uclinux; +Cc: linuxppc-dev, Linux Kernel Mailing List
In-Reply-To: <n2vfa686aa41004272213wceeef998tadc483ec1de7b792@mail.gmail.com>
Grant -
Thanks for the feedback. My responses are inline.
--Steve
On Tue, 2010-04-27 at 23:13 -0600, Grant Likely wrote:
> Hi Sergey and Steven,
>
> On Tue, Apr 27, 2010 at 8:29 PM, Steven J. Magnani
> <steve@digidescorp.com> wrote:
> > On Wed, 2010-04-28 at 02:06 +0400, Sergey Temerkhanov wrote:
> >> This is the 2nd version of Xilinx MPMC LocalLink SDMA subsystem
> >>
[snip]
> >> diff -r baced9e29ab5 drivers/dma/Kconfig
> >> --- a/drivers/dma/Kconfig Tue Apr 27 20:48:50 2010 +0400
> >> +++ b/drivers/dma/Kconfig Wed Apr 28 02:00:51 2010 +0400
> >> @@ -97,6 +97,14 @@
> >> Support the TXx9 SoC internal DMA controller. This can be
> >> integrated in chips such as the Toshiba TX4927/38/39.
> >>
> >> +config XLLSDMA
>
> I'd prefer XILINX_LLSDMA. XLLSDMA could be ambiguous in the global
> namespace (for a human reader), particularly as it is something that
> few people will actually see.
>
> >> + bool "Xilinx MPMC DMA support"
> >> + depends on XILINX_VIRTEX || MICROBLAZE
> >> + select DMA_ENGINE
> >> + help
> >> + Support fot Xilinx MPMC LocalLink SDMA. Virtex FPGA family
> >> + has it integrated or fabric-based.
> >> +
> >
> > fot --> for
> >
> > Since the xllsdma driver provides services to other drivers - not to userland -
> > I think this would be better as a "silent" option, selected automatically when
> > something like ll_temac or the forthcoming Xilinx DMA engine is selected.
> > If we do it that way, note that XLLSDMA is independent of DMA_ENGINE so
> > drivers/Makefile will need to be patched so that the dma subdirectory is
> > always "y".
>
> Agreed. However, looking at this code, I don't see anything that
> actually uses DMA_ENGINE here. Am I missing something?
This is a low-level driver that provides services to ll_temac and the
(still in the works) DMA engine driver. There's no real relationship
between this driver and DMA_ENGINE, and the still-in-the-works driver
will come later after we stabilize the interface of this one.
> >> +++ b/drivers/dma/xllsdma.c Wed Apr 28 02:00:51 2010 +0400
[snip]
> >> +
> >> +static int __devinit mpmc_of_probe(struct of_device *op,
> >> + const struct of_device_id *match)
> >> +{
> >> + int err = mpmc_init(&op->dev);
> >> + if (err)
> >> + return err;
> >> +
> >> + of_platform_bus_probe(op->node, xllsdma_of_match, &op->dev);
> >> + return 0;
> >> +}
>
> Okay, I think I've figured out what is bothering me....
>
> While this *does* work; it really is the long way to go about things.
> Doing it this way requires going back out to the driver model to tell
> it things and trigger a probe on things that *this* driver needs, and
> *this* driver already knows about. It doesn't need to be this
> complex.
>
> Rather than register a bunch more of_platform devices, do something
> like this instead:
>
> +static int __devinit mpmc_of_probe(struct of_device *op,
> + const struct of_device_id *match)
> +{
> + struct mpmc_device *mpmc;
> + struct device_node *node;
> +
> + mpmc = mpmc_init(&op->dev);
> + if (!mpmc)
> + return -ENODEV;
> +
> + for_each_child_of_node(op->node, node) {
> + xllsdma_of_init(mpmc, node);
> + }
> + return 0;
> +}
>
> You do *not* need to register a separate struct device for each DMA
> channel sub node (at least with regard to the driver model; I don't
> know about the dma subsystem). If you *want* a struct device, then
> xllsdma_of_init() is free to register one, but it does not need to be
> on the of_platform_bus, and this driver should not require a probe
> step for each DMA channel.
The DMA engine driver (and ll_temac, I imagine) will gain access to this driver via xllsdma_find_device().
They should not need a struct device.
[snip]
> >> +#else /* CONFIG_OF */
>
> Why else? It is perfectly valid to have both of_platform and platform
> bus bindings. That being said, this split will become unnecessary in
> the very near future. I've eliminated of_platform_bus_type, and
> automatically moved all users of it over to the platform bus (without
> driver changes).
Agreed. It would help to have an idea of the timeline for the
convergence. I haven't been following any OF-related discussions on
LKML; I'll try to pay more attention.
>
> However, current powerpc and microblaze code makes CONFIG_OF
> manditory. What condition will compile in the platform bus
> attachment?
At the moment, none in mainline. See my next comment.
>
> >> +/*---------------------------------------------------------------------------
> >> + * Platform bus attachment
> >> + */
> >> +
> >> +static __devexit int xllsdma_plat_remove(struct platform_device *pdev)
> >> +{
> >> + xllsdma_cleanup(&pdev->dev);
> >> + return 0;
> >> +}
> >> +
> >> +static int __devinit xllsdma_plat_probe(struct platform_device *pdev)
> >> +{
> >> + struct resource *rx_irq, *tx_irq, *mem;
> >> + int err = 0;
> >> +
> >> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> + if (!mem) {
> >> + dev_err(&pdev->dev, "invalid address\n");
> >> + err = -EINVAL;
> >> + goto fail;
> >> + }
> >> +
> >> + /* RX interrupt is optional, and first */
> >> + rx_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> >> +
> >> + /* TX interrupt is optional, and second */
> >> + tx_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 1);
> >> +
> >
> > If it is impossible to create duplicate phandles in Device Tree,
> > there should be a check here that no device with pdev->id already exists
> > (i.e., it's NOT impossible with platform bus). It might be just as well to
> > put the check in xllsdma_init() since that's not a 'hot' code path.
>
> Question: microblaze and powerpc both use the device tree. What is
> the use-case for the non-dts version?
My kernel, for one. During microblaze integration into mainline,
toolchains that supported device tree were not available to common folk,
so I had to hack up my kernel to work around this. In my kernel, OF is a
selectable option. Since then new tools have become available (although
AFAIK none officially from Xilinx - we are still having lots of problems
with toolchains), but my last attempt to switch to device tree failed.
At this point we're too far in the development cycle to justify the
effort involved in a switch.
[snip]
> >> +int __init xllsdma_plat_init(void)
> >> +{
> >> + int err = platform_driver_register(&xllsdma_plat_driver);
> >> + if (err) {
> >> + platform_driver_unregister(&xllsdma_plat_driver);
> >> + printk(KERN_ERR "registering driver failed: err=%i", err);
> >> + return err;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +subsys_initcall(xllsdma_plat_init);
>
> Again, if driver registration fails, then the driver doesn't need to
> be unregistered. Same as with the of_platform_bus binding.
Agreed.
>
> >> +
> >> +void xllsdma_plat_exit(void)
> >> +{
> >> + platform_driver_unregister(&xllsdma_plat_driver);
> >> +}
> >> +
> >> +static int mpmc_plat_probe(struct platform_device *pdev)
> >> +{
> >> + return mpmc_init(&pdev->dev);
> >> +}
> >> +
> >> +static int __devexit mpmc_plat_remove(struct platform_device *pdev)
> >> +{
> >> + mpmc_cleanup(&pdev->dev);
> >> + return 0;
> >> +}
> >> +
> >> +static struct platform_driver mpmc_plat_driver = {
> >> + .probe = mpmc_plat_probe,
> >> + .remove = __devexit_p(mpmc_plat_remove),
> >> + .driver = {
> >> + .owner = THIS_MODULE,
> >> + .name = "xilinx-mpmc",
> >> + },
> >> +};
>
> I'll make the same argument here. The multiple registrations in this
> driver is weird. This driver already knows about all of it's dma
> channels, so why depend on the core driver model to probe a driver
> instance for each?
This code depends on the same infrastructure/concept as its OF
countertpart. When those change, so will this.
------------------------------------------------------------------------
Steven J. Magnani "I claim this network for MARS!
www.digidescorp.com Earthling, return my space modulator!"
#include <standard.disclaimer>
^ permalink raw reply
* Re: [alsa-devel] [PATCH 1/2] powerpc: add platform registration for ALSA SoC drivers
From: Timur Tabi @ 2010-04-28 17:27 UTC (permalink / raw)
To: Grant Likely; +Cc: alsa-devel, kumar.gala, broonie, linuxppc-dev, Liam Girdwood
In-Reply-To: <j2lfa686aa41004280947wbd8a90aeo3516475aa0987c78@mail.gmail.com>
Grant Likely wrote:
> Didn't you just finish saying that you cannot see any situation where
> you would want the SSI devices linked into a single audio device? So
> then if both SSIs are being used for audio, then do you not need a
> machine driver for each ssi?
That's right. But in order for my fabric/machine driver to be called at all, I thought I needed to make it a platform driver and use platform_device_register_simple() and platform_driver_register().
>> Are you saying that I should call platform_device_register_simple()
>> from the SSI's driver initialization function, fsl_ssi_init()?
>
> No, I'm saying call it from the probe hook.
But then platform_device_register_simple() will be called twice, and it should be called only once.
I must be missing something here.
> However, you should probably do a two stage platform_device_alloc() /
> platform_device_add() so you can add data (node pointer) to the
> platform device before it gets probed by the machine driver.
Don't you mean platform_device_add_resources() instead of platform_device_add()? That is, use platform_device_add_resources() too add information about each SSI that gets probed, and then after all of the SSIs are probed, then call platform_device_add().
If that's what you mean, then where do I call platform_device_add()?
> I'm not talking about platform_data or about the ssi driver decoding
> things that the machine driver needs. I'm talking about giving the
> machine driver a pointer to the ssi device tree node so you don't need
> to go through weird gymnastics to find the correct node again.
The only gymnastics I need to go through is this:
while ((np = of_find_compatible_node(np, NULL, "fsl,mpc8610-ssi"))) {
This finds every SSI node. I then extract the information from the SSI nodes and all the other nodes, and then register it with ASoC.
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: [alsa-devel] [PATCH 1/2] powerpc: add platform registration for ALSA SoC drivers
From: Grant Likely @ 2010-04-28 16:47 UTC (permalink / raw)
To: Timur Tabi; +Cc: alsa-devel, kumar.gala, broonie, linuxppc-dev, Liam Girdwood
In-Reply-To: <k2ped82fe3e1004280920xb1d8ef7ew34f86fd0ae67cf61@mail.gmail.com>
On Wed, Apr 28, 2010 at 10:20 AM, Timur Tabi <timur@freescale.com> wrote:
> On Wed, Apr 28, 2010 at 8:57 AM, Grant Likely <grant.likely@secretlab.ca>=
wrote:
>
>> I'm just saying move the registration of the machine device out of
>> arch/powerpc platform code and into the ssi driver.
>
> But the SSI driver is an OF driver, and it gets probed for every SSI
> node in the device tree. =A0On the 8610, that means being probed twice.
> But I should only call platform_device_register_simple() once.
Didn't you just finish saying that you cannot see any situation where
you would want the SSI devices linked into a single audio device? So
then if both SSIs are being used for audio, then do you not need a
machine driver for each ssi?
> Are you saying that I should call platform_device_register_simple()
> from the SSI's driver initialization function, fsl_ssi_init()?
No, I'm saying call it from the probe hook.
However, you should probably do a two stage platform_device_alloc() /
platform_device_add() so you can add data (node pointer) to the
platform device before it gets probed by the machine driver.
>> Then you've got a
>> reasonable place to pass shared data (either the ssi device node or
>> device instance or name. =A0Whatever you need) to the machine driver.
>
> The problem is that the fabric driver needs much more information from
> the device tree than the SSI driver needs. =A0So if the SSI driver is
> going to pass that information to the fabric driver via the platform
> data, it's going to have to know what information the fabric driver
> needs. =A0Then the SSI driver is not board-independent.
I'm not talking about platform_data or about the ssi driver decoding
things that the machine driver needs. I'm talking about giving the
machine driver a pointer to the ssi device tree node so you don't need
to go through weird gymnastics to find the correct node again.
g.
^ permalink raw reply
* Re: [alsa-devel] [PATCH 1/2] powerpc: add platform registration for ALSA SoC drivers
From: Timur Tabi @ 2010-04-28 16:20 UTC (permalink / raw)
To: Grant Likely; +Cc: alsa-devel, kumar.gala, broonie, linuxppc-dev, Liam Girdwood
In-Reply-To: <j2zfa686aa41004280657g49db18d9g563e643e16ca64e9@mail.gmail.com>
On Wed, Apr 28, 2010 at 8:57 AM, Grant Likely <grant.likely@secretlab.ca> w=
rote:
> I'm just saying move the registration of the machine device out of
> arch/powerpc platform code and into the ssi driver.
But the SSI driver is an OF driver, and it gets probed for every SSI
node in the device tree. On the 8610, that means being probed twice.
But I should only call platform_device_register_simple() once.
Are you saying that I should call platform_device_register_simple()
from the SSI's driver initialization function, fsl_ssi_init()?
> Then you've got a
> reasonable place to pass shared data (either the ssi device node or
> device instance or name. =A0Whatever you need) to the machine driver.
The problem is that the fabric driver needs much more information from
the device tree than the SSI driver needs. So if the SSI driver is
going to pass that information to the fabric driver via the platform
data, it's going to have to know what information the fabric driver
needs. Then the SSI driver is not board-independent.
--=20
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: [PATCH v2] powerpc/4xx: Add optional "reset_type" property to control reboot via dts
From: Josh Boyer @ 2010-04-28 14:42 UTC (permalink / raw)
To: Stefan Roese; +Cc: linuxppc-dev
In-Reply-To: <1272442414-8018-1-git-send-email-sr@denx.de>
On Wed, Apr 28, 2010 at 10:13:34AM +0200, Stefan Roese wrote:
>By setting "reset_type" to one of the following values, the default
>software reset mechanism may be overidden. Here the possible values of
>"reset_type":
>
> 1 - PPC4xx core reset
> 2 - PPC4xx chip reset
> 3 - PPC4xx system reset (default)
>
>This will be used by a new PPC440SPe board port, which needs a "chip
>reset" instead of the default "system reset" to be asserted.
>
>Signed-off-by: Stefan Roese <sr@denx.de>
>Cc: Josh Boyer <jwboyer@linux.vnet.ibm.com>
>Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>---
>v2:
>- Add small property description to Documentation
>- Add sanity check for property value
Thanks for the quick respin.
Acked-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>
Ben, I don't have anything else pending for -next at the moment. Can you pick
this up yourself, or do you want me to create a pull request for just this
patch?
josh
>
> Documentation/powerpc/dts-bindings/4xx/reboot.txt | 18 +++++++++++++++
> arch/powerpc/sysdev/ppc4xx_soc.c | 24 +++++++++++++++++++-
> 2 files changed, 40 insertions(+), 2 deletions(-)
> create mode 100644 Documentation/powerpc/dts-bindings/4xx/reboot.txt
>
>diff --git a/Documentation/powerpc/dts-bindings/4xx/reboot.txt b/Documentation/powerpc/dts-bindings/4xx/reboot.txt
>new file mode 100644
>index 0000000..d721726
>--- /dev/null
>+++ b/Documentation/powerpc/dts-bindings/4xx/reboot.txt
>@@ -0,0 +1,18 @@
>+Reboot property to control system reboot on PPC4xx systems:
>+
>+By setting "reset_type" to one of the following values, the default
>+software reset mechanism may be overidden. Here the possible values of
>+"reset_type":
>+
>+ 1 - PPC4xx core reset
>+ 2 - PPC4xx chip reset
>+ 3 - PPC4xx system reset (default)
>+
>+Example:
>+
>+ cpu@0 {
>+ device_type = "cpu";
>+ model = "PowerPC,440SPe";
>+ ...
>+ reset-type = <2>; /* Use chip-reset */
>+ };
>diff --git a/arch/powerpc/sysdev/ppc4xx_soc.c b/arch/powerpc/sysdev/ppc4xx_soc.c
>index 5c01435..d3d6ce3 100644
>--- a/arch/powerpc/sysdev/ppc4xx_soc.c
>+++ b/arch/powerpc/sysdev/ppc4xx_soc.c
>@@ -191,11 +191,31 @@ static int __init ppc4xx_l2c_probe(void)
> arch_initcall(ppc4xx_l2c_probe);
>
> /*
>- * At present, this routine just applies a system reset.
>+ * Apply a system reset. Alternatively a board specific value may be
>+ * provided via the "reset-type" property in the cpu node.
> */
> void ppc4xx_reset_system(char *cmd)
> {
>- mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) | DBCR0_RST_SYSTEM);
>+ struct device_node *np;
>+ u32 reset_type = DBCR0_RST_SYSTEM;
>+ const u32 *prop;
>+
>+ np = of_find_node_by_type(NULL, "cpu");
>+ if (np) {
>+ prop = of_get_property(np, "reset-type", NULL);
>+
>+ /*
>+ * Check if property exists and if it is in range:
>+ * 1 - PPC4xx core reset
>+ * 2 - PPC4xx chip reset
>+ * 3 - PPC4xx system reset (default)
>+ */
>+ if ((prop) && ((prop[0] >= 1) && (prop[0] <= 3)))
>+ reset_type = prop[0] << 28;
>+ }
>+
>+ mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) | reset_type);
>+
> while (1)
> ; /* Just in case the reset doesn't work */
> }
>--
>1.7.1
>
^ permalink raw reply
* Re: [alsa-devel] [PATCH 1/2] powerpc: add platform registration for ALSA SoC drivers
From: Grant Likely @ 2010-04-28 13:57 UTC (permalink / raw)
To: Timur Tabi; +Cc: alsa-devel, kumar.gala, broonie, linuxppc-dev, Liam Girdwood
In-Reply-To: <u2xed82fe3e1004280635vaeb792d2xf86996cac299ba2b@mail.gmail.com>
On Wed, Apr 28, 2010 at 7:35 AM, Timur Tabi <timur@freescale.com> wrote:
> On Wed, Apr 28, 2010 at 12:37 AM, Grant Likely
> <grant.likely@secretlab.ca> wrote:
>
>> Why not? =A0Just have the ssi driver probe routine register the fabric
>> device based on the existence of the codec-handle property. =A0It is the
>> best way to go about things with the data that you've got available,
>> and it is no big deal. =A0The relevant fabric driver can then bind
>> against that. =A0You should probably also stuff the ssi device node
>> pointer into the fabric device of_node pointer.
>
> And then where do I put the board-specific initialization code that's
> currently in the fabric driver? =A0The programming information for that
> initialization is not in the device tree.
In the fabric driver; where it is right now. I'm saying *instantiate*
the device when the ssi driver gets probed. Use the top level board
name when assigning the name so that the correct asoc machine driver
gets bound to it.
> It sounds to me like you're saying I should take all the code from the
> fabric driver and shove it into the SSI driver, just so that I can
> avoid instantiating a platform driver.
Nope.
> Keep in mind that asoc likes to have a different struct device for the
> fabric driver and the SSI nodes, so I would need to manually create a
> struct device for the fabric device anyway.
You can do it this way too, but this is not what I'm saying.
>> Linux struct device registrations are cheap, and every struct device
>> has a device_node pointer available. =A0It is totally fine to have both
>> the ssi device and the fabric device point to the same device node if
>> that helps solve your problem of finding references to the right
>> things in each driver. =A0(Just as long as only one of them is an
>> of_platform driver).
>
> But I already have it set up like that. =A0The SSI driver is an OF
> driver, and the fabric driver is a platform driver. =A0I might be able
> to move some code from the fabric driver into the SSI driver to make
> it the fabric driver less obnoxious about scanning the device tree.
I'm just saying move the registration of the machine device out of
arch/powerpc platform code and into the ssi driver. Then you've got a
reasonable place to pass shared data (either the ssi device node or
device instance or name. Whatever you need) to the machine driver.
g.
^ permalink raw reply
* Re: [PATCH 1/3] video: add support for getting video mode from device tree
From: Anatolij Gustschin @ 2010-04-28 13:43 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: linux-fbdev, wd, dzu, devicetree-discuss, linuxppc-dev,
Mitch Bradley, yorksun
In-Reply-To: <1267415120.23523.1898.camel@pasglop>
On Mon, 01 Mar 2010 14:45:20 +1100
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> On Sat, 2010-02-27 at 22:44 -1000, Mitch Bradley wrote:
>
> > As it turns out, I'm doing exactly that - exporting verbatim EDID
> > data
> > as the value of the "edid" property - for the display node on the Via
> > version of the OLPC machine. The kernel driver uses it instead of
> > trying to obtain the EDID data from the monitor, because the builtin
> > OLPC display cannot supply EDID data through the usual hardware
> > interfaces.
>
> This is actually a common practice (though EDID is most often in
> uppercase) on Apple hardware too. It has issues though in the sense that
> it doesn't carry proper connector information and falls over in many
> multi-head cases.
>
> I think passing the EDID data, when available, is thus the right thing
> to do indeed, however that doesn't solve two problems:
>
> - Where to put that property ? This is a complicated problem and we
> might argue on a binding for weeks because video cards typically support
> multiple outputs, etc. etc... I think the best for now is to stick as
> closely as possible to the existing OF fb binding, and have "display"
> nodes for each output, which can eventually contain an EDID. We might
> also want to add a string that indicate the connector type. Specific
> drivers might want to define additional properties here where it makes
> sense such as binding of those outputs to CRTCs or such, up to you.
Putting EDID to display node would be really sufficient for LCDs in
our case. Other systems might define this additional connector type
property.
>
> - We -also- want a way to specify the "default" mode as set by the
> firmware, at least on some devices. The EDID gives capabilities, and
> often for LCDs also the "preferred" mode which is almost always the
> "default" mode ... but could be different. In order to avoid "flicking",
> the driver might wants to know what is the currently programmed mode.
> For that, having split out properties makes sense, though I would like
> to either prefix them all with "mode," or stick them in a sub-node of
> the display@.
I would propose defining following properties in the case the
programmed mode is different from "default" mode:
display@...{
compatible = "<vendor>,<model>"
EDID = [edid-data];
current-mode {
pixel_clock = <value>;
horizontal_active = <value>;
horizontal_blank = <value>;
vertical_active = <value>;
vertical_blank = <value>;
horizontal_active = <value>;
hsync_offset = <value>;
hsync_pulse_width = <value>;
vsync_offset = <value>;
vsync_pulse_width = <value>;
hsync_positive;
vsync_positive;
}
};
The firmware can set the "default" mode using the EDID's preferred
Detailed Timing Descriptor data. If on some devices it sets another
mode than the preferred mode, then the firmware can insert a
"current-mode" sub-node with currently programmed mode. The driver
can check for this sub-node and use it's data and if it isn't present,
it can use the preferred timing data from EDID. The names of the
properties here are actually what Detailed Timing Descriptor in EDID
specifies. What do you think?
Thanks,
Anatolij
^ permalink raw reply
* Re: [alsa-devel] [PATCH 1/2] powerpc: add platform registration for ALSA SoC drivers
From: Mark Brown @ 2010-04-28 13:39 UTC (permalink / raw)
To: Timur Tabi; +Cc: alsa-devel, kumar.gala, linuxppc-dev, Liam Girdwood
In-Reply-To: <r2oed82fe3e1004280619j29ba990fr9d0fc5b3ffa450f8@mail.gmail.com>
On Wed, Apr 28, 2010 at 08:19:00AM -0500, Timur Tabi wrote:
> On Tue, Apr 27, 2010 at 5:29 PM, Mark Brown
> > You also want to be representing unused pins here.
> If they're unused, how do I represent them? Can you give me an example?
You should arrange for something to call snd_soc_dapm_nc_pin() on any
unconnected pins.
^ 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