[PATCH 1/2] dt-bindings: interrupt-controller: Add Sophgo SG2044 MSI controller

[PATCH 1/2] dt-bindings: interrupt-controller: Add Sophgo SG2044 MSI controller

[Inochi Amaoto]

Like SG2042, SG2044 uses a similar msi controller to provide MSI
interrupt for PCIe controllers.

Add support for the SG2044 msi controller

Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
---
 .../bindings/interrupt-controller/sophgo,sg2042-msi.yaml      | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/sophgo,sg2042-msi.yaml b/Documentation/devicetree/bindings/interrupt-controller/sophgo,sg2042-msi.yaml
index e1ffd55fa7bf..f6b8b1d92f79 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/sophgo,sg2042-msi.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/sophgo,sg2042-msi.yaml
@@ -18,7 +18,9 @@ allOf:
 
 properties:
   compatible:
-    const: sophgo,sg2042-msi
+    enum:
+      - sophgo,sg2042-msi
+      - sophgo,sg2044-msi
 
   reg:
     items:
-- 
2.48.1

Re: [PATCH 1/2] dt-bindings: interrupt-controller: Add Sophgo SG2044 MSI controller

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1252 bytes --]

On Mon, Mar 03, 2025 at 07:16:46PM +0800, Inochi Amaoto wrote:
> Like SG2042, SG2044 uses a similar msi controller to provide MSI
> interrupt for PCIe controllers.

In the future, could you mention the way in which they differ?
"similar" could mean 99% the same but incompatible, or 90% the same but
capable of using a fallback?
Acked-by: Conor Dooley <conor.dooley@microchip.com>

> 
> Add support for the SG2044 msi controller
> 
> Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
> ---
>  .../bindings/interrupt-controller/sophgo,sg2042-msi.yaml      | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/sophgo,sg2042-msi.yaml b/Documentation/devicetree/bindings/interrupt-controller/sophgo,sg2042-msi.yaml
> index e1ffd55fa7bf..f6b8b1d92f79 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/sophgo,sg2042-msi.yaml
> +++ b/Documentation/devicetree/bindings/interrupt-controller/sophgo,sg2042-msi.yaml
> @@ -18,7 +18,9 @@ allOf:
>  
>  properties:
>    compatible:
> -    const: sophgo,sg2042-msi
> +    enum:
> +      - sophgo,sg2042-msi
> +      - sophgo,sg2044-msi
>  
>    reg:
>      items:
> -- 
> 2.48.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/2] dt-bindings: interrupt-controller: Add Sophgo SG2044 MSI controller

[Inochi Amaoto]

On Mon, Mar 03, 2025 at 04:29:53PM +0000, Conor Dooley wrote:
> On Mon, Mar 03, 2025 at 07:16:46PM +0800, Inochi Amaoto wrote:
> > Like SG2042, SG2044 uses a similar msi controller to provide MSI
> > interrupt for PCIe controllers.
> 
> In the future, could you mention the way in which they differ?
> "similar" could mean 99% the same but incompatible, or 90% the same but
> capable of using a fallback?
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> 

It is OK for me. The difference between these two controller
are the number of MSI interrupts supports, and the way to
mask/map MSI message.

I will update the comment message for it.

Regards,
Inochi

[PATCH 2/2] irqchip/sg2042-msi: Add the Sophgo SG2044 MSI interrupt controller

[Inochi Amaoto]

Add support for Sophgo SG2044 MSI interrupt controller.

Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
---
 drivers/irqchip/irq-sg2042-msi.c | 86 ++++++++++++++++++++++++++++++--
 1 file changed, 82 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/irq-sg2042-msi.c b/drivers/irqchip/irq-sg2042-msi.c
index 9c0a5f2777a4..4b3992821797 100644
--- a/drivers/irqchip/irq-sg2042-msi.c
+++ b/drivers/irqchip/irq-sg2042-msi.c
@@ -21,6 +21,11 @@
 
 #define SG2042_MAX_MSI_VECTOR	32
 
