| Message ID | 20230321201022.1052743-5-noltari@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: add BCM63268 timer clock and reset | expand |
Quoting Álvaro Fernández Rojas (2023-03-21 13:10:22) > diff --git a/drivers/clk/bcm/clk-bcm63268-timer.c b/drivers/clk/bcm/clk-bcm63268-timer.c > new file mode 100644 > index 000000000000..6a1fdd193cb5 > --- /dev/null > +++ b/drivers/clk/bcm/clk-bcm63268-timer.c > @@ -0,0 +1,232 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * BCM63268 Timer Clock and Reset Controller Driver [...] > + > +static inline struct bcm63268_tclkrst_hw * > +to_bcm63268_timer_reset(struct reset_controller_dev *rcdev) > +{ > + return container_of(rcdev, struct bcm63268_tclkrst_hw, rcdev); > +} > + > +static int bcm63268_timer_reset_update(struct reset_controller_dev *rcdev, > + unsigned long id, bool assert) > +{ > + struct bcm63268_tclkrst_hw *reset = to_bcm63268_timer_reset(rcdev); > + unsigned long flags; > + uint32_t val; > + > + spin_lock_irqsave(&reset->lock, flags); > + val = __raw_readl(reset->regs); Use regular ol readl() here, unless you have some need for no barrires or byte swapping. > + if (assert) > + val &= ~BIT(id); > + else > + val |= BIT(id); > + __raw_writel(val, reset->regs); Same. > + spin_unlock_irqrestore(&reset->lock, flags); > + > + return 0; > +} > + [...] > + > +static int bcm63268_tclk_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + const struct bcm63268_tclk_table_entry *entry, *table; > + struct bcm63268_tclkrst_hw *hw; > + struct clk_hw *clk; > + u8 maxbit = 0; > + int i, ret; > + > + table = of_device_get_match_data(dev); Use device_get_match_data() instead. > + if (!table) > + return -EINVAL; > + > + for (entry = table; entry->name; entry++) > + maxbit = max(maxbit, entry->bit); > + maxbit++; > + > + hw = devm_kzalloc(&pdev->dev, struct_size(hw, data.hws, maxbit), > + GFP_KERNEL); > + if (!hw) > + return -ENOMEM; > + > + platform_set_drvdata(pdev, hw); > + > + spin_lock_init(&hw->lock); > + > + hw->data.num = maxbit; > + for (i = 0; i < maxbit; i++) > + hw->data.hws[i] = ERR_PTR(-ENODEV); > + > + hw->regs = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(hw->regs)) > + return PTR_ERR(hw->regs); > + > + for (entry = table; entry->name; entry++) { > + clk = clk_hw_register_gate(dev, entry->name, NULL, 0, Use devm? > + hw->regs, entry->bit, > + CLK_GATE_BIG_ENDIAN, &hw->lock); > + if (IS_ERR(clk)) { > + ret = PTR_ERR(clk); > + goto out_err; > + } > + > + hw->data.hws[entry->bit] = clk; > + } > + > + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, > + &hw->data); > + if (ret) > + return ret; > + > + hw->rcdev.of_node = dev->of_node; > + hw->rcdev.ops = &bcm63268_timer_reset_ops; > + > + ret = devm_reset_controller_register(dev, &hw->rcdev); > + if (ret) > + dev_err(dev, "Failed to register reset controller\n"); > + > + return 0; > + > +out_err: > + for (i = 0; i < hw->data.num; i++) { > + if (!IS_ERR(hw->data.hws[i])) > + clk_hw_unregister_gate(hw->data.hws[i]); And then drop this? > + } > + > + return ret; > +} > + > +static const struct of_device_id bcm63268_tclk_dt_ids[] = { > + { > + .compatible = "brcm,bcm63268-timer-clocks", > + .data = &bcm63268_timer_clocks, Are you planning on adding more SoCs to this driver? The data can currently be always assumed to be bcm63268_timer_clocks > + }, { > + /* sentinel */ > + } > +}; > +
On 3/21/23 15:57, Stephen Boyd wrote: > Quoting Álvaro Fernández Rojas (2023-03-21 13:10:22) >> diff --git a/drivers/clk/bcm/clk-bcm63268-timer.c b/drivers/clk/bcm/clk-bcm63268-timer.c >> new file mode 100644 >> index 000000000000..6a1fdd193cb5 >> --- /dev/null >> +++ b/drivers/clk/bcm/clk-bcm63268-timer.c >> @@ -0,0 +1,232 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * BCM63268 Timer Clock and Reset Controller Driver > [...] >> + >> +static inline struct bcm63268_tclkrst_hw * >> +to_bcm63268_timer_reset(struct reset_controller_dev *rcdev) >> +{ >> + return container_of(rcdev, struct bcm63268_tclkrst_hw, rcdev); >> +} >> + >> +static int bcm63268_timer_reset_update(struct reset_controller_dev *rcdev, >> + unsigned long id, bool assert) >> +{ >> + struct bcm63268_tclkrst_hw *reset = to_bcm63268_timer_reset(rcdev); >> + unsigned long flags; >> + uint32_t val; >> + >> + spin_lock_irqsave(&reset->lock, flags); >> + val = __raw_readl(reset->regs); > > Use regular ol readl() here, unless you have some need for no barrires > or byte swapping. These SoCs are big-endian, require native endian register access and have no posted writes within their bus logic (UBUS) and require no barriers, hence the use of __raw_readl() and __raw_writel() is adequate.
Quoting Florian Fainelli (2023-03-21 16:00:29) > On 3/21/23 15:57, Stephen Boyd wrote: > > Quoting Álvaro Fernández Rojas (2023-03-21 13:10:22) > >> diff --git a/drivers/clk/bcm/clk-bcm63268-timer.c b/drivers/clk/bcm/clk-bcm63268-timer.c > >> new file mode 100644 > >> index 000000000000..6a1fdd193cb5 > >> --- /dev/null > >> +++ b/drivers/clk/bcm/clk-bcm63268-timer.c > >> @@ -0,0 +1,232 @@ > >> +// SPDX-License-Identifier: GPL-2.0 > >> +/* > >> + * BCM63268 Timer Clock and Reset Controller Driver > > [...] > >> + > >> +static inline struct bcm63268_tclkrst_hw * > >> +to_bcm63268_timer_reset(struct reset_controller_dev *rcdev) > >> +{ > >> + return container_of(rcdev, struct bcm63268_tclkrst_hw, rcdev); > >> +} > >> + > >> +static int bcm63268_timer_reset_update(struct reset_controller_dev *rcdev, > >> + unsigned long id, bool assert) > >> +{ > >> + struct bcm63268_tclkrst_hw *reset = to_bcm63268_timer_reset(rcdev); > >> + unsigned long flags; > >> + uint32_t val; > >> + > >> + spin_lock_irqsave(&reset->lock, flags); > >> + val = __raw_readl(reset->regs); > > > > Use regular ol readl() here, unless you have some need for no barrires > > or byte swapping. > > These SoCs are big-endian, require native endian register access and > have no posted writes within their bus logic (UBUS) and require no > barriers, hence the use of __raw_readl() and __raw_writel() is adequate. > Use ioread32be() then?
On 3/21/23 16:06, Stephen Boyd wrote: > Quoting Florian Fainelli (2023-03-21 16:00:29) >> On 3/21/23 15:57, Stephen Boyd wrote: >>> Quoting Álvaro Fernández Rojas (2023-03-21 13:10:22) >>>> diff --git a/drivers/clk/bcm/clk-bcm63268-timer.c b/drivers/clk/bcm/clk-bcm63268-timer.c >>>> new file mode 100644 >>>> index 000000000000..6a1fdd193cb5 >>>> --- /dev/null >>>> +++ b/drivers/clk/bcm/clk-bcm63268-timer.c >>>> @@ -0,0 +1,232 @@ >>>> +// SPDX-License-Identifier: GPL-2.0 >>>> +/* >>>> + * BCM63268 Timer Clock and Reset Controller Driver >>> [...] >>>> + >>>> +static inline struct bcm63268_tclkrst_hw * >>>> +to_bcm63268_timer_reset(struct reset_controller_dev *rcdev) >>>> +{ >>>> + return container_of(rcdev, struct bcm63268_tclkrst_hw, rcdev); >>>> +} >>>> + >>>> +static int bcm63268_timer_reset_update(struct reset_controller_dev *rcdev, >>>> + unsigned long id, bool assert) >>>> +{ >>>> + struct bcm63268_tclkrst_hw *reset = to_bcm63268_timer_reset(rcdev); >>>> + unsigned long flags; >>>> + uint32_t val; >>>> + >>>> + spin_lock_irqsave(&reset->lock, flags); >>>> + val = __raw_readl(reset->regs); >>> >>> Use regular ol readl() here, unless you have some need for no barrires >>> or byte swapping. >> >> These SoCs are big-endian, require native endian register access and >> have no posted writes within their bus logic (UBUS) and require no >> barriers, hence the use of __raw_readl() and __raw_writel() is adequate. >> > > Use ioread32be() then? BCM63xx drivers tend to use __raw_{read,write}l for consistency and to make it clear that no barriers, no endian swapping is necessary, I would prefer to remain consistent with that convention.
Quoting Florian Fainelli (2023-03-21 16:09:54) > On 3/21/23 16:06, Stephen Boyd wrote: > > Quoting Florian Fainelli (2023-03-21 16:00:29) > >> > >> These SoCs are big-endian, require native endian register access and > >> have no posted writes within their bus logic (UBUS) and require no > >> barriers, hence the use of __raw_readl() and __raw_writel() is adequate. > >> > > > > Use ioread32be() then? > > BCM63xx drivers tend to use __raw_{read,write}l for consistency and to > make it clear that no barriers, no endian swapping is necessary, I would > prefer to remain consistent with that convention. Ok. Is the clk device big-endian? Or the CPU is big-endian? SoC being big-endian sounds like the devices in the SoC are big-endian. I hope we never plop this device down with a CPU that's litle-endian.
El mié, 22 mar 2023 a las 0:23, Stephen Boyd (<sboyd@kernel.org>) escribió: > > Quoting Florian Fainelli (2023-03-21 16:09:54) > > On 3/21/23 16:06, Stephen Boyd wrote: > > > Quoting Florian Fainelli (2023-03-21 16:00:29) > > >> > > >> These SoCs are big-endian, require native endian register access and > > >> have no posted writes within their bus logic (UBUS) and require no > > >> barriers, hence the use of __raw_readl() and __raw_writel() is adequate. > > >> > > > > > > Use ioread32be() then? > > > > BCM63xx drivers tend to use __raw_{read,write}l for consistency and to > > make it clear that no barriers, no endian swapping is necessary, I would > > prefer to remain consistent with that convention. > > Ok. > > Is the clk device big-endian? Or the CPU is big-endian? SoC being > big-endian sounds like the devices in the SoC are big-endian. I hope we > never plop this device down with a CPU that's litle-endian. The SoC is big-endian. I've only worked with MIPS big-endian devices from Broadcom, so I'm not really sure, but this seems to be very BCM63268-specific... Other BCM63xx devices I know have separate clock and reset controllers, but not this kind of timer, clock and reset controller... -- Álvaro
On 3/21/23 16:23, Stephen Boyd wrote: > Quoting Florian Fainelli (2023-03-21 16:09:54) >> On 3/21/23 16:06, Stephen Boyd wrote: >>> Quoting Florian Fainelli (2023-03-21 16:00:29) >>>> >>>> These SoCs are big-endian, require native endian register access and >>>> have no posted writes within their bus logic (UBUS) and require no >>>> barriers, hence the use of __raw_readl() and __raw_writel() is adequate. >>>> >>> >>> Use ioread32be() then? >> >> BCM63xx drivers tend to use __raw_{read,write}l for consistency and to >> make it clear that no barriers, no endian swapping is necessary, I would >> prefer to remain consistent with that convention. > > Ok. > > Is the clk device big-endian? Or the CPU is big-endian? SoC being > big-endian sounds like the devices in the SoC are big-endian. I hope we > never plop this device down with a CPU that's litle-endian. The CPU is big endian and the peripheral and bus to access the peripheral are native endian, so also big endian in that case. The newer SoCs are ARM-based and are little endian, but we already have a clock driver for those.
diff --git a/drivers/clk/bcm/Kconfig b/drivers/clk/bcm/Kconfig index 77266afb1c79..a972d763eb77 100644 --- a/drivers/clk/bcm/Kconfig +++ b/drivers/clk/bcm/Kconfig @@ -37,6 +37,15 @@ config CLK_BCM_63XX_GATE Enable common clock framework support for Broadcom BCM63xx DSL SoCs based on the MIPS architecture +config CLK_BCM63268_TIMER + bool "Broadcom BCM63268 timer clock and reset support" + depends on BMIPS_GENERIC || COMPILE_TEST + default BMIPS_GENERIC + select RESET_CONTROLLER + help + Enable timer clock and reset support for Broadcom BCM63268 DSL SoCs + based on the MIPS architecture. + config CLK_BCM_KONA bool "Broadcom Kona CCU clock support" depends on ARCH_BCM_MOBILE || COMPILE_TEST diff --git a/drivers/clk/bcm/Makefile b/drivers/clk/bcm/Makefile index edb66b44cb27..d0b6f4b1fb08 100644 --- a/drivers/clk/bcm/Makefile +++ b/drivers/clk/bcm/Makefile @@ -1,6 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_CLK_BCM_63XX) += clk-bcm63xx.o obj-$(CONFIG_CLK_BCM_63XX_GATE) += clk-bcm63xx-gate.o +obj-$(CONFIG_CLK_BCM63268_TIMER) += clk-bcm63268-timer.o obj-$(CONFIG_CLK_BCM_KONA) += clk-kona.o obj-$(CONFIG_CLK_BCM_KONA) += clk-kona-setup.o obj-$(CONFIG_CLK_BCM_KONA) += clk-bcm281xx.o diff --git a/drivers/clk/bcm/clk-bcm63268-timer.c b/drivers/clk/bcm/clk-bcm63268-timer.c new file mode 100644 index 000000000000..6a1fdd193cb5 --- /dev/null +++ b/drivers/clk/bcm/clk-bcm63268-timer.c @@ -0,0 +1,232 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * BCM63268 Timer Clock and Reset Controller Driver + * + * Copyright (C) 2023 Álvaro Fernández Rojas <noltari@gmail.com> + */ + +#include <linux/clk-provider.h> +#include <linux/delay.h> +#include <linux/io.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/reset-controller.h> + +#include <dt-bindings/clock/bcm63268-clock.h> + +#define BCM63268_TIMER_RESET_SLEEP_MIN_US 10000 +#define BCM63268_TIMER_RESET_SLEEP_MAX_US 20000 + +struct bcm63268_tclkrst_hw { + void __iomem *regs; + spinlock_t lock; + + struct reset_controller_dev rcdev; + struct clk_hw_onecell_data data; +}; + +struct bcm63268_tclk_table_entry { + const char * const name; + u8 bit; +}; + +static const struct bcm63268_tclk_table_entry bcm63268_timer_clocks[] = { + { + .name = "ephy1", + .bit = BCM63268_TCLK_EPHY1, + }, { + .name = "ephy2", + .bit = BCM63268_TCLK_EPHY2, + }, { + .name = "ephy3", + .bit = BCM63268_TCLK_EPHY3, + }, { + .name = "gphy1", + .bit = BCM63268_TCLK_GPHY1, + }, { + .name = "dsl", + .bit = BCM63268_TCLK_DSL, + }, { + .name = "wakeon_ephy", + .bit = BCM63268_TCLK_WAKEON_EPHY, + }, { + .name = "wakeon_dsl", + .bit = BCM63268_TCLK_WAKEON_DSL, + }, { + .name = "fap1_pll", + .bit = BCM63268_TCLK_FAP1, + }, { + .name = "fap2_pll", + .bit = BCM63268_TCLK_FAP2, + }, { + .name = "uto_50", + .bit = BCM63268_TCLK_UTO_50, + }, { + .name = "uto_extin", + .bit = BCM63268_TCLK_UTO_EXTIN, + }, { + .name = "usb_ref", + .bit = BCM63268_TCLK_USB_REF, + }, { + /* sentinel */ + } +}; + +static inline struct bcm63268_tclkrst_hw * +to_bcm63268_timer_reset(struct reset_controller_dev *rcdev) +{ + return container_of(rcdev, struct bcm63268_tclkrst_hw, rcdev); +} + +static int bcm63268_timer_reset_update(struct reset_controller_dev *rcdev, + unsigned long id, bool assert) +{ + struct bcm63268_tclkrst_hw *reset = to_bcm63268_timer_reset(rcdev); + unsigned long flags; + uint32_t val; + + spin_lock_irqsave(&reset->lock, flags); + val = __raw_readl(reset->regs); + if (assert) + val &= ~BIT(id); + else + val |= BIT(id); + __raw_writel(val, reset->regs); + spin_unlock_irqrestore(&reset->lock, flags); + + return 0; +} + +static int bcm63268_timer_reset_assert(struct reset_controller_dev *rcdev, + unsigned long id) +{ + return bcm63268_timer_reset_update(rcdev, id, true); +} + +static int bcm63268_timer_reset_deassert(struct reset_controller_dev *rcdev, + unsigned long id) +{ + return bcm63268_timer_reset_update(rcdev, id, false); +} + +static int bcm63268_timer_reset_reset(struct reset_controller_dev *rcdev, + unsigned long id) +{ + bcm63268_timer_reset_update(rcdev, id, true); + usleep_range(BCM63268_TIMER_RESET_SLEEP_MIN_US, + BCM63268_TIMER_RESET_SLEEP_MAX_US); + + bcm63268_timer_reset_update(rcdev, id, false); + /* + * Ensure component is taken out reset state by sleeping also after + * deasserting the reset. Otherwise, the component may not be ready + * for operation. + */ + usleep_range(BCM63268_TIMER_RESET_SLEEP_MIN_US, + BCM63268_TIMER_RESET_SLEEP_MAX_US); + + return 0; +} + +static int bcm63268_timer_reset_status(struct reset_controller_dev *rcdev, + unsigned long id) +{ + struct bcm63268_tclkrst_hw *reset = to_bcm63268_timer_reset(rcdev); + + return !(__raw_readl(reset->regs) & BIT(id)); +} + +static struct reset_control_ops bcm63268_timer_reset_ops = { + .assert = bcm63268_timer_reset_assert, + .deassert = bcm63268_timer_reset_deassert, + .reset = bcm63268_timer_reset_reset, + .status = bcm63268_timer_reset_status, +}; + +static int bcm63268_tclk_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + const struct bcm63268_tclk_table_entry *entry, *table; + struct bcm63268_tclkrst_hw *hw; + struct clk_hw *clk; + u8 maxbit = 0; + int i, ret; + + table = of_device_get_match_data(dev); + if (!table) + return -EINVAL; + + for (entry = table; entry->name; entry++) + maxbit = max(maxbit, entry->bit); + maxbit++; + + hw = devm_kzalloc(&pdev->dev, struct_size(hw, data.hws, maxbit), + GFP_KERNEL); + if (!hw) + return -ENOMEM; + + platform_set_drvdata(pdev, hw); + + spin_lock_init(&hw->lock); + + hw->data.num = maxbit; + for (i = 0; i < maxbit; i++) + hw->data.hws[i] = ERR_PTR(-ENODEV); + + hw->regs = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(hw->regs)) + return PTR_ERR(hw->regs); + + for (entry = table; entry->name; entry++) { + clk = clk_hw_register_gate(dev, entry->name, NULL, 0, + hw->regs, entry->bit, + CLK_GATE_BIG_ENDIAN, &hw->lock); + if (IS_ERR(clk)) { + ret = PTR_ERR(clk); + goto out_err; + } + + hw->data.hws[entry->bit] = clk; + } + + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, + &hw->data); + if (ret) + return ret; + + hw->rcdev.of_node = dev->of_node; + hw->rcdev.ops = &bcm63268_timer_reset_ops; + + ret = devm_reset_controller_register(dev, &hw->rcdev); + if (ret) + dev_err(dev, "Failed to register reset controller\n"); + + return 0; + +out_err: + for (i = 0; i < hw->data.num; i++) { + if (!IS_ERR(hw->data.hws[i])) + clk_hw_unregister_gate(hw->data.hws[i]); + } + + return ret; +} + +static const struct of_device_id bcm63268_tclk_dt_ids[] = { + { + .compatible = "brcm,bcm63268-timer-clocks", + .data = &bcm63268_timer_clocks, + }, { + /* sentinel */ + } +}; + +static struct platform_driver bcm63268_tclk = { + .probe = bcm63268_tclk_probe, + .driver = { + .name = "bcm63268-timer-clock", + .of_match_table = bcm63268_tclk_dt_ids, + }, +}; +builtin_platform_driver(bcm63268_tclk);
Add driver for BCM63268 timer clock and reset controller. Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> --- v3: add missing <linux/io.h> include to fix build warning v2: add changes suggested by Stephen Boyd drivers/clk/bcm/Kconfig | 9 ++ drivers/clk/bcm/Makefile | 1 + drivers/clk/bcm/clk-bcm63268-timer.c | 232 +++++++++++++++++++++++++++ 3 files changed, 242 insertions(+) create mode 100644 drivers/clk/bcm/clk-bcm63268-timer.c