* Re: [v6 PATCH] arm64: mm: show direct mapping use in /proc/meminfo
From: Yang Shi @ 2026-05-14 0:02 UTC (permalink / raw)
To: Will Deacon
Cc: catalin.marinas, ryan.roberts, cl, linux-arm-kernel, linux-kernel
In-Reply-To: <agNFou9cAavEILXl@willie-the-truck>
On 5/12/26 8:22 AM, Will Deacon wrote:
> On Mon, Mar 16, 2026 at 12:29:33PM -0700, Yang Shi wrote:
>> Since commit a166563e7ec3 ("arm64: mm: support large block mapping when
>> rodata=full"), the direct mapping may be split on some machines instead
>> keeping static since boot. It makes more sense to show the direct mapping
>> use in /proc/meminfo than before.
>> This patch will make /proc/meminfo show the direct mapping use like the
>> below (4K base page size):
>> DirectMap4K: 94792 kB
>> DirectMap64K: 134208 kB
>> DirectMap2M: 1173504 kB
>> DirectMap32M: 5636096 kB
>> DirectMap1G: 529530880 kB
>>
>> Although just the machines which support BBML2_NOABORT can split the
>> direct mapping, show it on all machines regardless of BBML2_NOABORT so
>> that the users have consistent view in order to avoid confusion.
>>
>> Although ptdump also can tell the direct map use, but it needs to dump
>> the whole kernel page table. It is costly and overkilling. It is also
>> in debugfs which may not be enabled by all distros. So showing direct
>> map use in /proc/meminfo seems more convenient and has less overhead.
>>
>> Signed-off-by: Yang Shi <yang@os.amperecomputing.com>
>> ---
>> v6: * Rebased to v7.0-rc3
>> * Rebased on top of Anshuman's v5 "arm64/mm: Enable batched TLB flush
>> in unmap_hotplug_range()"
>> * Used const for direct map type array per Will
>> * Defined PUD size for 16K/64K even though it is not used per Will
>> * Removed the misleading comment in init_pmd() per Will
>> v5: * Rebased to v6.19-rc4
>> * Fixed the build error for !CONFIG_PROC_FS
>> v4: * Used PAGE_END instead of _PAGE_END(VA_BITS_MIN) per Ryan
>> * Used shorter name for the helpers and variables per Ryan
>> * Fixed accounting for memory hotunplug
>> v3: * Fixed the over-accounting problems per Ryan
>> * Introduced helpers for add/sub direct map use and #ifdef them with
>> CONFIG_PROC_FS per Ryan
>> * v3 is a fix patch on top of v2
>> v2: * Counted in size instead of the number of entries per Ryan
>> * Removed shift array per Ryan
>> * Use lower case "k" per Ryan
>> * Fixed a couple of build warnings reported by kernel test robot
>> * Fixed a couple of poential miscounts
>>
>> arch/arm64/mm/mmu.c | 197 +++++++++++++++++++++++++++++++++++++++-----
>> 1 file changed, 176 insertions(+), 21 deletions(-)
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index 5fb9a66f0754..7e95dbc69d57 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -29,6 +29,7 @@
>> #include <linux/mm_inline.h>
>> #include <linux/pagewalk.h>
>> #include <linux/stop_machine.h>
>> +#include <linux/proc_fs.h>
>>
>> #include <asm/barrier.h>
>> #include <asm/cputype.h>
>> @@ -171,6 +172,87 @@ static void init_clear_pgtable(void *table)
>> dsb(ishst);
>> }
>>
>> +enum dm_type {
>> + PTE,
>> + CONT_PTE,
>> + PMD,
>> + CONT_PMD,
>> + PUD,
>> + NR_DM_TYPE,
>> +};
>> +
>> +#ifdef CONFIG_PROC_FS
>> +static unsigned long dm_meminfo[NR_DM_TYPE];
>> +
>> +void arch_report_meminfo(struct seq_file *m)
>> +{
>> + const char *size[NR_DM_TYPE];
>> +
>> +#if defined(CONFIG_ARM64_4K_PAGES)
>> + size[PTE] = "4k";
>> + size[CONT_PTE] = "64k";
>> + size[PMD] = "2M";
>> + size[CONT_PMD] = "32M";
>> + size[PUD] = "1G";
>> +#elif defined(CONFIG_ARM64_16K_PAGES)
>> + size[PTE] = "16k";
>> + size[CONT_PTE] = "2M";
>> + size[PMD] = "32M";
>> + size[CONT_PMD] = "1G";
>> + size[PUD] = "64G";
>> +#elif defined(CONFIG_ARM64_64K_PAGES)
>> + size[PTE] = "64k";
>> + size[CONT_PTE] = "2M";
>> + size[PMD] = "512M";
>> + size[CONT_PMD] = "16G";
>> + size[PUD] = "4T";
>> +#endif
>> +
>> + seq_printf(m, "DirectMap%s: %8lu kB\n",
>> + size[PTE], dm_meminfo[PTE] >> 10);
>> + seq_printf(m, "DirectMap%s: %8lu kB\n",
>> + size[CONT_PTE],
>> + dm_meminfo[CONT_PTE] >> 10);
>> + seq_printf(m, "DirectMap%s: %8lu kB\n",
>> + size[PMD], dm_meminfo[PMD] >> 10);
>> + seq_printf(m, "DirectMap%s: %8lu kB\n",
>> + size[CONT_PMD],
>> + dm_meminfo[CONT_PMD] >> 10);
>> + if (pud_sect_supported())
>> + seq_printf(m, "DirectMap%s: %8lu kB\n",
>> + size[PUD], dm_meminfo[PUD] >> 10);
>> +}
>> +
>> +static inline bool is_dm_addr(unsigned long addr)
>> +{
>> + return (addr >= PAGE_OFFSET) && (addr < PAGE_END);
>> +}
> Just use __is_lm_address()?
Yeah, good idea.
>
> For better or worse, the arm64 arch code tends to talk about the "linear
> map" rather than the "direct map", so a little bit of renaming would be
> good (i.e. s/dm/lm/). I'm fine if you want to keep the user-visible
> strings as "DirectMap".
Sure, I will rename the internal APIs to "lm" to keep the consistency. I
think we'd better to keep the user-visible strings as "DirectMap".
Thanks,
Yang
>
> Will
^ permalink raw reply
* Re: [PATCH v3 02/13] driver core: Enable suppliers to implement fine grained sync_state support
From: Saravana Kannan @ 2026-05-14 0:04 UTC (permalink / raw)
To: Ulf Hansson
Cc: Saravana Kannan, Danilo Krummrich, Rafael J . Wysocki,
Greg Kroah-Hartman, driver-core, linux-pm, Sudeep Holla,
Cristian Marussi, Kevin Hilman, Stephen Boyd, Marek Szyprowski,
Bjorn Andersson, Abel Vesa, Peng Fan, Tomi Valkeinen, Maulik Shah,
Konrad Dybcio, Thierry Reding, Jonathan Hunter,
Geert Uytterhoeven, Dmitry Baryshkov, linux-arm-kernel,
linux-kernel
In-Reply-To: <CAPDyKFpYkzmmx+d2-x0YskO0H2UD2v1LmZLMRqMmDoLSfPk8Xg@mail.gmail.com>
On Wed, May 13, 2026 at 2:05 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Wed, 13 May 2026 at 07:01, Saravana Kannan <saravanak@kernel.org> wrote:
> >
> > On Mon, May 11, 2026 at 2:43 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Mon, 11 May 2026 at 07:09, Saravana Kannan <saravanak@kernel.org> wrote:
> > > >
> > > > On Fri, May 8, 2026 at 5:39 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > >
> > > > > The common sync_state support isn't fine grained enough for some types of
> > > > > suppliers, like power domains for example. Especially when a supplier
> > > > > provides multiple independent power domains, each with their own set of
> > > > > consumers. In these cases we need to wait for all consumers for all the
> > > > > provided power domains before invoking the supplier's ->sync_state().
> > > > >
> > > > > To allow a more fine grained sync_state support to be implemented on per
> > > > > supplier's driver basis, let's add a new optional callback. As soon as
> > > > > there is an update worth to consider in regards to managing sync_state for
> > > > > a supplier device, __device_links_queue_sync_state() queues the device in a
> > > > > list, allowing the new callback to be invoked when flushing the list in
> > > > > device_links_flush_sync_list().
> > > > >
> > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > ---
> > > > >
> > > > > Changes in v3:
> > > > > - Re-worked the approach to use a list to queue/flush devices for
> > > > > ->queue_sync_state(). This should make sure the device lock is being
> > > > > held when it's needed, as pointed out by Danilo.
> > > > >
> > > >
> > > > Hi Ulf,
> > > >
> > > > Thanks for working on this!
> > > >
> > > > And please bear with my slow replies.
> > >
> > > I will try, but taking more than 2 months to reply isn't sustainable,
> > > I think. Let's hope you can get some more bandwidth for reviews when
> > > moving forward.
> > >
> > > >
> > > > > ---
> > > > > drivers/base/base.h | 18 ++++++++
> > > > > drivers/base/core.c | 77 ++++++++++++++++++++++++++---------
> > > > > drivers/base/driver.c | 7 ++++
> > > > > include/linux/device.h | 2 +
> > > > > include/linux/device/driver.h | 7 ++++
> > > > > 5 files changed, 91 insertions(+), 20 deletions(-)
> > > > >
> > > > > diff --git a/drivers/base/base.h b/drivers/base/base.h
> > > > > index 30b416588617..c8be24af92c3 100644
> > > > > --- a/drivers/base/base.h
> > > > > +++ b/drivers/base/base.h
> > > > > @@ -196,6 +196,24 @@ static inline void dev_sync_state(struct device *dev)
> > > > > dev->driver->sync_state(dev);
> > > > > }
> > > > >
> > > > > +static inline bool dev_has_queue_sync_state(struct device *dev)
> > > >
> > > > Let's please pick a better name. This is too similar to the actual
> > > > queue function you call __device_links_queue_sync_state() and is very
> > > > confusing. Maybe something that has a meaning along the lines of
> > > > "another consumer probed". So, maybe:
> > > > * consumer_probed()
> > > > * change_of_active_consumers()
> > >
> > > The whole point of naming it "queue_sync_state" was exactly to refer
> > > to __device_links_queue_sync_state(). The point is, the callback can't
> > > be invoked unless __device_links_queue_sync_state() has been called
> > > for the device first.
> > >
> > > Not sure why you think that is confusing? To me, that is rather the
> > > opposite. :-)
> >
> >
> > sync_state() is a callback telling the driver that it's okay to sync
> > the state of the hardware with the software.
> >
> > queue_sync_state() means nothing to the driver. It's just leaking an
> > internal implementation detail (name of the internal function) to the
> > driver that's not really relevant. What you are really telling the
> > driver is that some additional consumers have probed.
> >
> > > Before deciding on another name, note also that
> > > __device_links_queue_sync_state() is called when resuming sync_state
> > > from device_links_supplier_sync_state_resume() and from
> > > device_links_driver_bound(). I am not sure "consumer_probed" a good
> > > name that covers both of these cases; what do you think?
> >
> > Yes, I'm well aware of that :) The contract of sync_state() is that
> > it'll come after a point where it's safe to sync the state. Not "as
> > soon as it's safe" -- meaning no timing guarantee. Really, this is
> > just to make sure sync_state() doesn't come before late initcalls
> > (because a lot of frameworks make assumptions around it) are done and
> > before all the top level devices are added.
> >
> > So, even today, sync_state() can come a bit late. In general, the
> > driver framework doesn't guarantee immediate action. There is no
> > guarantee that a drive probe will be called as soon as it's registered
> > (if there is a device) and vice versa. The same will be true for a
> > "consumer_probed()" callback too.
> >
> > Also, your implementation is literally trying to check which consumers
> > have probed and which ones haven't. So something like
> > "consumer_probed()" or "consumer_change()" should work well and is
> > actually meaningful for a driver developer.
>
> I have no strong opinion on the name, so if you prefer
> "consumer_probed|changed", let me just pick one of them then.
Thanks for taking my input.
I'm thinking "consumer_state_changed()" would be good because:
1. Not using "probed" because we might need to send this in the future
when a consumer gets deleted.
2. Adding "state" because "consumer_changed()" could be mistaken to
mean the actual consumer(s) changed.
With that said, I don't have a strong preference between
consumer_probed(), consumer_changed() or consumer_state_changed().
Thanks,
Saravana
^ permalink raw reply
* Re: [PATCH v3 06/13] pmdomain: core: Add initial fine grained sync_state support
From: Saravana Kannan @ 2026-05-14 0:27 UTC (permalink / raw)
To: Ulf Hansson
Cc: Saravana Kannan, Danilo Krummrich, Rafael J . Wysocki,
Greg Kroah-Hartman, driver-core, linux-pm, Sudeep Holla,
Cristian Marussi, Kevin Hilman, Stephen Boyd, Marek Szyprowski,
Bjorn Andersson, Abel Vesa, Peng Fan, Tomi Valkeinen, Maulik Shah,
Konrad Dybcio, Thierry Reding, Jonathan Hunter,
Geert Uytterhoeven, Dmitry Baryshkov, linux-arm-kernel,
linux-kernel
In-Reply-To: <CAPDyKFo4yoxGtvKNzCfTY0tOwPPxo+Ub7=VvCKtVPr3M31o=YA@mail.gmail.com>
On Wed, May 13, 2026 at 2:36 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Wed, 13 May 2026 at 07:34, Saravana Kannan <saravanak@kernel.org> wrote:
> >
> > On Mon, May 11, 2026 at 3:25 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Mon, 11 May 2026 at 07:09, Saravana Kannan <saravanak@kernel.org> wrote:
> > > >
> > > > On Fri, May 8, 2026 at 5:39 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > >
> > > > > A onecell (#power-domain-cells = <1 or 2>; in DT) power domain provider
> > > > > typically provides multiple independent power domains, each with their own
> > > > > corresponding consumers. In these cases we have to wait for all consumers
> > > > > for all the provided power domains before the ->sync_state() callback gets
> > > > > called for the supplier.
> > > > >
> > > > > In a first step to improve this, let's implement support for fine grained
> > > > > sync_state support a per genpd basis by using the ->queue_sync_state()
> > > > > callback. To take step by step, let's initially limit the improvement to
> > > > > the internal genpd provider driver and to its corresponding genpd devices
> > > > > for onecell providers.
> > > > >
> > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > ---
> > > > >
> > > > > Changes in v3:
> > > > > - Addressed some cosmetic comments from Geert.
> > > > >
> > > > > ---
> > > > > drivers/pmdomain/core.c | 124 ++++++++++++++++++++++++++++++++++++++
> > > > > include/linux/pm_domain.h | 1 +
> > > > > 2 files changed, 125 insertions(+)
> > > > >
> > > > > diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> > > > > index ad57846f02a3..c01a9a96e5c2 100644
> > > > > --- a/drivers/pmdomain/core.c
> > > > > +++ b/drivers/pmdomain/core.c
> > > > > @@ -2699,6 +2699,119 @@ static struct generic_pm_domain *genpd_get_from_provider(
> > > > > return genpd;
> > > > > }
> > > > >
> > > > > +static bool genpd_should_wait_for_consumer(struct device_node *np)
> > > > > +{
> > > > > + struct generic_pm_domain *genpd;
> > > > > + bool should_wait = false;
> > > > > +
> > > > > + mutex_lock(&gpd_list_lock);
> > > > > + list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
> > > > > + if (genpd->provider == of_fwnode_handle(np)) {
> > > > > + genpd_lock(genpd);
> > > > > +
> > > > > + /* Clear the previous state before reevaluating. */
> > > > > + genpd->wait_for_consumer = false;
> > > > > +
> > > > > + /*
> > > > > + * Unless there is at least one genpd for the provider
> > > > > + * that is being kept powered-on, we don't have to care
> > > > > + * about waiting for consumers.
> > > > > + */
> > > > > + if (genpd->stay_on)
> > > > > + should_wait = true;
> > > > > +
> > > > > + genpd_unlock(genpd);
> > > > > + }
> > > > > + }
> > > > > + mutex_unlock(&gpd_list_lock);
> > > >
> > > > I think I understand the intent of this function, but haven't dug into
> > > > genpd code enough to comment on this yet. I'll come back to this
> > > > later.
> > > >
> > > > > +
> > > > > + return should_wait;
> > > > > +}
> > > > > +
> > > > > +static void genpd_parse_for_consumer(struct device_node *sup,
> > > > > + struct device_node *con)
> > > > > +{
> > > > > + struct generic_pm_domain *genpd;
> > > > > +
> > > > > + for (unsigned int i = 0; ; i++) {
> > > > > + struct of_phandle_args pd_args;
> > > > > +
> > > > > + if (of_parse_phandle_with_args(con, "power-domains",
> > > > > + "#power-domain-cells",
> > > > > + i, &pd_args))
> > > > > + break;
> > > >
> > > > Why not use a while or a do while() instead of this infinite for loop
> > > > with a break?
> > >
> > > I guess it's a matter of personal preference. I'm not sure the code
> > > gets any nicer with a do/while, but if you really insist I can change
> > > it.
> > >
> > > >
> > > > > +
> > > > > + /*
> > > > > + * The phandle must correspond to the supplier's genpd provider
> > > > > + * to be relevant else let's move to the next index.
> > > > > + */
> > > > > + if (sup != pd_args.np) {
> > > > > + of_node_put(pd_args.np);
> > > > > + continue;
> > > > > + }
> > > > > +
> > > > > + mutex_lock(&gpd_list_lock);
> > > > > + genpd = genpd_get_from_provider(&pd_args);
> > > > > + if (!IS_ERR(genpd)) {
> > > > > + genpd_lock(genpd);
> > > > > + genpd->wait_for_consumer = true;
> > > > > + genpd_unlock(genpd);
> > > > > + }
> > > > > + mutex_unlock(&gpd_list_lock);
> > > > > +
> > > > > + of_node_put(pd_args.np);
> > > > > + }
> > > > > +}
> > > > > +
> > > > > +static void _genpd_queue_sync_state(struct device_node *np)
> > > > > +{
> > > > > + struct generic_pm_domain *genpd;
> > > > > +
> > > > > + mutex_lock(&gpd_list_lock);
> > > > > + list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
> > > > > + if (genpd->provider == of_fwnode_handle(np)) {
> > > > > + genpd_lock(genpd);
> > > > > + if (genpd->stay_on && !genpd->wait_for_consumer) {
> > > > > + genpd->stay_on = false;
> > > > > + genpd_queue_power_off_work(genpd);
> > > > > + }
> > > > > + genpd_unlock(genpd);
> > > > > + }
> > > > > + }
> > > > > + mutex_unlock(&gpd_list_lock);
> > > > > +}
> > > > > +
> > > > > +static void genpd_queue_sync_state(struct device *dev)
> > > > > +{
> > > > > + struct device_node *np = dev->of_node;
> > > > > + struct device_link *link;
> > > > > +
> > > > > + if (!genpd_should_wait_for_consumer(np))
> > > > > + return;
> > > > > +
> > > > > + list_for_each_entry(link, &dev->links.consumers, s_node) {
> > > >
> > > > Couple of issues:
> > > > 1. I don't want the frameworks to be so deeply aware of driver core
> > > > internals. I want the driver core maintainers to be able to change the
> > > > devlink implementation without having to worry about going and fixing
> > > > all the frameworks. So, please add a for_each_consumer_dev(supplier,
> > > > callback) and for_each_supplier_dev(consumer, callback) helper
> > > > functions.
> > >
> > > I understand your concern and I like the idea. However, maybe it's
> > > better to get this landed (the series is complicated as is) first and
> > > then can continue to improve the code on top, with helper functions
> > > etc.
> >
> > That patch should be pretty simple, so let's just do it? You are
> > literally just moving the code to another file and massaging it a bit.
> > I can send one out if you want/
>
> Okay, I will fold in a patch into my series that adds the helper. No
> point you sending it as it needs to be apart of my series to have a
> user for it.
Ok, thanks.
>
> >
> > > >
> > > > 2. This doesn't ignore "SYNC_STATE_ONLY" links and that's going to
> > > > confuse the consumer count/check you might do or at the least waste
> > > > parsing those.
> > >
> > > I am not sure I understand how I should take SYNC_STATE_ONLY links
> > > into account here.
> > >
> > > At each call to the genpd_queue_sync_state(), we walk through all the
> > > provided genpds for the provider. No previous state is cached to track
> > > consumer counts.
> >
> > Ok, correct me if I'm wrong here, but it looks like you are looking at
> > all the consumers, finding the ones that haven't probed yet and then
> > trying to see which specific genpd provider each one of them is using
> > by parsing their dt properties and then keep those ON.
>
> The code cares only about the genpd OF provider that the supplier
> device belongs to, if any. Any other genpd OF providers and their
> genpds is untouched.
>
> The corresponding genpd OF provider may provide multiple genpds
> through the same fwnode.
Yeah, that's what I meant. Since I don't look at the genpd code often
I was conflating the "genpd OF provider" vs "genpd" terminology,
>
> >
> > If my understanding above is wrong then, please help me understand
> > what the code is trying to do.
> >
> > If it's right, then here's the issue:
> >
> > fw_devlink relies on device links and fwnode links to keep track of
> > consumers. fwnode links get created first and then get converted to
> > device links when both the supplier AND consumer devices are created.
> >
> > Consider a case when the supplier S is created and the consumer device
> > C is several levels deep inside a parent device A.
> > S { #power-domain-cells = <1>; }
> > A { B { C { power-domains = <&S MY_DOMAIN>; } } }
> >
> > fw_devlink can't just go "oh there's no consumer device that hasn't
> > probed yet, let me call sync_state()". It needs to wait for C. But
> > there's no way to create a device link to C. So, here's the sequence
> > that happens:
> >
> > 1. When device A gets added, it creates a "proxy" SYNC_STATE_ONLY link
> > between S and A.
> > 2. When A probes, it adds device B.
> > 3. The SYNC_STATE_ONLY link between S to B is created.
> > 4. The SYNC_STATE_ONLY link between S to A is deleted.
> > 5. When B probes, it adds device C.
> > 6. The actual device link between S and C is created.
> > 7. The SYNC_STATE_ONLY link between S to B is deleted.
> > 8. Device C probes, the device links get updated, sync_state() gets called.
> >
> > So, if your code doesn't account for SYNC_STATE_ONLY links, you are
> > going to check device "A" to see which providers are used. You'll
> > think that MY_DOMAIN isn't used by any unprobed consumer and turn it
> > off.
> >
> > >
> > > >
> > > > 3. **Device** links are not the complete list of consumers because
> > > > they can only link consumer **devices** once the consumer **device**
> > > > is created.
> > > >
> > > > 4. What you really need is a for_each_consumer_fwnode(supplier,
> > > > callback) that first loops through all the consumer device links and
> > > > calls the callback() on their fwnode and then the same function needs
> > > > to loop through all the fwnode links and then pass those consumer
> > > > fwnodes to the callback. And inside that callback you can do whatever
> > > > you want.
> > >
> > > The ->queue_sync_state() callback is invoked *after*
> > > __device_links_queue_sync_state() has been called for the device,
> > > which is also when the conditions for calling ->sync_state() is
> > > checked.
> >
> > I'm not sure what you are trying to say here. Yes, what you are saying
> > is true. But at the point the current code returns before calling
> > sync_state(), your patch 2/13 ends up calling the "consumer_probed()"
> > callback.
> >
> > If you see the example I gave above, there is a SYNC_STATE_ONLY link
> > all the way up to step 7 that'll prevent sync_state() from being
> > called. And until step 5, there is no device link between S and C and
> > you'll have to go look at S's fwnode links to find C.
>
> Thanks for the details, but I don't think the code in genpd needs to
> take these details into account. Or at least that is my goal, if
> possible.
>
> As I understand it, all of the above should already been taken care of
> when __device_links_queue_sync_state() is called, as it's at that
> point when we can validate whether all consumers for a supplier have
> been probed, right?
Not really, and I explained it in my earlier email.
Let me explain it in a different way. For now, forget what
sync_state() does or how it's called.
When you get your new callback:
1. You agree that you want to check the actual consumer DT nodes that
have the "power-domains" property?
2. You agree that if you miss checking a consumer node then you'll
incorrectly calculate wait_for_consumer for a genpd belonging to a
one-cell genpd OF provider?
3. You agree that if wait_for_consumer is set to false before all
consumers explicitly pointing to that specific genpd using the cell
number, then you'll turn off the genpd too early?
I'm pretty sure you'll agree to (1) (2) and (3).
I'm saying that since this new callback is called way more often than
sync_state() it will end up being called when some consumer struct
devices haven't been created yet (I explained why in my earlier email
-- I'm 100% certain this will happen). So, you'll also have to check
the fwnode link consumers too to make sure you don't set
wait_for_consumer = false too early.
The SYNC_STATE_ONLY link ignoring is just an optimization. If you
don't ignore those, you'll be parsing their DT nodes unnecessarily
because they are just proxy consumers (I explain this in my earlier
email too).
>
> >
> > > If there are problems with not yet registered consumer device links,
> > > why isn't that as problem for regular ->sync_state() in
> > > __device_links_queue_sync_state()?
> >
> > Does my explanation above make sense?
>
> I think you may have misunderstood how the code in $subject patch
> works. Let's try again.
I took a closer look today and I'm sure I understand this patch
correctly. There is a "turns off too early" bug.
Thanks,
Saravana
>
> >
> > Welcome to the annoying worlds of fw_devlink corner cases/nuances.
> > There's a case where the child is the supplier of the
> > parent/grandparent. There's a comment for that in the fw_devlink code.
>
> Yeah, I am fully aware of the high level of complexity and as I said,
> I don't want genpd to have to know about *all* of that.
>
> [...]
>
> Kind regards
> Uffe
^ permalink raw reply
* Re: [PATCH] mailbox: exynos: Drop unused register definitions
From: Alexey Klimov @ 2026-05-14 0:57 UTC (permalink / raw)
To: Tudor Ambarus, Jassi Brar, Krzysztof Kozlowski, Alim Akhtar
Cc: linux-kernel, linux-samsung-soc, linux-arm-kernel
In-Reply-To: <20260430-exynos-mbox-dead-def-v1-1-a69176b7d0f0@linaro.org>
On Thu Apr 30, 2026 at 12:12 PM BST, Tudor Ambarus wrote:
> Leaving these dead definitions in place hides which registers are
> actually being used by the hardware, making the driver harder to read
> and maintain. Remove them to clean up the file.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
> drivers/mailbox/exynos-mailbox.c | 7 -------
> 1 file changed, 7 deletions(-)
Reviewed-by: Alexey Klimov <alexey.klimov@linaro.org>
^ permalink raw reply
* [PATCH v5 1/7] perf unwind: Refactor get_entries to allow dynamic libdw/libunwind selection
From: Ian Rogers @ 2026-05-13 23:31 UTC (permalink / raw)
To: acme, adrian.hunter, dapeng1.mi, james.clark, namhyung,
Florian Fainelli, Li Guan
Cc: 9erthalion6, alex, alexander.shishkin, andrew.jones, aou,
atrajeev, howardchu95, john.g.garry, jolsa, leo.yan,
libunwind-devel, linux-arm-kernel, linux-kernel, linux-perf-users,
linux-riscv, mingo, palmer, peterz, pjw, shimin.guo, tglozar,
tmricht, will, Ian Rogers
In-Reply-To: <20260513233151.572332-1-irogers@google.com>
Currently, both libdw and libunwind define 'unwind__get_entries'. This
causes a duplicate symbol build failure when both are compiled into
perf.
This commit refactors the DWARF unwind post-processing to be
configurable at runtime via the .perfconfig file option
'unwind.style', or using the argument '--unwind-style' in the commands
'perf report', 'perf script' and 'perf inject', in a similar manner to
the addr2line or the disassembler style.
The file 'tools/perf/util/unwind.c' adds the top-level dispatch
function 'unwind__get_entries'. The backend implementations are
renamed to 'libdw__get_entries' and 'libunwind__get_entries'. Both are
attempted as fallbacks if not configured, or if the primary backend
fails.
Fixes: 2e9191573a69 ("perf build: Remove NO_LIBDW_DWARF_UNWIND option")
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/builtin-inject.c | 4 +
tools/perf/builtin-report.c | 4 +
tools/perf/builtin-script.c | 4 +
tools/perf/util/Build | 1 +
tools/perf/util/config.c | 4 +
tools/perf/util/symbol_conf.h | 10 +++
tools/perf/util/unwind-libdw.c | 18 +++-
tools/perf/util/unwind-libunwind-local.c | 27 ++++--
tools/perf/util/unwind-libunwind.c | 2 +-
tools/perf/util/unwind.c | 104 +++++++++++++++++++++++
tools/perf/util/unwind.h | 61 ++++++++-----
11 files changed, 207 insertions(+), 32 deletions(-)
create mode 100644 tools/perf/util/unwind.c
diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index 6ab20df358c4..a2493f1097df 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -26,6 +26,7 @@
#include "util/synthetic-events.h"
#include "util/thread.h"
#include "util/namespaces.h"
+#include "util/unwind.h"
#include "util/util.h"
#include "util/tsc.h"
@@ -2563,6 +2564,9 @@ int cmd_inject(int argc, const char **argv)
OPT_STRING(0, "guestmount", &symbol_conf.guestmount, "directory",
"guest mount directory under which every guest os"
" instance has a subdir"),
+ OPT_CALLBACK(0, "unwind-style", NULL, "unwind style",
+ "unwind styles (libdw,libunwind)",
+ unwind__option),
OPT_BOOLEAN(0, "convert-callchain", &inject.convert_callchain,
"Generate callchains using DWARF and drop register/stack data"),
OPT_END()
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 95c0bdba6b11..0b0966d94128 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -48,6 +48,7 @@
#include "util/time-utils.h"
#include "util/auxtrace.h"
#include "util/units.h"
+#include "util/unwind.h"
#include "util/util.h" // perf_tip()
#include "ui/ui.h"
#include "ui/progress.h"
@@ -1449,6 +1450,9 @@ int cmd_report(int argc, const char **argv)
OPT_CALLBACK(0, "addr2line-style", NULL, "addr2line style",
"addr2line styles (libdw,llvm,libbfd,addr2line)",
report_parse_addr2line_config),
+ OPT_CALLBACK(0, "unwind-style", NULL, "unwind style",
+ "unwind styles (libdw,libunwind)",
+ unwind__option),
OPT_BOOLEAN(0, "demangle", &symbol_conf.demangle,
"Symbol demangling. Enabled by default, use --no-demangle to disable."),
OPT_BOOLEAN(0, "demangle-kernel", &symbol_conf.demangle_kernel,
diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index c8ac9f01a36b..fd0b4609516b 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -63,6 +63,7 @@
#include <linux/err.h>
#include "util/dlfilter.h"
#include "util/record.h"
+#include "util/unwind.h"
#include "util/util.h"
#include "util/cgroup.h"
#include "util/annotate.h"
@@ -4159,6 +4160,9 @@ int cmd_script(int argc, const char **argv)
"Enable symbol demangling"),
OPT_BOOLEAN(0, "demangle-kernel", &symbol_conf.demangle_kernel,
"Enable kernel symbol demangling"),
+ OPT_CALLBACK(0, "unwind-style", NULL, "unwind style",
+ "unwind styles (libdw,libunwind)",
+ unwind__option),
OPT_STRING(0, "addr2line", &symbol_conf.addr2line_path, "path",
"addr2line binary to use for line numbers"),
OPT_STRING(0, "time", &script.time_str, "str",
diff --git a/tools/perf/util/Build b/tools/perf/util/Build
index 70cc91d00804..01edfccebb88 100644
--- a/tools/perf/util/Build
+++ b/tools/perf/util/Build
@@ -216,6 +216,7 @@ ifndef CONFIG_SETNS
perf-util-y += setns.o
endif
+perf-util-y += unwind.o
perf-util-$(CONFIG_LIBDW) += probe-finder.o
perf-util-$(CONFIG_LIBDW) += dwarf-aux.o
perf-util-$(CONFIG_LIBDW) += dwarf-regs.o
diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index 087002fb1b9b..7988149dc7ed 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -23,6 +23,7 @@
#include "build-id.h"
#include "debug.h"
#include "config.h"
+#include "unwind.h"
#include <sys/types.h>
#include <sys/stat.h>
#include <stdlib.h>
@@ -525,6 +526,9 @@ int perf_default_config(const char *var, const char *value,
if (strstarts(var, "addr2line."))
return addr2line_configure(var, value, dummy);
+ if (strstarts(var, "unwind."))
+ return unwind__configure(var, value, dummy);
+
/* Add other config variables here. */
return 0;
}
diff --git a/tools/perf/util/symbol_conf.h b/tools/perf/util/symbol_conf.h
index 6cd454d7c98e..0dee5aa6a534 100644
--- a/tools/perf/util/symbol_conf.h
+++ b/tools/perf/util/symbol_conf.h
@@ -9,6 +9,15 @@
struct strlist;
struct intlist;
+enum unwind_style {
+ UNWIND_STYLE_UNKNOWN = 0,
+ UNWIND_STYLE_LIBDW,
+ UNWIND_STYLE_LIBUNWIND,
+};
+
+#define MAX_UNWIND_STYLE (UNWIND_STYLE_LIBUNWIND + 1)
+
+
enum a2l_style {
A2L_STYLE_UNKNOWN = 0,
A2L_STYLE_LIBDW,
@@ -81,6 +90,7 @@ struct symbol_conf {
const char *addr2line_path;
enum a2l_style addr2line_style[MAX_A2L_STYLE];
int addr2line_timeout_ms;
+ enum unwind_style unwind_style[MAX_UNWIND_STYLE];
unsigned long time_quantum;
struct strlist *dso_list,
*comm_list,
diff --git a/tools/perf/util/unwind-libdw.c b/tools/perf/util/unwind-libdw.c
index 05e8e68bd49c..21171a6a878c 100644
--- a/tools/perf/util/unwind-libdw.c
+++ b/tools/perf/util/unwind-libdw.c
@@ -339,7 +339,7 @@ frame_callback(Dwfl_Frame *state, void *arg)
DWARF_CB_ABORT : DWARF_CB_OK;
}
-int unwind__get_entries(unwind_entry_cb_t cb, void *arg,
+int libdw__get_entries(unwind_entry_cb_t cb, void *arg,
struct thread *thread,
struct perf_sample *data,
int max_stack,
@@ -356,7 +356,7 @@ int unwind__get_entries(unwind_entry_cb_t cb, void *arg,
int err = -EINVAL, i;
if (!data->user_regs || !data->user_regs->regs)
- return -EINVAL;
+ return 0;
ui = zalloc(sizeof(*ui) + sizeof(ui->entries[0]) * max_stack);
if (!ui)
@@ -430,6 +430,18 @@ int unwind__get_entries(unwind_entry_cb_t cb, void *arg,
map_symbol__exit(&ui->entries[i].ms);
dwfl_ui_ti->ui = NULL;
+ int entries = (int)ui->idx;
free(ui);
- return 0;
+ /*
+ * Unwinder return contract:
+ * > 0 : unwinding succeeded (stops fallback). If we found frames but hit an error
+ * (e.g. truncated stack), report success to preserve existing frames.
+ * 0 : unwinding failed without yielding frames. Ignore non-fatal errors
+ * (e.g. missing debug info, DWARF corruption) to allow fallback unwinder or
+ * kernel callchain resolution to proceed.
+ * < 0 : fatal error (e.g. -ENOMEM). Aborts unwinding entirely.
+ */
+ if (err)
+ return (err == -ENOMEM) ? -ENOMEM : (entries > 0 ? 1 : 0);
+ return entries;
}
diff --git a/tools/perf/util/unwind-libunwind-local.c b/tools/perf/util/unwind-libunwind-local.c
index 87d496e9dfa6..27e2f7b31789 100644
--- a/tools/perf/util/unwind-libunwind-local.c
+++ b/tools/perf/util/unwind-libunwind-local.c
@@ -744,7 +744,7 @@ static int get_entries(struct unwind_info *ui, unwind_entry_cb_t cb,
ret = perf_reg_value(&val, perf_sample__user_regs(ui->sample),
perf_arch_reg_ip(e_machine));
if (ret)
- return ret;
+ return 0;
ips[i++] = (unw_word_t) val;
@@ -757,7 +757,7 @@ static int get_entries(struct unwind_info *ui, unwind_entry_cb_t cb,
addr_space = maps__addr_space(thread__maps(ui->thread));
if (addr_space == NULL)
- return -1;
+ return 0;
ret = unw_init_remote(&c, addr_space, ui);
if (ret && !ui->best_effort)
@@ -785,15 +785,30 @@ static int get_entries(struct unwind_info *ui, unwind_entry_cb_t cb,
/*
* Display what we got based on the order setup.
*/
+ int entries = 0;
for (i = 0; i < max_stack && !ret; i++) {
int j = i;
if (callchain_param.order == ORDER_CALLER)
j = max_stack - i - 1;
- ret = ips[j] ? entry(ips[j], ui->thread, cb, arg) : 0;
+ if (ips[j]) {
+ ret = entry(ips[j], ui->thread, cb, arg);
+ if (ret)
+ break;
+ entries++;
+ }
}
- return ret;
+ /*
+ * Unwinder return contract:
+ * > 0 : unwinding succeeded (stops fallback).
+ * 0 : unwinding failed without yielding frames. Ignore non-fatal errors
+ * (e.g. stepping failure) to allow fallback unwinder or kernel callchains.
+ * < 0 : fatal error (e.g. -ENOMEM). Aborts unwinding entirely.
+ */
+ if (ret == -ENOMEM)
+ return -ENOMEM;
+ return (entries > 0 || ret == 0) ? entries : 0;
}
static int _unwind__get_entries(unwind_entry_cb_t cb, void *arg,
@@ -809,10 +824,10 @@ static int _unwind__get_entries(unwind_entry_cb_t cb, void *arg,
};
if (!data->user_regs || !data->user_regs->regs)
- return -EINVAL;
+ return 0;
if (max_stack <= 0)
- return -EINVAL;
+ return 0;
return get_entries(&ui, cb, arg, max_stack);
}
diff --git a/tools/perf/util/unwind-libunwind.c b/tools/perf/util/unwind-libunwind.c
index cb8be6acfb6f..a0016b897dae 100644
--- a/tools/perf/util/unwind-libunwind.c
+++ b/tools/perf/util/unwind-libunwind.c
@@ -79,7 +79,7 @@ void unwind__finish_access(struct maps *maps)
ops->finish_access(maps);
}
-int unwind__get_entries(unwind_entry_cb_t cb, void *arg,
+int libunwind__get_entries(unwind_entry_cb_t cb, void *arg,
struct thread *thread,
struct perf_sample *data, int max_stack,
bool best_effort)
diff --git a/tools/perf/util/unwind.c b/tools/perf/util/unwind.c
new file mode 100644
index 000000000000..4ed4b1d55c69
--- /dev/null
+++ b/tools/perf/util/unwind.c
@@ -0,0 +1,104 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "debug.h"
+#include "symbol_conf.h"
+#include "unwind.h"
+#include <linux/string.h>
+#include <string.h>
+#include <stdlib.h>
+
+int unwind__get_entries(unwind_entry_cb_t cb __maybe_unused, void *arg __maybe_unused,
+ struct thread *thread __maybe_unused,
+ struct perf_sample *data __maybe_unused,
+ int max_stack __maybe_unused,
+ bool best_effort __maybe_unused)
+{
+ int ret = 0;
+
+#if defined(HAVE_LIBDW_SUPPORT) || defined(HAVE_LIBUNWIND_SUPPORT)
+ if (symbol_conf.unwind_style[0] == UNWIND_STYLE_UNKNOWN) {
+ int i = 0;
+#ifdef HAVE_LIBDW_SUPPORT
+ symbol_conf.unwind_style[i++] = UNWIND_STYLE_LIBDW;
+#endif
+#ifdef HAVE_LIBUNWIND_SUPPORT
+ symbol_conf.unwind_style[i++] = UNWIND_STYLE_LIBUNWIND;
+#endif
+ }
+#endif //defined(HAVE_LIBDW_SUPPORT) || defined(HAVE_LIBUNWIND_SUPPORT)
+
+ for (size_t i = 0; i < ARRAY_SIZE(symbol_conf.unwind_style); i++) {
+ switch (symbol_conf.unwind_style[i]) {
+ case UNWIND_STYLE_LIBDW:
+ ret = libdw__get_entries(cb, arg, thread, data, max_stack, best_effort);
+ break;
+ case UNWIND_STYLE_LIBUNWIND:
+ ret = libunwind__get_entries(cb, arg, thread, data, max_stack, best_effort);
+ break;
+ case UNWIND_STYLE_UNKNOWN:
+ default:
+#if !defined(HAVE_LIBDW_SUPPORT) && !defined(HAVE_LIBUNWIND_SUPPORT)
+ pr_warning_once(
+ "Error: dwarf unwinding not supported, build perf with libdw or libunwind.\n");
+#endif
+ ret = 0;
+ break;
+ }
+ if (ret > 0) {
+ ret = 0;
+ break;
+ }
+ if (ret < 0)
+ break;
+ }
+ return ret;
+}
+
+int unwind__configure(const char *var, const char *value, void *cb __maybe_unused)
+{
+ static const char * const unwind_style_names[] = {
+ [UNWIND_STYLE_LIBDW] = "libdw",
+ [UNWIND_STYLE_LIBUNWIND] = "libunwind",
+ NULL
+ };
+ char *s, *p, *saveptr;
+ size_t i = 0;
+
+ if (strcmp(var, "unwind.style"))
+ return 0;
+
+ if (!value)
+ return -1;
+
+ s = strdup(value);
+ if (!s)
+ return -1;
+
+ memset(symbol_conf.unwind_style, 0, sizeof(symbol_conf.unwind_style));
+
+ p = strtok_r(s, ",", &saveptr);
+ while (p && i < ARRAY_SIZE(symbol_conf.unwind_style)) {
+ bool found = false;
+ char *q = strim(p);
+
+ for (size_t j = UNWIND_STYLE_LIBDW; j < MAX_UNWIND_STYLE; j++) {
+ if (!strcasecmp(q, unwind_style_names[j])) {
+ symbol_conf.unwind_style[i++] = j;
+ found = true;
+ break;
+ }
+ }
+ if (!found)
+ pr_warning("Unknown unwind style: %s\n", q);
+ p = strtok_r(NULL, ",", &saveptr);
+ }
+
+ free(s);
+ return 0;
+}
+
+int unwind__option(const struct option *opt __maybe_unused,
+ const char *arg,
+ int unset __maybe_unused)
+{
+ return unwind__configure("unwind.style", arg, NULL);
+}
diff --git a/tools/perf/util/unwind.h b/tools/perf/util/unwind.h
index 9f7164c6d9aa..28db3e3b9b51 100644
--- a/tools/perf/util/unwind.h
+++ b/tools/perf/util/unwind.h
@@ -4,9 +4,10 @@
#include <linux/compiler.h>
#include <linux/types.h>
-#include "util/map_symbol.h"
+#include "map_symbol.h"
struct maps;
+struct option;
struct perf_sample;
struct thread;
@@ -26,7 +27,9 @@ struct unwind_libunwind_ops {
struct perf_sample *data, int max_stack, bool best_effort);
};
-#ifdef HAVE_DWARF_UNWIND_SUPPORT
+int unwind__configure(const char *var, const char *value, void *cb);
+int unwind__option(const struct option *opt, const char *arg, int unset);
+
/*
* When best_effort is set, don't report errors and fail silently. This could
* be expanded in the future to be more permissive about things other than
@@ -36,8 +39,31 @@ int unwind__get_entries(unwind_entry_cb_t cb, void *arg,
struct thread *thread,
struct perf_sample *data, int max_stack,
bool best_effort);
-/* libunwind specific */
+
+#ifdef HAVE_LIBDW_SUPPORT
+int libdw__get_entries(unwind_entry_cb_t cb, void *arg,
+ struct thread *thread,
+ struct perf_sample *data, int max_stack,
+ bool best_effort);
+#else
+#include "debug.h"
+static inline int libdw__get_entries(unwind_entry_cb_t cb __maybe_unused, void *arg __maybe_unused,
+ struct thread *thread __maybe_unused,
+ struct perf_sample *data __maybe_unused,
+ int max_stack __maybe_unused,
+ bool best_effort __maybe_unused)
+{
+ pr_err("Error: libdw dwarf unwinding not built into perf\n");
+ return 0;
+}
+#endif
+
#ifdef HAVE_LIBUNWIND_SUPPORT
+/* libunwind specific */
+int libunwind__get_entries(unwind_entry_cb_t cb, void *arg,
+ struct thread *thread,
+ struct perf_sample *data, int max_stack,
+ bool best_effort);
#ifndef LIBUNWIND__ARCH_REG_ID
#define LIBUNWIND__ARCH_REG_ID(regnum) libunwind__arch_reg_id(regnum)
#endif
@@ -47,25 +73,15 @@ int unwind__prepare_access(struct maps *maps, struct map *map, bool *initialized
void unwind__flush_access(struct maps *maps);
void unwind__finish_access(struct maps *maps);
#else
-static inline int unwind__prepare_access(struct maps *maps __maybe_unused,
- struct map *map __maybe_unused,
- bool *initialized __maybe_unused)
-{
- return 0;
-}
-
-static inline void unwind__flush_access(struct maps *maps __maybe_unused) {}
-static inline void unwind__finish_access(struct maps *maps __maybe_unused) {}
-#endif
-#else
-static inline int
-unwind__get_entries(unwind_entry_cb_t cb __maybe_unused,
- void *arg __maybe_unused,
- struct thread *thread __maybe_unused,
- struct perf_sample *data __maybe_unused,
- int max_stack __maybe_unused,
- bool best_effort __maybe_unused)
+#include "debug.h"
+static inline int libunwind__get_entries(unwind_entry_cb_t cb __maybe_unused,
+ void *arg __maybe_unused,
+ struct thread *thread __maybe_unused,
+ struct perf_sample *data __maybe_unused,
+ int max_stack __maybe_unused,
+ bool best_effort __maybe_unused)
{
+ pr_err("Error: libunwind dwarf unwinding not built into perf\n");
return 0;
}
@@ -78,5 +94,6 @@ static inline int unwind__prepare_access(struct maps *maps __maybe_unused,
static inline void unwind__flush_access(struct maps *maps __maybe_unused) {}
static inline void unwind__finish_access(struct maps *maps __maybe_unused) {}
-#endif /* HAVE_DWARF_UNWIND_SUPPORT */
+#endif
+
#endif /* __UNWIND_H */
--
2.54.0.563.g4f69b47b94-goog
^ permalink raw reply related
* [PATCH 1/4] perf: nvidia_t410_cmem_latency: fix cpuhp state leak on init failure
From: Saurav Sachidanand @ 2026-05-14 1:06 UTC (permalink / raw)
To: Will Deacon
Cc: Mark Rutland, Besar Wicaksono, Ilkka Koskinen, Andi Shyti,
linux-arm-kernel, linux-perf-users, linux-kernel, aghayev, juew,
Saurav Sachidanand
If platform_driver_register() fails, the cpuhp multi-state registered
by cpuhp_setup_state_multi() is never cleaned up. Add
cpuhp_remove_multi_state() on the error path, mirroring the cleanup
in cmem_lat_pmu_exit().
Fixes: 429b7638b2df ("perf: add NVIDIA Tegra410 CPU Memory Latency PMU")
Signed-off-by: Saurav Sachidanand <sauravsc@amazon.com>
---
drivers/perf/nvidia_t410_cmem_latency_pmu.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/perf/nvidia_t410_cmem_latency_pmu.c b/drivers/perf/nvidia_t410_cmem_latency_pmu.c
index acb8f5571522c..e27bf31b2b366 100644
--- a/drivers/perf/nvidia_t410_cmem_latency_pmu.c
+++ b/drivers/perf/nvidia_t410_cmem_latency_pmu.c
@@ -719,7 +719,11 @@ static int __init cmem_lat_pmu_init(void)
cmem_lat_pmu_cpuhp_state = ret;
- return platform_driver_register(&cmem_lat_pmu_driver);
+ ret = platform_driver_register(&cmem_lat_pmu_driver);
+ if (ret)
+ cpuhp_remove_multi_state(cmem_lat_pmu_cpuhp_state);
+
+ return ret;
}
static void __exit cmem_lat_pmu_exit(void)
--
2.47.3
^ permalink raw reply related
* [PATCH 2/4] perf: nvidia_t410_cmem_latency: handle PERF_EF_UPDATE in stop
From: Saurav Sachidanand @ 2026-05-14 1:06 UTC (permalink / raw)
To: Will Deacon
Cc: Mark Rutland, Besar Wicaksono, Ilkka Koskinen, Andi Shyti,
linux-arm-kernel, linux-perf-users, linux-kernel, aghayev, juew,
Saurav Sachidanand
In-Reply-To: <20260514010629.76558-1-sauravsc@amazon.com>
cmem_lat_pmu_stop() does not read the final counter value when called
with PERF_EF_UPDATE. When perf core calls pmu->del() -> pmu->stop()
with PERF_EF_UPDATE, the last counter delta is lost because the event
is marked stopped without reading hardware.
Add the standard PMU stop pattern: bail out if already stopped, call
the event update function when PERF_EF_UPDATE is requested, then mark
the event stopped.
Fixes: 429b7638b2df ("perf: add NVIDIA Tegra410 CPU Memory Latency PMU")
Signed-off-by: Saurav Sachidanand <sauravsc@amazon.com>
---
drivers/perf/nvidia_t410_cmem_latency_pmu.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/perf/nvidia_t410_cmem_latency_pmu.c b/drivers/perf/nvidia_t410_cmem_latency_pmu.c
index e27bf31b2b366..c7fa54c7a7c9e 100644
--- a/drivers/perf/nvidia_t410_cmem_latency_pmu.c
+++ b/drivers/perf/nvidia_t410_cmem_latency_pmu.c
@@ -303,6 +303,12 @@ static void cmem_lat_pmu_start(struct perf_event *event, int pmu_flags)
static void cmem_lat_pmu_stop(struct perf_event *event, int pmu_flags)
{
+ if (event->hw.state & PERF_HES_STOPPED)
+ return;
+
+ if (pmu_flags & PERF_EF_UPDATE)
+ cmem_lat_pmu_event_update(event);
+
event->hw.state |= PERF_HES_STOPPED;
}
--
2.47.3
^ permalink raw reply related
* [PATCH 3/4] perf: nvidia_t410_c2c: fix cpuhp state leak on init failure
From: Saurav Sachidanand @ 2026-05-14 1:06 UTC (permalink / raw)
To: Will Deacon
Cc: Mark Rutland, Besar Wicaksono, Ilkka Koskinen, Andi Shyti,
linux-arm-kernel, linux-perf-users, linux-kernel, aghayev, juew,
Saurav Sachidanand
In-Reply-To: <20260514010629.76558-1-sauravsc@amazon.com>
If platform_driver_register() fails, the cpuhp multi-state registered
by cpuhp_setup_state_multi() is never cleaned up. Add
cpuhp_remove_multi_state() on the error path, mirroring the cleanup
in nv_c2c_pmu_exit().
Fixes: 2f89b7f78c50 ("perf: add NVIDIA Tegra410 C2C PMU")
Signed-off-by: Saurav Sachidanand <sauravsc@amazon.com>
---
drivers/perf/nvidia_t410_c2c_pmu.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/perf/nvidia_t410_c2c_pmu.c b/drivers/perf/nvidia_t410_c2c_pmu.c
index 411987153ff3f..662fa1bc833a5 100644
--- a/drivers/perf/nvidia_t410_c2c_pmu.c
+++ b/drivers/perf/nvidia_t410_c2c_pmu.c
@@ -1034,7 +1034,12 @@ static int __init nv_c2c_pmu_init(void)
return ret;
nv_c2c_pmu_cpuhp_state = ret;
- return platform_driver_register(&nv_c2c_pmu_driver);
+
+ ret = platform_driver_register(&nv_c2c_pmu_driver);
+ if (ret)
+ cpuhp_remove_multi_state(nv_c2c_pmu_cpuhp_state);
+
+ return ret;
}
static void __exit nv_c2c_pmu_exit(void)
--
2.47.3
^ permalink raw reply related
* [PATCH 4/4] perf: nvidia_t410_c2c: handle PERF_EF_UPDATE in stop
From: Saurav Sachidanand @ 2026-05-14 1:06 UTC (permalink / raw)
To: Will Deacon
Cc: Mark Rutland, Besar Wicaksono, Ilkka Koskinen, Andi Shyti,
linux-arm-kernel, linux-perf-users, linux-kernel, aghayev, juew,
Saurav Sachidanand
In-Reply-To: <20260514010629.76558-1-sauravsc@amazon.com>
nv_c2c_pmu_stop() does not read the final counter value when called
with PERF_EF_UPDATE. The last counter delta is lost when perf core
removes the event.
Add the standard PMU stop pattern: bail out if already stopped, call
nv_c2c_pmu_event_update() when PERF_EF_UPDATE is requested, then mark
the event stopped.
Fixes: 2f89b7f78c50 ("perf: add NVIDIA Tegra410 C2C PMU")
Signed-off-by: Saurav Sachidanand <sauravsc@amazon.com>
---
drivers/perf/nvidia_t410_c2c_pmu.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/perf/nvidia_t410_c2c_pmu.c b/drivers/perf/nvidia_t410_c2c_pmu.c
index 662fa1bc833a5..39d0e661e4c5c 100644
--- a/drivers/perf/nvidia_t410_c2c_pmu.c
+++ b/drivers/perf/nvidia_t410_c2c_pmu.c
@@ -394,6 +394,12 @@ static void nv_c2c_pmu_start(struct perf_event *event, int pmu_flags)
static void nv_c2c_pmu_stop(struct perf_event *event, int pmu_flags)
{
+ if (event->hw.state & PERF_HES_STOPPED)
+ return;
+
+ if (pmu_flags & PERF_EF_UPDATE)
+ nv_c2c_pmu_event_update(event);
+
event->hw.state |= PERF_HES_STOPPED;
}
--
2.47.3
^ permalink raw reply related
* Re: [PATCH v4 0/3] iio: adc: xilinx-ams: refactor alarm handling to table-driven design
From: Salih Erim @ 2026-05-14 1:21 UTC (permalink / raw)
To: Guilherme Ivo Bozi, anand.ashok.dumbre, andy, conall.ogriofa,
dlechner, jic23, manish.narani, michal.simek, nuno.sa
Cc: Jonathan.Cameron, linux-arm-kernel, linux-iio
In-Reply-To: <20260513003503.339418-1-guilherme.bozi@usp.br>
Hi,
On 5/13/2026 1:31 AM, Guilherme Ivo Bozi wrote:
> v3 -> v4:
> - Removed unnecessary 'event < 0' check for type e32
Thanks for addressing the feedback.
Tested on ZCU102 Rev1.0. Driver probes successfully with no errors.
For the series:
Reviewed-by: Salih Erim <salih.erim@amd.com>
Tested-by: Salih Erim <salih.erim@amd.com>
Salih
^ permalink raw reply
* Re: [PATCH 1/3] usb: dwc3: xilinx: fix missing space before closing comment delimiter
From: Thinh Nguyen @ 2026-05-14 1:23 UTC (permalink / raw)
To: Radhey Shyam Pandey
Cc: Thinh Nguyen, gregkh@linuxfoundation.org, michal.simek@amd.com,
p.zabel@pengutronix.de, linux-usb@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, git@amd.com
In-Reply-To: <20260511160814.2904882-2-radhey.shyam.pandey@amd.com>
On Mon, May 11, 2026, Radhey Shyam Pandey wrote:
> Add missing space before '*/' in an inline comment to follow
> the kernel coding style.
>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> ---
> drivers/usb/dwc3/dwc3-xilinx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c
> index f41b0da5e89d..a3c7dc258c7d 100644
> --- a/drivers/usb/dwc3/dwc3-xilinx.c
> +++ b/drivers/usb/dwc3/dwc3-xilinx.c
> @@ -196,7 +196,7 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
> }
>
> if (priv_data->usb3_phy) {
> - /* Set PIPE Power Present signal in FPD Power Present Register*/
> + /* Set PIPE Power Present signal in FPD Power Present Register */
> writel(FPD_POWER_PRSNT_OPTION, priv_data->regs + XLNX_USB_FPD_POWER_PRSNT);
> /* Set the PIPE Clock Select bit in FPD PIPE Clock register */
> writel(PIPE_CLK_SELECT, priv_data->regs + XLNX_USB_FPD_PIPE_CLK);
> --
> 2.44.4
>
Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
BR,
Thinh
^ permalink raw reply
* Re: [PATCH 2/4] ASoC: stm: stm32_i2s: Use guard() for spin locks
From: Mark Brown @ 2026-05-14 1:25 UTC (permalink / raw)
To: phucduc.bui
Cc: Olivier Moysan, Arnaud Pouliquen, Liam Girdwood, Jaroslav Kysela,
Takashi Iwai, Maxime Coquelin, Alexandre Torgue, linux-sound,
linux-stm32, linux-arm-kernel, linux-kernel
In-Reply-To: <20260513104329.81592-3-phucduc.bui@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 644 bytes --]
On Wed, May 13, 2026 at 05:43:27PM +0700, phucduc.bui@gmail.com wrote:
> @@ -1016,21 +1014,18 @@ static int stm32_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
> I2S_IER_OVRIE,
> (unsigned int)~I2S_IER_OVRIE);
>
> - spin_lock(&i2s->lock_fd);
> - i2s->refcount--;
> - if (i2s->refcount) {
> - spin_unlock(&i2s->lock_fd);
> - break;
> - }
> + scoped_guard(spinlock, &i2s->lock_fd) {
> + i2s->refcount--;
> + if (i2s->refcount)
> + break;
How does scoped_guard interact with break statements - does this still
apply to the switch? I've not looked at how they're implemented...
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v2 03/11] m68k: mcf5441x: setup DAC clock name as per driver name
From: Greg Ungerer @ 2026-05-14 1:27 UTC (permalink / raw)
To: Angelo Dureghello, Geert Uytterhoeven, Steven King, Arnd Bergmann,
Maxime Coquelin, Alexandre Torgue, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko
Cc: Greg Ungerer, linux-m68k, linux-kernel, linux-stm32,
linux-arm-kernel, linux-iio
In-Reply-To: <20260513-wip-stmark2-dac-v2-3-fcdae50cf51a@baylibre.com>
Hi Angelo,
On 13/5/26 19:14, Angelo Dureghello wrote:
> From: Angelo Dureghello <adureghello@baylibre.com>
>
> Later in this patchset, the mcf54415 DAC driver is added.
> Considering some other different ColdFire cpu DACs exists, the DAC driver
> is named as "mcf54415_dac", related to the mcf5441x family SoCs with
> DACs (mcf54415/6/7/8).
>
> So updating DAC clock names to bind with proper driver name.
I am not sure I like naming the clocks here with a prefix for the
specific SoC part number this is in. It might be unlikely now, but
what if another ColdFire family SoC member uses this same hardware block?
That is very common amongst other hardware blocks within the ColdFire
family. Can we come up with a name more specific to just this type
of DAC hardware block?
Regards
Greg
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> ---
> arch/m68k/coldfire/m5441x.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/m68k/coldfire/m5441x.c b/arch/m68k/coldfire/m5441x.c
> index 5b5e09ecf487..b724d7fc1a08 100644
> --- a/arch/m68k/coldfire/m5441x.c
> +++ b/arch/m68k/coldfire/m5441x.c
> @@ -43,8 +43,8 @@ DEFINE_CLK(0, "mcfpit.2", 34, MCF_BUSCLK);
> DEFINE_CLK(0, "mcfpit.3", 35, MCF_BUSCLK);
> DEFINE_CLK(0, "mcfeport.0", 36, MCF_CLK);
> DEFINE_CLK(0, "mcfadc.0", 37, MCF_CLK);
> -DEFINE_CLK(0, "mcfdac.0", 38, MCF_CLK);
> -DEFINE_CLK(0, "mcfdac.1", 39, MCF_CLK);
> +DEFINE_CLK(0, "mcf54415_dac.0", 38, MCF_CLK);
> +DEFINE_CLK(0, "mcf54415_dac.1", 39, MCF_CLK);
> DEFINE_CLK(0, "mcfrtc.0", 42, MCF_CLK);
> DEFINE_CLK(0, "mcfsim.0", 43, MCF_CLK);
> DEFINE_CLK(0, "mcfusb-otg.0", 44, MCF_CLK);
> @@ -106,8 +106,8 @@ static struct clk_lookup m5411x_clk_lookup[] = {
> CLKDEV_INIT("mcfpit.3", NULL, &__clk_0_35),
> CLKDEV_INIT("mcfeport.0", NULL, &__clk_0_36),
> CLKDEV_INIT("mcfadc.0", NULL, &__clk_0_37),
> - CLKDEV_INIT("mcfdac.0", NULL, &__clk_0_38),
> - CLKDEV_INIT("mcfdac.1", NULL, &__clk_0_39),
> + CLKDEV_INIT("mcf54415_dac.0", NULL, &__clk_0_38),
> + CLKDEV_INIT("mcf54415_dac.1", NULL, &__clk_0_39),
> CLKDEV_INIT("mcfrtc.0", NULL, &__clk_0_42),
> CLKDEV_INIT("mcfsim.0", NULL, &__clk_0_43),
> CLKDEV_INIT("mcfusb-otg.0", NULL, &__clk_0_44),
>
^ permalink raw reply
* Re: [PATCH v2] iio: adc: xilinx-ams: Replace spin_lock() and unlock() calls with guard(spinlock*)()
From: Salih Erim @ 2026-05-14 1:33 UTC (permalink / raw)
To: Maxwell Doose, conall.ogriofa, jic23, michal.simek
Cc: David Lechner, Nuno Sá, Andy Shevchenko,
open list:XILINX AMS DRIVER, moderated list:ARM/ZYNQ ARCHITECTURE,
open list, git
In-Reply-To: <20260508124513.17752-1-m32285159@gmail.com>
Hi Maxwell,
On 5/8/2026 1:45 PM, Maxwell Doose wrote:
> Include linux/cleanup.h to take advantage of RAII macros.
>
> Replace spin_lock() and unlock() calls with their RAII macro
> counterparts, which modernizes the code and increases readability.
>
> Remove "flags" variables where spin_lock_irqsave() has been replaced
> with guard(spinlock_irqsave)().
>
> Signed-off-by: Maxwell Doose <m32285159@gmail.com>
> ---
> v2:
> - Replace guard(spinlock_irq)() in ams_unmask_worker() with
> scoped_guard() per Jonathan's suggestion.
Nice cleanup, thanks for taking care of this.
Reviewed-by: Salih Erim <salih.erim@amd.com>
Salih.
^ permalink raw reply
* Re: [PATCH 2/3] usb: dwc3: xilinx: use reset_control_reset() in versal init
From: Thinh Nguyen @ 2026-05-14 1:34 UTC (permalink / raw)
To: Radhey Shyam Pandey
Cc: Thinh Nguyen, gregkh@linuxfoundation.org, michal.simek@amd.com,
p.zabel@pengutronix.de, linux-usb@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, git@amd.com
In-Reply-To: <20260511160814.2904882-3-radhey.shyam.pandey@amd.com>
On Mon, May 11, 2026, Radhey Shyam Pandey wrote:
> Replace separate reset_control_assert() and reset_control_deassert() calls
> with reset_control_reset(), which pulses the reset in one step. Report
> failures with dev_err_probe() and a single message. No functional change.
>
The behavior of reset_control_reset() is a little different. I wouldn't
call this "No functional change". However, I assumed this was tested.
Please provide a proper reason for this change in the change log.
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> ---
> drivers/usb/dwc3/dwc3-xilinx.c | 16 ++++------------
> 1 file changed, 4 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c
> index a3c7dc258c7d..94458b3da1a0 100644
> --- a/drivers/usb/dwc3/dwc3-xilinx.c
> +++ b/drivers/usb/dwc3/dwc3-xilinx.c
> @@ -98,18 +98,10 @@ static int dwc3_xlnx_init_versal(struct dwc3_xlnx *priv_data)
>
> dwc3_xlnx_mask_phy_rst(priv_data, false);
>
> - /* Assert and De-assert reset */
> - ret = reset_control_assert(crst);
> - if (ret < 0) {
> - dev_err_probe(dev, ret, "failed to assert Reset\n");
> - return ret;
> - }
> -
> - ret = reset_control_deassert(crst);
> - if (ret < 0) {
> - dev_err_probe(dev, ret, "failed to De-assert Reset\n");
> - return ret;
> - }
> + /* assert and deassert reset */
> + ret = reset_control_reset(crst);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to assert and deassert reset\n");
>
> dwc3_xlnx_mask_phy_rst(priv_data, true);
> dwc3_xlnx_set_coherency(priv_data, XLNX_USB2_TRAFFIC_ROUTE_CONFIG);
> --
> 2.44.4
>
^ permalink raw reply
* Re: [PATCH 3/3] usb: dwc3: xilinx: fix error handling in zynqmp init error paths
From: Thinh Nguyen @ 2026-05-14 1:43 UTC (permalink / raw)
To: Radhey Shyam Pandey
Cc: Thinh Nguyen, gregkh@linuxfoundation.org, michal.simek@amd.com,
p.zabel@pengutronix.de, linux-usb@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, git@amd.com, stable@vger.kernel.org
In-Reply-To: <20260511160814.2904882-4-radhey.shyam.pandey@amd.com>
On Mon, May 11, 2026, Radhey Shyam Pandey wrote:
> Fix error handling and resource cleanup i.e remove invalid
> phy_exit() after failed phy_init(), route failures through
> proper cleanup paths and return 0 explicitly on success.
>
> Fixes: 84770f028fab ("usb: dwc3: Add driver for Xilinx platforms")
> Cc: stable@vger.kernel.org
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> ---
> drivers/usb/dwc3/dwc3-xilinx.c | 27 +++++++++++++++------------
> 1 file changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c
> index 94458b3da1a0..b832505e1b04 100644
> --- a/drivers/usb/dwc3/dwc3-xilinx.c
> +++ b/drivers/usb/dwc3/dwc3-xilinx.c
> @@ -176,15 +176,13 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
> }
>
> ret = phy_init(priv_data->usb3_phy);
> - if (ret < 0) {
> - phy_exit(priv_data->usb3_phy);
> + if (ret < 0)
> goto err;
> - }
>
> ret = reset_control_deassert(apbrst);
> if (ret < 0) {
> dev_err(dev, "Failed to release APB reset\n");
> - goto err;
> + goto err_phy_exit;
> }
>
> if (priv_data->usb3_phy) {
> @@ -200,26 +198,24 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
> ret = reset_control_deassert(crst);
> if (ret < 0) {
> dev_err(dev, "Failed to release core reset\n");
> - goto err;
> + goto err_phy_exit;
> }
>
> ret = reset_control_deassert(hibrst);
> if (ret < 0) {
> dev_err(dev, "Failed to release hibernation reset\n");
> - goto err;
> + goto err_phy_exit;
> }
>
> ret = phy_power_on(priv_data->usb3_phy);
> - if (ret < 0) {
> - phy_exit(priv_data->usb3_phy);
> - goto err;
> - }
> + if (ret < 0)
> + goto err_phy_exit;
>
> /* ulpi reset via gpio-modepin or gpio-framework driver */
> reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> if (IS_ERR(reset_gpio)) {
> - return dev_err_probe(dev, PTR_ERR(reset_gpio),
> - "Failed to request reset GPIO\n");
> + ret = PTR_ERR(reset_gpio);
> + goto err_phy_power_off;
> }
>
> if (reset_gpio) {
> @@ -229,6 +225,13 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
> }
>
> dwc3_xlnx_set_coherency(priv_data, XLNX_USB_TRAFFIC_ROUTE_CONFIG);
> +
> + return 0;
> +
> +err_phy_power_off:
> + phy_power_off(priv_data->usb3_phy);
> +err_phy_exit:
> + phy_exit(priv_data->usb3_phy);
> err:
> return ret;
> }
> --
> 2.44.4
>
This fix should be a separate patch from this cleanup series.
Thanks,
Thinh
^ permalink raw reply
* Re: [net-next v8 2/3] net: ethernet: mtk_eth_soc: Add RSS support
From: Jakub Kicinski @ 2026-05-14 1:52 UTC (permalink / raw)
To: linux
Cc: Jakub Kicinski, nbd, lorenzo, andrew+netdev, davem, edumazet,
pabeni, matthias.bgg, angelogioacchino.delregno, linux,
mason-cw.chang, netdev, linux-kernel, daniel, linux-mediatek,
linux-arm-kernel
In-Reply-To: <20260509190938.169290-3-linux@fw-web.de>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
Subject: net: ethernet: mtk_eth_soc: Add RSS support
This commit adds Receive Side Scaling support for the mtk_eth_soc driver,
enabling up to four RX rings on MT7988 with ethtool -x and
--set-rxfh-indir support. It also reworks the RX NAPI / IRQ handling
into per-ring instances and renames eth->irq[] to eth->irq_fe[] /
eth->irq_pdma[].
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
[ ... ]
> @@ -2235,16 +2199,15 @@ static int mtk_poll_rx(struct napi_struct *napi, int budget,
[ ... ]
> @@ -2436,7 +2399,7 @@ static int mtk_poll_rx(struct napi_struct *napi, int budget,
> * we continue
> */
> wmb();
> - mtk_update_rx_cpu_idx(eth);
> + mtk_update_rx_cpu_idx(eth, ring);
> }
>
> eth->rx_packets += done;
[High]
With RSS enabled, up to MTK_RX_NAPI_NUM (=4) NAPI instances can run
concurrently on different CPUs. eth->rx_packets, eth->rx_bytes (here)
and eth->rx_events (incremented in mtk_handle_irq_rx with IRQF_SHARED)
are plain u32 fields with no atomic, no spinlock, and no per-CPU
storage:
eth->rx_packets += done;
eth->rx_bytes += bytes;
dim_update_sample(eth->rx_events, eth->rx_packets, eth->rx_bytes,
&dim_sample);
net_dim(ð->rx_dim, &dim_sample);
Pre-patch a single rx_napi serialised these accesses. Are these
unsynchronised cross-CPU updates intentional? The corrupted values
feed dim_update_sample() / net_dim() which drives interrupt
moderation, and eth->rx_dim is a single struct dim shared across all
RX NAPIs.
> @@ -2645,7 +2608,9 @@ static int mtk_napi_tx(struct napi_struct *napi, int budget)
>
> static int mtk_napi_rx(struct napi_struct *napi, int budget)
> {
> - struct mtk_eth *eth = container_of(napi, struct mtk_eth, rx_napi);
> + struct mtk_napi *rx_napi = container_of(napi, struct mtk_napi, napi);
> + struct mtk_eth *eth = rx_napi->eth;
> + struct mtk_rx_ring *ring = rx_napi->rx_ring;
> const struct mtk_reg_map *reg_map = eth->soc->reg_map;
> int rx_done_total = 0;
[Medium]
mtk_napi_rx() calls mtk_handle_status_irq(eth) on entry of every poll.
mtk_handle_status_irq() does an unprotected read / clear of
MTK_INT_STATUS2 and calls mtk_stats_update():
status2 = mtk_r32(eth, MTK_INT_STATUS2);
if (unlikely(status2 & (MTK_GDM1_AF | MTK_GDM2_AF))) {
mtk_stats_update(eth);
mtk_w32(eth, (MTK_GDM1_AF | MTK_GDM2_AF), MTK_INT_STATUS2);
}
With multiple rx_napi instances polling on different CPUs, can a fresh
AF event arriving between the read and the write be silently dropped,
or stats be double-counted? The per-MAC stats_lock taken inside
mtk_stats_update_mac does not synchronise the status read or the
clear.
[ ... ]
> @@ -3222,6 +3192,105 @@ static int mtk_hwlro_get_fdir_all(struct net_device *dev,
[ ... ]
> +static int mtk_rss_init(struct mtk_eth *eth)
> +{
> + const struct mtk_soc_data *soc = eth->soc;
> + const struct mtk_reg_map *reg_map = eth->soc->reg_map;
> + struct mtk_rss_params *rss_params = ð->rss_params;
> + u32 val;
> + int i;
> +
> + netdev_rss_key_fill(rss_params->hash_key, MTK_RSS_HASH_KEYSIZE);
> +
> + for (i = 0; i < MTK_RSS_MAX_INDIRECTION_TABLE; i++)
> + rss_params->indirection_table[i] = ethtool_rxfh_indir_default(i, eth->soc->rss_num);
> +
> + if (soc->rx.desc_size == sizeof(struct mtk_rx_dma)) {
> + /* Set RSS rings to PSE modes */
> + for (i = 1; i <= MTK_HW_LRO_RING_NUM(eth); i++) {
> + val = mtk_r32(eth, MTK_LRO_CTRL_DW2_CFG(reg_map, i));
> + val |= MTK_RING_PSE_MODE;
> + mtk_w32(eth, val, MTK_LRO_CTRL_DW2_CFG(reg_map, i));
> + }
> +
> + /* Enable non-lro multiple rx */
> + val = mtk_r32(eth, reg_map->pdma.lro_ctrl_dw0);
> + val |= MTK_NON_LRO_MULTI_EN;
> + mtk_w32(eth, val, reg_map->pdma.lro_ctrl_dw0);
> +
> + /* Enable RSS dly int supoort */
> + val |= MTK_LRO_DLY_INT_EN;
> + mtk_w32(eth, val, reg_map->pdma.lro_ctrl_dw0);
> + }
[Medium]
Is this block reachable on any SoC? mtk_rss_init() is only invoked
from mtk_dma_init() under MTK_HAS_CAPS(MTK_RSS). Only mt7988_data
sets MTK_RSS in caps, and mt7988_data uses
rx.desc_size = sizeof(struct mtk_rx_dma_v2), not
sizeof(struct mtk_rx_dma).
If the PSE-mode / MTK_NON_LRO_MULTI_EN / MTK_LRO_DLY_INT_EN
programming is needed on MT7988, is it being silently skipped? If it
was intended for mt7981 / mt7986, those SoCs never reach this path
because their caps lack MTK_RSS.
There is also a typo above: "RSS dly int supoort" -> "support".
[ ... ]
> + /* Set perRSS GRP INT */
> + mtk_m32(eth, MTK_RX_DONE_INT(eth, MTK_RSS_RING(1)),
> + MTK_RX_DONE_INT(eth, MTK_RSS_RING(1)), reg_map->pdma.int_grp);
[ ... ]
> +
> + return 0;
> +}
[ ... ]
> @@ -3312,6 +3381,17 @@ static int mtk_dma_init(struct mtk_eth *eth)
[ ... ]
> + if (MTK_HAS_CAPS(eth->soc->caps, MTK_RSS)) {
> + for (i = 1; i < MTK_RX_RSS_NUM(eth); i++) {
> + err = mtk_rx_alloc(eth, MTK_RSS_RING(i), MTK_RX_FLAGS_NORMAL);
> + if (err)
> + return err;
> + }
> + err = mtk_rss_init(eth);
> + if (err)
> + return err;
> + }
> +
[ ... ]
> @@ -3437,14 +3523,36 @@ static int mtk_get_irqs(struct platform_device *pdev, struct mtk_eth *eth)
> return 0;
> }
>
> -static irqreturn_t mtk_handle_irq_rx(int irq, void *_eth)
> +static int mtk_get_irqs_pdma(struct platform_device *pdev, struct mtk_eth *eth)
> {
> - struct mtk_eth *eth = _eth;
> + char rxring[] = "pdma0";
> + int i;
> +
> + for (i = 0; i < MTK_PDMA_IRQ_NUM; i++) {
> + rxring[4] = '0' + i;
> + eth->irq_pdma[i] = platform_get_irq_byname(pdev, rxring);
> + if (eth->irq_pdma[i] < 0)
> + return eth->irq_pdma[i];
> + }
> +
> + return 0;
> +}
[High]
This loop demands all four pdma0..pdma3 IRQs whenever
MTK_HAS_CAPS(MTK_PDMA_INT) is set, but the same patch adds
MTK_PDMA_INT to MT7981_CAPS and MT7986_CAPS — SoCs which do not
have MTK_RSS. On those SoCs only rx_napi[0] / pdma0 is ever
referenced (every pdma1..pdma3 use site is gated on
MTK_HAS_CAPS(MTK_RSS) in probe / open / stop / dma_init / dma_free).
Existing MT7981 / MT7986 device trees declare only fe1 / fe2. Does
this turn into a probe-time -ENXIO failure for those boards? Should
the call to mtk_get_irqs_pdma(), or at least the requests for
pdma1..pdma3, be folded under MTK_HAS_CAPS(MTK_RSS) so older DTs
continue to probe?
> +
> +static irqreturn_t mtk_handle_irq_rx(int irq, void *priv)
> +{
> + struct mtk_napi *rx_napi = priv;
> + struct mtk_eth *eth = rx_napi->eth;
> + struct mtk_rx_ring *ring = rx_napi->rx_ring;
>
> eth->rx_events++;
> - if (likely(napi_schedule_prep(ð->rx_napi))) {
> - mtk_rx_irq_disable(eth, eth->soc->rx.irq_done_mask);
> - __napi_schedule(ð->rx_napi);
> + if (unlikely(!(mtk_r32(eth, eth->soc->reg_map->pdma.irq_status) &
> + mtk_r32(eth, eth->soc->reg_map->pdma.irq_mask) &
> + MTK_RX_DONE_INT(eth, ring->ring_no))))
> + return IRQ_NONE;
[High]
Pre-patch mt7988_data set rx.irq_done_mask = MTK_RX_DONE_INT_V2 =
BIT(14). Post-patch the per-soc field is removed and replaced with
MTK_RX_DONE_INT(eth, ring_no), which for V3 (mt7988) expands to
BIT(24 + ring_no), so MTK_RX_DONE_INT(eth, 0) is now BIT(24) instead
of BIT(14).
Is the bit number change intentional? If MT7988 RX-done is actually
signalled on BIT(14), the new mask never matches, mtk_handle_irq_rx
returns IRQ_NONE, and RX traffic stalls. If BIT(14) was wrong all
along, could the commit message describe the silent fix? The patch
otherwise advertises only RSS.
[ ... ]
> @@ -3888,9 +4011,7 @@ static void mtk_dim_rx(struct work_struct *work)
[ ... ]
> - val = mtk_r32(eth, reg_map->pdma.delay_irq);
> - val &= MTK_PDMA_DELAY_TX_MASK;
> - val |= MTK_PDMA_DELAY_RX_EN;
> + val = MTK_PDMA_DELAY_RX_EN;
[ ... ]
> - mtk_w32(eth, val, reg_map->pdma.delay_irq);
> if (MTK_HAS_CAPS(eth->soc->caps, MTK_QDMA))
> - mtk_w32(eth, val, reg_map->qdma.delay_irq);
> + mtk_m32(eth, MTK_PDMA_DELAY_TX_MASK,
> + val << MTK_PDMA_DELAY_TX_PTIME_SHIFT, reg_map->qdma.delay_irq);
> +
> + if (eth->soc->rx.desc_size == sizeof(struct mtk_rx_dma)) {
> + mtk_m32(eth, MTK_PDMA_DELAY_RX_MASK, val, reg_map->pdma.delay_irq);
> + mtk_w32(eth, val, reg_map->pdma.lro_rx1_dly_int);
> + mtk_w32(eth, val, reg_map->pdma.lro_rx1_dly_int + 0x4);
> + mtk_w32(eth, val, reg_map->pdma.lro_rx1_dly_int + 0x8);
> + } else {
> + val = val | (val << MTK_PDMA_DELAY_RX_RING_SHIFT);
> + mtk_w32(eth, val, reg_map->pdma.rx_delay_irq);
> + }
[Critical]
The new branch is gated on rx.desc_size, not on a capability bit.
rt5350_data (MT7628) has rx.desc_size = sizeof(struct mtk_rx_dma) and
uses mt7628_reg_map, which does not initialise lro_rx1_dly_int (it
defaults to 0).
mtk_hw_init() calls mtk_dim_rx(ð->rx_dim.work) inside the MT7628
branch, and net_dim invokes it again on every moderation update. Each
call therefore performs:
mtk_w32(eth, val, 0);
mtk_w32(eth, val, 0x4);
mtk_w32(eth, val, 0x8);
These offsets land in the Frame Engine global config region
(MTK_FE_GLO_CFG(0) = 0x00 etc.) instead of any LRO/RSS register, with
val containing MTK_PDMA_DELAY_RX_EN = BIT(15). Can this corrupt
MT7628 frame-engine state on every boot and every NAPI moderation
update? Should the branch be gated on MTK_HWLRO or MTK_RSS rather
than on rx.desc_size?
[Medium]
On V3 hardware (the only chip enabling RSS) only two ring slots are
programmed here:
val = val | (val << MTK_PDMA_DELAY_RX_RING_SHIFT);
mtk_w32(eth, val, reg_map->pdma.rx_delay_irq);
With MTK_PDMA_DELAY_RX_RING_SHIFT = 16 the register holds two rings,
but RSS uses MTK_RX_RSS_NUM(eth) = 4 rings. Do rings 2 and 3 ever
receive DIM-driven moderation updates after probe, or do they stay at
the hardware default?
[ ... ]
> @@ -4149,6 +4285,25 @@ static void mtk_hw_reset_monitor_work(struct work_struct *work)
[ ... ]
> +static int mtk_napi_init(struct mtk_eth *eth)
> +{
[ ... ]
> +}
[ ... ]
> @@ -4238,12 +4393,11 @@ static int mtk_hw_init(struct mtk_eth *eth, bool reset)
[ ... ]
> @@ -4700,8 +4860,13 @@ static void mtk_get_ethtool_stats(struct net_device *dev,
>
> static u32 mtk_get_rx_ring_count(struct net_device *dev)
> {
> + struct mtk_mac *mac = netdev_priv(dev);
> + struct mtk_eth *eth = mac->hw;
> +
> if (dev->hw_features & NETIF_F_LRO)
> return MTK_MAX_RX_RING_NUM;
> + else if (MTK_HAS_CAPS(eth->soc->caps, MTK_RSS))
> + return MTK_RX_RSS_NUM(eth);
>
> return 0;
> }
[ ... ]
> @@ -4784,6 +4949,70 @@ static int mtk_set_eee(struct net_device *dev, struct ethtool_keee *eee)
[ ... ]
> +static int mtk_set_rxfh(struct net_device *dev, struct ethtool_rxfh_param *rxfh,
> + struct netlink_ext_ack *extack)
> +{
> + struct mtk_mac *mac = netdev_priv(dev);
> + struct mtk_eth *eth = mac->hw;
> + struct mtk_rss_params *rss_params = ð->rss_params;
> + const struct mtk_reg_map *reg_map = eth->soc->reg_map;
> + int i;
> +
> + if (rxfh->hfunc != ETH_RSS_HASH_NO_CHANGE &&
> + rxfh->hfunc != ETH_RSS_HASH_TOP)
> + return -EOPNOTSUPP;
> +
> + if (rxfh->key) {
> + memcpy(rss_params->hash_key, rxfh->key,
> + sizeof(rss_params->hash_key));
> + for (i = 0; i < MTK_RSS_HASH_KEYSIZE / sizeof(u32); i++)
> + mtk_w32(eth, rss_params->hash_key[i],
> + MTK_RSS_HASH_KEY_DW(reg_map, i));
> + }
> +
> + if (rxfh->indir) {
> + for (i = 0; i < MTK_RSS_MAX_INDIRECTION_TABLE; i++)
> + rss_params->indirection_table[i] = rxfh->indir[i];
> + for (i = 0; i < MTK_RSS_MAX_INDIRECTION_TABLE / 16; i++)
> + mtk_w32(eth, mtk_rss_indr_table(rss_params, i),
> + MTK_RSS_INDR_TABLE_DW(reg_map, i));
> + }
> +
> + return 0;
> +}
[High]
Should this op (and .get_rxfh / .get_rxfh_key_size /
.get_rxfh_indir_size) be gated on MTK_HAS_CAPS(MTK_RSS)? As written,
mtk_ethtool_ops exposes them unconditionally for every SoC the driver
supports.
MTK_RSS_HASH_KEY_DW(reg_map, x) and MTK_RSS_INDR_TABLE_DW(reg_map, x)
are computed as (reg_map)->pdma.rss_glo_cfg + 0x20 + ... and + 0x50 +
... . mtk_reg_map (mt2701/mt7621/mt7622/mt7623/mt7629) and
mt7628_reg_map do not initialise rss_glo_cfg, so it defaults to 0.
A CAP_NET_ADMIN user invoking ethtool -X dev hkey ... on those SoCs
would write user-controlled u32 values into absolute frame-engine
offsets 0x20..0x44 (and indirection writes to 0x50..0x6c). Can these
ops be made conditional on MTK_RSS, or filled in only for the RSS-
capable SoC?
[Medium]
mtk_set_rxfh() does not validate user-supplied indir[i] entries
against MTK_RX_RSS_NUM(eth). The packing helper allots only 2 bits
per entry:
val |= (rss_params->indirection_table[i] << (2 * (i % 16)));
ethtool core only enforces indir[i] < num_rx_rings when
num_rx_rings is non-zero, and MTK_RSS hardware has at most 4 rings.
Should the driver reject values >= MTK_RX_RSS_NUM(eth) so they don't
silently bleed across the 2-bit slots in the packed register? The
indirection table also stores the raw u32 into a u8 field, which
truncates 4..255 instead of catching them.
[Medium]
mtk_rss_init() wraps key / indirection writes with an
MTK_RSS_CFG_REQ pause / release handshake:
/* Pause */
val |= MTK_RSS_CFG_REQ;
mtk_w32(eth, val, reg_map->pdma.rss_glo_cfg);
...
/* Release pause */
val &= ~(MTK_RSS_CFG_REQ);
mtk_set_rxfh() writes the same registers at runtime without that
handshake. Can the hardware sample partially-updated key /
indirection state and steer packets to incorrect or out-of-range
rings while the write loop is in flight?
[ ... ]
> @@ -5424,6 +5708,7 @@ static const struct mtk_soc_data mt2701_data = {
[ ... entries for mt2701/mt7621/mt7622/mt7623/mt7629/rt5350 ... ]
> @@ -5567,16 +5851,16 @@ static const struct mtk_soc_data mt7981_data = {
[ ... ]
> .foe_entry_size = MTK_FOE_ENTRY_V2_SIZE,
> + .rss_num = 4,
[ ... ]
> @@ -5597,6 +5881,7 @@ static const struct mtk_soc_data mt7986_data = {
[ ... ]
> .foe_entry_size = MTK_FOE_ENTRY_V2_SIZE,
> + .rss_num = 4,
[Low]
mt7981_data and mt7986_data set rss_num = 4, but MT7981_CAPS and
MT7986_CAPS do not include MTK_RSS (only MT7988_CAPS does). Every
RSS code path is gated on MTK_HAS_CAPS(MTK_RSS) before consulting
rss_num, so this has no current effect, but the SoC capability table
becomes internally inconsistent. Should rss_num be 0 on these SoCs
until MTK_RSS is actually wired up and tested for them?
> @@ -5627,20 +5911,20 @@ static const struct mtk_soc_data mt7988_data = {
[ ... ]
> .foe_entry_size = MTK_FOE_ENTRY_V3_SIZE,
> + .rss_num = 4,
[ ... ]
> .rx = {
> .desc_size = sizeof(struct mtk_rx_dma_v2),
> - .irq_done_mask = MTK_RX_DONE_INT_V2,
> .dma_l4_valid = RX_DMA_L4_VALID_V2,
> .dma_max_len = MTK_TX_DMA_BUF_LEN_V2,
> .dma_len_offset = 8,
> - .dma_size = MTK_DMA_SIZE(2K),
> + .dma_size = MTK_DMA_SIZE(1K),
> },
> };
[ ... ]
--
pw-bot: cr
^ permalink raw reply
* Re: [net-next v8 3/3] net: ethernet: mtk_eth_soc: Add LRO support
From: Jakub Kicinski @ 2026-05-14 1:52 UTC (permalink / raw)
To: linux
Cc: Jakub Kicinski, nbd, lorenzo, andrew+netdev, davem, edumazet,
pabeni, matthias.bgg, angelogioacchino.delregno, linux,
mason-cw.chang, netdev, linux-kernel, daniel, linux-mediatek,
linux-arm-kernel
In-Reply-To: <20260509190938.169290-4-linux@fw-web.de>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
Subject: net: ethernet: mtk_eth_soc: Add LRO support
This patch adds Hardware Large Receive Offload support to the
mediatek ethernet driver and enables it on MT7988 by adding
MTK_HWLRO to MT7988_CAPS. The patch parameterizes the LRO
register accessors and ring counts on the SoC version, and adds
LRO-specific NAPI/IRQ wiring.
A few questions on the changes below.
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> index 908fd88287ac..8035fc2557de 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> @@ -2806,7 +2806,7 @@ static int mtk_rx_alloc(struct mtk_eth *eth, int ring_no, int rx_flag)
> if (!ring->data)
> return -ENOMEM;
>
> - if (mtk_page_pool_enabled(eth)) {
> + if (mtk_page_pool_enabled(eth) && rcu_access_pointer(eth->prog)) {
> struct page_pool *pp;
>
> pp = mtk_create_page_pool(eth, &ring->xdp_q, ring_no,
[High]
Could the rationale for this gating change be explained in the commit
message? The subject is "Add LRO support" but this hunk changes the
page_pool creation policy for every netsys v2+ chip.
Before the patch, every v2+ RX ring was backed by a page_pool (since
mtk_page_pool_enabled() returns mtk_is_netsys_v2_or_greater(eth)).
After the patch, a page_pool is only created when an XDP program is
already attached at ring allocation time.
mtk_xdp_setup() also rejects XDP attach with -EOPNOTSUPP when
eth->hwlro is true:
if (eth->hwlro) {
NL_SET_ERR_MSG_MOD(extack, "HW LRO and XDP are mutually exclusive");
return -EOPNOTSUPP;
}
So on MT7988 (where this patch enables MTK_HWLRO) eth->prog is
permanently NULL and mtk_create_page_pool() is now never reached
for any RX ring, including the non-LRO ring 0.
Does this intentionally disable page_pool/DMA buffer recycling on
MT7988, and on every other v2+ chip until a BPF program is loaded?
mtk_ethtool_pp_stats() depends on ring->page_pool, so the page-pool
counters reported through ethtool would also become empty.
The driver still advertises NETDEV_XDP_ACT_BASIC|REDIRECT|NDO_XMIT|
NDO_XMIT_SG via xdp_features for v2+, which appears to diverge from
the buffer-pool capability after this change.
> @@ -2974,30 +2975,50 @@ static int mtk_hwlro_rx_init(struct mtk_eth *eth)
> ring_ctrl_dw2 |= MTK_RING_MAX_AGG_CNT_L;
> ring_ctrl_dw3 |= MTK_RING_MAX_AGG_CNT_H;
>
> - for (i = 1; i < MTK_MAX_RX_RING_NUM; i++) {
> + for (i = 1; i <= MTK_HW_LRO_RING_NUM(eth); i++) {
> mtk_w32(eth, ring_ctrl_dw1, MTK_LRO_CTRL_DW1_CFG(reg_map, i));
[ ... ]
> /* set refresh timer for altering flows to 1 sec. (unit: 20us) */
> mtk_w32(eth, (MTK_HW_LRO_TIMER_UNIT << 16) | MTK_HW_LRO_REFRESH_TIME,
> MTK_PDMA_LRO_ALT_REFRESH_TIMER);
[Medium]
Should MTK_PDMA_LRO_ALT_REFRESH_TIMER also have been converted to
go through reg_map like every other LRO register touched by this
patch? Looking at the header, it is still defined as a hardcoded
absolute offset 0x1c that has lived in the v1 PDMA window since
2016. On MT7988 (newly added to MTK_HWLRO via MT7988_CAPS) the
LRO block lives at 0x6c08-0x6c44 per mt7988_reg_map, while offset
0x1c on v3 falls in the Frame Engine register area between
MTK_FE_INT_ENABLE and MTK_FE_INT_GRP.
Every other LRO register access in this function was switched to
MTK_PDMA_LRO_CTRL_DW0(reg_map), MTK_PDMA_LRO_ALT_SCORE_DELTA(reg_map),
MTK_LRO_DIP_DW0_CFG(reg_map, x) and so on. Was the omission of
this one register intentional, or should it be a similar
reg_map-based accessor so that on MT7988 the auto-learn refresh
timer is programmed in the LRO block instead of in the FE engine
region?
> - /* set HW LRO mode & the max aggregation count for rx packets */
> - lro_ctrl_dw3 |= MTK_ADMA_MODE | (MTK_HW_LRO_MAX_AGG_CNT & 0xff);
> + if (mtk_is_netsys_v3_or_greater(eth)) {
[ ... ]
> /* enable HW LRO */
> lro_ctrl_dw0 |= MTK_LRO_EN;
>
> - mtk_w32(eth, lro_ctrl_dw3, MTK_PDMA_LRO_CTRL_DW3);
> - mtk_w32(eth, lro_ctrl_dw0, MTK_PDMA_LRO_CTRL_DW0);
> + mtk_w32(eth, lro_ctrl_dw3, MTK_PDMA_LRO_CTRL_DW3(reg_map));
> + mtk_w32(eth, lro_ctrl_dw0, MTK_PDMA_LRO_CTRL_DW0(reg_map));
> +
> + if (mtk_is_netsys_v2_or_greater(eth)) {
> + i = (soc->rx.desc_size == sizeof(struct mtk_rx_dma_v2)) ? 1 : 0;
> + mtk_m32(eth, MTK_RX_DONE_INT(eth, MTK_HW_LRO_RING(eth, i)),
> + MTK_RX_DONE_INT(eth, MTK_HW_LRO_RING(eth, i)), reg_map->pdma.int_grp);
> + mtk_m32(eth, MTK_RX_DONE_INT(eth, MTK_HW_LRO_RING(eth, i + 1)),
> + MTK_RX_DONE_INT(eth, MTK_HW_LRO_RING(eth, i + 1)),
> + reg_map->pdma.int_grp + 0x4);
> + mtk_m32(eth, MTK_RX_DONE_INT(eth, MTK_HW_LRO_RING(eth, i + 2)),
> + MTK_RX_DONE_INT(eth, MTK_HW_LRO_RING(eth, i + 2)), reg_map->pdma.int_grp3);
> + }
[Medium]
On v3 (the only chip with both MTK_HWLRO and v2+ enabled by this
patch), the desc_size check yields i = 1, so this block routes
MTK_HW_LRO_RING(eth, 1..3) — physical rings 5, 6, 7 with done bits
BIT(29..31) — into int_grp, int_grp+0x4 and int_grp3 respectively.
What routes MTK_HW_LRO_RING(eth, 0), i.e. physical ring 4 with done
bit BIT(28)?
mtk_probe() registers an IRQ handler for all MTK_HW_LRO_RING_NUM(eth)
= 4 LRO rings using IRQF_SHARED, so a NAPI is attached for ring 4 on
eth->irq_pdma[MTK_HW_LRO_IRQ(eth, 0)] (= irq_pdma[0] on v3). Without
ring 4's BIT(28) being OR'd into any int_grp register, does ring 4's
RX interrupt actually reach NAPI, or does that depend on hardware
reset defaults of int_grp on MT7988?
> return 0;
> }
> @@ -3080,6 +3112,65 @@ static int mtk_hwlro_get_ip_cnt(struct mtk_mac *mac)
> return cnt;
> }
>
> +static int mtk_hwlro_add_ipaddr_idx(struct net_device *dev, u32 ip4dst)
> +{
> + struct mtk_mac *mac = netdev_priv(dev);
> + const struct mtk_reg_map *reg_map;
> + struct mtk_eth *eth = mac->hw;
> + u32 reg_val;
> + int i;
> +
> + reg_map = eth->soc->reg_map;
> +
> + /* check for duplicate IP address in the current DIP list */
> + for (i = 1; i <= MTK_HW_LRO_DIP_NUM(eth); i++) {
> + reg_val = mtk_r32(eth, MTK_LRO_DIP_DW0_CFG(reg_map, i));
> + if (reg_val == ip4dst)
> + break;
> + }
> +
> + if (i < MTK_HW_LRO_DIP_NUM(eth) + 1) {
> + netdev_warn(dev, "Duplicate IP address at DIP(%d)!\n", i);
> + return -EEXIST;
> + }
> +
> + /* find out available DIP index */
> + for (i = 1; i <= MTK_HW_LRO_DIP_NUM(eth); i++) {
> + reg_val = mtk_r32(eth, MTK_LRO_DIP_DW0_CFG(reg_map, i));
> + if (reg_val == 0UL)
> + break;
> + }
> +
> + if (i >= MTK_HW_LRO_DIP_NUM(eth) + 1) {
> + netdev_warn(dev, "DIP index is currently out of resource!\n");
> + return -EBUSY;
> + }
> +
> + return i;
> +}
[High]
Is it intentional that the DIP-table allocation policy is changed
from a per-MAC partition (previously hwlro_idx = mac->id *
MTK_MAX_LRO_IP_CNT + fsp->location) to a single global pool searched
across all MACs? The duplicate check and free-slot search above
both iterate every DIP register without scoping by mac->id.
Two observations:
1. Two netdevs that both want HW LRO on the same destination IP
collide on the duplicate-check and the second one fails with
-EEXIST. A single MAC can also fill all DIP slots and starve
the other; non-v3 silicon has only 3 slots while the uAPI
advertises 2 MACs * MTK_MAX_LRO_IP_CNT = 4 rules.
2. mtk_hwlro_add_ipaddr() (below) does not invalidate any
pre-existing entry at mac->hwlro_ip[fsp->location] before
calling this helper, so adding a new IP at a location that
already had one finds a fresh slot for the new IP and then
overwrites the software state. The old slot still has the
original IP with MYIP_VLD set in hardware, and a later del at
the same location only finds and invalidates the new entry.
Together they look like a CAP_NET_ADMIN-reachable way to leak DIP
slots until device reset, while mac->hwlro_ip[] looks consistent.
> +static int mtk_hwlro_get_ipaddr_idx(struct net_device *dev, u32 ip4dst)
> +{
> + struct mtk_mac *mac = netdev_priv(dev);
> + struct mtk_eth *eth = mac->hw;
> + u32 reg_val;
> + int i;
> +
> + /* find out DIP index that matches the given IP address */
> + for (i = 1; i <= MTK_HW_LRO_DIP_NUM(eth); i++) {
> + reg_val = mtk_r32(eth, MTK_LRO_DIP_DW0_CFG(eth->soc->reg_map, i));
> + if (reg_val == ip4dst)
> + break;
> + }
> +
> + if (i >= MTK_HW_LRO_DIP_NUM(eth) + 1) {
> + netdev_warn(dev, "DIP address is not exist!\n");
> + return -ENOENT;
> + }
> +
> + return i;
> +}
> +
> static int mtk_hwlro_add_ipaddr(struct net_device *dev,
> struct ethtool_rxnfc *cmd)
> {
> @@ -3112,13 +3207,17 @@ static int mtk_hwlro_del_ipaddr(struct net_device *dev,
> struct mtk_mac *mac = netdev_priv(dev);
> struct mtk_eth *eth = mac->hw;
> int hwlro_idx;
> + u32 ip4dst;
>
> if (fsp->location > 1)
> return -EINVAL;
>
> - mac->hwlro_ip[fsp->location] = 0;
> - hwlro_idx = (mac->id * MTK_MAX_LRO_IP_CNT) + fsp->location;
> + ip4dst = mac->hwlro_ip[fsp->location];
> + hwlro_idx = mtk_hwlro_get_ipaddr_idx(dev, ip4dst);
> + if (hwlro_idx < 0)
> + return hwlro_idx;
>
> + mac->hwlro_ip[fsp->location] = 0;
> mac->hwlro_ip_cnt = mtk_hwlro_get_ip_cnt(mac);
>
> mtk_hwlro_inval_ipaddr(eth, hwlro_idx);
[Medium]
Should this path validate that mac->hwlro_ip[fsp->location] is
non-zero before passing it to mtk_hwlro_get_ipaddr_idx()? When the
location was never populated (or was already deleted), ip4dst is 0.
mtk_hwlro_get_ipaddr_idx() then matches the first slot whose register
reads 0 — i.e. an arbitrary unrelated empty slot — and returns a
positive index, after which mtk_hwlro_inval_ipaddr() clears MYIP_VLD
on that unrelated slot and the call returns 0 to userspace.
Either rejecting the call with -ENOENT here when
mac->hwlro_ip[fsp->location] == 0, or having mtk_hwlro_get_ipaddr_idx()
refuse ip4dst == 0, would avoid that.
> @@ -3126,6 +3225,24 @@ static int mtk_hwlro_del_ipaddr(struct net_device *dev,
> return 0;
> }
>
> +static void mtk_hwlro_netdev_enable(struct net_device *dev)
> +{
> + struct mtk_mac *mac = netdev_priv(dev);
> + struct mtk_eth *eth = mac->hw;
> + int i, hwlro_idx;
> +
> + for (i = 0; i < MTK_MAX_LRO_IP_CNT; i++) {
> + if (mac->hwlro_ip[i] == 0)
> + continue;
> +
> + hwlro_idx = mtk_hwlro_get_ipaddr_idx(dev, mac->hwlro_ip[i]);
> + if (hwlro_idx < 0)
> + continue;
> +
> + mtk_hwlro_val_ipaddr(eth, hwlro_idx, mac->hwlro_ip[i]);
> + }
> +}
> +
[Low]
This isn't a bug, but is mtk_hwlro_netdev_enable() reachable in
practice? It uses mtk_hwlro_get_ipaddr_idx(), which only succeeds
when the IP is already programmed in hardware. The only path that
disables LRO is mtk_hwlro_netdev_disable() below, which clears
mac->hwlro_ip[i] = 0 (so the first guard always continues) and also
invalidates the DIP entries. After ethtool -K lro off the IPs are
gone from both software and hardware, so the subsequent ethtool -K
lro on appears to do nothing.
If the intent is restore-on-re-enable, should the disable preserve
mac->hwlro_ip[] and/or should this enable path call
mtk_hwlro_add_ipaddr_idx() to allocate fresh slots?
> static void mtk_hwlro_netdev_disable(struct net_device *dev)
> {
> struct mtk_mac *mac = netdev_priv(dev);
> @@ -3133,8 +3250,14 @@ static void mtk_hwlro_netdev_disable(struct net_device *dev)
> int i, hwlro_idx;
>
> for (i = 0; i < MTK_MAX_LRO_IP_CNT; i++) {
> + if (mac->hwlro_ip[i] == 0)
> + continue;
> +
> + hwlro_idx = mtk_hwlro_get_ipaddr_idx(dev, mac->hwlro_ip[i]);
> + if (hwlro_idx < 0)
> + continue;
> +
> mac->hwlro_ip[i] = 0;
> - hwlro_idx = (mac->id * MTK_MAX_LRO_IP_CNT) + i;
>
> mtk_hwlro_inval_ipaddr(eth, hwlro_idx);
> }
[Medium]
Can this leave mac->hwlro_ip[i] populated while mac->hwlro_ip_cnt is
reset to 0? When mtk_hwlro_get_ipaddr_idx() returns < 0 (for example
because the DIP table was already cleared by a prior
mtk_hwlro_rx_uninit, by a parallel teardown on another MAC, or by a
hardware reset), the continue skips the mac->hwlro_ip[i] = 0
assignment. After the loop, mac->hwlro_ip_cnt = 0 still runs
unconditionally.
The previous code unconditionally cleared mac->hwlro_ip[i]. In the
new shape, ETHTOOL_GRXCLSRLCNT would report 0 while
ETHTOOL_GRXCLSRLALL via mtk_hwlro_get_fdir_all() still enumerates
the stale entries, and mtk_hwlro_get_ip_cnt() called from a later
add would recount them.
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> index 378cf47913ef..f7e7299fef6b 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> @@ -35,7 +35,7 @@
> #define MTK_DMA_SIZE(x) (SZ_##x)
> #define MTK_FQ_DMA_HEAD 32
> #define MTK_FQ_DMA_LENGTH 2048
> -#define MTK_RX_ETH_HLEN (ETH_HLEN + ETH_FCS_LEN)
> +#define MTK_RX_ETH_HLEN (VLAN_ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN)
> #define MTK_RX_HLEN (NET_SKB_PAD + MTK_RX_ETH_HLEN + NET_IP_ALIGN)
[High]
Could this change be called out in the commit message? It widens
MTK_RX_ETH_HLEN from 18 to 26 globally for every supported chip,
not just MT7988/LRO.
mtk_add_mac() sets:
eth->netdev[id]->max_mtu = MTK_MAX_RX_LENGTH_2K - MTK_RX_ETH_HLEN;
(and MTK_MAX_RX_LENGTH - MTK_RX_ETH_HLEN for MT7628), so the
maximum MTU advertised by every supported MediaTek SoC drops by 8
bytes (e.g. 2030 -> 2022 on the 2K path, 1518 -> 1510 on MT7628).
Userspace configuration that previously set MTU values up to the
prior limit will now be rejected with -EINVAL.
If headroom for stacked VLANs is needed only on the LRO path, would
it be possible to confine the +8 to MTK_MAX_LRO_RX_LENGTH instead
of changing MTK_RX_ETH_HLEN for every chip?
^ permalink raw reply
* Re: [net-next v8 3/3] net: ethernet: mtk_eth_soc: Add LRO support
From: Jakub Kicinski @ 2026-05-14 1:53 UTC (permalink / raw)
To: Frank Wunderlich
Cc: Felix Fietkau, Lorenzo Bianconi, Andrew Lunn, David S. Miller,
Eric Dumazet, Paolo Abeni, Matthias Brugger,
AngeloGioacchino Del Regno, Russell King, Frank Wunderlich,
netdev, linux-kernel, linux-arm-kernel, linux-mediatek,
Mason Chang, Daniel Golle
In-Reply-To: <20260509190938.169290-4-linux@fw-web.de>
On Sat, 9 May 2026 21:09:32 +0200 Frank Wunderlich wrote:
> + mtk_rx_irq_disable(eth, MTK_RX_DONE_INT(eth, MTK_HW_LRO_RING(eth, i)));
> + napi_synchronize(ð->rx_napi[MTK_HW_LRO_RING(eth, i)].napi);
> + napi_disable(ð->rx_napi[MTK_HW_LRO_RING(eth, i)].napi);
What purpose that napi_synchronize() serve?
Also we don't charge for temporary variables, maybe save that
MTK_HW_LRO_RING(eth, i) to make this slightly more readable.
^ permalink raw reply
* Re: [net-next v8 2/3] net: ethernet: mtk_eth_soc: Add RSS support
From: Jakub Kicinski @ 2026-05-14 1:56 UTC (permalink / raw)
To: Frank Wunderlich
Cc: Felix Fietkau, Lorenzo Bianconi, Andrew Lunn, David S. Miller,
Eric Dumazet, Paolo Abeni, Matthias Brugger,
AngeloGioacchino Del Regno, Russell King, Frank Wunderlich,
netdev, linux-kernel, linux-arm-kernel, linux-mediatek,
Mason Chang, Daniel Golle
In-Reply-To: <20260509190938.169290-3-linux@fw-web.de>
On Sat, 9 May 2026 21:09:31 +0200 Frank Wunderlich wrote:
> From: Mason Chang <mason-cw.chang@mediatek.com>
>
> Add support for Receive Side Scaling.
>
> We can adjust SMP affinity with the following command:
> echo [CPU bitmap num] > /proc/irq/[virtual IRQ ID]/smp_affinity,
> with interrupts evenly assigned to 4 CPUs, we were able to measure
> an RX throughput of 7.3Gbps using iperf3 on the MT7988. Further
> optimizations will be carried out in the future.
Would be great to split this up a little more for ease of review.
> +static int mtk_rss_init(struct mtk_eth *eth)
> +{
> + const struct mtk_soc_data *soc = eth->soc;
> + const struct mtk_reg_map *reg_map = eth->soc->reg_map;
> + struct mtk_rss_params *rss_params = ð->rss_params;
reverse xmas tree should be followed, please fix everywhere in this
submission
> + u32 val;
> + int i;
> +
> + netdev_rss_key_fill(rss_params->hash_key, MTK_RSS_HASH_KEYSIZE);
> +
> + for (i = 0; i < MTK_RSS_MAX_INDIRECTION_TABLE; i++)
> + rss_params->indirection_table[i] = ethtool_rxfh_indir_default(i, eth->soc->rss_num);
> +
> + if (soc->rx.desc_size == sizeof(struct mtk_rx_dma)) {
> + /* Set RSS rings to PSE modes */
> + for (i = 1; i <= MTK_HW_LRO_RING_NUM(eth); i++) {
> + val = mtk_r32(eth, MTK_LRO_CTRL_DW2_CFG(reg_map, i));
> + val |= MTK_RING_PSE_MODE;
> + mtk_w32(eth, val, MTK_LRO_CTRL_DW2_CFG(reg_map, i));
> + }
> +
> + /* Enable non-lro multiple rx */
> + val = mtk_r32(eth, reg_map->pdma.lro_ctrl_dw0);
> + val |= MTK_NON_LRO_MULTI_EN;
> + mtk_w32(eth, val, reg_map->pdma.lro_ctrl_dw0);
> +
> + /* Enable RSS dly int supoort */
> + val |= MTK_LRO_DLY_INT_EN;
> + mtk_w32(eth, val, reg_map->pdma.lro_ctrl_dw0);
> + }
> +
> + /* Hash Type */
> + val = mtk_r32(eth, reg_map->pdma.rss_glo_cfg);
> + val |= MTK_RSS_IPV4_STATIC_HASH;
> + val |= MTK_RSS_IPV6_STATIC_HASH;
> + mtk_w32(eth, val, reg_map->pdma.rss_glo_cfg);
> +
> + /* Hash Key */
> + for (i = 0; i < MTK_RSS_HASH_KEYSIZE / sizeof(u32); i++)
> + mtk_w32(eth, rss_params->hash_key[i], MTK_RSS_HASH_KEY_DW(reg_map, i));
> +
> + /* Select the size of indirection table */
> + for (i = 0; i < MTK_RSS_MAX_INDIRECTION_TABLE / 16; i++)
> + mtk_w32(eth, mtk_rss_indr_table(rss_params, i),
> + MTK_RSS_INDR_TABLE_DW(reg_map, i));
> +
> + /* Pause */
> + val |= MTK_RSS_CFG_REQ;
> + mtk_w32(eth, val, reg_map->pdma.rss_glo_cfg);
> +
> + /* Enable RSS */
> + val |= MTK_RSS_EN;
> + mtk_w32(eth, val, reg_map->pdma.rss_glo_cfg);
> +
> + /* Release pause */
> + val &= ~(MTK_RSS_CFG_REQ);
> + mtk_w32(eth, val, reg_map->pdma.rss_glo_cfg);
> +
> + /* Set perRSS GRP INT */
> + mtk_m32(eth, MTK_RX_DONE_INT(eth, MTK_RSS_RING(1)),
> + MTK_RX_DONE_INT(eth, MTK_RSS_RING(1)), reg_map->pdma.int_grp);
> + mtk_m32(eth, MTK_RX_DONE_INT(eth, MTK_RSS_RING(2)),
> + MTK_RX_DONE_INT(eth, MTK_RSS_RING(2)), reg_map->pdma.int_grp + 0x4);
> + mtk_m32(eth, MTK_RX_DONE_INT(eth, MTK_RSS_RING(3)),
> + MTK_RX_DONE_INT(eth, MTK_RSS_RING(3)), reg_map->pdma.int_grp3);
> +
> + return 0;
> +}
> +/* struct mtk_rss_params - This is the structure holding parameters
> + * for the RSS ring
> + * @hash_key The element is used to record the
> + * secret key for the RSS ring
> + * indirection_table The element is used to record the
> + * indirection table for the RSS ring
> + */
Quite odd looking comment. Having the right side aligned like that
makes it header to correlate where doc for fields start.
And there's @ missing for indirection_table.
^ permalink raw reply
* Re: Re: [PATCH net-next v7 2/4] net: stmmac: eic7700: enable clocks before syscon access and correct RX sampling timing
From: 李志 @ 2026-05-14 2:25 UTC (permalink / raw)
To: Jakub Kicinski
Cc: andrew+netdev, devicetree, davem, edumazet, robh, krzk+dt,
conor+dt, netdev, pabeni, mcoquelin.stm32, alexandre.torgue,
rmk+kernel, pjw, palmer, aou, alex, linux-riscv, linux-stm32,
linux-arm-kernel, linux-kernel, maxime.chevallier, ningyu, linmin,
pinkesh.vaghela, pritesh.patel, weishangjuan, horms
In-Reply-To: <20260512161315.141aba88@kernel.org>
> -----Original Messages-----
> From: "Jakub Kicinski" <kuba@kernel.org>
> Send time:Wednesday, 13/05/2026 07:13:15
> To: 李志 <lizhi2@eswincomputing.com>
> Cc: andrew+netdev@lunn.ch, devicetree@vger.kernel.org, davem@davemloft.net, edumazet@google.com, robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org, netdev@vger.kernel.org, pabeni@redhat.com, mcoquelin.stm32@gmail.com, alexandre.torgue@foss.st.com, rmk+kernel@armlinux.org.uk, pjw@kernel.org, palmer@dabbelt.com, aou@eecs.berkeley.edu, alex@ghiti.fr, linux-riscv@lists.infradead.org, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, maxime.chevallier@bootlin.com, ningyu@eswincomputing.com, linmin@eswincomputing.com, pinkesh.vaghela@einfochips.com, pritesh.patel@einfochips.com, weishangjuan@eswincomputing.com, horms@kernel.org
> Subject: Re: [PATCH net-next v7 2/4] net: stmmac: eic7700: enable clocks before syscon access and correct RX sampling timing
>
> On Tue, 12 May 2026 13:39:12 +0800 (GMT+08:00) 李志 wrote:
> > For the eth1 enablement part, my current understanding is that it
> > should be treated as a new independent v1 series for net-next,
> > since the scope and target tree have changed after the split.
> >
> > Would you prefer this eth1 series to start as v1, or should it
> > continue as v8 for continuity with the original series?
>
> v8 is better, but is the fix in net-next already?
>
> If this is the posting you're referring to:
> https://lore.kernel.org/all/20260507083214.192-1-lizhi2@eswincomputing.com/
> it has been dropped based on feedback from Maxime and I don't see a v2.
Yes, the eth0 fixes have now been reposted as a dedicated series targeting net:
https://lore.kernel.org/lkml/20260507083037.152-1-lizhi2@eswincomputing.com/
The previous net-next eth1-related posting was dropped following your guidance
to first complete the eth0 fixes in net before continuing with eth1. I will
keep eth1 as v8 as previously suggested.
For the eth0 net series, I will split the remaining changes in v2 as suggested
by Maxime to improve reviewability and bisectability, and resend shortly.
Once that is completed, I will proceed with the eth1 enablement series (v8)
based on the updated context.
Thanks,
Zhi
^ permalink raw reply
* Re: [PATCH RFC 8/8] clk: sunxi-ng: a733: Add reset lines
From: Junhui Liu @ 2026-05-14 2:41 UTC (permalink / raw)
To: Andre Przywara, Junhui Liu
Cc: Michael Turquette, Stephen Boyd, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
Philipp Zabel, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Alexandre Ghiti, Richard Cochran, linux-clk, devicetree,
linux-arm-kernel, linux-sunxi, linux-kernel, linux-riscv, netdev
In-Reply-To: <20260514012226.691ae185@ryzen.lan>
Hi Andre,
Thanks for your review.
On Thu May 14, 2026 at 7:22 AM CST, Andre Przywara wrote:
> On Tue, 10 Mar 2026 16:34:01 +0800
> Junhui Liu <junhui.liu@pigmoral.tech> wrote:
>
> Hi,
>
> compare the list below against my version of the manual. You list more
> than shown there, can you say where those extra reset bits come from?
They are from the vendor BSP, I will add comments to explain them in the
next version.
>
>> Add the reset lines for the Allwinner A733 SoC. These reset control bits
>> are integrated into the Bus Gate Reset (BGR) registers, typically
>> sharing the same register address with their corresponding bus clock
>> gates. Integrate them into the main CCU driver using the existing
>> sunxi-ng ccu_reset framework, allowing the CCU to also function as a
>> reset controller for the SoC.
>>
>> Signed-off-by: Junhui Liu <junhui.liu@pigmoral.tech>
>> ---
>> drivers/clk/sunxi-ng/ccu-sun60i-a733.c | 128 +++++++++++++++++++++++++++++++++
>> 1 file changed, 128 insertions(+)
>>
>> diff --git a/drivers/clk/sunxi-ng/ccu-sun60i-a733.c b/drivers/clk/sunxi-ng/ccu-sun60i-a733.c
>> index c0b09f9197d1..7d1ee9235436 100644
>> --- a/drivers/clk/sunxi-ng/ccu-sun60i-a733.c
>> +++ b/drivers/clk/sunxi-ng/ccu-sun60i-a733.c
>> @@ -17,6 +17,7 @@
>> #include "../clk.h"
>>
>> #include "ccu_common.h"
>> +#include "ccu_reset.h"
>>
>> #include "ccu_div.h"
>> #include "ccu_gate.h"
>> @@ -2169,11 +2170,138 @@ static struct clk_hw_onecell_data sun60i_a733_hw_clks = {
>> .num = CLK_FANOUT3 + 1,
>> };
>>
>> +static struct ccu_reset_map sun60i_a733_ccu_resets[] = {
>> + [RST_BUS_ITS_PCIE] = { 0x574, BIT(16) },
>> + [RST_BUS_NSI] = { 0x580, BIT(30) },
>
> What is this NSI device? Some interconnect? Do we really want to handle
> this reset and clock? Is there some device referencing this? Otherwise
> the kernel will turn at least the clock off, which is probably fatal.
> Also the manual says this is a secure register, so I feel like the
> kernel must not mess with this.
Yes, NSI appears to be a high-speed interconnect. The vendor kernel
provides a sunxi-nsi driver for it accross sun8i, sun55i, and sun60i.
Since the mainline kernel doesn't manage this interconnect for the
already mainlined sun8i and sun55i platforms, I will drop the NSI clocks
and resets in the next version.
>
>> + [RST_BUS_NSI_CFG] = { 0x584, BIT(16) },
>
> Similar here, I guess, though this might not be secure only.
>
>> + [RST_BUS_IOMMU0_SYS] = { 0x58c, BIT(16) },
>> + [RST_BUS_MSI_LITE0_AHB] = { 0x594, BIT(16) },
>> + [RST_BUS_MSI_LITE0_MBUS] = { 0x594, BIT(17) },
>> + [RST_BUS_MSI_LITE1_AHB] = { 0x59c, BIT(16) },
>> + [RST_BUS_MSI_LITE1_MBUS] = { 0x59c, BIT(17) },
>> + [RST_BUS_MSI_LITE2_AHB] = { 0x5a4, BIT(16) },
>> + [RST_BUS_MSI_LITE2_MBUS] = { 0x5a4, BIT(17) },
>> + [RST_BUS_IOMMU1_SYS] = { 0x5b4, BIT(16) },
>> + [RST_BUS_DMA0] = { 0x704, BIT(16) },
>> + [RST_BUS_DMA1] = { 0x70c, BIT(16) },
>> + [RST_BUS_SPINLOCK] = { 0x724, BIT(16) },
>> + [RST_BUS_MSGBOX] = { 0x744, BIT(16) },
>> + [RST_BUS_PWM0] = { 0x784, BIT(16) },
>> + [RST_BUS_PWM1] = { 0x78c, BIT(16) },
>> + [RST_BUS_DBG] = { 0x7a4, BIT(16) },
>> + [RST_BUS_SYSDAP] = { 0x7ac, BIT(16) },
>> + [RST_BUS_TIMER0] = { 0x850, BIT(16) },
>> + [RST_BUS_DE] = { 0xa04, BIT(16) },
>> + [RST_BUS_DI] = { 0xa24, BIT(16) },
>> + [RST_BUS_G2D] = { 0xa44, BIT(16) },
>> + [RST_BUS_EINK] = { 0xa6c, BIT(16) },
>> + [RST_BUS_DE_SYS] = { 0xa74, BIT(16) },
>> + [RST_BUS_VE_ENC] = { 0xa8c, BIT(16) },
>
> The manual calls this ENC0, and since bit 17 is not documented, I
> wonder if there is an ENC1 used with some other packaging, maybe? So
> maybe calling it ENC0 would be safer here.
Okay, I will rename it to ENC0.
>
>> + [RST_BUS_VE_DEC] = { 0xa8c, BIT(18) },
>> + [RST_BUS_CE] = { 0xac4, BIT(16) },
>> + [RST_BUS_CE_SYS] = { 0xac4, BIT(17) },
>> + [RST_BUS_NPU_CORE] = { 0xb04, BIT(16) },
>> + [RST_BUS_NPU_AXI] = { 0xb04, BIT(17) },
>> + [RST_BUS_NPU_AHB] = { 0xb04, BIT(18) },
>> + [RST_BUS_NPU_SRAM] = { 0xb04, BIT(19) },
>> + [RST_BUS_GPU] = { 0xb24, BIT(16) },
>> + [RST_BUS_DRAM] = { 0xc0c, BIT(16) },
>> + [RST_BUS_NAND] = { 0xc8c, BIT(16) },
>> + [RST_BUS_MMC0] = { 0xd0c, BIT(16) },
>> + [RST_BUS_MMC1] = { 0xd1c, BIT(16) },
>> + [RST_BUS_MMC2] = { 0xd2c, BIT(16) },
>> + [RST_BUS_MMC3] = { 0xd3c, BIT(16) },
>> + [RST_BUS_UFS_AHB] = { 0xd8c, BIT(16) },
>> + [RST_BUS_UFS_AXI] = { 0xd8c, BIT(17) },
>> + [RST_BUS_UFS_PHY] = { 0xd8c, BIT(18) },
>> + [RST_BUS_UFS_CORE] = { 0xd8c, BIT(19) },
>> + [RST_BUS_UART0] = { 0xe00, BIT(16) },
>> + [RST_BUS_UART1] = { 0xe04, BIT(16) },
>> + [RST_BUS_UART2] = { 0xe08, BIT(16) },
>> + [RST_BUS_UART3] = { 0xe0c, BIT(16) },
>> + [RST_BUS_UART4] = { 0xe10, BIT(16) },
>> + [RST_BUS_UART5] = { 0xe14, BIT(16) },
>> + [RST_BUS_UART6] = { 0xe18, BIT(16) },
>> + [RST_BUS_I2C0] = { 0xe80, BIT(16) },
>> + [RST_BUS_I2C1] = { 0xe84, BIT(16) },
>> + [RST_BUS_I2C2] = { 0xe88, BIT(16) },
>> + [RST_BUS_I2C3] = { 0xe8c, BIT(16) },
>> + [RST_BUS_I2C4] = { 0xe90, BIT(16) },
>> + [RST_BUS_I2C5] = { 0xe94, BIT(16) },
>> + [RST_BUS_I2C6] = { 0xe98, BIT(16) },
>> + [RST_BUS_I2C7] = { 0xe9c, BIT(16) },
>> + [RST_BUS_I2C8] = { 0xea0, BIT(16) },
>> + [RST_BUS_I2C9] = { 0xea4, BIT(16) },
>> + [RST_BUS_I2C10] = { 0xea8, BIT(16) },
>> + [RST_BUS_I2C11] = { 0xeac, BIT(16) },
>> + [RST_BUS_I2C12] = { 0xeb0, BIT(16) },
>> + [RST_BUS_SPI0] = { 0xf04, BIT(16) },
>> + [RST_BUS_SPI1] = { 0xf0c, BIT(16) },
>> + [RST_BUS_SPI2] = { 0xf14, BIT(16) },
>> + [RST_BUS_SPIF] = { 0xf1c, BIT(16) },
>> + [RST_BUS_SPI3] = { 0xf24, BIT(16) },
>> + [RST_BUS_SPI4] = { 0xf2c, BIT(16) },
>
> SPI4 isn't mentioned in my version of the manual.
>
>> + [RST_BUS_GPADC] = { 0xfc4, BIT(16) },
>> + [RST_BUS_THS] = { 0xfe4, BIT(16) },
>> + [RST_BUS_IRRX] = { 0x1004, BIT(16) },
>> + [RST_BUS_IRTX] = { 0x100c, BIT(16) },
>> + [RST_BUS_LRADC] = { 0x1024, BIT(16) },
>> + [RST_BUS_SGPIO] = { 0x1064, BIT(16) },
>> + [RST_BUS_LPC] = { 0x1084, BIT(16) },
>
> Where do those two come from? There are not in my version of the manual.
>
>> + [RST_BUS_I2SPCM0] = { 0x120c, BIT(16) },
>> + [RST_BUS_I2SPCM1] = { 0x121c, BIT(16) },
>> + [RST_BUS_I2SPCM2] = { 0x122c, BIT(16) },
>> + [RST_BUS_I2SPCM3] = { 0x123c, BIT(16) },
>> + [RST_BUS_I2SPCM4] = { 0x124c, BIT(16) },
>> + [RST_BUS_OWA] = { 0x128c, BIT(16) },
>> + [RST_BUS_DMIC] = { 0x12cc, BIT(16) },
>> + [RST_USB_PHY0] = { 0x1300, BIT(30) },
>> + [RST_BUS_OHCI0] = { 0x1304, BIT(16) },
>> + [RST_BUS_EHCI0] = { 0x1304, BIT(20) },
>> + [RST_BUS_OTG] = { 0x1304, BIT(24) },
>> + [RST_USB_PHY1] = { 0x1308, BIT(30) },
>> + [RST_BUS_OHCI1] = { 0x130c, BIT(16) },
>> + [RST_BUS_EHCI1] = { 0x130c, BIT(20) },
>> + [RST_BUS_USB2] = { 0x135c, BIT(16) },
>> + [RST_BUS_PCIE] = { 0x138c, BIT(17) },
>> + [RST_BUS_PCIE_PWRUP] = { 0x138c, BIT(16) },
>
> Just a nit, but those two are ordered wrongly.
Will do.
>
>> + [RST_BUS_SERDES] = { 0x13c4, BIT(16) },
>> + [RST_BUS_GMAC0] = { 0x141c, BIT(16) },
>> + [RST_BUS_GMAC0_AXI] = { 0x141c, BIT(17) },
>> + [RST_BUS_GMAC1] = { 0x142c, BIT(16) },
>> + [RST_BUS_GMAC1_AXI] = { 0x142c, BIT(17) },
>
> GMAC1 is not listed in my manual, where does this come from?
>
>> + [RST_BUS_TCON_LCD0] = { 0x1504, BIT(16) },
>> + [RST_BUS_TCON_LCD1] = { 0x150c, BIT(16) },
>> + [RST_BUS_TCON_LCD2] = { 0x1514, BIT(16) },
>
> No LCD2 in my manual.
>
> The rest looks alright.
>
> Cheers,
> Andre
>
>> + [RST_BUS_LVDS0] = { 0x1544, BIT(16) },
>> + [RST_BUS_LVDS1] = { 0x154c, BIT(16) },
>> + [RST_BUS_DSI0] = { 0x1584, BIT(16) },
>> + [RST_BUS_DSI1] = { 0x158c, BIT(16) },
>> + [RST_BUS_TCON_TV0] = { 0x1604, BIT(16) },
>> + [RST_BUS_TCON_TV1] = { 0x160c, BIT(16) },
>> + [RST_BUS_EDP] = { 0x164c, BIT(16) },
>> + [RST_BUS_HDMI_MAIN] = { 0x168c, BIT(16) },
>> + [RST_BUS_HDMI_SUB] = { 0x168c, BIT(17) },
>> + [RST_BUS_HDMI_HDCP] = { 0x168c, BIT(18) },
>> + [RST_BUS_DPSS_TOP0] = { 0x16c4, BIT(16) },
>> + [RST_BUS_DPSS_TOP1] = { 0x16cc, BIT(16) },
>> + [RST_BUS_VIDEO_OUT0] = { 0x16e4, BIT(16) },
>> + [RST_BUS_VIDEO_OUT1] = { 0x16ec, BIT(16) },
>> + [RST_BUS_LEDC] = { 0x1704, BIT(16) },
>> + [RST_BUS_DSC] = { 0x1744, BIT(16) },
>> + [RST_BUS_CSI] = { 0x1844, BIT(16) },
>> + [RST_BUS_VIDEO_IN] = { 0x1884, BIT(16) },
>> + [RST_BUS_APB2JTAG] = { 0x1c04, BIT(16) },
>> +};
>> +
>> static const struct sunxi_ccu_desc sun60i_a733_ccu_desc = {
>> .ccu_clks = sun60i_a733_ccu_clks,
>> .num_ccu_clks = ARRAY_SIZE(sun60i_a733_ccu_clks),
>>
>> .hw_clks = &sun60i_a733_hw_clks,
>> +
>> + .resets = sun60i_a733_ccu_resets,
>> + .num_resets = ARRAY_SIZE(sun60i_a733_ccu_resets),
>> };
>>
>> static const u32 pll_regs[] = {
>>
--
Best regards,
Junhui Liu
^ permalink raw reply
* [PATCH v2 0/2] ARM: dts: aspeed: Add ASRock Rack B650D4U support
From: Prasanth Kumar Padarthi @ 2026-05-14 3:16 UTC (permalink / raw)
To: joel, andrew
Cc: robh, krzk+dt, conor+dt, devicetree, linux-aspeed,
linux-arm-kernel, Prasanth Kumar Padarthi
This series adds initial device tree support for the ASRock Rack
B650D4U BMC, which is based on the ASPEED AST2600 SoC.
The B650D4U is a server motherboard for AMD Ryzen processors. The initial
DTS describes the BMC console, FMC flash, dedicated management Ethernet,
FRU EEPROM, and watchdog.
The dedicated management port is described as MAC0 connected to an external
Clause 22 PHY on MDIO0 at address 0. The MAC uses RGMII with RX internal
delay, matching the vendor DTB description.
Tested:
- Built aspeed-bmc-asrock-b650d4u.dtb
- Ran dtbs_check for the board/aspeed binding
- QEMU boot smoke-tested with ast2600-evb
Changes in v2:
- Added MDIO0 PHY node for the dedicated management port.
- Set mac0 phy-mode to rgmii-rxid and added phy-handle.
- Removed unused GPIO include from the DTS.
Prasanth Kumar Padarthi (2):
dt-bindings: arm: aspeed: Add ASRock Rack B650D4U
ARM: dts: aspeed: Add ASRock Rack B650D4U BMC
.../bindings/arm/aspeed/aspeed.yaml | 1 +
arch/arm/boot/dts/aspeed/Makefile | 1 +
.../dts/aspeed/aspeed-bmc-asrock-b650d4u.dts | 71 +++++++++++++++++++
3 files changed, 73 insertions(+)
create mode 100644 arch/arm/boot/dts/aspeed/aspeed-bmc-asrock-b650d4u.dts
--
2.47.3
^ permalink raw reply
* [PATCH v2 1/2] dt-bindings: arm: aspeed: Add ASRock Rack B650D4U
From: Prasanth Kumar Padarthi @ 2026-05-14 3:16 UTC (permalink / raw)
To: joel, andrew
Cc: robh, krzk+dt, conor+dt, devicetree, linux-aspeed,
linux-arm-kernel, Prasanth Kumar Padarthi
In-Reply-To: <20260514031622.1416922-1-prasanth.padarthi10@gmail.com>
Add the compatible string for the ASRock Rack B650D4U BMC,
which is an AST2600-based server motherboard.
Signed-off-by: Prasanth Kumar Padarthi <prasanth.padarthi10@gmail.com>
---
Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml b/Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml
index 2f92b8ab0..656397850 100644
--- a/Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml
+++ b/Documentation/devicetree/bindings/arm/aspeed/aspeed.yaml
@@ -77,6 +77,7 @@ properties:
- ampere,mtmitchell-bmc
- aspeed,ast2600-evb
- aspeed,ast2600-evb-a1
+ - asrock,b650d4u-bmc
- asus,x4tf-bmc
- facebook,bletchley-bmc
- facebook,catalina-bmc
--
2.47.3
^ permalink raw reply related
* [PATCH v2 2/2] ARM: dts: aspeed: Add ASRock Rack B650D4U BMC
From: Prasanth Kumar Padarthi @ 2026-05-14 3:16 UTC (permalink / raw)
To: joel, andrew
Cc: robh, krzk+dt, conor+dt, devicetree, linux-aspeed,
linux-arm-kernel, Prasanth Kumar Padarthi
In-Reply-To: <20260514031622.1416922-1-prasanth.padarthi10@gmail.com>
Add initial device tree support for the ASRock Rack B650D4U BMC.
The B650D4U is a server motherboard utilizing the ASPEED AST2600
SoC for management.
Signed-off-by: Prasanth Kumar Padarthi <prasanth.padarthi10@gmail.com>
---
arch/arm/boot/dts/aspeed/Makefile | 1 +
.../dts/aspeed/aspeed-bmc-asrock-b650d4u.dts | 71 +++++++++++++++++++
2 files changed, 72 insertions(+)
create mode 100644 arch/arm/boot/dts/aspeed/aspeed-bmc-asrock-b650d4u.dts
diff --git a/arch/arm/boot/dts/aspeed/Makefile b/arch/arm/boot/dts/aspeed/Makefile
index c4f064e4b..124d4f8f8 100644
--- a/arch/arm/boot/dts/aspeed/Makefile
+++ b/arch/arm/boot/dts/aspeed/Makefile
@@ -13,6 +13,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
aspeed-bmc-asrock-romed8hm3.dtb \
aspeed-bmc-asrock-spc621d8hm3.dtb \
aspeed-bmc-asrock-x570d4u.dtb \
+ aspeed-bmc-asrock-b650d4u.dtb \
aspeed-bmc-asus-x4tf.dtb \
aspeed-bmc-bytedance-g220a.dtb \
aspeed-bmc-delta-ahe50dc.dtb \
diff --git a/arch/arm/boot/dts/aspeed/aspeed-bmc-asrock-b650d4u.dts b/arch/arm/boot/dts/aspeed/aspeed-bmc-asrock-b650d4u.dts
new file mode 100644
index 000000000..130b7f3e0
--- /dev/null
+++ b/arch/arm/boot/dts/aspeed/aspeed-bmc-asrock-b650d4u.dts
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0
+/dts-v1/;
+
+#include "aspeed-g6.dtsi"
+
+/ {
+ model = "ASRock Rack B650D4U BMC";
+ compatible = "asrock,b650d4u-bmc", "aspeed,ast2600";
+
+ aliases {
+ serial4 = &uart5;
+ };
+
+ chosen {
+ stdout-path = "serial4:115200n8";
+ };
+
+ memory@80000000 {
+ device_type = "memory";
+ reg = <0x80000000 0x40000000>;
+ };
+};
+
+/* BMC Console UART */
+&uart5 {
+ status = "okay";
+};
+
+/* SPI Flash Management */
+&fmc {
+ status = "okay";
+ flash@0 {
+ status = "okay";
+ m25p,fast-read;
+ label = "bmc";
+ };
+};
+
+/* Dedicated Management LAN */
+&mdio0 {
+ status = "okay";
+
+ ethphy0: ethernet-phy@0 {
+ compatible = "ethernet-phy-ieee802.3-c22";
+ reg = <0>;
+ };
+};
+
+&mac0 {
+ status = "okay";
+ phy-mode = "rgmii-rxid";
+ phy-handle = <ðphy0>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_rgmii1_default &pinctrl_mdio1_default>;
+};
+
+/* I2C Bus for FRU/EEPROM Storage */
+&i2c7 {
+ status = "okay";
+ eeprom@57 {
+ compatible = "atmel,24c02";
+ reg = <0x57>;
+ pagesize = <16>;
+ };
+};
+
+/* System Watchdog */
+&wdt1 {
+ status = "okay";
+ aspeed,reset-type = "soc";
+};
--
2.47.3
^ permalink raw reply related
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