All of lore.kernel.org
 help / color / mirror / Atom feed
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 16:39:10 +0100	[thread overview]
Message-ID: <2087303.0ORDEBS3hn@wuerfel> (raw)
In-Reply-To: <56B2164E.5060007@synopsys.com>

On Wednesday 03 February 2016 15:01:34 Joao Pinto wrote:
> 
> Hi Arnd,
> 
> On 2/3/2016 12:54 PM, Arnd Bergmann wrote:
> > 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
> 
> This controller recent releases was 2.0, but we released last year 1.1. The
> driver works with both. The driver must work with all DWC UFS versions.

Ok, then make the driver match on the "snps,ufshcd-1.1" compatible
string, but document both strings in the binding document, and make
it mandatory to specify the 1.1 version as a compatible fallback.

If we ever need to handle a quirk for the 2.0 version then, it can
easily be done.

> >> +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
> 
> We could do that, but imagine that we select SCSI_UFS_QCOM, then the synopsys
> hooks would be selected also which in my opinion is not very accurate.
> In my opinion we should have a selectable DWC_HOOKS.

I don't understand. At the moment, you can enable SCSI_UFS_DWC_HOOKS
even if nothing uses it and you only have SCSI_UFS_QCOM enabled.

With my suggestion, the hooks would disappear unless they are
actually used.

Then again, with my later comments, we no longer need the hooks.


> >> +/**
> >> + * 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.
> 
> Could you please point an example for me to check?

drivers/phy/phy-qcom-ufs-qmp-14nm.c is a phy driver, and it gets used through
the generic devm_phy_get()/phy_power_on()/phy_power_off()/... interfaces.

This should probably be moved into the generic UFS platform driver so the PHY
handling can be shared between all backends.

> >>  };
> > 
> > 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.
> 
> I have a branch with that approach, but honestly it would be a big change in the
> UFS arch for the pci and I decided to make it simple. I sent that suggestion for
> the scsi mailing list and the comments showed me that. Does anyone have anything
> against putting ufshcd-pci.c as a pci common code and then have a ufs-dwc-pci.c
> and a ufs-samsung-pci.c that uses that common code?

Another approach would be to just rename the existing file to ufs-samsung-pci.c
and start the  ufs-dwc-pci.c as a copy of that. The file is not really all that
large anyway.

	Arnd

  reply	other threads:[~2016-02-03 15:39 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
2016-02-03 15:01     ` Joao Pinto
2016-02-03 15:01       ` Joao Pinto
2016-02-03 15:39       ` Arnd Bergmann [this message]
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=2087303.0ORDEBS3hn@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.