diff mbox

[PATCHv3,3/4] mmc: dw_mmc-socfpga: Clean up SOCFPGA platform specific funcationality

Message ID 1386197576-3825-4-git-send-email-dinguyen@altera.com (mailing list archive)
State New, archived
Headers show

Commit Message

Dinh Nguyen Dec. 4, 2013, 10:52 p.m. UTC 
From: Dinh Nguyen <dinguyen@altera.com>

The SDR timing registers for the SD/MMC IP block for SOCFPGA is located
in the system manager. This system manager IP block is located outside of
the SD IP block itself. We can use the normal clock API to set the SDR
settings.

Also, there is no need for "altr,dw-mshc-ciu-div" as the driver can get
the value of the CIU clock from the common clock API.

Signed-off-by: Dinh Nguyen <dinguyen@altera.com>
---
v3: none
v2: none
---
 drivers/mmc/host/dw_mmc-socfpga.c |   78 +++++--------------------------------
 1 file changed, 10 insertions(+), 68 deletions(-)

Comments

Mark Rutland Dec. 5, 2013, 11:47 a.m. UTC | #1 
On Wed, Dec 04, 2013 at 10:52:55PM +0000, dinguyen@altera.com wrote:
> From: Dinh Nguyen <dinguyen@altera.com>
> 
> The SDR timing registers for the SD/MMC IP block for SOCFPGA is located
> in the system manager. This system manager IP block is located outside of
> the SD IP block itself. We can use the normal clock API to set the SDR
> settings.
> 
> Also, there is no need for "altr,dw-mshc-ciu-div" as the driver can get
> the value of the CIU clock from the common clock API.

If this property isn't necessary, please mark it as deprecated in the
documentation.

[...]

> +	if (IS_ERR(sysmgr_clk))
> +		dev_err(host->dev, "sysmgr-sdr-mmc not available\n");
> +	else {
> +		clk_disable_unprepare(host->ciu_clk);
> +		clk_prepare_enable(sysmgr_clk);
> +		clk_prepare_enable(host->ciu_clk);
> +	}
>  	return 0;
>  }

This looks a little odd. Should you not return an error if you don't
have the requisite clocks?

[...]

> -	priv->sysreg = syscon_regmap_lookup_by_compatible("altr,sys-mgr");
> -	if (IS_ERR(priv->sysreg)) {
> -		dev_err(host->dev, "regmap for altr,sys-mgr lookup failed.\n");
> -		return PTR_ERR(priv->sysreg);
> -	}

Is this property deprecated?

> -
> -	ret = of_property_read_u32(np, "altr,dw-mshc-ciu-div", &div);
> -	if (ret)
> -		dev_info(host->dev, "No dw-mshc-ciu-div specified, assuming 1");
> -	priv->ciu_div = div;
> -
> -	ret = of_property_read_u32_array(np,
> -			"altr,dw-mshc-sdr-timing", timing, 2);

Deprecated too?

Thanks,
Mark.
Dinh Nguyen Dec. 5, 2013, 4:18 p.m. UTC | #2 
On Thu, 2013-12-05 at 11:47 +0000, Mark Rutland wrote:
> On Wed, Dec 04, 2013 at 10:52:55PM +0000, dinguyen@altera.com wrote:
> > From: Dinh Nguyen <dinguyen@altera.com>
> > 
> > The SDR timing registers for the SD/MMC IP block for SOCFPGA is located
> > in the system manager. This system manager IP block is located outside of
> > the SD IP block itself. We can use the normal clock API to set the SDR
> > settings.
> > 
> > Also, there is no need for "altr,dw-mshc-ciu-div" as the driver can get
> > the value of the CIU clock from the common clock API.
> 
> If this property isn't necessary, please mark it as deprecated in the
> documentation.

I would deprecate it. If only there was documentation for it.

> 
> [...]
> 
> > +	if (IS_ERR(sysmgr_clk))
> > +		dev_err(host->dev, "sysmgr-sdr-mmc not available\n");
> > +	else {
> > +		clk_disable_unprepare(host->ciu_clk);
> > +		clk_prepare_enable(sysmgr_clk);
> > +		clk_prepare_enable(host->ciu_clk);
> > +	}
> >  	return 0;
> >  }
> 
> This looks a little odd. Should you not return an error if you don't
> have the requisite clocks?
> 
> [...]
> 
> > -	priv->sysreg = syscon_regmap_lookup_by_compatible("altr,sys-mgr");
> > -	if (IS_ERR(priv->sysreg)) {
> > -		dev_err(host->dev, "regmap for altr,sys-mgr lookup failed.\n");
> > -		return PTR_ERR(priv->sysreg);
> > -	}
> 
> Is this property deprecated?

"altr,sys-mgr" is used in other places, so no deprecated is necessary.

> 
> > -
> > -	ret = of_property_read_u32(np, "altr,dw-mshc-ciu-div", &div);
> > -	if (ret)
> > -		dev_info(host->dev, "No dw-mshc-ciu-div specified, assuming 1");
> > -	priv->ciu_div = div;
> > -
> > -	ret = of_property_read_u32_array(np,
> > -			"altr,dw-mshc-sdr-timing", timing, 2);
> 
> Deprecated too?

