Re: [PATCH v2 3/3] mfd: add CSR SiRFSoC on-chip power management module driver

[Lee Jones <lee.jones@linaro.org>]

On Mon, 21 Sep 2015, Barry Song wrote:
> 2015-09-20 12:15 GMT+08:00 Lee Jones <lee.jones@linaro.org>:
> > On Thu, 17 Sep 2015, Barry Song wrote:
> >
> >> From: Guo Zeng <Guo.Zeng@csr.com>
> >>
> >> CSR SiRFSoC Power Control Module includes power on or power
> >> off for sysctl power and gnss, it also includes onkey, RTC
> >> domain clock controllers and interrupt controller for power
> >> events.
> >
> > There are lots of Three (and four) Letter Abbreviations (TLAs) here,
> > which mean nothing to the average reader.  Please break them out in
> > the commit log as I have done i.e. "Long Abbreviated Word (LAW)", so
> > us normies can see what they mean.
> >
> >> Signed-off-by: Guo Zeng <Guo.Zeng@csr.com>
> >> Signed-off-by: Barry Song <Baohua.Song@csr.com>
> >> ---
> >>  .../devicetree/bindings/mfd/sirf-pwrc.txt          |  37 ++++
> >
> > This should be in a separate patch.
> >
> >>  drivers/mfd/Kconfig                                |  12 ++
> >>  drivers/mfd/Makefile                               |   2 +
> >>  drivers/mfd/sirfsoc_pwrc.c                         | 238 +++++++++++++++++++++
> >>  include/linux/mfd/sirfsoc_pwrc.h                   |  97 +++++++++
> >>  5 files changed, 386 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/mfd/sirf-pwrc.txt
> >>  create mode 100644 drivers/mfd/sirfsoc_pwrc.c
> >>  create mode 100644 include/linux/mfd/sirfsoc_pwrc.h

[...]

