diff mbox series

gpio: dwapb: add support for new hisilicon ascend soc

Message ID 1598070473-46624-1-git-send-email-dingtianhong@huawei.com
State New
Headers show
Series gpio: dwapb: add support for new hisilicon ascend soc | expand

Commit Message

Ding Tianhong Aug. 22, 2020, 4:27 a.m. UTC 
The hisilicon ascend soc's gpio is based on the synopsys DW gpio,
and expand the register to support for INTCOMB_MASK, the new
register is used to enable/disable the interrupt combine features.

Both support for ACPI and Device Tree.

Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>

---
 drivers/gpio/gpio-dwapb.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

-- 
1.8.3.1

Comments

Serge Semin Aug. 25, 2020, 9:57 a.m. UTC | #1 
Hello Ding,
Thanks for the patch. My comments are below.

On Sat, Aug 22, 2020 at 12:27:53PM +0800, Ding Tianhong wrote:
> The hisilicon ascend soc's gpio is based on the synopsys DW gpio,

> and expand the register to support for INTCOMB_MASK, the new

> register is used to enable/disable the interrupt combine features.


I am wondering what does the "Interrupt Combine" feature do? Is it the same as
the GPIO_INTR_IO pre-synthesize parameter of the DW_apb_gpio IP-core? Is it
possible to tune the DW APB GPIO controller at runtime sending up to the IRQ
controller either combined or individual interrupts?

If the later is true, then probably having the "Interrupt Combine" feature
enabled must depend on whether an individual or combined interrupts are supplied
in dts, etc...

Could you explain the way the feature works and the corresponding layout
register in more details?

> 

> Both support for ACPI and Device Tree.

> 

> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>

> ---

>  drivers/gpio/gpio-dwapb.c | 28 ++++++++++++++++++++++++++++

>  1 file changed, 28 insertions(+)

> 

> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c

> index 1d8d55b..923b381 100644

> --- a/drivers/gpio/gpio-dwapb.c

> +++ b/drivers/gpio/gpio-dwapb.c

> @@ -49,6 +49,8 @@

>  #define GPIO_EXT_PORTC		0x58

>  #define GPIO_EXT_PORTD		0x5c


>  

> +#define GPIO_INTCOMB_MASK	0xffc


If the register is the HiSilicon Ascend SoC specific, then I'd suggest to add
the vendor-specific prefix like: GPIO_HS_INTCOMB_MASK.

Also pay attention to the register naming. Here you define it as "INTCOMB",
later as "INT_COMB". It's better to have the same references everywhere: either
with underscore or without it.

Also please move the new register definition to be after the corresponding
feature macro definition (see the next comment for detailts).

> +

>  #define DWAPB_DRIVER_NAME	"gpio-dwapb"

>  #define DWAPB_MAX_PORTS		4

>  

> @@ -58,6 +60,10 @@

>  

>  #define GPIO_REG_OFFSET_V2	1


>  

> +#define GPIO_REG_INT_COMB	2


Please move this macro to be define after the "GPIO_PORTA_EOI_V2" one, so to
make the blocked-like macro definitions order. Like this:
	#define GPIO_REG_OFFSET_V2	BIT(0)	// Reg V2 Feature macro

	#define GPIO_INTMASK_V2		0x44	// Reg V2-specific macro
	...
	#define GPIO_PORTA_EOI_V2	0x40	// Reg V2-specific macro

	+ #define GPIO_REG_HS_INTCOMB	BIT(1)	// HiSilicon Feature macro
	+ 
	+ #define GPIO_HS_INTCOMB_MASK	0xffc	// HiSilicon-specific macro
	+ 

I missed that in my series, but having the flags defined as decimals isn't good.
Could you convert GPIO_REG_HS_INTCOMB and GPIO_REG_OFFSET_V2 to be defined as BIT(x)?

The same comment about HS_-prefixing. Perhaps GPIO_REG_HS_INTCOMB?

> +#define ENABLE_INT_COMB		1

> +#define DISABLE_INT_COMB	0


I don't really see a point in having these two macros defined. They are basically
just the boolean flags. Please see my next comment.

> +

>  #define GPIO_INTMASK_V2		0x44

>  #define GPIO_INTTYPE_LEVEL_V2	0x34

>  #define GPIO_INT_POLARITY_V2	0x38

> @@ -354,6 +360,20 @@ static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id)

>  	return IRQ_RETVAL(dwapb_do_irq(dev_id));

>  }

>  


> +static void dwapb_enable_inq_combine(struct dwapb_gpio *gpio, unsigned int enable)


inq_combine or int_combine?

"enable" is used here as boolean, then it should be declared as one.

> +{

> +	u32 val;

> +

> +	if (gpio->flags & GPIO_REG_INT_COMB) {

> +		val = dwapb_read(gpio, GPIO_INTCOMB_MASK);

> +		if (enable)

> +			val |= BIT(0);

> +		else

> +			val &= BIT(0);

> +		dwapb_write(gpio, GPIO_INTCOMB_MASK, val);

> +	}

> +}

> +


Do you need to preserve the register content by using the read-modify-write procedure
here? If not, then inlining something like this should be fine:
	if (gpio->flags & GPIO_REG_HS_INTCOMB)
		dwapb_write(gpio, GPIO_HS_INTCOMB_MASK, 1);

>  static void dwapb_configure_irqs(struct dwapb_gpio *gpio,

>  				 struct dwapb_gpio_port *port,

>  				 struct dwapb_port_property *pp)

> @@ -446,6 +466,8 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,

>  		irq_create_mapping(gpio->domain, hwirq);

>  

>  	port->gc.to_irq = dwapb_gpio_to_irq;

> +

> +	dwapb_enable_inq_combine(gpio, ENABLE_INT_COMB);

>  }

>  

>  static void dwapb_irq_teardown(struct dwapb_gpio *gpio)

> @@ -618,6 +640,7 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)

>  static const struct of_device_id dwapb_of_match[] = {

>  	{ .compatible = "snps,dw-apb-gpio", .data = (void *)0},

>  	{ .compatible = "apm,xgene-gpio-v2", .data = (void *)GPIO_REG_OFFSET_V2},

> +	{ .compatible = "hisi,ascend-gpio", .data = (void *)GPIO_REG_INT_COMB},

>  	{ /* Sentinel */ }

>  };

>  MODULE_DEVICE_TABLE(of, dwapb_of_match);

> @@ -626,6 +649,7 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)

>  	{"HISI0181", 0},

>  	{"APMC0D07", 0},

>  	{"APMC0D81", GPIO_REG_OFFSET_V2},

> +	{"HISI19XX", GPIO_REG_INT_COMB},

>  	{ }

>  };

>  MODULE_DEVICE_TABLE(acpi, dwapb_acpi_match);


> @@ -713,6 +737,8 @@ static int dwapb_gpio_remove(struct platform_device *pdev)

>  	reset_control_assert(gpio->rst);

>  	clk_bulk_disable_unprepare(DWAPB_NR_CLOCKS, gpio->clks);

>  

> +	dwapb_enable_inq_combine(gpio, DISABLE_INT_COMB);

> +

>  	return 0;

>  }


Note the dwapb_gpio_remove method will be discarded from the driver in the framework
of the next series: https://patchwork.ozlabs.org/project/linux-gpio/list/?series=193334

So if you really need to revert the GPIO_HS_INTCOMB_MASK flag setting, then
you'd need to create and register a dedicated devm-action (see 8 and 9 patch in
my series for example).

BTW Linus, could you take a look at my series? Andy and Rob have finished reviewing
it almost a month ago.

-Sergey

>  

> @@ -794,6 +820,8 @@ static int dwapb_gpio_resume(struct device *dev)

>  			dwapb_write(gpio, GPIO_INTEN, ctx->int_en);

>  			dwapb_write(gpio, GPIO_INTMASK, ctx->int_mask);

>  

> +			dwapb_enable_inq_combine(gpio, ENABLE_INT_COMB);

> +

>  			/* Clear out spurious interrupts */

