ARC dw-mshc binding compat string

Hi!

I noticed that arch/arc/boot/dts/axs10x_mb.dtsi uses "altr," prefix in the DT compatible string:

mmc@0x15000 {
        compatible = "altr,socfpga-dw-mshc";
        reg = < 0x15000 0x400 >;
        num-slots = < 1 >;
        fifo-depth = < 16 >;
        card-detect-delay = < 200 >;
        clocks = <&apbclk>, <&mmcclk>;
        clock-names = "biu", "ciu";
        interrupts = < 7 >;
        bus-width = < 4 >;
};

I don't think this is OK, since ARC is unrelated to Altera, which is what the "altr," prefix stands for. I think the socfpga-dw-mshc shim should be extended with another compatibility string, something like "snps,arc-dw-mshc" and the axs10x_mb.dtsi should be adjusted accordingly. What do you think ?

Quoted 17 lines · Marek Vasut

Hi!

I noticed that arch/arc/boot/dts/axs10x_mb.dtsi uses "altr," prefix in the DT compatible string:

mmc@0x15000 {
        compatible = "altr,socfpga-dw-mshc";
        reg = < 0x15000 0x400 >;
        num-slots = < 1 >;
        fifo-depth = < 16 >;
        card-detect-delay = < 200 >;
        clocks = <&apbclk>, <&mmcclk>;
        clock-names = "biu", "ciu";
        interrupts = < 7 >;
        bus-width = < 4 >;
};

Quoted 6 lines

I don't think this is OK, since ARC is unrelated to Altera, which is what the "altr," prefix stands for. I think the socfpga-dw-mshc shim should be extended with another compatibility string, something like "snps,arc-dw-mshc" and the axs10x_mb.dtsi should be adjusted accordingly. What do you think ?

There is "snps,dw-mshc" described in Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt and supported by dw_mmc host controller driver.

On 03/26/2016 06:26 PM, Vlad

Quoted 17 lines · Marek Vasut

Hi!

I noticed that arch/arc/boot/dts/axs10x_mb.dtsi uses "altr," prefix in the DT compatible string:

mmc@0x15000 {
        compatible = "altr,socfpga-dw-mshc";
        reg = < 0x15000 0x400 >;
        num-slots = < 1 >;
        fifo-depth = < 16 >;
        card-detect-delay = < 200 >;
        clocks = <&apbclk>, <&mmcclk>;
        clock-names = "biu", "ciu";
        interrupts = < 7 >;
        bus-width = < 4 >;
};

Quoted 6 lines

I don't think this is OK, since ARC is unrelated to Altera, which is what the "altr," prefix stands for. I think the socfpga-dw-mshc shim should be extended with another compatibility string, something like "snps,arc-dw-mshc" and the axs10x_mb.dtsi should be adjusted accordingly. What do you think ?

There is "snps,dw-mshc" described in Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt and supported by dw_mmc host controller driver.

Quoted 25 lines · Vladimir Zapolskiy

Quoted 23 lines · Marek Vasut

Hi!

I noticed that arch/arc/boot/dts/axs10x_mb.dtsi uses "altr," prefix in the DT compatible string:

mmc@0x15000 {
        compatible = "altr,socfpga-dw-mshc";
        reg = < 0x15000 0x400 >;
        num-slots = < 1 >;
        fifo-depth = < 16 >;
        card-detect-delay = < 200 >;
        clocks = <&apbclk>, <&mmcclk>;
        clock-names = "biu", "ciu";
        interrupts = < 7 >;
        bus-width = < 4 >;
};

I don't think this is OK, since ARC is unrelated to Altera, which is what the "altr," prefix stands for. I think the socfpga-dw-mshc shim should be extended with another compatibility string, something like "snps,arc-dw-mshc" and the axs10x_mb.dtsi should be adjusted accordingly. What do you think ?

Quoted 3 lines

There is "snps,dw-mshc" described in Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt and supported by dw_mmc host controller driver.

Thanks, that's even better.

btw what do you think of using altr, prefix on non-altera system, that doesn't seem ok, right ?

Hi Marek,

Quoted 32 lines · Marek Vasut

Quoted 25 lines · Vladimir Zapolskiy

Quoted 17 lines · Marek Vasut

Hi!

I noticed that arch/arc/boot/dts/axs10x_mb.dtsi uses "altr," prefix in the DT compatible string:

mmc@0x15000 {
        compatible = "altr,socfpga-dw-mshc";
        reg = < 0x15000 0x400 >;
        num-slots = < 1 >;
        fifo-depth = < 16 >;
        card-detect-delay = < 200 >;
        clocks = <&apbclk>, <&mmcclk>;
        clock-names = "biu", "ciu";
        interrupts = < 7 >;
        bus-width = < 4 >;
};

Quoted 6 lines

I don't think this is OK, since ARC is unrelated to Altera, which is what the "altr," prefix stands for. I think the socfpga-dw-mshc shim should be extended with another compatibility string, something like "snps,arc-dw-mshc" and the axs10x_mb.dtsi should be adjusted accordingly. What do you think ?

Quoted 3 lines

There is "snps,dw-mshc" described in Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt and supported by dw_mmc host controller driver.

Thanks, that's even better.

Quoted 2 lines

btw what do you think of using altr, prefix on non-altera system, that doesn't seem ok, right ?

according to ePAPR the prefix should represent a device (IP block here I believe) manufacturer, so it should be okay to use "altr" prefix on non-Altera system, if Altera provides another hardware vendor with some own IP block.

That said, I would rather prefer to see "snps,dw-mshc" prefix on description of an MMC controller found on SoCFPGA series, "altr,socfpga-dw-mshc" seems to be redundant.

--
With best wishes,
Vladimir

Quoted 38 lines · Vladimir Zapolskiy

Hi Marek,

Quoted 34 lines · Marek Vasut

Quoted 25 lines · Vladimir Zapolskiy

Quoted 17 lines · Marek Vasut

Hi!

I noticed that arch/arc/boot/dts/axs10x_mb.dtsi uses "altr," prefix in the DT compatible string:

mmc@0x15000 {
        compatible = "altr,socfpga-dw-mshc";
        reg = < 0x15000 0x400 >;
        num-slots = < 1 >;
        fifo-depth = < 16 >;
        card-detect-delay = < 200 >;
        clocks = <&apbclk>, <&mmcclk>;
        clock-names = "biu", "ciu";
        interrupts = < 7 >;
        bus-width = < 4 >;
};

Quoted 6 lines

I don't think this is OK, since ARC is unrelated to Altera, which is what the "altr," prefix stands for. I think the socfpga-dw-mshc shim should be extended with another compatibility string, something like "snps,arc-dw-mshc" and the axs10x_mb.dtsi should be adjusted accordingly. What do you think ?

Quoted 3 lines

There is "snps,dw-mshc" described in Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt and supported by dw_mmc host controller driver.

Thanks, that's even better.

btw what do you think of using altr, prefix on non-altera system, that doesn't seem ok, right ?

Quoted 4 lines

according to ePAPR the prefix should represent a device (IP block here I believe) manufacturer, so it should be okay to use "altr" prefix on non-Altera system, if Altera provides another hardware vendor with some own IP block.

In this case, it's Synopsys who provides the SD/MMC/MS core to other chip makers (Altera etc).

Quoted 3 lines

That said, I would rather prefer to see "snps,dw-mshc" prefix on description of an MMC controller found on SoCFPGA series, "altr,socfpga-dw-mshc" seems to be redundant.

According to drivers/mmc/host/dw_mmc-pltfm.c , the Altera SoCFPGA one "altr,socfpga-dw-mshc" and also Imagination Technology Pistacio one "img,pistachio-dw-mshc" need specialty bit (SDMMC_CMD_USE_HOLD_REG), while the stock one "snps,dw-mshc" does not. I am not sure if the ARC one needs it as well, but most likely yes.

I wonder if that bit is needed on some particular version of the DWMMC core. In that case, should we have "snps,dw-mshc" and "snps,dw-mshc-vN" binding ? Or should we use DT property to discern the need for this bit ?

Quoted 39 lines · Marek Vasut

Quoted 38 lines · Vladimir Zapolskiy

Hi Marek,

Quoted 34 lines · Marek Vasut

Quoted 25 lines · Vladimir Zapolskiy

Quoted 17 lines · Marek Vasut

Hi!

I noticed that arch/arc/boot/dts/axs10x_mb.dtsi uses "altr," prefix in the DT compatible string:

mmc@0x15000 {
        compatible = "altr,socfpga-dw-mshc";
        reg = < 0x15000 0x400 >;
        num-slots = < 1 >;
        fifo-depth = < 16 >;
        card-detect-delay = < 200 >;
        clocks = <&apbclk>, <&mmcclk>;
        clock-names = "biu", "ciu";
        interrupts = < 7 >;
        bus-width = < 4 >;
};

Quoted 6 lines

I don't think this is OK, since ARC is unrelated to Altera, which is what the "altr," prefix stands for. I think the socfpga-dw-mshc shim should be extended with another compatibility string, something like "snps,arc-dw-mshc" and the axs10x_mb.dtsi should be adjusted accordingly. What do you think ?

Quoted 3 lines

There is "snps,dw-mshc" described in Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt and supported by dw_mmc host controller driver.

Thanks, that's even better.

btw what do you think of using altr, prefix on non-altera system, that doesn't seem ok, right ?

Quoted 7 lines

Quoted 4 lines

according to ePAPR the prefix should represent a device (IP block here I believe) manufacturer, so it should be okay to use "altr" prefix on non-Altera system, if Altera provides another hardware vendor with some own IP block.

In this case, it's Synopsys who provides the SD/MMC/MS core to other chip makers (Altera etc).

Correct.

Quoted 10 lines

Quoted 3 lines

That said, I would rather prefer to see "snps,dw-mshc" prefix on description of an MMC controller found on SoCFPGA series, "altr,socfpga-dw-mshc" seems to be redundant.

According to drivers/mmc/host/dw_mmc-pltfm.c , the Altera SoCFPGA one "altr,socfpga-dw-mshc" and also Imagination Technology Pistacio one "img,pistachio-dw-mshc" need specialty bit (SDMMC_CMD_USE_HOLD_REG), while the stock one "snps,dw-mshc" does not. I am not sure if the ARC one needs it as well, but most likely yes.

Quoted 4 lines

I wonder if that bit is needed on some particular version of the DWMMC core. In that case, should we have "snps,dw-mshc" and "snps,dw-mshc-vN" binding ? Or should we use DT property to discern the need for this bit ?

That's the most common way to take into account peculiarities, add a property and handle it from the driver.

Quoted 65 lines · Vladimir Zapolskiy

Quoted 39 lines · Marek Vasut

Quoted 38 lines · Vladimir Zapolskiy

Hi Marek,

Quoted 34 lines · Marek Vasut

Quoted 25 lines · Vladimir Zapolskiy

Quoted 17 lines · Marek Vasut

Hi!

I noticed that arch/arc/boot/dts/axs10x_mb.dtsi uses "altr," prefix in the DT compatible string:

mmc@0x15000 {
        compatible = "altr,socfpga-dw-mshc";
        reg = < 0x15000 0x400 >;
        num-slots = < 1 >;
        fifo-depth = < 16 >;
        card-detect-delay = < 200 >;
        clocks = <&apbclk>, <&mmcclk>;
        clock-names = "biu", "ciu";
        interrupts = < 7 >;
        bus-width = < 4 >;
};

Quoted 6 lines

I don't think this is OK, since ARC is unrelated to Altera, which is what the "altr," prefix stands for. I think the socfpga-dw-mshc shim should be extended with another compatibility string, something like "snps,arc-dw-mshc" and the axs10x_mb.dtsi should be adjusted accordingly. What do you think ?

Quoted 3 lines

There is "snps,dw-mshc" described in Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt and supported by dw_mmc host controller driver.

Thanks, that's even better.

btw what do you think of using altr, prefix on non-altera system, that doesn't seem ok, right ?

Quoted 7 lines

Quoted 4 lines

according to ePAPR the prefix should represent a device (IP block here I believe) manufacturer, so it should be okay to use "altr" prefix on non-Altera system, if Altera provides another hardware vendor with some own IP block.

In this case, it's Synopsys who provides the SD/MMC/MS core to other chip makers (Altera etc).

Correct.

Quoted 14 lines

Quoted 3 lines

That said, I would rather prefer to see "snps,dw-mshc" prefix on description of an MMC controller found on SoCFPGA series, "altr,socfpga-dw-mshc" seems to be redundant.

According to drivers/mmc/host/dw_mmc-pltfm.c , the Altera SoCFPGA one "altr,socfpga-dw-mshc" and also Imagination Technology Pistacio one "img,pistachio-dw-mshc" need specialty bit (SDMMC_CMD_USE_HOLD_REG), while the stock one "snps,dw-mshc" does not. I am not sure if the ARC one needs it as well, but most likely yes.

