linux-i2c.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH 0/6] i2c: Enabling use of aspeed-i2c with ACPI
@ 2023-05-25 15:21 Jonathan Cameron
  2023-05-25 15:21 ` [RFC PATCH 1/6] i2c: acpi: set slave mode flag Jonathan Cameron
                   ` (6 more replies)
  0 siblings, 7 replies; 16+ messages in thread
From: Jonathan Cameron @ 2023-05-25 15:21 UTC (permalink / raw)
  To: linux-i2c
  Cc: Wolfram Sang, Niyas Sait, Klaus Jensen, Andy Shevchenko,
	linux-acpi, Jeremy Kerr, Matt Johnston,
	Shesha Bhushan Sreenivasamurthy, linux-cxl, linuxarm,
	Viacheslav A . Dubeyko

From the school of dirty hacks we do to prove something works, enable
work to proceed elsewhere:

MCTP over I2C from ACPI emulated hosts (both x86 and ARM64).

The first 4 patches might be suitable for upstream inclusion, the
last two not - though I hope we can move to Niyas' work on ACPI clock
management once that is ready.

Why do this crazy thing?

Ultimately we want a standards based way to use the CXL Fabric Management
API FM-API. In real systems that is likely to be driven from a separate
'host' such as a BMC, but for test purposes it is convenient to be able
to do that from an QEMU emulated machine that is also capable of using
the CXL kernel stack.

That CXL kernel stack is currently ACPI only (and people care about
x86 for some reason). One of the defined interfaces over which FM-API
commands can be issued is MCTP.

The kernel MCTP stack has upstream drivers for MCTP over I2C.
Upstream QEMU emulates the Aspeed I2C controller with the necessary
two way support. There are patches on list adding the MCTP parts
https://lore.kernel.org/qemu-devel/20230425063540.46143-2-its@irrelevant.dk/
and I've ported an earlier CXL FMAPI EP emulator over to that.

ACPI has a 'magic' HID of PRP0001 which allows the use of a device tree binding
(mostly) with an ACPI DSDT entry.  A suitable chunk is something like

(dumped from a working x86 setup)

    Device (MCTP)
    {
        Name (_HID, "PRP0001")  // _HID: Hardware ID
        Name (_DSD, Package (0x02)  // _DSD: Device-Specific Data
        {
            ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */,
            Package (0x03)
            {
                Package (0x02)
                {
                     "compatible",
                     "aspeed,ast2600-i2c-bus"
                },
                Package (0x02)
                {
                    "bus-frequency",
                    0x00061A80
                },
                Package (0x02)
                {
                    "mctp-controller",
                    One
                }
            }
        })
        Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
        {
            QWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
                0x0000000000000000, // Granularity
                0x00000004800FC080, // Range Minimum
                0x00000004800FC0FF, // Range Maximum
                0x0000000000000000, // Translation Offset
                0x0000000000000080, // Length
                ,, , AddressRangeMemory, TypeStatic)
            Interrupt (ResourceConsumer, Level, ActiveHigh, Shared, ,, )
            {
                0x00000007,
            }
        })
    }
    Device (MCTS)
    {
        Name (_HID, "PRP0001")  // _HID: Hardware ID
        Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
        {
            I2cSerialBusV2 (0x0050, DeviceInitiated, 0x000186A0,
                AddressingMode7Bit, "\\_SB.MCTP",
                0x00, ResourceProducer, , Exclusive,
                )
        })
        Name (_DSD, Package (0x02)  // _DSD: Device-Specific Data
        {
            ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */,
            Package (0x01)
            {
                Package (0x02)
                {
                    "compatible",
                    "mctp-i2c-controller"
                }
            }
        })
    }

QEMU patches will follow soon and will include documentation on
how to actually poke this to do something useful. I'll post a reply
to this with the link when posted.

Cc: Niyas Sait <niyas.sait@linaro.org>
Cc: Klaus Jensen <its@irrelevant.dk>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Jeremy Kerr <jk@codeconstruct.com.au>
Cc: Matt Johnston <matt@codeconstruct.com.au>
Cc: Shesha Bhushan Sreenivasamurthy <sheshas@marvell.com>

Jonathan Cameron (6):
  i2c: acpi: set slave mode flag
  i2c: aspeed: Use platform_get_irq() instead of opencoding
  i2c: aspeed: switch to generic fw properties.
  i2c: aspeed: Set the fwnode for the adap->dev
  HACK: i2c: aspeed: Comment the clock and reset out.
  HACK: i2c: aspeed: Enable build without COMPILE_TEST

 drivers/i2c/busses/Kconfig      |  1 -
 drivers/i2c/busses/i2c-aspeed.c | 36 ++++++++++++++++-----------------
 drivers/i2c/i2c-core-acpi.c     |  3 +++
 3 files changed, 20 insertions(+), 20 deletions(-)


