| Message ID | 20211112010137.149174-3-jaewon02.kim@samsung.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: exynos5: add support for ExynosAutov9 SoC | expand |
On 12/11/2021 02:01, Jaewon Kim wrote: > Serial IPs(UART, I2C, SPI) are integrated into New IP-Core > called USI(Universal Serial Interface). > > As it is integrated into USI, there are additinal HW changes. > Registers to control USI and sysreg to set serial IPs have been added. > Also, some timing registres have been changed. > > Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com> > --- > drivers/i2c/busses/i2c-exynos5.c | 135 ++++++++++++++++++++++++++++--- > 1 file changed, 125 insertions(+), 10 deletions(-) > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> Best regards, Krzysztof
> With this patch the Exynos850 HSI2C becomes functional. The only nit-pick > from my side (just a food for thought): do we want to configure USI > related config inside of particular drivers (SPI, I2C, UART)? Or it would > be better design to implement some platform driver for that, so we can > choose USI configuration (SPI/I2C/UART) in device tree? I think this > series is good to be merged as is, but we should probably consider all > upsides and downsides of each option, for the future work. I'm also considering how to support this USI configuration gracefully. Current version of USI is v2 which means there is a v1 version as well. It might be a non-upstream SoC so we don't need to consider it so far. But, there is a possibility that the USI hw version can be bumped for future SoCs. As you probably know, earlier version of the product kernel has a USI SoC driver[1] and it was designed to be configured the USI settings by device tree. Option1) Make a USI driver under soc/samsung/ like [1]. Option2) Use more generic driver such as "reset driver"? This might be required to extend the reset core driver. Option3) Each USI driver(uart/i2c/spi) has its own USI configurations respectively and expose some configurations which can be variable as device tree. [1]: https://github.com/ianmacd/d2s/blob/master/drivers/soc/samsung/usi_v2.c Best Regards, Chanho Park
> Current version of USI is v2 which means there is a v1 version as well. > It might be a non-upstream SoC so we don't need to consider it so far. The Exynos7885 I'm working on has USI v1. It doesn't seem to be heavily used as the SoC has just 3 USI blocks if I didn't miss anything. The most obvious difference I saw was instead of having 3 modes (SPI, UART, and HSI2C) It has: - SPI - HSI2C0 (meaning I2C pins are connected to the first 2 pins out of the 4 if I understand it correctly) - HSI2C1 (connected to last 2 pins) - HSI2C0_HSI2C1 (2 I2C devices connected to first 2 and last 2 pins) - UART - UART_HSI2C1 (first 2 pins are UART, rest is I2C) Also there doesn't seem to be any USI_CON or USI_OPTION registers in SPI, UART, or I2C. It seems like it's just the USI driver doing all the work (just setting up the SYSREG) and the I2C driver writing values to the SYSREG at suspend/resume for some reason.
On 18/11/2021 20:59, Sam Protsenko wrote: > On Tue, 16 Nov 2021 at 11:32, Krzysztof Kozlowski > <krzysztof.kozlowski@canonical.com> wrote: >> >> On 16/11/2021 02:12, Chanho Park wrote: >>>> With this patch the Exynos850 HSI2C becomes functional. The only nit-pick >>>> from my side (just a food for thought): do we want to configure USI >>>> related config inside of particular drivers (SPI, I2C, UART)? Or it would >>>> be better design to implement some platform driver for that, so we can >>>> choose USI configuration (SPI/I2C/UART) in device tree? I think this >>>> series is good to be merged as is, but we should probably consider all >>>> upsides and downsides of each option, for the future work. >>> >>> I'm also considering how to support this USI configuration gracefully. >>> Current version of USI is v2 which means there is a v1 version as well. It might be a non-upstream SoC so we don't need to consider it so far. >>> But, there is a possibility that the USI hw version can be bumped for future SoCs. >>> >>> As you probably know, earlier version of the product kernel has a USI SoC driver[1] and it was designed to be configured the USI settings by device tree. >>> >>> Option1) Make a USI driver under soc/samsung/ like [1]. >>> Option2) Use more generic driver such as "reset driver"? This might be required to extend the reset core driver. >>> Option3) Each USI driver(uart/i2c/spi) has its own USI configurations respectively and expose some configurations which can be variable as device tree. >>> >>> [1]: https://github.com/ianmacd/d2s/blob/master/drivers/soc/samsung/usi_v2.c >> >> I don't have user manuals, so all my knowledge here is based on >> Exynos9825 vendor source code, therefore it is quite limited. In >> devicetree the USI devices have their own nodes - but does it mean it's >> separate SFR range dedicated to USI? Looks like that, especially that >> address space is just for one register (4 bytes). >> >> In such case having separate dedicated driver makes sense and you would >> only have to care about driver ordering (e.g. via device links or phandles). >> >> Option 2 looks interesting - reusing reset framework to set proper USI >> mode, however this looks more like a hack. As you said Chanho, if there >> is a USI version 3, this reset framework might not be sufficient. >> >> In option 3 each driver (UART/I2C/SPI) would need to receive second IO >> range and toggle some registers, which could be done via shared >> function. If USI v3 is coming, all such drivers could get more complicated. >> >> I think option 1 is the cleanest and extendable in future. It's easy to >> add usi-v3 or whatever without modifying the UART/I2C/SPI drivers. It >> also nicely encapsulates USI-related stuff in separate driver. Probe >> ordering should not be a problem now. >> >> But as I said, I don't have even the big picture here, so I rely on your >> opinions more. >> > > Hi Krzysztof, > > Can you please let me know if you're going to apply this series as is, > or if you want me to submit USIv2 driver first, and then rework this > patch on top of it? I'm working on some HSI2C related patches right > now, and thus it'd nice to know about your decision on this series > beforehand, as some of my patches (like bindings doc patches) might > depend on it. Basically I'd like to base my patches on the proper > baseline, so we don't have to rebase those later. This set won't go via my tree anyway, but I am against it. David pointed out that his USIv1 is a little bit different and embedding in each of I2C/UART/SPI drivers the logic of controlling USIv1 and USIv2 looks too big. The solution with a dedicated driver looks to me more flexible and encapsulated/cleaner. Therefore after the discussions I am against this solution, so a soft-NAK from my side. Best regards, Krzysztof
On Fri, 19 Nov 2021 at 10:54, Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > On 18/11/2021 20:59, Sam Protsenko wrote: > > On Tue, 16 Nov 2021 at 11:32, Krzysztof Kozlowski > > <krzysztof.kozlowski@canonical.com> wrote: > >> > >> On 16/11/2021 02:12, Chanho Park wrote: > >>>> With this patch the Exynos850 HSI2C becomes functional. The only nit-pick > >>>> from my side (just a food for thought): do we want to configure USI > >>>> related config inside of particular drivers (SPI, I2C, UART)? Or it would > >>>> be better design to implement some platform driver for that, so we can > >>>> choose USI configuration (SPI/I2C/UART) in device tree? I think this > >>>> series is good to be merged as is, but we should probably consider all > >>>> upsides and downsides of each option, for the future work. > >>> > >>> I'm also considering how to support this USI configuration gracefully. > >>> Current version of USI is v2 which means there is a v1 version as well. It might be a non-upstream SoC so we don't need to consider it so far. > >>> But, there is a possibility that the USI hw version can be bumped for future SoCs. > >>> > >>> As you probably know, earlier version of the product kernel has a USI SoC driver[1] and it was designed to be configured the USI settings by device tree. > >>> > >>> Option1) Make a USI driver under soc/samsung/ like [1]. > >>> Option2) Use more generic driver such as "reset driver"? This might be required to extend the reset core driver. > >>> Option3) Each USI driver(uart/i2c/spi) has its own USI configurations respectively and expose some configurations which can be variable as device tree. > >>> > >>> [1]: https://github.com/ianmacd/d2s/blob/master/drivers/soc/samsung/usi_v2.c > >> > >> I don't have user manuals, so all my knowledge here is based on > >> Exynos9825 vendor source code, therefore it is quite limited. In > >> devicetree the USI devices have their own nodes - but does it mean it's > >> separate SFR range dedicated to USI? Looks like that, especially that > >> address space is just for one register (4 bytes). > >> > >> In such case having separate dedicated driver makes sense and you would > >> only have to care about driver ordering (e.g. via device links or phandles). > >> > >> Option 2 looks interesting - reusing reset framework to set proper USI > >> mode, however this looks more like a hack. As you said Chanho, if there > >> is a USI version 3, this reset framework might not be sufficient. > >> > >> In option 3 each driver (UART/I2C/SPI) would need to receive second IO > >> range and toggle some registers, which could be done via shared > >> function. If USI v3 is coming, all such drivers could get more complicated. > >> > >> I think option 1 is the cleanest and extendable in future. It's easy to > >> add usi-v3 or whatever without modifying the UART/I2C/SPI drivers. It > >> also nicely encapsulates USI-related stuff in separate driver. Probe > >> ordering should not be a problem now. > >> > >> But as I said, I don't have even the big picture here, so I rely on your > >> opinions more. > >> > > > > Hi Krzysztof, > > > > Can you please let me know if you're going to apply this series as is, > > or if you want me to submit USIv2 driver first, and then rework this > > patch on top of it? I'm working on some HSI2C related patches right > > now, and thus it'd nice to know about your decision on this series > > beforehand, as some of my patches (like bindings doc patches) might > > depend on it. Basically I'd like to base my patches on the proper > > baseline, so we don't have to rebase those later. > > This set won't go via my tree anyway, but I am against it. David pointed > out that his USIv1 is a little bit different and embedding in each of > I2C/UART/SPI drivers the logic of controlling USIv1 and USIv2 looks too > big. The solution with a dedicated driver looks to me more flexible and > encapsulated/cleaner. > > Therefore after the discussions I am against this solution, so a > soft-NAK from my side. > Hi Jaewon, I'm going to submit USI driver soon, and also some more HSI2C patches. Do you mind if I rework your patches to rely on USI drver (instead of modifying System Register in HSI2C driver), and include those in my patch series? Of course, I'll preserve your authorship. Just think that would be easier and faster this way. Thanks! > > Best regards, > Krzysztof
Hi Protsenko, > On Fri, 19 Nov 2021 at 10:54, Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > > > On 18/11/2021 20:59, Sam Protsenko wrote: > > > On Tue, 16 Nov 2021 at 11:32, Krzysztof Kozlowski > > > <krzysztof.kozlowski@canonical.com> wrote: > > >> > > >> On 16/11/2021 02:12, Chanho Park wrote: > > >>>> With this patch the Exynos850 HSI2C becomes functional. The only > > >>>> nit-pick from my side (just a food for thought): do we want to > > >>>> configure USI related config inside of particular drivers (SPI, > > >>>> I2C, UART)? Or it would be better design to implement some > > >>>> platform driver for that, so we can choose USI configuration > > >>>> (SPI/I2C/UART) in device tree? I think this series is good to be > > >>>> merged as is, but we should probably consider all upsides and downsides of each option, for the > future work. > > >>> > > >>> I'm also considering how to support this USI configuration gracefully. > > >>> Current version of USI is v2 which means there is a v1 version as well. It might be a non- > upstream SoC so we don't need to consider it so far. > > >>> But, there is a possibility that the USI hw version can be bumped for future SoCs. > > >>> > > >>> As you probably know, earlier version of the product kernel has a USI SoC driver[1] and it was > designed to be configured the USI settings by device tree. > > >>> > > >>> Option1) Make a USI driver under soc/samsung/ like [1]. > > >>> Option2) Use more generic driver such as "reset driver"? This might be required to extend the > reset core driver. > > >>> Option3) Each USI driver(uart/i2c/spi) has its own USI configurations respectively and expose > some configurations which can be variable as device tree. > > >>> > > >>> [1]: > > >>> https://protect2.fireeye.com/v1/url?k=b290a67b-ed0b9f6a-b2912d34-0 > > >>> cc47a31cdbc-ceadd8e62313162a&q=1&e=317825c0-3fac-46ad-9b4e-f93de42 > > >>> ad5ba&u=https%3A%2F%2Fgithub.com%2Fianmacd%2Fd2s%2Fblob%2Fmaster%2 > > >>> Fdrivers%2Fsoc%2Fsamsung%2Fusi_v2.c > > >> > > >> I don't have user manuals, so all my knowledge here is based on > > >> Exynos9825 vendor source code, therefore it is quite limited. In > > >> devicetree the USI devices have their own nodes - but does it mean > > >> it's separate SFR range dedicated to USI? Looks like that, > > >> especially that address space is just for one register (4 bytes). > > >> > > >> In such case having separate dedicated driver makes sense and you > > >> would only have to care about driver ordering (e.g. via device links or phandles). > > >> > > >> Option 2 looks interesting - reusing reset framework to set proper > > >> USI mode, however this looks more like a hack. As you said Chanho, > > >> if there is a USI version 3, this reset framework might not be sufficient. > > >> > > >> In option 3 each driver (UART/I2C/SPI) would need to receive second > > >> IO range and toggle some registers, which could be done via shared > > >> function. If USI v3 is coming, all such drivers could get more complicated. > > >> > > >> I think option 1 is the cleanest and extendable in future. It's > > >> easy to add usi-v3 or whatever without modifying the UART/I2C/SPI > > >> drivers. It also nicely encapsulates USI-related stuff in separate > > >> driver. Probe ordering should not be a problem now. > > >> > > >> But as I said, I don't have even the big picture here, so I rely on > > >> your opinions more. > > >> > > > > > > Hi Krzysztof, > > > > > > Can you please let me know if you're going to apply this series as > > > is, or if you want me to submit USIv2 driver first, and then rework > > > this patch on top of it? I'm working on some HSI2C related patches > > > right now, and thus it'd nice to know about your decision on this > > > series beforehand, as some of my patches (like bindings doc patches) > > > might depend on it. Basically I'd like to base my patches on the > > > proper baseline, so we don't have to rebase those later. > > > > This set won't go via my tree anyway, but I am against it. David > > pointed out that his USIv1 is a little bit different and embedding in > > each of I2C/UART/SPI drivers the logic of controlling USIv1 and USIv2 > > looks too big. The solution with a dedicated driver looks to me more > > flexible and encapsulated/cleaner. > > > > Therefore after the discussions I am against this solution, so a > > soft-NAK from my side. > > > > Hi Jaewon, > > I'm going to submit USI driver soon, and also some more HSI2C patches. > Do you mind if I rework your patches to rely on USI drver (instead of modifying System Register in > HSI2C driver), and include those in my patch series? Of course, I'll preserve your authorship. Just > think that would be easier and faster this way. > > Thanks! > I'm glad you're working on a USI driver. You can use my patchset. > > > > Best regards, > > Krzysztof Thanks Jaewon Kim
diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c index 97d4f3ac0abd..6ce94795a618 100644 --- a/drivers/i2c/busses/i2c-exynos5.c +++ b/drivers/i2c/busses/i2c-exynos5.c @@ -22,6 +22,8 @@ #include <linux/of_device.h> #include <linux/of_irq.h> #include <linux/spinlock.h> +#include <linux/mfd/syscon.h> +#include <linux/regmap.h> /* * HSI2C controller from Samsung supports 2 modes of operation @@ -166,9 +168,21 @@ #define EXYNOS5_I2C_TIMEOUT (msecs_to_jiffies(100)) +/* USI(Universal Serial Interface) Register map */ +#define USI_CON 0xc4 +#define USI_OPTION 0xc8 + +/* USI(Universal Serial Interface) Register bits */ +#define USI_CON_RESET BIT(0) + +/* SYSREG Register bit */ +#define SYSREG_USI_SW_CONF_MASK (0x7 << 0) +#define SYSREG_I2C_SW_CONF BIT(2) + enum i2c_type_exynos { I2C_TYPE_EXYNOS5, I2C_TYPE_EXYNOS7, + I2C_TYPE_EXYNOSAUTOV9, }; struct exynos5_i2c { @@ -199,6 +213,10 @@ struct exynos5_i2c { /* Version of HS-I2C Hardware */ const struct exynos_hsi2c_variant *variant; + + /* USI sysreg info */ + struct regmap *usi_sysreg; + unsigned int usi_offset; }; /** @@ -213,21 +231,31 @@ struct exynos5_i2c { struct exynos_hsi2c_variant { unsigned int fifo_depth; enum i2c_type_exynos hw; + bool has_usi; }; static const struct exynos_hsi2c_variant exynos5250_hsi2c_data = { .fifo_depth = 64, .hw = I2C_TYPE_EXYNOS5, + .has_usi = false, }; static const struct exynos_hsi2c_variant exynos5260_hsi2c_data = { .fifo_depth = 16, .hw = I2C_TYPE_EXYNOS5, + .has_usi = false, }; static const struct exynos_hsi2c_variant exynos7_hsi2c_data = { .fifo_depth = 16, .hw = I2C_TYPE_EXYNOS7, + .has_usi = false, +}; + +static const struct exynos_hsi2c_variant exynosautov9_hsi2c_data = { + .fifo_depth = 64, + .hw = I2C_TYPE_EXYNOSAUTOV9, + .has_usi = true, }; static const struct of_device_id exynos5_i2c_match[] = { @@ -243,6 +271,9 @@ static const struct of_device_id exynos5_i2c_match[] = { }, { .compatible = "samsung,exynos7-hsi2c", .data = &exynos7_hsi2c_data + }, { + .compatible = "samsung,exynosautov9-hsi2c", + .data = &exynosautov9_hsi2c_data }, {}, }; MODULE_DEVICE_TABLE(of, exynos5_i2c_match); @@ -281,6 +312,32 @@ static int exynos5_i2c_set_timing(struct exynos5_i2c *i2c, bool hs_timings) i2c->op_clock; int div, clk_cycle, temp; + /* + * In case of HSI2C controllers in EXYNOSAUTOV9 + * timing control formula changed. + * + * FSCL = IPCLK / ((CLK_DIV + 1) * 16) + * T_SCL_LOW = IPCLK * (CLK_DIV + 1) * (N + M) + * [N : number of 0's in the TSCL_H_HS] + * [M : number of 0's in the TSCL_L_HS] + * T_SCL_HIGH = IPCLK * (CLK_DIV + 1) * (N + M) + * [N : number of 1's in the TSCL_H_HS] + * [M : number of 1's in the TSCL_L_HS] + * + * result of (N + M) is always 8. + * In general use case, we don't need to control timing_s1, timing_s2. + */ + if (i2c->variant->hw == I2C_TYPE_EXYNOSAUTOV9) { + div = ((clkin / (16 * i2c->op_clock)) - 1); + i2c_timing_s3 = div << 16; + if (hs_timings) + writel(i2c_timing_s3, i2c->regs + HSI2C_TIMING_HS3); + else + writel(i2c_timing_s3, i2c->regs + HSI2C_TIMING_FS3); + + return 0; + } + /* * In case of HSI2C controller in Exynos5 series * FPCLK / FI2C = @@ -355,6 +412,20 @@ static int exynos5_hsi2c_clock_setup(struct exynos5_i2c *i2c) return exynos5_i2c_set_timing(i2c, true); } +static void exynos_usi_reset(struct exynos5_i2c *i2c) +{ + u32 val; + + val = readl(i2c->regs + USI_CON); + val |= USI_CON_RESET; + writel(val, i2c->regs + USI_CON); + udelay(1); + + val = readl(i2c->regs + USI_CON); + val &= ~USI_CON_RESET; + writel(val, i2c->regs + USI_CON); +} + /* * exynos5_i2c_init: configures the controller for I2C functionality * Programs I2C controller for Master mode operation @@ -385,6 +456,9 @@ static void exynos5_i2c_reset(struct exynos5_i2c *i2c) { u32 i2c_ctl; + if (i2c->variant->hw == I2C_TYPE_EXYNOSAUTOV9) + exynos_usi_reset(i2c); + /* Set and clear the bit for reset */ i2c_ctl = readl(i2c->regs + HSI2C_CTL); i2c_ctl |= HSI2C_SW_RST; @@ -422,7 +496,8 @@ static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id) writel(int_status, i2c->regs + HSI2C_INT_STATUS); /* handle interrupt related to the transfer status */ - if (i2c->variant->hw == I2C_TYPE_EXYNOS7) { + if (i2c->variant->hw == I2C_TYPE_EXYNOS7 || + i2c->variant->hw == I2C_TYPE_EXYNOSAUTOV9) { if (int_status & HSI2C_INT_TRANS_DONE) { i2c->trans_done = 1; i2c->state = 0; @@ -569,13 +644,13 @@ static void exynos5_i2c_bus_check(struct exynos5_i2c *i2c) { unsigned long timeout; - if (i2c->variant->hw != I2C_TYPE_EXYNOS7) + if (i2c->variant->hw == I2C_TYPE_EXYNOS5) return; /* - * HSI2C_MASTER_ST_LOSE state in EXYNOS7 variant before transaction - * indicates that bus is stuck (SDA is low). In such case bus recovery - * can be performed. + * HSI2C_MASTER_ST_LOSE state in EXYNOS7 or EXYNOSAUTOV9 variant before + * transaction indicates that bus is stuck (SDA is low). + * In such case bus recovery can be performed. */ timeout = jiffies + msecs_to_jiffies(100); for (;;) { @@ -611,10 +686,10 @@ static void exynos5_i2c_message_start(struct exynos5_i2c *i2c, int stop) unsigned long flags; unsigned short trig_lvl; - if (i2c->variant->hw == I2C_TYPE_EXYNOS7) - int_en |= HSI2C_INT_I2C_TRANS; - else + if (i2c->variant->hw == I2C_TYPE_EXYNOS5) int_en |= HSI2C_INT_I2C; + else + int_en |= HSI2C_INT_I2C_TRANS; i2c_ctl = readl(i2c->regs + HSI2C_CTL); i2c_ctl &= ~(HSI2C_TXCHON | HSI2C_RXCHON); @@ -738,6 +813,42 @@ static const struct i2c_algorithm exynos5_i2c_algorithm = { .functionality = exynos5_i2c_func, }; +static int exynos_usi_init(struct exynos5_i2c *i2c) +{ + struct device *dev = i2c->dev; + int ret; + + if (!i2c->variant->has_usi) + return 0; + + /* + * System Register has a field that can select the serial IP + * provided by USI. We need to set it to I2C to use I2C. + */ + i2c->usi_sysreg = syscon_regmap_lookup_by_phandle(dev->of_node, + "samsung,sysreg"); + if (IS_ERR(i2c->usi_sysreg)) { + dev_err(dev, "Cannot find sysreg\n"); + return PTR_ERR(i2c->usi_sysreg); + } + + ret = of_property_read_u32_index(dev->of_node, "samsung,sysreg", + 1, &i2c->usi_offset); + if (ret) { + dev_err(dev, "sysreg offset is not specified\n"); + return ret; + } + + ret = regmap_update_bits(i2c->usi_sysreg, i2c->usi_offset, + SYSREG_USI_SW_CONF_MASK, SYSREG_I2C_SW_CONF); + if (ret < 0) + return ret; + + exynos_usi_reset(i2c); + + return 0; +} + static int exynos5_i2c_probe(struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node; @@ -777,6 +888,12 @@ static int exynos5_i2c_probe(struct platform_device *pdev) i2c->adap.algo_data = i2c; i2c->adap.dev.parent = &pdev->dev; + i2c->variant = of_device_get_match_data(&pdev->dev); + + ret = exynos_usi_init(i2c); + if (ret) + return ret; + /* Clear pending interrupts from u-boot or misc causes */ exynos5_i2c_clr_pend_irq(i2c); @@ -794,8 +911,6 @@ static int exynos5_i2c_probe(struct platform_device *pdev) goto err_clk; } - i2c->variant = of_device_get_match_data(&pdev->dev); - ret = exynos5_hsi2c_clock_setup(i2c); if (ret) goto err_clk;
Serial IPs(UART, I2C, SPI) are integrated into New IP-Core called USI(Universal Serial Interface). As it is integrated into USI, there are additinal HW changes. Registers to control USI and sysreg to set serial IPs have been added. Also, some timing registres have been changed. Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com> --- drivers/i2c/busses/i2c-exynos5.c | 135 ++++++++++++++++++++++++++++--- 1 file changed, 125 insertions(+), 10 deletions(-)