Hi Pierre, Thanks a lot for the excellent review! They are valuable comments. I'll fix them in next submission.
Regards, Yunpeng >-----Original Message----- >From: Tardy, Pierre >Sent: Friday, December 17, 2010 10:57 PM >To: Gao, Yunpeng; [email protected] >Subject: RE: [Meego-kernel] [patch v2 1/2]MFLD mmc: Implement the eMMC mutex >acquire/release APIs > >> >> From bc325f4abbcbd7e4032ce148d7a6850eb2a754c8 Mon Sep 17 00:00:00 2001 >> From: Yunpeng Gao <[email protected]> >> Date: Fri, 17 Dec 2010 19:00:10 +0800 >> Subject: [PATCH] Implement the Medfield eMMC mutex (Dekker Algorithm) >acquire/release APIs > >The subject line is too long. You should restraint your subject lines to 80 >chars, >and avoid special chars line ( ) / >When your commit is converted to a patch, the name will be truncated to 80 >chars, >and special chars removed. > >Good subject would be >Implement the Medfield eMMC mutex APIs > >> >> Changes compared with the old implementation: >> 1. Remove the card re-init operation if last owner is SCU. >> 2. Add a usage counter to manage the acquire/release pair on IA side. >Dont put change-from-last-submission comment in patch comment. >You should rather put. > >1) Problem you want to resolve. >2) How you resolve it. > >One should understand rationals of your patch without having to look in mailing >lists for original discussion. > >> >> +static int intel_mfld_emmc0_probe(struct sdhci_pci_chip *chip) >> +{ >> + if (chip->pdev->revision == 0) /* Penwell A0 */ >> + chip->quirks |= SDHCI_QUIRK_MFD_EMMC_SDIO_RESTRICTION; > > >A0 references should be removed when you will push that upstream. > >> + else if (chip->pdev->revision == 4) /* Penwell B0 */ >> + /* >> + * The eMMC mutex (Dekker algorithm) support in SCU firmware >> + * is only available for Penwell B0. >> + */ >> + chip->quirks |= SDHCI_QUIRK_NEED_DEKKER_MUTEX; >> + >> + return 0; >> +} >> + >> +/* >> + * Get the base address in shared SRAM for eMMC mutex >> + * (Dekker's algorithm) through IPC call. >> + * >> + * Please note it'll always return 0 whether the address requesting >> + * success or not. So, the mmc driver will still work well if the scu >> + * firmware is not ready yet. >Then if this always work, you dont need to detect A0 in previous function. >A0 does not support the IPC so it will just work as B0 with old FW. > >> +/* Implement the Dekker's algorithm on the IA processor side. >Should use kernel doc style comments >Should say that this function sleeps. > >> + * >> + * Return value: >> + * 0 - Acquried the ownership successfully. The last owner is IA >> + * 1 - Acquried the ownership succesffully. The last owenr is SCU >> + * -EBUSY - failed to acquire ownership within the timeout period. >> + */ >> +static int sdhci_acquire_ownership(struct mmc_host *mmc) >> +{ >> + struct sdhci_host *host; >> + unsigned long t1, t2; >> + >> + host = mmc_priv(mmc); >> + >> + if (!((host->quirks & SDHCI_QUIRK_NEED_DEKKER_MUTEX) && >> + (host->sram_addr))) >> + return 0; >> + >> + atomic_inc(&host->usage_cnt); >What if two IA threads are entering in this loop simulteanously? > >> + >> + DBG("Acquire ownership - eMMC owner: %d, IA req: %d, SCU req: %d\n", >> + readl(host->sram_addr + DEKKER_EMMC_OWNER_OFFSET), >> + readl(host->sram_addr + DEKKER_IA_REQ_OFFSET), >> + readl(host->sram_addr + DEKKER_SCU_REQ_OFFSET)); >> + >> + writel(1, host->sram_addr + DEKKER_IA_REQ_OFFSET); >> + >> + t1 = jiffies + 10 * HZ; >> + t2 = 500; >> + >> + while (readl(host->sram_addr + DEKKER_SCU_REQ_OFFSET)) { >Access macro might make this easier to read like >READ_DEKKER_SCU_REQ(host) > >> + if (readl(host->sram_addr + DEKKER_EMMC_OWNER_OFFSET) != >> + DEKKER_OWNER_IA) { >> + writel(0, host->sram_addr + DEKKER_IA_REQ_OFFSET); >> + while (t2) { >> + if (readl(host->sram_addr + >> + DEKKER_EMMC_OWNER_OFFSET) == >> + DEKKER_OWNER_IA) >> + break; >> + msleep(10); >> + t2--; >> + } >> + if (t2) { >> + writel(1, host->sram_addr + >> + DEKKER_IA_REQ_OFFSET); >> + } else { >> + pr_err("eMMC mutex timeout (owner)!\n"); >> + goto timeout; >> + } >> + } >> + if (time_after(jiffies, t1)) { >> + pr_err("eMMC mutex timeout (req)!\n"); >> + goto timeout; >> + } >> + cpu_relax(); >Why cpu_relax() here, and msleep(10) for t2? Needs short inline comments. > >> + } >> + >> + if (readl(host->sram_addr + DEKKER_EMMC_OWNER_OFFSET) == >> + DEKKER_OWNER_IA) >> + return 1; /* Tell caller to re-config the host controller */ >> + >> + return 0; >> +timeout: >> + writel(DEKKER_OWNER_SCU, host->sram_addr + DEKKER_EMMC_OWNER_OFFSET); >> + writel(0, host->sram_addr + DEKKER_IA_REQ_OFFSET); >> + return -EBUSY; >> +} >> + >> +static void sdhci_release_ownership(struct mmc_host *mmc) >> +{ >> + struct sdhci_host *host; >> + >> + host = mmc_priv(mmc); >> + >> + if (!((host->quirks & SDHCI_QUIRK_NEED_DEKKER_MUTEX) && >> + (host->sram_addr))) >> + return; >> + >> + if (atomic_dec_and_test(&host->usage_cnt)) { >> + writel(DEKKER_OWNER_SCU, >> + host->sram_addr + DEKKER_EMMC_OWNER_OFFSET); >> + writel(0, host->sram_addr + DEKKER_IA_REQ_OFFSET); >> + DBG("Exit ownership - " >> + "eMMC owner: %d, IA req: %d, SCU req: %d\n", >> + readl(host->sram_addr + DEKKER_EMMC_OWNER_OFFSET), >> + readl(host->sram_addr + DEKKER_IA_REQ_OFFSET), >> + readl(host->sram_addr + DEKKER_SCU_REQ_OFFSET)); >> + } >> +} >> + >> +/* >> + * Re-configure host controller registers here since SCU firmware >> + * has possibly changed some registers already >> + */ >> +static void sdhci_reconfig_host_controller(struct mmc_host *mmc) >> +{ >> + struct sdhci_host *host; >> + >> + host = mmc_priv(mmc); >> + >> + if (!((host->quirks & SDHCI_QUIRK_NEED_DEKKER_MUTEX) && >> + (host->sram_addr))) >> + return; >> + >> + sdhci_init(host, 0); >> + sdhci_set_ios(host->mmc, &host->mmc->ios); >> +} >> + >> static const struct mmc_host_ops sdhci_ops = { >> .request = sdhci_request, >> .set_ios = sdhci_set_ios, >> .get_ro = sdhci_get_ro, >> .enable_sdio_irq = sdhci_enable_sdio_irq, >> + .acquire_ownership = sdhci_acquire_ownership, >> + .release_ownership = sdhci_release_ownership, >> + .reconfig_host_controller = sdhci_reconfig_host_controller, >> }; >> >> >/************************************************************************** >***\ >> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h >> index 01e4886..abb2cab 100644 >> --- a/include/linux/mmc/host.h >> +++ b/include/linux/mmc/host.h >> @@ -117,6 +117,11 @@ struct mmc_host_ops { >> >> /* optional callback for HC quirks */ >> void (*init_card)(struct mmc_host *host, struct mmc_card *card); >> + >> + /* optional callback for HC mutex (Dekker algorithm) */ >> + int (*acquire_ownership)(struct mmc_host *host); >> + void (*release_ownership)(struct mmc_host *host); >> + void (*reconfig_host_controller)(struct mmc_host *host); >> }; >> >> struct mmc_card; >> diff --git a/include/linux/mmc/sdhci.h b/include/linux/mmc/sdhci.h >> index dc712bb..5ae4eea 100644 >> --- a/include/linux/mmc/sdhci.h >> +++ b/include/linux/mmc/sdhci.h >> @@ -91,11 +91,23 @@ struct sdhci_host { >> /* Controller of Medfield specific restriction */ >> #define SDHCI_QUIRK_MFD_SD_RESTRICTION (1ULL<<33) >> #define SDHCI_QUIRK_MFD_EMMC_SDIO_RESTRICTION (1ULL<<34) >> - >> +/* One controller port will be accessed by driver and fw at the same time >*/ >> +#define SDHCI_QUIRK_NEED_DEKKER_MUTEX (1ULL<<35) >> >> int irq; /* Device IRQ */ >> void __iomem *ioaddr; /* Mapped address */ >> >> + /* XXX: SCU/X86 mutex variables base address in shared SRAM */ >> + void __iomem *sram_addr; /* Shared SRAM address */ >> + >> +#define DEKKER_EMMC_OWNER_OFFSET 0 >> +#define DEKKER_IA_REQ_OFFSET 0x04 >> +#define DEKKER_SCU_REQ_OFFSET 0x08 >> +#define DEKKER_OWNER_IA 0 >> +#define DEKKER_OWNER_SCU 1 >> + >> + atomic_t usage_cnt; /* eMMC mutex usage count */ >> + >> const struct sdhci_ops *ops; /* Low level hw interface */ >> >> struct regulator *vmmc; /* Power regulator */ >> -- >> 1.5.4.5 >> >> _______________________________________________ >> MeeGo-kernel mailing list >> [email protected] >> http://lists.meego.com/listinfo/meego-kernel _______________________________________________ MeeGo-kernel mailing list [email protected] http://lists.meego.com/listinfo/meego-kernel
