Creating a new platform_bus inside a spi_driver

The idea is that "fpga-spi" is a spi_driver which instantiates all of the "fpga-deviceN" as platform_devices, through the use of of_platform_populate(dev->of_node, NULL, NULL, dev).

The visible problem we're facing with this approach is that, as the internal platform_devices have a "reg" property, of_platform_populate() eventually triggers an address translation which is apparently trying to translate the addresses of the internal platform_bus to addresses of the processor memory map.
This translation is however not part of our intention, as we intend to have an internal bus with its own memory map.
This fails when __of_translate_address() reaches the spi-master boundary because (as it seems to make sense) it isn't possible to translate them past that.
A KERN_ERR rated message like
"prom_parse: Bad cell count for /soc@f0000000/spi@2000/fpga@1"
is thrown by __of_translate_address() and later it is not possible to obtain the "reg" address with platform_get_resource().

On this scenario, we have a few questions and, depending on the outcome of these, possibly a patch.

1. Is it possible to have an internal platform_bus with a different memory map as we intended? Or are platform_busses and platform_devices supposed to always be mapped on the processor memory map?

2. If platform_bus and platform_device were actually designed to always be mappable to the processor memory map, what would be a different approach to this problem? One alternative considered was to define a new "fpga_bus" and "fpga_device" but that seemed as an overkill approach to the problem.

Quoted 19 lines · On Thursday 06 November 2014 18:02:52 DATACOM - Érico Nunes wrote:

The idea is that "fpga-spi" is a spi_driver which instantiates all of the "fpga-deviceN" as platform_devices, through the use of of_platform_populate(dev->of_node, NULL, NULL, dev).

The visible problem we're facing with this approach is that, as the internal platform_devices have a "reg" property, of_platform_populate() eventually triggers an address translation which is apparently trying to translate the addresses of the internal platform_bus to addresses of the processor memory map.
This translation is however not part of our intention, as we intend to have an internal bus with its own memory map.
This fails when __of_translate_address() reaches the spi-master boundary because (as it seems to make sense) it isn't possible to translate them past that.
A KERN_ERR rated message like
"prom_parse: Bad cell count for /soc@f0000000/spi@2000/fpga@1"
is thrown by __of_translate_address() and later it is not possible to obtain the "reg" address with platform_get_resource().

Quoted 6 lines

On this scenario, we have a few questions and, depending on the outcome of these, possibly a patch.

1. Is it possible to have an internal platform_bus with a different memory map as we intended? Or are platform_busses and platform_devices supposed to always be mapped on the processor memory map?

It's inconsistent. We have some code that assumes that platform devices are always memory mapped, and some other code that breaks this assumption.

Quoted 4 lines

2. If platform_bus and platform_device were actually designed to always be mappable to the processor memory map, what would be a different approach to this problem? One alternative considered was to define a new "fpga_bus" and "fpga_device" but that seemed as an overkill approach to the problem.

I think the existing mfd framework should do what you need, when you call mfd_add_devices() and pass a table of cells with the compatible strings for your devices, it should create the platform devices you want. If not, that can probably be fixed in the mfd core code.

Hello Arnd and all,

Quoted 20 lines · Arnd Bergmann

Quoted 19 lines · On Thursday 06 November 2014 18:02:52 DATACOM - Érico Nunes wrote:

The idea is that "fpga-spi" is a spi_driver which instantiates all of the "fpga-deviceN" as platform_devices, through the use of of_platform_populate(dev->of_node, NULL, NULL, dev).

The visible problem we're facing with this approach is that, as the internal platform_devices have a "reg" property, of_platform_populate() eventually triggers an address translation which is apparently trying to translate the addresses of the internal platform_bus to addresses of the processor memory map.
This translation is however not part of our intention, as we intend to have an internal bus with its own memory map.
This fails when __of_translate_address() reaches the spi-master boundary because (as it seems to make sense) it isn't possible to translate them past that.
A KERN_ERR rated message like
"prom_parse: Bad cell count for /soc@f0000000/spi@2000/fpga@1"
is thrown by __of_translate_address() and later it is not possible to obtain the "reg" address with platform_get_resource().

Quoted 8 lines

Quoted 6 lines

On this scenario, we have a few questions and, depending on the outcome of these, possibly a patch.

1. Is it possible to have an internal platform_bus with a different memory map as we intended? Or are platform_busses and platform_devices supposed to always be mapped on the processor memory map?

It's inconsistent. We have some code that assumes that platform devices are always memory mapped, and some other code that breaks this assumption.

By this I take that the platform subsystem could be made generic so it can be used in both ways (mapped to processor memory map or mapped to a private memory map). There seems to be no strict requirement enforcing it to be processor memory map.

