Re: [PATCH] mmc:host:sdhci-pci:V2-Addition of Arasan PCI controller with integrated phy.

On 10/10/17 20:42, Atul Garg wrote:

The Arasan controller is based on a FPGA platform and has integrated phy
with specific phy registers used during the initialization and
management of different modes. The phy and the controller are integrated
and registers are very specific to Arasan.

Arasan being an IP provider, licenses these IPs to various companies for
integration of IP in custom SOCs. The custom SOCs define own register
map depending on how bits are tied inside the SOC for phy registers,
depending on SOC memory plan and hence will require own platform drivers.

If more details on phy registers are required, an interface document is
hosted at https://arasan.com/NF/eMMC5.1 PHY Programming in Linux.pdf.

Signed-off-by: Atul Garg <redacted>

Sekhar Noir already covered some things.  There are some more comments below.

Why not usleep_range instead?

+	} while (timeout--);
+	if (!timeout)
+		return -ENODEV;
+	return 0;
+}
+
+static int arasan_phy_read(struct sdhci_host *host, u8 offset, u8 *data)
+{
+	u32 timeout;
+	u8 busy;
+
+	sdhci_writew(host, 0, HOST_PHY_DATA_REG);
+	*data = sdhci_readw(host, HOST_PHY_DATA_REG) & 0xFF;
+	sdhci_writew(host, ((0<<8) | offset), HOST_PHY_ADDR_REG);
+	timeout = 20;
+	do {
+		busy = sdhci_readw(host, HOST_PHY_ADDR_REG) & (1<<9);
+		if (!busy)
+			break;
+		mdelay(1);

Why not usleep_range instead?

+	} while (timeout--);
+	if (!timeout)
+		return -ENODEV;
+	*data = sdhci_readw(host, HOST_PHY_DATA_REG) & 0xFF;
+	return 0;
+}
+
+/* Initialize the Arasan PHY */
+static int arasan_phy_init(struct sdhci_host *host)
+{
+	struct sdhci_pci_slot *slot = sdhci_priv(host);
+	struct pci_dev *pdev = slot->chip->pdev;
+	u8 val;
+
+	if (arasan_phy_write(host, 0, PHY_CONTROL))
+		return -ENODEV;
+	if (arasan_phy_read(host, PHY_IOPADCONTROL1, &val))
+		return -ENODEV;
+	if (arasan_phy_write(host, val | RETB_ENABLE, PHY_IOPADCONTROL1))
+		return -ENODEV;
+	if (arasan_phy_read(host, PHY_IOPADCONTROL2, &val))
+		return -ENODEV;
+	if (arasan_phy_write(host, val | RTRIM_ENABLE, PHY_IOPADCONTROL2))
+		return -ENODEV;

All these statements can be combined e.g.

	if (arasan_phy_write(host, 0, PHY_CONTROL) ||
	    arasan_phy_read(host, PHY_IOPADCONTROL1, &val) ||
	    arasan_phy_write(host, val | RETB_ENABLE, PHY_IOPADCONTROL1) ||
	    arasan_phy_read(host, PHY_IOPADCONTROL2, &val) ||
	    arasan_phy_write(host, val | RTRIM_ENABLE, PHY_IOPADCONTROL2))
		return -ENODEV;

Same with more examples below.

+	mdelay(10);

Why not msleep instead?

+
+	if (arasan_phy_read(host, PHY_IOPADSTATUS, &val)) {
+		if (!(val & CALDONE_MASK)) {
+			dev_err(&pdev->dev, "Phy calibration not done\n");
+			return -ENODEV;
+		}
+	}
+	if (arasan_phy_read(host, PHY_IOPADCONTROL1, &val))
+		return -ENODEV;
+	if (arasan_phy_write(host, val | PDB_ENABLE, PHY_IOPADCONTROL1))
+		return -ENODEV;
+	mdelay(10);

Why not msleep instead?

+
+	if (arasan_phy_read(host, PHY_IOPADSTATUS, &val)) {
+		if (!(val & CALDONE_MASK)) {
+			dev_err(&pdev->dev, "Phy calibration not done\n");
+			return -ENODEV;
+		}
+	}
+	if (arasan_phy_read(host, PHY_IORENCONTROL1, &val))
+		return -ENODEV;
+	if (arasan_phy_write(host, val | REN_CMD | REN_STRB, PHY_IORENCONTROL1))
+		return -ENODEV;
+	if (arasan_phy_read(host, PHY_IOPUCONTROL1, &val))
+		return -ENODEV;
+	if (arasan_phy_write(host, val | PU_CMD, PHY_IOPUCONTROL1))
+		return -ENODEV;
+	if (arasan_phy_read(host, PHY_CMDCONTROL, &val))
+		return -ENODEV;
+	if (arasan_phy_write(host, val | PDB_CMD, PHY_CMDCONTROL))
+		return -ENODEV;
+	if (arasan_phy_read(host, PHY_IORENCONTROL2, &val))
+		return -ENODEV;
+	if (arasan_phy_write(host, val | REN_DAT, PHY_IORENCONTROL2))
+		return -ENODEV;
+	if (arasan_phy_read(host, PHY_IOPUCONTROL2, &val))
+		return -ENODEV;
+	if (arasan_phy_write(host, val | PU_DAT, PHY_IOPUCONTROL2))
+		return -ENODEV;
+	if (arasan_phy_read(host, PHY_DATACONTROL, &val))
+		return -ENODEV;
+	if (arasan_phy_write(host, val | PDB_DAT, PHY_DATACONTROL))
+		return -ENODEV;
+	if (arasan_phy_read(host, PHY_STROBECONTROL, &val))
+		return -ENODEV;
+	if (arasan_phy_write(host, val | PDB_STRB, PHY_STROBECONTROL))
+		return -ENODEV;
+	if (arasan_phy_read(host, PHY_CLKCONTROL, &val))
+		return -ENODEV;
+	if (arasan_phy_write(host, val | PDB_CLK, PHY_CLKCONTROL))
+		return -ENODEV;
+	if (arasan_phy_read(host, PHY_CLKBUFSELECT, &val))
+		return -ENODEV;
+	if (arasan_phy_write(host, val | 0x7, PHY_CLKBUFSELECT))
+		return -ENODEV;
+	if (arasan_phy_write(host, 0x4, PHY_MODECONTROL))
+		return -ENODEV;
+	return 0;
+}
+
+
+static int arasan_set_phy(struct sdhci_host *host)
+{
+	u8 clk_sel;
+	static u32 chg_clk;
+	u8 val;
+	u8 otap, itap, dll_sts, io_pad;
+
+	if (chg_clk != host->mmc->ios.clock) {
+		chg_clk = host->mmc->ios.clock;
+		if (host->mmc->ios.clock == 200000000)
+			clk_sel = 0;
+		else if (host->mmc->ios.clock == 100000000)
+			clk_sel = 2;
+		else if (host->mmc->ios.clock == 50000000)
+			clk_sel = 1;
+		else
+			clk_sel = 0;
+		/* Change phy settings only if there is a change in clock */
+		goto set_phy;
+	} else
+		return 0;
+set_phy:
+	if (host->mmc_host_ops.hs400_enhanced_strobe) {
+		if (arasan_phy_write(host, 0x1, PHY_MODECONTROL))
+			return -ENODEV;
+		otap = ((0x2<<1) | OTAP_DLY_EN);
+		if (arasan_phy_write(host, otap, PHY_OUTPUTTAPDELAY))
+			return -ENODEV;
+		if (arasan_phy_write(host, 0, PHY_INPUTTAPDELAY))
+			return -ENODEV;
+		if (arasan_phy_write(host, DLL_TRIM, PHY_DLLTRIM))
+			return -ENODEV;
+		if (arasan_phy_write(host, 0, PHY_DLLCONTROL))
+			return -ENODEV;
+		dll_sts = (clk_sel<<5) | DLL_ENABLE;
+		if (arasan_phy_write(host, dll_sts, PHY_DLLCONTROL))
+			return -ENODEV;
+		if (arasan_phy_read(host, PHY_DLLCONTROL, &val))
+			return -ENODEV;
+	} else {
+		switch (host->mmc->ios.timing) {
+		case MMC_TIMING_LEGACY:
+			if (arasan_phy_write(host, 0x4, PHY_MODECONTROL))
+				return -ENODEV;
+			if (arasan_phy_write(host, 0, PHY_INPUTTAPDELAY))
+				return -ENODEV;
+			if (arasan_phy_write(host, 0, PHY_OUTPUTTAPDELAY))
+				return -ENODEV;
+			if (arasan_phy_write(host, 0, PHY_DLLCONTROL))
+				return -ENODEV;
+			if (arasan_phy_read(host, PHY_DLLCONTROL, &val))
+				return -ENODEV;
+			break;
+		case MMC_TIMING_MMC_HS:
+			if (arasan_phy_write(host, 0, PHY_MODECONTROL))
+				return -ENODEV;
+			otap = ((0x2<<3) | OTAP_DLY_EN);
+			if (arasan_phy_write(host, otap, PHY_OUTPUTTAPDELAY))
+				return -ENODEV;
+			itap = ((0x2<<1) | ITAP_DLY_EN);
+			if (arasan_phy_write(host, itap, PHY_INPUTTAPDELAY))
+				return -ENODEV;
+			if (arasan_phy_write(host, 0, PHY_INPUTTAPDELAY))
+				return -ENODEV;
+			if (arasan_phy_write(host, DLL_TRIM, PHY_DLLTRIM))
+				return -ENODEV;
+			if (arasan_phy_write(host, 0, PHY_DLLCONTROL))
+				return -ENODEV;
+			dll_sts = (clk_sel<<5) | DLL_ENABLE;
+			if (arasan_phy_write(host, dll_sts, PHY_DLLCONTROL))
+				return -ENODEV;
+			if (arasan_phy_read(host, PHY_DLLCONTROL, &val))
+				return -ENODEV;
+			break;
+		case MMC_TIMING_MMC_HS200:
+			if (arasan_phy_write(host, 0, PHY_MODECONTROL))
+				return -ENODEV;
+			if (arasan_phy_read(host, PHY_IOPADCONTROL1, &val))
+				return -ENODEV;
+			io_pad = val | (host->mmc->ios.drv_type<<2);
+			if (arasan_phy_write(host, io_pad, PHY_IOPADCONTROL1))
+				return -ENODEV;
+			otap = ((0x2<<1) | OTAP_DLY_EN);
+			if (arasan_phy_write(host, otap, PHY_OUTPUTTAPDELAY))
+				return -ENODEV;
+			if (arasan_phy_write(host, 0, PHY_INPUTTAPDELAY))
+				return -ENODEV;
+			if (arasan_phy_write(host, DLL_TRIM, PHY_DLLTRIM))
+				return -ENODEV;
+			if (arasan_phy_write(host, 0, PHY_DLLCONTROL))
+				return -ENODEV;
+			dll_sts = (clk_sel<<5) | DLL_ENABLE;
+			if (arasan_phy_write(host, dll_sts, PHY_DLLCONTROL))
+				return -ENODEV;
+			if (arasan_phy_read(host, PHY_DLLCONTROL, &val))
+				return -ENODEV;
+			if (arasan_phy_read(host, PHY_DLLCONTROL, &val))
+				return -ENODEV;
+			break;
+		case MMC_TIMING_UHS_DDR50:
+			if (arasan_phy_write(host, 0x8, PHY_MODECONTROL))
+				return -ENODEV;
+			otap = ((0x2<<3) | OTAP_DLY_EN);
+			if (arasan_phy_write(host, otap, PHY_OUTPUTTAPDELAY))
+				return -ENODEV;
+			itap = ((0x2<<1) | ITAP_DLY_EN);
+			if (arasan_phy_write(host, itap, PHY_INPUTTAPDELAY))
+				return -ENODEV;
+			if (arasan_phy_write(host, DLL_TRIM, PHY_DLLTRIM))
+				return -ENODEV;
+			if (arasan_phy_write(host, 0, PHY_DLLCONTROL))
+				return -ENODEV;
+			dll_sts = (clk_sel<<5) | DLL_ENABLE;
+			if (arasan_phy_write(host, dll_sts, PHY_DLLCONTROL))
+				return -ENODEV;
+			if (arasan_phy_read(host, PHY_DLLCONTROL, &val))
+				return -ENODEV;
+			break;
+		case MMC_TIMING_MMC_HS400:
+			if (arasan_phy_write(host, 0x2, PHY_MODECONTROL))
+				return -ENODEV;
+			if (arasan_phy_read(host, PHY_IOPADCONTROL1, &val))
+				return -ENODEV;
+			io_pad = val | (host->mmc->ios.drv_type<<2);
+			if (arasan_phy_write(host, io_pad, PHY_IOPADCONTROL1))
+				return -ENODEV;
+			otap = ((0x2<<1) | OTAP_DLY_EN);
+			if (arasan_phy_write(host, otap, PHY_OUTPUTTAPDELAY))
+				return -ENODEV;
+			itap = ((0xA<<1) | ITAP_DLY_EN);
+			if (arasan_phy_write(host, itap, PHY_INPUTTAPDELAY))
+				return -ENODEV;
+			if (arasan_phy_write(host, DLL_TRIM, PHY_DLLTRIM))
+				return -ENODEV;
+			if (arasan_phy_write(host, 0, PHY_DLLCONTROL))
+				return -ENODEV;
+			dll_sts = (clk_sel<<5) | DLL_ENABLE;
+			if (arasan_phy_write(host, dll_sts, PHY_DLLCONTROL))
+				return -ENODEV;
+			if (arasan_phy_read(host, PHY_DLLCONTROL, &val))
+				return -ENODEV;
+			if (arasan_phy_write(host, 0x0E, PHY_CLKBUFSELECT))
+				return -ENODEV;
+			break;
+		default:
+			break;
+		}
+	}
+	return 0;
+}
+
+int arasan_pci_probe(struct sdhci_pci_chip *chip)
+{
+	struct pci_dev *dev;
+
+	dev = pci_get_device(PCI_VENDOR_ID_ARASAN,
+			PCI_DEVICE_ID_ARASAN_PHY_EMMC, NULL);

What is this for?

We shouldn't have header files we don't need.  Please put definitions in
sdhci-pci-arasan.c instead.

These 3 are only ever going to be called in sdhci-pci-core.c so let's just
put the declarations there - see below.  Also "extern" is redundant.

Let's just put the declarations here:

int arasan_pci_probe_slot(struct sdhci_pci_slot *slot);
int arasan_pci_probe(struct sdhci_pci_chip *chip);
void arasan_sdhci_set_clock(struct sdhci_host *host, unsigned int clock);

+static const struct sdhci_ops arasan_sdhci_pci_ops = {
+	.set_clock			= arasan_sdhci_set_clock,
+	.enable_dma			= sdhci_pci_enable_dma,
+	.set_bus_width			= sdhci_pci_set_bus_width,

sdhci_pci_set_bus_width has been replaced with sdhci_set_bus_width

Please omit the comment - the ID names are already self-explanatory.

+#define PCI_VENDOR_ID_ARASAN	0x16e6
+#define PCI_DEVICE_ID_ARASAN_PHY_EMMC	0x0670
+
 /*
  * PCI device class and mask
  */