From: Alexey Charkov <alchark@gmail.com>
To: Ryan Mallon <ryan@bluewatersys.com>
Cc: "Russell King - ARM Linux" <linux@arm.linux.org.uk>,
linux-arm-kernel@lists.infradead.org,
vt8500-wm8505-linux-kernel@googlegroups.com,
"Eric Miao" <eric.y.miao@gmail.com>,
"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
"Albin Tonnerre" <albin.tonnerre@free-electrons.com>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/6 v9] ARM: Add basic architecture support for VIA/WonderMedia 85xx SoC's
Date: Tue, 21 Dec 2010 13:00:43 +0300 [thread overview]
Message-ID: <AANLkTi=7O-HwQRcybnLSt+r+A64g=D7rZY7NbdEMFys3@mail.gmail.com> (raw)
In-Reply-To: <4D1011CF.9080800@bluewatersys.com>
2010/12/21 Ryan Mallon <ryan@bluewatersys.com>:
> On 12/21/2010 12:49 PM, Alexey Charkov wrote:
>> 2010/12/21 Ryan Mallon <ryan@bluewatersys.com>:
>>> On 12/21/2010 12:00 PM, Alexey Charkov wrote:
>>>> 2010/12/21 Ryan Mallon <ryan@bluewatersys.com>:
>>>>> On 12/21/2010 10:48 AM, Alexey Charkov wrote:
>>>>>> 2010/12/20 Ryan Mallon <ryan@bluewatersys.com>:
>>>>>>> On 12/21/2010 08:54 AM, Alexey Charkov wrote:
>>>>>>>> This adds support for the family of Systems-on-Chip produced initially
>>>>>>>> by VIA and now its subsidiary WonderMedia that have recently become
>>>>>>>> widespread in lower-end Chinese ARM-based tablets and netbooks.
>>>>>>>>
>>>>>>>> Support is included for both VT8500 and WM8505, selectable by a
>>>>>>>> configuration switch at kernel build time.
>>>>>>>>
>>>>>>>> Included are basic machine initialization files, register and
>>>>>>>> interrupt definitions, support for the on-chip interrupt controller,
>>>>>>>> high-precision OS timer, GPIO lines, necessary macros for early debug,
>>>>>>>> pulse-width-modulated outputs control, as well as platform device
>>>>>>>> configurations for the specific drivers implemented elsewhere.
>>>>>>>>
>>>>>>>> Signed-off-by: Alexey Charkov <alchark@gmail.com>
>>>>>>>
>>>>>>> Hi Alexey,
>>>>>>>
>>>>>>> Quick review below.
>>>>>
>>>>>> <snip>
>>>>>>>> +void __init wmt_set_resources(void)
>>>>>>>> +{
>>>>>>>> + resources_lcdc[0].start = wmt_current_regs->lcdc;
>>>>>>>> + resources_lcdc[0].end = wmt_current_regs->lcdc + SZ_1K - 1;
>>>>>>>> + resources_lcdc[1].start = wmt_current_irqs->lcdc;
>>>>>>>> + resources_lcdc[1].end = wmt_current_irqs->lcdc;
>>>>>>>
>>>>>>> Ah, this makes more sense. But why have all the indirection? The
>>>>>>> wmt_regmaps table could just be replaced with #defines and then have
>>>>>>> separate device files for the VT8500 and the WM8505. This would also
>>>>>>> make clearer which variants have which peripherals.
>>>>>>>
>>>>>>
>>>>>> This was the way I implemented it originally. However, Arnd made quite
>>>>>> a valid suggestion to allow runtime selection of the chip variant,
>>>>>> thus registers and interrupts need to be held in an indexed data type
>>>>>> instead of just compile-time macros. In addition, there is now some
>>>>>> overall movement towards unification of binary kernel images for
>>>>>> different ARM variants (as far as I can see), so this would be
>>>>>> required in any case.
>>>>>>
>>>>>> Furthermore, as with many unbranded Chinese products, it's somewhat
>>>>>> difficult to reliably determine the exact chip version used in your
>>>>>> netbook without disassembling it. Reading a hardware register for
>>>>>> identification is easier :)
>>>>>
>>>>> Okay, that makes sense. I still think there must be a better way than
>>>>> having a massive indirect table with all the values. Why not detect the
>>>>> variant in the core code and then have something like:
>>>>>
>>>>> int init_devices(void)
>>>>> {
>>>>> int board_type = detect_board_type();
>>>>>
>>>>> switch (board_type) {
>>>>> case BOARD_TYPE_VT8500:
>>>>> return vt8500_init_devices();
>>>>>
>>>>> case BOARD_TYPE_WM8505:
>>>>> return wm8500_init_devices();
>>>>> }
>>>>>
>>>>> pr_err("Unknown board type\n");
>>>>> BUG(); /* panic()? */
>>>>> while (1)
>>>>> ;
>>>>> }
>>>>>
>>>>> Then you can have the peripheral setup for each of the variants in their
>>>>> own files and use #defines. It may get tricky in a couple of places if
>>>>> you need to be able to access some value directly which is different on
>>>>> each of the variants, but that can be handled on a case by case basis.
>>>>> The majority of the numbers will be passed into drivers via the resource
>>>>> structs.
>>>>>
>>>>
>>>> This is more or less what I'm doing right now - except for the
>>>> separation between different files. I tried to avoid duplication of
>>>> similar things here. Is the indirect table really a big issue? I'm a
>>>> bit reluctant to copy about the whole devices.c for each chip variant,
>>>> which would be otherwise required. Further, it would add more
>>>> complexity to the timer, irq, gpio, i8042 and probably some other
>>>> places.
>>>
>>> Yeah, agreed about the duplication. My approach would require the common
>>> peripherals to be defined for each variant. I'm not entirely against the
>>> indirect table (and am even starting to think it may be the best overall
>>> solution), it's just that it can be a bit difficult to follow because to
>>> add a device you need to do the following:
>>>
>>> - Add a partially empty resource/platform_device struct
>>> - Add resource entries to the resource table definition
>>> - Add resource values to the resource table
>>> - Add assignment of resource values to device init code
>>>
>>
>> That's actually only one step more than what machines with static
>> resource definitions require (the last one). Flexibility does come at
>> a cost, so there should be a mathematical limit to optimization of
>> this thing :)
>
> No it isn't. You don't have the massive table, which requires
> modifications to both the definition and declaration, on machines with
> static resource definitions.
>
> How about using the resource structures directly rather than introducing
> the table which is effectively holding the same information? Something
> like this?
>
> In vt8500_resources.c (and similarly for wm8505_resources.c):
>
> static struct resource vt8500_resources_uart0[] __initdata = {
> [0] = {
> .flags = IORESOURCE_MEM,
> .start = VT8500_UART0_PHYS_BASE,
> .end = VT8500_UART0_PHYS_BASE + 0xff,
> },
> [1] = {
> .flags = IORESOURCE_IRQ,
> .start = VT8500_UART0_IRQ,
> .end = VT8500_UART0_IRQ,
> },
> };
>
> struct resource *vt8500_resources[] __initdata = {
> [VT8500_UART0] = &vt8500_resources_uart0,
> ...
> };
>
> In devices.c:
>
> extern struct resource *vt8500_resources;
> extern struct resource *wm8505_resources;
>
> /* Set this pointer according to board variant */
> static struct resource *resources;
>
> void __init wmt_set_resources(void)
> {
> vt8500_device_uart0.resource = resources[VT8500_UART0];
> ...
> }
>
> This way we only have a single externed resource structure per
> board-variant, there is no additional table needed, and the resource
> definitions can be read clearly. Alternatively the wmt_regmaps/wmt_irqs
> tables could be modified to use struct resource rather than individual
> fields which would simplify the assignments later.
This way we will again duplicate quite much: those files will mostly
differ in just the macro definitions of specific registers/irqs.
What if I just move all the initializations inside my runtime helper
function, and add a macro to save space and improve readability?
Something along the lines of:
static struct resource resources_lcdc[2] __initdata;
#define WMT_MMIO_RES(__start, __size) \
{\
.flags = IORESOURCE_MEM,\
.start = __start,\
.end = __start + __size - 1,\
}
#define WMT_IRQ_RES(__irq) \
{\
.flags = IORESOURCE_IRQ,\
.start = __irq,\
.end = __irq,\
}
void __init wmt_set_resources(void)
{
resources_lcdc[0] = WMT_MMIO_RES(wmt_current_regs->lcdc, SZ_1K);
resources_lcdc[1] = WMT_IRQ_RES(wmt_current_irqs->lcdc);
...
}
Then there will be no half-empty initializations scattered around
separate from the other assignments (which is probably the worst thing
in current configuration).
Best regards,
Alexey
next prev parent reply other threads:[~2010-12-21 10:00 UTC|newest]
Thread overview: 91+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-11-07 16:28 [PATCH 1/6 v2] ARM: Add basic architecture support for VIA/WonderMedia 85xx SoC's Alexey Charkov
2010-11-07 16:28 ` [PATCH 2/6 v2] serial: Add support for UART on VIA VT8500 and compatibles Alexey Charkov
2010-11-07 23:08 ` Alan Cox
2010-11-08 0:58 ` [PATCH 2/6 v3] " Alexey Charkov
2010-11-08 10:46 ` Alan Cox
2010-11-08 17:33 ` [PATCH 2/6 v4] " Alexey Charkov
2010-11-07 16:28 ` [PATCH 3/6 v2] input: Add support for VIA VT8500 and compatibles in i8042 Alexey Charkov
2010-11-12 22:54 ` Alexey Charkov
2010-11-12 23:30 ` Dmitry Torokhov
2010-11-13 0:00 ` Alexey Charkov
2010-12-22 21:41 ` [PATCH 3/6 v3] " Alexey Charkov
2010-11-07 16:28 ` [PATCH 4/6 v2] usb: Add support for VIA VT8500 and compatibles in EHCI HCD Alexey Charkov
2010-11-07 16:28 ` [PATCH 5/6 v2] rtc: Add support for the RTC in VIA VT8500 and compatibles Alexey Charkov
2010-11-12 22:53 ` Alexey Charkov
2010-11-13 12:14 ` Lars-Peter Clausen
2010-11-13 23:56 ` [PATCH 5/6 v3] " Alexey Charkov
2010-11-14 15:50 ` Lars-Peter Clausen
2010-11-14 17:00 ` [PATCH 5/6 v4] " Alexey Charkov
2010-11-23 19:17 ` Alexey Charkov
2010-11-24 19:23 ` Lars-Peter Clausen
2010-11-24 22:47 ` [PATCH 5/6 v5] " Alexey Charkov
2010-11-07 16:28 ` [PATCH 6/6 v2] ARM: Add support for the display controllers in VT8500 and WM8505 Alexey Charkov
2010-11-08 4:17 ` Paul Mundt
2010-11-08 12:56 ` Alexey Charkov
2010-11-08 14:14 ` [PATCH 6/6 v3] " Alexey Charkov
2010-11-08 20:43 ` Paul Mundt
2010-11-08 21:15 ` Alexey Charkov
2010-11-08 21:30 ` Paul Mundt
2010-11-08 23:42 ` [PATCH 6/6 v4] " Alexey Charkov
2010-11-08 23:54 ` Paul Mundt
2010-11-09 0:03 ` Alexey Charkov
2010-11-09 7:36 ` Guennadi Liakhovetski
2010-11-09 9:39 ` Paul Mundt
2010-11-09 9:49 ` Alexey Charkov
2010-11-09 10:33 ` [PATCH 6/6 v3] " Russell King - ARM Linux
2010-11-09 10:51 ` Paul Mundt
2010-11-09 11:04 ` Russell King - ARM Linux
2010-11-09 13:02 ` Geert Uytterhoeven
2010-11-09 13:33 ` Arnd Bergmann
2010-11-09 16:20 ` Paul Mundt
2010-11-08 8:47 ` [PATCH 6/6 v2] " Arnd Bergmann
2010-11-09 10:23 ` Alexey Charkov
2010-11-09 15:03 ` Arnd Bergmann
2010-11-07 16:57 ` [PATCH 1/6 v2] ARM: Add basic architecture support for VIA/WonderMedia 85xx SoC's Russell King - ARM Linux
2010-11-07 17:08 ` Alexey Charkov
2010-11-07 17:17 ` Russell King - ARM Linux
2010-11-07 18:25 ` [PATCH 1/6 v3] " Alexey Charkov
2010-11-08 17:19 ` [PATCH 1/6 v4] " Alexey Charkov
2010-11-10 15:16 ` saeed bishara
2010-11-10 15:18 ` Russell King - ARM Linux
2010-11-10 15:20 ` saeed bishara
2010-11-11 21:23 ` [PATCH 1/6 v5] " Alexey Charkov
2010-11-11 23:49 ` Russell King - ARM Linux
2010-11-12 16:54 ` [PATCH 1/6 v6] " Alexey Charkov
2010-11-12 20:14 ` Alexey Charkov
2010-11-23 19:50 ` [PATCH 1/6 v7] " Alexey Charkov
2010-12-19 17:40 ` [PATCH 1/6 v8] " Alexey Charkov
2010-12-20 18:15 ` Arnd Bergmann
2010-12-20 19:15 ` Russell King - ARM Linux
2010-12-20 19:26 ` Alexey Charkov
2010-12-20 19:54 ` [PATCH 1/6 v9] " Alexey Charkov
2010-12-20 20:50 ` Ryan Mallon
2010-12-20 21:48 ` Alexey Charkov
2010-12-20 22:23 ` Ryan Mallon
2010-12-20 23:00 ` Alexey Charkov
2010-12-20 23:22 ` Ryan Mallon
2010-12-20 23:49 ` Alexey Charkov
2010-12-21 0:09 ` Alexey Charkov
2010-12-21 2:32 ` Ryan Mallon
2010-12-21 10:00 ` Alexey Charkov [this message]
2010-12-21 12:05 ` Arnd Bergmann
2010-12-21 19:39 ` Ryan Mallon
2010-12-22 21:18 ` [PATCH 1/6 v10] " Alexey Charkov
2010-12-22 21:52 ` Ryan Mallon
2010-12-22 22:02 ` Alexey Charkov
2010-12-22 22:32 ` Ryan Mallon
2010-12-22 22:25 ` [PATCH 1/6 v11] " Alexey Charkov
2010-12-22 22:59 ` Ryan Mallon
2010-12-22 23:38 ` [PATCH 1/6 v12] " Alexey Charkov
2010-12-28 14:52 ` Alexey Charkov
2011-01-15 0:53 ` Alexey Charkov
2011-01-15 0:58 ` Russell King - ARM Linux
2011-01-15 1:13 ` Alexey Charkov
2011-01-21 10:43 ` Russell King - ARM Linux
2011-07-06 12:34 ` [PATCH 1/6 v8] " Russell King - ARM Linux
2011-07-07 7:13 ` Alexey Charkov
2011-07-07 7:54 ` Arnd Bergmann
2011-07-07 7:59 ` Russell King - ARM Linux
2011-07-07 8:05 ` Arnd Bergmann
2010-11-07 17:00 ` [PATCH 1/6 v2] " Russell King - ARM Linux
2010-11-07 17:16 ` Alexey Charkov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='AANLkTi=7O-HwQRcybnLSt+r+A64g=D7rZY7NbdEMFys3@mail.gmail.com' \
--to=alchark@gmail.com \
--cc=albin.tonnerre@free-electrons.com \
--cc=eric.y.miao@gmail.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@arm.linux.org.uk \
--cc=ryan@bluewatersys.com \
--cc=u.kleine-koenig@pengutronix.de \
--cc=vt8500-wm8505-linux-kernel@googlegroups.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).