On 7/2/22 9:47 AM, Andre Przywara wrote:
> On Sat, 25 Jun 2022 21:11:47 -0500
> Samuel Holland <samuel@xxxxxxxxxxxx> 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.
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.
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@xxxxxxxxxxxx>
>> ---
>>
>> 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
>> + 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@xxxxxxxxxxxxxxxxx
>> + * Copyright (c) 2021-2022 Samuel Holland <samuel@xxxxxxxxxxxx>
>> + */
>> +
>> +#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
