[PATCH 3/9] ARM: pxa: support pxa95x
Eric Miao
eric.y.miao at gmail.com
Tue Nov 23 01:13:39 EST 2010
On Tue, Nov 23, 2010 at 12:11 PM, Haojian Zhuang <hzhuang1 at marvell.com> wrote:
>
>
>>-----Original Message-----
>>From: Eric Miao [mailto:eric.y.miao at gmail.com]
>>Sent: 2010年11月22日 2:59 PM
>>To: Haojian Zhuang
>>Cc: linux-arm-kernel at lists.infradead.org
>>Subject: Re: [PATCH 3/9] ARM: pxa: support pxa95x
>>
>>On Mon, Nov 22, 2010 at 12:18 PM, Haojian Zhuang <hzhuang1 at marvell.com> wrote:
>>>
>>>
>>>>-----Original Message-----
>>>>From: Eric Miao [mailto:eric.y.miao at gmail.com]
>>>>Sent: 2010年11月22日 9:08 AM
>>>>To: Haojian Zhuang
>>>>Cc: linux-arm-kernel at lists.infradead.org
>>>>Subject: Re: [PATCH 3/9] ARM: pxa: support pxa95x
>>>>
>>>>On Fri, Nov 19, 2010 at 5:16 PM, Haojian Zhuang <hzhuang1 at marvell.com> wrote:
>>>>>
>>>>>
>>>>>>-----Original Message-----
>>>>>>From: Eric Miao [mailto:eric.y.miao at gmail.com]
>>>>>>Sent: 2010年11月19日 4:45 PM
>>>>>>To: Haojian Zhuang
>>>>>>Cc: linux-arm-kernel at lists.infradead.org
>>>>>>Subject: Re: [PATCH 3/9] ARM: pxa: support pxa95x
>>>>>>
>>>>>>On Wed, Nov 17, 2010 at 7:03 PM, Haojian Zhuang
>>>>>><haojian.zhuang at marvell.com> wrote:
>>>>>>> The core of PXA955 is PJ4. Add new PJ4 support. And add new macro
>>>>>>> CONFIG_PXA95x.
>>>>>>>
>>>>>>> Signed-off-by: Haojian Zhuang <haojian.zhuang at marvell.com>
>>>>>>> Cc: Eric Miao <eric.y.miao at gmail.com>
>>>>>>> ---
>>>>>>> arch/arm/mach-pxa/Kconfig | 12 +-
>>>>>>> arch/arm/mach-pxa/Makefile | 1 +
>>>>>>> arch/arm/mach-pxa/clock.h | 20 ++
>>>>>>> arch/arm/mach-pxa/devices.c | 267 +++++++++---------
>>>>>>> arch/arm/mach-pxa/generic.c | 8 +-
>>>>>>> arch/arm/mach-pxa/generic.h | 3 +-
>>>>>>> arch/arm/mach-pxa/include/mach/hardware.h | 34 ++-
>>>>>>> arch/arm/mach-pxa/include/mach/irqs.h | 1 +
>>>>>>> arch/arm/mach-pxa/pxa3xx.c | 9 -
>>>>>>> arch/arm/mach-pxa/pxa930.c | 2 +-
>>>>>>> arch/arm/mach-pxa/pxa95x.c | 437
>>>>>>+++++++++++++++++++++++++++++
>>>>>>> arch/arm/mm/Kconfig | 6 +
>>>>>>> arch/arm/plat-pxa/Makefile | 1 +
>>>>>>> arch/arm/plat-pxa/include/plat/mfp.h | 4 +-
>>>>>>> 14 files changed, 643 insertions(+), 162 deletions(-)
>>>>>>> create mode 100644 arch/arm/mach-pxa/pxa95x.c
>>>>>>>
>>>>>>> +void __init pxa95x_init_irq(void)
>>>>>>> +{
>>>>>>
>>>>>>No enabling of CP6 is needed for pxa95x?
>>>>>>
>>>>> Yes, PXA95x can't support to access interrupt registers via co-processors.
>>>>>
>>>>>>> + pxa_init_irq(96, NULL);
>>>>>>> + pxa_init_ext_wakeup_irq(NULL);
>>>>>>> + pxa_init_gpio(IRQ_GPIO_2_x, 2, 127, NULL);
>>>>>>> +}
>>>>>>> +
>>>>>>> +
>>>>>>> +static int __init pxa95x_init(void)
>>>>>>> +{
>>>>>>> + int ret = 0;
>>>>>>> +
>>>>>>> + if (cpu_is_pxa95x()) {
>>>>>>> + mfp_init_base(io_p2v(MFPR_BASE));
>>>>>>> + mfp_init_addr(pxa95x_mfp_addr_map);
>>>>>>> +
>>>>>>> + reset_status = ARSR;
>>>>>>> +
>>>>>>> + /*
>>>>>>> + * clear RDH bit every time after reset
>>>>>>> + *
>>>>>>> + * Note: the last 3 bits DxS are write-1-to-clear so carefully
>>>>>>> + * preserve them here in case they will be referenced later
>>>>>>> + */
>>>>>>> + ASCR &= ~(ASCR_RDH | ASCR_D1S | ASCR_D2S | ASCR_D3S);
>>>>>>> +
>>>>>>> + clkdev_add_table(pxa95x_clkregs, ARRAY_SIZE(pxa95x_clkregs));
>>>>>>> +
>>>>>>> + if ((ret = pxa_init_dma(IRQ_DMA, 32)))
>>>>>>> + return ret;
>>>>>>> +
>>>>>>
>>>>>>No need to register pxa3xx_sysdev[]? My understanding is the IRQ/MFP/GPIO
>>>>>>subsystems are just same.
>>>>>>
>>>>> I don't want to support PM feature now. So I remove them in current patch.
>>>>>
>>>>>>> + ret = platform_add_devices(devices, ARRAY_SIZE(devices));
>>>>>>> + }
>>>>>>> +
>>>>>>> + return ret;
>>>>>>> +}
>>>>>>
>>>>>>I'm afraid the above duplicates most part of pxa3xx.c, the only subtle
>>>>>>difference
>>>>>>is the missing of pxa3xx_init_pm(), which is due to the difference of the power
>>>>>>management. Instead of duplicating so much code, another better way out is:
>>>>>>
>>>>>>1) remove pxa3xx_init_pm() from pxa3xx_init()
>>>>>>2) remove pxa3xx_init() from postcore_init()
>>>>>>3) change pxa3xx_init() to pxa3xx_common_init()
>>>>>>4) in pxa300_init() and pxa320_init(), call pxa3xx_common_init() as
>>>>>>well as pxa3xx_init_pm()
>>>>>>5) in pxa95x_init(), call pxa3xx_common_init() and pxa95x's specific
>>>>>>pm initialization
>>>>>> (e.g. pxa95x_init_pm)
>>>>>>
>>>>> The name of pxa3xx_init() and pxa3xx_common_init() is very confusion.
>>>>
>>>>I know it's a bit confusing. But they are sharing the same code, and it's
>>>>only a matter of naming issue. Like many drivers we have, they are called
>>>>'pxa2xx-*' but actually able to support pxa3xx, pxa9xx and even pxa168.
>>>>
>>>>> PXA95x is based on ARMv7. I don't prefer to compile it with PXA3xx in one image
>>now.
>>>>Actually I failed to compile them together.
>>>>>
>>>>
>>>>Nah, it's not a hard requirement to build them together. It's about reducing
>>>>code duplication.
>>>>
>>>>My idea would be to separate clock and pm into separate files, making them
>>>>more self-contained driver-like, instead of hardcoding them into pxa3xx.c.
>>>>So in the end, the pxa95x_init() will be something like below:
>>>>
>>>>static int __init pxa95x_init(void)
>>>>{
>>>> if (cpu_is_pxa95x()) {
>>>> pxa_init_dma(IRQ_DMA, 32);
>>>> pxa3xx_init_pm();
>>>> pxa3xx_init_clock();
>>>> clkdev_add_table(ARRAY_AND_SIZE(pxa95x_clkregs));
>>>>
>>>> for_each(p, pxa95x_sysdevs)
>>>> sysdev_register(p);
>>>>
>>>> platform_add_devices(ARRAY_AND_SIZE(pxa95x_devices));
>>>> }
>>>>}
>>>>
>>>>So it reads like pxa95x is using the same subsystem as pxa3xx PM
>>>>and clock, and if there are any subtle differences to handle, we can
>>>>just change the API, and let know the difference through parameters,
>>>>just like pxa_init_dma().
>>>>
>>>>Let me know if you think this is better and acceptable.
>>>
>>> I don't agree invoking pxa3xx_init_pm() and pxa3xx_init_clock() in pxa95x.c. It's based
>>on two reasons of naming confusion and force building pxa3xx.c for pxa95x.
>>>
>>> If we want to share code, I suggest to move the sharing code into generic.c. So
>>pxa95x.c will call xsc3_init_pm()/xsc3_init_clock() or
>>common_init_pm()/common_init_clock(). What's your opinion?
>>>
>>
>>Now that's also confusing, cuz pxa95x is using PJ4. So a trade-off
>>would be for the naming to stick to the first SoC it appears. E.g.
>>pxa27x_keypad.c, where the keypad controller first appears on pxa27x,
>>but we are later re-using this on pxa3xx and pxa93x, and we are adding
>>device named "pxa27x-keypad" on pxa3xx and pxa93x, do you find
>>this approach acceptable?
>>
>>Regarding the naming confusion that Marvell created, I don't have any
>>other better idea.
>>
>>> Now we move PXA300/PXA310/PXA320/PXA930/PXA935 into pxa3xx family. So we
>>shouldn't invoke pxa3xx_xxx() functions in other family. Otherwise, it's hard to be
>>understood and maintained.
>>>
>>
>>If we treat things like clock, power management, and so on, as normal
>>devices, you probably won't feel too upset about the naming then. If we
>>tear the SoC apart, we'll finally end up in a list of components as:
>>
>>Core: PJ4
>>IRQ: re-using legacy pxa3xx
>>GPIO: reusing pxa3xx
>>PM: specific to pxa95x
>>CLOCK: reusing pxa3xx
>>MEM: specific to pxa95x
>>
> CLOCK: There is no problem after seperate clock code from pxa3xx.c to independent clock file. My original concern is focused on calling APIs what are in pxa3xx.c from pxa95x.c.
> MFP: a bit different on register bits definition. So suggest to move some operation from common head file to mfp-pxa3xx.c and create pxa95x_mfp_class.
That's completely fine. If necessary I don't mind having a separate
mfp-pxa95x.c for the different parts.
> Because there's always a bit difference among silicons on MFP (drive setting and low power setting) whatever it belongs to PXA series or MMP series. And we can share some code in same c file for both pxa3xx and pxa95x.
> MEM: which MEM do you mean? ISRAM allocation or SMEMC/DMEMC map region?
All of them. So it could be further divided into sub-components, and decide
if they can be shared or not.
>
> Thanks
> Haojian
>
>>(just for illustration purpose only), and so on.
>>
>>I do respect your opinions. So please let me know your concerns.
>
>
More information about the linux-arm-kernel
mailing list