LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 1/1] scsi/ufs: qcom: Don't enable PHY_QCOM_UFS by default
@ 2018-04-09 13:24 Vivek Gautam
  2018-04-09 16:51 ` Bjorn Andersson
  0 siblings, 1 reply; 7+ messages in thread
From: Vivek Gautam @ 2018-04-09 13:24 UTC (permalink / raw)
  To: martin.petersen, kishon
  Cc: jejb, subhashj, linux-scsi, linux-kernel, linux-arm-msm, Vivek Gautam

While we try to transition from a separate ufs phy driver to a
more integrated qmp phy driver, don't let SCSI_UFS_QCOM to
enable PHY_QCOM_UFS config by default.
The users should enable this in their defconfigs.
Also add inline definitions for couple of functions -
a) ufs_qcom_phy_set_tx_lane_enable()
b) void ufs_qcom_phy_save_controller_version()
to enable clean build for SCSI_UFS_QCOM.

Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
---
 drivers/scsi/ufs/Kconfig         |  1 -
 include/linux/phy/phy-qcom-ufs.h | 15 ++++++++++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
index e27b4d4e6ae2..7e8898b6eb67 100644
--- a/drivers/scsi/ufs/Kconfig
+++ b/drivers/scsi/ufs/Kconfig
@@ -91,7 +91,6 @@ config SCSI_UFS_DWC_TC_PLATFORM
 config SCSI_UFS_QCOM
 	tristate "QCOM specific hooks to UFS controller platform driver"
 	depends on SCSI_UFSHCD_PLATFORM && ARCH_QCOM
-	select PHY_QCOM_UFS
 	help
 	  This selects the QCOM specific additions to UFSHCD platform driver.
 	  UFS host on QCOM needs some vendor specific configuration before
diff --git a/include/linux/phy/phy-qcom-ufs.h b/include/linux/phy/phy-qcom-ufs.h
index 0a2c18a9771d..1388c2a2965e 100644
--- a/include/linux/phy/phy-qcom-ufs.h
+++ b/include/linux/phy/phy-qcom-ufs.h
@@ -31,8 +31,21 @@ void ufs_qcom_phy_enable_dev_ref_clk(struct phy *phy);
  */
 void ufs_qcom_phy_disable_dev_ref_clk(struct phy *phy);
 
+#if IS_ENABLED(CONFIG_PHY_QCOM_UFS)
 int ufs_qcom_phy_set_tx_lane_enable(struct phy *phy, u32 tx_lanes);
 void ufs_qcom_phy_save_controller_version(struct phy *phy,
-			u8 major, u16 minor, u16 step);
+					  u8 major, u16 minor, u16 step);
+#else
+static inline int ufs_qcom_phy_set_tx_lane_enable(struct phy *phy, u32 tx_lanes)
+{
+	return -ENOSYS;
+}
+
+static inline void ufs_qcom_phy_save_controller_version(struct phy *phy,
+							u8 major, u16 minor,
+							u16 step)
+{
+}
+#endif /* PHY_QCOM_UFS */
 
 #endif /* PHY_QCOM_UFS_H_ */
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] scsi/ufs: qcom: Don't enable PHY_QCOM_UFS by default
  2018-04-09 13:24 [PATCH 1/1] scsi/ufs: qcom: Don't enable PHY_QCOM_UFS by default Vivek Gautam
@ 2018-04-09 16:51 ` Bjorn Andersson
  2018-04-09 17:38   ` Vivek Gautam
  0 siblings, 1 reply; 7+ messages in thread
From: Bjorn Andersson @ 2018-04-09 16:51 UTC (permalink / raw)
  To: Vivek Gautam
  Cc: martin.petersen, kishon, jejb, subhashj, linux-scsi,
	linux-kernel, linux-arm-msm

On Mon 09 Apr 06:24 PDT 2018, Vivek Gautam wrote:

> While we try to transition from a separate ufs phy driver to a
> more integrated qmp phy driver, don't let SCSI_UFS_QCOM to
> enable PHY_QCOM_UFS config by default.
> The users should enable this in their defconfigs.
> Also add inline definitions for couple of functions -
> a) ufs_qcom_phy_set_tx_lane_enable()
> b) void ufs_qcom_phy_save_controller_version()
> to enable clean build for SCSI_UFS_QCOM.
> 
> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> ---
>  drivers/scsi/ufs/Kconfig         |  1 -
>  include/linux/phy/phy-qcom-ufs.h | 15 ++++++++++++++-
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
> index e27b4d4e6ae2..7e8898b6eb67 100644
> --- a/drivers/scsi/ufs/Kconfig
> +++ b/drivers/scsi/ufs/Kconfig
> @@ -91,7 +91,6 @@ config SCSI_UFS_DWC_TC_PLATFORM
>  config SCSI_UFS_QCOM
>  	tristate "QCOM specific hooks to UFS controller platform driver"
>  	depends on SCSI_UFSHCD_PLATFORM && ARCH_QCOM
> -	select PHY_QCOM_UFS

As you're depending on function in the UFS phy, which can be compiled as
a module you might end up in a configuration where UFS_QCOM is builtin
and the PHY_QCOM_UFS is module, which will fail to link.

So you need to make this:

	depends on PHY_QCOM_UFS || PHY_QCOM_UFS=n

In which case we say that if PHY_QCOM_UFS is a module UFS_QCOM must be a
module and if PHY_QCOM_UFS is not compiled in UFS_QCOM can be either
builtin or a module.

>  	help
>  	  This selects the QCOM specific additions to UFSHCD platform driver.
>  	  UFS host on QCOM needs some vendor specific configuration before
> diff --git a/include/linux/phy/phy-qcom-ufs.h b/include/linux/phy/phy-qcom-ufs.h
> index 0a2c18a9771d..1388c2a2965e 100644
> --- a/include/linux/phy/phy-qcom-ufs.h
> +++ b/include/linux/phy/phy-qcom-ufs.h
> @@ -31,8 +31,21 @@ void ufs_qcom_phy_enable_dev_ref_clk(struct phy *phy);
>   */
>  void ufs_qcom_phy_disable_dev_ref_clk(struct phy *phy);
>  
> +#if IS_ENABLED(CONFIG_PHY_QCOM_UFS)
>  int ufs_qcom_phy_set_tx_lane_enable(struct phy *phy, u32 tx_lanes);
>  void ufs_qcom_phy_save_controller_version(struct phy *phy,
> -			u8 major, u16 minor, u16 step);
> +					  u8 major, u16 minor, u16 step);
> +#else
> +static inline int ufs_qcom_phy_set_tx_lane_enable(struct phy *phy, u32 tx_lanes)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline void ufs_qcom_phy_save_controller_version(struct phy *phy,
> +							u8 major, u16 minor,
> +							u16 step)
> +{
> +}
> +#endif /* PHY_QCOM_UFS */

What's the timeline for getting rid of the references to these
functions? I presume that code depending on these being here will
compile but won't actually work?

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] scsi/ufs: qcom: Don't enable PHY_QCOM_UFS by default
  2018-04-09 16:51 ` Bjorn Andersson
