On Mon, Mar 30, 2009 at 10:21:05PM -0700, Kevin Hilman wrote:
> "Mark A. Greer" <mgr...@mvista.com> writes:
> > There are differences, however. Some of those differences
> > prevent support for davinci and da830 platforms to work
> > in the same kernel binary. Those differences are:
> >
> > 1) Different physical address for RAM. This is relevant
> > to Makefile.boot addresses and PHYS_OFFSET. The
> > Makefile.boot issue isn't truly a kernel issue but
> > it means u-boot won't work with a uImage including
> > both architectures. The PHYS_OFFSET issue is
> > addressed by the "Allow for runtime-determined
> > PHYS_OFFSET" patch by Lennert Buytenhek but it
> > hasn't been accepted yet.
>
> Other than (2), have you verified that the the PHYS_OFFSET patch
> submitted can build and boot a single kernel for da8xx and the
> DAVINCI_TRUE SoCs?
I did back when I was messing with the runtime PHYS_OFFSET patch.
> > The da830 currently has an issue with writeback data
> > cache so CONFIG_CPU_DCACHE_WRITETHROUGH is forced on
> > when CONFIG_ARCH_DAVINCI_DA830 is enabled.
>
> Hmm, any thoughts on how this issue might be addressed with a single
> kernel? Doesn't seem too simple. I assume a writethrough kernel works
> fine on the other DaVincis as well.
I haven't tried it because they didn't need it. I would expect it to
work.
> What exactly is the "issue" being worked around. Is it a HW bug? Is
> this workaround always required?
Sekhar beat me to this one.
> Is this something that could be changed slightly later in the boot by
> SoC specific code?
Nope. But, for future ease, I don't think I'll force it on if its a
da830. Instead, I'll just put it in the defconfig for the da830 so
its easier to remove when the fixed part finds its way out.
> > +config ARCH_DAVINCI_TRUE
> > + bool
> > +
>
> Still not crazy about the name "true" here. Seems to unnecessarily
> belittle the da830. ;)
>
> Maybe ARCH_DAVINCI_DMx or ARCH_DAVINCI_COMMON?
Definitely not _COMMON since that makes me think the da8xx should be
included. I like ARCH_DAVINCI_DMx, though. Thanks for suggestion.
> In any case, I think that Kconfig item could disappear once the issues
> you mentioned in the description are sorted out.
OK.
> > +config ARCH_DAVINCI_DA830
> > + bool "DA830/OMAP-L137 based system"
> > + depends on !ARCH_DAVINCI_TRUE
>
> I suggest you drop this restriction and let a compile be attempted.
> The #errors inserted below will maybe irritate someone to help fix the
> problems. :)
I disagree but not that much so I'll do it.
The reason I disagree is that all that really does is allow someone to
waste time getting the error, then having to go back into make config
and restart the build. Bit of a PITA.
> > diff --git a/arch/arm/mach-davinci/Makefile b/arch/arm/mach-davinci/Makefile
> > index fab706e..b50bc41 100644
> > --- a/arch/arm/mach-davinci/Makefile
> > +++ b/arch/arm/mach-davinci/Makefile
> > @@ -15,10 +15,13 @@ obj-$(CONFIG_TI_DAVINCI_EMAC_MODULE) += mac_addr.o
> > obj-$(CONFIG_ARCH_DAVINCI_DM644x) += dm644x.o
> > obj-$(CONFIG_ARCH_DAVINCI_DM646x) += dm646x.o
> > obj-$(CONFIG_ARCH_DAVINCI_DM355) += dm355.o
> > +obj-$(CONFIG_ARCH_DAVINCI_DA830) += da830.o devices-da830.o
> >
> > obj-$(CONFIG_AINTC) += irq.o
> > obj-$(CONFIG_CP_INTC) += cp_intc.o
> >
> > +obj-$(CONFIG_ARCH_DAVINCI_TRUE) += devices.o
> > +
>
> How about appending devices.o to dm*.c above and dropping this use of
> DAVINCI_TRUE.
That's how I originally had it but I figured we have
CONFIG_ARCH_DAVINCI_TRUE anyway, may as well use it.
I don't really care but it seems a bit cleaner this way.
> > +#ifdef CONFIG_DAVINCI_MUX
> > +/*
> > + * Device specific mux setup
> > + *
> > + * soc description mux mode mode mux dbg
> > + * reg offset mask mode
> > + */
> > +static const struct mux_config da830_pins[] = {
>
> As pointed out in the MUX patch, move the #ifdef here, and drop the other
> #ifdefs.
OK
> > +++ b/arch/arm/mach-davinci/include/mach/da830.h
>
> [...]
>
> > +
> > +#define DA830_ARM_INTC_BASE 0xfffee000
> > +#define DA830_CP_INTC_SIZE SZ_8K
> > +#define DA830_CP_INTC_VIRT (IO_VIRT - DA830_CP_INTC_SIZE - SZ_4K)
>
> I think a comment is missing here as to the need for this virtual
> mapping. I understand it now, but I know someday I'll forget why.
> That would also clarify why you're not using IO_ADDRESS() here.
OK
> > +enum {
> > + DA830_PDEV_EDMA,
> > + DA830_PDEV_I2C_0,
> > + DA830_PDEV_I2C_1,
> > + DA830_PDEV_SPI_0,
> > + DA830_PDEV_SPI_1,
> > + DA830_PDEV_WATCHDOG,
> > + DA830_PDEV_USB_20,
> > + DA830_PDEV_USB_11,
> > + DA830_PDEV_EMAC,
> > + DA830_PDEV_MMC,
> > + DA830_PDEV_RTC,
> > + DA830_PDEV_EQEP_0,
> > + DA830_PDEV_EQEP_1,
> > + DA830_PDEV_LCDC,
> > + DA830_PDEV_NUM
> > +};
> > +
> > +struct da830_pdata {
> > + int dev; /* Device to enable */
> > + void *pdata; /* platform_data for this device */
> > +};
> > +
> > +void da830_add_devices(struct da830_pdata pdata[DA830_PDEV_NUM],
> > + unsigned long num);
> > +
>
> Maybe it's getting too late for me, but I don't see the point of these enums
> and the extra array of dev/pdata associations.
>
> I think it's simpler and clearer for the SoC code to just register all the
> devices and platform data all the time.
There is a patch from Steve Chen (which you haven't seen) that
automatically sets up the pinmux from clk_enable. The patch is
really nice for 2 reason:
1) The da830 has sooo many pinmux regs to set up, its really
nice to have it done automatically (and not having a hundred
davinci_cfg_reg() calls in every board file).
2) It detects and complains about pinmux contention and there
is a lot of potential contention with the da830.
So, assuming that patch eventually makes it in, we can't just register
every device because the drivers can pick unwanted devices, clk_enable
them and break some other device because it stole its pins.
For example, uart2 (console) and spi1 can't be set up at the same time
but we definitely want spi0 enabled. So, with the spi driver enabled
for spi0 and the device data for spi1 there, the spi driver will
clk_enable spi1 causing it to steal uart2's pins.
That's all in the future but I wanted things set up so its was easy
for the board code to only enable what it needs.
> > diff --git a/arch/arm/mach-davinci/include/mach/irqs.h
> > b/arch/arm/mach-davinci/include/mach/irqs.h
> > +#ifdef CONFIG_ARCH_DAVINCI_DA830
> > +#define DAVINCI_N_GPIO 128
> > +#define NR_IRQS (DA830_N_CP_INTC_IRQ +
> > DAVINCI_N_GPIO)
> > +#else
> > +#define DAVINCI_N_GPIO 104
> > +#define NR_IRQS (DAVINCI_N_AINTC_IRQ +
> > DAVINCI_N_GPIO)
> > +#endif
> > +
>
> No #ifdef here please. These #defines are to reflect the maximum
> number on any supported SoC. If there's an #ifdef here, then the next
> chip to add support will add another #ifdef and will most likely break
> multi-SoC support.
>
> Just drop the #idef and do something like:
>
> /* Define the maximum value of any SoC */
> #define DAVINCI_N_GPIO 128 /* da8xx has most GPIO
> lines */
> #define NR_IRQS 96 /* CP_INTC has more than
> AINTC */
OK.
Mark
--
_______________________________________________
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
