[PATCH v2] pinctrl: single: add big endian support

[PATCH v2] pinctrl: single: add big endian support

[Rustam Adilov]

Add support for big endian pin controllers by taking into account
a big-endian device tree property.

This is needed on big endian MIPS platforms like RTL930X, RTL931X
and RTL9607C who use pinctrl-single, because when SWAP_IO_SPACE is
enabled, the mmio accessors in pinctrl-single perform a byte swap.

Signed-off-by: Rustam Adilov <adilov@disroot.org>
---
Change in v2:
- Change to generic big endian io read/write functions.

 drivers/pinctrl/pinctrl-single.c | 41 ++++++++++++++++++++++++++++----
 1 file changed, 37 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index e0eb7240a985..cdfe38f259b0 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -265,6 +265,26 @@ static void pcs_writel(unsigned int val, void __iomem *reg)
 	writel(val, reg);
 }
 
+static unsigned int pcs_readw_be(void __iomem *reg)
+{
+	return ioread16be(reg);
+}
+
+static unsigned int pcs_readl_be(void __iomem *reg)
+{
+	return ioread32be(reg);
+}
+
+static void pcs_writew_be(unsigned int val, void __iomem *reg)
+{
+	iowrite16be(val, reg);
+}
+
+static void pcs_writel_be(unsigned int val, void __iomem *reg)
+{
+	iowrite32be(val, reg);
+}
+
 static unsigned int pcs_pin_reg_offset_get(struct pcs_device *pcs,
 					   unsigned int pin)
 {
@@ -1785,6 +1805,7 @@ static int pcs_probe(struct platform_device *pdev)
 	struct resource *res;
 	struct pcs_device *pcs;
 	const struct pcs_soc_data *soc;
+	bool byte_be;
 	int ret;
 
 	soc = of_device_get_match_data(&pdev->dev);
@@ -1860,18 +1881,30 @@ static int pcs_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, pcs);
 
