Re: [PATCH v7 08/15] ahci-imx: Port to library-ised ahci_platform

[Russell King - ARM Linux <linux@arm.linux.org.uk>]

I've moved the devicetree mailing list to the To: header in case anyone
there wants to comment on this.

On Sat, Mar 01, 2014 at 11:38:32AM +0100, Hans de Goede wrote:
> I'm sure Tejun would welcome patches to add a dts property for
> setting the board-specific phy parameters, but I won't be
> writing it.

Don't worry, I already have something :)

>> I'm in two minds about this - whether to make the existing binding
>> with its compatible string always use these settings, and invent a
>> new compatible string for one which is fully configurable (as it
>> _should_ have been from the very start) or whether to make this
>> the default if the various properties aren't specified.
>
> IMHO this does not warrant doing a new compatibole-string. Simply
> default to the current hardcoded phy settings if no settings are
> specified through devicetree.

The problem is that we need to keep existing setups working - which
means when there's no properties specified, we need to default to the
settings hard-coded into the driver.

That means introducing properties like:

	fsl,no-spread-spectrum

so that the hard-coded defaults can be turned off, and also deal with
a whole load of defaults for the other properties.  That's not
particularly nice.  Doing it this way, what I currently have is this:

struct reg_value {
        u32 of_value;
        u32 reg_value;
};

struct reg_property {
        const char *name;
        const struct reg_value *values;
        size_t num_values;
        u32 def_value;
        u32 set_value;
};

static const struct reg_value gpr13_tx_level[] = {
        {  937, IMX6Q_GPR13_SATA_TX_LVL_0_937_V },
        {  947, IMX6Q_GPR13_SATA_TX_LVL_0_947_V },
...
        { 1230, IMX6Q_GPR13_SATA_TX_LVL_1_230_V },
        { 1240, IMX6Q_GPR13_SATA_TX_LVL_1_240_V }
};

static const struct reg_value gpr13_tx_boost[] = {
        {    0, IMX6Q_GPR13_SATA_TX_BOOST_0_00_DB },
        {  370, IMX6Q_GPR13_SATA_TX_BOOST_0_37_DB },
...
        {  528, IMX6Q_GPR13_SATA_TX_BOOST_5_28_DB },
        {  575, IMX6Q_GPR13_SATA_TX_BOOST_5_75_DB }
};

static const struct reg_value gpr13_tx_atten[] = {
        {  8, IMX6Q_GPR13_SATA_TX_ATTEN_8_16 },
        {  9, IMX6Q_GPR13_SATA_TX_ATTEN_9_16 },
...
        { 14, IMX6Q_GPR13_SATA_TX_ATTEN_14_16 },
        { 16, IMX6Q_GPR13_SATA_TX_ATTEN_16_16 },
};

static const struct reg_value gpr13_rx_eq[] = {
        {  500, IMX6Q_GPR13_SATA_RX_EQ_VAL_0_5_DB },
        { 1000, IMX6Q_GPR13_SATA_RX_EQ_VAL_1_0_DB },
...
        { 3500, IMX6Q_GPR13_SATA_RX_EQ_VAL_3_5_DB },
        { 4000, IMX6Q_GPR13_SATA_RX_EQ_VAL_4_0_DB },
};

static const struct reg_property gpr13_props[] = {
        {
                .name = "fsl,transmit-level-mV",
                .values = gpr13_tx_level,
                .num_values = ARRAY_SIZE(gpr13_tx_level),
                .def_value = IMX6Q_GPR13_SATA_TX_LVL_1_025_V,
        }, {
                .name = "fsl,transmit-boost-mdB",
                .values = gpr13_tx_boost,
                .num_values = ARRAY_SIZE(gpr13_tx_boost),
                .def_value = IMX6Q_GPR13_SATA_TX_BOOST_3_33_DB,
        }, {
                .name = "fsl,transmit-atten-16ths",
                .values = gpr13_tx_atten,
                .num_values = ARRAY_SIZE(gpr13_tx_atten),
                .def_value = IMX6Q_GPR13_SATA_TX_ATTEN_9_16,
        }, {
                .name = "fsl,receive-eq-mdB",
                .values = gpr13_rx_eq,
                .num_values = ARRAY_SIZE(gpr13_rx_eq),
                .def_value = IMX6Q_GPR13_SATA_RX_EQ_VAL_3_0_DB,
        }, {
                .name = "fsl,no-spread-spectrum",
                .def_value = IMX6Q_GPR13_SATA_MPLL_SS_EN,
                .set_value = 0,
        },
};

and then a bunch of code to read through these tables and work out the
GPR13 register value from it - it doesn't handle everything yet because
I've not worked out a good way to do the last remaining three - I'm
thinking that they want to be strings:

                regmap_update_bits(imxpriv->gpr, IOMUXC_GPR13,
                                   IMX6Q_GPR13_SATA_RX_EQ_VAL_MASK |
...
                                   IMX6Q_GPR13_SATA_TX_EDGE_RATE,
                                   IMX6Q_GPR13_SATA_RX_LOS_LVL_SATA2M |
                                   IMX6Q_GPR13_SATA_RX_DPLL_MODE_2P_4F |
                                   IMX6Q_GPR13_SATA_SPD_MODE_3P0G |
                                   reg_value);

	RX_LOS_LVL: SATA1I / SATA1M / SATA1X / SATA2I / SATA2M / SATA2X
	RX_DPLL_MODE: 1P_1F / 2P_2F / 1P_4F / 2P_4F
	SPD_MODE: 3P0G / 1P5G (I do wonder whether this should be changed
		when the Linux wants to slow the link speed.)

With a new compatible string, we can use the hard-coded version for
fsl,imx6q-ahci, but require all properties (with values) to be specified
for a different compatible string, thereby eliminating all the defaults,
and making things like "no-spread-spectrum" be a positive property instead
of negative, and this simplifies the parsing code.

There's also the obvious question about which of these properties should
be generic ones for AHCI/SATA interfaces...  The only one I see with any
kind of electrical properties specified is sata_highbank:

- calxeda,tx-atten  : a u32 array that contains TX attenuation override
                        codes, one per port. The upper 3 bytes are always
                        0 and thus ignored.

and that seems to be a register-coded value rather than an actual
attenuation figure.

A simpler solution to this would be to just provide an imx6-specific
property which encodes the GPR13 value directly, and not bother with
trying to provide individual properties.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.