mbox series

[v8,00/34] NVIDIA Tegra power management patches for 5.16

Message ID 20210817012754.8710-1-digetx@gmail.com
Headers show
Series NVIDIA Tegra power management patches for 5.16 | expand

Message

Dmitry Osipenko Aug. 17, 2021, 1:27 a.m. UTC
This series adds runtime PM support to Tegra drivers and enables core
voltage scaling for Tegra20/30 SoCs, resolving overheating troubles.

All patches should go via Tegra tree because they are interdependent,
please review and ack.

If you haven't seen this series before, that's because I wanted to
finalize the GENPD part at first and didn't bother you previously.

Changelog:

v8: - Added new generic dev_pm_opp_sync() helper that syncs OPP state with
      hardware. All drivers changed to use it. This replaces GENPD attach_dev
      callback hacks that were used in v7.

    - Added new patch patch "soc/tegra: regulators: Prepare for suspend"
      that fixes dying Tegra20 SoC after enabling VENC power domain during
      resume from suspend. It matches to what downstream kernel does on
      suspend/resume.

    - After a second thought, I dropped patches which added RPM to memory
      drivers since hardware is always-on and RPM not needed.

    - Replaced the "dummy host1x driver" patch with new "Disable unused
      host1x hardware" patch, since it's a cleaner solution.

Dmitry Osipenko (34):
  opp: Add dev_pm_opp_sync() helper
  soc/tegra: pmc: Disable PMC state syncing
  soc/tegra: Don't print error message when OPPs not available
  soc/tegra: Add devm_tegra_core_dev_init_opp_table_simple()
  soc/tegra: Use dev_pm_opp_sync()
  dt-bindings: clock: tegra-car: Document new tegra-clocks sub-node
  clk: tegra: Support runtime PM and power domain
  dt-bindings: host1x: Document OPP and power domain properties
  dt-bindings: host1x: Document Memory Client resets of Host1x, GR2D and
    GR3D
  gpu: host1x: Add host1x_channel_stop()
  gpu: host1x: Add runtime PM and OPP support
  drm/tegra: dc: Support OPP and SoC core voltage scaling
  drm/tegra: hdmi: Add OPP support
  drm/tegra: gr2d: Support power management
  drm/tegra: gr3d: Support power management
  drm/tegra: vic: Support system suspend
  usb: chipidea: tegra: Add runtime PM and OPP support
  bus: tegra-gmi: Add runtime PM and OPP support
  pwm: tegra: Add runtime PM and OPP support
  mmc: sdhci-tegra: Add runtime PM and OPP support
  mtd: rawnand: tegra: Add runtime PM and OPP support
  spi: tegra20-slink: Add OPP support
  media: dt: bindings: tegra-vde: Convert to schema
  media: dt: bindings: tegra-vde: Document OPP and power domain
  media: staging: tegra-vde: Support generic power domain and OPP
  soc/tegra: fuse: Add OPP support
  soc/tegra: fuse: Reset hardware
  soc/tegra: regulators: Prepare for suspend
  soc/tegra: pmc: Enable core domain support for Tegra20 and Tegra30
  ARM: tegra: Add OPP tables and power domains to Tegra20 device-trees
  ARM: tegra: Add OPP tables and power domains to Tegra30 device-trees
  ARM: tegra: Add Memory Client resets to Tegra20 GR2D, GR3D and Host1x
  ARM: tegra: Add Memory Client resets to Tegra30 GR2D, GR3D and Host1x
  ARM: tegra20/30: Disable unused host1x hardware

 .../bindings/clock/nvidia,tegra20-car.yaml    |   51 +
 .../display/tegra/nvidia,tegra20-host1x.txt   |   53 +
 .../bindings/media/nvidia,tegra-vde.txt       |   64 -
 .../bindings/media/nvidia,tegra-vde.yaml      |  119 ++
 .../boot/dts/tegra20-acer-a500-picasso.dts    |    1 +
 arch/arm/boot/dts/tegra20-colibri.dtsi        |    3 +-
 arch/arm/boot/dts/tegra20-harmony.dts         |    3 +-
 arch/arm/boot/dts/tegra20-paz00.dts           |    1 +
 .../arm/boot/dts/tegra20-peripherals-opp.dtsi |  941 +++++++++++
 arch/arm/boot/dts/tegra20-seaboard.dts        |    3 +-
 arch/arm/boot/dts/tegra20-tamonten.dtsi       |    3 +-
 arch/arm/boot/dts/tegra20-trimslice.dts       |    9 +
 arch/arm/boot/dts/tegra20-ventana.dts         |    1 +
 arch/arm/boot/dts/tegra20.dtsi                |  119 +-
 .../tegra30-asus-nexus7-grouper-common.dtsi   |    1 +
 arch/arm/boot/dts/tegra30-beaver.dts          |    1 +
 arch/arm/boot/dts/tegra30-cardhu.dtsi         |    1 +
 arch/arm/boot/dts/tegra30-colibri.dtsi        |   17 +-
 arch/arm/boot/dts/tegra30-ouya.dts            |    1 +
 .../arm/boot/dts/tegra30-peripherals-opp.dtsi | 1412 +++++++++++++++++
 arch/arm/boot/dts/tegra30.dtsi                |  181 ++-
 drivers/bus/tegra-gmi.c                       |   92 +-
 drivers/clk/tegra/Makefile                    |    1 +
 drivers/clk/tegra/clk-device.c                |  222 +++
 drivers/clk/tegra/clk-pll.c                   |    2 +-
 drivers/clk/tegra/clk-super.c                 |    2 +-
 drivers/clk/tegra/clk-tegra20.c               |   39 +-
 drivers/clk/tegra/clk-tegra30.c               |   70 +-
 drivers/clk/tegra/clk.c                       |   64 +
 drivers/clk/tegra/clk.h                       |    2 +
 drivers/gpu/drm/tegra/dc.c                    |   74 +
 drivers/gpu/drm/tegra/dc.h                    |    2 +
 drivers/gpu/drm/tegra/gr2d.c                  |  154 +-
 drivers/gpu/drm/tegra/gr3d.c                  |  393 ++++-
 drivers/gpu/drm/tegra/hdmi.c                  |   15 +-
 drivers/gpu/drm/tegra/vic.c                   |    4 +
 drivers/gpu/host1x/channel.c                  |    8 +
 drivers/gpu/host1x/debug.c                    |   15 +
 drivers/gpu/host1x/dev.c                      |  157 +-
 drivers/gpu/host1x/dev.h                      |    3 +-
 drivers/gpu/host1x/hw/channel_hw.c            |   44 +-
 drivers/gpu/host1x/intr.c                     |    3 -
 drivers/gpu/host1x/syncpt.c                   |    5 +-
 drivers/mmc/host/sdhci-tegra.c                |  146 +-
 drivers/mtd/nand/raw/tegra_nand.c             |   62 +-
 drivers/opp/core.c                            |   42 +-
 drivers/pwm/pwm-tegra.c                       |  104 +-
 drivers/soc/tegra/common.c                    |   34 +-
 drivers/soc/tegra/fuse/fuse-tegra.c           |   36 +
 drivers/soc/tegra/fuse/fuse.h                 |    1 +
 drivers/soc/tegra/pmc.c                       |   17 +
 drivers/soc/tegra/regulators-tegra20.c        |   99 ++
 drivers/soc/tegra/regulators-tegra30.c        |  122 ++
 drivers/spi/spi-tegra20-slink.c               |   15 +-
 drivers/staging/media/tegra-vde/vde.c         |   65 +-
 drivers/usb/chipidea/ci_hdrc_tegra.c          |   61 +-
 include/linux/host1x.h                        |    1 +
 include/linux/pm_opp.h                        |    6 +
 include/soc/tegra/common.h                    |   13 +
 59 files changed, 4796 insertions(+), 384 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/media/nvidia,tegra-vde.txt
 create mode 100644 Documentation/devicetree/bindings/media/nvidia,tegra-vde.yaml
 create mode 100644 drivers/clk/tegra/clk-device.c

