| Message ID | 20201109163519.32605-2-zajec5@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On 11/9/2020 8:35 AM, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > Power Management Controller can be found on BCM4908 and is needed to > power on SoC blocks. It can be controlled using: > 1. Firmware calls > 2. Direct regs access > > Only the later method is supported by this initial driver. It was > successfully tested on BCM4908 using HCD controllers. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- [snip] > +#define PMBM_CTRL 0x00 > +#define PMC_PMBM_START (1 << 31) > +#define PMC_PMBM_TIMEOUT (1 << 30) > +#define PMC_PMBM_SLAVE_ERR (1 << 29) > +#define PMC_PMBM_BUSY (1 << 28) > +#define PMC_PMBM_READ (0 << 20) > +#define PMC_PMBM_WRITE (1 << 20) > +#define PMBM_WR_DATA 0x04 > +#define PMBM_TIMEOUT 0x08 > +#define PMBM_RD_DATA 0x0c Can you re-use the definitions from include/linux/reset/bcm63xx_pmb.h, you have the same constant names. > + > +#define BPCM_ID_REG 0x00 > +#define BPCM_CAPABILITIES 0x04 > +#define BPCM_CONTROL 0x08 > +#define BPCM_STATUS 0x0c > +#define BPCM_ROSC_CONTROL 0x10 > +#define BPCM_ROSC_THRESH_H 0x14 > +#define BPCM_ROSC_THRESHOLD_BCM6838 0x14 > +#define BPCM_ROSC_THRESH_S 0x18 > +#define BPCM_ROSC_COUNT_BCM6838 0x18 > +#define BPCM_ROSC_COUNT 0x1c > +#define BPCM_PWD_CONTROL_BCM6838 0x1c > +#define BPCM_PWD_CONTROL 0x20 > +#define BPCM_SR_CONTROL_BCM6838 0x20 > +#define BPCM_PWD_ACCUM_CONTROL 0x24 > +#define BPCM_SR_CONTROL 0x28 > +#define BPCM_GLOBAL_CONTROL 0x2c > +#define BPCM_MISC_CONTROL 0x30 > +#define BPCM_MISC_CONTROL2 0x34 > +#define BPCM_SGPHY_CNTL 0x38 > +#define BPCM_SGPHY_STATUS 0x3c > +#define BPCM_ZONE0 0x40 > +#define BPCM_ZONE_CONTROL 0x00 > +#define BPCM_ZONE_CONFIG1 0x04 > +#define BPCM_ZONE_CONFIG2 0x08 > +#define BPCM_ZONE_FREQ_SCALAR_CONTROL 0x0c > +#define BPCM_ZONE_SIZE 0x10 > + > +enum brcm_pmc_chipset { > + BCM4908, > + BCM6848, > + BCM6858, > +}; > + > +struct brcm_pmc_data { > + enum brcm_pmc_chipset chipset; > + int misc_strap_bus_reg; > + int misc_strap_bus_pmc_rom_boot_bit; > +}; > + > +static const struct brcm_pmc_data brcm_pmc_4908_data = { > + .chipset = BCM4908, > + .misc_strap_bus_reg = 0x00, > + .misc_strap_bus_pmc_rom_boot_bit = BIT(11), > +}; > + > +union bpcm_capabilities { > + struct { > + int num_zones:8; > + int sr_reg_bits:8; > + int pllType:2; > + int reserved0:1; > + int ubus:1; > + int reserved1:12; > + } bits; > + u32 raw32; > +} __attribute__((__packed__)); This is not going to work well if we extend this driver to support the BCM63xx DSL SoCs which are done by the same engineering group within Broadcom. Can you convert that to bits/masks instead such that this becomes endian independent? > + > +union bpcm_zone_control { > + struct { > + u32 manual_clk_en:1; > + u32 manual_reset_ctl:1; > + u32 freq_scale_used:1; /* R/O */ > + u32 dpg_capable:1; /* R/O */ > + u32 manual_mem_pwr:2; > + u32 manual_iso_ctl:1; > + u32 manual_ctl:1; > + u32 dpg_ctl_en:1; > + u32 pwr_dn_req:1; > + u32 pwr_up_req:1; > + u32 mem_pwr_ctl_en:1; > + u32 blk_reset_assert:1; > + u32 mem_stby:1; > + u32 reserved:5; > + u32 pwr_cntl_state:5; > + u32 freq_scalar_dyn_sel:1; /* R/O */ > + u32 pwr_off_state:1; /* R/O */ > + u32 pwr_on_state:1; /* R/O */ > + u32 pwr_good:1; /* R/O */ > + u32 dpg_pwr_state:1; /* R/O */ > + u32 mem_pwr_state:1; /* R/O */ > + u32 iso_state:1; /* R/O */ > + u32 reset_state:1; /* R/O */ > + } bits; > + u32 raw32; > +} __attribute__((__packed__)); > + > +struct brcm_pmc { > + const struct brcm_pmc_data *data; > + void __iomem *base; > + struct regmap *misc; > + struct regmap *procmon; > + struct device *dev; > + struct reset_controller_dev rcdev; > + bool direct; > + bool little_endian; > +}; > + > +static DEFINE_SPINLOCK(lock); You have a platform device, can you store the lock there instead of having a global? > + > +#define cpu_to_dev32(pmc, val) \ > + ((pmc)->little_endian ? cpu_to_le32(val) : cpu_to_be32(val)) This would probably be better written as a static inline helper, and maybe something like this: static inline u32 brcm_pmc_readl(struct brcm_pmc *pmc, u8 offset) { if (pmc->little_endian) return ioread32(pmc->base + offset); else return ioread32_be(pmc->base + offset); } > + > +#define dev32_to_cpu(pmc, val) \ > + ((pmc)->little_endian ? le32_to_cpu(val) : be32_to_cpu(val)) > + > +static int brcm_pmc_bpcm_wait(struct brcm_pmc *pmc, int bus) > +{ > + unsigned long deadline = jiffies + usecs_to_jiffies(1000); > + int pmbm = PROCMON_PMBM + bus * PROCMON_PMBM_SIZE; > + u32 val; > + > + do { > + regmap_read(pmc->procmon, pmbm + PMBM_CTRL, &val); > + if (!(val & PMC_PMBM_START)) > + return 0; > + cpu_relax(); > + udelay(10); > + } while (!time_after_eq(jiffies, deadline)); > + > + dev_err(pmc->dev, "Timeout waiting for the BPCM\n"); Can you use include/linux/reset/bcm63xx_pmb.h again here and check arch/arm/mach-bcm/bcm63xx_pmb.c for how it is used. > + > + return -EBUSY; > +} > + > +static int brcm_pmc_bpcm_read(struct brcm_pmc *pmc, u16 addr, int word_offset, u32 *val) > +{ > + int err = 0; > + > + if (!pmc->direct) { > + err = -EOPNOTSUPP; Do an early return here so you don't have to indent the block under by one tab. > + } else { > + u8 device = addr & 0xff; > + u8 bus = addr >> 8; > + unsigned long flags; > + int pmbm; > + u32 tmp; > + > + pmbm = PROCMON_PMBM + bus * PROCMON_PMBM_SIZE; > + > + spin_lock_irqsave(&lock, flags); > + > + /* TODO: Make sure PMBM is not busy */ > + > + regmap_write(pmc->procmon, pmbm + PMBM_CTRL, > + PMC_PMBM_START | PMC_PMBM_READ | (device << 12) | word_offset); > + > + err = brcm_pmc_bpcm_wait(pmc, bus); > + if (!err) > + err = regmap_read(pmc->procmon, pmbm + PMBM_RD_DATA, val); > + > + spin_unlock_irqrestore(&lock, flags); > + } > + > + return err; > +} > + > +static int brcm_pmc_bpcm_write(struct brcm_pmc *pmc, u16 addr, int word_offset, u32 val) > +{ > + int err = 0; > + > + if (!pmc->direct) { > + err = -EOPNOTSUPP; Likewise. [snip] > +static int brcm_pmc_reset_xlate(struct reset_controller_dev *rcdev, > + const struct of_phandle_args *reset_spec) > +{ > + u8 device = reset_spec->args[1]; > + u8 bus = reset_spec->args[0]; > + > + if (bus > 1) > + return -EINVAL; > + > + return (bus << 8) | device; IIRC the bus is limited to 8 bits, and the device ID certainly is limited to 8 bits so you need to validate that before doing the assignment otherwise you are going to see truncated values which could be wrapped around and return some bogus reset ID combination. You are using the same binding as Documentation/devicetree/bindings/reset/brcm,bcm63138-pmb.txt, so maybe you can add that compatible string into your binding document and deprecate this old binding file. Other than those, this looks great to me, thanks!
On 10.11.2020 05:52, Florian Fainelli wrote: > On 11/9/2020 8:35 AM, Rafał Miłecki wrote: >> From: Rafał Miłecki <rafal@milecki.pl> >> >> Power Management Controller can be found on BCM4908 and is needed to >> power on SoC blocks. It can be controlled using: >> 1. Firmware calls >> 2. Direct regs access >> >> Only the later method is supported by this initial driver. It was >> successfully tested on BCM4908 using HCD controllers. >> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >> --- > > [snip] > >> +#define PMBM_CTRL 0x00 >> +#define PMC_PMBM_START (1 << 31) >> +#define PMC_PMBM_TIMEOUT (1 << 30) >> +#define PMC_PMBM_SLAVE_ERR (1 << 29) >> +#define PMC_PMBM_BUSY (1 << 28) >> +#define PMC_PMBM_READ (0 << 20) >> +#define PMC_PMBM_WRITE (1 << 20) >> +#define PMBM_WR_DATA 0x04 >> +#define PMBM_TIMEOUT 0x08 >> +#define PMBM_RD_DATA 0x0c > > Can you re-use the definitions from include/linux/reset/bcm63xx_pmb.h, > you have the same constant names. > >> + >> +#define BPCM_ID_REG 0x00 >> +#define BPCM_CAPABILITIES 0x04 >> +#define BPCM_CONTROL 0x08 >> +#define BPCM_STATUS 0x0c >> +#define BPCM_ROSC_CONTROL 0x10 >> +#define BPCM_ROSC_THRESH_H 0x14 >> +#define BPCM_ROSC_THRESHOLD_BCM6838 0x14 >> +#define BPCM_ROSC_THRESH_S 0x18 >> +#define BPCM_ROSC_COUNT_BCM6838 0x18 >> +#define BPCM_ROSC_COUNT 0x1c >> +#define BPCM_PWD_CONTROL_BCM6838 0x1c >> +#define BPCM_PWD_CONTROL 0x20 >> +#define BPCM_SR_CONTROL_BCM6838 0x20 >> +#define BPCM_PWD_ACCUM_CONTROL 0x24 >> +#define BPCM_SR_CONTROL 0x28 >> +#define BPCM_GLOBAL_CONTROL 0x2c >> +#define BPCM_MISC_CONTROL 0x30 >> +#define BPCM_MISC_CONTROL2 0x34 >> +#define BPCM_SGPHY_CNTL 0x38 >> +#define BPCM_SGPHY_STATUS 0x3c >> +#define BPCM_ZONE0 0x40 >> +#define BPCM_ZONE_CONTROL 0x00 >> +#define BPCM_ZONE_CONFIG1 0x04 >> +#define BPCM_ZONE_CONFIG2 0x08 >> +#define BPCM_ZONE_FREQ_SCALAR_CONTROL 0x0c >> +#define BPCM_ZONE_SIZE 0x10 >> + >> +enum brcm_pmc_chipset { >> + BCM4908, >> + BCM6848, >> + BCM6858, >> +}; >> + >> +struct brcm_pmc_data { >> + enum brcm_pmc_chipset chipset; >> + int misc_strap_bus_reg; >> + int misc_strap_bus_pmc_rom_boot_bit; >> +}; >> + >> +static const struct brcm_pmc_data brcm_pmc_4908_data = { >> + .chipset = BCM4908, >> + .misc_strap_bus_reg = 0x00, >> + .misc_strap_bus_pmc_rom_boot_bit = BIT(11), >> +}; >> + >> +union bpcm_capabilities { >> + struct { >> + int num_zones:8; >> + int sr_reg_bits:8; >> + int pllType:2; >> + int reserved0:1; >> + int ubus:1; >> + int reserved1:12; >> + } bits; >> + u32 raw32; >> +} __attribute__((__packed__)); > > This is not going to work well if we extend this driver to support the > BCM63xx DSL SoCs which are done by the same engineering group within > Broadcom. Can you convert that to bits/masks instead such that this > becomes endian independent? > >> + >> +union bpcm_zone_control { >> + struct { >> + u32 manual_clk_en:1; >> + u32 manual_reset_ctl:1; >> + u32 freq_scale_used:1; /* R/O */ >> + u32 dpg_capable:1; /* R/O */ >> + u32 manual_mem_pwr:2; >> + u32 manual_iso_ctl:1; >> + u32 manual_ctl:1; >> + u32 dpg_ctl_en:1; >> + u32 pwr_dn_req:1; >> + u32 pwr_up_req:1; >> + u32 mem_pwr_ctl_en:1; >> + u32 blk_reset_assert:1; >> + u32 mem_stby:1; >> + u32 reserved:5; >> + u32 pwr_cntl_state:5; >> + u32 freq_scalar_dyn_sel:1; /* R/O */ >> + u32 pwr_off_state:1; /* R/O */ >> + u32 pwr_on_state:1; /* R/O */ >> + u32 pwr_good:1; /* R/O */ >> + u32 dpg_pwr_state:1; /* R/O */ >> + u32 mem_pwr_state:1; /* R/O */ >> + u32 iso_state:1; /* R/O */ >> + u32 reset_state:1; /* R/O */ >> + } bits; >> + u32 raw32; >> +} __attribute__((__packed__)); >> + >> +struct brcm_pmc { >> + const struct brcm_pmc_data *data; >> + void __iomem *base; >> + struct regmap *misc; >> + struct regmap *procmon; >> + struct device *dev; >> + struct reset_controller_dev rcdev; >> + bool direct; >> + bool little_endian; >> +}; >> + >> +static DEFINE_SPINLOCK(lock); > > You have a platform device, can you store the lock there instead of > having a global? > >> + >> +#define cpu_to_dev32(pmc, val) \ >> + ((pmc)->little_endian ? cpu_to_le32(val) : cpu_to_be32(val)) > > This would probably be better written as a static inline helper, and > maybe something like this: > > static inline u32 brcm_pmc_readl(struct brcm_pmc *pmc, u8 offset) > { > if (pmc->little_endian) > return ioread32(pmc->base + offset); > else > return ioread32_be(pmc->base + offset); > } > >> + >> +#define dev32_to_cpu(pmc, val) \ >> + ((pmc)->little_endian ? le32_to_cpu(val) : be32_to_cpu(val)) >> + >> +static int brcm_pmc_bpcm_wait(struct brcm_pmc *pmc, int bus) >> +{ >> + unsigned long deadline = jiffies + usecs_to_jiffies(1000); >> + int pmbm = PROCMON_PMBM + bus * PROCMON_PMBM_SIZE; >> + u32 val; >> + >> + do { >> + regmap_read(pmc->procmon, pmbm + PMBM_CTRL, &val); >> + if (!(val & PMC_PMBM_START)) >> + return 0; >> + cpu_relax(); >> + udelay(10); >> + } while (!time_after_eq(jiffies, deadline)); >> + >> + dev_err(pmc->dev, "Timeout waiting for the BPCM\n"); > > Can you use include/linux/reset/bcm63xx_pmb.h again here and check > arch/arm/mach-bcm/bcm63xx_pmb.c for how it is used. I use regmap while bcm63xx_pmb.h depends on raw IO access. See below though. >> + return -EBUSY; >> +} >> + >> +static int brcm_pmc_bpcm_read(struct brcm_pmc *pmc, u16 addr, int word_offset, u32 *val) >> +{ >> + int err = 0; >> + >> + if (!pmc->direct) { >> + err = -EOPNOTSUPP; > > Do an early return here so you don't have to indent the block under by > one tab. In a long term I'll need a full if + else anyway. Having it this way now will save me some code moving (indent) later. >> + } else { >> + u8 device = addr & 0xff; >> + u8 bus = addr >> 8; >> + unsigned long flags; >> + int pmbm; >> + u32 tmp; >> + >> + pmbm = PROCMON_PMBM + bus * PROCMON_PMBM_SIZE; >> + >> + spin_lock_irqsave(&lock, flags); >> + >> + /* TODO: Make sure PMBM is not busy */ >> + >> + regmap_write(pmc->procmon, pmbm + PMBM_CTRL, >> + PMC_PMBM_START | PMC_PMBM_READ | (device << 12) | word_offset); >> + >> + err = brcm_pmc_bpcm_wait(pmc, bus); >> + if (!err) >> + err = regmap_read(pmc->procmon, pmbm + PMBM_RD_DATA, val); >> + >> + spin_unlock_irqrestore(&lock, flags); >> + } >> + >> + return err; >> +} >> + >> +static int brcm_pmc_bpcm_write(struct brcm_pmc *pmc, u16 addr, int word_offset, u32 val) >> +{ >> + int err = 0; >> + >> + if (!pmc->direct) { >> + err = -EOPNOTSUPP; > > Likewise. > > [snip] > >> +static int brcm_pmc_reset_xlate(struct reset_controller_dev *rcdev, >> + const struct of_phandle_args *reset_spec) >> +{ >> + u8 device = reset_spec->args[1]; >> + u8 bus = reset_spec->args[0]; >> + >> + if (bus > 1) >> + return -EINVAL; >> + >> + return (bus << 8) | device; > > IIRC the bus is limited to 8 bits, and the device ID certainly is > limited to 8 bits so you need to validate that before doing the > assignment otherwise you are going to see truncated values which could > be wrapped around and return some bogus reset ID combination. > > You are using the same binding as > Documentation/devicetree/bindings/reset/brcm,bcm63138-pmb.txt, so maybe > you can add that compatible string into your binding document and > deprecate this old binding file. Looking at arch/arm/mach-bcm/bcm63xx_pmb.c made me realize this design may be actually more complex than that. *** PMB *** It seems like PMB Master is reset controller on its own. Even though we don't have actual driver for the (documented) "brcm,bcm63138-pmb" binding, arch code treates it like a reset. So it seems that a single PMB is capable at least of: 1. Resetting ARM CPU core (id by 8b addr) 2. Resetting devices (id by 8b addr) by (en|dis)abling zones Above operations are performed in a different (programming) ways. *** PMC *** PMC seems to be a reset controller with firmware capable at least of: 1. Resetting devices (id by 8b addr) by (en|dis)abling zones that fallback to using PMB blocks when needed / required. *** Bindings *** So I started wondering if following bindings were not more applicable: pmb0: reset-controller@4800c0 { compatible = "brcm,bcm4908-pmb"; reg = <0x802800c0 0x10>; #reset-cells = <2>; }; pmb1: reset-controller@4800e0 { compatible = "brcm,bcm4908-pmb"; reg = <0x802800e0 0x10>; #reset-cells = <2>; }; pmc: reset-controller@80200000 { compatible = "brcm,bcm4908-pmc"; reg = <0x80200000 0x5000>; brcm,syscon-misc = <&misc>; pmb-resets = <BUS_ID_0 &pmb0>, <BUS_ID_1 &pmb1>; #reset-cells = <2>; }; cpu@1 { resets = <&pmb0 PMB_ARM 4>; /* ARM CPU */ } foo { resets = <&pmb1 PMB_DEVICE 17>; /* Reset USB using PMB directly */ } foo { resets = <&pmc BUS_ID_1 17>; /* Reset USB using PMC */ } Then we would have: 1. PMB driver handling PMB_ARM and PMB_DEVICE with 2 different code paths 2. PMC driver falling back to PMB when needed / required Florian: can you have an extra look at it please and review proposed design, comment on it?
On 12.11.2020 15:55, Rafał Miłecki wrote: > *** PMB *** > > It seems like PMB Master is reset controller on its own. Even though we don't > have actual driver for the (documented) "brcm,bcm63138-pmb" binding, arch code > treates it like a reset. > > So it seems that a single PMB is capable at least of: > 1. Resetting ARM CPU core (id by 8b addr) > 2. Resetting devices (id by 8b addr) by (en|dis)abling zones > > Above operations are performed in a different (programming) ways. > > > *** PMC *** > > PMC seems to be a reset controller with firmware capable at least of: > 1. Resetting devices (id by 8b addr) by (en|dis)abling zones > > that fallback to using PMB blocks when needed / required. This just got more complex as I started playing with PMC / PMB and PCIe controller. 1. BPCM_CAPABILITIES reg for PCIe controller reports 0 num_zones 2. Enabling PCIe requires powering on zone 0 manually 3. After powering on zone, PCIe requires setting SR_CONTROL It means that PMB driver (and so PMC one as it fallback to the PMB) needs to know what type of device we're addressing. Anything simple like: resets = <&pmb0 14>; resets = <&pmc 1 14>; won't work (unless we hardcode ids in driver - which is unwanted for DTS). So I guess that after all we'll need something like: cpu@1 { resets = <&pmb0 PMB_ARM 4>; /* ARM CPU */ } foo { resets = <&pmb1 PMB_USB_DEVICE 17>; /* Reset USB using PMB directly */ } bar { resets = <&pmb1 PMB_PCIE_DEVICE 15>; /* Reset PCIE using PMB directly */ } and also qux { resets = <&pmc PMB_PCIE_DEVICE 1 15>; /* Reset PCIe using PMC */ }
On 11/17/20 10:44 AM, Rafał Miłecki wrote: > On 12.11.2020 15:55, Rafał Miłecki wrote: >> *** PMB *** >> >> It seems like PMB Master is reset controller on its own. Even though >> we don't >> have actual driver for the (documented) "brcm,bcm63138-pmb" binding, >> arch code >> treates it like a reset. >> >> So it seems that a single PMB is capable at least of: >> 1. Resetting ARM CPU core (id by 8b addr) >> 2. Resetting devices (id by 8b addr) by (en|dis)abling zones >> >> Above operations are performed in a different (programming) ways. >> >> >> *** PMC *** >> >> PMC seems to be a reset controller with firmware capable at least of: >> 1. Resetting devices (id by 8b addr) by (en|dis)abling zones >> >> that fallback to using PMB blocks when needed / required. > > This just got more complex as I started playing with PMC / PMB and PCIe > controller. > > 1. BPCM_CAPABILITIES reg for PCIe controller reports 0 num_zones > 2. Enabling PCIe requires powering on zone 0 manually > 3. After powering on zone, PCIe requires setting SR_CONTROL > > It means that PMB driver (and so PMC one as it fallback to the PMB) > needs to > know what type of device we're addressing. From prior attempts at getting the 63138 supported which was a fairly early silicon revision with a mix of PMB and PMC ultimately, the reset driver does need to know what kind of device ID is being reset/powered on. I stopped after seeing that SATA, USB, PCIe, Switch would all need to be treated differently, and the board I had should have been upgraded to a B0: https://github.com/ffainelli/linux/commit/50f1dfb17b624ee41a11dda01bc899e6756869b7 There are way too many quirks for Device Tree to express all of them, and the whole point of the BPCM was to design a re-usable and self-discoverable power/reset/clocking module. As you found out, if the capabilities are wrong, it defeats the purpose. > > Anything simple like: > > resets = <&pmb0 14>; > resets = <&pmc 1 14>; > > won't work (unless we hardcode ids in driver - which is unwanted for DTS). > > > So I guess that after all we'll need something like: > > cpu@1 { > resets = <&pmb0 PMB_ARM 4>; /* ARM CPU */ > } > > foo { > resets = <&pmb1 PMB_USB_DEVICE 17>; /* Reset USB using PMB directly */ > } > > bar { > resets = <&pmb1 PMB_PCIE_DEVICE 15>; /* Reset PCIE using PMB > directly */ > } What is the second reset cell mapping to? Maybe you can define a C preprocessor macro that expressed both the PMB bus + device ID? > > and also > > qux { > resets = <&pmc PMB_PCIE_DEVICE 1 15>; /* Reset PCIe using PMC */ > } -- Florian
On 17.11.2020 19:55, Florian Fainelli wrote: > On 11/17/20 10:44 AM, Rafał Miłecki wrote: >> On 12.11.2020 15:55, Rafał Miłecki wrote: >>> *** PMB *** >>> >>> It seems like PMB Master is reset controller on its own. Even though >>> we don't >>> have actual driver for the (documented) "brcm,bcm63138-pmb" binding, >>> arch code >>> treates it like a reset. >>> >>> So it seems that a single PMB is capable at least of: >>> 1. Resetting ARM CPU core (id by 8b addr) >>> 2. Resetting devices (id by 8b addr) by (en|dis)abling zones >>> >>> Above operations are performed in a different (programming) ways. >>> >>> >>> *** PMC *** >>> >>> PMC seems to be a reset controller with firmware capable at least of: >>> 1. Resetting devices (id by 8b addr) by (en|dis)abling zones >>> >>> that fallback to using PMB blocks when needed / required. >> >> This just got more complex as I started playing with PMC / PMB and PCIe >> controller. >> >> 1. BPCM_CAPABILITIES reg for PCIe controller reports 0 num_zones >> 2. Enabling PCIe requires powering on zone 0 manually >> 3. After powering on zone, PCIe requires setting SR_CONTROL >> >> It means that PMB driver (and so PMC one as it fallback to the PMB) >> needs to >> know what type of device we're addressing. > > From prior attempts at getting the 63138 supported which was a fairly > early silicon revision with a mix of PMB and PMC ultimately, the reset > driver does need to know what kind of device ID is being reset/powered > on. I stopped after seeing that SATA, USB, PCIe, Switch would all need > to be treated differently, and the board I had should have been upgraded > to a B0: > > https://github.com/ffainelli/linux/commit/50f1dfb17b624ee41a11dda01bc899e6756869b7 > > There are way too many quirks for Device Tree to express all of them, > and the whole point of the BPCM was to design a re-usable and > self-discoverable power/reset/clocking module. As you found out, if the > capabilities are wrong, it defeats the purpose. Good to know we share conclusions. The problem with PMC (except having to support firmware / ucode / you call it) is it's still not capable of handling everything. As mentioned earlier, powering up PCIe controller can be done by PMC and powering up zone 0 BUT it still requires setting that SR_CTRL. I see you ended up hardcoding IDs in the bcm63138-pmb.c . I still hope I can get something slightly more generic. >> Anything simple like: >> >> resets = <&pmb0 14>; >> resets = <&pmc 1 14>; >> >> won't work (unless we hardcode ids in driver - which is unwanted for DTS). >> >> >> So I guess that after all we'll need something like: >> >> cpu@1 { >> resets = <&pmb0 PMB_ARM 4>; /* ARM CPU */ >> } >> >> foo { >> resets = <&pmb1 PMB_USB_DEVICE 17>; /* Reset USB using PMB directly */ >> } >> >> bar { >> resets = <&pmb1 PMB_PCIE_DEVICE 15>; /* Reset PCIE using PMB >> directly */ >> } > > What is the second reset cell mapping to? Maybe you can define a C > preprocessor macro that expressed both the PMB bus + device ID? Device ID. Just like 4 in the : #define PMB_ADDR_AIP (4 | PMB_BUS_AIP << PMB_BUS_ID_SHIFT) So for BCM4908 I could have: resets = <&pmb1 PMB_PCIE_DEVICE 14>; /* PCIe 0 */ resets = <&pmb1 PMB_PCIE_DEVICE 15>; /* PCIe 1 */ Do you mean C macro for combining two values? Sure, I can do that, but is it worth it, just to save n * 32 b of .dtb data? >> and also >> >> qux { >> resets = <&pmc PMB_PCIE_DEVICE 1 15>; /* Reset PCIe using PMC */ >> } > >
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig index 84baec01aa30..d27104e7fa24 100644 --- a/drivers/reset/Kconfig +++ b/drivers/reset/Kconfig @@ -41,6 +41,14 @@ config RESET_BERLIN help This enables the reset controller driver for Marvell Berlin SoCs. +config RESET_BRCM_PMC + tristate "Broadcom PMC reset controller" + depends on ARCH_BCM4908 || COMPILE_TEST + default ARCH_BCM4908 + help + This enables the Broadcom PMC (Power Management Controller) reset + controller driver. + config RESET_BRCMSTB tristate "Broadcom STB reset controller" depends on ARCH_BRCMSTB || COMPILE_TEST diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile index 16947610cc3b..bbab031e9f19 100644 --- a/drivers/reset/Makefile +++ b/drivers/reset/Makefile @@ -7,6 +7,7 @@ obj-$(CONFIG_RESET_A10SR) += reset-a10sr.o obj-$(CONFIG_RESET_ATH79) += reset-ath79.o obj-$(CONFIG_RESET_AXS10X) += reset-axs10x.o obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o +obj-$(CONFIG_RESET_BRCM_PMC) += reset-brcm-pmc.o obj-$(CONFIG_RESET_BRCMSTB) += reset-brcmstb.o obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o diff --git a/drivers/reset/reset-brcm-pmc.c b/drivers/reset/reset-brcm-pmc.c new file mode 100644 index 000000000000..6ab31f5d026c --- /dev/null +++ b/drivers/reset/reset-brcm-pmc.c @@ -0,0 +1,415 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2013 Broadcom + * Copyright (C) 2020 Rafał Miłecki <rafal@milecki.pl> + */ + +#include <linux/mfd/syscon.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/reset-controller.h> + +#define PROCMON_MONITORCTRL 0x00 +#define PROCMON_ROSC 0x20 +#define PROCMON_MISC 0x40 +#define PROCMON_SSBMASTER 0x60 +#define PROCMON_ECTR 0x80 +#define PROCMON_PMBM 0xc0 /* PMB Master bus 0 */ +#define PROCMON_PMBM_SIZE 0x20 +#define PROCMON_PMBM1 0xe0 /* PMB Master bus 1 */ + +#define PMBM_CTRL 0x00 +#define PMC_PMBM_START (1 << 31) +#define PMC_PMBM_TIMEOUT (1 << 30) +#define PMC_PMBM_SLAVE_ERR (1 << 29) +#define PMC_PMBM_BUSY (1 << 28) +#define PMC_PMBM_READ (0 << 20) +#define PMC_PMBM_WRITE (1 << 20) +#define PMBM_WR_DATA 0x04 +#define PMBM_TIMEOUT 0x08 +#define PMBM_RD_DATA 0x0c + +#define BPCM_ID_REG 0x00 +#define BPCM_CAPABILITIES 0x04 +#define BPCM_CONTROL 0x08 +#define BPCM_STATUS 0x0c +#define BPCM_ROSC_CONTROL 0x10 +#define BPCM_ROSC_THRESH_H 0x14 +#define BPCM_ROSC_THRESHOLD_BCM6838 0x14 +#define BPCM_ROSC_THRESH_S 0x18 +#define BPCM_ROSC_COUNT_BCM6838 0x18 +#define BPCM_ROSC_COUNT 0x1c +#define BPCM_PWD_CONTROL_BCM6838 0x1c +#define BPCM_PWD_CONTROL 0x20 +#define BPCM_SR_CONTROL_BCM6838 0x20 +#define BPCM_PWD_ACCUM_CONTROL 0x24 +#define BPCM_SR_CONTROL 0x28 +#define BPCM_GLOBAL_CONTROL 0x2c +#define BPCM_MISC_CONTROL 0x30 +#define BPCM_MISC_CONTROL2 0x34 +#define BPCM_SGPHY_CNTL 0x38 +#define BPCM_SGPHY_STATUS 0x3c +#define BPCM_ZONE0 0x40 +#define BPCM_ZONE_CONTROL 0x00 +#define BPCM_ZONE_CONFIG1 0x04 +#define BPCM_ZONE_CONFIG2 0x08 +#define BPCM_ZONE_FREQ_SCALAR_CONTROL 0x0c +#define BPCM_ZONE_SIZE 0x10 + +enum brcm_pmc_chipset { + BCM4908, + BCM6848, + BCM6858, +}; + +struct brcm_pmc_data { + enum brcm_pmc_chipset chipset; + int misc_strap_bus_reg; + int misc_strap_bus_pmc_rom_boot_bit; +}; + +static const struct brcm_pmc_data brcm_pmc_4908_data = { + .chipset = BCM4908, + .misc_strap_bus_reg = 0x00, + .misc_strap_bus_pmc_rom_boot_bit = BIT(11), +}; + +union bpcm_capabilities { + struct { + int num_zones:8; + int sr_reg_bits:8; + int pllType:2; + int reserved0:1; + int ubus:1; + int reserved1:12; + } bits; + u32 raw32; +} __attribute__((__packed__)); + +union bpcm_zone_control { + struct { + u32 manual_clk_en:1; + u32 manual_reset_ctl:1; + u32 freq_scale_used:1; /* R/O */ + u32 dpg_capable:1; /* R/O */ + u32 manual_mem_pwr:2; + u32 manual_iso_ctl:1; + u32 manual_ctl:1; + u32 dpg_ctl_en:1; + u32 pwr_dn_req:1; + u32 pwr_up_req:1; + u32 mem_pwr_ctl_en:1; + u32 blk_reset_assert:1; + u32 mem_stby:1; + u32 reserved:5; + u32 pwr_cntl_state:5; + u32 freq_scalar_dyn_sel:1; /* R/O */ + u32 pwr_off_state:1; /* R/O */ + u32 pwr_on_state:1; /* R/O */ + u32 pwr_good:1; /* R/O */ + u32 dpg_pwr_state:1; /* R/O */ + u32 mem_pwr_state:1; /* R/O */ + u32 iso_state:1; /* R/O */ + u32 reset_state:1; /* R/O */ + } bits; + u32 raw32; +} __attribute__((__packed__)); + +struct brcm_pmc { + const struct brcm_pmc_data *data; + void __iomem *base; + struct regmap *misc; + struct regmap *procmon; + struct device *dev; + struct reset_controller_dev rcdev; + bool direct; + bool little_endian; +}; + +static DEFINE_SPINLOCK(lock); + +#define cpu_to_dev32(pmc, val) \ + ((pmc)->little_endian ? cpu_to_le32(val) : cpu_to_be32(val)) + +#define dev32_to_cpu(pmc, val) \ + ((pmc)->little_endian ? le32_to_cpu(val) : be32_to_cpu(val)) + +static int brcm_pmc_bpcm_wait(struct brcm_pmc *pmc, int bus) +{ + unsigned long deadline = jiffies + usecs_to_jiffies(1000); + int pmbm = PROCMON_PMBM + bus * PROCMON_PMBM_SIZE; + u32 val; + + do { + regmap_read(pmc->procmon, pmbm + PMBM_CTRL, &val); + if (!(val & PMC_PMBM_START)) + return 0; + cpu_relax(); + udelay(10); + } while (!time_after_eq(jiffies, deadline)); + + dev_err(pmc->dev, "Timeout waiting for the BPCM\n"); + + return -EBUSY; +} + +static int brcm_pmc_bpcm_read(struct brcm_pmc *pmc, u16 addr, int word_offset, u32 *val) +{ + int err = 0; + + if (!pmc->direct) { + err = -EOPNOTSUPP; + } else { + u8 device = addr & 0xff; + u8 bus = addr >> 8; + unsigned long flags; + int pmbm; + u32 tmp; + + pmbm = PROCMON_PMBM + bus * PROCMON_PMBM_SIZE; + + spin_lock_irqsave(&lock, flags); + + /* TODO: Make sure PMBM is not busy */ + + regmap_write(pmc->procmon, pmbm + PMBM_CTRL, + PMC_PMBM_START | PMC_PMBM_READ | (device << 12) | word_offset); + + err = brcm_pmc_bpcm_wait(pmc, bus); + if (!err) + err = regmap_read(pmc->procmon, pmbm + PMBM_RD_DATA, val); + + spin_unlock_irqrestore(&lock, flags); + } + + return err; +} + +static int brcm_pmc_bpcm_write(struct brcm_pmc *pmc, u16 addr, int word_offset, u32 val) +{ + int err = 0; + + if (!pmc->direct) { + err = -EOPNOTSUPP; + } else { + u8 device = addr & 0xff; + u8 bus = addr >> 8; + unsigned long flags; + int pmbm; + u32 tmp; + int err = 0; + + pmbm = PROCMON_PMBM + bus * PROCMON_PMBM_SIZE; + + spin_lock_irqsave(&lock, flags); + + /* TODO: Make sure PMBM is not busy */ + + regmap_write(pmc->procmon, pmbm + PMBM_WR_DATA, val); + regmap_write(pmc->procmon, pmbm + PMBM_CTRL, + PMC_PMBM_START | PMC_PMBM_WRITE | (device << 12) | word_offset); + + err = brcm_pmc_bpcm_wait(pmc, bus); + + spin_unlock_irqrestore(&lock, flags); + } + + return err; +} + +static int brcm_pmc_power_on_zone(struct brcm_pmc *pmc, u16 addr, int zone) +{ + int err; + + if ((pmc->data->chipset == BCM6848 || pmc->data->chipset == BCM6858) && !pmc->direct) { + err = -EOPNOTSUPP; + } else { + union bpcm_zone_control control; + int offset; + int err; + + offset = BPCM_ZONE0 + zone * BPCM_ZONE_SIZE + BPCM_ZONE_CONTROL; + + err = brcm_pmc_bpcm_read(pmc, addr, offset >> 2, (u32 *)&control.raw32); + if (err) + return err; + control.raw32 = dev32_to_cpu(pmc, control.raw32); + + if (control.bits.pwr_on_state == 0) { + control.bits.pwr_dn_req = 0; + control.bits.dpg_ctl_en = 1; + control.bits.pwr_up_req = 1; + control.bits.mem_pwr_ctl_en = 1; + control.bits.blk_reset_assert = 1; + + control.raw32 = cpu_to_dev32(pmc, control.raw32); + err = brcm_pmc_bpcm_write(pmc, addr, offset >> 2, control.raw32); + } + } + + return err; +} + +static int brcm_pmc_power_off_device(struct brcm_pmc *pmc, u16 addr) +{ + int err; + + if (!pmc->direct) { + err = -EOPNOTSUPP; + } else { + /* Entire device can be powered off by powering off the 0th zone */ + union bpcm_zone_control control; + int offset; + int err; + + offset = BPCM_ZONE0 + BPCM_ZONE_CONTROL; + + err = brcm_pmc_bpcm_read(pmc, addr, offset >> 2, (u32 *)&control.raw32); + if (err) + return err; + control.raw32 = dev32_to_cpu(pmc, control.raw32); + + if (!control.bits.pwr_off_state) { + control.bits.pwr_dn_req = 1; + + control.raw32 = cpu_to_dev32(pmc, control.raw32); + err = brcm_pmc_bpcm_write(pmc, addr, offset >> 2, control.raw32); + } + } + + return err; +} + +static int brcm_pmc_power_on_device(struct brcm_pmc *pmc, u16 addr) +{ + int err; + + if (!pmc->direct) { + err = -EOPNOTSUPP; + } else { + union bpcm_capabilities caps; + int offset; + int err; + int i; + + offset = BPCM_CAPABILITIES; + + err = brcm_pmc_bpcm_read(pmc, addr, offset >> 2, (u32 *)&caps.raw32); + if (err) + return err; + caps.raw32 = dev32_to_cpu(pmc, caps.raw32); + + for (i = 0; i < caps.bits.num_zones; i++) { + err = brcm_pmc_power_on_zone(pmc, addr, i); + if (err) + return err; + } + } + + return err; +} + +static int brcm_pmc_assert(struct reset_controller_dev *rcdev, unsigned long id) +{ + struct brcm_pmc *pmc = container_of(rcdev, struct brcm_pmc, rcdev); + + return brcm_pmc_power_off_device(pmc, id); +} + +static int brcm_pmc_deassert(struct reset_controller_dev *rcdev, unsigned long id) +{ + struct brcm_pmc *pmc = container_of(rcdev, struct brcm_pmc, rcdev); + + return brcm_pmc_power_on_device(pmc, id); +} + +static int brcm_pmc_reset_xlate(struct reset_controller_dev *rcdev, + const struct of_phandle_args *reset_spec) +{ + u8 device = reset_spec->args[1]; + u8 bus = reset_spec->args[0]; + + if (bus > 1) + return -EINVAL; + + return (bus << 8) | device; +} + +static const struct reset_control_ops brcm_pmc_reset_control_ops = { + .assert = brcm_pmc_assert, + .deassert = brcm_pmc_deassert, +}; + +static const struct of_device_id brcm_pmc_reset_of_match[] = { + { .compatible = "brcm,bcm4908-pmc", .data = &brcm_pmc_4908_data, }, + { }, +}; + +static int brcm_pmc_reset_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct brcm_pmc *pmc; + struct resource *res; + unsigned int val; + int err; + + pmc = devm_kzalloc(dev, sizeof(*pmc), GFP_KERNEL); + if (!pmc) + return -ENOMEM; + + pmc->data = of_device_get_match_data(dev); + if (!pmc->data) { + dev_err(dev, "Failed to find chipset data\n"); + return -EINVAL; + } + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + pmc->base = devm_ioremap_resource(dev, res); + if (IS_ERR(pmc->base)) + return PTR_ERR(pmc->base); + + pmc->misc = syscon_regmap_lookup_by_phandle(dev->of_node, "syscon-misc"); + if (IS_ERR(pmc->misc)) + return PTR_ERR(pmc->misc); + + pmc->procmon = syscon_regmap_lookup_by_phandle(dev->of_node, "syscon-procmon"); + if (IS_ERR(pmc->procmon)) + return PTR_ERR(pmc->procmon); + + pmc->dev = dev; + + pmc->rcdev.ops = &brcm_pmc_reset_control_ops; + pmc->rcdev.owner = THIS_MODULE; + pmc->rcdev.of_node = dev->of_node; + pmc->rcdev.of_reset_n_cells = 2; + pmc->rcdev.of_xlate = brcm_pmc_reset_xlate; + + err = regmap_read(pmc->misc, pmc->data->misc_strap_bus_reg, &val); + if (err) + return err; + pmc->direct = !(val & pmc->data->misc_strap_bus_pmc_rom_boot_bit); + if (!pmc->direct) { + dev_err(dev, "detected unsupported access mode, falling back to the direct one\n"); + pmc->direct = 1; + } + + pmc->little_endian = !of_device_is_big_endian(dev->of_node); + + return devm_reset_controller_register(dev, &pmc->rcdev); +} + +static struct platform_driver brcm_pmc_reset_driver = { + .probe = brcm_pmc_reset_probe, + .driver = { + .name = "brcm-pmc", + .of_match_table = brcm_pmc_reset_of_match, + } +}; +module_platform_driver(brcm_pmc_reset_driver); + +MODULE_AUTHOR("Rafał Miłecki"); +MODULE_LICENSE("GPL v2"); +MODULE_DEVICE_TABLE(of, brcm_pmc_reset_of_match);