diff mbox series

mmc: sdhci-iproc: Add ACPI bindings for the rpi4

Message ID 20210108211339.1724769-1-jeremy.linton@arm.com
State New
Headers show
Series mmc: sdhci-iproc: Add ACPI bindings for the rpi4 | expand

Commit Message

Jeremy Linton Jan. 8, 2021, 9:13 p.m. UTC
The rpi4 has a Arasan controller it carries over
from the rpi3, and a newer eMMC2 controller.
Because of a couple "quirks" it seems wiser to bind
these controllers to the same driver that DT is using
on this platform rather than the generic sdhci_acpi
driver with PNP0D40.

So, we use BCM2847 for the older Arasan and BRCME88C
for the newer eMMC2.

With this change linux is capable of utilizing the
SD card slot, and the wifi on this platform
with linux.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

---
 drivers/mmc/host/sdhci-iproc.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

-- 
2.26.2

Comments

Florian Fainelli Jan. 8, 2021, 9:27 p.m. UTC | #1
On 1/8/2021 1:13 PM, Jeremy Linton wrote:
> The rpi4 has a Arasan controller it carries over

> from the rpi3, and a newer eMMC2 controller.

> Because of a couple "quirks" it seems wiser to bind

> these controllers to the same driver that DT is using

> on this platform rather than the generic sdhci_acpi

> driver with PNP0D40.

> 

> So, we use BCM2847 for the older Arasan and BRCME88C

> for the newer eMMC2.

> 

> With this change linux is capable of utilizing the

> SD card slot, and the wifi on this platform

> with linux.

> 

> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>


As far as ACPI ID's go:

Acked-by: Florian Fainelli <f.fainelli@gmail.com>

-- 
Florian
kernel test robot Jan. 9, 2021, 1:10 a.m. UTC | #2
Hi Jeremy,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.11-rc2 next-20210108]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jeremy-Linton/mmc-sdhci-iproc-Add-ACPI-bindings-for-the-rpi4/20210109-051645
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 6279d812eab67a6df6b22fa495201db6f2305924
config: riscv-randconfig-r012-20210108 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project bc556e5685c0f97e79fb7b3c6f15cc5062db8e36)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/0day-ci/linux/commit/659eacf5a5de971ea94390dd6c7443c82d53ea5e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jeremy-Linton/mmc-sdhci-iproc-Add-ACPI-bindings-for-the-rpi4/20210109-051645
        git checkout 659eacf5a5de971ea94390dd6c7443c82d53ea5e
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=riscv 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from drivers/mmc/host/sdhci-iproc.c:24:
   In file included from drivers/mmc/host/sdhci-pltfm.h:13:
   In file included from drivers/mmc/host/sdhci.h:13:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/riscv/include/asm/io.h:149:
   include/asm-generic/io.h:556:9: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           return inb(addr);
                  ^~~~~~~~~
   arch/riscv/include/asm/io.h:55:76: note: expanded from macro 'inb'
   #define inb(c)          ({ u8  __v; __io_pbr(); __v = readb_cpu((void*)(PCI_IOBASE + (c))); __io_par(__v); __v; })
                                                                           ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:87:48: note: expanded from macro 'readb_cpu'
   #define readb_cpu(c)            ({ u8  __r = __raw_readb(c); __r; })
                                                            ^
   In file included from drivers/mmc/host/sdhci-iproc.c:24:
   In file included from drivers/mmc/host/sdhci-pltfm.h:13:
   In file included from drivers/mmc/host/sdhci.h:13:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/riscv/include/asm/io.h:149:
   include/asm-generic/io.h:564:9: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           return inw(addr);
                  ^~~~~~~~~
   arch/riscv/include/asm/io.h:56:76: note: expanded from macro 'inw'
   #define inw(c)          ({ u16 __v; __io_pbr(); __v = readw_cpu((void*)(PCI_IOBASE + (c))); __io_par(__v); __v; })
                                                                           ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:88:76: note: expanded from macro 'readw_cpu'
   #define readw_cpu(c)            ({ u16 __r = le16_to_cpu((__force __le16)__raw_readw(c)); __r; })
                                                                                        ^
   include/uapi/linux/byteorder/little_endian.h:36:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from drivers/mmc/host/sdhci-iproc.c:24:
   In file included from drivers/mmc/host/sdhci-pltfm.h:13:
   In file included from drivers/mmc/host/sdhci.h:13:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/riscv/include/asm/io.h:149:
   include/asm-generic/io.h:572:9: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           return inl(addr);
                  ^~~~~~~~~
   arch/riscv/include/asm/io.h:57:76: note: expanded from macro 'inl'
   #define inl(c)          ({ u32 __v; __io_pbr(); __v = readl_cpu((void*)(PCI_IOBASE + (c))); __io_par(__v); __v; })
                                                                           ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:89:76: note: expanded from macro 'readl_cpu'
   #define readl_cpu(c)            ({ u32 __r = le32_to_cpu((__force __le32)__raw_readl(c)); __r; })
                                                                                        ^
   include/uapi/linux/byteorder/little_endian.h:34:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from drivers/mmc/host/sdhci-iproc.c:24:
   In file included from drivers/mmc/host/sdhci-pltfm.h:13:
   In file included from drivers/mmc/host/sdhci.h:13:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/riscv/include/asm/io.h:149:
   include/asm-generic/io.h:580:2: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           outb(value, addr);
           ^~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/io.h:59:68: note: expanded from macro 'outb'
   #define outb(v,c)       ({ __io_pbw(); writeb_cpu((v),(void*)(PCI_IOBASE + (c))); __io_paw(); })
                                                                 ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:91:52: note: expanded from macro 'writeb_cpu'
   #define writeb_cpu(v, c)        ((void)__raw_writeb((v), (c)))
                                                             ^
   In file included from drivers/mmc/host/sdhci-iproc.c:24:
   In file included from drivers/mmc/host/sdhci-pltfm.h:13:
   In file included from drivers/mmc/host/sdhci.h:13:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/riscv/include/asm/io.h:149:
   include/asm-generic/io.h:588:2: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           outw(value, addr);
           ^~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/io.h:60:68: note: expanded from macro 'outw'
   #define outw(v,c)       ({ __io_pbw(); writew_cpu((v),(void*)(PCI_IOBASE + (c))); __io_paw(); })
                                                                 ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:92:76: note: expanded from macro 'writew_cpu'
   #define writew_cpu(v, c)        ((void)__raw_writew((__force u16)cpu_to_le16(v), (c)))
                                                                                     ^
   In file included from drivers/mmc/host/sdhci-iproc.c:24:
   In file included from drivers/mmc/host/sdhci-pltfm.h:13:
   In file included from drivers/mmc/host/sdhci.h:13:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/riscv/include/asm/io.h:149:
   include/asm-generic/io.h:596:2: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           outl(value, addr);
           ^~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/io.h:61:68: note: expanded from macro 'outl'
   #define outl(v,c)       ({ __io_pbw(); writel_cpu((v),(void*)(PCI_IOBASE + (c))); __io_paw(); })
                                                                 ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:93:76: note: expanded from macro 'writel_cpu'
   #define writel_cpu(v, c)        ((void)__raw_writel((__force u32)cpu_to_le32(v), (c)))
                                                                                     ^
   In file included from drivers/mmc/host/sdhci-iproc.c:24:
   In file included from drivers/mmc/host/sdhci-pltfm.h:13:
   In file included from drivers/mmc/host/sdhci.h:13:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/riscv/include/asm/io.h:149:
   include/asm-generic/io.h:1005:55: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port;
                                                     ~~~~~~~~~~ ^
