From mboxrd@z Thu Jan 1 00:00:00 1970 From: Derald D. Woods Date: Sun, 28 Feb 2016 22:58:08 -0600 Subject: [U-Boot] [PATCH] dm: ns16550: Add support for reg-offset property In-Reply-To: References: <70bfc564698cfd32c932a382a98321be169ccd87.1455635867.git.michal.simek@xilinx.com> <56CABB54.8070901@xilinx.com> <56CD9351.20802@xilinx.com> <20160225044745.GA25633@exodus> <56CEB72C.6050001@xilinx.com> <20160225133819.GB25633@exodus> <56D3772E.3060104@suse.de> <56D386A6.9070603@gmail.com> <56D38812.6070105@gmail.com> Message-ID: <56D3CFE0.8090309@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Sun, Feb 28, 2016 at 5:51 PM, Derald D. Woods 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 >>>>>>>>> wrote: >>>>>>>>>> Hi Michal, >>>>>>>>>> >>>>>>>>>> On 22 February 2016 at 00:40, Michal Simek >>>>>>>>>> wrote: >>>>>>>>>>> On 19.2.2016 21:55, Simon Glass wrote: >>>>>>>>>>>> Hi Michal, >>>>>>>>>>>> >>>>>>>>>>>> On 16 February 2016 at 08:17, Michal Simek >>>>>>>>>>>> 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 >>>>>>>>>>>>> --- >>>>>>>>>>>>> >>>>>>>>>>>>> drivers/serial/ns16550.c | 6 ++++-- >>>>>>>>>>>>> include/ns16550.h | 1 + >>>>>>>>>>>>> 2 files changed, 5 insertions(+), 2 deletions(-) >>>>>>>>>>>> Reviewed-by: Simon Glass >>>>>>>>>>>> >>>>>>>>>>>> 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