From patchwork Fri May 13 03:19:31 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chris Packham X-Patchwork-Id: 621828 X-Patchwork-Delegate: hs@denx.de Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from theia.denx.de (theia.denx.de [85.214.87.163]) by ozlabs.org (Postfix) with ESMTP id 3r5Zq34Pfmz9sBX for ; Fri, 13 May 2016 13:19:58 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b=hCh4sVeK; dkim-atps=neutral Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 07AFEA75BF; Fri, 13 May 2016 05:19:55 +0200 (CEST) Received: from theia.denx.de ([127.0.0.1]) by localhost (theia.denx.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id oRhA-p68tQZD; Fri, 13 May 2016 05:19:54 +0200 (CEST) Received: from theia.denx.de (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 74335A750A; Fri, 13 May 2016 05:19:54 +0200 (CEST) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id C1FB1A750A for ; Fri, 13 May 2016 05:19:50 +0200 (CEST) Received: from theia.denx.de ([127.0.0.1]) by localhost (theia.denx.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id uxvK9XTuS50W for ; Fri, 13 May 2016 05:19:50 +0200 (CEST) X-policyd-weight: NOT_IN_SBL_XBL_SPAMHAUS=-1.5 NOT_IN_SPAMCOP=-1.5 NOT_IN_BL_NJABL=-1.5 (only DNSBL check requested) Received: from mail-pf0-f195.google.com (mail-pf0-f195.google.com [209.85.192.195]) by theia.denx.de (Postfix) with ESMTPS id 2A075A74DB for ; Fri, 13 May 2016 05:19:46 +0200 (CEST) Received: by mail-pf0-f195.google.com with SMTP id b66so204860pfb.2 for ; Thu, 12 May 2016 20:19:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=BCaoYDpBMIsdx+QZQHSeNPZQeftnWMTKfznXj3CwhTs=; b=hCh4sVeKkSV1bDEZ1yPN5PZCB32L7EI3AtwjhsGFZ0eV8n7n4FjEZLgDImMLgRFygb 36jgann0ldfvoNsb/FCsCNJqhUlLaKMp056X+dirws+idTeJbsD1ETPe9aAM0VKEW5hF b69bivGAzi8hphKizvTGl2LMUT6Cojv5IDujIu0EMJtekZAJPLLyA/iyhwAeB8yXxxWT TQUiBnedvr9gXlHE64Ziw+b24tShTFGPCFFxof+oi3TY5tRlq7/9rDE4zjJ+Wh7Q24f6 4iAhbDaajgxNI49mHhoxJh+vItkwkdLMJ2C5SPhRDw3uRspqir5UONKc6Y6IxCG1IGjs DS2g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=BCaoYDpBMIsdx+QZQHSeNPZQeftnWMTKfznXj3CwhTs=; b=OW9sGFoD2FPDWB1yhMOtCb2C5n4IHvsvJkeItL0h9WQzkQzlr/iE/lfdZ+YGuQyIpS 3er6iEieF8VFHwetoglfBrZpF8ffdHia5MYroBWeiNuFPBOuKwHpyqGlRqicIcaQiJRr Oo1aiFDjvpBTxEyrdE1uKxRWrYFYWd1OKubwpGJ1bqieXUT8wPGwK/wDajaKjG7ISA1H 1fsFZDqIjf3rSeDF+c2XnmfqrXcNfy7UztNNxEoEpKUj1nOaLCCVnceVIobmToawoYnC Oz8XhG5VtXO1A0PoWx+9251W3Tl/PWLLdIFnXDUktbthS0yIKOwjFiPGx+d2acrSZts0 GsGQ== X-Gm-Message-State: AOPr4FU8rKbSXIo4ttmZrOryqJGf4aWPKdD8/DJHNCq88kCD5SJFhvE2u80XrJXcdH0Bhw== X-Received: by 10.98.92.71 with SMTP id q68mr19383537pfb.138.1463109584280; Thu, 12 May 2016 20:19:44 -0700 (PDT) Received: from chrisp-dl.ws.atlnz.lc (2-163-36-202-static.alliedtelesis.co.nz. [202.36.163.2]) by smtp.gmail.com with ESMTPSA id r73sm23032819pfi.4.2016.05.12.20.19.39 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Thu, 12 May 2016 20:19:43 -0700 (PDT) From: Chris Packham To: u-boot@lists.denx.de Date: Fri, 13 May 2016 15:19:31 +1200 Message-Id: <1463109571-2101-1-git-send-email-judge.packham@gmail.com> X-Mailer: git-send-email 2.8.2 In-Reply-To: References: Cc: Albert Aribaud , Stefan Roese , Chris Packham Subject: [U-Boot] [PATCH v2] i2c: mvtwsi: Eliminate twsi_control_flags X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.15 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" In a system where the initial u-boot location is genuinely NOR flash (as opposed to RAM or a cache-line setup by a pre-bootloader) writes to the data section are problematic. At best these writes have no effect, at worst they put the flash memory into a status mode which changes the executable code underneath us. Pass around a stack variable from the top of the twsi i2c driver to avoid writing to global data. Signed-off-by: Chris Packham --- > On Thu, May 12, 2016 at 9:50 PM, Stefan Roese wrote: >> Hi Chris, >> >> >> On 12.05.2016 04:55, Chris Packham wrote: >>> struct mvtwsi_registers *twsi = twsi_get_base(adap); >>> /* ensure controller will be enabled by any twsi*() function >>> */ >>> - twsi_control_flags = MVTWSI_CONTROL_TWSIEN; >>> + if (gd->flags & GD_FLG_RELOC) >>> + twsi_control_flags = MVTWSI_CONTROL_TWSIEN; >>> /* reset controller */ >>> writel(0, &twsi->soft_reset); >>> /* wait 2 ms -- this is what the Marvell LSP does */ >> >> >> I've stumbled over this global data variable also before and would >> very much like to get rid of it. Can't you move this variable into >> a (newly created) private data struct instead? > > I'll take a look. The other deficiency with my solution is that > although it avoids the hang the driver still won't work because the > state that is reflected in twsi_control_flags will either cause a new > hang or not be updated. > So I think this should work (I have tested it on my platform but if someone else could give it ago on theirs that'd be helpful). By passing the flags around I avoid the global data variable problems. Changes in v2: - Rather than just avoiding writing to twsi_control_flags pass a variable on the stack drivers/i2c/mvtwsi.c | 62 ++++++++++++++++++++++++---------------------------- 1 file changed, 29 insertions(+), 33 deletions(-) diff --git a/drivers/i2c/mvtwsi.c b/drivers/i2c/mvtwsi.c index 221ff4f..bf44432 100644 --- a/drivers/i2c/mvtwsi.c +++ b/drivers/i2c/mvtwsi.c @@ -185,26 +185,17 @@ static int twsi_wait(struct i2c_adapter *adap, int expected_status) } /* - * These flags are ORed to any write to the control register - * They allow global setting of TWSIEN and ACK. - * By default none are set. - * twsi_start() sets TWSIEN (in case the controller was disabled) - * twsi_recv() sets ACK or resets it depending on expected status. - */ -static u8 twsi_control_flags = MVTWSI_CONTROL_TWSIEN; - -/* * Assert the START condition, either in a single I2C transaction * or inside back-to-back ones (repeated starts). */ -static int twsi_start(struct i2c_adapter *adap, int expected_status) +static int twsi_start(struct i2c_adapter *adap, int expected_status, u8 *flags) { struct mvtwsi_registers *twsi = twsi_get_base(adap); /* globally set TWSIEN in case it was not */ - twsi_control_flags |= MVTWSI_CONTROL_TWSIEN; + *flags |= MVTWSI_CONTROL_TWSIEN; /* assert START */ - writel(twsi_control_flags | MVTWSI_CONTROL_START | + writel(*flags | MVTWSI_CONTROL_START | MVTWSI_CONTROL_CLEAR_IFLG, &twsi->control); /* wait for controller to process START */ return twsi_wait(adap, expected_status); @@ -213,14 +204,15 @@ static int twsi_start(struct i2c_adapter *adap, int expected_status) /* * Send a byte (i2c address or data). */ -static int twsi_send(struct i2c_adapter *adap, u8 byte, int expected_status) +static int twsi_send(struct i2c_adapter *adap, u8 byte, int expected_status, + u8 *flags) { struct mvtwsi_registers *twsi = twsi_get_base(adap); /* put byte in data register for sending */ writel(byte, &twsi->data); /* clear any pending interrupt -- that'll cause sending */ - writel(twsi_control_flags | MVTWSI_CONTROL_CLEAR_IFLG, &twsi->control); + writel(*flags | MVTWSI_CONTROL_CLEAR_IFLG, &twsi->control); /* wait for controller to receive byte and check ACK */ return twsi_wait(adap, expected_status); } @@ -229,18 +221,18 @@ static int twsi_send(struct i2c_adapter *adap, u8 byte, int expected_status) * Receive a byte. * Global mvtwsi_control_flags variable says if we should ack or nak. */ -static int twsi_recv(struct i2c_adapter *adap, u8 *byte) +static int twsi_recv(struct i2c_adapter *adap, u8 *byte, u8 *flags) { struct mvtwsi_registers *twsi = twsi_get_base(adap); int expected_status, status; /* compute expected status based on ACK bit in global control flags */ - if (twsi_control_flags & MVTWSI_CONTROL_ACK) + if (*flags & MVTWSI_CONTROL_ACK) expected_status = MVTWSI_STATUS_DATA_R_ACK; else expected_status = MVTWSI_STATUS_DATA_R_NAK; /* acknowledge *previous state* and launch receive */ - writel(twsi_control_flags | MVTWSI_CONTROL_CLEAR_IFLG, &twsi->control); + writel(*flags | MVTWSI_CONTROL_CLEAR_IFLG, &twsi->control); /* wait for controller to receive byte and assert ACK or NAK */ status = twsi_wait(adap, expected_status); /* if we did receive expected byte then store it */ @@ -296,8 +288,7 @@ static unsigned int twsi_calc_freq(const int n, const int m) static void twsi_reset(struct i2c_adapter *adap) { struct mvtwsi_registers *twsi = twsi_get_base(adap); - /* ensure controller will be enabled by any twsi*() function */ - twsi_control_flags = MVTWSI_CONTROL_TWSIEN; + /* reset controller */ writel(0, &twsi->soft_reset); /* wait 2 ms -- this is what the Marvell LSP does */ @@ -353,7 +344,7 @@ static void twsi_i2c_init(struct i2c_adapter *adap, int speed, int slaveadd) * Expected address status will derive from direction bit (bit 0) in addr. */ static int i2c_begin(struct i2c_adapter *adap, int expected_start_status, - u8 addr) + u8 addr, u8 *flags) { int status, expected_addr_status; @@ -363,10 +354,11 @@ static int i2c_begin(struct i2c_adapter *adap, int expected_start_status, else /* writing */ expected_addr_status = MVTWSI_STATUS_ADDR_W_ACK; /* assert START */ - status = twsi_start(adap, expected_start_status); + status = twsi_start(adap, expected_start_status, flags); /* send out the address if the start went well */ if (status == 0) - status = twsi_send(adap, addr, expected_addr_status); + status = twsi_send(adap, addr, expected_addr_status, + flags); /* return ok or status of first failure to caller */ return status; } @@ -378,13 +370,14 @@ static int i2c_begin(struct i2c_adapter *adap, int expected_start_status, static int twsi_i2c_probe(struct i2c_adapter *adap, uchar chip) { u8 dummy_byte; + u8 flags = 0; int status; /* begin i2c read */ - status = i2c_begin(adap, MVTWSI_STATUS_START, (chip << 1) | 1); + status = i2c_begin(adap, MVTWSI_STATUS_START, (chip << 1) | 1, &flags); /* dummy read was accepted: receive byte but NAK it. */ if (status == 0) - status = twsi_recv(adap, &dummy_byte); + status = twsi_recv(adap, &dummy_byte, &flags); /* Stop transaction */ twsi_stop(adap, 0); /* return 0 or status of first failure */ @@ -405,27 +398,28 @@ static int twsi_i2c_read(struct i2c_adapter *adap, uchar chip, uint addr, int alen, uchar *data, int length) { int status; + u8 flags = 0; /* begin i2c write to send the address bytes */ - status = i2c_begin(adap, MVTWSI_STATUS_START, (chip << 1)); + status = i2c_begin(adap, MVTWSI_STATUS_START, (chip << 1), &flags); /* send addr bytes */ while ((status == 0) && alen--) status = twsi_send(adap, addr >> (8*alen), - MVTWSI_STATUS_DATA_W_ACK); + MVTWSI_STATUS_DATA_W_ACK, &flags); /* begin i2c read to receive eeprom data bytes */ if (status == 0) status = i2c_begin(adap, MVTWSI_STATUS_REPEATED_START, - (chip << 1) | 1); + (chip << 1) | 1, &flags); /* prepare ACK if at least one byte must be received */ if (length > 0) - twsi_control_flags |= MVTWSI_CONTROL_ACK; + flags |= MVTWSI_CONTROL_ACK; /* now receive actual bytes */ while ((status == 0) && length--) { /* reset NAK if we if no more to read now */ if (length == 0) - twsi_control_flags &= ~MVTWSI_CONTROL_ACK; + flags &= ~MVTWSI_CONTROL_ACK; /* read current byte */ - status = twsi_recv(adap, data++); + status = twsi_recv(adap, data++, &flags); } /* Stop transaction */ status = twsi_stop(adap, status); @@ -441,16 +435,18 @@ static int twsi_i2c_write(struct i2c_adapter *adap, uchar chip, uint addr, int alen, uchar *data, int length) { int status; + u8 flags = 0; /* begin i2c write to send the eeprom adress bytes then data bytes */ - status = i2c_begin(adap, MVTWSI_STATUS_START, (chip << 1)); + status = i2c_begin(adap, MVTWSI_STATUS_START, (chip << 1), &flags); /* send addr bytes */ while ((status == 0) && alen--) status = twsi_send(adap, addr >> (8*alen), - MVTWSI_STATUS_DATA_W_ACK); + MVTWSI_STATUS_DATA_W_ACK, &flags); /* send data bytes */ while ((status == 0) && (length-- > 0)) - status = twsi_send(adap, *(data++), MVTWSI_STATUS_DATA_W_ACK); + status = twsi_send(adap, *(data++), MVTWSI_STATUS_DATA_W_ACK, + &flags); /* Stop transaction */ status = twsi_stop(adap, status); /* return 0 or status of first failure */