Re: [PATCH RFC 1/4] mmc: host: Register changes for sdcc V5

From: Vijay Viswanath
Date: Thu May 03 2018 - 05:37:53 EST



On 5/2/2018 1:58 PM, Ulf Hansson wrote:
On 1 May 2018 at 12:39, Vijay Viswanath <vviswana@xxxxxxxxxxxxxx> wrote:
From: Sayali Lokhande <sayalil@xxxxxxxxxxxxxx>

For SDCC version 5.0.0, MCI registers are removed from SDCC
interface and some registers are moved to HC. This change is
to support MCI register removal for msmfalcon. New compatible
string "qcom,sdhci-msm-v5" is added for msmfalcon to support
this change.

To simplify review, I would recommend to split this up in a few more
pieces, a few re-factoring while the final change adds the
qcom,sdhci-msm-v5 support.


Will do


Change-Id: I0febfd9bb2222436a8eff20c20107dd4180c9781
Signed-off-by: Sayali Lokhande <sayalil@xxxxxxxxxxxxxx>
Signed-off-by: Vijay Viswanath <vviswana@xxxxxxxxxxxxxx>
---
.../devicetree/bindings/mmc/sdhci-msm.txt | 5 +-

Please move DT changes into separate patches and make sure to sent it
to DT maintainers as well.

drivers/mmc/host/sdhci-msm.c | 485 +++++++++++++++------
2 files changed, 365 insertions(+), 125 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
index bfdcdc4..c2b7b2b 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
+++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.txt
@@ -4,7 +4,10 @@ This file documents differences between the core properties in mmc.txt
and the properties used by the sdhci-msm driver.

Required properties:
-- compatible: Should contain "qcom,sdhci-msm-v4".
+- compatible: Should contain "qcom,sdhci-msm-v4" or "qcom,sdhci-msm-v5".
+ For SDCC version 5.0.0, MCI registers are removed from SDCC
+ interface and some registers are moved to HC. New compatible
+ string is added to support this change - "qcom,sdhci-msm-v5".
- reg: Base address and length of the register in the following order:
- Host controller register map (required)
- SD Core register map (required)
diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index bb11916..d4d432b 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c

[...]


+struct sdhci_msm_offset {

I think a better idea is to create a struct sdhci_msm_variant and make
it contain all the variant specific data. So, not only the offsets,
but other variant data as well.

More comments below.

+ u32 core_mci_data_cnt;
+ u32 core_mci_status;
+ u32 core_mci_fifo_cnt;
+ u32 core_mci_version;
+ u32 core_generics;
+ u32 core_testbus_config;
+ u32 core_testbus_sel2_bit;
+ u32 core_testbus_ena;
+ u32 core_testbus_sel2;
+ u32 core_pwrctl_status;
+ u32 core_pwrctl_mask;
+ u32 core_pwrctl_clear;
+ u32 core_pwrctl_ctl;
+ u32 core_sdcc_debug_reg;
+ u32 core_dll_config;
+ u32 core_dll_status;
+ u32 core_vendor_spec;
+ u32 core_vendor_spec_adma_err_addr0;
+ u32 core_vendor_spec_adma_err_addr1;
+ u32 core_vendor_spec_func2;
+ u32 core_vendor_spec_capabilities0;
+ u32 core_ddr_200_cfg;
+ u32 core_vendor_spec3;
+ u32 core_dll_config_2;
+ u32 core_ddr_config;
+ u32 core_ddr_config_2;
+};
+
+struct sdhci_msm_offset sdhci_msm_offset_mci_removed = {
+ .core_mci_data_cnt = 0x35c,
+ .core_mci_status = 0x324,
+ .core_mci_fifo_cnt = 0x308,
+ .core_mci_version = 0x318,
+ .core_generics = 0x320,
+ .core_testbus_config = 0x32c,
+ .core_testbus_sel2_bit = 3,
+ .core_testbus_ena = (1 << 31),
+ .core_testbus_sel2 = (1 << 3),
+ .core_pwrctl_status = 0x240,
+ .core_pwrctl_mask = 0x244,
+ .core_pwrctl_clear = 0x248,
+ .core_pwrctl_ctl = 0x24c,
+ .core_sdcc_debug_reg = 0x358,
+ .core_dll_config = 0x200,
+ .core_dll_status = 0x208,
+ .core_vendor_spec = 0x20c,
+ .core_vendor_spec_adma_err_addr0 = 0x214,
+ .core_vendor_spec_adma_err_addr1 = 0x218,
+ .core_vendor_spec_func2 = 0x210,
+ .core_vendor_spec_capabilities0 = 0x21c,
+ .core_ddr_200_cfg = 0x224,
+ .core_vendor_spec3 = 0x250,
+ .core_dll_config_2 = 0x254,
+ .core_ddr_config = 0x258,
+ .core_ddr_config_2 = 0x25c,
+};
+
+struct sdhci_msm_offset sdhci_msm_offset_mci_present = {
+ .core_mci_data_cnt = 0x30,
+ .core_mci_status = 0x34,
+ .core_mci_fifo_cnt = 0x44,
+ .core_mci_version = 0x050,
+ .core_generics = 0x70,
+ .core_testbus_config = 0x0CC,
+ .core_testbus_sel2_bit = 4,
+ .core_testbus_ena = (1 << 3),
+ .core_testbus_sel2 = (1 << 4),
+ .core_pwrctl_status = 0xDC,
+ .core_pwrctl_mask = 0xE0,
+ .core_pwrctl_clear = 0xE4,
+ .core_pwrctl_ctl = 0xE8,
+ .core_sdcc_debug_reg = 0x124,
+ .core_dll_config = 0x100,
+ .core_dll_status = 0x108,
+ .core_vendor_spec = 0x10C,
+ .core_vendor_spec_adma_err_addr0 = 0x114,
+ .core_vendor_spec_adma_err_addr1 = 0x118,
+ .core_vendor_spec_func2 = 0x110,
+ .core_vendor_spec_capabilities0 = 0x11C,
+ .core_ddr_200_cfg = 0x184,
+ .core_vendor_spec3 = 0x1B0,
+ .core_dll_config_2 = 0x1B4,
+ .core_ddr_config = 0x1B8,
+ .core_ddr_config_2 = 0x1BC,
+};
+
struct sdhci_msm_host {
struct platform_device *pdev;
void __iomem *core_mem; /* MSM SDCC mapped address */
@@ -156,8 +232,72 @@ struct sdhci_msm_host {
wait_queue_head_t pwr_irq_wait;
bool pwr_irq_flag;
u32 caps_0;
+ bool mci_removed;
+ const struct sdhci_msm_offset *offset;
};

+/*
+ * APIs to write to vendor specific registers which were there in the core_mem
+ * region before MCI was removed.
+ */
+u8 sdhci_msm_vendor_readb_relaxed(struct sdhci_host *host, u32 offset)
+{
+ struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+ struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+ void __iomem *base_addr;
+
+ if (msm_host->mci_removed)
+ base_addr = host->ioaddr;

I would rather use a separate set of functions, which you assign at ->probe().

That may speed things up a little bit, as you don't need to check for
"mci_removed", each time in these read/write functions.

+ else
+ base_addr = msm_host->core_mem;
+
+ return readb_relaxed(base_addr + offset);
+}

[...]

static const struct of_device_id sdhci_msm_dt_match[] = {
{ .compatible = "qcom,sdhci-msm-v4" },
+ {.compatible = "qcom,sdhci-msm-v5"},
{},
};

@@ -1430,6 +1651,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
u16 host_version, core_minor;
u32 core_version, config;
u8 core_major;
+ const struct sdhci_msm_offset *msm_offset;

host = sdhci_pltfm_init(pdev, &sdhci_msm_pdata, sizeof(*msm_host));
if (IS_ERR(host))
@@ -1445,6 +1667,15 @@ static int sdhci_msm_probe(struct platform_device *pdev)
if (ret)
goto pltfm_free;

+ if (of_find_compatible_node(NULL, NULL, "qcom,sdhci-msm-v5")) {
+ msm_host->mci_removed = true;
+ msm_host->offset = &sdhci_msm_offset_mci_removed;

I would suggest to extend your array of "struct of_device_id" to use
the .data member to store all the variant specific data (such as the
offsets for example).

Then you can use of_device_get_match_data(), to get the variant data.

+ } else {
+ msm_host->mci_removed = false;
+ msm_host->offset = &sdhci_msm_offset_mci_present;
+ }
+ msm_offset = msm_host->offset;
+

[...]

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html


I will explore the approach you suggested and will get back to you.

Thanks,
Vijay