Message ID | 20201109163519.32605-1-zajec5@gmail.com |
---|---|
State | Changes Requested, archived |
Headers | show |
Series | [1/2] dt-bindings: reset: document Broadcom's PMC binding | expand |
Context | Check | Description |
---|---|---|
robh/checkpatch | success | |
robh/dt-meta-schema | success |
On 11/9/2020 8:35 AM, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > Broadcom's PMC is Power Management Controller that is used for disabling > and enabling SoC devices. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- [snip] > + > + big-endian: > + $ref: /schemas/types.yaml#/definitions/flag > + description: > + Flag to use for block working in big endian mode. > + > + "#reset-cells": > + const: 2 You would need to describe what these two cells are supposed to be.
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 Mon, Nov 09, 2020 at 05:35:18PM +0100, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > Broadcom's PMC is Power Management Controller that is used for disabling > and enabling SoC devices. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > .../bindings/reset/brcm,bcm-pmc.yaml | 69 +++++++++++++++++++ > 1 file changed, 69 insertions(+) > create mode 100644 Documentation/devicetree/bindings/reset/brcm,bcm-pmc.yaml > > diff --git a/Documentation/devicetree/bindings/reset/brcm,bcm-pmc.yaml b/Documentation/devicetree/bindings/reset/brcm,bcm-pmc.yaml > new file mode 100644 > index 000000000000..2afc2048997f > --- /dev/null > +++ b/Documentation/devicetree/bindings/reset/brcm,bcm-pmc.yaml > @@ -0,0 +1,69 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/reset/brcm,bcm-pmc.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Broadcom PMC (Power Management Controller) reset controller > + > +description: This document describes the Broadcom's PMC (Power Management > + Controller). It supports resetting devices identified by the bus id and device > + address. > + > +maintainers: > + - Rafał Miłecki <rafal@milecki.pl> > + > +properties: > + compatible: > + enum: > + - brcm,bcm4908-pmc # PMC on BCM4908 and compatible SoCs > + > + reg: > + maxItems: 1 > + > + syscon-misc: Needs a vendor prefix > + $ref: /schemas/types.yaml#/definitions/phandle > + description: > + Phandle to the MISC system controller node. > + > + syscon-procmon: Needs a vendor prefix > + $ref: /schemas/types.yaml#/definitions/phandle > + description: > + Phandle to the PROC MON system controller node. > + > + big-endian: > + $ref: /schemas/types.yaml#/definitions/flag > + description: > + Flag to use for block working in big endian mode. > + > + "#reset-cells": > + const: 2 > + > +required: > + - compatible > + - reg > + - syscon-misc > + - syscon-procmon > + - "#reset-cells" > + > +additionalProperties: false > + > +examples: > + - | > + pmc: reset-controller@80200000 { > + compatible = "brcm,bcm4908-pmc"; > + reg = <0x80200000 0x5000>; > + syscon-misc = <&misc>; > + syscon-procmon = <&procmon>; > + #reset-cells = <2>; > + }; > + > + procmon: syscon@80280000 { > + compatible = "brcm,misc", "syscon"; IIRC, not a documented compatible. Nor is it specific enough. You can just drop these from the example. > + reg = <0x80280000 0x1000>; > + }; > + > + misc: syscon@ff802600 { > + compatible = "brcm,misc", "syscon"; > + reg = <0xff802600 0xe4>; > + }; > -- > 2.27.0 >
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 */ > }
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/Documentation/devicetree/bindings/reset/brcm,bcm-pmc.yaml b/Documentation/devicetree/bindings/reset/brcm,bcm-pmc.yaml new file mode 100644 index 000000000000..2afc2048997f --- /dev/null +++ b/Documentation/devicetree/bindings/reset/brcm,bcm-pmc.yaml @@ -0,0 +1,69 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/reset/brcm,bcm-pmc.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Broadcom PMC (Power Management Controller) reset controller + +description: This document describes the Broadcom's PMC (Power Management + Controller). It supports resetting devices identified by the bus id and device + address. + +maintainers: + - Rafał Miłecki <rafal@milecki.pl> + +properties: + compatible: + enum: + - brcm,bcm4908-pmc # PMC on BCM4908 and compatible SoCs + + reg: + maxItems: 1 + + syscon-misc: + $ref: /schemas/types.yaml#/definitions/phandle + description: + Phandle to the MISC system controller node. + + syscon-procmon: + $ref: /schemas/types.yaml#/definitions/phandle + description: + Phandle to the PROC MON system controller node. + + big-endian: + $ref: /schemas/types.yaml#/definitions/flag + description: + Flag to use for block working in big endian mode. + + "#reset-cells": + const: 2 + +required: + - compatible + - reg + - syscon-misc + - syscon-procmon + - "#reset-cells" + +additionalProperties: false + +examples: + - | + pmc: reset-controller@80200000 { + compatible = "brcm,bcm4908-pmc"; + reg = <0x80200000 0x5000>; + syscon-misc = <&misc>; + syscon-procmon = <&procmon>; + #reset-cells = <2>; + }; + + procmon: syscon@80280000 { + compatible = "brcm,misc", "syscon"; + reg = <0x80280000 0x1000>; + }; + + misc: syscon@ff802600 { + compatible = "brcm,misc", "syscon"; + reg = <0xff802600 0xe4>; + };