+struct sg2042_msi_of_data {
+	const struct irq_chip		*irqchip;
+	const struct msi_parent_ops	*parent_ops;
+};
+
 struct sg2042_msi_chipdata {
 	void __iomem	*reg_clr;	// clear reg, see TRM, 10.1.33, GP_INTR0_CLR
 
@@ -29,8 +34,10 @@ struct sg2042_msi_chipdata {
 	u32		irq_first;	// The vector number that MSIs starts
 	u32		num_irqs;	// The number of vectors for MSIs
 
-	DECLARE_BITMAP(msi_map, SG2042_MAX_MSI_VECTOR);
+	unsigned long	*msi_map;
 	struct mutex	msi_map_lock;	// lock for msi_map
+
+	const struct sg2042_msi_of_data	*data;
 };
 
 static int sg2042_msi_allocate_hwirq(struct sg2042_msi_chipdata *data, int num_req)
@@ -81,6 +88,37 @@ static const struct irq_chip sg2042_msi_middle_irq_chip = {
 	.irq_compose_msi_msg	= sg2042_msi_irq_compose_msi_msg,
 };
 
+static void sg2044_msi_irq_ack(struct irq_data *d)
+{
+	struct sg2042_msi_chipdata *data = irq_data_get_irq_chip_data(d);
+
+	writel(0, (unsigned int *)data->reg_clr + d->hwirq);
+
+	irq_chip_ack_parent(d);
+}
+
+static void sg2044_msi_irq_compose_msi_msg(struct irq_data *d,
+					   struct msi_msg *msg)
+{
+	struct sg2042_msi_chipdata *data = irq_data_get_irq_chip_data(d);
+	phys_addr_t doorbell = data->doorbell_addr +  4 * (d->hwirq / 32);
+
+	msg->address_lo = lower_32_bits(doorbell);
+	msg->address_hi = upper_32_bits(doorbell);
+	msg->data = d->hwirq % 32;
+}
+
+static struct irq_chip sg2044_msi_middle_irq_chip = {
+	.name			= "SG2044 MSI",
+	.irq_ack		= sg2044_msi_irq_ack,
+	.irq_mask		= irq_chip_mask_parent,
+	.irq_unmask		= irq_chip_unmask_parent,
+#ifdef CONFIG_SMP
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+#endif
+	.irq_compose_msi_msg	= sg2044_msi_irq_compose_msi_msg,
+};
+
 static int sg2042_msi_parent_domain_alloc(struct irq_domain *domain,
 					  unsigned int virq, int hwirq)
 {
@@ -119,7 +157,7 @@ static int sg2042_msi_middle_domain_alloc(struct irq_domain *domain,
 			goto err_hwirq;
 
 		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
-					      &sg2042_msi_middle_irq_chip, data);
+					      data->data->irqchip, data);
 	}
 
 	return 0;
@@ -162,6 +200,21 @@ static const struct msi_parent_ops sg2042_msi_parent_ops = {
 	.init_dev_msi_info	= msi_lib_init_dev_msi_info,
 };
 
+#define SG2044_MSI_FLAGS_REQUIRED (MSI_FLAG_USE_DEF_DOM_OPS |	\
+				   MSI_FLAG_USE_DEF_CHIP_OPS)
+
+#define SG2044_MSI_FLAGS_SUPPORTED (MSI_GENERIC_FLAGS_MASK |	\
+				    MSI_FLAG_PCI_MSIX)
+
+static const struct msi_parent_ops sg2044_msi_parent_ops = {
+	.required_flags		= SG2044_MSI_FLAGS_REQUIRED,
+	.supported_flags	= SG2044_MSI_FLAGS_SUPPORTED,
+	.bus_select_mask	= MATCH_PCI_MSI,
+	.bus_select_token	= DOMAIN_BUS_NEXUS,
+	.prefix			= "SG2044-",
+	.init_dev_msi_info	= msi_lib_init_dev_msi_info,
+};
+
 static int sg2042_msi_init_domains(struct sg2042_msi_chipdata *data,
 				   struct irq_domain *plic_domain,
 				   struct device *dev)
@@ -181,7 +234,7 @@ static int sg2042_msi_init_domains(struct sg2042_msi_chipdata *data,
 	irq_domain_update_bus_token(middle_domain, DOMAIN_BUS_NEXUS);
 
 	middle_domain->flags |= IRQ_DOMAIN_FLAG_MSI_PARENT;
-	middle_domain->msi_parent_ops = &sg2042_msi_parent_ops;
+	middle_domain->msi_parent_ops = data->data->parent_ops;
 
 	return 0;
 }
@@ -199,6 +252,12 @@ static int sg2042_msi_probe(struct platform_device *pdev)
 	if (!data)
 		return -ENOMEM;
 
+	data->data = device_get_match_data(&pdev->dev);
+	if (!data->data) {
+		dev_err(&pdev->dev, "Failed to get irqchip\n");
+		return -EINVAL;
+	}
+
 	data->reg_clr = devm_platform_ioremap_resource_byname(pdev, "clr");
 	if (IS_ERR(data->reg_clr)) {
 		dev_err(dev, "Failed to map clear register\n");
@@ -240,11 +299,30 @@ static int sg2042_msi_probe(struct platform_device *pdev)
 
 	mutex_init(&data->msi_map_lock);
 
+	data->msi_map = devm_bitmap_zalloc(&pdev->dev, data->num_irqs, GFP_KERNEL);
+	if (!data->msi_map) {
+		dev_err(&pdev->dev, "Unable to allocate msi mapping\n");
+		return -ENOMEM;
+	}
+
 	return sg2042_msi_init_domains(data, plic_domain, dev);
 }
 
