From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965660AbcKXLhj (ORCPT ); Thu, 24 Nov 2016 06:37:39 -0500 Received: from mail-wm0-f46.google.com ([74.125.82.46]:36269 "EHLO mail-wm0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965228AbcKXLhf (ORCPT ); Thu, 24 Nov 2016 06:37:35 -0500 MIME-Version: 1.0 In-Reply-To: References: From: Ulf Hansson Date: Thu, 24 Nov 2016 12:37:32 +0100 Message-ID: Subject: Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC To: Gregory CLEMENT Cc: Adrian Hunter , "linux-mmc@vger.kernel.org" , Jason Cooper , Andrew Lunn , Sebastian Hesselbarth , Rob Herring , "devicetree@vger.kernel.org" , Thomas Petazzoni , "linux-arm-kernel@lists.infradead.org" , Ziji Hu , "Jack(SH) Zhu" , Jimmy Xu , Jisheng Zhang , Nadav Haklai , Ryan Gao , Doug Jones , Shiwu Zhang , Victor Gu , "Wei(SOCP) Liu" , Wilson Ding , Xueping Liu , Hilbert Zhang , Keji Zhang , Liuliu Zhao , Peng Zhu , Yu Cao , Romain Perier , Yehuda Yitschak , Marcin Wojtas , Hanna Hawa , Kostya Porotchkin , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 31 October 2016 at 12:09, Gregory CLEMENT wrote: > From: Ziji Hu > > Marvell Xenon eMMC/SD/SDIO Host Controller contains PHY. > Three types of PHYs are supported. > > Add support to multiple types of PHYs init and configuration. > Add register definitions of PHYs. > > Signed-off-by: Hu Ziji > Signed-off-by: Gregory CLEMENT > --- > MAINTAINERS | 2 +- > drivers/mmc/host/Makefile | 2 +- > drivers/mmc/host/sdhci-xenon-phy.c | 1181 +++++++++++++++++++++++++++++- > drivers/mmc/host/sdhci-xenon-phy.h | 157 ++++- > drivers/mmc/host/sdhci-xenon.c | 4 +- > drivers/mmc/host/sdhci-xenon.h | 17 +- > 6 files changed, 1361 insertions(+), 2 deletions(-) > create mode 100644 drivers/mmc/host/sdhci-xenon-phy.c > create mode 100644 drivers/mmc/host/sdhci-xenon-phy.h Can you please consider to split this up somehow!? It would make it easier to review... Anyway, allow me to provide some initial feedback, particularly around those things that Adrian and you requested for my input. [...] > > + > +static int __xenon_emmc_delay_adj_test(struct mmc_card *card) > +{ > + int err; > + u8 *ext_csd = NULL; > + > + err = mmc_get_ext_csd(card, &ext_csd); > + kfree(ext_csd); Why do you read the ext csd here? > + > + return err; > +} > + > +static int __xenon_sdio_delay_adj_test(struct mmc_card *card) > +{ > + struct mmc_command cmd = {0}; > + int err; > + > + cmd.opcode = SD_IO_RW_DIRECT; > + cmd.flags = MMC_RSP_R5 | MMC_CMD_AC; > + > + err = mmc_wait_for_cmd(card->host, &cmd, 0); > + if (err) > + return err; > + > + if (cmd.resp[0] & R5_ERROR) > + return -EIO; > + if (cmd.resp[0] & R5_FUNCTION_NUMBER) > + return -EINVAL; > + if (cmd.resp[0] & R5_OUT_OF_RANGE) > + return -ERANGE; > + return 0; No thanks! MMC/SD/SDIO protocol code belongs in the core. > +} > + > +static int __xenon_sd_delay_adj_test(struct mmc_card *card) > +{ > + struct mmc_command cmd = {0}; > + int err; > + > + cmd.opcode = MMC_SEND_STATUS; > + cmd.arg = card->rca << 16; > + cmd.flags = MMC_RSP_R1 | MMC_CMD_AC; > + > + err = mmc_wait_for_cmd(card->host, &cmd, 0); > + return err; No thanks! MMC/SD/SDIO protocol code belongs in the core. > +} > + [...] > +int xenon_phy_adj(struct sdhci_host *host, struct mmc_ios *ios) > +{ > + struct mmc_host *mmc = host->mmc; > + struct mmc_card *card; > + int ret = 0; > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host); > + > + if (!host->clock) { > + priv->clock = 0; > + return 0; > + } > + > + /* > + * The timing, frequency or bus width is changed, > + * better to set eMMC PHY based on current setting > + * and adjust Xenon SDHC delay. > + */ > + if ((host->clock == priv->clock) && > + (ios->bus_width == priv->bus_width) && > + (ios->timing == priv->timing)) > + return 0; > + > + xenon_phy_set(host, ios->timing); > + > + /* Update the record */ > + priv->bus_width = ios->bus_width; > + /* Temp stage from HS200 to HS400 */ > + if (((priv->timing == MMC_TIMING_MMC_HS200) && > + (ios->timing == MMC_TIMING_MMC_HS)) || > + ((ios->timing == MMC_TIMING_MMC_HS) && > + (priv->clock > host->clock))) { > + priv->timing = ios->timing; > + priv->clock = host->clock; > + return 0; > + } > + /* > + * Skip temp stages from HS400 t0 HS200: > + * from 200MHz to 52MHz in HS400 > + * from HS400 to HS DDR in 52MHz > + * from HS DDR to HS in 52MHz > + * from HS to HS200 in 52MHz > + */ > + if (((priv->timing == MMC_TIMING_MMC_HS400) && > + ((host->clock == MMC_HIGH_52_MAX_DTR) || > + (ios->timing == MMC_TIMING_MMC_DDR52))) || > + ((priv->timing == MMC_TIMING_MMC_DDR52) && > + (ios->timing == MMC_TIMING_MMC_HS)) || > + ((ios->timing == MMC_TIMING_MMC_HS200) && > + (ios->clock == MMC_HIGH_52_MAX_DTR))) { > + priv->timing = ios->timing; > + priv->clock = host->clock; > + return 0; > + } > + priv->timing = ios->timing; > + priv->clock = host->clock; > + > + /* Legacy mode is a special case */ > + if (ios->timing == MMC_TIMING_LEGACY) > + return 0; > + > + if (mmc->card) > + card = mmc->card; > + else > + /* > + * Only valid during initialization > + * before mmc->card is set > + */ > + card = priv->card_candidate; > + if (unlikely(!card)) { > + dev_warn(mmc_dev(mmc), "card is not present\n"); > + return -EINVAL; > + } That your host need to hold a copy of the card pointer, tells me that something is not really correct. I might be wrong, if this turns out to be a special case, but I doubt it. Although, if it *is* a special such case, we shall most likely try to extend the the mmc core layer instead of adding all these hacks in your host driver. [...] Another suggestion of a general improvement; could you perhaps try to add some brief information about what goes on in function headers. Perhaps that could help to more easily understand things. Kind regards Uffe From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ulf Hansson Subject: Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC Date: Thu, 24 Nov 2016 12:37:32 +0100 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Gregory CLEMENT Cc: Adrian Hunter , "linux-mmc@vger.kernel.org" , Jason Cooper , Andrew Lunn , Sebastian Hesselbarth , Rob Herring , "devicetree@vger.kernel.org" , Thomas Petazzoni , "linux-arm-kernel@lists.infradead.org" , Ziji Hu , "Jack(SH) Zhu" , Jimmy Xu , Jisheng Zhang , Nadav Haklai , Ryan Gao , Doug Jones , Shiwu Zhang , Victor Gu , Wei(SOCP) Liu List-Id: devicetree@vger.kernel.org On 31 October 2016 at 12:09, Gregory CLEMENT wrote: > From: Ziji Hu > > Marvell Xenon eMMC/SD/SDIO Host Controller contains PHY. > Three types of PHYs are supported. > > Add support to multiple types of PHYs init and configuration. > Add register definitions of PHYs. > > Signed-off-by: Hu Ziji > Signed-off-by: Gregory CLEMENT > --- > MAINTAINERS | 2 +- > drivers/mmc/host/Makefile | 2 +- > drivers/mmc/host/sdhci-xenon-phy.c | 1181 +++++++++++++++++++++++++++++- > drivers/mmc/host/sdhci-xenon-phy.h | 157 ++++- > drivers/mmc/host/sdhci-xenon.c | 4 +- > drivers/mmc/host/sdhci-xenon.h | 17 +- > 6 files changed, 1361 insertions(+), 2 deletions(-) > create mode 100644 drivers/mmc/host/sdhci-xenon-phy.c > create mode 100644 drivers/mmc/host/sdhci-xenon-phy.h Can you please consider to split this up somehow!? It would make it easier to review... Anyway, allow me to provide some initial feedback, particularly around those things that Adrian and you requested for my input. [...] > > + > +static int __xenon_emmc_delay_adj_test(struct mmc_card *card) > +{ > + int err; > + u8 *ext_csd = NULL; > + > + err = mmc_get_ext_csd(card, &ext_csd); > + kfree(ext_csd); Why do you read the ext csd here? > + > + return err; > +} > + > +static int __xenon_sdio_delay_adj_test(struct mmc_card *card) > +{ > + struct mmc_command cmd = {0}; > + int err; > + > + cmd.opcode = SD_IO_RW_DIRECT; > + cmd.flags = MMC_RSP_R5 | MMC_CMD_AC; > + > + err = mmc_wait_for_cmd(card->host, &cmd, 0); > + if (err) > + return err; > + > + if (cmd.resp[0] & R5_ERROR) > + return -EIO; > + if (cmd.resp[0] & R5_FUNCTION_NUMBER) > + return -EINVAL; > + if (cmd.resp[0] & R5_OUT_OF_RANGE) > + return -ERANGE; > + return 0; No thanks! MMC/SD/SDIO protocol code belongs in the core. > +} > + > +static int __xenon_sd_delay_adj_test(struct mmc_card *card) > +{ > + struct mmc_command cmd = {0}; > + int err; > + > + cmd.opcode = MMC_SEND_STATUS; > + cmd.arg = card->rca << 16; > + cmd.flags = MMC_RSP_R1 | MMC_CMD_AC; > + > + err = mmc_wait_for_cmd(card->host, &cmd, 0); > + return err; No thanks! MMC/SD/SDIO protocol code belongs in the core. > +} > + [...] > +int xenon_phy_adj(struct sdhci_host *host, struct mmc_ios *ios) > +{ > + struct mmc_host *mmc = host->mmc; > + struct mmc_card *card; > + int ret = 0; > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host); > + > + if (!host->clock) { > + priv->clock = 0; > + return 0; > + } > + > + /* > + * The timing, frequency or bus width is changed, > + * better to set eMMC PHY based on current setting > + * and adjust Xenon SDHC delay. > + */ > + if ((host->clock == priv->clock) && > + (ios->bus_width == priv->bus_width) && > + (ios->timing == priv->timing)) > + return 0; > + > + xenon_phy_set(host, ios->timing); > + > + /* Update the record */ > + priv->bus_width = ios->bus_width; > + /* Temp stage from HS200 to HS400 */ > + if (((priv->timing == MMC_TIMING_MMC_HS200) && > + (ios->timing == MMC_TIMING_MMC_HS)) || > + ((ios->timing == MMC_TIMING_MMC_HS) && > + (priv->clock > host->clock))) { > + priv->timing = ios->timing; > + priv->clock = host->clock; > + return 0; > + } > + /* > + * Skip temp stages from HS400 t0 HS200: > + * from 200MHz to 52MHz in HS400 > + * from HS400 to HS DDR in 52MHz > + * from HS DDR to HS in 52MHz > + * from HS to HS200 in 52MHz > + */ > + if (((priv->timing == MMC_TIMING_MMC_HS400) && > + ((host->clock == MMC_HIGH_52_MAX_DTR) || > + (ios->timing == MMC_TIMING_MMC_DDR52))) || > + ((priv->timing == MMC_TIMING_MMC_DDR52) && > + (ios->timing == MMC_TIMING_MMC_HS)) || > + ((ios->timing == MMC_TIMING_MMC_HS200) && > + (ios->clock == MMC_HIGH_52_MAX_DTR))) { > + priv->timing = ios->timing; > + priv->clock = host->clock; > + return 0; > + } > + priv->timing = ios->timing; > + priv->clock = host->clock; > + > + /* Legacy mode is a special case */ > + if (ios->timing == MMC_TIMING_LEGACY) > + return 0; > + > + if (mmc->card) > + card = mmc->card; > + else > + /* > + * Only valid during initialization > + * before mmc->card is set > + */ > + card = priv->card_candidate; > + if (unlikely(!card)) { > + dev_warn(mmc_dev(mmc), "card is not present\n"); > + return -EINVAL; > + } That your host need to hold a copy of the card pointer, tells me that something is not really correct. I might be wrong, if this turns out to be a special case, but I doubt it. Although, if it *is* a special such case, we shall most likely try to extend the the mmc core layer instead of adding all these hacks in your host driver. [...] Another suggestion of a general improvement; could you perhaps try to add some brief information about what goes on in function headers. Perhaps that could help to more easily understand things. Kind regards Uffe From mboxrd@z Thu Jan 1 00:00:00 1970 From: ulf.hansson@linaro.org (Ulf Hansson) Date: Thu, 24 Nov 2016 12:37:32 +0100 Subject: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC In-Reply-To: References: Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 31 October 2016 at 12:09, Gregory CLEMENT wrote: > From: Ziji Hu > > Marvell Xenon eMMC/SD/SDIO Host Controller contains PHY. > Three types of PHYs are supported. > > Add support to multiple types of PHYs init and configuration. > Add register definitions of PHYs. > > Signed-off-by: Hu Ziji > Signed-off-by: Gregory CLEMENT > --- > MAINTAINERS | 2 +- > drivers/mmc/host/Makefile | 2 +- > drivers/mmc/host/sdhci-xenon-phy.c | 1181 +++++++++++++++++++++++++++++- > drivers/mmc/host/sdhci-xenon-phy.h | 157 ++++- > drivers/mmc/host/sdhci-xenon.c | 4 +- > drivers/mmc/host/sdhci-xenon.h | 17 +- > 6 files changed, 1361 insertions(+), 2 deletions(-) > create mode 100644 drivers/mmc/host/sdhci-xenon-phy.c > create mode 100644 drivers/mmc/host/sdhci-xenon-phy.h Can you please consider to split this up somehow!? It would make it easier to review... Anyway, allow me to provide some initial feedback, particularly around those things that Adrian and you requested for my input. [...] > > + > +static int __xenon_emmc_delay_adj_test(struct mmc_card *card) > +{ > + int err; > + u8 *ext_csd = NULL; > + > + err = mmc_get_ext_csd(card, &ext_csd); > + kfree(ext_csd); Why do you read the ext csd here? > + > + return err; > +} > + > +static int __xenon_sdio_delay_adj_test(struct mmc_card *card) > +{ > + struct mmc_command cmd = {0}; > + int err; > + > + cmd.opcode = SD_IO_RW_DIRECT; > + cmd.flags = MMC_RSP_R5 | MMC_CMD_AC; > + > + err = mmc_wait_for_cmd(card->host, &cmd, 0); > + if (err) > + return err; > + > + if (cmd.resp[0] & R5_ERROR) > + return -EIO; > + if (cmd.resp[0] & R5_FUNCTION_NUMBER) > + return -EINVAL; > + if (cmd.resp[0] & R5_OUT_OF_RANGE) > + return -ERANGE; > + return 0; No thanks! MMC/SD/SDIO protocol code belongs in the core. > +} > + > +static int __xenon_sd_delay_adj_test(struct mmc_card *card) > +{ > + struct mmc_command cmd = {0}; > + int err; > + > + cmd.opcode = MMC_SEND_STATUS; > + cmd.arg = card->rca << 16; > + cmd.flags = MMC_RSP_R1 | MMC_CMD_AC; > + > + err = mmc_wait_for_cmd(card->host, &cmd, 0); > + return err; No thanks! MMC/SD/SDIO protocol code belongs in the core. > +} > + [...] > +int xenon_phy_adj(struct sdhci_host *host, struct mmc_ios *ios) > +{ > + struct mmc_host *mmc = host->mmc; > + struct mmc_card *card; > + int ret = 0; > + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host); > + > + if (!host->clock) { > + priv->clock = 0; > + return 0; > + } > + > + /* > + * The timing, frequency or bus width is changed, > + * better to set eMMC PHY based on current setting > + * and adjust Xenon SDHC delay. > + */ > + if ((host->clock == priv->clock) && > + (ios->bus_width == priv->bus_width) && > + (ios->timing == priv->timing)) > + return 0; > + > + xenon_phy_set(host, ios->timing); > + > + /* Update the record */ > + priv->bus_width = ios->bus_width; > + /* Temp stage from HS200 to HS400 */ > + if (((priv->timing == MMC_TIMING_MMC_HS200) && > + (ios->timing == MMC_TIMING_MMC_HS)) || > + ((ios->timing == MMC_TIMING_MMC_HS) && > + (priv->clock > host->clock))) { > + priv->timing = ios->timing; > + priv->clock = host->clock; > + return 0; > + } > + /* > + * Skip temp stages from HS400 t0 HS200: > + * from 200MHz to 52MHz in HS400 > + * from HS400 to HS DDR in 52MHz > + * from HS DDR to HS in 52MHz > + * from HS to HS200 in 52MHz > + */ > + if (((priv->timing == MMC_TIMING_MMC_HS400) && > + ((host->clock == MMC_HIGH_52_MAX_DTR) || > + (ios->timing == MMC_TIMING_MMC_DDR52))) || > + ((priv->timing == MMC_TIMING_MMC_DDR52) && > + (ios->timing == MMC_TIMING_MMC_HS)) || > + ((ios->timing == MMC_TIMING_MMC_HS200) && > + (ios->clock == MMC_HIGH_52_MAX_DTR))) { > + priv->timing = ios->timing; > + priv->clock = host->clock; > + return 0; > + } > + priv->timing = ios->timing; > + priv->clock = host->clock; > + > + /* Legacy mode is a special case */ > + if (ios->timing == MMC_TIMING_LEGACY) > + return 0; > + > + if (mmc->card) > + card = mmc->card; > + else > + /* > + * Only valid during initialization > + * before mmc->card is set > + */ > + card = priv->card_candidate; > + if (unlikely(!card)) { > + dev_warn(mmc_dev(mmc), "card is not present\n"); > + return -EINVAL; > + } That your host need to hold a copy of the card pointer, tells me that something is not really correct. I might be wrong, if this turns out to be a special case, but I doubt it. Although, if it *is* a special such case, we shall most likely try to extend the the mmc core layer instead of adding all these hacks in your host driver. [...] Another suggestion of a general improvement; could you perhaps try to add some brief information about what goes on in function headers. Perhaps that could help to more easily understand things. Kind regards Uffe