Received-SPF: pass (google.com: domain of linux-kernel+bounces-72867-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
From: Minda Chen <minda.chen@starfivetech.com>
To: Conor Dooley <conor@kernel.org>, =?iso-8859-2?Q?Krzysztof_Wilczy=F1ski?=
	<kw@linux.com>, Rob Herring <robh+dt@kernel.org>, Bjorn Helgaas
	<bhelgaas@google.com>, Lorenzo Pieralisi <lpieralisi@kernel.org>, Thomas
 Gleixner <tglx@linutronix.de>, Daire McNamara <daire.mcnamara@microchip.com>,
	Emil Renner Berthing <emil.renner.berthing@canonical.com>, Krzysztof
 Kozlowski <krzysztof.kozlowski+dt@linaro.org>
CC: "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-riscv@lists.infradead.org" <linux-riscv@lists.infradead.org>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>, Paul Walmsley
	<paul.walmsley@sifive.com>, Palmer Dabbelt <palmer@dabbelt.com>, Albert Ou
	<aou@eecs.berkeley.edu>, Philipp Zabel <p.zabel@pengutronix.de>, Mason Huo
	<mason.huo@starfivetech.com>, Leyfoon Tan <leyfoon.tan@starfivetech.com>,
	Kevin Xie <kevin.xie@starfivetech.com>
Subject: Re: [PATCH v15 15/23] PCI: microchip: Add event irqchip field to host
 port and add PLDA irqchip
Thread-Topic: [PATCH v15 15/23] PCI: microchip: Add event irqchip field to
 host port and add PLDA irqchip
Thread-Index: AQHaYlPYD6wUc0NM7EGAypJO2so9u7ETE0sg
Date: Tue, 20 Feb 2024 11:00:12 +0000
Message-ID:
 <SHXPR01MB0863FCE82CA2155E52A3EB6FE650A@SHXPR01MB0863.CHNPR01.prod.partner.outlook.cn>
References: <20240218101831.113469-1-minda.chen@starfivetech.com>
 <20240218101831.113469-2-minda.chen@starfivetech.com>
In-Reply-To: <20240218101831.113469-2-minda.chen@starfivetech.com>
Accept-Language: en-US
Content-Language: zh-CN
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0:
 =?iso-8859-2?Q?prn2hXtv+g9Xl/xzEVHAsb3w+XpeqRSklrpFg7+oklSUEjvOxf9tqglmp+?=
 =?iso-8859-2?Q?+UjcWaIgJvFdmi3pAkV1mIQ/fALo+zAEZIMa2efbTZ5z8rjZR/TA3MgglA?=
 =?iso-8859-2?Q?xFQE+Mt6S302oWwmrp/1QjNANS+UmvXFgs4PqJH7E6r80VejrimBTiCIto?=
 =?iso-8859-2?Q?6Hd8VEL9Pxv1/TVKrER3CUpaNTruEbhIH98/vx8zPhfLe7AA+1YgBuR+j7?=
 =?iso-8859-2?Q?oFx+nx41ncjPeajlg6ofDZTRuZmGt5PcQB7Gb922fjbZYSHn0iOkCNgVCW?=
 =?iso-8859-2?Q?OwBVBUO+1gHZaHrN3jcf0E6qiwf0skXFpwSZ1Xm5uegwQ/dMjszwA3l7sP?=
 =?iso-8859-2?Q?6BhiNKBzgsLaUAXOSzxZWjMnfokR7S8uEk2+cnsk6gq+nWdoMQfcB+bKTL?=
 =?iso-8859-2?Q?mI+oQGqFwx9TWeN/u4EddJnONXkW66VXb1Ad8r6JDmwAr40G6pxXab6fg6?=
 =?iso-8859-2?Q?WbB1hY5KTZGnVcFWt41+RkVwG1jYqAVoUrr+2aCy3P+T1r/ZZz+701WZ0d?=
 =?iso-8859-2?Q?ADSNZX5+xAtoxP7b6cmqKqyh8ymzcq4MQY8O/8bJ8qHNzIkEvB/8VcwusD?=
 =?iso-8859-2?Q?AuEKeUtdrMp7lOw8xJ05KQrfmoaT1mRRQ3PKnBUmG2CxvBpLpTCSKoUo1p?=
 =?iso-8859-2?Q?7VAtUUuAFESf5ZmdxXVfs31HsdvNotAxWeMONFeUjYz31N/VaQwVQT8nWb?=
 =?iso-8859-2?Q?7E1YZZbn3lpG8VQaWKvm2eXjAEFlbAaaaBXE7mbUmKYZSrLy3BApfTjmkw?=
 =?iso-8859-2?Q?72b1sUE25ZWk3er3dE1/twYrVMcPEJ2NTbcf8VAk7Y+p0AtePt1ipbYmWB?=
 =?iso-8859-2?Q?UAikWI1PQMboCXv+o84QMrZb83dhvRX3TAjpWiq5R/3KNpuh3u+5UaySqn?=
 =?iso-8859-2?Q?gnoMMI/e6M5FTcKu4LFEa7ruP6PCkQvC4k+iL84aKeYsaKOPuKfRcREkN2?=
 =?iso-8859-2?Q?R0z880GdW4BRhF0rvpdfBBQpey9hyXGfiXnyIkJNDMt5YKjYr9cHIxS6Wd?=
 =?iso-8859-2?Q?gVFYZ6BaNYRLPRorjC85F54MsR/qkSRDZNRt3VJE3umViHQZiwAHSOhxB7?=
 =?iso-8859-2?Q?qU8YGMbrCrNae6Rlo4qo3ETqLY9VNmFHyBnEplWAGjMtpbfhyq+gmwG/s6?=
 =?iso-8859-2?Q?k4fqANeWMr3KzoT7jYRTGeIdolt77FGCAN2UE7Aj7DRgfXnsFlWRfARGst?=
 =?iso-8859-2?Q?IJgKzpd6/0Rb+Y7wZzrQKUnLkMi6iFU7UMoO4R76a2MvDHVQT7EMzY/xJ/?=
 =?iso-8859-2?Q?XSXA4BVjQ3vUTV08kQTtXU4fSy461IUgoghNdhTEDNCbBKfU8sTeZu6GY6?=
 =?iso-8859-2?Q?VylphJ65yjWE/Wu88zfRgBiQRjA7VTImwAZATbjjsxDdfUKUqTy+nHmVP8?=
 =?iso-8859-2?Q?8mQfwsDcEqkIuGIWW3+JuX3JLSJBqpFVj2h13MgvhCKygKRWQYdgjlFKHS?=
 =?iso-8859-2?Q?eOejIGPZRdiFPOFukUBL2dOfwzrp1ktlk2WYONCyFke4o0Zmdilii+gR4C?=
 =?iso-8859-2?Q?x178YSZL+baXDLOItr97PBeck2uOu/oWGVd2iqTecc2fQ7ieMh3eQCgKah?=
 =?iso-8859-2?Q?jnhxgGY05mOcvJmGrglDXfnEtljM4PHa+el2lMJMgOHnVmkLRSB1Mv3JmP?=
 =?iso-8859-2?Q?9+yB57lDnH7aVUk7YQiCPTtV1CxiJox/4m?=
Content-Type: text/plain; charset="iso-8859-2"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-OriginatorOrg: starfivetech.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SHXPR01MB0863.CHNPR01.prod.partner.outlook.cn
X-MS-Exchange-CrossTenant-Network-Message-Id: d8bc096b-5f1a-4ecd-4bb2-08dc32031cd9
X-MS-Exchange-CrossTenant-originalarrivaltime: 20 Feb 2024 11:00:12.9933
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 06fe3fa3-1221-43d3-861b-5a4ee687a85c
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: WF36AW0k+7d6C/HWaHG1FTe5gZ3xs7CJrcyZAnm3lVfD4GT676aBNLtd/LLk4qYraGg6XSb63NlnJoD+KyQgf7O6X8AyHF7A4Tmp/p2LoAE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SHXPR01MB0783


>=20
> As PLDA dts binding doc(Documentation/devicetree/bindings/pci/
> plda,xpressrich3-axi-common.yaml) showed, PLDA PCIe contains an interrupt
> controller.
>=20
> Microchip PolarFire PCIE event IRQs includes PLDA interrupts and Polarfir=
e their
> own interrupts. The interrupt irqchip ops includes ack/mask/unmask interr=
upt
> ops, which will write correct registers.
> Microchip Polarfire PCIe additional interrupts require to write Polarfire=
 SoC
> self-defined registers. So Microchip PCIe event irqchip ops can not be re=
-used.
>=20
> To support PLDA its own event IRQ process, implements PLDA irqchip ops an=
d
> add event irqchip field to struct pcie_plda_rp.
>=20
> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  .../pci/controller/plda/pcie-microchip-host.c | 66 ++++++++++++++++++-
>  drivers/pci/controller/plda/pcie-plda.h       |  3 +
>  2 files changed, 68 insertions(+), 1 deletion(-)
>=20
> diff --git a/drivers/pci/controller/plda/pcie-microchip-host.c
> b/drivers/pci/controller/plda/pcie-microchip-host.c
> index b3df373a2141..beaf5c27da84 100644
> --- a/drivers/pci/controller/plda/pcie-microchip-host.c
> +++ b/drivers/pci/controller/plda/pcie-microchip-host.c
> @@ -770,6 +770,64 @@ static struct irq_chip mc_event_irq_chip =3D {
>  	.irq_unmask =3D mc_unmask_event_irq,
>  };
>=20
Hi Thomas
  I think this patch code it is easy to review. If you busy, Could you let =
other
IRQ maintainer review? Thanks.

Hi Lorenzo, Bjorn and Krzysztof
  Now the code is pending several weeks. Maybe this patch is blocking.
Actually I write irqchip ops(ack/mask/unmask) like microchip irqchip ops.=20
They looks very simple that write PLDA mask or status register. They all ca=
ll the=20
same function plda_hwirq_to_mask(). Now you can see this function below.=20
Except INTx interrupt, one irqnum mapping to one register bit. The PLDA reg=
ister
graph can be seen 14th patch, which can show all the PLDA interrupts and ea=
sy to
 get PLDA codes logic.

  Now the 6.9-next will be closed less than 20 days. I still hope the refac=
toring patches
(patch 1 - 16) can be accepted to 6.9. (Starfive and PLDA patches have to b=
e delayed=20
to 6.10 or later). I will try my best to achieve it because this series pat=
ches reviewed lasts=20
a long period and Conor have reviewed all the refactoring patches.

I have no experience in refactoring code before this series patches. I try =
my best to do this.
Maybe I did something wrong in this. Please forgive me.

> +static u32 plda_hwirq_to_mask(int hwirq) {
> +	u32 mask;
> +
> +	/* hwirq 23 - 0 are the same with register */
> +	if (hwirq < EVENT_PM_MSI_INT_INTX)
> +		mask =3D BIT(hwirq);
> +	else if (hwirq =3D=3D EVENT_PM_MSI_INT_INTX)
> +		mask =3D PM_MSI_INT_INTX_MASK;
> +	else
> +		mask =3D BIT(hwirq + PCI_NUM_INTX - 1);
> +
> +	return mask;
> +}
> +
> +static void plda_ack_event_irq(struct irq_data *data) {
> +	struct plda_pcie_rp *port =3D irq_data_get_irq_chip_data(data);
> +
> +	writel_relaxed(plda_hwirq_to_mask(data->hwirq),
> +		       port->bridge_addr + ISTATUS_LOCAL); }
> +
> +static void plda_mask_event_irq(struct irq_data *data) {
> +	struct plda_pcie_rp *port =3D irq_data_get_irq_chip_data(data);
> +	u32 mask, val;
> +
> +	mask =3D plda_hwirq_to_mask(data->hwirq);
> +
> +	raw_spin_lock(&port->lock);
> +	val =3D readl_relaxed(port->bridge_addr + IMASK_LOCAL);
> +	val &=3D ~mask;
> +	writel_relaxed(val, port->bridge_addr + IMASK_LOCAL);
> +	raw_spin_unlock(&port->lock);
> +}
> +
> +static void plda_unmask_event_irq(struct irq_data *data) {
> +	struct plda_pcie_rp *port =3D irq_data_get_irq_chip_data(data);
> +	u32 mask, val;
> +
> +	mask =3D plda_hwirq_to_mask(data->hwirq);
> +
> +	raw_spin_lock(&port->lock);
> +	val =3D readl_relaxed(port->bridge_addr + IMASK_LOCAL);
> +	val |=3D mask;
> +	writel_relaxed(val, port->bridge_addr + IMASK_LOCAL);
> +	raw_spin_unlock(&port->lock);
> +}
> +
> +static struct irq_chip plda_event_irq_chip =3D {
> +	.name =3D "PLDA PCIe EVENT",
> +	.irq_ack =3D plda_ack_event_irq,
> +	.irq_mask =3D plda_mask_event_irq,
> +	.irq_unmask =3D plda_unmask_event_irq,
> +};
> +
>  static const struct plda_event_ops plda_event_ops =3D {
>  	.get_events =3D plda_get_events,
>  };
> @@ -777,7 +835,9 @@ static const struct plda_event_ops plda_event_ops =3D
> {  static int plda_pcie_event_map(struct irq_domain *domain, unsigned int=
 irq,
>  			       irq_hw_number_t hwirq)
>  {
> -	irq_set_chip_and_handler(irq, &mc_event_irq_chip, handle_level_irq);
> +	struct plda_pcie_rp *port =3D (void *)domain->host_data;
> +
> +	irq_set_chip_and_handler(irq, port->event_irq_chip, handle_level_irq);
>  	irq_set_chip_data(irq, domain->host_data);
>=20
>  	return 0;
> @@ -962,6 +1022,9 @@ static int plda_init_interrupts(struct platform_devi=
ce
> *pdev,
>  	if (!port->event_ops)
>  		port->event_ops =3D &plda_event_ops;
>=20
> +	if (!port->event_irq_chip)
> +		port->event_irq_chip =3D &plda_event_irq_chip;
> +
>  	ret =3D plda_pcie_init_irq_domains(port);
>  	if (ret) {
>  		dev_err(dev, "failed creating IRQ domains\n"); @@ -1039,6 +1102,7
> @@ static int mc_platform_init(struct pci_config_window *cfg)
>  		return ret;
>=20
>  	port->plda.event_ops =3D &mc_event_ops;
> +	port->plda.event_irq_chip =3D &mc_event_irq_chip;
>=20
>  	/* Address translation is up; safe to enable interrupts */
>  	ret =3D plda_init_interrupts(pdev, &port->plda, &mc_event); diff --git
> a/drivers/pci/controller/plda/pcie-plda.h
> b/drivers/pci/controller/plda/pcie-plda.h
> index e0e5e7cc8434..a3ce01735bea 100644
> --- a/drivers/pci/controller/plda/pcie-plda.h
> +++ b/drivers/pci/controller/plda/pcie-plda.h
> @@ -107,6 +107,8 @@ enum plda_int_event {
>=20
>  #define PLDA_NUM_DMA_EVENTS			16
>=20
> +#define EVENT_PM_MSI_INT_INTX			(PLDA_NUM_DMA_EVENTS +
> PLDA_INTX)
> +#define EVENT_PM_MSI_INT_MSI			(PLDA_NUM_DMA_EVENTS +
> PLDA_MSI)
>  #define PLDA_MAX_EVENT_NUM			(PLDA_NUM_DMA_EVENTS +
> PLDA_INT_EVENT_NUM)
>=20
>  /*
> @@ -155,6 +157,7 @@ struct plda_pcie_rp {
>  	raw_spinlock_t lock;
>  	struct plda_msi msi;
>  	const struct plda_event_ops *event_ops;
> +	const struct irq_chip *event_irq_chip;
>  	void __iomem *bridge_addr;
>  	int num_events;
>  };
> --
> 2.17.1