Comments

Ulf Hansson Aug. 17, 2021, 12:04 p.m. UTC | #1
On Tue, 17 Aug 2021 at 03:30, Dmitry Osipenko <digetx@gmail.com> wrote:
>
> Add runtime PM and OPP support to the Host1x driver. It's required for
> enabling system-wide DVFS and supporting dynamic power management using
> a generic power domain. For the starter we will keep host1x always-on
> because dynamic power management require a major refactoring of the driver
> code since lot's of code paths will need the RPM handling and we're going
> to remove some of these paths in the future. Host1x doesn't consume much
> power so it is good enough, we at least need to resume Host1x in order
> to initialize the power state.
>
> Tested-by: Peter Geis <pgwipeout@gmail.com> # Ouya T30
> Tested-by: Paul Fertser <fercerpav@gmail.com> # PAZ00 T20
> Tested-by: Nicolas Chauvet <kwizart@gmail.com> # PAZ00 T20 and TK1 T124
> Tested-by: Matt Merhar <mattmerhar@protonmail.com> # Ouya T30
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---

[...]

> +
>  static int host1x_probe(struct platform_device *pdev)
>  {
>         struct host1x *host;
> @@ -394,6 +423,10 @@ static int host1x_probe(struct platform_device *pdev)
>         /* set common host1x device data */
>         platform_set_drvdata(pdev, host);
>
> +       err = devm_tegra_core_dev_init_opp_table_simple(&pdev->dev);
> +       if (err)
> +               return err;
> +
>         host->regs = devm_ioremap_resource(&pdev->dev, regs);
>         if (IS_ERR(host->regs))
>                 return PTR_ERR(host->regs);
> @@ -423,12 +456,9 @@ static int host1x_probe(struct platform_device *pdev)
>                 return err;
>         }
>
> -       host->rst = devm_reset_control_get(&pdev->dev, "host1x");
> -       if (IS_ERR(host->rst)) {
> -               err = PTR_ERR(host->rst);
> -               dev_err(&pdev->dev, "failed to get reset: %d\n", err);
> +       err = host1x_get_resets(host);
> +       if (err)
>                 return err;
> -       }
>
>         err = host1x_iommu_init(host);
>         if (err < 0) {
> @@ -443,22 +473,10 @@ static int host1x_probe(struct platform_device *pdev)
>                 goto iommu_exit;
>         }
>
> -       err = clk_prepare_enable(host->clk);
> -       if (err < 0) {
> -               dev_err(&pdev->dev, "failed to enable clock\n");
> -               goto free_channels;
> -       }
> -
> -       err = reset_control_deassert(host->rst);
> -       if (err < 0) {
> -               dev_err(&pdev->dev, "failed to deassert reset: %d\n", err);
> -               goto unprepare_disable;
> -       }
> -

Removing the clk_prepare_enable() and reset_control_deassert() from
host1x_probe(), might not be a good idea. See more about why, below.

>         err = host1x_syncpt_init(host);
>         if (err) {
>                 dev_err(&pdev->dev, "failed to initialize syncpts\n");
> -               goto reset_assert;
> +               goto free_channels;
>         }
>
>         err = host1x_intr_init(host, syncpt_irq);
> @@ -467,10 +485,14 @@ static int host1x_probe(struct platform_device *pdev)
>                 goto deinit_syncpt;
>         }
>
> -       host1x_debug_init(host);
> +       pm_runtime_enable(&pdev->dev);
>
> -       if (host->info->has_hypervisor)
> -               host1x_setup_sid_table(host);
> +       /* the driver's code isn't ready yet for the dynamic RPM */
> +       err = pm_runtime_resume_and_get(&pdev->dev);

If the driver is being built with the CONFIG_PM Kconfig option being
unset, pm_runtime_resume_and_get() will return 0 to indicate success -
and without calling the ->runtime_resume() callback.
In other words, the clock will remain gated and the reset will not be
deasserted, likely causing the driver to be malfunctioning.

If the driver isn't ever being built with CONFIG_PM unset, feel free
to ignore my above comments.

Otherwise, if it needs to work both with and without CONFIG_PM being
set, you may use the following pattern in host1x_probe() to deploy
runtime PM support:

"Enable the needed resources to probe the device"
pm_runtime_get_noresume()
pm_runtime_set_active()
pm_runtime_enable()

"Before successfully completing probe"
pm_runtime_put()

> +       if (err)
> +               goto deinit_intr;
> +
> +       host1x_debug_init(host);
>
>         err = host1x_register(host);
>         if (err < 0)
> @@ -486,13 +508,13 @@ static int host1x_probe(struct platform_device *pdev)
>         host1x_unregister(host);
>  deinit_debugfs:
>         host1x_debug_deinit(host);
> +
> +       pm_runtime_put(&pdev->dev);
> +       pm_runtime_disable(&pdev->dev);
> +deinit_intr:
>         host1x_intr_deinit(host);
>  deinit_syncpt:
>         host1x_syncpt_deinit(host);
> -reset_assert:
> -       reset_control_assert(host->rst);
> -unprepare_disable:
> -       clk_disable_unprepare(host->clk);
>  free_channels:
>         host1x_channel_list_free(&host->channel_list);
>  iommu_exit:

[...]

