* RE: [PATCH V4 3/3] powerpc/fsl-pci: Unify pci/pcie initialization code
From: Jia Hongtao-B38951 @ 2012-08-03 2:20 UTC (permalink / raw)
To: Wood Scott-B07421; +Cc: linuxppc-dev@lists.ozlabs.org, Li Yang-R58472
In-Reply-To: <501AE0A2.4010809@freescale.com>
DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogV29vZCBTY290dC1CMDc0
MjENCj4gU2VudDogRnJpZGF5LCBBdWd1c3QgMDMsIDIwMTIgNDoxOSBBTQ0KPiBUbzogSmlhIEhv
bmd0YW8tQjM4OTUxDQo+IENjOiBsaW51eHBwYy1kZXZAbGlzdHMub3psYWJzLm9yZzsgZ2FsYWtA
a2VybmVsLmNyYXNoaW5nLm9yZzsgV29vZCBTY290dC0NCj4gQjA3NDIxOyBMaSBZYW5nLVI1ODQ3
Mg0KPiBTdWJqZWN0OiBSZTogW1BBVENIIFY0IDMvM10gcG93ZXJwYy9mc2wtcGNpOiBVbmlmeSBw
Y2kvcGNpZQ0KPiBpbml0aWFsaXphdGlvbiBjb2RlDQo+IA0KPiBPbiAwOC8wMi8yMDEyIDA2OjQy
IEFNLCBKaWEgSG9uZ3RhbyB3cm90ZToNCj4gPiBXZSB1bmlmaWVkIHRoZSBGcmVlc2NhbGUgcGNp
L3BjaWUgaW5pdGlhbGl6YXRpb24gYnkgY2hhbmdpbmcgdGhlDQo+IGZzbF9wY2kNCj4gPiB0byBh
IHBsYXRmb3JtIGRyaXZlci4gSW4gcHJldmlvdXMgUENJIGNvZGUgYXJjaGl0ZWN0dXJlIHRoZQ0K
PiBpbml0aWFsaXphdGlvbg0KPiA+IHJvdXRpbmUgaXMgY2FsbGVkIGF0IGJvYXJkX3NldHVwX2Fy
Y2ggc3RhZ2UuIE5vdyB0aGUgaW5pdGlhbGl6YXRpb24gaXMNCj4gZG9uZQ0KPiA+IGluIHByb2Jl
IGZ1bmN0aW9uIHdoaWNoIGlzIGFyY2hpdGVjdHVyYWwgYmV0dGVyLiBBbHNvIEl0J3MgY29udmVu
aWVudA0KPiBmb3INCj4gPiBhZGRpbmcgUE0gc3VwcG9ydCBmb3IgUENJIGNvbnRyb2xsZXIgaW4g
bGF0ZXIgcGF0Y2guDQo+ID4NCj4gPiBOb3cgd2UgcmVnaXN0ZXJlZCBwY2kgY29udHJvbGxlcnMg
YXMgcGxhdGZvcm0gZGV2aWNlcy4gU28gd2UgY29tYmluZQ0KPiB0d28NCj4gPiBpbml0aWFsaXph
dGlvbiBjb2RlIGFzIG9uZSBwbGF0Zm9ybSBkcml2ZXIuDQo+ID4NCj4gPiBTaWduZWQtb2ZmLWJ5
OiBKaWEgSG9uZ3RhbyA8QjM4OTUxQGZyZWVzY2FsZS5jb20+DQo+ID4gU2lnbmVkLW9mZi1ieTog
TGkgWWFuZyA8bGVvbGlAZnJlZXNjYWxlLmNvbT4NCj4gPiBTaWduZWQtb2ZmLWJ5OiBDaHVuaGUg
TGFuIDxDaHVuaGUuTGFuQGZyZWVzY2FsZS5jb20+DQo+ID4gLS0tDQo+ID4gIGFyY2gvcG93ZXJw
Yy9wbGF0Zm9ybXMvODV4eC9tcGM4NXh4X2RzLmMgfCAgIDMyICsrLS0tLS0tLS0NCj4gPiAgYXJj
aC9wb3dlcnBjL3N5c2Rldi9mc2xfcGNpLmMgICAgICAgICAgICB8ICAxMDIgKysrKysrKysrKysr
KysrKysrLS0tLQ0KPiAtLS0tLS0tDQo+ID4gIGFyY2gvcG93ZXJwYy9zeXNkZXYvZnNsX3BjaS5o
ICAgICAgICAgICAgfCAgICA2ICstDQo+ID4gIGRyaXZlcnMvZWRhYy9tcGM4NXh4X2VkYWMuYyAg
ICAgICAgICAgICAgfCAgIDQzICsrKystLS0tLS0tLS0NCj4gPiAgNCBmaWxlcyBjaGFuZ2VkLCA4
MyBpbnNlcnRpb25zKCspLCAxMDAgZGVsZXRpb25zKC0pDQo+ID4NCj4gPiBkaWZmIC0tZ2l0IGEv
YXJjaC9wb3dlcnBjL3BsYXRmb3Jtcy84NXh4L21wYzg1eHhfZHMuYw0KPiBiL2FyY2gvcG93ZXJw
Yy9wbGF0Zm9ybXMvODV4eC9tcGM4NXh4X2RzLmMNCj4gPiBpbmRleCA1NmY4YzhmLi5mMmM3YjFj
IDEwMDY0NA0KPiA+IC0tLSBhL2FyY2gvcG93ZXJwYy9wbGF0Zm9ybXMvODV4eC9tcGM4NXh4X2Rz
LmMNCj4gPiArKysgYi9hcmNoL3Bvd2VycGMvcGxhdGZvcm1zLzg1eHgvbXBjODV4eF9kcy5jDQo+
ID4gQEAgLTIwLDcgKzIwLDYgQEANCj4gPiAgI2luY2x1ZGUgPGxpbnV4L3NlcV9maWxlLmg+DQo+
ID4gICNpbmNsdWRlIDxsaW51eC9pbnRlcnJ1cHQuaD4NCj4gPiAgI2luY2x1ZGUgPGxpbnV4L29m
X3BsYXRmb3JtLmg+DQo+ID4gLSNpbmNsdWRlIDxsaW51eC9tZW1ibG9jay5oPg0KPiA+DQo+ID4g
ICNpbmNsdWRlIDxhc20vdGltZS5oPg0KPiA+ICAjaW5jbHVkZSA8YXNtL21hY2hkZXAuaD4NCj4g
PiBAQCAtMTE3LDQwICsxMTYsMTYgQEAgdm9pZCBfX2luaXQgbXBjODV4eF9kc19waWNfaW5pdCh2
b2lkKQ0KPiA+ICBleHRlcm4gaW50IHVsaV9leGNsdWRlX2RldmljZShzdHJ1Y3QgcGNpX2NvbnRy
b2xsZXIgKmhvc2UsDQo+ID4gIAkJCQl1X2NoYXIgYnVzLCB1X2NoYXIgZGV2Zm4pOw0KPiA+DQo+
ID4gLXN0YXRpYyBzdHJ1Y3QgZGV2aWNlX25vZGUgKnBjaV93aXRoX3VsaTsNCj4gPiAtDQo+ID4g
IHN0YXRpYyBpbnQgbXBjODV4eF9leGNsdWRlX2RldmljZShzdHJ1Y3QgcGNpX2NvbnRyb2xsZXIg
Kmhvc2UsDQo+ID4gIAkJCQkgICB1X2NoYXIgYnVzLCB1X2NoYXIgZGV2Zm4pDQo+ID4gIHsNCj4g
PiAtCWlmIChob3NlLT5kbiA9PSBwY2lfd2l0aF91bGkpDQo+ID4gKwlpZiAoaG9zZS0+ZG4gPT0g
ZnNsX3BjaV9wcmltYXJ5KQ0KPiA+ICAJCXJldHVybiB1bGlfZXhjbHVkZV9kZXZpY2UoaG9zZSwg
YnVzLCBkZXZmbik7DQo+ID4NCj4gPiAgCXJldHVybiBQQ0lCSU9TX1NVQ0NFU1NGVUw7DQo+ID4g
IH0NCj4gPiAgI2VuZGlmCS8qIENPTkZJR19QQ0kgKi8NCj4gPg0KPiA+IC1zdGF0aWMgdm9pZCBf
X2luaXQgbXBjODV4eF9kc19wY2lfaW5pdCh2b2lkKQ0KPiA+IC17DQo+ID4gLSNpZmRlZiBDT05G
SUdfUENJDQo+ID4gLQlzdHJ1Y3QgZGV2aWNlX25vZGUgKm5vZGU7DQo+ID4gLQ0KPiA+IC0JZnNs
X3BjaV9pbml0KCk7DQo+ID4gLQ0KPiA+IC0JLyogU2VlIGlmIHdlIGhhdmUgYSBVTEkgdW5kZXIg
dGhlIHByaW1hcnkgKi8NCj4gPiAtDQo+ID4gLQlub2RlID0gb2ZfZmluZF9ub2RlX2J5X25hbWUo
TlVMTCwgInVsaTE1NzUiKTsNCj4gPiAtCXdoaWxlICgocGNpX3dpdGhfdWxpID0gb2ZfZ2V0X3Bh
cmVudChub2RlKSkpIHsNCj4gPiAtCQlvZl9ub2RlX3B1dChub2RlKTsNCj4gPiAtCQlub2RlID0g
cGNpX3dpdGhfdWxpOw0KPiA+IC0NCj4gPiAtCQlpZiAocGNpX3dpdGhfdWxpID09IGZzbF9wY2lf
cHJpbWFyeSkgew0KPiA+IC0JCQlwcGNfbWQucGNpX2V4Y2x1ZGVfZGV2aWNlID0gbXBjODV4eF9l
eGNsdWRlX2RldmljZTsNCj4gPiAtCQkJYnJlYWs7DQo+ID4gLQkJfQ0KPiA+IC0JfQ0KPiA+IC0j
ZW5kaWYNCj4gPiAtfQ0KPiA+IC0NCj4gPiAgLyoNCj4gPiAgICogU2V0dXAgdGhlIGFyY2hpdGVj
dHVyZQ0KPiA+ICAgKi8NCj4gPiBAQCAtMTU5LDggKzEzNCwxMSBAQCBzdGF0aWMgdm9pZCBfX2lu
aXQgbXBjODV4eF9kc19zZXR1cF9hcmNoKHZvaWQpDQo+ID4gIAlpZiAocHBjX21kLnByb2dyZXNz
KQ0KPiA+ICAJCXBwY19tZC5wcm9ncmVzcygibXBjODV4eF9kc19zZXR1cF9hcmNoKCkiLCAwKTsN
Cj4gPg0KPiA+ICsjaWZkZWYgQ09ORklHX1BDSQ0KPiA+ICsJcHBjX21kLnBjaV9leGNsdWRlX2Rl
dmljZSA9IG1wYzg1eHhfZXhjbHVkZV9kZXZpY2U7DQo+ID4gKyNlbmRpZg0KPiANCj4gV2h5IGFy
ZSB5b3UgZWxpbWluYXRpbmcgdGhlIHVsaSBsb29rdXA/ICBXZSBkb24ndCB3YW50IHRvIGNhbGwN
Cj4gdWxpX2V4Y2x1ZGVfZGV2aWNlIG9uIGJvYXJkcyB0aGF0IGRvbid0IGhhdmUgYSB1bGkuDQo+
IA0KPiAtU2NvdHQNCg0KSSBmb3VuZCBvdXQgdGhhdCBhbGwgODV4eF9kcyBib2FyZHMgKG1wYzg1
NzJkcywgbXBjODU0NGRzLCBwMjAyMGRzKSBoYXZlDQpVTEkuIEFsc28gaW4gcGxhdGZvcm0gZHJp
dmVyIGZzbF9wY2lfcHJpbWFyeSBpcyBkZXRlcm1pbmVkIGF0IGFyY2hfaW5pdGNhbGwNCndoaWNo
IG1lYW5zIGF0IHRoZSBzdGFnZSBvZiBib2FyZF9zZXR1cF9hcmNoIGZzbF9wY2lfcHJpbWFyeSBp
cyBub3QgcmVhZHkuDQoNCi1Ib25ndGFvLg0KDQo=
^ permalink raw reply
* Re: [PATCH V4 3/3] powerpc/fsl-pci: Unify pci/pcie initialization code
From: Scott Wood @ 2012-08-02 20:18 UTC (permalink / raw)
To: Jia Hongtao; +Cc: B07421, linuxppc-dev
In-Reply-To: <1343907741-20589-4-git-send-email-B38951@freescale.com>
On 08/02/2012 06:42 AM, Jia Hongtao wrote:
> We unified the Freescale pci/pcie initialization by changing the fsl_pci
> to a platform driver. In previous PCI code architecture the initialization
> routine is called at board_setup_arch stage. Now the initialization is done
> in probe function which is architectural better. Also It's convenient for
> adding PM support for PCI controller in later patch.
>
> Now we registered pci controllers as platform devices. So we combine two
> initialization code as one platform driver.
>
> Signed-off-by: Jia Hongtao <B38951@freescale.com>
> Signed-off-by: Li Yang <leoli@freescale.com>
> Signed-off-by: Chunhe Lan <Chunhe.Lan@freescale.com>
> ---
> arch/powerpc/platforms/85xx/mpc85xx_ds.c | 32 ++--------
> arch/powerpc/sysdev/fsl_pci.c | 102 ++++++++++++++++++-----------
> arch/powerpc/sysdev/fsl_pci.h | 6 +-
> drivers/edac/mpc85xx_edac.c | 43 ++++---------
> 4 files changed, 83 insertions(+), 100 deletions(-)
>
> diff --git a/arch/powerpc/platforms/85xx/mpc85xx_ds.c b/arch/powerpc/platforms/85xx/mpc85xx_ds.c
> index 56f8c8f..f2c7b1c 100644
> --- a/arch/powerpc/platforms/85xx/mpc85xx_ds.c
> +++ b/arch/powerpc/platforms/85xx/mpc85xx_ds.c
> @@ -20,7 +20,6 @@
> #include <linux/seq_file.h>
> #include <linux/interrupt.h>
> #include <linux/of_platform.h>
> -#include <linux/memblock.h>
>
> #include <asm/time.h>
> #include <asm/machdep.h>
> @@ -117,40 +116,16 @@ void __init mpc85xx_ds_pic_init(void)
> extern int uli_exclude_device(struct pci_controller *hose,
> u_char bus, u_char devfn);
>
> -static struct device_node *pci_with_uli;
> -
> static int mpc85xx_exclude_device(struct pci_controller *hose,
> u_char bus, u_char devfn)
> {
> - if (hose->dn == pci_with_uli)
> + if (hose->dn == fsl_pci_primary)
> return uli_exclude_device(hose, bus, devfn);
>
> return PCIBIOS_SUCCESSFUL;
> }
> #endif /* CONFIG_PCI */
>
> -static void __init mpc85xx_ds_pci_init(void)
> -{
> -#ifdef CONFIG_PCI
> - struct device_node *node;
> -
> - fsl_pci_init();
> -
> - /* See if we have a ULI under the primary */
> -
> - node = of_find_node_by_name(NULL, "uli1575");
> - while ((pci_with_uli = of_get_parent(node))) {
> - of_node_put(node);
> - node = pci_with_uli;
> -
> - if (pci_with_uli == fsl_pci_primary) {
> - ppc_md.pci_exclude_device = mpc85xx_exclude_device;
> - break;
> - }
> - }
> -#endif
> -}
> -
> /*
> * Setup the architecture
> */
> @@ -159,8 +134,11 @@ static void __init mpc85xx_ds_setup_arch(void)
> if (ppc_md.progress)
> ppc_md.progress("mpc85xx_ds_setup_arch()", 0);
>
> +#ifdef CONFIG_PCI
> + ppc_md.pci_exclude_device = mpc85xx_exclude_device;
> +#endif
Why are you eliminating the uli lookup? We don't want to call
uli_exclude_device on boards that don't have a uli.
-Scott
^ permalink raw reply
* Re: [PATCH V4 3/3] powerpc/fsl-pci: Unify pci/pcie initialization code
From: Scott Wood @ 2012-08-02 15:59 UTC (permalink / raw)
To: Kumar Gala; +Cc: B07421, linuxppc-dev, Jia Hongtao
In-Reply-To: <A4B0DB85-7B49-4748-8B94-0E1F1B7B663A@kernel.crashing.org>
On 08/02/2012 07:23 AM, Kumar Gala wrote:
> You need to convert all boards to use fsl_pci_init before this patch. Otherwise we'll end up with PCI getting initialized twice on boards.
Alternatively, don't make fsl_pci_init an initcall until all boards have
been converted.
-Scott
^ permalink raw reply
* Re: [PATCH v5 3/6] fsl-dma: change release process of dma descriptor for supporting async_tx
From: Ira W. Snyder @ 2012-08-02 15:55 UTC (permalink / raw)
To: Liu Qiang-B32616
Cc: herbert@gondor.apana.org.au, Vinod Koul,
linux-kernel@vger.kernel.org, dan.j.williams@gmail.com,
linux-crypto@vger.kernel.org, Dan Williams,
linuxppc-dev@lists.ozlabs.org, davem@davemloft.net
In-Reply-To: <BCB48C05FCE8BC4D9E61E841ECBE6DB70D2E04@039-SN2MPN1-011.039d.mgd.msft.net>
On Thu, Aug 02, 2012 at 07:21:51AM +0000, Liu Qiang-B32616 wrote:
> > -----Original Message-----
> > From: Ira W. Snyder [mailto:iws@ovro.caltech.edu]
> > Sent: Thursday, August 02, 2012 1:25 AM
> > To: Liu Qiang-B32616
> > Cc: linux-crypto@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-
> > kernel@vger.kernel.org; dan.j.williams@gmail.com; Vinod Koul;
> > herbert@gondor.hengli.com.au; Dan Williams; davem@davemloft.net
> > Subject: Re: [PATCH v5 3/6] fsl-dma: change release process of dma
> > descriptor for supporting async_tx
> >
> > On Wed, Aug 01, 2012 at 04:49:17PM +0800, qiang.liu@freescale.com wrote:
> > > From: Qiang Liu <qiang.liu@freescale.com>
> > >
> > > Fix the potential risk when enable config NET_DMA and ASYNC_TX.
> > > Async_tx is lack of support in current release process of dma
> > > descriptor, all descriptors will be released whatever is acked or
> > > no-acked by async_tx, so there is a potential race condition when dma
> > > engine is uesd by others clients (e.g. when enable NET_DMA to offload
> > TCP).
> > >
> > > In our case, a race condition which is raised when use both of talitos
> > > and dmaengine to offload xor is because napi scheduler will sync all
> > > pending requests in dma channels, it affects the process of raid
> > > operations due to ack_tx is not checked in fsl dma. The no-acked
> > > descriptor is freed which is submitted just now, as a dependent tx,
> > > this freed descriptor trigger
> > > BUG_ON(async_tx_test_ack(depend_tx)) in async_tx_submit().
> > >
> > > TASK = ee1a94a0[1390] 'md0_raid5' THREAD: ecf40000 CPU: 0
> > > GPR00: 00000001 ecf41ca0 ee44/921a94a0 0000003f 00000001 c00593e4
> > > 00000000 00000001
> > > GPR08: 00000000 a7a7a7a7 00000001 045/920000002 42028042 100a38d4
> > > ed576d98 00000000
> > > GPR16: ed5a11b0 00000000 2b162000 00000200 046/920000000 2d555000
> > > ed3015e8 c15a7aa0
> > > GPR24: 00000000 c155fc40 00000000 ecb63220 ecf41d28 e47/92f640bb0
> > > ef640c30 ecf41ca0 NIP [c02b048c] async_tx_submit+0x6c/0x2b4 LR
> > > [c02b068c] async_tx_submit+0x26c/0x2b4 Call Trace:
> > > [ecf41ca0] [c02b068c] async_tx_submit+0x26c/0x2b448/92 (unreliable)
> > > [ecf41cd0] [c02b0a4c] async_memcpy+0x240/0x25c [ecf41d20] [c0421064]
> > > async_copy_data+0xa0/0x17c [ecf41d70] [c0421cf4]
> > > __raid_run_ops+0x874/0xe10 [ecf41df0] [c0426ee4]
> > > handle_stripe+0x820/0x25e8 [ecf41e90] [c0429080] raid5d+0x3d4/0x5b4
> > > [ecf41f40] [c04329b8] md_thread+0x138/0x16c [ecf41f90] [c008277c]
> > > kthread+0x8c/0x90 [ecf41ff0] [c0011630] kernel_thread+0x4c/0x68
> > >
> > > Another major modification in this patch is the change to completed
> > > descriptors, there is a potential risk which caused by exception
> > > interrupt, all descriptors in ld_running list are seemed completed
> > > when an interrupt raised, it works fine under normal condition, but if
> > > there is an exception occured, it cannot work as our excepted.
> > > Hardware should not depend on s/w list, the right way is to read
> > > current descriptor address register to find the last completed
> > > descriptor. If an interrupt is raised by an error, all descriptors in
> > > ld_running should not be seemed finished, or these unfinished
> > descriptors in ld_running will be released wrongly.
> > >
> > > A simple way to reproduce,
> > > Enable dmatest first, then insert some bad descriptors which can
> > > trigger Programming Error interrupts before the good descriptors.
> > > Last, the good descriptors will be freed before they are processsed
> > > because of the exception intrerrupt.
> > >
> > > Note: the bad descriptors are only for simulating an exception
> > interrupt.
> > > This case can illustrate the potential risk in current fsl-dma very
> > well.
> > >
> >
> > I've never managed to trigger a PE (programming error) interrupt on the
> > 83xx hardware. Any time I intentionally caused an error, the hardware
> > wedged itself. The CB (channel busy) bit is stuck high, and cannot be
> > cleared without a hard reset of the board.
> Sorry, the exception indeed will be occurred, actually, the capability DMA_INTERRUPT
> will reproduce the issue under conditions. It will trigger a exception because of
> bad descriptor (length is zero, src and dst are zero, a->b->c->bada->badb->d, we cannot find out which one is really finished in an interrupt tasklet).
> So, we'd better consider this case.
>
> BTW, I have already found out your patch which is used to resolve the issue of dma lock,
> http://lkml.indiana.edu/hypermail/linux/kernel/1103.0/01519.html
>
Ok. I haven't tested bad descriptors since several years ago. I agree
that it can happen, so we should fix it.
> >
> > I agree the "snoop on the hardware" technique works. As far as I can tell,
> > you have implemented the code correctly.
> >
> > The MPC8349EARM.pdf from Freescale indicates that the hardware will halt
> > in response to a programming error, and generate a PE interrupt. See
> > section 12.5.3.3 (pg 568).
> >
> > The driver, as it is written, will never recover from such a condition.
> > Since you are complaining about this situation, do you intend to fix it?
> Frankly, I don't think your patch really can resolve the issue. Now, I understand what problem happen to you, I will follow it.
>
You are correct. My patch does not resolve the issue.
> Yes, you are right, the driver will never recover except reset the board.
> I see your description and I can reproduce it with dmatest on p1022ds with latest kernel, Dmatest with 6 threads, 200,000 iterations per thread several is passed with or without my patch, but dma is locked when up to 300,000 itrerations even though drop my patch.
> Another test on p4080, 8 threads with 1,000,000 iterations per thread is passed with/without my patch.
> I will follow this issue and try to find the root cause, but it should be another topic:)
>
I agree, it can be another topic. We can focus on getting the MD RAID
problems fixed first, and then fix the lockup.
Do you have CONFIG_NET_DMA enabled? I have always set CONFIG_NET_DMA=n,
I do not use it. I wonder if this is a factor.
> Here, I agree with yours most comments, I will merge some functions from your patch, I will send v6 if you agree with my comments. Thanks.
> Please see my comments inline.
>
All of the comments below look good. I look forward to reviewing v6.
Thanks for responding,
Ira
> >
> > > Cc: Dan Williams <dan.j.williams@intel.com>
> > > Cc: Dan Williams <dan.j.williams@gmail.com>
> > > Cc: Vinod Koul <vinod.koul@intel.com>
> > > Cc: Li Yang <leoli@freescale.com>
> > > Cc: Ira W. Snyder <iws@ovro.caltech.edu>
> > > Signed-off-by: Qiang Liu <qiang.liu@freescale.com>
> > > ---
> > > drivers/dma/fsldma.c | 242 +++++++++++++++++++++++++++++++++++-------
> > --------
> > > drivers/dma/fsldma.h | 1 +
> > > 2 files changed, 172 insertions(+), 71 deletions(-)
> > >
> > > diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c index
> > > 4f2f212..87f52c0 100644
> > > --- a/drivers/dma/fsldma.c
> > > +++ b/drivers/dma/fsldma.c
> > > @@ -400,6 +400,125 @@ out_splice:
> > > list_splice_tail_init(&desc->tx_list, &chan->ld_pending); }
> > >
> > > +static void fsldma_cleanup_descriptor(struct fsldma_chan *chan);
> > > +static void fsl_chan_xfer_ld_queue(struct fsldma_chan *chan);
> > > +
> >
> > As noted in my reply to patch 4/6, please swap the order of this patch
> > and the following patch.
> >
> > These lines should not be added or removed in either patch.
> Ok.
>
> >
> > > +/**
> > > + * fsldma_clean_completed_descriptor - free all descriptors which
> > > + * has been completed and acked
> > > + * @chan: Freescale DMA channel
> > > + *
> > > + * This function is used on all completed and acked descriptors.
> > > + * All descriptors should only be freed in this function.
> > > + */
> > > +static int
> > > +fsldma_clean_completed_descriptor(struct fsldma_chan *chan)
> >
> > This should be 'static void'. It does not return an error code.
> >
> Ok.
>
> > > +{
> > > + struct fsl_desc_sw *desc, *_desc;
> > > +
> > > + /* Run the callback for each descriptor, in order */
> > > + list_for_each_entry_safe(desc, _desc, &chan->ld_completed, node) {
> > > +
> > > + if (async_tx_test_ack(&desc->async_tx)) {
> > > + /* Remove from the list of transactions */
> > > + list_del(&desc->node);
> > > +#ifdef FSL_DMA_LD_DEBUG
> > > + chan_dbg(chan, "LD %p free\n", desc); #endif
> > > + dma_pool_free(chan->desc_pool, desc,
> > > + desc->async_tx.phys);
> >
> > This code appears in multiple places in the driver. Please consider
> > adding my patch 3/7 titled "[PATCH 3/7] fsl-dma: add
> > fsl_dma_free_descriptor() to reduce code duplication" to your patch
> > series.
> Accept.
>
> >
> > > + }
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > > + * fsldma_run_tx_complete_actions - cleanup and free a single link
> > > +descriptor
> >
> > This documentation is incorrect. This code NEVER frees a descriptor.
> I will correct it.
>
> >
> > > + * @chan: Freescale DMA channel
> > > + * @desc: descriptor to cleanup and free
> > > + * @cookie: Freescale DMA transaction identifier
> > > + *
> > > + * This function is used on a descriptor which has been executed by
> > > +the DMA
> > > + * controller. It will run any callbacks, submit any dependencies.
> > > + */
> > > +static dma_cookie_t fsldma_run_tx_complete_actions(struct fsl_desc_sw
> > *desc,
> > > + struct fsldma_chan *chan, dma_cookie_t cookie)
> >
> > Please change the parameter order to:
> >
> > static dma_cookie_t fsldma_run_tx_complete_actions(struct fsldma_chan
> > *chan,
> > struct fsl_desc_sw *desc, dma_cookie_t cookie)
> >
> > Every other function in the driver uses this parameter order. Channel
> > comes first, then descriptor.
> >
> My fault, I will correct it.
>
> > > +{
> > > + struct dma_async_tx_descriptor *txd = &desc->async_tx;
> > > + struct device *dev = chan->common.device->dev;
> > > + dma_addr_t src = get_desc_src(chan, desc);
> > > + dma_addr_t dst = get_desc_dst(chan, desc);
> > > + u32 len = get_desc_cnt(chan, desc);
> > > +
> > > + BUG_ON(txd->cookie < 0);
> > > +
> > > + if (txd->cookie > 0) {
> >
> > It will significantly reduce your patch size if you move this if
> > statement to the function which calls this one. I've provided an example
> > down below, in the one place where this code is used.
> My comments as below.
>
> >
> > > + cookie = txd->cookie;
> > > +
> > > + /* Run the link descriptor callback function */
> > > + if (txd->callback) {
> > > +#ifdef FSL_DMA_LD_DEBUG
> > > + chan_dbg(chan, "LD %p callback\n", desc); #endif
> > > + txd->callback(txd->callback_param);
> > > + }
> > > +
> > > + /* Unmap the dst buffer, if requested */
> > > + if (!(txd->flags & DMA_COMPL_SKIP_DEST_UNMAP)) {
> > > + if (txd->flags & DMA_COMPL_DEST_UNMAP_SINGLE)
> > > + dma_unmap_single(dev, dst, len, DMA_FROM_DEVICE);
> > > + else
> > > + dma_unmap_page(dev, dst, len, DMA_FROM_DEVICE);
> > > + }
> > > +
> > > + /* Unmap the src buffer, if requested */
> > > + if (!(txd->flags & DMA_COMPL_SKIP_SRC_UNMAP)) {
> > > + if (txd->flags & DMA_COMPL_SRC_UNMAP_SINGLE)
> > > + dma_unmap_single(dev, src, len, DMA_TO_DEVICE);
> > > + else
> > > + dma_unmap_page(dev, src, len, DMA_TO_DEVICE);
> > > + }
> > > + }
> > > +
> > > + /* Run any dependencies */
> > > + dma_run_dependencies(txd);
> > > +
> > > + return cookie;
> > > +}
> > > +
> > > +/**
> > > + * fsldma_clean_running_descriptor - move the completed descriptor
> > > +from
> > > + * ld_running to ld_completed
> > > + * @chan: Freescale DMA channel
> > > + * @desc: the descriptor which is completed
> > > + *
> > > + * Free the descriptor directly if acked by async_tx api, or move it
> > > +to
> > > + * queue ld_completed.
> > > + */
> > > +static int
> >
> > This code never returns an error code. It should be 'static void'.
> I will correct it.
>
> >
> > > +fsldma_clean_running_descriptor(struct fsldma_chan *chan,
> > > + struct fsl_desc_sw *desc)
> > > +{
> > > + /* Remove from the list of transactions */
> > > + list_del(&desc->node);
> > > + /*
> > > + * the client is allowed to attach dependent operations
> > > + * until 'ack' is set
> > > + */
> > > + if (!async_tx_test_ack(&desc->async_tx)) {
> > > + /*
> > > + * Move this descriptor to the list of descriptors which is
> > > + * completed, but still awaiting the 'ack' bit to be set.
> > > + */
> > > + list_add_tail(&desc->node, &chan->ld_completed);
> > > + return 0;
> > > + }
> > > +
> > > + dma_pool_free(chan->desc_pool, desc, desc->async_tx.phys);
> > > + return 0;
> > > +}
> > > +
> > > static dma_cookie_t fsl_dma_tx_submit(struct dma_async_tx_descriptor
> > > *tx) {
> > > struct fsldma_chan *chan = to_fsl_chan(tx->chan); @@ -534,8 +653,10
> > > @@ static void fsl_dma_free_chan_resources(struct dma_chan *dchan)
> > >
> > > chan_dbg(chan, "free all channel resources\n");
> > > spin_lock_irqsave(&chan->desc_lock, flags);
> > > + fsldma_cleanup_descriptor(chan);
> > > fsldma_free_desc_list(chan, &chan->ld_pending);
> > > fsldma_free_desc_list(chan, &chan->ld_running);
> > > + fsldma_free_desc_list(chan, &chan->ld_completed);
> > > spin_unlock_irqrestore(&chan->desc_lock, flags);
> > >
> > > dma_pool_destroy(chan->desc_pool);
> > > @@ -819,46 +940,53 @@ static int fsl_dma_device_control(struct dma_chan
> > *dchan,
> > > * controller. It will run any callbacks, submit any dependencies, and
> > then
> > > * free the descriptor.
> > > */
> >
> > This documentation is now wrong. This function no longer operates on a
> > single descriptor. It operates on all descriptors in ld_running and
> > ld_completed.
> >
> > Please fix the documentation, and add locking notes.
> No, it only frees one descriptor.
>
> >
> > > -static void fsldma_cleanup_descriptor(struct fsldma_chan *chan,
> > > - struct fsl_desc_sw *desc)
> > > +static void fsldma_cleanup_descriptor(struct fsldma_chan *chan)
> >
> > I think the name should change to fsldma_cleanup_descriptors(). It cleans
> > up one or more descriptors now.
> It only frees one descriptor as its designed.
>
> >
> > > {
> > > - struct dma_async_tx_descriptor *txd = &desc->async_tx;
> > > - struct device *dev = chan->common.device->dev;
> > > - dma_addr_t src = get_desc_src(chan, desc);
> > > - dma_addr_t dst = get_desc_dst(chan, desc);
> > > - u32 len = get_desc_cnt(chan, desc);
> > > + struct fsl_desc_sw *desc, *_desc;
> > > + dma_cookie_t cookie = 0;
> > > + dma_addr_t curr_phys = get_cdar(chan);
> > > + int idle = dma_is_idle(chan);
> > > + int seen_current = 0;
> > >
> >
> > The hardware can advance quite a bit between here, where you save the
> > current descriptor address and idle status.
> >
> > > - /* Run the link descriptor callback function */
> > > - if (txd->callback) {
> > > -#ifdef FSL_DMA_LD_DEBUG
> > > - chan_dbg(chan, "LD %p callback\n", desc);
> > > -#endif
> > > - txd->callback(txd->callback_param);
> > > - }
> > > + fsldma_clean_completed_descriptor(chan);
> > >
> > > - /* Run any dependencies */
> > > - dma_run_dependencies(txd);
> > > + /* Run the callback for each descriptor, in order */
> > > + list_for_each_entry_safe(desc, _desc, &chan->ld_running, node) {
> > > + /*
> > > + * do not advance past the current descriptor loaded into the
> > > + * hardware channel, subsequent descriptors are either in
> > > + * process or have not been submitted
> > > + */
> > > + if (seen_current)
> > > + break;
> > >
> > > - /* Unmap the dst buffer, if requested */
> > > - if (!(txd->flags & DMA_COMPL_SKIP_DEST_UNMAP)) {
> > > - if (txd->flags & DMA_COMPL_DEST_UNMAP_SINGLE)
> > > - dma_unmap_single(dev, dst, len, DMA_FROM_DEVICE);
> > > - else
> > > - dma_unmap_page(dev, dst, len, DMA_FROM_DEVICE);
> > > - }
> > > + /*
> > > + * stop the search if we reach the current descriptor and the
> > > + * channel is busy
> > > + */
> > > + if (desc->async_tx.phys == curr_phys) {
> > > + seen_current = 1;
> > > + if (!idle)
> > > + break;
> > > + }
> >
> > And here, where you check the current descriptor address and idle status.
> >
> > Should this change to:
> >
> > if (desc->async_tx.phys == get_cdar(chan)) {
> > seen_current = 1;
> > if (!dma_is_idle(chan))
> > break;
> > }
> Ok, I will use your code here.
>
> >
> > > +
> > > + cookie = fsldma_run_tx_complete_actions(desc, chan, cookie);
> > > +
> >
> > I would prefer if the code just kept track of the cookie here, rather
> > than passing it through this function call. This code also illustrates
> > how you can remove the "if (txd->cookie > 0)" check from
> > fsldma_run_tx_complete_actions() to reduce the patch size.
> >
> I cannot agree with you, patch size is important, but program readable is also important.
> My reason as below,
> According to my knowledge, cookie is used to judge whether this descriptor is finished, if it is zero, it means we didn't assign a value for it. We should keep it original meaning for clear?
> Second, I think we should not set a complex process to free descriptor, especially according to different state of the descriptor, the interface should be seemed more reusable and common.
> Last, I don't want to see the interface is coupled too many functions. It's easier extended for future.
> How's your thinking? Of course, your implement is also ok.
>
> > /*
> > * Only descriptors with non-zero cookies need their completion
> > * actions run.
> > */
> > if (desc->async_tx.cookie > 0) {
> > cookie = desc->async_tx.cookie;
> > fsldma_run_tx_complete_actions(chan, desc);
> > desc->async_tx.cookie = 0;
> > }
> >
> > /* This descriptor has been ACKed, free it */ if
> > (async_tx_test_ack(&desc->async_tx)) {
> > fsl_dma_free_descriptor(chan, desc);
> > continue;
> > }
> >
> > /*
> > * This descriptor was not ACKed, add it to the ld_completed
> > * list, to be freed after the ACK bit is set.
> > */
> > list_del(&desc->node);
> > list_add_tail(&desc->node, &chan->ld_completed);
> >
> >
> > > + if (fsldma_clean_running_descriptor(chan, desc))
> > > + break;
> > >
> >
> > This if statement will never trigger. fsldma_clean_running_descriptor()
> > only returns 0. It is useless.
> >
> > > - /* Unmap the src buffer, if requested */
> > > - if (!(txd->flags & DMA_COMPL_SKIP_SRC_UNMAP)) {
> > > - if (txd->flags & DMA_COMPL_SRC_UNMAP_SINGLE)
> > > - dma_unmap_single(dev, src, len, DMA_TO_DEVICE);
> > > - else
> > > - dma_unmap_page(dev, src, len, DMA_TO_DEVICE);
> > > }
> > >
> > > -#ifdef FSL_DMA_LD_DEBUG
> > > - chan_dbg(chan, "LD %p free\n", desc);
> > > -#endif
> > > - dma_pool_free(chan->desc_pool, desc, txd->phys);
> > > + /*
> > > + * Start any pending transactions automatically
> > > + *
> > > + * In the ideal case, we keep the DMA controller busy while we go
> > > + * ahead and free the descriptors below.
> > > + */
> > > + fsl_chan_xfer_ld_queue(chan);
> > > +
> > > + if (cookie > 0)
> > > + chan->common.completed_cookie = cookie;
> > > }
> > >
> > > /**
> > > @@ -954,11 +1082,15 @@ static enum dma_status fsl_tx_status(struct
> > dma_chan *dchan,
> > > enum dma_status ret;
> > > unsigned long flags;
> > >
> > > - spin_lock_irqsave(&chan->desc_lock, flags);
> > > ret = dma_cookie_status(dchan, cookie, txstate);
> > > + if (ret == DMA_SUCCESS)
> > > + return ret;
> > > +
> > > + spin_lock_irqsave(&chan->desc_lock, flags);
> > > + fsldma_cleanup_descriptor(chan);
> > > spin_unlock_irqrestore(&chan->desc_lock, flags);
> > >
> > > - return ret;
> > > + return dma_cookie_status(dchan, cookie, txstate);
> > > }
> > >
> > >
> > > /*--------------------------------------------------------------------
> > > --------*/ @@ -1035,52 +1167,19 @@ static irqreturn_t
> > > fsldma_chan_irq(int irq, void *data) static void
> > > dma_do_tasklet(unsigned long data) {
> > > struct fsldma_chan *chan = (struct fsldma_chan *)data;
> > > - struct fsl_desc_sw *desc, *_desc;
> > > - LIST_HEAD(ld_cleanup);
> > > unsigned long flags;
> > >
> > > chan_dbg(chan, "tasklet entry\n");
> > >
> > > spin_lock_irqsave(&chan->desc_lock, flags);
> > >
> > > - /* update the cookie if we have some descriptors to cleanup */
> > > - if (!list_empty(&chan->ld_running)) {
> > > - dma_cookie_t cookie;
> > > -
> > > - desc = to_fsl_desc(chan->ld_running.prev);
> > > - cookie = desc->async_tx.cookie;
> > > - dma_cookie_complete(&desc->async_tx);
> > > -
> > > - chan_dbg(chan, "completed_cookie=%d\n", cookie);
> > > - }
> > > -
> > > - /*
> > > - * move the descriptors to a temporary list so we can drop the lock
> > > - * during the entire cleanup operation
> > > - */
> > > - list_splice_tail_init(&chan->ld_running, &ld_cleanup);
> > > -
> > > /* the hardware is now idle and ready for more */
> > > chan->idle = true;
> > >
> > > - /*
> > > - * Start any pending transactions automatically
> > > - *
> > > - * In the ideal case, we keep the DMA controller busy while we go
> > > - * ahead and free the descriptors below.
> > > - */
> > > - fsl_chan_xfer_ld_queue(chan);
> > > - spin_unlock_irqrestore(&chan->desc_lock, flags);
> > > -
> > > - /* Run the callback for each descriptor, in order */
> > > - list_for_each_entry_safe(desc, _desc, &ld_cleanup, node) {
> > > + /* Run all cleanup for this descriptor */
> >
> > Nitpick. This should be:
> >
> > /* Run cleanup for all descriptors */
> No, this "all" means all operations for one descriptor but not "all descriptors".
>
> >
> > > + fsldma_cleanup_descriptor(chan);
> > >
> > > - /* Remove from the list of transactions */
> > > - list_del(&desc->node);
> > > -
> > > - /* Run all cleanup for this descriptor */
> > > - fsldma_cleanup_descriptor(chan, desc);
> > > - }
> > > + spin_unlock_irqrestore(&chan->desc_lock, flags);
> > >
> > > chan_dbg(chan, "tasklet exit\n");
> > > }
> > > @@ -1262,6 +1361,7 @@ static int __devinit fsl_dma_chan_probe(struct
> > fsldma_device *fdev,
> > > spin_lock_init(&chan->desc_lock);
> > > INIT_LIST_HEAD(&chan->ld_pending);
> > > INIT_LIST_HEAD(&chan->ld_running);
> > > + INIT_LIST_HEAD(&chan->ld_completed);
> > > chan->idle = true;
> > >
> > > chan->common.device = &fdev->common; diff --git
> > > a/drivers/dma/fsldma.h b/drivers/dma/fsldma.h index f5c3879..7ede908
> > > 100644
> > > --- a/drivers/dma/fsldma.h
> > > +++ b/drivers/dma/fsldma.h
> > > @@ -140,6 +140,7 @@ struct fsldma_chan {
> > > spinlock_t desc_lock; /* Descriptor operation lock */
> > > struct list_head ld_pending; /* Link descriptors queue */
> > > struct list_head ld_running; /* Link descriptors queue */
> > > + struct list_head ld_completed; /* Link descriptors queue */
> >
> > It may help to add some documentation here. It would have helped me to
> > review this patch. Something like this:
> >
> > /*
> > * Descriptors which are queued to run, but have not yet been handed
> > * to the hardware for execution
> > */
> > struct list_head ld_pending;
> >
> > /*
> > * Descriptors which are currently being executed by the hardware */
> > struct list_head ld_running;
> >
> > /*
> > * Descriptors which have finished execution by the hardware. These
> > * descriptors have already had their cleanup actions run. They are
> > * waiting for the ACK bit to be set by the async_tx API.
> > */
> > struct list_head ld_completed;
> Ok, I will add these comments. Thanks.
>
> >
> > > struct dma_chan common; /* DMA common channel */
> > > struct dma_pool *desc_pool; /* Descriptors pool */
> > > struct device *dev; /* Channel device */
> > > --
> > > 1.7.5.1
> > >
> > >
> > > _______________________________________________
> > > Linuxppc-dev mailing list
> > > Linuxppc-dev@lists.ozlabs.org
> > > https://lists.ozlabs.org/listinfo/linuxppc-dev
>
>
^ permalink raw reply
* Re: [PATCH V4 2/3] powerpc/swiotlb: Enable at early stage and disable if not necessary
From: Kumar Gala @ 2012-08-02 12:54 UTC (permalink / raw)
To: Jia Hongtao; +Cc: B07421, linuxppc-dev
In-Reply-To: <1343907741-20589-3-git-send-email-B38951@freescale.com>
On Aug 2, 2012, at 6:42 AM, Jia Hongtao wrote:
> Remove the dependency on PCI initialization for SWIOTLB =
initialization.
> So that PCI can be initialized at proper time.
>=20
> SWIOTLB is partly determined by PCI inbound/outbound map which is =
assigned
> in PCI initialization. But swiotlb_init() should be done at the stage =
of
> mem_init() which is much earlier than PCI initialization. So we =
reserve the
> memory for SWIOTLB first and free it if not necessary.
>=20
> All boards are converted to fit this change.
>=20
> Signed-off-by: Jia Hongtao <B38951@freescale.com>
> Signed-off-by: Li Yang <leoli@freescale.com>
> ---
This doesn't seem like it addresses our issue w/regards to not being =
able to map all of memory from PCI.
> arch/powerpc/include/asm/swiotlb.h | 6 ++++++
> arch/powerpc/kernel/dma-swiotlb.c | 20 ++++++++++++++++++++
> arch/powerpc/mm/mem.c | 3 +--
> arch/powerpc/platforms/44x/currituck.c | 10 ++--------
> arch/powerpc/platforms/85xx/mpc85xx_ds.c | 1 +
> arch/powerpc/platforms/85xx/qemu_e500.c | 2 +-
> arch/powerpc/sysdev/fsl_pci.c | 5 +----
> 7 files changed, 32 insertions(+), 15 deletions(-)
Don't we also want to update all these:
arch/powerpc/platforms/85xx/corenet_ds.c: =
ppc_swiotlb_enable =3D 1;
arch/powerpc/platforms/85xx/ge_imp3a.c: ppc_swiotlb_enable =3D =
1;
arch/powerpc/platforms/85xx/mpc8536_ds.c: =
ppc_swiotlb_enable =3D 1;
arch/powerpc/platforms/85xx/mpc85xx_mds.c: =
ppc_swiotlb_enable =3D 1;
arch/powerpc/platforms/85xx/p1022_ds.c: ppc_swiotlb_enable =3D =
1;
arch/powerpc/platforms/86xx/mpc86xx_hpcn.c: =
ppc_swiotlb_enable =3D 1;
>=20
> diff --git a/arch/powerpc/include/asm/swiotlb.h =
b/arch/powerpc/include/asm/swiotlb.h
> index 8979d4c..de99d6e 100644
> --- a/arch/powerpc/include/asm/swiotlb.h
> +++ b/arch/powerpc/include/asm/swiotlb.h
> @@ -22,4 +22,10 @@ int __init swiotlb_setup_bus_notifier(void);
>=20
> extern void pci_dma_dev_setup_swiotlb(struct pci_dev *pdev);
>=20
> +#ifdef CONFIG_SWIOTLB
> +void swiotlb_detect_4g(void);
> +#else
> +static inline void swiotlb_detect_4g(void) {}
> +#endif
> +
> #endif /* __ASM_SWIOTLB_H */
> diff --git a/arch/powerpc/kernel/dma-swiotlb.c =
b/arch/powerpc/kernel/dma-swiotlb.c
> index 4ab88da..aa85550 100644
> --- a/arch/powerpc/kernel/dma-swiotlb.c
> +++ b/arch/powerpc/kernel/dma-swiotlb.c
> @@ -104,3 +104,23 @@ int __init swiotlb_setup_bus_notifier(void)
> &ppc_swiotlb_plat_bus_notifier);
> return 0;
> }
> +
> +void swiotlb_detect_4g(void)
> +{
> + if ((memblock_end_of_DRAM() - 1) > 0xffffffff)
> + ppc_swiotlb_enable =3D 1;
> +}
> +
> +static int __init swiotlb_late_init(void)
> +{
> + if (ppc_swiotlb_enable) {
> + swiotlb_print_info();
> + set_pci_dma_ops(&swiotlb_dma_ops);
> + ppc_md.pci_dma_dev_setup =3D pci_dma_dev_setup_swiotlb;
> + } else {
> + swiotlb_free();
> + }
> +
> + return 0;
> +}
> +subsys_initcall(swiotlb_late_init);
> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
> index baaafde..f23c4e0 100644
> --- a/arch/powerpc/mm/mem.c
> +++ b/arch/powerpc/mm/mem.c
> @@ -300,8 +300,7 @@ void __init mem_init(void)
> unsigned long reservedpages =3D 0, codesize, initsize, datasize, =
bsssize;
>=20
> #ifdef CONFIG_SWIOTLB
> - if (ppc_swiotlb_enable)
> - swiotlb_init(1);
> + swiotlb_init(0);
> #endif
>=20
> num_physpages =3D memblock_phys_mem_size() >> PAGE_SHIFT;
> diff --git a/arch/powerpc/platforms/44x/currituck.c =
b/arch/powerpc/platforms/44x/currituck.c
> index 9f6c33d..6bd89a0 100644
> --- a/arch/powerpc/platforms/44x/currituck.c
> +++ b/arch/powerpc/platforms/44x/currituck.c
> @@ -21,7 +21,6 @@
> */
>=20
> #include <linux/init.h>
> -#include <linux/memblock.h>
> #include <linux/of.h>
> #include <linux/of_platform.h>
> #include <linux/rtc.h>
> @@ -159,13 +158,8 @@ static void __init ppc47x_setup_arch(void)
>=20
> /* No need to check the DMA config as we /know/ our windows are =
all of
> * RAM. Lets hope that doesn't change */
> -#ifdef CONFIG_SWIOTLB
> - if ((memblock_end_of_DRAM() - 1) > 0xffffffff) {
> - ppc_swiotlb_enable =3D 1;
> - set_pci_dma_ops(&swiotlb_dma_ops);
> - ppc_md.pci_dma_dev_setup =3D pci_dma_dev_setup_swiotlb;
> - }
> -#endif
> + swiotlb_detect_4g();
> +
> ppc47x_smp_init();
> }
>=20
> diff --git a/arch/powerpc/platforms/85xx/mpc85xx_ds.c =
b/arch/powerpc/platforms/85xx/mpc85xx_ds.c
> index 6d3265f..56f8c8f 100644
> --- a/arch/powerpc/platforms/85xx/mpc85xx_ds.c
> +++ b/arch/powerpc/platforms/85xx/mpc85xx_ds.c
> @@ -159,6 +159,7 @@ static void __init mpc85xx_ds_setup_arch(void)
> if (ppc_md.progress)
> ppc_md.progress("mpc85xx_ds_setup_arch()", 0);
>=20
> + swiotlb_detect_4g();
> mpc85xx_ds_pci_init();
> mpc85xx_smp_init();
>=20
> diff --git a/arch/powerpc/platforms/85xx/qemu_e500.c =
b/arch/powerpc/platforms/85xx/qemu_e500.c
> index 95a2e53..04260cd 100644
> --- a/arch/powerpc/platforms/85xx/qemu_e500.c
> +++ b/arch/powerpc/platforms/85xx/qemu_e500.c
> @@ -41,7 +41,7 @@ static void __init qemu_e500_setup_arch(void)
> {
> ppc_md.progress("qemu_e500_setup_arch()", 0);
>=20
> - fsl_pci_init();
> + swiotlb_detect_4g();
removing fsl_pci_init() seems wrong.
> mpc85xx_smp_init();
> }
>=20
> diff --git a/arch/powerpc/sysdev/fsl_pci.c =
b/arch/powerpc/sysdev/fsl_pci.c
> index 6938792..da7a3d7 100644
> --- a/arch/powerpc/sysdev/fsl_pci.c
> +++ b/arch/powerpc/sysdev/fsl_pci.c
> @@ -872,11 +872,8 @@ void __devinit fsl_pci_init(void)
> * we need SWIOTLB to handle buffers located outside of
> * dma capable memory region
> */
> - if (memblock_end_of_DRAM() - 1 > max) {
> + if (memblock_end_of_DRAM() - 1 > max)
> ppc_swiotlb_enable =3D 1;
> - set_pci_dma_ops(&swiotlb_dma_ops);
> - ppc_md.pci_dma_dev_setup =3D pci_dma_dev_setup_swiotlb;
> - }
> #endif
> }
> #endif
> --=20
> 1.7.5.1
>=20
^ permalink raw reply
* Re: [PATCH V4 3/3] powerpc/fsl-pci: Unify pci/pcie initialization code
From: Kumar Gala @ 2012-08-02 12:23 UTC (permalink / raw)
To: Jia Hongtao; +Cc: B07421, linuxppc-dev
In-Reply-To: <1343907741-20589-4-git-send-email-B38951@freescale.com>
On Aug 2, 2012, at 6:42 AM, Jia Hongtao wrote:
> We unified the Freescale pci/pcie initialization by changing the =
fsl_pci
> to a platform driver. In previous PCI code architecture the =
initialization
> routine is called at board_setup_arch stage. Now the initialization is =
done
> in probe function which is architectural better. Also It's convenient =
for
> adding PM support for PCI controller in later patch.
>=20
> Now we registered pci controllers as platform devices. So we combine =
two
> initialization code as one platform driver.
>=20
> Signed-off-by: Jia Hongtao <B38951@freescale.com>
> Signed-off-by: Li Yang <leoli@freescale.com>
> Signed-off-by: Chunhe Lan <Chunhe.Lan@freescale.com>
> ---
> arch/powerpc/platforms/85xx/mpc85xx_ds.c | 32 ++--------
> arch/powerpc/sysdev/fsl_pci.c | 102 =
++++++++++++++++++-----------
> arch/powerpc/sysdev/fsl_pci.h | 6 +-
> drivers/edac/mpc85xx_edac.c | 43 ++++---------
> 4 files changed, 83 insertions(+), 100 deletions(-)
You need to convert all boards to use fsl_pci_init before this patch. =
Otherwise we'll end up with PCI getting initialized twice on boards.
- k=
^ permalink raw reply
* [PATCH V4 3/3] powerpc/fsl-pci: Unify pci/pcie initialization code
From: Jia Hongtao @ 2012-08-02 11:42 UTC (permalink / raw)
To: linuxppc-dev, galak; +Cc: B07421, b38951
In-Reply-To: <1343907741-20589-1-git-send-email-B38951@freescale.com>
We unified the Freescale pci/pcie initialization by changing the fsl_pci
to a platform driver. In previous PCI code architecture the initialization
routine is called at board_setup_arch stage. Now the initialization is done
in probe function which is architectural better. Also It's convenient for
adding PM support for PCI controller in later patch.
Now we registered pci controllers as platform devices. So we combine two
initialization code as one platform driver.
Signed-off-by: Jia Hongtao <B38951@freescale.com>
Signed-off-by: Li Yang <leoli@freescale.com>
Signed-off-by: Chunhe Lan <Chunhe.Lan@freescale.com>
---
arch/powerpc/platforms/85xx/mpc85xx_ds.c | 32 ++--------
arch/powerpc/sysdev/fsl_pci.c | 102 ++++++++++++++++++-----------
arch/powerpc/sysdev/fsl_pci.h | 6 +-
drivers/edac/mpc85xx_edac.c | 43 ++++---------
4 files changed, 83 insertions(+), 100 deletions(-)
diff --git a/arch/powerpc/platforms/85xx/mpc85xx_ds.c b/arch/powerpc/platforms/85xx/mpc85xx_ds.c
index 56f8c8f..f2c7b1c 100644
--- a/arch/powerpc/platforms/85xx/mpc85xx_ds.c
+++ b/arch/powerpc/platforms/85xx/mpc85xx_ds.c
@@ -20,7 +20,6 @@
#include <linux/seq_file.h>
#include <linux/interrupt.h>
#include <linux/of_platform.h>
-#include <linux/memblock.h>
#include <asm/time.h>
#include <asm/machdep.h>
@@ -117,40 +116,16 @@ void __init mpc85xx_ds_pic_init(void)
extern int uli_exclude_device(struct pci_controller *hose,
u_char bus, u_char devfn);
-static struct device_node *pci_with_uli;
-
static int mpc85xx_exclude_device(struct pci_controller *hose,
u_char bus, u_char devfn)
{
- if (hose->dn == pci_with_uli)
+ if (hose->dn == fsl_pci_primary)
return uli_exclude_device(hose, bus, devfn);
return PCIBIOS_SUCCESSFUL;
}
#endif /* CONFIG_PCI */
-static void __init mpc85xx_ds_pci_init(void)
-{
-#ifdef CONFIG_PCI
- struct device_node *node;
-
- fsl_pci_init();
-
- /* See if we have a ULI under the primary */
-
- node = of_find_node_by_name(NULL, "uli1575");
- while ((pci_with_uli = of_get_parent(node))) {
- of_node_put(node);
- node = pci_with_uli;
-
- if (pci_with_uli == fsl_pci_primary) {
- ppc_md.pci_exclude_device = mpc85xx_exclude_device;
- break;
- }
- }
-#endif
-}
-
/*
* Setup the architecture
*/
@@ -159,8 +134,11 @@ static void __init mpc85xx_ds_setup_arch(void)
if (ppc_md.progress)
ppc_md.progress("mpc85xx_ds_setup_arch()", 0);
+#ifdef CONFIG_PCI
+ ppc_md.pci_exclude_device = mpc85xx_exclude_device;
+#endif
+
swiotlb_detect_4g();
- mpc85xx_ds_pci_init();
mpc85xx_smp_init();
printk("MPC85xx DS board from Freescale Semiconductor\n");
diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
index da7a3d7..6408d9d 100644
--- a/arch/powerpc/sysdev/fsl_pci.c
+++ b/arch/powerpc/sysdev/fsl_pci.c
@@ -826,54 +826,78 @@ static const struct of_device_id pci_ids[] = {
struct device_node *fsl_pci_primary;
-void __devinit fsl_pci_init(void)
+/* Checkout if PCI contains ISA node */
+static int of_pci_has_isa(struct device_node *pci_node)
+{
+ struct device_node *np;
+ int ret = 0;
+
+ if (!pci_node)
+ return 0;
+
+ read_lock(&devtree_lock);
+ np = pci_node->allnext;
+
+ /* Only scan the children of PCI node */
+ for (; np != pci_node->sibling; np = np->allnext) {
+ if (np->type && (of_node_cmp(np->type, "isa") == 0)
+ && of_node_get(np)) {
+ ret = 1;
+ break;
+ }
+ }
+
+ of_node_put(pci_node);
+ read_unlock(&devtree_lock);
+
+ return ret;
+}
+
+static int __devinit fsl_pci_probe(struct platform_device *pdev)
{
int ret;
- struct device_node *node;
struct pci_controller *hose;
- dma_addr_t max = 0xffffffff;
+ int is_primary = 0;
- /* Callers can specify the primary bus using other means. */
if (!fsl_pci_primary) {
- /* If a PCI host bridge contains an ISA node, it's primary. */
- node = of_find_node_by_type(NULL, "isa");
- while ((fsl_pci_primary = of_get_parent(node))) {
- of_node_put(node);
- node = fsl_pci_primary;
-
- if (of_match_node(pci_ids, node))
- break;
- }
+ is_primary = of_pci_has_isa(pdev->dev.of_node);
+ if (is_primary)
+ fsl_pci_primary = pdev->dev.of_node;
}
- node = NULL;
- for_each_node_by_type(node, "pci") {
- if (of_match_node(pci_ids, node)) {
- /*
- * If there's no PCI host bridge with ISA, arbitrarily
- * designate one as primary. This can go away once
- * various bugs with primary-less systems are fixed.
- */
- if (!fsl_pci_primary)
- fsl_pci_primary = node;
-
- ret = fsl_add_bridge(node, fsl_pci_primary == node);
- if (ret == 0) {
- hose = pci_find_hose_for_OF_device(node);
- max = min(max, hose->dma_window_base_cur +
- hose->dma_window_size);
- }
- }
- }
+ ret = fsl_add_bridge(pdev->dev.of_node, is_primary);
#ifdef CONFIG_SWIOTLB
- /*
- * if we couldn't map all of DRAM via the dma windows
- * we need SWIOTLB to handle buffers located outside of
- * dma capable memory region
- */
- if (memblock_end_of_DRAM() - 1 > max)
- ppc_swiotlb_enable = 1;
+ if (ret == 0) {
+ hose = pci_find_hose_for_OF_device(pdev->dev.of_node);
+
+ /*
+ * if we couldn't map all of DRAM via the dma windows
+ * we need SWIOTLB to handle buffers located outside of
+ * dma capable memory region
+ */
+ if (memblock_end_of_DRAM() - 1 > hose->dma_window_base_cur +
+ hose->dma_window_size)
+ ppc_swiotlb_enable = 1;
+ }
#endif
+
+ mpc85xx_pci_err_probe(pdev);
+
+ return 0;
+}
+
+static struct platform_driver fsl_pci_driver = {
+ .driver = {
+ .name = "fsl-pci",
+ .of_match_table = pci_ids,
+ },
+ .probe = fsl_pci_probe,
+};
+
+static int __init fsl_pci_init(void)
+{
+ return platform_driver_register(&fsl_pci_driver);
}
+arch_initcall(fsl_pci_init);
#endif
diff --git a/arch/powerpc/sysdev/fsl_pci.h b/arch/powerpc/sysdev/fsl_pci.h
index baa0fd1..e3fcc6c 100644
--- a/arch/powerpc/sysdev/fsl_pci.h
+++ b/arch/powerpc/sysdev/fsl_pci.h
@@ -95,10 +95,10 @@ u64 fsl_pci_immrbar_base(struct pci_controller *hose);
extern struct device_node *fsl_pci_primary;
-#ifdef CONFIG_FSL_PCI
-void fsl_pci_init(void);
+#ifdef CONFIG_EDAC_MPC85XX
+int mpc85xx_pci_err_probe(struct platform_device *op);
#else
-static inline void fsl_pci_init(void) {}
+static inline int mpc85xx_pci_err_probe(struct platform_device *op) {}
#endif
#endif /* __POWERPC_FSL_PCI_H */
diff --git a/drivers/edac/mpc85xx_edac.c b/drivers/edac/mpc85xx_edac.c
index 0e37462..2677883 100644
--- a/drivers/edac/mpc85xx_edac.c
+++ b/drivers/edac/mpc85xx_edac.c
@@ -200,7 +200,7 @@ static irqreturn_t mpc85xx_pci_isr(int irq, void *dev_id)
return IRQ_HANDLED;
}
-static int __devinit mpc85xx_pci_err_probe(struct platform_device *op)
+int __devinit mpc85xx_pci_err_probe(struct platform_device *op)
{
struct edac_pci_ctl_info *pci;
struct mpc85xx_pci_pdata *pdata;
@@ -214,6 +214,16 @@ static int __devinit mpc85xx_pci_err_probe(struct platform_device *op)
if (!pci)
return -ENOMEM;
+ /* make sure error reporting method is sane */
+ switch (edac_op_state) {
+ case EDAC_OPSTATE_POLL:
+ case EDAC_OPSTATE_INT:
+ break;
+ default:
+ edac_op_state = EDAC_OPSTATE_INT;
+ break;
+ }
+
pdata = pci->pvt_info;
pdata->name = "mpc85xx_pci_err";
pdata->irq = NO_IRQ;
@@ -303,6 +313,7 @@ err:
devres_release_group(&op->dev, mpc85xx_pci_err_probe);
return res;
}
+EXPORT_SYMBOL_GPL(mpc85xx_pci_err_probe);
static int mpc85xx_pci_err_remove(struct platform_device *op)
{
@@ -326,27 +337,6 @@ static int mpc85xx_pci_err_remove(struct platform_device *op)
return 0;
}
-static struct of_device_id mpc85xx_pci_err_of_match[] = {
- {
- .compatible = "fsl,mpc8540-pcix",
- },
- {
- .compatible = "fsl,mpc8540-pci",
- },
- {},
-};
-MODULE_DEVICE_TABLE(of, mpc85xx_pci_err_of_match);
-
-static struct platform_driver mpc85xx_pci_err_driver = {
- .probe = mpc85xx_pci_err_probe,
- .remove = __devexit_p(mpc85xx_pci_err_remove),
- .driver = {
- .name = "mpc85xx_pci_err",
- .owner = THIS_MODULE,
- .of_match_table = mpc85xx_pci_err_of_match,
- },
-};
-
#endif /* CONFIG_PCI */
/**************************** L2 Err device ***************************/
@@ -1193,12 +1183,6 @@ static int __init mpc85xx_mc_init(void)
if (res)
printk(KERN_WARNING EDAC_MOD_STR "L2 fails to register\n");
-#ifdef CONFIG_PCI
- res = platform_driver_register(&mpc85xx_pci_err_driver);
- if (res)
- printk(KERN_WARNING EDAC_MOD_STR "PCI fails to register\n");
-#endif
-
#ifdef CONFIG_FSL_SOC_BOOKE
pvr = mfspr(SPRN_PVR);
@@ -1235,9 +1219,6 @@ static void __exit mpc85xx_mc_exit(void)
on_each_cpu(mpc85xx_mc_restore_hid1, NULL, 0);
}
#endif
-#ifdef CONFIG_PCI
- platform_driver_unregister(&mpc85xx_pci_err_driver);
-#endif
platform_driver_unregister(&mpc85xx_l2_err_driver);
platform_driver_unregister(&mpc85xx_mc_err_driver);
}
--
1.7.5.1
^ permalink raw reply related
* [PATCH V4 2/3] powerpc/swiotlb: Enable at early stage and disable if not necessary
From: Jia Hongtao @ 2012-08-02 11:42 UTC (permalink / raw)
To: linuxppc-dev, galak; +Cc: B07421, b38951
In-Reply-To: <1343907741-20589-1-git-send-email-B38951@freescale.com>
Remove the dependency on PCI initialization for SWIOTLB initialization.
So that PCI can be initialized at proper time.
SWIOTLB is partly determined by PCI inbound/outbound map which is assigned
in PCI initialization. But swiotlb_init() should be done at the stage of
mem_init() which is much earlier than PCI initialization. So we reserve the
memory for SWIOTLB first and free it if not necessary.
All boards are converted to fit this change.
Signed-off-by: Jia Hongtao <B38951@freescale.com>
Signed-off-by: Li Yang <leoli@freescale.com>
---
arch/powerpc/include/asm/swiotlb.h | 6 ++++++
arch/powerpc/kernel/dma-swiotlb.c | 20 ++++++++++++++++++++
arch/powerpc/mm/mem.c | 3 +--
arch/powerpc/platforms/44x/currituck.c | 10 ++--------
arch/powerpc/platforms/85xx/mpc85xx_ds.c | 1 +
arch/powerpc/platforms/85xx/qemu_e500.c | 2 +-
arch/powerpc/sysdev/fsl_pci.c | 5 +----
7 files changed, 32 insertions(+), 15 deletions(-)
diff --git a/arch/powerpc/include/asm/swiotlb.h b/arch/powerpc/include/asm/swiotlb.h
index 8979d4c..de99d6e 100644
--- a/arch/powerpc/include/asm/swiotlb.h
+++ b/arch/powerpc/include/asm/swiotlb.h
@@ -22,4 +22,10 @@ int __init swiotlb_setup_bus_notifier(void);
extern void pci_dma_dev_setup_swiotlb(struct pci_dev *pdev);
+#ifdef CONFIG_SWIOTLB
+void swiotlb_detect_4g(void);
+#else
+static inline void swiotlb_detect_4g(void) {}
+#endif
+
#endif /* __ASM_SWIOTLB_H */
diff --git a/arch/powerpc/kernel/dma-swiotlb.c b/arch/powerpc/kernel/dma-swiotlb.c
index 4ab88da..aa85550 100644
--- a/arch/powerpc/kernel/dma-swiotlb.c
+++ b/arch/powerpc/kernel/dma-swiotlb.c
@@ -104,3 +104,23 @@ int __init swiotlb_setup_bus_notifier(void)
&ppc_swiotlb_plat_bus_notifier);
return 0;
}
+
+void swiotlb_detect_4g(void)
+{
+ if ((memblock_end_of_DRAM() - 1) > 0xffffffff)
+ ppc_swiotlb_enable = 1;
+}
+
+static int __init swiotlb_late_init(void)
+{
+ if (ppc_swiotlb_enable) {
+ swiotlb_print_info();
+ set_pci_dma_ops(&swiotlb_dma_ops);
+ ppc_md.pci_dma_dev_setup = pci_dma_dev_setup_swiotlb;
+ } else {
+ swiotlb_free();
+ }
+
+ return 0;
+}
+subsys_initcall(swiotlb_late_init);
diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
index baaafde..f23c4e0 100644
--- a/arch/powerpc/mm/mem.c
+++ b/arch/powerpc/mm/mem.c
@@ -300,8 +300,7 @@ void __init mem_init(void)
unsigned long reservedpages = 0, codesize, initsize, datasize, bsssize;
#ifdef CONFIG_SWIOTLB
- if (ppc_swiotlb_enable)
- swiotlb_init(1);
+ swiotlb_init(0);
#endif
num_physpages = memblock_phys_mem_size() >> PAGE_SHIFT;
diff --git a/arch/powerpc/platforms/44x/currituck.c b/arch/powerpc/platforms/44x/currituck.c
index 9f6c33d..6bd89a0 100644
--- a/arch/powerpc/platforms/44x/currituck.c
+++ b/arch/powerpc/platforms/44x/currituck.c
@@ -21,7 +21,6 @@
*/
#include <linux/init.h>
-#include <linux/memblock.h>
#include <linux/of.h>
#include <linux/of_platform.h>
#include <linux/rtc.h>
@@ -159,13 +158,8 @@ static void __init ppc47x_setup_arch(void)
/* No need to check the DMA config as we /know/ our windows are all of
* RAM. Lets hope that doesn't change */
-#ifdef CONFIG_SWIOTLB
- if ((memblock_end_of_DRAM() - 1) > 0xffffffff) {
- ppc_swiotlb_enable = 1;
- set_pci_dma_ops(&swiotlb_dma_ops);
- ppc_md.pci_dma_dev_setup = pci_dma_dev_setup_swiotlb;
- }
-#endif
+ swiotlb_detect_4g();
+
ppc47x_smp_init();
}
diff --git a/arch/powerpc/platforms/85xx/mpc85xx_ds.c b/arch/powerpc/platforms/85xx/mpc85xx_ds.c
index 6d3265f..56f8c8f 100644
--- a/arch/powerpc/platforms/85xx/mpc85xx_ds.c
+++ b/arch/powerpc/platforms/85xx/mpc85xx_ds.c
@@ -159,6 +159,7 @@ static void __init mpc85xx_ds_setup_arch(void)
if (ppc_md.progress)
ppc_md.progress("mpc85xx_ds_setup_arch()", 0);
+ swiotlb_detect_4g();
mpc85xx_ds_pci_init();
mpc85xx_smp_init();
diff --git a/arch/powerpc/platforms/85xx/qemu_e500.c b/arch/powerpc/platforms/85xx/qemu_e500.c
index 95a2e53..04260cd 100644
--- a/arch/powerpc/platforms/85xx/qemu_e500.c
+++ b/arch/powerpc/platforms/85xx/qemu_e500.c
@@ -41,7 +41,7 @@ static void __init qemu_e500_setup_arch(void)
{
ppc_md.progress("qemu_e500_setup_arch()", 0);
- fsl_pci_init();
+ swiotlb_detect_4g();
mpc85xx_smp_init();
}
diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
index 6938792..da7a3d7 100644
--- a/arch/powerpc/sysdev/fsl_pci.c
+++ b/arch/powerpc/sysdev/fsl_pci.c
@@ -872,11 +872,8 @@ void __devinit fsl_pci_init(void)
* we need SWIOTLB to handle buffers located outside of
* dma capable memory region
*/
- if (memblock_end_of_DRAM() - 1 > max) {
+ if (memblock_end_of_DRAM() - 1 > max)
ppc_swiotlb_enable = 1;
- set_pci_dma_ops(&swiotlb_dma_ops);
- ppc_md.pci_dma_dev_setup = pci_dma_dev_setup_swiotlb;
- }
#endif
}
#endif
--
1.7.5.1
^ permalink raw reply related
* [PATCH V4 0/3] PCI patch set description
From: Jia Hongtao @ 2012-08-02 11:42 UTC (permalink / raw)
To: linuxppc-dev, galak; +Cc: B07421, b38951
The first patch fixed a kernel panic when scanning PCI bus if it is working
in agent mode.
In later two patches we unified PCI initialization code by changing fsl_pci
to a platform drvier. The approach will affect swiotlb init and this issue
is addressed in the second patch.
We now just convet IBM 44x, MPC85xxDS and QEMU to this new mechanism. All
other boards will be coverted if this patch set is accepted.
^ permalink raw reply
* [PATCH V4 1/3] powerpc/fsl-pci: Only scan PCI bus if configured as a host
From: Jia Hongtao @ 2012-08-02 11:42 UTC (permalink / raw)
To: linuxppc-dev, galak; +Cc: B07421, b38951
In-Reply-To: <1343907741-20589-1-git-send-email-B38951@freescale.com>
We change fsl_add_bridge to return -ENODEV if the controller is working in
agent mode. Then check the return value of fsl_add_bridge to guarantee
that only successfully added host bus will be scanned.
Signed-off-by: Jia Hongtao <B38951@freescale.com>
Signed-off-by: Li Yang <leoli@freescale.com>
---
arch/powerpc/sysdev/fsl_pci.c | 13 ++++++++-----
1 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
index 50a38b3..6938792 100644
--- a/arch/powerpc/sysdev/fsl_pci.c
+++ b/arch/powerpc/sysdev/fsl_pci.c
@@ -465,7 +465,7 @@ int __init fsl_add_bridge(struct device_node *dev, int is_primary)
iounmap(hose->cfg_data);
iounmap(hose->cfg_addr);
pcibios_free_controller(hose);
- return 0;
+ return -ENODEV;
}
setup_pci_cmd(hose);
@@ -828,6 +828,7 @@ struct device_node *fsl_pci_primary;
void __devinit fsl_pci_init(void)
{
+ int ret;
struct device_node *node;
struct pci_controller *hose;
dma_addr_t max = 0xffffffff;
@@ -856,10 +857,12 @@ void __devinit fsl_pci_init(void)
if (!fsl_pci_primary)
fsl_pci_primary = node;
- fsl_add_bridge(node, fsl_pci_primary == node);
- hose = pci_find_hose_for_OF_device(node);
- max = min(max, hose->dma_window_base_cur +
- hose->dma_window_size);
+ ret = fsl_add_bridge(node, fsl_pci_primary == node);
+ if (ret == 0) {
+ hose = pci_find_hose_for_OF_device(node);
+ max = min(max, hose->dma_window_base_cur +
+ hose->dma_window_size);
+ }
}
}
--
1.7.5.1
^ permalink raw reply related
* RE: [PATCH 5/6] powerpc/fsl-pci: Add pci inbound/outbound PM support
From: Jia Hongtao-B38951 @ 2012-08-02 11:35 UTC (permalink / raw)
To: Kumar Gala
Cc: Wood Scott-B07421, linuxppc-dev@lists.ozlabs.org, Li Yang-R58472
In-Reply-To: <BAB2AC7D-90D8-473A-83A0-59BD6F730E62@kernel.crashing.org>
> -----Original Message-----
> From: Kumar Gala [mailto:galak@kernel.crashing.org]
> Sent: Tuesday, July 31, 2012 9:37 PM
> To: Jia Hongtao-B38951
> Cc: linuxppc-dev@lists.ozlabs.org; Wood Scott-B07421; Li Yang-R58472
> Subject: Re: [PATCH 5/6] powerpc/fsl-pci: Add pci inbound/outbound PM
> support
>=20
>=20
> On Jul 30, 2012, at 1:09 AM, Jia Hongtao-B38951 wrote:
>=20
> >> -----Original Message-----
> >> From: Kumar Gala [mailto:galak@kernel.crashing.org]
> >> Sent: Friday, July 27, 2012 9:24 PM
> >> To: Jia Hongtao-B38951
> >> Cc: linuxppc-dev@lists.ozlabs.org; Wood Scott-B07421; Li Yang-R58472
> >> Subject: Re: [PATCH 5/6] powerpc/fsl-pci: Add pci inbound/outbound PM
> >> support
> >>
> >>
> >> On Jul 24, 2012, at 5:20 AM, Jia Hongtao wrote:
> >>
> >>> Power supply for PCI inbound/outbound window registers is off when
> >> system
> >>> go to deep-sleep state. We save the values of registers before
> suspend
> >>> and restore to registers after resume.
> >>>
> >>> Signed-off-by: Jiang Yutang <b14898@freescale.com>
> >>> Signed-off-by: Jia Hongtao <B38951@freescale.com>
> >>> Signed-off-by: Li Yang <leoli@freescale.com>
> >>> ---
> >>> arch/powerpc/include/asm/pci-bridge.h | 2 +-
> >>> arch/powerpc/sysdev/fsl_pci.c | 121
> >> +++++++++++++++++++++++++++++++++
> >>> 2 files changed, 122 insertions(+), 1 deletions(-)
> >>
> >> Remind me why we need to save/restore PCI ATMUs, why not just re-parse
> >> the device tree to restore?
> >>
> >> - k
> >
> > Save/restore is the more efficient way. Latency of sleep/wakeup is one
> of
> > most important features in power management.
> >
> > -Hongtao.
>=20
> I don't think the time it takes to run through setup_pci_atmu() is that
> long compared to fsl_pci_resume().
>=20
> Also, don't you need to setup PCICCSRBAR and do setup_pci_cmd() on resume=
?
>=20
> - k
I will investigate on this and send the patch later.
-Hongtao.
^ permalink raw reply
* RE: [PATCH v5 3/6] fsl-dma: change release process of dma descriptor for supporting async_tx
From: Liu Qiang-B32616 @ 2012-08-02 11:13 UTC (permalink / raw)
To: Ira W. Snyder
Cc: herbert@gondor.apana.org.au, Vinod Koul,
linux-kernel@vger.kernel.org, dan.j.williams@gmail.com,
linux-crypto@vger.kernel.org, Dan Williams,
linuxppc-dev@lists.ozlabs.org, davem@davemloft.net
In-Reply-To: <20120801172526.GD11359@ovro.caltech.edu>
Hi Ira,
Here, I want to talk about the issue of dma lock when use dmatest with orig=
inal patch.
I do some tests on p1022ds, 2 cores, 6 dma channels (actually is 4 channels=
, I am investigating why is 6, but it doesn't matter). I would like to shar=
e with you to find something:)
First, it is easy to reproduce your issue with 6 channels after up to 200,0=
00 iterations per channel, there is not any response from terminal, then I =
run top to see the 2 cores load, I found cpu load is 100%, terminal won't u=
pdate any info after a long time;
Second, I only enable 4 channels to run dmatest, I found cpu load is 50% pe=
r channel, terminal also update info after a long time;
Last, I only enable 1 channel to run dmatest, serial interrupt is attached =
to core1, and dma channel interrupt is attached to core0, I found terminal =
is very slow after boot up, but it's only slow whatever how many interrupts=
raised by the only dma channel (over 900,000 iterations).
So I want to know, our test results are same? How do you know dma engine is=
self locked but not for heavy cpu load, there is no chance to schedule?
I also read dmatest.c, there is not any sleep to release cpu (msleep(100) i=
f tx is null).
If our test results are same, I think I can explain why your original patch=
can "fix" the issue of dma lock, because your patch avoid mass cpu data op=
eration. 83xx is old, and P1 series is low end chip of Freescale, this mayb=
e the reason why p4080 cannot reproduce this issue.
You can have a test to verify my point if you are available. I will follow =
this issue until it's resolved. I will let you know if any other new clues.
Thanks.
=20
> -----Original Message-----
> From: Ira W. Snyder [mailto:iws@ovro.caltech.edu]
> Sent: Thursday, August 02, 2012 1:25 AM
> To: Liu Qiang-B32616
> Cc: linux-crypto@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-
> kernel@vger.kernel.org; dan.j.williams@gmail.com; Vinod Koul;
> herbert@gondor.hengli.com.au; Dan Williams; davem@davemloft.net
> Subject: Re: [PATCH v5 3/6] fsl-dma: change release process of dma
> descriptor for supporting async_tx
>=20
> On Wed, Aug 01, 2012 at 04:49:17PM +0800, qiang.liu@freescale.com wrote:
> > From: Qiang Liu <qiang.liu@freescale.com>
> >
> > Fix the potential risk when enable config NET_DMA and ASYNC_TX.
> > Async_tx is lack of support in current release process of dma
> descriptor,
> > all descriptors will be released whatever is acked or no-acked by
> async_tx,
> > so there is a potential race condition when dma engine is uesd by
> others
> > clients (e.g. when enable NET_DMA to offload TCP).
> >
> > In our case, a race condition which is raised when use both of talitos
> > and dmaengine to offload xor is because napi scheduler will sync all
> > pending requests in dma channels, it affects the process of raid
> operations
> > due to ack_tx is not checked in fsl dma. The no-acked descriptor is
> freed
> > which is submitted just now, as a dependent tx, this freed descriptor
> trigger
> > BUG_ON(async_tx_test_ack(depend_tx)) in async_tx_submit().
> >
> > TASK =3D ee1a94a0[1390] 'md0_raid5' THREAD: ecf40000 CPU: 0
> > GPR00: 00000001 ecf41ca0 ee44/921a94a0 0000003f 00000001 c00593e4
> 00000000 00000001
> > GPR08: 00000000 a7a7a7a7 00000001 045/920000002 42028042 100a38d4
> ed576d98 00000000
> > GPR16: ed5a11b0 00000000 2b162000 00000200 046/920000000 2d555000
> ed3015e8 c15a7aa0
> > GPR24: 00000000 c155fc40 00000000 ecb63220 ecf41d28 e47/92f640bb0
> ef640c30 ecf41ca0
> > NIP [c02b048c] async_tx_submit+0x6c/0x2b4
> > LR [c02b068c] async_tx_submit+0x26c/0x2b4
> > Call Trace:
> > [ecf41ca0] [c02b068c] async_tx_submit+0x26c/0x2b448/92 (unreliable)
> > [ecf41cd0] [c02b0a4c] async_memcpy+0x240/0x25c
> > [ecf41d20] [c0421064] async_copy_data+0xa0/0x17c
> > [ecf41d70] [c0421cf4] __raid_run_ops+0x874/0xe10
> > [ecf41df0] [c0426ee4] handle_stripe+0x820/0x25e8
> > [ecf41e90] [c0429080] raid5d+0x3d4/0x5b4
> > [ecf41f40] [c04329b8] md_thread+0x138/0x16c
> > [ecf41f90] [c008277c] kthread+0x8c/0x90
> > [ecf41ff0] [c0011630] kernel_thread+0x4c/0x68
> >
> > Another major modification in this patch is the change to completed
> descriptors,
> > there is a potential risk which caused by exception interrupt, all
> descriptors
> > in ld_running list are seemed completed when an interrupt raised, it
> works fine
> > under normal condition, but if there is an exception occured, it cannot
> work
> > as our excepted. Hardware should not depend on s/w list, the right way
> is
> > to read current descriptor address register to find the last completed
> > descriptor. If an interrupt is raised by an error, all descriptors in
> ld_running
> > should not be seemed finished, or these unfinished descriptors in
> ld_running
> > will be released wrongly.
> >
> > A simple way to reproduce,
> > Enable dmatest first, then insert some bad descriptors which can
> trigger
> > Programming Error interrupts before the good descriptors. Last, the
> good
> > descriptors will be freed before they are processsed because of the
> exception
> > intrerrupt.
> >
> > Note: the bad descriptors are only for simulating an exception
> interrupt.
> > This case can illustrate the potential risk in current fsl-dma very
> well.
> >
>=20
> I've never managed to trigger a PE (programming error) interrupt on the
> 83xx hardware. Any time I intentionally caused an error, the hardware
> wedged itself. The CB (channel busy) bit is stuck high, and cannot be
> cleared without a hard reset of the board.
>=20
> I agree the "snoop on the hardware" technique works. As far as I can
> tell, you have implemented the code correctly.
>=20
> The MPC8349EARM.pdf from Freescale indicates that the hardware will halt
> in response to a programming error, and generate a PE interrupt. See
> section 12.5.3.3 (pg 568).
>=20
> The driver, as it is written, will never recover from such a condition.
> Since you are complaining about this situation, do you intend to fix it?
>=20
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: Dan Williams <dan.j.williams@gmail.com>
> > Cc: Vinod Koul <vinod.koul@intel.com>
> > Cc: Li Yang <leoli@freescale.com>
> > Cc: Ira W. Snyder <iws@ovro.caltech.edu>
> > Signed-off-by: Qiang Liu <qiang.liu@freescale.com>
> > ---
> > drivers/dma/fsldma.c | 242 +++++++++++++++++++++++++++++++++++-------
> --------
> > drivers/dma/fsldma.h | 1 +
> > 2 files changed, 172 insertions(+), 71 deletions(-)
> >
> > diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
> > index 4f2f212..87f52c0 100644
> > --- a/drivers/dma/fsldma.c
> > +++ b/drivers/dma/fsldma.c
> > @@ -400,6 +400,125 @@ out_splice:
> > list_splice_tail_init(&desc->tx_list, &chan->ld_pending);
> > }
> >
> > +static void fsldma_cleanup_descriptor(struct fsldma_chan *chan);
> > +static void fsl_chan_xfer_ld_queue(struct fsldma_chan *chan);
> > +
>=20
> As noted in my reply to patch 4/6, please swap the order of this patch
> and the following patch.
>=20
> These lines should not be added or removed in either patch.
>=20
> > +/**
> > + * fsldma_clean_completed_descriptor - free all descriptors which
> > + * has been completed and acked
> > + * @chan: Freescale DMA channel
> > + *
> > + * This function is used on all completed and acked descriptors.
> > + * All descriptors should only be freed in this function.
> > + */
> > +static int
> > +fsldma_clean_completed_descriptor(struct fsldma_chan *chan)
>=20
> This should be 'static void'. It does not return an error code.
>=20
> > +{
> > + struct fsl_desc_sw *desc, *_desc;
> > +
> > + /* Run the callback for each descriptor, in order */
> > + list_for_each_entry_safe(desc, _desc, &chan->ld_completed, node) {
> > +
> > + if (async_tx_test_ack(&desc->async_tx)) {
> > + /* Remove from the list of transactions */
> > + list_del(&desc->node);
> > +#ifdef FSL_DMA_LD_DEBUG
> > + chan_dbg(chan, "LD %p free\n", desc);
> > +#endif
> > + dma_pool_free(chan->desc_pool, desc,
> > + desc->async_tx.phys);
>=20
> This code appears in multiple places in the driver. Please consider
> adding my patch 3/7 titled "[PATCH 3/7] fsl-dma: add
> fsl_dma_free_descriptor() to reduce code duplication" to your patch
> series.
>=20
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * fsldma_run_tx_complete_actions - cleanup and free a single link
> descriptor
>=20
> This documentation is incorrect. This code NEVER frees a descriptor.
>=20
> > + * @chan: Freescale DMA channel
> > + * @desc: descriptor to cleanup and free
> > + * @cookie: Freescale DMA transaction identifier
> > + *
> > + * This function is used on a descriptor which has been executed by
> the DMA
> > + * controller. It will run any callbacks, submit any dependencies.
> > + */
> > +static dma_cookie_t fsldma_run_tx_complete_actions(struct fsl_desc_sw
> *desc,
> > + struct fsldma_chan *chan, dma_cookie_t cookie)
>=20
> Please change the parameter order to:
>=20
> static dma_cookie_t fsldma_run_tx_complete_actions(struct fsldma_chan
> *chan,
> struct fsl_desc_sw *desc, dma_cookie_t cookie)
>=20
> Every other function in the driver uses this parameter order. Channel
> comes first, then descriptor.
>=20
> > +{
> > + struct dma_async_tx_descriptor *txd =3D &desc->async_tx;
> > + struct device *dev =3D chan->common.device->dev;
> > + dma_addr_t src =3D get_desc_src(chan, desc);
> > + dma_addr_t dst =3D get_desc_dst(chan, desc);
> > + u32 len =3D get_desc_cnt(chan, desc);
> > +
> > + BUG_ON(txd->cookie < 0);
> > +
> > + if (txd->cookie > 0) {
>=20
> It will significantly reduce your patch size if you move this if
> statement to the function which calls this one. I've provided an example
> down below, in the one place where this code is used.
>=20
> > + cookie =3D txd->cookie;
> > +
> > + /* Run the link descriptor callback function */
> > + if (txd->callback) {
> > +#ifdef FSL_DMA_LD_DEBUG
> > + chan_dbg(chan, "LD %p callback\n", desc);
> > +#endif
> > + txd->callback(txd->callback_param);
> > + }
> > +
> > + /* Unmap the dst buffer, if requested */
> > + if (!(txd->flags & DMA_COMPL_SKIP_DEST_UNMAP)) {
> > + if (txd->flags & DMA_COMPL_DEST_UNMAP_SINGLE)
> > + dma_unmap_single(dev, dst, len, DMA_FROM_DEVICE);
> > + else
> > + dma_unmap_page(dev, dst, len, DMA_FROM_DEVICE);
> > + }
> > +
> > + /* Unmap the src buffer, if requested */
> > + if (!(txd->flags & DMA_COMPL_SKIP_SRC_UNMAP)) {
> > + if (txd->flags & DMA_COMPL_SRC_UNMAP_SINGLE)
> > + dma_unmap_single(dev, src, len, DMA_TO_DEVICE);
> > + else
> > + dma_unmap_page(dev, src, len, DMA_TO_DEVICE);
> > + }
> > + }
> > +
> > + /* Run any dependencies */
> > + dma_run_dependencies(txd);
> > +
> > + return cookie;
> > +}
> > +
> > +/**
> > + * fsldma_clean_running_descriptor - move the completed descriptor
> from
> > + * ld_running to ld_completed
> > + * @chan: Freescale DMA channel
> > + * @desc: the descriptor which is completed
> > + *
> > + * Free the descriptor directly if acked by async_tx api, or move it
> to
> > + * queue ld_completed.
> > + */
> > +static int
>=20
> This code never returns an error code. It should be 'static void'.
>=20
> > +fsldma_clean_running_descriptor(struct fsldma_chan *chan,
> > + struct fsl_desc_sw *desc)
> > +{
> > + /* Remove from the list of transactions */
> > + list_del(&desc->node);
> > + /*
> > + * the client is allowed to attach dependent operations
> > + * until 'ack' is set
> > + */
> > + if (!async_tx_test_ack(&desc->async_tx)) {
> > + /*
> > + * Move this descriptor to the list of descriptors which is
> > + * completed, but still awaiting the 'ack' bit to be set.
> > + */
> > + list_add_tail(&desc->node, &chan->ld_completed);
> > + return 0;
> > + }
> > +
> > + dma_pool_free(chan->desc_pool, desc, desc->async_tx.phys);
> > + return 0;
> > +}
> > +
> > static dma_cookie_t fsl_dma_tx_submit(struct dma_async_tx_descriptor
> *tx)
> > {
> > struct fsldma_chan *chan =3D to_fsl_chan(tx->chan);
> > @@ -534,8 +653,10 @@ static void fsl_dma_free_chan_resources(struct
> dma_chan *dchan)
> >
> > chan_dbg(chan, "free all channel resources\n");
> > spin_lock_irqsave(&chan->desc_lock, flags);
> > + fsldma_cleanup_descriptor(chan);
> > fsldma_free_desc_list(chan, &chan->ld_pending);
> > fsldma_free_desc_list(chan, &chan->ld_running);
> > + fsldma_free_desc_list(chan, &chan->ld_completed);
> > spin_unlock_irqrestore(&chan->desc_lock, flags);
> >
> > dma_pool_destroy(chan->desc_pool);
> > @@ -819,46 +940,53 @@ static int fsl_dma_device_control(struct dma_chan
> *dchan,
> > * controller. It will run any callbacks, submit any dependencies, and
> then
> > * free the descriptor.
> > */
>=20
> This documentation is now wrong. This function no longer operates on a
> single descriptor. It operates on all descriptors in ld_running and
> ld_completed.
>=20
> Please fix the documentation, and add locking notes.
>=20
> > -static void fsldma_cleanup_descriptor(struct fsldma_chan *chan,
> > - struct fsl_desc_sw *desc)
> > +static void fsldma_cleanup_descriptor(struct fsldma_chan *chan)
>=20
> I think the name should change to fsldma_cleanup_descriptors(). It
> cleans up one or more descriptors now.
>=20
> > {
> > - struct dma_async_tx_descriptor *txd =3D &desc->async_tx;
> > - struct device *dev =3D chan->common.device->dev;
> > - dma_addr_t src =3D get_desc_src(chan, desc);
> > - dma_addr_t dst =3D get_desc_dst(chan, desc);
> > - u32 len =3D get_desc_cnt(chan, desc);
> > + struct fsl_desc_sw *desc, *_desc;
> > + dma_cookie_t cookie =3D 0;
> > + dma_addr_t curr_phys =3D get_cdar(chan);
> > + int idle =3D dma_is_idle(chan);
> > + int seen_current =3D 0;
> >
>=20
> The hardware can advance quite a bit between here, where you save the
> current descriptor address and idle status.
>=20
> > - /* Run the link descriptor callback function */
> > - if (txd->callback) {
> > -#ifdef FSL_DMA_LD_DEBUG
> > - chan_dbg(chan, "LD %p callback\n", desc);
> > -#endif
> > - txd->callback(txd->callback_param);
> > - }
> > + fsldma_clean_completed_descriptor(chan);
> >
> > - /* Run any dependencies */
> > - dma_run_dependencies(txd);
> > + /* Run the callback for each descriptor, in order */
> > + list_for_each_entry_safe(desc, _desc, &chan->ld_running, node) {
> > + /*
> > + * do not advance past the current descriptor loaded into the
> > + * hardware channel, subsequent descriptors are either in
> > + * process or have not been submitted
> > + */
> > + if (seen_current)
> > + break;
> >
> > - /* Unmap the dst buffer, if requested */
> > - if (!(txd->flags & DMA_COMPL_SKIP_DEST_UNMAP)) {
> > - if (txd->flags & DMA_COMPL_DEST_UNMAP_SINGLE)
> > - dma_unmap_single(dev, dst, len, DMA_FROM_DEVICE);
> > - else
> > - dma_unmap_page(dev, dst, len, DMA_FROM_DEVICE);
> > - }
> > + /*
> > + * stop the search if we reach the current descriptor and the
> > + * channel is busy
> > + */
> > + if (desc->async_tx.phys =3D=3D curr_phys) {
> > + seen_current =3D 1;
> > + if (!idle)
> > + break;
> > + }
>=20
> And here, where you check the current descriptor address and idle
> status.
>=20
> Should this change to:
>=20
> if (desc->async_tx.phys =3D=3D get_cdar(chan)) {
> seen_current =3D 1;
> if (!dma_is_idle(chan))
> break;
> }
>=20
> > +
> > + cookie =3D fsldma_run_tx_complete_actions(desc, chan, cookie);
> > +
>=20
> I would prefer if the code just kept track of the cookie here, rather
> than passing it through this function call. This code also illustrates
> how you can remove the "if (txd->cookie > 0)" check from
> fsldma_run_tx_complete_actions() to reduce the patch size.
>=20
> /*
> * Only descriptors with non-zero cookies need their completion
> * actions run.
> */
> if (desc->async_tx.cookie > 0) {
> cookie =3D desc->async_tx.cookie;
> fsldma_run_tx_complete_actions(chan, desc);
> desc->async_tx.cookie =3D 0;
> }
>=20
> /* This descriptor has been ACKed, free it */
> if (async_tx_test_ack(&desc->async_tx)) {
> fsl_dma_free_descriptor(chan, desc);
> continue;
> }
>=20
> /*
> * This descriptor was not ACKed, add it to the ld_completed
> * list, to be freed after the ACK bit is set.
> */
> list_del(&desc->node);
> list_add_tail(&desc->node, &chan->ld_completed);
>=20
>=20
> > + if (fsldma_clean_running_descriptor(chan, desc))
> > + break;
> >
>=20
> This if statement will never trigger. fsldma_clean_running_descriptor()
> only returns 0. It is useless.
>=20
> > - /* Unmap the src buffer, if requested */
> > - if (!(txd->flags & DMA_COMPL_SKIP_SRC_UNMAP)) {
> > - if (txd->flags & DMA_COMPL_SRC_UNMAP_SINGLE)
> > - dma_unmap_single(dev, src, len, DMA_TO_DEVICE);
> > - else
> > - dma_unmap_page(dev, src, len, DMA_TO_DEVICE);
> > }
> >
> > -#ifdef FSL_DMA_LD_DEBUG
> > - chan_dbg(chan, "LD %p free\n", desc);
> > -#endif
> > - dma_pool_free(chan->desc_pool, desc, txd->phys);
> > + /*
> > + * Start any pending transactions automatically
> > + *
> > + * In the ideal case, we keep the DMA controller busy while we go
> > + * ahead and free the descriptors below.
> > + */
> > + fsl_chan_xfer_ld_queue(chan);
> > +
> > + if (cookie > 0)
> > + chan->common.completed_cookie =3D cookie;
> > }
> >
> > /**
> > @@ -954,11 +1082,15 @@ static enum dma_status fsl_tx_status(struct
> dma_chan *dchan,
> > enum dma_status ret;
> > unsigned long flags;
> >
> > - spin_lock_irqsave(&chan->desc_lock, flags);
> > ret =3D dma_cookie_status(dchan, cookie, txstate);
> > + if (ret =3D=3D DMA_SUCCESS)
> > + return ret;
> > +
> > + spin_lock_irqsave(&chan->desc_lock, flags);
> > + fsldma_cleanup_descriptor(chan);
> > spin_unlock_irqrestore(&chan->desc_lock, flags);
> >
> > - return ret;
> > + return dma_cookie_status(dchan, cookie, txstate);
> > }
> >
> > /*--------------------------------------------------------------------
> --------*/
> > @@ -1035,52 +1167,19 @@ static irqreturn_t fsldma_chan_irq(int irq,
> void *data)
> > static void dma_do_tasklet(unsigned long data)
> > {
> > struct fsldma_chan *chan =3D (struct fsldma_chan *)data;
> > - struct fsl_desc_sw *desc, *_desc;
> > - LIST_HEAD(ld_cleanup);
> > unsigned long flags;
> >
> > chan_dbg(chan, "tasklet entry\n");
> >
> > spin_lock_irqsave(&chan->desc_lock, flags);
> >
> > - /* update the cookie if we have some descriptors to cleanup */
> > - if (!list_empty(&chan->ld_running)) {
> > - dma_cookie_t cookie;
> > -
> > - desc =3D to_fsl_desc(chan->ld_running.prev);
> > - cookie =3D desc->async_tx.cookie;
> > - dma_cookie_complete(&desc->async_tx);
> > -
> > - chan_dbg(chan, "completed_cookie=3D%d\n", cookie);
> > - }
> > -
> > - /*
> > - * move the descriptors to a temporary list so we can drop the lock
> > - * during the entire cleanup operation
> > - */
> > - list_splice_tail_init(&chan->ld_running, &ld_cleanup);
> > -
> > /* the hardware is now idle and ready for more */
> > chan->idle =3D true;
> >
> > - /*
> > - * Start any pending transactions automatically
> > - *
> > - * In the ideal case, we keep the DMA controller busy while we go
> > - * ahead and free the descriptors below.
> > - */
> > - fsl_chan_xfer_ld_queue(chan);
> > - spin_unlock_irqrestore(&chan->desc_lock, flags);
> > -
> > - /* Run the callback for each descriptor, in order */
> > - list_for_each_entry_safe(desc, _desc, &ld_cleanup, node) {
> > + /* Run all cleanup for this descriptor */
>=20
> Nitpick. This should be:
>=20
> /* Run cleanup for all descriptors */
>=20
> > + fsldma_cleanup_descriptor(chan);
> >
> > - /* Remove from the list of transactions */
> > - list_del(&desc->node);
> > -
> > - /* Run all cleanup for this descriptor */
> > - fsldma_cleanup_descriptor(chan, desc);
> > - }
> > + spin_unlock_irqrestore(&chan->desc_lock, flags);
> >
> > chan_dbg(chan, "tasklet exit\n");
> > }
> > @@ -1262,6 +1361,7 @@ static int __devinit fsl_dma_chan_probe(struct
> fsldma_device *fdev,
> > spin_lock_init(&chan->desc_lock);
> > INIT_LIST_HEAD(&chan->ld_pending);
> > INIT_LIST_HEAD(&chan->ld_running);
> > + INIT_LIST_HEAD(&chan->ld_completed);
> > chan->idle =3D true;
> >
> > chan->common.device =3D &fdev->common;
> > diff --git a/drivers/dma/fsldma.h b/drivers/dma/fsldma.h
> > index f5c3879..7ede908 100644
> > --- a/drivers/dma/fsldma.h
> > +++ b/drivers/dma/fsldma.h
> > @@ -140,6 +140,7 @@ struct fsldma_chan {
> > spinlock_t desc_lock; /* Descriptor operation lock */
> > struct list_head ld_pending; /* Link descriptors queue */
> > struct list_head ld_running; /* Link descriptors queue */
> > + struct list_head ld_completed; /* Link descriptors queue */
>=20
> It may help to add some documentation here. It would have helped me to
> review this patch. Something like this:
>=20
> /*
> * Descriptors which are queued to run, but have not yet been handed
> * to the hardware for execution
> */
> struct list_head ld_pending;
>=20
> /*
> * Descriptors which are currently being executed by the hardware
> */
> struct list_head ld_running;
>=20
> /*
> * Descriptors which have finished execution by the hardware. These
> * descriptors have already had their cleanup actions run. They are
> * waiting for the ACK bit to be set by the async_tx API.
> */
> struct list_head ld_completed;
>=20
> > struct dma_chan common; /* DMA common channel */
> > struct dma_pool *desc_pool; /* Descriptors pool */
> > struct device *dev; /* Channel device */
> > --
> > 1.7.5.1
> >
> >
> > _______________________________________________
> > Linuxppc-dev mailing list
> > Linuxppc-dev@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* Re: [PATCH v8 5/7] powerpc/85xx: add sleep and deep sleep support
From: Zhao Chenhui @ 2012-08-02 11:12 UTC (permalink / raw)
To: Kumar Gala; +Cc: scottwood, linuxppc-dev, linux-kernel
In-Reply-To: <F8FE8505-6F6A-4A4B-8302-C1CF16B48B60@kernel.crashing.org>
On Tue, Jul 31, 2012 at 09:15:33AM -0500, Kumar Gala wrote:
>
> On Jul 20, 2012, at 7:42 AM, Zhao Chenhui wrote:
>
> > In sleep PM mode, the clocks of e500 core and unused IP blocks is
> > turned off. IP blocks which are allowed to wake up the processor
> > are still running.
> >
> > Some Freescale chips like MPC8536 and P1022 has deep sleep PM mode
> > in addtion to the sleep PM mode.
> >
> > While in deep sleep PM mode, additionally, the power supply is
> > removed from e500 core and most IP blocks. Only the blocks needed
> > to wake up the chip out of deep sleep are ON.
> >
> > This patch supports 32-bit and 36-bit address space.
> >
> > The sleep mode is equal to the Standby state in Linux. The deep sleep
> > mode is equal to the Suspend-to-RAM state of Linux Power Management.
> >
> > Command to enter sleep mode.
> > echo standby > /sys/power/state
> > Command to enter deep sleep mode.
> > echo mem > /sys/power/state
> >
> > Signed-off-by: Dave Liu <daveliu@freescale.com>
> > Signed-off-by: Li Yang <leoli@freescale.com>
> > Signed-off-by: Jin Qing <b24347@freescale.com>
> > Signed-off-by: Jerry Huang <Chang-Ming.Huang@freescale.com>
> > Cc: Scott Wood <scottwood@freescale.com>
> > Signed-off-by: Zhao Chenhui <chenhui.zhao@freescale.com>
> > ---
> > arch/powerpc/Kconfig | 2 +-
> > arch/powerpc/include/asm/cacheflush.h | 2 +
> > arch/powerpc/kernel/Makefile | 3 +
> > arch/powerpc/kernel/l2cache_85xx.S | 56 +++
> > arch/powerpc/platforms/85xx/Makefile | 2 +-
> > arch/powerpc/platforms/85xx/sleep.S | 621 +++++++++++++++++++++++++++++++++
> > arch/powerpc/sysdev/fsl_pmc.c | 98 +++++-
> > arch/powerpc/sysdev/fsl_soc.h | 5 +
> > 8 files changed, 769 insertions(+), 20 deletions(-)
> > create mode 100644 arch/powerpc/kernel/l2cache_85xx.S
> > create mode 100644 arch/powerpc/platforms/85xx/sleep.S
> >
> > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > index a7c6914..9d6de82 100644
> > --- a/arch/powerpc/Kconfig
> > +++ b/arch/powerpc/Kconfig
> > @@ -665,7 +665,7 @@ config FSL_PCI
> > config FSL_PMC
> > bool
> > default y
> > - depends on SUSPEND && (PPC_85xx || PPC_86xx)
> > + depends on SUSPEND && (PPC_85xx || PPC_86xx) && !PPC_E500MC
> > help
> > Freescale MPC85xx/MPC86xx power management controller support
> > (suspend/resume). For MPC83xx see platforms/83xx/suspend.c
> > diff --git a/arch/powerpc/include/asm/cacheflush.h b/arch/powerpc/include/asm/cacheflush.h
> > index b843e35..6c5f1c2 100644
> > --- a/arch/powerpc/include/asm/cacheflush.h
> > +++ b/arch/powerpc/include/asm/cacheflush.h
> > @@ -58,6 +58,8 @@ extern void flush_inval_dcache_range(unsigned long start, unsigned long stop);
> > extern void flush_dcache_phys_range(unsigned long start, unsigned long stop);
> > #endif
> >
> > +extern void flush_dcache_L1(void);
> > +
> > #define copy_to_user_page(vma, page, vaddr, dst, src, len) \
> > do { \
> > memcpy(dst, src, len); \
> > diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> > index 83afacd..0ddef24 100644
> > --- a/arch/powerpc/kernel/Makefile
> > +++ b/arch/powerpc/kernel/Makefile
> > @@ -64,6 +64,9 @@ obj-$(CONFIG_FA_DUMP) += fadump.o
> > ifeq ($(CONFIG_PPC32),y)
> > obj-$(CONFIG_E500) += idle_e500.o
> > endif
> > +ifneq ($(CONFIG_PPC_E500MC),y)
> > +obj-$(CONFIG_PPC_85xx) += l2cache_85xx.o
> > +endif
>
> why do we need this, beyond reduce code size on an e500mc kernel build? If so why isn't 85xx/sleep.S doing the same thing?
> - k
>
Yes, it is a little awkward. I have an idea to put e500/e500mc/e5500/e6500 related flush cache routines
into this file, and rename it to cache_fsl_booke.S.
As for 85xx/sleep.S, it is used by fsl_pmc.c. I will use CONFIG_FSL_PMC to guard it.
-Chenhui
^ permalink raw reply
* Serial interrupt overrun and hard deadlock on 3.3.2 & 3.5, freescale 8347.
From: Christian Melki @ 2012-08-02 10:46 UTC (permalink / raw)
To: linuxppc-dev@lists.ozlabs.org
Hi.
I've spent a couple of hours lifting our freescale 8347 platform from 2.6.3=
2 to 3.3.2 and just now to 3.5. The 3.x kernels are giving me some problems=
.
I get serial input overruns especially when cpu is loded, but I can reprodu=
ce overruns even at 0 other load. I can't even remember when I saw overruns=
like this the last time, must have been in 2.2.x or early 2.4.x.
Also I can reliably trigger a hard kernel lock when using a USB-camera and =
acessing sequential blocks from the mtd device (read to verify flash). I'm =
thinking that this maybe has something to do with the reworks of IRQ handli=
ng in the powerpc arch lately.
I've tried most things I can think of to see if I can get a symptom change =
somewhere (incl, preemption, smp-kernel etc etc) but I get nothing.
Hard lock detection seems to be NMI bound on x86 only (?), so I can't use t=
hat.
A jtag-debugger does not give me much either, since I can't record the inst=
ruction stream in realtime anyway.
I'm kind of starved for ideas, so any help or tip as to what might be wrong=
would be appreciated.
Regards,
Christian=
^ permalink raw reply
* Re: [PATCH] powerpc/smp: Do not disable IPI interrupts during suspend
From: Zhao Chenhui @ 2012-08-02 10:04 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: linuxppc-dev@lists.ozlabs.org list,
linux-kernel@vger.kernel.org list
In-Reply-To: <1343427631.21647.1.camel@pasglop>
On Sat, Jul 28, 2012 at 08:20:31AM +1000, Benjamin Herrenschmidt wrote:
> On Fri, 2012-07-27 at 16:58 -0500, Kumar Gala wrote:
> > On Jul 20, 2012, at 7:47 AM, Zhao Chenhui wrote:
> >
> > > During suspend, all interrupts including IPI will be disabled. In this case,
> > > the suspend process will hang in SMP. To prevent this, pass the flag
> > > IRQF_NO_SUSPEND when requesting IPI irq.
> > >
> > > Signed-off-by: Zhao Chenhui <chenhui.zhao@freescale.com>
> > > Signed-off-by: Li Yang <leoli@freescale.com>
> > > ---
> > > arch/powerpc/kernel/smp.c | 2 +-
> > > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > BenH,
> >
> > Can you ack?
>
> No I'll merge it but not until it's been in next for a bit unless you
> have some strong emergency there, it's on my mental list of things to
> shovel into next after rc1.
>
> Curiosity: didn't we use to disable all non-boot CPUs on suspend ?
>
> Cheers,
> Ben.
Yes, we disabled all non-boot CPUs on suspend by calling disable_nonboot_cpus().
The disable_nonboot_cpus() needs IPIs to work. But prior to
calling disable_nonboot_cpus(), the IPIs are disabled in dpm_suspend_noirq().
-Chenhui
^ permalink raw reply
* RE: Problem in phy.c, when using fixed network speed
From: Jenkins, Clive @ 2012-08-02 9:35 UTC (permalink / raw)
To: Michael Koch, linuxppc-dev
In-Reply-To: <50197214.7090703@gmx.net>
> Hi all,
> during testing i encountered a problem with setting
> up a 5200B controller with a MICREL phy at static
> 100MBit full duplex - without autonegotiation.
>
> I performed this as usual with ethtool and was
> succesful when i had my link partner up, providing
> a link.
>
> When kepping the link partner off, meaning no link
> at all, my machine started to degrade its link
> capabilities ending 10MBit half duplex.
>
> I tracked it down to drivers/net/phy/phy.c:
> in the function phy_state_machine, the case block
> PHY_FORCING causes this (at least for me) undesired
> behaviour. Calling the phy_force_reduction function
> degrades actually an intentionally static setup.
>
> I deactivated those lines, and it works for me.
>
> But anyhow i feel soe need to have a general
> solution that takes static non autonagotiation
> setups into account.
>
> What do you think?
>
> Hope to hear from you
>
> Michael
Yes, I have encountered this before. I think it dates=20
back to before Auto Negotiation became part of the=20
IEEE802* standard and each manufacturer implemented=20
its own strategy to establish a link. Although it is=20
possible "by experiment" to find the speed of your=20
link partner, it is impossible to determine its=20
Full/Half Duplex mode. IMHO when a fixed speed and=20
duplex setting is applied, phy.c should keep that=20
setting regardless of whether or not the link is=20
established. Not only is this undesirable behaviour,=20
but it deviates from the standard.
Clive
^ permalink raw reply
* RE: [PATCH v5 3/6] fsl-dma: change release process of dma descriptor for supporting async_tx
From: Liu Qiang-B32616 @ 2012-08-02 7:21 UTC (permalink / raw)
To: Ira W. Snyder
Cc: herbert@gondor.apana.org.au, Vinod Koul,
linux-kernel@vger.kernel.org, dan.j.williams@gmail.com,
linux-crypto@vger.kernel.org, Dan Williams,
linuxppc-dev@lists.ozlabs.org, davem@davemloft.net
In-Reply-To: <20120801172526.GD11359@ovro.caltech.edu>
> -----Original Message-----
> From: Ira W. Snyder [mailto:iws@ovro.caltech.edu]
> Sent: Thursday, August 02, 2012 1:25 AM
> To: Liu Qiang-B32616
> Cc: linux-crypto@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-
> kernel@vger.kernel.org; dan.j.williams@gmail.com; Vinod Koul;
> herbert@gondor.hengli.com.au; Dan Williams; davem@davemloft.net
> Subject: Re: [PATCH v5 3/6] fsl-dma: change release process of dma
> descriptor for supporting async_tx
>=20
> On Wed, Aug 01, 2012 at 04:49:17PM +0800, qiang.liu@freescale.com wrote:
> > From: Qiang Liu <qiang.liu@freescale.com>
> >
> > Fix the potential risk when enable config NET_DMA and ASYNC_TX.
> > Async_tx is lack of support in current release process of dma
> > descriptor, all descriptors will be released whatever is acked or
> > no-acked by async_tx, so there is a potential race condition when dma
> > engine is uesd by others clients (e.g. when enable NET_DMA to offload
> TCP).
> >
> > In our case, a race condition which is raised when use both of talitos
> > and dmaengine to offload xor is because napi scheduler will sync all
> > pending requests in dma channels, it affects the process of raid
> > operations due to ack_tx is not checked in fsl dma. The no-acked
> > descriptor is freed which is submitted just now, as a dependent tx,
> > this freed descriptor trigger
> > BUG_ON(async_tx_test_ack(depend_tx)) in async_tx_submit().
> >
> > TASK =3D ee1a94a0[1390] 'md0_raid5' THREAD: ecf40000 CPU: 0
> > GPR00: 00000001 ecf41ca0 ee44/921a94a0 0000003f 00000001 c00593e4
> > 00000000 00000001
> > GPR08: 00000000 a7a7a7a7 00000001 045/920000002 42028042 100a38d4
> > ed576d98 00000000
> > GPR16: ed5a11b0 00000000 2b162000 00000200 046/920000000 2d555000
> > ed3015e8 c15a7aa0
> > GPR24: 00000000 c155fc40 00000000 ecb63220 ecf41d28 e47/92f640bb0
> > ef640c30 ecf41ca0 NIP [c02b048c] async_tx_submit+0x6c/0x2b4 LR
> > [c02b068c] async_tx_submit+0x26c/0x2b4 Call Trace:
> > [ecf41ca0] [c02b068c] async_tx_submit+0x26c/0x2b448/92 (unreliable)
> > [ecf41cd0] [c02b0a4c] async_memcpy+0x240/0x25c [ecf41d20] [c0421064]
> > async_copy_data+0xa0/0x17c [ecf41d70] [c0421cf4]
> > __raid_run_ops+0x874/0xe10 [ecf41df0] [c0426ee4]
> > handle_stripe+0x820/0x25e8 [ecf41e90] [c0429080] raid5d+0x3d4/0x5b4
> > [ecf41f40] [c04329b8] md_thread+0x138/0x16c [ecf41f90] [c008277c]
> > kthread+0x8c/0x90 [ecf41ff0] [c0011630] kernel_thread+0x4c/0x68
> >
> > Another major modification in this patch is the change to completed
> > descriptors, there is a potential risk which caused by exception
> > interrupt, all descriptors in ld_running list are seemed completed
> > when an interrupt raised, it works fine under normal condition, but if
> > there is an exception occured, it cannot work as our excepted.
> > Hardware should not depend on s/w list, the right way is to read
> > current descriptor address register to find the last completed
> > descriptor. If an interrupt is raised by an error, all descriptors in
> > ld_running should not be seemed finished, or these unfinished
> descriptors in ld_running will be released wrongly.
> >
> > A simple way to reproduce,
> > Enable dmatest first, then insert some bad descriptors which can
> > trigger Programming Error interrupts before the good descriptors.
> > Last, the good descriptors will be freed before they are processsed
> > because of the exception intrerrupt.
> >
> > Note: the bad descriptors are only for simulating an exception
> interrupt.
> > This case can illustrate the potential risk in current fsl-dma very
> well.
> >
>=20
> I've never managed to trigger a PE (programming error) interrupt on the
> 83xx hardware. Any time I intentionally caused an error, the hardware
> wedged itself. The CB (channel busy) bit is stuck high, and cannot be
> cleared without a hard reset of the board.
Sorry, the exception indeed will be occurred, actually, the capability DMA_=
INTERRUPT
will reproduce the issue under conditions. It will trigger a exception beca=
use of
bad descriptor (length is zero, src and dst are zero, a->b->c->bada->badb->=
d, we cannot find out which one is really finished in an interrupt tasklet)=
.
So, we'd better consider this case.
BTW, I have already found out your patch which is used to resolve the issue=
of dma lock,
http://lkml.indiana.edu/hypermail/linux/kernel/1103.0/01519.html
>=20
> I agree the "snoop on the hardware" technique works. As far as I can tell=
,
> you have implemented the code correctly.
>=20
> The MPC8349EARM.pdf from Freescale indicates that the hardware will halt
> in response to a programming error, and generate a PE interrupt. See
> section 12.5.3.3 (pg 568).
>=20
> The driver, as it is written, will never recover from such a condition.
> Since you are complaining about this situation, do you intend to fix it?
Frankly, I don't think your patch really can resolve the issue. Now, I unde=
rstand what problem happen to you, I will follow it.
Yes, you are right, the driver will never recover except reset the board.
I see your description and I can reproduce it with dmatest on p1022ds with =
latest kernel, Dmatest with 6 threads, 200,000 iterations per thread severa=
l is passed with or without my patch, but dma is locked when up to 300,000 =
itrerations even though drop my patch.
Another test on p4080, 8 threads with 1,000,000 iterations per thread is pa=
ssed with/without my patch.
I will follow this issue and try to find the root cause, but it should be a=
nother topic:)
Here, I agree with yours most comments, I will merge some functions from yo=
ur patch, I will send v6 if you agree with my comments. Thanks.
Please see my comments inline.
>=20
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: Dan Williams <dan.j.williams@gmail.com>
> > Cc: Vinod Koul <vinod.koul@intel.com>
> > Cc: Li Yang <leoli@freescale.com>
> > Cc: Ira W. Snyder <iws@ovro.caltech.edu>
> > Signed-off-by: Qiang Liu <qiang.liu@freescale.com>
> > ---
> > drivers/dma/fsldma.c | 242 +++++++++++++++++++++++++++++++++++-------
> --------
> > drivers/dma/fsldma.h | 1 +
> > 2 files changed, 172 insertions(+), 71 deletions(-)
> >
> > diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c index
> > 4f2f212..87f52c0 100644
> > --- a/drivers/dma/fsldma.c
> > +++ b/drivers/dma/fsldma.c
> > @@ -400,6 +400,125 @@ out_splice:
> > list_splice_tail_init(&desc->tx_list, &chan->ld_pending); }
> >
> > +static void fsldma_cleanup_descriptor(struct fsldma_chan *chan);
> > +static void fsl_chan_xfer_ld_queue(struct fsldma_chan *chan);
> > +
>=20
> As noted in my reply to patch 4/6, please swap the order of this patch
> and the following patch.
>=20
> These lines should not be added or removed in either patch.
Ok.
>=20
> > +/**
> > + * fsldma_clean_completed_descriptor - free all descriptors which
> > + * has been completed and acked
> > + * @chan: Freescale DMA channel
> > + *
> > + * This function is used on all completed and acked descriptors.
> > + * All descriptors should only be freed in this function.
> > + */
> > +static int
> > +fsldma_clean_completed_descriptor(struct fsldma_chan *chan)
>=20
> This should be 'static void'. It does not return an error code.
>=20
Ok.
> > +{
> > + struct fsl_desc_sw *desc, *_desc;
> > +
> > + /* Run the callback for each descriptor, in order */
> > + list_for_each_entry_safe(desc, _desc, &chan->ld_completed, node) {
> > +
> > + if (async_tx_test_ack(&desc->async_tx)) {
> > + /* Remove from the list of transactions */
> > + list_del(&desc->node);
> > +#ifdef FSL_DMA_LD_DEBUG
> > + chan_dbg(chan, "LD %p free\n", desc); #endif
> > + dma_pool_free(chan->desc_pool, desc,
> > + desc->async_tx.phys);
>=20
> This code appears in multiple places in the driver. Please consider
> adding my patch 3/7 titled "[PATCH 3/7] fsl-dma: add
> fsl_dma_free_descriptor() to reduce code duplication" to your patch
> series.
Accept.
>=20
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * fsldma_run_tx_complete_actions - cleanup and free a single link
> > +descriptor
>=20
> This documentation is incorrect. This code NEVER frees a descriptor.
I will correct it.
>=20
> > + * @chan: Freescale DMA channel
> > + * @desc: descriptor to cleanup and free
> > + * @cookie: Freescale DMA transaction identifier
> > + *
> > + * This function is used on a descriptor which has been executed by
> > +the DMA
> > + * controller. It will run any callbacks, submit any dependencies.
> > + */
> > +static dma_cookie_t fsldma_run_tx_complete_actions(struct fsl_desc_sw
> *desc,
> > + struct fsldma_chan *chan, dma_cookie_t cookie)
>=20
> Please change the parameter order to:
>=20
> static dma_cookie_t fsldma_run_tx_complete_actions(struct fsldma_chan
> *chan,
> struct fsl_desc_sw *desc, dma_cookie_t cookie)
>=20
> Every other function in the driver uses this parameter order. Channel
> comes first, then descriptor.
>=20
My fault, I will correct it.
> > +{
> > + struct dma_async_tx_descriptor *txd =3D &desc->async_tx;
> > + struct device *dev =3D chan->common.device->dev;
> > + dma_addr_t src =3D get_desc_src(chan, desc);
> > + dma_addr_t dst =3D get_desc_dst(chan, desc);
> > + u32 len =3D get_desc_cnt(chan, desc);
> > +
> > + BUG_ON(txd->cookie < 0);
> > +
> > + if (txd->cookie > 0) {
>=20
> It will significantly reduce your patch size if you move this if
> statement to the function which calls this one. I've provided an example
> down below, in the one place where this code is used.
My comments as below.
>=20
> > + cookie =3D txd->cookie;
> > +
> > + /* Run the link descriptor callback function */
> > + if (txd->callback) {
> > +#ifdef FSL_DMA_LD_DEBUG
> > + chan_dbg(chan, "LD %p callback\n", desc); #endif
> > + txd->callback(txd->callback_param);
> > + }
> > +
> > + /* Unmap the dst buffer, if requested */
> > + if (!(txd->flags & DMA_COMPL_SKIP_DEST_UNMAP)) {
> > + if (txd->flags & DMA_COMPL_DEST_UNMAP_SINGLE)
> > + dma_unmap_single(dev, dst, len, DMA_FROM_DEVICE);
> > + else
> > + dma_unmap_page(dev, dst, len, DMA_FROM_DEVICE);
> > + }
> > +
> > + /* Unmap the src buffer, if requested */
> > + if (!(txd->flags & DMA_COMPL_SKIP_SRC_UNMAP)) {
> > + if (txd->flags & DMA_COMPL_SRC_UNMAP_SINGLE)
> > + dma_unmap_single(dev, src, len, DMA_TO_DEVICE);
> > + else
> > + dma_unmap_page(dev, src, len, DMA_TO_DEVICE);
> > + }
> > + }
> > +
> > + /* Run any dependencies */
> > + dma_run_dependencies(txd);
> > +
> > + return cookie;
> > +}
> > +
> > +/**
> > + * fsldma_clean_running_descriptor - move the completed descriptor
> > +from
> > + * ld_running to ld_completed
> > + * @chan: Freescale DMA channel
> > + * @desc: the descriptor which is completed
> > + *
> > + * Free the descriptor directly if acked by async_tx api, or move it
> > +to
> > + * queue ld_completed.
> > + */
> > +static int
>=20
> This code never returns an error code. It should be 'static void'.
I will correct it.
>=20
> > +fsldma_clean_running_descriptor(struct fsldma_chan *chan,
> > + struct fsl_desc_sw *desc)
> > +{
> > + /* Remove from the list of transactions */
> > + list_del(&desc->node);
> > + /*
> > + * the client is allowed to attach dependent operations
> > + * until 'ack' is set
> > + */
> > + if (!async_tx_test_ack(&desc->async_tx)) {
> > + /*
> > + * Move this descriptor to the list of descriptors which is
> > + * completed, but still awaiting the 'ack' bit to be set.
> > + */
> > + list_add_tail(&desc->node, &chan->ld_completed);
> > + return 0;
> > + }
> > +
> > + dma_pool_free(chan->desc_pool, desc, desc->async_tx.phys);
> > + return 0;
> > +}
> > +
> > static dma_cookie_t fsl_dma_tx_submit(struct dma_async_tx_descriptor
> > *tx) {
> > struct fsldma_chan *chan =3D to_fsl_chan(tx->chan); @@ -534,8 +653,10
> > @@ static void fsl_dma_free_chan_resources(struct dma_chan *dchan)
> >
> > chan_dbg(chan, "free all channel resources\n");
> > spin_lock_irqsave(&chan->desc_lock, flags);
> > + fsldma_cleanup_descriptor(chan);
> > fsldma_free_desc_list(chan, &chan->ld_pending);
> > fsldma_free_desc_list(chan, &chan->ld_running);
> > + fsldma_free_desc_list(chan, &chan->ld_completed);
> > spin_unlock_irqrestore(&chan->desc_lock, flags);
> >
> > dma_pool_destroy(chan->desc_pool);
> > @@ -819,46 +940,53 @@ static int fsl_dma_device_control(struct dma_chan
> *dchan,
> > * controller. It will run any callbacks, submit any dependencies, and
> then
> > * free the descriptor.
> > */
>=20
> This documentation is now wrong. This function no longer operates on a
> single descriptor. It operates on all descriptors in ld_running and
> ld_completed.
>=20
> Please fix the documentation, and add locking notes.
No, it only frees one descriptor.
>=20
> > -static void fsldma_cleanup_descriptor(struct fsldma_chan *chan,
> > - struct fsl_desc_sw *desc)
> > +static void fsldma_cleanup_descriptor(struct fsldma_chan *chan)
>=20
> I think the name should change to fsldma_cleanup_descriptors(). It cleans
> up one or more descriptors now.
It only frees one descriptor as its designed.
>=20
> > {
> > - struct dma_async_tx_descriptor *txd =3D &desc->async_tx;
> > - struct device *dev =3D chan->common.device->dev;
> > - dma_addr_t src =3D get_desc_src(chan, desc);
> > - dma_addr_t dst =3D get_desc_dst(chan, desc);
> > - u32 len =3D get_desc_cnt(chan, desc);
> > + struct fsl_desc_sw *desc, *_desc;
> > + dma_cookie_t cookie =3D 0;
> > + dma_addr_t curr_phys =3D get_cdar(chan);
> > + int idle =3D dma_is_idle(chan);
> > + int seen_current =3D 0;
> >
>=20
> The hardware can advance quite a bit between here, where you save the
> current descriptor address and idle status.
>=20
> > - /* Run the link descriptor callback function */
> > - if (txd->callback) {
> > -#ifdef FSL_DMA_LD_DEBUG
> > - chan_dbg(chan, "LD %p callback\n", desc);
> > -#endif
> > - txd->callback(txd->callback_param);
> > - }
> > + fsldma_clean_completed_descriptor(chan);
> >
> > - /* Run any dependencies */
> > - dma_run_dependencies(txd);
> > + /* Run the callback for each descriptor, in order */
> > + list_for_each_entry_safe(desc, _desc, &chan->ld_running, node) {
> > + /*
> > + * do not advance past the current descriptor loaded into the
> > + * hardware channel, subsequent descriptors are either in
> > + * process or have not been submitted
> > + */
> > + if (seen_current)
> > + break;
> >
> > - /* Unmap the dst buffer, if requested */
> > - if (!(txd->flags & DMA_COMPL_SKIP_DEST_UNMAP)) {
> > - if (txd->flags & DMA_COMPL_DEST_UNMAP_SINGLE)
> > - dma_unmap_single(dev, dst, len, DMA_FROM_DEVICE);
> > - else
> > - dma_unmap_page(dev, dst, len, DMA_FROM_DEVICE);
> > - }
> > + /*
> > + * stop the search if we reach the current descriptor and the
> > + * channel is busy
> > + */
> > + if (desc->async_tx.phys =3D=3D curr_phys) {
> > + seen_current =3D 1;
> > + if (!idle)
> > + break;
> > + }
>=20
> And here, where you check the current descriptor address and idle status.
>=20
> Should this change to:
>=20
> if (desc->async_tx.phys =3D=3D get_cdar(chan)) {
> seen_current =3D 1;
> if (!dma_is_idle(chan))
> break;
> }
Ok, I will use your code here.
>=20
> > +
> > + cookie =3D fsldma_run_tx_complete_actions(desc, chan, cookie);
> > +
>=20
> I would prefer if the code just kept track of the cookie here, rather
> than passing it through this function call. This code also illustrates
> how you can remove the "if (txd->cookie > 0)" check from
> fsldma_run_tx_complete_actions() to reduce the patch size.
>=20
I cannot agree with you, patch size is important, but program readable is a=
lso important.
My reason as below,
According to my knowledge, cookie is used to judge whether this descriptor =
is finished, if it is zero, it means we didn't assign a value for it. We sh=
ould keep it original meaning for clear?
Second, I think we should not set a complex process to free descriptor, esp=
ecially according to different state of the descriptor, the interface shoul=
d be seemed more reusable and common.
Last, I don't want to see the interface is coupled too many functions. It's=
easier extended for future.
How's your thinking? Of course, your implement is also ok.
> /*
> * Only descriptors with non-zero cookies need their completion
> * actions run.
> */
> if (desc->async_tx.cookie > 0) {
> cookie =3D desc->async_tx.cookie;
> fsldma_run_tx_complete_actions(chan, desc);
> desc->async_tx.cookie =3D 0;
> }
>=20
> /* This descriptor has been ACKed, free it */ if
> (async_tx_test_ack(&desc->async_tx)) {
> fsl_dma_free_descriptor(chan, desc);
> continue;
> }
>=20
> /*
> * This descriptor was not ACKed, add it to the ld_completed
> * list, to be freed after the ACK bit is set.
> */
> list_del(&desc->node);
> list_add_tail(&desc->node, &chan->ld_completed);
>=20
>=20
> > + if (fsldma_clean_running_descriptor(chan, desc))
> > + break;
> >
>=20
> This if statement will never trigger. fsldma_clean_running_descriptor()
> only returns 0. It is useless.
>=20
> > - /* Unmap the src buffer, if requested */
> > - if (!(txd->flags & DMA_COMPL_SKIP_SRC_UNMAP)) {
> > - if (txd->flags & DMA_COMPL_SRC_UNMAP_SINGLE)
> > - dma_unmap_single(dev, src, len, DMA_TO_DEVICE);
> > - else
> > - dma_unmap_page(dev, src, len, DMA_TO_DEVICE);
> > }
> >
> > -#ifdef FSL_DMA_LD_DEBUG
> > - chan_dbg(chan, "LD %p free\n", desc);
> > -#endif
> > - dma_pool_free(chan->desc_pool, desc, txd->phys);
> > + /*
> > + * Start any pending transactions automatically
> > + *
> > + * In the ideal case, we keep the DMA controller busy while we go
> > + * ahead and free the descriptors below.
> > + */
> > + fsl_chan_xfer_ld_queue(chan);
> > +
> > + if (cookie > 0)
> > + chan->common.completed_cookie =3D cookie;
> > }
> >
> > /**
> > @@ -954,11 +1082,15 @@ static enum dma_status fsl_tx_status(struct
> dma_chan *dchan,
> > enum dma_status ret;
> > unsigned long flags;
> >
> > - spin_lock_irqsave(&chan->desc_lock, flags);
> > ret =3D dma_cookie_status(dchan, cookie, txstate);
> > + if (ret =3D=3D DMA_SUCCESS)
> > + return ret;
> > +
> > + spin_lock_irqsave(&chan->desc_lock, flags);
> > + fsldma_cleanup_descriptor(chan);
> > spin_unlock_irqrestore(&chan->desc_lock, flags);
> >
> > - return ret;
> > + return dma_cookie_status(dchan, cookie, txstate);
> > }
> >
> >
> > /*--------------------------------------------------------------------
> > --------*/ @@ -1035,52 +1167,19 @@ static irqreturn_t
> > fsldma_chan_irq(int irq, void *data) static void
> > dma_do_tasklet(unsigned long data) {
> > struct fsldma_chan *chan =3D (struct fsldma_chan *)data;
> > - struct fsl_desc_sw *desc, *_desc;
> > - LIST_HEAD(ld_cleanup);
> > unsigned long flags;
> >
> > chan_dbg(chan, "tasklet entry\n");
> >
> > spin_lock_irqsave(&chan->desc_lock, flags);
> >
> > - /* update the cookie if we have some descriptors to cleanup */
> > - if (!list_empty(&chan->ld_running)) {
> > - dma_cookie_t cookie;
> > -
> > - desc =3D to_fsl_desc(chan->ld_running.prev);
> > - cookie =3D desc->async_tx.cookie;
> > - dma_cookie_complete(&desc->async_tx);
> > -
> > - chan_dbg(chan, "completed_cookie=3D%d\n", cookie);
> > - }
> > -
> > - /*
> > - * move the descriptors to a temporary list so we can drop the lock
> > - * during the entire cleanup operation
> > - */
> > - list_splice_tail_init(&chan->ld_running, &ld_cleanup);
> > -
> > /* the hardware is now idle and ready for more */
> > chan->idle =3D true;
> >
> > - /*
> > - * Start any pending transactions automatically
> > - *
> > - * In the ideal case, we keep the DMA controller busy while we go
> > - * ahead and free the descriptors below.
> > - */
> > - fsl_chan_xfer_ld_queue(chan);
> > - spin_unlock_irqrestore(&chan->desc_lock, flags);
> > -
> > - /* Run the callback for each descriptor, in order */
> > - list_for_each_entry_safe(desc, _desc, &ld_cleanup, node) {
> > + /* Run all cleanup for this descriptor */
>=20
> Nitpick. This should be:
>=20
> /* Run cleanup for all descriptors */
No, this "all" means all operations for one descriptor but not "all descrip=
tors".
>=20
> > + fsldma_cleanup_descriptor(chan);
> >
> > - /* Remove from the list of transactions */
> > - list_del(&desc->node);
> > -
> > - /* Run all cleanup for this descriptor */
> > - fsldma_cleanup_descriptor(chan, desc);
> > - }
> > + spin_unlock_irqrestore(&chan->desc_lock, flags);
> >
> > chan_dbg(chan, "tasklet exit\n");
> > }
> > @@ -1262,6 +1361,7 @@ static int __devinit fsl_dma_chan_probe(struct
> fsldma_device *fdev,
> > spin_lock_init(&chan->desc_lock);
> > INIT_LIST_HEAD(&chan->ld_pending);
> > INIT_LIST_HEAD(&chan->ld_running);
> > + INIT_LIST_HEAD(&chan->ld_completed);
> > chan->idle =3D true;
> >
> > chan->common.device =3D &fdev->common; diff --git
> > a/drivers/dma/fsldma.h b/drivers/dma/fsldma.h index f5c3879..7ede908
> > 100644
> > --- a/drivers/dma/fsldma.h
> > +++ b/drivers/dma/fsldma.h
> > @@ -140,6 +140,7 @@ struct fsldma_chan {
> > spinlock_t desc_lock; /* Descriptor operation lock */
> > struct list_head ld_pending; /* Link descriptors queue */
> > struct list_head ld_running; /* Link descriptors queue */
> > + struct list_head ld_completed; /* Link descriptors queue */
>=20
> It may help to add some documentation here. It would have helped me to
> review this patch. Something like this:
>=20
> /*
> * Descriptors which are queued to run, but have not yet been handed
> * to the hardware for execution
> */
> struct list_head ld_pending;
>=20
> /*
> * Descriptors which are currently being executed by the hardware */
> struct list_head ld_running;
>=20
> /*
> * Descriptors which have finished execution by the hardware. These
> * descriptors have already had their cleanup actions run. They are
> * waiting for the ACK bit to be set by the async_tx API.
> */
> struct list_head ld_completed;
Ok, I will add these comments. Thanks.
>=20
> > struct dma_chan common; /* DMA common channel */
> > struct dma_pool *desc_pool; /* Descriptors pool */
> > struct device *dev; /* Channel device */
> > --
> > 1.7.5.1
> >
> >
> > _______________________________________________
> > Linuxppc-dev mailing list
> > Linuxppc-dev@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* [PATCH v2] powerpc: fix personality handling in ppc64_personality()
From: Jiri Kosina @ 2012-08-02 7:10 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Paul Mackerras, linuxppc-dev, linux-kernel
In-Reply-To: <m27gtitfv4.fsf@igel.home>
Directly comparing current->personality against PER_LINUX32 doesn't work
in cases when any of the personality flags stored in the top three bytes
are used.
Directly forcefully setting personality to PER_LINUX32 or PER_LINUX
discards any flags stored in the top three bytes
Use personality() macro to compare only PER_MASK bytes and make sure that
we are setting only the bits that should be set, instead of
overwriting the whole value.
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
changed since v1: fix the bit ops to reflect the fact that PER_LINUX is
actually 0
arch/powerpc/kernel/syscalls.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c
index f2496f2..dc1558e 100644
--- a/arch/powerpc/kernel/syscalls.c
+++ b/arch/powerpc/kernel/syscalls.c
@@ -107,11 +107,11 @@ long ppc64_personality(unsigned long personality)
long ret;
if (personality(current->personality) == PER_LINUX32
- && personality == PER_LINUX)
- personality = PER_LINUX32;
+ && personality(personality) == PER_LINUX)
+ personality |= PER_LINUX32;
ret = sys_personality(personality);
- if (ret == PER_LINUX32)
- ret = PER_LINUX;
+ if (personality(ret) == PER_LINUX32)
+ ret &= ~PER_LINUX32;
return ret;
}
#endif
--
1.7.3.1
--
Jiri Kosina
SUSE Labs
^ permalink raw reply related
* RE: [PATCH v5 5/6] fsl-dma: use spin_lock_bh to instead of spin_lock_irqsave
From: Liu Qiang-B32616 @ 2012-08-02 4:56 UTC (permalink / raw)
To: Ira W. Snyder
Cc: herbert@gondor.apana.org.au, Vinod Koul, Tabi Timur-B04825,
linux-kernel@vger.kernel.org, dan.j.williams@gmail.com,
linux-crypto@vger.kernel.org, Dan Williams,
linuxppc-dev@lists.ozlabs.org, davem@davemloft.net
In-Reply-To: <20120801173059.GF11359@ovro.caltech.edu>
> -----Original Message-----
> From: Ira W. Snyder [mailto:iws@ovro.caltech.edu]
> Sent: Thursday, August 02, 2012 1:31 AM
> To: Liu Qiang-B32616
> Cc: linux-crypto@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-
> kernel@vger.kernel.org; dan.j.williams@gmail.com; Vinod Koul; Tabi Timur-
> B04825; herbert@gondor.hengli.com.au; Dan Williams; davem@davemloft.net
> Subject: Re: [PATCH v5 5/6] fsl-dma: use spin_lock_bh to instead of
> spin_lock_irqsave
>=20
> On Wed, Aug 01, 2012 at 04:50:09PM +0800, qiang.liu@freescale.com wrote:
> > From: Qiang Liu <qiang.liu@freescale.com>
> >
> > - use spin_lock_bh() is the right way to use async_tx api,
> > dma_run_dependencies() should not be protected by spin_lock_irqsave();
> > - use spin_lock_bh to instead of spin_lock_irqsave for improving
> > performance, There is not any place to access descriptor queues in
> > fsl-dma ISR except its tasklet, spin_lock_bh() is more proper here.
> > Interrupts will be turned off and context will be save in irqsave,
> there is needless to use irqsave..
> >
>=20
> This description is not very clear English. I understand it is not your
> native language. Let me try to help.
>=20
> """
> The use of spin_lock_irqsave() is a stronger locking mechanism than is
> required throughout the driver. The minimum locking required should be
> used instead.
>=20
> Change all instances of spin_lock_irqsave() to spin_lock_bh(). All
> manipulation of protected fields is done using tasklet context or weaker,
> which makes spin_lock_bh() the correct choice.
> """
I will modify the description in v6, please check later.
>=20
> Other than that,
> Acked-by: Ira W. Snyder <iws@ovro.caltech.edu>
Thanks.
>=20
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: Vinod Koul <vinod.koul@intel.com>
> > Cc: Li Yang <leoli@freescale.com>
> > Cc: Timur Tabi <timur@freescale.com>
> > Signed-off-by: Qiang Liu <qiang.liu@freescale.com>
> > ---
> > drivers/dma/fsldma.c | 30 ++++++++++++------------------
> > 1 files changed, 12 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c index
> > bb883c0..e3814aa 100644
> > --- a/drivers/dma/fsldma.c
> > +++ b/drivers/dma/fsldma.c
> > @@ -645,10 +645,9 @@ static dma_cookie_t fsl_dma_tx_submit(struct
> dma_async_tx_descriptor *tx)
> > struct fsldma_chan *chan =3D to_fsl_chan(tx->chan);
> > struct fsl_desc_sw *desc =3D tx_to_fsl_desc(tx);
> > struct fsl_desc_sw *child;
> > - unsigned long flags;
> > dma_cookie_t cookie;
> >
> > - spin_lock_irqsave(&chan->desc_lock, flags);
> > + spin_lock_bh(&chan->desc_lock);
> >
> > /*
> > * assign cookies to all of the software descriptors @@ -661,7
> > +660,7 @@ static dma_cookie_t fsl_dma_tx_submit(struct
> dma_async_tx_descriptor *tx)
> > /* put this transaction onto the tail of the pending queue */
> > append_ld_queue(chan, desc);
> >
> > - spin_unlock_irqrestore(&chan->desc_lock, flags);
> > + spin_unlock_bh(&chan->desc_lock);
> >
> > return cookie;
> > }
> > @@ -770,15 +769,14 @@ static void fsldma_free_desc_list_reverse(struct
> > fsldma_chan *chan, static void fsl_dma_free_chan_resources(struct
> > dma_chan *dchan) {
> > struct fsldma_chan *chan =3D to_fsl_chan(dchan);
> > - unsigned long flags;
> >
> > chan_dbg(chan, "free all channel resources\n");
> > - spin_lock_irqsave(&chan->desc_lock, flags);
> > + spin_lock_bh(&chan->desc_lock);
> > fsldma_cleanup_descriptor(chan);
> > fsldma_free_desc_list(chan, &chan->ld_pending);
> > fsldma_free_desc_list(chan, &chan->ld_running);
> > fsldma_free_desc_list(chan, &chan->ld_completed);
> > - spin_unlock_irqrestore(&chan->desc_lock, flags);
> > + spin_unlock_bh(&chan->desc_lock);
> >
> > dma_pool_destroy(chan->desc_pool);
> > chan->desc_pool =3D NULL;
> > @@ -997,7 +995,6 @@ static int fsl_dma_device_control(struct dma_chan
> > *dchan, {
> > struct dma_slave_config *config;
> > struct fsldma_chan *chan;
> > - unsigned long flags;
> > int size;
> >
> > if (!dchan)
> > @@ -1007,7 +1004,7 @@ static int fsl_dma_device_control(struct
> > dma_chan *dchan,
> >
> > switch (cmd) {
> > case DMA_TERMINATE_ALL:
> > - spin_lock_irqsave(&chan->desc_lock, flags);
> > + spin_lock_bh(&chan->desc_lock);
> >
> > /* Halt the DMA engine */
> > dma_halt(chan);
> > @@ -1017,7 +1014,7 @@ static int fsl_dma_device_control(struct dma_chan
> *dchan,
> > fsldma_free_desc_list(chan, &chan->ld_running);
> > chan->idle =3D true;
> >
> > - spin_unlock_irqrestore(&chan->desc_lock, flags);
> > + spin_unlock_bh(&chan->desc_lock);
> > return 0;
> >
> > case DMA_SLAVE_CONFIG:
> > @@ -1059,11 +1056,10 @@ static int fsl_dma_device_control(struct
> > dma_chan *dchan, static void fsl_dma_memcpy_issue_pending(struct
> > dma_chan *dchan) {
> > struct fsldma_chan *chan =3D to_fsl_chan(dchan);
> > - unsigned long flags;
> >
> > - spin_lock_irqsave(&chan->desc_lock, flags);
> > + spin_lock_bh(&chan->desc_lock);
> > fsl_chan_xfer_ld_queue(chan);
> > - spin_unlock_irqrestore(&chan->desc_lock, flags);
> > + spin_unlock_bh(&chan->desc_lock);
> > }
> >
> > /**
> > @@ -1076,15 +1072,14 @@ static enum dma_status fsl_tx_status(struct
> > dma_chan *dchan, {
> > struct fsldma_chan *chan =3D to_fsl_chan(dchan);
> > enum dma_status ret;
> > - unsigned long flags;
> >
> > ret =3D dma_cookie_status(dchan, cookie, txstate);
> > if (ret =3D=3D DMA_SUCCESS)
> > return ret;
> >
> > - spin_lock_irqsave(&chan->desc_lock, flags);
> > + spin_lock_bh(&chan->desc_lock);
> > fsldma_cleanup_descriptor(chan);
> > - spin_unlock_irqrestore(&chan->desc_lock, flags);
> > + spin_unlock_bh(&chan->desc_lock);
> >
> > return dma_cookie_status(dchan, cookie, txstate); } @@ -1163,11
> > +1158,10 @@ static irqreturn_t fsldma_chan_irq(int irq, void *data)
> > static void dma_do_tasklet(unsigned long data) {
> > struct fsldma_chan *chan =3D (struct fsldma_chan *)data;
> > - unsigned long flags;
> >
> > chan_dbg(chan, "tasklet entry\n");
> >
> > - spin_lock_irqsave(&chan->desc_lock, flags);
> > + spin_lock_bh(&chan->desc_lock);
> >
> > /* the hardware is now idle and ready for more */
> > chan->idle =3D true;
> > @@ -1175,7 +1169,7 @@ static void dma_do_tasklet(unsigned long data)
> > /* Run all cleanup for this descriptor */
> > fsldma_cleanup_descriptor(chan);
> >
> > - spin_unlock_irqrestore(&chan->desc_lock, flags);
> > + spin_unlock_bh(&chan->desc_lock);
> >
> > chan_dbg(chan, "tasklet exit\n");
> > }
> > --
> > 1.7.5.1
> >
> >
> > _______________________________________________
> > Linuxppc-dev mailing list
> > Linuxppc-dev@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* RE: [PATCH v5 4/6] fsl-dma: move the function ahead of its invoke function
From: Liu Qiang-B32616 @ 2012-08-02 4:54 UTC (permalink / raw)
To: Ira W. Snyder
Cc: Vinod Koul, linux-kernel@vger.kernel.org,
dan.j.williams@gmail.com, herbert@gondor.hengli.com.au,
linux-crypto@vger.kernel.org, Dan Williams,
linuxppc-dev@lists.ozlabs.org, davem@davemloft.net
In-Reply-To: <20120801163125.GB11359@ovro.caltech.edu>
> -----Original Message-----
> From: Ira W. Snyder [mailto:iws@ovro.caltech.edu]
> Sent: Thursday, August 02, 2012 12:31 AM
> To: Liu Qiang-B32616
> Cc: linux-crypto@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-
> kernel@vger.kernel.org; dan.j.williams@gmail.com; Vinod Koul;
> herbert@gondor.hengli.com.au; Dan Williams; davem@davemloft.net
> Subject: Re: [PATCH v5 4/6] fsl-dma: move the function ahead of its
> invoke function
>=20
> On Wed, Aug 01, 2012 at 04:49:43PM +0800, qiang.liu@freescale.com wrote:
> > From: Qiang Liu <qiang.liu@freescale.com>
> >
> > Move the function fsldma_cleanup_descriptor() and
> fsl_chan_xfer_ld_queue()
> > ahead of its invoke function for avoiding redundant definition.
> >
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: Vinod Koul <vinod.koul@intel.com>
> > Cc: Li Yang <leoli@freescale.com>
> > Signed-off-by: Qiang Liu <qiang.liu@freescale.com>
> > ---
> > drivers/dma/fsldma.c | 252 +++++++++++++++++++++++++-----------------
> --------
> > 1 files changed, 124 insertions(+), 128 deletions(-)
> >
> > diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
> > index 87f52c0..bb883c0 100644
> > --- a/drivers/dma/fsldma.c
> > +++ b/drivers/dma/fsldma.c
> > @@ -400,9 +400,6 @@ out_splice:
> > list_splice_tail_init(&desc->tx_list, &chan->ld_pending);
> > }
> >
> > -static void fsldma_cleanup_descriptor(struct fsldma_chan *chan);
> > -static void fsl_chan_xfer_ld_queue(struct fsldma_chan *chan);
> > -
>=20
> Please swap the order of this patch (patch 4/6) and the previous patch
> (patch 3/6).
>=20
> You added these lines in the patch 3/6 and deleted them here. If you
> reverse the order of the patches, this doesn't happen.
>=20
> Adding lines only to delete them in the next patch should be avoided.
I will swap the order in v6. Thanks.
>=20
> > /**
> > * fsldma_clean_completed_descriptor - free all descriptors which
> > * has been completed and acked
> > @@ -519,6 +516,130 @@ fsldma_clean_running_descriptor(struct
> fsldma_chan *chan,
> > return 0;
> > }
> >
> > +/**
> > + * fsl_chan_xfer_ld_queue - transfer any pending transactions
> > + * @chan : Freescale DMA channel
> > + *
> > + * HARDWARE STATE: idle
> > + * LOCKING: must hold chan->desc_lock
> > + */
> > +static void fsl_chan_xfer_ld_queue(struct fsldma_chan *chan)
> > +{
> > + struct fsl_desc_sw *desc;
> > +
> > + /*
> > + * If the list of pending descriptors is empty, then we
> > + * don't need to do any work at all
> > + */
> > + if (list_empty(&chan->ld_pending)) {
> > + chan_dbg(chan, "no pending LDs\n");
> > + return;
> > + }
> > +
> > + /*
> > + * The DMA controller is not idle, which means that the interrupt
> > + * handler will start any queued transactions when it runs after
> > + * this transaction finishes
> > + */
> > + if (!chan->idle) {
> > + chan_dbg(chan, "DMA controller still busy\n");
> > + return;
> > + }
> > +
> > + /*
> > + * If there are some link descriptors which have not been
> > + * transferred, we need to start the controller
> > + */
> > +
> > + /*
> > + * Move all elements from the queue of pending transactions
> > + * onto the list of running transactions
> > + */
> > + chan_dbg(chan, "idle, starting controller\n");
> > + desc =3D list_first_entry(&chan->ld_pending, struct fsl_desc_sw,
> node);
> > + list_splice_tail_init(&chan->ld_pending, &chan->ld_running);
> > +
> > + /*
> > + * The 85xx DMA controller doesn't clear the channel start bit
> > + * automatically at the end of a transfer. Therefore we must clear
> > + * it in software before starting the transfer.
> > + */
> > + if ((chan->feature & FSL_DMA_IP_MASK) =3D=3D FSL_DMA_IP_85XX) {
> > + u32 mode;
> > +
> > + mode =3D DMA_IN(chan, &chan->regs->mr, 32);
> > + mode &=3D ~FSL_DMA_MR_CS;
> > + DMA_OUT(chan, &chan->regs->mr, mode, 32);
> > + }
> > +
> > + /*
> > + * Program the descriptor's address into the DMA controller,
> > + * then start the DMA transaction
> > + */
> > + set_cdar(chan, desc->async_tx.phys);
> > + get_cdar(chan);
> > +
> > + dma_start(chan);
> > + chan->idle =3D false;
> > +}
> > +
> > +/**
> > + * fsldma_cleanup_descriptor - cleanup and free a single link
> descriptor
> > + * @chan: Freescale DMA channel
> > + * @desc: descriptor to cleanup and free
> > + *
> > + * This function is used on a descriptor which has been executed by
> the DMA
> > + * controller. It will run any callbacks, submit any dependencies, and
> then
> > + * free the descriptor.
> > + */
> > +static void fsldma_cleanup_descriptor(struct fsldma_chan *chan)
> > +{
> > + struct fsl_desc_sw *desc, *_desc;
> > + dma_cookie_t cookie =3D 0;
> > + dma_addr_t curr_phys =3D get_cdar(chan);
> > + int idle =3D dma_is_idle(chan);
> > + int seen_current =3D 0;
> > +
> > + fsldma_clean_completed_descriptor(chan);
> > +
> > + /* Run the callback for each descriptor, in order */
> > + list_for_each_entry_safe(desc, _desc, &chan->ld_running, node) {
> > + /*
> > + * do not advance past the current descriptor loaded into the
> > + * hardware channel, subsequent descriptors are either in
> > + * process or have not been submitted
> > + */
> > + if (seen_current)
> > + break;
> > +
> > + /*
> > + * stop the search if we reach the current descriptor and the
> > + * channel is busy
> > + */
> > + if (desc->async_tx.phys =3D=3D curr_phys) {
> > + seen_current =3D 1;
> > + if (!idle)
> > + break;
> > + }
> > +
> > + cookie =3D fsldma_run_tx_complete_actions(desc, chan, cookie);
> > +
> > + if (fsldma_clean_running_descriptor(chan, desc))
> > + break;
> > + }
> > +
> > + /*
> > + * Start any pending transactions automatically
> > + *
> > + * In the ideal case, we keep the DMA controller busy while we go
> > + * ahead and free the descriptors below.
> > + */
> > + fsl_chan_xfer_ld_queue(chan);
> > +
> > + if (cookie > 0)
> > + chan->common.completed_cookie =3D cookie;
> > +}
> > +
> > static dma_cookie_t fsl_dma_tx_submit(struct dma_async_tx_descriptor
> *tx)
> > {
> > struct fsldma_chan *chan =3D to_fsl_chan(tx->chan);
> > @@ -932,131 +1053,6 @@ static int fsl_dma_device_control(struct
> dma_chan *dchan,
> > }
> >
> > /**
> > - * fsldma_cleanup_descriptor - cleanup and free a single link
> descriptor
> > - * @chan: Freescale DMA channel
> > - * @desc: descriptor to cleanup and free
> > - *
> > - * This function is used on a descriptor which has been executed by
> the DMA
> > - * controller. It will run any callbacks, submit any dependencies, and
> then
> > - * free the descriptor.
> > - */
> > -static void fsldma_cleanup_descriptor(struct fsldma_chan *chan)
> > -{
> > - struct fsl_desc_sw *desc, *_desc;
> > - dma_cookie_t cookie =3D 0;
> > - dma_addr_t curr_phys =3D get_cdar(chan);
> > - int idle =3D dma_is_idle(chan);
> > - int seen_current =3D 0;
> > -
> > - fsldma_clean_completed_descriptor(chan);
> > -
> > - /* Run the callback for each descriptor, in order */
> > - list_for_each_entry_safe(desc, _desc, &chan->ld_running, node) {
> > - /*
> > - * do not advance past the current descriptor loaded into the
> > - * hardware channel, subsequent descriptors are either in
> > - * process or have not been submitted
> > - */
> > - if (seen_current)
> > - break;
> > -
> > - /*
> > - * stop the search if we reach the current descriptor and the
> > - * channel is busy
> > - */
> > - if (desc->async_tx.phys =3D=3D curr_phys) {
> > - seen_current =3D 1;
> > - if (!idle)
> > - break;
> > - }
> > -
> > - cookie =3D fsldma_run_tx_complete_actions(desc, chan, cookie);
> > -
> > - if (fsldma_clean_running_descriptor(chan, desc))
> > - break;
> > -
> > - }
> > -
> > - /*
> > - * Start any pending transactions automatically
> > - *
> > - * In the ideal case, we keep the DMA controller busy while we go
> > - * ahead and free the descriptors below.
> > - */
> > - fsl_chan_xfer_ld_queue(chan);
> > -
> > - if (cookie > 0)
> > - chan->common.completed_cookie =3D cookie;
> > -}
> > -
> > -/**
> > - * fsl_chan_xfer_ld_queue - transfer any pending transactions
> > - * @chan : Freescale DMA channel
> > - *
> > - * HARDWARE STATE: idle
> > - * LOCKING: must hold chan->desc_lock
> > - */
> > -static void fsl_chan_xfer_ld_queue(struct fsldma_chan *chan)
> > -{
> > - struct fsl_desc_sw *desc;
> > -
> > - /*
> > - * If the list of pending descriptors is empty, then we
> > - * don't need to do any work at all
> > - */
> > - if (list_empty(&chan->ld_pending)) {
> > - chan_dbg(chan, "no pending LDs\n");
> > - return;
> > - }
> > -
> > - /*
> > - * The DMA controller is not idle, which means that the interrupt
> > - * handler will start any queued transactions when it runs after
> > - * this transaction finishes
> > - */
> > - if (!chan->idle) {
> > - chan_dbg(chan, "DMA controller still busy\n");
> > - return;
> > - }
> > -
> > - /*
> > - * If there are some link descriptors which have not been
> > - * transferred, we need to start the controller
> > - */
> > -
> > - /*
> > - * Move all elements from the queue of pending transactions
> > - * onto the list of running transactions
> > - */
> > - chan_dbg(chan, "idle, starting controller\n");
> > - desc =3D list_first_entry(&chan->ld_pending, struct fsl_desc_sw,
> node);
> > - list_splice_tail_init(&chan->ld_pending, &chan->ld_running);
> > -
> > - /*
> > - * The 85xx DMA controller doesn't clear the channel start bit
> > - * automatically at the end of a transfer. Therefore we must clear
> > - * it in software before starting the transfer.
> > - */
> > - if ((chan->feature & FSL_DMA_IP_MASK) =3D=3D FSL_DMA_IP_85XX) {
> > - u32 mode;
> > -
> > - mode =3D DMA_IN(chan, &chan->regs->mr, 32);
> > - mode &=3D ~FSL_DMA_MR_CS;
> > - DMA_OUT(chan, &chan->regs->mr, mode, 32);
> > - }
> > -
> > - /*
> > - * Program the descriptor's address into the DMA controller,
> > - * then start the DMA transaction
> > - */
> > - set_cdar(chan, desc->async_tx.phys);
> > - get_cdar(chan);
> > -
> > - dma_start(chan);
> > - chan->idle =3D false;
> > -}
> > -
> > -/**
> > * fsl_dma_memcpy_issue_pending - Issue the DMA start command
> > * @chan : Freescale DMA channel
> > */
> > --
> > 1.7.5.1
> >
> >
> > _______________________________________________
> > Linuxppc-dev mailing list
> > Linuxppc-dev@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* RE: [PATCH v5 2/6] fsl-dma: remove attribute DMA_INTERRUPT of dmaengine
From: Liu Qiang-B32616 @ 2012-08-02 4:52 UTC (permalink / raw)
To: Ira W. Snyder
Cc: herbert@gondor.apana.org.au, Vinod Koul,
linux-kernel@vger.kernel.org, dan.j.williams@gmail.com,
linux-crypto@vger.kernel.org, Dan Williams,
linuxppc-dev@lists.ozlabs.org, davem@davemloft.net
In-Reply-To: <20120801163553.GC11359@ovro.caltech.edu>
> -----Original Message-----
> From: Ira W. Snyder [mailto:iws@ovro.caltech.edu]
> Sent: Thursday, August 02, 2012 12:36 AM
> To: Liu Qiang-B32616
> Cc: linux-crypto@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; linux-
> kernel@vger.kernel.org; dan.j.williams@gmail.com; Vinod Koul;
> herbert@gondor.hengli.com.au; Dan Williams; davem@davemloft.net
> Subject: Re: [PATCH v5 2/6] fsl-dma: remove attribute DMA_INTERRUPT of
> dmaengine
>=20
> On Wed, Aug 01, 2012 at 04:49:08PM +0800, qiang.liu@freescale.com wrote:
> > From: Qiang Liu <qiang.liu@freescale.com>
> >
> > Delete attribute DMA_INTERRUPT because fsl-dma doesn't support this
> > function, exception will be thrown if talitos is used to offload xor at
> the same time.
> >
>=20
> I have no problem with this patch.
>=20
> However, it ***WILL BREAK*** both drivers in drivers/misc/carma. Please
> add my patch 7/7 titled "[PATCH 7/7] carma: remove unnecessary
> DMA_INTERRUPT capability" to your series. I suggest placing it
> immediately after this patch in your series.
>=20
> The carma drivers use the fsldma driver exclusively.
Fine, thanks.
>=20
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: Vinod Koul <vinod.koul@intel.com>
> > Cc: Li Yang <leoli@freescale.com>
> > Signed-off-by: Qiang Liu <qiang.liu@freescale.com>
> > Acked-by: Ira W. Snyder <iws@ovro.caltech.edu>
> > ---
> > drivers/dma/fsldma.c | 31 -------------------------------
> > 1 files changed, 0 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c index
> > 8f84761..4f2f212 100644
> > --- a/drivers/dma/fsldma.c
> > +++ b/drivers/dma/fsldma.c
> > @@ -543,35 +543,6 @@ static void fsl_dma_free_chan_resources(struct
> > dma_chan *dchan) }
> >
> > static struct dma_async_tx_descriptor *
> > -fsl_dma_prep_interrupt(struct dma_chan *dchan, unsigned long flags)
> > -{
> > - struct fsldma_chan *chan;
> > - struct fsl_desc_sw *new;
> > -
> > - if (!dchan)
> > - return NULL;
> > -
> > - chan =3D to_fsl_chan(dchan);
> > -
> > - new =3D fsl_dma_alloc_descriptor(chan);
> > - if (!new) {
> > - chan_err(chan, "%s\n", msg_ld_oom);
> > - return NULL;
> > - }
> > -
> > - new->async_tx.cookie =3D -EBUSY;
> > - new->async_tx.flags =3D flags;
> > -
> > - /* Insert the link descriptor to the LD ring */
> > - list_add_tail(&new->node, &new->tx_list);
> > -
> > - /* Set End-of-link to the last link descriptor of new list */
> > - set_ld_eol(chan, new);
> > -
> > - return &new->async_tx;
> > -}
> > -
> > -static struct dma_async_tx_descriptor * fsl_dma_prep_memcpy(struct
> > dma_chan *dchan,
> > dma_addr_t dma_dst, dma_addr_t dma_src,
> > size_t len, unsigned long flags)
> > @@ -1352,12 +1323,10 @@ static int __devinit fsldma_of_probe(struct
> platform_device *op)
> > fdev->irq =3D irq_of_parse_and_map(op->dev.of_node, 0);
> >
> > dma_cap_set(DMA_MEMCPY, fdev->common.cap_mask);
> > - dma_cap_set(DMA_INTERRUPT, fdev->common.cap_mask);
> > dma_cap_set(DMA_SG, fdev->common.cap_mask);
> > dma_cap_set(DMA_SLAVE, fdev->common.cap_mask);
> > fdev->common.device_alloc_chan_resources =3D
> fsl_dma_alloc_chan_resources;
> > fdev->common.device_free_chan_resources =3D
> fsl_dma_free_chan_resources;
> > - fdev->common.device_prep_dma_interrupt =3D fsl_dma_prep_interrupt;
> > fdev->common.device_prep_dma_memcpy =3D fsl_dma_prep_memcpy;
> > fdev->common.device_prep_dma_sg =3D fsl_dma_prep_sg;
> > fdev->common.device_tx_status =3D fsl_tx_status;
> > --
> > 1.7.5.1
> >
> >
> > _______________________________________________
> > Linuxppc-dev mailing list
> > Linuxppc-dev@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* Re: [PATCH -V6 01/12] arch/powerpc: Replace open coded CONTEXT_BITS value
From: Paul Mackerras @ 2012-08-02 0:00 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: linuxppc-dev
In-Reply-To: <1343837623-9046-2-git-send-email-aneesh.kumar@linux.vnet.ibm.com>
On Wed, Aug 01, 2012 at 09:43:32PM +0530, Aneesh Kumar K.V wrote:
> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
>
> To clarify the meaning for future readers, replace the open coded
> 19 with CONTEXT_BITS
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
Reviewed-by: Paul Mackerras <paulus@samba.org>
^ permalink raw reply
* Re: [PATCH] powerpc: fix personality handling in ppc64_personality()
From: Andreas Schwab @ 2012-08-01 22:19 UTC (permalink / raw)
To: Jiri Kosina; +Cc: Paul Mackerras, linuxppc-dev, linux-kernel
In-Reply-To: <alpine.LNX.2.00.1208012201520.14910@pobox.suse.cz>
Jiri Kosina <jkosina@suse.cz> writes:
> if (personality(current->personality) == PER_LINUX32
> - && personality == PER_LINUX)
> - personality = PER_LINUX32;
> + && personality(personality) == PER_LINUX)
> + personality &= ~PER_LINUX | PER_LINUX32;
That doesn't work. ~PER_LINUX is -1, so this is a no-op.
> ret = sys_personality(personality);
> - if (ret == PER_LINUX32)
> - ret = PER_LINUX;
> + if (personality(ret) == PER_LINUX32)
> + ret &= ~PER_LINUX32 | PER_LINUX;
That only "works" because PER_LINUX is 0.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply
* [PATCH] powerpc: fix personality handling in ppc64_personality()
From: Jiri Kosina @ 2012-08-01 20:03 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras; +Cc: linuxppc-dev, linux-kernel
Directly comparing current->personality against PER_LINUX32 doesn't work
in cases when any of the personality flags stored in the top three bytes
are used.
Directly forcefully setting personality to PER_LINUX32 or PER_LINUX
discards any flags stored in the top three bytes
Use personality() macro to compare only PER_MASK bytes and make sure that
we are setting only the bits that should be set, instead of
overwriting the whole value.
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
Found accidentally. Untested, I don't have the hardware.
arch/powerpc/kernel/syscalls.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/kernel/syscalls.c b/arch/powerpc/kernel/syscalls.c
index f2496f2..4dcc7c6 100644
--- a/arch/powerpc/kernel/syscalls.c
+++ b/arch/powerpc/kernel/syscalls.c
@@ -107,11 +107,11 @@ long ppc64_personality(unsigned long personality)
long ret;
if (personality(current->personality) == PER_LINUX32
- && personality == PER_LINUX)
- personality = PER_LINUX32;
+ && personality(personality) == PER_LINUX)
+ personality &= ~PER_LINUX | PER_LINUX32;
ret = sys_personality(personality);
- if (ret == PER_LINUX32)
- ret = PER_LINUX;
+ if (personality(ret) == PER_LINUX32)
+ ret &= ~PER_LINUX32 | PER_LINUX;
return ret;
}
#endif
--
Jiri Kosina
SUSE Labs
^ permalink raw reply related
* Problem in phy.c, when using fixed network speed
From: Michael Koch @ 2012-08-01 18:14 UTC (permalink / raw)
To: linuxppc-dev
Hi all,
during testing i encountered a problem with setting up a 5200B
controller with a MICREL phy at static 100MBit full duplex - without
autonegotiation.
I performed this as usual with ethtool and was succesful when i had my
link partner up, providing a link.
When kepping the link partner off, meaning no link at all, my machine
started to degrade its link capabilities ending 10MBit half duplex.
I tracked it down to drivers/net/phy/phy.c:
in the function phy_state_machine, the case block PHY_FORCING causes
this (at least for me) undesired behaviour. Calling the
phy_force_reduction function degrades actually an intentionally static
setup.
I deactivated those lines, and it works for me.
But anyhow i feel soe need to have a general solution that takes static
non autonagotiation setups into account.
What do you think?
Hope to hear from you
Michael
^ 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