| Message ID | 61928ad523a7aeffb8f16d4caad56836b65ae407.1518424204.git.ryder.lee@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Feb 12, 2018 at 5:28 AM, Ryder Lee <ryder.lee@mediatek.com> wrote: > The MediaTek audio hardware block that exposes functionalities that are > handled by separate subsystems in the kernel. These functions are all > mapped somewhere at 0x112xxxxx, and there are some control bits are mixed > up with other functions within the same registers. I still don't think this change is necessary. Just because a hardware block in DT maps to different subsystems in a particular OS doesn't mean you need a DT node for each OS subsystem. What we have subsystems for changes over time and DT shouldn't really be changing based on that. And DT is not the only way to instantiate drivers. Rob
On Mon, 2018-02-19 at 12:29 -0600, Rob Herring wrote: > On Mon, Feb 12, 2018 at 5:28 AM, Ryder Lee <ryder.lee@mediatek.com> wrote: > > The MediaTek audio hardware block that exposes functionalities that are > > handled by separate subsystems in the kernel. These functions are all > > mapped somewhere at 0x112xxxxx, and there are some control bits are mixed > > up with other functions within the same registers. > > I still don't think this change is necessary. > > Just because a hardware block in DT maps to different subsystems in a > particular OS doesn't mean you need a DT node for each OS subsystem. > What we have subsystems for changes over time and DT shouldn't really > be changing based on that. And DT is not the only way to instantiate > drivers. > Apart right now we have the definition of both functions. The other location is here:../sonud/mt2701-afe-pcm.txt. The ways I could come up with are: 1. Add a dummy MFD driver (need to think a new compatible or just use '*-audsys' which has already been picked by clock driver) to instantiate two sub-devices through id_table and mfd_cell. 2. For the sake of simplification - add a new compatible "simple-mfd". 3. The last thing - keep two nodes separated/independent. (x) I'm not sure which one is better. @Lee @Matthias: What do you suggest? Ryder.
On Wed, Feb 21, 2018 at 12:04 AM, Ryder Lee <ryder.lee@mediatek.com> wrote: > On Mon, 2018-02-19 at 12:29 -0600, Rob Herring wrote: >> On Mon, Feb 12, 2018 at 5:28 AM, Ryder Lee <ryder.lee@mediatek.com> wrote: >> > The MediaTek audio hardware block that exposes functionalities that are >> > handled by separate subsystems in the kernel. These functions are all >> > mapped somewhere at 0x112xxxxx, and there are some control bits are mixed >> > up with other functions within the same registers. >> >> I still don't think this change is necessary. >> >> Just because a hardware block in DT maps to different subsystems in a >> particular OS doesn't mean you need a DT node for each OS subsystem. >> What we have subsystems for changes over time and DT shouldn't really >> be changing based on that. And DT is not the only way to instantiate >> drivers. >> > > Apart right now we have the definition of both functions. The other > location is here:../sonud/mt2701-afe-pcm.txt. The ways I could come up > with are: There are several problems you need to fix. First, "mediatek,mt2701-audsys" is not documented. It is only used in the example. Second, bindings/arm/mediatek/mediatek,audsys.txt should move to bindings/sound/ if it is only audio related functions. Or perhaps just combine the 2 documents because it is all inconsistent currently. The 2 documents are inconsistent as to what is the relationship of -audsys and -audio (afe) nodes. mt2701-afe-pcm.txt shows that the AFE is already a child of -audsys. The -audsys node should have #clock-cells. It should also not be a simple-mfd (another inconsistency in the binding) because it needs to probe first to provide clocks to child nodes, and then trigger probing the child nodes. Rob > > 1. Add a dummy MFD driver (need to think a new compatible or just use > '*-audsys' which has already been picked by clock driver) to instantiate > two sub-devices through id_table and mfd_cell. > > 2. For the sake of simplification - add a new compatible "simple-mfd". > > 3. The last thing - keep two nodes separated/independent. (x) > > I'm not sure which one is better. > > > > @Lee @Matthias: What do you suggest? > > Ryder. > >
On Wed, 2018-02-21 at 08:10 -0600, Rob Herring wrote: > On Wed, Feb 21, 2018 at 12:04 AM, Ryder Lee <ryder.lee@mediatek.com> wrote: > > On Mon, 2018-02-19 at 12:29 -0600, Rob Herring wrote: > >> On Mon, Feb 12, 2018 at 5:28 AM, Ryder Lee <ryder.lee@mediatek.com> wrote: > >> > The MediaTek audio hardware block that exposes functionalities that are > >> > handled by separate subsystems in the kernel. These functions are all > >> > mapped somewhere at 0x112xxxxx, and there are some control bits are mixed > >> > up with other functions within the same registers. > >> > >> I still don't think this change is necessary. > >> > >> Just because a hardware block in DT maps to different subsystems in a > >> particular OS doesn't mean you need a DT node for each OS subsystem. > >> What we have subsystems for changes over time and DT shouldn't really > >> be changing based on that. And DT is not the only way to instantiate > >> drivers. > >> > > > > Apart right now we have the definition of both functions. The other > > location is here:../sonud/mt2701-afe-pcm.txt. The ways I could come up > > with are: > > There are several problems you need to fix. First, > "mediatek,mt2701-audsys" is not documented. It is only used in the > example. Second, bindings/arm/mediatek/mediatek,audsys.txt should move > to bindings/sound/ if it is only audio related functions. Or perhaps > just combine the 2 documents because it is all inconsistent currently. Because the series crossed subsystems but didn't apply at the same time. > The 2 documents are inconsistent as to what is the relationship of > -audsys and -audio (afe) nodes. mt2701-afe-pcm.txt shows that the AFE > is already a child of -audsys. The -audsys node should have > #clock-cells. It should also not be a simple-mfd (another > inconsistency in the binding) because it needs to probe first to > provide clocks to child nodes, and then trigger probing the child > nodes. This is the 1st version I sent before, and the clock parts still under review :( . But yes, the 2 inconsistent documents should be fixed - this may depend on what we end up doing with the DT appearance. IMHO, apart from overlapping regions with other functions I didn't see any difference between audsys and other clock drivers (providers). For the sake of uniformity, I make the 2 sub-devices parallel and move "simple-mfd" to the top, and the sequences should actually be handled through "probe deferral mechanism" - that would make this kind of situations much easier to manage. BTW, I could make the AFE driver be instantiated/probed from the clock driver but this seems superfluous to me. Just make sure is this what you want? Ryder
On Wed, Feb 21, 2018 at 10:14 PM, Ryder Lee <ryder.lee@mediatek.com> wrote: > On Wed, 2018-02-21 at 08:10 -0600, Rob Herring wrote: >> On Wed, Feb 21, 2018 at 12:04 AM, Ryder Lee <ryder.lee@mediatek.com> wrote: >> > On Mon, 2018-02-19 at 12:29 -0600, Rob Herring wrote: >> >> On Mon, Feb 12, 2018 at 5:28 AM, Ryder Lee <ryder.lee@mediatek.com> wrote: >> >> > The MediaTek audio hardware block that exposes functionalities that are >> >> > handled by separate subsystems in the kernel. These functions are all >> >> > mapped somewhere at 0x112xxxxx, and there are some control bits are mixed >> >> > up with other functions within the same registers. >> >> >> >> I still don't think this change is necessary. >> >> >> >> Just because a hardware block in DT maps to different subsystems in a >> >> particular OS doesn't mean you need a DT node for each OS subsystem. >> >> What we have subsystems for changes over time and DT shouldn't really >> >> be changing based on that. And DT is not the only way to instantiate >> >> drivers. >> >> >> > >> > Apart right now we have the definition of both functions. The other >> > location is here:../sonud/mt2701-afe-pcm.txt. The ways I could come up >> > with are: >> >> There are several problems you need to fix. First, >> "mediatek,mt2701-audsys" is not documented. It is only used in the >> example. Second, bindings/arm/mediatek/mediatek,audsys.txt should move >> to bindings/sound/ if it is only audio related functions. Or perhaps >> just combine the 2 documents because it is all inconsistent currently. > > Because the series crossed subsystems but didn't apply at the same time. > >> The 2 documents are inconsistent as to what is the relationship of >> -audsys and -audio (afe) nodes. mt2701-afe-pcm.txt shows that the AFE >> is already a child of -audsys. The -audsys node should have >> #clock-cells. It should also not be a simple-mfd (another >> inconsistency in the binding) because it needs to probe first to >> provide clocks to child nodes, and then trigger probing the child >> nodes. > > This is the 1st version I sent before, and the clock parts still under > review :( . But yes, the 2 inconsistent documents should be fixed - > this may depend on what we end up doing with the DT appearance. > > IMHO, apart from overlapping regions with other functions I didn't see > any difference between audsys and other clock drivers (providers). > > For the sake of uniformity, I make the 2 sub-devices parallel and move > "simple-mfd" to the top, and the sequences should actually be handled > through "probe deferral mechanism" - that would make this kind of > situations much easier to manage. If a child node has a dependency on the parent, probe deferral is not the correct solution. The parent should probe first and control probing of the chlidren. "simple-mfd" is intended for cases where there is no dependency on the parent node. > BTW, I could make the AFE driver be instantiated/probed from the clock > driver but this seems superfluous to me. Just make sure is this what > you want? I would think it would be the other way around. The AFE driver creates some clocks. Whether that's a separate driver or a single driver is a kernel issue that has nothing to do with the binding. IIRC, there's several examples in display controllers and camera interfaces where those drivers are also clock providers. Rob
On Wed, 2018-02-28 at 09:13 -0600, Rob Herring wrote: > On Wed, Feb 21, 2018 at 10:14 PM, Ryder Lee <ryder.lee@mediatek.com> wrote: > > On Wed, 2018-02-21 at 08:10 -0600, Rob Herring wrote: > >> On Wed, Feb 21, 2018 at 12:04 AM, Ryder Lee <ryder.lee@mediatek.com> wrote: > >> > On Mon, 2018-02-19 at 12:29 -0600, Rob Herring wrote: > >> >> On Mon, Feb 12, 2018 at 5:28 AM, Ryder Lee <ryder.lee@mediatek.com> wrote: > >> >> > The MediaTek audio hardware block that exposes functionalities that are > >> >> > handled by separate subsystems in the kernel. These functions are all > >> >> > mapped somewhere at 0x112xxxxx, and there are some control bits are mixed > >> >> > up with other functions within the same registers. > >> >> > >> >> I still don't think this change is necessary. > >> >> > >> >> Just because a hardware block in DT maps to different subsystems in a > >> >> particular OS doesn't mean you need a DT node for each OS subsystem. > >> >> What we have subsystems for changes over time and DT shouldn't really > >> >> be changing based on that. And DT is not the only way to instantiate > >> >> drivers. > >> >> > >> > > >> > Apart right now we have the definition of both functions. The other > >> > location is here:../sonud/mt2701-afe-pcm.txt. The ways I could come up > >> > with are: > >> > >> There are several problems you need to fix. First, > >> "mediatek,mt2701-audsys" is not documented. It is only used in the > >> example. Second, bindings/arm/mediatek/mediatek,audsys.txt should move > >> to bindings/sound/ if it is only audio related functions. Or perhaps > >> just combine the 2 documents because it is all inconsistent currently. > > > > Because the series crossed subsystems but didn't apply at the same time. > > > >> The 2 documents are inconsistent as to what is the relationship of > >> -audsys and -audio (afe) nodes. mt2701-afe-pcm.txt shows that the AFE > >> is already a child of -audsys. The -audsys node should have > >> #clock-cells. It should also not be a simple-mfd (another > >> inconsistency in the binding) because it needs to probe first to > >> provide clocks to child nodes, and then trigger probing the child > >> nodes. > > > > This is the 1st version I sent before, and the clock parts still under > > review :( . But yes, the 2 inconsistent documents should be fixed - > > this may depend on what we end up doing with the DT appearance. > > > > IMHO, apart from overlapping regions with other functions I didn't see > > any difference between audsys and other clock drivers (providers). > > > > For the sake of uniformity, I make the 2 sub-devices parallel and move > > "simple-mfd" to the top, and the sequences should actually be handled > > through "probe deferral mechanism" - that would make this kind of > > situations much easier to manage. > > If a child node has a dependency on the parent, probe deferral is not > the correct solution. The parent should probe first and control > probing of the chlidren. "simple-mfd" is intended for cases where > there is no dependency on the parent node. > I think the device hierarchy for AUDSYS has to be depicted more appropriately at first before we keep going to discuss the topic. ------- clock-controller - AUDSYS ---------- audio-controller - ------- other controllers not being modeled yet Currently, AUDSYS just works as a container containing all the devices present within the range reserved for audio relevant function and even partial registers and bits for these devices are mixing within some of the range. And for clock controller, they're just all being used to provide to the audio-controller or other functional devices under AUDSYS internally. Thus, deferral actually happens between siblings, rather than between parent and child. For example, the probe deferral Ryder said is pointing to that audio-controller needs to wait until all resources like clock resources all in ready state. I'm not fully sure MFD can be used properly to describe such kind of device hierarchy, but it's really worth having a good way to describe the composition of hardware blocks precisely in device tree and to ease extension device support under AUDSYS for future. There is also the same thing happened in MMSYS as [1] stated. [1] https://lkml.org/lkml/2017/11/14/669 > > BTW, I could make the AFE driver be instantiated/probed from the clock > > driver but this seems superfluous to me. Just make sure is this what > > you want? > > I would think it would be the other way around. The AFE driver creates > some clocks. Whether that's a separate driver or a single driver is a > kernel issue that has nothing to do with the binding. IIRC, there's > several examples in display controllers and camera interfaces where > those drivers are also clock providers. > > Rob > > _______________________________________________ > Linux-mediatek mailing list > Linux-mediatek@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-mediatek
On Wed, 2018-02-28 at 09:13 -0600, Rob Herring wrote: > >> There are several problems you need to fix. First, > >> "mediatek,mt2701-audsys" is not documented. It is only used in the > >> example. Second, bindings/arm/mediatek/mediatek,audsys.txt should move > >> to bindings/sound/ if it is only audio related functions. Or perhaps > >> just combine the 2 documents because it is all inconsistent currently. > > > > Because the series crossed subsystems but didn't apply at the same time. > > > >> The 2 documents are inconsistent as to what is the relationship of > >> -audsys and -audio (afe) nodes. mt2701-afe-pcm.txt shows that the AFE > >> is already a child of -audsys. The -audsys node should have > >> #clock-cells. It should also not be a simple-mfd (another > >> inconsistency in the binding) because it needs to probe first to > >> provide clocks to child nodes, and then trigger probing the child > >> nodes. > > > > This is the 1st version I sent before, and the clock parts still under > > review :( . But yes, the 2 inconsistent documents should be fixed - > > this may depend on what we end up doing with the DT appearance. > > > > IMHO, apart from overlapping regions with other functions I didn't see > > any difference between audsys and other clock drivers (providers). > > > > For the sake of uniformity, I make the 2 sub-devices parallel and move > > "simple-mfd" to the top, and the sequences should actually be handled > > through "probe deferral mechanism" - that would make this kind of > > situations much easier to manage. > > If a child node has a dependency on the parent, probe deferral is not > the correct solution. The parent should probe first and control > probing of the chlidren. "simple-mfd" is intended for cases where > there is no dependency on the parent node. In my opinion, they are just the provider-consumer relationship not the parent-child relationship, and we leverage this mechanism for most CCF users. Please correct me if I'm wrong. That's why I sent the a new one to separate them (but I forgot to modify mt2701-afe-pcm.txt). > > BTW, I could make the AFE driver be instantiated/probed from the clock > > driver but this seems superfluous to me. Just make sure is this what > > you want? > > I would think it would be the other way around. The AFE driver creates > some clocks. Whether that's a separate driver or a single driver is a > kernel issue that has nothing to do with the binding. IIRC, there's > several examples in display controllers and camera interfaces where > those drivers are also clock providers. I've look at some display drivers, but couldn't find a good case. The different is, that the driver I saw provides clocks to other submodules, while the AFE is just a user. So it's odd if we register AFE itself as a clock provider (and use the clocks it generates).
diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,audsys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,audsys.txt index 9b8f578..677af40 100644 --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,audsys.txt +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,audsys.txt @@ -1,22 +1,45 @@ -MediaTek AUDSYS controller +MediaTek Audio Subsystem ============================ +The audio subsystem is one of the multi-function blocks of MediaTek SoCs. +It contains a system controller, which provides a number registers giving +access to two features: AUDSYS clocks and Audio Front End (AFE) components. +For the top level node: +- compatible: must be: "syscon", "simple-mfd"; +- reg: register area of the Audio Subsystem + +Required sub-nodes: + +AUDSYS clocks: +------- The MediaTek AUDSYS controller provides various clocks to the system. Required Properties: - compatible: Should be one of: - - "mediatek,mt7622-audsys", "syscon" + - "mediatek,mt2701-audsys"; + - "mediatek,mt7622-audsys"; - #clock-cells: Must be 1 The AUDSYS controller uses the common clk binding from Documentation/devicetree/bindings/clock/clock-bindings.txt The available clocks are defined in dt-bindings/clock/mt*-clk.h. +AFE components: +------- +For common binding part and usage, refer to +../sonud/mt2701-afe-pcm.txt. + Example: -audsys: audsys@11220000 { - compatible = "mediatek,mt7622-audsys", "syscon"; - reg = <0 0x11220000 0 0x1000>; - #clock-cells = <1>; -}; + audio-subsystem@11220000 { + compatible = "syscon", "simple-mfd"; + reg = <0 0x11220000 0 0x2000>; + + audsys: clock { + compatible = "mediatek,mt2701-audsys"; + #clock-cells = <1>; + }; + + ... + };
The MediaTek audio hardware block that exposes functionalities that are handled by separate subsystems in the kernel. These functions are all mapped somewhere at 0x112xxxxx, and there are some control bits are mixed up with other functions within the same registers. Therefore, it's better to switch the AUDSYS to MFD device for the sake of avoiding adding individual nodes with the overlapping regions for those devices. This patch adds a top level node "simple-mfd" to represent whole subsystem, which consists of two portions - audio componets and clock part. Signed-off-by: Ryder Lee <ryder.lee@mediatek.com> --- .../bindings/arm/mediatek/mediatek,audsys.txt | 37 ++++++++++++++++++---- 1 file changed, 30 insertions(+), 7 deletions(-)