@ 2018-04-09 17:38   ` Vivek Gautam
  2018-04-09 20:09     ` Bjorn Andersson
  0 siblings, 1 reply; 7+ messages in thread
From: Vivek Gautam @ 2018-04-09 17:38 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: martin.petersen, kishon, jejb, subhashj, linux-scsi,
	linux-kernel, linux-arm-msm

Hi Bjorn,


On 4/9/2018 10:21 PM, Bjorn Andersson wrote:
> On Mon 09 Apr 06:24 PDT 2018, Vivek Gautam wrote:
>
>> While we try to transition from a separate ufs phy driver to a
>> more integrated qmp phy driver, don't let SCSI_UFS_QCOM to
>> enable PHY_QCOM_UFS config by default.
>> The users should enable this in their defconfigs.
>> Also add inline definitions for couple of functions -
>> a) ufs_qcom_phy_set_tx_lane_enable()
>> b) void ufs_qcom_phy_save_controller_version()
>> to enable clean build for SCSI_UFS_QCOM.
>>
>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
>> ---
>>   drivers/scsi/ufs/Kconfig         |  1 -
>>   include/linux/phy/phy-qcom-ufs.h | 15 ++++++++++++++-
>>   2 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
>> index e27b4d4e6ae2..7e8898b6eb67 100644
>> --- a/drivers/scsi/ufs/Kconfig
>> +++ b/drivers/scsi/ufs/Kconfig
>> @@ -91,7 +91,6 @@ config SCSI_UFS_DWC_TC_PLATFORM
>>   config SCSI_UFS_QCOM
>>   	tristate "QCOM specific hooks to UFS controller platform driver"
>>   	depends on SCSI_UFSHCD_PLATFORM && ARCH_QCOM
>> -	select PHY_QCOM_UFS
> As you're depending on function in the UFS phy, which can be compiled as
> a module you might end up in a configuration where UFS_QCOM is builtin
> and the PHY_QCOM_UFS is module, which will fail to link.
>
> So you need to make this:
>
> 	depends on PHY_QCOM_UFS || PHY_QCOM_UFS=n
>
> In which case we say that if PHY_QCOM_UFS is a module UFS_QCOM must be a
> module and if PHY_QCOM_UFS is not compiled in UFS_QCOM can be either
> builtin or a module.

