On 2/13/25 4:58 AM, Yixun Lan wrote:
The SDHCI controller found in SpacemiT K1 SoC features SD,
SDIO, eMMC support, such as:
- Compatible for 4-bit SDIO 3.0 UHS-I protocol, up to SDR104
- Compatible for 4-bit SD 3.0 UHS-I protocol, up to SDR104
- Compatible for 8bit eMMC5.1, up to HS400
Signed-off-by: Yixun Lan <dlan@xxxxxxxxxx>
Why is this RFC? Have you tested it?
I have a few minor comments but this seems reasonable to me.
---
drivers/mmc/host/Kconfig | 14 ++
drivers/mmc/host/Makefile | 1 +
drivers/mmc/host/sdhci-of-k1.c | 320 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 335 insertions(+)
. . .
+#define SDHC_PHY_DLLCFG 0x168
+#define DLL_PREDLY_NUM 0x04
+#define DLL_FULLDLY_RANGE 0x10
+#define DLL_VREG_CTRL 0x40
+#define DLL_ENABLE 0x80000000
+#define DLL_REFRESH_SWEN_SHIFT 0x1C
+#define DLL_REFRESH_SW_SHIFT 0x1D
+
+#define SDHC_PHY_DLLCFG1 0x16C
+#define DLL_REG2_CTRL 0x0C
+#define DLL_REG3_CTRL_MASK 0xFF
As Adrian said, please use GENMASK() (or BIT()) to define
these masks, and FIELD_GET() or similar to manipulate them.
I prefer lower-case hex digits too.
+#define DLL_REG3_CTRL_SHIFT 0x10
+#define DLL_REG2_CTRL_MASK 0xFF
+#define DLL_REG2_CTRL_SHIFT 0x08
+#define DLL_REG1_CTRL 0x92
+#define DLL_REG1_CTRL_MASK 0xFF
+#define DLL_REG1_CTRL_SHIFT 0x00
+
+#define SDHC_PHY_DLLSTS 0x170
+#define DLL_LOCK_STATE 0x01
+
+#define SDHC_PHY_DLLSTS1 0x174
+#define DLL_MASTER_DELAY_MASK 0xFF
+#define DLL_MASTER_DELAY_SHIFT 0x10
+
+#define SDHC_PHY_PADCFG_REG 0x178
+#define RX_BIAS_CTRL BIT(5)
+#define PHY_DRIVE_SEL_MASK 0x7
+#define PHY_DRIVE_SEL_DEFAULT 0x4
+
+struct spacemit_sdhci_host {
+ struct clk *clk_core;
+ struct clk *clk_io;
+};
+
I don't think the next few functions add any real value.
Just call sdhci_writel() and sdhci_readl() directly. It
might even take fewer characters (but above all, I think
it's clearer without the function hiding what's done).
+static inline void spacemit_sdhci_setbits(struct sdhci_host *host, u32 val, int reg)
+{
+ sdhci_writel(host, sdhci_readl(host, reg) | val, reg);
+}
+
+static inline void spacemit_sdhci_clrbits(struct sdhci_host *host, u32 val, int reg)
+{
+ sdhci_writel(host, sdhci_readl(host, reg) & ~val, reg);
+}
+
This too, just open-code this function in the two places it's used.
+static inline void spacemit_sdhci_clrsetbits(struct sdhci_host *host, u32 clr, u32 set, int reg)
+{
+ u32 val = sdhci_readl(host, reg);
+
+ val = (val & ~clr) | set;
+ sdhci_writel(host, val, reg);
+}
+
+static void spacemit_sdhci_reset(struct sdhci_host *host, u8 mask)
+{
+ struct platform_device *pdev;
+
. . .
+ udelay(5);
+
+ spacemit_sdhci_setbits(host, PHY_FUNC_EN | PHY_PLL_LOCK, SDHC_PHY_CTRL_REG);
+}
+
I don't feel as strongly about this, but...
Here too, what the next function does is very typical and all
of it could go in the probe function. I do understand that it
groups the clock-related code though.
But aside from that, I think assigning pltfm_host->clock could
be done in the probe function rather than hiding it in here.
+static inline int spacemit_sdhci_get_clocks(struct device *dev,
+ struct sdhci_pltfm_host *pltfm_host)
+{
+ struct spacemit_sdhci_host *sdhst = sdhci_pltfm_priv(pltfm_host);
+
+ sdhst->clk_core = devm_clk_get_enabled(dev, "core");
+ if (IS_ERR(sdhst->clk_core))
+ return -EINVAL;
+
+ sdhst->clk_io = devm_clk_get_enabled(dev, "io");
+ if (IS_ERR(sdhst->clk_io))
+ return -EINVAL;
+
+ pltfm_host->clk = sdhst->clk_io;
+
+ return 0;
+}
+
+static const struct sdhci_ops spacemit_sdhci_ops = {
+ .get_max_clock = spacemit_sdhci_clk_get_max_clock,
+ .reset = spacemit_sdhci_reset,
+ .set_bus_width = sdhci_set_bus_width,
+ .set_clock = spacemit_sdhci_set_clock,
+ .set_uhs_signaling = spacemit_sdhci_set_uhs_signaling,
+};
+
I think you should make the next structure be used as platform
data for "spacemit,k1-sdhci", rather than just a global. That
way you could conceivably use the same driver with slightly
different (or even the same) quirks for future hardware.
-Alex
+static const struct sdhci_pltfm_data spacemit_sdhci_k1_pdata = {
+ .ops = &spacemit_sdhci_ops,
+ .quirks = SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK |
+ SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC |
+ SDHCI_QUIRK_32BIT_ADMA_SIZE |
+ SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN |
+ SDHCI_QUIRK_BROKEN_CARD_DETECTION |
+ SDHCI_QUIRK_BROKEN_TIMEOUT_VAL,
+ .quirks2 = SDHCI_QUIRK2_BROKEN_64_BIT_DMA |
+ SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
+};
+. . .
