[PATCH 1/8] ARM: support for Moschip MCS814x SoCs

[Arnd Bergmann]

On Tuesday 17 July 2012, Florian Fainelli wrote:
> On Monday 16 July 2012 15:54:04 Arnd Bergmann wrote:
> > On Sunday 15 July 2012, Florian Fainelli wrote:
> > 
> > > diff --git a/arch/arm/mach-mcs814x/Kconfig b/arch/arm/mach-mcs814x/Kconfig
> > > new file mode 100644
> > > index 0000000..c89422f
> > > --- /dev/null
> > > +++ b/arch/arm/mach-mcs814x/Kconfig
> > > @@ -0,0 +1,11 @@
> > > +if ARCH_MCS814X
> > > +
> > > +config MCS8140
> > > +	select CPU_ARM926T
> > > +	bool
> > > +
> > > +menu "Moschip MCS8140 boards"
> > > +
> > > +endmenu
> > > +
> > > +endif
> > 
> > What values does the Kconfig file actually provide? I see that you are
> > adding the boards here later, but I think it would be nice to just 
> > make the new platform a single Kconfig option with no individual
> > board selection.
> 
> I plan on adding support for MCS8142 later on, thus I thought it would be nice 
> to be able to selectively compile board support for MCS8140 or some later 
> chip.

But is there much difference between the two? How much code would
actually get built for one chip but not the other? If all the
differences can be run-time detected from the device tree, I
think it's better to not have different compile-time options at
all.

> > Platforms are now converting to the common clock framework in drivers/clk.
> > Mike Turquette as the subsystem maintainer can probably judge better whether
> > we should still be allowing new platforms with their own implementation
> > of clk, but my feeling is that you should use the subsystem instead
> > and add a driver in a subdirectory of drivers/clk instead of in the
> > platform.
> > 
> > > +void __iomem *mcs814x_sysdbg_base;
> > 
> > Does this have to be globally visible? I would recommend making it static.
> 
> Yes, since it is used both by clock.c and common.c

Ok. How about converting this to an global function in common.c that is
called from clk_local_onoff_enable instead?

> > > +void __init mcs814x_map_io(void)
> > > +{
> > > +	iotable_init(mcs814x_io_desc, ARRAY_SIZE(mcs814x_io_desc));
> > > +
> > > +	mcs814x_sysdbg_base = ioremap(MCS814X_IO_START + MCS814X_SYSDBG,
> > > +					MCS814X_SYSDBG_SIZE);
> > > +	if (!mcs814x_sysdbg_base)
> > > +		panic("unable to remap sysdbg base");
> > > +}
> > > +
> > > +void mcs814x_restart(char mode, const char *cmd)
> > > +{
> > > +	__raw_writel(~(1 << 31), mcs814x_sysdbg_base);
> > > +}
> > 
> > You should generally avoid using __raw_readl etc. and instead use
> > readl/write or readl_relaxed/writel_relaxed.
> 
> Allright, even on an ARM926-EJS-based system?

Yes, there is no practical difference in the way they are defined at
the moment, but the __raw_ versions are still wrong, you should
really never be using them from driver code.

> > > diff --git a/arch/arm/mach-mcs814x/include/mach/cpu.h b/arch/arm/mach-
> mcs814x/include/mach/cpu.h
> > > new file mode 100644
> > > index 0000000..1ef3c4a
> > > --- /dev/null
> > > +++ b/arch/arm/mach-mcs814x/include/mach/cpu.h
> > > @@ -0,0 +1,16 @@
> > > +#ifndef __ASM_ARCH_CPU_H__
> > > +#define __ASM_ARCH_CPU_H__
> > > +
> > > +#include <asm/cputype.h>
> > > +
> > > +#define MCS8140_ID	0x41069260	/* ARM926EJ-S */
> > > +#define MCS814X_MASK	0xff0ffff0
> > > +
> > > +#ifdef CONFIG_MCS8140
> > > +/* Moschip MCS8140 is based on an ARM926EJ-S core */
> > > +#define soc_is_mcs8140()	((read_cpuid_id() & MCS814X_MASK) == MCS8140_ID)
> > > +#else
> > > +#define soc_is_mcs8140()	(0)
> > > +#endif /* !CONFIG_MCS8140 */
> > > +
> > > +#endif /* __ASM_ARCH_CPU_H__ */
> > 
> > Can you explain what this is used for? Are there other SoCs in the same
> > platform that will get added later?
> 
> Yes, that's the plan.

Ok. But why do you need to know the soc type? If it's only for the
uncompress.h code, it should probably stay local to that file.

> > > +/* IRQ controller register offset */
> > > +#define MCS814X_IRQ_ICR		0x00
> > > +#define MCS814X_IRQ_ISR		0x04
> > > +#define MCS814X_IRQ_MASK	0x20
> > > +#define MCS814X_IRQ_STS0	0x40
> > 
> > I'm pretty sure these can be in irq.c
> 
> Not unless I switch to MULTI_IRQ_HANDLER.

I think you should do that, too. Forgot to mention it.

> > > +
> > > +static struct resource io_mem = {
> > > +	.name	= "PCI I/O space",
> > > +	.start	= MCS8140_PCI_HOST_BASE + MCS8140_PCI_IOMISC_BASE,
> > > +	.end	= MCS8140_PCI_HOST_BASE + MCS8140_PCI_IOMISC_BASE + SZ_64M,
> > > +	.flags	= IORESOURCE_IO,
> > > +};
> > 
> > This is wrong: MCS8140_PCI_HOST_BASE is an address in IORESOURCE_MEM space.
> > You probably also mean SZ_64K instead of SZ_64M.
> 
> I will double check with the hardware manual, but I really think it is 
> 64MBytes.

Are there any devices hardwired to your PCI bus that require more than a few
bytes of I/O space? If not, I think it should be 64KB. No known add-on
cards require more than that, and it would just waste virtual address space,
and prevents us from using the new common PCI I/O space helpers.

> > 
> > Leave this one at the default of 0x1000. MCS8140_PCI_HOST_BASE is the
> > address from the CPU's point of view, not from the bus. You just need
> > to stay out of the range of possible legacy ISA devices.
> > 
> > It's probably a good idea to keep the PCI support as a separate patch
> > for reviewing purposes.
> 
> Good idea, I will do that. Note that none of the boards I have actually have 
> PCI, so the code is just there in case someone needs it. I could also go along 
> and remove it, since I cannot test it, which explains why there are so many 
> inconsistencies with it.

Fair enough. Best drop it for now then. The PCI code is currently undergoing
some changes, and when someone wants to get this to work, they should can
probably better start from scratch with an independent bus driver that will
end up looking much simpler than what you have today.

	Arnd