On Wed, Mar 23, 2022 at 05:15:59PM +0100, Ulf Hansson wrote: > On Tue, 22 Feb 2022 at 04:40, Jason Lai <jasonlai.genesyslogic@xxxxxxxxx> wrote: > > > > From: Jason Lai <jason.lai@xxxxxxxxxxxxxxxxxxx> > > > > 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@xxxxxxxxx/ To honor the original work, we should add Intel's copyright notice here as I did before. -Takahiro Akashi > > Signed-off-by: Jason Lai <jason.lai@xxxxxxxxxxxxxxxxxxx> > > --- > > 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 <ulf.hansson@xxxxxxxxxx> > > + * Author: Jason Lai <jason.lai@xxxxxxxxxxxxxxxxxxx> > > * > > * Support for SD UHS-II cards > > */ > > @@ -10,19 +11,31 @@ > > > > #include <linux/mmc/host.h> > > #include <linux/mmc/card.h> > > +#include <linux/mmc/mmc.h> > > +#include <linux/mmc/sd_uhs2.h> > > > > #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
