* Re: [PATCH v2 5/7] i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller
From: Bjorn Andersson @ 2018-01-18 5:23 UTC (permalink / raw)
To: Karthikeyan Ramasubramanian
Cc: corbet-T1hC0tSOHrs, andy.gross-QSEj5FYQhm4dnm+yROfE0A,
david.brown-QSEj5FYQhm4dnm+yROfE0A,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
wsa-z923LK4zBo2bacvFa/9K2g,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-serial-u79uwXL29TY76Z2rM5mHXA, jslaby-IBi9RG/b67k,
Sagar Dharia, Girish Mahadevan
In-Reply-To: <1515805547-22816-6-git-send-email-kramasub-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
On Fri 12 Jan 17:05 PST 2018, Karthikeyan Ramasubramanian wrote:
> This bus driver supports the GENI based i2c hardware controller in the
> Qualcomm SOCs. The Qualcomm Generic Interface (GENI) is a programmable
> module supporting a wide range of serial interfaces including I2C. The
> driver supports FIFO mode and DMA mode of transfer and switches modes
> dynamically depending on the size of the transfer.
>
> Signed-off-by: Karthikeyan Ramasubramanian <kramasub-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Signed-off-by: Sagar Dharia <sdharia-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> Signed-off-by: Girish Mahadevan <girishm-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> ---
> drivers/i2c/busses/Kconfig | 10 +
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-qcom-geni.c | 656 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 667 insertions(+)
> create mode 100644 drivers/i2c/busses/i2c-qcom-geni.c
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 009345d..caef309 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -838,6 +838,16 @@ config I2C_PXA_SLAVE
> is necessary for systems where the PXA may be a target on the
> I2C bus.
>
> +config I2C_QCOM_GENI
> + tristate "Qualcomm Technologies Inc.'s GENI based I2C controller"
> + depends on ARCH_QCOM
Just depend on the GENI wrapper as well.
> + help
> + If you say yes to this option, support will be included for the
> + built-in I2C interface on the Qualcomm Technologies Inc.'s SoCs.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-qcom-geni.
> +
> config I2C_QUP
> tristate "Qualcomm QUP based I2C controller"
> depends on ARCH_QCOM
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 2ce8576..201fce1 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -84,6 +84,7 @@ obj-$(CONFIG_I2C_PNX) += i2c-pnx.o
> obj-$(CONFIG_I2C_PUV3) += i2c-puv3.o
> obj-$(CONFIG_I2C_PXA) += i2c-pxa.o
> obj-$(CONFIG_I2C_PXA_PCI) += i2c-pxa-pci.o
> +obj-$(CONFIG_I2C_QCOM_GENI) += i2c-qcom-geni.o
> obj-$(CONFIG_I2C_QUP) += i2c-qup.o
> obj-$(CONFIG_I2C_RIIC) += i2c-riic.o
> obj-$(CONFIG_I2C_RK3X) += i2c-rk3x.o
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> new file mode 100644
> index 0000000..59ad4da
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -0,0 +1,656 @@
> +/*
> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
Use SPDX license header.
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
Unused?
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/qcom-geni-se.h>
> +
> +#define SE_I2C_TX_TRANS_LEN (0x26C)
Drop the parenthesis, when not needed.
> +#define SE_I2C_RX_TRANS_LEN (0x270)
> +#define SE_I2C_SCL_COUNTERS (0x278)
> +#define SE_GENI_IOS (0x908)
> +
> +#define SE_I2C_ERR (M_CMD_OVERRUN_EN | M_ILLEGAL_CMD_EN | M_CMD_FAILURE_EN |\
> + M_GP_IRQ_1_EN | M_GP_IRQ_3_EN | M_GP_IRQ_4_EN)
> +#define SE_I2C_ABORT (1U << 1)
> +/* M_CMD OP codes for I2C */
> +#define I2C_WRITE (0x1)
> +#define I2C_READ (0x2)
> +#define I2C_WRITE_READ (0x3)
> +#define I2C_ADDR_ONLY (0x4)
> +#define I2C_BUS_CLEAR (0x6)
> +#define I2C_STOP_ON_BUS (0x7)
> +/* M_CMD params for I2C */
> +#define PRE_CMD_DELAY (BIT(0))
> +#define TIMESTAMP_BEFORE (BIT(1))
> +#define STOP_STRETCH (BIT(2))
> +#define TIMESTAMP_AFTER (BIT(3))
> +#define POST_COMMAND_DELAY (BIT(4))
> +#define IGNORE_ADD_NACK (BIT(6))
> +#define READ_FINISHED_WITH_ACK (BIT(7))
> +#define BYPASS_ADDR_PHASE (BIT(8))
> +#define SLV_ADDR_MSK (GENMASK(15, 9))
> +#define SLV_ADDR_SHFT (9)
> +
> +#define I2C_CORE2X_VOTE (10000)
> +#define GP_IRQ0 0
> +#define GP_IRQ1 1
> +#define GP_IRQ2 2
> +#define GP_IRQ3 3
> +#define GP_IRQ4 4
> +#define GP_IRQ5 5
> +#define GENI_OVERRUN 6
> +#define GENI_ILLEGAL_CMD 7
> +#define GENI_ABORT_DONE 8
> +#define GENI_TIMEOUT 9
> +
> +#define I2C_NACK GP_IRQ1
> +#define I2C_BUS_PROTO GP_IRQ3
> +#define I2C_ARB_LOST GP_IRQ4
> +#define DM_I2C_CB_ERR ((BIT(GP_IRQ1) | BIT(GP_IRQ3) | BIT(GP_IRQ4)) \
> + << 5)
> +
> +#define I2C_AUTO_SUSPEND_DELAY 250
> +#define KHz(freq) (1000 * freq)
> +
> +struct geni_i2c_dev {
> + struct device *dev;
> + void __iomem *base;
> + unsigned int tx_wm;
> + int irq;
> + int err;
> + struct i2c_adapter adap;
> + struct completion xfer;
How about naming this "done" or something like that, gi2c->xfer doesn't
really give the sense of being a "we're done with the operation"-event.
> + struct i2c_msg *cur;
> + struct geni_se_rsc i2c_rsc;
> + int cur_wr;
> + int cur_rd;
> + struct device *wrapper_dev;
This is already availabe in i2c_rsc, and in particular if you pass the
i2c_rsc down to the wrapper in the 2 cases you use the wrapper_dev you
don't need this duplication.
> + u32 clk_freq_out;
> + int clk_fld_idx;
Keep track of the const struct geni_i2c_clk_fld * here instead.
> +};
> +
> +struct geni_i2c_err_log {
> + int err;
> + const char *msg;
> +};
> +
> +static struct geni_i2c_err_log gi2c_log[] = {
> + [GP_IRQ0] = {-EINVAL, "Unknown I2C err GP_IRQ0"},
> + [I2C_NACK] = {-ENOTCONN,
> + "NACK: slv unresponsive, check its power/reset-ln"},
Break the 80-char rule, to improve readability.
> + [GP_IRQ2] = {-EINVAL, "Unknown I2C err GP IRQ2"},
> + [I2C_BUS_PROTO] = {-EPROTO,
> + "Bus proto err, noisy/unepxected start/stop"},
> + [I2C_ARB_LOST] = {-EBUSY,
> + "Bus arbitration lost, clock line undriveable"},
> + [GP_IRQ5] = {-EINVAL, "Unknown I2C err GP IRQ5"},
> + [GENI_OVERRUN] = {-EIO, "Cmd overrun, check GENI cmd-state machine"},
> + [GENI_ILLEGAL_CMD] = {-EILSEQ,
> + "Illegal cmd, check GENI cmd-state machine"},
> + [GENI_ABORT_DONE] = {-ETIMEDOUT, "Abort after timeout successful"},
> + [GENI_TIMEOUT] = {-ETIMEDOUT, "I2C TXN timed out"},
> +};
> +
> +struct geni_i2c_clk_fld {
> + u32 clk_freq_out;
> + u8 clk_div;
> + u8 t_high;
> + u8 t_low;
> + u8 t_cycle;
> +};
> +
> +static struct geni_i2c_clk_fld geni_i2c_clk_map[] = {
const
> + {KHz(100), 7, 10, 11, 26},
> + {KHz(400), 2, 5, 12, 24},
> + {KHz(1000), 1, 3, 9, 18},
> +};
> +
> +static int geni_i2c_clk_map_idx(struct geni_i2c_dev *gi2c)
> +{
> + int i;
> + int ret = 0;
> + bool clk_map_present = false;
> + struct geni_i2c_clk_fld *itr = geni_i2c_clk_map;
> +
> + for (i = 0; i < ARRAY_SIZE(geni_i2c_clk_map); i++, itr++) {
> + if (itr->clk_freq_out == gi2c->clk_freq_out) {
> + clk_map_present = true;
> + break;
Make this:
gi2c->clk_fld = geni_i2c_clk_map + i;
return 0;
> + }
> + }
> +
...then you can drop ret and clk_map_present and just return -EINVAL
here.
> + if (clk_map_present)
> + gi2c->clk_fld_idx = i;
> + else
> + ret = -EINVAL;
> +
> + return ret;
> +}
> +
> +static inline void qcom_geni_i2c_conf(struct geni_i2c_dev *gi2c, int dfs)
Drop the "inline", if it makes sense the compiler will inline it, if not
it knows better than we do.
dfs is always 0, so drop this parameter and hard code the value below.
> +{
> + struct geni_i2c_clk_fld *itr = geni_i2c_clk_map + gi2c->clk_fld_idx;
> +
> + geni_write_reg(dfs, gi2c->base, SE_GENI_CLK_SEL);
> +
> + geni_write_reg((itr->clk_div << 4) | 1, gi2c->base, GENI_SER_M_CLK_CFG);
> + geni_write_reg(((itr->t_high << 20) | (itr->t_low << 10) |
> + itr->t_cycle), gi2c->base, SE_I2C_SCL_COUNTERS);
> +
> + /* Ensure Clk config completes before return */
That's not what "mb" does, it ensures that later memory operations
aren't reordered beyond the barrier.
> + mb();
> +}
> +
> +static void geni_i2c_err(struct geni_i2c_dev *gi2c, int err)
Looking at the code in this function it's just a bunch of logic to print
various debug messages...except for the last line that uses the gi2c_log
lookup table to convert the error to a errno. Sneaky...and not very
nice.
> +{
> + u32 m_cmd = readl_relaxed(gi2c->base + SE_GENI_M_CMD0);
Unless you have a really good reason, just use readl/writel in favour of
their _relaxed versions.
> + u32 m_stat = readl_relaxed(gi2c->base + SE_GENI_M_IRQ_STATUS);
> + u32 geni_s = readl_relaxed(gi2c->base + SE_GENI_STATUS);
> + u32 geni_ios = readl_relaxed(gi2c->base + SE_GENI_IOS);
> + u32 dma = readl_relaxed(gi2c->base + SE_GENI_DMA_MODE_EN);
> + u32 rx_st, tx_st;
> +
> + if (gi2c->cur)
> + dev_dbg(gi2c->dev, "len:%d, slv-addr:0x%x, RD/WR:%d\n",
> + gi2c->cur->len, gi2c->cur->addr, gi2c->cur->flags);
> +
> + if (err == I2C_NACK || err == GENI_ABORT_DONE) {
> + dev_dbg(gi2c->dev, "%s\n", gi2c_log[err].msg);
> + goto err_ret;
> + } else {
> + dev_err(gi2c->dev, "%s\n", gi2c_log[err].msg);
> + }
> +
> + if (dma) {
> + rx_st = readl_relaxed(gi2c->base + SE_DMA_RX_IRQ_STAT);
> + tx_st = readl_relaxed(gi2c->base + SE_DMA_TX_IRQ_STAT);
> + } else {
> + rx_st = readl_relaxed(gi2c->base + SE_GENI_RX_FIFO_STATUS);
> + tx_st = readl_relaxed(gi2c->base + SE_GENI_TX_FIFO_STATUS);
> + }
> + dev_dbg(gi2c->dev, "DMA:%d tx_stat:0x%x, rx_stat:0x%x, irq-stat:0x%x\n",
> + dma, tx_st, rx_st, m_stat);
> + dev_dbg(gi2c->dev, "m_cmd:0x%x, geni_status:0x%x, geni_ios:0x%x\n",
> + m_cmd, geni_s, geni_ios);
> +err_ret:
> + gi2c->err = gi2c_log[err].err;
> +}
> +
> +static irqreturn_t geni_i2c_irq(int irq, void *dev)
> +{
> + struct geni_i2c_dev *gi2c = dev;
> + int i, j;
> + u32 m_stat = readl_relaxed(gi2c->base + SE_GENI_M_IRQ_STATUS);
> + u32 rx_st = readl_relaxed(gi2c->base + SE_GENI_RX_FIFO_STATUS);
> + u32 dm_tx_st = readl_relaxed(gi2c->base + SE_DMA_TX_IRQ_STAT);
> + u32 dm_rx_st = readl_relaxed(gi2c->base + SE_DMA_RX_IRQ_STAT);
> + u32 dma = readl_relaxed(gi2c->base + SE_GENI_DMA_MODE_EN);
> + struct i2c_msg *cur = gi2c->cur;
> +
> + if (!cur || (m_stat & M_CMD_FAILURE_EN) ||
> + (dm_rx_st & (DM_I2C_CB_ERR)) ||
> + (m_stat & M_CMD_ABORT_EN)) {
> +
> + if (m_stat & M_GP_IRQ_1_EN)
> + geni_i2c_err(gi2c, I2C_NACK);
> + if (m_stat & M_GP_IRQ_3_EN)
> + geni_i2c_err(gi2c, I2C_BUS_PROTO);
> + if (m_stat & M_GP_IRQ_4_EN)
> + geni_i2c_err(gi2c, I2C_ARB_LOST);
> + if (m_stat & M_CMD_OVERRUN_EN)
> + geni_i2c_err(gi2c, GENI_OVERRUN);
> + if (m_stat & M_ILLEGAL_CMD_EN)
> + geni_i2c_err(gi2c, GENI_ILLEGAL_CMD);
> + if (m_stat & M_CMD_ABORT_EN)
> + geni_i2c_err(gi2c, GENI_ABORT_DONE);
> + if (m_stat & M_GP_IRQ_0_EN)
> + geni_i2c_err(gi2c, GP_IRQ0);
> +
> + if (!dma)
> + writel_relaxed(0, (gi2c->base +
> + SE_GENI_TX_WATERMARK_REG));
Drop the extra parenthesis. And using writel() instead keeps you under
80 chars.
> + goto irqret;
> + }
> +
> + if (dma) {
> + dev_dbg(gi2c->dev, "i2c dma tx:0x%x, dma rx:0x%x\n", dm_tx_st,
> + dm_rx_st);
> + goto irqret;
> + }
> +
> + if (((m_stat & M_RX_FIFO_WATERMARK_EN) ||
> + (m_stat & M_RX_FIFO_LAST_EN)) && (cur->flags & I2C_M_RD)) {
Some of these parenthesis are unnecessary, but more importantly the way
you wrap this line is misleading; the wrapping indicates that the two
latter conditionals are related, but the parenthesis says the first two
are.
The most significant part of this conditional is "is this a read
operation", so put this first.
> + u32 rxcnt = rx_st & RX_FIFO_WC_MSK;
> +
> + for (j = 0; j < rxcnt; j++) {
> + u32 temp;
> + int p;
> +
> + temp = readl_relaxed(gi2c->base + SE_GENI_RX_FIFOn);
> + for (i = gi2c->cur_rd, p = 0; (i < cur->len && p < 4);
> + i++, p++)
> + cur->buf[i] = (u8) ((temp >> (p * 8)) & 0xff);
> + gi2c->cur_rd = i;
The two-range for loop with a line wrap isn't very clean and the wrap
calls for a set of braces - which will look ugly.
How about something like this instead?
p = 0;
while (p < 4 && gi2c->cur_rd < cur->len) {
cur->buf[gi2c->cur_rd++] = temp & 0xff;
temp >>= 8;
p++;
}
> + if (gi2c->cur_rd == cur->len) {
> + dev_dbg(gi2c->dev, "FIFO i:%d,read 0x%x\n",
> + i, temp);
Who's the consumer of this debug line?
> + break;
> + }
> + }
> + } else if ((m_stat & M_TX_FIFO_WATERMARK_EN) &&
> + !(cur->flags & I2C_M_RD)) {
> + for (j = 0; j < gi2c->tx_wm; j++) {
> + u32 temp = 0;
> + int p;
> +
> + for (i = gi2c->cur_wr, p = 0; (i < cur->len && p < 4);
> + i++, p++)
> + temp |= (((u32)(cur->buf[i]) << (p * 8)));
> + writel_relaxed(temp, gi2c->base + SE_GENI_TX_FIFOn);
Same concern as above.
> + gi2c->cur_wr = i;
> + dev_dbg(gi2c->dev, "FIFO i:%d,wrote 0x%x\n", i, temp);
> + if (gi2c->cur_wr == cur->len) {
> + dev_dbg(gi2c->dev, "FIFO i2c bytes done writing\n");
> + writel_relaxed(0,
> + (gi2c->base + SE_GENI_TX_WATERMARK_REG));
The line break here is bad. checkpatch.pl --strict will help you find
these.
Also using writel() and dropping the parenthesis keeps you below 80
chars.
> + break;
> + }
> + }
> + }
> +irqret:
> + if (m_stat)
> + writel_relaxed(m_stat, gi2c->base + SE_GENI_M_IRQ_CLEAR);
Is it okay for this to be re-ordered wrt above writes?
> +
> + if (dma) {
> + if (dm_tx_st)
> + writel_relaxed(dm_tx_st, gi2c->base +
> + SE_DMA_TX_IRQ_CLR);
Use writel() and you don't have to wrap the line.
> + if (dm_rx_st)
> + writel_relaxed(dm_rx_st, gi2c->base +
> + SE_DMA_RX_IRQ_CLR);
> + /* Ensure all writes are done before returning from ISR. */
That's not what wmb does.
> + wmb();
> + }
> + /* if this is err with done-bit not set, handle that thr' timeout. */
Is "thr'" should for "through"?
> + if (m_stat & M_CMD_DONE_EN)
> + complete(&gi2c->xfer);
> + else if ((dm_tx_st & TX_DMA_DONE) || (dm_rx_st & RX_DMA_DONE))
> + complete(&gi2c->xfer);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int geni_i2c_xfer(struct i2c_adapter *adap,
> + struct i2c_msg msgs[],
> + int num)
> +{
> + struct geni_i2c_dev *gi2c = i2c_get_adapdata(adap);
> + int i, ret = 0, timeout = 0;
No need to initialize these, first use is an assignment.
> +
> + gi2c->err = 0;
> + gi2c->cur = &msgs[0];
You're assigning cur in each iteration of the loop below, why do you
need to do it here as well?
> + reinit_completion(&gi2c->xfer);
> + ret = pm_runtime_get_sync(gi2c->dev);
> + if (ret < 0) {
> + dev_err(gi2c->dev, "error turning SE resources:%d\n", ret);
> + pm_runtime_put_noidle(gi2c->dev);
> + /* Set device in suspended since resume failed */
> + pm_runtime_set_suspended(gi2c->dev);
> + return ret;
With the questionable assignment above you're leaving the function with
gi2c->cur pointing to an object that will soon be invalid.
> + }
> +
> + qcom_geni_i2c_conf(gi2c, 0);
> + dev_dbg(gi2c->dev, "i2c xfer:num:%d, msgs:len:%d,flg:%d\n",
> + num, msgs[0].len, msgs[0].flags);
Use dynamic function tracing instead of debug prints for this.
> + for (i = 0; i < num; i++) {
I suggest that you split out two functions here, one for rx-one-message
and one for tx-one-message. There are some duplication between the two,
but not too bad.
> + int stretch = (i < (num - 1));
> + u32 m_param = 0;
> + u32 m_cmd = 0;
> + dma_addr_t tx_dma = 0;
> + dma_addr_t rx_dma = 0;
> + enum geni_se_xfer_mode mode = FIFO_MODE;
No need to initialize mode, as the first use is an assignment.
> +
> + m_param |= (stretch ? STOP_STRETCH : 0);
> + m_param |= ((msgs[i].addr & 0x7F) << SLV_ADDR_SHFT);
Drop some parenthesis.
> +
> + gi2c->cur = &msgs[i];
> + mode = msgs[i].len > 32 ? SE_DMA : FIFO_MODE;
> + ret = geni_se_select_mode(gi2c->base, mode);
> + if (ret) {
> + dev_err(gi2c->dev, "%s: Error mode init %d:%d:%d\n",
> + __func__, mode, i, msgs[i].len);
> + break;
> + }
> + if (msgs[i].flags & I2C_M_RD) {
> + dev_dbg(gi2c->dev,
> + "READ,n:%d,i:%d len:%d, stretch:%d\n",
> + num, i, msgs[i].len, stretch);
> + geni_write_reg(msgs[i].len,
> + gi2c->base, SE_I2C_RX_TRANS_LEN);
> + m_cmd = I2C_READ;
> + geni_se_setup_m_cmd(gi2c->base, m_cmd, m_param);
Drop m_cmd and just write I2C_READ in the function call.
> + if (mode == SE_DMA) {
> + ret = geni_se_rx_dma_prep(gi2c->wrapper_dev,
> + gi2c->base, msgs[i].buf,
> + msgs[i].len, &rx_dma);
> + if (ret) {
> + mode = FIFO_MODE;
> + ret = geni_se_select_mode(gi2c->base,
> + mode);
> + }
> + }
> + } else {
> + dev_dbg(gi2c->dev,
> + "WRITE:n:%d,i:%d len:%d, stretch:%d, m_param:0x%x\n",
> + num, i, msgs[i].len, stretch, m_param);
> + geni_write_reg(msgs[i].len, gi2c->base,
> + SE_I2C_TX_TRANS_LEN);
> + m_cmd = I2C_WRITE;
> + geni_se_setup_m_cmd(gi2c->base, m_cmd, m_param);
> + if (mode == SE_DMA) {
> + ret = geni_se_tx_dma_prep(gi2c->wrapper_dev,
> + gi2c->base, msgs[i].buf,
> + msgs[i].len, &tx_dma);
> + if (ret) {
> + mode = FIFO_MODE;
> + ret = geni_se_select_mode(gi2c->base,
> + mode);
> + }
> + }
> + if (mode == FIFO_MODE) /* Get FIFO IRQ */
> + geni_write_reg(1, gi2c->base,
> + SE_GENI_TX_WATERMARK_REG);
> + }
> + /* Ensure FIFO write go through before waiting for Done evet */
That's not what mb does, drop it.
> + mb();
> + timeout = wait_for_completion_timeout(&gi2c->xfer, HZ);
As written you should just use "ret", but the return value of
wait_for_completion_timeout() is unsigned long, so change the type of
timeout instead.
> + if (!timeout) {
> + geni_i2c_err(gi2c, GENI_TIMEOUT);
> + gi2c->cur = NULL;
> + geni_se_abort_m_cmd(gi2c->base);
> + timeout = wait_for_completion_timeout(&gi2c->xfer, HZ);
What if it takes a fraction above HZ time to complete the previous
operation, then you might end up here with the completion completed, but
from the wrong operation.
> + }
> + gi2c->cur_wr = 0;
> + gi2c->cur_rd = 0;
> + if (mode == SE_DMA) {
> + if (gi2c->err) {
> + if (msgs[i].flags != I2C_M_RD)
> + writel_relaxed(1, gi2c->base +
> + SE_DMA_TX_FSM_RST);
> + else
> + writel_relaxed(1, gi2c->base +
> + SE_DMA_RX_FSM_RST);
> + wait_for_completion_timeout(&gi2c->xfer, HZ);
> + }
> + geni_se_rx_dma_unprep(gi2c->wrapper_dev, rx_dma,
> + msgs[i].len);
Reduce the length of gi2c->wrapper_dev here by using a local variable,
so that you get below (or close to) 80 chars.
Also, in either rx or tx cases above I see only that you prep one of
thse, and then you're unprepping both.
> + geni_se_tx_dma_unprep(gi2c->wrapper_dev, tx_dma,
> + msgs[i].len);
> + }
> + ret = gi2c->err;
> + if (gi2c->err) {
> + dev_err(gi2c->dev, "i2c error :%d\n", gi2c->err);
> + break;
> + }
> + }
> + if (ret == 0)
> + ret = num;
> +
> + pm_runtime_mark_last_busy(gi2c->dev);
> + pm_runtime_put_autosuspend(gi2c->dev);
> + gi2c->cur = NULL;
> + gi2c->err = 0;
> + dev_dbg(gi2c->dev, "i2c txn ret:%d\n", ret);
> + return ret;
> +}
[..]
> +static int geni_i2c_probe(struct platform_device *pdev)
> +{
> + struct geni_i2c_dev *gi2c;
> + struct resource *res;
> + int ret;
> +
> + gi2c = devm_kzalloc(&pdev->dev, sizeof(*gi2c), GFP_KERNEL);
> + if (!gi2c)
> + return -ENOMEM;
> +
> + gi2c->dev = &pdev->dev;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -EINVAL;
Put this right before devm_ioremap_resource() and drop the error check.
> +
> + gi2c->wrapper_dev = pdev->dev.parent;
> + gi2c->i2c_rsc.wrapper_dev = pdev->dev.parent;
As suggested in the other patch, if you have an helper function in the
wrapper driver that initializes the i2c_rsc then this could fill out the
actual wrapper, instead of just the device.
> + gi2c->i2c_rsc.se_clk = devm_clk_get(&pdev->dev, "se-clk");
> + if (IS_ERR(gi2c->i2c_rsc.se_clk)) {
> + ret = PTR_ERR(gi2c->i2c_rsc.se_clk);
> + dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret);
> + return ret;
> + }
> +
> + gi2c->base = devm_ioremap_resource(gi2c->dev, res);
> + if (IS_ERR(gi2c->base))
> + return PTR_ERR(gi2c->base);
> +
> + gi2c->i2c_rsc.geni_pinctrl = devm_pinctrl_get(&pdev->dev);
Drop all the pinctrl stuff. You might still want to call
pinctrl_pm_select_{idle,default,sleep}_state(), in the various stages of
PM state though.
> + if (IS_ERR_OR_NULL(gi2c->i2c_rsc.geni_pinctrl)) {
> + dev_err(&pdev->dev, "No pinctrl config specified\n");
> + ret = PTR_ERR(gi2c->i2c_rsc.geni_pinctrl);
> + return ret;
> + }
[..]
> +static int geni_i2c_runtime_resume(struct device *dev)
> +{
> + int ret;
> + struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
> +
> + ret = geni_se_resources_on(&gi2c->i2c_rsc);
> + if (ret)
> + return ret;
> +
> + if (!unlikely(gi2c->tx_wm)) {
Drop the unlikely()
> + int proto = geni_se_get_proto(gi2c->base);
> + int gi2c_tx_depth = geni_se_get_tx_fifo_depth(gi2c->base);
> +
> + if (unlikely(proto != I2C)) {
Has this changes since probe? Can't we verify that the proto is correct
during probe and then just trust that the hardware doesn't change
function?
> + dev_err(gi2c->dev, "Invalid proto %d\n", proto);
> + geni_se_resources_off(&gi2c->i2c_rsc);
> + return -ENXIO;
> + }
> +
> + gi2c->tx_wm = gi2c_tx_depth - 1;
> + geni_se_init(gi2c->base, gi2c->tx_wm, gi2c_tx_depth);
> + geni_se_config_packing(gi2c->base, 8, 4, true);
> + dev_dbg(gi2c->dev, "i2c fifo/se-dma mode. fifo depth:%d\n",
> + gi2c_tx_depth);
> + }
> + enable_irq(gi2c->irq);
> +
> + return 0;
> +}
[..]
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:i2c_geni");
What will reference the kernel module by this alias?
> --
> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 3/7] soc: qcom: Add GENI based QUP Wrapper driver
From: Rajendra Nayak @ 2018-01-18 9:13 UTC (permalink / raw)
To: Bjorn Andersson, Karthikeyan Ramasubramanian
Cc: corbet, andy.gross, david.brown, robh+dt, mark.rutland, wsa,
gregkh, linux-doc, linux-arm-msm, devicetree, linux-i2c,
linux-serial, jslaby, Sagar Dharia, Girish Mahadevan
In-Reply-To: <20180117062002.GA6620@minitux>
[]..
>> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
>> new file mode 100644
>> index 0000000..3f43582
>> --- /dev/null
>> +++ b/drivers/soc/qcom/qcom-geni-se.c
>> @@ -0,0 +1,1016 @@
>> +/*
>> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>
> Please use SPDX style license header, i.e. this file should start with:
>
> // SPDX-License-Identifier: GPL-2.0
> /*
> * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> */
Looks like Mark Brown commented elsewhere [1] that we should use the C++
commenting style even for the Linux Foundation copyright?
[1] https://marc.info/?l=linux-clk&m=151497978626135&w=2
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply
* Re: [PATCH v6 00/36] Andes(nds32) Linux Kernel Port
From: Arnd Bergmann @ 2018-01-18 9:49 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial, Geert Uytterhoeven, Linus Walleij, Mark Rutland,
Greg KH, Guo Ren
In-Reply-To: <cover.1515766253.git.green.hu@gmail.com>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu@gmail.com> wrote:
> This is the 6th version patchset to add the Linux kernel port for Andes(nds32)
> processors. Almost all of the feedbacks from v5 patchseries has been addressed.
> Thanks to everyone who provided feedback on the previous version.
>
>
> This patchset adds core architecture support to Linux for Andestech's
> N13, N15, D15, N10, D10 processor cores.
>
> Based on the 16/32-bit AndeStar RISC-like architecture, we designed the
> configurable AndesCore series of embedded processor families. AndesCores
> range from highly performance-efficient small-footprint cores for
> microcontrollers and deeply-embedded applications to 1GHz+ cores running
> Linux, covering general-purpose N-series cores for a wide range of computing
> need, DSP-capable D-series cores for digital signal control,
> instruction-extensible E-series cores for application-specific acceleration,
> and secure S-series cores for best protection of the most valuable.
>
> The patches are based on v4.14-rc8, and can also be found in the
> following git tree:
> https://github.com/andestech/linux.git nds32-4.14-rc8-v6
>
> The build script and toolchain repositories are able to be found here:
> https://github.com/andestech/build_script.git
>
> Freely available instruction set and architecture overview documents can
> be found on the following page:
> http://www.andestech.com/product.php?cls=9
>
>
> Vincent Ren-Wei Chen and I will maintain this port. Thanks to everyone who
> helped us and contributed to it. :) Any feedback is welcome.
Hi Greentime,
I think it's time to move this to the next step towards inclusion, as you appear
to have addressed all major issues, and only smaller details remain.
This is what I'd suggest for moving forward:
- split the patches into two branches in git: one 'next' branch for
everything that
is ready to get merged in one pull request that you send to Linus, including
drivers that you have received an Ack for from the respective subsystem
maintainers, and one 'testing' branch for anything that is either
not quite ready
or that you expect to get merged through someone else's tree (e.g.
most device drivers). The 'testing' branch can merge in the 'next' branch
or get rebased on it frequently.
- Ask reviewers to send 'Acked-by' or 'Reviewed-by' tags for the patches they
are happy with, or to complain if they still see any major issues that
should prevent the series from going in. I'll go through the submission once
more myself and Ack any patches that I have actually reviewed.
- Decide what base to use for the 'next' branch, rebase it on that release one
more time and then plan to never rebase it again. This will be the branch
to send to linux-next and to Linus Torvalds. Given the current timing of the
merge window, I would suggest to base it on top of v4.16-rc1 once that
gets released, and then send it for inclusion in 4.17. Any changes you do
in the meantime would be added on top of the original set.
- Ask Stephen Rothwell to include your 'next' branch in linux-next.
(if you base on 4.16-rc1, wait until that is out).
- Submit any remaining driver patches that you do not have an 'Ack' for to
the respective subsystem maintainers for inclusion in their trees.
- Upload a gpg key (4096 bits) for your greentime@andestech.com address
to the keyservers, and arrange to have that signed by other kernel
developers. You will need to sign git tags for pull requests with your
key, and the key itself should be signed for this to work best.
- Once you have at least three signatures on your gpg key, apply for
an account on kernel.org and move your git tree there,
see https://www.kernel.org/category/faq.html. Alternatively,
host the git tree on a web-facing git server from andestech.com
(github works initially, but has a couple of shortcomings).
Arnd
^ permalink raw reply
* Re: [PATCH v6 03/36] sparc: io: To use the define of ioremap_[nocache|wc|wb] in asm-generic/io.h
From: Arnd Bergmann @ 2018-01-18 9:56 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial-u79uwXL29TY76Z2rM5mHXA, Geert Uytterhoeven,
Linus Walleij, Mark Rutland, Greg KH, Guo Ren
In-Reply-To: <790e05e9b9675b3f6ef41693a794d1f09795c151.1515766253.git.green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
>
> It will be built failed if commit id: d25ea659 is selected. This patch
> can fix this build error.
>
> Signed-off-by: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
The change is fine, but the reference to commit 'd25ea659' is not for
two reasons:
- when you rebase the tree, you will get a different ID
- the recommended format for referring to another commit is to list it with
12 digits and the one-line summary like this:
commit d25ea659bc37 ("asm-generic/io.h: move
ioremap_nocache/ioremap_uc/ioremap_wc/ioremap_…")
- Ideally you use a 'Fixes:' tag in the patch description.
You can add these lines in your .gitconfig to help you here
[alias]
fixes = show --format='Fixes: %h (\"%s\")' -s
[core]
abbrev = 12
That will give you a 'git fixes' command to output the Fixes: line
in the correct format for future bug fixes. For this particular change,
I would actually just merge the two patches into one patch that
then doesn't break anything in the first place.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v6 04/36] earlycon: add reg-offset to physical address before mapping
From: Arnd Bergmann @ 2018-01-18 10:00 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial, Geert Uytterhoeven, Linus Walleij, Mark Rutland,
Greg KH, Guo Ren
In-Reply-To: <030b9e2ee2d6ee309eebe00feea0e92687304e64.1515766253.git.green.hu@gmail.com>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> It will get the wrong virtual address because port->mapbase is not added
> the correct reg-offset yet. We have to update it before earlycon_map()
> is called
>
> Signed-off-by: Greentime Hu <greentime@andestech.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Cc: Peter Hurley <peter@hurleysoftware.com>
Cc: stable@vger.kernel.org
Fixes: 088da2a17619 ("of: earlycon: Initialize port fields from DT properties")
As this is a bugfix for generic code, this might be good to
have Greg pick up directly for his serial drivers tree. An Ack from
Peter Hurley would also help, as he introduced the reg-offset handling
originally.
Arnd
^ permalink raw reply
* Re: [PATCH v6 05/36] nds32: Assembly macros and definitions
From: Arnd Bergmann @ 2018-01-18 10:01 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial-u79uwXL29TY76Z2rM5mHXA, Geert Uytterhoeven,
Linus Walleij, Mark Rutland, Greg KH, Guo Ren
In-Reply-To: <d36e188c167392994a84c8f52ef0dab90b93ed8d.1515766253.git.green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
>
> This patch includes assembly macros, bit field definitions used in .S
> files across arch/nds32/.
>
> Signed-off-by: Vincent Chen <vincentc-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
> Signed-off-by: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
Acked-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v6 06/36] nds32: Kernel booting and initialization
From: Arnd Bergmann @ 2018-01-18 10:11 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial, Geert Uytterhoeven, Linus Walleij, Mark Rutland,
Greg KH, Guo Ren
In-Reply-To: <5930d2df872116555cc553284b6c111dce29e298.1515766253.git.green.hu@gmail.com>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu@gmail.com> wrote:
I had not looked at this patch in enough detail earlier, sorry about
that. It should be
easy enough to fix though.
> +#ifdef CONFIG_VGA_CONSOLE
> +struct screen_info screen_info;
> +#endif
I would assume that you can't ever have a VGA console. Just drop all
the references
here and instead send a patch to the fbdev maintainer to add the dependency
at CONFIG_VGA_CONSOLE to prevent selecting it with nds32.
> +extern struct mm_struct init_mm;
init_mm is declared in linux/mm_types.h, so you should need another declaration.
In general, you should never put 'extern' declarations in to .c files anyway.
> +
> +extern void __init early_init_devtree(void *params);
> +extern void __init early_trap_init(void);
similarly, these are declared in include/linux/of_fdt.h
> +void __init setup_arch(char **cmdline_p)
> +{
> + early_init_devtree(__atags_pointer ?
> + phys_to_virt(__atags_pointer) : __dtb_start);
The reference to '__atags_pointer' appears to be a leftover from pre-DT
days. Can you just remove that?
> +void calibrate_delay(void)
> +{
> + const int *val;
> + struct device_node *cpu = NULL;
> + cpu = of_find_compatible_node(NULL, NULL, "andestech,nds32v3");
> + val = of_get_property(cpu, "clock-frequency", NULL);
> + if (!val || !*val)
> + panic("no cpu 'clock-frequency' parameter in device tree");
> + loops_per_jiffy = be32_to_cpup(val) / HZ;
> + pr_cont("%lu.%02lu BogoMIPS (lpj=%lu)\n",
> + loops_per_jiffy / (500000 / HZ),
> + (loops_per_jiffy / (5000 / HZ)) % 100, loops_per_jiffy);
> +}
This seems very odd to me: The 'clock-frequency' property in the
cpu node should refer to the actual frequency it is running at, but that
tends to be different from the bogomips as needed by the ndelay()
function. Can you explain what is going on here?
Arnd
^ permalink raw reply
* Re: [PATCH v6 07/36] nds32: Exception handling
From: Arnd Bergmann @ 2018-01-18 10:14 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial, Geert Uytterhoeven, Linus Walleij, Mark Rutland,
Greg KH, Guo Ren
In-Reply-To: <28c2c9da4da4c8b07473e97cd4a6cb953f4b507c.1515766253.git.green.hu@gmail.com>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> This patch includes the exception/interrupt entries, pt_reg structure and
> related accessors.
>
> Signed-off-by: Vincent Chen <vincentc@andestech.com>
> Signed-off-by: Greentime Hu <greentime@andestech.com>
Here it would be good to have a more detailed explanation about the alignment
trap handling. I remember discussing it with you before, but don't remember
the exact outcome. In particular you should explain here why you need to
handle alignment traps in the first place, and what the expected defaults
are (e.g. always disabled unless a user requests it, or always enabled) and
what kind of code runs into the traps (e.g. buggy kernel code, correct
kernel code, buggy user space code etc).
Arnd
^ permalink raw reply
* Re: [PATCH v6 08/36] nds32: MMU definitions
From: Arnd Bergmann @ 2018-01-18 10:14 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial, Geert Uytterhoeven, Linus Walleij, Mark Rutland,
Greg KH, Guo Ren
In-Reply-To: <ccf70038a1f6154ef498face5fc677e4248826f9.1515766253.git.green.hu@gmail.com>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> This patch includes virtual memory layout, PHYS_OFFSET is defined as 0x0. It
> also includes the 4KB/8KB page size configurations and pte operations.
>
> Signed-off-by: Vincent Chen <vincentc@andestech.com>
> Signed-off-by: Greentime Hu <greentime@andestech.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
* Re: [PATCH v6 09/36] nds32: MMU initialization
From: Arnd Bergmann @ 2018-01-18 10:16 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial, Geert Uytterhoeven, Linus Walleij, Mark Rutland,
Greg KH, Guo Ren
In-Reply-To: <f27b75af63adeb7f543151fdc248dceea85ef0c4.1515766253.git.green.hu@gmail.com>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> This patch includes memory initializations and highmem supporting.
>
> Signed-off-by: Vincent Chen <vincentc@andestech.com>
> Signed-off-by: Greentime Hu <greentime@andestech.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
* Re: [PATCH v6 10/36] nds32: MMU fault handling and page table management
From: Arnd Bergmann @ 2018-01-18 10:16 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial, Geert Uytterhoeven, Linus Walleij, Mark Rutland,
Greg KH, Guo Ren
In-Reply-To: <baad6ca0d7c2b2d1e2d91b9bb6735b50d886de95.1515766253.git.green.hu@gmail.com>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> This patch includes page fault handler, mmap and fixup implementations.
>
> Signed-off-by: Vincent Chen <vincentc@andestech.com>
> Signed-off-by: Greentime Hu <greentime@andestech.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
* Re: [PATCH v6 11/36] nds32: Cache and TLB routines
From: Arnd Bergmann @ 2018-01-18 10:17 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial-u79uwXL29TY76Z2rM5mHXA, Geert Uytterhoeven,
Linus Walleij, Mark Rutland, Greg KH, Guo Ren
In-Reply-To: <e1f55cfd5a5384967f1c4691bc766edbd06fee1d.1515766253.git.green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
>
> This patch contains cache and TLB maintenance functions.
>
> Signed-off-by: Vincent Chen <vincentc-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
> Signed-off-by: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
Acked-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v6 12/36] nds32: Process management
From: Arnd Bergmann @ 2018-01-18 10:22 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial, Geert Uytterhoeven, Linus Walleij, Mark Rutland,
Greg KH, Guo Ren
In-Reply-To: <10820926f3c1a4757acc8f58b8e07dd0191ffd30.1515766253.git.green.hu@gmail.com>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu@gmail.com> wrote:
> +void machine_restart(char *__unused)
> +{
> + /*
> + * Clean and disable cache, and turn off interrupts
> + */
> + cpu_proc_fin();
> +
> + /*
> + * Tell the mm system that we are going to reboot -
> + * we may need it to insert some 1:1 mappings so that
> + * soft boot works.
> + */
> + setup_mm_for_reboot(reboot_mode);
> +
> + /*
> + * Now call the architecture specific reboot code.
> + */
> + arch_reset(reboot_mode);
> +
> + /*
> + * Whoops - the architecture was unable to reboot.
> + * Tell the user!
> + */
> + mdelay(1000);
> + pr_info("Reboot failed -- System halted\n");
> + while (1) ;
> +}
You should insert a call to do_kernel_restart() in this function to allow e.g. a
watchdog driver to provide a machine-specific reboot method. Otherwise
the patch looks good to me,
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
* Re: [PATCH v6 13/36] nds32: IRQ handling
From: Arnd Bergmann @ 2018-01-18 10:22 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial, Geert Uytterhoeven, Linus Walleij, Mark Rutland,
Greg KH, Guo Ren
In-Reply-To: <84ac81fe8ba6873e7665764d7912e79a900f0c54.1515766253.git.green.hu@gmail.com>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> This patch includes irq related functions and irqchip_init().
>
> Signed-off-by: Vincent Chen <vincentc@andestech.com>
> Signed-off-by: Greentime Hu <greentime@andestech.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
* Re: [PATCH v6 14/36] nds32: Atomic operations
From: Arnd Bergmann @ 2018-01-18 10:23 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial, Geert Uytterhoeven, Linus Walleij, Mark Rutland,
Greg KH, Guo Ren
In-Reply-To: <7f2b842dc182f3c042897cb6faf427ce0537a805.1515766253.git.green.hu@gmail.com>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> This patch includes the atomic and futex operations. Many atomic operations use
> the load-lock word(llw) and store-condition word(scw) operations.
>
> Signed-off-by: Vincent Chen <vincentc@andestech.com>
> Signed-off-by: Greentime Hu <greentime@andestech.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
* Re: [PATCH v6 15/36] nds32: Device specific operations
From: Arnd Bergmann @ 2018-01-18 10:25 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial, Geert Uytterhoeven, Linus Walleij, Mark Rutland,
Greg KH, Guo Ren
In-Reply-To: <b19fe719ae9d56901fbf7670d9d7fd6aa035c355.1515766253.git.green.hu@gmail.com>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> This patch introduces ioremap implementations.
>
> Signed-off-by: Vincent Chen <vincentc@andestech.com>
> Signed-off-by: Greentime Hu <greentime@andestech.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
* Re: [PATCH v6 16/36] nds32: DMA mapping API
From: Arnd Bergmann @ 2018-01-18 10:26 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial, Geert Uytterhoeven, Linus Walleij, Mark Rutland,
Greg KH, Guo Ren
In-Reply-To: <ecd1742c718c5b45983486cf9dfb44adb6e06923.1515766253.git.green.hu@gmail.com>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> This patch adds support for the DMA mapping API. It uses dma_map_ops for
> flexibility.
>
> Signed-off-by: Vincent Chen <vincentc@andestech.com>
> Signed-off-by: Greentime Hu <greentime@andestech.com>
I'm still unhappy about the way the cache flushes are done here as discussed
before. It's not a show-stopped, but no Ack from me.
Arnd
^ permalink raw reply
* Re: [PATCH v6 17/36] nds32: ELF definitions
From: Arnd Bergmann @ 2018-01-18 10:27 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial, Geert Uytterhoeven, Linus Walleij, Mark Rutland,
Greg KH, Guo Ren
In-Reply-To: <14df961e510b69c9db304dd2d51a02b6fd307436.1515766253.git.green.hu@gmail.com>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> This patch adds definitions for the ELF format, relocation types, vdso
> locations and EXEC_PAGESIZE.
>
> Signed-off-by: Vincent Chen <vincentc@andestech.com>
> Signed-off-by: Greentime Hu <greentime@andestech.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
* Re: [PATCH v6 18/36] nds32: System calls handling
From: Arnd Bergmann @ 2018-01-18 10:27 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial-u79uwXL29TY76Z2rM5mHXA, Geert Uytterhoeven,
Linus Walleij, Mark Rutland, Greg KH, Guo Ren
In-Reply-To: <933f69aa93645513e06c6e87cc2dd48bc998343d.1515766253.git.green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
>
> This patch adds support for system calls.
>
> Signed-off-by: Vincent Chen <vincentc-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
> Signed-off-by: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
You seem to have finally addressed all my previous concerns, looks good.
Reviewed-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v6 19/36] nds32: VDSO support
From: Arnd Bergmann @ 2018-01-18 10:28 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial-u79uwXL29TY76Z2rM5mHXA, Geert Uytterhoeven,
Linus Walleij, Mark Rutland, Greg KH, Guo Ren
In-Reply-To: <4a602db0a58cc858515c3c669d5ac34c567b061c.1515766253.git.green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
>
> This patch adds VDSO support. The VDSO code is currently used for
> sys_rt_sigreturn() and optimised gettimeofday() (using the SoC timer counter).
>
> Signed-off-by: Vincent Chen <vincentc-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
> Signed-off-by: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
Acked-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v6 20/36] nds32: Signal handling support
From: Arnd Bergmann @ 2018-01-18 10:30 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial, Geert Uytterhoeven, Linus Walleij, Mark Rutland,
Greg KH, Guo Ren
In-Reply-To: <e145c39b3c50b3aee05bc60ad0117f1ec7cc5153.1515766253.git.green.hu@gmail.com>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> This patch adds support for signal handling.
>
> Signed-off-by: Vincent Chen <vincentc@andestech.com>
> Signed-off-by: Greentime Hu <greentime@andestech.com>
I never feel qualified enough to properly review signal handling code, so
no Ack from me for this code even though I don't see anything wrong with it.
Hopefully someone else can give an Ack after looking more closely.
Arnd
^ permalink raw reply
* Re: [PATCH v6 21/36] nds32: Library functions
From: Arnd Bergmann @ 2018-01-18 10:31 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial-u79uwXL29TY76Z2rM5mHXA, Geert Uytterhoeven,
Linus Walleij, Mark Rutland, Greg KH, Guo Ren
In-Reply-To: <6e8431d52d635f077ca49b2f1f7dd905bdf27a3c.1515766253.git.green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
>
> This patch add support for various library functions.
>
> Signed-off-by: Vincent Chen <vincentc-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
> Signed-off-by: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
Acked-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v6 22/36] nds32: Debugging support
From: Arnd Bergmann @ 2018-01-18 10:37 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial-u79uwXL29TY76Z2rM5mHXA, Geert Uytterhoeven,
Linus Walleij, Mark Rutland, Greg KH, Guo Ren
In-Reply-To: <65ae3b5b7eabe8680857b1821c72127f611c950b.1515766253.git.green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
>
> This patch adds ptrace support.
>
> Signed-off-by: Vincent Chen <vincentc-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
> Signed-off-by: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
I must have missed this patch earlier, unfortunately I don't think
this is ready:
> +long arch_ptrace(struct task_struct *child, long request, unsigned long addr,
> + unsigned long data)
> +{
> + int ret;
> +
> + switch (request) {
> + case PTRACE_PEEKUSR:
> + ret =
> + ptrace_read_user(child, addr, (unsigned long __user *)data);
> + break;
> +
> + case PTRACE_POKEUSR:
> + ret = ptrace_write_user(child, addr, data);
> + break;
> +
> + case PTRACE_GETREGS:
> + ret = ptrace_getregs(child, (void __user *)data);
> + break;
> +
> + case PTRACE_SETREGS:
> + ret = ptrace_setregs(child, (void __user *)data);
> + break;
> +
> + case PTRACE_GETFPREGS:
> + ret = ptrace_getfpregs(child, (void __user *)data);
> + break;
> +
> + case PTRACE_SETFPREGS:
> + ret = ptrace_setfpregs(child, (void __user *)data);
> + break;
> +
> + default:
> + ret = ptrace_request(child, request, addr, data);
> + break;
> + }
> +
> + return ret;
> +}
It appears that you are implementing the old-style ptrace handling
with architecture specific commands. Please have a look at how
this is done in risc-v or arm64. If this takes more too much time
to address, I'd suggest using an empty stub function for sys_ptrace
and adding it back at a later point, but not send the current version
upstream.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v6 23/36] nds32: L2 cache support
From: Arnd Bergmann @ 2018-01-18 10:37 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial, Geert Uytterhoeven, Linus Walleij, Mark Rutland,
Greg KH, Guo Ren
In-Reply-To: <37ba0ace68e4fbd60a8283c9d82423c0b094056e.1515766253.git.green.hu@gmail.com>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> This patch adds L2 cache support.
>
> Signed-off-by: Vincent Chen <vincentc@andestech.com>
> Signed-off-by: Greentime Hu <greentime@andestech.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
* Re: [PATCH v6 24/36] nds32: Loadable modules
From: Arnd Bergmann @ 2018-01-18 10:41 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, Networking, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial, Geert Uytterhoeven, Linus Walleij, Mark Rutland,
Greg KH, Guo Ren
In-Reply-To: <0a0789a3f776a5248ebc01706d502a3a9823cfac.1515766253.git.green.hu@gmail.com>
On Mon, Jan 15, 2018 at 6:53 AM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> This patch adds support for loadable modules.
One detail:
You still seem to have both the ELF_REL and ELF_RELA based functions
implemented here, you should drop the unused ELF_REL version:
> diff --git a/arch/nds32/kernel/module.c b/arch/nds32/kernel/module.c
> new file mode 100644
> index 0000000..714a6d6
> --- /dev/null
> +++ b/arch/nds32/kernel/module.c
> @@ -0,0 +1,286 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2005-2017 Andes Technology Corporation
> +
> +#include <linux/module.h>
> +#include <linux/elf.h>
> +#include <linux/vmalloc.h>
> +
> +#include <asm/pgtable.h>
include <linux/moduleloader.h> to catch this.
> +int
> +apply_relocate(Elf32_Shdr * sechdrs, const char *strtab,
> + unsigned int symindex, unsigned int relsec,
> + struct module *module)
> +{
> + return 0;
> +}
and drop this.
With that change,
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox