* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Mark Brown @ 2013-07-23 17:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130723173710.GB28284@kroah.com>
[-- Attachment #1: Type: text/plain, Size: 757 bytes --]
On Tue, Jul 23, 2013 at 10:37:11AM -0700, Greg KH wrote:
> On Tue, Jul 23, 2013 at 06:50:29PM +0200, Tomasz Figa wrote:
> > I fully agree that a simple, single string will not scale even in some, not
> > so uncommon cases, but there is already a lot of existing lookup solutions
> > over the kernel and so there is no point in introducing another one.
> I'm trying to get _rid_ of lookup "solutions" and just use a real
> pointer, as you should. I'll go tackle those other ones after this one
> is taken care of, to show how the others should be handled as well.
What are the problems you are seeing with doing things with lookups?
Having to write platform data for everything gets old fast and the code
duplication is pretty tedious...
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Tomasz Figa @ 2013-07-23 17:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130723173710.GB28284@kroah.com>
On Tuesday 23 of July 2013 10:37:11 Greg KH wrote:
> On Tue, Jul 23, 2013 at 06:50:29PM +0200, Tomasz Figa wrote:
> > > Ick, no. Why can't you just pass the pointer to the phy itself? If
> > > you
> > > had a "priv" pointer to search from, then you could have just passed
> > > the
> > > original phy pointer in the first place, right?
> >
> > IMHO it would be better if you provided some code example, but let's
> > try to check if I understood you correctly.
>
> It's not my code that I want to have added, so I don't have to write
> examples, I just get to complain about the existing stuff :)
Still, I think that some small code snippets illustrating the idea are
really helpful.
> > 8><--------------------------------------------------------------------
> > ----
> >
> > [Board file]
> >
> > static struct phy my_phy;
> >
> > static struct platform_device phy_pdev = {
> >
> > /* ... */
> > .platform_data = &my_phy;
> > /* ... */
> >
> > };
> >
> > static struct platform_device phy_pdev = {
> >
> > /* ... */
> > .platform_data = &my_phy;
> > /* ... */
> >
> > };
> >
> > [Provider driver]
> >
> > struct phy *phy = pdev->dev.platform_data;
> >
> > ret = phy_create(phy);
> >
> > [Consumer driver]
> >
> > struct phy *phy = pdev->dev.platform_data;
> >
> > ret = phy_get(&pdev->dev, phy);
> >
> > -----------------------------------------------------------------------
> > -><8
> >
> > Is this what you mean?
>
> No. Well, kind of. What's wrong with using the platform data structure
> unique to the board to have the pointer?
>
> For example (just randomly picking one), the ata-pxa driver would change
> include/linux/platform_data/ata-pxa.h to have a phy pointer in it:
>
> struct phy;
>
> struct pata_pxa_pdata {
> /* PXA DMA DREQ<0:2> pin */
> uint32_t dma_dreq;
> /* Register shift */
> uint32_t reg_shift;
> /* IRQ flags */
> uint32_t irq_flags;
> /* PHY */
> struct phy *phy;
> };
>
> Then, when you create the platform, set the phy* pointer with a call to
> phy_create(). Then you can use that pointer wherever that plaform data
> is available (i.e. whereever platform_data is at).
Hmm? So, do you suggest to call phy_create() from board file? What phy_ops
struct and other hardware parameters would it take?
> > > The issue is that a string "name" is not going to scale at all, as it
> > > requires hard-coded information that will change over time (as the
> > > existing clock interface is already showing.)
> >
> > I fully agree that a simple, single string will not scale even in some,
> > not so uncommon cases, but there is already a lot of existing lookup
> > solutions over the kernel and so there is no point in introducing
> > another one.
> I'm trying to get _rid_ of lookup "solutions" and just use a real
> pointer, as you should. I'll go tackle those other ones after this one
> is taken care of, to show how the others should be handled as well.
There was a reason for introducing lookup solutions. The reason was that in
board file there is no way to get a pointer to something that is going to be
created much later in time. We don't do time travel ;-).
> > > Please just pass the real "phy" pointer around, that's what it is
> > > there
> > > for. Your "board binding" logic/code should be able to handle this,
> > > as
> > > it somehow was going to do the same thing with a "name".
> >
> > It's technically correct, but quality of this solution isn't really
> > nice, because it's a layering violation (at least if I understood what
> > you mean). This is because you need to have full definition of struct
> > phy in board file and a structure that is used as private data in PHY
> > core comes from platform code.
>
> No, just a pointer, you don't need the "full" structure until you get to
> some .c code that actually manipulates the phy itself, for all other
> places, you are just dealing with a pointer and a structure you never
> reference.
>
> Does that make more sense?
Well, to the point that I think I now understood your suggestion.
Unfortunately the suggestion alone isn't really something that can be done,
considering how driver core and generic frameworks work.
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Greg KH @ 2013-07-23 18:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130723174456.GM9858@sirena.org.uk>
On Tue, Jul 23, 2013 at 06:44:56PM +0100, Mark Brown wrote:
> On Tue, Jul 23, 2013 at 10:37:11AM -0700, Greg KH wrote:
> > On Tue, Jul 23, 2013 at 06:50:29PM +0200, Tomasz Figa wrote:
>
> > > I fully agree that a simple, single string will not scale even in some, not
> > > so uncommon cases, but there is already a lot of existing lookup solutions
> > > over the kernel and so there is no point in introducing another one.
>
> > I'm trying to get _rid_ of lookup "solutions" and just use a real
> > pointer, as you should. I'll go tackle those other ones after this one
> > is taken care of, to show how the others should be handled as well.
>
> What are the problems you are seeing with doing things with lookups?
You don't "know" the id of the device you are looking up, due to
multiple devices being in the system (dynamic ids, look back earlier in
this thread for details about that.)
> Having to write platform data for everything gets old fast and the code
> duplication is pretty tedious...
Adding a single pointer is "tedious"? Where is the "name" that you are
going to lookup going to come from? That code doesn't write itself...
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Greg KH @ 2013-07-23 18:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <19656720.FrnhefyPXl@amdc1227>
On Tue, Jul 23, 2013 at 07:48:11PM +0200, Tomasz Figa wrote:
> On Tuesday 23 of July 2013 10:37:11 Greg KH wrote:
> > On Tue, Jul 23, 2013 at 06:50:29PM +0200, Tomasz Figa wrote:
> > > > Ick, no. Why can't you just pass the pointer to the phy itself? If
> > > > you
> > > > had a "priv" pointer to search from, then you could have just passed
> > > > the
> > > > original phy pointer in the first place, right?
> > >
> > > IMHO it would be better if you provided some code example, but let's
> > > try to check if I understood you correctly.
> >
> > It's not my code that I want to have added, so I don't have to write
> > examples, I just get to complain about the existing stuff :)
>
> Still, I think that some small code snippets illustrating the idea are
> really helpful.
>
> > > 8><--------------------------------------------------------------------
> > > ----
> > >
> > > [Board file]
> > >
> > > static struct phy my_phy;
> > >
> > > static struct platform_device phy_pdev = {
> > >
> > > /* ... */
> > > .platform_data = &my_phy;
> > > /* ... */
> > >
> > > };
> > >
> > > static struct platform_device phy_pdev = {
> > >
> > > /* ... */
> > > .platform_data = &my_phy;
> > > /* ... */
> > >
> > > };
> > >
> > > [Provider driver]
> > >
> > > struct phy *phy = pdev->dev.platform_data;
> > >
> > > ret = phy_create(phy);
> > >
> > > [Consumer driver]
> > >
> > > struct phy *phy = pdev->dev.platform_data;
> > >
> > > ret = phy_get(&pdev->dev, phy);
> > >
> > > -----------------------------------------------------------------------
> > > -><8
> > >
> > > Is this what you mean?
> >
> > No. Well, kind of. What's wrong with using the platform data structure
> > unique to the board to have the pointer?
> >
> > For example (just randomly picking one), the ata-pxa driver would change
> > include/linux/platform_data/ata-pxa.h to have a phy pointer in it:
> >
> > struct phy;
> >
> > struct pata_pxa_pdata {
> > /* PXA DMA DREQ<0:2> pin */
> > uint32_t dma_dreq;
> > /* Register shift */
> > uint32_t reg_shift;
> > /* IRQ flags */
> > uint32_t irq_flags;
> > /* PHY */
> > struct phy *phy;
> > };
> >
> > Then, when you create the platform, set the phy* pointer with a call to
> > phy_create(). Then you can use that pointer wherever that plaform data
> > is available (i.e. whereever platform_data is at).
>
> Hmm? So, do you suggest to call phy_create() from board file? What phy_ops
> struct and other hardware parameters would it take?
>
> > > > The issue is that a string "name" is not going to scale at all, as it
> > > > requires hard-coded information that will change over time (as the
> > > > existing clock interface is already showing.)
> > >
> > > I fully agree that a simple, single string will not scale even in some,
> > > not so uncommon cases, but there is already a lot of existing lookup
> > > solutions over the kernel and so there is no point in introducing
> > > another one.
> > I'm trying to get _rid_ of lookup "solutions" and just use a real
> > pointer, as you should. I'll go tackle those other ones after this one
> > is taken care of, to show how the others should be handled as well.
>
> There was a reason for introducing lookup solutions. The reason was that in
> board file there is no way to get a pointer to something that is going to be
> created much later in time. We don't do time travel ;-).
>
> > > > Please just pass the real "phy" pointer around, that's what it is
> > > > there
> > > > for. Your "board binding" logic/code should be able to handle this,
> > > > as
> > > > it somehow was going to do the same thing with a "name".
> > >
> > > It's technically correct, but quality of this solution isn't really
> > > nice, because it's a layering violation (at least if I understood what
> > > you mean). This is because you need to have full definition of struct
> > > phy in board file and a structure that is used as private data in PHY
> > > core comes from platform code.
> >
> > No, just a pointer, you don't need the "full" structure until you get to
> > some .c code that actually manipulates the phy itself, for all other
> > places, you are just dealing with a pointer and a structure you never
> > reference.
> >
> > Does that make more sense?
>
> Well, to the point that I think I now understood your suggestion.
> Unfortunately the suggestion alone isn't really something that can be done,
> considering how driver core and generic frameworks work.
Ok, given that I seem to be totally confused as to exactly how the
board-specific frameworks work, I'll take your word for it.
But again, I will not accept "lookup by name" type solutions, when the
"name" is dynamic and will change. Because you are using a "name", you
can deal with a pointer, putting it _somewhere_ in your board-specific
data structures, as you are going to need to store it anyway (hint, you
had to get that "name" from somewhere, right?)
And maybe the way that these "generic frameworks" are created is wrong,
given that you don't feel that a generic pointer can be passed to the
needed devices. That seems like a huge problem, one that has already
been pointed out is causing issues with other subsystems.
So maybe they need to be fixed?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Mark Brown @ 2013-07-23 19:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130723180110.GA8688@kroah.com>
[-- Attachment #1: Type: text/plain, Size: 1634 bytes --]
On Tue, Jul 23, 2013 at 11:01:10AM -0700, Greg KH wrote:
> On Tue, Jul 23, 2013 at 06:44:56PM +0100, Mark Brown wrote:
> > What are the problems you are seeing with doing things with lookups?
> You don't "know" the id of the device you are looking up, due to
> multiple devices being in the system (dynamic ids, look back earlier in
> this thread for details about that.)
I got copied in very late so don't have most of the thread I'm afraid,
I did try looking at web archives but didn't see a clear problem
statement. In any case this is why the APIs doing lookups do the
lookups in the context of the requesting device - devices ask for
whatever name they use locally.
> > Having to write platform data for everything gets old fast and the code
> > duplication is pretty tedious...
> Adding a single pointer is "tedious"? Where is the "name" that you are
> going to lookup going to come from? That code doesn't write itself...
It's adding platform data in the first place that gets tedious - and of
course there's also DT and ACPI to worry about, it's not just a case of
platform data and then you're done. Pushing the lookup into library
code means that drivers don't have to worry about any of this stuff.
For most of the APIs doing this there is a clear and unambiguous name in
the hardware that can be used (and for hardware process reasons is
unlikely to get changed). The major exception to this is the clock API
since it is relatively rare to have clear, segregated IP level
information for IPs baked into larger chips. The other APIs tend to be
establishing chip to chip links.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Alan Stern @ 2013-07-23 19:36 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1374129984-765-2-git-send-email-kishon@ti.com>
On Tue, 23 Jul 2013, Tomasz Figa wrote:
> IMHO it would be better if you provided some code example, but let's try to
> check if I understood you correctly.
>
> 8><------------------------------------------------------------------------
>
> [Board file]
>
> static struct phy my_phy;
>
> static struct platform_device phy_pdev = {
> /* ... */
> .platform_data = &my_phy;
> /* ... */
> };
>
> static struct platform_device phy_pdev = {
This should be controller_pdev, not phy_pdev, yes?
> /* ... */
> .platform_data = &my_phy;
> /* ... */
> };
>
> [Provider driver]
>
> struct phy *phy = pdev->dev.platform_data;
>
> ret = phy_create(phy);
>
> [Consumer driver]
>
> struct phy *phy = pdev->dev.platform_data;
>
> ret = phy_get(&pdev->dev, phy);
Or even just phy_get(&pdev->dev), because phy_get() could be smart
enough to to set phy = dev->platform_data.
> ------------------------------------------------------------------------><8
>
> Is this what you mean?
That's what I was going to suggest too. The struct phy is defined in
the board file, which already knows about all the PHYs that exist in
the system. (Or perhaps it is allocated dynamically, so that when many
board files are present in the same kernel, only the entries listed in
the board file for the current system get created.) Then the
structure's address is stored in the platform data and made available
to both the provider and the consumer.
Even though the struct phy is defined (or allocated) in the board file,
its contents don't get filled in until the PHY driver provides the
details.
> It's technically correct, but quality of this solution isn't really nice,
> because it's a layering violation (at least if I understood what you mean).
> This is because you need to have full definition of struct phy in board file
> and a structure that is used as private data in PHY core comes from
> platform code.
You don't have to have a full definition in the board file. Just a
partial definition -- most of the contents can be filled in later, when
the PHY driver is ready to store the private data.
It's not a layering violation for one region of the kernel to store
private data in a structure defined by another part of the kernel.
This happens all the time (e.g., dev_set_drvdata).
Alan Stern
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Greg KH @ 2013-07-23 19:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130723193105.GP9858@sirena.org.uk>
On Tue, Jul 23, 2013 at 08:31:05PM +0100, Mark Brown wrote:
> > You don't "know" the id of the device you are looking up, due to
> > multiple devices being in the system (dynamic ids, look back earlier in
> > this thread for details about that.)
>
> I got copied in very late so don't have most of the thread I'm afraid,
> I did try looking at web archives but didn't see a clear problem
> statement. In any case this is why the APIs doing lookups do the
> lookups in the context of the requesting device - devices ask for
> whatever name they use locally.
What do you mean by "locally"?
The problem with the api was that the phy core wanted a id and a name to
create a phy, and then later other code was doing a "lookup" based on
the name and id (mushed together), because it "knew" that this device
was the one it wanted.
Just like the clock api, which, for multiple devices, has proven to
cause problems. I don't want to see us accept an api that we know has
issues in it now, I'd rather us fix it up properly.
Subsystems should be able to create ids how ever they want to, and not
rely on the code calling them to specify the names of the devices that
way, otherwise the api is just too fragile.
I think, that if you create a device, then just carry around the pointer
to that device (in this case a phy) and pass it to whatever other code
needs it. No need to do lookups on "known names" or anything else, just
normal pointers, with no problems for multiple devices, busses, or
naming issues.
> > > Having to write platform data for everything gets old fast and the code
> > > duplication is pretty tedious...
>
> > Adding a single pointer is "tedious"? Where is the "name" that you are
> > going to lookup going to come from? That code doesn't write itself...
>
> It's adding platform data in the first place that gets tedious - and of
> course there's also DT and ACPI to worry about, it's not just a case of
> platform data and then you're done. Pushing the lookup into library
> code means that drivers don't have to worry about any of this stuff.
I agree, so just pass around the pointer to the phy and all is good. No
need to worry about DT or ACPI or anything else.
> For most of the APIs doing this there is a clear and unambiguous name in
> the hardware that can be used (and for hardware process reasons is
> unlikely to get changed). The major exception to this is the clock API
> since it is relatively rare to have clear, segregated IP level
> information for IPs baked into larger chips. The other APIs tend to be
> establishing chip to chip links.
The clock api is having problems with multiple "names" due to dynamic
devices from what I was told. I want to prevent the PHY interface from
having that same issue.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Tomasz Figa @ 2013-07-23 20:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130723194423.GA22984@kroah.com>
On Tuesday 23 of July 2013 12:44:23 Greg KH wrote:
> On Tue, Jul 23, 2013 at 08:31:05PM +0100, Mark Brown wrote:
> > > You don't "know" the id of the device you are looking up, due to
> > > multiple devices being in the system (dynamic ids, look back earlier
> > > in
> > > this thread for details about that.)
> >
> > I got copied in very late so don't have most of the thread I'm afraid,
> > I did try looking at web archives but didn't see a clear problem
> > statement. In any case this is why the APIs doing lookups do the
> > lookups in the context of the requesting device - devices ask for
> > whatever name they use locally.
>
> What do you mean by "locally"?
>
> The problem with the api was that the phy core wanted a id and a name to
> create a phy, and then later other code was doing a "lookup" based on
> the name and id (mushed together), because it "knew" that this device
> was the one it wanted.
>
> Just like the clock api, which, for multiple devices, has proven to
> cause problems. I don't want to see us accept an api that we know has
> issues in it now, I'd rather us fix it up properly.
>
> Subsystems should be able to create ids how ever they want to, and not
> rely on the code calling them to specify the names of the devices that
> way, otherwise the api is just too fragile.
>
> I think, that if you create a device, then just carry around the pointer
> to that device (in this case a phy) and pass it to whatever other code
> needs it. No need to do lookups on "known names" or anything else,
> just normal pointers, with no problems for multiple devices, busses, or
> naming issues.
PHY object is not a device, it is something that a device driver creates
(one or more instances of) when it is being probed. You don't have a clean
way to export this PHY object to other driver, other than keeping this PHY
on a list inside PHY core with some well-known ID (e.g. device name +
consumer port name/index, like in regulator core) and then to use this
well-known ID inside consumer driver as a lookup key passed to phy_get();
Actually I think for PHY case, exactly the same way as used for regulators
might be completely fine:
1. Each PHY would have some kind of platform, non-unique name, that is
just used to print some messages (like the platform/board name of a
regulator).
2. Each PHY would have an array of consumers. Consumer specifier would
consist of consumer device name and consumer port name - just like in
regulator subsystem.
3. PHY driver receives an array of, let's say, phy_init_data inside its
platform data that it would use to register its PHYs.
4. Consumer drivers would have constant consumer port names and wouldn't
receive any information about PHYs from platform code.
Code example:
[Board file]
static const struct phy_consumer_data usb_20_phy0_consumers[] = {
{
.devname = "foo-ehci",
.port = "usbphy",
},
};
static const struct phy_consumer_data usb_20_phy1_consumers[] = {
{
.devname = "foo-otg",
.port = "otgphy",
},
};
static const struct phy_init_data my_phys[] = {
{
.name = "USB 2.0 PHY 0",
.consumers = usb_20_phy0_consumers,
.num_consumers = ARRAY_SIZE(usb_20_phy0_consumers),
},
{
.name = "USB 2.0 PHY 1",
.consumers = usb_20_phy1_consumers,
.num_consumers = ARRAY_SIZE(usb_20_phy1_consumers),
},
{ }
};
static const struct platform_device usb_phy_pdev = {
.name = "foo-usbphy",
.id = -1,
.dev = {
.platform_data = my_phys,
},
};
[PHY driver]
static int foo_usbphy_probe(pdev)
{
struct foo_usbphy *foo;
struct phy_init_data *init_data = pdev->dev.platform_data;
/* ... */
// for each PHY in init_data {
phy_register(&foo->phy[i], &init_data[i]);
// }
/* ... */
}
[EHCI driver]
static int foo_ehci_probe(pdev)
{
struct phy *phy;
/* ... */
phy = phy_get(&pdev->dev, "usbphy");
/* ... */
}
[OTG driver]
static int foo_otg_probe(pdev)
{
struct phy *phy;
/* ... */
phy = phy_get(&pdev->dev, "otgphy");
/* ... */
}
> > > > Having to write platform data for everything gets old fast and the
> > > > code
> > > > duplication is pretty tedious...
> > >
> > > Adding a single pointer is "tedious"? Where is the "name" that you
> > > are
> > > going to lookup going to come from? That code doesn't write
> > > itself...
> >
> > It's adding platform data in the first place that gets tedious - and
> > of
> > course there's also DT and ACPI to worry about, it's not just a case
> > of
> > platform data and then you're done. Pushing the lookup into library
> > code means that drivers don't have to worry about any of this stuff.
>
> I agree, so just pass around the pointer to the phy and all is good. No
> need to worry about DT or ACPI or anything else.
With Device Tree we don't have board files anymore. How would you pass any
pointers between provider and consumer drivers in this case?
> > For most of the APIs doing this there is a clear and unambiguous name
> > in the hardware that can be used (and for hardware process reasons is
> > unlikely to get changed). The major exception to this is the clock
> > API since it is relatively rare to have clear, segregated IP level
> > information for IPs baked into larger chips. The other APIs tend to
> > be establishing chip to chip links.
>
> The clock api is having problems with multiple "names" due to dynamic
> devices from what I was told. I want to prevent the PHY interface from
> having that same issue.
Yes, the clock API has a problem related to the clock namespace being
global for all registered clock controllers. This is not a problem with
lookup in general, but with wrong lookup key chosen.
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Tomasz Figa @ 2013-07-23 20:20 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <Pine.LNX.4.44L0.1307231518310.1304-100000@iolanthe.rowland.org>
On Tuesday 23 of July 2013 15:36:00 Alan Stern wrote:
> On Tue, 23 Jul 2013, Tomasz Figa wrote:
> > IMHO it would be better if you provided some code example, but let's
> > try to check if I understood you correctly.
> >
> > 8><-------------------------------------------------------------------
> > -----
> >
> > [Board file]
> >
> > static struct phy my_phy;
> >
> > static struct platform_device phy_pdev = {
> >
> > /* ... */
> > .platform_data = &my_phy;
> > /* ... */
> >
> > };
> >
> > static struct platform_device phy_pdev = {
>
> This should be controller_pdev, not phy_pdev, yes?
Right. A copy-pasto.
>
> > /* ... */
> > .platform_data = &my_phy;
> > /* ... */
> >
> > };
> >
> > [Provider driver]
> >
> > struct phy *phy = pdev->dev.platform_data;
> >
> > ret = phy_create(phy);
> >
> > [Consumer driver]
> >
> > struct phy *phy = pdev->dev.platform_data;
> >
> > ret = phy_get(&pdev->dev, phy);
>
> Or even just phy_get(&pdev->dev), because phy_get() could be smart
> enough to to set phy = dev->platform_data.
Unless you need more than one PHY in this driver...
>
> > ----------------------------------------------------------------------
> > --><8
> >
> > Is this what you mean?
>
> That's what I was going to suggest too. The struct phy is defined in
> the board file, which already knows about all the PHYs that exist in
> the system. (Or perhaps it is allocated dynamically, so that when many
> board files are present in the same kernel, only the entries listed in
> the board file for the current system get created.)
Well, such dynamic allocation is a must. We don't accept non-multiplatform
aware code anymore, not even saying about multiboard.
> Then the
> structure's address is stored in the platform data and made available
> to both the provider and the consumer.
Yes, technically this can work. You would still have to perform some kind
of synchronization to make sure that the PHY bound to this structure is
actually present. This is again technically doable (e.g. a list of
registered struct phys inside PHY core).
> Even though the struct phy is defined (or allocated) in the board file,
> its contents don't get filled in until the PHY driver provides the
> details.
You can't assure this. Board file is free to do whatever it wants with
this struct. A clean solution would prevent this.
> > It's technically correct, but quality of this solution isn't really
> > nice, because it's a layering violation (at least if I understood
> > what you mean). This is because you need to have full definition of
> > struct phy in board file and a structure that is used as private data
> > in PHY core comes from platform code.
>
> You don't have to have a full definition in the board file. Just a
> partial definition -- most of the contents can be filled in later, when
> the PHY driver is ready to store the private data.
>
> It's not a layering violation for one region of the kernel to store
> private data in a structure defined by another part of the kernel.
> This happens all the time (e.g., dev_set_drvdata).
Not really. The phy struct is something that _is_ private data of PHY
subsystem, not something that can store private data of PHY subsystem
(sure it can store private data of particular PHY driver, but that's
another story) and only PHY subsystem should have access to its contents.
By the way, we need to consider other cases here as well, for example it
would be nice to have a single phy_get() function that works for both non-
DT and DT cases to make the consumer driver not have to worry whether it's
being probed from DT or not.
I'd suggest simply reusing the lookup method of regulator framework, just
as I suggested here:
http://thread.gmane.org/gmane.linux.ports.arm.kernel/252813/focus\x101661
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Tomasz Figa @ 2013-07-23 20:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130723180414.GA9630@kroah.com>
On Tuesday 23 of July 2013 11:04:14 Greg KH wrote:
> On Tue, Jul 23, 2013 at 07:48:11PM +0200, Tomasz Figa wrote:
> > On Tuesday 23 of July 2013 10:37:11 Greg KH wrote:
> > > On Tue, Jul 23, 2013 at 06:50:29PM +0200, Tomasz Figa wrote:
> > > > > Ick, no. Why can't you just pass the pointer to the phy itself?
> > > > > If
> > > > > you
> > > > > had a "priv" pointer to search from, then you could have just
> > > > > passed
> > > > > the
> > > > > original phy pointer in the first place, right?
> > > >
> > > > IMHO it would be better if you provided some code example, but
> > > > let's
> > > > try to check if I understood you correctly.
> > >
> > > It's not my code that I want to have added, so I don't have to write
> > > examples, I just get to complain about the existing stuff :)
> >
> > Still, I think that some small code snippets illustrating the idea are
> > really helpful.
> >
> > > > 8><---------------------------------------------------------------
> > > > -----
> > > > ----
> > > >
> > > > [Board file]
> > > >
> > > > static struct phy my_phy;
> > > >
> > > > static struct platform_device phy_pdev = {
> > > >
> > > > /* ... */
> > > > .platform_data = &my_phy;
> > > > /* ... */
> > > >
> > > > };
> > > >
> > > > static struct platform_device phy_pdev = {
> > > >
> > > > /* ... */
> > > > .platform_data = &my_phy;
> > > > /* ... */
> > > >
> > > > };
> > > >
> > > > [Provider driver]
> > > >
> > > > struct phy *phy = pdev->dev.platform_data;
> > > >
> > > > ret = phy_create(phy);
> > > >
> > > > [Consumer driver]
> > > >
> > > > struct phy *phy = pdev->dev.platform_data;
> > > >
> > > > ret = phy_get(&pdev->dev, phy);
> > > >
> > > > ------------------------------------------------------------------
> > > > -----
> > > > -><8
> > > >
> > > > Is this what you mean?
> > >
> > > No. Well, kind of. What's wrong with using the platform data
> > > structure unique to the board to have the pointer?
> > >
> > > For example (just randomly picking one), the ata-pxa driver would
> > > change include/linux/platform_data/ata-pxa.h to have a phy pointer
> > > in it:
> > >
> > > struct phy;
> > >
> > > struct pata_pxa_pdata {
> > >
> > > /* PXA DMA DREQ<0:2> pin */
> > > uint32_t dma_dreq;
> > > /* Register shift */
> > > uint32_t reg_shift;
> > > /* IRQ flags */
> > > uint32_t irq_flags;
> > > /* PHY */
> > > struct phy *phy;
> > >
> > > };
> > >
> > > Then, when you create the platform, set the phy* pointer with a call
> > > to
> > > phy_create(). Then you can use that pointer wherever that plaform
> > > data
> > > is available (i.e. whereever platform_data is at).
> >
> > Hmm? So, do you suggest to call phy_create() from board file? What
> > phy_ops struct and other hardware parameters would it take?
> >
> > > > > The issue is that a string "name" is not going to scale at all,
> > > > > as it
> > > > > requires hard-coded information that will change over time (as
> > > > > the
> > > > > existing clock interface is already showing.)
> > > >
> > > > I fully agree that a simple, single string will not scale even in
> > > > some,
> > > > not so uncommon cases, but there is already a lot of existing
> > > > lookup
> > > > solutions over the kernel and so there is no point in introducing
> > > > another one.
> > >
> > > I'm trying to get _rid_ of lookup "solutions" and just use a real
> > > pointer, as you should. I'll go tackle those other ones after this
> > > one
> > > is taken care of, to show how the others should be handled as well.
> >
> > There was a reason for introducing lookup solutions. The reason was
> > that in board file there is no way to get a pointer to something that
> > is going to be created much later in time. We don't do time travel
> > ;-).
> >
> > > > > Please just pass the real "phy" pointer around, that's what it
> > > > > is
> > > > > there
> > > > > for. Your "board binding" logic/code should be able to handle
> > > > > this,
> > > > > as
> > > > > it somehow was going to do the same thing with a "name".
> > > >
> > > > It's technically correct, but quality of this solution isn't
> > > > really
> > > > nice, because it's a layering violation (at least if I understood
> > > > what
> > > > you mean). This is because you need to have full definition of
> > > > struct
> > > > phy in board file and a structure that is used as private data in
> > > > PHY
> > > > core comes from platform code.
> > >
> > > No, just a pointer, you don't need the "full" structure until you
> > > get to some .c code that actually manipulates the phy itself, for
> > > all other places, you are just dealing with a pointer and a
> > > structure you never reference.
> > >
> > > Does that make more sense?
> >
> > Well, to the point that I think I now understood your suggestion.
> > Unfortunately the suggestion alone isn't really something that can be
> > done, considering how driver core and generic frameworks work.
>
> Ok, given that I seem to be totally confused as to exactly how the
> board-specific frameworks work, I'll take your word for it.
Well, they are working in a way that keeps separation of layers, making
things clean. Platform code should not (well, there might exist some in
tree hacks, but this should not be propagated) used to exchange data
between drivers, but rather to specify board specific parameters for
generic drivers. If drivers need to cooperate, there must be a dedicated
interface for this, like the PHY framework Kishon is introducing here.
Sure, with platform code you can do a lot of hacky things, for example you
can simply provide PHY callbacks inside platform_data, like it was being
done historically, but that's just ugly.
Anyway, board files should now be rather considered a historical thing. We
are moving towards full DT-based description on ARM systems and so board
files and related things, like name-based lookups, statically registered
platform devices and so one are going away. Device Tree handles such
provider-consumer links automatically using specifiers with phandles and
lookup by node + provider-specific specifier args, so all the problems
with binding things together just go away.
> But again, I will not accept "lookup by name" type solutions, when the
> "name" is dynamic and will change. Because you are using a "name", you
> can deal with a pointer, putting it _somewhere_ in your board-specific
> data structures, as you are going to need to store it anyway (hint, you
> had to get that "name" from somewhere, right?)
Yes. This kind of artificial names passed to both provider and consumer
isn't really a good way of lookup. This is just an example of bad lookup
key, though. IMHO for a good example of lookup you should see regulator
framework.
> And maybe the way that these "generic frameworks" are created is wrong,
> given that you don't feel that a generic pointer can be passed to the
> needed devices. That seems like a huge problem, one that has already
> been pointed out is causing issues with other subsystems.
What problem are you talking about here? AFAIK frameworks using correctly
designed lookup do work fine for most, if not all, people. See the
regulator framework.
> So maybe they need to be fixed?
I don't really think anything is broken here, so there is nothing to fix.
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Greg KH @ 2013-07-23 20:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1731726.KENstTPhkb@flatron>
On Tue, Jul 23, 2013 at 10:07:52PM +0200, Tomasz Figa wrote:
> On Tuesday 23 of July 2013 12:44:23 Greg KH wrote:
> > On Tue, Jul 23, 2013 at 08:31:05PM +0100, Mark Brown wrote:
> > > > You don't "know" the id of the device you are looking up, due to
> > > > multiple devices being in the system (dynamic ids, look back earlier
> > > > in
> > > > this thread for details about that.)
> > >
> > > I got copied in very late so don't have most of the thread I'm afraid,
> > > I did try looking at web archives but didn't see a clear problem
> > > statement. In any case this is why the APIs doing lookups do the
> > > lookups in the context of the requesting device - devices ask for
> > > whatever name they use locally.
> >
> > What do you mean by "locally"?
> >
> > The problem with the api was that the phy core wanted a id and a name to
> > create a phy, and then later other code was doing a "lookup" based on
> > the name and id (mushed together), because it "knew" that this device
> > was the one it wanted.
> >
> > Just like the clock api, which, for multiple devices, has proven to
> > cause problems. I don't want to see us accept an api that we know has
> > issues in it now, I'd rather us fix it up properly.
> >
> > Subsystems should be able to create ids how ever they want to, and not
> > rely on the code calling them to specify the names of the devices that
> > way, otherwise the api is just too fragile.
> >
> > I think, that if you create a device, then just carry around the pointer
> > to that device (in this case a phy) and pass it to whatever other code
> > needs it. No need to do lookups on "known names" or anything else,
> > just normal pointers, with no problems for multiple devices, busses, or
> > naming issues.
>
> PHY object is not a device, it is something that a device driver creates
> (one or more instances of) when it is being probed.
But you created a 'struct device' for it, so I think of it as a "device"
be it "virtual" or "real" :)
> You don't have a clean way to export this PHY object to other driver,
> other than keeping this PHY on a list inside PHY core with some
> well-known ID (e.g. device name + consumer port name/index, like in
> regulator core) and then to use this well-known ID inside consumer
> driver as a lookup key passed to phy_get();
>
> Actually I think for PHY case, exactly the same way as used for
> regulators might be completely fine:
>
> 1. Each PHY would have some kind of platform, non-unique name, that is
> just used to print some messages (like the platform/board name of a
> regulator).
> 2. Each PHY would have an array of consumers. Consumer specifier would
> consist of consumer device name and consumer port name - just like in
> regulator subsystem.
> 3. PHY driver receives an array of, let's say, phy_init_data inside its
> platform data that it would use to register its PHYs.
> 4. Consumer drivers would have constant consumer port names and wouldn't
> receive any information about PHYs from platform code.
>
> Code example:
>
> [Board file]
>
> static const struct phy_consumer_data usb_20_phy0_consumers[] = {
> {
> .devname = "foo-ehci",
> .port = "usbphy",
> },
> };
>
> static const struct phy_consumer_data usb_20_phy1_consumers[] = {
> {
> .devname = "foo-otg",
> .port = "otgphy",
> },
> };
>
> static const struct phy_init_data my_phys[] = {
> {
> .name = "USB 2.0 PHY 0",
> .consumers = usb_20_phy0_consumers,
> .num_consumers = ARRAY_SIZE(usb_20_phy0_consumers),
> },
> {
> .name = "USB 2.0 PHY 1",
> .consumers = usb_20_phy1_consumers,
> .num_consumers = ARRAY_SIZE(usb_20_phy1_consumers),
> },
> { }
> };
>
> static const struct platform_device usb_phy_pdev = {
> .name = "foo-usbphy",
> .id = -1,
> .dev = {
> .platform_data = my_phys,
> },
> };
>
> [PHY driver]
>
> static int foo_usbphy_probe(pdev)
> {
> struct foo_usbphy *foo;
> struct phy_init_data *init_data = pdev->dev.platform_data;
> /* ... */
> // for each PHY in init_data {
> phy_register(&foo->phy[i], &init_data[i]);
> // }
> /* ... */
> }
>
> [EHCI driver]
>
> static int foo_ehci_probe(pdev)
> {
> struct phy *phy;
> /* ... */
> phy = phy_get(&pdev->dev, "usbphy");
> /* ... */
> }
>
> [OTG driver]
>
> static int foo_otg_probe(pdev)
> {
> struct phy *phy;
> /* ... */
> phy = phy_get(&pdev->dev, "otgphy");
> /* ... */
> }
That's not so bad, as long as you let the phy core use whatever name it
wants for the device when it registers it with sysfs. Use the name you
are requesting as a "tag" or some such "hint" as to what the phy can be
looked up by.
Good luck handling duplicate "tags" :)
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Alan Stern @ 2013-07-23 20:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1374129984-765-2-git-send-email-kishon@ti.com>
On Tue, 23 Jul 2013, Tomasz Figa wrote:
> > That's what I was going to suggest too. The struct phy is defined in
> > the board file, which already knows about all the PHYs that exist in
> > the system. (Or perhaps it is allocated dynamically, so that when many
> > board files are present in the same kernel, only the entries listed in
> > the board file for the current system get created.)
>
> Well, such dynamic allocation is a must. We don't accept non-multiplatform
> aware code anymore, not even saying about multiboard.
>
> > Then the
> > structure's address is stored in the platform data and made available
> > to both the provider and the consumer.
>
> Yes, technically this can work. You would still have to perform some kind
> of synchronization to make sure that the PHY bound to this structure is
> actually present. This is again technically doable (e.g. a list of
> registered struct phys inside PHY core).
The synchronization takes place inside phy_get. If phy_create hasn't
been called for this structure by the time phy_get runs, phy_get will
return an error.
> > Even though the struct phy is defined (or allocated) in the board file,
> > its contents don't get filled in until the PHY driver provides the
> > details.
>
> You can't assure this. Board file is free to do whatever it wants with
> this struct. A clean solution would prevent this.
I'm not sure what you mean here. Of course I can't prevent a board
file from messing up a data structure. I can't prevent it from causing
memory access violations either; in fact, I can't prevent any bugs in
other people's code.
Besides, why do you say the board file is free to do whatever it wants
with the struct phy? Currently the struct phy is created by the PHY
provider and the PHY core, right? It's not even mentioned in the board
file.
> > > It's technically correct, but quality of this solution isn't really
> > > nice, because it's a layering violation (at least if I understood
> > > what you mean). This is because you need to have full definition of
> > > struct phy in board file and a structure that is used as private data
> > > in PHY core comes from platform code.
> >
> > You don't have to have a full definition in the board file. Just a
> > partial definition -- most of the contents can be filled in later, when
> > the PHY driver is ready to store the private data.
> >
> > It's not a layering violation for one region of the kernel to store
> > private data in a structure defined by another part of the kernel.
> > This happens all the time (e.g., dev_set_drvdata).
>
> Not really. The phy struct is something that _is_ private data of PHY
> subsystem, not something that can store private data of PHY subsystem
> (sure it can store private data of particular PHY driver, but that's
> another story) and only PHY subsystem should have access to its contents.
If you want to keep the phy struct completely separate from the board
file, there's an easy way to do it. Let's say the board file knows
about N different PHYs in the system. Then you define an array of N
pointers to phys:
struct phy *(phy_address[N]);
In the platform data for both PHY j and its controller, store
&phy_address[j]. The PHY provider passes this cookie to phy_create:
cookie = pdev->dev.platform_data;
ret = phy_create(phy, cookie);
and phy_create simply stores: *cookie = phy. The PHY consumer does
much the same the same thing:
cookie = pdev->dev.platform_data;
phy = phy_get(cookie);
phy_get returns *cookie if it isn't NULL, or an ERR_PTR otherwise.
> By the way, we need to consider other cases here as well, for example it
> would be nice to have a single phy_get() function that works for both non-
> DT and DT cases to make the consumer driver not have to worry whether it's
> being probed from DT or not.
You ought to be able to adapt this scheme to work with DT. Maybe by
having multiple "phy_address" arrays.
Alan Stern
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Tomasz Figa @ 2013-07-23 21:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <Pine.LNX.4.44L0.1307231632580.1304-100000@iolanthe.rowland.org>
On Tuesday 23 of July 2013 16:53:55 Alan Stern wrote:
> On Tue, 23 Jul 2013, Tomasz Figa wrote:
> > > That's what I was going to suggest too. The struct phy is defined
> > > in
> > > the board file, which already knows about all the PHYs that exist in
> > > the system. (Or perhaps it is allocated dynamically, so that when
> > > many
> > > board files are present in the same kernel, only the entries listed
> > > in
> > > the board file for the current system get created.)
> >
> > Well, such dynamic allocation is a must. We don't accept
> > non-multiplatform aware code anymore, not even saying about
> > multiboard.
> >
> > > Then the
> > > structure's address is stored in the platform data and made
> > > available
> > > to both the provider and the consumer.
> >
> > Yes, technically this can work. You would still have to perform some
> > kind of synchronization to make sure that the PHY bound to this
> > structure is actually present. This is again technically doable (e.g.
> > a list of registered struct phys inside PHY core).
>
> The synchronization takes place inside phy_get. If phy_create hasn't
> been called for this structure by the time phy_get runs, phy_get will
> return an error.
Yes, this is the solution that I had in mind when saying that this is
doable.
> > > Even though the struct phy is defined (or allocated) in the board
> > > file,
> > > its contents don't get filled in until the PHY driver provides the
> > > details.
> >
> > You can't assure this. Board file is free to do whatever it wants with
> > this struct. A clean solution would prevent this.
>
> I'm not sure what you mean here. Of course I can't prevent a board
> file from messing up a data structure. I can't prevent it from causing
> memory access violations either; in fact, I can't prevent any bugs in
> other people's code.
>
> Besides, why do you say the board file is free to do whatever it wants
> with the struct phy? Currently the struct phy is created by the PHY
> provider and the PHY core, right? It's not even mentioned in the board
> file.
I mean, if you have a struct type of which full declaration is available
for some code, this code can access any memeber of it without any hacks,
which is not something that we want to have in board files. The phy struct
should be opaque for them.
> > > > It's technically correct, but quality of this solution isn't
> > > > really
> > > > nice, because it's a layering violation (at least if I understood
> > > > what you mean). This is because you need to have full definition
> > > > of
> > > > struct phy in board file and a structure that is used as private
> > > > data
> > > > in PHY core comes from platform code.
> > >
> > > You don't have to have a full definition in the board file. Just a
> > > partial definition -- most of the contents can be filled in later,
> > > when
> > > the PHY driver is ready to store the private data.
> > >
> > > It's not a layering violation for one region of the kernel to store
> > > private data in a structure defined by another part of the kernel.
> > > This happens all the time (e.g., dev_set_drvdata).
> >
> > Not really. The phy struct is something that _is_ private data of PHY
> > subsystem, not something that can store private data of PHY subsystem
> > (sure it can store private data of particular PHY driver, but that's
> > another story) and only PHY subsystem should have access to its
> > contents.
> If you want to keep the phy struct completely separate from the board
> file, there's an easy way to do it. Let's say the board file knows
> about N different PHYs in the system. Then you define an array of N
> pointers to phys:
>
> struct phy *(phy_address[N]);
>
> In the platform data for both PHY j and its controller, store
> &phy_address[j]. The PHY provider passes this cookie to phy_create:
>
> cookie = pdev->dev.platform_data;
> ret = phy_create(phy, cookie);
>
> and phy_create simply stores: *cookie = phy. The PHY consumer does
> much the same the same thing:
>
> cookie = pdev->dev.platform_data;
> phy = phy_get(cookie);
>
> phy_get returns *cookie if it isn't NULL, or an ERR_PTR otherwise.
OK, this can work. Again, just technically, because it's rather ugly.
> > By the way, we need to consider other cases here as well, for example
> > it would be nice to have a single phy_get() function that works for
> > both non- DT and DT cases to make the consumer driver not have to
> > worry whether it's being probed from DT or not.
>
> You ought to be able to adapt this scheme to work with DT. Maybe by
> having multiple "phy_address" arrays.
Where would you want to have those phy_address arrays stored? There are no
board files when booting with DT. Not even saying that you don't need to
use any hacky schemes like this when you have DT that nicely specifies
relations between devices.
Anyway, board file should not be considered as a method to exchange data
between drivers. It should be used only to pass data from it to drivers,
not the other way. Ideally all data in a board file should be marked as
const and __init and dropped after system initialization.
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Tomasz Figa @ 2013-07-23 21:05 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130723205007.GA27166@kroah.com>
On Tuesday 23 of July 2013 13:50:07 Greg KH wrote:
> On Tue, Jul 23, 2013 at 10:07:52PM +0200, Tomasz Figa wrote:
> > On Tuesday 23 of July 2013 12:44:23 Greg KH wrote:
> > > On Tue, Jul 23, 2013 at 08:31:05PM +0100, Mark Brown wrote:
> > > > > You don't "know" the id of the device you are looking up, due to
> > > > > multiple devices being in the system (dynamic ids, look back
> > > > > earlier
> > > > > in
> > > > > this thread for details about that.)
> > > >
> > > > I got copied in very late so don't have most of the thread I'm
> > > > afraid,
> > > > I did try looking at web archives but didn't see a clear problem
> > > > statement. In any case this is why the APIs doing lookups do the
> > > > lookups in the context of the requesting device - devices ask for
> > > > whatever name they use locally.
> > >
> > > What do you mean by "locally"?
> > >
> > > The problem with the api was that the phy core wanted a id and a
> > > name to create a phy, and then later other code was doing a
> > > "lookup" based on the name and id (mushed together), because it
> > > "knew" that this device was the one it wanted.
> > >
> > > Just like the clock api, which, for multiple devices, has proven to
> > > cause problems. I don't want to see us accept an api that we know
> > > has
> > > issues in it now, I'd rather us fix it up properly.
> > >
> > > Subsystems should be able to create ids how ever they want to, and
> > > not
> > > rely on the code calling them to specify the names of the devices
> > > that
> > > way, otherwise the api is just too fragile.
> > >
> > > I think, that if you create a device, then just carry around the
> > > pointer to that device (in this case a phy) and pass it to whatever
> > > other code needs it. No need to do lookups on "known names" or
> > > anything else, just normal pointers, with no problems for multiple
> > > devices, busses, or naming issues.
> >
> > PHY object is not a device, it is something that a device driver
> > creates (one or more instances of) when it is being probed.
>
> But you created a 'struct device' for it, so I think of it as a "device"
> be it "virtual" or "real" :)
Keep in mind that those virtual devices are created by PHY driver bound to
a real device and one real device can have multiple virtual devices behind
it.
> > You don't have a clean way to export this PHY object to other driver,
> > other than keeping this PHY on a list inside PHY core with some
> > well-known ID (e.g. device name + consumer port name/index, like in
> > regulator core) and then to use this well-known ID inside consumer
> > driver as a lookup key passed to phy_get();
> >
> > Actually I think for PHY case, exactly the same way as used for
> > regulators might be completely fine:
> >
> > 1. Each PHY would have some kind of platform, non-unique name, that is
> > just used to print some messages (like the platform/board name of a
> > regulator).
> > 2. Each PHY would have an array of consumers. Consumer specifier would
> > consist of consumer device name and consumer port name - just like in
> > regulator subsystem.
> > 3. PHY driver receives an array of, let's say, phy_init_data inside
> > its
> > platform data that it would use to register its PHYs.
> > 4. Consumer drivers would have constant consumer port names and
> > wouldn't receive any information about PHYs from platform code.
> >
> > Code example:
> >
> > [Board file]
> >
> > static const struct phy_consumer_data usb_20_phy0_consumers[] = {
> >
> > {
> >
> > .devname = "foo-ehci",
> > .port = "usbphy",
> >
> > },
> >
> > };
> >
> > static const struct phy_consumer_data usb_20_phy1_consumers[] = {
> >
> > {
> >
> > .devname = "foo-otg",
> > .port = "otgphy",
> >
> > },
> >
> > };
> >
> > static const struct phy_init_data my_phys[] = {
> >
> > {
> >
> > .name = "USB 2.0 PHY 0",
> > .consumers = usb_20_phy0_consumers,
> > .num_consumers = ARRAY_SIZE(usb_20_phy0_consumers),
> >
> > },
> > {
> >
> > .name = "USB 2.0 PHY 1",
> > .consumers = usb_20_phy1_consumers,
> > .num_consumers = ARRAY_SIZE(usb_20_phy1_consumers),
> >
> > },
> > { }
> >
> > };
> >
> > static const struct platform_device usb_phy_pdev = {
> >
> > .name = "foo-usbphy",
> > .id = -1,
> > .dev = {
> >
> > .platform_data = my_phys,
> >
> > },
> >
> > };
> >
> > [PHY driver]
> >
> > static int foo_usbphy_probe(pdev)
> > {
> >
> > struct foo_usbphy *foo;
> > struct phy_init_data *init_data = pdev->dev.platform_data;
> > /* ... */
> > // for each PHY in init_data {
> >
> > phy_register(&foo->phy[i], &init_data[i]);
> >
> > // }
> > /* ... */
> >
> > }
> >
> > [EHCI driver]
> >
> > static int foo_ehci_probe(pdev)
> > {
> >
> > struct phy *phy;
> > /* ... */
> > phy = phy_get(&pdev->dev, "usbphy");
> > /* ... */
> >
> > }
> >
> > [OTG driver]
> >
> > static int foo_otg_probe(pdev)
> > {
> >
> > struct phy *phy;
> > /* ... */
> > phy = phy_get(&pdev->dev, "otgphy");
> > /* ... */
> >
> > }
>
> That's not so bad, as long as you let the phy core use whatever name it
> wants for the device when it registers it with sysfs.
Yes, in regulator core consumer names are completely separated from this.
Regulator core simply assigns a sequential integer ID to each regulator
and registers /sys/class/regulator/regulator.ID for each regulator.
> Use the name you
> are requesting as a "tag" or some such "hint" as to what the phy can be
> looked up by.
>
> Good luck handling duplicate "tags" :)
The tag alone is not a key. Lookup key consists of two components,
consumer device name and consumer tag. What kind of duplicate tags can be
a problem here?
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Alan Stern @ 2013-07-23 21:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1374129984-765-2-git-send-email-kishon@ti.com>
On Tue, 23 Jul 2013, Tomasz Figa wrote:
> > If you want to keep the phy struct completely separate from the board
> > file, there's an easy way to do it. Let's say the board file knows
> > about N different PHYs in the system. Then you define an array of N
> > pointers to phys:
> >
> > struct phy *(phy_address[N]);
> >
> > In the platform data for both PHY j and its controller, store
> > &phy_address[j]. The PHY provider passes this cookie to phy_create:
> >
> > cookie = pdev->dev.platform_data;
> > ret = phy_create(phy, cookie);
> >
> > and phy_create simply stores: *cookie = phy. The PHY consumer does
> > much the same the same thing:
> >
> > cookie = pdev->dev.platform_data;
> > phy = phy_get(cookie);
> >
> > phy_get returns *cookie if it isn't NULL, or an ERR_PTR otherwise.
>
> OK, this can work. Again, just technically, because it's rather ugly.
There's no reason the phy_address things have to be arrays. A separate
individual pointer for each PHY would work just as well.
> Where would you want to have those phy_address arrays stored? There are no
> board files when booting with DT. Not even saying that you don't need to
> use any hacky schemes like this when you have DT that nicely specifies
> relations between devices.
If everybody agrees DT has a nice scheme for specifying relations
between devices, why not use that same scheme in the PHY core?
> Anyway, board file should not be considered as a method to exchange data
> between drivers. It should be used only to pass data from it to drivers,
> not the other way. Ideally all data in a board file should be marked as
> const and __init and dropped after system initialization.
The phy_address things don't have to be defined or allocated in the
board file; they could be set up along with the platform data.
In any case, this was simply meant to be a suggestion to show that it
is relatively easy to do what you need without using name or ID
strings.
Alan Stern
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Greg KH @ 2013-07-23 21:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1769609.rbAYfG9ir3@flatron>
On Tue, Jul 23, 2013 at 11:05:48PM +0200, Tomasz Figa wrote:
> > That's not so bad, as long as you let the phy core use whatever name it
> > wants for the device when it registers it with sysfs.
>
> Yes, in regulator core consumer names are completely separated from this.
> Regulator core simply assigns a sequential integer ID to each regulator
> and registers /sys/class/regulator/regulator.ID for each regulator.
Yes, that's fine.
> > Use the name you
> > are requesting as a "tag" or some such "hint" as to what the phy can be
> > looked up by.
> >
> > Good luck handling duplicate "tags" :)
>
> The tag alone is not a key. Lookup key consists of two components,
> consumer device name and consumer tag. What kind of duplicate tags can be
> a problem here?
Ok, I didn't realize it looked at both parts, that makes sense, thanks.
greg k-h
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Tomasz Figa @ 2013-07-23 21:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <Pine.LNX.4.44L0.1307231708020.1304-100000@iolanthe.rowland.org>
On Tuesday 23 of July 2013 17:14:20 Alan Stern wrote:
> On Tue, 23 Jul 2013, Tomasz Figa wrote:
> > > If you want to keep the phy struct completely separate from the
> > > board
> > > file, there's an easy way to do it. Let's say the board file knows
> > > about N different PHYs in the system. Then you define an array of N
> > > pointers to phys:
> > >
> > > struct phy *(phy_address[N]);
> > >
> > > In the platform data for both PHY j and its controller, store
> > >
> > > &phy_address[j]. The PHY provider passes this cookie to phy_create:
> > > cookie = pdev->dev.platform_data;
> > > ret = phy_create(phy, cookie);
> > >
> > > and phy_create simply stores: *cookie = phy. The PHY consumer does
> > >
> > > much the same the same thing:
> > > cookie = pdev->dev.platform_data;
> > > phy = phy_get(cookie);
> > >
> > > phy_get returns *cookie if it isn't NULL, or an ERR_PTR otherwise.
> >
> > OK, this can work. Again, just technically, because it's rather ugly.
>
> There's no reason the phy_address things have to be arrays. A separate
> individual pointer for each PHY would work just as well.
>
> > Where would you want to have those phy_address arrays stored? There
> > are no board files when booting with DT. Not even saying that you
> > don't need to use any hacky schemes like this when you have DT that
> > nicely specifies relations between devices.
>
> If everybody agrees DT has a nice scheme for specifying relations
> between devices, why not use that same scheme in the PHY core?
It is already used, for cases when consumer device has a DT node attached.
In non-DT case this kind lookup translates loosely to something that is
being done in regulator framework - you can't bind devices by pointers,
because you don't have those pointers, so you need to use device names.
> > Anyway, board file should not be considered as a method to exchange
> > data between drivers. It should be used only to pass data from it to
> > drivers, not the other way. Ideally all data in a board file should
> > be marked as const and __init and dropped after system
> > initialization.
>
> The phy_address things don't have to be defined or allocated in the
> board file; they could be set up along with the platform data.
There is no platform data when booting with DT.
> In any case, this was simply meant to be a suggestion to show that it
> is relatively easy to do what you need without using name or ID
> strings.
Sure. It's good to have different options discussed as well.
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Mark Brown @ 2013-07-23 23:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130723194423.GA22984@kroah.com>
[-- Attachment #1: Type: text/plain, Size: 4782 bytes --]
On Tue, Jul 23, 2013 at 12:44:23PM -0700, Greg KH wrote:
> On Tue, Jul 23, 2013 at 08:31:05PM +0100, Mark Brown wrote:
> > statement. In any case this is why the APIs doing lookups do the
> > lookups in the context of the requesting device - devices ask for
> > whatever name they use locally.
> What do you mean by "locally"?
Within themselves - for example a regulator consumer asks for a given
supply on the device in terms of the supply names the device has.
> The problem with the api was that the phy core wanted a id and a name to
> create a phy, and then later other code was doing a "lookup" based on
> the name and id (mushed together), because it "knew" that this device
> was the one it wanted.
Ah, that sounds like the API is missing a component to link things
together. But I could be wrong. What I would expect to see is that the
consumer says "I want the PHY called X" and the PHY driver says "I
provide this set of PHYs" with a layer in between that plugs those
together. This would normally involve talking about the parent device
rather than the PHY itself.
> I think, that if you create a device, then just carry around the pointer
> to that device (in this case a phy) and pass it to whatever other code
> needs it. No need to do lookups on "known names" or anything else, just
> normal pointers, with no problems for multiple devices, busses, or
> naming issues.
I think you're not really talking about the lookup API at all here but
rather about one way in which the matching code can be written. What
everything *really* wants to do is work in terms of resources namespaced
within struct devices since every bit of hardware in the system should
have one of those it can use and if you have a struct device you can do
useful things like call dev_printk() and find the device tree data to do
device tree based lookups.
Unfortunately for a number of buses even when statically registering the
struct device doesn't get allocated until the device is probed so what
everyone fell back on doing was using dev_name() in cases where the
struct device wasn't there yet, or just always using it for consistency
since for most of the affected buses dev_name() is fixed for human
interface reasons. I think this is the issue you're concerned about
here since if the dev_name() is dynamically allocated this breaks down.
This only affects board files, DT and ACPI can both use their own data
structures to do the mapping.
I had thought you were talking about picking the names that the
consumers use (which isn't actually that big a deal, it's just a bit
annoying for the clock API).
> > It's adding platform data in the first place that gets tedious - and of
> > course there's also DT and ACPI to worry about, it's not just a case of
> > platform data and then you're done. Pushing the lookup into library
> > code means that drivers don't have to worry about any of this stuff.
> I agree, so just pass around the pointer to the phy and all is good. No
> need to worry about DT or ACPI or anything else.
No, in practice passing around the pointer gets tricky if you're using
something other than board files (or even are doing any kind of dynamic
stuff with board files) since the two devices need to find each other
and if you're using platform data then the code doing the matching has
to know about the platform data for every device it might need to match
which is just miserable.
Something would need to do something like allocate the PHY objects and
then arrange for them to be passed to both provider and consumer devices
prior to those being registered, knowing where to place the pointers in
the platform data for each device. This is straightforward with board
files but not otherwise, people have tried this before.
> > For most of the APIs doing this there is a clear and unambiguous name in
> > the hardware that can be used (and for hardware process reasons is
> > unlikely to get changed). The major exception to this is the clock API
> > since it is relatively rare to have clear, segregated IP level
> > information for IPs baked into larger chips. The other APIs tend to be
> > establishing chip to chip links.
> The clock api is having problems with multiple "names" due to dynamic
> devices from what I was told. I want to prevent the PHY interface from
> having that same issue.
I think the underlying issue here is that we don't have a good enough
general way for board files (or other C code but mostly them) to talk
about devices prior to their being registered - rather than have the
pointer you're talking about be the PHY object itself have the pointer
be something which allows us to match the struct device when it's
created. This should be transparent to drivers and would be usable by
all the existing APIs.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH] video: xilinxfb: Fix compilation warning
From: Michal Simek @ 2013-07-24 5:31 UTC (permalink / raw)
To: Jean-Christophe Plagniol-Villard
Cc: Michal Simek, Tomi Valkeinen, linux-fbdev, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1036 bytes --]
regs_phys is phys_addr_t (u32 or u64).
Lets retype it to u64.
Fixes compilation warning introduced by:
video: xilinxfb: Use drvdata->regs_phys instead of physaddr
(sha1: c88fafef0135e1e1c3e23c3e32ccbeeabc587f81)
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
ppc44x_defconfig
Fixes regressions in v3.11-rc2
---
drivers/video/xilinxfb.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/video/xilinxfb.c b/drivers/video/xilinxfb.c
index f3d4a69..79175a6 100644
--- a/drivers/video/xilinxfb.c
+++ b/drivers/video/xilinxfb.c
@@ -341,8 +341,8 @@ static int xilinxfb_assign(struct platform_device *pdev,
if (drvdata->flags & BUS_ACCESS_FLAG) {
/* Put a banner in the log (for DEBUG) */
- dev_dbg(dev, "regs: phys=%x, virt=%p\n", drvdata->regs_phys,
- drvdata->regs);
+ dev_dbg(dev, "regs: phys=%llx, virt=%p\n",
+ (unsigned long long)drvdata->regs_phys, drvdata->regs);
}
/* Put a banner in the log (for DEBUG) */
dev_dbg(dev, "fb: phys=%llx, virt=%p, size=%x\n",
--
1.8.2.3
[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply related
* Re: [PATCH] video: xilinxfb: Fix compilation warning
From: Jingoo Han @ 2013-07-24 6:00 UTC (permalink / raw)
To: 'Michal Simek',
'Jean-Christophe Plagniol-Villard'
Cc: 'Michal Simek', 'Tomi Valkeinen', linux-fbdev,
linux-kernel, 'Stepan Moskovchenko', Jingoo Han
In-Reply-To: <ed64cda456be840147ae1f04bec6a99733f41c24.1374643896.git.michal.simek@xilinx.com>
On Wednesday, July 24, 2013 2:32 PM, Michal Simek wrote:
>
> regs_phys is phys_addr_t (u32 or u64).
> Lets retype it to u64.
>
> Fixes compilation warning introduced by:
> video: xilinxfb: Use drvdata->regs_phys instead of physaddr
> (sha1: c88fafef0135e1e1c3e23c3e32ccbeeabc587f81)
CC'ed Stepan Moskovchenko
phys_addr_t is defined as below:
#ifdef CONFIG_PHYS_ADDR_T_64BIT
typedef u64 phys_addr_t;
#else
typedef u32 phys_addr_t;
#endif
According to 'Documentation/printk-formats.txt',
Physical addresses:
%pa 0x01234567 or 0x0123456789abcdef
For printing a phys_addr_t type (and its derivatives, such as
resource_size_t) which can vary based on build options, regardless of
the width of the CPU data path. Passed by reference.
Thus, '%pa' option looks proper, instead of casting (unsigned long long).
dev_dbg(dev, "regs: phys=%pa, virt=%p\n", drvdata->regs_phys,
drvdata->regs);
Best regards,
Jingoo Han
>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
> ppc44x_defconfig
> Fixes regressions in v3.11-rc2
>
> ---
> drivers/video/xilinxfb.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/xilinxfb.c b/drivers/video/xilinxfb.c
> index f3d4a69..79175a6 100644
> --- a/drivers/video/xilinxfb.c
> +++ b/drivers/video/xilinxfb.c
> @@ -341,8 +341,8 @@ static int xilinxfb_assign(struct platform_device *pdev,
>
> if (drvdata->flags & BUS_ACCESS_FLAG) {
> /* Put a banner in the log (for DEBUG) */
> - dev_dbg(dev, "regs: phys=%x, virt=%p\n", drvdata->regs_phys,
> - drvdata->regs);
> + dev_dbg(dev, "regs: phys=%llx, virt=%p\n",
> + (unsigned long long)drvdata->regs_phys, drvdata->regs);
> }
> /* Put a banner in the log (for DEBUG) */
> dev_dbg(dev, "fb: phys=%llx, virt=%p, size=%x\n",
> --
> 1.8.2.3
>
^ permalink raw reply
* Re: [PATCH] video: xilinxfb: Fix compilation warning
From: Michal Simek @ 2013-07-24 7:17 UTC (permalink / raw)
To: Vivek Subbarao
Cc: Michal Simek, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
linux-fbdev, linux-kernel
In-Reply-To: <CAAY4GiVG-c63oJO1Kqv0Jf2Wa2Ba+hgLtHQYFqyuRW-f6hJ14Q@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 678 bytes --]
On 07/24/2013 08:05 AM, Vivek Subbarao wrote:
> Why is there a necessity to type cast to unsigned long long ? Whats the
> warning ?
>
Geerts: Build regressions/improvements in v3.11-rc2
+ drivers/video/xilinxfb.c: warning: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'phys_addr_t' [-Wformat]: => 344:3
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply
* Re: [PATCH] video: xilinxfb: Fix compilation warning
From: Michal Simek @ 2013-07-24 7:18 UTC (permalink / raw)
To: Jingoo Han
Cc: 'Michal Simek',
'Jean-Christophe Plagniol-Villard',
'Tomi Valkeinen', linux-fbdev, linux-kernel,
'Stepan Moskovchenko'
In-Reply-To: <007401ce8833$22ce0600$686a1200$@samsung.com>
[-- Attachment #1: Type: text/plain, Size: 1438 bytes --]
On 07/24/2013 08:00 AM, Jingoo Han wrote:
> On Wednesday, July 24, 2013 2:32 PM, Michal Simek wrote:
>>
>> regs_phys is phys_addr_t (u32 or u64).
>> Lets retype it to u64.
>>
>> Fixes compilation warning introduced by:
>> video: xilinxfb: Use drvdata->regs_phys instead of physaddr
>> (sha1: c88fafef0135e1e1c3e23c3e32ccbeeabc587f81)
>
> CC'ed Stepan Moskovchenko
>
>
> phys_addr_t is defined as below:
>
> #ifdef CONFIG_PHYS_ADDR_T_64BIT
> typedef u64 phys_addr_t;
> #else
> typedef u32 phys_addr_t;
> #endif
>
> According to 'Documentation/printk-formats.txt',
> Physical addresses:
> %pa 0x01234567 or 0x0123456789abcdef
> For printing a phys_addr_t type (and its derivatives, such as
> resource_size_t) which can vary based on build options, regardless of
> the width of the CPU data path. Passed by reference.
>
> Thus, '%pa' option looks proper, instead of casting (unsigned long long).
>
> dev_dbg(dev, "regs: phys=%pa, virt=%p\n", drvdata->regs_phys,
> drvdata->regs);
>
Ah. Wasn't aware about that.
Will retest.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply
* Re: [PATCH] video: xilinxfb: Fix compilation warning
From: Michal Simek @ 2013-07-24 7:41 UTC (permalink / raw)
To: monstr
Cc: Jingoo Han, 'Michal Simek',
'Jean-Christophe Plagniol-Villard',
'Tomi Valkeinen', linux-fbdev, linux-kernel,
'Stepan Moskovchenko'
In-Reply-To: <51EF7FBD.8070103@monstr.eu>
[-- Attachment #1: Type: text/plain, Size: 3286 bytes --]
On 07/24/2013 09:18 AM, Michal Simek wrote:
> On 07/24/2013 08:00 AM, Jingoo Han wrote:
>> On Wednesday, July 24, 2013 2:32 PM, Michal Simek wrote:
>>>
>>> regs_phys is phys_addr_t (u32 or u64).
>>> Lets retype it to u64.
>>>
>>> Fixes compilation warning introduced by:
>>> video: xilinxfb: Use drvdata->regs_phys instead of physaddr
>>> (sha1: c88fafef0135e1e1c3e23c3e32ccbeeabc587f81)
>>
>> CC'ed Stepan Moskovchenko
>>
>>
>> phys_addr_t is defined as below:
>>
>> #ifdef CONFIG_PHYS_ADDR_T_64BIT
>> typedef u64 phys_addr_t;
>> #else
>> typedef u32 phys_addr_t;
>> #endif
>>
>> According to 'Documentation/printk-formats.txt',
>> Physical addresses:
>> %pa 0x01234567 or 0x0123456789abcdef
>> For printing a phys_addr_t type (and its derivatives, such as
>> resource_size_t) which can vary based on build options, regardless of
>> the width of the CPU data path. Passed by reference.
>>
>> Thus, '%pa' option looks proper, instead of casting (unsigned long long).
>>
>> dev_dbg(dev, "regs: phys=%pa, virt=%p\n", drvdata->regs_phys,
>> drvdata->regs);
>>
>
> Ah. Wasn't aware about that.
> Will retest.
On ppc44x_defconfig
$ powerpc-unknown-linux-gnu-gcc --version
powerpc-unknown-linux-gnu-gcc (crosstool-NG-svn_unknown@20110406.104745) 4.3.2
This fix
diff --git a/drivers/video/xilinxfb.c b/drivers/video/xilinxfb.c
index 79175a6..a9a1167 100644
--- a/drivers/video/xilinxfb.c
+++ b/drivers/video/xilinxfb.c
@@ -341,12 +341,12 @@ static int xilinxfb_assign(struct platform_device *pdev,
if (drvdata->flags & BUS_ACCESS_FLAG) {
/* Put a banner in the log (for DEBUG) */
- dev_dbg(dev, "regs: phys=%llx, virt=%p\n",
- (unsigned long long)drvdata->regs_phys, drvdata->regs);
+ dev_dbg(dev, "regs: phys=%pa, virt=%p\n",
+ drvdata->regs_phys, drvdata->regs);
}
/* Put a banner in the log (for DEBUG) */
- dev_dbg(dev, "fb: phys=%llx, virt=%p, size=%x\n",
- (unsigned long long)drvdata->fb_phys, drvdata->fb_virt, fbsize);
+ dev_dbg(dev, "fb: phys=%pa, virt=%p, size=%x\n",
+ drvdata->fb_phys, drvdata->fb_virt, fbsize);
return 0; /* success */
Generates two warnings even it should be ok according to link to specification you sent.
CC [M] drivers/video/xilinxfb.o
drivers/video/xilinxfb.c: In function 'xilinxfb_assign':
drivers/video/xilinxfb.c:344: warning: format '%p' expects type 'void *', but argument 4 has type 'phys_addr_t'
drivers/video/xilinxfb.c:348: warning: format '%p' expects type 'void *', but argument 4 has type 'dma_addr_t'
On microblaze toolchain I see the same warnings. (mmu_defconfig)
I have also grepped the kernel and I see that it is used in 4 c files which seems to me
weird because phy_addr_t or dma_addr_t are used on a lot of places.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply related
* Re: [PATCH] video: xilinxfb: Fix compilation warning
From: Jingoo Han @ 2013-07-24 8:08 UTC (permalink / raw)
To: Michal Simek
Cc: 'Michal Simek',
'Jean-Christophe Plagniol-Villard',
'Tomi Valkeinen', linux-fbdev, linux-kernel,
'Stepan Moskovchenko', Jingoo Han
In-Reply-To: <51EF8530.90809@monstr.eu>
On Wednesday, July 24, 2013 4:42 PM, Michal Simek wrote:
> On 07/24/2013 09:18 AM, Michal Simek wrote:
> > On 07/24/2013 08:00 AM, Jingoo Han wrote:
> >> On Wednesday, July 24, 2013 2:32 PM, Michal Simek wrote:
> >>>
> >>> regs_phys is phys_addr_t (u32 or u64).
> >>> Lets retype it to u64.
> >>>
> >>> Fixes compilation warning introduced by:
> >>> video: xilinxfb: Use drvdata->regs_phys instead of physaddr
> >>> (sha1: c88fafef0135e1e1c3e23c3e32ccbeeabc587f81)
> >>
> >> CC'ed Stepan Moskovchenko
> >>
> >>
> >> phys_addr_t is defined as below:
> >>
> >> #ifdef CONFIG_PHYS_ADDR_T_64BIT
> >> typedef u64 phys_addr_t;
> >> #else
> >> typedef u32 phys_addr_t;
> >> #endif
> >>
> >> According to 'Documentation/printk-formats.txt',
> >> Physical addresses:
> >> %pa 0x01234567 or 0x0123456789abcdef
> >> For printing a phys_addr_t type (and its derivatives, such as
> >> resource_size_t) which can vary based on build options, regardless of
> >> the width of the CPU data path. Passed by reference.
> >>
> >> Thus, '%pa' option looks proper, instead of casting (unsigned long long).
> >>
> >> dev_dbg(dev, "regs: phys=%pa, virt=%p\n", drvdata->regs_phys,
> >> drvdata->regs);
> >>
> >
> > Ah. Wasn't aware about that.
> > Will retest.
>
> On ppc44x_defconfig
>
> $ powerpc-unknown-linux-gnu-gcc --version
> powerpc-unknown-linux-gnu-gcc (crosstool-NG-svn_unknown@20110406.104745) 4.3.2
>
> This fix
> diff --git a/drivers/video/xilinxfb.c b/drivers/video/xilinxfb.c
> index 79175a6..a9a1167 100644
> --- a/drivers/video/xilinxfb.c
> +++ b/drivers/video/xilinxfb.c
> @@ -341,12 +341,12 @@ static int xilinxfb_assign(struct platform_device *pdev,
>
> if (drvdata->flags & BUS_ACCESS_FLAG) {
> /* Put a banner in the log (for DEBUG) */
> - dev_dbg(dev, "regs: phys=%llx, virt=%p\n",
> - (unsigned long long)drvdata->regs_phys, drvdata->regs);
> + dev_dbg(dev, "regs: phys=%pa, virt=%p\n",
> + drvdata->regs_phys, drvdata->regs);
> }
> /* Put a banner in the log (for DEBUG) */
> - dev_dbg(dev, "fb: phys=%llx, virt=%p, size=%x\n",
> - (unsigned long long)drvdata->fb_phys, drvdata->fb_virt, fbsize);
> + dev_dbg(dev, "fb: phys=%pa, virt=%p, size=%x\n",
> + drvdata->fb_phys, drvdata->fb_virt, fbsize);
>
Hi Michal Simek,
Just now, I tested that the same problem happens on ARM config.
Also, I solved it by adding '&' operator.
'&' operator is necessary as below:
dev_dbg(dev, "regs: phys=%pa, virt=%p\n",
&drvdata->regs_phys, drvdata->regs);
^^^
dev_dbg(dev, "fb: phys=%pa, virt=%p, size=%x\n",
&drvdata->fb_phys, drvdata->fb_virt, fbsize);
^^^
Best regards,
Jingoo Han
> return 0; /* success */
>
> Generates two warnings even it should be ok according to link to specification you sent.
> CC [M] drivers/video/xilinxfb.o
> drivers/video/xilinxfb.c: In function 'xilinxfb_assign':
> drivers/video/xilinxfb.c:344: warning: format '%p' expects type 'void *', but argument 4 has type
> 'phys_addr_t'
> drivers/video/xilinxfb.c:348: warning: format '%p' expects type 'void *', but argument 4 has type
> 'dma_addr_t'
>
> On microblaze toolchain I see the same warnings. (mmu_defconfig)
>
> I have also grepped the kernel and I see that it is used in 4 c files which seems to me
> weird because phy_addr_t or dma_addr_t are used on a lot of places.
>
> Thanks,
> Michal
>
> --
> Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
> w: www.monstr.eu p: +42-0-721842854
> Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
> Maintainer of Linux kernel - Xilinx Zynq ARM architecture
> Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
>
^ permalink raw reply
* Re: [PATCH] video: xilinxfb: Fix compilation warning
From: Michal Simek @ 2013-07-24 8:24 UTC (permalink / raw)
To: Jingoo Han
Cc: 'Michal Simek',
'Jean-Christophe Plagniol-Villard',
'Tomi Valkeinen', linux-fbdev, linux-kernel,
'Stepan Moskovchenko'
In-Reply-To: <003a01ce8845$06df46e0$149dd4a0$@samsung.com>
[-- Attachment #1: Type: text/plain, Size: 3494 bytes --]
On 07/24/2013 10:08 AM, Jingoo Han wrote:
> On Wednesday, July 24, 2013 4:42 PM, Michal Simek wrote:
>> On 07/24/2013 09:18 AM, Michal Simek wrote:
>>> On 07/24/2013 08:00 AM, Jingoo Han wrote:
>>>> On Wednesday, July 24, 2013 2:32 PM, Michal Simek wrote:
>>>>>
>>>>> regs_phys is phys_addr_t (u32 or u64).
>>>>> Lets retype it to u64.
>>>>>
>>>>> Fixes compilation warning introduced by:
>>>>> video: xilinxfb: Use drvdata->regs_phys instead of physaddr
>>>>> (sha1: c88fafef0135e1e1c3e23c3e32ccbeeabc587f81)
>>>>
>>>> CC'ed Stepan Moskovchenko
>>>>
>>>>
>>>> phys_addr_t is defined as below:
>>>>
>>>> #ifdef CONFIG_PHYS_ADDR_T_64BIT
>>>> typedef u64 phys_addr_t;
>>>> #else
>>>> typedef u32 phys_addr_t;
>>>> #endif
>>>>
>>>> According to 'Documentation/printk-formats.txt',
>>>> Physical addresses:
>>>> %pa 0x01234567 or 0x0123456789abcdef
>>>> For printing a phys_addr_t type (and its derivatives, such as
>>>> resource_size_t) which can vary based on build options, regardless of
>>>> the width of the CPU data path. Passed by reference.
>>>>
>>>> Thus, '%pa' option looks proper, instead of casting (unsigned long long).
>>>>
>>>> dev_dbg(dev, "regs: phys=%pa, virt=%p\n", drvdata->regs_phys,
>>>> drvdata->regs);
>>>>
>>>
>>> Ah. Wasn't aware about that.
>>> Will retest.
>>
>> On ppc44x_defconfig
>>
>> $ powerpc-unknown-linux-gnu-gcc --version
>> powerpc-unknown-linux-gnu-gcc (crosstool-NG-svn_unknown@20110406.104745) 4.3.2
>>
>> This fix
>> diff --git a/drivers/video/xilinxfb.c b/drivers/video/xilinxfb.c
>> index 79175a6..a9a1167 100644
>> --- a/drivers/video/xilinxfb.c
>> +++ b/drivers/video/xilinxfb.c
>> @@ -341,12 +341,12 @@ static int xilinxfb_assign(struct platform_device *pdev,
>>
>> if (drvdata->flags & BUS_ACCESS_FLAG) {
>> /* Put a banner in the log (for DEBUG) */
>> - dev_dbg(dev, "regs: phys=%llx, virt=%p\n",
>> - (unsigned long long)drvdata->regs_phys, drvdata->regs);
>> + dev_dbg(dev, "regs: phys=%pa, virt=%p\n",
>> + drvdata->regs_phys, drvdata->regs);
>> }
>> /* Put a banner in the log (for DEBUG) */
>> - dev_dbg(dev, "fb: phys=%llx, virt=%p, size=%x\n",
>> - (unsigned long long)drvdata->fb_phys, drvdata->fb_virt, fbsize);
>> + dev_dbg(dev, "fb: phys=%pa, virt=%p, size=%x\n",
>> + drvdata->fb_phys, drvdata->fb_virt, fbsize);
>>
>
> Hi Michal Simek,
>
> Just now, I tested that the same problem happens on ARM config.
> Also, I solved it by adding '&' operator.
>
> '&' operator is necessary as below:
>
> dev_dbg(dev, "regs: phys=%pa, virt=%p\n",
> &drvdata->regs_phys, drvdata->regs);
> ^^^
>
> dev_dbg(dev, "fb: phys=%pa, virt=%p, size=%x\n",
> &drvdata->fb_phys, drvdata->fb_virt, fbsize);
> ^^^
ok.
The truth is as I said there are just 5 files in the whole kernel
which are using %pa and if %pa is right way to go we should
probably fix all off them.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox