On Sun, Sep 6, 2020 at 9:52 PM Dmitry Osipenko <digetx@xxxxxxxxx> wrote: > > Reorder location of functions in the code in order to have definition > of functions closer to the place of the invocation. This change makes > easier to navigate around the code and removes the need to have a > prototype for tegra_i2c_init(). > This patch as 25th and one I commented before have a ping-pong style of programming (you touch a lot of LOCs which you introduced in previous patches). Please try to reorder series in a way that you minimize the churn of shuffling the code. > Reviewed-by: Michał Mirosław <mirq-linux@xxxxxxxxxxxx> > Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx> > --- > drivers/i2c/busses/i2c-tegra.c | 482 ++++++++++++++++----------------- > 1 file changed, 240 insertions(+), 242 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > index 5fe138ead45e..d34b6bb295b9 100644 > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c > @@ -288,8 +288,6 @@ struct tegra_i2c_dev { > bool is_curr_atomic_xfer; > }; > > -static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev); > - > static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, u32 reg) > { > writel_relaxed(val, i2c_dev->base + reg); > @@ -463,174 +461,6 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev) > return err; > } > > -static int tegra_i2c_poll_register(struct tegra_i2c_dev *i2c_dev, > - u32 reg, u32 mask, u32 delay_us, > - u32 timeout_us) > -{ > - void __iomem *addr = i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg); > - u32 val; > - > - if (!i2c_dev->is_curr_atomic_xfer) > - return readl_relaxed_poll_timeout(addr, val, !(val & mask), > - delay_us, timeout_us); > - > - return readl_relaxed_poll_timeout_atomic(addr, val, !(val & mask), > - delay_us, timeout_us); > -} > - > -static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev) > -{ > - u32 mask, val, offset; > - int err; > - > - if (i2c_dev->hw->has_mst_fifo) { > - mask = I2C_MST_FIFO_CONTROL_TX_FLUSH | > - I2C_MST_FIFO_CONTROL_RX_FLUSH; > - offset = I2C_MST_FIFO_CONTROL; > - } else { > - mask = I2C_FIFO_CONTROL_TX_FLUSH | > - I2C_FIFO_CONTROL_RX_FLUSH; > - offset = I2C_FIFO_CONTROL; > - } > - > - val = i2c_readl(i2c_dev, offset); > - val |= mask; > - i2c_writel(i2c_dev, val, offset); > - > - err = tegra_i2c_poll_register(i2c_dev, offset, mask, 1000, 1000000); > - if (err) { > - dev_err(i2c_dev->dev, "failed to flush FIFO\n"); > - return err; > - } > - return 0; > -} > - > -static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev) > -{ > - size_t buf_remaining = i2c_dev->msg_buf_remaining; > - u8 *buf = i2c_dev->msg_buf; > - int words_to_transfer; > - int rx_fifo_avail; > - u32 val; > - > - /* > - * Catch overflow due to message fully sent > - * before the check for RX FIFO availability. > - */ > - if (WARN_ON_ONCE(!(i2c_dev->msg_buf_remaining))) > - return -EINVAL; > - > - if (i2c_dev->hw->has_mst_fifo) { > - val = i2c_readl(i2c_dev, I2C_MST_FIFO_STATUS); > - rx_fifo_avail = FIELD_GET(I2C_MST_FIFO_STATUS_RX, val); > - } else { > - val = i2c_readl(i2c_dev, I2C_FIFO_STATUS); > - rx_fifo_avail = FIELD_GET(I2C_FIFO_STATUS_RX, val); > - } > - > - /* Rounds down to not include partial word at the end of buf */ > - words_to_transfer = buf_remaining / BYTES_PER_FIFO_WORD; > - if (words_to_transfer > rx_fifo_avail) > - words_to_transfer = rx_fifo_avail; > - > - i2c_readsl(i2c_dev, buf, I2C_RX_FIFO, words_to_transfer); > - > - buf += words_to_transfer * BYTES_PER_FIFO_WORD; > - buf_remaining -= words_to_transfer * BYTES_PER_FIFO_WORD; > - rx_fifo_avail -= words_to_transfer; > - > - /* > - * If there is a partial word at the end of buf, handle it manually to > - * prevent overwriting past the end of buf > - */ > - if (rx_fifo_avail > 0 && buf_remaining > 0) { > - /* > - * buf_remaining > 3 check not needed as rx_fifo_avail == 0 > - * when (words_to_transfer was > rx_fifo_avail) earlier > - * in this function. > - */ > - val = i2c_readl(i2c_dev, I2C_RX_FIFO); > - val = cpu_to_le32(val); > - memcpy(buf, &val, buf_remaining); > - buf_remaining = 0; > - rx_fifo_avail--; > - } > - > - /* RX FIFO must be drained, otherwise it's an Overflow case. */ > - if (WARN_ON_ONCE(rx_fifo_avail)) > - return -EINVAL; > - > - i2c_dev->msg_buf_remaining = buf_remaining; > - i2c_dev->msg_buf = buf; > - > - return 0; > -} > - > -static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev) > -{ > - size_t buf_remaining = i2c_dev->msg_buf_remaining; > - u8 *buf = i2c_dev->msg_buf; > - int words_to_transfer; > - int tx_fifo_avail; > - u32 val; > - > - if (i2c_dev->hw->has_mst_fifo) { > - val = i2c_readl(i2c_dev, I2C_MST_FIFO_STATUS); > - tx_fifo_avail = FIELD_GET(I2C_MST_FIFO_STATUS_TX, val); > - } else { > - val = i2c_readl(i2c_dev, I2C_FIFO_STATUS); > - tx_fifo_avail = FIELD_GET(I2C_FIFO_STATUS_TX, val); > - } > - > - /* Rounds down to not include partial word at the end of buf */ > - words_to_transfer = buf_remaining / BYTES_PER_FIFO_WORD; > - > - /* It's very common to have < 4 bytes, so optimize that case. */ > - if (words_to_transfer) { > - if (words_to_transfer > tx_fifo_avail) > - words_to_transfer = tx_fifo_avail; > - > - /* > - * Update state before writing to FIFO. Note that this may > - * cause us to finish writing all bytes (AKA buf_remaining > - * goes to 0), hence we have a potential for an interrupt > - * (PACKET_XFER_COMPLETE is not maskable), but GIC interrupt > - * is disabled at this point. > - */ > - buf_remaining -= words_to_transfer * BYTES_PER_FIFO_WORD; > - tx_fifo_avail -= words_to_transfer; > - i2c_dev->msg_buf_remaining = buf_remaining; > - i2c_dev->msg_buf = buf + > - words_to_transfer * BYTES_PER_FIFO_WORD; > - > - i2c_writesl(i2c_dev, buf, I2C_TX_FIFO, words_to_transfer); > - > - buf += words_to_transfer * BYTES_PER_FIFO_WORD; > - } > - > - /* > - * If there is a partial word at the end of buf, handle it manually to > - * prevent reading past the end of buf, which could cross a page > - * boundary and fault. > - */ > - if (tx_fifo_avail > 0 && buf_remaining > 0) { > - /* > - * buf_remaining > 3 check not needed as tx_fifo_avail == 0 > - * when (words_to_transfer was > tx_fifo_avail) earlier > - * in this function for non-zero words_to_transfer. > - */ > - memcpy(&val, buf, buf_remaining); > - val = le32_to_cpu(val); > - > - i2c_dev->msg_buf_remaining = 0; > - i2c_dev->msg_buf = NULL; > - > - i2c_writel(i2c_dev, val, I2C_TX_FIFO); > - } > - > - return 0; > -} > - > /* > * One of the Tegra I2C blocks is inside the DVC (Digital Voltage Controller) > * block. This block is identical to the rest of the I2C blocks, except that > @@ -652,46 +482,48 @@ static void tegra_dvc_init(struct tegra_i2c_dev *i2c_dev) > dvc_writel(i2c_dev, val, DVC_CTRL_REG1); > } > > -static int __maybe_unused tegra_i2c_runtime_resume(struct device *dev) > +static void tegra_i2c_vi_init(struct tegra_i2c_dev *i2c_dev) > { > - struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev); > - int ret; > + u32 value; > > - ret = pinctrl_pm_select_default_state(i2c_dev->dev); > - if (ret) > - return ret; > + value = FIELD_PREP(I2C_INTERFACE_TIMING_THIGH, 2) | > + FIELD_PREP(I2C_INTERFACE_TIMING_TLOW, 4); > + i2c_writel(i2c_dev, value, I2C_INTERFACE_TIMING_0); > > - ret = clk_bulk_enable(i2c_dev->nclocks, i2c_dev->clocks); > - if (ret) > - return ret; > + value = FIELD_PREP(I2C_INTERFACE_TIMING_TBUF, 4) | > + FIELD_PREP(I2C_INTERFACE_TIMING_TSU_STO, 7) | > + FIELD_PREP(I2C_INTERFACE_TIMING_THD_STA, 4) | > + FIELD_PREP(I2C_INTERFACE_TIMING_TSU_STA, 4); > + i2c_writel(i2c_dev, value, I2C_INTERFACE_TIMING_1); > > - /* > - * VI I2C device is attached to VE power domain which goes through > - * power ON/OFF during PM runtime resume/suspend. So, controller > - * should go through reset and need to re-initialize after power > - * domain ON. > - */ > - if (i2c_dev->is_vi) { > - ret = tegra_i2c_init(i2c_dev); > - if (ret) > - goto disable_clocks; > - } > + value = FIELD_PREP(I2C_HS_INTERFACE_TIMING_THIGH, 3) | > + FIELD_PREP(I2C_HS_INTERFACE_TIMING_TLOW, 8); > + i2c_writel(i2c_dev, value, I2C_HS_INTERFACE_TIMING_0); > > - return 0; > + value = FIELD_PREP(I2C_HS_INTERFACE_TIMING_TSU_STO, 11) | > + FIELD_PREP(I2C_HS_INTERFACE_TIMING_THD_STA, 11) | > + FIELD_PREP(I2C_HS_INTERFACE_TIMING_TSU_STA, 11); > + i2c_writel(i2c_dev, value, I2C_HS_INTERFACE_TIMING_1); > > -disable_clocks: > - clk_bulk_disable(i2c_dev->nclocks, i2c_dev->clocks); > + value = FIELD_PREP(I2C_BC_SCLK_THRESHOLD, 9) | I2C_BC_STOP_COND; > + i2c_writel(i2c_dev, value, I2C_BUS_CLEAR_CNFG); > > - return ret; > + i2c_writel(i2c_dev, 0x0, I2C_TLOW_SEXT); > } > > -static int __maybe_unused tegra_i2c_runtime_suspend(struct device *dev) > +static int tegra_i2c_poll_register(struct tegra_i2c_dev *i2c_dev, > + u32 reg, u32 mask, u32 delay_us, > + u32 timeout_us) > { > - struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev); > + void __iomem *addr = i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg); > + u32 val; > > - clk_bulk_disable(i2c_dev->nclocks, i2c_dev->clocks); > + if (!i2c_dev->is_curr_atomic_xfer) > + return readl_relaxed_poll_timeout(addr, val, !(val & mask), > + delay_us, timeout_us); > > - return pinctrl_pm_select_idle_state(i2c_dev->dev); > + return readl_relaxed_poll_timeout_atomic(addr, val, !(val & mask), > + delay_us, timeout_us); > } > > static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev) > @@ -713,33 +545,31 @@ static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev) > return 0; > } > > -static void tegra_i2c_vi_init(struct tegra_i2c_dev *i2c_dev) > +static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev) > { > - u32 value; > - > - value = FIELD_PREP(I2C_INTERFACE_TIMING_THIGH, 2) | > - FIELD_PREP(I2C_INTERFACE_TIMING_TLOW, 4); > - i2c_writel(i2c_dev, value, I2C_INTERFACE_TIMING_0); > - > - value = FIELD_PREP(I2C_INTERFACE_TIMING_TBUF, 4) | > - FIELD_PREP(I2C_INTERFACE_TIMING_TSU_STO, 7) | > - FIELD_PREP(I2C_INTERFACE_TIMING_THD_STA, 4) | > - FIELD_PREP(I2C_INTERFACE_TIMING_TSU_STA, 4); > - i2c_writel(i2c_dev, value, I2C_INTERFACE_TIMING_1); > - > - value = FIELD_PREP(I2C_HS_INTERFACE_TIMING_THIGH, 3) | > - FIELD_PREP(I2C_HS_INTERFACE_TIMING_TLOW, 8); > - i2c_writel(i2c_dev, value, I2C_HS_INTERFACE_TIMING_0); > + u32 mask, val, offset; > + int err; > > - value = FIELD_PREP(I2C_HS_INTERFACE_TIMING_TSU_STO, 11) | > - FIELD_PREP(I2C_HS_INTERFACE_TIMING_THD_STA, 11) | > - FIELD_PREP(I2C_HS_INTERFACE_TIMING_TSU_STA, 11); > - i2c_writel(i2c_dev, value, I2C_HS_INTERFACE_TIMING_1); > + if (i2c_dev->hw->has_mst_fifo) { > + mask = I2C_MST_FIFO_CONTROL_TX_FLUSH | > + I2C_MST_FIFO_CONTROL_RX_FLUSH; > + offset = I2C_MST_FIFO_CONTROL; > + } else { > + mask = I2C_FIFO_CONTROL_TX_FLUSH | > + I2C_FIFO_CONTROL_RX_FLUSH; > + offset = I2C_FIFO_CONTROL; > + } > > - value = FIELD_PREP(I2C_BC_SCLK_THRESHOLD, 9) | I2C_BC_STOP_COND; > - i2c_writel(i2c_dev, value, I2C_BUS_CLEAR_CNFG); > + val = i2c_readl(i2c_dev, offset); > + val |= mask; > + i2c_writel(i2c_dev, val, offset); > > - i2c_writel(i2c_dev, 0x0, I2C_TLOW_SEXT); > + err = tegra_i2c_poll_register(i2c_dev, offset, mask, 1000, 1000000); > + if (err) { > + dev_err(i2c_dev->dev, "failed to flush FIFO\n"); > + return err; > + } > + return 0; > } > > static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev) > @@ -856,6 +686,132 @@ static int tegra_i2c_disable_packet_mode(struct tegra_i2c_dev *i2c_dev) > return tegra_i2c_wait_for_config_load(i2c_dev); > } > > +static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev) > +{ > + size_t buf_remaining = i2c_dev->msg_buf_remaining; > + u8 *buf = i2c_dev->msg_buf; > + int words_to_transfer; > + int rx_fifo_avail; > + u32 val; > + > + /* > + * Catch overflow due to message fully sent > + * before the check for RX FIFO availability. > + */ > + if (WARN_ON_ONCE(!(i2c_dev->msg_buf_remaining))) > + return -EINVAL; > + > + if (i2c_dev->hw->has_mst_fifo) { > + val = i2c_readl(i2c_dev, I2C_MST_FIFO_STATUS); > + rx_fifo_avail = FIELD_GET(I2C_MST_FIFO_STATUS_RX, val); > + } else { > + val = i2c_readl(i2c_dev, I2C_FIFO_STATUS); > + rx_fifo_avail = FIELD_GET(I2C_FIFO_STATUS_RX, val); > + } > + > + /* Rounds down to not include partial word at the end of buf */ > + words_to_transfer = buf_remaining / BYTES_PER_FIFO_WORD; > + if (words_to_transfer > rx_fifo_avail) > + words_to_transfer = rx_fifo_avail; > + > + i2c_readsl(i2c_dev, buf, I2C_RX_FIFO, words_to_transfer); > + > + buf += words_to_transfer * BYTES_PER_FIFO_WORD; > + buf_remaining -= words_to_transfer * BYTES_PER_FIFO_WORD; > + rx_fifo_avail -= words_to_transfer; > + > + /* > + * If there is a partial word at the end of buf, handle it manually to > + * prevent overwriting past the end of buf > + */ > + if (rx_fifo_avail > 0 && buf_remaining > 0) { > + /* > + * buf_remaining > 3 check not needed as rx_fifo_avail == 0 > + * when (words_to_transfer was > rx_fifo_avail) earlier > + * in this function. > + */ > + val = i2c_readl(i2c_dev, I2C_RX_FIFO); > + val = cpu_to_le32(val); > + memcpy(buf, &val, buf_remaining); > + buf_remaining = 0; > + rx_fifo_avail--; > + } > + > + /* RX FIFO must be drained, otherwise it's an Overflow case. */ > + if (WARN_ON_ONCE(rx_fifo_avail)) > + return -EINVAL; > + > + i2c_dev->msg_buf_remaining = buf_remaining; > + i2c_dev->msg_buf = buf; > + > + return 0; > +} > + > +static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev) > +{ > + size_t buf_remaining = i2c_dev->msg_buf_remaining; > + u8 *buf = i2c_dev->msg_buf; > + int words_to_transfer; > + int tx_fifo_avail; > + u32 val; > + > + if (i2c_dev->hw->has_mst_fifo) { > + val = i2c_readl(i2c_dev, I2C_MST_FIFO_STATUS); > + tx_fifo_avail = FIELD_GET(I2C_MST_FIFO_STATUS_TX, val); > + } else { > + val = i2c_readl(i2c_dev, I2C_FIFO_STATUS); > + tx_fifo_avail = FIELD_GET(I2C_FIFO_STATUS_TX, val); > + } > + > + /* Rounds down to not include partial word at the end of buf */ > + words_to_transfer = buf_remaining / BYTES_PER_FIFO_WORD; > + > + /* It's very common to have < 4 bytes, so optimize that case. */ > + if (words_to_transfer) { > + if (words_to_transfer > tx_fifo_avail) > + words_to_transfer = tx_fifo_avail; > + > + /* > + * Update state before writing to FIFO. Note that this may > + * cause us to finish writing all bytes (AKA buf_remaining > + * goes to 0), hence we have a potential for an interrupt > + * (PACKET_XFER_COMPLETE is not maskable), but GIC interrupt > + * is disabled at this point. > + */ > + buf_remaining -= words_to_transfer * BYTES_PER_FIFO_WORD; > + tx_fifo_avail -= words_to_transfer; > + i2c_dev->msg_buf_remaining = buf_remaining; > + i2c_dev->msg_buf = buf + > + words_to_transfer * BYTES_PER_FIFO_WORD; > + > + i2c_writesl(i2c_dev, buf, I2C_TX_FIFO, words_to_transfer); > + > + buf += words_to_transfer * BYTES_PER_FIFO_WORD; > + } > + > + /* > + * If there is a partial word at the end of buf, handle it manually to > + * prevent reading past the end of buf, which could cross a page > + * boundary and fault. > + */ > + if (tx_fifo_avail > 0 && buf_remaining > 0) { > + /* > + * buf_remaining > 3 check not needed as tx_fifo_avail == 0 > + * when (words_to_transfer was > tx_fifo_avail) earlier > + * in this function for non-zero words_to_transfer. > + */ > + memcpy(&val, buf, buf_remaining); > + val = le32_to_cpu(val); > + > + i2c_dev->msg_buf_remaining = 0; > + i2c_dev->msg_buf = NULL; > + > + i2c_writel(i2c_dev, val, I2C_TX_FIFO); > + } > + > + return 0; > +} > + > static irqreturn_t tegra_i2c_isr(int irq, void *dev_id) > { > const u32 status_err = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST; > @@ -1411,27 +1367,6 @@ static u32 tegra_i2c_func(struct i2c_adapter *adap) > return ret; > } > > -static void tegra_i2c_parse_dt(struct tegra_i2c_dev *i2c_dev) > -{ > - struct device_node *np = i2c_dev->dev->of_node; > - bool multi_mode; > - int ret; > - > - ret = of_property_read_u32(np, "clock-frequency", > - &i2c_dev->bus_clk_rate); > - if (ret) > - i2c_dev->bus_clk_rate = I2C_MAX_STANDARD_MODE_FREQ; /* default clock rate */ > - > - multi_mode = of_property_read_bool(np, "multi-master"); > - i2c_dev->is_multimaster_mode = multi_mode; > - > - if (of_device_is_compatible(np, "nvidia,tegra20-i2c-dvc")) > - i2c_dev->is_dvc = true; > - > - if (of_device_is_compatible(np, "nvidia,tegra210-i2c-vi")) > - i2c_dev->is_vi = true; > -} > - > static const struct i2c_algorithm tegra_i2c_algo = { > .master_xfer = tegra_i2c_xfer, > .master_xfer_atomic = tegra_i2c_xfer_atomic, > @@ -1637,6 +1572,27 @@ static const struct of_device_id tegra_i2c_of_match[] = { > }; > MODULE_DEVICE_TABLE(of, tegra_i2c_of_match); > > +static void tegra_i2c_parse_dt(struct tegra_i2c_dev *i2c_dev) > +{ > + struct device_node *np = i2c_dev->dev->of_node; > + bool multi_mode; > + int ret; > + > + ret = of_property_read_u32(np, "clock-frequency", > + &i2c_dev->bus_clk_rate); > + if (ret) > + i2c_dev->bus_clk_rate = I2C_MAX_STANDARD_MODE_FREQ; /* default clock rate */ > + > + multi_mode = of_property_read_bool(np, "multi-master"); > + i2c_dev->is_multimaster_mode = multi_mode; > + > + if (of_device_is_compatible(np, "nvidia,tegra20-i2c-dvc")) > + i2c_dev->is_dvc = true; > + > + if (of_device_is_compatible(np, "nvidia,tegra210-i2c-vi")) > + i2c_dev->is_vi = true; > +} > + > static int tegra_i2c_init_clocks(struct tegra_i2c_dev *i2c_dev) > { > unsigned int i; > @@ -1821,6 +1777,48 @@ static int tegra_i2c_remove(struct platform_device *pdev) > return 0; > } > > +static int __maybe_unused tegra_i2c_runtime_resume(struct device *dev) > +{ > + struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev); > + int ret; > + > + ret = pinctrl_pm_select_default_state(i2c_dev->dev); > + if (ret) > + return ret; > + > + ret = clk_bulk_enable(i2c_dev->nclocks, i2c_dev->clocks); > + if (ret) > + return ret; > + > + /* > + * VI I2C device is attached to VE power domain which goes through > + * power ON/OFF during PM runtime resume/suspend. So, controller > + * should go through reset and need to re-initialize after power > + * domain ON. > + */ > + if (i2c_dev->is_vi) { > + ret = tegra_i2c_init(i2c_dev); > + if (ret) > + goto disable_clocks; > + } > + > + return 0; > + > +disable_clocks: > + clk_bulk_disable(i2c_dev->nclocks, i2c_dev->clocks); > + > + return ret; > +} > + > +static int __maybe_unused tegra_i2c_runtime_suspend(struct device *dev) > +{ > + struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev); > + > + clk_bulk_disable(i2c_dev->nclocks, i2c_dev->clocks); > + > + return pinctrl_pm_select_idle_state(i2c_dev->dev); > +} > + > static int __maybe_unused tegra_i2c_suspend(struct device *dev) > { > struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev); > -- > 2.27.0 > -- With Best Regards, Andy Shevchenko