Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1331398 > unrolled thread

Re: [PATCH v5] mmc: OCTEON: Add host driver for OCTEON MMC controller

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5] mmc: OCTEON: Add host driver for OCTEON MMC controller David Daney <ddaney@caviumnetworks.com> - 2016-02-10 19:10 +0100
    Re: [PATCH v5] mmc: OCTEON: Add host driver for OCTEON MMC controller Aaro Koskinen <aaro.koskinen@iki.fi> - 2016-02-11 00:50 +0100
      Re: [PATCH v5] mmc: OCTEON: Add host driver for OCTEON MMC controller David Daney <ddaney@caviumnetworks.com> - 2016-02-11 01:40 +0100
        Re: [PATCH v5] mmc: OCTEON: Add host driver for OCTEON MMC controller Florian Fainelli <f.fainelli@gmail.com> - 2016-02-11 04:00 +0100
          Re: [PATCH v5] mmc: OCTEON: Add host driver for OCTEON MMC controller Matt Redfearn <matt.redfearn@imgtec.com> - 2016-02-11 09:20 +0100
        Re: [PATCH v5] mmc: OCTEON: Add host driver for OCTEON MMC  controller "Maciej W. Rozycki" <macro@imgtec.com> - 2016-02-11 20:30 +0100
    Re: [PATCH v5] mmc: OCTEON: Add host driver for OCTEON MMC controller Matt Redfearn <matt.redfearn@imgtec.com> - 2016-02-11 09:20 +0100

#1331398 — Re: [PATCH v5] mmc: OCTEON: Add host driver for OCTEON MMC controller

On 02/10/2016 09:36 AM, Matt Redfearn wrote:
> From: Aleksey Makarov<aleksey.makarov@caviumnetworks.com>
>
> The OCTEON MMC controller is currently found on cn61XX and cnf71XX
> devices.  Device parameters are configured from device tree data.
>
> eMMC, MMC and SD devices are supported.
>
> Tested-by: Aaro Koskinen<aaro.koskinen@iki.fi>
> Signed-off-by: Chandrakala Chavva<cchavva@caviumnetworks.com>
> Signed-off-by: David Daney<david.daney@cavium.com>
> Signed-off-by: Aleksey Makarov<aleksey.makarov@auriga.com>
> Signed-off-by: Leonid Rosenboim<lrosenboim@caviumnetworks.com>
> Signed-off-by: Peter Swain<pswain@cavium.com>
> Signed-off-by: Aaron Williams<aaron.williams@cavium.com>
> Signed-off-by: Matt Redfearn<matt.redfearn@imgtec.com>
> ---
> v5:
> Incoroprate comments from review
> http://patchwork.linux-mips.org/patch/9558/
> - Use standard <bus-width> property instead of <cavium,bus-max-width>.
> - Use standard <max-frequency> property instead of <spi-max-frequency>.
> - Add octeon_mmc_of_parse_legacy function to deal with the above
>    properties, since many devices have shipped with those properties
>    embedded in firmware.
> - Allow the <vmmc-supply> binding in addition to the legacy
>    <gpios-power>.
> - Remove the secondary driver for each slot.
> - Use core gpio cd/wp handling
>
[...]

> +static int octeon_mmc_of_copy_legacy_u32(struct device_node *node,
> +					  const char *legacy_name,
> +					  const char *new_name)
> +{
> +	u32 value;
> +	int ret;
> +
> +	ret = of_property_read_u32(node, legacy_name, &value);
> +	if (!ret) {
> +		/* Found legacy - set generic property */
> +		struct property *new_p;
> +		u32 *new_v;
> +
> +		pr_warn(FW_WARN "%s: Legacy property '%s'. Please remove\n",
> +			node->full_name, legacy_name);
> +

I don't like this warning message.

The vast majority of people that see it will not be able to change their 
firmware.  So it will be forever cluttering up their boot logs.

We are not ever planning on removing support for legacy firmware 
properties, so alarming people is really all this message does.

If you insist on a message then make it something like pr_info("This is 
working properly, but please consider using modern device tree 
properties...")

> +		new_p = kzalloc(sizeof(*new_p), GFP_KERNEL);
> +		new_v = kzalloc(sizeof(u32), GFP_KERNEL);
> +		if (!new_p || !new_v)
> +			return -ENOMEM;
> +
> +		*new_v = value;
> +		new_p->name = kstrdup(new_name, GFP_KERNEL);
> +		new_p->length = sizeof(u32);
> +		new_p->value = new_v;
> +
> +		of_update_property(node, new_p);
> +	}
> +	return 0;
> +}
> +
> +/*
> + * This function parses the legacy device tree that may be found in devices
> + * shipped before the driver was upstreamed. Future devices should not require
> + * it as standard bindings should be used
> + */
> +static int octeon_mmc_of_parse_legacy(struct device *dev,
> +				      struct device_node *node,
> +				      struct octeon_mmc_slot *slot)
> +{
> +	int ret;
> +
> +	ret = octeon_mmc_of_copy_legacy_u32(node, "cavium,bus-max-width",
> +					    "bus-width");
> +	if (ret)
> +		return ret;
> +
> +	ret = octeon_mmc_of_copy_legacy_u32(node, "spi-max-frequency",
> +					    "max-frequency");
> +	if (ret)
> +		return ret;
> +
> +	slot->pwr_gpiod = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW);
> +	if (!IS_ERR(slot->pwr_gpiod)) {
> +		pr_warn(FW_WARN "%s: Legacy property '%s'. Please remove\n",
> +			node->full_name, "gpios-power");
> +	}
> +
> +	return 0;
> +}
> +

