From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x22b.google.com (mail-wm0-x22b.google.com [IPv6:2a00:1450:400c:c09::22b]) (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 020FB20958BE9 for ; Fri, 1 Sep 2017 15:01:15 -0700 (PDT) Received: by mail-wm0-x22b.google.com with SMTP id 137so3239310wmj.1 for ; Fri, 01 Sep 2017 15:03:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=SI2GVPCBegOwJ/n+fL/MKg+EcmyHJbk+3QyMl/hj3ws=; b=OJoap9laNg7OrDn5wg9ul48jForESh3GZJ7vFH3gHbVVre1C79R2zgz2Bcq1sPMklr PPKH3wyArOYrOhWLRuvYxNCaBiVnAZ4CZXApq+QhQ0WSfEz72Fbmx2UEjxQRc1f+3H1J kfsUPgpYhzeQWMHI8CAuHmEP7su+93ThNuPUc= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=SI2GVPCBegOwJ/n+fL/MKg+EcmyHJbk+3QyMl/hj3ws=; b=Z8X10V3F2tjH9ML+gupBD7ae/ccTzWFl82If0AFU4JNLzVyMrMgHzJ2ddyn6/8YjAR K72xxEhY2cOv3XqWPrXYIXo06SbeH0qyEU9BPkB3lEzOQx1xUoqzevx9n5S4CfDeTBzF OFc0AV4SeH+O52Mer2rjqRWEW+Otl1/ia88aa/OVlp7NmN5l2cqIS4A+ABN4YPpIcr29 CUekZbhwjHubFKdJN8tYieSYeqn6os+fA/BMmom5XgMr1r/ws6ZwEX3ijesnGZKHLZ8/ ggNO9DYj2yGTYBF6wkPuyJtFqokbszJDtMBHLp386UQ6A1szrUQ3uZAU4F1uH5r0GKR5 CsDg== X-Gm-Message-State: AHPjjUg4HYXFL4oIbcVOBIvCQQ6RyNTyiHRCKiF0Qjus4TjhyGKLZ6hk 56WVsBFgIC757HRN X-Google-Smtp-Source: ADKCNb6mHQASx62Im7FWm3Uz1sqkEBWz13Ocy80xwmOqUVuHqnLE7hxwflVSBXFvJLzEHQjtzE8Rhw== X-Received: by 10.28.155.11 with SMTP id d11mr1177534wme.4.1504303438522; Fri, 01 Sep 2017 15:03:58 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id j15sm1439135wmg.15.2017.09.01.15.03.56 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 01 Sep 2017 15:03:57 -0700 (PDT) Date: Fri, 1 Sep 2017 23:03:54 +0100 From: Leif Lindholm To: Marcin Wojtas Cc: edk2-devel-01 , Ard Biesheuvel , nadavh@marvell.com, Neta Zur Hershkovits , Kostya Porotchkin , Hua Jing , Alexander Graf , semihalf-dabros-jan Message-ID: <20170901220354.t2gzpfujchnoixtn@bivouac.eciton.net> References: <1504271303-1782-1-git-send-email-mw@semihalf.com> <1504271303-1782-11-git-send-email-mw@semihalf.com> <20170901153325.4lladsqfaomt5jcf@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [platforms: PATCH 10/11] Drivers/Spi/Devices/MvSpiFlash: Enable dynamic SPI Flash detection X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 01 Sep 2017 22:01:15 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri, Sep 01, 2017 at 07:20:06PM +0200, Marcin Wojtas wrote: > 2017-09-01 17:33 GMT+02:00 Leif Lindholm : > > On Fri, Sep 01, 2017 at 03:08:22PM +0200, Marcin Wojtas wrote: > >> Hitherto mechanism of fixing SPI flash model in the PCDs, > >> occured to be very inefficient and problematic. Enable > >> dynamic detection by reworking MvSpiFlashReadId() command, > >> which now reads the Id and goes through newly added table > >> with JEDEC compliant devices and their description. > >> > >> On the occasion fix the ReadId process by using master's > >> ReadWrite routine instead of pure Transfer - no longer > >> swapping and byte shifting is needed. Simplify code by > >> using local array instead of dynamic allocation. Also > >> reduced number of ReadId arguments allowed for cleaning > >> Fupdate and Sf shell commands probe routines. > >> > >> Additionally, use SpiFlashInfo fields instead of PCDs, > >> and configure needed settings in MvSpiFlashInit. > >> > >> Update PortingGuide documentation accordingly. > >> > >> Contributed-under: TianoCore Contribution Agreement 1.1 > >> Signed-off-by: Marcin Wojtas > > > > So, this looks _really_ good, but I'm seeing three separate patches in > > here really: > > - Style fixups (yay). I love them, but squashing them in make the > > functional changes impossible to spot in the noise. > > - SPI Flash Id table. Love it! > > But this is a completely generic feature. Medium-term, this should > > be living in EDK2. For now, can you break it out into a separate > > library that can be used by other platforms (and will be easier to > > move)? > > - Functional improvements to existing driver - great! (but can't > > really review before the above two have been broken out). > > > > I'll check and do the style fixup/functional improvement split. Thanks! > About flash ids in a separate library - we already have > SpiFlashProtocol->ReadId callback. In fact despite Mv prefix the > MvSpiFlash.c driver and its associated SPI_FLASH_PROTOCOL are > completely generic for now and SoC controller agnostic. It works in > very similar way as U-Boot driver (drivers/mtd/spi) and Linux one > (drivers/mtd/spi-nor/spi-nor.c). Due to lack of similar solution, how > about moving it to EDK2? Do you think there's chance to promote such > solution? Oh, certainly. The only question would be whether you would be willing/able to make that happen for this series. Having the identification portion as a separate library may not be a bad idea regardless ... and it's a decent halfway house. / Leif