| Message ID | 1431963229-12867-1-git-send-email-jonathanh@nvidia.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Mon, May 18, 2015 at 04:33:49PM +0100, Jon Hunter wrote: > Background: > ========== > On tegra124 and tegra132 devices the pads used by the Display Port Auxiliary > (DPAUX) channel are multiplexed such that they can also be used by one of the > internal i2c controllers. Note that this is different from i2c-over-AUX > supported by the DPAUX controller. The register that configures these pads is > part of the DPAUX controllers register set and so requires the clock for the > DPAUX controller to be enabled to access the register as well as keeping the > SOR (serial output resource) power domain enabled. > > Currently, there is no pinctrl device for these pads and so cannot be easily > mapped to function as an i2c interface. Furthermore, when using the pads for > the DPAUX channel, the DPAUX driver (drivers/gpu/drm/tegra/dpaux.c) directly > writes the to appropriate register to setup the pads. > > There are some products based upon the tegra132 that use these pads for an > internal i2c controller and hence we want to support this configuration in the > kernel. Good timing, I was going to (reluctantly) add this to my long TODO list. I generally like the proposal. > Proposal: > ======== > Add a DPAUX MFD device that consists of a DPAUX controller, for the Display > Port Auxiliary related functionality and a DPAUX pad controller, for handling > the pinctrl for the DPAUX pads. Both the DPAUX controller and DPAUX pad > controller need to access the DPAUX register set and therefore, by making the > MFD compatible with "simple-mfd" and "syscon", a regmap for the DPAUX registers > will be created to synchronise register accesses made by the drivers. Can we not do without an MFD here? Not only would it break DT ABI, but it's also way more complicated than it needs to be in my opinion, we're only sharing a single register (or perhaps even two) after all. Keeping everything in a single DT node would also make the binding less awkward because the power domain doesn't apply to the pad controller part of DPAUX. Can't the dpaux driver simply register the pinmux controller itself? Thierry
On 19/05/15 15:46, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Mon, May 18, 2015 at 04:33:49PM +0100, Jon Hunter wrote: >> Background: >> ========== >> On tegra124 and tegra132 devices the pads used by the Display Port Auxiliary >> (DPAUX) channel are multiplexed such that they can also be used by one of the >> internal i2c controllers. Note that this is different from i2c-over-AUX >> supported by the DPAUX controller. The register that configures these pads is >> part of the DPAUX controllers register set and so requires the clock for the >> DPAUX controller to be enabled to access the register as well as keeping the >> SOR (serial output resource) power domain enabled. >> >> Currently, there is no pinctrl device for these pads and so cannot be easily >> mapped to function as an i2c interface. Furthermore, when using the pads for >> the DPAUX channel, the DPAUX driver (drivers/gpu/drm/tegra/dpaux.c) directly >> writes the to appropriate register to setup the pads. >> >> There are some products based upon the tegra132 that use these pads for an >> internal i2c controller and hence we want to support this configuration in the >> kernel. > > Good timing, I was going to (reluctantly) add this to my long TODO list. > I generally like the proposal. Ok, great. >> Proposal: >> ======== >> Add a DPAUX MFD device that consists of a DPAUX controller, for the Display >> Port Auxiliary related functionality and a DPAUX pad controller, for handling >> the pinctrl for the DPAUX pads. Both the DPAUX controller and DPAUX pad >> controller need to access the DPAUX register set and therefore, by making the >> MFD compatible with "simple-mfd" and "syscon", a regmap for the DPAUX registers >> will be created to synchronise register accesses made by the drivers. > > Can we not do without an MFD here? Not only would it break DT ABI, but > it's also way more complicated than it needs to be in my opinion, we're > only sharing a single register (or perhaps even two) after all. Keeping > everything in a single DT node would also make the binding less awkward > because the power domain doesn't apply to the pad controller part of > DPAUX. > > Can't the dpaux driver simply register the pinmux controller itself? Do you think something that looks like the below? +Example (tegra124 DPAUX): + +/ { + ... + + host1x { + compatible = "nvidia,tegra124-host1x", "simple-bus"; + ... + + dpaux: dpaux@0,545c0000 { + compatible = "nvidia,tegra124-dpaux", + reg = <0x0 0x545c0000 0x0 0x40000>; + interrupts = <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&tegra_car TEGRA124_CLK_DPAUX>, + <&tegra_car TEGRA124_CLK_PLL_DP>; + clock-names = "dpaux", "parent"; + resets = <&tegra_car 181>; + reset-names = "dpaux"; + pinctrl-0 = <&dpaux_state>; + pinctrl-names = "default"; + status = "disabled"; + + dpaux_padctl@0,545c0124 { + compatible = "nvidia,tegra124-dpaux-padctl"; + + dpaux_state: dpaux_state0 { + dpaux { + nvidia,function = "dpaux"; + }; + }; + + i2c_state: i2c_state0 { + i2c { + nvidia,function = "i2c"; + }; + }; + }; + }; + }; +}; Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 20, 2015 at 02:46:07PM +0100, Jon Hunter wrote: > > On 19/05/15 15:46, Thierry Reding wrote: > > * PGP Signed by an unknown key > > > > On Mon, May 18, 2015 at 04:33:49PM +0100, Jon Hunter wrote: > >> Background: > >> ========== > >> On tegra124 and tegra132 devices the pads used by the Display Port Auxiliary > >> (DPAUX) channel are multiplexed such that they can also be used by one of the > >> internal i2c controllers. Note that this is different from i2c-over-AUX > >> supported by the DPAUX controller. The register that configures these pads is > >> part of the DPAUX controllers register set and so requires the clock for the > >> DPAUX controller to be enabled to access the register as well as keeping the > >> SOR (serial output resource) power domain enabled. > >> > >> Currently, there is no pinctrl device for these pads and so cannot be easily > >> mapped to function as an i2c interface. Furthermore, when using the pads for > >> the DPAUX channel, the DPAUX driver (drivers/gpu/drm/tegra/dpaux.c) directly > >> writes the to appropriate register to setup the pads. > >> > >> There are some products based upon the tegra132 that use these pads for an > >> internal i2c controller and hence we want to support this configuration in the > >> kernel. > > > > Good timing, I was going to (reluctantly) add this to my long TODO list. > > I generally like the proposal. > > Ok, great. > > >> Proposal: > >> ======== > >> Add a DPAUX MFD device that consists of a DPAUX controller, for the Display > >> Port Auxiliary related functionality and a DPAUX pad controller, for handling > >> the pinctrl for the DPAUX pads. Both the DPAUX controller and DPAUX pad > >> controller need to access the DPAUX register set and therefore, by making the > >> MFD compatible with "simple-mfd" and "syscon", a regmap for the DPAUX registers > >> will be created to synchronise register accesses made by the drivers. > > > > Can we not do without an MFD here? Not only would it break DT ABI, but > > it's also way more complicated than it needs to be in my opinion, we're > > only sharing a single register (or perhaps even two) after all. Keeping > > everything in a single DT node would also make the binding less awkward > > because the power domain doesn't apply to the pad controller part of > > DPAUX. > > > > Can't the dpaux driver simply register the pinmux controller itself? > > Do you think something that looks like the below? > > +Example (tegra124 DPAUX): > + > +/ { > + ... > + > + host1x { > + compatible = "nvidia,tegra124-host1x", "simple-bus"; > + ... > + > + dpaux: dpaux@0,545c0000 { > + compatible = "nvidia,tegra124-dpaux", > + reg = <0x0 0x545c0000 0x0 0x40000>; > + interrupts = <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&tegra_car TEGRA124_CLK_DPAUX>, > + <&tegra_car TEGRA124_CLK_PLL_DP>; > + clock-names = "dpaux", "parent"; > + resets = <&tegra_car 181>; > + reset-names = "dpaux"; > + pinctrl-0 = <&dpaux_state>; > + pinctrl-names = "default"; > + status = "disabled"; > + > + dpaux_padctl@0,545c0124 { > + compatible = "nvidia,tegra124-dpaux-padctl"; > + > + dpaux_state: dpaux_state0 { > + dpaux { > + nvidia,function = "dpaux"; > + }; > + }; > + > + i2c_state: i2c_state0 { > + i2c { > + nvidia,function = "i2c"; > + }; > + }; > + }; Why even have this subnode? Couldn't we simply have this: host1x@... { ... dpaux@... { compatible = "nvidia,tegra124-dpaux"; ... pinctrl-0 = <&dpaux_aux_state>; pinctrl-1 = <&dpaux_i2c_state>; pinctrl-names = "aux", "i2c"; ... dpaux_aux_state: pinmux-aux { ... }; dpaux_i2c_state: pinmux-i2c { ... }; }; }; ? We might need to add in indices to tell apart DPAUX and DPAUX1, though perhaps we could refer to these states by path instead of phandle to avoid that. Anyway, I don't see any particular reason why a subnode would be necessary. Thierry
On 20/05/15 16:40, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Wed, May 20, 2015 at 02:46:07PM +0100, Jon Hunter wrote: >> >> On 19/05/15 15:46, Thierry Reding wrote: >>>> Old Signed by an unknown key >>> >>> On Mon, May 18, 2015 at 04:33:49PM +0100, Jon Hunter wrote: >>>> Background: >>>> ========== >>>> On tegra124 and tegra132 devices the pads used by the Display Port Auxiliary >>>> (DPAUX) channel are multiplexed such that they can also be used by one of the >>>> internal i2c controllers. Note that this is different from i2c-over-AUX >>>> supported by the DPAUX controller. The register that configures these pads is >>>> part of the DPAUX controllers register set and so requires the clock for the >>>> DPAUX controller to be enabled to access the register as well as keeping the >>>> SOR (serial output resource) power domain enabled. >>>> >>>> Currently, there is no pinctrl device for these pads and so cannot be easily >>>> mapped to function as an i2c interface. Furthermore, when using the pads for >>>> the DPAUX channel, the DPAUX driver (drivers/gpu/drm/tegra/dpaux.c) directly >>>> writes the to appropriate register to setup the pads. >>>> >>>> There are some products based upon the tegra132 that use these pads for an >>>> internal i2c controller and hence we want to support this configuration in the >>>> kernel. >>> >>> Good timing, I was going to (reluctantly) add this to my long TODO list. >>> I generally like the proposal. >> >> Ok, great. >> >>>> Proposal: >>>> ======== >>>> Add a DPAUX MFD device that consists of a DPAUX controller, for the Display >>>> Port Auxiliary related functionality and a DPAUX pad controller, for handling >>>> the pinctrl for the DPAUX pads. Both the DPAUX controller and DPAUX pad >>>> controller need to access the DPAUX register set and therefore, by making the >>>> MFD compatible with "simple-mfd" and "syscon", a regmap for the DPAUX registers >>>> will be created to synchronise register accesses made by the drivers. >>> >>> Can we not do without an MFD here? Not only would it break DT ABI, but >>> it's also way more complicated than it needs to be in my opinion, we're >>> only sharing a single register (or perhaps even two) after all. Keeping >>> everything in a single DT node would also make the binding less awkward >>> because the power domain doesn't apply to the pad controller part of >>> DPAUX. >>> >>> Can't the dpaux driver simply register the pinmux controller itself? >> >> Do you think something that looks like the below? >> >> +Example (tegra124 DPAUX): >> + >> +/ { >> + ... >> + >> + host1x { >> + compatible = "nvidia,tegra124-host1x", "simple-bus"; >> + ... >> + >> + dpaux: dpaux@0,545c0000 { >> + compatible = "nvidia,tegra124-dpaux", >> + reg = <0x0 0x545c0000 0x0 0x40000>; >> + interrupts = <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>; >> + clocks = <&tegra_car TEGRA124_CLK_DPAUX>, >> + <&tegra_car TEGRA124_CLK_PLL_DP>; >> + clock-names = "dpaux", "parent"; >> + resets = <&tegra_car 181>; >> + reset-names = "dpaux"; >> + pinctrl-0 = <&dpaux_state>; >> + pinctrl-names = "default"; >> + status = "disabled"; >> + >> + dpaux_padctl@0,545c0124 { >> + compatible = "nvidia,tegra124-dpaux-padctl"; >> + >> + dpaux_state: dpaux_state0 { >> + dpaux { >> + nvidia,function = "dpaux"; >> + }; >> + }; >> + >> + i2c_state: i2c_state0 { >> + i2c { >> + nvidia,function = "i2c"; >> + }; >> + }; >> + }; > > Why even have this subnode? Couldn't we simply have this: > > host1x@... { > ... > > dpaux@... { > compatible = "nvidia,tegra124-dpaux"; > ... > pinctrl-0 = <&dpaux_aux_state>; > pinctrl-1 = <&dpaux_i2c_state>; > pinctrl-names = "aux", "i2c"; > ... > > dpaux_aux_state: pinmux-aux { > ... > }; > > dpaux_i2c_state: pinmux-i2c { > ... > }; > }; > }; > > ? > > We might need to add in indices to tell apart DPAUX and DPAUX1, though > perhaps we could refer to these states by path instead of phandle to > avoid that. Anyway, I don't see any particular reason why a subnode > would be necessary. My thinking was that we would have a pinctrl driver for dpaux in drivers/pinctrl/pinctrl-tegra-dpaux.c and therefore, I had assumed that we would need a sub-node and compatible string to probe the device. Are you sugguesting that the pinctrl driver for dpaux lives in drivers/gpu/drm/tegra/dpaux.c? Sorry if I am misunderstanding something here. Jon -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/20/2015 09:54 AM, Jon Hunter wrote: > > On 20/05/15 16:40, Thierry Reding wrote: >> * PGP Signed by an unknown key >> >> On Wed, May 20, 2015 at 02:46:07PM +0100, Jon Hunter wrote: >>> >>> On 19/05/15 15:46, Thierry Reding wrote: >>>>> Old Signed by an unknown key >>>> >>>> On Mon, May 18, 2015 at 04:33:49PM +0100, Jon Hunter wrote: >>>>> Background: >>>>> ========== >>>>> On tegra124 and tegra132 devices the pads used by the Display Port Auxiliary >>>>> (DPAUX) channel are multiplexed such that they can also be used by one of the >>>>> internal i2c controllers. Note that this is different from i2c-over-AUX >>>>> supported by the DPAUX controller. The register that configures these pads is >>>>> part of the DPAUX controllers register set and so requires the clock for the >>>>> DPAUX controller to be enabled to access the register as well as keeping the >>>>> SOR (serial output resource) power domain enabled. >>>>> >>>>> Currently, there is no pinctrl device for these pads and so cannot be easily >>>>> mapped to function as an i2c interface. Furthermore, when using the pads for >>>>> the DPAUX channel, the DPAUX driver (drivers/gpu/drm/tegra/dpaux.c) directly >>>>> writes the to appropriate register to setup the pads. >>>>> >>>>> There are some products based upon the tegra132 that use these pads for an >>>>> internal i2c controller and hence we want to support this configuration in the >>>>> kernel. >>>> >>>> Good timing, I was going to (reluctantly) add this to my long TODO list. >>>> I generally like the proposal. >>> >>> Ok, great. >>> >>>>> Proposal: >>>>> ======== >>>>> Add a DPAUX MFD device that consists of a DPAUX controller, for the Display >>>>> Port Auxiliary related functionality and a DPAUX pad controller, for handling >>>>> the pinctrl for the DPAUX pads. Both the DPAUX controller and DPAUX pad >>>>> controller need to access the DPAUX register set and therefore, by making the >>>>> MFD compatible with "simple-mfd" and "syscon", a regmap for the DPAUX registers >>>>> will be created to synchronise register accesses made by the drivers. >>>> >>>> Can we not do without an MFD here? Not only would it break DT ABI, but >>>> it's also way more complicated than it needs to be in my opinion, we're >>>> only sharing a single register (or perhaps even two) after all. Keeping >>>> everything in a single DT node would also make the binding less awkward >>>> because the power domain doesn't apply to the pad controller part of >>>> DPAUX. >>>> >>>> Can't the dpaux driver simply register the pinmux controller itself? >>> >>> Do you think something that looks like the below? >>> >>> +Example (tegra124 DPAUX): >>> + >>> +/ { >>> + ... >>> + >>> + host1x { >>> + compatible = "nvidia,tegra124-host1x", "simple-bus"; >>> + ... >>> + >>> + dpaux: dpaux@0,545c0000 { >>> + compatible = "nvidia,tegra124-dpaux", >>> + reg = <0x0 0x545c0000 0x0 0x40000>; >>> + interrupts = <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>; >>> + clocks = <&tegra_car TEGRA124_CLK_DPAUX>, >>> + <&tegra_car TEGRA124_CLK_PLL_DP>; >>> + clock-names = "dpaux", "parent"; >>> + resets = <&tegra_car 181>; >>> + reset-names = "dpaux"; >>> + pinctrl-0 = <&dpaux_state>; >>> + pinctrl-names = "default"; >>> + status = "disabled"; >>> + >>> + dpaux_padctl@0,545c0124 { >>> + compatible = "nvidia,tegra124-dpaux-padctl"; >>> + >>> + dpaux_state: dpaux_state0 { >>> + dpaux { >>> + nvidia,function = "dpaux"; >>> + }; >>> + }; >>> + >>> + i2c_state: i2c_state0 { >>> + i2c { >>> + nvidia,function = "i2c"; >>> + }; >>> + }; >>> + }; >> >> Why even have this subnode? Couldn't we simply have this: >> >> host1x@... { >> ... >> >> dpaux@... { >> compatible = "nvidia,tegra124-dpaux"; >> ... >> pinctrl-0 = <&dpaux_aux_state>; >> pinctrl-1 = <&dpaux_i2c_state>; >> pinctrl-names = "aux", "i2c"; >> ... >> >> dpaux_aux_state: pinmux-aux { >> ... >> }; >> >> dpaux_i2c_state: pinmux-i2c { >> ... >> }; >> }; >> }; >> >> ? >> >> We might need to add in indices to tell apart DPAUX and DPAUX1, though >> perhaps we could refer to these states by path instead of phandle to >> avoid that. Anyway, I don't see any particular reason why a subnode >> would be necessary. > > My thinking was that we would have a pinctrl driver for dpaux in > drivers/pinctrl/pinctrl-tegra-dpaux.c and therefore, I had assumed that > we would need a sub-node and compatible string to probe the device. > > Are you sugguesting that the pinctrl driver for dpaux lives in > drivers/gpu/drm/tegra/dpaux.c? > > Sorry if I am misunderstanding something here. I think a single DT node for the single HW block makes sense. IIUC, that would most correctly reflect how the HW is actually structured. I don't see any conceptual reason why the driver that binds to that node can't register as both a pinctrl driver plus anything else it needs to. For example, there are plenty of Linux drivers that register as both GPIO and pinctrl drivers already. If having the same "struct device" register with multiple subsystems doesn't work out (IIRC some subsystems attempt to own the struct device's one driver_data field), then the top-level driver can internally create whatever child devices it needs to do its job. Using MFD to do that feels like overkill in this situation since those child devices are unlikely to ever show up with some different parent device or register offset. Either way, the choice of whether to use MFD or not shouldn't affect the DT binding in any way. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20/05/15 20:12, Stephen Warren wrote: > On 05/20/2015 09:54 AM, Jon Hunter wrote: >> >> On 20/05/15 16:40, Thierry Reding wrote: >>> * PGP Signed by an unknown key >>> >>> On Wed, May 20, 2015 at 02:46:07PM +0100, Jon Hunter wrote: >>>> >>>> On 19/05/15 15:46, Thierry Reding wrote: >>>>>> Old Signed by an unknown key >>>>> >>>>> On Mon, May 18, 2015 at 04:33:49PM +0100, Jon Hunter wrote: >>>>>> Background: >>>>>> ========== >>>>>> On tegra124 and tegra132 devices the pads used by the Display Port >>>>>> Auxiliary >>>>>> (DPAUX) channel are multiplexed such that they can also be used by >>>>>> one of the >>>>>> internal i2c controllers. Note that this is different from >>>>>> i2c-over-AUX >>>>>> supported by the DPAUX controller. The register that configures >>>>>> these pads is >>>>>> part of the DPAUX controllers register set and so requires the >>>>>> clock for the >>>>>> DPAUX controller to be enabled to access the register as well as >>>>>> keeping the >>>>>> SOR (serial output resource) power domain enabled. >>>>>> >>>>>> Currently, there is no pinctrl device for these pads and so cannot >>>>>> be easily >>>>>> mapped to function as an i2c interface. Furthermore, when using >>>>>> the pads for >>>>>> the DPAUX channel, the DPAUX driver >>>>>> (drivers/gpu/drm/tegra/dpaux.c) directly >>>>>> writes the to appropriate register to setup the pads. >>>>>> >>>>>> There are some products based upon the tegra132 that use these >>>>>> pads for an >>>>>> internal i2c controller and hence we want to support this >>>>>> configuration in the >>>>>> kernel. >>>>> >>>>> Good timing, I was going to (reluctantly) add this to my long TODO >>>>> list. >>>>> I generally like the proposal. >>>> >>>> Ok, great. >>>> >>>>>> Proposal: >>>>>> ======== >>>>>> Add a DPAUX MFD device that consists of a DPAUX controller, for >>>>>> the Display >>>>>> Port Auxiliary related functionality and a DPAUX pad controller, >>>>>> for handling >>>>>> the pinctrl for the DPAUX pads. Both the DPAUX controller and >>>>>> DPAUX pad >>>>>> controller need to access the DPAUX register set and therefore, by >>>>>> making the >>>>>> MFD compatible with "simple-mfd" and "syscon", a regmap for the >>>>>> DPAUX registers >>>>>> will be created to synchronise register accesses made by the drivers. >>>>> >>>>> Can we not do without an MFD here? Not only would it break DT ABI, but >>>>> it's also way more complicated than it needs to be in my opinion, >>>>> we're >>>>> only sharing a single register (or perhaps even two) after all. >>>>> Keeping >>>>> everything in a single DT node would also make the binding less >>>>> awkward >>>>> because the power domain doesn't apply to the pad controller part of >>>>> DPAUX. >>>>> >>>>> Can't the dpaux driver simply register the pinmux controller itself? >>>> >>>> Do you think something that looks like the below? >>>> >>>> +Example (tegra124 DPAUX): >>>> + >>>> +/ { >>>> + ... >>>> + >>>> + host1x { >>>> + compatible = "nvidia,tegra124-host1x", "simple-bus"; >>>> + ... >>>> + >>>> + dpaux: dpaux@0,545c0000 { >>>> + compatible = "nvidia,tegra124-dpaux", >>>> + reg = <0x0 0x545c0000 0x0 0x40000>; >>>> + interrupts = <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>; >>>> + clocks = <&tegra_car TEGRA124_CLK_DPAUX>, >>>> + <&tegra_car TEGRA124_CLK_PLL_DP>; >>>> + clock-names = "dpaux", "parent"; >>>> + resets = <&tegra_car 181>; >>>> + reset-names = "dpaux"; >>>> + pinctrl-0 = <&dpaux_state>; >>>> + pinctrl-names = "default"; >>>> + status = "disabled"; >>>> + >>>> + dpaux_padctl@0,545c0124 { >>>> + compatible = >>>> "nvidia,tegra124-dpaux-padctl"; >>>> + >>>> + dpaux_state: dpaux_state0 { >>>> + dpaux { >>>> + nvidia,function = >>>> "dpaux"; >>>> + }; >>>> + }; >>>> + >>>> + i2c_state: i2c_state0 { >>>> + i2c { >>>> + nvidia,function = >>>> "i2c"; >>>> + }; >>>> + }; >>>> + }; >>> >>> Why even have this subnode? Couldn't we simply have this: >>> >>> host1x@... { >>> ... >>> >>> dpaux@... { >>> compatible = "nvidia,tegra124-dpaux"; >>> ... >>> pinctrl-0 = <&dpaux_aux_state>; >>> pinctrl-1 = <&dpaux_i2c_state>; >>> pinctrl-names = "aux", "i2c"; >>> ... >>> >>> dpaux_aux_state: pinmux-aux { >>> ... >>> }; >>> >>> dpaux_i2c_state: pinmux-i2c { >>> ... >>> }; >>> }; >>> }; >>> >>> ? >>> >>> We might need to add in indices to tell apart DPAUX and DPAUX1, though >>> perhaps we could refer to these states by path instead of phandle to >>> avoid that. Anyway, I don't see any particular reason why a subnode >>> would be necessary. >> >> My thinking was that we would have a pinctrl driver for dpaux in >> drivers/pinctrl/pinctrl-tegra-dpaux.c and therefore, I had assumed that >> we would need a sub-node and compatible string to probe the device. >> >> Are you sugguesting that the pinctrl driver for dpaux lives in >> drivers/gpu/drm/tegra/dpaux.c? >> >> Sorry if I am misunderstanding something here. > > I think a single DT node for the single HW block makes sense. IIUC, that > would most correctly reflect how the HW is actually structured. Yes that would be more aligned with the HW. > I don't see any conceptual reason why the driver that binds to that node > can't register as both a pinctrl driver plus anything else it needs to. > For example, there are plenty of Linux drivers that register as both > GPIO and pinctrl drivers already. If having the same "struct device" > register with multiple subsystems doesn't work out (IIRC some subsystems > attempt to own the struct device's one driver_data field), then the > top-level driver can internally create whatever child devices it needs > to do its job. Using MFD to do that feels like overkill in this > situation since those child devices are unlikely to ever show up with > some different parent device or register offset. Either way, the choice > of whether to use MFD or not shouldn't affect the DT binding in any way. Looking at it there should not be a problem here with regard to the driver_data member of the device struct and so I don't see why the tegra_dpaux_probe() could not call pinctrl_register() to register the device. However, it does mean that all the pinctrl/pinmux/pinconf ops for this pinctrl device will need to live in drivers/gpu/drm/tegra/dpaux.c which is fine, but I *believe* that would require moving drivers/pinctrl/pinctrl-utils.h to include/linux/pinctrl/ in order to make use of these functions. May be that is fine too. I could put together a patch series and see what everyone thinks. Jon -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 21, 2015 at 02:03:47PM +0100, Jon Hunter wrote: > > On 20/05/15 20:12, Stephen Warren wrote: > > On 05/20/2015 09:54 AM, Jon Hunter wrote: > >> > >> On 20/05/15 16:40, Thierry Reding wrote: > >>> * PGP Signed by an unknown key > >>> > >>> On Wed, May 20, 2015 at 02:46:07PM +0100, Jon Hunter wrote: > >>>> > >>>> On 19/05/15 15:46, Thierry Reding wrote: > >>>>>> Old Signed by an unknown key > >>>>> > >>>>> On Mon, May 18, 2015 at 04:33:49PM +0100, Jon Hunter wrote: > >>>>>> Background: > >>>>>> ========== > >>>>>> On tegra124 and tegra132 devices the pads used by the Display Port > >>>>>> Auxiliary > >>>>>> (DPAUX) channel are multiplexed such that they can also be used by > >>>>>> one of the > >>>>>> internal i2c controllers. Note that this is different from > >>>>>> i2c-over-AUX > >>>>>> supported by the DPAUX controller. The register that configures > >>>>>> these pads is > >>>>>> part of the DPAUX controllers register set and so requires the > >>>>>> clock for the > >>>>>> DPAUX controller to be enabled to access the register as well as > >>>>>> keeping the > >>>>>> SOR (serial output resource) power domain enabled. > >>>>>> > >>>>>> Currently, there is no pinctrl device for these pads and so cannot > >>>>>> be easily > >>>>>> mapped to function as an i2c interface. Furthermore, when using > >>>>>> the pads for > >>>>>> the DPAUX channel, the DPAUX driver > >>>>>> (drivers/gpu/drm/tegra/dpaux.c) directly > >>>>>> writes the to appropriate register to setup the pads. > >>>>>> > >>>>>> There are some products based upon the tegra132 that use these > >>>>>> pads for an > >>>>>> internal i2c controller and hence we want to support this > >>>>>> configuration in the > >>>>>> kernel. > >>>>> > >>>>> Good timing, I was going to (reluctantly) add this to my long TODO > >>>>> list. > >>>>> I generally like the proposal. > >>>> > >>>> Ok, great. > >>>> > >>>>>> Proposal: > >>>>>> ======== > >>>>>> Add a DPAUX MFD device that consists of a DPAUX controller, for > >>>>>> the Display > >>>>>> Port Auxiliary related functionality and a DPAUX pad controller, > >>>>>> for handling > >>>>>> the pinctrl for the DPAUX pads. Both the DPAUX controller and > >>>>>> DPAUX pad > >>>>>> controller need to access the DPAUX register set and therefore, by > >>>>>> making the > >>>>>> MFD compatible with "simple-mfd" and "syscon", a regmap for the > >>>>>> DPAUX registers > >>>>>> will be created to synchronise register accesses made by the drivers. > >>>>> > >>>>> Can we not do without an MFD here? Not only would it break DT ABI, but > >>>>> it's also way more complicated than it needs to be in my opinion, > >>>>> we're > >>>>> only sharing a single register (or perhaps even two) after all. > >>>>> Keeping > >>>>> everything in a single DT node would also make the binding less > >>>>> awkward > >>>>> because the power domain doesn't apply to the pad controller part of > >>>>> DPAUX. > >>>>> > >>>>> Can't the dpaux driver simply register the pinmux controller itself? > >>>> > >>>> Do you think something that looks like the below? > >>>> > >>>> +Example (tegra124 DPAUX): > >>>> + > >>>> +/ { > >>>> + ... > >>>> + > >>>> + host1x { > >>>> + compatible = "nvidia,tegra124-host1x", "simple-bus"; > >>>> + ... > >>>> + > >>>> + dpaux: dpaux@0,545c0000 { > >>>> + compatible = "nvidia,tegra124-dpaux", > >>>> + reg = <0x0 0x545c0000 0x0 0x40000>; > >>>> + interrupts = <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>; > >>>> + clocks = <&tegra_car TEGRA124_CLK_DPAUX>, > >>>> + <&tegra_car TEGRA124_CLK_PLL_DP>; > >>>> + clock-names = "dpaux", "parent"; > >>>> + resets = <&tegra_car 181>; > >>>> + reset-names = "dpaux"; > >>>> + pinctrl-0 = <&dpaux_state>; > >>>> + pinctrl-names = "default"; > >>>> + status = "disabled"; > >>>> + > >>>> + dpaux_padctl@0,545c0124 { > >>>> + compatible = > >>>> "nvidia,tegra124-dpaux-padctl"; > >>>> + > >>>> + dpaux_state: dpaux_state0 { > >>>> + dpaux { > >>>> + nvidia,function = > >>>> "dpaux"; > >>>> + }; > >>>> + }; > >>>> + > >>>> + i2c_state: i2c_state0 { > >>>> + i2c { > >>>> + nvidia,function = > >>>> "i2c"; > >>>> + }; > >>>> + }; > >>>> + }; > >>> > >>> Why even have this subnode? Couldn't we simply have this: > >>> > >>> host1x@... { > >>> ... > >>> > >>> dpaux@... { > >>> compatible = "nvidia,tegra124-dpaux"; > >>> ... > >>> pinctrl-0 = <&dpaux_aux_state>; > >>> pinctrl-1 = <&dpaux_i2c_state>; > >>> pinctrl-names = "aux", "i2c"; > >>> ... > >>> > >>> dpaux_aux_state: pinmux-aux { > >>> ... > >>> }; > >>> > >>> dpaux_i2c_state: pinmux-i2c { > >>> ... > >>> }; > >>> }; > >>> }; > >>> > >>> ? > >>> > >>> We might need to add in indices to tell apart DPAUX and DPAUX1, though > >>> perhaps we could refer to these states by path instead of phandle to > >>> avoid that. Anyway, I don't see any particular reason why a subnode > >>> would be necessary. > >> > >> My thinking was that we would have a pinctrl driver for dpaux in > >> drivers/pinctrl/pinctrl-tegra-dpaux.c and therefore, I had assumed that > >> we would need a sub-node and compatible string to probe the device. > >> > >> Are you sugguesting that the pinctrl driver for dpaux lives in > >> drivers/gpu/drm/tegra/dpaux.c? > >> > >> Sorry if I am misunderstanding something here. > > > > I think a single DT node for the single HW block makes sense. IIUC, that > > would most correctly reflect how the HW is actually structured. > > Yes that would be more aligned with the HW. > > > I don't see any conceptual reason why the driver that binds to that node > > can't register as both a pinctrl driver plus anything else it needs to. > > For example, there are plenty of Linux drivers that register as both > > GPIO and pinctrl drivers already. If having the same "struct device" > > register with multiple subsystems doesn't work out (IIRC some subsystems > > attempt to own the struct device's one driver_data field), then the > > top-level driver can internally create whatever child devices it needs > > to do its job. Using MFD to do that feels like overkill in this > > situation since those child devices are unlikely to ever show up with > > some different parent device or register offset. Either way, the choice > > of whether to use MFD or not shouldn't affect the DT binding in any way. > > Looking at it there should not be a problem here with regard to the > driver_data member of the device struct and so I don't see why the > tegra_dpaux_probe() could not call pinctrl_register() to register the > device. Yes, I think that'd be the best solution. > However, it does mean that all the pinctrl/pinmux/pinconf ops for this > pinctrl device will need to live in drivers/gpu/drm/tegra/dpaux.c which > is fine, but I *believe* that would require moving > drivers/pinctrl/pinctrl-utils.h to include/linux/pinctrl/ in order to > make use of these functions. May be that is fine too. I could put > together a patch series and see what everyone thinks. I guess it depends mostly on whether Linus (Cc'ed) is willing to have drivers outside of drivers/pinctrl implement pin controllers. If not it'd still be possible to have the split and expose a custom API that would allow the DPAUX driver to register the pinctrl subcomponent (much like we do for the SMMU part of the memory controller). Thierry
On Thu, May 21, 2015 at 4:03 PM, Thierry Reding <thierry.reding@gmail.com> wrote: >> > I don't see any conceptual reason why the driver that binds to that node >> > can't register as both a pinctrl driver plus anything else it needs to. (...) >> Looking at it there should not be a problem here with regard to the >> driver_data member of the device struct and so I don't see why the >> tegra_dpaux_probe() could not call pinctrl_register() to register the >> device. > > Yes, I think that'd be the best solution. I'm pretty much ready to go to any compromises to get DRM/GPU drivers to use internal kernel subsystems. The tendency there is to reimplement all kernel driver frameworks and hammer registers they need to access. There are good reasons for. These drivers are usually so complex that things like probing and power up/down become a nightmare with cross-subsystem dependencies. They are a special case. I had a long discussion with Intel's Daniel Vetter about this and they (Intel) eventually used GPIO for some stuff where it would fit nicely, but didn't go to use fixed regulators as I had suggested. >> However, it does mean that all the pinctrl/pinmux/pinconf ops for this >> pinctrl device will need to live in drivers/gpu/drm/tegra/dpaux.c which >> is fine, Yeah that's cool I already have e.g. GPIO chips all over the map, including DRM IIRC. >> but I *believe* that would require moving >> drivers/pinctrl/pinctrl-utils.h to include/linux/pinctrl/ in order to >> make use of these functions. Well I originally intended that to be private and neat, but whatever. Call it pinctrl-utils-internal.h or something then. >> May be that is fine too. I could put >> together a patch series and see what everyone thinks. > > I guess it depends mostly on whether Linus (Cc'ed) is willing to have > drivers outside of drivers/pinctrl implement pin controllers. For this case: go ahead. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 22, 2015 at 09:03:17AM +0200, Linus Walleij wrote: > On Thu, May 21, 2015 at 4:03 PM, Thierry Reding > <thierry.reding@gmail.com> wrote: > > >> > I don't see any conceptual reason why the driver that binds to that node > >> > can't register as both a pinctrl driver plus anything else it needs to. > (...) > >> Looking at it there should not be a problem here with regard to the > >> driver_data member of the device struct and so I don't see why the > >> tegra_dpaux_probe() could not call pinctrl_register() to register the > >> device. > > > > Yes, I think that'd be the best solution. > > I'm pretty much ready to go to any compromises to get DRM/GPU > drivers to use internal kernel subsystems. The tendency there > is to reimplement all kernel driver frameworks and hammer registers > they need to access. > > There are good reasons for. These drivers are usually so complex > that things like probing and power up/down become a nightmare > with cross-subsystem dependencies. > > They are a special case. I had a long discussion with Intel's > Daniel Vetter about this and they (Intel) eventually used GPIO > for some stuff where it would fit nicely, but didn't go to use fixed > regulators as I had suggested. I think the situation is somewhat more complicated for Intel than it is for ARM because most of these interfaces have grown out of the ARM camp as a means to address the absence of things like ACPI to deal with this kind of resource. It seems one of the obstacles Intel is facing in the mobile/embedded market is that some aspects of the SoCs are structured very similarly to what we're used to from the ARM world, but there's not enough infrastructure to deal with this disconnect. I think they are pretty much at a point now where ARM was a couple of years ago, and I suspect they might run into similar problems unless they address this early on. One of the examples I've seen, and I think that was where you were involved too, was that they now need essentially board files to set up these types of resources. I've always thought x86 should adopt device tree more aggressively, especially in the embedded/mobile space where they're facing the same problems that we've long solved on ARM. Most of the glue is already there, and platforms exist that use it successfully (I've worked on some of them before myself). > >> However, it does mean that all the pinctrl/pinmux/pinconf ops for this > >> pinctrl device will need to live in drivers/gpu/drm/tegra/dpaux.c which > >> is fine, > > Yeah that's cool I already have e.g. GPIO chips all over the map, > including DRM IIRC. > > >> but I *believe* that would require moving > >> drivers/pinctrl/pinctrl-utils.h to include/linux/pinctrl/ in order to > >> make use of these functions. > > Well I originally intended that to be private and neat, but whatever. > Call it pinctrl-utils-internal.h or something then. > > >> May be that is fine too. I could put > >> together a patch series and see what everyone thinks. > > > > I guess it depends mostly on whether Linus (Cc'ed) is willing to have > > drivers outside of drivers/pinctrl implement pin controllers. > > For this case: go ahead. Thanks for being pragmatic here. I think there's potential to make the job as a maintainer more difficult because now you have to scan more than a single directory when you update API, and you potentially have a harder time build testing. But I think that problem can be addressed by improving our tooling rather than by imposing artificial barriers on driver code. Thierry
On 22/05/15 08:03, Linus Walleij wrote: > On Thu, May 21, 2015 at 4:03 PM, Thierry Reding > <thierry.reding@gmail.com> wrote: > >>>> I don't see any conceptual reason why the driver that binds to that node >>>> can't register as both a pinctrl driver plus anything else it needs to. > (...) >>> Looking at it there should not be a problem here with regard to the >>> driver_data member of the device struct and so I don't see why the >>> tegra_dpaux_probe() could not call pinctrl_register() to register the >>> device. >> >> Yes, I think that'd be the best solution. > > I'm pretty much ready to go to any compromises to get DRM/GPU > drivers to use internal kernel subsystems. The tendency there > is to reimplement all kernel driver frameworks and hammer registers > they need to access. Thanks Linus, but after looking at this further I don't think this approach will work after all. In order to call pinctrl_register() from outside the pinctrl directory, also means exposing the pinctrl_dev and pinctrl_desc structures and so this will get messy. Or at least a bigger churn than I would have hoped for. Thierry, what if we add a drivers/pinctrl/pinctrl-tegra-dpaux.c and then simply call platform_device_register_data() from tegra_dpaux_probe() to register the device? I could still use the parent of_node to match the platform data for the dpaux pin controller. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 22, 2015 at 01:17:02PM +0100, Jon Hunter wrote: > > On 22/05/15 08:03, Linus Walleij wrote: > > On Thu, May 21, 2015 at 4:03 PM, Thierry Reding > > <thierry.reding@gmail.com> wrote: > > > >>>> I don't see any conceptual reason why the driver that binds to that node > >>>> can't register as both a pinctrl driver plus anything else it needs to. > > (...) > >>> Looking at it there should not be a problem here with regard to the > >>> driver_data member of the device struct and so I don't see why the > >>> tegra_dpaux_probe() could not call pinctrl_register() to register the > >>> device. > >> > >> Yes, I think that'd be the best solution. > > > > I'm pretty much ready to go to any compromises to get DRM/GPU > > drivers to use internal kernel subsystems. The tendency there > > is to reimplement all kernel driver frameworks and hammer registers > > they need to access. > > Thanks Linus, but after looking at this further I don't think this > approach will work after all. In order to call pinctrl_register() from > outside the pinctrl directory, also means exposing the pinctrl_dev and > pinctrl_desc structures and so this will get messy. Or at least a bigger > churn than I would have hoped for. It's not that much churn, is it? The structure definitions could be moved to include/linux/pinctrl/machine.h. That's included by core.h in drivers/pinctrl anyway, so that should really be all there is to it. > Thierry, what if we add a drivers/pinctrl/pinctrl-tegra-dpaux.c and then > simply call platform_device_register_data() from tegra_dpaux_probe() to > register the device? I could still use the parent of_node to match the > platform data for the dpaux pin controller. That'd work for me too. But if that's what it comes down to, I don't see a need to go through the driver model. Why not simply expose a simple set of functions to setup and tear down the pinctrl that the DPAUX driver can call? I'd still clearly prefer to have the pinctrl code live directly in the DPAUX driver, so I think we should at least give that a shot. Thierry
On 22/05/15 15:37, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Fri, May 22, 2015 at 01:17:02PM +0100, Jon Hunter wrote: >> >> On 22/05/15 08:03, Linus Walleij wrote: >>> On Thu, May 21, 2015 at 4:03 PM, Thierry Reding >>> <thierry.reding@gmail.com> wrote: >>> >>>>>> I don't see any conceptual reason why the driver that binds to that node >>>>>> can't register as both a pinctrl driver plus anything else it needs to. >>> (...) >>>>> Looking at it there should not be a problem here with regard to the >>>>> driver_data member of the device struct and so I don't see why the >>>>> tegra_dpaux_probe() could not call pinctrl_register() to register the >>>>> device. >>>> >>>> Yes, I think that'd be the best solution. >>> >>> I'm pretty much ready to go to any compromises to get DRM/GPU >>> drivers to use internal kernel subsystems. The tendency there >>> is to reimplement all kernel driver frameworks and hammer registers >>> they need to access. >> >> Thanks Linus, but after looking at this further I don't think this >> approach will work after all. In order to call pinctrl_register() from >> outside the pinctrl directory, also means exposing the pinctrl_dev and >> pinctrl_desc structures and so this will get messy. Or at least a bigger >> churn than I would have hoped for. > > It's not that much churn, is it? The structure definitions could be > moved to include/linux/pinctrl/machine.h. That's included by core.h in > drivers/pinctrl anyway, so that should really be all there is to it. I took a quick look and I would need to move pinctrl, pinctrl_dev and pinctrl_state. So may be not too bad. pinctrl/machine.h makes logical sense, but not sure if they would be more suited to pinctrl/pinctrl.h. >> Thierry, what if we add a drivers/pinctrl/pinctrl-tegra-dpaux.c and then >> simply call platform_device_register_data() from tegra_dpaux_probe() to >> register the device? I could still use the parent of_node to match the >> platform data for the dpaux pin controller. > > That'd work for me too. But if that's what it comes down to, I don't see > a need to go through the driver model. Why not simply expose a simple > set of functions to setup and tear down the pinctrl that the DPAUX > driver can call? Sorry, but I don't understand how that would work :-( > I'd still clearly prefer to have the pinctrl code live directly in the > DPAUX driver, so I think we should at least give that a shot. It's fine with me if Linus is ok with the changes. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/22/2015 01:03 AM, Linus Walleij wrote: > On Thu, May 21, 2015 at 4:03 PM, Thierry Reding > <thierry.reding@gmail.com> wrote: > >>>> I don't see any conceptual reason why the driver that binds to that node >>>> can't register as both a pinctrl driver plus anything else it needs to. > (...) >>> Looking at it there should not be a problem here with regard to the >>> driver_data member of the device struct and so I don't see why the >>> tegra_dpaux_probe() could not call pinctrl_register() to register the >>> device. >> >> Yes, I think that'd be the best solution. > > I'm pretty much ready to go to any compromises to get DRM/GPU > drivers to use internal kernel subsystems. The tendency there > is to reimplement all kernel driver frameworks and hammer registers > they need to access. > > There are good reasons for. These drivers are usually so complex > that things like probing and power up/down become a nightmare > with cross-subsystem dependencies. > > They are a special case. I had a long discussion with Intel's > Daniel Vetter about this and they (Intel) eventually used GPIO > for some stuff where it would fit nicely, but didn't go to use fixed > regulators as I had suggested. > >>> However, it does mean that all the pinctrl/pinmux/pinconf ops for this >>> pinctrl device will need to live in drivers/gpu/drm/tegra/dpaux.c which >>> is fine, > > Yeah that's cool I already have e.g. GPIO chips all over the map, > including DRM IIRC. > >>> but I *believe* that would require moving >>> drivers/pinctrl/pinctrl-utils.h to include/linux/pinctrl/ in order to >>> make use of these functions. > > Well I originally intended that to be private and neat, but whatever. > Call it pinctrl-utils-internal.h or something then. Sorry for the bikeshed: perhaps s/internal/provider/? If all providers would need this, does it even make sense to merge it into include/linuxp/pinctrl/pinctrl.h, which defines all the other stuff providers use? -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 22, 2015 at 5:41 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 05/22/2015 01:03 AM, Linus Walleij wrote: >> Well I originally intended that to be private and neat, but whatever. >> Call it pinctrl-utils-internal.h or something then. > > Sorry for the bikeshed: perhaps s/internal/provider/? If all providers would > need this, does it even make sense to merge it into > include/linuxp/pinctrl/pinctrl.h, which defines all the other stuff > providers use? Yeah sort of, I guess it's just local to that subdir for convenience. No big deal if we move the things to the global headers. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22/05/15 15:37, Thierry Reding wrote: > I'd still clearly prefer to have the pinctrl code live directly in the > DPAUX driver, so I think we should at least give that a shot. I have been working on this more this week and the good news is that by using some of the pinconf-generic handlers I can simplify the code and avoid moving headers and structures around. However, I have ran into another issue. The current binding looks like this ... dpaux: dpaux@0,545c0000 { compatible = "nvidia,tegra124-dpaux"; reg = <0x0 0x545c0000 0x0 0x40000>; interrupts = <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>; clocks = <&tegra_car TEGRA124_CLK_DPAUX>, <&tegra_car TEGRA124_CLK_PLL_DP>; clock-names = "dpaux", "parent"; resets = <&tegra_car 181>; reset-names = "dpaux"; status = "disabled"; dpaux_state: dpaux_state0 { dpaux { groups = "dpaux_io"; function = "dpaux"; nvidia,dpaux-drvi; nvidia,dpaux-drvz; nvidia,dpaux-cmh; }; }; i2c_state: i2c_state0 { i2c { groups = "dpaux_io"; function = "i2c"; }; }; This all works well, however, because the display-port binding now has child devices which are not i2c clients I see the following messages during kernel boot ... [ 1.607606] i2c i2c-6: of_i2c: modalias failure on /host1x@0,50000000/dpaux@0,545c0000/dpaux_state0 [ 1.616658] i2c i2c-6: of_i2c: modalias failure on /host1x@0,50000000/dpaux@0,545c0000/i2c_state0 In other words, i2c_add_adapter() (which is called by probing the dpaux) then parses the child nodes looking for i2c client devices. However, these child devices are not i2c client devices and hence the above error messages. I can't find an easy way to avoid this. There is no side-effect from these messages, but I would prefer not to see them. Thierry, my understanding is the i2c-over-aux protocol is a simplified version of the i2c protocol. From a DT perspective would a dpaux device ever have i2c client devices as children like a normal i2c device has? I am wondering if it would be valid in this case to tell the i2c-core not to search for children. Although that sounds like a hack. I am not sure if the i2c folks would allow us to make these dev_dbg() prints. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/29/2015 09:50 AM, Jon Hunter wrote: > > On 22/05/15 15:37, Thierry Reding wrote: >> I'd still clearly prefer to have the pinctrl code live directly in the >> DPAUX driver, so I think we should at least give that a shot. > > I have been working on this more this week and the good news is that by > using some of the pinconf-generic handlers I can simplify the code and > avoid moving headers and structures around. > > However, I have ran into another issue. The current binding looks like > this ... > > dpaux: dpaux@0,545c0000 { > compatible = "nvidia,tegra124-dpaux"; > reg = <0x0 0x545c0000 0x0 0x40000>; > interrupts = <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>; > clocks = <&tegra_car TEGRA124_CLK_DPAUX>, > <&tegra_car TEGRA124_CLK_PLL_DP>; > clock-names = "dpaux", "parent"; > resets = <&tegra_car 181>; > reset-names = "dpaux"; > status = "disabled"; > > dpaux_state: dpaux_state0 { > dpaux { > groups = "dpaux_io"; > function = "dpaux"; > nvidia,dpaux-drvi; > nvidia,dpaux-drvz; > nvidia,dpaux-cmh; > }; > }; > > i2c_state: i2c_state0 { > i2c { > groups = "dpaux_io"; > function = "i2c"; > }; > }; > > This all works well, however, because the display-port binding now has > child devices which are not i2c clients I see the following messages > during kernel boot ... > > [ 1.607606] i2c i2c-6: of_i2c: modalias failure on > /host1x@0,50000000/dpaux@0,545c0000/dpaux_state0 > [ 1.616658] i2c i2c-6: of_i2c: modalias failure on > /host1x@0,50000000/dpaux@0,545c0000/i2c_state0 Hmm. The DT binding doc for dpaux doesn't say anything about the device being an I2C controller and hence inheriting/aggregating the core I2C schema. It should... Equally, being an I2C controller means the node should have #address-cells/#size-cells properties for the I2C address. > In other words, i2c_add_adapter() (which is called by probing the dpaux) > then parses the child nodes looking for i2c client devices. However, > these child devices are not i2c client devices and hence the above error > messages. I can't find an easy way to avoid this. There is no > side-effect from these messages, but I would prefer not to see them. If this were a completely new binding, I think the best way would be to have the dpaux node contain a child node for each semantic service implemented by the device, e.g.: dpaux { compatible = "nvidia,tegra124-dpaux"; ... pinctrl { dpaux_state: dpaux_state0 { ... i2c_state: i2c_state0 { ... }; i2c-bus { // i2c devices go here // I2C core pointed at this sub-node, not the dpaux node }; }; I guess we can't change the binding this way since it already exists, unless we introduce a new compatible value to distinguish the old and new styles. Perhaps i2c_add_adapter should only attempt to instantiate devices for sub-nodes that contain a compatible and/or a reg property? > Thierry, my understanding is the i2c-over-aux protocol is a simplified > version of the i2c protocol. From a DT perspective would a dpaux device > ever have i2c client devices as children like a normal i2c device has? I > am wondering if it would be valid in this case to tell the i2c-core not > to search for children. Although that sounds like a hack. I am not sure > if the i2c folks would allow us to make these dev_dbg() prints. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/06/15 20:24, Stephen Warren wrote: > On 05/29/2015 09:50 AM, Jon Hunter wrote: >> >> On 22/05/15 15:37, Thierry Reding wrote: >>> I'd still clearly prefer to have the pinctrl code live directly in the >>> DPAUX driver, so I think we should at least give that a shot. >> >> I have been working on this more this week and the good news is that by >> using some of the pinconf-generic handlers I can simplify the code and >> avoid moving headers and structures around. >> >> However, I have ran into another issue. The current binding looks like >> this ... >> >> dpaux: dpaux@0,545c0000 { >> compatible = "nvidia,tegra124-dpaux"; >> reg = <0x0 0x545c0000 0x0 0x40000>; >> interrupts = <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>; >> clocks = <&tegra_car TEGRA124_CLK_DPAUX>, >> <&tegra_car TEGRA124_CLK_PLL_DP>; >> clock-names = "dpaux", "parent"; >> resets = <&tegra_car 181>; >> reset-names = "dpaux"; >> status = "disabled"; >> >> dpaux_state: dpaux_state0 { >> dpaux { >> groups = "dpaux_io"; >> function = "dpaux"; >> nvidia,dpaux-drvi; >> nvidia,dpaux-drvz; >> nvidia,dpaux-cmh; >> }; >> }; >> >> i2c_state: i2c_state0 { >> i2c { >> groups = "dpaux_io"; >> function = "i2c"; >> }; >> }; >> >> This all works well, however, because the display-port binding now has >> child devices which are not i2c clients I see the following messages >> during kernel boot ... >> >> [ 1.607606] i2c i2c-6: of_i2c: modalias failure on >> /host1x@0,50000000/dpaux@0,545c0000/dpaux_state0 >> [ 1.616658] i2c i2c-6: of_i2c: modalias failure on >> /host1x@0,50000000/dpaux@0,545c0000/i2c_state0 > > Hmm. The DT binding doc for dpaux doesn't say anything about the device > being an I2C controller and hence inheriting/aggregating the core I2C > schema. It should... Yes that would definitely be clearer. > Equally, being an I2C controller means the node should have > #address-cells/#size-cells properties for the I2C address. Agree. >> In other words, i2c_add_adapter() (which is called by probing the dpaux) >> then parses the child nodes looking for i2c client devices. However, >> these child devices are not i2c client devices and hence the above error >> messages. I can't find an easy way to avoid this. There is no >> side-effect from these messages, but I would prefer not to see them. > > If this were a completely new binding, I think the best way would be to > have the dpaux node contain a child node for each semantic service > implemented by the device, e.g.: > > dpaux { > compatible = "nvidia,tegra124-dpaux"; > ... > pinctrl { > dpaux_state: dpaux_state0 { > ... > i2c_state: i2c_state0 { > ... > }; > i2c-bus { > // i2c devices go here > // I2C core pointed at this sub-node, not the dpaux node > }; > }; > > I guess we can't change the binding this way since it already exists, > unless we introduce a new compatible value to distinguish the old and > new styles. > > Perhaps i2c_add_adapter should only attempt to instantiate devices for > sub-nodes that contain a compatible and/or a reg property? It does. However, because I tried to add the pinctrl mappings as sub-nodes, this causes the i2c-core to dump error messages during initialisation of the dpaux driver, because these child nodes are not i2c devices. So what I have works, but I see these error messages from the i2c-core, which in this case are benign. Therefore, to avoid the i2c error messages, the i2c-bus and pinctrl nodes need to be sub-nodes like you have above. I like your above proposal, but like you said, it does mean adding a new compatibility string not to break existing dtbs. Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
========== On tegra124 and tegra132 devices the pads used by the Display Port Auxiliary (DPAUX) channel are multiplexed such that they can also be used by one of the internal i2c controllers. Note that this is different from i2c-over-AUX supported by the DPAUX controller. The register that configures these pads is part of the DPAUX controllers register set and so requires the clock for the DPAUX controller to be enabled to access the register as well as keeping the SOR (serial output resource) power domain enabled. Currently, there is no pinctrl device for these pads and so cannot be easily mapped to function as an i2c interface. Furthermore, when using the pads for the DPAUX channel, the DPAUX driver (drivers/gpu/drm/tegra/dpaux.c) directly writes the to appropriate register to setup the pads. There are some products based upon the tegra132 that use these pads for an internal i2c controller and hence we want to support this configuration in the kernel. Proposal: ======== Add a DPAUX MFD device that consists of a DPAUX controller, for the Display Port Auxiliary related functionality and a DPAUX pad controller, for handling the pinctrl for the DPAUX pads. Both the DPAUX controller and DPAUX pad controller need to access the DPAUX register set and therefore, by making the MFD compatible with "simple-mfd" and "syscon", a regmap for the DPAUX registers will be created to synchronise register accesses made by the drivers. The DPAUX driver (drivers/gpu/drm/tegra/dpaux.c) will be converted to use the DPAUX pad controller driver to ensure that multiple devices cannot configure the pads without first allocating the pads. The following shows a strawman proposal for the device-tree bindings to add further context to how this would work and get some feedback on this proposal (from the tegra maintainers). If this proposal is reasonable then I plan to generate an initial patch of a working implementation for review (looping in the relevant maintainers). --- .../bindings/gpu/nvidia,tegra20-host1x.txt | 85 +++++++++++++- .../pinctrl/nvidia,tegra124-dpaux-padctl.txt | 130 +++++++++++++++++++++ 2 files changed, 209 insertions(+), 6 deletions(-) create mode 100644 Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-dpaux-padctl.txt diff --git a/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt b/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt index 009f4bfa1590..9ce5586719ff 100644 --- a/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt +++ b/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt @@ -5,9 +5,11 @@ Required properties: - reg: Physical base address and length of the controller's registers. - interrupts: The interrupt outputs from the controller. - #address-cells: The number of cells used to represent physical base addresses - in the host1x address space. Should be 1. + in the host1x address space. Should be 1 for tegra20, tegra30 and tegra114 + and should be 2 for tegra124. - #size-cells: The number of cells used to represent the size of an address - range in the host1x address space. Should be 1. + range in the host1x address space. Should be 1 for tegra20, tegra30 and + tegra114 and should be 2 for tegra124. - ranges: The mapping of the host1x address space to the CPU address space. - clocks: Must contain one entry, for the module clock. See ../clocks/clock-bindings.txt for details. @@ -224,10 +226,25 @@ of the following host1x client modules: - nvidia,dpaux: phandle to a DispayPort AUX interface - dpaux: DisplayPort AUX interface - - compatible: For Tegra124, must contain "nvidia,tegra124-dpaux". Otherwise, - must contain '"nvidia,<chip>-dpaux", "nvidia,tegra124-dpaux"', where - <chip> is tegra132. + - compatible: For Tegra124, must contain '"nvidia,tegra124-dpaux-mfd", + "simple-mfd", "syscon"'. Otherwise, must contain '"nvidia,<chip>-dpaux-mfd", + "nvidia,tegra124-dpaux-mfd", "simple-mfd", "syscon"', where <chip> is + tegra132. - reg: Physical base address and length of the controller's registers. + - #address-cells: The number of cells used to represent physical base + addresses in the host1x address space. Should match host1x node. + - #size-cells: The number of cells used to represent the size of an address + range in the host1x address space. Should match host1x node. + - ranges: Describes mapping of DPAUX registers to parent (host1x) address + space. Should be empty to signify a direct mapping. + + The following nodes, "dpaux_controller" and "dpaux_padctl", are children of + the "dpaux" node. + +- dpaux_controller: DisplayPort AUX controller + - compatible: For Tegra124, must contain "nvidia,tegra124-dpaux-controller". + Otherwise, must contain '"nvidia,<chip>-dpaux-controller", + "nvidia,tegra124-dpaux-controller"', where <chip> is tegra132. - interrupts: The interrupt outputs from the controller. - clocks: Must contain an entry for each entry in clock-names. See ../clocks/clock-bindings.txt for details. @@ -239,8 +256,15 @@ of the following host1x client modules: - reset-names: Must include the following entries: - dpaux - vdd-supply: phandle of a supply that powers the DisplayPort link + - pinctrl-0: phandle of pin-control configuration for the DisplayPort pads. + See ../pinctrl/nvidia,tegra124-dpaux-padctl.txt for details. + - pinctrl-names: Should be "default". -Example: +- dpaux_padctl: DisplayPort AUX Pad Controller + - See ../pinctrl/nvidia,tegra124-dpaux-padctl.txt for details. + + +Example (tegra20): / { ... @@ -376,3 +400,52 @@ Example: ... }; + + +Example (tegra124 DPAUX): + +/ { + ... + + host1x { + compatible = "nvidia,tegra124-host1x", "simple-bus"; + ... + + dpaux: dpaux@0,545c0000 { + compatible = "nvidia,tegra124-dpaux-mfd", "simple-mfd", "syscon"; + reg = <0x0 0x545c0000 0x0 0x40000>; + #address-cells = <2>; + #size-cells = <2>; + ranges; + + dpaux_controller: dpaux_controller@0,545c0000 { + compatible = "nvidia,tegra124-dpaux-controller"; + interrupts = <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&tegra_car TEGRA124_CLK_DPAUX>, + <&tegra_car TEGRA124_CLK_PLL_DP>; + clock-names = "dpaux", "parent"; + resets = <&tegra_car 181>; + reset-names = "dpaux"; + pinctrl-0 = <&dpaux_state>; + pinctrl-names = "default"; + status = "disabled"; + }; + + dpaux_padctl@0,545c0124 { + compatible = "nvidia,tegra124-dpaux-padctl"; + + dpaux_state: dpaux_state0 { + dpaux { + nvidia,function = "dpaux"; + }; + }; + + i2c_state: i2c_state0 { + i2c { + nvidia,function = "i2c"; + }; + }; + }; + }; + }; +}; diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-dpaux-padctl.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-dpaux-padctl.txt new file mode 100644 index 000000000000..9e7226086dd6 --- /dev/null +++ b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra124-dpaux-padctl.txt @@ -0,0 +1,130 @@ +Device tree binding for NVIDIA Tegra DPAUX pad controller +======================================================== + +The Tegra Display Port Auxiliary (DPAUX) pad controller manages two pins +which can be assigned to either the DPAUX channel or to an I2C controller. +For Tegra124 the I2C controller that can be mapped to these pins is +i2c@0,7000d1000. + +This document defines the device-specific binding for the DPAUX pad controller. +Refer to pinctrl-bindings.txt in thiis directory for generic information about +pin controller device tree bindings. + +Required properties: +-------------------- +- compatible: For Tegra124, must contain "nvidia,tegra124-dpaux-padctl". + Otherwise, must contain '"nvidia,<chip>-dpaux-padctl", + "nvidia-tegra124-dpaux-padctl"', where <chip> is tegra132 or + tegra210. +- power-domains: The power controller phandle and power-domain specifier that + specifies power-domain dependency for the DPAUX pad controller. + For Tegra124, the power-domain specifier must be + "TEGRA_POWERGATE_SOR". See ../power/power_domain.txt for + details. + +Pin muxing: +------------ + +Child nodes contain the pinmux configurations following the conventions from +the pinctrl-bindings.txt document. + +Since only two configurations are possible, only two child nodes are needed to +describe the pin mux'ing options for the DPAUX pads. Furthermore, given that +the pad functions are only applicable to a single set of pads, the child nodes +do not need to describe the pads the functions are being applied to. + +When the pads are used for a DPAUX channel, there are additional pin +configuration settings that can be applied. These options are described in +optional properties for the child nodes. + +Required properties: +- nvidia,function: Must be either "dpaux" or "i2c". + +Optional properties: +- nvidia,dpaux-enable-input-rcv: Boolean property, when present enables active + high receiver for the DPAUX channel pad. +- nvidia,dpaux-drvi: A value between 0-63 that controls the active + high output driver current. (TODO: Describe + output current range) +- nvidia,dpaux-drvz: A value between 0-7 that controls the active + high output driver impedance. The available + impedance settings are: + 0 = 78 ohms + 1 = 60 ohms + 2 = 54 ohms + 3 = 50 ohms + 4 = 45 ohms + 5 = 42 ohms + 6 = 39 ohms + 7 = 34 ohms +- nvidia,dpaux-cmh: A value between 0-3 that controls the output + common mode voltage. The available common mode + voltage settings are: + 0 = 0.60V + 1 = 0.64V + 2 = 0.70V + 3 = 0.56V + + +Example: +======== + +SoC file extract: +----------------- + + dpaux: dpaux@0,545c0000 { + ... + + dpaux_controller: dpaux_controller@0,545c0000 { + ... + pinctrl-0 = <&dpaux_state>; + pinctrl-names = "default"; + status = "disabled"; + }; + + dpaux_padctl@0,545c0124 { + compatible = "nvidia,tegra124-dpaux-padctl"; + power-domains = <&pmc TEGRA_POWERGATE_SOR> + + dpaux_state: dpaux_state0 { + dpaux { + nvidia,function = "dpaux"; + }; + }; + + i2c_state: i2c_state0 { + i2c { + nvidia,function = "i2c"; + }; + }; + }; + }; + + ... + + i2c@0,7000d100 { + ... + pinctrl-0 = <&i2c_state>; + pinctrl-names = "default"; + status = "disabled"; + }; + + +Board file extract: +------------------- + + dpaux@0,545c0000 { + ... + + dpaux_padctl@0,545c0124 { + + dpaux_state0 { + dpaux { + nvidia,dpaux-enable-input-rcv; + nvidia,dpaux-drvi = <0x18>; + nvidia,dpaux-drvz = <4>; + nvidia,dpaux-cmh = <2>; + }; + }; + }; + };