+	byte_be = device_is_big_endian(pcs->dev);
+
 	switch (pcs->width) {
 	case 8:
 		pcs->read = pcs_readb;
 		pcs->write = pcs_writeb;
 		break;
 	case 16:
-		pcs->read = pcs_readw;
-		pcs->write = pcs_writew;
+		if (byte_be) {
+			pcs->read = pcs_readw_be;
+			pcs->write = pcs_writew_be;
+		} else {
+			pcs->read = pcs_readw;
+			pcs->write = pcs_writew;
+		}
 		break;
 	case 32:
-		pcs->read = pcs_readl;
-		pcs->write = pcs_writel;
+		if (byte_be) {
+			pcs->read = pcs_readl_be;
+			pcs->write = pcs_writel_be;
+		} else {
+			pcs->read = pcs_readl;
+			pcs->write = pcs_writel;
+		}
 		break;
 	default:
 		break;
-- 
2.56.0

Re: [PATCH v2] pinctrl: single: add big endian support

[Linus Walleij]

On Sat, Oct 10, 2026 at 7:57 PM Rustam Adilov <adilov@disroot.org> wrote:

> Add support for big endian pin controllers by taking into account
> a big-endian device tree property.
>
> This is needed on big endian MIPS platforms like RTL930X, RTL931X
> and RTL9607C who use pinctrl-single, because when SWAP_IO_SPACE is
> enabled, the mmio accessors in pinctrl-single perform a byte swap.
>
> Signed-off-by: Rustam Adilov <adilov@disroot.org>

I'm not so sure about expanding the usage of pinctrl-single, we have
said that this is used in the cases when you want to stash all the
hardware data into the device tree (such as which groups and functions
that exist) and it seems mostly done to save memory for the kernel
so you don't need to compile in that data for all systems.

The argument that this is "simple" if you just have some hardware
data in a file is moot these days IMO, because you can now just
give that data to an LLM and it will spit out any group and function
definitions you want.

So I'd like to hear some of the reasoning about why you want to
do this thing, we have other drivers doing this but without using
pinctrl-single.

One of the driver maintainers have to ACK it too.

> +static unsigned int pcs_readw_be(void __iomem *reg)
> +{
> +       return ioread16be(reg);
> +}
> +
> +static unsigned int pcs_readl_be(void __iomem *reg)
> +{
> +       return ioread32be(reg);
> +}
> +
> +static void pcs_writew_be(unsigned int val, void __iomem *reg)
> +{
> +       iowrite16be(val, reg);
> +}
> +
> +static void pcs_writel_be(unsigned int val, void __iomem *reg)
> +{
> +       iowrite32be(val, reg);
> +}

Why this indirection? Can't you just assign the standard
accessors directly?

>         case 16:
> -               pcs->read = pcs_readw;
> -               pcs->write = pcs_writew;
> +               if (byte_be) {
> +                       pcs->read = pcs_readw_be;
> +                       pcs->write = pcs_writew_be;
> +               } else {
> +                       pcs->read = pcs_readw;
> +                       pcs->write = pcs_writew;

Maybe they should all do that actually, can't we rewrite
this so we just:

if (byte_be) {
  pcs->read = ioread16be;
  pcs->write = iowrite16be;
} else {
  pcs->read = ioread16;
  pcs->write = iowrite16;

I don't see how the wrappers help and I hope the compiler
just optimize them away anyway.

Yours,
Linus Walleij

Re: [PATCH v2] pinctrl: single: add big endian support

[Rustam Adilov]

On 2026-10-11 20:14, Linus Walleij wrote:
> On Sat, Oct 10, 2026 at 7:57 PM Rustam Adilov <adilov@disroot.org> wrote:
> 
>> Add support for big endian pin controllers by taking into account
>> a big-endian device tree property.
>>
>> This is needed on big endian MIPS platforms like RTL930X, RTL931X
>> and RTL9607C who use pinctrl-single, because when SWAP_IO_SPACE is
>> enabled, the mmio accessors in pinctrl-single perform a byte swap.
>>
>> Signed-off-by: Rustam Adilov <adilov@disroot.org>
> 
> I'm not so sure about expanding the usage of pinctrl-single, we have
> said that this is used in the cases when you want to stash all the
> hardware data into the device tree (such as which groups and functions
> that exist) and it seems mostly done to save memory for the kernel
> so you don't need to compile in that data for all systems.
> 
> The argument that this is "simple" if you just have some hardware
> data in a file is moot these days IMO, because you can now just
> give that data to an LLM and it will spit out any group and function
> definitions you want.
> 
> So I'd like to hear some of the reasoning about why you want to
> do this thing, we have other drivers doing this but without using
> pinctrl-single.

Um, what? I don't understand how that is related to anything with my
commit message.

In MIPS context, SWAP_IO_SPACE is only about [1].

[1] - https://elixir.bootlin.com/linux/v7.2.9/source/arch/mips/include/asm/mach-generic/mangle-port.h#L28

> One of the driver maintainers have to ACK it too.
> 
>> +static unsigned int pcs_readw_be(void __iomem *reg)
>> +{
>> +       return ioread16be(reg);
>> +}
>> +
>> +static unsigned int pcs_readl_be(void __iomem *reg)
>> +{
>> +       return ioread32be(reg);
>> +}
>> +
>> +static void pcs_writew_be(unsigned int val, void __iomem *reg)
>> +{
>> +       iowrite16be(val, reg);
>> +}
>> +
>> +static void pcs_writel_be(unsigned int val, void __iomem *reg)
>> +{
>> +       iowrite32be(val, reg);
>> +}
> 
> Why this indirection? Can't you just assign the standard
> accessors directly?

To mirror the normal pcs_read/write? And yes we can just as easily
assign them to io functions directly.

>>         case 16:
>> -               pcs->read = pcs_readw;
>> -               pcs->write = pcs_writew;
>> +               if (byte_be) {
>> +                       pcs->read = pcs_readw_be;
>> +                       pcs->write = pcs_writew_be;
>> +               } else {
>> +                       pcs->read = pcs_readw;
>> +                       pcs->write = pcs_writew;
> 
> Maybe they should all do that actually, can't we rewrite
> this so we just:
> 
> if (byte_be) {
>   pcs->read = ioread16be;
>   pcs->write = iowrite16be;
> } else {
>   pcs->read = ioread16;
>   pcs->write = iowrite16;
> 
> I don't see how the wrappers help and I hope the compiler
> just optimize them away anyway.

A little out of scope but i am cool with a extending this patch to
a 2 part series to do that.

Best,
Rustam