base-commit: f1fcbaa18b28dec10281551dfe6ed3a3ed80e3d6
-- 
2.39.2


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [RFC PATCH 1/6] i2c: acpi: set slave mode flag
  2023-05-25 15:21 [RFC PATCH 0/6] i2c: Enabling use of aspeed-i2c with ACPI Jonathan Cameron
@ 2023-05-25 15:21 ` Jonathan Cameron
  2023-05-26 19:43   ` Andy Shevchenko
  2023-05-25 15:21 ` [RFC PATCH 2/6] i2c: aspeed: Use platform_get_irq() instead of opencoding Jonathan Cameron
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Jonathan Cameron @ 2023-05-25 15:21 UTC (permalink / raw)
  To: linux-i2c
  Cc: Wolfram Sang, Niyas Sait, Klaus Jensen, Andy Shevchenko,
	linux-acpi, Jeremy Kerr, Matt Johnston,
	Shesha Bhushan Sreenivasamurthy, linux-cxl, linuxarm,
	Viacheslav A . Dubeyko

If the GenericSerialBusConnection includes the General Flag
for slave mode set it during parsing.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/i2c/i2c-core-acpi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index d6037a328669..7dda5eab9645 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -125,6 +125,9 @@ static int i2c_acpi_fill_info(struct acpi_resource *ares, void *data)
 	if (sb->access_mode == ACPI_I2C_10BIT_MODE)
 		info->flags |= I2C_CLIENT_TEN;
 
+	if (sb->slave_mode)
+		info->flags |= I2C_CLIENT_SLAVE;
+
 	return 1;
 }
 
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [RFC PATCH 2/6] i2c: aspeed: Use platform_get_irq() instead of opencoding
  2023-05-25 15:21 [RFC PATCH 0/6] i2c: Enabling use of aspeed-i2c with ACPI Jonathan Cameron
  2023-05-25 15:21 ` [RFC PATCH 1/6] i2c: acpi: set slave mode flag Jonathan Cameron
@ 2023-05-25 15:21 ` Jonathan Cameron
  2023-05-26 21:06   ` Andy Shevchenko
  2023-05-25 15:22 ` [RFC PATCH 3/6] i2c: aspeed: switch to generic fw properties Jonathan Cameron
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Jonathan Cameron @ 2023-05-25 15:21 UTC (permalink / raw)
  To: linux-i2c
  Cc: Wolfram Sang, Niyas Sait, Klaus Jensen, Andy Shevchenko,
	linux-acpi, Jeremy Kerr, Matt Johnston,
	Shesha Bhushan Sreenivasamurthy, linux-cxl, linuxarm,
	Viacheslav A . Dubeyko

A cleanup in it's own right.
This has the handy side effect of working for ACPI FW as well
(unlike fwnode_irq_get() which works for ARM64 but not x86 ACPI)

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/i2c/busses/i2c-aspeed.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index d3c99c5b3247..04155c9a50a5 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -19,7 +19,6 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of_address.h>
-#include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/reset.h>
@@ -1043,7 +1042,7 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
-	irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
+	irq = platform_get_irq(pdev, 0);
 	ret = devm_request_irq(&pdev->dev, irq, aspeed_i2c_bus_irq,
 			       0, dev_name(&pdev->dev), bus);
 	if (ret < 0)
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [RFC PATCH 3/6] i2c: aspeed: switch to generic fw properties.
  2023-05-25 15:21 [RFC PATCH 0/6] i2c: Enabling use of aspeed-i2c with ACPI Jonathan Cameron
  2023-05-25 15:21 ` [RFC PATCH 1/6] i2c: acpi: set slave mode flag Jonathan Cameron
  2023-05-25 15:21 ` [RFC PATCH 2/6] i2c: aspeed: Use platform_get_irq() instead of opencoding Jonathan Cameron
@ 2023-05-25 15:22 ` Jonathan Cameron
  2023-05-26 21:11   ` Andy Shevchenko
  2023-05-25 15:22 ` [RFC PATCH 4/6] i2c: aspeed: Set the fwnode for the adap->dev Jonathan Cameron
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Jonathan Cameron @ 2023-05-25 15:22 UTC (permalink / raw)
  To: linux-i2c
  Cc: Wolfram Sang, Niyas Sait, Klaus Jensen, Andy Shevchenko,
	linux-acpi, Jeremy Kerr, Matt Johnston,
	Shesha Bhushan Sreenivasamurthy, linux-cxl, linuxarm,
	Viacheslav A . Dubeyko

Not tested on device tree but works nicely for ACPI :)

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/i2c/busses/i2c-aspeed.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 04155c9a50a5..73508fb9dc08 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -18,9 +18,8 @@
 #include <linux/irq.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/of_address.h>