>> drivers/mmc/host/sdhci-iproc.c:272:38: warning: unused variable 'bcm_arasan_data' [-Wunused-const-variable]

   static const struct sdhci_iproc_data bcm_arasan_data = {
                                        ^
   8 warnings generated.


vim +/bcm_arasan_data +272 drivers/mmc/host/sdhci-iproc.c

   271	
 > 272	static const struct sdhci_iproc_data bcm_arasan_data = {

   273		.pdata = &sdhci_bcm_arasan_data,
   274	};
   275	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Jeremy Linton Jan. 9, 2021, 3:18 a.m. UTC | #3
Hi,

On 1/8/21 7:10 PM, kernel test robot wrote:
> Hi Jeremy,

> 

> Thank you for the patch! Perhaps something to improve:

> 

> [auto build test WARNING on linus/master]

> [also build test WARNING on v5.11-rc2 next-20210108]

> [If your patch is applied to the wrong git tree, kindly drop us a note.

> And when submitting patch, we suggest to use '--base' as documented in

> https://git-scm.com/docs/git-format-patch]

> 

> url:    https://github.com/0day-ci/linux/commits/Jeremy-Linton/mmc-sdhci-iproc-Add-ACPI-bindings-for-the-rpi4/20210109-051645

> base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 6279d812eab67a6df6b22fa495201db6f2305924

> config: riscv-randconfig-r012-20210108 (attached as .config)

> compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project bc556e5685c0f97e79fb7b3c6f15cc5062db8e36)

> reproduce (this is a W=1 build):

>          wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross

>          chmod +x ~/bin/make.cross

>          # install riscv cross compiling tool for clang build

>          # apt-get install binutils-riscv64-linux-gnu

>          # https://github.com/0day-ci/linux/commit/659eacf5a5de971ea94390dd6c7443c82d53ea5e

>          git remote add linux-review https://github.com/0day-ci/linux

>          git fetch --no-tags linux-review Jeremy-Linton/mmc-sdhci-iproc-Add-ACPI-bindings-for-the-rpi4/20210109-051645

>          git checkout 659eacf5a5de971ea94390dd6c7443c82d53ea5e

>          # save the attached .config to linux build tree

>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=riscv

> 

> If you fix the issue, kindly add following tag as appropriate

> Reported-by: kernel test robot <lkp@intel.com>

> 

> All warnings (new ones prefixed by >>):


(trimming)

>     include/asm-generic/io.h:1005:55: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]

>             return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port;

>                                                       ~~~~~~~~~~ ^

>>> drivers/mmc/host/sdhci-iproc.c:272:38: warning: unused variable 'bcm_arasan_data' [-Wunused-const-variable]

