Re: [PATCH v2 0/6] UHS-I support for sh_mobile_sdhi

[Ben Hutchings <ben.hutchings@codethink.co.uk>]

On Thu, 2015-06-11 at 11:49 +0900, Simon Horman wrote:
> On Thu, Jun 11, 2015 at 12:57:57AM +0100, Ben Hutchings wrote:
> > On Wed, 2015-06-10 at 11:16 +0200, Ulf Hansson wrote:
> > > On 10 June 2015 at 01:21, Ben Hutchings <ben.hutchings@codethink.co.uk> wrote:
> > > > This series adds support for UHS-I in sh_mobile_sdhi, partly implemented
> > > > in tmio_mmc.  This does not yet include tuning for SDR-104, but SDR-50 now
> > > > works on the R8A7790 Lager board and another development board.
> > > >
> > > > The pfc block needs to be reconfigured from 3.3V to 1.8V signalling on
> > > > the pins wired to the SD card.  This is supported by adding separate
> > > > functions for 1.8V signalling in sh-pfc ("sdhi0_1v8" etc.).  I expect
> > > > that several SH SoCs have this capability, but I only have the R8A7790
> > > > data sheet so I only implemented it for that one.
> > > >
> > > > Changes since v1:
> > > > - Reword commit message for "mmc: tmio: Add UHS-I mode support"
> > > > - Make sh_mobile_sdhi_start_signal_voltage_switch() succeed if asked
> > > >   to switch to 3.3V and the regulator or pinctrl or pinctrl state is
> > > >   missing
> > > > - Drop change to mmcif clock on Lager
> > > > - Correct original author for sdhi clock changes on Lager
> > > >
> > > > Changes since the RFC:
> > > > - Replace the 'regulator' devices for signal voltage switching with
> > > >   pinctrl functions and states
> > > > - Drop 'mmc: sh_mobile_sdhi: Add actual clock rate support' as it's
> > > >   redundant
> > > > - Use a switch statement in sh_mobile_sdhi_start_signal_voltage_switch()
> > > > - Fix subject prefix for the DT changes
> > > >
> > > > Ben.
> > > >
> > > > Ben Hutchings (5):
> > > >   mmc: tmio: Add UHS-I mode support
> > > >   pinctrl: sh-pfc: Add set_mux operation to struct sh_pfc_function
> > > >   pinctrl: sh-pfc: r8a7790: Add separate functions for SDHI 1.8V
> > > >     operation
> > > >   mmc: sh_mobile_sdhi: Add UHS-I mode support
> > > >   ARM: shmobile: lager: Enable UHS-I SDR-50
> > > >
> > > > Ian Molton (1):
> > > >   ARM: shmobile: lager: Set clock rates for SDHI
> > > >
> > > >  arch/arm/boot/dts/r8a7790-lager.dts  | 24 +++++++++++--
> > > >  drivers/mmc/host/sh_mobile_sdhi.c    | 60 +++++++++++++++++++++++++++++++
> > > >  drivers/mmc/host/tmio_mmc.h          |  3 ++
> > > >  drivers/mmc/host/tmio_mmc_pio.c      | 31 ++++++++++++++++
> > > >  drivers/pinctrl/sh-pfc/core.c        |  2 +-
> > > >  drivers/pinctrl/sh-pfc/core.h        |  1 +
> > > >  drivers/pinctrl/sh-pfc/pfc-r8a7790.c | 70 +++++++++++++++++++++++++++++++++---
> > > >  drivers/pinctrl/sh-pfc/pinctrl.c     |  4 +++
> > > >  drivers/pinctrl/sh-pfc/sh_pfc.h      | 10 +++++-
> > > >  9 files changed, 197 insertions(+), 8 deletions(-)
> > > >
> > > > --
> > > > 2.1.4
> > > >
> > > 
> > > Hi Ben,
> > > 
> > > I have looked at the mmc patches, those looks good to me. Regarding
> > > the pinctrl and ARM patches, I suppose these can be taken through
> > > their respective trees and I can take the mmc patches?
> > 
> > The problem with that is that I think the device tree change will cause
> > a regression if it's applied without the driver changes.  I would much
> > prefer if I could get the pinctrl and device tree changes acked by the
> > respective maintainers to go through the MMC tree.  (I should probably
> > have said that up front.)
> 
> Hi Ben,
> 
> I may be misunderstanding the above, if so I apologise, but I would
> strongly prefer to avoid an arrangement where the kernel and device tree
> blob (DTS/DTSI -> DTB) need to be upgrade in lock-step as this tends not to
> lead to a good experience for users.

I agree.

> My preference would be to maintain
> backwards and forwards compatibility and if appropriate schedule removal of
> such compatibility.
[...]

The problem is that the 'sd-uhs-sdr50' property is interpreted by the
MMC core, so I think it will start trying to use this mode even if the
driver hasn't implemented the necessary operations.  I don't see any way
around that.

The regression is relatively minor as I think the MMC core will fall
back to a lower speed after failing to enable SDR50.  But it will slow
down probing of a card.

Ben.