* Re: [PATCH 0/9] drm: meson: global clean-up (use proper macros, update comments ...)
From: Neil Armstrong @ 2019-08-08 14:12 UTC (permalink / raw)
To: Kevin Hilman, Julien Masson
Cc: linux-amlogic, dri-devel, linux-arm-kernel, linux-kernel
In-Reply-To: <7ho92mwor0.fsf@baylibre.com>
On 25/06/2019 01:24, Kevin Hilman wrote:
> Julien Masson <jmasson@baylibre.com> writes:
>
>> This patch series aims to clean-up differents parts of the drm meson
>> code source.
>>
>> Couple macros have been defined and used to set several registers
>> instead of using magic constants.
>>
>> I also took the opportunity to:
>> - add/remove/update comments
>> - remove useless code
>> - minor fix/improvment
>
> Nice set of cleanups, thanks! I especially like the extra in-code
> comments.
>
> Could you also add to the cover-letter how this was tested, and on what
> platforms so we know it's not going to introduce any regressions.
>
> Thanks,
>
> Kevin
>
Apart the wrong magic value in patch 4 that I'll fix while applying,
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
I'll run a few tests on all the supported SoC versions:
- GXBB
- GXL
- GXM
- G12A/G12B
and push to drm-misc-next.
Neil
_______________________________________________
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 8/9] drm: meson: add macro used to enable HDMI PLL
From: Neil Armstrong @ 2019-08-08 14:10 UTC (permalink / raw)
To: Kevin Hilman, 86zhm782g5.fsf
Cc: Julien Masson, linux-amlogic, dri-devel, linux-arm-kernel,
linux-kernel
In-Reply-To: <7hwohawoxu.fsf@baylibre.com>
On 25/06/2019 01:20, Kevin Hilman wrote:
> Julien Masson <jmasson@baylibre.com> writes:
>
>> This patch add new macro HHI_HDMI_PLL_CNTL_EN which is used to enable
>> HDMI PLL.
>>
>> Signed-off-by: Julien Masson <jmasson@baylibre.com>
>> ---
>> drivers/gpu/drm/meson/meson_vclk.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/meson/meson_vclk.c b/drivers/gpu/drm/meson/meson_vclk.c
>> index e7c2b439d0f7..be6e152fc75a 100644
>> --- a/drivers/gpu/drm/meson/meson_vclk.c
>> +++ b/drivers/gpu/drm/meson/meson_vclk.c
>> @@ -96,6 +96,7 @@
>> #define HHI_VDAC_CNTL1 0x2F8 /* 0xbe offset in data sheet */
>>
>> #define HHI_HDMI_PLL_CNTL 0x320 /* 0xc8 offset in data sheet */
>> +#define HHI_HDMI_PLL_CNTL_EN BIT(30)
>> #define HHI_HDMI_PLL_CNTL2 0x324 /* 0xc9 offset in data sheet */
>> #define HHI_HDMI_PLL_CNTL3 0x328 /* 0xca offset in data sheet */
>> #define HHI_HDMI_PLL_CNTL4 0x32C /* 0xcb offset in data sheet */
>> @@ -468,7 +469,7 @@ void meson_hdmi_pll_set_params(struct meson_drm *priv, unsigned int m,
>>
>> /* Enable and unreset */
>> regmap_update_bits(priv->hhi, HHI_HDMI_PLL_CNTL,
>> - 0x7 << 28, 0x4 << 28);
>> + 0x7 << 28, HHI_HDMI_PLL_CNTL_EN);
I'll do a pass on the PLL part since it needs much more work than a cleanup,
it's ok for me.
Neil
>
> still using a magic const for the mask. Can use GENMASK() for this?
>
> Kevin
>
_______________________________________________
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 4/9] drm: meson: vpp: use proper macros instead of magic constants
From: Neil Armstrong @ 2019-08-08 14:09 UTC (permalink / raw)
To: Kevin Hilman, 86zhm782g5.fsf
Cc: Julien Masson, linux-amlogic, dri-devel, linux-arm-kernel,
linux-kernel
In-Reply-To: <7h36jyy3qn.fsf@baylibre.com>
On 25/06/2019 01:15, Kevin Hilman wrote:
> Julien Masson <jmasson@baylibre.com> writes:
>
>> This patch add new macros which are used to set the following
>> registers:
>> - VPP_OSD_SCALE_COEF_IDX
>> - VPP_DOLBY_CTRL
>> - VPP_OFIFO_SIZE
>> - VPP_HOLD_LINES
>> - VPP_SC_MISC
>> - VPP_VADJ_CTRL
>>
>> Signed-off-by: Julien Masson <jmasson@baylibre.com>
>
> [...]
>
>> @@ -97,20 +97,22 @@ void meson_vpp_init(struct meson_drm *priv)
>> else if (meson_vpu_is_compatible(priv, "amlogic,meson-gxm-vpu")) {
>> writel_bits_relaxed(0xff << 16, 0xff << 16,
>> priv->io_base + _REG(VIU_MISC_CTRL1));
>> - writel_relaxed(0x20000, priv->io_base + _REG(VPP_DOLBY_CTRL));
>> - writel_relaxed(0x1020080,
>> + writel_relaxed(VPP_PPS_DUMMY_DATA_MODE,
>> + priv->io_base + _REG(VPP_DOLBY_CTRL));
>> + writel_relaxed(0x108080,
>
> nit: still a magic constant here, and it's not obvious why it's
> different from the current one.
This is a magic constant, it's a color, but indeed it should not change.
0x1020080 is the 10bit variant of 0x108080, on purpose.
Neil
>
> Kevin
>
_______________________________________________
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] arm64: Clarify when cpu_enable() is called
From: Mark Brown @ 2019-08-08 14:07 UTC (permalink / raw)
To: Suzuki K Poulose, will, catalin.marinas; +Cc: linux-arm-kernel
In-Reply-To: <20190808140554.13212-1-broonie@kernel.org>
[-- Attachment #1.1: Type: text/plain, Size: 434 bytes --]
On Thu, Aug 08, 2019 at 03:05:54PM +0100, Mark Brown wrote:
> Strengthen the wording in the documentation for cpu_enable() to make it
> more obvious to readers not already familiar with the code when the core
> will call this callback and that this is intentional.
>
> Signed-off-by: Mark Brown <broonie@kernel.org>
Missed this at the bottom of Suzuki's mail sorry:
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
[-- 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
* [PATCH v3] arm64: Clarify when cpu_enable() is called
From: Mark Brown @ 2019-08-08 14:05 UTC (permalink / raw)
To: Suzuki K Poulose, will, catalin.marinas; +Cc: Mark Brown, linux-arm-kernel
Strengthen the wording in the documentation for cpu_enable() to make it
more obvious to readers not already familiar with the code when the core
will call this callback and that this is intentional.
Signed-off-by: Mark Brown <broonie@kernel.org>
---
v3: Whitespace
v2: Much more verbose wording from Suzuki.
arch/arm64/include/asm/cpufeature.h | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index cf65a47ee6b4..eaeb030d0b95 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -289,9 +289,16 @@ struct arm64_cpu_capabilities {
u16 type;
bool (*matches)(const struct arm64_cpu_capabilities *caps, int scope);
/*
- * Take the appropriate actions to enable this capability for this CPU.
- * For each successfully booted CPU, this method is called for each
- * globally detected capability.
+ * Take the appropriate actions to configure this capability
+ * for this CPU. If the capability is detected by the kernel
+ * this will be called on all the CPUs in the system,
+ * including the hotplugged CPUs, regardless of if the
+ * capability was *available* on that specific CPU. This is
+ * useful for some capabilities (e.g, working around CPU
+ * errata), where all the CPUs must take some action (e.g,
+ * changing system control/configuration). Thus, if an action
+ * is required only if the CPU has the capability, then the
+ * routine must check it before taking any action.
*/
void (*cpu_enable)(const struct arm64_cpu_capabilities *cap);
union {
--
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 v2] arm64: Clarify when cpu_enable() is called
From: Suzuki K Poulose @ 2019-08-08 13:57 UTC (permalink / raw)
To: broonie, will, catalin.marinas; +Cc: linux-arm-kernel
In-Reply-To: <20190808135328.12655-1-broonie@kernel.org>
On 08/08/2019 14:53, Mark Brown wrote:
> Strengthen the wording in the documentation for cpu_enable() to make it
> more obvious to readers not already familiar with the code when the core
> will call this callback and that this is intentional.
>
Looks good to me, except for some minor space related issues below.
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>
> v2: Much more verbose wording from Suzuki.
>
> arch/arm64/include/asm/cpufeature.h | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index cf65a47ee6b4..2447d4afbf54 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -289,9 +289,17 @@ struct arm64_cpu_capabilities {
> u16 type;
> bool (*matches)(const struct arm64_cpu_capabilities *caps, int scope);
> /*
> - * Take the appropriate actions to enable this capability for this CPU.
> - * For each successfully booted CPU, this method is called for each
> - * globally detected capability.
> + * Take the appropriate actions to configure this capability
> + * for this CPU. If the capability is detected by the kernel
minor nit: double space ^^
> + * this will be called on all the CPUs in the system,
> + * including the hotplugged CPUs, regardless of if the
> + * capability was *available* on that specific CPU. This is
> + * useful for some capabilities (e.g, working around CPU
> + * errata), where all the CPUs must take some action (e.g,
> + * changing system control/configuration). Thus, if an action
and here before "Thus".
> + * is required only if the CPU has the capability then the
> + * routine must check it before taking any action.
> +
spurious new line ?
> */
> void (*cpu_enable)(const struct arm64_cpu_capabilities *cap);
> union {
>
With the above addressed,
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
_______________________________________________
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 V5 0/3] perf: imx8_ddr_perf: add AXI ID filter
From: Frank Li @ 2019-08-08 13:56 UTC (permalink / raw)
To: Joakim Zhang, robin.murphy@arm.com, will@kernel.org,
mark.rutland@arm.com
Cc: dl-linux-imx, linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190808064216.6950-1-qiangqing.zhang@nxp.com>
> -----Original Message-----
> From: Joakim Zhang
> Sent: Thursday, August 8, 2019 1:45 AM
> To: robin.murphy@arm.com; will@kernel.org; mark.rutland@arm.com
> Cc: Frank Li <frank.li@nxp.com>; linux-arm-kernel@lists.infradead.org; dl-linux-
> imx <linux-imx@nxp.com>; Joakim Zhang <qiangqing.zhang@nxp.com>
> Subject: [PATCH V5 0/3] perf: imx8_ddr_perf: add AXI ID filter
>
> Add AXI ID filter for imx8m ddr perf.
>
> Joakim Zhang (3):
> perf: imx8_ddr_perf: add AXI ID filter support
> Documentation: admin-guide: perf: add i.MX8 ddr pmu user doc
> MAINTAINERS: add imx8 ddr perf admin-guide maintainer information
>
> Documentation/admin-guide/perf/imx-ddr.rst | 30 +++++++++++
> MAINTAINERS | 1 +
> drivers/perf/fsl_imx8_ddr_perf.c | 63 +++++++++++++++++++++-
> 3 files changed, 92 insertions(+), 2 deletions(-) create mode 100644
> Documentation/admin-guide/perf/imx-ddr.rst
>
Acked-by: Frank Li <Frank.li@nxp.com>
> --
> 2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [V1, 2/2] media: i2c: Add more sensor mode for ov8856 camera sensor
From: Sakari Ailus @ 2019-08-08 13:53 UTC (permalink / raw)
To: dongchun.zhu
Cc: mark.rutland, devicetree, drinkcat, srv_heupstream, shengnan.wang,
tfiga, louis.kuo, sj.huang, robh+dt, linux-mediatek, sakari.ailus,
matthias.bgg, bingbu.cao, mchehab, linux-arm-kernel, linux-media
In-Reply-To: <20190808092215.5608-3-dongchun.zhu@mediatek.com>
Hi Dongchun,
Thanks for the patch.
On Thu, Aug 08, 2019 at 05:22:15PM +0800, dongchun.zhu@mediatek.com wrote:
> From: Dongchun Zhu <dongchun.zhu@mediatek.com>
>
> This patch mainly adds two more sensor modes for OV8856 image sensor.
> The OV8856 driver currently supports output format: 10-bit Raw,
> the resolution of 1632*1224 and 3264*2448, and the bayer order of BGGR.
> The hardware version also differs in some OTP regiser,
> as well as PLL register setting.
>
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> ---
> drivers/media/i2c/ov8856.c | 624 ++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 621 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> index cd347d6..e0610b6 100644
> --- a/drivers/media/i2c/ov8856.c
> +++ b/drivers/media/i2c/ov8856.c
> @@ -1,12 +1,15 @@
> // SPDX-License-Identifier: GPL-2.0
> // Copyright (c) 2019 Intel Corporation.
>
> +#include <linux/clk.h>
> #include <asm/unaligned.h>
> #include <linux/acpi.h>
> #include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/i2c.h>
> #include <linux/module.h>
> #include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> #include <media/v4l2-ctrls.h>
> #include <media/v4l2-device.h>
> #include <media/v4l2-fwnode.h>
> @@ -19,6 +22,7 @@
> #define OV8856_LINK_FREQ_180MHZ 180000000ULL
> #define OV8856_SCLK 144000000ULL
> #define OV8856_MCLK 19200000
> +#define OV8856_XVCLK_FREQ 24000000
The driver currenctly uses, perhaps misleadingly, OV8856_MCLK for this
purpose. You could rename the existing MCLK as XVCLK.
This also means the driver needs to differentiate configurations for 24 and
19,2 MHz which it currently does not do. I think it may make sense to make
this a separate patch from the rest.
> #define OV8856_DATA_LANES 4
> #define OV8856_RGB_DEPTH 10
>
> @@ -29,6 +33,18 @@
> #define OV8856_MODE_STANDBY 0x00
> #define OV8856_MODE_STREAMING 0x01
>
> +/* define 1B module */
> +#define OV8856_1B_MODULE 0x02
> +
> +/* otp sram register */
> +#define OV8856_OTP_REG 0x700f
> +#define OV8856_OTP_REG_ONE 0x3d84
> +#define OV8856_OTP_REG_TWO 0x3d81
> +
> +/* clock register */
> +#define OV8856_CLK_REG 0x3614
> +#define OV8856_CLK_REG_1B_VAL 0x20
> +
> /* vertical-timings from sensor */
> #define OV8856_REG_VTS 0x380e
> #define OV8856_VTS_MAX 0x7fff
> @@ -64,6 +80,14 @@
>
> #define to_ov8856(_sd) container_of(_sd, struct ov8856, sd)
>
> +static const char * const ov8856_supply_names[] = {
> + "dovdd", /* Digital I/O power */
> + "avdd", /* Analog power */
> + "dvdd", /* Digital core power */
> +};
> +
> +#define OV8856_NUM_SUPPLIES ARRAY_SIZE(ov8856_supply_names)
> +
> enum {
> OV8856_LINK_FREQ_720MBPS,
> OV8856_LINK_FREQ_360MBPS,
> @@ -316,6 +340,208 @@ static const struct ov8856_reg mode_3280x2464_regs[] = {
> {0x5e00, 0x00}
> };
>
> +static const struct ov8856_reg mode_3264x2448_regs[] = {
> + {0x0103, 0x01},
> + {0x0302, 0x3c},
> + {0x0303, 0x01},
> + {0x031e, 0x0c},
> + {0x3000, 0x00},
> + {0x300e, 0x00},
> + {0x3010, 0x00},
> + {0x3015, 0x84},
> + {0x3018, 0x72},
> + {0x3021, 0x23},
> + {0x3033, 0x24},
> + {0x3500, 0x00},
> + {0x3501, 0x9a},
> + {0x3502, 0x20},
> + {0x3503, 0x08},
> + {0x3505, 0x83},
> + {0x3508, 0x01},
> + {0x3509, 0x80},
> + {0x350c, 0x00},
> + {0x350d, 0x80},
> + {0x350e, 0x04},
> + {0x350f, 0x00},
> + {0x3510, 0x00},
> + {0x3511, 0x02},
> + {0x3512, 0x00},
> + {0x3600, 0x72},
> + {0x3601, 0x40},
> + {0x3602, 0x30},
> + {0x3610, 0xc5},
> + {0x3611, 0x58},
> + {0x3612, 0x5c},
> + {0x3613, 0xca},
> + {0x3614, 0x60},
> + {0x3628, 0xff},
> + {0x3629, 0xff},
> + {0x362a, 0xff},
> + {0x3633, 0x10},
> + {0x3634, 0x10},
> + {0x3635, 0x10},
> + {0x3636, 0x10},
> + {0x3663, 0x08},
> + {0x3669, 0x34},
> + {0x366d, 0x00},
> + {0x366e, 0x10},
> + {0x3706, 0x86},
> + {0x370b, 0x7e},
> + {0x3714, 0x23},
> + {0x3730, 0x12},
> + {0x3733, 0x10},
> + {0x3764, 0x00},
> + {0x3765, 0x00},
> + {0x3769, 0x62},
> + {0x376a, 0x2a},
> + {0x376b, 0x30},
> + {0x3780, 0x00},
> + {0x3781, 0x24},
> + {0x3782, 0x00},
> + {0x3783, 0x23},
> + {0x3798, 0x2f},
> + {0x37a1, 0x60},
> + {0x37a8, 0x6a},
> + {0x37ab, 0x3f},
> + {0x37c2, 0x04},
> + {0x37c3, 0xf1},
> + {0x37c9, 0x80},
> + {0x37cb, 0x16},
> + {0x37cc, 0x16},
> + {0x37cd, 0x16},
> + {0x37ce, 0x16},
> + {0x3800, 0x00},
> + {0x3801, 0x00},
> + {0x3802, 0x00},
> + {0x3803, 0x0c},
> + {0x3804, 0x0c},
> + {0x3805, 0xdf},
> + {0x3806, 0x09},
> + {0x3807, 0xa3},
> + {0x3808, 0x0c},
> + {0x3809, 0xc0},
> + {0x380a, 0x09},
> + {0x380b, 0x90},
> + {0x380c, 0x07},
> + {0x380d, 0x8c},
> + {0x380e, 0x09},
> + {0x380f, 0xb2},
> + {0x3810, 0x00},
> + {0x3811, 0x04},
> + {0x3812, 0x00},
> + {0x3813, 0x02},
> + {0x3814, 0x01},
> + {0x3815, 0x01},
> + {0x3816, 0x00},
> + {0x3817, 0x00},
> + {0x3818, 0x00},
> + {0x3819, 0x00},
> + {0x3820, 0x80},
> + {0x3821, 0x46},
> + {0x382a, 0x01},
> + {0x382b, 0x01},
> + {0x3830, 0x06},
> + {0x3836, 0x02},
> + {0x3862, 0x04},
> + {0x3863, 0x08},
> + {0x3cc0, 0x33},
> + {0x3d85, 0x17},
> + {0x3d8c, 0x73},
> + {0x3d8d, 0xde},
> + {0x4001, 0xe0},
> + {0x4003, 0x40},
> + {0x4008, 0x00},
> + {0x4009, 0x0b},
> + {0x400a, 0x00},
> + {0x400b, 0x84},
> + {0x400f, 0x80},
> + {0x4010, 0xf0},
> + {0x4011, 0xff},
> + {0x4012, 0x02},
> + {0x4013, 0x01},
> + {0x4014, 0x01},
> + {0x4015, 0x01},
> + {0x4042, 0x00},
> + {0x4043, 0x80},
> + {0x4044, 0x00},
> + {0x4045, 0x80},
> + {0x4046, 0x00},
> + {0x4047, 0x80},
> + {0x4048, 0x00},
> + {0x4049, 0x80},
> + {0x4041, 0x03},
> + {0x404c, 0x20},
> + {0x404d, 0x00},
> + {0x404e, 0x20},
> + {0x4203, 0x80},
> + {0x4307, 0x30},
> + {0x4317, 0x00},
> + {0x4502, 0x50},
> + {0x4503, 0x08},
> + {0x4601, 0x80},
> + {0x4800, 0x44},
> + {0x4816, 0x53},
> + {0x481b, 0x50},
> + {0x481f, 0x27},
> + {0x4823, 0x3c},
> + {0x482b, 0x00},
> + {0x4831, 0x66},
> + {0x4837, 0x16},
> + {0x483c, 0x0f},
> + {0x484b, 0x05},
> + {0x5000, 0x77},
> + {0x5001, 0x0a},
> + {0x5003, 0xc8},
> + {0x5004, 0x04},
> + {0x5006, 0x00},
> + {0x5007, 0x00},
> + {0x502e, 0x03},
> + {0x5030, 0x41},
> + {0x5780, 0x14},
> + {0x5781, 0x0f},
> + {0x5782, 0x44},
> + {0x5783, 0x02},
> + {0x5784, 0x01},
> + {0x5785, 0x01},
> + {0x5786, 0x00},
> + {0x5787, 0x04},
> + {0x5788, 0x02},
> + {0x5789, 0x0f},
> + {0x578a, 0xfd},
> + {0x578b, 0xf5},
> + {0x578c, 0xf5},
> + {0x578d, 0x03},
> + {0x578e, 0x08},
> + {0x578f, 0x0c},
> + {0x5790, 0x08},
> + {0x5791, 0x04},
> + {0x5792, 0x00},
> + {0x5793, 0x52},
> + {0x5794, 0xa3},
> + {0x5795, 0x02},
> + {0x5796, 0x20},
> + {0x5797, 0x20},
> + {0x5798, 0xd5},
> + {0x5799, 0xd5},
> + {0x579a, 0x00},
> + {0x579b, 0x50},
> + {0x579c, 0x00},
> + {0x579d, 0x2c},
> + {0x579e, 0x0c},
> + {0x579f, 0x40},
> + {0x57a0, 0x09},
> + {0x57a1, 0x40},
> + {0x59f8, 0x3d},
> + {0x5a08, 0x02},
> + {0x5b00, 0x02},
> + {0x5b01, 0x10},
> + {0x5b02, 0x03},
> + {0x5b03, 0xcf},
> + {0x5b05, 0x6c},
> + {0x5e00, 0x00},
> + {0x5e10, 0xfc}
> +};
> +
> static const struct ov8856_reg mode_1640x1232_regs[] = {
> {0x3000, 0x20},
> {0x3003, 0x08},
> @@ -506,6 +732,208 @@ static const struct ov8856_reg mode_1640x1232_regs[] = {
> {0x5e00, 0x00}
> };
>
> +static const struct ov8856_reg mode_1632x1224_regs[] = {
> + {0x0103, 0x01},
> + {0x0302, 0x3c},
> + {0x0303, 0x01},
> + {0x031e, 0x0c},
> + {0x3000, 0x00},
> + {0x300e, 0x00},
> + {0x3010, 0x00},
> + {0x3015, 0x84},
> + {0x3018, 0x72},
> + {0x3021, 0x23},
> + {0x3033, 0x24},
> + {0x3500, 0x00},
> + {0x3501, 0x4c},
> + {0x3502, 0xe0},
> + {0x3503, 0x08},
> + {0x3505, 0x83},
> + {0x3508, 0x01},
> + {0x3509, 0x80},
> + {0x350c, 0x00},
> + {0x350d, 0x80},
> + {0x350e, 0x04},
> + {0x350f, 0x00},
> + {0x3510, 0x00},
> + {0x3511, 0x02},
> + {0x3512, 0x00},
> + {0x3600, 0x72},
> + {0x3601, 0x40},
> + {0x3602, 0x30},
> + {0x3610, 0xc5},
> + {0x3611, 0x58},
> + {0x3612, 0x5c},
> + {0x3613, 0xca},
> + {0x3614, 0x60},
> + {0x3628, 0xff},
> + {0x3629, 0xff},
> + {0x362a, 0xff},
> + {0x3633, 0x10},
> + {0x3634, 0x10},
> + {0x3635, 0x10},
> + {0x3636, 0x10},
> + {0x3663, 0x08},
> + {0x3669, 0x34},
> + {0x366d, 0x00},
> + {0x366e, 0x08},
> + {0x3706, 0x86},
> + {0x370b, 0x7e},
> + {0x3714, 0x27},
> + {0x3730, 0x12},
> + {0x3733, 0x10},
> + {0x3764, 0x00},
> + {0x3765, 0x00},
> + {0x3769, 0x62},
> + {0x376a, 0x2a},
> + {0x376b, 0x30},
> + {0x3780, 0x00},
> + {0x3781, 0x24},
> + {0x3782, 0x00},
> + {0x3783, 0x23},
> + {0x3798, 0x2f},
> + {0x37a1, 0x60},
> + {0x37a8, 0x6a},
> + {0x37ab, 0x3f},
> + {0x37c2, 0x14},
> + {0x37c3, 0xf1},
> + {0x37c9, 0x80},
> + {0x37cb, 0x16},
> + {0x37cc, 0x16},
> + {0x37cd, 0x16},
> + {0x37ce, 0x16},
> + {0x3800, 0x00},
> + {0x3801, 0x00},
> + {0x3802, 0x00},
> + {0x3803, 0x0c},
> + {0x3804, 0x0c},
> + {0x3805, 0xdf},
> + {0x3806, 0x09},
> + {0x3807, 0xa3},
> + {0x3808, 0x06},
> + {0x3809, 0x60},
> + {0x380a, 0x04},
> + {0x380b, 0xc8},
> + {0x380c, 0x07},
> + {0x380d, 0x8c},
> + {0x380e, 0x09},
> + {0x380f, 0xb2},
> + {0x3810, 0x00},
> + {0x3811, 0x02},
> + {0x3812, 0x00},
> + {0x3813, 0x02},
> + {0x3814, 0x03},
> + {0x3815, 0x01},
> + {0x3816, 0x00},
> + {0x3817, 0x00},
> + {0x3818, 0x00},
> + {0x3819, 0x00},
> + {0x3820, 0x80},
> + {0x3821, 0x47},
> + {0x382a, 0x03},
> + {0x382b, 0x01},
> + {0x3830, 0x06},
> + {0x3836, 0x02},
> + {0x3862, 0x04},
> + {0x3863, 0x08},
> + {0x3cc0, 0x33},
> + {0x3d85, 0x17},
> + {0x3d8c, 0x73},
> + {0x3d8d, 0xde},
> + {0x4001, 0xe0},
> + {0x4003, 0x40},
> + {0x4008, 0x00},
> + {0x4009, 0x05},
> + {0x400a, 0x00},
> + {0x400b, 0x84},
> + {0x400f, 0x80},
> + {0x4010, 0xf0},
> + {0x4011, 0xff},
> + {0x4012, 0x02},
> + {0x4013, 0x01},
> + {0x4014, 0x01},
> + {0x4015, 0x01},
> + {0x4042, 0x00},
> + {0x4043, 0x80},
> + {0x4044, 0x00},
> + {0x4045, 0x80},
> + {0x4046, 0x00},
> + {0x4047, 0x80},
> + {0x4048, 0x00},
> + {0x4049, 0x80},
> + {0x4041, 0x03},
> + {0x404c, 0x20},
> + {0x404d, 0x00},
> + {0x404e, 0x20},
> + {0x4203, 0x80},
> + {0x4307, 0x30},
> + {0x4317, 0x00},
> + {0x4502, 0x50},
> + {0x4503, 0x08},
> + {0x4601, 0x80},
> + {0x4800, 0x44},
> + {0x4816, 0x53},
> + {0x481b, 0x50},
> + {0x481f, 0x27},
> + {0x4823, 0x3c},
> + {0x482b, 0x00},
> + {0x4831, 0x66},
> + {0x4837, 0x16},
> + {0x483c, 0x0f},
> + {0x484b, 0x05},
> + {0x5000, 0x77},
> + {0x5001, 0x0a},
> + {0x5003, 0xc8},
> + {0x5004, 0x04},
> + {0x5006, 0x00},
> + {0x5007, 0x00},
> + {0x502e, 0x03},
> + {0x5030, 0x41},
> + {0x5795, 0x00},
> + {0x5796, 0x10},
> + {0x5797, 0x10},
> + {0x5798, 0x73},
> + {0x5799, 0x73},
> + {0x579a, 0x00},
> + {0x579b, 0x28},
> + {0x579c, 0x00},
> + {0x579d, 0x16},
> + {0x579e, 0x06},
> + {0x579f, 0x20},
> + {0x57a0, 0x04},
> + {0x57a1, 0xa0},
> + {0x5780, 0x14},
> + {0x5781, 0x0f},
> + {0x5782, 0x44},
> + {0x5783, 0x02},
> + {0x5784, 0x01},
> + {0x5785, 0x01},
> + {0x5786, 0x00},
> + {0x5787, 0x04},
> + {0x5788, 0x02},
> + {0x5789, 0x0f},
> + {0x578a, 0xfd},
> + {0x578b, 0xf5},
> + {0x578c, 0xf5},
> + {0x578d, 0x03},
> + {0x578e, 0x08},
> + {0x578f, 0x0c},
> + {0x5790, 0x08},
> + {0x5791, 0x04},
> + {0x5792, 0x00},
> + {0x5793, 0x52},
> + {0x5794, 0xa3},
> + {0x59f8, 0x3d},
> + {0x5a08, 0x02},
> + {0x5b00, 0x02},
> + {0x5b01, 0x10},
> + {0x5b02, 0x03},
> + {0x5b03, 0xcf},
> + {0x5b05, 0x6c},
> + {0x5e00, 0x00},
> + {0x5e10, 0xfc}
> +};
> +
> static const char * const ov8856_test_pattern_menu[] = {
> "Disabled",
> "Standard Color Bar",
> @@ -548,6 +976,18 @@ static const struct ov8856_mode supported_modes[] = {
> .link_freq_index = OV8856_LINK_FREQ_720MBPS,
> },
> {
> + .width = 3264,
> + .height = 2448,
> + .hts = 1932,
> + .vts_def = 2482,
> + .vts_min = 2482,
> + .reg_list = {
> + .num_of_regs = ARRAY_SIZE(mode_3264x2448_regs),
> + .regs = mode_3264x2448_regs,
> + },
> + .link_freq_index = OV8856_LINK_FREQ_720MBPS,
> + },
> + {
> .width = 1640,
> .height = 1232,
> .hts = 3820,
> @@ -558,6 +998,18 @@ static const struct ov8856_mode supported_modes[] = {
> .regs = mode_1640x1232_regs,
> },
> .link_freq_index = OV8856_LINK_FREQ_360MBPS,
> + },
> + {
> + .width = 1632,
> + .height = 1224,
> + .hts = 1932,
> + .vts_def = 2482,
> + .vts_min = 2482,
> + .reg_list = {
> + .num_of_regs = ARRAY_SIZE(mode_1632x1224_regs),
> + .regs = mode_1632x1224_regs,
> + },
> + .link_freq_index = OV8856_LINK_FREQ_360MBPS,
> }
> };
>
> @@ -566,6 +1018,10 @@ struct ov8856 {
> struct media_pad pad;
> struct v4l2_ctrl_handler ctrl_handler;
>
> + struct clk *xvclk;
> + struct gpio_desc *reset_gpio;
> + struct regulator_bulk_data supplies[OV8856_NUM_SUPPLIES];
> +
> /* V4L2 Controls */
> struct v4l2_ctrl *link_freq;
> struct v4l2_ctrl *pixel_rate;
> @@ -576,6 +1032,9 @@ struct ov8856 {
> /* Current mode */
> const struct ov8856_mode *cur_mode;
>
> + /* module hardware version */
> + bool is_1B_module;
What other hardware versions are there, and what are the differences?
> +
> /* To serialize asynchronus callbacks */
> struct mutex mutex;
>
> @@ -696,6 +1155,24 @@ static int ov8856_test_pattern(struct ov8856 *ov8856, u32 pattern)
> OV8856_REG_VALUE_08BIT, pattern);
> }
>
> +static int ov8856_update_otp_reg(struct ov8856 *ov8856)
> +{
> + int ret;
> +
> + ret = ov8856_write_reg(ov8856, OV8856_REG_MODE_SELECT,
> + OV8856_REG_VALUE_08BIT, OV8856_MODE_STREAMING);
> + if (ret)
> + return ret;
> +
> + ret = ov8856_write_reg(ov8856, OV8856_OTP_REG_ONE,
> + OV8856_REG_VALUE_08BIT, OV8856_MODE_STANDBY);
> + if (ret)
> + return ret;
> +
> + return ov8856_write_reg(ov8856, OV8856_OTP_REG_TWO,
> + OV8856_REG_VALUE_08BIT, OV8856_MODE_STREAMING);
> +}
What does this do?
> +
> static int ov8856_set_ctrl(struct v4l2_ctrl *ctrl)
> {
> struct ov8856 *ov8856 = container_of(ctrl->handler,
> @@ -825,7 +1302,13 @@ static void ov8856_update_pad_format(const struct ov8856_mode *mode,
> {
> fmt->width = mode->width;
> fmt->height = mode->height;
> - fmt->code = MEDIA_BUS_FMT_SGRBG10_1X10;
> +
> + /* Bayer Order is determined by image resolution */
Ouch.
This rather looks like a side effect of vertical cropping. How about
either cropping one line above or below, to keep the same Bayer order?
The driver is based on register lists that heavily restricts the
possibilities of configuring the sensor. The alternative, should more
free-form configuration be enabled, would be to expose the cropping
capability to the user --- as well as binning.
> + if (fmt->width == 3264 || fmt->width == 1632)
> + fmt->code = MEDIA_BUS_FMT_SBGGR10_1X10;
> + else
> + fmt->code = MEDIA_BUS_FMT_SGRBG10_1X10;
> +
> fmt->field = V4L2_FIELD_NONE;
> }
>
> @@ -850,6 +1333,17 @@ static int ov8856_start_streaming(struct ov8856 *ov8856)
> return ret;
> }
>
> + /* update R3614 for 1B module */
> + if (ov8856->is_1B_module) {
> + ret = ov8856_write_reg(ov8856, OV8856_CLK_REG,
> + OV8856_REG_VALUE_08BIT,
> + OV8856_CLK_REG_1B_VAL);
> + if (ret) {
> + dev_err(&client->dev, "failed to set R3614");
> + return ret;
> + }
> + }
> +
> ret = __v4l2_ctrl_handler_setup(ov8856->sd.ctrl_handler);
> if (ret)
> return ret;
> @@ -882,6 +1376,8 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable)
> if (ov8856->streaming == enable)
> return 0;
>
> + dev_dbg(&client->dev, "hardware version: (%d)\n", ov8856->is_1B_module);
> +
> mutex_lock(&ov8856->mutex);
> if (enable) {
> ret = pm_runtime_get_sync(&client->dev);
> @@ -908,6 +1404,54 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable)
> return ret;
> }
>
> +/* Calculate the delay in us by clock rate and clock cycles */
> +static inline u32 ov8856_cal_delay(u32 cycles)
> +{
> + return DIV_ROUND_UP(cycles, OV8856_XVCLK_FREQ / 1000 / 1000);
The frequency is rounded down. As it is used to calculate a delay needed,
rounding up should be done for the frequency, too.
> +}
> +
> +static int __ov8856_power_on(struct ov8856 *ov8856)
> +{
> + int ret;
> + u32 delay_us;
> + struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> +
> + ret = clk_prepare_enable(ov8856->xvclk);
> + if (ret < 0) {
> + dev_err(&client->dev, "Failed to enable xvclk\n");
> + return ret;
> + }
> +
> + gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> +
> + ret = regulator_bulk_enable(OV8856_NUM_SUPPLIES, ov8856->supplies);
> + if (ret < 0) {
> + dev_err(&client->dev, "Failed to enable regulators\n");
> + goto disable_clk;
> + }
> +
> + gpiod_set_value_cansleep(ov8856->reset_gpio, 0);
> +
> + /* 8192 cycles prior to first SCCB transaction */
> + delay_us = ov8856_cal_delay(8192);
> + usleep_range(delay_us * 2, delay_us * 4);
Why multiply by 2?
Note that the driver still needs to work even if the resources aren't
visible to the software. That's the case e.g. on ACPI based systems.
> +
> + return 0;
> +
> +disable_clk:
> + clk_disable_unprepare(ov8856->xvclk);
> +
> + return ret;
> +}
> +
> +static void __ov8856_power_off(struct ov8856 *ov8856)
> +{
> + clk_disable_unprepare(ov8856->xvclk);
> + gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> +
> + regulator_bulk_disable(OV8856_NUM_SUPPLIES, ov8856->supplies);
> +}
> +
> static int __maybe_unused ov8856_suspend(struct device *dev)
> {
> struct i2c_client *client = to_i2c_client(dev);
> @@ -915,8 +1459,8 @@ static int __maybe_unused ov8856_suspend(struct device *dev)
> struct ov8856 *ov8856 = to_ov8856(sd);
>
> mutex_lock(&ov8856->mutex);
> - if (ov8856->streaming)
> - ov8856_stop_streaming(ov8856);
This seems like an unrelated change.
> +
> + __ov8856_power_off(ov8856);
>
> mutex_unlock(&ov8856->mutex);
>
> @@ -1089,6 +1633,20 @@ static int ov8856_identify_module(struct ov8856 *ov8856)
> return -ENXIO;
> }
>
> + /* set R3614 to distinguish harward versions */
> + ret = ov8856_update_otp_reg(ov8856);
> + if (ret) {
> + dev_err(&client->dev, "failed to set otp register");
> + return ret;
> + }
> +
> + ret = ov8856_read_reg(ov8856, OV8856_OTP_REG,
> + OV8856_REG_VALUE_08BIT, &val);
> + if (ret)
> + return ret;
> +
> + ov8856->is_1B_module = (val == OV8856_1B_MODULE) ? 1 : 0;
> +
> return 0;
> }
>
> @@ -1164,11 +1722,27 @@ static int ov8856_remove(struct i2c_client *client)
> media_entity_cleanup(&sd->entity);
> v4l2_ctrl_handler_free(sd->ctrl_handler);
> pm_runtime_disable(&client->dev);
> + if (!pm_runtime_status_suspended(&client->dev))
> + __ov8856_power_off(ov8856);
> + pm_runtime_set_suspended(&client->dev);
> mutex_destroy(&ov8856->mutex);
>
> return 0;
> }
>
> +static int ov8856_configure_regulators(struct ov8856 *ov8856)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd);
> + int i;
unsigned int
> +
> + for (i = 0; i < OV8856_NUM_SUPPLIES; i++)
> + ov8856->supplies[i].supply = ov8856_supply_names[i];
> +
> + return devm_regulator_bulk_get(&client->dev,
> + OV8856_NUM_SUPPLIES,
> + ov8856->supplies);
Remember to put the regulators, too.
> +}
> +
> static int ov8856_probe(struct i2c_client *client)
> {
> struct ov8856 *ov8856;
> @@ -1186,6 +1760,40 @@ static int ov8856_probe(struct i2c_client *client)
> return -ENOMEM;
>
> v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops);
> +
> + ov8856->xvclk = devm_clk_get(&client->dev, "xvclk");
> + if (IS_ERR(ov8856->xvclk)) {
> + dev_err(&client->dev, "Failed to get xvclk\n");
> + return -EINVAL;
> + }
> +
> + ret = clk_set_rate(ov8856->xvclk, OV8856_XVCLK_FREQ);
> + if (ret < 0) {
> + dev_err(&client->dev, "Failed to set xvclk rate (24MHz)\n");
> + return ret;
> + }
> + if (clk_get_rate(ov8856->xvclk) != OV8856_XVCLK_FREQ)
> + dev_warn(&client->dev,
> + "xvclk mismatched, modes are based on 24MHz\n");
> +
> + ov8856->reset_gpio = devm_gpiod_get(&client->dev,
> + "reset",
Fits on the previous line.
> + GPIOD_OUT_LOW);
> + if (IS_ERR(ov8856->reset_gpio)) {
> + dev_err(&client->dev, "Failed to get reset-gpios\n");
> + return -EINVAL;
> + }
> +
> + ret = ov8856_configure_regulators(ov8856);
> + if (ret) {
> + dev_err(&client->dev, "Failed to get power regulators\n");
> + return ret;
> + }
> +
> + ret = __ov8856_power_on(ov8856);
> + if (ret)
> + goto probe_error_v4l2_ctrl_handler_free;
> +
> ret = ov8856_identify_module(ov8856);
> if (ret) {
> dev_err(&client->dev, "failed to find sensor: %d", ret);
> @@ -1251,11 +1859,21 @@ static const struct acpi_device_id ov8856_acpi_ids[] = {
> MODULE_DEVICE_TABLE(acpi, ov8856_acpi_ids);
> #endif
>
> +#if IS_ENABLED(CONFIG_OF)
> +static const struct of_device_id ov8856_of_match[] = {
> + { .compatible = "ovti,ov8856" },
> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(of, ov8856_of_match);
> +#endif
> +
> static struct i2c_driver ov8856_i2c_driver = {
> .driver = {
> .name = "ov8856",
> .pm = &ov8856_pm_ops,
> .acpi_match_table = ACPI_PTR(ov8856_acpi_ids),
> + .of_match_table = of_match_ptr(ov8856_of_match),
> },
> .probe_new = ov8856_probe,
> .remove = ov8856_remove,
--
Regards,
Sakari Ailus
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH v2] arm64: Clarify when cpu_enable() is called
From: Mark Brown @ 2019-08-08 13:53 UTC (permalink / raw)
To: Suzuki K Poulose, will, catalin.marinas; +Cc: Mark Brown, linux-arm-kernel
Strengthen the wording in the documentation for cpu_enable() to make it
more obvious to readers not already familiar with the code when the core
will call this callback and that this is intentional.
Signed-off-by: Mark Brown <broonie@kernel.org>
---
v2: Much more verbose wording from Suzuki.
arch/arm64/include/asm/cpufeature.h | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index cf65a47ee6b4..2447d4afbf54 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -289,9 +289,17 @@ struct arm64_cpu_capabilities {
u16 type;
bool (*matches)(const struct arm64_cpu_capabilities *caps, int scope);
/*
- * Take the appropriate actions to enable this capability for this CPU.
- * For each successfully booted CPU, this method is called for each
- * globally detected capability.
+ * Take the appropriate actions to configure this capability
+ * for this CPU. If the capability is detected by the kernel
+ * this will be called on all the CPUs in the system,
+ * including the hotplugged CPUs, regardless of if the
+ * capability was *available* on that specific CPU. This is
+ * useful for some capabilities (e.g, working around CPU
+ * errata), where all the CPUs must take some action (e.g,
+ * changing system control/configuration). Thus, if an action
+ * is required only if the CPU has the capability then the
+ * routine must check it before taking any action.
+
*/
void (*cpu_enable)(const struct arm64_cpu_capabilities *cap);
union {
--
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] arm64: Clarify when cpu_enable() is called
From: Mark Brown @ 2019-08-08 13:46 UTC (permalink / raw)
To: Suzuki K Poulose; +Cc: catalin.marinas, will, linux-arm-kernel
In-Reply-To: <4975be8c-09ca-9acc-06ea-87e831f7fd79@arm.com>
[-- Attachment #1.1: Type: text/plain, Size: 2003 bytes --]
On Thu, Aug 08, 2019 at 02:21:42PM +0100, Suzuki K Poulose wrote:
> On 08/08/2019 13:19, Mark Brown wrote:
> > > > If this is called for any CPU in the system then it will be
> > > > called for all of them.
> > > > might cover it?
> > > * current CPU. If this capability is detected by the kernel, this will
> > > * called on all the CPUs in the system, including the hotplugged
> > > * CPUs.
> > > */
> > If this capability is detected by the kernel
> > this will called on all the CPUs in the system, including
> > the hotplugged CPUs, regardless of if the capability was
> > detected on that specific CPU.
> I think the only issue with this, as also with the original statement, is that
> you are overloading "detected" for the "specific CPU" case. In the first
> use, the "detect" is dependent on the SCOPE of the capability and in the
> latter one
That's not quite what I'm trying to get over here - what I'm trying to
get over is that the enable does not have the same scope as the
detection, I think it's fairly natural to assume that that is the case.
That is to say that the behaviour for the system scope detection case is
expected but for anything that's CPU local it's a surprise.
> is strictly "LOCAL" scope. If you replace the second "detected" with say, "not
> available" or even "not matched", it makes it less confusing.
> If the capability is detected by the kernel this will be called on all
> the CPUs in the system, including the hotplugged CPUs, regardless of if
> the capability was *available* on that specific CPU. This is useful for
> some capabilities (e.g, working around CPU errata), where all the CPUs
> must take some action (e.g, changing system control/configuration).
> Thus, if an action is required only if the CPU has the capability, then
> the routine must check it before taking any action.
That's a bit verbose but I think it's sufficiently unambiguous. I'm
still confused about how this differs from what I originally proposed :/
[-- 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: [RFC PATCH 09/11] devfreq: exynos-bus: Add interconnect functionality to exynos-bus
From: Artur Świgoń @ 2019-08-08 13:28 UTC (permalink / raw)
To: Georgi Djakov, devicetree, linux-arm-kernel, linux-samsung-soc,
linux-kernel, linux-pm, dri-devel
Cc: Bartłomiej Żołnierkiewicz, sw0312.kim, krzk,
inki.dae, cw00.choi, myungjoo.ham, m.szyprowski
In-Reply-To: <4155482f-8f8f-a659-63ba-25701540b2c5@linaro.org>
Hi,
On Wed, 2019-08-07 at 17:21 +0300, Georgi Djakov wrote:
> Hi Artur,
>
> On 8/1/19 10:59, Artur Świgoń wrote:
> > Hi Georgi,
> >
> > On Fri, 2019-07-26 at 11:05 +0300, Georgi Djakov wrote:
> > > Hi Artur,
> > >
> > > On 7/23/19 15:20, Artur Świgoń wrote:
> > > > This patch adds interconnect functionality to the exynos-bus devfreq
> > > > driver.
> > > >
> > > > The SoC topology is a graph (or, more specifically, a tree) and most of
> > > > its
> > > > edges are taken from the devfreq parent-child hierarchy (cf.
> > > > Documentation/devicetree/bindings/devfreq/exynos-bus.txt). The previous
> > > > patch adds missing edges to the DT (under the name 'parent'). Due to
> > > > unspecified relative probing order, -EPROBE_DEFER may be propagated to
> > > > guarantee that a child is probed before its parent.
> > > >
> > > > Each bus is now an interconnect provider and an interconnect node as
> > > > well
> > > > (cf. Documentation/interconnect/interconnect.rst), i.e. every bus
> > > > registers
> > > > itself as a node. Node IDs are not hardcoded but rather assigned at
> > > > runtime, in probing order (subject to the above-mentioned exception
> > > > regarding relative order). This approach allows for using this driver
> > > > with
> > > > various Exynos SoCs.
> > >
> > > I am not familiar with the Exynos bus topology, but it seems to me that
> > > it's not
> > > represented correctly. An interconnect provider with just a single node
> > > (port)
> > > is odd. I would expect that each provider consists of multiple master and
> > > slave
> > > nodes. This data would be used by a framework to understand what are the
> > > links
> > > and how the traffic flows between the IP blocks and through which buses.
> >
> > To summarize the exynos-bus topology[1] used by the devfreq driver: There
> > are
> > many data buses for data transfer in Samsung Exynos SoC. Every bus has its
> > own
> > clock. Buses often share power lines, in which case one of the buses on the
> > power line is referred to as 'parent' (or as 'devfreq' in the DT). In the
> > particular case of Exynos4412[1][2], the topology can be expressed as
> > follows:
> >
> > bus_dmc
> > -- bus_acp
> > -- bus_c2c
> >
> > bus_leftbus
> > -- bus_rightbus
> > -- bus_display
> > -- bus_fsys
> > -- bus_peri
> > -- bus_mfc
> >
> > Where bus_dmc and bus_leftbus probably could be referred to as masters, and
> > the
> > following indented nodes as slaves. Patch 08/11 of this RFC additionally
> > adds
> > the following to the DT:
> >
> > bus_dmc
> > -- bus_leftbus
> >
> > Which makes the topology a valid tree.
> >
> > The exynos-bus concept in devfreq[3] is designed in such a way that every
> > bus is
> > probed separately as a platform device, and is a largely independent entity.
> >
> > This RFC proposes an extension to the existing devfreq driver that basically
> > provides a simple QoS to ensure minimum clock frequency for selected buses
> > (possibly overriding devfreq governor calculations) using the interconnect
> > framework.
> >
> > The hierarchy is modelled in such a way that every bus is an interconnect
> > node.
> > On the other hand, what is considered an interconnect provider here is quite
> > arbitrary, but for the reasons mentioned in the above paragraph, this RFC
> > assumes that every bus is a provider of itself as a node. Using an
> > alternative
>
> IIUC, in case we want to transfer data between the display and the memory
> controller, the path would look like this:
>
> display --> bus_display --> bus_leftbus --> bus_dmc --> memory
>
> But the bus_display for example would have not one, but two nodes (ports),
> right? One representing the link to the display controller and another one
> representing the link to bus_leftbus? So i think that all the buses should
> have at least two nodes, to represent each end of the wire.
I do not think we really need that for our simple tree hierarchy. Of course, I
can split every tree node into two nodes/ports (e.g., 'in' for children and
'out' for parent), but neither 'display' nor 'memory' from your diagram above
are registered with the interconnect framework (only buses are). The devfreq
devices used in the driver are virtual anyway.
> > singleton provider approach was deemed more complicated since the 'dev'
> > field in
> > 'struct icc_provider' has to be set to something meaningful and we are tied
> > to
> > the 'samsung,exynos-bus' compatible string in the driver (and multiple
> > instances
> > of exynos-bus probed in indeterminate relative order).
> >
>
> Sure, the rest makes sense to me.
>
> Thanks,
> Georgi
Regards,
--
Artur Świgoń
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
* Re: [PATCH] dma: stm32-mdma: Fix a possible null-pointer dereference in stm32_mdma_irq_handler()
From: Vinod Koul @ 2019-08-08 13:24 UTC (permalink / raw)
To: Jia-Ju Bai
Cc: alexandre.torgue, linux-kernel, mcoquelin.stm32, dmaengine,
dan.j.williams, linux-stm32, linux-arm-kernel
In-Reply-To: <20190729020849.17971-1-baijiaju1990@gmail.com>
On 29-07-19, 10:08, Jia-Ju Bai wrote:
> In stm32_mdma_irq_handler(), chan is checked on line 1368.
> When chan is NULL, it is still used on line 1369:
> dev_err(chan2dev(chan), "MDMA channel not initialized\n");
>
> Thus, a possible null-pointer dereference may occur.
>
> To fix this bug, "dev_dbg(mdma2dev(dmadev), ...)" is used instead.
Applied after changing subsystem name in patch title to dmaengine: ...,
Also while fixing it helps to add Fixes tag, have added
Thanks
--
~Vinod
_______________________________________________
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] arm64: Clarify when cpu_enable() is called
From: Suzuki K Poulose @ 2019-08-08 13:21 UTC (permalink / raw)
To: broonie; +Cc: catalin.marinas, will, linux-arm-kernel
In-Reply-To: <20190808121956.GA3795@sirena.co.uk>
On 08/08/2019 13:19, Mark Brown wrote:
> On Thu, Aug 08, 2019 at 12:05:02PM +0100, Suzuki K Poulose wrote:
>> On 07/08/2019 17:51, Mark Brown wrote:
>>> On Wed, Aug 07, 2019 at 05:01:08PM +0100, Will Deacon wrote:
>>>> On Tue, Aug 06, 2019 at 06:00:43PM +0100, Mark Brown wrote:
>
>>> I guess you're thinking of the ARM64_CPUCAP_SYSTEM_FEATURE case where we
>>> match the feature on all CPUs so we could see the feature on some CPUs
>>> but not detect it as we're requiring a match on all?
>
>> We don't run the "match" check (i.e, detect) on all CPUs for SYSTEM scoped
>> features. Instead, we use sanitised feature set to detect the system features.
>
> Right, but the sanitised feature set involves merging the capabilities
> of all the CPUs.
>
>>> If this is called for any CPU in the system then it will be
>>> called for all of them.
>
>>> might cover it?
>
>> * current CPU. If this capability is detected by the kernel, this will
>> * called on all the CPUs in the system, including the hotplugged
>> * CPUs.
>> */
>
> How about adding ", regardless of if the capability was detected on that
> specific CPU" at the end? The above is *accurate* but it's still easy to
> insert an "on that CPU" in there when reading especially with the
> awkward phrasing. Or possibly just drop the first comma. The reason I
> said "If this is called" rather than "if this is detected" is to make it
> as clear as possible that the calls don't depend on detection without
> being overly verbose.
>
> If this capability is detected by the kernel
> this will called on all the CPUs in the system, including
> the hotplugged CPUs, regardless of if the capability was
> detected on that specific CPU.
I think the only issue with this, as also with the original statement, is that
you are overloading "detected" for the "specific CPU" case. In the first use,
the "detect" is dependent on the SCOPE of the capability and in the latter one
is strictly "LOCAL" scope. If you replace the second "detected" with say, "not
available" or even "not matched", it makes it less confusing.
How about:
If the capability is detected by the kernel this will be called on all
the CPUs in the system, including the hotplugged CPUs, regardless of if
the capability was *available* on that specific CPU. This is useful for
some capabilities (e.g, working around CPU errata), where all the CPUs
must take some action (e.g, changing system control/configuration).
Thus, if an action is required only if the CPU has the capability, then
the routine must check it before taking any action.
Suzuki
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RFC PATCH 09/11] devfreq: exynos-bus: Add interconnect functionality to exynos-bus
From: Artur Świgoń @ 2019-08-08 13:19 UTC (permalink / raw)
To: Leonard Crestez, Georgi Djakov
Cc: devicetree@vger.kernel.org, linux-samsung-soc@vger.kernel.org,
linux-pm@vger.kernel.org, sw0312.kim@samsung.com,
linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
inki.dae@samsung.com, cw00.choi@samsung.com,
myungjoo.ham@samsung.com, krzk@kernel.org,
linux-arm-kernel@lists.infradead.org, m.szyprowski@samsung.com
In-Reply-To: <VI1PR04MB5055BED59960B4F4147479AEEED50@VI1PR04MB5055.eurprd04.prod.outlook.com>
Hi,
Thank you for your comments.
On Tue, 2019-08-06 at 13:41 +0000, Leonard Crestez wrote:
> On 23.07.2019 15:21, Artur Świgoń wrote:
>
> > +static int exynos_bus_icc_aggregate(struct icc_node *node, u32 avg_bw,
> > + u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
> > +{
> > + *agg_peak = *agg_avg = peak_bw;
> > +
> > + return 0;
> > +}
>
> The only current provider aggregates "avg" with "sum" and "peak" with
> "max", any particular reason to do something different? This function
> doesn't even really do aggregation, if there is a second request for "0"
> then the first request is lost.
Yes, you're right. I adopted an oversimplified solution for the purpose of this
RFC (please bear in mind that currently only one icc_path is used, so there is
no aggregation anyway).
> I didn't find any docs but my interpretation of avg/peak is that "avg"
> is for constant traffic like a display or VPU pushing pixels and "peak"
> is for variable traffic like networking. I assume devices which make
> "peak" requests are aggregated with max because they're not expected to
> all max-out together.
That's correct (according to my understanding).
> In PATCH 11 you're making a bandwidth request based on resolution, that
> traffic is constant and guaranteed to happend while the display is on so
> it would make sense to request it as an "avg" and aggregate it with "sum".
>
> --
> Regards,
> Leonard
>
--
Artur Świgoń
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
* Re: [RFC PATCH 09/11] devfreq: exynos-bus: Add interconnect functionality to exynos-bus
From: Artur Świgoń @ 2019-08-08 13:18 UTC (permalink / raw)
To: Chanwoo Choi, devicetree, linux-arm-kernel, linux-samsung-soc,
linux-kernel, linux-pm, dri-devel
Cc: sw0312.kim, krzk, inki.dae, myungjoo.ham, georgi.djakov,
m.szyprowski
In-Reply-To: <5a82bf8a-d925-ba54-a26f-98b64bedc6e1@samsung.com>
Hi,
Thank you for your remarks. I will take them into account while preparing RFCv2.
On Mon, 2019-07-29 at 10:52 +0900, Chanwoo Choi wrote:
> Hi,
>
> On 19. 7. 23. 오후 9:20, Artur Świgoń wrote:
> > This patch adds interconnect functionality to the exynos-bus devfreq
> > driver.
> >
> > The SoC topology is a graph (or, more specifically, a tree) and most of its
> > edges are taken from the devfreq parent-child hierarchy (cf.
> > Documentation/devicetree/bindings/devfreq/exynos-bus.txt). The previous
> > patch adds missing edges to the DT (under the name 'parent'). Due to
> > unspecified relative probing order, -EPROBE_DEFER may be propagated to
> > guarantee that a child is probed before its parent.
> >
> > Each bus is now an interconnect provider and an interconnect node as well
> > (cf. Documentation/interconnect/interconnect.rst), i.e. every bus registers
> > itself as a node. Node IDs are not hardcoded but rather assigned at
> > runtime, in probing order (subject to the above-mentioned exception
> > regarding relative order). This approach allows for using this driver with
> > various Exynos SoCs.
> >
> > The devfreq target() callback provided by exynos-bus now selects either the
> > frequency calculated by the devfreq governor or the frequency requested via
> > the interconnect API for the given node, whichever is higher.
>
> Basically, I agree to support the QoS requirement between devices.
> But, I think that need to consider the multiple cases.
>
>
> 1. When changing the devfreq governor by user,
> For example of the connection between bus_dmc/leftbus/display on patch8,
> there are possible multiple cases with various devfreq governor
> which is changed on the runtime by user through sysfs interface.
>
> If users changes the devfreq governor as following:
> Before,
> - bus_dmc (simple_ondemand, available frequency 100/200/300/400 MHz)
> --> bus_leftbus(simple_ondemand, available frequency 100/200/300/400 MHz)
> ----> bus_display(passive)
>
> After changed governor of bus_dmc,
> if the min_freq by interconnect requirement is 400Mhz,
> - bus_dmc (powersave) : min_freq and max_freq and cur_freq is 100MHz
> --> bus_leftbus(simple_ondemand) : cur_freq is 400Mhz
> ----> bus_display(passive)
>
> The final frequency is 400MHz of bus_dmc
> even if the min_freq/max_freq/cur_freq is 100MHz.
> It cannot show the correct min_freq/max_freq through
> devfreq sysfs interface.
>
>
> 2. When disabling the some frequency by devfreq-thermal throttling,
> This patch checks the min_freq of interconnect requirement
> in the exynos_bus_target() and exynos_bus_passive_target().
> Also, it cannot show the correct min_freq/max_freq through
> devfreq sysfs interface.
>
> For example of bus_dmc bus,
> - The available frequencies are 100MHz, 200MHz, 300MHz, 400MHz
> - Disable 400MHz by devfreq-thermal throttling
> - min_freq is 100MHz
> - max_freq is 300MHz
> - min_freq of interconnect is 400MHz
>
> In result, the final frequency is 400MHz by exynos_bus_target()
> There are no problem for working. But, the user cannot know
> reason why cur_freq is 400MHz even if max_freq is 300MHz.
>
> Basically, update_devfreq() considers the all constraints
> of min_freq/max_freq to decide the proper target frequency.
>
>
> 3.
> I think that the exynos_bus_passive_target() is used for devfreq device
> using 'passive' governor. The frequency already depends on the parent device.
>
> If already the parent devfreq device like bus_leftbus consider
> the minimum frequency of QoS requirement like interconnect,
> it is not necessary. The next frequency of devfreq device
> with 'passive' governor, it will apply the QoS requirement
> without any additional code.
>
> >
> > Please note that it is not an error when CONFIG_INTERCONNECT is 'n', in
> > which case all interconnect API functions are no-op.
> >
> > Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
> > ---
> > drivers/devfreq/exynos-bus.c | 145 +++++++++++++++++++++++++++++++++++
> > 1 file changed, 145 insertions(+)
> >
> > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> > index 412511ca7703..12fb7c84ae50 100644
> > --- a/drivers/devfreq/exynos-bus.c
> > +++ b/drivers/devfreq/exynos-bus.c
> > @@ -14,6 +14,7 @@
> > #include <linux/devfreq-event.h>
> > #include <linux/device.h>
> > #include <linux/export.h>
> > +#include <linux/interconnect-provider.h>
> > #include <linux/module.h>
> > #include <linux/of.h>
> > #include <linux/pm_opp.h>
> > @@ -23,6 +24,8 @@
> > #define DEFAULT_SATURATION_RATIO 40
> > #define DEFAULT_VOLTAGE_TOLERANCE 2
> >
> > +#define icc_units_to_hz(x) ((x) * 1000UL / 8)
> > +
> > struct exynos_bus {
> > struct device *dev;
> >
> > @@ -31,12 +34,17 @@ struct exynos_bus {
> > unsigned int edev_count;
> > struct mutex lock;
> >
> > + unsigned long min_freq;
> > unsigned long curr_freq;
> >
> > struct regulator *regulator;
> > struct clk *clk;
> > unsigned int voltage_tolerance;
> > unsigned int ratio;
> > +
> > + /* One provider per bus, one node per provider */
> > + struct icc_provider provider;
> > + struct icc_node *node;
> > };
> >
> > /*
> > @@ -61,6 +69,13 @@ exynos_bus_ops_edev(enable_edev);
> > exynos_bus_ops_edev(disable_edev);
> > exynos_bus_ops_edev(set_event);
> >
> > +static int exynos_bus_next_id(void)
> > +{
> > + static int exynos_bus_node_id;
> > +
> > + return exynos_bus_node_id++;
> > +}
> > +
> > static int exynos_bus_get_event(struct exynos_bus *bus,
> > struct devfreq_event_data *edata)
> > {
> > @@ -98,6 +113,8 @@ static int exynos_bus_target(struct device *dev, unsigned
> > long *freq, u32 flags)
> > unsigned long old_freq, new_freq, new_volt, tol;
> > int ret = 0;
> >
> > + *freq = max(*freq, bus->min_freq);
> > +
> > /* Get new opp-bus instance according to new bus clock */
> > new_opp = devfreq_recommended_opp(dev, freq, flags);
> > if (IS_ERR(new_opp)) {
> > @@ -208,6 +225,8 @@ static int exynos_bus_passive_target(struct device *dev,
> > unsigned long *freq,
> > unsigned long old_freq, new_freq;
> > int ret = 0;
> >
> > + *freq = max(*freq, bus->min_freq);
> > +
> > /* Get new opp-bus instance according to new bus clock */
> > new_opp = devfreq_recommended_opp(dev, freq, flags);
> > if (IS_ERR(new_opp)) {
> > @@ -251,6 +270,35 @@ static void exynos_bus_passive_exit(struct device *dev)
> > clk_disable_unprepare(bus->clk);
> > }
> >
> > +static int exynos_bus_icc_set(struct icc_node *src, struct icc_node *dst)
> > +{
> > + struct exynos_bus *src_bus = src->data, *dst_bus = dst->data;
> > +
> > + src_bus->min_freq = icc_units_to_hz(src->peak_bw);
> > + dst_bus->min_freq = icc_units_to_hz(dst->peak_bw);
> > +
> > + return 0;
> > +}
> > +
> > +static int exynos_bus_icc_aggregate(struct icc_node *node, u32 avg_bw,
> > + u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
> > +{
> > + *agg_peak = *agg_avg = peak_bw;
> > +
> > + return 0;
> > +}
> > +
> > +static struct icc_node *exynos_bus_icc_xlate(struct of_phandle_args *spec,
> > + void *data)
> > +{
> > + struct exynos_bus *bus = data;
> > +
> > + if (spec->np != bus->dev->of_node)
> > + return ERR_PTR(-EINVAL);
> > +
> > + return bus->node;
> > +}
> > +
> > static int exynos_bus_parent_parse_of(struct device_node *np,
> > struct exynos_bus *bus)
> > {
> > @@ -469,6 +517,95 @@ static int exynos_bus_profile_init_passive(struct
> > exynos_bus *bus,
> > return ret;
> > }
> >
> > +static int exynos_bus_icc_connect(struct exynos_bus *bus)
> > +{
> > + struct device_node *np = bus->dev->of_node;
> > + struct devfreq *parent_devfreq;
> > + struct icc_node *parent_node = NULL;
> > + struct of_phandle_args args;
> > + int ret = 0;
> > +
> > + parent_devfreq = devfreq_get_devfreq_by_phandle(bus->dev, 0);
> > + if (!IS_ERR(parent_devfreq)) {
> > + struct exynos_bus *parent_bus;
> > +
> > + parent_bus = dev_get_drvdata(parent_devfreq->dev.parent);
> > + parent_node = parent_bus->node;
> > + } else {
> > + /* Look for parent in DT */
> > + int num = of_count_phandle_with_args(np, "parent",
> > + "#interconnect-cells");
> > + if (num != 1)
> > + goto out;
> > +
> > + ret = of_parse_phandle_with_args(np, "parent",
> > + "#interconnect-cells",
> > + 0, &args);
> > + if (ret < 0)
> > + goto out;
> > +
> > + of_node_put(args.np);
> > +
> > + parent_node = of_icc_get_from_provider(&args);
> > + if (IS_ERR(parent_node)) {
> > + /* May be -EPROBE_DEFER */
> > + ret = PTR_ERR(parent_node);
> > + goto out;
> > + }
> > + }
> > +
> > + ret = icc_link_create(bus->node, parent_node->id);
> > +
> > +out:
> > + return ret;
> > +}
> > +
> > +static int exynos_bus_icc_init(struct exynos_bus *bus)
> > +{
> > + struct device *dev = bus->dev;
> > + struct icc_provider *provider = &bus->provider;
> > + struct icc_node *node;
> > + int id, ret;
> > +
> > + /* Initialize the interconnect provider */
> > + provider->set = exynos_bus_icc_set;
> > + provider->aggregate = exynos_bus_icc_aggregate;
> > + provider->xlate = exynos_bus_icc_xlate;
> > + provider->dev = dev;
> > + provider->data = bus;
> > +
> > + ret = icc_provider_add(provider);
> > + if (ret < 0)
> > + goto out;
> > +
> > + id = exynos_bus_next_id();
> > + node = icc_node_create(id);
> > + if (IS_ERR(node)) {
> > + ret = PTR_ERR(node);
> > + goto err_node;
> > + }
> > +
> > + bus->node = node;
> > + node->name = dev->of_node->name;
> > + node->data = bus;
> > + icc_node_add(node, provider);
> > +
> > + ret = exynos_bus_icc_connect(bus);
> > + if (ret < 0)
> > + goto err_connect;
> > +
> > +out:
> > + return ret;
> > +
> > +err_connect:
> > + icc_node_del(node);
> > + icc_node_destroy(id);
> > +err_node:
> > + icc_provider_del(provider);
> > +
> > + return ret;
> > +}
> > +
> > static int exynos_bus_probe(struct platform_device *pdev)
> > {
> > struct device *dev = &pdev->dev;
> > @@ -517,6 +654,14 @@ static int exynos_bus_probe(struct platform_device
> > *pdev)
> > goto err;
> > }
> >
> > + /*
> > + * Initialize interconnect provider. A return value of -ENOTSUPP means
> > + * that CONFIG_INTERCONNECT is disabled.
> > + */
> > + ret = exynos_bus_icc_init(bus);
> > + if (ret < 0 && ret != -ENOTSUPP)
> > + goto err;
> > +
> > max_state = bus->devfreq->profile->max_state;
> > min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
> > max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
> >
>
>
--
Artur Świgoń
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
* Re: [PATCH v2 3/9] soc: samsung: Add Exynos Adaptive Supply Voltage driver
From: Krzysztof Kozlowski @ 2019-08-08 13:14 UTC (permalink / raw)
To: Robin Murphy
Cc: devicetree, linux-samsung-soc@vger.kernel.org,
Bartłomiej Żołnierkiewicz, pankaj.dubey, linux-pm,
linux-kernel@vger.kernel.org, robh+dt, kgene, Sylwester Nawrocki,
vireshk, linux-arm-kernel, Marek Szyprowski
In-Reply-To: <669c6b25-eb7e-ed3a-72a2-ee155a568363@arm.com>
On Thu, 8 Aug 2019 at 14:48, Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 08/08/2019 13:31, Krzysztof Kozlowski wrote:
> > On Thu, 8 Aug 2019 at 14:07, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote:
> >>>> +static unsigned int exynos5422_asv_parse_table(struct exynos_asv *asv,
> >>>> + unsigned int pkg_id)
> >>>> +{
> >>>> + return (pkg_id >> EXYNOS5422_TABLE_OFFSET) & EXYNOS5422_TABLE_MASK;
> >>>> +}
> >>>> +
> >>>> +static bool exynos5422_asv_parse_bin2(struct exynos_asv *asv,
> >>>> + unsigned int pkg_id)
> >>>> +{
> >>>> + return (pkg_id >> EXYNOS5422_BIN2_OFFSET) & EXYNOS5422_BIN2_MASK;
> >>>
> >>> return !!() for converting to boolean.
> >>
> >> I'm not convinced it is needed, the return type of the function is bool
> >> and value of the expression will be implicitly converted to that type.
> >> Is there any compiler warning related to that?
> >
> > Yeah, but bool is int so there will be no implicit conversion... I
> > guess it is a convention. In theory !! is the proper conversion to
> > bool but if bool==int then it's essentially conversion to 1. I am not
> > sure what's the benefit, maybe for some wrong code which would do
> > comparisons on result like if (exynos5422_asv_parse_bin2() == TRUE)...
>
> Not so - since we use "-std=gnu89", we have C99-like _Bool, which our
> bool is a typedef of. Conversions, either implicit or explicit, are
> well-defined:
>
> "6.3.1.2 Boolean type
>
> When any scalar value is converted to _Bool, the result is 0 if the
> value compares equal
> to 0; otherwise, the result is 1."
>
> This is even called out in Documentation/process/coding-style.rst:
>
> "When using bool types the !! construction is not needed, which
> eliminates a class of bugs."
Good point, thanks!
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
* Re: usb zero copy dma handling
From: Greg KH @ 2019-08-08 13:05 UTC (permalink / raw)
To: Robin Murphy; +Cc: yvahkhfo.1df7f8c2, linux-usb, security, linux-arm-kernel
In-Reply-To: <20190808100726.GB23844@kroah.com>
On Thu, Aug 08, 2019 at 12:07:26PM +0200, Greg KH wrote:
> On Thu, Aug 08, 2019 at 10:46:24AM +0100, Robin Murphy wrote:
> > On 2019-08-08 9:58 am, Greg KH wrote:
> > > On Thu, Aug 08, 2019 at 10:46:36AM +0200, yvahkhfo.1df7f8c2@hashmail.org wrote:
> > > > Hello linux-usb and linux-arm.
> > > >
> > > > Ccing security@ because "the kernel dma code is mapping randomish
> > > > kernel/user mem to a user process" seems to have security implications
> > > > even though i didnt research that aspect past "its a 100% reliable way
> > > > to crash a raspi from userspace".
> > > >
> > > > tried submitting this through linux-arm-kernel ~2 weeks ago but
> > > > the only "response" i got was phishing-spam.
> > > > tried to follow up through raspi-internals chat, they suggested
> > > > i try linux-usb instead, but otoh the original reporter was
> > > > deflected from -usb to "try some other mls, they might care".
> > > > https://www.spinics.net/lists/linux-usb/msg173277.html
> > > >
> > > > if i am not following some arcane ritual or indenting convention required
> > > > by regular users of these lists i apologize in advance, but i am not a
> > > > kernel developer, i am just here as a user with a bug and a patch.
> > > > (and the vger FAQ link 404s...)
> > >
> > > The "arcane ritual" should be really well documented by now, it's in
> > > Documentation/SubmittingPatches in your kernel tree, and you can read it
> > > online at:
> > > https://www.kernel.org/doc/html/latest/process/submitting-patches.html
> > >
> > >
> > > > i rediffed against HEAD even though the two weeks old patch still applied
> > > > cleanly with +2 offset.
> > > >
> > > > # stepping off soap box # actual technical content starts here #
> > > >
> > > > this is a followup to that thread from 2018-11:
> > > > https://www.spinics.net/lists/arm-kernel/msg685598.html
> > > >
> > > > the issue was discussed in more detail than i can claim
> > > > to fully understand back then, but no fix ever merged.
> > > > but i would really like to use rtl_433 on a raspi without
> > > > having to build a custom-patched kernel first.
> > > >
> > > > the attached patch is my stripdown/cleanup of a devel-diff
> > > > provided to me by the original reporter Steve Markgraf.
> > > > credits to him for the good parts, blame to me for the bad parts.
> > > >
> > > > this does not cover the additional case of "PIO-based usb controllers"
> > > > mainly because i dont understand what that means (or how to handle it)
> > > > and if its broken right now (as the thread indicates) it might
> > > > as well stay broken until someone who understands cares enough.
> > > >
> > > > could you please get this on track for merging?
> > >
> > >
> > > >
> > > > regards,
> > > > x23
> > > >
> > > >
> > > >
> > >
> > > > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> > > > index b265ab5405f9..69594c2169ea 100644
> > > > --- a/drivers/usb/core/devio.c
> > > > +++ b/drivers/usb/core/devio.c
> > > > @@ -238,9 +238,14 @@ static int usbdev_mmap(struct file *file, struct vm_area_struct *vma)
> > > > usbm->vma_use_count = 1;
> > > > INIT_LIST_HEAD(&usbm->memlist);
> > > > +#ifdef CONFIG_X86
> > > > if (remap_pfn_range(vma, vma->vm_start,
> > > > virt_to_phys(usbm->mem) >> PAGE_SHIFT,
> > > > size, vma->vm_page_prot) < 0) {
> > > > +#else /* !CONFIG_X86 */
> > > > + if (dma_mmap_coherent(ps->dev->bus->sysdev,
> > > > + vma, mem, dma_handle, size) < 0) {
> > > > +#endif /* !CONFIG_X86 */
> > > > dec_usb_memory_use_count(usbm, &usbm->vma_use_count);
> > > > return -EAGAIN;
> > > > }
> > >
> > > First off, we need this in a format we could apply it in (hint, read the
> > > above links).
> > >
> > > But the main issue here is what exactly is this "fixing"? What is wrong
> > > with the existing code that non-x86 systems have such a problem with?
> > > Shouldn't all of these dma issues be handled by the platform with the
> > > remap_pfn_range() call itself?
> >
> > If usbm->mem is (or ever can be) a CPU address returned by
> > dma_alloc_coherent(), then doing virt_to_phys() on it is bogus and may yield
> > a nonsense 'PFN' to begin with. However, it it can can ever come from a
> > regular page allocation/kmalloc/vmalloc then unconditionally passing it to
> > dma_mmap_coherent wouldn't be right either.
>
> usbm->mem comes from a call to usb_alloc_coherent() which calls
> hcd_buffer_alloc() which tries to allocate memory in the best possible
> way for that specific host controller. If the host controller has a
> pool of memory, it uses that, if the host controller has PIO it uses
> kmalloc(), if there are some "pools" of host controller memory it uses
> dma_pool_alloc() and as a total last resort, calls dma_alloc_coherent().
>
> So yes, this could happen.
>
> So how to fix this properly? What host controller driver is being used
> here that ends up defaulting to dma_alloc_coherent()? Shouldn't that be
> fixed up no matter what?
>
> And then, if what you say is correct then a real fix for devio.c could
> be made, but that is NOT going to just depend on the arch the system is
> running on, as all of this depends on the host controller being accessed
> at that moment for that device.
Also see this thread:
https://lore.kernel.org/linux-usb/20190801220134.3295-1-gavinli@thegavinli.com/
where this just came up and how the proposed patch here would cause
warnings to occur in the kernel log of users for no good reason. That
issue is supposed to be fixed "soon"...
thanks,
greg k-h
_______________________________________________
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 1/2] mmc: sdhci-of-at91: add quirk for broken HS200
From: Adrian Hunter @ 2019-08-08 13:00 UTC (permalink / raw)
To: Eugen.Hristev, Nicolas.Ferre, Ludovic.Desroches,
alexandre.belloni, ulf.hansson, linux-arm-kernel, devicetree,
linux-kernel, linux-mmc
In-Reply-To: <1565252928-28994-1-git-send-email-eugen.hristev@microchip.com>
On 8/08/19 11:35 AM, Eugen.Hristev@microchip.com wrote:
> From: Eugen Hristev <eugen.hristev@microchip.com>
>
> HS200 is not implemented in the driver, but the controller claims it
> through caps.
> Remove it via quirk.
> Without this quirk, the mmc core will try to enable hs200, which will fail,
> and the eMMC initialization will fail.
>
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
> drivers/mmc/host/sdhci-of-at91.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
> index 57fe3b2..3a8c6d8 100644
> --- a/drivers/mmc/host/sdhci-of-at91.c
> +++ b/drivers/mmc/host/sdhci-of-at91.c
> @@ -370,6 +370,9 @@ static int sdhci_at91_probe(struct platform_device *pdev)
> pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
> pm_runtime_use_autosuspend(&pdev->dev);
>
> + /* HS200 is broken at this moment */
> + host->quirks2 = SDHCI_QUIRK2_BROKEN_HS200;
> +
> ret = sdhci_add_host(host);
> if (ret)
> goto pm_runtime_disable;
>
_______________________________________________
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 2/2] ARM: dts: at91: sama5d27_som1_ek: add mmc capabilities for SDMMC0
From: Adrian Hunter @ 2019-08-08 12:57 UTC (permalink / raw)
To: Eugen Hristev - M18282, Nicolas Ferre - M43238,
alexandre.belloni@bootlin.com, ulf.hansson@linaro.org,
linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-mmc@vger.kernel.org
In-Reply-To: <20190808124217.wrmcxohw5i6ju2qe@M43218.corp.atmel.com>
On 8/08/19 3:42 PM, Ludovic Desroches wrote:
> On Thu, Aug 08, 2019 at 10:35:43AM +0200, Eugen Hristev - M18282 wrote:
>> From: Eugen Hristev <eugen.hristev@microchip.com>
>>
>> Add mmc capabilities for SDMMC0 for this board.
>> With this enabled, eMMC connected card is detected as:
>>
>> mmc0: new DDR MMC card at address 0001
>>
>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com>
>
> I am interested to have the some insights about the use of sd-uhs-*
> properties.
>
> Our IP can't deal with 1V8 by itself. It has a 1V8SEL signal which can
> be used as the logic control input of a mux. So even if the IP claims
> to support UHS modes, it depends on the board.
>
> Are the sd-uhs-* properties a way to deal with this? I tend to think no
> as sdhci_setup_host() will set the caps depending on the content of the
> capabilities register. Do we have to use the SDHCI_QUIRK_MISSING_CAPS
> quirk or sdhci-caps/sdhci-caps-mask?
There is "no-1-8-v" which it looks like sdhci-of-at91.c already supports:
sdhci_at91_probe() -> sdhci_get_of_property() -> sdhci_get_property()
if (device_property_present(dev, "no-1-8-v"))
host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
>
> Regards
>
> Ludovic
>
>> ---
>> arch/arm/boot/dts/at91-sama5d27_som1_ek.dts | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/arm/boot/dts/at91-sama5d27_som1_ek.dts b/arch/arm/boot/dts/at91-sama5d27_som1_ek.dts
>> index 149e539..194b3a3 100644
>> --- a/arch/arm/boot/dts/at91-sama5d27_som1_ek.dts
>> +++ b/arch/arm/boot/dts/at91-sama5d27_som1_ek.dts
>> @@ -54,6 +54,7 @@
>>
>> sdmmc0: sdio-host@a0000000 {
>> bus-width = <8>;
>> + mmc-ddr-3_3v;
>> pinctrl-names = "default";
>> pinctrl-0 = <&pinctrl_sdmmc0_default>;
>> status = "okay";
>> --
>> 2.7.4
>>
>
_______________________________________________
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 6/6] PSCI: cpuidle: Refactor CPU suspend power_state parameter handling
From: Sudeep Holla @ 2019-08-08 12:55 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Mark Rutland, Ulf Hansson, linux-pm, Catalin Marinas,
Daniel Lezcano, Rafael J. Wysocki, LKML, Will Deacon, LAKML
In-Reply-To: <20190722153745.32446-7-lorenzo.pieralisi@arm.com>
On Mon, Jul 22, 2019 at 04:37:45PM +0100, Lorenzo Pieralisi wrote:
> Current PSCI code handles idle state entry through the
> psci_cpu_suspend_enter() API, that takes an idle state index as a
> parameter and convert the index into a previously initialized
> power_state parameter before calling the PSCI.CPU_SUSPEND() with it.
>
> This is unwieldly, since it forces the PSCI firmware layer to keep track
> of power_state parameter for every idle state so that the
> index->power_state conversion can be made in the PSCI firmware layer
> instead of the CPUidle driver implementations.
>
> Move the power_state handling out of drivers/firmware/psci
> into the respective ACPI/DT PSCI CPUidle backends and convert
> the psci_cpu_suspend_enter() API to get the power_state
> parameter as input, which makes it closer to its firmware
> interface PSCI.CPU_SUSPEND() API.
>
> A notable side effect is that the PSCI ACPI/DT CPUidle backends
> now can directly handle (and if needed update) power_state
> parameters before handing them over to the PSCI firmware
> interface to trigger PSCI.CPU_SUSPEND() calls.
>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> +static __init int psci_cpu_init_idle(unsigned int cpu)
> +{
> + struct device_node *cpu_node;
> + int ret;
> +
> + /*
> + * If the PSCI cpu_suspend function hook has not been initialized
> + * idle states must not be enabled, so bail out
> + */
> + if (!psci_ops.cpu_suspend)
> + return -EOPNOTSUPP;
> +
> + cpu_node = of_get_cpu_node(cpu, NULL);
[nit] You could use of_cpu_device_node_get in linux/of_device.h as
it may avoid parsing if used later during the boot(i.e. after
cpu->of_node is populated). I think there's another instance in
psci_idle_init_cpu
--
Regards,
Sudeep
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH v2 2/2] ARM: zynq: Use memcpy_toio instead of memcpy on smp bring-up
From: Luis Araneda @ 2019-08-08 12:52 UTC (permalink / raw)
To: linux, michal.simek; +Cc: stable, linux-kernel, linux-arm-kernel, Luis Araneda
In-Reply-To: <20190808125243.31046-1-luaraneda@gmail.com>
This fixes a kernel panic on memcpy when
FORTIFY_SOURCE is enabled.
The initial smp implementation on commit aa7eb2bb4e4a
("arm: zynq: Add smp support")
used memcpy, which worked fine until commit ee333554fed5
("ARM: 8749/1: Kconfig: Add ARCH_HAS_FORTIFY_SOURCE")
enabled overflow checks at runtime, producing a read
overflow panic.
The computed size of memcpy args are:
- p_size (dst): 4294967295 = (size_t) -1
- q_size (src): 1
- size (len): 8
Additionally, the memory is marked as __iomem, so one of
the memcpy_* functions should be used for read/write.
Fixes: aa7eb2bb4e4a ("arm: zynq: Add smp support")
Signed-off-by: Luis Araneda <luaraneda@gmail.com>
Cc: stable@vger.kernel.org
---
Changes:
v1 -> v2:
- Reword commit message to include related commits
- Add Fixes tag
- Add Cc to stable
---
arch/arm/mach-zynq/platsmp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/mach-zynq/platsmp.c b/arch/arm/mach-zynq/platsmp.c
index 38728badabd4..a10085be9073 100644
--- a/arch/arm/mach-zynq/platsmp.c
+++ b/arch/arm/mach-zynq/platsmp.c
@@ -57,7 +57,7 @@ int zynq_cpun_start(u32 address, int cpu)
* 0x4: Jump by mov instruction
* 0x8: Jumping address
*/
- memcpy((__force void *)zero, &zynq_secondary_trampoline,
+ memcpy_toio(zero, &zynq_secondary_trampoline,
trampoline_size);
writel(address, zero + trampoline_size);
--
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 v2 1/2] ARM: zynq: support smp in thumb mode
From: Luis Araneda @ 2019-08-08 12:52 UTC (permalink / raw)
To: linux, michal.simek; +Cc: linux-kernel, linux-arm-kernel, Luis Araneda
In-Reply-To: <20190808125243.31046-1-luaraneda@gmail.com>
Add .arm directive to headsmp.S to ensure that the
CPU starts in 32-bit ARM mode and the correct code
size is copied on smp bring-up.
This is related to the fix applied to SoCFPGA by
commit 5616f36713ea
("ARM: SoCFPGA: Fix secondary CPU startup in thumb2 kernel")
Additionally, start secondary CPUs on secondary_startup_arm
to automatically switch from ARM to thumb on a thumb kernel
Signed-off-by: Luis Araneda <luaraneda@gmail.com>
Suggested-by: Michal Simek <michal.simek@xilinx.com>
---
Changes:
v1 -> v2:
- Reword commit message to include related commits
---
arch/arm/mach-zynq/headsmp.S | 2 ++
arch/arm/mach-zynq/platsmp.c | 2 +-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-zynq/headsmp.S b/arch/arm/mach-zynq/headsmp.S
index ab85003cf9ad..3449e0d1f990 100644
--- a/arch/arm/mach-zynq/headsmp.S
+++ b/arch/arm/mach-zynq/headsmp.S
@@ -7,6 +7,8 @@
#include <linux/init.h>
#include <asm/assembler.h>
+ .arm
+
ENTRY(zynq_secondary_trampoline)
ARM_BE8(setend be) @ ensure we are in BE8 mode
ldr r0, zynq_secondary_trampoline_jump
diff --git a/arch/arm/mach-zynq/platsmp.c b/arch/arm/mach-zynq/platsmp.c
index a7cfe07156f4..38728badabd4 100644
--- a/arch/arm/mach-zynq/platsmp.c
+++ b/arch/arm/mach-zynq/platsmp.c
@@ -81,7 +81,7 @@ EXPORT_SYMBOL(zynq_cpun_start);
static int zynq_boot_secondary(unsigned int cpu, struct task_struct *idle)
{
- return zynq_cpun_start(__pa_symbol(secondary_startup), cpu);
+ return zynq_cpun_start(__pa_symbol(secondary_startup_arm), cpu);
}
/*
--
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 v2 0/2] ARM: zynq: smp improvements
From: Luis Araneda @ 2019-08-08 12:52 UTC (permalink / raw)
To: linux, michal.simek; +Cc: linux-kernel, linux-arm-kernel, Luis Araneda
This series adds support for kernel compiled in Thumb mode
and fixes a kernel panic on smp bring-up when FORTIFY_SOURCE
is enabled.
The series started with the second patch as an RFC, and
the first patch were suggested on the review to complement
the fix.
The changes were run-tested on a Digilent Zybo Z7 board
---
Changes:
v1 -> v2:
- Reword commit messages to include related commits
- Add Fixes tag to relevant commits
- Add Cc to stable to relevant commits
Luis Araneda (2):
ARM: zynq: support smp in thumb mode
ARM: zynq: Use memcpy_toio instead of memcpy on smp bring-up
arch/arm/mach-zynq/headsmp.S | 2 ++
arch/arm/mach-zynq/platsmp.c | 4 ++--
2 files changed, 4 insertions(+), 2 deletions(-)
--
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: [PATCH 0/6] hwspinlock: allow sharing of hwspinlocks
From: Fabien DESSENNE @ 2019-08-08 12:52 UTC (permalink / raw)
To: Suman Anna, Bjorn Andersson
Cc: Ohad Ben-Cohen, Mark Rutland, Alexandre TORGUE, Jonathan Corbet,
linux-doc@vger.kernel.org, linux-remoteproc@vger.kernel.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
Rob Herring, Maxime Coquelin,
linux-stm32@st-md-mailman.stormreply.com,
linux-arm-kernel@lists.infradead.org, Benjamin GAIGNARD
In-Reply-To: <f0893b3f-0124-007a-3ca2-831f60ad9a80@ti.com>
On 07/08/2019 6:19 PM, Suman Anna wrote:
> Hi Fabien,
>
> On 8/7/19 3:39 AM, Fabien DESSENNE wrote:
>> Hi
>>
>> On 06/08/2019 11:30 PM, Suman Anna wrote:
>>> On 8/6/19 1:21 PM, Bjorn Andersson wrote:
>>>> On Tue 06 Aug 10:38 PDT 2019, Suman Anna wrote:
>>>>
>>>>> Hi Fabien,
>>>>>
>>>>> On 8/5/19 12:46 PM, Bjorn Andersson wrote:
>>>>>> On Mon 05 Aug 01:48 PDT 2019, Fabien DESSENNE wrote:
>>>>>>
>>>>>>> On 01/08/2019 9:14 PM, Bjorn Andersson wrote:
>>>>>>>> On Wed 13 Mar 08:50 PDT 2019, Fabien Dessenne wrote:
>>>> [..]
>>>>>>> B/ This would introduce some inconsistency between the two 'request' API
>>>>>>> which are hwspin_lock_request() and hwspin_lock_request_specific().
>>>>>>> hwspin_lock_request() looks for an unused lock, so requests for an exclusive
>>>>>>> usage. On the other side, request_specific() would request shared locks.
>>>>>>> Worst the following sequence can transform an exclusive usage into a shared
>>>>>>>
>>>>>> There is already an inconsistency in between these; as with above any
>>>>>> system that uses both request() and request_specific() will be suffering
>>>>>> from intermittent failures due to probe ordering.
>>>>>>
>>>>>>> one:
>>>>>>> -hwspin_lock_request() -> returns Id#0 (exclusive)
>>>>>>> -hwspin_lock_request() -> returns Id#1 (exclusive)
>>>>>>> -hwspin_lock_request_specific(0) -> returns Id#0 and makes Id#0 shared
>>>>>>> Honestly I am not sure that this is a real issue, but it's better to have it
>>>>>>> in mind before we take ay decision
>>>>> Wouldn't it be actually simpler to just introduce a new specific API
>>>>> variant for this, similar to the reset core for example (it uses a
>>>>> separate exclusive API), without having to modify the bindings at all.
>>>>> It is just a case of your driver using the right API, and the core can
>>>>> be modified to use the additional tag semantics based on the API. It
>>>>> should avoid any confusion with say using a different second cell value
>>>>> for the same lock in two different nodes.
>>>>>
>>>> But this implies that there is an actual need to hold these locks
>>>> exclusively. Given that they are (except for the raw case) all wrapped
>>>> by Linux locking primitives there shouldn't be a problem sharing a lock
>>>> (except possibly for the raw case).
>>> Yes agreed, the HWLOCK_RAW and HWLOCK_IN_ATOMIC cases are unprotected. I
>>> am still trying to understand better the usecase to see if the same lock
>>> is being multiplexed for different protection contexts, or if all of
>>> them are protecting the same context.
>>
>> Here are two different examples that explain the need for changes.
>> In both cases the Linux clients are talking to a single entity on the
>> remote-side.
>>
>> Example 1:
>> exti: interrupt-controller@5000d000 {
>> compatible = "st,stm32mp1-exti", "syscon";
>> interrupt-controller;
>> #interrupt-cells = <2>;
>> reg = <0x5000d000 0x400>;
>> hwlocks = <&hsem 1>;
>> };
>> The two drivers (stm32mp1-exti and syscon) refer to the same hwlock.
>> With the current hwspinlock implementation, only the first driver succeeds
>> in requesting (hwspin_lock_request_specific) the hwlock. The second request
>> fails.
>> Here, we really need to share the hwlock between the two drivers.
>> Note: hardware spinlock support for regmap was 'recently' introduced in 4.15
>> see https://lore.kernel.org/patchwork/patch/845941/
>>
>>
>>
>> Example 2:
>> Here it is more a question of optimization : we want to save the number of
>> hwlocks used to protect resources, using an unique hwlock to protect all
>> pinctrl resources:
>> pinctrl: pin-controller@50002000 {
>> compatible = "st,stm32mp157-pinctrl";
>> ranges = <0 0x50002000 0xa400>;
>> hwlocks = <&hsem 0 1>;
>>
>> pinctrl_z: pin-controller-z@54004000 {
>> compatible = "st,stm32mp157-z-pinctrl";
>> ranges = <0 0x54004000 0x400>;
>> pins-are-numbered;
>> hwlocks = <&hsem 0 1>;
> Thanks for the examples.
>
>>>> I agree that we shouldn't specify this property in DT - if anything it
>>>> should be a variant of the API.
>>
>> If we decide to add a 'shared' API, then, what about the generic regmap
>> driver?
>>
>> In the context of above example1, this would require to update the
>> regmap driver.
>>
>> But would this be acceptable for any driver using syscon/regmap?
>>
>>
>> I think it is better to keep the existing API (modifying it so it always
>> allows
>>
>> hwlocks sharing, so no need for bindings update) than adding another API.
> For your usecases, you would definitely need the syscon/regmap behavior
> to be shared right. Whether we introduce a 'shared' API or an
> 'exclusive' API and change the current API behavior to shared, it is
> definitely a case-by-case usage scenario for the existing drivers and
> usage right. The main contention point is what to do with the
> unprotected usecases like Bjorn originally pointed out.
OK, I see : the hwspinlock framework does not offer any lock protection
with the RAW/IN_ATOMIC modes.
This is an issue if several different 'local' drivers try to get a
shared lock in the same time.
And this is a personal problem since I need to use shared locks in
...atomic mode.
I have tried to see how it is possible to put a constraint on the
callers, just like this is documented for the RAW mode which is:
"Caution: If the mode is HWLOCK_RAW, that means user must protect
the routine
of getting hardware lock with mutex or spinlock.."
I do not think that it is acceptable to ask several drivers to share a
common mutex/spinlock for shared locks.
But I think about another option: the driver implementing the trylock
ops may offer such protection. This is the case if the driver returns
"busy" if the lock is already taken, not only by the remote processor,
but also by the local host.
So what do you think about adding such a documentation note :
"Caution : the HWLOCK_RAW / HWLOCK_IN_ATOMIC modes shall not be used
with shared locks unless the hwspinlock driver supports local lock
protection"
Optionally, we may add a "local_lock_protection" flag in the
hwspinlock_device struct, set by the driver before it calls
hwspin_lock_register().
This flag can then be checked by hwspinlock core to allow/deny use of
shared locks in the raw/atomic modes.
Let me know what you think about it.
BR
Fabien
>
> regards
> Suman
>
>>
>>
>>>>> If you are sharing a hwlock on the Linux side, surely your driver should
>>>>> be aware that it is a shared lock. The tag can be set during the first
>>>>> request API, and you look through both tags when giving out a handle.
>>>>>
>>>> Why would the driver need to know about it?
>>> Just the semantics if we were to support single user vs multiple users
>>> on Linux-side to even get a handle. Your point is that this may be moot
>>> since we have protection anyway other than the raw cases. But we need to
>>> be able to have the same API work across all cases.
>>>
>>> So far, it had mostly been that there would be one user on Linux
>>> competing with other equivalent peer entities on different processors.
>>> It is not common to have multiple users since these protection schemes
>>> are usually needed only at the lowest levels of a stack, so the
>>> exclusive handle stuff had been sufficient.
>>>
>>>>> Obviously, the hwspin_lock_request() API usage semantics always had the
>>>>> implied additional need for communicating the lock id to the other peer
>>>>> entity, so a realistic usage is most always the specific API variant. I
>>>>> doubt this API would be of much use for the shared driver usage. This
>>>>> also implies that the client user does not care about specifying a lock
>>>>> in DT.
>>>>>
>>>> Afaict if the lock are shared then there shouldn't be a problem with
>>>> some clients using the request API and others request_specific(). As any
>>>> collisions would simply mean that there are more contention on the lock.
>>>>
>>>> With the current exclusive model that is not possible and the success of
>>>> the request_specific will depend on probe order.
>>>>
>>>> But perhaps it should be explicitly prohibited to use both APIs on the
>>>> same hwspinlock instance?
>>> Yeah, they are meant to be complimentary usage, though I doubt we will
>>> ever have any realistic users for the generic API if we haven't had a
>>> usage so far. I had posted a concept of reserved locks long back [1] to
>>> keep away certain locks from the generic requestor, but dropped it since
>>> we did not have an actual use-case needing it.
>>>
>>> regards
>>> Suman
>>>
>>> [1] https://lwn.net/Articles/611944/
_______________________________________________
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 v2 3/9] soc: samsung: Add Exynos Adaptive Supply Voltage driver
From: Robin Murphy @ 2019-08-08 12:48 UTC (permalink / raw)
To: Krzysztof Kozlowski, Sylwester Nawrocki
Cc: devicetree, linux-samsung-soc@vger.kernel.org,
Bartłomiej Żołnierkiewicz, pankaj.dubey, linux-pm,
linux-kernel@vger.kernel.org, robh+dt, kgene, vireshk,
linux-arm-kernel, Marek Szyprowski
In-Reply-To: <CAJKOXPc8iFo=2JAGEZSC46N3sZae4+JcZYBCjpKysb6PFPzyaQ@mail.gmail.com>
On 08/08/2019 13:31, Krzysztof Kozlowski wrote:
> On Thu, 8 Aug 2019 at 14:07, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote:
>>>> +static unsigned int exynos5422_asv_parse_table(struct exynos_asv *asv,
>>>> + unsigned int pkg_id)
>>>> +{
>>>> + return (pkg_id >> EXYNOS5422_TABLE_OFFSET) & EXYNOS5422_TABLE_MASK;
>>>> +}
>>>> +
>>>> +static bool exynos5422_asv_parse_bin2(struct exynos_asv *asv,
>>>> + unsigned int pkg_id)
>>>> +{
>>>> + return (pkg_id >> EXYNOS5422_BIN2_OFFSET) & EXYNOS5422_BIN2_MASK;
>>>
>>> return !!() for converting to boolean.
>>
>> I'm not convinced it is needed, the return type of the function is bool
>> and value of the expression will be implicitly converted to that type.
>> Is there any compiler warning related to that?
>
> Yeah, but bool is int so there will be no implicit conversion... I
> guess it is a convention. In theory !! is the proper conversion to
> bool but if bool==int then it's essentially conversion to 1. I am not
> sure what's the benefit, maybe for some wrong code which would do
> comparisons on result like if (exynos5422_asv_parse_bin2() == TRUE)...
Not so - since we use "-std=gnu89", we have C99-like _Bool, which our
bool is a typedef of. Conversions, either implicit or explicit, are
well-defined:
"6.3.1.2 Boolean type
When any scalar value is converted to _Bool, the result is 0 if the
value compares equal
to 0; otherwise, the result is 1."
This is even called out in Documentation/process/coding-style.rst:
"When using bool types the !! construction is not needed, which
eliminates a class of bugs."
Robin.
_______________________________________________
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