* [PATCH] pinctrl: ingenic: Garbage-collect code paths for SoCs disabled by config
@ 2022-03-15 17:39 Paul Cercueil
2022-03-16 14:47 ` Andy Shevchenko
0 siblings, 1 reply; 7+ messages in thread
From: Paul Cercueil @ 2022-03-15 17:39 UTC (permalink / raw)
To: Linus Walleij; +Cc: od, linux-mips, linux-gpio, linux-kernel, Paul Cercueil
By being a bit smarter about how the SoC version checks are performed,
it is possible to have all the code paths that correspond to SoCs
disabled in the kernel config automatically marked as dead code by the
compiler, and therefore garbage-collected.
With this patch, when compiling a kernel that only targets the JZ4760
for instance, the driver is now about 4.5 KiB smaller.
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
drivers/pinctrl/pinctrl-ingenic.c | 118 ++++++++++++++++++------------
1 file changed, 71 insertions(+), 47 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-ingenic.c b/drivers/pinctrl/pinctrl-ingenic.c
index 2712f51eb238..61d42a5e9768 100644
--- a/drivers/pinctrl/pinctrl-ingenic.c
+++ b/drivers/pinctrl/pinctrl-ingenic.c
@@ -137,6 +137,30 @@ struct ingenic_gpio_chip {
unsigned int irq, reg_base;
};
+static const u32 enabled_socs =
+ IS_ENABLED(CONFIG_MACH_JZ4730) << ID_JZ4730 |
+ IS_ENABLED(CONFIG_MACH_JZ4740) << ID_JZ4740 |
+ IS_ENABLED(CONFIG_MACH_JZ4725B) << ID_JZ4725B |
+ IS_ENABLED(CONFIG_MACH_JZ4750) << ID_JZ4750 |
+ IS_ENABLED(CONFIG_MACH_JZ4755) << ID_JZ4755 |
+ IS_ENABLED(CONFIG_MACH_JZ4760) << ID_JZ4760 |
+ IS_ENABLED(CONFIG_MACH_JZ4770) << ID_JZ4770 |
+ IS_ENABLED(CONFIG_MACH_JZ4775) << ID_JZ4775 |
+ IS_ENABLED(CONFIG_MACH_JZ4780) << ID_JZ4780 |
+ IS_ENABLED(CONFIG_MACH_X1000) << ID_X1000 |
+ IS_ENABLED(CONFIG_MACH_X1500) << ID_X1500 |
+ IS_ENABLED(CONFIG_MACH_X1830) << ID_X1830 |
+ IS_ENABLED(CONFIG_MACH_X2000) << ID_X2000 |
+ IS_ENABLED(CONFIG_MACH_X2100) << ID_X2100;
+
+static bool
+is_soc_or_above(const struct ingenic_pinctrl *jzpc, enum jz_version version)
+{
+ return (enabled_socs >> version) &&
+ (!(enabled_socs & GENMASK((unsigned int)version - 1, 0))
+ || jzpc->info->version >= version);
+}
+
static const u32 jz4730_pull_ups[4] = {
0x3fa3320f, 0xf200ffff, 0xffffffff, 0xffffffff,
};
@@ -3203,7 +3227,7 @@ static u32 ingenic_gpio_read_reg(struct ingenic_gpio_chip *jzgc, u8 reg)
static void ingenic_gpio_set_bit(struct ingenic_gpio_chip *jzgc,
u8 reg, u8 offset, bool set)
{
- if (jzgc->jzpc->info->version == ID_JZ4730) {
+ if (!is_soc_or_above(jzgc->jzpc, ID_JZ4740)) {
regmap_update_bits(jzgc->jzpc->map, jzgc->reg_base + reg,
BIT(offset), set ? BIT(offset) : 0);
return;
@@ -3261,9 +3285,9 @@ static inline bool ingenic_gpio_get_value(struct ingenic_gpio_chip *jzgc,
static void ingenic_gpio_set_value(struct ingenic_gpio_chip *jzgc,
u8 offset, int value)
{
- if (jzgc->jzpc->info->version >= ID_JZ4770)
+ if (is_soc_or_above(jzgc->jzpc, ID_JZ4770))
ingenic_gpio_set_bit(jzgc, JZ4770_GPIO_PAT0, offset, !!value);
- else if (jzgc->jzpc->info->version >= ID_JZ4740)
+ else if (is_soc_or_above(jzgc->jzpc, ID_JZ4740))
ingenic_gpio_set_bit(jzgc, JZ4740_GPIO_DATA, offset, !!value);
else
ingenic_gpio_set_bit(jzgc, JZ4730_GPIO_DATA, offset, !!value);
@@ -3298,10 +3322,10 @@ static void irq_set_type(struct ingenic_gpio_chip *jzgc,
break;
}
- if (jzgc->jzpc->info->version >= ID_JZ4770) {
+ if (is_soc_or_above(jzgc->jzpc, ID_JZ4770)) {
reg1 = JZ4770_GPIO_PAT1;
reg2 = JZ4770_GPIO_PAT0;
- } else if (jzgc->jzpc->info->version >= ID_JZ4740) {
+ } else if (is_soc_or_above(jzgc->jzpc, ID_JZ4740)) {
reg1 = JZ4740_GPIO_TRIG;
reg2 = JZ4740_GPIO_DIR;
} else {
@@ -3311,12 +3335,12 @@ static void irq_set_type(struct ingenic_gpio_chip *jzgc,
return;
}
- if (jzgc->jzpc->info->version >= ID_X2000) {
+ if (is_soc_or_above(jzgc->jzpc, ID_X2000)) {
ingenic_gpio_shadow_set_bit(jzgc, reg2, offset, val1);
ingenic_gpio_shadow_set_bit(jzgc, reg1, offset, val2);
ingenic_gpio_shadow_set_bit_load(jzgc);
ingenic_gpio_set_bit(jzgc, X2000_GPIO_EDG, offset, val3);
- } else if (jzgc->jzpc->info->version >= ID_X1000) {
+ } else if (is_soc_or_above(jzgc->jzpc, ID_X1000)) {
ingenic_gpio_shadow_set_bit(jzgc, reg2, offset, val1);
ingenic_gpio_shadow_set_bit(jzgc, reg1, offset, val2);
ingenic_gpio_shadow_set_bit_load(jzgc);
@@ -3332,7 +3356,7 @@ static void ingenic_gpio_irq_mask(struct irq_data *irqd)
struct ingenic_gpio_chip *jzgc = gpiochip_get_data(gc);
int irq = irqd->hwirq;
- if (jzgc->jzpc->info->version >= ID_JZ4740)
+ if (is_soc_or_above(jzgc->jzpc, ID_JZ4740))
ingenic_gpio_set_bit(jzgc, GPIO_MSK, irq, true);
else
ingenic_gpio_set_bit(jzgc, JZ4730_GPIO_GPIMR, irq, true);
@@ -3344,7 +3368,7 @@ static void ingenic_gpio_irq_unmask(struct irq_data *irqd)
struct ingenic_gpio_chip *jzgc = gpiochip_get_data(gc);
int irq = irqd->hwirq;
- if (jzgc->jzpc->info->version >= ID_JZ4740)
+ if (is_soc_or_above(jzgc->jzpc, ID_JZ4740))
ingenic_gpio_set_bit(jzgc, GPIO_MSK, irq, false);
else
ingenic_gpio_set_bit(jzgc, JZ4730_GPIO_GPIMR, irq, false);
@@ -3356,9 +3380,9 @@ static void ingenic_gpio_irq_enable(struct irq_data *irqd)
struct ingenic_gpio_chip *jzgc = gpiochip_get_data(gc);
int irq = irqd->hwirq;
- if (jzgc->jzpc->info->version >= ID_JZ4770)
+ if (is_soc_or_above(jzgc->jzpc, ID_JZ4770))
ingenic_gpio_set_bit(jzgc, JZ4770_GPIO_INT, irq, true);
- else if (jzgc->jzpc->info->version >= ID_JZ4740)
+ else if (is_soc_or_above(jzgc->jzpc, ID_JZ4740))
ingenic_gpio_set_bit(jzgc, JZ4740_GPIO_SELECT, irq, true);
else
ingenic_gpio_set_bit(jzgc, JZ4730_GPIO_GPIER, irq, true);
@@ -3374,9 +3398,9 @@ static void ingenic_gpio_irq_disable(struct irq_data *irqd)
ingenic_gpio_irq_mask(irqd);
- if (jzgc->jzpc->info->version >= ID_JZ4770)
+ if (is_soc_or_above(jzgc->jzpc, ID_JZ4770))
ingenic_gpio_set_bit(jzgc, JZ4770_GPIO_INT, irq, false);
- else if (jzgc->jzpc->info->version >= ID_JZ4740)
+ else if (is_soc_or_above(jzgc->jzpc, ID_JZ4740))
ingenic_gpio_set_bit(jzgc, JZ4740_GPIO_SELECT, irq, false);
else
ingenic_gpio_set_bit(jzgc, JZ4730_GPIO_GPIER, irq, false);
@@ -3390,7 +3414,7 @@ static void ingenic_gpio_irq_ack(struct irq_data *irqd)
bool high;
if ((irqd_get_trigger_type(irqd) == IRQ_TYPE_EDGE_BOTH) &&
- (jzgc->jzpc->info->version < ID_X2000)) {
+ !is_soc_or_above(jzgc->jzpc, ID_X2000)) {
/*
* Switch to an interrupt for the opposite edge to the one that
* triggered the interrupt being ACKed.
@@ -3402,9 +3426,9 @@ static void ingenic_gpio_irq_ack(struct irq_data *irqd)
irq_set_type(jzgc, irq, IRQ_TYPE_LEVEL_HIGH);
}
- if (jzgc->jzpc->info->version >= ID_JZ4770)
+ if (is_soc_or_above(jzgc->jzpc, ID_JZ4770))
ingenic_gpio_set_bit(jzgc, JZ4770_GPIO_FLAG, irq, false);
- else if (jzgc->jzpc->info->version >= ID_JZ4740)
+ else if (is_soc_or_above(jzgc->jzpc, ID_JZ4740))
ingenic_gpio_set_bit(jzgc, JZ4740_GPIO_DATA, irq, true);
else
ingenic_gpio_set_bit(jzgc, JZ4730_GPIO_GPFR, irq, false);
@@ -3429,7 +3453,7 @@ static int ingenic_gpio_irq_set_type(struct irq_data *irqd, unsigned int type)
irq_set_handler_locked(irqd, handle_bad_irq);
}
- if ((type == IRQ_TYPE_EDGE_BOTH) && (jzgc->jzpc->info->version < ID_X2000)) {
+ if ((type == IRQ_TYPE_EDGE_BOTH) && !is_soc_or_above(jzgc->jzpc, ID_X2000)) {
/*
* The hardware does not support interrupts on both edges. The
* best we can do is to set up a single-edge interrupt and then
@@ -3461,9 +3485,9 @@ static void ingenic_gpio_irq_handler(struct irq_desc *desc)
chained_irq_enter(irq_chip, desc);
- if (jzgc->jzpc->info->version >= ID_JZ4770)
+ if (is_soc_or_above(jzgc->jzpc, ID_JZ4770))
flag = ingenic_gpio_read_reg(jzgc, JZ4770_GPIO_FLAG);
- else if (jzgc->jzpc->info->version >= ID_JZ4740)
+ else if (is_soc_or_above(jzgc->jzpc, ID_JZ4740))
flag = ingenic_gpio_read_reg(jzgc, JZ4740_GPIO_FLAG);
else
flag = ingenic_gpio_read_reg(jzgc, JZ4730_GPIO_GPFR);
@@ -3508,14 +3532,14 @@ static inline void ingenic_config_pin(struct ingenic_pinctrl *jzpc,
unsigned int offt = pin / PINS_PER_GPIO_CHIP;
if (set) {
- if (jzpc->info->version >= ID_JZ4740)
+ if (is_soc_or_above(jzpc, ID_JZ4740))
regmap_write(jzpc->map, offt * jzpc->info->reg_offset +
REG_SET(reg), BIT(idx));
else
regmap_set_bits(jzpc->map, offt * jzpc->info->reg_offset +
reg, BIT(idx));
} else {
- if (jzpc->info->version >= ID_JZ4740)
+ if (is_soc_or_above(jzpc, ID_JZ4740))
regmap_write(jzpc->map, offt * jzpc->info->reg_offset +
REG_CLEAR(reg), BIT(idx));
else
@@ -3574,12 +3598,12 @@ static int ingenic_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
struct ingenic_pinctrl *jzpc = jzgc->jzpc;
unsigned int pin = gc->base + offset;
- if (jzpc->info->version >= ID_JZ4770) {
+ if (is_soc_or_above(jzpc, ID_JZ4770)) {
if (ingenic_get_pin_config(jzpc, pin, JZ4770_GPIO_INT) ||
ingenic_get_pin_config(jzpc, pin, JZ4770_GPIO_PAT1))
return GPIO_LINE_DIRECTION_IN;
return GPIO_LINE_DIRECTION_OUT;
- } else if (jzpc->info->version == ID_JZ4730) {
+ } else if (!is_soc_or_above(jzpc, ID_JZ4740)) {
if (!ingenic_get_pin_config(jzpc, pin, JZ4730_GPIO_GPDIR))
return GPIO_LINE_DIRECTION_IN;
return GPIO_LINE_DIRECTION_OUT;
@@ -3630,18 +3654,18 @@ static int ingenic_pinmux_set_pin_fn(struct ingenic_pinctrl *jzpc,
dev_dbg(jzpc->dev, "set pin P%c%u to function %u\n",
'A' + offt, idx, func);
- if (jzpc->info->version >= ID_X1000) {
+ if (is_soc_or_above(jzpc, ID_X1000)) {
ingenic_shadow_config_pin(jzpc, pin, JZ4770_GPIO_INT, false);
ingenic_shadow_config_pin(jzpc, pin, GPIO_MSK, false);
ingenic_shadow_config_pin(jzpc, pin, JZ4770_GPIO_PAT1, func & 0x2);
ingenic_shadow_config_pin(jzpc, pin, JZ4770_GPIO_PAT0, func & 0x1);
ingenic_shadow_config_pin_load(jzpc, pin);
- } else if (jzpc->info->version >= ID_JZ4770) {
+ } else if (is_soc_or_above(jzpc, ID_JZ4770)) {
ingenic_config_pin(jzpc, pin, JZ4770_GPIO_INT, false);
ingenic_config_pin(jzpc, pin, GPIO_MSK, false);
ingenic_config_pin(jzpc, pin, JZ4770_GPIO_PAT1, func & 0x2);
ingenic_config_pin(jzpc, pin, JZ4770_GPIO_PAT0, func & 0x1);
- } else if (jzpc->info->version >= ID_JZ4740) {
+ } else if (is_soc_or_above(jzpc, ID_JZ4740)) {
ingenic_config_pin(jzpc, pin, JZ4740_GPIO_FUNC, true);
ingenic_config_pin(jzpc, pin, JZ4740_GPIO_TRIG, func & 0x2);
ingenic_config_pin(jzpc, pin, JZ4740_GPIO_SELECT, func & 0x1);
@@ -3699,16 +3723,16 @@ static int ingenic_pinmux_gpio_set_direction(struct pinctrl_dev *pctldev,
dev_dbg(pctldev->dev, "set pin P%c%u to %sput\n",
'A' + offt, idx, input ? "in" : "out");
- if (jzpc->info->version >= ID_X1000) {
+ if (is_soc_or_above(jzpc, ID_X1000)) {
ingenic_shadow_config_pin(jzpc, pin, JZ4770_GPIO_INT, false);
ingenic_shadow_config_pin(jzpc, pin, GPIO_MSK, true);
ingenic_shadow_config_pin(jzpc, pin, JZ4770_GPIO_PAT1, input);
ingenic_shadow_config_pin_load(jzpc, pin);
- } else if (jzpc->info->version >= ID_JZ4770) {
+ } else if (is_soc_or_above(jzpc, ID_JZ4770)) {
ingenic_config_pin(jzpc, pin, JZ4770_GPIO_INT, false);
ingenic_config_pin(jzpc, pin, GPIO_MSK, true);
ingenic_config_pin(jzpc, pin, JZ4770_GPIO_PAT1, input);
- } else if (jzpc->info->version >= ID_JZ4740) {
+ } else if (is_soc_or_above(jzpc, ID_JZ4740)) {
ingenic_config_pin(jzpc, pin, JZ4740_GPIO_SELECT, false);
ingenic_config_pin(jzpc, pin, JZ4740_GPIO_DIR, !input);
ingenic_config_pin(jzpc, pin, JZ4740_GPIO_FUNC, false);
@@ -3740,7 +3764,7 @@ static int ingenic_pinconf_get(struct pinctrl_dev *pctldev,
unsigned int bias, reg;
bool pull, pullup, pulldown;
- if (jzpc->info->version >= ID_X2000) {
+ if (is_soc_or_above(jzpc, ID_X2000)) {
pullup = ingenic_get_pin_config(jzpc, pin, X2000_GPIO_PEPU) &&
!ingenic_get_pin_config(jzpc, pin, X2000_GPIO_PEPD) &&
(jzpc->info->pull_ups[offt] & BIT(idx));
@@ -3748,7 +3772,7 @@ static int ingenic_pinconf_get(struct pinctrl_dev *pctldev,
!ingenic_get_pin_config(jzpc, pin, X2000_GPIO_PEPU) &&
(jzpc->info->pull_downs[offt] & BIT(idx));
- } else if (jzpc->info->version >= ID_X1830) {
+ } else if (is_soc_or_above(jzpc, ID_X1830)) {
unsigned int half = PINS_PER_GPIO_CHIP / 2;
unsigned int idxh = (pin % half) * 2;
@@ -3765,9 +3789,9 @@ static int ingenic_pinconf_get(struct pinctrl_dev *pctldev,
pulldown = (bias == GPIO_PULL_DOWN) && (jzpc->info->pull_downs[offt] & BIT(idx));
} else {
- if (jzpc->info->version >= ID_JZ4770)
+ if (is_soc_or_above(jzpc, ID_JZ4770))
pull = !ingenic_get_pin_config(jzpc, pin, JZ4770_GPIO_PEN);
- else if (jzpc->info->version >= ID_JZ4740)
+ else if (is_soc_or_above(jzpc, ID_JZ4740))
pull = !ingenic_get_pin_config(jzpc, pin, JZ4740_GPIO_PULL_DIS);
else
pull = ingenic_get_pin_config(jzpc, pin, JZ4730_GPIO_GPPUR);
@@ -3796,9 +3820,9 @@ static int ingenic_pinconf_get(struct pinctrl_dev *pctldev,
break;
case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
- if (jzpc->info->version >= ID_X2000)
+ if (is_soc_or_above(jzpc, ID_X2000))
reg = X2000_GPIO_SMT;
- else if (jzpc->info->version >= ID_X1830)
+ else if (is_soc_or_above(jzpc, ID_X1830))
reg = X1830_GPIO_SMT;
else
return -EINVAL;
@@ -3807,9 +3831,9 @@ static int ingenic_pinconf_get(struct pinctrl_dev *pctldev,
break;
case PIN_CONFIG_SLEW_RATE:
- if (jzpc->info->version >= ID_X2000)
+ if (is_soc_or_above(jzpc, ID_X2000))
reg = X2000_GPIO_SR;
- else if (jzpc->info->version >= ID_X1830)
+ else if (is_soc_or_above(jzpc, ID_X1830))
reg = X1830_GPIO_SR;
else
return -EINVAL;
@@ -3828,7 +3852,7 @@ static int ingenic_pinconf_get(struct pinctrl_dev *pctldev,
static void ingenic_set_bias(struct ingenic_pinctrl *jzpc,
unsigned int pin, unsigned int bias)
{
- if (jzpc->info->version >= ID_X2000) {
+ if (is_soc_or_above(jzpc, ID_X2000)) {
switch (bias) {
case GPIO_PULL_UP:
ingenic_config_pin(jzpc, pin, X2000_GPIO_PEPD, false);
@@ -3846,7 +3870,7 @@ static void ingenic_set_bias(struct ingenic_pinctrl *jzpc,
ingenic_config_pin(jzpc, pin, X2000_GPIO_PEPD, false);
}
- } else if (jzpc->info->version >= ID_X1830) {
+ } else if (is_soc_or_above(jzpc, ID_X1830)) {
unsigned int idx = pin % PINS_PER_GPIO_CHIP;
unsigned int half = PINS_PER_GPIO_CHIP / 2;
unsigned int idxh = (pin % half) * 2;
@@ -3864,9 +3888,9 @@ static void ingenic_set_bias(struct ingenic_pinctrl *jzpc,
REG_SET(X1830_GPIO_PEH), bias << idxh);
}
- } else if (jzpc->info->version >= ID_JZ4770) {
+ } else if (is_soc_or_above(jzpc, ID_JZ4770)) {
ingenic_config_pin(jzpc, pin, JZ4770_GPIO_PEN, !bias);
- } else if (jzpc->info->version >= ID_JZ4740) {
+ } else if (is_soc_or_above(jzpc, ID_JZ4740)) {
ingenic_config_pin(jzpc, pin, JZ4740_GPIO_PULL_DIS, !bias);
} else {
ingenic_config_pin(jzpc, pin, JZ4730_GPIO_GPPUR, bias);
@@ -3876,7 +3900,7 @@ static void ingenic_set_bias(struct ingenic_pinctrl *jzpc,
static void ingenic_set_schmitt_trigger(struct ingenic_pinctrl *jzpc,
unsigned int pin, bool enable)
{
- if (jzpc->info->version >= ID_X2000)
+ if (is_soc_or_above(jzpc, ID_X2000))
ingenic_config_pin(jzpc, pin, X2000_GPIO_SMT, enable);
else
ingenic_config_pin(jzpc, pin, X1830_GPIO_SMT, enable);
@@ -3885,9 +3909,9 @@ static void ingenic_set_schmitt_trigger(struct ingenic_pinctrl *jzpc,
static void ingenic_set_output_level(struct ingenic_pinctrl *jzpc,
unsigned int pin, bool high)
{
- if (jzpc->info->version >= ID_JZ4770)
+ if (is_soc_or_above(jzpc, ID_JZ4770))
ingenic_config_pin(jzpc, pin, JZ4770_GPIO_PAT0, high);
- else if (jzpc->info->version >= ID_JZ4740)
+ else if (is_soc_or_above(jzpc, ID_JZ4740))
ingenic_config_pin(jzpc, pin, JZ4740_GPIO_DATA, high);
else
ingenic_config_pin(jzpc, pin, JZ4730_GPIO_DATA, high);
@@ -3896,7 +3920,7 @@ static void ingenic_set_output_level(struct ingenic_pinctrl *jzpc,
static void ingenic_set_slew_rate(struct ingenic_pinctrl *jzpc,
unsigned int pin, unsigned int slew)
{
- if (jzpc->info->version >= ID_X2000)
+ if (is_soc_or_above(jzpc, ID_X2000))
ingenic_config_pin(jzpc, pin, X2000_GPIO_SR, slew);
else
ingenic_config_pin(jzpc, pin, X1830_GPIO_SR, slew);
@@ -3952,7 +3976,7 @@ static int ingenic_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
break;
case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
- if (jzpc->info->version < ID_X1830)
+ if (!is_soc_or_above(jzpc, ID_X1830))
return -EINVAL;
ingenic_set_schmitt_trigger(jzpc, pin, arg);
@@ -3967,7 +3991,7 @@ static int ingenic_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
break;
case PIN_CONFIG_SLEW_RATE:
- if (jzpc->info->version < ID_X1830)
+ if (!is_soc_or_above(jzpc, ID_X1830))
return -EINVAL;
ingenic_set_slew_rate(jzpc, pin, arg);
--
2.35.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] pinctrl: ingenic: Garbage-collect code paths for SoCs disabled by config
2022-03-15 17:39 [PATCH] pinctrl: ingenic: Garbage-collect code paths for SoCs disabled by config Paul Cercueil
@ 2022-03-16 14:47 ` Andy Shevchenko
2022-03-16 15:05 ` Paul Cercueil
0 siblings, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2022-03-16 14:47 UTC (permalink / raw)
To: Paul Cercueil
Cc: Linus Walleij, od, open list:BROADCOM NVRAM DRIVER,
open list:GPIO SUBSYSTEM, Linux Kernel Mailing List
On Wed, Mar 16, 2022 at 4:11 AM Paul Cercueil <paul@crapouillou.net> wrote:
>
> By being a bit smarter about how the SoC version checks are performed,
> it is possible to have all the code paths that correspond to SoCs
> disabled in the kernel config automatically marked as dead code by the
> compiler, and therefore garbage-collected.
>
> With this patch, when compiling a kernel that only targets the JZ4760
> for instance, the driver is now about 4.5 KiB smaller.
...
> +static const u32 enabled_socs =
If you make it unsigned long, it would be easier to switch to bitmap
APIs if needed in the future.
> + IS_ENABLED(CONFIG_MACH_JZ4730) << ID_JZ4730 |
> + IS_ENABLED(CONFIG_MACH_JZ4740) << ID_JZ4740 |
> + IS_ENABLED(CONFIG_MACH_JZ4725B) << ID_JZ4725B |
> + IS_ENABLED(CONFIG_MACH_JZ4750) << ID_JZ4750 |
> + IS_ENABLED(CONFIG_MACH_JZ4755) << ID_JZ4755 |
> + IS_ENABLED(CONFIG_MACH_JZ4760) << ID_JZ4760 |
> + IS_ENABLED(CONFIG_MACH_JZ4770) << ID_JZ4770 |
> + IS_ENABLED(CONFIG_MACH_JZ4775) << ID_JZ4775 |
> + IS_ENABLED(CONFIG_MACH_JZ4780) << ID_JZ4780 |
> + IS_ENABLED(CONFIG_MACH_X1000) << ID_X1000 |
> + IS_ENABLED(CONFIG_MACH_X1500) << ID_X1500 |
> + IS_ENABLED(CONFIG_MACH_X1830) << ID_X1830 |
> + IS_ENABLED(CONFIG_MACH_X2000) << ID_X2000 |
> + IS_ENABLED(CONFIG_MACH_X2100) << ID_X2100;
...
> +is_soc_or_above(const struct ingenic_pinctrl *jzpc, enum jz_version version)
> +{
> + return (enabled_socs >> version) &&
> + (!(enabled_socs & GENMASK((unsigned int)version - 1, 0))
Why casting? Why not use BIT()?
But these two lines seem like a very interesting way to reinvent the test_bit().
If I'm mistaken, this all version code needs a good comment.
> + || jzpc->info->version >= version);
> +}
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] pinctrl: ingenic: Garbage-collect code paths for SoCs disabled by config
2022-03-16 14:47 ` Andy Shevchenko
@ 2022-03-16 15:05 ` Paul Cercueil
2022-03-16 15:37 ` Andy Shevchenko
0 siblings, 1 reply; 7+ messages in thread
From: Paul Cercueil @ 2022-03-16 15:05 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Linus Walleij, od, BROADCOM NVRAM DRIVER
Hi Andy,
Le mer., mars 16 2022 at 16:47:24 +0200, Andy Shevchenko
<andy.shevchenko@gmail.com> a écrit :
> On Wed, Mar 16, 2022 at 4:11 AM Paul Cercueil <paul@crapouillou.net>
> wrote:
>>
>> By being a bit smarter about how the SoC version checks are
>> performed,
>> it is possible to have all the code paths that correspond to SoCs
>> disabled in the kernel config automatically marked as dead code by
>> the
>> compiler, and therefore garbage-collected.
>>
>> With this patch, when compiling a kernel that only targets the
>> JZ4760
>> for instance, the driver is now about 4.5 KiB smaller.
>
> ...
>
>> +static const u32 enabled_socs =
>
> If you make it unsigned long, it would be easier to switch to bitmap
> APIs if needed in the future.
Sure, I can make it a unsigned long. But we'd need a lot more entries
to justify a switch to bitmap APIs...
>
>> + IS_ENABLED(CONFIG_MACH_JZ4730) << ID_JZ4730 |
>> + IS_ENABLED(CONFIG_MACH_JZ4740) << ID_JZ4740 |
>> + IS_ENABLED(CONFIG_MACH_JZ4725B) << ID_JZ4725B |
>> + IS_ENABLED(CONFIG_MACH_JZ4750) << ID_JZ4750 |
>> + IS_ENABLED(CONFIG_MACH_JZ4755) << ID_JZ4755 |
>> + IS_ENABLED(CONFIG_MACH_JZ4760) << ID_JZ4760 |
>> + IS_ENABLED(CONFIG_MACH_JZ4770) << ID_JZ4770 |
>> + IS_ENABLED(CONFIG_MACH_JZ4775) << ID_JZ4775 |
>> + IS_ENABLED(CONFIG_MACH_JZ4780) << ID_JZ4780 |
>> + IS_ENABLED(CONFIG_MACH_X1000) << ID_X1000 |
>> + IS_ENABLED(CONFIG_MACH_X1500) << ID_X1500 |
>> + IS_ENABLED(CONFIG_MACH_X1830) << ID_X1830 |
>> + IS_ENABLED(CONFIG_MACH_X2000) << ID_X2000 |
>> + IS_ENABLED(CONFIG_MACH_X2100) << ID_X2100;
>
> ...
>
>> +is_soc_or_above(const struct ingenic_pinctrl *jzpc, enum
>> jz_version version)
>> +{
>> + return (enabled_socs >> version) &&
>> + (!(enabled_socs & GENMASK((unsigned int)version -
>> 1, 0))
>
> Why casting? Why not use BIT()?
Casting just to be explicit about what we're doing here - I don't like
doing arithmetic on enums. But I can certainly remove it.
> But these two lines seem like a very interesting way to reinvent the
> test_bit().
I don't use BIT() or test_bit() because I am not checking a bit, but a
mask:
- (enabled_socs >> version) checks that the config supports either
(version) or anything above;
- !(enabled_socs & GENMASK(version - 1, 0)) checks that the config does
not support anything below. If true, the actual version check can be
skipped and the operation is a compile-time constant, and GCC will trim
the dead branches accordingly.
Cheers,
-Paul
> If I'm mistaken, this all version code needs a good comment.
>
>> + || jzpc->info->version >= version);
>> +}
>
> --
> With Best Regards,
> Andy Shevchenko
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] pinctrl: ingenic: Garbage-collect code paths for SoCs disabled by config
2022-03-16 15:05 ` Paul Cercueil
@ 2022-03-16 15:37 ` Andy Shevchenko
2022-03-16 16:03 ` Paul Cercueil
0 siblings, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2022-03-16 15:37 UTC (permalink / raw)
To: Paul Cercueil; +Cc: Linus Walleij, od, BROADCOM NVRAM DRIVER
On Wed, Mar 16, 2022 at 5:05 PM Paul Cercueil <paul@crapouillou.net> wrote:
> Le mer., mars 16 2022 at 16:47:24 +0200, Andy Shevchenko
> <andy.shevchenko@gmail.com> a écrit :
> > On Wed, Mar 16, 2022 at 4:11 AM Paul Cercueil <paul@crapouillou.net>
> > wrote:
...
> >> +static const u32 enabled_socs =
> >
> > If you make it unsigned long, it would be easier to switch to bitmap
> > APIs if needed in the future.
>
> Sure, I can make it a unsigned long. But we'd need a lot more entries
> to justify a switch to bitmap APIs...
It was given below, i.e. using test_bit().
> >> + IS_ENABLED(CONFIG_MACH_JZ4730) << ID_JZ4730 |
> >> + IS_ENABLED(CONFIG_MACH_JZ4740) << ID_JZ4740 |
> >> + IS_ENABLED(CONFIG_MACH_JZ4725B) << ID_JZ4725B |
> >> + IS_ENABLED(CONFIG_MACH_JZ4750) << ID_JZ4750 |
> >> + IS_ENABLED(CONFIG_MACH_JZ4755) << ID_JZ4755 |
> >> + IS_ENABLED(CONFIG_MACH_JZ4760) << ID_JZ4760 |
> >> + IS_ENABLED(CONFIG_MACH_JZ4770) << ID_JZ4770 |
> >> + IS_ENABLED(CONFIG_MACH_JZ4775) << ID_JZ4775 |
> >> + IS_ENABLED(CONFIG_MACH_JZ4780) << ID_JZ4780 |
> >> + IS_ENABLED(CONFIG_MACH_X1000) << ID_X1000 |
> >> + IS_ENABLED(CONFIG_MACH_X1500) << ID_X1500 |
> >> + IS_ENABLED(CONFIG_MACH_X1830) << ID_X1830 |
> >> + IS_ENABLED(CONFIG_MACH_X2000) << ID_X2000 |
> >> + IS_ENABLED(CONFIG_MACH_X2100) << ID_X2100;
...
> >> +is_soc_or_above(const struct ingenic_pinctrl *jzpc, enum
> >> jz_version version)
> >> +{
> >> + return (enabled_socs >> version) &&
> >> + (!(enabled_socs & GENMASK((unsigned int)version -
> >> 1, 0))
> >
> > Why casting? Why not use BIT()?
>
> Casting just to be explicit about what we're doing here - I don't like
> doing arithmetic on enums. But I can certainly remove it.
>
> > But these two lines seem like a very interesting way to reinvent the
> > test_bit().
>
> I don't use BIT() or test_bit() because I am not checking a bit, but a
> mask:
> - (enabled_socs >> version) checks that the config supports either
> (version) or anything above;
> - !(enabled_socs & GENMASK(version - 1, 0)) checks that the config does
> not support anything below. If true, the actual version check can be
> skipped and the operation is a compile-time constant, and GCC will trim
> the dead branches accordingly.
You can still simplify the code, no? Calling ffs() (or similar, I
don't remember by heart all of the details) will give you the result
in one op. And it may also be optimized away by the compiler.
> > If I'm mistaken, this all version code needs a good comment.
> >
> >> + || jzpc->info->version >= version);
> >> +}
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] pinctrl: ingenic: Garbage-collect code paths for SoCs disabled by config
2022-03-16 15:37 ` Andy Shevchenko
@ 2022-03-16 16:03 ` Paul Cercueil
2022-03-16 16:20 ` Andy Shevchenko
0 siblings, 1 reply; 7+ messages in thread
From: Paul Cercueil @ 2022-03-16 16:03 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Linus Walleij, od, BROADCOM NVRAM DRIVER
Le mer., mars 16 2022 at 17:37:37 +0200, Andy Shevchenko
<andy.shevchenko@gmail.com> a écrit :
> On Wed, Mar 16, 2022 at 5:05 PM Paul Cercueil <paul@crapouillou.net>
> wrote:
>> Le mer., mars 16 2022 at 16:47:24 +0200, Andy Shevchenko
>> <andy.shevchenko@gmail.com> a écrit :
>> > On Wed, Mar 16, 2022 at 4:11 AM Paul Cercueil
>> <paul@crapouillou.net>
>> > wrote:
>
> ...
>
>> >> +static const u32 enabled_socs =
>> >
>> > If you make it unsigned long, it would be easier to switch to
>> bitmap
>> > APIs if needed in the future.
>>
>> Sure, I can make it a unsigned long. But we'd need a lot more
>> entries
>> to justify a switch to bitmap APIs...
>
> It was given below, i.e. using test_bit().
Again, I am not checking a bit, but a mask.
>> >> + IS_ENABLED(CONFIG_MACH_JZ4730) << ID_JZ4730 |
>> >> + IS_ENABLED(CONFIG_MACH_JZ4740) << ID_JZ4740 |
>> >> + IS_ENABLED(CONFIG_MACH_JZ4725B) << ID_JZ4725B |
>> >> + IS_ENABLED(CONFIG_MACH_JZ4750) << ID_JZ4750 |
>> >> + IS_ENABLED(CONFIG_MACH_JZ4755) << ID_JZ4755 |
>> >> + IS_ENABLED(CONFIG_MACH_JZ4760) << ID_JZ4760 |
>> >> + IS_ENABLED(CONFIG_MACH_JZ4770) << ID_JZ4770 |
>> >> + IS_ENABLED(CONFIG_MACH_JZ4775) << ID_JZ4775 |
>> >> + IS_ENABLED(CONFIG_MACH_JZ4780) << ID_JZ4780 |
>> >> + IS_ENABLED(CONFIG_MACH_X1000) << ID_X1000 |
>> >> + IS_ENABLED(CONFIG_MACH_X1500) << ID_X1500 |
>> >> + IS_ENABLED(CONFIG_MACH_X1830) << ID_X1830 |
>> >> + IS_ENABLED(CONFIG_MACH_X2000) << ID_X2000 |
>> >> + IS_ENABLED(CONFIG_MACH_X2100) << ID_X2100;
>
> ...
>
>> >> +is_soc_or_above(const struct ingenic_pinctrl *jzpc, enum
>> >> jz_version version)
>> >> +{
>> >> + return (enabled_socs >> version) &&
>> >> + (!(enabled_socs & GENMASK((unsigned int)version
>> -
>> >> 1, 0))
>> >
>> > Why casting? Why not use BIT()?
>>
>> Casting just to be explicit about what we're doing here - I don't
>> like
>> doing arithmetic on enums. But I can certainly remove it.
>>
>> > But these two lines seem like a very interesting way to reinvent
>> the
>> > test_bit().
>>
>> I don't use BIT() or test_bit() because I am not checking a bit,
>> but a
>> mask:
>> - (enabled_socs >> version) checks that the config supports either
>> (version) or anything above;
>> - !(enabled_socs & GENMASK(version - 1, 0)) checks that the config
>> does
>> not support anything below. If true, the actual version check can be
>> skipped and the operation is a compile-time constant, and GCC will
>> trim
>> the dead branches accordingly.
>
> You can still simplify the code, no? Calling ffs() (or similar, I
> don't remember by heart all of the details) will give you the result
> in one op. And it may also be optimized away by the compiler.
ffs() gives me the first bit set, this is absolutely not what I want.
Let's say that my kernel supports the JZ4750, JZ4755, JZ4760 SoCs.
That means that I have enabled_socs == 0x38.
Calling is_soc_or_above(jzpc, ID_JZ4740) would resolve to:
(0x38 >> 1) && (!(0x38 & GENMASK(0, 0)) || jzpc->info->version >=
version);
== 1 && (1 || jzpc->info->version >= version)
Which is a compile-time constant equal to 1.
Calling is_soc_or_above(jzpc, ID_JZ4755) would resolve to:
(0x38 >> 4) && (!(0x38 & GENMASK(3, 0)) || jzpc->info->version >=
version);
== 1 && (0 || jzpc->info->version >= version)
which is not a compile-time constant, so the jzpc->info->version must
be checked.
Calling is_soc_or_above(jzpc, ID_JZ4770) would resolve to:
(0x38 >> 6) && (!(0x38 & GENMASK(5, 0)) || jzpc->info->version >=
version);
== 0 && ...
which is a compile-time constant equal to 0.
Cheers,
-Paul
>> > If I'm mistaken, this all version code needs a good comment.
>> >
>> >> + || jzpc->info->version >= version);
>> >> +}
>
> --
> With Best Regards,
> Andy Shevchenko
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] pinctrl: ingenic: Garbage-collect code paths for SoCs disabled by config
2022-03-16 16:03 ` Paul Cercueil
@ 2022-03-16 16:20 ` Andy Shevchenko
2022-03-16 16:45 ` Paul Cercueil
0 siblings, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2022-03-16 16:20 UTC (permalink / raw)
To: Paul Cercueil; +Cc: Linus Walleij, od, BROADCOM NVRAM DRIVER
On Wed, Mar 16, 2022 at 6:03 PM Paul Cercueil <paul@crapouillou.net> wrote:
> Le mer., mars 16 2022 at 17:37:37 +0200, Andy Shevchenko
> <andy.shevchenko@gmail.com> a écrit :
> > On Wed, Mar 16, 2022 at 5:05 PM Paul Cercueil <paul@crapouillou.net>
> > wrote:
> >> Le mer., mars 16 2022 at 16:47:24 +0200, Andy Shevchenko
> >> <andy.shevchenko@gmail.com> a écrit :
> >> > On Wed, Mar 16, 2022 at 4:11 AM Paul Cercueil
> >> <paul@crapouillou.net>
> >> > wrote:
...
> > It was given below, i.e. using test_bit().
>
> Again, I am not checking a bit, but a mask.
Yes, that's why "was".
...
> >> >> + return (enabled_socs >> version) &&
> >> >> + (!(enabled_socs & GENMASK((unsigned int)version
> >> -
> >> >> 1, 0))
> >> >
> >> > Why casting? Why not use BIT()?
> >>
> >> Casting just to be explicit about what we're doing here - I don't
> >> like
> >> doing arithmetic on enums. But I can certainly remove it.
> >>
> >> > But these two lines seem like a very interesting way to reinvent
> >> the
> >> > test_bit().
> >>
> >> I don't use BIT() or test_bit() because I am not checking a bit,
> >> but a
> >> mask:
> >> - (enabled_socs >> version) checks that the config supports either
> >> (version) or anything above;
> >> - !(enabled_socs & GENMASK(version - 1, 0)) checks that the config
> >> does
> >> not support anything below. If true, the actual version check can be
> >> skipped and the operation is a compile-time constant, and GCC will
> >> trim
> >> the dead branches accordingly.
> >
> > You can still simplify the code, no? Calling ffs() (or similar, I
> > don't remember by heart all of the details) will give you the result
> > in one op. And it may also be optimized away by the compiler.
>
> ffs() gives me the first bit set, this is absolutely not what I want.
>
> Let's say that my kernel supports the JZ4750, JZ4755, JZ4760 SoCs.
> That means that I have enabled_socs == 0x38.
>
> Calling is_soc_or_above(jzpc, ID_JZ4740) would resolve to:
> (0x38 >> 1) && (!(0x38 & GENMASK(0, 0)) || jzpc->info->version >=
> version);
> == 1 && (1 || jzpc->info->version >= version)
>
> Which is a compile-time constant equal to 1.
>
> Calling is_soc_or_above(jzpc, ID_JZ4755) would resolve to:
> (0x38 >> 4) && (!(0x38 & GENMASK(3, 0)) || jzpc->info->version >=
> version);
> == 1 && (0 || jzpc->info->version >= version)
>
> which is not a compile-time constant, so the jzpc->info->version must
> be checked.
>
> Calling is_soc_or_above(jzpc, ID_JZ4770) would resolve to:
> (0x38 >> 6) && (!(0x38 & GENMASK(5, 0)) || jzpc->info->version >=
> version);
> == 0 && ...
>
> which is a compile-time constant equal to 0.
And what's wrong with
ffs(enabled_socs) >= BIT(version)
?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] pinctrl: ingenic: Garbage-collect code paths for SoCs disabled by config
2022-03-16 16:20 ` Andy Shevchenko
@ 2022-03-16 16:45 ` Paul Cercueil
0 siblings, 0 replies; 7+ messages in thread
From: Paul Cercueil @ 2022-03-16 16:45 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Linus Walleij, od, BROADCOM NVRAM DRIVER
Le mer., mars 16 2022 at 18:20:27 +0200, Andy Shevchenko
<andy.shevchenko@gmail.com> a écrit :
> On Wed, Mar 16, 2022 at 6:03 PM Paul Cercueil <paul@crapouillou.net>
> wrote:
>> Le mer., mars 16 2022 at 17:37:37 +0200, Andy Shevchenko
>> <andy.shevchenko@gmail.com> a écrit :
>> > On Wed, Mar 16, 2022 at 5:05 PM Paul Cercueil
>> <paul@crapouillou.net>
>> > wrote:
>> >> Le mer., mars 16 2022 at 16:47:24 +0200, Andy Shevchenko
>> >> <andy.shevchenko@gmail.com> a écrit :
>> >> > On Wed, Mar 16, 2022 at 4:11 AM Paul Cercueil
>> >> <paul@crapouillou.net>
>> >> > wrote:
>
> ...
>
>> > It was given below, i.e. using test_bit().
>>
>> Again, I am not checking a bit, but a mask.
>
> Yes, that's why "was".
>
> ...
>
>> >> >> + return (enabled_socs >> version) &&
>> >> >> + (!(enabled_socs & GENMASK((unsigned
>> int)version
>> >> -
>> >> >> 1, 0))
>> >> >
>> >> > Why casting? Why not use BIT()?
>> >>
>> >> Casting just to be explicit about what we're doing here - I
>> don't
>> >> like
>> >> doing arithmetic on enums. But I can certainly remove it.
>> >>
>> >> > But these two lines seem like a very interesting way to
>> reinvent
>> >> the
>> >> > test_bit().
>> >>
>> >> I don't use BIT() or test_bit() because I am not checking a bit,
>> >> but a
>> >> mask:
>> >> - (enabled_socs >> version) checks that the config supports
>> either
>> >> (version) or anything above;
>> >> - !(enabled_socs & GENMASK(version - 1, 0)) checks that the
>> config
>> >> does
>> >> not support anything below. If true, the actual version check
>> can be
>> >> skipped and the operation is a compile-time constant, and GCC
>> will
>> >> trim
>> >> the dead branches accordingly.
>> >
>> > You can still simplify the code, no? Calling ffs() (or similar, I
>> > don't remember by heart all of the details) will give you the
>> result
>> > in one op. And it may also be optimized away by the compiler.
>>
>> ffs() gives me the first bit set, this is absolutely not what I
>> want.
>>
>> Let's say that my kernel supports the JZ4750, JZ4755, JZ4760 SoCs.
>> That means that I have enabled_socs == 0x38.
>>
>> Calling is_soc_or_above(jzpc, ID_JZ4740) would resolve to:
>> (0x38 >> 1) && (!(0x38 & GENMASK(0, 0)) || jzpc->info->version >=
>> version);
>> == 1 && (1 || jzpc->info->version >= version)
>>
>> Which is a compile-time constant equal to 1.
>>
>> Calling is_soc_or_above(jzpc, ID_JZ4755) would resolve to:
>> (0x38 >> 4) && (!(0x38 & GENMASK(3, 0)) || jzpc->info->version >=
>> version);
>> == 1 && (0 || jzpc->info->version >= version)
>>
>> which is not a compile-time constant, so the jzpc->info->version
>> must
>> be checked.
>>
>> Calling is_soc_or_above(jzpc, ID_JZ4770) would resolve to:
>> (0x38 >> 6) && (!(0x38 & GENMASK(5, 0)) || jzpc->info->version >=
>> version);
>> == 0 && ...
>>
>> which is a compile-time constant equal to 0.
>
> And what's wrong with
>
> ffs(enabled_socs) >= BIT(version)
>
> ?
Well, for starters you're comparing a bit position [0:31] with a bit
mask (1 << [0:31])
;)
I believe you meant:
ffs(enabled_socs) >= version
With my former example, the first case (ID_JZ4740) would give you a
compile-time constant of 1, so it works.
The second case (ID_JZ4755) would cause the jzpc->info->version to be
checked, which works too.
The third case (ID_JZ4770) however, would also cause the
jzpc->info->version to be checked, which is not the wanted behaviour.
Since in my example we only support up to JZ4760, this should instead
resolve to a compile-time constant 0.
Cheers,
-Paul
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-03-16 16:45 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-15 17:39 [PATCH] pinctrl: ingenic: Garbage-collect code paths for SoCs disabled by config Paul Cercueil
2022-03-16 14:47 ` Andy Shevchenko
2022-03-16 15:05 ` Paul Cercueil
2022-03-16 15:37 ` Andy Shevchenko
2022-03-16 16:03 ` Paul Cercueil
2022-03-16 16:20 ` Andy Shevchenko
2022-03-16 16:45 ` Paul Cercueil
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.