[2/2] spi/fsl-lib: Get the SPI controller bus number from DTS
diff mbox

Message ID 1394789757-40732-2-git-send-email-B48286@freescale.com
State New, archived
Headers show

Commit Message

Hou Zhiqiang March 14, 2014, 9:35 a.m. UTC
Get the spi_master's bus_num from DTS to make the spi_master's name
static. So "mtdparts=spi.bus_num.chip_select:..." in cmdline can be
used to asign mtd partions of spi flash.

Signed-off-by: Hou Zhiqiang <B48286@freescale.com>
---
 drivers/spi/spi-fsl-lib.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Mark Brown March 14, 2014, 7:19 p.m. UTC | #1
On Fri, Mar 14, 2014 at 05:35:57PM +0800, Hou Zhiqiang wrote:
> Get the spi_master's bus_num from DTS to make the spi_master's name
> static. So "mtdparts=spi.bus_num.chip_select:..." in cmdline can be
> used to asign mtd partions of spi flash.

If we are going to do this it shouldn't be device specific (it should be
done in the framework since nothing is specific to the controller there)
but I'm not convinced that we should be doing it - this is all very
Linux specific.

The DT already has support for specifying flash layouts, can't those be
used (for example via chosen if they're not fixed for the board)?  Or if
it's just picking the correct filesystem then UUIDs and labels are the
standard way to do things.
Hou Zhiqiang March 17, 2014, 5:11 a.m. UTC | #2
> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: Saturday, March 15, 2014 3:19 AM
> To: Hou Zhiqiang-B48286
> Cc: linux-spi@vger.kernel.org; devicetree@vger.kernel.org;
> rob.herring@calxeda.com; pawel.moll@arm.com; mark.rutland@arm.com;
> ijc+devicetree@hellion.org.uk; galak@codeaurora.org;
> grant.likely@secretlab.ca; Wood Scott-B07421; Hu Mingkai-B21284
> Subject: Re: [PATCH 2/2] spi/fsl-lib: Get the SPI controller bus number
> from DTS
> 
> On Fri, Mar 14, 2014 at 05:35:57PM +0800, Hou Zhiqiang wrote:
> > Get the spi_master's bus_num from DTS to make the spi_master's name
> > static. So "mtdparts=spi.bus_num.chip_select:..." in cmdline can be
> > used to asign mtd partions of spi flash.
> 
> If we are going to do this it shouldn't be device specific (it should be
> done in the framework since nothing is specific to the controller there)
> but I'm not convinced that we should be doing it - this is all very Linux
> specific.

This patch just assign a bus number to the controller. It is driver's 
responsibility to distribute a bus number to spi_master and the definition
of bus_num is used to distinguish controllers. So, it is specific for the
controller and doesn't affect the framework. 

> 
> The DT already has support for specifying flash layouts, can't those be
> used (for example via chosen if they're not fixed for the board)?  Or if
> it's just picking the correct filesystem then UUIDs and labels are the
> standard way to do things.

The DT specifying flash layouts is ok. There is another way to make the
flash layouts using command line, but it is not safe because of the dynamic
bus_num. It is not the reason that the way of DT is supported flash layouts,
to live the other way unsafe, right?

Thanks,
Zhiqiang
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown March 17, 2014, 1:01 p.m. UTC | #3
On Mon, Mar 17, 2014 at 05:11:11AM +0000, B48286@freescale.com wrote:
> > On Fri, Mar 14, 2014 at 05:35:57PM +0800, Hou Zhiqiang wrote:
> > > Get the spi_master's bus_num from DTS to make the spi_master's name
> > > static. So "mtdparts=spi.bus_num.chip_select:..." in cmdline can be
> > > used to asign mtd partions of spi flash.

> > If we are going to do this it shouldn't be device specific (it should be
> > done in the framework since nothing is specific to the controller there)
> > but I'm not convinced that we should be doing it - this is all very Linux
> > specific.

> This patch just assign a bus number to the controller. It is driver's 
> responsibility to distribute a bus number to spi_master and the definition
> of bus_num is used to distinguish controllers. So, it is specific for the
> controller and doesn't affect the framework. 

You are adding a property to specify a bus number.  There is no reason
why such a property should be specific to a single controller, every
controller in every system has a bus number assigned to it so if we have
a way to specify one controller via the device tree we should have a way
to specify it for all.

> > The DT already has support for specifying flash layouts, can't those be
> > used (for example via chosen if they're not fixed for the board)?  Or if
> > it's just picking the correct filesystem then UUIDs and labels are the
> > standard way to do things.

> The DT specifying flash layouts is ok. There is another way to make the
> flash layouts using command line, but it is not safe because of the dynamic
> bus_num. It is not the reason that the way of DT is supported flash layouts,
> to live the other way unsafe, right?

This sounds to me like we need a better way of talking about flash
device names on the Linux command line rather than a way to fix the bus
number - for example, being able to refer to them using a fixed property
like the physical address.  Being able to refer to devices via an alias
assigned in the DT would also be useful (and more readable), I think
there may already be a mechanism for doing that which would need to be
plumbed in but I'm not 100% sure.
Hou Zhiqiang March 18, 2014, 7:40 a.m. UTC | #4
> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: Monday, March 17, 2014 9:01 PM
> To: Hou Zhiqiang-B48286
> Cc: linux-spi@vger.kernel.org; devicetree@vger.kernel.org;
> rob.herring@calxeda.com; pawel.moll@arm.com; mark.rutland@arm.com;
> ijc+devicetree@hellion.org.uk; galak@codeaurora.org;
> grant.likely@secretlab.ca; Wood Scott-B07421; Hu Mingkai-B21284
> Subject: Re: [PATCH 2/2] spi/fsl-lib: Get the SPI controller bus number
> from DTS
> 
> On Mon, Mar 17, 2014 at 05:11:11AM +0000, B48286@freescale.com wrote:
> > > On Fri, Mar 14, 2014 at 05:35:57PM +0800, Hou Zhiqiang wrote:
> > > > Get the spi_master's bus_num from DTS to make the spi_master's
> > > > name static. So "mtdparts=spi.bus_num.chip_select:..." in cmdline
> > > > can be used to asign mtd partions of spi flash.
> 
> > > If we are going to do this it shouldn't be device specific (it
> > > should be done in the framework since nothing is specific to the
> > > controller there) but I'm not convinced that we should be doing it -
> > > this is all very Linux specific.
> 
> > This patch just assign a bus number to the controller. It is driver's
> > responsibility to distribute a bus number to spi_master and the
> > definition of bus_num is used to distinguish controllers. So, it is
> > specific for the controller and doesn't affect the framework.
> 
> You are adding a property to specify a bus number.  There is no reason
> why such a property should be specific to a single controller, every
> controller in every system has a bus number assigned to it so if we have
> a way to specify one controller via the device tree we should have a way
> to specify it for all.
> 

The reason to specify a bus number to a single controller is to avoid 
allocating one dynamically, so we know the bus number before booting up the
kernel. If there are several controllers, you should add this property to all
of them. But it is unnecessary to add this property, if you do not care about
the bus number, and it will be allocated dynamically. 

> > > The DT already has support for specifying flash layouts, can't those
> > > be used (for example via chosen if they're not fixed for the board)?
> > > Or if it's just picking the correct filesystem then UUIDs and labels
> > > are the standard way to do things.
> 
> > The DT specifying flash layouts is ok. There is another way to make
> > the flash layouts using command line, but it is not safe because of
> > the dynamic bus_num. It is not the reason that the way of DT is
> > supported flash layouts, to live the other way unsafe, right?
> 
> This sounds to me like we need a better way of talking about flash device
> names on the Linux command line rather than a way to fix the bus number -
> for example, being able to refer to them using a fixed property like the
> physical address.  Being able to refer to devices via an alias assigned
> in the DT would also be useful (and more readable), I think there may
> already be a mechanism for doing that which would need to be plumbed in
> but I'm not 100% sure.

The bus number is the variable designed to distinguish one spi controller
from others. Why spi controller's physical address must be use instead of
bus number?
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Geert Uytterhoeven March 18, 2014, 8:55 a.m. UTC | #5
On Tue, Mar 18, 2014 at 8:40 AM, B48286@freescale.com
<B48286@freescale.com> wrote:
>> > > The DT already has support for specifying flash layouts, can't those
>> > > be used (for example via chosen if they're not fixed for the board)?
>> > > Or if it's just picking the correct filesystem then UUIDs and labels
>> > > are the standard way to do things.
>>
>> > The DT specifying flash layouts is ok. There is another way to make
>> > the flash layouts using command line, but it is not safe because of
>> > the dynamic bus_num. It is not the reason that the way of DT is
>> > supported flash layouts, to live the other way unsafe, right?
>>
>> This sounds to me like we need a better way of talking about flash device
>> names on the Linux command line rather than a way to fix the bus number -
>> for example, being able to refer to them using a fixed property like the
>> physical address.  Being able to refer to devices via an alias assigned
>> in the DT would also be useful (and more readable), I think there may
>> already be a mechanism for doing that which would need to be plumbed in
>> but I'm not 100% sure.
>
> The bus number is the variable designed to distinguish one spi controller
> from others. Why spi controller's physical address must be use instead of
> bus number?

Because the bus number is dynamic, while the physical address doesn't
change, so it can be used to uniqely identify the device before booting the
kernel.
Cfr. "spi1" vs. "e6e20000.spi".

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hou Zhiqiang March 18, 2014, 9:34 a.m. UTC | #6
DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogZ2VlcnQudXl0dGVyaG9l
dmVuQGdtYWlsLmNvbSBbbWFpbHRvOmdlZXJ0LnV5dHRlcmhvZXZlbkBnbWFpbC5jb21dDQo+IE9u
IEJlaGFsZiBPZiBHZWVydCBVeXR0ZXJob2V2ZW4NCj4gU2VudDogVHVlc2RheSwgTWFyY2ggMTgs
IDIwMTQgNDo1NiBQTQ0KPiBUbzogSG91IFpoaXFpYW5nLUI0ODI4Ng0KPiBDYzogTWFyayBCcm93
bjsgbGludXgtc3BpQHZnZXIua2VybmVsLm9yZzsgZGV2aWNldHJlZUB2Z2VyLmtlcm5lbC5vcmc7
DQo+IHJvYi5oZXJyaW5nQGNhbHhlZGEuY29tOyBwYXdlbC5tb2xsQGFybS5jb207IG1hcmsucnV0
bGFuZEBhcm0uY29tOw0KPiBpamMrZGV2aWNldHJlZUBoZWxsaW9uLm9yZy51azsgZ2FsYWtAY29k
ZWF1cm9yYS5vcmc7DQo+IGdyYW50Lmxpa2VseUBzZWNyZXRsYWIuY2E7IFdvb2QgU2NvdHQtQjA3
NDIxOyBIdSBNaW5na2FpLUIyMTI4NA0KPiBTdWJqZWN0OiBSZTogW1BBVENIIDIvMl0gc3BpL2Zz
bC1saWI6IEdldCB0aGUgU1BJIGNvbnRyb2xsZXIgYnVzIG51bWJlcg0KPiBmcm9tIERUUw0KPiAN
Cj4gT24gVHVlLCBNYXIgMTgsIDIwMTQgYXQgODo0MCBBTSwgQjQ4Mjg2QGZyZWVzY2FsZS5jb20N
Cj4gPEI0ODI4NkBmcmVlc2NhbGUuY29tPiB3cm90ZToNCj4gPj4gPiA+IFRoZSBEVCBhbHJlYWR5
IGhhcyBzdXBwb3J0IGZvciBzcGVjaWZ5aW5nIGZsYXNoIGxheW91dHMsIGNhbid0DQo+ID4+ID4g
PiB0aG9zZSBiZSB1c2VkIChmb3IgZXhhbXBsZSB2aWEgY2hvc2VuIGlmIHRoZXkncmUgbm90IGZp
eGVkIGZvciB0aGUNCj4gYm9hcmQpPw0KPiA+PiA+ID4gT3IgaWYgaXQncyBqdXN0IHBpY2tpbmcg
dGhlIGNvcnJlY3QgZmlsZXN5c3RlbSB0aGVuIFVVSURzIGFuZA0KPiA+PiA+ID4gbGFiZWxzIGFy
ZSB0aGUgc3RhbmRhcmQgd2F5IHRvIGRvIHRoaW5ncy4NCj4gPj4NCj4gPj4gPiBUaGUgRFQgc3Bl
Y2lmeWluZyBmbGFzaCBsYXlvdXRzIGlzIG9rLiBUaGVyZSBpcyBhbm90aGVyIHdheSB0byBtYWtl
DQo+ID4+ID4gdGhlIGZsYXNoIGxheW91dHMgdXNpbmcgY29tbWFuZCBsaW5lLCBidXQgaXQgaXMg
bm90IHNhZmUgYmVjYXVzZSBvZg0KPiA+PiA+IHRoZSBkeW5hbWljIGJ1c19udW0uIEl0IGlzIG5v
dCB0aGUgcmVhc29uIHRoYXQgdGhlIHdheSBvZiBEVCBpcw0KPiA+PiA+IHN1cHBvcnRlZCBmbGFz
aCBsYXlvdXRzLCB0byBsaXZlIHRoZSBvdGhlciB3YXkgdW5zYWZlLCByaWdodD8NCj4gPj4NCj4g
Pj4gVGhpcyBzb3VuZHMgdG8gbWUgbGlrZSB3ZSBuZWVkIGEgYmV0dGVyIHdheSBvZiB0YWxraW5n
IGFib3V0IGZsYXNoDQo+ID4+IGRldmljZSBuYW1lcyBvbiB0aGUgTGludXggY29tbWFuZCBsaW5l
IHJhdGhlciB0aGFuIGEgd2F5IHRvIGZpeCB0aGUNCj4gPj4gYnVzIG51bWJlciAtIGZvciBleGFt
cGxlLCBiZWluZyBhYmxlIHRvIHJlZmVyIHRvIHRoZW0gdXNpbmcgYSBmaXhlZA0KPiA+PiBwcm9w
ZXJ0eSBsaWtlIHRoZSBwaHlzaWNhbCBhZGRyZXNzLiAgQmVpbmcgYWJsZSB0byByZWZlciB0byBk
ZXZpY2VzDQo+ID4+IHZpYSBhbiBhbGlhcyBhc3NpZ25lZCBpbiB0aGUgRFQgd291bGQgYWxzbyBi
ZSB1c2VmdWwgKGFuZCBtb3JlDQo+ID4+IHJlYWRhYmxlKSwgSSB0aGluayB0aGVyZSBtYXkgYWxy
ZWFkeSBiZSBhIG1lY2hhbmlzbSBmb3IgZG9pbmcgdGhhdA0KPiA+PiB3aGljaCB3b3VsZCBuZWVk
IHRvIGJlIHBsdW1iZWQgaW4gYnV0IEknbSBub3QgMTAwJSBzdXJlLg0KPiA+DQo+ID4gVGhlIGJ1
cyBudW1iZXIgaXMgdGhlIHZhcmlhYmxlIGRlc2lnbmVkIHRvIGRpc3Rpbmd1aXNoIG9uZSBzcGkN
Cj4gPiBjb250cm9sbGVyIGZyb20gb3RoZXJzLiBXaHkgc3BpIGNvbnRyb2xsZXIncyBwaHlzaWNh
bCBhZGRyZXNzIG11c3QgYmUNCj4gPiB1c2UgaW5zdGVhZCBvZiBidXMgbnVtYmVyPw0KPiANCj4g
QmVjYXVzZSB0aGUgYnVzIG51bWJlciBpcyBkeW5hbWljLCB3aGlsZSB0aGUgcGh5c2ljYWwgYWRk
cmVzcyBkb2Vzbid0DQo+IGNoYW5nZSwgc28gaXQgY2FuIGJlIHVzZWQgdG8gdW5pcWVseSBpZGVu
dGlmeSB0aGUgZGV2aWNlIGJlZm9yZSBib290aW5nDQo+IHRoZSBrZXJuZWwuDQo+IENmci4gInNw
aTEiIHZzLiAiZTZlMjAwMDAuc3BpIi4NCj4gDQoNClRoZSBwcmVjb25kaXRpb24gb2YgZHluYW1p
YyBidXMgbnVtYmVyIGlzIGluaXRpYWwgaXQgd2l0aCAtMSBpbiB0aGUgY29udHJvbGxlcg0KZHJp
dmVyLiBCdXQgbm93IEkgbmVlZCBhIHJlYXNvbmFibGUgYnVzIG51bWJlciwgSSBkb24ndCB3YW50
IGEgZHluYW1pYyBvbmUuDQpXaHkgZG9lcyB1c2UgdGhlIGNvbnRyb2xsZXIncyBwaHlzaWNhbCBh
ZGRyZXNzIHRvIHRha2UgdGhlIHJvbGUgb2YgYnVzIG51bWJlcg0KdG8gZGlzdGluZ3Vpc2ggY29u
dHJvbGxlcnMuIA0KDQo+IEdye29ldGplLGVldGluZ31zLA0KPiANCj4gICAgICAgICAgICAgICAg
ICAgICAgICAgR2VlcnQNCj4gDQo+IC0tDQo+IEdlZXJ0IFV5dHRlcmhvZXZlbiAtLSBUaGVyZSdz
IGxvdHMgb2YgTGludXggYmV5b25kIGlhMzIgLS0gZ2VlcnRAbGludXgtDQo+IG02OGsub3JnDQo+
IA0KPiBJbiBwZXJzb25hbCBjb252ZXJzYXRpb25zIHdpdGggdGVjaG5pY2FsIHBlb3BsZSwgSSBj
YWxsIG15c2VsZiBhIGhhY2tlci4NCj4gQnV0IHdoZW4gSSdtIHRhbGtpbmcgdG8gam91cm5hbGlz
dHMgSSBqdXN0IHNheSAicHJvZ3JhbW1lciIgb3Igc29tZXRoaW5nDQo+IGxpa2UgdGhhdC4NCj4g
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAtLSBMaW51cyBUb3J2YWxkcw0KPiANCg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Wood March 18, 2014, 10:08 p.m. UTC | #7
On Tue, 2014-03-18 at 04:34 -0500, Hou Zhiqiang-B48286 wrote:
> 
> > -----Original Message-----
> > From: geert.uytterhoeven@gmail.com [mailto:geert.uytterhoeven@gmail.com]
> > On Behalf Of Geert Uytterhoeven
> > Sent: Tuesday, March 18, 2014 4:56 PM
> > To: Hou Zhiqiang-B48286
> > Cc: Mark Brown; linux-spi@vger.kernel.org; devicetree@vger.kernel.org;
> > rob.herring@calxeda.com; pawel.moll@arm.com; mark.rutland@arm.com;
> > ijc+devicetree@hellion.org.uk; galak@codeaurora.org;
> > grant.likely@secretlab.ca; Wood Scott-B07421; Hu Mingkai-B21284
> > Subject: Re: [PATCH 2/2] spi/fsl-lib: Get the SPI controller bus number
> > from DTS
> > 
> > On Tue, Mar 18, 2014 at 8:40 AM, B48286@freescale.com
> > <B48286@freescale.com> wrote:
> > >> > > The DT already has support for specifying flash layouts, can't
> > >> > > those be used (for example via chosen if they're not fixed for the
> > board)?
> > >> > > Or if it's just picking the correct filesystem then UUIDs and
> > >> > > labels are the standard way to do things.
> > >>
> > >> > The DT specifying flash layouts is ok. There is another way to make
> > >> > the flash layouts using command line, but it is not safe because of
> > >> > the dynamic bus_num. It is not the reason that the way of DT is
> > >> > supported flash layouts, to live the other way unsafe, right?
> > >>
> > >> This sounds to me like we need a better way of talking about flash
> > >> device names on the Linux command line rather than a way to fix the
> > >> bus number - for example, being able to refer to them using a fixed
> > >> property like the physical address.  Being able to refer to devices
> > >> via an alias assigned in the DT would also be useful (and more
> > >> readable), I think there may already be a mechanism for doing that
> > >> which would need to be plumbed in but I'm not 100% sure.
> > >
> > > The bus number is the variable designed to distinguish one spi
> > > controller from others. Why spi controller's physical address must be
> > > use instead of bus number?
> > 
> > Because the bus number is dynamic, while the physical address doesn't
> > change, so it can be used to uniqely identify the device before booting
> > the kernel.
> > Cfr. "spi1" vs. "e6e20000.spi".
> > 
> 
> The precondition of dynamic bus number is initial it with -1 in the controller
> driver. But now I need a reasonable bus number, I don't want a dynamic one.
> Why does use the controller's physical address to take the role of bus number
> to distinguish controllers. 

Where are you going to get this "reasonable" non-dynamic number from?
How are you going to ensure there are no conflicts with other SPI
controllers (e.g. on a dynamic add-on card)?

Physical addresses work well because they are tied to something real,
rather than an arbitrary enumeration.  Our NAND controllers use the
physical address for the MTD name.  Device tree NOR flash allows the
device tree to set the mtd name[1], and otherwise falls back on the
platform device name, which contains the physical address.

-Scott

[1] This violates the "device tree describes hardware rather than
configures Linux" rule...


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hou Zhiqiang March 19, 2014, 2:49 a.m. UTC | #8
> -----Original Message-----

> From: Wood Scott-B07421

> Sent: Wednesday, March 19, 2014 6:09 AM

> To: Hou Zhiqiang-B48286

> Cc: 'Geert Uytterhoeven'; Mark Brown; linux-spi@vger.kernel.org;

> devicetree@vger.kernel.org; rob.herring@calxeda.com; pawel.moll@arm.com;

> mark.rutland@arm.com; ijc+devicetree@hellion.org.uk; galak@codeaurora.org;

> grant.likely@secretlab.ca; Hu Mingkai-B21284

> Subject: Re: [PATCH 2/2] spi/fsl-lib: Get the SPI controller bus number

> from DTS

> 

> On Tue, 2014-03-18 at 04:34 -0500, Hou Zhiqiang-B48286 wrote:

> >

> > > -----Original Message-----

> > > From: geert.uytterhoeven@gmail.com

> > > [mailto:geert.uytterhoeven@gmail.com]

> > > On Behalf Of Geert Uytterhoeven

> > > Sent: Tuesday, March 18, 2014 4:56 PM

> > > To: Hou Zhiqiang-B48286

> > > Cc: Mark Brown; linux-spi@vger.kernel.org;

> > > devicetree@vger.kernel.org; rob.herring@calxeda.com;

> > > pawel.moll@arm.com; mark.rutland@arm.com;

> > > ijc+devicetree@hellion.org.uk; galak@codeaurora.org;

> > > grant.likely@secretlab.ca; Wood Scott-B07421; Hu Mingkai-B21284

> > > Subject: Re: [PATCH 2/2] spi/fsl-lib: Get the SPI controller bus

> > > number from DTS

> > >

> > > On Tue, Mar 18, 2014 at 8:40 AM, B48286@freescale.com

> > > <B48286@freescale.com> wrote:

> > > >> > > The DT already has support for specifying flash layouts,

> > > >> > > can't those be used (for example via chosen if they're not

> > > >> > > fixed for the

> > > board)?

> > > >> > > Or if it's just picking the correct filesystem then UUIDs and

> > > >> > > labels are the standard way to do things.

> > > >>

> > > >> > The DT specifying flash layouts is ok. There is another way to

> > > >> > make the flash layouts using command line, but it is not safe

> > > >> > because of the dynamic bus_num. It is not the reason that the

> > > >> > way of DT is supported flash layouts, to live the other way

> unsafe, right?

> > > >>

> > > >> This sounds to me like we need a better way of talking about

> > > >> flash device names on the Linux command line rather than a way to

> > > >> fix the bus number - for example, being able to refer to them

> > > >> using a fixed property like the physical address.  Being able to

> > > >> refer to devices via an alias assigned in the DT would also be

> > > >> useful (and more readable), I think there may already be a

> > > >> mechanism for doing that which would need to be plumbed in but I'm

> not 100% sure.

> > > >

> > > > The bus number is the variable designed to distinguish one spi

> > > > controller from others. Why spi controller's physical address must

> > > > be use instead of bus number?

> > >

> > > Because the bus number is dynamic, while the physical address

> > > doesn't change, so it can be used to uniqely identify the device

> > > before booting the kernel.

> > > Cfr. "spi1" vs. "e6e20000.spi".

> > >

> >

> > The precondition of dynamic bus number is initial it with -1 in the

> > controller driver. But now I need a reasonable bus number, I don't want

> a dynamic one.

> > Why does use the controller's physical address to take the role of bus

> > number to distinguish controllers.

> 

> Where are you going to get this "reasonable" non-dynamic number from?

> How are you going to ensure there are no conflicts with other SPI

> controllers (e.g. on a dynamic add-on card)?

> 

"other than negative (== assign one dynamically), bus_num is fully
board-specific.  usually that simplifies to being SOC-specific.
example:  one SOC has three SPI controllers, numbered 0..2,
and one board's schematics might show it using SPI-2.  software
would normally use bus_num=2 for that controller."
The above paragraph is description of bus_num in spi.h. The "reasonable"
is from it.
Other controllers should also include this property, otherwise it will be
dynamic. So there is not conflict.

> Physical addresses work well because they are tied to something real,

> rather than an arbitrary enumeration.  Our NAND controllers use the

> physical address for the MTD name.  Device tree NOR flash allows the

> device tree to set the mtd name[1], and otherwise falls back on the

> platform device name, which contains the physical address.

>

I know the physical work well, but there is a mechanism of bus number.
As the description say above, isn't it reasonable?
 
> -Scott

> 

> [1] This violates the "device tree describes hardware rather than

> configures Linux" rule...

>
Scott Wood March 19, 2014, 3:04 a.m. UTC | #9
On Tue, 2014-03-18 at 21:49 -0500, Hou Zhiqiang-B48286 wrote:
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Wednesday, March 19, 2014 6:09 AM
> > To: Hou Zhiqiang-B48286
> > Cc: 'Geert Uytterhoeven'; Mark Brown; linux-spi@vger.kernel.org;
> > devicetree@vger.kernel.org; rob.herring@calxeda.com; pawel.moll@arm.com;
> > mark.rutland@arm.com; ijc+devicetree@hellion.org.uk; galak@codeaurora.org;
> > grant.likely@secretlab.ca; Hu Mingkai-B21284
> > Subject: Re: [PATCH 2/2] spi/fsl-lib: Get the SPI controller bus number
> > from DTS
> > 
> > On Tue, 2014-03-18 at 04:34 -0500, Hou Zhiqiang-B48286 wrote:
> > >
> > > > -----Original Message-----
> > > > From: geert.uytterhoeven@gmail.com
> > > > [mailto:geert.uytterhoeven@gmail.com]
> > > > On Behalf Of Geert Uytterhoeven
> > > > Sent: Tuesday, March 18, 2014 4:56 PM
> > > > To: Hou Zhiqiang-B48286
> > > > Cc: Mark Brown; linux-spi@vger.kernel.org;
> > > > devicetree@vger.kernel.org; rob.herring@calxeda.com;
> > > > pawel.moll@arm.com; mark.rutland@arm.com;
> > > > ijc+devicetree@hellion.org.uk; galak@codeaurora.org;
> > > > grant.likely@secretlab.ca; Wood Scott-B07421; Hu Mingkai-B21284
> > > > Subject: Re: [PATCH 2/2] spi/fsl-lib: Get the SPI controller bus
> > > > number from DTS
> > > >
> > > > On Tue, Mar 18, 2014 at 8:40 AM, B48286@freescale.com
> > > > <B48286@freescale.com> wrote:
> > > > >> > > The DT already has support for specifying flash layouts,
> > > > >> > > can't those be used (for example via chosen if they're not
> > > > >> > > fixed for the
> > > > board)?
> > > > >> > > Or if it's just picking the correct filesystem then UUIDs and
> > > > >> > > labels are the standard way to do things.
> > > > >>
> > > > >> > The DT specifying flash layouts is ok. There is another way to
> > > > >> > make the flash layouts using command line, but it is not safe
> > > > >> > because of the dynamic bus_num. It is not the reason that the
> > > > >> > way of DT is supported flash layouts, to live the other way
> > unsafe, right?
> > > > >>
> > > > >> This sounds to me like we need a better way of talking about
> > > > >> flash device names on the Linux command line rather than a way to
> > > > >> fix the bus number - for example, being able to refer to them
> > > > >> using a fixed property like the physical address.  Being able to
> > > > >> refer to devices via an alias assigned in the DT would also be
> > > > >> useful (and more readable), I think there may already be a
> > > > >> mechanism for doing that which would need to be plumbed in but I'm
> > not 100% sure.
> > > > >
> > > > > The bus number is the variable designed to distinguish one spi
> > > > > controller from others. Why spi controller's physical address must
> > > > > be use instead of bus number?
> > > >
> > > > Because the bus number is dynamic, while the physical address
> > > > doesn't change, so it can be used to uniqely identify the device
> > > > before booting the kernel.
> > > > Cfr. "spi1" vs. "e6e20000.spi".
> > > >
> > >
> > > The precondition of dynamic bus number is initial it with -1 in the
> > > controller driver. But now I need a reasonable bus number, I don't want
> > a dynamic one.
> > > Why does use the controller's physical address to take the role of bus
> > > number to distinguish controllers.
> > 
> > Where are you going to get this "reasonable" non-dynamic number from?
> > How are you going to ensure there are no conflicts with other SPI
> > controllers (e.g. on a dynamic add-on card)?
> > 
> "other than negative (== assign one dynamically), bus_num is fully
> board-specific.  usually that simplifies to being SOC-specific.
> example:  one SOC has three SPI controllers, numbered 0..2,
> and one board's schematics might show it using SPI-2.  software
> would normally use bus_num=2 for that controller."
> The above paragraph is description of bus_num in spi.h. The "reasonable"
> is from it.
> Other controllers should also include this property, otherwise it will be
> dynamic. So there is not conflict.

