From: "Christian Daudt" <csd@broadcom.com> To: "Arnd Bergmann" <arnd@arndb.de> Cc: "Grant Likely" <grant.likely@secretlab.ca>, "Rob Herring" <rob.herring@calxeda.com>, "Rob Landley" <rob@landley.net>, "Russell King" <linux@arm.linux.org.uk>, "Chris Ball" <cjb@laptop.org>, "Stephen Warren" <swarren@nvidia.com>, "Olof Johansson" <olof@lixom.net>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>, "Wei WANG" <wei_wang@realsil.com.cn>, "Ludovic Desroches" <ludovic.desroches@atmel.com>, devicetree-discuss@lists.ozlabs.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mmc@vger.kernel.org, csd_b@daudt.org Subject: Re: [PATCH V2 1/2] ARM: mmc: bcm281xx SDHCI driver Date: Tue, 21 May 2013 01:22:05 -0700 [thread overview] Message-ID: <519B2EAD.4000208@broadcom.com> (raw) In-Reply-To: <201305170009.46244.arnd@arndb.de> Hi Arnd, Thanks for the review. See below for comments. On 13-05-16 03:09 PM, Arnd Bergmann wrote: > On Friday 10 May 2013, Christian Daudt wrote: >> + >> +struct sdhci_bcm_kona_cfg { >> + unsigned int max_freq; >> + int is_8bit; >> + int irq; >> + int cd_gpio; >> + int wp_gpio; >> + int non_removable; >> +}; > I see no use for this structure to be separate: a lot of the fields are > duplicated in the sdhci_host, or should just get merged into > sdhci_bcm_kona_dev. ok. Will do >> +struct sdhci_bcm_kona_dev { >> + struct sdhci_bcm_kona_cfg *cfg; >> + struct device *dev; >> + struct sdhci_host *host; >> + struct clk *peri_clk; >> + struct clk *sleep_clk; >> +}; > The *dev and *host members in this structure are redundant, just > allocate it together with sdhci_host and use use container_of() > to get from the sdhci_host back it it. ok. >> +static void sdhci_bcm_kona_sd_init(struct sdhci_host *host) >> +{ >> + unsigned int val; >> + >> + /* enable the interrupt from the IP core */ >> + val = sdhci_readl(host, KONA_SDHOST_COREIMR); >> + val |= KONA_SDHOST_IP; >> + sdhci_writel(host, val, KONA_SDHOST_COREIMR); >> + >> + /* Enable the AHB clock gating module to the host */ >> + val = sdhci_readl(host, KONA_SDHOST_CORECTRL); >> + val |= KONA_SDHOST_EN; >> + >> + /* >> + * Back-to-Back register write needs a delay of 1ms at bootup (min 10uS) >> + * Back-to-Back writes to same register needs delay when SD bus clock >> + * is very low w.r.t AHB clock, mainly during boot-time and during card >> + * insert-removal. >> + */ >> + mdelay(1); >> + sdhci_writel(host, val, KONA_SDHOST_CORECTRL); >> +} > Why not use msleep() instead of mdelay() here? I don't think that there's any reason. will replace. > >> +static int sdhci_bcm_kona_sd_card_emulate(struct sdhci_host *host, int insert) >> +{ >> + struct sdhci_pltfm_host *pltfm_priv = sdhci_priv(host); >> + struct sdhci_bcm_kona_dev *kona_dev = pltfm_priv->priv; >> + u32 val; >> + unsigned long flags; >> + >> + /* this function can be called from various contexts including ISR */ >> + spin_lock_irqsave(&host->lock, flags); >> + /* Ensure SD bus scanning to detect media change */ >> + host->mmc->rescan_disable = 0; >> + >> + /* >> + * Back-to-Back register write needs a delay of min 10uS. >> + * Back-to-Back writes to same register needs delay when SD bus clock >> + * is very low w.r.t AHB clock, mainly during boot-time and during card >> + * insert-removal. >> + * We keep 20uS >> + */ >> + udelay(20); >> + val = sdhci_readl(host, KONA_SDHOST_CORESTAT); > Does the delay have to be done with interrupts disabled? That is not particularly > nice. > > I hope the hardware designers have been appropriately punished for the creating > such crap. I had some internal discussions on this one, and the code was originally written for non-threaded irqs. Now that it is only called as threaded_irq thread_fn, it is safe to replace the spinlock that includes the delay with a mutex instead. >> +static void sdhci_bcm_kona_init_74_clocks(struct sdhci_host *host, >> + u8 power_mode) >> +{ >> + if (power_mode == MMC_POWER_OFF) >> + return; >> + else >> + mdelay(10); >> +} > This requires at the minimum a comment about why the mdelay is needed. > Maybe we can change the set_ios function so we never need to call it > in atomic context. I'll look into this one. >> +static struct sdhci_bcm_kona_cfg * __init sdhci_bcm_kona_parse_dt( >> + struct platform_device *pdev) >> +{ >> + struct sdhci_bcm_kona_cfg *cfg; >> + struct device_node *np = pdev->dev.of_node; >> + u32 temp; > fold this function into probe() ok. >> + if (!np) >> + return NULL; > impossible ok >> + cfg = devm_kzalloc(&pdev->dev, sizeof(*cfg), GFP_KERNEL); >> + if (!cfg) { >> + dev_err(&pdev->dev, "Can't allocate platform cfg\n"); >> + return NULL; >> + } > Not needed what is not needed ? >> +static int __init sdhci_bcm_kona_probe(struct platform_device *pdev) >> +{ >> + const struct of_device_id *match; > constant, so not needed. I'll remove it. >> + struct sdhci_bcm_kona_cfg *kona_cfg = NULL; > No need to initialize this. ok >> + const struct sdhci_pltfm_data *plat_data; > make it global. why make this global ? > >> + struct sdhci_bcm_kona_dev *kona_dev = NULL; > No need to initialize this. ok. >> + kona_dev = devm_kzalloc(dev, sizeof(*kona_dev), GFP_KERNEL); >> + if (!kona_dev) { >> + dev_err(dev, "Can't allocate kona_dev\n"); >> + ret = -ENOMEM; >> + goto err_pltfm_free; >> + } > It is rather silly to have the base sdhci code allocate extra > memory for the platform drivers but then require an extra allocation. > Better change the sdhci_pltfm_init function to let you pass the extra > allocation size. ok, I'll look into this. >> +MODULE_AUTHOR("Broadcom"); > No person? > A collective effort :) Thanks, csd
WARNING: multiple messages have this Message-ID (diff)
From: csd@broadcom.com (Christian Daudt) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH V2 1/2] ARM: mmc: bcm281xx SDHCI driver Date: Tue, 21 May 2013 01:22:05 -0700 [thread overview] Message-ID: <519B2EAD.4000208@broadcom.com> (raw) In-Reply-To: <201305170009.46244.arnd@arndb.de> Hi Arnd, Thanks for the review. See below for comments. On 13-05-16 03:09 PM, Arnd Bergmann wrote: > On Friday 10 May 2013, Christian Daudt wrote: >> + >> +struct sdhci_bcm_kona_cfg { >> + unsigned int max_freq; >> + int is_8bit; >> + int irq; >> + int cd_gpio; >> + int wp_gpio; >> + int non_removable; >> +}; > I see no use for this structure to be separate: a lot of the fields are > duplicated in the sdhci_host, or should just get merged into > sdhci_bcm_kona_dev. ok. Will do >> +struct sdhci_bcm_kona_dev { >> + struct sdhci_bcm_kona_cfg *cfg; >> + struct device *dev; >> + struct sdhci_host *host; >> + struct clk *peri_clk; >> + struct clk *sleep_clk; >> +}; > The *dev and *host members in this structure are redundant, just > allocate it together with sdhci_host and use use container_of() > to get from the sdhci_host back it it. ok. >> +static void sdhci_bcm_kona_sd_init(struct sdhci_host *host) >> +{ >> + unsigned int val; >> + >> + /* enable the interrupt from the IP core */ >> + val = sdhci_readl(host, KONA_SDHOST_COREIMR); >> + val |= KONA_SDHOST_IP; >> + sdhci_writel(host, val, KONA_SDHOST_COREIMR); >> + >> + /* Enable the AHB clock gating module to the host */ >> + val = sdhci_readl(host, KONA_SDHOST_CORECTRL); >> + val |= KONA_SDHOST_EN; >> + >> + /* >> + * Back-to-Back register write needs a delay of 1ms at bootup (min 10uS) >> + * Back-to-Back writes to same register needs delay when SD bus clock >> + * is very low w.r.t AHB clock, mainly during boot-time and during card >> + * insert-removal. >> + */ >> + mdelay(1); >> + sdhci_writel(host, val, KONA_SDHOST_CORECTRL); >> +} > Why not use msleep() instead of mdelay() here? I don't think that there's any reason. will replace. > >> +static int sdhci_bcm_kona_sd_card_emulate(struct sdhci_host *host, int insert) >> +{ >> + struct sdhci_pltfm_host *pltfm_priv = sdhci_priv(host); >> + struct sdhci_bcm_kona_dev *kona_dev = pltfm_priv->priv; >> + u32 val; >> + unsigned long flags; >> + >> + /* this function can be called from various contexts including ISR */ >> + spin_lock_irqsave(&host->lock, flags); >> + /* Ensure SD bus scanning to detect media change */ >> + host->mmc->rescan_disable = 0; >> + >> + /* >> + * Back-to-Back register write needs a delay of min 10uS. >> + * Back-to-Back writes to same register needs delay when SD bus clock >> + * is very low w.r.t AHB clock, mainly during boot-time and during card >> + * insert-removal. >> + * We keep 20uS >> + */ >> + udelay(20); >> + val = sdhci_readl(host, KONA_SDHOST_CORESTAT); > Does the delay have to be done with interrupts disabled? That is not particularly > nice. > > I hope the hardware designers have been appropriately punished for the creating > such crap. I had some internal discussions on this one, and the code was originally written for non-threaded irqs. Now that it is only called as threaded_irq thread_fn, it is safe to replace the spinlock that includes the delay with a mutex instead. >> +static void sdhci_bcm_kona_init_74_clocks(struct sdhci_host *host, >> + u8 power_mode) >> +{ >> + if (power_mode == MMC_POWER_OFF) >> + return; >> + else >> + mdelay(10); >> +} > This requires at the minimum a comment about why the mdelay is needed. > Maybe we can change the set_ios function so we never need to call it > in atomic context. I'll look into this one. >> +static struct sdhci_bcm_kona_cfg * __init sdhci_bcm_kona_parse_dt( >> + struct platform_device *pdev) >> +{ >> + struct sdhci_bcm_kona_cfg *cfg; >> + struct device_node *np = pdev->dev.of_node; >> + u32 temp; > fold this function into probe() ok. >> + if (!np) >> + return NULL; > impossible ok >> + cfg = devm_kzalloc(&pdev->dev, sizeof(*cfg), GFP_KERNEL); >> + if (!cfg) { >> + dev_err(&pdev->dev, "Can't allocate platform cfg\n"); >> + return NULL; >> + } > Not needed what is not needed ? >> +static int __init sdhci_bcm_kona_probe(struct platform_device *pdev) >> +{ >> + const struct of_device_id *match; > constant, so not needed. I'll remove it. >> + struct sdhci_bcm_kona_cfg *kona_cfg = NULL; > No need to initialize this. ok >> + const struct sdhci_pltfm_data *plat_data; > make it global. why make this global ? > >> + struct sdhci_bcm_kona_dev *kona_dev = NULL; > No need to initialize this. ok. >> + kona_dev = devm_kzalloc(dev, sizeof(*kona_dev), GFP_KERNEL); >> + if (!kona_dev) { >> + dev_err(dev, "Can't allocate kona_dev\n"); >> + ret = -ENOMEM; >> + goto err_pltfm_free; >> + } > It is rather silly to have the base sdhci code allocate extra > memory for the platform drivers but then require an extra allocation. > Better change the sdhci_pltfm_init function to let you pass the extra > allocation size. ok, I'll look into this. >> +MODULE_AUTHOR("Broadcom"); > No person? > A collective effort :) Thanks, csd
next prev parent reply other threads:[~2013-05-21 8:22 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2013-05-10 15:48 [PATCH V2 1/2] ARM: mmc: bcm281xx SDHCI driver Christian Daudt 2013-05-10 15:48 ` Christian Daudt 2013-05-10 15:48 ` [PATCH V2 2/2] ARM: mmc: bcm281xx SDHCI driver (dt mods) Christian Daudt 2013-05-10 15:48 ` Christian Daudt 2013-05-22 18:01 ` Matt Porter 2013-05-22 18:01 ` Matt Porter 2013-05-16 22:09 ` [PATCH V2 1/2] ARM: mmc: bcm281xx SDHCI driver Arnd Bergmann 2013-05-16 22:09 ` Arnd Bergmann 2013-05-21 8:22 ` Christian Daudt [this message] 2013-05-21 8:22 ` Christian Daudt 2013-05-21 18:23 ` Arnd Bergmann 2013-05-21 18:23 ` Arnd Bergmann
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=519B2EAD.4000208@broadcom.com \ --to=csd@broadcom.com \ --cc=arnd@arndb.de \ --cc=cjb@laptop.org \ --cc=csd_b@daudt.org \ --cc=devicetree-discuss@lists.ozlabs.org \ --cc=grant.likely@secretlab.ca \ --cc=gregkh@linuxfoundation.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-doc@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mmc@vger.kernel.org \ --cc=linux@arm.linux.org.uk \ --cc=ludovic.desroches@atmel.com \ --cc=olof@lixom.net \ --cc=rob.herring@calxeda.com \ --cc=rob@landley.net \ --cc=swarren@nvidia.com \ --cc=wei_wang@realsil.com.cn \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.