[U-Boot] [PATCH] dm: ns16550: Add support for reg-offset property

[Michal Simek <monstr@monstr.eu>]

On 29.2.2016 06:15, Simon Glass wrote:
> Hi,
> 
> On 28 February 2016 at 21:58, Derald D. Woods <woods.technical@gmail.com> wrote:
>> On Sun, Feb 28, 2016 at 5:51 PM, Derald D. Woods <woods.technical@gmail.com>
>> wrote:
>>>>
>>>> On 02/28/2016 05:45 PM, Derald D. Woods wrote:
>>>>>
>>>>> On 02/28/2016 04:39 PM, Alexander Graf wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 02/25/2016 02:38 PM, Derald D. Woods wrote:
>>>>>>>
>>>>>>> On Thu, Feb 25, 2016 at 09:11:24AM +0100, Michal Simek wrote:
>>>>>>>>
>>>>>>>> On 25.2.2016 05:47, Derald D. Woods wrote:
>>>>>>>>>
>>>>>>>>> On Wed, Feb 24, 2016 at 12:26:09PM +0100, Michal Simek wrote:
>>>>>>>>>>
>>>>>>>>>> On 24.2.2016 11:56, Adam Ford wrote:
>>>>>>>>>>>
>>>>>>>>>>> On Tue, Feb 23, 2016 at 12:38 AM, Simon Glass <sjg@chromium.org>
>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> Hi Michal,
>>>>>>>>>>>>
>>>>>>>>>>>> On 22 February 2016 at 00:40, Michal Simek
>>>>>>>>>>>> <michal.simek@xilinx.com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 19.2.2016 21:55, Simon Glass wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi Michal,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 16 February 2016 at 08:17, Michal Simek
>>>>>>>>>>>>>> <michal.simek@xilinx.com> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> reg-offset is the part of standard 8250 binding in the kernel.
>>>>>>>>>>>>>>> It is shifting start of address space by reg-offset.
>>>>>>>>>>>>>>> On Xilinx platform this offset is typically 0x1000.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>    drivers/serial/ns16550.c | 6 ++++--
>>>>>>>>>>>>>>>    include/ns16550.h        | 1 +
>>>>>>>>>>>>>>>    2 files changed, 5 insertions(+), 2 deletions(-)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Do you support the debug UART feature on your boards?
>>>>>>>>>>>>>
>>>>>>>>>>>>> yes. I do support it but there you can put just address plus
>>>>>>>>>>>>> offset and
>>>>>>>>>>>>> there is no reason to add one more option to Kconfig.
>>>>>>>>>>>>> But let me know if you think that this is incorrect flow.
>>>>>>>>>>>
>>>>>>>>>>> This patch seems to break my OMAP3 board.  Does anyone know if I
>>>>>>>>>>> need
>>>>>>>>>>> to set a certain offset for OMAP3 to make this work (and where is
>>>>>>>>>>> the
>>>>>>>>>>> right place for it) ?
>>>>>>>>>>
>>>>>>>>>> Are you using DT init? Check your DT description if there is
>>>>>>>>>> reg-offset
>>>>>>>>>> property. I expect if your board worked before and you remove this
>>>>>>>>>> property it will start to work again.
>>>>>>>>>>
>>>>>>>>> I am seeing the same problem with my BeagleBoard Rev. C4. There is
>>>>>>>>> something common, to more than one board, happening with this
>>>>>>>>> commit.
>>>>>>>>
>>>>>>>> You should enable debug console and send the log.
>>>>>>>> Do you have enough space for malloc?
>>>>>>>>
>>>>>>> I will have little time this weekend to go further. Some things will
>>>>>>> need to be un-configured to have enough space. I am around 7 KiB over
>>>>>>> with DEBUG enabled.
>>>>>>
>>>>>>
>>>>>> I'm not quite sure what exactly is going wrong here - maybe some asm
>>>>>> code
>>>>>> is accessing the fields without proper offset generation?
>>>>>>
>>>>>> Either way, the patch below seems to fix the issue for me (on
>>>>>> beaglebone):
>>>>>>
>>>>>> diff --git a/include/ns16550.h b/include/ns16550.h
>>>>>> index 5eeacd6..1311f4c 100644
>>>>>> --- a/include/ns16550.h
>>>>>> +++ b/include/ns16550.h
>>>>>> @@ -54,9 +54,9 @@
>>>>>>    */
>>>>>>   struct ns16550_platdata {
>>>>>>          unsigned long base;
>>>>>> -       int reg_offset;
>>>>>>          int reg_shift;
>>>>>>          int clock;
>>>>>> +       int reg_offset;
>>>>>>   };
>>>>>>
>>>>>>   struct udevice;
>>>>>>
>>>>> I see the following grep results:
>>>>>
>>>>> $ grep -RI -e "const struct ns16550_platdata" .
>>>>> ./arch/arm/cpu/armv7/am33xx/board.c:static const struct ns16550_platdata
>>>>> am33xx_serial[] = {
>>>>> ./arch/arm/cpu/arm926ejs/lpc32xx/devices.c:static const struct
>>>>> ns16550_platdata lpc32xx_uart[] = {
>>>>> ./board/timll/devkit8000/devkit8000.c:static const struct
>>>>> ns16550_platdata
>>>>> devkit8000_serial = {
>>>>> ./board/ti/beagle/beagle.c:static const struct ns16550_platdata
>>>>> beagle_serial = {
>>>>> ./board/logicpd/zoom1/zoom1.c:static const struct ns16550_platdata
>>>>> zoom1_serial = {
>>>>> ./board/logicpd/omap3som/omap3logic.c:static const struct
>>>>> ns16550_platdata
>>>>> omap3logic_serial = {
>>>>> ./board/quipos/cairo/cairo.c:static const struct ns16550_platdata
>>>>> cairo_serial = {
>>>>> ./board/lge/sniper/sniper.c:static const struct ns16550_platdata
>>>>> serial_omap_platdata = {
>>>>> ./board/isee/igep00x0/igep00x0.c:static const struct ns16550_platdata
>>>>> igep_serial = {
>>>>> ./board/overo/overo.c:static const struct ns16550_platdata overo_serial
>>>>> =
>>>>> {
>>>>>
>>>>> Could the use of 'const' be a part of the problem?
>>>>>
>>>>> - Derald
>>>>>
>>>>>
>>>> The structure initializers need rework for the additional member.
>>>>
>>>>
>>>> - Derald
>>>> _______________________________________________
>>>> U-Boot mailing list
>>>> U-Boot at lists.denx.de
>>>> http://lists.denx.de/mailman/listinfo/u-boot
>>
>> On 02/28/2016 09:56 PM, Adam Ford wrote:
>>>
>>> I first tried removing the 'const' in the board file as suggested by
>>> Derald, but that wasn't successful.  I can boot with Alexander's
>>> patch, but modifying the order inside the header seems weird to me.  I
>>> haven't had any time to  look this weekend, but I wonder if something
>>> in one of the files is modifying the structure and expects the order
>>> of the structure to always be a certain order.
>>>
>>> adam
>>>
>>>
>>
>> According to "doc/driver-model/README.txt" the use of 'U_BOOT_DEVICE' will
>> the problematic going forward.
>>
>> For now, I would expect something like the following would work, without
>> modifying the structure again:
>>
>> [board/logicpd/omap3som/omap3logic.c]
>> ...
>> static struct ns16550_platdata omap3logic_serial = {
>>     OMAP34XX_UART1,
>>     0,
>>     2,
>>     V_NS16550_CLK
>> };
>> ...
>>
>> [board/ti/beagle/beagle.c]
>> ...
>> static const struct ns16550_platdata beagle_serial = {
>>     OMAP34XX_UART3,
>>     0,
>>     2,
>>     V_NS16550_CLK
>> };
>> ...
>>
>> All static instances of the structure initialization would be incorrect
>> without adding the new 'reg_offset' member after 'base'.
>>
>> If 'U_BOOT_DEVICE' is not expected to work anymore, then efforts may need to
>> be directed towards the new DM/FDT way.
> 
> Yes indeed.
> 
> For platform data I'd recommend adding member names.

yes please.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160229/bd7b4292/attachment.sig>