| Message ID | 1432565608-26036-3-git-send-email-tomeu.vizoso@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/25/2015 08:53 AM, Tomeu Vizoso wrote: > Specify how the GPIOs map to the pins in T124, so the dependency is > explicit. > > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> > --- > arch/arm/boot/dts/tegra124.dtsi | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi > index 13cc7ca..5d1d35f 100644 > --- a/arch/arm/boot/dts/tegra124.dtsi > +++ b/arch/arm/boot/dts/tegra124.dtsi > @@ -254,6 +254,7 @@ > gpio-controller; > #interrupt-cells = <2>; > interrupt-controller; > + gpio-ranges = <&pinmux 0 0 250>; We should be consistent between SoCs. Why not make the same change for all Tegra SoCs? I think this change will cause the GPIO subsystem to call into the pinctrl subsystem and create/add/register a new GPIO<->pinctrl range structure. The pinctrl driver already does this, so I think we'll end up with two duplicate entries in the pinctrl device's gpio_ranges list. This probably won't cause a problem, but I wanted to make sure you'd thought about it to make sure. Right now, I think we get lucky and pinctrl ends up probing first (or at least very early) anyway. Somewhat related to this series, I wonder if we shouldn't add pinctrl client properties to every node in the Tegra DT that describes a controller that makes use of external pins that are affected by the pinmux. Such a change would guarantee this desired probing order. In order to preserve the "program the entire pinmux at once" semantics, these new pinctrl client properties would all need to reference empty states, yet would still need to exist to represent the dependency.
On 26 May 2015 at 21:41, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 05/25/2015 08:53 AM, Tomeu Vizoso wrote: >> >> Specify how the GPIOs map to the pins in T124, so the dependency is >> explicit. >> >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> >> --- >> arch/arm/boot/dts/tegra124.dtsi | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/arch/arm/boot/dts/tegra124.dtsi >> b/arch/arm/boot/dts/tegra124.dtsi >> index 13cc7ca..5d1d35f 100644 >> --- a/arch/arm/boot/dts/tegra124.dtsi >> +++ b/arch/arm/boot/dts/tegra124.dtsi >> @@ -254,6 +254,7 @@ >> gpio-controller; >> #interrupt-cells = <2>; >> interrupt-controller; >> + gpio-ranges = <&pinmux 0 0 250>; > > > We should be consistent between SoCs. Why not make the same change for all > Tegra SoCs? > > I think this change will cause the GPIO subsystem to call into the pinctrl > subsystem and create/add/register a new GPIO<->pinctrl range structure. The > pinctrl driver already does this, so I think we'll end up with two duplicate > entries in the pinctrl device's gpio_ranges list. This probably won't cause > a problem, but I wanted to make sure you'd thought about it to make sure. Actually, I have checked and see that gpio-tegra.c registers 256 gpios, but pinctrl-tegra124.c adds a range of only 251. I don't really remember where I got the 250 value from, sorry :( I don't see how that would cause any concrete problems, but maybe we should have a single authoritative source (not sure we can do so without breaking DT ABI though). > Right now, I think we get lucky and pinctrl ends up probing first (or at > least very early) anyway. Somewhat related to this series, I wonder if we > shouldn't add pinctrl client properties to every node in the Tegra DT that > describes a controller that makes use of external pins that are affected by > the pinmux. Such a change would guarantee this desired probing order. In > order to preserve the "program the entire pinmux at once" semantics, these > new pinctrl client properties would all need to reference empty states, yet > would still need to exist to represent the dependency. I think so, but aren't almost all those pins used as gpios? If so, then such a controller's driver will request the gpio it wants which will cause the gpio driver to be registered (and hopefully probed) if needed, which in turn will check that the corresponding pinctrl device has been registered. Or am I missing something? Regards, Tomeu > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
On 05/27/2015 08:18 AM, Tomeu Vizoso wrote: > On 26 May 2015 at 21:41, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 05/25/2015 08:53 AM, Tomeu Vizoso wrote: >>> >>> Specify how the GPIOs map to the pins in T124, so the dependency is >>> explicit. >>> >>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> >>> --- >>> arch/arm/boot/dts/tegra124.dtsi | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/arch/arm/boot/dts/tegra124.dtsi >>> b/arch/arm/boot/dts/tegra124.dtsi >>> index 13cc7ca..5d1d35f 100644 >>> --- a/arch/arm/boot/dts/tegra124.dtsi >>> +++ b/arch/arm/boot/dts/tegra124.dtsi >>> @@ -254,6 +254,7 @@ >>> gpio-controller; >>> #interrupt-cells = <2>; >>> interrupt-controller; >>> + gpio-ranges = <&pinmux 0 0 250>; >> >> >> We should be consistent between SoCs. Why not make the same change for all >> Tegra SoCs? >> >> I think this change will cause the GPIO subsystem to call into the pinctrl >> subsystem and create/add/register a new GPIO<->pinctrl range structure. The >> pinctrl driver already does this, so I think we'll end up with two duplicate >> entries in the pinctrl device's gpio_ranges list. This probably won't cause >> a problem, but I wanted to make sure you'd thought about it to make sure. > > Actually, I have checked and see that gpio-tegra.c registers 256 > gpios, but pinctrl-tegra124.c adds a range of only 251. I don't really > remember where I got the 250 value from, sorry :( > > I don't see how that would cause any concrete problems, but maybe we > should have a single authoritative source (not sure we can do so > without breaking DT ABI though). > >> Right now, I think we get lucky and pinctrl ends up probing first (or at >> least very early) anyway. Somewhat related to this series, I wonder if we >> shouldn't add pinctrl client properties to every node in the Tegra DT that >> describes a controller that makes use of external pins that are affected by >> the pinmux. Such a change would guarantee this desired probing order. In >> order to preserve the "program the entire pinmux at once" semantics, these >> new pinctrl client properties would all need to reference empty states, yet >> would still need to exist to represent the dependency. > > I think so, but aren't almost all those pins used as gpios? If so, > then such a controller's driver will request the gpio it wants which > will cause the gpio driver to be registered (and hopefully probed) if > needed, which in turn will check that the corresponding pinctrl device > has been registered. Or am I missing something? As you say this probably works out fine for pins that are used as GPIOs. I was thinking more about SFIOs. Take an I2C controller, which doesn't use any GPIOs itself. The pinctrl device should be probed before the I2C device, so that the I2C driver can initiate transactions on the I2C bus during its probe if it wanted to (or at least, clients could initiate transactions at any completely arbitrary time as soon as probe was complete).
On 27 May 2015 at 16:49, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 05/27/2015 08:18 AM, Tomeu Vizoso wrote: >> >> On 26 May 2015 at 21:41, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> >>> On 05/25/2015 08:53 AM, Tomeu Vizoso wrote: >>>> >>>> >>>> Specify how the GPIOs map to the pins in T124, so the dependency is >>>> explicit. >>>> >>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> >>>> --- >>>> arch/arm/boot/dts/tegra124.dtsi | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/arch/arm/boot/dts/tegra124.dtsi >>>> b/arch/arm/boot/dts/tegra124.dtsi >>>> index 13cc7ca..5d1d35f 100644 >>>> --- a/arch/arm/boot/dts/tegra124.dtsi >>>> +++ b/arch/arm/boot/dts/tegra124.dtsi >>>> @@ -254,6 +254,7 @@ >>>> gpio-controller; >>>> #interrupt-cells = <2>; >>>> interrupt-controller; >>>> + gpio-ranges = <&pinmux 0 0 250>; >>> >>> >>> >>> We should be consistent between SoCs. Why not make the same change for >>> all >>> Tegra SoCs? >>> >>> I think this change will cause the GPIO subsystem to call into the >>> pinctrl >>> subsystem and create/add/register a new GPIO<->pinctrl range structure. >>> The >>> pinctrl driver already does this, so I think we'll end up with two >>> duplicate >>> entries in the pinctrl device's gpio_ranges list. This probably won't >>> cause >>> a problem, but I wanted to make sure you'd thought about it to make sure. >> >> >> Actually, I have checked and see that gpio-tegra.c registers 256 >> gpios, but pinctrl-tegra124.c adds a range of only 251. I don't really >> remember where I got the 250 value from, sorry :( >> >> I don't see how that would cause any concrete problems, but maybe we >> should have a single authoritative source (not sure we can do so >> without breaking DT ABI though). >> >>> Right now, I think we get lucky and pinctrl ends up probing first (or at >>> least very early) anyway. Somewhat related to this series, I wonder if we >>> shouldn't add pinctrl client properties to every node in the Tegra DT >>> that >>> describes a controller that makes use of external pins that are affected >>> by >>> the pinmux. Such a change would guarantee this desired probing order. In >>> order to preserve the "program the entire pinmux at once" semantics, >>> these >>> new pinctrl client properties would all need to reference empty states, >>> yet >>> would still need to exist to represent the dependency. >> >> >> I think so, but aren't almost all those pins used as gpios? If so, >> then such a controller's driver will request the gpio it wants which >> will cause the gpio driver to be registered (and hopefully probed) if >> needed, which in turn will check that the corresponding pinctrl device >> has been registered. Or am I missing something? > > > As you say this probably works out fine for pins that are used as GPIOs. I > was thinking more about SFIOs. Take an I2C controller, which doesn't use any > GPIOs itself. The pinctrl device should be probed before the I2C device, so > that the I2C driver can initiate transactions on the I2C bus during its > probe if it wanted to (or at least, clients could initiate transactions at > any completely arbitrary time as soon as probe was complete). What is using the SFIO in this case, the I2C master or the I2C client? In any case, is the problem you are referring to that ICs may rely on a specific pinmux configuration but that's not currently reflected on the DT because pinmux configuration happened so early that things just worked? Thanks, Tomeu
On 05/28/2015 02:26 AM, Tomeu Vizoso wrote: > On 27 May 2015 at 16:49, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 05/27/2015 08:18 AM, Tomeu Vizoso wrote: >>> >>> On 26 May 2015 at 21:41, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>> >>>> On 05/25/2015 08:53 AM, Tomeu Vizoso wrote: >>>>> >>>>> >>>>> Specify how the GPIOs map to the pins in T124, so the dependency is >>>>> explicit. >>>>> >>>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> >>>>> --- >>>>> arch/arm/boot/dts/tegra124.dtsi | 1 + >>>>> 1 file changed, 1 insertion(+) >>>>> >>>>> diff --git a/arch/arm/boot/dts/tegra124.dtsi >>>>> b/arch/arm/boot/dts/tegra124.dtsi >>>>> index 13cc7ca..5d1d35f 100644 >>>>> --- a/arch/arm/boot/dts/tegra124.dtsi >>>>> +++ b/arch/arm/boot/dts/tegra124.dtsi >>>>> @@ -254,6 +254,7 @@ >>>>> gpio-controller; >>>>> #interrupt-cells = <2>; >>>>> interrupt-controller; >>>>> + gpio-ranges = <&pinmux 0 0 250>; >>>> >>>> >>>> >>>> We should be consistent between SoCs. Why not make the same change for >>>> all >>>> Tegra SoCs? >>>> >>>> I think this change will cause the GPIO subsystem to call into the >>>> pinctrl >>>> subsystem and create/add/register a new GPIO<->pinctrl range structure. >>>> The >>>> pinctrl driver already does this, so I think we'll end up with two >>>> duplicate >>>> entries in the pinctrl device's gpio_ranges list. This probably won't >>>> cause >>>> a problem, but I wanted to make sure you'd thought about it to make sure. >>> >>> >>> Actually, I have checked and see that gpio-tegra.c registers 256 >>> gpios, but pinctrl-tegra124.c adds a range of only 251. I don't really >>> remember where I got the 250 value from, sorry :( >>> >>> I don't see how that would cause any concrete problems, but maybe we >>> should have a single authoritative source (not sure we can do so >>> without breaking DT ABI though). >>> >>>> Right now, I think we get lucky and pinctrl ends up probing first (or at >>>> least very early) anyway. Somewhat related to this series, I wonder if we >>>> shouldn't add pinctrl client properties to every node in the Tegra DT >>>> that >>>> describes a controller that makes use of external pins that are affected >>>> by >>>> the pinmux. Such a change would guarantee this desired probing order. In >>>> order to preserve the "program the entire pinmux at once" semantics, >>>> these >>>> new pinctrl client properties would all need to reference empty states, >>>> yet >>>> would still need to exist to represent the dependency. >>> >>> >>> I think so, but aren't almost all those pins used as gpios? If so, >>> then such a controller's driver will request the gpio it wants which >>> will cause the gpio driver to be registered (and hopefully probed) if >>> needed, which in turn will check that the corresponding pinctrl device >>> has been registered. Or am I missing something? >> >> >> As you say this probably works out fine for pins that are used as GPIOs. I >> was thinking more about SFIOs. Take an I2C controller, which doesn't use any >> GPIOs itself. The pinctrl device should be probed before the I2C device, so >> that the I2C driver can initiate transactions on the I2C bus during its >> probe if it wanted to (or at least, clients could initiate transactions at >> any completely arbitrary time as soon as probe was complete). > > What is using the SFIO in this case, the I2C master or the I2C client? The I2C controller a/k/a the I2C master. > In any case, is the problem you are referring to that ICs may rely on s/ICs/drivers/ I think. > a specific pinmux configuration but that's not currently reflected on > the DT because pinmux configuration happened so early that things just > worked? Yes; the dependency of some nodes on pinctrl isn't explicitly called out in the DT. It's probably not a good idea to have such implicit dependencies, although I suppose for something so central as pinmux, maybe it's not terrible, since almost everything depends on it and it's pretty obvious.
On Tue, May 26, 2015 at 9:41 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 05/25/2015 08:53 AM, Tomeu Vizoso wrote: >> >> Specify how the GPIOs map to the pins in T124, so the dependency is >> explicit. >> >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> >> --- >> arch/arm/boot/dts/tegra124.dtsi | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/arch/arm/boot/dts/tegra124.dtsi >> b/arch/arm/boot/dts/tegra124.dtsi >> index 13cc7ca..5d1d35f 100644 >> --- a/arch/arm/boot/dts/tegra124.dtsi >> +++ b/arch/arm/boot/dts/tegra124.dtsi >> @@ -254,6 +254,7 @@ >> gpio-controller; >> #interrupt-cells = <2>; >> interrupt-controller; >> + gpio-ranges = <&pinmux 0 0 250>; > > > We should be consistent between SoCs. Why not make the same change for all > Tegra SoCs? Agreed. > I think this change will cause the GPIO subsystem to call into the pinctrl > subsystem and create/add/register a new GPIO<->pinctrl range structure. The > pinctrl driver already does this, so I think we'll end up with two duplicate > entries in the pinctrl device's gpio_ranges list. This probably won't cause > a problem, but I wanted to make sure you'd thought about it to make sure. That sounds like duplication indeed, I would expect that first a patch adds the ranges to the dts[i] files and then a second patch delete the same ranges from the pinctrl driver then, if these shall come in from the device tree. With GPIO ranges being possible to register from the pin controller, gpio chip and also the device tree, things get a bit complex admittedly :/ sorry for this, just choose one. Yours, Linus Walleij
On 06/02/2015 05:28 AM, Linus Walleij wrote: > On Tue, May 26, 2015 at 9:41 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 05/25/2015 08:53 AM, Tomeu Vizoso wrote: >>> >>> Specify how the GPIOs map to the pins in T124, so the dependency is >>> explicit. >>> >>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> >>> --- >>> arch/arm/boot/dts/tegra124.dtsi | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/arch/arm/boot/dts/tegra124.dtsi >>> b/arch/arm/boot/dts/tegra124.dtsi >>> index 13cc7ca..5d1d35f 100644 >>> --- a/arch/arm/boot/dts/tegra124.dtsi >>> +++ b/arch/arm/boot/dts/tegra124.dtsi >>> @@ -254,6 +254,7 @@ >>> gpio-controller; >>> #interrupt-cells = <2>; >>> interrupt-controller; >>> + gpio-ranges = <&pinmux 0 0 250>; >> >> >> We should be consistent between SoCs. Why not make the same change for all >> Tegra SoCs? > > Agreed. > >> I think this change will cause the GPIO subsystem to call into the pinctrl >> subsystem and create/add/register a new GPIO<->pinctrl range structure. The >> pinctrl driver already does this, so I think we'll end up with two duplicate >> entries in the pinctrl device's gpio_ranges list. This probably won't cause >> a problem, but I wanted to make sure you'd thought about it to make sure. > > That sounds like duplication indeed, I would expect that first a patch > adds the ranges to the dts[i] files and then a second patch delete the > same ranges from the pinctrl driver then, if these shall come in from > the device tree. We can't delete the gpio-range-registration code from the Tegra pinmux driver, or old DTs won't work correctly. We could make it conditional based upon whether the DT contains the property or not.
On 28 May 2015 at 17:50, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 05/28/2015 02:26 AM, Tomeu Vizoso wrote: >> >> On 27 May 2015 at 16:49, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> >>> On 05/27/2015 08:18 AM, Tomeu Vizoso wrote: >>>> >>>> >>>> On 26 May 2015 at 21:41, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>>> >>>>> >>>>> On 05/25/2015 08:53 AM, Tomeu Vizoso wrote: >>>>>> >>>>>> >>>>>> >>>>>> Specify how the GPIOs map to the pins in T124, so the dependency is >>>>>> explicit. >>>>>> >>>>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> >>>>>> --- >>>>>> arch/arm/boot/dts/tegra124.dtsi | 1 + >>>>>> 1 file changed, 1 insertion(+) >>>>>> >>>>>> diff --git a/arch/arm/boot/dts/tegra124.dtsi >>>>>> b/arch/arm/boot/dts/tegra124.dtsi >>>>>> index 13cc7ca..5d1d35f 100644 >>>>>> --- a/arch/arm/boot/dts/tegra124.dtsi >>>>>> +++ b/arch/arm/boot/dts/tegra124.dtsi >>>>>> @@ -254,6 +254,7 @@ >>>>>> gpio-controller; >>>>>> #interrupt-cells = <2>; >>>>>> interrupt-controller; >>>>>> + gpio-ranges = <&pinmux 0 0 250>; >>>>> >>>>> >>>>> >>>>> >>>>> We should be consistent between SoCs. Why not make the same change for >>>>> all >>>>> Tegra SoCs? >>>>> >>>>> I think this change will cause the GPIO subsystem to call into the >>>>> pinctrl >>>>> subsystem and create/add/register a new GPIO<->pinctrl range structure. >>>>> The >>>>> pinctrl driver already does this, so I think we'll end up with two >>>>> duplicate >>>>> entries in the pinctrl device's gpio_ranges list. This probably won't >>>>> cause >>>>> a problem, but I wanted to make sure you'd thought about it to make >>>>> sure. >>>> >>>> >>>> >>>> Actually, I have checked and see that gpio-tegra.c registers 256 >>>> gpios, but pinctrl-tegra124.c adds a range of only 251. I don't really >>>> remember where I got the 250 value from, sorry :( >>>> >>>> I don't see how that would cause any concrete problems, but maybe we >>>> should have a single authoritative source (not sure we can do so >>>> without breaking DT ABI though). >>>> >>>>> Right now, I think we get lucky and pinctrl ends up probing first (or >>>>> at >>>>> least very early) anyway. Somewhat related to this series, I wonder if >>>>> we >>>>> shouldn't add pinctrl client properties to every node in the Tegra DT >>>>> that >>>>> describes a controller that makes use of external pins that are >>>>> affected >>>>> by >>>>> the pinmux. Such a change would guarantee this desired probing order. >>>>> In >>>>> order to preserve the "program the entire pinmux at once" semantics, >>>>> these >>>>> new pinctrl client properties would all need to reference empty states, >>>>> yet >>>>> would still need to exist to represent the dependency. >>>> >>>> >>>> >>>> I think so, but aren't almost all those pins used as gpios? If so, >>>> then such a controller's driver will request the gpio it wants which >>>> will cause the gpio driver to be registered (and hopefully probed) if >>>> needed, which in turn will check that the corresponding pinctrl device >>>> has been registered. Or am I missing something? >>> >>> >>> >>> As you say this probably works out fine for pins that are used as GPIOs. >>> I >>> was thinking more about SFIOs. Take an I2C controller, which doesn't use >>> any >>> GPIOs itself. The pinctrl device should be probed before the I2C device, >>> so >>> that the I2C driver can initiate transactions on the I2C bus during its >>> probe if it wanted to (or at least, clients could initiate transactions >>> at >>> any completely arbitrary time as soon as probe was complete). >> >> >> What is using the SFIO in this case, the I2C master or the I2C client? > > > The I2C controller a/k/a the I2C master. > >> In any case, is the problem you are referring to that ICs may rely on > > > s/ICs/drivers/ I think. > >> a specific pinmux configuration but that's not currently reflected on >> the DT because pinmux configuration happened so early that things just >> worked? > > > Yes; the dependency of some nodes on pinctrl isn't explicitly called out in > the DT. It's probably not a good idea to have such implicit dependencies, > although I suppose for something so central as pinmux, maybe it's not > terrible, since almost everything depends on it and it's pretty obvious. Yup, agreed. Thanks, Tomeu
On 2 June 2015 at 17:40, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 06/02/2015 05:28 AM, Linus Walleij wrote: >> >> On Tue, May 26, 2015 at 9:41 PM, Stephen Warren <swarren@wwwdotorg.org> >> wrote: >>> >>> On 05/25/2015 08:53 AM, Tomeu Vizoso wrote: >>>> >>>> >>>> Specify how the GPIOs map to the pins in T124, so the dependency is >>>> explicit. >>>> >>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> >>>> --- >>>> arch/arm/boot/dts/tegra124.dtsi | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/arch/arm/boot/dts/tegra124.dtsi >>>> b/arch/arm/boot/dts/tegra124.dtsi >>>> index 13cc7ca..5d1d35f 100644 >>>> --- a/arch/arm/boot/dts/tegra124.dtsi >>>> +++ b/arch/arm/boot/dts/tegra124.dtsi >>>> @@ -254,6 +254,7 @@ >>>> gpio-controller; >>>> #interrupt-cells = <2>; >>>> interrupt-controller; >>>> + gpio-ranges = <&pinmux 0 0 250>; >>> >>> >>> >>> We should be consistent between SoCs. Why not make the same change for >>> all >>> Tegra SoCs? >> >> >> Agreed. >> >>> I think this change will cause the GPIO subsystem to call into the >>> pinctrl >>> subsystem and create/add/register a new GPIO<->pinctrl range structure. >>> The >>> pinctrl driver already does this, so I think we'll end up with two >>> duplicate >>> entries in the pinctrl device's gpio_ranges list. This probably won't >>> cause >>> a problem, but I wanted to make sure you'd thought about it to make sure. >> >> >> That sounds like duplication indeed, I would expect that first a patch >> adds the ranges to the dts[i] files and then a second patch delete the >> same ranges from the pinctrl driver then, if these shall come in from >> the device tree. > > > We can't delete the gpio-range-registration code from the Tegra pinmux > driver, or old DTs won't work correctly. We could make it conditional based > upon whether the DT contains the property or not. I've been looking at this and haven't found a good solution. From what I see, the pinctrl driver doesn't have a reference to the gpio device node so cannot tell if it needs to add the range or not. The gpio driver can tell whether it should add the range or not, but if it has to because the gpio-ranges property isn't there, then it doesn't have the reference to the pinctrl device to set the range to. So, given that pinctrl_add_gpio_range is deprecated already, wonder if the lesser evil isn't leaving the duplicated entries for now. On newer SoC revisions such as T210 we can stop calling pinctrl_add_gpio_range at all. Or, we can accept that nobody is going to boot a newer kernel with an older DT on the affected boards and just rely on the presence of the gpio-ranges property :) Regards, Tomeu
On 06/16/2015 02:42 AM, Tomeu Vizoso wrote: > On 2 June 2015 at 17:40, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 06/02/2015 05:28 AM, Linus Walleij wrote: >>> >>> On Tue, May 26, 2015 at 9:41 PM, Stephen Warren <swarren@wwwdotorg.org> >>> wrote: >>>> >>>> On 05/25/2015 08:53 AM, Tomeu Vizoso wrote: >>>>> >>>>> >>>>> Specify how the GPIOs map to the pins in T124, so the dependency is >>>>> explicit. >>>>> >>>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> >>>>> --- >>>>> arch/arm/boot/dts/tegra124.dtsi | 1 + >>>>> 1 file changed, 1 insertion(+) >>>>> >>>>> diff --git a/arch/arm/boot/dts/tegra124.dtsi >>>>> b/arch/arm/boot/dts/tegra124.dtsi >>>>> index 13cc7ca..5d1d35f 100644 >>>>> --- a/arch/arm/boot/dts/tegra124.dtsi >>>>> +++ b/arch/arm/boot/dts/tegra124.dtsi >>>>> @@ -254,6 +254,7 @@ >>>>> gpio-controller; >>>>> #interrupt-cells = <2>; >>>>> interrupt-controller; >>>>> + gpio-ranges = <&pinmux 0 0 250>; >>>> >>>> >>>> >>>> We should be consistent between SoCs. Why not make the same change for >>>> all >>>> Tegra SoCs? >>> >>> >>> Agreed. >>> >>>> I think this change will cause the GPIO subsystem to call into the >>>> pinctrl >>>> subsystem and create/add/register a new GPIO<->pinctrl range structure. >>>> The >>>> pinctrl driver already does this, so I think we'll end up with two >>>> duplicate >>>> entries in the pinctrl device's gpio_ranges list. This probably won't >>>> cause >>>> a problem, but I wanted to make sure you'd thought about it to make sure. >>> >>> >>> That sounds like duplication indeed, I would expect that first a patch >>> adds the ranges to the dts[i] files and then a second patch delete the >>> same ranges from the pinctrl driver then, if these shall come in from >>> the device tree. >> >> >> We can't delete the gpio-range-registration code from the Tegra pinmux >> driver, or old DTs won't work correctly. We could make it conditional based >> upon whether the DT contains the property or not. > > I've been looking at this and haven't found a good solution. From what > I see, the pinctrl driver doesn't have a reference to the gpio device > node so cannot tell if it needs to add the range or not. Well, we know what the node must be called, so the pinctrl driver could search for it by name. > The gpio driver can tell whether it should add the range or not, but > if it has to because the gpio-ranges property isn't there, then it > doesn't have the reference to the pinctrl device to set the range to. > > So, given that pinctrl_add_gpio_range is deprecated already, wonder if > the lesser evil isn't leaving the duplicated entries for now. On newer > SoC revisions such as T210 we can stop calling pinctrl_add_gpio_range > at all. > > Or, we can accept that nobody is going to boot a newer kernel with an > older DT on the affected boards and just rely on the presence of the > gpio-ranges property :) Isn't the simplest solution to just leave it as it is? Nothing's broken is it? For any new SoCs we add, we can certainly switch to a new scheme if we want, but we need to catch/implement that early before the base .dtsi file is included in its first kernel release.
On 16 June 2015 at 22:32, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 06/16/2015 02:42 AM, Tomeu Vizoso wrote: >> >> On 2 June 2015 at 17:40, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> >>> On 06/02/2015 05:28 AM, Linus Walleij wrote: >>>> >>>> >>>> On Tue, May 26, 2015 at 9:41 PM, Stephen Warren <swarren@wwwdotorg.org> >>>> wrote: >>>>> >>>>> >>>>> On 05/25/2015 08:53 AM, Tomeu Vizoso wrote: >>>>>> >>>>>> >>>>>> >>>>>> Specify how the GPIOs map to the pins in T124, so the dependency is >>>>>> explicit. >>>>>> >>>>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> >>>>>> --- >>>>>> arch/arm/boot/dts/tegra124.dtsi | 1 + >>>>>> 1 file changed, 1 insertion(+) >>>>>> >>>>>> diff --git a/arch/arm/boot/dts/tegra124.dtsi >>>>>> b/arch/arm/boot/dts/tegra124.dtsi >>>>>> index 13cc7ca..5d1d35f 100644 >>>>>> --- a/arch/arm/boot/dts/tegra124.dtsi >>>>>> +++ b/arch/arm/boot/dts/tegra124.dtsi >>>>>> @@ -254,6 +254,7 @@ >>>>>> gpio-controller; >>>>>> #interrupt-cells = <2>; >>>>>> interrupt-controller; >>>>>> + gpio-ranges = <&pinmux 0 0 250>; >>>>> >>>>> >>>>> >>>>> >>>>> We should be consistent between SoCs. Why not make the same change for >>>>> all >>>>> Tegra SoCs? >>>> >>>> >>>> >>>> Agreed. >>>> >>>>> I think this change will cause the GPIO subsystem to call into the >>>>> pinctrl >>>>> subsystem and create/add/register a new GPIO<->pinctrl range structure. >>>>> The >>>>> pinctrl driver already does this, so I think we'll end up with two >>>>> duplicate >>>>> entries in the pinctrl device's gpio_ranges list. This probably won't >>>>> cause >>>>> a problem, but I wanted to make sure you'd thought about it to make >>>>> sure. >>>> >>>> >>>> >>>> That sounds like duplication indeed, I would expect that first a patch >>>> adds the ranges to the dts[i] files and then a second patch delete the >>>> same ranges from the pinctrl driver then, if these shall come in from >>>> the device tree. >>> >>> >>> >>> We can't delete the gpio-range-registration code from the Tegra pinmux >>> driver, or old DTs won't work correctly. We could make it conditional >>> based >>> upon whether the DT contains the property or not. >> >> >> I've been looking at this and haven't found a good solution. From what >> I see, the pinctrl driver doesn't have a reference to the gpio device >> node so cannot tell if it needs to add the range or not. > > > Well, we know what the node must be called, so the pinctrl driver could > search for it by name. Ok, will write something in this direction. >> The gpio driver can tell whether it should add the range or not, but >> if it has to because the gpio-ranges property isn't there, then it >> doesn't have the reference to the pinctrl device to set the range to. >> >> So, given that pinctrl_add_gpio_range is deprecated already, wonder if >> the lesser evil isn't leaving the duplicated entries for now. On newer >> SoC revisions such as T210 we can stop calling pinctrl_add_gpio_range >> at all. >> >> Or, we can accept that nobody is going to boot a newer kernel with an >> older DT on the affected boards and just rely on the presence of the >> gpio-ranges property :) > > > Isn't the simplest solution to just leave it as it is? Nothing's broken is > it? Well, without an explicit dependency from the gpio node to the pinctrl node I cannot avoid a deferred probe (which will cause other deferred probes in turn). > For any new SoCs we add, we can certainly switch to a new scheme if we want, > but we need to catch/implement that early before the base .dtsi file is > included in its first kernel release. Definitely. Regards, Tomeu > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi index 13cc7ca..5d1d35f 100644 --- a/arch/arm/boot/dts/tegra124.dtsi +++ b/arch/arm/boot/dts/tegra124.dtsi @@ -254,6 +254,7 @@ gpio-controller; #interrupt-cells = <2>; interrupt-controller; + gpio-ranges = <&pinmux 0 0 250>; }; apbdma: dma@0,60020000 {
Specify how the GPIOs map to the pins in T124, so the dependency is explicit. Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> --- arch/arm/boot/dts/tegra124.dtsi | 1 + 1 file changed, 1 insertion(+)