mbox series

[v3,0/7] Add Aspeed SSIF BMC driver

Message ID 20210519074934.20712-1-quan@os.amperecomputing.com
Headers show
Series Add Aspeed SSIF BMC driver | expand

Message

Quan Nguyen May 19, 2021, 7:49 a.m. UTC
This series add support for the Aspeed specific SSIF BMC driver which
is to perform in-band IPMI communication with the host in management
(BMC) side.

v3:
  + Switched binding doc to use DT schema format [Rob]
  + Splited into generic ssif_bmc and aspeed-specific [Corey, Joel]
  + Removed redundant license info [Joel]
  + Switched to use traditional if-else [Joel]
  + Removed unused ssif_bmc_ioctl() [Joel]
  + Made handle_request()/complete_response() to return void [Joel]
  + Refactored send_ssif_bmc_response()/receive_ssif_bmc_request() [Corey]
  + Remove mutex [Corey]
  + Use spin_lock/unlock_irqsave/restore in callback [Corey]
  + Removed the unnecessary memset [Corey]
  + Switch to use dev_err() [Corey]
  + Combine mask/unmask two interrupts together [Corey]
  + Fixed unhandled Tx done with NAK [Quan]
  + Late ack'ed Tx done w/wo Ack irq [Quan]
  + Use aspeed-specific exported aspeed_set_slave_busy() when slave busy
to fix the deadlock [Graeme, Philipp, Quan]
  + Clean buffer for last multipart read [Quan]
  + Handle unknown incoming command [Quan]

v2:
  + Fixed compiling error with COMPILE_TEST for arc

Quan Nguyen (7):
  i2c: i2c-core-smbus: Expose PEC calculate function for generic use
  ipmi: ssif_bmc: Add SSIF BMC driver
  i2c: aspeed: Fix unhandled Tx done with NAK
  i2c: aspeed: Acknowledge Tx done w/wo ACK irq late
  i2c: aspeed: Add aspeed_set_slave_busy()
  ipmi: ssif_bmc: Add Aspeed SSIF BMC driver
  bindings: ipmi: Add binding for Aspeed SSIF BMC driver

 .../bindings/ipmi/aspeed-ssif-bmc.yaml        |  33 +
 drivers/char/ipmi/Kconfig                     |  22 +
 drivers/char/ipmi/Makefile                    |   2 +
 drivers/char/ipmi/ssif_bmc.c                  | 605 ++++++++++++++++++
 drivers/char/ipmi/ssif_bmc.h                  |  93 +++
 drivers/char/ipmi/ssif_bmc_aspeed.c           |  75 +++
 drivers/i2c/busses/i2c-aspeed.c               |  51 +-
 drivers/i2c/i2c-core-smbus.c                  |  12 +-
 include/linux/i2c.h                           |   1 +
 9 files changed, 884 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.yaml
 create mode 100644 drivers/char/ipmi/ssif_bmc.c
 create mode 100644 drivers/char/ipmi/ssif_bmc.h
 create mode 100644 drivers/char/ipmi/ssif_bmc_aspeed.c

Comments

Corey Minyard May 19, 2021, 12:34 p.m. UTC | #1
On Wed, May 19, 2021 at 02:49:27PM +0700, Quan Nguyen wrote:
> This series add support for the Aspeed specific SSIF BMC driver which
> is to perform in-band IPMI communication with the host in management
> (BMC) side.
> 
> v3:
>   + Switched binding doc to use DT schema format [Rob]
>   + Splited into generic ssif_bmc and aspeed-specific [Corey, Joel]
>   + Removed redundant license info [Joel]
>   + Switched to use traditional if-else [Joel]
>   + Removed unused ssif_bmc_ioctl() [Joel]
>   + Made handle_request()/complete_response() to return void [Joel]
>   + Refactored send_ssif_bmc_response()/receive_ssif_bmc_request() [Corey]
>   + Remove mutex [Corey]
>   + Use spin_lock/unlock_irqsave/restore in callback [Corey]
>   + Removed the unnecessary memset [Corey]
>   + Switch to use dev_err() [Corey]
>   + Combine mask/unmask two interrupts together [Corey]
>   + Fixed unhandled Tx done with NAK [Quan]
>   + Late ack'ed Tx done w/wo Ack irq [Quan]
>   + Use aspeed-specific exported aspeed_set_slave_busy() when slave busy
> to fix the deadlock [Graeme, Philipp, Quan]
>   + Clean buffer for last multipart read [Quan]
>   + Handle unknown incoming command [Quan]
> 
> v2:
>   + Fixed compiling error with COMPILE_TEST for arc
> 
> Quan Nguyen (7):
>   i2c: i2c-core-smbus: Expose PEC calculate function for generic use

Note that for this I2C-specific change, I will need acks from the I2C
maintainers to be able to include this in my tree.

>   ipmi: ssif_bmc: Add SSIF BMC driver
>   i2c: aspeed: Fix unhandled Tx done with NAK

For the aspeed changes, they don't really belong here, they belong in
the aspeed tree.  I see that you need them for the device to work
correctly, though.  I'll need acks from maintainers to include them.

>   i2c: aspeed: Acknowledge Tx done w/wo ACK irq late
>   i2c: aspeed: Add aspeed_set_slave_busy()
>   ipmi: ssif_bmc: Add Aspeed SSIF BMC driver
>   bindings: ipmi: Add binding for Aspeed SSIF BMC driver
> 
>  .../bindings/ipmi/aspeed-ssif-bmc.yaml        |  33 +
>  drivers/char/ipmi/Kconfig                     |  22 +
>  drivers/char/ipmi/Makefile                    |   2 +
>  drivers/char/ipmi/ssif_bmc.c                  | 605 ++++++++++++++++++
>  drivers/char/ipmi/ssif_bmc.h                  |  93 +++
>  drivers/char/ipmi/ssif_bmc_aspeed.c           |  75 +++
>  drivers/i2c/busses/i2c-aspeed.c               |  51 +-
>  drivers/i2c/i2c-core-smbus.c                  |  12 +-
>  include/linux/i2c.h                           |   1 +
>  9 files changed, 884 insertions(+), 10 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.yaml
>  create mode 100644 drivers/char/ipmi/ssif_bmc.c
>  create mode 100644 drivers/char/ipmi/ssif_bmc.h
>  create mode 100644 drivers/char/ipmi/ssif_bmc_aspeed.c
> 
> -- 
> 2.28.0
>
Rob Herring May 19, 2021, 3:29 p.m. UTC | #2
On Wed, 19 May 2021 14:49:34 +0700, Quan Nguyen wrote:
> Add device tree binding document for the Aspeed SSIF BMC driver.
> 
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> ---
> v3:
>   + Switched to use DT schema format [Rob]
> 
>  .../bindings/ipmi/aspeed-ssif-bmc.yaml        | 33 +++++++++++++++++++
>  1 file changed, 33 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.yaml
> 

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/ipmi/aspeed-ssif-bmc.example.dts:21.13-26: Warning (reg_format): /example-0/ssif-bmc@10:reg: property has invalid length (4 bytes) (#address-cells == 1, #size-cells == 1)
Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.example.dt.yaml: Warning (pci_device_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.example.dt.yaml: Warning (pci_device_bus_num): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.example.dt.yaml: Warning (simple_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.example.dt.yaml: Warning (i2c_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.example.dt.yaml: Warning (spi_bus_reg): Failed prerequisite 'reg_format'
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.example.dt.yaml: example-0: ssif-bmc@10:reg:0: [16] is too short
	From schema: /usr/local/lib/python3.8/dist-packages/dtschema/schemas/reg.yaml

See https://patchwork.ozlabs.org/patch/1480727

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.
Ryan Chen May 20, 2021, 11:06 a.m. UTC | #3
> -----Original Message-----

> From: openbmc

> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On Behalf

> Of Quan Nguyen

> Sent: Wednesday, May 19, 2021 3:50 PM

> To: Corey Minyard <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>;

> Joel Stanley <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan

> Higgins <brendanhiggins@google.com>; Benjamin Herrenschmidt

> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel

> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net;

> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;

> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;

> linux-i2c@vger.kernel.org

> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .

> Nguyen <thang@os.amperecomputing.com>; Phong Vo

> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org

> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

> 

> Slave i2c device on AST2500 received a lot of slave irq while it is busy

> processing the response. To handle this case, adds and exports

> aspeed_set_slave_busy() for controller to temporary stop slave irq while slave

> is handling the response, and re-enable them again when the response is ready.

> 

> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>

> ---

> v3:

>   + First introduce in v3 [Quan]

> 

>  drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++

>  1 file changed, 20 insertions(+)

> 

> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c

> index b2e9c8f0ddf7..9926d04831a2 100644

> --- a/drivers/i2c/busses/i2c-aspeed.c

> +++ b/drivers/i2c/busses/i2c-aspeed.c

> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus

> *bus,

>  	return 0;

>  }

> 

> +#if IS_ENABLED(CONFIG_I2C_SLAVE)

> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) {

> +	struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);

> +	unsigned long current_mask, flags;

> +

> +	spin_lock_irqsave(&bus->lock, flags);

> +

> +	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);

> +	if (busy)

> +		current_mask &= ~(ASPEED_I2CD_INTR_RX_DONE |

> ASPEED_I2CD_INTR_SLAVE_MATCH);

> +	else

> +		current_mask |= ASPEED_I2CD_INTR_RX_DONE |

> ASPEED_I2CD_INTR_SLAVE_MATCH;

> +	writel(current_mask, bus->base + ASPEED_I2C_INTR_CTRL_REG);

> +

> +	spin_unlock_irqrestore(&bus->lock, flags); }

> +EXPORT_SYMBOL_GPL(aspeed_set_slave_busy);

> +#endif

> +

