* [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC @ 2019-04-30 4:47 ` Chuanhua Han 0 siblings, 0 replies; 43+ messages in thread From: Chuanhua Han @ 2019-04-30 4:47 UTC (permalink / raw) To: shawnguo, s.hauer, leoyang.li, robh+dt, mark.rutland Cc: linux-kernel, linux-i2c, linux-arm-kernel, devicetree, festevam, linux-imx, wsa+renesas, u.kleine-koenig, eha, linux, l.stach, peda, sumit.batra, Chuanhua Han 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@nxp.com> Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com> --- 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 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC @ 2019-04-30 4:47 ` Chuanhua Han 0 siblings, 0 replies; 43+ messages in thread From: Chuanhua Han @ 2019-04-30 4:47 UTC (permalink / raw) To: shawnguo, s.hauer, leoyang.li, robh+dt, mark.rutland Cc: devicetree, sumit.batra, eha, Chuanhua Han, linux-kernel, linux, wsa+renesas, linux-imx, u.kleine-koenig, l.stach, festevam, peda, linux-arm-kernel, linux-i2c 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@nxp.com> Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com> --- 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC @ 2019-04-30 4:47 ` Chuanhua Han 0 siblings, 0 replies; 43+ messages in thread From: Chuanhua Han @ 2019-04-30 4:47 UTC (permalink / raw) To: shawnguo, s.hauer, leoyang.li, robh+dt, mark.rutland Cc: devicetree, sumit.batra, eha, Chuanhua Han, linux-kernel, linux, wsa+renesas, linux-imx, u.kleine-koenig, l.stach, festevam, peda, linux-arm-kernel, linux-i2c 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@nxp.com> Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com> --- 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 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 2/2] arm64: dts: fsl: ls1046a: Add the guts node in dts 2019-04-30 4:47 ` Chuanhua Han @ 2019-04-30 4:47 ` Chuanhua Han -1 siblings, 0 replies; 43+ messages in thread From: Chuanhua Han @ 2019-04-30 4:47 UTC (permalink / raw) To: shawnguo, s.hauer, leoyang.li, robh+dt, mark.rutland Cc: linux-kernel, linux-i2c, linux-arm-kernel, devicetree, festevam, linux-imx, wsa+renesas, u.kleine-koenig, eha, linux, l.stach, peda, sumit.batra, Chuanhua Han For NXP ls1046a SoC, the i2c clock needs to be configured with the appropriate bit of RCW, so we add the guts node (GUTS/DCFG global utilities block) for the driver to read. Signed-off-by: Sumit Batra <sumit.batra@nxp.com> Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com> --- arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi index 373310e4c0ea..f88599df18bb 100644 --- a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi +++ b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi @@ -205,6 +205,11 @@ status = "disabled"; }; + guts: global-utilities@1ee0000 { + compatible = "fsl,qoriq-device-config"; + reg = <0x0 0x1ee0000 0x0 0x1000>; + }; + qspi: spi@1550000 { compatible = "fsl,ls1021a-qspi"; #address-cells = <1>; -- 2.17.1 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 2/2] arm64: dts: fsl: ls1046a: Add the guts node in dts @ 2019-04-30 4:47 ` Chuanhua Han 0 siblings, 0 replies; 43+ messages in thread From: Chuanhua Han @ 2019-04-30 4:47 UTC (permalink / raw) To: shawnguo, s.hauer, leoyang.li, robh+dt, mark.rutland Cc: devicetree, sumit.batra, eha, Chuanhua Han, linux-kernel, linux, wsa+renesas, linux-imx, u.kleine-koenig, l.stach, festevam, peda, linux-arm-kernel, linux-i2c For NXP ls1046a SoC, the i2c clock needs to be configured with the appropriate bit of RCW, so we add the guts node (GUTS/DCFG global utilities block) for the driver to read. Signed-off-by: Sumit Batra <sumit.batra@nxp.com> Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com> --- arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi index 373310e4c0ea..f88599df18bb 100644 --- a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi +++ b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi @@ -205,6 +205,11 @@ status = "disabled"; }; + guts: global-utilities@1ee0000 { + compatible = "fsl,qoriq-device-config"; + reg = <0x0 0x1ee0000 0x0 0x1000>; + }; + qspi: spi@1550000 { compatible = "fsl,ls1021a-qspi"; #address-cells = <1>; -- 2.17.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH 2/2] arm64: dts: fsl: ls1046a: Add the guts node in dts 2019-04-30 4:47 ` Chuanhua Han (?) @ 2019-05-06 7:41 ` Sascha Hauer -1 siblings, 0 replies; 43+ messages in thread From: Sascha Hauer @ 2019-05-06 7:41 UTC (permalink / raw) To: Chuanhua Han Cc: shawnguo, leoyang.li, robh+dt, mark.rutland, linux-kernel, linux-i2c, linux-arm-kernel, devicetree, festevam, linux-imx, wsa+renesas, u.kleine-koenig, eha, linux, l.stach, peda, sumit.batra On Tue, Apr 30, 2019 at 12:47:19PM +0800, Chuanhua Han wrote: > For NXP ls1046a SoC, the i2c clock needs to be configured with the > appropriate bit of RCW, so we add the guts node (GUTS/DCFG global > utilities block) for the driver to read. > > Signed-off-by: Sumit Batra <sumit.batra@nxp.com> > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com> > --- > arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi > index 373310e4c0ea..f88599df18bb 100644 > --- a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi > +++ b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi > @@ -205,6 +205,11 @@ > status = "disabled"; > }; > > + guts: global-utilities@1ee0000 { > + compatible = "fsl,qoriq-device-config"; > + reg = <0x0 0x1ee0000 0x0 0x1000>; > + }; According to Documentation/devicetree/bindings/soc/fsl/guts.txt we have the following compatibles: "fsl,qoriq-device-config-1.0" "fsl,qoriq-device-config-2.0" "fsl,<chip>-device-config" "fsl,<chip>-guts" "fsl,qoriq-device-config" is none of them and I don't think you should give this SoC specific thing a generic compatible. "fsl,ls1046a-device-config" would be better. 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 | ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 2/2] arm64: dts: fsl: ls1046a: Add the guts node in dts @ 2019-05-06 7:41 ` Sascha Hauer 0 siblings, 0 replies; 43+ messages in thread From: Sascha Hauer @ 2019-05-06 7:41 UTC (permalink / raw) To: Chuanhua Han Cc: mark.rutland, devicetree, sumit.batra, eha, festevam, linux-kernel, leoyang.li, wsa+renesas, robh+dt, l.stach, linux-i2c, u.kleine-koenig, linux, shawnguo, peda, linux-arm-kernel, linux-imx On Tue, Apr 30, 2019 at 12:47:19PM +0800, Chuanhua Han wrote: > For NXP ls1046a SoC, the i2c clock needs to be configured with the > appropriate bit of RCW, so we add the guts node (GUTS/DCFG global > utilities block) for the driver to read. > > Signed-off-by: Sumit Batra <sumit.batra@nxp.com> > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com> > --- > arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi > index 373310e4c0ea..f88599df18bb 100644 > --- a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi > +++ b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi > @@ -205,6 +205,11 @@ > status = "disabled"; > }; > > + guts: global-utilities@1ee0000 { > + compatible = "fsl,qoriq-device-config"; > + reg = <0x0 0x1ee0000 0x0 0x1000>; > + }; According to Documentation/devicetree/bindings/soc/fsl/guts.txt we have the following compatibles: "fsl,qoriq-device-config-1.0" "fsl,qoriq-device-config-2.0" "fsl,<chip>-device-config" "fsl,<chip>-guts" "fsl,qoriq-device-config" is none of them and I don't think you should give this SoC specific thing a generic compatible. "fsl,ls1046a-device-config" would be better. 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 | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 2/2] arm64: dts: fsl: ls1046a: Add the guts node in dts @ 2019-05-06 7:41 ` Sascha Hauer 0 siblings, 0 replies; 43+ messages in thread From: Sascha Hauer @ 2019-05-06 7:41 UTC (permalink / raw) To: Chuanhua Han Cc: mark.rutland, devicetree, sumit.batra, eha, festevam, linux-kernel, leoyang.li, wsa+renesas, robh+dt, l.stach, linux-i2c, u.kleine-koenig, linux, shawnguo, peda, linux-arm-kernel, linux-imx On Tue, Apr 30, 2019 at 12:47:19PM +0800, Chuanhua Han wrote: > For NXP ls1046a SoC, the i2c clock needs to be configured with the > appropriate bit of RCW, so we add the guts node (GUTS/DCFG global > utilities block) for the driver to read. > > Signed-off-by: Sumit Batra <sumit.batra@nxp.com> > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com> > --- > arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi > index 373310e4c0ea..f88599df18bb 100644 > --- a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi > +++ b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi > @@ -205,6 +205,11 @@ > status = "disabled"; > }; > > + guts: global-utilities@1ee0000 { > + compatible = "fsl,qoriq-device-config"; > + reg = <0x0 0x1ee0000 0x0 0x1000>; > + }; According to Documentation/devicetree/bindings/soc/fsl/guts.txt we have the following compatibles: "fsl,qoriq-device-config-1.0" "fsl,qoriq-device-config-2.0" "fsl,<chip>-device-config" "fsl,<chip>-guts" "fsl,qoriq-device-config" is none of them and I don't think you should give this SoC specific thing a generic compatible. "fsl,ls1046a-device-config" would be better. 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 | ^ permalink raw reply [flat|nested] 43+ messages in thread
* RE: [EXT] Re: [PATCH 2/2] arm64: dts: fsl: ls1046a: Add the guts node in dts 2019-05-06 7:41 ` Sascha Hauer @ 2019-05-06 7:44 ` Chuanhua Han -1 siblings, 0 replies; 43+ messages in thread From: Chuanhua Han @ 2019-05-06 7:44 UTC (permalink / raw) To: Sascha Hauer Cc: shawnguo, Leo Li, robh+dt, mark.rutland, linux-kernel, linux-i2c, linux-arm-kernel, devicetree, festevam, dl-linux-imx, wsa+renesas, u.kleine-koenig, eha, linux, l.stach, peda, Sumit Batra > -----Original Message----- > From: Sascha Hauer <s.hauer@pengutronix.de> > Sent: 2019年5月6日 15:41 > To: Chuanhua Han <chuanhua.han@nxp.com> > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; > mark.rutland@arm.com; linux-kernel@vger.kernel.org; > linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; > l.stach@pengutronix.de; peda@axentia.se; Sumit Batra > <sumit.batra@nxp.com> > Subject: [EXT] Re: [PATCH 2/2] arm64: dts: fsl: ls1046a: Add the guts node in > dts > > Caution: EXT Email > > On Tue, Apr 30, 2019 at 12:47:19PM +0800, Chuanhua Han wrote: > > For NXP ls1046a SoC, the i2c clock needs to be configured with the > > appropriate bit of RCW, so we add the guts node (GUTS/DCFG global > > utilities block) for the driver to read. > > > > Signed-off-by: Sumit Batra <sumit.batra@nxp.com> > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com> > > --- > > arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi > > b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi > > index 373310e4c0ea..f88599df18bb 100644 > > --- a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi > > +++ b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi > > @@ -205,6 +205,11 @@ > > status = "disabled"; > > }; > > > > + guts: global-utilities@1ee0000 { > > + compatible = "fsl,qoriq-device-config"; > > + reg = <0x0 0x1ee0000 0x0 0x1000>; > > + }; > > According to Documentation/devicetree/bindings/soc/fsl/guts.txt we have the > following compatibles: > > "fsl,qoriq-device-config-1.0" > "fsl,qoriq-device-config-2.0" > "fsl,<chip>-device-config" > "fsl,<chip>-guts" > > "fsl,qoriq-device-config" is none of them and I don't think you should give this > SoC specific thing a generic compatible. > "fsl,ls1046a-device-config" would be better. yes, you are right,I will modify it > > 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%7C139 > 23fe17a1d46dad7e708d6d1f63f41%7C686ea1d3bc2b4c6fa92cd99c5c301635 > %7C0%7C0%7C636927252885458344&sdata=RLeDiCtLJRYzOZQ4P8CN8g > hTUGNF%2FKA%2FT%2FtLSCrgEaE%3D&reserved=0 | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 > | > Amtsgericht Hildesheim, HRA 2686 | Fax: > +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 43+ messages in thread
* RE: [EXT] Re: [PATCH 2/2] arm64: dts: fsl: ls1046a: Add the guts node in dts @ 2019-05-06 7:44 ` Chuanhua Han 0 siblings, 0 replies; 43+ messages in thread From: Chuanhua Han @ 2019-05-06 7:44 UTC (permalink / raw) To: Sascha Hauer Cc: mark.rutland, devicetree, Sumit Batra, eha, festevam, linux-kernel, Leo Li, wsa+renesas, robh+dt, l.stach, linux-i2c, u.kleine-koenig, linux, shawnguo, peda, linux-arm-kernel, dl-linux-imx > -----Original Message----- > From: Sascha Hauer <s.hauer@pengutronix.de> > Sent: 2019年5月6日 15:41 > To: Chuanhua Han <chuanhua.han@nxp.com> > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; > mark.rutland@arm.com; linux-kernel@vger.kernel.org; > linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; > l.stach@pengutronix.de; peda@axentia.se; Sumit Batra > <sumit.batra@nxp.com> > Subject: [EXT] Re: [PATCH 2/2] arm64: dts: fsl: ls1046a: Add the guts node in > dts > > Caution: EXT Email > > On Tue, Apr 30, 2019 at 12:47:19PM +0800, Chuanhua Han wrote: > > For NXP ls1046a SoC, the i2c clock needs to be configured with the > > appropriate bit of RCW, so we add the guts node (GUTS/DCFG global > > utilities block) for the driver to read. > > > > Signed-off-by: Sumit Batra <sumit.batra@nxp.com> > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com> > > --- > > arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi > > b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi > > index 373310e4c0ea..f88599df18bb 100644 > > --- a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi > > +++ b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi > > @@ -205,6 +205,11 @@ > > status = "disabled"; > > }; > > > > + guts: global-utilities@1ee0000 { > > + compatible = "fsl,qoriq-device-config"; > > + reg = <0x0 0x1ee0000 0x0 0x1000>; > > + }; > > According to Documentation/devicetree/bindings/soc/fsl/guts.txt we have the > following compatibles: > > "fsl,qoriq-device-config-1.0" > "fsl,qoriq-device-config-2.0" > "fsl,<chip>-device-config" > "fsl,<chip>-guts" > > "fsl,qoriq-device-config" is none of them and I don't think you should give this > SoC specific thing a generic compatible. > "fsl,ls1046a-device-config" would be better. yes, you are right,I will modify it > > 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%7C139 > 23fe17a1d46dad7e708d6d1f63f41%7C686ea1d3bc2b4c6fa92cd99c5c301635 > %7C0%7C0%7C636927252885458344&sdata=RLeDiCtLJRYzOZQ4P8CN8g > hTUGNF%2FKA%2FT%2FtLSCrgEaE%3D&reserved=0 | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 > | > Amtsgericht Hildesheim, HRA 2686 | Fax: > +49-5121-206917-5555 | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 43+ messages in thread
* RE: [EXT] Re: [PATCH 2/2] arm64: dts: fsl: ls1046a: Add the guts node in dts 2019-05-06 7:41 ` Sascha Hauer @ 2019-05-08 11:38 ` Chuanhua Han -1 siblings, 0 replies; 43+ messages in thread From: Chuanhua Han @ 2019-05-08 11:38 UTC (permalink / raw) To: Sascha Hauer Cc: shawnguo, Leo Li, robh+dt, mark.rutland, linux-kernel, linux-i2c, linux-arm-kernel, devicetree, festevam, dl-linux-imx, wsa+renesas, u.kleine-koenig, eha, linux, l.stach, peda, Sumit Batra > -----Original Message----- > From: Sascha Hauer <s.hauer@pengutronix.de> > Sent: 2019年5月6日 15:41 > To: Chuanhua Han <chuanhua.han@nxp.com> > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; > mark.rutland@arm.com; linux-kernel@vger.kernel.org; > linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; > l.stach@pengutronix.de; peda@axentia.se; Sumit Batra > <sumit.batra@nxp.com> > Subject: [EXT] Re: [PATCH 2/2] arm64: dts: fsl: ls1046a: Add the guts node in > dts > > Caution: EXT Email > > On Tue, Apr 30, 2019 at 12:47:19PM +0800, Chuanhua Han wrote: > > For NXP ls1046a SoC, the i2c clock needs to be configured with the > > appropriate bit of RCW, so we add the guts node (GUTS/DCFG global > > utilities block) for the driver to read. > > > > Signed-off-by: Sumit Batra <sumit.batra@nxp.com> > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com> > > --- > > arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi > > b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi > > index 373310e4c0ea..f88599df18bb 100644 > > --- a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi > > +++ b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi > > @@ -205,6 +205,11 @@ > > status = "disabled"; > > }; > > > > + guts: global-utilities@1ee0000 { > > + compatible = "fsl,qoriq-device-config"; > > + reg = <0x0 0x1ee0000 0x0 0x1000>; > > + }; > > According to Documentation/devicetree/bindings/soc/fsl/guts.txt we have the > following compatibles: > > "fsl,qoriq-device-config-1.0" > "fsl,qoriq-device-config-2.0" > "fsl,<chip>-device-config" > "fsl,<chip>-guts" > > "fsl,qoriq-device-config" is none of them and I don't think you should give this > SoC specific thing a generic compatible. > "fsl,ls1046a-device-config" would be better. > Yes, you should be right > 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%7C139 > 23fe17a1d46dad7e708d6d1f63f41%7C686ea1d3bc2b4c6fa92cd99c5c301635 > %7C0%7C0%7C636927252885458344&sdata=RLeDiCtLJRYzOZQ4P8CN8g > hTUGNF%2FKA%2FT%2FtLSCrgEaE%3D&reserved=0 | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 > | > Amtsgericht Hildesheim, HRA 2686 | Fax: > +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 43+ messages in thread
* RE: [EXT] Re: [PATCH 2/2] arm64: dts: fsl: ls1046a: Add the guts node in dts @ 2019-05-08 11:38 ` Chuanhua Han 0 siblings, 0 replies; 43+ messages in thread From: Chuanhua Han @ 2019-05-08 11:38 UTC (permalink / raw) To: Sascha Hauer Cc: mark.rutland, devicetree, Sumit Batra, eha, festevam, linux-kernel, Leo Li, wsa+renesas, robh+dt, l.stach, linux-i2c, u.kleine-koenig, linux, shawnguo, peda, linux-arm-kernel, dl-linux-imx > -----Original Message----- > From: Sascha Hauer <s.hauer@pengutronix.de> > Sent: 2019年5月6日 15:41 > To: Chuanhua Han <chuanhua.han@nxp.com> > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; > mark.rutland@arm.com; linux-kernel@vger.kernel.org; > linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; > l.stach@pengutronix.de; peda@axentia.se; Sumit Batra > <sumit.batra@nxp.com> > Subject: [EXT] Re: [PATCH 2/2] arm64: dts: fsl: ls1046a: Add the guts node in > dts > > Caution: EXT Email > > On Tue, Apr 30, 2019 at 12:47:19PM +0800, Chuanhua Han wrote: > > For NXP ls1046a SoC, the i2c clock needs to be configured with the > > appropriate bit of RCW, so we add the guts node (GUTS/DCFG global > > utilities block) for the driver to read. > > > > Signed-off-by: Sumit Batra <sumit.batra@nxp.com> > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com> > > --- > > arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi > > b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi > > index 373310e4c0ea..f88599df18bb 100644 > > --- a/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi > > +++ b/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi > > @@ -205,6 +205,11 @@ > > status = "disabled"; > > }; > > > > + guts: global-utilities@1ee0000 { > > + compatible = "fsl,qoriq-device-config"; > > + reg = <0x0 0x1ee0000 0x0 0x1000>; > > + }; > > According to Documentation/devicetree/bindings/soc/fsl/guts.txt we have the > following compatibles: > > "fsl,qoriq-device-config-1.0" > "fsl,qoriq-device-config-2.0" > "fsl,<chip>-device-config" > "fsl,<chip>-guts" > > "fsl,qoriq-device-config" is none of them and I don't think you should give this > SoC specific thing a generic compatible. > "fsl,ls1046a-device-config" would be better. > Yes, you should be right > 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%7C139 > 23fe17a1d46dad7e708d6d1f63f41%7C686ea1d3bc2b4c6fa92cd99c5c301635 > %7C0%7C0%7C636927252885458344&sdata=RLeDiCtLJRYzOZQ4P8CN8g > hTUGNF%2FKA%2FT%2FtLSCrgEaE%3D&reserved=0 | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 > | > Amtsgericht Hildesheim, HRA 2686 | Fax: > +49-5121-206917-5555 | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC 2019-04-30 4:47 ` Chuanhua Han @ 2019-04-30 12:50 ` Sascha Hauer -1 siblings, 0 replies; 43+ messages in thread From: Sascha Hauer @ 2019-04-30 12:50 UTC (permalink / raw) To: Chuanhua Han Cc: shawnguo, leoyang.li, robh+dt, mark.rutland, linux-kernel, linux-i2c, linux-arm-kernel, devicetree, festevam, linux-imx, wsa+renesas, u.kleine-koenig, eha, linux, l.stach, peda, sumit.batra 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 | ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC @ 2019-04-30 12:50 ` Sascha Hauer 0 siblings, 0 replies; 43+ messages in thread From: Sascha Hauer @ 2019-04-30 12:50 UTC (permalink / raw) To: Chuanhua Han Cc: mark.rutland, devicetree, sumit.batra, eha, festevam, linux-kernel, leoyang.li, wsa+renesas, robh+dt, l.stach, linux-i2c, u.kleine-koenig, linux, shawnguo, peda, linux-arm-kernel, linux-imx 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 | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 43+ messages in thread
* RE: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC 2019-04-30 12:50 ` Sascha Hauer @ 2019-05-04 9:28 ` Chuanhua Han -1 siblings, 0 replies; 43+ messages in thread From: Chuanhua Han @ 2019-05-04 9:28 UTC (permalink / raw) To: Sascha Hauer Cc: shawnguo, Leo Li, robh+dt, mark.rutland, linux-kernel, linux-i2c, linux-arm-kernel, devicetree, festevam, dl-linux-imx, wsa+renesas, u.kleine-koenig, eha, linux, l.stach, peda, Sumit Batra > -----Original Message----- > From: Sascha Hauer <s.hauer@pengutronix.de> > Sent: 2019年4月30日 20:51 > To: Chuanhua Han <chuanhua.han@nxp.com> > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; > mark.rutland@arm.com; linux-kernel@vger.kernel.org; > linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; > l.stach@pengutronix.de; peda@axentia.se; Sumit Batra > <sumit.batra@nxp.com> > 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 | ^ permalink raw reply [flat|nested] 43+ messages in thread
* RE: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC @ 2019-05-04 9:28 ` Chuanhua Han 0 siblings, 0 replies; 43+ messages in thread From: Chuanhua Han @ 2019-05-04 9:28 UTC (permalink / raw) To: Sascha Hauer Cc: mark.rutland, devicetree, Sumit Batra, eha, festevam, linux-kernel, Leo Li, wsa+renesas, robh+dt, l.stach, linux-i2c, u.kleine-koenig, linux, shawnguo, peda, linux-arm-kernel, dl-linux-imx > -----Original Message----- > From: Sascha Hauer <s.hauer@pengutronix.de> > Sent: 2019年4月30日 20:51 > To: Chuanhua Han <chuanhua.han@nxp.com> > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; > mark.rutland@arm.com; linux-kernel@vger.kernel.org; > linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; > l.stach@pengutronix.de; peda@axentia.se; Sumit Batra > <sumit.batra@nxp.com> > 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 | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC 2019-05-04 9:28 ` Chuanhua Han @ 2019-05-06 7:37 ` Sascha Hauer -1 siblings, 0 replies; 43+ messages in thread From: Sascha Hauer @ 2019-05-06 7:37 UTC (permalink / raw) To: Chuanhua Han Cc: shawnguo, Leo Li, robh+dt, mark.rutland, linux-kernel, linux-i2c, linux-arm-kernel, devicetree, festevam, dl-linux-imx, wsa+renesas, u.kleine-koenig, eha, linux, l.stach, peda, Sumit Batra On Sat, May 04, 2019 at 09:28:48AM +0000, Chuanhua Han wrote: > > > > -----Original Message----- > > From: Sascha Hauer <s.hauer@pengutronix.de> > > Sent: 2019年4月30日 20:51 > > To: Chuanhua Han <chuanhua.han@nxp.com> > > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; > > mark.rutland@arm.com; linux-kernel@vger.kernel.org; > > linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > > u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; > > l.stach@pengutronix.de; peda@axentia.se; Sumit Batra > > <sumit.batra@nxp.com> > > 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 | ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC @ 2019-05-06 7:37 ` Sascha Hauer 0 siblings, 0 replies; 43+ messages in thread From: Sascha Hauer @ 2019-05-06 7:37 UTC (permalink / raw) To: Chuanhua Han Cc: mark.rutland, devicetree, Sumit Batra, eha, festevam, linux-kernel, Leo Li, wsa+renesas, robh+dt, l.stach, linux-i2c, u.kleine-koenig, linux, shawnguo, peda, linux-arm-kernel, dl-linux-imx On Sat, May 04, 2019 at 09:28:48AM +0000, Chuanhua Han wrote: > > > > -----Original Message----- > > From: Sascha Hauer <s.hauer@pengutronix.de> > > Sent: 2019年4月30日 20:51 > > To: Chuanhua Han <chuanhua.han@nxp.com> > > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; > > mark.rutland@arm.com; linux-kernel@vger.kernel.org; > > linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > > u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; > > l.stach@pengutronix.de; peda@axentia.se; Sumit Batra > > <sumit.batra@nxp.com> > > 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 | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 43+ messages in thread
* RE: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC 2019-05-06 7:37 ` Sascha Hauer @ 2019-05-06 7:46 ` Chuanhua Han -1 siblings, 0 replies; 43+ messages in thread From: Chuanhua Han @ 2019-05-06 7:46 UTC (permalink / raw) To: Sascha Hauer Cc: shawnguo, Leo Li, robh+dt, mark.rutland, linux-kernel, linux-i2c, linux-arm-kernel, devicetree, festevam, dl-linux-imx, wsa+renesas, u.kleine-koenig, eha, linux, l.stach, peda, Sumit Batra > -----Original Message----- > From: Sascha Hauer <s.hauer@pengutronix.de> > Sent: 2019年5月6日 15:38 > To: Chuanhua Han <chuanhua.han@nxp.com> > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; > mark.rutland@arm.com; linux-kernel@vger.kernel.org; > linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; > l.stach@pengutronix.de; peda@axentia.se; Sumit Batra > <sumit.batra@nxp.com> > 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 <s.hauer@pengutronix.de> > > > Sent: 2019年4月30日 20:51 > > > To: Chuanhua Han <chuanhua.han@nxp.com> > > > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; > > > robh+dt@kernel.org; mark.rutland@arm.com; > > > linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; > > > linux-arm-kernel@lists.infradead.org; > > > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > > > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > > > u.kleine-koenig@pengutronix.de; eha@deif.com; > > > linux@rempel-privat.de; l.stach@pengutronix.de; peda@axentia.se; > > > Sumit Batra <sumit.batra@nxp.com> > > > 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 | ^ permalink raw reply [flat|nested] 43+ messages in thread
* RE: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC @ 2019-05-06 7:46 ` Chuanhua Han 0 siblings, 0 replies; 43+ messages in thread From: Chuanhua Han @ 2019-05-06 7:46 UTC (permalink / raw) To: Sascha Hauer Cc: mark.rutland, devicetree, Sumit Batra, eha, festevam, linux-kernel, Leo Li, wsa+renesas, robh+dt, l.stach, linux-i2c, u.kleine-koenig, linux, shawnguo, peda, linux-arm-kernel, dl-linux-imx > -----Original Message----- > From: Sascha Hauer <s.hauer@pengutronix.de> > Sent: 2019年5月6日 15:38 > To: Chuanhua Han <chuanhua.han@nxp.com> > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; > mark.rutland@arm.com; linux-kernel@vger.kernel.org; > linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; > l.stach@pengutronix.de; peda@axentia.se; Sumit Batra > <sumit.batra@nxp.com> > 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 <s.hauer@pengutronix.de> > > > Sent: 2019年4月30日 20:51 > > > To: Chuanhua Han <chuanhua.han@nxp.com> > > > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; > > > robh+dt@kernel.org; mark.rutland@arm.com; > > > linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; > > > linux-arm-kernel@lists.infradead.org; > > > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > > > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > > > u.kleine-koenig@pengutronix.de; eha@deif.com; > > > linux@rempel-privat.de; l.stach@pengutronix.de; peda@axentia.se; > > > Sumit Batra <sumit.batra@nxp.com> > > > 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 | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 43+ messages in thread
* RE: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC 2019-05-06 7:37 ` Sascha Hauer (?) @ 2019-05-08 11:34 ` Chuanhua Han -1 siblings, 0 replies; 43+ messages in thread From: Chuanhua Han @ 2019-05-08 11:34 UTC (permalink / raw) To: Sascha Hauer Cc: shawnguo, Leo Li, robh+dt, mark.rutland, linux-kernel, linux-i2c, linux-arm-kernel, devicetree, festevam, dl-linux-imx, wsa+renesas, u.kleine-koenig, eha, linux, l.stach, peda, Sumit Batra > -----Original Message----- > From: Sascha Hauer <s.hauer@pengutronix.de> > Sent: 2019年5月6日 15:38 > To: Chuanhua Han <chuanhua.han@nxp.com> > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; > mark.rutland@arm.com; linux-kernel@vger.kernel.org; > linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; > l.stach@pengutronix.de; peda@axentia.se; Sumit Batra > <sumit.batra@nxp.com> > 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 <s.hauer@pengutronix.de> > > > Sent: 2019年4月30日 20:51 > > > To: Chuanhua Han <chuanhua.han@nxp.com> > > > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; > > > robh+dt@kernel.org; mark.rutland@arm.com; > > > linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; > > > linux-arm-kernel@lists.infradead.org; > > > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > > > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > > > u.kleine-koenig@pengutronix.de; eha@deif.com; > > > linux@rempel-privat.de; l.stach@pengutronix.de; peda@axentia.se; > > > Sumit Batra <sumit.batra@nxp.com> > > > 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 | ^ permalink raw reply [flat|nested] 43+ messages in thread
* RE: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC @ 2019-05-08 11:34 ` Chuanhua Han 0 siblings, 0 replies; 43+ messages in thread From: Chuanhua Han @ 2019-05-08 11:34 UTC (permalink / raw) To: Sascha Hauer Cc: mark.rutland, devicetree, Sumit Batra, eha, festevam, linux-kernel, Leo Li, wsa+renesas, robh+dt, l.stach, linux-i2c, u.kleine-koenig, linux, shawnguo, peda, linux-arm-kernel, dl-linux-imx > -----Original Message----- > From: Sascha Hauer <s.hauer@pengutronix.de> > Sent: 2019年5月6日 15:38 > To: Chuanhua Han <chuanhua.han@nxp.com> > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; > mark.rutland@arm.com; linux-kernel@vger.kernel.org; > linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; > l.stach@pengutronix.de; peda@axentia.se; Sumit Batra > <sumit.batra@nxp.com> > 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 <s.hauer@pengutronix.de> > > > Sent: 2019年4月30日 20:51 > > > To: Chuanhua Han <chuanhua.han@nxp.com> > > > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; > > > robh+dt@kernel.org; mark.rutland@arm.com; > > > linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; > > > linux-arm-kernel@lists.infradead.org; > > > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > > > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > > > u.kleine-koenig@pengutronix.de; eha@deif.com; > > > linux@rempel-privat.de; l.stach@pengutronix.de; peda@axentia.se; > > > Sumit Batra <sumit.batra@nxp.com> > > > 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 | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 43+ messages in thread
* RE: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC @ 2019-05-08 11:34 ` Chuanhua Han 0 siblings, 0 replies; 43+ messages in thread From: Chuanhua Han @ 2019-05-08 11:34 UTC (permalink / raw) To: Sascha Hauer Cc: shawnguo, Leo Li, robh+dt, mark.rutland, linux-kernel, linux-i2c, linux-arm-kernel, devicetree, festevam, dl-linux-imx, wsa+renesas, u.kleine-koenig, eha, linux, l.stach > -----Original Message----- > From: Sascha Hauer <s.hauer@pengutronix.de> > Sent: 2019年5月6日 15:38 > To: Chuanhua Han <chuanhua.han@nxp.com> > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; > mark.rutland@arm.com; linux-kernel@vger.kernel.org; > linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; > l.stach@pengutronix.de; peda@axentia.se; Sumit Batra > <sumit.batra@nxp.com> > 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 <s.hauer@pengutronix.de> > > > Sent: 2019年4月30日 20:51 > > > To: Chuanhua Han <chuanhua.han@nxp.com> > > > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; > > > robh+dt@kernel.org; mark.rutland@arm.com; > > > linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; > > > linux-arm-kernel@lists.infradead.org; > > > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > > > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > > > u.kleine-koenig@pengutronix.de; eha@deif.com; > > > linux@rempel-privat.de; l.stach@pengutronix.de; peda@axentia.se; > > > Sumit Batra <sumit.batra@nxp.com> > > > 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 | ^ permalink raw reply [flat|nested] 43+ messages in thread
* RE: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC 2019-05-08 11:34 ` Chuanhua Han (?) @ 2019-05-09 4:35 ` Sumit Batra -1 siblings, 0 replies; 43+ messages in thread From: Sumit Batra @ 2019-05-09 4:35 UTC (permalink / raw) To: Chuanhua Han, Sascha Hauer Cc: shawnguo, Leo Li, robh+dt, mark.rutland, linux-kernel, linux-i2c, linux-arm-kernel, devicetree, festevam, dl-linux-imx, wsa+renesas, u.kleine-koenig, eha, linux, l.stach, peda Hi Sascha, Please check my comment -----Original Message----- From: Chuanhua Han Sent: Wednesday, May 8, 2019 5:05 PM To: Sascha Hauer <s.hauer@pengutronix.de> Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; mark.rutland@arm.com; linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; l.stach@pengutronix.de; peda@axentia.se; Sumit Batra <sumit.batra@nxp.com> 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 <s.hauer@pengutronix.de> > Sent: 2019年5月6日 15:38 > To: Chuanhua Han <chuanhua.han@nxp.com> > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; > robh+dt@kernel.org; mark.rutland@arm.com; > linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; > linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; > l.stach@pengutronix.de; peda@axentia.se; Sumit Batra > <sumit.batra@nxp.com> > 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 <s.hauer@pengutronix.de> > > > Sent: 2019年4月30日 20:51 > > > To: Chuanhua Han <chuanhua.han@nxp.com> > > > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; > > > robh+dt@kernel.org; mark.rutland@arm.com; > > > linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; > > > linux-arm-kernel@lists.infradead.org; > > > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > > > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > > > u.kleine-koenig@pengutronix.de; eha@deif.com; > > > linux@rempel-privat.de; l.stach@pengutronix.de; peda@axentia.se; > > > Sumit Batra <sumit.batra@nxp.com> > > > 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 | ^ permalink raw reply [flat|nested] 43+ messages in thread
* RE: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC @ 2019-05-09 4:35 ` Sumit Batra 0 siblings, 0 replies; 43+ messages in thread From: Sumit Batra @ 2019-05-09 4:35 UTC (permalink / raw) To: Chuanhua Han, Sascha Hauer Cc: mark.rutland, devicetree, eha, festevam, linux-kernel, Leo Li, wsa+renesas, robh+dt, l.stach, linux-i2c, u.kleine-koenig, linux, shawnguo, peda, linux-arm-kernel, dl-linux-imx Hi Sascha, Please check my comment -----Original Message----- From: Chuanhua Han Sent: Wednesday, May 8, 2019 5:05 PM To: Sascha Hauer <s.hauer@pengutronix.de> Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; mark.rutland@arm.com; linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; l.stach@pengutronix.de; peda@axentia.se; Sumit Batra <sumit.batra@nxp.com> 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 <s.hauer@pengutronix.de> > Sent: 2019年5月6日 15:38 > To: Chuanhua Han <chuanhua.han@nxp.com> > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; > robh+dt@kernel.org; mark.rutland@arm.com; > linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; > linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; > l.stach@pengutronix.de; peda@axentia.se; Sumit Batra > <sumit.batra@nxp.com> > 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 <s.hauer@pengutronix.de> > > > Sent: 2019年4月30日 20:51 > > > To: Chuanhua Han <chuanhua.han@nxp.com> > > > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; > > > robh+dt@kernel.org; mark.rutland@arm.com; > > > linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; > > > linux-arm-kernel@lists.infradead.org; > > > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > > > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > > > u.kleine-koenig@pengutronix.de; eha@deif.com; > > > linux@rempel-privat.de; l.stach@pengutronix.de; peda@axentia.se; > > > Sumit Batra <sumit.batra@nxp.com> > > > 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 | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 43+ messages in thread
* RE: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC @ 2019-05-09 4:35 ` Sumit Batra 0 siblings, 0 replies; 43+ messages in thread From: Sumit Batra @ 2019-05-09 4:35 UTC (permalink / raw) To: Chuanhua Han, Sascha Hauer Cc: shawnguo, Leo Li, robh+dt, mark.rutland, linux-kernel, linux-i2c, linux-arm-kernel, devicetree, festevam, dl-linux-imx, wsa+renesas, u.kleine-koenig, eha, linux, l.stach Hi Sascha, Please check my comment -----Original Message----- From: Chuanhua Han Sent: Wednesday, May 8, 2019 5:05 PM To: Sascha Hauer <s.hauer@pengutronix.de> Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; mark.rutland@arm.com; linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; l.stach@pengutronix.de; peda@axentia.se; Sumit Batra <sumit.batra@nxp.com> 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 <s.hauer@pengutronix.de> > Sent: 2019年5月6日 15:38 > To: Chuanhua Han <chuanhua.han@nxp.com> > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; > robh+dt@kernel.org; mark.rutland@arm.com; > linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; > linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; > l.stach@pengutronix.de; peda@axentia.se; Sumit Batra > <sumit.batra@nxp.com> > 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 <s.hauer@pengutronix.de> > > > Sent: 2019年4月30日 20:51 > > > To: Chuanhua Han <chuanhua.han@nxp.com> > > > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; > > > robh+dt@kernel.org; mark.rutland@arm.com; > > > linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; > > > linux-arm-kernel@lists.infradead.org; > > > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > > > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > > > u.kleine-koenig@pengutronix.de; eha@deif.com; > > > linux@rempel-privat.de; l.stach@pengutronix.de; peda@axentia.se; > > > Sumit Batra <sumit.batra@nxp.com> > > > 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 | ^ permalink raw reply [flat|nested] 43+ messages in thread
* RE: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC 2019-05-09 4:35 ` Sumit Batra (?) @ 2019-05-09 5:10 ` Sumit Batra -1 siblings, 0 replies; 43+ messages in thread From: Sumit Batra @ 2019-05-09 5:10 UTC (permalink / raw) To: Chuanhua Han, Sascha Hauer Cc: shawnguo, Leo Li, robh+dt, mark.rutland, linux-kernel, linux-i2c, linux-arm-kernel, devicetree, festevam, dl-linux-imx, wsa+renesas, u.kleine-koenig, eha, linux, l.stach, peda -----Original Message----- From: Sumit Batra Sent: Thursday, May 9, 2019 10:06 AM To: Chuanhua Han <chuanhua.han@nxp.com>; Sascha Hauer <s.hauer@pengutronix.de> Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; mark.rutland@arm.com; linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; l.stach@pengutronix.de; peda@axentia.se 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 <s.hauer@pengutronix.de> Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; mark.rutland@arm.com; linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; l.stach@pengutronix.de; peda@axentia.se; Sumit Batra <sumit.batra@nxp.com> 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 <s.hauer@pengutronix.de> > Sent: 2019年5月6日 15:38 > To: Chuanhua Han <chuanhua.han@nxp.com> > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; > robh+dt@kernel.org; mark.rutland@arm.com; > linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; > linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; > l.stach@pengutronix.de; peda@axentia.se; Sumit Batra > <sumit.batra@nxp.com> > 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 <s.hauer@pengutronix.de> > > > Sent: 2019年4月30日 20:51 > > > To: Chuanhua Han <chuanhua.han@nxp.com> > > > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; > > > robh+dt@kernel.org; mark.rutland@arm.com; > > > linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; > > > linux-arm-kernel@lists.infradead.org; > > > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > > > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > > > u.kleine-koenig@pengutronix.de; eha@deif.com; > > > linux@rempel-privat.de; l.stach@pengutronix.de; peda@axentia.se; > > > Sumit Batra <sumit.batra@nxp.com> > > > 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 | ^ permalink raw reply [flat|nested] 43+ messages in thread
* RE: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC @ 2019-05-09 5:10 ` Sumit Batra 0 siblings, 0 replies; 43+ messages in thread From: Sumit Batra @ 2019-05-09 5:10 UTC (permalink / raw) To: Chuanhua Han, Sascha Hauer Cc: mark.rutland, devicetree, eha, festevam, linux-kernel, Leo Li, wsa+renesas, robh+dt, l.stach, linux-i2c, u.kleine-koenig, linux, shawnguo, peda, linux-arm-kernel, dl-linux-imx -----Original Message----- From: Sumit Batra Sent: Thursday, May 9, 2019 10:06 AM To: Chuanhua Han <chuanhua.han@nxp.com>; Sascha Hauer <s.hauer@pengutronix.de> Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; mark.rutland@arm.com; linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; l.stach@pengutronix.de; peda@axentia.se 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 <s.hauer@pengutronix.de> Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; mark.rutland@arm.com; linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; l.stach@pengutronix.de; peda@axentia.se; Sumit Batra <sumit.batra@nxp.com> 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 <s.hauer@pengutronix.de> > Sent: 2019年5月6日 15:38 > To: Chuanhua Han <chuanhua.han@nxp.com> > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; > robh+dt@kernel.org; mark.rutland@arm.com; > linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; > linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; > l.stach@pengutronix.de; peda@axentia.se; Sumit Batra > <sumit.batra@nxp.com> > 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 <s.hauer@pengutronix.de> > > > Sent: 2019年4月30日 20:51 > > > To: Chuanhua Han <chuanhua.han@nxp.com> > > > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; > > > robh+dt@kernel.org; mark.rutland@arm.com; > > > linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; > > > linux-arm-kernel@lists.infradead.org; > > > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > > > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > > > u.kleine-koenig@pengutronix.de; eha@deif.com; > > > linux@rempel-privat.de; l.stach@pengutronix.de; peda@axentia.se; > > > Sumit Batra <sumit.batra@nxp.com> > > > 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 | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 43+ messages in thread
* RE: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC @ 2019-05-09 5:10 ` Sumit Batra 0 siblings, 0 replies; 43+ messages in thread From: Sumit Batra @ 2019-05-09 5:10 UTC (permalink / raw) To: Chuanhua Han, Sascha Hauer Cc: shawnguo, Leo Li, robh+dt, mark.rutland, linux-kernel, linux-i2c, linux-arm-kernel, devicetree, festevam, dl-linux-imx, wsa+renesas, u.kleine-koenig, eha, linux, l.stach -----Original Message----- From: Sumit Batra Sent: Thursday, May 9, 2019 10:06 AM To: Chuanhua Han <chuanhua.han@nxp.com>; Sascha Hauer <s.hauer@pengutronix.de> Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; mark.rutland@arm.com; linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; l.stach@pengutronix.de; peda@axentia.se 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 <s.hauer@pengutronix.de> Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; mark.rutland@arm.com; linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; l.stach@pengutronix.de; peda@axentia.se; Sumit Batra <sumit.batra@nxp.com> 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 <s.hauer@pengutronix.de> > Sent: 2019年5月6日 15:38 > To: Chuanhua Han <chuanhua.han@nxp.com> > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; > robh+dt@kernel.org; mark.rutland@arm.com; > linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; > linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; > l.stach@pengutronix.de; peda@axentia.se; Sumit Batra > <sumit.batra@nxp.com> > 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 <s.hauer@pengutronix.de> > > > Sent: 2019年4月30日 20:51 > > > To: Chuanhua Han <chuanhua.han@nxp.com> > > > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; > > > robh+dt@kernel.org; mark.rutland@arm.com; > > > linux-kernel@vger.kernel.org; linux-i2c@vger.kernel.org; > > > linux-arm-kernel@lists.infradead.org; > > > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > > > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > > > u.kleine-koenig@pengutronix.de; eha@deif.com; > > > linux@rempel-privat.de; l.stach@pengutronix.de; peda@axentia.se; > > > Sumit Batra <sumit.batra@nxp.com> > > > 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 | ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC 2019-05-09 4:35 ` Sumit Batra (?) @ 2019-05-09 7:48 ` Sascha Hauer -1 siblings, 0 replies; 43+ messages in thread From: Sascha Hauer @ 2019-05-09 7:48 UTC (permalink / raw) To: Sumit Batra Cc: Chuanhua Han, shawnguo, Leo Li, robh+dt, mark.rutland, linux-kernel, linux-i2c, linux-arm-kernel, devicetree, festevam, dl-linux-imx, wsa+renesas, u.kleine-koenig, eha, linux, l.stach, peda 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 | ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC @ 2019-05-09 7:48 ` Sascha Hauer 0 siblings, 0 replies; 43+ messages in thread From: Sascha Hauer @ 2019-05-09 7:48 UTC (permalink / raw) To: Sumit Batra Cc: mark.rutland, devicetree, eha, festevam, u.kleine-koenig, linux-kernel, Leo Li, wsa+renesas, robh+dt, l.stach, linux-i2c, Chuanhua Han, linux, shawnguo, peda, linux-arm-kernel, dl-linux-imx 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 | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC @ 2019-05-09 7:48 ` Sascha Hauer 0 siblings, 0 replies; 43+ messages in thread From: Sascha Hauer @ 2019-05-09 7:48 UTC (permalink / raw) To: Sumit Batra Cc: Chuanhua Han, shawnguo, Leo Li, robh+dt, mark.rutland, linux-kernel, linux-i2c, linux-arm-kernel, devicetree, festevam, dl-linux-imx, wsa+renesas, u.kleine-koenig, eha, linux, l.stach 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 | ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC 2019-05-09 7:48 ` Sascha Hauer (?) @ 2019-05-09 7:55 ` Wolfram Sang -1 siblings, 0 replies; 43+ messages in thread From: Wolfram Sang @ 2019-05-09 7:55 UTC (permalink / raw) To: Sascha Hauer Cc: Sumit Batra, Chuanhua Han, shawnguo, Leo Li, robh+dt, mark.rutland, linux-kernel, linux-i2c, linux-arm-kernel, devicetree, festevam, dl-linux-imx, wsa+renesas, u.kleine-koenig, eha, linux, l.stach, peda [-- Attachment #1: Type: text/plain, Size: 1193 bytes --] > > 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. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC @ 2019-05-09 7:55 ` Wolfram Sang 0 siblings, 0 replies; 43+ messages in thread From: Wolfram Sang @ 2019-05-09 7:55 UTC (permalink / raw) To: Sascha Hauer Cc: mark.rutland, devicetree, Sumit Batra, eha, festevam, u.kleine-koenig, linux-kernel, Leo Li, wsa+renesas, robh+dt, l.stach, linux-i2c, Chuanhua Han, linux, shawnguo, peda, linux-arm-kernel, dl-linux-imx [-- Attachment #1.1: Type: text/plain, Size: 1193 bytes --] > > 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. [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC @ 2019-05-09 7:55 ` Wolfram Sang 0 siblings, 0 replies; 43+ messages in thread From: Wolfram Sang @ 2019-05-09 7:55 UTC (permalink / raw) To: Sascha Hauer Cc: Sumit Batra, Chuanhua Han, shawnguo, Leo Li, robh+dt, mark.rutland, linux-kernel, linux-i2c, linux-arm-kernel, devicetree, festevam, dl-linux-imx, wsa+renesas, u.kleine-koenig, eha [-- Attachment #1: Type: text/plain, Size: 1193 bytes --] > > 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. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC 2019-04-30 4:47 ` Chuanhua Han (?) @ 2019-05-06 7:47 ` Sascha Hauer -1 siblings, 0 replies; 43+ messages in thread From: Sascha Hauer @ 2019-05-06 7:47 UTC (permalink / raw) To: Chuanhua Han Cc: shawnguo, leoyang.li, robh+dt, mark.rutland, linux-kernel, linux-i2c, linux-arm-kernel, devicetree, festevam, linux-imx, wsa+renesas, u.kleine-koenig, eha, linux, l.stach, peda, sumit.batra 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@nxp.com> > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com> > --- > 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 | ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC @ 2019-05-06 7:47 ` Sascha Hauer 0 siblings, 0 replies; 43+ messages in thread From: Sascha Hauer @ 2019-05-06 7:47 UTC (permalink / raw) To: Chuanhua Han Cc: mark.rutland, devicetree, sumit.batra, eha, festevam, linux-kernel, leoyang.li, wsa+renesas, robh+dt, l.stach, linux-i2c, u.kleine-koenig, linux, shawnguo, peda, linux-arm-kernel, linux-imx 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@nxp.com> > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com> > --- > 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 | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC @ 2019-05-06 7:47 ` Sascha Hauer 0 siblings, 0 replies; 43+ messages in thread From: Sascha Hauer @ 2019-05-06 7:47 UTC (permalink / raw) To: Chuanhua Han Cc: mark.rutland, devicetree, sumit.batra, eha, festevam, linux-kernel, leoyang.li, wsa+renesas, robh+dt, l.stach, linux-i2c, u.kleine-koenig, linux, shawnguo, peda, linux-arm-kernel, linux-imx 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@nxp.com> > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com> > --- > 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 | ^ permalink raw reply [flat|nested] 43+ messages in thread
* RE: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC 2019-05-06 7:47 ` Sascha Hauer @ 2019-05-06 7:53 ` Chuanhua Han -1 siblings, 0 replies; 43+ messages in thread From: Chuanhua Han @ 2019-05-06 7:53 UTC (permalink / raw) To: Sascha Hauer Cc: shawnguo, Leo Li, robh+dt, mark.rutland, linux-kernel, linux-i2c, linux-arm-kernel, devicetree, festevam, dl-linux-imx, wsa+renesas, u.kleine-koenig, eha, linux, l.stach, peda, Sumit Batra > -----Original Message----- > From: Sascha Hauer <s.hauer@pengutronix.de> > Sent: 2019年5月6日 15:48 > To: Chuanhua Han <chuanhua.han@nxp.com> > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; > mark.rutland@arm.com; linux-kernel@vger.kernel.org; > linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; > l.stach@pengutronix.de; peda@axentia.se; Sumit Batra > <sumit.batra@nxp.com> > 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. Ok,thank you for your advice! > > 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@nxp.com> > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com> > > --- > > 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 | > 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 | ^ permalink raw reply [flat|nested] 43+ messages in thread
* RE: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC @ 2019-05-06 7:53 ` Chuanhua Han 0 siblings, 0 replies; 43+ messages in thread From: Chuanhua Han @ 2019-05-06 7:53 UTC (permalink / raw) To: Sascha Hauer Cc: mark.rutland, devicetree, Sumit Batra, eha, festevam, linux-kernel, Leo Li, wsa+renesas, robh+dt, l.stach, linux-i2c, u.kleine-koenig, linux, shawnguo, peda, linux-arm-kernel, dl-linux-imx > -----Original Message----- > From: Sascha Hauer <s.hauer@pengutronix.de> > Sent: 2019年5月6日 15:48 > To: Chuanhua Han <chuanhua.han@nxp.com> > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; > mark.rutland@arm.com; linux-kernel@vger.kernel.org; > linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; > l.stach@pengutronix.de; peda@axentia.se; Sumit Batra > <sumit.batra@nxp.com> > 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. Ok,thank you for your advice! > > 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@nxp.com> > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com> > > --- > > 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 | > 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 | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 43+ messages in thread
* RE: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC 2019-05-06 7:47 ` Sascha Hauer (?) @ 2019-05-08 11:42 ` Chuanhua Han -1 siblings, 0 replies; 43+ messages in thread From: Chuanhua Han @ 2019-05-08 11:42 UTC (permalink / raw) To: Sascha Hauer Cc: shawnguo, Leo Li, robh+dt, mark.rutland, linux-kernel, linux-i2c, linux-arm-kernel, devicetree, festevam, dl-linux-imx, wsa+renesas, u.kleine-koenig, eha, linux, l.stach, peda, Sumit Batra > -----Original Message----- > From: Sascha Hauer <s.hauer@pengutronix.de> > Sent: 2019年5月6日 15:48 > To: Chuanhua Han <chuanhua.han@nxp.com> > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; > mark.rutland@arm.com; linux-kernel@vger.kernel.org; > linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; > l.stach@pengutronix.de; peda@axentia.se; Sumit Batra > <sumit.batra@nxp.com> > 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@nxp.com> > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com> > > --- > > 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 | ^ permalink raw reply [flat|nested] 43+ messages in thread
* RE: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC @ 2019-05-08 11:42 ` Chuanhua Han 0 siblings, 0 replies; 43+ messages in thread From: Chuanhua Han @ 2019-05-08 11:42 UTC (permalink / raw) To: Sascha Hauer Cc: mark.rutland, devicetree, Sumit Batra, eha, festevam, linux-kernel, Leo Li, wsa+renesas, robh+dt, l.stach, linux-i2c, u.kleine-koenig, linux, shawnguo, peda, linux-arm-kernel, dl-linux-imx > -----Original Message----- > From: Sascha Hauer <s.hauer@pengutronix.de> > Sent: 2019年5月6日 15:48 > To: Chuanhua Han <chuanhua.han@nxp.com> > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; > mark.rutland@arm.com; linux-kernel@vger.kernel.org; > linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; > l.stach@pengutronix.de; peda@axentia.se; Sumit Batra > <sumit.batra@nxp.com> > 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@nxp.com> > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com> > > --- > > 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 | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 43+ messages in thread
* RE: [EXT] Re: [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC @ 2019-05-08 11:42 ` Chuanhua Han 0 siblings, 0 replies; 43+ messages in thread From: Chuanhua Han @ 2019-05-08 11:42 UTC (permalink / raw) To: Sascha Hauer Cc: shawnguo, Leo Li, robh+dt, mark.rutland, linux-kernel, linux-i2c, linux-arm-kernel, devicetree, festevam, dl-linux-imx, wsa+renesas, u.kleine-koenig, eha, linux, l.stach > -----Original Message----- > From: Sascha Hauer <s.hauer@pengutronix.de> > Sent: 2019年5月6日 15:48 > To: Chuanhua Han <chuanhua.han@nxp.com> > Cc: shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; > mark.rutland@arm.com; linux-kernel@vger.kernel.org; > linux-i2c@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; festevam@gmail.com; dl-linux-imx > <linux-imx@nxp.com>; wsa+renesas@sang-engineering.com; > u.kleine-koenig@pengutronix.de; eha@deif.com; linux@rempel-privat.de; > l.stach@pengutronix.de; peda@axentia.se; Sumit Batra > <sumit.batra@nxp.com> > 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@nxp.com> > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com> > > --- > > 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 | ^ permalink raw reply [flat|nested] 43+ messages in thread
end of thread, other threads:[~2019-05-09 8:09 UTC | newest] Thread overview: 43+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-30 4:47 [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC Chuanhua Han 2019-04-30 4:47 ` Chuanhua Han 2019-04-30 4:47 ` Chuanhua Han 2019-04-30 4:47 ` [PATCH 2/2] arm64: dts: fsl: ls1046a: Add the guts node in dts Chuanhua Han 2019-04-30 4:47 ` Chuanhua Han 2019-05-06 7:41 ` Sascha Hauer 2019-05-06 7:41 ` Sascha Hauer 2019-05-06 7:41 ` Sascha Hauer 2019-05-06 7:44 ` [EXT] " Chuanhua Han 2019-05-06 7:44 ` Chuanhua Han 2019-05-08 11:38 ` Chuanhua Han 2019-05-08 11:38 ` Chuanhua Han 2019-04-30 12:50 ` [PATCH 1/2] i2c: imx: I2C Driver doesn't consider I2C_IPGCLK_SEL RCW bit when using ls1046a SoC Sascha Hauer 2019-04-30 12:50 ` Sascha Hauer 2019-05-04 9:28 ` [EXT] " Chuanhua Han 2019-05-04 9:28 ` Chuanhua Han 2019-05-06 7:37 ` Sascha Hauer 2019-05-06 7:37 ` Sascha Hauer 2019-05-06 7:46 ` Chuanhua Han 2019-05-06 7:46 ` Chuanhua Han 2019-05-08 11:34 ` Chuanhua Han 2019-05-08 11:34 ` Chuanhua Han 2019-05-08 11:34 ` Chuanhua Han 2019-05-09 4:35 ` Sumit Batra 2019-05-09 4:35 ` Sumit Batra 2019-05-09 4:35 ` Sumit Batra 2019-05-09 5:10 ` Sumit Batra 2019-05-09 5:10 ` Sumit Batra 2019-05-09 5:10 ` Sumit Batra 2019-05-09 7:48 ` Sascha Hauer 2019-05-09 7:48 ` Sascha Hauer 2019-05-09 7:48 ` Sascha Hauer 2019-05-09 7:55 ` Wolfram Sang 2019-05-09 7:55 ` Wolfram Sang 2019-05-09 7:55 ` Wolfram Sang 2019-05-06 7:47 ` Sascha Hauer 2019-05-06 7:47 ` Sascha Hauer 2019-05-06 7:47 ` Sascha Hauer 2019-05-06 7:53 ` [EXT] " Chuanhua Han 2019-05-06 7:53 ` Chuanhua Han 2019-05-08 11:42 ` Chuanhua Han 2019-05-08 11:42 ` Chuanhua Han 2019-05-08 11:42 ` Chuanhua Han
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.