diff mbox

[v4,5/6] mmc: kconfig: select FSL_GUTS for MMC_SDHCI_OF_ESDHC

Message ID 1450067067-44869-6-git-send-email-yangbo.lu@freescale.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

yangbo lu Dec. 14, 2015, 4:24 a.m. UTC
The sdhci-of-esdhc driver needs the GUTS driver support
to access the global utilities registers for SVR value.
So we select FSL_GUTS for MMC_SDHCI_OF_ESDHC here.

Signed-off-by: Yangbo Lu <yangbo.lu@freescale.com>
---
Changes for v2:
	- None
Changes for v3:
	- None
Changes for v4:
	- Added this patch
---
 drivers/mmc/host/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

Comments

Ulf Hansson Dec. 14, 2015, 1:08 p.m. UTC | #1
On 14 December 2015 at 05:24, Yangbo Lu <yangbo.lu@freescale.com> wrote:
> The sdhci-of-esdhc driver needs the GUTS driver support
> to access the global utilities registers for SVR value.
> So we select FSL_GUTS for MMC_SDHCI_OF_ESDHC here.
>
> Signed-off-by: Yangbo Lu <yangbo.lu@freescale.com>
> ---
> Changes for v2:
>         - None
> Changes for v3:
>         - None
> Changes for v4:
>         - Added this patch
> ---
>  drivers/mmc/host/Kconfig | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 1526b8a..df57c14 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -142,6 +142,8 @@ config MMC_SDHCI_OF_ESDHC
>         depends on MMC_SDHCI_PLTFM
>         depends on PPC || ARCH_MXC || ARCH_LAYERSCAPE
>         select MMC_SDHCI_IO_ACCESSORS
> +       select SOC_FSL
> +       select FSL_GUTS

This is weird.

First, perhaps it would make sense to have stub functions for those
the FSL_GUTS driver provides via its API, thus the above wouldn't be
required at all. Of course this makes only sense if you think there
are/could be configurations for a cross SOC driver which don't need
the GUTS driver.

Second, even if you think the stubs above is a bad idea, I would from
the top-level Kconfig for your platform, add the needed "selects" as I
think it's where it belongs and then change this to "depends on"
instead.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Wood Dec. 14, 2015, 6:04 p.m. UTC | #2
On Mon, 2015-12-14 at 14:08 +0100, Ulf Hansson wrote:
> On 14 December 2015 at 05:24, Yangbo Lu <yangbo.lu@freescale.com> wrote:
> > The sdhci-of-esdhc driver needs the GUTS driver support
> > to access the global utilities registers for SVR value.
> > So we select FSL_GUTS for MMC_SDHCI_OF_ESDHC here.
> > 
> > Signed-off-by: Yangbo Lu <yangbo.lu@freescale.com>
> > ---
> > Changes for v2:
> >         - None
> > Changes for v3:
> >         - None
> > Changes for v4:
> >         - Added this patch
> > ---
> >  drivers/mmc/host/Kconfig | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> > index 1526b8a..df57c14 100644
> > --- a/drivers/mmc/host/Kconfig
> > +++ b/drivers/mmc/host/Kconfig
> > @@ -142,6 +142,8 @@ config MMC_SDHCI_OF_ESDHC
> >         depends on MMC_SDHCI_PLTFM
> >         depends on PPC || ARCH_MXC || ARCH_LAYERSCAPE
> >         select MMC_SDHCI_IO_ACCESSORS
> > +       select SOC_FSL
> > +       select FSL_GUTS
> 
> This is weird.
> 
> First, perhaps it would make sense to have stub functions for those
> the FSL_GUTS driver provides via its API, thus the above wouldn't be
> required at all. Of course this makes only sense if you think there
> are/could be configurations for a cross SOC driver which don't need
> the GUTS driver.
> 
> Second, even if you think the stubs above is a bad idea, I would from
> the top-level Kconfig for your platform, add the needed "selects" as I
> think it's where it belongs and then change this to "depends on"
> instead.

Why is it weird for a driver to select another driver that it makes calls to? 
 Much easier to do it here than in all the platforms that use this driver.

And I think stubs for reading SVR is quite a bad idea.  It'll make the driver
build but it will silently not be able to apply SVR-based workarounds.

-Scott

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Wood Dec. 14, 2015, 10:14 p.m. UTC | #3
On Mon, 2015-12-14 at 12:24 +0800, Yangbo Lu wrote:
> The sdhci-of-esdhc driver needs the GUTS driver support
> to access the global utilities registers for SVR value.
> So we select FSL_GUTS for MMC_SDHCI_OF_ESDHC here.
> 
> Signed-off-by: Yangbo Lu <yangbo.lu@freescale.com>
> ---
> Changes for v2:
> 	- None
> Changes for v3:
> 	- None
> Changes for v4:
> 	- Added this patch
> ---
>  drivers/mmc/host/Kconfig | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 1526b8a..df57c14 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -142,6 +142,8 @@ config MMC_SDHCI_OF_ESDHC
>  	depends on MMC_SDHCI_PLTFM
>  	depends on PPC || ARCH_MXC || ARCH_LAYERSCAPE
>  	select MMC_SDHCI_IO_ACCESSORS
> +	select SOC_FSL
> +	select FSL_GUTS
>  	help
>  	  This selects the Freescale eSDHC controller support.
>  

What is "SOC_FSL"?  If you mean "FSL_SOC", why are you selecting it?

-Scott

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson Dec. 15, 2015, 9:46 a.m. UTC | #4
[...]
>> > --- a/drivers/mmc/host/Kconfig
>> > +++ b/drivers/mmc/host/Kconfig
>> > @@ -142,6 +142,8 @@ config MMC_SDHCI_OF_ESDHC
>> >         depends on MMC_SDHCI_PLTFM
>> >         depends on PPC || ARCH_MXC || ARCH_LAYERSCAPE
>> >         select MMC_SDHCI_IO_ACCESSORS
>> > +       select SOC_FSL
>> > +       select FSL_GUTS
>>
>> This is weird.
>>
>> First, perhaps it would make sense to have stub functions for those
>> the FSL_GUTS driver provides via its API, thus the above wouldn't be
>> required at all. Of course this makes only sense if you think there
>> are/could be configurations for a cross SOC driver which don't need
>> the GUTS driver.
>>
>> Second, even if you think the stubs above is a bad idea, I would from
>> the top-level Kconfig for your platform, add the needed "selects" as I
>> think it's where it belongs and then change this to "depends on"
>> instead.
>
> Why is it weird for a driver to select another driver that it makes calls to?
>  Much easier to do it here than in all the platforms that use this driver.

Because using "select" will not consider the dependencies for the new
selected Kconfig option. I can imagine that it might become a problem,
sooner or later.

So, "select" shall be used by care and in this case I think we can
cope fine with using "depends on".

>
> And I think stubs for reading SVR is quite a bad idea.  It'll make the driver
> build but it will silently not be able to apply SVR-based workarounds.

It doesn't have to be "silent", the driver can return an error (and
print error messages) from its ->probe() method, if the calls to the
GUTS driver fails.

Anyway, I mentioned this idea only to understand the need for
*optional* GUTS supports. Perhaps there is a cross SOC drivers that
for some platforms depends on GUTS but on others it doesn't.

