Vesnin BMC uses microSD with card presence signal inversion in the
schematics. Change the .get_cd callback to detect 'cd-inverted' option
in dts. There is no WP switch, due to this 'disable-wp' also was added
into vesnin dts for sdhci.
Ivan Mikhaylov (2):
aspeed: dts: add sd card for vesnin
mmc: sdhci-of-aspeed: add inversion signal presence
arch/arm/boot/dts/aspeed-bmc-opp-vesnin.dts | 13 +++++++++++++
drivers/mmc/host/sdhci-of-aspeed.c | 17 +++++++++++++++++
2 files changed, 30 insertions(+)
--
2.20.1
Change the default .get_cd callback. Add inverted signal card detection
check.
Signed-off-by: Ivan Mikhaylov <[email protected]>
diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
index 8962f6664381..186559ee8fcc 100644
--- a/drivers/mmc/host/sdhci-of-aspeed.c
+++ b/drivers/mmc/host/sdhci-of-aspeed.c
@@ -143,6 +143,19 @@ static inline int aspeed_sdhci_calculate_slot(struct aspeed_sdhci *dev,
return (delta / 0x100) - 1;
}
+static int aspeed_get_cd(struct mmc_host *mmc)
+{
+ struct sdhci_host *host = mmc_priv(mmc);
+
+ int present = !!(sdhci_readl(host, SDHCI_PRESENT_STATE)
+ & SDHCI_CARD_PRESENT);
+
+ if (mmc->caps2 & MMC_CAP2_CD_ACTIVE_HIGH)
+ present = !present;
+
+ return present;
+}
+
static int aspeed_sdhci_probe(struct platform_device *pdev)
{
struct sdhci_pltfm_host *pltfm_host;
@@ -183,6 +196,10 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
goto err_pltfm_free;
}
+ host->mmc_host_ops.get_cd = aspeed_get_cd;
+ if (of_property_read_bool(pdev->dev.of_node, "cd-inverted"))
+ dev_info(&pdev->dev, "aspeed: sdhci: presence signal inversion enabled\n");
+
ret = mmc_of_parse(host->mmc);
if (ret)
goto err_sdhci_add;
--
2.20.1
Presence signal is inverted for vesnin boards, 'cd-inverted' added
for invertion signal enablement. Vesnin BMC uses microSD, there is
no WP switch, 'disable-wp' is used for this purpose.
Signed-off-by: Ivan Mikhaylov <[email protected]>
diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-vesnin.dts b/arch/arm/boot/dts/aspeed-bmc-opp-vesnin.dts
index a27c88d23056..7ae3436e0d99 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-vesnin.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-vesnin.dts
@@ -232,3 +232,16 @@
&wdt2 {
aspeed,alt-boot;
};
+
+&sdmmc {
+ status = "okay";
+};
+
+&sdhci1 {
+ status = "okay";
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_sd2_default>;
+ cd-inverted;
+ disable-wp;
+};
--
2.20.1
On 14/11/19 2:54 PM, Ivan Mikhaylov wrote:
> Change the default .get_cd callback. Add inverted signal card detection
> check.
>
> Signed-off-by: Ivan Mikhaylov <[email protected]>
>
> diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
> index 8962f6664381..186559ee8fcc 100644
> --- a/drivers/mmc/host/sdhci-of-aspeed.c
> +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> @@ -143,6 +143,19 @@ static inline int aspeed_sdhci_calculate_slot(struct aspeed_sdhci *dev,
> return (delta / 0x100) - 1;
> }
>
> +static int aspeed_get_cd(struct mmc_host *mmc)
> +{
> + struct sdhci_host *host = mmc_priv(mmc);
> +
> + int present = !!(sdhci_readl(host, SDHCI_PRESENT_STATE)
> + & SDHCI_CARD_PRESENT);
> +
> + if (mmc->caps2 & MMC_CAP2_CD_ACTIVE_HIGH)
> + present = !present;
Perhaps safer to flip the bit using CONFIG_MMC_SDHCI_IO_ACCESSORS and
->readl() callback
> +
> + return present;
> +}
> +
> static int aspeed_sdhci_probe(struct platform_device *pdev)
> {
> struct sdhci_pltfm_host *pltfm_host;
> @@ -183,6 +196,10 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
> goto err_pltfm_free;
> }
>
> + host->mmc_host_ops.get_cd = aspeed_get_cd;
> + if (of_property_read_bool(pdev->dev.of_node, "cd-inverted"))
> + dev_info(&pdev->dev, "aspeed: sdhci: presence signal inversion enabled\n");
Is this print really needed?
> +
> ret = mmc_of_parse(host->mmc);
> if (ret)
> goto err_sdhci_add;
>
On Thu, 2019-11-14 at 15:10 +0200, Adrian Hunter wrote:
> On 14/11/19 2:54 PM, Ivan Mikhaylov wrote:
> > Change the default .get_cd callback. Add inverted signal card detection
> > check.
> >
> > Signed-off-by: Ivan Mikhaylov <[email protected]>
> >
> > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-
> > aspeed.c
> > index 8962f6664381..186559ee8fcc 100644
> > --- a/drivers/mmc/host/sdhci-of-aspeed.c
> > +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> > @@ -143,6 +143,19 @@ static inline int aspeed_sdhci_calculate_slot(struct
> > aspeed_sdhci *dev,
> > return (delta / 0x100) - 1;
> > }
> >
> > +static int aspeed_get_cd(struct mmc_host *mmc)
> > +{
> > + struct sdhci_host *host = mmc_priv(mmc);
> > +
> > + int present = !!(sdhci_readl(host, SDHCI_PRESENT_STATE)
> > + & SDHCI_CARD_PRESENT);
> > +
> > + if (mmc->caps2 & MMC_CAP2_CD_ACTIVE_HIGH)
> > + present = !present;
>
> Perhaps safer to flip the bit using CONFIG_MMC_SDHCI_IO_ACCESSORS and
> ->readl() callback
>
Sorry, don't quite understand what you're saying. You want to instantiate
'.read_l' callback instead of '.get_cd' in sdhci_ops and substitute the real
value?
res = readl(base, reg);
if (reg == SDHCI_PRESENT_STATE)
if (mmc->caps2 & MMC_CAP2_CD_ACTIVE_HIGH)
return !res;
return res;
Something like this?
>
> > + host->mmc_host_ops.get_cd = aspeed_get_cd;
> > + if (of_property_read_bool(pdev->dev.of_node, "cd-inverted"))
> > + dev_info(&pdev->dev, "aspeed: sdhci: presence signal inversion
> > enabled\n");
>
> Is this print really needed?
>
I can remove it if you think it's redundant.
Thanks.
On 14/11/19 7:19 PM, Ivan Mikhaylov wrote:
> On Thu, 2019-11-14 at 15:10 +0200, Adrian Hunter wrote:
>> On 14/11/19 2:54 PM, Ivan Mikhaylov wrote:
>>> Change the default .get_cd callback. Add inverted signal card detection
>>> check.
>>>
>>> Signed-off-by: Ivan Mikhaylov <[email protected]>
>>>
>>> diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-
>>> aspeed.c
>>> index 8962f6664381..186559ee8fcc 100644
>>> --- a/drivers/mmc/host/sdhci-of-aspeed.c
>>> +++ b/drivers/mmc/host/sdhci-of-aspeed.c
>>> @@ -143,6 +143,19 @@ static inline int aspeed_sdhci_calculate_slot(struct
>>> aspeed_sdhci *dev,
>>> return (delta / 0x100) - 1;
>>> }
>>>
>>> +static int aspeed_get_cd(struct mmc_host *mmc)
>>> +{
>>> + struct sdhci_host *host = mmc_priv(mmc);
>>> +
>>> + int present = !!(sdhci_readl(host, SDHCI_PRESENT_STATE)
>>> + & SDHCI_CARD_PRESENT);
>>> +
>>> + if (mmc->caps2 & MMC_CAP2_CD_ACTIVE_HIGH)
>>> + present = !present;
>>
>> Perhaps safer to flip the bit using CONFIG_MMC_SDHCI_IO_ACCESSORS and
>> ->readl() callback
>>
>
> Sorry, don't quite understand what you're saying. You want to instantiate
> '.read_l' callback instead of '.get_cd' in sdhci_ops and substitute the real
> value?
>
> res = readl(base, reg);
> if (reg == SDHCI_PRESENT_STATE)
> if (mmc->caps2 & MMC_CAP2_CD_ACTIVE_HIGH)
> return !res;
Presumably just flip the SDHCI_CARD_PRESENT bit i.e.
return res ^ SDHCI_CARD_PRESENT;
> return res;
>
> Something like this?
Yes
>
>>
>>> + host->mmc_host_ops.get_cd = aspeed_get_cd;
>>> + if (of_property_read_bool(pdev->dev.of_node, "cd-inverted"))
>>> + dev_info(&pdev->dev, "aspeed: sdhci: presence signal inversion
>>> enabled\n");
>>
>> Is this print really needed?
>>
> I can remove it if you think it's redundant.
>
> Thanks.
>
>
On Fri, 2019-11-15 at 09:20 +0200, Adrian Hunter wrote:
> On 14/11/19 7:19 PM, Ivan Mikhaylov wrote:
> > On Thu, 2019-11-14 at 15:10 +0200, Adrian Hunter wrote:
> > On 14/11/19 2:54 PM, Ivan Mikhaylov wrote:
> > > > Change the default .get_cd callback. Add inverted signal card detection
> > > > check.
> > > >
> > > > Signed-off-by: Ivan Mikhaylov <[email protected]>
> > > >
> > > > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c
> > > > b/drivers/mmc/host/sdhci-of-
> > > > aspeed.c
> > > > index 8962f6664381..186559ee8fcc 100644
> > > > --- a/drivers/mmc/host/sdhci-of-aspeed.c
> > > > +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> > > > @@ -143,6 +143,19 @@ static inline int
> > > > aspeed_sdhci_calculate_slot(struct
> > > > aspeed_sdhci *dev,
> > > > return (delta / 0x100) - 1;
> > > > }
> > > >
> > > > +static int aspeed_get_cd(struct mmc_host *mmc)
> > > > +{
> > > > + struct sdhci_host *host = mmc_priv(mmc);
> > > > +
> > > > + int present = !!(sdhci_readl(host, SDHCI_PRESENT_STATE)
> > > > + & SDHCI_CARD_PRESENT);
> > > > +
> > > > + if (mmc->caps2 & MMC_CAP2_CD_ACTIVE_HIGH)
> > > > + present = !present;
> > >
> > > Perhaps safer to flip the bit using CONFIG_MMC_SDHCI_IO_ACCESSORS and
> > > ->readl() callback
>
>
> > Sorry, don't quite understand what you're saying. You want to instantiate
> > '.read_l' callback instead of '.get_cd' in sdhci_ops and substitute the real
> > value?
> >
> > res = readl(base, reg);
> > if (reg == SDHCI_PRESENT_STATE)
> > if (mmc->caps2 & MMC_CAP2_CD_ACTIVE_HIGH)
> > return !res;
>
> Presumably just flip the SDHCI_CARD_PRESENT bit i.e.
>
> return res ^ SDHCI_CARD_PRESENT;
>
> > return res;
> >
> > Something like this?
>
> Yes
>
Don't you think it will bring a little overhead on any sdhci_readl plus
sdhci_readl will not get the real value in case of inverted signal which seems
is not right from communication fairness between hw and sw? I took that approach
with .get_cd from variety of drivers in host/mmc but if you think it will be
better and safer with .read_l - I'll do that way.
Sorry for the link in subject, didn't notice that I put it in previous message
somehow.
Thanks.
On 15/11/19 2:56 PM, Ivan Mikhaylov wrote:
> On Fri, 2019-11-15 at 09:20 +0200, Adrian Hunter wrote:
>> On 14/11/19 7:19 PM, Ivan Mikhaylov wrote:
>>> On Thu, 2019-11-14 at 15:10 +0200, Adrian Hunter wrote:
>>> On 14/11/19 2:54 PM, Ivan Mikhaylov wrote:
>>>>> Change the default .get_cd callback. Add inverted signal card detection
>>>>> check.
>>>>>
>>>>> Signed-off-by: Ivan Mikhaylov <[email protected]>
>>>>>
>>>>> diff --git a/drivers/mmc/host/sdhci-of-aspeed.c
>>>>> b/drivers/mmc/host/sdhci-of-
>>>>> aspeed.c
>>>>> index 8962f6664381..186559ee8fcc 100644
>>>>> --- a/drivers/mmc/host/sdhci-of-aspeed.c
>>>>> +++ b/drivers/mmc/host/sdhci-of-aspeed.c
>>>>> @@ -143,6 +143,19 @@ static inline int
>>>>> aspeed_sdhci_calculate_slot(struct
>>>>> aspeed_sdhci *dev,
>>>>> return (delta / 0x100) - 1;
>>>>> }
>>>>>
>>>>> +static int aspeed_get_cd(struct mmc_host *mmc)
>>>>> +{
>>>>> + struct sdhci_host *host = mmc_priv(mmc);
>>>>> +
>>>>> + int present = !!(sdhci_readl(host, SDHCI_PRESENT_STATE)
>>>>> + & SDHCI_CARD_PRESENT);
>>>>> +
>>>>> + if (mmc->caps2 & MMC_CAP2_CD_ACTIVE_HIGH)
>>>>> + present = !present;
>>>>
>>>> Perhaps safer to flip the bit using CONFIG_MMC_SDHCI_IO_ACCESSORS and
>>>> ->readl() callback
>>
>>
>>> Sorry, don't quite understand what you're saying. You want to instantiate
>>> '.read_l' callback instead of '.get_cd' in sdhci_ops and substitute the real
>>> value?
>>>
>>> res = readl(base, reg);
>>> if (reg == SDHCI_PRESENT_STATE)
>>> if (mmc->caps2 & MMC_CAP2_CD_ACTIVE_HIGH)
>>> return !res;
>>
>> Presumably just flip the SDHCI_CARD_PRESENT bit i.e.
>>
>> return res ^ SDHCI_CARD_PRESENT;
>>
>>> return res;
>>>
>>> Something like this?
>>
>> Yes
>>
>
> Don't you think it will bring a little overhead on any sdhci_readl plus
Register accesses are usually slow (~1us) compared with logic. Of course,
I/O requests are even slower >100ms so it is unlikely that the overhead is
significant.
> sdhci_readl will not get the real value in case of inverted signal which seems
> is not right from communication fairness between hw and sw? I took that approach
One of the purposes of the accessors is to smooth over the difference
between the SDHCI standard and actual hardware. Given that
SDHCI_PRESENT_STATE is also used in sdhci_set_card_detection() and
sdhci_irq(), it is surprising that changing only ->get_cd() works correctly.
> with .get_cd from variety of drivers in host/mmc but if you think it will be
> better and safer with .read_l - I'll do that way.
>
> Sorry for the link in subject, didn't notice that I put it in previous message
> somehow.
>
> Thanks.
>
>