I wonder if that bit is needed on some particular version of the DWMMC core. In that case, should we have "snps,dw-mshc" and "snps,dw-mshc-vN" binding ? Or should we use DT property to discern the need for this bit ?

Quoted 2 lines

That's the most common way to take into account peculiarities, add a property and handle it from the driver.

And by "that" you mean which of those two I listed , the "snps,dw-mshc-vN" or adding new DT prop ?

Quoted 66 lines · Marek Vasut

Quoted 65 lines · Vladimir Zapolskiy

Quoted 39 lines · Marek Vasut

Quoted 38 lines · Vladimir Zapolskiy

Hi Marek,

Quoted 34 lines · Marek Vasut

Quoted 25 lines · Vladimir Zapolskiy

Quoted 17 lines · Marek Vasut

Hi!

I noticed that arch/arc/boot/dts/axs10x_mb.dtsi uses "altr," prefix in the DT compatible string:

mmc@0x15000 {
        compatible = "altr,socfpga-dw-mshc";
        reg = < 0x15000 0x400 >;
        num-slots = < 1 >;
        fifo-depth = < 16 >;
        card-detect-delay = < 200 >;
        clocks = <&apbclk>, <&mmcclk>;
        clock-names = "biu", "ciu";
        interrupts = < 7 >;
        bus-width = < 4 >;
};

Quoted 6 lines

I don't think this is OK, since ARC is unrelated to Altera, which is what the "altr," prefix stands for. I think the socfpga-dw-mshc shim should be extended with another compatibility string, something like "snps,arc-dw-mshc" and the axs10x_mb.dtsi should be adjusted accordingly. What do you think ?

Quoted 3 lines

There is "snps,dw-mshc" described in Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt and supported by dw_mmc host controller driver.

Thanks, that's even better.

btw what do you think of using altr, prefix on non-altera system, that doesn't seem ok, right ?

Quoted 7 lines

Quoted 4 lines

according to ePAPR the prefix should represent a device (IP block here I believe) manufacturer, so it should be okay to use "altr" prefix on non-Altera system, if Altera provides another hardware vendor with some own IP block.

In this case, it's Synopsys who provides the SD/MMC/MS core to other chip makers (Altera etc).

Correct.

Quoted 14 lines

Quoted 3 lines

That said, I would rather prefer to see "snps,dw-mshc" prefix on description of an MMC controller found on SoCFPGA series, "altr,socfpga-dw-mshc" seems to be redundant.

According to drivers/mmc/host/dw_mmc-pltfm.c , the Altera SoCFPGA one "altr,socfpga-dw-mshc" and also Imagination Technology Pistacio one "img,pistachio-dw-mshc" need specialty bit (SDMMC_CMD_USE_HOLD_REG), while the stock one "snps,dw-mshc" does not. I am not sure if the ARC one needs it as well, but most likely yes.

I wonder if that bit is needed on some particular version of the DWMMC core. In that case, should we have "snps,dw-mshc" and "snps,dw-mshc-vN" binding ? Or should we use DT property to discern the need for this bit ?

Quoted 6 lines

Quoted 2 lines

That's the most common way to take into account peculiarities, add a property and handle it from the driver.

And by "that" you mean which of those two I listed , the "snps,dw-mshc-vN" or adding new DT prop ?

I meant to add a new property, not a new compatible, but that's just my experience.

Let me say it __might__ happen that a particular change you need is specific to a particular version of the DWMMC IP (query Synopsys by the way), but more probably it might be e.g. the same IP version with a different reduced or extended configuration or a minor fix/improvement to the IP block without resulting version number bump.

For example I don't remember that errata fixes in IP blocks result in a new compatible, instead there are quite common optional "quirk" properties for broken IPs -- e.g. check bindings/usb/dwc3.txt :)

Quoted 84 lines · Vladimir Zapolskiy

Quoted 64 lines · Marek Vasut

Quoted 62 lines · Vladimir Zapolskiy

Quoted 39 lines · Marek Vasut

Quoted 37 lines · Vladimir Zapolskiy

Hi Marek,

Quoted 33 lines · Marek Vasut

Quoted 26 lines · Vladimir Zapolskiy

Quoted 18 lines · Marek Vasut

Hi!

I noticed that arch/arc/boot/dts/axs10x_mb.dtsi uses "altr," prefix in the DT compatible string:

mmc@0x15000 {
        compatible = "altr,socfpga-dw-mshc";
        reg = < 0x15000 0x400 >;
        num-slots = < 1 >;
        fifo-depth = < 16 >;
        card-detect-delay = < 200 >;
        clocks = <&apbclk>, <&mmcclk>;
        clock-names = "biu", "ciu";
        interrupts = < 7 >;
        bus-width = < 4 >;
};

Quoted 6 lines

I don't think this is OK, since ARC is unrelated to Altera, which is what the "altr," prefix stands for. I think the socfpga-dw-mshc shim should be extended with another compatibility string, something like "snps,arc-dw-mshc" and the axs10x_mb.dtsi should be adjusted accordingly. What do you think ?

Quoted 3 lines

There is "snps,dw-mshc" described in Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt and supported by dw_mmc host controller driver.

Thanks, that's even better.

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

btw what do you think of using altr, prefix on non-altera system, that doesn't seem ok, right ?

according to ePAPR the prefix should represent a device (IP block here I believe) manufacturer, so it should be okay to use "altr" prefix on non-Altera system, if Altera provides  another hardware vendor with some own IP block.

In this case, it's Synopsys who provides the SD/MMC/MS core to other chip makers (Altera etc).

Correct.

Quoted 11 lines

Quoted 5 lines

That said, I would rather prefer to see "snps,dw-mshc" prefix on description of an MMC controller found on SoCFPGA series, "altr,socfpga-dw-mshc" seems to be redundant.

According to drivers/mmc/host/dw_mmc-pltfm.c , the Altera SoCFPGA one "altr,socfpga-dw-mshc" and also Imagination Technology Pistacio one "img,pistachio-dw-mshc" need specialty bit (SDMMC_CMD_USE_HOLD_REG), while the stock one "snps,dw-mshc" does not. I am not sure if the ARC one needs it as well, but most likely yes.

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

I wonder if that bit is needed on some particular version of the DWMMC core. In that case, should we have "snps,dw-mshc" and "snps,dw-mshc-vN" binding ? Or should we use DT property to discern the need for this bit ?

That's the most common way to take into account peculiarities, add a property and handle it from the driver.

And by "that" you mean which of those two I listed , the "snps,dw-mshc-vN" or adding new DT prop ?

I meant to add a new property, not a new compatible, but that's just my experience.

