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:     Jamin Lin <jamin_lin@aspeedtech.com>
CC:     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>,
        "open list:I2C SUBSYSTEM HOST DRIVERS" <linux-i2c@vger.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        "moderated list:ARM/ASPEED MACHINE SUPPORT" 
        <linux-arm-kernel@lists.infradead.org>,
        "moderated list:ARM/ASPEED MACHINE SUPPORT" 
        <linux-aspeed@lists.ozlabs.org>,
        open list <linux-kernel@vger.kernel.org>,
        "moderated list:ARM/ASPEED I2C DRIVER" <openbmc@lists.ozlabs.org>,
        Steven Lee <steven_lee@aspeedtech.com>,
        ChiaWei Wang <chiawei_wang@aspeedtech.com>,
        Troy Lee <troy_lee@aspeedtech.com>,
        Ryan Chen <ryan_chen@aspeedtech.com>
Subject: Re: [PATCH 1/3] i2c: aspeed: avoid new registers definition of
 AST2600
Thread-Topic: [PATCH 1/3] i2c: aspeed: avoid new registers definition of
 AST2600
Thread-Index: AQHXUOIG88H/dxjruU+R5Lxy+695eA==
Date:   Mon, 24 May 2021 21:16:13 +0000
Message-ID: <YKwXnPH0XyYLRtfa@packtop>
References: <20210519080436.18975-1-jamin_lin@aspeedtech.com>
 <20210519080436.18975-2-jamin_lin@aspeedtech.com> <YKVg2Kfbex3DYbNI@packtop>
 <20210524020846.GB2591@aspeedtech.com>
In-Reply-To: <20210524020846.GB2591@aspeedtech.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <DC05F5B0E7C4C249B02B5CE459090DFE@namprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 3cc9909e-21e3-4934-ab87-08d91ef92946
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 May 2021 21:16:13.7213
 (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: LK0XyFvfJHfL4HXHZwJcmGuOwqhIEqjlFE/GNymjC9eX4Uk/8v2yd5eO5F1OvvBbnXDoNgCFS7p9m7BB5GB7kw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR04MB0364
X-Proofpoint-GUID: WnbUeubIyoxiFUOdvot_Te8wxziVxV-f
X-Proofpoint-ORIG-GUID: WnbUeubIyoxiFUOdvot_Te8wxziVxV-f
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.391,18.0.761
 definitions=2021-05-24_09:2021-05-24,2021-05-24 signatures=0
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 clxscore=1015 adultscore=0
 malwarescore=0 priorityscore=1501 impostorscore=0 phishscore=0
 mlxlogscore=999 mlxscore=0 lowpriorityscore=0 suspectscore=0 spamscore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2104190000 definitions=main-2105240126
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Sun, May 23, 2021 at 09:08:47PM CDT, Jamin Lin wrote:
>The 05/19/2021 19:02, Zev Weiss wrote:
>> On Wed, May 19, 2021 at 03:04:27AM CDT, Jamin Lin wrote:
>> >The register definition between AST2600 A2 and A3 is different.
>> >This patch avoid new registers definition of AST2600 to use
>> >this driver. We will submit the path for the new registers
>> >definition of AST2600.
>> >
>> >Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
>> >---
>> > drivers/i2c/busses/i2c-aspeed.c | 22 ++++++++++++++++++++++
>> > 1 file changed, 22 insertions(+)
>> >
>> >diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-a=
speed.c
>> >index 724bf30600d6..007309077d9f 100644
>> >--- a/drivers/i2c/busses/i2c-aspeed.c
>> >+++ b/drivers/i2c/busses/i2c-aspeed.c
>> >@@ -19,14 +19,20 @@
>> > #include <linux/irqchip/chained_irq.h>
>> > #include <linux/irqdomain.h>
>> > #include <linux/kernel.h>
>> >+#include <linux/mfd/syscon.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/regmap.h>
>> > #include <linux/reset.h>
>> > #include <linux/slab.h>
>> >
>> >+/* I2C Global Registers */
>> >+/* 0x0c : I2CG Global Control Register (AST2500)  */
>> >+#define ASPEED_I2CG_GLOBAL_CTRL_REG			0x0c
>> >+
>> > /* I2C Register */
>> > #define ASPEED_I2C_FUN_CTRL_REG				0x00
>> > #define ASPEED_I2C_AC_TIMING_REG1			0x04
>> >@@ -973,6 +979,22 @@ static int aspeed_i2c_probe_bus(struct platform_de=
vice *pdev)
>> > 	struct resource *res;
>> > 	int irq, ret;
>> >
>> >+	if (of_device_is_compatible(pdev->dev.of_node,
>> >+				    "aspeed,ast2600-i2c-bus")) {
>> >+		u32 global_ctrl;
>> >+		struct regmap *gr_regmap;
>> >+
>> >+		gr_regmap =3D syscon_regmap_lookup_by_compatible("aspeed,ast2600-i2c=
-global");
>> >+
>> >+		if (IS_ERR(gr_regmap)) {
>> >+			ret =3D PTR_ERR(gr_regmap);
>> >+		} else {
>> >+			regmap_read(gr_regmap, ASPEED_I2CG_GLOBAL_CTRL_REG, &global_ctrl);
>> >+			if (global_ctrl & BIT(2))
>> >+				return -EIO;
>>
>> A macro definition might be a bit nicer than a raw BIT(2) here I'd
>> think.
>Will modify
>>
>> Also, it seems a bit unfortunate to just bail on the device entirely if
>> we find this bit set (seems like a good way for a bootloader to
>> inadvertently DoS the kernel), though I guess poking global syscon bits
>> in the bus probe function might not be ideal.  Could/should we consider
>> some module-level init code to ensure that bit is cleared?
>>
>>
>We use syscon API to get the global register of i2c not the specific i2c
>bus.
>Can you describe it more detail?

Sure -- I just meant that if for whatever reason the kernel is booting
on a system that's had that syscon bit set to enable the new register
access mode (e.g. by a newer bootloader or something), it seems like
we'd just give up entirely on enabling any i2c busses, when as far as I
know there shouldn't be anything stopping us from resetting the bit back
to be in the state this driver needs it to be in (old register mode) and
then continuing along normally.


Zev