Message-ID: <20220704104214.2d9cd1a0@donnerap.cambridge.arm.com>
Date:   Mon, 4 Jul 2022 10:42:14 +0100
From:   Andre Przywara <andre.przywara@....com>
To:     Samuel Holland <samuel@...lland.org>
Cc:     Chen-Yu Tsai <wens@...e.org>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Maxime Ripard <mripard@...nel.org>, Ondrej Jirman <x@....cz>,
        Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-gpio@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-sunxi@...ts.linux.dev,
        Heiko Stuebner <heiko@...ech.de>
Subject: Re: [PATCH 6/6] pinctrl: sunxi: Add driver for Allwinner D1/D1s

On Sat, 2 Jul 2022 10:43:05 -0500
Samuel Holland <samuel@...lland.org> wrote:

Hi Samuel,

> On 7/2/22 9:47 AM, Andre Przywara wrote:
> > On Sat, 25 Jun 2022 21:11:47 -0500
> > Samuel Holland <samuel@...lland.org> wrote:
> > 
> > Hi Samuel,
> >   
> >> These SoCs contain a pinctrl with a new register layout. Use the variant
> >> parameter to set the right register offsets. This pinctrl also increases
> >> the number of functions per pin from 8 to 16, taking advantage of all 4
> >> bits in the mux config field (so far, only functions 0-8 and 14-15 are
> >> used). This increases the maximum possible number of functions.
> >>
> >> D1s is a low pin count version of the D1 SoC, with some pins omitted.
> >> The remaining pins have the same function assignments as D1.  
> > 
> > So do we actually need this extra variant, if there are no conflicts?
> > The D1s seems to be a simple subset of the D1. I think we followed the
> > same approach for the H616 already, where there are more pins in the
> > pinctrl driver than the manual describes, and which are used in other
> > package variants, like the T507.
> > In case of the H616, those pins are there, you can program them (which
> > is not the case for not implemented pins otherwise), they are just not
> > connected to the package.
> > I would expect a DT to never reference them, and even if, it doesn't do
> > any harm other than just not working.  
> 
> I am following the example of V3/V3s here, so it seems we are inconsistent on
> this point. I needed to supply one variant for the register layout anyway, so I
> though I might as well be "accurate".
> 
> But with Allwinner releasing lots of packages per die, it is probably overkill
> to have a separate compatible per packge. As you note, there is no harm in
> configuring pins that do not map to any pad.

Yes, thanks, that current inflation of SoC variants is what I was
concerned about.

> Some notes for completeness:
>  - D1 documents all three JTAG functions (ARM, RISC-V, and DSP), although the
> ARM JTAG does not work.
>  - D1s/F133 only documents the RISC-V JTAG function.
>  - T113 only documents the ARM and DSP JTAG functions.
>  - T113 adds a CAN function on mux 8 of PB2-PB5. The CAN controller accidentally
> made it in to one version of the D1 datasheet, so it may unofficially exist there.

Have you checked whether the CAN peripheral registers are there, on the D1?

One more thing below:

> None of these variations are conflicting.
> 
> > For the table below: I checked every pin against the D1 manual (yes,
> > that took an hour), and found only one small issue and some nits in
> > PortE, see inline.
> >   
> >> Signed-off-by: Samuel Holland <samuel@...lland.org>
> >> ---
> >>
> >>  drivers/pinctrl/sunxi/Kconfig             |   5 +
> >>  drivers/pinctrl/sunxi/Makefile            |   1 +
> >>  drivers/pinctrl/sunxi/pinctrl-sun20i-d1.c | 860 ++++++++++++++++++++++
> >>  drivers/pinctrl/sunxi/pinctrl-sunxi.c     |  16 +-
> >>  drivers/pinctrl/sunxi/pinctrl-sunxi.h     |   7 +
> >>  5 files changed, 884 insertions(+), 5 deletions(-)
> >>  create mode 100644 drivers/pinctrl/sunxi/pinctrl-sun20i-d1.c
> >>
> >> diff --git a/drivers/pinctrl/sunxi/Kconfig b/drivers/pinctrl/sunxi/Kconfig
> >> index 33751a6a0757..a6ac1c1f2585 100644
> >> --- a/drivers/pinctrl/sunxi/Kconfig
> >> +++ b/drivers/pinctrl/sunxi/Kconfig
> >> @@ -84,6 +84,11 @@ config PINCTRL_SUN9I_A80_R
> >>  	depends on RESET_CONTROLLER
> >>  	select PINCTRL_SUNXI
> >>  
> >> +config PINCTRL_SUN20I_D1
> >> +	bool "Support for the Allwinner D1 PIO"
> >> +	default RISCV && ARCH_SUNXI

