* [PATCH 1/2] powerpc/44x: Fix PCI MSI support for APM821xx SoC and Bluestone board
@ 2012-03-06 3:29 Mai La
2012-03-06 4:37 ` [1/2] " Milton Miller
0 siblings, 1 reply; 6+ messages in thread
From: Mai La @ 2012-03-06 3:29 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Josh Boyer, Matt Porter,
Tirumala R Marri, Grant Likely, Michael Neuling, Kumar Gala,
Anton Blanchard, linuxppc-dev, linux-kernel
Cc: open-source-review, Mai La
This patch consists of:
- Enable PCI MSI as default for Bluestone board
- Define number of MSI interrupt for Maui APM821xx SoC using in Bluestone board
- Fix returning ENODEV as finding MSI node
- Fix MSI physical high and low address
- Keep MSI data logically
Signed-off-by: Mai La <mla@apm.com>
---
arch/powerpc/platforms/44x/Kconfig | 2 ++
arch/powerpc/sysdev/ppc4xx_msi.c | 28 ++++++++++++++++++----------
2 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/platforms/44x/Kconfig b/arch/powerpc/platforms/44x/Kconfig
index fcf6bf2..9f04ce3 100644
--- a/arch/powerpc/platforms/44x/Kconfig
+++ b/arch/powerpc/platforms/44x/Kconfig
@@ -23,6 +23,8 @@ config BLUESTONE
default n
select PPC44x_SIMPLE
select APM821xx
+ select PCI_MSI
+ select PPC4xx_MSI
select IBM_EMAC_RGMII
help
This option enables support for the APM APM821xx Evaluation board.
diff --git a/arch/powerpc/sysdev/ppc4xx_msi.c b/arch/powerpc/sysdev/ppc4xx_msi.c
index 1c2d7af..6103908 100644
--- a/arch/powerpc/sysdev/ppc4xx_msi.c
+++ b/arch/powerpc/sysdev/ppc4xx_msi.c
@@ -31,7 +31,7 @@
#include <asm/prom.h>
#include <asm/hw_irq.h>
#include <asm/ppc-pci.h>
-#include <boot/dcr.h>
+#include <asm/dcr.h>
#include <asm/dcr-regs.h>
#include <asm/msi_bitmap.h>
@@ -43,7 +43,12 @@
#define PEIH_FLUSH0 0x30
#define PEIH_FLUSH1 0x38
#define PEIH_CNTRST 0x48
+
+#ifdef CONFIG_APM821xx
+#define NR_MSI_IRQS 8
+#else
#define NR_MSI_IRQS 4
+#endif
struct ppc4xx_msi {
u32 msi_addr_lo;
@@ -150,12 +155,11 @@ static int ppc4xx_setup_pcieh_hw(struct platform_device *dev,
if (!sdr_addr)
return -1;
- SDR0_WRITE(sdr_addr, (u64)res.start >> 32); /*HIGH addr */
- SDR0_WRITE(sdr_addr + 1, res.start & 0xFFFFFFFF); /* Low addr */
-
+ mtdcri(SDR0, *sdr_addr, (u64)res.start >> 32); /*HIGH addr */
+ mtdcri(SDR0, *sdr_addr + 1, res.start & 0xFFFFFFFF);/* Low addr */
msi->msi_dev = of_find_node_by_name(NULL, "ppc4xx-msi");
- if (msi->msi_dev)
+ if (!msi->msi_dev)
return -ENODEV;
msi->msi_regs = of_iomap(msi->msi_dev, 0);
@@ -167,9 +171,12 @@ static int ppc4xx_setup_pcieh_hw(struct platform_device *dev,
(u32) (msi->msi_regs + PEIH_TERMADH), (u32) (msi->msi_regs));
msi_virt = dma_alloc_coherent(&dev->dev, 64, &msi_phys, GFP_KERNEL);
- msi->msi_addr_hi = 0x0;
- msi->msi_addr_lo = (u32) msi_phys;
- dev_dbg(&dev->dev, "PCIE-MSI: msi address 0x%x\n", msi->msi_addr_lo);
+ if (!msi_virt)
+ return -ENOMEM;
+ msi->msi_addr_hi = (u32)(msi_phys >> 32);
+ msi->msi_addr_lo = (u32)(msi_phys & 0xffffffff);
+ dev_dbg(&dev->dev, "PCIE-MSI: msi address high 0x%x, low 0x%x\n",
+ msi->msi_addr_hi, msi->msi_addr_lo);
/* Progam the Interrupt handler Termination addr registers */
out_be32(msi->msi_regs + PEIH_TERMADH, msi->msi_addr_hi);
@@ -185,6 +192,8 @@ static int ppc4xx_setup_pcieh_hw(struct platform_device *dev,
out_be32(msi->msi_regs + PEIH_MSIED, *msi_data);
out_be32(msi->msi_regs + PEIH_MSIMK, *msi_mask);
+ dma_free_coherent(&dev->dev, 64, msi_virt, msi_phys);
+
return 0;
}
@@ -215,8 +224,6 @@ static int __devinit ppc4xx_msi_probe(struct platform_device *dev)
struct resource res;
int err = 0;
- msi = &ppc4xx_msi;/*keep the msi data for further use*/
-
dev_dbg(&dev->dev, "PCIE-MSI: Setting up MSI support...\n");
msi = kzalloc(sizeof(struct ppc4xx_msi), GFP_KERNEL);
@@ -242,6 +249,7 @@ static int __devinit ppc4xx_msi_probe(struct platform_device *dev)
dev_err(&dev->dev, "Error allocating MSI bitmap\n");
goto error_out;
}
+ ppc4xx_msi = *msi;
ppc_md.setup_msi_irqs = ppc4xx_setup_msi_irqs;
ppc_md.teardown_msi_irqs = ppc4xx_teardown_msi_irqs;
--
1.7.3.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [1/2] powerpc/44x: Fix PCI MSI support for APM821xx SoC and Bluestone board
2012-03-06 3:29 [PATCH 1/2] powerpc/44x: Fix PCI MSI support for APM821xx SoC and Bluestone board Mai La
@ 2012-03-06 4:37 ` Milton Miller
2012-03-06 7:11 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 6+ messages in thread
From: Milton Miller @ 2012-03-06 4:37 UTC (permalink / raw)
To: Mai La
Cc: Michael Neuling, open-source-review, Tirumala R Marri,
linux-kernel, Josh Boyer, Anton Blanchard, Paul Mackerras,
linuxppc-dev
On Mon, 05 Mar 2012 about 17:29:41 -0000, Mai La wrote:
>
> @@ -43,7 +43,12 @@
> #define PEIH_FLUSH0 0x30
> #define PEIH_FLUSH1 0x38
> #define PEIH_CNTRST 0x48
> +
> +#ifdef CONFIG_APM821xx
> +#define NR_MSI_IRQS 8
> +#else
> #define NR_MSI_IRQS 4
> +#endif
>
does this need to go into the dts binding?
> struct ppc4xx_msi {
> u32 msi_addr_lo;
> @@ -150,12 +155,11 @@ static int ppc4xx_setup_pcieh_hw(struct platform_device *dev,
> if (!sdr_addr)
> return -1;
>
> - SDR0_WRITE(sdr_addr, (u64)res.start >> 32); /*HIGH addr */
> - SDR0_WRITE(sdr_addr + 1, res.start & 0xFFFFFFFF); /* Low addr */
> -
> + mtdcri(SDR0, *sdr_addr, (u64)res.start >> 32); /*HIGH addr */
> + mtdcri(SDR0, *sdr_addr + 1, res.start & 0xFFFFFFFF);/* Low addr */
Please use upper_32_bits and lower_32_bits from linux/kernel.h
> msi->msi_dev = of_find_node_by_name(NULL, "ppc4xx-msi");
> - if (msi->msi_dev)
> + if (!msi->msi_dev)
> return -ENODEV;
>
> msi->msi_regs = of_iomap(msi->msi_dev, 0);
> @@ -167,9 +171,12 @@ static int ppc4xx_setup_pcieh_hw(struct platform_device *dev,
> (u32) (msi->msi_regs + PEIH_TERMADH), (u32) (msi->msi_regs));
>
> msi_virt = dma_alloc_coherent(&dev->dev, 64, &msi_phys, GFP_KERNEL);
> - msi->msi_addr_hi = 0x0;
> - msi->msi_addr_lo = (u32) msi_phys;
> - dev_dbg(&dev->dev, "PCIE-MSI: msi address 0x%x\n", msi->msi_addr_lo);
> + if (!msi_virt)
> + return -ENOMEM;
> + msi->msi_addr_hi = (u32)(msi_phys >> 32);
> + msi->msi_addr_lo = (u32)(msi_phys & 0xffffffff);
ditto
milton
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [1/2] powerpc/44x: Fix PCI MSI support for APM821xx SoC and Bluestone board
2012-03-06 4:37 ` [1/2] " Milton Miller
@ 2012-03-06 7:11 ` Benjamin Herrenschmidt
0 siblings, 0 replies; 6+ messages in thread
From: Benjamin Herrenschmidt @ 2012-03-06 7:11 UTC (permalink / raw)
To: Milton Miller
Cc: Michael Neuling, open-source-review, Tirumala R Marri,
linux-kernel, Josh Boyer, Anton Blanchard, Mai La, Paul Mackerras,
linuxppc-dev
On Mon, 2012-03-05 at 22:37 -0600, Milton Miller wrote:
> On Mon, 05 Mar 2012 about 17:29:41 -0000, Mai La wrote:
> >
> > @@ -43,7 +43,12 @@
> > #define PEIH_FLUSH0 0x30
> > #define PEIH_FLUSH1 0x38
> > #define PEIH_CNTRST 0x48
> > +
> > +#ifdef CONFIG_APM821xx
> > +#define NR_MSI_IRQS 8
> > +#else
> > #define NR_MSI_IRQS 4
> > +#endif
> >
>
> does this need to go into the dts binding?
A compile time #define is definitely not acceptable as it would break a
multiplatform kernel.
Ben.
> > struct ppc4xx_msi {
> > u32 msi_addr_lo;
> > @@ -150,12 +155,11 @@ static int ppc4xx_setup_pcieh_hw(struct platform_device *dev,
> > if (!sdr_addr)
> > return -1;
> >
> > - SDR0_WRITE(sdr_addr, (u64)res.start >> 32); /*HIGH addr */
> > - SDR0_WRITE(sdr_addr + 1, res.start & 0xFFFFFFFF); /* Low addr */
> > -
> > + mtdcri(SDR0, *sdr_addr, (u64)res.start >> 32); /*HIGH addr */
> > + mtdcri(SDR0, *sdr_addr + 1, res.start & 0xFFFFFFFF);/* Low addr */
>
> Please use upper_32_bits and lower_32_bits from linux/kernel.h
>
> > msi->msi_dev = of_find_node_by_name(NULL, "ppc4xx-msi");
> > - if (msi->msi_dev)
> > + if (!msi->msi_dev)
> > return -ENODEV;
> >
> > msi->msi_regs = of_iomap(msi->msi_dev, 0);
> > @@ -167,9 +171,12 @@ static int ppc4xx_setup_pcieh_hw(struct platform_device *dev,
> > (u32) (msi->msi_regs + PEIH_TERMADH), (u32) (msi->msi_regs));
> >
> > msi_virt = dma_alloc_coherent(&dev->dev, 64, &msi_phys, GFP_KERNEL);
> > - msi->msi_addr_hi = 0x0;
> > - msi->msi_addr_lo = (u32) msi_phys;
> > - dev_dbg(&dev->dev, "PCIE-MSI: msi address 0x%x\n", msi->msi_addr_lo);
> > + if (!msi_virt)
> > + return -ENOMEM;
> > + msi->msi_addr_hi = (u32)(msi_phys >> 32);
> > + msi->msi_addr_lo = (u32)(msi_phys & 0xffffffff);
>
> ditto
>
>
> milton
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] powerpc/44x: Fix PCI MSI support for APM821xx SoC and Bluestone board
@ 2012-02-29 8:47 Mai La
2012-02-29 14:18 ` Josh Boyer
0 siblings, 1 reply; 6+ messages in thread
From: Mai La @ 2012-02-29 8:47 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Josh Boyer, Matt Porter,
Tirumala R Marri, Grant Likely, Michael Neuling, Kumar Gala,
Anton Blanchard, linuxppc-dev, linux-kernel
Cc: open-source-review, Mai La
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 4148 bytes --]
This patch consists of:
- Enable PCI MSI as default for Bluestone board
- Define number of MSI interrupt for Maui APM821xx
- Fix returning ENODEV as finding MSI node
- Fix MSI physical high and low address
- Keep MSI data logically
Signed-off-by: Mai La <mla@apm.com>
---
arch/powerpc/platforms/44x/Kconfig | 2 ++
arch/powerpc/sysdev/ppc4xx_msi.c | 28 ++++++++++++++++++----------
2 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/platforms/44x/Kconfig b/arch/powerpc/platforms/44x/Kconfig
index fcf6bf2..9f04ce3 100644
--- a/arch/powerpc/platforms/44x/Kconfig
+++ b/arch/powerpc/platforms/44x/Kconfig
@@ -23,6 +23,8 @@ config BLUESTONE
default n
select PPC44x_SIMPLE
select APM821xx
+ select PCI_MSI
+ select PPC4xx_MSI
select IBM_EMAC_RGMII
help
This option enables support for the APM APM821xx Evaluation board.
diff --git a/arch/powerpc/sysdev/ppc4xx_msi.c b/arch/powerpc/sysdev/ppc4xx_msi.c
index 1c2d7af..6103908 100644
--- a/arch/powerpc/sysdev/ppc4xx_msi.c
+++ b/arch/powerpc/sysdev/ppc4xx_msi.c
@@ -31,7 +31,7 @@
#include <asm/prom.h>
#include <asm/hw_irq.h>
#include <asm/ppc-pci.h>
-#include <boot/dcr.h>
+#include <asm/dcr.h>
#include <asm/dcr-regs.h>
#include <asm/msi_bitmap.h>
@@ -43,7 +43,12 @@
#define PEIH_FLUSH0 0x30
#define PEIH_FLUSH1 0x38
#define PEIH_CNTRST 0x48
+
+#ifdef CONFIG_APM821xx
+#define NR_MSI_IRQS 8
+#else
#define NR_MSI_IRQS 4
+#endif
struct ppc4xx_msi {
u32 msi_addr_lo;
@@ -150,12 +155,11 @@ static int ppc4xx_setup_pcieh_hw(struct platform_device *dev,
if (!sdr_addr)
return -1;
- SDR0_WRITE(sdr_addr, (u64)res.start >> 32); /*HIGH addr */
- SDR0_WRITE(sdr_addr + 1, res.start & 0xFFFFFFFF); /* Low addr */
-
+ mtdcri(SDR0, *sdr_addr, res.start >> 32); /*HIGH addr */
+ mtdcri(SDR0, *sdr_addr + 1, res.start & 0xFFFFFFFF);/* Low addr */
msi->msi_dev = of_find_node_by_name(NULL, "ppc4xx-msi");
- if (msi->msi_dev)
+ if (!msi->msi_dev)
return -ENODEV;
msi->msi_regs = of_iomap(msi->msi_dev, 0);
@@ -167,9 +171,12 @@ static int ppc4xx_setup_pcieh_hw(struct platform_device *dev,
(u32) (msi->msi_regs + PEIH_TERMADH), (u32) (msi->msi_regs));
msi_virt = dma_alloc_coherent(&dev->dev, 64, &msi_phys, GFP_KERNEL);
- msi->msi_addr_hi = 0x0;
- msi->msi_addr_lo = (u32) msi_phys;
- dev_dbg(&dev->dev, "PCIE-MSI: msi address 0x%x\n", msi->msi_addr_lo);
+ if (!msi_virt)
+ return -ENOMEM;
+ msi->msi_addr_hi = (u32)(msi_phys >> 32);
+ msi->msi_addr_lo = (u32)(msi_phys & 0xffffffff);
+ dev_dbg(&dev->dev, "PCIE-MSI: msi address high 0x%x, low 0x%x\n",
+ msi->msi_addr_hi, msi->msi_addr_lo);
/* Progam the Interrupt handler Termination addr registers */
out_be32(msi->msi_regs + PEIH_TERMADH, msi->msi_addr_hi);
@@ -185,6 +192,8 @@ static int ppc4xx_setup_pcieh_hw(struct platform_device *dev,
out_be32(msi->msi_regs + PEIH_MSIED, *msi_data);
out_be32(msi->msi_regs + PEIH_MSIMK, *msi_mask);
+ dma_free_coherent(&dev->dev, 64, msi_virt, msi_phys);
+
return 0;
}
@@ -215,8 +224,6 @@ static int __devinit ppc4xx_msi_probe(struct platform_device *dev)
struct resource res;
int err = 0;
- msi = &ppc4xx_msi;/*keep the msi data for further use*/
-
dev_dbg(&dev->dev, "PCIE-MSI: Setting up MSI support...\n");
msi = kzalloc(sizeof(struct ppc4xx_msi), GFP_KERNEL);
@@ -242,6 +249,7 @@ static int __devinit ppc4xx_msi_probe(struct platform_device *dev)
dev_err(&dev->dev, "Error allocating MSI bitmap\n");
goto error_out;
}
+ ppc4xx_msi = *msi;
ppc_md.setup_msi_irqs = ppc4xx_setup_msi_irqs;
ppc_md.teardown_msi_irqs = ppc4xx_teardown_msi_irqs;
--
1.7.3.4
CONFIDENTIALITY NOTICE: This e-mail message, including any attachments,
is for the sole use of the intended recipient(s) and contains information
that is confidential and proprietary to AppliedMicro Corporation or its subsidiaries.
It is to be used solely for the purpose of furthering the parties' business relationship.
All unauthorized review, use, disclosure or distribution is prohibited.
If you are not the intended recipient, please contact the sender by reply e-mail
and destroy all copies of the original message.
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] powerpc/44x: Fix PCI MSI support for APM821xx SoC and Bluestone board
2012-02-29 8:47 [PATCH 1/2] " Mai La
@ 2012-02-29 14:18 ` Josh Boyer
2012-03-06 2:56 ` Mai La
0 siblings, 1 reply; 6+ messages in thread
From: Josh Boyer @ 2012-02-29 14:18 UTC (permalink / raw)
To: Mai La
Cc: open-source-review, Tirumala R Marri, linux-kernel,
Paul Mackerras, linuxppc-dev
On Wed, Feb 29, 2012 at 3:47 AM, Mai La <mla@apm.com> wrote:
> This patch consists of:
> - Enable PCI MSI as default for Bluestone board
> - Define number of MSI interrupt for Maui APM821xx
What is Maui? Is that the same thing as Bluestone?
> - Fix returning ENODEV as finding MSI node
> - Fix MSI physical high and low address
> - Keep MSI data logically
>
> Signed-off-by: Mai La <mla@apm.com>
Wow. So there are a lot of bugfixes here. I'm surprised this ever worked =
at
all with some of the things you're fixing. Nice to see.
> ---
> =A0arch/powerpc/platforms/44x/Kconfig | =A0 =A02 ++
> =A0arch/powerpc/sysdev/ppc4xx_msi.c =A0 | =A0 28 ++++++++++++++++++------=
----
> =A02 files changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/arch/powerpc/platforms/44x/Kconfig b/arch/powerpc/platforms/=
44x/Kconfig
> index fcf6bf2..9f04ce3 100644
> --- a/arch/powerpc/platforms/44x/Kconfig
> +++ b/arch/powerpc/platforms/44x/Kconfig
> @@ -23,6 +23,8 @@ config BLUESTONE
> =A0 =A0 =A0 =A0default n
> =A0 =A0 =A0 =A0select PPC44x_SIMPLE
> =A0 =A0 =A0 =A0select APM821xx
> + =A0 =A0 =A0 select PCI_MSI
> + =A0 =A0 =A0 select PPC4xx_MSI
> =A0 =A0 =A0 =A0select IBM_EMAC_RGMII
> =A0 =A0 =A0 =A0help
> =A0 =A0 =A0 =A0 =A0This option enables support for the APM APM821xx Evalu=
ation board.
> diff --git a/arch/powerpc/sysdev/ppc4xx_msi.c b/arch/powerpc/sysdev/ppc4x=
x_msi.c
> index 1c2d7af..6103908 100644
> --- a/arch/powerpc/sysdev/ppc4xx_msi.c
> +++ b/arch/powerpc/sysdev/ppc4xx_msi.c
> @@ -31,7 +31,7 @@
> =A0#include <asm/prom.h>
> =A0#include <asm/hw_irq.h>
> =A0#include <asm/ppc-pci.h>
> -#include <boot/dcr.h>
> +#include <asm/dcr.h>
> =A0#include <asm/dcr-regs.h>
> =A0#include <asm/msi_bitmap.h>
>
> @@ -43,7 +43,12 @@
> =A0#define PEIH_FLUSH0 =A0 =A00x30
> =A0#define PEIH_FLUSH1 =A0 =A00x38
> =A0#define PEIH_CNTRST =A0 =A00x48
> +
> +#ifdef CONFIG_APM821xx
> +#define NR_MSI_IRQS =A0 =A08
> +#else
> =A0#define NR_MSI_IRQS =A0 =A04
> +#endif
Hm. Do you think this is going to change quite a bit depending on which So=
C
is being used? If so, it might be better to introduce a Kconfig variable
that just defines this instead. Something like:
config 4xx_MSI_IRQS
int "Number of MSI IRQs"
depends on 4xx
default "8" if APM821xx
default "4" if !APM821xx
If there aren't going to be a wide variety of numbers, then the simple ifde=
f
you have is probably sufficient.
> =A0struct ppc4xx_msi {
> =A0 =A0 =A0 =A0u32 msi_addr_lo;
> @@ -150,12 +155,11 @@ static int ppc4xx_setup_pcieh_hw(struct platform_de=
vice *dev,
> =A0 =A0 =A0 =A0if (!sdr_addr)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -1;
>
> - =A0 =A0 =A0 SDR0_WRITE(sdr_addr, (u64)res.start >> 32); =A0 =A0 =A0/*HI=
GH addr */
> - =A0 =A0 =A0 SDR0_WRITE(sdr_addr + 1, res.start & 0xFFFFFFFF); /* Low ad=
dr */
> -
> + =A0 =A0 =A0 mtdcri(SDR0, *sdr_addr, res.start >> 32); =A0 =A0 =A0 /*HIG=
H addr */
> + =A0 =A0 =A0 mtdcri(SDR0, *sdr_addr + 1, res.start & 0xFFFFFFFF);/* Low =
addr */
Don't you still want the (u64) cast on res.start?
> CONFIDENTIALITY NOTICE: This e-mail message, including any attachments,
> is for the sole use of the intended recipient(s) and contains information
> that is confidential and proprietary to AppliedMicro Corporation or its s=
ubsidiaries.
> It is to be used solely for the purpose of furthering the parties' busine=
ss relationship.
> All unauthorized review, use, disclosure or distribution is prohibited.
> If you are not the intended recipient, please contact the sender by reply=
e-mail
> and destroy all copies of the original message.
Is there a way you can drop this? Others from APM seem to have figured out
how to do that, so hopefully it won't be a big problem.
josh
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] powerpc/44x: Fix PCI MSI support for APM821xx SoC and Bluestone board
2012-02-29 14:18 ` Josh Boyer
@ 2012-03-06 2:56 ` Mai La
0 siblings, 0 replies; 6+ messages in thread
From: Mai La @ 2012-03-06 2:56 UTC (permalink / raw)
To: Josh Boyer
Cc: open-source-review, Tirumala R Marri, linux-kernel,
Paul Mackerras, linuxppc-dev
[-- Attachment #1: Type: text/plain, Size: 4425 bytes --]
Please see my in-line reply.
On Wed, Feb 29, 2012 at 9:18 PM, Josh Boyer <jwboyer@gmail.com> wrote:
> On Wed, Feb 29, 2012 at 3:47 AM, Mai La <mla@apm.com> wrote:
> > This patch consists of:
> > - Enable PCI MSI as default for Bluestone board
> > - Define number of MSI interrupt for Maui APM821xx
>
> What is Maui? Is that the same thing as Bluestone?
>
=> Bluestone board uses Maui APM821xx SoC. I would make the description
clearer like:
This patch consists of:
- Enable PCI MSI as default for Bluestone board
- Define number of MSI interrupt for Maui APM821xxx SoC using in Bluestone
board
> > - Fix returning ENODEV as finding MSI node
> > - Fix MSI physical high and low address
> > - Keep MSI data logically
> >
> > Signed-off-by: Mai La <mla@apm.com>
>
> Wow. So there are a lot of bugfixes here. I'm surprised this ever worked
> at
> all with some of the things you're fixing. Nice to see.
>
> ---
> > arch/powerpc/platforms/44x/Kconfig | 2 ++
> > arch/powerpc/sysdev/ppc4xx_msi.c | 28 ++++++++++++++++++----------
> > 2 files changed, 20 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/powerpc/platforms/44x/Kconfig
> b/arch/powerpc/platforms/44x/Kconfig
> > index fcf6bf2..9f04ce3 100644
> > --- a/arch/powerpc/platforms/44x/Kconfig
> > +++ b/arch/powerpc/platforms/44x/Kconfig
> > @@ -23,6 +23,8 @@ config BLUESTONE
> > default n
> > select PPC44x_SIMPLE
> > select APM821xx
> > + select PCI_MSI
> > + select PPC4xx_MSI
> > select IBM_EMAC_RGMII
> > help
> > This option enables support for the APM APM821xx Evaluation
> board.
> > diff --git a/arch/powerpc/sysdev/ppc4xx_msi.c
> b/arch/powerpc/sysdev/ppc4xx_msi.c
> > index 1c2d7af..6103908 100644
> > --- a/arch/powerpc/sysdev/ppc4xx_msi.c
> > +++ b/arch/powerpc/sysdev/ppc4xx_msi.c
> > @@ -31,7 +31,7 @@
> > #include <asm/prom.h>
> > #include <asm/hw_irq.h>
> > #include <asm/ppc-pci.h>
> > -#include <boot/dcr.h>
> > +#include <asm/dcr.h>
> > #include <asm/dcr-regs.h>
> > #include <asm/msi_bitmap.h>
> >
> > @@ -43,7 +43,12 @@
> > #define PEIH_FLUSH0 0x30
> > #define PEIH_FLUSH1 0x38
> > #define PEIH_CNTRST 0x48
> > +
> > +#ifdef CONFIG_APM821xx
> > +#define NR_MSI_IRQS 8
> > +#else
> > #define NR_MSI_IRQS 4
> > +#endif
>
> Hm. Do you think this is going to change quite a bit depending on which
> SoC
> is being used? If so, it might be better to introduce a Kconfig variable
> that just defines this instead. Something like:
>
> config 4xx_MSI_IRQS
> int "Number of MSI IRQs"
> depends on 4xx
> default "8" if APM821xx
> default "4" if !APM821xx
>
> If there aren't going to be a wide variety of numbers, then the simple
> ifdef
> you have is probably sufficient.
>
=> So far we don't have a wide variety of numbers. So I think just keep
ifdef is fine.
> > struct ppc4xx_msi {
> > u32 msi_addr_lo;
> > @@ -150,12 +155,11 @@ static int ppc4xx_setup_pcieh_hw(struct
> platform_device *dev,
> > if (!sdr_addr)
> > return -1;
> >
> > - SDR0_WRITE(sdr_addr, (u64)res.start >> 32); /*HIGH addr */
> > - SDR0_WRITE(sdr_addr + 1, res.start & 0xFFFFFFFF); /* Low addr */
> > -
> > + mtdcri(SDR0, *sdr_addr, res.start >> 32); /*HIGH addr */
> > + mtdcri(SDR0, *sdr_addr + 1, res.start & 0xFFFFFFFF);/* Low addr
> */
>
> Don't you still want the (u64) cast on res.start?
>
=> Keep (u64) is OK. So I would keep it like:
mtdcri(SDR0, *sdr_addr, (u64)res.start >> 32); /*HIGH addr */
> > CONFIDENTIALITY NOTICE: This e-mail message, including any attachments,
> > is for the sole use of the intended recipient(s) and contains information
> > that is confidential and proprietary to AppliedMicro Corporation or its
> subsidiaries.
> > It is to be used solely for the purpose of furthering the parties'
> business relationship.
> > All unauthorized review, use, disclosure or distribution is prohibited.
> > If you are not the intended recipient, please contact the sender by
> reply e-mail
> > and destroy all copies of the original message.
>
> Is there a way you can drop this? Others from APM seem to have figured out
> how to do that, so hopefully it won't be a big problem.
>
> => Our IT guy help me to remove this!
Thank you! Do you have any further comment? I would send another patch with
the above fix soon.
josh
>
[-- Attachment #2: Type: text/html, Size: 6576 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-03-06 7:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-03-06 3:29 [PATCH 1/2] powerpc/44x: Fix PCI MSI support for APM821xx SoC and Bluestone board Mai La
2012-03-06 4:37 ` [1/2] " Milton Miller
2012-03-06 7:11 ` Benjamin Herrenschmidt
-- strict thread matches above, loose matches on Subject: below --
2012-02-29 8:47 [PATCH 1/2] " Mai La
2012-02-29 14:18 ` Josh Boyer
2012-03-06 2:56 ` Mai La
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).