Once again "altr,dw-mshc-sdr-timing" was not documented. But either way,
v4 of this patch will not be introducing any new bindings. Thanks to
Arnd's suggestion, I found a way to hook into the existing socfpga clock
driver.

Thanks alot for your review!

Dinh

> 
> Thanks,
> Mark.
>
diff mbox

Patch

diff --git a/drivers/mmc/host/dw_mmc-socfpga.c b/drivers/mmc/host/dw_mmc-socfpga.c
index 3e8e53a..d790cc0 100644
--- a/drivers/mmc/host/dw_mmc-socfpga.c
+++ b/drivers/mmc/host/dw_mmc-socfpga.c
@@ -13,95 +13,37 @@ 
  * Taken from dw_mmc-exynos.c
  */
 #include <linux/clk.h>
-#include <linux/mfd/syscon.h>
 #include <linux/mmc/host.h>
 #include <linux/mmc/dw_mmc.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
-#include <linux/regmap.h>
 
 #include "dw_mmc.h"
 #include "dw_mmc-pltfm.h"
 
-#define SYSMGR_SDMMCGRP_CTRL_OFFSET		0x108
-#define DRV_CLK_PHASE_SHIFT_SEL_MASK	0x7
-#define SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel)          \
-	((((smplsel) & 0x7) << 3) | (((drvsel) & 0x7) << 0))
-
-/* SOCFPGA implementation specific driver private data */
-struct dw_mci_socfpga_priv_data {
-	u8	ciu_div; /* card interface unit divisor */
-	u32	hs_timing; /* bitmask for CIU clock phase shift */
-	struct regmap   *sysreg; /* regmap for system manager register */
-};
-
-static int dw_mci_socfpga_priv_init(struct dw_mci *host)
-{
-	return 0;
-}
-
 static int dw_mci_socfpga_setup_clock(struct dw_mci *host)
 {
-	struct dw_mci_socfpga_priv_data *priv = host->priv;
-
-	clk_disable_unprepare(host->ciu_clk);
-	regmap_write(priv->sysreg, SYSMGR_SDMMCGRP_CTRL_OFFSET,
-		priv->hs_timing);
-	clk_prepare_enable(host->ciu_clk);
-
-	host->bus_hz /= (priv->ciu_div + 1);
+	struct clk *sysmgr_clk = devm_clk_get(host->dev, "sysmgr-sdr-mmc");
+
+	if (IS_ERR(sysmgr_clk))
+		dev_err(host->dev, "sysmgr-sdr-mmc not available\n");
+	else {
+		clk_disable_unprepare(host->ciu_clk);
+		clk_prepare_enable(sysmgr_clk);
+		clk_prepare_enable(host->ciu_clk);
+	}
 	return 0;
 }
 
 static void dw_mci_socfpga_prepare_command(struct dw_mci *host, u32 *cmdr)
 {
-	struct dw_mci_socfpga_priv_data *priv = host->priv;
-
-	if (priv->hs_timing & DRV_CLK_PHASE_SHIFT_SEL_MASK)
-		*cmdr |= SDMMC_CMD_USE_HOLD_REG;
-}
-
-static int dw_mci_socfpga_parse_dt(struct dw_mci *host)
-{
-	struct dw_mci_socfpga_priv_data *priv;
-	struct device_node *np = host->dev->of_node;
-	u32 timing[2];
-	u32 div = 0;
-	int ret;
-
-	priv = devm_kzalloc(host->dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv) {
-		dev_err(host->dev, "mem alloc failed for private data\n");
-		return -ENOMEM;
-	}
-
-	priv->sysreg = syscon_regmap_lookup_by_compatible("altr,sys-mgr");
-	if (IS_ERR(priv->sysreg)) {
-		dev_err(host->dev, "regmap for altr,sys-mgr lookup failed.\n");
-		return PTR_ERR(priv->sysreg);
-	}
-
-	ret = of_property_read_u32(np, "altr,dw-mshc-ciu-div", &div);
-	if (ret)
-		dev_info(host->dev, "No dw-mshc-ciu-div specified, assuming 1");
-	priv->ciu_div = div;
-
-	ret = of_property_read_u32_array(np,
-			"altr,dw-mshc-sdr-timing", timing, 2);
-	if (ret)
-		return ret;
-
-	priv->hs_timing = SYSMGR_SDMMC_CTRL_SET(timing[0], timing[1]);
-	host->priv = priv;
-	return 0;
+	*cmdr |= SDMMC_CMD_USE_HOLD_REG;
 }
 
 static const struct dw_mci_drv_data socfpga_drv_data = {
-	.init			= dw_mci_socfpga_priv_init,
 	.setup_clock		= dw_mci_socfpga_setup_clock,
 	.prepare_command	= dw_mci_socfpga_prepare_command,
-	.parse_dt		= dw_mci_socfpga_parse_dt,
 };
 
 static const struct of_device_id dw_mci_socfpga_match[] = {