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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 555D2C433F5 for ; Thu, 24 Mar 2022 10:50:18 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S242718AbiCXKvs (ORCPT ); Thu, 24 Mar 2022 06:51:48 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56980 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S244486AbiCXKvr (ORCPT ); Thu, 24 Mar 2022 06:51:47 -0400 Received: from mail-pj1-x102d.google.com (mail-pj1-x102d.google.com [IPv6:2607:f8b0:4864:20::102d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 65B735676C for ; Thu, 24 Mar 2022 03:50:13 -0700 (PDT) Received: by mail-pj1-x102d.google.com with SMTP id gp15-20020a17090adf0f00b001c7cd11b0b3so249266pjb.3 for ; Thu, 24 Mar 2022 03:50:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-disposition:in-reply-to; bh=qGDumVMKawy+/ZRxQGLEQJDcVNYjmnOQHzc6Mj/c7AU=; b=wM1aK6Pz0kNGBbuyv5Usfrj7qDY4xh6wn39lJ9n1IbMvVTOJyg44Fs0+ojCVKRtP3L F9fzEvHxNbuBhXSB+BrulgAUM4ho1TNLB0zcboe8jAMSt8+oMKlbOk/XsI9NJPIN4En9 A9YWELK/8M09HrhI29X5OFR7iS1nUehscPFwt3YhCkhyZWVgInp+FrPuO4b2rSEfYelq bX2IXkkIZMDTaVOrGH/C1vA+Gx2jlAiwyZ0Uzq2APZNryREX8xlOnwrRSeu3dFmqMOBw Q9dFOmzIFhBD2jGmseRgkOveLstdPQJS/pYskO+OS4sSN4sa7yaE6mJt4Zsgb6UUKJZc l6ig== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :in-reply-to; bh=qGDumVMKawy+/ZRxQGLEQJDcVNYjmnOQHzc6Mj/c7AU=; b=UO54MoDfEdIq45tnjbnwYQdx63TAiMyoi6pzsKNpRgEArRoYTUEp9R2EAo4ctK/ehl zLjCGQ0bfC7Xr/S9rVzUcDClINHfoyyXu+aQ7uTS03aasF16Qhb7k7xT8aw6YvPJ6TvT 253tn8zb/94PZw3NjnXu+ZEI1qJcDOyQ3f+GinDXgJj7aVbc8KCwf8al/SL24mSK+BzK 5EWAF45B6NnTvPQzclXGlQz07z+ags45FvAA+MErSlQz7eO9YMuQQjXByzrGMGTLazfR XFj7/0eMK3hSZMIet/bwla/vR5kUvaigi/FTXmRDRC1/gZR9k2wXe5t5PKbYUZ8SOZeh QhTw== X-Gm-Message-State: AOAM530Gtqj2ZSimPo31JWr+VffslM0adJsdoNmU4MH2Yv6lw4p7UsOC HRfkdsV/nkqHvF2pTQoD2G9s8L8QeZ81z/an X-Google-Smtp-Source: ABdhPJw8as6JCG1Oop0KW15LlseFmKJIiRsmRcSXTRbr+TFsJxq3/lpXeGEzQkAF4TYokTVk0IR5bg== X-Received: by 2002:a17:902:e812:b0:154:1e0a:ca3b with SMTP id u18-20020a170902e81200b001541e0aca3bmr5194357plg.24.1648119012544; Thu, 24 Mar 2022 03:50:12 -0700 (PDT) Received: from laputa ([2400:4050:c3e1:100:7d3a:4634:c891:518]) by smtp.gmail.com with ESMTPSA id kk11-20020a17090b4a0b00b001c73933d803sm9456897pjb.10.2022.03.24.03.50.09 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 24 Mar 2022 03:50:12 -0700 (PDT) Date: Thu, 24 Mar 2022 19:50:07 +0900 From: AKASHI Takahiro To: Ulf Hansson Cc: Jason Lai , adrian.hunter@intel.com, linux-mmc@vger.kernel.org, dlunev@chromium.org, ben.chuang@genesyslogic.com.tw, greg.tu@genesyslogic.com.tw, Jason.Lai@genesyslogic.com.tw, otis.wu@genesyslogic.com.tw Subject: Re: [PATCH V3 6/7] mmc: Implement content of UHS-II card initialization functions Message-ID: <20220324105007.GB13268@laputa> Mail-Followup-To: AKASHI Takahiro , Ulf Hansson , Jason Lai , adrian.hunter@intel.com, linux-mmc@vger.kernel.org, dlunev@chromium.org, ben.chuang@genesyslogic.com.tw, greg.tu@genesyslogic.com.tw, Jason.Lai@genesyslogic.com.tw, otis.wu@genesyslogic.com.tw References: <20220222033931.237638-1-jasonlai.genesyslogic@gmail.com> <20220222033931.237638-7-jasonlai.genesyslogic@gmail.com> <20220324012929.GA6955@laputa> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-mmc@vger.kernel.org On Thu, Mar 24, 2022 at 11:22:16AM +0100, Ulf Hansson wrote: > On Thu, 24 Mar 2022 at 02:29, AKASHI Takahiro > wrote: > > > > On Wed, Mar 23, 2022 at 05:15:59PM +0100, Ulf Hansson wrote: > > > On Tue, 22 Feb 2022 at 04:40, Jason Lai wrote: > > > > > > > > From: Jason Lai > > > > > > > > UHS-II card initialization flow is divided into 2 categories: PHY & Card. > > > > Part 1 - PHY Initialization: > > > > Every host controller may need their own avtivation operation to > > > > establish LINK between controller and card. So we add a new member > > > > function(uhs2_detect_init) in struct mmc_host_ops for host > > > > controller use. > > > > Part 2 - Card Initialization: > > > > This part can be divided into 6 substeps. > > > > 1. Send UHS-II CCMD DEVICE_INIT to card. > > > > 2. Send UHS-II CCMD ENUMERATE to card. > > > > 3. Send UHS-II Native Read CCMD to obtain capabilities in CFG_REG > > > > of card. > > > > 4. Host compares capabilities of host controller and card, then > > > > write the negotiated values to Setting field in CFG_REG of card > > > > through UHS-II Native Write CCMD. > > > > 5. Switch host controller's clock to Range B if it is supported by > > > > both host controller and card. > > > > 6. Execute legacy SD initialization flow. > > > > Part 3 - Provide a function to tranaform legacy SD command packet into > > > > UHS-II SD-TRAN DCMD packet. > > > > > > > > Most of the code added above came from Intel's original patch[3]. > > > > > > > > [3] > > > > https://patchwork.kernel.org/project/linux-mmc/patch/1419672479-30852-2- > > > > git-send-email-yi.y.sun@intel.com/ > > > > To honor the original work, we should add Intel's copyright notice here > > as I did before. > > I think Jason already did, at least for some of the files in this series. > > Note also that, the initial code that was posted, is very far from > what code that is going to be merged. Simply because the quality was > very poor and not acceptable for the upstream kernel. I don't think so. I don't see much difference from my modified version[1] which is also based on Intel's original work. [1] https://www.spinics.net/lists/linux-mmc/msg57321.html -Takahiro Akashi > That said, I am > not sure we need to keep the copyrights for this, but I leave that > call to Jason to decide. > > > > > -Takahiro Akashi > > > > Kind regards > Uffe > > > > > > > Signed-off-by: Jason Lai > > > > --- > > > > drivers/mmc/core/sd_uhs2.c | 835 ++++++++++++++++++++++++++++++++++++- > > > > 1 file changed, 817 insertions(+), 18 deletions(-) > > > > > > > > diff --git a/drivers/mmc/core/sd_uhs2.c b/drivers/mmc/core/sd_uhs2.c > > > > index 800957f74632..f1e8e30301eb 100644 > > > > --- a/drivers/mmc/core/sd_uhs2.c > > > > +++ b/drivers/mmc/core/sd_uhs2.c > > > > @@ -3,6 +3,7 @@ > > > > * Copyright (C) 2021 Linaro Ltd > > > > * > > > > * Author: Ulf Hansson > > > > + * Author: Jason Lai > > > > * > > > > * Support for SD UHS-II cards > > > > */ > > > > @@ -10,19 +11,31 @@ > > > > > > > > #include > > > > #include > > > > +#include > > > > +#include > > > > > > > > #include "core.h" > > > > #include "bus.h" > > > > +#include "card.h" > > > > #include "sd.h" > > > > +#include "sd_ops.h" > > > > #include "mmc_ops.h" > > > > +#include "sd_uhs2.h" > > > > > > > > static const unsigned int sd_uhs2_freqs[] = { 52000000, 26000000 }; > > > > > > > > static int sd_uhs2_set_ios(struct mmc_host *host) > > > > { > > > > struct mmc_ios *ios = &host->ios; > > > > + int err = 0; > > > > > > > > - return host->ops->uhs2_set_ios(host, ios); > > > > + pr_debug("%s: clock %uHz powermode %u Vdd %u timing %u\n", > > > > + mmc_hostname(host), ios->clock, ios->power_mode, ios->vdd, > > > > + ios->timing); > > > > + > > > > + host->ops->set_ios(host, ios); > > > > > > We discussed using the ->set_ios() callback in a previous version. To > > > repeat myself, I don't think it's a good idea. UHS-II needs an > > > entirely different power sequence than the legacy interface(s), hence > > > I think it's simply cleaner to separate them. > > > > > > To move forward, I see two options. > > > 1) Use only the ->uhs2_host_operation() ops. > > > 2) Use a combination of the ->uhs2_set_ios() ops and the > > > ->uhs2_host_operation() ops. > > > > > > Both options work for me. However, perhaps if you could incorporate > > > the changes done on the host driver at next submission, it becomes > > > easier for me to understand what makes best sense. > > > > > > > + > > > > + return err; > > > > } > > > > > > > > static int sd_uhs2_power_up(struct mmc_host *host) > > > > @@ -45,6 +58,43 @@ static void sd_uhs2_power_off(struct mmc_host *host) > > > > sd_uhs2_set_ios(host); > > > > } > > > > > > [...] > > > > > > > > > > > /* > > > > @@ -61,6 +119,77 @@ static int sd_uhs2_phy_init(struct mmc_host *host) > > > > */ > > > > static int sd_uhs2_dev_init(struct mmc_host *host) > > > > { > > > > + struct mmc_command cmd = {0}; > > > > + struct uhs2_command uhs2_cmd = {}; > > > > + u32 cnt; > > > > + u32 dap, gap, resp_gap; > > > > + u16 header = 0, arg = 0; > > > > > > No need to initiate these. > > > > > > > + u32 payload[1]; > > > > > > u32? > > > > > > > + u8 plen = 1; > > > > + u8 gd = 0, cf = 1; > > > > + u8 resp[6] = {0}; > > > > + u8 resp_len = 6; > > > > > > Many of these variables are just constant numbers. If it makes sense > > > to add definitions for them, then please do that instead. If not, just > > > give the value directly in the code. > > > > > > For example: plen = 1; (I assume that is payload length). This can > > > just be given as an in-parameter to sd_uhs2_cmd_assemble(), without > > > further explanation. > > > > > > The point is, sd_uhs2_cmd_assemble() should have a well described > > > description of its in-parameters, so no need for further descriptions, > > > I think. > > > > > > This comment applies to all the new code/functions that are added in > > > the $subject patch. Please go through all of the code and fix this. > > > > > > > > > > + int err; > > > > + > > > > + dap = host->uhs2_caps.dap; > > > > + gap = host->uhs2_caps.gap; > > > > + > > > > + header = UHS2_NATIVE_PACKET | UHS2_PACKET_TYPE_CCMD; > > > > + arg = ((UHS2_DEV_CMD_DEVICE_INIT & 0xFF) << 8) | > > > > + UHS2_NATIVE_CMD_WRITE | > > > > + UHS2_NATIVE_CMD_PLEN_4B | > > > > + (UHS2_DEV_CMD_DEVICE_INIT >> 8); > > > > + > > > > + /* > > > > + * Refer to UHS-II Addendum Version 1.02 section 6.3.1. > > > > + * Max. time from DEVICE_INIT CCMD EOP reception on Device > > > > + * Rx to its SOP transmission on Device Tx(Tfwd_init_cmd) is > > > > + * 1 second. > > > > + */ > > > > + cmd.busy_timeout = 1000; > > > > + > > > > + /* > > > > + * Refer to UHS-II Addendum Version 1.02 section 6.2.6.3. > > > > + * When the number of the DEVICE_INIT commands is reach to > > > > + * 30 tiems, Host shall stop issuing DEVICE_INIT command > > > > + * and regard it as an error. > > > > + */ > > > > + for (cnt = 0; cnt < 30; cnt++) { > > > > + payload[0] = ((dap & 0xF) << 12) | > > > > + (cf << 11) | > > > > + ((gd & 0xF) << 4) | > > > > + (gap & 0xF); > > > > > > To me, it looks like the payload data deserves to be explained a bit. > > > Perhaps you can add a comment explaining what pieces it consists of so > > > this becomes more clear? > > > > > > > + > > > > + sd_uhs2_cmd_assemble(&cmd, &uhs2_cmd, header, arg, payload, plen, resp, resp_len); > > > > + > > > > + err = mmc_wait_for_cmd(host, &cmd, 0); > > > > + > > > > + if (err) { > > > > + pr_err("%s: %s: UHS2 CMD send fail, err= 0x%x!\n", > > > > + mmc_hostname(host), __func__, err); > > > > + return -EIO; > > > > > > Why do you override the original error code that was returned from > > > mmc_wait_for_cmd()? > > > > > > Normally it's preferred to keep the error code, unless there is good > > > reason not to. > > > > > > Again, I won't add more comments like this in the code from the > > > $subject patch. But please go through it all to avoid this kind of > > > thing. > > > > > > > + } > > > > + > > > > + if (resp[3] != (UHS2_DEV_CMD_DEVICE_INIT & 0xFF)) { > > > > + pr_err("%s: DEVICE_INIT response is wrong!\n", > > > > + mmc_hostname(host)); > > > > + return -EIO; > > > > + } > > > > + > > > > + if (resp[5] & 0x8) { > > > > + host->uhs2_caps.group_desc = gd; > > > > + break; > > > > > > I suggest you do a return 0 here. In this way you can skip the check > > > "if (cnt == 30)" below and just return an error code instead. > > > > > > > + } > > > > + resp_gap = resp[4] & 0x0F; > > > > + if (gap == resp_gap) > > > > + gd++; > > > > + } > > > > + if (cnt == 30) { > > > > + pr_err("%s: DEVICE_INIT fail, already 30 times!\n", > > > > + mmc_hostname(host)); > > > > + return -EIO; > > > > + } > > > > + > > > > return 0; > > > > } > > > > > > > > > > > static int sd_uhs2_config_read(struct mmc_host *host, struct mmc_card *card) > > > > { > > > > + struct mmc_command cmd = {0}; > > > > + struct uhs2_command uhs2_cmd = {}; > > > > + u16 header = 0, arg = 0; > > > > + u32 cap; > > > > + int err; > > > > + > > > > + header = UHS2_NATIVE_PACKET | > > > > + UHS2_PACKET_TYPE_CCMD | > > > > + card->uhs2_config.node_id; > > > > + arg = ((UHS2_DEV_CONFIG_GEN_CAPS & 0xFF) << 8) | > > > > + UHS2_NATIVE_CMD_READ | > > > > + UHS2_NATIVE_CMD_PLEN_4B | > > > > + (UHS2_DEV_CONFIG_GEN_CAPS >> 8); > > > > + > > > > + /* There is no payload because per spec, there should be > > > > + * no payload field for read CCMD. > > > > + * Plen is set in arg. Per spec, plen for read CCMD > > > > + * represents the len of read data which is assigned in payload > > > > + * of following RES (p136). > > > > + */ > > > > + sd_uhs2_cmd_assemble(&cmd, &uhs2_cmd, header, arg, NULL, 0, NULL, 0); > > > > > > We are reading the configuration data here and onwards, piece by > > > piece. Perhaps if you can add a small comment about each piece we are > > > reading, before each call to mmc_wait_for_cmd(), that can help to > > > easier understand what goes on. > > > > > > [...] > > > > > > > static int sd_uhs2_config_write(struct mmc_host *host, struct mmc_card *card) > > > > { > > > > + struct mmc_command cmd = {0}; > > > > + struct uhs2_command uhs2_cmd = {}; > > > > + u16 header = 0, arg = 0; > > > > + u32 payload[2]; > > > > + u8 nMinDataGap; > > > > + u8 plen; > > > > + int err; > > > > + u8 resp[5] = {0}; > > > > + u8 resp_len = 5; > > > > + > > > > + header = UHS2_NATIVE_PACKET | > > > > + UHS2_PACKET_TYPE_CCMD | card->uhs2_config.node_id; > > > > + arg = ((UHS2_DEV_CONFIG_GEN_SET & 0xFF) << 8) | > > > > + UHS2_NATIVE_CMD_WRITE | > > > > + UHS2_NATIVE_CMD_PLEN_8B | > > > > + (UHS2_DEV_CONFIG_GEN_SET >> 8); > > > > + > > > > + if (card->uhs2_config.n_lanes == UHS2_DEV_CONFIG_2L_HD_FD && > > > > + host->uhs2_caps.n_lanes == UHS2_DEV_CONFIG_2L_HD_FD) { > > > > + /* Support HD */ > > > > + host->uhs2_caps.flags |= MMC_UHS2_2L_HD; > > > > > > How is the uhs2_caps.flags field intended to be used? To me it looks > > > like a way for the mmc core to exchange status/configuration > > > information about the initialization process of the card, with the mmc > > > host driver. Perhaps there is more too. Is that correct? > > > > > > If so, I think it looks quite similar for what we have in the struct > > > mmc_ios, for the legacy interface(s). I am not saying we should use > > > that, just trying to understand what would suit best here. > > > > > > > + nMinDataGap = 1; > > > > + } else { > > > > + /* Only support 2L-FD so far */ > > > > + host->uhs2_caps.flags &= ~MMC_UHS2_2L_HD; > > > > + nMinDataGap = 3; > > > > + } > > > > + > > > > + /* > > > > + * Most UHS-II cards only support FD and 2L-HD mode. Other lane numbers > > > > + * defined in UHS-II addendem Ver1.01 are optional. > > > > + */ > > > > + host->uhs2_caps.n_lanes_set = UHS2_DEV_CONFIG_GEN_SET_2L_FD_HD; > > > > + card->uhs2_config.n_lanes_set = UHS2_DEV_CONFIG_GEN_SET_2L_FD_HD; > > > > > > [...] > > > > > > > +static int sd_uhs2_go_dormant(struct mmc_host *host, bool hibernate, u32 node_id) > > > > +{ > > > > > > Looks like the in-parameter "hibernate" is superfluous, as it's always > > > set to "false" by the caller. > > > > > > > + struct mmc_command cmd = {0}; > > > > + struct uhs2_command uhs2_cmd = {}; > > > > + u16 header = 0, arg = 0; > > > > + u32 payload[1]; > > > > + u8 plen = 1; > > > > + int err; > > > > + > > > > + /* Disable Normal INT */ > > > > + if (!host->ops->uhs2_host_operation(host, UHS2_DISABLE_INT)) { > > > > + pr_err("%s: %s: UHS2 DISABLE_INT fail!\n", > > > > + mmc_hostname(host), __func__); > > > > + return -EIO; > > > > + } > > > > + > > > > + header = UHS2_NATIVE_PACKET | UHS2_PACKET_TYPE_CCMD | node_id; > > > > + > > > > + arg = ((UHS2_DEV_CMD_GO_DORMANT_STATE & 0xFF) << 8) | > > > > + UHS2_NATIVE_CMD_WRITE | > > > > + UHS2_NATIVE_CMD_PLEN_4B | > > > > + (UHS2_DEV_CMD_GO_DORMANT_STATE >> 8); > > > > + > > > > + if (hibernate) > > > > + payload[0] = UHS2_DEV_CMD_DORMANT_HIBER; > > > > + > > > > + sd_uhs2_cmd_assemble(&cmd, &uhs2_cmd, header, arg, payload, plen, NULL, 0); > > > > + > > > > + err = mmc_wait_for_cmd(host, &cmd, 0); > > > > + if (err) { > > > > + pr_err("%s: %s: UHS2 CMD send fail, err= 0x%x!\n", > > > > + mmc_hostname(host), __func__, err); > > > > + return -EIO; > > > > + } > > > > + > > > > + /* Check Dormant State in Present */ > > > > + if (!host->ops->uhs2_host_operation(host, UHS2_CHECK_DORMANT)) { > > > > + pr_err("%s: %s: UHS2 GO_DORMANT_STATE fail!\n", > > > > + mmc_hostname(host), __func__); > > > > + return -EIO; > > > > + } > > > > + > > > > + host->ops->uhs2_host_operation(host, UHS2_DISABLE_CLK); > > > > + > > > > return 0; > > > > } > > > > > > > > +static int sd_uhs2_change_speed(struct mmc_host *host, u32 node_id) > > > > +{ > > > > + struct mmc_command cmd = {0}; > > > > + struct uhs2_command uhs2_cmd = {}; > > > > + u16 header = 0, arg = 0; > > > > + int err; > > > > + int timeout = 100; > > > > + > > > > + /* Change Speed Range at controller side. */ > > > > + if (!host->ops->uhs2_host_operation(host, UHS2_SET_SPEED_B)) { > > > > + pr_err("%s: %s: UHS2 SET_SPEED fail!\n", > > > > + mmc_hostname(host), __func__); > > > > + return -EIO; > > > > + } > > > > + > > > > + err = sd_uhs2_go_dormant(host, false, node_id); > > > > + if (err) { > > > > + pr_err("%s: %s: UHS2 GO_DORMANT_STATE fail, err= 0x%x!\n", > > > > + mmc_hostname(host), __func__, err); > > > > + return -EIO; > > > > + } > > > > + > > > > + /* restore sd clock */ > > > > + mmc_delay(5); > > > > + host->ops->uhs2_host_operation(host, UHS2_ENABLE_CLK); > > > > > > I think the code can be a bit better structured here. More precisely, > > > since sd_uhs2_go_dormant() is the one that calls > > > ->uhs2_host_operation(host, UHS2_DISABLE_INT) and > > > ->uhs2_host_operation(host, UHS2_DISABLE_CLK), it's then up to > > > sd_uhs2_change_speed() to restore these changes. > > > > > > To me, it would be more clear if both enabling and disabling of the > > > clock /interrupt are managed in sd_uhs2_change_speed(). > > > > > > > + > > > > + /* Enable Normal INT */ > > > > + if (!host->ops->uhs2_host_operation(host, UHS2_ENABLE_INT)) { > > > > + pr_err("%s: %s: UHS2 ENABLE_INT fail!\n", > > > > + mmc_hostname(host), __func__); > > > > + return -EIO; > > > > + } > > > > + > > > > + /* > > > > + * According to UHS-II Addendum Version 1.01, chapter 6.2.3, wait card > > > > + * switch to Active State > > > > + */ > > > > + header = UHS2_NATIVE_PACKET | UHS2_PACKET_TYPE_CCMD | node_id; > > > > + arg = ((UHS2_DEV_CONFIG_GEN_SET & 0xFF) << 8) | > > > > + UHS2_NATIVE_CMD_READ | > > > > + UHS2_NATIVE_CMD_PLEN_8B | > > > > + (UHS2_DEV_CONFIG_GEN_SET >> 8); > > > > + do { > > > > + sd_uhs2_cmd_assemble(&cmd, &uhs2_cmd, header, arg, NULL, 0, NULL, 0); > > > > + err = mmc_wait_for_cmd(host, &cmd, 0); > > > > + if (err) { > > > > + pr_err("%s: %s: UHS2 CMD send fail, err= 0x%x!\n", > > > > + mmc_hostname(host), __func__, err); > > > > + return -EIO; > > > > + } > > > > + > > > > + if (cmd.resp[1] & UHS2_DEV_CONFIG_GEN_SET_CFG_COMPLETE) > > > > + break; > > > > + > > > > + timeout--; > > > > + if (timeout == 0) { > > > > + pr_err("%s: %s: Not switch to Active in 100 ms\n", > > > > + mmc_hostname(host), __func__); > > > > + return -EIO; > > > > + } > > > > + > > > > + mmc_delay(1); > > > > + } while (1); > > > > > > We really want to avoid these kinds of polling loops, for several > > > reasons. Please convert into using __mmc_poll_for_busy() instead. > > > > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int sd_uhs2_get_ro(struct mmc_host *host) > > > > +{ > > > > + int ro; > > > > + > > > > + /* > > > > + * Some systems don't feature a write-protect pin and don't need one. > > > > + * E.g. because they only have micro-SD card slot. For those systems > > > > + * assume that the SD card is always read-write. > > > > + */ > > > > + if (host->caps2 & MMC_CAP2_NO_WRITE_PROTECT) > > > > + return 0; > > > > + > > > > + if (!host->ops->get_ro) > > > > + return -1; > > > > + > > > > + ro = host->ops->get_ro(host); > > > > + > > > > + return ro; > > > > > > This can be replaced with mmc_sd_get_ro(). Let's avoid the open coding > > > and make that function being shared instead. > > > > > > > +} > > > > + > > > > /* > > > > * Initialize the UHS-II card through the SD-TRAN transport layer. This enables > > > > * commands/requests to be backwards compatible through the legacy SD protocol. > > > > @@ -107,9 +696,127 @@ static int sd_uhs2_config_write(struct mmc_host *host, struct mmc_card *card) > > > > */ > > > > static int sd_uhs2_legacy_init(struct mmc_host *host, struct mmc_card *card) > > > > { > > > > + int err; > > > > + u32 cid[4]; > > > > + u32 ocr; > > > > + u32 rocr = 0; > > > > + int ro; > > > > + > > > > + WARN_ON(!host->claimed); > > > > > > Drop this, it's an internal function, we should know that the host is > > > claimed before calling sd_uhs2_legacy_init(). > > > > > > > + > > > > + /* Send CMD0 to reset SD card */ > > > > + mmc_go_idle(host); > > > > + > > > > + /* Send CMD8 to communicate SD interface operation condition */ > > > > + err = mmc_send_if_cond(host, host->ocr_avail); > > > > + if (err) { > > > > + pr_err("%s: %s: SEND_IF_COND fail!\n", > > > > + mmc_hostname(host), __func__); > > > > > > Please drop these prints for every command/operation that fails. We > > > already have trace/debug options for commands/requests. > > > > > > This applies to all the below code as well (perhaps there are few > > > cases not covered by the existing trace/debug support, those may be > > > converted to pr_debug(). > > > > > > > + return err; > > > > + } > > > > + > > > > + /* > > > > + * Probe SD card working voltage. > > > > + */ > > > > + err = mmc_send_app_op_cond(host, 0, &ocr); > > > > + if (err) { > > > > + pr_err("%s: %s: SD_SEND_OP_COND fail!\n", > > > > + mmc_hostname(host), __func__); > > > > + return err; > > > > + } > > > > + card->ocr = ocr; > > > > + > > > > + /* > > > > + * Some SD cards claims an out of spec VDD voltage range. Let's treat > > > > + * these bits as being in-valid and especially also bit7. > > > > + */ > > > > + ocr &= ~0x7FFF; > > > > + rocr = mmc_select_voltage(host, ocr); > > > > > > If the host has MMC_CAP2_FULL_PWR_CYCLE set, mmc_select_voltage() may > > > end up calling mmc_power_cycle(). This is not going to work for > > > UHS-II. > > > > > > Either we need to modify mmc_select_voltage() so it becomes aware that > > > it can be called for UHS-II initialization, allowing it to avoid the > > > path to mmc_power_cycle() - or simply open code the part from > > > mmc_select_voltage() for UHS-II here. I think I prefer the latter. > > > > > > > + > > > > + /* > > > > + * Some cards have zero value of rocr in UHS-II mode. Assign host's > > > > + * ocr value to rocr. > > > > + */ > > > > + if (!rocr) { > > > > + if (host->ocr_avail) { > > > > + rocr = host->ocr_avail; > > > > > > host->ocr_avail should really be checked in when the host driver calls > > > mmc_add_host(). It must not be zero, then we should let mmc_add_host() > > > return an error code. I look into this and send a patch for this > > > separately. > > > > > > In other words, you should not need to check it here, but just trust that's set. > > > > > > > + } else { > > > > + pr_err("%s: %s: there is no valid OCR.\n", > > > > + mmc_hostname(host), __func__); > > > > + return -EINVAL; > > > > + } > > > > + } > > > > + > > > > + /* Wait SD power on ready */ > > > > + ocr = rocr; > > > > + err = mmc_send_app_op_cond(host, ocr, &rocr); > > > > + if (err) { > > > > + pr_err("%s: %s: SD_SEND_OP_COND fail!\n", mmc_hostname(host), > > > > + __func__); > > > > + return err; > > > > + } > > > > + > > > > + err = mmc_send_cid(host, cid); > > > > + if (err) { > > > > + pr_err("%s: %s: SD_SEND_CID fail!\n", mmc_hostname(host), > > > > + __func__); > > > > + return err; > > > > + } > > > > + memcpy(card->raw_cid, cid, sizeof(card->raw_cid)); > > > > + > > > > + /* > > > > + * Call the optional HC's init_card function to handle quirks. > > > > + */ > > > > + if (host->ops->init_card) > > > > + host->ops->init_card(host, card); > > > > > > This can be removed, as it's only for the legacy interface, I think. > > > > > > > + > > > > + /* > > > > + * For native busses: get card RCA and quit open drain mode. > > > > + */ > > > > + err = mmc_send_relative_addr(host, &card->rca); > > > > + if (err) { > > > > + pr_err("%s: %s: SD_SEND_RCA fail!\n", mmc_hostname(host), > > > > + __func__); > > > > + return err; > > > > + } > > > > + > > > > + err = mmc_sd_get_csd(card); > > > > + if (err) { > > > > + pr_err("%s: %s: SD_SEND_CSD fail!\n", mmc_hostname(host), > > > > + __func__); > > > > + return err; > > > > + } > > > > + > > > > + /* > > > > + * Select card, as all following commands rely on that. > > > > + */ > > > > + err = mmc_select_card(card); > > > > + if (err) { > > > > + pr_err("%s: %s: SD_SEL_DSEL fail!\n", mmc_hostname(host), > > > > + __func__); > > > > + return err; > > > > + } > > > > + > > > > + /* > > > > + * Check if read-only switch is active. > > > > + */ > > > > + ro = sd_uhs2_get_ro(host); > > > > + if (ro < 0) { > > > > + pr_warn("%s: host does not support read-only switch, assuming write-enable\n", > > > > + mmc_hostname(host)); > > > > + } else if (ro > 0) { > > > > + mmc_card_set_readonly(card); > > > > + } > > > > + > > > > return 0; > > > > } > > > > > > > > +static void sd_uhs2_remove(struct mmc_host *host) > > > > +{ > > > > + mmc_remove_card(host->card); > > > > + host->card = NULL; > > > > +} > > > > + > > > > /* > > > > * Allocate the data structure for the mmc_card and run the UHS-II specific > > > > * initialization sequence. > > > > @@ -121,16 +828,21 @@ static int sd_uhs2_init_card(struct mmc_host *host) > > > > int err; > > > > > > > > err = sd_uhs2_dev_init(host); > > > > - if (err) > > > > + if (err) { > > > > + pr_err("%s: UHS2 DEVICE_INIT fail!\n", mmc_hostname(host)); > > > > return err; > > > > + } > > > > > > > > err = sd_uhs2_enum(host, &node_id); > > > > - if (err) > > > > + if (err) { > > > > + pr_err("%s: UHS2 ENUMERATE fail!\n", mmc_hostname(host)); > > > > return err; > > > > + } > > > > > > > > card = mmc_alloc_card(host, &sd_type); > > > > if (IS_ERR(card)) > > > > return PTR_ERR(card); > > > > + host->card = card; > > > > > > > > card->uhs2_config.node_id = node_id; > > > > card->type = MMC_TYPE_SD; > > > > @@ -139,6 +851,16 @@ static int sd_uhs2_init_card(struct mmc_host *host) > > > > if (err) > > > > goto err; > > > > > > > > + /* Change to Speed Range B if it is supported */ > > > > + if (host->uhs2_caps.flags & MMC_UHS2_SPEED_B) { > > > > + err = sd_uhs2_change_speed(host, node_id); > > > > + if (err) { > > > > + pr_err("%s: %s: UHS2 sd_uhs2_change_speed() fail!\n", > > > > + mmc_hostname(host), __func__); > > > > + return err; > > > > + } > > > > + } > > > > + > > > > err = sd_uhs2_config_write(host, card); > > > > if (err) > > > > goto err; > > > > @@ -147,20 +869,13 @@ static int sd_uhs2_init_card(struct mmc_host *host) > > > > if (err) > > > > goto err; > > > > > > > > - host->card = card; > > > > return 0; > > > > > > > > err: > > > > - mmc_remove_card(card); > > > > + sd_uhs2_remove(host); > > > > return err; > > > > } > > > > > > [...] > > > > > > Kind regards > > > Uffe