From: Wei WANG <[email protected]>
In some platforms, specially Thinkpad series, rts5249 won't be
initialized properly. So we need adjust some phy parameters to
improve the compatibility issue.
Signed-off-by: Wei WANG <[email protected]>
---
drivers/mfd/rts5249.c | 48 ++++++++++++++++++++++++++++++++++++--
include/linux/mfd/rtsx_pci.h | 53 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 99 insertions(+), 2 deletions(-)
diff --git a/drivers/mfd/rts5249.c b/drivers/mfd/rts5249.c
index 3b835f5..573de7b 100644
--- a/drivers/mfd/rts5249.c
+++ b/drivers/mfd/rts5249.c
@@ -130,13 +130,57 @@ static int rts5249_optimize_phy(struct rtsx_pcr *pcr)
{
int err;
- err = rtsx_pci_write_phy_register(pcr, PHY_REG_REV, 0xFE46);
+ err = rtsx_pci_write_phy_register(pcr, PHY_REG_REV,
+ PHY_REG_REV_RESV | PHY_REG_REV_RXIDLE_LATCHED |
+ PHY_REG_REV_P1_EN | PHY_REG_REV_RXIDLE_EN |
+ PHY_REG_REV_RX_PWST | PHY_REG_REV_CLKREQ_DLY_TIMER_1_0 |
+ PHY_REG_REV_STOP_CLKRD | PHY_REG_REV_STOP_CLKWR);
if (err < 0)
return err;
msleep(1);
- return rtsx_pci_write_phy_register(pcr, PHY_BPCR, 0x05C0);
+ err = rtsx_pci_write_phy_register(pcr, PHY_BPCR,
+ PHY_BPCR_IBRXSEL | PHY_BPCR_IBTXSEL |
+ PHY_BPCR_IB_FILTER | PHY_BPCR_CMIRROR_EN);
+ if (err < 0)
+ return err;
+ err = rtsx_pci_write_phy_register(pcr, PHY_PCR,
+ PHY_PCR_FORCE_CODE | PHY_PCR_OOBS_CALI_50 |
+ PHY_PCR_OOBS_VCM_08 | PHY_PCR_OOBS_SEN_90 |
+ PHY_PCR_RSSI_EN);
+ if (err < 0)
+ return err;
+ err = rtsx_pci_write_phy_register(pcr, PHY_RCR2,
+ PHY_RCR2_EMPHASE_EN | PHY_RCR2_NADJR |
+ PHY_RCR2_CDR_CP_10 | PHY_RCR2_CDR_SR_2 |
+ PHY_RCR2_FREQSEL_12 | PHY_RCR2_CPADJEN |
+ PHY_RCR2_CDR_SC_8 | PHY_RCR2_CALIB_LATE);
+ if (err < 0)
+ return err;
+ err = rtsx_pci_write_phy_register(pcr, PHY_FLD4,
+ PHY_FLD4_FLDEN_SEL | PHY_FLD4_REQ_REF |
+ PHY_FLD4_RXAMP_OFF | PHY_FLD4_REQ_ADDA |
+ PHY_FLD4_BER_COUNT | PHY_FLD4_BER_TIMER |
+ PHY_FLD4_BER_CHK_EN);
+ if (err < 0)
+ return err;
+ err = rtsx_pci_write_phy_register(pcr, PHY_RDR, PHY_RDR_RXDSEL_1_9);
+ if (err < 0)
+ return err;
+ err = rtsx_pci_write_phy_register(pcr, PHY_RCR1,
+ PHY_RCR1_ADP_TIME | PHY_RCR1_VCO_COARSE);
+ if (err < 0)
+ return err;
+ err = rtsx_pci_write_phy_register(pcr, PHY_FLD3,
+ PHY_FLD3_TIMER_4 | PHY_FLD3_TIMER_6 |
+ PHY_FLD3_RXDELINK);
+ if (err < 0)
+ return err;
+ return rtsx_pci_write_phy_register(pcr, PHY_TUNE,
+ PHY_TUNE_TUNEREF_1_0 | PHY_TUNE_VBGSEL_1252 |
+ PHY_TUNE_SDBUS_33 | PHY_TUNE_TUNED18 |
+ PHY_TUNE_TUNED12);
}
static int rts5249_turn_on_led(struct rtsx_pcr *pcr)
diff --git a/include/linux/mfd/rtsx_pci.h b/include/linux/mfd/rtsx_pci.h
index d1382df..0ce7721 100644
--- a/include/linux/mfd/rtsx_pci.h
+++ b/include/linux/mfd/rtsx_pci.h
@@ -756,6 +756,59 @@
#define PCR_SETTING_REG2 0x814
#define PCR_SETTING_REG3 0x747
+/* Phy bits */
+#define PHY_PCR_FORCE_CODE 0xB000
+#define PHY_PCR_OOBS_CALI_50 0x0800
+#define PHY_PCR_OOBS_VCM_08 0x0200
+#define PHY_PCR_OOBS_SEN_90 0x0040
+#define PHY_PCR_RSSI_EN 0x0002
+
+#define PHY_RCR1_ADP_TIME 0x0100
+#define PHY_RCR1_VCO_COARSE 0x001F
+
+#define PHY_RCR2_EMPHASE_EN 0x8000
+#define PHY_RCR2_NADJR 0x4000
+#define PHY_RCR2_CDR_CP_10 0x0400
+#define PHY_RCR2_CDR_SR_2 0x0100
+#define PHY_RCR2_FREQSEL_12 0x0040
+#define PHY_RCR2_CPADJEN 0x0020
+#define PHY_RCR2_CDR_SC_8 0x0008
+#define PHY_RCR2_CALIB_LATE 0x0002
+
+#define PHY_RDR_RXDSEL_1_9 0x4000
+
+#define PHY_TUNE_TUNEREF_1_0 0x4000
+#define PHY_TUNE_VBGSEL_1252 0x0C00
+#define PHY_TUNE_SDBUS_33 0x0200
+#define PHY_TUNE_TUNED18 0x01C0
+#define PHY_TUNE_TUNED12 0X0020
+
+#define PHY_BPCR_IBRXSEL 0x0400
+#define PHY_BPCR_IBTXSEL 0x0100
+#define PHY_BPCR_IB_FILTER 0x0080
+#define PHY_BPCR_CMIRROR_EN 0x0040
+
+#define PHY_REG_REV_RESV 0xE000
+#define PHY_REG_REV_RXIDLE_LATCHED 0x1000
+#define PHY_REG_REV_P1_EN 0x0800
+#define PHY_REG_REV_RXIDLE_EN 0x0400
+#define PHY_REG_REV_CLKREQ_DLY_TIMER_1_0 0x0040
+#define PHY_REG_REV_STOP_CLKRD 0x0020
+#define PHY_REG_REV_RX_PWST 0x0008
+#define PHY_REG_REV_STOP_CLKWR 0x0004
+
+#define PHY_FLD3_TIMER_4 0x7800
+#define PHY_FLD3_TIMER_6 0x00E0
+#define PHY_FLD3_RXDELINK 0x0004
+
+#define PHY_FLD4_FLDEN_SEL 0x4000
+#define PHY_FLD4_REQ_REF 0x2000
+#define PHY_FLD4_RXAMP_OFF 0x1000
+#define PHY_FLD4_REQ_ADDA 0x0800
+#define PHY_FLD4_BER_COUNT 0x00E0
+#define PHY_FLD4_BER_TIMER 0x000A
+#define PHY_FLD4_BER_CHK_EN 0x0001
+
#define rtsx_pci_init_cmd(pcr) ((pcr)->ci = 0)
struct rtsx_pcr;
--
1.7.9.5
> From: Wei WANG <[email protected]>
>
> In some platforms, specially Thinkpad series, rts5249 won't be
> initialized properly. So we need adjust some phy parameters to
> improve the compatibility issue.
The code looks so much more readable now, thanks for that.
I would like some more information in the commit log though. You're
making a lot of configuration changes here and due to the
incomprehensible 'magic numbers' used previously, it's impossible to
know what you're changing by just reading the code.
Why won't the rts**** be initialise properly and what exactly are you
changing to rectify the situation?
> Signed-off-by: Wei WANG <[email protected]>
> ---
> drivers/mfd/rts5249.c | 48 ++++++++++++++++++++++++++++++++++++--
> include/linux/mfd/rtsx_pci.h | 53 ++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 99 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mfd/rts5249.c b/drivers/mfd/rts5249.c
> index 3b835f5..573de7b 100644
> --- a/drivers/mfd/rts5249.c
> +++ b/drivers/mfd/rts5249.c
> @@ -130,13 +130,57 @@ static int rts5249_optimize_phy(struct rtsx_pcr *pcr)
> {
> int err;
>
> - err = rtsx_pci_write_phy_register(pcr, PHY_REG_REV, 0xFE46);
> + err = rtsx_pci_write_phy_register(pcr, PHY_REG_REV,
> + PHY_REG_REV_RESV | PHY_REG_REV_RXIDLE_LATCHED |
> + PHY_REG_REV_P1_EN | PHY_REG_REV_RXIDLE_EN |
> + PHY_REG_REV_RX_PWST | PHY_REG_REV_CLKREQ_DLY_TIMER_1_0 |
> + PHY_REG_REV_STOP_CLKRD | PHY_REG_REV_STOP_CLKWR);
> if (err < 0)
> return err;
>
> msleep(1);
>
> - return rtsx_pci_write_phy_register(pcr, PHY_BPCR, 0x05C0);
> + err = rtsx_pci_write_phy_register(pcr, PHY_BPCR,
> + PHY_BPCR_IBRXSEL | PHY_BPCR_IBTXSEL |
> + PHY_BPCR_IB_FILTER | PHY_BPCR_CMIRROR_EN);
> + if (err < 0)
> + return err;
> + err = rtsx_pci_write_phy_register(pcr, PHY_PCR,
> + PHY_PCR_FORCE_CODE | PHY_PCR_OOBS_CALI_50 |
> + PHY_PCR_OOBS_VCM_08 | PHY_PCR_OOBS_SEN_90 |
> + PHY_PCR_RSSI_EN);
> + if (err < 0)
> + return err;
> + err = rtsx_pci_write_phy_register(pcr, PHY_RCR2,
> + PHY_RCR2_EMPHASE_EN | PHY_RCR2_NADJR |
> + PHY_RCR2_CDR_CP_10 | PHY_RCR2_CDR_SR_2 |
> + PHY_RCR2_FREQSEL_12 | PHY_RCR2_CPADJEN |
> + PHY_RCR2_CDR_SC_8 | PHY_RCR2_CALIB_LATE);
> + if (err < 0)
> + return err;
> + err = rtsx_pci_write_phy_register(pcr, PHY_FLD4,
> + PHY_FLD4_FLDEN_SEL | PHY_FLD4_REQ_REF |
> + PHY_FLD4_RXAMP_OFF | PHY_FLD4_REQ_ADDA |
> + PHY_FLD4_BER_COUNT | PHY_FLD4_BER_TIMER |
> + PHY_FLD4_BER_CHK_EN);
> + if (err < 0)
> + return err;
> + err = rtsx_pci_write_phy_register(pcr, PHY_RDR, PHY_RDR_RXDSEL_1_9);
> + if (err < 0)
> + return err;
> + err = rtsx_pci_write_phy_register(pcr, PHY_RCR1,
> + PHY_RCR1_ADP_TIME | PHY_RCR1_VCO_COARSE);
> + if (err < 0)
> + return err;
> + err = rtsx_pci_write_phy_register(pcr, PHY_FLD3,
> + PHY_FLD3_TIMER_4 | PHY_FLD3_TIMER_6 |
> + PHY_FLD3_RXDELINK);
> + if (err < 0)
> + return err;
> + return rtsx_pci_write_phy_register(pcr, PHY_TUNE,
> + PHY_TUNE_TUNEREF_1_0 | PHY_TUNE_VBGSEL_1252 |
> + PHY_TUNE_SDBUS_33 | PHY_TUNE_TUNED18 |
> + PHY_TUNE_TUNED12);
> }
>
> static int rts5249_turn_on_led(struct rtsx_pcr *pcr)
> diff --git a/include/linux/mfd/rtsx_pci.h b/include/linux/mfd/rtsx_pci.h
> index d1382df..0ce7721 100644
> --- a/include/linux/mfd/rtsx_pci.h
> +++ b/include/linux/mfd/rtsx_pci.h
> @@ -756,6 +756,59 @@
> #define PCR_SETTING_REG2 0x814
> #define PCR_SETTING_REG3 0x747
>
> +/* Phy bits */
> +#define PHY_PCR_FORCE_CODE 0xB000
> +#define PHY_PCR_OOBS_CALI_50 0x0800
> +#define PHY_PCR_OOBS_VCM_08 0x0200
> +#define PHY_PCR_OOBS_SEN_90 0x0040
> +#define PHY_PCR_RSSI_EN 0x0002
> +
> +#define PHY_RCR1_ADP_TIME 0x0100
> +#define PHY_RCR1_VCO_COARSE 0x001F
> +
> +#define PHY_RCR2_EMPHASE_EN 0x8000
> +#define PHY_RCR2_NADJR 0x4000
> +#define PHY_RCR2_CDR_CP_10 0x0400
> +#define PHY_RCR2_CDR_SR_2 0x0100
> +#define PHY_RCR2_FREQSEL_12 0x0040
> +#define PHY_RCR2_CPADJEN 0x0020
> +#define PHY_RCR2_CDR_SC_8 0x0008
> +#define PHY_RCR2_CALIB_LATE 0x0002
> +
> +#define PHY_RDR_RXDSEL_1_9 0x4000
> +
> +#define PHY_TUNE_TUNEREF_1_0 0x4000
> +#define PHY_TUNE_VBGSEL_1252 0x0C00
> +#define PHY_TUNE_SDBUS_33 0x0200
> +#define PHY_TUNE_TUNED18 0x01C0
> +#define PHY_TUNE_TUNED12 0X0020
> +
> +#define PHY_BPCR_IBRXSEL 0x0400
> +#define PHY_BPCR_IBTXSEL 0x0100
> +#define PHY_BPCR_IB_FILTER 0x0080
> +#define PHY_BPCR_CMIRROR_EN 0x0040
> +
> +#define PHY_REG_REV_RESV 0xE000
> +#define PHY_REG_REV_RXIDLE_LATCHED 0x1000
> +#define PHY_REG_REV_P1_EN 0x0800
> +#define PHY_REG_REV_RXIDLE_EN 0x0400
> +#define PHY_REG_REV_CLKREQ_DLY_TIMER_1_0 0x0040
> +#define PHY_REG_REV_STOP_CLKRD 0x0020
> +#define PHY_REG_REV_RX_PWST 0x0008
> +#define PHY_REG_REV_STOP_CLKWR 0x0004
> +
> +#define PHY_FLD3_TIMER_4 0x7800
> +#define PHY_FLD3_TIMER_6 0x00E0
> +#define PHY_FLD3_RXDELINK 0x0004
> +
> +#define PHY_FLD4_FLDEN_SEL 0x4000
> +#define PHY_FLD4_REQ_REF 0x2000
> +#define PHY_FLD4_RXAMP_OFF 0x1000
> +#define PHY_FLD4_REQ_ADDA 0x0800
> +#define PHY_FLD4_BER_COUNT 0x00E0
> +#define PHY_FLD4_BER_TIMER 0x000A
> +#define PHY_FLD4_BER_CHK_EN 0x0001
> +
> #define rtsx_pci_init_cmd(pcr) ((pcr)->ci = 0)
>
> struct rtsx_pcr;
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
于 2013年09月10日 17:28, Lee Jones 写道:
> I would like some more information in the commit log though. You're
> making a lot of configuration changes here and due to the
> incomprehensible 'magic numbers' used previously, it's impossible to
> know what you're changing by just reading the code.
>
> Why won't the rts**** be initialise properly and what exactly are you
> changing to rectify the situation?
Hi Lee:
It's a little difficult to describe it very clearly. To put it simply,
the default setting of rts5249 is not good, and it will cause the signal
quality very bad. So we have to change those values to achieve a better
signal quality.
Do I need amend the commit and add the above description and resend it ?
BR,
Wei
On Tue, 10 Sep 2013, wwang wrote:
> 于 2013年09月10日 17:28, Lee Jones 写道:
> >I would like some more information in the commit log though. You're
> >making a lot of configuration changes here and due to the
> >incomprehensible 'magic numbers' used previously, it's impossible to
> >know what you're changing by just reading the code.
> >
> >Why won't the rts**** be initialise properly and what exactly are you
> >changing to rectify the situation?
>
> Hi Lee:
>
> It's a little difficult to describe it very clearly. To put it
> simply, the default setting of rts5249 is not good, and it will
> cause the signal quality very bad. So we have to change those values
> to achieve a better signal quality.
>
> Do I need amend the commit and add the above description and resend it ?
I'm not asking for in-depth analysis, just an overview.
What's wrong with the default config?
Why is the signal quality bad and what makes it bad?
What did the old magic numbers do?
How will the configuration differ if I applied your patch?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
于 2013年09月10日 19:08, Lee Jones 写道:
> I'm not asking for in-depth analysis, just an overview.
>
> What's wrong with the default config?
> Why is the signal quality bad and what makes it bad?
> What did the old magic numbers do?
> How will the configuration differ if I applied your patch?
It is a little different between simulation and real chip. We have no idea about which configuration is better before tape-out. We set default settings according to simulation, but need to tune these parameters after getting the real chip.
Those old magic numbers are proper in simulation environment, but not good in real ASIC chip.
If the new patch won't be applied, we may failed to access SD card in those platforms equipped with rts5249 module.
> >I'm not asking for in-depth analysis, just an overview.
> >
> >What's wrong with the default config?
> >Why is the signal quality bad and what makes it bad?
> >What did the old magic numbers do?
> >How will the configuration differ if I applied your patch?
I'm not sure I'm getting the answers I crave here.
> It is a little different between simulation and real chip. We have
> no idea about which configuration is better before tape-out. We set
> default settings according to simulation, but need to tune these
> parameters after getting the real chip.
What parameters? I can see they've changed, but what were they before?
> Those old magic numbers are proper in simulation environment, but
> not good in real ASIC chip.
I'm sure they suited at some point or they wouldn't have been used,
but what's the difference between the old magic numbers and the new
defines? 0xFE46 tells me nothing. I can't make a comparison without
you telling me, in words, in the commit log what you're actually
doing.
> If the new patch won't be applied, we may failed to access SD card
> in those platforms equipped with rts5249 module.
Fine. I get that. I have no problem applying the patch, but I'd really
like to know what it is that you're changing before blindly doing so.
Other changes which deserve a little explaining:
Why don't you set the PHY_BPCR anymore? 0x05C0 looks like quite a bit
of configuration. What was it and why isn't it required anymore?
And why is there a need to set the; PHY_PCR, PHY_RCR2, PHY_FLD4,
PHY_RDR, PHY_RCR1, PHY_FLD3 and PHY_TUNE registers where there
wasn't this requirement before?
You don't need to reply to this message. Write a proper, informative
commit log into the patch and resubmit. If I'm satisfied I know what
you're actually adapting in the configuration I'll have no issue and
apply the patch - the code looks good.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog