On 1/3/2017 3:42 PM, Adrian Hunter wrote: > On 12/12/16 11:03, Shyam Sundar S K wrote: >> This patch adds support for HS200 tuning mode on AMD eMMC-4.5.1 >> >> Reviewed-by: Sen, Pankaj <Pankaj.Sen@xxxxxxx> >> Reviewed-by: Shah, Nehal-bakulchandra <Nehal-bakulchandra.Shah@xxxxxxx> >> Reviewed-by: Agrawal, Nitesh-kumar <Nitesh-kumar.Agrawal@xxxxxxx> >> Signed-off-by: S-k, Shyam-sundar <Shyam-sundar.S-k@xxxxxxx> >> --- >> >> Changes since v1: (https://patchwork.kernel.org/patch/9429071/) >> * Reworked on review comments received which has the following: >> -> Removed the amd_tuning_descriptor struct and amd_find_good_phase() >> and handled the tuning changes in amd_execute_tuning(). >> -> Fix some alignment problems. >> -> Defined macros for registers and bit fields used. >> -> removed mdelay and used msleep >> >> >> Changes since v2: (https://patchwork.kernel.org/patch/9469247/) >> * Resubmitted the v1 patch by removing unused variables. >> >> Changes in v3: (https://patchwork.kernel.org/patch/9469571/) >> * Adding a changelog description and resubmitting the changes made in v2 >> >> drivers/mmc/host/sdhci-pci-core.c | 152 +++++++++++++++++++++++++++++++++++++- >> 1 file changed, 149 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c >> index 1d9e00a..c2a8da4 100644 >> --- a/drivers/mmc/host/sdhci-pci-core.c >> +++ b/drivers/mmc/host/sdhci-pci-core.c >> @@ -817,6 +817,140 @@ enum amd_chipset_gen { >> AMD_CHIPSET_UNKNOWN, >> }; >> >> +/* AMD registers */ >> +#define AMD_SD_AUTO_PATTERN (0xB8) >> +#define AMD_MSLEEP_DURATION (4) >> +#define AMD_SD_MISC_CONTROL (0xD0) >> +#define AMD_MAX_TUNE_VALUE (0x0B) >> +#define AMD_AUTO_TUNE_SEL (0x10800) >> +#define AMD_FIFO_PTR (0x30) >> +#define AMD_BIT_MASK (0x1F) > > The parentheses are not needed. Please remove them. I feel this is a good practice to keep within the brackets. If you still insist to remove it, I will resubmit the patch. > >> + >> +static int amd_tuning_reset(struct sdhci_host *host) >> +{ >> + unsigned int val; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&host->lock, flags); > > sdhci makes too much use of the spin lock. Please correct me if I am wrong > but I don't see how it is needed here, so remove the locking. > >> + >> + val = sdhci_readw(host, SDHCI_HOST_CONTROL2); >> + val |= SDHCI_CTRL_PRESET_VAL_ENABLE | SDHCI_CTRL_EXEC_TUNING; >> + sdhci_writew(host, val, SDHCI_HOST_CONTROL2); >> + >> + val = sdhci_readw(host, SDHCI_HOST_CONTROL2); >> + val &= ~SDHCI_CTRL_EXEC_TUNING; >> + sdhci_writew(host, val, SDHCI_HOST_CONTROL2); >> + >> + spin_unlock_irqrestore(&host->lock, flags); >> + >> + return 0; >> +} >> + >> +static int amd_config_tuning_phase(struct sdhci_host *host, u8 phase) >> +{ >> + struct sdhci_pci_slot *slot = sdhci_priv(host); >> + struct pci_dev *pdev = slot->chip->pdev; >> + unsigned int val; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&host->lock, flags); > > Ditto wrt locking. > >> + >> + pci_read_config_dword(pdev, AMD_SD_AUTO_PATTERN, &val); >> + val &= ~AMD_BIT_MASK; >> + val |= (AMD_AUTO_TUNE_SEL | (phase << 1)); >> + pci_write_config_dword(pdev, AMD_SD_AUTO_PATTERN, val); >> + >> + spin_unlock_irqrestore(&host->lock, flags); >> + >> + return 0; >> +} >> + >> +static int amd_enable_manual_tuning(struct sdhci_pci_slot *slot) >> +{ >> + struct pci_dev *pdev = slot->chip->pdev; >> + unsigned int val; >> + >> + pci_read_config_dword(pdev, AMD_SD_MISC_CONTROL, &val); >> + val |= AMD_FIFO_PTR; >> + pci_write_config_dword(pdev, AMD_SD_MISC_CONTROL, val); >> + >> + return 0; >> +} >> + >> +static int amd_execute_tuning(struct sdhci_host *host, u32 opcode) >> +{ >> + struct sdhci_pci_slot *slot = sdhci_priv(host); >> + u8 ctrl, tune_around, valid_f = 0, valid_win_max = 0; >> + u8 tune_low_max = 0, tune_low = 0, valid_win = 0, tune_res = 0; >> + bool this_tune_ok = 0, last_tune_ok = 0; > > Use "false" instead of 0 for the bools. > >> + >> + amd_tuning_reset(host); >> + >> + /*********************************************************************/ >> + /* Enabling Software Tuning */ >> + /********************************************************************/ >> + /* 1. First switch the eMMC to HS200 Mode >> + * 2. Prepare the registers by using the sampling clock select >> + * 3. Send the CMD21 12 times with block length of 64 bytes >> + * 4. Everytime change the clk phase and check for CRC error >> + * (CMD and DATA),if error, soft reset the CMD and DATA line >> + * 5. Calculate the window and set the clock phase. >> + */ > > Do we really need this comment. I think the code speaks for itself. The algorithm details were added based on the feedback from you for the earlier submission which I made :-) "Please consider adding some explanation of the tuning algorithm and defining some of the constants." I am not really sure, why this needs to be removed now. > > You could say something about whether you support tuning for SD cards, > or do you know this host controller is only used for eMMC? > >> + >> + for (tune_around = 0; tune_around < 12; tune_around++) { >> + amd_config_tuning_phase(host, tune_around); >> + >> + if (mmc_send_tuning(host->mmc, opcode, NULL)) { >> + this_tune_ok = false; >> + host->mmc->need_retune = 0; > > You should not need to change host->mmc->need_retune. It will > be zero anyway. > >> + msleep(AMD_MSLEEP_DURATION); >> + ctrl = SDHCI_RESET_CMD | SDHCI_RESET_DATA; >> + sdhci_writeb(host, ctrl, SDHCI_SOFTWARE_RESET); > > Are you sure you don't have to wait for the reset to complete? > >> + } else { >> + this_tune_ok = true; >> + valid_f += 1; >> + } >> + >> + /* find good phase */ >> + if ((!this_tune_ok && last_tune_ok) || (tune_around == 11)) { >> + if (valid_win > valid_win_max) { >> + valid_win_max = valid_win; >> + tune_low_max = tune_low; >> + } >> + } >> + >> + if (this_tune_ok && (!last_tune_ok)) >> + tune_low = tune_around; >> + if (!this_tune_ok && last_tune_ok) >> + valid_win = 0; >> + else if (this_tune_ok) >> + valid_win += 1; >> + >> + last_tune_ok = this_tune_ok; >> + >> + if (tune_around == 11) { > > So this code really belongs after the loop. But really the logic > looks overly complicated. If all you are doing is finding the centre > of the biggest valid window, then the logic should be more like this: > Is it possible to take this patch as-is without affecting the current logic of amd_execute_tuning ? Can we take up the optimization of the logic in the next submission ? > > static int amd_execute_tuning(struct sdhci_host *host, u32 opcode) > { > struct sdhci_pci_slot *slot = sdhci_priv(host); > int valid_win = 0; > int valid_win_max = 0; > int valid_win_end = 0; > > amd_tuning_reset(host); > > for (tune_around = 0; tune_around < 12; tune_around++) { > amd_config_tuning_phase(host, tune_around); > > if (mmc_send_tuning(host->mmc, opcode, NULL)) { > valid_win = 0; > msleep(AMD_MSLEEP_DURATION); > ctrl = SDHCI_RESET_CMD | SDHCI_RESET_DATA; > sdhci_writeb(host, ctrl, SDHCI_SOFTWARE_RESET); > } else if (++valid_win > valid_win_max) > valid_win_max = valid_win; > valid_win_end = tune_around; > } > } > > if (!valid_win_max) { > dev_err(&slot->chip->pdev->dev, > "no tuning point found\n"); > return -EIO; > } > > amd_config_tuning_phase(host, valid_win_end - valid_win_max / 2); > > amd_enable_manual_tuning(slot); > > host->mmc->retune_period = 0; > > return 0; > } > >> + if ((valid_f + valid_win) > valid_win_max) { >> + if (valid_f > valid_win) >> + tune_res = ((valid_f - valid_win) >> 1); >> + else >> + tune_res = tune_low + ((valid_win + >> + valid_f) >> 1); >> + } else { >> + tune_res = tune_low_max + (valid_win_max >> 1); >> + } >> + >> + if (tune_res > AMD_MAX_TUNE_VALUE) >> + tune_res = AMD_MAX_TUNE_VALUE; >> + >> + amd_config_tuning_phase(host, tune_res); >> + } >> + } >> + host->mmc->retune_period = 0; >> + >> + amd_enable_manual_tuning(slot); >> + return 0; >> +} >> + >> static int amd_probe(struct sdhci_pci_chip *chip) >> { >> struct pci_dev *smbus_dev; >> @@ -839,16 +973,17 @@ static int amd_probe(struct sdhci_pci_chip *chip) >> } >> } >> >> - if ((gen == AMD_CHIPSET_BEFORE_ML) || (gen == AMD_CHIPSET_CZ)) { >> + if ((gen == AMD_CHIPSET_BEFORE_ML) || (gen == AMD_CHIPSET_CZ)) > > Please remove the redundant (). > >> chip->quirks2 |= SDHCI_QUIRK2_CLEAR_TRANSFERMODE_REG_BEFORE_CMD; >> - chip->quirks2 |= SDHCI_QUIRK2_BROKEN_HS200; >> - } >> >> return 0; >> } >> >> +static const struct sdhci_ops amd_sdhci_pci_ops; > > Please just put the definition of amd_sdhci_pci_ops here instead of the forward declaration. > >> + >> static const struct sdhci_pci_fixes sdhci_amd = { >> .probe = amd_probe, >> + .ops = &amd_sdhci_pci_ops, >> }; >> >> static const struct pci_device_id pci_ids[] = { >> @@ -1469,6 +1604,17 @@ static const struct sdhci_ops sdhci_pci_ops = { >> .select_drive_strength = sdhci_pci_select_drive_strength, >> }; >> >> +static const struct sdhci_ops amd_sdhci_pci_ops = { >> + .set_clock = sdhci_set_clock, >> + .enable_dma = sdhci_pci_enable_dma, >> + .set_bus_width = sdhci_pci_set_bus_width, >> + .reset = sdhci_reset, >> + .set_uhs_signaling = sdhci_set_uhs_signaling, >> + .hw_reset = sdhci_pci_hw_reset, >> + .select_drive_strength = sdhci_pci_select_drive_strength, > > You don't use select_drive_strength or hw_reset so you can leave those ones out. > >> + .platform_execute_tuning = amd_execute_tuning, >> +}; >> + >> /*****************************************************************************\ >> * * >> * Suspend/resume * >> > If you are okay with above comments, for remaining comments I will resubmit the patch. Hopefully that will be having all the review comments incorporated :-) We would like to have this patch as early as possible to mainline. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
