| Message ID | 20180124002738.32338-1-martin.blumenstingl@googlemail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v1] pinctrl: meson: meson8b: fix requesting GPIOs greater than GPIOZ_3 | expand |
On Wed, 2018-01-24 at 01:27 +0100, Martin Blumenstingl wrote: > Meson8b is a cost reduced variant of the Meson8 SoC. It's package size > is smaller than Meson8. > Unfortunately there are a few key differences which cannot be seen > without close inspection of the code and the public S805 data-sheet: > - the GPIOX bank is missing the GPIOX_12, GPIOX_13, GPIOX_14 and > GPIOX_15 GPIOs > - the GPIOY bank is missing the GPIOY_2, GPIOY_4, GPIOY_5, GPIOY_15 and > GPIOY_16 GPIOs > - the GPIODV bank is missing all GPIOs except GPIODV_9, GPIODV_24, > GPIODV_25, GPIODV_26, GPIODV_27, GPIODV_28 and GPIODV_29 > - the GPIOZ bank is missing completely > - there is a new GPIO bank called "DIF" > - (all other pads exist on both, Meson8 and Meson8b) > > The pinctrl-meson driver internally uses struct meson_bank, which > assumes that the GPIOs are continuous, without any holes in between. > This also matches with how the registers work: > - GPIOX_0 for example uses bit 0 for switching this pad between > input/output, configuring pull-up/down, etc. > - GPIOX_16 uses bit 16 for switching this pad between input/output, > configuring pull-up/down/, etc > GPIOX_16 uses bit 16 even though GPIOX12..15 don't exist. (This is > probably the reason why Meson8b inherits the dt-bindings - with all GPIO > IDs - from Meson8) > This means that Meson8b only has 83 actual GPIO lines. Without any holes > there would be 130 GPIO lines in total (120 are inherited from Meson8 > plus 10 new from the DIF bank). > > The pinctrl framework handles holes in the pin list fine, which can be > seen in debugfs: > $ cat /sys/kernel/debug/pinctrl/c1109880.pinctrl/pins > pin 9 (GPIOX_9) c1109880.pinctrl > pin 10 (GPIOX_10) c1109880.pinctrl > pin 11 (GPIOX_11) c1109880.pinctrl > pin 16 (GPIOX_16) c1109880.pinctrl > pin 17 (GPIOX_17) c1109880.pinctrl Hi Martin, While working on this controller a few weeks ago, I have seen that our current design does not tolerate having holes in the declared PINS. This is something that predate the changes mentioned here. I'm not a big fan of the this overrides_ngpio here, I'm sure it fixes your problem but it seems a bit hacky, don't you think ? I would prefer either of these 2 approach: * Update the binding and kill the necessary pins. We are still figuring out those chips, which is why the bindings are marked as un-stable. If sharing the binding with meson8 was a mistake, now is the time to fix it. * If meson8b is indeed a cost reduction, it is likely to be same the approach as s905x vs s905d: The D version is the same SoC with less ball pad but, as far as we know, the logic is still there on the silicium (same die), it is just not routed to the outside world. Both version share the same pinctrl driver (gxl). The pad not routed to the outside world can just be considered "internal pads" Long story short, if meson8 and meson8b share the same bindings, maybe they should also share the driver. If they can't share the driver because they are definitely incompatible, maybe they should not share the same bindings > > GPIOs which don't exist cannot be requested either ("base" of the GPIO > controller is 382): > $ echo 394 > /sys/class/gpio/export > meson8b-pinctrl c1109880.pinctrl: pin 12 is not registered so it cannot be requested > meson8b-pinctrl c1109880.pinctrl: pin-12 (cbus-banks:394) status -22 > > Unfortunately GPIOs greater GPIOZ_3 (whose ID is 83 - as a reminder: > this is exactly the number of actual GPIO lines on Meson8b) cannot be > requested. Using CARD_6 (ID 100, "base of the GPIO controller is 382) as > an example: > $ echo 482 > /sys/class/gpio/export > export_store: invalid GPIO 482 > > The problem here is that struct gpio_chip's "ngpio" is set to 83. Thus > requesting a GPIO higher than 83 fails. Commit 984cffdeaeb7ea ("pinctrl: > Fix gpio/pin mapping for Meson8b") fixed this problem a while ago, by > setting "ngpio" to 130. Unfortunately this broke again while cleaning up > the pinctrl-meson driver in commit db80f0e158e621 ("pinctrl: meson: get > rid of unneeded domain structures"), which started using > ARRAY_SIZE(meson8b_cbus_pins) - which evaluates to 83 - as "ngpio". > > This is now fixed by introducing a new "override_ngpio" field in struct > meson_pinctrl_data. This value is passed to struct gpio_chip's "ngpio" > when set - if override_ngpio is <= 0 then num_pins is used (which works > for all other Meson SoCs, except Meson8b). > The definitions in "include/dt-bindings/gpio/meson8b-gpio.h" were not > changed. Cleaning up these definitions would allow us to get rid of the > GPIOZ definitions (which are inherited from Meson8). However, we still > need to handle GPIO banks with holes (such as GPIOX), so this include is > not touched for now (touching it could also break the device-tree ABI). > > Meson8b's AO GPIO controller is not affected by this issue since it does > not have any holes in it - only the CBUS GPIO controller is affected. > > This was initially seen by Linus Lüssing who was preparing SD card > support on Odroid-C1 which uses CARD_6 as "card detect" GPIO. > > Fixes: db80f0e158e621 ("pinctrl: meson: get rid of unneeded domain structures") > Reported-by: Linus Lüssing <linus.luessing@c0d3.blue> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > --- > drivers/pinctrl/meson/pinctrl-meson.c | 7 ++++++- > drivers/pinctrl/meson/pinctrl-meson.h | 1 + > drivers/pinctrl/meson/pinctrl-meson8b.c | 14 ++++++++++++++ > 3 files changed, 21 insertions(+), 1 deletion(-) > > diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c > index 29a458da78db..1befe67fd187 100644 > --- a/drivers/pinctrl/meson/pinctrl-meson.c > +++ b/drivers/pinctrl/meson/pinctrl-meson.c > @@ -413,7 +413,12 @@ static int meson_gpiolib_register(struct meson_pinctrl *pc) > pc->chip.get = meson_gpio_get; > pc->chip.set = meson_gpio_set; > pc->chip.base = -1; > - pc->chip.ngpio = pc->data->num_pins; > + > + if (pc->data->override_ngpio > 0) > + pc->chip.ngpio = pc->data->override_ngpio; > + else > + pc->chip.ngpio = pc->data->num_pins; > + > pc->chip.can_sleep = false; > pc->chip.of_node = pc->of_node; > pc->chip.of_gpio_n_cells = 2; > diff --git a/drivers/pinctrl/meson/pinctrl-meson.h b/drivers/pinctrl/meson/pinctrl-meson.h > index 183b6e471635..afd7efaad9bb 100644 > --- a/drivers/pinctrl/meson/pinctrl-meson.h > +++ b/drivers/pinctrl/meson/pinctrl-meson.h > @@ -108,6 +108,7 @@ struct meson_pinctrl_data { > struct meson_bank *banks; > unsigned int num_banks; > const struct pinmux_ops *pmx_ops; > + unsigned int override_ngpio; > }; > > struct meson_pinctrl { > diff --git a/drivers/pinctrl/meson/pinctrl-meson8b.c b/drivers/pinctrl/meson/pinctrl-meson8b.c > index 5bd808dc81e1..05bb4c3143d9 100644 > --- a/drivers/pinctrl/meson/pinctrl-meson8b.c > +++ b/drivers/pinctrl/meson/pinctrl-meson8b.c > @@ -916,6 +916,20 @@ static struct meson_pinctrl_data meson8b_cbus_pinctrl_data = { > .num_funcs = ARRAY_SIZE(meson8b_cbus_functions), > .num_banks = ARRAY_SIZE(meson8b_cbus_banks), > .pmx_ops = &meson8_pmx_ops, > + /* > + * there are holes in the meson8b_cbus_pins mapping because we are > + * mapping the GPIO identifiers to register bits. on Meson8b some bits > + * (and thus the corresponding pads) are not used on the SoC > + * (presumably because they were skipped because Meson8b is a cost > + * reduced variant of Meson8 and it didn't make sense to create a whole > + * new register layout for this variant). > + * this informs the GPIO framework know about the number of GPIOs we > + * have, including the pins which are not available on Meson8b (= the > + * holes). The pinctrl/GPIO framework knows the holes because it checks > + * all pinctrl_pin_desc in meson8b_cbus_pins and reports an error as > + * soon as a GPIO is requested which is not part of meson8b_cbus_pins. > + */ > + .override_ngpio = 130, > }; > > static struct meson_pinctrl_data meson8b_aobus_pinctrl_data = { -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jerome, On Wed, Jan 24, 2018 at 9:28 AM, Jerome Brunet <jbrunet@baylibre.com> wrote: > On Wed, 2018-01-24 at 01:27 +0100, Martin Blumenstingl wrote: >> Meson8b is a cost reduced variant of the Meson8 SoC. It's package size >> is smaller than Meson8. >> Unfortunately there are a few key differences which cannot be seen >> without close inspection of the code and the public S805 data-sheet: >> - the GPIOX bank is missing the GPIOX_12, GPIOX_13, GPIOX_14 and >> GPIOX_15 GPIOs >> - the GPIOY bank is missing the GPIOY_2, GPIOY_4, GPIOY_5, GPIOY_15 and >> GPIOY_16 GPIOs >> - the GPIODV bank is missing all GPIOs except GPIODV_9, GPIODV_24, >> GPIODV_25, GPIODV_26, GPIODV_27, GPIODV_28 and GPIODV_29 >> - the GPIOZ bank is missing completely >> - there is a new GPIO bank called "DIF" >> - (all other pads exist on both, Meson8 and Meson8b) >> >> The pinctrl-meson driver internally uses struct meson_bank, which >> assumes that the GPIOs are continuous, without any holes in between. >> This also matches with how the registers work: >> - GPIOX_0 for example uses bit 0 for switching this pad between >> input/output, configuring pull-up/down, etc. >> - GPIOX_16 uses bit 16 for switching this pad between input/output, >> configuring pull-up/down/, etc >> GPIOX_16 uses bit 16 even though GPIOX12..15 don't exist. (This is >> probably the reason why Meson8b inherits the dt-bindings - with all GPIO >> IDs - from Meson8) >> This means that Meson8b only has 83 actual GPIO lines. Without any holes >> there would be 130 GPIO lines in total (120 are inherited from Meson8 >> plus 10 new from the DIF bank). >> >> The pinctrl framework handles holes in the pin list fine, which can be >> seen in debugfs: >> $ cat /sys/kernel/debug/pinctrl/c1109880.pinctrl/pins >> pin 9 (GPIOX_9) c1109880.pinctrl >> pin 10 (GPIOX_10) c1109880.pinctrl >> pin 11 (GPIOX_11) c1109880.pinctrl >> pin 16 (GPIOX_16) c1109880.pinctrl >> pin 17 (GPIOX_17) c1109880.pinctrl > > Hi Martin, > > While working on this controller a few weeks ago, I have seen that our current > design does not tolerate having holes in the declared PINS. This is something > that predate the changes mentioned here. > > I'm not a big fan of the this overrides_ngpio here, I'm sure it fixes your > problem but it seems a bit hacky, don't you think ? this is the reason why I sent it as RFC - I'm open to better solutions! > I would prefer either of these 2 approach: > > * Update the binding and kill the necessary pins. We are still figuring out > those chips, which is why the bindings are marked as un-stable. If sharing the > binding with meson8 was a mistake, now is the time to fix it. let's say we produce a header file which does not contain GPIOX_12: this would be nice since it gives compile errors if non-existent GPIOs are used (instead of failing only at run-time) however, the problem is meson8b_cbus_banks (annotated here to make it easier to read): BANK("X", /* name */ GPIOX_0, GPIOX_21, /* first and last GPIO */ 97, 118, /* first and last IRQ */ 4, 0, /* pull-up/down enable register and bit offset */ 4, 0, /* pull-up/down configuration register and bit offset */ 0, 0, /* input/output direction register and bit offset */ 1, 0, /* output value register and bit offset */ 2, 0), /* input value register and bit offset */ let's say we want to configure GPIOX_0 as an input: - we need to find the "offset" of this pad within the bank -> 0 - find the input/output direction register -> 0 - find the bit in that register: bit 0 + pad offset 0 = 0 - we clear bit 0 in register 0 with the current code configuring GPIOX_16 as input works like this: - we need to find the "offset" of this pad within the bank -> 16 - find the input/output direction register -> 0 - find the bit in that register: bit 0 + pad offset 16 = 16 - we clear bit 16 in register 0 let's assume that we removed GPIOX_12...15 from Meson8b GPIOX_16 could now have the same value that was assigned to GPIOX_12 before (= 12) but how do we now know the offset of GPIOX_16? if we don't consider it in the calculation above we would end up writing to bit 12 (instead of bit 16). we could still cleanup the header file but leave the IDs of the existing GPIOs untouched (so GPIOX_16 would still be 16). however, that means that this patch is still needed because our last ID would be DIF_4_N (= value 129) again (so with holes our total count is still 130). with that we wouldn't even break the DT ABI > * If meson8b is indeed a cost reduction, it is likely to be same the approach as > s905x vs s905d: The D version is the same SoC with less ball pad but, as far as > we know, the logic is still there on the silicium (same die), it is just not > routed to the outside world. Both version share the same pinctrl driver (gxl). > The pad not routed to the outside world can just be considered "internal pads" there are a few differences between Meson8 and Meson8b, see also [0]: - Meson8 (and Meson8m2) CPU cores: 4x Cortex-A9 - Meson8b CPU cores: 4x Cortex-A5 - Meson8 (and Meson8m2) GPU: Mali-450MP8 - Meson8b GPU: Mali-450MP4 - Meson8 (and Meson8m2) package size: 19mm x 19 mm, LFBGA - Meson8b package size: 12mm x 12 mm, LFBGA the pinctrl register layout seems to be *mostly* compatible between Meson8 and Meson8b. unfortunately it's not entirely compatible, here is one example: - Meson8: GROUP(uart_tx_b1, 6, 23), - Meson8b: GROUP(uart_tx_b1, 6, 19), - Meson8: GROUP(eth_mdc, 6, 5), - Meson8b: GROUP(eth_mdc, 6, 9), > Long story short, if meson8 and meson8b share the same bindings, maybe they > should also share the driver. If they can't share the driver because they are > definitely incompatible, maybe they should not share the same bindings based on what I have seen so far I believe that they should not share the same bindings >> >> GPIOs which don't exist cannot be requested either ("base" of the GPIO >> controller is 382): >> $ echo 394 > /sys/class/gpio/export >> meson8b-pinctrl c1109880.pinctrl: pin 12 is not registered so it cannot be requested >> meson8b-pinctrl c1109880.pinctrl: pin-12 (cbus-banks:394) status -22 >> >> Unfortunately GPIOs greater GPIOZ_3 (whose ID is 83 - as a reminder: >> this is exactly the number of actual GPIO lines on Meson8b) cannot be >> requested. Using CARD_6 (ID 100, "base of the GPIO controller is 382) as >> an example: >> $ echo 482 > /sys/class/gpio/export >> export_store: invalid GPIO 482 >> >> The problem here is that struct gpio_chip's "ngpio" is set to 83. Thus >> requesting a GPIO higher than 83 fails. Commit 984cffdeaeb7ea ("pinctrl: >> Fix gpio/pin mapping for Meson8b") fixed this problem a while ago, by >> setting "ngpio" to 130. Unfortunately this broke again while cleaning up >> the pinctrl-meson driver in commit db80f0e158e621 ("pinctrl: meson: get >> rid of unneeded domain structures"), which started using >> ARRAY_SIZE(meson8b_cbus_pins) - which evaluates to 83 - as "ngpio". >> >> This is now fixed by introducing a new "override_ngpio" field in struct >> meson_pinctrl_data. This value is passed to struct gpio_chip's "ngpio" >> when set - if override_ngpio is <= 0 then num_pins is used (which works >> for all other Meson SoCs, except Meson8b). >> The definitions in "include/dt-bindings/gpio/meson8b-gpio.h" were not >> changed. Cleaning up these definitions would allow us to get rid of the >> GPIOZ definitions (which are inherited from Meson8). However, we still >> need to handle GPIO banks with holes (such as GPIOX), so this include is >> not touched for now (touching it could also break the device-tree ABI). >> >> Meson8b's AO GPIO controller is not affected by this issue since it does >> not have any holes in it - only the CBUS GPIO controller is affected. >> >> This was initially seen by Linus Lüssing who was preparing SD card >> support on Odroid-C1 which uses CARD_6 as "card detect" GPIO. >> >> Fixes: db80f0e158e621 ("pinctrl: meson: get rid of unneeded domain structures") >> Reported-by: Linus Lüssing <linus.luessing@c0d3.blue> >> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> >> --- >> drivers/pinctrl/meson/pinctrl-meson.c | 7 ++++++- >> drivers/pinctrl/meson/pinctrl-meson.h | 1 + >> drivers/pinctrl/meson/pinctrl-meson8b.c | 14 ++++++++++++++ >> 3 files changed, 21 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c >> index 29a458da78db..1befe67fd187 100644 >> --- a/drivers/pinctrl/meson/pinctrl-meson.c >> +++ b/drivers/pinctrl/meson/pinctrl-meson.c >> @@ -413,7 +413,12 @@ static int meson_gpiolib_register(struct meson_pinctrl *pc) >> pc->chip.get = meson_gpio_get; >> pc->chip.set = meson_gpio_set; >> pc->chip.base = -1; >> - pc->chip.ngpio = pc->data->num_pins; >> + >> + if (pc->data->override_ngpio > 0) >> + pc->chip.ngpio = pc->data->override_ngpio; >> + else >> + pc->chip.ngpio = pc->data->num_pins; >> + >> pc->chip.can_sleep = false; >> pc->chip.of_node = pc->of_node; >> pc->chip.of_gpio_n_cells = 2; >> diff --git a/drivers/pinctrl/meson/pinctrl-meson.h b/drivers/pinctrl/meson/pinctrl-meson.h >> index 183b6e471635..afd7efaad9bb 100644 >> --- a/drivers/pinctrl/meson/pinctrl-meson.h >> +++ b/drivers/pinctrl/meson/pinctrl-meson.h >> @@ -108,6 +108,7 @@ struct meson_pinctrl_data { >> struct meson_bank *banks; >> unsigned int num_banks; >> const struct pinmux_ops *pmx_ops; >> + unsigned int override_ngpio; >> }; >> >> struct meson_pinctrl { >> diff --git a/drivers/pinctrl/meson/pinctrl-meson8b.c b/drivers/pinctrl/meson/pinctrl-meson8b.c >> index 5bd808dc81e1..05bb4c3143d9 100644 >> --- a/drivers/pinctrl/meson/pinctrl-meson8b.c >> +++ b/drivers/pinctrl/meson/pinctrl-meson8b.c >> @@ -916,6 +916,20 @@ static struct meson_pinctrl_data meson8b_cbus_pinctrl_data = { >> .num_funcs = ARRAY_SIZE(meson8b_cbus_functions), >> .num_banks = ARRAY_SIZE(meson8b_cbus_banks), >> .pmx_ops = &meson8_pmx_ops, >> + /* >> + * there are holes in the meson8b_cbus_pins mapping because we are >> + * mapping the GPIO identifiers to register bits. on Meson8b some bits >> + * (and thus the corresponding pads) are not used on the SoC >> + * (presumably because they were skipped because Meson8b is a cost >> + * reduced variant of Meson8 and it didn't make sense to create a whole >> + * new register layout for this variant). >> + * this informs the GPIO framework know about the number of GPIOs we >> + * have, including the pins which are not available on Meson8b (= the >> + * holes). The pinctrl/GPIO framework knows the holes because it checks >> + * all pinctrl_pin_desc in meson8b_cbus_pins and reports an error as >> + * soon as a GPIO is requested which is not part of meson8b_cbus_pins. >> + */ >> + .override_ngpio = 130, >> }; >> >> static struct meson_pinctrl_data meson8b_aobus_pinctrl_data = { > [0] https://www.cnx-software.com/2014/04/25/amlogic-stb-socs-comparison-aml8726-mx-s802-s805-and-s812/ -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2018-01-24 at 11:10 +0100, Martin Blumenstingl wrote: > Hi Jerome, > > On Wed, Jan 24, 2018 at 9:28 AM, Jerome Brunet <jbrunet@baylibre.com> wrote: > > On Wed, 2018-01-24 at 01:27 +0100, Martin Blumenstingl wrote: > > > Meson8b is a cost reduced variant of the Meson8 SoC. It's package size > > > is smaller than Meson8. > > > Unfortunately there are a few key differences which cannot be seen > > > without close inspection of the code and the public S805 data-sheet: > > > - the GPIOX bank is missing the GPIOX_12, GPIOX_13, GPIOX_14 and > > > GPIOX_15 GPIOs > > > - the GPIOY bank is missing the GPIOY_2, GPIOY_4, GPIOY_5, GPIOY_15 and > > > GPIOY_16 GPIOs > > > - the GPIODV bank is missing all GPIOs except GPIODV_9, GPIODV_24, > > > GPIODV_25, GPIODV_26, GPIODV_27, GPIODV_28 and GPIODV_29 > > > - the GPIOZ bank is missing completely > > > - there is a new GPIO bank called "DIF" > > > - (all other pads exist on both, Meson8 and Meson8b) > > > > > > The pinctrl-meson driver internally uses struct meson_bank, which > > > assumes that the GPIOs are continuous, without any holes in between. > > > This also matches with how the registers work: > > > - GPIOX_0 for example uses bit 0 for switching this pad between > > > input/output, configuring pull-up/down, etc. > > > - GPIOX_16 uses bit 16 for switching this pad between input/output, > > > configuring pull-up/down/, etc > > > GPIOX_16 uses bit 16 even though GPIOX12..15 don't exist. (This is > > > probably the reason why Meson8b inherits the dt-bindings - with all GPIO > > > IDs - from Meson8) > > > This means that Meson8b only has 83 actual GPIO lines. Without any holes > > > there would be 130 GPIO lines in total (120 are inherited from Meson8 > > > plus 10 new from the DIF bank). > > > > > > The pinctrl framework handles holes in the pin list fine, which can be > > > seen in debugfs: > > > $ cat /sys/kernel/debug/pinctrl/c1109880.pinctrl/pins > > > pin 9 (GPIOX_9) c1109880.pinctrl > > > pin 10 (GPIOX_10) c1109880.pinctrl > > > pin 11 (GPIOX_11) c1109880.pinctrl > > > pin 16 (GPIOX_16) c1109880.pinctrl > > > pin 17 (GPIOX_17) c1109880.pinctrl > > > > Hi Martin, > > > > While working on this controller a few weeks ago, I have seen that our current > > design does not tolerate having holes in the declared PINS. This is something > > that predate the changes mentioned here. > > > > I'm not a big fan of the this overrides_ngpio here, I'm sure it fixes your > > problem but it seems a bit hacky, don't you think ? > > this is the reason why I sent it as RFC - I'm open to better solutions! > > > I would prefer either of these 2 approach: > > > > * Update the binding and kill the necessary pins. We are still figuring out > > those chips, which is why the bindings are marked as un-stable. If sharing the > > binding with meson8 was a mistake, now is the time to fix it. > > let's say we produce a header file which does not contain GPIOX_12: > this would be nice since it gives compile errors if non-existent GPIOs > are used (instead of failing only at run-time) > > however, the problem is meson8b_cbus_banks (annotated here to make it > easier to read): > BANK("X", /* name */ > GPIOX_0, GPIOX_21, /* first and last GPIO */ > 97, 118, /* first and last IRQ */ > 4, 0, /* pull-up/down enable register and bit offset */ > 4, 0, /* pull-up/down configuration register and bit offset */ > 0, 0, /* input/output direction register and bit offset */ > 1, 0, /* output value register and bit offset */ > 2, 0), /* input value register and bit offset */ > > let's say we want to configure GPIOX_0 as an input: > - we need to find the "offset" of this pad within the bank -> 0 > - find the input/output direction register -> 0 > - find the bit in that register: bit 0 + pad offset 0 = 0 > - we clear bit 0 in register 0 > > with the current code configuring GPIOX_16 as input works like this: > - we need to find the "offset" of this pad within the bank -> 16 > - find the input/output direction register -> 0 > - find the bit in that register: bit 0 + pad offset 16 = 16 > - we clear bit 16 in register 0 If offset calculation is really the problem, nothing is stopping you from the splitting BANK in two. With your example of GPIOX_12...15 missing: BANK("X1", /* name */ GPIOX_0, GPIOX_11, /* first and last GPIO */ 97, 108, /* first and last IRQ */ 4, 0, /* pull-up/down enable register and bit offset */ 4, 0, /* pull-up/down configuration register and bit offset */ 0, 0, /* input/output direction register and bit offset */ 1, 0, /* output value register and bit offset */ 2, 0), /* input value register and bit offset */ BANK("X2", /* name */ GPIOX_16, GPIOX_21, /* first and last GPIO */ 113, 118, /* first and last IRQ */ X, Y, /* pull-up/down enable register and bit offset */ X, Y, /* pull-up/down configuration register and bit offset */ Z, XX, /* input/output direction register and bit offset */ FOO, 0, /* output value register and bit offset */ BAR, 0), /* input value register and bit offset */ > > let's assume that we removed GPIOX_12...15 from Meson8b > GPIOX_16 could now have the same value that was assigned to GPIOX_12 > before (= 12) > but how do we now know the offset of GPIOX_16? > if we don't consider it in the calculation above we would end up > writing to bit 12 (instead of bit 16). > > we could still cleanup the header file but leave the IDs of the > existing GPIOs untouched (so GPIOX_16 would still be 16). I don't really get the point of doing so, especially is this patch is still needed in the end. > however, that means that this patch is still needed because our last > ID would be DIF_4_N (= value 129) again (so with holes our total count > is still 130). > with that we wouldn't even break the DT ABI While I prefer changing the bindings over the 'over_ngpio' tweak, it is not my favorite solution. > > > * If meson8b is indeed a cost reduction, it is likely to be same the approach as > > s905x vs s905d: The D version is the same SoC with less ball pad but, as far as > > we know, the logic is still there on the silicium (same die), it is just not > > routed to the outside world. Both version share the same pinctrl driver (gxl). > > The pad not routed to the outside world can just be considered "internal pads" > > there are a few differences between Meson8 and Meson8b, see also [0]: > - Meson8 (and Meson8m2) CPU cores: 4x Cortex-A9 > - Meson8b CPU cores: 4x Cortex-A5 > - Meson8 (and Meson8m2) GPU: Mali-450MP8 > - Meson8b GPU: Mali-450MP4 > - Meson8 (and Meson8m2) package size: 19mm x 19 mm, LFBGA > - Meson8b package size: 12mm x 12 mm, LFBGA > > the pinctrl register layout seems to be *mostly* compatible between > Meson8 and Meson8b. > unfortunately it's not entirely compatible, here is one example: > - Meson8: GROUP(uart_tx_b1, 6, 23), > - Meson8b: GROUP(uart_tx_b1, 6, 19), > - Meson8: GROUP(eth_mdc, 6, 5), > - Meson8b: GROUP(eth_mdc, 6, 9), > > > Long story short, if meson8 and meson8b share the same bindings, maybe they > > should also share the driver. If they can't share the driver because they are > > definitely incompatible, maybe they should not share the same bindings > > based on what I have seen so far I believe that they should not share > the same bindings It looks to me to be same gpio/pinconf IP with a few pads not exposed to the outside world, so I think they could share the bindings, and apparently fair part of the driver data. Apparently amlogic tweaked a bit the pinmux. With the current architecture of our pinctrl driver, it should be fairly easy to register 2 drivers sharing everything but the GROUP table (and maybe the FUNCS if necessary) If possible, I would be more in favor of this last solution, as we could kill one the pinctrl-meson8.c file, which are mostly a copy/paste of one another. While fixing you issue, we would end up with less code to maintain and save a bit of memory. Do you see any other big issue with this approach ? > > > > > > > GPIOs which don't exist cannot be requested either ("base" of the GPIO > > > controller is 382): > > > $ echo 394 > /sys/class/gpio/export > > > meson8b-pinctrl c1109880.pinctrl: pin 12 is not registered so it cannot be requested > > > meson8b-pinctrl c1109880.pinctrl: pin-12 (cbus-banks:394) status -22 > > > > > > Unfortunately GPIOs greater GPIOZ_3 (whose ID is 83 - as a reminder: > > > this is exactly the number of actual GPIO lines on Meson8b) cannot be > > > requested. Using CARD_6 (ID 100, "base of the GPIO controller is 382) as > > > an example: > > > $ echo 482 > /sys/class/gpio/export > > > export_store: invalid GPIO 482 > > > > > > The problem here is that struct gpio_chip's "ngpio" is set to 83. Thus > > > requesting a GPIO higher than 83 fails. Commit 984cffdeaeb7ea ("pinctrl: > > > Fix gpio/pin mapping for Meson8b") fixed this problem a while ago, by > > > setting "ngpio" to 130. Unfortunately this broke again while cleaning up > > > the pinctrl-meson driver in commit db80f0e158e621 ("pinctrl: meson: get > > > rid of unneeded domain structures"), which started using > > > ARRAY_SIZE(meson8b_cbus_pins) - which evaluates to 83 - as "ngpio". > > > > > > This is now fixed by introducing a new "override_ngpio" field in struct > > > meson_pinctrl_data. This value is passed to struct gpio_chip's "ngpio" > > > when set - if override_ngpio is <= 0 then num_pins is used (which works > > > for all other Meson SoCs, except Meson8b). > > > The definitions in "include/dt-bindings/gpio/meson8b-gpio.h" were not > > > changed. Cleaning up these definitions would allow us to get rid of the > > > GPIOZ definitions (which are inherited from Meson8). However, we still > > > need to handle GPIO banks with holes (such as GPIOX), so this include is > > > not touched for now (touching it could also break the device-tree ABI). > > > > > > Meson8b's AO GPIO controller is not affected by this issue since it does > > > not have any holes in it - only the CBUS GPIO controller is affected. > > > > > > This was initially seen by Linus Lüssing who was preparing SD card > > > support on Odroid-C1 which uses CARD_6 as "card detect" GPIO. > > > > > > Fixes: db80f0e158e621 ("pinctrl: meson: get rid of unneeded domain structures") > > > Reported-by: Linus Lüssing <linus.luessing@c0d3.blue> > > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > > --- > > > drivers/pinctrl/meson/pinctrl-meson.c | 7 ++++++- > > > drivers/pinctrl/meson/pinctrl-meson.h | 1 + > > > drivers/pinctrl/meson/pinctrl-meson8b.c | 14 ++++++++++++++ > > > 3 files changed, 21 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c > > > index 29a458da78db..1befe67fd187 100644 > > > --- a/drivers/pinctrl/meson/pinctrl-meson.c > > > +++ b/drivers/pinctrl/meson/pinctrl-meson.c > > > @@ -413,7 +413,12 @@ static int meson_gpiolib_register(struct meson_pinctrl *pc) > > > pc->chip.get = meson_gpio_get; > > > pc->chip.set = meson_gpio_set; > > > pc->chip.base = -1; > > > - pc->chip.ngpio = pc->data->num_pins; > > > + > > > + if (pc->data->override_ngpio > 0) > > > + pc->chip.ngpio = pc->data->override_ngpio; > > > + else > > > + pc->chip.ngpio = pc->data->num_pins; > > > + > > > pc->chip.can_sleep = false; > > > pc->chip.of_node = pc->of_node; > > > pc->chip.of_gpio_n_cells = 2; > > > diff --git a/drivers/pinctrl/meson/pinctrl-meson.h b/drivers/pinctrl/meson/pinctrl-meson.h > > > index 183b6e471635..afd7efaad9bb 100644 > > > --- a/drivers/pinctrl/meson/pinctrl-meson.h > > > +++ b/drivers/pinctrl/meson/pinctrl-meson.h > > > @@ -108,6 +108,7 @@ struct meson_pinctrl_data { > > > struct meson_bank *banks; > > > unsigned int num_banks; > > > const struct pinmux_ops *pmx_ops; > > > + unsigned int override_ngpio; > > > }; > > > > > > struct meson_pinctrl { > > > diff --git a/drivers/pinctrl/meson/pinctrl-meson8b.c b/drivers/pinctrl/meson/pinctrl-meson8b.c > > > index 5bd808dc81e1..05bb4c3143d9 100644 > > > --- a/drivers/pinctrl/meson/pinctrl-meson8b.c > > > +++ b/drivers/pinctrl/meson/pinctrl-meson8b.c > > > @@ -916,6 +916,20 @@ static struct meson_pinctrl_data meson8b_cbus_pinctrl_data = { > > > .num_funcs = ARRAY_SIZE(meson8b_cbus_functions), > > > .num_banks = ARRAY_SIZE(meson8b_cbus_banks), > > > .pmx_ops = &meson8_pmx_ops, > > > + /* > > > + * there are holes in the meson8b_cbus_pins mapping because we are > > > + * mapping the GPIO identifiers to register bits. on Meson8b some bits > > > + * (and thus the corresponding pads) are not used on the SoC > > > + * (presumably because they were skipped because Meson8b is a cost > > > + * reduced variant of Meson8 and it didn't make sense to create a whole > > > + * new register layout for this variant). > > > + * this informs the GPIO framework know about the number of GPIOs we > > > + * have, including the pins which are not available on Meson8b (= the > > > + * holes). The pinctrl/GPIO framework knows the holes because it checks > > > + * all pinctrl_pin_desc in meson8b_cbus_pins and reports an error as > > > + * soon as a GPIO is requested which is not part of meson8b_cbus_pins. > > > + */ > > > + .override_ngpio = 130, > > > }; > > > > > > static struct meson_pinctrl_data meson8b_aobus_pinctrl_data = { > > [0] https://www.cnx-software.com/2014/04/25/amlogic-stb-socs-comparison-aml8726-mx-s802-s805-and-s812/ -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jerome, On Wed, Jan 24, 2018 at 3:52 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: > On Wed, 2018-01-24 at 11:10 +0100, Martin Blumenstingl wrote: >> Hi Jerome, >> >> On Wed, Jan 24, 2018 at 9:28 AM, Jerome Brunet <jbrunet@baylibre.com> wrote: >> > On Wed, 2018-01-24 at 01:27 +0100, Martin Blumenstingl wrote: >> > > Meson8b is a cost reduced variant of the Meson8 SoC. It's package size >> > > is smaller than Meson8. >> > > Unfortunately there are a few key differences which cannot be seen >> > > without close inspection of the code and the public S805 data-sheet: >> > > - the GPIOX bank is missing the GPIOX_12, GPIOX_13, GPIOX_14 and >> > > GPIOX_15 GPIOs >> > > - the GPIOY bank is missing the GPIOY_2, GPIOY_4, GPIOY_5, GPIOY_15 and >> > > GPIOY_16 GPIOs >> > > - the GPIODV bank is missing all GPIOs except GPIODV_9, GPIODV_24, >> > > GPIODV_25, GPIODV_26, GPIODV_27, GPIODV_28 and GPIODV_29 >> > > - the GPIOZ bank is missing completely >> > > - there is a new GPIO bank called "DIF" >> > > - (all other pads exist on both, Meson8 and Meson8b) >> > > >> > > The pinctrl-meson driver internally uses struct meson_bank, which >> > > assumes that the GPIOs are continuous, without any holes in between. >> > > This also matches with how the registers work: >> > > - GPIOX_0 for example uses bit 0 for switching this pad between >> > > input/output, configuring pull-up/down, etc. >> > > - GPIOX_16 uses bit 16 for switching this pad between input/output, >> > > configuring pull-up/down/, etc >> > > GPIOX_16 uses bit 16 even though GPIOX12..15 don't exist. (This is >> > > probably the reason why Meson8b inherits the dt-bindings - with all GPIO >> > > IDs - from Meson8) >> > > This means that Meson8b only has 83 actual GPIO lines. Without any holes >> > > there would be 130 GPIO lines in total (120 are inherited from Meson8 >> > > plus 10 new from the DIF bank). >> > > >> > > The pinctrl framework handles holes in the pin list fine, which can be >> > > seen in debugfs: >> > > $ cat /sys/kernel/debug/pinctrl/c1109880.pinctrl/pins >> > > pin 9 (GPIOX_9) c1109880.pinctrl >> > > pin 10 (GPIOX_10) c1109880.pinctrl >> > > pin 11 (GPIOX_11) c1109880.pinctrl >> > > pin 16 (GPIOX_16) c1109880.pinctrl >> > > pin 17 (GPIOX_17) c1109880.pinctrl >> > >> > Hi Martin, >> > >> > While working on this controller a few weeks ago, I have seen that our current >> > design does not tolerate having holes in the declared PINS. This is something >> > that predate the changes mentioned here. >> > >> > I'm not a big fan of the this overrides_ngpio here, I'm sure it fixes your >> > problem but it seems a bit hacky, don't you think ? >> >> this is the reason why I sent it as RFC - I'm open to better solutions! >> >> > I would prefer either of these 2 approach: >> > >> > * Update the binding and kill the necessary pins. We are still figuring out >> > those chips, which is why the bindings are marked as un-stable. If sharing the >> > binding with meson8 was a mistake, now is the time to fix it. >> >> let's say we produce a header file which does not contain GPIOX_12: >> this would be nice since it gives compile errors if non-existent GPIOs >> are used (instead of failing only at run-time) >> >> however, the problem is meson8b_cbus_banks (annotated here to make it >> easier to read): >> BANK("X", /* name */ >> GPIOX_0, GPIOX_21, /* first and last GPIO */ >> 97, 118, /* first and last IRQ */ >> 4, 0, /* pull-up/down enable register and bit offset */ >> 4, 0, /* pull-up/down configuration register and bit offset */ >> 0, 0, /* input/output direction register and bit offset */ >> 1, 0, /* output value register and bit offset */ >> 2, 0), /* input value register and bit offset */ >> >> let's say we want to configure GPIOX_0 as an input: >> - we need to find the "offset" of this pad within the bank -> 0 >> - find the input/output direction register -> 0 >> - find the bit in that register: bit 0 + pad offset 0 = 0 >> - we clear bit 0 in register 0 >> >> with the current code configuring GPIOX_16 as input works like this: >> - we need to find the "offset" of this pad within the bank -> 16 >> - find the input/output direction register -> 0 >> - find the bit in that register: bit 0 + pad offset 16 = 16 >> - we clear bit 16 in register 0 > > If offset calculation is really the problem, nothing is stopping you from the > splitting BANK in two. With your example of GPIOX_12...15 missing: > > BANK("X1", /* name */ > GPIOX_0, GPIOX_11, /* first and last GPIO */ > 97, 108, /* first and last IRQ */ > 4, 0, /* pull-up/down enable register and bit offset */ > 4, 0, /* pull-up/down configuration register and bit offset */ > 0, 0, /* input/output direction register and bit offset */ > 1, 0, /* output value register and bit offset */ > 2, 0), /* input value register and bit offset */ > > BANK("X2", /* name */ > GPIOX_16, GPIOX_21, /* first and last GPIO */ > 113, 118, /* first and last IRQ */ > X, Y, /* pull-up/down enable register and bit offset */ > X, Y, /* pull-up/down configuration register and bit offset */ > Z, XX, /* input/output direction register and bit offset */ > FOO, 0, /* output value register and bit offset */ > BAR, 0), /* input value register and bit offset */ nice thinking :) the only solution I could come up with was to change the MESON_PIN definition to something like: #define MESON_PIN(pad, pullen_bit, pull_bit, direction_bit, output_bit, input_bit, irq_num) that however means that we would have to change a lot of code in all pinctrl drivers >> >> let's assume that we removed GPIOX_12...15 from Meson8b >> GPIOX_16 could now have the same value that was assigned to GPIOX_12 >> before (= 12) >> but how do we now know the offset of GPIOX_16? >> if we don't consider it in the calculation above we would end up >> writing to bit 12 (instead of bit 16). >> >> we could still cleanup the header file but leave the IDs of the >> existing GPIOs untouched (so GPIOX_16 would still be 16). > > I don't really get the point of doing so, especially is this patch is still > needed in the end. > >> however, that means that this patch is still needed because our last >> ID would be DIF_4_N (= value 129) again (so with holes our total count >> is still 130). >> with that we wouldn't even break the DT ABI > > While I prefer changing the bindings over the 'over_ngpio' tweak, it is not my > favorite solution. > >> >> > * If meson8b is indeed a cost reduction, it is likely to be same the approach as >> > s905x vs s905d: The D version is the same SoC with less ball pad but, as far as >> > we know, the logic is still there on the silicium (same die), it is just not >> > routed to the outside world. Both version share the same pinctrl driver (gxl). >> > The pad not routed to the outside world can just be considered "internal pads" >> >> there are a few differences between Meson8 and Meson8b, see also [0]: >> - Meson8 (and Meson8m2) CPU cores: 4x Cortex-A9 >> - Meson8b CPU cores: 4x Cortex-A5 >> - Meson8 (and Meson8m2) GPU: Mali-450MP8 >> - Meson8b GPU: Mali-450MP4 >> - Meson8 (and Meson8m2) package size: 19mm x 19 mm, LFBGA >> - Meson8b package size: 12mm x 12 mm, LFBGA >> >> the pinctrl register layout seems to be *mostly* compatible between >> Meson8 and Meson8b. >> unfortunately it's not entirely compatible, here is one example: >> - Meson8: GROUP(uart_tx_b1, 6, 23), >> - Meson8b: GROUP(uart_tx_b1, 6, 19), >> - Meson8: GROUP(eth_mdc, 6, 5), >> - Meson8b: GROUP(eth_mdc, 6, 9), >> >> > Long story short, if meson8 and meson8b share the same bindings, maybe they >> > should also share the driver. If they can't share the driver because they are >> > definitely incompatible, maybe they should not share the same bindings >> >> based on what I have seen so far I believe that they should not share >> the same bindings > > It looks to me to be same gpio/pinconf IP with a few pads not exposed to the > outside world, so I think they could share the bindings, and apparently fair > part of the driver data. > > Apparently amlogic tweaked a bit the pinmux. With the current architecture of > our pinctrl driver, it should be fairly easy to register 2 drivers sharing > everything but the GROUP table (and maybe the FUNCS if necessary) > > If possible, I would be more in favor of this last solution, as we could kill > one the pinctrl-meson8.c file, which are mostly a copy/paste of one another. > While fixing you issue, we would end up with less code to maintain and save a > bit of memory. > > Do you see any other big issue with this approach ? I would have to go through the whole list of pads to see what the actual differences are a quick comparison (diff between pinctrl-meson8.c and pinctrl-meson8b.c) shows that the largest differences are: - GPIOZ bank which exists on Meson8 but not on Meson8b - DIF bank which exists on Meson8b but not on Meson8 - (the other changes seem to be mostly either sorting differences, or the "holes" where pads were removed from Meson8b) if we do this we would have to keep the following information separated: - groups (and with that: num_groups) - banks (and with that: num_banks) - because the IRQ numbers are different between Meson8 and Meson8b - as you already mentioned: maybe functions (and with that: num_functions) with this approach we would lose the ability to check whether a pin exists for a specific SoC (as far as I can see) requesting GPIOX_12 on Meson8b would not show any errors, but obviously nothing can be done with it (since it's not routed to a pad) do we have the same issue on GXL (with S905X, S905D and S912)? Regards Martin -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 24, 2018 at 01:27:38AM +0100, Martin Blumenstingl wrote: > Meson8b's AO GPIO controller is not affected by this issue since it does > not have any holes in it - only the CBUS GPIO controller is affected. > > This was initially seen by Linus Lüssing who was preparing SD card > support on Odroid-C1 which uses CARD_6 as "card detect" GPIO. > > Fixes: db80f0e158e621 ("pinctrl: meson: get rid of unneeded domain structures") > Reported-by: Linus Lüssing <linus.luessing@c0d3.blue> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> Hi Martin, Just wanted to confirm that this indeed fixes the splats and crashes I was experiencing. And I was able to successfully read/write from/to a microSD card. Also card detect worked fine, I could successfully unplug, replug and then read from it again. Here some dmesg from a successful card detection: [ 512.584687] mmcblk0: error -84 sending status command, retrying [ 512.586985] mmcblk0: timed out sending r/w cmd command, card status 0xd00 [ 512.591756] mmcblk0: status not valid, retrying timeout [ 638.652986] mmc0: card 1234 removed [ 656.722455] mmc0: new high speed SDHC card at address 1234 [ 656.724667] mmcblk0: mmc0:1234 SA16G 14.4 GiB [ 656.730312] mmcblk0: p1 p2 There error happened when typing "$ sync" after the write. However the written file seemed fine in the end. The address 1234 also seemed like a weird coincidence. But maybe that's just a bug in my DT changes. I'll post the DT changes I had been experimenting with as an RFC to linux-amlogic shortly. Regards, Linus -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jerome, On Wed, Jan 24, 2018 at 6:54 PM, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > Hi Jerome, > > On Wed, Jan 24, 2018 at 3:52 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: >> On Wed, 2018-01-24 at 11:10 +0100, Martin Blumenstingl wrote: >>> Hi Jerome, >>> >>> On Wed, Jan 24, 2018 at 9:28 AM, Jerome Brunet <jbrunet@baylibre.com> wrote: >>> > On Wed, 2018-01-24 at 01:27 +0100, Martin Blumenstingl wrote: >>> > > Meson8b is a cost reduced variant of the Meson8 SoC. It's package size >>> > > is smaller than Meson8. >>> > > Unfortunately there are a few key differences which cannot be seen >>> > > without close inspection of the code and the public S805 data-sheet: >>> > > - the GPIOX bank is missing the GPIOX_12, GPIOX_13, GPIOX_14 and >>> > > GPIOX_15 GPIOs >>> > > - the GPIOY bank is missing the GPIOY_2, GPIOY_4, GPIOY_5, GPIOY_15 and >>> > > GPIOY_16 GPIOs >>> > > - the GPIODV bank is missing all GPIOs except GPIODV_9, GPIODV_24, >>> > > GPIODV_25, GPIODV_26, GPIODV_27, GPIODV_28 and GPIODV_29 >>> > > - the GPIOZ bank is missing completely >>> > > - there is a new GPIO bank called "DIF" >>> > > - (all other pads exist on both, Meson8 and Meson8b) >>> > > >>> > > The pinctrl-meson driver internally uses struct meson_bank, which >>> > > assumes that the GPIOs are continuous, without any holes in between. >>> > > This also matches with how the registers work: >>> > > - GPIOX_0 for example uses bit 0 for switching this pad between >>> > > input/output, configuring pull-up/down, etc. >>> > > - GPIOX_16 uses bit 16 for switching this pad between input/output, >>> > > configuring pull-up/down/, etc >>> > > GPIOX_16 uses bit 16 even though GPIOX12..15 don't exist. (This is >>> > > probably the reason why Meson8b inherits the dt-bindings - with all GPIO >>> > > IDs - from Meson8) >>> > > This means that Meson8b only has 83 actual GPIO lines. Without any holes >>> > > there would be 130 GPIO lines in total (120 are inherited from Meson8 >>> > > plus 10 new from the DIF bank). >>> > > >>> > > The pinctrl framework handles holes in the pin list fine, which can be >>> > > seen in debugfs: >>> > > $ cat /sys/kernel/debug/pinctrl/c1109880.pinctrl/pins >>> > > pin 9 (GPIOX_9) c1109880.pinctrl >>> > > pin 10 (GPIOX_10) c1109880.pinctrl >>> > > pin 11 (GPIOX_11) c1109880.pinctrl >>> > > pin 16 (GPIOX_16) c1109880.pinctrl >>> > > pin 17 (GPIOX_17) c1109880.pinctrl >>> > >>> > Hi Martin, >>> > >>> > While working on this controller a few weeks ago, I have seen that our current >>> > design does not tolerate having holes in the declared PINS. This is something >>> > that predate the changes mentioned here. >>> > >>> > I'm not a big fan of the this overrides_ngpio here, I'm sure it fixes your >>> > problem but it seems a bit hacky, don't you think ? >>> >>> this is the reason why I sent it as RFC - I'm open to better solutions! >>> >>> > I would prefer either of these 2 approach: >>> > >>> > * Update the binding and kill the necessary pins. We are still figuring out >>> > those chips, which is why the bindings are marked as un-stable. If sharing the >>> > binding with meson8 was a mistake, now is the time to fix it. >>> >>> let's say we produce a header file which does not contain GPIOX_12: >>> this would be nice since it gives compile errors if non-existent GPIOs >>> are used (instead of failing only at run-time) >>> >>> however, the problem is meson8b_cbus_banks (annotated here to make it >>> easier to read): >>> BANK("X", /* name */ >>> GPIOX_0, GPIOX_21, /* first and last GPIO */ >>> 97, 118, /* first and last IRQ */ >>> 4, 0, /* pull-up/down enable register and bit offset */ >>> 4, 0, /* pull-up/down configuration register and bit offset */ >>> 0, 0, /* input/output direction register and bit offset */ >>> 1, 0, /* output value register and bit offset */ >>> 2, 0), /* input value register and bit offset */ >>> >>> let's say we want to configure GPIOX_0 as an input: >>> - we need to find the "offset" of this pad within the bank -> 0 >>> - find the input/output direction register -> 0 >>> - find the bit in that register: bit 0 + pad offset 0 = 0 >>> - we clear bit 0 in register 0 >>> >>> with the current code configuring GPIOX_16 as input works like this: >>> - we need to find the "offset" of this pad within the bank -> 16 >>> - find the input/output direction register -> 0 >>> - find the bit in that register: bit 0 + pad offset 16 = 16 >>> - we clear bit 16 in register 0 >> >> If offset calculation is really the problem, nothing is stopping you from the >> splitting BANK in two. With your example of GPIOX_12...15 missing: >> >> BANK("X1", /* name */ >> GPIOX_0, GPIOX_11, /* first and last GPIO */ >> 97, 108, /* first and last IRQ */ >> 4, 0, /* pull-up/down enable register and bit offset */ >> 4, 0, /* pull-up/down configuration register and bit offset */ >> 0, 0, /* input/output direction register and bit offset */ >> 1, 0, /* output value register and bit offset */ >> 2, 0), /* input value register and bit offset */ >> >> BANK("X2", /* name */ >> GPIOX_16, GPIOX_21, /* first and last GPIO */ >> 113, 118, /* first and last IRQ */ >> X, Y, /* pull-up/down enable register and bit offset */ >> X, Y, /* pull-up/down configuration register and bit offset */ >> Z, XX, /* input/output direction register and bit offset */ >> FOO, 0, /* output value register and bit offset */ >> BAR, 0), /* input value register and bit offset */ > nice thinking :) > the only solution I could come up with was to change the MESON_PIN > definition to something like: > #define MESON_PIN(pad, pullen_bit, pull_bit, direction_bit, > output_bit, input_bit, irq_num) > > that however means that we would have to change a lot of code in all > pinctrl drivers > >>> >>> let's assume that we removed GPIOX_12...15 from Meson8b >>> GPIOX_16 could now have the same value that was assigned to GPIOX_12 >>> before (= 12) >>> but how do we now know the offset of GPIOX_16? >>> if we don't consider it in the calculation above we would end up >>> writing to bit 12 (instead of bit 16). >>> >>> we could still cleanup the header file but leave the IDs of the >>> existing GPIOs untouched (so GPIOX_16 would still be 16). >> >> I don't really get the point of doing so, especially is this patch is still >> needed in the end. >> >>> however, that means that this patch is still needed because our last >>> ID would be DIF_4_N (= value 129) again (so with holes our total count >>> is still 130). >>> with that we wouldn't even break the DT ABI >> >> While I prefer changing the bindings over the 'over_ngpio' tweak, it is not my >> favorite solution. >> >>> >>> > * If meson8b is indeed a cost reduction, it is likely to be same the approach as >>> > s905x vs s905d: The D version is the same SoC with less ball pad but, as far as >>> > we know, the logic is still there on the silicium (same die), it is just not >>> > routed to the outside world. Both version share the same pinctrl driver (gxl). >>> > The pad not routed to the outside world can just be considered "internal pads" >>> >>> there are a few differences between Meson8 and Meson8b, see also [0]: >>> - Meson8 (and Meson8m2) CPU cores: 4x Cortex-A9 >>> - Meson8b CPU cores: 4x Cortex-A5 >>> - Meson8 (and Meson8m2) GPU: Mali-450MP8 >>> - Meson8b GPU: Mali-450MP4 >>> - Meson8 (and Meson8m2) package size: 19mm x 19 mm, LFBGA >>> - Meson8b package size: 12mm x 12 mm, LFBGA >>> >>> the pinctrl register layout seems to be *mostly* compatible between >>> Meson8 and Meson8b. >>> unfortunately it's not entirely compatible, here is one example: >>> - Meson8: GROUP(uart_tx_b1, 6, 23), >>> - Meson8b: GROUP(uart_tx_b1, 6, 19), >>> - Meson8: GROUP(eth_mdc, 6, 5), >>> - Meson8b: GROUP(eth_mdc, 6, 9), >>> >>> > Long story short, if meson8 and meson8b share the same bindings, maybe they >>> > should also share the driver. If they can't share the driver because they are >>> > definitely incompatible, maybe they should not share the same bindings >>> >>> based on what I have seen so far I believe that they should not share >>> the same bindings >> >> It looks to me to be same gpio/pinconf IP with a few pads not exposed to the >> outside world, so I think they could share the bindings, and apparently fair >> part of the driver data. >> >> Apparently amlogic tweaked a bit the pinmux. With the current architecture of >> our pinctrl driver, it should be fairly easy to register 2 drivers sharing >> everything but the GROUP table (and maybe the FUNCS if necessary) >> >> If possible, I would be more in favor of this last solution, as we could kill >> one the pinctrl-meson8.c file, which are mostly a copy/paste of one another. >> While fixing you issue, we would end up with less code to maintain and save a >> bit of memory. >> >> Do you see any other big issue with this approach ? > I would have to go through the whole list of pads to see what the > actual differences are I tried to automate this as far as possible with the attached script you can use it like this: - first get the gpio.c files from the Amlogic 3.10 kernel for Meson8 and Meson8b: [0] and [1] - ./list-meson8-pinmux-reg-bits.sh path/to/meson8/gpio.c | sort -u > meson8.txt - ./list-meson8-pinmux-reg-bits.sh path/to/meson8b/gpio.c | sort -u > meson8b.txt - diff -Naur meson8.txt meson8b.txt > diff.patch - (the resulting diff is attached) - compare the Meson8 documentation (found in a .csv file, see [2]) with the public Meson8b datasheet (see [3]) the major differences are: - DIF_TTL bank only exists on Meson8b - GPIOZ bank only exists on Meson8 - HDMI_TTL bank only exists on Meson8b (not supported by our mainline driver) - some of the Ethernet pins are using the GPIOH bank on Meson8b while Meson8 has these in the GPIOZ bank - Meson8b has various SPI pins in the GPIOH bank while Meson8 has them in the GPIOZ bank - Meson8 only has pins for one TSin interface, Meson8b seems to support two TSin interfaces smaller differences are: - the LCD_* pins (maybe even all of the LCD support) in GPIODV only exists on Meson8 - Meson8b has the I2C_A pins on GPIODV_24 and GPIODV_25 (Meson8 has multiple pins for this in the GPIOZ bank) - Meson8b has the I2C_B pins on GPIODV_26 and GPIODV_27 (Meson8 has these on GPIOZ_2 and GPIOZ_3) - Meson8b has the I2C_C pins on GPIODV_28 and GPIODV_29 (Meson8 two chices: either GPIOY_0 and GPIOY_1 or GPIOZ_4 and GPIOZ_5) - the XTAL_24M_OUT signal uses GPIODV_29 on Meson8b but GPIOX_11 on Meson8 - Meson8b can output UART_CTS_B on GPIOX_10 - Meson8 supports GPIODV_26 instead - Meson8b can output UART_RTS_B on GPIOX_20 - Meson8 supports GPIODV_27 instead - Meson8b can output SPI_SS0 on GPIOX_20 - Meson8 supports GPIOZ_9 instead not checked yet: - Meson8b seems to have a few more bits in BOOT_8 BOOT_10 and BOOT_18 - Meson8b seems to have a few more bits in GPIOAO_2, GPIOAO_3, GPIOAO_6, GPIOAO_7, GPIOAO_9, GPIOAO_10 and GPIOAO_13 - GPIOY_0, GPIOX_4, GPIOX_5, GPIOX_6, GPIOX_7, GPIOX_8, GPIOX_9 - the changes (there are quite a few) in the GPIOY bank notes for other pins I have randomly checked: - Meson8b's gpio.c has a function for GPIOX_0 at reg9[24] - this seems undocumented - Meson8b's gpio.c has a function for GPIOX_1 at reg9[23] - this seems undocumented - Meson8b's gpio.c has a function for GPIOX_2 at reg9[22] - this seems undocumented - Meson8b's gpio.c has a function for GPIOX_3 at reg9[21] - this seems undocumented I'm not sure if we should try to create a unified pinctrl driver for Meson8 and Meson8b so far we have: - meson_pmx_group are roughly 67% identical (meson_pmx_func depend on meson_pmx_group) - different pinctrl_pin_desc (due to different GPIO banks) - different IRQ numbering so I wonder how much we would actually save by creating a unified Meson8/Meson8b pinctrl driver. as a side-note: I found out that Meson8m2 has 10 pins which each add one function compared to Meson8, see: [4] I would integrate these into the Meson8 driver (if we keep separate drivers for Meson8 and Meson8b) when needed, because these definitely don't justify adding a new pinctrl driver Regards Martin [0] https://github.com/endlessm/linux-meson/blob/5cb4882cdda584878a29132aeb9a90497a121df9/arch/arm/mach-meson8/gpio.c [1] https://github.com/endlessm/linux-meson/blob/5cb4882cdda584878a29132aeb9a90497a121df9/arch/arm/mach-meson8b/gpio.c [2] https://github.com/endlessm/linux-meson/blob/5cb4882cdda584878a29132aeb9a90497a121df9/arch/arm/mach-meson8/tool/m8_gpio.csv [3] https://dn.odroid.com/S805/Datasheet/S805_Datasheet%20V0.8%2020150126.pdf [4] https://github.com/endlessm/linux-meson/blob/5cb4882cdda584878a29132aeb9a90497a121df9/arch/arm/mach-meson8/gpio.c#L242 --- meson8-processed.txt 2018-02-17 18:01:35.617738060 +0100 +++ meson8b-processed.txt 2018-02-17 18:01:43.772869250 +0100 @@ -2,6 +2,8 @@ BOOT_0 P_PIN_MUX_REG(29@6) BOOT_0 P_PIN_MUX_REG(30@4) BOOT_10 P_PIN_MUX_REG(17@2) +BOOT_10 P_PIN_MUX_REG(18@7) +BOOT_10 P_PIN_MUX_REG(30@6) BOOT_11 P_PIN_MUX_REG(1@5) BOOT_11 P_PIN_MUX_REG(21@2) BOOT_12 P_PIN_MUX_REG(20@2) @@ -17,6 +19,7 @@ BOOT_17 P_PIN_MUX_REG(24@6) BOOT_17 P_PIN_MUX_REG(26@4) BOOT_18 P_PIN_MUX_REG(0@5) +BOOT_18 P_PIN_MUX_REG(28@2) BOOT_1 P_PIN_MUX_REG(26@2) BOOT_1 P_PIN_MUX_REG(28@6) BOOT_1 P_PIN_MUX_REG(29@4) @@ -34,7 +37,9 @@ BOOT_6 P_PIN_MUX_REG(28@4) BOOT_7 P_PIN_MUX_REG(26@2) BOOT_7 P_PIN_MUX_REG(28@4) +BOOT_8 P_PIN_MUX_REG(19@7) BOOT_8 P_PIN_MUX_REG(25@2) +BOOT_8 P_PIN_MUX_REG(31@6) BOOT_9 P_PIN_MUX_REG(24@2) CARD_0 P_PIN_MUX_REG(14@2) CARD_0 P_PIN_MUX_REG(6@2) @@ -52,15 +57,42 @@ CARD_5 P_PIN_MUX_REG(6@2) CARD_5 P_PIN_MUX_REG(7@8) CARD_5 P_PIN_MUX_REG(9@8) +DIF_TTL_0_N P_PIN_MUX_REG(1@6) +DIF_TTL_0_N P_PIN_MUX_REG(24@0) +DIF_TTL_0_P P_PIN_MUX_REG(0@6) +DIF_TTL_0_P P_PIN_MUX_REG(24@0) +DIF_TTL_1_N P_PIN_MUX_REG(24@0) +DIF_TTL_1_N P_PIN_MUX_REG(3@6) +DIF_TTL_1_P P_PIN_MUX_REG(24@0) +DIF_TTL_1_P P_PIN_MUX_REG(2@6) +DIF_TTL_2_N P_PIN_MUX_REG(23@7) +DIF_TTL_2_N P_PIN_MUX_REG(25@0) +DIF_TTL_2_N P_PIN_MUX_REG(5@6) +DIF_TTL_2_P P_PIN_MUX_REG(22@7) +DIF_TTL_2_P P_PIN_MUX_REG(25@0) +DIF_TTL_2_P P_PIN_MUX_REG(4@6) +DIF_TTL_3_N P_PIN_MUX_REG(25@0) +DIF_TTL_3_N P_PIN_MUX_REG(8@6) +DIF_TTL_3_P P_PIN_MUX_REG(25@0) +DIF_TTL_3_P P_PIN_MUX_REG(6@6) +DIF_TTL_4_N P_PIN_MUX_REG(10@6) +DIF_TTL_4_N P_PIN_MUX_REG(25@0) +DIF_TTL_4_P P_PIN_MUX_REG(25@0) +DIF_TTL_4_P P_PIN_MUX_REG(9@6) GPIOAO_0 P_PIN_MUX_REG(12@10) GPIOAO_0 P_PIN_MUX_REG(26@10) +GPIOAO_10 P_PIN_MUX_REG(14@1) GPIOAO_10 P_PIN_MUX_REG(28@10) GPIOAO_11 P_PIN_MUX_REG(27@10) GPIOAO_12 P_PIN_MUX_REG(17@10) +GPIOAO_13 P_PIN_MUX_REG(19@10) GPIOAO_13 P_PIN_MUX_REG(31@10) GPIOAO_1 P_PIN_MUX_REG(11@10) GPIOAO_1 P_PIN_MUX_REG(25@10) GPIOAO_2 P_PIN_MUX_REG(10@10) +GPIOAO_2 P_PIN_MUX_REG(8@10) +GPIOAO_3 P_PIN_MUX_REG(22@10) +GPIOAO_3 P_PIN_MUX_REG(7@10) GPIOAO_3 P_PIN_MUX_REG(9@10) GPIOAO_4 P_PIN_MUX_REG(2@10) GPIOAO_4 P_PIN_MUX_REG(24@10) @@ -68,31 +100,29 @@ GPIOAO_5 P_PIN_MUX_REG(1@10) GPIOAO_5 P_PIN_MUX_REG(23@10) GPIOAO_5 P_PIN_MUX_REG(5@10) +GPIOAO_6 P_PIN_MUX_REG(13@1) +GPIOAO_6 P_PIN_MUX_REG(16@10) GPIOAO_6 P_PIN_MUX_REG(18@10) GPIOAO_7 P_PIN_MUX_REG(0@10) +GPIOAO_7 P_PIN_MUX_REG(21@10) GPIOAO_8 P_PIN_MUX_REG(30@10) +GPIOAO_9 P_PIN_MUX_REG(15@1) GPIOAO_9 P_PIN_MUX_REG(29@10) GPIODV_0 P_PIN_MUX_REG(0@7) GPIODV_0 P_PIN_MUX_REG(1@0) GPIODV_0 P_PIN_MUX_REG(27@8) GPIODV_0 P_PIN_MUX_REG(6@0) GPIODV_10 P_PIN_MUX_REG(10@7) -GPIODV_10 P_PIN_MUX_REG(2@0) GPIODV_10 P_PIN_MUX_REG(6@0) GPIODV_11 P_PIN_MUX_REG(11@7) -GPIODV_11 P_PIN_MUX_REG(2@0) GPIODV_11 P_PIN_MUX_REG(6@0) GPIODV_12 P_PIN_MUX_REG(12@7) -GPIODV_12 P_PIN_MUX_REG(2@0) GPIODV_12 P_PIN_MUX_REG(6@0) GPIODV_13 P_PIN_MUX_REG(13@7) -GPIODV_13 P_PIN_MUX_REG(2@0) GPIODV_13 P_PIN_MUX_REG(6@0) GPIODV_14 P_PIN_MUX_REG(14@7) -GPIODV_14 P_PIN_MUX_REG(2@0) GPIODV_14 P_PIN_MUX_REG(6@0) GPIODV_15 P_PIN_MUX_REG(15@7) -GPIODV_15 P_PIN_MUX_REG(2@0) GPIODV_15 P_PIN_MUX_REG(6@0) GPIODV_16 P_PIN_MUX_REG(16@7) GPIODV_16 P_PIN_MUX_REG(25@8) @@ -101,60 +131,55 @@ GPIODV_17 P_PIN_MUX_REG(17@7) GPIODV_17 P_PIN_MUX_REG(5@0) GPIODV_17 P_PIN_MUX_REG(6@0) -GPIODV_18 P_PIN_MUX_REG(4@0) GPIODV_18 P_PIN_MUX_REG(6@0) -GPIODV_19 P_PIN_MUX_REG(4@0) GPIODV_19 P_PIN_MUX_REG(6@0) GPIODV_1 P_PIN_MUX_REG(1@0) GPIODV_1 P_PIN_MUX_REG(1@7) GPIODV_1 P_PIN_MUX_REG(6@0) -GPIODV_20 P_PIN_MUX_REG(4@0) GPIODV_20 P_PIN_MUX_REG(6@0) -GPIODV_21 P_PIN_MUX_REG(4@0) GPIODV_21 P_PIN_MUX_REG(6@0) -GPIODV_22 P_PIN_MUX_REG(4@0) GPIODV_22 P_PIN_MUX_REG(6@0) -GPIODV_23 P_PIN_MUX_REG(4@0) GPIODV_23 P_PIN_MUX_REG(6@0) GPIODV_24 P_PIN_MUX_REG(19@0) GPIODV_24 P_PIN_MUX_REG(21@0) GPIODV_24 P_PIN_MUX_REG(23@6) GPIODV_24 P_PIN_MUX_REG(24@8) +GPIODV_24 P_PIN_MUX_REG(31@9) GPIODV_24 P_PIN_MUX_REG(9@0) GPIODV_25 P_PIN_MUX_REG(18@0) GPIODV_25 P_PIN_MUX_REG(20@0) GPIODV_25 P_PIN_MUX_REG(22@6) GPIODV_25 P_PIN_MUX_REG(23@8) +GPIODV_25 P_PIN_MUX_REG(30@9) GPIODV_25 P_PIN_MUX_REG(8@0) GPIODV_26 P_PIN_MUX_REG(20@8) GPIODV_26 P_PIN_MUX_REG(21@6) GPIODV_26 P_PIN_MUX_REG(21@8) GPIODV_26 P_PIN_MUX_REG(22@8) +GPIODV_26 P_PIN_MUX_REG(29@9) GPIODV_26 P_PIN_MUX_REG(7@0) GPIODV_27 P_PIN_MUX_REG(10@0) GPIODV_27 P_PIN_MUX_REG(19@8) GPIODV_27 P_PIN_MUX_REG(20@6) GPIODV_27 P_PIN_MUX_REG(28@8) +GPIODV_27 P_PIN_MUX_REG(28@9) GPIODV_28 P_PIN_MUX_REG(26@3) GPIODV_28 P_PIN_MUX_REG(27@7) +GPIODV_28 P_PIN_MUX_REG(27@9) GPIODV_29 P_PIN_MUX_REG(25@3) +GPIODV_29 P_PIN_MUX_REG(25@7) GPIODV_29 P_PIN_MUX_REG(26@7) -GPIODV_2 P_PIN_MUX_REG(0@0) +GPIODV_29 P_PIN_MUX_REG(26@9) GPIODV_2 P_PIN_MUX_REG(2@7) GPIODV_2 P_PIN_MUX_REG(6@0) -GPIODV_3 P_PIN_MUX_REG(0@0) GPIODV_3 P_PIN_MUX_REG(3@7) GPIODV_3 P_PIN_MUX_REG(6@0) -GPIODV_4 P_PIN_MUX_REG(0@0) GPIODV_4 P_PIN_MUX_REG(4@7) GPIODV_4 P_PIN_MUX_REG(6@0) -GPIODV_5 P_PIN_MUX_REG(0@0) GPIODV_5 P_PIN_MUX_REG(5@7) GPIODV_5 P_PIN_MUX_REG(6@0) -GPIODV_6 P_PIN_MUX_REG(0@0) GPIODV_6 P_PIN_MUX_REG(6@0) GPIODV_6 P_PIN_MUX_REG(6@7) -GPIODV_7 P_PIN_MUX_REG(0@0) GPIODV_7 P_PIN_MUX_REG(6@0) GPIODV_7 P_PIN_MUX_REG(7@7) GPIODV_8 P_PIN_MUX_REG(26@8) @@ -166,26 +191,53 @@ GPIODV_9 P_PIN_MUX_REG(3@0) GPIODV_9 P_PIN_MUX_REG(6@0) GPIODV_9 P_PIN_MUX_REG(9@7) +GPIOH_0 P_PIN_MUX_REG(11@8) GPIOH_0 P_PIN_MUX_REG(26@1) +GPIOH_0 P_PIN_MUX_REG(4@1) +GPIOH_1 P_PIN_MUX_REG(12@8) +GPIOH_1 P_PIN_MUX_REG(2@1) GPIOH_1 P_PIN_MUX_REG(25@1) +GPIOH_1 P_PIN_MUX_REG(3@1) +GPIOH_2 P_PIN_MUX_REG(1@1) GPIOH_2 P_PIN_MUX_REG(24@1) +GPIOH_3 P_PIN_MUX_REG(0@1) GPIOH_3 P_PIN_MUX_REG(13@9) GPIOH_3 P_PIN_MUX_REG(23@1) +GPIOH_3 P_PIN_MUX_REG(27@5) GPIOH_4 P_PIN_MUX_REG(12@9) +GPIOH_4 P_PIN_MUX_REG(18@8) +GPIOH_4 P_PIN_MUX_REG(26@5) GPIOH_5 P_PIN_MUX_REG(11@9) +GPIOH_5 P_PIN_MUX_REG(15@6) +GPIOH_5 P_PIN_MUX_REG(18@8) +GPIOH_5 P_PIN_MUX_REG(21@7) +GPIOH_5 P_PIN_MUX_REG(25@5) GPIOH_6 P_PIN_MUX_REG(10@9) +GPIOH_6 P_PIN_MUX_REG(14@6) +GPIOH_6 P_PIN_MUX_REG(17@8) +GPIOH_6 P_PIN_MUX_REG(20@7) +GPIOH_6 P_PIN_MUX_REG(24@5) +GPIOH_7 P_PIN_MUX_REG(13@6) +GPIOH_7 P_PIN_MUX_REG(17@8) GPIOH_7 P_PIN_MUX_REG(3@4) +GPIOH_8 P_PIN_MUX_REG(12@6) +GPIOH_8 P_PIN_MUX_REG(16@8) GPIOH_8 P_PIN_MUX_REG(2@4) +GPIOH_9 P_PIN_MUX_REG(11@6) GPIOH_9 P_PIN_MUX_REG(1@4) -GPIOH_9 P_PIN_MUX_REG(19@3) +GPIOH_9 P_PIN_MUX_REG(16@8) GPIO_TEST_N P_PIN_MUX_REG(19@10) GPIOX_0 P_PIN_MUX_REG(14@5) +GPIOX_0 P_PIN_MUX_REG(24@9) GPIOX_0 P_PIN_MUX_REG(5@8) +GPIOX_10 P_PIN_MUX_REG(17@6) GPIOX_10 P_PIN_MUX_REG(19@9) GPIOX_10 P_PIN_MUX_REG(22@3) +GPIOX_10 P_PIN_MUX_REG(23@4) GPIOX_10 P_PIN_MUX_REG(31@7) +GPIOX_10 P_PIN_MUX_REG(8@3) GPIOX_11 P_PIN_MUX_REG(14@3) -GPIOX_11 P_PIN_MUX_REG(23@3) +GPIOX_11 P_PIN_MUX_REG(20@3) GPIOX_11 P_PIN_MUX_REG(30@7) GPIOX_11 P_PIN_MUX_REG(3@2) GPIOX_12 P_PIN_MUX_REG(13@3) @@ -211,30 +263,45 @@ GPIOX_19 P_PIN_MUX_REG(18@4) GPIOX_19 P_PIN_MUX_REG(6@4) GPIOX_1 P_PIN_MUX_REG(13@5) +GPIOX_1 P_PIN_MUX_REG(23@9) GPIOX_1 P_PIN_MUX_REG(4@8) +GPIOX_20 P_PIN_MUX_REG(16@6) +GPIOX_20 P_PIN_MUX_REG(25@4) +GPIOX_20 P_PIN_MUX_REG(9@3) GPIOX_2 P_PIN_MUX_REG(13@5) +GPIOX_2 P_PIN_MUX_REG(22@9) GPIOX_2 P_PIN_MUX_REG(3@8) GPIOX_3 P_PIN_MUX_REG(13@5) +GPIOX_3 P_PIN_MUX_REG(21@9) GPIOX_3 P_PIN_MUX_REG(2@8) GPIOX_4 P_PIN_MUX_REG(12@5) GPIOX_4 P_PIN_MUX_REG(17@4) +GPIOX_4 P_PIN_MUX_REG(29@5) GPIOX_4 P_PIN_MUX_REG(30@3) GPIOX_5 P_PIN_MUX_REG(12@5) GPIOX_5 P_PIN_MUX_REG(16@4) +GPIOX_5 P_PIN_MUX_REG(28@5) GPIOX_5 P_PIN_MUX_REG(29@3) GPIOX_6 P_PIN_MUX_REG(12@5) GPIOX_6 P_PIN_MUX_REG(15@4) GPIOX_6 P_PIN_MUX_REG(28@3) +GPIOX_6 P_PIN_MUX_REG(28@5) +GPIOX_6 P_PIN_MUX_REG(9@5) GPIOX_7 P_PIN_MUX_REG(12@5) GPIOX_7 P_PIN_MUX_REG(14@4) GPIOX_7 P_PIN_MUX_REG(27@3) +GPIOX_7 P_PIN_MUX_REG(28@5) +GPIOX_7 P_PIN_MUX_REG(8@5) GPIOX_8 P_PIN_MUX_REG(11@5) GPIOX_8 P_PIN_MUX_REG(1@8) +GPIOX_8 P_PIN_MUX_REG(19@6) +GPIOX_8 P_PIN_MUX_REG(22@4) +GPIOX_8 P_PIN_MUX_REG(6@3) GPIOX_9 P_PIN_MUX_REG(0@8) GPIOX_9 P_PIN_MUX_REG(10@5) -GPIOY_0 P_PIN_MUX_REG(10@1) -GPIOY_0 P_PIN_MUX_REG(15@1) -GPIOY_0 P_PIN_MUX_REG(19@1) +GPIOX_9 P_PIN_MUX_REG(18@6) +GPIOX_9 P_PIN_MUX_REG(24@4) +GPIOX_9 P_PIN_MUX_REG(7@3) GPIOY_0 P_PIN_MUX_REG(2@3) GPIOY_0 P_PIN_MUX_REG(9@9) GPIOY_10 P_PIN_MUX_REG(3@9) @@ -245,87 +312,40 @@ GPIOY_12 P_PIN_MUX_REG(5@3) GPIOY_13 P_PIN_MUX_REG(3@9) GPIOY_13 P_PIN_MUX_REG(5@3) +GPIOY_13 P_PIN_MUX_REG(7@5) GPIOY_14 P_PIN_MUX_REG(2@9) GPIOY_14 P_PIN_MUX_REG(5@3) -GPIOY_15 P_PIN_MUX_REG(1@9) -GPIOY_15 P_PIN_MUX_REG(5@3) -GPIOY_16 P_PIN_MUX_REG(0@9) +GPIOY_14 P_PIN_MUX_REG(6@5) +GPIOY_15 P_PIN_MUX_REG(15@9) GPIOY_16 P_PIN_MUX_REG(14@9) GPIOY_16 P_PIN_MUX_REG(29@7) -GPIOY_16 P_PIN_MUX_REG(5@3) GPIOY_1 P_PIN_MUX_REG(1@3) -GPIOY_1 P_PIN_MUX_REG(14@1) -GPIOY_1 P_PIN_MUX_REG(18@1) -GPIOY_1 P_PIN_MUX_REG(19@1) GPIOY_1 P_PIN_MUX_REG(8@9) GPIOY_2 P_PIN_MUX_REG(17@1) -GPIOY_2 P_PIN_MUX_REG(18@3) GPIOY_2 P_PIN_MUX_REG(8@1) -GPIOY_3 P_PIN_MUX_REG(11@3) +GPIOY_2 P_PIN_MUX_REG(9@1) GPIOY_3 P_PIN_MUX_REG(16@1) +GPIOY_3 P_PIN_MUX_REG(18@3) GPIOY_3 P_PIN_MUX_REG(7@1) -GPIOY_4 P_PIN_MUX_REG(25@4) +GPIOY_4 P_PIN_MUX_REG(10@1) +GPIOY_4 P_PIN_MUX_REG(19@1) GPIOY_4 P_PIN_MUX_REG(3@3) -GPIOY_4 P_PIN_MUX_REG(6@1) -GPIOY_5 P_PIN_MUX_REG(15@3) -GPIOY_5 P_PIN_MUX_REG(24@4) +GPIOY_5 P_PIN_MUX_REG(11@1) +GPIOY_5 P_PIN_MUX_REG(18@1) GPIOY_5 P_PIN_MUX_REG(5@1) -GPIOY_5 P_PIN_MUX_REG(7@9) -GPIOY_6 P_PIN_MUX_REG(16@3) -GPIOY_6 P_PIN_MUX_REG(23@4) -GPIOY_6 P_PIN_MUX_REG(3@1) -GPIOY_6 P_PIN_MUX_REG(4@1) +GPIOY_6 P_PIN_MUX_REG(5@3) GPIOY_6 P_PIN_MUX_REG(6@9) -GPIOY_7 P_PIN_MUX_REG(1@1) -GPIOY_7 P_PIN_MUX_REG(17@3) -GPIOY_7 P_PIN_MUX_REG(2@1) -GPIOY_7 P_PIN_MUX_REG(22@4) +GPIOY_7 P_PIN_MUX_REG(5@3) GPIOY_7 P_PIN_MUX_REG(5@9) -GPIOY_8 P_PIN_MUX_REG(0@1) GPIOY_8 P_PIN_MUX_REG(0@3) GPIOY_8 P_PIN_MUX_REG(4@9) -GPIOY_9 P_PIN_MUX_REG(11@1) GPIOY_9 P_PIN_MUX_REG(3@9) GPIOY_9 P_PIN_MUX_REG(4@3) -GPIOZ_0 P_PIN_MUX_REG(0@6) -GPIOZ_0 P_PIN_MUX_REG(16@9) -GPIOZ_0 P_PIN_MUX_REG(18@9) -GPIOZ_0 P_PIN_MUX_REG(25@7) -GPIOZ_0 P_PIN_MUX_REG(31@5) -GPIOZ_10 P_PIN_MUX_REG(12@8) -GPIOZ_10 P_PIN_MUX_REG(8@5) -GPIOZ_10 P_PIN_MUX_REG(8@6) -GPIOZ_11 P_PIN_MUX_REG(15@8) -GPIOZ_11 P_PIN_MUX_REG(7@5) -GPIOZ_11 P_PIN_MUX_REG(7@6) -GPIOZ_12 P_PIN_MUX_REG(14@8) -GPIOZ_12 P_PIN_MUX_REG(6@5) -GPIOZ_12 P_PIN_MUX_REG(6@6) -GPIOZ_13 P_PIN_MUX_REG(13@8) -GPIOZ_13 P_PIN_MUX_REG(5@6) -GPIOZ_14 P_PIN_MUX_REG(17@8) -GPIOZ_14 P_PIN_MUX_REG(17@9) -GPIOZ_1 P_PIN_MUX_REG(15@9) -GPIOZ_1 P_PIN_MUX_REG(1@6) -GPIOZ_1 P_PIN_MUX_REG(24@7) -GPIOZ_1 P_PIN_MUX_REG(30@5) -GPIOZ_2 P_PIN_MUX_REG(2@6) -GPIOZ_2 P_PIN_MUX_REG(27@5) -GPIOZ_3 P_PIN_MUX_REG(26@5) -GPIOZ_3 P_PIN_MUX_REG(3@6) -GPIOZ_4 P_PIN_MUX_REG(15@6) -GPIOZ_4 P_PIN_MUX_REG(25@5) -GPIOZ_5 P_PIN_MUX_REG(14@6) -GPIOZ_5 P_PIN_MUX_REG(24@5) -GPIOZ_6 P_PIN_MUX_REG(13@6) -GPIOZ_6 P_PIN_MUX_REG(21@3) -GPIOZ_7 P_PIN_MUX_REG(0@2) -GPIOZ_7 P_PIN_MUX_REG(12@6) -GPIOZ_7 P_PIN_MUX_REG(23@7) -GPIOZ_8 P_PIN_MUX_REG(10@6) -GPIOZ_8 P_PIN_MUX_REG(1@2) -GPIOZ_8 P_PIN_MUX_REG(22@7) -GPIOZ_8 P_PIN_MUX_REG(9@6) -GPIOZ_9 P_PIN_MUX_REG(11@6) -GPIOZ_9 P_PIN_MUX_REG(16@8) -GPIOZ_9 P_PIN_MUX_REG(9@5) +HDMI_TTL_0_N P_PIN_MUX_REG(22@0) +HDMI_TTL_0_P P_PIN_MUX_REG(22@0) +HDMI_TTL_1_N P_PIN_MUX_REG(22@0) +HDMI_TTL_1_P P_PIN_MUX_REG(22@0) +HDMI_TTL_2_N P_PIN_MUX_REG(22@0) +HDMI_TTL_2_P P_PIN_MUX_REG(22@0) +HDMI_TTL_CK_N P_PIN_MUX_REG(23@0) +HDMI_TTL_CK_P P_PIN_MUX_REG(23@0)
On Sat, 2018-02-17 at 20:44 +0100, Martin Blumenstingl wrote: > Hi Jerome, > > On Wed, Jan 24, 2018 at 6:54 PM, Martin Blumenstingl > <martin.blumenstingl@googlemail.com> wrote: > > Hi Jerome, > > > > On Wed, Jan 24, 2018 at 3:52 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: > > > On Wed, 2018-01-24 at 11:10 +0100, Martin Blumenstingl wrote: > > > > Hi Jerome, > > > > > > > > On Wed, Jan 24, 2018 at 9:28 AM, Jerome Brunet <jbrunet@baylibre.com> wrote: > > > > > On Wed, 2018-01-24 at 01:27 +0100, Martin Blumenstingl wrote: > > > > > > Meson8b is a cost reduced variant of the Meson8 SoC. It's package size > > > > > > is smaller than Meson8. > > > > > > Unfortunately there are a few key differences which cannot be seen > > > > > > without close inspection of the code and the public S805 data-sheet: > > > > > > - the GPIOX bank is missing the GPIOX_12, GPIOX_13, GPIOX_14 and > > > > > > GPIOX_15 GPIOs > > > > > > - the GPIOY bank is missing the GPIOY_2, GPIOY_4, GPIOY_5, GPIOY_15 and > > > > > > GPIOY_16 GPIOs > > > > > > - the GPIODV bank is missing all GPIOs except GPIODV_9, GPIODV_24, > > > > > > GPIODV_25, GPIODV_26, GPIODV_27, GPIODV_28 and GPIODV_29 > > > > > > - the GPIOZ bank is missing completely > > > > > > - there is a new GPIO bank called "DIF" > > > > > > - (all other pads exist on both, Meson8 and Meson8b) > > > > > > > > > > > > The pinctrl-meson driver internally uses struct meson_bank, which > > > > > > assumes that the GPIOs are continuous, without any holes in between. > > > > > > This also matches with how the registers work: > > > > > > - GPIOX_0 for example uses bit 0 for switching this pad between > > > > > > input/output, configuring pull-up/down, etc. > > > > > > - GPIOX_16 uses bit 16 for switching this pad between input/output, > > > > > > configuring pull-up/down/, etc > > > > > > GPIOX_16 uses bit 16 even though GPIOX12..15 don't exist. (This is > > > > > > probably the reason why Meson8b inherits the dt-bindings - with all GPIO > > > > > > IDs - from Meson8) > > > > > > This means that Meson8b only has 83 actual GPIO lines. Without any holes > > > > > > there would be 130 GPIO lines in total (120 are inherited from Meson8 > > > > > > plus 10 new from the DIF bank). > > > > > > > > > > > > The pinctrl framework handles holes in the pin list fine, which can be > > > > > > seen in debugfs: > > > > > > $ cat /sys/kernel/debug/pinctrl/c1109880.pinctrl/pins > > > > > > pin 9 (GPIOX_9) c1109880.pinctrl > > > > > > pin 10 (GPIOX_10) c1109880.pinctrl > > > > > > pin 11 (GPIOX_11) c1109880.pinctrl > > > > > > pin 16 (GPIOX_16) c1109880.pinctrl > > > > > > pin 17 (GPIOX_17) c1109880.pinctrl > > > > > > > > > > Hi Martin, > > > > > > > > > > While working on this controller a few weeks ago, I have seen that our current > > > > > design does not tolerate having holes in the declared PINS. This is something > > > > > that predate the changes mentioned here. > > > > > > > > > > I'm not a big fan of the this overrides_ngpio here, I'm sure it fixes your > > > > > problem but it seems a bit hacky, don't you think ? > > > > > > > > this is the reason why I sent it as RFC - I'm open to better solutions! > > > > > > > > > I would prefer either of these 2 approach: > > > > > > > > > > * Update the binding and kill the necessary pins. We are still figuring out > > > > > those chips, which is why the bindings are marked as un-stable. If sharing the > > > > > binding with meson8 was a mistake, now is the time to fix it. > > > > > > > > let's say we produce a header file which does not contain GPIOX_12: > > > > this would be nice since it gives compile errors if non-existent GPIOs > > > > are used (instead of failing only at run-time) > > > > > > > > however, the problem is meson8b_cbus_banks (annotated here to make it > > > > easier to read): > > > > BANK("X", /* name */ > > > > GPIOX_0, GPIOX_21, /* first and last GPIO */ > > > > 97, 118, /* first and last IRQ */ > > > > 4, 0, /* pull-up/down enable register and bit offset */ > > > > 4, 0, /* pull-up/down configuration register and bit offset */ > > > > 0, 0, /* input/output direction register and bit offset */ > > > > 1, 0, /* output value register and bit offset */ > > > > 2, 0), /* input value register and bit offset */ > > > > > > > > let's say we want to configure GPIOX_0 as an input: > > > > - we need to find the "offset" of this pad within the bank -> 0 > > > > - find the input/output direction register -> 0 > > > > - find the bit in that register: bit 0 + pad offset 0 = 0 > > > > - we clear bit 0 in register 0 > > > > > > > > with the current code configuring GPIOX_16 as input works like this: > > > > - we need to find the "offset" of this pad within the bank -> 16 > > > > - find the input/output direction register -> 0 > > > > - find the bit in that register: bit 0 + pad offset 16 = 16 > > > > - we clear bit 16 in register 0 > > > > > > If offset calculation is really the problem, nothing is stopping you from the > > > splitting BANK in two. With your example of GPIOX_12...15 missing: > > > > > > BANK("X1", /* name */ > > > GPIOX_0, GPIOX_11, /* first and last GPIO */ > > > 97, 108, /* first and last IRQ */ > > > 4, 0, /* pull-up/down enable register and bit offset */ > > > 4, 0, /* pull-up/down configuration register and bit offset */ > > > 0, 0, /* input/output direction register and bit offset */ > > > 1, 0, /* output value register and bit offset */ > > > 2, 0), /* input value register and bit offset */ > > > > > > BANK("X2", /* name */ > > > GPIOX_16, GPIOX_21, /* first and last GPIO */ > > > 113, 118, /* first and last IRQ */ > > > X, Y, /* pull-up/down enable register and bit offset */ > > > X, Y, /* pull-up/down configuration register and bit offset */ > > > Z, XX, /* input/output direction register and bit offset */ > > > FOO, 0, /* output value register and bit offset */ > > > BAR, 0), /* input value register and bit offset */ > > > > nice thinking :) > > the only solution I could come up with was to change the MESON_PIN > > definition to something like: > > #define MESON_PIN(pad, pullen_bit, pull_bit, direction_bit, > > output_bit, input_bit, irq_num) > > > > that however means that we would have to change a lot of code in all > > pinctrl drivers > > > > > > > > > > let's assume that we removed GPIOX_12...15 from Meson8b > > > > GPIOX_16 could now have the same value that was assigned to GPIOX_12 > > > > before (= 12) > > > > but how do we now know the offset of GPIOX_16? > > > > if we don't consider it in the calculation above we would end up > > > > writing to bit 12 (instead of bit 16). > > > > > > > > we could still cleanup the header file but leave the IDs of the > > > > existing GPIOs untouched (so GPIOX_16 would still be 16). > > > > > > I don't really get the point of doing so, especially is this patch is still > > > needed in the end. > > > > > > > however, that means that this patch is still needed because our last > > > > ID would be DIF_4_N (= value 129) again (so with holes our total count > > > > is still 130). > > > > with that we wouldn't even break the DT ABI > > > > > > While I prefer changing the bindings over the 'over_ngpio' tweak, it is not my > > > favorite solution. > > > > > > > > > > > > * If meson8b is indeed a cost reduction, it is likely to be same the approach as > > > > > s905x vs s905d: The D version is the same SoC with less ball pad but, as far as > > > > > we know, the logic is still there on the silicium (same die), it is just not > > > > > routed to the outside world. Both version share the same pinctrl driver (gxl). > > > > > The pad not routed to the outside world can just be considered "internal pads" > > > > > > > > there are a few differences between Meson8 and Meson8b, see also [0]: > > > > - Meson8 (and Meson8m2) CPU cores: 4x Cortex-A9 > > > > - Meson8b CPU cores: 4x Cortex-A5 > > > > - Meson8 (and Meson8m2) GPU: Mali-450MP8 > > > > - Meson8b GPU: Mali-450MP4 > > > > - Meson8 (and Meson8m2) package size: 19mm x 19 mm, LFBGA > > > > - Meson8b package size: 12mm x 12 mm, LFBGA > > > > > > > > the pinctrl register layout seems to be *mostly* compatible between > > > > Meson8 and Meson8b. > > > > unfortunately it's not entirely compatible, here is one example: > > > > - Meson8: GROUP(uart_tx_b1, 6, 23), > > > > - Meson8b: GROUP(uart_tx_b1, 6, 19), > > > > - Meson8: GROUP(eth_mdc, 6, 5), > > > > - Meson8b: GROUP(eth_mdc, 6, 9), > > > > > > > > > Long story short, if meson8 and meson8b share the same bindings, maybe they > > > > > should also share the driver. If they can't share the driver because they are > > > > > definitely incompatible, maybe they should not share the same bindings > > > > > > > > based on what I have seen so far I believe that they should not share > > > > the same bindings > > > > > > It looks to me to be same gpio/pinconf IP with a few pads not exposed to the > > > outside world, so I think they could share the bindings, and apparently fair > > > part of the driver data. > > > > > > Apparently amlogic tweaked a bit the pinmux. With the current architecture of > > > our pinctrl driver, it should be fairly easy to register 2 drivers sharing > > > everything but the GROUP table (and maybe the FUNCS if necessary) > > > > > > If possible, I would be more in favor of this last solution, as we could kill > > > one the pinctrl-meson8.c file, which are mostly a copy/paste of one another. > > > While fixing you issue, we would end up with less code to maintain and save a > > > bit of memory. > > > > > > Do you see any other big issue with this approach ? > > > > I would have to go through the whole list of pads to see what the > > actual differences are > > I tried to automate this as far as possible with the attached script > you can use it like this: > - first get the gpio.c files from the Amlogic 3.10 kernel for Meson8 > and Meson8b: [0] and [1] > - ./list-meson8-pinmux-reg-bits.sh path/to/meson8/gpio.c | sort -u > meson8.txt > - ./list-meson8-pinmux-reg-bits.sh path/to/meson8b/gpio.c | sort -u > > meson8b.txt > - diff -Naur meson8.txt meson8b.txt > diff.patch > - (the resulting diff is attached) > - compare the Meson8 documentation (found in a .csv file, see [2]) > with the public Meson8b datasheet (see [3]) > > the major differences are: > - DIF_TTL bank only exists on Meson8b > - GPIOZ bank only exists on Meson8 > - HDMI_TTL bank only exists on Meson8b (not supported by our mainline driver) > - some of the Ethernet pins are using the GPIOH bank on Meson8b while > Meson8 has these in the GPIOZ bank > - Meson8b has various SPI pins in the GPIOH bank while Meson8 has them > in the GPIOZ bank > - Meson8 only has pins for one TSin interface, Meson8b seems to > support two TSin interfaces > > smaller differences are: > - the LCD_* pins (maybe even all of the LCD support) in GPIODV only > exists on Meson8 > - Meson8b has the I2C_A pins on GPIODV_24 and GPIODV_25 (Meson8 has > multiple pins for this in the GPIOZ bank) > - Meson8b has the I2C_B pins on GPIODV_26 and GPIODV_27 (Meson8 has > these on GPIOZ_2 and GPIOZ_3) > - Meson8b has the I2C_C pins on GPIODV_28 and GPIODV_29 (Meson8 two > chices: either GPIOY_0 and GPIOY_1 or GPIOZ_4 and GPIOZ_5) > - the XTAL_24M_OUT signal uses GPIODV_29 on Meson8b but GPIOX_11 on Meson8 > - Meson8b can output UART_CTS_B on GPIOX_10 - Meson8 supports GPIODV_26 instead > - Meson8b can output UART_RTS_B on GPIOX_20 - Meson8 supports GPIODV_27 instead > - Meson8b can output SPI_SS0 on GPIOX_20 - Meson8 supports GPIOZ_9 instead > > not checked yet: > - Meson8b seems to have a few more bits in BOOT_8 BOOT_10 and BOOT_18 > - Meson8b seems to have a few more bits in GPIOAO_2, GPIOAO_3, > GPIOAO_6, GPIOAO_7, GPIOAO_9, GPIOAO_10 and GPIOAO_13 > - GPIOY_0, GPIOX_4, GPIOX_5, GPIOX_6, GPIOX_7, GPIOX_8, GPIOX_9 > - the changes (there are quite a few) in the GPIOY bank > > notes for other pins I have randomly checked: > - Meson8b's gpio.c has a function for GPIOX_0 at reg9[24] - this seems > undocumented > - Meson8b's gpio.c has a function for GPIOX_1 at reg9[23] - this seems > undocumented > - Meson8b's gpio.c has a function for GPIOX_2 at reg9[22] - this seems > undocumented > - Meson8b's gpio.c has a function for GPIOX_3 at reg9[21] - this seems > undocumented > > I'm not sure if we should try to create a unified pinctrl driver for > Meson8 and Meson8b Thanks the detailed summary Martin ! I agree with you. meson8b obviously derives from meson8 but there is to many subtle differences to create unified driver ... w/o adding spaghetti code around Is the "2 banks" solution enough to solve the problem cleanly ? If not and it still leaves problems to be solved in the future, maybe we would be better off with different bindings for meson8 and meson8b ? and be done with it ... > so far we have: > - meson_pmx_group are roughly 67% identical (meson_pmx_func depend on > meson_pmx_group) > - different pinctrl_pin_desc (due to different GPIO banks) > - different IRQ numbering > > so I wonder how much we would actually save by creating a unified > Meson8/Meson8b pinctrl driver. > > as a side-note: I found out that Meson8m2 has 10 pins which each add > one function compared to Meson8, see: [4] > I would integrate these into the Meson8 driver (if we keep separate > drivers for Meson8 and Meson8b) when needed, because these definitely > don't justify adding a new pinctrl driver > > > Regards > Martin > > > [0] https://github.com/endlessm/linux-meson/blob/5cb4882cdda584878a29132aeb9a90497a121df9/arch/arm/mach-meson8/gpio.c > [1] https://github.com/endlessm/linux-meson/blob/5cb4882cdda584878a29132aeb9a90497a121df9/arch/arm/mach-meson8b/gpio.c > [2] https://github.com/endlessm/linux-meson/blob/5cb4882cdda584878a29132aeb9a90497a121df9/arch/arm/mach-meson8/tool/m8_gpio.csv > [3] https://dn.odroid.com/S805/Datasheet/S805_Datasheet%20V0.8%2020150126.pdf > [4] https://github.com/endlessm/linux-meson/blob/5cb4882cdda584878a29132aeb9a90497a121df9/arch/arm/mach-meson8/gpio.c#L242 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jerome, On Mon, Feb 19, 2018 at 9:07 AM, Jerome Brunet <jbrunet@baylibre.com> wrote: > On Sat, 2018-02-17 at 20:44 +0100, Martin Blumenstingl wrote: >> Hi Jerome, >> >> On Wed, Jan 24, 2018 at 6:54 PM, Martin Blumenstingl >> <martin.blumenstingl@googlemail.com> wrote: >> > Hi Jerome, >> > >> > On Wed, Jan 24, 2018 at 3:52 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: >> > > On Wed, 2018-01-24 at 11:10 +0100, Martin Blumenstingl wrote: >> > > > Hi Jerome, >> > > > >> > > > On Wed, Jan 24, 2018 at 9:28 AM, Jerome Brunet <jbrunet@baylibre.com> wrote: >> > > > > On Wed, 2018-01-24 at 01:27 +0100, Martin Blumenstingl wrote: >> > > > > > Meson8b is a cost reduced variant of the Meson8 SoC. It's package size >> > > > > > is smaller than Meson8. >> > > > > > Unfortunately there are a few key differences which cannot be seen >> > > > > > without close inspection of the code and the public S805 data-sheet: >> > > > > > - the GPIOX bank is missing the GPIOX_12, GPIOX_13, GPIOX_14 and >> > > > > > GPIOX_15 GPIOs >> > > > > > - the GPIOY bank is missing the GPIOY_2, GPIOY_4, GPIOY_5, GPIOY_15 and >> > > > > > GPIOY_16 GPIOs >> > > > > > - the GPIODV bank is missing all GPIOs except GPIODV_9, GPIODV_24, >> > > > > > GPIODV_25, GPIODV_26, GPIODV_27, GPIODV_28 and GPIODV_29 >> > > > > > - the GPIOZ bank is missing completely >> > > > > > - there is a new GPIO bank called "DIF" >> > > > > > - (all other pads exist on both, Meson8 and Meson8b) >> > > > > > >> > > > > > The pinctrl-meson driver internally uses struct meson_bank, which >> > > > > > assumes that the GPIOs are continuous, without any holes in between. >> > > > > > This also matches with how the registers work: >> > > > > > - GPIOX_0 for example uses bit 0 for switching this pad between >> > > > > > input/output, configuring pull-up/down, etc. >> > > > > > - GPIOX_16 uses bit 16 for switching this pad between input/output, >> > > > > > configuring pull-up/down/, etc >> > > > > > GPIOX_16 uses bit 16 even though GPIOX12..15 don't exist. (This is >> > > > > > probably the reason why Meson8b inherits the dt-bindings - with all GPIO >> > > > > > IDs - from Meson8) >> > > > > > This means that Meson8b only has 83 actual GPIO lines. Without any holes >> > > > > > there would be 130 GPIO lines in total (120 are inherited from Meson8 >> > > > > > plus 10 new from the DIF bank). >> > > > > > >> > > > > > The pinctrl framework handles holes in the pin list fine, which can be >> > > > > > seen in debugfs: >> > > > > > $ cat /sys/kernel/debug/pinctrl/c1109880.pinctrl/pins >> > > > > > pin 9 (GPIOX_9) c1109880.pinctrl >> > > > > > pin 10 (GPIOX_10) c1109880.pinctrl >> > > > > > pin 11 (GPIOX_11) c1109880.pinctrl >> > > > > > pin 16 (GPIOX_16) c1109880.pinctrl >> > > > > > pin 17 (GPIOX_17) c1109880.pinctrl >> > > > > >> > > > > Hi Martin, >> > > > > >> > > > > While working on this controller a few weeks ago, I have seen that our current >> > > > > design does not tolerate having holes in the declared PINS. This is something >> > > > > that predate the changes mentioned here. >> > > > > >> > > > > I'm not a big fan of the this overrides_ngpio here, I'm sure it fixes your >> > > > > problem but it seems a bit hacky, don't you think ? >> > > > >> > > > this is the reason why I sent it as RFC - I'm open to better solutions! >> > > > >> > > > > I would prefer either of these 2 approach: >> > > > > >> > > > > * Update the binding and kill the necessary pins. We are still figuring out >> > > > > those chips, which is why the bindings are marked as un-stable. If sharing the >> > > > > binding with meson8 was a mistake, now is the time to fix it. >> > > > >> > > > let's say we produce a header file which does not contain GPIOX_12: >> > > > this would be nice since it gives compile errors if non-existent GPIOs >> > > > are used (instead of failing only at run-time) >> > > > >> > > > however, the problem is meson8b_cbus_banks (annotated here to make it >> > > > easier to read): >> > > > BANK("X", /* name */ >> > > > GPIOX_0, GPIOX_21, /* first and last GPIO */ >> > > > 97, 118, /* first and last IRQ */ >> > > > 4, 0, /* pull-up/down enable register and bit offset */ >> > > > 4, 0, /* pull-up/down configuration register and bit offset */ >> > > > 0, 0, /* input/output direction register and bit offset */ >> > > > 1, 0, /* output value register and bit offset */ >> > > > 2, 0), /* input value register and bit offset */ >> > > > >> > > > let's say we want to configure GPIOX_0 as an input: >> > > > - we need to find the "offset" of this pad within the bank -> 0 >> > > > - find the input/output direction register -> 0 >> > > > - find the bit in that register: bit 0 + pad offset 0 = 0 >> > > > - we clear bit 0 in register 0 >> > > > >> > > > with the current code configuring GPIOX_16 as input works like this: >> > > > - we need to find the "offset" of this pad within the bank -> 16 >> > > > - find the input/output direction register -> 0 >> > > > - find the bit in that register: bit 0 + pad offset 16 = 16 >> > > > - we clear bit 16 in register 0 >> > > >> > > If offset calculation is really the problem, nothing is stopping you from the >> > > splitting BANK in two. With your example of GPIOX_12...15 missing: >> > > >> > > BANK("X1", /* name */ >> > > GPIOX_0, GPIOX_11, /* first and last GPIO */ >> > > 97, 108, /* first and last IRQ */ >> > > 4, 0, /* pull-up/down enable register and bit offset */ >> > > 4, 0, /* pull-up/down configuration register and bit offset */ >> > > 0, 0, /* input/output direction register and bit offset */ >> > > 1, 0, /* output value register and bit offset */ >> > > 2, 0), /* input value register and bit offset */ >> > > >> > > BANK("X2", /* name */ >> > > GPIOX_16, GPIOX_21, /* first and last GPIO */ >> > > 113, 118, /* first and last IRQ */ >> > > X, Y, /* pull-up/down enable register and bit offset */ >> > > X, Y, /* pull-up/down configuration register and bit offset */ >> > > Z, XX, /* input/output direction register and bit offset */ >> > > FOO, 0, /* output value register and bit offset */ >> > > BAR, 0), /* input value register and bit offset */ >> > >> > nice thinking :) >> > the only solution I could come up with was to change the MESON_PIN >> > definition to something like: >> > #define MESON_PIN(pad, pullen_bit, pull_bit, direction_bit, >> > output_bit, input_bit, irq_num) >> > >> > that however means that we would have to change a lot of code in all >> > pinctrl drivers >> > >> > > > >> > > > let's assume that we removed GPIOX_12...15 from Meson8b >> > > > GPIOX_16 could now have the same value that was assigned to GPIOX_12 >> > > > before (= 12) >> > > > but how do we now know the offset of GPIOX_16? >> > > > if we don't consider it in the calculation above we would end up >> > > > writing to bit 12 (instead of bit 16). >> > > > >> > > > we could still cleanup the header file but leave the IDs of the >> > > > existing GPIOs untouched (so GPIOX_16 would still be 16). >> > > >> > > I don't really get the point of doing so, especially is this patch is still >> > > needed in the end. >> > > >> > > > however, that means that this patch is still needed because our last >> > > > ID would be DIF_4_N (= value 129) again (so with holes our total count >> > > > is still 130). >> > > > with that we wouldn't even break the DT ABI >> > > >> > > While I prefer changing the bindings over the 'over_ngpio' tweak, it is not my >> > > favorite solution. >> > > >> > > > >> > > > > * If meson8b is indeed a cost reduction, it is likely to be same the approach as >> > > > > s905x vs s905d: The D version is the same SoC with less ball pad but, as far as >> > > > > we know, the logic is still there on the silicium (same die), it is just not >> > > > > routed to the outside world. Both version share the same pinctrl driver (gxl). >> > > > > The pad not routed to the outside world can just be considered "internal pads" >> > > > >> > > > there are a few differences between Meson8 and Meson8b, see also [0]: >> > > > - Meson8 (and Meson8m2) CPU cores: 4x Cortex-A9 >> > > > - Meson8b CPU cores: 4x Cortex-A5 >> > > > - Meson8 (and Meson8m2) GPU: Mali-450MP8 >> > > > - Meson8b GPU: Mali-450MP4 >> > > > - Meson8 (and Meson8m2) package size: 19mm x 19 mm, LFBGA >> > > > - Meson8b package size: 12mm x 12 mm, LFBGA >> > > > >> > > > the pinctrl register layout seems to be *mostly* compatible between >> > > > Meson8 and Meson8b. >> > > > unfortunately it's not entirely compatible, here is one example: >> > > > - Meson8: GROUP(uart_tx_b1, 6, 23), >> > > > - Meson8b: GROUP(uart_tx_b1, 6, 19), >> > > > - Meson8: GROUP(eth_mdc, 6, 5), >> > > > - Meson8b: GROUP(eth_mdc, 6, 9), >> > > > >> > > > > Long story short, if meson8 and meson8b share the same bindings, maybe they >> > > > > should also share the driver. If they can't share the driver because they are >> > > > > definitely incompatible, maybe they should not share the same bindings >> > > > >> > > > based on what I have seen so far I believe that they should not share >> > > > the same bindings >> > > >> > > It looks to me to be same gpio/pinconf IP with a few pads not exposed to the >> > > outside world, so I think they could share the bindings, and apparently fair >> > > part of the driver data. >> > > >> > > Apparently amlogic tweaked a bit the pinmux. With the current architecture of >> > > our pinctrl driver, it should be fairly easy to register 2 drivers sharing >> > > everything but the GROUP table (and maybe the FUNCS if necessary) >> > > >> > > If possible, I would be more in favor of this last solution, as we could kill >> > > one the pinctrl-meson8.c file, which are mostly a copy/paste of one another. >> > > While fixing you issue, we would end up with less code to maintain and save a >> > > bit of memory. >> > > >> > > Do you see any other big issue with this approach ? >> > >> > I would have to go through the whole list of pads to see what the >> > actual differences are >> >> I tried to automate this as far as possible with the attached script >> you can use it like this: >> - first get the gpio.c files from the Amlogic 3.10 kernel for Meson8 >> and Meson8b: [0] and [1] >> - ./list-meson8-pinmux-reg-bits.sh path/to/meson8/gpio.c | sort -u > meson8.txt >> - ./list-meson8-pinmux-reg-bits.sh path/to/meson8b/gpio.c | sort -u > >> meson8b.txt >> - diff -Naur meson8.txt meson8b.txt > diff.patch >> - (the resulting diff is attached) >> - compare the Meson8 documentation (found in a .csv file, see [2]) >> with the public Meson8b datasheet (see [3]) >> >> the major differences are: >> - DIF_TTL bank only exists on Meson8b >> - GPIOZ bank only exists on Meson8 >> - HDMI_TTL bank only exists on Meson8b (not supported by our mainline driver) >> - some of the Ethernet pins are using the GPIOH bank on Meson8b while >> Meson8 has these in the GPIOZ bank >> - Meson8b has various SPI pins in the GPIOH bank while Meson8 has them >> in the GPIOZ bank >> - Meson8 only has pins for one TSin interface, Meson8b seems to >> support two TSin interfaces >> >> smaller differences are: >> - the LCD_* pins (maybe even all of the LCD support) in GPIODV only >> exists on Meson8 >> - Meson8b has the I2C_A pins on GPIODV_24 and GPIODV_25 (Meson8 has >> multiple pins for this in the GPIOZ bank) >> - Meson8b has the I2C_B pins on GPIODV_26 and GPIODV_27 (Meson8 has >> these on GPIOZ_2 and GPIOZ_3) >> - Meson8b has the I2C_C pins on GPIODV_28 and GPIODV_29 (Meson8 two >> chices: either GPIOY_0 and GPIOY_1 or GPIOZ_4 and GPIOZ_5) >> - the XTAL_24M_OUT signal uses GPIODV_29 on Meson8b but GPIOX_11 on Meson8 >> - Meson8b can output UART_CTS_B on GPIOX_10 - Meson8 supports GPIODV_26 instead >> - Meson8b can output UART_RTS_B on GPIOX_20 - Meson8 supports GPIODV_27 instead >> - Meson8b can output SPI_SS0 on GPIOX_20 - Meson8 supports GPIOZ_9 instead >> >> not checked yet: >> - Meson8b seems to have a few more bits in BOOT_8 BOOT_10 and BOOT_18 >> - Meson8b seems to have a few more bits in GPIOAO_2, GPIOAO_3, >> GPIOAO_6, GPIOAO_7, GPIOAO_9, GPIOAO_10 and GPIOAO_13 >> - GPIOY_0, GPIOX_4, GPIOX_5, GPIOX_6, GPIOX_7, GPIOX_8, GPIOX_9 >> - the changes (there are quite a few) in the GPIOY bank >> >> notes for other pins I have randomly checked: >> - Meson8b's gpio.c has a function for GPIOX_0 at reg9[24] - this seems >> undocumented >> - Meson8b's gpio.c has a function for GPIOX_1 at reg9[23] - this seems >> undocumented >> - Meson8b's gpio.c has a function for GPIOX_2 at reg9[22] - this seems >> undocumented >> - Meson8b's gpio.c has a function for GPIOX_3 at reg9[21] - this seems >> undocumented >> >> I'm not sure if we should try to create a unified pinctrl driver for >> Meson8 and Meson8b > > Thanks the detailed summary Martin ! I agree with you. meson8b obviously derives > from meson8 but there is to many subtle differences to create unified driver ... > w/o adding spaghetti code around yes, I think in this case some code-duplication is better than making the code harder to read by introducing quite a few "use this register bit or pad on Meson8, but another on Meson8b" > Is the "2 banks" solution enough to solve the problem cleanly ? I haven't tested it yet - I'll report back once I did but based on your previous suggestion it should work > If not and it still leaves problems to be solved in the future, maybe we would > be better off with different bindings for meson8 and meson8b ? and be done with > it ... I'll test your split bank solution first, then we can discuss the next steps > >> so far we have: >> - meson_pmx_group are roughly 67% identical (meson_pmx_func depend on >> meson_pmx_group) >> - different pinctrl_pin_desc (due to different GPIO banks) >> - different IRQ numbering >> >> so I wonder how much we would actually save by creating a unified >> Meson8/Meson8b pinctrl driver. >> >> as a side-note: I found out that Meson8m2 has 10 pins which each add >> one function compared to Meson8, see: [4] >> I would integrate these into the Meson8 driver (if we keep separate >> drivers for Meson8 and Meson8b) when needed, because these definitely >> don't justify adding a new pinctrl driver >> >> >> Regards >> Martin >> >> >> [0] https://github.com/endlessm/linux-meson/blob/5cb4882cdda584878a29132aeb9a90497a121df9/arch/arm/mach-meson8/gpio.c >> [1] https://github.com/endlessm/linux-meson/blob/5cb4882cdda584878a29132aeb9a90497a121df9/arch/arm/mach-meson8b/gpio.c >> [2] https://github.com/endlessm/linux-meson/blob/5cb4882cdda584878a29132aeb9a90497a121df9/arch/arm/mach-meson8/tool/m8_gpio.csv >> [3] https://dn.odroid.com/S805/Datasheet/S805_Datasheet%20V0.8%2020150126.pdf >> [4] https://github.com/endlessm/linux-meson/blob/5cb4882cdda584878a29132aeb9a90497a121df9/arch/arm/mach-meson8/gpio.c#L242 > Regards Martin -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c index 29a458da78db..1befe67fd187 100644 --- a/drivers/pinctrl/meson/pinctrl-meson.c +++ b/drivers/pinctrl/meson/pinctrl-meson.c @@ -413,7 +413,12 @@ static int meson_gpiolib_register(struct meson_pinctrl *pc) pc->chip.get = meson_gpio_get; pc->chip.set = meson_gpio_set; pc->chip.base = -1; - pc->chip.ngpio = pc->data->num_pins; + + if (pc->data->override_ngpio > 0) + pc->chip.ngpio = pc->data->override_ngpio; + else + pc->chip.ngpio = pc->data->num_pins; + pc->chip.can_sleep = false; pc->chip.of_node = pc->of_node; pc->chip.of_gpio_n_cells = 2; diff --git a/drivers/pinctrl/meson/pinctrl-meson.h b/drivers/pinctrl/meson/pinctrl-meson.h index 183b6e471635..afd7efaad9bb 100644 --- a/drivers/pinctrl/meson/pinctrl-meson.h +++ b/drivers/pinctrl/meson/pinctrl-meson.h @@ -108,6 +108,7 @@ struct meson_pinctrl_data { struct meson_bank *banks; unsigned int num_banks; const struct pinmux_ops *pmx_ops; + unsigned int override_ngpio; }; struct meson_pinctrl { diff --git a/drivers/pinctrl/meson/pinctrl-meson8b.c b/drivers/pinctrl/meson/pinctrl-meson8b.c index 5bd808dc81e1..05bb4c3143d9 100644 --- a/drivers/pinctrl/meson/pinctrl-meson8b.c +++ b/drivers/pinctrl/meson/pinctrl-meson8b.c @@ -916,6 +916,20 @@ static struct meson_pinctrl_data meson8b_cbus_pinctrl_data = { .num_funcs = ARRAY_SIZE(meson8b_cbus_functions), .num_banks = ARRAY_SIZE(meson8b_cbus_banks), .pmx_ops = &meson8_pmx_ops, + /* + * there are holes in the meson8b_cbus_pins mapping because we are + * mapping the GPIO identifiers to register bits. on Meson8b some bits + * (and thus the corresponding pads) are not used on the SoC + * (presumably because they were skipped because Meson8b is a cost + * reduced variant of Meson8 and it didn't make sense to create a whole + * new register layout for this variant). + * this informs the GPIO framework know about the number of GPIOs we + * have, including the pins which are not available on Meson8b (= the + * holes). The pinctrl/GPIO framework knows the holes because it checks + * all pinctrl_pin_desc in meson8b_cbus_pins and reports an error as + * soon as a GPIO is requested which is not part of meson8b_cbus_pins. + */ + .override_ngpio = 130, }; static struct meson_pinctrl_data meson8b_aobus_pinctrl_data = {
Meson8b is a cost reduced variant of the Meson8 SoC. It's package size is smaller than Meson8. Unfortunately there are a few key differences which cannot be seen without close inspection of the code and the public S805 data-sheet: - the GPIOX bank is missing the GPIOX_12, GPIOX_13, GPIOX_14 and GPIOX_15 GPIOs - the GPIOY bank is missing the GPIOY_2, GPIOY_4, GPIOY_5, GPIOY_15 and GPIOY_16 GPIOs - the GPIODV bank is missing all GPIOs except GPIODV_9, GPIODV_24, GPIODV_25, GPIODV_26, GPIODV_27, GPIODV_28 and GPIODV_29 - the GPIOZ bank is missing completely - there is a new GPIO bank called "DIF" - (all other pads exist on both, Meson8 and Meson8b) The pinctrl-meson driver internally uses struct meson_bank, which assumes that the GPIOs are continuous, without any holes in between. This also matches with how the registers work: - GPIOX_0 for example uses bit 0 for switching this pad between input/output, configuring pull-up/down, etc. - GPIOX_16 uses bit 16 for switching this pad between input/output, configuring pull-up/down/, etc GPIOX_16 uses bit 16 even though GPIOX12..15 don't exist. (This is probably the reason why Meson8b inherits the dt-bindings - with all GPIO IDs - from Meson8) This means that Meson8b only has 83 actual GPIO lines. Without any holes there would be 130 GPIO lines in total (120 are inherited from Meson8 plus 10 new from the DIF bank). The pinctrl framework handles holes in the pin list fine, which can be seen in debugfs: $ cat /sys/kernel/debug/pinctrl/c1109880.pinctrl/pins pin 9 (GPIOX_9) c1109880.pinctrl pin 10 (GPIOX_10) c1109880.pinctrl pin 11 (GPIOX_11) c1109880.pinctrl pin 16 (GPIOX_16) c1109880.pinctrl pin 17 (GPIOX_17) c1109880.pinctrl GPIOs which don't exist cannot be requested either ("base" of the GPIO controller is 382): $ echo 394 > /sys/class/gpio/export meson8b-pinctrl c1109880.pinctrl: pin 12 is not registered so it cannot be requested meson8b-pinctrl c1109880.pinctrl: pin-12 (cbus-banks:394) status -22 Unfortunately GPIOs greater GPIOZ_3 (whose ID is 83 - as a reminder: this is exactly the number of actual GPIO lines on Meson8b) cannot be requested. Using CARD_6 (ID 100, "base of the GPIO controller is 382) as an example: $ echo 482 > /sys/class/gpio/export export_store: invalid GPIO 482 The problem here is that struct gpio_chip's "ngpio" is set to 83. Thus requesting a GPIO higher than 83 fails. Commit 984cffdeaeb7ea ("pinctrl: Fix gpio/pin mapping for Meson8b") fixed this problem a while ago, by setting "ngpio" to 130. Unfortunately this broke again while cleaning up the pinctrl-meson driver in commit db80f0e158e621 ("pinctrl: meson: get rid of unneeded domain structures"), which started using ARRAY_SIZE(meson8b_cbus_pins) - which evaluates to 83 - as "ngpio". This is now fixed by introducing a new "override_ngpio" field in struct meson_pinctrl_data. This value is passed to struct gpio_chip's "ngpio" when set - if override_ngpio is <= 0 then num_pins is used (which works for all other Meson SoCs, except Meson8b). The definitions in "include/dt-bindings/gpio/meson8b-gpio.h" were not changed. Cleaning up these definitions would allow us to get rid of the GPIOZ definitions (which are inherited from Meson8). However, we still need to handle GPIO banks with holes (such as GPIOX), so this include is not touched for now (touching it could also break the device-tree ABI). Meson8b's AO GPIO controller is not affected by this issue since it does not have any holes in it - only the CBUS GPIO controller is affected. This was initially seen by Linus Lüssing who was preparing SD card support on Odroid-C1 which uses CARD_6 as "card detect" GPIO. Fixes: db80f0e158e621 ("pinctrl: meson: get rid of unneeded domain structures") Reported-by: Linus Lüssing <linus.luessing@c0d3.blue> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> --- drivers/pinctrl/meson/pinctrl-meson.c | 7 ++++++- drivers/pinctrl/meson/pinctrl-meson.h | 1 + drivers/pinctrl/meson/pinctrl-meson8b.c | 14 ++++++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-)