Thanks for reviewing. You are right. I was trying to test all possible
cases for the PHY_QCOM_UFS, and SCSI_UFS_QCOM configs, but clearly i missed
this case.
Will modify as suggested.

>>   	help
>>   	  This selects the QCOM specific additions to UFSHCD platform driver.
>>   	  UFS host on QCOM needs some vendor specific configuration before
>> diff --git a/include/linux/phy/phy-qcom-ufs.h b/include/linux/phy/phy-qcom-ufs.h
>> index 0a2c18a9771d..1388c2a2965e 100644
>> --- a/include/linux/phy/phy-qcom-ufs.h
>> +++ b/include/linux/phy/phy-qcom-ufs.h
>> @@ -31,8 +31,21 @@ void ufs_qcom_phy_enable_dev_ref_clk(struct phy *phy);
>>    */
>>   void ufs_qcom_phy_disable_dev_ref_clk(struct phy *phy);
>>   
>> +#if IS_ENABLED(CONFIG_PHY_QCOM_UFS)
>>   int ufs_qcom_phy_set_tx_lane_enable(struct phy *phy, u32 tx_lanes);
>>   void ufs_qcom_phy_save_controller_version(struct phy *phy,
>> -			u8 major, u16 minor, u16 step);
>> +					  u8 major, u16 minor, u16 step);
>> +#else
>> +static inline int ufs_qcom_phy_set_tx_lane_enable(struct phy *phy, u32 tx_lanes)
>> +{
>> +	return -ENOSYS;
>> +}
>> +
>> +static inline void ufs_qcom_phy_save_controller_version(struct phy *phy,
>> +							u8 major, u16 minor,
>> +							u16 step)
>> +{
>> +}
>> +#endif /* PHY_QCOM_UFS */
> What's the timeline for getting rid of the references to these
> functions? I presume that code depending on these being here will
> compile but won't actually work?

Yes, these inline definitions are just to keep ufs-qcom happy with the 
direct
calls that it makes to these functions.
As you would know these couple of functions are just used by the 20nm phy.
However, we don't have any platform yet in the upstream that enables 
this phy.
I am hoping that we will eventually get rid of these functions when we 
further
clean up ufs-qcom driver.

