Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751870AbcDZMcA (ORCPT ); Tue, 26 Apr 2016 08:32:00 -0400 Received: from smtprelay2.synopsys.com ([198.182.60.111]:57483 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751082AbcDZMb6 convert rfc822-to-8bit (ORCPT ); Tue, 26 Apr 2016 08:31:58 -0400 From: Prabu Thangamuthu To: Jaehoon Chung , Ludovic Desroches CC: Prabu Thangamuthu , "Ulf Hansson (ulf.hansson@linaro.org)" , Adrian Hunter , Bjorn Helgaas , Andrew Morton , "David S. Miller" , "Greg Kroah-Hartman" , Kalle Valo , Mauro Carvalho Chehab , Guenter Roeck , Jiri Slaby , "Chaotian Jing" , Andrei Pistirica , Ben Hutchings , Joshua Henderson , Manjunath M Bettegowda , "linux-mmc@vger.kernel.org" , "linux-kernel@vger.kernel.org" , CPGS Subject: RE: [PATCH v3] mmc: sdhci-pci: add Support of Synopsys DWC_MSHC IP Thread-Topic: [PATCH v3] mmc: sdhci-pci: add Support of Synopsys DWC_MSHC IP Thread-Index: AdGa/hTuNAUYV5TORBu0qoRtJeQV5wEbaf6AAAO6RYAADbTn8A== Date: Tue, 26 Apr 2016 12:31:50 +0000 Message-ID: <705D14B1C7978B40A723277C067CEDE2010A4B4B87@IN01WEMBXB.internal.synopsys.com> References: <705D14B1C7978B40A723277C067CEDE2010A4B12F8@IN01WEMBXB.internal.synopsys.com> <20160426085845.GA8756@odux.rfo.atmel.com> <571F46C9.9050905@samsung.com> In-Reply-To: <571F46C9.9050905@samsung.com> Accept-Language: en-US, en-IN Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.144.52.117] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 18371 Lines: 570 Hi Ludovic, Jaehoon Chung, Thank you for your review comments, On 04/26/2016 04:15 PM, Jaehoon Chung wrote: > > On 04/26/2016 05:58 PM, Ludovic Desroches wrote: > > On Wed, Apr 20, 2016 at 12:22:59PM +0000, Prabu Thangamuthu wrote: > >> Patch for Standard SD Host Controller Interface compliant Synopsys > >> sdhci-dwc controller driver. This code supports PCI based interface. > >> > >> Signed-off-by: Prabu Thangamuthu > >> --- > >> Change log v3: > >> -Removed unused code. > >> -Updated review comments. > >> > >> 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-pci-core.c | 14 ++ > >> drivers/mmc/host/sdhci-pci-dwc.c | 260 > >> ++++++++++++++++++++++++++++++++++++++ > >> drivers/mmc/host/sdhci-pci-dwc.h | 55 ++++++++ > >> 5 files changed, 338 insertions(+), 1 deletion(-) create mode > >> 100644 drivers/mmc/host/sdhci-pci-dwc.c create mode 100644 > >> drivers/mmc/host/sdhci-pci-dwc.h > >> > >> diff --git a/MAINTAINERS b/MAINTAINERS index 1d5b4be..01b40da > 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 > >> +M: Manjunath M B > >> +L: linux-mmc@vger.kernel.org > >> +S: Maintained > >> +F: drivers/mmc/host/sdhci-pci-dwc* > >> + > >> SYSTEM TRACE MODULE CLASS > >> M: Alexander Shishkin > >> S: Maintained > >> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile > >> index af918d2..092a746 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-pci-dwc.o > >> 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-pci-core.c > >> b/drivers/mmc/host/sdhci-pci-core.c > >> index 79e1901..4a7769c 100644 > >> --- a/drivers/mmc/host/sdhci-pci-core.c > >> +++ b/drivers/mmc/host/sdhci-pci-core.c > >> @@ -31,6 +31,7 @@ > >> #include "sdhci.h" > >> #include "sdhci-pci.h" > >> #include "sdhci-pci-o2micro.h" > >> +#include "sdhci-pci-dwc.h" > >> > >> > /********************************************************** > *******************\ > >> * * > >> @@ -70,6 +71,11 @@ static int ricoh_mmc_resume(struct sdhci_pci_chip > *chip) > >> msleep(500); > >> return 0; > >> } > >> +/* Synopsys SDHCI compliant Controller */ static const struct > >> +sdhci_pci_fixes sdhci_snps = { > >> + .probe_slot = sdhci_pci_probe_slot_snps, > >> + .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN, > >> +}; > >> > >> static const struct sdhci_pci_fixes sdhci_ricoh = { > >> .probe = ricoh_probe, > >> @@ -797,6 +803,14 @@ static const struct sdhci_pci_fixes sdhci_amd = > >> { > >> > >> static const struct pci_device_id pci_ids[] = { > >> { > >> + .vendor = PCI_VENDOR_ID_SYNOPSYS, > >> + .device = 0xc201, /* Device ID of sdhci-dwc on > Haps51 */ > >> + .subvendor = PCI_ANY_ID, > >> + .subdevice = PCI_ANY_ID, > >> + .driver_data = (kernel_ulong_t)&sdhci_snps, > >> + }, > >> + > >> + { > >> .vendor = PCI_VENDOR_ID_RICOH, > >> .device = PCI_DEVICE_ID_RICOH_R5C822, > >> .subvendor = PCI_ANY_ID, > >> diff --git a/drivers/mmc/host/sdhci-pci-dwc.c > >> b/drivers/mmc/host/sdhci-pci-dwc.c > >> new file mode 100644 > >> index 0000000..3490af8 > >> --- /dev/null > >> +++ b/drivers/mmc/host/sdhci-pci-dwc.c > >> @@ -0,0 +1,260 @@ > >> +/* > >> + * Copyright (C) 2016 Synopsys, Inc. > >> + * > >> + * Author: Manjunath M B > >> + * > >> + * 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 > >> +#include > >> +#include > >> +#include > >> + > >> +#include "sdhci.h" > >> +#include "sdhci-pci.h" > >> +#include "sdhci-pci-dwc.h" > >> + > >> > +/********************************************************* > ********************\ > >> + * * > >> + * Hardware specific clock handling * > >> + * * > >> > +\********************************************************* > ********************/ > >> +static const struct sdhci_ops *sdhci_ops; /* Low level hw interface */ > >> + > >> +static void snps_reset_dcm(struct sdhci_host *host, > >> + unsigned int mask, unsigned char reset) > > unsigned int/unsigned char -> u32/u8 ? > OK, We will update it accordingly. > > >> +{ > >> + u32 reg = 0; > >> + u16 vendor_ptr = 0; > > > > These variables can't be used uninitialized so no need to initialize > > them. > > OK, We will update it accordingly. > >> + > >> + 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, > >> + unsigned int clock) > >> +{ > >> + int div = 0; > >> + int mul = 0; > >> + int div_val = 0; > >> + int mul_val = 0; > >> + int mul_div_val = 0; > >> + u32 reg = 0; > >> + u16 clk = 0; > >> + u16 vendor_ptr = 0; > >> + u32 mask = 0; > >> + 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; > >> + > > > > Some variables don't need to be initialized here. > > tx/rx_clk_phase_val don't need? > i didn't see any modifying these vales. > > And some variables don't need to define..can be reused. > OK, We will update it accordingly. > > > >> + /* > >> + * 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 > >> + */ > >> + > >> + if (clock <= 25000000) { > >> + /* Then call generic set_clock */ > >> + if (sdhci_ops->set_clock) > >> + sdhci_ops->set_clock(host, clock); > > > > I am not sure about this part. You may simply call sdhci_set_clock() > > instead of sdhci_ops->set_clock(). See my latest comment, it is not > > clear what is behind set_clock(). > > As we have built our own clocking logic using DCM, we want to use our own set_clock function for the frequency above 25MHz. For lower frequencies, our clocking module program is same as sdhci_set_clock(). So we are calling sdhci_ops->set_clock() which is same as sdhci_set_clock(). > >> + } else { > >> + > >> + host->mmc->actual_clock = 0; > >> + vendor_ptr = sdhci_readw(host, SDHCI_UHS2_VENDOR); > >> + > >> + /* 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_writel(host, reg, (SDHC_GPIO_OUT + vendor_ptr)); > >> + mdelay(10); > >> + > >> + sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL); > >> + > >> + /* 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 > >> + * 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 > >> + */ > >> + > >> + mask = SDHC_CARD_TX_CLK_DCM_RST; > >> + snps_reset_dcm(host, mask, 1); > >> + > >> + mul_div_val = (mul_val << 8) | div_val; > >> + sdhci_writew(host, mul_div_val, > TXRX_CLK_DCM_MUL_DIV_DRP); > >> + > >> + reg = sdhci_readl(host, TXRX_CLK_DCM_DRP_BASE_51); > >> + > >> + snps_reset_dcm(host, mask, 0); > >> + > >> + 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); > >> + > >> + /* > >> + * This Clock might have affected the TX CLOCK DCM and RX > CLOCK > >> + * DCM which are used for Phase control; Reset these DCM's > >> + * for proper clock output > >> + * > >> + * Step 1: Reset the DCM > >> + * Step 2: De-Assert reset to DCM > >> + */ > >> + > >> + mask = SDHC_TUNING_TX_CLK_DCM_RST | > SDHC_TUNING_RX_CLK_DCM_RST; > >> + snps_reset_dcm(host, mask, 1); > >> + mdelay(10); > >> + snps_reset_dcm(host, mask, 0); > >> + > >> + /* Select working phase value if clock is <= 50MHz */ > >> + if (clock <= 50000000) { > >> + /*Change the Tx Phase value here */ > >> + reg = sdhci_readl(host, (SDHC_GPIO_OUT + > vendor_ptr)); > >> + reg |= (SDHC_TUNING_TX_CLK_SEL_MASK & > >> + (tx_clk_phase_val << > SDHC_TUNING_TX_CLK_SEL_SHIFT)); > >> + > >> + sdhci_writel(host, reg, (SDHC_GPIO_OUT + > vendor_ptr)); > >> + mdelay(10); > >> + > >> + /* Program to select phase shifted clock */ > >> + reg |= SDHC_TX_CLK_SEL_TUNED; > >> + sdhci_writel(host, reg, (SDHC_GPIO_OUT + > vendor_ptr)); > >> + > >> + /* > >> + * For 50Mhz, tuning is not possible. > >> + * Lets fix the sampling Phase of Rx Clock here. > >> + */ > >> + reg = sdhci_readl(host, (SDHC_DBOUNCE + > vendor_ptr)); > >> + reg &= ~SDHC_TUNING_RX_CLK_SEL_MASK; > >> + reg |= (SDHC_TUNING_RX_CLK_SEL_MASK & > >> + rx_clk_phase_val); > >> + sdhci_writel(host, reg, (SDHC_DBOUNCE + > vendor_ptr)); > >> + } > >> + mdelay(10); > >> + } > >> +} > >> + > >> +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 mask = 0; > >> + > > > > Some variables don't need initialization here or you can directly > > assign the right value: int div = 2, mul = 2, etc. > > I think div/mul variables don't need to define, right? > and mul_val and div_val also don't need. > > Finally, mul,div didn't need..just use mul_val and div_val are 1? > It seems that mul_val and div_val are already fixed. (just 1) > > > > >> + /* Configure the BCLK DRP to get 100 MHZ Clock */ > > Combine with the below comment. OK. We will update it accordingly. > > >> + > >> + /* > >> + * 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: 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 > >> + */ > >> + mask = SDHC_BCLK_DCM_RST; > >> + snps_reset_dcm(host, mask, 1); > > snps_reset_dcm(host, SDHC_BCLK_DCM_RST, 1); > OK, We will update it accordingly. > >> + > >> + mul_div_val = (mul_val << 8) | div_val; > > > mul_div_val is also fixed? > > mul_val = 1, div_val = 1..then mul_div_val is 0x101? > As of now, mul_val and div_val are fixed to select 100MHz input frequency. But, If we need to increase the frequency in future, we need to modify the mul_val and div_val. Anyway, I can use constant "mul_div_val" for the current setup. > > >> + sdhci_writew(host, mul_div_val, BCLK_DCM_MUL_DIV_DRP); > >> + > >> + reg = sdhci_readl(host, BCLK_DCM_DRP_BASE_51); > > just read? then > sdhci_readl(host, BCLK_DCM_DRP_BASE_51);? > > If my comments are right, you can remove above parameters. > (div, mul, div_val, mul_val, mul_div_val, reg, mask..all) OK, We will update it accordingly. > > Best Regards, > Jaehoon Chung > > >> + > >> + snps_reset_dcm(host, mask, 0); > >> + > >> + /* > >> + * By Default Clocks to Controller 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; > >> + > >> +} > >> +static struct sdhci_ops sdhci_pci_ops_snps = { > >> + .set_clock = sdhci_set_clock_snps, > >> +}; > >> + > >> +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; > >> + > >> + sdhci_pci_ops_snps.enable_dma = sdhci_ops- > >enable_dma; > >> + sdhci_pci_ops_snps.set_bus_width = sdhci_ops->set_bus_width; > >> + sdhci_pci_ops_snps.reset = sdhci_ops->reset; > >> + sdhci_pci_ops_snps.set_uhs_signaling = sdhci_ops- > >set_uhs_signaling; > >> + sdhci_pci_ops_snps.hw_reset = sdhci_ops->hw_reset; > >> + > >> + host->ops = &sdhci_pci_ops_snps; > > > > I am lost. sdhci_ops is a kind of backup of the original ops from pci > > core. Your sdhci_pci_ops_snps is a copy of ops from pci core excepting > > set_clock and finally you update pci core ops with sdhci_pci_ops_snps. > > > > It seems a bit complex. At the end, unless I have missed something, > > you have only changed set_clock from pci ops. > > You are correct, sdhci_pci_ops_snps is using pci core ops except set_clock. As I mentioned at the top, We have built our own clocking logic using DCM. So, we have to use our own set_clock function and We are reusing the pci core ops for the reaming ops. > > > > Regards > > > > Ludovic > > > >> + > >> + /* 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-pci-dwc.h > >> b/drivers/mmc/host/sdhci-pci-dwc.h > >> new file mode 100644 > >> index 0000000..02e2213 > >> --- /dev/null > >> +++ b/drivers/mmc/host/sdhci-pci-dwc.h > >> @@ -0,0 +1,55 @@ > >> +/* > >> + * Copyright (C) 2016 Synopsys, Inc. > >> + * > >> + * Author: Manjunath M B > >> + * > >> + * 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-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_MASK 0x000000FF > >> +#define SDHC_GPIO_OUT 0x34 > >> +/* HAPS 51 Based implementation */ > >> +#define SDHC_BCLK_DCM_RST 0x00000001 > >> +#define SDHC_CARD_TX_CLK_DCM_RST 0x00000002 > >> +#define SDHC_TUNING_RX_CLK_DCM_RST 0x00000004 > >> +#define SDHC_TUNING_TX_CLK_DCM_RST 0x00000008 > >> +#define SDHC_TUNING_TX_CLK_SEL_MASK 0x00000070 > >> +#define SDHC_TUNING_TX_CLK_SEL_SHIFT 4 > >> +#define SDHC_TX_CLK_SEL_TUNED 0x00000080 > >> + > >> +/* 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 > >> + > >> +/* Offset of TX and RX CLK DCM DRP */ > >> +#define TXRX_CLK_DCM_DRP_BASE_51 0x2000 > >> +#define TXRX_CLK_DCM_MUL_DIV_DRP 0x2050 > >> + > >> +int sdhci_pci_probe_slot_snps(struct sdhci_pci_slot *slot); > >> + > >> +#endif > >> -- > >> 1.9.1 > >> > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-mmc" > > in the body of a message to majordomo@vger.kernel.org More > majordomo > > info at http://vger.kernel.org/majordomo-info.html > > > > Regards, Prabu Thangamuthu.