>  static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus)  {

>  	struct platform_device *pdev = to_platform_device(bus->dev);

> --

> 2.28.0


Hello,
	The better idea is use disable i2c slave mode. 
	Due to if i2c controller running in slave will get slave match, and latch the SCL.
	Until cpu clear interrupt status. 
Ryan
Ryan Chen May 20, 2021, 11:28 a.m. UTC | #4
> -----Original Message-----

> From: Joel Stanley <joel@jms.id.au>

> Sent: Thursday, May 20, 2021 7:29 AM

> To: Quan Nguyen <quan@os.amperecomputing.com>; Ryan Chen

> <ryan_chen@aspeedtech.com>

> Cc: Corey Minyard <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>;

> Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins

> <brendanhiggins@google.com>; Benjamin Herrenschmidt

> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel

> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net;

> devicetree <devicetree@vger.kernel.org>; Linux ARM

> <linux-arm-kernel@lists.infradead.org>; linux-aspeed

> <linux-aspeed@lists.ozlabs.org>; Linux Kernel Mailing List

> <linux-kernel@vger.kernel.org>; linux-i2c@vger.kernel.org; Open Source

> Submission <patches@amperecomputing.com>; Phong Vo

> <phong@os.amperecomputing.com>; Thang Q . Nguyen

> <thang@os.amperecomputing.com>; OpenBMC Maillist

> <openbmc@lists.ozlabs.org>

> Subject: Re: [PATCH v3 3/7] i2c: aspeed: Fix unhandled Tx done with NAK

> 

> Ryan, can you please review this change?

> 

> On Wed, 19 May 2021 at 07:50, Quan Nguyen

> <quan@os.amperecomputing.com> wrote:

> >

> > It is observed that in normal condition, when the last byte sent by

> > slave, the Tx Done with NAK irq will raise.

> > But it is also observed that sometimes master issues next transaction

> > too quick while the slave irq handler is not yet invoked and Tx Done

> > with NAK irq of last byte of previous READ PROCESSED was not ack'ed.

> > This Tx Done with NAK irq is raised together with the Slave Match and

> > Rx Done irq of the next coming transaction from master.

> > Unfortunately, the current slave irq handler handles the Slave Match

> > and Rx Done only in higher priority and ignore the Tx Done with NAK,

> > causing the complain as below:

> > "aspeed-i2c-bus 1e78a040.i2c-bus: irq handled != irq. expected

> > 0x00000086, but was 0x00000084"

> >

> > This commit handles this case by emitting a Slave Stop event for the

> > Tx Done with NAK before processing Slave Match and Rx Done for the

> > coming transaction from master.

> 

> It sounds like this patch is independent of the rest of the series, and can go in

> on it's own. Please send it separately to the i2c maintainers and add a suitable

> Fixes line, such as:

> 

>   Fixes: f9eb91350bb2 ("i2c: aspeed: added slave support for Aspeed I2C

> driver")

> 

> >

> > Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>

> > ---

> > v3:

> >   + First introduce in v3 [Quan]

> >

> >  drivers/i2c/busses/i2c-aspeed.c | 5 +++++

> >  1 file changed, 5 insertions(+)

> >

> > diff --git a/drivers/i2c/busses/i2c-aspeed.c

> > b/drivers/i2c/busses/i2c-aspeed.c index 724bf30600d6..3fb37c3f23d4

> > 100644

> > --- a/drivers/i2c/busses/i2c-aspeed.c

> > +++ b/drivers/i2c/busses/i2c-aspeed.c

> > @@ -254,6 +254,11 @@ static u32 aspeed_i2c_slave_irq(struct

> > aspeed_i2c_bus *bus, u32 irq_status)

> >

> >         /* Slave was requested, restart state machine. */

> >         if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {

> 

> Can you explain why you need to do this handing inside the SLAVE_MATCH

> case?

> 

> Could you instead move the TX_NAK handling to be above the SLAVE_MATCH

> case?

> 

> > +               if (irq_status & ASPEED_I2CD_INTR_TX_NAK &&

> > +                   bus->slave_state ==

> > + ASPEED_I2C_SLAVE_READ_PROCESSED) {

> 

> Either way, this needs a comment to explain what we're working around.

> 

> > +                       irq_handled |= ASPEED_I2CD_INTR_TX_NAK;

> > +                       i2c_slave_event(slave, I2C_SLAVE_STOP,

> &value);


According the patch assume slave receive TX_NAK will be go to SLAVE_STOP state?

> > +               }

> >                 irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;

> >                 bus->slave_state = ASPEED_I2C_SLAVE_START;

> >         }

> > --

> > 2.28.0

> >
Quan Nguyen May 20, 2021, 1:48 p.m. UTC | #5
On 20/05/2021 06:28, Joel Stanley wrote:
> Ryan, can you please review this change?
> 
> On Wed, 19 May 2021 at 07:50, Quan Nguyen <quan@os.amperecomputing.com> wrote:
>>
>> It is observed that in normal condition, when the last byte sent by
>> slave, the Tx Done with NAK irq will raise.
>> But it is also observed that sometimes master issues next transaction
>> too quick while the slave irq handler is not yet invoked and Tx Done
>> with NAK irq of last byte of previous READ PROCESSED was not ack'ed.
>> This Tx Done with NAK irq is raised together with the Slave Match and
>> Rx Done irq of the next coming transaction from master.
>> Unfortunately, the current slave irq handler handles the Slave Match and
>> Rx Done only in higher priority and ignore the Tx Done with NAK, causing
>> the complain as below:
>> "aspeed-i2c-bus 1e78a040.i2c-bus: irq handled != irq. expected
>> 0x00000086, but was 0x00000084"
>>
>> This commit handles this case by emitting a Slave Stop event for the
>> Tx Done with NAK before processing Slave Match and Rx Done for the
>> coming transaction from master.
> 
> It sounds like this patch is independent of the rest of the series,
> and can go in on it's own. Please send it separately to the i2c
> maintainers and add a suitable Fixes line, such as:
> 
>    Fixes: f9eb91350bb2 ("i2c: aspeed: added slave support for Aspeed I2C driver")
> 
Will separate this patch into independent series in next version.

>>
>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>> ---
>> v3:
>>    + First introduce in v3 [Quan]
>>
>>   drivers/i2c/busses/i2c-aspeed.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>> index 724bf30600d6..3fb37c3f23d4 100644
>> --- a/drivers/i2c/busses/i2c-aspeed.c
>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>> @@ -254,6 +254,11 @@ static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>>
>>          /* Slave was requested, restart state machine. */
>>          if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
> 
> Can you explain why you need to do this handing inside the SLAVE_MATCH case?

> Could you instead move the TX_NAK handling to be above the SLAVE_MATCH case?
>
>> +               if (irq_status & ASPEED_I2CD_INTR_TX_NAK &&
>> +                   bus->slave_state == ASPEED_I2C_SLAVE_READ_PROCESSED) {
> 
> Either way, this needs a comment to explain what we're working around.
>
Let me explain with the two examples below in normal case and the case 
where this patch is for:

In normal case:
The first transaction is Slave send (Master read):
    20(addr) 03(singlepart read) 03 1c 2e d5

Then the second Master write follow as below:
    20(addr) 02(singlepart write) 02 18 08 59

The irq will raise in sequence below:

  irq      data  from-state      to-state
00000084  20    INACTIVE        WRITE_RECEIVED
00000004  03    WRITE_RECEIVED  WRITE_RECEIVED <= RX_DONE
00000084  03    WRITE_RECEIVED  READ_PROCESSED
00000001  1c    READ_PROCESSED  READ_PROCESSED <= TX_ACK
00000001  2e    READ_PROCESSED  READ_PROCESSED
00000001  d5    READ_PROCESSED  READ_PROCESSED
00000002  xx    READ_PROCESSED  INACTIVE       <= TX_NAK

00000084  20    INACTIVE        WRITE_RECEIVED <= SLAVE_MATCH & RX_DONE
00000004  02    WRITE_RECEIVED  WRITE_RECEIVED
00000084  02    WRITE_RECEIVED  WRITE_RECEIVED
00000004  18    WRITE_RECEIVED  WRITE_RECEIVED
00000004  08    WRITE_RECEIVED  WRITE_RECEIVED
00000004  59    WRITE_RECEIVED  WRITE_RECEIVED
00000010  xx    WRITE_RECEIVED  INACTIVE

But sometimes:
The first transaction is Slave send (Master read):
    20(addr) 03(singlepart read) 03 1c 42 cc a5

Then the second Master write follow as below:
    20(addr) 02(singlepart write) 03 18 42 0c 63

The irq will raise in sequence below:

  irq      data  from-state      to-state
00000084  20    INACTIVE        WRITE_RECEIVED
00000004  03    WRITE_RECEIVED  WRITE_RECEIVED
00000084  03    WRITE_RECEIVED  READ_PROCESSED
00000001  1c    READ_PROCESSED  READ_PROCESSED
00000001  42    READ_PROCESSED  READ_PROCESSED
00000001  0c    READ_PROCESSED  READ_PROCESSED
00000001  63    READ_PROCESSED  READ_PROCESSED

00000086  20    READ_PROCESSED  WRITE_RECEIVED <= both 3 irqs raised
00000004  02    WRITE_RECEIVED  WRITE_RECEIVED
00000084  03    WRITE_RECEIVED  WRITE_RECEIVED
00000004  18    WRITE_RECEIVED  WRITE_RECEIVED
00000004  42    WRITE_RECEIVED  WRITE_RECEIVED
00000004  0c    WRITE_RECEIVED  WRITE_RECEIVED
00000004  63    WRITE_RECEIVED  WRITE_RECEIVED
00000010  xx    WRITE_RECEIVED  INACTIVE

This patch is to address this case where TX_NAK, SLAVE_MATCH and RX_DONE 
are raised together.

>> +                       irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
>> +                       i2c_slave_event(slave, I2C_SLAVE_STOP, &value);
>> +               }
>>                  irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;
>>                  bus->slave_state = ASPEED_I2C_SLAVE_START;
>>          }
>> --
>> 2.28.0
>>
Quan Nguyen May 20, 2021, 2:10 p.m. UTC | #6
On 20/05/2021 18:06, Ryan Chen wrote:
>> -----Original Message-----

>> From: openbmc

>> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On Behalf

>> Of Quan Nguyen

>> Sent: Wednesday, May 19, 2021 3:50 PM

>> To: Corey Minyard <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>;

>> Joel Stanley <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan

>> Higgins <brendanhiggins@google.com>; Benjamin Herrenschmidt

>> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel

>> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net;

>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;

>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;

>> linux-i2c@vger.kernel.org

>> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .

>> Nguyen <thang@os.amperecomputing.com>; Phong Vo

>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org

>> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

>>

>> Slave i2c device on AST2500 received a lot of slave irq while it is busy

>> processing the response. To handle this case, adds and exports

>> aspeed_set_slave_busy() for controller to temporary stop slave irq while slave

>> is handling the response, and re-enable them again when the response is ready.

>>

>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>

>> ---

>> v3:

>>    + First introduce in v3 [Quan]

>>

>>   drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++

>>   1 file changed, 20 insertions(+)

>>

>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c

>> index b2e9c8f0ddf7..9926d04831a2 100644

>> --- a/drivers/i2c/busses/i2c-aspeed.c

>> +++ b/drivers/i2c/busses/i2c-aspeed.c

>> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus

>> *bus,

>>   	return 0;

>>   }

