From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id E7DE3C433EF for ; Tue, 1 Feb 2022 17:13:14 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 2495380FDD; Tue, 1 Feb 2022 18:13:13 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=ltec.ch Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: by phobos.denx.de (Postfix, from userid 109) id 0F05C83049; Tue, 1 Feb 2022 18:13:12 +0100 (CET) Received: from mail.ltec.ch (mail.ltec.ch [95.143.48.181]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id C5D588018F for ; Tue, 1 Feb 2022 18:13:01 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=ltec.ch Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=fb@ltec.ch Received: from nebula.ltec ([172.27.11.2] helo=[172.27.11.35]) by mail.ltec.ch with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.94.2) (envelope-from ) id 1nEwiC-002vgq-JT; Tue, 01 Feb 2022 18:13:00 +0100 Message-ID: <4ddd2a02-f1c7-5d1d-d1a8-063e31f761fd@ltec.ch> Date: Tue, 1 Feb 2022 18:13:00 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 Subject: Re: Early debug UART not working on AM33XX SoC Content-Language: en-US To: Simon Glass Cc: U-Boot Mailing List References: <2854cc5f-31ff-6e14-edc5-eed5da3f8213@ltec.ch> <346fb194-b902-d5e0-5ab4-06e8bc1bb8b9@ltec.ch> From: Felix Brack In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.5 at phobos.denx.de X-Virus-Status: Clean Hello Simon On 01.02.22 15:05, Simon Glass wrote: > Hi Felix, > > On Tue, 1 Feb 2022 at 03:48, Felix Brack wrote: >> >> Hello Simon, >> >> On 31.01.22 17:12, Simon Glass wrote: >>> Hi Felix, >>> >>> On Mon, 31 Jan 2022 at 02:43, Felix Brack wrote: >>>> >>>> Hello Simon >>>> >>>> On 27.01.22 18:33, Simon Glass wrote: >>>>> Hi Felix, >>>>> >>>>> On Thu, 27 Jan 2022 at 09:27, Felix Brack wrote: >>>>>> >>>>>> Hello Simon, >>>>>> >>>>>> On 27.01.22 16:05, Simon Glass wrote: >>>>>>> Hi Felix, >>>>>>> >>>>>>> On Wed, 26 Jan 2022 at 07:02, Felix Brack wrote: >>>>>>>> >>>>>>>> Hello Simon, >>>>>>>> >>>>>>>> I am trying to get the current U-Boot master working on the PDU001 >>>>>>>> board. This involves the use of an early debug UART. >>>>>>>> >>>>>>>> With commit 0dba4586 (arm: Init the debug UART) the early debug UART on >>>>>>>> the AM33XX SoC doesn't work anymore. >>>>>>>> >>>>>>>> By looking at the code involved I believe a call to >>>>>>>> setup_clocks_for_console() implemented in clock_am33xx.c before the call >>>>>>>> to debug_uart_init() is missing. This is also what happens prior to >>>>>>>> commit 0dba4586 by a call to early_system_init() which in turn calls >>>>>>>> setup_early_clocks() which then calls setup_clocks_for_console(). >>>>>>>> >>>>>>>> My quick and dirty fix consist of inserting a call in crt0.S to >>>>>>>> setup_clocks_for_console() just before the call to debug_uart_init() >>>>>>>> which was added in commit 0dba4586. I have guarded this call with >>>>>>>> #if/#endif testing for CONFIG_AM33XX. The code sequence in crt0.S now >>>>>>>> looks like this: >>>>>>>> >>>>>>>> #if defined(CONFIG_DEBUG_UART) && CONFIG_IS_ENABLED(SERIAL) >>>>>>>> #if defined(CONFIG_AM33XX) >>>>>>>> bl setup_clocks_for_console >>>>>>>> #endif >>>>>>>> bl debug_uart_init >>>>>>>> #endif >>>>>>>> >>>>>>>> However, from what I understand the crt0.S is for _all_ ARM boards hence >>>>>>>> it's probably a bad idea polluting it with such #if/#endif tests for >>>>>>>> different SoCs. What do you think? >>>>>>>> >>>>>>>> If my quick and dirty fix is not that dirty I would be happy to send a >>>>>>>> patch which would also include the removal of the currently remaining >>>>>>>> call to debug_uart_init() in am33xx/board.c >>>>>>>> >>>>>>>> Please apologize my narrow view of the matter dealing only with AM33XX SoCs. >>>>>>> >>>>>>> Are you able to put that call into board_debug_uart_init() and enable >>>>>>> CONFIG_DEBUG_UART_BOARD_INIT ? >>>>>>> >>>>>> Sure I can but there two drawbacks: >>>>>> 1. this fixes the problem only for my board, others might remain broken >>>>> >>>>> I suggest you make the function common to all boards that need it. >>>>> >>>> I will check that. Maybe the board_debug_uart_init() is not the right >>>> place then as it is board specific. Probably better to have a AM33XX >>>> platform specific function. >>> >>> Despite the board_ prefix you can define it in an SoC file, for >>> example. It may not always be board-specific. >>> >> Thanks for the hint! In fact I was not aware of that. However for the >> PDU001 board board_debug_uart_init() must remain with the board as it >> (also) configures the pin multiplexer which is specific to the PDU001 board. >> >> For the patch to be useful for other boards too, I think a platform >> specific function is required. A weak function named something like >> platform_debug_uart_init(). This function could then be implemented in >> the platform specific board.c file, in my case the one for AM33XX. > > Would it be OK to create a non-weak function that is called from > board_debug_uart_init()? > > But yes we could create another function. How about > soc_debug_uart_init() as a name, though? > >> >> But where to place the default, empty implementation? In the common file >> board_init.c probably? > > At present we have a CONFIG option to enable board_debug_uart_init() > so we could do the same for soc. But should it run first or second? > That is why I feel that doing everything from board_debug_uart_init() > might be better. > >> >> Frankly I don't feel really comfortable with my own proposal above. It >> sounds a little bit like patchwork to me. On the other hand I don't see >> a better solution given the fact that it should solve not just my >> problem. Should I use it as foundation for sending a patch anyway? >> >>>> >>>> Having said that there is still something I don't understand: commit >>>> 0dba4586 has added a call to debug_uart_init() in crt0.S but not removed >>>> any existing call to debug_uart_init(). Hence this function is called twice. >>>> Is this intentional (and if so, why ?) or are the remaining calls some >>>> sort of leftovers? >>> >>> Just that I was not confident removing it from the various affected >>> boards since some have the same problem as you found with yours. >>> Calling it twice is generally fine. >>> >> Agreed. It is just a little bit confusing especially with >> DEBUG_UART_ANNOUNCE enabled. > > I will do a patch to remove them and see what happens. Great! I can't imagine that this should provoke any additional problems. > > Regards, > Simon -- Regards, Felix