So instead of using something concrete like a physical address, you want
to use a number from a manual.  Again, what happens in a system where
SPI controllers can be added dynamically (forget about whether this is
possible on the systems you care about), or even statically from a
different source (e.g. board logic)?  How does the driver know what
number the manual assigns to a given controller?  Why would you even
want to deal with this when using the physical address is so easy?

> > Physical addresses work well because they are tied to something real,
> > rather than an arbitrary enumeration.  Our NAND controllers use the
> > physical address for the MTD name.  Device tree NOR flash allows the
> > device tree to set the mtd name[1], and otherwise falls back on the
> > platform device name, which contains the physical address.
> >
> I know the physical work well, but there is a mechanism of bus number.
> As the description say above, isn't it reasonable?

No.

-Scott


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown March 19, 2014, 10:37 a.m. UTC | #10
On Tue, Mar 18, 2014 at 07:40:28AM +0000, B48286@freescale.com wrote:

> > You are adding a property to specify a bus number.  There is no reason
> > why such a property should be specific to a single controller, every
> > controller in every system has a bus number assigned to it so if we have
> > a way to specify one controller via the device tree we should have a way
> > to specify it for all.

> The reason to specify a bus number to a single controller is to avoid 
> allocating one dynamically, so we know the bus number before booting up the
> kernel. If there are several controllers, you should add this property to all
> of them. But it is unnecessary to add this property, if you do not care about
> the bus number, and it will be allocated dynamically. 

