V4 main changes: Thanks for Niklas' comments. - Update the commit message in #2 patch of v4. - Split the clean up unrelated codes to #3 and #4 of v4. - Remove the Cc: stable@vger.kernel.org and Fixes tag in #5 of v4.
V3 main changes: - Use GENMASK() macro to define the _MASK. - Refine the macro names.
V2 main changes: - Add Rob's reviewed-by in the binding patch. - Re-name the error out lables and new RXWM macro more descriptive. - In #3 patch, add one fix tag, and CC stable kernel.
Based on i.MX8QM HSIO PHY driver, refine i.MX8QM SATA driver by using PHY interface.
[PATCH v4 1/6] dt-bindings: ata: Add i.MX8QM AHCI compatible string [PATCH v4 2/6] ata: ahci_imx: Clean up code by using i.MX8Q HSIO PHY [PATCH v4 3/6] ata: ahci_imx: AHB clock rate setting is not required [PATCH v4 4/6] ata: ahci_imx: Add 32bits DMA limit for i.MX8QM AHCI [PATCH v4 5/6] ata: ahci_imx: Enlarge RX water mark for i.MX8QM SATA [PATCH v4 6/6] ata: ahci_imx: Correct the email address
Documentation/devicetree/bindings/ata/imx-sata.yaml | 47 +++++++++++ drivers/ata/ahci_imx.c | 406 ++++++++++++++++++++++++----------------------------------------------------------------- 2 files changed, 155 insertions(+), 298 deletions(-)
Add i.MX8QM AHCI "fsl,imx8qm-ahci" compatible strings.
i.MX8QM AHCI SATA doesn't require AHB clock rate to set the vendor specified TIMER1MS register. ahb clock is not required by i.MX8QM AHCI.
Update the description of clocks in the dt-binding accordingly.
Signed-off-by: Richard Zhu hongxing.zhu@nxp.com Reviewed-by: Rob Herring (Arm) robh@kernel.org --- .../devicetree/bindings/ata/imx-sata.yaml | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+)
diff --git a/Documentation/devicetree/bindings/ata/imx-sata.yaml b/Documentation/devicetree/bindings/ata/imx-sata.yaml index 68ffb97ddc9b2..f4eb3550a0960 100644 --- a/Documentation/devicetree/bindings/ata/imx-sata.yaml +++ b/Documentation/devicetree/bindings/ata/imx-sata.yaml @@ -19,6 +19,7 @@ properties: - fsl,imx53-ahci - fsl,imx6q-ahci - fsl,imx6qp-ahci + - fsl,imx8qm-ahci
reg: maxItems: 1 @@ -27,12 +28,14 @@ properties: maxItems: 1
clocks: + minItems: 2 items: - description: sata clock - description: sata reference clock - description: ahb clock
clock-names: + minItems: 2 items: - const: sata - const: sata_ref @@ -58,6 +61,25 @@ properties: $ref: /schemas/types.yaml#/definitions/flag description: if present, disable spread-spectrum clocking on the SATA link.
+ phys: + items: + - description: phandle to SATA PHY. + Since "REXT" pin is only present for first lane of i.MX8QM PHY, it's + calibration result will be stored, passed through second lane, and + shared with all three lanes PHY. The first two lanes PHY are used as + calibration PHYs, although only the third lane PHY is used by SATA. + - description: phandle to the first lane PHY of i.MX8QM. + - description: phandle to the second lane PHY of i.MX8QM. + + phy-names: + items: + - const: sata-phy + - const: cali-phy0 + - const: cali-phy1 + + power-domains: + maxItems: 1 + required: - compatible - reg @@ -65,6 +87,31 @@ required: - clocks - clock-names
+allOf: + - if: + properties: + compatible: + contains: + enum: + - fsl,imx53-ahci + - fsl,imx6q-ahci + - fsl,imx6qp-ahci + then: + properties: + clock-names: + minItems: 3 + + - if: + properties: + compatible: + contains: + enum: + - fsl,imx8qm-ahci + then: + properties: + clock-names: + minItems: 2 + additionalProperties: false
examples:
Clean up code by using PHY interface provided by the PHY driver under PHY subsystem(drivers/phy/freescale/phy-fsl-imx8qm-hsio.c).
Signed-off-by: Richard Zhu hongxing.zhu@nxp.com --- drivers/ata/ahci_imx.c | 365 +++++++++-------------------------------- 1 file changed, 80 insertions(+), 285 deletions(-)
diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c index cb768f66f0a70..75258ed42d2ee 100644 --- a/drivers/ata/ahci_imx.c +++ b/drivers/ata/ahci_imx.c @@ -19,6 +19,7 @@ #include <linux/libata.h> #include <linux/hwmon.h> #include <linux/hwmon-sysfs.h> +#include <linux/phy/phy.h> #include <linux/thermal.h> #include "ahci.h"
@@ -44,42 +45,6 @@ enum { /* Clock Reset Register */ IMX_CLOCK_RESET = 0x7f3f, IMX_CLOCK_RESET_RESET = 1 << 0, - /* IMX8QM HSIO AHCI definitions */ - IMX8QM_SATA_PHY_RX_IMPED_RATIO_OFFSET = 0x03, - IMX8QM_SATA_PHY_TX_IMPED_RATIO_OFFSET = 0x09, - IMX8QM_SATA_PHY_IMPED_RATIO_85OHM = 0x6c, - IMX8QM_LPCG_PHYX2_OFFSET = 0x00000, - IMX8QM_CSR_PHYX2_OFFSET = 0x90000, - IMX8QM_CSR_PHYX1_OFFSET = 0xa0000, - IMX8QM_CSR_PHYX_STTS0_OFFSET = 0x4, - IMX8QM_CSR_PCIEA_OFFSET = 0xb0000, - IMX8QM_CSR_PCIEB_OFFSET = 0xc0000, - IMX8QM_CSR_SATA_OFFSET = 0xd0000, - IMX8QM_CSR_PCIE_CTRL2_OFFSET = 0x8, - IMX8QM_CSR_MISC_OFFSET = 0xe0000, - - IMX8QM_LPCG_PHYX2_PCLK0_MASK = (0x3 << 16), - IMX8QM_LPCG_PHYX2_PCLK1_MASK = (0x3 << 20), - IMX8QM_PHY_APB_RSTN_0 = BIT(0), - IMX8QM_PHY_MODE_SATA = BIT(19), - IMX8QM_PHY_MODE_MASK = (0xf << 17), - IMX8QM_PHY_PIPE_RSTN_0 = BIT(24), - IMX8QM_PHY_PIPE_RSTN_OVERRIDE_0 = BIT(25), - IMX8QM_PHY_PIPE_RSTN_1 = BIT(26), - IMX8QM_PHY_PIPE_RSTN_OVERRIDE_1 = BIT(27), - IMX8QM_STTS0_LANE0_TX_PLL_LOCK = BIT(4), - IMX8QM_MISC_IOB_RXENA = BIT(0), - IMX8QM_MISC_IOB_TXENA = BIT(1), - IMX8QM_MISC_PHYX1_EPCS_SEL = BIT(12), - IMX8QM_MISC_CLKREQN_OUT_OVERRIDE_1 = BIT(24), - IMX8QM_MISC_CLKREQN_OUT_OVERRIDE_0 = BIT(25), - IMX8QM_MISC_CLKREQN_IN_OVERRIDE_1 = BIT(28), - IMX8QM_MISC_CLKREQN_IN_OVERRIDE_0 = BIT(29), - IMX8QM_SATA_CTRL_RESET_N = BIT(12), - IMX8QM_SATA_CTRL_EPCS_PHYRESET_N = BIT(7), - IMX8QM_CTRL_BUTTON_RST_N = BIT(21), - IMX8QM_CTRL_POWER_UP_RST_N = BIT(23), - IMX8QM_CTRL_LTSSM_ENABLE = BIT(4), };
enum ahci_imx_type { @@ -95,14 +60,10 @@ struct imx_ahci_priv { struct clk *sata_clk; struct clk *sata_ref_clk; struct clk *ahb_clk; - struct clk *epcs_tx_clk; - struct clk *epcs_rx_clk; - struct clk *phy_apbclk; - struct clk *phy_pclk0; - struct clk *phy_pclk1; - void __iomem *phy_base; - struct gpio_desc *clkreq_gpiod; struct regmap *gpr; + struct phy *sata_phy; + struct phy *cali_phy0; + struct phy *cali_phy1; bool no_device; bool first_time; u32 phy_params; @@ -450,201 +411,73 @@ ATTRIBUTE_GROUPS(fsl_sata_ahci);
static int imx8_sata_enable(struct ahci_host_priv *hpriv) { - u32 val, reg; - int i, ret; + u32 val; + int ret; struct imx_ahci_priv *imxpriv = hpriv->plat_data; struct device *dev = &imxpriv->ahci_pdev->dev;
- /* configure the hsio for sata */ - ret = clk_prepare_enable(imxpriv->phy_pclk0); - if (ret < 0) { - dev_err(dev, "can't enable phy_pclk0.\n"); + /* + * Since "REXT" pin is only present for first lane of i.MX8QM + * PHY, its calibration results will be stored, passed through + * to the second lane PHY, and shared with all three lane PHYs. + * + * Initialize the first two lane PHYs here, although only the + * third lane PHY is used by SATA. + */ + ret = phy_init(imxpriv->cali_phy0); + if (ret) { + dev_err(dev, "cali PHY init failed\n"); return ret; } - ret = clk_prepare_enable(imxpriv->phy_pclk1); - if (ret < 0) { - dev_err(dev, "can't enable phy_pclk1.\n"); - goto disable_phy_pclk0; + ret = phy_power_on(imxpriv->cali_phy0); + if (ret) { + dev_err(dev, "cali PHY power on failed\n"); + goto err_cali_phy0_exit; } - ret = clk_prepare_enable(imxpriv->epcs_tx_clk); - if (ret < 0) { - dev_err(dev, "can't enable epcs_tx_clk.\n"); - goto disable_phy_pclk1; + ret = phy_init(imxpriv->cali_phy1); + if (ret) { + dev_err(dev, "cali PHY1 init failed\n"); + goto err_cali_phy0_off; } - ret = clk_prepare_enable(imxpriv->epcs_rx_clk); - if (ret < 0) { - dev_err(dev, "can't enable epcs_rx_clk.\n"); - goto disable_epcs_tx_clk; + ret = phy_power_on(imxpriv->cali_phy1); + if (ret) { + dev_err(dev, "cali PHY1 power on failed\n"); + goto err_cali_phy1_exit; } - ret = clk_prepare_enable(imxpriv->phy_apbclk); - if (ret < 0) { - dev_err(dev, "can't enable phy_apbclk.\n"); - goto disable_epcs_rx_clk; + ret = phy_init(imxpriv->sata_phy); + if (ret) { + dev_err(dev, "sata PHY init failed\n"); + goto err_cali_phy1_off; } - /* Configure PHYx2 PIPE_RSTN */ - regmap_read(imxpriv->gpr, IMX8QM_CSR_PCIEA_OFFSET + - IMX8QM_CSR_PCIE_CTRL2_OFFSET, &val); - if ((val & IMX8QM_CTRL_LTSSM_ENABLE) == 0) { - /* The link of the PCIEA of HSIO is down */ - regmap_update_bits(imxpriv->gpr, - IMX8QM_CSR_PHYX2_OFFSET, - IMX8QM_PHY_PIPE_RSTN_0 | - IMX8QM_PHY_PIPE_RSTN_OVERRIDE_0, - IMX8QM_PHY_PIPE_RSTN_0 | - IMX8QM_PHY_PIPE_RSTN_OVERRIDE_0); + ret = phy_set_mode(imxpriv->sata_phy, PHY_MODE_SATA); + if (ret) { + dev_err(dev, "unable to set SATA PHY mode\n"); + goto err_sata_phy_exit; } - regmap_read(imxpriv->gpr, IMX8QM_CSR_PCIEB_OFFSET + - IMX8QM_CSR_PCIE_CTRL2_OFFSET, ®); - if ((reg & IMX8QM_CTRL_LTSSM_ENABLE) == 0) { - /* The link of the PCIEB of HSIO is down */ - regmap_update_bits(imxpriv->gpr, - IMX8QM_CSR_PHYX2_OFFSET, - IMX8QM_PHY_PIPE_RSTN_1 | - IMX8QM_PHY_PIPE_RSTN_OVERRIDE_1, - IMX8QM_PHY_PIPE_RSTN_1 | - IMX8QM_PHY_PIPE_RSTN_OVERRIDE_1); - } - if (((reg | val) & IMX8QM_CTRL_LTSSM_ENABLE) == 0) { - /* The links of both PCIA and PCIEB of HSIO are down */ - regmap_update_bits(imxpriv->gpr, - IMX8QM_LPCG_PHYX2_OFFSET, - IMX8QM_LPCG_PHYX2_PCLK0_MASK | - IMX8QM_LPCG_PHYX2_PCLK1_MASK, - 0); + ret = phy_power_on(imxpriv->sata_phy); + if (ret) { + dev_err(dev, "sata PHY power up failed\n"); + goto err_sata_phy_exit; }
- /* set PWR_RST and BT_RST of csr_pciea */ - val = IMX8QM_CSR_PCIEA_OFFSET + IMX8QM_CSR_PCIE_CTRL2_OFFSET; - regmap_update_bits(imxpriv->gpr, - val, - IMX8QM_CTRL_BUTTON_RST_N, - IMX8QM_CTRL_BUTTON_RST_N); - regmap_update_bits(imxpriv->gpr, - val, - IMX8QM_CTRL_POWER_UP_RST_N, - IMX8QM_CTRL_POWER_UP_RST_N); - - /* PHYX1_MODE to SATA */ - regmap_update_bits(imxpriv->gpr, - IMX8QM_CSR_PHYX1_OFFSET, - IMX8QM_PHY_MODE_MASK, - IMX8QM_PHY_MODE_SATA); + /* The cali_phy# can be turned off after SATA PHY is initialized. */ + phy_power_off(imxpriv->cali_phy1); + phy_exit(imxpriv->cali_phy1); + phy_power_off(imxpriv->cali_phy0); + phy_exit(imxpriv->cali_phy0);
- /* - * BIT0 RXENA 1, BIT1 TXENA 0 - * BIT12 PHY_X1_EPCS_SEL 1. - */ - regmap_update_bits(imxpriv->gpr, - IMX8QM_CSR_MISC_OFFSET, - IMX8QM_MISC_IOB_RXENA, - IMX8QM_MISC_IOB_RXENA); - regmap_update_bits(imxpriv->gpr, - IMX8QM_CSR_MISC_OFFSET, - IMX8QM_MISC_IOB_TXENA, - 0); - regmap_update_bits(imxpriv->gpr, - IMX8QM_CSR_MISC_OFFSET, - IMX8QM_MISC_PHYX1_EPCS_SEL, - IMX8QM_MISC_PHYX1_EPCS_SEL); - /* - * It is possible, for PCIe and SATA are sharing - * the same clock source, HPLL or external oscillator. - * When PCIe is in low power modes (L1.X or L2 etc), - * the clock source can be turned off. In this case, - * if this clock source is required to be toggling by - * SATA, then SATA functions will be abnormal. - * Set the override here to avoid it. - */ - regmap_update_bits(imxpriv->gpr, - IMX8QM_CSR_MISC_OFFSET, - IMX8QM_MISC_CLKREQN_OUT_OVERRIDE_1 | - IMX8QM_MISC_CLKREQN_OUT_OVERRIDE_0 | - IMX8QM_MISC_CLKREQN_IN_OVERRIDE_1 | - IMX8QM_MISC_CLKREQN_IN_OVERRIDE_0, - IMX8QM_MISC_CLKREQN_OUT_OVERRIDE_1 | - IMX8QM_MISC_CLKREQN_OUT_OVERRIDE_0 | - IMX8QM_MISC_CLKREQN_IN_OVERRIDE_1 | - IMX8QM_MISC_CLKREQN_IN_OVERRIDE_0); - - /* clear PHY RST, then set it */ - regmap_update_bits(imxpriv->gpr, - IMX8QM_CSR_SATA_OFFSET, - IMX8QM_SATA_CTRL_EPCS_PHYRESET_N, - 0); - - regmap_update_bits(imxpriv->gpr, - IMX8QM_CSR_SATA_OFFSET, - IMX8QM_SATA_CTRL_EPCS_PHYRESET_N, - IMX8QM_SATA_CTRL_EPCS_PHYRESET_N); - - /* CTRL RST: SET -> delay 1 us -> CLEAR -> SET */ - regmap_update_bits(imxpriv->gpr, - IMX8QM_CSR_SATA_OFFSET, - IMX8QM_SATA_CTRL_RESET_N, - IMX8QM_SATA_CTRL_RESET_N); - udelay(1); - regmap_update_bits(imxpriv->gpr, - IMX8QM_CSR_SATA_OFFSET, - IMX8QM_SATA_CTRL_RESET_N, - 0); - regmap_update_bits(imxpriv->gpr, - IMX8QM_CSR_SATA_OFFSET, - IMX8QM_SATA_CTRL_RESET_N, - IMX8QM_SATA_CTRL_RESET_N); - - /* APB reset */ - regmap_update_bits(imxpriv->gpr, - IMX8QM_CSR_PHYX1_OFFSET, - IMX8QM_PHY_APB_RSTN_0, - IMX8QM_PHY_APB_RSTN_0); - - for (i = 0; i < 100; i++) { - reg = IMX8QM_CSR_PHYX1_OFFSET + - IMX8QM_CSR_PHYX_STTS0_OFFSET; - regmap_read(imxpriv->gpr, reg, &val); - val &= IMX8QM_STTS0_LANE0_TX_PLL_LOCK; - if (val == IMX8QM_STTS0_LANE0_TX_PLL_LOCK) - break; - udelay(1); - } - - if (val != IMX8QM_STTS0_LANE0_TX_PLL_LOCK) { - dev_err(dev, "TX PLL of the PHY is not locked\n"); - ret = -ENODEV; - } else { - writeb(imxpriv->imped_ratio, imxpriv->phy_base + - IMX8QM_SATA_PHY_RX_IMPED_RATIO_OFFSET); - writeb(imxpriv->imped_ratio, imxpriv->phy_base + - IMX8QM_SATA_PHY_TX_IMPED_RATIO_OFFSET); - reg = readb(imxpriv->phy_base + - IMX8QM_SATA_PHY_RX_IMPED_RATIO_OFFSET); - if (unlikely(reg != imxpriv->imped_ratio)) - dev_info(dev, "Can't set PHY RX impedance ratio.\n"); - reg = readb(imxpriv->phy_base + - IMX8QM_SATA_PHY_TX_IMPED_RATIO_OFFSET); - if (unlikely(reg != imxpriv->imped_ratio)) - dev_info(dev, "Can't set PHY TX impedance ratio.\n"); - usleep_range(50, 100); - - /* - * To reduce the power consumption, gate off - * the PHY clks - */ - clk_disable_unprepare(imxpriv->phy_apbclk); - clk_disable_unprepare(imxpriv->phy_pclk1); - clk_disable_unprepare(imxpriv->phy_pclk0); - return ret; - } + return 0;
- clk_disable_unprepare(imxpriv->phy_apbclk); -disable_epcs_rx_clk: - clk_disable_unprepare(imxpriv->epcs_rx_clk); -disable_epcs_tx_clk: - clk_disable_unprepare(imxpriv->epcs_tx_clk); -disable_phy_pclk1: - clk_disable_unprepare(imxpriv->phy_pclk1); -disable_phy_pclk0: - clk_disable_unprepare(imxpriv->phy_pclk0); +err_sata_phy_exit: + phy_exit(imxpriv->sata_phy); +err_cali_phy1_off: + phy_power_off(imxpriv->cali_phy1); +err_cali_phy1_exit: + phy_exit(imxpriv->cali_phy1); +err_cali_phy0_off: + phy_power_off(imxpriv->cali_phy0); +err_cali_phy0_exit: + phy_exit(imxpriv->cali_phy0);
return ret; } @@ -698,6 +531,9 @@ static int imx_sata_enable(struct ahci_host_priv *hpriv) } } else if (imxpriv->type == AHCI_IMX8QM) { ret = imx8_sata_enable(hpriv); + if (ret) + goto disable_clk; + }
usleep_range(1000, 2000); @@ -736,8 +572,10 @@ static void imx_sata_disable(struct ahci_host_priv *hpriv) break;
case AHCI_IMX8QM: - clk_disable_unprepare(imxpriv->epcs_rx_clk); - clk_disable_unprepare(imxpriv->epcs_tx_clk); + if (imxpriv->sata_phy) { + phy_power_off(imxpriv->sata_phy); + phy_exit(imxpriv->sata_phy); + } break;
default: @@ -760,6 +598,9 @@ static void ahci_imx_error_handler(struct ata_port *ap)
ahci_error_handler(ap);
+ if (imxpriv->type == AHCI_IMX8QM) + return; + if (!(imxpriv->first_time) || ahci_imx_hotplug) return;
@@ -986,65 +827,19 @@ static const struct scsi_host_template ahci_platform_sht = {
static int imx8_sata_probe(struct device *dev, struct imx_ahci_priv *imxpriv) { - struct resource *phy_res; - struct platform_device *pdev = imxpriv->ahci_pdev; - struct device_node *np = dev->of_node; - - if (of_property_read_u32(np, "fsl,phy-imp", &imxpriv->imped_ratio)) - imxpriv->imped_ratio = IMX8QM_SATA_PHY_IMPED_RATIO_85OHM; - phy_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "phy"); - if (phy_res) { - imxpriv->phy_base = devm_ioremap(dev, phy_res->start, - resource_size(phy_res)); - if (!imxpriv->phy_base) { - dev_err(dev, "error with ioremap\n"); - return -ENOMEM; - } - } else { - dev_err(dev, "missing *phy* reg region.\n"); - return -ENOMEM; - } - imxpriv->gpr = - syscon_regmap_lookup_by_phandle(np, "hsio"); - if (IS_ERR(imxpriv->gpr)) { - dev_err(dev, "unable to find gpr registers\n"); - return PTR_ERR(imxpriv->gpr); - } - - imxpriv->epcs_tx_clk = devm_clk_get(dev, "epcs_tx"); - if (IS_ERR(imxpriv->epcs_tx_clk)) { - dev_err(dev, "can't get epcs_tx_clk clock.\n"); - return PTR_ERR(imxpriv->epcs_tx_clk); - } - imxpriv->epcs_rx_clk = devm_clk_get(dev, "epcs_rx"); - if (IS_ERR(imxpriv->epcs_rx_clk)) { - dev_err(dev, "can't get epcs_rx_clk clock.\n"); - return PTR_ERR(imxpriv->epcs_rx_clk); - } - imxpriv->phy_pclk0 = devm_clk_get(dev, "phy_pclk0"); - if (IS_ERR(imxpriv->phy_pclk0)) { - dev_err(dev, "can't get phy_pclk0 clock.\n"); - return PTR_ERR(imxpriv->phy_pclk0); - } - imxpriv->phy_pclk1 = devm_clk_get(dev, "phy_pclk1"); - if (IS_ERR(imxpriv->phy_pclk1)) { - dev_err(dev, "can't get phy_pclk1 clock.\n"); - return PTR_ERR(imxpriv->phy_pclk1); - } - imxpriv->phy_apbclk = devm_clk_get(dev, "phy_apbclk"); - if (IS_ERR(imxpriv->phy_apbclk)) { - dev_err(dev, "can't get phy_apbclk clock.\n"); - return PTR_ERR(imxpriv->phy_apbclk); - } - - /* Fetch GPIO, then enable the external OSC */ - imxpriv->clkreq_gpiod = devm_gpiod_get_optional(dev, "clkreq", - GPIOD_OUT_LOW | GPIOD_FLAGS_BIT_NONEXCLUSIVE); - if (IS_ERR(imxpriv->clkreq_gpiod)) - return PTR_ERR(imxpriv->clkreq_gpiod); - if (imxpriv->clkreq_gpiod) - gpiod_set_consumer_name(imxpriv->clkreq_gpiod, "SATA CLKREQ"); - + imxpriv->sata_phy = devm_phy_get(dev, "sata-phy"); + if (IS_ERR(imxpriv->sata_phy)) + return dev_err_probe(dev, PTR_ERR(imxpriv->sata_phy), + "Failed to get sata_phy\n"); + + imxpriv->cali_phy0 = devm_phy_get(dev, "cali-phy0"); + if (IS_ERR(imxpriv->cali_phy0)) + return dev_err_probe(dev, PTR_ERR(imxpriv->cali_phy0), + "Failed to get cali_phy0\n"); + imxpriv->cali_phy1 = devm_phy_get(dev, "cali-phy1"); + if (IS_ERR(imxpriv->cali_phy1)) + return dev_err_probe(dev, PTR_ERR(imxpriv->cali_phy1), + "Failed to get cali_phy1\n"); return 0; }
i.MX8QM AHCI SATA doesn't need AHB clock rate to set the vendor specified TIMER1MS register. Do the AHB clock rate setting for i.MX53 and i.MX6Q AHCI SATA only.
Signed-off-by: Richard Zhu hongxing.zhu@nxp.com --- drivers/ata/ahci_imx.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-)
diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c index 75258ed42d2ee..4dd98368f8562 100644 --- a/drivers/ata/ahci_imx.c +++ b/drivers/ata/ahci_imx.c @@ -872,12 +872,6 @@ static int imx_ahci_probe(struct platform_device *pdev) return PTR_ERR(imxpriv->sata_ref_clk); }
- imxpriv->ahb_clk = devm_clk_get(dev, "ahb"); - if (IS_ERR(imxpriv->ahb_clk)) { - dev_err(dev, "can't get ahb clock.\n"); - return PTR_ERR(imxpriv->ahb_clk); - } - if (imxpriv->type == AHCI_IMX6Q || imxpriv->type == AHCI_IMX6QP) { u32 reg_value;
@@ -937,11 +931,8 @@ static int imx_ahci_probe(struct platform_device *pdev) goto disable_clk;
/* - * Configure the HWINIT bits of the HOST_CAP and HOST_PORTS_IMPL, - * and IP vendor specific register IMX_TIMER1MS. - * Configure CAP_SSS (support stagered spin up). - * Implement the port0. - * Get the ahb clock rate, and configure the TIMER1MS register. + * Configure the HWINIT bits of the HOST_CAP and HOST_PORTS_IMPL. + * Set CAP_SSS (support stagered spin up) and Implement the port0. */ reg_val = readl(hpriv->mmio + HOST_CAP); if (!(reg_val & HOST_CAP_SSS)) { @@ -954,8 +945,19 @@ static int imx_ahci_probe(struct platform_device *pdev) writel(reg_val, hpriv->mmio + HOST_PORTS_IMPL); }
- reg_val = clk_get_rate(imxpriv->ahb_clk) / 1000; - writel(reg_val, hpriv->mmio + IMX_TIMER1MS); + if (imxpriv->type != AHCI_IMX8QM) { + /* + * Get AHB clock rate and configure the vendor specified + * TIMER1MS register on i.MX53, i.MX6Q and i.MX6QP only. + */ + imxpriv->ahb_clk = devm_clk_get(dev, "ahb"); + if (IS_ERR(imxpriv->ahb_clk)) { + dev_err(dev, "Failed to get ahb clock\n"); + goto disable_sata; + } + reg_val = clk_get_rate(imxpriv->ahb_clk) / 1000; + writel(reg_val, hpriv->mmio + IMX_TIMER1MS); + }
ret = ahci_platform_init_host(pdev, hpriv, &ahci_imx_port_info, &ahci_platform_sht);
Since i.MX8QM AHCI SATA only has 32bits DMA capability. Add 32bits DMA limit here.
Signed-off-by: Richard Zhu hongxing.zhu@nxp.com --- drivers/ata/ahci_imx.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c index 4dd98368f8562..e94c0fdea2260 100644 --- a/drivers/ata/ahci_imx.c +++ b/drivers/ata/ahci_imx.c @@ -827,6 +827,9 @@ static const struct scsi_host_template ahci_platform_sht = {
static int imx8_sata_probe(struct device *dev, struct imx_ahci_priv *imxpriv) { + if (!(dev->bus_dma_limit)) + dev->bus_dma_limit = DMA_BIT_MASK(32); + imxpriv->sata_phy = devm_phy_get(dev, "sata-phy"); if (IS_ERR(imxpriv->sata_phy)) return dev_err_probe(dev, PTR_ERR(imxpriv->sata_phy),
Hi,
Thanks for your patch.
FYI: kernel test robot notices the stable kernel rule is not satisfied.
The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#opti...
Rule: add the tag "Cc: stable@vger.kernel.org" in the sign-off area to have the patch automatically included in the stable tree. Subject: [PATCH v4 4/6] ata: ahci_imx: Add 32bits DMA limit for i.MX8QM AHCI SATA Link: https://lore.kernel.org/stable/1721367736-30156-5-git-send-email-hongxing.zh...
On Fri, Jul 19, 2024 at 01:42:14PM +0800, Richard Zhu wrote:
Since i.MX8QM AHCI SATA only has 32bits DMA capability. Add 32bits DMA limit here.
Signed-off-by: Richard Zhu hongxing.zhu@nxp.com
drivers/ata/ahci_imx.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c index 4dd98368f8562..e94c0fdea2260 100644 --- a/drivers/ata/ahci_imx.c +++ b/drivers/ata/ahci_imx.c @@ -827,6 +827,9 @@ static const struct scsi_host_template ahci_platform_sht = { static int imx8_sata_probe(struct device *dev, struct imx_ahci_priv *imxpriv) {
- if (!(dev->bus_dma_limit))
dev->bus_dma_limit = DMA_BIT_MASK(32);- imxpriv->sata_phy = devm_phy_get(dev, "sata-phy"); if (IS_ERR(imxpriv->sata_phy)) return dev_err_probe(dev, PTR_ERR(imxpriv->sata_phy),
-- 2.37.1
Why is this needed?
ahci_imx.c calls ahci_platform_init_host(), which calls dma_coerce_mask_and_coherent(): https://github.com/torvalds/linux/blob/v6.10/drivers/ata/libahci_platform.c#...
Should this code perhaps look more like: https://github.com/torvalds/linux/blob/v6.10/drivers/ata/ahci.c#L1048-L1054
where we set it to 64 or 32 bit explicitly.
Does this solve your problem: diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c index 581704e61f28..fc86e2c8c42b 100644 --- a/drivers/ata/libahci_platform.c +++ b/drivers/ata/libahci_platform.c @@ -747,12 +747,11 @@ int ahci_platform_init_host(struct platform_device *pdev, ap->ops = &ata_dummy_port_ops; }
- if (hpriv->cap & HOST_CAP_64) { - rc = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64)); - if (rc) { - dev_err(dev, "Failed to enable 64-bit DMA.\n"); - return rc; - } + rc = dma_coerce_mask_and_coherent(dev, + DMA_BIT_MASK((hpriv->cap & HOST_CAP_64) ? 64 : 32)); + if (rc) { + dev_err(dev, "DMA enable failed\n"); + return rc; }
rc = ahci_reset_controller(host);
Kind regards, Niklas
-----Original Message----- From: Niklas Cassel cassel@kernel.org Sent: 2024年7月24日 0:04 To: Hongxing Zhu hongxing.zhu@nxp.com Cc: tj@kernel.org; dlemoal@kernel.org; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com; linux-ide@vger.kernel.org; stable@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; imx@lists.linux.dev; kernel@pengutronix.de Subject: Re: [PATCH v4 4/6] ata: ahci_imx: Add 32bits DMA limit for i.MX8QM AHCI SATA
On Fri, Jul 19, 2024 at 01:42:14PM +0800, Richard Zhu wrote:
Since i.MX8QM AHCI SATA only has 32bits DMA capability. Add 32bits DMA limit here.
Signed-off-by: Richard Zhu hongxing.zhu@nxp.com
drivers/ata/ahci_imx.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c index 4dd98368f8562..e94c0fdea2260 100644 --- a/drivers/ata/ahci_imx.c +++ b/drivers/ata/ahci_imx.c @@ -827,6 +827,9 @@ static const struct scsi_host_template ahci_platform_sht = {
static int imx8_sata_probe(struct device *dev, struct imx_ahci_priv *imxpriv) {
- if (!(dev->bus_dma_limit))
dev->bus_dma_limit = DMA_BIT_MASK(32);- imxpriv->sata_phy = devm_phy_get(dev, "sata-phy"); if (IS_ERR(imxpriv->sata_phy)) return dev_err_probe(dev, PTR_ERR(imxpriv->sata_phy),
-- 2.37.1
Why is this needed?
ahci_imx.c calls ahci_platform_init_host(), which calls dma_coerce_mask_and_coherent(): https://github.co/ m%2Ftorvalds%2Flinux%2Fblob%2Fv6.10%2Fdrivers%2Fata%2Flibahci_platfor m.c%23L750-L756&data=05%7C02%7Chongxing.zhu%40nxp.com%7C9d64eab e3c5f4af9d15808dcab312651%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0 %7C0%7C638573474782493607%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC 4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7 C%7C&sdata=4gHxszzym8hOgVIo6%2BM2JHMyBa5I5U65j08fH3P34BY%3D&re served=0
Should this code perhaps look more like: https://github.co/ m%2Ftorvalds%2Flinux%2Fblob%2Fv6.10%2Fdrivers%2Fata%2Fahci.c%23L104 8-L1054&data=05%7C02%7Chongxing.zhu%40nxp.com%7C9d64eabe3c5f4af9 d15808dcab312651%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C 638573474782506903%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwM DAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sd ata=vpn1QyX8p7IZhBi5n2iOi8ezFRPTbGk1fqlK5ZsPhYk%3D&reserved=0
where we set it to 64 or 32 bit explicitly.
Does this solve your problem: diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c index 581704e61f28..fc86e2c8c42b 100644 --- a/drivers/ata/libahci_platform.c +++ b/drivers/ata/libahci_platform.c @@ -747,12 +747,11 @@ int ahci_platform_init_host(struct platform_device *pdev, ap->ops = &ata_dummy_port_ops; }
if (hpriv->cap & HOST_CAP_64) {rc = dma_coerce_mask_and_coherent(dev,DMA_BIT_MASK(64));
if (rc) {dev_err(dev, "Failed to enable 64-bit DMA.\n");return rc;}
rc = dma_coerce_mask_and_coherent(dev,DMA_BIT_MASK((hpriv->cap & HOST_CAP_64) ? 64 :32));
if (rc) {dev_err(dev, "DMA enable failed\n");return rc; } rc = ahci_reset_controller(host);
Hi Niklas: I'm so sorry to reply late. About the 32bit DMA limitation of i.MX8QM AHCI SATA. It's seems that one "dma-ranges" property in the DT can let i.MX8QM SATA works fine in my past days tests without this commit. How about drop these driver changes, and add "dma-ranges" for i.MX8QM SATA? Thanks a lot for your kindly help.
Best Regards Richard Zhu
Kind regards, Niklas
On Fri, Aug 02, 2024 at 02:30:45AM +0000, Hongxing Zhu wrote:
Does this solve your problem: diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c index 581704e61f28..fc86e2c8c42b 100644 --- a/drivers/ata/libahci_platform.c +++ b/drivers/ata/libahci_platform.c @@ -747,12 +747,11 @@ int ahci_platform_init_host(struct platform_device *pdev, ap->ops = &ata_dummy_port_ops; }
if (hpriv->cap & HOST_CAP_64) {rc = dma_coerce_mask_and_coherent(dev,DMA_BIT_MASK(64));
if (rc) {dev_err(dev, "Failed to enable 64-bit DMA.\n");return rc;}
rc = dma_coerce_mask_and_coherent(dev,DMA_BIT_MASK((hpriv->cap & HOST_CAP_64) ? 64 :32));
if (rc) {dev_err(dev, "DMA enable failed\n");return rc; } rc = ahci_reset_controller(host);Hi Niklas: I'm so sorry to reply late. About the 32bit DMA limitation of i.MX8QM AHCI SATA. It's seems that one "dma-ranges" property in the DT can let i.MX8QM SATA works fine in my past days tests without this commit. How about drop these driver changes, and add "dma-ranges" for i.MX8QM SATA? Thanks a lot for your kindly help.
Hello Richard,
did you try my suggested patch above?
If you look at dma-ranges: https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetre...
"dma-ranges" property should be used on a bus device node (such as PCI host bridges).
It does not seem correct to add this property (describing the DMA limit of the AHCI controller, a PCI endpoint) on the PCI host bridge/controller.
This property belongs to the AHCI controller, not the upstream PCI host bridge/controller.
AHCI has a specific register to describe if the hardware can support 64-bit DMA addresses or not, so if my suggested patch works for you, it seems like a more elegant solution (which also avoids having to abuse device tree properties).
Kind regards, Niklas
-----Original Message----- From: Niklas Cassel cassel@kernel.org Sent: 2024年8月8日 6:35 To: Hongxing Zhu hongxing.zhu@nxp.com Cc: tj@kernel.org; dlemoal@kernel.org; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com; linux-ide@vger.kernel.org; stable@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; imx@lists.linux.dev; kernel@pengutronix.de Subject: Re: [PATCH v4 4/6] ata: ahci_imx: Add 32bits DMA limit for i.MX8QM AHCI SATA
On Fri, Aug 02, 2024 at 02:30:45AM +0000, Hongxing Zhu wrote:
Does this solve your problem: diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c index 581704e61f28..fc86e2c8c42b 100644 --- a/drivers/ata/libahci_platform.c +++ b/drivers/ata/libahci_platform.c @@ -747,12 +747,11 @@ int ahci_platform_init_host(struct platform_device *pdev, ap->ops = &ata_dummy_port_ops; }
if (hpriv->cap & HOST_CAP_64) {rc = dma_coerce_mask_and_coherent(dev,DMA_BIT_MASK(64));
if (rc) {dev_err(dev, "Failed to enable 64-bit DMA.\n");return rc;}
rc = dma_coerce_mask_and_coherent(dev,DMA_BIT_MASK((hpriv->cap & HOST_CAP_64) ?64 :
32));
if (rc) {dev_err(dev, "DMA enable failed\n");return rc; } rc = ahci_reset_controller(host);Hi Niklas: I'm so sorry to reply late. About the 32bit DMA limitation of i.MX8QM AHCI SATA. It's seems that one "dma-ranges" property in the DT can let i.MX8QM SATA works fine in my past days tests without this commit. How about drop these driver changes, and add "dma-ranges" for i.MX8QM
SATA?
Thanks a lot for your kindly help.
Hello Richard,
did you try my suggested patch above?
If you look at dma-ranges: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdevicetre e-specification.readthedocs.io%2Fen%2Flatest%2Fchapter2-devicetree-basics.ht ml%23dma-ranges&data=05%7C02%7Chongxing.zhu%40nxp.com%7C97f87f99 37844f97dbf508dcb731345e%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0 %7C0%7C638586669175980044%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC 4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7 C%7C&sdata=PO0bQVUA7o47HAecOp27TKwwNiG1ydfQq4DTtgEva%2Fg%3D& reserved=0
"dma-ranges" property should be used on a bus device node (such as PCI host bridges).
It does not seem correct to add this property (describing the DMA limit of the AHCI controller, a PCI endpoint) on the PCI host bridge/controller.
This property belongs to the AHCI controller, not the upstream PCI host bridge/controller.
AHCI has a specific register to describe if the hardware can support 64-bit DMA addresses or not, so if my suggested patch works for you, it seems like a more elegant solution (which also avoids having to abuse device tree properties).
Hi Niklas: Thank you very much for your kindly reply. In i.MX8QM, both AHCI and PCIe are contained in HSIO(High Speed IO) module. The "dma-ranges" property is defined in HSIO dts node actually.
BTW, I had tried the method you mentioned before. Unfortunately, it doesn't work for i.MX8QM AHCI.
Best Regards Richard Zhu
Kind regards, Niklas
On Thu, Aug 08, 2024 at 12:35:01AM +0200, Niklas Cassel wrote:
On Fri, Aug 02, 2024 at 02:30:45AM +0000, Hongxing Zhu wrote:
Does this solve your problem: diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c index 581704e61f28..fc86e2c8c42b 100644 --- a/drivers/ata/libahci_platform.c +++ b/drivers/ata/libahci_platform.c @@ -747,12 +747,11 @@ int ahci_platform_init_host(struct platform_device *pdev, ap->ops = &ata_dummy_port_ops; }
if (hpriv->cap & HOST_CAP_64) {rc = dma_coerce_mask_and_coherent(dev,DMA_BIT_MASK(64));
if (rc) {dev_err(dev, "Failed to enable 64-bit DMA.\n");return rc;}
rc = dma_coerce_mask_and_coherent(dev,DMA_BIT_MASK((hpriv->cap & HOST_CAP_64) ? 64 :32));
if (rc) {dev_err(dev, "DMA enable failed\n");return rc; } rc = ahci_reset_controller(host);Hi Niklas: I'm so sorry to reply late. About the 32bit DMA limitation of i.MX8QM AHCI SATA. It's seems that one "dma-ranges" property in the DT can let i.MX8QM SATA works fine in my past days tests without this commit. How about drop these driver changes, and add "dma-ranges" for i.MX8QM SATA? Thanks a lot for your kindly help.
Hello Richard,
did you try my suggested patch above?
If you look at dma-ranges: https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetre...
"dma-ranges" property should be used on a bus device node (such as PCI host bridges).
Yes, 32bit is limited by internal bus farbic, not AHCI controller.
It look like hsio-bus { dma-ranges = <low 4G space> sata@addr { ... } }
This should be correct reflect hardware behavior. force ahci to 32bit dma mask should be a wordaround.
Frank
It does not seem correct to add this property (describing the DMA limit of the AHCI controller, a PCI endpoint) on the PCI host bridge/controller.
This property belongs to the AHCI controller, not the upstream PCI host bridge/controller.
AHCI has a specific register to describe if the hardware can support 64-bit DMA addresses or not, so if my suggested patch works for you, it seems like a more elegant solution (which also avoids having to abuse device tree properties).
Kind regards, Niklas
On Thu, Aug 08, 2024 at 10:03:17AM -0400, Frank Li wrote:
On Thu, Aug 08, 2024 at 12:35:01AM +0200, Niklas Cassel wrote:
On Fri, Aug 02, 2024 at 02:30:45AM +0000, Hongxing Zhu wrote:
Hi Niklas: I'm so sorry to reply late. About the 32bit DMA limitation of i.MX8QM AHCI SATA. It's seems that one "dma-ranges" property in the DT can let i.MX8QM SATA works fine in my past days tests without this commit. How about drop these driver changes, and add "dma-ranges" for i.MX8QM SATA? Thanks a lot for your kindly help.
Hello Richard,
did you try my suggested patch above?
If you look at dma-ranges: https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetre...
"dma-ranges" property should be used on a bus device node (such as PCI host bridges).
Yes, 32bit is limited by internal bus farbic, not AHCI controller.
If the limit is by the interconnect/bus, then the limit will affect all devices connected to that bus, i.e. both the PCIe controller and the AHCI controller, and using "dma-ranges" in that case is of course correct.
I guess I'm mostly surprised that i.MX8QM doesn't already have this property defined in its device tree.
Anyway, please send a v5 of this series without the patch in $subject, and we should be able to queue it up for 6.12.
Kind regards, Niklas
-----Original Message----- From: Niklas Cassel cassel@kernel.org Sent: 2024年8月9日 0:24 To: Frank Li frank.li@nxp.com Cc: Hongxing Zhu hongxing.zhu@nxp.com; tj@kernel.org; dlemoal@kernel.org; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com; linux-ide@vger.kernel.org; stable@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; imx@lists.linux.dev; kernel@pengutronix.de Subject: Re: [PATCH v4 4/6] ata: ahci_imx: Add 32bits DMA limit for i.MX8QM AHCI SATA
On Thu, Aug 08, 2024 at 10:03:17AM -0400, Frank Li wrote:
On Thu, Aug 08, 2024 at 12:35:01AM +0200, Niklas Cassel wrote:
On Fri, Aug 02, 2024 at 02:30:45AM +0000, Hongxing Zhu wrote:
Hi Niklas: I'm so sorry to reply late. About the 32bit DMA limitation of i.MX8QM AHCI SATA. It's seems that one "dma-ranges" property in the DT can let i.MX8QM SATA works fine in my past days tests without this commit. How about drop these driver changes, and add "dma-ranges" for i.MX8QM
SATA?
Thanks a lot for your kindly help.
Hello Richard,
did you try my suggested patch above?
If you look at dma-ranges: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fde vicetree-specification.readthedocs.io%2Fen%2Flatest%2Fchapter2-devic
etree-basics.html%23dma-ranges&data=05%7C02%7Chongxing.zhu%40nxp.co m
%7Cb18159f7aa424f3e0d8f08dcb7c68f38%7C686ea1d3bc2b4c6fa92cd99c
5c3016
35%7C0%7C0%7C638587310631122078%7CUnknown%7CTWFpbGZsb3d8eyJ WIjoiMC4w
LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C% 7C&
sdata=uYJFF7Mci4j068LF1Ew8qrFvc1pzrmWhJS0UqaWtvYQ%3D&reserved=0
"dma-ranges" property should be used on a bus device node (such as PCI host bridges).
Yes, 32bit is limited by internal bus farbic, not AHCI controller.
If the limit is by the interconnect/bus, then the limit will affect all devices connected to that bus, i.e. both the PCIe controller and the AHCI controller, and using "dma-ranges" in that case is of course correct.
I guess I'm mostly surprised that i.MX8QM doesn't already have this property defined in its device tree.
Anyway, please send a v5 of this series without the patch in $subject, and we should be able to queue it up for 6.12.
Hi Niklas: Thank you very much. I had already sent out the v5 series patch-set a few days ago. https://patchwork.kernel.org/project/linux-arm-kernel/cover/1722581213-15221... BTW, I'm a little confused about " without the patch in $subject,". Do you mean to remove the "PATCH" from Subject of each patch? v5: Subject: [PATCH v5 1/5] dt-bindings: ata: Add i.MX8QM AHCI compatible string v6 without patch in $subject: Subject: [ v6 1/5] dt-bindings: ata: Add i.MX8QM AHCI compatible string If yes, I can do that and add Frank's reviewed-by tag in the v6 series.
Best Regards Richard Zhu
Kind regards, Niklas
On Fri, Aug 09, 2024 at 08:45:12AM +0000, Hongxing Zhu wrote:
Hi Niklas: Thank you very much. I had already sent out the v5 series patch-set a few days ago. https://patchwork.kernel.org/project/linux-arm-kernel/cover/1722581213-15221...
Well, your V5 was not sent to libata (linux-ide) mailing list, and libata maintainers were not in "To:" or "Cc:".
BTW, I'm a little confused about " without the patch in $subject,". Do you mean to remove the "PATCH" from Subject of each patch? v5: Subject: [PATCH v5 1/5] dt-bindings: ata: Add i.MX8QM AHCI compatible string v6 without patch in $subject: Subject: [ v6 1/5] dt-bindings: ata: Add i.MX8QM AHCI compatible string If yes, I can do that and add Frank's reviewed-by tag in the v6 series.
I simply meant drop patch: [PATCH v4 4/6] ata: ahci_imx: Add 32bits DMA limit for i.MX8QM AHCI SATA and send the series as a V5.
But considering that you sent a V5 already (to the wrong list), please send a V6 to the correct list, with Reviewed-by tags from V5 picked up.
Kind regards, Niklas
-----Original Message----- From: Niklas Cassel cassel@kernel.org Sent: 2024年8月9日 21:48 To: Hongxing Zhu hongxing.zhu@nxp.com Cc: Frank Li frank.li@nxp.com; tj@kernel.org; dlemoal@kernel.org; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com; linux-ide@vger.kernel.org; stable@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; imx@lists.linux.dev; kernel@pengutronix.de Subject: Re: [PATCH v4 4/6] ata: ahci_imx: Add 32bits DMA limit for i.MX8QM AHCI SATA
On Fri, Aug 09, 2024 at 08:45:12AM +0000, Hongxing Zhu wrote:
Hi Niklas: Thank you very much. I had already sent out the v5 series patch-set a few days ago. https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
hwork.kernel.org%2Fproject%2Flinux-arm-kernel%2Fcover%2F1722581213-15 2
21-1-git-send-email-hongxing.zhu%40nxp.com%2F&data=05%7C02%7Chongxin g.
zhu%40nxp.com%7C0dbdfe3d113a4cf2f97308dcb879e27f%7C686ea1d3bc2b 4c6fa92
cd99c5c301635%7C0%7C0%7C638588080830018229%7CUnknown%7CTWFp bGZsb3d8eyJ
WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C 0%7C
%7C%7C&sdata=HkrgspjOk5lgk%2F8o%2F2hQ4RV86EVZVKbuQM2%2FeaZgB
Co%3D&rese
rved=0
Well, your V5 was not sent to libata (linux-ide) mailing list, and libata maintainers were not in "To:" or "Cc:".
BTW, I'm a little confused about " without the patch in $subject,". Do you mean to remove the "PATCH" from Subject of each patch? v5: Subject: [PATCH v5 1/5] dt-bindings: ata: Add i.MX8QM AHCI compatible string v6 without patch in $subject: Subject: [ v6 1/5] dt-bindings: ata: Add i.MX8QM AHCI compatible string If yes, I can do that and add Frank's reviewed-by tag in the v6 series.
I simply meant drop patch: [PATCH v4 4/6] ata: ahci_imx: Add 32bits DMA limit for i.MX8QM AHCI SATA and send the series as a V5.
But considering that you sent a V5 already (to the wrong list), please send a V6 to the correct list, with Reviewed-by tags from V5 picked up.
It's my fault to use a wrong send-email script to send out v5. Okay, v6 would be sent out to the correct email list. Thank you very much.
Best Regards Richard Zhu
Kind regards, Niklas
The RXWM(RxWaterMark) sets the minimum number of free location within the RX FIFO before the watermark is exceeded which in turn will cause the Transport Layer to instruct the Link Layer to transmit HOLDS to the transmitting end.
Based on the default RXWM value 0x20, RX FIFO overflow might be observed on i.MX8QM MEK board, when some Gen3 SATA disks are used.
The FIFO overflow will result in CRC error, internal error and protocol error, then the SATA link is not stable anymore.
To fix this issue, enlarge RX water mark setting from 0x20 to 0x29.
Signed-off-by: Richard Zhu hongxing.zhu@nxp.com --- drivers/ata/ahci_imx.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c index e94c0fdea2260..b6ad0fe5fbbcc 100644 --- a/drivers/ata/ahci_imx.c +++ b/drivers/ata/ahci_imx.c @@ -45,6 +45,10 @@ enum { /* Clock Reset Register */ IMX_CLOCK_RESET = 0x7f3f, IMX_CLOCK_RESET_RESET = 1 << 0, + /* IMX8QM SATA specific control registers */ + IMX8QM_SATA_AHCI_PTC = 0xc8, + IMX8QM_SATA_AHCI_PTC_RXWM_MASK = GENMASK(6, 0), + IMX8QM_SATA_AHCI_PTC_RXWM = 0x29, };
enum ahci_imx_type { @@ -466,6 +470,12 @@ static int imx8_sata_enable(struct ahci_host_priv *hpriv) phy_power_off(imxpriv->cali_phy0); phy_exit(imxpriv->cali_phy0);
+ /* RxWaterMark setting */ + val = readl(hpriv->mmio + IMX8QM_SATA_AHCI_PTC); + val &= ~IMX8QM_SATA_AHCI_PTC_RXWM_MASK; + val |= IMX8QM_SATA_AHCI_PTC_RXWM; + writel(val, hpriv->mmio + IMX8QM_SATA_AHCI_PTC); + return 0;
err_sata_phy_exit:
Correct the email address of driver author.
Signed-off-by: Richard Zhu hongxing.zhu@nxp.com --- drivers/ata/ahci_imx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c index b6ad0fe5fbbcc..965466c7a27e5 100644 --- a/drivers/ata/ahci_imx.c +++ b/drivers/ata/ahci_imx.c @@ -1039,6 +1039,6 @@ static struct platform_driver imx_ahci_driver = { module_platform_driver(imx_ahci_driver);
MODULE_DESCRIPTION("Freescale i.MX AHCI SATA platform driver"); -MODULE_AUTHOR("Richard Zhu Hong-Xing.Zhu@freescale.com"); +MODULE_AUTHOR("Richard Zhu hongxing.zhu@nxp.com"); MODULE_LICENSE("GPL"); MODULE_ALIAS("platform:" DRV_NAME);
linux-stable-mirror@lists.linaro.org
-
Frank Li -
Hongxing Zhu -
kernel test robot -
Niklas Cassel -
Richard Zhu