Re: [PATCH v4 3/5] i2c: designware: Add slave definitions
On Thu, 2016-12-22 at 14:59 +, Luis Oliveira wrote:
> On 13-Dec-16 14:11, Rob Herring wrote:
> > Something like this:
> >
> > of_for_each_child_node(child) {
> > of_property_read_u32(child, "reg", ®);
> > if (reg & I2C_OWN_SLAVE_ADDRESS))
> > im_a_slave = true;
> > }
> >
> > ...rather than testing "mode" is equal to "slave".
> >
> > Rob
> >
>
> Hi Rob, Andy,
>
> I'm struggling to implement your suggestion @Rob. I checked the
> tegra124-jetson-tk1.dts that uses that approach but I have some
> doubts.
>
> My DT is as follows
>
> i2c@0x2000 {
> compatible = "snps,designware-i2c";
> reg = <0x2000 0x100>;
> clock-frequency = <40>;
> clocks = <&i2cclk>;
> interrupts = <0>;
>
> I could add something like this:
>
> eeprom@64 {
> compatible = "linux,slave-24c02";
> reg = <(I2C_OWN_SLAVE_ADDRESS | 0x64)>;
> }
>
> But I think this is different form what I was doing before. I have two
> questions:
>
> - This way the I2C controller is identified as a slave controller or
> just the
> subnode eeprom?
> - This way looks like my slave address will be fixed
>
> In the previous Patch v3 submission @Andy suggested a property that
> selects mode
> that I did and it's working. And you @Rob suggested to do it a common
> property.
> It is implemented in the DT like:
>
> mode = "slave";
>
> So before I do this changes can you please agree both if you still
> think this is
> the best approach?
I'm a bit lost in the discussion (and TBH busy by something else), so I
would agree on whatever you and Rob make an agreement on.
--
Andy Shevchenko
Intel Finland Oy
Re: [PATCH v4 3/5] i2c: designware: Add slave definitions
On 13-Dec-16 14:11, Rob Herring wrote:
> Again, please don't top post. And your line wrapping is messed up.
> IOW, you can't use Outlook.
>
> On Tue, Dec 13, 2016 at 4:50 AM, Luis de Oliveira
> wrote:
>> The controller for i2c-designware cannot be slave/master at the same time
>> and it has to be enabled knowing beforehand if we want it to be slave or
>> master by something outside of the controller itself.
>>
>> I as looking and I see the use of this I2C_OWN_SLAVE_ADDRESS with the
>> "linux,slave-24c02" slave interface but I am not seeing how it will help me
>> identify a selected i2c-designware block as a "slave" device before
>> instantiation. I'm sorry if I'm not understanding properly.
>> I use the "linux,slave-24c02" to instantiate the i2c-designware as a slave
>> with an address so I can do write/read operations, it is how I tested it.
>
> Something like this:
>
> of_for_each_child_node(child) {
> of_property_read_u32(child, "reg", ®);
> if (reg & I2C_OWN_SLAVE_ADDRESS))
> im_a_slave = true;
> }
>
> ...rather than testing "mode" is equal to "slave".
>
> Rob
>
Hi Rob, Andy,
I'm struggling to implement your suggestion @Rob. I checked the
tegra124-jetson-tk1.dts that uses that approach but I have some doubts.
My DT is as follows
i2c@0x2000 {
compatible = "snps,designware-i2c";
reg = <0x2000 0x100>;
clock-frequency = <40>;
clocks = <&i2cclk>;
interrupts = <0>;
I could add something like this:
eeprom@64 {
compatible = "linux,slave-24c02";
reg = <(I2C_OWN_SLAVE_ADDRESS | 0x64)>;
}
But I think this is different form what I was doing before. I have two
questions:
- This way the I2C controller is identified as a slave controller or just the
subnode eeprom?
- This way looks like my slave address will be fixed
In the previous Patch v3 submission @Andy suggested a property that selects mode
that I did and it's working. And you @Rob suggested to do it a common property.
It is implemented in the DT like:
mode = "slave";
So before I do this changes can you please agree both if you still think this is
the best approach?
Thank you both for your time,
Luis
>>
>> Luis
>>
>> -Original Message-
>> From: Rob Herring [mailto:r...@kernel.org]
>> Sent: Monday, December 12, 2016 23:16
>> To: Luis de Oliveira
>> Cc: w...@the-dreams.de; mark.rutl...@arm.com; jarkko.nik...@linux.intel.com;
>> andriy.shevche...@linux.intel.com; mika.westerb...@linux.intel.com;
>> linux-...@vger.kernel.org; devicet...@vger.kernel.org;
>> linux-kernel@vger.kernel.org; ramiro.olive...@synopsys.com;
>> joao.pi...@synopsys.com; carlos.palmi...@synopsys.com
>> Subject: Re: [PATCH v4 3/5] i2c: designware: Add slave definitions
>>
>> On Mon, Dec 12, 2016 at 12:35 PM, Luis de Oliveira
>> wrote:
>>> Hi all,
>>
>> Please don't top post.
>>
>>>
>>> The slave address could be set by the I2C slave backend so I can't use it
>>> to setup the controller.
>>> A boolean property was my initial approach then Andy and Wolfram Sang
>>> suggested the use of compatible strings and later It was suggested to use a
>>> property to select mode but I can do it again if it's the best way.
>>> Can you please tell me where should it be documented?
>>
>> bindings/i2c/i2c.txt.
>>
>> Actually, looking at this some more, we already have a way to describe the
>> controller being a slave device with the I2C_OWN_SLAVE_ADDRESS flag in the
>> reg property. We should just need a helper to read reg property of each
>> child and check for the bit set.
>>
>> Rob
Re: [PATCH v4 3/5] i2c: designware: Add slave definitions
Again, please don't top post. And your line wrapping is messed up.
IOW, you can't use Outlook.
On Tue, Dec 13, 2016 at 4:50 AM, Luis de Oliveira
wrote:
> The controller for i2c-designware cannot be slave/master at the same time and
> it has to be enabled knowing beforehand if we want it to be slave or master
> by something outside of the controller itself.
>
> I as looking and I see the use of this I2C_OWN_SLAVE_ADDRESS with the
> "linux,slave-24c02" slave interface but I am not seeing how it will help me
> identify a selected i2c-designware block as a "slave" device before
> instantiation. I'm sorry if I'm not understanding properly.
> I use the "linux,slave-24c02" to instantiate the i2c-designware as a slave
> with an address so I can do write/read operations, it is how I tested it.
Something like this:
of_for_each_child_node(child) {
of_property_read_u32(child, "reg", ®);
if (reg & I2C_OWN_SLAVE_ADDRESS))
im_a_slave = true;
}
...rather than testing "mode" is equal to "slave".
Rob
>
> Luis
>
> -Original Message-
> From: Rob Herring [mailto:r...@kernel.org]
> Sent: Monday, December 12, 2016 23:16
> To: Luis de Oliveira
> Cc: w...@the-dreams.de; mark.rutl...@arm.com; jarkko.nik...@linux.intel.com;
> andriy.shevche...@linux.intel.com; mika.westerb...@linux.intel.com;
> linux-...@vger.kernel.org; devicet...@vger.kernel.org;
> linux-kernel@vger.kernel.org; ramiro.olive...@synopsys.com;
> joao.pi...@synopsys.com; carlos.palmi...@synopsys.com
> Subject: Re: [PATCH v4 3/5] i2c: designware: Add slave definitions
>
> On Mon, Dec 12, 2016 at 12:35 PM, Luis de Oliveira
> wrote:
>> Hi all,
>
> Please don't top post.
>
>>
>> The slave address could be set by the I2C slave backend so I can't use it to
>> setup the controller.
>> A boolean property was my initial approach then Andy and Wolfram Sang
>> suggested the use of compatible strings and later It was suggested to use a
>> property to select mode but I can do it again if it's the best way.
>> Can you please tell me where should it be documented?
>
> bindings/i2c/i2c.txt.
>
> Actually, looking at this some more, we already have a way to describe the
> controller being a slave device with the I2C_OWN_SLAVE_ADDRESS flag in the
> reg property. We should just need a helper to read reg property of each child
> and check for the bit set.
>
> Rob
RE: [PATCH v4 3/5] i2c: designware: Add slave definitions
The controller for i2c-designware cannot be slave/master at the same time and it has to be enabled knowing beforehand if we want it to be slave or master by something outside of the controller itself. I as looking and I see the use of this I2C_OWN_SLAVE_ADDRESS with the "linux,slave-24c02" slave interface but I am not seeing how it will help me identify a selected i2c-designware block as a "slave" device before instantiation. I'm sorry if I'm not understanding properly. I use the "linux,slave-24c02" to instantiate the i2c-designware as a slave with an address so I can do write/read operations, it is how I tested it. Luis -Original Message- From: Rob Herring [mailto:r...@kernel.org] Sent: Monday, December 12, 2016 23:16 To: Luis de Oliveira Cc: w...@the-dreams.de; mark.rutl...@arm.com; jarkko.nik...@linux.intel.com; andriy.shevche...@linux.intel.com; mika.westerb...@linux.intel.com; linux-...@vger.kernel.org; devicet...@vger.kernel.org; linux-kernel@vger.kernel.org; ramiro.olive...@synopsys.com; joao.pi...@synopsys.com; carlos.palmi...@synopsys.com Subject: Re: [PATCH v4 3/5] i2c: designware: Add slave definitions On Mon, Dec 12, 2016 at 12:35 PM, Luis de Oliveira wrote: > Hi all, Please don't top post. > > The slave address could be set by the I2C slave backend so I can't use it to > setup the controller. > A boolean property was my initial approach then Andy and Wolfram Sang > suggested the use of compatible strings and later It was suggested to use a > property to select mode but I can do it again if it's the best way. > Can you please tell me where should it be documented? bindings/i2c/i2c.txt. Actually, looking at this some more, we already have a way to describe the controller being a slave device with the I2C_OWN_SLAVE_ADDRESS flag in the reg property. We should just need a helper to read reg property of each child and check for the bit set. Rob
Re: [PATCH v4 3/5] i2c: designware: Add slave definitions
On Mon, Dec 12, 2016 at 12:35 PM, Luis de Oliveira wrote: > Hi all, Please don't top post. > > The slave address could be set by the I2C slave backend so I can't use it to > setup the controller. > A boolean property was my initial approach then Andy and Wolfram Sang > suggested the use of compatible strings and later It was suggested to use a > property to select mode but I can do it again if it's the best way. > Can you please tell me where should it be documented? bindings/i2c/i2c.txt. Actually, looking at this some more, we already have a way to describe the controller being a slave device with the I2C_OWN_SLAVE_ADDRESS flag in the reg property. We should just need a helper to read reg property of each child and check for the bit set. Rob
RE: [PATCH v4 3/5] i2c: designware: Add slave definitions
Hi all, The slave address could be set by the I2C slave backend so I can't use it to setup the controller. A boolean property was my initial approach then Andy and Wolfram Sang suggested the use of compatible strings and later It was suggested to use a property to select mode but I can do it again if it's the best way. Can you please tell me where should it be documented? Luis -Original Message- From: Rob Herring [mailto:r...@kernel.org] Sent: Monday, December 12, 2016 17:02 To: Luis Oliveira Cc: w...@the-dreams.de; mark.rutl...@arm.com; jarkko.nik...@linux.intel.com; andriy.shevche...@linux.intel.com; mika.westerb...@linux.intel.com; linux-...@vger.kernel.org; devicet...@vger.kernel.org; linux-kernel@vger.kernel.org; ramiro.olive...@synopsys.com; joao.pi...@synopsys.com; carlos.palmi...@synopsys.com Subject: Re: [PATCH v4 3/5] i2c: designware: Add slave definitions On Wed, Dec 07, 2016 at 05:55:50PM +, Luis Oliveira wrote: > - Add slave definitions to i2c-designware-core > - Changes in Kconfig to auto-enable I2C_SLAVE when compiling the > modules > - Add mode property to designware-core.txt that enable the "slave" selection: > - "mode" is an optional property that could be "slave" or "master" > - if "mode" is not set the block is considered master by default > > Signed-off-by: Luis Oliveira > --- > Changes V3->V4: (Andy Shevchenko) > - created a common property for modes > - placed the generic dependency first > > .../devicetree/bindings/i2c/i2c-designware.txt | 4 > drivers/i2c/busses/Kconfig | 1 + > drivers/i2c/busses/i2c-designware-common.c | 6 + > drivers/i2c/busses/i2c-designware-core.h | 26 > ++ > 4 files changed, 37 insertions(+) > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt > b/Documentation/devicetree/bindings/i2c/i2c-designware.txt > index fee26dc3e858..8ed2b532cd54 100644 > --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt > +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt > @@ -20,6 +20,9 @@ Optional properties : > - i2c-sda-falling-time-ns : should contain the SDA falling time in > nanoseconds. > This value which is by default 300ns is used to compute the tHIGH period. > > + - mode : should be either: > + - "master" to setup the hardware block as a I2C master > + - "slave" to setup the hardware block as a I2C slave This should be documented in a common location. Can't it be a boolean to enable slave mode? Or don't you need to set the slave address? That could be enough to enable slave mode and there's already one example doing that. Rob
Re: [PATCH v4 3/5] i2c: designware: Add slave definitions
On Wed, Dec 07, 2016 at 05:55:50PM +, Luis Oliveira wrote: > - Add slave definitions to i2c-designware-core > - Changes in Kconfig to auto-enable I2C_SLAVE when compiling the modules > - Add mode property to designware-core.txt that enable the "slave" selection: > - "mode" is an optional property that could be "slave" or "master" > - if "mode" is not set the block is considered master by default > > Signed-off-by: Luis Oliveira > --- > Changes V3->V4: (Andy Shevchenko) > - created a common property for modes > - placed the generic dependency first > > .../devicetree/bindings/i2c/i2c-designware.txt | 4 > drivers/i2c/busses/Kconfig | 1 + > drivers/i2c/busses/i2c-designware-common.c | 6 + > drivers/i2c/busses/i2c-designware-core.h | 26 > ++ > 4 files changed, 37 insertions(+) > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt > b/Documentation/devicetree/bindings/i2c/i2c-designware.txt > index fee26dc3e858..8ed2b532cd54 100644 > --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt > +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt > @@ -20,6 +20,9 @@ Optional properties : > - i2c-sda-falling-time-ns : should contain the SDA falling time in > nanoseconds. > This value which is by default 300ns is used to compute the tHIGH period. > > + - mode : should be either: > + - "master" to setup the hardware block as a I2C master > + - "slave" to setup the hardware block as a I2C slave This should be documented in a common location. Can't it be a boolean to enable slave mode? Or don't you need to set the slave address? That could be enough to enable slave mode and there's already one example doing that. Rob
Re: [PATCH v4 3/5] i2c: designware: Add slave definitions
On Wed, 2016-12-07 at 17:55 +, Luis Oliveira wrote:
> - Add slave definitions to i2c-designware-core
> - Changes in Kconfig to auto-enable I2C_SLAVE when compiling the
> modules
> - Add mode property to designware-core.txt that enable the "slave"
> selection:
> - "mode" is an optional property that could be "slave" or "master"
> - if "mode" is not set the block is considered master by default
>
> Signed-off-by: Luis Oliveira
I'm okay with the DT portion (still needs Ack from DT people), but the
problem with the patch that you break bisectability, i.e. you introduce
pieces of code that are not present ATM. So, this should go *after*
actual slave patch.
> --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> @@ -20,6 +20,9 @@ Optional properties :
> - i2c-sda-falling-time-ns : should contain the SDA falling time in
> nanoseconds.
> This value which is by default 300ns is used to compute the tHIGH
> period.
>
> + - mode : should be either:
> + - "master" to setup the hardware block as a I2C master
> + - "slave" to setup the hardware block as a I2C slave
> Example :
>
> i2c@f {
> @@ -42,4 +45,5 @@ Example :
> i2c-sda-hold-time-ns = <300>;
> i2c-sda-falling-time-ns = <300>;
> i2c-scl-falling-time-ns = <300>;
> + mode = "slave";
I would suggest to use "master" here, since most common use is master.
> };
So, the above can go to the patch
"... Add new property to describe mode"
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -469,6 +469,7 @@ config I2C_DESIGNWARE_CORE
>
> config I2C_DESIGNWARE_PLATFORM
> tristate "Synopsys DesignWare Platform"
> + select I2C_SLAVE
>
> --- a/drivers/i2c/busses/i2c-designware-common.c
> +++ b/drivers/i2c/busses/i2c-designware-common.c
> @@ -55,6 +55,12 @@ static char *abort_sources[] = {
> "trying to use disabled adapter",
> [ARB_LOST] =
> "lost arbitration",
> + [ABRT_SLAVE_FLUSH_TXFIFO] =
> + "read command so flush old data in the TX FIFO",
> + [ABRT_SLAVE_ARBLOST] =
> + "slave lost the bus while transmitting data to a
> remote master",
> + [ABRT_SLAVE_RD_INTX] =
> + "slave request for data to be transmitted and",
These are part of slave patch.
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -36,15 +36,20 @@
> #define DW_IC_CON_SPEED_FAST 0x4
> #define DW_IC_CON_SPEED_HIGH 0x6
> #define DW_IC_CON_SPEED_MASK 0x6
> +#define DW_IC_CON_10BITADDR_SLAVE0x8
All definitions would be split to a patch like
"... Introduce definitions for i2c slave mode"
> @@ -206,6 +225,7 @@ struct dw_i2c_dev {
> void __iomem*base;
> struct completion cmd_complete;
> struct clk *clk;
> + struct i2c_client *slave;
>
> @@ -225,6 +245,7 @@ struct dw_i2c_dev {
> struct i2c_adapter adapter;
> u32 functionality;
> u32 master_cfg;
> + u32 slave_cfg;
>
> +extern int i2c_dw_init_slave(struct dw_i2c_dev *dev);
> +extern void i2c_dw_disable_slave(struct dw_i2c_dev *dev);
> +extern void i2c_dw_disable_int_slave(struct dw_i2c_dev *dev);
> +extern u32 i2c_dw_read_comp_param_slave(struct dw_i2c_dev *dev);
> +extern int i2c_dw_probe_slave(struct dw_i2c_dev *dev);
The above is a part of slave patch.
--
Andy Shevchenko
Intel Finland Oy