Best regards
Vivek
>
> Regards,
> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] scsi/ufs: qcom: Don't enable PHY_QCOM_UFS by default
  2018-04-09 17:38   ` Vivek Gautam
@ 2018-04-09 20:09     ` Bjorn Andersson
  2018-04-10  6:31       ` Vivek Gautam
  0 siblings, 1 reply; 7+ messages in thread
From: Bjorn Andersson @ 2018-04-09 20:09 UTC (permalink / raw)
  To: Vivek Gautam
  Cc: martin.petersen, kishon, jejb, subhashj, linux-scsi,
	linux-kernel, linux-arm-msm

On Mon 09 Apr 10:38 PDT 2018, Vivek Gautam wrote:
> On 4/9/2018 10:21 PM, Bjorn Andersson wrote:
> > On Mon 09 Apr 06:24 PDT 2018, Vivek Gautam wrote:
[..]
> > > diff --git a/include/linux/phy/phy-qcom-ufs.h b/include/linux/phy/phy-qcom-ufs.h
> > > index 0a2c18a9771d..1388c2a2965e 100644
> > > --- a/include/linux/phy/phy-qcom-ufs.h
> > > +++ b/include/linux/phy/phy-qcom-ufs.h
> > > @@ -31,8 +31,21 @@ void ufs_qcom_phy_enable_dev_ref_clk(struct phy *phy);
> > >    */
> > >   void ufs_qcom_phy_disable_dev_ref_clk(struct phy *phy);
> > > +#if IS_ENABLED(CONFIG_PHY_QCOM_UFS)
> > >   int ufs_qcom_phy_set_tx_lane_enable(struct phy *phy, u32 tx_lanes);
> > >   void ufs_qcom_phy_save_controller_version(struct phy *phy,
> > > -			u8 major, u16 minor, u16 step);
> > > +					  u8 major, u16 minor, u16 step);
> > > +#else
> > > +static inline int ufs_qcom_phy_set_tx_lane_enable(struct phy *phy, u32 tx_lanes)
> > > +{
> > > +	return -ENOSYS;
> > > +}
> > > +
> > > +static inline void ufs_qcom_phy_save_controller_version(struct phy *phy,
> > > +							u8 major, u16 minor,
> > > +							u16 step)
> > > +{
> > > +}
> > > +#endif /* PHY_QCOM_UFS */
> > What's the timeline for getting rid of the references to these
> > functions? I presume that code depending on these being here will
> > compile but won't actually work?
> 
> Yes, these inline definitions are just to keep ufs-qcom happy with the
> direct
> calls that it makes to these functions.
> As you would know these couple of functions are just used by the 20nm phy.
> However, we don't have any platform yet in the upstream that enables this
> phy.
> I am hoping that we will eventually get rid of these functions when we
> further
> clean up ufs-qcom driver.
> 

I see, but that means that we're calling this function with a struct phy
that might not be a struct ufs_qcom_phy and as such a defconfig with
both enabled will have undefined outcome for the migrated phys.

In particular we do expect that the same kernel will boot on db820c and
sdm845-mtp, so we will have to enable support for the 14nm & 20nm phy
driver (and we don't want random crashes because someone happened to
enable it).


So either we add the 10nm support to the existing driver or I think we
should migrate, at least, the 14nm support to the QMP driver (and mark
the remaining 20nm BROKEN for now).

Regardless of the path chosen this should be cleaned up to the point
where all three phys are supported.

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] scsi/ufs: qcom: Don't enable PHY_QCOM_UFS by default
  2018-04-09 20:09     ` Bjorn Andersson
@ 2018-04-10  6:31       ` Vivek Gautam
  2018-04-17 23:11         ` Bjorn Andersson
  0 siblings, 1 reply; 7+ messages in thread
From: Vivek Gautam @ 2018-04-10  6:31 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: martin.petersen, kishon, jejb, subhashj, linux-scsi,
	linux-kernel, linux-arm-msm



