From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DD2C4C31E57 for ; Mon, 17 Jun 2019 12:22:23 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id B16F62064A for ; Mon, 17 Jun 2019 12:22:23 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="CKNpUj/k"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="BaY/Mw4u" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B16F62064A Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=8tS9KD3os1b5o9xNd7ZWP9fQoBCrOHCFjr5fnoiPKd4=; b=CKNpUj/kAuhv/K clytXnzFnSjtZ7anh61zC1AyUP/nTUwCF+j8o0kiP5ZNdLHZzqtxOYkUES58L41dEEtaFEvGiuh6q 3NsOZvWVfgHSy1n3bVgpp5PZmUAPuAT3JK4JSeJyskBxzpLHffLQ0YOgUHIWKh0/7sSHs1QPUaKCG Dr10BoRDLH3sUx7xaS6md6+f8hH34BUBfeoUOtwtpJy0va5/3ab7yORpJ8gPfXdiI5COPIjhreme8 y0+7vrPgFWE8Y7VRqZDaMuzRDx+TN5IH9szkjYOrLQ70GG442dWCn0AxPGESUEZ7+hDFKGB7FRyDO IxO1DbE1/3osqFqaSg+w==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92 #3 (Red Hat Linux)) id 1hcqeU-000570-GL; Mon, 17 Jun 2019 12:22:22 +0000 Received: from mail-vs1-xe42.google.com ([2607:f8b0:4864:20::e42]) by bombadil.infradead.org with esmtps (Exim 4.92 #3 (Red Hat Linux)) id 1hcqeF-0004xT-9g for linux-arm-kernel@lists.infradead.org; Mon, 17 Jun 2019 12:22:08 +0000 Received: by mail-vs1-xe42.google.com with SMTP id 190so5950963vsf.9 for ; Mon, 17 Jun 2019 05:22:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=inGSDUfgy12MnV2jo2D4Nubdqwd8cc039JMXj9GTRrA=; b=BaY/Mw4ucNOLlZG6021hvYu+v8OFPCGaf5W2OANB6XPbg0UqT/cgAB3AravOtr3BEF yUGEhb38oekQt0UrrCBEpZgcUST8GsaDeV4pe+g7KoeJlRnXUAyDklVTAS8hGgTZTVnY A7ZsBtlltgi8+h1kaTi80I2Kbpf6jzD/DnljX9n76hZJmsVDNxJRQTcOl2TKSU6F5crE SyA8FgL2RvuBCgYHXHZgDLkut1yb6DaP09n2as2TT836YKODLrQ0ewzsz86A5EIXw8YI NjiHJywpoahWsy5SzgYUhCKomVxgX2YyRex2O/fOhOQlTT+Ub10tpaDPZmfd+2GCDERS +T5Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=inGSDUfgy12MnV2jo2D4Nubdqwd8cc039JMXj9GTRrA=; b=X/XUjjRNXPdvIWI3D/Le487i9s1cewqLb+OMGvuNLgWGBSUSWUbIK7eBrcx8LG5/DR roAWVkj71w7WYYh7spEw+S54ZM3c+kRYXqNVPW2zYk0O49WzzjBNMSSfxtwYZlRKgaHt tjsnfdC7/NQhWwBKmIZ9t9MGFxyAJg2iFHM2xVCXItCRJWY3fyhCqcXFk5WNK41LxeEF 3VV6EC1VMbOb9fomNOFfV6kZUWtLEUlzKSD+DVsUsUdJCHd12IRqCxQ8LNWNcKX0hxKr tmQEG6kDs/0DmgvsenvEnilMYtMnx8evuV8Sz9fcUPtvG3T7YzCZtsldZwmNs0oKJxgF lSGA== X-Gm-Message-State: APjAAAVeECaUMndJTJO9Coefcaipv78xfpA9fm3L2S3LeDXb2+eqhL3W pW+Dm4qhMFSJWblbUaiOe03ox3Kn93a5mU9xR84wfq+qQJc= X-Google-Smtp-Source: APXvYqwvsX2qorPP589qYvilkHLdtcwg2ZCmDdhmhV70oupOdc+lud+k/srIUL3u3WxMmegbWHGCs6qQXzT1ePDp16s= X-Received: by 2002:a67:3254:: with SMTP id y81mr20750067vsy.34.1560774125745; Mon, 17 Jun 2019 05:22:05 -0700 (PDT) MIME-Version: 1.0 References: <1560247011-26369-1-git-send-email-manish.narani@xilinx.com> <1560247011-26369-4-git-send-email-manish.narani@xilinx.com> <5feac3fb-bef3-b7d1-57d6-81e115e1f555@xilinx.com> In-Reply-To: <5feac3fb-bef3-b7d1-57d6-81e115e1f555@xilinx.com> From: Ulf Hansson Date: Mon, 17 Jun 2019 14:21:28 +0200 Message-ID: Subject: Re: [PATCH 3/3] mmc: sdhci-of-arasan: Add support for ZynqMP Platform Tap Delays Setup To: Michal Simek X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190617_052207_339932_79A4BE13 X-CRM114-Status: GOOD ( 36.62 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , DTML , rajan.vaja@xilinx.com, nava.manne@xilinx.com, "linux-mmc@vger.kernel.org" , Adrian Hunter , Linux Kernel Mailing List , Olof Johansson , Rob Herring , Manish Narani , jolly.shah@xilinx.com, Linux ARM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, 17 Jun 2019 at 13:28, Michal Simek wrote: > > Hi, > > On 17. 06. 19 13:15, Ulf Hansson wrote: > > On Tue, 11 Jun 2019 at 11:57, Manish Narani wrote: > >> > >> Apart from taps set by auto tuning, ZynqMP platform has feature to set > >> the tap values manually. Add support to read tap delay values from > >> DT and set the same in HW via ZynqMP SoC framework. Reading Tap > >> Delays from DT is optional, if the property is not available in DT the > >> driver will use the pre-defined Tap Delay Values. > >> > >> Signed-off-by: Manish Narani > >> --- > >> drivers/mmc/host/sdhci-of-arasan.c | 173 ++++++++++++++++++++++++++++++++++++- > >> 1 file changed, 172 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c > >> index b12abf9..7af6cec 100644 > >> --- a/drivers/mmc/host/sdhci-of-arasan.c > >> +++ b/drivers/mmc/host/sdhci-of-arasan.c > >> @@ -22,6 +22,7 @@ > >> #include > >> #include > >> #include > >> +#include > >> > >> #include "cqhci.h" > >> #include "sdhci-pltfm.h" > >> @@ -32,6 +33,10 @@ > >> > >> #define PHY_CLK_TOO_SLOW_HZ 400000 > >> > >> +/* Default settings for ZynqMP Tap Delays */ > >> +#define ZYNQMP_ITAP_DELAYS {0, 0x15, 0x15, 0, 0x15, 0, 0, 0x3D, 0x12, 0, 0} > >> +#define ZYNQMP_OTAP_DELAYS {0, 0x5, 0x6, 0, 0x5, 0x3, 0x3, 0x4, 0x6, 0x3, 0} > >> + > >> /* > >> * On some SoCs the syscon area has a feature where the upper 16-bits of > >> * each 32-bit register act as a write mask for the lower 16-bits. This allows > >> @@ -81,6 +86,7 @@ struct sdhci_arasan_soc_ctl_map { > >> * @sdcardclk: Pointer to normal 'struct clock' for sdcardclk_hw. > >> * @soc_ctl_base: Pointer to regmap for syscon for soc_ctl registers. > >> * @soc_ctl_map: Map to get offsets into soc_ctl registers. > >> + * @of_data: Platform specific runtime data storage pointer > >> */ > >> struct sdhci_arasan_data { > >> struct sdhci_host *host; > >> @@ -101,6 +107,15 @@ struct sdhci_arasan_data { > >> /* Controller immediately reports SDHCI_CLOCK_INT_STABLE after enabling the > >> * internal clock even when the clock isn't stable */ > >> #define SDHCI_ARASAN_QUIRK_CLOCK_UNSTABLE BIT(1) > >> + > >> + void *of_data; > >> +}; > >> + > >> +struct sdhci_arasan_zynqmp_data { > >> + void (*set_tap_delay)(struct sdhci_host *host); > >> + const struct zynqmp_eemi_ops *eemi_ops; > >> + u8 tapdly[MMC_TIMING_MMC_HS400 + 1][2]; /* [0] for input delay, */ > >> + /* [1] for output delay */ > >> }; > > > > Please use two different structs, one for the clock provider data and > > one for the mmc variant/platform data. This makes the code more > > readable. > > Origin version before sending that out was using two fields. > + u32 itapdly[MMC_TIMING_MMC_HS400 + 1]; > + u32 otapdly[MMC_TIMING_MMC_HS400 + 1]; > > I did asked for putting it together to two dimensional array for > improving readability of this code. The reason was that you need to take > care about input/output together. > One thing I was also suggesting was to use instead of 2 just enum values > to specify IN=0/OUT/MAX to improve readability of this. > Do you think that using enum should be enough? Not sure I understand what you suggest here, sorry. I have no problem with the enums. The important point I am trying to make here, is that we should split the clock provider data and the mmc variant data, simply because those doesn't really belong to each each other. Something like this: struct sdhci_arasan_zynqmp_data { bool tap_delays; u8 tapdly[MMC_TIMING_MMC_HS400 + 1][2]; /* [0] for input delay, [1] for output delay */ + other variant specific data one may want to put here } These are just regular mmc OF data that are parsed as any other property of the mmc device. The "const struct zynqmp_eemi_ops *eemi_ops; should then be moved into a clock provider specific struct, which is assigned when calling sdhci_arasan_register_sdclk. I understand that all the clock data is folded into struct sdhci_arasan_data today, but I think that should be moved into a "sub-struct" for the clock specifics. Moreover, when registering the clock, we should convert from using devm_clk_register() into devm_clk_hw_register() as the first one is now deprecated. > > > > In regards to the mmc data part, I suggest to drop the > > ->set_tap_delay() callback, but rather use a boolean flag to indicate > > whether clock phases needs to be changed for the variant. Potentially > > that could even be skipped and instead call clk_set_phase() > > unconditionally, as the clock core deals fine with clock providers > > that doesn't support the ->set_phase() callback. > > In connection to another version of this driver for latest Xilinx chip > it would be better to keep set_tap_delay callback in the driver. The > reason is that new chip/ip is capable to setup tap delays directly > without asking firmware to do it. That's why for versal IP there is a > need to call different setup_tap_delay function. The ->set_tap_delay() callback is for ZyncMp pointing to sdhci_arasan_zynqmp_set_tap_delay(). This function calls the clk_set_phase() API. What does ->set_tap_delay() do for the latest version? > > > > > [...] > > > > Otherwise this looks good to me! > > > > When it comes to patch1, I need an ack from Michal to pick it up. > > I am waiting till Rob ack dt binding and then I wanted to talk to you if > you want to take it with 1/3 or if you want me to take all of them via > my tree. > In previous releases I was taking them via my tree because there were > several subsystem changing firmware interface. In this cycle there are > just small changes to firmware interface that's why taking it via your > tree shouldn't be a problem too. Okay, then let's target this via my mmc tree this time. Kind regards Uffe _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel