Please see my in-line reply.<br><br><div class="gmail_quote">On Wed, Feb 29, 2012 at 9:18 PM, Josh Boyer <span dir="ltr"><<a href="mailto:jwboyer@gmail.com" target="_blank">jwboyer@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div>On Wed, Feb 29, 2012 at 3:47 AM, Mai La <<a href="mailto:mla@apm.com" target="_blank">mla@apm.com</a>> wrote:<br>
> This patch consists of:<br>
> - Enable PCI MSI as default for Bluestone board<br>
> - Define number of MSI interrupt for Maui APM821xx<br>
<br>
</div>What is Maui? Is that the same thing as Bluestone?<br></blockquote><div><font color="#000066"><span style="color:rgb(0,0,0)"><br>=> Bluestone board uses Maui APM821xx SoC. I would make the description clearer like:</span><br style="color:rgb(0,0,0)">
<span style="color:rgb(0,0,0)"> This patch consists of:</span><br style="color:rgb(0,0,0)"><span style="color:rgb(0,0,0)">
- Enable PCI MSI as default for Bluestone board<br>- Define number of MSI interrupt for Maui APM821xxx SoC using in Bluestone board<br><br></span></font></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><br>
> - Fix returning ENODEV as finding MSI node<br>
> - Fix MSI physical high and low address<br>
> - Keep MSI data logically<br>
><br>
> Signed-off-by: Mai La <<a href="mailto:mla@apm.com" target="_blank">mla@apm.com</a>><br>
<br>
</div>Wow. So there are a lot of bugfixes here. I'm surprised this ever worked at<br>
all with some of the things you're fixing. Nice to see.<br><div><div></div></div></blockquote><div></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><div>
> ---<br>
> arch/powerpc/platforms/44x/Kconfig | 2 ++<br>
> arch/powerpc/sysdev/ppc4xx_msi.c | 28 ++++++++++++++++++----------<br>
> 2 files changed, 20 insertions(+), 10 deletions(-)<br>
><br>
> diff --git a/arch/powerpc/platforms/44x/Kconfig b/arch/powerpc/platforms/44x/Kconfig<br>
> index fcf6bf2..9f04ce3 100644<br>
> --- a/arch/powerpc/platforms/44x/Kconfig<br>
> +++ b/arch/powerpc/platforms/44x/Kconfig<br>
> @@ -23,6 +23,8 @@ config BLUESTONE<br>
> default n<br>
> select PPC44x_SIMPLE<br>
> select APM821xx<br>
> + select PCI_MSI<br>
> + select PPC4xx_MSI<br>
> select IBM_EMAC_RGMII<br>
> help<br>
> This option enables support for the APM APM821xx Evaluation board.<br>
> diff --git a/arch/powerpc/sysdev/ppc4xx_msi.c b/arch/powerpc/sysdev/ppc4xx_msi.c<br>
> index 1c2d7af..6103908 100644<br>
> --- a/arch/powerpc/sysdev/ppc4xx_msi.c<br>
> +++ b/arch/powerpc/sysdev/ppc4xx_msi.c<br>
> @@ -31,7 +31,7 @@<br>
> #include <asm/prom.h><br>
> #include <asm/hw_irq.h><br>
> #include <asm/ppc-pci.h><br>
> -#include <boot/dcr.h><br>
> +#include <asm/dcr.h><br>
> #include <asm/dcr-regs.h><br>
> #include <asm/msi_bitmap.h><br>
><br>
> @@ -43,7 +43,12 @@<br>
> #define PEIH_FLUSH0 0x30<br>
> #define PEIH_FLUSH1 0x38<br>
> #define PEIH_CNTRST 0x48<br>
> +<br>
> +#ifdef CONFIG_APM821xx<br>
> +#define NR_MSI_IRQS 8<br>
> +#else<br>
> #define NR_MSI_IRQS 4<br>
> +#endif<br>
<br>
</div></div>Hm. Do you think this is going to change quite a bit depending on which SoC<br>
is being used? If so, it might be better to introduce a Kconfig variable<br>
that just defines this instead. Something like:<br>
<br>
config 4xx_MSI_IRQS<br>
int "Number of MSI IRQs"<br>
depends on 4xx<br>
default "8" if APM821xx<br>
default "4" if !APM821xx<br>
<br>
If there aren't going to be a wide variety of numbers, then the simple ifdef<br>
you have is probably sufficient.<br><div></div></blockquote><div><br>=> So far we don't have a wide variety of numbers. So I think just keep ifdef is fine.<br> <br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div>
> struct ppc4xx_msi {<br>
> u32 msi_addr_lo;<br>
> @@ -150,12 +155,11 @@ static int ppc4xx_setup_pcieh_hw(struct platform_device *dev,<br>
> if (!sdr_addr)<br>
> return -1;<br>
><br>
> - SDR0_WRITE(sdr_addr, (u64)res.start >> 32); /*HIGH addr */<br>
> - SDR0_WRITE(sdr_addr + 1, res.start & 0xFFFFFFFF); /* Low addr */<br>
> -<br>
> + mtdcri(SDR0, *sdr_addr, res.start >> 32); /*HIGH addr */<br>
> + mtdcri(SDR0, *sdr_addr + 1, res.start & 0xFFFFFFFF);/* Low addr */<br>
<br>
</div>Don't you still want the (u64) cast on res.start?<br></blockquote><div><br>=> Keep (u64) is OK. So I would keep it like:<br> mtdcri(SDR0, *sdr_addr, (u64)res.start >> 32); /*HIGH addr */<br><br></div>
<blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> CONFIDENTIALITY NOTICE: This e-mail message, including any attachments,<br>
> is for the sole use of the intended recipient(s) and contains information<br>
> that is confidential and proprietary to AppliedMicro Corporation or its subsidiaries.<br>
> It is to be used solely for the purpose of furthering the parties' business relationship.<br>
> All unauthorized review, use, disclosure or distribution is prohibited.<br>
> If you are not the intended recipient, please contact the sender by reply e-mail<br>
> and destroy all copies of the original message.<br>
<br>
Is there a way you can drop this? Others from APM seem to have figured out<br>
how to do that, so hopefully it won't be a big problem.<br>
<span><font color="#888888"><br></font></span></blockquote><div>=> Our IT guy help me to remove this! <br><br>Thank you! Do you have any further comment? I would send another patch with the above fix soon.<br><br></div>
<blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span><font color="#888888">
josh<br>
</font></span></blockquote></div>