Received: by 10.223.164.202 with SMTP id h10csp1318867wrb; Wed, 8 Nov 2017 02:21:27 -0800 (PST) X-Google-Smtp-Source: ABhQp+QXEx/PJpoJdgy32NU7et1u5dUqtAtOa6vnFupStzv4UAVDku6ide5cPCpJOVl3eyS6LfpE X-Received: by 10.101.71.132 with SMTP id e4mr1992pgs.206.1510136487131; Wed, 08 Nov 2017 02:21:27 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1510136487; cv=none; d=google.com; s=arc-20160816; b=owsCx9AwtmM0paKJ37J8+2kujO2hmj9OmSPSYb1sQiAK9lxEyMZCb/Qc7NopahlDtT JARmCvY409CnnHlTm/JHM+PIKHV5D+MuVwjpt73fdRCi6fG+YVHX5thHZzrybX7oEeav ldFHYpy97/5TD9jNu6PECvkhz8s12/GVNH/p9jBE3SoDlYin/DwrSZgBt1Qdz2LxfMNs ggBregFz06+tH2cwmgWc2LjmfsW54a56cW+y/UlszYaFF6nhp2xxy/nzLu+t577E4Ppy bHB+DWd2RHvisipBo04ILhdbWJztnYSKMtcYaKiq0K8Rv3W6uG86BfLK7f9qDgiYjoJd ToWg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject:arc-authentication-results; bh=pohnWJzf1TxChbZsTZ027kntuY6WX3lvqFdfJcdgTJY=; b=bOGoOrRCK/plfanEBoTf/bmN4BkfW43XSMLCo6Q9/n+OwraoZlAwGtV8tbMmT0Ous/ Uwr42ZPV9vOFqfiw6ZJ1azIDlEQ2giLciCTHvSSgmcG4lGoVoI/qYXR4lleShuCOmMo7 qYqqkfu1/sPD2qJVYGbdfBFh1q7lKciPcds2PJyxw5xOhYPzFaPEjwhpCSwEFXn4hqNb xmtP7fM0QjvAGEhp2jMxBW6yCvkO5LClTbMHfOLZmqVKHGLryPkrqawqbK1dTsrC8yN0 gCU+b0rPHv1Yti6CY1gwvyLa8jyCZNEIjZ1CalzO/YvxYTDv6w0zo0F4uvahBhi8Ia+C HxZw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=nvidia.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id k7si3578009pff.283.2017.11.08.02.21.14; Wed, 08 Nov 2017 02:21:27 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=nvidia.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751872AbdKHKUj (ORCPT + 91 others); Wed, 8 Nov 2017 05:20:39 -0500 Received: from hqemgate14.nvidia.com ([216.228.121.143]:12757 "EHLO hqemgate14.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751081AbdKHKUf (ORCPT ); Wed, 8 Nov 2017 05:20:35 -0500 Received: from hqpgpgate101.nvidia.com (Not Verified[216.228.121.13]) by hqemgate14.nvidia.com id ; Wed, 08 Nov 2017 02:20:33 -0800 Received: from HQMAIL107.nvidia.com ([172.20.161.6]) by hqpgpgate101.nvidia.com (PGP Universal service); Wed, 08 Nov 2017 02:20:35 -0800 X-PGP-Universal: processed; by hqpgpgate101.nvidia.com on Wed, 08 Nov 2017 02:20:35 -0800 Received: from UKMAIL101.nvidia.com (10.26.138.13) by HQMAIL107.nvidia.com (172.20.187.13) with Microsoft SMTP Server (TLS) id 15.0.1293.2; Wed, 8 Nov 2017 10:20:34 +0000 Received: from [10.21.132.144] (10.21.132.144) by UKMAIL101.nvidia.com (10.26.138.13) with Microsoft SMTP Server (TLS) id 15.0.1293.2; Wed, 8 Nov 2017 10:20:31 +0000 Subject: Re: [PATCH v3] platform/chrome: Use proper protocol transfer function To: Doug Anderson CC: Shawn N , Olof Johansson , "Benson Leung" , Lee Jones , "linux-kernel@vger.kernel.org" , Brian Norris , Brian Norris , Gwendal Grignou , Enric Balletbo , Tomeu Vizoso , "linux-tegra@vger.kernel.org" , Alexandru M Stan References: <20170908205011.77986-1-briannorris@chromium.org> <02aa65e7-e967-055b-2af3-2e9b6ef77935@nvidia.com>

<20170919171401.GA10968@google.com> <20170920061317.GB13616@google.com>

<6beb7b8c-f602-112b-73b2-22188dcea28e@nvidia.com> <5ae1292d-14dc-b917-84cc-2758e82c4795@nvidia.com> From: Jon Hunter Message-ID: Date: Wed, 8 Nov 2017 10:20:30 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: X-Originating-IP: [10.21.132.144] X-ClientProxiedBy: UKMAIL102.nvidia.com (10.26.138.15) To UKMAIL101.nvidia.com (10.26.138.13) Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Doug, On 07/11/17 17:22, Doug Anderson wrote: > On Tue, Nov 7, 2017 at 3:28 AM, Jon Hunter wrote: >> On 10/10/17 17:52, Doug Anderson wrote: >> >> ... >> >>>> I'm still not clear on why we see an error only on the first >>>> transaction after boot. In this case, the embedded controller >>>> previously handled host commands from firmware just fine, and the >>>> handoff between firmware and the kernel shouldn't be significant, from >>>> the EC point of view. If host command timing is consistent from the >>>> master, I would expect to see some constant error rate, eg. some >>>> chance any host command will fail, rather than the first host command >>>> always failing. >>> >>> The AP itself is often quite busy at boot and so the timings for >>> everything change. That could easily explain the problems. >> >> Sorry for the delay, but I have finally had some time to look at this a >> bit closer. I have been able to track down where the additional delay is >> really needed and seems to explain what is going on. >> >> For starters, the SPI chip-select is under h/w control and so the >> software delay has no impact on the timing between the chip-select >> going active and the transaction starting as I had first thought. >> >> I found that a delay is needed between completing the probe the Tegra >> SPI device and the first SPI transaction issued to the EC. In the Tegra >> SPI probe the SPI controller is programmed for master mode, but at the >> same time it clears the chip-select inactive polarity bit meaning that >> initially the SPI chip-select default to active-high (rather that low >> which seems odd). I believe that this then drives the chip-select low >> (active for the EC) and until it is then configured when spi_setup() is >> called which configures it as active-low for the EC. >> To get the first transaction to work for the EC there needs to be a >> delay after we program the chip-select polarity when spi_setup() is >> called. For example ... >> >> diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c >> index 584367f3a0ed..c1075c3c60c8 100644 >> --- a/drivers/mfd/cros_ec_spi.c >> +++ b/drivers/mfd/cros_ec_spi.c >> @@ -648,6 +648,8 @@ static int cros_ec_spi_probe(struct spi_device *spi) >> if (err < 0) >> return err; >> >> + udelay(100); >> + > > This isn't totally crazy, but actually you could probably do this: > > ec_spi->last_transfer_ns = ktime_get_ns(); > > ...that will leverage already existing code and constants and also > will avoid doing a delay if it wasn't needed. You could also then get > rid of some "if (ec_spi->last_transfer_ns)" tests in the code. I'd > support landing that. > > >> ec_spi = devm_kzalloc(dev, sizeof(*ec_spi), GFP_KERNEL); >> if (ec_spi == NULL) >> return -ENOMEM; >> OK, yes and that does work well too. I will send a patch with the following for review. diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c index 584367f3a0ed..477f8e81dc34 100644 --- a/drivers/mfd/cros_ec_spi.c +++ b/drivers/mfd/cros_ec_spi.c @@ -671,6 +671,7 @@ static int cros_ec_spi_probe(struct spi_device *spi) sizeof(struct ec_response_get_protocol_info); ec_dev->dout_size = sizeof(struct ec_host_request); + ec_spi->last_transfer_ns = ktime_get_ns(); err = cros_ec_register(ec_dev); if (err) { >> You may say why not put a delay in the tegra_spi_setup() itself, but we >> have some other SPI devices such as flash devices which are also use an >> active-low chip-select which don't have any issues with this to date. >> Furthermore, this delay is also probably device specific. >> >> From an EC perspective, if the chip-select is asserted is there a >> turnaround time before it can be asserted again? Or in this case maybe >> the issue is that the chip-select is asserted but no transaction occurs >> before it is de-asserted and so is causing a problem. Please note that a >> delay of around ~50us above still fails but 100us seems to be ok. > > Really nice detective work! > > >> Finally, this also works-around the problem by avoiding the chip-select >> from being pulsed low by defaulting all chip-selects to active-low but >> maybe this just masks the problem. >> >> diff --git a/drivers/spi/spi-tegra114.c b/drivers/spi/spi-tegra114.c >> index a76acedd7e2f..7c18204e61d9 100644 >> --- a/drivers/spi/spi-tegra114.c >> +++ b/drivers/spi/spi-tegra114.c >> @@ -1117,7 +1117,7 @@ static int tegra_spi_probe(struct platform_device >> goto exit_pm_disable; >> } >> - tspi->def_command1_reg = SPI_M_S; >> + tspi->def_command1_reg = SPI_M_S | SPI_CS_POL_INACTIVE_MASK; > > Note that even though I'd support adding some sort of delay to the > cros_ec driver, I'd also say that it _might_ make sense to mess with > the SPI driver too, just to avoid glitching the lines at bootup. As > you said, you shouldn't just willy nilly change the default but it > seems like it could make sense to define an initial (board level) > pinctrl state that's in effect until the first call to setup_bus(). I am thinking about making that above change as well, because the reset value of the chip-select polarity bits default to active-low. For active-high devices I don't think that you can ever avoid the chip-select asserting for a period after reset as that is the default setting, but I don't see why we are clearing these bits in probe. I will do a bit more testing with this to avoid any regressions, but both changes seem worthwhile IMO. Cheers Jon -- nvpublic From 1583461002690453246@xxx Wed Nov 08 01:54:49 +0000 2017 X-GM-THRID: 1578006392712496529 X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread