| Message ID | 87zgipgu3s.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | pinctrl: renesas: r8a779g0: Add pins, groups and functions | expand |
Hi Morimoto-san, On Tue, 7 Jun 2022, Kuninori Morimoto wrote: > From: LUU HOAI <hoai.luu.ub@renesas.com> > > This patch adds initial pinctrl support for the R8A779G0 (V4H) SoC, > including bias, drive strength and voltage control. > > [Morimoto merged Kihara-san's MODSEL8 fixup patch, > and cleanuped white space, care reserved bit on each configs, > fixup setting miss] > Signed-off-by: LUU HOAI <hoai.luu.ub@renesas.com> > Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> Thanks for your patch! Below is a first set of comments. You can expect a second set of comments after the weekend. > --- /dev/null > +++ b/drivers/pinctrl/renesas/pfc-r8a779g0.c > @@ -0,0 +1,2467 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * R8A779A0 processor support - PFC hardware block. > + * > + * Copyright (C) 2021 Renesas Electronics Corp. > + * > + * This file is based on the drivers/pinctrl/renesas/pfc-r8a779a0.c > + */ > + > +#include <linux/errno.h> > +#include <linux/io.h> > +#include <linux/kernel.h> > + > +#include "core.h" There is no need to include "core.h". > +#include "sh_pfc.h" > + > +#define CFG_FLAGS (SH_PFC_PIN_CFG_DRIVE_STRENGTH | SH_PFC_PIN_CFG_PULL_UP_DOWN) > + > +#define CPU_ALL_GP(fn, sfx) \ > + PORT_GP_CFG_19(0, fn, sfx, CFG_FLAGS | SH_PFC_PIN_CFG_IO_VOLTAGE_18_33), \ > + PORT_GP_CFG_29(1, fn, sfx, CFG_FLAGS | SH_PFC_PIN_CFG_IO_VOLTAGE_18_33), \ GP1_23 to GP1_28 do not support voltage control. > + PORT_GP_CFG_20(2, fn, sfx, CFG_FLAGS), \ > + PORT_GP_CFG_13(3, fn, sfx, CFG_FLAGS | SH_PFC_PIN_CFG_IO_VOLTAGE_18_33), \ > + PORT_GP_CFG_1(3, 13, fn, sfx, CFG_FLAGS), \ > + PORT_GP_CFG_1(3, 14, fn, sfx, CFG_FLAGS), \ > + PORT_GP_CFG_1(3, 15, fn, sfx, CFG_FLAGS), \ > + PORT_GP_CFG_1(3, 16, fn, sfx, CFG_FLAGS), \ > + PORT_GP_CFG_1(3, 17, fn, sfx, CFG_FLAGS), \ > + PORT_GP_CFG_1(3, 18, fn, sfx, CFG_FLAGS), \ > + PORT_GP_CFG_1(3, 19, fn, sfx, CFG_FLAGS), \ > + PORT_GP_CFG_1(3, 20, fn, sfx, CFG_FLAGS), \ > + PORT_GP_CFG_1(3, 21, fn, sfx, CFG_FLAGS), \ > + PORT_GP_CFG_1(3, 22, fn, sfx, CFG_FLAGS), \ > + PORT_GP_CFG_1(3, 23, fn, sfx, CFG_FLAGS), \ > + PORT_GP_CFG_1(3, 24, fn, sfx, CFG_FLAGS), \ > + PORT_GP_CFG_1(3, 25, fn, sfx, CFG_FLAGS), \ > + PORT_GP_CFG_1(3, 26, fn, sfx, CFG_FLAGS), \ > + PORT_GP_CFG_1(3, 27, fn, sfx, CFG_FLAGS), \ > + PORT_GP_CFG_1(3, 28, fn, sfx, CFG_FLAGS), \ > + PORT_GP_CFG_1(3, 29, fn, sfx, CFG_FLAGS), \ > + PORT_GP_CFG_25(4, fn, sfx, CFG_FLAGS), \ > + PORT_GP_CFG_21(5, fn, sfx, CFG_FLAGS), \ > + PORT_GP_CFG_21(6, fn, sfx, CFG_FLAGS), \ > + PORT_GP_CFG_21(7, fn, sfx, CFG_FLAGS), \ > + PORT_GP_CFG_14(8, fn, sfx, CFG_FLAGS | SH_PFC_PIN_CFG_IO_VOLTAGE_18_33) > + > +#define CPU_ALL_NOGP(fn) \ > + PIN_NOGP_CFG(PRESETOUT_N, "PRESETOUT#", fn, SH_PFC_PIN_CFG_PULL_UP_DOWN), \ As the PUDSYS register is not documented, I cannot verify that this pin supports both pull-up and pull-down. > + PIN_NOGP_CFG(PRESETOUT0_N, "PRESETOUT0#", fn, SH_PFC_PIN_CFG_PULL_DOWN), \ > + PIN_NOGP_CFG(PRESETOUT1_N, "PRESETOUT1#", fn, SH_PFC_PIN_CFG_PULL_DOWN), \ "PRESETOUT0#" and "PRESETOUT1#" do not seem to exist. > + PIN_NOGP_CFG(EXTALR, "EXTALR", fn, SH_PFC_PIN_CFG_PULL_UP_DOWN), \ As the PUDSYS register is not documented, I cannot verify that this pin supports both pull-up and pull-down. > + PIN_NOGP_CFG(DCUTRST0_N, "DCUTRST0#", fn, SH_PFC_PIN_CFG_PULL_DOWN), \ DCUTRST_N_LPDRST_N and "DCUTRST#/LPDRST#"? > + PIN_NOGP_CFG(DCUTCK0, "DCUTCK0", fn, SH_PFC_PIN_CFG_PULL_UP), \ DCUTCK_LPDCLK and "DCUTCK/LPDCLK"? > + PIN_NOGP_CFG(DCUTMS0, "DCUTMS0", fn, SH_PFC_PIN_CFG_PULL_UP), \ DCUTMS and "DCUTMS"? > + PIN_NOGP_CFG(DCUTDI0, "DCUTDI0", fn, SH_PFC_PIN_CFG_PULL_UP), \ DCUTDI_LPDI and "DCUTDI/LPDI"? > + PIN_NOGP_CFG(DCUTRST1_N, "DCUTRST1#", fn, SH_PFC_PIN_CFG_PULL_DOWN), \ > + PIN_NOGP_CFG(DCUTCK1, "DCUTCK1", fn, SH_PFC_PIN_CFG_PULL_UP), \ > + PIN_NOGP_CFG(DCUTMS1, "DCUTMS1", fn, SH_PFC_PIN_CFG_PULL_UP), \ > + PIN_NOGP_CFG(DCUTDI1, "DCUTDI1", fn, SH_PFC_PIN_CFG_PULL_UP), \ > + PIN_NOGP_CFG(EVTI_N, "EVTI#", fn, SH_PFC_PIN_CFG_PULL_UP), \ > + PIN_NOGP_CFG(MSYN_N, "MSYN#", fn, SH_PFC_PIN_CFG_PULL_UP) The above 6 pins do not seem to exist. Regardless of the above, pull up/down support for the NOGP pins is not supported yet, as this driver does not use PINMUX_NOGP_ALL(), and it does not have definitions for the PUENSYS register. Perhaps the CPU_ALL_NOGP() macro should just be dropped for now? > +/* SR0 */ > +/* IP0SR0 */ /* 0 */ /* 1 */ /* 2 */ /* 3 4 5 6 7 8 9 A B C D E F */ > +#define IP0SR0_3_0 F_(0, 0) FM(ERROROUTC) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) ^^^^^^^^ Missing TCLK2_A > +#define IP0SR0_7_4 F_(0, 0) FM(MSIOF3_SS1) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP0SR0_11_8 F_(0, 0) FM(MSIOF3_SS2) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP0SR0_15_12 FM(IRQ3) FM(MSIOF3_SCK) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP0SR0_19_16 FM(IRQ2) FM(MSIOF3_TXD) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP0SR0_23_20 FM(IRQ1) FM(MSIOF3_RXD) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP0SR0_27_24 FM(IRQ0) FM(MSIOF3_SYNC) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP0SR0_31_28 FM(MSIOF5_SS2) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > + > +/* IP1SR0 */ /* 0 */ /* 1 */ /* 2 */ /* 3 4 5 6 7 8 9 A B C D E F */ > +#define IP1SR0_3_0 FM(MSIOF5_SS1) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP1SR0_7_4 FM(MSIOF5_SYNC) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP1SR0_11_8 FM(MSIOF5_TXD) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP1SR0_15_12 FM(MSIOF5_SCK) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP1SR0_19_16 FM(MSIOF5_RXD) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP1SR0_23_20 FM(MSIOF2_SS2) FM(TCLK1) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) ^^^^^^^^ Missing IRQ2_A > +#define IP1SR0_27_24 FM(MSIOF2_SS1) FM(HTX1) FM(TX1) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP1SR0_31_28 FM(MSIOF2_SYNC) FM(HRX1) FM(RX1) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > + > +/* IP2SR0 */ /* 0 */ /* 1 */ /* 2 */ /* 3 4 5 6 7 8 9 A B C D E F */ > +#define IP2SR0_3_0 FM(MSIOF2_TXD) FM(HCTS1_N) FM(CTS1_N) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP2SR0_7_4 FM(MSIOF2_SCK) FM(HRTS1_N) FM(RTS1_N) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP2SR0_11_8 FM(MSIOF2_RXD) FM(HSCK1) FM(SCK1) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP2SR0_15_12 F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP2SR0_19_16 F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP2SR0_23_20 F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP2SR0_27_24 F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP2SR0_31_28 F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) Due to the recently-introduced reserved field handling, if the definition of an IPxSRy_n_m() macro consists of a series of "F_(0, 0)" only, you can just omit that definition, and omit its use in the definition of the PINMUX_IPSR() macro below. This applies to several other defitions below. > + > +/* SR1 */ > +/* IP0SR1 */ /* 0 */ /* 1 */ /* 2 */ /* 3 4 5 6 7 8 9 A B C D E F */ > +#define IP0SR1_3_0 FM(MSIOF1_SS2) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP0SR1_7_4 FM(MSIOF1_SS1) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP0SR1_11_8 FM(MSIOF1_SYNC) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP0SR1_15_12 FM(MSIOF1_SCK) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP0SR1_19_16 FM(MSIOF1_TXD) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) ^^^^^^^^ ^^^^^^^^ Missing alternative set "A" for the five HSCIF3 signals Missing primary set of five SCIF3 signals > +#define IP0SR1_23_20 FM(MSIOF1_RXD) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP0SR1_27_24 FM(MSIOF0_SS2) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP0SR1_31_28 FM(MSIOF0_SS1) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > + > +/* IP1SR1 */ /* 0 */ /* 1 */ /* 2 */ /* 3 4 5 6 7 8 9 A B C D E F */ > +#define IP1SR1_3_0 FM(MSIOF0_SYNC) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP1SR1_7_4 FM(MSIOF0_TXD) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) ^^^^^^^^ Missing CANFD5_[TR]X_B > +#define IP1SR1_11_8 FM(MSIOF0_SCK) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) ^^^^^^^^ ^^^^^^^^ Missing sets for the five HSCIF1 and five SCIF1 signals. For both, the set name is not documented, so we do not know if this is the primary or an alternative set. > +#define IP1SR1_15_12 FM(MSIOF0_RXD) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP1SR1_19_16 FM(HTX0) FM(TX0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP1SR1_23_20 FM(HCTS0_N) FM(CTS0_N) FM(PWM8) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) ^^^^ PWM8_A > +#define IP1SR1_27_24 FM(HRTS0_N) FM(RTS0_N) FM(PWM9) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) ^^^^ PWM9_A > +#define IP1SR1_31_28 FM(HSCK0) FM(SCK0) FM(PWM0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) ^^^^ PWM0_A > + > +/* IP2SR1 */ /* 0 */ /* 1 */ /* 2 */ /* 3 4 5 6 7 8 9 A B C D E F */ > +#define IP2SR1_3_0 FM(HRX0) FM(RX0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP2SR1_7_4 FM(SCIF_CLK) FM(IRQ4) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) ^^^^ IRQ4_A > +#define IP2SR1_11_8 FM(SSI_SCK) FM(TCLK3) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP2SR1_15_12 FM(SSI_WS) FM(TCLK4) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP2SR1_19_16 FM(SSI_SD) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP2SR1_23_20 FM(AUDIO_CLKOUT) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) ^^^^^^^^ Missing IRQ0_A and IRQ1_A > +#define IP2SR1_27_24 FM(AUDIO_CLKIN) FM(PWM3) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) ^^^^ PWM3_A > +#define IP2SR1_31_28 F_(0, 0) FM(TCLK2) FM(MSIOF4_SS1) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) ^^^^^^^^ Missing IRQ3_B > + > +/* IP3SR1 */ /* 0 */ /* 1 */ /* 2 */ /* 3 4 5 6 7 8 9 A B C D E F */ > +#define IP3SR1_3_0 FM(HRX3) FM(SCK3) FM(MSIOF4_SS2) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP3SR1_7_4 FM(HSCK3) FM(CTS3_N) FM(MSIOF4_SCK) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP3SR1_11_8 FM(HRTS3_N) FM(RTS3_N) FM(MSIOF4_TXD) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) ^^^^^^^^ Missing TPU0TO[01]_A > +#define IP3SR1_15_12 FM(HCTS3_N) FM(RX3) FM(MSIOF4_RXD) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP3SR1_19_16 FM(HTX3) FM(TX3) FM(MSIOF4_SYNC) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) ^^^^^^^^ According to the pin function spreadsheet, thse 5 pins are actually the "A" set for SCIF3 > +#define IP3SR1_23_20 F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP3SR1_27_24 F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP3SR1_31_28 F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > + > +/* SR2 */ > +/* IP0SR2 */ /* 0 */ /* 1 */ /* 2 */ /* 3 4 5 6 7 8 9 A B C D E F */ > +#define IP0SR2_3_0 FM(FXR_TXDA) FM(CANFD1_TX) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) ^^^^^^^^ Missing TPU0TO2_A > +#define IP0SR2_7_4 FM(FXR_TXENA_N) FM(CANFD1_RX) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) ^^^^^^^^ Missing TPU0TO3_A > +#define IP0SR2_11_8 FM(RXDA_EXTFXR) FM(CANFD5_TX) FM(IRQ5) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP0SR2_15_12 FM(CLK_EXTFXR) FM(CANFD5_RX) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) ^^^^^^^^ Missing IRQ4_B > +#define IP0SR2_19_16 FM(RXDB_EXTFXR) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP0SR2_23_20 FM(FXR_TXENB_N) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP0SR2_27_24 FM(FXR_TXDB) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP0SR2_31_28 FM(TPU0TO1) FM(CANFD6_TX) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) ^^^^^^^^ Missing TCLK2_B > + > +/* IP1SR2 */ /* 0 */ /* 1 */ /* 2 */ /* 3 4 5 6 7 8 9 A B C D E F */ > +#define IP1SR2_3_0 FM(TPU0TO0) FM(CANFD6_RX) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) ^^^^^^^^ Missing TCLK1_A > +#define IP1SR2_7_4 FM(CAN_CLK) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP1SR2_11_8 FM(CANFD0_TX) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) ^^^^^^^^ Missing set (undocumented name) for FXR_TXEN[AB]_N. > +#define IP1SR2_15_12 FM(CANFD0_RX) FM(STPWT_EXTFXR) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP1SR2_19_16 FM(CANFD2_TX) FM(TPU0TO2) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) ^^^^^^^^ Missing TCLK3_A > +#define IP1SR2_23_20 FM(CANFD2_RX) FM(TPU0TO3) FM(PWM1) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) ^^^^ ^^^^^^^^ PWM1_B Missing TCLK4_A > +#define IP1SR2_27_24 FM(CANFD3_TX) F_(0, 0) FM(PWM2) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) ^^^^ PWM2_B > +#define IP1SR2_31_28 FM(CANFD3_RX) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) ^^^^^^^^ Missing PWM3_B > + > +/* IP2SR2 */ /* 0 */ /* 1 */ /* 2 */ /* 3 4 5 6 7 8 9 A B C D E F */ > +#define IP2SR2_3_0 FM(CANFD4_TX) F_(0, 0) FM(PWM4) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP2SR2_7_4 FM(CANFD4_RX) F_(0, 0) FM(PWM5) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP2SR2_11_8 FM(CANFD7_TX) F_(0, 0) FM(PWM6) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP2SR2_15_12 FM(CANFD7_RX) F_(0, 0) FM(PWM7) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP2SR2_19_16 F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP2SR2_23_20 F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP2SR2_27_24 F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP2SR2_31_28 F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > + > +/* SR3 */ > +/* IP0SR3 */ /* 0 */ /* 1 */ /* 2 */ /* 3 4 5 6 7 8 9 A B C D E F */ > +#define IP0SR3_3_0 FM(MMC_SD_D1) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP0SR3_7_4 FM(MMC_SD_D0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP0SR3_11_8 FM(MMC_SD_D2) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP0SR3_15_12 FM(MMC_SD_CLK) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP0SR3_19_16 FM(MMC_DS) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP0SR3_23_20 FM(MMC_SD_D3) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP0SR3_27_24 FM(MMC_D5) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP0SR3_31_28 FM(MMC_D4) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > + > +/* IP1SR3 */ /* 0 */ /* 1 */ /* 2 */ /* 3 4 5 6 7 8 9 A B C D E F */ > +#define IP1SR3_3_0 FM(MMC_D7) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP1SR3_7_4 FM(MMC_D6) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP1SR3_11_8 FM(MMC_SD_CMD) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP1SR3_15_12 FM(SD_CD) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP1SR3_19_16 FM(SD_WP) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > +#define IP1SR3_23_20 FM(IPC_CLKIN) FM(IPC_CLKEN_IN) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) ^^^^^^^^ ^^^^^^^^ Missing PWM1_A and TCLK3 (unnamed, but conflicting with IP2SR1_11_8) > +#define IP1SR3_27_24 FM(IPC_CLKOUT) FM(IPC_CLKEN_OUT) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) ^^^^^^^^ ^^^^^^^^ Missing ERROROUTC_A and TCLK4 (unnamed, but conflicting with IP2SR1_15_12) > +#define IP1SR3_31_28 FM(QSPI0_SSL) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) > + { PINMUX_DRIVE_REG("DRV0CTRL2", 0xE6058080) { > + { RCAR_GP_PIN(2, 7), 28, 3 }, /* TPU0TO1 */ > + { RCAR_GP_PIN(2, 6), 24, 3 }, /* FXR_TXDB */ > + { RCAR_GP_PIN(2, 5), 20, 3 }, /* FXR_TXENB_N */ > + { RCAR_GP_PIN(2, 4), 16, 3 }, /* RXDB_EXTFXR */ > + { RCAR_GP_PIN(2, 3), 12, 3 }, /* CLK_EXTFXR */ > + { RCAR_GP_PIN(2, 2), 8, 3 }, /* RXDA_EXTFXR */ > + { RCAR_GP_PIN(2, 1), 4, 3 }, /* XR_TXENA_N */ FXR_TXENA_N > + { RCAR_GP_PIN(2, 0), 0, 3 }, /* FXR_TXDA */ > + } }, > + { PINMUX_DRIVE_REG("DRV2CTRL2", 0xE6058088) { > + { RCAR_GP_PIN(2, 19), 12, 3 }, /* CANFD7_RX DRV1 */ > + { RCAR_GP_PIN(2, 18), 8, 3 }, /* CANFD7_TX DRV1 */ > + { RCAR_GP_PIN(2, 17), 4, 3 }, /* CANFD4_RX DRV1 */ > + { RCAR_GP_PIN(2, 16), 0, 3 }, /* CANFD4_TX DRV1 */ Please drop "DRV1" from the above four comments. > + } }, > + { PINMUX_DRIVE_REG("DRV3CTRL3", 0xE605888C) { > + { RCAR_GP_PIN(3, 29), 20, 2 }, /* RPC_INT_N */ > + { RCAR_GP_PIN(3, 28), 16, 2 }, /* RPC_WP_N */ > + { RCAR_GP_PIN(3, 27), 12, 2 }, /* RPC_RESET */ RPC_RESET_N > + { RCAR_GP_PIN(3, 26), 8, 2 }, /* QSPI1_IO3 */ > + { RCAR_GP_PIN(3, 25), 4, 2 }, /* QSPI1_SSL */ > + { RCAR_GP_PIN(3, 24), 0, 2 }, /* QSPI1_IO2 */ > + } }, > +enum ioctrl_regs { > + POC0, > + POC1, > + POC2, POC 2 is unused, and not documented. Please remove it. > + POC3, > + POC4, > + POC5, > + POC6, > + POC7, > + POC8, > + TD0SEL3, > +}; > + > +static const struct pinmux_ioctrl_reg pinmux_ioctrl_regs[] = { > + [POC0] = { 0xE60500A0, }, > + [POC1] = { 0xE60508A0, }, > + [POC2] = { 0xE60580A0, }, POC 2 is unused, and not documented. Please remove it. > + [POC3] = { 0xE60588A0, }, > + [POC4] = { 0xE60600A0, }, > + [POC5] = { 0xE60608A0, }, > + [POC6] = { 0xE60610A0, }, > + [POC7] = { 0xE60618A0, }, > + [POC8] = { 0xE60680A0, }, > + [TD0SEL3] = { 0xE60589C0, }, TD0SEL3 is not used. As all fields must be written all zeroes, there is no point in keeping it here. > + { /* sentinel */ }, > +}; > + > +static int r8a779g0_pin_to_pocctrl(unsigned int pin, u32 *pocctrl) > +{ > + int bit = pin & 0x1f; > + > + *pocctrl = pinmux_ioctrl_regs[POC0].reg; > + if (pin >= RCAR_GP_PIN(0, 0) && pin <= RCAR_GP_PIN(0, 18)) > + return bit; > + > + *pocctrl = pinmux_ioctrl_regs[POC1].reg; > + if (pin >= RCAR_GP_PIN(1, 0) && pin <= RCAR_GP_PIN(1, 28)) "<= RCAR_GP_PIN(1, 22)", as GP1_23 to GP1_28 do not support voltage control. > + return bit; > + > + *pocctrl = pinmux_ioctrl_regs[POC3].reg; > + if (pin >= RCAR_GP_PIN(3, 0) && pin <= RCAR_GP_PIN(3, 12)) > + return bit; We probably want to add voltage control for TSN0 and AVB[0-2] someday... > + > + *pocctrl = pinmux_ioctrl_regs[POC8].reg; > + if (pin >= RCAR_GP_PIN(8, 0) && pin <= RCAR_GP_PIN(8, 13)) > + return bit; > + > + return -EINVAL; > +} > + { PINMUX_BIAS_REG("PUEN2", 0xE60580C0, "PUD2", 0xE60580E0) { > + [ 0] = RCAR_GP_PIN(2, 0), /* FXR_TXDA */ > + [ 1] = RCAR_GP_PIN(2, 1), /* FXR_TXENA_N */ > + [ 2] = RCAR_GP_PIN(2, 2), /* RXDA_EXTFXR */ > + [ 3] = RCAR_GP_PIN(2, 3), /* CLK_EXTFXR */ > + [ 4] = RCAR_GP_PIN(2, 4), /* RXDB_EXTFXR */ > + [ 5] = RCAR_GP_PIN(2, 5), /* FXR_TXENB_N */ > + [ 6] = RCAR_GP_PIN(2, 6), /* FXR_TXDB */ > + [ 7] = RCAR_GP_PIN(2, 7), /* TPU0TO0 */ TPU0TO1 > + [ 8] = RCAR_GP_PIN(2, 8), /* TPU0TO1 */ TPU0TO0 > + [ 9] = RCAR_GP_PIN(2, 9), /* CAN_CLK */ > + [10] = RCAR_GP_PIN(2, 10), /* CANFD0_TX */ > + [11] = RCAR_GP_PIN(2, 11), /* CANFD0_RX */ > + [12] = RCAR_GP_PIN(2, 12), /* CANFD2_TX */ > + [13] = RCAR_GP_PIN(2, 13), /* CANFD2_RX */ > + [14] = RCAR_GP_PIN(2, 14), /* CANFD3_TX */ > + [15] = RCAR_GP_PIN(2, 15), /* CANFD3_RX */ > + [16] = RCAR_GP_PIN(2, 16), /* CANFD4_TX */ > + [17] = RCAR_GP_PIN(2, 17), /* CANFD4_RX */ > + [18] = RCAR_GP_PIN(2, 18), /* CANFD7_TX */ > + [19] = RCAR_GP_PIN(2, 19), /* CANFD7_RX */ > + [20] = SH_PFC_PIN_NONE, > + [21] = SH_PFC_PIN_NONE, > + [22] = SH_PFC_PIN_NONE, > + [23] = SH_PFC_PIN_NONE, > + [24] = SH_PFC_PIN_NONE, > + [25] = SH_PFC_PIN_NONE, > + [26] = SH_PFC_PIN_NONE, > + [27] = SH_PFC_PIN_NONE, > + [28] = SH_PFC_PIN_NONE, > + [29] = SH_PFC_PIN_NONE, > + [30] = SH_PFC_PIN_NONE, > + [31] = SH_PFC_PIN_NONE, > + } }, > +static const struct sh_pfc_soc_operations pinmux_ops = { Please rename this to r8a779g0_pfc_ops, to avoid confusion with struct pinmux_ops. Cfr. commit c614d12c4bc003a7 ("pinctrl: renesas: Rename sh_pfc_soc_operations instances"). > + .pin_to_pocctrl = r8a779g0_pin_to_pocctrl, > + .get_bias = rcar_pinmux_get_bias, > + .set_bias = rcar_pinmux_set_bias, > +}; Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert > > From: LUU HOAI <hoai.luu.ub@renesas.com> > > > > This patch adds initial pinctrl support for the R8A779G0 (V4H) SoC, > > including bias, drive strength and voltage control. > > > > [Morimoto merged Kihara-san's MODSEL8 fixup patch, > > and cleanuped white space, care reserved bit on each configs, > > fixup setting miss] > > Signed-off-by: LUU HOAI <hoai.luu.ub@renesas.com> > > Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com> > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > Thanks for your patch! > > Below is a first set of comments. > You can expect a second set of comments after the weekend. May I suggest ? v1 patch was almost as-is of working implementation, I did was just cleanup. v2 patch was tidyup implementation which found via runtime. So far, I think we can say "v2 patch is cleanup version of working implementation". But, v3 needs too many tidyups, and it is related to very deep parts. I'm very afraid it breaks working implementation for some reasons. In such case, it is very difficult to find and/or fix the issue because PFC file is too big and too complicated. I'm happy to fix all of you pointed, but I want to make it as additional patch. But what do you think ? Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Morimoto-san, On Tue, 7 Jun 2022, Kuninori Morimoto wrote: > From: LUU HOAI <hoai.luu.ub@renesas.com> > > This patch adds initial pinctrl support for the R8A779G0 (V4H) SoC, > including bias, drive strength and voltage control. > > [Morimoto merged Kihara-san's MODSEL8 fixup patch, > and cleanuped white space, care reserved bit on each configs, > fixup setting miss] > Signed-off-by: LUU HOAI <hoai.luu.ub@renesas.com> > Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> Thanks for your patch! Below is my second (and final) set of comments. > --- /dev/null > +++ b/drivers/pinctrl/renesas/pfc-r8a779g0.c > +/* MOD_SEL4 */ /* 0 */ /* 1 */ > +#define MOD_SEL4_19 FM(SEL_TSN0_TD2_0) FM(SEL_TSN0_TD2_1) > +#define MOD_SEL4_18 FM(SEL_TSN0_TD3_0) FM(SEL_TSN0_TD3_1) > +#define MOD_SEL4_17 F_(0, 0) F_(0, 0) > +#define MOD_SEL4_16 F_(0, 0) F_(0, 0) > +#define MOD_SEL4_15 FM(SEL_TSN0_TD0_0) FM(SEL_TSN0_TD0_1) > +#define MOD_SEL4_14 FM(SEL_TSN0_TD1_0) FM(SEL_TSN0_TD1_1) > +#define MOD_SEL4_13 F_(0, 0) F_(0, 0) > +#define MOD_SEL4_12 FM(SEL_TSN0_TXC_0) FM(SEL_TSN0_TXC_1) > +#define MOD_SEL4_11 F_(0, 0) F_(0, 0) > +#define MOD_SEL4_10 F_(0, 0) F_(0, 0) > +#define MOD_SEL4_9 FM(SEL_TSN0_TX_CTL_0) FM(SEL_TSN0_TX_CTL_1) > +#define MOD_SEL4_8 FM(SEL_TSN0_AVTP_PPS0_0) FM(SEL_TSN0_AVTP_PPS0_1) > +#define MOD_SEL4_7 F_(0, 0) F_(0, 0) > +#define MOD_SEL4_6 F_(0, 0) F_(0, 0) > +#define MOD_SEL4_5 FM(SEL_TSN0_AVTP_MATCH_0) FM(SEL_TSN0_AVTP_MATCH_1) > +#define MOD_SEL4_4 F_(0, 0) F_(0, 0) > +#define MOD_SEL4_3 F_(0, 0) F_(0, 0) > +#define MOD_SEL4_2 FM(SEL_TSN0_AVTP_PPS1_0) FM(SEL_TSN0_AVTP_PPS1_1) > +#define MOD_SEL4_1 FM(SEL_TSN0_MDC_0) FM(SEL_TSN0_MDC_1) > +#define MOD_SEL4_0 F_(0, 0) F_(0, 0) Like for the IPxSRy_n_m() macros, if the definition of a MOD_SELn_m() macro consists of a series of "F_(0, 0)" only, you can just omit that definition, and omit its use in the definition of the PINMUX_MOD_SELS() macro below. Note that you already omitted MOD_SEL4_n for n > 19. > +static const u16 pinmux_data[] = { > + PINMUX_DATA_GP_ALL(), Given the inset for I2C selection in Figure 7.1 ("PFC Block Diagram"), and the documentation for the MODSEL8 register bits in Rev. 0.51 of the R-Car V4H Series Hardware User’s Manual, I think you need to override GP_8_[0-12]_FN here to use the GPIO function on I2C-capable pins. See also commits 4288caed9a6319b7 ("pinctrl: renesas: r8a779a0: Fix GPIO function on I2C-capable pins") and 8bdd369dba7ff2f8 ("pinctrl: renesas: r8a779f0: Fix GPIO function on I2C-capable pins"). And probably you need something similar to configure MODSEL[4567] when using TSN0 or AVB[012] pins as GPIOs, or when using CC5_OSCOUT? > + > + PINMUX_SINGLE(AVS1), > + PINMUX_SINGLE(AVS0), > + PINMUX_SINGLE(PCIE1_CLKREQ_N), > + PINMUX_SINGLE(PCIE0_CLKREQ_N), > + PINMUX_SINGLE(TSN0_TXCREFCLK), > + PINMUX_SINGLE(TSN0_TD2), As using TSN0_{TD[0-3],TXC,TX_CTL,AVTP_PPS[01],AVTP_MATCH,MDC} needs configuration in MODSEL4, you should use PINMUX_IPSR_NOGM() instead. E.g. PINMUX_IPSR_NOGM(0, TSN0_TD2, SEL_TSN0_TD2_1). > + PINMUX_SINGLE(TSN0_TD3), > + PINMUX_SINGLE(TSN0_RD2), > + PINMUX_SINGLE(TSN0_RD3), > + PINMUX_SINGLE(TSN0_TD0), > + PINMUX_SINGLE(TSN0_TD1), > + PINMUX_SINGLE(TSN0_RD1), > + PINMUX_SINGLE(TSN0_TXC), > + PINMUX_SINGLE(TSN0_RXC), > + PINMUX_SINGLE(TSN0_RD0), > + PINMUX_SINGLE(TSN0_TX_CTL), > + PINMUX_SINGLE(TSN0_AVTP_PPS0), > + PINMUX_SINGLE(TSN0_RX_CTL), > + PINMUX_SINGLE(TSN0_AVTP_CAPTURE), > + PINMUX_SINGLE(TSN0_AVTP_MATCH), > + PINMUX_SINGLE(TSN0_LINK), > + PINMUX_SINGLE(TSN0_PHY_INT), > + PINMUX_SINGLE(TSN0_AVTP_PPS1), > + PINMUX_SINGLE(TSN0_MDC), > + PINMUX_SINGLE(TSN0_MDIO), > + > + PINMUX_SINGLE(AVB2_RX_CTL), > + PINMUX_SINGLE(AVB2_TX_CTL), As using AVB2_{TX_CTL,TXC,TD[0-3],MDC,MAGIC,AVTP_MATCH,AVTP_CAPTURE} needs configuration in MODSEL5, you should use PINMUX_IPSR_NOGM() instead. E.g. PINMUX_IPSR_NOGM(0, AVB2_TX_CTL, SEL_AVB2_TX_CTL_1). > + PINMUX_SINGLE(AVB2_RXC), > + PINMUX_SINGLE(AVB2_RD0), > + PINMUX_SINGLE(AVB2_TXC), > + PINMUX_SINGLE(AVB2_TD0), > + PINMUX_SINGLE(AVB2_RD1), > + PINMUX_SINGLE(AVB2_RD2), > + PINMUX_SINGLE(AVB2_TD1), > + PINMUX_SINGLE(AVB2_TD2), > + PINMUX_SINGLE(AVB2_MDIO), > + PINMUX_SINGLE(AVB2_RD3), > + PINMUX_SINGLE(AVB2_TD3), > + PINMUX_SINGLE(AVB2_TXCREFCLK), > + PINMUX_SINGLE(AVB2_MDC), > + PINMUX_SINGLE(AVB2_MAGIC), > + PINMUX_SINGLE(AVB2_PHY_INT), > + PINMUX_SINGLE(AVB2_LINK), > + PINMUX_SINGLE(AVB2_AVTP_MATCH), > + PINMUX_SINGLE(AVB2_AVTP_CAPTURE), > + PINMUX_SINGLE(AVB2_AVTP_PPS), > + > + /* IP0SR0 */ > + PINMUX_IPSR_GPSR(IP0SR0_3_0, ERROROUTC), Missing TCLK2_A > + > + PINMUX_IPSR_GPSR(IP0SR0_7_4, MSIOF3_SS1), > + > + PINMUX_IPSR_GPSR(IP0SR0_11_8, MSIOF3_SS2), > + > + PINMUX_IPSR_GPSR(IP0SR0_15_12, IRQ3), > + PINMUX_IPSR_GPSR(IP0SR0_15_12, MSIOF3_SCK), > + > + PINMUX_IPSR_GPSR(IP0SR0_19_16, IRQ2), > + PINMUX_IPSR_GPSR(IP0SR0_19_16, MSIOF3_TXD), > + > + PINMUX_IPSR_GPSR(IP0SR0_23_20, IRQ1), > + PINMUX_IPSR_GPSR(IP0SR0_23_20, MSIOF3_RXD), > + > + PINMUX_IPSR_GPSR(IP0SR0_27_24, IRQ0), > + PINMUX_IPSR_GPSR(IP0SR0_27_24, MSIOF3_SYNC), > + > + PINMUX_IPSR_GPSR(IP0SR0_31_28, MSIOF5_SS2), > + > + /* IP1SR0 */ > + PINMUX_IPSR_GPSR(IP1SR0_3_0, MSIOF5_SS1), > + > + PINMUX_IPSR_GPSR(IP1SR0_7_4, MSIOF5_SYNC), > + > + PINMUX_IPSR_GPSR(IP1SR0_11_8, MSIOF5_TXD), > + > + PINMUX_IPSR_GPSR(IP1SR0_15_12, MSIOF5_SCK), > + > + PINMUX_IPSR_GPSR(IP1SR0_19_16, MSIOF5_RXD), > + > + PINMUX_IPSR_GPSR(IP1SR0_23_20, MSIOF2_SS2), > + PINMUX_IPSR_GPSR(IP1SR0_23_20, TCLK1), Missing IRQ2_A > + > + PINMUX_IPSR_GPSR(IP1SR0_27_24, MSIOF2_SS1), > + PINMUX_IPSR_GPSR(IP1SR0_27_24, HTX1), > + PINMUX_IPSR_GPSR(IP1SR0_27_24, TX1), > + > + PINMUX_IPSR_GPSR(IP1SR0_31_28, MSIOF2_SYNC), > + PINMUX_IPSR_GPSR(IP1SR0_31_28, HRX1), > + PINMUX_IPSR_GPSR(IP1SR0_31_28, RX1), > + > + /* IP2SR0 */ > + PINMUX_IPSR_GPSR(IP2SR0_3_0, MSIOF2_TXD), > + PINMUX_IPSR_GPSR(IP2SR0_3_0, HCTS1_N), > + PINMUX_IPSR_GPSR(IP2SR0_3_0, CTS1_N), > + > + PINMUX_IPSR_GPSR(IP2SR0_7_4, MSIOF2_SCK), > + PINMUX_IPSR_GPSR(IP2SR0_7_4, HRTS1_N), > + PINMUX_IPSR_GPSR(IP2SR0_7_4, RTS1_N), > + > + PINMUX_IPSR_GPSR(IP2SR0_11_8, MSIOF2_RXD), > + PINMUX_IPSR_GPSR(IP2SR0_11_8, HSCK1), > + PINMUX_IPSR_GPSR(IP2SR0_11_8, SCK1), > + > + /* IP0SR1 */ > + PINMUX_IPSR_GPSR(IP0SR1_3_0, MSIOF1_SS2), Missing HTX3_A and TX3_A > + PINMUX_IPSR_GPSR(IP0SR1_7_4, MSIOF1_SS1), Missing HCTS3_N_A and RX3_A > + PINMUX_IPSR_GPSR(IP0SR1_11_8, MSIOF1_SYNC), Missing RTS3_N_A and RTS3_N > + PINMUX_IPSR_GPSR(IP0SR1_15_12, MSIOF1_SCK), Missing HSCK3_A and CTS3_N > + PINMUX_IPSR_GPSR(IP0SR1_19_16, MSIOF1_TXD), Missing HRX3_A and SCK3 > + PINMUX_IPSR_GPSR(IP0SR1_23_20, MSIOF1_RXD), > + PINMUX_IPSR_GPSR(IP0SR1_27_24, MSIOF0_SS2), Missing HTX1 and TX1 > + PINMUX_IPSR_GPSR(IP0SR1_31_28, MSIOF0_SS1), Missing HRX1 and TX1 > + > + /* IP1SR1 */ > + PINMUX_IPSR_GPSR(IP1SR1_3_0, MSIOF0_SYNC), Missing HCTS1_N, CTS1_N, and CANFD5_TX_B > + > + PINMUX_IPSR_GPSR(IP1SR1_7_4, MSIOF0_TXD), Missing HRTS1_N, RTS1_N, and CANFD5_RX_B > + > + PINMUX_IPSR_GPSR(IP1SR1_11_8, MSIOF0_SCK), Missing HSCK1 and SCK1 > + > + PINMUX_IPSR_GPSR(IP1SR1_15_12, MSIOF0_RXD), > + > + PINMUX_IPSR_GPSR(IP1SR1_19_16, HTX0), > + PINMUX_IPSR_GPSR(IP1SR1_19_16, TX0), > + > + PINMUX_IPSR_GPSR(IP1SR1_23_20, HCTS0_N), > + PINMUX_IPSR_GPSR(IP1SR1_23_20, CTS0_N), > + PINMUX_IPSR_GPSR(IP1SR1_23_20, PWM8), PWM8_A > + > + PINMUX_IPSR_GPSR(IP1SR1_27_24, HRTS0_N), > + PINMUX_IPSR_GPSR(IP1SR1_27_24, RTS0_N), > + PINMUX_IPSR_GPSR(IP1SR1_27_24, PWM9), PWM9_A > + > + PINMUX_IPSR_GPSR(IP1SR1_31_28, HSCK0), > + PINMUX_IPSR_GPSR(IP1SR1_31_28, SCK0), > + PINMUX_IPSR_GPSR(IP1SR1_31_28, PWM0), PWM0_A > + > + /* IP2SR1 */ > + PINMUX_IPSR_GPSR(IP2SR1_3_0, HRX0), > + PINMUX_IPSR_GPSR(IP2SR1_3_0, RX0), > + > + PINMUX_IPSR_GPSR(IP2SR1_7_4, SCIF_CLK), > + PINMUX_IPSR_GPSR(IP2SR1_7_4, IRQ4), IRQ4_A > + > + PINMUX_IPSR_GPSR(IP2SR1_11_8, SSI_SCK), > + PINMUX_IPSR_GPSR(IP2SR1_11_8, TCLK3), > + > + PINMUX_IPSR_GPSR(IP2SR1_15_12, SSI_WS), > + PINMUX_IPSR_GPSR(IP2SR1_15_12, TCLK4), > + > + PINMUX_IPSR_GPSR(IP2SR1_19_16, SSI_SD), Missing IRQ0_A > + > + PINMUX_IPSR_GPSR(IP2SR1_23_20, AUDIO_CLKOUT), Missing IRQ1_A > + > + PINMUX_IPSR_GPSR(IP2SR1_27_24, AUDIO_CLKIN), > + PINMUX_IPSR_GPSR(IP2SR1_27_24, PWM3), PWM3_A > + > + PINMUX_IPSR_GPSR(IP2SR1_31_28, TCLK2), > + PINMUX_IPSR_GPSR(IP2SR1_31_28, MSIOF4_SS1), Missing IRQ3_B > + > + /* IP3SR1 */ > + PINMUX_IPSR_GPSR(IP3SR1_3_0, HRX3), > + PINMUX_IPSR_GPSR(IP3SR1_3_0, SCK3), SCK3_A > + PINMUX_IPSR_GPSR(IP3SR1_3_0, MSIOF4_SS2), > + > + PINMUX_IPSR_GPSR(IP3SR1_7_4, HSCK3), > + PINMUX_IPSR_GPSR(IP3SR1_7_4, CTS3_N), CTS3_N_A > + PINMUX_IPSR_GPSR(IP3SR1_7_4, MSIOF4_SCK), Missing TPU0TO0_A > + > + PINMUX_IPSR_GPSR(IP3SR1_11_8, HRTS3_N), > + PINMUX_IPSR_GPSR(IP3SR1_11_8, RTS3_N), RTS3_N_A > + PINMUX_IPSR_GPSR(IP3SR1_11_8, MSIOF4_TXD), Missing TPU0TO1_A > + > + PINMUX_IPSR_GPSR(IP3SR1_15_12, HCTS3_N), > + PINMUX_IPSR_GPSR(IP3SR1_15_12, RX3), RX3_A > + PINMUX_IPSR_GPSR(IP3SR1_15_12, MSIOF4_RXD), > + > + PINMUX_IPSR_GPSR(IP3SR1_19_16, HTX3), > + PINMUX_IPSR_GPSR(IP3SR1_19_16, TX3), TX3_A > + PINMUX_IPSR_GPSR(IP3SR1_19_16, MSIOF4_SYNC), > + > + /* IP0SR2 */ > + PINMUX_IPSR_GPSR(IP0SR2_3_0, FXR_TXDA), > + PINMUX_IPSR_GPSR(IP0SR2_3_0, CANFD1_TX), Missing TPU0TO2_A > + > + PINMUX_IPSR_GPSR(IP0SR2_7_4, FXR_TXENA_N), > + PINMUX_IPSR_GPSR(IP0SR2_7_4, CANFD1_RX), Missing TPU0TO3_A > + > + PINMUX_IPSR_GPSR(IP0SR2_11_8, RXDA_EXTFXR), > + PINMUX_IPSR_GPSR(IP0SR2_11_8, CANFD5_TX), > + PINMUX_IPSR_GPSR(IP0SR2_11_8, IRQ5), > + > + PINMUX_IPSR_GPSR(IP0SR2_15_12, CLK_EXTFXR), > + PINMUX_IPSR_GPSR(IP0SR2_15_12, CANFD5_RX), Missing IRQ4_B > + > + PINMUX_IPSR_GPSR(IP0SR2_19_16, RXDB_EXTFXR), > + > + PINMUX_IPSR_GPSR(IP0SR2_23_20, FXR_TXENB_N), > + > + PINMUX_IPSR_GPSR(IP0SR2_27_24, FXR_TXDB), > + > + PINMUX_IPSR_GPSR(IP0SR2_31_28, TPU0TO1), > + PINMUX_IPSR_GPSR(IP0SR2_31_28, CANFD6_TX), Missing TCLK2_B > + > + /* IP1SR2 */ > + PINMUX_IPSR_GPSR(IP1SR2_3_0, TPU0TO0), > + PINMUX_IPSR_GPSR(IP1SR2_3_0, CANFD6_RX), Missing TCLK1_A > + > + PINMUX_IPSR_GPSR(IP1SR2_7_4, CAN_CLK), Missing FXR_TXENA_N > + > + PINMUX_IPSR_GPSR(IP1SR2_11_8, CANFD0_TX), Missing FXR_TXENB_N > + > + PINMUX_IPSR_GPSR(IP1SR2_15_12, CANFD0_RX), > + PINMUX_IPSR_GPSR(IP1SR2_15_12, STPWT_EXTFXR), > + > + PINMUX_IPSR_GPSR(IP1SR2_19_16, CANFD2_TX), > + PINMUX_IPSR_GPSR(IP1SR2_19_16, TPU0TO2), Missing TCLK3_A > + > + PINMUX_IPSR_GPSR(IP1SR2_23_20, CANFD2_RX), > + PINMUX_IPSR_GPSR(IP1SR2_23_20, TPU0TO3), > + PINMUX_IPSR_GPSR(IP1SR2_23_20, PWM1), PWM1_B Missing TCLK4_A > + > + PINMUX_IPSR_GPSR(IP1SR2_27_24, CANFD3_TX), > + PINMUX_IPSR_GPSR(IP1SR2_27_24, PWM2), PWM2_B > + > + PINMUX_IPSR_GPSR(IP1SR2_31_28, CANFD3_RX), Missing PWM3_B > + > + /* IP2SR2 */ > + PINMUX_IPSR_GPSR(IP2SR2_3_0, CANFD4_TX), > + PINMUX_IPSR_GPSR(IP2SR2_3_0, PWM4), > + > + PINMUX_IPSR_GPSR(IP2SR2_7_4, CANFD4_RX), > + PINMUX_IPSR_GPSR(IP2SR2_7_4, PWM5), > + > + PINMUX_IPSR_GPSR(IP2SR2_11_8, CANFD7_TX), > + PINMUX_IPSR_GPSR(IP2SR2_11_8, PWM6), > + > + PINMUX_IPSR_GPSR(IP2SR2_15_12, CANFD7_RX), > + PINMUX_IPSR_GPSR(IP2SR2_15_12, PWM7), > + > + /* IP0SR3 */ > + PINMUX_IPSR_GPSR(IP0SR3_3_0, MMC_SD_D1), > + PINMUX_IPSR_GPSR(IP0SR3_7_4, MMC_SD_D0), > + PINMUX_IPSR_GPSR(IP0SR3_11_8, MMC_SD_D2), > + PINMUX_IPSR_GPSR(IP0SR3_15_12, MMC_SD_CLK), > + PINMUX_IPSR_GPSR(IP0SR3_19_16, MMC_DS), > + PINMUX_IPSR_GPSR(IP0SR3_23_20, MMC_SD_D3), > + PINMUX_IPSR_GPSR(IP0SR3_27_24, MMC_D5), > + PINMUX_IPSR_GPSR(IP0SR3_31_28, MMC_D4), > + > + /* IP1SR3 */ > + PINMUX_IPSR_GPSR(IP1SR3_3_0, MMC_D7), > + > + PINMUX_IPSR_GPSR(IP1SR3_7_4, MMC_D6), > + > + PINMUX_IPSR_GPSR(IP1SR3_11_8, MMC_SD_CMD), > + > + PINMUX_IPSR_GPSR(IP1SR3_15_12, SD_CD), > + > + PINMUX_IPSR_GPSR(IP1SR3_19_16, SD_WP), > + > + PINMUX_IPSR_GPSR(IP1SR3_23_20, IPC_CLKIN), > + PINMUX_IPSR_GPSR(IP1SR3_23_20, IPC_CLKEN_IN), Missing PWM1_A and TCLK3 > + > + PINMUX_IPSR_GPSR(IP1SR3_27_24, IPC_CLKOUT), > + PINMUX_IPSR_GPSR(IP1SR3_27_24, IPC_CLKEN_OUT), Missing ERROROUTC_A, TCLK4, and PWM2_A (the latter is only listed in the pin function spreadsheet, as the main PDF file lacks a column for function 4?). > + > + PINMUX_IPSR_GPSR(IP1SR3_31_28, QSPI0_SSL), > + > + /* IP2SR3 */ > + PINMUX_IPSR_GPSR(IP2SR3_3_0, QSPI0_IO3), > + PINMUX_IPSR_GPSR(IP2SR3_7_4, QSPI0_IO2), > + PINMUX_IPSR_GPSR(IP2SR3_11_8, QSPI0_MISO_IO1), > + PINMUX_IPSR_GPSR(IP2SR3_15_12, QSPI0_MOSI_IO0), > + PINMUX_IPSR_GPSR(IP2SR3_19_16, QSPI0_SPCLK), > + PINMUX_IPSR_GPSR(IP2SR3_23_20, QSPI1_MOSI_IO0), > + PINMUX_IPSR_GPSR(IP2SR3_27_24, QSPI1_SPCLK), > + PINMUX_IPSR_GPSR(IP2SR3_31_28, QSPI1_MISO_IO1), > + > + /* IP3SR3 */ > + PINMUX_IPSR_GPSR(IP3SR3_3_0, QSPI1_IO2), > + PINMUX_IPSR_GPSR(IP3SR3_7_4, QSPI1_SSL), > + PINMUX_IPSR_GPSR(IP3SR3_11_8, QSPI1_IO3), > + PINMUX_IPSR_GPSR(IP3SR3_15_12, RPC_RESET_N), > + PINMUX_IPSR_GPSR(IP3SR3_19_16, RPC_WP_N), > + PINMUX_IPSR_GPSR(IP3SR3_23_20, RPC_INT_N), > + > + /* IP0SR6 */ > + PINMUX_IPSR_GPSR(IP0SR6_3_0, AVB1_MDIO), > + > + PINMUX_IPSR_GPSR(IP0SR6_7_4, AVB1_MAGIC), > + > + PINMUX_IPSR_GPSR(IP0SR6_11_8, AVB1_MDC), > + > + PINMUX_IPSR_GPSR(IP0SR6_15_12, AVB1_PHY_INT), > + > + PINMUX_IPSR_GPSR(IP0SR6_19_16, AVB1_LINK), > + PINMUX_IPSR_GPSR(IP0SR6_19_16, AVB1_MII_TX_ER), Hmm, the MII functions for SVB[01] are only documented in the main PDF file, not in the pin function spreadsheet... > + > + PINMUX_IPSR_GPSR(IP0SR6_23_20, AVB1_AVTP_MATCH), > + PINMUX_IPSR_GPSR(IP0SR6_23_20, AVB1_MII_RX_ER), > + > + PINMUX_IPSR_GPSR(IP0SR6_27_24, AVB1_TXC), > + PINMUX_IPSR_GPSR(IP0SR6_27_24, AVB1_MII_TXC), > + > + PINMUX_IPSR_GPSR(IP0SR6_31_28, AVB1_TX_CTL), > + PINMUX_IPSR_GPSR(IP0SR6_31_28, AVB1_MII_TX_EN), > + > + /* IP1SR6 */ > + PINMUX_IPSR_GPSR(IP1SR6_3_0, AVB1_RXC), > + PINMUX_IPSR_GPSR(IP1SR6_3_0, AVB1_MII_RXC), > + > + PINMUX_IPSR_GPSR(IP1SR6_7_4, AVB1_RX_CTL), > + PINMUX_IPSR_GPSR(IP1SR6_7_4, AVB1_MII_RX_DV), > + > + PINMUX_IPSR_GPSR(IP1SR6_11_8, AVB1_AVTP_PPS), > + PINMUX_IPSR_GPSR(IP1SR6_11_8, AVB1_MII_COL), > + > + PINMUX_IPSR_GPSR(IP1SR6_15_12, AVB1_AVTP_CAPTURE), > + PINMUX_IPSR_GPSR(IP1SR6_15_12, AVB1_MII_CRS), > + > + PINMUX_IPSR_GPSR(IP1SR6_19_16, AVB1_TD1), > + PINMUX_IPSR_GPSR(IP1SR6_19_16, AVB1_MII_TD1), > + > + PINMUX_IPSR_GPSR(IP1SR6_23_20, AVB1_TD0), > + PINMUX_IPSR_GPSR(IP1SR6_23_20, AVB1_MII_TD0), > + > + PINMUX_IPSR_GPSR(IP1SR6_27_24, AVB1_RD1), > + PINMUX_IPSR_GPSR(IP1SR6_27_24, AVB1_MII_RD1), > + > + PINMUX_IPSR_GPSR(IP1SR6_31_28, AVB1_RD0), > + PINMUX_IPSR_GPSR(IP1SR6_31_28, AVB1_MII_RD0), > + > + /* IP2SR6 */ > + PINMUX_IPSR_GPSR(IP2SR6_3_0, AVB1_TD2), > + PINMUX_IPSR_GPSR(IP2SR6_3_0, AVB1_MII_TD2), > + > + PINMUX_IPSR_GPSR(IP2SR6_7_4, AVB1_RD2), > + PINMUX_IPSR_GPSR(IP2SR6_7_4, AVB1_MII_RD2), > + > + PINMUX_IPSR_GPSR(IP2SR6_11_8, AVB1_TD3), > + PINMUX_IPSR_GPSR(IP2SR6_11_8, AVB1_MII_TD3), > + > + PINMUX_IPSR_GPSR(IP2SR6_15_12, AVB1_RD3), > + PINMUX_IPSR_GPSR(IP2SR6_15_12, AVB1_MII_RD3), > + > + PINMUX_IPSR_GPSR(IP2SR6_19_16, AVB1_TXCREFCLK), Some of the above need to configure MODSEL6, so you should use PINMUX_IPSR_MSEL(..., AVB1_*, SEL_AVB1_*). > + > + /* IP0SR7 */ > + PINMUX_IPSR_MSEL(IP0SR7_3_0, AVB0_AVTP_PPS, SEL_AVB0_AVTP_PPS_1), > + PINMUX_IPSR_MSEL(IP0SR7_3_0, AVB0_MII_COL, SEL_AVB0_AVTP_PPS_0), > + > + PINMUX_IPSR_GPSR(IP0SR7_7_4, AVB0_AVTP_CAPTURE), > + PINMUX_IPSR_GPSR(IP0SR7_7_4, AVB0_MII_CRS), > + > + PINMUX_IPSR_MSEL(IP0SR7_11_8, AVB0_AVTP_MATCH, SEL_AVB0_AVTP_MATCH_1), > + PINMUX_IPSR_MSEL(IP0SR7_11_8, AVB0_MII_RX_ER, SEL_AVB0_AVTP_MATCH_0), > + PINMUX_IPSR_MSEL(IP0SR7_11_8, CC5_OSCOUT, SEL_AVB0_AVTP_MATCH_0), > + > + PINMUX_IPSR_MSEL(IP0SR7_15_12, AVB0_TD3, SEL_AVB0_TD3_1), > + PINMUX_IPSR_MSEL(IP0SR7_15_12, AVB0_MII_TD3, SEL_AVB0_TD3_0), > + > + PINMUX_IPSR_GPSR(IP0SR7_19_16, AVB0_LINK), > + PINMUX_IPSR_GPSR(IP0SR7_19_16, AVB0_MII_TX_ER), > + > + PINMUX_IPSR_GPSR(IP0SR7_23_20, AVB0_PHY_INT), > + > + PINMUX_IPSR_MSEL(IP0SR7_27_24, AVB0_TD2, SEL_AVB0_TD2_1), > + PINMUX_IPSR_MSEL(IP0SR7_27_24, AVB0_MII_TD2, SEL_AVB0_TD2_0), > + > + PINMUX_IPSR_MSEL(IP0SR7_31_28, AVB0_TD1, SEL_AVB0_TD1_1), > + PINMUX_IPSR_MSEL(IP0SR7_31_28, AVB0_MII_TD1, SEL_AVB0_TD1_0), > + > + /* IP1SR7 */ > + PINMUX_IPSR_GPSR(IP1SR7_3_0, AVB0_RD3), > + PINMUX_IPSR_GPSR(IP1SR7_3_0, AVB0_MII_RD3), > + > + PINMUX_IPSR_GPSR(IP1SR7_7_4, AVB0_TXCREFCLK), > + > + PINMUX_IPSR_MSEL(IP1SR7_11_8, AVB0_MAGIC, SEL_AVB0_MAGIC_1), > + > + PINMUX_IPSR_MSEL(IP1SR7_15_12, AVB0_TD0, SEL_AVB0_TD0_1), > + PINMUX_IPSR_MSEL(IP1SR7_15_12, AVB0_MII_TD0, SEL_AVB0_TD0_0), > + > + PINMUX_IPSR_GPSR(IP1SR7_19_16, AVB0_RD2), > + PINMUX_IPSR_GPSR(IP1SR7_19_16, AVB0_MII_RD2), > + > + PINMUX_IPSR_MSEL(IP1SR7_23_20, AVB0_MDC, SEL_AVB0_MDC_1), > + > + PINMUX_IPSR_GPSR(IP1SR7_27_24, AVB0_MDIO), > + > + PINMUX_IPSR_MSEL(IP1SR7_31_28, AVB0_TXC, SEL_AVB0_TXC_1), > + PINMUX_IPSR_MSEL(IP1SR7_31_28, AVB0_MII_TXC, SEL_AVB0_TXC_0), > + > + /* IP2SR7 */ > + PINMUX_IPSR_MSEL(IP2SR7_3_0, AVB0_TX_CTL, SEL_AVB0_TX_CTL_1), > + PINMUX_IPSR_MSEL(IP2SR7_3_0, AVB0_MII_TX_EN, SEL_AVB0_TX_CTL_0), Are the above SEL_AVB0_* values correct? I am not an MII expert, but I would expect e.g. both AVB0_TD3 and AVB0_MII_TD3 to need SEL_AVB0_TD3_1 (= output enabled). > + > + PINMUX_IPSR_GPSR(IP2SR7_7_4, AVB0_RD1), > + PINMUX_IPSR_GPSR(IP2SR7_7_4, AVB0_MII_RD1), > + > + PINMUX_IPSR_GPSR(IP2SR7_11_8, AVB0_RD0), > + PINMUX_IPSR_GPSR(IP2SR7_11_8, AVB0_MII_RD0), > + > + PINMUX_IPSR_GPSR(IP2SR7_15_12, AVB0_RXC), > + PINMUX_IPSR_GPSR(IP2SR7_15_12, AVB0_MII_RXC), > + > + PINMUX_IPSR_GPSR(IP2SR7_19_16, AVB0_RX_CTL), > + PINMUX_IPSR_GPSR(IP2SR7_19_16, AVB0_MII_RX_DV), > + > + /* IP0SR8 */ > + PINMUX_IPSR_MSEL(IP0SR8_3_0, SCL0, SEL_SCL0_0), SEL_SCL0_1 > + PINMUX_IPSR_MSEL(IP0SR8_7_4, SDA0, SEL_SDA0_0), SEL_SDA0_1 > + PINMUX_IPSR_MSEL(IP0SR8_11_8, SCL1, SEL_SCL1_0), SEL_SCL1_1 > + PINMUX_IPSR_MSEL(IP0SR8_15_12, SDA1, SEL_SDA1_0), SEL_SDA1_1 > + PINMUX_IPSR_MSEL(IP0SR8_19_16, SCL2, SEL_SCL2_0), SEL_SCL2_1 > + PINMUX_IPSR_MSEL(IP0SR8_23_20, SDA2, SEL_SDA2_0), SEL_SDA2_1 > + PINMUX_IPSR_MSEL(IP0SR8_27_24, SCL3, SEL_SCL3_0), SEL_SCL3_1 > + PINMUX_IPSR_MSEL(IP0SR8_31_28, SDA3, SEL_SDA3_0), SEL_SDA3_1 > + > + /* IP1SR8 */ > + PINMUX_IPSR_MSEL(IP1SR8_3_0, SCL4, SEL_SCL4_0), SEL_SCL4_1 > + PINMUX_IPSR_MSEL(IP1SR8_3_0, HRX2, SEL_SCL4_0), > + PINMUX_IPSR_MSEL(IP1SR8_3_0, SCK4, SEL_SCL4_0), > + > + PINMUX_IPSR_MSEL(IP1SR8_7_4, SDA4, SEL_SDA4_0), SEL_SDA4_1 > + PINMUX_IPSR_MSEL(IP1SR8_7_4, HTX2, SEL_SDA4_0), > + PINMUX_IPSR_MSEL(IP1SR8_7_4, CTS4_N, SEL_SDA4_0), > + > + PINMUX_IPSR_MSEL(IP1SR8_11_8, SCL5, SEL_SCL5_0), SEL_SCL5_1 > + PINMUX_IPSR_MSEL(IP1SR8_11_8, HRTS2_N, SEL_SCL5_0), > + PINMUX_IPSR_MSEL(IP1SR8_11_8, RTS4_N, SEL_SCL5_0), > + > + PINMUX_IPSR_MSEL(IP1SR8_15_12, SDA5, SEL_SDA5_0), SEL_SDA5_1 > + PINMUX_IPSR_MSEL(IP1SR8_15_12, SCIF_CLK2, SEL_SDA5_0), > + > + PINMUX_IPSR_GPSR(IP1SR8_19_16, HCTS2_N), > + PINMUX_IPSR_GPSR(IP1SR8_19_16, TX4), > + > + PINMUX_IPSR_GPSR(IP1SR8_23_20, HSCK2), > + PINMUX_IPSR_GPSR(IP1SR8_23_20, RX4), > +}; Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Morimoto-san, On Mon, Jun 13, 2022 at 1:51 AM Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote: > > > From: LUU HOAI <hoai.luu.ub@renesas.com> > > > > > > This patch adds initial pinctrl support for the R8A779G0 (V4H) SoC, > > > including bias, drive strength and voltage control. > > > > > > [Morimoto merged Kihara-san's MODSEL8 fixup patch, > > > and cleanuped white space, care reserved bit on each configs, > > > fixup setting miss] > > > Signed-off-by: LUU HOAI <hoai.luu.ub@renesas.com> > > > Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com> > > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > > > Thanks for your patch! > > > > Below is a first set of comments. > > You can expect a second set of comments after the weekend. > > May I suggest ? > > v1 patch was almost as-is of working implementation, > I did was just cleanup. > v2 patch was tidyup implementation which found via runtime. > So far, I think we can say "v2 patch is cleanup version of working implementation". > > But, v3 needs too many tidyups, and it is related to very deep parts. > I'm very afraid it breaks working implementation for some reasons. > In such case, it is very difficult to find and/or fix the issue > because PFC file is too big and too complicated. > > I'm happy to fix all of you pointed, but I want to make it as additional patch. > But what do you think ? For missing features (e.g. missing alternative functions, missing support for using I2C-capable pins as GPIOs, ...) I agree: these can be added incrementally. For descriptions that are wrong, I disagree: please try to fix them in the patch that introduces them, if possible. This is especially true for descriptions that become user-visible: e.g. set names (no suffix vs. "_A" vs. "_B" suffixes) become DT ABI once their pin groups have been defined. Unfortunately some of the (lack of) set names need confirmation or clarification from the hardware documentation team. BTW, do you know which devices and pin groups have been tested on actual hardware? Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert Thank you for your feedback > For missing features (e.g. missing alternative functions, missing > support for using I2C-capable pins as GPIOs, ...) I agree: these can > be added incrementally. > > For descriptions that are wrong, I disagree: please try to fix them > in the patch that introduces them, if possible. Yes of course. v3 patch of [3/4][4/4] will includes comment fixup, alphabetical order fixup, variable name fixup, etc, for each patches. Basically non code area. And, it will have additional patch for missing features (add xxx support). > Unfortunately some of the (lack of) set names need confirmation or > clarification from the hardware documentation team. Yes. In such case, I will use _X or _Y for now to avoid future conflict. > BTW, do you know which devices and pin groups have been tested on > actual hardware? I didn't test, but [4/4] patches are, I guess. Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Geert > > +#define CPU_ALL_GP(fn, sfx) \ > > + PORT_GP_CFG_19(0, fn, sfx, CFG_FLAGS | SH_PFC_PIN_CFG_IO_VOLTAGE_18_33), \ > > + PORT_GP_CFG_29(1, fn, sfx, CFG_FLAGS | SH_PFC_PIN_CFG_IO_VOLTAGE_18_33), \ > > GP1_23 to GP1_28 do not support voltage control. (snip) > "<= RCAR_GP_PIN(1, 22)", as GP1_23 to GP1_28 do not support voltage > control. Am I missing something ? I guess GP1_23 to GP1_28 are same as other GP1_xxx. Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Morimoto-san, On Tue, Jun 14, 2022 at 1:34 AM Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote: > > > +#define CPU_ALL_GP(fn, sfx) \ > > > + PORT_GP_CFG_19(0, fn, sfx, CFG_FLAGS | SH_PFC_PIN_CFG_IO_VOLTAGE_18_33), \ > > > + PORT_GP_CFG_29(1, fn, sfx, CFG_FLAGS | SH_PFC_PIN_CFG_IO_VOLTAGE_18_33), \ > > > > GP1_23 to GP1_28 do not support voltage control. > (snip) > > "<= RCAR_GP_PIN(1, 22)", as GP1_23 to GP1_28 do not support voltage > > control. > > Am I missing something ? > I guess GP1_23 to GP1_28 are same as other GP1_xxx. Table 7.28 ("Configuration of Registers in POC0 , POC1") documents voltage control bits in POC1 for bits 0-22 only. However, the pin function spreadsheet says GP1_23 to GP1_28 are 1.8/3.3V. Which one is correct? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert > > > "<= RCAR_GP_PIN(1, 22)", as GP1_23 to GP1_28 do not support voltage > > > control. > > > > Am I missing something ? > > I guess GP1_23 to GP1_28 are same as other GP1_xxx. > > Table 7.28 ("Configuration of Registers in POC0 , POC1") documents > voltage control bits in POC1 for bits 0-22 only. > > However, the pin function spreadsheet says GP1_23 to GP1_28 are > 1.8/3.3V. > > Which one is correct? OK, I see. I will ask it. I want to keep original patch almost as-is (only cleanup). I will post it as "fixup patch". Thank you for your help !! Best regards --- Kuninori Morimoto