Let me say it __might__ happen that a particular change you need is specific to a particular version of the DWMMC IP (query Synopsys by the way), but more probably it might be e.g. the same IP version with a different reduced or extended configuration or a minor fix/improvement to the IP block without resulting version number bump.

Quoted 3 lines

For example I don't remember that errata fixes in IP blocks result in a new compatible, instead there are quite common optional "quirk" properties for broken IPs -- e.g. check bindings/usb/dwc3.txt :)

Right, this very much matches how I see it as well. Thanks for confirming.

Alexey, can you tell us if the requirement for setting SDMMC_CMD_USE_HOLD_REG came with some new revision of the core or disappeared with some revision OR if this is some configuration option of the core during synthesis ?

Hi Marek, Vladimir,

Quoted 61 lines · Marek Vasut

Quoted 60 lines · Vladimir Zapolskiy

Quoted 58 lines · Marek Vasut

Quoted 56 lines · Vladimir Zapolskiy

Quoted 39 lines · Marek Vasut

Quoted 37 lines · Vladimir Zapolskiy

Hi Marek,

Quoted 33 lines · Marek Vasut

Quoted 26 lines · Vladimir Zapolskiy

Quoted 18 lines · Marek Vasut

Hi!

I noticed that arch/arc/boot/dts/axs10x_mb.dtsi uses "altr," prefix in the DT compatible string:

mmc@0x15000 {
        compatible = "altr,socfpga-dw-mshc";
        reg = < 0x15000 0x400 >;
        num-slots = < 1 >;
        fifo-depth = < 16 >;
        card-detect-delay = < 200 >;
        clocks = <&apbclk>, <&mmcclk>;
        clock-names = "biu", "ciu";
        interrupts = < 7 >;
        bus-width = < 4 >;
};

Quoted 6 lines

I don't think this is OK, since ARC is unrelated to Altera, which is what the "altr," prefix stands for. I think the socfpga-dw-mshc shim should be extended with another compatibility string, something like "snps,arc-dw-mshc" and the axs10x_mb.dtsi should be adjusted accordingly. What do you think ?

Quoted 3 lines

There is "snps,dw-mshc" described in Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt and supported by dw_mmc host controller driver.

Thanks, that's even better.

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

btw what do you think of using altr, prefix on non-altera system, that doesn't seem ok, right ?

according to ePAPR the prefix should represent a device (IP block here I believe) manufacturer, so it should be okay to use "altr" prefix on non-Altera system, if Altera provides another hardware vendor with some own IP block.

In this case, it's Synopsys who provides the SD/MMC/MS core to other chip makers (Altera etc).

Correct.

Quoted 5 lines

Quoted 5 lines

That said, I would rather prefer to see "snps,dw-mshc" prefix on description of an MMC controller found on SoCFPGA series, "altr,socfpga-dw-mshc" seems to be redundant.

Quoted 29 lines

Quoted 24 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

According to drivers/mmc/host/dw_mmc-pltfm.c , the Altera SoCFPGA one "altr,socfpga-dw-mshc" and also Imagination Technology Pistacio one "img,pistachio-dw-mshc" need specialty bit (SDMMC_CMD_USE_HOLD_REG), while the stock one "snps,dw-mshc" does not. I am not sure if the ARC one needs it as well, but most likely yes.

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

I wonder if that bit is needed on some particular version of the DWMMC core. In that case, should we have "snps,dw-mshc" and "snps,dw-mshc-vN" binding ? Or should we use DT property to discern the need for this bit ?

That's the most common way to take into account peculiarities, add a property and handle it from the driver.

And by "that" you mean which of those two I listed , the "snps,dw-mshc-vN" or adding new DT prop ?

I meant to add a new property, not a new compatible, but that's just my experience.

Let me say it __might__ happen that a particular change you need is specific to a particular version of the DWMMC IP (query Synopsys by the way), but more probably it might be e.g. the same IP version with a different reduced or extended configuration or a minor fix/improvement to the IP block without resulting version number bump.

Quoted 3 lines

For example I don't remember that errata fixes in IP blocks result in a new compatible, instead there are quite common optional "quirk" properties for broken IPs -- e.g. check bindings/usb/dwc3.txt :)

Right, this very much matches how I see it as well. Thanks for confirming.

Quoted 4 lines

Alexey, can you tell us if the requirement for setting SDMMC_CMD_USE_HOLD_REG came with some new revision of the core or disappeared with some revision OR if this is some configuration option of the core during synthesis ?

Sorry for not following that discussion during my weekend but I'll try to address all questions now.

DW Mobile Storage databook says:
--------------------->8-----------------------
To meet the relatively high Input Hold Time requirement for SDR12, SDR25, and other MMC speed modes, you should program bit[29]use_hold_Reg of the CMD register to 1'b1. --------------------->8-----------------------

So I'd say this specific setting has nothing to do with a particular IP block but instead it is related to card's mode of operation. More precisely bus clock. SDR12 stands for 12.5 MByte/s, SDR25 stands for 25 MByte/s. I.e. we probably need so set that bit just for certain cases and regardless board that uses DW MMC.

I'm adding DW MMC maintainer as well as linux-mmc mailing list so people who understands that stuff better may comment here as well.

-Alexey--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi,

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

That said, I would rather prefer to see "snps,dw-mshc" prefix on description of an MMC controller found on SoCFPGA series, "altr,socfpga-dw-mshc" seems to be redundant.

Yes..it's redundant..i should be combined to "snps,dw-mshc".

Quoted 33 lines

Quoted 33 lines

Quoted 33 lines

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

According to drivers/mmc/host/dw_mmc-pltfm.c , the Altera SoCFPGA one "altr,socfpga-dw-mshc" and also Imagination Technology Pistacio one "img,pistachio-dw-mshc" need specialty bit (SDMMC_CMD_USE_HOLD_REG), while the stock one "snps,dw-mshc" does not. I am not sure if the ARC one needs it as well, but most likely yes.

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

I wonder if that bit is needed on some particular version of the DWMMC core. In that case, should we have "snps,dw-mshc" and "snps,dw-mshc-vN" binding ? Or should we use DT property to discern the need for this bit ?

That's the most common way to take into account peculiarities, add a property and handle it from the driver.

And by "that" you mean which of those two I listed , the "snps,dw-mshc-vN" or adding new DT prop ?

I meant to add a new property, not a new compatible, but that's just my experience.

Let me say it __might__ happen that a particular change you need is specific to a particular version of the DWMMC IP (query Synopsys by the way), but more probably it might be e.g. the same IP version with a different reduced or extended configuration or a minor fix/improvement to the IP block without resulting version number bump.

