From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754239AbdFWDwR (ORCPT ); Thu, 22 Jun 2017 23:52:17 -0400 Received: from regular1.263xmail.com ([211.150.99.134]:42405 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751927AbdFWDwP (ORCPT ); Thu, 22 Jun 2017 23:52:15 -0400 X-263anti-spam: BIG:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-ADDR-CHECKED4: 1 X-ABS-CHECKED: 1 X-SKE-CHECKED: 1 X-ANTISPAM-LEVEL: 2 X-RL-SENDER: jeffy.chen@rock-chips.com X-FST-TO: dianders@chromium.org X-SENDER-IP: 103.29.142.67 X-LOGIN-NAME: jeffy.chen@rock-chips.com X-UNIQUE-TAG: <52c7be3cb1b0c91fefb6a1c0d42fdf7c> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Message-ID: <594C905E.1040208@rock-chips.com> Date: Fri, 23 Jun 2017 11:51:58 +0800 From: jeffy User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:19.0) Gecko/20130126 Thunderbird/19.0 MIME-Version: 1.0 To: Doug Anderson , Brian Norris CC: Mark Brown , "linux-kernel@vger.kernel.org" , =?UTF-8?B?SGVpa28gU3TDvGJuZXI=?= , "devicetree@vger.kernel.org" , "open list:ARM/Rockchip SoC..." , Rob Herring , "linux-arm-kernel@lists.infradead.org" , Will Deacon , Mark Rutland , Catalin Marinas Subject: Re: [PATCH v2 4/4] arm64: dts: rockchip: use cs-gpios for cros_ec_spi References: <1497331543-8565-1-git-send-email-jeffy.chen@rock-chips.com> <1497331543-8565-4-git-send-email-jeffy.chen@rock-chips.com> <20170613175043.GC9026@google.com> <20170613182225.smahsf3jzvbc7w7z@sirena.org.uk> <20170620004739.GA67314@google.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi doug, Thanx for your comments. On 06/23/2017 06:47 AM, Doug Anderson wrote: > Hi, > > On Mon, Jun 19, 2017 at 5:47 PM, Brian Norris wrote: >> Hi Mark, >> >> Forgot to follow up here: >> >> On Tue, Jun 13, 2017 at 07:22:25PM +0100, Mark Brown wrote: >>> On Tue, Jun 13, 2017 at 10:50:44AM -0700, Brian Norris wrote: >>>> On Tue, Jun 13, 2017 at 01:25:43PM +0800, Jeffy Chen wrote: >>>>> The cros_ec requires CS line to be active after last message. But the CS >>>>> would be toggled when powering off/on rockchip spi, which breaks ec xfer. >>>>> Use GPIO CS to prevent that. >>> >>>> I suppose this change is fine. (At least, I don't have a good reason not >>>> to do this.) >>> >>>> But I still wonder whether this is something that the SPI core can be >>>> expected to handle. drivers/mfd/cros_ec_spi.c already sets the >>>> appropriate trans->cs_change bits, to ensure CS remains active in >>>> between certain messages (all under spi_bus_lock()). But you're >>>> suggesting that your bus controller may deassert CS if you runtime >>>> suspend the device (e.g., in between messages). >>> >>>> So, is your controller just peculiar? Or should the SPI core avoid >>>> autosuspending the bus controller when it's been instructed to keep CS >>>> active? Any thoughts Mark? >>> >>> This sounds like the controller being unusual - though frankly the >>> ChromeOS chip select usage is also odd so it's fairly rare for something >>> like this to come up. I'd not expect a runtime suspend to loose the pin >>> state, though possibly through use of pinctrl rather than the >>> controller. >> >> I haven't personally verified this behavior (it probably wouldn't be too >> hard to rig up a test driver to hold CS low while allowing the >> controller to autosuspend? spidev can do this?), but Rockchip folks seem >> to have concluded this. >> >> I suppose I'm fine with relying on cs-gpios as a workaround. > > I'm similarly hesitant to rely on cs-gpios as a workaround, though I > won't directly stand in its way... ...it seems like it would be > slightly better to actually add a runtime_suspend() callback and > adjust the pinmux dynamically (that would allow us to use the hardware > chip select control if we ever enable that in the driver), but I'm not > sure all the extra work to do that is worth it. > > It feels a little bit to me like the workaround here doesn't belong in > the board's device tree file, though. This is a quirk of the SoC's > SPI controller whenever it's runtime suspended. Any board using this > SPI could possibly be affected, right? hmm, so i should add cs_gpio to all rockchip boards right? > > > Oh wait (!!!!) > > > Let's think about this. Let me ask a question. When you runtime > suspend the SPI part (and turn off the power domain) but don't > configure pins to be GPIO, what happens? I'm assuming it's one of > three things: > > 1. The line is driven a certain direction (probably low). This seems unlikely. > > 2. The line is no longer driven by the SPI controller and thus the > pin's pulls take effect. This seems _likely_. > > 3. The line is no longer driven by the SPI controller and somehow the > pulls stop taking effect. This seems unlikely. > > > ...I'll assume that #2 is right (please correct if I'm wrong). > Thinking about that makes me think that we need to worry not just > about the chip select line but about the other SPI lines too, right? > AKA if the SPI controller stops driving the chip select line, it's > probably also not driving MISO, MOSI, or TXD. > > ...so looking at all the SPI lines, they all have pullup configured in > the "default" mode in rk3399.dtsi. > > ...and looking as "cros_ec_spi.c", I see that we appear to be using MODE_0. > > > That means if you runtime suspend while the cros EC code was running > and none of your patches have landed, all lines will float high. > > 1. Chip select will be deasserted (this is the problem you're trying to solve). > > 2. Data line and clock line will get pulled high. > > Using spi.h, MODE_0 means SPI_CPOL=0 and SPI_CPHA=0. Using Wikipedia > (which is never wrong, of course), that means data is captured on the > clock's rising edge. Thus we'll actually clock one bit of data here, > but at the same time that we try to turn off chip select. > > > ...now we look at your proposed solution and we'll leave chip select > on, but we'll still clock one bit of data (oops). ...or, I guess, if > the EC itself has pulls configured we might be in some state where the > different pulls are fighting, but that still seems non-ideal. > > --- > > So how do we fix this? IMHO: > > Add 4 new pinctrl states in rk3399.dtsi: > > cs_low_clk_low, cs_low_clk_high, cs_high_clk_low, cs_high_clk_high > > These would each look something like this: > > spi5_cs_low_data_low: spi5-cs-low-data-low { > rockchip,pins = <2 22 RK_FUNC_0 &pcfg_output_low>, > <2 23 RK_FUNC_0 &pcfg_output_low>; > }; > > Where "pcfg_output_low" would be moved from the existing location in > "rk3399-gru.dtsi" to the main "rk3399.dtsi" file. > > > ...now, you'd define runtime_suspend and runtime_resume functions > where you'd look at the current state of the chip select and output > and select one of these 4 pinmuxes so that things _don't_ change. > > If the pinmuxes didn't exist you'd simply deny PM Runtime Transitions. > That would nicely take care of the backward compatibility problem. > Old DTS files would work, they just wouldn't be able to Runtime PM > properly. so we should use runtime pinmuxes to fix this issue, and also support cs_gpio as an option right? > > --- > > Anyway, maybe that's all crazy. What do others think? > > > -Doug > > >