From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754010AbdCOOdw (ORCPT ); Wed, 15 Mar 2017 10:33:52 -0400 Received: from mx0b-0016f401.pphosted.com ([67.231.156.173]:35520 "EHLO mx0b-0016f401.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751147AbdCOOcj (ORCPT ); Wed, 15 Mar 2017 10:32:39 -0400 Subject: Re: [PATCH v6 09/14] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC To: Ulf Hansson , Gregory CLEMENT References:

CC: Adrian Hunter , "linux-mmc@vger.kernel.org" , Jason Cooper , Andrew Lunn , Sebastian Hesselbarth , Thomas Petazzoni , "linux-arm-kernel@lists.infradead.org" , Mike Turquette , Stephen Boyd , linux-clk , "linux-kernel@vger.kernel.org" , Rob Herring , "devicetree@vger.kernel.org" , Jimmy Xu , Jisheng Zhang , Nadav Haklai , Ryan Gao , Doug Jones , Victor Gu , "Wei(SOCP) Liu" , Wilson Ding , Yehuda Yitschak , Marcin Wojtas , Hanna Hawa , Kostya Porotchkin From: Ziji Hu Message-ID: <288b58dc-cce7-11c3-e946-7bf0ea5a4863@marvell.com> Date: Wed, 15 Mar 2017 22:31:53 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-03-15_05:,, signatures=0 X-Proofpoint-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 malwarescore=0 suspectscore=2 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1702020001 definitions=main-1703150113 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Ulf, On 2017/3/15 21:39, Ulf Hansson wrote: > On 14 February 2017 at 18:01, Gregory CLEMENT > wrote: >> From: Hu Ziji >> >> + >> + /* >> + * FIXME: should depends on the specific board timing. >> + */ >> + if ((timing == MMC_TIMING_MMC_HS400) || >> + (timing == MMC_TIMING_MMC_HS200) || >> + (timing == MMC_TIMING_UHS_SDR50) || >> + (timing == MMC_TIMING_UHS_SDR104) || >> + (timing == MMC_TIMING_UHS_DDR50) || >> + (timing == MMC_TIMING_UHS_SDR25) || >> + (timing == MMC_TIMING_MMC_DDR52)) { >> + reg = sdhci_readl(host, phy_regs->timing_adj); >> + reg &= ~XENON_OUTPUT_QSN_PHASE_SELECT; >> + sdhci_writel(host, reg, phy_regs->timing_adj); >> + } >> + >> + >> + reg = sdhci_readl(host, phy_regs->func_ctrl); >> + if ((timing == MMC_TIMING_UHS_DDR50) || >> + (timing == MMC_TIMING_MMC_HS400) || >> + (timing == MMC_TIMING_MMC_DDR52)) >> + reg |= (XENON_DQ_DDR_MODE_MASK << XENON_DQ_DDR_MODE_SHIFT) | >> + XENON_CMD_DDR_MODE; >> + else >> + reg &= ~((XENON_DQ_DDR_MODE_MASK << XENON_DQ_DDR_MODE_SHIFT) | >> + XENON_CMD_DDR_MODE); > > Again you are checking the MMC_TIMING_* in several if statements. > Perhaps you can use switch statement instead and build a couple of > different register variables, that you use when reading/writing > afterwards. > Sorry. I don't quite understand your comment. Do you mean combine all the related register settings under a specific MMC_TIMING_*? swhich (timing) { case MMC_TIMING_HS400: config reg_1; config reg_2; ... case MMC_TIMING_HS200: config reg_2; config reg_3 .. case ... }; I have a little concern that there might be a lot of duplicate register settings. Some PHY operations are irrelevant to the speed mode. They have to be executed in a particular sequence in all speed modes. >> +} >> + >> +static int emmc_phy_parse_param_dt(struct sdhci_host *host, >> + struct device_node *np, >> + struct emmc_phy_params *params) >> +{ >> + u32 value; >> + >> + if (of_property_read_bool(np, "marvell,xenon-phy-slow-mode")) >> + params->slow_mode = true; >> + else >> + params->slow_mode = false; >> + >> + if (!of_property_read_u32(np, "marvell,xenon-phy-znr", &value)) >> + params->znr = value & XENON_ZNR_MASK; >> + else >> + params->znr = XENON_ZNR_DEF_VALUE; >> + >> + if (!of_property_read_u32(np, "marvell,xenon-phy-zpr", &value)) >> + params->zpr = value & XENON_ZPR_MASK; >> + else >> + params->zpr = XENON_ZPR_DEF_VALUE; >> + >> + if (!of_property_read_u32(np, "marvell,xenon-phy-nr-success-tun", >> + &value)) >> + params->nr_tun_times = value & XENON_TUN_CONSECUTIVE_TIMES_MASK; >> + else >> + params->nr_tun_times = XENON_TUN_CONSECUTIVE_TIMES; >> + >> + if (!of_property_read_u32(np, "marvell,xenon-phy-tun-step-divider", >> + &value)) >> + params->tun_step_divider = value & 0xFF; >> + else >> + params->tun_step_divider = XENON_TUNING_STEP_DIVIDER; >> + >> + return 0; > > Instead of all these if-else, let's start by assigning default values > instead. This makes the code more readable. > Sure. I'll change it. >> +} >> + >> +/* >> + * Setting PHY when card is working in High Speed Mode. >> + * HS400 set data strobe line. >> + * HS200/SDR104 set tuning config to prepare for tuning. >> + */ >> +static int xenon_hs_delay_adj(struct sdhci_host *host) >> +{ >> + int ret = 0; >> + >> + if (WARN_ON(host->clock <= XENON_DEFAULT_SDCLK_FREQ)) >> + return -EINVAL; >> + >> + if (host->timing == MMC_TIMING_MMC_HS400) { >> + emmc_phy_strobe_delay_adj(host); >> + return 0; >> + } >> + >> + if ((host->timing == MMC_TIMING_MMC_HS200) || >> + (host->timing == MMC_TIMING_UHS_SDR104)) >> + return emmc_phy_config_tuning(host); >> + >> + /* >> + * DDR Mode requires driver to scan Sampling Fixed Delay Line, >> + * to find out a perfect operation sampling point. >> + * It is hard to implement such a scan in host driver since initiating >> + * commands by host driver is not safe. >> + * Thus so far just keep PHY Sampling Fixed Delay in default value >> + * in DDR mode. >> + * >> + * If any timing issue occurs in DDR mode on Marvell products, >> + * please contact maintainer to ask for internal support in Marvell. >> + */ >> + if ((host->timing == MMC_TIMING_MMC_DDR52) || >> + (host->timing == MMC_TIMING_UHS_DDR50)) >> + dev_warn_once(mmc_dev(host->mmc), "Timing issue might occur in DDR mode\n"); > > Please consider a to convert to use a switch statment instead of the > several if statements. > It is a good idea to use switch here. >> + return ret; >> +} >> + > >> +int xenon_phy_parse_dt(struct device_node *np, struct sdhci_host *host) >> +{ >> + const char *phy_type = NULL; >> + >> + if (!of_property_read_string(np, "marvell,xenon-phy-type", &phy_type)) >> + return add_xenon_phy(np, host, phy_type); >> + >> + dev_info(mmc_dev(host->mmc), "Fail to get Xenon PHY type. Use default eMMC 5.1 PHY\n"); > > Isn't there already a dev_err() being printed for this case. You > probably only need one. By default, "emmc 5.1 phy" is used on most of our products. Thus this dt property is an optional one. I can remove this dev_info. > >> + return add_xenon_phy(np, host, "emmc 5.1 phy"); >> +} > > [...] > > Kind regards > Uffe >