Is this correct?

Quoted 10 lines

Quoted 4 lines

2. If platform_bus and platform_device were actually designed to always be mappable to the processor memory map, what would be a different approach to this problem? One alternative considered was to define a new "fpga_bus" and "fpga_device" but that seemed as an overkill approach to the problem.

I think the existing mfd framework should do what you need, when you call mfd_add_devices() and pass a table of cells with the compatible strings for your devices, it should create the platform devices you want. If not, that can probably be fixed in the mfd core code.

Thanks for the tip, we were not aware of the purpose of this mfd framework and we will take a look at this framework now.
However I'm thinking now that eventually it would fall in the same case of trying to translate the address of any "reg" dts property to the processor memory map, and fail with the same error for the SPI case.

Considering this and taking the answer to the first question, do you think a patch fixing the "error" report by the translation function would be acceptable?
We can prepare/test that under our platform and submit it.

Quoted 37 lines · On Friday 07 November 2014 14:37:26 DATACOM - Érico Nunes wrote:

Hello Arnd and all,

Quoted 20 lines · Arnd Bergmann

Quoted 19 lines · On Thursday 06 November 2014 18:02:52 DATACOM - Érico Nunes wrote:

The idea is that "fpga-spi" is a spi_driver which instantiates all of the "fpga-deviceN" as platform_devices, through the use of of_platform_populate(dev->of_node, NULL, NULL, dev).

The visible problem we're facing with this approach is that, as the internal platform_devices have a "reg" property, of_platform_populate() eventually triggers an address translation which is apparently trying to translate the addresses of the internal platform_bus to addresses of the processor memory map.
This translation is however not part of our intention, as we intend to have an internal bus with its own memory map.
This fails when __of_translate_address() reaches the spi-master boundary because (as it seems to make sense) it isn't possible to translate them past that.
A KERN_ERR rated message like
"prom_parse: Bad cell count for /soc@f0000000/spi@2000/fpga@1"
is thrown by __of_translate_address() and later it is not possible to obtain the "reg" address with platform_get_resource().

Quoted 8 lines

Quoted 6 lines

On this scenario, we have a few questions and, depending on the outcome of these, possibly a patch.

1. Is it possible to have an internal platform_bus with a different memory map as we intended? Or are platform_busses and platform_devices supposed to always be mapped on the processor memory map?

It's inconsistent. We have some code that assumes that platform devices are always memory mapped, and some other code that breaks this assumption.

By this I take that the platform subsystem could be made generic so it can be used in both ways (mapped to processor memory map or mapped to a private memory map). There seems to be no strict requirement enforcing it to be processor memory map.

Quoted 1 line

Is this correct?

It could be, but I'm sure if that is a good idea or not. It might complicate things elsewhere, so it would at least need careful testing and consensus among a broader group of developers.

Quoted 38 lines · Arnd Bergmann

Quoted 37 lines · On Friday 07 November 2014 14:37:26 DATACOM - Érico Nunes wrote:

Hello Arnd and all,

Quoted 20 lines · Arnd Bergmann

Quoted 19 lines · On Thursday 06 November 2014 18:02:52 DATACOM - Érico Nunes wrote:

The idea is that "fpga-spi" is a spi_driver which instantiates all of the "fpga-deviceN" as platform_devices, through the use of of_platform_populate(dev->of_node, NULL, NULL, dev).

The visible problem we're facing with this approach is that, as the internal platform_devices have a "reg" property, of_platform_populate() eventually triggers an address translation which is apparently trying to translate the addresses of the internal platform_bus to addresses of the processor memory map.
This translation is however not part of our intention, as we intend to have an internal bus with its own memory map.
This fails when __of_translate_address() reaches the spi-master boundary because (as it seems to make sense) it isn't possible to translate them past that.
A KERN_ERR rated message like
"prom_parse: Bad cell count for /soc@f0000000/spi@2000/fpga@1"
is thrown by __of_translate_address() and later it is not possible to obtain the "reg" address with platform_get_resource().

Quoted 8 lines

Quoted 6 lines

On this scenario, we have a few questions and, depending on the outcome of these, possibly a patch.

1. Is it possible to have an internal platform_bus with a different memory map as we intended? Or are platform_busses and platform_devices supposed to always be mapped on the processor memory map?

It's inconsistent. We have some code that assumes that platform devices are always memory mapped, and some other code that breaks this assumption.

By this I take that the platform subsystem could be made generic so it can be used in both ways (mapped to processor memory map or mapped to a private memory map). There seems to be no strict requirement enforcing it to be processor memory map.

Quoted 5 lines

Quoted 1 line

Is this correct?

