Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933494Ab0LUCbO (ORCPT ); Mon, 20 Dec 2010 21:31:14 -0500 Received: from mail.bluewatersys.com ([202.124.120.130]:16218 "EHLO hayes.bluewaternz.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S933446Ab0LUCbN (ORCPT ); Mon, 20 Dec 2010 21:31:13 -0500 Message-ID: <4D1011CF.9080800@bluewatersys.com> Date: Tue, 21 Dec 2010 15:32:47 +1300 From: Ryan Mallon User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.12) Gecko/20100915 Thunderbird/3.0.8 MIME-Version: 1.0 To: Alexey Charkov CC: Russell King - ARM Linux , linux-arm-kernel@lists.infradead.org, vt8500-wm8505-linux-kernel@googlegroups.com, Eric Miao , =?UTF-8?B?VXdlIEtsZWluZS1Lw7ZuaWc=?= , Albin Tonnerre , linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/6 v9] ARM: Add basic architecture support for VIA/WonderMedia 85xx SoC's References: <1289147348-31969-1-git-send-email-alchark@gmail.com> <20101107165745.GB1759@n2100.arm.linux.org.uk> <20101107171726.GF1759@n2100.arm.linux.org.uk> <20101108171930.GA1471@alchark-u3s.lan> <20101111212322.GA15533@alchark-u3s.lan> <20101111234957.GA28735@n2100.arm.linux.org.uk> <20101219174017.GA31832@alchark-u3s> <20101220191549.GH28157@n2100.arm.linux.org.uk> <20101220195423.GA14509@alchark-u3s> <4D0FC1AE.2050708@bluewatersys.com> <4D0FD747.9020700@bluewatersys.com> <4D0FE546.7050302@bluewatersys.com> In-Reply-To: X-Enigmail-Version: 1.0.1 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6868 Lines: 171 On 12/21/2010 12:49 PM, Alexey Charkov wrote: > 2010/12/21 Ryan Mallon : >> On 12/21/2010 12:00 PM, Alexey Charkov wrote: >>> 2010/12/21 Ryan Mallon : >>>> On 12/21/2010 10:48 AM, Alexey Charkov wrote: >>>>> 2010/12/20 Ryan Mallon : >>>>>> 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 >>>>>> >>>>>> Hi Alexey, >>>>>> >>>>>> Quick review below. >>>> >>>>> >>>>>>> +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. ~Ryan -- Bluewater Systems Ltd - ARM Technology Solution Centre Ryan Mallon 5 Amuri Park, 404 Barbadoes St ryan@bluewatersys.com PO Box 13 889, Christchurch 8013 http://www.bluewatersys.com New Zealand Phone: +64 3 3779127 Freecall: Australia 1800 148 751 Fax: +64 3 3779135 USA 1800 261 2934 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/