| Message ID | 20180409132404.20876-1-vivek.gautam@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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
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
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
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
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
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
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_ */
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(-)