Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Zev Weiss <zweiss@equinix.com>
To:     Andrew Jeffery <andrew@aj.id.au>
CC:     "openipmi-developer@lists.sourceforge.net" 
        <openipmi-developer@lists.sourceforge.net>,
        "openbmc@lists.ozlabs.org" <openbmc@lists.ozlabs.org>,
        "minyard@acm.org" <minyard@acm.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "ryan_chen@aspeedtech.com" <ryan_chen@aspeedtech.com>,
        "tmaimon77@gmail.com" <tmaimon77@gmail.com>,
        "linux-aspeed@lists.ozlabs.org" <linux-aspeed@lists.ozlabs.org>,
        "avifishman70@gmail.com" <avifishman70@gmail.com>,
        "venture@google.com" <venture@google.com>,
        "linus.walleij@linaro.org" <linus.walleij@linaro.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "tali.perry1@gmail.com" <tali.perry1@gmail.com>,
        "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "lee.jones@linaro.org" <lee.jones@linaro.org>,
        "chiawei_wang@aspeedtech.com" <chiawei_wang@aspeedtech.com>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "benjaminfair@google.com" <benjaminfair@google.com>
Subject: Re [PATCH v2 21/21] ipmi: kcs_bmc_aspeed: Optionally apply status
 address
Thread-Topic: Re [PATCH v2 21/21] ipmi: kcs_bmc_aspeed: Optionally apply
 status address
Thread-Index: AQHXJyNeAUueKu6Dl0GGRLjVvWP0kA==
Date:   Thu, 1 Apr 2021 18:18:09 +0000
Message-ID: <YGYOX4HcdKcWOUx1@packtop>
References: <20210319062752.145730-1-andrew@aj.id.au>
 <20210319062752.145730-21-andrew@aj.id.au>