Maybe that isn't case then!?

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Wood Dec. 16, 2015, 10:48 p.m. UTC | #5
On Tue, 2015-12-15 at 10:46 +0100, Ulf Hansson wrote:
> [...]
> > > > --- a/drivers/mmc/host/Kconfig
> > > > +++ b/drivers/mmc/host/Kconfig
> > > > @@ -142,6 +142,8 @@ config MMC_SDHCI_OF_ESDHC
> > > >         depends on MMC_SDHCI_PLTFM
> > > >         depends on PPC || ARCH_MXC || ARCH_LAYERSCAPE
> > > >         select MMC_SDHCI_IO_ACCESSORS
> > > > +       select SOC_FSL
> > > > +       select FSL_GUTS
> > > 
> > > This is weird.
> > > 
> > > First, perhaps it would make sense to have stub functions for those
> > > the FSL_GUTS driver provides via its API, thus the above wouldn't be
> > > required at all. Of course this makes only sense if you think there
> > > are/could be configurations for a cross SOC driver which don't need
> > > the GUTS driver.
> > > 
> > > Second, even if you think the stubs above is a bad idea, I would from
> > > the top-level Kconfig for your platform, add the needed "selects" as I
> > > think it's where it belongs and then change this to "depends on"
> > > instead.
> > 
> > Why is it weird for a driver to select another driver that it makes calls
> > to?
> >  Much easier to do it here than in all the platforms that use this driver.
> 
> Because using "select" will not consider the dependencies for the new
> selected Kconfig option. I can imagine that it might become a problem,
> sooner or later.

It's not a problem as long as the selected option's dependencies (if any) are
selected, or depended on by the selecting driver.  I wouldn't expect the
FSL_GUTS driver to depend on anything other than basic OF support.

> So, "select" shall be used by care and in this case I think we can
> cope fine with using "depends on".

What does select exist for if not situations like this?  What "care" is
missing?

-Scott

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson Dec. 17, 2015, 11:25 a.m. UTC | #6
On 16 December 2015 at 23:48, Scott Wood <scottwood@freescale.com> wrote:
> On Tue, 2015-12-15 at 10:46 +0100, Ulf Hansson wrote:
>> [...]
>> > > > --- a/drivers/mmc/host/Kconfig
>> > > > +++ b/drivers/mmc/host/Kconfig
>> > > > @@ -142,6 +142,8 @@ config MMC_SDHCI_OF_ESDHC
>> > > >         depends on MMC_SDHCI_PLTFM
>> > > >         depends on PPC || ARCH_MXC || ARCH_LAYERSCAPE
>> > > >         select MMC_SDHCI_IO_ACCESSORS
>> > > > +       select SOC_FSL
>> > > > +       select FSL_GUTS
>> > >
>> > > This is weird.
>> > >
>> > > First, perhaps it would make sense to have stub functions for those
>> > > the FSL_GUTS driver provides via its API, thus the above wouldn't be
>> > > required at all. Of course this makes only sense if you think there
>> > > are/could be configurations for a cross SOC driver which don't need
>> > > the GUTS driver.
>> > >
>> > > Second, even if you think the stubs above is a bad idea, I would from
>> > > the top-level Kconfig for your platform, add the needed "selects" as I
>> > > think it's where it belongs and then change this to "depends on"
>> > > instead.
>> >
>> > Why is it weird for a driver to select another driver that it makes calls
>> > to?
>> >  Much easier to do it here than in all the platforms that use this driver.
>>
>> Because using "select" will not consider the dependencies for the new
>> selected Kconfig option. I can imagine that it might become a problem,
>> sooner or later.
>
> It's not a problem as long as the selected option's dependencies (if any) are
> selected, or depended on by the selecting driver.  I wouldn't expect the
> FSL_GUTS driver to depend on anything other than basic OF support.
>
>> So, "select" shall be used by care and in this case I think we can
>> cope fine with using "depends on".
>
> What does select exist for if not situations like this?  What "care" is
> missing?
>

The GUTS driver is SoC specific driver. If/when dependencies are added
for that driver it will break the usage of the select for the MMC host
driver. No, thanks it's fragile!

To me, the best way to use select is either from top-level platform
Kconfig or where it's better isolated, perhaps within a subsystem.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson Dec. 17, 2015, 11:30 a.m. UTC | #7
[...]

>>
>> And I think stubs for reading SVR is quite a bad idea.  It'll make the driver
>> build but it will silently not be able to apply SVR-based workarounds.
>
> It doesn't have to be "silent", the driver can return an error (and
> print error messages) from its ->probe() method, if the calls to the
> GUTS driver fails.
>
> Anyway, I mentioned this idea only to understand the need for
> *optional* GUTS supports. Perhaps there is a cross SOC drivers that
> for some platforms depends on GUTS but on others it doesn't.
>
> Maybe that isn't case then!?

Can you please answer this question!?

According to the earlier versions of this patchset and from your
comments [1], it *do* seems like the GUTS driver may be optional and
thus stubs could address this.

Kind regards
Uffe

