Re: [PATCH 1/2] powerpc/44x: Fix PCI MSI support for APM821xx SoC and Bluestone board

Please see my in-line reply.

On Wed, Feb 29, 2012 at 9:18 PM, Josh Boyer [off-list ref] wrote:

On Wed, Feb 29, 2012 at 3:47 AM, Mai La [off-list ref] wrote:

quoted

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

quoted

- Fix returning ENODEV as finding MSI node
- Fix MSI physical high and low address
- Keep MSI data logically

Signed-off-by: Mai La <redacted>

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.

 > ---

quoted

 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

quoted

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.

quoted

diff --git a/arch/powerpc/sysdev/ppc4xx_msi.c

b/arch/powerpc/sysdev/ppc4xx_msi.c

quoted

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 {

quoted

       u32 msi_addr_lo;

@@ -150,12 +155,11 @@ static int ppc4xx_setup_pcieh_hw(struct

platform_device *dev,

quoted

       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 */

quoted

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.

quoted

It is to be used solely for the purpose of furthering the parties'

business relationship.

quoted

All unauthorized review, use, disclosure or distribution is prohibited.
If you are not the intended recipient, please contact the sender by

reply e-mail

quoted

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