On 4/10/2018 1:39 AM, Bjorn Andersson wrote:
> On Mon 09 Apr 10:38 PDT 2018, Vivek Gautam wrote:
>> On 4/9/2018 10:21 PM, Bjorn Andersson wrote:
>>> On Mon 09 Apr 06:24 PDT 2018, Vivek Gautam wrote:
> [..]
>>>> diff --git a/include/linux/phy/phy-qcom-ufs.h b/include/linux/phy/phy-qcom-ufs.h
>>>> index 0a2c18a9771d..1388c2a2965e 100644
>>>> --- a/include/linux/phy/phy-qcom-ufs.h
>>>> +++ b/include/linux/phy/phy-qcom-ufs.h
>>>> @@ -31,8 +31,21 @@ void ufs_qcom_phy_enable_dev_ref_clk(struct phy *phy);
>>>>     */
>>>>    void ufs_qcom_phy_disable_dev_ref_clk(struct phy *phy);
>>>> +#if IS_ENABLED(CONFIG_PHY_QCOM_UFS)
>>>>    int ufs_qcom_phy_set_tx_lane_enable(struct phy *phy, u32 tx_lanes);
>>>>    void ufs_qcom_phy_save_controller_version(struct phy *phy,
>>>> -			u8 major, u16 minor, u16 step);
>>>> +					  u8 major, u16 minor, u16 step);
>>>> +#else
>>>> +static inline int ufs_qcom_phy_set_tx_lane_enable(struct phy *phy, u32 tx_lanes)
>>>> +{
>>>> +	return -ENOSYS;
>>>> +}
>>>> +
>>>> +static inline void ufs_qcom_phy_save_controller_version(struct phy *phy,
>>>> +							u8 major, u16 minor,
>>>> +							u16 step)
>>>> +{
>>>> +}
>>>> +#endif /* PHY_QCOM_UFS */
>>> What's the timeline for getting rid of the references to these
>>> functions? I presume that code depending on these being here will
>>> compile but won't actually work?
>> Yes, these inline definitions are just to keep ufs-qcom happy with the
>> direct
>> calls that it makes to these functions.
>> As you would know these couple of functions are just used by the 20nm phy.
>> However, we don't have any platform yet in the upstream that enables this
>> phy.
>> I am hoping that we will eventually get rid of these functions when we
>> further
>> clean up ufs-qcom driver.
>>
> I see, but that means that we're calling this function with a struct phy
> that might not be a struct ufs_qcom_phy and as such a defconfig with
> both enabled will have undefined outcome for the migrated phys.

No, we will have to add support for separate phys as sdm845 has phy per 
each lane,
and the older struct phy will exist alongside.
We will call this function only with the older phy pointer.

> In particular we do expect that the same kernel will boot on db820c and
> sdm845-mtp, so we will have to enable support for the 14nm & 20nm phy
> driver (and we don't want random crashes because someone happened to
> enable it).

Right, so we create new struct phy while keeping older one intact to 
keep the
ufs-qcom work with both - ufs_qcom_phy and qmp_phy.
Some of the controller drivers, such as usb/dwc3/ keep support for old 
and new phys,
although there the difference is between generic phy and the usb-phy.
So, I am assuming that if we want to keep ufs-qcom on platforms using 20nm,
14nm and 10nm phys happy, we will have to keep the phys separately for 
sometime.
What do you say about it?

On db820c, we can still work with the ufs_qcom_phy.