Kind regards
Uffe
Thierry Reding Aug. 17, 2021, 2:02 p.m. UTC | #2
On Tue, Aug 17, 2021 at 02:04:38PM +0200, Ulf Hansson wrote:
> On Tue, 17 Aug 2021 at 03:30, Dmitry Osipenko <digetx@gmail.com> wrote:
> >
> > Add runtime PM and OPP support to the Host1x driver. It's required for
> > enabling system-wide DVFS and supporting dynamic power management using
> > a generic power domain. For the starter we will keep host1x always-on
> > because dynamic power management require a major refactoring of the driver
> > code since lot's of code paths will need the RPM handling and we're going
> > to remove some of these paths in the future. Host1x doesn't consume much
> > power so it is good enough, we at least need to resume Host1x in order
> > to initialize the power state.
> >
> > Tested-by: Peter Geis <pgwipeout@gmail.com> # Ouya T30
> > Tested-by: Paul Fertser <fercerpav@gmail.com> # PAZ00 T20
> > Tested-by: Nicolas Chauvet <kwizart@gmail.com> # PAZ00 T20 and TK1 T124
> > Tested-by: Matt Merhar <mattmerhar@protonmail.com> # Ouya T30
> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> > ---
> 
> [...]
> 
> > +
> >  static int host1x_probe(struct platform_device *pdev)
> >  {
> >         struct host1x *host;
> > @@ -394,6 +423,10 @@ static int host1x_probe(struct platform_device *pdev)
> >         /* set common host1x device data */
> >         platform_set_drvdata(pdev, host);
> >
> > +       err = devm_tegra_core_dev_init_opp_table_simple(&pdev->dev);
> > +       if (err)
> > +               return err;
> > +
> >         host->regs = devm_ioremap_resource(&pdev->dev, regs);
> >         if (IS_ERR(host->regs))
> >                 return PTR_ERR(host->regs);
> > @@ -423,12 +456,9 @@ static int host1x_probe(struct platform_device *pdev)
> >                 return err;
> >         }
> >
> > -       host->rst = devm_reset_control_get(&pdev->dev, "host1x");
> > -       if (IS_ERR(host->rst)) {
> > -               err = PTR_ERR(host->rst);
> > -               dev_err(&pdev->dev, "failed to get reset: %d\n", err);
> > +       err = host1x_get_resets(host);
> > +       if (err)
> >                 return err;
> > -       }
> >
> >         err = host1x_iommu_init(host);
> >         if (err < 0) {
> > @@ -443,22 +473,10 @@ static int host1x_probe(struct platform_device *pdev)
> >                 goto iommu_exit;
> >         }
> >
> > -       err = clk_prepare_enable(host->clk);
> > -       if (err < 0) {
> > -               dev_err(&pdev->dev, "failed to enable clock\n");
> > -               goto free_channels;
> > -       }
> > -
> > -       err = reset_control_deassert(host->rst);
> > -       if (err < 0) {
> > -               dev_err(&pdev->dev, "failed to deassert reset: %d\n", err);
> > -               goto unprepare_disable;
> > -       }
> > -
> 
> Removing the clk_prepare_enable() and reset_control_deassert() from
> host1x_probe(), might not be a good idea. See more about why, below.
> 
> >         err = host1x_syncpt_init(host);
> >         if (err) {
> >                 dev_err(&pdev->dev, "failed to initialize syncpts\n");
> > -               goto reset_assert;
> > +               goto free_channels;
> >         }
> >
> >         err = host1x_intr_init(host, syncpt_irq);
> > @@ -467,10 +485,14 @@ static int host1x_probe(struct platform_device *pdev)
> >                 goto deinit_syncpt;
> >         }
> >
> > -       host1x_debug_init(host);
> > +       pm_runtime_enable(&pdev->dev);
> >
> > -       if (host->info->has_hypervisor)
> > -               host1x_setup_sid_table(host);
> > +       /* the driver's code isn't ready yet for the dynamic RPM */
> > +       err = pm_runtime_resume_and_get(&pdev->dev);
> 
> If the driver is being built with the CONFIG_PM Kconfig option being
> unset, pm_runtime_resume_and_get() will return 0 to indicate success -
> and without calling the ->runtime_resume() callback.
> In other words, the clock will remain gated and the reset will not be
> deasserted, likely causing the driver to be malfunctioning.
> 
> If the driver isn't ever being built with CONFIG_PM unset, feel free
> to ignore my above comments.
> 
> Otherwise, if it needs to work both with and without CONFIG_PM being
> set, you may use the following pattern in host1x_probe() to deploy
> runtime PM support:
> 
> "Enable the needed resources to probe the device"
> pm_runtime_get_noresume()
> pm_runtime_set_active()
> pm_runtime_enable()
> 
> "Before successfully completing probe"
> pm_runtime_put()

We made a conscious decision a few years ago to have ARCH_TEGRA select
PM on both 32-bit and 64-bit ARM, specifically to avoid the need to do
this dance (though there are still a few drivers left that do this, I
think).

So I think this should be unnecessary. Unless perhaps if the sysfs PM
controls have any influence on this. As far as I know, as long as the
PM kconfig option is enabled, the sysfs control only influence the
runtime behaviour (i.e. setting the sysfs PM control to "on" is going
to force runtime PM to be resumed) but there's no way to disable
runtime PM altogether via sysfs that would make the above necessary.

Thierry
Rob Herring (Arm) Aug. 18, 2021, 1:15 a.m. UTC | #3
On Tue, Aug 17, 2021 at 04:27:26AM +0300, Dmitry Osipenko wrote:
> Document tegra-clocks sub-node which describes Tegra SoC clocks that
> require a higher voltage of the core power domain in order to operate
> properly on a higher clock rates.  Each node contains a phandle to OPP
> table and power domain.
> 
> The root PLLs and system clocks don't have any specific device dedicated
> to them, clock controller is in charge of managing power for them.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  .../bindings/clock/nvidia,tegra20-car.yaml    | 51 +++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml
> index 459d2a525393..7f5cd27e4ce0 100644
> --- a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml
> +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml
> @@ -42,6 +42,48 @@ properties:
>    "#reset-cells":
>      const: 1
>  
> +  tegra-clocks:
> +    description: child nodes are the output clocks from the CAR
> +    type: object
> +
> +    patternProperties:
> +      "^[a-z]+[0-9]+$":
> +        type: object
> +        properties:
> +          compatible:
> +            allOf:
> +              - items:
> +                  - enum:
> +                      - nvidia,tegra20-sclk
> +                      - nvidia,tegra30-sclk
> +                      - nvidia,tegra30-pllc
> +                      - nvidia,tegra30-plle
> +                      - nvidia,tegra30-pllm
> +              - const: nvidia,tegra-clock

You are saying the first string must be both one of the enums and 
'nvidia,tegra-clock'. You don't get an error because your pattern 
doesn't match 'sclk'.

> +
> +          operating-points-v2:
> +            $ref: /schemas/types.yaml#/definitions/phandle
> +            description:
> +              Phandle to OPP table that contains frequencies, voltages and
> +              opp-supported-hw property, which is a bitfield indicating
> +              SoC process or speedo ID mask.

Just 'operating-points-v2: true' is enough.