[toc] | [next] | [standalone]

#1331588

Hi,

On Wed, Feb 10, 2016 at 10:02:23AM -0800, David Daney wrote:
> On 02/10/2016 09:36 AM, Matt Redfearn wrote:
> >+		pr_warn(FW_WARN "%s: Legacy property '%s'. Please remove\n",
> >+			node->full_name, legacy_name);
> 
> I don't like this warning message.
> 
> The vast majority of people that see it will not be able to change their
> firmware.  So it will be forever cluttering up their boot logs.

Until they switch to use APPENDED_DTB. :-)

A.

[toc] | [prev] | [next] | [standalone]

#1331608

On 02/10/2016 03:49 PM, Aaro Koskinen wrote:
> Hi,
>
> On Wed, Feb 10, 2016 at 10:02:23AM -0800, David Daney wrote:
>> On 02/10/2016 09:36 AM, Matt Redfearn wrote:
>>> +		pr_warn(FW_WARN "%s: Legacy property '%s'. Please remove\n",
>>> +			node->full_name, legacy_name);
>>
>> I don't like this warning message.
>>
>> The vast majority of people that see it will not be able to change their
>> firmware.  So it will be forever cluttering up their boot logs.
>
> Until they switch to use APPENDED_DTB. :-)
>

I am philosophically opposed to making the DTB an internal kernel 
implementation detail.

For OCTEON boards, it is an ABI between the boot firmware and the 
kernel, and is impractical to change.

One could argue that many years ago, when the decision was made (by me), 
that we should have opted to carry in the kernel source code tree the 
DTS files for all OCTEON boards ever made, but we did not do that.  Due 
to the non-reversibility of time, the decision is hard to reverse.

In the case of this MMC driver, the only real difference is that two 
properties have legacy names that later had differing "official" names. 
  The overhead of carrying the legacy bindings is very low.

David.

[toc] | [prev] | [next] | [standalone]

#1331662

Le 10/02/2016 16:32, David Daney a écrit :
> On 02/10/2016 03:49 PM, Aaro Koskinen wrote:
>> Hi,
>>
>> On Wed, Feb 10, 2016 at 10:02:23AM -0800, David Daney wrote:
>>> On 02/10/2016 09:36 AM, Matt Redfearn wrote:
>>>> +        pr_warn(FW_WARN "%s: Legacy property '%s'. Please remove\n",
>>>> +            node->full_name, legacy_name);
>>>
>>> I don't like this warning message.
>>>
>>> The vast majority of people that see it will not be able to change their
>>> firmware.  So it will be forever cluttering up their boot logs.
>>
>> Until they switch to use APPENDED_DTB. :-)
>>
> 
> I am philosophically opposed to making the DTB an internal kernel
> implementation detail.
> 
> For OCTEON boards, it is an ABI between the boot firmware and the
> kernel, and is impractical to change.
> 
> One could argue that many years ago, when the decision was made (by me),
> that we should have opted to carry in the kernel source code tree the
> DTS files for all OCTEON boards ever made, but we did not do that.  Due
> to the non-reversibility of time, the decision is hard to reverse.
> 
> In the case of this MMC driver, the only real difference is that two
> properties have legacy names that later had differing "official" names.
>  The overhead of carrying the legacy bindings is very low.

Since there is an existing FDT patching infrastructure in
arch/mips/cavium-octeon/ would not that be a place where you could put
an adaptation layer between your legacy firmware properties and the
upstream binding?
-- 
Florian

[toc] | [prev] | [next] | [standalone]

#1331722

Hi Florian.