> >> +#include <linux/rtc/sirfsoc_rtciobrg.h>
> >
> > What's this for?
> 
> the registers are behind a rtc io bridge. HW needs to access the
> bridge to access registers.
> another example similar with pwrc MFD is sysrtc as below:
> 
> 2019                         rtc-iobg@18840000 {
> 2020                                 compatible = "sirf,prima2-rtciobg",
> 2021                                         "sirf-prima2-rtciobg-bus",
> 2022                                         "simple-bus";
> 2023                                 #address-cells = <1>;
> 2024                                 #size-cells = <1>;
> 2025                                 reg = <0x18840000 0x1000>;
> 2026
> 2027                                 sysrtc@2000 {
> 2028                                         compatible = "sirf,prima2-sysrtc";
> 2029                                         reg = <0x2000 0x100>;
> 2030                                         interrupts = <0 52 0>;
> 2031                                 };
> 2032                                 pwrc@3000 {
> 2033                                         compatible = "sirf,atlas7-pwrc";
> 2034                                         reg = <0x3000 0x100>;
> 2035                                         interrupts = <0 32 0>;
> 2036                                         rtcmclk: clock-controller {
> 
> its driver is at
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/rtc/rtc-sirfsoc.c

I meant what are you using in the header file, but I looked it up.

> >> +#include <linux/mfd/core.h>
> >> +#include <linux/mfd/sirfsoc_pwrc.h>
> >
> > Header files should be in alphabetical order.
> >
> >> +struct sirfsoc_pwrc_register sirfsoc_a7da_pwrc = {
> >> +     .pwrc_pdn_ctrl_set = 0x0,
> >> +     .pwrc_pdn_ctrl_clr = 0x4,
> >> +     .pwrc_pon_status = 0x8,
> >> +     .pwrc_trigger_en_set = 0xc,
> >> +     .pwrc_trigger_en_clr = 0x10,
> >> +     .pwrc_int_mask_set = 0x14,
> >> +     .pwrc_int_mask_clr = 0x18,
> >> +     .pwrc_int_status = 0x1c,
> >> +     .pwrc_pin_status = 0x20,
> >> +     .pwrc_rtc_pll_ctrl = 0x28,
> >> +     .pwrc_gpio3_debug = 0x34,
> >> +     .pwrc_rtc_noc_pwrctl_set = 0x38,
> >> +     .pwrc_rtc_noc_pwrctl_clr = 0x3c,
> >> +     .pwrc_rtc_can_ctrl = 0x48,
> >> +     .pwrc_rtc_can_status = 0x4c,
> >> +     .pwrc_fsm_m3_ctrl = 0x50,
> >> +     .pwrc_fsm_state = 0x54,
> >> +     .pwrc_rtcldo_reg = 0x58,
> >> +     .pwrc_gnss_ctrl = 0x5c,
> >> +     .pwrc_gnss_status = 0x60,
> >> +     .pwrc_xtal_reg = 0x64,
> >> +     .pwrc_xtal_ldo_mux_sel = 0x68,
> >> +     .pwrc_rtc_sw_rstc_set = 0x6c,
> >> +     .pwrc_rtc_sw_rstc_clr = 0x70,
> >> +     .pwrc_power_sw_ctrl_set = 0x74,
> >> +     .pwrc_power_sw_ctrl_clr = 0x78,
> >> +     .pwrc_rtc_dcog = 0x7c,
> >> +     .pwrc_m3_memories = 0x80,
> >> +     .pwrc_can0_memory = 0x84,
> >> +     .pwrc_rtc_gnss_memory = 0x88,
> >> +     .pwrc_m3_clk_en = 0x8c,
> >> +     .pwrc_can0_clk_en = 0x90,
> >> +     .pwrc_spi0_clk_en = 0x94,
> >> +     .pwrc_rtc_sec_clk_en = 0x98,
> >> +     .pwrc_rtc_noc_clk_en = 0x9c,
> >> +};
> >> +
> >> +struct sirfsoc_pwrc_register sirfsoc_prima2_pwrc = {
> >> +     .pwrc_pdn_ctrl_set = 0x0,
> >> +     .pwrc_pon_status = 0x4,
> >> +     .pwrc_trigger_en_set = 0x8,
> >> +     .pwrc_int_status = 0xc,
> >> +     .pwrc_int_mask_set = 0x10,
> >> +     .pwrc_pin_status = 0x14,
> >> +     .pwrc_scratch_pad1 = 0x18,
> >> +     .pwrc_scratch_pad2 = 0x1c,
> >> +     .pwrc_scratch_pad3 = 0x20,
> >> +     .pwrc_scratch_pad4 = 0x24,
> >> +     .pwrc_scratch_pad5 = 0x28,
> >> +     .pwrc_scratch_pad6 = 0x2c,
> >> +     .pwrc_scratch_pad7 = 0x30,
> >> +     .pwrc_scratch_pad8 = 0x34,
> >> +     .pwrc_scratch_pad9 = 0x38,
> >> +     .pwrc_scratch_pad10 = 0x3c,
> >> +     .pwrc_scratch_pad11 = 0x40,
> >> +     .pwrc_scratch_pad12 = 0x44,
> >> +     .pwrc_gpio3_clk = 0x54,
> >> +     .pwrc_gpio_ds = 0x78,
> >> +};
> >
> > This is not the way we usually define register addresses.
> 
> i agree using MARCO is the general rules. but here moving to struct is
> for avoiding things like:
> 
> if (prima2...)
>    ...
> elif(atlas7)
>   ...
> 
> it is making registers offset private data which can be a member of the object.

Hmm... it's pretty unusual, but I can't think of a better way of doing
it yet.

[...]

> >> +static int sirfsoc_pwrc_probe(struct platform_device *pdev)
> >> +{
> >> +     struct device_node *np = pdev->dev.of_node;
> >> +     const struct of_device_id *match;
> >> +     struct sirfsoc_pwrc_info *pwrcinfo;
> >> +     struct regmap_irq_chip *regmap_irq_chip;
> >> +     struct sirfsoc_pwrc_register *pwrc_reg;
> >> +     struct regmap *map;
> >> +     int ret;
> >> +     u32 base;
> >> +
> >> +     if (of_property_read_u32(np, "reg", &base))
> >> +             panic("unable to find base address of pwrc node in dtb\n");
> >
> > It looks like this driver should depend on OF.
> >
> > Why are you obtaining the base address manually? Use:
> >
> >   res = platform_get_resource();
> >   devm_ioremap_resource(res);
> >
> > ... instead.
> 
> this was explained as they are not in memory space, they are behind a
> bus bridge.

Use 'ranges' in the DT, then you can pull out the proper address
without hand rolling your own method.

[...]

> >> +     regmap_irq_chip = &pwrc_irq_chip;
> >> +     pwrcinfo->regmap_irq_chip = regmap_irq_chip;
> >> +
> >> +     pwrc_reg = pwrcinfo->pwrc_reg;
> >> +     regmap_irq_chip->mask_base = pwrcinfo->base +
> >> +                                             pwrc_reg->pwrc_int_mask_set;
> >> +     regmap_irq_chip->unmask_base = pwrcinfo->base +
> >> +                             pwrc_reg->pwrc_int_mask_clr;
> >> +     regmap_irq_chip->status_base = pwrcinfo->base +
> >> +                                             pwrc_reg->pwrc_int_status;
> >> +     regmap_irq_chip->ack_base = pwrcinfo->base +
> >> +                                             pwrc_reg->pwrc_int_status;
> >
> > This is ugly.
> >
> > Better to create 2 regmap_irq_chip structures, one for each device.
> 
> there is only one device. why two regmap_irq_chip structures?
> 
> the driver is compatible with prima2 and atlas7, but any time there is
> only one of them,
> and the register needs to be adjust from dts and offset table.

Why does the 'base' offset have to be drawn from DT?  Does it change?

I think you should create two static regmap_irq_chip structures and do
only pass the relevant one to regmap.

See how everyone else does it.

[...]

> >> +static struct platform_driver sirfsoc_pwrc_driver = {
> >> +     .probe  = sirfsoc_pwrc_probe,
> >
> > .remove?
> >
> >> +     .driver = {
> >> +             .name = "sirfsoc_pwrc",
> >> +             .of_match_table = pwrc_ids,
> >
> > of_match_ptr()
> >
> >> +     },
> >> +};
> >> +module_platform_driver(sirfsoc_pwrc_driver);
> >
> > This isn't a module.
> 
> 
> so do you think it is still a platform, what is the best way to probe them?

Yes, it's still a platform.  It's just not a module.

[...]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog