Hi Harish,
On 04/02/2018 12:23 PM, Harish Jenny K N wrote:
> From: Diwakar Sharma <diwakar.sharma@xxxxxxxxxxxx>
>
> This patch adds a quirk to reduce clock rate for "SDR104" mode on
> IMX side for Marvell 8887 WiFi + Bluetooth chip side, as Marvell
> does not recommend to use SDIO at the speed of higher than 150MHz.
>
> Signed-off-by: Diwakar Sharma <diwakar.sharma@xxxxxxxxxxxx>
> [Harish: Reduced SDIO clock of SDR104 to 150MHz]
> Signed-off-by: Harish Jenny K N <harish_kandiga@xxxxxxxxxx>
> ---
> drivers/mmc/core/card.h | 5 +++++
> drivers/mmc/core/quirks.h | 3 +++
> drivers/mmc/core/sdio.c | 21 +++++++++++++++++++++
> include/linux/mmc/card.h | 4 ++++
> include/linux/mmc/sdio_ids.h | 1 +
> 5 files changed, 34 insertions(+)
>
> diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
> index 9c821ee..e6c7dad 100644
> --- a/drivers/mmc/core/card.h
> +++ b/drivers/mmc/core/card.h
> @@ -221,4 +221,9 @@ static inline int mmc_card_broken_hpi(const struct mmc_card *c)
> return c->quirks & MMC_QUIRK_BROKEN_HPI;
> }
>
> +static inline int mmc_card_broken_uhs(const struct mmc_card *c)
> +{
> + return c->quirks & MMC_QUIRK_BROKEN_UHS;
> +}
> +
> #endif
> diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h
> index 5153577..575b6f7 100644
> --- a/drivers/mmc/core/quirks.h
> +++ b/drivers/mmc/core/quirks.h
> @@ -132,6 +132,9 @@
> SDIO_FIXUP(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8797_F0,
> add_quirk, MMC_QUIRK_BROKEN_IRQ_POLLING),
>
> + SDIO_FIXUP(SDIO_VENDOR_ID_MARVELL, SDIO_DEVICE_ID_MARVELL_8887WLAN,
> + add_quirk, MMC_QUIRK_BROKEN_UHS),
> +
> END_FIXUP
> };
>
I would recommend to split the change into two commits:
1. add new generic quirk and its handling into sdio.c,
2. add Marvell 8887 card and apply the quirk to it.
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index c599a62..4389f57 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -444,6 +444,8 @@ static int sdio_set_bus_speed_mode(struct mmc_card *card)
> unsigned int bus_speed, timing;
> int err;
> unsigned char speed;
> + int cccr_vsn;
> + unsigned char data;
>
- int cccr_vsn;
- unsigned char data;
+ unsigned char cccr;
> /*
> * If the host doesn't support any of the UHS-I modes, fallback on
> @@ -460,6 +462,25 @@ static int sdio_set_bus_speed_mode(struct mmc_card *card)
> timing = MMC_TIMING_UHS_SDR104;
> card->sw_caps.uhs_max_dtr = UHS_SDR104_MAX_DTR;
> card->sd_bus_speed = UHS_SDR104_BUS_SPEED;
> +
> + err = mmc_io_rw_direct(card, 0, 0, SDIO_CCCR_CCCR,
> + 0, &data);
mmc_io_rw_direct(card, 0, 0, SDIO_CCCR_CCCR, 0, &cccr);
> + if (err)
> + return err;
> +
> + cccr_vsn = data & 0x0f;
> + if (cccr_vsn >= SDIO_CCCR_REV_3_00) {
if ((cccr & 0x0f) >= SDIO_CCCR_REV_3_00) {
Minus two lines of code as a result.
> + if (mmc_host_uhs(card->host) &&
> + mmc_card_broken_uhs(card)) {
> + /*
> + * Reduce the max clock rate to
> + * 150MHz for SD_MODE_UHS_SDR104.
> + */
> + card->sw_caps.uhs_max_dtr =
> + UHS_SDR104_REDUCED_DTR;
> + }
> + }
> +
> } else if ((card->host->caps & MMC_CAP_UHS_DDR50) &&
> (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_DDR50)) {
> bus_speed = SDIO_SPEED_DDR50;
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index 279b390..ccb29f8 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -268,6 +268,10 @@ struct mmc_card {
> #define MMC_QUIRK_BROKEN_IRQ_POLLING (1<<11) /* Polling SDIO_CCCR_INTx could create a fake interrupt */
> #define MMC_QUIRK_TRIM_BROKEN (1<<12) /* Skip trim */
> #define MMC_QUIRK_BROKEN_HPI (1<<13) /* Disable broken HPI support */
> +#define MMC_QUIRK_BROKEN_UHS (1<<14) /* Report a lower SDIO clock rate */
Please use tab symbols for value indentation like on the lines above.
"Report a lower SDIO clock rate for broken UHS" is too verbose IMHO,
"Broken UHS" as a terse comment should be sufficient.
> + /* for broken UHS */
> +
> +#define UHS_SDR104_REDUCED_DTR 150000000
>
In my opinion it makes sense to define UHS_SDR104_REDUCED_DTR on
the next line after #define UHS_SDR104_MAX_DTR.
> bool reenable_cmdq; /* Re-enable Command Queue */
>
> diff --git a/include/linux/mmc/sdio_ids.h b/include/linux/mmc/sdio_ids.h
> index cdd66a5..00e18e2 100644
> --- a/include/linux/mmc/sdio_ids.h
> +++ b/include/linux/mmc/sdio_ids.h
> @@ -55,6 +55,7 @@
> #define SDIO_DEVICE_ID_MARVELL_8688WLAN 0x9104
> #define SDIO_DEVICE_ID_MARVELL_8688BT 0x9105
> #define SDIO_DEVICE_ID_MARVELL_8797_F0 0x9128
> +#define SDIO_DEVICE_ID_MARVELL_8887WLAN 0x9134
Please use tab symbols for value indentation like on the lines above.
--
With best wishes,
Vladimir
--
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