For example I don't remember that errata fixes in IP blocks result in a new compatible, instead there are quite common optional "quirk" properties for broken IPs -- e.g. check bindings/usb/dwc3.txt :)

Quoted 8 lines

Quoted 6 lines

Right, this very much matches how I see it as well. Thanks for confirming.

Alexey, can you tell us if the requirement for setting SDMMC_CMD_USE_HOLD_REG came with some new revision of the core or disappeared with some revision OR if this is some configuration option of the core during synthesis ?

Sorry for not following that discussion during my weekend but I'll try to address all questions now.

SDMMC_CMD_USE_HOLD_REG didn't come with new revision..It's using continuously. But it's difficult to use the generic feature..because it's considered the below things.

If Card is SDR50/SDR104/DDR50 mode..
1) and phase shift of cclk_in_drv is 0 then SDMMC_CMD_USE_HOLD_REG bit is set to 0,
2) and phase shift of cclk_in_drv > 0 then SDMMC_CMD_USE_HOLD_REG bit is set to 1,
If Card is SDR12/SDR25 mode, then this bit is set to 1.

We need to check phase shift scheme..but as i knew, each SoC have been implemented differently for phase shift. (Phase shift have dependency to SoC.)

And it have to check HCON register..there is IMPLEMENT_HOLD_REG(bit[22]). (It described whether IP have hold register or not)

I didn't read this thread entirely.
I'm not sure what you have discussed..but my understanding is right..i recommend to use "snps,dw-mshc" for ARC compat string.
Otherwise it need to add "dw_mmc-<SoC>.c". dw_mmc-pltfm.c should provide the basic dw-mmc controller functionality.

Hi Jaehoon,

Quoted 1 line · Jaehoon Chung

Hi,

[snip]

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 4 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

That said, I would rather prefer to see "snps,dw-mshc" prefix on description of an MMC controller found on SoCFPGA series, "altr,socfpga-dw-mshc" seems to be redundant.

Yes..it's redundant..i should be combined to "snps,dw-mshc".

So for socfpga platform compat string should be something like "snps,dw-mshc-socfpga" then?

Quoted 45 lines

Quoted 34 lines

Quoted 34 lines

Quoted 34 lines

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

According to drivers/mmc/host/dw_mmc-pltfm.c , the Altera SoCFPGA one "altr,socfpga-dw-mshc" and also Imagination Technology Pistacio one "img,pistachio-dw-mshc" need specialty bit (SDMMC_CMD_USE_HOLD_REG), while the stock one "snps,dw-mshc" does not. I am not sure if the ARC one needs it as well, but most likely yes.

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

I wonder if that bit is needed on some particular version of the DWMMC core. In that case, should we have "snps,dw-mshc" and "snps,dw-mshc-vN" binding ? Or should we use DT property to discern the need for this bit ?

That's the most common way to take into account peculiarities, add a property and handle it from the driver.

And by "that" you mean which of those two I listed , the "snps,dw-mshc-vN" or adding new DT prop ?

I meant to add a new property, not a new compatible, but that's just my experience.

Let me say it __might__ happen that a particular change you need is specific to a particular version of the DWMMC IP (query Synopsys by the way), but more probably it might be e.g. the same IP version with a different reduced or extended configuration or a minor fix/improvement to the IP block without resulting version number bump.

For example I don't remember that errata fixes in IP blocks result in a new compatible, instead there are quite common optional "quirk" properties for broken IPs -- e.g. check bindings/usb/dwc3.txt :)

Quoted 8 lines

Quoted 6 lines

Right, this very much matches how I see it as well. Thanks for confirming.

Alexey, can you tell us if the requirement for setting SDMMC_CMD_USE_HOLD_REG came with some new revision of the core or disappeared with some revision OR if this is some configuration option of the core during synthesis ?

Sorry for not following that discussion during my weekend but I'll try to address all questions now.

SDMMC_CMD_USE_HOLD_REG didn't come with new revision..It's using continuously. But it's difficult to use the generic feature..because it's considered the below things.

Quoted 4 lines

If Card is SDR50/SDR104/DDR50 mode..
1) and phase shift of cclk_in_drv is 0 then SDMMC_CMD_USE_HOLD_REG bit is set to 0,
2) and phase shift of cclk_in_drv > 0 then SDMMC_CMD_USE_HOLD_REG bit is set to 1,
If Card is SDR12/SDR25 mode, then this bit is set to 1.

So card type is also important here and for certain card type we don't need to set SDMMC_CMD_USE_HOLD_REG, right?

Quoted 3 lines

We need to check phase shift scheme..but as i knew, each SoC have been implemented differently for phase shift. (Phase shift have dependency to SoC.)

Given my assumption above we need to check 2 things:
* Card type
* SoC-specific implementation detail (phase shift scheme)

Quoted 3 lines

And it have to check HCON register..there is IMPLEMENT_HOLD_REG(bit[22]). (It described whether IP have hold register or not)

Ah actually 3 things
+ IMPLEMENT_HOLD_REG

Quoted 5 lines

I didn't read this thread entirely.
I'm not sure what you have discussed..but my understanding is right..i recommend to use "snps,dw-mshc" for ARC compat string.
Otherwise it need to add "dw_mmc-<SoC>.c". dw_mmc-pltfm.c should provide the basic dw-mmc controller functionality.

Hm, interesting looks like you already made some changes here:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=aaaaeb7a933471f6413ca44dd36efd57f2fa9429

So now driver checks if SoC has HOLD REG then SDMMC_CMD_USE_HOLD_REG will be set (regardless card type).

And what's interesting and connected to this discussion since mentioned commit there's no point in having both "altr,socfpga-dw-mshc" and "img,pistachio-dw-mshc" compat strings because the do nothing now. I.e. it's time to replace both mentioned compat strings with generic "snps,dw-mshc".

Anybody volunteers for that .dts* cleanup?

-Alexey--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi,

Quoted 61 lines · Alexey Brodkin

Hi Marek, Vladimir,

Quoted 58 lines · Marek Vasut

Quoted 57 lines · Vladimir Zapolskiy

Quoted 55 lines · Marek Vasut

Quoted 53 lines · Vladimir Zapolskiy

Quoted 39 lines · Marek Vasut

Quoted 37 lines · Vladimir Zapolskiy

Hi Marek,

Quoted 33 lines · Marek Vasut

Quoted 26 lines · Vladimir Zapolskiy

Quoted 18 lines · Marek Vasut

Hi!

I noticed that arch/arc/boot/dts/axs10x_mb.dtsi uses "altr," prefix in the DT compatible string:

