From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c0c::241; helo=mail-wr0-x241.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wr0-x241.google.com (mail-wr0-x241.google.com [IPv6:2a00:1450:400c:c0c::241]) (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 BB742210F16CA for ; Tue, 12 Jun 2018 15:41:08 -0700 (PDT) Received: by mail-wr0-x241.google.com with SMTP id k16-v6so685986wro.0 for ; Tue, 12 Jun 2018 15:41:08 -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=7Y74ju++Ug5j+2GJHUJSGFuOiLPe7ABp5/fr0Jf1NkU=; b=SFEN7e1v5C4NtsaMP/u0+xbtO1sGUV77cc8GzcfsmIudTZslOp5/cxCJKhfx8SXQUe y155Ur6RSA/fihIljiMhyrQTLf14tNKfstVhuaWhJCaf0fUWTRUJFLZUFHpZc/zNNSzA 8NgghtW11k5VIfb8f0Qo+5oLvrmeD6WSVKH4c= 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=7Y74ju++Ug5j+2GJHUJSGFuOiLPe7ABp5/fr0Jf1NkU=; b=U8evpL43BiPX7xW6WU0hPuKR5MP2CEzDsLX1BVGXWv/xm18MsTSes33ROV7+FCndWM eYygXxhXoBfvaP4z4UDYsCHtBoqBiHeGU/wN5yIXjd48ELeqd7XhtjwHYOkvP+hWv4C+ RJQpMUUgNq171vDcaVsSfUbEvhbPlvyYsPLLt6cpHUivIFfqOyqOlXyoAFQElZqnWTK5 HIIb9BlzT0rR9qcYaB2yVAhgfDAr4Rn7JeHu4BD+HAUKhqtFRyizL4no/zdMfYs8kmua ebovujO949w1D1dJLejd+5+28C9DvCion8vK94Lqs18/wYWzwuuhKL9CZidq0oGh8Obi 7RDQ== X-Gm-Message-State: APt69E2rUC/8mf+gieyqQ1Qsu7Cea3B4kwaVmjnxxKEKhury1t4n9fjr /0Ww/FngLxr/9iZG6YI+xgmkVXa8X78= X-Google-Smtp-Source: ADUXVKKg5uqt6bFX4gdkmb94eGVB4hAOzahVqx5Wwsi9Ke2DNAvkOtxaTYTECu+DOj2MI9mKydx4TQ== X-Received: by 2002:adf:dd8c:: with SMTP id x12-v6mr2290196wrl.212.1528843266825; Tue, 12 Jun 2018 15:41:06 -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 84-v6sm1883420wme.16.2018.06.12.15.41.05 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 12 Jun 2018 15:41:05 -0700 (PDT) Date: Tue, 12 Jun 2018 23:41:04 +0100 From: Leif Lindholm To: Marcin Wojtas Cc: edk2-devel@lists.01.org, ard.biesheuvel@linaro.org, nadavh@marvell.com, jinghua@marvell.com, jsd@semihalf.com, jaz@semihalf.com Message-ID: <20180612224104.mycg7dfxc6qkgsbj@bivouac.eciton.net> References: <1528472063-1660-1-git-send-email-mw@semihalf.com> <1528472063-1660-24-git-send-email-mw@semihalf.com> MIME-Version: 1.0 In-Reply-To: <1528472063-1660-24-git-send-email-mw@semihalf.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [platforms PATCH 23/25] Marvell/Drivers: MvBoardDesc: Extend protocol with I2C support X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 12 Jun 2018 22:41:09 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri, Jun 08, 2018 at 05:34:21PM +0200, Marcin Wojtas wrote: > Introduce new callback that can provide information > about I2C controllers to the I2c driver. > > Extend ArmadaBoardDescLib with new structure MV_BOARD_I2C_DESC, > for holding board specific data. In further steps it should > be extended and replace PCD I2C devices' representation with the > appropriate structures. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Marcin Wojtas > Reviewed-by: Hua Jing > --- > Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c | 62 ++++++++++++++++++++ > Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.inf | 1 + > Silicon/Marvell/Include/Library/ArmadaBoardDescLib.h | 11 ++++ > Silicon/Marvell/Include/Protocol/BoardDesc.h | 8 +++ > 4 files changed, 82 insertions(+) > > diff --git a/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c > index 8f3bdfa..a133085 100644 > --- a/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c > +++ b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.c > @@ -96,6 +96,67 @@ MvBoardDescComPhyGet ( > > STATIC > EFI_STATUS > +MvBoardDescI2cGet ( > + IN MARVELL_BOARD_DESC_PROTOCOL *This, > + IN OUT MV_BOARD_I2C_DESC **I2cDesc > + ) > +{ > + UINT8 *I2cDeviceTable, I2cCount; UINTN. Having gone through this set, this pattern is becoming clearer to me. But ideally, it would have been obvious from the first instance. Should not all of these "Tables" be treated simply as enable flags? Because that's all they seem to be doing. If you rename this one I2cDeviceEnabled ... > + UINTN I2cDeviceTableSize, I2cIndex, Index; > + MV_BOARD_I2C_DESC *BoardDesc; > + MV_SOC_I2C_DESC *SoCDesc; > + EFI_STATUS Status; > + > + /* Get SoC data about all available I2C controllers */ > + Status = ArmadaSoCDescI2cGet (&SoCDesc, &I2cCount); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + /* Obtain table with enabled I2C controllers */ > + I2cDeviceTable = (UINT8 *)PcdGetPtr (PcdI2cControllersEnabled); > + if (I2cDeviceTable == NULL) { > + /* No I2C on platform */ > + return EFI_SUCCESS; > + } > + > + I2cDeviceTableSize = PcdGetSize (PcdI2cControllersEnabled); > + > + /* Check if PCD with I2C controllers is correctly defined */ > + if (I2cDeviceTableSize > I2cCount) { > + DEBUG ((DEBUG_ERROR, > + "%a: Wrong PcdI2cControllersEnabled format\n", > + __FUNCTION__)); > + return EFI_INVALID_PARAMETER; > + } > + > + /* Allocate and fill board description */ > + BoardDesc = AllocateZeroPool (I2cDeviceTableSize * sizeof (MV_BOARD_I2C_DESC)); > + if (BoardDesc == NULL) { > + DEBUG ((DEBUG_ERROR, "%a: Cannot allocate memory\n", __FUNCTION__)); > + return EFI_OUT_OF_RESOURCES; > + } > + > + I2cIndex = 0; > + for (Index = 0; Index < I2cDeviceTableSize; Index++) { > + if (!MVHW_DEV_ENABLED (I2c, Index)) { ... then this one becomes simply if (!I2cDeviceEnabled[Index]) { Please consider making this change throughout the set. > + DEBUG ((DEBUG_INFO, "%a: Skip I2c controller %d\n", __FUNCTION__, Index)); > + continue; > + } > + > + BoardDesc[I2cIndex].SoC = &SoCDesc[Index]; > + I2cIndex++; > + } > + > + BoardDesc->I2cDevCount = I2cIndex; > + > + *I2cDesc = BoardDesc; > + > + return EFI_SUCCESS; > +} > + > +STATIC > +EFI_STATUS > MvBoardDescMdioGet ( > IN MARVELL_BOARD_DESC_PROTOCOL *This, > IN OUT MV_BOARD_MDIO_DESC **MdioDesc > @@ -470,6 +531,7 @@ MvBoardDescInitProtocol ( > ) > { > BoardDescProtocol->BoardDescComPhyGet = MvBoardDescComPhyGet; > + BoardDescProtocol->BoardDescI2cGet = MvBoardDescI2cGet; > BoardDescProtocol->BoardDescMdioGet = MvBoardDescMdioGet; > BoardDescProtocol->BoardDescAhciGet = MvBoardDescAhciGet; > BoardDescProtocol->BoardDescSdMmcGet = MvBoardDescSdMmcGet; > diff --git a/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.inf b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.inf > index 71b7ebd..cc93eba 100644 > --- a/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.inf > +++ b/Silicon/Marvell/Drivers/BoardDesc/MvBoardDescDxe.inf > @@ -58,6 +58,7 @@ > > [Pcd] > gMarvellTokenSpaceGuid.PcdComPhyDevices > + gMarvellTokenSpaceGuid.PcdI2cControllersEnabled Alphabetically ... > gMarvellTokenSpaceGuid.PcdPp2Controllers > gMarvellTokenSpaceGuid.PcdUtmiControllersEnabled > gMarvellTokenSpaceGuid.PcdUtmiPortType > diff --git a/Silicon/Marvell/Include/Library/ArmadaBoardDescLib.h b/Silicon/Marvell/Include/Library/ArmadaBoardDescLib.h > index 5379679..74361d4 100644 > --- a/Silicon/Marvell/Include/Library/ArmadaBoardDescLib.h > +++ b/Silicon/Marvell/Include/Library/ArmadaBoardDescLib.h > @@ -28,6 +28,17 @@ typedef struct { > } MV_BOARD_COMPHY_DESC; > > // > +// I2C devices per-board description > +// > +// TODO - Extend structure with entire > +// ports description instead of PCDs. Please drop TODO. > +// > +typedef struct { > + MV_SOC_I2C_DESC *SoC; > + UINT8 I2cDevCount; UINTN. (To clarify: this is because the pointer above already forces struct alignment to the same as UINTN. The only point at which any space could be saved would be if something with less alignment than UINTN was placed immediately after it, and even then it would increase code size for accessing it.) / Leif > +} MV_BOARD_I2C_DESC; > + > +// > // MDIO devices per-board description > // > typedef struct { > diff --git a/Silicon/Marvell/Include/Protocol/BoardDesc.h b/Silicon/Marvell/Include/Protocol/BoardDesc.h > index cff802a..0b73d27 100644 > --- a/Silicon/Marvell/Include/Protocol/BoardDesc.h > +++ b/Silicon/Marvell/Include/Protocol/BoardDesc.h > @@ -50,6 +50,13 @@ EFI_STATUS > > typedef > EFI_STATUS > +(EFIAPI *MV_BOARD_DESC_I2C_GET) ( > + IN MARVELL_BOARD_DESC_PROTOCOL *This, > + IN OUT MV_BOARD_I2C_DESC **I2cDesc > + ); > + > +typedef > +EFI_STATUS > (EFIAPI *MV_BOARD_DESC_MDIO_GET) ( > IN MARVELL_BOARD_DESC_PROTOCOL *This, > IN OUT MV_BOARD_MDIO_DESC **MdioDesc > @@ -98,6 +105,7 @@ VOID > > struct _MARVELL_BOARD_DESC_PROTOCOL { > MV_BOARD_DESC_COMPHY_GET BoardDescComPhyGet; > + MV_BOARD_DESC_I2C_GET BoardDescI2cGet; > MV_BOARD_DESC_MDIO_GET BoardDescMdioGet; > MV_BOARD_DESC_AHCI_GET BoardDescAhciGet; > MV_BOARD_DESC_SDMMC_GET BoardDescSdMmcGet; > -- > 2.7.4 >