> +
> +          clocks:
> +            items:
> +              - description: node's clock
> +
> +          power-domains:
> +            maxItems: 1
> +            description: phandle to the core SoC power domain
> +
> +        required:
> +          - compatible
> +          - operating-points-v2
> +          - clocks
> +          - power-domains
> +
> +        additionalProperties: false
> +
>  required:
>    - compatible
>    - reg
> @@ -59,6 +101,15 @@ examples:
>          reg = <0x60006000 0x1000>;
>          #clock-cells = <1>;
>          #reset-cells = <1>;
> +
> +        tegra-clocks {
> +            sclk {
> +                compatible = "nvidia,tegra20-sclk", "nvidia,tegra-clock";
> +                operating-points-v2 = <&opp_table>;
> +                clocks = <&tegra_car TEGRA20_CLK_SCLK>;
> +                power-domains = <&domain>;
> +            };
> +        };
>      };
>  
>      usb-controller@c5004000 {
> -- 
> 2.32.0
> 
>
Rob Herring (Arm) Aug. 18, 2021, 1:17 a.m. UTC | #4
On Tue, 17 Aug 2021 04:27:43 +0300, Dmitry Osipenko wrote:
> Convert NVIDIA Tegra video decoder binding to schema.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  .../bindings/media/nvidia,tegra-vde.txt       |  64 -----------
>  .../bindings/media/nvidia,tegra-vde.yaml      | 107 ++++++++++++++++++
>  2 files changed, 107 insertions(+), 64 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/media/nvidia,tegra-vde.txt
>  create mode 100644 Documentation/devicetree/bindings/media/nvidia,tegra-vde.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>
Dmitry Osipenko Aug. 18, 2021, 1:44 a.m. UTC | #5
18.08.2021 04:15, Rob Herring пишет:
>> +  tegra-clocks:
>> +    description: child nodes are the output clocks from the CAR
>> +    type: object
>> +
>> +    patternProperties:
>> +      "^[a-z]+[0-9]+$":
>> +        type: object
>> +        properties:
>> +          compatible:
>> +            allOf:
>> +              - items:
>> +                  - enum:
>> +                      - nvidia,tegra20-sclk
>> +                      - nvidia,tegra30-sclk
>> +                      - nvidia,tegra30-pllc
>> +                      - nvidia,tegra30-plle
>> +                      - nvidia,tegra30-pllm
>> +              - const: nvidia,tegra-clock
> You are saying the first string must be both one of the enums and 
> 'nvidia,tegra-clock'. You don't get an error because your pattern 
> doesn't match 'sclk'.
> 

Could you please rephrase or clarify? If pattern doesn't match 'sclk',
then it must match any other enum. I'm not sure what you're meaning.

The 'nvidia,tegra-clock' actually could be removed since it's
superfluous now. I'll consider the removal in v9.
Ulf Hansson Aug. 18, 2021, 8:35 a.m. UTC | #6
On Tue, 17 Aug 2021 at 16:03, Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Tue, Aug 17, 2021 at 02:04:38PM +0200, Ulf Hansson wrote:
> > On Tue, 17 Aug 2021 at 03:30, Dmitry Osipenko <digetx@gmail.com> wrote:
> > >
> > > Add runtime PM and OPP support to the Host1x driver. It's required for
> > > enabling system-wide DVFS and supporting dynamic power management using
> > > a generic power domain. For the starter we will keep host1x always-on
> > > because dynamic power management require a major refactoring of the driver
> > > code since lot's of code paths will need the RPM handling and we're going
> > > to remove some of these paths in the future. Host1x doesn't consume much
> > > power so it is good enough, we at least need to resume Host1x in order
> > > to initialize the power state.
> > >
> > > Tested-by: Peter Geis <pgwipeout@gmail.com> # Ouya T30
> > > Tested-by: Paul Fertser <fercerpav@gmail.com> # PAZ00 T20
> > > Tested-by: Nicolas Chauvet <kwizart@gmail.com> # PAZ00 T20 and TK1 T124
> > > Tested-by: Matt Merhar <mattmerhar@protonmail.com> # Ouya T30
> > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> > > ---
> >
> > [...]
> >
> > > +
> > >  static int host1x_probe(struct platform_device *pdev)
> > >  {
> > >         struct host1x *host;
> > > @@ -394,6 +423,10 @@ static int host1x_probe(struct platform_device *pdev)
> > >         /* set common host1x device data */
> > >         platform_set_drvdata(pdev, host);
> > >
> > > +       err = devm_tegra_core_dev_init_opp_table_simple(&pdev->dev);
> > > +       if (err)
> > > +               return err;
> > > +
> > >         host->regs = devm_ioremap_resource(&pdev->dev, regs);
> > >         if (IS_ERR(host->regs))
> > >                 return PTR_ERR(host->regs);
> > > @@ -423,12 +456,9 @@ static int host1x_probe(struct platform_device *pdev)
> > >                 return err;
> > >         }
> > >
> > > -       host->rst = devm_reset_control_get(&pdev->dev, "host1x");
> > > -       if (IS_ERR(host->rst)) {
> > > -               err = PTR_ERR(host->rst);
> > > -               dev_err(&pdev->dev, "failed to get reset: %d\n", err);
> > > +       err = host1x_get_resets(host);
> > > +       if (err)
> > >                 return err;
> > > -       }
> > >
> > >         err = host1x_iommu_init(host);
> > >         if (err < 0) {
> > > @@ -443,22 +473,10 @@ static int host1x_probe(struct platform_device *pdev)
> > >                 goto iommu_exit;
> > >         }
> > >
> > > -       err = clk_prepare_enable(host->clk);
> > > -       if (err < 0) {
> > > -               dev_err(&pdev->dev, "failed to enable clock\n");
> > > -               goto free_channels;
> > > -       }
> > > -
> > > -       err = reset_control_deassert(host->rst);
> > > -       if (err < 0) {
> > > -               dev_err(&pdev->dev, "failed to deassert reset: %d\n", err);
> > > -               goto unprepare_disable;
> > > -       }
> > > -
> >
> > Removing the clk_prepare_enable() and reset_control_deassert() from
> > host1x_probe(), might not be a good idea. See more about why, below.
> >
> > >         err = host1x_syncpt_init(host);
> > >         if (err) {
> > >                 dev_err(&pdev->dev, "failed to initialize syncpts\n");
> > > -               goto reset_assert;
> > > +               goto free_channels;
> > >         }
> > >
> > >         err = host1x_intr_init(host, syncpt_irq);
> > > @@ -467,10 +485,14 @@ static int host1x_probe(struct platform_device *pdev)
> > >                 goto deinit_syncpt;
> > >         }
> > >
> > > -       host1x_debug_init(host);
> > > +       pm_runtime_enable(&pdev->dev);
> > >
> > > -       if (host->info->has_hypervisor)
> > > -               host1x_setup_sid_table(host);
> > > +       /* the driver's code isn't ready yet for the dynamic RPM */
> > > +       err = pm_runtime_resume_and_get(&pdev->dev);
> >
> > If the driver is being built with the CONFIG_PM Kconfig option being
> > unset, pm_runtime_resume_and_get() will return 0 to indicate success -
> > and without calling the ->runtime_resume() callback.
> > In other words, the clock will remain gated and the reset will not be
> > deasserted, likely causing the driver to be malfunctioning.
> >
> > If the driver isn't ever being built with CONFIG_PM unset, feel free
> > to ignore my above comments.
> >
> > Otherwise, if it needs to work both with and without CONFIG_PM being
> > set, you may use the following pattern in host1x_probe() to deploy
> > runtime PM support:
> >
> > "Enable the needed resources to probe the device"
> > pm_runtime_get_noresume()
> > pm_runtime_set_active()
> > pm_runtime_enable()
> >
> > "Before successfully completing probe"
> > pm_runtime_put()
>
> We made a conscious decision a few years ago to have ARCH_TEGRA select
> PM on both 32-bit and 64-bit ARM, specifically to avoid the need to do
> this dance (though there are still a few drivers left that do this, I
> think).
>
> So I think this should be unnecessary. Unless perhaps if the sysfs PM
> controls have any influence on this. As far as I know, as long as the
> PM kconfig option is enabled, the sysfs control only influence the
> runtime behaviour (i.e. setting the sysfs PM control to "on" is going
> to force runtime PM to be resumed) but there's no way to disable
> runtime PM altogether via sysfs that would make the above necessary.

