i2c: aspeed: avoid new registers definition of AST2600
Subsystems (4)
cover i2c: aspeed: avoid new registers definition of AST2600
Jamin Lin (3):
i2c: aspeed: avoid new registers definition of AST2600
ARM: dts: aspeed: Add node for AST2600 I2C
dt-bindings: aspeed-i2c: Convert txt to yaml format
.../devicetree/bindings/i2c/aspeed,i2c.yaml | 89 +++++++++++++++++++ .../devicetree/bindings/i2c/i2c-aspeed.txt | 49 ---------- arch/arm/boot/dts/aspeed-g6.dtsi | 8 ++ drivers/i2c/busses/i2c-aspeed.c | 22 +++++ 4 files changed, 119 insertions(+), 49 deletions(-) create mode 100644 Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
Signature · 1 line
2.17.1
PATCH 1/3 i2c: aspeed: avoid new registers definition of AST2600
The register definition between AST2600 A2 and A3 is different. This patch avoid new registers definition of AST2600 to use this driver. We will submit the path for the new registers definition of AST2600.
--- drivers/i2c/busses/i2c-aspeed.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
drivers/i2c/busses/i2c-aspeed.c +22 -0
@@ -19,14 +19,20 @@#include <linux/irqchip/chained_irq.h> #include <linux/irqdomain.h> #include <linux/kernel.h> +#include <linux/mfd/syscon.h> #include <linux/module.h> #include <linux/of_address.h> #include <linux/of_irq.h> #include <linux/of_platform.h> #include <linux/platform_device.h> +#include <linux/regmap.h> #include <linux/reset.h> #include <linux/slab.h> +/* I2C Global Registers */ +/* 0x0c : I2CG Global Control Register (AST2500) */ +#define ASPEED_I2CG_GLOBAL_CTRL_REG 0x0c +/* I2C Register */ #define ASPEED_I2C_FUN_CTRL_REG 0x00#define ASPEED_I2C_AC_TIMING_REG1 0x04
@@ -973,6 +979,22 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)struct resource *res;int irq, ret; +if (of_device_is_compatible (pdev->dev.of_node, +"aspeed,ast2600-i2c-bus" )) { +u32 global_ctrl; +struct regmap *gr_regmap; + + gr_regmap =syscon_regmap_lookup_by_compatible ("aspeed,ast2600-i2c-global" ); + +if (IS_ERR (gr_regmap)) { + ret =PTR_ERR (gr_regmap); + }else { +regmap_read (gr_regmap,ASPEED_I2CG_GLOBAL_CTRL_REG , &global_ctrl); +if (global_ctrl &BIT (2 )) +return -EIO ; + } + } + bus =devm_kzalloc (&pdev->dev,sizeof (*bus),GFP_KERNEL );if (!bus)return -ENOMEM ;
Signature · 1 line
2.17.1
Quoted 42 lines · Jamin Lin
The register definition between AST2600 A2 and A3 is different. This patch avoid new registers definition of AST2600 to use this driver. We will submit the path for the new registers definition of AST2600.
Signed-off-by Jamin Lin <[email protected]>---
drivers/i2c/busses/i2c-aspeed.c| 22 ++++++++++++++++++++++1 file changed, 22 insertions(+)drivers/i2c/busses/i2c-aspeed.c +9
-0@@ -19,14 +19,20 @@#include <linux/irqchip/chained_irq.h> #include <linux/irqdomain.h> #include <linux/kernel.h> +#include <linux/mfd/syscon.h> #include <linux/module.h> #include <linux/of_address.h> #include <linux/of_irq.h> #include <linux/of_platform.h> #include <linux/platform_device.h> +#include <linux/regmap.h> #include <linux/reset.h> #include <linux/slab.h> +/* I2C Global Registers */ +/* 0x0c : I2CG Global Control Register (AST2500) */ +#define ASPEED_I2CG_GLOBAL_CTRL_REG 0x0c +/* I2C Register */ #define ASPEED_I2C_FUN_CTRL_REG 0x00#define ASPEED_I2C_AC_TIMING_REG1 0x04@@ -973,6 +979,22 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)struct resource *res;int irq, ret; +if (of_device_is_compatible (pdev->dev.of_node, +"aspeed,ast2600-i2c-bus" )) { +u32 global_ctrl;
drivers/i2c/busses/i2c-aspeed.c static int aspeed_i2c_probe_bus(struct platform_device *pdev) · line 994
Quoted 10 lines
drivers/i2c/busses/i2c-aspeed.c +10
-0+struct regmap *gr_regmap; + + gr_regmap =syscon_regmap_lookup_by_compatible ("aspeed,ast2600-i2c-global" ); + +if (IS_ERR (gr_regmap)) { + ret =PTR_ERR (gr_regmap); + }else { +regmap_read (gr_regmap,ASPEED_I2CG_GLOBAL_CTRL_REG , &global_ctrl); +if (global_ctrl &BIT (2 )) +return -EIO ;
A macro definition might be a bit nicer than a raw BIT(2) here I'd think.
Also, it seems a bit unfortunate to just bail on the device entirely if we find this bit set (seems like a good way for a bootloader to inadvertently DoS the kernel), though I guess poking global syscon bits in the bus probe function might not be ideal. Could/should we consider some module-level init code to ensure that bit is cleared?
Zev
Quoted 9 lines
drivers/i2c/busses/i2c-aspeed.c +3
-0+ } + } + bus =devm_kzalloc (&pdev->dev,sizeof (*bus),GFP_KERNEL );if (!bus)return -ENOMEM ;Signature · 1 line
2.17.1
Quoted 40 lines · Zev Weiss
Quoted 39 lines · Jamin Lin
The register definition between AST2600 A2 and A3 is different. This patch avoid new registers definition of AST2600 to use this driver. We will submit the path for the new registers definition of AST2600.
Signed-off-by Jamin Lin <[email protected]>---
drivers/i2c/busses/i2c-aspeed.c| 22 ++++++++++++++++++++++1 file changed, 22 insertions(+)drivers/i2c/busses/i2c-aspeed.c +6
-0@@ -19,14 +19,20 @@#include <linux/irqchip/chained_irq.h> #include <linux/irqdomain.h> #include <linux/kernel.h> +#include <linux/mfd/syscon.h> #include <linux/module.h> #include <linux/of_address.h> #include <linux/of_irq.h> #include <linux/of_platform.h> #include <linux/platform_device.h> +#include <linux/regmap.h> #include <linux/reset.h> #include <linux/slab.h> +/* I2C Global Registers */ +/* 0x0c : I2CG Global Control Register (AST2500) */ +#define ASPEED_I2CG_GLOBAL_CTRL_REG 0x0c +/* I2C Register */ #define ASPEED_I2C_FUN_CTRL_REG 0x00#define ASPEED_I2C_AC_TIMING_REG1 0x04@@ -973,6 +979,22 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)struct resource *res;int irq, ret;
Quoted 16 lines
drivers/i2c/busses/i2c-aspeed.c static int aspeed_i2c_probe_bus(struct platform_device *pdev) · line 994
Quoted 13 lines
drivers/i2c/busses/i2c-aspeed.c +13
-0+if (of_device_is_compatible (pdev->dev.of_node, +"aspeed,ast2600-i2c-bus" )) { +u32 global_ctrl; +struct regmap *gr_regmap; + + gr_regmap =syscon_regmap_lookup_by_compatible ("aspeed,ast2600-i2c-global" ); + +if (IS_ERR (gr_regmap)) { + ret =PTR_ERR (gr_regmap); + }else { +regmap_read (gr_regmap,ASPEED_I2CG_GLOBAL_CTRL_REG , &global_ctrl); +if (global_ctrl &BIT (2 )) +return -EIO ;A macro definition might be a bit nicer than a raw BIT(2) here I'd think.
Will modify
Quoted 8 lines
Also, it seems a bit unfortunate to just bail on the device entirely if we find this bit set (seems like a good way for a bootloader to inadvertently DoS the kernel), though I guess poking global syscon bits in the bus probe function might not be ideal. Could/should we consider some module-level init code to ensure that bit is cleared?
We use syscon API to get the global register of i2c not the specific i2c bus.
Can you describe it more detail?
Thanks-Jamin
Quoted 11 lines
Zev
Quoted 9 lines
drivers/i2c/busses/i2c-aspeed.c +3
-0+ } + } + bus =devm_kzalloc (&pdev->dev,sizeof (*bus),GFP_KERNEL );if (!bus)return -ENOMEM ;Signature · 1 line
2.17.1
Quoted 66 lines · Jamin Lin
Quoted 40 lines · Zev Weiss
Quoted 39 lines · Jamin Lin
The register definition between AST2600 A2 and A3 is different. This patch avoid new registers definition of AST2600 to use this driver. We will submit the path for the new registers definition of AST2600.
Signed-off-by Jamin Lin <[email protected]>---
drivers/i2c/busses/i2c-aspeed.c| 22 ++++++++++++++++++++++1 file changed, 22 insertions(+)drivers/i2c/busses/i2c-aspeed.c +6
-0@@ -19,14 +19,20 @@#include <linux/irqchip/chained_irq.h> #include <linux/irqdomain.h> #include <linux/kernel.h> +#include <linux/mfd/syscon.h> #include <linux/module.h> #include <linux/of_address.h> #include <linux/of_irq.h> #include <linux/of_platform.h> #include <linux/platform_device.h> +#include <linux/regmap.h> #include <linux/reset.h> #include <linux/slab.h> +/* I2C Global Registers */ +/* 0x0c : I2CG Global Control Register (AST2500) */ +#define ASPEED_I2CG_GLOBAL_CTRL_REG 0x0c +/* I2C Register */ #define ASPEED_I2C_FUN_CTRL_REG 0x00#define ASPEED_I2C_AC_TIMING_REG1 0x04@@ -973,6 +979,22 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)struct resource *res;int irq, ret;Quoted 16 lines
drivers/i2c/busses/i2c-aspeed.c static int aspeed_i2c_probe_bus(struct platform_device *pdev) · line 994
Quoted 13 lines
drivers/i2c/busses/i2c-aspeed.c +13
-0+if (of_device_is_compatible (pdev->dev.of_node, +"aspeed,ast2600-i2c-bus" )) { +u32 global_ctrl; +struct regmap *gr_regmap; + + gr_regmap =syscon_regmap_lookup_by_compatible ("aspeed,ast2600-i2c-global" ); + +if (IS_ERR (gr_regmap)) { + ret =PTR_ERR (gr_regmap); + }else { +regmap_read (gr_regmap,ASPEED_I2CG_GLOBAL_CTRL_REG , &global_ctrl); +if (global_ctrl &BIT (2 )) +return -EIO ;A macro definition might be a bit nicer than a raw BIT(2) here I'd think.
Will modify
Quoted 8 lines
Also, it seems a bit unfortunate to just bail on the device entirely if we find this bit set (seems like a good way for a bootloader to inadvertently DoS the kernel), though I guess poking global syscon bits in the bus probe function might not be ideal. Could/should we consider some module-level init code to ensure that bit is cleared?
Quoted 3 lines
We use syscon API to get the global register of i2c not the specific i2c bus.
Can you describe it more detail?
Sure -- I just meant that if for whatever reason the kernel is booting on a system that's had that syscon bit set to enable the new register access mode (e.g. by a newer bootloader or something), it seems like we'd just give up entirely on enabling any i2c busses, when as far as I know there shouldn't be anything stopping us from resetting the bit back to be in the state this driver needs it to be in (old register mode) and then continuing along normally.
Zev
Quoted 80 lines · Zev Weiss
Quoted 66 lines · Jamin Lin
Quoted 40 lines · Zev Weiss
Quoted 39 lines · Jamin Lin
The register definition between AST2600 A2 and A3 is different. This patch avoid new registers definition of AST2600 to use this driver. We will submit the path for the new registers definition of AST2600.
Signed-off-by Jamin Lin <[email protected]>---
drivers/i2c/busses/i2c-aspeed.c| 22 ++++++++++++++++++++++1 file changed, 22 insertions(+)drivers/i2c/busses/i2c-aspeed.c +6
-0@@ -19,14 +19,20 @@#include <linux/irqchip/chained_irq.h> #include <linux/irqdomain.h> #include <linux/kernel.h> +#include <linux/mfd/syscon.h> #include <linux/module.h> #include <linux/of_address.h> #include <linux/of_irq.h> #include <linux/of_platform.h> #include <linux/platform_device.h> +#include <linux/regmap.h> #include <linux/reset.h> #include <linux/slab.h> +/* I2C Global Registers */ +/* 0x0c : I2CG Global Control Register (AST2500) */ +#define ASPEED_I2CG_GLOBAL_CTRL_REG 0x0c +/* I2C Register */ #define ASPEED_I2C_FUN_CTRL_REG 0x00#define ASPEED_I2C_AC_TIMING_REG1 0x04@@ -973,6 +979,22 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)struct resource *res;int irq, ret;Quoted 16 lines
drivers/i2c/busses/i2c-aspeed.c static int aspeed_i2c_probe_bus(struct platform_device *pdev) · line 994
Quoted 13 lines
drivers/i2c/busses/i2c-aspeed.c +13
-0+if (of_device_is_compatible (pdev->dev.of_node, +"aspeed,ast2600-i2c-bus" )) { +u32 global_ctrl; +struct regmap *gr_regmap; + + gr_regmap =syscon_regmap_lookup_by_compatible ("aspeed,ast2600-i2c-global" ); + +if (IS_ERR (gr_regmap)) { + ret =PTR_ERR (gr_regmap); + }else { +regmap_read (gr_regmap,ASPEED_I2CG_GLOBAL_CTRL_REG , &global_ctrl); +if (global_ctrl &BIT (2 )) +return -EIO ;A macro definition might be a bit nicer than a raw BIT(2) here I'd think.
Will modify
Quoted 8 lines
Also, it seems a bit unfortunate to just bail on the device entirely if we find this bit set (seems like a good way for a bootloader to inadvertently DoS the kernel), though I guess poking global syscon bits in the bus probe function might not be ideal. Could/should we consider some module-level init code to ensure that bit is cleared?
Quoted 3 lines
We use syscon API to get the global register of i2c not the specific i2c bus.
Can you describe it more detail?Sure -- I just meant that if for whatever reason the kernel is booting on a system that's had that syscon bit set to enable the new register access mode (e.g. by a newer bootloader or something), it seems like we'd just give up entirely on enabling any i2c busses, when as far as I know there shouldn't be anything stopping us from resetting the bit back to be in the state this driver needs it to be in (old register mode) and then continuing along normally.
Quoted 1 line
Zev
Thanks for your suggestion. I will submit the new i2c driver for AST2600.
Jamin
Joel Stanley
maintainerQuoted 5 lines · Jamin Lin
The register definition between AST2600 A2 and A3 is different. This patch avoid new registers definition of AST2600 to use this driver. We will submit the path for the new registers definition of AST2600.
The AST2600 v9 datasheet says that bit 2 selects between old and new register sets, and that the old register set is the default.
Has the default changed for the A3?, and the datasheet is incorrect?
Does the A3 still support the old register set?
Quoted 57 lines
Signed-off-by Jamin Lin <[email protected]>--- drivers/i2c/busses/i2c-aspeed.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)drivers/i2c/busses/i2c-aspeed.c +22
-1@@ -19,14 +19,20 @@#include <linux/irqchip/chained_irq.h> #include <linux/irqdomain.h> #include <linux/kernel.h> +#include <linux/mfd/syscon.h> #include <linux/module.h> #include <linux/of_address.h> #include <linux/of_irq.h> #include <linux/of_platform.h> #include <linux/platform_device.h> +#include <linux/regmap.h> #include <linux/reset.h> #include <linux/slab.h> +/* I2C Global Registers */ +/* 0x0c : I2CG Global Control Register (AST2500) */ +#define ASPEED_I2CG_GLOBAL_CTRL_REG 0x0c +/* I2C Register */ #define ASPEED_I2C_FUN_CTRL_REG 0x00#define ASPEED_I2C_AC_TIMING_REG1 0x04@@ -973,6 +979,22 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)struct resource *res;int irq, ret; +if (of_device_is_compatible (pdev->dev.of_node, +"aspeed,ast2600-i2c-bus" )) { +u32 global_ctrl; +struct regmap *gr_regmap; + + gr_regmap =syscon_regmap_lookup_by_compatible ("aspeed,ast2600-i2c-global" ); + +if (IS_ERR (gr_regmap)) { + ret =PTR_ERR (gr_regmap); + }else { +regmap_read (gr_regmap,ASPEED_I2CG_GLOBAL_CTRL_REG , &global_ctrl); +if (global_ctrl &BIT (2 )) +return -EIO ; + } + } + bus =devm_kzalloc (&pdev->dev,sizeof (*bus),GFP_KERNEL );if (!bus)return -ENOMEM ; --2.17.1
Quoted 7 lines · Joel Stanley
Quoted 5 lines · Jamin Lin
The register definition between AST2600 A2 and A3 is different. This patch avoid new registers definition of AST2600 to use this driver. We will submit the path for the new registers definition of AST2600.
Quoted 7 lines
The AST2600 v9 datasheet says that bit 2 selects between old and new register sets, and that the old register set is the default.
Has the default changed for the A3?, and the datasheet is incorrect?
Does the A3 still support the old register set?
We suggest user to use the new i2c driver for AST2600 and we will sumbit it. This driver is used to AST2500 and AST2400 SOCs. Change this driver to check global register of i2c to avoid user build the wrong driver.
Quoted 57 lines
Quoted 57 lines
Signed-off-by Jamin Lin <[email protected]>--- drivers/i2c/busses/i2c-aspeed.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)drivers/i2c/busses/i2c-aspeed.c +22
-1@@ -19,14 +19,20 @@#include <linux/irqchip/chained_irq.h> #include <linux/irqdomain.h> #include <linux/kernel.h> +#include <linux/mfd/syscon.h> #include <linux/module.h> #include <linux/of_address.h> #include <linux/of_irq.h> #include <linux/of_platform.h> #include <linux/platform_device.h> +#include <linux/regmap.h> #include <linux/reset.h> #include <linux/slab.h> +/* I2C Global Registers */ +/* 0x0c : I2CG Global Control Register (AST2500) */ +#define ASPEED_I2CG_GLOBAL_CTRL_REG 0x0c +/* I2C Register */ #define ASPEED_I2C_FUN_CTRL_REG 0x00#define ASPEED_I2C_AC_TIMING_REG1 0x04@@ -973,6 +979,22 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)struct resource *res;int irq, ret; +if (of_device_is_compatible (pdev->dev.of_node, +"aspeed,ast2600-i2c-bus" )) { +u32 global_ctrl; +struct regmap *gr_regmap; + + gr_regmap =syscon_regmap_lookup_by_compatible ("aspeed,ast2600-i2c-global" ); + +if (IS_ERR (gr_regmap)) { + ret =PTR_ERR (gr_regmap); + }else { +regmap_read (gr_regmap,ASPEED_I2CG_GLOBAL_CTRL_REG , &global_ctrl); +if (global_ctrl &BIT (2 )) +return -EIO ; + } + } + bus =devm_kzalloc (&pdev->dev,sizeof (*bus),GFP_KERNEL );if (!bus)return -ENOMEM ; --2.17.1
Hi Jamin,
Quoted 11 lines · Jamin Lin
Quoted 10 lines · Joel Stanley
Quoted 5 lines · Jamin Lin
The register definition between AST2600 A2 and A3 is different. This patch avoid new registers definition of AST2600 to use this driver. We will submit the path for the new registers definition of AST2600.
The AST2600 v9 datasheet says that bit 2 selects between old and new register sets, and that the old register set is the default.
Quoted 7 lines
Quoted 4 lines
Has the default changed for the A3?, and the datasheet is incorrect?
Does the A3 still support the old register set?
We suggest user to use the new i2c driver for AST2600 and we will sumbit it. This driver is used to AST2500 and AST2400 SOCs. Change this driver to check global register of i2c to avoid user build the wrong driver.
If I understand correctly, the answer implies old register set is still supported in A3 although aspeed suggest people using the new driver/mode?
Can you please share more context behind the suggestion? Such as new register mode has better performance? Or some known issues that were deteted in old mode are fixed in new register mode?
Cheers,
Tao
Quoted 22 lines · Tao Ren
Hi Jamin,
Quoted 11 lines · Jamin Lin
Quoted 10 lines · Joel Stanley
Quoted 5 lines · Jamin Lin
The register definition between AST2600 A2 and A3 is different. This patch avoid new registers definition of AST2600 to use this driver. We will submit the path for the new registers definition of AST2600.
The AST2600 v9 datasheet says that bit 2 selects between old and new register sets, and that the old register set is the default.
Quoted 7 lines
Quoted 4 lines
Has the default changed for the A3?, and the datasheet is incorrect?
Does the A3 still support the old register set?
We suggest user to use the new i2c driver for AST2600 and we will sumbit it. This driver is used to AST2500 and AST2400 SOCs. Change this driver to check global register of i2c to avoid user build the wrong driver.
Quoted 7 lines
If I understand correctly, the answer implies old register set is still supported in A3 although aspeed suggest people using the new driver/mode?
Can you please share more context behind the suggestion? Such as new register mode has better performance? Or some known issues that were deteted in old mode are fixed in new register mode?
Yes, AST2600 A1, A2 and A3 support both old and new register set. The difference between old and new register set are the register address and supported registers. User can choose to use both old and new register set. However, ASPEED would like to change new register set by default for AST2600.
Thanks-Jamin
Quoted 4 lines
Cheers,
Tao
Joel Stanley
maintainerQuoted 27 lines · Jamin Lin
Quoted 25 lines · Tao Ren
Hi Jamin,
Quoted 11 lines · Jamin Lin
Quoted 10 lines · Joel Stanley
Quoted 5 lines · Jamin Lin
The register definition between AST2600 A2 and A3 is different. This patch avoid new registers definition of AST2600 to use this driver. We will submit the path for the new registers definition of AST2600.
The AST2600 v9 datasheet says that bit 2 selects between old and new register sets, and that the old register set is the default.
Quoted 7 lines
Quoted 4 lines
Has the default changed for the A3?, and the datasheet is incorrect?
Does the A3 still support the old register set?
We suggest user to use the new i2c driver for AST2600 and we will sumbit it. This driver is used to AST2500 and AST2400 SOCs. Change this driver to check global register of i2c to avoid user build the wrong driver.
If I understand correctly, the answer implies old register set is still supported in A3 although aspeed suggest people using the new driver/mode?
Quoted 8 lines
Quoted 4 lines
Can you please share more context behind the suggestion? Such as new register mode has better performance? Or some known issues that were deteted in old mode are fixed in new register mode?
Yes, AST2600 A1, A2 and A3 support both old and new register set. The difference between old and new register set are the register address and supported registers. User can choose to use both old and new register set. However, ASPEED would like to change new register set by default for AST2600.
We can certainly make the driver for the new register set the default for AST2600 when the new driver is merged.
I disagree that we should introduce a run time check to fail to probe the old driver. Please do not merge this patch.
Please focus your effort on getting the new driver merged instead.
Cheers,
Joel
Quoted 40 lines · Joel Stanley
Quoted 27 lines · Jamin Lin
Quoted 25 lines · Tao Ren
Hi Jamin,
Quoted 11 lines · Jamin Lin
Quoted 10 lines · Joel Stanley
Quoted 5 lines · Jamin Lin
The register definition between AST2600 A2 and A3 is different. This patch avoid new registers definition of AST2600 to use this driver. We will submit the path for the new registers definition of AST2600.
The AST2600 v9 datasheet says that bit 2 selects between old and new register sets, and that the old register set is the default.
Quoted 7 lines
Quoted 4 lines
Has the default changed for the A3?, and the datasheet is incorrect?
Does the A3 still support the old register set?
We suggest user to use the new i2c driver for AST2600 and we will sumbit it. This driver is used to AST2500 and AST2400 SOCs. Change this driver to check global register of i2c to avoid user build the wrong driver.
If I understand correctly, the answer implies old register set is still supported in A3 although aspeed suggest people using the new driver/mode?
Quoted 8 lines
Quoted 4 lines
Can you please share more context behind the suggestion? Such as new register mode has better performance? Or some known issues that were deteted in old mode are fixed in new register mode?
Yes, AST2600 A1, A2 and A3 support both old and new register set. The difference between old and new register set are the register address and supported registers. User can choose to use both old and new register set. However, ASPEED would like to change new register set by default for AST2600.
We can certainly make the driver for the new register set the default for AST2600 when the new driver is merged.
Quoted 8 lines
I disagree that we should introduce a run time check to fail to probe the old driver. Please do not merge this patch.
Please focus your effort on getting the new driver merged instead.
Cheers,
Joel
Thanks for your suggestion. I will submit the new i2c driver for AST2600 soon.
Jamin
PATCH 2/3 ARM: dts: aspeed: Add node for AST2600 I2C
In reply to Jamin LinAdd node to get the global register of i2c for AST2600.
--- arch/arm/boot/dts/aspeed-g6.dtsi | 8 ++++++++ 1 file changed, 8 insertions(+)
arch/arm/boot/dts/aspeed-g6.dtsi +8 -0
@@ -674,6 +674,14 @@ #include "aspeed-g6-pinctrl.dtsi" &i2c { + + i2c_gr: i2c-global-regs at 0 { + compatible = "aspeed,ast2600-i2c-global", "syscon"; + reg = <0x0 0x20>; + clocks = <&syscon ASPEED_CLK_APB2>; + resets = <&syscon ASPEED_RESET_I2C>; + }; + i2c0: i2c-bus at 80 { #address-cells = <1>; #size-cells = <0>;
Signature · 1 line
2.17.1
PATCH 3/3 dt-bindings: aspeed-i2c: Convert txt to yaml format
In reply to Jamin LinAdd global-reg node for AST2600. Document the properties for "aspeed,ast2600-i2c-global" compatible node.
--- .../devicetree/bindings/i2c/aspeed,i2c.yaml | 89 +++++++++++++++++++ .../devicetree/bindings/i2c/i2c-aspeed.txt | 49 ---------- 2 files changed, 89 insertions(+), 49 deletions(-) create mode 100644 Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml new file +89 -0
@@ -0,0 +1,89 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/i2c/aspeed,i2c.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: ASPEED I2C on the AST24XX, AST25XX, and AST26XX SoCs Device Tree Bindings + +maintainers: + - Rayn Chen <[email protected]> + +allOf: + - $ref: /schemas/i2c/i2c-controller.yaml# + +properties: + compatible: + enum: + - aspeed,ast2400-i2c-bus + - aspeed,ast2500-i2c-bus + - aspeed,ast2600-i2c-bus + - aspeed,ast2600-i2c-global syscon + + "#size-cells": + const: 0 + + "#address-cells": + const: 1 + + reg: + minItems: 1 + maxItems: 2 + items: + - description: address offset and range of bus + - description: address offset and range of bus buffer + + interrupts: + description: interrupt number + + clocks: + description: + root clock of bus, should reference the APB + clock in the second cell + + reset: + description: phandle to reset controller with the reset number in + the second cell + + bus-frequency: + minimum: 10000 + maximum: 3400000 + default: 100000 + description: frequency of the bus clock in Hz defaults to 100 kHz when not + specified + + multi-master: + maxItems: 1 + description: + states that there is another master active on this bus + +required: + - reg + - compatible + - clocks + - resets + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/clock/ast2600-clock.h> + i2c_gr: i2c-global-regs at 0 { + compatible = "aspeed,ast2600-i2c-global", "syscon"; + reg = <0x0 0x20>; + clocks = <&syscon ASPEED_CLK_APB2>; + resets = <&syscon ASPEED_RESET_I2C>; + }; + + i2c0: i2c-bus at 80 { + #address-cells = <1>; + #size-cells = <0>; + #interrupt-cells = <1>; + reg = <0x80 0x80>; + compatible = "aspeed,ast2600-i2c-bus"; + clocks = <&syscon ASPEED_CLK_APB2>; + resets = <&syscon ASPEED_RESET_I2C>; + interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>; + bus-frequency = <100000>; + };
Documentation/devicetree/bindings/i2c/i2c-aspeed.txt deleted +0 -49
49 lines not on this page · raw message
Signature · 1 line
2.17.1
Rob Herring
maintainerRe: [PATCH 3/3] dt-bindings: aspeed-i2c: Convert txt to yaml format
Quoted 11 lines · Jamin Lin
Add global-reg node for AST2600. Document the properties for "aspeed,ast2600-i2c-global" compatible node.
Signed-off-by Jamin Lin <[email protected]>--- .../devicetree/bindings/i2c/aspeed,i2c.yaml | 89 +++++++++++++++++++ .../devicetree/bindings/i2c/i2c-aspeed.txt | 49 ---------- 2 files changed, 89 insertions(+), 49 deletions(-) create mode 100644 Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:Documentation/devicetree/bindings/i2c/aspeed,i2c.example.dt.yaml:0:0: /example-0/i2c-global-regs at 0: failed to match any schema with compatible: ['aspeed,ast2600-i2c-global', 'syscon']
See https://patchwork.ozlabs.org/patch/1480769
This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1.
If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:
pip3 install dtschema --upgrade
Please check and re-submit.
Re: [PATCH 3/3] dt-bindings: aspeed-i2c: Convert txt to yaml format
Quoted 26 lines · Rob Herring
Quoted 11 lines · Jamin Lin
Add global-reg node for AST2600. Document the properties for "aspeed,ast2600-i2c-global" compatible node.
Signed-off-by Jamin Lin <[email protected]>--- .../devicetree/bindings/i2c/aspeed,i2c.yaml | 89 +++++++++++++++++++ .../devicetree/bindings/i2c/i2c-aspeed.txt | 49 ---------- 2 files changed, 89 insertions(+), 49 deletions(-) create mode 100644 Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-aspeed.txtMy bot found errors running 'make DT_CHECKER_FLAGS=-m
dt_binding_check' on your patch (DT_CHECKER_FLAGSis new in v5.13):yamllint warnings/errors:
dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/i2c/aspeed,i2c.example.dt.yaml:0:0: /example-0/i2c-global-regs at 0: failed to match any schema with compatible: ['aspeed,ast2600-i2c-global', 'syscon']See https://patchwork.ozlabs.org/patch/1480769
This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1.
Quoted 8 lines
If you already ran 'make
dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:pip3 install dtschema --upgrade
Please check and re-submit.
Thanks for your review.
yes, I did not add "DT_CHECKER_FLAGS" to check my patch. I will re-sent this patch.
Thanks
Rob Herring
maintainerRe: [PATCH 3/3] dt-bindings: aspeed-i2c: Convert txt to yaml format
In reply to Jamin LinQuoted 28 lines · Jamin Lin
Add global-reg node for AST2600. Document the properties for "aspeed,ast2600-i2c-global" compatible node.
Signed-off-by Jamin Lin <[email protected]>--- .../devicetree/bindings/i2c/aspeed,i2c.yaml | 89 +++++++++++++++++++ .../devicetree/bindings/i2c/i2c-aspeed.txt | 49 ---------- 2 files changed, 89 insertions(+), 49 deletions(-) create mode 100644 Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-aspeed.txtDocumentation/devicetree/bindings/i2c/aspeed,i2c.yaml new file +11
-0@@ -0,0 +1,89 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/i2c/aspeed,i2c.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: ASPEED I2C on the AST24XX, AST25XX, and AST26XX SoCs Device Tree Bindings + +maintainers: + - Rayn Chen <[email protected]> +
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml line 21
Quoted 10 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml new file +10
-0+allOf: + - $ref: /schemas/i2c/i2c-controller.yaml# + +properties: + compatible: + enum: + - aspeed,ast2400-i2c-bus + - aspeed,ast2500-i2c-bus + - aspeed,ast2600-i2c-bus + - aspeed,ast2600-i2c-global syscon
"aspeed,ast2600-i2c-global syscon" is not a valid compatible value.
What makes this a syscon?
Quoted 11 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml new file +11
-0+ + "#size-cells": + const: 0 + + "#address-cells": + const: 1 + + reg: + minItems: 1 + maxItems: 2 + items:
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml line 42
Quoted 10 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml new file +10
-0+ - description: address offset and range of bus + - description: address offset and range of bus buffer + + interrupts: + description: interrupt number + + clocks: + description: + root clock of bus, should reference the APB + clock in the second cell
How many clocks? Needs maxItems or items.
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml line 46
Quoted 4 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml new file +4
-0+ + reset: + description: phandle to reset controller with the reset number in + the second cell
How many?
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml line 56
Quoted 10 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml new file +10
-0+ + bus-frequency: + minimum: 10000 + maximum: 3400000 + default: 100000 + description: frequency of the bus clock in Hz defaults to 100 kHz when not + specified + + multi-master: + maxItems: 1
multi-master is an array?
Quoted 91 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml new file +33
-0+ description: + states that there is another master active on this bus + +required: + - reg + - compatible + - clocks + - resets + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/clock/ast2600-clock.h> + i2c_gr: i2c-global-regs at 0 { + compatible = "aspeed,ast2600-i2c-global", "syscon"; + reg = <0x0 0x20>; + clocks = <&syscon ASPEED_CLK_APB2>; + resets = <&syscon ASPEED_RESET_I2C>; + }; + + i2c0: i2c-bus at 80 { + #address-cells = <1>; + #size-cells = <0>; + #interrupt-cells = <1>; + reg = <0x80 0x80>; + compatible = "aspeed,ast2600-i2c-bus"; + clocks = <&syscon ASPEED_CLK_APB2>; + resets = <&syscon ASPEED_RESET_I2C>; + interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>; + bus-frequency = <100000>; + };Documentation/devicetree/bindings/i2c/i2c-aspeed.txt deleted +0
-49@@ -1,49 +0,0 @@ -Device tree configuration for the I2C busses on the AST24XX, AST25XX, and AST26XX SoCs. - -Required Properties: -- #address-cells : should be 1 -- #size-cells : should be 0 -- reg : address offset and range of bus -- compatible : should be "aspeed,ast2400-i2c-bus" - or "aspeed,ast2500-i2c-bus" - or "aspeed,ast2600-i2c-bus" -- clocks : root clock of bus, should reference the APB - clock in the second cell -- resets : phandle to reset controller with the reset number in - the second cell -- interrupts : interrupt number - -Optional Properties: -- bus-frequency : frequency of the bus clock in Hz defaults to 100 kHz when not - specified -- multi-master : states that there is another master active on this bus. - -Example: - -i2c { - compatible = "simple-bus"; - #address-cells = <1>; - #size-cells = <1>; - ranges = <0 0x1e78a000 0x1000>; - - i2c_ic: interrupt-controller at 0 { - #interrupt-cells = <1>; - compatible = "aspeed,ast2400-i2c-ic"; - reg = <0x0 0x40>; - interrupts = <12>; - interrupt-controller; - }; - - i2c0: i2c-bus at 40 { - #address-cells = <1>; - #size-cells = <0>; - #interrupt-cells = <1>; - reg = <0x40 0x40>; - compatible = "aspeed,ast2400-i2c-bus"; - clocks = <&syscon ASPEED_CLK_APB>; - resets = <&syscon ASPEED_RESET_I2C>; - bus-frequency = <100000>; - interrupts = <0>; - interrupt-parent = <&i2c_ic>; - }; -};Signature · 1 line
2.17.1