Re: [PATCH v4] mmc: sdhci-pci: add Support of Synopsys DWC_MSHC IP
On Thu, Apr 28, 2016 at 11:35 AM, Prabu Thangamuthu wrote: >> > + mul = 0x2; >> > + for (div = 1; div <= 32; div++) { >> > + if (((host->max_clk * mul) / div) >> > + <= clock) >> >> Too many parens. >> >> > + break; >> > + } >> >> So, you would like to find a minimum divider that guarantees the clock > >> max_clk * mul / div. >> >> This is completely long way to achieve. > > This for loop is exactly taken from drivers/mmc/host/sdhci.c > We want to maintain the consistency between our file and sdhci.c Any strong argument to do so? For now it's not an excuse, sorry. >> What about >> div = DIV_ROUND_UP(max_clk * mul / clock) >> >> div_val = max(div, 32); >> > + timeout = 20; >> > + while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL)) >> > + & SDHCI_CLOCK_INT_STABLE)) { >> > + if (timeout == 0) { >> >> This is invariant to the loop. Check how it's done in other drivers. > > This while loop also exactly taken from drivers/mmc/host/sdhci.c (function > name sdhci_set_clock). > We want to maintain the consistency between our file and sdhci.c for easy > readability. Ditto. >> Something like >> >> while (condition && --timeout) { >> ... >> } >> if (!timeout) { >> ... >> } >> > + if (clock <= 5000) { >> > + /*Change the Tx Phase value here */ >> >> Space > > I didn't get any warning/error when I ran scripts/checkpatch.pl against my > patch. > Did you find this "Space" issue by manual check or by running any scripts? I just noticed this like one above during manual review. >> > +#define SDHCI_UHS2_VENDOR 0xE8 >> > + >> > +#define DRIVER_NAME "sdhci-pci-dwc" >> > +#define SDHC_DEF_TX_CLK_PH_VAL 4 >> > +#define SDHC_DEF_RX_CLK_PH_VAL 4 >> > + >> > +/* Synopsys Vendor Specific Registers */ >> > +#define SDHC_DBOUNCE 0x08 >> > +#define SDHC_TUNING_RX_CLK_SEL_MASK0x00FF GENMASK() ? >> > +#define SDHC_GPIO_OUT 0x34 >> > +/* HAPS 51 Based implementation */ >> > +#define SDHC_BCLK_DCM_RST 0x0001 BIT() ? >> > +#define SDHC_CARD_TX_CLK_DCM_RST 0x0002 >> > +#define SDHC_TUNING_RX_CLK_DCM_RST 0x0004 >> > +#define SDHC_TUNING_TX_CLK_DCM_RST 0x0008 Ditto. >> > +#define SDHC_TUNING_TX_CLK_SEL_MASK0x0070 GENMASK() ? >> > +#define SDHC_TUNING_TX_CLK_SEL_SHIFT 4 >> > +#define SDHC_TX_CLK_SEL_TUNED 0x0080 >> > + >> > +/* Offset of BCLK DCM DRP Attributes */ >> > +/* Every attribute is of 16 bit wide */ >> > +#define BCLK_DCM_DRP_BASE_51 0x1000 >> > + >> > +#define BCLK_DCM_MUL_DIV_DRP 0x1050 >> > +#define MUL_MASK_DRP 0xFF00 >> > +#define DIV_MASK_DRP 0x00FF Ditto. -- With Best Regards, Andy Shevchenko
RE: [PATCH v4] mmc: sdhci-pci: add Support of Synopsys DWC_MSHC IP
Hi Andy Shevchenko, On 04/27/2016 7:09 PM, Andy Shevchenko wrote: > > On Wed, Apr 27, 2016 at 2:51 PM, Prabu Thangamuthu > wrote: > > Patch for Standard SD Host Controller Interface compliant Synopsys > > sdhci-dwc controller driver. This code supports PCI based interface. > > > @@ -0,0 +1,244 @@ > > +/* > > + * Copyright (C) 2016 Synopsys, Inc. > > + * > > + * Author: Manjunath M B > > Authors > > > + *Prabu Thangamuthu > > + * > > + * This software is licensed under the terms of the GNU General > > + Public > > + * License version 2, as published by the Free Software Foundation, > > + and > > + * may be copied, distributed, and modified under those terms. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + */ > > > > > +/* > \ > > + * > > * > > + * Hardware specific clock handling > > * > > + * > > * > > > +\* > *** > > +*/ > > Most of the lines are non-informative. > > > +static void snps_reset_dcm(struct sdhci_host *host, u32 mask, u8 > > +reset) { > > + u16 vendor_ptr; > > + u32 reg; > > + > > + vendor_ptr = sdhci_readw(host, SDHCI_UHS2_VENDOR); > > + > > + reg = sdhci_readl(host, (SDHC_GPIO_OUT + vendor_ptr)); > > + > > + if (reset == 1) > > + reg |= mask; > > + else > > + reg &= ~mask; > > + > > + sdhci_writel(host, reg, (SDHC_GPIO_OUT + vendor_ptr)); } > > + > > +static void sdhci_set_clock_snps(struct sdhci_host *host, u32 clock) > > +{ > > + u8 div; > > + u8 mul; > > I understand that CodingStyle recommends to keep one variable by line, but > here might be useful to combine related variables > > u8 mul, div; > > > + u8 div_val; > > + u8 mul_val; > > u8 mul_val, div_val; > > Up to you and to maintainers. > > > + u8 timeout; > > + u16 clk; > > + u16 vendor_ptr; > > + u16 mul_div_val; > > + u32 reg; > > + > > + /* > > +* if clock is less than 25MHz, divided clock is used. > > if -> If > > > +* For divided clock, we can use the standard sdhci_set_clock(). > > +* For clock above 25MHz, DRP clock is used > > Dot. > > > +* Here, we cannot use sdhci_set_clock(), we need to program > > +* TX RX CLOCK DCM DRP for appropriate clock > > Ditto. > > > +*/ > > > + > > Redundant. > > > + if (clock <= 2500) { > > + /* Then call standard set_clock */ > > + sdhci_set_clock(host, clock); > > > + } else { > > You may save indentation if you return directly in positive branch. > > + > > + host->mmc->actual_clock = 0; > > + vendor_ptr = sdhci_readw(host, SDHCI_UHS2_VENDOR); > > + > > + /* Select un-phase shifted clock before reset Tx > > + Tuning DCM*/ > > Space is missed > > > + reg = sdhci_readl(host, (SDHC_GPIO_OUT + vendor_ptr)); > > + reg &= ~SDHC_TX_CLK_SEL_TUNED; > > + sdhci_writel(host, reg, (SDHC_GPIO_OUT + vendor_ptr)); > > > + mdelay(10); > > Why?! Explanation is needed. > > > + > > + sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL); > > + > > + /* Lets chose the Mulitplier value to be 0x2 */ > > Lets -> Let's > > No explanation why so? > > > + mul = 0x2; > > + for (div = 1; div <= 32; div++) { > > + if (((host->max_clk * mul) / div) > > + <= clock) > > Too many parens. > > > + break; > > + } > > So, you would like to find a minimum divider that guarantees the clock > > max_clk * mul / div. > > This is completely long way to achieve. This for loop is exactly taken from drivers/mmc/host/sdhci.c We want to maintain the consistency between our file and sdhci.c > > What about > div = DIV_ROUND_UP(max_clk * mul / clock) > > div_val = max(div, 32); > > > > + /* > > +* Set Programmable Clock Mode in the Clock > > +* Control register. > > +*/ > > I suppose one line after fixing indentation. > > > + div_val = div - 1; > > + mul_val = mul - 1; > > + > > + host->mmc->actual_clock = (host->max_clk * mul) / div; > > + /* > > +* Program the DCM DRP > > +* Step 1: Assert DCM Reset > > +
Re: [PATCH v4] mmc: sdhci-pci: add Support of Synopsys DWC_MSHC IP
On Wed, Apr 27, 2016 at 2:51 PM, Prabu Thangamuthu wrote: > Patch for Standard SD Host Controller Interface compliant Synopsys > sdhci-dwc controller driver. This code supports PCI based interface. > @@ -0,0 +1,244 @@ > +/* > + * Copyright (C) 2016 Synopsys, Inc. > + * > + * Author: Manjunath M B Authors > + *Prabu Thangamuthu > + * > + * This software is licensed under the terms of the GNU General Public > + * License version 2, as published by the Free Software Foundation, and > + * may be copied, distributed, and modified under those terms. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ > +/*\ > + * > * > + * Hardware specific clock handling > * > + * > * > +\*/ Most of the lines are non-informative. > +static void snps_reset_dcm(struct sdhci_host *host, u32 mask, u8 reset) > +{ > + u16 vendor_ptr; > + u32 reg; > + > + vendor_ptr = sdhci_readw(host, SDHCI_UHS2_VENDOR); > + > + reg = sdhci_readl(host, (SDHC_GPIO_OUT + vendor_ptr)); > + > + if (reset == 1) > + reg |= mask; > + else > + reg &= ~mask; > + > + sdhci_writel(host, reg, (SDHC_GPIO_OUT + vendor_ptr)); > +} > + > +static void sdhci_set_clock_snps(struct sdhci_host *host, u32 clock) > +{ > + u8 div; > + u8 mul; I understand that CodingStyle recommends to keep one variable by line, but here might be useful to combine related variables u8 mul, div; > + u8 div_val; > + u8 mul_val; u8 mul_val, div_val; Up to you and to maintainers. > + u8 timeout; > + u16 clk; > + u16 vendor_ptr; > + u16 mul_div_val; > + u32 reg; > + > + /* > +* if clock is less than 25MHz, divided clock is used. if -> If > +* For divided clock, we can use the standard sdhci_set_clock(). > +* For clock above 25MHz, DRP clock is used Dot. > +* Here, we cannot use sdhci_set_clock(), we need to program > +* TX RX CLOCK DCM DRP for appropriate clock Ditto. > +*/ > + Redundant. > + if (clock <= 2500) { > + /* Then call standard set_clock */ > + sdhci_set_clock(host, clock); > + } else { You may save indentation if you return directly in positive branch. > + > + host->mmc->actual_clock = 0; > + vendor_ptr = sdhci_readw(host, SDHCI_UHS2_VENDOR); > + > + /* Select un-phase shifted clock before reset Tx Tuning DCM*/ Space is missed > + reg = sdhci_readl(host, (SDHC_GPIO_OUT + vendor_ptr)); > + reg &= ~SDHC_TX_CLK_SEL_TUNED; > + sdhci_writel(host, reg, (SDHC_GPIO_OUT + vendor_ptr)); > + mdelay(10); Why?! Explanation is needed. > + > + sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL); > + > + /* Lets chose the Mulitplier value to be 0x2 */ Lets -> Let's No explanation why so? > + mul = 0x2; > + for (div = 1; div <= 32; div++) { > + if (((host->max_clk * mul) / div) > + <= clock) Too many parens. > + break; > + } So, you would like to find a minimum divider that guarantees the clock > max_clk * mul / div. This is completely long way to achieve. What about div = DIV_ROUND_UP(max_clk * mul / clock) div_val = max(div, 32); > + /* > +* Set Programmable Clock Mode in the Clock > +* Control register. > +*/ I suppose one line after fixing indentation. > + div_val = div - 1; > + mul_val = mul - 1; > + > + host->mmc->actual_clock = (host->max_clk * mul) / div; > + /* > +* Program the DCM DRP > +* Step 1: Assert DCM Reset > +* Step 2: Program the mul and div values in DRP > +* Step 3: Read from DRP base 0x00 to restore DCM output as > per > +* www.xilinx.com/support/documentation/user_guides/ug191.pdf > +* Step 4: De-Assert reset to DCM > +*/ > + > + snps_reset_dcm(host, SDHC_CARD_TX_CLK_DCM_RST, 1); > + > + mul_div_val = (mul_val << 8) | div_val; > + sdhci_writew(host, mul_div_val, TXRX_CLK_DCM_MUL_DIV_DRP); > + sdhci_readl(host, TXRX_CLK_DCM