+static const struct sg2042_msi_of_data sg2042_of_data = {
+	.irqchip	= &sg2042_msi_middle_irq_chip,
+	.parent_ops	= &sg2042_msi_parent_ops,
+};
+
+static const struct sg2042_msi_of_data sg2044_of_data = {
+	.irqchip	= &sg2044_msi_middle_irq_chip,
+	.parent_ops	= &sg2044_msi_parent_ops,
+};
+
 static const struct of_device_id sg2042_msi_of_match[] = {
-	{ .compatible	= "sophgo,sg2042-msi" },
+	{ .compatible	= "sophgo,sg2042-msi",
+	  .data		= &sg2042_of_data },
+	{ .compatible	= "sophgo,sg2044-msi",
+	  .data		= &sg2044_of_data },
 	{}
 };
 
-- 
2.48.1

Re: [PATCH 2/2] irqchip/sg2042-msi: Add the Sophgo SG2044 MSI interrupt controller

[Thomas Gleixner]

On Mon, Mar 03 2025 at 19:16, Inochi Amaoto wrote:
> Add support for Sophgo SG2044 MSI interrupt controller.

This patch fails to apply on top of:

     git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/drivers

Please always ensure that your patches apply against the tree/branch
into which they are supposed to be merged. Grabbing random patches from
the mailing list as base is not sufficient. It's clearly documented
against what you should work.
     
