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 5EF4AECAAD4 for ; Thu, 1 Sep 2022 02:31:04 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 189F284A5D; Thu, 1 Sep 2022 04:28:12 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=chromium.org header.i=@chromium.org header.b="IP9cIGiM"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 0A87584A3A; Thu, 1 Sep 2022 04:28:02 +0200 (CEST) Received: from mail-oa1-x29.google.com (mail-oa1-x29.google.com [IPv6:2001:4860:4864:20::29]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 81F0784A46 for ; Thu, 1 Sep 2022 04:27:57 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=sjg@google.com Received: by mail-oa1-x29.google.com with SMTP id 586e51a60fabf-11eab59db71so24633685fac.11 for ; Wed, 31 Aug 2022 19:27:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc; bh=bCoiXm95i9cjfXoMOCXU8703IX2VwvRo1JzSANDZo4A=; b=IP9cIGiMPZmg+ZGwFKhg9+x6msCqFFjeeh8gcL5jbSQac9jN636rVxB34LJ7DGUHxN jng02fIv4HZb2DYtQDf2PndZYutOKfStVNlbcB6HooFSiBpKgp9b7bPERKeWO42wrM7R 7ouZ+c/ZplL9hNLdslAeb/tOiBWOoq/vwXXdg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc; bh=bCoiXm95i9cjfXoMOCXU8703IX2VwvRo1JzSANDZo4A=; b=uymd+OKq6f5kE86RDQ4ftfY0CDDPRmtFaVye0u/x1MFYP5DFises2ahvtkChbgnqVi HvPw/8l4uS6GPZUz42xI/adgBziZf583UPQx1R4ygugkEojrF+rzjBpMhPTwG8ecJOsb vcSva9zysqPFvCWPikcK0+iK5MJGSRL5SK+zvDcabII2Pmga3IJHR742SQUR9rERrDrR /nGxd1H8CfnEixeMFQUNr1qoVOTAL16wRozRq2a6O5YpC4rSGzGqpTrJzeWBq/6+oz+x b34pVEmEx1hDfjpypL9SJ+9JFAA9pM/j+pg1Zu1/fGd6SAE4wySaHs4tu04DwNZPH11P a0/w== X-Gm-Message-State: ACgBeo2JkbkjthZo0zWH9CeLLR3t/uTcFZ/s7+zL2QGRVVLXnMPVFNH3 vbZb9Y1J1fPoT0XrZV9Qu9Caqxnv3T5QtyfjoBTNpw== X-Google-Smtp-Source: AA6agR6QaUo10P19dxvguTyuoa9mDJckjHXugslUVpC9M6HSm0yPoWq7R9h7E4SQCdQi5j2zijei+B9dkZd6x12EhMc= X-Received: by 2002:a05:6870:5821:b0:11c:cafa:eb64 with SMTP id r33-20020a056870582100b0011ccafaeb64mr3172531oap.64.1661999275892; Wed, 31 Aug 2022 19:27:55 -0700 (PDT) MIME-Version: 1.0 References: <20220830115317.410812-1-sr@denx.de> <282eefe1-58d4-89cd-f940-2428bdd42b59@denx.de> In-Reply-To: From: Simon Glass Date: Wed, 31 Aug 2022 20:27:43 -0600 Message-ID: Subject: Re: [PATCH 0/6] Enable CONFIG_TIMER for all Kirwood / MVEBU boards To: Tony Dinh Cc: Stefan Roese , U-Boot Mailing List , =?UTF-8?Q?Pali_Roh=C3=A1r?= , Michael Walle Content-Type: text/plain; charset="UTF-8" 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.6 at phobos.denx.de X-Virus-Status: Clean Hi Tony, On Wed, 31 Aug 2022 at 19:39, Tony Dinh wrote: > > Hi Stefan, > > On Wed, Aug 31, 2022 at 2:53 PM Tony Dinh wrote: > > > > Hi Stefan, > > > > On Wed, Aug 31, 2022 at 8:08 AM Tony Dinh wrote: > > > > > > Hi Stefan, > > > > > > On Wed, Aug 31, 2022 at 12:22 AM Stefan Roese wrote: > > > > > > > > Hi Tony, > > > > > > > > On 31.08.22 08:30, Tony Dinh wrote: > > > > > Hi Stefan, > > > > > > > > > > On Tue, Aug 30, 2022 at 10:08 PM Stefan Roese wrote: > > > > >> > > > > >> Hi Tony, > > > > >> > > > > >> On 31.08.22 07:02, Stefan Roese wrote: > > > > >>> Hi Tony, > > > > >>> > > > > >>> On 31.08.22 00:15, Tony Dinh wrote: > > > > >>>> Hi Stefan, > > > > >>>> > > > > >>>> On Tue, Aug 30, 2022 at 4:53 AM Stefan Roese wrote: > > > > >>>>> > > > > >>>>> This patchset enhaces the recently added Orion Timer driver to support > > > > >>>>> all other Kirkwood & 32bit MVEBU Armada platforms. Additionally, this > > > > >>>>> timer support is then enabled per default for those platforms, so that > > > > >>>>> the board config files don't need to be changed. Also necessary is > > > > >>>>> some dts hacking, so that the timer DT node is available in early > > > > >>>>> U-Boot stages. > > > > >>>>> > > > > >>>>> I've successfully tested this patchset on an Armada XP board. Additional > > > > >>>>> test on other boards and platforms are very welcome and necessary. > > > > >>>> > > > > >>>> I've run some tests with the following 2 Kirkwood boards: Cloud > > > > >>>> Engines Pogo V4 88F6192 (with CONFIG_DM_RTC and CONFIG_RTC_EMULATION), > > > > >>>> and Marvell Sheevaplug 88F6281 (with CONFIG_DM_RTC and CONFIG_RTC_MV). > > > > >>>> > > > > >>>> It seems that it was either frozen or the timer did not expire at some > > > > >>>> subsequent sleep commands. Sometime it happened at 2nd command, some > > > > >>>> time at a later sleep command. For example, > > > > >>>> > > > > >>>> === Pogo V4 (the 1st sleep command works correctly at 10 seconds on my > > > > >>>> stopwatch) > > > > >>>> > > > > >>>> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 13:38:24 -0700) > > > > >>>> Pogoplug V4 > > > > >>>> > > > > >>>> Hit any key to stop autoboot: 0 > > > > >>>> Pogo_V4> sleep 10 > > > > >>>> Pogo_V4> sleep 31.5 > > > > >>>> > > > > >>> > > > > >>> Does the cmd interface support fractial numbers? Please test again with > > > > >>> 31 or other integral numbers. > > > > >>> > > > > >>>> === Sheevaplug (RTC battery is old, so the date was not updated, but > > > > >>>> the clock seems OK) > > > > >>>> > > > > >>>> U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 30 2022 - 14:14:24 -0700) > > > > >>>> Marvell-Sheevaplug > > > > >>>> Hit any key to stop autoboot: 0 > > > > >>>> => date > > > > >>>> Date: 2000-01-01 (Saturday) Time: 0:02:55 > > > > >>>> => sleep 10 > > > > >>>> => date > > > > >>>> Date: 2000-01-01 (Saturday) Time: 0:03:18 > > > > >>>> => sleep 10 > > > > >>>> => sleep 20.1 > > > > >>>> > > > > >>>> > > > > >>>> Please let me know what I can do (i.e. perhaps running a debug patch). > > > > >>> > > > > >>> Please see above. I assume that the fractional numbers result in very > > > > >>> long numbers internally, which result in a frozen / hanging system. > > > > >> > > > > >> I just tested fractional numbers on another board and hey, it just > > > > >> works. Learned something new. So we seem to have a problem here. Let > > > > >> me see, if I can find something. > > > > > > > > > > I've added debug printfs and possibly tracked down this issue. Seems > > > > > like in the 2nd call to sleep, get_timer(0) did not reset the start > > > > > number. > > > > > > > > > > cmd/sleep.c > > > > > static int do_sleep(struct cmd_tbl *cmdtp, int flag, int argc, > > > > > char *const argv[]) > > > > > { > > > > > ulong start = get_timer(0); > > > > > > > > > > "do_sleep got a timer start = 2015" is the 1st call to sleep 5. > > > > > "do_sleep got a timer start = 16304" is the 2nd call to sleep 10. > > > > > > > > > > > > > > > Pogo_V4> sleep 5 > > > > > do_sleep got a timer start = 2015 > > > > > do_sleep delay = 5000 > > > > > do_sleep delay = 5000 > > > > > do_sleep sleeping... > > > > > do_sleep start 2015 curent 100 > > > > > do_sleep start 2015 curent 200 > > > > > do_sleep start 2015 curent 300 > > > > > > > > > > do_sleep start 2015 curent 4900 > > > > > do_sleep end of sleep ... current = 5000 > > > > > Pogo_V4> > > > > > > > > > > Pogo_V4> sleep 10 > > > > > do_sleep got a timer start = 16304 > > > > > do_sleep delay = 10000 > > > > > do_sleep delay = 10000 > > > > > do_sleep sleeping... > > > > > > > > > > > > > > > > > > > > > > > > > So somewhere in the DM timer, "start" got accumulated. I think each > > > > > get_timer(0) should be a different timer instance. It looks like the > > > > > same timer instance is used again and again, causing the "start "to > > > > > grow bigger, and at one point it might just overflow. > > > > > > > > Frankly I don't really understand the problem you describe above. What > > > > do you mean with "timer instance"? get_timer(0) will return different > > > > values, depending on when you call this function. So where exactly is > > > > the problem with the 2nd "sleep 10" above? > > > > > > Please ignore what I said above! I misunderstood what get_timer() > > > does. I'll try again on another KIrkwood board to see if the behavior > > > will be the same. > > > > I've run the tests again with the Seagate GoFlex Home board (Kirkwood > > 88F6281). Below is the log, with my annotated comments starting with > > ****. > > In these tests, current = get_timer(start) is inside the while loop. > > And I printed out the start and current values when (current % 100 == > > 0). > > > > > > U-Boot 2022.10-rc3-00048-g66ccd87a9c-dirty (Aug 31 2022 - 13:06:04 -0700) > > Seagate GoFlex Home > > > > SoC: Kirkwood 88F6281_A1 > > DRAM: 128 MiB > > Core: 12 devices, 10 uclasses, devicetree: separate > > NAND: orion_timer_probe successful > > 256 MiB > > Loading Environment from NAND... OK > > In: serial > > Out: serial > > Err: serial > > Net: eth0: ethernet-controller@72000 > > Hit any key to stop autoboot: 0 > > > > GoFlexHome> date > > Date: 2022-08-31 (Wednesday) Time: 20:43:03 > > > > GoFlexHome> sleep 5 > > do_sleep got a timer start = 1244 > > do_sleep delay = 5000 > > do_sleep delay = 5000 > > do_sleep sleeping... > > do_sleep start 1244 current 100 > > > > do_sleep start 1244 current 2300 > > > > do_sleep start 1244 current 3700 > > > > do_sleep start 1244 current 4800 > > do_sleep start 1244 current 4900 > > do_sleep end of sleep ... current = 5000 > > > > **** working as intended > > > > GoFlexHome> sleep 10 > > do_sleep got a timer start = 11428 > > do_sleep delay = 10000 > > do_sleep delay = 10000 > > do_sleep sleeping... > > do_sleep start 11428 current 100 > > > > do_sleep start 11428 current 2300 > > > > do_sleep start 11428 current 9900 > > do_sleep end of sleep ... current = 10000 > > > > **** working as intended > > > > GoFlexHome> sleep 20.5 > > do_sleep got a timer start = 15031 > > do_sleep delay = 20000 > > do_sleep delay = 20500 > > do_sleep sleeping... > > do_sleep start 15031 current 100 > > > > do_sleep start 15031 current 6400 > > do_sleep end of sleep ... current = 4294952265 > > > > *** Something strange happened here. current should be 6500, but it > > seems to have garbage. So the loop exits prematurely. > > > > GoFlexHome> sleep 20.5 > > do_sleep got a timer start = 8339 > > do_sleep delay = 20000 > > do_sleep delay = 20500 > > do_sleep sleeping... > > do_sleep start 8339 current 100 > > do_sleep start 8339 current 200 > > > > do_sleep start 8339 current 12300 > > do_sleep start 8339 current 12400 > > do_sleep start 8339 current 12500 > > do_sleep start 8339 current 12600 > > do_sleep start 8339 current 12700 > > do_sleep start 8339 current 12800 > > do_sleep start 8339 current 12900 > > do_sleep start 8339 current 13000 > > do_sleep start 8339 current 13100 > > > > *** It was frozen right here. Control-C could not interrupt the loop, > > so my guess > > *** either it was stuck during the call to get_timer(). Or the current value > > *** overflowed and crashed the system. > > > > > > > > Finally, I removed the patch set, and rerun the sleep tests on this > > board. They all worked fine, I did not see anything strange. > > > > Some ideas. > > The get_timer() function looks wrong assigning an uint64_t to ulong. > > lib/time.c > > static uint64_t notrace tick_to_time(uint64_t tick) > uint64_t notrace get_ticks(void) > uint64_t __weak notrace get_ticks(void) > > ulong __weak get_timer(ulong base) > { > return tick_to_time(get_ticks()) - base; > } > > Most of the timer infrastructure is using uint64_t. I'm seeing this > __weak function get_timer was invoked in Kirkwood boards. Both in > sleep and timer commands. The get_ticks() thing can run at 1MHz but the timer is 1KHz, so that is why we don't need a u64 for the timer. Regards, Simon