From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753896AbdF1ElZ (ORCPT ); Wed, 28 Jun 2017 00:41:25 -0400 Received: from regular1.263xmail.com ([211.150.99.134]:51432 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751545AbdF1ElQ (ORCPT ); Wed, 28 Jun 2017 00:41:16 -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: X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Message-ID: <59533365.60003@rock-chips.com> Date: Wed, 28 Jun 2017 12:41:09 +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 CC: "linux-kernel@vger.kernel.org" , "broonie@kernel.org" , Brian Norris , =?UTF-8?B?SGVpa28gU3TDvGJuZXI=?= , linux-spi , "open list:ARM/Rockchip SoC..." , "linux-arm-kernel@lists.infradead.org" Subject: Re: [PATCH v2] spi: rockchip: Disable Runtime PM when chip select is asserted References: <1498530035-22070-1-git-send-email-jeffy.chen@rock-chips.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/28/2017 03:27 AM, Doug Anderson wrote: > Hi, > > On Mon, Jun 26, 2017 at 7:20 PM, Jeffy Chen wrote: >> The rockchip spi would stop driving pins when runtime suspended, which >> might break slave's xfer(for example cros_ec). >> >> Since we have pullups on those pins, we only need to care about the CS >> asserted case. >> >> So let's keep the spi alive when chip select is asserted for that. >> >> Also change use pm_runtime_put instead of pm_runtime_put_sync. >> >> Suggested-by: Doug Anderson >> Signed-off-by: Jeffy Chen >> >> --- >> >> Changes in v2: >> Improve commit message and comments and coding style. >> >> drivers/spi/spi-rockchip.c | 23 ++++++++++++++++++----- >> 1 file changed, 18 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c >> index acf31f3..ea0edd7 100644 >> --- a/drivers/spi/spi-rockchip.c >> +++ b/drivers/spi/spi-rockchip.c >> @@ -264,7 +264,7 @@ static inline u32 rx_max(struct rockchip_spi *rs) >> >> static void rockchip_spi_set_cs(struct spi_device *spi, bool enable) >> { >> - u32 ser; >> + u32 ser, new_ser; >> struct spi_master *master = spi->master; >> struct rockchip_spi *rs = spi_master_get_devdata(master); >> >> @@ -288,13 +288,26 @@ static void rockchip_spi_set_cs(struct spi_device *spi, bool enable) >> * Note: enable(rockchip_spi_set_cs) = !enable(spi_set_cs) >> */ >> if (!enable) >> - ser |= 1 << spi->chip_select; >> + new_ser = ser | BIT(spi->chip_select); >> else >> - ser &= ~(1 << spi->chip_select); >> + new_ser = ser & ~BIT(spi->chip_select); >> >> - writel_relaxed(ser, rs->regs + ROCKCHIP_SPI_SER); >> + if (new_ser != ser) { > > IMHO it's not a great idea to use the state of the hardware register > here. Using the state of the hardware register probably works OK, but > it makes me just a little nervous. If something happened to the state > of the register (someone used /dev/mem to tweak, or the peripherals > got reset, or ...) then you could end up with an unbalanced set of PM > Runtime calls. I know none of those things are common, but it still > seems less than great to me. > > I'd rather we kept track in "struct rockchip_spi" whether we already > called pm_runtime_get_sync(). AKA the following (untested): > > bool cs_asserted = !enable; > > /* Return immediately for no-op */ > if (cs_asserted == rs->cs_asserted) > return; > > /* Keep things powered as long as CS is asserted */ > if (cs_asserted) { > pm_runtime_get_sync(rs->dev); > rs->cs_asserted = true; > } > > ser = ... > ... > ... > > if (!cs_asserted) > pm_runtime_put(rs->dev); > > > NOTE: another advantage of storing the state in 'struct rockchip_spi' > is that you can access it without pm_runtime_get_sync(). > ok, that make sense :) > >> + writel_relaxed(new_ser, rs->regs + ROCKCHIP_SPI_SER); >> >> - pm_runtime_put_sync(rs->dev); >> + /* >> + * The rockchip spi would stop driving all pins when powered >> + * down. >> + * So hold a runtime PM reference as long as CS is asserted. >> + */ >> + if (!enable) >> + return; >> + >> + /* Drop reference from when we first asserted CS */ >> + pm_runtime_put(rs->dev); >> + } >> + >> + pm_runtime_put(rs->dev); > > One note is that you should still submit your patch to add > "SPI_MASTER_GPIO_SS" to spi-rockchip.c. It's important that the SPI > driver see the CS transitions so that it can do the PM Runtime get/put > even when someone uses a GPIO chip select. Even though the GPIO chip > select will keep state, we don't want the rest of the lines to stop > being driven. ok, will do. > > > -Doug > > >