On 11/02/16 02:55, Florian Fainelli wrote:
> Le 10/02/2016 16:32, David Daney a écrit :
>> On 02/10/2016 03:49 PM, Aaro Koskinen wrote:
>>> Hi,
>>>
>>> On Wed, Feb 10, 2016 at 10:02:23AM -0800, David Daney wrote:
>>>> On 02/10/2016 09:36 AM, Matt Redfearn wrote:
>>>>> +        pr_warn(FW_WARN "%s: Legacy property '%s'. Please remove\n",
>>>>> +            node->full_name, legacy_name);
>>>> I don't like this warning message.
>>>>
>>>> The vast majority of people that see it will not be able to change their
>>>> firmware.  So it will be forever cluttering up their boot logs.
>>> Until they switch to use APPENDED_DTB. :-)
>>>
>> I am philosophically opposed to making the DTB an internal kernel
>> implementation detail.
>>
>> For OCTEON boards, it is an ABI between the boot firmware and the
>> kernel, and is impractical to change.
>>
>> One could argue that many years ago, when the decision was made (by me),
>> that we should have opted to carry in the kernel source code tree the
>> DTS files for all OCTEON boards ever made, but we did not do that.  Due
>> to the non-reversibility of time, the decision is hard to reverse.
>>
>> In the case of this MMC driver, the only real difference is that two
>> properties have legacy names that later had differing "official" names.
>>   The overhead of carrying the legacy bindings is very low.
> Since there is an existing FDT patching infrastructure in
> arch/mips/cavium-octeon/ would not that be a place where you could put
> an adaptation layer between your legacy firmware properties and the
> upstream binding?
Thanks for your constructive advice. That does, indeed, look like a 
better place to put this.

Thanks,
Matt

[toc] | [prev] | [next] | [standalone]

#1332330 — Re: [PATCH v5] mmc: OCTEON: Add host driver for OCTEON MMC controller

On Thu, 11 Feb 2016, David Daney wrote:

> > > The vast majority of people that see it will not be able to change their
> > > firmware.  So it will be forever cluttering up their boot logs.
> > 
> > Until they switch to use APPENDED_DTB. :-)
> > 
> 
> I am philosophically opposed to making the DTB an internal kernel
> implementation detail.
> 
> For OCTEON boards, it is an ABI between the boot firmware and the kernel, and
> is impractical to change.
> 
> One could argue that many years ago, when the decision was made (by me), that
> we should have opted to carry in the kernel source code tree the DTS files for
> all OCTEON boards ever made, but we did not do that.  Due to the
> non-reversibility of time, the decision is hard to reverse.

 I concur, a very good decision as far as I'm concerned!

 I had the misfortune to work with some Freescale Power boards which used 
in-kernel DTS files in a hope to match the respective board's firmware 
(U-boot).  Needless to say, that didn't quite work.  The mapping of board 
resources was reportedly changed in some version of the firmware to give 
more flexibility and the DTS files bundled with Linux updated accordingly, 
however no version of the old files was kept around and maintained.  So a 
kernel upgrade, which turned out inevitable at one point, became a 
challenging task to update the DTS files so as to match the version of the 
firmware the boards had.

 With some pain I was eventually able to sort this out through patching 
the old DTS files to match the ever-changing DTS syntax and get them 
accepted for a DTB build and work to an acceptable extent with the then 
current version of Linux.  However some board resources were lost, for 
example the IDE interface was no longer accessible; fortunately I didn't 
need it, so I just left it like that and didn't figure out what else I'd 
have to do to regain access.

 So no, no standalone DTS/DTBs please, thank you very much.

  Maciej

[toc] | [prev] | [next] | [standalone]

#1331723

Hi David

