On Thursday 01 October 2015 13:39:29 Alim Akhtar wrote:
> +static int exynos7_ufs_drv_init(struct device *dev, struct exynos_ufs *ufs)
> +{
> + int ret;
> + const char *const clks[] = {
> + "mout_sclk_combo_phy_embedded",
> + "top_sclk_phy_fsys1_26m",
> + };
> +
These clocks are neither in the binding nor in the example.
> +struct exynos_ufs_drv_data exynos_ufs_drvs[] = {
> +{
> + .compatible = "samsung,exynos7-ufs",
> + .uic_attr = &exynos7_uic_attr,
> + .quirks = UFSHCI_QUIRK_BYTE_ALIGN_UTRD |
> + UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR |
> + UFSHCI_QUIRK_BROKEN_HCE |
> + UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR,
> + .opts = EXYNOS_UFS_OPT_HAS_APB_CLK_CTRL |
> + EXYNOS_UFS_OPT_BROKEN_AUTO_CLK_CTRL |
> + EXYNOS_UFS_OPT_BROKEN_RX_SEL_IDX,
> + .drv_init = exynos7_ufs_drv_init,
> + .pre_link = exynos7_ufs_pre_link,
> + .post_link = exynos7_ufs_post_link,
> + .pre_pwr_change = exynos7_ufs_pre_pwr_change,
> + .post_pwr_change = exynos7_ufs_post_pwr_change,
> +}, {
> +}, };
The indentation is a bit unusual here.
> diff --git a/drivers/scsi/ufs/ufs-exynos-hw.h
> b/drivers/scsi/ufs/ufs-exynos-hw.h
> new file mode 100644
> index 0000000..8464ec8
> --- /dev/null
> +++ b/drivers/scsi/ufs/ufs-exynos-hw.h
> @@ -0,0 +1,43 @@
> +/*
> + * UFS Host Controller driver for Exynos specific extensions
> + *
> + * Copyright (C) 2014-2015 Samsung Electronics Co., Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#ifndef _UFS_EXYNOS_HW_H_
> +#define _UFS_EXYNOS_HW_H_
> +
> +#include "ufs-exynos.h"
> +#include "unipro.h"
> +
> +static struct exynos_ufs_uic_attr exynos7_uic_attr = {
You cannot put 'static' variables into a header file.
> +
> +/**
> + * exynos_ufs_auto_ctrl_hcc - HCI core clock control by h/w
> + * Control should be disabled in the below cases
> + * - Before host controller S/W reset
> + * - Access to UFS protector's register
> + */
> +static void exynos_ufs_auto_ctrl_hcc(struct exynos_ufs *ufs, bool en)
> +{
> + u32 misc = hci_readl(ufs, HCI_MISC);
> +
> + if (en)
> + hci_writel(ufs, misc | HCI_CORECLK_CTRL_EN, HCI_MISC);
> + else
> + hci_writel(ufs, misc & ~HCI_CORECLK_CTRL_EN, HCI_MISC);
Does this need a spinlock to ensure the change is done atomically?
> +}
> +
> +static void exynos_ufs_ctrl_clkstop(struct exynos_ufs *ufs, bool en)
> +{
> + u32 ctrl = hci_readl(ufs, HCI_CLKSTOP_CTRL);
> + u32 misc = hci_readl(ufs, HCI_MISC);
> +
> + if (en) {
> + hci_writel(ufs, misc | CLK_CTRL_EN_MASK, HCI_MISC);
> + hci_writel(ufs, ctrl | CLK_STOP_MASK, HCI_CLKSTOP_CTRL);
> + } else {
> + hci_writel(ufs, ctrl & ~CLK_STOP_MASK, HCI_CLKSTOP_CTRL);
> + hci_writel(ufs, misc & ~CLK_CTRL_EN_MASK, HCI_MISC);
> + }
same here.
> +
> +static int exynos_ufs_get_clk_info(struct exynos_ufs *ufs)
> +{
> + struct ufs_hba *hba = ufs->hba;
> + struct list_head *head = &hba->clk_list_head;
> + struct ufs_clk_info *clki;
> + u32 pclk_rate;
> + u32 f_min, f_max;
> + u8 div = 0;
> + int ret = 0;
> +
> + if (!head || list_empty(head))
> + goto out;
> +
> + list_for_each_entry(clki, head, list) {
> + if (!IS_ERR_OR_NULL(clki->clk)) {
> + if (!strcmp(clki->name, "aclk_ufs"))
> + ufs->clk_hci_core = clki->clk;
> + else if (!strcmp(clki->name, "sclk_unipro_apb"))
> + ufs->clk_apb = clki->clk;
> + else if (!strcmp(clki->name, "sclk_unipro_main"))
> + ufs->clk_unipro_main = clki->clk;
> + }
> + }
Using IS_ERR_OR_NULL is normally a bug. Also the list/loop can likely be
replaced with another way to express this.
> + do {
> + delta = h8_time - ktime_us_delta(ktime_get(),
> + ufs->entry_hibern8_t);
> + if (delta <= 0)
> + break;
> +
> + us = min_t(s64, delta, USEC_PER_MSEC);
> + if (us >= 10)
> + usleep_range(us, us + 10);
> + else
> + udelay(us);
> + } while (1);
us is at least 1000 (USEC_PER_MSEC), so the else clause is never needed.
> +
> +const struct ufs_hba_variant_ops ufs_hba_exynos_ops = {
> + .name = "exynos_ufs",
> + .init = exynos_ufs_init,
> + .hce_enable_notify = exynos_ufs_hce_enable_notify,
> + .link_startup_notify = exynos_ufs_link_startup_notify,
> + .pwr_change_notify = exynos_ufs_pwr_change_notify,
> + .specify_nexus_t_xfer_req = exynos_ufs_specify_nexus_t_xfer_req,
> + .specify_nexus_t_tm_req = exynos_ufs_specify_nexus_t_tm_req,
> + .hibern8_notify = exynos_ufs_hibern8_notify,
> + .suspend = exynos_ufs_suspend,
> + .resume = exynos_ufs_resume,
> +};
> +EXPORT_SYMBOL(ufs_hba_exynos_ops);
As mentioned in my reply to the build bug report, please restructure the
driver so this is not a global exported function but gets passed by the
probe function of the exynos driver into an exported function of the
common driver.
> diff --git a/drivers/scsi/ufs/ufs-exynos.h b/drivers/scsi/ufs/ufs-exynos.h
> new file mode 100644
> index 0000000..58aa714
> --- /dev/null
> +++ b/drivers/scsi/ufs/ufs-exynos.h
> @@ -0,0 +1,463 @@
> +/*
> + * UFS Host Controller driver for Exynos specific extensions
> + *
> + * Copyright (C) 2014-2015 Samsung Electronics Co., Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#ifndef _UFS_EXYNOS_H_
> +#define _UFS_EXYNOS_H_
You have a lot of things in this header that are only used in one of the
.c files, so just move them there and make the header as small as possible.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
