[PATCH 1/8] ARM: support for Moschip MCS814x SoCs
Florian Fainelli
florian at openwrt.org
Tue Jul 17 09:32:37 EDT 2012
On Tuesday 17 July 2012 13:07:23 Arnd Bergmann wrote:
> 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.
The big difference between the two is actually the CPU core, for MCS8140 they
used a genuine ARM926EJS and for MCS8142 they actually use a FA526 core.
>
> > > 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?
Sounds good.
>
> > > > +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.
Ok.
>
> > >
> > > 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.
Yeah, I like that :)
>
> Arnd
More information about the linux-arm-kernel
mailing list