linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] i2c: aspeed: disable additional device addresses on ast2[56]xx
@ 2020-09-15 18:45 Zev Weiss
  2020-12-18 21:39 ` Zev Weiss
  0 siblings, 1 reply; 3+ messages in thread
From: Zev Weiss @ 2020-09-15 18:45 UTC (permalink / raw)
  To: Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
	Andrew Jeffery, linux-i2c, openbmc, linux-arm-kernel,
	linux-aspeed, linux-kernel
  Cc: Zev Weiss

The ast25xx and ast26xx have, respectively, two and three configurable
slave device addresses to the ast24xx's one.  We only support using
one at a time, but the others may come up in an indeterminate state
depending on hardware/bootloader behavior, so we need to make sure we
disable them so as to avoid ending up with phantom devices on the bus.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 drivers/i2c/busses/i2c-aspeed.c | 50 +++++++++++++++++++++++++++------
 1 file changed, 41 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index a7be6f24450b..20028a7a9f67 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -117,6 +117,8 @@
 
 /* 0x18 : I2CD Slave Device Address Register   */
 #define ASPEED_I2CD_DEV_ADDR_MASK			GENMASK(6, 0)
+#define ASPEED_I2CD_DEV_ADDR2_ENABLE			BIT(15)
+#define ASPEED_I2CD_DEV_ADDR3_ENABLE			BIT(23)
 
 enum aspeed_i2c_master_state {
 	ASPEED_I2C_MASTER_INACTIVE,
@@ -139,6 +141,16 @@ enum aspeed_i2c_slave_state {
 	ASPEED_I2C_SLAVE_STOP,
 };
 
+struct aspeed_i2c_model {
+	u32 (*get_clk_reg_val)(struct device *dev, u32 divisor);
+
+	/*
+	 * Some models support multiple device addresses -- we only support
+	 * using one, but we need to disable the others if they're present.
+	 */
+	unsigned int num_device_addrs;
+};
+
 struct aspeed_i2c_bus {
 	struct i2c_adapter		adap;
 	struct device			*dev;
@@ -147,8 +159,7 @@ struct aspeed_i2c_bus {
 	/* Synchronizes I/O mem access to base. */
 	spinlock_t			lock;
 	struct completion		cmd_complete;
-	u32				(*get_clk_reg_val)(struct device *dev,
-							   u32 divisor);
+	const struct aspeed_i2c_model	*model;
 	unsigned long			parent_clk_frequency;
 	u32				bus_frequency;
 	/* Transaction state. */
@@ -726,6 +737,13 @@ static void __aspeed_i2c_reg_slave(struct aspeed_i2c_bus *bus, u16 slave_addr)
 	addr_reg_val = readl(bus->base + ASPEED_I2C_DEV_ADDR_REG);
 	addr_reg_val &= ~ASPEED_I2CD_DEV_ADDR_MASK;
 	addr_reg_val |= slave_addr & ASPEED_I2CD_DEV_ADDR_MASK;
+
+	/* Disable additional addresses on hardware that has them. */
+	if (bus->model->num_device_addrs > 1)
+		addr_reg_val &= ~ASPEED_I2CD_DEV_ADDR2_ENABLE;
+	if (bus->model->num_device_addrs > 2)
+		addr_reg_val &= ~ASPEED_I2CD_DEV_ADDR3_ENABLE;
+
 	writel(addr_reg_val, bus->base + ASPEED_I2C_DEV_ADDR_REG);
 
 	/* Turn on slave mode. */
@@ -863,6 +881,11 @@ static u32 aspeed_i2c_24xx_get_clk_reg_val(struct device *dev, u32 divisor)
 	return aspeed_i2c_get_clk_reg_val(dev, GENMASK(2, 0), divisor);
 }
 
+static const struct aspeed_i2c_model aspeed_i2c_24xx_bus = {
+	.get_clk_reg_val = aspeed_i2c_24xx_get_clk_reg_val,
+	.num_device_addrs = 1,
+};
+
 static u32 aspeed_i2c_25xx_get_clk_reg_val(struct device *dev, u32 divisor)
 {
 	/*
@@ -872,6 +895,16 @@ static u32 aspeed_i2c_25xx_get_clk_reg_val(struct device *dev, u32 divisor)
 	return aspeed_i2c_get_clk_reg_val(dev, GENMASK(3, 0), divisor);
 }
 
+static const struct aspeed_i2c_model aspeed_i2c_25xx_bus = {
+	.get_clk_reg_val = aspeed_i2c_25xx_get_clk_reg_val,
+	.num_device_addrs = 2,
+};
+
+static const struct aspeed_i2c_model aspeed_i2c_26xx_bus = {
+	.get_clk_reg_val = aspeed_i2c_25xx_get_clk_reg_val,
+	.num_device_addrs = 3,
+};
+
 /* precondition: bus.lock has been acquired. */
 static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus)
 {
@@ -882,7 +915,7 @@ static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus)
 	clk_reg_val &= (ASPEED_I2CD_TIME_TBUF_MASK |
 			ASPEED_I2CD_TIME_THDSTA_MASK |
 			ASPEED_I2CD_TIME_TACST_MASK);
-	clk_reg_val |= bus->get_clk_reg_val(bus->dev, divisor);
+	clk_reg_val |= bus->model->get_clk_reg_val(bus->dev, divisor);
 	writel(clk_reg_val, bus->base + ASPEED_I2C_AC_TIMING_REG1);
 	writel(ASPEED_NO_TIMEOUT_CTRL, bus->base + ASPEED_I2C_AC_TIMING_REG2);
 
@@ -946,15 +979,15 @@ static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus)
 static const struct of_device_id aspeed_i2c_bus_of_table[] = {
 	{
 		.compatible = "aspeed,ast2400-i2c-bus",
-		.data = aspeed_i2c_24xx_get_clk_reg_val,
+		.data = &aspeed_i2c_24xx_bus,
 	},
 	{
 		.compatible = "aspeed,ast2500-i2c-bus",
-		.data = aspeed_i2c_25xx_get_clk_reg_val,
+		.data = &aspeed_i2c_25xx_bus,
 	},
 	{
 		.compatible = "aspeed,ast2600-i2c-bus",
-		.data = aspeed_i2c_25xx_get_clk_reg_val,
+		.data = &aspeed_i2c_26xx_bus,
 	},
 	{ },
 };
@@ -1002,10 +1035,9 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
 
 	match = of_match_node(aspeed_i2c_bus_of_table, pdev->dev.of_node);
 	if (!match)
-		bus->get_clk_reg_val = aspeed_i2c_24xx_get_clk_reg_val;
+		bus->model = &aspeed_i2c_24xx_bus;
 	else
-		bus->get_clk_reg_val = (u32 (*)(struct device *, u32))
-				match->data;
+		bus->model = match->data;
 
 	/* Initialize the I2C adapter */
 	spin_lock_init(&bus->lock);
-- 
2.28.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

* Re: [PATCH] i2c: aspeed: disable additional device addresses on ast2[56]xx
  2020-09-15 18:45 [PATCH] i2c: aspeed: disable additional device addresses on ast2[56]xx Zev Weiss
@ 2020-12-18 21:39 ` Zev Weiss
  2020-12-22  4:08   ` Joel Stanley
  0 siblings, 1 reply; 3+ messages in thread
From: Zev Weiss @ 2020-12-18 21:39 UTC (permalink / raw)
  To: Brendan Higgins, Benjamin Herrenschmidt, Joel Stanley,
	Andrew Jeffery, linux-i2c, openbmc, linux-arm-kernel,
	linux-aspeed, linux-kernel

On Tue, Sep 15, 2020 at 01:45:25PM CDT, Zev Weiss wrote:
>The ast25xx and ast26xx have, respectively, two and three configurable
>slave device addresses to the ast24xx's one.  We only support using
>one at a time, but the others may come up in an indeterminate state
>depending on hardware/bootloader behavior, so we need to make sure we
>disable them so as to avoid ending up with phantom devices on the bus.
>
>Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>---
> drivers/i2c/busses/i2c-aspeed.c | 50 +++++++++++++++++++++++++++------
> 1 file changed, 41 insertions(+), 9 deletions(-)
>
> <snip>

Ping...any thoughts on this patch?


Thanks,
Zev


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

* Re: [PATCH] i2c: aspeed: disable additional device addresses on ast2[56]xx
  2020-12-18 21:39 ` Zev Weiss
@ 2020-12-22  4:08   ` Joel Stanley
  0 siblings, 0 replies; 3+ messages in thread
From: Joel Stanley @ 2020-12-22  4:08 UTC (permalink / raw)
  To: Zev Weiss
  Cc: linux-aspeed, Andrew Jeffery, Benjamin Herrenschmidt,
	OpenBMC Maillist, Brendan Higgins, Linux Kernel Mailing List,
	linux-i2c, Linux ARM

On Fri, 18 Dec 2020 at 21:40, Zev Weiss <zev@bewilderbeest.net> wrote:
>
> On Tue, Sep 15, 2020 at 01:45:25PM CDT, Zev Weiss wrote:
> >The ast25xx and ast26xx have, respectively, two and three configurable
> >slave device addresses to the ast24xx's one.  We only support using
> >one at a time, but the others may come up in an indeterminate state
> >depending on hardware/bootloader behavior, so we need to make sure we
> >disable them so as to avoid ending up with phantom devices on the bus.
> >
> >Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> >---
> > drivers/i2c/busses/i2c-aspeed.c | 50 +++++++++++++++++++++++++++------
> > 1 file changed, 41 insertions(+), 9 deletions(-)
> >
> > <snip>
>
> Ping...any thoughts on this patch?

Apologies for the delay, this one slipped through the cracks.

The rework is fine, and lends itself to supporting the other addresses
in the future. However, a simpler fix would be to construct reg 0x18
from zero, so only the functions that are supported are enabled.

static void __aspeed_i2c_reg_slave(struct aspeed_i2c_bus *bus, u16 slave_addr)
{
    u32 addr_reg_val, func_ctrl_reg_val;

    /* Set slave addr. */
    addr_reg_val = slave_addr & ASPEED_I2CD_DEV_ADDR_MASK;
    writel(addr_reg_val, bus->base + ASPEED_I2C_DEV_ADDR_REG);

    /* 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);
}

You could go further and ensure slave mode is always disabled unless
requested by clearing 0x18 when 0x00 is cleared at aspeed_i2c_init.

Cheers,

Joel

>
>
> Thanks,
> Zev
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

end of thread, other threads:[~2020-12-22  4:09 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-15 18:45 [PATCH] i2c: aspeed: disable additional device addresses on ast2[56]xx Zev Weiss
2020-12-18 21:39 ` Zev Weiss
2020-12-22  4:08   ` Joel Stanley

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