-#include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <linux/reset.h>
 #include <linux/slab.h>
 
@@ -975,7 +974,6 @@ MODULE_DEVICE_TABLE(of, aspeed_i2c_bus_of_table);
 
 static int aspeed_i2c_probe_bus(struct platform_device *pdev)
 {
-	const struct of_device_id *match;
 	struct aspeed_i2c_bus *bus;
 	struct clk *parent_clk;
 	int irq, ret;
@@ -1003,7 +1001,7 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
 	}
 	reset_control_deassert(bus->rst);
 
-	ret = of_property_read_u32(pdev->dev.of_node,
+	ret = device_property_read_u32(&pdev->dev,
 				   "bus-frequency", &bus->bus_frequency);
 	if (ret < 0) {
 		dev_err(&pdev->dev,
@@ -1011,12 +1009,10 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
 		bus->bus_frequency = I2C_MAX_STANDARD_MODE_FREQ;
 	}
 
-	match = of_match_node(aspeed_i2c_bus_of_table, pdev->dev.of_node);
-	if (!match)
+	bus->get_clk_reg_val = (u32 (*)(struct device *, u32))
+		device_get_match_data(&pdev->dev);
+	if (!bus->get_clk_reg_val)
 		bus->get_clk_reg_val = aspeed_i2c_24xx_get_clk_reg_val;
-	else
-		bus->get_clk_reg_val = (u32 (*)(struct device *, u32))
-				match->data;
 
 	/* Initialize the I2C adapter */
 	spin_lock_init(&bus->lock);
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [RFC PATCH 4/6] i2c: aspeed: Set the fwnode for the adap->dev
  2023-05-25 15:21 [RFC PATCH 0/6] i2c: Enabling use of aspeed-i2c with ACPI Jonathan Cameron
                   ` (2 preceding siblings ...)
  2023-05-25 15:22 ` [RFC PATCH 3/6] i2c: aspeed: switch to generic fw properties Jonathan Cameron
@ 2023-05-25 15:22 ` Jonathan Cameron
  2023-05-26 21:13   ` Andy Shevchenko
  2023-05-25 15:22 ` [RFC PATCH 5/6] HACK: i2c: aspeed: Comment the clock and reset out Jonathan Cameron
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Jonathan Cameron @ 2023-05-25 15:22 UTC (permalink / raw)
  To: linux-i2c
  Cc: Wolfram Sang, Niyas Sait, Klaus Jensen, Andy Shevchenko,
	linux-acpi, Jeremy Kerr, Matt Johnston,
	Shesha Bhushan Sreenivasamurthy, linux-cxl, linuxarm,
	Viacheslav A . Dubeyko

This is needed for the bus matching used for ACPI based
i2c client registration.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/i2c/busses/i2c-aspeed.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 73508fb9dc08..adbb93444d7a 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -1044,6 +1044,8 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
+	device_set_node(&bus->adap.dev, dev_fwnode(&pdev->dev));
+
 	ret = i2c_add_adapter(&bus->adap);
 	if (ret < 0)
 		return ret;
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [RFC PATCH 5/6] HACK: i2c: aspeed: Comment the clock and reset out.
  2023-05-25 15:21 [RFC PATCH 0/6] i2c: Enabling use of aspeed-i2c with ACPI Jonathan Cameron
                   ` (3 preceding siblings ...)
  2023-05-25 15:22 ` [RFC PATCH 4/6] i2c: aspeed: Set the fwnode for the adap->dev Jonathan Cameron
@ 2023-05-25 15:22 ` Jonathan Cameron
  2023-05-26 21:16   ` Andy Shevchenko
  2023-05-25 15:22 ` [RFC PATCH 6/6] HACK: i2c: aspeed: Enable build without COMPILE_TEST Jonathan Cameron
  2023-05-25 16:18 ` [RFC PATCH 0/6] i2c: Enabling use of aspeed-i2c with ACPI Jonathan Cameron
  6 siblings, 1 reply; 16+ messages in thread
From: Jonathan Cameron @ 2023-05-25 15:22 UTC (permalink / raw)
  To: linux-i2c
  Cc: Wolfram Sang, Niyas Sait, Klaus Jensen, Andy Shevchenko,
	linux-acpi, Jeremy Kerr, Matt Johnston,
	Shesha Bhushan Sreenivasamurthy, linux-cxl, linuxarm,
	Viacheslav A . Dubeyko

Needs tidying up - hopefully can do clock right
using on going work from Niyas
https://linaro.atlassian.net/wiki/spaces/CLIENTPC/pages/28832333867/ACPI+Clock+Management

Don't think ACPI provide an equivalent reset deasert / assert. _RST
doesn't fit that model.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/i2c/busses/i2c-aspeed.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index adbb93444d7a..df20382970f3 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -986,20 +986,21 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
 	if (IS_ERR(bus->base))
 		return PTR_ERR(bus->base);
 
-	parent_clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(parent_clk))
-		return PTR_ERR(parent_clk);
-	bus->parent_clk_frequency = clk_get_rate(parent_clk);
+	//	parent_clk = devm_clk_get(&pdev->dev, NULL);
+	//	if (IS_ERR(parent_clk))//
+	//		return PTR_ERR(parent_clk);
+	bus->parent_clk_frequency = 1000000;//clk_get_rate(parent_clk);
 	/* We just need the clock rate, we don't actually use the clk object. */
-	devm_clk_put(&pdev->dev, parent_clk);
+	//devm_clk_put(&pdev->dev, parent_clk);
 
 	bus->rst = devm_reset_control_get_shared(&pdev->dev, NULL);
 	if (IS_ERR(bus->rst)) {
-		dev_err(&pdev->dev,
+		dev_warn(&pdev->dev,
 			"missing or invalid reset controller device tree entry\n");
-		return PTR_ERR(bus->rst);
+		bus->rst = 0;
+	} else {
+		reset_control_deassert(bus->rst);
 	}
-	reset_control_deassert(bus->rst);
 
 	ret = device_property_read_u32(&pdev->dev,
 				   "bus-frequency", &bus->bus_frequency);
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [RFC PATCH 6/6] HACK: i2c: aspeed: Enable build without COMPILE_TEST
  2023-05-25 15:21 [RFC PATCH 0/6] i2c: Enabling use of aspeed-i2c with ACPI Jonathan Cameron
                   ` (4 preceding siblings ...)
  2023-05-25 15:22 ` [RFC PATCH 5/6] HACK: i2c: aspeed: Comment the clock and reset out Jonathan Cameron
@ 2023-05-25 15:22 ` Jonathan Cameron
  2023-05-30 14:44   ` Jonathan Cameron
  2023-05-25 16:18 ` [RFC PATCH 0/6] i2c: Enabling use of aspeed-i2c with ACPI Jonathan Cameron
  6 siblings, 1 reply; 16+ messages in thread
From: Jonathan Cameron @ 2023-05-25 15:22 UTC (permalink / raw)
  To: linux-i2c
  Cc: Wolfram Sang, Niyas Sait, Klaus Jensen, Andy Shevchenko,
	linux-acpi, Jeremy Kerr, Matt Johnston,
	Shesha Bhushan Sreenivasamurthy, linux-cxl, linuxarm,
	Viacheslav A . Dubeyko

See cover letter for why...
This works nicely on x86 with appropriate evil emulation.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/i2c/busses/Kconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 87600b4aacb3..96bb5a05e195 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -391,7 +391,6 @@ config I2C_ALTERA
 
 config I2C_ASPEED
 	tristate "Aspeed I2C Controller"
-	depends on ARCH_ASPEED || COMPILE_TEST
 	help
 	  If you say yes to this option, support will be included for the
 	  Aspeed I2C controller.
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH 0/6] i2c: Enabling use of aspeed-i2c with ACPI
  2023-05-25 15:21 [RFC PATCH 0/6] i2c: Enabling use of aspeed-i2c with ACPI Jonathan Cameron
                   ` (5 preceding siblings ...)
  2023-05-25 15:22 ` [RFC PATCH 6/6] HACK: i2c: aspeed: Enable build without COMPILE_TEST Jonathan Cameron
@ 2023-05-25 16:18 ` Jonathan Cameron
  6 siblings, 0 replies; 16+ messages in thread
From: Jonathan Cameron @ 2023-05-25 16:18 UTC (permalink / raw)
  To: linux-i2c
  Cc: Wolfram Sang, Niyas Sait, Klaus Jensen, Andy Shevchenko,
	linux-acpi, Jeremy Kerr, Matt Johnston,
	Shesha Bhushan Sreenivasamurthy, linux-cxl,
	Viacheslav A . Dubeyko

On Thu, 25 May 2023 16:21:57 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> From the school of dirty hacks we do to prove something works, enable
> work to proceed elsewhere:
> 
> MCTP over I2C from ACPI emulated hosts (both x86 and ARM64).
> 
> The first 4 patches might be suitable for upstream inclusion, the
> last two not - though I hope we can move to Niyas' work on ACPI clock
> management once that is ready.
> 
> Why do this crazy thing?
> 
> Ultimately we want a standards based way to use the CXL Fabric Management
> API FM-API. In real systems that is likely to be driven from a separate
> 'host' such as a BMC, but for test purposes it is convenient to be able
> to do that from an QEMU emulated machine that is also capable of using
> the CXL kernel stack.
> 
> That CXL kernel stack is currently ACPI only (and people care about
> x86 for some reason). One of the defined interfaces over which FM-API
> commands can be issued is MCTP.
> 
> The kernel MCTP stack has upstream drivers for MCTP over I2C.
> Upstream QEMU emulates the Aspeed I2C controller with the necessary
> two way support. There are patches on list adding the MCTP parts
> https://lore.kernel.org/qemu-devel/20230425063540.46143-2-its@irrelevant.dk/
> and I've ported an earlier CXL FMAPI EP emulator over to that.
> 
> ACPI has a 'magic' HID of PRP0001 which allows the use of a device tree binding
> (mostly) with an ACPI DSDT entry.  A suitable chunk is something like
> 
> (dumped from a working x86 setup)
> 
>     Device (MCTP)
>     {
>         Name (_HID, "PRP0001")  // _HID: Hardware ID
>         Name (_DSD, Package (0x02)  // _DSD: Device-Specific Data
>         {
>             ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */,
>             Package (0x03)
>             {
>                 Package (0x02)
>                 {
>                      "compatible",
>                      "aspeed,ast2600-i2c-bus"
>                 },
>                 Package (0x02)
>                 {
>                     "bus-frequency",
>                     0x00061A80
>                 },
>                 Package (0x02)
>                 {
>                     "mctp-controller",
>                     One
>                 }
>             }
>         })
>         Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
>         {
>             QWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
>                 0x0000000000000000, // Granularity
>                 0x00000004800FC080, // Range Minimum
>                 0x00000004800FC0FF, // Range Maximum
>                 0x0000000000000000, // Translation Offset
>                 0x0000000000000080, // Length
>                 ,, , AddressRangeMemory, TypeStatic)
>             Interrupt (ResourceConsumer, Level, ActiveHigh, Shared, ,, )
>             {
>                 0x00000007,
>             }
>         })
>     }
>     Device (MCTS)
>     {
>         Name (_HID, "PRP0001")  // _HID: Hardware ID
>         Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
>         {
>             I2cSerialBusV2 (0x0050, DeviceInitiated, 0x000186A0,
>                 AddressingMode7Bit, "\\_SB.MCTP",
>                 0x00, ResourceProducer, , Exclusive,
>                 )
>         })
>         Name (_DSD, Package (0x02)  // _DSD: Device-Specific Data
>         {
>             ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device Properties for _DSD */,
>             Package (0x01)
>             {
>                 Package (0x02)
>                 {
>                     "compatible",
>                     "mctp-i2c-controller"
>                 }
>             }
>         })
>     }
> 
> QEMU patches will follow soon and will include documentation on
> how to actually poke this to do something useful. I'll post a reply
> to this with the link when posted.

https://lore.kernel.org/linux-cxl/20230525160859.32517-1-Jonathan.Cameron@huawei.com/T/#t
https://gitlab.com/jic23/qemu cxl-2023-05-25

> 
> Cc: Niyas Sait <niyas.sait@linaro.org>
> Cc: Klaus Jensen <its@irrelevant.dk>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Jeremy Kerr <jk@codeconstruct.com.au>
> Cc: Matt Johnston <matt@codeconstruct.com.au>
> Cc: Shesha Bhushan Sreenivasamurthy <sheshas@marvell.com>
> 
> Jonathan Cameron (6):
>   i2c: acpi: set slave mode flag
>   i2c: aspeed: Use platform_get_irq() instead of opencoding
>   i2c: aspeed: switch to generic fw properties.
>   i2c: aspeed: Set the fwnode for the adap->dev
>   HACK: i2c: aspeed: Comment the clock and reset out.
>   HACK: i2c: aspeed: Enable build without COMPILE_TEST
> 
>  drivers/i2c/busses/Kconfig      |  1 -
>  drivers/i2c/busses/i2c-aspeed.c | 36 ++++++++++++++++-----------------
>  drivers/i2c/i2c-core-acpi.c     |  3 +++
>  3 files changed, 20 insertions(+), 20 deletions(-)
> 
> 
> base-commit: f1fcbaa18b28dec10281551dfe6ed3a3ed80e3d6


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH 1/6] i2c: acpi: set slave mode flag
  2023-05-25 15:21 ` [RFC PATCH 1/6] i2c: acpi: set slave mode flag Jonathan Cameron
@ 2023-05-26 19:43   ` Andy Shevchenko
  0 siblings, 0 replies; 16+ messages in thread
From: Andy Shevchenko @ 2023-05-26 19:43 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linux-i2c, Wolfram Sang, Niyas Sait, Klaus Jensen,
	Andy Shevchenko, linux-acpi, Jeremy Kerr, Matt Johnston,
	Shesha Bhushan Sreenivasamurthy, linux-cxl, linuxarm,
	Viacheslav A . Dubeyko

On Thu, May 25, 2023 at 6:22 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> If the GenericSerialBusConnection includes the General Flag
> for slave mode set it during parsing.

Since it's obvious that you are using the existing ACPI specification
bits, it's a nice patch!
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Nevertheless the remark is that most likely this feature was never
tested on the ACPI implementations other than ACPICA (evidently by
you). Would it be interesting to know Microsoft's point of view on
this.

> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/i2c/i2c-core-acpi.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> index d6037a328669..7dda5eab9645 100644
> --- a/drivers/i2c/i2c-core-acpi.c
> +++ b/drivers/i2c/i2c-core-acpi.c
> @@ -125,6 +125,9 @@ static int i2c_acpi_fill_info(struct acpi_resource *ares, void *data)
>         if (sb->access_mode == ACPI_I2C_10BIT_MODE)
>                 info->flags |= I2C_CLIENT_TEN;
>
> +       if (sb->slave_mode)
> +               info->flags |= I2C_CLIENT_SLAVE;
> +
>         return 1;
>  }
>
> --
> 2.39.2
>


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH 2/6] i2c: aspeed: Use platform_get_irq() instead of opencoding
  2023-05-25 15:21 ` [RFC PATCH 2/6] i2c: aspeed: Use platform_get_irq() instead of opencoding Jonathan Cameron
@ 2023-05-26 21:06   ` Andy Shevchenko
  0 siblings, 0 replies; 16+ messages in thread
From: Andy Shevchenko @ 2023-05-26 21:06 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linux-i2c, Wolfram Sang, Niyas Sait, Klaus Jensen,
	Andy Shevchenko, linux-acpi, Jeremy Kerr, Matt Johnston,
	Shesha Bhushan Sreenivasamurthy, linux-cxl, linuxarm,
	Viacheslav A . Dubeyko

On Thu, May 25, 2023 at 6:23 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> A cleanup in it's own right.

its ?

> This has the handy side effect of working for ACPI FW as well
> (unlike fwnode_irq_get() which works for ARM64 but not x86 ACPI)

...

> -       irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> +       irq = platform_get_irq(pdev, 0);

It makes sense now not to even try a wrong IRQ, i.e.

  if (irq < 0)
    return irq;

+ blank line here.

>         ret = devm_request_irq(&pdev->dev, irq, aspeed_i2c_bus_irq,
>                                0, dev_name(&pdev->dev), bus);
>         if (ret < 0)


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH 3/6] i2c: aspeed: switch to generic fw properties.
  2023-05-25 15:22 ` [RFC PATCH 3/6] i2c: aspeed: switch to generic fw properties Jonathan Cameron
@ 2023-05-26 21:11   ` Andy Shevchenko
  2023-05-30 14:16     ` Jonathan Cameron
  0 siblings, 1 reply; 16+ messages in thread
From: Andy Shevchenko @ 2023-05-26 21:11 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linux-i2c, Wolfram Sang, Niyas Sait, Klaus Jensen,
	Andy Shevchenko, linux-acpi, Jeremy Kerr, Matt Johnston,
	Shesha Bhushan Sreenivasamurthy, linux-cxl, linuxarm,
	Viacheslav A . Dubeyko

On Thu, May 25, 2023 at 6:23 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> Not tested on device tree but works nicely for ACPI :)

