[U-Boot] [PATCH] dm: ns16550: Add support for reg-offset property
Simon Glass
sjg at chromium.org
Mon Feb 29 06:15:08 CET 2016
Hi,
On 28 February 2016 at 21:58, Derald D. Woods <woods.technical at gmail.com> wrote:
> 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.
Yes indeed.
For platform data I'd recommend adding member names.
Regards,
Simon
More information about the U-Boot
mailing list