It could be, but I'm sure if that is a good idea or not. It might complicate things elsewhere, so it would at least need careful testing and consensus among a broader group of developers.

I don't think it is a good idea. I would prefer to make the behaviour of of_platform_populate() generic so it could work for multiple bus types rather than reusing/abusing platform_device in this way.

If the devices on the FPGA were memory mapped, it would be a different situation, but being behind an SPI bus where the access to those devices must always be mediated by the SPI controller, it would be better to have a separate bus type and a separate pool of drivers for those devices.

Hi Grant, Arnd and Erico

Quoted 57 lines · Grant Likely

Quoted 38 lines · Arnd Bergmann

Quoted 37 lines · On Friday 07 November 2014 14:37:26 DATACOM - Érico Nunes wrote:

Hello Arnd and all,

Quoted 20 lines · Arnd Bergmann

Quoted 19 lines · On Thursday 06 November 2014 18:02:52 DATACOM - Érico Nunes wrote:

The idea is that "fpga-spi" is a spi_driver which instantiates all of the "fpga-deviceN" as platform_devices, through the use of of_platform_populate(dev->of_node, NULL, NULL, dev).

The visible problem we're facing with this approach is that, as the internal platform_devices have a "reg" property, of_platform_populate() eventually triggers an address translation which is apparently trying to translate the addresses of the internal platform_bus to addresses of the processor memory map.
This translation is however not part of our intention, as we intend to have an internal bus with its own memory map.
This fails when __of_translate_address() reaches the spi-master boundary because (as it seems to make sense) it isn't possible to translate them past that.
A KERN_ERR rated message like
"prom_parse: Bad cell count for /soc@f0000000/spi@2000/fpga@1"
is thrown by __of_translate_address() and later it is not possible to obtain the "reg" address with platform_get_resource().

Quoted 8 lines

Quoted 6 lines

On this scenario, we have a few questions and, depending on the outcome of these, possibly a patch.

1. Is it possible to have an internal platform_bus with a different memory map as we intended? Or are platform_busses and platform_devices supposed to always be mapped on the processor memory map?

It's inconsistent. We have some code that assumes that platform devices are always memory mapped, and some other code that breaks this assumption.

By this I take that the platform subsystem could be made generic so it can be used in both ways (mapped to processor memory map or mapped to a private memory map). There seems to be no strict requirement enforcing it to be processor memory map.

Quoted 5 lines

Quoted 1 line

Is this correct?

It could be, but I'm sure if that is a good idea or not. It might complicate things elsewhere, so it would at least need careful testing and consensus among a broader group of developers.

I don't think it is a good idea. I would prefer to make the behaviour of of_platform_populate() generic so it could work for multiple bus types rather than reusing/abusing platform_device in this way.

If the devices on the FPGA were memory mapped, it would be a different situation, but being behind an SPI bus where the access to those devices must always be mediated by the SPI controller, it would be better to have a separate bus type and a separate pool of drivers for those devices.

This is exactly the same problem that we have on Qualcomm SPMI PMIC mfd driver. We had a discussion at [1] where we tried to solve it by IORESOURCE_REG. Unfortunately there is no conclusion yet.

I'm glad to see that someone else have the same issue, maybe it is time to restart the discussion. The last proposal from Grant was to implement dev_get_localbus_address() API in drivers/base.

Hi Grant, Arnd and Erico

Quoted 57 lines · Grant Likely

Quoted 38 lines · Arnd Bergmann

Quoted 37 lines · On Friday 07 November 2014 14:37:26 DATACOM - Érico Nunes wrote:

Hello Arnd and all,

Quoted 20 lines · Arnd Bergmann

Quoted 19 lines · On Thursday 06 November 2014 18:02:52 DATACOM - Érico Nunes wrote:

The idea is that "fpga-spi" is a spi_driver which instantiates all of the "fpga-deviceN" as platform_devices, through the use of of_platform_populate(dev->of_node, NULL, NULL, dev).

The visible problem we're facing with this approach is that, as the internal platform_devices have a "reg" property, of_platform_populate() eventually triggers an address translation which is apparently trying to translate the addresses of the internal platform_bus to addresses of the processor memory map.
This translation is however not part of our intention, as we intend to have an internal bus with its own memory map.
This fails when __of_translate_address() reaches the spi-master boundary because (as it seems to make sense) it isn't possible to translate them past that.
A KERN_ERR rated message like
"prom_parse: Bad cell count for /soc@f0000000/spi@2000/fpga@1"
is thrown by __of_translate_address() and later it is not possible to obtain the "reg" address with platform_get_resource().

Quoted 8 lines

Quoted 6 lines

On this scenario, we have a few questions and, depending on the outcome of these, possibly a patch.

