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@xxxxxxxxxxxx>
> >
> > 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
