| Message ID | 20200902144344.16684-2-pali@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: aardvark: Fix comphy with old ATF | expand |
On Wed, Sep 02, 2020 at 04:43:43PM +0200, Pali Rohár wrote: > Driver ->power_on and ->power_off callbacks leaks internal SMCC firmware > return codes to phy caller. This patch converts SMCC error codes to > standard linux errno codes. Include file linux/arm-smccc.h already provides > defines for SMCC error codes, so use them instead of custom driver defines. > Note that return value is signed 32bit, but stored in unsigned long type > with zero padding. > > Signed-off-by: Pali Rohár <pali@kernel.org> > --- > drivers/phy/marvell/phy-mvebu-a3700-comphy.c | 14 +++++++++++--- > drivers/phy/marvell/phy-mvebu-cp110-comphy.c | 14 +++++++++++--- > 2 files changed, 22 insertions(+), 6 deletions(-) > > diff --git a/drivers/phy/marvell/phy-mvebu-a3700-comphy.c b/drivers/phy/marvell/phy-mvebu-a3700-comphy.c > index 1a138be8bd6a..810f25a47632 100644 > --- a/drivers/phy/marvell/phy-mvebu-a3700-comphy.c > +++ b/drivers/phy/marvell/phy-mvebu-a3700-comphy.c > @@ -26,7 +26,6 @@ > #define COMPHY_SIP_POWER_ON 0x82000001 > #define COMPHY_SIP_POWER_OFF 0x82000002 > #define COMPHY_SIP_PLL_LOCK 0x82000003 > -#define COMPHY_FW_NOT_SUPPORTED (-1) > > #define COMPHY_FW_MODE_SATA 0x1 > #define COMPHY_FW_MODE_SGMII 0x2 > @@ -112,10 +111,19 @@ static int mvebu_a3700_comphy_smc(unsigned long function, unsigned long lane, > unsigned long mode) > { > struct arm_smccc_res res; > + s32 ret; > > arm_smccc_smc(function, lane, mode, 0, 0, 0, 0, 0, &res); > + ret = res.a0; > > - return res.a0; > + switch (ret) { > + case SMCCC_RET_SUCCESS: > + return 0; > + case SMCCC_RET_NOT_SUPPORTED: > + return -EOPNOTSUPP; > + default: > + return -EINVAL; > + } > } Hi Pali Maybe this should be a global helper translating SMCCC_RET_* into a standard errno value? Andrew
On Wednesday 02 September 2020 18:13:28 Andrew Lunn wrote: > On Wed, Sep 02, 2020 at 04:43:43PM +0200, Pali Rohár wrote: > > Driver ->power_on and ->power_off callbacks leaks internal SMCC firmware > > return codes to phy caller. This patch converts SMCC error codes to > > standard linux errno codes. Include file linux/arm-smccc.h already provides > > defines for SMCC error codes, so use them instead of custom driver defines. > > Note that return value is signed 32bit, but stored in unsigned long type > > with zero padding. > > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > --- > > drivers/phy/marvell/phy-mvebu-a3700-comphy.c | 14 +++++++++++--- > > drivers/phy/marvell/phy-mvebu-cp110-comphy.c | 14 +++++++++++--- > > 2 files changed, 22 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/phy/marvell/phy-mvebu-a3700-comphy.c b/drivers/phy/marvell/phy-mvebu-a3700-comphy.c > > index 1a138be8bd6a..810f25a47632 100644 > > --- a/drivers/phy/marvell/phy-mvebu-a3700-comphy.c > > +++ b/drivers/phy/marvell/phy-mvebu-a3700-comphy.c > > @@ -26,7 +26,6 @@ > > #define COMPHY_SIP_POWER_ON 0x82000001 > > #define COMPHY_SIP_POWER_OFF 0x82000002 > > #define COMPHY_SIP_PLL_LOCK 0x82000003 > > -#define COMPHY_FW_NOT_SUPPORTED (-1) > > > > #define COMPHY_FW_MODE_SATA 0x1 > > #define COMPHY_FW_MODE_SGMII 0x2 > > @@ -112,10 +111,19 @@ static int mvebu_a3700_comphy_smc(unsigned long function, unsigned long lane, > > unsigned long mode) > > { > > struct arm_smccc_res res; > > + s32 ret; > > > > arm_smccc_smc(function, lane, mode, 0, 0, 0, 0, 0, &res); > > + ret = res.a0; > > > > - return res.a0; > > > + switch (ret) { > > + case SMCCC_RET_SUCCESS: > > + return 0; > > + case SMCCC_RET_NOT_SUPPORTED: > > + return -EOPNOTSUPP; > > + default: > > + return -EINVAL; > > + } > > } > > Hi Pali > > Maybe this should be a global helper translating SMCCC_RET_* into a > standard errno value? > > Andrew Hello Andrew! Well, I'm not sure if some standard global helper is the correct way for marvell comphy handler. It returns 0 for success and -1 on error when handler is not supported. Generic SMCC helper would need to support also other values which are defined/used by SMCC. And I'm not sure if some helpers cannot define and use also non-standard codecs. I think that such generic helper would need larger discussion and aim of these patches is to fix PCIe support on Armada 3720 HW which is broken since mentioned commit when users use factory firmware. So patches could be easily backported to stable kernels. On Espressobin is kernel stored on uSD card, so updating it is simple and easy -- as opposite of updating ATF firmware which is stored in SPI.
> > > + switch (ret) { > > > + case SMCCC_RET_SUCCESS: > > > + return 0; > > > + case SMCCC_RET_NOT_SUPPORTED: > > > + return -EOPNOTSUPP; > > > + default: > > > + return -EINVAL; > > > + } > > > } > > > > Hi Pali > > > > Maybe this should be a global helper translating SMCCC_RET_* into a > > standard errno value? > > > > Andrew > > Hello Andrew! > > Well, I'm not sure if some standard global helper is the correct way for > marvell comphy handler. It returns 0 for success and -1 on error when > handler is not supported. No, i was meaning just switch (ret) { case SMCCC_RET_SUCCESS: return 0; case SMCCC_RET_NOT_SUPPORTED: return -EOPNOTSUPP; default: return -EINVAL; } There is nothing Marvell specific here. Andrew
On Wednesday 02 September 2020 19:00:10 Andrew Lunn wrote: > > > > + switch (ret) { > > > > + case SMCCC_RET_SUCCESS: > > > > + return 0; > > > > + case SMCCC_RET_NOT_SUPPORTED: > > > > + return -EOPNOTSUPP; > > > > + default: > > > > + return -EINVAL; > > > > + } > > > > } > > > > > > Hi Pali > > > > > > Maybe this should be a global helper translating SMCCC_RET_* into a > > > standard errno value? > > > > > > Andrew > > > > Hello Andrew! > > > > Well, I'm not sure if some standard global helper is the correct way for > > marvell comphy handler. It returns 0 for success and -1 on error when > > handler is not supported. > > No, i was meaning just > > switch (ret) { > case SMCCC_RET_SUCCESS: > return 0; > case SMCCC_RET_NOT_SUPPORTED: > return -EOPNOTSUPP; > default: > return -EINVAL; > } But this is not a complete generic helper. There are more generic SMCC return codes and generic helper should define and translate all of them.
On Wed, Sep 02, 2020 at 07:05:25PM +0200, Pali Rohár wrote: > On Wednesday 02 September 2020 19:00:10 Andrew Lunn wrote: > > > > > + switch (ret) { > > > > > + case SMCCC_RET_SUCCESS: > > > > > + return 0; > > > > > + case SMCCC_RET_NOT_SUPPORTED: > > > > > + return -EOPNOTSUPP; > > > > > + default: > > > > > + return -EINVAL; > > > > > + } > > > > > } > > > > > > > > Hi Pali > > > > > > > > Maybe this should be a global helper translating SMCCC_RET_* into a > > > > standard errno value? > > > > > > > > Andrew > > > > > > Hello Andrew! > > > > > > Well, I'm not sure if some standard global helper is the correct way for > > > marvell comphy handler. It returns 0 for success and -1 on error when > > > handler is not supported. > > > > No, i was meaning just > > > > switch (ret) { > > case SMCCC_RET_SUCCESS: > > return 0; > > case SMCCC_RET_NOT_SUPPORTED: > > return -EOPNOTSUPP; > > default: > > return -EINVAL; > > } > > But this is not a complete generic helper. There are more generic SMCC > return codes and generic helper should define and translate all of them. /* * Return codes defined in ARM DEN 0070A * ARM DEN 0070A is now merged/consolidated into ARM DEN 0028 C */ #define SMCCC_RET_SUCCESS 0 #define SMCCC_RET_NOT_SUPPORTED -1 #define SMCCC_RET_NOT_REQUIRED -2 #define SMCCC_RET_INVALID_PARAMETER -3 I only see problems with SMCCC_RET_NOT_REQUIRED and what value to use for it. Do you have any idea what is actually means? A parameter was passed which was not required? Or that the call itself is not required? Looking at the uses of it currently in the kernel, it does not seem to be an actual error. So maybe just return 0? Andrew
On Wednesday 02 September 2020 19:20:29 Andrew Lunn wrote: > On Wed, Sep 02, 2020 at 07:05:25PM +0200, Pali Rohár wrote: > > On Wednesday 02 September 2020 19:00:10 Andrew Lunn wrote: > > > > > > + switch (ret) { > > > > > > + case SMCCC_RET_SUCCESS: > > > > > > + return 0; > > > > > > + case SMCCC_RET_NOT_SUPPORTED: > > > > > > + return -EOPNOTSUPP; > > > > > > + default: > > > > > > + return -EINVAL; > > > > > > + } > > > > > > } > > > > > > > > > > Hi Pali > > > > > > > > > > Maybe this should be a global helper translating SMCCC_RET_* into a > > > > > standard errno value? > > > > > > > > > > Andrew > > > > > > > > Hello Andrew! > > > > > > > > Well, I'm not sure if some standard global helper is the correct way for > > > > marvell comphy handler. It returns 0 for success and -1 on error when > > > > handler is not supported. > > > > > > No, i was meaning just > > > > > > switch (ret) { > > > case SMCCC_RET_SUCCESS: > > > return 0; > > > case SMCCC_RET_NOT_SUPPORTED: > > > return -EOPNOTSUPP; > > > default: > > > return -EINVAL; > > > } > > > > But this is not a complete generic helper. There are more generic SMCC > > return codes and generic helper should define and translate all of them. > > /* > * Return codes defined in ARM DEN 0070A > * ARM DEN 0070A is now merged/consolidated into ARM DEN 0028 C > */ > #define SMCCC_RET_SUCCESS 0 > #define SMCCC_RET_NOT_SUPPORTED -1 > #define SMCCC_RET_NOT_REQUIRED -2 > #define SMCCC_RET_INVALID_PARAMETER -3 Routines can use also other custom return codes. These are IIRC just standard defined. > I only see problems with SMCCC_RET_NOT_REQUIRED and what value to use > for it. Do you have any idea what is actually means? A parameter was > passed which was not required? Or that the call itself is not > required? Looking at the uses of it currently in the kernel, it does > not seem to be an actual error. So maybe just return 0? I'm not sure. That is why I wrote that larger discussion about generic helper is needed. There are for sure people who understand SMC better and have deep insight. For Marvell comphy we cannot use return code -2 as success like 0.
On Wed, 02 Sep 2020 16:43:43 +0200, Pali Rohár wrote: > Driver ->power_on and ->power_off callbacks leaks internal SMCC firmware > return codes to phy caller. This patch converts SMCC error codes to > standard linux errno codes. Include file linux/arm-smccc.h already provides > defines for SMCC error codes, so use them instead of custom driver defines. > Note that return value is signed 32bit, but stored in unsigned long type > with zero padding. > > Signed-off-by: Pali Rohár <pali@kernel.org> > --- > drivers/phy/marvell/phy-mvebu-a3700-comphy.c | 14 +++++++++++--- > drivers/phy/marvell/phy-mvebu-cp110-comphy.c | 14 +++++++++++--- > 2 files changed, 22 insertions(+), 6 deletions(-) > Reviewed-by: Rob Herring <robh@kernel.org>
diff --git a/drivers/phy/marvell/phy-mvebu-a3700-comphy.c b/drivers/phy/marvell/phy-mvebu-a3700-comphy.c index 1a138be8bd6a..810f25a47632 100644 --- a/drivers/phy/marvell/phy-mvebu-a3700-comphy.c +++ b/drivers/phy/marvell/phy-mvebu-a3700-comphy.c @@ -26,7 +26,6 @@ #define COMPHY_SIP_POWER_ON 0x82000001 #define COMPHY_SIP_POWER_OFF 0x82000002 #define COMPHY_SIP_PLL_LOCK 0x82000003 -#define COMPHY_FW_NOT_SUPPORTED (-1) #define COMPHY_FW_MODE_SATA 0x1 #define COMPHY_FW_MODE_SGMII 0x2 @@ -112,10 +111,19 @@ static int mvebu_a3700_comphy_smc(unsigned long function, unsigned long lane, unsigned long mode) { struct arm_smccc_res res; + s32 ret; arm_smccc_smc(function, lane, mode, 0, 0, 0, 0, 0, &res); + ret = res.a0; - return res.a0; + switch (ret) { + case SMCCC_RET_SUCCESS: + return 0; + case SMCCC_RET_NOT_SUPPORTED: + return -EOPNOTSUPP; + default: + return -EINVAL; + } } static int mvebu_a3700_comphy_get_fw_mode(int lane, int port, @@ -220,7 +228,7 @@ static int mvebu_a3700_comphy_power_on(struct phy *phy) } ret = mvebu_a3700_comphy_smc(COMPHY_SIP_POWER_ON, lane->id, fw_param); - if (ret == COMPHY_FW_NOT_SUPPORTED) + if (ret == -EOPNOTSUPP) dev_err(lane->dev, "unsupported SMC call, try updating your firmware\n"); diff --git a/drivers/phy/marvell/phy-mvebu-cp110-comphy.c b/drivers/phy/marvell/phy-mvebu-cp110-comphy.c index e41367f36ee1..53ad127b100f 100644 --- a/drivers/phy/marvell/phy-mvebu-cp110-comphy.c +++ b/drivers/phy/marvell/phy-mvebu-cp110-comphy.c @@ -123,7 +123,6 @@ #define COMPHY_SIP_POWER_ON 0x82000001 #define COMPHY_SIP_POWER_OFF 0x82000002 -#define COMPHY_FW_NOT_SUPPORTED (-1) /* * A lane is described by the following bitfields: @@ -273,10 +272,19 @@ static int mvebu_comphy_smc(unsigned long function, unsigned long phys, unsigned long lane, unsigned long mode) { struct arm_smccc_res res; + s32 ret; arm_smccc_smc(function, phys, lane, mode, 0, 0, 0, 0, &res); + ret = res.a0; - return res.a0; + switch (ret) { + case SMCCC_RET_SUCCESS: + return 0; + case SMCCC_RET_NOT_SUPPORTED: + return -EOPNOTSUPP; + default: + return -EINVAL; + } } static int mvebu_comphy_get_mode(bool fw_mode, int lane, int port, @@ -819,7 +827,7 @@ static int mvebu_comphy_power_on(struct phy *phy) if (!ret) return ret; - if (ret == COMPHY_FW_NOT_SUPPORTED) + if (ret == -EOPNOTSUPP) dev_err(priv->dev, "unsupported SMC call, try updating your firmware\n");
Driver ->power_on and ->power_off callbacks leaks internal SMCC firmware return codes to phy caller. This patch converts SMCC error codes to standard linux errno codes. Include file linux/arm-smccc.h already provides defines for SMCC error codes, so use them instead of custom driver defines. Note that return value is signed 32bit, but stored in unsigned long type with zero padding. Signed-off-by: Pali Rohár <pali@kernel.org> --- drivers/phy/marvell/phy-mvebu-a3700-comphy.c | 14 +++++++++++--- drivers/phy/marvell/phy-mvebu-cp110-comphy.c | 14 +++++++++++--- 2 files changed, 22 insertions(+), 6 deletions(-)