* 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
* Re: [PATCH V3 1/5] powerpc/fsl-pci: Unify pci/pcie initialization code
From: Joakim Tjernlund @ 2012-08-01 17:42 UTC (permalink / raw)
To: Kumar Gala; +Cc: B07421, linuxppc-dev, Jia Hongtao
In-Reply-To: <F5E29818-CC20-444E-A125-249E5E3BA365@kernel.crashing.org>
>
>
> On Jul 26, 2012, at 7:30 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.
> >
> > One issue introduced by this architecture is the timing of swiotlb_init.
> > During PCI initialization the need of swiotlb is determined and this should
> > be done before swiotlb_init. So a new function to determine swiotlb by
> > parsing pci ranges is made. This function is called at board_setup_arch
> > stage which is earlier than swiotlb_init.
> >
> > Signed-off-by: Jia Hongtao <B38951@freescale.com>
> > Signed-off-by: Li Yang <leoli@freescale.com>
> > ---
> > Changed for V3:
> > - Rebase the patch set on the latest tree
> > - merge PCI unify and swiotlb patch into one
> >
> > arch/powerpc/sysdev/fsl_pci.c | 155 ++++++++++++++++++++++++++++++++---------
> > arch/powerpc/sysdev/fsl_pci.h | 9 +--
> > 2 files changed, 125 insertions(+), 39 deletions(-)
>
> I'd like the SWIOTLB refactoring as a separate patch. Additionally, the order of patches should be as follows:
>
> 1. refactor PCI node parsing code
> 2. add pci_determine_swiotlb (should rename to fsl_pci_determine_swiotlb)
> 3. Determine primary bus by looking for ISA node
> 4. convert all boards over to fsl_pci_init
> 5. convert fsl pci to platform driver (edac and other fixes should be merged in here)
> 6. PM support
Could you add:
7. Fix PPC_INDIRECT_TYPE_NO_PCIE_LINK issue. The link is only probed at init and a subsequent
pci/rescan does not change that. See mail thread titled:
mpc8xxx PCIe hotplug needs fixing, some clues ..
Jocke
^ permalink raw reply
* Re: [PATCH v5 5/6] fsl-dma: use spin_lock_bh to instead of spin_lock_irqsave
From: Ira W. Snyder @ 2012-08-01 17:30 UTC (permalink / raw)
To: qiang.liu
Cc: Vinod Koul, linux-kernel, davem, dan.j.williams, herbert,
linux-crypto, Dan Williams, linuxppc-dev, Timur Tabi
In-Reply-To: <1343811009-25466-1-git-send-email-qiang.liu@freescale.com>
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..
>
This description is not very clear English. I understand it is not your
native language. Let me try to help.
"""
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.
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.
"""
Other than that,
Acked-by: Ira W. Snyder <iws@ovro.caltech.edu>
> 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 = to_fsl_chan(tx->chan);
> struct fsl_desc_sw *desc = 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 = 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 = 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 = 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 = 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 = to_fsl_chan(dchan);
> enum dma_status ret;
> - unsigned long flags;
>
> ret = dma_cookie_status(dchan, cookie, txstate);
> if (ret == 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 = (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 = 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 6/6] fsl-dma: fix a warning of unitialized cookie
From: Ira W. Snyder @ 2012-08-01 17:25 UTC (permalink / raw)
To: qiang.liu
Cc: Vinod Koul, linux-kernel, dan.j.williams, herbert, linux-crypto,
Dan Williams, linuxppc-dev, davem
In-Reply-To: <1343811027-25516-1-git-send-email-qiang.liu@freescale.com>
On Wed, Aug 01, 2012 at 04:50:27PM +0800, qiang.liu@freescale.com wrote:
> From: Qiang Liu <qiang.liu@freescale.com>
>
> Fix a warning of unitialized value when compile with -Wuninitialized.
>
Looks good to me.
Acked-by: Ira W. Snyder <iws@ovro.caltech.edu>
> 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>
> Reported-by: Kim Phillips <kim.phillips@freescale.com>
> ---
> drivers/dma/fsldma.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
> index e3814aa..6fc22eb 100644
> --- a/drivers/dma/fsldma.c
> +++ b/drivers/dma/fsldma.c
> @@ -645,7 +645,7 @@ static dma_cookie_t fsl_dma_tx_submit(struct dma_async_tx_descriptor *tx)
> struct fsldma_chan *chan = to_fsl_chan(tx->chan);
> struct fsl_desc_sw *desc = tx_to_fsl_desc(tx);
> struct fsl_desc_sw *child;
> - dma_cookie_t cookie;
> + dma_cookie_t cookie = 0;
>
> spin_lock_bh(&chan->desc_lock);
>
> --
> 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 3/6] fsl-dma: change release process of dma descriptor for supporting async_tx
From: Ira W. Snyder @ 2012-08-01 17:25 UTC (permalink / raw)
To: qiang.liu
Cc: Vinod Koul, linux-kernel, dan.j.williams, herbert, linux-crypto,
Dan Williams, linuxppc-dev, davem
In-Reply-To: <1343810957-25378-1-git-send-email-qiang.liu@freescale.com>
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.
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?
> 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.
> +/**
> + * 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.
> +{
> + 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.
> + }
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * fsldma_run_tx_complete_actions - cleanup and free a single link descriptor
This documentation is incorrect. This code NEVER frees a descriptor.
> + * @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.
> +{
> + 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.
> + 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'.
> +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.
> -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.
> {
> - 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;
}
> +
> + 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.
/*
* 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 */
> + 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;
> 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
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