* [PATCH 2/2] arm64: dts: ti: k3-j721e-main: Fix gic-its node unit-address
From: Suman Anna @ 2019-08-08 14:39 UTC (permalink / raw)
To: Tero Kristo, Nishanth Menon; +Cc: devicetree, Robert Tivy, linux-arm-kernel
In-Reply-To: <20190808143929.11148-1-s-anna@ti.com>
The gic-its node unit-address has an additional zero compared
to the actual reg value. Fix it.
Fixes: 2d87061e70de ("arm64: dts: ti: Add Support for J721E SoC")
Reported-by: Robert Tivy <rtivy@ti.com>
Signed-off-by: Suman Anna <s-anna@ti.com>
---
arch/arm64/boot/dts/ti/k3-j721e-main.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
index a01308142f77..ea5c0fd42baf 100644
--- a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
@@ -31,7 +31,7 @@
/* vcpumntirq: virtual CPU interface maintenance interrupt */
interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
- gic_its: gic-its@18200000 {
+ gic_its: gic-its@1820000 {
compatible = "arm,gic-v3-its";
reg = <0x00 0x01820000 0x00 0x10000>;
socionext,synquacer-pre-its = <0x1000000 0x400000>;
--
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/2] K3 dts minor typo fixes
From: Suman Anna @ 2019-08-08 14:39 UTC (permalink / raw)
To: Tero Kristo, Nishanth Menon; +Cc: devicetree, linux-arm-kernel
Hi Tero, Nishanth,
The following 2 patches are minor typo fixes in git-its nodes
on both AM65x and J721E SoCs. Patches done on top of the latest
master.
regards
Suman
Suman Anna (2):
arm64: dts: ti: k3-am65-main: Fix gic-its node unit-address
arm64: dts: ti: k3-j721e-main: Fix gic-its node unit-address
arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 2 +-
arch/arm64/boot/dts/ti/k3-j721e-main.dtsi | 2 +-
2 files changed, 2 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
* [PATCHv2] PM / devfreq: Add dev_pm_qos support
From: Leonard Crestez @ 2019-08-08 14:39 UTC (permalink / raw)
To: MyungJoo Ham, Viresh Kumar, Saravana Kannan
Cc: Artur Świgoń, linux-pm, Rafael J. Wysocki,
Krzysztof Kozlowski, Lukasz Luba, Chanwoo Choi, Kyungmin Park,
Alexandre Bailon, Georgi Djakov, linux-arm-kernel
Add dev_pm_qos notifies to devfreq core in order to support frequency
limits via the dev_pm_qos_add_request.
Unlike the rest of devfreq the dev_pm_qos frequency is measured Khz:
this is consistent with current dev_pm_qos usage for cpufreq and allows
frequencies above 2Ghz (pm_qos expresses limits as s32).
Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
Changes since v1:
* Add doxygen comments for min_nb/max_nb
* Remove notifiers on error/cleanup paths. Keep gotos simple by relying
on dev_pm_qos_remove_notifier ignoring notifiers which were not added.
Link to v1: https://patchwork.kernel.org/patch/11078475/
drivers/devfreq/devfreq.c | 83 ++++++++++++++++++++++++++++++++++-----
include/linux/devfreq.h | 5 +++
2 files changed, 79 insertions(+), 9 deletions(-)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 784c08e4f931..7f1f273562f4 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -22,10 +22,11 @@
#include <linux/platform_device.h>
#include <linux/list.h>
#include <linux/printk.h>
#include <linux/hrtimer.h>
#include <linux/of.h>
+#include <linux/pm_qos.h>
#include "governor.h"
#define CREATE_TRACE_POINTS
#include <trace/events/devfreq.h>
@@ -96,10 +97,26 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
dev_pm_opp_put(opp);
return max_freq;
}
+static unsigned long get_effective_min_freq(struct devfreq *devfreq)
+{
+ return max3(devfreq->scaling_min_freq, devfreq->min_freq,
+ 1000 * (unsigned long)dev_pm_qos_read_value(
+ devfreq->dev.parent,
+ DEV_PM_QOS_MIN_FREQUENCY));
+}
+
+static unsigned long get_effective_max_freq(struct devfreq *devfreq)
+{
+ return min3(devfreq->scaling_max_freq, devfreq->max_freq,
+ 1000 * (unsigned long)dev_pm_qos_read_value(
+ devfreq->dev.parent,
+ DEV_PM_QOS_MAX_FREQUENCY));
+}
+
/**
* devfreq_get_freq_level() - Lookup freq_table for the frequency
* @devfreq: the devfreq instance
* @freq: the target frequency
*/
@@ -356,12 +373,12 @@ int update_devfreq(struct devfreq *devfreq)
*
* List from the highest priority
* max_freq
* min_freq
*/
- max_freq = min(devfreq->scaling_max_freq, devfreq->max_freq);
- min_freq = max(devfreq->scaling_min_freq, devfreq->min_freq);
+ max_freq = get_effective_max_freq(devfreq);
+ min_freq = get_effective_min_freq(devfreq);
if (freq < min_freq) {
freq = min_freq;
flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
}
@@ -570,10 +587,31 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
mutex_unlock(&devfreq->lock);
return ret;
}
+static int devfreq_qos_notifier_call(struct devfreq *devfreq)
+{
+ int ret;
+
+ mutex_lock(&devfreq->lock);
+ ret = update_devfreq(devfreq);
+ mutex_unlock(&devfreq->lock);
+
+ return ret;
+}
+
+static int devfreq_qos_min_notifier_call(struct notifier_block *nb, unsigned long val, void *ptr)
+{
+ return devfreq_qos_notifier_call(container_of(nb, struct devfreq, nb_min));
+}
+
+static int devfreq_qos_max_notifier_call(struct notifier_block *nb, unsigned long val, void *ptr)
+{
+ return devfreq_qos_notifier_call(container_of(nb, struct devfreq, nb_max));
+}
+
/**
* devfreq_dev_release() - Callback for struct device to release the device.
* @dev: the devfreq device
*
* Remove devfreq from the list and release its resources.
@@ -592,10 +630,14 @@ static void devfreq_dev_release(struct device *dev)
mutex_unlock(&devfreq_list_lock);
if (devfreq->profile->exit)
devfreq->profile->exit(devfreq->dev.parent);
+ dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max,
+ DEV_PM_QOS_MAX_FREQUENCY);
+ dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min,
+ DEV_PM_QOS_MIN_FREQUENCY);
mutex_destroy(&devfreq->lock);
kfree(devfreq);
}
/**
@@ -636,21 +678,44 @@ struct devfreq *devfreq_add_device(struct device *dev,
err = -ENOMEM;
goto err_out;
}
mutex_init(&devfreq->lock);
- mutex_lock(&devfreq->lock);
devfreq->dev.parent = dev;
devfreq->dev.class = devfreq_class;
devfreq->dev.release = devfreq_dev_release;
devfreq->profile = profile;
strncpy(devfreq->governor_name, governor_name, DEVFREQ_NAME_LEN);
devfreq->previous_freq = profile->initial_freq;
devfreq->last_status.current_frequency = profile->initial_freq;
devfreq->data = data;
devfreq->nb.notifier_call = devfreq_notifier_call;
+ /*
+ * notifier from pm_qos
+ *
+ * initialized outside of devfreq->lock to avoid circular warning
+ * between devfreq->lock and dev_pm_qos_mtx
+ */
+ devfreq->nb_min.notifier_call = devfreq_qos_min_notifier_call;
+ devfreq->nb_max.notifier_call = devfreq_qos_max_notifier_call;
+
+ err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min,
+ DEV_PM_QOS_MIN_FREQUENCY);
+ if (err) {
+ dev_err(dev, "Failed to register MIN QoS notifier: %d\n", err);
+ goto err_dev;
+ }
+
+ err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max,
+ DEV_PM_QOS_MAX_FREQUENCY);
+ if (err) {
+ dev_err(dev, "Failed to register MAX QoS notifier: %d\n", err);
+ goto err_dev;
+ }
+
+ mutex_lock(&devfreq->lock);
if (!devfreq->profile->max_state && !devfreq->profile->freq_table) {
mutex_unlock(&devfreq->lock);
err = set_freq_table(devfreq);
if (err < 0)
goto err_dev;
@@ -741,10 +806,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
mutex_unlock(&devfreq_list_lock);
err_devfreq:
devfreq_remove_device(devfreq);
devfreq = NULL;
err_dev:
+ dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max,
+ DEV_PM_QOS_MAX_FREQUENCY);
+ dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min,
+ DEV_PM_QOS_MIN_FREQUENCY);
kfree(devfreq);
err_out:
return ERR_PTR(err);
}
EXPORT_SYMBOL(devfreq_add_device);
@@ -1311,13 +1380,11 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
}
static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
- struct devfreq *df = to_devfreq(dev);
-
- return sprintf(buf, "%lu\n", max(df->scaling_min_freq, df->min_freq));
+ return sprintf(buf, "%lu\n", get_effective_min_freq(to_devfreq(dev)));
}
static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
@@ -1356,13 +1423,11 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
static DEVICE_ATTR_RW(min_freq);
static ssize_t max_freq_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
- struct devfreq *df = to_devfreq(dev);
-
- return sprintf(buf, "%lu\n", min(df->scaling_max_freq, df->max_freq));
+ return sprintf(buf, "%lu\n", get_effective_max_freq(to_devfreq(dev)));
}
static DEVICE_ATTR_RW(max_freq);
static ssize_t available_frequencies_show(struct device *d,
struct device_attribute *attr,
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 2bae9ed3c783..8b92ccbd1962 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -134,10 +134,12 @@ struct devfreq_dev_profile {
* @total_trans: Number of devfreq transitions
* @trans_table: Statistics of devfreq transitions
* @time_in_state: Statistics of devfreq states
* @last_stat_updated: The last time stat updated
* @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier
+ * @nb_min: Notifier block for DEV_PM_QOS_MIN_FREQUENCY
+ * @nb_max: Notifier block for DEV_PM_QOS_MAX_FREQUENCY
*
* This structure stores the devfreq information for a give device.
*
* Note that when a governor accesses entries in struct devfreq in its
* functions except for the context of callbacks defined in struct
@@ -176,10 +178,13 @@ struct devfreq {
unsigned int *trans_table;
unsigned long *time_in_state;
unsigned long last_stat_updated;
struct srcu_notifier_head transition_notifier_list;
+
+ struct notifier_block nb_min;
+ struct notifier_block nb_max;
};
struct devfreq_freqs {
unsigned long old;
unsigned long new;
--
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 related
* Re: [PATCH v2] arm64: dts: rockchip: Add dts for Leez RK3399 P710 SBC
From: Rob Herring @ 2019-08-08 14:24 UTC (permalink / raw)
To: Andy Yan
Cc: devicetree, linux-kernel@vger.kernel.org, heiko@sntech.de,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
open list:ARM/Rockchip SoC...
In-Reply-To: <CANbgqATvVSo_D-n_mW2hK2KEK_8cs3374ddB6C8GcZZwjMSoRQ@mail.gmail.com>
On Wed, Aug 7, 2019 at 12:14 AM Andy Yan <andyshrk@gmail.com> wrote:
>
> Hi Rob:
>
> Rob Herring <robh+dt@kernel.org> 于2019年8月6日周二 下午10:48写道：
>>
>> On Mon, Aug 5, 2019 at 6:40 AM Andy Yan <andyshrk@gmail.com> wrote:
>> >
>> > P710 is a RK3399 based SBC, designed by Leez [0].
>> >
>> > Specification
>> > - Rockchip RK3399
>> > - 4/2GB LPDDR4
>> > - TF sd scard slot
>> > - eMMC
>> > - M.2 B-Key for 4G LTE
>> > - AP6256 for WiFi + BT
>> > - Gigabit ethernet
>> > - HDMI out
>> > - 40 pin header
>> > - USB 2.0 x 2
>> > - USB 3.0 x 1
>> > - USB 3.0 Type-C x 1
>> > - TYPE-C Power supply
>> >
>> > [0]https://leez.lenovo.com
>>
>> I'm not really convinced Leez is a vendor. Looks like branding to me.
>> We have enough with company names changing, we don't need changing
>> brands too. Use 'lenovo'.
>>
>
> I had checked with Leez people before V1, they said Leez will run as an independent company, so they don't want to
> give a lenovo label for this board.
Okay.
Reviewed-by: Rob Herring <robh@kernel.org>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: i2c: imx: support slave mode for imx I2C driver
From: Sascha Hauer @ 2019-08-08 14:18 UTC (permalink / raw)
To: Biwen Li
Cc: wsa, shawnguo, linux-kernel, linux-i2c, kernel, laurentiu.tudor,
festevam, linux-arm-kernel, linux-imx
In-Reply-To: <20190808035343.34120-1-biwen.li@nxp.com>
Hi,
On Thu, Aug 08, 2019 at 11:53:43AM +0800, Biwen Li wrote:
> The patch supports slave mode for imx I2C driver
>
> Signed-off-by: Biwen Li <biwen.li@nxp.com>
> ---
> drivers/i2c/busses/i2c-imx.c | 199 ++++++++++++++++++++++++++++++++---
> 1 file changed, 185 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index b1b8b938d7f4..f7583a9fa56f 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -202,6 +202,9 @@ struct imx_i2c_struct {
> struct pinctrl_state *pinctrl_pins_gpio;
>
> struct imx_i2c_dma *dma;
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> + struct i2c_client *slave;
> +#endif /* CONFIG_I2C_SLAVE */
Other drivers just do a "select I2C_SLAVE" in Kconfig to get rid of
these #ifs. We should do the same.
> };
>
> static const struct imx_i2c_hwdata imx1_i2c_hwdata = {
> @@ -583,23 +586,40 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
> imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> }
>
> -static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
> +/* Clear interrupt flag bit */
> +static void i2c_imx_clr_if_bit(struct imx_i2c_struct *i2c_imx)
> {
> - struct imx_i2c_struct *i2c_imx = dev_id;
> - unsigned int temp;
> + unsigned int status;
>
> - temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> - if (temp & I2SR_IIF) {
> - /* save status register */
> - i2c_imx->i2csr = temp;
> - temp &= ~I2SR_IIF;
> - temp |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IIF);
> - imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
> - wake_up(&i2c_imx->queue);
> - return IRQ_HANDLED;
> - }
> + status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> + status &= ~I2SR_IIF;
> + status |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IIF);
> + imx_i2c_write_reg(status, i2c_imx, IMX_I2C_I2SR);
> +}
> +
> +/* Clear arbitration lost bit */
> +static void i2c_imx_clr_al_bit(struct imx_i2c_struct *i2c_imx)
> +{
> + unsigned int status;
> +
> + status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> + status &= ~I2SR_IAL;
> + imx_i2c_write_reg(status, i2c_imx, IMX_I2C_I2SR);
> +}
>
> - return IRQ_NONE;
> +static irqreturn_t i2c_imx_master_isr(struct imx_i2c_struct *i2c_imx)
> +{
> + unsigned int status;
> +
> + dev_dbg(&i2c_imx->adapter.dev, "<%s>: master interrupt\n", __func__);
Generally this driver has way too many dev_dbg spread around in hot
pathes already. IMO adding more doesn't make the output more useful.
> +
> + /* Save status register */
> + status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> + i2c_imx->i2csr = status | I2SR_IIF;
> +
> + wake_up(&i2c_imx->queue);
> +
> + return IRQ_HANDLED;
> }
>
> static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
> @@ -1043,11 +1063,162 @@ static u32 i2c_imx_func(struct i2c_adapter *adapter)
> | I2C_FUNC_SMBUS_READ_BLOCK_DATA;
> }
>
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +static void i2c_imx_slave_init(struct imx_i2c_struct *i2c_imx)
> +{
> + unsigned int temp;
> +
> + dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
> +
> + /* Set slave addr. */
> + imx_i2c_write_reg((i2c_imx->slave->addr << 1), i2c_imx, IMX_I2C_IADR);
> +
> + /* Disable i2c module */
> + temp = i2c_imx->hwdata->i2cr_ien_opcode
> + ^ I2CR_IEN;
unnecessary line break.
> + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +
> + /* Reset status register */
> + imx_i2c_write_reg(i2c_imx->hwdata->i2sr_clr_opcode, i2c_imx,
> + IMX_I2C_I2SR);
> +
> + /* Enable module and enable interrupt from i2c module */
> + temp = i2c_imx->hwdata->i2cr_ien_opcode
> + | I2CR_IIEN;
ditto.
> + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +
> + /* Wait controller to be stable */
> + usleep_range(50, 150);
> +}
> +
> +static irqreturn_t i2c_imx_slave_isr(struct imx_i2c_struct *i2c_imx)
> +{
> + unsigned int status, ctl;
> + u8 value;
> +
> + if (!i2c_imx->slave) {
> + dev_err(&i2c_imx->adapter.dev, "cannot deal with slave irq,i2c_imx->slave is null");
> + return IRQ_NONE;
> + }
> +
> + status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> + ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> + if (status & I2SR_IAL) { /* Arbitration lost */
> + i2c_imx_clr_al_bit(i2c_imx);
> + } else if (status & I2SR_IAAS) { /* Addressed as a slave */
> + if (status & I2SR_SRW) { /* Master wants to read from us*/
> + dev_dbg(&i2c_imx->adapter.dev, "read requested");
> + i2c_slave_event(i2c_imx->slave, I2C_SLAVE_READ_REQUESTED, &value);
> +
> + /* Slave transimt */
s/transimt/transmit/
> + ctl |= I2CR_MTX;
> + imx_i2c_write_reg(ctl, i2c_imx, IMX_I2C_I2CR);
> +
> + /* Send data */
> + imx_i2c_write_reg(value, i2c_imx, IMX_I2C_I2DR);
> + } else { /* Master wants to write to us */
> + dev_dbg(&i2c_imx->adapter.dev, "write requested");
> + i2c_slave_event(i2c_imx->slave, I2C_SLAVE_WRITE_REQUESTED, &value);
> +
> + /* Slave receive */
> + ctl &= ~I2CR_MTX;
> + imx_i2c_write_reg(ctl, i2c_imx, IMX_I2C_I2CR);
> + /* Dummy read */
> + value = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
value is unused, no need to assign a value to it.
> + }
> + } else {
> + if (!(ctl & I2CR_MTX)) { /* Receive mode */
Since you have an 'else' path please convert this to positive logic.
This makes it easier to read.
> + if (status & I2SR_IBB) { /* No STOP signal detected */
> + ctl &= ~I2CR_MTX;
> + imx_i2c_write_reg(ctl, i2c_imx, IMX_I2C_I2CR);
> +
> + value = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
> + i2c_slave_event(i2c_imx->slave, I2C_SLAVE_WRITE_RECEIVED, &value);
> + } else { /* STOP signal is detected */
> + dev_dbg(&i2c_imx->adapter.dev,
> + "STOP signal detected");
> + i2c_slave_event(i2c_imx->slave, I2C_SLAVE_STOP, &value);
> + }
> + } else { /* Transmit mode */
> + if (!(status & I2SR_RXAK)) { /* Received ACK */
Same here.
> + ctl |= I2CR_MTX;
> + imx_i2c_write_reg(ctl, i2c_imx, IMX_I2C_I2CR);
> +
> + i2c_slave_event(i2c_imx->slave, I2C_SLAVE_READ_PROCESSED, &value);
> +
> + imx_i2c_write_reg(value, i2c_imx, IMX_I2C_I2DR);
> + } else { /* Received NAK */
> + ctl &= ~I2CR_MTX;
> + imx_i2c_write_reg(ctl, i2c_imx, IMX_I2C_I2CR);
> + value = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
Also value unused.
> + }
> + }
> + }
> + return IRQ_HANDLED;
> +}
> +
> +static int i2c_imx_reg_slave(struct i2c_client *client)
> +{
> + struct imx_i2c_struct *i2c_imx = i2c_get_adapdata(client->adapter);
> +
> + if (i2c_imx->slave)
> + return -EINVAL;
Better -EBUSY?
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
_______________________________________________
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/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
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