Re: [PATCH v6 2/2] add support for DWC UFS Host Controller

[Arnd Bergmann <arnd@arndb.de>]

On Wednesday 10 February 2016 16:06:13 Joao Pinto wrote:
> This patch has the goal to add support for DesignWare UFS Controller
> specific operations and to add specific platform and pci drivers.
> 
> Signed-off-by: Joao Pinto <jpinto@synopsys.com>
>  Documentation/devicetree/bindings/ufs/ufs-dwc.txt |  17 +
>  MAINTAINERS                                       |   6 +
>  drivers/scsi/ufs/Kconfig                          |  41 ++
>  drivers/scsi/ufs/Makefile                         |   3 +
>  drivers/scsi/ufs/ufs-dwc-pci.c                    | 180 ++++++
>  drivers/scsi/ufs/ufs-dwc.c                        | 102 +++
>  drivers/scsi/ufs/ufshcd-dwc.c                     | 736 ++++++++++++++++++++++
>  drivers/scsi/ufs/ufshcd-dwc.h                     |  18 +
>  drivers/scsi/ufs/ufshcd.c                         |  50 +-
>  drivers/scsi/ufs/ufshcd.h                         |  13 +
>  drivers/scsi/ufs/ufshci-dwc.h                     |  42 ++
>  drivers/scsi/ufs/ufshci.h                         |   1 +
>  drivers/scsi/ufs/unipro.h                         |  39 ++

Can you split this into separate patches for changes to the common code,
the addition of the PCI driver and the addition of the platform driver?

> diff --git a/Documentation/devicetree/bindings/ufs/ufs-dwc.txt b/Documentation/devicetree/bindings/ufs/ufs-dwc.txt
> new file mode 100644
> index 0000000..f38a3f5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ufs/ufs-dwc.txt
> @@ -0,0 +1,17 @@
> +* 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 string ("snps,ufshcd-1.0", "snps,ufshcd-1.1"
> +  or "snps,ufshcd-2.0")
> +- reg               : <registers mapping>
> +- interrupts        : <interrupt mapping for UFS host controller IRQ>
> +
> +Example:
> +	dwc_ufshcd@0xD0000000 {

Please fix the node name and address in the example. I think you want "ufs@d0000000".

> +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_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.

I don't think I understood you explanation why this has to be here,
rather than using a proper PHY driver. Please try again.

> +
> +#ifdef CONFIG_PM
> +/**
> + * ufs_dw_pci_suspend - suspend power management function
> + * @pdev: pointer to PCI device handle
> + * @state: power state
> + *
> + * Returns 0 if successful
> + * Returns non-zero otherwise
> + */
> +static int ufs_dw_pci_suspend(struct device *dev)
> +{
> +	return ufshcd_system_suspend(dev_get_drvdata(dev));
> +}

Please remove the #ifdef here.

> +
> +static const struct dev_pm_ops ufs_dw_pci_pm_ops = {
> +	.suspend	= ufs_dw_pci_suspend,
> +	.resume		= ufs_dw_pci_resume,
> +	.runtime_suspend = ufs_dw_pci_runtime_suspend,
> +	.runtime_resume  = ufs_dw_pci_runtime_resume,
> +	.runtime_idle    = ufs_dw_pci_runtime_idle,
> +};

Instead, use the macros from include/linux/pm.h

> +#ifdef CONFIG_SCSI_UFS_DWC_40BIT_RMMI
> +/**
> + * ufshcd_dwc_setup_40bit_rmmi()
> + * This function configures Synopsys MPHY specific atributes (40-bit RMMI)
> + * @hba: Pointer to drivers structure
> + *
> + * Returns 0 on success or non-zero value on failure
> + */
> +static int ufshcd_dwc_setup_40bit_rmmi(struct ufs_hba *hba)
> +{
> +	int ret = 0;

This looks like it should go into the external driver

> +
> +#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
> +	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

In particular, the part above cannot possibly work: When a distro ships a kernel
with CONFIG_SCSI_UFS_DWC_40BIT_RMMI set, it won't ever call the 
ufshcd_dwc_setup_20bit_rmmi() function, regardless of what the hardware is.

	Arnd