Needs a better commit message obviously :-)

...

> -       ret = of_property_read_u32(pdev->dev.of_node,
> +       ret = device_property_read_u32(&pdev->dev,
>                                    "bus-frequency", &bus->bus_frequency);

Oh, please avoid double effort, i.e. go further and use I²C core APIs
for the timings. Oh, wait, do they use non-standard property?!

...

> +       bus->get_clk_reg_val = (u32 (*)(struct device *, u32))
> +               device_get_match_data(&pdev->dev);

Personally I prefer using pointers in driver_data so we can avoid
ambiguity for the 0/NULL value returned by this call. But if 0 value
is considered invalid here, it's probably fine.

> +       if (!bus->get_clk_reg_val)
>                 bus->get_clk_reg_val = aspeed_i2c_24xx_get_clk_reg_val;
> -       else
> -               bus->get_clk_reg_val = (u32 (*)(struct device *, u32))
> -                               match->data;


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH 4/6] i2c: aspeed: Set the fwnode for the adap->dev
  2023-05-25 15:22 ` [RFC PATCH 4/6] i2c: aspeed: Set the fwnode for the adap->dev Jonathan Cameron
@ 2023-05-26 21:13   ` Andy Shevchenko
  0 siblings, 0 replies; 16+ messages in thread
From: Andy Shevchenko @ 2023-05-26 21:13 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linux-i2c, Wolfram Sang, Niyas Sait, Klaus Jensen,
	Andy Shevchenko, linux-acpi, Jeremy Kerr, Matt Johnston,
	Shesha Bhushan Sreenivasamurthy, linux-cxl, linuxarm,
	Viacheslav A . Dubeyko

