| Message ID | 20160727212457.20038-5-swarren@wwwdotorg.org |
|---|---|
| State | Superseded |
| Delegated to: | Tom Warren |
| Headers | show |
Hi Stephen, On 27 July 2016 at 15:24, Stephen Warren <swarren@wwwdotorg.org> wrote: > From: Stephen Warren <swarren@nvidia.com> > > In Tegra186, on-SoC clocks are manipulated using IPC requests to the BPMP > (Boot and Power Management Processor). This change implements a driver > that does that. A tegra/ sub-directory is created to follow the existing > pattern. It is unconditionally selected by CONFIG_TEGRA186 since virtually > any Tegra186 build of U-Boot will need the feature. > > Signed-off-by: Stephen Warren <swarren@nvidia.com> > --- > arch/arm/mach-tegra/Kconfig | 2 + > drivers/clk/Kconfig | 1 + > drivers/clk/Makefile | 2 + > drivers/clk/tegra/Kconfig | 6 +++ > drivers/clk/tegra/Makefile | 5 ++ > drivers/clk/tegra/tegra186-clk.c | 99 ++++++++++++++++++++++++++++++++++++++++ > 6 files changed, 115 insertions(+) > create mode 100644 drivers/clk/tegra/Kconfig > create mode 100644 drivers/clk/tegra/Makefile > create mode 100644 drivers/clk/tegra/tegra186-clk.c > > diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig > index ec2d8ac6a1a3..e8186d515856 100644 > --- a/arch/arm/mach-tegra/Kconfig > +++ b/arch/arm/mach-tegra/Kconfig > @@ -64,9 +64,11 @@ config TEGRA210 > > config TEGRA186 > bool "Tegra186 family" > + select CLK > select DM_MAILBOX > select MISC > select TEGRA186_BPMP > + select TEGRA186_CLOCK > select TEGRA186_GPIO > select TEGRA_ARMV8_COMMON > select TEGRA_HSP > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig > index 6eee8eb369bf..7dd56738b06a 100644 > --- a/drivers/clk/Kconfig > +++ b/drivers/clk/Kconfig > @@ -20,6 +20,7 @@ config SPL_CLK > setting up clocks within SPL, and allows the same drivers to be > used as U-Boot proper. > > +source "drivers/clk/tegra/Kconfig" > source "drivers/clk/uniphier/Kconfig" > source "drivers/clk/exynos/Kconfig" > > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile > index f7a88912e06a..37b946cb6bdc 100644 > --- a/drivers/clk/Makefile > +++ b/drivers/clk/Makefile > @@ -11,5 +11,7 @@ obj-$(CONFIG_ROCKCHIP_RK3288) += clk_rk3288.o > obj-$(CONFIG_SANDBOX) += clk_sandbox.o > obj-$(CONFIG_SANDBOX) += clk_sandbox_test.o > obj-$(CONFIG_MACH_PIC32) += clk_pic32.o > + > +obj-y += tegra/ > obj-$(CONFIG_CLK_UNIPHIER) += uniphier/ > obj-$(CONFIG_CLK_EXYNOS) += exynos/ > diff --git a/drivers/clk/tegra/Kconfig b/drivers/clk/tegra/Kconfig > new file mode 100644 > index 000000000000..659fe022c2af > --- /dev/null > +++ b/drivers/clk/tegra/Kconfig > @@ -0,0 +1,6 @@ > +config TEGRA186_CLOCK > + bool "Enable Tegra186 BPMP-based clock driver" > + depends on TEGRA186_BPMP > + help > + Enable support for manipulating Tegra's on-SoC clocks via IPC > + requests to the BPMP (Boot and Power Management Processor). > diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile > new file mode 100644 > index 000000000000..f32998ccc27d > --- /dev/null > +++ b/drivers/clk/tegra/Makefile > @@ -0,0 +1,5 @@ > +# Copyright (c) 2016, NVIDIA CORPORATION. > +# > +# SPDX-License-Identifier: GPL-2.0 > + > +obj-$(CONFIG_TEGRA186_CLOCK) += tegra186-clk.o > diff --git a/drivers/clk/tegra/tegra186-clk.c b/drivers/clk/tegra/tegra186-clk.c > new file mode 100644 > index 000000000000..fcfe3b47714a > --- /dev/null > +++ b/drivers/clk/tegra/tegra186-clk.c > @@ -0,0 +1,99 @@ > +/* > + * Copyright (c) 2016, NVIDIA CORPORATION. > + * > + * SPDX-License-Identifier: GPL-2.0 > + */ > + > +#include <common.h> > +#include <clk-uclass.h> > +#include <dm.h> > +#include <asm/arch-tegra/bpmp_abi.h> > +#include <asm/arch-tegra/tegra186_bpmp.h> > + > +static ulong tegra186_clk_get_rate(struct clk *clk) > +{ > + struct mrq_clk_request req; > + struct mrq_clk_response resp; > + int ret; > + > + debug("%s(clk=%p) (dev=%p, id=%lu)\n", __func__, clk, clk->dev, > + clk->id); > + > + req.cmd_and_id = (CMD_CLK_GET_RATE << 24) | clk->id; > + > + ret = tegra186_bpmp_call(clk->dev->parent, MRQ_CLK, > + &req, sizeof(req), &resp, sizeof(resp)); Isn't his a MISC driver? Perhaps you can add a new method to UCLASS_MISC matching your requirements here? > + if (ret) > + return ret; > + > + return resp.clk_get_rate.rate; > +} > + > +static ulong tegra186_clk_set_rate(struct clk *clk, ulong rate) > +{ > + struct mrq_clk_request req; > + struct mrq_clk_response resp; > + int ret; > + > + debug("%s(clk=%p, rate=%lu) (dev=%p, id=%lu)\n", __func__, clk, rate, > + clk->dev, clk->id); > + > + req.cmd_and_id = (CMD_CLK_SET_RATE << 24) | clk->id; > + req.clk_set_rate.rate = rate; > + > + ret = tegra186_bpmp_call(clk->dev->parent, MRQ_CLK, > + &req, sizeof(req), &resp, sizeof(resp)); > + if (ret) > + return ret; > + > + return resp.clk_set_rate.rate; > +} > + > +static int tegra186_clk_en_dis(struct clk *clk, > + enum mrq_reset_commands cmd) > +{ > + struct mrq_clk_request req; > + struct mrq_clk_response resp; > + > + req.cmd_and_id = (cmd << 24) | clk->id; > + > + return tegra186_bpmp_call(clk->dev->parent, MRQ_CLK, > + &req, sizeof(req), &resp, sizeof(resp)); > +} > + > +static int tegra186_clk_enable(struct clk *clk) > +{ > + debug("%s(clk=%p) (dev=%p, id=%lu)\n", __func__, clk, clk->dev, > + clk->id); > + > + return tegra186_clk_en_dis(clk, CMD_CLK_ENABLE); > +} > + > +static int tegra186_clk_disable(struct clk *clk) > +{ > + debug("%s(clk=%p) (dev=%p, id=%lu)\n", __func__, clk, clk->dev, > + clk->id); > + > + return tegra186_clk_en_dis(clk, CMD_CLK_DISABLE); > +} > + > +static struct clk_ops tegra186_clk_ops = { > + .get_rate = tegra186_clk_get_rate, > + .set_rate = tegra186_clk_set_rate, > + .enable = tegra186_clk_enable, > + .disable = tegra186_clk_disable, > +}; > + > +static int tegra186_clk_probe(struct udevice *dev) > +{ > + debug("%s(dev=%p)\n", __func__, dev); > + > + return 0; > +} > + > +U_BOOT_DRIVER(tegra186_clk) = { > + .name = "tegra186_clk", > + .id = UCLASS_CLK, > + .probe = tegra186_clk_probe, > + .ops = &tegra186_clk_ops, > +}; > -- > 2.9.2 > Regards, Simon
Hi Stephen, On 1 August 2016 at 09:46, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 07/31/2016 07:02 PM, Simon Glass wrote: >> >> Hi Stephen, >> >> On 27 July 2016 at 15:24, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> >>> From: Stephen Warren <swarren@nvidia.com> >>> >>> In Tegra186, on-SoC clocks are manipulated using IPC requests to the BPMP >>> (Boot and Power Management Processor). This change implements a driver >>> that does that. A tegra/ sub-directory is created to follow the existing >>> pattern. It is unconditionally selected by CONFIG_TEGRA186 since >>> virtually >>> any Tegra186 build of U-Boot will need the feature. > > >>> diff --git a/drivers/clk/tegra/tegra186-clk.c >>> b/drivers/clk/tegra/tegra186-clk.c > > >>> +static ulong tegra186_clk_get_rate(struct clk *clk) >>> +{ >>> + struct mrq_clk_request req; >>> + struct mrq_clk_response resp; >>> + int ret; >>> + >>> + debug("%s(clk=%p) (dev=%p, id=%lu)\n", __func__, clk, clk->dev, >>> + clk->id); >>> + >>> + req.cmd_and_id = (CMD_CLK_GET_RATE << 24) | clk->id; >>> + >>> + ret = tegra186_bpmp_call(clk->dev->parent, MRQ_CLK, >>> + &req, sizeof(req), &resp, sizeof(resp)); >> >> >> Isn't his a MISC driver? Perhaps you can add a new method to >> UCLASS_MISC matching your requirements here? > > > The core BPMP driver is a MISC driver, yes. > > I don't see any advantage of making this call something that the MISC uclass > supports directly. This function is an internal implementation detail of the > BPMP, and certainly not something that every single MISC driver (for any SoC > for any HW module) would implement. If we did add direct support to the MISC > uclass, then the MISC "ops" structure would basically grow forever since > every single SoC's/HW's internal function calls would be added to it. This > would just bloat it up unnecessarily, and I don't see any advantage to > offset the disadvantage of that bloat. I'm really just suggesting that you could add one more method to the misc uclass, which does a simultaneous write and read. > > FWIW, the Linux MFD (Multi-Function Devices) stack typically has "child" > (sub-)devices call custom APIs like this on the "parent"/container. Yes, I'm just trying to encourage use of the driver-model features - one of the design goals is to expose otherwise private driver interfaces. Regards, Simon
On 08/04/2016 07:36 PM, Simon Glass wrote: > Hi Stephen, > > On 1 August 2016 at 09:46, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 07/31/2016 07:02 PM, Simon Glass wrote: >>> >>> Hi Stephen, >>> >>> On 27 July 2016 at 15:24, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>> >>>> From: Stephen Warren <swarren@nvidia.com> >>>> >>>> In Tegra186, on-SoC clocks are manipulated using IPC requests to the BPMP >>>> (Boot and Power Management Processor). This change implements a driver >>>> that does that. A tegra/ sub-directory is created to follow the existing >>>> pattern. It is unconditionally selected by CONFIG_TEGRA186 since >>>> virtually >>>> any Tegra186 build of U-Boot will need the feature. >> >> >>>> diff --git a/drivers/clk/tegra/tegra186-clk.c >>>> b/drivers/clk/tegra/tegra186-clk.c >> >> >>>> +static ulong tegra186_clk_get_rate(struct clk *clk) >>>> +{ >>>> + struct mrq_clk_request req; >>>> + struct mrq_clk_response resp; >>>> + int ret; >>>> + >>>> + debug("%s(clk=%p) (dev=%p, id=%lu)\n", __func__, clk, clk->dev, >>>> + clk->id); >>>> + >>>> + req.cmd_and_id = (CMD_CLK_GET_RATE << 24) | clk->id; >>>> + >>>> + ret = tegra186_bpmp_call(clk->dev->parent, MRQ_CLK, >>>> + &req, sizeof(req), &resp, sizeof(resp)); >>> >>> >>> Isn't his a MISC driver? Perhaps you can add a new method to >>> UCLASS_MISC matching your requirements here? >> >> >> The core BPMP driver is a MISC driver, yes. >> >> I don't see any advantage of making this call something that the MISC uclass >> supports directly. This function is an internal implementation detail of the >> BPMP, and certainly not something that every single MISC driver (for any SoC >> for any HW module) would implement. If we did add direct support to the MISC >> uclass, then the MISC "ops" structure would basically grow forever since >> every single SoC's/HW's internal function calls would be added to it. This >> would just bloat it up unnecessarily, and I don't see any advantage to >> offset the disadvantage of that bloat. > > I'm really just suggesting that you could add one more method to the > misc uclass, which does a simultaneous write and read. OK, I guess I will do that if I must. However, I doubt we will see much/any benefit from this. Taking a look at the cros_ec example you mentioned in your other mail, you'll note that the EC and BPMP ABIs are different. Sure, both accept: device(udev) to "communicate on" TX buffer, length RX buffer, length However, both of these functions take the message body from the client, locally create the TX header before sending, and strip off the RX header before giving it back to the caller. This means the function also takes all the parameters to construct the message header. These are different for EC (message number and version) vs. BPMP (just message number). While we could make the prototype of the misc uclass function accept both and just ignore the extra information, that seems quite hokey. Eventually we'll probably end up with some other similar situation that needs more parameters and we'll just have to edit every existing caller to provide dummy data. Equally, routing everything through the uclass function prevents any kind of type-safety for these generic parameters (say using an enum, or appropriate data size) since by definition the common function is common and therefore must use very generic types. It's very obvious to me how routing truly common operations through standard APIs makes sense; it brings order, allows drivers to use the same APIs on all platforms, etc. However, once you get down to low-level internal details of tightly couple drivers, the benefits disappear. Ideally, we'd actually have a single BPMP driver that provided all of clock, reset, power domain, I2C, ... from the one device (instantiated from the one BPMP device node). It's just an artificial limitation of U-Boot's DM that a given driver/device can only provide one service, so we must have multiple communicating devices. If DM allowed separation between devices and the services they provide (as Linux does for most subsystems at least), there wouldn't even be any driver-to-driver/device-to-device within the context of BPMP, so all this would be irrelevant. >> FWIW, the Linux MFD (Multi-Function Devices) stack typically has "child" >> (sub-)devices call custom APIs like this on the "parent"/container. > > Yes, I'm just trying to encourage use of the driver-model features - > one of the design goals is to expose otherwise private driver > interfaces. I'm not sure that's a good goal as an absolute general thing. Sure it makes sense to standardize interfaces between unrelated drivers to remove coupling. However, when the devices/drivers really are tightly coupled rather than independent IP blocks, and especially if they only artificially exist due to DM structure, I don't see any benefit of forcing everything through DM. You'd mentioned enhancing debug and visibility in your other email. I don't believe either would be benefited here. The debugger will step through a call to a function or a call to a function pointer just as easily. A human on the other hand will be able to understand what's going on much more easily if the call is directly to a named function rather than having to look up where a function pointer points, since there's no extra work to find out where the function pointer points.
diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig index ec2d8ac6a1a3..e8186d515856 100644 --- a/arch/arm/mach-tegra/Kconfig +++ b/arch/arm/mach-tegra/Kconfig @@ -64,9 +64,11 @@ config TEGRA210 config TEGRA186 bool "Tegra186 family" + select CLK select DM_MAILBOX select MISC select TEGRA186_BPMP + select TEGRA186_CLOCK select TEGRA186_GPIO select TEGRA_ARMV8_COMMON select TEGRA_HSP diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index 6eee8eb369bf..7dd56738b06a 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -20,6 +20,7 @@ config SPL_CLK setting up clocks within SPL, and allows the same drivers to be used as U-Boot proper. +source "drivers/clk/tegra/Kconfig" source "drivers/clk/uniphier/Kconfig" source "drivers/clk/exynos/Kconfig" diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index f7a88912e06a..37b946cb6bdc 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -11,5 +11,7 @@ obj-$(CONFIG_ROCKCHIP_RK3288) += clk_rk3288.o obj-$(CONFIG_SANDBOX) += clk_sandbox.o obj-$(CONFIG_SANDBOX) += clk_sandbox_test.o obj-$(CONFIG_MACH_PIC32) += clk_pic32.o + +obj-y += tegra/ obj-$(CONFIG_CLK_UNIPHIER) += uniphier/ obj-$(CONFIG_CLK_EXYNOS) += exynos/ diff --git a/drivers/clk/tegra/Kconfig b/drivers/clk/tegra/Kconfig new file mode 100644 index 000000000000..659fe022c2af --- /dev/null +++ b/drivers/clk/tegra/Kconfig @@ -0,0 +1,6 @@ +config TEGRA186_CLOCK + bool "Enable Tegra186 BPMP-based clock driver" + depends on TEGRA186_BPMP + help + Enable support for manipulating Tegra's on-SoC clocks via IPC + requests to the BPMP (Boot and Power Management Processor). diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile new file mode 100644 index 000000000000..f32998ccc27d --- /dev/null +++ b/drivers/clk/tegra/Makefile @@ -0,0 +1,5 @@ +# Copyright (c) 2016, NVIDIA CORPORATION. +# +# SPDX-License-Identifier: GPL-2.0 + +obj-$(CONFIG_TEGRA186_CLOCK) += tegra186-clk.o diff --git a/drivers/clk/tegra/tegra186-clk.c b/drivers/clk/tegra/tegra186-clk.c new file mode 100644 index 000000000000..fcfe3b47714a --- /dev/null +++ b/drivers/clk/tegra/tegra186-clk.c @@ -0,0 +1,99 @@ +/* + * Copyright (c) 2016, NVIDIA CORPORATION. + * + * SPDX-License-Identifier: GPL-2.0 + */ + +#include <common.h> +#include <clk-uclass.h> +#include <dm.h> +#include <asm/arch-tegra/bpmp_abi.h> +#include <asm/arch-tegra/tegra186_bpmp.h> + +static ulong tegra186_clk_get_rate(struct clk *clk) +{ + struct mrq_clk_request req; + struct mrq_clk_response resp; + int ret; + + debug("%s(clk=%p) (dev=%p, id=%lu)\n", __func__, clk, clk->dev, + clk->id); + + req.cmd_and_id = (CMD_CLK_GET_RATE << 24) | clk->id; + + ret = tegra186_bpmp_call(clk->dev->parent, MRQ_CLK, + &req, sizeof(req), &resp, sizeof(resp)); + if (ret) + return ret; + + return resp.clk_get_rate.rate; +} + +static ulong tegra186_clk_set_rate(struct clk *clk, ulong rate) +{ + struct mrq_clk_request req; + struct mrq_clk_response resp; + int ret; + + debug("%s(clk=%p, rate=%lu) (dev=%p, id=%lu)\n", __func__, clk, rate, + clk->dev, clk->id); + + req.cmd_and_id = (CMD_CLK_SET_RATE << 24) | clk->id; + req.clk_set_rate.rate = rate; + + ret = tegra186_bpmp_call(clk->dev->parent, MRQ_CLK, + &req, sizeof(req), &resp, sizeof(resp)); + if (ret) + return ret; + + return resp.clk_set_rate.rate; +} + +static int tegra186_clk_en_dis(struct clk *clk, + enum mrq_reset_commands cmd) +{ + struct mrq_clk_request req; + struct mrq_clk_response resp; + + req.cmd_and_id = (cmd << 24) | clk->id; + + return tegra186_bpmp_call(clk->dev->parent, MRQ_CLK, + &req, sizeof(req), &resp, sizeof(resp)); +} + +static int tegra186_clk_enable(struct clk *clk) +{ + debug("%s(clk=%p) (dev=%p, id=%lu)\n", __func__, clk, clk->dev, + clk->id); + + return tegra186_clk_en_dis(clk, CMD_CLK_ENABLE); +} + +static int tegra186_clk_disable(struct clk *clk) +{ + debug("%s(clk=%p) (dev=%p, id=%lu)\n", __func__, clk, clk->dev, + clk->id); + + return tegra186_clk_en_dis(clk, CMD_CLK_DISABLE); +} + +static struct clk_ops tegra186_clk_ops = { + .get_rate = tegra186_clk_get_rate, + .set_rate = tegra186_clk_set_rate, + .enable = tegra186_clk_enable, + .disable = tegra186_clk_disable, +}; + +static int tegra186_clk_probe(struct udevice *dev) +{ + debug("%s(dev=%p)\n", __func__, dev); + + return 0; +} + +U_BOOT_DRIVER(tegra186_clk) = { + .name = "tegra186_clk", + .id = UCLASS_CLK, + .probe = tegra186_clk_probe, + .ops = &tegra186_clk_ops, +};