On 7/06/23 04:48, Chevron Li wrote:
> From: Chevron Li <chevron.li@xxxxxxxxxxxxxx>
>
> Add Bayhub new chip GG8 support for SD express card.
> This patch depends on patch 1/2.
Patch subject should be different from patch 1
>
> Signed-off-by: Chevron Li <chevron.li@xxxxxxxxxxxxxx>
Some comments below.
> ---
> Change in V1:
> 1.Implement the SD express card callback routine.
> 2.Add SD express card support for Bayhub GG8 chip.
> ---
> drivers/mmc/host/sdhci-pci-o2micro.c | 61 +++++++++++++++++++++++++++-
> 1 file changed, 60 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-pci-o2micro.c b/drivers/mmc/host/sdhci-pci-o2micro.c
> index 8243a63b3c81..b2d8ddbb4095 100644
> --- a/drivers/mmc/host/sdhci-pci-o2micro.c
> +++ b/drivers/mmc/host/sdhci-pci-o2micro.c
> @@ -21,6 +21,7 @@
> * O2Micro device registers
> */
>
> +#define O2_SD_PCIE_SWITCH 0x54
> #define O2_SD_MISC_REG5 0x64
> #define O2_SD_LD0_CTRL 0x68
> #define O2_SD_DEV_CTRL 0x88
> @@ -631,6 +632,63 @@ static void sdhci_pci_o2_set_clock(struct sdhci_host *host, unsigned int clock)
> sdhci_o2_enable_clk(host, clk);
> }
>
> +static u8 sdhci_o2_sd_express_clkq_assert(struct sdhci_host *host)
> +{
> + return sdhci_readb(host, O2_SD_EXP_INT_REG);
> +}
> +
> +static int sdhci_pci_o2_init_sd_express(struct mmc_host *mmc, struct mmc_ios *ios)
> +{
> + struct sdhci_host *host = mmc_priv(mmc);
> + struct sdhci_pci_slot *slot = sdhci_priv(host);
> + struct sdhci_pci_chip *chip = slot->chip;
> + u8 scratch8 = 0;
> + u16 scratch16 = 0;
> + bool ret = false;
ret should be an int and does not need to be initialized.
> +
> + /* Disable clock */
> + sdhci_writeb(host, 0, SDHCI_CLOCK_CONTROL);
> +
> + /* Set VDD2 voltage*/
> + scratch8 = sdhci_readb(host, SDHCI_POWER_CONTROL);
> + scratch8 &= 0x0F;
> + if ((host->mmc->ios.timing == MMC_TIMING_SD_EXP_1_2V) &&
> + (host->mmc->caps2 & MMC_CAP2_SD_EXP_1_2V)) {
Unnecessary parentheses
> + scratch8 |= BIT(4) | BIT(7);
> + } else
braces {} should be used on all arms of this statement
Have a look at the output of checkpatch.pl --strict
There are "Alignment should match open parenthesis"
messages also
> + scratch8 |= BIT(4) | BIT(5) | BIT(7);
Please use SDHCI_VDD2_POWER_ON, SDHCI_VDD2_POWER_120, SDHCI_VDD2_POWER_180
> + sdhci_writeb(host, scratch8, SDHCI_POWER_CONTROL);
> +
> + /* UnLock WP */
> + pci_read_config_byte(chip->pdev, O2_SD_LOCK_WP, &scratch8);
> + scratch8 &= 0x7f;
> + pci_write_config_byte(chip->pdev, O2_SD_LOCK_WP, scratch8);
> +
> + ret = readx_poll_timeout(sdhci_o2_sd_express_clkq_assert, host,
> + scratch8, !(scratch8 & BIT(0)), 1, 30000) == 0 ? 0 : 1;
This can use read_poll_timeout() then sdhci_o2_sd_express_clkq_assert()
is not needed. And the ' == 0 ? 0 : 1' does not seem needed.
> +
> + if (!ret) {
> + /* switch to PCIe mode */
> + scratch16 = sdhci_readw(host, O2_SD_PCIE_SWITCH);
> + scratch16 |= BIT(8);
> + sdhci_writew(host, scratch16, O2_SD_PCIE_SWITCH);
> + } else {
> + /* keep mode as USHI */
USHI ?
Do you intend to fall back to SD mode here? Because
returning non-zero will cause the initialization to
fail. To continue to initialize SD mode, it looks
like you need to change host->ios.timing = MMC_TIMING_LEGACY
and return 0
> + pci_read_config_word(chip->pdev,
> + O2_SD_PARA_SET_REG1, &scratch16);
Unneeded line break
> + scratch16 &= ~BIT(11);
> + pci_write_config_word(chip->pdev,
> + O2_SD_PARA_SET_REG1, scratch16);
Unneeded line break
> + }
> + /* Lock WP */
> + pci_read_config_byte(chip->pdev,
> + O2_SD_LOCK_WP, &scratch8);
> + scratch8 |= 0x80;
> + pci_write_config_byte(chip->pdev, O2_SD_LOCK_WP, scratch8);
> +
> + return ret;
> +}
> +
> static int sdhci_pci_o2_probe_slot(struct sdhci_pci_slot *slot)
> {
> struct sdhci_pci_chip *chip;
> @@ -703,10 +761,11 @@ static int sdhci_pci_o2_probe_slot(struct sdhci_pci_slot *slot)
> case PCI_DEVICE_ID_O2_GG8_9861:
> case PCI_DEVICE_ID_O2_GG8_9862:
> case PCI_DEVICE_ID_O2_GG8_9863:
> - host->mmc->caps2 |= MMC_CAP2_NO_SDIO;
> + host->mmc->caps2 |= MMC_CAP2_NO_SDIO | MMC_CAP2_SD_EXP | MMC_CAP2_SD_EXP_1_2V;
> host->mmc->caps |= MMC_CAP_HW_RESET;
> host->quirks2 |= SDHCI_QUIRK2_PRESET_VALUE_BROKEN;
> slot->host->mmc_host_ops.get_cd = sdhci_o2_get_cd;
> + host->mmc_host_ops.init_sd_express = sdhci_pci_o2_init_sd_express;
> break;
> default:
> break;