In-Reply-To: <20210319062752.145730-21-andrew@aj.id.au>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <AC3397F50CBE58469B51BB8E9DC2F662@namprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 7608cad5-7260-44a4-1166-08d8f53a815a
X-MS-Exchange-CrossTenant-originalarrivaltime: 01 Apr 2021 18:18:09.8867
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 72adb271-2fc7-4afe-a5ee-9de6a59f6bfb
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: UGeVNydP2XiYIOmax/4ChE+ZeAJjVqku8jbSKXdvZCmnwLJQOZAo5sc4u1Ns6iAWArjOIJuSnUkBxYZpKpfaJg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR04MB5531
X-Proofpoint-ORIG-GUID: 9xFTfW-aWC2nPu82TyuKK2VFTkpxKw2F
X-Proofpoint-GUID: 9xFTfW-aWC2nPu82TyuKK2VFTkpxKw2F
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.369,18.0.761
 definitions=2021-04-01_09:2021-04-01,2021-04-01 signatures=0
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 phishscore=0 bulkscore=0
 lowpriorityscore=0 impostorscore=0 mlxlogscore=999 suspectscore=0
 malwarescore=0 mlxscore=0 adultscore=0 spamscore=0 priorityscore=1501
 clxscore=1015 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2103310000 definitions=main-2104010119
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Mar 19, 2021 at 01:27:52AM CDT, Andrew Jeffery wrote:
>Some Aspeed KCS devices can derive the status register address from the
>address of the data register. As such, the address of the status
>register can be implicit in the configuration if desired. On the other
>hand, sometimes address schemes might be requested that are incompatible
>with the default addressing scheme. Allow these requests where possible
>if the devicetree specifies the status register address.
>
>Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>---
> drivers/char/ipmi/kcs_bmc_aspeed.c | 113 +++++++++++++++++++++--------
> 1 file changed, 81 insertions(+), 32 deletions(-)
>
>diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bm=
c_aspeed.c
>index 7334b1f51dcc..98789b837690 100644
>--- a/drivers/char/ipmi/kcs_bmc_aspeed.c
>+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
>@@ -83,6 +83,8 @@
> #define LPC_STR2             0x040
> #define LPC_STR3             0x044
> #define LPC_HICRB            0x100
>+#define     LPC_HICRB_EN16LADR2      BIT(5)
>+#define     LPC_HICRB_EN16LADR1      BIT(4)
> #define     LPC_HICRB_IBFIE4         BIT(1)
> #define     LPC_HICRB_LPC4E          BIT(0)
> #define LPC_HICRC            0x104
>@@ -96,6 +98,11 @@
> #define LPC_IDR4             0x114
> #define LPC_ODR4             0x118
> #define LPC_STR4             0x11C
>+#define LPC_LSADR12	     0x120
>+#define     LPC_LSADR12_LSADR2_MASK  GENMASK(31, 16)
>+#define     LPC_LSADR12_LSADR2_SHIFT 16
>+#define     LPC_LSADR12_LSADR1_MASK  GENMASK(15, 0)
>+#define     LPC_LSADR12_LSADR1_SHIFT 0
>
> #define OBE_POLL_PERIOD	     (HZ / 2)
>
>@@ -123,7 +130,7 @@ struct aspeed_kcs_bmc {
>
> struct aspeed_kcs_of_ops {
> 	int (*get_channel)(struct platform_device *pdev);
>-	int (*get_io_address)(struct platform_device *pdev);
>+	int (*get_io_address)(struct platform_device *pdev, u32 addrs[2]);
> };
>
> static inline struct aspeed_kcs_bmc *to_aspeed_kcs_bmc(struct kcs_bmc_dev=
ice *kcs_bmc)
>@@ -217,38 +224,64 @@ static void aspeed_kcs_updateb(struct kcs_bmc_device=
 *kcs_bmc, u32 reg, u8 mask,
>  *     C. KCS4
>  *        D / C : CA4h / CA5h
>  */
>-static void aspeed_kcs_set_address(struct kcs_bmc_device *kcs_bmc, u16 ad=
dr)
>+static int aspeed_kcs_set_address(struct kcs_bmc_device *kcs_bmc, u32 add=
rs[2], int nr_addrs)
> {
> 	struct aspeed_kcs_bmc *priv =3D to_aspeed_kcs_bmc(kcs_bmc);
>
>-	switch (kcs_bmc->channel) {
>+	if (WARN_ON(nr_addrs < 1 || nr_addrs > 2))
>+		return -EINVAL;
>+
>+	switch (priv->kcs_bmc.channel) {
> 	case 1:
>-		regmap_update_bits(priv->map, LPC_HICR4,
>-				LPC_HICR4_LADR12AS, 0);
>-		regmap_write(priv->map, LPC_LADR12H, addr >> 8);
>-		regmap_write(priv->map, LPC_LADR12L, addr & 0xFF);
>+		regmap_update_bits(priv->map, LPC_HICR4, LPC_HICR4_LADR12AS, 0);
>+		regmap_write(priv->map, LPC_LADR12H, addrs[0] >> 8);
>+		regmap_write(priv->map, LPC_LADR12L, addrs[0] & 0xFF);
>+		if (nr_addrs =3D=3D 2) {
>+			regmap_update_bits(priv->map, LPC_LSADR12, LPC_LSADR12_LSADR1_MASK,
>+					   addrs[1] << LPC_LSADR12_LSADR1_SHIFT);
>+
>+			regmap_update_bits(priv->map, LPC_HICRB, LPC_HICRB_EN16LADR1,
>+					   LPC_HICRB_EN16LADR1);
>+		}
> 		break;
>
> 	case 2:
>-		regmap_update_bits(priv->map, LPC_HICR4,
>-				LPC_HICR4_LADR12AS, LPC_HICR4_LADR12AS);
>-		regmap_write(priv->map, LPC_LADR12H, addr >> 8);
>-		regmap_write(priv->map, LPC_LADR12L, addr & 0xFF);
>+		regmap_update_bits(priv->map, LPC_HICR4, LPC_HICR4_LADR12AS, LPC_HICR4_=
LADR12AS);
>+		regmap_write(priv->map, LPC_LADR12H, addrs[0] >> 8);
>+		regmap_write(priv->map, LPC_LADR12L, addrs[0] & 0xFF);
>+		if (nr_addrs =3D=3D 2) {
>+			regmap_update_bits(priv->map, LPC_LSADR12, LPC_LSADR12_LSADR2_MASK,
>+					   addrs[1] << LPC_LSADR12_LSADR2_SHIFT);
>+
>+			regmap_update_bits(priv->map, LPC_HICRB, LPC_HICRB_EN16LADR2,
>+					   LPC_HICRB_EN16LADR2);
>+		}
> 		break;
>
> 	case 3:
>-		regmap_write(priv->map, LPC_LADR3H, addr >> 8);
>-		regmap_write(priv->map, LPC_LADR3L, addr & 0xFF);
>+		if (nr_addrs =3D=3D 2) {
>+			dev_err(priv->kcs_bmc.dev,
>+				"Channel 3 only supports inferred status IO address\n");
>+			return -EINVAL;
>+		}
>+
>+		regmap_write(priv->map, LPC_LADR3H, addrs[0] >> 8);
>+		regmap_write(priv->map, LPC_LADR3L, addrs[0] & 0xFF);
> 		break;
>
> 	case 4:
>-		regmap_write(priv->map, LPC_LADR4, ((addr + 1) << 16) |
>-			addr);
>+		if (nr_addrs =3D=3D 1)
>+			regmap_write(priv->map, LPC_LADR4, ((addrs[0] + 1) << 16) | addrs[0]);
>+		else
>+			regmap_write(priv->map, LPC_LADR4, (addrs[1] << 16) | addrs[0]);
>+
> 		break;
>
> 	default:
>-		break;
>+		return -EINVAL;
> 	}
>+
>+	return 0;
> }
>
> static inline int aspeed_kcs_map_serirq_type(u32 dt_type)
>@@ -462,18 +495,18 @@ static int aspeed_kcs_of_v1_get_channel(struct platf=
orm_device *pdev)
> 	return channel;
> }
>
>-static int aspeed_kcs_of_v1_get_io_address(struct platform_device *pdev)
>+static int
>+aspeed_kcs_of_v1_get_io_address(struct platform_device *pdev, u32 addrs[2=
])
> {
>-	u32 slave;
> 	int rc;
>
>-	rc =3D of_property_read_u32(pdev->dev.of_node, "kcs_addr", &slave);
>-	if (rc || slave > 0xffff) {
>+	rc =3D of_property_read_u32(pdev->dev.of_node, "kcs_addr", addrs);
>+	if (rc || addrs[0] > 0xffff) {
> 		dev_err(&pdev->dev, "no valid 'kcs_addr' configured\n");
> 		return -EINVAL;
> 	}
>
>-	return slave;
>+	return 1;
> }
>
> static int aspeed_kcs_of_v2_get_channel(struct platform_device *pdev)
>@@ -509,16 +542,27 @@ static int aspeed_kcs_of_v2_get_channel(struct platf=
orm_device *pdev)
> 	return -EINVAL;
> }
>
>-static int aspeed_kcs_of_v2_get_io_address(struct platform_device *pdev)
>+static int
>+aspeed_kcs_of_v2_get_io_address(struct platform_device *pdev, u32 addrs[2=
])
> {
>-	uint32_t slave;
> 	int rc;
>
>-	rc =3D of_property_read_u32(pdev->dev.of_node, "aspeed,lpc-io-reg", &sla=
ve);
>-	if (rc || slave > 0xffff)
>+	rc =3D of_property_read_variable_u32_array(pdev->dev.of_node,
>+						 "aspeed,lpc-io-reg",
>+						 addrs, 1, 2);
>+	if (rc < 0)
>+		return rc;
>+
>+	if (WARN_ON(rc =3D=3D 0))
>+		return -EINVAL;

Is this check necessary?  It looks like
of_property_read_variable_u32_array() should fail in that case given
sz_min=3D=3D1, so this seems like it should be impossible to trigger.

>+
>+	if (addrs[0] > 0xffff)
>+		return -EINVAL;
>+
>+	if (rc =3D=3D 2 && addrs[1] > 0xffff)
> 		return -EINVAL;
>
>-	return slave;
>+	return rc;
> }
>
> static int aspeed_kcs_probe(struct platform_device *pdev)
>@@ -527,9 +571,11 @@ static int aspeed_kcs_probe(struct platform_device *p=
dev)
> 	struct kcs_bmc_device *kcs_bmc;
> 	struct aspeed_kcs_bmc *priv;
> 	struct device_node *np;
>-	int rc, channel, addr;
> 	bool have_upstream_irq;
> 	u32 upstream_irq[2];
>+	int rc, channel;
>+	int nr_addrs;
>+	u32 addrs[2];
>
> 	np =3D pdev->dev.of_node->parent;
> 	if (!of_device_is_compatible(np, "aspeed,ast2400-lpc-v2") &&
>@@ -547,9 +593,9 @@ static int aspeed_kcs_probe(struct platform_device *pd=
ev)
> 	if (channel < 0)
> 		return channel;
>
>-	addr =3D ops->get_io_address(pdev);
>-	if (addr < 0)
>-		return addr;
>+	nr_addrs =3D ops->get_io_address(pdev, addrs);
>+	if (nr_addrs < 0)
>+		return nr_addrs;
>
> 	np =3D pdev->dev.of_node;
> 	rc =3D of_property_read_u32_array(np, "aspeed,lpc-interrupts", upstream_=
irq, 2);
>@@ -578,7 +624,9 @@ static int aspeed_kcs_probe(struct platform_device *pd=
ev)
> 	priv->obe.remove =3D false;
> 	timer_setup(&priv->obe.timer, aspeed_kcs_check_obe, 0);
>
>-	aspeed_kcs_set_address(kcs_bmc, addr);
>+	rc =3D aspeed_kcs_set_address(kcs_bmc, addrs, nr_addrs);
>+	if (rc)
>+		return rc;
>
> 	/* Host to BMC IRQ */
> 	rc =3D aspeed_kcs_config_downstream_irq(kcs_bmc, pdev);
>@@ -600,7 +648,8 @@ static int aspeed_kcs_probe(struct platform_device *pd=
ev)
> 	if (rc < 0)
> 		return rc;
>
>-	dev_info(&pdev->dev, "Initialised channel %d at 0x%x\n", kcs_bmc->channe=
l, addr);
>+	dev_info(&pdev->dev, "Initialised channel %d at 0x%x\n",
>+			kcs_bmc->channel, addrs[0]);
>
> 	return 0;
> }
>--=20
>2.27.0
>=