Regards
Vivek
>
>
> So either we add the 10nm support to the existing driver or I think we
> should migrate, at least, the 14nm support to the QMP driver (and mark
> the remaining 20nm BROKEN for now).
>
> Regardless of the path chosen this should be cleaned up to the point
> where all three phys are supported.
>
> Regards,
> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] scsi/ufs: qcom: Don't enable PHY_QCOM_UFS by default
  2018-04-10  6:31       ` Vivek Gautam
@ 2018-04-17 23:11         ` Bjorn Andersson
  2018-04-19 18:51           ` Vivek Gautam
  0 siblings, 1 reply; 7+ messages in thread
From: Bjorn Andersson @ 2018-04-17 23:11 UTC (permalink / raw)
  To: Vivek Gautam
  Cc: martin.petersen, kishon, jejb, subhashj, linux-scsi,
	linux-kernel, linux-arm-msm

On Mon 09 Apr 23:31 PDT 2018, Vivek Gautam wrote:

> 
> 
> On 4/10/2018 1:39 AM, Bjorn Andersson wrote:
> > On Mon 09 Apr 10:38 PDT 2018, Vivek Gautam wrote:
> > > On 4/9/2018 10:21 PM, Bjorn Andersson wrote:
> > > > On Mon 09 Apr 06:24 PDT 2018, Vivek Gautam wrote:
> > [..]
> > > > > diff --git a/include/linux/phy/phy-qcom-ufs.h b/include/linux/phy/phy-qcom-ufs.h
> > > > > index 0a2c18a9771d..1388c2a2965e 100644
> > > > > --- a/include/linux/phy/phy-qcom-ufs.h
> > > > > +++ b/include/linux/phy/phy-qcom-ufs.h
> > > > > @@ -31,8 +31,21 @@ void ufs_qcom_phy_enable_dev_ref_clk(struct phy *phy);
> > > > >     */
> > > > >    void ufs_qcom_phy_disable_dev_ref_clk(struct phy *phy);
> > > > > +#if IS_ENABLED(CONFIG_PHY_QCOM_UFS)
> > > > >    int ufs_qcom_phy_set_tx_lane_enable(struct phy *phy, u32 tx_lanes);
> > > > >    void ufs_qcom_phy_save_controller_version(struct phy *phy,
> > > > > -			u8 major, u16 minor, u16 step);
> > > > > +					  u8 major, u16 minor, u16 step);
> > > > > +#else
> > > > > +static inline int ufs_qcom_phy_set_tx_lane_enable(struct phy *phy, u32 tx_lanes)
> > > > > +{
> > > > > +	return -ENOSYS;
> > > > > +}
> > > > > +
> > > > > +static inline void ufs_qcom_phy_save_controller_version(struct phy *phy,
> > > > > +							u8 major, u16 minor,
> > > > > +							u16 step)
> > > > > +{
> > > > > +}
> > > > > +#endif /* PHY_QCOM_UFS */
> > > > What's the timeline for getting rid of the references to these
> > > > functions? I presume that code depending on these being here will
> > > > compile but won't actually work?
> > > Yes, these inline definitions are just to keep ufs-qcom happy with the
> > > direct
> > > calls that it makes to these functions.
> > > As you would know these couple of functions are just used by the 20nm phy.
> > > However, we don't have any platform yet in the upstream that enables this
> > > phy.
> > > I am hoping that we will eventually get rid of these functions when we
> > > further
> > > clean up ufs-qcom driver.
> > > 
> > I see, but that means that we're calling this function with a struct phy
> > that might not be a struct ufs_qcom_phy and as such a defconfig with
> > both enabled will have undefined outcome for the migrated phys.
> 
> No, we will have to add support for separate phys as sdm845 has phy per each
> lane,
> and the older struct phy will exist alongside.
> We will call this function only with the older phy pointer.
> 
> > In particular we do expect that the same kernel will boot on db820c and
> > sdm845-mtp, so we will have to enable support for the 14nm & 20nm phy
> > driver (and we don't want random crashes because someone happened to
> > enable it).
> 
> Right, so we create new struct phy while keeping older one intact to keep
> the
> ufs-qcom work with both - ufs_qcom_phy and qmp_phy.
> Some of the controller drivers, such as usb/dwc3/ keep support for old and
> new phys,
> although there the difference is between generic phy and the usb-phy.
> So, I am assuming that if we want to keep ufs-qcom on platforms using 20nm,
> 14nm and 10nm phys happy, we will have to keep the phys separately for
> sometime.
> What do you say about it?
> 

My concern is only that the UFS HCI driver doesn't have a way to know if
it's the new or old "type" of phy, but if you can get that working then
I don't have any objections about doing so for a transitional period.

But, you may not use kernel config options to handle this, the same
Image should boot on msm8916, msm8996 and sdm845 (with appropriate dtb
for each one).

> On db820c, we can still work with the ufs_qcom_phy.
> 

I do not have an issue with that.

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] scsi/ufs: qcom: Don't enable PHY_QCOM_UFS by default
  2018-04-17 23:11         ` Bjorn Andersson
