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

[Derald D. Woods]

On Sun, Feb 28, 2016 at 5:51 PM, Derald D. Woods 
<woods.technical at 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 at chromium.org>
>>>>>>>>> wrote:
>>>>>>>>>> Hi Michal,
>>>>>>>>>>
>>>>>>>>>> On 22 February 2016 at 00:40, Michal Simek
>>>>>>>>>> <michal.simek at 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 at 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 at xilinx.com>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>
>>>>>>>>>>>>>    drivers/serial/ns16550.c | 6 ++++--
>>>>>>>>>>>>>    include/ns16550.h        | 1 +
>>>>>>>>>>>>>    2 files changed, 5 insertions(+), 2 deletions(-)
>>>>>>>>>>>> Reviewed-by: Simon Glass <sjg at 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.


- Derald