| Message ID | 20171003073334.18917-2-chris.brandt@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Chris, thanks for the patches On Tue, Oct 03, 2017 at 02:33:33AM -0500, Chris Brandt wrote: > Aspects like the number of ports and the location where peripherals are > brought out differ between the RZ/A1H and RZ/A1L. > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> > --- > drivers/pinctrl/pinctrl-rza1.c | 134 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 134 insertions(+) > > diff --git a/drivers/pinctrl/pinctrl-rza1.c b/drivers/pinctrl/pinctrl-rza1.c > index 04d058706b80..717c0f4449a0 100644 > --- a/drivers/pinctrl/pinctrl-rza1.c > +++ b/drivers/pinctrl/pinctrl-rza1.c > @@ -302,6 +302,134 @@ static const struct rza1_pinmux_conf rza1h_pmx_conf = { > .swio_entries = rza1h_swio_entries, > }; > > +/* ---------------------------------------------------------------------------- > + * RZ/A1L (r7s72102) pinmux flags > + */ Do you have other authoritative sources for pin's bidir settings? According to the publicly available manual, there are other pins that needs bidir not listed here. As the same happened with RZ/A1H I'm not too concerned, but I'm listing some of there here nonetheless > + > +static const struct rza1_bidir_pin rza1l_bidir_pins_p1[] = { > + { .pin = 0, .func = 1 }, > + { .pin = 1, .func = 1 }, > + { .pin = 2, .func = 1 }, > + { .pin = 3, .func = 1 }, > + { .pin = 4, .func = 1 }, > + { .pin = 5, .func = 1 }, > + { .pin = 6, .func = 1 }, > + { .pin = 7, .func = 1 }, > +}; > + P2 has several TIOC pins in mode 4 and SSI in mode 2 > +static const struct rza1_bidir_pin rza1l_bidir_pins_p3[] = { > + { .pin = 0, .func = 2 }, > + { .pin = 1, .func = 2 }, > + { .pin = 2, .func = 2 }, > + { .pin = 4, .func = 2 }, > + { .pin = 5, .func = 2 }, > + { .pin = 10, .func = 2 }, > + { .pin = 11, .func = 2 }, > + { .pin = 12, .func = 2 }, > + { .pin = 13, .func = 2 }, > +}; TIOC in mode3, SKC3 and SCK1 in mode 5 > + > +static const struct rza1_bidir_pin rza1l_bidir_pins_p4[] = { > + { .pin = 1, .func = 4 }, > + { .pin = 2, .func = 2 }, > + { .pin = 3, .func = 2 }, > + { .pin = 6, .func = 2 }, > + { .pin = 7, .func = 2 }, > +}; Confused, you're here listing TIOC2B pin (pin 1 mode #4) but not TIOC1B (pin 0 mode #4) ET_MDIO in mode 4, which we list as bidir in RZ/A1H > + > +static const struct rza1_bidir_pin rza1l_bidir_pins_p5[] = { > + { .pin = 0, .func = 1 }, > + { .pin = 1, .func = 1 }, > + { .pin = 2, .func = 1 }, > + { .pin = 3, .func = 1 }, > + { .pin = 4, .func = 1 }, > + { .pin = 5, .func = 1 }, > + { .pin = 6, .func = 1 }, > + { .pin = 7, .func = 1 }, > + { .pin = 8, .func = 1 }, > + { .pin = 9, .func = 1 }, > + { .pin = 10, .func = 1 }, > + { .pin = 11, .func = 1 }, > + { .pin = 12, .func = 1 }, > + { .pin = 13, .func = 1 }, > + { .pin = 14, .func = 1 }, > + { .pin = 15, .func = 1 }, > + { .pin = 0, .func = 2 }, > + { .pin = 1, .func = 2 }, > + { .pin = 2, .func = 2 }, > + { .pin = 3, .func = 2 }, > +}; I'll stop here with bidir as I guess we're following different documents, as it happened for RZ/A1H you may have another source of informations > + > +static const struct rza1_bidir_pin rza1l_bidir_pins_p6[] = { > + { .pin = 0, .func = 1 }, > + { .pin = 1, .func = 1 }, > + { .pin = 2, .func = 1 }, > + { .pin = 3, .func = 1 }, > + { .pin = 4, .func = 1 }, > + { .pin = 5, .func = 1 }, > + { .pin = 6, .func = 1 }, > + { .pin = 7, .func = 1 }, > + { .pin = 8, .func = 1 }, > + { .pin = 9, .func = 1 }, > + { .pin = 10, .func = 1 }, > + { .pin = 11, .func = 1 }, > + { .pin = 12, .func = 1 }, > + { .pin = 13, .func = 1 }, > + { .pin = 14, .func = 1 }, > + { .pin = 15, .func = 1 }, > +}; > + > +static const struct rza1_bidir_pin rza1l_bidir_pins_p7[] = { > + { .pin = 2, .func = 2 }, > + { .pin = 3, .func = 2 }, > + { .pin = 5, .func = 2 }, > + { .pin = 6, .func = 2 }, > + { .pin = 7, .func = 2 }, > + { .pin = 2, .func = 3 }, > + { .pin = 3, .func = 3 }, > + { .pin = 5, .func = 3 }, > + { .pin = 6, .func = 3 }, > + { .pin = 7, .func = 3 }, > +}; > + > +static const struct rza1_bidir_pin rza1l_bidir_pins_p9[] = { > + { .pin = 1, .func = 2 }, > + { .pin = 0, .func = 3 }, > + { .pin = 1, .func = 3 }, > + { .pin = 3, .func = 3 }, > + { .pin = 4, .func = 3 }, > + { .pin = 5, .func = 3 }, > +}; > + > +static const struct rza1_swio_pin rza1l_swio_pins[] = { > + { .port = 2, .pin = 8, .func = 2, .input = 0 }, > + { .port = 5, .pin = 6, .func = 3, .input = 0 }, > + { .port = 6, .pin = 6, .func = 3, .input = 0 }, > + { .port = 6, .pin = 10, .func = 3, .input = 0 }, > + { .port = 7, .pin = 10, .func = 2, .input = 0 }, > + { .port = 8, .pin = 2, .func = 3, .input = 0 }, > +}; I count 20 SWIO pins TIOC[0-4][A-D] SSITx[0-3] WDTOVF Thanks j > + > +static const struct rza1_bidir_entry rza1l_bidir_entries[RZA1_NPORTS] = { > + [1] = { ARRAY_SIZE(rza1l_bidir_pins_p1), rza1l_bidir_pins_p1 }, > + [3] = { ARRAY_SIZE(rza1l_bidir_pins_p3), rza1l_bidir_pins_p3 }, > + [4] = { ARRAY_SIZE(rza1l_bidir_pins_p4), rza1l_bidir_pins_p4 }, > + [5] = { ARRAY_SIZE(rza1l_bidir_pins_p4), rza1l_bidir_pins_p5 }, > + [6] = { ARRAY_SIZE(rza1l_bidir_pins_p6), rza1l_bidir_pins_p6 }, > + [7] = { ARRAY_SIZE(rza1l_bidir_pins_p7), rza1l_bidir_pins_p7 }, > + [9] = { ARRAY_SIZE(rza1l_bidir_pins_p9), rza1l_bidir_pins_p9 }, > +}; > + > +static const struct rza1_swio_entry rza1l_swio_entries[] = { > + [0] = { ARRAY_SIZE(rza1h_swio_pins), rza1h_swio_pins }, > +}; > + > +/* RZ/A1L (r7s72102x) pinmux flags table */ > +static const struct rza1_pinmux_conf rza1l_pmx_conf = { > + .bidir_entries = rza1l_bidir_entries, > + .swio_entries = rza1l_swio_entries, > +}; > + > /* ---------------------------------------------------------------------------- > * RZ/A1 types > */ > @@ -1283,9 +1411,15 @@ static int rza1_pinctrl_probe(struct platform_device *pdev) > > static const struct of_device_id rza1_pinctrl_of_match[] = { > { > + /* RZ/A1H, RZ/A1M */ > .compatible = "renesas,r7s72100-ports", > .data = &rza1h_pmx_conf, > }, > + { > + /* RZ/A1L */ > + .compatible = "renesas,r7s72102-ports", > + .data = &rza1l_pmx_conf, > + }, > { } > }; > > -- > 2.14.1 > >
Hi Jacopo, On Tuesday, October 03, 2017, jacopo mondi wrote: > Do you have other authoritative sources for pin's bidir settings? > According to the publicly available manual, there are other pins that > needs bidir not listed here. > As the same happened with RZ/A1H I'm not too concerned, but I'm > listing some of there here nonetheless Back when you were working on this, I discussed with the guys back in Japan. While in the table there are multiple pins that show 'bi-directional capable', they don't all need the bi-dir bit set. This was the list of peripheral: I2C, MDIO (ETH), MMC, SDHI, QSPI, Data (D0-D31) > P2 has several TIOC pins in mode 4 and SSI in mode 2 Port 2??? Port 2, mode 4 in the RZ/A1L manual only has IRQ7,6,5,2,1 I thought we decided on that TIOC pins are ones that are manually set to either input (input-enable, output-enable) by the users Device Tree, so they don't need to be put into a table. If input-enable or output-enable is specified, then you manually set it to that direction. As for SSI, the manual says only the SSITx pins need bi-dir, not the SSIRx pins. > TIOC in mode3, SKC3 and SCK1 in mode 5 > > + > > +static const struct rza1_bidir_pin rza1l_bidir_pins_p4[] = { > > + { .pin = 1, .func = 4 }, > > + { .pin = 2, .func = 2 }, > > + { .pin = 3, .func = 2 }, > > + { .pin = 6, .func = 2 }, > > + { .pin = 7, .func = 2 }, > > +}; > > Confused, you're here listing TIOC2B pin (pin 1 mode #4) but not TIOC1B > (pin 0 mode #4) > ET_MDIO in mode 4, which we list as bidir in RZ/A1H P4_1 mode 4 is ET_MDIO. TIOC1B is P4_0 mode 3. TIOC2B is P4_1 Mode 3 Are you looking at Table 41.19 in the Rev.3.00 version of the RZ/A1L Hardware Manual? > > +static const struct rza1_swio_pin rza1l_swio_pins[] = { > > + { .port = 2, .pin = 8, .func = 2, .input = 0 }, > > + { .port = 5, .pin = 6, .func = 3, .input = 0 }, > > + { .port = 6, .pin = 6, .func = 3, .input = 0 }, > > + { .port = 6, .pin = 10, .func = 3, .input = 0 }, > > + { .port = 7, .pin = 10, .func = 2, .input = 0 }, > > + { .port = 8, .pin = 2, .func = 3, .input = 0 }, > > +}; > > I count 20 SWIO pins > > TIOC[0-4][A-D] > SSITx[0-3] > WDTOVF I thought we said TIOC didn't need to be in a table because the user has to specify them anyway because the user has to choose if he wants them as input or output, so if he does that, then we automatically know it's a swio pin. Hmm, maybe I have to go back and look at what the driver ended up implementing. Chris
Hi Chris, On Tue, Oct 03, 2017 at 01:36:57PM +0000, Chris Brandt wrote: > Hi Jacopo, > > On Tuesday, October 03, 2017, jacopo mondi wrote: > > Do you have other authoritative sources for pin's bidir settings? > > According to the publicly available manual, there are other pins that > > needs bidir not listed here. > > As the same happened with RZ/A1H I'm not too concerned, but I'm > > listing some of there here nonetheless > > Back when you were working on this, I discussed with the guys back in > Japan. While in the table there are multiple pins that show > 'bi-directional capable', they don't all need the bi-dir bit set. This was the list of > peripheral: > I2C, MDIO (ETH), MMC, SDHI, QSPI, Data (D0-D31) Oh, I just received the list of pin in txt format iirc and didn't noted down which peripherals where actually bi-dir > > > > P2 has several TIOC pins in mode 4 and SSI in mode 2 > > Port 2??? > Port 2, mode 4 in the RZ/A1L manual only has IRQ7,6,5,2,1 That was mode #3, sorry about this. > > > I thought we decided on that TIOC pins are ones that are manually set to > either input (input-enable, output-enable) by the users Device Tree, so > they don't need to be put into a table. If input-enable or > output-enable is specified, then you manually set it to that direction. > Correct > As for SSI, the manual says only the SSITx pins need bi-dir, not the > SSIRx pins. Looking at RZ/A1H tables you're right. I wonder why in RZ/A1L SSI pins are listed as bidir and in RZ/A1H they're not. > > > > TIOC in mode3, SKC3 and SCK1 in mode 5 > > > + > > > +static const struct rza1_bidir_pin rza1l_bidir_pins_p4[] = { > > > + { .pin = 1, .func = 4 }, > > > + { .pin = 2, .func = 2 }, > > > + { .pin = 3, .func = 2 }, > > > + { .pin = 6, .func = 2 }, > > > + { .pin = 7, .func = 2 }, > > > +}; > > > > Confused, you're here listing TIOC2B pin (pin 1 mode #4) but not TIOC1B > > (pin 0 mode #4) > > ET_MDIO in mode 4, which we list as bidir in RZ/A1H > > P4_1 mode 4 is ET_MDIO. And I keep counting modes from 0... > > TIOC1B is P4_0 mode 3. > TIOC2B is P4_1 Mode 3 So it's fine to leave TIOC out from this table. > > Are you looking at Table 41.19 in the Rev.3.00 version of the RZ/A1L > Hardware Manual? > > > > +static const struct rza1_swio_pin rza1l_swio_pins[] = { > > > + { .port = 2, .pin = 8, .func = 2, .input = 0 }, > > > + { .port = 5, .pin = 6, .func = 3, .input = 0 }, > > > + { .port = 6, .pin = 6, .func = 3, .input = 0 }, > > > + { .port = 6, .pin = 10, .func = 3, .input = 0 }, > > > + { .port = 7, .pin = 10, .func = 2, .input = 0 }, > > > + { .port = 8, .pin = 2, .func = 3, .input = 0 }, > > > +}; > > > > I count 20 SWIO pins > > > > TIOC[0-4][A-D] > > SSITx[0-3] > > WDTOVF > > I thought we said TIOC didn't need to be in a table because the user has > to specify them anyway because the user has to choose if he wants them > as input or output, so if he does that, then we automatically know it's > a swio pin. Hmm, maybe I have to go back and look at what the driver > ended up implementing. > Can confirm TIOC pins are not listed in SWIO table in RZ/A1H. Thanks for double checking. Your source from Japan is more authoritative than manuals and defintely more accurate than me ;) Thanks j > > Chris
Hi Jacopo, On Tuesday, October 03, 2017 1, jacopo mondi wrote: > > As for SSI, the manual says only the SSITx pins need bi-dir, not the > > SSIRx pins. > > Looking at RZ/A1H tables you're right. I wonder why in RZ/A1L SSI pins > are listed as bidir and in RZ/A1H they're not. Sorry, to be clear, SSITx pins have nothing to do with "bi-dir". Those were pins that you have to add to the rza1l_swio_pins[] array because they must have to have both PIPCn.PIPC and PM bits set to '0' (ie, forced to output). However, there is no SSITxD5 pin in RZ/A1L. RZ/A1H: SSITxD0, SSITxD1, SSITxD3, SSITxD5 RZ/A1L: SSITxD0, SSITxD1, SSITxD3 > > I thought we said TIOC didn't need to be in a table because the user has > > to specify them anyway because the user has to choose if he wants them > > as input or output, so if he does that, then we automatically know it's > > a swio pin. Hmm, maybe I have to go back and look at what the driver > > ended up implementing. > > > > Can confirm TIOC pins are not listed in SWIO table in RZ/A1H. Yes, that 'are' listed in the table. But for a good reason. The TIOC pins can be uses as an output (PWM for example) or as a input (input capture for example), so they are not really a fixed direction. That is why the user needs to specify how they plan to use them in the DT. And...that's also why it doesn't make sense to put them in the rza1l_swio_pins[] array (we have no idea how they want to use them). So, the hardware manual has the TIOC pins in the SWIO table to tell the user 'hey, you have to manually set these pins to either input or output yourself because we can't magically know what you want to use them for. But... The reason for the rza1l_swio_pins[] array is for weird pin configurations that are fixed (like LCD LVDS output pins need to be configured as inputs). > Thanks for double checking. Your source from Japan is more > authoritative than manuals and defintely more accurate than me ;) So, after all that, I don't think we found any issues with my patch. Correct??? Thanks! Chris
Hi Chris, On Tue, Oct 03, 2017 at 03:40:51PM +0000, Chris Brandt wrote: > Hi Jacopo, > > > Yes, that 'are' listed in the table. But for a good reason. > > The TIOC pins can be uses as an output (PWM for example) or as a input > (input capture for example), so they are not really a fixed direction. > That is why the user needs to specify how they plan to use them in the DT. > And...that's also why it doesn't make sense to put them in the > rza1l_swio_pins[] array (we have no idea how they want to use them). > > So, the hardware manual has the TIOC pins in the SWIO table to tell the > user 'hey, you have to manually set these pins to either input or output > yourself because we can't magically know what you want to use them for. > > But... > The reason for the rza1l_swio_pins[] array is for weird pin > configurations that are fixed (like LCD LVDS output pins need to be configured as > inputs). > > > > Thanks for double checking. Your source from Japan is more > > authoritative than manuals and defintely more accurate than me ;) > > So, after all that, I don't think we found any issues with my patch. > Correct??? Correct! :) After your clarification above, for both patches: Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org> Thanks j > > > Thanks! > > Chris >
diff --git a/drivers/pinctrl/pinctrl-rza1.c b/drivers/pinctrl/pinctrl-rza1.c index 04d058706b80..717c0f4449a0 100644 --- a/drivers/pinctrl/pinctrl-rza1.c +++ b/drivers/pinctrl/pinctrl-rza1.c @@ -302,6 +302,134 @@ static const struct rza1_pinmux_conf rza1h_pmx_conf = { .swio_entries = rza1h_swio_entries, }; +/* ---------------------------------------------------------------------------- + * RZ/A1L (r7s72102) pinmux flags + */ + +static const struct rza1_bidir_pin rza1l_bidir_pins_p1[] = { + { .pin = 0, .func = 1 }, + { .pin = 1, .func = 1 }, + { .pin = 2, .func = 1 }, + { .pin = 3, .func = 1 }, + { .pin = 4, .func = 1 }, + { .pin = 5, .func = 1 }, + { .pin = 6, .func = 1 }, + { .pin = 7, .func = 1 }, +}; + +static const struct rza1_bidir_pin rza1l_bidir_pins_p3[] = { + { .pin = 0, .func = 2 }, + { .pin = 1, .func = 2 }, + { .pin = 2, .func = 2 }, + { .pin = 4, .func = 2 }, + { .pin = 5, .func = 2 }, + { .pin = 10, .func = 2 }, + { .pin = 11, .func = 2 }, + { .pin = 12, .func = 2 }, + { .pin = 13, .func = 2 }, +}; + +static const struct rza1_bidir_pin rza1l_bidir_pins_p4[] = { + { .pin = 1, .func = 4 }, + { .pin = 2, .func = 2 }, + { .pin = 3, .func = 2 }, + { .pin = 6, .func = 2 }, + { .pin = 7, .func = 2 }, +}; + +static const struct rza1_bidir_pin rza1l_bidir_pins_p5[] = { + { .pin = 0, .func = 1 }, + { .pin = 1, .func = 1 }, + { .pin = 2, .func = 1 }, + { .pin = 3, .func = 1 }, + { .pin = 4, .func = 1 }, + { .pin = 5, .func = 1 }, + { .pin = 6, .func = 1 }, + { .pin = 7, .func = 1 }, + { .pin = 8, .func = 1 }, + { .pin = 9, .func = 1 }, + { .pin = 10, .func = 1 }, + { .pin = 11, .func = 1 }, + { .pin = 12, .func = 1 }, + { .pin = 13, .func = 1 }, + { .pin = 14, .func = 1 }, + { .pin = 15, .func = 1 }, + { .pin = 0, .func = 2 }, + { .pin = 1, .func = 2 }, + { .pin = 2, .func = 2 }, + { .pin = 3, .func = 2 }, +}; + +static const struct rza1_bidir_pin rza1l_bidir_pins_p6[] = { + { .pin = 0, .func = 1 }, + { .pin = 1, .func = 1 }, + { .pin = 2, .func = 1 }, + { .pin = 3, .func = 1 }, + { .pin = 4, .func = 1 }, + { .pin = 5, .func = 1 }, + { .pin = 6, .func = 1 }, + { .pin = 7, .func = 1 }, + { .pin = 8, .func = 1 }, + { .pin = 9, .func = 1 }, + { .pin = 10, .func = 1 }, + { .pin = 11, .func = 1 }, + { .pin = 12, .func = 1 }, + { .pin = 13, .func = 1 }, + { .pin = 14, .func = 1 }, + { .pin = 15, .func = 1 }, +}; + +static const struct rza1_bidir_pin rza1l_bidir_pins_p7[] = { + { .pin = 2, .func = 2 }, + { .pin = 3, .func = 2 }, + { .pin = 5, .func = 2 }, + { .pin = 6, .func = 2 }, + { .pin = 7, .func = 2 }, + { .pin = 2, .func = 3 }, + { .pin = 3, .func = 3 }, + { .pin = 5, .func = 3 }, + { .pin = 6, .func = 3 }, + { .pin = 7, .func = 3 }, +}; + +static const struct rza1_bidir_pin rza1l_bidir_pins_p9[] = { + { .pin = 1, .func = 2 }, + { .pin = 0, .func = 3 }, + { .pin = 1, .func = 3 }, + { .pin = 3, .func = 3 }, + { .pin = 4, .func = 3 }, + { .pin = 5, .func = 3 }, +}; + +static const struct rza1_swio_pin rza1l_swio_pins[] = { + { .port = 2, .pin = 8, .func = 2, .input = 0 }, + { .port = 5, .pin = 6, .func = 3, .input = 0 }, + { .port = 6, .pin = 6, .func = 3, .input = 0 }, + { .port = 6, .pin = 10, .func = 3, .input = 0 }, + { .port = 7, .pin = 10, .func = 2, .input = 0 }, + { .port = 8, .pin = 2, .func = 3, .input = 0 }, +}; + +static const struct rza1_bidir_entry rza1l_bidir_entries[RZA1_NPORTS] = { + [1] = { ARRAY_SIZE(rza1l_bidir_pins_p1), rza1l_bidir_pins_p1 }, + [3] = { ARRAY_SIZE(rza1l_bidir_pins_p3), rza1l_bidir_pins_p3 }, + [4] = { ARRAY_SIZE(rza1l_bidir_pins_p4), rza1l_bidir_pins_p4 }, + [5] = { ARRAY_SIZE(rza1l_bidir_pins_p4), rza1l_bidir_pins_p5 }, + [6] = { ARRAY_SIZE(rza1l_bidir_pins_p6), rza1l_bidir_pins_p6 }, + [7] = { ARRAY_SIZE(rza1l_bidir_pins_p7), rza1l_bidir_pins_p7 }, + [9] = { ARRAY_SIZE(rza1l_bidir_pins_p9), rza1l_bidir_pins_p9 }, +}; + +static const struct rza1_swio_entry rza1l_swio_entries[] = { + [0] = { ARRAY_SIZE(rza1h_swio_pins), rza1h_swio_pins }, +}; + +/* RZ/A1L (r7s72102x) pinmux flags table */ +static const struct rza1_pinmux_conf rza1l_pmx_conf = { + .bidir_entries = rza1l_bidir_entries, + .swio_entries = rza1l_swio_entries, +}; + /* ---------------------------------------------------------------------------- * RZ/A1 types */ @@ -1283,9 +1411,15 @@ static int rza1_pinctrl_probe(struct platform_device *pdev) static const struct of_device_id rza1_pinctrl_of_match[] = { { + /* RZ/A1H, RZ/A1M */ .compatible = "renesas,r7s72100-ports", .data = &rza1h_pmx_conf, }, + { + /* RZ/A1L */ + .compatible = "renesas,r7s72102-ports", + .data = &rza1l_pmx_conf, + }, { } };
Aspects like the number of ports and the location where peripherals are brought out differ between the RZ/A1H and RZ/A1L. Signed-off-by: Chris Brandt <chris.brandt@renesas.com> --- drivers/pinctrl/pinctrl-rza1.c | 134 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 134 insertions(+)