[1]
http://www.spinics.net/lists/linux-mmc/msg34412.html
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yangbo Lu Dec. 28, 2015, 10:26 a.m. UTC | #8
PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBVbGYgSGFuc3NvbiBbbWFpbHRv
OnVsZi5oYW5zc29uQGxpbmFyby5vcmddDQo+IFNlbnQ6IFRodXJzZGF5LCBEZWNlbWJlciAxNywg
MjAxNSA3OjMxIFBNDQo+IFRvOiBTY290dCBXb29kDQo+IENjOiBMdSBZYW5nYm8tQjQ3MDkzOyBs
aW51eC1tbWM7IFhpZSBYaWFvYm8tUjYzMDYxOyBMZW8gbGkNCj4gU3ViamVjdDogUmU6IFt2NCwg
NS82XSBtbWM6IGtjb25maWc6IHNlbGVjdCBGU0xfR1VUUyBmb3INCj4gTU1DX1NESENJX09GX0VT
REhDDQo+IA0KPiBbLi4uXQ0KPiANCj4gPj4NCj4gPj4gQW5kIEkgdGhpbmsgc3R1YnMgZm9yIHJl
YWRpbmcgU1ZSIGlzIHF1aXRlIGEgYmFkIGlkZWEuICBJdCdsbCBtYWtlDQo+ID4+IHRoZSBkcml2
ZXIgYnVpbGQgYnV0IGl0IHdpbGwgc2lsZW50bHkgbm90IGJlIGFibGUgdG8gYXBwbHkgU1ZSLWJh
c2VkDQo+IHdvcmthcm91bmRzLg0KPiA+DQo+ID4gSXQgZG9lc24ndCBoYXZlIHRvIGJlICJzaWxl
bnQiLCB0aGUgZHJpdmVyIGNhbiByZXR1cm4gYW4gZXJyb3IgKGFuZA0KPiA+IHByaW50IGVycm9y
IG1lc3NhZ2VzKSBmcm9tIGl0cyAtPnByb2JlKCkgbWV0aG9kLCBpZiB0aGUgY2FsbHMgdG8gdGhl
DQo+ID4gR1VUUyBkcml2ZXIgZmFpbHMuDQo+ID4NCj4gPiBBbnl3YXksIEkgbWVudGlvbmVkIHRo
aXMgaWRlYSBvbmx5IHRvIHVuZGVyc3RhbmQgdGhlIG5lZWQgZm9yDQo+ID4gKm9wdGlvbmFsKiBH
VVRTIHN1cHBvcnRzLiBQZXJoYXBzIHRoZXJlIGlzIGEgY3Jvc3MgU09DIGRyaXZlcnMgdGhhdA0K
PiA+IGZvciBzb21lIHBsYXRmb3JtcyBkZXBlbmRzIG9uIEdVVFMgYnV0IG9uIG90aGVycyBpdCBk
b2Vzbid0Lg0KPiA+DQo+ID4gTWF5YmUgdGhhdCBpc24ndCBjYXNlIHRoZW4hPw0KPiANCj4gQ2Fu
IHlvdSBwbGVhc2UgYW5zd2VyIHRoaXMgcXVlc3Rpb24hPw0KPiANCj4gQWNjb3JkaW5nIHRvIHRo
ZSBlYXJsaWVyIHZlcnNpb25zIG9mIHRoaXMgcGF0Y2hzZXQgYW5kIGZyb20geW91ciBjb21tZW50
cw0KPiBbMV0sIGl0ICpkbyogc2VlbXMgbGlrZSB0aGUgR1VUUyBkcml2ZXIgbWF5IGJlIG9wdGlv
bmFsIGFuZCB0aHVzIHN0dWJzDQo+IGNvdWxkIGFkZHJlc3MgdGhpcy4NCj4gDQo+IEtpbmQgcmVn
YXJkcw0KPiBVZmZlDQo+IA0KPiBbMV0NCj4gaHR0cDovL3d3dy5zcGluaWNzLm5ldC9saXN0cy9s
aW51eC1tbWMvbXNnMzQ0MTIuaHRtbA0KDQpbTHUgWWFuZ2JvLUI0NzA5M10gSGkgU2NvdHQgYW5k
IFVmZmUsIA0KSW4gdGhlIGVhcmxpZXIgdmVyc2lvbiwgSSdkIGxpa2UgdG8gdXNlIHN5c2NvbiBz
dXBwb3J0IGFuZCBvbmx5IGFkZCAnc3lzY29uJyBjb21wYXRpYmxlIGluIHRoZSBkdHMgd2hvc2Ug
ZVNESEMgbmVlZHMgdG8gdXNlIGl0IHRvIGdldCBTVlIuDQpCdXQgSSBuZXZlciB0aG91Z2h0IHRo
aXMgaGFkIGNhdXNlZCBzbyBtdWNoIGRpc2N1c3Npb24uLi4gOigNCkNvdWxkIHdlIHJlYWNoIGFu
IGFncmVlbWVudCBhYm91dCB0aGUgJ29wdGlvbmFsJyBvciBub3QgJ29wdGlvbmFsJz8NCg0KVGhh
bmsgeW91IHNvIG11Y2ghDQoNCiANCg0KDQoNCg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson Dec. 28, 2015, 12:10 p.m. UTC | #9
On 28 December 2015 at 11:26, Yangbo Lu <yangbo.lu@nxp.com> wrote:
>> -----Original Message-----
>> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
>> Sent: Thursday, December 17, 2015 7:31 PM
>> To: Scott Wood
>> Cc: Lu Yangbo-B47093; linux-mmc; Xie Xiaobo-R63061; Leo li
>> Subject: Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for
>> MMC_SDHCI_OF_ESDHC
>>
>> [...]
>>
>> >>
>> >> And I think stubs for reading SVR is quite a bad idea.  It'll make
>> >> the driver build but it will silently not be able to apply SVR-based
>> workarounds.
>> >
>> > It doesn't have to be "silent", the driver can return an error (and
>> > print error messages) from its ->probe() method, if the calls to the
>> > GUTS driver fails.
>> >
>> > Anyway, I mentioned this idea only to understand the need for
>> > *optional* GUTS supports. Perhaps there is a cross SOC drivers that
>> > for some platforms depends on GUTS but on others it doesn't.
>> >
>> > Maybe that isn't case then!?
>>
>> Can you please answer this question!?
>>
>> According to the earlier versions of this patchset and from your comments
>> [1], it *do* seems like the GUTS driver may be optional and thus stubs
>> could address this.
>>
>> Kind regards
>> Uffe
>>
>> [1]
>> http://www.spinics.net/lists/linux-mmc/msg34412.html
>
> [Lu Yangbo-B47093] Hi Scott and Uffe,
> In the earlier version, I'd like to use syscon support and only add 'syscon' compatible in the dts whose eSDHC needs to use it to get SVR.
> But I never thought this had caused so much discussion... :(

Sorry, I understand your frustration but that's life sometimes. :-)

To me, the syscon solution is more elegant...

> Could we reach an agreement about the 'optional' or not 'optional'?

...but I am fine with the current approach as well, as long as my
recent comments gets addressed. Let's make it optional.

Address Scott's and my review-comments, get other peoples ack for the
non-mmc parts, then I will happily pick up the patches.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Wood Dec. 28, 2015, 6:47 p.m. UTC | #10
On Thu, 2015-12-17 at 12:30 +0100, Ulf Hansson wrote:
> [...]
> 
> > > 
> > > And I think stubs for reading SVR is quite a bad idea.  It'll make the
> > > driver
> > > build but it will silently not be able to apply SVR-based workarounds.
> > 
> > It doesn't have to be "silent", the driver can return an error (and
> > print error messages) from its ->probe() method, if the calls to the
> > GUTS driver fails.
> > 
> > Anyway, I mentioned this idea only to understand the need for
> > *optional* GUTS supports. Perhaps there is a cross SOC drivers that
> > for some platforms depends on GUTS but on others it doesn't.
> > 
> > Maybe that isn't case then!?
> 
> Can you please answer this question!?
> 
> According to the earlier versions of this patchset and from your
> comments [1], it *do* seems like the GUTS driver may be optional and
> thus stubs could address this.

I'd rather it not be optional at build time for the reason I explained above. 
 It would be too easy for users to accidentally not enable the guts driver and