On Thu, May 25, 2023 at 6:24 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> This is needed for the bus matching used for ACPI based
> i2c client registration.

...

> +       device_set_node(&bus->adap.dev, dev_fwnode(&pdev->dev));

Please, remove this
https://elixir.bootlin.com/linux/v6.4-rc1/source/drivers/i2c/busses/i2c-aspeed.c#L1029

at the same time.

>         ret = i2c_add_adapter(&bus->adap);
>         if (ret < 0)
>                 return ret;

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH 5/6] HACK: i2c: aspeed: Comment the clock and reset out.
  2023-05-25 15:22 ` [RFC PATCH 5/6] HACK: i2c: aspeed: Comment the clock and reset out Jonathan Cameron
@ 2023-05-26 21:16   ` Andy Shevchenko
  2023-05-30 14:40     ` Jonathan Cameron
  0 siblings, 1 reply; 16+ messages in thread
From: Andy Shevchenko @ 2023-05-26 21:16 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linux-i2c, Wolfram Sang, Niyas Sait, Klaus Jensen,
	Andy Shevchenko, linux-acpi, Jeremy Kerr, Matt Johnston,
	Shesha Bhushan Sreenivasamurthy, linux-cxl, linuxarm,
	Viacheslav A . Dubeyko

On Thu, May 25, 2023 at 6:24 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> Needs tidying up - hopefully can do clock right
> using on going work from Niyas
> https://linaro.atlassian.net/wiki/spaces/CLIENTPC/pages/28832333867/ACPI+Clock+Management

For the current code base the easiest way is to switch to _optional
for clock, or request them based on the type of the fwnode. (Personal
preference is the _optional() API to call). For the reset isn't it
transparent already so we got a dummy control (as for regulator)?


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH 3/6] i2c: aspeed: switch to generic fw properties.
  2023-05-26 21:11   ` Andy Shevchenko
