On Friday 10 May 2013, Christian Daudt wrote:ok. Will do+I see no use for this structure to be separate: a lot of the fields are
+struct sdhci_bcm_kona_cfg {
+ unsigned int max_freq;
+ int is_8bit;
+ int irq;
+ int cd_gpio;
+ int wp_gpio;
+ int non_removable;
+};
duplicated in the sdhci_host, or should just get merged into
sdhci_bcm_kona_dev.
ok.+struct sdhci_bcm_kona_dev {The *dev and *host members in this structure are redundant, just
+ struct sdhci_bcm_kona_cfg *cfg;
+ struct device *dev;
+ struct sdhci_host *host;
+ struct clk *peri_clk;
+ struct clk *sleep_clk;
+};
allocate it together with sdhci_host and use use container_of()
to get from the sdhci_host back it it.
I don't think that there's any reason. will replace.+static void sdhci_bcm_kona_sd_init(struct sdhci_host *host)Why not use msleep() instead of mdelay() here?
+{
+ 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);
+}
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 int sdhci_bcm_kona_sd_card_emulate(struct sdhci_host *host, int insert)Does the delay have to be done with interrupts disabled? That is not particularly
+{
+ 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);
nice.
I hope the hardware designers have been appropriately punished for the creating
such crap.
I'll look into this one.+static void sdhci_bcm_kona_init_74_clocks(struct sdhci_host *host,This requires at the minimum a comment about why the mdelay is needed.
+ u8 power_mode)
+{
+ if (power_mode == MMC_POWER_OFF)
+ return;
+ else
+ mdelay(10);
+}
Maybe we can change the set_ios function so we never need to call it
in atomic context.
ok.+static struct sdhci_bcm_kona_cfg * __init sdhci_bcm_kona_parse_dt(fold this function into probe()
+ struct platform_device *pdev)
+{
+ struct sdhci_bcm_kona_cfg *cfg;
+ struct device_node *np = pdev->dev.of_node;
+ u32 temp;
ok+ if (!np)impossible
+ return NULL;
what is not needed ?+ cfg = devm_kzalloc(&pdev->dev, sizeof(*cfg), GFP_KERNEL);Not needed
+ if (!cfg) {
+ dev_err(&pdev->dev, "Can't allocate platform cfg\n");
+ return NULL;
+ }
I'll remove it.+static int __init sdhci_bcm_kona_probe(struct platform_device *pdev)constant, so not needed.
+{
+ const struct of_device_id *match;
ok+ struct sdhci_bcm_kona_cfg *kona_cfg = NULL;No need to initialize this.
why make this global ?+ const struct sdhci_pltfm_data *plat_data;make it global.
ok.
+ struct sdhci_bcm_kona_dev *kona_dev = NULL;No need to initialize this.
ok, I'll look into this.+ kona_dev = devm_kzalloc(dev, sizeof(*kona_dev), GFP_KERNEL);It is rather silly to have the base sdhci code allocate extra
+ if (!kona_dev) {
+ dev_err(dev, "Can't allocate kona_dev\n");
+ ret = -ENOMEM;
+ goto err_pltfm_free;
+ }
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.
A collective effort :)+MODULE_AUTHOR("Broadcom");No person?