miss the workaround.  Even if an error is printed it's easy to miss among all
the boot spam -- and if there's any legitimate reason to not have the driver
enabled then why would that be an "error"?  If the guts driver fails to bind
(e.g. running in a VM, or a platform the guts driver doesn't recognize) that's
another matter, but that should be handled by an error check in the guts
driver, not a build-time stub.

-Scott


--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Wood Dec. 28, 2015, 7:03 p.m. UTC | #11
On Thu, 2015-12-17 at 12:25 +0100, Ulf Hansson wrote:
> On 16 December 2015 at 23:48, Scott Wood <scottwood@freescale.com> wrote:
> > On Tue, 2015-12-15 at 10:46 +0100, Ulf Hansson wrote:
> > > [...]
> > > > > > --- a/drivers/mmc/host/Kconfig
> > > > > > +++ b/drivers/mmc/host/Kconfig
> > > > > > @@ -142,6 +142,8 @@ config MMC_SDHCI_OF_ESDHC
> > > > > >         depends on MMC_SDHCI_PLTFM
> > > > > >         depends on PPC || ARCH_MXC || ARCH_LAYERSCAPE
> > > > > >         select MMC_SDHCI_IO_ACCESSORS
> > > > > > +       select SOC_FSL
> > > > > > +       select FSL_GUTS
> > > > > 
> > > > > This is weird.
> > > > > 
> > > > > First, perhaps it would make sense to have stub functions for those
> > > > > the FSL_GUTS driver provides via its API, thus the above wouldn't be
> > > > > required at all. Of course this makes only sense if you think there
> > > > > are/could be configurations for a cross SOC driver which don't need
> > > > > the GUTS driver.
> > > > > 
> > > > > Second, even if you think the stubs above is a bad idea, I would
> > > > > from
> > > > > the top-level Kconfig for your platform, add the needed "selects" as
> > > > > I
> > > > > think it's where it belongs and then change this to "depends on"
> > > > > instead.
> > > > 
> > > > Why is it weird for a driver to select another driver that it makes
> > > > calls
> > > > to?
> > > >  Much easier to do it here than in all the platforms that use this
> > > > driver.
> > > 
> > > Because using "select" will not consider the dependencies for the new
> > > selected Kconfig option. I can imagine that it might become a problem,
> > > sooner or later.
> > 
> > It's not a problem as long as the selected option's dependencies (if any)
> > are
> > selected, or depended on by the selecting driver.  I wouldn't expect the
> > FSL_GUTS driver to depend on anything other than basic OF support.
> > 
> > > So, "select" shall be used by care and in this case I think we can
> > > cope fine with using "depends on".
> > 
> > What does select exist for if not situations like this?  What "care" is
> > missing?
> > 
> 
> The GUTS driver is SoC specific driver. If/when dependencies are added
> for that driver it will break the usage of the select for the MMC host
> driver. No, thanks it's fragile!
> 
> To me, the best way to use select is either from top-level platform
> Kconfig or where it's better isolated, perhaps within a subsystem.

The guts driver is not going to depend on anything that the esdhc driver
shouldn't already be depending on (why does MMC_SDHCI_OF_ESDHC not currently
depend on OF?) and if the guts driver ever did grow a dependency that isn't
met but it is selected anyway, kconfig will warn.  I realize that growing a
guts dependency list in every user would be bad but I just don't see such
dependencies happening.