@ 2023-05-30 14:16     ` Jonathan Cameron
  0 siblings, 0 replies; 16+ messages in thread
From: Jonathan Cameron @ 2023-05-30 14:16 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: linux-i2c, Wolfram Sang, Niyas Sait, Klaus Jensen,
	Andy Shevchenko, linux-acpi, Jeremy Kerr, Matt Johnston,
	Shesha Bhushan Sreenivasamurthy, linux-cxl, linuxarm,
	Viacheslav A . Dubeyko

On Sat, 27 May 2023 00:11:09 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, May 25, 2023 at 6:23 PM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > Not tested on device tree but works nicely for ACPI :)  

I was planning to abandon these as 'on list for anyone who
cared' but now you've reviewed them I guess I better do
an RFC v2 :)

> 
> Needs a better commit message obviously :-)

:)

> 
> ...
> 
> > -       ret = of_property_read_u32(pdev->dev.of_node,
> > +       ret = device_property_read_u32(&pdev->dev,
> >                                    "bus-frequency", &bus->bus_frequency);  
> 
> Oh, please avoid double effort, i.e. go further and use I²C core APIs
> for the timings. Oh, wait, do they use non-standard property?!

yup :(

Though it is documented as having a default of 100kHz in the devicetree
binding so the original code shouldn't be calling dev_err() and should
just do:

	bus->frequency = I2C_MAX_STANDARD_MODE_FREQ;
	device_property_read_u32(&pdev->dev,
				 "bus-frequency, &bus->frequency);

Fixing that is an unrelated change though. I'll do it for dt
in a precusor patch then carry that forward to here.

> 
> ...
> 
> > +       bus->get_clk_reg_val = (u32 (*)(struct device *, u32))
> > +               device_get_match_data(&pdev->dev);  
> 
> Personally I prefer using pointers in driver_data so we can avoid
> ambiguity for the 0/NULL value returned by this call. But if 0 value
> is considered invalid here, it's probably fine.

It is a pointer, just a function pointer rather than to a structure.
I could wrap it up in a structure but that would be an unrelated
driver change so at very least a separate patch. 

> 
> > +       if (!bus->get_clk_reg_val)
> >                 bus->get_clk_reg_val = aspeed_i2c_24xx_get_clk_reg_val;
> > -       else
> > -               bus->get_clk_reg_val = (u32 (*)(struct device *, u32))
> > -                               match->data;  
> 
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH 5/6] HACK: i2c: aspeed: Comment the clock and reset out.
  2023-05-26 21:16   ` Andy Shevchenko
