Re: [PATCH v2] mmc: sdhci-pci: add Support of Synopsys DWC_MSHC IP

[Jaehoon Chung]

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 <prabu.t@xxxxxxxxxxxx>
>>> ---
>>> 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 <prabu.t@xxxxxxxxxxxx>
>>> +M:	Manjunath M B <manjumb@xxxxxxxxxxxx>
>>> +L:	linux-mmc@xxxxxxxxxxxxxxx
>>> +S:	Maintained
>>> +F:	drivers/mmc/host/sdhci-dwc-mshc*
>>> +
>>>  SYSTEM TRACE MODULE CLASS
>>>  M:	Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
>>>  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 <manjumb@xxxxxxxxxxxx>
>>> + *
>>> + * 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 <manjumb@xxxxxxxxxxxx>
>>> + *
>>> + * 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.
> 
> 
>