| Message ID | 20180717220328.792-5-matthias.bgg@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2018-07-18 at 00:03 +0200, matthias.bgg@kernel.org wrote: > From: Matthias Brugger <mbrugger@suse.com> > > The MMSYS subsystem includes clocks and drm components. > This patch adds an initailization path through a platform device > for the clock part, so that both drivers get probed from the same > device tree compatible. > Sorry for that I should have a response earlier for the series. Some points I felt they're not exactly right and should be fixed up before we're moving on Currently, drm driver have a wrong reference to the dt-binding, "mediatek,mt2701-mmsys" or "mediatek,mt8173-mmsys", they should be all for the subsystem exporting clock and reset line such common resource to its sub-devices. Every subsystem has a similar shape. I hope mmsys shouldn't be an exception. DRM device needs to have its own dt-binding show how connections between DRM components being made and its node should be put under mmsys node. In this way, it becomes easy to see how the topology of the subsystem is and grows, like a tree "device tree", instead of hiding the details in the implementation. The similar example we already did for audsys on mt2701 and mt7623 as below audsys: clock-controller@11220000 { compatible = "mediatek,mt7623-audsys", "mediatek,mt2701-audsys", "syscon"; ... afe: audio-controller { compatible = "mediatek,mt7623-audio", "mediatek,mt2701-audio"; ... }; }; Sean > Signed-off-by: Matthias Brugger <mbrugger@suse.com> > --- > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 18 ++++++++++++++++++ > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 2 ++ > 2 files changed, 20 insertions(+) > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c > index dd249cf5121e..c946aea722e5 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c > @@ -173,6 +173,7 @@ static const struct mtk_mmsys_driver_data mt2701_mmsys_driver_data = { > .ext_path = mt2701_mtk_ddp_ext, > .ext_len = ARRAY_SIZE(mt2701_mtk_ddp_ext), > .shadow_register = true, > + .clk_drv_name = "clk-mt2701-mm", > }; > > static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = { > @@ -180,6 +181,7 @@ static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = { > .main_len = ARRAY_SIZE(mt8173_mtk_ddp_main), > .ext_path = mt8173_mtk_ddp_ext, > .ext_len = ARRAY_SIZE(mt8173_mtk_ddp_ext), > + .clk_drv_name = "clk-mt8173-mm", > }; > > static int mtk_drm_kms_init(struct drm_device *drm) > @@ -411,6 +413,19 @@ static int mtk_drm_probe(struct platform_device *pdev) > if (IS_ERR(private->config_regs)) > return PTR_ERR(private->config_regs); > > + if (private->data->clk_drv_name) { > + private->clk_dev = platform_device_register_data(dev, > + private->data->clk_drv_name, -1, > + NULL, 0); > + > + if (IS_ERR(private->clk_dev)) { > + pr_err("failed to register %s platform device\n", > + private->data->clk_drv_name); > + > + return PTR_ERR(private->clk_dev); > + } > + } > + > /* Iterate over sibling DISP function blocks */ > for_each_child_of_node(dev->of_node->parent, node) { > const struct of_device_id *of_id; > @@ -515,6 +530,9 @@ static int mtk_drm_remove(struct platform_device *pdev) > for (i = 0; i < DDP_COMPONENT_ID_MAX; i++) > of_node_put(private->comp_node[i]); > > + if (private->clk_dev) > + platform_device_unregister(private->clk_dev); > + > return 0; > } > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h > index 86cec19193c4..200eee5de419 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h > @@ -34,11 +34,13 @@ struct mtk_mmsys_driver_data { > const enum mtk_ddp_comp_id *ext_path; > unsigned int ext_len; > bool shadow_register; > + const char *clk_drv_name; > }; > > struct mtk_drm_private { > struct drm_device *drm; > struct device *dma_dev; > + struct platform_device *clk_dev; > > unsigned int num_pipes; >
Hi Sean, On 18/07/18 04:57, Sean Wang wrote: > On Wed, 2018-07-18 at 00:03 +0200, matthias.bgg@kernel.org wrote: >> From: Matthias Brugger <mbrugger@suse.com> >> >> The MMSYS subsystem includes clocks and drm components. >> This patch adds an initailization path through a platform device >> for the clock part, so that both drivers get probed from the same >> device tree compatible. >> > > Sorry for that I should have a response earlier for the series. > > Some points I felt they're not exactly right and should be fixed up > before we're moving on > > Currently, drm driver have a wrong reference to the dt-binding, > "mediatek,mt2701-mmsys" or "mediatek,mt8173-mmsys", they should be all > for the subsystem exporting clock and reset line such common resource to > its sub-devices. Every subsystem has a similar shape. I hope mmsys > shouldn't be an exception. I'm not sure if I understand what you mean. You proposed to use a platform device [1] and now you say we should change the DTS? What do you mean by "drm driver has a wrong reference to the dt-binding"? How would you want to name it and most important, what would be your approach to have backwards compatibility to older device tree bindings? > > DRM device needs to have its own dt-binding show how connections between > DRM components being made and its node should be put under mmsys node. > > In this way, it becomes easy to see how the topology of the subsystem is > and grows, like a tree "device tree", instead of hiding the details in > the implementation. > > The similar example we already did for audsys on mt2701 and mt7623 as > below > > audsys: clock-controller@11220000 { > compatible = "mediatek,mt7623-audsys", > "mediatek,mt2701-audsys", > "syscon"; > ... > > afe: audio-controller { > compatible = "mediatek,mt7623-audio", > "mediatek,mt2701-audio"; > > ... > }; > }; > I think this approach is not working as, due to backwards compatibility, we have to make sure that the DRM driver gets probed with the mmsys binding. Regards, Matthias > > Sean > > >> Signed-off-by: Matthias Brugger <mbrugger@suse.com> >> --- >> drivers/gpu/drm/mediatek/mtk_drm_drv.c | 18 ++++++++++++++++++ >> drivers/gpu/drm/mediatek/mtk_drm_drv.h | 2 ++ >> 2 files changed, 20 insertions(+) >> >> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c >> index dd249cf5121e..c946aea722e5 100644 >> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c >> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c >> @@ -173,6 +173,7 @@ static const struct mtk_mmsys_driver_data mt2701_mmsys_driver_data = { >> .ext_path = mt2701_mtk_ddp_ext, >> .ext_len = ARRAY_SIZE(mt2701_mtk_ddp_ext), >> .shadow_register = true, >> + .clk_drv_name = "clk-mt2701-mm", >> }; >> >> static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = { >> @@ -180,6 +181,7 @@ static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = { >> .main_len = ARRAY_SIZE(mt8173_mtk_ddp_main), >> .ext_path = mt8173_mtk_ddp_ext, >> .ext_len = ARRAY_SIZE(mt8173_mtk_ddp_ext), >> + .clk_drv_name = "clk-mt8173-mm", >> }; >> >> static int mtk_drm_kms_init(struct drm_device *drm) >> @@ -411,6 +413,19 @@ static int mtk_drm_probe(struct platform_device *pdev) >> if (IS_ERR(private->config_regs)) >> return PTR_ERR(private->config_regs); >> >> + if (private->data->clk_drv_name) { >> + private->clk_dev = platform_device_register_data(dev, >> + private->data->clk_drv_name, -1, >> + NULL, 0); >> + >> + if (IS_ERR(private->clk_dev)) { >> + pr_err("failed to register %s platform device\n", >> + private->data->clk_drv_name); >> + >> + return PTR_ERR(private->clk_dev); >> + } >> + } >> + >> /* Iterate over sibling DISP function blocks */ >> for_each_child_of_node(dev->of_node->parent, node) { >> const struct of_device_id *of_id; >> @@ -515,6 +530,9 @@ static int mtk_drm_remove(struct platform_device *pdev) >> for (i = 0; i < DDP_COMPONENT_ID_MAX; i++) >> of_node_put(private->comp_node[i]); >> >> + if (private->clk_dev) >> + platform_device_unregister(private->clk_dev); >> + >> return 0; >> } >> >> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h >> index 86cec19193c4..200eee5de419 100644 >> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h >> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h >> @@ -34,11 +34,13 @@ struct mtk_mmsys_driver_data { >> const enum mtk_ddp_comp_id *ext_path; >> unsigned int ext_len; >> bool shadow_register; >> + const char *clk_drv_name; >> }; >> >> struct mtk_drm_private { >> struct drm_device *drm; >> struct device *dma_dev; >> + struct platform_device *clk_dev; >> >> unsigned int num_pipes; >> > >
On 18/07/18 09:27, Matthias Brugger wrote: > Hi Sean, > > On 18/07/18 04:57, Sean Wang wrote: >> On Wed, 2018-07-18 at 00:03 +0200, matthias.bgg@kernel.org wrote: >>> From: Matthias Brugger <mbrugger@suse.com> >>> >>> The MMSYS subsystem includes clocks and drm components. >>> This patch adds an initailization path through a platform device >>> for the clock part, so that both drivers get probed from the same >>> device tree compatible. >>> >> >> Sorry for that I should have a response earlier for the series. >> >> Some points I felt they're not exactly right and should be fixed up >> before we're moving on >> >> Currently, drm driver have a wrong reference to the dt-binding, >> "mediatek,mt2701-mmsys" or "mediatek,mt8173-mmsys", they should be all >> for the subsystem exporting clock and reset line such common resource to >> its sub-devices. Every subsystem has a similar shape. I hope mmsys >> shouldn't be an exception. > > I'm not sure if I understand what you mean. You proposed to use a platform > device [1] and now you say we should change the DTS? > > What do you mean by "drm driver has a wrong reference to the dt-binding"? How > would you want to name it and most important, what would be your approach to > have backwards compatibility to older device tree bindings? > I forgot the reference: [1] https://patchwork.kernel.org/patch/10367797/ >> >> DRM device needs to have its own dt-binding show how connections between >> DRM components being made and its node should be put under mmsys node. >>> In this way, it becomes easy to see how the topology of the subsystem is >> and grows, like a tree "device tree", instead of hiding the details in >> the implementation. >> >> The similar example we already did for audsys on mt2701 and mt7623 as >> below >> >> audsys: clock-controller@11220000 { >> compatible = "mediatek,mt7623-audsys", >> "mediatek,mt2701-audsys", >> "syscon"; >> ... >> >> afe: audio-controller { >> compatible = "mediatek,mt7623-audio", >> "mediatek,mt2701-audio"; >> >> ... >> }; >> }; >> > > I think this approach is not working as, due to backwards compatibility, we have > to make sure that the DRM driver gets probed with the mmsys binding. > > Regards, > Matthias > >> >> Sean >> >> >>> Signed-off-by: Matthias Brugger <mbrugger@suse.com> >>> --- >>> drivers/gpu/drm/mediatek/mtk_drm_drv.c | 18 ++++++++++++++++++ >>> drivers/gpu/drm/mediatek/mtk_drm_drv.h | 2 ++ >>> 2 files changed, 20 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c >>> index dd249cf5121e..c946aea722e5 100644 >>> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c >>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c >>> @@ -173,6 +173,7 @@ static const struct mtk_mmsys_driver_data mt2701_mmsys_driver_data = { >>> .ext_path = mt2701_mtk_ddp_ext, >>> .ext_len = ARRAY_SIZE(mt2701_mtk_ddp_ext), >>> .shadow_register = true, >>> + .clk_drv_name = "clk-mt2701-mm", >>> }; >>> >>> static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = { >>> @@ -180,6 +181,7 @@ static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = { >>> .main_len = ARRAY_SIZE(mt8173_mtk_ddp_main), >>> .ext_path = mt8173_mtk_ddp_ext, >>> .ext_len = ARRAY_SIZE(mt8173_mtk_ddp_ext), >>> + .clk_drv_name = "clk-mt8173-mm", >>> }; >>> >>> static int mtk_drm_kms_init(struct drm_device *drm) >>> @@ -411,6 +413,19 @@ static int mtk_drm_probe(struct platform_device *pdev) >>> if (IS_ERR(private->config_regs)) >>> return PTR_ERR(private->config_regs); >>> >>> + if (private->data->clk_drv_name) { >>> + private->clk_dev = platform_device_register_data(dev, >>> + private->data->clk_drv_name, -1, >>> + NULL, 0); >>> + >>> + if (IS_ERR(private->clk_dev)) { >>> + pr_err("failed to register %s platform device\n", >>> + private->data->clk_drv_name); >>> + >>> + return PTR_ERR(private->clk_dev); >>> + } >>> + } >>> + >>> /* Iterate over sibling DISP function blocks */ >>> for_each_child_of_node(dev->of_node->parent, node) { >>> const struct of_device_id *of_id; >>> @@ -515,6 +530,9 @@ static int mtk_drm_remove(struct platform_device *pdev) >>> for (i = 0; i < DDP_COMPONENT_ID_MAX; i++) >>> of_node_put(private->comp_node[i]); >>> >>> + if (private->clk_dev) >>> + platform_device_unregister(private->clk_dev); >>> + >>> return 0; >>> } >>> >>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h >>> index 86cec19193c4..200eee5de419 100644 >>> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h >>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h >>> @@ -34,11 +34,13 @@ struct mtk_mmsys_driver_data { >>> const enum mtk_ddp_comp_id *ext_path; >>> unsigned int ext_len; >>> bool shadow_register; >>> + const char *clk_drv_name; >>> }; >>> >>> struct mtk_drm_private { >>> struct drm_device *drm; >>> struct device *dma_dev; >>> + struct platform_device *clk_dev; >>> >>> unsigned int num_pipes; >>> >> >>
Hi Sean, On Wednesday, 18 July 2018 05:57:35 EEST Sean Wang wrote: > On Wed, 2018-07-18 at 00:03 +0200, matthias.bgg@kernel.org wrote: > > From: Matthias Brugger <mbrugger@suse.com> > > > > The MMSYS subsystem includes clocks and drm components. > > This patch adds an initailization path through a platform device > > for the clock part, so that both drivers get probed from the same > > device tree compatible. > > Sorry for that I should have a response earlier for the series. > > Some points I felt they're not exactly right and should be fixed up > before we're moving on > > Currently, drm driver have a wrong reference to the dt-binding, > "mediatek,mt2701-mmsys" or "mediatek,mt8173-mmsys", they should be all > for the subsystem exporting clock and reset line such common resource to > its sub-devices. Every subsystem has a similar shape. I hope mmsys > shouldn't be an exception. > > DRM device needs to have its own dt-binding show how connections between > DRM components being made and its node should be put under mmsys node. > > In this way, it becomes easy to see how the topology of the subsystem is > and grows, like a tree "device tree", instead of hiding the details in > the implementation. > > The similar example we already did for audsys on mt2701 and mt7623 as > below > > audsys: clock-controller@11220000 { > compatible = "mediatek,mt7623-audsys", > "mediatek,mt2701-audsys", > "syscon"; > ... > > afe: audio-controller { > compatible = "mediatek,mt7623-audio", > "mediatek,mt2701-audio"; > > ... > }; > }; This looks very strange to me. I'm not familiar with the hardware architecture, but a clock controller that includes an audio controller seems like a very weird design. It's usually the other way around, you have an audio controller, and it also contains hardware that produces clocks, and possibly handles miscellaneous audio-related routing tasks. I would thus expect the reverse in the device tree: afe: audio-controller@11220000 { compatible = "mediatek,mt7623-audio", "mediatek,mt2701-audio"; .. audsys: clock-controller { compatible = "mediatek,mt7623-audsys", "mediatek,mt2701-audsys", "syscon"; ... }; }; And if audsys only exposes clocks, you don't even need a subnode to represent it, the audio controller itself can be a clock provider. afe: audio-controller@11220000 { compatible = "mediatek,mt7623-audio", "mediatek,mt2701-audio"; #clock-cells = <1>; .. }; > > Signed-off-by: Matthias Brugger <mbrugger@suse.com> > > --- > > > > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 18 ++++++++++++++++++ > > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 2 ++ > > 2 files changed, 20 insertions(+) > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c > > b/drivers/gpu/drm/mediatek/mtk_drm_drv.c index dd249cf5121e..c946aea722e5 > > 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c > > @@ -173,6 +173,7 @@ static const struct mtk_mmsys_driver_data > > mt2701_mmsys_driver_data = {> > > .ext_path = mt2701_mtk_ddp_ext, > > .ext_len = ARRAY_SIZE(mt2701_mtk_ddp_ext), > > .shadow_register = true, > > > > + .clk_drv_name = "clk-mt2701-mm", > > > > }; > > > > static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = { > > > > @@ -180,6 +181,7 @@ static const struct mtk_mmsys_driver_data > > mt8173_mmsys_driver_data = {> > > .main_len = ARRAY_SIZE(mt8173_mtk_ddp_main), > > .ext_path = mt8173_mtk_ddp_ext, > > .ext_len = ARRAY_SIZE(mt8173_mtk_ddp_ext), > > > > + .clk_drv_name = "clk-mt8173-mm", > > > > }; > > > > static int mtk_drm_kms_init(struct drm_device *drm) > > > > @@ -411,6 +413,19 @@ static int mtk_drm_probe(struct platform_device > > *pdev) > > > > if (IS_ERR(private->config_regs)) > > > > return PTR_ERR(private->config_regs); > > > > + if (private->data->clk_drv_name) { > > + private->clk_dev = platform_device_register_data(dev, > > + private->data->clk_drv_name, -1, > > + NULL, 0); > > + > > + if (IS_ERR(private->clk_dev)) { > > + pr_err("failed to register %s platform device\n", > > + private->data->clk_drv_name); > > + > > + return PTR_ERR(private->clk_dev); > > + } > > + } > > + > > > > /* Iterate over sibling DISP function blocks */ > > for_each_child_of_node(dev->of_node- >parent, node) { > > > > const struct of_device_id *of_id; > > > > @@ -515,6 +530,9 @@ static int mtk_drm_remove(struct platform_device > > *pdev) > > > > for (i = 0; i < DDP_COMPONENT_ID_MAX; i++) > > > > of_node_put(private->comp_node[i]); > > > > + if (private->clk_dev) > > + platform_device_unregister(private->clk_dev); > > + > > > > return 0; > > > > } > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h > > b/drivers/gpu/drm/mediatek/mtk_drm_drv.h index 86cec19193c4..200eee5de419 > > 100644 > > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h > > @@ -34,11 +34,13 @@ struct mtk_mmsys_driver_data { > > > > const enum mtk_ddp_comp_id *ext_path; > > unsigned int ext_len; > > bool shadow_register; > > > > + const char *clk_drv_name; > > > > }; > > > > struct mtk_drm_private { > > > > struct drm_device *drm; > > struct device *dma_dev; > > > > + struct platform_device *clk_dev; > > > > unsigned int num_pipes;
On Wed, 2018-07-18 at 09:27 +0200, Matthias Brugger wrote: > Hi Sean, > > On 18/07/18 04:57, Sean Wang wrote: > > On Wed, 2018-07-18 at 00:03 +0200, matthias.bgg@kernel.org wrote: > >> From: Matthias Brugger <mbrugger@suse.com> > >> > >> The MMSYS subsystem includes clocks and drm components. > >> This patch adds an initailization path through a platform device > >> for the clock part, so that both drivers get probed from the same > >> device tree compatible. > >> > > > > Sorry for that I should have a response earlier for the series. > > > > Some points I felt they're not exactly right and should be fixed up > > before we're moving on > > > > Currently, drm driver have a wrong reference to the dt-binding, > > "mediatek,mt2701-mmsys" or "mediatek,mt8173-mmsys", they should be all > > for the subsystem exporting clock and reset line such common resource to > > its sub-devices. Every subsystem has a similar shape. I hope mmsys > > shouldn't be an exception. > > I'm not sure if I understand what you mean. You proposed to use a platform > device [1] and now you say we should change the DTS? > Yes, my initial thought is that I would like to embed drm platform device into mmsys. It is the easiest way to solve problems caused by compatible string conflict we met here in short term. > What do you mean by "drm driver has a wrong reference to the dt-binding"? How But for long-term goal, device has to define its own dt-binding before the driver is being implemented. Currently, drm driver uses wrong compatible string, "mediatek,mt2701-mmsys" or "mediatek,mt8173-mmsys", that are owned by mmsys device [1], not by drm device. [1] https://elixir.bootlin.com/linux/v4.18-rc5/source/Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.txt > would you want to name it and most important, what would be your approach to > have backwards compatibility to older device tree bindings? > We can use embed a platform device into mmsys for solving issue caused by the older device tree binding. But for long-term, it's neccessary to have a drm device dt-binding document allowing drm driver and its components driver to follow. > > > > DRM device needs to have its own dt-binding show how connections between > > DRM components being made and its node should be put under mmsys node. > > > In this way, it becomes easy to see how the topology of the subsystem is > > and grows, like a tree "device tree", instead of hiding the details in > > the implementation. > > > > The similar example we already did for audsys on mt2701 and mt7623 as > > below > > > > audsys: clock-controller@11220000 { > > compatible = "mediatek,mt7623-audsys", > > "mediatek,mt2701-audsys", > > "syscon"; > > ... > > > > afe: audio-controller { > > compatible = "mediatek,mt7623-audio", > > "mediatek,mt2701-audio"; > > > > ... > > }; > > }; > > > > I think this approach is not working as, due to backwards compatibility, we have > to make sure that the DRM driver gets probed with the mmsys binding. > > Regards, > Matthias > > > > > Sean > > > > > >> Signed-off-by: Matthias Brugger <mbrugger@suse.com> > >> --- > >> drivers/gpu/drm/mediatek/mtk_drm_drv.c | 18 ++++++++++++++++++ > >> drivers/gpu/drm/mediatek/mtk_drm_drv.h | 2 ++ > >> 2 files changed, 20 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c > >> index dd249cf5121e..c946aea722e5 100644 > >> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c > >> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c > >> @@ -173,6 +173,7 @@ static const struct mtk_mmsys_driver_data mt2701_mmsys_driver_data = { > >> .ext_path = mt2701_mtk_ddp_ext, > >> .ext_len = ARRAY_SIZE(mt2701_mtk_ddp_ext), > >> .shadow_register = true, > >> + .clk_drv_name = "clk-mt2701-mm", > >> }; > >> > >> static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = { > >> @@ -180,6 +181,7 @@ static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = { > >> .main_len = ARRAY_SIZE(mt8173_mtk_ddp_main), > >> .ext_path = mt8173_mtk_ddp_ext, > >> .ext_len = ARRAY_SIZE(mt8173_mtk_ddp_ext), > >> + .clk_drv_name = "clk-mt8173-mm", > >> }; > >> > >> static int mtk_drm_kms_init(struct drm_device *drm) > >> @@ -411,6 +413,19 @@ static int mtk_drm_probe(struct platform_device *pdev) > >> if (IS_ERR(private->config_regs)) > >> return PTR_ERR(private->config_regs); > >> > >> + if (private->data->clk_drv_name) { > >> + private->clk_dev = platform_device_register_data(dev, > >> + private->data->clk_drv_name, -1, > >> + NULL, 0); > >> + > >> + if (IS_ERR(private->clk_dev)) { > >> + pr_err("failed to register %s platform device\n", > >> + private->data->clk_drv_name); > >> + > >> + return PTR_ERR(private->clk_dev); > >> + } > >> + } > >> + > >> /* Iterate over sibling DISP function blocks */ > >> for_each_child_of_node(dev->of_node->parent, node) { > >> const struct of_device_id *of_id; > >> @@ -515,6 +530,9 @@ static int mtk_drm_remove(struct platform_device *pdev) > >> for (i = 0; i < DDP_COMPONENT_ID_MAX; i++) > >> of_node_put(private->comp_node[i]); > >> > >> + if (private->clk_dev) > >> + platform_device_unregister(private->clk_dev); > >> + > >> return 0; > >> } > >> > >> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h > >> index 86cec19193c4..200eee5de419 100644 > >> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h > >> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h > >> @@ -34,11 +34,13 @@ struct mtk_mmsys_driver_data { > >> const enum mtk_ddp_comp_id *ext_path; > >> unsigned int ext_len; > >> bool shadow_register; > >> + const char *clk_drv_name; > >> }; > >> > >> struct mtk_drm_private { > >> struct drm_device *drm; > >> struct device *dma_dev; > >> + struct platform_device *clk_dev; > >> > >> unsigned int num_pipes; > >> > > > >
On Wed, 2018-07-18 at 11:05 +0300, Laurent Pinchart wrote: > Hi Sean, > > On Wednesday, 18 July 2018 05:57:35 EEST Sean Wang wrote: > > On Wed, 2018-07-18 at 00:03 +0200, matthias.bgg@kernel.org wrote: > > > From: Matthias Brugger <mbrugger@suse.com> > > > > > > The MMSYS subsystem includes clocks and drm components. > > > This patch adds an initailization path through a platform device > > > for the clock part, so that both drivers get probed from the same > > > device tree compatible. > > > > Sorry for that I should have a response earlier for the series. > > > > Some points I felt they're not exactly right and should be fixed up > > before we're moving on > > > > Currently, drm driver have a wrong reference to the dt-binding, > > "mediatek,mt2701-mmsys" or "mediatek,mt8173-mmsys", they should be all > > for the subsystem exporting clock and reset line such common resource to > > its sub-devices. Every subsystem has a similar shape. I hope mmsys > > shouldn't be an exception. > > > > DRM device needs to have its own dt-binding show how connections between > > DRM components being made and its node should be put under mmsys node. > > > > In this way, it becomes easy to see how the topology of the subsystem is > > and grows, like a tree "device tree", instead of hiding the details in > > the implementation. > > > > The similar example we already did for audsys on mt2701 and mt7623 as > > below > > > > audsys: clock-controller@11220000 { > > > compatible = "mediatek,mt7623-audsys", > > > "mediatek,mt2701-audsys", > > > "syscon"; > > > ... > > > > > afe: audio-controller { > > > compatible = "mediatek,mt7623-audio", > > > > "mediatek,mt2701-audio"; > > > > > ... > > > }; > > }; > > This looks very strange to me. I'm not familiar with the hardware > architecture, but a clock controller that includes an audio controller seems > like a very weird design. It's usually the other way around, you have an audio yes, naming audsys as clock controller is really not good. it should be worth a better naming. mtk subsystem AFAIK works as a container, at least provides clocks, reset, syscon access, these common resource to these devices running on the subsystem. It also has a power gate independent from other subsystem, that can be controlled when these devices all powered down or once up. And it should be better that we don't assume what exact devices are running on since it is possible that there're different devices running on the same subsystem per SoC. mtk has many subsystem working in this way. mmsys is just a case. we can see in [1] [1] https://elixir.bootlin.com/linux/v4.18-rc5/source/Documentation/devicetree/bindings/arm/mediatek > controller, and it also contains hardware that produces clocks, and possibly > handles miscellaneous audio-related routing tasks. I would thus expect the > reverse in the device tree: > > afe: audio-controller@11220000 { > > compatible = "mediatek,mt7623-audio", > > "mediatek,mt2701-audio"; > > .. > > > audsys: clock-controller { > > compatible = "mediatek,mt7623-audsys", > > > "mediatek,mt2701-audsys", > > > "syscon"; > > ... > > }; > }; > > And if audsys only exposes clocks, you don't even need a subnode to represent > it, the audio controller itself can be a clock provider. > > afe: audio-controller@11220000 { > > compatible = "mediatek,mt7623-audio", > > "mediatek,mt2701-audio"; > > #clock-cells = <1>; > > .. > }; > > > > Signed-off-by: Matthias Brugger <mbrugger@suse.com> > > > --- > > > > > > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 18 ++++++++++++++++++ > > > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 2 ++ > > > 2 files changed, 20 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c > > > b/drivers/gpu/drm/mediatek/mtk_drm_drv.c index dd249cf5121e..c946aea722e5 > > > 100644 > > > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c > > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c > > > @@ -173,6 +173,7 @@ static const struct mtk_mmsys_driver_data > > > mt2701_mmsys_driver_data = {> > > > .ext_path = mt2701_mtk_ddp_ext, > > > .ext_len = > ARRAY_SIZE(mt2701_mtk_ddp_ext), > > > .shadow_register = true, > > > > > > + .clk_drv_name = "clk-mt2701-mm", > > > > > > }; > > > > > > static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = { > > > > > > @@ -180,6 +181,7 @@ static const struct mtk_mmsys_driver_data > > > mt8173_mmsys_driver_data = {> > > > .main_len = > ARRAY_SIZE(mt8173_mtk_ddp_main), > > > .ext_path = mt8173_mtk_ddp_ext, > > > .ext_len = > ARRAY_SIZE(mt8173_mtk_ddp_ext), > > > > > > + .clk_drv_name = "clk-mt8173-mm", > > > > > > }; > > > > > > static int mtk_drm_kms_init(struct drm_device *drm) > > > > > > @@ -411,6 +413,19 @@ static int mtk_drm_probe(struct platform_device > > > *pdev) > > > > > > if (IS_ERR(private->config_regs)) > > > > > > > return PTR_ERR(private->config_regs); > > > > > > + if (private->data->clk_drv_name) { > > > + > private->clk_dev = platform_device_register_data(dev, > > > + > > > private->data->clk_drv_name, -1, > > > + > > > NULL, 0); > > > + > > > + > if (IS_ERR(private->clk_dev)) { > > > + > pr_err("failed to register %s platform > device\n", > > > + > > > private->data->clk_drv_name); > > > + > > > + > return PTR_ERR(private->clk_dev); > > > + > } > > > + } > > > + > > > > > > /* Iterate over sibling DISP > function blocks */ > > > for_each_child_of_node(dev->of_node- > >parent, node) { > > > > > > > const struct of_device_id *of_id; > > > > > > @@ -515,6 +530,9 @@ static int mtk_drm_remove(struct platform_device > > > *pdev) > > > > > > for (i = 0; i < > DDP_COMPONENT_ID_MAX; i++) > > > > > > > of_node_put(private->comp_node[i]); > > > > > > + if (private->clk_dev) > > > + > platform_device_unregister(private->clk_dev); > > > + > > > > > > return 0; > > > > > > } > > > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h > > > b/drivers/gpu/drm/mediatek/mtk_drm_drv.h index 86cec19193c4..200eee5de419 > > > 100644 > > > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h > > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h > > > @@ -34,11 +34,13 @@ struct mtk_mmsys_driver_data { > > > > > > const enum mtk_ddp_comp_id > *ext_path; > > > unsigned int ext_len; > > > bool shadow_register; > > > > > > + const char *clk_drv_name; > > > > > > }; > > > > > > struct mtk_drm_private { > > > > > > struct drm_device *drm; > > > struct device *dma_dev; > > > > > > + struct platform_device *clk_dev; > > > > > > unsigned int num_pipes; > >
Quoting Sean Wang (2018-07-18 03:06:27) > On Wed, 2018-07-18 at 11:05 +0300, Laurent Pinchart wrote: > > Hi Sean, > > > > On Wednesday, 18 July 2018 05:57:35 EEST Sean Wang wrote: > > > On Wed, 2018-07-18 at 00:03 +0200, matthias.bgg@kernel.org wrote: > > > > From: Matthias Brugger <mbrugger@suse.com> > > > > > > > > The MMSYS subsystem includes clocks and drm components. > > > > This patch adds an initailization path through a platform device > > > > for the clock part, so that both drivers get probed from the same > > > > device tree compatible. > > > > > > Sorry for that I should have a response earlier for the series. > > > > > > Some points I felt they're not exactly right and should be fixed up > > > before we're moving on > > > > > > Currently, drm driver have a wrong reference to the dt-binding, > > > "mediatek,mt2701-mmsys" or "mediatek,mt8173-mmsys", they should be all > > > for the subsystem exporting clock and reset line such common resource to > > > its sub-devices. Every subsystem has a similar shape. I hope mmsys > > > shouldn't be an exception. > > > > > > DRM device needs to have its own dt-binding show how connections between > > > DRM components being made and its node should be put under mmsys node. > > > > > > In this way, it becomes easy to see how the topology of the subsystem is > > > and grows, like a tree "device tree", instead of hiding the details in > > > the implementation. > > > > > > The similar example we already did for audsys on mt2701 and mt7623 as > > > below > > > > > > audsys: clock-controller@11220000 { > > > > > compatible = "mediatek,mt7623-audsys", > > > > > "mediatek,mt2701-audsys", > > > > > "syscon"; > > > > > ... > > > > > > > > afe: audio-controller { > > > > > compatible = "mediatek,mt7623-audio", > > > > > > > "mediatek,mt2701-audio"; > > > > > > > > ... > > > > > }; > > > }; > > > > This looks very strange to me. I'm not familiar with the hardware > > architecture, but a clock controller that includes an audio controller seems > > like a very weird design. It's usually the other way around, you have an audio > > yes, naming audsys as clock controller is really not good. it should be > worth a better naming. > > mtk subsystem AFAIK works as a container, at least provides clocks, > reset, syscon access, these common resource to these devices running on > the subsystem. It also has a power gate independent from other > subsystem, that can be controlled when these devices all powered down > or once up. > > And it should be better that we don't assume what exact devices are > running on since it is possible that there're different devices running > on the same subsystem per SoC. mtk has many subsystem working in this > way. mmsys is just a case. we can see in [1] > > > [1] > https://elixir.bootlin.com/linux/v4.18-rc5/source/Documentation/devicetree/bindings/arm/mediatek > Sean, is this an ack for this patch series? I think the consensus is to not modify DT to add a subnode for the "clk" part of the hardware, but instead either register the clks from the device driver that has the clk hardware inside of it, or do what this patch series does and register a device in software from the "parent" device driver so that logically clk things are contained in drivers/clk/
Quoting Stephen Boyd (2018-07-26 00:35:14) > Quoting Sean Wang (2018-07-18 03:06:27) > > On Wed, 2018-07-18 at 11:05 +0300, Laurent Pinchart wrote: > > > Hi Sean, > > > > > > This looks very strange to me. I'm not familiar with the hardware > > > architecture, but a clock controller that includes an audio controller seems > > > like a very weird design. It's usually the other way around, you have an audio > > > > yes, naming audsys as clock controller is really not good. it should be > > worth a better naming. > > > > mtk subsystem AFAIK works as a container, at least provides clocks, > > reset, syscon access, these common resource to these devices running on > > the subsystem. It also has a power gate independent from other > > subsystem, that can be controlled when these devices all powered down > > or once up. > > > > And it should be better that we don't assume what exact devices are > > running on since it is possible that there're different devices running > > on the same subsystem per SoC. mtk has many subsystem working in this > > way. mmsys is just a case. we can see in [1] > > > > > > [1] > > https://elixir.bootlin.com/linux/v4.18-rc5/source/Documentation/devicetree/bindings/arm/mediatek > > > > Sean, is this an ack for this patch series? I think the consensus is to > not modify DT to add a subnode for the "clk" part of the hardware, but > instead either register the clks from the device driver that has the clk > hardware inside of it, or do what this patch series does and register a > device in software from the "parent" device driver so that logically clk > things are contained in drivers/clk/ > Please resend this series. This is too old now and I'm dropping this from (the very tail) of my queue.
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c index dd249cf5121e..c946aea722e5 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c @@ -173,6 +173,7 @@ static const struct mtk_mmsys_driver_data mt2701_mmsys_driver_data = { .ext_path = mt2701_mtk_ddp_ext, .ext_len = ARRAY_SIZE(mt2701_mtk_ddp_ext), .shadow_register = true, + .clk_drv_name = "clk-mt2701-mm", }; static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = { @@ -180,6 +181,7 @@ static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = { .main_len = ARRAY_SIZE(mt8173_mtk_ddp_main), .ext_path = mt8173_mtk_ddp_ext, .ext_len = ARRAY_SIZE(mt8173_mtk_ddp_ext), + .clk_drv_name = "clk-mt8173-mm", }; static int mtk_drm_kms_init(struct drm_device *drm) @@ -411,6 +413,19 @@ static int mtk_drm_probe(struct platform_device *pdev) if (IS_ERR(private->config_regs)) return PTR_ERR(private->config_regs); + if (private->data->clk_drv_name) { + private->clk_dev = platform_device_register_data(dev, + private->data->clk_drv_name, -1, + NULL, 0); + + if (IS_ERR(private->clk_dev)) { + pr_err("failed to register %s platform device\n", + private->data->clk_drv_name); + + return PTR_ERR(private->clk_dev); + } + } + /* Iterate over sibling DISP function blocks */ for_each_child_of_node(dev->of_node->parent, node) { const struct of_device_id *of_id; @@ -515,6 +530,9 @@ static int mtk_drm_remove(struct platform_device *pdev) for (i = 0; i < DDP_COMPONENT_ID_MAX; i++) of_node_put(private->comp_node[i]); + if (private->clk_dev) + platform_device_unregister(private->clk_dev); + return 0; } diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h index 86cec19193c4..200eee5de419 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h @@ -34,11 +34,13 @@ struct mtk_mmsys_driver_data { const enum mtk_ddp_comp_id *ext_path; unsigned int ext_len; bool shadow_register; + const char *clk_drv_name; }; struct mtk_drm_private { struct drm_device *drm; struct device *dma_dev; + struct platform_device *clk_dev; unsigned int num_pipes;