Please re-read what I wrote above, you're completely missing the point.
The problem above is that you are adding this in driver specific code,
even if this is a good idea there's nothing specific to this device
about it so we shouldn't be doing it in a driver.

Patch
diff mbox

diff --git a/drivers/spi/spi-fsl-lib.c b/drivers/spi/spi-fsl-lib.c
index 0b75f26..f5f0307b 100644
--- a/drivers/spi/spi-fsl-lib.c
+++ b/drivers/spi/spi-fsl-lib.c
@@ -198,7 +198,7 @@  int of_mpc8xxx_spi_probe(struct platform_device *ofdev)
 	struct mpc8xxx_spi_probe_info *pinfo;
 	struct fsl_spi_platform_data *pdata;
 	const void *prop;
-	int ret = -ENOMEM;
+	int ret = -ENOMEM, bus_num;
 
 	pinfo = kzalloc(sizeof(*pinfo), GFP_KERNEL);
 	if (!pinfo)
@@ -207,8 +207,12 @@  int of_mpc8xxx_spi_probe(struct platform_device *ofdev)
 	pdata = &pinfo->pdata;
 	dev->platform_data = pdata;
 
-	/* Allocate bus num dynamically. */
-	pdata->bus_num = -1;
+	ret = of_property_read_u32(np, "bus-num", &bus_num);
+	if (ret < 0) {
+		/* Allocate bus num dynamically. */
+		bus_num = -1;
+	}
+	pdata->bus_num = bus_num;
 
 #ifdef CONFIG_FSL_SOC
 	/* SPI controller is either clocked from QE or SoC clock. */