mmc@0x15000 {
        compatible = "altr,socfpga-dw-mshc";
        reg = < 0x15000 0x400 >;
        num-slots = < 1 >;
        fifo-depth = < 16 >;
        card-detect-delay = < 200 >;
        clocks = <&apbclk>, <&mmcclk>;
        clock-names = "biu", "ciu";
        interrupts = < 7 >;
        bus-width = < 4 >;
};

Quoted 6 lines

I don't think this is OK, since ARC is unrelated to Altera, which is what the "altr," prefix stands for. I think the socfpga-dw-mshc shim should be extended with another compatibility string, something like "snps,arc-dw-mshc" and the axs10x_mb.dtsi should be adjusted accordingly. What do you think ?

Quoted 3 lines

There is "snps,dw-mshc" described in Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt and supported by dw_mmc host controller driver.

Thanks, that's even better.

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

btw what do you think of using altr, prefix on non-altera system, that doesn't seem ok, right ?

according to ePAPR the prefix should represent a device (IP block here I believe) manufacturer, so it should be okay to use "altr" prefix on non-Altera system, if Altera provides another hardware vendor with some own IP block.

In this case, it's Synopsys who provides the SD/MMC/MS core to other chip makers (Altera etc).

Correct.

Quoted 2 lines

Quoted 2 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

That said, I would rather prefer to see "snps,dw-mshc" prefix on description of an MMC controller found on SoCFPGA series, "altr,socfpga-dw-mshc" seems to be redundant.

Yes..it's redundant..i should be combined to "snps,dw-mshc".

Quoted 29 lines

Quoted 29 lines

Quoted 24 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

According to drivers/mmc/host/dw_mmc-pltfm.c , the Altera SoCFPGA one "altr,socfpga-dw-mshc" and also Imagination Technology Pistacio one "img,pistachio-dw-mshc" need specialty bit (SDMMC_CMD_USE_HOLD_REG), while the stock one "snps,dw-mshc" does not. I am not sure if the ARC one needs it as well, but most likely yes.

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

I wonder if that bit is needed on some particular version of the DWMMC core. In that case, should we have "snps,dw-mshc" and "snps,dw-mshc-vN" binding ? Or should we use DT property to discern the need for this bit ?

That's the most common way to take into account peculiarities, add a property and handle it from the driver.

And by "that" you mean which of those two I listed , the "snps,dw-mshc-vN" or adding new DT prop ?

I meant to add a new property, not a new compatible, but that's just my experience.

Let me say it __might__ happen that a particular change you need is specific to a particular version of the DWMMC IP (query Synopsys by the way), but more probably it might be e.g. the same IP version with a different reduced or extended configuration or a minor fix/improvement to the IP block without resulting version number bump.

Quoted 3 lines

For example I don't remember that errata fixes in IP blocks result in a new compatible, instead there are quite common optional "quirk" properties for broken IPs -- e.g. check bindings/usb/dwc3.txt :)

Right, this very much matches how I see it as well. Thanks for confirming.

Quoted 7 lines

Quoted 4 lines

Alexey, can you tell us if the requirement for setting SDMMC_CMD_USE_HOLD_REG came with some new revision of the core or disappeared with some revision OR if this is some configuration option of the core during synthesis ?

Sorry for not following that discussion during my weekend but I'll try to address all questions now.

SDMMC_CMD_USE_HOLD_REG didn't come with new revision..It's using continuously. But it's difficult to use the generic feature..because it's considered the below things.

If Card is SDR50/SDR104/DDR50 mode..
1) and phase shift of cclk_in_drv is 0 then SDMMC_CMD_USE_HOLD_REG bit is set to 0,
2) and phase shift of cclk_in_drv > 0 then SDMMC_CMD_USE_HOLD_REG bit is set to 1,
If Card is SDR12/SDR25 mode, then this bit is set to 1.

We need to check phase shift scheme..but as i knew, each SoC have been implemented differently for phase shift. (Phase shift have dependency to SoC.)

And it have to check HCON register..there is IMPLEMENT_HOLD_REG(bit[22]). (It described whether IP have hold register or not)

I didn't read this thread entirely.
I'm not sure what you have discussed..but my understanding is right..i recommend to use "snps,dw-mshc" for ARC compat string. Otherwise it need to add "dw_mmc-<SoC>.c". dw_mmc-pltfm.c should provide the basic dw-mmc controller functionality.

Hi,

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

That said, I would rather prefer to see "snps,dw-mshc" prefix on description of an MMC controller found on SoCFPGA series, "altr,socfpga-dw-mshc" seems to be redundant.

Yes..it's redundant..i should be combined to "snps,dw-mshc".

Quoted 29 lines

Quoted 29 lines

Quoted 24 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

According to drivers/mmc/host/dw_mmc-pltfm.c , the Altera SoCFPGA one "altr,socfpga-dw-mshc" and also Imagination Technology Pistacio one "img,pistachio-dw-mshc" need specialty bit (SDMMC_CMD_USE_HOLD_REG), while the stock one "snps,dw-mshc" does not. I am not sure if the ARC one needs it as well, but most likely yes.

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

I wonder if that bit is needed on some particular version of the DWMMC core. In that case, should we have "snps,dw-mshc" and "snps,dw-mshc-vN" binding ? Or should we use DT property to discern the need for this bit ?

That's the most common way to take into account peculiarities, add a property and handle it from the driver.

And by "that" you mean which of those two I listed , the "snps,dw-mshc-vN" or adding new DT prop ?

I meant to add a new property, not a new compatible, but that's just my experience.

Let me say it __might__ happen that a particular change you need is specific to a particular version of the DWMMC IP (query Synopsys by the way), but more probably it might be e.g. the same IP version with a different reduced or extended configuration or a minor fix/improvement to the IP block without resulting version number bump.

Quoted 3 lines

For example I don't remember that errata fixes in IP blocks result in a new compatible, instead there are quite common optional "quirk" properties for broken IPs -- e.g. check bindings/usb/dwc3.txt :)

Right, this very much matches how I see it as well. Thanks for confirming.

Quoted 7 lines

Quoted 4 lines

Alexey, can you tell us if the requirement for setting SDMMC_CMD_USE_HOLD_REG came with some new revision of the core or disappeared with some revision OR if this is some configuration option of the core during synthesis ?

Sorry for not following that discussion during my weekend but I'll try to address all questions now.

SDMMC_CMD_USE_HOLD_REG didn't come with new revision..It's using continuously. But it's difficult to use the generic feature..because it's considered the below things.