Thanks for clarifying! As I said, feel free to ignore my comments then.

For this and the other patches in the series, I assume you only need
to care about whether the driver is a cross SoC driver and used on
other platforms than Tegra then.

>
> Thierry

Kind regards
Uffe
Thierry Reding Aug. 18, 2021, 1:52 p.m. UTC | #7
On Wed, Aug 18, 2021 at 04:44:30AM +0300, Dmitry Osipenko wrote:
> 18.08.2021 04:15, Rob Herring пишет:
> >> +  tegra-clocks:
> >> +    description: child nodes are the output clocks from the CAR
> >> +    type: object
> >> +
> >> +    patternProperties:
> >> +      "^[a-z]+[0-9]+$":
> >> +        type: object
> >> +        properties:
> >> +          compatible:
> >> +            allOf:
> >> +              - items:
> >> +                  - enum:
> >> +                      - nvidia,tegra20-sclk
> >> +                      - nvidia,tegra30-sclk
> >> +                      - nvidia,tegra30-pllc
> >> +                      - nvidia,tegra30-plle
> >> +                      - nvidia,tegra30-pllm
> >> +              - const: nvidia,tegra-clock
> > You are saying the first string must be both one of the enums and 
> > 'nvidia,tegra-clock'. You don't get an error because your pattern 
> > doesn't match 'sclk'.
> > 
> 
> Could you please rephrase or clarify? If pattern doesn't match 'sclk',
> then it must match any other enum. I'm not sure what you're meaning.

"sclk" doesn't match "^[a-z]+[0-9]+$" because it's missing at least one
digit at the end. Perhaps that last + was supposed to be *?

> 
> The 'nvidia,tegra-clock' actually could be removed since it's
> superfluous now. I'll consider the removal in v9.

It also looks like your schema was meant to be something like:

	compatible:
	  - items:
	      - enum:
	          - nvidia,tegra20-sclk
	          - nvidia,tegra30-sclk
	          - nvidia,tegra30-pllc
	          - nvidia,tegra30-plle
	          - nvidia,tegra30-pllm
	      - const: nvidia,tegra-clock

Note how the const: element is indented one more level. Now this means:
one of the enumeration values, followed by the constant value. That
matches what the example has.

That said, I agree that nvidia,tegra-clock seems a bit useless. There's
really no such thing as a generic clock, they're all different in some
way.

Thierry
Thierry Reding Aug. 18, 2021, 1:59 p.m. UTC | #8
On Tue, Aug 17, 2021 at 04:27:26AM +0300, Dmitry Osipenko wrote:
> Document tegra-clocks sub-node which describes Tegra SoC clocks that
> require a higher voltage of the core power domain in order to operate
> properly on a higher clock rates.  Each node contains a phandle to OPP
> table and power domain.
> 
> The root PLLs and system clocks don't have any specific device dedicated
> to them, clock controller is in charge of managing power for them.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  .../bindings/clock/nvidia,tegra20-car.yaml    | 51 +++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml
> index 459d2a525393..7f5cd27e4ce0 100644
> --- a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml
> +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml
> @@ -42,6 +42,48 @@ properties:
>    "#reset-cells":
>      const: 1
>  
> +  tegra-clocks:
> +    description: child nodes are the output clocks from the CAR
> +    type: object
> +
> +    patternProperties:
> +      "^[a-z]+[0-9]+$":
> +        type: object
> +        properties:
> +          compatible:
> +            allOf:
> +              - items:
> +                  - enum:
> +                      - nvidia,tegra20-sclk
> +                      - nvidia,tegra30-sclk
> +                      - nvidia,tegra30-pllc
> +                      - nvidia,tegra30-plle
> +                      - nvidia,tegra30-pllm
> +              - const: nvidia,tegra-clock
> +
> +          operating-points-v2:
> +            $ref: /schemas/types.yaml#/definitions/phandle
> +            description:
> +              Phandle to OPP table that contains frequencies, voltages and
> +              opp-supported-hw property, which is a bitfield indicating
> +              SoC process or speedo ID mask.
> +
> +          clocks:
> +            items:
> +              - description: node's clock
> +
> +          power-domains:
> +            maxItems: 1
> +            description: phandle to the core SoC power domain
> +
> +        required:
> +          - compatible
> +          - operating-points-v2
> +          - clocks
> +          - power-domains
> +
> +        additionalProperties: false
> +
>  required:
>    - compatible
>    - reg
> @@ -59,6 +101,15 @@ examples:
>          reg = <0x60006000 0x1000>;
>          #clock-cells = <1>;
>          #reset-cells = <1>;
> +
> +        tegra-clocks {
> +            sclk {
> +                compatible = "nvidia,tegra20-sclk", "nvidia,tegra-clock";
> +                operating-points-v2 = <&opp_table>;
> +                clocks = <&tegra_car TEGRA20_CLK_SCLK>;
> +                power-domains = <&domain>;
> +            };
> +        };

I wonder if it'd be better to match on the name of the node rather than
add an artificial compatible string. We usually use the compatible
string to match a device, but here you're really trying to add
information about a resource provided by the CAR controller.

We do similar things for example in PMIC bindings where the individual
regulators are represented in the device tree via nodes named after the
regulator.

You could then also leave out the clocks property, which is weird as it
is because it's basically a self-reference. But you don't really need
the reference here in the first place because the CAR is already the
parent of SCLK.

Also, I don't think the tegra- prefix is necessary here. The parent node
is already identified as Tegra via the compatible string.

In the case of CAR, I'd imagine something like:

	clocks {
		sclk {
			operating-points-v2 = <&opp_table>;
			power-domains = <&domain>;
		};
	};

Now you've only got the bare minimum in here that you actually add. All
the other data that you used to have is simply derived from the parent.

Thierry

