From: Arnd Bergmann <arnd@arndb.de>
To: Joao Pinto <Joao.Pinto@synopsys.com>
Cc: santosh.sy@samsung.com, h.vinayak@samsung.com,
julian.calaby@gmail.com, akinobu.mita@gmail.com,
hch@infradead.org, gbroner@codeaurora.org,
subhashj@codeaurora.org, CARLOS.PALMINHA@synopsys.com,
ijc+devicetree@hellion.org.uk, linux-kernel@vger.kernel.org,
linux-scsi@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH 3/3] add support for DWC UFS Host Controller
Date: Wed, 03 Feb 2016 13:54:25 +0100 [thread overview]
Message-ID: <2072510.CA47OHQsUN@wuerfel> (raw)
In-Reply-To: <a519617624ecdc98be82c376d4359a91f6a823b0.1454498292.git.jpinto@synopsys.com>
On Wednesday 03 February 2016 11:28:26 Joao Pinto wrote:
>
> Signed-off-by: Joao Pinto <jpinto@synopsys.com>
This needs a changelog comment, like every patch.
> @@ -0,0 +1,16 @@
> +* Universal Flash Storage (UFS) DesignWare Host Controller
> +
> +DWC_UFSHC nodes are defined to describe on-chip UFS host controllers.
> +Each UFS controller instance should have its own node.
> +
> +Required properties:
> +- compatible : compatible list, contains "snps,ufshcd"
Are there multiple versions of this controller? Usually for designware
parts the version is known, so we should document which versions exist
> +
> +config SCSI_UFS_DWC_HOOKS
> + bool "DesignWare hooks to UFS controller"
> + depends on SCSI_UFSHCD
> + ---help---
> + This selects the DesignWare hooks for the UFS host controller.
> +
> + Select this if you have a DesignWare UFS controller.
> + If unsure, say N.
This could be a silent symbol that gets selected by SCSI_UFS_DWC_PLAT
> +config SCSI_UFS_DWC_MPHY_TC
> + bool "Support for the Synopsys MPHY Test Chip"
> + depends on SCSI_UFS_DWC_HOOKS && (SCSI_UFSHCD_PCI || SCSI_UFS_DWC_PLAT)
> + ---help---
> + This selects the support for the Synopsys MPHY Test Chip.
> +
> + Select this if you have a Synopsys MPHY Test Chip.
> + If unsure, say N.
> +
> +config SCSI_UFS_DWC_20BIT_RMMI
> + bool "20-bit RMMI MPHY"
> + depends on SCSI_UFS_DWC_MPHY_TC
> + ---help---
> + This specifies that the Synopsys MPHY supports 40-bit RMMI operations.
> +
> + Select this if you are using a 40-bit RMMI Synopsys MPHY.
> + If unsure, say N.
> +
> +config SCSI_UFS_DWC_40BIT_RMMI
> + bool "40-bit RMMI MPHY"
> + depends on SCSI_UFS_DWC_MPHY_TC
> + ---help---
> + This specifies that the Synopsys MPHY supports 40-bit RMMI operations.
> +
> + Select this if you are using a 40-bit RMMI Synopsys MPHY.
> + If unsure, say N.
Move the PHY config options to drivers/phy/ along with the respective
drivers, the device using the PHY should not need to be aware which
one is being used.
> +/**
> + * ufshcd_dwc_setup_mphy()
> + * This function configures Local (host) Synopsys MPHY specific attributes
> + *
> + * @hba: Pointer to drivers structure
> + *
> + * Returns 0 on success non-zero value on failure
> + */
> +int ufshcd_dwc_setup_mphy(struct ufs_hba *hba)
> +{
> + int ret = 0;
> +
> +#ifdef CONFIG_SCSI_UFS_DWC_40BIT_RMMI
> + dev_info(hba->dev, "Configuring MPHY 40-bit RMMI");
> + ret = ufshcd_dwc_setup_40bit_rmmi(hba);
> + if (ret) {
> + dev_err(hba->dev, "40-bit RMMI configuration failed");
> + goto out;
> + }
> +#else
> +#ifdef CONFIG_SCSI_UFS_DWC_20BIT_RMMI
> + dev_info(hba->dev, "Configuring MPHY 20-bit RMMI");
> + ret = ufshcd_dwc_setup_20bit_rmmi(hba);
> + if (ret) {
> + dev_err(hba->dev, "20-bit RMMI configuration failed");
> + goto out;
> + }
> +#endif
> +#endif
> + /* To write Shadow register bank to effective configuration block */
> + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(VS_MPHYCFGUPDT), 0x01);
> + if (ret)
> + goto out;
> +
> + /* To configure Debug OMC */
> + ret = ufshcd_dme_set(hba, UIC_ARG_MIB(VS_DEBUGOMC), 0x01);
> +
> +out:
> + return ret;
> +}
Try to use the generic PHY abstraction here and remove all the #ifdef etc.
> diff --git a/drivers/scsi/ufs/ufshcd-pci.c b/drivers/scsi/ufs/ufshcd-pci.c
> index d15eaa4..66518a1 100644
> --- a/drivers/scsi/ufs/ufshcd-pci.c
> +++ b/drivers/scsi/ufs/ufshcd-pci.c
> @@ -34,6 +34,9 @@
> */
>
> #include "ufshcd.h"
> +#ifdef CONFIG_SCSI_UFS_DWC_HOOKS
> +#include "ufshcd-dwc.h"
> +#endif
> #include <linux/pci.h>
> #include <linux/pm_runtime.h>
>
> @@ -83,6 +86,16 @@ static int ufshcd_pci_runtime_idle(struct device *dev)
> #define ufshcd_pci_runtime_idle NULL
> #endif /* CONFIG_PM */
>
> +#ifdef CONFIG_SCSI_UFS_DWC_HOOKS
> +/**
> + * struct ufs_hba_dwc_vops - UFS DWC specific variant operations
> + */
> +static struct ufs_hba_variant_ops ufs_dwc_pci_hba_vops = {
> + .name = "dwc-pci",
> + .custom_probe_hba = ufshcd_dwc_configuration,
> +};
> +#endif
> +
> /**
> * ufshcd_pci_shutdown - main function to put the controller in reset state
> * @pdev: pointer to PCI device handle
> @@ -144,6 +157,9 @@ ufshcd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>
> INIT_LIST_HEAD(&hba->clk_list_head);
>
> +#ifdef CONFIG_SCSI_UFS_DWC_HOOKS
> + hba->vops = &ufs_dwc_pci_hba_vops;
> +#endif
> err = ufshcd_init(hba, mmio_base, pdev->irq);
> if (err) {
> dev_err(&pdev->dev, "Initialization failed\n");
> @@ -167,6 +183,10 @@ static const struct dev_pm_ops ufshcd_pci_pm_ops = {
>
> static const struct pci_device_id ufshcd_pci_tbl[] = {
> { PCI_VENDOR_ID_SAMSUNG, 0xC00C, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
> +#ifdef CONFIG_SCSI_UFS_DWC_HOOKS
> + { PCI_VENDOR_ID_SYNOPSYS, 0xB101, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
> + { PCI_VENDOR_ID_SYNOPSYS, 0xB102, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
> +#endif
> { } /* terminate list */
> };
I think you're better off with a separate PCI driver for this. Remove
all the #ifdef mess here, put whatever is dwc specific into a new file,
and perhaps move the common parts into a shared file that can be used
by both the samsung and designware drivers.
Arnd
next prev parent reply other threads:[~2016-02-03 12:55 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-03 11:28 [PATCH 0/3] add support for DWC UFS Controller Joao Pinto
2016-02-03 11:28 ` [PATCH 1/3] fixed typo in ufshcd-pltfrm Joao Pinto
2016-02-03 11:28 ` [PATCH 2/3] added support for ufs 2.0 Joao Pinto
2016-02-03 11:28 ` [PATCH 3/3] add support for DWC UFS Host Controller Joao Pinto
2016-02-03 12:54 ` Arnd Bergmann [this message]
2016-02-03 15:01 ` Joao Pinto
2016-02-03 15:01 ` Joao Pinto
2016-02-03 15:39 ` Arnd Bergmann
2016-02-03 15:54 ` Joao Pinto
2016-02-03 15:54 ` Joao Pinto
2016-02-04 16:27 ` Mark Rutland
2016-02-04 16:27 ` Mark Rutland
2016-02-08 15:17 ` Joao Pinto
2016-02-08 15:17 ` Joao Pinto
2016-02-08 15:30 ` Mark Rutland
2016-02-08 15:36 ` Joao Pinto
2016-02-08 15:36 ` Joao Pinto
2016-02-08 16:15 ` Mark Rutland
2016-02-08 16:17 ` Joao Pinto
2016-02-08 16:17 ` Joao Pinto
2016-02-03 17:21 ` Joao Pinto
2016-02-03 17:21 ` Joao Pinto
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=2072510.CA47OHQsUN@wuerfel \
--to=arnd@arndb.de \
--cc=CARLOS.PALMINHA@synopsys.com \
--cc=Joao.Pinto@synopsys.com \
--cc=akinobu.mita@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=gbroner@codeaurora.org \
--cc=h.vinayak@samsung.com \
--cc=hch@infradead.org \
--cc=ijc+devicetree@hellion.org.uk \
--cc=julian.calaby@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=santosh.sy@samsung.com \
--cc=subhashj@codeaurora.org \
/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: link
Be 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.