Is there any chance you can add the "MACH_SUN8I || " already?
Not that it is really surprising, but I explicitly compared the pin mux
overview tables of the R528(ARMv7) and D1(RISC-V) manuals, and they are
identical. So we just use the D1 compatible string as a fallback for the
R528 and T113 parts, in which case the driver should be build for ARM, too.

Cheers,
Andre

> >> +	select PINCTRL_SUNXI
> >> +
> >>  config PINCTRL_SUN50I_A64
> >>  	bool "Support for the Allwinner A64 PIO"
> >>  	default ARM64 && ARCH_SUNXI
> >> diff --git a/drivers/pinctrl/sunxi/Makefile b/drivers/pinctrl/sunxi/Makefile
> >> index d3440c42b9d6..2ff5a55927ad 100644
> >> --- a/drivers/pinctrl/sunxi/Makefile
> >> +++ b/drivers/pinctrl/sunxi/Makefile
> >> @@ -20,6 +20,7 @@ obj-$(CONFIG_PINCTRL_SUN8I_A83T_R)	+= pinctrl-sun8i-a83t-r.o
> >>  obj-$(CONFIG_PINCTRL_SUN8I_H3)		+= pinctrl-sun8i-h3.o
> >>  obj-$(CONFIG_PINCTRL_SUN8I_H3_R)	+= pinctrl-sun8i-h3-r.o
> >>  obj-$(CONFIG_PINCTRL_SUN8I_V3S)		+= pinctrl-sun8i-v3s.o
> >> +obj-$(CONFIG_PINCTRL_SUN20I_D1)		+= pinctrl-sun20i-d1.o
> >>  obj-$(CONFIG_PINCTRL_SUN50I_H5)		+= pinctrl-sun50i-h5.o
> >>  obj-$(CONFIG_PINCTRL_SUN50I_H6)		+= pinctrl-sun50i-h6.o
> >>  obj-$(CONFIG_PINCTRL_SUN50I_H6_R)	+= pinctrl-sun50i-h6-r.o
> >> diff --git a/drivers/pinctrl/sunxi/pinctrl-sun20i-d1.c b/drivers/pinctrl/sunxi/pinctrl-sun20i-d1.c
> >> new file mode 100644
> >> index 000000000000..7247c1f1d92c
> >> --- /dev/null
> >> +++ b/drivers/pinctrl/sunxi/pinctrl-sun20i-d1.c
> >> @@ -0,0 +1,860 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Allwinner D1 SoC pinctrl driver.
> >> + *
> >> + * Copyright (c) 2020 wuyan@...winnertech.com
> >> + * Copyright (c) 2021-2022 Samuel Holland <samuel@...lland.org>
> >> + */
> >> +
> >> +#include <linux/module.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_device.h>
> >> +#include <linux/pinctrl/pinctrl.h>
> >> +
> >> +#include "pinctrl-sunxi.h"
> >> +
> >> +static const struct sunxi_desc_pin d1_pins[] = {
> >> +	/* PB */
> >> +	SUNXI_PIN_VARIANT(SUNXI_PINCTRL_PIN(B, 0),
> >> +		PINCTRL_SUN20I_D1,
> >> +		SUNXI_FUNCTION(0x0, "gpio_in"),
> >> +		SUNXI_FUNCTION(0x1, "gpio_out"),
> >> +		SUNXI_FUNCTION(0x2, "pwm"),  
> > 
> > The manual mentions the PWM channel number in the pin name, and it
> > seems like in other pinctrl drivers we use the number either in the
> > function name, or at least in the comment.
> > Shall we do one of them here as well?  
> 
> I originally had the numbers in the function name, but then I realized that no
> pin has multiple PWM muxes, so I removed them. As you mention, other drivers
> have them, so I will add them back.
> 
> > And the mux numbers for pwm are all over the place, so lets hope we
> > never need pwm in U-Boot ;-)  
> 
> PWM is used for the CPU voltage regulator on at least one board (Nezha), but I
> think we can get away without U-Boot support for that. And including the PWM
> number in the function name will improve things somewhat.
> 
> I will fix the typos you noted below.
> 
> Regards,
> Samuel

Powered by blists - more mailing lists