Hi Pradu, On 04/20/2016 04:00 PM, Prabu Thangamuthu wrote: > Hi Jaehoon Chung, > > Thank you for detailed review comments. > > On 04/19/2016 04:31 PM, Jaehoon Chung wrote: >> On 04/19/2016 04:18 PM, Prabu Thangamuthu wrote: >>> Synopsys DWC_MSHC is compliant with SD Host Specifications. This patch >>> is to support DWC_MSHC controller on PCI interface. >> >> There is dwmmc controller for Synopsys DWC_MSHC. >> According to commit message, dwmmc controller also is compliant with SD >> host specifications. >> >> I think it's enough to use "sdhci-dwc".(To prevent confusion.) > > For Information, Synopsys has two different host controllers, > 1. dw_mmc - It's old controller and not compliant with SDHCI specification. > 2. dwc_mshc - It's new controller and compliant with SDHCI specification.
Thanks for this information. Actually, it should be difficult to distinguish between dw_mmc and dwc_mshc. As you know, DesignWare Cores Mobile Storage Host Databook is also dwmmc controller's TRM. mshc is Mobile Storage Host controller, right? it's confusion. > > This patch is to support second controller (dwc_mshc). > > As you mentioned, we will use "sdhci-dwc" instead of Synopsys DWC_MSHC > to avoid confusion. Thanks for accepting my suggestion. :) > >> >>> >>> Signed-off-by: Prabu Thangamuthu <[email protected]> >>> --- >>> Change log v2: >>> -Removed Synopsys specific PCI device ID's from pci_ids.h. >>> -Updated the PCI device ID's in sdhci-pci-core.c. >>> >>> MAINTAINERS | 7 + >>> drivers/mmc/host/Makefile | 3 +- >>> drivers/mmc/host/sdhci-dwc-mshc-pci.c | 298 >>> ++++++++++++++++++++++++++++++++++ >>> drivers/mmc/host/sdhci-dwc-mshc-pci.h | 58 +++++++ >>> drivers/mmc/host/sdhci-pci-core.c | 14 ++ >>> 5 files changed, 379 insertions(+), 1 deletion(-) create mode 100644 >>> drivers/mmc/host/sdhci-dwc-mshc-pci.c >>> create mode 100644 drivers/mmc/host/sdhci-dwc-mshc-pci.h >>> >>> diff --git a/MAINTAINERS b/MAINTAINERS index 1d5b4be..b260b97 100644 >>> --- a/MAINTAINERS >>> +++ b/MAINTAINERS >>> @@ -9751,6 +9751,13 @@ S: Maintained >>> F: include/linux/mmc/dw_mmc.h >>> F: drivers/mmc/host/dw_mmc* >>> >>> +SYNOPSYS SDHCI COMPLIANT DWC MSHC DRIVER >>> +M: Prabu Thangamuthu <[email protected]> >>> +M: Manjunath M B <[email protected]> >>> +L: [email protected] >>> +S: Maintained >>> +F: drivers/mmc/host/sdhci-dwc-mshc* >>> + >>> SYSTEM TRACE MODULE CLASS >>> M: Alexander Shishkin <[email protected]> >>> S: Maintained >>> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile >>> index af918d2..1d2a3cf 100644 >>> --- a/drivers/mmc/host/Makefile >>> +++ b/drivers/mmc/host/Makefile >>> @@ -9,7 +9,8 @@ obj-$(CONFIG_MMC_MXC) += mxcmmc.o >>> obj-$(CONFIG_MMC_MXS) += mxs-mmc.o >>> obj-$(CONFIG_MMC_SDHCI) += sdhci.o >>> obj-$(CONFIG_MMC_SDHCI_PCI) += sdhci-pci.o >>> -sdhci-pci-y += sdhci-pci-core.o sdhci-pci-o2micro.o >>> +sdhci-pci-y += sdhci-pci-core.o sdhci-pci-o2micro.o >>> \ >>> + sdhci-dwc-mshc-pci.o >> >> how about using sdhci-pci-XXX? > > OK, We will update it accordingly. > > >>> obj-$(subst m,y,$(CONFIG_MMC_SDHCI_PCI)) += sdhci-pci-data.o >>> obj-$(CONFIG_MMC_SDHCI_ACPI) += sdhci-acpi.o >>> obj-$(CONFIG_MMC_SDHCI_PXAV3) += sdhci-pxav3.o >>> diff --git a/drivers/mmc/host/sdhci-dwc-mshc-pci.c >>> b/drivers/mmc/host/sdhci-dwc-mshc-pci.c >>> new file mode 100644 >>> index 0000000..e934af4 >>> --- /dev/null >>> +++ b/drivers/mmc/host/sdhci-dwc-mshc-pci.c >>> @@ -0,0 +1,298 @@ >>> +/* >>> + * Copyright (C) 2016 Synopsys, Inc. >>> + * >>> + * Author: Manjunath M B <[email protected]> >>> + * >>> + * 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. >>> + * >>> + */ >>> + >>> +#include <linux/pci.h> >>> +#include <linux/delay.h> >>> +#include <linux/module.h> >>> +#include <linux/moduleparam.h> >>> + >>> +#include "sdhci.h" >>> +#include "sdhci-pci.h" >>> +#include "sdhci-dwc-mshc-pci.h" >>> + >>> >> +/********************************************************* >> ********************\ >>> + * >>> * >>> + * Hardware specific clock handling >>> * >>> + * >>> * >>> >> +\********************************************************* >> ********************/ >>> +static const struct sdhci_ops *sdhci_ops; /* Low level hw interface */ >>> + >>> +static void sdhci_set_clock_snps(struct sdhci_host *host, >>> + unsigned int clock) >>> +{ >>> + int div = 0; >>> + int mul = 0; >>> + int div_val = 0; >>> + int mul_val = 0; >>> + int mul_div_val = 0; >>> + int reg = 0; >>> + u16 clk = 0; >>> + u32 vendor_ptr; >>> + unsigned long timeout; >>> + u32 tx_clk_phase_val = SDHC_DEF_TX_CLK_PH_VAL; >>> + u32 rx_clk_phase_val = SDHC_DEF_RX_CLK_PH_VAL; >>> + >>> + /* DWC MSHC Specific clock settings */ >> >> I think this comment don't need..because this file already is IP specific. >> > > OK, We will update it. > >>> + >>> + /* if clock is less than 25MHz, divided clock is used. >>> + * For divided clock, we can use the standard sdhci_set_clock(). >>> + * For clock above 25MHz, DRP clock is used >>> + * Here, we cannot use sdhci_set_clock(), we need to program >>> + * TX RX CLOCK DCM DRP for appropriate clock >>> + */ >>> + >>> + vendor_ptr = sdhci_readw(host, SDHCI_UHS2_VENDOR); >>> + >>> + if (clock <= 25000000) { >>> + /* Then call generic set_clock */ >>> + sdhci_ops->set_clock(host, clock); >>> + } else { >>> + >>> + host->mmc->actual_clock = 0; >>> + >>> + /* Select un-phase shifted clock before reset Tx Tuning >> DCM*/ >>> + reg = sdhci_readl(host, (SDHC_GPIO_OUT + vendor_ptr)); >>> + reg &= ~SDHC_TX_CLK_SEL_TUNED; >>> + sdhci_writew(host, reg, (SDHC_GPIO_OUT + vendor_ptr)); >>> + mdelay(10); >>> + >>> + sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL); >>> + >>> + if (clock == 0) >>> + return; >> >> Why return here? Can this locate above? >> > > We will remove it as it's dead code. > >>> + >>> + /* Lets chose the Mulitplier value to be 0x2 */ >>> + mul = 0x2; >>> + for (div = 1; div <= 32; div++) { >>> + if (((host->max_clk * mul) / div) >>> + <= clock) >>> + break; >>> + } >>> + /* >>> + * Set Programmable Clock Mode in the Clock >>> + * Control register. >>> + */ >>> + 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 */ >> >> I think more readable that you write the all steps on here. >> /* >> * Step1 :... >> * Step2 :... >> > > OK, We will update it. > >>> + reg = sdhci_readl(host, (SDHC_GPIO_OUT + vendor_ptr)); >>> + reg = reg | SDHC_CARD_TX_CLK_DCM_RST; >> >> reg |= SDHC_CARD_... >> > > OK, We will update it. > >>> + sdhci_writew(host, reg, (SDHC_GPIO_OUT + vendor_ptr)); >>> + >>> + /* Step 2: Program the mul and div values in DRP */ >>> + mul_div_val = (mul_val << 8) | div_val; >>> + sdhci_writew(host, mul_div_val, >> TXRX_CLK_DCM_MUL_DIV_DRP); >>> + >>> + /* Step 3: issue a dummy read from DRP base 0x00 as per >>> + * >> www.xilinx.com/support/documentation/user_guides/ug191.pdf >>> + */ >>> + reg = sdhci_readw(host, TXRX_CLK_DCM_DRP_BASE_51); >> >> This code is workaroud for issue? >> > > This is not an issue. > We use Dynamic Reconfigurable Port(DRP) of Xilinx Digital Clock Manager(DCM). > To restore DCM status output, read from DRP base 0x00 is needed as per > document > mentioned. > > >>> + >>> + /* Step 4: De-Assert reset to DCM */ >>> + reg = sdhci_readl(host, (SDHC_GPIO_OUT + vendor_ptr)); >>> + reg &= ~SDHC_CARD_TX_CLK_DCM_RST; >>> + sdhci_writew(host, reg, (SDHC_GPIO_OUT + vendor_ptr)); >>> + >>> + clk |= SDHCI_PROG_CLOCK_MODE | SDHCI_CLOCK_INT_EN; >>> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); >>> + >>> + /* Wait max 20 ms */ >>> + timeout = 20; >>> + while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL)) >>> + & SDHCI_CLOCK_INT_STABLE)) { >>> + if (timeout == 0) { >>> + pr_err("%s: Internal clock never stabilised\n", >>> + mmc_hostname(host->mmc)); >>> + return; >>> + } >>> + timeout--; >>> + mdelay(1); >>> + } >>> + >>> + clk |= SDHCI_CLOCK_CARD_EN; >>> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL); >>> + >>> + /* For some bit-files we may have to do phase shifting for >>> + * Tx Clock; Let's do it here >>> + */ >>> + reg = sdhci_readl(host, (SDHC_GPIO_OUT + vendor_ptr)); >>> + reg = reg | SDHC_TUNING_TX_CLK_DCM_RST; >> >> Ditto. >> > > OK, We will update it. > >>> + sdhci_writew(host, reg, (SDHC_GPIO_OUT + vendor_ptr)); >>> + mdelay(10); >>> + reg = sdhci_readl(host, (SDHC_GPIO_OUT + vendor_ptr)); >>> + reg &= ~SDHC_TUNING_TX_CLK_DCM_RST; >>> + sdhci_writew(host, reg, (SDHC_GPIO_OUT + vendor_ptr)); >>> + >>> + /* Select working phase value if clock is <= 50MHz */ >>> + if (clock <= 50000000) { >>> + /*Change the Phase value here */ >>> + reg = sdhci_readl(host, (SDHC_GPIO_OUT + >> vendor_ptr)); >>> + reg = reg | (SDHC_TUNING_TX_CLK_SEL_MASK & >>> + (tx_clk_phase_val << >> SDHC_TUNING_TX_CLK_SEL_SHIFT)); >>> + sdhci_writew(host, reg, (SDHC_GPIO_OUT + >> vendor_ptr)); >>> + mdelay(10); >>> + >>> + /* Program to select phase shifted clock */ >>> + reg = sdhci_readl(host, (SDHC_GPIO_OUT + >> vendor_ptr)); >>> + reg = reg | SDHC_TX_CLK_SEL_TUNED; >>> + sdhci_writew(host, reg, (SDHC_GPIO_OUT + >> vendor_ptr)); >>> + mdelay(10); >>> + } >>> + >>> + /* This Clock might have affected the RX CLOCK DCM >>> + * used for Phase control; Reset this DCM for proper clock >>> + */ >>> + >>> + /* Program the DCM DRP */ >>> + /* Step 1: Reset the DCM */ >>> + reg = sdhci_readl(host, (SDHC_GPIO_OUT + vendor_ptr)); >>> + reg = reg | SDHC_TUNING_RX_CLK_DCM_RST; >>> + sdhci_writew(host, reg, (SDHC_GPIO_OUT + vendor_ptr)); >>> + mdelay(10); >>> + /* Step 2: De-Assert reset to DCM */ >>> + reg = sdhci_readl(host, (SDHC_GPIO_OUT + vendor_ptr)); >>> + reg &= ~SDHC_TUNING_RX_CLK_DCM_RST; >>> + sdhci_writew(host, reg, (SDHC_GPIO_OUT + vendor_ptr)); >> >> Can these codes reuse? > > We will modify the code to reuse it. > >> If clock is upper than 50MHz, then only this code is running twice..is it >> correct? >> > > No, We are handling Transmit clock control and Receive clock control here. > The code under (clock<=50000000) is shifting the transmit clock phase as per > board requirement. Ok. i see. I read something wrong. :) RX - TX. Best Regards, Jaehoon Chung > > >>> + >>> + /* For 50Mhz, tuning is not possible.. Lets fix the sampling >>> + * Phase of Rx Clock here >>> + */ >>> + if (clock <= 50000000) { >>> + /*Change the Phase value here */ >>> + reg = sdhci_readl(host, (SDHC_DBOUNCE + >> vendor_ptr)); >>> + reg &= ~SDHC_TUNING_RX_CLK_SEL_MASK; >>> + reg = reg | (SDHC_TUNING_RX_CLK_SEL_MASK & >>> + rx_clk_phase_val); >>> + sdhci_writew(host, reg, (SDHC_DBOUNCE + >> vendor_ptr)); >>> + } >>> + mdelay(10); >>> + } >>> +} >>> + >>> +static int sdhci_pci_enable_dma_snps(struct sdhci_host *host) { >>> + /* DWC MSHC Specific Dma Enabling */ >> >> Ditto.. this file is already DWC specific. > > OK, We will update it. > > >>> + >>> + /* Call generic emable_dma */ >>> + return sdhci_ops->enable_dma(host); >>> +} >>> + >>> +static void sdhci_pci_set_bus_width_snps(struct sdhci_host *host, int >>> +width) { >>> + /* DWC MSHC Specific Bus Width Setting */ >> >> Ditto. > > OK, We will update it. > >>> + >>> + /* Call generic set_bus_width */ >>> + sdhci_ops->set_bus_width(host, width); } >>> + >>> +static void sdhci_reset_snps(struct sdhci_host *host, u8 mask) { >>> + /* DWC MSHC Specific hci reset */ >>> + >>> + /* Call generic reset */ >>> + sdhci_ops->reset(host, mask); >>> +} >>> +static void sdhci_set_uhs_signaling_snps(struct sdhci_host *host, >>> + unsigned int timing) >>> +{ >>> + /* DWC MSHC Specific UHS-I Signaling */ >>> + >>> + /* Call generic UHS-I signaling */ >>> + sdhci_ops->set_uhs_signaling(host, timing); } static void >>> +sdhci_pci_hw_reset_snps(struct sdhci_host *host) { >>> + /* DWC MSHC Specific hw reset */ >>> + >>> + /* Call generic hw_reset */ >>> + if (host->ops && host->ops->hw_reset) >> >> Above other host->ops-> didn't check..do you think what's correct? >> > > We missed it. We will modify the code considering Ludovic's comments. > >>> + sdhci_ops->hw_reset(host); >>> +} >>> +static const struct sdhci_ops sdhci_pci_ops_snps = { >>> + .set_clock = sdhci_set_clock_snps, >>> + .enable_dma = sdhci_pci_enable_dma_snps, >>> + .set_bus_width = sdhci_pci_set_bus_width_snps, >>> + .reset = sdhci_reset_snps, >>> + .set_uhs_signaling = sdhci_set_uhs_signaling_snps, >>> + .hw_reset = sdhci_pci_hw_reset_snps, >>> +}; >>> + >>> +static int snps_init_clock(struct sdhci_host *host) { >>> + int div = 0; >>> + int mul = 0; >>> + int div_val = 0; >>> + int mul_val = 0; >>> + int mul_div_val = 0; >>> + int reg = 0; >>> + u32 vendor_ptr; >>> + >>> + vendor_ptr = sdhci_readw(host, SDHCI_UHS2_VENDOR); >>> + >>> + /* Configure the BCLK DRP to get 100 MHZ Clock */ >>> + >>> + /* To get 100MHz from 100MHz input freq, >>> + * mul=2 and div=2 >>> + * Formula: output_clock = (input clock * mul) / div >>> + */ >>> + mul = 2; >>> + div = 2; >>> + mul_val = mul - 1; >>> + div_val = div - 1; >>> + /* Program the DCM DRP */ >>> + /* Step 1: Reset the DCM */ >>> + reg = sdhci_readl(host, (SDHC_GPIO_OUT + vendor_ptr)); >>> + reg = reg | SDHC_BCLK_DCM_RST; >>> + sdhci_writew(host, reg, (SDHC_GPIO_OUT + vendor_ptr)); >>> + /* Step 2: Program the mul and div values in DRP */ >>> + mul_div_val = (mul_val << 8) | div_val; >>> + sdhci_writew(host, mul_div_val, BCLK_DCM_MUL_DIV_DRP); >>> + /* Step 3: issue a dummy read from DRP base 0x00 as per >>> + * >> http://www.xilinx.com/support/documentation/user_guides/ug191.pdf >>> + */ >>> + reg = sdhci_readw(host, BCLK_DCM_DRP_BASE_51); >>> + /* Step 4: De-Assert reset to DCM */ >>> + /* de assert reset*/ >>> + reg = sdhci_readl(host, (SDHC_GPIO_OUT + vendor_ptr)); >>> + reg &= ~SDHC_BCLK_DCM_RST; >>> + sdhci_writew(host, reg, (SDHC_GPIO_OUT + vendor_ptr)); >> >> I'm not sure but it seems these codes are also simliar to above codes. >> If i'm correct, you can reduce the code lines. >> > > It's to program the DCM which is used for base clock. > We will try to optimize/reuse the code. > >>> + >>> + /* By Default Clocks to MSHC are off.. >>> + * Before stack applies reset; we need to turn on the clock >>> + */ >>> + sdhci_writew(host, SDHCI_CLOCK_INT_EN, >> SDHCI_CLOCK_CONTROL); >>> + >>> + return 0; >>> + >>> +} >>> + >>> +int sdhci_pci_probe_slot_snps(struct sdhci_pci_slot *slot) { >>> + int ret = 0; >>> + struct sdhci_host *host; >>> + >>> + host = slot->host; >>> + sdhci_ops = host->ops; >>> + host->ops = &sdhci_pci_ops_snps; >>> + >>> + /* Board specific clock initialization */ >>> + ret = snps_init_clock(host); >>> + return ret; >>> +} >>> +EXPORT_SYMBOL_GPL(sdhci_pci_probe_slot_snps); >>> diff --git a/drivers/mmc/host/sdhci-dwc-mshc-pci.h >>> b/drivers/mmc/host/sdhci-dwc-mshc-pci.h >>> new file mode 100644 >>> index 0000000..1635bf2 >>> --- /dev/null >>> +++ b/drivers/mmc/host/sdhci-dwc-mshc-pci.h >>> @@ -0,0 +1,58 @@ >>> +/* >>> + * Copyright (C) 2016 Synopsys, Inc. >>> + * >>> + * Author: Manjunath M B <[email protected]> >>> + * >>> + * 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. >>> + * >>> + */ >>> + >>> +#ifndef __SDHCI_DWC_MSHC_PCI_H__ >>> +#define __SDHCI_DWC_MSHC_PCI_H__ >>> + >>> +#include "sdhci-pci.h" >>> + >>> +#define SDHCI_UHS2_VENDOR 0xE8 >>> + >>> +#define DRIVER_NAME "sdhci_snps" >> >> What do you use .."snps" or "dwc_mshc"? >> As i mentioned, i want not to use "*mshc*" in these patches. >> > > We will use "sdhci-pci-dwc" to make it uniform. > >> Best Regards, >> Jaehoon Chung >> >>> +#define SDHC_DEF_TX_CLK_PH_VAL 4 >>> +#define SDHC_DEF_RX_CLK_PH_VAL 4 > > Thanks & Regards, > Prabu Thangamuthu. > > >