If Card is SDR50/SDR104/DDR50 mode..
1) and phase shift of cclk_in_drv is 0 then SDMMC_CMD_USE_HOLD_REG bit is set to 0,
2) and phase shift of cclk_in_drv > 0 then SDMMC_CMD_USE_HOLD_REG bit is set to 1,
If Card is SDR12/SDR25 mode, then this bit is set to 1.

We need to check phase shift scheme..but as i knew, each SoC have been implemented differently for phase shift. (Phase shift have dependency to SoC.)

And it have to check HCON register..there is IMPLEMENT_HOLD_REG(bit[22]). (It described whether IP have hold register or not)

I didn't read this thread entirely.
I'm not sure what you have discussed..but my understanding is right..i recommend to use "snps,dw-mshc" for ARC compat string. Otherwise it need to add "dw_mmc-<SoC>.c". dw_mmc-pltfm.c should provide the basic dw-mmc controller functionality.

After read this thread entirely, i will check more detailed what you discussed. If i missed something, let me know, plz.

Best Regards,
Jaehoon Chung

Quoted 22 lines

DW Mobile Storage databook says:
--------------------->8-----------------------
To meet the relatively high Input Hold Time requirement for SDR12, SDR25, and other MMC speed modes, you should program bit[29]use_hold_Reg of the CMD register to 1'b1. --------------------->8-----------------------

So I'd say this specific setting has nothing to do with a particular IP block but instead it is related to card's mode of operation. More precisely bus clock. SDR12 stands for 12.5 MByte/s, SDR25 stands for 25 MByte/s. I.e. we probably need so set that bit just for certain cases and regardless board that uses DW MMC.

I'm adding DW MMC maintainer as well as linux-mmc mailing list so people who understands that stuff better may comment here as well.

-Alexey--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 64 lines · Jaehoon Chung

Hi,

Quoted 61 lines · Alexey Brodkin

Hi Marek, Vladimir,

Quoted 58 lines · Marek Vasut

Quoted 57 lines · Vladimir Zapolskiy

Quoted 55 lines · Marek Vasut

Quoted 53 lines · Vladimir Zapolskiy

Quoted 39 lines · Marek Vasut

Quoted 37 lines · Vladimir Zapolskiy

Hi Marek,

Quoted 33 lines · Marek Vasut

Quoted 26 lines · Vladimir Zapolskiy

Quoted 18 lines · Marek Vasut

Hi!

I noticed that arch/arc/boot/dts/axs10x_mb.dtsi uses "altr," prefix in the DT compatible string:

mmc@0x15000 {
        compatible = "altr,socfpga-dw-mshc";
        reg = < 0x15000 0x400 >;
        num-slots = < 1 >;
        fifo-depth = < 16 >;
        card-detect-delay = < 200 >;
        clocks = <&apbclk>, <&mmcclk>;
        clock-names = "biu", "ciu";
        interrupts = < 7 >;
        bus-width = < 4 >;
};

Quoted 6 lines

I don't think this is OK, since ARC is unrelated to Altera, which is what the "altr," prefix stands for. I think the socfpga-dw-mshc shim should be extended with another compatibility string, something like "snps,arc-dw-mshc" and the axs10x_mb.dtsi should be adjusted accordingly. What do you think ?

Quoted 3 lines

There is "snps,dw-mshc" described in Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt and supported by dw_mmc host controller driver.

Thanks, that's even better.

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

btw what do you think of using altr, prefix on non-altera system, that doesn't seem ok, right ?

according to ePAPR the prefix should represent a device (IP block here I believe) manufacturer, so it should be okay to use "altr" prefix on non-Altera system, if Altera provides another hardware vendor with some own IP block.

In this case, it's Synopsys who provides the SD/MMC/MS core to other chip makers (Altera etc).

Correct.

Quoted 2 lines

Quoted 2 lines

Quoted 5 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

That said, I would rather prefer to see "snps,dw-mshc" prefix on description of an MMC controller found on SoCFPGA series, "altr,socfpga-dw-mshc" seems to be redundant.

Yes..it's redundant..i should be combined to "snps,dw-mshc".

socfpga is done correctly, IMO.

Quoted 51 lines

Quoted 29 lines

Quoted 29 lines

Quoted 24 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

According to drivers/mmc/host/dw_mmc-pltfm.c , the Altera SoCFPGA one "altr,socfpga-dw-mshc" and also Imagination Technology Pistacio one "img,pistachio-dw-mshc" need specialty bit (SDMMC_CMD_USE_HOLD_REG), while the stock one "snps,dw-mshc" does not. I am not sure if the ARC one needs it as well, but most likely yes.

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

I wonder if that bit is needed on some particular version of the DWMMC core. In that case, should we have "snps,dw-mshc" and "snps,dw-mshc-vN" binding ? Or should we use DT property to discern the need for this bit ?

That's the most common way to take into account peculiarities, add a property and handle it from the driver.

And by "that" you mean which of those two I listed , the "snps,dw-mshc-vN" or adding new DT prop ?

I meant to add a new property, not a new compatible, but that's just my experience.

Let me say it __might__ happen that a particular change you need is specific to a particular version of the DWMMC IP (query Synopsys by the way), but more probably it might be e.g. the same IP version with a different reduced or extended configuration or a minor fix/improvement to the IP block without resulting version number bump.

Quoted 3 lines

For example I don't remember that errata fixes in IP blocks result in a new compatible, instead there are quite common optional "quirk" properties for broken IPs -- e.g. check bindings/usb/dwc3.txt :)

Right, this very much matches how I see it as well. Thanks for confirming.

Quoted 7 lines

Quoted 4 lines

Alexey, can you tell us if the requirement for setting SDMMC_CMD_USE_HOLD_REG came with some new revision of the core or disappeared with some revision OR if this is some configuration option of the core during synthesis ?

Sorry for not following that discussion during my weekend but I'll try to address all questions now.

SDMMC_CMD_USE_HOLD_REG didn't come with new revision..It's using continuously. But it's difficult to use the generic feature..because it's considered the below things.

If Card is SDR50/SDR104/DDR50 mode..
1) and phase shift of cclk_in_drv is 0 then SDMMC_CMD_USE_HOLD_REG bit is set to 0,
2) and phase shift of cclk_in_drv > 0 then SDMMC_CMD_USE_HOLD_REG bit is set to 1,
If Card is SDR12/SDR25 mode, then this bit is set to 1.

We need to check phase shift scheme..but as i knew, each SoC have been implemented differently for phase shift. (Phase shift have dependency to SoC.)

And it have to check HCON register..there is IMPLEMENT_HOLD_REG(bit[22]). (It described whether IP have hold register or not)

Quoted 3 lines

