[2/5] clk: bcm2835: enable management of PCM clock

Commit Message

Martin Sperl Jan. 10, 2016, 6:01 p.m. UTC

> On 10.01.2016, at 14:02, Geert Uytterhoeven <[e-mail]> wrote:
> 
> I wrote:
> | Hence it can better be replaced (it seems to be unused in dts files, but you
> | can keep the definition to be 100% sure) by an ARRAY_SIZE() in the C driver.
> | This requires changing the driver to e.g. initialize clks[] in
> |bcm2835_clk_probe() based on a table instead of explicit code.
> 
> If you fill in clks[] from a static table, you can take ARRAY_SIZE of
> the static table.

You mean something like the below?
(note: copy/paste from console issues - spaces instead of tabs)



If so, then I will send it as a patch plus the PCM patch after I have
finished testing the patch.

This patch-set would also include the complete list of clocks still 
missing from dt-bindings.h as well as the patch that adds all the
clock registers, which I had sent earlier.

Comments

Martin Sperl Jan. 10, 2016, 7:07 p.m. UTC | #1

> On 10.01.2016, at 19:56, Geert Uytterhoeven <[e-mail]> wrote:
> 
> Hi Martin,
> 
> On Sun, Jan 10, 2016 at 7:01 PM, Martin Sperl <[e-mail]> wrote:
>>> On 10.01.2016, at 14:02, Geert Uytterhoeven <[e-mail]> wrote:
>>> I wrote:
>>> | Hence it can better be replaced (it seems to be unused in dts files, but you
>>> | can keep the definition to be 100% sure) by an ARRAY_SIZE() in the C driver.
>>> | This requires changing the driver to e.g. initialize clks[] in
>>> |bcm2835_clk_probe() based on a table instead of explicit code.
>>> 
>>> If you fill in clks[] from a static table, you can take ARRAY_SIZE of
>>> the static table.
>> 
>> You mean something like the below?
>> (note: copy/paste from console issues - spaces instead of tabs)
> 
> More or less.
> 
>> };
>> 
>> static inline void cprman_write(struct bcm2835_cprman *cprman, u32 reg, u32 val
>> @@ -496,6 +496,14 @@ static const struct bcm2835_pll_data bcm2835_pllh_data = {
>>        .max_fb_rate = BCM2835_MAX_FB_RATE,
>> };
>> 
>> +static const struct bcm2835_pll_data *bcm2835_plls[] = {
>> +       [BCM2835_PLLA] = &bcm2835_plla_data,
>> +       [BCM2835_PLLB] = &bcm2835_pllb_data,
>> +       [BCM2835_PLLC] = &bcm2835_pllc_data,
>> +       [BCM2835_PLLD] = &bcm2835_plld_data,
>> +       [BCM2835_PLLH] = &bcm2835_pllh_data,
>> +};
> 
> This is a sparse array?
Yes - for all.

> 
>> struct bcm2835_pll {
>>        struct clk_hw hw;
>>        struct bcm2835_cprman *cprman;
>> @@ -1560,8 +1600,13 @@ static int bcm2835_clk_probe(struct platform_device *pdev)
>>        struct clk **clks;
>>        struct bcm2835_cprman *cprman;
>>        struct resource *res;
>> +       const int clks_cnt = max(ARRAY_SIZE(bcm2835_plls),
>> +                                max(ARRAY_SIZE(bcm2835_pll_divs),
>> +                                    ARRAY_SIZE(bcm2835_clks))) + 1;
>> +       size_t alloc = sizeof(*cprman) + clks_cnt * sizeof(*clks);
>> +       size_t i;
> 
> If you combine all 3 arrays in a single non-sparse array, you could get rid
> of the dynamic allocation using the maximum of the 3 sizes, and can just
> use a single ARRAY_SIZE().


The problem is that we need different initialization methods for pll, pll-dividers
and derived clocks, so we can not easily make them a common setting unless we
would use function and void pointers to work arround this, which would make it
less readable (and more code again just for the - repeated - assignment).

As far as I can tell from my testing the patched version works and the kernel
boots correctly.

I will respin the patchset tomorrow - it might take slightly longer, as I
found that the fractional clock divider is not working unless the MASH
functionality is activated in the registers. This results in “frequency shifts”
for audio, which we want to avoid - this means we need another patch to
enable this MASH feature.

Patch

diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
index dee67b3..5ce5e7f 100644
--- a/drivers/clk/bcm/clk-bcm2835.c
+++ b/drivers/clk/bcm/clk-bcm2835.c
@@ -294,7 +294,7 @@  struct bcm2835_cprman {
        const char *osc_name;

        struct clk_onecell_data onecell;
-       struct clk *clks[BCM2835_CLOCK_COUNT];
+       struct clk *clks[];
 };

 static inline void cprman_write(struct bcm2835_cprman *cprman, u32 reg, u32 val
@@ -496,6 +496,14 @@  static const struct bcm2835_pll_data bcm2835_pllh_data = {
        .max_fb_rate = BCM2835_MAX_FB_RATE,
 };

+static const struct bcm2835_pll_data *bcm2835_plls[] = {
+       [BCM2835_PLLA] = &bcm2835_plla_data,
+       [BCM2835_PLLB] = &bcm2835_pllb_data,
+       [BCM2835_PLLC] = &bcm2835_pllc_data,
+       [BCM2835_PLLD] = &bcm2835_plld_data,
+       [BCM2835_PLLH] = &bcm2835_pllh_data,
+};
+
 struct bcm2835_pll_divider_data {
        const char *name;
        const struct bcm2835_pll_data *source_pll;
@@ -625,6 +633,20 @@  static const struct bcm2835_pll_divider_data bcm2835_pllh_p
        .fixed_divider = 10,
 };

+static const struct bcm2835_pll_divider_data *bcm2835_pll_divs[] = {
+       [BCM2835_PLLA_CORE]     = &bcm2835_plla_core_data,
+       [BCM2835_PLLA_PER]      = &bcm2835_plla_per_data,
+       [BCM2835_PLLC_CORE0]    = &bcm2835_pllc_core0_data,
+       [BCM2835_PLLC_CORE1]    = &bcm2835_pllc_core1_data,
+       [BCM2835_PLLC_CORE2]    = &bcm2835_pllc_core2_data,
+       [BCM2835_PLLC_PER]      = &bcm2835_pllc_per_data,
+       [BCM2835_PLLD_CORE]     = &bcm2835_plld_core_data,
+       [BCM2835_PLLD_PER]      = &bcm2835_plld_per_data,
+       [BCM2835_PLLH_RCAL]     = &bcm2835_pllh_rcal_data,
+       [BCM2835_PLLH_AUX]      = &bcm2835_pllh_aux_data,
+       [BCM2835_PLLH_PIX]      = &bcm2835_pllh_pix_data,
+};
+
 struct bcm2835_clock_data {
        const char *name;

@@ -837,6 +859,24 @@  static const struct bcm2835_clock_data bcm2835_clock_pcm_da
        .mash = 1,
 };

+static const struct bcm2835_clock_data *bcm2835_clks[] = {
+       [BCM2835_CLOCK_TIMER]   = &bcm2835_clock_timer_data,
+       [BCM2835_CLOCK_OTP]     = &bcm2835_clock_otp_data,
+       [BCM2835_CLOCK_TSENS]   = &bcm2835_clock_tsens_data,
+       [BCM2835_CLOCK_VPU]     = &bcm2835_clock_vpu_data,
+       [BCM2835_CLOCK_V3D]     = &bcm2835_clock_v3d_data,
+       [BCM2835_CLOCK_ISP]     = &bcm2835_clock_isp_data,
+       [BCM2835_CLOCK_H264]    = &bcm2835_clock_h264_data,
+       [BCM2835_CLOCK_V3D]     = &bcm2835_clock_v3d_data,
+       [BCM2835_CLOCK_SDRAM]   = &bcm2835_clock_sdram_data,
+       [BCM2835_CLOCK_UART]    = &bcm2835_clock_uart_data,
+       [BCM2835_CLOCK_VEC]     = &bcm2835_clock_vec_data,
+       [BCM2835_CLOCK_HSM]     = &bcm2835_clock_hsm_data,
+       [BCM2835_CLOCK_EMMC]    = &bcm2835_clock_emmc_data,
+       [BCM2835_CLOCK_PWM]     = &bcm2835_clock_pwm_data,
+       [BCM2835_CLOCK_PCM]     = &bcm2835_clock_pcm_data,
+};
+
 struct bcm2835_pll {
        struct clk_hw hw;
        struct bcm2835_cprman *cprman;
@@ -1560,8 +1600,13 @@  static int bcm2835_clk_probe(struct platform_device *pdev)
        struct clk **clks;
        struct bcm2835_cprman *cprman;
        struct resource *res;
+       const int clks_cnt = max(ARRAY_SIZE(bcm2835_plls),
+                                max(ARRAY_SIZE(bcm2835_pll_divs),
+                                    ARRAY_SIZE(bcm2835_clks))) + 1;
+       size_t alloc = sizeof(*cprman) + clks_cnt * sizeof(*clks);
+       size_t i;

-       cprman = devm_kzalloc(dev, sizeof(*cprman), GFP_KERNEL);
+       cprman = devm_kzalloc(dev, alloc , GFP_KERNEL);
        if (!cprman)
                return -ENOMEM;

@@ -1578,67 +1623,43 @@  static int bcm2835_clk_probe(struct platform_device *pdev)

        platform_set_drvdata(pdev, cprman);

-       cprman->onecell.clk_num = BCM2835_CLOCK_COUNT;
+       cprman->onecell.clk_num = clks_cnt;
        cprman->onecell.clks = cprman->clks;
        clks = cprman->clks;

-       clks[BCM2835_PLLA] = bcm2835_register_pll(cprman, &bcm2835_plla_data);
-       clks[BCM2835_PLLB] = bcm2835_register_pll(cprman, &bcm2835_pllb_data);
-       clks[BCM2835_PLLC] = bcm2835_register_pll(cprman, &bcm2835_pllc_data);
-       clks[BCM2835_PLLD] = bcm2835_register_pll(cprman, &bcm2835_plld_data);
-       clks[BCM2835_PLLH] = bcm2835_register_pll(cprman, &bcm2835_pllh_data);
-
-       clks[BCM2835_PLLA_CORE] =
-               bcm2835_register_pll_divider(cprman, &bcm2835_plla_core_data);
-       clks[BCM2835_PLLA_PER] =
-               bcm2835_register_pll_divider(cprman, &bcm2835_plla_per_data);
-       clks[BCM2835_PLLC_CORE0] =
-               bcm2835_register_pll_divider(cprman, &bcm2835_pllc_core0_data);
-       clks[BCM2835_PLLC_CORE1] =
-               bcm2835_register_pll_divider(cprman, &bcm2835_pllc_core1_data);
-       clks[BCM2835_PLLC_CORE2] =
-               bcm2835_register_pll_divider(cprman, &bcm2835_pllc_core2_data);
-       clks[BCM2835_PLLC_PER] =
-               bcm2835_register_pll_divider(cprman, &bcm2835_pllc_per_data);
-       clks[BCM2835_PLLD_CORE] =
-               bcm2835_register_pll_divider(cprman, &bcm2835_plld_core_data);
-       clks[BCM2835_PLLD_PER] =
-               bcm2835_register_pll_divider(cprman, &bcm2835_plld_per_data);
-       clks[BCM2835_PLLH_RCAL] =
-               bcm2835_register_pll_divider(cprman, &bcm2835_pllh_rcal_data);
-       clks[BCM2835_PLLH_AUX] =
-               bcm2835_register_pll_divider(cprman, &bcm2835_pllh_aux_data);
-       clks[BCM2835_PLLH_PIX] =
-               bcm2835_register_pll_divider(cprman, &bcm2835_pllh_pix_data);
-
-       clks[BCM2835_CLOCK_TIMER] =
-               bcm2835_register_clock(cprman, &bcm2835_clock_timer_data);
-       clks[BCM2835_CLOCK_OTP] =
-               bcm2835_register_clock(cprman, &bcm2835_clock_otp_data);
-       clks[BCM2835_CLOCK_TSENS] =
-               bcm2835_register_clock(cprman, &bcm2835_clock_tsens_data);
-       clks[BCM2835_CLOCK_VPU] =
-               bcm2835_register_clock(cprman, &bcm2835_clock_vpu_data);
-       clks[BCM2835_CLOCK_V3D] =
-               bcm2835_register_clock(cprman, &bcm2835_clock_v3d_data);
-       clks[BCM2835_CLOCK_ISP] =
-               bcm2835_register_clock(cprman, &bcm2835_clock_isp_data);
-       clks[BCM2835_CLOCK_H264] =
-               bcm2835_register_clock(cprman, &bcm2835_clock_h264_data);
-       clks[BCM2835_CLOCK_V3D] =
-               bcm2835_register_clock(cprman, &bcm2835_clock_v3d_data);
-       clks[BCM2835_CLOCK_SDRAM] =
-               bcm2835_register_clock(cprman, &bcm2835_clock_sdram_data);
-       clks[BCM2835_CLOCK_UART] =
-               bcm2835_register_clock(cprman, &bcm2835_clock_uart_data);
-       clks[BCM2835_CLOCK_VEC] =
-               bcm2835_register_clock(cprman, &bcm2835_clock_vec_data);
-       clks[BCM2835_CLOCK_HSM] =
-               bcm2835_register_clock(cprman, &bcm2835_clock_hsm_data);
-       clks[BCM2835_CLOCK_EMMC] =
-               bcm2835_register_clock(cprman, &bcm2835_clock_emmc_data);
-       clks[BCM2835_CLOCK_PCM] =
-               bcm2835_register_clock(cprman, &bcm2835_clock_pcm_data);
+       /*  register pll */
+       for(i = 0; i< ARRAY_SIZE(bcm2835_plls); i++) {
+               if (!bcm2835_plls[i])
+                       continue;
+               clks[i] = bcm2835_register_pll( cprman, bcm2835_plls[i]);
+       }
+
+       /* register pll divider */
+       for(i = 0; i< ARRAY_SIZE(bcm2835_pll_divs); i++) {
+               if (!bcm2835_pll_divs[i])
+                       continue;
+               if (clks[i]) {
+                       dev_err(dev,
+                               "Could not register pll_div_id %i - is already defined as: %pC\n",
+                               i, clks[i]);
+                       continue;
+               }
+               clks[i] = bcm2835_register_pll_divider(cprman,
+                                                      bcm2835_pll_divs[i]);
+       }
+
+       /* register clocks */
+       for(i = 0; i< ARRAY_SIZE(bcm2835_clks); i++) {
+               if (!bcm2835_clks[i])
+                       continue;
+               if (clks[i]) {
+                       dev_err(dev,
+                               "Could not register clock_id %i - is already defined as: %pC\n",
+                               i, clks[i]);
+                       continue;
+               }
+               clks[i] = bcm2835_register_clock(cprman, bcm2835_clks[i]);
+       }

        /*
         * CM_PERIICTL (and CM_PERIACTL, CM_SYSCTL and CM_VPUCTL if
@@ -1652,8 +1673,6 @@  static int bcm2835_clk_probe(struct platform_device *pdev)
                                  cprman->regs + CM_PERIICTL, CM_GATE_BIT,
                                  0, &cprman->regs_lock);

-       clks[BCM2835_CLOCK_PWM] =
-               bcm2835_register_clock(cprman, &bcm2835_clock_pwm_data);

        return of_clk_add_provider(dev->of_node, of_clk_src_onecell_get,
                                   &cprman->onecell);