From: Nicolas DET <nd@bplan-gmbh•de>
To: Benjamin Herrenschmidt <benh@kernel•crashing.org>
Cc: linuxppc-dev@ozlabs•org, sl@bplan-gmbh•de, sha@pengutronix•de,
linuxppc-embedded@ozlabs•org
Subject: Re: [PATCH 1/2] Add MPC52xx Interrupt controller support for ARCH=powerpc
Date: Wed, 1 Nov 2006 10:24:22 +0100 (MET) [thread overview]
Message-ID: <454867AB.2010904@bplan-gmbh.de> (raw)
In-Reply-To: <1162331943.25682.358.camel@localhost.localdomain>
[-- Attachment #1: Type: text/plain, Size: 5641 bytes --]
Benjamin Herrenschmidt wrote:
> On Tue, 2006-10-31 at 21:04 +0100, Nicolas DET wrote:
>
>> Here is is ;-).
>> As my mailer can insert a file, I copy paste. I hope it's still ok...
>
> No, your patch got wrapped. Its damaged. I see you used Thunderbird. I
> have no experience with sending patches with it, so I don't know what
> the trick is to have them undamaged. With evolution, the trick is to use
> the pre-defined style "preformat".
>
I'll figure uot something...
> Anyway, minor comments, we're getting there...
>
Everything fixed.
>> +
>> +define_machine(efika)
>> +{
>> + .name = EFIKA_PLATFORM_NAME,
>> + .probe = efika_probe,
>> + .setup_arch = efika_setup_arch,
>> + .init = efika_init,
>> + .show_cpuinfo = efika_show_cpuinfo,
>> + .init_IRQ = efika_init_IRQ,
>> + .get_irq = mpc52xx_get_irq,
>> + .restart = rtas_restart,
>> + .power_off = rtas_power_off,
>> + .halt = rtas_halt,
>> + .set_rtc_time = rtas_set_rtc_time,
>> + .get_rtc_time = rtas_get_rtc_time,
>> + .progress = rtas_progress,
>> + .get_boot_time = rtas_get_boot_time,
>> + .calibrate_decr = generic_calibrate_decr,
>> + .phys_mem_access_prot = pci_phys_mem_access_prot,
>> + .pcibios_fixup = efika_pciirq_map,
>> +};
>
> The later can go away if you apply the patch I posted last week
> [PATCH] Powerpc: Make pci_read_irq_line the default: on mpc7448hpc2
> board. First.
>
Ok.
>
> The whole function is not needed. Just apply my other patch first.
>
Compiling...
>> + default y
>> +
>> +config PPC_EFIKA
>> + bool "bPlan Efika 5k2. MPC5200B based computer"
>> + depends on PPC_MULTIPLATFORM && PPC32
>> + select PPC_RTAS
>> + select RTAS_PROC
>> + select PPC_MPC52xx
>> + select PPC_MPC52xx_PIC
>> + default y
>> +
>> config PPC_PMAC
>> bool "Apple PowerMac based machines"
>> depends on PPC_MULTIPLATFORM
>>
>
> PIC patch separate please.
>
Ok.
>> +/*
>> + * Critial interrupt irq_chip
>> +*/
>> +static void mpc52xx_crit_mask(unsigned int virq)
>> +{
>> + int irq;
>> + int l2irq;
>> +
>> + irq = irq_map[virq].hwirq;
>> + l2irq = (irq & MPC52xx_IRQ_L2_MASK) >> MPC52xx_IRQ_L2_OFFSET;
>> +
>> + pr_debug("%s: irq=%x, l2=%d\n", __func__, irq, l2irq);
>> +
>> + BUG_ON(l2irq != 0);
>> +
>> + io_be_clrbit(&intr->ctrl, 11);
>> +}
>
> I'm not sure you understood my previous comment... any reason why crit
> and mainirq are two different sets of functions and a different level 1
> since crit is just basically mainirq 0 ? And main & mainirq, on the
> contrary, should be different L1s since they are ... different :)
In my opinion, as it reflects a bit better hwow the hw itself is
architectured (critical, main, peripheral...) it's better to do it like
this. I do not wish to change this. Moreover, it's yet working pretty well.
>> + mpc52xx_irqhost =
>> + irq_alloc_host(IRQ_HOST_MAP_LINEAR, 0xd0,
>> + &mpc52xx_irqhost_ops, -1);
>
> I would prefer that 0xd0 to be a symbolic constant in the .h
Ok. will be done
>> + if (mpc52xx_irqhost)
>> + mpc52xx_irqhost->host_data = (void *)find_mpc52xx_picnode();
>
> Casting to void * is fairly useless :)
>
Removed.
>> +#ifdef CONFIG_PCI
>> +#define _IO_BASE isa_io_base
>> +#define _ISA_MEM_BASE isa_mem_base
>> +#define PCI_DRAM_OFFSET pci_dram_offset
>> +#else
>> +#define _IO_BASE 0
>> +#define _ISA_MEM_BASE 0
>> +#define PCI_DRAM_OFFSET 0
>> +#endif
>
> I told you several times to remove the above. The whole thing is
> duplicate of io.h.
>
> The fact that the former has a special case for CONFIG_PPC_MPC52xx is
> bogus in the first place... you might want to turn -that- into a
It normaly does not compile if I remove it as state earlier. I'll remove
them and fixed the compile issue.
> if defined(CONFIG_PPC_MPC52xx) && !defined(CONFIG_PPC_MERGE)
>
>> +/*
>> ======================================================================== */
>> +/* Main registers/struct addresses
>> */
>> +/*
>> ======================================================================== */
>> +
>> +/* MBAR position */
>> +#define MPC52xx_MBAR 0xf0000000 /* Phys address */
>> +#define MPC52xx_MBAR_VIRT 0xf0000000 /* Virt address */
>> +#define MPC52xx_MBAR_SIZE 0x00010000
>> +
>> +#define MPC52xx_PA(x) ((phys_addr_t)(MPC52xx_MBAR + (x)))
>> +#define MPC52xx_VA(x) ((void __iomem *)(MPC52xx_MBAR_VIRT + (x)))
>
> The above definitions are all bogus for arch/powerpc, just remove them.
Ok.
>> +/*
>> + * 24 peripherals ints
>> + * + 16 mains ints
>> + * + 4 crit
>> + * + 16 bestcomm task
>> + * = 64
>> +*/
>> +#define MPC52xx_IRQ_MAXCOUNT (64)
>
> The above is both not correct anymore and not used. Please fix it and
> use it instead of hard coding.
Will be removed and replace by another define to reflect the highest
virq (0xd0).
#define MPC52xx_IRQ_MACVIRQ (0xd0)
sounds ok ?
>> +static inline struct device_node *find_mpc52xx_picnode(void)
>> +{
>> + return of_find_compatible_node(NULL, "interrupt-controller",
>> + "mpc5200-pic");
>> +}
>
> Any reason why you need that inline since it's not used anywhere else
> but the PIC code ? Just put that of_find_compatible_node() statement in
> the .c and be done with it.
>
Done.
>> + /* Matching of PSC function */
>> +struct mpc52xx_psc_func {
>> + int id;
>> + char *func;
>> +};
>>
>> +extern int mpc52xx_match_psc_function(int psc_idx, const char *func);
>> +extern struct mpc52xx_psc_func mpc52xx_psc_functions[];
>> + /* This array is to be defined in platform file */
>
> The above doesn't look like it should migrate to arch/powerpc... what is
> it supposed to be ?
>
Removed.
I'll re submit the patch as soon as 'done, compiled, tested'.
Bye
[-- Attachment #2: nd.vcf --]
[-- Type: text/x-vcard, Size: 249 bytes --]
begin:vcard
fn:Nicolas DET ( bplan GmbH )
n:DET;Nicolas
org:bplan GmbH
adr:;;;;;;Germany
email;internet:nd@bplan-gmbh•de
title:Software Entwicklung
tel;work:+49 6171 9187 - 31
x-mozilla-html:FALSE
url:http://www.bplan-gmbh.de
version:2.1
end:vcard
next prev parent reply other threads:[~2006-11-01 9:24 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2006-10-29 23:10 [PATCH 1/2] Add MPC52xx Interrupt controller support for ARCH=powerpc Nicolas DET
2006-10-30 17:37 ` Dale Farnsworth
2006-10-30 17:47 ` Dale Farnsworth
2006-10-30 23:18 ` Sylvain Munaut
2006-10-31 7:10 ` Nicolas DET
2006-10-30 22:25 ` Kumar Gala
2006-10-30 22:31 ` Benjamin Herrenschmidt
2006-10-30 23:15 ` Sylvain Munaut
2006-10-31 1:11 ` Kumar Gala
2006-10-31 6:59 ` Sylvain Munaut
2006-10-31 7:05 ` Benjamin Herrenschmidt
2006-10-31 7:14 ` Nicolas DET
2006-10-31 7:38 ` Benjamin Herrenschmidt
2006-10-31 8:25 ` Nicolas DET
2006-10-31 8:42 ` Benjamin Herrenschmidt
2006-10-31 9:08 ` Nicolas DET
2006-10-31 20:04 ` Nicolas DET
2006-10-31 21:59 ` Benjamin Herrenschmidt
2006-10-31 22:08 ` Grant Likely
2006-10-31 22:11 ` Benjamin Herrenschmidt
2006-10-31 23:08 ` Grant Likely
2006-11-01 1:06 ` Benjamin Herrenschmidt
2006-11-01 9:24 ` Nicolas DET [this message]
2006-11-01 20:56 ` Benjamin Herrenschmidt
2006-10-31 14:34 ` Kumar Gala
2006-10-31 16:24 ` Grant Likely
2006-10-31 4:27 ` Benjamin Herrenschmidt
2006-10-31 7:09 ` Nicolas DET
2006-10-31 7:21 ` Benjamin Herrenschmidt
2006-10-31 7:49 ` Nicolas DET
2006-10-31 7:58 ` Benjamin Herrenschmidt
2006-10-31 8:28 ` Nicolas DET
2006-10-31 8:44 ` Benjamin Herrenschmidt
2006-10-31 9:04 ` Nicolas DET
2006-10-31 9:07 ` Benjamin Herrenschmidt
2006-10-31 9:46 ` Nicolas DET
2006-10-31 20:29 ` Benjamin Herrenschmidt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=454867AB.2010904@bplan-gmbh.de \
--to=nd@bplan-gmbh$(echo .)de \
--cc=benh@kernel$(echo .)crashing.org \
--cc=linuxppc-dev@ozlabs$(echo .)org \
--cc=linuxppc-embedded@ozlabs$(echo .)org \
--cc=sha@pengutronix$(echo .)de \
--cc=sl@bplan-gmbh$(echo .)de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox