| Message ID | 20180717114837.21839-4-alexandre.belloni@bootlin.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add support for MSCC Ocelot i2c | expand |
On Tue, 2018-07-17 at 13:48 +0200, Alexandre Belloni wrote: > The Microsemi Ocelot I2C controller is a designware IP. It also has a > second set of registers to allow tweaking SDA hold time and spike > filtering. Can you elaborate a bit? Are they platform specific? Are they shadow registers? Are they something else? Datasheet link / excerpt would be also good to read. > Optional properties : > + - reg : for "mscc,ocelot-i2c", a second register set to configure > the SDA hold > + time, named ICPU_CFG:TWI_DELAY in the datasheet. > + Hmm... Is this registers unique to the SoC in question? Is address of them fixed or may be configured on RTL level? If former is right, why do we need a separate property? > > +#define MSCC_ICPU_CFG_TWI_DELAY 0x0 > +#define MSCC_ICPU_CFG_TWI_DELAY_ENABLE BIT(0) > +#define MSCC_ICPU_CFG_TWI_SPIKE_FILTER 0x4 > + > +static int mscc_twi_set_sda_hold_time(struct dw_i2c_dev *dev) > +{ > + writel((dev->sda_hold_time << 1) | > MSCC_ICPU_CFG_TWI_DELAY_ENABLE, > + dev->base_ext + MSCC_ICPU_CFG_TWI_DELAY); > + > + return 0; > +} Hmm... And does how this make native DesignWare IP's registers obsolete? > + if (of_device_is_compatible(pdev->dev.of_node, "mscc,ocelot- > i2c")) Can't you just ask for this unconditionally? Why not? (It seems I might have known why not, but can we use named resource instead in case this is not so SoC specific)
On 17/07/2018 15:19:08+0300, Andy Shevchenko wrote: > On Tue, 2018-07-17 at 13:48 +0200, Alexandre Belloni wrote: > > The Microsemi Ocelot I2C controller is a designware IP. It also has a > > second set of registers to allow tweaking SDA hold time and spike > > filtering. > > Can you elaborate a bit? > > Are they platform specific? Are they shadow registers? Are they > something else? Datasheet link / excerpt would be also good to read. > > > Optional properties : > > + - reg : for "mscc,ocelot-i2c", a second register set to configure > > the SDA hold > > + time, named ICPU_CFG:TWI_DELAY in the datasheet. > > + > > Hmm... Is this registers unique to the SoC in question? Is address of > them fixed or may be configured on RTL level? > > If former is right, why do we need a separate property? > Those are registers from the SoC, their position varies depending on the SoC. Even if the position was fixed, I'm pretty sure another register set is needed. It is not a new property. > > > > +#define MSCC_ICPU_CFG_TWI_DELAY 0x0 > > +#define MSCC_ICPU_CFG_TWI_DELAY_ENABLE BIT(0) > > +#define MSCC_ICPU_CFG_TWI_SPIKE_FILTER 0x4 > > + > > +static int mscc_twi_set_sda_hold_time(struct dw_i2c_dev *dev) > > +{ > > + writel((dev->sda_hold_time << 1) | > > MSCC_ICPU_CFG_TWI_DELAY_ENABLE, > > + dev->base_ext + MSCC_ICPU_CFG_TWI_DELAY); > > + > > + return 0; > > +} > > Hmm... And does how this make native DesignWare IP's registers obsolete? > DW_IC_SDA_HOLD doesn't exist in this version of the IP. It is replaced by this SoC specific register. > > > + if (of_device_is_compatible(pdev->dev.of_node, "mscc,ocelot- > > i2c")) > > Can't you just ask for this unconditionally? Why not? > (It seems I might have known why not, but can we use named resource > instead in case this is not so SoC specific) > It is SoC specific.
On Tue, 2018-07-17 at 18:16 +0300, Andy Shevchenko wrote: > On Tue, 2018-07-17 at 13:48 +0200, Alexandre Belloni wrote: > > The Microsemi Ocelot I2C controller is a designware IP. It also has > > a > > second set of registers to allow tweaking SDA hold time and spike > > filtering. > What do you think? You can also split it to 2-3 patches, like: - move to device_get_match_data() - move OF device table above in the code (no func changes) - add support for MSCC
On Tue, Jul 17, 2018 at 01:48:35PM +0200, Alexandre Belloni wrote: > The Microsemi Ocelot I2C controller is a designware IP. It also has a > second set of registers to allow tweaking SDA hold time and spike > filtering. > > Cc: Rob Herring <robh+dt@kernel.org> > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > --- > .../bindings/i2c/i2c-designware.txt | 5 ++++- > drivers/i2c/busses/i2c-designware-core.h | 1 + > drivers/i2c/busses/i2c-designware-platdrv.c | 20 +++++++++++++++++++ > 3 files changed, 25 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt > index fbb0a6d8b964..7af4176da4af 100644 > --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt > +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt > @@ -2,7 +2,7 @@ > > Required properties : > > - - compatible : should be "snps,designware-i2c" > + - compatible : should be "snps,designware-i2c" or "mscc,ocelot-i2c" Sounds like the registers are optional (or could be initialized by firmware), so shouldn't 'snps,designware-i2c' be a fallback compatible? > - reg : Offset and length of the register set for the device > - interrupts : <IRQ> where IRQ is the interrupt number. > > @@ -11,6 +11,9 @@ Recommended properties : > - clock-frequency : desired I2C bus clock frequency in Hz. > > Optional properties : > + - reg : for "mscc,ocelot-i2c", a second register set to configure the SDA hold > + time, named ICPU_CFG:TWI_DELAY in the datasheet. > + > - i2c-sda-hold-time-ns : should contain the SDA hold time in nanoseconds. > This option is only supported in hardware blocks version 1.11a or newer. Perhaps this needs an update too? It sounds like Microsemi fixed this problem on their own before version 1.11a of the IP block. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt index fbb0a6d8b964..7af4176da4af 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt @@ -2,7 +2,7 @@ Required properties : - - compatible : should be "snps,designware-i2c" + - compatible : should be "snps,designware-i2c" or "mscc,ocelot-i2c" - reg : Offset and length of the register set for the device - interrupts : <IRQ> where IRQ is the interrupt number. @@ -11,6 +11,9 @@ Recommended properties : - clock-frequency : desired I2C bus clock frequency in Hz. Optional properties : + - reg : for "mscc,ocelot-i2c", a second register set to configure the SDA hold + time, named ICPU_CFG:TWI_DELAY in the datasheet. + - i2c-sda-hold-time-ns : should contain the SDA hold time in nanoseconds. This option is only supported in hardware blocks version 1.11a or newer. diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index b2778b6d8aca..37f63d084c1d 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -237,6 +237,7 @@ struct dw_i2c_dev { struct device *dev; void __iomem *base; + void __iomem *base_ext; struct completion cmd_complete; struct clk *clk; struct reset_control *rst; diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index 5660daf6c92e..1a476a6e3551 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -204,6 +204,18 @@ static void i2c_dw_configure_slave(struct dw_i2c_dev *dev) dev->mode = DW_IC_SLAVE; } +#define MSCC_ICPU_CFG_TWI_DELAY 0x0 +#define MSCC_ICPU_CFG_TWI_DELAY_ENABLE BIT(0) +#define MSCC_ICPU_CFG_TWI_SPIKE_FILTER 0x4 + +static int mscc_twi_set_sda_hold_time(struct dw_i2c_dev *dev) +{ + writel((dev->sda_hold_time << 1) | MSCC_ICPU_CFG_TWI_DELAY_ENABLE, + dev->base_ext + MSCC_ICPU_CFG_TWI_DELAY); + + return 0; +} + static void dw_i2c_set_fifo_size(struct dw_i2c_dev *dev, int id) { u32 param, tx_fifo_depth, rx_fifo_depth; @@ -342,6 +354,13 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) 1000000); } + if (of_device_is_compatible(pdev->dev.of_node, "mscc,ocelot-i2c")) { + mem = platform_get_resource(pdev, IORESOURCE_MEM, 1); + dev->base_ext = devm_ioremap_resource(&pdev->dev, mem); + if (!IS_ERR(dev->base_ext)) + dev->set_sda_hold_time = mscc_twi_set_sda_hold_time; + } + dw_i2c_set_fifo_size(dev, pdev->id); adap = &dev->adapter; @@ -410,6 +429,7 @@ static int dw_i2c_plat_remove(struct platform_device *pdev) #ifdef CONFIG_OF static const struct of_device_id dw_i2c_of_match[] = { { .compatible = "snps,designware-i2c", }, + { .compatible = "mscc,ocelot-i2c", }, {}, }; MODULE_DEVICE_TABLE(of, dw_i2c_of_match);
The Microsemi Ocelot I2C controller is a designware IP. It also has a second set of registers to allow tweaking SDA hold time and spike filtering. Cc: Rob Herring <robh+dt@kernel.org> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com> --- .../bindings/i2c/i2c-designware.txt | 5 ++++- drivers/i2c/busses/i2c-designware-core.h | 1 + drivers/i2c/busses/i2c-designware-platdrv.c | 20 +++++++++++++++++++ 3 files changed, 25 insertions(+), 1 deletion(-)