> +struct sg2042_msi_of_data {

There is nothing specific to OF in this data structure. This structure
contains the chip and the MSI parent ops of each variant. So something
like sg204x_chip_info is way more descriptive.

> +	const struct irq_chip		*irqchip;
> +	const struct msi_parent_ops	*parent_ops;
> +};
> +
>  struct sg2042_msi_chipdata {

and rename that one to sg204x_... as it is not longer sg2042 specific.

>  	void __iomem	*reg_clr;	// clear reg, see TRM, 10.1.33, GP_INTR0_CLR
>  
> @@ -29,8 +34,10 @@ struct sg2042_msi_chipdata {
>  	u32		irq_first;	// The vector number that MSIs starts
>  	u32		num_irqs;	// The number of vectors for MSIs
>  
> -	DECLARE_BITMAP(msi_map, SG2042_MAX_MSI_VECTOR);
> +	unsigned long	*msi_map;
>  	struct mutex	msi_map_lock;	// lock for msi_map
> +
> +	const struct sg2042_msi_of_data	*data;

Please keep the tabular formatting of this struct. See:

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#coding-style-notes

>  };
>  
>  static int sg2042_msi_allocate_hwirq(struct sg2042_msi_chipdata *data, int num_req)
> @@ -81,6 +88,37 @@ static const struct irq_chip sg2042_msi_middle_irq_chip = {
>  	.irq_compose_msi_msg	= sg2042_msi_irq_compose_msi_msg,
>  };
>  
> +static void sg2044_msi_irq_ack(struct irq_data *d)
> +{
> +	struct sg2042_msi_chipdata *data = irq_data_get_irq_chip_data(d);
> +
> +	writel(0, (unsigned int *)data->reg_clr + d->hwirq);
> +

Pointless newline

> +	irq_chip_ack_parent(d);
> +}
> +
> +static void sg2044_msi_irq_compose_msi_msg(struct irq_data *d,
> +					   struct msi_msg *msg)

No line break required. Please use up to 100 characters.

>  static int sg2042_msi_parent_domain_alloc(struct irq_domain *domain,
>  					  unsigned int virq, int hwirq)
>  {
> @@ -119,7 +157,7 @@ static int sg2042_msi_middle_domain_alloc(struct irq_domain *domain,
>  			goto err_hwirq;
>  
>  		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> -					      &sg2042_msi_middle_irq_chip, data);
> +					      data->data->irqchip, data);

The conversion of the existing code to this should be a preparatory patch
for ease of review and the support for the new chip built on top.

Also please come up with a sensible name for this new 'data' pointer.

     data->data->

is horribly unintuitive. It's not the same data type. 

     data->chip_info

or such makes it clear what this is about.

Thanks,

        tglx

Re: [PATCH 2/2] irqchip/sg2042-msi: Add the Sophgo SG2044 MSI interrupt controller

[Inochi Amaoto]

On Mon, Mar 03, 2025 at 08:31:32PM +0100, Thomas Gleixner wrote:
> On Mon, Mar 03 2025 at 19:16, Inochi Amaoto wrote:
> > Add support for Sophgo SG2044 MSI interrupt controller.
> 
> This patch fails to apply on top of:
> 
>      git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/drivers
> 
> Please always ensure that your patches apply against the tree/branch
> into which they are supposed to be merged. Grabbing random patches from
> the mailing list as base is not sufficient. It's clearly documented
> against what you should work.
>      

Thanks for pointing that, I will check the tip tree and see what
is conflicted. I forgot there will be something changed when
merging patch.

> > +struct sg2042_msi_of_data {
> 
> There is nothing specific to OF in this data structure. This structure
> contains the chip and the MSI parent ops of each variant. So something
> like sg204x_chip_info is way more descriptive.
> 

Yeah, chip_info it more clear than of_data. I have forgotten this
driver is not just for dtb but also UEFI fdt.

> > +	const struct irq_chip		*irqchip;
> > +	const struct msi_parent_ops	*parent_ops;
> > +};
> > +
> >  struct sg2042_msi_chipdata {
> 
> and rename that one to sg204x_... as it is not longer sg2042 specific.
> 

This is OK for me.

> >  	void __iomem	*reg_clr;	// clear reg, see TRM, 10.1.33, GP_INTR0_CLR
> >  
> > @@ -29,8 +34,10 @@ struct sg2042_msi_chipdata {
> >  	u32		irq_first;	// The vector number that MSIs starts
> >  	u32		num_irqs;	// The number of vectors for MSIs
> >  
> > -	DECLARE_BITMAP(msi_map, SG2042_MAX_MSI_VECTOR);
> > +	unsigned long	*msi_map;
> >  	struct mutex	msi_map_lock;	// lock for msi_map
> > +
> > +	const struct sg2042_msi_of_data	*data;
> 
> Please keep the tabular formatting of this struct. See:
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#coding-style-notes
> 
> >  };
> >  
> >  static int sg2042_msi_allocate_hwirq(struct sg2042_msi_chipdata *data, int num_req)
> > @@ -81,6 +88,37 @@ static const struct irq_chip sg2042_msi_middle_irq_chip = {
> >  	.irq_compose_msi_msg	= sg2042_msi_irq_compose_msi_msg,
> >  };
> >  
> > +static void sg2044_msi_irq_ack(struct irq_data *d)
> > +{
> > +	struct sg2042_msi_chipdata *data = irq_data_get_irq_chip_data(d);
> > +
> > +	writel(0, (unsigned int *)data->reg_clr + d->hwirq);
> > +
> 
> Pointless newline
> 
> > +	irq_chip_ack_parent(d);
> > +}
> > +
> > +static void sg2044_msi_irq_compose_msi_msg(struct irq_data *d,
> > +					   struct msi_msg *msg)
> 
> No line break required. Please use up to 100 characters.
> 
> >  static int sg2042_msi_parent_domain_alloc(struct irq_domain *domain,
> >  					  unsigned int virq, int hwirq)
> >  {
> > @@ -119,7 +157,7 @@ static int sg2042_msi_middle_domain_alloc(struct irq_domain *domain,
> >  			goto err_hwirq;
> >  
> >  		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> > -					      &sg2042_msi_middle_irq_chip, data);
> > +					      data->data->irqchip, data);
> 
> The conversion of the existing code to this should be a preparatory patch
> for ease of review and the support for the new chip built on top.
> 
> Also please come up with a sensible name for this new 'data' pointer.
> 
>      data->data->
> 
> is horribly unintuitive. It's not the same data type. 
> 
>      data->chip_info
> 
> or such makes it clear what this is about.
> 

Good, I will take care of that.

Regards,
Inochi

Re: [PATCH 2/2] irqchip/sg2042-msi: Add the Sophgo SG2044 MSI interrupt controller

[Chen Wang]

On 2025/3/3 19:16, Inochi Amaoto wrote:
> Add support for Sophgo SG2044 MSI interrupt controller.
>
> Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
> ---
>   drivers/irqchip/irq-sg2042-msi.c | 86 ++++++++++++++++++++++++++++++--
>   1 file changed, 82 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/irqchip/irq-sg2042-msi.c b/drivers/irqchip/irq-sg2042-msi.c
> index 9c0a5f2777a4..4b3992821797 100644
> --- a/drivers/irqchip/irq-sg2042-msi.c
> +++ b/drivers/irqchip/irq-sg2042-msi.c
> @@ -21,6 +21,11 @@
>   
>   #define SG2042_MAX_MSI_VECTOR	32

Drop this since it is useless now.

[......]

Chen