* [PATCH 1/3] devicetree: bindings: pinctrl: Add binding for ti,da850-pupd
From: Sekhar Nori @ 2016-11-23 11:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1479871767-20160-2-git-send-email-david@lechnology.com>
On Wednesday 23 November 2016 08:59 AM, David Lechner wrote:
> Device-tree bindings for TI DA8XX/OMAP-L138/AM18XX pullup/pulldown
s/DA8XX/DA850. It looks like this support is absent from DA830.
> pinconf controller.
>
> Signed-off-by: David Lechner <david@lechnology.com>
Thanks,
Sekhar
^ permalink raw reply
* [PATCH net-next 1/4] net: mvneta: Convert to be 64 bits compatible
From: Jisheng Zhang @ 2016-11-23 11:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <9432400.S1OrxC027t@wuerfel>
Hi Arnd,
On Wed, 23 Nov 2016 11:15:32 +0100 Arnd Bergmann wrote:
> On Wednesday, November 23, 2016 5:53:41 PM CET Jisheng Zhang wrote:
> > On Tue, 22 Nov 2016 22:04:12 +0100 Arnd Bergmann wrote:
> >
> > > On Tuesday, November 22, 2016 5:48:41 PM CET Gregory CLEMENT wrote:
> > > > +#ifdef CONFIG_64BIT
> > > > + void *data_tmp;
> > > > +
> > > > + /* In Neta HW only 32 bits data is supported, so in order to
> > > > + * obtain whole 64 bits address from RX descriptor, we store
> > > > + * the upper 32 bits when allocating buffer, and put it back
> > > > + * when using buffer cookie for accessing packet in memory.
> > > > + * Frags should be allocated from single 'memory' region,
> > > > + * hence common upper address half should be sufficient.
> > > > + */
> > > > + data_tmp = mvneta_frag_alloc(pp->frag_size);
> > > > + if (data_tmp) {
> > > > + pp->data_high = (u64)upper_32_bits((u64)data_tmp) << 32;
> > > > + mvneta_frag_free(pp->frag_size, data_tmp);
> > > > + }
> > > >
> > >
> > > How does this work when the region spans a n*4GB address boundary?
> >
> > indeed. We also make use of this driver on 64bit platforms. We use
> > different solution to make the driver 64bit safe.
> >
> > solA: make use of the reserved field in the mvneta_rx_desc, such
> > as reserved2 etc. Yes, the field is marked as "for future use, PnC", but
> > now it's not used at all. This is one possible solution however.
>
> Right, this sounds like the most straightforward choice.
>
> > solB: allocate a shadow buf cookie during init, e.g
> >
> > rxq->descs_bufcookie = kmalloc(rxq->size * sizeof(void*), GFP_KERNEL);
> >
> > then modify mvneta_rx_desc_fill a bit to save the 64bit pointer in
> > the shadow buf cookie, e.g
> > static void mvneta_rx_desc_fill(struct mvneta_rx_desc *rx_desc,
> > u32 phys_addr, u32 cookie,
sorry, this line should be:
u32 phys_addr, void *cookie
> > struct mvneta_rx_queue *rxq)
> >
> > {
> > int i;
> >
> > rx_desc->buf_cookie = cookie;
> > rx_desc->buf_phys_addr = phys_addr;
> > i = rx_desc - rxq->descs;
> > rxq->descs_bufcookie[i] = cookie;
> > }
> >
> > then fetch the desc from the shadow buf cookie in all code path, such
> > as mvneta_rx() etc.
> >
> > Both solutions should not have the problems pointed out by Arnd.
>
> Wait, since you compute an index 'i' here, can't you just store 'i'
> directly in the descriptor instead of the pointer?
>
we need to store the pointer, it's to store the buffer allocated by
mvneta_frag_alloc()
Thanks,
Jisheng
^ permalink raw reply
* [PATCH 2/3] pinctrl: New driver for TI DA8XX/OMAP-L138/AM18XX pinconf
From: Sekhar Nori @ 2016-11-23 11:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1479871767-20160-3-git-send-email-david@lechnology.com>
On Wednesday 23 November 2016 08:59 AM, David Lechner wrote:
> This adds a new driver for pinconf on TI DA8XX/OMAP-L138/AM18XX. These
s/DA8XX/DA850/
> SoCs have a separate controller for controlling pullup/pulldown groups.
>
> Signed-off-by: David Lechner <david@lechnology.com>
> +static const char *da850_pupd_get_get_group_name(struct pinctrl_dev *pctldev,
> + unsigned int selector)
> +{
> + return da850_pupd_group_names[selector];
> +}
> +
> +static int da850_pupd_get_get_group_pins(struct pinctrl_dev *pctldev,
> + unsigned int selector,
> + const unsigned int **pins,
> + unsigned int *num_pins)
> +{
> + *num_pins = 0;
> +
> + return 0;
> +}
usage of get_get_ in the function names above is odd.
Thanks,
Sekhar
^ permalink raw reply
* [PATCH v3 0/3] da8xx: fix section mismatch in new drivers
From: Bartosz Golaszewski @ 2016-11-23 11:06 UTC (permalink / raw)
To: linux-arm-kernel
Sekhar noticed there's a section mismatch in the da8xx-mstpri and
da8xx-ddrctl drivers. This is caused by calling
of_flat_dt_get_machine_name() which has an __init annotation.
This series addresses this issue by open coding routines that return
the machine compatible string in both drivers. Once a general function
for that in of/base is merged, we'll remove them.
The third patch fixes a typo that got in last time.
v1 -> v2:
- drop patch [1/3] from v1
- introduce internal routines in the drivers instead of a general
function in of/base.c
v2 -> v3:
- use of_property_read_string_index() instead of
of_property_read_string() to get the first compatible entry
- s/priotities/priorities
Bartosz Golaszewski (3):
bus: da8xx-mstpri: drop the call to of_flat_dt_get_machine_name()
memory: da8xx-ddrctl: drop the call to of_flat_dt_get_machine_name()
bus: da8xx-mstpri: fix a typo
drivers/bus/da8xx-mstpri.c | 25 ++++++++++++++++++++++---
drivers/memory/da8xx-ddrctl.c | 23 +++++++++++++++++++++--
2 files changed, 43 insertions(+), 5 deletions(-)
--
2.9.3
^ permalink raw reply
* [PATCH v3 1/3] bus: da8xx-mstpri: drop the call to of_flat_dt_get_machine_name()
From: Bartosz Golaszewski @ 2016-11-23 11:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1479899187-10199-1-git-send-email-bgolaszewski@baylibre.com>
In order to avoid a section mismatch use a locally implemented routine
instead of of_flat_dt_get_machine_name() when printing the error
message.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
drivers/bus/da8xx-mstpri.c | 23 +++++++++++++++++++++--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/drivers/bus/da8xx-mstpri.c b/drivers/bus/da8xx-mstpri.c
index 85f0b53..064eeb9 100644
--- a/drivers/bus/da8xx-mstpri.c
+++ b/drivers/bus/da8xx-mstpri.c
@@ -16,7 +16,6 @@
#include <linux/platform_device.h>
#include <linux/io.h>
#include <linux/regmap.h>
-#include <linux/of_fdt.h>
/*
* REVISIT: Linux doesn't have a good framework for the kind of performance
@@ -190,6 +189,26 @@ static const struct da8xx_mstpri_board_priorities da8xx_mstpri_board_confs[] = {
},
};
+/*
+ * FIXME Remove this function once of/base gets a general routine for getting
+ * the machine model/compatible string.
+ */
+static const char *da8xx_mstpri_machine_get_compatible(void)
+{
+ struct device_node *root;
+ const char *compatible;
+ int ret = -1;
+
+ root = of_find_node_by_path("/");
+ if (root) {
+ ret = of_property_read_string_index(root, "compatible",
+ 0, &compatible);
+ of_node_put(root);
+ }
+
+ return ret ? NULL : compatible;
+}
+
static const struct da8xx_mstpri_board_priorities *
da8xx_mstpri_get_board_prio(void)
{
@@ -227,7 +246,7 @@ static int da8xx_mstpri_probe(struct platform_device *pdev)
prio_list = da8xx_mstpri_get_board_prio();
if (!prio_list) {
dev_err(dev, "no master priotities defined for board '%s'\n",
- of_flat_dt_get_machine_name());
+ da8xx_mstpri_machine_get_compatible());
return -EINVAL;
}
--
2.9.3
^ permalink raw reply related
* [PATCH v3 2/3] memory: da8xx-ddrctl: drop the call to of_flat_dt_get_machine_name()
From: Bartosz Golaszewski @ 2016-11-23 11:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1479899187-10199-1-git-send-email-bgolaszewski@baylibre.com>
In order to avoid a section mismatch use a locally implemented routine
instead of of_flat_dt_get_machine_name() when printing the error
message.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
drivers/memory/da8xx-ddrctl.c | 23 +++++++++++++++++++++--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/drivers/memory/da8xx-ddrctl.c b/drivers/memory/da8xx-ddrctl.c
index a20e7bb..1b962ee 100644
--- a/drivers/memory/da8xx-ddrctl.c
+++ b/drivers/memory/da8xx-ddrctl.c
@@ -14,7 +14,6 @@
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_device.h>
-#include <linux/of_fdt.h>
#include <linux/platform_device.h>
#include <linux/io.h>
@@ -71,6 +70,26 @@ static const struct da8xx_ddrctl_board_settings da8xx_ddrctl_board_confs[] = {
},
};
+/*
+ * FIXME Remove this function once of/base gets a general routine for getting
+ * the machine model/compatible string.
+ */
+static const char *da8xx_ddrctl_machine_get_compatible(void)
+{
+ struct device_node *root;
+ const char *compatible;
+ int ret = -1;
+
+ root = of_find_node_by_path("/");
+ if (root) {
+ ret = of_property_read_string_index(root, "compatible",
+ 0, &compatible);
+ of_node_put(root);
+ }
+
+ return ret ? NULL : compatible;
+}
+
static const struct da8xx_ddrctl_config_knob *
da8xx_ddrctl_match_knob(const struct da8xx_ddrctl_setting *setting)
{
@@ -118,7 +137,7 @@ static int da8xx_ddrctl_probe(struct platform_device *pdev)
setting = da8xx_ddrctl_get_board_settings();
if (!setting) {
dev_err(dev, "no settings for board '%s'\n",
- of_flat_dt_get_machine_name());
+ da8xx_ddrctl_machine_get_compatible());
return -EINVAL;
}
--
2.9.3
^ permalink raw reply related
* [PATCH v3 3/3] bus: da8xx-mstpri: fix a typo
From: Bartosz Golaszewski @ 2016-11-23 11:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1479899187-10199-1-git-send-email-bgolaszewski@baylibre.com>
Should have been priorities.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
drivers/bus/da8xx-mstpri.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/bus/da8xx-mstpri.c b/drivers/bus/da8xx-mstpri.c
index 064eeb9..b17ba97 100644
--- a/drivers/bus/da8xx-mstpri.c
+++ b/drivers/bus/da8xx-mstpri.c
@@ -245,7 +245,7 @@ static int da8xx_mstpri_probe(struct platform_device *pdev)
prio_list = da8xx_mstpri_get_board_prio();
if (!prio_list) {
- dev_err(dev, "no master priotities defined for board '%s'\n",
+ dev_err(dev, "no master priorities defined for board '%s'\n",
da8xx_mstpri_machine_get_compatible());
return -EINVAL;
}
--
2.9.3
^ permalink raw reply related
* Synopsys Ethernet QoS Driver
From: Joao Pinto @ 2016-11-23 11:10 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <c37c47e1-8e21-1b11-ed15-6b899ed1dd03@st.com>
Hi Peppe and Lars,
On 23-11-2016 10:59, Giuseppe CAVALLARO wrote:
> Hello Joao, Lars.
>
> On 11/22/2016 3:16 PM, Joao Pinto wrote:
>>> Ok, it makes sense.
>>> > Just for curiosity the target setup is the following:
>>> > https://www.youtube.com/watch?v=8V-LB5y2Cos
>>> > but instead of using internal drivers, we desire to use mainline drivers only.
>>> >
>>> > Thanks!
>> Regarding this subject, I am thinking of making the following adaption:
>>
>> a) delete ethernet/synopsys
>> b) rename ethernet/stmicro/stmmac to ethernet/synopsys
>>
>> and send you a patch for you to evaluate. Both agree with the approach?
>> To have a new work base would be important, because I will add to the "new"
>> structure some missing QoS features like Multichannel support, CBS and later TSN.
>
> IMO, we have to agree on a common strategy making the change for
> net-next; I imaged the following steps:
Yes it makes totally sense.
>
> - to port missing feature or fixes from ethernet/synopsys
> inside the stmmac taking care about the documentation too.
@Lars: You are familiar with the synopsys qos driver. Could you please do this
porting. You can also make an analysis of what to port and I can do the porting
for you if you don't have the availability for it.
> - remove ethernet/synopsys
> - rename ethernet/stmicro/stmmac to ethernet/synopsys
I volunteer to do this task.
>
> These latest two have some relevant impacts.
>
> This change should be propagated to all the platforms that are using:
> CONFIG_SYNOPSYS_DWC_ETH_QOS and CONFIG_STMMAC_ETH
> plus device-tree compatibility.
I volunteer to do this task also.
>
> - enhance the stmmac with new features and new glue (part of these
> can be anticipated for sure).
I have to implement 3 new features for now, but I will take some time for it, so
I would suggest to make the previous task and incrementally add features.
>
> what do you think? does it make sense? If yes, we can also
> understand how/who starts.
>
> Regards,
> Peppe
Thanks and regards.
Joao
>
>> Thanks.
>
^ permalink raw reply
* [PATCH] mfd: twl-core: export twl_get_regmap
From: Russell King - ARM Linux @ 2016-11-23 11:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161121133755.GF23750@n2100.armlinux.org.uk>
On Mon, Nov 21, 2016 at 01:37:55PM +0000, Russell King - ARM Linux wrote:
> On Mon, Nov 21, 2016 at 12:03:03PM +0200, Nicolae Rosia wrote:
> > On Mon, Nov 21, 2016 at 11:31 AM, Russell King - ARM Linux
> > <linux@armlinux.org.uk> wrote:
> > > Passing data between drivers using *_set_drvdata() is a layering
> > > violation:
> > >
> > > 1. Driver data is supposed to be driver private data associated with
> > > the currently bound driver.
> > > 2. The driver data pointer is NULL'd when the driver unbinds from the
> > > device. See __device_release_driver() and the
> > > dev_set_drvdata(dev, NULL).
> > > 3. It will break with CONFIG_DEBUG_TEST_DRIVER_REMOVE enabled for a
> > > similar reason to (2).
> > >
> > > So, do not pass data between drivers using *_set_drvdata() - any
> > > examples in the kernel already are founded on bad practice, are
> > > fragile, and are already broken for some kernel configurations.
> >
> > After inspecting mfd_add_device, it seems that it creates a
> > platform_device which has the parent set to the driver calling the
> > function.
> > Isn't module unloading forbidden if there is a parent->child
> > relationship in place and you're removing the parent?
>
> Forget this idea that there's any connection between modules and
> the struct device relationships - there isn't anything of the kind!
>
> Each struct device is refcounted, and child devices will hold a
> reference to their parent device, so the parent device doesn't get
> freed before its children are all gone.
>
> That's a completely separate issue to when a struct device is bound
> to a struct device_driver - it's entirely possible for parent drivers
> to be unbound at any time, even when there are child drivers in place.
>
> There are cases where we want that to happen - think of any driver
> which is a bus driver in itself - eg, PCMCIA, MMC, USB, etc. These
> drivers enumerate their children, and destroy their children when
> the driver is unbound - but the driver has to be in the process of
> being unbound for that to happen. That process may very well start
> with the child devices being bound to their drivers.
>
> What makes the child drivers unbind is when the bus driver deletes
> the child struct devices.
>
> > What should be the best practice to share data between drivers?
> > Reference counted data?
>
> I guess so, but you will still have a race if you do something like:
>
> struct parent_private_data *parent_priv = dev_get_drvdata(dev->parent);
>
> Yes, that'll get the parent's driver private data, but what you don't
> know is whether the pointer remains valid, and even if you do as the
> very next step:
>
> kref_get(&parent_priv->kref);
>
> you don't know whether parent_priv was kfree()d between these two
> statements.
>
> However, if the parent driver creates the struct device that you're
> using and deletes the struct device before it frees its private data,
> then you can be sure that parent_priv will be valid, because the child
> drivers will be unbound during the parent driver's ->remove function,
> _before_ the private data is freed.
>
> > In the case of TWL, the twl-core is just a simple container for
> > regmaps - all other "sub devices" are using those regmaps to access
> > the I2C device's registers, it makes no sense to remove the parent
> > driver since it does *nothing*.
>
> I can't comment on what twl-core is doing, I haven't looked at it in
> ages, but most MFD drivers have the parent device creating and destroying
> their children, so it should be fine.
>
> My original comment was more along the lines of a parent device poking
> driver-private data into the child devices it was creating for the
> child drivers to pick up. However, it's worth discussing the validity
> cases of the parent's driver data too, as per the above.
I was just curious, and I took a peek at the OMAP/TWL DT files, and
I see that it's left to DT to create the children.
So, there is already _no_ lifetime relationship between the children
and the parent device drivers being probed.
What's even more fun is this:
static int
twl_probe(struct i2c_client *client, const struct i2c_device_id *id)
{
...
if (twl_priv) {
dev_dbg(&client->dev, "only one instance of %s allowed\n",
DRIVER_NAME);
return -EBUSY;
}
...
twl_priv = devm_kzalloc(&client->dev, sizeof(struct twl_private),
GFP_KERNEL);
if (!twl_priv) {
status = -ENOMEM;
goto free;
}
...
twl->regmap = devm_regmap_init_i2c(twl->client,
&twl_regmap_config[i]);
if (IS_ERR(twl->regmap)) {
status = PTR_ERR(twl->regmap);
dev_err(&client->dev,
"Failed to allocate regmap %d, err: %d\n", i,
status);
goto fail;
}
...
So, if we get a failure after successfully allocating twl_priv, then
the driver and device are dead - it can't ever be retried. What's
more is that twl_priv contains a stale pointer - and use of it would
be a use-after-free bug, even to inspect twl_priv->ready.
That brings us on to the remove path:
static int twl_remove(struct i2c_client *client)
{
...
twl_priv->ready = false;
return 0;
}
which is pretty much useless - twl_priv will be kfree()d after this
function returns, so dereferencing twl_priv is again a use-after-free
bug. What's more is that the memory pointed to by twl_priv can be
reallocated, and ->ready could contain any value.
Now, there's a bunch of sub-nodes declared in DT which cause drivers
to be probed (eg, the twl-pwmled driver). These make use of
twl_i2c_read_u8() etc to read/write registers on the device. These
call through to twl_i2c_read() and twl_i2c_write(), both of which
use twl_get_regmap().
twl_get_regmap() dereferences twl_priv, which as established above may
have been kfree()d if the twl-core driver has been unbound. Even if
twl_priv survives with its stale data, the regmap in twl->regmap will
also have been freed, so the regmap accesses are likely to screw up.
In any case, the result is likely not going to be nice.
Note that you can't fail in a driver's remove method, so you can't stop
the twl-core driver being unbound by returning an error there: the
return value is ignored.
One possible approach to this would be to make twl-core built-in only,
remove the .remove method from the driver, and set suppress_bind_attrs
in the driver structure, so userspace can't bind/unbind the I2C
driver. However, that's just papering over the problem - if the I2C
_bus_ driver gets unbound, exactly the same problem exists - I2C will
delete the clients on the bus which will cause drivers to be unbound.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* [PATCH 3/3] ARM: dts: da850: Add node for pullup/pulldown pinconf
From: Sekhar Nori @ 2016-11-23 11:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1479871767-20160-4-git-send-email-david@lechnology.com>
On Wednesday 23 November 2016 08:59 AM, David Lechner wrote:
> This SoC has a separate pin controller for configuring pullup/pulldown
> bias on groups of pins.
>
> Signed-off-by: David Lechner <david@lechnology.com>
> ---
> arch/arm/boot/dts/da850.dtsi | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
> index 8945815..1c0224c 100644
> --- a/arch/arm/boot/dts/da850.dtsi
> +++ b/arch/arm/boot/dts/da850.dtsi
> @@ -210,6 +210,11 @@
> };
>
> };
> + pinconf: pin-controller at 22c00c {
> + compatible = "ti,da850-pupd";
> + reg = <0x22c00c 0x8>;
> + status = "disabled";
> + };
Can you please place this below the i2c1 node. I am trying to keep the
nodes sorted by unit address. I know thats broken in many places today,
but lets add the new ones where they should eventually end up.
Thanks,
Sekhar
^ permalink raw reply
* [PATCH 0/3] TI DA850/OMAP-L138/AM18x pinconf
From: Sekhar Nori @ 2016-11-23 11:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1479871767-20160-1-git-send-email-david@lechnology.com>
On Wednesday 23 November 2016 08:59 AM, David Lechner wrote:
> This series adds a new driver and DT bindings for TI DA850/OMAP-L138/AM18x
> pinconf (bias pullup/pulldown).
>
> The motivation for this series is LEGO MINDSTORMS EV3 support. It needs most,
> if not all, internal pullup/down resistors disabled in order to work correctly.
This looks really neat to my non-pinconf trained eyes. I have sent some
minor comments. But apart from that:
Reviewed-by: Sekhar Nori <nsekhar@ti.com>
3/3 should go through my tree. If the driver patches are going to get
queued for v4.10, I can queue the DTS portion through my tree.
Thanks,
Sekhar
^ permalink raw reply
* [PATCH V9 11/11] ARM64/PCI: Support for ACPI based PCI host controller
From: Tomasz Nowicki @ 2016-11-23 11:21 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161122231321.GA20246@bhelgaas-glaptop.roam.corp.google.com>
Hi Bjorn,
On 23.11.2016 00:13, Bjorn Helgaas wrote:
> Hi Tomasz,
>
> On Fri, Jun 10, 2016 at 09:55:19PM +0200, Tomasz Nowicki wrote:
>> Implement pci_acpi_scan_root and other arch-specific call so that ARM64
>> can start using ACPI to setup and enumerate PCI buses.
>>
>> Prior to buses enumeration the pci_acpi_scan_root() implementation looks
>> for configuration space start address (obtained through ACPI _CBA method or
>> MCFG interface). If succeed, it uses ECAM library to create new mapping.
>> Then it attaches generic ECAM ops (pci_generic_ecam_ops) which are used
>> for accessing configuration space later on.
>> ...
>
>> +static struct acpi_pci_root_ops acpi_pci_root_ops = {
>> + .release_info = pci_acpi_generic_release_info,
>> +};
>> +
>> +/* Interface called from ACPI code to setup PCI host controller */
>> struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>> {
>> - /* TODO: Should be revisited when implementing PCI on ACPI */
>> - return NULL;
>> + int node = acpi_get_node(root->device->handle);
>> + struct acpi_pci_generic_root_info *ri;
>> + struct pci_bus *bus, *child;
>> +
>> + ri = kzalloc_node(sizeof(*ri), GFP_KERNEL, node);
>> + if (!ri)
>> + return NULL;
>> +
>> + ri->cfg = pci_acpi_setup_ecam_mapping(root);
>> + if (!ri->cfg) {
>> + kfree(ri);
>> + return NULL;
>> + }
>> +
>> + acpi_pci_root_ops.pci_ops = &ri->cfg->ops->pci_ops;
>
> This has already been merged, but this isn't right, is it? We're
> writing a host controller-specific pointer into the single system-wide
> acpi_pci_root_ops, then passing it on to acpi_pci_root_create().
>
> Today, I think ri->cfg->ops->pci_ops is always &pci_generic_ecam_ops,
> from this path:
>
> ri->cfg = pci_acpi_setup_ecam_mapping
> cfg = pci_ecam_create(..., &pci_generic_ecam_ops)
> cfg = kzalloc(...)
> cfg->ops = ops # &pci_generic_ecam_ops
>
> But we're about to merge the ECAM quirks series, which will mean it
> may not be &pci_generic_ecam_ops. Even apart from the ECAM quirks, we
> should avoid this pattern of putting device-specific info in a single
> shared structure because it's too difficult to verify that it's
> correct.
>
Well spotted. I agree, we need to fix this. How about this:
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index fb439c7..31c0e1c 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -152,33 +152,35 @@ static void pci_acpi_generic_release_info(struct
acpi_pci_root_info *ci)
ri = container_of(ci, struct acpi_pci_generic_root_info, common);
pci_ecam_free(ri->cfg);
+ kfree(ci->ops);
kfree(ri);
}
-static struct acpi_pci_root_ops acpi_pci_root_ops = {
- .release_info = pci_acpi_generic_release_info,
-};
-
/* Interface called from ACPI code to setup PCI host controller */
struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
{
int node = acpi_get_node(root->device->handle);
struct acpi_pci_generic_root_info *ri;
struct pci_bus *bus, *child;
+ struct acpi_pci_root_ops *root_ops;
ri = kzalloc_node(sizeof(*ri), GFP_KERNEL, node);
if (!ri)
return NULL;
+ root_ops = kzalloc_node(sizeof(*root_ops), GFP_KERNEL, node);
+ if (!root_ops)
+ return NULL;
+
ri->cfg = pci_acpi_setup_ecam_mapping(root);
if (!ri->cfg) {
kfree(ri);
+ kfree(root_ops);
return NULL;
}
- acpi_pci_root_ops.pci_ops = &ri->cfg->ops->pci_ops;
- bus = acpi_pci_root_create(root, &acpi_pci_root_ops, &ri->common,
- ri->cfg);
+ root_ops->release_info = pci_acpi_generic_release_info;
+ root_ops->pci_ops = &ri->cfg->ops->pci_ops;
+ bus = acpi_pci_root_create(root, root_ops, &ri->common, ri->cfg);
if (!bus)
return NULL;
Of course, this should be the part of ECAM quirks core patches.
The other option we have is to remove "struct pci_ops *pci_ops;" from
acpi_pci_root_ops structure and pass struct pci_ops as an extra argument
to acpi_pci_root_create(). What do you think?
Thanks,
Tomasz
^ permalink raw reply related
* [PATCH] mfd: twl-core: export twl_get_regmap
From: Rosia, Nicolae @ 2016-11-23 11:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161123111223.GG23750@n2100.armlinux.org.uk>
Hello,
On Wed, 2016-11-23 at 11:12 +0000, Russell King - ARM Linux wrote:
> I was just curious, and I took a peek at the OMAP/TWL DT files, and
> I see that it's left to DT to create the children.
I'm converting the driver to use mfd_add_devices and
mfd_remove_devices, the subdrivers will access the parent's private
data which will remain valid since in the remove method we will be
calling mfd_remove_devices first.
After removing all global calls to twl-core.c methods, I will also get
rid of the "only one instance" of twl_priv and the "ready" flag.
Thanks for your input,
Nicolae
^ permalink raw reply
* [PATCH] dt-bindings: mfd: Improve readability for TPS65217 interrupt sources
From: Milo Kim @ 2016-11-23 11:38 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161122155759.GG10134@dell.home>
On 11/23/2016 12:57 AM, Lee Jones wrote:
>> pwrbutton {
>> > - interrupts = <TPS65217_IRQ_PB>;
>> > + interrupts = <TPS65217_IRQ_PUSHBUTTON>;
> Push button or power button?
>
According to the datasheet, push button interrupt is correct.
http://www.ti.com/lit/ds/symlink/tps65217.pdf
This is used for a power button input in Beaglebone boards. In other
words, the power button is one of push button usages.
So, I'd like to keep general name for the interrupt.
Best regards,
Milo
^ permalink raw reply
* Synopsys Ethernet QoS Driver
From: Lars Persson @ 2016-11-23 11:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <7c259adb-5c73-f997-6b96-5be427157b08@synopsys.com>
> 23 nov. 2016 kl. 12:11 skrev Joao Pinto <Joao.Pinto@synopsys.com>:
>
> Hi Peppe and Lars,
>
>> On 23-11-2016 10:59, Giuseppe CAVALLARO wrote:
>> Hello Joao, Lars.
>>
>>> On 11/22/2016 3:16 PM, Joao Pinto wrote:
>>>>> Ok, it makes sense.
>>>>> Just for curiosity the target setup is the following:
>>>>> https://www.youtube.com/watch?v=8V-LB5y2Cos
>>>>> but instead of using internal drivers, we desire to use mainline drivers only.
>>>>>
>>>>> Thanks!
>>> Regarding this subject, I am thinking of making the following adaption:
>>>
>>> a) delete ethernet/synopsys
>>> b) rename ethernet/stmicro/stmmac to ethernet/synopsys
>>>
>>> and send you a patch for you to evaluate. Both agree with the approach?
>>> To have a new work base would be important, because I will add to the "new"
>>> structure some missing QoS features like Multichannel support, CBS and later TSN.
>>
>> IMO, we have to agree on a common strategy making the change for
>> net-next; I imaged the following steps:
>
> Yes it makes totally sense.
>
>>
>> - to port missing feature or fixes from ethernet/synopsys
>> inside the stmmac taking care about the documentation too.
>
> @Lars: You are familiar with the synopsys qos driver. Could you please do this
> porting. You can also make an analysis of what to port and I can do the porting
> for you if you don't have the availability for it.
As my main duty is changing diapers until March next year, please go ahead with this step if you can spend time on it before I am back in office.
Rabin Vincent can review and test that the port works properly on our Artpec-chips that use dwc_eth_qos.c today.
The main porting step is to implement the device tree binding in bindings/net/snps,dwc-qos-ethernet.txt. Also our chip has a strict requirement that the phy is enabled when the SWR reset bit is set (it needs a tx clock to complete the reset).
- Lars
>
>> - remove ethernet/synopsys
>> - rename ethernet/stmicro/stmmac to ethernet/synopsys
>
> I volunteer to do this task.
>
>>
>> These latest two have some relevant impacts.
>>
>> This change should be propagated to all the platforms that are using:
>> CONFIG_SYNOPSYS_DWC_ETH_QOS and CONFIG_STMMAC_ETH
>> plus device-tree compatibility.
>
> I volunteer to do this task also.
>
>>
>> - enhance the stmmac with new features and new glue (part of these
>> can be anticipated for sure).
>
> I have to implement 3 new features for now, but I will take some time for it, so
> I would suggest to make the previous task and incrementally add features.
>
>>
>> what do you think? does it make sense? If yes, we can also
>> understand how/who starts.
>>
>> Regards,
>> Peppe
>
> Thanks and regards.
>
> Joao
>
>>
>>> Thanks.
>>
>
^ permalink raw reply
* [GIT PULL] Second Round of Renesas ARM Based SoC Drivers Updates for v4.10
From: Arnd Bergmann @ 2016-11-23 11:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161119013551.GB2543@localhost>
On Friday, November 18, 2016 5:35:51 PM CET Olof Johansson wrote:
>
> So, this pull request contains 8 patches, not 4. Seems like your pull
> request doesn't show any of the code from Geert's branch, didn't mention
> it in the tag and only in the email text above. Furthermore, Geert's
> branch modifies driver core code, so it's extra important to make sure
> it's clear that it's an unusual pull request.
>
> Given that this modifies driver core, please either merge that code
> through Greg first, or get an ack from him. If you merge through him,
> make sure it's on a standalone topic branch that we can share.
As discussed on IRC, I think it makes sense to track that branch
separately in arm-soc, as its own top-level next/* branch.
The branch was first merged into the mmc tree after we got consensus
on the approach.
Arnd
^ permalink raw reply
* Synopsys Ethernet QoS Driver
From: Joao Pinto @ 2016-11-23 11:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <899DC02E-84BB-489E-A1FE-5D8F3BB795B6@axis.com>
On 23-11-2016 11:41, Lars Persson wrote:
>
>> 23 nov. 2016 kl. 12:11 skrev Joao Pinto <Joao.Pinto@synopsys.com>:
>>
>> Hi Peppe and Lars,
>>
>>> On 23-11-2016 10:59, Giuseppe CAVALLARO wrote:
>>> Hello Joao, Lars.
>>>
>>>> On 11/22/2016 3:16 PM, Joao Pinto wrote:
>>>>>> Ok, it makes sense.
>>>>>> Just for curiosity the target setup is the following:
>>>>>> https://www.youtube.com/watch?v=8V-LB5y2Cos
>>>>>> but instead of using internal drivers, we desire to use mainline drivers only.
>>>>>>
>>>>>> Thanks!
>>>> Regarding this subject, I am thinking of making the following adaption:
>>>>
>>>> a) delete ethernet/synopsys
>>>> b) rename ethernet/stmicro/stmmac to ethernet/synopsys
>>>>
>>>> and send you a patch for you to evaluate. Both agree with the approach?
>>>> To have a new work base would be important, because I will add to the "new"
>>>> structure some missing QoS features like Multichannel support, CBS and later TSN.
>>>
>>> IMO, we have to agree on a common strategy making the change for
>>> net-next; I imaged the following steps:
>>
>> Yes it makes totally sense.
>>
>>>
>>> - to port missing feature or fixes from ethernet/synopsys
>>> inside the stmmac taking care about the documentation too.
>>
>> @Lars: You are familiar with the synopsys qos driver. Could you please do this
>> porting. You can also make an analysis of what to port and I can do the porting
>> for you if you don't have the availability for it.
>
> As my main duty is changing diapers until March next year, please go ahead with this step if you can spend time on it before I am back in office.
Congratulations :)!
>
> Rabin Vincent can review and test that the port works properly on our Artpec-chips that use dwc_eth_qos.c today.
>
> The main porting step is to implement the device tree binding in bindings/net/snps,dwc-qos-ethernet.txt. Also our chip has a strict requirement that the phy is enabled when the SWR reset bit is set (it needs a tx clock to complete the reset).
>
> - Lars
Ok, I will do the task.
@Peppe: Agree with the plan?
>
>>
>>> - remove ethernet/synopsys
>>> - rename ethernet/stmicro/stmmac to ethernet/synopsys
>>
>> I volunteer to do this task.
>>
>>>
>>> These latest two have some relevant impacts.
>>>
>>> This change should be propagated to all the platforms that are using:
>>> CONFIG_SYNOPSYS_DWC_ETH_QOS and CONFIG_STMMAC_ETH
>>> plus device-tree compatibility.
>>
>> I volunteer to do this task also.
>>
>>>
>>> - enhance the stmmac with new features and new glue (part of these
>>> can be anticipated for sure).
>>
>> I have to implement 3 new features for now, but I will take some time for it, so
>> I would suggest to make the previous task and incrementally add features.
>>
>>>
>>> what do you think? does it make sense? If yes, we can also
>>> understand how/who starts.
>>>
>>> Regards,
>>> Peppe
>>
>> Thanks and regards.
>>
>> Joao
>>
>>>
>>>> Thanks.
>>>
>>
^ permalink raw reply
* [PATCH 1/3] of: base: add support to get machine compatible string
From: Sekhar Nori @ 2016-11-23 11:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <2a644b8c-d91e-5ab1-200b-00f749a36307@arm.com>
On Wednesday 23 November 2016 03:35 PM, Sudeep Holla wrote:
>
>
> On 23/11/16 07:49, Sekhar Nori wrote:
>> On Tuesday 22 November 2016 09:16 PM, Sudeep Holla wrote:
>>> Hi Sekhar,
>>>
>>> On 22/11/16 15:06, Sekhar Nori wrote:
>>>> Hi Sudeep,
>>>>
>>>> On Tuesday 22 November 2016 04:23 PM, Sudeep Holla wrote:
>>>>>
>>>>>
>>>>> On 22/11/16 10:41, Bartosz Golaszewski wrote:
>>>>>> Add a function allowing to retrieve the compatible string of the root
>>>>>> node of the device tree.
>>>>>>
>>>>>
>>>>> Rob has queued [1] and it's in -next today. You can reuse that if you
>>>>> are planning to target this for v4.11 or just use open coding in your
>>>>> driver for v4.10 and target this move for v4.11 to avoid cross tree
>>>>> dependencies as I already mentioned in your previous thread.
>>>>
>>>> I dont have your original patch in my mailbox, but I wonder if
>>>> returning a pointer to property string for a node whose reference has
>>>> already been released is safe to do? Probably not an issue for the root
>>>> node, but still feels counter-intuitive.
>>>>
>>>
>>> I am not sure if I understand the issue here. Are you referring a case
>>> where of_root is freed ?
>>
>> Yes, right, thats what I was hinting at. Since you are giving up the
>> reference to the device node before the function returns, the user can
>> be left with a dangling reference.
>>
>
> Yes I agree.
So, the if(!of_node_get()) is just an expensive NULL pointer check. I think
it is better to be explicit about it by not using of_node_get/put() at all.
How about:
+int of_machine_get_model_name(const char **model)
+{
+ int error;
+
+ if (!of_root)
+ return -EINVAL;
+
+ error = of_property_read_string(of_root, "model", model);
+ if (error)
+ error = of_property_read_string_index(of_root, "compatible",
+ 0, model);
+ return error;
+}
+EXPORT_SYMBOL(of_machine_get_model_name);
I know the patch is already in -next so I guess it depends on how strongly
Rob feels about this.
Thanks,
Sekhar
^ permalink raw reply
* [PATCH] dt-bindings: mfd: Improve readability for TPS65217 interrupt sources
From: Arnd Bergmann @ 2016-11-23 11:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <bd4944d8-3cef-b3f7-4fba-6de590493047@gmail.com>
On Wednesday, November 23, 2016 8:38:00 PM CET Milo Kim wrote:
> On 11/23/2016 12:57 AM, Lee Jones wrote:
> >> pwrbutton {
> >> > - interrupts = <TPS65217_IRQ_PB>;
> >> > + interrupts = <TPS65217_IRQ_PUSHBUTTON>;
> > Push button or power button?
> >
>
> According to the datasheet, push button interrupt is correct.
>
> http://www.ti.com/lit/ds/symlink/tps65217.pdf
>
> This is used for a power button input in Beaglebone boards. In other
> words, the power button is one of push button usages.
>
> So, I'd like to keep general name for the interrupt.
Ah, the numbers come from the data sheet. Please just remove the
header then, there is no need to keep them as an ABI, in particular
when the driver doesn't even include that header today.
I see you even have names for them in the node:
charger {
- interrupts = <TPS65217_IRQ_AC>, <TPS65217_IRQ_USB>;
+ interrupts = <TPS65217_IRQ_AC_POWER>, <TPS65217_IRQ_USB_POWER>;
interrupts-names = "AC", "USB";
status = "okay";
What matters here is the binding documentation in
Documentation/devicetree/bindings/power/supply/tps65217_charger.txt
Please fix that instead to mandate that the two interrupts are
required, what their functions are, and what the interrupt-names
(not interrupts-names) property must list.
Arnd
^ permalink raw reply
* [PATCH v16 13/15] acpi/arm64: Add memory-mapped timer support in GTDT driver
From: Fu Wei @ 2016-11-23 11:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161118142249.GA18029@red-moon>
Hi Lorenzo,
On 18 November 2016 at 22:22, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Wed, Nov 16, 2016 at 09:49:06PM +0800, fu.wei at linaro.org wrote:
>> From: Fu Wei <fu.wei@linaro.org>
>>
>> On platforms booting with ACPI, architected memory-mapped timers'
>> configuration data is provided by firmware through the ACPI GTDT
>> static table.
>>
>> The clocksource architected timer kernel driver requires a firmware
>> interface to collect timer configuration and configure its driver.
>> this infrastructure is present for device tree systems, but it is
>> missing on systems booting with ACPI.
>>
>> Implement the kernel infrastructure required to parse the static
>> ACPI GTDT table so that the architected timer clocksource driver can
>> make use of it on systems booting with ACPI, therefore enabling
>> the corresponding timers configuration.
>>
>> Signed-off-by: Fu Wei <fu.wei@linaro.org>
>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> ---
>> drivers/acpi/arm64/gtdt.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/acpi.h | 1 +
>> 2 files changed, 96 insertions(+)
>>
>> diff --git a/drivers/acpi/arm64/gtdt.c b/drivers/acpi/arm64/gtdt.c
>> index 2de79aa..08d9506 100644
>> --- a/drivers/acpi/arm64/gtdt.c
>> +++ b/drivers/acpi/arm64/gtdt.c
>> @@ -51,6 +51,14 @@ static inline bool is_timer_block(void *platform_timer)
>> return gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK;
>> }
>>
>> +static inline struct acpi_gtdt_timer_block *get_timer_block(unsigned int index)
>> +{
>> + if (index >= acpi_gtdt_desc.timer_block_count || !timer_block)
>> + return NULL;
>> +
>> + return timer_block[index];
>> +}
>> +
>> static inline bool is_watchdog(void *platform_timer)
>> {
>> struct acpi_gtdt_header *gh = platform_timer;
>> @@ -214,3 +222,90 @@ int __init acpi_gtdt_init(struct acpi_table_header *table)
>> acpi_gtdt_release();
>> return -EINVAL;
>> }
>> +
>> +/*
>> + * Get ONE GT block info for memory-mapped timer from GTDT table.
>> + * @data: the GT block data (parsing result)
>> + * @index: the index number of GT block
>> + * Note: we already verify @data in caller, it can't be NULL here.
>> + * Returns 0 if success, -EINVAL/-ENODEV if error.
>> + */
>
> Documentation/kernel-doc-nano-HOWTO.txt
Great thanks , will fix all the comments :-)
>
>> +int __init gtdt_arch_timer_mem_init(struct arch_timer_mem *data,
>> + unsigned int index)
>
> Nit: acpi_arch_timer_mem_init() to make it consistent with other ACPI
> calls ?
yes, it makes sense, will do this way for the function which is exported
>
>> +{
>> + struct acpi_gtdt_timer_block *block;
>> + struct acpi_gtdt_timer_entry *frame;
>> + int i;
>> +
>> + block = get_timer_block(index);
>> + if (!block)
>> + return -ENODEV;
>> +
>> + if (!block->timer_count) {
>> + pr_err(FW_BUG "GT block present, but frame count is zero.");
>> + return -ENODEV;
>> + }
>> +
>> + if (block->timer_count > ARCH_TIMER_MEM_MAX_FRAMES) {
>> + pr_err(FW_BUG "GT block lists %d frames, ACPI spec only allows 8\n",
>> + block->timer_count);
>> + return -EINVAL;
>> + }
>
> Nit: these two checks can be carried out at GTDT init where the GTDT
> is parsed first. Actually you could have two functions one to init
this check is the verify the data in GT block, but the GTDT init
won't get into the block,
so I think that is better to keep this in GT block init function
"acpi_arch_timer_mem_init()"
> timers (say acpi_gtdt_timers_init()) and one watchdogs, that would
> eliminate the duplicated timer_block/watchdog arrays (both sized
> Platform Timer Count) and acpi_gtdt_timers_init() can return
> the number of entries found so that you can loop with a determined
> upper bound in the arm arch timer driver.
Yes, I did this way in previous patchset, but we still need to do scan
loop in both functions for
two types of platform timer.
So I decide to keep the scan loop in the acpi_gtdt_init to get the
number and the entries pointer
of each type of platform timers in one go. then we don't need to scan
GTDT in two functions.
>
> Just thinking aloud, these are just improvements I can carry them
> out later, the more important question here is the interface between the
> parsing code and the arch timer probing code which depends on other
> patches and that needs to be agreed, this patch is not really a problem.
yes, every time I modify the interface I have to change the driver :-(
>
>> + data->cntctlbase = (phys_addr_t)block->block_address;
>> + /*
>> + * We can NOT get the size info from GTDT table,
>> + * but according to "Table * CNTCTLBase memory map" of
>> + * <ARM Architecture Reference Manual> for ARMv8,
>> + * it should be 4KB(Offset 0x000 ? 0xFFC).
>
> That's the reason why it is not explicit in the GTDT table :)
>
>> + data->size = SZ_4K;
>> + data->num_frames = block->timer_count;
>> +
>> + frame = (void *)block + block->timer_offset;
>> + if (frame + block->timer_count != (void *)block + block->header.length)
>> + return -EINVAL;
>> +
>> + /*
>> + * Get the GT timer Frame data for every GT Block Timer
>> + */
>> + for (i = 0; i < block->timer_count; i++, frame++) {
>> + if (!frame->base_address || !frame->timer_interrupt)
>> + return -EINVAL;
>> +
>> + data->frame[i].phys_irq = map_gt_gsi(frame->timer_interrupt,
>> + frame->timer_flags);
>> + if (data->frame[i].phys_irq <= 0) {
>> + pr_warn("failed to map physical timer irq in frame %d.\n",
>> + i);
>> + return -EINVAL;
>> + }
>> +
>> + if (frame->virtual_timer_interrupt) {
>> + data->frame[i].virt_irq =
>> + map_gt_gsi(frame->virtual_timer_interrupt,
>> + frame->virtual_timer_flags);
>> + if (data->frame[i].virt_irq <= 0) {
>> + pr_warn("failed to map virtual timer irq in frame %d.\n",
>> + i);
>> + return -EINVAL;
>
> You should unregister phys_irq here for correctness, right ?
yup, thanks for pointing this bug out. :-)
>
>> + }
>> + }
>> +
>> + data->frame[i].frame_nr = frame->frame_number;
>> + data->frame[i].cntbase = frame->base_address;
>> + /*
>> + * We can NOT get the size info from GTDT table,
>> + * but according to "Table * CNTBaseN memory map" of
>> + * <ARM Architecture Reference Manual> for ARMv8,
>> + * it should be 4KB(Offset 0x000 ? 0xFFC).
>
> See above.
yes, you are right, I will fix both comments
>
>> + */
>> + data->frame[i].size = SZ_4K;
>> + }
>> +
>> + if (acpi_gtdt_desc.timer_block_count)
>> + pr_info("parsed No.%d of %d memory-mapped timer block(s).\n",
>> + index, acpi_gtdt_desc.timer_block_count);
>
> I am not sure how helpful this message can be honestly.
yup, I will simplify it :-)
>
>> +
>> + return 0;
>> +}
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index a1611d1..44b8c1b 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -582,6 +582,7 @@ int acpi_gtdt_init(struct acpi_table_header *table);
>> int acpi_gtdt_map_ppi(int type);
>> bool acpi_gtdt_c3stop(int type);
>> void acpi_gtdt_release(void);
>> +int gtdt_arch_timer_mem_init(struct arch_timer_mem *data, unsigned int index);
>
> See above.
>
> Overall it looks fine as long as the interface with clocksource is
> settled, which is what really needs to be agreed upon in this series.
Thanks for your help :-)
>
> Lorenzo
>
>> #endif
>>
>> #else /* !CONFIG_ACPI */
>> --
>> 2.7.4
>>
--
Best regards,
Fu Wei
Software Engineer
Red Hat
^ permalink raw reply
* [PATCH v16 11/15] acpi/arm64: Add GTDT table parse driver
From: Fu Wei @ 2016-11-23 12:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161118201250.GO1197@leverpostej>
Hi Mark,
On 19 November 2016 at 04:12, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Nov 16, 2016 at 09:49:04PM +0800, fu.wei at linaro.org wrote:
>
>> +#define for_each_platform_timer(_g) for (; _g; _g = next_platform_timer(_g))
>
> This doesn't fit the usual for_each_* pattern, since _g has to be
> manually initialised first. Either come up with a way of maknig this fit
> the usual pattern, or get rid of this, and use:
>
> t = however_you_get_the_first_timer();
>
> if (!t)
> bailt_out_somehow();
>
> do {
> ...
> } while (t = next_platform_timer(t));
Thanks, will do
>
>> +/*
>> + * Release the memory we have allocated in acpi_gtdt_init.
>> + * This should be called, when the driver who called "acpi_gtdt_init" previously
>> + * doesn't need the GTDT info anymore.
>> + */
>> +void __init acpi_gtdt_release(void)
>> +{
>> + kfree(timer_block);
>> + kfree(watchdog);
>> + timer_block = NULL;
>> + watchdog = NULL;
>> +}
>
> Why is this not simply in the error path of acpi_gtdt_init()?
>
>> +
>> +/*
>> + * Get some basic info from GTDT table, and init the global variables above
>> + * for all timers initialization of Generic Timer.
>> + * This function does some validation on GTDT table.
>> + */
>> +int __init acpi_gtdt_init(struct acpi_table_header *table)
>> +{
>
>> + timer_block = kcalloc(timer_count,
>> + sizeof(struct acpi_gtdt_timer_block *),
>> + GFP_KERNEL);
>> + if (!timer_block)
>> + return -ENOMEM;
>> +
>> + watchdog = kcalloc(timer_count, sizeof(struct acpi_gtdt_watchdog *),
>> + GFP_KERNEL);
>> + if (!watchdog) {
>> + kfree(timer_block);
>> + timer_block = NULL;
>> + return -ENOMEM;
>> + }
>
> Please have a common error path below, and branch to that when you need
> to free these.
OK , will do
>
>> +error:
>> + acpi_gtdt_release();
>> + return -EINVAL;
>> +}
>
> [...]
>
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index 61a3d90..a1611d1 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -577,6 +577,13 @@ enum acpi_reconfig_event {
>> int acpi_reconfig_notifier_register(struct notifier_block *nb);
>> int acpi_reconfig_notifier_unregister(struct notifier_block *nb);
>>
>> +#ifdef CONFIG_ACPI_GTDT
>> +int acpi_gtdt_init(struct acpi_table_header *table);
>> +int acpi_gtdt_map_ppi(int type);
>> +bool acpi_gtdt_c3stop(int type);
>> +void acpi_gtdt_release(void);
>
> Why do these need to be here?
>
> What possible value is ther in exporting acpi_gtdt_release() !?
because I need to release these after mem timer init,
But like your comment in 14/15, if I fill in the entire arch_timer_mem
in one go, I don't need to export acpi_gtdt_release.
Will try this way in v17
>
> Thanks,
> Mark.
--
Best regards,
Fu Wei
Software Engineer
Red Hat
^ permalink raw reply
* [PATCH v2 1/3] ARM: davinci: da8xx: Fix ohci device name
From: Sekhar Nori @ 2016-11-23 12:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161103160308.29588-2-ahaslam@baylibre.com>
On Thursday 03 November 2016 09:33 PM, Axel Haslam wrote:
> While the clk lookup table is making reference to "ohci"
> other subsystems (such as phy) are trying to match "ohci.0"
>
> Since there is a single ohci instance, instead of changing
> the clk name, change the dev id to -1, and add the "-da8xx"
> postfix to match the driver name that will also be changed
> in a subsequent patch.
>
> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
Applied to v4.10/soc
Thanks,
Sekhar
^ permalink raw reply
* [PATCH 1/3] of: base: add support to get machine compatible string
From: Sudeep Holla @ 2016-11-23 12:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <ff1ea88a-87c6-6721-8e63-ddb05900cc58@ti.com>
On 23/11/16 11:47, Sekhar Nori wrote:
> On Wednesday 23 November 2016 03:35 PM, Sudeep Holla wrote:
>>
>>
>> On 23/11/16 07:49, Sekhar Nori wrote:
>>> On Tuesday 22 November 2016 09:16 PM, Sudeep Holla wrote:
>>>> Hi Sekhar,
>>>>
>>>> On 22/11/16 15:06, Sekhar Nori wrote:
>>>>> Hi Sudeep,
>>>>>
>>>>> On Tuesday 22 November 2016 04:23 PM, Sudeep Holla wrote:
>>>>>>
>>>>>>
>>>>>> On 22/11/16 10:41, Bartosz Golaszewski wrote:
>>>>>>> Add a function allowing to retrieve the compatible string of the root
>>>>>>> node of the device tree.
>>>>>>>
>>>>>>
>>>>>> Rob has queued [1] and it's in -next today. You can reuse that if you
>>>>>> are planning to target this for v4.11 or just use open coding in your
>>>>>> driver for v4.10 and target this move for v4.11 to avoid cross tree
>>>>>> dependencies as I already mentioned in your previous thread.
>>>>>
>>>>> I dont have your original patch in my mailbox, but I wonder if
>>>>> returning a pointer to property string for a node whose reference has
>>>>> already been released is safe to do? Probably not an issue for the root
>>>>> node, but still feels counter-intuitive.
>>>>>
>>>>
>>>> I am not sure if I understand the issue here. Are you referring a case
>>>> where of_root is freed ?
>>>
>>> Yes, right, thats what I was hinting at. Since you are giving up the
>>> reference to the device node before the function returns, the user can
>>> be left with a dangling reference.
>>>
>>
>> Yes I agree.
>
> So, the if(!of_node_get()) is just an expensive NULL pointer check. I think
> it is better to be explicit about it by not using of_node_get/put() at all.
> How about:
>
Are we planning to use this in any time sensitive paths? Anyways I am
fine removing them.
> +int of_machine_get_model_name(const char **model)
> +{
> + int error;
> +
> + if (!of_root)
> + return -EINVAL;
> +
> + error = of_property_read_string(of_root, "model", model);
> + if (error)
> + error = of_property_read_string_index(of_root, "compatible",
> + 0, model);
> + return error;
> +}
> +EXPORT_SYMBOL(of_machine_get_model_name);
>
> I know the patch is already in -next so I guess it depends on how strongly
> Rob feels about this.
Frank expressed his concerns and it may be reverted.
--
Regards,
Sudeep
^ permalink raw reply
* [PATCH 1/3] of: base: add support to get machine compatible string
From: Sekhar Nori @ 2016-11-23 12:13 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <f0ba1a7a-8948-21c3-8a96-d46bf105bb7e@arm.com>
On Wednesday 23 November 2016 05:37 PM, Sudeep Holla wrote:
>> So, the if(!of_node_get()) is just an expensive NULL pointer check. I
>> think
>> it is better to be explicit about it by not using of_node_get/put() at
>> all.
>> How about:
>>
>
> Are we planning to use this in any time sensitive paths? Anyways I am
> fine removing them.
Not worried about the time taken as much as it serving as a bad example
and getting carried over to other places where the impact might actually
be real.
>
>> +int of_machine_get_model_name(const char **model)
>> +{
>> + int error;
>> +
>> + if (!of_root)
>> + return -EINVAL;
>> +
>> + error = of_property_read_string(of_root, "model", model);
>> + if (error)
>> + error = of_property_read_string_index(of_root,
>> "compatible",
>> + 0, model);
>> + return error;
>> +}
>> +EXPORT_SYMBOL(of_machine_get_model_name);
>>
>> I know the patch is already in -next so I guess it depends on how
>> strongly
>> Rob feels about this.
>
> Frank expressed his concerns and it may be reverted.
Didn't notice that. I will check that thread. Thanks!
Regards,
Sekhar
^ permalink raw reply
* Tearing down DMA transfer setup after DMA client has finished
From: Måns Rullgård @ 2016-11-23 12:13 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <58356EA8.2010806@free.fr>
Mason <slash.tmp@free.fr> writes:
> Hello,
>
> On my platform, setting up a DMA transfer is a two-step process:
>
> 1) configure the "switch box" to connect a device to a memory channel
> 2) configure the transfer details (address, size, command)
>
> When the transfer is done, the sbox setup can be torn down,
> and the DMA driver can start another transfer.
>
> The current software architecture for my NFC (NAND Flash controller)
> driver is as follows (for one DMA transfer).
>
> sg_init_one
> dma_map_sg
> dmaengine_prep_slave_sg
> dmaengine_submit
> dma_async_issue_pending
> configure_NFC_transfer
> wait_for_IRQ_from_DMA_engine // via DMA_PREP_INTERRUPT
> wait_for_NFC_idle
> dma_unmap_sg
>
> The problem is that the DMA driver tears down the sbox setup
> as soon as it receives the IRQ. However, when writing to the
> device, the interrupt only means "I have pushed all data from
> memory to the memory channel". These data have not reached
> the device yet, and may still be "in flight". Thus the sbox
> setup can only be torn down after the NFC is idle.
>
> How do I call back into the DMA driver after wait_for_NFC_idle,
> to request sbox tear down?
>
> The new architecture would become:
>
> sg_init_one
> dma_map_sg
> dmaengine_prep_slave_sg
> dmaengine_submit
> dma_async_issue_pending
> configure_NFC_transfer
> wait_for_IRQ_from_DMA_engine // via DMA_PREP_INTERRUPT
> wait_for_NFC_idle
> request_sbox_tear_down /*** HOW TO DO THAT ***/
> dma_unmap_sg
>
> As far as I can tell, my NFC driver should call dmaengine_synchronize ??
> (In other words request_sbox_tear_down == dmaengine_synchronize)
>
> So the DMA driver should implement the device_synchronize hook,
> and tear the sbox down in that function.
>
> Is that correct? Or am I on the wrong track?
dmaengine_synchronize() is not meant for this. See the documentation:
/**
* dmaengine_synchronize() - Synchronize DMA channel termination
* @chan: The channel to synchronize
*
* Synchronizes to the DMA channel termination to the current context. When this
* function returns it is guaranteed that all transfers for previously issued
* descriptors have stopped and and it is safe to free the memory assoicated
* with them. Furthermore it is guaranteed that all complete callback functions
* for a previously submitted descriptor have finished running and it is safe to
* free resources accessed from within the complete callbacks.
*
* The behavior of this function is undefined if dma_async_issue_pending() has
* been called between dmaengine_terminate_async() and this function.
*
* This function must only be called from non-atomic context and must not be
* called from within a complete callback of a descriptor submitted on the same
* channel.
*/
This is for use after a dmaengine_terminate_async() call to wait for the
dma engine to finish whatever it was doing. This is not the problem
here. Your problem is that the dma engine interrupt fires before the
transfer is actually complete. Although you get an indication from the
target device when it has received all the data, there is no way to make
the dma driver wait for this.
--
M?ns Rullg?rd
^ 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