>>

>> +#if IS_ENABLED(CONFIG_I2C_SLAVE)

>> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) {

>> +	struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);

>> +	unsigned long current_mask, flags;

>> +

>> +	spin_lock_irqsave(&bus->lock, flags);

>> +

>> +	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);

>> +	if (busy)

>> +		current_mask &= ~(ASPEED_I2CD_INTR_RX_DONE |

>> ASPEED_I2CD_INTR_SLAVE_MATCH);

>> +	else

>> +		current_mask |= ASPEED_I2CD_INTR_RX_DONE |

>> ASPEED_I2CD_INTR_SLAVE_MATCH;

>> +	writel(current_mask, bus->base + ASPEED_I2C_INTR_CTRL_REG);

>> +

>> +	spin_unlock_irqrestore(&bus->lock, flags); }

>> +EXPORT_SYMBOL_GPL(aspeed_set_slave_busy);

>> +#endif

>> +

>>   static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus)  {

>>   	struct platform_device *pdev = to_platform_device(bus->dev);

>> --

>> 2.28.0

> 

> Hello,

> 	The better idea is use disable i2c slave mode.

> 	Due to if i2c controller running in slave will get slave match, and latch the SCL.

> 	Until cpu clear interrupt status.

> Ryan

> 

Thanks Ryan,

Do you mean to enable/disable slave function as per example code below ?

         /* Turn on slave mode. */
         func_ctrl_reg_val = readl(bus->base + ASPEED_I2C_FUN_CTRL_REG);
         func_ctrl_reg_val |= ASPEED_I2CD_SLAVE_EN;
         writel(func_ctrl_reg_val, bus->base + ASPEED_I2C_FUN_CTRL_REG);

Will try this idea.
- Quan
Quan Nguyen May 20, 2021, 2:15 p.m. UTC | #7
On 20/05/2021 18:28, Ryan Chen wrote:
>> -----Original Message-----
>> From: Joel Stanley <joel@jms.id.au>
>> Sent: Thursday, May 20, 2021 7:29 AM
>> To: Quan Nguyen <quan@os.amperecomputing.com>; Ryan Chen
>> <ryan_chen@aspeedtech.com>
>> Cc: Corey Minyard <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>;
>> Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins
>> <brendanhiggins@google.com>; Benjamin Herrenschmidt
>> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel
>> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net;
>> devicetree <devicetree@vger.kernel.org>; Linux ARM
>> <linux-arm-kernel@lists.infradead.org>; linux-aspeed
>> <linux-aspeed@lists.ozlabs.org>; Linux Kernel Mailing List
>> <linux-kernel@vger.kernel.org>; linux-i2c@vger.kernel.org; Open Source
>> Submission <patches@amperecomputing.com>; Phong Vo
>> <phong@os.amperecomputing.com>; Thang Q . Nguyen
>> <thang@os.amperecomputing.com>; OpenBMC Maillist
>> <openbmc@lists.ozlabs.org>
>> Subject: Re: [PATCH v3 3/7] i2c: aspeed: Fix unhandled Tx done with NAK
>>
>> Ryan, can you please review this change?
>>
>> On Wed, 19 May 2021 at 07:50, Quan Nguyen
>> <quan@os.amperecomputing.com> wrote:
>>>
>>> It is observed that in normal condition, when the last byte sent by
>>> slave, the Tx Done with NAK irq will raise.
>>> But it is also observed that sometimes master issues next transaction
>>> too quick while the slave irq handler is not yet invoked and Tx Done
>>> with NAK irq of last byte of previous READ PROCESSED was not ack'ed.
>>> This Tx Done with NAK irq is raised together with the Slave Match and
>>> Rx Done irq of the next coming transaction from master.
>>> Unfortunately, the current slave irq handler handles the Slave Match
>>> and Rx Done only in higher priority and ignore the Tx Done with NAK,
>>> causing the complain as below:
>>> "aspeed-i2c-bus 1e78a040.i2c-bus: irq handled != irq. expected
>>> 0x00000086, but was 0x00000084"
>>>
>>> This commit handles this case by emitting a Slave Stop event for the
>>> Tx Done with NAK before processing Slave Match and Rx Done for the
>>> coming transaction from master.
>>
>> It sounds like this patch is independent of the rest of the series, and can go in
>> on it's own. Please send it separately to the i2c maintainers and add a suitable
>> Fixes line, such as:
>>
>>    Fixes: f9eb91350bb2 ("i2c: aspeed: added slave support for Aspeed I2C
>> driver")
>>
>>>
>>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>>> ---
>>> v3:
>>>    + First introduce in v3 [Quan]
>>>
>>>   drivers/i2c/busses/i2c-aspeed.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c
>>> b/drivers/i2c/busses/i2c-aspeed.c index 724bf30600d6..3fb37c3f23d4
>>> 100644
>>> --- a/drivers/i2c/busses/i2c-aspeed.c
>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>>> @@ -254,6 +254,11 @@ static u32 aspeed_i2c_slave_irq(struct
>>> aspeed_i2c_bus *bus, u32 irq_status)
>>>
>>>          /* Slave was requested, restart state machine. */
>>>          if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
>>
>> Can you explain why you need to do this handing inside the SLAVE_MATCH
>> case?
>>
>> Could you instead move the TX_NAK handling to be above the SLAVE_MATCH
>> case?
>>
>>> +               if (irq_status & ASPEED_I2CD_INTR_TX_NAK &&
>>> +                   bus->slave_state ==
>>> + ASPEED_I2C_SLAVE_READ_PROCESSED) {
>>
>> Either way, this needs a comment to explain what we're working around.
>>
>>> +                       irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
>>> +                       i2c_slave_event(slave, I2C_SLAVE_STOP,
>> &value);
> 
> According the patch assume slave receive TX_NAK will be go to SLAVE_STOP state?
> 
Hi Ryan,

As per my explain in other email, we need to emit one SLAVE_STOP event 
to complete the previous transaction before start processing for the 
next transaction.

- Quan

>>> +               }
>>>                  irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;
>>>                  bus->slave_state = ASPEED_I2C_SLAVE_START;
>>>          }
>>> --
>>> 2.28.0
>>>
Quan Nguyen May 20, 2021, 2:23 p.m. UTC | #8
On 19/05/2021 19:34, Corey Minyard wrote:
> On Wed, May 19, 2021 at 02:49:27PM +0700, Quan Nguyen wrote:
>> This series add support for the Aspeed specific SSIF BMC driver which
>> is to perform in-band IPMI communication with the host in management
>> (BMC) side.
>>
>> v3:
>>    + Switched binding doc to use DT schema format [Rob]
>>    + Splited into generic ssif_bmc and aspeed-specific [Corey, Joel]
>>    + Removed redundant license info [Joel]
>>    + Switched to use traditional if-else [Joel]
>>    + Removed unused ssif_bmc_ioctl() [Joel]
>>    + Made handle_request()/complete_response() to return void [Joel]
>>    + Refactored send_ssif_bmc_response()/receive_ssif_bmc_request() [Corey]
>>    + Remove mutex [Corey]
>>    + Use spin_lock/unlock_irqsave/restore in callback [Corey]
>>    + Removed the unnecessary memset [Corey]
>>    + Switch to use dev_err() [Corey]
>>    + Combine mask/unmask two interrupts together [Corey]
>>    + Fixed unhandled Tx done with NAK [Quan]
>>    + Late ack'ed Tx done w/wo Ack irq [Quan]
>>    + Use aspeed-specific exported aspeed_set_slave_busy() when slave busy
>> to fix the deadlock [Graeme, Philipp, Quan]
>>    + Clean buffer for last multipart read [Quan]
>>    + Handle unknown incoming command [Quan]
>>
>> v2:
>>    + Fixed compiling error with COMPILE_TEST for arc
>>
>> Quan Nguyen (7):
>>    i2c: i2c-core-smbus: Expose PEC calculate function for generic use
> 
> Note that for this I2C-specific change, I will need acks from the I2C
> maintainers to be able to include this in my tree.
>  >>    ipmi: ssif_bmc: Add SSIF BMC driver
>>    i2c: aspeed: Fix unhandled Tx done with NAK
> 
> For the aspeed changes, they don't really belong here, they belong in
> the aspeed tree.  I see that you need them for the device to work
> correctly, though.  I'll need acks from maintainers to include them.
> 
Thanks for you information.
Will separate these i2c-aspeed's patches into other independent series 
in next version.

>>    i2c: aspeed: Acknowledge Tx done w/wo ACK irq late
>>    i2c: aspeed: Add aspeed_set_slave_busy()
>>    ipmi: ssif_bmc: Add Aspeed SSIF BMC driver
>>    bindings: ipmi: Add binding for Aspeed SSIF BMC driver
>>
>>   .../bindings/ipmi/aspeed-ssif-bmc.yaml        |  33 +
>>   drivers/char/ipmi/Kconfig                     |  22 +
>>   drivers/char/ipmi/Makefile                    |   2 +
>>   drivers/char/ipmi/ssif_bmc.c                  | 605 ++++++++++++++++++
>>   drivers/char/ipmi/ssif_bmc.h                  |  93 +++
>>   drivers/char/ipmi/ssif_bmc_aspeed.c           |  75 +++
>>   drivers/i2c/busses/i2c-aspeed.c               |  51 +-
>>   drivers/i2c/i2c-core-smbus.c                  |  12 +-
>>   include/linux/i2c.h                           |   1 +
>>   9 files changed, 884 insertions(+), 10 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.yaml
>>   create mode 100644 drivers/char/ipmi/ssif_bmc.c
>>   create mode 100644 drivers/char/ipmi/ssif_bmc.h
>>   create mode 100644 drivers/char/ipmi/ssif_bmc_aspeed.c
>>
>> -- 
>> 2.28.0
>>
Quan Nguyen May 20, 2021, 2:24 p.m. UTC | #9
On 19/05/2021 22:29, Rob Herring wrote:
> On Wed, 19 May 2021 14:49:34 +0700, Quan Nguyen wrote:
>> Add device tree binding document for the Aspeed SSIF BMC driver.
>>
>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>> ---
>> v3:
>>    + Switched to use DT schema format [Rob]
>>
>>   .../bindings/ipmi/aspeed-ssif-bmc.yaml        | 33 +++++++++++++++++++
>>   1 file changed, 33 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.yaml
>>
> 
> 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/ipmi/aspeed-ssif-bmc.example.dts:21.13-26: Warning (reg_format): /example-0/ssif-bmc@10:reg: property has invalid length (4 bytes) (#address-cells == 1, #size-cells == 1)
> Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.example.dt.yaml: Warning (pci_device_reg): Failed prerequisite 'reg_format'
> Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.example.dt.yaml: Warning (pci_device_bus_num): Failed prerequisite 'reg_format'
> Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.example.dt.yaml: Warning (simple_bus_reg): Failed prerequisite 'reg_format'
> Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.example.dt.yaml: Warning (i2c_bus_reg): Failed prerequisite 'reg_format'
> Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.example.dt.yaml: Warning (spi_bus_reg): Failed prerequisite 'reg_format'
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/ipmi/aspeed-ssif-bmc.example.dt.yaml: example-0: ssif-bmc@10:reg:0: [16] is too short
> 	From schema: /usr/local/lib/python3.8/dist-packages/dtschema/schemas/reg.yaml
> 
> See https://patchwork.ozlabs.org/patch/1480727
> 
> 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.
> 
Thanks,
Will fix in next version.

- Quan
Ryan Chen May 21, 2021, 6:09 a.m. UTC | #10
> -----Original Message-----

> From: Quan Nguyen <quan@os.amperecomputing.com>

> Sent: Thursday, May 20, 2021 10:10 PM

> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard

> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley

> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins

> <brendanhiggins@google.com>; Benjamin Herrenschmidt

> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel

> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net;

> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;

> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;

> linux-i2c@vger.kernel.org

> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .

> Nguyen <thang@os.amperecomputing.com>; Phong Vo

> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org

> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

> 

> On 20/05/2021 18:06, Ryan Chen wrote:

> >> -----Original Message-----

> >> From: openbmc

> >> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On

> Behalf

> >> Of Quan Nguyen

> >> Sent: Wednesday, May 19, 2021 3:50 PM

> >> To: Corey Minyard <minyard@acm.org>; Rob Herring

> >> <robh+dt@kernel.org>; Joel Stanley <joel@jms.id.au>; Andrew Jeffery

> >> <andrew@aj.id.au>; Brendan Higgins <brendanhiggins@google.com>;

> >> Benjamin Herrenschmidt <benh@kernel.crashing.org>; Wolfram Sang

> >> <wsa@kernel.org>; Philipp Zabel <p.zabel@pengutronix.de>;

> >> openipmi-developer@lists.sourceforge.net;

> >> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;

> >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;

> >> linux-i2c@vger.kernel.org

> >> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .

> >> Nguyen <thang@os.amperecomputing.com>; Phong Vo

> >> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org

> >> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

> >>

> >> Slave i2c device on AST2500 received a lot of slave irq while it is

> >> busy processing the response. To handle this case, adds and exports

> >> aspeed_set_slave_busy() for controller to temporary stop slave irq

> >> while slave is handling the response, and re-enable them again when the

> response is ready.

> >>

> >> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>

> >> ---

> >> v3:

> >>    + First introduce in v3 [Quan]

> >>

> >>   drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++

> >>   1 file changed, 20 insertions(+)

> >>

> >> diff --git a/drivers/i2c/busses/i2c-aspeed.c

> >> b/drivers/i2c/busses/i2c-aspeed.c index b2e9c8f0ddf7..9926d04831a2

> >> 100644

> >> --- a/drivers/i2c/busses/i2c-aspeed.c

> >> +++ b/drivers/i2c/busses/i2c-aspeed.c

> >> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus

> >> *bus,

> >>   	return 0;

> >>   }

> >>

> >> +#if IS_ENABLED(CONFIG_I2C_SLAVE)

> >> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) {

> >> +	struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);

