Hi,
2017-04-10 17:50 GMT+02:00 Ard Biesheuvel ard.biesheuvel@linaro.org:
On 10 April 2017 at 16:43, Leif Lindholm leif.lindholm@linaro.org wrote:
On Fri, Apr 07, 2017 at 05:58:41PM +0200, Marcin Wojtas wrote:
This patch adds analog parameters configuration for SATA with the values defined during electrical tests of the interface.
Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Marcin Wojtas mw@semihalf.com
Platforms/Marvell/Library/ComPhyLib/ComPhyCp110.c | 177 ++++++++++++++++++++- Platforms/Marvell/Library/ComPhyLib/ComPhyLib.h | 179 +++++++++++++++++++++- 2 files changed, 346 insertions(+), 10 deletions(-)
diff --git a/Platforms/Marvell/Library/ComPhyLib/ComPhyCp110.c b/Platforms/Marvell/Library/ComPhyLib/ComPhyCp110.c index ee3ce99..e3f6d45 100755 --- a/Platforms/Marvell/Library/ComPhyLib/ComPhyCp110.c +++ b/Platforms/Marvell/Library/ComPhyLib/ComPhyCp110.c @@ -608,9 +608,182 @@ ComPhySataPhyConfiguration ( STATIC VOID ComPhySataSetAnalogParameters (
- IN EFI_PHYSICAL_ADDRESS HpipeAddr
- IN EFI_PHYSICAL_ADDRESS HpipeAddr,
- IN EFI_PHYSICAL_ADDRESS SdIpAddr
) {
- UINT32 Data, Mask;
- /* G1 settings */
What's G1? No need to abbreviate in comments.
- Mask = HPIPE_G1_SET_1_G1_RX_SELMUPI_MASK;
- Data = 0x0 << HPIPE_G1_SET_1_G1_RX_SELMUPI_OFFSET;
- Mask |= HPIPE_G1_SET_1_G1_RX_SELMUPP_MASK;
- Data |= 0x1 << HPIPE_G1_SET_1_G1_RX_SELMUPP_OFFSET;
- Mask |= HPIPE_G1_SET_1_G1_RX_SELMUFI_MASK;
- Data |= 0x0 << HPIPE_G1_SET_1_G1_RX_SELMUFI_OFFSET;
- Mask |= HPIPE_G1_SET_1_G1_RX_SELMUFF_MASK;
- Data |= 0x3 << HPIPE_G1_SET_1_G1_RX_SELMUFF_OFFSET;
- Mask |= HPIPE_G1_SET_1_G1_RX_DIGCK_DIV_MASK;
- Data |= 0x1 << HPIPE_G1_SET_1_G1_RX_DIGCK_DIV_OFFSET;
- RegSet (HpipeAddr + HPIPE_G1_SET_1_REG, Data, Mask);
Obviously, this isn't really something I can review properly. Nevertheless, is there any way we can get rid of the last remaining magic numbers in the .c file? Those 0x0, 0x1 and 0x3 make no sense without already knowing the internal formats of those registers.
- Mask = HPIPE_G1_SETTINGS_3_G1_FFE_CAP_SEL_MASK;
- Data = 0xf << HPIPE_G1_SETTINGS_3_G1_FFE_CAP_SEL_OFFSET;
- Mask |= HPIPE_G1_SETTINGS_3_G1_FFE_RES_SEL_MASK;
- Data |= 0x2 << HPIPE_G1_SETTINGS_3_G1_FFE_RES_SEL_OFFSET;
- Mask |= HPIPE_G1_SETTINGS_3_G1_FFE_SETTING_FORCE_MASK;
- Data |= 0x1 << HPIPE_G1_SETTINGS_3_G1_FFE_SETTING_FORCE_OFFSET;
- Mask |= HPIPE_G1_SETTINGS_3_G1_FFE_DEG_RES_LEVEL_MASK;
- Data |= 0x1 << HPIPE_G1_SETTINGS_3_G1_FFE_DEG_RES_LEVEL_OFFSET;
- Mask |= HPIPE_G1_SETTINGS_3_G1_FFE_LOAD_RES_LEVEL_MASK;
- Data |= 0x1 << HPIPE_G1_SETTINGS_3_G1_FFE_LOAD_RES_LEVEL_OFFSET;
- RegSet (HpipeAddr + HPIPE_G1_SETTINGS_3_REG, Data, Mask);
- /* G2 settings */
- Mask = HPIPE_G2_SET_1_G2_RX_SELMUPI_MASK;
- Data = 0x0 << HPIPE_G2_SET_1_G2_RX_SELMUPI_OFFSET;
- Mask |= HPIPE_G2_SET_1_G2_RX_SELMUPP_MASK;
- Data |= 0x1 << HPIPE_G2_SET_1_G2_RX_SELMUPP_OFFSET;
- Mask |= HPIPE_G2_SET_1_G2_RX_SELMUFI_MASK;
- Data |= 0x0 << HPIPE_G2_SET_1_G2_RX_SELMUFI_OFFSET;
- Mask |= HPIPE_G2_SET_1_G2_RX_SELMUFF_MASK;
- Data |= 0x3 << HPIPE_G2_SET_1_G2_RX_SELMUFF_OFFSET;
- Mask |= HPIPE_G2_SET_1_G2_RX_DIGCK_DIV_MASK;
- Data |= 0x1 << HPIPE_G2_SET_1_G2_RX_DIGCK_DIV_OFFSET;
- RegSet (HpipeAddr + HPIPE_G2_SET_1_REG, Data, Mask);
- /* G3 settings */
- Mask = HPIPE_G3_SET_1_G3_RX_SELMUPI_MASK;
- Data = 0x2 << HPIPE_G3_SET_1_G3_RX_SELMUPI_OFFSET;
- Mask |= HPIPE_G3_SET_1_G3_RX_SELMUPF_MASK;
- Data |= 0x2 << HPIPE_G3_SET_1_G3_RX_SELMUPF_OFFSET;
- Mask |= HPIPE_G3_SET_1_G3_RX_SELMUFI_MASK;
- Data |= 0x3 << HPIPE_G3_SET_1_G3_RX_SELMUFI_OFFSET;
- Mask |= HPIPE_G3_SET_1_G3_RX_SELMUFF_MASK;
- Data |= 0x3 << HPIPE_G3_SET_1_G3_RX_SELMUFF_OFFSET;
- Mask |= HPIPE_G3_SET_1_G3_RX_DFE_EN_MASK;
- Data |= 0x1 << HPIPE_G3_SET_1_G3_RX_DFE_EN_OFFSET;
- Mask |= HPIPE_G3_SET_1_G3_RX_DIGCK_DIV_MASK;
- Data |= 0x2 << HPIPE_G3_SET_1_G3_RX_DIGCK_DIV_OFFSET;
- Mask |= HPIPE_G3_SET_1_G3_SAMPLER_INPAIRX2_EN_MASK;
- Data |= 0x0 << HPIPE_G3_SET_1_G3_SAMPLER_INPAIRX2_EN_OFFSET;
- RegSet (HpipeAddr + HPIPE_G3_SET_1_REG, Data, Mask);
- /* DTL Control */
- Mask = HPIPE_PWR_CTR_DTL_SQ_DET_EN_MASK;
- Data = 0x1 << HPIPE_PWR_CTR_DTL_SQ_DET_EN_OFFSET;
- Mask |= HPIPE_PWR_CTR_DTL_SQ_PLOOP_EN_MASK;
- Data |= 0x1 << HPIPE_PWR_CTR_DTL_SQ_PLOOP_EN_OFFSET;
- Mask |= HPIPE_PWR_CTR_DTL_FLOOP_EN_MASK;
- Data |= 0x1 << HPIPE_PWR_CTR_DTL_FLOOP_EN_OFFSET;
- Mask |= HPIPE_PWR_CTR_DTL_CLAMPING_SEL_MASK;
- Data |= 0x1 << HPIPE_PWR_CTR_DTL_CLAMPING_SEL_OFFSET;
- Mask |= HPIPE_PWR_CTR_DTL_INTPCLK_DIV_FORCE_MASK;
- Data |= 0x1 << HPIPE_PWR_CTR_DTL_INTPCLK_DIV_FORCE_OFFSET;
- Mask |= HPIPE_PWR_CTR_DTL_CLK_MODE_MASK;
- Data |= 0x1 << HPIPE_PWR_CTR_DTL_CLK_MODE_OFFSET;
- Mask |= HPIPE_PWR_CTR_DTL_CLK_MODE_FORCE_MASK;
- Data |= 0x1 << HPIPE_PWR_CTR_DTL_CLK_MODE_FORCE_OFFSET;
- RegSet (HpipeAddr + HPIPE_PWR_CTR_DTL_REG, Data, Mask);
- /* Trigger sampler enable pulse (by toggleing the bit) */
- Mask = HPIPE_SAMPLER_MASK;
- Data = 0x1 << HPIPE_SAMPLER_OFFSET;
- RegSet (HpipeAddr + HPIPE_SAMPLER_N_PROC_CALIB_CTRL_REG, Data, Mask);
- Mask = HPIPE_SAMPLER_MASK;
- Data = 0x0 << HPIPE_SAMPLER_OFFSET;
- RegSet (HpipeAddr + HPIPE_SAMPLER_N_PROC_CALIB_CTRL_REG, Data, Mask);
- /* VDD Calibration Control 3 */
- Mask = HPIPE_EXT_SELLV_RXSAMPL_MASK;
- Data = 0x10 << HPIPE_EXT_SELLV_RXSAMPL_OFFSET;
- RegSet (HpipeAddr + HPIPE_VDD_CAL_CTRL_REG, Data, Mask);
- /* DFE Resolution Control */
- Mask = HPIPE_DFE_RES_FORCE_MASK;
- Data = 0x1 << HPIPE_DFE_RES_FORCE_OFFSET;
- RegSet (HpipeAddr + HPIPE_DFE_REG0, Data, Mask);
- /* DFE F3-F5 Coefficient Control */
- Mask = HPIPE_DFE_F3_F5_DFE_EN_MASK;
- Data = 0x0 << HPIPE_DFE_F3_F5_DFE_EN_OFFSET;
- Mask |= HPIPE_DFE_F3_F5_DFE_CTRL_MASK;
- Data = 0x0 << HPIPE_DFE_F3_F5_DFE_CTRL_OFFSET;
- RegSet (HpipeAddr + HPIPE_DFE_F3_F5_REG, Data, Mask);
- /* G3 Setting 3 */
- Mask = HPIPE_G3_FFE_CAP_SEL_MASK;
- Data = 0xf << HPIPE_G3_FFE_CAP_SEL_OFFSET;
- Mask |= HPIPE_G3_FFE_RES_SEL_MASK;
- Data |= 0x4 << HPIPE_G3_FFE_RES_SEL_OFFSET;
- Mask |= HPIPE_G3_FFE_SETTING_FORCE_MASK;
- Data |= 0x1 << HPIPE_G3_FFE_SETTING_FORCE_OFFSET;
- Mask |= HPIPE_G3_FFE_DEG_RES_LEVEL_MASK;
- Data |= 0x1 << HPIPE_G3_FFE_DEG_RES_LEVEL_OFFSET;
- Mask |= HPIPE_G3_FFE_LOAD_RES_LEVEL_MASK;
- Data |= 0x3 << HPIPE_G3_FFE_LOAD_RES_LEVEL_OFFSET;
- RegSet (HpipeAddr + HPIPE_G3_SETTING_3_REG, Data, Mask);
- /* G3 Setting 4 */
- Mask = HPIPE_G3_DFE_RES_MASK;
- Data = 0x2 << HPIPE_G3_DFE_RES_OFFSET;
- RegSet (HpipeAddr + HPIPE_G3_SETTING_4_REG, Data, Mask);
- /* Offset Phase Control */
- Mask = HPIPE_OS_PH_OFFSET_MASK;
- Data = 0x5c << HPIPE_OS_PH_OFFSET_OFFSET;
- Mask |= HPIPE_OS_PH_OFFSET_FORCE_MASK;
- Data |= 0x1 << HPIPE_OS_PH_OFFSET_FORCE_OFFSET;
- RegSet (HpipeAddr + HPIPE_PHASE_CONTROL_REG, Data, Mask);
- Mask = HPIPE_OS_PH_VALID_MASK;
- Data = 0x1 << HPIPE_OS_PH_VALID_OFFSET;
- RegSet (HpipeAddr + HPIPE_PHASE_CONTROL_REG, Data, Mask);
- Mask = HPIPE_OS_PH_VALID_MASK;
- Data = 0x0 << HPIPE_OS_PH_VALID_OFFSET;
- RegSet (HpipeAddr + HPIPE_PHASE_CONTROL_REG, Data, Mask);
- /* Set G1 TX amplitude and TX post emphasis value */
- Mask = HPIPE_G1_SET_0_G1_TX_AMP_MASK;
- Data = 0x8 << HPIPE_G1_SET_0_G1_TX_AMP_OFFSET;
- Mask |= HPIPE_G1_SET_0_G1_TX_AMP_ADJ_MASK;
- Data |= 0x1 << HPIPE_G1_SET_0_G1_TX_AMP_ADJ_OFFSET;
- Mask |= HPIPE_G1_SET_0_G1_TX_EMPH1_MASK;
- Data |= 0x1 << HPIPE_G1_SET_0_G1_TX_EMPH1_OFFSET;
- Mask |= HPIPE_G1_SET_0_G1_TX_EMPH1_EN_MASK;
- Data |= 0x1 << HPIPE_G1_SET_0_G1_TX_EMPH1_EN_OFFSET;
- RegSet (HpipeAddr + HPIPE_G1_SET_0_REG, Data, Mask);
- /* Set G2 TX amplitude and TX post emphasis value */
- Mask = HPIPE_G2_SET_0_G2_TX_AMP_MASK;
- Data = 0xa << HPIPE_G2_SET_0_G2_TX_AMP_OFFSET;
- Mask |= HPIPE_G2_SET_0_G2_TX_AMP_ADJ_MASK;
- Data |= 0x1 << HPIPE_G2_SET_0_G2_TX_AMP_ADJ_OFFSET;
- Mask |= HPIPE_G2_SET_0_G2_TX_EMPH1_MASK;
- Data |= 0x2 << HPIPE_G2_SET_0_G2_TX_EMPH1_OFFSET;
- Mask |= HPIPE_G2_SET_0_G2_TX_EMPH1_EN_MASK;
- Data |= 0x1 << HPIPE_G2_SET_0_G2_TX_EMPH1_EN_OFFSET;
- RegSet (HpipeAddr + HPIPE_G2_SET_0_REG, Data, Mask);
- /* Set G3 TX amplitude and TX post emphasis value */
- Mask = HPIPE_G3_SET_0_G3_TX_AMP_MASK;
- Data = 0xe << HPIPE_G3_SET_0_G3_TX_AMP_OFFSET;
- Mask |= HPIPE_G3_SET_0_G3_TX_AMP_ADJ_MASK;
- Data |= 0x1 << HPIPE_G3_SET_0_G3_TX_AMP_ADJ_OFFSET;
- Mask |= HPIPE_G3_SET_0_G3_TX_EMPH1_MASK;
- Data |= 0x6 << HPIPE_G3_SET_0_G3_TX_EMPH1_OFFSET;
- Mask |= HPIPE_G3_SET_0_G3_TX_EMPH1_EN_MASK;
- Data |= 0x1 << HPIPE_G3_SET_0_G3_TX_EMPH1_EN_OFFSET;
- Mask |= HPIPE_G3_SET_0_G3_TX_SLEW_RATE_SEL_MASK;
- Data |= 0x4 << HPIPE_G3_SET_0_G3_TX_SLEW_RATE_SEL_OFFSET;
- Mask |= HPIPE_G3_SET_0_G3_TX_SLEW_CTRL_EN_MASK;
- Data |= 0x0 << HPIPE_G3_SET_0_G3_TX_SLEW_CTRL_EN_OFFSET;
- RegSet (HpipeAddr + HPIPE_G3_SET_0_REG, Data, Mask);
- /* SERDES External Configuration 2 register */
- Mask = SD_EXTERNAL_CONFIG2_SSC_ENABLE_MASK;
- Data = 0x1 << SD_EXTERNAL_CONFIG2_SSC_ENABLE_OFFSET;
- RegSet (SdIpAddr + SD_EXTERNAL_CONFIG2_REG, Data, Mask);
There looks to be a spectacular amount of duplication, looking for example at the RX_DIGCK_DIV_OFFSET definitions. Is there any difference whatsoever between the internal structures of G1, G2 and G2? If not, please cut away the redundancy.
In general, the additions in this patch looks like a transposition of raw assembly into C. While this will never be a highly readable function, we should try to make it as readable as possible.
Indeed. Compare
Mask = HPIPE_G1_SET_1_G1_RX_SELMUPI_MASK; Data = 0x0 << HPIPE_G1_SET_1_G1_RX_SELMUPI_OFFSET; Mask |= HPIPE_G1_SET_1_G1_RX_SELMUPP_MASK; Data |= 0x1 << HPIPE_G1_SET_1_G1_RX_SELMUPP_OFFSET; Mask |= HPIPE_G1_SET_1_G1_RX_SELMUFI_MASK; Data |= 0x0 << HPIPE_G1_SET_1_G1_RX_SELMUFI_OFFSET; Mask |= HPIPE_G1_SET_1_G1_RX_SELMUFF_MASK; Data |= 0x3 << HPIPE_G1_SET_1_G1_RX_SELMUFF_OFFSET; Mask |= HPIPE_G1_SET_1_G1_RX_DIGCK_DIV_MASK; Data |= 0x1 << HPIPE_G1_SET_1_G1_RX_DIGCK_DIV_OFFSET; RegSet (HpipeAddr + HPIPE_G1_SET_1_REG, Data, Mask);
with
Mask = HPIPE_G1_SET_1_G1_RX_SELMUPI_MASK | HPIPE_G1_SET_1_G1_RX_SELMUPP_MASK | HPIPE_G1_SET_1_G1_RX_SELMUFI_MASK | HPIPE_G1_SET_1_G1_RX_SELMUFF_MASK | HPIPE_G1_SET_1_G1_RX_DIGCK_DIV_MASK;
Data = (0x0 << HPIPE_G1_SET_1_G1_RX_SELMUPI_OFFSET) | (0x1 << HPIPE_G1_SET_1_G1_RX_SELMUPP_OFFSET) | (0x0 << HPIPE_G1_SET_1_G1_RX_SELMUFI_OFFSET) | (0x3 << HPIPE_G1_SET_1_G1_RX_SELMUFF_OFFSET) | (0x1 << HPIPE_G1_SET_1_G1_RX_DIGCK_DIV_OFFSET);
RegSet (HpipeAddr + HPIPE_G1_SET_1_REG, Data, Mask);
I'll check if the code can be more optimised and also if by any chance I can replace magic numbers with the defines (I think it's not very well documented atm.).
Best regards, Marcin