>  			dwapb_write(gpio, GPIO_PORTA_EOI, 0xffffffff);

>  		}

> -- 

> 1.8.3.1

>
Andy Shevchenko Aug. 27, 2020, 8:19 a.m. UTC | #2 
On Tue, Aug 25, 2020 at 12:58 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> On Sat, Aug 22, 2020 at 12:27:53PM +0800, Ding Tianhong wrote:


> BTW Linus, could you take a look at my series? Andy and Rob have finished reviewing

> it almost a month ago.


I was wondering the same, but in normal cases (not closer to the merge
window) Bart is taking care of drivers/gpio (AFAIU).


-- 
With Best Regards,
Andy Shevchenko
Linus Walleij Aug. 27, 2020, 8:34 a.m. UTC | #3 
On Thu, Aug 27, 2020 at 10:20 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Aug 25, 2020 at 12:58 PM Serge Semin <fancer.lancer@gmail.com> wrote:

> > On Sat, Aug 22, 2020 at 12:27:53PM +0800, Ding Tianhong wrote:

>

> > BTW Linus, could you take a look at my series? Andy and Rob have finished reviewing

> > it almost a month ago.

>

> I was wondering the same, but in normal cases (not closer to the merge

> window) Bart is taking care of drivers/gpio (AFAIU).


I just had too much to do, sorry folks :/
It is queued now.

Yours,
Linus Walleij
Bartosz Golaszewski Aug. 28, 2020, 6:21 p.m. UTC | #4 
On Thu, Aug 27, 2020 at 10:20 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>

> On Tue, Aug 25, 2020 at 12:58 PM Serge Semin <fancer.lancer@gmail.com> wrote:

> > On Sat, Aug 22, 2020 at 12:27:53PM +0800, Ding Tianhong wrote:

>

> > BTW Linus, could you take a look at my series? Andy and Rob have finished reviewing

> > it almost a month ago.

>

> I was wondering the same, but in normal cases (not closer to the merge

> window) Bart is taking care of drivers/gpio (AFAIU).

>

>


Yeah, normally I'd have queued it by now but I'm only coming back to
100% activity on Tuesday - I was on a leave since July so I was rather
inactive lately.

Bartosz
Serge Semin Sept. 12, 2020, 1:31 p.m. UTC | #5 
On Sun, Sep 06, 2020 at 06:18:07AM +0000, Dingtianhong wrote:

[...]

> > On Sat, Aug 22, 2020 at 12:27:53PM +0800, Ding Tianhong wrote:

> >> The hisilicon ascend soc's gpio is based on the synopsys DW gpio,

> >> and expand the register to support for INTCOMB_MASK, the new

> >> register is used to enable/disable the interrupt combine features.

> > 

> > I am wondering what does the "Interrupt Combine" feature do? Is it the same as

> > the GPIO_INTR_IO pre-synthesize parameter of the DW_apb_gpio IP-core? Is it

> > possible to tune the DW APB GPIO controller at runtime sending up to the IRQ

> > controller either combined or individual interrupts?

> > 

> 

> looks like the same.

> 

> > If the later is true, then probably having the "Interrupt Combine" feature

> > enabled must depend on whether an individual or combined interrupts are supplied

> > in dts, etc...

> > 

> 

> needed.

> 

> > Could you explain the way the feature works and the corresponding layout

> > register in more details?

> > 

> 

> Ok

> The hisilicon chip use the register called GPIO_INTCOMB_MASK (offset is 0xffc) to enable the combien interrupt.

> it is very simple, if GPIO_INTCOMB_MASK.bit0 is 0, then all combine interrupte could not be used (default

> setting), otherwise if 1, then the 32 ports could use the same irq line, that is all.


The main question is whether your hardware is capable of working with both
combined and individual interrupts. Is your version of the DW APB GPIO
controller able to generate both types of them? How is it connected to the
parental interrupt controller?

