>-----Original Message-----
>From: Mikko Perttunen
>Sent: Monday, November 28, 2016 6:02 PM
>To: Preetham Chandru; preetham260@xxxxxxxxx
>Cc: tj@xxxxxxxxxx; swarren@xxxxxxxxxxxxx; thierry.reding@xxxxxxxxx;
>Laxman Dewangan; linux-ide@xxxxxxxxxxxxxxx; Venu Byravarasu; Pavan
>Kunapuli; linux-tegra@xxxxxxxxxxxxxxx
>Subject: Re: [v2,1/3] ata: ahci_tegra: add support for tegra210
>
>+Cc linux-tegra
>
>Hi Preetham,
>
>I'll do a review pass. Please also Cc the next version to linux-
>tegra@xxxxxxxxxxxxxxx so that more Tegra developers have visibility.
>
Ok.
>On 24.11.2016 09:43, PREETHAM RAMACHANDRA wrote:
>> From: Preetham Chandru R <pchandru@xxxxxxxxxx>
>>
>> Add AHCI support for tegra210
>> 1. Moved tegra124 specifics to tegra124_ahci_init.
>> 2. Separated out the regulators needed for tegra124 and tegra210.
>> 3. Set the LPM capabilities
>> 4. Create inline functions for read/write and modify to
>> SATA, SATA Config and SATA Aux registers.
>>
>> Signed-off-by: Preetham Chandru R <pchandru@xxxxxxxxxx>
>> ---
>> v2:
>> * Fixed indentation issues
>> * Moved the change to disable DIPM, HIPM, DevSlp, partial,
>> slumber and NCQ into a separate patch
>>
>> drivers/ata/ahci_tegra.c | 478
>> ++++++++++++++++++++++++++++++++++-------------
>> 1 file changed, 348 insertions(+), 130 deletions(-)
>>
>> diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c index
>> 3a62eb2..d12e2a9 100644
>> --- a/drivers/ata/ahci_tegra.c
>> +++ b/drivers/ata/ahci_tegra.c
>> @@ -33,32 +33,74 @@
>>
>> #define DRV_NAME "tegra-ahci"
>>
>> +#define SATA_FPCI_BAR5_0 0x94
>> +#define FPCI_BAR5_START_MASK (0xFFFFFFF <<
>4)
>> +#define FPCI_BAR5_START (0x0040020
><< 4)
>> +#define FPCI_BAR5_ACCESS_TYPE (0x1)
>
>Please use the existing convention in this file, that is, fields are prefixed with
>the register name. Also, please use small letters for hex digits as used in the
>file elsewhere.
>
Ok.
>> +
>> #define SATA_CONFIGURATION_0 0x180
>> -#define SATA_CONFIGURATION_EN_FPCI BIT(0)
>> +#define SATA_CONFIGURATION_0_EN_FPCI BIT(0)
>> +#define SATA_CONFIGURATION_CLK_OVERRIDE BIT(31)
>> +
>> +#define SATA_INTR_MASK_0 0x188
>> +#define IP_INT_MASK BIT(16)
>>
>> #define SCFG_OFFSET 0x1000
>>
>> -#define T_SATA0_CFG_1 0x04
>> -#define T_SATA0_CFG_1_IO_SPACE BIT(0)
>> -#define T_SATA0_CFG_1_MEMORY_SPACE BIT(1)
>> -#define T_SATA0_CFG_1_BUS_MASTER BIT(2)
>> -#define T_SATA0_CFG_1_SERR BIT(8)
>> +#define T_SATA_CFG_1 0x4
>> +#define T_SATA_CFG_1_IO_SPACE BIT(0)
>> +#define T_SATA_CFG_1_MEMORY_SPACE BIT(1)
>> +#define T_SATA_CFG_1_BUS_MASTER BIT(2)
>> +#define T_SATA_CFG_1_SERR BIT(8)
>
>Try not to rename and move fields unnecessarily. It makes it very difficult to
>see where things have actually changed and makes the patch unnecessarily
>long.
>
Ok.
>> +
>> +#define T_SATA_CFG_9 0x24
>> +#define T_SATA_CFG_9_BASE_ADDRESS 0x40020000
>> +
>> +#define T_SATA0_CFG_35 0x94
>> +#define T_SATA0_CFG_35_IDP_INDEX_MASK (0x7FF
><< 2)
>> +#define T_SATA0_CFG_35_IDP_INDEX (0x2A << 2)
>>
>> -#define T_SATA0_CFG_9 0x24
>> -#define T_SATA0_CFG_9_BASE_ADDRESS_SHIFT 13
>> +#define T_SATA0_AHCI_IDP1 0x98
>> +#define T_SATA0_AHCI_IDP1_DATA
> (0x400040)
>>
>> -#define SATA_FPCI_BAR5 0x94
>> -#define SATA_FPCI_BAR5_START_SHIFT 4
>> +#define T_SATA0_CFG_PHY_1 0x12C
>> +#define T_SATA0_CFG_PHY_1_PADS_IDDQ_EN BIT(23)
>> +#define T_SATA0_CFG_PHY_1_PAD_PLL_IDDQ_EN BIT(22)
>>
>> -#define SATA_INTR_MASK 0x188
>> -#define SATA_INTR_MASK_IP_INT_MASK BIT(16)
>> +#define T_SATA0_NVOOB 0x114
>> +#define T_SATA0_NVOOB_COMMA_CNT_MASK (0xff << 16)
>> +#define T_SATA0_NVOOB_COMMA_CNT (0x07 << 16)
>> +#define T_SATA0_NVOOB_SQUELCH_FILTER_MODE_MASK (0x3 <<
>24)
>> +#define T_SATA0_NVOOB_SQUELCH_FILTER_MODE (0x1 << 24)
>> +#define T_SATA0_NVOOB_SQUELCH_FILTER_LENGTH_MASK (0x3 <<
>26)
>> +#define T_SATA0_NVOOB_SQUELCH_FILTER_LENGTH (0x3 << 26)
>> +
>> +#define T_SATA_CFG_PHY_0 0x120
>> +#define T_SATA_CFG_PHY_0_USE_7BIT_ALIGN_DET_FOR_SPD BIT(11)
>> +#define T_SATA_CFG_PHY_0_MASK_SQUELCH BIT(24)
>> +
>> +#define FUSE_SATA_CALIB 0x124
>> +#define FUSE_SATA_CALIB_MASK 0x3
>> +
>> +#define T_SATA0_CFG2NVOOB_2 0x134
>> +#define T_SATA0_CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW_MASK
> (0x1ff << 18)
>> +#define T_SATA0_CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW (0xc <<
>18)
>>
>> #define T_SATA0_AHCI_HBA_CAP_BKDR 0x300
>> +#define T_SATA0_AHCI_HBA_CAP_BKDR_PARTIAL_ST_CAP BIT(13)
>> +#define T_SATA0_AHCI_HBA_CAP_BKDR_SLUMBER_ST_CAP BIT(14)
>> +#define T_SATA0_AHCI_HBA_CAP_BKDR_SALP BIT(26)
>> +#define T_SATA0_AHCI_HBA_CAP_BKDR_SUPP_PM BIT(17)
>> +#define T_SATA0_AHCI_HBA_CAP_BKDR_SNCQ BIT(30)
>>
>> -#define T_SATA0_BKDOOR_CC 0x4a4
>> +#define T_SATA_BKDOOR_CC 0x4A4
>> +#define T_SATA_BKDOOR_CC_CLASS_CODE_MASK (0xFFFF << 16)
>> +#define T_SATA_BKDOOR_CC_CLASS_CODE
> (0x0106 << 16)
>> +#define T_SATA_BKDOOR_CC_PROG_IF_MASK (0xFF
><< 8)
>> +#define T_SATA_BKDOOR_CC_PROG_IF (0x01 << 8)
>>
>> -#define T_SATA0_CFG_SATA 0x54c
>> -#define T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN BIT(12)
>> +#define T_SATA_CFG_SATA 0x54C
>> +#define T_SATA_CFG_SATA_BACKDOOR_PROG_IF_EN BIT(12)
>>
>> #define T_SATA0_CFG_MISC 0x550
>>
>> @@ -82,8 +124,27 @@
>> #define T_SATA0_CHX_PHY_CTRL11 0x6d0
>> #define T_SATA0_CHX_PHY_CTRL11_GEN2_RX_EQ (0x2800 <<
>16)
>>
>> -#define FUSE_SATA_CALIB 0x124
>> -#define FUSE_SATA_CALIB_MASK 0x3
>> +/* Electrical settings for better link stability */
>> +#define T_SATA0_CHX_PHY_CTRL17_0 0x6e8
>> +#define T_SATA0_CHX_PHY_CTRL17_0_RX_EQ_CTRL_L_GEN1
> 0x55010000
>> +#define T_SATA0_CHX_PHY_CTRL18_0 0x6ec
>> +#define T_SATA0_CHX_PHY_CTRL18_0_RX_EQ_CTRL_L_GEN2
> 0x55010000
>> +#define T_SATA0_CHX_PHY_CTRL20_0 0x6f4
>> +#define T_SATA0_CHX_PHY_CTRL20_0_RX_EQ_CTRL_H_GEN1 0x1
>> +#define T_SATA0_CHX_PHY_CTRL21_0 0x6f8
>> +#define T_SATA0_CHX_PHY_CTRL21_0_RX_EQ_CTRL_H_GEN2 0x1
>> +
>> +/* AUX Registers */
>> +#define SATA_AUX_MISC_CNTL_1_0 0x8
>> +#define DEVSLP_OVERRIDE BIT(17)
>> +#define SDS_SUPPORT BIT(13)
>> +#define DESO_SUPPORT BIT(15)
>> +
>> +#define SATA_AUX_RX_STAT_INT_0 0xc
>> +#define SATA_DEVSLP BIT(7)
>> +
>> +#define SATA_AUX_SPARE_CFG0_0 0x18
>> +#define MDAT_TIMER_AFTER_PG_VALID BIT(14)
>>
>> struct sata_pad_calibration {
>> u8 gen1_tx_amp;
>> @@ -99,15 +160,135 @@ static const struct sata_pad_calibration
>tegra124_pad_calibration[] = {
>> {0x14, 0x0e, 0x1a, 0x0e},
>> };
>>
>> +struct tegra_ahci_ops {
>> + int (*init)(struct ahci_host_priv *); };
>> +
>> +struct tegra_ahci_soc {
>> + const char *const *supply_names;
>> + unsigned int num_supplies;
>> + struct tegra_ahci_ops ops;
>> +};
>> +
>> struct tegra_ahci_priv {
>> - struct platform_device *pdev;
>> - void __iomem *sata_regs;
>> - struct reset_control *sata_rst;
>> - struct reset_control *sata_oob_rst;
>> - struct reset_control *sata_cold_rst;
>> + struct platform_device *pdev;
>> + void __iomem *sata_regs;
>> + void __iomem *sata_aux_regs;
>> + struct reset_control *sata_rst;
>> + struct reset_control *sata_oob_rst;
>> + struct reset_control *sata_cold_rst;
>> /* Needs special handling, cannot use ahci_platform */
>> - struct clk *sata_clk;
>> - struct regulator_bulk_data supplies[5];
>> + struct clk *sata_clk;
>> + struct regulator_bulk_data *supplies;
>> + struct tegra_ahci_soc *soc_data;
>> +};
>
>I don't particularly care whether the field names are aligned or not, but I
>remember the ata maintainers wanting them to be. Anyway, changing the
>style makes the patch larger and the actual functionality changes harder to
>review.
>
Ok will keep them aligned.
>> +
>> +static const char *const tegra124_supply_names[] = {
>> + "avdd", "hvdd", "vddio", "target-5v", "target-12v"
>> +};
>> +
>> +static inline void tegra_ahci_sata_update(struct tegra_ahci_priv *tegra,
>> + u32 val, u32 mask, u32 offset) {
>> + u32 uval;
>> +
>> + uval = readl(tegra->sata_regs + offset);
>> + uval = (uval & ~mask) | (val & mask);
>> + writel(uval, tegra->sata_regs + offset); }
>
>I'm not very fond of these _update functions. I think it is clearer to just write
>it out at the callsite, especially since this is used only four times.
>
Ok will remove them.
>> +
>> +static inline void tegra_ahci_scfg_writel(struct tegra_ahci_priv *tegra,
>> + u32 val, u32 offset)
>> +{
>> + writel(val, tegra->sata_regs + SCFG_OFFSET + offset); }
>> +
>> +static inline void tegra_ahci_scfg_update(struct tegra_ahci_priv *tegra,
>> + u32 val, u32 mask, u32 offset) {
>> + u32 uval;
>> +
>> + uval = readl(tegra->sata_regs + SCFG_OFFSET + offset);
>> + uval = (uval & ~mask) | (val & mask);
>> + writel(uval, tegra->sata_regs + SCFG_OFFSET + offset); }
>> +
>> +static inline u32 tegra_ahci_aux_readl(struct tegra_ahci_priv *tegra,
>> + u32 offset)
>> +{
>> + return readl(tegra->sata_aux_regs + offset); }
>
>This is never called.
>
Will remove it.
>> +
>> +static inline void tegra_ahci_aux_update(struct tegra_ahci_priv *tegra,
>u32 val,
>> + u32 mask, u32 offset)
>> +{
>> + u32 uval;
>> +
>> + uval = readl(tegra->sata_aux_regs + offset);
>> + uval = (uval & ~mask) | (val & mask);
>> + writel(uval, tegra->sata_aux_regs + offset); }
>
>This is only called once.
>
Will remove it.
>> +
>> +static int tegra124_ahci_init(struct ahci_host_priv *hpriv) {
>> + struct tegra_ahci_priv *tegra = hpriv->plat_data;
>> + struct sata_pad_calibration calib;
>> + int ret;
>> + u32 val;
>> + u32 mask;
>> +
>> + /* Pad calibration */
>> + ret = tegra_fuse_readl(FUSE_SATA_CALIB, &val);
>> + if (ret)
>> + return ret;
>> +
>> + calib = tegra124_pad_calibration[val & FUSE_SATA_CALIB_MASK];
>> +
>> + tegra_ahci_scfg_writel(tegra, BIT(0), T_SATA0_INDEX);
>> +
>> + mask = T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_MASK |
>> + T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_MASK;
>> + val = (calib.gen1_tx_amp <<
>T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT) |
>> + (calib.gen1_tx_peak <<
>T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT);
>> + tegra_ahci_scfg_update(tegra, val, mask,
>> +T_SATA0_CHX_PHY_CTRL1_GEN1);
>> +
>> + mask = T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_MASK |
>> + T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_MASK;
>> + val = (calib.gen2_tx_amp <<
>T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT) |
>> + (calib.gen2_tx_peak <<
>T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT);
>> + tegra_ahci_scfg_update(tegra, val, mask,
>> +T_SATA0_CHX_PHY_CTRL1_GEN2);
>> +
>> + tegra_ahci_scfg_writel(tegra,
>> + T_SATA0_CHX_PHY_CTRL11_GEN2_RX_EQ,
>> + T_SATA0_CHX_PHY_CTRL11);
>> + tegra_ahci_scfg_writel(tegra,
>> + T_SATA0_CHX_PHY_CTRL2_CDR_CNTL_GEN1,
>> + T_SATA0_CHX_PHY_CTRL2);
>> +
>> + tegra_ahci_scfg_writel(tegra, 0, T_SATA0_INDEX);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct tegra_ahci_soc tegra124_ahci_soc_data = {
>> + .supply_names = tegra124_supply_names,
>> + .num_supplies = ARRAY_SIZE(tegra124_supply_names),
>> + .ops = {
>> + .init = tegra124_ahci_init,
>> + },
>> +};
>> +
>> +static const char *const tegra210_supply_names[] = {
>> + "dvdd-sata-pll",
>> + "hvdd-sata",
>> + "l0-hvddio-sata",
>> + "l0-dvddio-sata",
>> + "hvdd-pex-pll-e"
>> +};
>
>Perhaps the "-sata-" should be removed from these - I believe the supply
>name here should refer to the usage of the supplied power within the IP
>block. The IP block here is SATA so it is clear that it is used for something
>SATA related. If someone is better informed, please comment.
>
>It also looks like this doesn't include the 5V and 12V supplies which had to
>be enabled on the Jetson TK1 to enable power output through the Molex
>connector - do you know if the Jetson TX1 no longer needs similar to enable
>the SATA power connector?
>
We need to keep regulator name matching with pin name. The regulatory framework and policy suggests that regulator name should match the pin name. So I have named it accordingly.
The 5V and 12V does not seem to be required for tx1. I will take a closer look at it and if required I will push another patch for it.
>> +
>> +static const struct tegra_ahci_soc tegra210_ahci_soc_data = {
>> + .supply_names = tegra210_supply_names,
>> + .num_supplies = ARRAY_SIZE(tegra210_supply_names),
>> };
>>
>> static int tegra_ahci_power_on(struct ahci_host_priv *hpriv) @@
>> -115,7 +296,7 @@ static int tegra_ahci_power_on(struct ahci_host_priv
>*hpriv)
>> struct tegra_ahci_priv *tegra = hpriv->plat_data;
>> int ret;
>>
>> - ret = regulator_bulk_enable(ARRAY_SIZE(tegra->supplies),
>> + ret = regulator_bulk_enable(tegra->soc_data->num_supplies,
>> tegra->supplies);
>> if (ret)
>> return ret;
>> @@ -144,8 +325,7 @@ static int tegra_ahci_power_on(struct
>ahci_host_priv *hpriv)
>> tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
>>
>> disable_regulators:
>> - regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra-
>>supplies);
>> -
>> + regulator_bulk_disable(tegra->soc_data->num_supplies,
>> +tegra->supplies);
>> return ret;
>> }
>>
>> @@ -162,101 +342,124 @@ static void tegra_ahci_power_off(struct
>ahci_host_priv *hpriv)
>> clk_disable_unprepare(tegra->sata_clk);
>> tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
>>
>> - regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra-
>>supplies);
>> + regulator_bulk_disable(tegra->soc_data->num_supplies,
>> +tegra->supplies);
>> }
>>
>> static int tegra_ahci_controller_init(struct ahci_host_priv *hpriv)
>> {
>> struct tegra_ahci_priv *tegra = hpriv->plat_data;
>> int ret;
>> - unsigned int val;
>> - struct sata_pad_calibration calib;
>> + u32 val;
>> + u32 mask;
>>
>> ret = tegra_ahci_power_on(hpriv);
>> - if (ret) {
>> - dev_err(&tegra->pdev->dev,
>> - "failed to power on AHCI controller: %d\n", ret);
>> - return ret;
>> - }
>> -
>> - val = readl(tegra->sata_regs + SATA_CONFIGURATION_0);
>> - val |= SATA_CONFIGURATION_EN_FPCI;
>> - writel(val, tegra->sata_regs + SATA_CONFIGURATION_0);
>> -
>> - /* Pad calibration */
>> -
>> - ret = tegra_fuse_readl(FUSE_SATA_CALIB, &val);
>> - if (ret) {
>> - dev_err(&tegra->pdev->dev,
>> - "failed to read calibration fuse: %d\n", ret);
>> + if (ret)
>> return ret;
>> - }
>> -
>> - calib = tegra124_pad_calibration[val & FUSE_SATA_CALIB_MASK];
>> -
>> - writel(BIT(0), tegra->sata_regs + SCFG_OFFSET + T_SATA0_INDEX);
>> -
>> - val = readl(tegra->sata_regs +
>> - SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL1_GEN1);
>> - val &= ~T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_MASK;
>> - val &= ~T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_MASK;
>> - val |= calib.gen1_tx_amp <<
>> - T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT;
>> - val |= calib.gen1_tx_peak <<
>> - T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT;
>> - writel(val, tegra->sata_regs + SCFG_OFFSET +
>> - T_SATA0_CHX_PHY_CTRL1_GEN1);
>> -
>> - val = readl(tegra->sata_regs +
>> - SCFG_OFFSET + T_SATA0_CHX_PHY_CTRL1_GEN2);
>> - val &= ~T_SATA0_CHX_PHY_CTRL1_GEN2_TX_AMP_MASK;
>> - val &= ~T_SATA0_CHX_PHY_CTRL1_GEN2_TX_PEAK_MASK;
>> - val |= calib.gen2_tx_amp <<
>> - T_SATA0_CHX_PHY_CTRL1_GEN1_TX_AMP_SHIFT;
>> - val |= calib.gen2_tx_peak <<
>> - T_SATA0_CHX_PHY_CTRL1_GEN1_TX_PEAK_SHIFT;
>> - writel(val, tegra->sata_regs + SCFG_OFFSET +
>> - T_SATA0_CHX_PHY_CTRL1_GEN2);
>> -
>> - writel(T_SATA0_CHX_PHY_CTRL11_GEN2_RX_EQ,
>> - tegra->sata_regs + SCFG_OFFSET +
>T_SATA0_CHX_PHY_CTRL11);
>> - writel(T_SATA0_CHX_PHY_CTRL2_CDR_CNTL_GEN1,
>> - tegra->sata_regs + SCFG_OFFSET +
>T_SATA0_CHX_PHY_CTRL2);
>> -
>> - writel(0, tegra->sata_regs + SCFG_OFFSET + T_SATA0_INDEX);
>> -
>> - /* Program controller device ID */
>>
>> - val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
>> - val |= T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN;
>> - writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
>> -
>> - writel(0x01060100, tegra->sata_regs + SCFG_OFFSET +
>T_SATA0_BKDOOR_CC);
>> -
>> - val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
>> - val &= ~T_SATA0_CFG_SATA_BACKDOOR_PROG_IF_EN;
>> - writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_SATA);
>> -
>> - /* Enable IO & memory access, bus master mode */
>> -
>> - val = readl(tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_1);
>> - val |= T_SATA0_CFG_1_IO_SPACE |
>T_SATA0_CFG_1_MEMORY_SPACE |
>> - T_SATA0_CFG_1_BUS_MASTER | T_SATA0_CFG_1_SERR;
>> - writel(val, tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_1);
>> -
>> - /* Program SATA MMIO */
>> -
>> - writel(0x10000 << SATA_FPCI_BAR5_START_SHIFT,
>> - tegra->sata_regs + SATA_FPCI_BAR5);
>> -
>> - writel(0x08000 << T_SATA0_CFG_9_BASE_ADDRESS_SHIFT,
>> - tegra->sata_regs + SCFG_OFFSET + T_SATA0_CFG_9);
>> -
>> - /* Unmask SATA interrupts */
>> -
>> - val = readl(tegra->sata_regs + SATA_INTR_MASK);
>> - val |= SATA_INTR_MASK_IP_INT_MASK;
>> - writel(val, tegra->sata_regs + SATA_INTR_MASK);
>> + /*
>> + * Program the following SATA IPFS registers
>> + * to allow SW accesses to SATA's MMIO Register
>> + */
>> + mask = FPCI_BAR5_START_MASK | FPCI_BAR5_ACCESS_TYPE;
>> + val = FPCI_BAR5_START | FPCI_BAR5_ACCESS_TYPE;
>> + tegra_ahci_sata_update(tegra, val, mask, SATA_FPCI_BAR5_0);
>> +
>> + /* Program the following SATA IPFS register to enable the SATA */
>> + val = SATA_CONFIGURATION_0_EN_FPCI;
>> + tegra_ahci_sata_update(tegra, val, val, SATA_CONFIGURATION_0);
>> +
>> + /* Electrical settings for better link stability */
>> + tegra_ahci_scfg_writel(tegra,
>> +
>T_SATA0_CHX_PHY_CTRL17_0_RX_EQ_CTRL_L_GEN1,
>> + T_SATA0_CHX_PHY_CTRL17_0);
>> + tegra_ahci_scfg_writel(tegra,
>> +
>T_SATA0_CHX_PHY_CTRL18_0_RX_EQ_CTRL_L_GEN2,
>> + T_SATA0_CHX_PHY_CTRL18_0);
>> + tegra_ahci_scfg_writel(tegra,
>> +
>T_SATA0_CHX_PHY_CTRL20_0_RX_EQ_CTRL_H_GEN1,
>> + T_SATA0_CHX_PHY_CTRL20_0);
>> + tegra_ahci_scfg_writel(tegra,
>> +
>T_SATA0_CHX_PHY_CTRL21_0_RX_EQ_CTRL_H_GEN2,
>> + T_SATA0_CHX_PHY_CTRL21_0);
>> +
>> + /* For SQUELCH Filter & Gen3 drive getting detected as Gen1 drive
>*/
>> +
>> + mask = T_SATA_CFG_PHY_0_MASK_SQUELCH |
>> + T_SATA_CFG_PHY_0_USE_7BIT_ALIGN_DET_FOR_SPD;
>> + val = T_SATA_CFG_PHY_0_MASK_SQUELCH;
>> + val &= ~T_SATA_CFG_PHY_0_USE_7BIT_ALIGN_DET_FOR_SPD;
>> + tegra_ahci_scfg_update(tegra, val, mask, T_SATA_CFG_PHY_0);
>> +
>> + mask = (T_SATA0_NVOOB_COMMA_CNT_MASK |
>> + T_SATA0_NVOOB_SQUELCH_FILTER_LENGTH_MASK |
>> + T_SATA0_NVOOB_SQUELCH_FILTER_MODE_MASK);
>> + val = (T_SATA0_NVOOB_COMMA_CNT |
>> + T_SATA0_NVOOB_SQUELCH_FILTER_LENGTH |
>> + T_SATA0_NVOOB_SQUELCH_FILTER_MODE);
>> + tegra_ahci_scfg_update(tegra, val, mask, T_SATA0_NVOOB);
>> +
>> + /*
>> + * Change CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW from 83.3 ns
>to 58.8ns
>> + */
>> + mask =
>T_SATA0_CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW_MASK;
>> + val = T_SATA0_CFG2NVOOB_2_COMWAKE_IDLE_CNT_LOW;
>> + tegra_ahci_scfg_update(tegra, val, mask, T_SATA0_CFG2NVOOB_2);
>> +
>> + if (tegra->soc_data->ops.init)
>> + tegra->soc_data->ops.init(hpriv);
>> +
>> + /*
>> + * Program the following SATA configuration registers
>> + * to initialize SATA
>> + */
>> + val = (T_SATA_CFG_1_IO_SPACE | T_SATA_CFG_1_MEMORY_SPACE
>|
>> + T_SATA_CFG_1_BUS_MASTER | T_SATA_CFG_1_SERR);
>> + tegra_ahci_scfg_update(tegra, val, val, T_SATA_CFG_1);
>> + tegra_ahci_scfg_writel(tegra, T_SATA_CFG_9_BASE_ADDRESS,
>> +T_SATA_CFG_9);
>> +
>> + /* Program Class Code and Programming interface for SATA */
>> + val = T_SATA_CFG_SATA_BACKDOOR_PROG_IF_EN;
>> + tegra_ahci_scfg_update(tegra, val, val, T_SATA_CFG_SATA);
>> +
>> + mask = T_SATA_BKDOOR_CC_CLASS_CODE_MASK |
>T_SATA_BKDOOR_CC_PROG_IF_MASK;
>> + val = T_SATA_BKDOOR_CC_CLASS_CODE |
>T_SATA_BKDOOR_CC_PROG_IF;
>> + tegra_ahci_scfg_update(tegra, val, mask, T_SATA_BKDOOR_CC);
>> +
>> + tegra_ahci_scfg_update(tegra, 0,
>T_SATA_CFG_SATA_BACKDOOR_PROG_IF_EN,
>> + T_SATA_CFG_SATA);
>> +
>> + /* Enabling LPM capabilities through Backdoor Programming */
>> + val = (T_SATA0_AHCI_HBA_CAP_BKDR_PARTIAL_ST_CAP |
>> + T_SATA0_AHCI_HBA_CAP_BKDR_SLUMBER_ST_CAP |
>> + T_SATA0_AHCI_HBA_CAP_BKDR_SALP |
>> + T_SATA0_AHCI_HBA_CAP_BKDR_SUPP_PM);
>> + tegra_ahci_scfg_update(tegra, val, val,
>T_SATA0_AHCI_HBA_CAP_BKDR);
>> +
>> + /* SATA Second Level Clock Gating configuration
>> + * Enabling Gating of Tx/Rx clocks and driving Pad IDDQ and Lane
>> + * IDDQ Signals
>> + */
>> + mask = T_SATA0_CFG_35_IDP_INDEX_MASK;
>> + val = T_SATA0_CFG_35_IDP_INDEX;
>> + tegra_ahci_scfg_update(tegra, val, mask, T_SATA0_CFG_35);
>> + tegra_ahci_scfg_writel(tegra, T_SATA0_AHCI_IDP1_DATA,
>> + T_SATA0_AHCI_IDP1);
>> + val = (T_SATA0_CFG_PHY_1_PADS_IDDQ_EN |
>> + T_SATA0_CFG_PHY_1_PAD_PLL_IDDQ_EN);
>> + tegra_ahci_scfg_update(tegra, val, val, T_SATA0_CFG_PHY_1);
>> +
>> + /*
>> + * Indicate Sata only has the capability to enter DevSleep
>> + * from slumber link.
>> + */
>> + tegra_ahci_aux_update(tegra, DESO_SUPPORT, DESO_SUPPORT,
>> + SATA_AUX_MISC_CNTL_1_0);
>> + /* Enabling IPFS Clock Gating */
>> + tegra_ahci_sata_update(tegra, 0,
>SATA_CONFIGURATION_CLK_OVERRIDE,
>> + SATA_CONFIGURATION_0);
>> +
>> + tegra_ahci_sata_update(tegra, IP_INT_MASK, IP_INT_MASK,
>> + SATA_INTR_MASK_0);
>>
>> return 0;
>> }
>> @@ -274,19 +477,24 @@ static void tegra_ahci_host_stop(struct
>ata_host
>> *host) }
>>
>> static struct ata_port_operations ahci_tegra_port_ops = {
>> - .inherits = &ahci_ops,
>> - .host_stop = tegra_ahci_host_stop,
>> + .inherits = &ahci_ops,
>> + .host_stop = tegra_ahci_host_stop,
>> };
>>
>> static const struct ata_port_info ahci_tegra_port_info = {
>> - .flags = AHCI_FLAG_COMMON,
>> - .pio_mask = ATA_PIO4,
>> - .udma_mask = ATA_UDMA6,
>> - .port_ops = &ahci_tegra_port_ops,
>> + .flags = AHCI_FLAG_COMMON,
>> + .pio_mask = ATA_PIO4,
>> + .udma_mask = ATA_UDMA6,
>> + .port_ops = &ahci_tegra_port_ops,
>> };
>
>Please keep the original style as to keep the patch as small as possible.
>
Ok.
>>
>> static const struct of_device_id tegra_ahci_of_match[] = {
>> - { .compatible = "nvidia,tegra124-ahci" },
>> + {
>> + .compatible = "nvidia,tegra124-ahci",
>> + .data = &tegra124_ahci_soc_data},
>> + {
>> + .compatible = "nvidia,tegra210-ahci",
>> + .data = &tegra210_ahci_soc_data},
>
>Please write these like
>
>{
> .compatible = "nvidia,tegra124-ahci",
> .data = &tegra124_ahci_soc_data,
>},
>
Ok.
>> {}
>> };
>> MODULE_DEVICE_TABLE(of, tegra_ahci_of_match); @@ -301,6 +509,7
>@@
>> static int tegra_ahci_probe(struct platform_device *pdev)
>> struct tegra_ahci_priv *tegra;
>> struct resource *res;
>> int ret;
>> + unsigned int i;
>>
>> hpriv = ahci_platform_get_resources(pdev);
>> if (IS_ERR(hpriv))
>> @@ -311,13 +520,18 @@ static int tegra_ahci_probe(struct
>platform_device *pdev)
>> return -ENOMEM;
>>
>> hpriv->plat_data = tegra;
>> -
>> tegra->pdev = pdev;
>> + tegra->soc_data =
>> + (struct tegra_ahci_soc *)of_device_get_match_data(&pdev->dev);
>>
>> res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> tegra->sata_regs = devm_ioremap_resource(&pdev->dev, res);
>> if (IS_ERR(tegra->sata_regs))
>> return PTR_ERR(tegra->sata_regs);
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
>> + tegra->sata_aux_regs = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(tegra->sata_aux_regs))
>> + return PTR_ERR(tegra->sata_aux_regs);
>
>Please add this register range also to the device tree binding documentation.
>Also, please provide a patch to enable the SATA controller on a Tegra210
>board - preferably the Jetson TX1 (or internal
>variant) so that it is easy to test.
>
Ok.
>>
>> tegra->sata_rst = devm_reset_control_get(&pdev->dev, "sata");
>> if (IS_ERR(tegra->sata_rst)) {
>> @@ -343,13 +557,17 @@ static int tegra_ahci_probe(struct
>platform_device *pdev)
>> return PTR_ERR(tegra->sata_clk);
>> }
>>
>> - tegra->supplies[0].supply = "avdd";
>> - tegra->supplies[1].supply = "hvdd";
>> - tegra->supplies[2].supply = "vddio";
>> - tegra->supplies[3].supply = "target-5v";
>> - tegra->supplies[4].supply = "target-12v";
>> + tegra->supplies = devm_kcalloc(&pdev->dev,
>> + tegra->soc_data->num_supplies,
>> + sizeof(*tegra->supplies), GFP_KERNEL);
>> + if (!tegra->supplies)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < tegra->soc_data->num_supplies; i++)
>> + tegra->supplies[i].supply = tegra->soc_data-
>>supply_names[i];
>>
>> - ret = devm_regulator_bulk_get(&pdev->dev, ARRAY_SIZE(tegra-
>>supplies),
>> + ret = devm_regulator_bulk_get(&pdev->dev,
>> + tegra->soc_data->num_supplies,
>> tegra->supplies);
>> if (ret) {
>> dev_err(&pdev->dev, "Failed to get regulators\n"); @@ -
>377,13
>> +595,13 @@ static struct platform_driver tegra_ahci_driver = {
>> .probe = tegra_ahci_probe,
>> .remove = ata_platform_remove_one,
>> .driver = {
>> - .name = DRV_NAME,
>> - .of_match_table = tegra_ahci_of_match,
>> - },
>> + .name = DRV_NAME,
>> + .of_match_table = tegra_ahci_of_match,
>> + },
>
>Please keep the style unchanged here as well.
>
Ok.
>> /* LP0 suspend support not implemented */ };
>> module_platform_driver(tegra_ahci_driver);
>>
>> MODULE_AUTHOR("Mikko Perttunen <mperttunen@xxxxxxxxxx>");
>> -MODULE_DESCRIPTION("Tegra124 AHCI SATA driver");
>> +MODULE_DESCRIPTION("Tegra AHCI SATA driver");
>
>Awesome!
>
>> MODULE_LICENSE("GPL v2");
>>
>>
>
>Thanks,
>Mikko
��.n��������+%������w��{.n�����{��'^�)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