> >> +	unsigned long current_mask, flags;

> >> +

> >> +	spin_lock_irqsave(&bus->lock, flags);

> >> +

> >> +	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);

> >> +	if (busy)

> >> +		current_mask &= ~(ASPEED_I2CD_INTR_RX_DONE |

> >> ASPEED_I2CD_INTR_SLAVE_MATCH);

> >> +	else

> >> +		current_mask |= ASPEED_I2CD_INTR_RX_DONE |

> >> ASPEED_I2CD_INTR_SLAVE_MATCH;

> >> +	writel(current_mask, bus->base + ASPEED_I2C_INTR_CTRL_REG);

> >> +

> >> +	spin_unlock_irqrestore(&bus->lock, flags); }

> >> +EXPORT_SYMBOL_GPL(aspeed_set_slave_busy);

> >> +#endif

> >> +

> >>   static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus)  {

> >>   	struct platform_device *pdev = to_platform_device(bus->dev);

> >> --

> >> 2.28.0

> >

> > Hello,

> > 	The better idea is use disable i2c slave mode.

> > 	Due to if i2c controller running in slave will get slave match, and latch the

> SCL.

> > 	Until cpu clear interrupt status.

> > Ryan

> >

> Thanks Ryan,

> 

> Do you mean to enable/disable slave function as per example code below ?

Yes. it is.
> 

>          /* Turn on slave mode. */

>          func_ctrl_reg_val = readl(bus->base +

> ASPEED_I2C_FUN_CTRL_REG);

>          func_ctrl_reg_val |= ASPEED_I2CD_SLAVE_EN;

>          writel(func_ctrl_reg_val, bus->base +

> ASPEED_I2C_FUN_CTRL_REG);

> 

> Will try this idea.

> - Quan
Ryan Chen May 24, 2021, 10:06 a.m. UTC | #11
> -----Original Message-----

> From: openbmc

> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On Behalf

> Of Quan Nguyen

> Sent: Wednesday, May 19, 2021 3:50 PM

> To: Corey Minyard <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>;

> Joel Stanley <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan

> Higgins <brendanhiggins@google.com>; Benjamin Herrenschmidt

> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel

> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net;

> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;

> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;

> linux-i2c@vger.kernel.org

> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .

> Nguyen <thang@os.amperecomputing.com>; Phong Vo

> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org

> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

> 

> Slave i2c device on AST2500 received a lot of slave irq while it is busy

> processing the response. To handle this case, adds and exports

> aspeed_set_slave_busy() for controller to temporary stop slave irq while slave

> is handling the response, and re-enable them again when the response is ready.

> 

> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>

> ---

> v3:

>   + First introduce in v3 [Quan]

> 

>  drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++

>  1 file changed, 20 insertions(+)

> 

> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c

> index b2e9c8f0ddf7..9926d04831a2 100644

> --- a/drivers/i2c/busses/i2c-aspeed.c

> +++ b/drivers/i2c/busses/i2c-aspeed.c

> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus

> *bus,

>  	return 0;

>  }

> 

> +#if IS_ENABLED(CONFIG_I2C_SLAVE)

> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) {

> +	struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);

> +	unsigned long current_mask, flags;

> +

> +	spin_lock_irqsave(&bus->lock, flags);

> +

> +	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);

Hello 
	Where the bus->base to be remap?

> +	if (busy)

> +		current_mask &= ~(ASPEED_I2CD_INTR_RX_DONE |

> ASPEED_I2CD_INTR_SLAVE_MATCH);

> +	else

> +		current_mask |= ASPEED_I2CD_INTR_RX_DONE |

> ASPEED_I2CD_INTR_SLAVE_MATCH;

> +	writel(current_mask, bus->base + ASPEED_I2C_INTR_CTRL_REG);

> +

> +	spin_unlock_irqrestore(&bus->lock, flags); }

> +EXPORT_SYMBOL_GPL(aspeed_set_slave_busy);

> +#endif

> +

>  static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus)  {

>  	struct platform_device *pdev = to_platform_device(bus->dev);

> --

> 2.28.0
Quan Nguyen May 24, 2021, 10:20 a.m. UTC | #12
On 24/05/2021 17:06, Ryan Chen wrote:
>> -----Original Message-----

>> From: openbmc

>> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On Behalf

>> Of Quan Nguyen

>> Sent: Wednesday, May 19, 2021 3:50 PM

>> To: Corey Minyard <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>;

>> Joel Stanley <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan

>> Higgins <brendanhiggins@google.com>; Benjamin Herrenschmidt

>> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel

>> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net;

>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;

>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;

>> linux-i2c@vger.kernel.org

>> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .

>> Nguyen <thang@os.amperecomputing.com>; Phong Vo

>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org

>> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

>>

>> Slave i2c device on AST2500 received a lot of slave irq while it is busy

>> processing the response. To handle this case, adds and exports

>> aspeed_set_slave_busy() for controller to temporary stop slave irq while slave

>> is handling the response, and re-enable them again when the response is ready.

>>

>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>

>> ---

>> v3:

>>    + First introduce in v3 [Quan]

>>

>>   drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++

>>   1 file changed, 20 insertions(+)

>>

>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c

>> index b2e9c8f0ddf7..9926d04831a2 100644

>> --- a/drivers/i2c/busses/i2c-aspeed.c

>> +++ b/drivers/i2c/busses/i2c-aspeed.c

>> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus

>> *bus,

>>   	return 0;

>>   }

>>

>> +#if IS_ENABLED(CONFIG_I2C_SLAVE)

>> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) {

>> +	struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);

>> +	unsigned long current_mask, flags;

>> +

>> +	spin_lock_irqsave(&bus->lock, flags);

>> +

>> +	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);

> Hello

> 	Where the bus->base to be remap?

> 


Hi Ryan,

In "[PATCH v3 6/7] ipmi: ssif_bmc: Add Aspeed SSIF BMC driver", the 
->priv is retrieved by calling i2c_get_adapdata(client->adapter). And in 
aspeed_set_ssif_bmc_status(), call the exported aspeed_set_slave_busy() 
using ->priv pointer as code below.

+extern void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy);
+static void aspeed_set_ssif_bmc_status(struct ssif_bmc_ctx *ssif_bmc, 
unsigned int status)
+{
+	if (status & SSIF_BMC_BUSY)
+		aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, true);
+	else if (status & SSIF_BMC_READY)
+		aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, false);
+}
+
+static int ssif_bmc_probe(struct i2c_client *client, const struct 
i2c_device_id *id)
+{
+	struct ssif_bmc_ctx *ssif_bmc;
+
+	ssif_bmc = ssif_bmc_alloc(client, 0);
+	if (IS_ERR(ssif_bmc))
+		return PTR_ERR(ssif_bmc);
+
+	ssif_bmc->priv = i2c_get_adapdata(client->adapter);
+	ssif_bmc->set_ssif_bmc_status = aspeed_set_ssif_bmc_status;
+
+	return 0;
+}

- Quan
Ryan Chen May 24, 2021, 10:36 a.m. UTC | #13
> -----Original Message-----

> From: Quan Nguyen <quan@os.amperecomputing.com>

> Sent: Monday, May 24, 2021 6:20 PM

> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard

> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley

> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins

> <brendanhiggins@google.com>; Benjamin Herrenschmidt

> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel

> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net;

> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;

> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;

> linux-i2c@vger.kernel.org

> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .

> Nguyen <thang@os.amperecomputing.com>; Phong Vo

> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org

> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

> 

> On 24/05/2021 17:06, Ryan Chen wrote:

> >> -----Original Message-----

> >> From: openbmc

> >> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On

> Behalf

> >> Of Quan Nguyen

> >> Sent: Wednesday, May 19, 2021 3:50 PM

> >> To: Corey Minyard <minyard@acm.org>; Rob Herring

> >> <robh+dt@kernel.org>; Joel Stanley <joel@jms.id.au>; Andrew Jeffery

> >> <andrew@aj.id.au>; Brendan Higgins <brendanhiggins@google.com>;

> >> Benjamin Herrenschmidt <benh@kernel.crashing.org>; Wolfram Sang

> >> <wsa@kernel.org>; Philipp Zabel <p.zabel@pengutronix.de>;

> >> openipmi-developer@lists.sourceforge.net;

> >> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;

> >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;

> >> linux-i2c@vger.kernel.org

> >> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .

> >> Nguyen <thang@os.amperecomputing.com>; Phong Vo

> >> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org

> >> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

> >>

> >> Slave i2c device on AST2500 received a lot of slave irq while it is

> >> busy processing the response. To handle this case, adds and exports

> >> aspeed_set_slave_busy() for controller to temporary stop slave irq

> >> while slave is handling the response, and re-enable them again when the

> response is ready.

> >>

> >> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>

> >> ---

> >> v3:

> >>    + First introduce in v3 [Quan]

> >>

> >>   drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++

> >>   1 file changed, 20 insertions(+)

> >>

> >> diff --git a/drivers/i2c/busses/i2c-aspeed.c

> >> b/drivers/i2c/busses/i2c-aspeed.c index b2e9c8f0ddf7..9926d04831a2

> >> 100644

> >> --- a/drivers/i2c/busses/i2c-aspeed.c

> >> +++ b/drivers/i2c/busses/i2c-aspeed.c

> >> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus

> >> *bus,

> >>   	return 0;

> >>   }

> >>

> >> +#if IS_ENABLED(CONFIG_I2C_SLAVE)

> >> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) {

> >> +	struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);

> >> +	unsigned long current_mask, flags;

> >> +

> >> +	spin_lock_irqsave(&bus->lock, flags);

> >> +

> >> +	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);

> > Hello

> > 	Where the bus->base to be remap?

> >

> 

> Hi Ryan,

> 

> In "[PATCH v3 6/7] ipmi: ssif_bmc: Add Aspeed SSIF BMC driver", the

> ->priv is retrieved by calling i2c_get_adapdata(client->adapter). And in

> aspeed_set_ssif_bmc_status(), call the exported aspeed_set_slave_busy()

> using ->priv pointer as code below.

> 

Yes, I see the probe function " ssif_bmc->priv = i2c_get_adapdata(client->adapter);" to get priv.
But my question I don’t see the bus->base address be assigned. 

> +extern void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy);

> +static void aspeed_set_ssif_bmc_status(struct ssif_bmc_ctx *ssif_bmc,

> unsigned int status)

> +{

> +	if (status & SSIF_BMC_BUSY)

> +		aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, true);

> +	else if (status & SSIF_BMC_READY)

> +		aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, false); }

> +

> +static int ssif_bmc_probe(struct i2c_client *client, const struct

> i2c_device_id *id)

> +{

> +	struct ssif_bmc_ctx *ssif_bmc;

> +

> +	ssif_bmc = ssif_bmc_alloc(client, 0);

> +	if (IS_ERR(ssif_bmc))

> +		return PTR_ERR(ssif_bmc);

> +

> +	ssif_bmc->priv = i2c_get_adapdata(client->adapter);

> +	ssif_bmc->set_ssif_bmc_status = aspeed_set_ssif_bmc_status;

> +

> +	return 0;

> +}

> 

> - Quan

> 

>
Quan Nguyen May 24, 2021, 10:48 a.m. UTC | #14
On 24/05/2021 17:36, Ryan Chen wrote:
>> -----Original Message-----

>> From: Quan Nguyen <quan@os.amperecomputing.com>

>> Sent: Monday, May 24, 2021 6:20 PM

>> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard

>> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley

>> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins

>> <brendanhiggins@google.com>; Benjamin Herrenschmidt

>> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel

>> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net;

>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;

>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;

>> linux-i2c@vger.kernel.org

>> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .

>> Nguyen <thang@os.amperecomputing.com>; Phong Vo

>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org

>> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

>>

>> On 24/05/2021 17:06, Ryan Chen wrote:

>>>> -----Original Message-----

>>>> From: openbmc

>>>> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On

>> Behalf

>>>> Of Quan Nguyen

>>>> Sent: Wednesday, May 19, 2021 3:50 PM

>>>> To: Corey Minyard <minyard@acm.org>; Rob Herring

>>>> <robh+dt@kernel.org>; Joel Stanley <joel@jms.id.au>; Andrew Jeffery

>>>> <andrew@aj.id.au>; Brendan Higgins <brendanhiggins@google.com>;

>>>> Benjamin Herrenschmidt <benh@kernel.crashing.org>; Wolfram Sang

>>>> <wsa@kernel.org>; Philipp Zabel <p.zabel@pengutronix.de>;

>>>> openipmi-developer@lists.sourceforge.net;

>>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;

>>>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;

>>>> linux-i2c@vger.kernel.org

>>>> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .

>>>> Nguyen <thang@os.amperecomputing.com>; Phong Vo

>>>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org

>>>> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

>>>>

>>>> Slave i2c device on AST2500 received a lot of slave irq while it is

>>>> busy processing the response. To handle this case, adds and exports

>>>> aspeed_set_slave_busy() for controller to temporary stop slave irq

>>>> while slave is handling the response, and re-enable them again when the

>> response is ready.

>>>>

>>>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>

>>>> ---

>>>> v3:

>>>>     + First introduce in v3 [Quan]

>>>>

>>>>    drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++

>>>>    1 file changed, 20 insertions(+)

>>>>

>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c

>>>> b/drivers/i2c/busses/i2c-aspeed.c index b2e9c8f0ddf7..9926d04831a2

>>>> 100644

>>>> --- a/drivers/i2c/busses/i2c-aspeed.c

>>>> +++ b/drivers/i2c/busses/i2c-aspeed.c

>>>> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus

>>>> *bus,

>>>>    	return 0;

>>>>    }

>>>>

>>>> +#if IS_ENABLED(CONFIG_I2C_SLAVE)

>>>> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) {

>>>> +	struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);

>>>> +	unsigned long current_mask, flags;

>>>> +

>>>> +	spin_lock_irqsave(&bus->lock, flags);

>>>> +

>>>> +	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);

>>> Hello

>>> 	Where the bus->base to be remap?

>>>

>>

>> Hi Ryan,

>>

>> In "[PATCH v3 6/7] ipmi: ssif_bmc: Add Aspeed SSIF BMC driver", the

>> ->priv is retrieved by calling i2c_get_adapdata(client->adapter). And in

>> aspeed_set_ssif_bmc_status(), call the exported aspeed_set_slave_busy()

>> using ->priv pointer as code below.

>>

> Yes, I see the probe function " ssif_bmc->priv = i2c_get_adapdata(client->adapter);" to get priv.

> But my question I don’t see the bus->base address be assigned.

> 

Hi Ryan,

In drivers/i2c/busses/i2c-aspeed.c:
struct aspeed_i2c_bus {
         struct i2c_adapter              adap;
         struct device                   *dev;
         void __iomem                    *base;
         struct reset_control            *rst;
         /* Synchronizes I/O mem access to base. */
         spinlock_t                      lock;

So when "struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);", the 
bus->base should point to the base of the aspeed_i2c_bus, which is 
already initialized by the aspeed i2c bus driver.

Do I miss something?

- Quan


>> +extern void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy);

>> +static void aspeed_set_ssif_bmc_status(struct ssif_bmc_ctx *ssif_bmc,

>> unsigned int status)

>> +{

>> +	if (status & SSIF_BMC_BUSY)

>> +		aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, true);

>> +	else if (status & SSIF_BMC_READY)

>> +		aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, false); }

>> +

>> +static int ssif_bmc_probe(struct i2c_client *client, const struct

>> i2c_device_id *id)

