From: Will Deacon <will.deacon@arm.com>
To: Jeremy Linton <jeremy.linton@arm.com>
Cc: linux-arm-kernel@lists.infradead.org, linux-acpi@vger.kernel.org,
linux-kernel@vger.kernel.org, catalin.marinas@arm.com,
rjw@rjwysocki.net, lenb@kernel.org, mark.rutland@arm.com,
sudeep.holla@arm.com, james.morse@arm.com,
lorenzo.pieralisi@arm.com, linuxarm@huawei.com
Subject: Re: [PATCH 4/4] perf: arm_spe: Enable ACPI/Platform automatic module loading
Date: Fri, 26 Apr 2019 09:04:45 +0100 [thread overview]
Message-ID: <20190426080445.GA28144@fuggles.cambridge.arm.com> (raw)
In-Reply-To: <1345818a-3a4a-e919-2168-e2950cee07b6@arm.com>
On Thu, Apr 25, 2019 at 07:58:28PM -0500, Jeremy Linton wrote:
> On 4/16/19 8:50 AM, Will Deacon wrote:
> > On Thu, Apr 04, 2019 at 12:24:38PM -0500, Jeremy Linton wrote:
> > > On 4/4/19 12:04 PM, Will Deacon wrote:
> > > > On Tue, Mar 26, 2019 at 05:39:38PM -0500, Jeremy Linton wrote:
> > > > > Lets add the MODULE_TABLE and platform id_table entries so that
> > > > > the SPE driver can attach to the ACPI platform device created by
> > > > > the core pmu code.
> > > > >
> > > > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> > > > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> > > > > ---
> > > > > drivers/perf/arm_spe_pmu.c | 11 +++++++++--
> > > > > 1 file changed, 9 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> > > > > index 7cb766dafe85..ffa2c76c08bb 100644
> > > > > --- a/drivers/perf/arm_spe_pmu.c
> > > > > +++ b/drivers/perf/arm_spe_pmu.c
> > > > > @@ -1176,7 +1176,13 @@ static const struct of_device_id arm_spe_pmu_of_match[] = {
> > > > > };
> > > > > MODULE_DEVICE_TABLE(of, arm_spe_pmu_of_match);
> > > > > -static int arm_spe_pmu_device_dt_probe(struct platform_device *pdev)
> > > > > +static const struct platform_device_id arm_spe_match[] = {
> > > > > + { "arm,spe-v1", 0},
> > > >
> > > > It would be nice if we could avoid duplicating this string from the ACPI
> > > > parsing code.
> > >
> > > Ok sure, I just need to find a good common place for it.
>
> There doesn't appear to be a good common place for this, so maybe arm_pmu.h,
> which can then be included in the spe driver is the right thing.
I'm fine with that.
> > > > > + { }
> > > > > +};
> > > > > +MODULE_DEVICE_TABLE(platform, arm_spe_match);
> > > > > +
> > > > > +static int arm_spe_pmu_device_probe(struct platform_device *pdev)
> > > > > {
> > > > > int ret;
> > > > > struct arm_spe_pmu *spe_pmu;
> > > > > @@ -1236,11 +1242,12 @@ static int arm_spe_pmu_device_remove(struct platform_device *pdev)
> > > > > }
> > > > > static struct platform_driver arm_spe_pmu_driver = {
> > > > > + .id_table = arm_spe_match,
> > > > > .driver = {
> > > > > .name = DRVNAME,
> > > > > .of_match_table = of_match_ptr(arm_spe_pmu_of_match),
> > > >
> > > > Hmm, so some other drivers don't hook .id_table like you do, but instead
> > > > hook .acpi_match_table in the driver structure. Is that not better?
> > >
> > > This isn't actually an ACPI device, (aka not defined in the namespace), so
> > > its missing much of the ACPI functionality. I think that also means its
> > > needs to be declared this way.
> >
> > Looking at platform_match(), I'd really like to avoid having both an
> > .id_table and an .of_match_table field.
>
>
> >
> > acpi_of_match_device() will actually use the .of_match_table, but it relies
> > on ACPI_COMPANION returning a valid acpi_device. If we don't have one of
>
> Right, via the fwnode it can cause an acpi DSDT defined device with a _DSD
> "compatible" property to match an entry in the of_match_table compatible
> string. I don't think this is us...
>
> > those, perhaps we can use the .id_table exclusively and drop the
> > .of_match_table instead?
>
> This definitely made me do my homework, the following is AFAIK:
FWIW: I'm also feeling my way here!
> Its possible to match on just a .id_table, but this requires matching the OF
> device name against the id_table name rather than against the OF compatible
> string (*). This doesn't seem like a good idea, despite platform_device_id
> entries being significantly smaller than the of_device_id ones. Plus, I
> think we end up with two duplicate tables because we still need the
> MODULE_TABLE(of,xxx) to assure that userspace can associate the modalias
> with the module.
Well spotted, I didn't notice that the compatible string isn't used for
matching in that case.
> OTOH, it seems possible to match on module name directly ('arm_spe_pmu'),
> but this limits us to only a single device type for all ACPI device
> variations unless we put platform checks in the module itself (ick!). I
> suspect in the future if a spe.v2 were to come out this would be a problem
> unless a separate module were created. Then there is the fact this still
> needs a platform_device_id table, as the modalias will read
> "platform:arm_spe_pmu". Which will cause people to question why its not just
> assigned and matched against the .id_table.
Ok, fair enough and sorry for the wild goose chase. Looks like we'll stick
with what you had, as the alternatives all seen considerably worse.
Will
next prev parent reply other threads:[~2019-04-26 8:04 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-26 22:39 [PATCH 0/4] arm64: SPE ACPI enablement Jeremy Linton
2019-03-26 22:39 ` [PATCH 1/4] ACPI/PPTT: Add function to return ACPI 6.3 Identical tokens Jeremy Linton
2019-03-28 10:04 ` Rafael J. Wysocki
2019-03-28 15:20 ` Jeremy Linton
2019-03-26 22:39 ` [PATCH 2/4] ACPI/PPTT: Modify node flag detection to find last IDENTICAL Jeremy Linton
2019-03-26 22:39 ` [PATCH 3/4] arm_pmu: acpi: spe: Add initial MADT/SPE probing Jeremy Linton
2019-03-28 12:40 ` John Garry
2019-04-02 19:14 ` Jeremy Linton
2019-04-05 9:23 ` John Garry
2019-04-05 9:23 ` John Garry
2019-03-26 22:39 ` [PATCH 4/4] perf: arm_spe: Enable ACPI/Platform automatic module loading Jeremy Linton
2019-04-04 17:04 ` Will Deacon
2019-04-04 17:24 ` Jeremy Linton
2019-04-16 13:50 ` Will Deacon
2019-04-16 13:50 ` Will Deacon
2019-04-26 0:58 ` Jeremy Linton
2019-04-26 0:58 ` Jeremy Linton
2019-04-26 8:04 ` Will Deacon [this message]
2019-04-26 8:04 ` Will Deacon
2019-06-15 1:09 [PATCH v4 0/4] arm64: SPE ACPI enablement Jeremy Linton
2019-06-15 1:09 ` [PATCH 4/4] perf: arm_spe: Enable ACPI/Platform automatic module loading Jeremy Linton
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190426080445.GA28144@fuggles.cambridge.arm.com \
--to=will.deacon@arm.com \
--cc=catalin.marinas@arm.com \
--cc=james.morse@arm.com \
--cc=jeremy.linton@arm.com \
--cc=lenb@kernel.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxarm@huawei.com \
--cc=lorenzo.pieralisi@arm.com \
--cc=mark.rutland@arm.com \
--cc=rjw@rjwysocki.net \
--cc=sudeep.holla@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).