| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAK00qKDMZQkwFq3Eovmzht1cGNQm_S954tEm2xOo6P2aj6Anxg@mail.gmail.com>
Date: Thu, 29 Aug 2024 17:22:05 +0800
From: Victor Shih <victorshihgli@...il.com>
To: Adrian Hunter <adrian.hunter@...el.com>
Cc: ulf.hansson@...aro.org, linux-mmc@...r.kernel.org,
linux-kernel@...r.kernel.org, benchuanggli@...il.com,
Lucas.Lai@...esyslogic.com.tw, HL.Liu@...esyslogic.com.tw,
Greg.tu@...esyslogic.com.tw, takahiro.akashi@...aro.org, dlunev@...omium.org,
Ben Chuang <ben.chuang@...esyslogic.com.tw>,
Victor Shih <victor.shih@...esyslogic.com.tw>
Subject: Re: [PATCH V18 22/22] mmc: sdhci-pci-gli: enable UHS-II mode for GL9767
On Fri, Aug 23, 2024 at 8:41 PM Adrian Hunter <adrian.hunter@...el.com> wrote:
>
> On 2/08/24 13:22, Victor Shih wrote:
> > From: Victor Shih <victor.shih@...esyslogic.com.tw>
> >
> > Changes are:
> > * Enable the internal clock when do reset on UHS-II mode.
> > * Increase timeout value before detecting UHS-II interface.
> > * Add vendor settings for UHS-II mode.
> >
> > Signed-off-by: Ben Chuang <ben.chuang@...esyslogic.com.tw>
> > Signed-off-by: Victor Shih <victor.shih@...esyslogic.com.tw>
> > ---
> >
> > Updates in V18:
> > - Add new register settings for gl9767.
> > - Add card_event in the sdhci_ops for gl9767.
> > - Add sdhci_gl9767_set_card_detect_debounce_time()
> > to configure the gl9767.
> > - Adjust the sdhci_gl9767_reset() process for gl9767.
> >
> > Updates in V17:
> > - Use mmc_card_uhs2() to simplify the code in the sdhci_gl9767_reset().
> > - Use mmc_card_uhs2() to simplify the code in the
> > sdhci_gl9767_set_power().
> > - Add sdhci_gli_overcurrent_event_enable() to sdhci_gl9767_set_power().
> >
> > Updates in V15:
> > - Add gl9767 to support uhs2 function.
> >
> > ---
> >
> > drivers/mmc/host/sdhci-pci-gli.c | 214 ++++++++++++++++++++++++++++++-
> > 1 file changed, 212 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
> > index a4164948de81..26f7cf4cb109 100644
> > --- a/drivers/mmc/host/sdhci-pci-gli.c
> > +++ b/drivers/mmc/host/sdhci-pci-gli.c
> > @@ -174,6 +174,15 @@
> > #define PCI_GLI_9755_MISC 0x78
> > #define PCI_GLI_9755_MISC_SSC_OFF BIT(26)
> >
> > +#define SDHCI_GLI_9767_SD_HOST_OPERATION_CTL 0x508
> > +#define SDHCI_GLI_9767_SD_HOST_OPERATION_CTL_CMD_CONFLICT_CHECK BIT(0)
> > +#define SDHCI_GLI_9767_SD_HOST_OPERATION_CTL_DEBOUNCE GENMASK(21, 16)
> > +#define SDHCI_GLI_9767_SD_HOST_OPERATION_CTL_DEBOUNCE_PLUG_IN_VALUE 0x05
> > +#define SDHCI_GLI_9767_SD_HOST_OPERATION_CTL_DEBOUNCE_PLUG_OUT_VALUE 0x3F
> > +#define SDHCI_GLI_9767_SD_HOST_OPERATION_CTL_DEBOUNCE_SCALE GENMASK(23, 22)
> > +#define SDHCI_GLI_9767_SD_HOST_OPERATION_CTL_DEBOUNCE_SCALE_1MS 0x2
> > +#define SDHCI_GLI_9767_SD_HOST_OPERATION_CTL_DEBOUNCE_SCALE_10MS 0x3
> > +
> > #define SDHCI_GLI_9767_GM_BURST_SIZE 0x510
> > #define SDHCI_GLI_9767_GM_BURST_SIZE_AXI_ALWAYS_SET BIT(8)
> >
> > @@ -210,6 +219,13 @@
> > #define PCIE_GLI_9767_SCR_CORE_PWR_D3_OFF BIT(21)
> > #define PCIE_GLI_9767_SCR_CFG_RST_DATA_LINK_DOWN BIT(30)
> >
> > +#define PCIE_GLI_9767_RESET_REG 0x8E4
> > +#define PCIE_GLI_9767_RESET_REG_SD_HOST_SW_RESET BIT(0)
> > +
> > +#define PCIE_GLI_9767_UHS2_PHY_SET_REG1 0x90C
> > +#define PCIE_GLI_9767_UHS2_PHY_SET_REG1_SERDES_INTR GENMASK(31, 29)
> > +#define PCIE_GLI_9767_UHS2_PHY_SET_REG1_SERDES_INTR_VALUE 0x3
> > +
> > #define PCIE_GLI_9767_SDHC_CAP 0x91C
> > #define PCIE_GLI_9767_SDHC_CAP_SDEI_RESULT BIT(5)
> >
> > @@ -228,9 +244,15 @@
> > #define PCIE_GLI_9767_SD_EXPRESS_CTL_SD_EXPRESS_MODE BIT(1)
> >
> > #define PCIE_GLI_9767_SD_DATA_MULTI_CTL 0x944
> > +#define PCIE_GLI_9767_SD_DATA_MULTI_CTL_SELECT_UHS2 BIT(5)
> > +#define PCIE_GLI_9767_SD_DATA_MULTI_CTL_UHS2_SWITCH_CTL BIT(8)
> > #define PCIE_GLI_9767_SD_DATA_MULTI_CTL_DISCONNECT_TIME GENMASK(23, 16)
> > #define PCIE_GLI_9767_SD_DATA_MULTI_CTL_DISCONNECT_TIME_VALUE 0x64
> >
> > +#define PCIE_GLI_9767_UHS2_PHY_SET_REG2 0x948
> > +#define PCIE_GLI_9767_UHS2_PHY_SET_REG2_SSC_PPM_SETTING GENMASK(22, 21)
> > +#define PCIE_GLI_9767_UHS2_PHY_SET_REG2_SSC_PPM_SETTING_VALUE 0x0
> > +
> > #define PCIE_GLI_9767_NORMAL_ERR_INT_STATUS_REG2 0x950
> > #define PCIE_GLI_9767_NORMAL_ERR_INT_STATUS_REG2_SDEI_COMPLETE BIT(0)
> >
> > @@ -240,6 +262,28 @@
> > #define PCIE_GLI_9767_NORMAL_ERR_INT_SIGNAL_EN_REG2 0x958
> > #define PCIE_GLI_9767_NORMAL_ERR_INT_SIGNAL_EN_REG2_SDEI_COMPLETE_SIGNAL_EN BIT(0)
> >
> > +#define PCIE_GLI_9767_UHS2_CTL1 0x95C
> > +#define PCIE_GLI_9767_UHS2_CTL1_TRANS_PASS BIT(5)
> > +#define PCIE_GLI_9767_UHS2_CTL1_TRANS_PASS_VALUE 0x1
> > +#define PCIE_GLI_9767_UHS2_CTL1_DECODING_CTL BIT(6)
> > +#define PCIE_GLI_9767_UHS2_CTL1_DECODING_CTL_VALUE 0x1
> > +#define PCIE_GLI_9767_UHS2_CTL1_SERDES_TRAN GENMASK(10, 7)
> > +#define PCIE_GLI_9767_UHS2_CTL1_SERDES_TRAN_VALUE 0x3
> > +#define PCIE_GLI_9767_UHS2_CTL1_SERDES_RECV GENMASK(14, 11)
> > +#define PCIE_GLI_9767_UHS2_CTL1_SERDES_RECV_VALUE 0xf
> > +#define PCIE_GLI_9767_UHS2_CTL1_DIR_TRANS GENMASK(16, 15)
> > +#define PCIE_GLI_9767_UHS2_CTL1_DIR_TRANS_VALUE 0x0
> > +#define PCIE_GLI_9767_UHS2_CTL1_DIR_RECV GENMASK(18, 17)
> > +#define PCIE_GLI_9767_UHS2_CTL1_DIR_RECV_VALUE 0x0
> > +#define PCIE_GLI_9767_UHS2_CTL1_PDRST BIT(25)
> > +#define PCIE_GLI_9767_UHS2_CTL1_PDRST_VALUE 0x1
> > +
> > +#define PCIE_GLI_9767_UHS2_CTL2 0x964
> > +#define PCIE_GLI_9767_UHS2_CTL2_ZC GENMASK(3, 0)
> > +#define PCIE_GLI_9767_UHS2_CTL2_ZC_VALUE 0xb
> > +#define PCIE_GLI_9767_UHS2_CTL2_ZC_CTL BIT(6)
> > +#define PCIE_GLI_9767_UHS2_CTL2_ZC_CTL_VALUE 0x1
> > +
> > #define GLI_MAX_TUNING_LOOP 40
> >
> > /* Genesys Logic chipset */
> > @@ -1152,6 +1196,31 @@ static void sdhci_gl9767_set_clock(struct sdhci_host *host, unsigned int clock)
> > gl9767_vhs_read(pdev);
> > }
> >
> > +static void sdhci_gl9767_set_card_detect_debounce_time(struct sdhci_host *host)
> > +{
> > + u32 value;
> > +
> > + value = sdhci_readl(host, SDHCI_GLI_9767_SD_HOST_OPERATION_CTL);
> > + value &= ~(SDHCI_GLI_9767_SD_HOST_OPERATION_CTL_DEBOUNCE |
> > + SDHCI_GLI_9767_SD_HOST_OPERATION_CTL_DEBOUNCE_SCALE);
> > + if (host->mmc->ops->get_cd(host->mmc))
>
> This is OK from the ->card_event() path but not from others like:
>
> sdhci_gl9767_reset() -> gli_set_9767() -> sdhci_gl9767_set_card_detect_debounce_time()
> o Reset can be called in atomic context (under spin lock), but
> host->mmc->ops->get_cd() can be reading a card detect GPIO which
> might sleep.
>
> gli_probe_slot_gl9767() -> gli_set_9767() -> sdhci_gl9767_set_card_detect_debounce_time()
> o gli_probe_slot_gl9767() gets called before card detect GPIO
> is set up in sdhci-pci-core.c
>
> However, presumably you are always using SDHCI_PRESENT_STATE register
> for card detect (hence the debounce settings on the controller), so
> it would be better to just read SDHCI_PRESENT_STATE here.
>
Hi, Adrian
I will correct this issue in the next version.
Thanks, Victor Shih
> > + value |= FIELD_PREP(SDHCI_GLI_9767_SD_HOST_OPERATION_CTL_DEBOUNCE,
> > + SDHCI_GLI_9767_SD_HOST_OPERATION_CTL_DEBOUNCE_PLUG_IN_VALUE) |
> > + FIELD_PREP(SDHCI_GLI_9767_SD_HOST_OPERATION_CTL_DEBOUNCE_SCALE,
> > + SDHCI_GLI_9767_SD_HOST_OPERATION_CTL_DEBOUNCE_SCALE_1MS);
> > + else
> > + value |= FIELD_PREP(SDHCI_GLI_9767_SD_HOST_OPERATION_CTL_DEBOUNCE,
> > + SDHCI_GLI_9767_SD_HOST_OPERATION_CTL_DEBOUNCE_PLUG_OUT_VALUE) |
> > + FIELD_PREP(SDHCI_GLI_9767_SD_HOST_OPERATION_CTL_DEBOUNCE_SCALE,
> > + SDHCI_GLI_9767_SD_HOST_OPERATION_CTL_DEBOUNCE_SCALE_10MS);
> > + sdhci_writel(host, value, SDHCI_GLI_9767_SD_HOST_OPERATION_CTL);
> > +}
> > +
> > +static void sdhci_gl9767_card_event(struct sdhci_host *host)
> > +{
> > + sdhci_gl9767_set_card_detect_debounce_time(host);
> > +}
> > +
> > static void gli_set_9767(struct sdhci_host *host)
> > {
> > u32 value;
> > @@ -1159,6 +1228,12 @@ static void gli_set_9767(struct sdhci_host *host)
> > value = sdhci_readl(host, SDHCI_GLI_9767_GM_BURST_SIZE);
> > value &= ~SDHCI_GLI_9767_GM_BURST_SIZE_AXI_ALWAYS_SET;
> > sdhci_writel(host, value, SDHCI_GLI_9767_GM_BURST_SIZE);
> > +
> > + value = sdhci_readl(host, SDHCI_GLI_9767_SD_HOST_OPERATION_CTL);
> > + value &= ~SDHCI_GLI_9767_SD_HOST_OPERATION_CTL_CMD_CONFLICT_CHECK;
> > + sdhci_writel(host, value, SDHCI_GLI_9767_SD_HOST_OPERATION_CTL);
> > +
> > + sdhci_gl9767_set_card_detect_debounce_time(host);
> > }
> >
> > static void gl9767_hw_setting(struct sdhci_pci_slot *slot)
> > @@ -1197,8 +1272,54 @@ static void gl9767_hw_setting(struct sdhci_pci_slot *slot)
> >
> > static void sdhci_gl9767_reset(struct sdhci_host *host, u8 mask)
> > {
> > - sdhci_reset(host, mask);
> > - gli_set_9767(host);
> > + struct sdhci_pci_slot *slot = sdhci_priv(host);
> > + struct pci_dev *pdev = slot->chip->pdev;
> > + u32 value;
> > + u16 clk_ctrl;
> > + u16 ctrl2;
> > +
> > + /* need internal clock */
> > + if (mask & SDHCI_RESET_ALL) {
> > + ctrl2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> > + clk_ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> > +
> > + if ((ctrl2 & SDHCI_CTRL_V4_MODE) && (ctrl2 & SDHCI_CTRL_UHS2_ENABLE)) {
> > + sdhci_writew(host, SDHCI_CLOCK_INT_EN, SDHCI_CLOCK_CONTROL);
> > + } else {
> > + sdhci_writew(host, SDHCI_CLOCK_INT_EN, SDHCI_CLOCK_CONTROL);
> > + sdhci_wait_clock_stable(host);
> > + sdhci_writew(host, SDHCI_CTRL_V4_MODE, SDHCI_HOST_CONTROL2);
> > + }
>
> Same code is in sdhci_gl9755_reset(), so could factor it out into
> a separate function.
>
Hi, Adrian
I will correct this issue in the next version.
Thanks, Victor Shih
> > +
> > + gl9767_vhs_write(pdev);
> > +
> > + pci_read_config_dword(pdev, PCIE_GLI_9767_RESET_REG, &value);
> > + value &= ~PCIE_GLI_9767_RESET_REG_SD_HOST_SW_RESET;
> > + pci_write_config_dword(pdev, PCIE_GLI_9767_RESET_REG, value);
> > +
> > + if (read_poll_timeout_atomic(pci_read_config_dword, value,
> > + !(value & PCIE_GLI_9767_RESET_REG_SD_HOST_SW_RESET),
> > + 1, 5, true, pdev, PCIE_GLI_9767_RESET_REG, &value)) {
> > + pr_warn("%s: %s: Reset SDHC AHB and TL-AMBA failure.\n",
> > + __func__, mmc_hostname(host->mmc));
> > + return;
> > + }
> > +
> > + gl9767_vhs_read(pdev);
> > + }
> > +
> > + if (mmc_card_uhs2(host->mmc)) {
> > + if (mask & (SDHCI_RESET_CMD | SDHCI_RESET_DATA)) {
> > + sdhci_writeb(host, mask, SDHCI_SOFTWARE_RESET);
>
> Don't you need to wait for that reset at some point?
>
Hi, Adrian
I will correct this issue in the next version.
Thanks, Victor Shih
> > + sdhci_gli_uhs2_reset_sd_tran(host);
> > + } else {
> > + sdhci_uhs2_reset(host, mask);
> > + }
> > + gli_set_9767(host);
> > + } else {
> > + sdhci_reset(host, mask);
> > + gli_set_9767(host);
> > + }
>
> gli_set_9767() is always called, so could be moved here.
>
Hi, Adrian
I will correct this issue in the next version.
Thanks, Victor Shih
> > }
> >
> > static int gl9767_init_sd_express(struct mmc_host *mmc, struct mmc_ios *ios)
> > @@ -1288,6 +1409,86 @@ static int gl9767_init_sd_express(struct mmc_host *mmc, struct mmc_ios *ios)
> > return 0;
> > }
> >
> > +static void gl9767_vendor_init(struct sdhci_host *host)
> > +{
> > + struct sdhci_pci_slot *slot = sdhci_priv(host);
> > + struct pci_dev *pdev = slot->chip->pdev;
> > + u32 value;
> > +
> > + gl9767_vhs_write(pdev);
> > +
> > + pci_read_config_dword(pdev, PCIE_GLI_9767_UHS2_PHY_SET_REG1, &value);
> > + value |= FIELD_PREP(PCIE_GLI_9767_UHS2_PHY_SET_REG1_SERDES_INTR,
> > + PCIE_GLI_9767_UHS2_PHY_SET_REG1_SERDES_INTR_VALUE);
> > + pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_PHY_SET_REG1, value);
> > +
> > + pci_read_config_dword(pdev, PCIE_GLI_9767_UHS2_PHY_SET_REG2, &value);
> > + value |= FIELD_PREP(PCIE_GLI_9767_UHS2_PHY_SET_REG2_SSC_PPM_SETTING,
> > + PCIE_GLI_9767_UHS2_PHY_SET_REG2_SSC_PPM_SETTING_VALUE);
> > + pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_PHY_SET_REG2, value);
> > +
> > + pci_read_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL1, &value);
> > + value |= FIELD_PREP(PCIE_GLI_9767_UHS2_CTL1_TRANS_PASS,
> > + PCIE_GLI_9767_UHS2_CTL1_TRANS_PASS_VALUE) |
> > + FIELD_PREP(PCIE_GLI_9767_UHS2_CTL1_DECODING_CTL,
> > + PCIE_GLI_9767_UHS2_CTL1_DECODING_CTL_VALUE) |
> > + FIELD_PREP(PCIE_GLI_9767_UHS2_CTL1_SERDES_TRAN,
> > + PCIE_GLI_9767_UHS2_CTL1_SERDES_TRAN_VALUE) |
> > + FIELD_PREP(PCIE_GLI_9767_UHS2_CTL1_SERDES_RECV,
> > + PCIE_GLI_9767_UHS2_CTL1_SERDES_RECV_VALUE) |
> > + FIELD_PREP(PCIE_GLI_9767_UHS2_CTL1_DIR_TRANS,
> > + PCIE_GLI_9767_UHS2_CTL1_DIR_TRANS_VALUE) |
> > + FIELD_PREP(PCIE_GLI_9767_UHS2_CTL1_DIR_RECV,
> > + PCIE_GLI_9767_UHS2_CTL1_DIR_RECV_VALUE) |
> > + FIELD_PREP(PCIE_GLI_9767_UHS2_CTL1_PDRST,
> > + PCIE_GLI_9767_UHS2_CTL1_PDRST_VALUE);
> > + pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL1, value);
> > +
> > + pci_read_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, &value);
> > + value |= FIELD_PREP(PCIE_GLI_9767_UHS2_CTL2_ZC,
> > + PCIE_GLI_9767_UHS2_CTL2_ZC_VALUE) |
> > + FIELD_PREP(PCIE_GLI_9767_UHS2_CTL2_ZC_CTL,
> > + PCIE_GLI_9767_UHS2_CTL2_ZC_CTL_VALUE);
> > + pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
> > +
> > + gl9767_vhs_read(pdev);
> > +}
> > +
> > +static void sdhci_gl9767_set_power(struct sdhci_host *host, unsigned char mode, unsigned short vdd)
> > +{
> > + struct sdhci_pci_slot *slot = sdhci_priv(host);
> > + struct pci_dev *pdev = slot->chip->pdev;
> > + u32 value;
> > +
> > + if (mmc_card_uhs2(host->mmc)) {
> > + gl9767_vhs_write(pdev);
> > +
> > + pci_read_config_dword(pdev, PCIE_GLI_9767_SD_DATA_MULTI_CTL, &value);
> > + value |= PCIE_GLI_9767_SD_DATA_MULTI_CTL_SELECT_UHS2 |
> > + PCIE_GLI_9767_SD_DATA_MULTI_CTL_UHS2_SWITCH_CTL;
> > + pci_write_config_dword(pdev, PCIE_GLI_9767_SD_DATA_MULTI_CTL, value);
> > +
> > + gl9767_vhs_read(pdev);
> > +
> > + sdhci_gli_overcurrent_event_enable(host, false);
> > + sdhci_uhs2_set_power(host, mode, vdd);
> > + sdhci_gli_overcurrent_event_enable(host, true);
> > + } else {
> > + gl9767_vhs_write(pdev);
> > +
> > + pci_read_config_dword(pdev, PCIE_GLI_9767_SD_DATA_MULTI_CTL, &value);
> > + value &= ~(PCIE_GLI_9767_SD_DATA_MULTI_CTL_SELECT_UHS2 |
> > + PCIE_GLI_9767_SD_DATA_MULTI_CTL_UHS2_SWITCH_CTL);
> > + pci_write_config_dword(pdev, PCIE_GLI_9767_SD_DATA_MULTI_CTL, value);
> > +
> > + gl9767_vhs_read(pdev);
> > +
> > + sdhci_gli_overcurrent_event_enable(host, false);
> > + sdhci_set_power(host, mode, vdd);
> > + sdhci_gli_overcurrent_event_enable(host, true);
> > + }
> > +}
> > +
> > static int gli_probe_slot_gl9750(struct sdhci_pci_slot *slot)
> > {
> > struct sdhci_host *host = slot->host;
> > @@ -1324,6 +1525,7 @@ static int gli_probe_slot_gl9767(struct sdhci_pci_slot *slot)
> > host->mmc->caps2 |= MMC_CAP2_SD_EXP;
> > host->mmc_host_ops.init_sd_express = gl9767_init_sd_express;
> > sdhci_enable_v4_mode(host);
> > + gl9767_vendor_init(host);
> >
> > return 0;
> > }
> > @@ -1827,12 +2029,20 @@ static const struct sdhci_ops sdhci_gl9767_ops = {
> > .reset = sdhci_gl9767_reset,
> > .set_uhs_signaling = sdhci_set_uhs_signaling,
> > .voltage_switch = sdhci_gl9767_voltage_switch,
> > + .dump_uhs2_regs = sdhci_uhs2_dump_regs,
> > + .set_timeout = sdhci_uhs2_set_timeout,
> > + .irq = sdhci_uhs2_irq,
> > + .set_power = sdhci_gl9767_set_power,
> > + .uhs2_pre_detect_init = sdhci_gli_pre_detect_init,
> > + .card_event = sdhci_gl9767_card_event,
> > };
> >
> > const struct sdhci_pci_fixes sdhci_gl9767 = {
> > .quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC,
> > .quirks2 = SDHCI_QUIRK2_BROKEN_DDR50,
> > .probe_slot = gli_probe_slot_gl9767,
> > + .add_host = sdhci_pci_uhs2_add_host,
> > + .remove_host = sdhci_pci_uhs2_remove_host,
> > .ops = &sdhci_gl9767_ops,
> > #ifdef CONFIG_PM_SLEEP
> > .resume = sdhci_pci_gli_resume,
>
Powered by blists - more mailing lists