RE: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC

[Chuanhua Han]

> -----Original Message-----
> From: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> Sent: 2019年5月6日 15:48
> To: Chuanhua Han <chuanhua.han@xxxxxxx>
> Cc: shawnguo@xxxxxxxxxx; Leo Li <leoyang.li@xxxxxxx>; robh+dt@xxxxxxxxxx;
> mark.rutland@xxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> linux-i2c@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; festevam@xxxxxxxxx; dl-linux-imx
> <linux-imx@xxxxxxx>; wsa+renesas@xxxxxxxxxxxxxxxxxxxx;
> u.kleine-koenig@xxxxxxxxxxxxxx; eha@xxxxxxxx; linux@xxxxxxxxxxxxxxxx;
> l.stach@xxxxxxxxxxxxxx; peda@xxxxxxxxxx; Sumit Batra
> <sumit.batra@xxxxxxx>
> Subject: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider
> I2C_IPGCLK_SEL RCW bit when using ls1046a SoC
> 
> Caution: EXT Email
> 
> 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.
Do you mean the global variable "i2c_ipgclk_sel"？
> 
> Sascha
> 
> --
> Pengutronix e.K.                           |
> |
> Industrial Linux Solutions                 |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pe
> ngutronix.de%2F&amp;data=02%7C01%7Cchuanhua.han%40nxp.com%7C7c0
> d621ad4bb46217cf108d6d1f733e1%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0%7C0%7C636927256987992315&amp;sdata=OwDFKCv8JVyvlXrbVhRJ0
> %2FNbr5uI7WtQw92jrXyRMsg%3D&amp;reserved=0  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0
> |
> Amtsgericht Hildesheim, HRA 2686           | Fax:
> +49-5121-206917-5555 |