Re: [PATCH 2/10 v2] Add the MPC8641 HPCN platform files.

[Jon Loeliger <jdl@jdl.com>]

So, like, the other day Benjamin Herrenschmidt mumbled:
> On Thu, 2006-06-08 at 16:57 -0500, Jon Loeliger wrote:
> 
> > +void
> > +mpc86xx_restart(char *cmd)
> > +{
> > +	void __iomem *rstcr;
> > +
> > +	local_irq_disable();
> > +
> > +	/* Assert reset request to Reset Control Register */
> > +	rstcr = ioremap(get_immrbase() + MPC86XX_RSTCR_OFFSET, 0x100);
> > +	out_be32(rstcr, 0x2);
> > +
> > +	/* not reached */
> > +}
> 
> ioremap with irq disabled isn't great....  You should do the ioremap
> once at boot.

OK.  Would this be acceptable instead:

    mpc86xx_restart(char *cmd)
    {
	void __iomem *rstcr;

	rstcr = ioremap(get_immrbase() + MPC86XX_RSTCR_OFFSET, 0x100);

	local_irq_disable();

	/* Assert reset request to Reset Control Register */
	out_be32(rstcr, 0x2);
	/* not reached */
    }


> Overall, that file is too small :) Move those into your setup.c and make
> those static...

Will do.

> [SMP code snipped]
>
> This file could/should be called *_smp.c and not need #ifdef's :)

OK.


> Most of the values above should probably be retreived from the
> device-tree.

I'll see what I can remove.  Some of the PCI/PCI-e bits
_will_ be device tree derived, but just not quite there yet...
It's just not going to be a perfect first patch;  there
will have to be incremental development in that direction.

> As I suggested before, the 2 above should be static in your setup file.

Right.

> > +extern int __init add_bridge(struct device_node *dev);
> 
> Aren't we exposing that already via some header ?

Perhaps.  I'll look around for it.

> > +extern void __init setup_indirect_pex_nomap(struct pci_controller* hose,
> > +					    void __iomem * cfg_addr,
> > +					    void __iomem * cfg_data);
> > +
> > +extern struct smp_ops_t smp_8641_ops;
> 
> See my comments about the PCI stuff with the PCI patch.

and...

> All of the above should of course come from the device-tree. 2.6.18 will
> have the support for having interrupt routing from it without having
> nodes for all devices. I'll post it to the list in a week or so, I'm
> coding right now :)

Right.  We're not quite entirely able to convert it
all to device tree yet.  Working on it still...

> > +	phys_addr_t OpenPIC_PAddr;
> > +
> > +	/* Determine the Physical Address of the OpenPIC regs */
> > +	OpenPIC_PAddr = get_immrbase() + MPC86xx_OPENPIC_OFFSET;
> 
> Do you really _need_ studly caps ? I know we did that before but you
> don't have to copy ugly stuff :)

Uh, sorry.  No problem.

> In general, you -need- a device node for the interrupt controller. It
> will be made mandatory by the new code.

I know.  There is a fully defined node for that in my
device tree already.

> You'll have to provide proper
> interrupt informations in your device-tree (it's easy, really).

It's there.

> If you get that right, it will be very easy to "just work" with my new
> code.

I'll send that to the list too for reference
just to make sure we are in sync here.

> > +	/* Alloc mpic structure and per isu has 16 INT entries. */
> > +	mpic1 = mpic_alloc(OpenPIC_PAddr,
> > +			MPIC_PRIMARY | MPIC_WANTS_RESET | MPIC_BIG_ENDIAN,
> > +			16, MPC86xx_OPENPIC_IRQ_OFFSET, 0, 250,
> > +			mpc86xx_hpcn_openpic_initsenses,
> > +			sizeof(mpc86xx_hpcn_openpic_initsenses),
> > +			" MPIC     ");
> > +	BUG_ON(mpic1 == NULL);
> > +
> > +	/* 48 Internal Interrupts */
> > +	mpic_assign_isu(mpic1, 0, OpenPIC_PAddr + 0x10200);
> > +	mpic_assign_isu(mpic1, 1, OpenPIC_PAddr + 0x10400);
> > +	mpic_assign_isu(mpic1, 2, OpenPIC_PAddr + 0x10600);
> 
> I haven't looked in detail at your memory map, but do you need separate
> ISUs ? They seem to be quite close together to me... Also, you should
> invent properties in the mpic node for some of those things, like
> big-endian (like apple does) indicating it's a big endian openpic,
> etc... If you manage to get close enough to spec & common usage, you
> might not even need your own init function at all in the future.

OK.  We'll work in that direction, but incrementally.

> > +	/* 16 External interrupts */
> > +	mpic_assign_isu(mpic1, 3, OpenPIC_PAddr + 0x10000);
> 
> That looks like you used ISUs in order to "re-order" them... why ?

Heck if I know.  We'll have to ask around some here... :-)

> > +#ifdef CONFIG_PEX
> > +	mpic_setup_cascade(MPC86xx_IRQ_EXT9, i8259_irq_cascade, NULL);
> > +	i8259_init(0, I8259_OFFSET);
> > +#endif
> > +}
> 
> Cascade handling is changing with my genirq port. It will be easy to
> adapt though. Same comments howveer, you should have a device-tree node
> for the 8259 (under an ISA bridge, those shall really be in the
> device-tree). In general, on-board bridges should be in the device-tree,
> only slots or standardly routed child PCI devices need not.

Can you send me an example?

> 
> All of the above shall be in the device-tree.

Yes, eventually.  But not yet...  We're working on
this with other folks like Kumar, and Monta Vista,
and other folks.

> > +static void __init
> > +mpc86xx_hpcn_setup_arch(void)
> > +{
> > +	struct device_node *np;
> > +
> > +#ifdef CONFIG_SMP
> > +	phys_addr_t mcm_paddr;
> > +	void *mcm_vaddr = NULL;
> > +	unsigned long vaddr;
> > +#endif
> > +
> > +	if (ppc_md.progress)
> > +		ppc_md.progress("mpc86xx_hpcn_setup_arch()", 0);
> > +
> > +	np = of_find_node_by_type(NULL, "cpu");
> > +	if (np != 0) {
> > +		unsigned int *fp;
> > +
> > +		fp = (int *)get_property(np, "clock-frequency", NULL);
> > +		if (fp != 0)
> > +			loops_per_jiffy = *fp / HZ;
> > +		else
> > +			loops_per_jiffy = 50000000 / HZ;
> > +		of_node_put(np);
> > +	}
> 
> The above looks dodgy... powerpc uses the timebase frequency for delays
> anyway, lpj will be initialized by the bogomips code, and should but
> unused mostly nowadays anyway.

Uh, you want I should rip it out, boss? :-)

> > +#ifdef CONFIG_PEX
> > +	for (np = NULL; (np = of_find_node_by_type(np, "pci")) != NULL;)
> > +		add_bridge(np);
> > +
> > +	ppc_md.pci_swizzle = common_swizzle;
> > +	ppc_md.pci_map_irq = mpc86xx_map_irq;
> > +	ppc_md.pci_exclude_device = mpc86xx_exclude_device;
> > +#endif
> 
> I'm not sure I like this CONFIG_PEX (in general). Just use CONFIG_PCI
> for now all over the place. PCI-E has it's own binding that we don't
> quite respect yet but will do and all of that will be in the
> device-tree. However, as far as the kernel is concerned, this is all
> under CONFIG_PCI.

But I think we are looking for independent PCI/PCI-E options
to be independently configurable still...?

> > +#ifdef CONFIG_SMP
> > +  	/* Release Core 1 in boot holdoff */
> > +	mcm_paddr = get_immrbase() + MPC86xx_MCM_OFFSET;
> > +	mcm_vaddr = ioremap(mcm_paddr, MPC86xx_MCM_SIZE);
> > +
> > +	vaddr = (unsigned long)mcm_vaddr +  MCM_PORT_CONFIG_OFFSET;
> > +	out_be32((volatile unsigned *)vaddr, CPU_ALL_RELEASED);
> > +	smp_ops = &smp_8641_ops;
> > +#endif
> > +}
> 
> Instead of ifdef's, just do a mpc86xx_smp_init() and put it somewhere
> with the other SMP things in the file that contains them (and remove the
> #ifdef's there too, just only build the file if CONFIG_SMP is set).

OK, I catch your drift, but if the file is missing and
there is a hard call to  mpc86xx_smp_init() here, how is
that resolved?  Is mpc86xx_smp_init #defined empty when
compiled non-SMP then?  Like in the header file:

    #ifndef CONFIG_SMP
        #define mpc86xx_smp_init()
    #endif

> > +void
> > +mpc86xx_hpcn_show_cpuinfo(struct seq_file *m)
> > +{
> > +	uint pvid, svid, phid1;
> > +	uint memsize = total_memory;
> > +
> > +	pvid = mfspr(SPRN_PVR);
> > +	svid = mfspr(SPRN_SVR);
> >
> > +	seq_printf(m, "Vendor\t\t: Freescale Semiconductor\n");
> > +	seq_printf(m, "Machine\t\t: MPC86xx HPCN Board\n");
> > +	seq_printf(m, "PVR\t\t: 0x%x\n", pvid);
> > +	seq_printf(m, "SVR\t\t: 0x%x\n", svid);
> 
> The PVR is probably a duplicate and the SVR should be added to per-cpu
> info instead.

I confess, I am not sure what you mean here.  Could you
clarify this a bit for me, or point to an example perhaps?

Thanks for you feedback!

jdl