PXA1928 SDHCI controller has few differences, for example,
PXAxxx PXA1928
====== =======
SDCLK_DELAY field 0x10A 0x114
SDCLK_DELAY mask 0x1F 0x3FF
SDCLK_DELAY shift 9 8
SDCLK_SEL shift 8 2 (SEL1)
So this patch series introduces new compatible device_id
(marvell,pxav3-1928-sdhci), and makes use of .data for handling
such differences.
The series also adds support like,
- independent ->set_clock() api, as we need to enable internal clock gate
and TX clock
- pinctrl configuration based on bus speed.
- introduce new quirk SDHCI_QUIRK2_MUST_SET_SDHCI_BUS_POWER
SD_BUS_POWER & SD_BUS_VLT bit-fields are used internally to gate the
clocks, so it is important to configure them as part of ->set_power()
More detailed description is written into commit log.
- Enable SDHCI_QUIRK_BROKEN_TIMEOUT_VAL for PXA1928 device_id
- Enable SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON quirk for pxa1928 device
V1 => V2:
=========
- Fixed typo issue (residual change remained by mistake)
- Added new patch updating DT binding document for pxa1928 support
- Added new patch enabling CARD_ON_NEEDS_BUS_ON for pxa1928 device
Testing:
I have done basic testing on both eMMC and SD card on PXA1928 based
platform.
Note: I tried to made sure that I do not break any other platform, which
used sdhci, except HS200 configuration.·
Unfortunately I do not have access to any other datasheets, where I can
cross check the details on HS200 bit-fields. Probably someone who has
access can confirm [PATCH 4/5], whether it impacts other platforms.
Kevin Liu (1):
mmc: sdhci-pxav3: Fix HS200 mode support
Vaibhav Hiremath (6):
mmc: sdhci-pxav3: Enable pxa1928 device support
mmc: sdhci-pxav3: binding: Add pxa1928 compatible support
mmc: sdhci-pxav3: Add platform specific set_clock ops
mmc: sdhci-pxav3: Add pinctl setting according to bus clock
mmc: sdhci: add new quirk for setting BUS_POWER & BUS_VLT fields
mmc: sdhci: enable SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON for pxa1928
.../devicetree/bindings/mmc/sdhci-pxa.txt | 2 +-
drivers/mmc/host/sdhci-pltfm.c | 4 +
drivers/mmc/host/sdhci-pxav3.c | 168 +++++++++++++++++++--
drivers/mmc/host/sdhci.c | 3 +-
drivers/mmc/host/sdhci.h | 2 +
5 files changed, 165 insertions(+), 14 deletions(-)
--
1.9.1
SDHCI controller present in PXA1928 has few differences as far as
register map is concerned.
For example,
PXAxxx PXA1928
====== =======
SDCLK_DELAY field 0x10A 0x114
SDCLK_DELAY mask 0x1F 0x3FF
SDCLK_DELAY shift 9 8
SDCLK_SEL shift 8 2 (SEL1)
So in order to support multi-platform, use sdhci_pxa_regdata structure
as a variant data according to platform.
Note that, there are some more differences, which would be added
as and when respective feature gets added to the driver.
Signed-off-by: Vaibhav Hiremath <redacted>
---
drivers/mmc/host/sdhci-pxav3.c | 62 ++++++++++++++++++++++++++++++++++--------
1 file changed, 51 insertions(+), 11 deletions(-)
@@ -58,11 +55,25 @@#define SDCE_MISC_INT BIT(2)#define SDCE_MISC_INT_EN BIT(1)+#define SD_RX_CFG_REG 0x114+/* IO Power control */#define IO_PWR_AKEY_ASFAR 0xbaba#define IO_PWR_AKEY_ASSAR 0xeb10#define IO_PWR_MMC1_PAD_1V8 BIT(2)+structsdhci_pxa_data{+u32sdclk_delay_reg;+u32sdclk_delay_mask;+u8sdclk_delay_shift;+u8sdclk_sel_mask;+u8sdclk_sel_shift;+/*+*Wehavefewmoredifferences,addthemalongwiththeir+*respectivefeaturesupport+*/+};+structsdhci_pxa{structclk*clk_core;structclk*clk_io;
@@ -70,6 +81,24 @@ struct sdhci_pxa {void__iomem*sdio3_conf_reg;void__iomem*io_pwr_reg;void__iomem*io_pwr_lock_reg;+structsdhci_pxa_data*data;+};++staticstructsdhci_pxa_datapxav3_data_v1={+.sdclk_delay_reg=SD_CLOCK_BURST_SIZE_SETUP,+.sdclk_delay_mask=0x1F,+.sdclk_delay_shift=9,+.sdclk_sel_mask=0x1,+.sdclk_sel_shift=8,+};++staticstructsdhci_pxa_datapxav3_data_v2={+.sdclk_delay_reg=SD_RX_CFG_REG,+.sdclk_delay_mask=0x3FF,+.sdclk_delay_shift=8,+/* Only set SDCLK_SEL1, as driver uses default value of SDCLK_SEL0 */+.sdclk_sel_mask=0x3,+.sdclk_sel_shift=2,/* SDCLK_SEL1 */};/*
In case of PXA1928 & family of devices, the TX BUS and internal clock
need to be set as part of ->set_clock() ops, so this patch adds
platform specific ->set_clock() operation.
Note that, in order to not break other platforms, this patch
introduced the flag, which controls whether controller/platform
specific clock configuration needs to be executed.
Signed-off-by: Vaibhav Hiremath <redacted>
---
drivers/mmc/host/sdhci-pxav3.c | 46 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 45 insertions(+), 1 deletion(-)
@@ -57,6 +61,9 @@#define SD_RX_CFG_REG 0x114+#define TX_CFG_REG 0x118+#define TX_INTERNAL_SEL_BUS_CLK BIT(30)+/* IO Power control */#define IO_PWR_AKEY_ASFAR 0xbaba#define IO_PWR_AKEY_ASSAR 0xeb10
@@ -68,6 +75,9 @@ struct sdhci_pxa_data {u8sdclk_delay_shift;u8sdclk_sel_mask;u8sdclk_sel_shift;++/* set this if platform needs separate clock configuration */+boolset_pltfrm_clk;/**Wehavefewmoredifferences,addthemalongwiththeir*respectivefeaturesupport
@@ -99,6 +110,7 @@ static struct sdhci_pxa_data pxav3_data_v2 = {/* Only set SDCLK_SEL1, as driver uses default value of SDCLK_SEL0 */.sdclk_sel_mask=0x3,.sdclk_sel_shift=2,/* SDCLK_SEL1 */+.set_pltfrm_clk=true,};/*
@@ -375,8 +387,40 @@ static void pxav3_voltage_switch(struct sdhci_host *host,writel(val,pxa->io_pwr_reg);}+staticvoidpxav3_set_tx_clock(structsdhci_host*host)+{+u32val;++val=sdhci_readl(host,TX_CFG_REG);+val|=TX_INTERNAL_SEL_BUS_CLK;+sdhci_writel(host,val,TX_CFG_REG);+}++staticvoidpxav3_set_clock(structsdhci_host*host,unsignedintclock)+{+structsdhci_pltfm_host*pltfm_host=sdhci_priv(host);+structsdhci_pxa*pxa=pltfm_host->priv;++/* We still use common sdhci_set_clock() */+sdhci_set_clock(host,clock);++/* platform/controller specific clock configuration */+if(pxa->data->set_pltfrm_clk&&clock!=0){+u32val;++val=sdhci_readw(host,SD_FIFO_PARAM);+/* Internal clock gate ON and CTRL = 0b11 */+val|=INTERNAL_CLK_GATE_CTRL|INTERNAL_CLK_GATE_ON;+sdhci_writew(host,val,SD_FIFO_PARAM);++/* TX internal clock selection */+pxav3_set_tx_clock(host);+}++}+staticconststructsdhci_opspxav3_sdhci_ops={-.set_clock=sdhci_set_clock,+.set_clock=pxav3_set_clock,.platform_send_init_74_clocks=pxav3_gen_init_74_clocks,.get_max_clock=sdhci_pltfm_clk_get_max_clock,.set_bus_width=sdhci_set_bus_width,
From: Kevin Liu <redacted>
IN case of MMC HS200 mode, current code does not enable
SD_CE_ATA_2.MMC_HS200 & SD_CE_ATA_2.MMC_CARD bit configurations.
So this patch updates the above bit fields correctly.
Signed-off-by: Tim Wang <redacted>
Signed-off-by: Kevin Liu <redacted>
Signed-off-by: Vaibhav Hiremath <redacted>
---
Note: Unfortunately I do not have access to any other datasheets
which uses sdhci-pxav3 driver, so quite not sure whether this would
break any existing platform, probably NOT, as I do not see any
references for this change.
If anyone can confirm that would be really great.
drivers/mmc/host/sdhci-pxav3.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
Card power is dependent on bus power, without that card
wouldn't respond (No CARD_INT). So this patch enables the
quirk SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON.
Signed-off-by: Vaibhav Hiremath <redacted>
---
drivers/mmc/host/sdhci-pltfm.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
IN case of Marvell 1928 family of devices, the SD_BUS_POWER and
SD_BUS_VLT bits are used internally to gate the clocks, so
we have to set these fields.
Pasting Spec words here,
The <SD_BUS_VLT> and <SD_BUS_POWER> fields should be configured
to correct values. These actually do not do the voltage selection
or switch power to the SD card. However these fields are used
internally to gate the clock. So if these fields are set
incorrectly, SD module will not function.
And during my development, I have seen that SD card wouldn't function
without right configuration into these fields.
So this patch adds new quirk (SDHCI_QUIRK2_MUST_SET_SDHCI_BUS_POWER),
which make sure that ->set_power() sets these fields.
Signed-off-by: Vaibhav Hiremath <redacted>
---
drivers/mmc/host/sdhci-pltfm.c | 3 +++
drivers/mmc/host/sdhci.c | 3 ++-
drivers/mmc/host/sdhci.h | 2 ++
3 files changed, 7 insertions(+), 1 deletion(-)
@@ -409,6 +409,8 @@ struct sdhci_host {#define SDHCI_QUIRK2_SUPPORT_SINGLE (1<<13)/* Controller broken with using ACMD23 */#define SDHCI_QUIRK2_ACMD23_BROKEN (1<<14)+/* Voltage capabilities of Controller must be set */+#define SDHCI_QUIRK2_MUST_SET_SDHCI_BUS_POWER (1<<15)intirq;/* Device IRQ */void__iomem*ioaddr;/* Mapped address */
Different bus clock may need different pin setting.
For example, fast bus clock like 208Mhz need pin drive fast
while slow bus clock prefer pin drive slow to guarantee
signal quality.
So this patch creates two states,
- Default (slow/normal) pin state
- And fast pin state for higher freq bus speed.
And selection of pin state is done based on timing mode.
Signed-off-by: Vaibhav Hiremath <redacted>
Signed-off-by: Kevin Liu <redacted>
---
drivers/mmc/host/sdhci-pxav3.c | 45 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 44 insertions(+), 1 deletion(-)
@@ -298,6 +303,33 @@ static void pxav3_gen_init_74_clocks(struct sdhci_host *host, u8 power_mode)pxa->power_mode=power_mode;}+staticintpxav3_select_pinstate(structsdhci_host*host,unsignedintuhs)+{+structsdhci_pltfm_host*pltfm_host=sdhci_priv(host);+structsdhci_pxa*pxa=pltfm_host->priv;+structpinctrl_state*pinctrl;++if(IS_ERR(pxa->pinctrl)||+IS_ERR(pxa->pins_default)||+IS_ERR(pxa->pins_fast))+return-EINVAL;++switch(uhs){+caseMMC_TIMING_UHS_SDR50:+caseMMC_TIMING_UHS_SDR104:+caseMMC_TIMING_MMC_HS200:+caseMMC_TIMING_MMC_HS400:+pinctrl=pxa->pins_fast;+break;+default:+/* back to default state for other legacy timing */+pinctrl=pxa->pins_default;+break;+}++returnpinctrl_select_state(pxa->pinctrl,pinctrl);+}+staticvoidpxav3_set_uhs_signaling(structsdhci_host*host,unsignedintuhs){structsdhci_pltfm_host*pltfm_host=sdhci_priv(host);
@@ -586,6 +619,16 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)}}+pxa->pinctrl=devm_pinctrl_get(dev);+if(!IS_ERR(pxa->pinctrl)){+pxa->pins_default=pinctrl_lookup_state(pxa->pinctrl,"default");+if(IS_ERR(pxa->pins_default))+dev_err(dev,"could not get default pinstate\n");+pxa->pins_fast=pinctrl_lookup_state(pxa->pinctrl,"fast");+if(IS_ERR(pxa->pins_fast))+dev_info(dev,"could not get fast pinstate\n");+}+pm_runtime_get_noresume(&pdev->dev);pm_runtime_set_active(&pdev->dev);pm_runtime_set_autosuspend_delay(&pdev->dev,PXAV3_RPM_DELAY_MS);
With support for pxa1928 family of devices , this patch
updates the binding document with compatible property
of "marvell,pxav3-1928-sdhci".
Signed-off-by: Vaibhav Hiremath <redacted>
---
Documentation/devicetree/bindings/mmc/sdhci-pxa.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -5,7 +5,7 @@ and the properties used by the sdhci-pxav2 and sdhci-pxav3 drivers. Required properties: - compatible: Should be "mrvl,pxav2-mmc", "mrvl,pxav3-mmc" or- "marvell,armada-380-sdhci".+ "marvell,armada-380-sdhci" or "marvell,pxav3-1928-sdhci". - reg: * for "mrvl,pxav2-mmc" and "mrvl,pxav3-mmc", one register area for the SDHCI registers.
Different bus clock may need different pin setting.
For example, fast bus clock like 208Mhz need pin drive fast
while slow bus clock prefer pin drive slow to guarantee
signal quality.
So this patch creates two states,
- Default (slow/normal) pin state
- And fast pin state for higher freq bus speed.
And selection of pin state is done based on timing mode.
Signed-off-by: Vaibhav Hiremath <redacted>
Signed-off-by: Kevin Liu <redacted>
---
drivers/mmc/host/sdhci-pxav3.c | 45 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 44 insertions(+), 1 deletion(-)
@@ -298,6 +303,33 @@ static void pxav3_gen_init_74_clocks(struct sdhci_host *host, u8 power_mode)pxa->power_mode=power_mode;}+staticintpxav3_select_pinstate(structsdhci_host*host,unsignedintuhs)+{+structsdhci_pltfm_host*pltfm_host=sdhci_priv(host);+structsdhci_pxa*pxa=pltfm_host->priv;+structpinctrl_state*pinctrl;++if(IS_ERR(pxa->pinctrl)||+IS_ERR(pxa->pins_default)||+IS_ERR(pxa->pins_fast))+return-EINVAL;++switch(uhs){+caseMMC_TIMING_UHS_SDR50:+caseMMC_TIMING_UHS_SDR104:+caseMMC_TIMING_MMC_HS200:+caseMMC_TIMING_MMC_HS400:+pinctrl=pxa->pins_fast;+break;+default:+/* back to default state for other legacy timing */+pinctrl=pxa->pins_default;+break;+}++returnpinctrl_select_state(pxa->pinctrl,pinctrl);+}+staticvoidpxav3_set_uhs_signaling(structsdhci_host*host,unsignedintuhs){structsdhci_pltfm_host*pltfm_host=sdhci_priv(host);
could we ignore this for those SDHCI hosts that don't need it?
+ if (!IS_ERR(pxa->pinctrl)) {
+ pxa->pins_default = pinctrl_lookup_state(pxa->pinctrl, "default");
+ if (IS_ERR(pxa->pins_default))
+ dev_err(dev, "could not get default pinstate\n");
Why those SDHCI hosts that don't need pinctl setting should got this error?
+ pxa->pins_fast = pinctrl_lookup_state(pxa->pinctrl, "fast");
+ if (IS_ERR(pxa->pins_fast))
+ dev_info(dev, "could not get fast pinstate\n");
+ }
+
pm_runtime_get_noresume(&pdev->dev);
pm_runtime_set_active(&pdev->dev);
pm_runtime_set_autosuspend_delay(&pdev->dev, PXAV3_RPM_DELAY_MS);
From: Kevin Liu <redacted>
IN case of MMC HS200 mode, current code does not enable
SD_CE_ATA_2.MMC_HS200 & SD_CE_ATA_2.MMC_CARD bit configurations.
So this patch updates the above bit fields correctly.
Signed-off-by: Tim Wang <redacted>
Signed-off-by: Kevin Liu <redacted>
Signed-off-by: Vaibhav Hiremath <redacted>
---
Note: Unfortunately I do not have access to any other datasheets
which uses sdhci-pxav3 driver, so quite not sure whether this would
break any existing platform, probably NOT, as I do not see any
references for this change.
If anyone can confirm that would be really great.
drivers/mmc/host/sdhci-pxav3.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Different bus clock may need different pin setting.
For example, fast bus clock like 208Mhz need pin drive fast
while slow bus clock prefer pin drive slow to guarantee
signal quality.
So this patch creates two states,
- Default (slow/normal) pin state
- And fast pin state for higher freq bus speed.
And selection of pin state is done based on timing mode.
Signed-off-by: Vaibhav Hiremath <redacted>
Signed-off-by: Kevin Liu <redacted>
---
drivers/mmc/host/sdhci-pxav3.c | 45 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 44 insertions(+), 1 deletion(-)
Different bus clock may need different pin setting.
For example, fast bus clock like 208Mhz need pin drive fast
while slow bus clock prefer pin drive slow to guarantee
signal quality.
So this patch creates two states,
- Default (slow/normal) pin state
- And fast pin state for higher freq bus speed.
And selection of pin state is done based on timing mode.
Signed-off-by: Vaibhav Hiremath <redacted>
Signed-off-by: Kevin Liu <redacted>
---
drivers/mmc/host/sdhci-pxav3.c | 45 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 44 insertions(+), 1 deletion(-)
@@ -298,6 +303,33 @@ static void pxav3_gen_init_74_clocks(struct sdhci_host *host, u8 power_mode)pxa->power_mode=power_mode;}+staticintpxav3_select_pinstate(structsdhci_host*host,unsignedintuhs)+{+structsdhci_pltfm_host*pltfm_host=sdhci_priv(host);+structsdhci_pxa*pxa=pltfm_host->priv;+structpinctrl_state*pinctrl;++if(IS_ERR(pxa->pinctrl)||+IS_ERR(pxa->pins_default)||+IS_ERR(pxa->pins_fast))+return-EINVAL;++switch(uhs){+caseMMC_TIMING_UHS_SDR50:+caseMMC_TIMING_UHS_SDR104:+caseMMC_TIMING_MMC_HS200:+caseMMC_TIMING_MMC_HS400:+pinctrl=pxa->pins_fast;+break;+default:+/* back to default state for other legacy timing */+pinctrl=pxa->pins_default;+break;+}++returnpinctrl_select_state(pxa->pinctrl,pinctrl);+}+staticvoidpxav3_set_uhs_signaling(structsdhci_host*host,unsignedintuhs){structsdhci_pltfm_host*pltfm_host=sdhci_priv(host);
And could we ignore this call for those SDHCI hosts that don't need it?
I do not want to introduce flags here.
And also, it is already ignored for those who don't need it.
The first thing in the function pxav3_select_pinstate() is
if (IS_ERR(pxa->pinctrl) ||
IS_ERR(pxa->pins_default) ||
IS_ERR(pxa->pins_fast))
return -EINVAL;
So its already handled.
could we ignore this for those SDHCI hosts that don't need it?
Again, no need to introduce flags here. This is standard call and
handled properly. So for the platforms not using this, it really should
not matter.
Also, lookup is getting executed only when pinctrl is populated.
So I do not see any need here.
quoted
+ if (!IS_ERR(pxa->pinctrl)) {
+ pxa->pins_default = pinctrl_lookup_state(pxa->pinctrl, "default");
+ if (IS_ERR(pxa->pins_default))
+ dev_err(dev, "could not get default pinstate\n");
Why those SDHCI hosts that don't need pinctl setting should got this error?
It won't.
If Host does not need pinctrl, the execution would never reach this
point.
The if condition check would handle it, isn't it?
pxa->pinctrl = devm_pinctrl_get(dev);
if (!IS_ERR(pxa->pinctrl)) {
...
}
So I do not see why is it issue here?
Thanks,
Vaibhav
From: Kevin Liu <redacted>
IN case of MMC HS200 mode, current code does not enable
SD_CE_ATA_2.MMC_HS200 & SD_CE_ATA_2.MMC_CARD bit configurations.
So this patch updates the above bit fields correctly.
Signed-off-by: Tim Wang <redacted>
Signed-off-by: Kevin Liu <redacted>
Signed-off-by: Vaibhav Hiremath <redacted>
---
Note: Unfortunately I do not have access to any other datasheets
which uses sdhci-pxav3 driver, so quite not sure whether this would
break any existing platform, probably NOT, as I do not see any
references for this change.
If anyone can confirm that would be really great.
drivers/mmc/host/sdhci-pxav3.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
Different bus clock may need different pin setting.
For example, fast bus clock like 208Mhz need pin drive fast
while slow bus clock prefer pin drive slow to guarantee
signal quality.
So this patch creates two states,
- Default (slow/normal) pin state
- And fast pin state for higher freq bus speed.
And selection of pin state is done based on timing mode.
Signed-off-by: Vaibhav Hiremath <redacted>
Signed-off-by: Kevin Liu <redacted>
---
drivers/mmc/host/sdhci-pxav3.c | 45 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 44 insertions(+), 1 deletion(-)
No it does not.
The reason I put it here is,
code under discussion is related to pinctrl configuration, so for all
higher bus speed mode we want to set pins into fast mode.
And from hs400 perspective, I would expect it must get handled much
before than this stage and should not reach here at all.
So this portion of the code is complete in itself.
I hope I clarified your doubt.
Thanks,
Vaibhav
Different bus clock may need different pin setting.
For example, fast bus clock like 208Mhz need pin drive fast
while slow bus clock prefer pin drive slow to guarantee
signal quality.
So this patch creates two states,
- Default (slow/normal) pin state
- And fast pin state for higher freq bus speed.
And selection of pin state is done based on timing mode.
Signed-off-by: Vaibhav Hiremath <redacted>
Signed-off-by: Kevin Liu <redacted>
---
drivers/mmc/host/sdhci-pxav3.c | 45 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 44 insertions(+), 1 deletion(-)
@@ -298,6 +303,33 @@ static void pxav3_gen_init_74_clocks(struct sdhci_host *host, u8 power_mode)pxa->power_mode=power_mode;}+staticintpxav3_select_pinstate(structsdhci_host*host,unsignedintuhs)+{+structsdhci_pltfm_host*pltfm_host=sdhci_priv(host);+structsdhci_pxa*pxa=pltfm_host->priv;+structpinctrl_state*pinctrl;++if(IS_ERR(pxa->pinctrl)||+IS_ERR(pxa->pins_default)||+IS_ERR(pxa->pins_fast))+return-EINVAL;++switch(uhs){+caseMMC_TIMING_UHS_SDR50:+caseMMC_TIMING_UHS_SDR104:+caseMMC_TIMING_MMC_HS200:+caseMMC_TIMING_MMC_HS400:+pinctrl=pxa->pins_fast;+break;+default:+/* back to default state for other legacy timing */+pinctrl=pxa->pins_default;+break;+}++returnpinctrl_select_state(pxa->pinctrl,pinctrl);+}+staticvoidpxav3_set_uhs_signaling(structsdhci_host*host,unsignedintuhs){structsdhci_pltfm_host*pltfm_host=sdhci_priv(host);
Can you please have a look at the function?
static int pxav3_select_pinstate(struct sdhci_host *host, unsigned int uhs)
quoted
And could we ignore this call for those SDHCI hosts that don't need it?
I do not want to introduce flags here.
And also, it is already ignored for those who don't need it.
The first thing in the function pxav3_select_pinstate() is
if (IS_ERR(pxa->pinctrl) ||
IS_ERR(pxa->pins_default) ||
IS_ERR(pxa->pins_fast))
return -EINVAL;
So its already handled.
could we ignore this for those SDHCI hosts that don't need it?
Again, no need to introduce flags here. This is standard call and
handled properly. So for the platforms not using this, it really should
not matter.
Also, lookup is getting executed only when pinctrl is populated.
So I do not see any need here.
quoted
quoted
+ if (!IS_ERR(pxa->pinctrl)) {
+ pxa->pins_default = pinctrl_lookup_state(pxa->pinctrl, "default");
+ if (IS_ERR(pxa->pins_default))
+ dev_err(dev, "could not get default pinstate\n");
Why those SDHCI hosts that don't need pinctl setting should got this error?
It won't.
It does. On Marvell Berlin SoCs, I got
[ 1.070000] sdhci-pxav3 f7ab0800.sdhci: could not get default pinstate
[ 1.080000] sdhci-pxav3 f7ab0800.sdhci: could not get fast pinstate
If Host does not need pinctrl, the execution would never reach this
point.
The if condition check would handle it, isn't it?
pxa->pinctrl = devm_pinctrl_get(dev);
It seems this function always succeed...
From another side, we may have default pin in dts, for example: pin muxed between
emmc and nandflash. But we don't have fast pinstate, so we at least need the
flag to fast pinstate. Otherwise, in such platforms, we could get something like
[ 1.000000] sdhci-pxav3 f7ab0000.sdhci: get default pinstate
[ 1.000000] sdhci-pxav3 f7ab0000.sdhci: could not get fast pinstate
if (!IS_ERR(pxa->pinctrl)) {
...
}
So I do not see why is it issue here?
Thanks,
Vaibhav
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Different bus clock may need different pin setting.
For example, fast bus clock like 208Mhz need pin drive fast
while slow bus clock prefer pin drive slow to guarantee
signal quality.
So this patch creates two states,
- Default (slow/normal) pin state
- And fast pin state for higher freq bus speed.
And selection of pin state is done based on timing mode.
Signed-off-by: Vaibhav Hiremath <redacted>
Signed-off-by: Kevin Liu <redacted>
---
drivers/mmc/host/sdhci-pxav3.c | 45 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 44 insertions(+), 1 deletion(-)
@@ -298,6 +303,33 @@ static void pxav3_gen_init_74_clocks(struct sdhci_host *host, u8 power_mode)pxa->power_mode=power_mode;}+staticintpxav3_select_pinstate(structsdhci_host*host,unsignedintuhs)+{+structsdhci_pltfm_host*pltfm_host=sdhci_priv(host);+structsdhci_pxa*pxa=pltfm_host->priv;+structpinctrl_state*pinctrl;++if(IS_ERR(pxa->pinctrl)||+IS_ERR(pxa->pins_default)||+IS_ERR(pxa->pins_fast))+return-EINVAL;++switch(uhs){+caseMMC_TIMING_UHS_SDR50:+caseMMC_TIMING_UHS_SDR104:+caseMMC_TIMING_MMC_HS200:+caseMMC_TIMING_MMC_HS400:+pinctrl=pxa->pins_fast;+break;+default:+/* back to default state for other legacy timing */+pinctrl=pxa->pins_default;+break;+}++returnpinctrl_select_state(pxa->pinctrl,pinctrl);+}+staticvoidpxav3_set_uhs_signaling(structsdhci_host*host,unsignedintuhs){structsdhci_pltfm_host*pltfm_host=sdhci_priv(host);
Can you please have a look at the function?
static int pxav3_select_pinstate(struct sdhci_host *host, unsigned int uhs)
I was referring to pxav3_set_uhs_signaling().
It is void, so what you are suggesting would generate warning.
quoted
quoted
And could we ignore this call for those SDHCI hosts that don't need it?
I do not want to introduce flags here.
And also, it is already ignored for those who don't need it.
The first thing in the function pxav3_select_pinstate() is
if (IS_ERR(pxa->pinctrl) ||
IS_ERR(pxa->pins_default) ||
IS_ERR(pxa->pins_fast))
return -EINVAL;
So its already handled.
could we ignore this for those SDHCI hosts that don't need it?
Again, no need to introduce flags here. This is standard call and
handled properly. So for the platforms not using this, it really should
not matter.
Also, lookup is getting executed only when pinctrl is populated.
So I do not see any need here.
quoted
quoted
+ if (!IS_ERR(pxa->pinctrl)) {
+ pxa->pins_default = pinctrl_lookup_state(pxa->pinctrl, "default");
+ if (IS_ERR(pxa->pins_default))
+ dev_err(dev, "could not get default pinstate\n");
Why those SDHCI hosts that don't need pinctl setting should got this error?
It won't.
It does. On Marvell Berlin SoCs, I got
[ 1.070000] sdhci-pxav3 f7ab0800.sdhci: could not get default pinstate
[ 1.080000] sdhci-pxav3 f7ab0800.sdhci: could not get fast pinstate
quoted
If Host does not need pinctrl, the execution would never reach this
point.
The if condition check would handle it, isn't it?
pxa->pinctrl = devm_pinctrl_get(dev);
It seems this function always succeed...
Not always.
I would succeed only if you have pinctrl defined in DT for this device.
And if you have pinctrl defined, isn't it is expected to have "default"
pin state to be always present?
And if answer is yes here, then it is fair to be prompting error for it.
From another side, we may have default pin in dts, for example: pin muxed between
emmc and nandflash. But we don't have fast pinstate, so we at least need the
flag to fast pinstate. Otherwise, in such platforms, we could get something like
That is exactly the reason behind keeping it as dev_info.
[ 1.000000] sdhci-pxav3 f7ab0000.sdhci: get default pinstate
[ 1.000000] sdhci-pxav3 f7ab0000.sdhci: could not get fast pinstate
Different bus clock may need different pin setting.
For example, fast bus clock like 208Mhz need pin drive fast
while slow bus clock prefer pin drive slow to guarantee
signal quality.
So this patch creates two states,
- Default (slow/normal) pin state
- And fast pin state for higher freq bus speed.
And selection of pin state is done based on timing mode.
Signed-off-by: Vaibhav Hiremath <redacted>
Signed-off-by: Kevin Liu <redacted>
---
drivers/mmc/host/sdhci-pxav3.c | 45 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 44 insertions(+), 1 deletion(-)
@@ -298,6 +303,33 @@ static void pxav3_gen_init_74_clocks(struct sdhci_host *host, u8 power_mode)pxa->power_mode=power_mode;}+staticintpxav3_select_pinstate(structsdhci_host*host,unsignedintuhs)+{+structsdhci_pltfm_host*pltfm_host=sdhci_priv(host);+structsdhci_pxa*pxa=pltfm_host->priv;+structpinctrl_state*pinctrl;++if(IS_ERR(pxa->pinctrl)||+IS_ERR(pxa->pins_default)||+IS_ERR(pxa->pins_fast))+return-EINVAL;++switch(uhs){+caseMMC_TIMING_UHS_SDR50:+caseMMC_TIMING_UHS_SDR104:+caseMMC_TIMING_MMC_HS200:+caseMMC_TIMING_MMC_HS400:+pinctrl=pxa->pins_fast;+break;+default:+/* back to default state for other legacy timing */+pinctrl=pxa->pins_default;+break;+}++returnpinctrl_select_state(pxa->pinctrl,pinctrl);+}+staticvoidpxav3_set_uhs_signaling(structsdhci_host*host,unsignedintuhs){structsdhci_pltfm_host*pltfm_host=sdhci_priv(host);
Can you please have a look at the function?
static int pxav3_select_pinstate(struct sdhci_host *host, unsigned int uhs)
I was referring to pxav3_set_uhs_signaling().
It is void, so what you are suggesting would generate warning.
Oops, you are correct.
quoted
quoted
quoted
And could we ignore this call for those SDHCI hosts that don't need it?
I do not want to introduce flags here.
And also, it is already ignored for those who don't need it.
The first thing in the function pxav3_select_pinstate() is
if (IS_ERR(pxa->pinctrl) ||
IS_ERR(pxa->pins_default) ||
IS_ERR(pxa->pins_fast))
return -EINVAL;
So its already handled.
could we ignore this for those SDHCI hosts that don't need it?
Again, no need to introduce flags here. This is standard call and
handled properly. So for the platforms not using this, it really should
not matter.
Also, lookup is getting executed only when pinctrl is populated.
So I do not see any need here.
quoted
quoted
+ if (!IS_ERR(pxa->pinctrl)) {
+ pxa->pins_default = pinctrl_lookup_state(pxa->pinctrl, "default");
+ if (IS_ERR(pxa->pins_default))
+ dev_err(dev, "could not get default pinstate\n");
Why those SDHCI hosts that don't need pinctl setting should got this error?
It won't.
It does. On Marvell Berlin SoCs, I got
[ 1.070000] sdhci-pxav3 f7ab0800.sdhci: could not get default pinstate
[ 1.080000] sdhci-pxav3 f7ab0800.sdhci: could not get fast pinstate
quoted
If Host does not need pinctrl, the execution would never reach this
point.
The if condition check would handle it, isn't it?
pxa->pinctrl = devm_pinctrl_get(dev);
It seems this function always succeed...
Not always.
I would succeed only if you have pinctrl defined in DT for this device.
Yes, that's what I thought, but I got
[ 1.070000] sdhci-pxav3 f7ab0800.sdhci: could not get default pinstate
[ 1.080000] sdhci-pxav3 f7ab0800.sdhci: could not get fast pinstate
there's no pinctrl for f7ab0800.sdhci. Am I missing somthing?
Thanks,
Jisheng
And if you have pinctrl defined, isn't it is expected to have "default"
pin state to be always present?
And if answer is yes here, then it is fair to be prompting error for it.
quoted
From another side, we may have default pin in dts, for example: pin muxed between
emmc and nandflash. But we don't have fast pinstate, so we at least need the
flag to fast pinstate. Otherwise, in such platforms, we could get something like
That is exactly the reason behind keeping it as dev_info.
quoted
[ 1.000000] sdhci-pxav3 f7ab0000.sdhci: get default pinstate
[ 1.000000] sdhci-pxav3 f7ab0000.sdhci: could not get fast pinstate
could we ignore this for those SDHCI hosts that don't need it?
Again, no need to introduce flags here. This is standard call and
handled properly. So for the platforms not using this, it really should
not matter.
Also, lookup is getting executed only when pinctrl is populated.
So I do not see any need here.
quoted
quoted
+ if (!IS_ERR(pxa->pinctrl)) {
+ pxa->pins_default = pinctrl_lookup_state(pxa->pinctrl, "default");
+ if (IS_ERR(pxa->pins_default))
+ dev_err(dev, "could not get default pinstate\n");
Why those SDHCI hosts that don't need pinctl setting should got this error?
It won't.
It does. On Marvell Berlin SoCs, I got
[ 1.070000] sdhci-pxav3 f7ab0800.sdhci: could not get default pinstate
[ 1.080000] sdhci-pxav3 f7ab0800.sdhci: could not get fast pinstate
quoted
If Host does not need pinctrl, the execution would never reach this
point.
The if condition check would handle it, isn't it?
pxa->pinctrl = devm_pinctrl_get(dev);
It seems this function always succeed...
Not always.
I would succeed only if you have pinctrl defined in DT for this device.
Yes, that's what I thought, but I got
[ 1.070000] sdhci-pxav3 f7ab0800.sdhci: could not get default pinstate
[ 1.080000] sdhci-pxav3 f7ab0800.sdhci: could not get fast pinstate
there's no pinctrl for f7ab0800.sdhci. Am I missing somthing?
Thanks,
Jisheng
quoted
And if you have pinctrl defined, isn't it is expected to have "default"
pin state to be always present?
And if answer is yes here, then it is fair to be prompting error for it.
quoted
From another side, we may have default pin in dts, for example: pin muxed between
emmc and nandflash. But we don't have fast pinstate, so we at least need the
flag to fast pinstate. Otherwise, in such platforms, we could get something like
That is exactly the reason behind keeping it as dev_info.
quoted
[ 1.000000] sdhci-pxav3 f7ab0000.sdhci: get default pinstate
[ 1.000000] sdhci-pxav3 f7ab0000.sdhci: could not get fast pinstate
I did some invastigation here on the execution flow,
and you know what, you are right here.
It seems, devm_pinctrl_get() always returns valid pinctrl pointer, even
though the DT property is not populated.
The return value from I did some invastigation () should have been
treated differently, but it is not. Instead it creates the
"struct pinctrl" and return back to the driver.
I am looping Linus Walleji here, probably he can comment/confirm on
this.
Thanks,
Vaibhav
On Mon, Sep 7, 2015 at 1:18 PM, Vaibhav Hiremath
[off-list ref] wrote:
Different bus clock may need different pin setting.
For example, fast bus clock like 208Mhz need pin drive fast
while slow bus clock prefer pin drive slow to guarantee
signal quality.
So this patch creates two states,
- Default (slow/normal) pin state
- And fast pin state for higher freq bus speed.
And selection of pin state is done based on timing mode.
Signed-off-by: Vaibhav Hiremath <redacted>
Signed-off-by: Kevin Liu <redacted>
(...)
+ pxa->pinctrl = devm_pinctrl_get(dev);
+ if (!IS_ERR(pxa->pinctrl)) {
+ pxa->pins_default = pinctrl_lookup_state(pxa->pinctrl, "default");
+ if (IS_ERR(pxa->pins_default))
+ dev_err(dev, "could not get default pinstate\n");
+ pxa->pins_fast = pinctrl_lookup_state(pxa->pinctrl, "fast");
+ if (IS_ERR(pxa->pins_fast))
+ dev_info(dev, "could not get fast pinstate\n");
+ }
This is exactly how I think it should be used from a pin control
point of view.
If you depended on CONFIG_PM you could use
pinctrl_pm_select_default_state() but for this simple scenario
this is fine.
Reviewed-by: Linus Walleij <redacted>
From a pinctrl point of view.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 08 September 2015 08:12 PM, Linus Walleij wrote:
On Mon, Sep 7, 2015 at 1:18 PM, Vaibhav Hiremath
[off-list ref] wrote:
quoted
Different bus clock may need different pin setting.
For example, fast bus clock like 208Mhz need pin drive fast
while slow bus clock prefer pin drive slow to guarantee
signal quality.
So this patch creates two states,
- Default (slow/normal) pin state
- And fast pin state for higher freq bus speed.
And selection of pin state is done based on timing mode.
Signed-off-by: Vaibhav Hiremath <redacted>
Signed-off-by: Kevin Liu <redacted>
(...)
quoted
+ pxa->pinctrl = devm_pinctrl_get(dev);
+ if (!IS_ERR(pxa->pinctrl)) {
+ pxa->pins_default = pinctrl_lookup_state(pxa->pinctrl, "default");
+ if (IS_ERR(pxa->pins_default))
+ dev_err(dev, "could not get default pinstate\n");
+ pxa->pins_fast = pinctrl_lookup_state(pxa->pinctrl, "fast");
+ if (IS_ERR(pxa->pins_fast))
+ dev_info(dev, "could not get fast pinstate\n");
+ }
This is exactly how I think it should be used from a pin control
point of view.
If you depended on CONFIG_PM you could use
pinctrl_pm_select_default_state() but for this simple scenario
this is fine.
Reviewed-by: Linus Walleij <redacted>
From a pinctrl point of view.
Thanks for your review.
Linus,
I agree this is how it should be used.
But I still have one small doubt on expectation from
devm_pinctrl_get() function.
If pinctrl properties are not populated in Devicetree node,
then, shouldn't devm_pinctrl_get() return error ?
I followed the code flow, and it seems even if pinctrl properties are
not populated in DT node, the devm_pinctrl_get() returns valid
pointer to "struct pinctrl", isn't this against the expectation of the
call?
Code flow -
devm_pinctrl_get()
...
--> creat_pinctrl()
--> pinctrl_dt_to_map()
...
pinctrl_dt_to_map() iterates for pinctrl-x (x = 0,1,...) and if it
founds the entry then it parses the node. If it doesn't find any
pinctrl property then also it returns 0. and subsequently rreturns
handle to "struct pinctrl" for the device. Why is so?
Thanks,
Vaibhav
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Rob Herring <robh@kernel.org> Date: 2015-09-09 05:39:55
On 09/07/2015 06:18 AM, Vaibhav Hiremath wrote:
quoted hunk
With support for pxa1928 family of devices , this patch
updates the binding document with compatible property
of "marvell,pxav3-1928-sdhci".
Signed-off-by: Vaibhav Hiremath <redacted>
---
Documentation/devicetree/bindings/mmc/sdhci-pxa.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -5,7 +5,7 @@ and the properties used by the sdhci-pxav2 and sdhci-pxav3 drivers. Required properties: - compatible: Should be "mrvl,pxav2-mmc", "mrvl,pxav3-mmc" or- "marvell,armada-380-sdhci".+ "marvell,armada-380-sdhci" or "marvell,pxav3-1928-sdhci".
v3 is implied by pxa1928. So I'd just do "marvell,pxa1928-sdhci" to
better match the chip name.
Rob
- reg:
* for "mrvl,pxav2-mmc" and "mrvl,pxav3-mmc", one register area for
the SDHCI registers.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 8, 2015 at 5:07 PM, Vaibhav Hiremath
[off-list ref] wrote:
But I still have one small doubt on expectation from
devm_pinctrl_get() function.
If pinctrl properties are not populated in Devicetree node,
then, shouldn't devm_pinctrl_get() return error ?
I followed the code flow, and it seems even if pinctrl properties are
not populated in DT node, the devm_pinctrl_get() returns valid
pointer to "struct pinctrl", isn't this against the expectation of the
call?
Code flow -
devm_pinctrl_get()
...
--> creat_pinctrl()
That is the spelling mistake Dennis Ritchie and Ken Thompson
wish they had avoided in the first syscall interface :D
--> pinctrl_dt_to_map()
...
pinctrl_dt_to_map() iterates for pinctrl-x (x = 0,1,...) and if it
founds the entry then it parses the node. If it doesn't find any
pinctrl property then also it returns 0. and subsequently rreturns
handle to "struct pinctrl" for the device. Why is so?
So pinctrl_dt_to_map() returns 0 if there are no maps in the
device tree.
This is correct: there may be systems that have a mixture
of device tree and platform data, and in that case the code
needs to continue after calling pinctrl_dt_to_map() because
else it bails out before coming to the loop in
create_pinctrl() where we iterate over the static maps.
So after the loop in create_pinctrl() it should be possible
to return something like -ENOENT if we didn't find
anything. The device core pinctrl handling in
drivers/base/pinctrl.c seems to cope with it.
It's a semantic change so we would need to test to toss
it in and see what happens on some different systems
but in principe I think you're right. What we get if there is
no state is basically a dummy pinctrl that does nothing.
Do you wanna make a patch for this?
(Looping in Stephen Warren so he can tell if I miss something
obviously evident in the design that require stubs to
be present.)
Yours,
Linus Walleij
On Wednesday 09 September 2015 05:19 AM, Rob Herring wrote:
On 09/07/2015 06:18 AM, Vaibhav Hiremath wrote:
quoted
With support for pxa1928 family of devices , this patch
updates the binding document with compatible property
of "marvell,pxav3-1928-sdhci".
Signed-off-by: Vaibhav Hiremath <redacted>
---
Documentation/devicetree/bindings/mmc/sdhci-pxa.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -5,7 +5,7 @@ and the properties used by the sdhci-pxav2 and sdhci-pxav3 drivers. Required properties: - compatible: Should be "mrvl,pxav2-mmc", "mrvl,pxav3-mmc" or- "marvell,armada-380-sdhci".+ "marvell,armada-380-sdhci" or "marvell,pxav3-1928-sdhci".
v3 is implied by pxa1928. So I'd just do "marvell,pxa1928-sdhci" to
better match the chip name.
Ok, No issues.
I followed the existing "armada-380-sdhci" naming style.
Will correct it in next version.
Thanks,
Vaibhav
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html