I didn't read this thread entirely.
I'm not sure what you have discussed..but my understanding is right..i recommend to use "snps,dw-mshc" for ARC compat string. Otherwise it need to add "dw_mmc-<SoC>.c". dw_mmc-pltfm.c should provide the basic dw-mmc controller functionality.

You should use "snps,dw-mshc", but there should also be an SoC specific compatible string. There are always integration differences and even using just versions of IP blocks is not specific enough. This should not require another driver file.

Rob

Quoted 1 line · Jaehoon Chung

Hi,

Hi,

[...]

Quoted 5 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

That said, I would rather prefer to see "snps,dw-mshc" prefix on description of an MMC controller found on SoCFPGA series, "altr,socfpga-dw-mshc" seems to be redundant.

Yes..it's redundant..i should be combined to "snps,dw-mshc".

Should the compat string be
compatible = "altr,socfpga-dw-mshc", "snps,dw-mshc";
or just
compatible = "snps,dw-mshc";
?

I am under the impression that a soc-specific identifier in addition to a generic one (used by the driver compat table) is a good idea, because it can help discerning the IP block from a generic one if needed at some future point in time. It will also not break the DT for systems which may depend on the non-generic compat, like *BSDs and such.

What do you think ? (btw this is very much my question in this thread)

Quoted 55 lines

Quoted 29 lines

Quoted 29 lines

Quoted 24 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

According to drivers/mmc/host/dw_mmc-pltfm.c , the Altera SoCFPGA one "altr,socfpga-dw-mshc" and also Imagination Technology Pistacio one "img,pistachio-dw-mshc" need specialty bit (SDMMC_CMD_USE_HOLD_REG), while the stock one "snps,dw-mshc" does not. I am not sure if the ARC one needs it as well, but most likely yes.

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

I wonder if that bit is needed on some particular version of the DWMMC core. In that case, should we have "snps,dw-mshc" and "snps,dw-mshc-vN" binding ? Or should we use DT property to discern the need for this bit ?

That's the most common way to take into account peculiarities, add a property and handle it from the driver.

And by "that" you mean which of those two I listed , the "snps,dw-mshc-vN" or adding new DT prop ?

I meant to add a new property, not a new compatible, but that's just my experience.

Let me say it __might__ happen that a particular change you need is specific to a particular version of the DWMMC IP (query Synopsys by the way), but more probably it might be e.g. the same IP version with a different reduced or extended configuration or a minor fix/improvement to the IP block without resulting version number bump.

Quoted 3 lines

For example I don't remember that errata fixes in IP blocks result in a new compatible, instead there are quite common optional "quirk" properties for broken IPs -- e.g. check bindings/usb/dwc3.txt :)

Right, this very much matches how I see it as well. Thanks for confirming.

Quoted 7 lines

Quoted 4 lines

Alexey, can you tell us if the requirement for setting SDMMC_CMD_USE_HOLD_REG came with some new revision of the core or disappeared with some revision OR if this is some configuration option of the core during synthesis ?

Sorry for not following that discussion during my weekend but I'll try to address all questions now.

SDMMC_CMD_USE_HOLD_REG didn't come with new revision..It's using continuously. But it's difficult to use the generic feature..because it's considered the below things.

If Card is SDR50/SDR104/DDR50 mode..
1) and phase shift of cclk_in_drv is 0 then SDMMC_CMD_USE_HOLD_REG bit is set to 0,
2) and phase shift of cclk_in_drv > 0 then SDMMC_CMD_USE_HOLD_REG bit is set to 1,
If Card is SDR12/SDR25 mode, then this bit is set to 1.

We need to check phase shift scheme..but as i knew, each SoC have been implemented differently for phase shift. (Phase shift have dependency to SoC.)

And it have to check HCON register..there is IMPLEMENT_HOLD_REG(bit[22]). (It described whether IP have hold register or not)

I didn't read this thread entirely.
I'm not sure what you have discussed..but my understanding is right..i recommend to use "snps,dw-mshc" for ARC compat string. Otherwise it need to add "dw_mmc-<SoC>.c". dw_mmc-pltfm.c should provide the basic dw-mmc controller functionality.

Quoted 2 lines

After read this thread entirely, i will check more detailed what you discussed. If i missed something, let me know, plz.

Thanks for the clarification, linux-next indeed contains changes which make snps,dw-mshc and altr,socfpga-dw-mshc equal.

Quoted 26 lines

Best Regards,
Jaehoon Chung

Quoted 22 lines

DW Mobile Storage databook says:
--------------------->8-----------------------
To meet the relatively high Input Hold Time requirement for SDR12, SDR25, and other MMC speed modes, you should program bit[29]use_hold_Reg of the CMD register to 1'b1. --------------------->8-----------------------

So I'd say this specific setting has nothing to do with a particular IP block but instead it is related to card's mode of operation. More precisely bus clock. SDR12 stands for 12.5 MByte/s, SDR25 stands for 25 MByte/s. I.e. we probably need so set that bit just for certain cases and regardless board that uses DW MMC.

I'm adding DW MMC maintainer as well as linux-mmc mailing list so people who understands that stuff better may comment here as well.

-Alexey--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi,

Quoted 25 lines · Marek Vasut

Quoted 1 line · Jaehoon Chung

Hi,

Hi,

[...]

Quoted 5 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

That said, I would rather prefer to see "snps,dw-mshc" prefix on description of an MMC controller found on SoCFPGA series, "altr,socfpga-dw-mshc" seems to be redundant.

Yes..it's redundant..i should be combined to "snps,dw-mshc".

Should the compat string be
compatible = "altr,socfpga-dw-mshc", "snps,dw-mshc";
or just
compatible = "snps,dw-mshc";
?

I am under the impression that a soc-specific identifier in addition to a generic one (used by the driver compat table) is a good idea, because it can help discerning the IP block from a generic one if needed at some future point in time. It will also not break the DT for systems which may depend on the non-generic compat, like *BSDs and such.

Quoted 1 line

What do you think ? (btw this is very much my question in this thread)

IMO just 'compatible = "snps,dw-mshc"' is good enough, if it completely describes the IP block on SoCFGPA --- and from what I get it is the case. You can add a SoC-specific compatible if it is needed later on, and to my taste only if SoC specific features can not be covered by properties.

The same sole "snps,dw-mshc" compatible is specified for NXP LPC18xx/43xx, ZTE ZX and HiSilicon ARM SoCs.

Another similar example is ARM PrimeCell PLxxx IP blocks, as far as I know there is no SoC-specific compatibles/aliases for PrimeCell IP blocks.