1. Is it possible to have an internal platform_bus with a different memory map as we intended? Or are platform_busses and platform_devices supposed to always be mapped on the processor memory map?

It's inconsistent. We have some code that assumes that platform devices are always memory mapped, and some other code that breaks this assumption.

By this I take that the platform subsystem could be made generic so it can be used in both ways (mapped to processor memory map or mapped to a private memory map). There seems to be no strict requirement enforcing it to be processor memory map.

Quoted 5 lines

Quoted 1 line

Is this correct?

It could be, but I'm sure if that is a good idea or not. It might complicate things elsewhere, so it would at least need careful testing and consensus among a broader group of developers.

I don't think it is a good idea. I would prefer to make the behaviour of of_platform_populate() generic so it could work for multiple bus types rather than reusing/abusing platform_device in this way.

If the devices on the FPGA were memory mapped, it would be a different situation, but being behind an SPI bus where the access to those devices must always be mediated by the SPI controller, it would be better to have a separate bus type and a separate pool of drivers for those devices.

This is exactly the same problem that we have on Qualcomm SPMI PMIC mfd driver. We had a discussion at [1] where we tried to solve it by IORESOURCE_REG. Unfortunately there is no conclusion yet.

I'm glad to see that someone else have the same issue, maybe it is time to restart the discussion. The last proposal from Grant was to implement dev_get_localbus_address() API in drivers/base.

Hello,

In our board we have a FPGA whose programmable logic communicates with the main processor via SPI. Inside the programmable logic we also have a range of registers (writable via SPI commands) which implement functionality of several distinct "devices".

Our approach to implement a driver framework for these devices was to have a platform_bus embedded in a spi_device. This device tree source draft illustrates the proposal:

{
    spi-master@2000 {
        #address-cells = <1>;
        #size-cells = <0>;
        compatible = "cpu-spi-master";
        reg = <0x2000 0x2000>;

        fpga@1 {
            #address-cells = <1>;
            #size-cells = <1>;
            compatible = "fpga-spi";
            reg = <1>;
            spi-max-frequency = <25000000>;

            fpga-device1@200 {
                compatible = "fpga-device1-driver";
                reg = <0x200 0x300>;
            };

            fpga-device2@500 {
                compatible = "fpga-device2-driver";
                reg = <0x500 0x100>;
            };

            fpga-device3@600 {
                compatible = "fpga-device3-driver";
                reg = <0x600 0x200>;
            };
        };
    };
}

The idea is that "fpga-spi" is a spi_driver which instantiates all of the "fpga-deviceN" as platform_devices, through the use of of_platform_populate(dev->of_node, NULL, NULL, dev).

The visible problem we're facing with this approach is that, as the internal platform_devices have a "reg" property, of_platform_populate() eventually triggers an address translation which is apparently trying to translate the addresses of the internal platform_bus to addresses of the processor memory map.
This translation is however not part of our intention, as we intend to have an internal bus with its own memory map.
This fails when __of_translate_address() reaches the spi-master boundary because (as it seems to make sense) it isn't possible to translate them past that.
A KERN_ERR rated message like
"prom_parse: Bad cell count for /soc@f0000000/spi@2000/fpga@1"
is thrown by __of_translate_address() and later it is not possible to obtain the "reg" address with platform_get_resource().

On this scenario, we have a few questions and, depending on the outcome of these, possibly a patch.

1. Is it possible to have an internal platform_bus with a different memory map as we intended? Or are platform_busses and platform_devices supposed to always be mapped on the processor memory map?

2. If platform_bus and platform_device were actually designed to always be mappable to the processor memory map, what would be a different approach to this problem? One alternative considered was to define a new "fpga_bus" and "fpga_device" but that seemed as an overkill approach to the problem.

3. By searching for the ocurrences of of_platform_populate(), some uses which look similar to our approach were found, in the following drivers:
drivers/mfd/smsc-ece1099.c
drivers/mfd/palmas.c
These have an i2c_driver which calls of_platform_populate() just like we attempted. Won't they run into a similar problem, if any of the children platform_devices provide a "reg" property?

4. By applying the patch attached at the end of this mail, things start to work as we intended. Basically instead of returning an error if we reach an untranslatable boundary, we return the resulting address so far. This is assuming that, for this case, we want an internal memory map for this device. Is this patch/assumption breaking any design decisions regarding the platform subsystem? It is of course not a widely tested patch, but could we be facing a bug which needs a fix like the one presented?

Thanks in advance!

Erico

[PATCH] of/address: return partial translation when crossing
untranslatable boundary

Signed-off-by erico.nunes <[email protected]>

---
 drivers/of/address.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/of/address.c +0 -0

index afdb782..ab56d31 100644