From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x22d.google.com (mail-io0-x22d.google.com [IPv6:2607:f8b0:4001:c06::22d]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 56C5E81E94 for ; Thu, 19 Jan 2017 10:16:02 -0800 (PST) Received: by mail-io0-x22d.google.com with SMTP id j13so44116910iod.3 for ; Thu, 19 Jan 2017 10:16:02 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=P9TQJvEYQF3dWdm8eiS00DyjFqMWIu1cQcfoJ3xLSIw=; b=Bot3whnLcyg8f9YtbY4ol/cwlBgrZU9hS8HEFiNjVNw4h6DxwTVbYuCdFO7Q5nXrJa BJC3fR7jmIiLBAPnpfmyjCNxHwh4PaCPKDNClLDE7nMoJm/ATP+ICMWqnc3XyCfgCvAc 5nHmH+bClbCV5hyorS9K4Ixps1QaJIxtjPQRU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=P9TQJvEYQF3dWdm8eiS00DyjFqMWIu1cQcfoJ3xLSIw=; b=iSonDHX9NC01r3/Mhmcv/6VjNsTRKh2KwA3T1NLmAKrXeQG6LXI8HwngsqxnerIm1Y 1ENpwkfWQKRXCZHVg0V9icbj3T+jXCMv9DZWYQzU+CZQwS3KwjvaL47kdG2DWohdEm7h /DRzE1DkM+Mf2JWYRIYtaxzKk0Ey6QaUCc31+mTz9uU9pmR+/teoPkzYzWdo2Kcl1kYG a/IETYisV8v+OrGuQZs6wqYZ6upcEnn7Jfs4R8xbrmHdQcLI8nbcadT9LBBqYK38lpwU wUJuPWGq+S8x3Llc4eCo7yTLyStwKwwFmu9wBPNscbWYkN2ix/SMJyicmf2gtD9VwEBh ykFA== X-Gm-Message-State: AIkVDXICwzFmqTx3xWLnTbjJGTC0wZSG0Gzgxx3NexboCsebXSMvwOOUUmdEj65D1V8jZC7fxtkyBYHSDLURce3N X-Received: by 10.107.32.207 with SMTP id g198mr8955946iog.83.1484849761482; Thu, 19 Jan 2017 10:16:01 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.144.135 with HTTP; Thu, 19 Jan 2017 10:16:00 -0800 (PST) In-Reply-To: <20170119123135.GB24076@e102648.cambridge.arm.com> References: <1484771046-21296-1-git-send-email-achin.gupta@arm.com> <20170118220500.GW25883@bivouac.eciton.net> <20170119123135.GB24076@e102648.cambridge.arm.com> From: Ard Biesheuvel Date: Thu, 19 Jan 2017 18:16:00 +0000 Message-ID: To: Achin Gupta Cc: Leif Lindholm , "edk2-devel@lists.01.org" , nd@arm.com, Supreeth Venkatesh Subject: Re: [PATCH] ArmPlatformPkg/ArmVExpressPkg: Fix memory attributes for NOR Flash X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 Jan 2017 18:16:02 -0000 Content-Type: text/plain; charset=UTF-8 On 19 January 2017 at 12:31, Achin Gupta wrote: > Hi Leif/Ard, > > On Wed, Jan 18, 2017 at 10:05:00PM +0000, Leif Lindholm wrote: >> Hi Achin, >> >> On Wed, Jan 18, 2017 at 08:24:06PM +0000, achin.gupta@arm.com wrote: >> > From: Achin Gupta >> > >> > The NOR flash banks were being mapped in the translation tables with the same >> > memory attributes as RAM in the system. These attributes mark the region as >> > Normal Memory and could additionally be cacheable or non-cacheable. >> > >> > Either type of attributes are unsuitable for NOR Flash since write operations >> > could be performed on it. Normal Memory does not guarantee ordering of >> > transactions that Device memory does. So the commands sent to the Flash device >> > may not arrive in the right order unless barriers are used. Commands might not >> > get past the CPU caches in case the region has been mapped with cacheable >> > attributes. >> > >> > This patch fixes the problem by mapping the NOR Flash memory region with Device >> > memory attributes. >> >> To add some background to Ard's comment - this was not unintentionally >> done: >> https://github.com/tianocore/edk2/commit/19bb46c411279dcd30d540c56e5993c5f771c319 > > Thanks! I missed this commit. There is some background to the problem I am > facing below. > >> >> Was the reasoning behind this commit incorrect - do you have a >> (pre-DXE?) use-case that creates a problem? > > AFAIU, The current memory attributes for NOR Flash work only for reads. They > should additionally be RO to flag any unexpected writes. > > Mine is a DXE use case! In NorFlashDxe.c, commands are send to the Flash (erase > block etc.). They might never reach the device if there is a write-back cache in > between. So either device or Normal memory with non-cacheable/write-through > attributes and barriers should work. > > If I turn on cache state modelling in the Base FVP, the code gets stuck in > NorFlashReadStatusRegister() in the below loop in NorFlashEraseSingleBlock(): > > // Wait until the status register gives us the all clear > do { > StatusRegister = NorFlashReadStatusRegister (Instance, BlockAddress); > } while ((StatusRegister & P30_SR_BIT_WRITE) != P30_SR_BIT_WRITE); > > I think the SEND_NOR_COMMANDs at the beginning of the function get stuck in the > cache. Changing the flash memory attributes as per this patch solves the > problem. > > The original patch from Ard mentions that the NOR Flash DXE driver should change > the attributes of the region it wants to write to. Is this what is missing? > NorFlashFvbInitialize() in ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c contains the following calls: Status = gDS->AddMemorySpace ( EfiGcdMemoryTypeMemoryMappedIo, Instance->DeviceBaseAddress, RuntimeMmioRegionSize, EFI_MEMORY_UC | EFI_MEMORY_RUNTIME ); ASSERT_EFI_ERROR (Status); Status = gDS->SetMemorySpaceAttributes ( Instance->DeviceBaseAddress, RuntimeMmioRegionSize, EFI_MEMORY_UC | EFI_MEMORY_RUNTIME); ASSERT_EFI_ERROR (Status); which is supposed to set device attributes on the NOR region that is actually exposed to the upper layers as read-write capable. Perhaps you can enable GCD debugging to trace these calls, to ensure that the address you are stalled on is actually covered?