>> +{

>> +	struct ssif_bmc_ctx *ssif_bmc;

>> +

>> +	ssif_bmc = ssif_bmc_alloc(client, 0);

>> +	if (IS_ERR(ssif_bmc))

>> +		return PTR_ERR(ssif_bmc);

>> +

>> +	ssif_bmc->priv = i2c_get_adapdata(client->adapter);

>> +	ssif_bmc->set_ssif_bmc_status = aspeed_set_ssif_bmc_status;

>> +

>> +	return 0;

>> +}

>>

>> - Quan

>>

>>

>
Ryan Chen May 25, 2021, 10:30 a.m. UTC | #15
> -----Original Message-----

> From: Quan Nguyen <quan@os.amperecomputing.com>

> Sent: Monday, May 24, 2021 6:49 PM

> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard

> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley

> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins

> <brendanhiggins@google.com>; Benjamin Herrenschmidt

> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel

> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net;

> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;

> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;

> linux-i2c@vger.kernel.org

> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .

> Nguyen <thang@os.amperecomputing.com>; Phong Vo

> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org

> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

> 

> On 24/05/2021 17:36, Ryan Chen wrote:

> >> -----Original Message-----

> >> From: Quan Nguyen <quan@os.amperecomputing.com>

> >> Sent: Monday, May 24, 2021 6:20 PM

> >> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard

> >> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley

> >> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins

> >> <brendanhiggins@google.com>; Benjamin Herrenschmidt

> >> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp

> >> Zabel <p.zabel@pengutronix.de>;

> >> openipmi-developer@lists.sourceforge.net;

> >> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;

> >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;

> >> linux-i2c@vger.kernel.org

> >> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .

> >> Nguyen <thang@os.amperecomputing.com>; Phong Vo

> >> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org

> >> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

> >>

> >> On 24/05/2021 17:06, Ryan Chen wrote:

> >>>> -----Original Message-----

> >>>> From: openbmc

> >>>> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On

> >> Behalf

> >>>> Of Quan Nguyen

> >>>> Sent: Wednesday, May 19, 2021 3:50 PM

> >>>> To: Corey Minyard <minyard@acm.org>; Rob Herring

> >>>> <robh+dt@kernel.org>; Joel Stanley <joel@jms.id.au>; Andrew Jeffery

> >>>> <andrew@aj.id.au>; Brendan Higgins <brendanhiggins@google.com>;

> >>>> Benjamin Herrenschmidt <benh@kernel.crashing.org>; Wolfram Sang

> >>>> <wsa@kernel.org>; Philipp Zabel <p.zabel@pengutronix.de>;

> >>>> openipmi-developer@lists.sourceforge.net;

> >>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;

> >>>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;

> >>>> linux-i2c@vger.kernel.org

> >>>> Cc: Open Source Submission <patches@amperecomputing.com>; Thang

> Q .

> >>>> Nguyen <thang@os.amperecomputing.com>; Phong Vo

> >>>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org

> >>>> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

> >>>>

> >>>> Slave i2c device on AST2500 received a lot of slave irq while it is

> >>>> busy processing the response. To handle this case, adds and exports

> >>>> aspeed_set_slave_busy() for controller to temporary stop slave irq

> >>>> while slave is handling the response, and re-enable them again when

> >>>> the

> >> response is ready.

> >>>>

> >>>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>

> >>>> ---

> >>>> v3:

> >>>>     + First introduce in v3 [Quan]

> >>>>

> >>>>    drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++

> >>>>    1 file changed, 20 insertions(+)

> >>>>

> >>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c

> >>>> b/drivers/i2c/busses/i2c-aspeed.c index b2e9c8f0ddf7..9926d04831a2

> >>>> 100644

> >>>> --- a/drivers/i2c/busses/i2c-aspeed.c

> >>>> +++ b/drivers/i2c/busses/i2c-aspeed.c

> >>>> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct

> >>>> aspeed_i2c_bus *bus,

> >>>>    	return 0;

> >>>>    }

> >>>>

> >>>> +#if IS_ENABLED(CONFIG_I2C_SLAVE)

> >>>> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) {

> >>>> +	struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);

> >>>> +	unsigned long current_mask, flags;

> >>>> +

> >>>> +	spin_lock_irqsave(&bus->lock, flags);

> >>>> +

> >>>> +	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);

> >>> Hello

> >>> 	Where the bus->base to be remap?

> >>>

> >>

> >> Hi Ryan,

> >>

> >> In "[PATCH v3 6/7] ipmi: ssif_bmc: Add Aspeed SSIF BMC driver", the

> >> ->priv is retrieved by calling i2c_get_adapdata(client->adapter). And

> >> ->in

> >> aspeed_set_ssif_bmc_status(), call the exported

> >> aspeed_set_slave_busy() using ->priv pointer as code below.

> >>

> > Yes, I see the probe function " ssif_bmc->priv =

> i2c_get_adapdata(client->adapter);" to get priv.

> > But my question I don’t see the bus->base address be assigned.

> >

> Hi Ryan,

> 

> In drivers/i2c/busses/i2c-aspeed.c:

> struct aspeed_i2c_bus {

>          struct i2c_adapter              adap;

>          struct device                   *dev;

>          void __iomem                    *base;

>          struct reset_control            *rst;

>          /* Synchronizes I/O mem access to base. */

>          spinlock_t                      lock;

> 

> So when "struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);", the

> bus->base should point to the base of the aspeed_i2c_bus, which is

> already initialized by the aspeed i2c bus driver.

> 

> Do I miss something?

Hello Quan,
	After study. I think the ssif_bmc_aspeed.c assume the priv data is the same struct.
	That is trick.
	Do we have a better for slave enable/disable call back to implement this?
	If add call back in struct i2c_algorithm; is it workable?
> 

> >> +extern void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy);

> >> +static void aspeed_set_ssif_bmc_status(struct ssif_bmc_ctx *ssif_bmc,

> >> unsigned int status)

> >> +{

> >> +	if (status & SSIF_BMC_BUSY)

> >> +		aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, true);

> >> +	else if (status & SSIF_BMC_READY)

> >> +		aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, false); }

> >> +

> >> +static int ssif_bmc_probe(struct i2c_client *client, const struct

> >> i2c_device_id *id)

> >> +{

> >> +	struct ssif_bmc_ctx *ssif_bmc;

> >> +

> >> +	ssif_bmc = ssif_bmc_alloc(client, 0);

> >> +	if (IS_ERR(ssif_bmc))

> >> +		return PTR_ERR(ssif_bmc);

> >> +

> >> +	ssif_bmc->priv = i2c_get_adapdata(client->adapter);

> >> +	ssif_bmc->set_ssif_bmc_status = aspeed_set_ssif_bmc_status;

> >> +

> >> +	return 0;

> >> +}

> >>

> >> - Quan

> >>

> >>

> >
Quan Nguyen May 28, 2021, 12:53 a.m. UTC | #16
On 25/05/2021 17:30, Ryan Chen wrote:
>> -----Original Message-----
>> From: Quan Nguyen <quan@os.amperecomputing.com>
>> Sent: Monday, May 24, 2021 6:49 PM
>> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard
>> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley
>> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins
>> <brendanhiggins@google.com>; Benjamin Herrenschmidt
>> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel
>> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net;
>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;
>> linux-i2c@vger.kernel.org
>> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .
>> Nguyen <thang@os.amperecomputing.com>; Phong Vo
>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org
>> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()
>>
>> On 24/05/2021 17:36, Ryan Chen wrote:
>>>> -----Original Message-----
>>>> From: Quan Nguyen <quan@os.amperecomputing.com>
>>>> Sent: Monday, May 24, 2021 6:20 PM
>>>> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard
>>>> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley
>>>> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins
>>>> <brendanhiggins@google.com>; Benjamin Herrenschmidt
>>>> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp
>>>> Zabel <p.zabel@pengutronix.de>;
>>>> openipmi-developer@lists.sourceforge.net;
>>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>>>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;
>>>> linux-i2c@vger.kernel.org
>>>> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .
>>>> Nguyen <thang@os.amperecomputing.com>; Phong Vo
>>>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org
>>>> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()
>>>>
>>>> On 24/05/2021 17:06, Ryan Chen wrote:
>>>>>> -----Original Message-----
>>>>>> From: openbmc
>>>>>> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On
>>>> Behalf
>>>>>> Of Quan Nguyen
>>>>>> Sent: Wednesday, May 19, 2021 3:50 PM
>>>>>> To: Corey Minyard <minyard@acm.org>; Rob Herring
>>>>>> <robh+dt@kernel.org>; Joel Stanley <joel@jms.id.au>; Andrew Jeffery
>>>>>> <andrew@aj.id.au>; Brendan Higgins <brendanhiggins@google.com>;
>>>>>> Benjamin Herrenschmidt <benh@kernel.crashing.org>; Wolfram Sang
>>>>>> <wsa@kernel.org>; Philipp Zabel <p.zabel@pengutronix.de>;
>>>>>> openipmi-developer@lists.sourceforge.net;
>>>>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>>>>>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;
>>>>>> linux-i2c@vger.kernel.org
>>>>>> Cc: Open Source Submission <patches@amperecomputing.com>; Thang
>> Q .
>>>>>> Nguyen <thang@os.amperecomputing.com>; Phong Vo
>>>>>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org
>>>>>> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()
>>>>>>
>>>>>> Slave i2c device on AST2500 received a lot of slave irq while it is
>>>>>> busy processing the response. To handle this case, adds and exports
>>>>>> aspeed_set_slave_busy() for controller to temporary stop slave irq
>>>>>> while slave is handling the response, and re-enable them again when
>>>>>> the
>>>> response is ready.
>>>>>>
>>>>>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
>>>>>> ---
>>>>>> v3:
>>>>>>      + First introduce in v3 [Quan]
>>>>>>
>>>>>>     drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++
>>>>>>     1 file changed, 20 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c
>>>>>> b/drivers/i2c/busses/i2c-aspeed.c index b2e9c8f0ddf7..9926d04831a2
>>>>>> 100644
>>>>>> --- a/drivers/i2c/busses/i2c-aspeed.c
>>>>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>>>>>> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct
>>>>>> aspeed_i2c_bus *bus,
>>>>>>     	return 0;
>>>>>>     }
>>>>>>
>>>>>> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
>>>>>> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) {
>>>>>> +	struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);
>>>>>> +	unsigned long current_mask, flags;
>>>>>> +
>>>>>> +	spin_lock_irqsave(&bus->lock, flags);
>>>>>> +
>>>>>> +	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);
>>>>> Hello
>>>>> 	Where the bus->base to be remap?
>>>>>
>>>>
>>>> Hi Ryan,
>>>>
>>>> In "[PATCH v3 6/7] ipmi: ssif_bmc: Add Aspeed SSIF BMC driver", the
>>>> ->priv is retrieved by calling i2c_get_adapdata(client->adapter). And
>>>> ->in
>>>> aspeed_set_ssif_bmc_status(), call the exported
>>>> aspeed_set_slave_busy() using ->priv pointer as code below.
>>>>
>>> Yes, I see the probe function " ssif_bmc->priv =
>> i2c_get_adapdata(client->adapter);" to get priv.
>>> But my question I don’t see the bus->base address be assigned.
>>>
>> Hi Ryan,
>>
>> In drivers/i2c/busses/i2c-aspeed.c:
>> struct aspeed_i2c_bus {
>>           struct i2c_adapter              adap;
>>           struct device                   *dev;
>>           void __iomem                    *base;
>>           struct reset_control            *rst;
>>           /* Synchronizes I/O mem access to base. */
>>           spinlock_t                      lock;
>>
>> So when "struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);", the
>> bus->base should point to the base of the aspeed_i2c_bus, which is
>> already initialized by the aspeed i2c bus driver.
>>
>> Do I miss something?
> Hello Quan,
> 	After study. I think the ssif_bmc_aspeed.c assume the priv data is the same struct.
> 	That is trick.
> 	Do we have a better for slave enable/disable call back to implement this?
> 	If add call back in struct i2c_algorithm; is it workable?
>>

Hi Ryan,

I dont know which is better, ie: adding callback to struct i2c_algorithm 
or to struct i2c_adapter.
I have tried to add generic callback to struct i2c_adapter as below and 
it works.

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 4e7714c88f95..6e9abf2d6abb 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -713,6 +713,10 @@ struct i2c_adapter {
  	const struct i2c_adapter_quirks *quirks;

  	struct irq_domain *host_notify_domain;
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+	int (*slave_enable)(struct i2c_adapter *adap);
+	int (*slave_disable)(struct i2c_adapter *adap);
+#endif
  };
  #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)

>>>> +extern void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy);
>>>> +static void aspeed_set_ssif_bmc_status(struct ssif_bmc_ctx *ssif_bmc,
>>>> unsigned int status)
>>>> +{
>>>> +	if (status & SSIF_BMC_BUSY)
>>>> +		aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, true);
>>>> +	else if (status & SSIF_BMC_READY)
>>>> +		aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv, false); }
>>>> +
>>>> +static int ssif_bmc_probe(struct i2c_client *client, const struct
>>>> i2c_device_id *id)
>>>> +{
>>>> +	struct ssif_bmc_ctx *ssif_bmc;
>>>> +
>>>> +	ssif_bmc = ssif_bmc_alloc(client, 0);
>>>> +	if (IS_ERR(ssif_bmc))
>>>> +		return PTR_ERR(ssif_bmc);
>>>> +
>>>> +	ssif_bmc->priv = i2c_get_adapdata(client->adapter);
>>>> +	ssif_bmc->set_ssif_bmc_status = aspeed_set_ssif_bmc_status;
>>>> +
>>>> +	return 0;
>>>> +}
>>>>
>>>> - Quan
>>>>
>>>>
>>>
>
Quan Nguyen May 28, 2021, 1 a.m. UTC | #17
On 21/05/2021 13:09, Ryan Chen wrote:
>> -----Original Message-----

>> From: Quan Nguyen <quan@os.amperecomputing.com>

>> Sent: Thursday, May 20, 2021 10:10 PM

>> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard

>> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley

>> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins

>> <brendanhiggins@google.com>; Benjamin Herrenschmidt

>> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel

>> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net;

>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;

>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;

>> linux-i2c@vger.kernel.org

>> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .

>> Nguyen <thang@os.amperecomputing.com>; Phong Vo

>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org

>> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

>>

>> On 20/05/2021 18:06, Ryan Chen wrote:

>>>> -----Original Message-----

>>>> From: openbmc

>>>> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On

>> Behalf

>>>> Of Quan Nguyen

>>>> Sent: Wednesday, May 19, 2021 3:50 PM

>>>> To: Corey Minyard <minyard@acm.org>; Rob Herring

>>>> <robh+dt@kernel.org>; Joel Stanley <joel@jms.id.au>; Andrew Jeffery

>>>> <andrew@aj.id.au>; Brendan Higgins <brendanhiggins@google.com>;

>>>> Benjamin Herrenschmidt <benh@kernel.crashing.org>; Wolfram Sang

>>>> <wsa@kernel.org>; Philipp Zabel <p.zabel@pengutronix.de>;

>>>> openipmi-developer@lists.sourceforge.net;

>>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;

>>>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;

>>>> linux-i2c@vger.kernel.org

>>>> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .

>>>> Nguyen <thang@os.amperecomputing.com>; Phong Vo

>>>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org

>>>> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

>>>>

>>>> Slave i2c device on AST2500 received a lot of slave irq while it is

>>>> busy processing the response. To handle this case, adds and exports

>>>> aspeed_set_slave_busy() for controller to temporary stop slave irq

>>>> while slave is handling the response, and re-enable them again when the

>> response is ready.

>>>>

>>>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>

>>>> ---

>>>> v3:

>>>>     + First introduce in v3 [Quan]

>>>>

>>>>    drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++

>>>>    1 file changed, 20 insertions(+)

>>>>

>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c

>>>> b/drivers/i2c/busses/i2c-aspeed.c index b2e9c8f0ddf7..9926d04831a2

>>>> 100644

>>>> --- a/drivers/i2c/busses/i2c-aspeed.c

>>>> +++ b/drivers/i2c/busses/i2c-aspeed.c

>>>> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus

>>>> *bus,

>>>>    	return 0;

>>>>    }

>>>>

>>>> +#if IS_ENABLED(CONFIG_I2C_SLAVE)

>>>> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) {

>>>> +	struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);

>>>> +	unsigned long current_mask, flags;

>>>> +

>>>> +	spin_lock_irqsave(&bus->lock, flags);

>>>> +

>>>> +	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);

>>>> +	if (busy)

>>>> +		current_mask &= ~(ASPEED_I2CD_INTR_RX_DONE |

>>>> ASPEED_I2CD_INTR_SLAVE_MATCH);

>>>> +	else

>>>> +		current_mask |= ASPEED_I2CD_INTR_RX_DONE |

>>>> ASPEED_I2CD_INTR_SLAVE_MATCH;

>>>> +	writel(current_mask, bus->base + ASPEED_I2C_INTR_CTRL_REG);

>>>> +

>>>> +	spin_unlock_irqrestore(&bus->lock, flags); }

>>>> +EXPORT_SYMBOL_GPL(aspeed_set_slave_busy);

>>>> +#endif

>>>> +

