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=-14.4 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,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 49D37C433ED for ; Mon, 3 May 2021 05:08:06 +0000 (UTC) Received: from desiato.infradead.org (desiato.infradead.org [90.155.92.199]) (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 8D33F613AC for ; Mon, 3 May 2021 05:08:05 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 8D33F613AC Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=aj.id.au Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+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=desiato.20200630; h=Sender:Content-Transfer-Encoding :Content-Type:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:Subject:Cc:To:From:Date:References:In-Reply-To: Message-Id:Mime-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=5nvZO66VXK6bhkjPQ/a3do/jxXdz8NrB56gcUyYkJuA=; b=o7pCM9zo5NgENJh5acX3S3dsm jx1HSk2FY30ivm5k17dhHKaLQ/QVSdkW472mvwhABTOFu2U2eMs28txTsUZEaArnfq5r1Bek5SFub rPF60fiydoOqBt52eP64YDsMN6h05iM0rO3mXyHN28OcNGBMXT+np5Q48S5VRmSJXq7N7mGx/2Kgs N1FQPKEwVUlCL/6qpszqwABQEkaekeT1BtKl4iNav5B8lMG5zRrTys/XTe/vKhJq9Z13xkBEr9hN1 RGiAUuaFT71kCbRj9hJdH8xYwq8i/aOCaAioo7t/0h9EF1Inn7nIEN9YqRcDD00lJu9h70rUUK4XP yEMr0QfQA==; Received: from localhost ([::1] helo=desiato.infradead.org) by desiato.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1ldQmA-00DC9W-8e; Mon, 03 May 2021 05:05:46 +0000 Received: from bombadil.infradead.org ([2607:7c80:54:e::133]) by desiato.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1ldQlf-00DBxv-0W for linux-arm-kernel@desiato.infradead.org; Mon, 03 May 2021 05:05:39 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20210309; h=Content-Type:Subject:Cc:To:From:Date: References:In-Reply-To:Message-Id:Mime-Version:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=wZbGrNNB8IKzZFmeNM9BGTHd9BMaGLOY6F6xSuyaF18=; b=kA0P2U34hOHDhMQjnS+z0prg7h zFt6bz4EZJdqQQo+oWKGicHGyz4sVuGA168JwAIPleXoyqctCvMGlixOcE8zQwIVoPZ9bOMZyEbmm gVmuylYFKUtfJcz4BIcG2EAUIUdrQstVIH5STfw21Yn3SG80Kkzp3+oETaWsUxxAvChIynnYfD0qG /lStidxfcoRFdgKbzcfdzQlk/1UDjSyGXWWbzB36Cokz+nNfOUGtzKYEVF+Vk8a78qFW7c2oaET2P 7bJOX9DZ5MEuoE1TbvtEWNPSQDrzlYzigAfRGaFpFQ8B8P2vWfMXY528DmTaQBQIBpLpDW5qrCA65 HUfwQ0Lg==; Received: from new2-smtp.messagingengine.com ([66.111.4.224]) by bombadil.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1ldQlb-002pXU-M8 for linux-arm-kernel@lists.infradead.org; Mon, 03 May 2021 05:05:13 +0000 Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailnew.nyi.internal (Postfix) with ESMTP id C8347580965; Mon, 3 May 2021 01:05:10 -0400 (EDT) Received: from imap2 ([10.202.2.52]) by compute3.internal (MEProxy); Mon, 03 May 2021 01:05:10 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=aj.id.au; h= mime-version:message-id:in-reply-to:references:date:from:to:cc :subject:content-type; s=fm2; bh=wZbGrNNB8IKzZFmeNM9BGTHd9BMaGLO Y6F6xSuyaF18=; b=an2TWdeMwdv8G1iuCS7DLTs+lkXvq+3tfPdMyDwI8dKWrwb nqewtPcOrnJcnUQVKnwvR25JJSISoytO/AM9I10ERmpOFh3YFchv8o/zgb2pUhkN 3rFGTOPy0wf9w1b92SHb/feEig5yygfJ8YHJ0H+A3zucZYwZ7q/fyRGPPKnpIsHH Q1/Oko9+7NSc/136v6C1+UD/KWP3m4HTymdJ/tug0hZVVH86PpjUbleRUIqX4Bmw uotf4YaCUQVHfh0pQIbCihGEIVvAVeTrdcgs34TcDN49yj48dBP0/3Yu86neanW5 IX18wDddBFZTiQuHMcQvRmfo+1IHUKQNvRG+CbQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm2; bh=wZbGrN NB8IKzZFmeNM9BGTHd9BMaGLOY6F6xSuyaF18=; b=YkbzeqPr/dG3JUY06vDlmK VJk70rwWuEiFMHNiufwjudRM4UPee9bLpphSHInHvz48X/pr8wm1QIIDdyEMeAH9 9L8K26/XQ4A3U/oJmErTK2TabpGw9ixtKXIAigvT2xLOZT57b7tenZsZT804NOZ7 YmZPaxphcpPvwvIxeErT0E4ZQDHx8ybIN3se1m8+D9z2ofm+wcLvDUK53lbKwFiM iUk2vKKAju/JwGtrKrHBHe6zWzLKA2qs1MfK8RcxmJHLFU+lAA9xEDXl52XqnJOp /DgiEGxQTL0fU5IBhwdtl28uM7WDSiBrA+ACnzPfgITw5EsI+lGAx8/yjaDIGvRQ == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduledrvdeffedguddvucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepofgfggfkjghffffhvffutgesthdtredtreerjeenucfhrhhomhepfdetnhgu rhgvficulfgvfhhfvghrhidfuceorghnughrvgifsegrjhdrihgurdgruheqnecuggftrf grthhtvghrnhepudehtddtleektedvfeeitdeljeekveelkeegvdfhtdejhefgfedtfedv jeejledtnecuffhomhgrihhnpehkvghrnhgvlhdrohhrghenucevlhhushhtvghrufhiii gvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpegrnhgurhgvfiesrghjrdhiugdrrghu X-ME-Proxy: Received: by mailuser.nyi.internal (Postfix, from userid 501) id 4BBB0A00079; Mon, 3 May 2021 01:05:09 -0400 (EDT) X-Mailer: MessagingEngine.com Webmail Interface User-Agent: Cyrus-JMAP/3.5.0-alpha0-403-gbc3c488b23-fm-20210419.005-gbc3c488b Mime-Version: 1.0 Message-Id: In-Reply-To: <20210503014336.20256-4-steven_lee@aspeedtech.com> References: <20210503014336.20256-1-steven_lee@aspeedtech.com> <20210503014336.20256-4-steven_lee@aspeedtech.com> Date: Mon, 03 May 2021 14:34:06 +0930 From: "Andrew Jeffery" To: "Steven Lee" , "Adrian Hunter" , "Ulf Hansson" , "Joel Stanley" , "Philipp Zabel" , "moderated list:ASPEED SD/MMC DRIVER" , "moderated list:ASPEED SD/MMC DRIVER" , "moderated list:ARM/ASPEED MACHINE SUPPORT" , "open list" Cc: "Chin-Ting Kuo" , "Ryan Chen" , "Hongwei Zhang" Subject: =?UTF-8?Q?Re:_[PATCH_v2_3/3]_mmc:_sdhci-of-aspeed:_Sync_capabilities_fro?= =?UTF-8?Q?m_device_tree_to_ast2600_SoC_registers?= X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210502_220511_842310_720EA36F X-CRM114-Status: GOOD ( 35.10 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Steven, On Mon, 3 May 2021, at 11:13, Steven Lee wrote: > Sync Capbility Registers(SDIO140, SDIO144, SDIO240, SDIO244) of ast2600 > SoC from the device tree. > The bit 26(Voltage Support 1.8v) of SDIO140/SDIO240 is set to 1 if > "mmc-hs200-1_8v" or "sd-uhs-sdr104" is added in the device tree. > The bit 1(SDR104 Supported) of SDR144/SDR244 is set to 1 if "sd-uhs-sdr104" > is added in the device tree. > "timing-phase" is synced to SDIO0F4(Colock Phase Control) > > Signed-off-by: Steven Lee > --- > drivers/mmc/host/sdhci-of-aspeed.c | 107 ++++++++++++++++++++++++++--- > 1 file changed, 98 insertions(+), 9 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c > b/drivers/mmc/host/sdhci-of-aspeed.c > index 7d8692e90996..2d755bac777a 100644 > --- a/drivers/mmc/host/sdhci-of-aspeed.c > +++ b/drivers/mmc/host/sdhci-of-aspeed.c > @@ -13,6 +13,7 @@ > #include > #include > #include > +#include > #include > > #include "sdhci-pltfm.h" > @@ -30,10 +31,18 @@ > #define ASPEED_SDC_S0_PHASE_IN_EN BIT(2) > #define ASPEED_SDC_S0_PHASE_OUT_EN GENMASK(1, 0) > #define ASPEED_SDC_PHASE_MAX 31 > +#define ASPEED_SDC_CAP1_1_8V BIT(26) > +#define ASPEED_SDC_CAP2_SDR104 BIT(1) > +#define PROBE_AFTER_ASSET_DEASSERT 0x1 > + > +struct aspeed_sdc_info { > + u32 flag; > +}; > > struct aspeed_sdc { > struct clk *clk; > struct resource *res; > + struct reset_control *rst; > > spinlock_t lock; > void __iomem *regs; > @@ -72,6 +81,44 @@ struct aspeed_sdhci { > const struct aspeed_sdhci_phase_desc *phase_desc; > }; > > +struct aspeed_sdc_info ast2600_sdc_info = { > + .flag = PROBE_AFTER_ASSET_DEASSERT > +}; > + > +/* > + * The function sets the mirror register for updating > + * capbilities of the current slot. > + * > + * slot | cap_idx | caps_reg | mirror_reg > + * -----|---------|----------|------------ > + * 0 | 0 | SDIO140 | SDIO10 > + * 0 | 1 | SDIO144 | SDIO14 > + * 1 | 0 | SDIO240 | SDIO20 > + * 1 | 1 | SDIO244 | SDIO24 > + */ > +static void aspeed_sdc_set_slot_capability(struct sdhci_host *host, > + struct aspeed_sdc *sdc, > + u32 reg_val, > + u8 slot, > + u8 cap_idx) Having thought about this some more now we have code, I wonder if we can get rid of `cap_idx` as a parameter. Something like: static void aspeed_sdc_set_slot_capability(struct sdhci_host *host, struct aspeed_sdc *sdc, int capability, bool enable, u8 slot); >From there, instead of #define ASPEED_SDC_CAP1_1_8V BIT(26) #define ASPEED_SDC_CAP2_SDR104 BIT(1) We do /* SDIO{10,20} */ #define ASPEED_SDC_CAP1_1_8V (0 * 32 + 26) /* SDIO{14,24} */ #define ASPEED_SDC_CAP2_SDR104 (1 * 32 + 1) Then in the implementation of aspeed_sdc_set_slot_capability() we derive cap_idx and reg_val: u8 reg_val = enable * BIT(capability % 32); u8 cap_reg = capability / 32; That way we get rid of the 0 and 1 magic values for cap_idx when invoking aspeed_sdc_set_slot_capability() and the caller can't accidentally pass the wrong value. > +{ > + u8 caps_reg_offset; > + u32 caps_reg; > + u32 mirror_reg_offset; > + u32 caps_val; > + > + if (cap_idx > 1 || slot > 1) > + return; > + > + caps_reg_offset = (cap_idx == 0) ? 0 : 4; > + caps_reg = 0x40 + caps_reg_offset; > + caps_val = sdhci_readl(host, caps_reg); Hmm, I think you used sdhci_readl() because I commented on that last time. If the global-area registers are truly mirrored we could read from them as well right? In which case we could just use readl(sdc->regs + mirror_reg_offset)? If so we can drop the host parameter and (incorporating my suggestion above) just have: static void aspeed_sdc_set_slot_capability(struct aspeed_sdc *sdc, int capability, bool enable, u8 slot); Sorry if I've sort of flip-flopped on that, but I think originally you were still reading from the SDHCI (read-only) address? > + caps_val |= reg_val; > + mirror_reg_offset = (slot == 0) ? 0x10 : 0x20; > + mirror_reg_offset += caps_reg_offset; > + writel(caps_val, sdc->regs + mirror_reg_offset); > +} > + > static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc, > struct aspeed_sdhci *sdhci, > bool bus8) > @@ -329,9 +376,11 @@ static int aspeed_sdhci_probe(struct platform_device *pdev) > { > const struct aspeed_sdhci_pdata *aspeed_pdata; > struct sdhci_pltfm_host *pltfm_host; > + struct device_node *np = pdev->dev.of_node; > struct aspeed_sdhci *dev; > struct sdhci_host *host; > struct resource *res; > + u32 reg_val; > int slot; > int ret; > > @@ -372,6 +421,21 @@ static int aspeed_sdhci_probe(struct platform_device *pdev) > > sdhci_get_of_property(pdev); > > + if (of_property_read_bool(np, "mmc-hs200-1_8v") || > + of_property_read_bool(np, "sd-uhs-sdr104")) > + aspeed_sdc_set_slot_capability(host, > + dev->parent, > + ASPEED_SDC_CAP1_1_8V, > + slot, > + 0); See the discussion above about reworking aspeed_sdc_set_slot_capability. > + > + if (of_property_read_bool(np, "sd-uhs-sdr104")) > + aspeed_sdc_set_slot_capability(host, > + dev->parent, > + ASPEED_SDC_CAP2_SDR104, > + slot, > + 1); Again here. > + > pltfm_host->clk = devm_clk_get(&pdev->dev, NULL); > if (IS_ERR(pltfm_host->clk)) > return PTR_ERR(pltfm_host->clk); > @@ -476,12 +540,25 @@ static struct platform_driver aspeed_sdhci_driver = { > .remove = aspeed_sdhci_remove, > }; > > +static const struct of_device_id aspeed_sdc_of_match[] = { > + { .compatible = "aspeed,ast2400-sd-controller", }, > + { .compatible = "aspeed,ast2500-sd-controller", }, > + { .compatible = "aspeed,ast2600-sd-controller", .data = &ast2600_sdc_info}, > + { } > +}; > + > +MODULE_DEVICE_TABLE(of, aspeed_sdc_of_match); > + > static int aspeed_sdc_probe(struct platform_device *pdev) > > { > struct device_node *parent, *child; > struct aspeed_sdc *sdc; > + const struct of_device_id *match = NULL; > + const struct aspeed_sdc_info *info = NULL; > + > int ret; > + u32 timing_phase; > > sdc = devm_kzalloc(&pdev->dev, sizeof(*sdc), GFP_KERNEL); > if (!sdc) > @@ -489,6 +566,23 @@ static int aspeed_sdc_probe(struct platform_device *pdev) > > spin_lock_init(&sdc->lock); > > + match = of_match_device(aspeed_sdc_of_match, &pdev->dev); > + if (!match) > + return -ENODEV; > + > + if (match->data) > + info = match->data; > + > + if (info) { > + if (info->flag & PROBE_AFTER_ASSET_DEASSERT) { > + sdc->rst = devm_reset_control_get(&pdev->dev, NULL); > + if (!IS_ERR(sdc->rst)) { > + reset_control_assert(sdc->rst); > + reset_control_deassert(sdc->rst); > + } > + } > + } I think this should be a separate patch. >From the code it seems that this is necessary for just the 2600? Where is this documented? > + > sdc->clk = devm_clk_get(&pdev->dev, NULL); > if (IS_ERR(sdc->clk)) > return PTR_ERR(sdc->clk); > @@ -506,6 +600,10 @@ static int aspeed_sdc_probe(struct platform_device *pdev) > goto err_clk; > } > > + if (!of_property_read_u32(pdev->dev.of_node, > + "timing-phase", &timing_phase)) > + writel(timing_phase, sdc->regs + ASPEED_SDC_PHASE); I asked on v1 that you use the phase support already in the bindings and in the driver. The example you added in the binding document[1] used the existing devicetree properties but it seems you haven't fixed the code. Please drop your phase implementation from the patch. [1] https://lore.kernel.org/lkml/20210503014336.20256-2-steven_lee@aspeedtech.com/ Cheers, Andrew _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel