* [PATCH] mmc: sdhci-pci-core: Tuning mode support for HS200 on AMD Platforms
@ 2016-10-04 8:42 Shyam Sundar S K
2016-10-10 7:55 ` Adrian Hunter
0 siblings, 1 reply; 6+ messages in thread
From: Shyam Sundar S K @ 2016-10-04 8:42 UTC (permalink / raw)
To: adrian.hunter, ulf.hansson
Cc: linux-mmc, Sen, Pankaj, Shah, Nehal-bakulchandra, Agrawal, Nitesh-kumar
This patch adds support for HS200 tuning mode on AMD eMMC-4.5.1
Reviewed-by: Sen, Pankaj <Pankaj.Sen@amd.com>
Reviewed-by: Shah, Nehal-bakulchandra <Nehal-bakulchandra.Shah@amd.com>
Signed-off-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
---
drivers/mmc/host/sdhci-pci-core.c | 182 +++++++++++++++++++++++++++++++++++++-
1 file changed, 180 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
index 897cfd2..5893ec4 100644
--- a/drivers/mmc/host/sdhci-pci-core.c
+++ b/drivers/mmc/host/sdhci-pci-core.c
@@ -734,6 +734,7 @@ static const struct sdhci_pci_fixes sdhci_via = {
.probe = via_probe,
};
+
static int rtsx_probe_slot(struct sdhci_pci_slot *slot)
{
slot->host->mmc->caps2 |= MMC_CAP2_HS200;
@@ -755,6 +756,172 @@ enum amd_chipset_gen {
AMD_CHIPSET_UNKNOWN,
};
+struct tuning_descriptor {
+ unsigned char tune_around;
+ bool this_tune_ok;
+ bool last_tune_ok;
+ bool valid_front_end;
+ unsigned char valid_front;
+ unsigned char valid_window_max;
+ unsigned char tune_low_max;
+ unsigned char tune_low;
+ unsigned char valid_window;
+ unsigned char tune_result;
+};
+
+static struct sdhci_ops sdhci_pci_ops;
+static struct tuning_descriptor tdescriptor;
+
+static int tuning_reset(struct sdhci_host *host)
+{
+ unsigned int val;
+ unsigned long flags;
+
+ spin_lock_irqsave(&host->lock, flags);
+
+ val = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+ val |= SDHCI_CTRL_PRESET_VAL_ENABLE | SDHCI_CTRL_EXEC_TUNING;
+ sdhci_writew(host, val, SDHCI_HOST_CONTROL2);
+
+ val = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+ val &= ~SDHCI_CTRL_EXEC_TUNING;
+ sdhci_writew(host, val, SDHCI_HOST_CONTROL2);
+
+ spin_unlock_irqrestore(&host->lock, flags);
+
+ return 0;
+}
+
+static int config_tuning_phase(struct sdhci_host *host, unsigned char phase)
+{
+ struct sdhci_pci_slot *slot = sdhci_priv(host);
+ struct pci_dev *pdev = slot->chip->pdev;
+ unsigned int val;
+ unsigned long flags;
+
+ spin_lock_irqsave(&host->lock, flags);
+
+ pci_read_config_dword(pdev, 0xb8, &val);
+ val &= ~0x1f;
+ val |= (0x10800 | (phase << 1));
+ pci_write_config_dword(pdev, 0xb8, val);
+
+ spin_unlock_irqrestore(&host->lock, flags);
+
+ return 0;
+}
+
+static int find_good_phase(struct sdhci_host *host)
+{
+ struct tuning_descriptor *td = &tdescriptor;
+ struct sdhci_pci_slot *slot = sdhci_priv(host);
+ struct pci_dev *pdev = slot->chip->pdev;
+ unsigned int val;
+ unsigned long flags;
+
+ spin_lock_irqsave(&host->lock, flags);
+
+ if (td->this_tune_ok == false)
+ td->valid_front_end = 1;
+
+ if (td->valid_front_end)
+ td->valid_front = td->valid_front;
+ else if (td->this_tune_ok)
+ td->valid_front = td->valid_front + 1;
+
+ if ((!td->this_tune_ok && td->last_tune_ok) ||
+ (td->tune_around == 11)) {
+
+ if (td->valid_window > td->valid_window_max) {
+ td->valid_window_max = td->valid_window;
+ td->tune_low_max = td->tune_low;
+ }
+ }
+
+ if (td->this_tune_ok && (!td->last_tune_ok))
+ td->tune_low = td->tune_around;
+ if (!td->this_tune_ok && td->last_tune_ok)
+ td->valid_window = 0x0;
+ else if (td->this_tune_ok)
+ td->valid_window = td->valid_window + 1;
+
+ td->last_tune_ok = td->this_tune_ok;
+
+ if (td->tune_around == 11) {
+ if ((td->valid_front + td->valid_window) >
+ td->valid_window_max) {
+ if (td->valid_front > td->valid_window)
+ td->tune_result = ((td->valid_front -
+ td->valid_window) >> 1);
+ else
+ td->tune_result = td->tune_low +
+ ((td->valid_window + td->valid_front) >> 1);
+ } else {
+ td->tune_result = td->tune_low_max +
+ (td->valid_window_max>>1);
+ }
+
+
+ if (td->tune_result > 0x0b)
+ td->tune_result = 0x0b;
+
+ pci_read_config_dword(pdev, 0xb8, &val);
+ val &= ~0x1f;
+ val |= (0x10800 | (td->tune_result<<1));
+ pci_write_config_dword(pdev, 0xb8, val);
+ }
+
+ spin_unlock_irqrestore(&host->lock, flags);
+
+ return 0;
+}
+
+static int amd_execute_tuning(struct sdhci_host *host, u32 opcode)
+{
+ struct tuning_descriptor *td = &tdescriptor;
+ u8 ctrl;
+
+ tuning_reset(host);
+ memset(td, 0x0, sizeof(struct tuning_descriptor));
+
+ if (host->quirks2 & SDHCI_QUIRK2_CLEAR_TRANSFERMODE_REG_BEFORE_CMD)
+ opcode = MMC_SEND_TUNING_BLOCK_HS200;
+
+ for (td->tune_around = 0; td->tune_around < 12; td->tune_around++) {
+
+ config_tuning_phase(host, td->tune_around);
+
+ if (mmc_send_tuning(host->mmc, opcode, NULL)) {
+ td->this_tune_ok = false;
+ host->mmc->need_retune = 0;
+ mdelay(4);
+ ctrl = SDHCI_RESET_CMD | SDHCI_RESET_DATA;
+ sdhci_writeb(host, ctrl, SDHCI_SOFTWARE_RESET);
+ } else {
+ td->this_tune_ok = true;
+ }
+
+ find_good_phase(host);
+ }
+
+ host->mmc->retune_period = 0;
+
+ return 0;
+}
+
+static int amd_enable_manual_tuning(struct sdhci_pci_slot *slot)
+{
+ struct pci_dev *pdev = slot->chip->pdev;
+ unsigned int val;
+
+ pci_read_config_dword(pdev, 0xd0, &val);
+ val &= 0xffffffcf;
+ val |= 0x30;
+ pci_write_config_dword(pdev, 0xd0, val);
+
+ return 0;
+}
+
static int amd_probe(struct sdhci_pci_chip *chip)
{
struct pci_dev *smbus_dev;
@@ -779,14 +946,25 @@ static int amd_probe(struct sdhci_pci_chip *chip)
if ((gen == AMD_CHIPSET_BEFORE_ML) || (gen == AMD_CHIPSET_CZ)) {
chip->quirks2 |= SDHCI_QUIRK2_CLEAR_TRANSFERMODE_REG_BEFORE_CMD;
- chip->quirks2 |= SDHCI_QUIRK2_BROKEN_HS200;
}
return 0;
}
+static int amd_probe_slot(struct sdhci_pci_slot *slot)
+{
+ struct sdhci_host *host = slot->host;
+
+ if (host->quirks2 & SDHCI_QUIRK2_CLEAR_TRANSFERMODE_REG_BEFORE_CMD) {
+ sdhci_pci_ops.platform_execute_tuning = amd_execute_tuning;
+ amd_enable_manual_tuning(slot);
+ }
+ return 0;
+}
+
static const struct sdhci_pci_fixes sdhci_amd = {
.probe = amd_probe,
+ .probe_slot = amd_probe_slot,
};
static const struct pci_device_id pci_ids[] = {
@@ -1397,7 +1575,7 @@ static int sdhci_pci_select_drive_strength(struct sdhci_host *host,
card_drv, drv_type);
}
-static const struct sdhci_ops sdhci_pci_ops = {
+static struct sdhci_ops sdhci_pci_ops = {
.set_clock = sdhci_set_clock,
.enable_dma = sdhci_pci_enable_dma,
.set_bus_width = sdhci_pci_set_bus_width,
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: sdhci-pci-core: Tuning mode support for HS200 on AMD Platforms
2016-10-04 8:42 [PATCH] mmc: sdhci-pci-core: Tuning mode support for HS200 on AMD Platforms Shyam Sundar S K
@ 2016-10-10 7:55 ` Adrian Hunter
2016-10-27 9:52 ` Shyam Sundar S K
0 siblings, 1 reply; 6+ messages in thread
From: Adrian Hunter @ 2016-10-10 7:55 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: ulf.hansson, linux-mmc, Sen, Pankaj, Shah, Nehal-bakulchandra,
Agrawal, Nitesh-kumar
On 04/10/16 11:42, Shyam Sundar S K wrote:
> This patch adds support for HS200 tuning mode on AMD eMMC-4.5.1
>
> Reviewed-by: Sen, Pankaj <Pankaj.Sen@amd.com>
> Reviewed-by: Shah, Nehal-bakulchandra <Nehal-bakulchandra.Shah@amd.com>
> Signed-off-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
> ---
> drivers/mmc/host/sdhci-pci-core.c | 182 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 180 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
> index 897cfd2..5893ec4 100644
> --- a/drivers/mmc/host/sdhci-pci-core.c
> +++ b/drivers/mmc/host/sdhci-pci-core.c
> @@ -734,6 +734,7 @@ static const struct sdhci_pci_fixes sdhci_via = {
> .probe = via_probe,
> };
>
> +
Unnecessary blank line
> static int rtsx_probe_slot(struct sdhci_pci_slot *slot)
> {
> slot->host->mmc->caps2 |= MMC_CAP2_HS200;
> @@ -755,6 +756,172 @@ enum amd_chipset_gen {
> AMD_CHIPSET_UNKNOWN,
> };
>
> +struct tuning_descriptor {
It would be nicer to prefix all structure and function names by something
specific to the device e.g. amd_...
> + unsigned char tune_around;
> + bool this_tune_ok;
> + bool last_tune_ok;
> + bool valid_front_end;
> + unsigned char valid_front;
> + unsigned char valid_window_max;
> + unsigned char tune_low_max;
> + unsigned char tune_low;
> + unsigned char valid_window;
> + unsigned char tune_result;
'unsigned char' -> 'u8'
> +};
> +
> +static struct sdhci_ops sdhci_pci_ops;
> +static struct tuning_descriptor tdescriptor;
tdescriptor should not be global. You need somewhere to put private
data. How about this:
From: Adrian Hunter <adrian.hunter@intel.com>
Date: Mon, 10 Oct 2016 10:04:45 +0300
Subject: [PATCH] mmc: sdhci-pci: Let devices define their own private data
Let devices define their own private data to facilitate device-specific
operations. The size of the private structure is specified in the
sdhci_pci_fixes structure, then sdhci_pci_probe_slot() will allocate extra
space for it, and sdhci_pci_priv() can be used to get a reference to it.
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
drivers/mmc/host/sdhci-pci-core.c | 3 ++-
drivers/mmc/host/sdhci-pci.h | 8 ++++++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
index 1d9e00a00e9f..782c8d25c0c8 100644
--- a/drivers/mmc/host/sdhci-pci-core.c
+++ b/drivers/mmc/host/sdhci-pci-core.c
@@ -1646,6 +1646,7 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot(
struct sdhci_pci_slot *slot;
struct sdhci_host *host;
int ret, bar = first_bar + slotno;
+ size_t priv_size = chip->fixes ? chip->fixes->priv_size : 0;
if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM)) {
dev_err(&pdev->dev, "BAR %d is not iomem. Aborting.\n", bar);
@@ -1667,7 +1668,7 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot(
return ERR_PTR(-ENODEV);
}
- host = sdhci_alloc_host(&pdev->dev, sizeof(struct sdhci_pci_slot));
+ host = sdhci_alloc_host(&pdev->dev, sizeof(*slot) + priv_size);
if (IS_ERR(host)) {
dev_err(&pdev->dev, "cannot allocate host\n");
return ERR_CAST(host);
diff --git a/drivers/mmc/host/sdhci-pci.h b/drivers/mmc/host/sdhci-pci.h
index 6bccf56bc5ff..6a1be6afe089 100644
--- a/drivers/mmc/host/sdhci-pci.h
+++ b/drivers/mmc/host/sdhci-pci.h
@@ -67,6 +67,7 @@ struct sdhci_pci_fixes {
int (*resume) (struct sdhci_pci_chip *);
const struct sdhci_ops *ops;
+ size_t priv_size;
};
struct sdhci_pci_slot {
@@ -87,6 +88,8 @@ struct sdhci_pci_slot {
struct mmc_card *card,
unsigned int max_dtr, int host_drv,
int card_drv, int *drv_type);
+
+ unsigned long private[0] ____cacheline_aligned;
};
struct sdhci_pci_chip {
@@ -101,4 +104,9 @@ struct sdhci_pci_chip {
struct sdhci_pci_slot *slots[MAX_SLOTS]; /* Pointers to host slots */
};
+static inline void *sdhci_pci_priv(struct sdhci_pci_slot *slot)
+{
+ return (void *)slot->private;
+}
+
#endif /* __SDHCI_PCI_H */
--
1.9.1
> +
> +static int tuning_reset(struct sdhci_host *host)
> +{
> + unsigned int val;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&host->lock, flags);
> +
> + val = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> + val |= SDHCI_CTRL_PRESET_VAL_ENABLE | SDHCI_CTRL_EXEC_TUNING;
> + sdhci_writew(host, val, SDHCI_HOST_CONTROL2);
> +
> + val = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> + val &= ~SDHCI_CTRL_EXEC_TUNING;
> + sdhci_writew(host, val, SDHCI_HOST_CONTROL2);
> +
> + spin_unlock_irqrestore(&host->lock, flags);
> +
> + return 0;
> +}
> +
> +static int config_tuning_phase(struct sdhci_host *host, unsigned char phase)
> +{
> + struct sdhci_pci_slot *slot = sdhci_priv(host);
> + struct pci_dev *pdev = slot->chip->pdev;
> + unsigned int val;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&host->lock, flags);
> +
> + pci_read_config_dword(pdev, 0xb8, &val);
> + val &= ~0x1f;
> + val |= (0x10800 | (phase << 1));
> + pci_write_config_dword(pdev, 0xb8, val);
> +
> + spin_unlock_irqrestore(&host->lock, flags);
> +
> + return 0;
> +}
> +
> +static int find_good_phase(struct sdhci_host *host)
> +{
> + struct tuning_descriptor *td = &tdescriptor;
> + struct sdhci_pci_slot *slot = sdhci_priv(host);
> + struct pci_dev *pdev = slot->chip->pdev;
> + unsigned int val;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&host->lock, flags);
> +
> + if (td->this_tune_ok == false)
> + td->valid_front_end = 1;
> +
> + if (td->valid_front_end)
> + td->valid_front = td->valid_front;
Assigning 'td->valid_front' to itself?
> + else if (td->this_tune_ok)
> + td->valid_front = td->valid_front + 1;
Use '+= 1'
> +
> + if ((!td->this_tune_ok && td->last_tune_ok) ||
> + (td->tune_around == 11)) {
> +
> + if (td->valid_window > td->valid_window_max) {
> + td->valid_window_max = td->valid_window;
> + td->tune_low_max = td->tune_low;
> + }
> + }
> +
> + if (td->this_tune_ok && (!td->last_tune_ok))
> + td->tune_low = td->tune_around;
> + if (!td->this_tune_ok && td->last_tune_ok)
> + td->valid_window = 0x0;
Just use 0 not 0x0.
> + else if (td->this_tune_ok)
> + td->valid_window = td->valid_window + 1;
Use '+= 1'
> +
> + td->last_tune_ok = td->this_tune_ok;
> +
> + if (td->tune_around == 11) {
> + if ((td->valid_front + td->valid_window) >
> + td->valid_window_max) {
> + if (td->valid_front > td->valid_window)
> + td->tune_result = ((td->valid_front -
> + td->valid_window) >> 1);
> + else
> + td->tune_result = td->tune_low +
> + ((td->valid_window + td->valid_front) >> 1);
> + } else {
> + td->tune_result = td->tune_low_max +
> + (td->valid_window_max>>1);
> + }
> +
> +
> + if (td->tune_result > 0x0b)
> + td->tune_result = 0x0b;
> +
> + pci_read_config_dword(pdev, 0xb8, &val);
> + val &= ~0x1f;
> + val |= (0x10800 | (td->tune_result<<1));
> + pci_write_config_dword(pdev, 0xb8, val);
> + }
> +
> + spin_unlock_irqrestore(&host->lock, flags);
> +
> + return 0;
> +}
> +
> +static int amd_execute_tuning(struct sdhci_host *host, u32 opcode)
> +{
> + struct tuning_descriptor *td = &tdescriptor;
> + u8 ctrl;
> +
> + tuning_reset(host);
> + memset(td, 0x0, sizeof(struct tuning_descriptor));
> +
> + if (host->quirks2 & SDHCI_QUIRK2_CLEAR_TRANSFERMODE_REG_BEFORE_CMD)
> + opcode = MMC_SEND_TUNING_BLOCK_HS200;
Why change the opcode?
> +
> + for (td->tune_around = 0; td->tune_around < 12; td->tune_around++) {
> +
> + config_tuning_phase(host, td->tune_around);
> +
> + if (mmc_send_tuning(host->mmc, opcode, NULL)) {
> + td->this_tune_ok = false;
> + host->mmc->need_retune = 0;
> + mdelay(4);
> + ctrl = SDHCI_RESET_CMD | SDHCI_RESET_DATA;
> + sdhci_writeb(host, ctrl, SDHCI_SOFTWARE_RESET);
> + } else {
> + td->this_tune_ok = true;
> + }
> +
> + find_good_phase(host);
It looks like tune_around is a temporary that should be passed as a parameter
to find_good_phase()
> + }
> +
> + host->mmc->retune_period = 0;
> +
> + return 0;
> +}
> +
> +static int amd_enable_manual_tuning(struct sdhci_pci_slot *slot)
> +{
> + struct pci_dev *pdev = slot->chip->pdev;
> + unsigned int val;
> +
> + pci_read_config_dword(pdev, 0xd0, &val);
> + val &= 0xffffffcf;
> + val |= 0x30;
> + pci_write_config_dword(pdev, 0xd0, val);
> +
> + return 0;
> +}
> +
> static int amd_probe(struct sdhci_pci_chip *chip)
> {
> struct pci_dev *smbus_dev;
> @@ -779,14 +946,25 @@ static int amd_probe(struct sdhci_pci_chip *chip)
>
> if ((gen == AMD_CHIPSET_BEFORE_ML) || (gen == AMD_CHIPSET_CZ)) {
> chip->quirks2 |= SDHCI_QUIRK2_CLEAR_TRANSFERMODE_REG_BEFORE_CMD;
> - chip->quirks2 |= SDHCI_QUIRK2_BROKEN_HS200;
> }
>
> return 0;
> }
>
> +static int amd_probe_slot(struct sdhci_pci_slot *slot)
> +{
> + struct sdhci_host *host = slot->host;
> +
> + if (host->quirks2 & SDHCI_QUIRK2_CLEAR_TRANSFERMODE_REG_BEFORE_CMD) {
Please don't use a quirk to identify the chip.
> + sdhci_pci_ops.platform_execute_tuning = amd_execute_tuning;
You can't do that because sdhci_pci_ops is shared with every PCI SDHCI host
controller not just AMD.
Have a look at:
https://marc.info/?l=linux-mmc&m=147565902811041&w=2
and re-base on top of that series.
> + amd_enable_manual_tuning(slot);
> + }
> + return 0;
> +}
> +
> static const struct sdhci_pci_fixes sdhci_amd = {
> .probe = amd_probe,
> + .probe_slot = amd_probe_slot,
> };
>
> static const struct pci_device_id pci_ids[] = {
> @@ -1397,7 +1575,7 @@ static int sdhci_pci_select_drive_strength(struct sdhci_host *host,
> card_drv, drv_type);
> }
>
> -static const struct sdhci_ops sdhci_pci_ops = {
> +static struct sdhci_ops sdhci_pci_ops = {
> .set_clock = sdhci_set_clock,
> .enable_dma = sdhci_pci_enable_dma,
> .set_bus_width = sdhci_pci_set_bus_width,
>
Please run checkpatch --strict and consider it suggestions.
Please consider adding some explanation of the tuning algorithm
and defining some of the constants.
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: sdhci-pci-core: Tuning mode support for HS200 on AMD Platforms
2016-10-10 7:55 ` Adrian Hunter
@ 2016-10-27 9:52 ` Shyam Sundar S K
2016-11-07 12:00 ` Ulf Hansson
0 siblings, 1 reply; 6+ messages in thread
From: Shyam Sundar S K @ 2016-10-27 9:52 UTC (permalink / raw)
To: Adrian Hunter
Cc: ulf.hansson, linux-mmc, Sen, Pankaj, Shah, Nehal-bakulchandra,
Agrawal, Nitesh-kumar
Made changes to the earlier submission based on the comments
received from Adrian.
Reviewed-by: Sen, Pankaj <Pankaj.Sen@amd.com>
Reviewed-by: Shah, Nehal-bakulchandra <Nehal-bakulchandra.Shah@amd.com>
Signed-off-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
Also, adding patch from Adrian for handling the device specific
private data.
From: Adrian Hunter <adrian.hunter@intel.com>
Date: Mon, 10 Oct 2016 10:04:45 +0300
Subject: [PATCH] mmc: sdhci-pci: Let devices define their own private data
Let devices define their own private data to facilitate device-specific
operations. The size of the private structure is specified in the
sdhci_pci_fixes structure, then sdhci_pci_probe_slot() will allocate extra
space for it, and sdhci_pci_priv() can be used to get a reference to it.
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
drivers/mmc/host/sdhci-pci-core.c | 181 +++++++++++++++++++++++++++++++++++++-
drivers/mmc/host/sdhci-pci.h | 7 ++
2 files changed, 186 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
index 1d9e00a..9576f82 100644
--- a/drivers/mmc/host/sdhci-pci-core.c
+++ b/drivers/mmc/host/sdhci-pci-core.c
@@ -817,6 +817,171 @@ enum amd_chipset_gen {
AMD_CHIPSET_UNKNOWN,
};
+static const struct sdhci_ops amd_sdhci_pci_ops;
+
+struct amd_tuning_descriptor {
+ u8 tune_around;
+ bool this_tune_ok;
+ bool last_tune_ok;
+ u8 valid_front;
+ u8 valid_window_max;
+ u8 tune_low_max;
+ u8 tune_low;
+ u8 valid_window;
+ u8 tune_result;
+};
+
+static int amd_tuning_reset(struct sdhci_host *host)
+{
+ unsigned int val;
+ unsigned long flags;
+
+ spin_lock_irqsave(&host->lock, flags);
+
+ val = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+ val |= SDHCI_CTRL_PRESET_VAL_ENABLE | SDHCI_CTRL_EXEC_TUNING;
+ sdhci_writew(host, val, SDHCI_HOST_CONTROL2);
+
+ val = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+ val &= ~SDHCI_CTRL_EXEC_TUNING;
+ sdhci_writew(host, val, SDHCI_HOST_CONTROL2);
+
+ spin_unlock_irqrestore(&host->lock, flags);
+
+ return 0;
+}
+
+static int amd_config_tuning_phase(struct sdhci_host *host, u8 phase)
+{
+ struct sdhci_pci_slot *slot = sdhci_priv(host);
+ struct pci_dev *pdev = slot->chip->pdev;
+ unsigned int val;
+ unsigned long flags;
+
+ spin_lock_irqsave(&host->lock, flags);
+
+ pci_read_config_dword(pdev, 0xb8, &val);
+ val &= ~0x1f;
+ val |= (0x10800 | (phase << 1));
+ pci_write_config_dword(pdev, 0xb8, val);
+
+ spin_unlock_irqrestore(&host->lock, flags);
+
+ return 0;
+}
+
+static int amd_find_good_phase(struct sdhci_host *host)
+{
+ struct sdhci_pci_slot *slot = sdhci_priv(host);
+ struct pci_dev *pdev = slot->chip->pdev;
+ struct amd_tuning_descriptor *td = sdhci_pci_priv(slot);
+
+ unsigned int val;
+ unsigned long flags;
+
+ spin_lock_irqsave(&host->lock, flags);
+
+ if (td->this_tune_ok)
+ td->valid_front += 1;
+ if ((!td->this_tune_ok && td->last_tune_ok) ||
+ (td->tune_around == 11)) {
+ if (td->valid_window > td->valid_window_max) {
+ td->valid_window_max = td->valid_window;
+ td->tune_low_max = td->tune_low;
+ }
+ }
+ if (td->this_tune_ok && (!td->last_tune_ok))
+ td->tune_low = td->tune_around;
+ if (!td->this_tune_ok && td->last_tune_ok)
+ td->valid_window = 0;
+ else if (td->this_tune_ok)
+ td->valid_window += 1;
+
+ td->last_tune_ok = td->this_tune_ok;
+
+ if (td->tune_around == 11) {
+ if ((td->valid_front + td->valid_window) >
+ td->valid_window_max) {
+ if (td->valid_front > td->valid_window)
+ td->tune_result = ((td->valid_front -
+ td->valid_window) >> 1);
+ else
+ td->tune_result = td->tune_low +
+ ((td->valid_window + td->valid_front) >> 1);
+ } else {
+ td->tune_result = td->tune_low_max +
+ (td->valid_window_max >> 1);
+ }
+
+ if (td->tune_result > 0x0b)
+ td->tune_result = 0x0b;
+
+ pci_read_config_dword(pdev, 0xb8, &val);
+ val &= ~0x1f;
+ val |= (0x10800 | (td->tune_result << 1));
+ pci_write_config_dword(pdev, 0xb8, val);
+ }
+
+ spin_unlock_irqrestore(&host->lock, flags);
+
+ return 0;
+}
+
+static int amd_enable_manual_tuning(struct sdhci_pci_slot *slot)
+{
+ struct pci_dev *pdev = slot->chip->pdev;
+ unsigned int val;
+
+ pci_read_config_dword(pdev, 0xd0, &val);
+ val &= 0xffffffcf;
+ val |= 0x30;
+ pci_write_config_dword(pdev, 0xd0, val);
+
+ return 0;
+}
+
+static int amd_execute_tuning(struct sdhci_host *host, u32 opcode)
+{
+ struct sdhci_pci_slot *slot = sdhci_priv(host);
+ struct amd_tuning_descriptor *td = sdhci_pci_priv(slot);
+ u8 ctrl;
+
+ amd_tuning_reset(host);
+ memset(td, 0, sizeof(struct amd_tuning_descriptor));
+
+ /*********************************************************************/
+ /* Enabling Software Tuning */
+ /********************************************************************/
+ /* 1. First switch the eMMC to HS200 Mode
+ * 2. Prepare the registers by using the sampling clock select
+ * 3. Send the CMD21 12 times with block length of 64 bytes
+ * 4. Everytime change the clk phase and check for CRC error
+ * (CMD and DATA),if error, soft reset the CMD and DATA line
+ * 5. Calculate the window and set the clock phase.
+ */
+
+ for (td->tune_around = 0; td->tune_around < 12; td->tune_around++) {
+ amd_config_tuning_phase(host, td->tune_around);
+
+ if (mmc_send_tuning(host->mmc, opcode, NULL)) {
+ td->this_tune_ok = false;
+ host->mmc->need_retune = 0;
+ mdelay(4);
+ ctrl = SDHCI_RESET_CMD | SDHCI_RESET_DATA;
+ sdhci_writeb(host, ctrl, SDHCI_SOFTWARE_RESET);
+ } else {
+ td->this_tune_ok = true;
+ }
+
+ amd_find_good_phase(host);
+ }
+
+ host->mmc->retune_period = 0;
+
+ amd_enable_manual_tuning(slot);
+ return 0;
+}
+
static int amd_probe(struct sdhci_pci_chip *chip)
{
struct pci_dev *smbus_dev;
@@ -841,7 +1006,6 @@ static int amd_probe(struct sdhci_pci_chip *chip)
if ((gen == AMD_CHIPSET_BEFORE_ML) || (gen == AMD_CHIPSET_CZ)) {
chip->quirks2 |= SDHCI_QUIRK2_CLEAR_TRANSFERMODE_REG_BEFORE_CMD;
- chip->quirks2 |= SDHCI_QUIRK2_BROKEN_HS200;
}
return 0;
@@ -849,6 +1013,7 @@ static int amd_probe(struct sdhci_pci_chip *chip)
static const struct sdhci_pci_fixes sdhci_amd = {
.probe = amd_probe,
+ .ops = &amd_sdhci_pci_ops,
};
static const struct pci_device_id pci_ids[] = {
@@ -1469,6 +1634,17 @@ static const struct sdhci_ops sdhci_pci_ops = {
.select_drive_strength = sdhci_pci_select_drive_strength,
};
+static const struct sdhci_ops amd_sdhci_pci_ops = {
+ .set_clock = sdhci_set_clock,
+ .enable_dma = sdhci_pci_enable_dma,
+ .set_bus_width = sdhci_pci_set_bus_width,
+ .reset = sdhci_reset,
+ .set_uhs_signaling = sdhci_set_uhs_signaling,
+ .hw_reset = sdhci_pci_hw_reset,
+ .select_drive_strength = sdhci_pci_select_drive_strength,
+ .platform_execute_tuning = amd_execute_tuning,
+};
+
/*****************************************************************************\
* *
* Suspend/resume *
@@ -1646,6 +1822,7 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot(
struct sdhci_pci_slot *slot;
struct sdhci_host *host;
int ret, bar = first_bar + slotno;
+ size_t priv_size = chip->fixes ? chip->fixes->priv_size : 0;
if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM)) {
dev_err(&pdev->dev, "BAR %d is not iomem. Aborting.\n", bar);
@@ -1667,7 +1844,7 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot(
return ERR_PTR(-ENODEV);
}
- host = sdhci_alloc_host(&pdev->dev, sizeof(struct sdhci_pci_slot));
+ host = sdhci_alloc_host(&pdev->dev, sizeof(*slot) + priv_size);
if (IS_ERR(host)) {
dev_err(&pdev->dev, "cannot allocate host\n");
return ERR_CAST(host);
diff --git a/drivers/mmc/host/sdhci-pci.h b/drivers/mmc/host/sdhci-pci.h
index 6bccf56..0bfd568 100644
--- a/drivers/mmc/host/sdhci-pci.h
+++ b/drivers/mmc/host/sdhci-pci.h
@@ -67,6 +67,7 @@ struct sdhci_pci_fixes {
int (*resume) (struct sdhci_pci_chip *);
const struct sdhci_ops *ops;
+ size_t priv_size;
};
struct sdhci_pci_slot {
@@ -87,6 +88,7 @@ struct sdhci_pci_slot {
struct mmc_card *card,
unsigned int max_dtr, int host_drv,
int card_drv, int *drv_type);
+ unsigned long private[0] ____cacheline_aligned;
};
struct sdhci_pci_chip {
@@ -101,4 +103,9 @@ struct sdhci_pci_chip {
struct sdhci_pci_slot *slots[MAX_SLOTS]; /* Pointers to host slots */
};
+static inline void *sdhci_pci_priv(struct sdhci_pci_slot *slot)
+{
+ return (void *)slot->private;
+}
+
#endif /* __SDHCI_PCI_H */
--
2.7.4
On 10/10/2016 1:25 PM, Adrian Hunter wrote:
> On 04/10/16 11:42, Shyam Sundar S K wrote:
>> This patch adds support for HS200 tuning mode on AMD eMMC-4.5.1
>>
>> Reviewed-by: Sen, Pankaj <Pankaj.Sen@amd.com>
>> Reviewed-by: Shah, Nehal-bakulchandra <Nehal-bakulchandra.Shah@amd.com>
>> Signed-off-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
>> ---
>> drivers/mmc/host/sdhci-pci-core.c | 182 +++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 180 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
>> index 897cfd2..5893ec4 100644
>> --- a/drivers/mmc/host/sdhci-pci-core.c
>> +++ b/drivers/mmc/host/sdhci-pci-core.c
>> @@ -734,6 +734,7 @@ static const struct sdhci_pci_fixes sdhci_via = {
>> .probe = via_probe,
>> };
>>
>> +
>
> Unnecessary blank line
>
>> static int rtsx_probe_slot(struct sdhci_pci_slot *slot)
>> {
>> slot->host->mmc->caps2 |= MMC_CAP2_HS200;
>> @@ -755,6 +756,172 @@ enum amd_chipset_gen {
>> AMD_CHIPSET_UNKNOWN,
>> };
>>
>> +struct tuning_descriptor {
>
> It would be nicer to prefix all structure and function names by something
> specific to the device e.g. amd_...
>
>> + unsigned char tune_around;
>> + bool this_tune_ok;
>> + bool last_tune_ok;
>> + bool valid_front_end;
>> + unsigned char valid_front;
>> + unsigned char valid_window_max;
>> + unsigned char tune_low_max;
>> + unsigned char tune_low;
>> + unsigned char valid_window;
>> + unsigned char tune_result;
>
> 'unsigned char' -> 'u8'
>
>> +};
>> +
>> +static struct sdhci_ops sdhci_pci_ops;
>> +static struct tuning_descriptor tdescriptor;
>
> tdescriptor should not be global. You need somewhere to put private
> data. How about this:
>
>
> From: Adrian Hunter <adrian.hunter@intel.com>
> Date: Mon, 10 Oct 2016 10:04:45 +0300
> Subject: [PATCH] mmc: sdhci-pci: Let devices define their own private data
>
> Let devices define their own private data to facilitate device-specific
> operations. The size of the private structure is specified in the
> sdhci_pci_fixes structure, then sdhci_pci_probe_slot() will allocate extra
> space for it, and sdhci_pci_priv() can be used to get a reference to it.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
> drivers/mmc/host/sdhci-pci-core.c | 3 ++-
> drivers/mmc/host/sdhci-pci.h | 8 ++++++++
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
> index 1d9e00a00e9f..782c8d25c0c8 100644
> --- a/drivers/mmc/host/sdhci-pci-core.c
> +++ b/drivers/mmc/host/sdhci-pci-core.c
> @@ -1646,6 +1646,7 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot(
> struct sdhci_pci_slot *slot;
> struct sdhci_host *host;
> int ret, bar = first_bar + slotno;
> + size_t priv_size = chip->fixes ? chip->fixes->priv_size : 0;
>
> if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM)) {
> dev_err(&pdev->dev, "BAR %d is not iomem. Aborting.\n", bar);
> @@ -1667,7 +1668,7 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot(
> return ERR_PTR(-ENODEV);
> }
>
> - host = sdhci_alloc_host(&pdev->dev, sizeof(struct sdhci_pci_slot));
> + host = sdhci_alloc_host(&pdev->dev, sizeof(*slot) + priv_size);
> if (IS_ERR(host)) {
> dev_err(&pdev->dev, "cannot allocate host\n");
> return ERR_CAST(host);
> diff --git a/drivers/mmc/host/sdhci-pci.h b/drivers/mmc/host/sdhci-pci.h
> index 6bccf56bc5ff..6a1be6afe089 100644
> --- a/drivers/mmc/host/sdhci-pci.h
> +++ b/drivers/mmc/host/sdhci-pci.h
> @@ -67,6 +67,7 @@ struct sdhci_pci_fixes {
> int (*resume) (struct sdhci_pci_chip *);
>
> const struct sdhci_ops *ops;
> + size_t priv_size;
> };
>
> struct sdhci_pci_slot {
> @@ -87,6 +88,8 @@ struct sdhci_pci_slot {
> struct mmc_card *card,
> unsigned int max_dtr, int host_drv,
> int card_drv, int *drv_type);
> +
> + unsigned long private[0] ____cacheline_aligned;
> };
>
> struct sdhci_pci_chip {
> @@ -101,4 +104,9 @@ struct sdhci_pci_chip {
> struct sdhci_pci_slot *slots[MAX_SLOTS]; /* Pointers to host slots */
> };
>
> +static inline void *sdhci_pci_priv(struct sdhci_pci_slot *slot)
> +{
> + return (void *)slot->private;
> +}
> +
> #endif /* __SDHCI_PCI_H */
>
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: sdhci-pci-core: Tuning mode support for HS200 on AMD Platforms
2016-10-27 9:52 ` Shyam Sundar S K
@ 2016-11-07 12:00 ` Ulf Hansson
2016-11-08 9:45 ` Adrian Hunter
0 siblings, 1 reply; 6+ messages in thread
From: Ulf Hansson @ 2016-11-07 12:00 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: Adrian Hunter, linux-mmc, Sen, Pankaj, Shah, Nehal-bakulchandra,
Agrawal, Nitesh-kumar
On 27 October 2016 at 11:52, Shyam Sundar S K <ssundark@amd.com> wrote:
> Made changes to the earlier submission based on the comments
> received from Adrian.
>
> Reviewed-by: Sen, Pankaj <Pankaj.Sen@amd.com>
> Reviewed-by: Shah, Nehal-bakulchandra <Nehal-bakulchandra.Shah@amd.com>
> Signed-off-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
>
> Also, adding patch from Adrian for handling the device specific
> private data.
>
> From: Adrian Hunter <adrian.hunter@intel.com>
> Date: Mon, 10 Oct 2016 10:04:45 +0300
> Subject: [PATCH] mmc: sdhci-pci: Let devices define their own private data
Shyam,
Seems like this should be two separate changes, one made by Adrian and
one by you. Can you please re-spin.
Kind regards
Uffe
>
> Let devices define their own private data to facilitate device-specific
> operations. The size of the private structure is specified in the
> sdhci_pci_fixes structure, then sdhci_pci_probe_slot() will allocate extra
> space for it, and sdhci_pci_priv() can be used to get a reference to it.
>
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
> drivers/mmc/host/sdhci-pci-core.c | 181 +++++++++++++++++++++++++++++++++++++-
> drivers/mmc/host/sdhci-pci.h | 7 ++
> 2 files changed, 186 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
> index 1d9e00a..9576f82 100644
> --- a/drivers/mmc/host/sdhci-pci-core.c
> +++ b/drivers/mmc/host/sdhci-pci-core.c
> @@ -817,6 +817,171 @@ enum amd_chipset_gen {
> AMD_CHIPSET_UNKNOWN,
> };
>
> +static const struct sdhci_ops amd_sdhci_pci_ops;
> +
> +struct amd_tuning_descriptor {
> + u8 tune_around;
> + bool this_tune_ok;
> + bool last_tune_ok;
> + u8 valid_front;
> + u8 valid_window_max;
> + u8 tune_low_max;
> + u8 tune_low;
> + u8 valid_window;
> + u8 tune_result;
> +};
> +
> +static int amd_tuning_reset(struct sdhci_host *host)
> +{
> + unsigned int val;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&host->lock, flags);
> +
> + val = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> + val |= SDHCI_CTRL_PRESET_VAL_ENABLE | SDHCI_CTRL_EXEC_TUNING;
> + sdhci_writew(host, val, SDHCI_HOST_CONTROL2);
> +
> + val = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> + val &= ~SDHCI_CTRL_EXEC_TUNING;
> + sdhci_writew(host, val, SDHCI_HOST_CONTROL2);
> +
> + spin_unlock_irqrestore(&host->lock, flags);
> +
> + return 0;
> +}
> +
> +static int amd_config_tuning_phase(struct sdhci_host *host, u8 phase)
> +{
> + struct sdhci_pci_slot *slot = sdhci_priv(host);
> + struct pci_dev *pdev = slot->chip->pdev;
> + unsigned int val;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&host->lock, flags);
> +
> + pci_read_config_dword(pdev, 0xb8, &val);
> + val &= ~0x1f;
> + val |= (0x10800 | (phase << 1));
> + pci_write_config_dword(pdev, 0xb8, val);
> +
> + spin_unlock_irqrestore(&host->lock, flags);
> +
> + return 0;
> +}
> +
> +static int amd_find_good_phase(struct sdhci_host *host)
> +{
> + struct sdhci_pci_slot *slot = sdhci_priv(host);
> + struct pci_dev *pdev = slot->chip->pdev;
> + struct amd_tuning_descriptor *td = sdhci_pci_priv(slot);
> +
> + unsigned int val;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&host->lock, flags);
> +
> + if (td->this_tune_ok)
> + td->valid_front += 1;
> + if ((!td->this_tune_ok && td->last_tune_ok) ||
> + (td->tune_around == 11)) {
> + if (td->valid_window > td->valid_window_max) {
> + td->valid_window_max = td->valid_window;
> + td->tune_low_max = td->tune_low;
> + }
> + }
> + if (td->this_tune_ok && (!td->last_tune_ok))
> + td->tune_low = td->tune_around;
> + if (!td->this_tune_ok && td->last_tune_ok)
> + td->valid_window = 0;
> + else if (td->this_tune_ok)
> + td->valid_window += 1;
> +
> + td->last_tune_ok = td->this_tune_ok;
> +
> + if (td->tune_around == 11) {
> + if ((td->valid_front + td->valid_window) >
> + td->valid_window_max) {
> + if (td->valid_front > td->valid_window)
> + td->tune_result = ((td->valid_front -
> + td->valid_window) >> 1);
> + else
> + td->tune_result = td->tune_low +
> + ((td->valid_window + td->valid_front) >> 1);
> + } else {
> + td->tune_result = td->tune_low_max +
> + (td->valid_window_max >> 1);
> + }
> +
> + if (td->tune_result > 0x0b)
> + td->tune_result = 0x0b;
> +
> + pci_read_config_dword(pdev, 0xb8, &val);
> + val &= ~0x1f;
> + val |= (0x10800 | (td->tune_result << 1));
> + pci_write_config_dword(pdev, 0xb8, val);
> + }
> +
> + spin_unlock_irqrestore(&host->lock, flags);
> +
> + return 0;
> +}
> +
> +static int amd_enable_manual_tuning(struct sdhci_pci_slot *slot)
> +{
> + struct pci_dev *pdev = slot->chip->pdev;
> + unsigned int val;
> +
> + pci_read_config_dword(pdev, 0xd0, &val);
> + val &= 0xffffffcf;
> + val |= 0x30;
> + pci_write_config_dword(pdev, 0xd0, val);
> +
> + return 0;
> +}
> +
> +static int amd_execute_tuning(struct sdhci_host *host, u32 opcode)
> +{
> + struct sdhci_pci_slot *slot = sdhci_priv(host);
> + struct amd_tuning_descriptor *td = sdhci_pci_priv(slot);
> + u8 ctrl;
> +
> + amd_tuning_reset(host);
> + memset(td, 0, sizeof(struct amd_tuning_descriptor));
> +
> + /*********************************************************************/
> + /* Enabling Software Tuning */
> + /********************************************************************/
> + /* 1. First switch the eMMC to HS200 Mode
> + * 2. Prepare the registers by using the sampling clock select
> + * 3. Send the CMD21 12 times with block length of 64 bytes
> + * 4. Everytime change the clk phase and check for CRC error
> + * (CMD and DATA),if error, soft reset the CMD and DATA line
> + * 5. Calculate the window and set the clock phase.
> + */
> +
> + for (td->tune_around = 0; td->tune_around < 12; td->tune_around++) {
> + amd_config_tuning_phase(host, td->tune_around);
> +
> + if (mmc_send_tuning(host->mmc, opcode, NULL)) {
> + td->this_tune_ok = false;
> + host->mmc->need_retune = 0;
> + mdelay(4);
> + ctrl = SDHCI_RESET_CMD | SDHCI_RESET_DATA;
> + sdhci_writeb(host, ctrl, SDHCI_SOFTWARE_RESET);
> + } else {
> + td->this_tune_ok = true;
> + }
> +
> + amd_find_good_phase(host);
> + }
> +
> + host->mmc->retune_period = 0;
> +
> + amd_enable_manual_tuning(slot);
> + return 0;
> +}
> +
> static int amd_probe(struct sdhci_pci_chip *chip)
> {
> struct pci_dev *smbus_dev;
> @@ -841,7 +1006,6 @@ static int amd_probe(struct sdhci_pci_chip *chip)
>
> if ((gen == AMD_CHIPSET_BEFORE_ML) || (gen == AMD_CHIPSET_CZ)) {
> chip->quirks2 |= SDHCI_QUIRK2_CLEAR_TRANSFERMODE_REG_BEFORE_CMD;
> - chip->quirks2 |= SDHCI_QUIRK2_BROKEN_HS200;
> }
>
> return 0;
> @@ -849,6 +1013,7 @@ static int amd_probe(struct sdhci_pci_chip *chip)
>
> static const struct sdhci_pci_fixes sdhci_amd = {
> .probe = amd_probe,
> + .ops = &amd_sdhci_pci_ops,
> };
>
> static const struct pci_device_id pci_ids[] = {
> @@ -1469,6 +1634,17 @@ static const struct sdhci_ops sdhci_pci_ops = {
> .select_drive_strength = sdhci_pci_select_drive_strength,
> };
>
> +static const struct sdhci_ops amd_sdhci_pci_ops = {
> + .set_clock = sdhci_set_clock,
> + .enable_dma = sdhci_pci_enable_dma,
> + .set_bus_width = sdhci_pci_set_bus_width,
> + .reset = sdhci_reset,
> + .set_uhs_signaling = sdhci_set_uhs_signaling,
> + .hw_reset = sdhci_pci_hw_reset,
> + .select_drive_strength = sdhci_pci_select_drive_strength,
> + .platform_execute_tuning = amd_execute_tuning,
> +};
> +
> /*****************************************************************************\
> * *
> * Suspend/resume *
> @@ -1646,6 +1822,7 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot(
> struct sdhci_pci_slot *slot;
> struct sdhci_host *host;
> int ret, bar = first_bar + slotno;
> + size_t priv_size = chip->fixes ? chip->fixes->priv_size : 0;
>
> if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM)) {
> dev_err(&pdev->dev, "BAR %d is not iomem. Aborting.\n", bar);
> @@ -1667,7 +1844,7 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot(
> return ERR_PTR(-ENODEV);
> }
>
> - host = sdhci_alloc_host(&pdev->dev, sizeof(struct sdhci_pci_slot));
> + host = sdhci_alloc_host(&pdev->dev, sizeof(*slot) + priv_size);
> if (IS_ERR(host)) {
> dev_err(&pdev->dev, "cannot allocate host\n");
> return ERR_CAST(host);
> diff --git a/drivers/mmc/host/sdhci-pci.h b/drivers/mmc/host/sdhci-pci.h
> index 6bccf56..0bfd568 100644
> --- a/drivers/mmc/host/sdhci-pci.h
> +++ b/drivers/mmc/host/sdhci-pci.h
> @@ -67,6 +67,7 @@ struct sdhci_pci_fixes {
> int (*resume) (struct sdhci_pci_chip *);
>
> const struct sdhci_ops *ops;
> + size_t priv_size;
> };
>
> struct sdhci_pci_slot {
> @@ -87,6 +88,7 @@ struct sdhci_pci_slot {
> struct mmc_card *card,
> unsigned int max_dtr, int host_drv,
> int card_drv, int *drv_type);
> + unsigned long private[0] ____cacheline_aligned;
> };
>
> struct sdhci_pci_chip {
> @@ -101,4 +103,9 @@ struct sdhci_pci_chip {
> struct sdhci_pci_slot *slots[MAX_SLOTS]; /* Pointers to host slots */
> };
>
> +static inline void *sdhci_pci_priv(struct sdhci_pci_slot *slot)
> +{
> + return (void *)slot->private;
> +}
> +
> #endif /* __SDHCI_PCI_H */
> --
> 2.7.4
>
> On 10/10/2016 1:25 PM, Adrian Hunter wrote:
>> On 04/10/16 11:42, Shyam Sundar S K wrote:
>>> This patch adds support for HS200 tuning mode on AMD eMMC-4.5.1
>>>
>>> Reviewed-by: Sen, Pankaj <Pankaj.Sen@amd.com>
>>> Reviewed-by: Shah, Nehal-bakulchandra <Nehal-bakulchandra.Shah@amd.com>
>>> Signed-off-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
>>> ---
>>> drivers/mmc/host/sdhci-pci-core.c | 182 +++++++++++++++++++++++++++++++++++++-
>>> 1 file changed, 180 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
>>> index 897cfd2..5893ec4 100644
>>> --- a/drivers/mmc/host/sdhci-pci-core.c
>>> +++ b/drivers/mmc/host/sdhci-pci-core.c
>>> @@ -734,6 +734,7 @@ static const struct sdhci_pci_fixes sdhci_via = {
>>> .probe = via_probe,
>>> };
>>>
>>> +
>>
>> Unnecessary blank line
>>
>>> static int rtsx_probe_slot(struct sdhci_pci_slot *slot)
>>> {
>>> slot->host->mmc->caps2 |= MMC_CAP2_HS200;
>>> @@ -755,6 +756,172 @@ enum amd_chipset_gen {
>>> AMD_CHIPSET_UNKNOWN,
>>> };
>>>
>>> +struct tuning_descriptor {
>>
>> It would be nicer to prefix all structure and function names by something
>> specific to the device e.g. amd_...
>>
>>> + unsigned char tune_around;
>>> + bool this_tune_ok;
>>> + bool last_tune_ok;
>>> + bool valid_front_end;
>>> + unsigned char valid_front;
>>> + unsigned char valid_window_max;
>>> + unsigned char tune_low_max;
>>> + unsigned char tune_low;
>>> + unsigned char valid_window;
>>> + unsigned char tune_result;
>>
>> 'unsigned char' -> 'u8'
>>
>>> +};
>>> +
>>> +static struct sdhci_ops sdhci_pci_ops;
>>> +static struct tuning_descriptor tdescriptor;
>>
>> tdescriptor should not be global. You need somewhere to put private
>> data. How about this:
>>
>>
>> From: Adrian Hunter <adrian.hunter@intel.com>
>> Date: Mon, 10 Oct 2016 10:04:45 +0300
>> Subject: [PATCH] mmc: sdhci-pci: Let devices define their own private data
>>
>> Let devices define their own private data to facilitate device-specific
>> operations. The size of the private structure is specified in the
>> sdhci_pci_fixes structure, then sdhci_pci_probe_slot() will allocate extra
>> space for it, and sdhci_pci_priv() can be used to get a reference to it.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>> drivers/mmc/host/sdhci-pci-core.c | 3 ++-
>> drivers/mmc/host/sdhci-pci.h | 8 ++++++++
>> 2 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
>> index 1d9e00a00e9f..782c8d25c0c8 100644
>> --- a/drivers/mmc/host/sdhci-pci-core.c
>> +++ b/drivers/mmc/host/sdhci-pci-core.c
>> @@ -1646,6 +1646,7 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot(
>> struct sdhci_pci_slot *slot;
>> struct sdhci_host *host;
>> int ret, bar = first_bar + slotno;
>> + size_t priv_size = chip->fixes ? chip->fixes->priv_size : 0;
>>
>> if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM)) {
>> dev_err(&pdev->dev, "BAR %d is not iomem. Aborting.\n", bar);
>> @@ -1667,7 +1668,7 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot(
>> return ERR_PTR(-ENODEV);
>> }
>>
>> - host = sdhci_alloc_host(&pdev->dev, sizeof(struct sdhci_pci_slot));
>> + host = sdhci_alloc_host(&pdev->dev, sizeof(*slot) + priv_size);
>> if (IS_ERR(host)) {
>> dev_err(&pdev->dev, "cannot allocate host\n");
>> return ERR_CAST(host);
>> diff --git a/drivers/mmc/host/sdhci-pci.h b/drivers/mmc/host/sdhci-pci.h
>> index 6bccf56bc5ff..6a1be6afe089 100644
>> --- a/drivers/mmc/host/sdhci-pci.h
>> +++ b/drivers/mmc/host/sdhci-pci.h
>> @@ -67,6 +67,7 @@ struct sdhci_pci_fixes {
>> int (*resume) (struct sdhci_pci_chip *);
>>
>> const struct sdhci_ops *ops;
>> + size_t priv_size;
>> };
>>
>> struct sdhci_pci_slot {
>> @@ -87,6 +88,8 @@ struct sdhci_pci_slot {
>> struct mmc_card *card,
>> unsigned int max_dtr, int host_drv,
>> int card_drv, int *drv_type);
>> +
>> + unsigned long private[0] ____cacheline_aligned;
>> };
>>
>> struct sdhci_pci_chip {
>> @@ -101,4 +104,9 @@ struct sdhci_pci_chip {
>> struct sdhci_pci_slot *slots[MAX_SLOTS]; /* Pointers to host slots */
>> };
>>
>> +static inline void *sdhci_pci_priv(struct sdhci_pci_slot *slot)
>> +{
>> + return (void *)slot->private;
>> +}
>> +
>> #endif /* __SDHCI_PCI_H */
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: sdhci-pci-core: Tuning mode support for HS200 on AMD Platforms
2016-11-07 12:00 ` Ulf Hansson
@ 2016-11-08 9:45 ` Adrian Hunter
2016-11-09 6:18 ` Shyam Sundar S K
0 siblings, 1 reply; 6+ messages in thread
From: Adrian Hunter @ 2016-11-08 9:45 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: Ulf Hansson, linux-mmc, Sen, Pankaj, Shah, Nehal-bakulchandra,
Agrawal, Nitesh-kumar
On 07/11/16 14:00, Ulf Hansson wrote:
> On 27 October 2016 at 11:52, Shyam Sundar S K <ssundark@amd.com> wrote:
>> Made changes to the earlier submission based on the comments
>> received from Adrian.
>>
>> Reviewed-by: Sen, Pankaj <Pankaj.Sen@amd.com>
>> Reviewed-by: Shah, Nehal-bakulchandra <Nehal-bakulchandra.Shah@amd.com>
>> Signed-off-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
>>
>> Also, adding patch from Adrian for handling the device specific
>> private data.
>>
>> From: Adrian Hunter <adrian.hunter@intel.com>
>> Date: Mon, 10 Oct 2016 10:04:45 +0300
>> Subject: [PATCH] mmc: sdhci-pci: Let devices define their own private data
>
> Shyam,
>
> Seems like this should be two separate changes, one made by Adrian and
> one by you. Can you please re-spin.
Yes, 2 separate patches please. I didn't mean mix the patches together -
sorry if there was confusion.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: sdhci-pci-core: Tuning mode support for HS200 on AMD Platforms
2016-11-08 9:45 ` Adrian Hunter
@ 2016-11-09 6:18 ` Shyam Sundar S K
0 siblings, 0 replies; 6+ messages in thread
From: Shyam Sundar S K @ 2016-11-09 6:18 UTC (permalink / raw)
To: Adrian Hunter
Cc: Ulf Hansson, linux-mmc, Sen, Pankaj, Shah, Nehal-bakulchandra,
Agrawal, Nitesh-kumar
Hi Ulf, Adrian,
Thank you for the feedback.
Apologies, I misunderstood it. Submitting two separate patches one from Adrian and the other from me re-spinned to the latest rc tree.
Thanks,
Shyam
On 11/8/2016 3:15 PM, Adrian Hunter wrote:
> On 07/11/16 14:00, Ulf Hansson wrote:
>> On 27 October 2016 at 11:52, Shyam Sundar S K <ssundark@amd.com> wrote:
>>> Made changes to the earlier submission based on the comments
>>> received from Adrian.
>>>
>>> Reviewed-by: Sen, Pankaj <Pankaj.Sen@amd.com>
>>> Reviewed-by: Shah, Nehal-bakulchandra <Nehal-bakulchandra.Shah@amd.com>
>>> Signed-off-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
>>>
>>> Also, adding patch from Adrian for handling the device specific
>>> private data.
>>>
>>> From: Adrian Hunter <adrian.hunter@intel.com>
>>> Date: Mon, 10 Oct 2016 10:04:45 +0300
>>> Subject: [PATCH] mmc: sdhci-pci: Let devices define their own private data
>>
>> Shyam,
>>
>> Seems like this should be two separate changes, one made by Adrian and
>> one by you. Can you please re-spin.
>
> Yes, 2 separate patches please. I didn't mean mix the patches together -
> sorry if there was confusion.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-11-09 6:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-04 8:42 [PATCH] mmc: sdhci-pci-core: Tuning mode support for HS200 on AMD Platforms Shyam Sundar S K
2016-10-10 7:55 ` Adrian Hunter
2016-10-27 9:52 ` Shyam Sundar S K
2016-11-07 12:00 ` Ulf Hansson
2016-11-08 9:45 ` Adrian Hunter
2016-11-09 6:18 ` Shyam Sundar S K
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.