On 10/02/16 18:02, David Daney wrote:
> On 02/10/2016 09:36 AM, Matt Redfearn wrote:
>> From: Aleksey Makarov<aleksey.makarov@caviumnetworks.com>
>>
>> The OCTEON MMC controller is currently found on cn61XX and cnf71XX
>> devices.  Device parameters are configured from device tree data.
>>
>> eMMC, MMC and SD devices are supported.
>>
>> Tested-by: Aaro Koskinen<aaro.koskinen@iki.fi>
>> Signed-off-by: Chandrakala Chavva<cchavva@caviumnetworks.com>
>> Signed-off-by: David Daney<david.daney@cavium.com>
>> Signed-off-by: Aleksey Makarov<aleksey.makarov@auriga.com>
>> Signed-off-by: Leonid Rosenboim<lrosenboim@caviumnetworks.com>
>> Signed-off-by: Peter Swain<pswain@cavium.com>
>> Signed-off-by: Aaron Williams<aaron.williams@cavium.com>
>> Signed-off-by: Matt Redfearn<matt.redfearn@imgtec.com>
>> ---
>> v5:
>> Incoroprate comments from review
>> http://patchwork.linux-mips.org/patch/9558/
>> - Use standard <bus-width> property instead of <cavium,bus-max-width>.
>> - Use standard <max-frequency> property instead of <spi-max-frequency>.
>> - Add octeon_mmc_of_parse_legacy function to deal with the above
>>    properties, since many devices have shipped with those properties
>>    embedded in firmware.
>> - Allow the <vmmc-supply> binding in addition to the legacy
>>    <gpios-power>.
>> - Remove the secondary driver for each slot.
>> - Use core gpio cd/wp handling
>>
> [...]
>
>> +static int octeon_mmc_of_copy_legacy_u32(struct device_node *node,
>> +                      const char *legacy_name,
>> +                      const char *new_name)
>> +{
>> +    u32 value;
>> +    int ret;
>> +
>> +    ret = of_property_read_u32(node, legacy_name, &value);
>> +    if (!ret) {
>> +        /* Found legacy - set generic property */
>> +        struct property *new_p;
>> +        u32 *new_v;
>> +
>> +        pr_warn(FW_WARN "%s: Legacy property '%s'. Please remove\n",
>> +            node->full_name, legacy_name);
>> +
>
> I don't like this warning message.
>
> The vast majority of people that see it will not be able to change 
> their firmware.  So it will be forever cluttering up their boot logs.
>
> We are not ever planning on removing support for legacy firmware 
> properties, so alarming people is really all this message does.
>
> If you insist on a message then make it something like pr_info("This 
> is working properly, but please consider using modern device tree 
> properties...")

Fair enough - I was just following what the PHY driver does when it 
encounters a whitelisted compatible string, e.g. when my board boots:

[Firmware Warn]: /soc@0/mdio@1180000001800/ethernet-phy@0: Whitelisted 
compatible string. Please remove
[Firmware Warn]: /soc@0/mdio@1180000001800/ethernet-phy@1: Whitelisted 
compatible string. Please remove
[Firmware Warn]: /soc@0/mdio@1180000001800/ethernet-phy@2: Whitelisted 
compatible string. Please remove
[Firmware Warn]: /soc@0/mdio@1180000001800/ethernet-phy@3: Whitelisted 
compatible string. Please remove
[Firmware Warn]: /soc@0/mdio@1180000001800/ethernet-phy@4: Whitelisted 
compatible string. Please remove
[Firmware Warn]: /soc@0/mdio@1180000001800/ethernet-phy@5: Whitelisted 
compatible string. Please remove
[Firmware Warn]: /soc@0/mdio@1180000001800/ethernet-phy@6: Whitelisted 
compatible string. Please remove
[Firmware Warn]: /soc@0/mdio@1180000001800/ethernet-phy@7: Whitelisted 
compatible string. Please remove

But I can see why it would be preferable to avoid this kind of message. 
I don't think it's essential so could be removed.

Thanks,
Matt
>
>> +        new_p = kzalloc(sizeof(*new_p), GFP_KERNEL);
>> +        new_v = kzalloc(sizeof(u32), GFP_KERNEL);
>> +        if (!new_p || !new_v)
>> +            return -ENOMEM;
>> +
>> +        *new_v = value;
>> +        new_p->name = kstrdup(new_name, GFP_KERNEL);
>> +        new_p->length = sizeof(u32);
>> +        new_p->value = new_v;
>> +
>> +        of_update_property(node, new_p);
>> +    }
>> +    return 0;
>> +}
>> +
>> +/*
>> + * This function parses the legacy device tree that may be found in 
>> devices
>> + * shipped before the driver was upstreamed. Future devices should 
>> not require
>> + * it as standard bindings should be used
>> + */
>> +static int octeon_mmc_of_parse_legacy(struct device *dev,
>> +                      struct device_node *node,
>> +                      struct octeon_mmc_slot *slot)
>> +{
>> +    int ret;
>> +
>> +    ret = octeon_mmc_of_copy_legacy_u32(node, "cavium,bus-max-width",
>> +                        "bus-width");
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = octeon_mmc_of_copy_legacy_u32(node, "spi-max-frequency",
>> +                        "max-frequency");
>> +    if (ret)
>> +        return ret;
>> +
>> +    slot->pwr_gpiod = devm_gpiod_get_optional(dev, "power", 
>> GPIOD_OUT_LOW);
>> +    if (!IS_ERR(slot->pwr_gpiod)) {
>> +        pr_warn(FW_WARN "%s: Legacy property '%s'. Please remove\n",
>> +            node->full_name, "gpios-power");
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web