So If the GPIO and IRQ controllers are attached to each other with a single lane
gpio_intr_flag, then indeed it's pure combined IRQ design and we'll have to make
sure that GPIO_INTCOMB_MASK.bit0 is set to one before using the DW GPIO block.
If they are also connected with each other by 32 individual GPIO-IRQ
gpio_intr{_n}N lanes, then setting or clearing of the GPIO_INTCOMB_MASK.bit0
flag will for example depend on the number IRQ lanes specified in a dts file. In
the later case the patch needs to be altered, but it would provide a better
support of the hisilicon ascend soc's GPIO capabilities.

-Sergey
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 1d8d55b..923b381 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -49,6 +49,8 @@ 
 #define GPIO_EXT_PORTC		0x58
 #define GPIO_EXT_PORTD		0x5c
 
+#define GPIO_INTCOMB_MASK	0xffc
+
 #define DWAPB_DRIVER_NAME	"gpio-dwapb"
 #define DWAPB_MAX_PORTS		4
 
@@ -58,6 +60,10 @@ 
 
 #define GPIO_REG_OFFSET_V2	1
 
+#define GPIO_REG_INT_COMB	2
+#define ENABLE_INT_COMB		1
+#define DISABLE_INT_COMB	0
+
 #define GPIO_INTMASK_V2		0x44
 #define GPIO_INTTYPE_LEVEL_V2	0x34
 #define GPIO_INT_POLARITY_V2	0x38
@@ -354,6 +360,20 @@  static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id)
 	return IRQ_RETVAL(dwapb_do_irq(dev_id));
 }
 
+static void dwapb_enable_inq_combine(struct dwapb_gpio *gpio, unsigned int enable)
+{
+	u32 val;
+
+	if (gpio->flags & GPIO_REG_INT_COMB) {
+		val = dwapb_read(gpio, GPIO_INTCOMB_MASK);
+		if (enable)
+			val |= BIT(0);
+		else
+			val &= BIT(0);
+		dwapb_write(gpio, GPIO_INTCOMB_MASK, val);
+	}
+}
+
 static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 				 struct dwapb_gpio_port *port,
 				 struct dwapb_port_property *pp)
@@ -446,6 +466,8 @@  static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 		irq_create_mapping(gpio->domain, hwirq);
 
 	port->gc.to_irq = dwapb_gpio_to_irq;
+
+	dwapb_enable_inq_combine(gpio, ENABLE_INT_COMB);
 }
 
 static void dwapb_irq_teardown(struct dwapb_gpio *gpio)
@@ -618,6 +640,7 @@  static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
 static const struct of_device_id dwapb_of_match[] = {
 	{ .compatible = "snps,dw-apb-gpio", .data = (void *)0},
 	{ .compatible = "apm,xgene-gpio-v2", .data = (void *)GPIO_REG_OFFSET_V2},
+	{ .compatible = "hisi,ascend-gpio", .data = (void *)GPIO_REG_INT_COMB},
 	{ /* Sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, dwapb_of_match);
@@ -626,6 +649,7 @@  static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
 	{"HISI0181", 0},
 	{"APMC0D07", 0},
 	{"APMC0D81", GPIO_REG_OFFSET_V2},
+	{"HISI19XX", GPIO_REG_INT_COMB},
 	{ }
 };
 MODULE_DEVICE_TABLE(acpi, dwapb_acpi_match);
@@ -713,6 +737,8 @@  static int dwapb_gpio_remove(struct platform_device *pdev)
 	reset_control_assert(gpio->rst);
 	clk_bulk_disable_unprepare(DWAPB_NR_CLOCKS, gpio->clks);
 
+	dwapb_enable_inq_combine(gpio, DISABLE_INT_COMB);
+
 	return 0;
 }
 
@@ -794,6 +820,8 @@  static int dwapb_gpio_resume(struct device *dev)
 			dwapb_write(gpio, GPIO_INTEN, ctx->int_en);
 			dwapb_write(gpio, GPIO_INTMASK, ctx->int_mask);
 
+			dwapb_enable_inq_combine(gpio, ENABLE_INT_COMB);
+
 			/* Clear out spurious interrupts */
 			dwapb_write(gpio, GPIO_PORTA_EOI, 0xffffffff);
 		}