>>>>    static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus)  {

>>>>    	struct platform_device *pdev = to_platform_device(bus->dev);

>>>> --

>>>> 2.28.0

>>>

>>> Hello,

>>> 	The better idea is use disable i2c slave mode.

>>> 	Due to if i2c controller running in slave will get slave match, and latch the

>> SCL.

>>> 	Until cpu clear interrupt status.

>>> Ryan

>>>

>> Thanks Ryan,

>>

>> Do you mean to enable/disable slave function as per example code below ?

> Yes. it is.


Dear Ryan,

This solution looks good. I'll switch to use this way in next version.
Thanks for the suggestion.

- Quan

>>

>>           /* Turn on slave mode. */

>>           func_ctrl_reg_val = readl(bus->base +

>> ASPEED_I2C_FUN_CTRL_REG);

>>           func_ctrl_reg_val |= ASPEED_I2CD_SLAVE_EN;

>>           writel(func_ctrl_reg_val, bus->base +

>> ASPEED_I2C_FUN_CTRL_REG);

>>

>> Will try this idea.

>> - Quan
Ryan Chen May 28, 2021, 2:57 a.m. UTC | #18
> -----Original Message-----

> From: Quan Nguyen <quan@os.amperecomputing.com>

> Sent: Friday, May 28, 2021 8:53 AM

> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard

> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley

> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins

> <brendanhiggins@google.com>; Benjamin Herrenschmidt

> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp Zabel

> <p.zabel@pengutronix.de>; openipmi-developer@lists.sourceforge.net;

> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;

> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;

> linux-i2c@vger.kernel.org

> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .

> Nguyen <thang@os.amperecomputing.com>; Phong Vo

> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org

> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

> 

> On 25/05/2021 17:30, Ryan Chen wrote:

> >> -----Original Message-----

> >> From: Quan Nguyen <quan@os.amperecomputing.com>

> >> Sent: Monday, May 24, 2021 6:49 PM

> >> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard

> >> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley

> >> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins

> >> <brendanhiggins@google.com>; Benjamin Herrenschmidt

> >> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp

> >> Zabel <p.zabel@pengutronix.de>;

> >> openipmi-developer@lists.sourceforge.net;

> >> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;

> >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;

> >> linux-i2c@vger.kernel.org

> >> Cc: Open Source Submission <patches@amperecomputing.com>; Thang Q .

> >> Nguyen <thang@os.amperecomputing.com>; Phong Vo

> >> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org

> >> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

> >>

> >> On 24/05/2021 17:36, Ryan Chen wrote:

> >>>> -----Original Message-----

> >>>> From: Quan Nguyen <quan@os.amperecomputing.com>

> >>>> Sent: Monday, May 24, 2021 6:20 PM

> >>>> To: Ryan Chen <ryan_chen@aspeedtech.com>; Corey Minyard

> >>>> <minyard@acm.org>; Rob Herring <robh+dt@kernel.org>; Joel Stanley

> >>>> <joel@jms.id.au>; Andrew Jeffery <andrew@aj.id.au>; Brendan Higgins

> >>>> <brendanhiggins@google.com>; Benjamin Herrenschmidt

> >>>> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>; Philipp

> >>>> Zabel <p.zabel@pengutronix.de>;

> >>>> openipmi-developer@lists.sourceforge.net;

> >>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;

> >>>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;

> >>>> linux-i2c@vger.kernel.org

> >>>> Cc: Open Source Submission <patches@amperecomputing.com>; Thang

> Q .

> >>>> Nguyen <thang@os.amperecomputing.com>; Phong Vo

> >>>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org

> >>>> Subject: Re: [PATCH v3 5/7] i2c: aspeed: Add

> >>>> aspeed_set_slave_busy()

> >>>>

> >>>> On 24/05/2021 17:06, Ryan Chen wrote:

> >>>>>> -----Original Message-----

> >>>>>> From: openbmc

> >>>>>> <openbmc-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On

> >>>> Behalf

> >>>>>> Of Quan Nguyen

> >>>>>> Sent: Wednesday, May 19, 2021 3:50 PM

> >>>>>> To: Corey Minyard <minyard@acm.org>; Rob Herring

> >>>>>> <robh+dt@kernel.org>; Joel Stanley <joel@jms.id.au>; Andrew

> >>>>>> Jeffery <andrew@aj.id.au>; Brendan Higgins

> >>>>>> <brendanhiggins@google.com>; Benjamin Herrenschmidt

> >>>>>> <benh@kernel.crashing.org>; Wolfram Sang <wsa@kernel.org>;

> >>>>>> Philipp Zabel <p.zabel@pengutronix.de>;

> >>>>>> openipmi-developer@lists.sourceforge.net;

> >>>>>> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;

> >>>>>> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;

> >>>>>> linux-i2c@vger.kernel.org

> >>>>>> Cc: Open Source Submission <patches@amperecomputing.com>;

> Thang

> >> Q .

> >>>>>> Nguyen <thang@os.amperecomputing.com>; Phong Vo

> >>>>>> <phong@os.amperecomputing.com>; openbmc@lists.ozlabs.org

> >>>>>> Subject: [PATCH v3 5/7] i2c: aspeed: Add aspeed_set_slave_busy()

> >>>>>>

> >>>>>> Slave i2c device on AST2500 received a lot of slave irq while it

> >>>>>> is busy processing the response. To handle this case, adds and

> >>>>>> exports

> >>>>>> aspeed_set_slave_busy() for controller to temporary stop slave

> >>>>>> irq while slave is handling the response, and re-enable them

> >>>>>> again when the

> >>>> response is ready.

> >>>>>>

> >>>>>> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>

> >>>>>> ---

> >>>>>> v3:

> >>>>>>      + First introduce in v3 [Quan]

> >>>>>>

> >>>>>>     drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++

> >>>>>>     1 file changed, 20 insertions(+)

> >>>>>>

> >>>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c

> >>>>>> b/drivers/i2c/busses/i2c-aspeed.c index

> >>>>>> b2e9c8f0ddf7..9926d04831a2

> >>>>>> 100644

> >>>>>> --- a/drivers/i2c/busses/i2c-aspeed.c

> >>>>>> +++ b/drivers/i2c/busses/i2c-aspeed.c

> >>>>>> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct

> >>>>>> aspeed_i2c_bus *bus,

> >>>>>>     	return 0;

> >>>>>>     }

> >>>>>>

> >>>>>> +#if IS_ENABLED(CONFIG_I2C_SLAVE) void

> >>>>>> +aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy) {

> >>>>>> +	struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);

> >>>>>> +	unsigned long current_mask, flags;

> >>>>>> +

> >>>>>> +	spin_lock_irqsave(&bus->lock, flags);

> >>>>>> +

> >>>>>> +	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);

> >>>>> Hello

> >>>>> 	Where the bus->base to be remap?

> >>>>>

> >>>>

> >>>> Hi Ryan,

> >>>>

> >>>> In "[PATCH v3 6/7] ipmi: ssif_bmc: Add Aspeed SSIF BMC driver", the

> >>>> ->priv is retrieved by calling i2c_get_adapdata(client->adapter).

> >>>> ->And in

> >>>> aspeed_set_ssif_bmc_status(), call the exported

> >>>> aspeed_set_slave_busy() using ->priv pointer as code below.

> >>>>

> >>> Yes, I see the probe function " ssif_bmc->priv =

> >> i2c_get_adapdata(client->adapter);" to get priv.

> >>> But my question I don’t see the bus->base address be assigned.

> >>>

> >> Hi Ryan,

> >>

> >> In drivers/i2c/busses/i2c-aspeed.c:

> >> struct aspeed_i2c_bus {

> >>           struct i2c_adapter              adap;

> >>           struct device                   *dev;

> >>           void __iomem                    *base;

> >>           struct reset_control            *rst;

> >>           /* Synchronizes I/O mem access to base. */

> >>           spinlock_t                      lock;

> >>

> >> So when "struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);", the

> >> bus->base should point to the base of the aspeed_i2c_bus, which is

> >> already initialized by the aspeed i2c bus driver.

> >>

> >> Do I miss something?

> > Hello Quan,

> > 	After study. I think the ssif_bmc_aspeed.c assume the priv data is the

> same struct.

> > 	That is trick.

> > 	Do we have a better for slave enable/disable call back to implement this?

> > 	If add call back in struct i2c_algorithm; is it workable?

> >>

> 

> Hi Ryan,

> 

> I dont know which is better, ie: adding callback to struct i2c_algorithm or to

> struct i2c_adapter.

> I have tried to add generic callback to struct i2c_adapter as below and it

> works.

> 

Hello Quan,
	Thanks your feedback.
	Because, if we apply it in callback. It can unify it in ssif_bmc.c to do callback.
	Not have ssif_bmc_aspeed.c, ssif_bmc_xxx.c .....
	Because the priv struct is different in different i2c driver implement. 
Ryan
	

> diff --git a/include/linux/i2c.h b/include/linux/i2c.h index

> 4e7714c88f95..6e9abf2d6abb 100644

> --- a/include/linux/i2c.h

> +++ b/include/linux/i2c.h

> @@ -713,6 +713,10 @@ struct i2c_adapter {

>   	const struct i2c_adapter_quirks *quirks;

> 

>   	struct irq_domain *host_notify_domain;

> +#if IS_ENABLED(CONFIG_I2C_SLAVE)

> +	int (*slave_enable)(struct i2c_adapter *adap);

> +	int (*slave_disable)(struct i2c_adapter *adap); #endif

>   };

>   #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)

> 

> >>>> +extern void aspeed_set_slave_busy(struct i2c_adapter *adap, bool

> >>>> +busy); static void aspeed_set_ssif_bmc_status(struct ssif_bmc_ctx

> >>>> +*ssif_bmc,

> >>>> unsigned int status)

> >>>> +{

> >>>> +	if (status & SSIF_BMC_BUSY)

> >>>> +		aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv,

> true);

> >>>> +	else if (status & SSIF_BMC_READY)

> >>>> +		aspeed_set_slave_busy((struct i2c_adapter *)ssif_bmc->priv,

> >>>> +false); }

> >>>> +

> >>>> +static int ssif_bmc_probe(struct i2c_client *client, const struct

> >>>> i2c_device_id *id)

> >>>> +{

> >>>> +	struct ssif_bmc_ctx *ssif_bmc;

> >>>> +

> >>>> +	ssif_bmc = ssif_bmc_alloc(client, 0);

> >>>> +	if (IS_ERR(ssif_bmc))

> >>>> +		return PTR_ERR(ssif_bmc);

> >>>> +

> >>>> +	ssif_bmc->priv = i2c_get_adapdata(client->adapter);

> >>>> +	ssif_bmc->set_ssif_bmc_status = aspeed_set_ssif_bmc_status;

> >>>> +

> >>>> +	return 0;

> >>>> +}

> >>>>

> >>>> - Quan

> >>>>

> >>>>

> >>>

> >
Graeme Gregory June 7, 2021, 2:57 p.m. UTC | #19
On Wed, May 19, 2021 at 02:49:32PM +0700, Quan Nguyen wrote:
> Slave i2c device on AST2500 received a lot of slave irq while it is
> busy processing the response. To handle this case, adds and exports
> aspeed_set_slave_busy() for controller to temporary stop slave irq
> while slave is handling the response, and re-enable them again when
> the response is ready.
> 
> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>
> ---
> v3:
>   + First introduce in v3 [Quan]
> 
>  drivers/i2c/busses/i2c-aspeed.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index b2e9c8f0ddf7..9926d04831a2 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -944,6 +944,26 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus *bus,
>  	return 0;
>  }
>  
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +void aspeed_set_slave_busy(struct i2c_adapter *adap, bool busy)
> +{
> +	struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);
> +	unsigned long current_mask, flags;
> +
> +	spin_lock_irqsave(&bus->lock, flags);

This as far as I can see is still a recursive spinlock, and the spinlock
debugger seems to agree with me.

Graeme

> +
> +	current_mask = readl(bus->base + ASPEED_I2C_INTR_CTRL_REG);
> +	if (busy)
> +		current_mask &= ~(ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_SLAVE_MATCH);
> +	else
> +		current_mask |= ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_SLAVE_MATCH;
> +	writel(current_mask, bus->base + ASPEED_I2C_INTR_CTRL_REG);
> +
> +	spin_unlock_irqrestore(&bus->lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(aspeed_set_slave_busy);
> +#endif
> +
>  static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus)
>  {
>  	struct platform_device *pdev = to_platform_device(bus->dev);
> -- 
> 2.28.0
>
Wolfram Sang June 25, 2021, 3:02 p.m. UTC | #20
On Wed, May 19, 2021 at 02:49:28PM +0700, Quan Nguyen wrote:
> Expose the PEC calculation i2c_smbus_pec() for generic use.

> 

> Signed-off-by: Quan Nguyen <quan@os.amperecomputing.com>


You are the second one who wanted this exported. I agree it makes sense
for slave drivers. So, I'll skip the required user because two are in
flight. Applied to for-next, thanks!