>     static const struct sdhci_iproc_data bcm_arasan_data = {


I think this is the only one caused by this patch, and its because the 
new structures are only used inside the #ifdef ACPI block.

I will post a v2.

>                                          ^

>     8 warnings generated.

> 

> 

> vim +/bcm_arasan_data +272 drivers/mmc/host/sdhci-iproc.c

> 

>     271	

>   > 272	static const struct sdhci_iproc_data bcm_arasan_data = {

>     273		.pdata = &sdhci_bcm_arasan_data,

>     274	};

>     275	

> 

> ---

> 0-DAY CI Kernel Test Service, Intel Corporation

> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

>
Stefan Wahren Jan. 9, 2021, 11:07 a.m. UTC | #4
Hi Jeremy,

+add Nicolas

Am 08.01.21 um 22:13 schrieb Jeremy Linton:
> The rpi4 has a Arasan controller it carries over

> from the rpi3, and a newer eMMC2 controller.

> Because of a couple "quirks" it seems wiser to bind

> these controllers to the same driver that DT is using

> on this platform rather than the generic sdhci_acpi

> driver with PNP0D40.

>

> So, we use BCM2847 for the older Arasan and BRCME88C

> for the newer eMMC2.

>

> With this change linux is capable of utilizing the

> SD card slot, and the wifi on this platform

> with linux.

>

> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

> ---

>  drivers/mmc/host/sdhci-iproc.c | 14 ++++++++++++++

>  1 file changed, 14 insertions(+)

>

> diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c

> index c9434b461aab..f79d97b41805 100644

> --- a/drivers/mmc/host/sdhci-iproc.c

> +++ b/drivers/mmc/host/sdhci-iproc.c

> @@ -250,6 +250,14 @@ static const struct sdhci_pltfm_data sdhci_bcm2835_pltfm_data = {

>  	.ops = &sdhci_iproc_32only_ops,

>  };

>  

> +static const struct sdhci_pltfm_data sdhci_bcm_arasan_data = {

> +	.quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION |

> +		  SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK |

> +		  SDHCI_QUIRK_NO_HISPD_BIT,

> +	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,

> +	.ops = &sdhci_iproc_32only_ops,

> +};

Why do we need an almost exact copy of bcm2835_data which works fine for
all Raspberry Pi boards?
> +

>  static const struct sdhci_iproc_data bcm2835_data = {

>  	.pdata = &sdhci_bcm2835_pltfm_data,

>  	.caps = ((0x1 << SDHCI_MAX_BLOCK_SHIFT)

> @@ -261,6 +269,10 @@ static const struct sdhci_iproc_data bcm2835_data = {

>  	.mmc_caps = 0x00000000,

>  };

>  

> +static const struct sdhci_iproc_data bcm_arasan_data = {

> +	.pdata = &sdhci_bcm_arasan_data,

> +};

> +

>  static const struct sdhci_ops sdhci_iproc_bcm2711_ops = {

>  	.read_l = sdhci_iproc_readl,

>  	.read_w = sdhci_iproc_readw,

> @@ -299,6 +311,8 @@ MODULE_DEVICE_TABLE(of, sdhci_iproc_of_match);

>  static const struct acpi_device_id sdhci_iproc_acpi_ids[] = {

>  	{ .id = "BRCM5871", .driver_data = (kernel_ulong_t)&iproc_cygnus_data },

>  	{ .id = "BRCM5872", .driver_data = (kernel_ulong_t)&iproc_data },

> +	{ .id = "BCM2847",  .driver_data = (kernel_ulong_t)&bcm_arasan_data },


Sorry, i don't have deeper knowledge about ACPI, but BCM2837 is the
official naming of the SoC on the RPi 3.

Is this a typo in the id?

> +	{ .id = "BRCME88C", .driver_data = (kernel_ulong_t)&bcm2711_data },

>  	{ /* sentinel */ }

>  };

>  MODULE_DEVICE_TABLE(acpi, sdhci_iproc_acpi_ids);
Jeremy Linton Jan. 11, 2021, 3:39 a.m. UTC | #5
Hi,

On 1/9/21 5:07 AM, Stefan Wahren wrote:
> Hi Jeremy,

> 

> +add Nicolas

> 

> Am 08.01.21 um 22:13 schrieb Jeremy Linton:

>> The rpi4 has a Arasan controller it carries over

>> from the rpi3, and a newer eMMC2 controller.

>> Because of a couple "quirks" it seems wiser to bind

>> these controllers to the same driver that DT is using

>> on this platform rather than the generic sdhci_acpi

>> driver with PNP0D40.

>>

>> So, we use BCM2847 for the older Arasan and BRCME88C

>> for the newer eMMC2.

>>

>> With this change linux is capable of utilizing the

>> SD card slot, and the wifi on this platform

>> with linux.

>>

>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

>> ---

>>   drivers/mmc/host/sdhci-iproc.c | 14 ++++++++++++++

>>   1 file changed, 14 insertions(+)

>>

>> diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c

>> index c9434b461aab..f79d97b41805 100644

>> --- a/drivers/mmc/host/sdhci-iproc.c

>> +++ b/drivers/mmc/host/sdhci-iproc.c

>> @@ -250,6 +250,14 @@ static const struct sdhci_pltfm_data sdhci_bcm2835_pltfm_data = {

>>   	.ops = &sdhci_iproc_32only_ops,

>>   };

>>   

>> +static const struct sdhci_pltfm_data sdhci_bcm_arasan_data = {

>> +	.quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION |

>> +		  SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK |

>> +		  SDHCI_QUIRK_NO_HISPD_BIT,

>> +	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,

>> +	.ops = &sdhci_iproc_32only_ops,

>> +};


First, thanks for taking a look at this!


> Why do we need an almost exact copy of bcm2835_data which works fine for

> all Raspberry Pi boards?


The short answer to the remainder of this email is that i'm trying to 
continue supporting existing OSs (windows) using the ACPI tables on the 
rpi3/rpi4 while adding rpi4+Linux support.

An even shorter answer is they don't work because ACPI doesn't provide 
the same clock/attributes/etc controls that exist with DT.

So, what happened here is that I got this controller "working" with the 
generic PNP0D40 sdhci_acpi driver. I managed this only by controlling 
the sdhci_caps/masks in the firmware. In theory this minimizes the 
amount of work needed on the other OS which are booting on the same ACPI 
tables (*bsds). They should only need to quirk the bcm/arasan specific 
functionality, rather than some of the quirking which change the caps 
behavior. But because we don't know which if any of the older rpi/arasan 
quirks are still needed the safest solution is to use the _iproc driver 
and just drop the quirk flags known to be worked around by the firmware 
caps override.


>> +

>>   static const struct sdhci_iproc_data bcm2835_data = {

>>   	.pdata = &sdhci_bcm2835_pltfm_data,

>>   	.caps = ((0x1 << SDHCI_MAX_BLOCK_SHIFT)

>> @@ -261,6 +269,10 @@ static const struct sdhci_iproc_data bcm2835_data = {

>>   	.mmc_caps = 0x00000000,

>>   };

>>   

>> +static const struct sdhci_iproc_data bcm_arasan_data = {

>> +	.pdata = &sdhci_bcm_arasan_data,

>> +};

>> +

>>   static const struct sdhci_ops sdhci_iproc_bcm2711_ops = {

>>   	.read_l = sdhci_iproc_readl,

>>   	.read_w = sdhci_iproc_readw,

>> @@ -299,6 +311,8 @@ MODULE_DEVICE_TABLE(of, sdhci_iproc_of_match);

>>   static const struct acpi_device_id sdhci_iproc_acpi_ids[] = {

>>   	{ .id = "BRCM5871", .driver_data = (kernel_ulong_t)&iproc_cygnus_data },

>>   	{ .id = "BRCM5872", .driver_data = (kernel_ulong_t)&iproc_data },

>> +	{ .id = "BCM2847",  .driver_data = (kernel_ulong_t)&bcm_arasan_data },

> 

> Sorry, i don't have deeper knowledge about ACPI, but BCM2837 is the

> official naming of the SoC on the RPi 3.

> 

> Is this a typo in the id?


Not really.

Some background: The PFTF is basically the custodian of the combined 
rpi3 port done by Microsoft and a few other peoples/organizations ports. 
That merged code base was upstreamed a couple years ago to edk2 for the 
rpi3 and is the official port. On the rpi3+uefi platform, linux is just 
using DT, but windows and possibly other OSs are using the ACPI tables. 
For the Rpi4, the intentions is to be an ACPI first platform, but we are 
inheriting the rpi3 legacy peripheral descriptions. So, for the past 
year+ everyone has been basing their rpi4 ACPI OS ports on those tables 
and only adjusting them in backwards compatible ways.

Meaning, that a few years back someone put that ID in the rpi3 ACPI 
tables, and now we are stuck with it unless we are willing to break 
other OSs.


> 

>> +	{ .id = "BRCME88C", .driver_data = (kernel_ulong_t)&bcm2711_data },

>>   	{ /* sentinel */ }

>>   };

>>   MODULE_DEVICE_TABLE(acpi, sdhci_iproc_acpi_ids);

>
Ard Biesheuvel Jan. 11, 2021, 8:45 a.m. UTC | #6
On Mon, 11 Jan 2021 at 04:40, Jeremy Linton <jeremy.linton@arm.com> wrote:
>

> Hi,

>

> On 1/9/21 5:07 AM, Stefan Wahren wrote:

> > Hi Jeremy,

> >

> > +add Nicolas

> >

> > Am 08.01.21 um 22:13 schrieb Jeremy Linton:

> >> The rpi4 has a Arasan controller it carries over

...
> >> @@ -299,6 +311,8 @@ MODULE_DEVICE_TABLE(of, sdhci_iproc_of_match);

> >>   static const struct acpi_device_id sdhci_iproc_acpi_ids[] = {

> >>      { .id = "BRCM5871", .driver_data = (kernel_ulong_t)&iproc_cygnus_data },

> >>      { .id = "BRCM5872", .driver_data = (kernel_ulong_t)&iproc_data },

> >> +    { .id = "BCM2847",  .driver_data = (kernel_ulong_t)&bcm_arasan_data },

> >

> > Sorry, i don't have deeper knowledge about ACPI, but BCM2837 is the

> > official naming of the SoC on the RPi 3.

> >

> > Is this a typo in the id?

>

> Not really.

>

> Some background: The PFTF is basically the custodian of the combined

> rpi3 port done by Microsoft and a few other peoples/organizations ports.

> That merged code base was upstreamed a couple years ago to edk2 for the

> rpi3 and is the official port. On the rpi3+uefi platform, linux is just

> using DT, but windows and possibly other OSs are using the ACPI tables.

> For the Rpi4, the intentions is to be an ACPI first platform, but we are

> inheriting the rpi3 legacy peripheral descriptions.


I wouldn't say ACPI first - Linux will likely always have far better
DT coverage for these platforms, with DT overlays etc. However, there
is a strong pull from the industry to support Windows, VMware,
RHEL/Centos and the BSDs on these systems, which is why the ACPI
firmware port is important.

RPi4 is also the most easily obtained Linux/arm64 machine with a
proper and fairly complete implementation of standards-based rich
firmware, which is why it makes sense to support both ACPI and DT boot
on it in Linux.

> So, for the past

> year+ everyone has been basing their rpi4 ACPI OS ports on those tables

> and only adjusting them in backwards compatible ways.

>

> Meaning, that a few years back someone put that ID in the rpi3 ACPI

> tables, and now we are stuck with it unless we are willing to break

> other OSs.

>


Note that most of the ACPI tables were contributed by Microsoft in
order to boot Windows for IOT (or whatever it was called at the time)
on the RPi3; they weren't just pulled out of thin air.


>

> >

> >> +    { .id = "BRCME88C", .driver_data = (kernel_ulong_t)&bcm2711_data },

> >>      { /* sentinel */ }

> >>   };

> >>   MODULE_DEVICE_TABLE(acpi, sdhci_iproc_acpi_ids);

> >

>

>

> _______________________________________________

> linux-arm-kernel mailing list

> linux-arm-kernel@lists.infradead.org

> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Stefan Wahren Jan. 11, 2021, 12:23 p.m. UTC | #7
Hi,

Am 11.01.21 um 04:39 schrieb Jeremy Linton:
> Hi,

>

> On 1/9/21 5:07 AM, Stefan Wahren wrote:

>> Hi Jeremy,

>>

>> +add Nicolas

>>

>> Am 08.01.21 um 22:13 schrieb Jeremy Linton:

>>> The rpi4 has a Arasan controller it carries over

>>> from the rpi3, and a newer eMMC2 controller.

>>> Because of a couple "quirks" it seems wiser to bind

>>> these controllers to the same driver that DT is using

>>> on this platform rather than the generic sdhci_acpi

>>> driver with PNP0D40.

>>>

>>> So, we use BCM2847 for the older Arasan and BRCME88C

>>> for the newer eMMC2.

>>>

>>> With this change linux is capable of utilizing the

>>> SD card slot, and the wifi on this platform

>>> with linux.

>>>

>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

>>> ---

>>>   drivers/mmc/host/sdhci-iproc.c | 14 ++++++++++++++

>>>   1 file changed, 14 insertions(+)

>>>

>>> diff --git a/drivers/mmc/host/sdhci-iproc.c

>>> b/drivers/mmc/host/sdhci-iproc.c

>>> index c9434b461aab..f79d97b41805 100644

>>> --- a/drivers/mmc/host/sdhci-iproc.c

>>> +++ b/drivers/mmc/host/sdhci-iproc.c

>>> @@ -250,6 +250,14 @@ static const struct sdhci_pltfm_data

>>> sdhci_bcm2835_pltfm_data = {

>>>       .ops = &sdhci_iproc_32only_ops,

>>>   };

>>>   +static const struct sdhci_pltfm_data sdhci_bcm_arasan_data = {

>>> +    .quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION |

>>> +          SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK |

>>> +          SDHCI_QUIRK_NO_HISPD_BIT,

>>> +    .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,

>>> +    .ops = &sdhci_iproc_32only_ops,

>>> +};

>

> First, thanks for taking a look at this!

>

>

>> Why do we need an almost exact copy of bcm2835_data which works fine for

>> all Raspberry Pi boards?

>

> The short answer to the remainder of this email is that i'm trying to

> continue supporting existing OSs (windows) using the ACPI tables on

> the rpi3/rpi4 while adding rpi4+Linux support.

>

> An even shorter answer is they don't work because ACPI doesn't provide

> the same clock/attributes/etc controls that exist with DT.

>

> So, what happened here is that I got this controller "working" with

> the generic PNP0D40 sdhci_acpi driver. I managed this only by

> controlling the sdhci_caps/masks in the firmware. In theory this

> minimizes the amount of work needed on the other OS which are booting

> on the same ACPI tables (*bsds). They should only need to quirk the

> bcm/arasan specific functionality, rather than some of the quirking

> which change the caps behavior. But because we don't know which if any

> of the older rpi/arasan quirks are still needed the safest solution is

> to use the _iproc driver and just drop the quirk flags known to be

> worked around by the firmware caps override.


okay, thanks for the explanation. I was also confused by bcm_arasan,
because there is already an Arasan specific sdhci driver. But now it's
clear to me.

Could you please add a short comment (above sdhci_bcm_arasan_data) why
we cannot use bcm2835_data?

Btw the subject isn't complete. The patch is also related to the rpi3.

Best regards
Stefan
Jeremy Linton Jan. 11, 2021, 9:43 p.m. UTC | #8
Hi,

On 1/11/21 6:23 AM, Stefan Wahren wrote:
> Hi,

> 

> Am 11.01.21 um 04:39 schrieb Jeremy Linton:

>> Hi,

>>

>> On 1/9/21 5:07 AM, Stefan Wahren wrote:

>>> Hi Jeremy,

>>>

>>> +add Nicolas

>>>

>>> Am 08.01.21 um 22:13 schrieb Jeremy Linton:

>>>> The rpi4 has a Arasan controller it carries over

>>>> from the rpi3, and a newer eMMC2 controller.

>>>> Because of a couple "quirks" it seems wiser to bind

>>>> these controllers to the same driver that DT is using

>>>> on this platform rather than the generic sdhci_acpi

>>>> driver with PNP0D40.

>>>>

>>>> So, we use BCM2847 for the older Arasan and BRCME88C

>>>> for the newer eMMC2.

>>>>

>>>> With this change linux is capable of utilizing the

>>>> SD card slot, and the wifi on this platform

>>>> with linux.

>>>>

>>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

>>>> ---

>>>>    drivers/mmc/host/sdhci-iproc.c | 14 ++++++++++++++

>>>>    1 file changed, 14 insertions(+)

>>>>

>>>> diff --git a/drivers/mmc/host/sdhci-iproc.c

>>>> b/drivers/mmc/host/sdhci-iproc.c

>>>> index c9434b461aab..f79d97b41805 100644

>>>> --- a/drivers/mmc/host/sdhci-iproc.c

>>>> +++ b/drivers/mmc/host/sdhci-iproc.c

>>>> @@ -250,6 +250,14 @@ static const struct sdhci_pltfm_data

>>>> sdhci_bcm2835_pltfm_data = {

>>>>        .ops = &sdhci_iproc_32only_ops,

>>>>    };

>>>>    +static const struct sdhci_pltfm_data sdhci_bcm_arasan_data = {

>>>> +    .quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION |

>>>> +          SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK |

>>>> +          SDHCI_QUIRK_NO_HISPD_BIT,

>>>> +    .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,

>>>> +    .ops = &sdhci_iproc_32only_ops,

>>>> +};

>>

>> First, thanks for taking a look at this!

>>

>>

>>> Why do we need an almost exact copy of bcm2835_data which works fine for

>>> all Raspberry Pi boards?

>>

>> The short answer to the remainder of this email is that i'm trying to

>> continue supporting existing OSs (windows) using the ACPI tables on

>> the rpi3/rpi4 while adding rpi4+Linux support.

>>

>> An even shorter answer is they don't work because ACPI doesn't provide

>> the same clock/attributes/etc controls that exist with DT.

>>

>> So, what happened here is that I got this controller "working" with

>> the generic PNP0D40 sdhci_acpi driver. I managed this only by

>> controlling the sdhci_caps/masks in the firmware. In theory this

>> minimizes the amount of work needed on the other OS which are booting

>> on the same ACPI tables (*bsds). They should only need to quirk the

>> bcm/arasan specific functionality, rather than some of the quirking

>> which change the caps behavior. But because we don't know which if any

>> of the older rpi/arasan quirks are still needed the safest solution is

>> to use the _iproc driver and just drop the quirk flags known to be

>> worked around by the firmware caps override.

> 

> okay, thanks for the explanation. I was also confused by bcm_arasan,

> because there is already an Arasan specific sdhci driver. But now it's

> clear to me.

> 

> Could you please add a short comment (above sdhci_bcm_arasan_data) why

> we cannot use bcm2835_data?


Sure.

> 

> Btw the subject isn't complete. The patch is also related to the rpi3.


Only via the historical ACPI tables. There isn't any attempt to get 
ACPI+Linux working on the rpi3. Its to far away from BSA. For starters 
it doesn't have a GIC. So while one could bind this driver on the rpi3, 
that would require being able to boot Linux on the rpi3 in ACPI mode.

Thanks again!



> 

> Best regards

> Stefan

> 

>
diff mbox series

Patch

diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
index c9434b461aab..f79d97b41805 100644
--- a/drivers/mmc/host/sdhci-iproc.c
+++ b/drivers/mmc/host/sdhci-iproc.c
@@ -250,6 +250,14 @@  static const struct sdhci_pltfm_data sdhci_bcm2835_pltfm_data = {
 	.ops = &sdhci_iproc_32only_ops,
 };
 
+static const struct sdhci_pltfm_data sdhci_bcm_arasan_data = {
+	.quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION |
+		  SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK |
+		  SDHCI_QUIRK_NO_HISPD_BIT,
+	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
+	.ops = &sdhci_iproc_32only_ops,
+};
+
 static const struct sdhci_iproc_data bcm2835_data = {
 	.pdata = &sdhci_bcm2835_pltfm_data,
 	.caps = ((0x1 << SDHCI_MAX_BLOCK_SHIFT)
@@ -261,6 +269,10 @@  static const struct sdhci_iproc_data bcm2835_data = {
 	.mmc_caps = 0x00000000,
 };
 
+static const struct sdhci_iproc_data bcm_arasan_data = {
+	.pdata = &sdhci_bcm_arasan_data,
+};
+
 static const struct sdhci_ops sdhci_iproc_bcm2711_ops = {
 	.read_l = sdhci_iproc_readl,
 	.read_w = sdhci_iproc_readw,
@@ -299,6 +311,8 @@  MODULE_DEVICE_TABLE(of, sdhci_iproc_of_match);
 static const struct acpi_device_id sdhci_iproc_acpi_ids[] = {
 	{ .id = "BRCM5871", .driver_data = (kernel_ulong_t)&iproc_cygnus_data },
 	{ .id = "BRCM5872", .driver_data = (kernel_ulong_t)&iproc_data },
+	{ .id = "BCM2847",  .driver_data = (kernel_ulong_t)&bcm_arasan_data },
+	{ .id = "BRCME88C", .driver_data = (kernel_ulong_t)&bcm2711_data },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(acpi, sdhci_iproc_acpi_ids);