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 <[email protected]>
Signed-off-by: Chuanhua Han <[email protected]>
---
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;
+
+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;
+
/* 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);
+
/*
* 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;
+ }
+ }
+
if (of_id) {
i2c_imx->hwdata = of_id->data;
ret = of_property_read_u32(pdev->dev.of_node,
--
2.17.1
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.
So the clock driver reports the wrong clock. Please fix the clock driver
then.
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 |
> -----Original Message-----
> From: Sascha Hauer <[email protected]>
> Sent: 2019??4??30?? 20:51
> To: Chuanhua Han <[email protected]>
> Cc: [email protected]; Leo Li <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; dl-linux-imx
> <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Sumit Batra
> <[email protected]>
> 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
>
> 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.
>
> So the clock driver reports the wrong clock. Please fix the clock driver then.
No, this is a problem with the i2c driver. It is not a problem with the clock driver, so the i2c driver needs to be modified.
>
> Sascha
>
> --
> Pengutronix e.K. |
> |
> Industrial Linux Solutions |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pe
> ngutronix.de%2F&data=02%7C01%7Cchuanhua.han%40nxp.com%7C2bb
> a89c908fb4bd37b6708d6cd6a7ff7%7C686ea1d3bc2b4c6fa92cd99c5c301635
> %7C0%7C0%7C636922254625472037&sdata=eC4bGDNAOhEu24xt9F0h
> kxE%2B1ffooCZ4CUr4o0gQGD4%3D&reserved=0 |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0
> |
> Amtsgericht Hildesheim, HRA 2686 | Fax:
> +49-5121-206917-5555 |
On Sat, May 04, 2019 at 09:28:48AM +0000, Chuanhua Han wrote:
>
>
> > -----Original Message-----
> > From: Sascha Hauer <[email protected]>
> > Sent: 2019年4月30日 20:51
> > To: Chuanhua Han <[email protected]>
> > Cc: [email protected]; Leo Li <[email protected]>; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected]; dl-linux-imx
> > <[email protected]>; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; Sumit Batra
> > <[email protected]>
> > 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
> >
> > 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.
> >
> > So the clock driver reports the wrong clock. Please fix the clock driver then.
> No, this is a problem with the i2c driver. It is not a problem with
> the clock driver, so the i2c driver needs to be modified.
So how does this RCW bit get evaluated? According to the reference
manual only one clock goes to the i2c module (described as 1/2 Platform
Clock) and the i2c module only takes one clock. So it seems there must
be a /2 divider somewhere, either in each i2c module or somewhere
outside. Can your IC guys tell you where it is?
One reason I suggested the clock driver is that the clock driver
contains SoC specific code already, so it should be easier to integrate
there.
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 |
> -----Original Message-----
> From: Sascha Hauer <[email protected]>
> Sent: 2019年5月6日 15:38
> To: Chuanhua Han <[email protected]>
> Cc: [email protected]; Leo Li <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; dl-linux-imx
> <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Sumit Batra
> <[email protected]>
> Subject: Re: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider
> I2C_IPGCLK_SEL RCW bit when using ls1046a SoC
>
> Caution: EXT Email
>
> On Sat, May 04, 2019 at 09:28:48AM +0000, Chuanhua Han wrote:
> >
> >
> > > -----Original Message-----
> > > From: Sascha Hauer <[email protected]>
> > > Sent: 2019年4月30日 20:51
> > > To: Chuanhua Han <[email protected]>
> > > Cc: [email protected]; Leo Li <[email protected]>;
> > > [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected];
> > > [email protected]; [email protected]; dl-linux-imx
> > > <[email protected]>; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected]; [email protected];
> > > Sumit Batra <[email protected]>
> > > 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
> > >
> > > 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.
> > >
> > > So the clock driver reports the wrong clock. Please fix the clock driver then.
> > No, this is a problem with the i2c driver. It is not a problem with
> > the clock driver, so the i2c driver needs to be modified.
>
> So how does this RCW bit get evaluated? According to the reference manual
> only one clock goes to the i2c module (described as 1/2 Platform
> Clock) and the i2c module only takes one clock. So it seems there must be a /2
> divider somewhere, either in each i2c module or somewhere outside. Can your
> IC guys tell you where it is?
>
> One reason I suggested the clock driver is that the clock driver contains SoC
> specific code already, so it should be easier to integrate there.
OK, I will see that it can be qualified in the clock driver.
>
> Sascha
>
>
> --
> Pengutronix e.K. |
> |
> Industrial Linux Solutions |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pe
> ngutronix.de%2F&data=02%7C01%7Cchuanhua.han%40nxp.com%7Cb2d
> 4680699c448e8514308d6d1f5bf82%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0%7C0%7C636927250743516563&sdata=pFdCbiDXE%2FDll01X9Nj
> Hg3SCDpECzgrr8MLtYBdKH5c%3D&reserved=0 |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0
> |
> Amtsgericht Hildesheim, HRA 2686 | Fax:
> +49-5121-206917-5555 |
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 <[email protected]>
> Signed-off-by: Chuanhua Han <[email protected]>
> ---
> 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 |
> -----Original Message-----
> From: Sascha Hauer <[email protected]>
> Sent: 2019年5月6日 15:38
> To: Chuanhua Han <[email protected]>
> Cc: [email protected]; Leo Li <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; dl-linux-imx
> <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Sumit Batra
> <[email protected]>
> Subject: Re: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider
> I2C_IPGCLK_SEL RCW bit when using ls1046a SoC
>
> Caution: EXT Email
>
> On Sat, May 04, 2019 at 09:28:48AM +0000, Chuanhua Han wrote:
> >
> >
> > > -----Original Message-----
> > > From: Sascha Hauer <[email protected]>
> > > Sent: 2019年4月30日 20:51
> > > To: Chuanhua Han <[email protected]>
> > > Cc: [email protected]; Leo Li <[email protected]>;
> > > [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected];
> > > [email protected]; [email protected]; dl-linux-imx
> > > <[email protected]>; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected]; [email protected];
> > > Sumit Batra <[email protected]>
> > > 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
> > >
> > > 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.
> > >
> > > So the clock driver reports the wrong clock. Please fix the clock driver then.
> > No, this is a problem with the i2c driver. It is not a problem with
> > the clock driver, so the i2c driver needs to be modified.
>
> So how does this RCW bit get evaluated?
According to the reference manual
> only one clock goes to the i2c module (described as 1/2 Platform
> Clock) and the i2c module only takes one clock. So it seems there must be a /2
> divider somewhere, either in each i2c module or somewhere outside. Can your
> IC guys tell you where it is?
I need to confirm this with the IC team
>
> One reason I suggested the clock driver is that the clock driver contains SoC
> specific code already, so it should be easier to integrate there.
It seems inappropriate to put the clock frequency division modification of i2c in the clock driver,
because the clock driver is for all IP and is a universal code, so I think it is better to modify the clock in the IP driver.
>
> Sascha
>
>
> --
> Pengutronix e.K. |
> |
> Industrial Linux Solutions |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pe
> ngutronix.de%2F&data=02%7C01%7Cchuanhua.han%40nxp.com%7Cb2d
> 4680699c448e8514308d6d1f5bf82%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0%7C0%7C636927250743516563&sdata=pFdCbiDXE%2FDll01X9Nj
> Hg3SCDpECzgrr8MLtYBdKH5c%3D&reserved=0 |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0
> |
> Amtsgericht Hildesheim, HRA 2686 | Fax:
> +49-5121-206917-5555 |
> -----Original Message-----
> From: Sascha Hauer <[email protected]>
> Sent: 2019??5??6?? 15:48
> To: Chuanhua Han <[email protected]>
> Cc: [email protected]; Leo Li <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; dl-linux-imx
> <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Sumit Batra
> <[email protected]>
> 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 <[email protected]>
> > Signed-off-by: Chuanhua Han <[email protected]>
> > ---
> > 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&data=02%7C01%7Cchuanhua.han%40nxp.com%7C7c0
> d621ad4bb46217cf108d6d1f733e1%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0%7C0%7C636927256987992315&sdata=OwDFKCv8JVyvlXrbVhRJ0
> %2FNbr5uI7WtQw92jrXyRMsg%3D&reserved=0 |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0
> |
> Amtsgericht Hildesheim, HRA 2686 | Fax:
> +49-5121-206917-5555 |
Hi Sascha,
Please check my comment
-----Original Message-----
From: Chuanhua Han
Sent: Wednesday, May 8, 2019 5:05 PM
To: Sascha Hauer <[email protected]>
Cc: [email protected]; Leo Li <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; dl-linux-imx <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Sumit Batra <[email protected]>
Subject: RE: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC
> -----Original Message-----
> From: Sascha Hauer <[email protected]>
> Sent: 2019年5月6日 15:38
> To: Chuanhua Han <[email protected]>
> Cc: [email protected]; Leo Li <[email protected]>;
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected];
> [email protected]; [email protected]; dl-linux-imx
> <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Sumit Batra
> <[email protected]>
> Subject: Re: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't
> consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC
>
> Caution: EXT Email
>
> On Sat, May 04, 2019 at 09:28:48AM +0000, Chuanhua Han wrote:
> >
> >
> > > -----Original Message-----
> > > From: Sascha Hauer <[email protected]>
> > > Sent: 2019年4月30日 20:51
> > > To: Chuanhua Han <[email protected]>
> > > Cc: [email protected]; Leo Li <[email protected]>;
> > > [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected];
> > > [email protected]; [email protected]; dl-linux-imx
> > > <[email protected]>; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected]; [email protected];
> > > Sumit Batra <[email protected]>
> > > 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
> > >
> > > 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.
> > >
> > > So the clock driver reports the wrong clock. Please fix the clock driver then.
> > No, this is a problem with the i2c driver. It is not a problem with
> > the clock driver, so the i2c driver needs to be modified.
>
> So how does this RCW bit get evaluated?
According to the reference manual
> only one clock goes to the i2c module (described as 1/2 Platform
> Clock) and the i2c module only takes one clock. So it seems there must
> be a /2 divider somewhere, either in each i2c module or somewhere
> outside. Can your IC guys tell you where it is?
I need to confirm this with the IC team
[Sumit Batra] There are 2 places where clock division takes place -
1) There is a clock divider outside of I2C block, which makes the clock reaching I2C module as - Platform Clock/2
2) There is another clock divider which specifically divides the clock to the I2C block, based on RCW bit 424 (if 424th bit is 0 then the baud clock source is Platform Clock/4, if 424th bit is 1 then it remains Platform Clock/2)
3) Now based on the what is the desired SCL value (100KHz etc) and the clock which is received by I2C block, there is a calculation that goes on inside the I2C driver module which is used to map a value in this imx_i2c_clk_div table.
This value is used to program the IMX_I2C_IFDR register of the I2C block. Now if we don't consider the RCW bit 424 in our I2C driver calculation then the IMX_I2C_IFDR value that gets set makes SCL half of what is desired by the user.
This is because if you make the RCW 424th bit as 0 then anyhow I2C block (hardware) will receive Platform Clock/4, but the driver (since it has not considered this bit) will consider it as Platform Clock/2 so it'll program a bigger divider from the imx_i2c_clk_div table
>
> One reason I suggested the clock driver is that the clock driver
> contains SoC specific code already, so it should be easier to integrate there.
It seems inappropriate to put the clock frequency division modification of i2c in the clock driver, because the clock driver is for all IP and is a universal code, so I think it is better to modify the clock in the IP driver.
>
> Sascha
>
>
> --
> Pengutronix e.K. |
> |
> Industrial Linux Solutions |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p
> e ngutronix.de%2F&data=02%7C01%7Cchuanhua.han%40nxp.com%7Cb2d
> 4680699c448e8514308d6d1f5bf82%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0%7C0%7C636927250743516563&sdata=pFdCbiDXE%2FDll01X9Nj
> Hg3SCDpECzgrr8MLtYBdKH5c%3D&reserved=0 | Peiner Str. 6-8, 31137
> Hildesheim, Germany | Phone: +49-5121-206917-0
> |
> Amtsgericht Hildesheim, HRA 2686 | Fax:
> +49-5121-206917-5555 |
-----Original Message-----
From: Sumit Batra
Sent: Thursday, May 9, 2019 10:06 AM
To: Chuanhua Han <[email protected]>; Sascha Hauer <[email protected]>
Cc: [email protected]; Leo Li <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; dl-linux-imx <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: RE: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC
Hi Sascha,
Please check my comment
-----Original Message-----
From: Chuanhua Han
Sent: Wednesday, May 8, 2019 5:05 PM
To: Sascha Hauer <[email protected]>
Cc: [email protected]; Leo Li <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; dl-linux-imx <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Sumit Batra <[email protected]>
Subject: RE: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC
> -----Original Message-----
> From: Sascha Hauer <[email protected]>
> Sent: 2019年5月6日 15:38
> To: Chuanhua Han <[email protected]>
> Cc: [email protected]; Leo Li <[email protected]>;
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected];
> [email protected]; [email protected]; dl-linux-imx
> <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Sumit Batra
> <[email protected]>
> Subject: Re: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't
> consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC
>
> Caution: EXT Email
>
> On Sat, May 04, 2019 at 09:28:48AM +0000, Chuanhua Han wrote:
> >
> >
> > > -----Original Message-----
> > > From: Sascha Hauer <[email protected]>
> > > Sent: 2019年4月30日 20:51
> > > To: Chuanhua Han <[email protected]>
> > > Cc: [email protected]; Leo Li <[email protected]>;
> > > [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected];
> > > [email protected]; [email protected]; dl-linux-imx
> > > <[email protected]>; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected]; [email protected];
> > > Sumit Batra <[email protected]>
> > > 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
> > >
> > > 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.
> > >
> > > So the clock driver reports the wrong clock. Please fix the clock driver then.
> > No, this is a problem with the i2c driver. It is not a problem with
> > the clock driver, so the i2c driver needs to be modified.
>
> So how does this RCW bit get evaluated?
According to the reference manual
> only one clock goes to the i2c module (described as 1/2 Platform
> Clock) and the i2c module only takes one clock. So it seems there must
> be a /2 divider somewhere, either in each i2c module or somewhere
> outside. Can your IC guys tell you where it is?
I need to confirm this with the IC team
[Sumit Batra] There are 2 places where clock division takes place -
1) There is a clock divider outside of I2C block, which makes the clock reaching I2C module as - Platform Clock/2
2) There is another clock divider which specifically divides the clock to the I2C block, based on RCW bit 424 (if 424th bit is 0 then the baud clock source is Platform Clock/4, if 424th bit is 1 then it remains Platform Clock/2)
3) Now based on the what is the desired SCL value (100KHz etc) and the clock which is received by I2C block, there is a calculation that goes on inside the I2C driver module which is used to map a value in this imx_i2c_clk_div table.
This value is used to program the IMX_I2C_IFDR register of the I2C block. Now if we don't consider the RCW bit 424 in our I2C driver calculation then the IMX_I2C_IFDR value that gets set makes SCL half of what is desired by the user.
This is because if you make the RCW 424th bit as 0 then anyhow I2C block (hardware) will receive Platform Clock/4, but the driver (since it has not considered this bit) will consider it as Platform Clock/2 so it'll program a bigger divider from
the imx_i2c_clk_div table
[Sumit Batra] Just to clarify... Platform Clock/2 happens for many blocks in the system, but this RCW 424th bit is specific for I2C modules (specifically in ls1046a), now for this one RCW bit which is specific to I2C module do you think it is advisable to change the clock driver ?
>
> One reason I suggested the clock driver is that the clock driver
> contains SoC specific code already, so it should be easier to integrate there.
It seems inappropriate to put the clock frequency division modification of i2c in the clock driver, because the clock driver is for all IP and is a universal code, so I think it is better to modify the clock in the IP driver.
>
> Sascha
>
>
> --
> Pengutronix e.K. |
> |
> Industrial Linux Solutions |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p
> e ngutronix.de%2F&data=02%7C01%7Cchuanhua.han%40nxp.com%7Cb2d
> 4680699c448e8514308d6d1f5bf82%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0%7C0%7C636927250743516563&sdata=pFdCbiDXE%2FDll01X9Nj
> Hg3SCDpECzgrr8MLtYBdKH5c%3D&reserved=0 | Peiner Str. 6-8, 31137
> Hildesheim, Germany | Phone: +49-5121-206917-0
> |
> Amtsgericht Hildesheim, HRA 2686 | Fax:
> +49-5121-206917-5555 |
On Thu, May 09, 2019 at 04:35:33AM +0000, Sumit Batra wrote:
> > > > So the clock driver reports the wrong clock. Please fix the clock driver then.
> > > No, this is a problem with the i2c driver. It is not a problem with
> > > the clock driver, so the i2c driver needs to be modified.
> >
> > So how does this RCW bit get evaluated?
> According to the reference manual
> > only one clock goes to the i2c module (described as 1/2 Platform
> > Clock) and the i2c module only takes one clock. So it seems there must
> > be a /2 divider somewhere, either in each i2c module or somewhere
> > outside. Can your IC guys tell you where it is?
> I need to confirm this with the IC team
[Reformated a bit to make it readable]:
> There are 2 places where clock division takes place -
>
> 1) There is a clock divider outside of I2C block, which makes the clock reaching
> I2C module as - Platform Clock/2
> 2) There is another clock divider which specifically divides the clock to the I2C block,
> based on RCW bit 424 (if 424th bit is 0 then the baud clock source is Platform Clock/4,
> if 424th bit is 1 then it remains Platform Clock/2)
So there is a clock divider which based on RCW bit 424 divides the clock
*to* the i2c module. This suggests the divider is outside of the i2c
module itself and thus part of the clock module.
We could argue that this divider sits between the clock module and the
i2c module, but for sure it's not in the i2c module. I really suggest to
put this SoC specific into the SoC specific clock driver rather than
littering the i2c driver with it.
Sascha
>
> Now based on the what is the desired SCL value (100KHz etc) and the clock which is
> received by I2C block, there is a calculation that goes on inside the I2C driver
> module which is used to map a value in this imx_i2c_clk_div table. This value is used
> to program the IMX_I2C_IFDR register of the I2C block. Now if we don't consider the
> RCW bit 424 in our I2C driver calculation then the IMX_I2C_IFDR value that gets set
> makes SCL half of what is desired by the user. This is because if you make the RCW
> 424th bit as 0 then anyhow I2C block (hardware) will receive Platform Clock/4, but
> the driver (since it has not considered this bit) will consider it as Platform Clock/2
> so it'll program a bigger divider from the imx_i2c_clk_div table
--
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 |
> > There are 2 places where clock division takes place -
> >
> > 1) There is a clock divider outside of I2C block, which makes the clock reaching
> > I2C module as - Platform Clock/2
> > 2) There is another clock divider which specifically divides the clock to the I2C block,
> > based on RCW bit 424 (if 424th bit is 0 then the baud clock source is Platform Clock/4,
> > if 424th bit is 1 then it remains Platform Clock/2)
>
> So there is a clock divider which based on RCW bit 424 divides the clock
> *to* the i2c module. This suggests the divider is outside of the i2c
> module itself and thus part of the clock module.
>
> We could argue that this divider sits between the clock module and the
> i2c module, but for sure it's not in the i2c module. I really suggest to
> put this SoC specific into the SoC specific clock driver rather than
> littering the i2c driver with it.
I am with Sascha here. The fact that you need to of_ioremap some
registers is a really strong indication that the code should go
somewhere else. I can't tell the best place (clock driver or seperate
GUTS driver or syscon driver), but the I2C bus driver seems not
suitable.