@ 2023-05-30 14:40     ` Jonathan Cameron
  0 siblings, 0 replies; 16+ messages in thread
From: Jonathan Cameron @ 2023-05-30 14:40 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: linux-i2c, Wolfram Sang, Niyas Sait, Klaus Jensen,
	Andy Shevchenko, linux-acpi, Jeremy Kerr, Matt Johnston,
	Shesha Bhushan Sreenivasamurthy, linux-cxl, linuxarm,
	Viacheslav A . Dubeyko

On Sat, 27 May 2023 00:16:36 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Thu, May 25, 2023 at 6:24 PM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > Needs tidying up - hopefully can do clock right
> > using on going work from Niyas
> > https://linaro.atlassian.net/wiki/spaces/CLIENTPC/pages/28832333867/ACPI+Clock+Management  
> 
> For the current code base the easiest way is to switch to _optional
> for clock, or request them based on the type of the fwnode. (Personal
> preference is the _optional() API to call). 

Absolutely agree that would the way to go if people want to support my
crazy.

However, that will leave the input clock frequency unknown which means we'll
program a garbage value into one of the device registers. Doesn't matter
to me, but not good in general.

This is avoiding for now the questions of:
1) Why devm for a clock we hold for 2 lines of code, none of which
   have an error return path...
2) clk_get_rate() is documented as not guaranteed to do anything for
   a clk until enabled, so this is relying on it being enabled by
   someone else or a quirk of the the chip. 

> For the reset isn't it
> transparent already so we got a dummy control (as for regulator)?

I don't think so, but maybe I'm missing something.
There is a devm_reset_control_get_optional() though, similar to the clock
one that returns a NULL if not present. 

I'll use that here to make this a slightly less ugly hack.
If I can handle clocks nicely using Niyas' work then can revisit
whether the i2c and aspeed maintainers would accept making the
reset optional.

Jonathan


> 
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [RFC PATCH 6/6] HACK: i2c: aspeed: Enable build without COMPILE_TEST
  2023-05-25 15:22 ` [RFC PATCH 6/6] HACK: i2c: aspeed: Enable build without COMPILE_TEST Jonathan Cameron
