| Message ID | 1448500204-2969-1-git-send-email-clsee@altera.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
On Thursday, November 26, 2015 at 02:10:04 AM, Chin Liang See wrote: > socfpga_dw_mmc driver will obtain the drvsel and > smplsel value from device tree instead of definition > in config header file. > > Signed-off-by: Chin Liang See <clsee@altera.com> > Cc: Dinh Nguyen <dinguyen@opensource.altera.com> > Cc: Dinh Nguyen <dinh.linux@gmail.com> > Cc: Pavel Machek <pavel@denx.de> > Cc: Marek Vasut <marex@denx.de> > Cc: Stefan Roese <sr@denx.de> > Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: Jaehoon Chung <jh80.chung@samsung.com> > --- > Changes for v2 > - Put default value for drvsel to 3 in case node in DT missing > - Remove unnecessary ad-hoc vairable > - Free up first calloc if second calloc failed > --- > arch/arm/dts/socfpga_cyclone5.dtsi | 2 ++ > drivers/mmc/socfpga_dw_mmc.c | 22 ++++++++++++++++++++-- > include/configs/socfpga_common.h | 2 -- > 3 files changed, 22 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/dts/socfpga_cyclone5.dtsi > b/arch/arm/dts/socfpga_cyclone5.dtsi index de36209..040b236 100644 > --- a/arch/arm/dts/socfpga_cyclone5.dtsi > +++ b/arch/arm/dts/socfpga_cyclone5.dtsi > @@ -25,6 +25,8 @@ > bus-width = <4>; > cap-mmc-highspeed; > cap-sd-highspeed; > + drvsel = <3>; > + smplsel = <0>; > }; > > sysmgr@ffd08000 { I'm not very fond of bunding DT changes and driver changes together, these should be separate, so it'd be nice if you sent a V3 and split this into two patches. Otherwise Acked-by: Marek Vasut <marex@denx.de> > diff --git a/drivers/mmc/socfpga_dw_mmc.c b/drivers/mmc/socfpga_dw_mmc.c > index 8076761..38eb783 100644 > --- a/drivers/mmc/socfpga_dw_mmc.c > +++ b/drivers/mmc/socfpga_dw_mmc.c > @@ -19,18 +19,25 @@ static const struct socfpga_clock_manager > *clock_manager_base = static const struct socfpga_system_manager > *system_manager_base = (void *)SOCFPGA_SYSMGR_ADDRESS; > > +/* socfpga implmentation specific drver private data */ > +struct dwmci_socfpga_priv_data { > + unsigned int drvsel; > + unsigned int smplsel; > +}; > + > static void socfpga_dwmci_clksel(struct dwmci_host *host) > { > unsigned int drvsel; > unsigned int smplsel; > + struct dwmci_socfpga_priv_data *priv = host->priv; > > /* Disable SDMMC clock. */ > clrbits_le32(&clock_manager_base->per_pll.en, > CLKMGR_PERPLLGRP_EN_SDMMCCLK_MASK); > > /* Configures drv_sel and smpl_sel */ > - drvsel = CONFIG_SOCFPGA_DWMMC_DRVSEL; > - smplsel = CONFIG_SOCFPGA_DWMMC_SMPSEL; > + drvsel = priv->drvsel; > + smplsel = priv->smplsel; You can probably just use priv->xxx directly in the code and drop those vars. > debug("%s: drvsel %d smplsel %d\n", __func__, drvsel, smplsel); > writel(SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel), [...]
On Thu, 2015-11-26 at 02:26 +0100, Marek Vasut wrote: > On Thursday, November 26, 2015 at 02:10:04 AM, Chin Liang See wrote: > > socfpga_dw_mmc driver will obtain the drvsel and > > smplsel value from device tree instead of definition > > in config header file. > > > > Signed-off-by: Chin Liang See <clsee@altera.com> > > Cc: Dinh Nguyen <dinguyen@opensource.altera.com> > > Cc: Dinh Nguyen <dinh.linux@gmail.com> > > Cc: Pavel Machek <pavel@denx.de> > > Cc: Marek Vasut <marex@denx.de> > > Cc: Stefan Roese <sr@denx.de> > > Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com> > > Cc: Simon Glass <sjg@chromium.org> > > Cc: Jaehoon Chung <jh80.chung@samsung.com> > > --- > > Changes for v2 > > - Put default value for drvsel to 3 in case node in DT missing > > - Remove unnecessary ad-hoc vairable > > - Free up first calloc if second calloc failed > > --- > > arch/arm/dts/socfpga_cyclone5.dtsi | 2 ++ > > drivers/mmc/socfpga_dw_mmc.c | 22 ++++++++++++++++++++-- > > include/configs/socfpga_common.h | 2 -- > > 3 files changed, 22 insertions(+), 4 deletions(-) > > > > diff --git a/arch/arm/dts/socfpga_cyclone5.dtsi > > b/arch/arm/dts/socfpga_cyclone5.dtsi index de36209..040b236 100644 > > --- a/arch/arm/dts/socfpga_cyclone5.dtsi > > +++ b/arch/arm/dts/socfpga_cyclone5.dtsi > > @@ -25,6 +25,8 @@ > > bus-width = <4>; > > cap-mmc-highspeed; > > cap-sd-highspeed; > > + drvsel = <3>; > > + smplsel = <0>; > > }; > > > > sysmgr@ffd08000 { > > I'm not very fond of bunding DT changes and driver changes together, > these > should be separate, so it'd be nice if you sent a V3 and split this > into > two patches. > > Otherwise > Acked-by: Marek Vasut <marex@denx.de> Sure, I can split that as its easy to do that. > > > diff --git a/drivers/mmc/socfpga_dw_mmc.c > > b/drivers/mmc/socfpga_dw_mmc.c > > index 8076761..38eb783 100644 > > --- a/drivers/mmc/socfpga_dw_mmc.c > > +++ b/drivers/mmc/socfpga_dw_mmc.c > > @@ -19,18 +19,25 @@ static const struct socfpga_clock_manager > > *clock_manager_base = static const struct socfpga_system_manager > > *system_manager_base = (void *)SOCFPGA_SYSMGR_ADDRESS; > > > > +/* socfpga implmentation specific drver private data */ > > +struct dwmci_socfpga_priv_data { > > + unsigned int drvsel; > > + unsigned int smplsel; > > +}; > > + > > static void socfpga_dwmci_clksel(struct dwmci_host *host) > > { > > unsigned int drvsel; > > unsigned int smplsel; > > + struct dwmci_socfpga_priv_data *priv = host->priv; > > > > /* Disable SDMMC clock. */ > > clrbits_le32(&clock_manager_base->per_pll.en, > > CLKMGR_PERPLLGRP_EN_SDMMCCLK_MASK); > > > > /* Configures drv_sel and smpl_sel */ > > - drvsel = CONFIG_SOCFPGA_DWMMC_DRVSEL; > > - smplsel = CONFIG_SOCFPGA_DWMMC_SMPSEL; > > + drvsel = priv->drvsel; > > + smplsel = priv->smplsel; > > You can probably just use priv->xxx directly in the code and drop > those vars. Sure, I can enhance that too Thanks Chin Liang > > > debug("%s: drvsel %d smplsel %d\n", __func__, drvsel, > > smplsel); > > writel(SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel), > [...]
On 11/26/2015 10:10 AM, Chin Liang See wrote: > socfpga_dw_mmc driver will obtain the drvsel and > smplsel value from device tree instead of definition > in config header file. > > Signed-off-by: Chin Liang See <clsee@altera.com> > Cc: Dinh Nguyen <dinguyen@opensource.altera.com> > Cc: Dinh Nguyen <dinh.linux@gmail.com> > Cc: Pavel Machek <pavel@denx.de> > Cc: Marek Vasut <marex@denx.de> > Cc: Stefan Roese <sr@denx.de> > Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: Jaehoon Chung <jh80.chung@samsung.com> > --- > Changes for v2 > - Put default value for drvsel to 3 in case node in DT missing > - Remove unnecessary ad-hoc vairable > - Free up first calloc if second calloc failed > --- > arch/arm/dts/socfpga_cyclone5.dtsi | 2 ++ > drivers/mmc/socfpga_dw_mmc.c | 22 ++++++++++++++++++++-- > include/configs/socfpga_common.h | 2 -- > 3 files changed, 22 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/dts/socfpga_cyclone5.dtsi b/arch/arm/dts/socfpga_cyclone5.dtsi > index de36209..040b236 100644 > --- a/arch/arm/dts/socfpga_cyclone5.dtsi > +++ b/arch/arm/dts/socfpga_cyclone5.dtsi > @@ -25,6 +25,8 @@ > bus-width = <4>; > cap-mmc-highspeed; > cap-sd-highspeed; > + drvsel = <3>; > + smplsel = <0>; > }; > > sysmgr@ffd08000 { > diff --git a/drivers/mmc/socfpga_dw_mmc.c b/drivers/mmc/socfpga_dw_mmc.c > index 8076761..38eb783 100644 > --- a/drivers/mmc/socfpga_dw_mmc.c > +++ b/drivers/mmc/socfpga_dw_mmc.c > @@ -19,18 +19,25 @@ static const struct socfpga_clock_manager *clock_manager_base = > static const struct socfpga_system_manager *system_manager_base = > (void *)SOCFPGA_SYSMGR_ADDRESS; > > +/* socfpga implmentation specific drver private data */ > +struct dwmci_socfpga_priv_data { > + unsigned int drvsel; > + unsigned int smplsel; > +}; > + > static void socfpga_dwmci_clksel(struct dwmci_host *host) > { > unsigned int drvsel; > unsigned int smplsel; > + struct dwmci_socfpga_priv_data *priv = host->priv; > > /* Disable SDMMC clock. */ > clrbits_le32(&clock_manager_base->per_pll.en, > CLKMGR_PERPLLGRP_EN_SDMMCCLK_MASK); > > /* Configures drv_sel and smpl_sel */ > - drvsel = CONFIG_SOCFPGA_DWMMC_DRVSEL; > - smplsel = CONFIG_SOCFPGA_DWMMC_SMPSEL; > + drvsel = priv->drvsel; > + smplsel = priv->smplsel; I found that drvsel and smplsel are used at only here. I think they should be removed. (priv->drvsel/smplse don't need to assign to drvsel/smplsel.) how about? Best Regards, Jaehoon Chung > > debug("%s: drvsel %d smplsel %d\n", __func__, drvsel, smplsel); > writel(SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel), > @@ -50,6 +57,7 @@ static int socfpga_dwmci_of_probe(const void *blob, int node, const int idx) > const unsigned long clk = cm_get_mmc_controller_clk_hz(); > > struct dwmci_host *host; > + struct dwmci_socfpga_priv_data *priv; > fdt_addr_t reg_base; > int bus_width, fifo_depth; > > @@ -83,6 +91,13 @@ static int socfpga_dwmci_of_probe(const void *blob, int node, const int idx) > if (!host) > return -ENOMEM; > > + /* Allocate the priv */ > + priv = calloc(1, sizeof(*priv)); > + if (!priv) { > + free(host); > + return -ENOMEM; > + } > + > host->name = "SOCFPGA DWMMC"; > host->ioaddr = (void *)reg_base; > host->buswidth = bus_width; > @@ -92,6 +107,9 @@ static int socfpga_dwmci_of_probe(const void *blob, int node, const int idx) > host->bus_hz = clk; > host->fifoth_val = MSIZE(0x2) | > RX_WMARK(fifo_depth / 2 - 1) | TX_WMARK(fifo_depth / 2); > + priv->drvsel = fdtdec_get_uint(blob, node, "drvsel", 3); > + priv->smplsel = fdtdec_get_uint(blob, node, "smplsel", 0); > + host->priv = priv; > > return add_dwmci(host, host->bus_hz, 400000); > } > diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h > index f6808b5..b661cc2 100644 > --- a/include/configs/socfpga_common.h > +++ b/include/configs/socfpga_common.h > @@ -153,8 +153,6 @@ > #define CONFIG_DWMMC > #define CONFIG_SOCFPGA_DWMMC > #define CONFIG_SOCFPGA_DWMMC_FIFO_DEPTH 1024 > -#define CONFIG_SOCFPGA_DWMMC_DRVSEL 3 > -#define CONFIG_SOCFPGA_DWMMC_SMPSEL 0 > /* FIXME */ > /* using smaller max blk cnt to avoid flooding the limited stack we have */ > #define CONFIG_SYS_MMC_MAX_BLK_COUNT 256 /* FIXME -- SPL only? */ >
On Thu, 2015-11-26 at 10:33 +0900, Jaehoon Chung wrote: > On 11/26/2015 10:10 AM, Chin Liang See wrote: > > socfpga_dw_mmc driver will obtain the drvsel and > > smplsel value from device tree instead of definition > > in config header file. > > > > Signed-off-by: Chin Liang See <clsee@altera.com> > > Cc: Dinh Nguyen <dinguyen@opensource.altera.com> > > Cc: Dinh Nguyen <dinh.linux@gmail.com> > > Cc: Pavel Machek <pavel@denx.de> > > Cc: Marek Vasut <marex@denx.de> > > Cc: Stefan Roese <sr@denx.de> > > Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com> > > Cc: Simon Glass <sjg@chromium.org> > > Cc: Jaehoon Chung <jh80.chung@samsung.com> > > --- > > Changes for v2 > > - Put default value for drvsel to 3 in case node in DT missing > > - Remove unnecessary ad-hoc vairable > > - Free up first calloc if second calloc failed > > --- > > arch/arm/dts/socfpga_cyclone5.dtsi | 2 ++ > > drivers/mmc/socfpga_dw_mmc.c | 22 ++++++++++++++++++++-- > > include/configs/socfpga_common.h | 2 -- > > 3 files changed, 22 insertions(+), 4 deletions(-) > > > > diff --git a/arch/arm/dts/socfpga_cyclone5.dtsi > > b/arch/arm/dts/socfpga_cyclone5.dtsi > > index de36209..040b236 100644 > > --- a/arch/arm/dts/socfpga_cyclone5.dtsi > > +++ b/arch/arm/dts/socfpga_cyclone5.dtsi > > @@ -25,6 +25,8 @@ > > bus-width = <4>; > > cap-mmc-highspeed; > > cap-sd-highspeed; > > + drvsel = <3>; > > + smplsel = <0>; > > }; > > > > sysmgr@ffd08000 { > > diff --git a/drivers/mmc/socfpga_dw_mmc.c > > b/drivers/mmc/socfpga_dw_mmc.c > > index 8076761..38eb783 100644 > > --- a/drivers/mmc/socfpga_dw_mmc.c > > +++ b/drivers/mmc/socfpga_dw_mmc.c > > @@ -19,18 +19,25 @@ static const struct socfpga_clock_manager > > *clock_manager_base = > > static const struct socfpga_system_manager *system_manager_base = > > (void *)SOCFPGA_SYSMGR_ADDRESS; > > > > +/* socfpga implmentation specific drver private data */ > > +struct dwmci_socfpga_priv_data { > > + unsigned int drvsel; > > + unsigned int smplsel; > > +}; > > + > > static void socfpga_dwmci_clksel(struct dwmci_host *host) > > { > > unsigned int drvsel; > > unsigned int smplsel; > > + struct dwmci_socfpga_priv_data *priv = host->priv; > > > > /* Disable SDMMC clock. */ > > clrbits_le32(&clock_manager_base->per_pll.en, > > CLKMGR_PERPLLGRP_EN_SDMMCCLK_MASK); > > > > /* Configures drv_sel and smpl_sel */ > > - drvsel = CONFIG_SOCFPGA_DWMMC_DRVSEL; > > - smplsel = CONFIG_SOCFPGA_DWMMC_SMPSEL; > > + drvsel = priv->drvsel; > > + smplsel = priv->smplsel; > > I found that drvsel and smplsel are used at only here. > I think they should be removed. > (priv->drvsel/smplse don't need to assign to drvsel/smplsel.) > > how about? > Yup, Marek just note this to me too. I will get rid the variable and use the priv directly Thanks Chin Liang > Best Regards, > Jaehoon Chung > > > > > debug("%s: drvsel %d smplsel %d\n", __func__, drvsel, > > smplsel); > > writel(SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel), > > @@ -50,6 +57,7 @@ static int socfpga_dwmci_of_probe(const void > > *blob, int node, const int idx) > > const unsigned long clk = cm_get_mmc_controller_clk_hz(); > > > > struct dwmci_host *host; > > + struct dwmci_socfpga_priv_data *priv; > > fdt_addr_t reg_base; > > int bus_width, fifo_depth; > > > > @@ -83,6 +91,13 @@ static int socfpga_dwmci_of_probe(const void > > *blob, int node, const int idx) > > if (!host) > > return -ENOMEM; > > > > + /* Allocate the priv */ > > + priv = calloc(1, sizeof(*priv)); > > + if (!priv) { > > + free(host); > > + return -ENOMEM; > > + } > > + > > host->name = "SOCFPGA DWMMC"; > > host->ioaddr = (void *)reg_base; > > host->buswidth = bus_width; > > @@ -92,6 +107,9 @@ static int socfpga_dwmci_of_probe(const void > > *blob, int node, const int idx) > > host->bus_hz = clk; > > host->fifoth_val = MSIZE(0x2) | > > RX_WMARK(fifo_depth / 2 - 1) | TX_WMARK(fifo_depth > > / 2); > > + priv->drvsel = fdtdec_get_uint(blob, node, "drvsel", 3); > > + priv->smplsel = fdtdec_get_uint(blob, node, "smplsel", 0); > > + host->priv = priv; > > > > return add_dwmci(host, host->bus_hz, 400000); > > } > > diff --git a/include/configs/socfpga_common.h > > b/include/configs/socfpga_common.h > > index f6808b5..b661cc2 100644 > > --- a/include/configs/socfpga_common.h > > +++ b/include/configs/socfpga_common.h > > @@ -153,8 +153,6 @@ > > #define CONFIG_DWMMC > > #define CONFIG_SOCFPGA_DWMMC > > #define CONFIG_SOCFPGA_DWMMC_FIFO_DEPTH 1024 > > -#define CONFIG_SOCFPGA_DWMMC_DRVSEL 3 > > -#define CONFIG_SOCFPGA_DWMMC_SMPSEL 0 > > /* FIXME */ > > /* using smaller max blk cnt to avoid flooding the limited stack > > we have */ > > #define CONFIG_SYS_MMC_MAX_BLK_COUNT 256 /* FIXME -- > > SPL only? */ > > >
diff --git a/arch/arm/dts/socfpga_cyclone5.dtsi b/arch/arm/dts/socfpga_cyclone5.dtsi index de36209..040b236 100644 --- a/arch/arm/dts/socfpga_cyclone5.dtsi +++ b/arch/arm/dts/socfpga_cyclone5.dtsi @@ -25,6 +25,8 @@ bus-width = <4>; cap-mmc-highspeed; cap-sd-highspeed; + drvsel = <3>; + smplsel = <0>; }; sysmgr@ffd08000 { diff --git a/drivers/mmc/socfpga_dw_mmc.c b/drivers/mmc/socfpga_dw_mmc.c index 8076761..38eb783 100644 --- a/drivers/mmc/socfpga_dw_mmc.c +++ b/drivers/mmc/socfpga_dw_mmc.c @@ -19,18 +19,25 @@ static const struct socfpga_clock_manager *clock_manager_base = static const struct socfpga_system_manager *system_manager_base = (void *)SOCFPGA_SYSMGR_ADDRESS; +/* socfpga implmentation specific drver private data */ +struct dwmci_socfpga_priv_data { + unsigned int drvsel; + unsigned int smplsel; +}; + static void socfpga_dwmci_clksel(struct dwmci_host *host) { unsigned int drvsel; unsigned int smplsel; + struct dwmci_socfpga_priv_data *priv = host->priv; /* Disable SDMMC clock. */ clrbits_le32(&clock_manager_base->per_pll.en, CLKMGR_PERPLLGRP_EN_SDMMCCLK_MASK); /* Configures drv_sel and smpl_sel */ - drvsel = CONFIG_SOCFPGA_DWMMC_DRVSEL; - smplsel = CONFIG_SOCFPGA_DWMMC_SMPSEL; + drvsel = priv->drvsel; + smplsel = priv->smplsel; debug("%s: drvsel %d smplsel %d\n", __func__, drvsel, smplsel); writel(SYSMGR_SDMMC_CTRL_SET(smplsel, drvsel), @@ -50,6 +57,7 @@ static int socfpga_dwmci_of_probe(const void *blob, int node, const int idx) const unsigned long clk = cm_get_mmc_controller_clk_hz(); struct dwmci_host *host; + struct dwmci_socfpga_priv_data *priv; fdt_addr_t reg_base; int bus_width, fifo_depth; @@ -83,6 +91,13 @@ static int socfpga_dwmci_of_probe(const void *blob, int node, const int idx) if (!host) return -ENOMEM; + /* Allocate the priv */ + priv = calloc(1, sizeof(*priv)); + if (!priv) { + free(host); + return -ENOMEM; + } + host->name = "SOCFPGA DWMMC"; host->ioaddr = (void *)reg_base; host->buswidth = bus_width; @@ -92,6 +107,9 @@ static int socfpga_dwmci_of_probe(const void *blob, int node, const int idx) host->bus_hz = clk; host->fifoth_val = MSIZE(0x2) | RX_WMARK(fifo_depth / 2 - 1) | TX_WMARK(fifo_depth / 2); + priv->drvsel = fdtdec_get_uint(blob, node, "drvsel", 3); + priv->smplsel = fdtdec_get_uint(blob, node, "smplsel", 0); + host->priv = priv; return add_dwmci(host, host->bus_hz, 400000); } diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h index f6808b5..b661cc2 100644 --- a/include/configs/socfpga_common.h +++ b/include/configs/socfpga_common.h @@ -153,8 +153,6 @@ #define CONFIG_DWMMC #define CONFIG_SOCFPGA_DWMMC #define CONFIG_SOCFPGA_DWMMC_FIFO_DEPTH 1024 -#define CONFIG_SOCFPGA_DWMMC_DRVSEL 3 -#define CONFIG_SOCFPGA_DWMMC_SMPSEL 0 /* FIXME */ /* using smaller max blk cnt to avoid flooding the limited stack we have */ #define CONFIG_SYS_MMC_MAX_BLK_COUNT 256 /* FIXME -- SPL only? */
socfpga_dw_mmc driver will obtain the drvsel and smplsel value from device tree instead of definition in config header file. Signed-off-by: Chin Liang See <clsee@altera.com> Cc: Dinh Nguyen <dinguyen@opensource.altera.com> Cc: Dinh Nguyen <dinh.linux@gmail.com> Cc: Pavel Machek <pavel@denx.de> Cc: Marek Vasut <marex@denx.de> Cc: Stefan Roese <sr@denx.de> Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com> Cc: Simon Glass <sjg@chromium.org> Cc: Jaehoon Chung <jh80.chung@samsung.com> --- Changes for v2 - Put default value for drvsel to 3 in case node in DT missing - Remove unnecessary ad-hoc vairable - Free up first calloc if second calloc failed --- arch/arm/dts/socfpga_cyclone5.dtsi | 2 ++ drivers/mmc/socfpga_dw_mmc.c | 22 ++++++++++++++++++++-- include/configs/socfpga_common.h | 2 -- 3 files changed, 22 insertions(+), 4 deletions(-)