Re: [PATCH 22/89] clk: bcm: rpi: Discover the firmware clocks

[Stephen Boyd <sboyd@kernel.org>]

Quoting Maxime Ripard (2020-02-24 01:06:24)
> The firmware has an interface to discover the clocks it exposes.
> 
> Let's use it to discover, register the clocks in the clocks framework and
> then expose them through the device tree for consumers to use them.

Everyone's doing it! :)

> diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
> index 3f21888a3e3e..bf6a1e2dc099 100644
> --- a/drivers/clk/bcm/clk-raspberrypi.c
> +++ b/drivers/clk/bcm/clk-raspberrypi.c
> @@ -285,6 +285,95 @@ static struct clk_hw *raspberrypi_register_pllb_arm(struct raspberrypi_clk *rpi)
>         return &raspberrypi_clk_pllb_arm.hw;
>  }
>  
> +static long raspberrypi_fw_dumb_round_rate(struct clk_hw *hw,
> +                                          unsigned long rate,
> +                                          unsigned long *parent_rate)
> +{
> +       return rate;

Can we get a comment here like "The firmware does the rounding but
doesn't tell us so we have to assume anything goes!"

> +}
> +
> +static const struct clk_ops raspberrypi_firmware_clk_ops = {
> +       .is_prepared    = raspberrypi_fw_is_prepared,
> +       .recalc_rate    = raspberrypi_fw_get_rate,
> +       .round_rate     = raspberrypi_fw_dumb_round_rate,
> +       .set_rate       = raspberrypi_fw_set_rate,
> +};
> +
> +static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk *rpi,
> +                                              unsigned int parent,
> +                                              unsigned int id)
> +{
> +       struct raspberrypi_clk_data *data;
> +       struct clk_init_data init = {};
> +       int ret;
> +
> +       if (id == RPI_FIRMWARE_ARM_CLK_ID) {
> +               struct clk_hw *hw;
> +
> +               hw = raspberrypi_register_pllb(rpi);
> +               if (IS_ERR(hw)) {
> +                       dev_err(rpi->dev, "Failed to initialize pllb, %ld\n",
> +                               PTR_ERR(hw));
> +                       return hw;
> +               }
> +
> +               hw = raspberrypi_register_pllb_arm(rpi);
> +               if (IS_ERR(hw))
> +                       return hw;
> +
> +               return hw;

Why the double return? Not just

	return raspberrypi_register_pllb_arm(rpi);

> +       }
> +
> +       data = devm_kzalloc(rpi->dev, sizeof(data), GFP_KERNEL);
> +       if (!data)
> +               return ERR_PTR(-ENOMEM);
> +       data->rpi = rpi;
> +       data->id = id;
> +
> +       init.name = devm_kasprintf(rpi->dev, GFP_KERNEL, "fw-clk-%u", id);
> +       init.ops = &raspberrypi_firmware_clk_ops;
> +       init.flags = CLK_GET_RATE_NOCACHE | CLK_IGNORE_UNUSED;

Why ignore unused? Doesn't seem to support enable/disable anyway so not
sure this flag adds any value.

> +
> +       data->hw.init = &init;
> +
> +       ret = devm_clk_hw_register(rpi->dev, &data->hw);
> +       if (ret)
> +               return ERR_PTR(ret);
> +
> +       return &data->hw;
> +}
> +
> +static int raspberrypi_discover_clocks(struct raspberrypi_clk *rpi,
> +                                      struct clk_hw_onecell_data *data)
> +{
> +       struct rpi_firmware_get_clocks_response *clks;
> +       size_t clks_size = NUM_FW_CLKS * sizeof(*clks);
> +       int ret;
> +
> +       clks = devm_kzalloc(rpi->dev, clks_size, GFP_KERNEL);

Use devm_kcalloc to avoid overflow and indicate it's an array.

> +       if (!clks)
> +               return -ENOMEM;
> +
> +       ret = rpi_firmware_property(rpi->firmware, RPI_FIRMWARE_GET_CLOCKS,
> +                                   clks, clks_size);
> +       if (ret)
> +               return ret;
> +
> +       while (clks->id) {
> +               struct clk_hw *hw;
> +
> +               hw = raspberrypi_clk_register(rpi, clks->parent, clks->id);
> +               if (IS_ERR(hw))
> +                       return PTR_ERR(hw);
> +
> +               data->hws[clks->id] = hw;
> +               data->num = clks->id + 1;
> +               clks++;
> +       }
> +
> +       return 0;
> +}
> +
>  static int raspberrypi_clk_probe(struct platform_device *pdev)
>  {
>         struct clk_hw_onecell_data *clk_data;
> diff --git a/include/soc/bcm2835/raspberrypi-firmware.h b/include/soc/bcm2835/raspberrypi-firmware.h
> index 7800e12ee042..e5b7a41bba6b 100644
> --- a/include/soc/bcm2835/raspberrypi-firmware.h
> +++ b/include/soc/bcm2835/raspberrypi-firmware.h
> @@ -135,6 +135,11 @@ enum rpi_firmware_property_tag {
>         RPI_FIRMWARE_GET_DMA_CHANNELS =                       0x00060001,
>  };
>  
> +struct rpi_firmware_get_clocks_response {
> +       __le32  parent;
> +       __le32  id;

Why double spaced or tabbed out?

> +};
> +
>  #if IS_ENABLED(CONFIG_RASPBERRYPI_FIRMWARE)
>  int rpi_firmware_property(struct rpi_firmware *fw,