-Scott

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Wood Dec. 28, 2015, 7:10 p.m. UTC | #12
On Mon, 2015-12-28 at 13:10 +0100, Ulf Hansson wrote:
> On 28 December 2015 at 11:26, Yangbo Lu <yangbo.lu@nxp.com> wrote:
> > > -----Original Message-----
> > > From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> > > Sent: Thursday, December 17, 2015 7:31 PM
> > > To: Scott Wood
> > > Cc: Lu Yangbo-B47093; linux-mmc; Xie Xiaobo-R63061; Leo li
> > > Subject: Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for
> > > MMC_SDHCI_OF_ESDHC
> > > 
> > > [...]
> > > 
> > > > > 
> > > > > And I think stubs for reading SVR is quite a bad idea.  It'll make
> > > > > the driver build but it will silently not be able to apply SVR-based
> > > workarounds.
> > > > 
> > > > It doesn't have to be "silent", the driver can return an error (and
> > > > print error messages) from its ->probe() method, if the calls to the
> > > > GUTS driver fails.
> > > > 
> > > > Anyway, I mentioned this idea only to understand the need for
> > > > *optional* GUTS supports. Perhaps there is a cross SOC drivers that
> > > > for some platforms depends on GUTS but on others it doesn't.
> > > > 
> > > > Maybe that isn't case then!?
> > > 
> > > Can you please answer this question!?
> > > 
> > > According to the earlier versions of this patchset and from your
> > > comments
> > > [1], it *do* seems like the GUTS driver may be optional and thus stubs
> > > could address this.
> > > 
> > > Kind regards
> > > Uffe
> > > 
> > > [1]
> > > http://www.spinics.net/lists/linux-mmc/msg34412.html
> > 
> > [Lu Yangbo-B47093] Hi Scott and Uffe,
> > In the earlier version, I'd like to use syscon support and only add
> > 'syscon' compatible in the dts whose eSDHC needs to use it to get SVR.
> > But I never thought this had caused so much discussion... :(
> 
> Sorry, I understand your frustration but that's life sometimes. :-)
> 
> To me, the syscon solution is more elegant...

The syscon patch was terrible.  It would have accessed a certain location in
any node labelled "syscon" whether it was guts or not, in addition to the
other complaints.

-Scott

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yangbo Lu Jan. 6, 2016, 6:58 a.m. UTC | #13
> -----Original Message-----

> From: Scott Wood [mailto:scottwood@freescale.com]

> Sent: Tuesday, December 29, 2015 3:10 AM

> To: Ulf Hansson; Yangbo Lu

> Cc: Lu Yangbo-B47093; linux-mmc; Xie Xiaobo-R63061; Leo li

> Subject: Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for

> MMC_SDHCI_OF_ESDHC

> 

> On Mon, 2015-12-28 at 13:10 +0100, Ulf Hansson wrote:

> > On 28 December 2015 at 11:26, Yangbo Lu <yangbo.lu@nxp.com> wrote:

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

> > > > From: Ulf Hansson [mailto:ulf.hansson@linaro.org]

> > > > Sent: Thursday, December 17, 2015 7:31 PM

> > > > To: Scott Wood

> > > > Cc: Lu Yangbo-B47093; linux-mmc; Xie Xiaobo-R63061; Leo li

> > > > Subject: Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for

> > > > MMC_SDHCI_OF_ESDHC

> > > >

> > > > [...]

> > > >

> > > > > >

> > > > > > And I think stubs for reading SVR is quite a bad idea.  It'll

> > > > > > make the driver build but it will silently not be able to

> > > > > > apply SVR-based

> > > > workarounds.

> > > > >

> > > > > It doesn't have to be "silent", the driver can return an error

> > > > > (and print error messages) from its ->probe() method, if the

> > > > > calls to the GUTS driver fails.

> > > > >

> > > > > Anyway, I mentioned this idea only to understand the need for

> > > > > *optional* GUTS supports. Perhaps there is a cross SOC drivers

> > > > > that for some platforms depends on GUTS but on others it doesn't.

> > > > >

> > > > > Maybe that isn't case then!?

> > > >

> > > > Can you please answer this question!?

> > > >

> > > > According to the earlier versions of this patchset and from your

> > > > comments [1], it *do* seems like the GUTS driver may be optional

> > > > and thus stubs could address this.

> > > >

> > > > Kind regards

> > > > Uffe

> > > >

> > > > [1]

> > > > http://www.spinics.net/lists/linux-mmc/msg34412.html

> > >

> > > [Lu Yangbo-B47093] Hi Scott and Uffe, In the earlier version, I'd

> > > like to use syscon support and only add 'syscon' compatible in the

> > > dts whose eSDHC needs to use it to get SVR.

> > > But I never thought this had caused so much discussion... :(

> >

> > Sorry, I understand your frustration but that's life sometimes. :-)

> >

> > To me, the syscon solution is more elegant...

> 

> The syscon patch was terrible.  It would have accessed a certain location

> in any node labelled "syscon" whether it was guts or not, in addition to

> the other complaints.

> 

> -Scott


[Lu Yangbo-B47093] As my understand, the syscon APIs would just check whether there is a 'syscon' compatible.
If no, the APIs return. We still could maintain a list of compatibles for guts if using syscon.
In my opinion, syscon and guts driver are just two method to get SVR.

I agree with Uffe, because I think syscon is really designed for this situation and many arm platforms are using it.
Of course, I still would like to try guts driver if you insist on it.
Yangbo Lu Jan. 6, 2016, 7:18 a.m. UTC | #14
> -----Original Message-----

> From: Scott Wood [mailto:scottwood@freescale.com]

> Sent: Tuesday, December 29, 2015 2:48 AM

> To: Ulf Hansson

> Cc: Yangbo Lu; linux-mmc; X.Xie@freescale.com; Leo li

> Subject: Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for

> MMC_SDHCI_OF_ESDHC

> 

> On Thu, 2015-12-17 at 12:30 +0100, Ulf Hansson wrote:

> > [...]

> >

> > > >

> > > > And I think stubs for reading SVR is quite a bad idea.  It'll make

> > > > the driver build but it will silently not be able to apply

> > > > SVR-based workarounds.

> > >

> > > It doesn't have to be "silent", the driver can return an error (and

> > > print error messages) from its ->probe() method, if the calls to the

> > > GUTS driver fails.

> > >

> > > Anyway, I mentioned this idea only to understand the need for

> > > *optional* GUTS supports. Perhaps there is a cross SOC drivers that

> > > for some platforms depends on GUTS but on others it doesn't.

> > >

> > > Maybe that isn't case then!?

> >

> > Can you please answer this question!?

> >

> > According to the earlier versions of this patchset and from your

> > comments [1], it *do* seems like the GUTS driver may be optional and

> > thus stubs could address this.

> 

> I'd rather it not be optional at build time for the reason I explained

> above.

>  It would be too easy for users to accidentally not enable the guts

> driver and miss the workaround.  Even if an error is printed it's easy to

> miss among all the boot spam -- and if there's any legitimate reason to

> not have the driver enabled then why would that be an "error"?  If the

> guts driver fails to bind (e.g. running in a VM, or a platform the guts

> driver doesn't recognize) that's another matter, but that should be

> handled by an error check in the guts driver, not a build-time stub.

> 

> -Scott

> 


[Lu Yangbo-B47093] For the 'optional or not optional' problem, I think Scott's opinion is reasonable.
If we use guts driver, 'select' could avoid missing the workaround.
If we use syscon, select is better than 'depend on' since 'depend on' needs user self to find out the dependency.

Anyway, we should continue the discussion to come to an agreement finally.
I would very appreciate!

Thanks.
Yangbo Lu Jan. 6, 2016, 7:23 a.m. UTC | #15
PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBVbGYgSGFuc3NvbiBbbWFpbHRv
OnVsZi5oYW5zc29uQGxpbmFyby5vcmddDQo+IFNlbnQ6IE1vbmRheSwgRGVjZW1iZXIgMjgsIDIw
MTUgODoxMCBQTQ0KPiBUbzogWWFuZ2JvIEx1DQo+IENjOiBTY290dCBXb29kOyBMdSBZYW5nYm8t
QjQ3MDkzOyBsaW51eC1tbWM7IFhpZSBYaWFvYm8tUjYzMDYxOyBMZW8gbGkNCj4gU3ViamVjdDog
UmU6IFt2NCwgNS82XSBtbWM6IGtjb25maWc6IHNlbGVjdCBGU0xfR1VUUyBmb3INCj4gTU1DX1NE
SENJX09GX0VTREhDDQo+IA0KPiBPbiAyOCBEZWNlbWJlciAyMDE1IGF0IDExOjI2LCBZYW5nYm8g
THUgPHlhbmdiby5sdUBueHAuY29tPiB3cm90ZToNCj4gPj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdl
LS0tLS0NCj4gPj4gRnJvbTogVWxmIEhhbnNzb24gW21haWx0bzp1bGYuaGFuc3NvbkBsaW5hcm8u
b3JnXQ0KPiA+PiBTZW50OiBUaHVyc2RheSwgRGVjZW1iZXIgMTcsIDIwMTUgNzozMSBQTQ0KPiA+
PiBUbzogU2NvdHQgV29vZA0KPiA+PiBDYzogTHUgWWFuZ2JvLUI0NzA5MzsgbGludXgtbW1jOyBY
aWUgWGlhb2JvLVI2MzA2MTsgTGVvIGxpDQo+ID4+IFN1YmplY3Q6IFJlOiBbdjQsIDUvNl0gbW1j
OiBrY29uZmlnOiBzZWxlY3QgRlNMX0dVVFMgZm9yDQo+ID4+IE1NQ19TREhDSV9PRl9FU0RIQw0K
PiA+Pg0KPiA+PiBbLi4uXQ0KPiA+Pg0KPiA+PiA+Pg0KPiA+PiA+PiBBbmQgSSB0aGluayBzdHVi
cyBmb3IgcmVhZGluZyBTVlIgaXMgcXVpdGUgYSBiYWQgaWRlYS4gIEl0J2xsIG1ha2UNCj4gPj4g
Pj4gdGhlIGRyaXZlciBidWlsZCBidXQgaXQgd2lsbCBzaWxlbnRseSBub3QgYmUgYWJsZSB0byBh
cHBseQ0KPiA+PiA+PiBTVlItYmFzZWQNCj4gPj4gd29ya2Fyb3VuZHMuDQo+ID4+ID4NCj4gPj4g
PiBJdCBkb2Vzbid0IGhhdmUgdG8gYmUgInNpbGVudCIsIHRoZSBkcml2ZXIgY2FuIHJldHVybiBh
biBlcnJvciAoYW5kDQo+ID4+ID4gcHJpbnQgZXJyb3IgbWVzc2FnZXMpIGZyb20gaXRzIC0+cHJv
YmUoKSBtZXRob2QsIGlmIHRoZSBjYWxscyB0bw0KPiA+PiA+IHRoZSBHVVRTIGRyaXZlciBmYWls
cy4NCj4gPj4gPg0KPiA+PiA+IEFueXdheSwgSSBtZW50aW9uZWQgdGhpcyBpZGVhIG9ubHkgdG8g
dW5kZXJzdGFuZCB0aGUgbmVlZCBmb3INCj4gPj4gPiAqb3B0aW9uYWwqIEdVVFMgc3VwcG9ydHMu
IFBlcmhhcHMgdGhlcmUgaXMgYSBjcm9zcyBTT0MgZHJpdmVycyB0aGF0DQo+ID4+ID4gZm9yIHNv
bWUgcGxhdGZvcm1zIGRlcGVuZHMgb24gR1VUUyBidXQgb24gb3RoZXJzIGl0IGRvZXNuJ3QuDQo+
ID4+ID4NCj4gPj4gPiBNYXliZSB0aGF0IGlzbid0IGNhc2UgdGhlbiE/DQo+ID4+DQo+ID4+IENh
biB5b3UgcGxlYXNlIGFuc3dlciB0aGlzIHF1ZXN0aW9uIT8NCj4gPj4NCj4gPj4gQWNjb3JkaW5n
IHRvIHRoZSBlYXJsaWVyIHZlcnNpb25zIG9mIHRoaXMgcGF0Y2hzZXQgYW5kIGZyb20geW91cg0K
PiA+PiBjb21tZW50cyBbMV0sIGl0ICpkbyogc2VlbXMgbGlrZSB0aGUgR1VUUyBkcml2ZXIgbWF5
IGJlIG9wdGlvbmFsIGFuZA0KPiA+PiB0aHVzIHN0dWJzIGNvdWxkIGFkZHJlc3MgdGhpcy4NCj4g
Pj4NCj4gPj4gS2luZCByZWdhcmRzDQo+ID4+IFVmZmUNCj4gPj4NCj4gPj4gWzFdDQo+ID4+IGh0
dHA6Ly93d3cuc3Bpbmljcy5uZXQvbGlzdHMvbGludXgtbW1jL21zZzM0NDEyLmh0bWwNCj4gPg0K
PiA+IFtMdSBZYW5nYm8tQjQ3MDkzXSBIaSBTY290dCBhbmQgVWZmZSwNCj4gPiBJbiB0aGUgZWFy
bGllciB2ZXJzaW9uLCBJJ2QgbGlrZSB0byB1c2Ugc3lzY29uIHN1cHBvcnQgYW5kIG9ubHkgYWRk
DQo+ICdzeXNjb24nIGNvbXBhdGlibGUgaW4gdGhlIGR0cyB3aG9zZSBlU0RIQyBuZWVkcyB0byB1
c2UgaXQgdG8gZ2V0IFNWUi4NCj4gPiBCdXQgSSBuZXZlciB0aG91Z2h0IHRoaXMgaGFkIGNhdXNl
ZCBzbyBtdWNoIGRpc2N1c3Npb24uLi4gOigNCj4gDQo+IFNvcnJ5LCBJIHVuZGVyc3RhbmQgeW91
ciBmcnVzdHJhdGlvbiBidXQgdGhhdCdzIGxpZmUgc29tZXRpbWVzLiA6LSkNCj4gDQoNCltMdSBZ
YW5nYm8tQjQ3MDkzXSBUaGFuayB5b3Ugc28gbXVjaCBmb3IgeW91ciB1bmRlcnN0YW5kaW5nIDop
DQoNCj4gVG8gbWUsIHRoZSBzeXNjb24gc29sdXRpb24gaXMgbW9yZSBlbGVnYW50Li4uDQo+IA0K
PiA+IENvdWxkIHdlIHJlYWNoIGFuIGFncmVlbWVudCBhYm91dCB0aGUgJ29wdGlvbmFsJyBvciBu
b3QgJ29wdGlvbmFsJz8NCj4gDQo+IC4uLmJ1dCBJIGFtIGZpbmUgd2l0aCB0aGUgY3VycmVudCBh
cHByb2FjaCBhcyB3ZWxsLCBhcyBsb25nIGFzIG15IHJlY2VudA0KPiBjb21tZW50cyBnZXRzIGFk
ZHJlc3NlZC4gTGV0J3MgbWFrZSBpdCBvcHRpb25hbC4NCj4gDQo+IEFkZHJlc3MgU2NvdHQncyBh
bmQgbXkgcmV2aWV3LWNvbW1lbnRzLCBnZXQgb3RoZXIgcGVvcGxlcyBhY2sgZm9yIHRoZQ0KPiBu
b24tbW1jIHBhcnRzLCB0aGVuIEkgd2lsbCBoYXBwaWx5IHBpY2sgdXAgdGhlIHBhdGNoZXMuDQo+
IA0KDQpbTHUgWWFuZ2JvLUI0NzA5M10gVGhhbmtzIGEgbG90LiBJIHdvdWxkIGFkZCBvdGhlciBy
ZXZpZXdlcnMgaW4gbmV4dCB2ZXJzaW9uLg0KIA0KPiBLaW5kIHJlZ2FyZHMNCj4gVWZmZQ0K
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yangbo Lu Jan. 6, 2016, 7:34 a.m. UTC | #16
> -----Original Message-----

> From: Yangbo Lu

> Sent: Wednesday, January 06, 2016 2:58 PM

> To: Scott Wood; Ulf Hansson

> Cc: Lu Yangbo-B47093; linux-mmc; Xie Xiaobo-R63061; Leo li

> Subject: RE: [v4, 5/6] mmc: kconfig: select FSL_GUTS for

> MMC_SDHCI_OF_ESDHC

> 

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

> > From: Scott Wood [mailto:scottwood@freescale.com]

> > Sent: Tuesday, December 29, 2015 3:10 AM

> > To: Ulf Hansson; Yangbo Lu

> > Cc: Lu Yangbo-B47093; linux-mmc; Xie Xiaobo-R63061; Leo li

> > Subject: Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for

> > MMC_SDHCI_OF_ESDHC

> >

> > On Mon, 2015-12-28 at 13:10 +0100, Ulf Hansson wrote:

> > > On 28 December 2015 at 11:26, Yangbo Lu <yangbo.lu@nxp.com> wrote:

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

> > > > > From: Ulf Hansson [mailto:ulf.hansson@linaro.org]

> > > > > Sent: Thursday, December 17, 2015 7:31 PM

> > > > > To: Scott Wood

> > > > > Cc: Lu Yangbo-B47093; linux-mmc; Xie Xiaobo-R63061; Leo li

> > > > > Subject: Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for

> > > > > MMC_SDHCI_OF_ESDHC

> > > > >

> > > > > [...]

> > > > >

> > > > > > >

> > > > > > > And I think stubs for reading SVR is quite a bad idea.

> > > > > > > It'll make the driver build but it will silently not be able

> > > > > > > to apply SVR-based

> > > > > workarounds.

> > > > > >

> > > > > > It doesn't have to be "silent", the driver can return an error

> > > > > > (and print error messages) from its ->probe() method, if the

> > > > > > calls to the GUTS driver fails.

> > > > > >

> > > > > > Anyway, I mentioned this idea only to understand the need for

> > > > > > *optional* GUTS supports. Perhaps there is a cross SOC drivers

> > > > > > that for some platforms depends on GUTS but on others it

> doesn't.

> > > > > >

> > > > > > Maybe that isn't case then!?

> > > > >

> > > > > Can you please answer this question!?

> > > > >

> > > > > According to the earlier versions of this patchset and from your

> > > > > comments [1], it *do* seems like the GUTS driver may be optional

> > > > > and thus stubs could address this.

> > > > >

> > > > > Kind regards

> > > > > Uffe

> > > > >

> > > > > [1]

> > > > > http://www.spinics.net/lists/linux-mmc/msg34412.html

> > > >

> > > > [Lu Yangbo-B47093] Hi Scott and Uffe, In the earlier version, I'd

> > > > like to use syscon support and only add 'syscon' compatible in the

> > > > dts whose eSDHC needs to use it to get SVR.

> > > > But I never thought this had caused so much discussion... :(

> > >

> > > Sorry, I understand your frustration but that's life sometimes. :-)

> > >

> > > To me, the syscon solution is more elegant...

> >

> > The syscon patch was terrible.  It would have accessed a certain

> > location in any node labelled "syscon" whether it was guts or not, in

> > addition to the other complaints.

> >

> > -Scott

> 

> [Lu Yangbo-B47093] As my understand, the syscon APIs would just check

> whether there is a 'syscon' compatible.

> If no, the APIs return. We still could maintain a list of compatibles for

> guts if using syscon.


[Lu Yangbo-B47093] It's ok to use a compatible name that is not 'syscon' as the parameter of the APIs.
The node needs just to contain 'syscon'. 

> In my opinion, syscon and guts driver are just two method to get SVR.

> 

> I agree with Uffe, because I think syscon is really designed for this

> situation and many arm platforms are using it.

> Of course, I still would like to try guts driver if you insist on it.

> 

> 

> 

> 

>
Yangbo Lu Jan. 8, 2016, 6:24 a.m. UTC | #17
PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBZYW5nYm8gTHUNCj4gU2VudDog
V2VkbmVzZGF5LCBKYW51YXJ5IDA2LCAyMDE2IDM6MTggUE0NCj4gVG86IFNjb3R0IFdvb2Q7IFVs
ZiBIYW5zc29uDQo+IENjOiBZYW5nYm8gTHU7IGxpbnV4LW1tYzsgWC5YaWVAZnJlZXNjYWxlLmNv
bTsgTGVvIGxpDQo+IFN1YmplY3Q6IFJFOiBbdjQsIDUvNl0gbW1jOiBrY29uZmlnOiBzZWxlY3Qg
RlNMX0dVVFMgZm9yDQo+IE1NQ19TREhDSV9PRl9FU0RIQw0KPiANCj4gPiAtLS0tLU9yaWdpbmFs
IE1lc3NhZ2UtLS0tLQ0KPiA+IEZyb206IFNjb3R0IFdvb2QgW21haWx0bzpzY290dHdvb2RAZnJl
ZXNjYWxlLmNvbV0NCj4gPiBTZW50OiBUdWVzZGF5LCBEZWNlbWJlciAyOSwgMjAxNSAyOjQ4IEFN
DQo+ID4gVG86IFVsZiBIYW5zc29uDQo+ID4gQ2M6IFlhbmdibyBMdTsgbGludXgtbW1jOyBYLlhp
ZUBmcmVlc2NhbGUuY29tOyBMZW8gbGkNCj4gPiBTdWJqZWN0OiBSZTogW3Y0LCA1LzZdIG1tYzog
a2NvbmZpZzogc2VsZWN0IEZTTF9HVVRTIGZvcg0KPiA+IE1NQ19TREhDSV9PRl9FU0RIQw0KPiA+
DQo+ID4gT24gVGh1LCAyMDE1LTEyLTE3IGF0IDEyOjMwICswMTAwLCBVbGYgSGFuc3NvbiB3cm90
ZToNCj4gPiA+IFsuLi5dDQo+ID4gPg0KPiA+ID4gPiA+DQo+ID4gPiA+ID4gQW5kIEkgdGhpbmsg
c3R1YnMgZm9yIHJlYWRpbmcgU1ZSIGlzIHF1aXRlIGEgYmFkIGlkZWEuICBJdCdsbA0KPiA+ID4g
PiA+IG1ha2UgdGhlIGRyaXZlciBidWlsZCBidXQgaXQgd2lsbCBzaWxlbnRseSBub3QgYmUgYWJs
ZSB0byBhcHBseQ0KPiA+ID4gPiA+IFNWUi1iYXNlZCB3b3JrYXJvdW5kcy4NCj4gPiA+ID4NCj4g
PiA+ID4gSXQgZG9lc24ndCBoYXZlIHRvIGJlICJzaWxlbnQiLCB0aGUgZHJpdmVyIGNhbiByZXR1
cm4gYW4gZXJyb3INCj4gPiA+ID4gKGFuZCBwcmludCBlcnJvciBtZXNzYWdlcykgZnJvbSBpdHMg
LT5wcm9iZSgpIG1ldGhvZCwgaWYgdGhlIGNhbGxzDQo+ID4gPiA+IHRvIHRoZSBHVVRTIGRyaXZl
ciBmYWlscy4NCj4gPiA+ID4NCj4gPiA+ID4gQW55d2F5LCBJIG1lbnRpb25lZCB0aGlzIGlkZWEg
b25seSB0byB1bmRlcnN0YW5kIHRoZSBuZWVkIGZvcg0KPiA+ID4gPiAqb3B0aW9uYWwqIEdVVFMg
c3VwcG9ydHMuIFBlcmhhcHMgdGhlcmUgaXMgYSBjcm9zcyBTT0MgZHJpdmVycw0KPiA+ID4gPiB0
aGF0IGZvciBzb21lIHBsYXRmb3JtcyBkZXBlbmRzIG9uIEdVVFMgYnV0IG9uIG90aGVycyBpdCBk
b2Vzbid0Lg0KPiA+ID4gPg0KPiA+ID4gPiBNYXliZSB0aGF0IGlzbid0IGNhc2UgdGhlbiE/DQo+
ID4gPg0KPiA+ID4gQ2FuIHlvdSBwbGVhc2UgYW5zd2VyIHRoaXMgcXVlc3Rpb24hPw0KPiA+ID4N
Cj4gPiA+IEFjY29yZGluZyB0byB0aGUgZWFybGllciB2ZXJzaW9ucyBvZiB0aGlzIHBhdGNoc2V0
IGFuZCBmcm9tIHlvdXINCj4gPiA+IGNvbW1lbnRzIFsxXSwgaXQgKmRvKiBzZWVtcyBsaWtlIHRo
ZSBHVVRTIGRyaXZlciBtYXkgYmUgb3B0aW9uYWwgYW5kDQo+ID4gPiB0aHVzIHN0dWJzIGNvdWxk
IGFkZHJlc3MgdGhpcy4NCj4gPg0KPiA+IEknZCByYXRoZXIgaXQgbm90IGJlIG9wdGlvbmFsIGF0
IGJ1aWxkIHRpbWUgZm9yIHRoZSByZWFzb24gSSBleHBsYWluZWQNCj4gPiBhYm92ZS4NCj4gPiAg
SXQgd291bGQgYmUgdG9vIGVhc3kgZm9yIHVzZXJzIHRvIGFjY2lkZW50YWxseSBub3QgZW5hYmxl
IHRoZSBndXRzDQo+ID4gZHJpdmVyIGFuZCBtaXNzIHRoZSB3b3JrYXJvdW5kLiAgRXZlbiBpZiBh
biBlcnJvciBpcyBwcmludGVkIGl0J3MgZWFzeQ0KPiA+IHRvIG1pc3MgYW1vbmcgYWxsIHRoZSBi
b290IHNwYW0gLS0gYW5kIGlmIHRoZXJlJ3MgYW55IGxlZ2l0aW1hdGUNCj4gPiByZWFzb24gdG8g
bm90IGhhdmUgdGhlIGRyaXZlciBlbmFibGVkIHRoZW4gd2h5IHdvdWxkIHRoYXQgYmUgYW4NCj4g
PiAiZXJyb3IiPyAgSWYgdGhlIGd1dHMgZHJpdmVyIGZhaWxzIHRvIGJpbmQgKGUuZy4gcnVubmlu
ZyBpbiBhIFZNLCBvciBhDQo+ID4gcGxhdGZvcm0gdGhlIGd1dHMgZHJpdmVyIGRvZXNuJ3QgcmVj
b2duaXplKSB0aGF0J3MgYW5vdGhlciBtYXR0ZXIsIGJ1dA0KPiA+IHRoYXQgc2hvdWxkIGJlIGhh
bmRsZWQgYnkgYW4gZXJyb3IgY2hlY2sgaW4gdGhlIGd1dHMgZHJpdmVyLCBub3QgYQ0KPiBidWls
ZC10aW1lIHN0dWIuDQo+ID4NCj4gPiAtU2NvdHQNCj4gPg0KPiANCj4gW0x1IFlhbmdiby1CNDcw
OTNdIEZvciB0aGUgJ29wdGlvbmFsIG9yIG5vdCBvcHRpb25hbCcgcHJvYmxlbSwgSSB0aGluaw0K
PiBTY290dCdzIG9waW5pb24gaXMgcmVhc29uYWJsZS4NCj4gSWYgd2UgdXNlIGd1dHMgZHJpdmVy
LCAnc2VsZWN0JyBjb3VsZCBhdm9pZCBtaXNzaW5nIHRoZSB3b3JrYXJvdW5kLg0KPiBJZiB3ZSB1
c2Ugc3lzY29uLCBzZWxlY3QgaXMgYmV0dGVyIHRoYW4gJ2RlcGVuZCBvbicgc2luY2UgJ2RlcGVu
ZCBvbicNCj4gbmVlZHMgdXNlciBzZWxmIHRvIGZpbmQgb3V0IHRoZSBkZXBlbmRlbmN5Lg0KPiAN
Cj4gQW55d2F5LCB3ZSBzaG91bGQgY29udGludWUgdGhlIGRpc2N1c3Npb24gdG8gY29tZSB0byBh
biBhZ3JlZW1lbnQgZmluYWxseS4NCj4gSSB3b3VsZCB2ZXJ5IGFwcHJlY2lhdGUhDQo+IA0KPiBU
aGFua3MuDQo+IA0KPiANCltMdSBZYW5nYm8tQjQ3MDkzXSBIaSBVZmZlLCBkbyB5b3UgaGF2ZSBh
bnkgY29tbWVudHM/IDopDQoNCg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Crystal Wood Jan. 8, 2016, 6:34 a.m. UTC | #18
On Fri, 2016-01-08 at 06:17 +0000, Yangbo Lu wrote:
> > -----Original Message-----
> > From: Scott Wood
> > Sent: Friday, January 08, 2016 6:24 AM
> > To: Yangbo Lu; Scott Wood; Ulf Hansson
> > Cc: Lu Y.B.; linux-mmc; Xiaobo Xie; Li Leo
> > Subject: Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for
> > MMC_SDHCI_OF_ESDHC
> > 
> > On 01/06/2016 12:58 AM, Yangbo Lu wrote:
> > > > -----Original Message-----
> > > > From: Scott Wood [mailto:scottwood@freescale.com]
> > > > Sent: Tuesday, December 29, 2015 3:10 AM
> > > > To: Ulf Hansson; Yangbo Lu
> > > > Cc: Lu Yangbo-B47093; linux-mmc; Xie Xiaobo-R63061; Leo li
> > > > Subject: Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for
> > > > MMC_SDHCI_OF_ESDHC
> > > > 
> > > > On Mon, 2015-12-28 at 13:10 +0100, Ulf Hansson wrote:
> > > > > On 28 December 2015 at 11:26, Yangbo Lu <yangbo.lu@nxp.com> wrote:
> > > > > > > -----Original Message-----
> > > > > > > From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> > > > > > > Sent: Thursday, December 17, 2015 7:31 PM
> > > > > > > To: Scott Wood
> > > > > > > Cc: Lu Yangbo-B47093; linux-mmc; Xie Xiaobo-R63061; Leo li
> > > > > > > Subject: Re: [v4, 5/6] mmc: kconfig: select FSL_GUTS for
> > > > > > > MMC_SDHCI_OF_ESDHC
> > > > > > > 
> > > > > > > [...]
> > > > > > > 
> > > > > > > > > 
> > > > > > > > > And I think stubs for reading SVR is quite a bad idea. 
> > > > > > > > >  It'll
> > > > > > > > > make the driver build but it will silently not be able to
> > > > > > > > > apply
> > > > > > > > > SVR-based
> > > > > > > workarounds.
> > > > > > > > 
> > > > > > > > It doesn't have to be "silent", the driver can return an error
> > > > > > > > (and print error messages) from its ->probe() method, if the
> > > > > > > > calls to the GUTS driver fails.
> > > > > > > > 
> > > > > > > > Anyway, I mentioned this idea only to understand the need for
> > > > > > > > *optional* GUTS supports. Perhaps there is a cross SOC drivers
> > > > > > > > that for some platforms depends on GUTS but on others it
> > > > > > > > doesn't.
> > > > > > > > 
> > > > > > > > Maybe that isn't case then!?
> > > > > > > 
> > > > > > > Can you please answer this question!?
> > > > > > > 
> > > > > > > According to the earlier versions of this patchset and from your
> > > > > > > comments [1], it *do* seems like the GUTS driver may be optional
> > > > > > > and thus stubs could address this.
> > > > > > > 
> > > > > > > Kind regards
> > > > > > > Uffe
> > > > > > > 
> > > > > > > [1]
> > > > > > > http://www.spinics.net/lists/linux-mmc/msg34412.html
> > > > > > 
> > > > > > [Lu Yangbo-B47093] Hi Scott and Uffe, In the earlier version, I'd
> > > > > > like to use syscon support and only add 'syscon' compatible in the
> > > > > > dts whose eSDHC needs to use it to get SVR.
> > > > > > But I never thought this had caused so much discussion... :(
> > > > > 
> > > > > Sorry, I understand your frustration but that's life sometimes. :-)
> > > > > 
> > > > > To me, the syscon solution is more elegant...
> > > > 
> > > > The syscon patch was terrible.  It would have accessed a certain
> > > > location in any node labelled "syscon" whether it was guts or not, in
> > > > addition to the other complaints.
> > > > 
> > > > -Scott
> > > 
> > > [Lu Yangbo-B47093] As my understand, the syscon APIs would just check
> > whether there is a 'syscon' compatible.
> > > If no, the APIs return. We still could maintain a list of compatibles
> > for guts if using syscon.
> > 
> > Where would that list of compatibles go?  Duplicated in every driver that
> > needs something from guts?  Why decentralize the implementation?
> > 
> > > In my opinion, syscon and guts driver are just two method to get SVR.
> > 
> > The guts driver could eventually handle more than just SVR, and it could
> > handle SVR better (e.g. on PPC fall back to reading the SPR if guts is
> > unavailable).
> > 
> > -Scott
> 
> [Lu Yangbo-B47093] Ok... It seems no proper place to maintain the guts list
> presently...
> I also have had a question about the guts driver.
> Could you suggest where to register the guts driver, to make it registered
> for all QorIQ platforms and avoid unavailability to call it?

In an initcall, just like other normal drivers.  Maybe subsys_initcall.

-Scott

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson Jan. 14, 2016, 10:31 a.m. UTC | #19
> [Lu Yangbo-B47093] For the 'optional or not optional' problem, I think Scott's opinion is reasonable.
> If we use guts driver, 'select' could avoid missing the workaround.
> If we use syscon, select is better than 'depend on' since 'depend on' needs user self to find out the dependency.
>
> Anyway, we should continue the discussion to come to an agreement finally.
> I would very appreciate!

Please, go ahead and use the "select guts driver" option, as you both
seems to feel that strong about it.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 1526b8a..df57c14 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -142,6 +142,8 @@  config MMC_SDHCI_OF_ESDHC
 	depends on MMC_SDHCI_PLTFM
 	depends on PPC || ARCH_MXC || ARCH_LAYERSCAPE
 	select MMC_SDHCI_IO_ACCESSORS
+	select SOC_FSL
+	select FSL_GUTS
 	help
 	  This selects the Freescale eSDHC controller support.