Hi,
In case we end up with the handling of this issue in the i2c driver,
here are the things to consider for v2.
On Tue, Apr 30, 2019 at 12:47:18PM +0800, Chuanhua Han wrote:
> The current kernel driver does not consider I2C_IPGCLK_SEL (424 bit
> of RCW) in deciding i2c_clk_rate in function i2c_imx_set_clk()
> { 0 Platform clock/4, 1 Platform clock/2}.
>
> When using ls1046a SoC, this populates incorrect value in IBFD register
> if I2C_IPGCLK_SEL = 0, which generates half of the desired Clock.
>
> Therefore, if ls1046a SoC is used, we need to set the i2c clock
> according to the corresponding RCW.
>
> Signed-off-by: Sumit Batra <sumit.batra@xxxxxxx>
> Signed-off-by: Chuanhua Han <chuanhua.han@xxxxxxx>
> ---
> drivers/i2c/busses/i2c-imx.c | 64 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 64 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 422f1a445b55..7186cf3c7d24 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -45,6 +45,8 @@
> #include <linux/pm_runtime.h>
> #include <linux/sched.h>
> #include <linux/slab.h>
> +#include <linux/fsl/guts.h>
> +#include <linux/sys_soc.h>
>
> /* This will be the driver name the kernel reports */
> #define DRIVER_NAME "imx-i2c"
> @@ -109,6 +111,21 @@
>
> #define I2C_PM_TIMEOUT 10 /* ms */
>
> +/* 14-1 Since array index starts from 0 */
> +#define RCW_I2C_IPGCLK_WORD (14 - 1)
> +/*
> + * Set mask for RCW 424th bit, reading from DCFG_CCSR RCW Status Registers
> + * Since this register in RM depicted as big endian,
> + * so consider 31st bit as LSB for creating the mask.
> + */
> +#define RCW_I2C_IPGCLK_MASK 0x800000
> +int i2c_ipgclk_sel = 1;
should be static.
> +
> +static const struct soc_device_attribute ls1046a_soc[] = {
> + {.family = "QorIQ LS1046A"},
> + { /* sentinel */ }
> +};
> +
> /*
> * sorted list of clock divider, register value pairs
> * taken from table 26-5, p.26-9, Freescale i.MX
> @@ -304,6 +321,11 @@ static const struct platform_device_id imx_i2c_devtype[] = {
> };
> MODULE_DEVICE_TABLE(platform, imx_i2c_devtype);
>
> +static const struct of_device_id guts_device_ids[] = {
> + { .compatible = "fsl,qoriq-device-config", },
> + {}
> +};
> +
> static const struct of_device_id i2c_imx_dt_ids[] = {
> { .compatible = "fsl,imx1-i2c", .data = &imx1_i2c_hwdata, },
> { .compatible = "fsl,imx21-i2c", .data = &imx21_i2c_hwdata, },
> @@ -533,6 +555,9 @@ static void i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx,
> unsigned int div;
> int i;
>
> + if (!i2c_ipgclk_sel)
> + i2c_clk_rate = i2c_clk_rate / 2;
It would be nice to have the variable inverted. You wouldn't have to
initialize a global variable with something else but 0 then.
> +
> /* Divider value calculation */
> if (i2c_imx->cur_clk == i2c_clk_rate)
> return;
> @@ -551,6 +576,10 @@ static void i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx,
> /* Store divider value */
> i2c_imx->ifdr = i2c_clk_div[i].val;
>
> + pr_alert("[%s] CLK Rate=%u Bitrate =%u Div =%u Value =%d\n",
> + __func__, i2c_clk_rate, i2c_imx->bitrate,
> + div, i2c_clk_div[i].val);
Please drop your debugging aids, for sure they shouldn't be pr_alert.
> +
> /*
> * There dummy delay is calculated.
> * It should be about one I2C clock period long.
> @@ -1116,6 +1145,9 @@ static int i2c_imx_probe(struct platform_device *pdev)
> int irq, ret;
> dma_addr_t phy_addr;
> u32 mul_value;
> + struct device_node *guts_node;
> + static struct ccsr_guts __iomem *guts_regs;
> + u32 rcw_reg;
>
> dev_dbg(&pdev->dev, "<%s>\n", __func__);
>
> @@ -1135,6 +1167,38 @@ static int i2c_imx_probe(struct platform_device *pdev)
> if (!i2c_imx)
> return -ENOMEM;
>
> + if (soc_device_match(ls1046a_soc)) {
> + /*
> + * Make device node for GUTS/DCFG (global utilities block)
> + * to read RCW.
> + */
> + guts_node = of_find_matching_node(NULL, guts_device_ids);
> + if (!guts_node) {
> + dev_err(&pdev->dev, "Could not find GUTS node\n");
> + return -ENODEV;
> + }
> + /*
> + * Memory (IO) MAP the DCFG registers(for RCW) to
> + * be used in kernel virtual address space.
> + */
> + guts_regs = of_iomap(guts_node, 0);
> + of_node_put(guts_node);
> + if (!guts_regs) {
> + dev_err(&pdev->dev, "IOREMAP of GUTS node failed\n");
> + return -ENOMEM;
> + }
> + /* Read rcw bit 424 (starting from 0) */
> + rcw_reg = ioread32be(&guts_regs->rcwsr[RCW_I2C_IPGCLK_WORD]);
> + pr_alert("RCW REG[%d]=0x%x\n", RCW_I2C_IPGCLK_WORD, rcw_reg);
> + if (rcw_reg & RCW_I2C_IPGCLK_MASK) {
> + pr_alert("Div by 2 Case Detected in RCW\n");
> + i2c_ipgclk_sel = 1;
> + } else {
> + pr_alert("Div by 4 Case Detected in RCW\n");
> + i2c_ipgclk_sel = 0;
> + }
> + }
This is done once per i2c controller, but it sets a variable valid for
all controllers. Either execute this code once outside of device
specific context or use a variable in driver data and not a global one.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
