Re: Adding a .platform_init callback to sdhci_arasan_ops

[Sebastian Frias]

Hi Doug,

On 28/11/16 19:02, Doug Anderson wrote:
> Hi,
> 
> On Mon, Nov 28, 2016 at 5:28 AM, Sebastian Frias <sf84@xxxxxxxxxxx> wrote:
>> +static void sdhci_tango4_platform_init(struct sdhci_host *host)
>> +{
>> +       printk("%s\n", __func__);
>> +
>> +       /*
>> +         pad_mode[2:0]=0    must be 0
>> +         sel_sdio[3]=1      must be 1 for SDIO
>> +         inv_sdwp_pol[4]=0  if set inverts the SD write protect polarity
>> +         inv_sdcd_pol[5]=0  if set inverts the SD card present polarity
>> +       */
>> +       sdhci_writel(host, 0x00000008, 0x100 + 0x0);
> 
> If I were doing this, I'd probably actually add these fields to the
> "sdhci_arasan_soc_ctl_map", then add a 3rd option to
> sdhci_arasan_syscon_write().  Right now it has 2 modes: "hiword
> update" and "non-hiword update".  You could add a 3rd indicating that
> you set config registers by just writing at some offset of the main
> SDHCI registers space.
> 
> So you'd add 4 fields:
> 
> .tango_pad_mode = { .reg = 0x0000, .width = 3, .shift = 0 },
> .sel_sdio = { .reg = 0x0000, .width = 1, .shift = 3},
> .inv_sdwp_pol = { .reg = 0x0000, .width = 1, .shift = 4},
> .inv_sdcd_pol = { .reg = 0x0000, .width = 1, .shift = 5},

Right now I'm using something like:

+       val = 0;
+       val |= PAD_MODE(0); /* must be 0 */
+       val |= SEL_SDIO;    /* enable SDIO */
+       sdhci_writel(host, val, 0x100 + 0x0);
+
+       val = 0;
+       val |= TIMEOUT_CLK_UNIT_MHZ;         /* unit: MHz */
+       val |= TIMEOUT_CLK_FREQ(52);         /* timeout clock: 52MHz */
+       val |= BUS_CLK_FREQ_FOR_SD_CLK(200); /* SDIO clock: 200MHz (TODO: get from DT) */
+       val |= MAX_BLOCK_LENGTH(3);          /* max block size: 4096 bytes */
+       val |= EXTENDED_MEDIA_BUS_SUPPORTED; /* DT? */
+       val |= ADMA2_SUPPORTED;              /* DT? */
+       val |= HIGH_SPEED_SUPPORTED;         /* DT? */
+       val |= SDMA_SUPPORTED;               /* DT? */
+       val |= SUSPEND_RESUME_SUPPORTED;     /* DT? */
+       val |= VOLTAGE_3_3_V_SUPPORTED;      /* DT? */
+#if 0
+       val |= VOLTAGE_1_8_V_SUPPORTED;      /* DT? */
+#endif
+       val |= ASYNCHRONOUS_IRQ_SUPPORTED;   /* DT? */
+       val |= SLOT_TYPE_REMOVABLE;          /* DT? */
+       val |= SDR50_SUPPORTED;              /* DT? */
+       sdhci_writel(host, val, 0x100 + 0x40);
+
+       val = 0;
+       val |= TIMER_COUNT_FOR_RETUNING(1);  /* DT? */
+       val |= CLOCK_MULTIPLIER(0);          /* DT? */
+       val |= SPI_MODE_SUPPORTED;           /* DT? */
+       val |= SPI_BLOCK_MODE_SUPPORTED;     /* DT? */
+       sdhci_writel(host, val, 0x100 + 0x44);
+
+       sdhci_writel(host, 0x0004022c, 0x100 + 0x28);
+       sdhci_writel(host, 0x00000002, 0x100 + 0x2c);
+
+       sdhci_writel(host, 0x00600000, 0x100 + 0x50);

which seems much easier to handle (and portable).

At any rate, one thing to note from this is that many of these
bits should probably be initialised based on DT, right?

For example, the DT has a "non-removable" property, which I think
should be used to setup SLOT_TYPE_EMBEDDED (if the property is
present) or SLOT_TYPE_REMOVABLE (if the property is not present)

Looking at Documentation/devicetree/bindings/mmc/mmc.txt we can see
more related properties:

Optional properties:
- bus-width: Number of data lines, can be <1>, <4>, or <8>.  The default
  will be <1> if the property is absent.
- wp-gpios: Specify GPIOs for write protection, see gpio binding
- cd-inverted: when present, polarity on the CD line is inverted. See the note
  below for the case, when a GPIO is used for the CD line
- wp-inverted: when present, polarity on the WP line is inverted. See the note
  below for the case, when a GPIO is used for the WP line
- disable-wp: When set no physical WP line is present. This property should
  only be specified when the controller has a dedicated write-protect
  detection logic. If a GPIO is always used for the write-protect detection
  logic it is sufficient to not specify wp-gpios property in the absence of a WP
  line.
- max-frequency: maximum operating clock frequency
- no-1-8-v: when present, denotes that 1.8v card voltage is not supported on
  this system, even if the controller claims it is.
- cap-sd-highspeed: SD high-speed timing is supported
- cap-mmc-highspeed: MMC high-speed timing is supported
- sd-uhs-sdr12: SD UHS SDR12 speed is supported
- sd-uhs-sdr25: SD UHS SDR25 speed is supported
- sd-uhs-sdr50: SD UHS SDR50 speed is supported
- sd-uhs-sdr104: SD UHS SDR104 speed is supported
- sd-uhs-ddr50: SD UHS DDR50 speed is supported
...

which makes me wonder, what is the recommended way of dealing with this?
- Should I use properties on the DT? If so, then I need to add code to set
up the register properly.
- Should I hardcode these values use a minimal DT? If so, then I need an
init function to put all this.
- Should I hardcode stuff at u-Boot level? If so, nothing is required and
should work without any modifications to the Arasan Linux driver.

It appears that the Linux driver is expecting most of these fields to be
hardcoded and "pre-set" before (maybe by the bootloader) it starts, hence
the lack of any "init" function so far.

> 
> In your platform-specific init you're proposing you could set
> tango_pad_mode to 0.  That seems tango-specific.
> 
> You'd want to hook into "set_ios" for setting sel_sdio or not.  That's
> important if anyone ever wants to plug in an external SDIO card to
> your slot.  This one good argument for putting this in
> sdhci_arasan_soc_ctl_map, since you wouldn't want to do a
> compatibility matching every time set_ios is called.

Thanks for the advice, I will look into that.

> 
> I'd have to look more into the whole SD/WP polarity issue.  There are
> already so many levels of inversion for these signals in Linux that
> it's confusing.  It seems like you might just want to hardcode them to
> "0" and let users use all the existing ways to invert things...  You
> could either put that hardcoding into your platform init code or (if
> you're using sdhci_arasan_soc_ctl_map) put it in the main init code so
> that if anyone else needs to init similar signals then they can take
> advantage of it.

Yes, I think I will leave them to 0.

> 
> --
> 
> One random side note is that as currently documented you need to
> specify a "shift" of -1 for any sdhci_arasan_soc_ctl_map fields you
> aren't using.  That seems stupid--not sure why I did that.  It seems
> better to clue off "width = 0" so that you could just freely not init
> any fields you don't need.

I see.
So far I'm not really convinced about using "soc_ctl_map" because what I
have so far is more portable, and can easily be put as is somewhere else
(i.e.: in different flavors of bootloaders)

Best regards,

Sebastian