@ 2023-05-30 14:44   ` Jonathan Cameron
  0 siblings, 0 replies; 16+ messages in thread
From: Jonathan Cameron @ 2023-05-30 14:44 UTC (permalink / raw)
  To: linux-i2c
  Cc: Wolfram Sang, Niyas Sait, Klaus Jensen, Andy Shevchenko,
	linux-acpi, Jeremy Kerr, Matt Johnston,
	Shesha Bhushan Sreenivasamurthy, linux-cxl,
	Viacheslav A . Dubeyko

On Thu, 25 May 2023 16:22:03 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> See cover letter for why...
> This works nicely on x86 with appropriate evil emulation.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
I'll drop this from v2. It's obviously unnecessary. I'll
just tell people who care to enable COMPILE_TEST
We shouldn't carry my lazy hack around ;)

Jonathan

> ---
>  drivers/i2c/busses/Kconfig | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 87600b4aacb3..96bb5a05e195 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -391,7 +391,6 @@ config I2C_ALTERA
>  
>  config I2C_ASPEED
>  	tristate "Aspeed I2C Controller"
> -	depends on ARCH_ASPEED || COMPILE_TEST
>  	help
>  	  If you say yes to this option, support will be included for the
>  	  Aspeed I2C controller.


^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2023-05-30 14:44 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-25 15:21 [RFC PATCH 0/6] i2c: Enabling use of aspeed-i2c with ACPI Jonathan Cameron
2023-05-25 15:21 ` [RFC PATCH 1/6] i2c: acpi: set slave mode flag Jonathan Cameron
2023-05-26 19:43   ` Andy Shevchenko
2023-05-25 15:21 ` [RFC PATCH 2/6] i2c: aspeed: Use platform_get_irq() instead of opencoding Jonathan Cameron
2023-05-26 21:06   ` Andy Shevchenko
2023-05-25 15:22 ` [RFC PATCH 3/6] i2c: aspeed: switch to generic fw properties Jonathan Cameron
2023-05-26 21:11   ` Andy Shevchenko
2023-05-30 14:16     ` Jonathan Cameron
2023-05-25 15:22 ` [RFC PATCH 4/6] i2c: aspeed: Set the fwnode for the adap->dev Jonathan Cameron
2023-05-26 21:13   ` Andy Shevchenko
2023-05-25 15:22 ` [RFC PATCH 5/6] HACK: i2c: aspeed: Comment the clock and reset out Jonathan Cameron
2023-05-26 21:16   ` Andy Shevchenko
2023-05-30 14:40     ` Jonathan Cameron
2023-05-25 15:22 ` [RFC PATCH 6/6] HACK: i2c: aspeed: Enable build without COMPILE_TEST Jonathan Cameron
2023-05-30 14:44   ` Jonathan Cameron
2023-05-25 16:18 ` [RFC PATCH 0/6] i2c: Enabling use of aspeed-i2c with ACPI Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).