Stefan Wahren writes: > Hi, > > Am 18.07.19 um 20:37 schrieb Eric Anholt: >> Florian Fainelli writes: >> >>> On 7/18/2019 1:47 AM, Matthias Brugger wrote: >>>> >>>> On 17/07/2019 21:50, Stefan Wahren wrote: >>>>> The new BCM2838 supports an additional clock for the emmc2 block. >>>>> So add a new compatible to register this clock only for BCM2838. >>>>> >>>>> Signed-off-by: Stefan Wahren >>>>> --- >>>>> drivers/clk/bcm/clk-bcm2835.c | 33 +++++++++++++++++++++++++++++++-- >>>>> 1 file changed, 31 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c >>>>> index 867ae3c..5fe4695 100644 >>>>> --- a/drivers/clk/bcm/clk-bcm2835.c >>>>> +++ b/drivers/clk/bcm/clk-bcm2835.c >>>>> @@ -31,7 +31,8 @@ >>>>> #include >>>>> #include >>>>> #include >>>>> -#include >>>>> +#include >>>>> + >>>>> #include >>>>> #include >>>>> #include >>>>> @@ -114,6 +115,8 @@ >>>>> #define CM_AVEODIV 0x1bc >>>>> #define CM_EMMCCTL 0x1c0 >>>>> #define CM_EMMCDIV 0x1c4 >>>>> +#define CM_EMMC2CTL 0x1d0 >>>>> +#define CM_EMMC2DIV 0x1d4 >>>>> >>>>> /* General bits for the CM_*CTL regs */ >>>>> # define CM_ENABLE BIT(4) >>>>> @@ -1950,6 +1953,15 @@ static const struct bcm2835_clk_desc clk_desc_array[] = { >>>>> .frac_bits = 8, >>>>> .tcnt_mux = 39), >>>>> >>>>> + /* EMMC2 clock (only available for BCM2838) */ >>>>> + [BCM2838_CLOCK_EMMC2] = REGISTER_PER_CLK( >>>>> + .name = "emmc2", >>>>> + .ctl_reg = CM_EMMC2CTL, >>>>> + .div_reg = CM_EMMC2DIV, >>>>> + .int_bits = 4, >>>>> + .frac_bits = 8, >>>>> + .tcnt_mux = 42), >>>>> + >>>>> /* General purpose (GPIO) clocks */ >>>>> [BCM2835_CLOCK_GP0] = REGISTER_PER_CLK( >>>>> .name = "gp0", >>>>> @@ -2101,6 +2113,14 @@ static int bcm2835_mark_sdc_parent_critical(struct clk *sdc) >>>>> return clk_prepare_enable(parent); >>>>> } >>>>> >>>>> +bool bcm2835_has_clk(size_t index) { >>>>> + return index != BCM2838_CLOCK_EMMC2; >>>>> +} >>>>> + >>>>> +bool bcm2838_has_clk(size_t index) { >>>>> + return true; >>>>> +} >>>>> + >>>>> static int bcm2835_clk_probe(struct platform_device *pdev) >>>>> { >>>>> struct device *dev = &pdev->dev; >>>>> @@ -2109,9 +2129,14 @@ static int bcm2835_clk_probe(struct platform_device *pdev) >>>>> struct resource *res; >>>>> const struct bcm2835_clk_desc *desc; >>>>> const size_t asize = ARRAY_SIZE(clk_desc_array); >>>>> + bool (*has_clk_func)(size_t); >>>>> size_t i; >>>>> int ret; >>>>> >>>>> + has_clk_func = of_device_get_match_data(&pdev->dev); >>>>> + if (!has_clk_func) >>>>> + return -ENODEV; >>>>> + >>>>> cprman = devm_kzalloc(dev, >>>>> struct_size(cprman, onecell.hws, asize), >>>>> GFP_KERNEL); >>>>> @@ -2146,6 +2171,9 @@ static int bcm2835_clk_probe(struct platform_device *pdev) >>>>> hws = cprman->onecell.hws; >>>>> >>>>> for (i = 0; i < asize; i++) { >>>>> + if (!has_clk_func(i)) >>>>> + continue; >>>>> + >>>> I think that's very hacky. Can't we just create a per SoC list which get's >>>> passed via .data and in probe we iterate through that list and enable the >>>> clocks? That would make clear which clocks are just for bcm2838, basically emmc2. >>> Or within the structure, add a flag that indicates whether the clock is >>> available or not for a given chip? That would avoid having to introduce >>> bcm283x_has_clk() functions that needs to maintain a possibly growing >>> list of clocks. You would still have to check the flag though. >> I think this is a good solution. > > i only want to make sure that i get it right: > > - add a new member supported (e.g. unsigned int) into struct > bcm2835_clk_desc > - bit 1 should be BCM2835 and 2 should be BCM2711 (so BCM7211 could use > 3 and so ...) > - during probing we should check the bit against the SoC bit before > registration Sounds good to me.