>      };
>  
>      usb-controller@c5004000 {
> -- 
> 2.32.0
>
Dmitry Osipenko Aug. 18, 2021, 3:04 p.m. UTC | #9
18.08.2021 16:52, Thierry Reding пишет:
> On Wed, Aug 18, 2021 at 04:44:30AM +0300, Dmitry Osipenko wrote:
>> 18.08.2021 04:15, Rob Herring пишет:
>>>> +  tegra-clocks:
>>>> +    description: child nodes are the output clocks from the CAR
>>>> +    type: object
>>>> +
>>>> +    patternProperties:
>>>> +      "^[a-z]+[0-9]+$":
>>>> +        type: object
>>>> +        properties:
>>>> +          compatible:
>>>> +            allOf:
>>>> +              - items:
>>>> +                  - enum:
>>>> +                      - nvidia,tegra20-sclk
>>>> +                      - nvidia,tegra30-sclk
>>>> +                      - nvidia,tegra30-pllc
>>>> +                      - nvidia,tegra30-plle
>>>> +                      - nvidia,tegra30-pllm
>>>> +              - const: nvidia,tegra-clock
>>> You are saying the first string must be both one of the enums and 
>>> 'nvidia,tegra-clock'. You don't get an error because your pattern 
>>> doesn't match 'sclk'.
>>>
>>
>> Could you please rephrase or clarify? If pattern doesn't match 'sclk',
>> then it must match any other enum. I'm not sure what you're meaning.
> 
> "sclk" doesn't match "^[a-z]+[0-9]+$" because it's missing at least one
> digit at the end. Perhaps that last + was supposed to be *?

Ah, the regex pattern. Yes, I forgot to update it.

>> The 'nvidia,tegra-clock' actually could be removed since it's
>> superfluous now. I'll consider the removal in v9.
> 
> It also looks like your schema was meant to be something like:
> 
> 	compatible:
> 	  - items:
> 	      - enum:
> 	          - nvidia,tegra20-sclk
> 	          - nvidia,tegra30-sclk
> 	          - nvidia,tegra30-pllc
> 	          - nvidia,tegra30-plle
> 	          - nvidia,tegra30-pllm
> 	      - const: nvidia,tegra-clock
> 
> Note how the const: element is indented one more level. Now this means:
> one of the enumeration values, followed by the constant value. That
> matches what the example has.
> 
> That said, I agree that nvidia,tegra-clock seems a bit useless. There's
> really no such thing as a generic clock, they're all different in some
> way.

It's a leftover from older versions of this patchset, I'll remove it.
Dmitry Osipenko Aug. 18, 2021, 3:05 p.m. UTC | #10
18.08.2021 16:59, Thierry Reding пишет:
> On Tue, Aug 17, 2021 at 04:27:26AM +0300, Dmitry Osipenko wrote:
>> Document tegra-clocks sub-node which describes Tegra SoC clocks that
>> require a higher voltage of the core power domain in order to operate
>> properly on a higher clock rates.  Each node contains a phandle to OPP
>> table and power domain.
>>
>> The root PLLs and system clocks don't have any specific device dedicated
>> to them, clock controller is in charge of managing power for them.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  .../bindings/clock/nvidia,tegra20-car.yaml    | 51 +++++++++++++++++++
>>  1 file changed, 51 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml
>> index 459d2a525393..7f5cd27e4ce0 100644
>> --- a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml
>> +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml
>> @@ -42,6 +42,48 @@ properties:
>>    "#reset-cells":
>>      const: 1
>>  
>> +  tegra-clocks:
>> +    description: child nodes are the output clocks from the CAR
>> +    type: object
>> +
>> +    patternProperties:
>> +      "^[a-z]+[0-9]+$":
>> +        type: object
>> +        properties:
>> +          compatible:
>> +            allOf:
>> +              - items:
>> +                  - enum:
>> +                      - nvidia,tegra20-sclk
>> +                      - nvidia,tegra30-sclk
>> +                      - nvidia,tegra30-pllc
>> +                      - nvidia,tegra30-plle
>> +                      - nvidia,tegra30-pllm
>> +              - const: nvidia,tegra-clock
>> +
>> +          operating-points-v2:
>> +            $ref: /schemas/types.yaml#/definitions/phandle
>> +            description:
>> +              Phandle to OPP table that contains frequencies, voltages and
>> +              opp-supported-hw property, which is a bitfield indicating
>> +              SoC process or speedo ID mask.
>> +
>> +          clocks:
>> +            items:
>> +              - description: node's clock
>> +
>> +          power-domains:
>> +            maxItems: 1
>> +            description: phandle to the core SoC power domain
>> +
>> +        required:
>> +          - compatible
>> +          - operating-points-v2
>> +          - clocks
>> +          - power-domains
>> +
>> +        additionalProperties: false
>> +
>>  required:
>>    - compatible
>>    - reg
>> @@ -59,6 +101,15 @@ examples:
>>          reg = <0x60006000 0x1000>;
>>          #clock-cells = <1>;
>>          #reset-cells = <1>;
>> +
>> +        tegra-clocks {
>> +            sclk {
>> +                compatible = "nvidia,tegra20-sclk", "nvidia,tegra-clock";
>> +                operating-points-v2 = <&opp_table>;
>> +                clocks = <&tegra_car TEGRA20_CLK_SCLK>;
>> +                power-domains = <&domain>;
>> +            };
>> +        };
> 
> I wonder if it'd be better to match on the name of the node rather than
> add an artificial compatible string. We usually use the compatible
> string to match a device, but here you're really trying to add
> information about a resource provided by the CAR controller.
> 
> We do similar things for example in PMIC bindings where the individual
> regulators are represented in the device tree via nodes named after the
> regulator.
> 
> You could then also leave out the clocks property, which is weird as it
> is because it's basically a self-reference. But you don't really need
> the reference here in the first place because the CAR is already the
> parent of SCLK.

We don't have a platform device for CaR. I don't see how it's going to
work. We need to create a platform device for each RPM-capable clock
because that's how RPM works. The compatible string is required for
instantiating OF-devices from a node, otherwise we will have to
re-invent the OF core.

> Also, I don't think the tegra- prefix is necessary here. The parent node
> is already identified as Tegra via the compatible string.
> 
> In the case of CAR, I'd imagine something like:
> 
> 	clocks {
> 		sclk {
> 			operating-points-v2 = <&opp_table>;
> 			power-domains = <&domain>;
> 		};
> 	};
> 
> Now you've only got the bare minimum in here that you actually add. All
> the other data that you used to have is simply derived from the parent.

'clocks' is already a generic keyword in DT. It's probably not okay to
redefine it.
Thierry Reding Aug. 18, 2021, 4:39 p.m. UTC | #11
On Wed, Aug 18, 2021 at 06:05:11PM +0300, Dmitry Osipenko wrote:
> 18.08.2021 16:59, Thierry Reding пишет:
> > On Tue, Aug 17, 2021 at 04:27:26AM +0300, Dmitry Osipenko wrote:
> >> Document tegra-clocks sub-node which describes Tegra SoC clocks that
> >> require a higher voltage of the core power domain in order to operate
> >> properly on a higher clock rates.  Each node contains a phandle to OPP
> >> table and power domain.
> >>
> >> The root PLLs and system clocks don't have any specific device dedicated
> >> to them, clock controller is in charge of managing power for them.
> >>
> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >> ---
> >>  .../bindings/clock/nvidia,tegra20-car.yaml    | 51 +++++++++++++++++++
> >>  1 file changed, 51 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml
> >> index 459d2a525393..7f5cd27e4ce0 100644
> >> --- a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml
> >> +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml
> >> @@ -42,6 +42,48 @@ properties:
> >>    "#reset-cells":
> >>      const: 1
> >>  
> >> +  tegra-clocks:
> >> +    description: child nodes are the output clocks from the CAR
> >> +    type: object
> >> +
> >> +    patternProperties:
> >> +      "^[a-z]+[0-9]+$":
> >> +        type: object
> >> +        properties:
> >> +          compatible:
> >> +            allOf:
> >> +              - items:
> >> +                  - enum:
> >> +                      - nvidia,tegra20-sclk
> >> +                      - nvidia,tegra30-sclk
> >> +                      - nvidia,tegra30-pllc
> >> +                      - nvidia,tegra30-plle
> >> +                      - nvidia,tegra30-pllm
> >> +              - const: nvidia,tegra-clock
> >> +
> >> +          operating-points-v2:
> >> +            $ref: /schemas/types.yaml#/definitions/phandle
> >> +            description:
> >> +              Phandle to OPP table that contains frequencies, voltages and
> >> +              opp-supported-hw property, which is a bitfield indicating
> >> +              SoC process or speedo ID mask.
> >> +
> >> +          clocks:
> >> +            items:
> >> +              - description: node's clock
> >> +
> >> +          power-domains:
> >> +            maxItems: 1
> >> +            description: phandle to the core SoC power domain
> >> +
> >> +        required:
> >> +          - compatible
> >> +          - operating-points-v2
> >> +          - clocks
> >> +          - power-domains
> >> +
> >> +        additionalProperties: false
> >> +
> >>  required:
> >>    - compatible
> >>    - reg
> >> @@ -59,6 +101,15 @@ examples:
> >>          reg = <0x60006000 0x1000>;
> >>          #clock-cells = <1>;
> >>          #reset-cells = <1>;
> >> +
> >> +        tegra-clocks {
> >> +            sclk {
> >> +                compatible = "nvidia,tegra20-sclk", "nvidia,tegra-clock";
> >> +                operating-points-v2 = <&opp_table>;
> >> +                clocks = <&tegra_car TEGRA20_CLK_SCLK>;
> >> +                power-domains = <&domain>;
> >> +            };
> >> +        };
> > 
> > I wonder if it'd be better to match on the name of the node rather than
> > add an artificial compatible string. We usually use the compatible
> > string to match a device, but here you're really trying to add
> > information about a resource provided by the CAR controller.
> > 
> > We do similar things for example in PMIC bindings where the individual
> > regulators are represented in the device tree via nodes named after the
> > regulator.
> > 
> > You could then also leave out the clocks property, which is weird as it
> > is because it's basically a self-reference. But you don't really need
> > the reference here in the first place because the CAR is already the
> > parent of SCLK.
> 
> We don't have a platform device for CaR. I don't see how it's going to
> work. We need to create a platform device for each RPM-capable clock
> because that's how RPM works. The compatible string is required for
> instantiating OF-devices from a node, otherwise we will have to
> re-invent the OF core.

I think we do have a platform device for CAR. It's just not bound
against by the driver because these clock drivers are "special". But
from other parts of the series you're already trying to fix that, at
least partially.

But it doesn't seem right to create a platform device for each RPM-
capable clock. Why do they need to be devices? They aren't, so why
pretend? Is it that some API that we want to use here requires the
struct device?

> > Also, I don't think the tegra- prefix is necessary here. The parent node
> > is already identified as Tegra via the compatible string.
> > 
> > In the case of CAR, I'd imagine something like:
> > 
> > 	clocks {
> > 		sclk {
> > 			operating-points-v2 = <&opp_table>;
> > 			power-domains = <&domain>;
> > 		};
> > 	};
> > 
> > Now you've only got the bare minimum in here that you actually add. All
> > the other data that you used to have is simply derived from the parent.
> 
> 'clocks' is already a generic keyword in DT. It's probably not okay to
> redefine it.

"clocks" is not a generic keyword. It's the name of a property and given
that we're talking about the clock provider here, it doesn't need a
clocks property of its own, so it should be fine to use that for the
node.

Thierry
Dmitry Osipenko Aug. 18, 2021, 4:57 p.m. UTC | #12
18.08.2021 19:39, Thierry Reding пишет:
>> We don't have a platform device for CaR. I don't see how it's going to
>> work. We need to create a platform device for each RPM-capable clock
>> because that's how RPM works. The compatible string is required for
>> instantiating OF-devices from a node, otherwise we will have to
>> re-invent the OF core.
> I think we do have a platform device for CAR. It's just not bound
> against by the driver because these clock drivers are "special". But
> from other parts of the series you're already trying to fix that, at
> least partially.
> 
> But it doesn't seem right to create a platform device for each RPM-
> capable clock. Why do they need to be devices? They aren't, so why
> pretend? Is it that some API that we want to use here requires the
> struct device?

The "device" representation is internal to the kernel. It's okay to me
to have PLLs represented by a device, it's a distinct h/w by itself.

CCF supports managing of clock's RPM and it requires to have clock to be
backed by a device. That's what we are using here.

Please see
https://elixir.bootlin.com/linux/v5.14-rc6/source/drivers/clk/clk.c#L109

>>> Also, I don't think the tegra- prefix is necessary here. The parent node
>>> is already identified as Tegra via the compatible string.
>>>
>>> In the case of CAR, I'd imagine something like:
>>>
>>> 	clocks {
>>> 		sclk {
>>> 			operating-points-v2 = <&opp_table>;
>>> 			power-domains = <&domain>;
>>> 		};
>>> 	};
>>>
>>> Now you've only got the bare minimum in here that you actually add. All
>>> the other data that you used to have is simply derived from the parent.
>> 'clocks' is already a generic keyword in DT. It's probably not okay to
>> redefine it.
> "clocks" is not a generic keyword. It's the name of a property and given
> that we're talking about the clock provider here, it doesn't need a
> clocks property of its own, so it should be fine to use that for the
> node.

I'm curious what Rob thinks about it. Rob, does this sound okay to you?
Dmitry Osipenko Aug. 18, 2021, 5:16 p.m. UTC | #13
18.08.2021 19:57, Dmitry Osipenko пишет:
>>>> Also, I don't think the tegra- prefix is necessary here. The parent node
>>>> is already identified as Tegra via the compatible string.
>>>>
>>>> In the case of CAR, I'd imagine something like:
>>>>
>>>> 	clocks {
>>>> 		sclk {
>>>> 			operating-points-v2 = <&opp_table>;
>>>> 			power-domains = <&domain>;
>>>> 		};
>>>> 	};
>>>>
>>>> Now you've only got the bare minimum in here that you actually add. All
>>>> the other data that you used to have is simply derived from the parent.
>>> 'clocks' is already a generic keyword in DT. It's probably not okay to
>>> redefine it.
>> "clocks" is not a generic keyword. It's the name of a property and given
>> that we're talking about the clock provider here, it doesn't need a
>> clocks property of its own, so it should be fine to use that for the
>> node.
> I'm curious what Rob thinks about it. Rob, does this sound okay to you?

I assume dt-schema won't be happy with a different meaning for the 'clocks'.
Dmitry Osipenko Aug. 18, 2021, 5:24 p.m. UTC | #14
18.08.2021 11:35, Ulf Hansson пишет:
> Thanks for clarifying! As I said, feel free to ignore my comments then.
> 
> For this and the other patches in the series, I assume you only need
> to care about whether the driver is a cross SoC driver and used on
> other platforms than Tegra then.

Yes, and all drivers touched by this series are Tegra-only drivers.
Thierry Reding Aug. 19, 2021, 4:31 p.m. UTC | #15
On Wed, Aug 18, 2021 at 07:57:04PM +0300, Dmitry Osipenko wrote:
> 18.08.2021 19:39, Thierry Reding пишет:
> >> We don't have a platform device for CaR. I don't see how it's going to
> >> work. We need to create a platform device for each RPM-capable clock
> >> because that's how RPM works. The compatible string is required for
> >> instantiating OF-devices from a node, otherwise we will have to
> >> re-invent the OF core.
> > I think we do have a platform device for CAR. It's just not bound
> > against by the driver because these clock drivers are "special". But
> > from other parts of the series you're already trying to fix that, at
> > least partially.
> > 
> > But it doesn't seem right to create a platform device for each RPM-
> > capable clock. Why do they need to be devices? They aren't, so why
> > pretend? Is it that some API that we want to use here requires the
> > struct device?
> 
> The "device" representation is internal to the kernel. It's okay to me
> to have PLLs represented by a device, it's a distinct h/w by itself.
> 
> CCF supports managing of clock's RPM and it requires to have clock to be
> backed by a device. That's what we are using here.
> 
> Please see
> https://elixir.bootlin.com/linux/v5.14-rc6/source/drivers/clk/clk.c#L109

Looking at the implementation of __clk_register() and where that device
pointer typically comes from, I don't think the way this is used here is
what was intended. The way I interpret the code is that a clock is
registered with a parent device (i.e. its provider) and
clk_pm_runtime_get() is then used internally as a way to make sure that
when a clock is prepared, it's parent device is runtime resumed. This is
presumably to ensure that any registers that the driver might need to
access in order to prepare and enable the clock are accessible (i.e. the
CAR is not powered off or in reset).

So the struct device that is passed to __clk_register() (or its callers)
should be that of the CAR rather than virtual struct devices created by
the CAR.

And it's a bit debatable whether or not PLLs represent distinct
hardware. Ultimately every transistor on a chip could be considered
distinct hardware. But a platform device is a device on a platform bus,
which is really just another way of saying it's a hardware block that's
accessible from the CPU via a memory-mapped address. A PLL (just like
other clocks) is merely a resource exposed by means of access to these
registers. So I don't think they should be platform devices. Even making
them struct device:s seems a bit of a stretch.

Is there any reason why struct clk can't be used for this? I mean, the
whole purpose of that structure is to represent clocks. Why do we need
to make them special?

> >>> Also, I don't think the tegra- prefix is necessary here. The parent node
> >>> is already identified as Tegra via the compatible string.
> >>>
> >>> In the case of CAR, I'd imagine something like:
> >>>
> >>> 	clocks {
> >>> 		sclk {
> >>> 			operating-points-v2 = <&opp_table>;
> >>> 			power-domains = <&domain>;
> >>> 		};
> >>> 	};
> >>>
> >>> Now you've only got the bare minimum in here that you actually add. All
> >>> the other data that you used to have is simply derived from the parent.
> >> 'clocks' is already a generic keyword in DT. It's probably not okay to
> >> redefine it.
> > "clocks" is not a generic keyword. It's the name of a property and given
> > that we're talking about the clock provider here, it doesn't need a
> > clocks property of its own, so it should be fine to use that for the
> > node.
> 
> I'm curious what Rob thinks about it. Rob, does this sound okay to you?

Another alternative would be to omit that level altogether and just make
sclk and siblings direct children of the CAR node.

Thierry
Dmitry Osipenko Aug. 19, 2021, 10:20 p.m. UTC | #16
19.08.2021 19:31, Thierry Reding пишет:
>>>>> Also, I don't think the tegra- prefix is necessary here. The parent node
>>>>> is already identified as Tegra via the compatible string.
>>>>>
>>>>> In the case of CAR, I'd imagine something like:
>>>>>
>>>>> 	clocks {
>>>>> 		sclk {
>>>>> 			operating-points-v2 = <&opp_table>;
>>>>> 			power-domains = <&domain>;
>>>>> 		};
>>>>> 	};
>>>>>
>>>>> Now you've only got the bare minimum in here that you actually add. All
>>>>> the other data that you used to have is simply derived from the parent.
>>>> 'clocks' is already a generic keyword in DT. It's probably not okay to
>>>> redefine it.
>>> "clocks" is not a generic keyword. It's the name of a property and given
>>> that we're talking about the clock provider here, it doesn't need a
>>> clocks property of its own, so it should be fine to use that for the
>>> node.
>> I'm curious what Rob thinks about it. Rob, does this sound okay to you?
> Another alternative would be to omit that level altogether and just make
> sclk and siblings direct children of the CAR node.

That can be done.
Dmitry Osipenko Aug. 20, 2021, 2:51 a.m. UTC | #17
19.08.2021 19:31, Thierry Reding пишет:
>> The "device" representation is internal to the kernel. It's okay to me
>> to have PLLs represented by a device, it's a distinct h/w by itself.
>>
>> CCF supports managing of clock's RPM and it requires to have clock to be
>> backed by a device. That's what we are using here.
>>
>> Please see
>> https://elixir.bootlin.com/linux/v5.14-rc6/source/drivers/clk/clk.c#L109
> Looking at the implementation of __clk_register() and where that device
> pointer typically comes from, I don't think the way this is used here is
> what was intended. The way I interpret the code is that a clock is
> registered with a parent device (i.e. its provider) and
> clk_pm_runtime_get() is then used internally as a way to make sure that
> when a clock is prepared, it's parent device is runtime resumed. This is
> presumably to ensure that any registers that the driver might need to
> access in order to prepare and enable the clock are accessible (i.e. the
> CAR is not powered off or in reset).
> 
> So the struct device that is passed to __clk_register() (or its callers)
> should be that of the CAR rather than virtual struct devices created by
> the CAR.
> 
> And it's a bit debatable whether or not PLLs represent distinct
> hardware. Ultimately every transistor on a chip could be considered
> distinct hardware. But a platform device is a device on a platform bus,
> which is really just another way of saying it's a hardware block that's
> accessible from the CPU via a memory-mapped address. A PLL (just like
> other clocks) is merely a resource exposed by means of access to these
> registers. So I don't think they should be platform devices. Even making
> them struct device:s seems a bit of a stretch.
> 
> Is there any reason why struct clk can't be used for this? I mean, the
> whole purpose of that structure is to represent clocks. Why do we need
> to make them special?

Because we need to perform DVFS for PLLs. The only way to do it without
having to reinvent existing frameworks is to use these frameworks and
they require a device.