@ 2018-04-19 18:51           ` Vivek Gautam
  0 siblings, 0 replies; 7+ messages in thread
From: Vivek Gautam @ 2018-04-19 18:51 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: martin.petersen, kishon, jejb, subhashj, linux-scsi,
	linux-kernel, linux-arm-msm, cang

Hi Bjorn,


On 4/18/2018 4:41 AM, Bjorn Andersson wrote:
> On Mon 09 Apr 23:31 PDT 2018, Vivek Gautam wrote:
>
>>
>> On 4/10/2018 1:39 AM, Bjorn Andersson wrote:
>>> On Mon 09 Apr 10:38 PDT 2018, Vivek Gautam wrote:
>>>> On 4/9/2018 10:21 PM, Bjorn Andersson wrote:
>>>>> On Mon 09 Apr 06:24 PDT 2018, Vivek Gautam wrote:
>>> [..]
>>>>>> diff --git a/include/linux/phy/phy-qcom-ufs.h b/include/linux/phy/phy-qcom-ufs.h
>>>>>> index 0a2c18a9771d..1388c2a2965e 100644
>>>>>> --- a/include/linux/phy/phy-qcom-ufs.h
>>>>>> +++ b/include/linux/phy/phy-qcom-ufs.h
>>>>>> @@ -31,8 +31,21 @@ void ufs_qcom_phy_enable_dev_ref_clk(struct phy *phy);
>>>>>>      */
>>>>>>     void ufs_qcom_phy_disable_dev_ref_clk(struct phy *phy);
>>>>>> +#if IS_ENABLED(CONFIG_PHY_QCOM_UFS)
>>>>>>     int ufs_qcom_phy_set_tx_lane_enable(struct phy *phy, u32 tx_lanes);
>>>>>>     void ufs_qcom_phy_save_controller_version(struct phy *phy,
>>>>>> -			u8 major, u16 minor, u16 step);
>>>>>> +					  u8 major, u16 minor, u16 step);
>>>>>> +#else
>>>>>> +static inline int ufs_qcom_phy_set_tx_lane_enable(struct phy *phy, u32 tx_lanes)
>>>>>> +{
>>>>>> +	return -ENOSYS;
>>>>>> +}
>>>>>> +
>>>>>> +static inline void ufs_qcom_phy_save_controller_version(struct phy *phy,
>>>>>> +							u8 major, u16 minor,
>>>>>> +							u16 step)
>>>>>> +{
>>>>>> +}
>>>>>> +#endif /* PHY_QCOM_UFS */
>>>>> What's the timeline for getting rid of the references to these
>>>>> functions? I presume that code depending on these being here will
>>>>> compile but won't actually work?
>>>> Yes, these inline definitions are just to keep ufs-qcom happy with the
>>>> direct
>>>> calls that it makes to these functions.
>>>> As you would know these couple of functions are just used by the 20nm phy.
>>>> However, we don't have any platform yet in the upstream that enables this
>>>> phy.
>>>> I am hoping that we will eventually get rid of these functions when we
>>>> further
>>>> clean up ufs-qcom driver.
>>>>
>>> I see, but that means that we're calling this function with a struct phy
>>> that might not be a struct ufs_qcom_phy and as such a defconfig with
>>> both enabled will have undefined outcome for the migrated phys.
>> No, we will have to add support for separate phys as sdm845 has phy per each
>> lane,
>> and the older struct phy will exist alongside.
>> We will call this function only with the older phy pointer.
>>
>>> In particular we do expect that the same kernel will boot on db820c and
>>> sdm845-mtp, so we will have to enable support for the 14nm & 20nm phy
>>> driver (and we don't want random crashes because someone happened to
>>> enable it).
>> Right, so we create new struct phy while keeping older one intact to keep
>> the
>> ufs-qcom work with both - ufs_qcom_phy and qmp_phy.
>> Some of the controller drivers, such as usb/dwc3/ keep support for old and
>> new phys,
>> although there the difference is between generic phy and the usb-phy.
>> So, I am assuming that if we want to keep ufs-qcom on platforms using 20nm,
>> 14nm and 10nm phys happy, we will have to keep the phys separately for
>> sometime.
>> What do you say about it?
>>
> My concern is only that the UFS HCI driver doesn't have a way to know if
> it's the new or old "type" of phy, but if you can get that working then
> I don't have any objections about doing so for a transitional period.
>
> But, you may not use kernel config options to handle this, the same
> Image should boot on msm8916, msm8996 and sdm845 (with appropriate dtb
> for each one).

Right, i get your concern. I will try to refactor the UFS HCI code to 
handle the two
'types' of phys.
I think Can Guo (CC'ed here) was already working on this. I will check 
with him
if he already has some code to do this.

Thanks
Vivek
>
>> On db820c, we can still work with the ufs_qcom_phy.
>>
> I do not have an issue with that.
>
> Regards,
> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-04-19 18:52 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-09 13:24 [PATCH 1/1] scsi/ufs: qcom: Don't enable PHY_QCOM_UFS by default Vivek Gautam
2018-04-09 16:51 ` Bjorn Andersson
2018-04-09 17:38   ` Vivek Gautam
2018-04-09 20:09     ` Bjorn Andersson
2018-04-10  6:31       ` Vivek Gautam
2018-04-17 23:11         ` Bjorn Andersson
2018-04-19 18:51           ` Vivek Gautam

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).