* [PATCH 0/3] soc: renesas: ARM errata updates
From: Geert Uytterhoeven @ 2019-08-21 12:45 UTC (permalink / raw)
To: Simon Horman, Magnus Damm
Cc: linux-renesas-soc, Geert Uytterhoeven, linux-arm-kernel
Hi all,
This patch series updates the handling of ARM errata for affected
Renesas SoCs.
The first patch enables the new ARM_ERRATA_814220 for Cortex-A7, using
Kconfig logic.
The second patch moves enablement of ARM_ERRATA_754322 for Cortex-A9
from shmobile_defconfig to Kconfig logic, to make sure it's always
enabled when needed.
The third patch disables PL310_ERRATA_588369, as it doesn't affect any
Renesas SoCs.
The last patch is marked RFC, as I don't know the revision of PL310 on
EMMA Mobile EV2, and cannot test it on EMEV2.
Thanks for your comments!
Geert Uytterhoeven (3):
soc: renesas: Enable ARM_ERRATA_814220 for affected Cortex-A7
soc: renesas: Enable ARM_ERRATA_754322 for affected Cortex-A9
[RFC] ARM: shmobile: defconfig: Disable PL310_ERRATA_588369
arch/arm/configs/shmobile_defconfig | 2 --
drivers/soc/renesas/Kconfig | 11 +++++++++++
2 files changed, 11 insertions(+), 2 deletions(-)
--
2.17.1
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3 2/9] soc: samsung: Convert exynos-chipid driver to use the regmap API
From: Sylwester Nawrocki @ 2019-08-21 12:41 UTC (permalink / raw)
To: Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz
Cc: devicetree, linux-samsung-soc@vger.kernel.org, Arnd Bergmann,
linux-pm, Sylwester Nawrocki, vireshk,
linux-kernel@vger.kernel.org, Jon Hunter, robh+dt, kgene,
pankaj.dubey, linux-tegra, linux-arm-kernel, Marek Szyprowski
In-Reply-To: <CAJKOXPdh9eHrAuCxHkQBvJMqEnUCeU2xwkK=9yyiJ6BuTLJ+_A@mail.gmail.com>
On 8/21/19 14:16, Krzysztof Kozlowski wrote:
>>> I'm also inclined to have it converted to a regular driver. We already
>>> have "exynos-asv" driver matching on the chipid node (patch 3/9).
>>> The ASV patches will not be merged soon anyway, all this needs some more
>>> thought. Krzysztof, can we abandon the chipid patches for now? Your
>>
>> chipid driver is good and useful on its own. The preferred solution
>> IMHO would be to just revert "soc: samsung: Convert exynos-chipid
>> driver to use the regmap API" commit.
>
> I queued the chipid as a dependency for ASV but ASV requires the
> regmap. What would be left after reverting the regmap part? Simple
> unused printk driver? No need for such. If reverting, then let's drop
> entire driver and rework it offline.
In fact there is now no dependency between the chipid and the ASV
driver (patch 3/9), the regmap is provided by the syscon driver/API.
I should have added "depends on REGMAP && MFD_SYSCON" to Kconfig.
Both drivers (chipid, ASV) share the registers region so the regmap
API seemed appropriate here.
Converting the chipid code to platform driver wouldn't make sense as
it wouldn't be useful early in arch/arm/mach-exynos and we can't have
two drivers for same device (the ASV driver matches on the chipid
compatible now).
--
Regards,
Sylwester
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 04/10] mailbox: sunxi-msgbox: Add a new mailbox driver
From: Maxime Ripard @ 2019-08-21 12:30 UTC (permalink / raw)
To: Samuel Holland
Cc: Mark Rutland, devicetree, linux-sunxi, Stephen Boyd,
Michael Turquette, Jassi Brar, linux-kernel, Chen-Yu Tsai,
Rob Herring, Corentin Labbe, linux-clk, linux-arm-kernel
In-Reply-To: <bc09e14c-1cf5-8124-fc34-c651b78577ce@sholland.org>
[-- Attachment #1.1: Type: text/plain, Size: 1225 bytes --]
On Tue, Aug 20, 2019 at 08:07:53AM -0500, Samuel Holland wrote:
> On 8/20/19 6:18 AM, Ondřej Jirman wrote:
> >> + reset = devm_reset_control_get(dev, NULL);
> >> + if (IS_ERR(reset)) {
> >> + ret = PTR_ERR(reset);
> >> + dev_err(dev, "Failed to get reset control: %d\n", ret);
> >> + goto err_disable_unprepare;
> >> + }
> >> +
> >> + ret = reset_control_deassert(reset);
> >> + if (ret) {
> >> + dev_err(dev, "Failed to deassert reset: %d\n", ret);
> >> + goto err_disable_unprepare;
> >> + }
> >
> > You need to assert the reset again from now on, in error paths. devm
> > will not do that for you.
>
> I know, and that's intentional. This same message box device is used for ATF to
> communicate with SCP firmware (on a different channel). This could be happening
> on a different core while Linux is running. So Linux is not allowed to deassert
> the reset. clk_disable_unprepare() is only okay because the clock is marked as
> critical.
I agree with Ondrej that since this is clearly not the standard use of
the API, this must have a big comment explaining why we're doing it
this way.
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 02/10] clk: sunxi-ng: Mark AR100 clocks as critical
From: Maxime Ripard @ 2019-08-21 12:24 UTC (permalink / raw)
To: Samuel Holland
Cc: Mark Rutland, devicetree, linux-sunxi, Stephen Boyd,
Michael Turquette, Jassi Brar, linux-kernel, Chen-Yu Tsai,
Rob Herring, Corentin Labbe, linux-clk, linux-arm-kernel
In-Reply-To: <3b67534a-eb1b-c1e8-b5e8-e0a74ae85792@sholland.org>
[-- Attachment #1.1: Type: text/plain, Size: 4574 bytes --]
On Tue, Aug 20, 2019 at 08:02:55AM -0500, Samuel Holland wrote:
> On 8/20/19 2:11 AM, Maxime Ripard wrote:
> > On Mon, Aug 19, 2019 at 10:23:03PM -0500, Samuel Holland wrote:
> >> On sun8i, sun9i, and sun50i SoCs, system suspend/resume support requires
> >> firmware running on the AR100 coprocessor (the "SCP"). Such firmware can
> >> provide additional features, such as thermal monitoring and poweron/off
> >> support for boards without a PMIC.
> >>
> >> Since the AR100 may be running critical firmware, even if Linux does not
> >> know about it or directly interact with it (all requests may go through
> >> an intermediary interface such as PSCI), Linux must not turn off its
> >> clock.
>
> This paragraph here is the key. The firmware won't necessarily have a device
> tree node, and in the current design it will not, since Linux never communicates
> with it directly. All communication goes through ATF via PSCI.
Sorry, I'm a bit lost in all those ARM firmware interfaces.
I thought SCPI was supposed to be a separate interface that had
nothing to do with PSCI?
Anyway, my main concern is that I don't really want to make exceptions
to the clock handling for everyone's usecase, and this creates a
precedent (and not even a permanent one, since eventually the plan is
to have all the clock handling happening in the firmware, right?).
There's the protected-clocks property in the DT though that will
achieve the same goal. The code to deal with it is not generic at the
moment, but it could be made that way. Would patching the DT to
protect the clock you care about, when you care about protecting them,
be an option for you?
> >> At this time, such power management firmware only exists for the A64 and
> >> H5 SoCs. However, it makes sense to take care of all CCU drivers now
> >> for consistency, and to ease the transition in the future once firmware
> >> is ported to the other SoCs.
> >>
> >> Leaving the clock running is safe even if no firmware is present, since
> >> the AR100 stays in reset by default. In most cases, the AR100 clock is
> >> kept enabled by Linux anyway, since it is the parent of all APB0 bus
> >> peripherals. This change only prevents Linux from turning off the AR100
> >> clock in the rare case that no peripherals are in use.
> >>
> >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >
> > So I'm not really sure where you want to go with this.
> >
> > That clock is only useful where you're having a firmware running on
> > the AR100, and that firmware would have a device tree node of its own,
> > where we could list the clocks needed for the firmware to keep
> > running, if it ever runs. If the driver has not been compiled in /
> > loaded, then we don't care either.
>
> See above. I don't expect that the firmware would have a device tree node,
> because the firmware doesn't need any Linux drivers.
>
> > But more fundamentally, if we're going to use SCPI, then those clocks
> > will not be handled by that driver anyway, but by the firmware, right?
>
> In the future, we might use SCPI clocks/sensors/regulators/etc. from Linux, but
> that's not the plan at the moment. Given that it's already been two years since
> I started this project, I'm trying to limit its scope so I can get at least some
> part merged. The first step is to integrate a firmware that provides
> suspend/resume functionality only. That firmware does implement SCPI, and if the
> top-level Linux SCPI driver worked with multiple mailbox channels, it could
> query the firmware's version and fetures. But all of the SCPI commands used for
> suspend/resume must go through ATF via PSCI.
I didn't know that you could / should do that with PSCI / SCPI.
> > So I'm not really sure that we should do it statically this way, and
> > that we should do it at all.
>
> Do you have a better way to model "firmware uses this clock behind the scenes,
> so Linux please don't touch it"? It's unfortunate that we have Linux and
> firmware fighting over the R_CCU, but since we didn't have firmware (e.g. SCPI
> clocks) in the beginning, it's where we are today.
>
> The AR100 clock doesn't actually have a gate, and it generally has dependencies
> like R_INTC in use. So as I mentioned in the commit message, the clock will
> normally be on anyway. The goal was to model the fact that there are users of
> this clock that Linux doesn't/can't know about.
Like I said, if that's an option, I'd prefer to have protected-clocks
work for everyone / for sunxi.
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3 2/9] soc: samsung: Convert exynos-chipid driver to use the regmap API
From: Krzysztof Kozlowski @ 2019-08-21 12:16 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: devicetree, linux-samsung-soc@vger.kernel.org, Arnd Bergmann,
linux-pm, Sylwester Nawrocki, vireshk,
linux-kernel@vger.kernel.org, Jon Hunter, robh+dt, kgene,
Sylwester Nawrocki, pankaj.dubey, linux-tegra, linux-arm-kernel,
Marek Szyprowski
In-Reply-To: <72eea1ea-2433-2f76-6265-5851554e845d@samsung.com>
On Wed, 21 Aug 2019 at 13:51, Bartlomiej Zolnierkiewicz
<b.zolnierkie@samsung.com> wrote:
> >>> Following this change, I am now seeing the above error on our Tegra
> >>> boards where this driver is enabled. This is triggering a kernel
> >>> warnings test we have to fail. Hence, I don't think that you can remove
> >>> the compatible node test here, unless you have a better way to determine
> >>> if this is a samsung device.
> >>
> >> Right, this is really wrong... I missed that it is not a probe but
> >> early init. And this init will be called on every board... Probably it
> >> should be converted to a regular driver.
>
> Early initialization is needed for SoC driver to be used from within
> arch/arm/mach-exynos/ and _initcall() usage is the usual way for SoC
> drivers to be initialized:
>
> drivers/soc/amlogic/meson-gx-socinfo.c
> drivers/soc/amlogic/meson-mx-socinfo.c
> drivers/soc/atmel/soc.c
> drivers/soc/bcm/brcmstb/common.c
> drivers/soc/imx/soc-imx-scu.c
> drivers/soc/imx/soc-imx8.c
> drivers/soc/renesas/renesas-soc.c
> drivers/soc/tegra/fuse/fuse-tegra.c
> drivers/soc/ux500/ux500-soc-id.c
> drivers/soc/versatile/soc-integrator.c
> drivers/soc/versatile/soc-integrator.c
>
> The only SoC drivers that are regular drivers are:
>
> drivers/soc/fsl/guts.c
> drivers/soc/versatile/soc-realview.c
Thanks for pointing it out.
Indeed, the initcall was needed in your set of patches here:
https://patchwork.kernel.org/project/linux-samsung-soc/list/?series=43565&state=*
but this work was not continued here. Maybe it will be later
resubmitted... maybe not... who knows? Therefore I would prefer proper
solution for current case (driver), unless patches for mach are being
brought back to life now.
> > I'm also inclined to have it converted to a regular driver. We already
> > have "exynos-asv" driver matching on the chipid node (patch 3/9).
> > The ASV patches will not be merged soon anyway, all this needs some more
> > thought. Krzysztof, can we abandon the chipid patches for now? Your
>
> chipid driver is good and useful on its own. The preferred solution
> IMHO would be to just revert "soc: samsung: Convert exynos-chipid
> driver to use the regmap API" commit.
I queued the chipid as a dependency for ASV but ASV requires the
regmap. What would be left after reverting the regmap part? Simple
unused printk driver? No need for such. If reverting, then let's drop
entire driver and rework it offline.
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Applied "ASoC: sun4i-i2s: Add support for TDM slots" to the asoc tree
From: Mark Brown @ 2019-08-21 12:15 UTC (permalink / raw)
To: Maxime Ripard
Cc: alsa-devel, lgirdwood, Maxime Ripard, linux-kernel, codekipper,
Chen-Yu Tsai, Mark Brown, linux-arm-kernel
In-Reply-To: <26392af30b3e7b31ee48d5b867d45be8675db046.1566242458.git-series.maxime.ripard@bootlin.com>
The patch
ASoC: sun4i-i2s: Add support for TDM slots
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.4
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
From 137befe19f310400a8b20fd8a4ce8c4141aafde0 Mon Sep 17 00:00:00 2001
From: Maxime Ripard <maxime.ripard@bootlin.com>
Date: Mon, 19 Aug 2019 21:25:27 +0200
Subject: [PATCH] ASoC: sun4i-i2s: Add support for TDM slots
The i2s controller supports TDM, for up to 8 slots. Let's support the TDM
API.
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
Link: https://lore.kernel.org/r/26392af30b3e7b31ee48d5b867d45be8675db046.1566242458.git-series.maxime.ripard@bootlin.com
Signed-off-by: Mark Brown <broonie@kernel.org>
---
sound/soc/sunxi/sun4i-i2s.c | 40 +++++++++++++++++++++++++++++++------
1 file changed, 34 insertions(+), 6 deletions(-)
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
index 9e691baee1e8..8326b8cfa569 100644
--- a/sound/soc/sunxi/sun4i-i2s.c
+++ b/sound/soc/sunxi/sun4i-i2s.c
@@ -168,6 +168,8 @@ struct sun4i_i2s {
struct reset_control *rst;
unsigned int mclk_freq;
+ unsigned int slots;
+ unsigned int slot_width;
struct snd_dmaengine_dai_dma_data capture_dma_data;
struct snd_dmaengine_dai_dma_data playback_dma_data;
@@ -287,7 +289,7 @@ static bool sun4i_i2s_oversample_is_valid(unsigned int oversample)
static int sun4i_i2s_set_clk_rate(struct snd_soc_dai *dai,
unsigned int rate,
- unsigned int channels,
+ unsigned int slots,
unsigned int word_size)
{
struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
@@ -335,7 +337,7 @@ static int sun4i_i2s_set_clk_rate(struct snd_soc_dai *dai,
bclk_parent_rate = i2s->variant->get_bclk_parent_rate(i2s);
bclk_div = sun4i_i2s_get_bclk_div(i2s, bclk_parent_rate,
- rate, channels, word_size);
+ rate, slots, word_size);
if (bclk_div < 0) {
dev_err(dai->dev, "Unsupported BCLK divider: %d\n", bclk_div);
return -EINVAL;
@@ -419,6 +421,10 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
const struct snd_pcm_hw_params *params)
{
unsigned int channels = params_channels(params);
+ unsigned int slots = channels;
+
+ if (i2s->slots)
+ slots = i2s->slots;
/* Map the channels for playback and capture */
regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_MAP_REG, 0x76543210);
@@ -428,7 +434,6 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG,
SUN4I_I2S_CHAN_SEL_MASK,
SUN4I_I2S_CHAN_SEL(channels));
-
regmap_update_bits(i2s->regmap, SUN8I_I2S_RX_CHAN_SEL_REG,
SUN4I_I2S_CHAN_SEL_MASK,
SUN4I_I2S_CHAN_SEL(channels));
@@ -452,10 +457,18 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)
{
struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
+ unsigned int word_size = params_width(params);
unsigned int channels = params_channels(params);
+ unsigned int slots = channels;
int ret, sr, wss;
u32 width;
+ if (i2s->slots)
+ slots = i2s->slots;
+
+ if (i2s->slot_width)
+ word_size = i2s->slot_width;
+
ret = i2s->variant->set_chan_cfg(i2s, params);
if (ret < 0) {
dev_err(dai->dev, "Invalid channel configuration\n");
@@ -477,15 +490,14 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
if (sr < 0)
return -EINVAL;
- wss = i2s->variant->get_wss(i2s, params_width(params));
+ wss = i2s->variant->get_wss(i2s, word_size);
if (wss < 0)
return -EINVAL;
regmap_field_write(i2s->field_fmt_wss, wss);
regmap_field_write(i2s->field_fmt_sr, sr);
- return sun4i_i2s_set_clk_rate(dai, params_rate(params),
- channels, params_width(params));
+ return sun4i_i2s_set_clk_rate(dai, params_rate(params), slots, word_size);
}
static int sun4i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
@@ -785,10 +797,26 @@ static int sun4i_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
return 0;
}
+static int sun4i_i2s_set_tdm_slot(struct snd_soc_dai *dai,
+ unsigned int tx_mask, unsigned int rx_mask,
+ int slots, int slot_width)
+{
+ struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
+
+ if (slots > 8)
+ return -EINVAL;
+
+ i2s->slots = slots;
+ i2s->slot_width = slot_width;
+
+ return 0;
+}
+
static const struct snd_soc_dai_ops sun4i_i2s_dai_ops = {
.hw_params = sun4i_i2s_hw_params,
.set_fmt = sun4i_i2s_set_fmt,
.set_sysclk = sun4i_i2s_set_sysclk,
+ .set_tdm_slot = sun4i_i2s_set_tdm_slot,
.trigger = sun4i_i2s_trigger,
};
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH 21/21] ASoC: sun4i-i2s: Add support for DSP formats
From: Mark Brown @ 2019-08-21 12:15 UTC (permalink / raw)
To: Maxime Ripard
Cc: alsa-devel, lgirdwood, linux-kernel, codekipper, Chen-Yu Tsai,
linux-arm-kernel
In-Reply-To: <74cc9562e056627e14f186089d349022b65f59e7.1566242458.git-series.maxime.ripard@bootlin.com>
[-- Attachment #1.1: Type: text/plain, Size: 282 bytes --]
On Mon, Aug 19, 2019 at 09:25:28PM +0200, Maxime Ripard wrote:
> From: Maxime Ripard <maxime.ripard@bootlin.com>
>
> In addition to the I2S format, the controller also supports the DSP_*
> formats.
This doesn't seem to apply against current code, please check and
resend.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [PATCH 0/2] nvmem: imx-ocotp: allow reads with arbitrary size and offset
From: Diaz de Grenu, Jose @ 2019-08-21 12:11 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: shawnguo@kernel.org, s.hauer@pengutronix.de,
linux-kernel@vger.kernel.org, linux-imx@nxp.com,
kernel@pengutronix.de, festevam@gmail.com,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <771a6f0a-3cc2-da20-2439-9a91dd2bf9d2@linaro.org>
On 06/08/2019 12:06 Srinivas Kandagatla wrote:
> Anyone form IMX can test this patchset before I push this out?
>
> Thanks,
> srini
Just for the record, I tested this on an i.MX6UL based board.
Let me know if there is something I can do to facilitate the testing to anyone from IMX.
Thanks.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 3/4] iommu/io-pgtable-arm: Rationalise TCR handling
From: Will Deacon @ 2019-08-21 12:11 UTC (permalink / raw)
To: Robin Murphy; +Cc: robdclark, joro, jcrouse, iommu, linux-arm-kernel
In-Reply-To: <8cc47f43-ad74-b4e2-e977-6c78780abc91@arm.com>
On Tue, Aug 20, 2019 at 07:41:52PM +0100, Robin Murphy wrote:
> On 20/08/2019 17:07, Will Deacon wrote:
> > On Tue, Aug 20, 2019 at 04:25:56PM +0100, Robin Murphy wrote:
> > > On 20/08/2019 11:31, Will Deacon wrote:
> > > > On Mon, Aug 19, 2019 at 07:19:30PM +0100, Robin Murphy wrote:
> > > > > Although it's conceptually nice for the io_pgtable_cfg to provide a
> > > > > standard VMSA TCR value, the reality is that no VMSA-compliant IOMMU
> > > > > looks exactly like an Arm CPU, and they all have various other TCR
> > > > > controls which io-pgtable can't be expected to understand. Thus since
> > > > > there is an expectation that drivers will have to add to the given TCR
> > > > > value anyway, let's strip it down to just the essentials that are
> > > > > directly relevant to io-pgatble's inner workings - namely the address
> > > > > sizes, walk attributes, and where appropriate, format selection.
> > > > >
> > > > > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > > > > ---
> > > > > drivers/iommu/arm-smmu-v3.c | 7 +------
> > > > > drivers/iommu/arm-smmu.c | 1 +
> > > > > drivers/iommu/arm-smmu.h | 2 ++
> > > > > drivers/iommu/io-pgtable-arm-v7s.c | 6 ++----
> > > > > drivers/iommu/io-pgtable-arm.c | 4 ----
> > > > > drivers/iommu/qcom_iommu.c | 2 +-
> > > > > 6 files changed, 7 insertions(+), 15 deletions(-)
> > > >
> > > > Hmm, so I'm a bit nervous about this one since I think we really should
> > > > be providing a TCR with EPD1 set if we're only giving you TTBR0. Relying
> > > > on the driver to do this worries me. See my comments on the next patch.
> > >
> > > The whole idea is that we already know we can't provide a *complete* TCR
> > > value (not least because anything above bit 31 is the wild west), thus
> > > there's really no point in io-pgtable trying to provide anything other than
> > > the parts it definitely controls. It makes sense to provide this partial TCR
> > > value "as if" for TTBR0, since that's the most common case, but ultimately
> > > io-pgatble doesn't know (or need to) which TTBR the caller intends to
> > > actually use for this table. Even if the caller *is* allocating it for
> > > TTBR0, io-pgtable doesn't know that they haven't got something live in TTBR1
> > > already, so it still wouldn't be in a position to make the EPD1 call either
> > > way.
> >
> > Ok, but the driver can happily rewrite/ignore what it gets back. I suppose
> > an alternative would be scrapped the 'u64 tcr' and instead having a bunch
> > of named bitfields for the stuff we're actually providing, although I'd
> > still like EPDx to be in there.
>
> I like the bitfield idea; it would certainly emphasise the "you have to do
> something more with this" angle that I'm pushing towards here, but still
> leave things framed in TCR terms without having to go to some more general
> abstraction. It really doesn't play into your EPD argument though - such a
> config would be providing TxSZ/TGx/IRGNx/ORGNx/SHx, but EPDy, for y = !x.
> For a driver to understand that and do the right thing with it is even more
> involved than for the driver to just set EPD1 by itself anyway.
Having considered the bitfield idea some more, I'm less attached to EPDx
because we simply wouldn't be making a statement about them, rather than a
(dangerous) zero value and expecting it to be ignored. So I think we're in
agreement on that.
The only part I'm still stuck to is that I think io-pgtable should know
whether it's targetting TTBR0 or TTBR1 so that it can sanitise input
addresses correctly. Doing this in the driver code is possible, but I'd
rather not start from that position, particularly as it would require things
like sign-extension in the TLBI callbacks.
> If only LPAE had created these bits as enables rather than disables then
> things would be logical and we could all be happy, but here we are...
I'm happy! :D:D:D
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 5/9] block: support diskcipher
From: Jens Axboe @ 2019-08-21 12:09 UTC (permalink / raw)
To: boojin.kim, linux-block, linux-kernel
Cc: 'Ulf Hansson', 'Mike Snitzer', dm-devel,
'Andreas Dilger', 'Alasdair Kergon',
'Eric Biggers', linux-samsung-soc, 'Herbert Xu',
'Krzysztof Kozlowski', 'Jaehoon Chung',
'Kukjin Kim', linux-ext4, 'Chao Yu',
linux-fscrypt, 'Jaegeuk Kim', linux-arm-kernel,
'Theodore Y. Ts'o', linux-mmc, linux-f2fs-devel,
linux-crypto, linux-fsdevel, 'David S. Miller'
In-Reply-To: <004101d557eb$98b00060$ca100120$@samsung.com>
On 8/21/19 12:42 AM, boojin.kim wrote:
> This patch supports crypto information to be maintained via BIO
> and passed to the storage driver.
>
> To do this, 'bi_aux_private', 'REQ_CYPTE' and 'bi_dun' are added
> to the block layer.
>
> 'bi_aux_private' is added for loading additional private information into
> BIO.
> 'REQ_CRYPT' is added to distinguish that bi_aux_private is being used
> for diskcipher.
> F2FS among encryption users uses DUN(device unit number) as
> the IV(initial vector) for cryptographic operations.
> DUN is stored in 'bi_dun' of bi_iter as a specific value for each BIO.
>
> Before attempting to merge the two BIOs, the operation is also added to
> verify that the crypto information contained in two BIOs is consistent.
This isn't going to happen. With this, and the inline encryption
proposed by Google, we'll bloat the bio even more. At least the Google
approach didn't include bio iter changes as well.
Please work it out between yourselves so we can have a single, clean
abstraction that works for both.
--
Jens Axboe
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 20/21] ASoC: sun4i-i2s: Add support for TDM slots
From: Mark Brown @ 2019-08-21 12:08 UTC (permalink / raw)
To: Maxime Ripard
Cc: alsa-devel, linux-kernel, lgirdwood, codekipper, Chen-Yu Tsai,
linux-arm-kernel, Sergey Suloev
In-Reply-To: <20190821120551.r4b3h4nnet357wem@flea>
[-- Attachment #1.1: Type: text/plain, Size: 972 bytes --]
On Wed, Aug 21, 2019 at 02:05:51PM +0200, Maxime Ripard wrote:
> On Tue, Aug 20, 2019 at 08:46:30AM +0300, Sergey Suloev wrote:
Please delete unneeded context from mails when replying. Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.
> > > .set_sysclk = sun4i_i2s_set_sysclk,
> > > + .set_tdm_slot = sun4i_i2s_set_tdm_slot,
> > > .trigger = sun4i_i2s_trigger,
> > > };
> > it seems like you forgot to implement sun4i_i2s_dai_ops.set_bclk_ratio
> > because, as I far as I understand, it should alter tdm slots functionality
> > indirectly.
> As far as I can see, while this indeed changes a few things on the TDM
> setup, it's optional, orthogonal and it has a single user in the tree
> (some intel platform card).
> So I'd say that if someone ever needs it, we can have it, but it's not
> a blocker.
Yes, that's a compltely orthogonal callback.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 03/10] dt-bindings: mailbox: Add a sunxi message box binding
From: Maxime Ripard @ 2019-08-21 12:07 UTC (permalink / raw)
To: Samuel Holland
Cc: Mark Rutland, devicetree, linux-sunxi, Rob Herring, Stephen Boyd,
Michael Turquette, Jassi Brar, linux-kernel, Chen-Yu Tsai,
Rob Herring, Corentin Labbe, linux-clk, linux-arm-kernel
In-Reply-To: <8947f4d1-3bb4-11b8-b114-5016339514b8@sholland.org>
[-- Attachment #1.1: Type: text/plain, Size: 1012 bytes --]
On Tue, Aug 20, 2019 at 08:04:26AM -0500, Samuel Holland wrote:
> On 8/20/19 2:14 AM, Maxime Ripard wrote:
> > Hi,
> >
> > On Mon, Aug 19, 2019 at 10:23:04PM -0500, Samuel Holland wrote:
> >> This mailbox hardware is present in Allwinner sun8i, sun9i, and sun50i
> >> SoCs. Add a device tree binding for it.
> >>
> >> Reviewed-by: Rob Herring <robh@kernel.org>
> >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >> ---
> >> .../mailbox/allwinner,sunxi-msgbox.yaml | 79 +++++++++++++++++++
> >> 1 file changed, 79 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/mailbox/allwinner,sunxi-msgbox.yaml
> >
> > So we merged a bunch of schemas already, with the convention that the
> > name was the first compatible to use that binding.
> >
> > That would be allwinner,sun6i-a31-msgbox.yaml in that case
>
> Okay, I'll rename the binding and driver (and Kconfig symbol?).
Yep, thanks!
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 20/21] ASoC: sun4i-i2s: Add support for TDM slots
From: Maxime Ripard @ 2019-08-21 12:05 UTC (permalink / raw)
To: Sergey Suloev
Cc: alsa-devel, lgirdwood, linux-kernel, codekipper, Chen-Yu Tsai,
broonie, linux-arm-kernel
In-Reply-To: <c311e88a-fdd2-8a01-275e-675d98dc90ba@orpaltech.com>
[-- Attachment #1.1: Type: text/plain, Size: 5143 bytes --]
Hi,
On Tue, Aug 20, 2019 at 08:46:30AM +0300, Sergey Suloev wrote:
> Hi, Maxime,
>
> On 8/19/19 10:25 PM, Maxime Ripard wrote:
> > From: Maxime Ripard <maxime.ripard@bootlin.com>
> >
> > The i2s controller supports TDM, for up to 8 slots. Let's support the TDM
> > API.
> >
> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > ---
> > sound/soc/sunxi/sun4i-i2s.c | 40 ++++++++++++++++++++++++++++++++------
> > 1 file changed, 34 insertions(+), 6 deletions(-)
> >
> > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> > index 0dac09814b65..4f76daeaaed7 100644
> > --- a/sound/soc/sunxi/sun4i-i2s.c
> > +++ b/sound/soc/sunxi/sun4i-i2s.c
> > @@ -168,6 +168,8 @@ struct sun4i_i2s {
> > struct reset_control *rst;
> > unsigned int mclk_freq;
> > + unsigned int slots;
> > + unsigned int slot_width;
> > struct snd_dmaengine_dai_dma_data capture_dma_data;
> > struct snd_dmaengine_dai_dma_data playback_dma_data;
> > @@ -287,7 +289,7 @@ static bool sun4i_i2s_oversample_is_valid(unsigned int oversample)
> > static int sun4i_i2s_set_clk_rate(struct snd_soc_dai *dai,
> > unsigned int rate,
> > - unsigned int channels,
> > + unsigned int slots,
> > unsigned int word_size)
> > {
> > struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > @@ -335,7 +337,7 @@ static int sun4i_i2s_set_clk_rate(struct snd_soc_dai *dai,
> > bclk_parent_rate = i2s->variant->get_bclk_parent_rate(i2s);
> > bclk_div = sun4i_i2s_get_bclk_div(i2s, bclk_parent_rate,
> > - rate, channels, word_size);
> > + rate, slots, word_size);
> > if (bclk_div < 0) {
> > dev_err(dai->dev, "Unsupported BCLK divider: %d\n", bclk_div);
> > return -EINVAL;
> > @@ -419,6 +421,10 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> > const struct snd_pcm_hw_params *params)
> > {
> > unsigned int channels = params_channels(params);
> > + unsigned int slots = channels;
> > +
> > + if (i2s->slots)
> > + slots = i2s->slots;
> > /* Map the channels for playback and capture */
> > regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_MAP_REG, 0x76543210);
> > @@ -428,7 +434,6 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s,
> > regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG,
> > SUN4I_I2S_CHAN_SEL_MASK,
> > SUN4I_I2S_CHAN_SEL(channels));
> > -
> > regmap_update_bits(i2s->regmap, SUN8I_I2S_RX_CHAN_SEL_REG,
> > SUN4I_I2S_CHAN_SEL_MASK,
> > SUN4I_I2S_CHAN_SEL(channels));
> > @@ -452,10 +457,18 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
> > struct snd_soc_dai *dai)
> > {
> > struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > + unsigned int word_size = params_width(params);
> > unsigned int channels = params_channels(params);
> > + unsigned int slots = channels;
> > int ret, sr, wss;
> > u32 width;
> > + if (i2s->slots)
> > + slots = i2s->slots;
> > +
> > + if (i2s->slot_width)
> > + word_size = i2s->slot_width;
> > +
> > ret = i2s->variant->set_chan_cfg(i2s, params);
> > if (ret < 0) {
> > dev_err(dai->dev, "Invalid channel configuration\n");
> > @@ -477,15 +490,14 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
> > if (sr < 0)
> > return -EINVAL;
> > - wss = i2s->variant->get_wss(i2s, params_width(params));
> > + wss = i2s->variant->get_wss(i2s, word_size);
> > if (wss < 0)
> > return -EINVAL;
> > regmap_field_write(i2s->field_fmt_wss, wss);
> > regmap_field_write(i2s->field_fmt_sr, sr);
> > - return sun4i_i2s_set_clk_rate(dai, params_rate(params),
> > - channels, params_width(params));
> > + return sun4i_i2s_set_clk_rate(dai, params_rate(params), slots, word_size);
> > }
> > static int sun4i_i2s_set_soc_fmt(const struct sun4i_i2s *i2s,
> > @@ -785,10 +797,26 @@ static int sun4i_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
> > return 0;
> > }
> > +static int sun4i_i2s_set_tdm_slot(struct snd_soc_dai *dai,
> > + unsigned int tx_mask, unsigned int rx_mask,
> > + int slots, int slot_width)
> > +{
> > + struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> > +
> > + if (slots > 8)
> > + return -EINVAL;
> > +
> > + i2s->slots = slots;
> > + i2s->slot_width = slot_width;
> > +
> > + return 0;
> > +}
> > +
> > static const struct snd_soc_dai_ops sun4i_i2s_dai_ops = {
> > .hw_params = sun4i_i2s_hw_params,
> > .set_fmt = sun4i_i2s_set_fmt,
> > .set_sysclk = sun4i_i2s_set_sysclk,
> > + .set_tdm_slot = sun4i_i2s_set_tdm_slot,
> > .trigger = sun4i_i2s_trigger,
> > };
>
> it seems like you forgot to implement sun4i_i2s_dai_ops.set_bclk_ratio
> because, as I far as I understand, it should alter tdm slots functionality
> indirectly.
As far as I can see, while this indeed changes a few things on the TDM
setup, it's optional, orthogonal and it has a single user in the tree
(some intel platform card).
So I'd say that if someone ever needs it, we can have it, but it's not
a blocker.
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 16/59] KVM: arm64: nv: Save/Restore vEL2 sysregs
From: Alexandru Elisei @ 2019-08-21 11:57 UTC (permalink / raw)
To: Marc Zyngier, linux-arm-kernel, kvmarm, kvm; +Cc: Andre Przywara, Dave Martin
In-Reply-To: <20190621093843.220980-17-marc.zyngier@arm.com>
On 6/21/19 10:38 AM, Marc Zyngier wrote:
> From: Andre Przywara <andre.przywara@arm.com>
>
> Whenever we need to restore the guest's system registers to the CPU, we
> now need to take care of the EL2 system registers as well. Most of them
> are accessed via traps only, but some have an immediate effect and also
> a guest running in VHE mode would expect them to be accessible via their
> EL1 encoding, which we do not trap.
>
> Split the current __sysreg_{save,restore}_el1_state() functions into
> handling common sysregs, then differentiate between the guest running in
> vEL2 and vEL1.
>
> For vEL2 we write the virtual EL2 registers with an identical format directly
> into their EL1 counterpart, and translate the few registers that have a
> different format for the same effect on the execution when running a
> non-VHE guest guest hypervisor.
>
> [ Commit message reworked and many bug fixes applied by Marc Zyngier
> and Christoffer Dall. ]
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> ---
> arch/arm64/kvm/hyp/sysreg-sr.c | 160 +++++++++++++++++++++++++++++++--
> 1 file changed, 153 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
> index 62866a68e852..2abb9c3ff24f 100644
> --- a/arch/arm64/kvm/hyp/sysreg-sr.c
> +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
> @@ -22,6 +22,7 @@
> #include <asm/kvm_asm.h>
> #include <asm/kvm_emulate.h>
> #include <asm/kvm_hyp.h>
> +#include <asm/kvm_nested.h>
>
> /*
> * Non-VHE: Both host and guest must save everything.
> @@ -51,11 +52,9 @@ static void __hyp_text __sysreg_save_user_state(struct kvm_cpu_context *ctxt)
> ctxt->sys_regs[TPIDRRO_EL0] = read_sysreg(tpidrro_el0);
> }
>
> -static void __hyp_text __sysreg_save_el1_state(struct kvm_cpu_context *ctxt)
> +static void __hyp_text __sysreg_save_vel1_state(struct kvm_cpu_context *ctxt)
> {
> - ctxt->sys_regs[CSSELR_EL1] = read_sysreg(csselr_el1);
> ctxt->sys_regs[SCTLR_EL1] = read_sysreg_el1(SYS_SCTLR);
> - ctxt->sys_regs[ACTLR_EL1] = read_sysreg(actlr_el1);
> ctxt->sys_regs[CPACR_EL1] = read_sysreg_el1(SYS_CPACR);
> ctxt->sys_regs[TTBR0_EL1] = read_sysreg_el1(SYS_TTBR0);
> ctxt->sys_regs[TTBR1_EL1] = read_sysreg_el1(SYS_TTBR1);
> @@ -69,14 +68,58 @@ static void __hyp_text __sysreg_save_el1_state(struct kvm_cpu_context *ctxt)
> ctxt->sys_regs[CONTEXTIDR_EL1] = read_sysreg_el1(SYS_CONTEXTIDR);
> ctxt->sys_regs[AMAIR_EL1] = read_sysreg_el1(SYS_AMAIR);
> ctxt->sys_regs[CNTKCTL_EL1] = read_sysreg_el1(SYS_CNTKCTL);
> - ctxt->sys_regs[PAR_EL1] = read_sysreg(par_el1);
> - ctxt->sys_regs[TPIDR_EL1] = read_sysreg(tpidr_el1);
>
> ctxt->gp_regs.sp_el1 = read_sysreg(sp_el1);
> ctxt->gp_regs.elr_el1 = read_sysreg_el1(SYS_ELR);
> ctxt->gp_regs.spsr[KVM_SPSR_EL1]= read_sysreg_el1(SYS_SPSR);
> }
>
> +static void __sysreg_save_vel2_state(struct kvm_cpu_context *ctxt)
> +{
> + ctxt->sys_regs[ESR_EL2] = read_sysreg_el1(SYS_ESR);
> + ctxt->sys_regs[AFSR0_EL2] = read_sysreg_el1(SYS_AFSR0);
> + ctxt->sys_regs[AFSR1_EL2] = read_sysreg_el1(SYS_AFSR1);
> + ctxt->sys_regs[FAR_EL2] = read_sysreg_el1(SYS_FAR);
> + ctxt->sys_regs[MAIR_EL2] = read_sysreg_el1(SYS_MAIR);
> + ctxt->sys_regs[VBAR_EL2] = read_sysreg_el1(SYS_VBAR);
> + ctxt->sys_regs[CONTEXTIDR_EL2] = read_sysreg_el1(SYS_CONTEXTIDR);
> + ctxt->sys_regs[AMAIR_EL2] = read_sysreg_el1(SYS_AMAIR);
> +
> + /*
> + * In VHE mode those registers are compatible between EL1 and EL2,
> + * and the guest uses the _EL1 versions on the CPU naturally.
> + * So we save them into their _EL2 versions here.
> + * For nVHE mode we trap accesses to those registers, so our
> + * _EL2 copy in sys_regs[] is always up-to-date and we don't need
> + * to save anything here.
> + */
> + if (__vcpu_el2_e2h_is_set(ctxt)) {
> + ctxt->sys_regs[SCTLR_EL2] = read_sysreg_el1(SYS_SCTLR);
> + ctxt->sys_regs[CPTR_EL2] = read_sysreg_el1(SYS_CPACR);
> + ctxt->sys_regs[TTBR0_EL2] = read_sysreg_el1(SYS_TTBR0);
> + ctxt->sys_regs[TTBR1_EL2] = read_sysreg_el1(SYS_TTBR1);
> + ctxt->sys_regs[TCR_EL2] = read_sysreg_el1(SYS_TCR);
> + ctxt->sys_regs[CNTHCTL_EL2] = read_sysreg_el1(SYS_CNTKCTL);
> + }
This can break guests that run with VHE on, then disable it. I stumbled into
this while working on kvm-unit-tests, which uses TTBR0 for the translation
tables. Let's consider the following scenario:
1. Guest sets HCR_EL2.E2H
2. Guest programs translation tables in TTBR0_EL1, which should reflect in
TTBR0_EL2.
3. Guest enabled MMU and does stuff.
4. Guest disables MMU and clears HCR_EL2.E2H
5. Guest turns MMU on. It doesn't change TTBR0_EL2, because it will use the same
translation tables as when running with E2H set.
6. The vcpu gets scheduled out. E2H is not set, so the value that the guest
programmed in hardware TTBR0_EL1 won't be copied to virtual TTBR0_EL2.
7. The vcpu gets scheduled back in. KVM will write the reset value for virtual
TTBR0_EL2 (which is 0x0).
8. The guest hangs.
I think this is actually a symptom of a deeper issue. When E2H is cleared, the
values that the guest wrote to the EL1 registers aren't immediately reflected in
the virtual EL2 registers, as it happens on real hardware. Instead, some of the
hardware values from the EL1 registers are copied to the corresponding EL2
registers on the next vcpu_put, which happens at a later time.
I am thinking that something like this will fix the issues (it did fix disabling
VHE in kvm-unit-tests):
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 1d896113f1f8..f2b5a39762d0 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -333,7 +333,8 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, int reg)
* to reverse-translate virtual EL2 system registers for a
* non-VHE guest hypervisor.
*/
- __vcpu_sys_reg(vcpu, reg) = val;
+ if (reg != HCR_EL2)
+ __vcpu_sys_reg(vcpu, reg) = val;
switch (reg) {
case ELR_EL2:
@@ -370,7 +371,17 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, int
reg)
return;
memory_write:
- __vcpu_sys_reg(vcpu, reg) = val;
+ if (reg == HCR_EL2 && vcpu_el2_e2h_is_set(vcpu) && !(val & HCR_E2H)) {
+ preempt_disable();
+ kvm_arch_vcpu_put(vcpu);
+
+ __vcpu_sys_reg(vcpu, reg) = val;
+
+ kvm_arch_vcpu_load(vcpu, smp_processor_id());
+ preempt_enable();
+ } else {
+ __vcpu_sys_reg(vcpu, reg) = val;
+ }
}
/* 3 bits per cache level, as per CLIDR, but non-existent caches always 0 */
I don't think there's any need to convert EL1 registers to their non-vhe EL2
format because of how RES0/RES1 is defined in the architecture glossary (ARM DDI
0487E.a, page 7893 for RES0 and 7894 for RES1):
"If a bit is RES0 only in some contexts:
A read of the bit must return the value last successfully written to the bit, by
either a direct or an indirect write, regardless of the use of the register when
the bit was written
[..]
While the use of the register is such that the bit is described as RES0, the
value of the bit must have no effect on the operation of the PE, other than
determining the value read back from that bit, unless this Manual explicitly
defines additional properties for the bit"
We have the translate functions that should take care of converting the non-vhe
EL2 format to the hardware EL1 format.
As an aside, the diff looks weird because the vcpu_write_sys_reg is very
complex, there are a LOT of exit points from the function, and the register
value is written twice for some registers. I think it's worth considering making
the function simpler, maybe splitting it into two separate functions, one for
EL2 registers, one for regular registers.
Here's the kvm-unit-tests diff that I used to spot the bug. It's very far from
being correct, but the test is able to finish with the fix (it hangs otherwise).
You can apply it on top of 2130fd4154ad ("tscdeadline_latency: Check condition
first before loop"):
diff --git a/arm/cstart64.S b/arm/cstart64.S
index b0e8baa1a23a..01357b3b116b 100644
--- a/arm/cstart64.S
+++ b/arm/cstart64.S
@@ -51,6 +51,18 @@ start:
b 1b
1:
+ mrs x4, CurrentEL
+ cmp x4, CurrentEL_EL2
+ b.ne 1f
+ mrs x4, mpidr_el1
+ msr vmpidr_el2, x4
+ mrs x4, midr_el1
+ msr vpidr_el2, x4
+ ldr x4, =(HCR_EL2_TGE | HCR_EL2_E2H)
+ msr hcr_el2, x4
+ isb
+
+1:
/* set up stack */
mov x4, #1
msr spsel, x4
@@ -101,6 +113,18 @@ get_mmu_off:
.globl secondary_entry
secondary_entry:
+ mrs x0, CurrentEL
+ cmp x0, CurrentEL_EL2
+ b.ne 1f
+ mrs x0, mpidr_el1
+ msr vmpidr_el2, x0
+ mrs x0, midr_el1
+ msr vpidr_el2, x0
+ ldr x0, =(HCR_EL2_TGE | HCR_EL2_E2H)
+ msr hcr_el2, x0
+ isb
+
+1:
/* Enable FP/ASIMD */
mov x0, #(3 << 20)
msr cpacr_el1, x0
@@ -194,6 +218,33 @@ asm_mmu_enable:
ret
+asm_mmu_enable_hyp:
+ ic iallu
+ tlbi alle2is
+ dsb ish
+
+ /* TCR */
+ ldr x1, =TCR_EL2_RES1 | \
+ TCR_T0SZ(VA_BITS) | \
+ TCR_TG0_64K | \
+ TCR_IRGN0_WBWA | TCR_ORGN0_WBWA | \
+ TCR_SH0_IS
+ mrs x2, id_aa64mmfr0_el1
+ bfi x1, x2, #TCR_EL2_PS_SHIFT, #3
+ msr tcr_el2, x1
+
+ /* Same MAIR and TTBR0 as in VHE mode */
+
+ /* SCTLR */
+ ldr x1, =SCTLR_EL2_RES1 | \
+ SCTLR_EL2_C | \
+ SCTLR_EL2_I | \
+ SCTLR_EL2_M
+ msr sctlr_el2, x1
+ isb
+
+ ret
+
.globl asm_mmu_disable
asm_mmu_disable:
mrs x0, sctlr_el1
@@ -202,6 +253,18 @@ asm_mmu_disable:
isb
ret
+.globl asm_disable_vhe
+asm_disable_vhe:
+ str x30, [sp, #-16]!
+
+ bl asm_mmu_disable
+ msr hcr_el2, xzr
+ isb
+ bl asm_mmu_enable_hyp
+
+ ldr x30, [sp], #16
+ ret
+
/*
* Vectors
* Adapted from arch/arm64/kernel/entry.S
diff --git a/arm/selftest.c b/arm/selftest.c
index 28a17f7a7531..68a18036221b 100644
--- a/arm/selftest.c
+++ b/arm/selftest.c
@@ -287,6 +287,12 @@ static void user_psci_system_off(struct pt_regs *regs,
unsigned int esr)
{
__user_psci_system_off();
}
+
+extern void asm_disable_vhe(void);
+static void check_el2(void)
+{
+ asm_disable_vhe();
+}
#endif
static void check_vectors(void *arg __unused)
@@ -369,6 +375,10 @@ int main(int argc, char **argv)
report("PSCI version", psci_check());
on_cpus(cpu_report, NULL);
+ } else if (strcmp(argv[1], "el2") == 0) {
+
+ check_el2();
+
} else {
printf("Unknown subtest\n");
abort();
diff --git a/lib/arm/psci.c b/lib/arm/psci.c
index c3d399064ae3..5ef1c0386ce1 100644
--- a/lib/arm/psci.c
+++ b/lib/arm/psci.c
@@ -16,7 +16,7 @@ int psci_invoke(unsigned long function_id, unsigned long arg0,
unsigned long arg1, unsigned long arg2)
{
asm volatile(
- "hvc #0"
+ "smc #0"
: "+r" (function_id)
: "r" (arg0), "r" (arg1), "r" (arg2));
return function_id;
diff --git a/lib/arm64/asm/pgtable-hwdef.h b/lib/arm64/asm/pgtable-hwdef.h
index 045a3ce12645..6b80e34dda0c 100644
--- a/lib/arm64/asm/pgtable-hwdef.h
+++ b/lib/arm64/asm/pgtable-hwdef.h
@@ -95,18 +95,42 @@
/*
* TCR flags.
*/
-#define TCR_TxSZ(x) (((UL(64) - (x)) << 16) | ((UL(64) - (x)) << 0))
-#define TCR_IRGN_NC ((UL(0) << 8) | (UL(0) << 24))
-#define TCR_IRGN_WBWA ((UL(1) << 8) | (UL(1) << 24))
-#define TCR_IRGN_WT ((UL(2) << 8) | (UL(2) << 24))
-#define TCR_IRGN_WBnWA ((UL(3) << 8) | (UL(3) << 24))
-#define TCR_IRGN_MASK ((UL(3) << 8) | (UL(3) << 24))
-#define TCR_ORGN_NC ((UL(0) << 10) | (UL(0) << 26))
-#define TCR_ORGN_WBWA ((UL(1) << 10) | (UL(1) << 26))
-#define TCR_ORGN_WT ((UL(2) << 10) | (UL(2) << 26))
-#define TCR_ORGN_WBnWA ((UL(3) << 10) | (UL(3) << 26))
-#define TCR_ORGN_MASK ((UL(3) << 10) | (UL(3) << 26))
-#define TCR_SHARED ((UL(3) << 12) | (UL(3) << 28))
+#define TCR_T0SZ(x) ((UL(64) - (x)) << 0)
+#define TCR_T1SZ(x) ((UL(64) - (x)) << 16)
+#define TCR_TxSZ(x) (TCR_T0SZ(x) | TCR_T1SZ(x))
+#define TCR_IRGN0_NC (UL(0) << 8)
+#define TCR_IRGN1_NC (UL(0) << 24)
+#define TCR_IRGN_NC (TCR_IRGN0_NC | TCR_IRGN1_NC)
+#define TCR_IRGN0_WBWA (UL(1) << 8)
+#define TCR_IRGN1_WBWA (UL(1) << 24)
+#define TCR_IRGN_WBWA (TCR_IRGN0_WBWA | TCR_IRGN1_WBWA)
+#define TCR_IRGN0_WT (UL(2) << 8)
+#define TCR_IRGN1_WT (UL(2) << 24)
+#define TCR_IRGN_WT (TCR_IRGN0_WT | TCR_IRGN1_WT)
+#define TCR_IRGN0_WBnWA (UL(3) << 8)
+#define TCR_IRGN1_WBnWA (UL(3) << 24)
+#define TCR_IRGN_WBnWA (TCR_IRGN0_WBnWA | TCR_IRGN1_WBnWA)
+#define TCR_IRGN0_MASK (UL(3) << 8)
+#define TCR_IRGN1_MASK (UL(3) << 24)
+#define TCR_IRGN_MASK (TCR_IRGN0_MASK | TCR_IRGN1_MASK)
+#define TCR_ORGN0_NC (UL(0) << 10)
+#define TCR_ORGN1_NC (UL(0) << 26)
+#define TCR_ORGN_NC (TCR_ORGN0_NC | TCR_ORGN1_NC)
+#define TCR_ORGN0_WBWA (UL(1) << 10)
+#define TCR_ORGN1_WBWA (UL(1) << 26)
+#define TCR_ORGN_WBWA (TCR_ORGN0_WBWA | TCR_ORGN1_WBWA)
+#define TCR_ORGN0_WT (UL(2) << 10)
+#define TCR_ORGN1_WT (UL(2) << 26)
+#define TCR_ORGN_WT (TCR_ORGN0_WT | TCR_ORGN1_WT)
+#define TCR_ORGN0_WBnWA (UL(3) << 8)
+#define TCR_ORGN1_WBnWA (UL(3) << 24)
+#define TCR_ORGN_WBnWA (TCR_ORGN0_WBnWA | TCR_ORGN1_WBnWA)
+#define TCR_ORGN0_MASK (UL(3) << 10)
+#define TCR_ORGN1_MASK (UL(3) << 26)
+#define TCR_ORGN_MASK (TCR_ORGN0_MASK | TCR_ORGN1_MASK)
+#define TCR_SH0_IS (UL(3) << 12)
+#define TCR_SH1_IS (UL(3) << 28)
+#define TCR_SHARED (TCR_SH0_IS | TCR_SH1_IS)
#define TCR_TG0_4K (UL(0) << 14)
#define TCR_TG0_64K (UL(1) << 14)
#define TCR_TG0_16K (UL(2) << 14)
@@ -116,6 +140,9 @@
#define TCR_ASID16 (UL(1) << 36)
#define TCR_TBI0 (UL(1) << 37)
+#define TCR_EL2_RES1 ((UL(1) << 31) | (UL(1) << 23))
+#define TCR_EL2_PS_SHIFT 16
+
/*
* Memory types available.
*/
diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
index 1d9223f728a5..b2136acda743 100644
--- a/lib/arm64/asm/processor.h
+++ b/lib/arm64/asm/processor.h
@@ -16,6 +16,16 @@
#define SCTLR_EL1_A (1 << 1)
#define SCTLR_EL1_M (1 << 0)
+#define HCR_EL2_TGE (1 << 27)
+#define HCR_EL2_E2H (1 << 34)
+
+#define SCTLR_EL2_RES1 ((UL(3) << 28) | (UL(3) << 22) | \
+ (UL(1) << 18) | (UL(1) << 16) | \
+ (UL(1) << 11) | (UL(3) << 4))
+#define SCTLR_EL2_I SCTLR_EL1_I
+#define SCTLR_EL2_C SCTLR_EL1_C
+#define SCTLR_EL2_M SCTLR_EL1_M
+
#ifndef __ASSEMBLY__
#include <asm/ptrace.h>
#include <asm/esr.h>
To run it:
lkvm run -f selftest.flat -c 1 -m 128 -p el2 --nested --irqchip gicv3 --console
serial
Thanks,
Alex
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v7 1/5] dt-bindings: media: Add Allwinner A10 CSI binding
From: Maxime Ripard @ 2019-08-21 11:57 UTC (permalink / raw)
To: Sakari Ailus
Cc: Mark Rutland, devicetree, Rob Herring, linux-kernel, Chen-Yu Tsai,
Rob Herring, Hans Verkuil, Laurent Pinchart, Thomas Petazzoni,
Mauro Carvalho Chehab, Frank Rowand, linux-arm-kernel,
linux-media
In-Reply-To: <20190820114849.GD5123@paasikivi.fi.intel.com>
[-- Attachment #1.1: Type: text/plain, Size: 4165 bytes --]
Hi Sakari,
On Tue, Aug 20, 2019 at 02:48:49PM +0300, Sakari Ailus wrote:
> Hi Maxime,
>
> On Tue, Aug 20, 2019 at 01:24:32PM +0200, Maxime Ripard wrote:
> > From: Maxime Ripard <maxime.ripard@bootlin.com>
> >
> > The Allwinner A10 CMOS Sensor Interface is a camera capture interface also
> > used in later (A10s, A13, A20, R8 and GR8) SoCs.
> >
> > On some SoCs, like the A10, there's multiple instances of that controller,
> > with one instance supporting more channels and having an ISP.
> >
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > ---
> > Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 107 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml
> > new file mode 100644
> > index 000000000000..9000bca344f9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml
> > @@ -0,0 +1,107 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/arm/allwinner,sun4i-a10-csi.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Allwinner A10 CMOS Sensor Interface (CSI) Device Tree Bindings
> > +
> > +maintainers:
> > + - Chen-Yu Tsai <wens@csie.org>
> > + - Maxime Ripard <maxime.ripard@bootlin.com>
> > +
> > +description: |-
> > + The Allwinner A10 and later has a CMOS Sensor Interface to retrieve
> > + frames from a parallel or BT656 sensor.
> > +
> > +properties:
> > + compatible:
> > + const: allwinner,sun7i-a20-csi0
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + interrupts:
> > + maxItems: 1
> > +
> > + clocks:
> > + items:
> > + - description: The CSI interface clock
> > + - description: The CSI module clock
> > + - description: The CSI ISP clock
> > + - description: The CSI DRAM clock
> > +
> > + clock-names:
> > + items:
> > + - const: bus
> > + - const: mod
> > + - const: isp
> > + - const: ram
> > +
> > + resets:
> > + maxItems: 1
> > +
> > + port:
> > + type: object
> > + additionalProperties: false
> > +
> > + properties:
> > + endpoint:
> > + properties:
> > + bus-width:
> > + const: 8
> > + description: Number of data lines actively used.
>
> Are other values supported? If not, you could omit this.
It can also support 16 bits data input, but this description is
redundant anyway, I'll remove it.
> > +
> > + data-active: true
> > + hsync-active: true
> > + pclk-sample: true
> > + remote-endpoint: true
> > + vsync-active: true
> > +
> > + required:
> > + - bus-width
> > + - data-active
> > + - hsync-active
> > + - pclk-sample
> > + - remote-endpoint
> > + - vsync-active
>
> Some of these are not allowed in the Bt.656 mode (vsync-active and
> hsync-active) while they're required in Bt.601 mode. Is there a way to tell
> that in YAML-based bindings?
You could, but that would be more suited in another schemas. The way
schemas works is that you can have several layers of them, and each
being validated in isolation from the others.
Here, we're just listing the values usable by that binding, and it
will be used only to validate that binding.
Eventually, we'll want to have a video-interfaces schemas that will
apply to all the OF graph users, with those constraints defined.
This way, we can avoid a lot of duplication and just have the binding
description.
I guess I could just have the remote endpoint being required, and the
rest will be in the generic part.
> Similarly, video-interfaces.txt should be referenced from here, shouldn't
> it?
Sure.
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3 2/9] soc: samsung: Convert exynos-chipid driver to use the regmap API
From: Bartlomiej Zolnierkiewicz @ 2019-08-21 11:51 UTC (permalink / raw)
To: Sylwester Nawrocki, Krzysztof Kozlowski
Cc: devicetree, linux-samsung-soc@vger.kernel.org, Arnd Bergmann,
linux-pm, vireshk, linux-kernel@vger.kernel.org, Jon Hunter,
robh+dt, kgene, Sylwester Nawrocki, pankaj.dubey, linux-tegra,
linux-arm-kernel, Marek Szyprowski
In-Reply-To: <1e428c8e-f4b5-0810-77f9-2c899c040fc7@kernel.org>
Hi,
On 8/20/19 11:38 PM, Sylwester Nawrocki wrote:
> On 8/20/19 21:37, Krzysztof Kozlowski wrote:
>>>> diff --git a/drivers/soc/samsung/exynos-chipid.c b/drivers/soc/samsung/exynos-chipid.c
>
>>>> @@ -51,29 +48,24 @@ static const char * __init product_id_to_soc_id(unsigned int product_id)
>>>> int __init exynos_chipid_early_init(void)
>>>> {
>>>> struct soc_device_attribute *soc_dev_attr;
>>>> - void __iomem *exynos_chipid_base;
>>>> struct soc_device *soc_dev;
>>>> struct device_node *root;
>>>> - struct device_node *np;
>>>> + struct regmap *regmap;
>>>> u32 product_id;
>>>> u32 revision;
>>>> + int ret;
>>>>
>>>> - /* look up for chipid node */
>>>> - np = of_find_compatible_node(NULL, NULL, "samsung,exynos4210-chipid");
>>>> - if (!np)
>>>> - return -ENODEV;
>>>> -
>>>> - exynos_chipid_base = of_iomap(np, 0);
>>>> - of_node_put(np);
>>>> -
>>>> - if (!exynos_chipid_base) {
>>>> - pr_err("Failed to map SoC chipid\n");
>>>> - return -ENXIO;
>>>> + regmap = syscon_regmap_lookup_by_compatible("samsung,exynos4210-chipid");
>>>> + if (IS_ERR(regmap)) {
>>>> + pr_err("Failed to get CHIPID regmap\n");
>>>> + return PTR_ERR(regmap);
>>>> }
>>> Following this change, I am now seeing the above error on our Tegra
>>> boards where this driver is enabled. This is triggering a kernel
>>> warnings test we have to fail. Hence, I don't think that you can remove
>>> the compatible node test here, unless you have a better way to determine
>>> if this is a samsung device.
>>
>> Right, this is really wrong... I missed that it is not a probe but
>> early init. And this init will be called on every board... Probably it
>> should be converted to a regular driver.
Early initialization is needed for SoC driver to be used from within
arch/arm/mach-exynos/ and _initcall() usage is the usual way for SoC
drivers to be initialized:
drivers/soc/amlogic/meson-gx-socinfo.c
drivers/soc/amlogic/meson-mx-socinfo.c
drivers/soc/atmel/soc.c
drivers/soc/bcm/brcmstb/common.c
drivers/soc/imx/soc-imx-scu.c
drivers/soc/imx/soc-imx8.c
drivers/soc/renesas/renesas-soc.c
drivers/soc/tegra/fuse/fuse-tegra.c
drivers/soc/ux500/ux500-soc-id.c
drivers/soc/versatile/soc-integrator.c
drivers/soc/versatile/soc-integrator.c
The only SoC drivers that are regular drivers are:
drivers/soc/fsl/guts.c
drivers/soc/versatile/soc-realview.c
> I'm also inclined to have it converted to a regular driver. We already
> have "exynos-asv" driver matching on the chipid node (patch 3/9).
> The ASV patches will not be merged soon anyway, all this needs some more
> thought. Krzysztof, can we abandon the chipid patches for now? Your
chipid driver is good and useful on its own. The preferred solution
IMHO would be to just revert "soc: samsung: Convert exynos-chipid
driver to use the regmap API" commit.
> pull request doesn't appear to be merged to arm-soc yet. Sorry about
> that.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH 5/5] arm64: dts: meson-sm1-sei610: add USB support
From: Neil Armstrong @ 2019-08-21 11:41 UTC (permalink / raw)
To: khilman, ulf.hansson
Cc: linux-amlogic, linux-pm, linux-kernel, linux-arm-kernel,
Neil Armstrong
In-Reply-To: <20190821114121.10430-1-narmstrong@baylibre.com>
Add the USB properties for the Amlogic SM1 Based SEI610 Board in order to
support the USB DRD Type-C port and the USB3 Type A port.
The USB DRD Type-C controller uses the ID signal to toggle the USB role
between the DWC3 Host controller and the DWC2 Device controller.
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts b/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts
index 66bd3bfbaf91..36ac2e4b970d 100644
--- a/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts
@@ -321,3 +321,8 @@
pinctrl-0 = <&uart_ao_a_pins>;
pinctrl-names = "default";
};
+
+&usb {
+ status = "okay";
+ dr_mode = "otg";
+};
--
2.22.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 1/5] dt-bindings: power: add Amlogic Everything-Else power domains bindings
From: Neil Armstrong @ 2019-08-21 11:41 UTC (permalink / raw)
To: khilman, ulf.hansson, devicetree
Cc: linux-amlogic, linux-pm, linux-kernel, linux-arm-kernel,
Neil Armstrong
In-Reply-To: <20190821114121.10430-1-narmstrong@baylibre.com>
Add the bindings for the Amlogic Everything-Else power domains,
controlling the Everything-Else peripherals power domains.
The bindings targets the Amlogic G12A and SM1 compatible SoCs,
support for earlier SoCs will be added later.
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
.../bindings/power/amlogic,meson-ee-pwrc.yaml | 93 +++++++++++++++++++
include/dt-bindings/power/meson-g12a-power.h | 13 +++
include/dt-bindings/power/meson-sm1-power.h | 18 ++++
3 files changed, 124 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power/amlogic,meson-ee-pwrc.yaml
create mode 100644 include/dt-bindings/power/meson-g12a-power.h
create mode 100644 include/dt-bindings/power/meson-sm1-power.h
diff --git a/Documentation/devicetree/bindings/power/amlogic,meson-ee-pwrc.yaml b/Documentation/devicetree/bindings/power/amlogic,meson-ee-pwrc.yaml
new file mode 100644
index 000000000000..aab70e8b681e
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/amlogic,meson-ee-pwrc.yaml
@@ -0,0 +1,93 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2019 BayLibre, SAS
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/power/amlogic,meson-ee-pwrc.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Amlogic Meson Everything-Else Power Domains
+
+maintainers:
+ - Neil Armstrong <narmstrong@baylibre.com>
+
+description: |+
+ The Everything-Else Power Domains node should be the child of a syscon
+ node with the required property:
+
+ - compatible: Should be the following:
+ "amlogic,meson-gx-hhi-sysctrl", "simple-mfd", "syscon"
+
+ Refer to the the bindings described in
+ Documentation/devicetree/bindings/mfd/syscon.txt
+
+properties:
+ compatible:
+ enum:
+ - amlogic,meson-g12a-pwrc
+ - amlogic,meson-sm1-pwrc
+
+ clocks:
+ minItems: 2
+
+ clock-names:
+ items:
+ - const: vpu
+ - const: vapb
+
+ resets:
+ minItems: 11
+
+ reset-names:
+ items:
+ - const: viu
+ - const: venc
+ - const: vcbus
+ - const: bt656
+ - const: rdma
+ - const: venci
+ - const: vencp
+ - const: vdac
+ - const: vdi6
+ - const: vencl
+ - const: vid_lock
+
+ "#power-domain-cells":
+ const: 1
+
+ amlogic,ao-sysctrl:
+ description: phandle to the AO sysctrl node
+ allOf:
+ - $ref: /schemas/types.yaml#/definitions/phandle
+
+required:
+ - compatible
+ - clocks
+ - clock-names
+ - resets
+ - reset-names
+ - "#power-domain-cells"
+ - amlogic,ao-sysctrl
+
+examples:
+ - |
+ pwrc: power-controller {
+ compatible = "amlogic,meson-sm1-pwrc";
+ #power-domain-cells = <1>;
+ amlogic,ao-sysctrl = <&rti>;
+ resets = <&reset_viu>,
+ <&reset_venc>,
+ <&reset_vcbus>,
+ <&reset_bt656>,
+ <&reset_rdma>,
+ <&reset_venci>,
+ <&reset_vencp>,
+ <&reset_vdac>,
+ <&reset_vdi6>,
+ <&reset_vencl>,
+ <&reset_vid_lock>;
+ reset-names = "viu", "venc", "vcbus", "bt656",
+ "rdma", "venci", "vencp", "vdac",
+ "vdi6", "vencl", "vid_lock";
+ clocks = <&clk_vpu>, <&clk_vapb>;
+ clock-names = "vpu", "vapb";
+ };
diff --git a/include/dt-bindings/power/meson-g12a-power.h b/include/dt-bindings/power/meson-g12a-power.h
new file mode 100644
index 000000000000..bb5e67a842de
--- /dev/null
+++ b/include/dt-bindings/power/meson-g12a-power.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: (GPL-2.0+ or MIT) */
+/*
+ * Copyright (c) 2019 BayLibre, SAS
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ */
+
+#ifndef _DT_BINDINGS_MESON_G12A_POWER_H
+#define _DT_BINDINGS_MESON_G12A_POWER_H
+
+#define PWRC_G12A_VPU_ID 0
+#define PWRC_G12A_ETH_ID 1
+
+#endif
diff --git a/include/dt-bindings/power/meson-sm1-power.h b/include/dt-bindings/power/meson-sm1-power.h
new file mode 100644
index 000000000000..a020ab00c134
--- /dev/null
+++ b/include/dt-bindings/power/meson-sm1-power.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: (GPL-2.0+ or MIT) */
+/*
+ * Copyright (c) 2019 BayLibre, SAS
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ */
+
+#ifndef _DT_BINDINGS_MESON_SM1_POWER_H
+#define _DT_BINDINGS_MESON_SM1_POWER_H
+
+#define PWRC_SM1_VPU_ID 0
+#define PWRC_SM1_NNA_ID 1
+#define PWRC_SM1_USB_ID 2
+#define PWRC_SM1_PCIE_ID 3
+#define PWRC_SM1_GE2D_ID 4
+#define PWRC_SM1_AUDIO_ID 5
+#define PWRC_SM1_ETH_ID 6
+
+#endif
--
2.22.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 2/5] soc: amlogic: Add support for Everything-Else power domains controller
From: Neil Armstrong @ 2019-08-21 11:41 UTC (permalink / raw)
To: khilman, ulf.hansson
Cc: linux-amlogic, linux-pm, linux-kernel, linux-arm-kernel,
Neil Armstrong
In-Reply-To: <20190821114121.10430-1-narmstrong@baylibre.com>
Add support for the General Purpose Amlogic Everything-Else Power controller,
with the first support for G12A and SM1 SoCs dedicated to the VPU, PCIe,
USB, NNA, GE2D and Ethernet Power Domains.
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
drivers/soc/amlogic/Kconfig | 11 +
drivers/soc/amlogic/Makefile | 1 +
drivers/soc/amlogic/meson-ee-pwrc.c | 560 ++++++++++++++++++++++++++++
3 files changed, 572 insertions(+)
create mode 100644 drivers/soc/amlogic/meson-ee-pwrc.c
diff --git a/drivers/soc/amlogic/Kconfig b/drivers/soc/amlogic/Kconfig
index 23bfb8ef4fdb..bc2c912949bd 100644
--- a/drivers/soc/amlogic/Kconfig
+++ b/drivers/soc/amlogic/Kconfig
@@ -37,6 +37,17 @@ config MESON_GX_PM_DOMAINS
Say yes to expose Amlogic Meson GX Power Domains as
Generic Power Domains.
+config MESON_EE_PM_DOMAINS
+ bool "Amlogic Meson Everything-Else Power Domains driver"
+ depends on ARCH_MESON || COMPILE_TEST
+ depends on PM && OF
+ default ARCH_MESON
+ select PM_GENERIC_DOMAINS
+ select PM_GENERIC_DOMAINS_OF
+ help
+ Say yes to expose Amlogic Meson Everything-Else Power Domains as
+ Generic Power Domains.
+
config MESON_MX_SOCINFO
bool "Amlogic Meson MX SoC Information driver"
depends on ARCH_MESON || COMPILE_TEST
diff --git a/drivers/soc/amlogic/Makefile b/drivers/soc/amlogic/Makefile
index f2e4ed171297..de79d044b545 100644
--- a/drivers/soc/amlogic/Makefile
+++ b/drivers/soc/amlogic/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_MESON_CLK_MEASURE) += meson-clk-measure.o
obj-$(CONFIG_MESON_GX_SOCINFO) += meson-gx-socinfo.o
obj-$(CONFIG_MESON_GX_PM_DOMAINS) += meson-gx-pwrc-vpu.o
obj-$(CONFIG_MESON_MX_SOCINFO) += meson-mx-socinfo.o
+obj-$(CONFIG_MESON_EE_PM_DOMAINS) += meson-ee-pwrc.o
diff --git a/drivers/soc/amlogic/meson-ee-pwrc.c b/drivers/soc/amlogic/meson-ee-pwrc.c
new file mode 100644
index 000000000000..7159888c850b
--- /dev/null
+++ b/drivers/soc/amlogic/meson-ee-pwrc.c
@@ -0,0 +1,560 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2019 BayLibre, SAS
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ */
+
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/bitfield.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of_device.h>
+#include <linux/reset-controller.h>
+#include <linux/reset.h>
+#include <linux/clk.h>
+#include <dt-bindings/power/meson-g12a-power.h>
+#include <dt-bindings/power/meson-sm1-power.h>
+
+/* AO Offsets */
+
+#define AO_RTI_GEN_PWR_SLEEP0 (0x3a << 2)
+#define AO_RTI_GEN_PWR_ISO0 (0x3b << 2)
+
+/* HHI Offsets */
+
+#define HHI_MEM_PD_REG0 (0x40 << 2)
+#define HHI_VPU_MEM_PD_REG0 (0x41 << 2)
+#define HHI_VPU_MEM_PD_REG1 (0x42 << 2)
+#define HHI_VPU_MEM_PD_REG3 (0x43 << 2)
+#define HHI_VPU_MEM_PD_REG4 (0x44 << 2)
+#define HHI_AUDIO_MEM_PD_REG0 (0x45 << 2)
+#define HHI_NANOQ_MEM_PD_REG0 (0x46 << 2)
+#define HHI_NANOQ_MEM_PD_REG1 (0x47 << 2)
+#define HHI_VPU_MEM_PD_REG2 (0x4d << 2)
+
+struct meson_ee_pwrc;
+struct meson_ee_pwrc_domain;
+
+struct meson_ee_pwrc_mem_domain {
+ unsigned int reg;
+ unsigned int mask;
+};
+
+struct meson_ee_pwrc_top_domain {
+ unsigned int sleep_reg;
+ unsigned int sleep_mask;
+ unsigned int iso_reg;
+ unsigned int iso_mask;
+};
+
+struct meson_ee_pwrc_domain_desc {
+ char *name;
+ char **reset_names;
+ unsigned int reset_names_count;
+ char **clk_names;
+ unsigned int clk_names_count;
+ struct meson_ee_pwrc_top_domain *top_pd;
+ unsigned int mem_pd_count;
+ struct meson_ee_pwrc_mem_domain *mem_pd;
+ bool (*get_power)(struct meson_ee_pwrc_domain *pwrc_domain);
+};
+
+struct meson_ee_pwrc_domain_data {
+ unsigned int count;
+ struct meson_ee_pwrc_domain_desc *domains;
+};
+
+/* Clock and Resets lists */
+
+static char *g12a_pwrc_vpu_resets[] = {
+ "viu", "venc", "vcbus", "bt656",
+ "rdma", "venci", "vencp", "vdac",
+ "vdi6", "vencl", "vid_lock",
+};
+
+static char *g12a_pwrc_vpu_clks[] = {
+ "vpu", "vapb",
+};
+
+/* TOP Power Domains */
+
+static struct meson_ee_pwrc_top_domain g12a_pwrc_vpu = {
+ .sleep_reg = AO_RTI_GEN_PWR_SLEEP0,
+ .sleep_mask = BIT(8),
+ .iso_reg = AO_RTI_GEN_PWR_SLEEP0,
+ .iso_mask = BIT(9),
+};
+
+#define SM1_EE_PD(__bit) \
+ { \
+ .sleep_reg = AO_RTI_GEN_PWR_SLEEP0, \
+ .sleep_mask = BIT(__bit), \
+ .iso_reg = AO_RTI_GEN_PWR_ISO0, \
+ .iso_mask = BIT(__bit), \
+ }
+
+static struct meson_ee_pwrc_top_domain sm1_pwrc_vpu = SM1_EE_PD(8);
+static struct meson_ee_pwrc_top_domain sm1_pwrc_nna = SM1_EE_PD(16);
+static struct meson_ee_pwrc_top_domain sm1_pwrc_usb = SM1_EE_PD(17);
+static struct meson_ee_pwrc_top_domain sm1_pwrc_pci = SM1_EE_PD(18);
+static struct meson_ee_pwrc_top_domain sm1_pwrc_ge2d = SM1_EE_PD(19);
+
+/* Memory PD Domains */
+
+#define VPU_MEMPD(__reg) \
+ { __reg, GENMASK(1, 0) }, \
+ { __reg, GENMASK(3, 2) }, \
+ { __reg, GENMASK(5, 4) }, \
+ { __reg, GENMASK(7, 6) }, \
+ { __reg, GENMASK(9, 8) }, \
+ { __reg, GENMASK(11, 10) }, \
+ { __reg, GENMASK(13, 12) }, \
+ { __reg, GENMASK(15, 14) }, \
+ { __reg, GENMASK(17, 16) }, \
+ { __reg, GENMASK(19, 18) }, \
+ { __reg, GENMASK(21, 20) }, \
+ { __reg, GENMASK(23, 22) }, \
+ { __reg, GENMASK(25, 24) }, \
+ { __reg, GENMASK(27, 26) }, \
+ { __reg, GENMASK(29, 28) }, \
+ { __reg, GENMASK(31, 30) }
+
+#define VPU_HHI_MEMPD(__reg) \
+ { __reg, BIT(8) }, \
+ { __reg, BIT(9) }, \
+ { __reg, BIT(10) }, \
+ { __reg, BIT(11) }, \
+ { __reg, BIT(12) }, \
+ { __reg, BIT(13) }, \
+ { __reg, BIT(14) }, \
+ { __reg, BIT(15) }
+
+static struct meson_ee_pwrc_mem_domain g12a_pwrc_mem_vpu[] = {
+ VPU_MEMPD(HHI_VPU_MEM_PD_REG0),
+ VPU_MEMPD(HHI_VPU_MEM_PD_REG1),
+ VPU_MEMPD(HHI_VPU_MEM_PD_REG2),
+ VPU_HHI_MEMPD(HHI_MEM_PD_REG0),
+};
+
+static struct meson_ee_pwrc_mem_domain g12a_pwrc_mem_eth[] = {
+ { HHI_MEM_PD_REG0, GENMASK(3, 2) },
+};
+
+static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_vpu[] = {
+ VPU_MEMPD(HHI_VPU_MEM_PD_REG0),
+ VPU_MEMPD(HHI_VPU_MEM_PD_REG1),
+ VPU_MEMPD(HHI_VPU_MEM_PD_REG2),
+ VPU_MEMPD(HHI_VPU_MEM_PD_REG3),
+ { HHI_VPU_MEM_PD_REG4, GENMASK(1, 0) },
+ { HHI_VPU_MEM_PD_REG4, GENMASK(3, 2) },
+ { HHI_VPU_MEM_PD_REG4, GENMASK(5, 4) },
+ { HHI_VPU_MEM_PD_REG4, GENMASK(7, 6) },
+ VPU_HHI_MEMPD(HHI_MEM_PD_REG0),
+};
+
+static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_nna[] = {
+ { HHI_NANOQ_MEM_PD_REG0, 0xff },
+ { HHI_NANOQ_MEM_PD_REG1, 0xff },
+};
+
+static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_usb[] = {
+ { HHI_MEM_PD_REG0, GENMASK(31, 30) },
+};
+
+static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_pcie[] = {
+ { HHI_MEM_PD_REG0, GENMASK(29, 26) },
+};
+
+static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_ge2d[] = {
+ { HHI_MEM_PD_REG0, GENMASK(25, 18) },
+};
+
+static struct meson_ee_pwrc_mem_domain sm1_pwrc_mem_audio[] = {
+ { HHI_MEM_PD_REG0, GENMASK(5, 4) },
+ { HHI_AUDIO_MEM_PD_REG0, GENMASK(1, 0) },
+ { HHI_AUDIO_MEM_PD_REG0, GENMASK(3, 2) },
+ { HHI_AUDIO_MEM_PD_REG0, GENMASK(5, 4) },
+ { HHI_AUDIO_MEM_PD_REG0, GENMASK(7, 6) },
+ { HHI_AUDIO_MEM_PD_REG0, GENMASK(13, 12) },
+ { HHI_AUDIO_MEM_PD_REG0, GENMASK(15, 14) },
+ { HHI_AUDIO_MEM_PD_REG0, GENMASK(17, 16) },
+ { HHI_AUDIO_MEM_PD_REG0, GENMASK(19, 18) },
+ { HHI_AUDIO_MEM_PD_REG0, GENMASK(21, 20) },
+ { HHI_AUDIO_MEM_PD_REG0, GENMASK(23, 22) },
+ { HHI_AUDIO_MEM_PD_REG0, GENMASK(25, 24) },
+ { HHI_AUDIO_MEM_PD_REG0, GENMASK(27, 26) },
+};
+
+#define VPU_PD(__name, __resets, __clks, __top_pd, __mem, __get_power) \
+ { \
+ .name = __name, \
+ .reset_names_count = ARRAY_SIZE(__resets), \
+ .reset_names = __resets, \
+ .clk_names_count = ARRAY_SIZE(__clks), \
+ .clk_names = __clks, \
+ .top_pd = __top_pd, \
+ .mem_pd_count = ARRAY_SIZE(__mem), \
+ .mem_pd = __mem, \
+ .get_power = __get_power, \
+ }
+
+#define TOP_PD(__name, __top_pd, __mem) \
+ { \
+ .name = __name, \
+ .top_pd = __top_pd, \
+ .mem_pd_count = ARRAY_SIZE(__mem), \
+ .mem_pd = __mem, \
+ }
+
+#define MEM_PD(__name, __mem) \
+ TOP_PD(__name, NULL, __mem)
+
+static bool pwrc_vpu_get_power(struct meson_ee_pwrc_domain *pwrc_domain);
+
+static struct meson_ee_pwrc_domain_desc g12a_pwrc_domains[] = {
+ [PWRC_G12A_VPU_ID] = VPU_PD("VPU", g12a_pwrc_vpu_resets,
+ g12a_pwrc_vpu_clks, &g12a_pwrc_vpu,
+ g12a_pwrc_mem_vpu,
+ pwrc_vpu_get_power),
+ [PWRC_G12A_ETH_ID] = MEM_PD("ETH", g12a_pwrc_mem_eth),
+};
+
+static struct meson_ee_pwrc_domain_desc sm1_pwrc_domains[] = {
+ [PWRC_SM1_VPU_ID] = VPU_PD("VPU", g12a_pwrc_vpu_resets,
+ g12a_pwrc_vpu_clks, &sm1_pwrc_vpu,
+ sm1_pwrc_mem_vpu,
+ pwrc_vpu_get_power),
+ [PWRC_SM1_NNA_ID] = TOP_PD("NNA", &sm1_pwrc_nna, sm1_pwrc_mem_nna),
+ [PWRC_SM1_USB_ID] = TOP_PD("USB", &sm1_pwrc_usb, sm1_pwrc_mem_usb),
+ [PWRC_SM1_PCIE_ID] = TOP_PD("PCI", &sm1_pwrc_pci, sm1_pwrc_mem_pcie),
+ [PWRC_SM1_GE2D_ID] = TOP_PD("GE2D", &sm1_pwrc_ge2d, sm1_pwrc_mem_ge2d),
+ [PWRC_SM1_AUDIO_ID] = MEM_PD("AUDIO", sm1_pwrc_mem_audio),
+ [PWRC_SM1_ETH_ID] = MEM_PD("ETH", g12a_pwrc_mem_eth),
+};
+
+struct meson_ee_pwrc_domain {
+ struct generic_pm_domain base;
+ bool enabled;
+ struct meson_ee_pwrc *pwrc;
+ struct meson_ee_pwrc_domain_desc desc;
+ struct clk **clks;
+ int num_clks;
+ struct reset_control **rstc;
+ int num_rstc;
+};
+
+struct meson_ee_pwrc {
+ struct regmap *regmap_ao;
+ struct regmap *regmap_hhi;
+ struct meson_ee_pwrc_domain *domains;
+ struct genpd_onecell_data xlate;
+};
+
+static bool pwrc_vpu_get_power(struct meson_ee_pwrc_domain *pwrc_domain)
+{
+ u32 reg;
+
+ regmap_read(pwrc_domain->pwrc->regmap_ao,
+ pwrc_domain->desc.top_pd->sleep_reg, ®);
+
+ return (reg & pwrc_domain->desc.top_pd->sleep_mask);
+}
+
+static int meson_ee_reset_assert(struct meson_ee_pwrc_domain *pwrc_domain)
+{
+ int i, ret;
+
+ for (i = 0 ; i < pwrc_domain->num_rstc ; ++i) {
+ ret = reset_control_assert(pwrc_domain->rstc[i]);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static int meson_ee_reset_deassert(struct meson_ee_pwrc_domain *pwrc_domain)
+{
+ int i, ret;
+
+ for (i = 0 ; i < pwrc_domain->num_rstc ; ++i) {
+ ret = reset_control_deassert(pwrc_domain->rstc[i]);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static int meson_ee_clk_disable(struct meson_ee_pwrc_domain *pwrc_domain)
+{
+ int i;
+
+ for (i = 0 ; i < pwrc_domain->num_clks ; ++i)
+ clk_disable(pwrc_domain->clks[i]);
+
+ for (i = 0 ; i < pwrc_domain->num_clks ; ++i)
+ clk_unprepare(pwrc_domain->clks[i]);
+
+ return 0;
+}
+
+static int meson_ee_clk_enable(struct meson_ee_pwrc_domain *pwrc_domain)
+{
+ int i, ret;
+
+ for (i = 0 ; i < pwrc_domain->num_clks ; ++i) {
+ ret = clk_prepare(pwrc_domain->clks[i]);
+ if (ret)
+ goto fail_prepare;
+ }
+
+ for (i = 0 ; i < pwrc_domain->num_clks ; ++i) {
+ ret = clk_enable(pwrc_domain->clks[i]);
+ if (ret)
+ goto fail_enable;
+ }
+
+ return 0;
+
+fail_enable:
+ while (--i)
+ clk_disable(pwrc_domain->clks[i]);
+
+ /* Unprepare all clocks */
+ i = pwrc_domain->num_clks;
+
+fail_prepare:
+ while (--i)
+ clk_unprepare(pwrc_domain->clks[i]);
+
+ return ret;
+}
+
+static int meson_ee_pwrc_off(struct generic_pm_domain *domain)
+{
+ struct meson_ee_pwrc_domain *pwrc_domain =
+ container_of(domain, struct meson_ee_pwrc_domain, base);
+ int i;
+
+ if (pwrc_domain->desc.top_pd)
+ regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
+ pwrc_domain->desc.top_pd->sleep_reg,
+ pwrc_domain->desc.top_pd->sleep_mask,
+ pwrc_domain->desc.top_pd->sleep_mask);
+ udelay(20);
+
+ for (i = 0 ; i < pwrc_domain->desc.mem_pd_count ; ++i)
+ regmap_update_bits(pwrc_domain->pwrc->regmap_hhi,
+ pwrc_domain->desc.mem_pd[i].reg,
+ pwrc_domain->desc.mem_pd[i].mask,
+ pwrc_domain->desc.mem_pd[i].mask);
+
+ udelay(20);
+
+ if (pwrc_domain->desc.top_pd)
+ regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
+ pwrc_domain->desc.top_pd->iso_reg,
+ pwrc_domain->desc.top_pd->iso_mask,
+ pwrc_domain->desc.top_pd->iso_mask);
+
+ if (pwrc_domain->num_clks) {
+ msleep(20);
+ meson_ee_clk_disable(pwrc_domain);
+ }
+
+ return 0;
+}
+
+static int meson_ee_pwrc_on(struct generic_pm_domain *domain)
+{
+ struct meson_ee_pwrc_domain *pwrc_domain =
+ container_of(domain, struct meson_ee_pwrc_domain, base);
+ int i, ret;
+
+ if (pwrc_domain->desc.top_pd)
+ regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
+ pwrc_domain->desc.top_pd->sleep_reg,
+ pwrc_domain->desc.top_pd->sleep_mask, 0);
+ udelay(20);
+
+ for (i = 0 ; i < pwrc_domain->desc.mem_pd_count ; ++i)
+ regmap_update_bits(pwrc_domain->pwrc->regmap_hhi,
+ pwrc_domain->desc.mem_pd[i].reg,
+ pwrc_domain->desc.mem_pd[i].mask, 0);
+
+ udelay(20);
+
+ ret = meson_ee_reset_assert(pwrc_domain);
+ if (ret)
+ return ret;
+
+ if (pwrc_domain->desc.top_pd)
+ regmap_update_bits(pwrc_domain->pwrc->regmap_ao,
+ pwrc_domain->desc.top_pd->iso_reg,
+ pwrc_domain->desc.top_pd->iso_mask, 0);
+
+ ret = meson_ee_reset_deassert(pwrc_domain);
+ if (ret)
+ return ret;
+
+ return meson_ee_clk_enable(pwrc_domain);
+}
+
+static int meson_ee_pwrc_init_domain(struct platform_device *pdev,
+ struct meson_ee_pwrc *sm1_pwrc,
+ struct meson_ee_pwrc_domain *dom)
+{
+ dom->pwrc = sm1_pwrc;
+ dom->num_rstc = dom->desc.reset_names_count;
+ dom->num_clks = dom->desc.clk_names_count;
+
+ if (dom->num_rstc) {
+ int rst;
+
+ dom->rstc = devm_kcalloc(&pdev->dev, dom->num_rstc,
+ sizeof(struct reset_control *), GFP_KERNEL);
+ if (!dom->rstc)
+ return -ENOMEM;
+
+ for (rst = 0 ; rst < dom->num_rstc ; ++rst) {
+ dom->rstc[rst] = devm_reset_control_get_exclusive(
+ &pdev->dev,
+ dom->desc.reset_names[rst]);
+ if (IS_ERR(dom->rstc[rst]))
+ return PTR_ERR(dom->rstc[rst]);
+ }
+ }
+
+ if (dom->num_clks) {
+ int clk;
+
+ dom->clks = devm_kcalloc(&pdev->dev, dom->num_clks,
+ sizeof(struct clk *), GFP_KERNEL);
+ if (!dom->clks)
+ return -ENOMEM;
+
+ for (clk = 0 ; clk < dom->num_clks ; ++clk) {
+ dom->clks[clk] = devm_clk_get(&pdev->dev,
+ dom->desc.clk_names[clk]);
+ if (IS_ERR(dom->clks[clk]))
+ return PTR_ERR(dom->clks[clk]);
+ }
+ }
+
+ dom->base.name = dom->desc.name;
+ dom->base.power_on = meson_ee_pwrc_on;
+ dom->base.power_off = meson_ee_pwrc_off;
+
+ if (dom->desc.get_power) {
+ bool powered_off = dom->desc.get_power(dom);
+ pm_genpd_init(&dom->base, &pm_domain_always_on_gov,
+ powered_off);
+ } else
+ pm_genpd_init(&dom->base, NULL, true);
+
+ return 0;
+}
+
+static int meson_ee_pwrc_probe(struct platform_device *pdev)
+{
+ const struct meson_ee_pwrc_domain_data *match;
+ struct regmap *regmap_ao, *regmap_hhi;
+ struct meson_ee_pwrc *sm1_pwrc;
+ int i, ret;
+
+ match = of_device_get_match_data(&pdev->dev);
+ if (!match) {
+ dev_err(&pdev->dev, "failed to get match data\n");
+ return -ENODEV;
+ }
+
+ sm1_pwrc = devm_kzalloc(&pdev->dev, sizeof(*sm1_pwrc), GFP_KERNEL);
+ if (!sm1_pwrc)
+ return -ENOMEM;
+
+ sm1_pwrc->xlate.domains =
+ devm_kcalloc(&pdev->dev,
+ match->count,
+ sizeof(*sm1_pwrc->xlate.domains),
+ GFP_KERNEL);
+ if (!sm1_pwrc->xlate.domains)
+ return -ENOMEM;
+
+ sm1_pwrc->domains =
+ devm_kcalloc(&pdev->dev,
+ match->count,
+ sizeof(*sm1_pwrc->domains),
+ GFP_KERNEL);
+ if (!sm1_pwrc->domains)
+ return -ENOMEM;
+
+ sm1_pwrc->xlate.num_domains = match->count;
+
+ regmap_hhi = syscon_node_to_regmap(of_get_parent(pdev->dev.of_node));
+ if (IS_ERR(regmap_hhi)) {
+ dev_err(&pdev->dev, "failed to get HHI regmap\n");
+ return PTR_ERR(regmap_hhi);
+ }
+
+ regmap_ao = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+ "amlogic,ao-sysctrl");
+ if (IS_ERR(regmap_ao)) {
+ dev_err(&pdev->dev, "failed to get AO regmap\n");
+ return PTR_ERR(regmap_ao);
+ }
+
+ sm1_pwrc->regmap_ao = regmap_ao;
+ sm1_pwrc->regmap_hhi = regmap_hhi;
+
+ platform_set_drvdata(pdev, sm1_pwrc);
+
+ for (i = 0 ; i < match->count ; ++i) {
+ struct meson_ee_pwrc_domain *dom = &sm1_pwrc->domains[i];
+
+ memcpy(&dom->desc, &match->domains[i], sizeof(dom->desc));
+
+ ret = meson_ee_pwrc_init_domain(pdev, sm1_pwrc, dom);
+ if (ret)
+ return ret;
+
+ sm1_pwrc->xlate.domains[i] = &dom->base;
+ }
+
+ of_genpd_add_provider_onecell(pdev->dev.of_node, &sm1_pwrc->xlate);
+
+ return 0;
+}
+
+static struct meson_ee_pwrc_domain_data meson_ee_g12a_pwrc_data = {
+ .count = ARRAY_SIZE(g12a_pwrc_domains),
+ .domains = g12a_pwrc_domains,
+};
+
+static struct meson_ee_pwrc_domain_data meson_ee_sm1_pwrc_data = {
+ .count = ARRAY_SIZE(sm1_pwrc_domains),
+ .domains = sm1_pwrc_domains,
+};
+
+static const struct of_device_id meson_ee_pwrc_match_table[] = {
+ {
+ .compatible = "amlogic,meson-g12a-pwrc",
+ .data = &meson_ee_g12a_pwrc_data,
+ },
+ {
+ .compatible = "amlogic,meson-sm1-pwrc",
+ .data = &meson_ee_sm1_pwrc_data,
+ },
+ { /* sentinel */ }
+};
+
+static struct platform_driver meson_ee_pwrc_driver = {
+ .probe = meson_ee_pwrc_probe,
+ .driver = {
+ .name = "meson_ee_pwrc",
+ .of_match_table = meson_ee_pwrc_match_table,
+ },
+};
+builtin_platform_driver(meson_ee_pwrc_driver);
--
2.22.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 3/5] arm64: meson-g12: add Everything-Else power domain controller
From: Neil Armstrong @ 2019-08-21 11:41 UTC (permalink / raw)
To: khilman, ulf.hansson
Cc: linux-amlogic, linux-pm, linux-kernel, linux-arm-kernel,
Neil Armstrong
In-Reply-To: <20190821114121.10430-1-narmstrong@baylibre.com>
Replace the VPU-centric power domain controller by the generic system-wide
Everything-Else power domain controller and setup the right power-domains
properties on the VPU, Ethernet & USB nodes.
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
.../boot/dts/amlogic/meson-g12-common.dtsi | 92 ++++++++++---------
arch/arm64/boot/dts/amlogic/meson-g12a.dtsi | 9 ++
arch/arm64/boot/dts/amlogic/meson-g12b.dtsi | 9 ++
arch/arm64/boot/dts/amlogic/meson-sm1.dtsi | 15 ++-
4 files changed, 77 insertions(+), 48 deletions(-)
diff --git a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
index a921d6334e5b..8baa6318f180 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
@@ -1426,6 +1426,53 @@
clocks = <&xtal>;
clock-names = "xtal";
};
+
+ pwrc: power-controller {
+ compatible = "amlogic,meson-g12a-pwrc";
+ #power-domain-cells = <1>;
+ amlogic,ao-sysctrl = <&rti>;
+ resets = <&reset RESET_VIU>,
+ <&reset RESET_VENC>,
+ <&reset RESET_VCBUS>,
+ <&reset RESET_BT656>,
+ <&reset RESET_RDMA>,
+ <&reset RESET_VENCI>,
+ <&reset RESET_VENCP>,
+ <&reset RESET_VDAC>,
+ <&reset RESET_VDI6>,
+ <&reset RESET_VENCL>,
+ <&reset RESET_VID_LOCK>;
+ reset-names = "viu", "venc", "vcbus", "bt656",
+ "rdma", "venci", "vencp", "vdac",
+ "vdi6", "vencl", "vid_lock";
+ clocks = <&clkc CLKID_VPU>,
+ <&clkc CLKID_VAPB>;
+ clock-names = "vpu", "vapb";
+ /*
+ * VPU clocking is provided by two identical clock paths
+ * VPU_0 and VPU_1 muxed to a single clock by a glitch
+ * free mux to safely change frequency while running.
+ * Same for VAPB but with a final gate after the glitch free mux.
+ */
+ assigned-clocks = <&clkc CLKID_VPU_0_SEL>,
+ <&clkc CLKID_VPU_0>,
+ <&clkc CLKID_VPU>, /* Glitch free mux */
+ <&clkc CLKID_VAPB_0_SEL>,
+ <&clkc CLKID_VAPB_0>,
+ <&clkc CLKID_VAPB_SEL>; /* Glitch free mux */
+ assigned-clock-parents = <&clkc CLKID_FCLK_DIV3>,
+ <0>, /* Do Nothing */
+ <&clkc CLKID_VPU_0>,
+ <&clkc CLKID_FCLK_DIV4>,
+ <0>, /* Do Nothing */
+ <&clkc CLKID_VAPB_0>;
+ assigned-clock-rates = <0>, /* Do Nothing */
+ <666666666>,
+ <0>, /* Do Nothing */
+ <0>, /* Do Nothing */
+ <250000000>,
+ <0>; /* Do Nothing */
+ };
};
};
@@ -1773,50 +1820,6 @@
clock-names = "xtal", "mpeg-clk";
};
- pwrc_vpu: power-controller-vpu {
- compatible = "amlogic,meson-g12a-pwrc-vpu";
- #power-domain-cells = <0>;
- amlogic,hhi-sysctrl = <&hhi>;
- resets = <&reset RESET_VIU>,
- <&reset RESET_VENC>,
- <&reset RESET_VCBUS>,
- <&reset RESET_BT656>,
- <&reset RESET_RDMA>,
- <&reset RESET_VENCI>,
- <&reset RESET_VENCP>,
- <&reset RESET_VDAC>,
- <&reset RESET_VDI6>,
- <&reset RESET_VENCL>,
- <&reset RESET_VID_LOCK>;
- clocks = <&clkc CLKID_VPU>,
- <&clkc CLKID_VAPB>;
- clock-names = "vpu", "vapb";
- /*
- * VPU clocking is provided by two identical clock paths
- * VPU_0 and VPU_1 muxed to a single clock by a glitch
- * free mux to safely change frequency while running.
- * Same for VAPB but with a final gate after the glitch free mux.
- */
- assigned-clocks = <&clkc CLKID_VPU_0_SEL>,
- <&clkc CLKID_VPU_0>,
- <&clkc CLKID_VPU>, /* Glitch free mux */
- <&clkc CLKID_VAPB_0_SEL>,
- <&clkc CLKID_VAPB_0>,
- <&clkc CLKID_VAPB_SEL>; /* Glitch free mux */
- assigned-clock-parents = <&clkc CLKID_FCLK_DIV3>,
- <0>, /* Do Nothing */
- <&clkc CLKID_VPU_0>,
- <&clkc CLKID_FCLK_DIV4>,
- <0>, /* Do Nothing */
- <&clkc CLKID_VAPB_0>;
- assigned-clock-rates = <0>, /* Do Nothing */
- <666666666>,
- <0>, /* Do Nothing */
- <0>, /* Do Nothing */
- <250000000>,
- <0>; /* Do Nothing */
- };
-
ao_pinctrl: pinctrl@14 {
compatible = "amlogic,meson-g12a-aobus-pinctrl";
#address-cells = <2>;
@@ -2169,7 +2172,6 @@
#address-cells = <1>;
#size-cells = <0>;
amlogic,canvas = <&canvas>;
- power-domains = <&pwrc_vpu>;
/* CVBS VDAC output port */
cvbs_vdac_port: port@0 {
diff --git a/arch/arm64/boot/dts/amlogic/meson-g12a.dtsi b/arch/arm64/boot/dts/amlogic/meson-g12a.dtsi
index 733a9d46fc4b..eb5d177d7a99 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12a.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-g12a.dtsi
@@ -4,6 +4,7 @@
*/
#include "meson-g12-common.dtsi"
+#include <dt-bindings/power/meson-g12a-power.h>
/ {
compatible = "amlogic,g12a";
@@ -110,6 +111,14 @@
};
};
+ðmac {
+ power-domains = <&pwrc PWRC_G12A_ETH_ID>;
+};
+
+&vpu {
+ power-domains = <&pwrc PWRC_G12A_VPU_ID>;
+};
+
&sd_emmc_a {
amlogic,dram-access-quirk;
};
diff --git a/arch/arm64/boot/dts/amlogic/meson-g12b.dtsi b/arch/arm64/boot/dts/amlogic/meson-g12b.dtsi
index d5edbc1a1991..5628ccd54531 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12b.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-g12b.dtsi
@@ -5,6 +5,7 @@
*/
#include "meson-g12-common.dtsi"
+#include <dt-bindings/power/meson-g12a-power.h>
/ {
compatible = "amlogic,g12b";
@@ -101,6 +102,14 @@
compatible = "amlogic,g12b-clkc";
};
+ðmac {
+ power-domains = <&pwrc PWRC_G12A_ETH_ID>;
+};
+
+&vpu {
+ power-domains = <&pwrc PWRC_G12A_VPU_ID>;
+};
+
&sd_emmc_a {
amlogic,dram-access-quirk;
};
diff --git a/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi b/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi
index e902d4f9165f..37064d7f66c1 100644
--- a/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi
@@ -5,6 +5,7 @@
*/
#include "meson-g12-common.dtsi"
+#include <dt-bindings/power/meson-sm1-power.h>
/ {
compatible = "amlogic,sm1";
@@ -59,10 +60,18 @@
compatible = "amlogic,meson-sm1-clk-measure";
};
-&pwrc_vpu {
- status = "disabled";
+ðmac {
+ power-domains = <&pwrc PWRC_SM1_ETH_ID>;
+};
+
+&pwrc {
+ compatible = "amlogic,meson-sm1-pwrc";
};
&vpu {
- status = "disabled";
+ power-domains = <&pwrc PWRC_SM1_VPU_ID>;
+};
+
+&usb {
+ power-domains = <&pwrc PWRC_SM1_USB_ID>;
};
--
2.22.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 4/5] arm64: dts: meson-sm1-sei610: add HDMI display support
From: Neil Armstrong @ 2019-08-21 11:41 UTC (permalink / raw)
To: khilman, ulf.hansson
Cc: linux-amlogic, linux-pm, linux-kernel, linux-arm-kernel,
Neil Armstrong
In-Reply-To: <20190821114121.10430-1-narmstrong@baylibre.com>
Update compatible of the pwc-vpu node and add the HDMI support nodes
for the Amlogic SM1 Based SEI610 Board.
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
.../boot/dts/amlogic/meson-sm1-sei610.dts | 23 +++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts b/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts
index 12dab0ba2f26..66bd3bfbaf91 100644
--- a/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-sm1-sei610.dts
@@ -51,6 +51,17 @@
};
};
+ hdmi-connector {
+ compatible = "hdmi-connector";
+ type = "a";
+
+ port {
+ hdmi_connector_in: endpoint {
+ remote-endpoint = <&hdmi_tx_tmds_out>;
+ };
+ };
+ };
+
leds {
compatible = "gpio-leds";
@@ -177,6 +188,18 @@
phy-mode = "rmii";
};
+&hdmi_tx {
+ status = "okay";
+ pinctrl-0 = <&hdmitx_hpd_pins>, <&hdmitx_ddc_pins>;
+ pinctrl-names = "default";
+};
+
+&hdmi_tx_tmds_port {
+ hdmi_tx_tmds_out: endpoint {
+ remote-endpoint = <&hdmi_connector_in>;
+ };
+};
+
&i2c3 {
status = "okay";
pinctrl-0 = <&i2c3_sda_a_pins>, <&i2c3_sck_a_pins>;
--
2.22.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 0/5] arm64: meson: add support for SM1 Power Domains
From: Neil Armstrong @ 2019-08-21 11:41 UTC (permalink / raw)
To: khilman, ulf.hansson
Cc: linux-amlogic, linux-pm, linux-kernel, linux-arm-kernel,
Neil Armstrong
This patchset introduces a new "Everything-Else Power Domain Controller"
designed to handle all the different non-Always On peripherals like :
- VPU
- Ethernet Memories
- USB, PCIe, Audio, NNA on SM1
The current "gx-vpu-pwrc" process has been integrated to support the VPU
and the other power domains in a single driver.
Support for SoC domains has been made generic and easily extendable.
In order to restart from clean architecture :
- the PWRC node has been moved into the HHI simple-mfd, this suits much
better than beeing in the AO RTI simple-mfd
- a brand new yaml bindings schemas has been written
- reset-names has been added to clarify which resets are needed, so we can
dispatch them to domains
For G12A, the PWRC now offers support for the ethmac memory power domain.
For SM1, it also offers support for PCIe, USB, NNA, ethmac and Audio power
domains.
The DOS domains has been excluded for now, but can be added very easily.
GX hasn't been integrated for now, but it would follow the same scheme
as G12A support.
Neil Armstrong (5):
dt-bindings: power: add Amlogic Everything-Else power domains bindings
soc: amlogic: Add support for Everything-Else power domains controller
arm64: meson-g12: add Everything-Else power domain controller
arm64: dts: meson-sm1-sei610: add HDMI display support
arm64: dts: meson-sm1-sei610: add USB support
.../bindings/power/amlogic,meson-ee-pwrc.yaml | 93 +++
.../boot/dts/amlogic/meson-g12-common.dtsi | 92 +--
arch/arm64/boot/dts/amlogic/meson-g12a.dtsi | 9 +
arch/arm64/boot/dts/amlogic/meson-g12b.dtsi | 9 +
.../boot/dts/amlogic/meson-sm1-sei610.dts | 28 +
arch/arm64/boot/dts/amlogic/meson-sm1.dtsi | 15 +-
drivers/soc/amlogic/Kconfig | 11 +
drivers/soc/amlogic/Makefile | 1 +
drivers/soc/amlogic/meson-ee-pwrc.c | 560 ++++++++++++++++++
include/dt-bindings/power/meson-g12a-power.h | 13 +
include/dt-bindings/power/meson-sm1-power.h | 18 +
11 files changed, 801 insertions(+), 48 deletions(-)
create mode 100644 Documentation/devicetree/bindings/power/amlogic,meson-ee-pwrc.yaml
create mode 100644 drivers/soc/amlogic/meson-ee-pwrc.c
create mode 100644 include/dt-bindings/power/meson-g12a-power.h
create mode 100644 include/dt-bindings/power/meson-sm1-power.h
--
2.22.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [V3, 2/2] media: i2c: Add Omnivision OV02A10 camera sensor driver
From: Sakari Ailus @ 2019-08-21 11:05 UTC (permalink / raw)
To: Tomasz Figa
Cc: mark.rutland, devicetree, drinkcat, srv_heupstream, shengnan.wang,
louis.kuo, sj.huang, robh+dt, linux-mediatek, dongchun.zhu,
matthias.bgg, bingbu.cao, mchehab, linux-arm-kernel, linux-media
In-Reply-To: <20190821103038.GA148543@chromium.org>
Hi Tomasz,
On Wed, Aug 21, 2019 at 07:30:38PM +0900, Tomasz Figa wrote:
...
> > +
> > +/*
> > + * xvclk 24Mhz
>
> This seems to assume 24MHz, but the driver allows a range in probe. Is that
> correct?
I think it'd be better to check for an exact frequency: this is board
specific and its exact value is known.
...
> > +static int __ov02a10_power_on(struct ov02a10 *ov02a10)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > + struct device *dev = &client->dev;
> > + int ret;
> > +
> > + ret = clk_prepare_enable(ov02a10->xvclk);
> > + if (ret < 0) {
> > + dev_err(dev, "Failed to enable xvclk\n");
> > + return ret;
> > + }
>
> Is it really correct to enable the clock before the regulators?
>
> According to the datasheet, it should be:
> - PD pin HIGH,
> - nRST pin LOW,
> - DVDDIO and AVDD28 power up and stabilize,
> - clock enabled,
> - min 5 ms delay,
> - PD pin LOW,
> - min 4 ms delay,
> - nRST pin HIGH,
> - min 5 ms delay,
> - I2C interface ready.
>
> > +
> > + /* Note: set 0 is high, set 1 is low */
>
> Why is that? If there is some inverter on the way that should be handled
> outside of this driver. (GPIO DT bindings have flags for this purpose.
>
> If the pins are nRESET and nPOWERDOWN in the hardware datasheet, we should
> call them like this in the driver too (+/- the lowercase and underscore
> convention).
>
> According to the datasheet, the reset pin is called RST and inverted, so we should
> call it n_rst, but the powerdown signal, called PD, is not inverted, so pd
> would be the right name.
For what it's worth sensors generally have xshutdown (or reset) pin that is
active high. Looking at the code, it is not the case here. It's a bit odd
since the usual arrangement saves power when the camera is not in use; it's
not a lot but still. Oh well.
...
> > +static struct i2c_driver ov02a10_i2c_driver = {
> > + .driver = {
> > + .name = "ov02a10",
> > + .pm = &ov02a10_pm_ops,
> > + .of_match_table = ov02a10_of_match,
>
> Please use of_match_ptr() wrapper.
Not really needed; the driver does expect regulators, GPIOs etc., but by
leaving out of_match_ptr(), the driver will also probe on ACPI based
systems.
--
Regards,
Sakari Ailus
sakari.ailus@linux.intel.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH v3] arm64: implement KPROBES_ON_FTRACE
From: Jisheng Zhang @ 2019-08-21 10:46 UTC (permalink / raw)
To: Catalin Marinas, Jonathan Corbet, Will Deacon, Thomas Gleixner,
Masami Hiramatsu, Naveen N. Rao, Peter Zijlstra
Cc: Ingo Molnar, Steven Rostedt, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, linux-doc@vger.kernel.org
KPROBES_ON_FTRACE avoids much of the overhead with regular kprobes as it
eliminates the need for a trap, as well as the need to emulate or
single-step instructions.
Tested on berlin arm64 platform.
~ # mount -t debugfs debugfs /sys/kernel/debug/
~ # cd /sys/kernel/debug/
/sys/kernel/debug # echo 'p _do_fork' > tracing/kprobe_events
before the patch:
/sys/kernel/debug # cat kprobes/list
ffffff801009fe28 k _do_fork+0x0 [DISABLED]
after the patch:
/sys/kernel/debug # cat kprobes/list
ffffff801009ff54 k _do_fork+0x4 [DISABLED][FTRACE]
Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
---
KPROBES_ON_FTRACE avoids much of the overhead with regular kprobes as it
eliminates the need for a trap, as well as the need to emulate or
single-step instructions.
Applied after arm64 FTRACE_WITH_REGS:
http://lists.infradead.org/pipermail/linux-arm-kernel/2019-August/674404.html
Changes since v2:
- remove patch1, make it a single cleanup patch
- remove "This patch" in the change log
- implement arm64's kprobe_lookup_name() and arch_kprobe_on_func_entry instead
patching the common kprobes code
Changes since v1:
- make the kprobes/x86: use instruction_pointer and instruction_pointer_set
as patch1
- add Masami's ACK to patch1
- add some description about KPROBES_ON_FTRACE and why we need it on
arm64
- correct the log before the patch
- remove the consolidation patch, make it as TODO
- only adjust kprobe's addr when KPROBE_FLAG_FTRACE is set
- if KPROBES_ON_FTRACE, ftrace_call_adjust() the kprobe's addr before
calling ftrace_location()
- update the kprobes-on-ftrace/arch-support.txt in doc
.../debug/kprobes-on-ftrace/arch-support.txt | 2 +-
arch/arm64/Kconfig | 1 +
arch/arm64/kernel/probes/Makefile | 1 +
arch/arm64/kernel/probes/ftrace.c | 60 +++++++++++++++++++
arch/arm64/kernel/probes/kprobes.c | 23 +++++++
5 files changed, 86 insertions(+), 1 deletion(-)
create mode 100644 arch/arm64/kernel/probes/ftrace.c
diff --git a/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt b/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt
index 68f266944d5f..e8358a38981c 100644
--- a/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt
+++ b/Documentation/features/debug/kprobes-on-ftrace/arch-support.txt
@@ -9,7 +9,7 @@
| alpha: | TODO |
| arc: | TODO |
| arm: | TODO |
- | arm64: | TODO |
+ | arm64: | ok |
| c6x: | TODO |
| csky: | TODO |
| h8300: | TODO |
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 663392d1eae2..928700f15e23 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -167,6 +167,7 @@ config ARM64
select HAVE_STACKPROTECTOR
select HAVE_SYSCALL_TRACEPOINTS
select HAVE_KPROBES
+ select HAVE_KPROBES_ON_FTRACE
select HAVE_KRETPROBES
select HAVE_GENERIC_VDSO
select IOMMU_DMA if IOMMU_SUPPORT
diff --git a/arch/arm64/kernel/probes/Makefile b/arch/arm64/kernel/probes/Makefile
index 8e4be92e25b1..4020cfc66564 100644
--- a/arch/arm64/kernel/probes/Makefile
+++ b/arch/arm64/kernel/probes/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_KPROBES) += kprobes.o decode-insn.o \
simulate-insn.o
obj-$(CONFIG_UPROBES) += uprobes.o decode-insn.o \
simulate-insn.o
+obj-$(CONFIG_KPROBES_ON_FTRACE) += ftrace.o
diff --git a/arch/arm64/kernel/probes/ftrace.c b/arch/arm64/kernel/probes/ftrace.c
new file mode 100644
index 000000000000..1f0c09d02bb8
--- /dev/null
+++ b/arch/arm64/kernel/probes/ftrace.c
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Dynamic Ftrace based Kprobes Optimization
+ *
+ * Copyright (C) Hitachi Ltd., 2012
+ * Copyright (C) 2019 Jisheng Zhang <jszhang@kernel.org>
+ * Synaptics Incorporated
+ */
+
+#include <linux/kprobes.h>
+
+/* Ftrace callback handler for kprobes -- called under preepmt disabed */
+void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
+ struct ftrace_ops *ops, struct pt_regs *regs)
+{
+ struct kprobe *p;
+ struct kprobe_ctlblk *kcb;
+
+ /* Preempt is disabled by ftrace */
+ p = get_kprobe((kprobe_opcode_t *)ip);
+ if (unlikely(!p) || kprobe_disabled(p))
+ return;
+
+ kcb = get_kprobe_ctlblk();
+ if (kprobe_running()) {
+ kprobes_inc_nmissed_count(p);
+ } else {
+ unsigned long orig_ip = instruction_pointer(regs);
+ /* Kprobe handler expects regs->pc = pc + 4 as breakpoint hit */
+ instruction_pointer_set(regs, ip + sizeof(kprobe_opcode_t));
+
+ __this_cpu_write(current_kprobe, p);
+ kcb->kprobe_status = KPROBE_HIT_ACTIVE;
+ if (!p->pre_handler || !p->pre_handler(p, regs)) {
+ /*
+ * Emulate singlestep (and also recover regs->pc)
+ * as if there is a nop
+ */
+ instruction_pointer_set(regs,
+ (unsigned long)p->addr + MCOUNT_INSN_SIZE);
+ if (unlikely(p->post_handler)) {
+ kcb->kprobe_status = KPROBE_HIT_SSDONE;
+ p->post_handler(p, regs, 0);
+ }
+ instruction_pointer_set(regs, orig_ip);
+ }
+ /*
+ * If pre_handler returns !0, it changes regs->pc. We have to
+ * skip emulating post_handler.
+ */
+ __this_cpu_write(current_kprobe, NULL);
+ }
+}
+NOKPROBE_SYMBOL(kprobe_ftrace_handler);
+
+int arch_prepare_kprobe_ftrace(struct kprobe *p)
+{
+ p->ainsn.api.insn = NULL;
+ return 0;
+}
diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index c4452827419b..f2bf8c70da79 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -551,6 +551,29 @@ void __kprobes __used *trampoline_probe_handler(struct pt_regs *regs)
return (void *)orig_ret_address;
}
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
+{
+ unsigned long addr = kallsyms_lookup_name(name);
+ unsigned long faddr;
+
+ /*
+ * with -fpatchable-function-entry=2, the first 4 bytes is the
+ * LR saver, then the actual call insn. So ftrace location is
+ * always on the first 4 bytes offset.
+ */
+ faddr = ftrace_location_range(addr, addr + AARCH64_INSN_SIZE);
+ if (faddr)
+ return (kprobe_opcode_t *)faddr;
+ return (kprobe_opcode_t *)addr;
+}
+
+bool arch_kprobe_on_func_entry(unsigned long offset)
+{
+ return offset <= AARCH64_INSN_SIZE;
+}
+#endif
+
void __kprobes arch_prepare_kretprobe(struct kretprobe_instance *ri,
struct pt_regs *regs)
{
--
2.23.0.rc1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [V3, 2/2] media: i2c: Add Omnivision OV02A10 camera sensor driver
From: Tomasz Figa @ 2019-08-21 10:30 UTC (permalink / raw)
To: dongchun.zhu
Cc: mark.rutland, devicetree, drinkcat, srv_heupstream, shengnan.wang,
louis.kuo, sj.huang, robh+dt, linux-mediatek, sakari.ailus,
matthias.bgg, bingbu.cao, mchehab, linux-arm-kernel, linux-media
In-Reply-To: <20190819034331.13098-3-dongchun.zhu@mediatek.com>
Hi Dongchun,
On Mon, Aug 19, 2019 at 11:43:31AM +0800, dongchun.zhu@mediatek.com wrote:
> From: Dongchun Zhu <dongchun.zhu@mediatek.com>
>
> This patch adds a V4L2 sub-device driver for OV02A10 image sensor.
> The OV02A10 is a 1/5" CMOS sensor from Omnivision,
> which supports output format: 10-bit Raw.
> The OV02A10 has a single MIPI lane interface and use the I2C bus
> for control and the CSI-2 bus for data.
>
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> ---
> MAINTAINERS | 1 +
> drivers/media/i2c/Kconfig | 11 +
> drivers/media/i2c/Makefile | 1 +
> drivers/media/i2c/ov02a10.c | 1018 +++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 1031 insertions(+)
> create mode 100644 drivers/media/i2c/ov02a10.c
>
Thanks for the patch! Please see my comments inline.
[snip]
> +#define CHIP_ID 0x2509
> +#define OV02A10_REG_CHIP_ID_H 0x02
> +#define OV02A10_REG_CHIP_ID_L 0x03
> +#define OV02A10_ID(_msb, _lsb) ((_msb) << 8 | (_lsb))
> +
> +/* Bit[1] vertical upside down */
> +/* Bit[0] horizontal mirror */
> +#define REG_MIRROR_FLIP_CONTROL 0x3f
> +
> +/* Orientation */
> +#define REG_CONFIG_MIRROR_FLIP 0x03
> +
> +#define REG_PAGE_SWITCH 0xfd
> +#define REG_GLOBAL_EFFECTIVE 0x01
> +#define REG_ENABLE BIT(0)
> +
> +#define REG_SC_CTRL_MODE 0xac
> +#define SC_CTRL_MODE_STANDBY 0x00
> +#define SC_CTRL_MODE_STREAMING 0x01
> +
> +#define OV02A10_REG_EXPOSURE_H 0x03
> +#define OV02A10_REG_EXPOSURE_L 0x04
> +#define OV02A10_EXPOSURE_MIN 4
> +#define OV02A10_EXPOSURE_STEP 1
> +
> +#define OV02A10_REG_VTS_H 0x05
> +#define OV02A10_REG_VTS_L 0x06
> +#define OV02A10_VTS_MAX 0x209f
> +#define OV02A10_VTS_MIN 0x04cf
> +#define OV02A10_BASIC_LINE 1224
> +
> +#define OV02A10_REG_GAIN 0x24
> +#define OV02A10_GAIN_MIN 0x10
> +#define OV02A10_GAIN_MAX 0xf8
> +#define OV02A10_GAIN_STEP 0x01
> +#define OV02A10_GAIN_DEFAULT 0x40
> +
> +#define REG_NULL 0xff
> +
> +#define OV02A10_LANES 1
> +#define OV02A10_BITS_PER_SAMPLE 10
> +
I think there is something wrong with the indentation in the code above.
Please use tabs wherever possible.
[snip]
> +
> +#define to_ov02a10(sd) container_of(sd, struct ov02a10, subdev)
Please use a static inline function for added compile-time type checks.
> +
> +static inline void msleep_range(unsigned int delay_base)
> +{
> + usleep_range(delay_base * 1000, delay_base * 1000 + 500);
> +}
Why not just use msleep()?
> +
> +/* MIPI color bar enable output */
> +static const struct regval ov02a10_test_pattern_enable_regs[] = {
> + {0xfd, 0x01},
> + {0x0d, 0x00},
> + {0xb6, 0x01},
> + {0x01, 0x01},
> + {0xfd, 0x01},
> + {0xac, 0x01},
> + {REG_NULL, 0x00}
> +};
> +
> +/* MIPI color bar disable output */
> +static const struct regval ov02a10_test_pattern_disable_regs[] = {
> + {0xfd, 0x01},
> + {0x0d, 0x00},
> + {0xb6, 0x00},
> + {0x01, 0x01},
> + {0xfd, 0x01},
> + {0xac, 0x01},
> + {REG_NULL, 0x00}
> +};
Hmm, only the register 0xb6 seems to here. Could we just set it directly,
without these arrays?
> +
> +/*
> + * xvclk 24Mhz
This seems to assume 24MHz, but the driver allows a range in probe. Is that
correct?
[snip]]
> +/* Write a register */
> +static int ov02a10_write_reg(struct ov02a10 *ov02a10, u8 addr, u8 val)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> + u8 buf[2] = {addr, val};
> + int ret;
> +
> + ret = i2c_master_send(client, buf, 2);
> +
> + if (ret != 2) {
> + dev_err(&client->dev, "%s: error: reg=%x, val=%x\n",
> + __func__, addr, val);
> + return -EIO;
> + }
> +
> + return 0;
> +}
Could this be replaced with i2c_smbus_write_byte_data()?
> +
> +static int ov02a10_write_array(struct ov02a10 *ov02a10,
> + const struct regval *regs)
> +{
> + u32 i;
> + int ret;
> +
> + for (i = 0; regs[i].addr != REG_NULL; i++) {
> + ret = ov02a10_write_reg(ov02a10, regs[i].addr, regs[i].val);
> + if (ret < 0)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +/* Read a register */
> +static int ov02a10_read_reg(struct ov02a10 *ov02a10, u8 reg, u8 *val)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> + u8 data = reg;
> + struct i2c_msg msg = {
> + .addr = client->addr,
> + .flags = 0,
> + .len = 1,
> + .buf = &data,
> + };
> + int ret;
> +
> + ret = i2c_transfer(client->adapter, &msg, 1);
> + if (ret < 0)
> + goto err_wr;
> +
> + msg.flags = I2C_M_RD;
> + ret = i2c_transfer(client->adapter, &msg, 1);
> + if (ret < 0)
> + goto err_rd;
Could we just have 2 messages in an array and just call i2c_transfer() once
for both write and read?
Or actually it sounds like the i2c_smbus_read_byte_data() helper could work
here.
> +
> + *val = data;
> + return 0;
> +
> +err_rd:
> + dev_err(&client->dev, "i2c_transfer --I2C_M_RD failed\n");
> +err_wr:
> + dev_err(&client->dev, "read error: reg=0x%02x: %d\n", reg, ret);
> + return ret;
> +}
> +
> +static void ov02a10_fill_fmt(const struct ov02a10_mode *mode,
> + struct v4l2_mbus_framefmt *fmt)
> +{
> + fmt->width = mode->width;
> + fmt->height = mode->height;
> + fmt->field = V4L2_FIELD_NONE;
> +}
> +
> +static int ov02a10_set_fmt(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_format *fmt)
> +{
> + struct ov02a10 *ov02a10 = to_ov02a10(sd);
> + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
> +
> + mutex_lock(&ov02a10->mutex);
> +
> + if (ov02a10->streaming) {
> + mutex_unlock(&ov02a10->mutex);
> + return -EBUSY;
> + }
> +
> + /* Only one sensor mode supported */
> + mbus_fmt->code = ov02a10->fmt.code;
> + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt);
> + ov02a10->fmt = fmt->format;
> +
> + mutex_unlock(&ov02a10->mutex);
> +
> + return 0;
> +}
> +
> +static int ov02a10_get_fmt(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_format *fmt)
> +{
> + struct ov02a10 *ov02a10 = to_ov02a10(sd);
> + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
> +
> + mutex_lock(&ov02a10->mutex);
> +
> + fmt->format = ov02a10->fmt;
> + mbus_fmt->code = ov02a10->fmt.code;
> + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt);
> +
> + mutex_unlock(&ov02a10->mutex);
> +
> + return 0;
> +}
> +
> +static int ov02a10_enum_mbus_code(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_mbus_code_enum *code)
> +{
> + struct ov02a10 *ov02a10 = to_ov02a10(sd);
> +
> + if (code->index >= ARRAY_SIZE(supported_modes) || !(code->index))
Hmm, ARRAY_SIZE(supported_modes) is 1 and we don't allow code->index to be
0 either. Is there a code->index value that wouldn't return an error here?
> + return -EINVAL;
> +
> + code->code = ov02a10->fmt.code;
> +
> + return 0;
> +}
> +
> +static int ov02a10_enum_frame_sizes(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_frame_size_enum *fse)
> +{
> + if (fse->index >= ARRAY_SIZE(supported_modes) || !(fse->index))
Same here.
> + return -EINVAL;
> +
> + fse->min_width = supported_modes[fse->index].width;
> + fse->max_width = supported_modes[fse->index].width;
> + fse->max_height = supported_modes[fse->index].height;
> + fse->min_height = supported_modes[fse->index].height;
> +
> + return 0;
> +}
> +
> +static int __ov02a10_power_on(struct ov02a10 *ov02a10)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> + struct device *dev = &client->dev;
> + int ret;
> +
> + ret = clk_prepare_enable(ov02a10->xvclk);
> + if (ret < 0) {
> + dev_err(dev, "Failed to enable xvclk\n");
> + return ret;
> + }
Is it really correct to enable the clock before the regulators?
According to the datasheet, it should be:
- PD pin HIGH,
- nRST pin LOW,
- DVDDIO and AVDD28 power up and stabilize,
- clock enabled,
- min 5 ms delay,
- PD pin LOW,
- min 4 ms delay,
- nRST pin HIGH,
- min 5 ms delay,
- I2C interface ready.
> +
> + /* Note: set 0 is high, set 1 is low */
Why is that? If there is some inverter on the way that should be handled
outside of this driver. (GPIO DT bindings have flags for this purpose.
If the pins are nRESET and nPOWERDOWN in the hardware datasheet, we should
call them like this in the driver too (+/- the lowercase and underscore
convention).
According to the datasheet, the reset pin is called RST and inverted, so we should
call it n_rst, but the powerdown signal, called PD, is not inverted, so pd
would be the right name.
> + gpiod_set_value_cansleep(ov02a10->reset_gpio, 1);
> + gpiod_set_value_cansleep(ov02a10->powerdown_gpio, 0);
> +
> + ret = regulator_bulk_enable(OV02A10_NUM_SUPPLIES, ov02a10->supplies);
> + if (ret < 0) {
> + dev_err(dev, "Failed to enable regulators\n");
> + goto disable_clk;
> + }
> + msleep_range(7);
> +
> + gpiod_set_value_cansleep(ov02a10->powerdown_gpio, 1);
> + msleep_range(10);
> +
> + gpiod_set_value_cansleep(ov02a10->reset_gpio, 0);
> + msleep_range(10);
> +
> + return 0;
> +
> +disable_clk:
> + clk_disable_unprepare(ov02a10->xvclk);
> +
> + return ret;
> +}
> +
> +static void __ov02a10_power_off(struct ov02a10 *ov02a10)
> +{
> + clk_disable_unprepare(ov02a10->xvclk);
> + gpiod_set_value_cansleep(ov02a10->reset_gpio, 1);
> + gpiod_set_value_cansleep(ov02a10->powerdown_gpio, 1);
> + regulator_bulk_disable(OV02A10_NUM_SUPPLIES, ov02a10->supplies);
This also doesn't seem to match my datasheet. The sequence there is:
- nRST goes LOW,
- clock stops,
- PD goes HIGH,
- regulators are powerd down.
> +}
> +
> +static int __ov02a10_start_stream(struct ov02a10 *ov02a10)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> + int ret;
> +
> + /* Apply default values of current mode */
> + ret = ov02a10_write_array(ov02a10, ov02a10->cur_mode->reg_list);
> + if (ret)
> + return ret;
> +
> + /* Apply customized values from user */
> + ret = __v4l2_ctrl_handler_setup(ov02a10->subdev.ctrl_handler);
> + if (ret)
> + return ret;
> +
> + /* Set orientation to 180 degree */
> + if (ov02a10->upside_down) {
> + ret = ov02a10_write_reg(ov02a10, REG_MIRROR_FLIP_CONTROL,
> + REG_CONFIG_MIRROR_FLIP);
> + if (ret) {
> + dev_err(&client->dev, "%s failed to set orientation\n",
> + __func__);
> + return ret;
> + }
> + ret = ov02a10_write_reg(ov02a10, REG_GLOBAL_EFFECTIVE,
> + REG_ENABLE);
> + if (ret < 0)
> + return ret;
> + }
> +
> + /* Set stream on register */
> + return ov02a10_write_reg(ov02a10,
> + REG_SC_CTRL_MODE, SC_CTRL_MODE_STREAMING);
> +}
> +
> +static int __ov02a10_stop_stream(struct ov02a10 *ov02a10)
> +{
> + return ov02a10_write_reg(ov02a10,
> + REG_SC_CTRL_MODE, SC_CTRL_MODE_STANDBY);
> +}
> +
> +static int ov02a10_entity_init_cfg(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_pad_config *cfg)
> +{
> + struct v4l2_subdev_format fmt = { 0 };
> +
> + fmt.which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> + fmt.format.width = 1600;
> + fmt.format.height = 1200;
Where do these values come from? Should we have some macros for them?
> +
> + ov02a10_set_fmt(subdev, cfg, &fmt);
> +
> + return 0;
> +}
> +
> +static int ov02a10_s_stream(struct v4l2_subdev *sd, int on)
> +{
> + struct ov02a10 *ov02a10 = to_ov02a10(sd);
> + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> + int ret = 0;
> +
> + mutex_lock(&ov02a10->mutex);
> +
> + if (ov02a10->streaming == on)
> + goto unlock_and_return;
> +
> + if (on) {
> + ret = pm_runtime_get_sync(&client->dev);
> + if (ret < 0) {
> + pm_runtime_put_noidle(&client->dev);
> + goto unlock_and_return;
> + }
> +
> + ret = __ov02a10_start_stream(ov02a10);
> + if (ret) {
> + __ov02a10_stop_stream(ov02a10);
> + ov02a10->streaming = !on;
> + goto err_rpm_put;
> + }
> + } else {
> + __ov02a10_stop_stream(ov02a10);
> + pm_runtime_put(&client->dev);
> + }
> +
> + ov02a10->streaming = on;
> + mutex_unlock(&ov02a10->mutex);
> +
> + return ret;
> +
> +err_rpm_put:
> + pm_runtime_put(&client->dev);
> +unlock_and_return:
> + mutex_unlock(&ov02a10->mutex);
> +
> + return ret;
> +}
> +
> +static int ov02a10_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> + struct ov02a10 *ov02a10 = to_ov02a10(sd);
> + struct v4l2_mbus_framefmt *try_fmt = v4l2_subdev_get_try_format(sd,
> + fh->pad,
> + 0);
Please separate the initialization from the declaration, because there
isn't just enough space on this line anymore.
> +
> + mutex_lock(&ov02a10->mutex);
> + /* Initialize try_fmt */
> + try_fmt->code = ov02a10->fmt.code;
> + ov02a10_fill_fmt(&supported_modes[0], try_fmt);
> +
> + mutex_unlock(&ov02a10->mutex);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused ov02a10_runtime_resume(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct ov02a10 *ov02a10 = to_ov02a10(sd);
> +
> + return __ov02a10_power_on(ov02a10);
> +}
> +
> +static int __maybe_unused ov02a10_runtime_suspend(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct ov02a10 *ov02a10 = to_ov02a10(sd);
> +
> + __ov02a10_power_off(ov02a10);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops ov02a10_pm_ops = {
> + SET_RUNTIME_PM_OPS(ov02a10_runtime_suspend,
> + ov02a10_runtime_resume, NULL)
Don't we need to implement and provide system PM ops too?
> +};
> +
> +static int ov02a10_set_test_pattern(struct ov02a10 *ov02a10, s32 value)
> +{
> + if (value)
> + return ov02a10_write_array(ov02a10,
> + ov02a10_test_pattern_enable_regs);
> +
> + return ov02a10_write_array(ov02a10,
> + ov02a10_test_pattern_disable_regs);
> +}
> +
> +static int ov02a10_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct ov02a10 *ov02a10 = container_of(ctrl->handler,
> + struct ov02a10, ctrl_handler);
> + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> + s64 max_expo;
> + int ret;
> +
> + /* Propagate change of current control to all related controls */
> + if (ctrl->id == V4L2_CID_VBLANK) {
> + /* Update max exposure while meeting expected vblanking */
> + max_expo = ov02a10->cur_mode->height + ctrl->val - 4;
> + __v4l2_ctrl_modify_range(ov02a10->exposure,
> + ov02a10->exposure->minimum, max_expo,
> + ov02a10->exposure->step,
> + ov02a10->exposure->default_value);
> + }
> +
> + /* V4L2 controls values will be applied only when power is already up */
> + if (!pm_runtime_get_if_in_use(&client->dev))
> + return 0;
> +
> + switch (ctrl->id) {
> + case V4L2_CID_EXPOSURE:
> + ret = ov02a10_write_reg(ov02a10, REG_PAGE_SWITCH, REG_ENABLE);
> + if (ret < 0)
> + return ret;
> + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_EXPOSURE_H,
> + ((ctrl->val >> 8) & 0xFF));
> + if (!ret) {
> + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_EXPOSURE_L,
> + (ctrl->val & 0xFF));
> + if (ret < 0)
> + return ret;
> + }
> + ret = ov02a10_write_reg(ov02a10, REG_GLOBAL_EFFECTIVE,
> + REG_ENABLE);
> + if (ret < 0)
> + return ret;
> + break;
> + case V4L2_CID_ANALOGUE_GAIN:
> + ret = ov02a10_write_reg(ov02a10, REG_PAGE_SWITCH, REG_ENABLE);
> + if (ret < 0)
> + return ret;
> + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_GAIN,
> + (ctrl->val & 0xFF));
> + if (ret < 0)
> + return ret;
> + ret = ov02a10_write_reg(ov02a10, REG_GLOBAL_EFFECTIVE,
> + REG_ENABLE);
> + if (ret < 0)
> + return ret;
> + break;
> + case V4L2_CID_VBLANK:
> + ret = ov02a10_write_reg(ov02a10, REG_PAGE_SWITCH, REG_ENABLE);
> + if (ret < 0)
> + return ret;
> + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_VTS_H,
> + (((ctrl->val +
> + ov02a10->cur_mode->height -
> + OV02A10_BASIC_LINE) >> 8)
> + & 0xFF));
> + if (!ret) {
> + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_VTS_L,
> + ((ctrl->val +
> + ov02a10->cur_mode->height -
> + OV02A10_BASIC_LINE) & 0xFF));
> + if (ret < 0)
> + return ret;
> + }
> + ret = ov02a10_write_reg(ov02a10, REG_GLOBAL_EFFECTIVE,
> + REG_ENABLE);
> + if (ret < 0)
> + return ret;
> + break;
> + case V4L2_CID_TEST_PATTERN:
> + ret = ov02a10_set_test_pattern(ov02a10, ctrl->val);
> + if (ret < 0)
> + return ret;
> + break;
> + default:
> + dev_warn(&client->dev, "%s Unhandled id:0x%x, val:0x%x\n",
> + __func__, ctrl->id, ctrl->val);
> + ret = -EINVAL;
> + break;
We shouldn't need to handle this, as the control framework wouldn't call us
with a control that we didn't register explicitly.
> + };
> +
> + pm_runtime_put(&client->dev);
> +
> + return ret;
> +}
> +
> +static const struct v4l2_subdev_video_ops ov02a10_video_ops = {
> + .s_stream = ov02a10_s_stream,
> +};
> +
> +static const struct v4l2_subdev_pad_ops ov02a10_pad_ops = {
> + .init_cfg = ov02a10_entity_init_cfg,
> + .enum_mbus_code = ov02a10_enum_mbus_code,
> + .enum_frame_size = ov02a10_enum_frame_sizes,
> + .get_fmt = ov02a10_get_fmt,
> + .set_fmt = ov02a10_set_fmt,
> +};
> +
> +static const struct v4l2_subdev_ops ov02a10_subdev_ops = {
> + .video = &ov02a10_video_ops,
> + .pad = &ov02a10_pad_ops,
> +};
> +
> +static const struct media_entity_operations ov02a10_subdev_entity_ops = {
> + .link_validate = v4l2_subdev_link_validate,
> +};
> +
> +static const struct v4l2_subdev_internal_ops ov02a10_internal_ops = {
> + .open = ov02a10_open,
> +};
> +
> +static const struct v4l2_ctrl_ops ov02a10_ctrl_ops = {
> + .s_ctrl = ov02a10_set_ctrl,
> +};
> +
> +static int ov02a10_initialize_controls(struct ov02a10 *ov02a10)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> + const struct ov02a10_mode *mode;
> + struct v4l2_ctrl_handler *handler;
> + struct v4l2_ctrl *ctrl;
> + u64 exposure_max;
> + u32 pixel_rate, h_blank;
> + int ret;
> +
> + handler = &ov02a10->ctrl_handler;
> + mode = ov02a10->cur_mode;
> + ret = v4l2_ctrl_handler_init(handler, 10);
I can see 6 controls registered below.
> + if (ret)
> + return ret;
> + handler->lock = &ov02a10->mutex;
> +
> + ctrl = v4l2_ctrl_new_int_menu(handler, NULL, V4L2_CID_LINK_FREQ,
> + 0, 0, link_freq_menu_items);
> + if (ctrl)
> + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> + pixel_rate = (link_freq_menu_items[0] * 2 * OV02A10_LANES) /
> + OV02A10_BITS_PER_SAMPLE;
> + v4l2_ctrl_new_std(handler, NULL, V4L2_CID_PIXEL_RATE,
> + 0, pixel_rate, 1, pixel_rate);
> +
> + h_blank = mode->hts_def - mode->width;
> + ov02a10->hblank = v4l2_ctrl_new_std(handler, NULL, V4L2_CID_HBLANK,
> + h_blank, h_blank, 1, h_blank);
> + if (ov02a10->hblank)
> + ov02a10->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> + ov02a10->vblank = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops,
> + V4L2_CID_VBLANK, mode->vts_def -
> + mode->height,
> + OV02A10_VTS_MAX - mode->height, 1,
> + mode->vts_def - mode->height);
> +
> + exposure_max = mode->vts_def - 4;
> + ov02a10->exposure = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops,
> + V4L2_CID_EXPOSURE,
> + OV02A10_EXPOSURE_MIN,
> + exposure_max,
> + OV02A10_EXPOSURE_STEP,
> + mode->exp_def);
> +
> + ov02a10->anal_gain = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops,
> + V4L2_CID_ANALOGUE_GAIN,
> + OV02A10_GAIN_MIN,
> + OV02A10_GAIN_MAX,
> + OV02A10_GAIN_STEP,
> + OV02A10_GAIN_DEFAULT);
> +
> + ov02a10->test_pattern =
> + v4l2_ctrl_new_std_menu_items(handler,
> + &ov02a10_ctrl_ops,
> + V4L2_CID_TEST_PATTERN,
> + ARRAY_SIZE(ov02a10_test_pattern_menu) -
> + 1, 0, 0, ov02a10_test_pattern_menu);
> +
> + if (handler->error) {
> + ret = handler->error;
> + dev_err(&client->dev,
> + "Failed to init controls(%d)\n", ret);
> + goto err_free_handler;
> + }
> +
> + ov02a10->subdev.ctrl_handler = handler;
> +
> + return 0;
> +
> +err_free_handler:
> + v4l2_ctrl_handler_free(handler);
> +
> + return ret;
> +}
> +
> +static int ov02a10_check_sensor_id(struct ov02a10 *ov02a10)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> + u16 id;
> + u8 pid = 0;
> + u8 ver = 0;
> + int ret;
> +
> + /* Check sensor revision */
> + ret = ov02a10_read_reg(ov02a10, OV02A10_REG_CHIP_ID_H, &pid);
> + if (ret)
> + return ret;
> +
> + ret = ov02a10_read_reg(ov02a10, OV02A10_REG_CHIP_ID_L, &ver);
> + if (ret)
> + return ret;
> +
> + id = OV02A10_ID(pid, ver);
> + if (id != CHIP_ID) {
> + dev_err(&client->dev, "Unexpected sensor id(%04x)\n", id);
> + return ret;
> + }
> +
> + dev_info(&client->dev, "Detected OV%04X sensor\n", id);
> +
> + return 0;
> +}
> +
> +static int ov02a10_configure_regulators(struct ov02a10 *ov02a10)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> + unsigned int i;
> +
> + for (i = 0; i < OV02A10_NUM_SUPPLIES; i++)
> + ov02a10->supplies[i].supply = ov02a10_supply_names[i];
> +
> + return devm_regulator_bulk_get(&client->dev,
> + OV02A10_NUM_SUPPLIES,
> + ov02a10->supplies);
> +}
I think we can just have this directly inside probe.
> +
> +static int ov02a10_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct ov02a10 *ov02a10;
> + u32 rotation;
> + u32 xclk_freq;
> + int ret;
> +
> + ov02a10 = devm_kzalloc(dev, sizeof(*ov02a10), GFP_KERNEL);
> + if (!ov02a10)
> + return -ENOMEM;
> +
> + v4l2_i2c_subdev_init(&ov02a10->subdev, client, &ov02a10_subdev_ops);
> + ov02a10->fmt.code = MEDIA_BUS_FMT_SBGGR10_1X10;
> +
> + /* Optional indication of physical rotation of sensor */
> + ret = fwnode_property_read_u32(dev_fwnode(dev), "rotation",
> + &rotation);
> + if (!ret) {
> + switch (rotation) {
> + case 180:
> + ov02a10->upside_down = true;
> + ov02a10->fmt.code = MEDIA_BUS_FMT_SRGGB10_1X10;
> + break;
> + case 0:
> + break;
> + default:
> + dev_warn(dev, "%u degrees rotation is not supported, ignoring...\n",
> + rotation);
> + }
> + }
> +
> + /* Get system clock (xvclk) */
> + ov02a10->xvclk = devm_clk_get(dev, "xvclk");
> + if (IS_ERR(ov02a10->xvclk)) {
> + dev_err(dev, "Failed to get xvclk\n");
> + return -EINVAL;
> + }
Hmm, it's called eclk in my datasheet.
> +
> + ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq);
> + if (ret) {
> + dev_err(dev, "Failed to get xclk frequency\n");
> + return ret;
> + }
> +
> + /* External clock must be 24MHz, allow 1% tolerance */
> + if (xclk_freq < 23760000 || xclk_freq > 24240000) {
How do we support a range of frequencies? I don't see the driver calculate
any register values based on this frequency. Are you sure that the register
arrays don't assume one specific frequency?
> + dev_err(dev, "external clock frequency %u is not supported\n",
> + xclk_freq);
> + return -EINVAL;
> + }
> + dev_dbg(dev, "external clock frequency %u\n", xclk_freq);
> +
> + ret = clk_set_rate(ov02a10->xvclk, xclk_freq);
> + if (ret) {
> + dev_err(dev, "Failed to set xvclk frequency (24MHz)\n");
> + return ret;
> + }
> +
> + ov02a10->powerdown_gpio = devm_gpiod_get(dev, "powerdown",
> + GPIOD_OUT_LOW);
Hmm, shouldn't this be HIGH? At least the datasheet has it so for the
powered down state.
> + if (IS_ERR(ov02a10->powerdown_gpio)) {
> + dev_err(dev, "Failed to get powerdown-gpios\n");
> + return -EINVAL;
> + }
> +
> + ov02a10->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
Also LOW here, similarly to the above.
> + if (IS_ERR(ov02a10->reset_gpio)) {
> + dev_err(dev, "Failed to get reset-gpios\n");
> + return -EINVAL;
> + }
> +
> + ret = ov02a10_configure_regulators(ov02a10);
> + if (ret) {
> + dev_err(dev, "Failed to get power regulators\n");
> + return ret;
> + }
> +
> + mutex_init(&ov02a10->mutex);
> + ov02a10->cur_mode = &supported_modes[0];
> + ret = ov02a10_initialize_controls(ov02a10);
> + if (ret) {
> + dev_err(dev, "Failed to initialize controls\n");
> + goto err_destroy_mutex;
> + }
> +
> + ret = __ov02a10_power_on(ov02a10);
> + if (ret)
> + goto err_free_handler;
> +
> + ret = ov02a10_check_sensor_id(ov02a10);
> + if (ret)
> + goto err_power_off;
> +
> + ov02a10->subdev.internal_ops = &ov02a10_internal_ops;
> + ov02a10->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> + ov02a10->subdev.entity.ops = &ov02a10_subdev_entity_ops;
> + ov02a10->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> + ov02a10->pad.flags = MEDIA_PAD_FL_SOURCE;
> + ret = media_entity_pads_init(&ov02a10->subdev.entity, 1, &ov02a10->pad);
> + if (ret < 0) {
> + dev_err(dev, "failed to init entity pads: %d", ret);
> + goto err_power_off;
> + }
> +
> + ret = v4l2_async_register_subdev(&ov02a10->subdev);
> + if (ret) {
> + dev_err(dev, "failed to register V4L2 subdev: %d",
> + ret);
> + goto err_clean_entity;
> + }
> +
> + pm_runtime_set_active(dev);
> + pm_runtime_enable(dev);
> + pm_runtime_idle(dev);
> +
> + dev_info(dev, "ov02a10 probe --\n");
Please remove this.
> + return 0;
> +
> +err_clean_entity:
> + media_entity_cleanup(&ov02a10->subdev.entity);
> +err_power_off:
> + __ov02a10_power_off(ov02a10);
> +err_free_handler:
> + v4l2_ctrl_handler_free(ov02a10->subdev.ctrl_handler);
> +err_destroy_mutex:
> + mutex_destroy(&ov02a10->mutex);
> +
> + return ret;
> +}
> +
> +static int ov02a10_remove(struct i2c_client *client)
> +{
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct ov02a10 *ov02a10 = to_ov02a10(sd);
> +
> + v4l2_async_unregister_subdev(sd);
> + media_entity_cleanup(&sd->entity);
> + v4l2_ctrl_handler_free(sd->ctrl_handler);
> + pm_runtime_disable(&client->dev);
> + if (!pm_runtime_status_suspended(&client->dev))
> + __ov02a10_power_off(ov02a10);
> + pm_runtime_set_suspended(&client->dev);
> + mutex_destroy(&ov02a10->mutex);
> +
> + return 0;
> +}
> +
> +#if IS_ENABLED(CONFIG_OF)
> +static const struct of_device_id ov02a10_of_match[] = {
> + { .compatible = "ovti,ov02a10" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, ov02a10_of_match);
> +#endif
> +
> +static struct i2c_driver ov02a10_i2c_driver = {
> + .driver = {
> + .name = "ov02a10",
> + .pm = &ov02a10_pm_ops,
> + .of_match_table = ov02a10_of_match,
Please use of_match_ptr() wrapper.
Best regards,
Tomasz
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox