On 17/07/2018 15:19:08+0300, Andy Shevchenko wrote:

On Tue, 2018-07-17 at 13:48 +0200, Alexandre Belloni wrote:

quoted

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.
 

quoted

 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.

quoted

 
+#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.

quoted

+	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.


-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com