From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steven Lee Date: Fri, 7 May 2021 14:59:18 +0800 Subject: [PATCH v3 4/5] mmc: sdhci-of-aspeed: Add a helper for updating capability register. In-Reply-To: References: <20210506100312.1638-1-steven_lee@aspeedtech.com> <20210506100312.1638-5-steven_lee@aspeedtech.com> Message-ID: <20210507065918.GE23749@aspeedtech.com> List-Id: To: linux-aspeed@lists.ozlabs.org MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit The 05/07/2021 10:13, Andrew Jeffery wrote: > Hi Steven, > > I have some minor comments. I expect you're going to do a v4 of the > series, so if you'd like to clean them up in the process I'd appreciate > it. > Yes, I am going to prepare v4 patch for meeting reviewer's expectation including your comment in this patch. I've learned a lot from your suggestion for driver upstream. Many thanks! > However, from a pragmatic standpoint I think the patch is in good shape. > > On Thu, 6 May 2021, at 19:33, Steven Lee wrote: > > The patch add a new function aspeed_sdc_set_slot_capability() for > > updating sdhci capability register. > > The commit message should explain why the patch is necessary and not > what it does, as what it does is contained in the diff. > > It's okay to explain *how* the patch acheives its goals if the > implementation is subtle or complex. > > Maybe the commit message could be something like: > > > ``` > Configure the SDHCIs as specified by the devicetree. > > The hardware provides capability configuration registers for each SDHCI > in the global configuration space for the SD controller. Writes to the > global capability registers are mirrored to the capability registers in > the associated SDHCI. Configuration of the capabilities must be written > through the mirror registers prior to initialisation of the SDHCI. > ``` > Thanks for the exmaple, I will modify my commit message. > > > > Signed-off-by: Steven Lee > > --- > > drivers/mmc/host/sdhci-of-aspeed.c | 57 ++++++++++++++++++++++++++++++ > > 1 file changed, 57 insertions(+) > > > > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c > > b/drivers/mmc/host/sdhci-of-aspeed.c > > index d001c51074a0..4979f98ffb52 100644 > > --- a/drivers/mmc/host/sdhci-of-aspeed.c > > +++ b/drivers/mmc/host/sdhci-of-aspeed.c > > @@ -31,6 +31,11 @@ > > #define ASPEED_SDC_S0_PHASE_OUT_EN GENMASK(1, 0) > > #define ASPEED_SDC_PHASE_MAX 31 > > > > +/* SDIO{10,20} */ > > +#define ASPEED_SDC_CAP1_1_8V (0 * 32 + 26) > > +/* SDIO{14,24} */ > > +#define ASPEED_SDC_CAP2_SDR104 (1 * 32 + 1) > > + > > struct aspeed_sdc { > > struct clk *clk; > > struct resource *res; > > @@ -70,8 +75,42 @@ struct aspeed_sdhci { > > u32 width_mask; > > struct mmc_clk_phase_map phase_map; > > const struct aspeed_sdhci_phase_desc *phase_desc; > > + > > }; > > > > +/* > > + * The function sets the mirror register for updating > > + * capbilities of the current slot. > > + * > > + * slot | capability | caps_reg | mirror_reg > > + * -----|-------------|----------|------------ > > + * 0 | CAP1_1_8V | SDIO140 | SDIO10 > > + * 0 | CAP2_SDR104 | SDIO144 | SDIO14 > > + * 1 | CAP1_1_8V | SDIO240 | SDIO20 > > + * 1 | CAP2_SDR104 | SDIO244 | SDIO24 > > It would be nice to align the columns to improve readability. > Columns seems are aligned in my mail client(mutt) and my editor(vim). I paste the above comment in Notepad++, columns are aligned as well. > > +static void aspeed_sdc_set_slot_capability(struct sdhci_host *host, > > + struct aspeed_sdc *sdc, > > + int capability, > > + bool enable, > > + u8 slot) > > I prefer we don't take up so much vertical space here. I think this > could be just a couple of lines with multiple variables per line. We > can go to 100 chars per line. > I will change the function as the follows: static void aspeed_sdc_set_slot_capability(struct sdhci_host *host, struct aspeed_sdc *sdc, int capability, bool enable, u8 slot) > > +{ > > + u8 cap_reg; > > + u32 mirror_reg_offset, cap_val; > > The rest of the driver follows "reverse christmas tree" (longest to > shortest declaration) style, so I prefer we try to maintain consistency > where we can. Essentially, declare them in this order: > > u32 mirror_reg_offset; > u32 cap_val; > u8 cap_reg; > Will modify it. > > + > > + if (slot > 1) > > + return; > > + > > + cap_reg = capability / 32; > > + cap_val = sdhci_readl(host, 0x40 + (cap_reg * 4)); > > + if (enable) > > + cap_val |= BIT(capability % 32); > > + else > > + cap_val &= ~BIT(capability % 32); > > + mirror_reg_offset = ((slot + 1) * 0x10) + (cap_reg * 4); > > + writel(cap_val, sdc->regs + mirror_reg_offset); > > +} > > + > > static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc, > > struct aspeed_sdhci *sdhci, > > bool bus8) > > @@ -329,6 +368,7 @@ 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; > > Again here with the reverse-christmas-tree style, so: > > const struct aspeed_sdhci_pdata *aspeed_pdata; > struct device_node *np = pdev->dev.of_node; > struct sdhci_pltfm_host *pltfm_host; > ... > Will modify it. > > struct aspeed_sdhci *dev; > > struct sdhci_host *host; > > struct resource *res; > > @@ -372,6 +412,23 @@ 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, > > + true, > > + slot); > > Again, this would be nicer if we compress it to as few lines as possible. > Will modify the function as follows: aspeed_sdc_set_slot_capability(host, dev->parent, ASPEED_SDC_CAP1_1_8V, true, slot); > > + } > > + > > + if (of_property_read_bool(np, "sd-uhs-sdr104")) { > > + aspeed_sdc_set_slot_capability(host, > > + dev->parent, > > + ASPEED_SDC_CAP2_SDR104, > > + true, > > + slot); > > As above. > Will modify the function as follows: aspeed_sdc_set_slot_capability(host, dev->parent, ASPEED_SDC_CAP2_SDR104, true, slot); > Cheers, > > Andrew 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=-15.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 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 02990C433ED for ; Fri, 7 May 2021 07:00:32 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id BEB29613D6 for ; Fri, 7 May 2021 07:00:31 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235023AbhEGHB2 (ORCPT ); Fri, 7 May 2021 03:01:28 -0400 Received: from twspam01.aspeedtech.com ([211.20.114.71]:51122 "EHLO twspam01.aspeedtech.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231355AbhEGHB2 (ORCPT ); Fri, 7 May 2021 03:01:28 -0400 Received: from mail.aspeedtech.com ([192.168.0.24]) by twspam01.aspeedtech.com with ESMTP id 1476lSrP039572; Fri, 7 May 2021 14:47:28 +0800 (GMT-8) (envelope-from steven_lee@aspeedtech.com) Received: from aspeedtech.com (192.168.100.253) by TWMBX02.aspeed.com (192.168.0.24) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Fri, 7 May 2021 14:59:20 +0800 Date: Fri, 7 May 2021 14:59:18 +0800 From: Steven Lee To: Andrew Jeffery CC: Ulf Hansson , Rob Herring , Joel Stanley , Adrian Hunter , Philipp Zabel , Ryan Chen , "moderated list:ASPEED SD/MMC DRIVER" , "moderated list:ASPEED SD/MMC DRIVER" , linux-mmc , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , "moderated list:ARM/ASPEED MACHINE SUPPORT" , open list , Hongwei Zhang , Ryan Chen , Chin-Ting Kuo Subject: Re: [PATCH v3 4/5] mmc: sdhci-of-aspeed: Add a helper for updating capability register. Message-ID: <20210507065918.GE23749@aspeedtech.com> References: <20210506100312.1638-1-steven_lee@aspeedtech.com> <20210506100312.1638-5-steven_lee@aspeedtech.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) X-Originating-IP: [192.168.100.253] X-ClientProxiedBy: TWMBX02.aspeed.com (192.168.0.24) To TWMBX02.aspeed.com (192.168.0.24) X-DNSRBL: X-MAIL: twspam01.aspeedtech.com 1476lSrP039572 Precedence: bulk List-ID: X-Mailing-List: linux-mmc@vger.kernel.org The 05/07/2021 10:13, Andrew Jeffery wrote: > Hi Steven, > > I have some minor comments. I expect you're going to do a v4 of the > series, so if you'd like to clean them up in the process I'd appreciate > it. > Yes, I am going to prepare v4 patch for meeting reviewer's expectation including your comment in this patch. I've learned a lot from your suggestion for driver upstream. Many thanks! > However, from a pragmatic standpoint I think the patch is in good shape. > > On Thu, 6 May 2021, at 19:33, Steven Lee wrote: > > The patch add a new function aspeed_sdc_set_slot_capability() for > > updating sdhci capability register. > > The commit message should explain why the patch is necessary and not > what it does, as what it does is contained in the diff. > > It's okay to explain *how* the patch acheives its goals if the > implementation is subtle or complex. > > Maybe the commit message could be something like: > > > ``` > Configure the SDHCIs as specified by the devicetree. > > The hardware provides capability configuration registers for each SDHCI > in the global configuration space for the SD controller. Writes to the > global capability registers are mirrored to the capability registers in > the associated SDHCI. Configuration of the capabilities must be written > through the mirror registers prior to initialisation of the SDHCI. > ``` > Thanks for the exmaple, I will modify my commit message. > > > > Signed-off-by: Steven Lee > > --- > > drivers/mmc/host/sdhci-of-aspeed.c | 57 ++++++++++++++++++++++++++++++ > > 1 file changed, 57 insertions(+) > > > > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c > > b/drivers/mmc/host/sdhci-of-aspeed.c > > index d001c51074a0..4979f98ffb52 100644 > > --- a/drivers/mmc/host/sdhci-of-aspeed.c > > +++ b/drivers/mmc/host/sdhci-of-aspeed.c > > @@ -31,6 +31,11 @@ > > #define ASPEED_SDC_S0_PHASE_OUT_EN GENMASK(1, 0) > > #define ASPEED_SDC_PHASE_MAX 31 > > > > +/* SDIO{10,20} */ > > +#define ASPEED_SDC_CAP1_1_8V (0 * 32 + 26) > > +/* SDIO{14,24} */ > > +#define ASPEED_SDC_CAP2_SDR104 (1 * 32 + 1) > > + > > struct aspeed_sdc { > > struct clk *clk; > > struct resource *res; > > @@ -70,8 +75,42 @@ struct aspeed_sdhci { > > u32 width_mask; > > struct mmc_clk_phase_map phase_map; > > const struct aspeed_sdhci_phase_desc *phase_desc; > > + > > }; > > > > +/* > > + * The function sets the mirror register for updating > > + * capbilities of the current slot. > > + * > > + * slot | capability | caps_reg | mirror_reg > > + * -----|-------------|----------|------------ > > + * 0 | CAP1_1_8V | SDIO140 | SDIO10 > > + * 0 | CAP2_SDR104 | SDIO144 | SDIO14 > > + * 1 | CAP1_1_8V | SDIO240 | SDIO20 > > + * 1 | CAP2_SDR104 | SDIO244 | SDIO24 > > It would be nice to align the columns to improve readability. > Columns seems are aligned in my mail client(mutt) and my editor(vim). I paste the above comment in Notepad++, columns are aligned as well. > > +static void aspeed_sdc_set_slot_capability(struct sdhci_host *host, > > + struct aspeed_sdc *sdc, > > + int capability, > > + bool enable, > > + u8 slot) > > I prefer we don't take up so much vertical space here. I think this > could be just a couple of lines with multiple variables per line. We > can go to 100 chars per line. > I will change the function as the follows: static void aspeed_sdc_set_slot_capability(struct sdhci_host *host, struct aspeed_sdc *sdc, int capability, bool enable, u8 slot) > > +{ > > + u8 cap_reg; > > + u32 mirror_reg_offset, cap_val; > > The rest of the driver follows "reverse christmas tree" (longest to > shortest declaration) style, so I prefer we try to maintain consistency > where we can. Essentially, declare them in this order: > > u32 mirror_reg_offset; > u32 cap_val; > u8 cap_reg; > Will modify it. > > + > > + if (slot > 1) > > + return; > > + > > + cap_reg = capability / 32; > > + cap_val = sdhci_readl(host, 0x40 + (cap_reg * 4)); > > + if (enable) > > + cap_val |= BIT(capability % 32); > > + else > > + cap_val &= ~BIT(capability % 32); > > + mirror_reg_offset = ((slot + 1) * 0x10) + (cap_reg * 4); > > + writel(cap_val, sdc->regs + mirror_reg_offset); > > +} > > + > > static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc, > > struct aspeed_sdhci *sdhci, > > bool bus8) > > @@ -329,6 +368,7 @@ 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; > > Again here with the reverse-christmas-tree style, so: > > const struct aspeed_sdhci_pdata *aspeed_pdata; > struct device_node *np = pdev->dev.of_node; > struct sdhci_pltfm_host *pltfm_host; > ... > Will modify it. > > struct aspeed_sdhci *dev; > > struct sdhci_host *host; > > struct resource *res; > > @@ -372,6 +412,23 @@ 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, > > + true, > > + slot); > > Again, this would be nicer if we compress it to as few lines as possible. > Will modify the function as follows: aspeed_sdc_set_slot_capability(host, dev->parent, ASPEED_SDC_CAP1_1_8V, true, slot); > > + } > > + > > + if (of_property_read_bool(np, "sd-uhs-sdr104")) { > > + aspeed_sdc_set_slot_capability(host, > > + dev->parent, > > + ASPEED_SDC_CAP2_SDR104, > > + true, > > + slot); > > As above. > Will modify the function as follows: aspeed_sdc_set_slot_capability(host, dev->parent, ASPEED_SDC_CAP2_SDR104, true, slot); > Cheers, > > Andrew 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=-15.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 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 EA27BC433ED for ; Fri, 7 May 2021 07:00:44 +0000 (UTC) Received: from lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (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 1B5B261164 for ; Fri, 7 May 2021 07:00:44 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1B5B261164 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=aspeedtech.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=openbmc-bounces+openbmc=archiver.kernel.org@lists.ozlabs.org Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4Fc1Yk5g37z2ymS for ; Fri, 7 May 2021 17:00:42 +1000 (AEST) Authentication-Results: lists.ozlabs.org; spf=fail (SPF fail - not authorized) smtp.mailfrom=aspeedtech.com (client-ip=211.20.114.71; helo=twspam01.aspeedtech.com; envelope-from=steven_lee@aspeedtech.com; receiver=) Received: from twspam01.aspeedtech.com (twspam01.aspeedtech.com [211.20.114.71]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4Fc1YH47rxz2xg5; Fri, 7 May 2021 17:00:17 +1000 (AEST) Received: from mail.aspeedtech.com ([192.168.0.24]) by twspam01.aspeedtech.com with ESMTP id 1476lSrP039572; Fri, 7 May 2021 14:47:28 +0800 (GMT-8) (envelope-from steven_lee@aspeedtech.com) Received: from aspeedtech.com (192.168.100.253) by TWMBX02.aspeed.com (192.168.0.24) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Fri, 7 May 2021 14:59:20 +0800 Date: Fri, 7 May 2021 14:59:18 +0800 From: Steven Lee To: Andrew Jeffery Subject: Re: [PATCH v3 4/5] mmc: sdhci-of-aspeed: Add a helper for updating capability register. Message-ID: <20210507065918.GE23749@aspeedtech.com> References: <20210506100312.1638-1-steven_lee@aspeedtech.com> <20210506100312.1638-5-steven_lee@aspeedtech.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) X-Originating-IP: [192.168.100.253] X-ClientProxiedBy: TWMBX02.aspeed.com (192.168.0.24) To TWMBX02.aspeed.com (192.168.0.24) X-DNSRBL: X-MAIL: twspam01.aspeedtech.com 1476lSrP039572 X-BeenThere: openbmc@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Development list for OpenBMC List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , Ulf Hansson , Ryan Chen , "moderated list:ASPEED SD/MMC DRIVER" , linux-mmc , "moderated list:ASPEED SD/MMC DRIVER" , Ryan Chen , Adrian Hunter , Chin-Ting Kuo , open list , Rob Herring , Philipp Zabel , Hongwei Zhang , "moderated list:ARM/ASPEED MACHINE SUPPORT" Errors-To: openbmc-bounces+openbmc=archiver.kernel.org@lists.ozlabs.org Sender: "openbmc" The 05/07/2021 10:13, Andrew Jeffery wrote: > Hi Steven, > > I have some minor comments. I expect you're going to do a v4 of the > series, so if you'd like to clean them up in the process I'd appreciate > it. > Yes, I am going to prepare v4 patch for meeting reviewer's expectation including your comment in this patch. I've learned a lot from your suggestion for driver upstream. Many thanks! > However, from a pragmatic standpoint I think the patch is in good shape. > > On Thu, 6 May 2021, at 19:33, Steven Lee wrote: > > The patch add a new function aspeed_sdc_set_slot_capability() for > > updating sdhci capability register. > > The commit message should explain why the patch is necessary and not > what it does, as what it does is contained in the diff. > > It's okay to explain *how* the patch acheives its goals if the > implementation is subtle or complex. > > Maybe the commit message could be something like: > > > ``` > Configure the SDHCIs as specified by the devicetree. > > The hardware provides capability configuration registers for each SDHCI > in the global configuration space for the SD controller. Writes to the > global capability registers are mirrored to the capability registers in > the associated SDHCI. Configuration of the capabilities must be written > through the mirror registers prior to initialisation of the SDHCI. > ``` > Thanks for the exmaple, I will modify my commit message. > > > > Signed-off-by: Steven Lee > > --- > > drivers/mmc/host/sdhci-of-aspeed.c | 57 ++++++++++++++++++++++++++++++ > > 1 file changed, 57 insertions(+) > > > > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c > > b/drivers/mmc/host/sdhci-of-aspeed.c > > index d001c51074a0..4979f98ffb52 100644 > > --- a/drivers/mmc/host/sdhci-of-aspeed.c > > +++ b/drivers/mmc/host/sdhci-of-aspeed.c > > @@ -31,6 +31,11 @@ > > #define ASPEED_SDC_S0_PHASE_OUT_EN GENMASK(1, 0) > > #define ASPEED_SDC_PHASE_MAX 31 > > > > +/* SDIO{10,20} */ > > +#define ASPEED_SDC_CAP1_1_8V (0 * 32 + 26) > > +/* SDIO{14,24} */ > > +#define ASPEED_SDC_CAP2_SDR104 (1 * 32 + 1) > > + > > struct aspeed_sdc { > > struct clk *clk; > > struct resource *res; > > @@ -70,8 +75,42 @@ struct aspeed_sdhci { > > u32 width_mask; > > struct mmc_clk_phase_map phase_map; > > const struct aspeed_sdhci_phase_desc *phase_desc; > > + > > }; > > > > +/* > > + * The function sets the mirror register for updating > > + * capbilities of the current slot. > > + * > > + * slot | capability | caps_reg | mirror_reg > > + * -----|-------------|----------|------------ > > + * 0 | CAP1_1_8V | SDIO140 | SDIO10 > > + * 0 | CAP2_SDR104 | SDIO144 | SDIO14 > > + * 1 | CAP1_1_8V | SDIO240 | SDIO20 > > + * 1 | CAP2_SDR104 | SDIO244 | SDIO24 > > It would be nice to align the columns to improve readability. > Columns seems are aligned in my mail client(mutt) and my editor(vim). I paste the above comment in Notepad++, columns are aligned as well. > > +static void aspeed_sdc_set_slot_capability(struct sdhci_host *host, > > + struct aspeed_sdc *sdc, > > + int capability, > > + bool enable, > > + u8 slot) > > I prefer we don't take up so much vertical space here. I think this > could be just a couple of lines with multiple variables per line. We > can go to 100 chars per line. > I will change the function as the follows: static void aspeed_sdc_set_slot_capability(struct sdhci_host *host, struct aspeed_sdc *sdc, int capability, bool enable, u8 slot) > > +{ > > + u8 cap_reg; > > + u32 mirror_reg_offset, cap_val; > > The rest of the driver follows "reverse christmas tree" (longest to > shortest declaration) style, so I prefer we try to maintain consistency > where we can. Essentially, declare them in this order: > > u32 mirror_reg_offset; > u32 cap_val; > u8 cap_reg; > Will modify it. > > + > > + if (slot > 1) > > + return; > > + > > + cap_reg = capability / 32; > > + cap_val = sdhci_readl(host, 0x40 + (cap_reg * 4)); > > + if (enable) > > + cap_val |= BIT(capability % 32); > > + else > > + cap_val &= ~BIT(capability % 32); > > + mirror_reg_offset = ((slot + 1) * 0x10) + (cap_reg * 4); > > + writel(cap_val, sdc->regs + mirror_reg_offset); > > +} > > + > > static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc, > > struct aspeed_sdhci *sdhci, > > bool bus8) > > @@ -329,6 +368,7 @@ 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; > > Again here with the reverse-christmas-tree style, so: > > const struct aspeed_sdhci_pdata *aspeed_pdata; > struct device_node *np = pdev->dev.of_node; > struct sdhci_pltfm_host *pltfm_host; > ... > Will modify it. > > struct aspeed_sdhci *dev; > > struct sdhci_host *host; > > struct resource *res; > > @@ -372,6 +412,23 @@ 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, > > + true, > > + slot); > > Again, this would be nicer if we compress it to as few lines as possible. > Will modify the function as follows: aspeed_sdc_set_slot_capability(host, dev->parent, ASPEED_SDC_CAP1_1_8V, true, slot); > > + } > > + > > + if (of_property_read_bool(np, "sd-uhs-sdr104")) { > > + aspeed_sdc_set_slot_capability(host, > > + dev->parent, > > + ASPEED_SDC_CAP2_SDR104, > > + true, > > + slot); > > As above. > Will modify the function as follows: aspeed_sdc_set_slot_capability(host, dev->parent, ASPEED_SDC_CAP2_SDR104, true, slot); > Cheers, > > Andrew 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=-15.9 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, USER_AGENT_SANE_1 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 9FB30C433ED for ; Fri, 7 May 2021 07:02:49 +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 10F2B613CD for ; Fri, 7 May 2021 07:02:49 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 10F2B613CD Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=aspeedtech.com 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:In-Reply-To:MIME-Version:References:Message-ID: Subject:CC:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=OXm7vLPmwsVcC4mTcHKK8eCcU4JA7TzUg6FiagXbQK4=; b=Ez4kP6sY3e2ElAxehC6KVOwQs xL2WGbED76wBHQebNanhm2e+QVC7M6PuZYAIyEW9543e/33y3XNZ6QQEBYtnf2v7mm0mBl6amIehN dQc5QXO8WUduXt595iasbsB3UWUnnPHAvkk/CWnxJFmy2OWeA0o0as/7HF4tJnysuCre/kFbVXh26 ox4rOZfl1PJMwA3Vz7R+XMudjHLnlaze4KUIxfJGW5hYeW4JwbBzwUIsA/jAlpRoMUCimdkJYuiwb WHDhgMPVCbakp57mXY+eScfhAM5GZJ/q0yM/N6sKpRaFi/hWcylit8VzO8CPUylYdfY3wZ0Ix3IQa MLX6F+qBQ==; Received: from localhost ([::1] helo=desiato.infradead.org) by desiato.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1leuTS-006KHF-Ds; Fri, 07 May 2021 07:00:34 +0000 Received: from casper.infradead.org ([2001:8b0:10b:1236::1]) by desiato.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1leuT8-006KEB-0G for linux-arm-kernel@desiato.infradead.org; Fri, 07 May 2021 07:00:17 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:CC:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=PhDOedvIfzYXZ0SMvsT6yIHbA4AptRO8nxiQWwwYo/0=; b=c0uZHxpm1d/HLuRTEHhPy4b0i9 5gcuno/mYHgyw2nbZ1DzYf683tNOLM1FJoAFkSxXQFDZ5KKYdqcbw5ZJSkzrZ3IY9l+VQZz3GZbQS sbkzEXam7TRLfW6WRiAT0r+nrPjGexaWKJTuxzifNOt3OHQ8iXlBkedP9MDxi02R4VQTGc4jgwcWr McMpTi6fCREDIjGEmIUAW62Pl80jPrDFt8mcjEYDuPENaMrullJ57BmPZ5D3DZdLlrk3PQZSGVVHe sLTBKbsF9/9sjZYrn++xIjTsob+J+xx7p2FOL8CnrNFmL7xf/dLJXZHtFgLhOWr8tYeja44HoffGY FszzxO9A==; Received: from twspam01.aspeedtech.com ([211.20.114.71]) by casper.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1leuSr-002unB-ES for linux-arm-kernel@lists.infradead.org; Fri, 07 May 2021 07:00:07 +0000 Received: from mail.aspeedtech.com ([192.168.0.24]) by twspam01.aspeedtech.com with ESMTP id 1476lSrP039572; Fri, 7 May 2021 14:47:28 +0800 (GMT-8) (envelope-from steven_lee@aspeedtech.com) Received: from aspeedtech.com (192.168.100.253) by TWMBX02.aspeed.com (192.168.0.24) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Fri, 7 May 2021 14:59:20 +0800 Date: Fri, 7 May 2021 14:59:18 +0800 From: Steven Lee To: Andrew Jeffery CC: Ulf Hansson , Rob Herring , Joel Stanley , Adrian Hunter , Philipp Zabel , Ryan Chen , "moderated list:ASPEED SD/MMC DRIVER" , "moderated list:ASPEED SD/MMC DRIVER" , linux-mmc , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , "moderated list:ARM/ASPEED MACHINE SUPPORT" , open list , Hongwei Zhang , Ryan Chen , Chin-Ting Kuo Subject: Re: [PATCH v3 4/5] mmc: sdhci-of-aspeed: Add a helper for updating capability register. Message-ID: <20210507065918.GE23749@aspeedtech.com> References: <20210506100312.1638-1-steven_lee@aspeedtech.com> <20210506100312.1638-5-steven_lee@aspeedtech.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) X-Originating-IP: [192.168.100.253] X-ClientProxiedBy: TWMBX02.aspeed.com (192.168.0.24) To TWMBX02.aspeed.com (192.168.0.24) X-DNSRBL: X-MAIL: twspam01.aspeedtech.com 1476lSrP039572 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210507_080005_803889_20F20413 X-CRM114-Status: GOOD ( 37.27 ) 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 The 05/07/2021 10:13, Andrew Jeffery wrote: > Hi Steven, > > I have some minor comments. I expect you're going to do a v4 of the > series, so if you'd like to clean them up in the process I'd appreciate > it. > Yes, I am going to prepare v4 patch for meeting reviewer's expectation including your comment in this patch. I've learned a lot from your suggestion for driver upstream. Many thanks! > However, from a pragmatic standpoint I think the patch is in good shape. > > On Thu, 6 May 2021, at 19:33, Steven Lee wrote: > > The patch add a new function aspeed_sdc_set_slot_capability() for > > updating sdhci capability register. > > The commit message should explain why the patch is necessary and not > what it does, as what it does is contained in the diff. > > It's okay to explain *how* the patch acheives its goals if the > implementation is subtle or complex. > > Maybe the commit message could be something like: > > > ``` > Configure the SDHCIs as specified by the devicetree. > > The hardware provides capability configuration registers for each SDHCI > in the global configuration space for the SD controller. Writes to the > global capability registers are mirrored to the capability registers in > the associated SDHCI. Configuration of the capabilities must be written > through the mirror registers prior to initialisation of the SDHCI. > ``` > Thanks for the exmaple, I will modify my commit message. > > > > Signed-off-by: Steven Lee > > --- > > drivers/mmc/host/sdhci-of-aspeed.c | 57 ++++++++++++++++++++++++++++++ > > 1 file changed, 57 insertions(+) > > > > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c > > b/drivers/mmc/host/sdhci-of-aspeed.c > > index d001c51074a0..4979f98ffb52 100644 > > --- a/drivers/mmc/host/sdhci-of-aspeed.c > > +++ b/drivers/mmc/host/sdhci-of-aspeed.c > > @@ -31,6 +31,11 @@ > > #define ASPEED_SDC_S0_PHASE_OUT_EN GENMASK(1, 0) > > #define ASPEED_SDC_PHASE_MAX 31 > > > > +/* SDIO{10,20} */ > > +#define ASPEED_SDC_CAP1_1_8V (0 * 32 + 26) > > +/* SDIO{14,24} */ > > +#define ASPEED_SDC_CAP2_SDR104 (1 * 32 + 1) > > + > > struct aspeed_sdc { > > struct clk *clk; > > struct resource *res; > > @@ -70,8 +75,42 @@ struct aspeed_sdhci { > > u32 width_mask; > > struct mmc_clk_phase_map phase_map; > > const struct aspeed_sdhci_phase_desc *phase_desc; > > + > > }; > > > > +/* > > + * The function sets the mirror register for updating > > + * capbilities of the current slot. > > + * > > + * slot | capability | caps_reg | mirror_reg > > + * -----|-------------|----------|------------ > > + * 0 | CAP1_1_8V | SDIO140 | SDIO10 > > + * 0 | CAP2_SDR104 | SDIO144 | SDIO14 > > + * 1 | CAP1_1_8V | SDIO240 | SDIO20 > > + * 1 | CAP2_SDR104 | SDIO244 | SDIO24 > > It would be nice to align the columns to improve readability. > Columns seems are aligned in my mail client(mutt) and my editor(vim). I paste the above comment in Notepad++, columns are aligned as well. > > +static void aspeed_sdc_set_slot_capability(struct sdhci_host *host, > > + struct aspeed_sdc *sdc, > > + int capability, > > + bool enable, > > + u8 slot) > > I prefer we don't take up so much vertical space here. I think this > could be just a couple of lines with multiple variables per line. We > can go to 100 chars per line. > I will change the function as the follows: static void aspeed_sdc_set_slot_capability(struct sdhci_host *host, struct aspeed_sdc *sdc, int capability, bool enable, u8 slot) > > +{ > > + u8 cap_reg; > > + u32 mirror_reg_offset, cap_val; > > The rest of the driver follows "reverse christmas tree" (longest to > shortest declaration) style, so I prefer we try to maintain consistency > where we can. Essentially, declare them in this order: > > u32 mirror_reg_offset; > u32 cap_val; > u8 cap_reg; > Will modify it. > > + > > + if (slot > 1) > > + return; > > + > > + cap_reg = capability / 32; > > + cap_val = sdhci_readl(host, 0x40 + (cap_reg * 4)); > > + if (enable) > > + cap_val |= BIT(capability % 32); > > + else > > + cap_val &= ~BIT(capability % 32); > > + mirror_reg_offset = ((slot + 1) * 0x10) + (cap_reg * 4); > > + writel(cap_val, sdc->regs + mirror_reg_offset); > > +} > > + > > static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc, > > struct aspeed_sdhci *sdhci, > > bool bus8) > > @@ -329,6 +368,7 @@ 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; > > Again here with the reverse-christmas-tree style, so: > > const struct aspeed_sdhci_pdata *aspeed_pdata; > struct device_node *np = pdev->dev.of_node; > struct sdhci_pltfm_host *pltfm_host; > ... > Will modify it. > > struct aspeed_sdhci *dev; > > struct sdhci_host *host; > > struct resource *res; > > @@ -372,6 +412,23 @@ 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, > > + true, > > + slot); > > Again, this would be nicer if we compress it to as few lines as possible. > Will modify the function as follows: aspeed_sdc_set_slot_capability(host, dev->parent, ASPEED_SDC_CAP1_1_8V, true, slot); > > + } > > + > > + if (of_property_read_bool(np, "sd-uhs-sdr104")) { > > + aspeed_sdc_set_slot_capability(host, > > + dev->parent, > > + ASPEED_SDC_CAP2_SDR104, > > + true, > > + slot); > > As above. > Will modify the function as follows: aspeed_sdc_set_slot_capability(host, dev->parent, ASPEED_SDC_CAP2_SDR104, true, slot); > Cheers, > > Andrew _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel