Message ID | 20230519163902.4170-1-quic_jhugo@quicinc.com |
---|---|
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp1373177vqo; Fri, 19 May 2023 09:47:44 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ51uh+CDhN+K3FzJBDpT1y8nTS+QciSOkkw1SwR4GXU01RCd8FI0UwEstck+NjiiM+9HeD9 X-Received: by 2002:a05:6a20:7484:b0:100:99a:7f71 with SMTP id p4-20020a056a20748400b00100099a7f71mr3309875pzd.2.1684514863727; Fri, 19 May 2023 09:47:43 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1684514863; cv=none; d=google.com; s=arc-20160816; b=BfBrZxVwwzPFCgCfIXv17peIfsVLwi/e64HNvT2jKVTxpiBbbivd0P3XBxmLbB8e7T M21TFybtncVXpfDzkWppdedKrn33l/vTkqi2c9+6R38vGHteNQlgCqvlHDZde2e5yc7/ QLYJ7JoRrWYCrm7a5D/hVE4tAh2LO725kPfRtkn6nI/mK7axZIeE5+bUY5Tj3yIXZzqg jexk9n4itWPHfbD6qJcxZh1vK2ES7B8QGKneIqzDva2qfdWoeltWX9EFdtJdiDgsC4b/ Bl4cnH2bN0MluwMhtPtXK8E6v8WUuqnlCf3QCz1uprin0WeoeBXO1q5M5Vx/pmKLhKw+ AqdQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=kgDDoBNwxvR277BxCyv7koD/uI+FCD5MDKi5umWHI6A=; b=cKGz78zOCAfDDrDI6P1Vfh0dwX4o0eYClpfaNHbjMoePpP9qjMVaZ37c+ksKvGBS/i kj2c/bkBaxj4sLQDZ+A/Z8tXAiAuNsC1h8w4S7zm6YcvOxfdZfnwhtJzUWZGJBRiqrtN +Wvc9ilqNE/xquHP054c1J2iX73EDWOF/Ad21yYNInoY/5NqOK/E0iNmG+rgcTFZwdp8 a7S/lpnOXBiedga5OWC5dWJSss9HmnPnWiqW4MV4jC8nVlFmn701D52R5fWLqWmF7l9f bXb8Nr+3Tupx403qHAE8AFAvRHR5WQ9IKLYGyLB+jhtOqvSlxtz+p/xlllSI9GyRXdK/ zfPA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@quicinc.com header.s=qcppdkim1 header.b=V1TRaFij; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=quicinc.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id y81-20020a626454000000b00647e5f1d563si2738518pfb.387.2023.05.19.09.47.28; Fri, 19 May 2023 09:47:43 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@quicinc.com header.s=qcppdkim1 header.b=V1TRaFij; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=quicinc.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231294AbjESQkJ (ORCPT <rfc822;wlfightup@gmail.com> + 99 others); Fri, 19 May 2023 12:40:09 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60296 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231400AbjESQjx (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 19 May 2023 12:39:53 -0400 Received: from mx0a-0031df01.pphosted.com (mx0a-0031df01.pphosted.com [205.220.168.131]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 70B21D1 for <linux-kernel@vger.kernel.org>; Fri, 19 May 2023 09:39:48 -0700 (PDT) Received: from pps.filterd (m0279863.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 34JDR7Qo024834; Fri, 19 May 2023 16:39:22 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; h=from : to : cc : subject : date : message-id : mime-version : content-transfer-encoding : content-type; s=qcppdkim1; bh=kgDDoBNwxvR277BxCyv7koD/uI+FCD5MDKi5umWHI6A=; b=V1TRaFijUqb9pIF08MD+X/VzNEF2oT9EwZ5DQn5rCzgO+V0ilO7nyjLut6Do6qw+/4+H fYBtaqKVSxs7I3otNk2DAp3y2pbH9Ur8qBznQON58s3i15pHE7HPIwVnIMNNRHEQG4Ui 1gsDYQJYK7jk7YNPAIgOUKiYnmPcIKR1e2IcM/InkP1SKbX5lOWNCGo/JTVKUhM58i9m 8Lfq+Fd8UmK91kQ3jOSStCMJKvXRfq3bX0xOGLzpJ1v6LshmnduopCchRQtysOuL7Yq7 VLfmk9/gI0sbQL8iiykKyWaeOiQMMxxYfob+NmJQC2wLX10SjBEJghFT1KfksXa2dnWX 9A== Received: from nalasppmta01.qualcomm.com (Global_NAT1.qualcomm.com [129.46.96.20]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3qnwk4j0dc-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 19 May 2023 16:39:21 +0000 Received: from nalasex01a.na.qualcomm.com (nalasex01a.na.qualcomm.com [10.47.209.196]) by NALASPPMTA01.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 34JGdL0m001824 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 19 May 2023 16:39:21 GMT Received: from jhugo-lnx.qualcomm.com (10.80.80.8) by nalasex01a.na.qualcomm.com (10.47.209.196) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.986.42; Fri, 19 May 2023 09:39:20 -0700 From: Jeffrey Hugo <quic_jhugo@quicinc.com> To: <mani@kernel.org> CC: <linux-kernel@vger.kernel.org>, <dri-devel@lists.freedesktop.org>, <mhi@lists.linux.dev>, Jeffrey Hugo <quic_jhugo@quicinc.com> Subject: [PATCH v2 0/2] Add MHI quirk for QAIC Date: Fri, 19 May 2023 10:39:00 -0600 Message-ID: <20230519163902.4170-1-quic_jhugo@quicinc.com> X-Mailer: git-send-email 2.40.1 MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Type: text/plain X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01a.na.qualcomm.com (10.52.223.231) To nalasex01a.na.qualcomm.com (10.47.209.196) X-QCInternal: smtphost X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=5800 signatures=585085 X-Proofpoint-GUID: Gy5WM1UCc3CbqIwU-6Q00Cn-2tX_UmbJ X-Proofpoint-ORIG-GUID: Gy5WM1UCc3CbqIwU-6Q00Cn-2tX_UmbJ X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.573,FMLib:17.11.170.22 definitions=2023-05-19_11,2023-05-17_02,2023-02-09_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 clxscore=1015 impostorscore=0 mlxscore=0 phishscore=0 priorityscore=1501 malwarescore=0 lowpriorityscore=0 adultscore=0 mlxlogscore=732 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2304280000 definitions=main-2305190142 X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE, SPF_PASS,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1766341857963750120?= X-GMAIL-MSGID: =?utf-8?q?1766341857963750120?= |
Series |
Add MHI quirk for QAIC
|
|
Message
Jeffrey Hugo
May 19, 2023, 4:39 p.m. UTC
With the QAIC driver in -next, I'd like to suggest some MHI changes that specific to AIC100 devices, but perhaps provide a framework for other device oddities. AIC100 devices technically violate the MHI spec in two ways. Sadly, these issues comes from the device hardware, so host SW needs to work around them. Thie first issue, presented in this series, has to do with the SOC_HW_VERSION register. This register is suposed to be initialized by the hardware prior to the MHI being accessable by the host to contain a version string for the SoC of the device. This could be used by the host MHI controller software to identify and handle version to version changes. The AIC100 hardware does not initialize this register, and thus it contains garbage. This would not be much of a problem normally - the QAIC driver would just never use it. However the MHI stack uses this register as part of the init sequence and if the controller reports that the register is inaccessable then the init sequence fails. On some AIC100 cards, the garbage value ends up being 0xFFFFFFFF which is PCIe spec defined to be a special value indicating the access failed. The MHI controller cannot tell if that value is a PCIe link issue, or just garbage. QAIC needs a way to tell MHI not to use this register. Other buses have a quirk mechanism - a way to describe oddities in a particular implementation that have some kind of workaround. Since this seems to be the first need for such a thing in MHI, introduce a quirk framework. The second issue AIC100 has involves the PK Hash registers. A solution for this is expected to be proposed in the near future and is anticipated to make use of the quirk framework proposed here. With PK Hash, there are two oddities to handle. AIC100 does not initialize these registers until the SBL is running, which is later than the spec indicates, and in practice is after MHI reads/caches them. Also, AIC100 does not have enough registers defined to fully report the 5 PK Hash slots, so a custom reporting format is defined by the device. v2: -Fix build error -Fix typo in commit text Jeffrey Hugo (2): bus: mhi: host: Add quirk framework and initial quirk accel/qaic: Add MHI_QUIRK_SOC_HW_VERSION_UNRELIABLE drivers/accel/qaic/mhi_controller.c | 1 + drivers/bus/mhi/host/init.c | 13 +++++++++---- include/linux/mhi.h | 18 ++++++++++++++++++ 3 files changed, 28 insertions(+), 4 deletions(-)
Comments
On Fri, May 19, 2023 at 10:39:00AM -0600, Jeffrey Hugo wrote: > With the QAIC driver in -next, I'd like to suggest some MHI changes that > specific to AIC100 devices, but perhaps provide a framework for other > device oddities. > > AIC100 devices technically violate the MHI spec in two ways. Sadly, these > issues comes from the device hardware, so host SW needs to work around > them. > > Thie first issue, presented in this series, has to do with the > SOC_HW_VERSION register. This register is suposed to be initialized by the > hardware prior to the MHI being accessable by the host to contain a > version string for the SoC of the device. This could be used by the host > MHI controller software to identify and handle version to version changes. > The AIC100 hardware does not initialize this register, and thus it > contains garbage. > > This would not be much of a problem normally - the QAIC driver would just > never use it. However the MHI stack uses this register as part of the init > sequence and if the controller reports that the register is inaccessable > then the init sequence fails. On some AIC100 cards, the garbage value > ends up being 0xFFFFFFFF which is PCIe spec defined to be a special value > indicating the access failed. The MHI controller cannot tell if that > value is a PCIe link issue, or just garbage. > > QAIC needs a way to tell MHI not to use this register. Other buses have a > quirk mechanism - a way to describe oddities in a particular > implementation that have some kind of workaround. Since this seems to be > the first need for such a thing in MHI, introduce a quirk framework. > > The second issue AIC100 has involves the PK Hash registers. A solution for > this is expected to be proposed in the near future and is anticipated to > make use of the quirk framework proposed here. With PK Hash, there are two > oddities to handle. AIC100 does not initialize these registers until the > SBL is running, which is later than the spec indicates, and in practice > is after MHI reads/caches them. Also, AIC100 does not have enough > registers defined to fully report the 5 PK Hash slots, so a custom > reporting format is defined by the device. > Looking at the two issues you reported above, it looks to me that they can be handled inside the aic100 mhi_controller driver itself. Since the MHI stack exports the read_reg callback to controller drivers, if some registers are not supported by the device, then the callback can provide some fixed dummy data emulating the register until the issue is fixed in the device (if at all). Quirk framework could be useful if the device misbehaves against the protocol itself but for the register issues like this, I think the controller driver can handle itself. What do you think? - Mani > v2: > -Fix build error > -Fix typo in commit text > > Jeffrey Hugo (2): > bus: mhi: host: Add quirk framework and initial quirk > accel/qaic: Add MHI_QUIRK_SOC_HW_VERSION_UNRELIABLE > > drivers/accel/qaic/mhi_controller.c | 1 + > drivers/bus/mhi/host/init.c | 13 +++++++++---- > include/linux/mhi.h | 18 ++++++++++++++++++ > 3 files changed, 28 insertions(+), 4 deletions(-) > > -- > 2.40.1 > >
On 6/8/2023 5:59 AM, Manivannan Sadhasivam wrote: > On Fri, May 19, 2023 at 10:39:00AM -0600, Jeffrey Hugo wrote: >> With the QAIC driver in -next, I'd like to suggest some MHI changes that >> specific to AIC100 devices, but perhaps provide a framework for other >> device oddities. >> >> AIC100 devices technically violate the MHI spec in two ways. Sadly, these >> issues comes from the device hardware, so host SW needs to work around >> them. >> >> Thie first issue, presented in this series, has to do with the >> SOC_HW_VERSION register. This register is suposed to be initialized by the >> hardware prior to the MHI being accessable by the host to contain a >> version string for the SoC of the device. This could be used by the host >> MHI controller software to identify and handle version to version changes. >> The AIC100 hardware does not initialize this register, and thus it >> contains garbage. >> >> This would not be much of a problem normally - the QAIC driver would just >> never use it. However the MHI stack uses this register as part of the init >> sequence and if the controller reports that the register is inaccessable >> then the init sequence fails. On some AIC100 cards, the garbage value >> ends up being 0xFFFFFFFF which is PCIe spec defined to be a special value >> indicating the access failed. The MHI controller cannot tell if that >> value is a PCIe link issue, or just garbage. >> >> QAIC needs a way to tell MHI not to use this register. Other buses have a >> quirk mechanism - a way to describe oddities in a particular >> implementation that have some kind of workaround. Since this seems to be >> the first need for such a thing in MHI, introduce a quirk framework. >> >> The second issue AIC100 has involves the PK Hash registers. A solution for >> this is expected to be proposed in the near future and is anticipated to >> make use of the quirk framework proposed here. With PK Hash, there are two >> oddities to handle. AIC100 does not initialize these registers until the >> SBL is running, which is later than the spec indicates, and in practice >> is after MHI reads/caches them. Also, AIC100 does not have enough >> registers defined to fully report the 5 PK Hash slots, so a custom >> reporting format is defined by the device. >> > > Looking at the two issues you reported above, it looks to me that they can be > handled inside the aic100 mhi_controller driver itself. Since the MHI stack > exports the read_reg callback to controller drivers, if some registers are not > supported by the device, then the callback can provide some fixed dummy data > emulating the register until the issue is fixed in the device (if at all). > > Quirk framework could be useful if the device misbehaves against the protocol > itself but for the register issues like this, I think the controller driver can > handle itself. > > What do you think? I think for the HW_VERSION register, your suggestion is very good, and something I plan to adopt. For the PK Hash registers, I don't think it quite works. HW_VERSION I can hard code to a valid value, or just stub out to 0 since that appears to be only consumed by the MHI Controller, and we don't use it. The PK Hash registers are programmed into the SoC, and can be unique from SoC to SoC. I don't see how the driver can provide valid, but faked information for them. Also, the user consumes this data via sysfs. We'd like to give the data to the user, and we can't fake it. Also the data is dynamic. Lets start with the dynamic data issue. Right now MHI reads these registers once, and caches the values. I would propose a quirk to change that behavior for AIC100, but does MHI really need to operate in a "read once" mode? Would something actually break if MHI read the registers every time the sysfs node is accessed? Then sysfs would display the latest data, which would be beneficial to AIC100 and should not be a behavior change for other devices which have static data (MHI just displays the same data because it hasn't changed). Do you recall the reason behind making the PK Hash registers read once and cached? > > - Mani > >> v2: >> -Fix build error >> -Fix typo in commit text >> >> Jeffrey Hugo (2): >> bus: mhi: host: Add quirk framework and initial quirk >> accel/qaic: Add MHI_QUIRK_SOC_HW_VERSION_UNRELIABLE >> >> drivers/accel/qaic/mhi_controller.c | 1 + >> drivers/bus/mhi/host/init.c | 13 +++++++++---- >> include/linux/mhi.h | 18 ++++++++++++++++++ >> 3 files changed, 28 insertions(+), 4 deletions(-) >> >> -- >> 2.40.1 >> >> >
On Mon, Jun 26, 2023 at 11:15:56AM -0600, Jeffrey Hugo wrote: > On 6/8/2023 5:59 AM, Manivannan Sadhasivam wrote: > > On Fri, May 19, 2023 at 10:39:00AM -0600, Jeffrey Hugo wrote: > > > With the QAIC driver in -next, I'd like to suggest some MHI changes that > > > specific to AIC100 devices, but perhaps provide a framework for other > > > device oddities. > > > > > > AIC100 devices technically violate the MHI spec in two ways. Sadly, these > > > issues comes from the device hardware, so host SW needs to work around > > > them. > > > > > > Thie first issue, presented in this series, has to do with the > > > SOC_HW_VERSION register. This register is suposed to be initialized by the > > > hardware prior to the MHI being accessable by the host to contain a > > > version string for the SoC of the device. This could be used by the host > > > MHI controller software to identify and handle version to version changes. > > > The AIC100 hardware does not initialize this register, and thus it > > > contains garbage. > > > > > > This would not be much of a problem normally - the QAIC driver would just > > > never use it. However the MHI stack uses this register as part of the init > > > sequence and if the controller reports that the register is inaccessable > > > then the init sequence fails. On some AIC100 cards, the garbage value > > > ends up being 0xFFFFFFFF which is PCIe spec defined to be a special value > > > indicating the access failed. The MHI controller cannot tell if that > > > value is a PCIe link issue, or just garbage. > > > > > > QAIC needs a way to tell MHI not to use this register. Other buses have a > > > quirk mechanism - a way to describe oddities in a particular > > > implementation that have some kind of workaround. Since this seems to be > > > the first need for such a thing in MHI, introduce a quirk framework. > > > > > > The second issue AIC100 has involves the PK Hash registers. A solution for > > > this is expected to be proposed in the near future and is anticipated to > > > make use of the quirk framework proposed here. With PK Hash, there are two > > > oddities to handle. AIC100 does not initialize these registers until the > > > SBL is running, which is later than the spec indicates, and in practice > > > is after MHI reads/caches them. Also, AIC100 does not have enough > > > registers defined to fully report the 5 PK Hash slots, so a custom > > > reporting format is defined by the device. > > > > > > > Looking at the two issues you reported above, it looks to me that they can be > > handled inside the aic100 mhi_controller driver itself. Since the MHI stack > > exports the read_reg callback to controller drivers, if some registers are not > > supported by the device, then the callback can provide some fixed dummy data > > emulating the register until the issue is fixed in the device (if at all). > > > > Quirk framework could be useful if the device misbehaves against the protocol > > itself but for the register issues like this, I think the controller driver can > > handle itself. > > > > What do you think? > > I think for the HW_VERSION register, your suggestion is very good, and > something I plan to adopt. > > For the PK Hash registers, I don't think it quite works. > > HW_VERSION I can hard code to a valid value, or just stub out to 0 since > that appears to be only consumed by the MHI Controller, and we don't use it. > > The PK Hash registers are programmed into the SoC, and can be unique from > SoC to SoC. I don't see how the driver can provide valid, but faked > information for them. Also, the user consumes this data via sysfs. We'd > like to give the data to the user, and we can't fake it. Also the data is > dynamic. > > Lets start with the dynamic data issue. Right now MHI reads these registers > once, and caches the values. I would propose a quirk to change that > behavior for AIC100, but does MHI really need to operate in a "read once" > mode? Would something actually break if MHI read the registers every time > the sysfs node is accessed? Then sysfs would display the latest data, which > would be beneficial to AIC100 and should not be a behavior change for other > devices which have static data (MHI just displays the same data because it > hasn't changed). > > Do you recall the reason behind making the PK Hash registers read once and > cached? > I don't see an issue with reading the PK hash dynamically. I think the intention for caching mostly come from the fact it was a static data. So you can dynamically read it all the time. - Mani > > > > - Mani > > > > > v2: > > > -Fix build error > > > -Fix typo in commit text > > > > > > Jeffrey Hugo (2): > > > bus: mhi: host: Add quirk framework and initial quirk > > > accel/qaic: Add MHI_QUIRK_SOC_HW_VERSION_UNRELIABLE > > > > > > drivers/accel/qaic/mhi_controller.c | 1 + > > > drivers/bus/mhi/host/init.c | 13 +++++++++---- > > > include/linux/mhi.h | 18 ++++++++++++++++++ > > > 3 files changed, 28 insertions(+), 4 deletions(-) > > > > > > -- > > > 2.40.1 > > > > > > > > > >