Message ID | 1575900490-74467-3-git-send-email-john.garry@huawei.com (mailing list archive) |
---|---|
State | Accepted |
Commit | a2ca53b52e007de81752bbb443d828f5950d6d04 |
Headers | show |
Series | HiSilicon v3xx SFC driver | expand |
On 09/12/2019 14:08, John Garry wrote: > + if (ret) > + return ret; > + > + if (op->data.dir == SPI_MEM_DATA_IN) > + hisi_sfc_v3xx_read_databuf(host, op->data.buf.in, len); > + > + return 0; > +} > + > +static int hisi_sfc_v3xx_exec_op(struct spi_mem *mem, > + const struct spi_mem_op *op) > +{ > + struct hisi_sfc_v3xx_host *host; > + struct spi_device *spi = mem->spi; > + u8 chip_select = spi->chip_select; > + > + host = spi_controller_get_devdata(spi->master); > + > + return hisi_sfc_v3xx_generic_exec_op(host, op, chip_select); > +} > + > +static const struct spi_controller_mem_ops hisi_sfc_v3xx_mem_ops = { > + .adjust_op_size = hisi_sfc_v3xx_adjust_op_size, > + .exec_op = hisi_sfc_v3xx_exec_op, > +}; > + > +static int hisi_sfc_v3xx_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct hisi_sfc_v3xx_host *host; > + struct spi_controller *ctlr; > + u32 version; > + int ret; > + > + ctlr = spi_alloc_master(&pdev->dev, sizeof(*host)); > + if (!ctlr) > + return -ENOMEM; > + Hi Mark, > + ctlr->mode_bits = SPI_RX_DUAL | SPI_RX_QUAD | > + SPI_TX_DUAL | SPI_TX_QUAD; I have an issue with dual/quad support. I naively thought that setting these bits would give me the highest protocol available. However, now I notice that spi_device.mode needs to be set for supported protocols for the slave - I'm using the generic spi mem ops to check if protocols are supported based on this value. From checking acpi_spi_add_resource() or anywhere else, I cannot see how SPI_RX_DUAL or the others are set for spi_device.mode. What am I missing? Are these just not supported yet for ACPI? Or should the spi-nor code not be relying on this since we should be able to get this info from the SPI NOR part? Cheers, John > + > + host = spi_controller_get_devdata(ctlr); > + host->dev = dev; > + > + platform_set_drvdata(pdev, host); > + > + host->regbase = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(host->regbase)) { > + ret = PTR_ERR(host->regbase); > + goto err_put_master;
On Thu, Jan 09, 2020 at 03:54:00PM +0000, John Garry wrote: > From checking acpi_spi_add_resource() or anywhere else, I cannot see how > SPI_RX_DUAL or the others are set for spi_device.mode. What am I missing? > Are these just not supported yet for ACPI? Or should the spi-nor code not be > relying on this since we should be able to get this info from the SPI NOR > part? I'm not aware of any work on integrating this sort of stuff into ACPI platforms so I think it's just not yet supported in ACPI. I'm not really sure what would be idiomatic for ACPI, figuring it out from what the part supports might well be idiomatic there though I don't know how common it is for people not to wire up all the data lines even if both controller and device support wider transfers. I've got a horrible feeling that the idiomatic thing is a combination of that and a bunch of per-device quirks. There may be a spec I'm not aware of though I'd be a bit surprised.
On 09/01/2020 21:28, Mark Brown wrote: > On Thu, Jan 09, 2020 at 03:54:00PM +0000, John Garry wrote: > >> From checking acpi_spi_add_resource() or anywhere else, I cannot see how >> SPI_RX_DUAL or the others are set for spi_device.mode. What am I missing? >> Are these just not supported yet for ACPI? Or should the spi-nor code not be >> relying on this since we should be able to get this info from the SPI NOR >> part? > Hi Mark, > I'm not aware of any work on integrating this sort of stuff into ACPI > platforms so I think it's just not yet supported in ACPI. I'm not > really sure what would be idiomatic for ACPI, figuring it out from what > the part supports might well be idiomatic there though I don't know how > common it is for people not to wire up all the data lines even if both > controller and device support wider transfers. OK, so I guess that is why we require the width property from the FW and can't blindly rely on SFDP. I've got a horrible > feeling that the idiomatic thing is a combination of that and a bunch of > per-device quirks. There may be a spec I'm not aware of though I'd be a > bit surprised. > I'm not sure on that. I don't see anything in the ACPI spec. I will note that PRP0001+"jedec,spi-nor" compatible DSD seems to be the defacto method to describe the SPI NOR-compat part for ACPI - that's what I'm using. We could add properties there, but that seems improper. I'll continue to look.... Thanks, John
On Fri, Jan 10, 2020 at 11:55:37AM +0000, John Garry wrote: > I will note that PRP0001+"jedec,spi-nor" compatible DSD seems to be the > defacto method to describe the SPI NOR-compat part for ACPI - that's what > I'm using. We could add properties there, but that seems improper. OK, so that's just reusing the DT binding in which case everything that's valid for the DT binding should also be valid for ACPI - I thought that actually worked automatically without you having to do anything in the code but ICBW.
On 10/01/2020 14:07, Mark Brown wrote: + Mika, Andy > On Fri, Jan 10, 2020 at 11:55:37AM +0000, John Garry wrote: > Hi Mark, >> I will note that PRP0001+"jedec,spi-nor" compatible DSD seems to be the >> defacto method to describe the SPI NOR-compat part for ACPI - that's what >> I'm using. We could add properties there, but that seems improper. > > OK, so that's just reusing the DT binding in which case everything > that's valid for the DT binding should also be valid for ACPI - I > thought that actually worked automatically without you having to do > anything in the code but ICBW. I thought that it would be improper as we could be mixing ACPI methods to describe the serial bus (SPI Serial Bus Connection Resource Descriptor) and also DT properties which could conflict, like CS active high. However I do see extra properties than "compatible" being added in DSD for PRP0001: https://patchwork.ozlabs.org/patch/662813/ (see EEPROM part) And if we were to do this, I think that we would need to add some device_property_read_u32("spi-rx-bus-width", ...), etc calls in the SPI FW parsing for ACPI path - I couldn't see that. Thanks, John
On Fri, Jan 10, 2020 at 02:58:54PM +0000, John Garry wrote: > On 10/01/2020 14:07, Mark Brown wrote: > > On Fri, Jan 10, 2020 at 11:55:37AM +0000, John Garry wrote: > > > OK, so that's just reusing the DT binding in which case everything > > that's valid for the DT binding should also be valid for ACPI - I > > thought that actually worked automatically without you having to do > > anything in the code but ICBW. > I thought that it would be improper as we could be mixing ACPI methods to > describe the serial bus (SPI Serial Bus Connection Resource Descriptor) and > also DT properties which could conflict, like CS active high. Yes, that's one of the issues with importing bits of DT into ACPI unfortunately - you will get conflicts, it's not clear it's a good idea to be using PRP0001 for SPI stuff given that there's bus level bindings for both ACPI and SPI and they don't line up exactly. > However I do see extra properties than "compatible" being added in DSD for > PRP0001: > https://patchwork.ozlabs.org/patch/662813/ (see EEPROM part) > And if we were to do this, I think that we would need to add some > device_property_read_u32("spi-rx-bus-width", ...), etc calls in the SPI FW > parsing for ACPI path - I couldn't see that. You'd need parsing code, yes.
On 10/01/2020 15:12, Mark Brown wrote: > On Fri, Jan 10, 2020 at 02:58:54PM +0000, John Garry wrote: >> On 10/01/2020 14:07, Mark Brown wrote: >>> On Fri, Jan 10, 2020 at 11:55:37AM +0000, John Garry wrote: > > Hi Mark, >>> OK, so that's just reusing the DT binding in which case everything >>> that's valid for the DT binding should also be valid for ACPI - I >>> thought that actually worked automatically without you having to do >>> anything in the code but ICBW. > >> I thought that it would be improper as we could be mixing ACPI methods to >> describe the serial bus (SPI Serial Bus Connection Resource Descriptor) and >> also DT properties which could conflict, like CS active high. > > Yes, that's one of the issues with importing bits of DT into ACPI > unfortunately - you will get conflicts, it's not clear it's a good idea > to be using PRP0001 for SPI stuff given that there's bus level bindings > for both ACPI and SPI and they don't line up exactly. Yeah, I'm not entirely comfortable with this yet. > >> However I do see extra properties than "compatible" being added in DSD for >> PRP0001: >> https://patchwork.ozlabs.org/patch/662813/ (see EEPROM part) > >> And if we were to do this, I think that we would need to add some >> device_property_read_u32("spi-rx-bus-width", ...), etc calls in the SPI FW >> parsing for ACPI path - I couldn't see that. > > You'd need parsing code, yes. > I'll continue to check the options. Thanks, john
On Fri, Jan 10, 2020 at 02:58:54PM +0000, John Garry wrote: > On 10/01/2020 14:07, Mark Brown wrote: > > On Fri, Jan 10, 2020 at 11:55:37AM +0000, John Garry wrote: > ... > > > I will note that PRP0001+"jedec,spi-nor" compatible DSD seems to be the > > > defacto method to describe the SPI NOR-compat part for ACPI - that's what > > > I'm using. We could add properties there, but that seems improper. > > > > OK, so that's just reusing the DT binding in which case everything > > that's valid for the DT binding should also be valid for ACPI - I > > thought that actually worked automatically without you having to do > > anything in the code but ICBW. > > I thought that it would be improper as we could be mixing ACPI methods to > describe the serial bus (SPI Serial Bus Connection Resource Descriptor) and > also DT properties which could conflict, like CS active high. > > However I do see extra properties than "compatible" being added in DSD for > PRP0001: > https://patchwork.ozlabs.org/patch/662813/ (see EEPROM part) PRP method is only for vendors to *test* the hardware in ACPI environment. The proper method is to allocate correct ACPI ID. Properties (_DSD in ACPI) may be used in the same way as for DT if we have no other means in ACPI specification for them. > And if we were to do this, I think that we would need to add some > device_property_read_u32("spi-rx-bus-width", ...), etc calls in the SPI FW > parsing for ACPI path - I couldn't see that. It's okay as long as you have ACPI ID. P.S. Most of the sensor drivers were updated in order to support ACPI PRP method due to DIY hobbyist on IoT sector and embedded devices. This should not be an official way how we support hardware on ACPI-based platforms.
On 10/01/2020 19:31, Andy Shevchenko wrote: > On Fri, Jan 10, 2020 at 02:58:54PM +0000, John Garry wrote: >> On 10/01/2020 14:07, Mark Brown wrote: >>> On Fri, Jan 10, 2020 at 11:55:37AM +0000, John Garry wrote: > > > ... > >>>> I will note that PRP0001+"jedec,spi-nor" compatible DSD seems to be the >>>> defacto method to describe the SPI NOR-compat part for ACPI - that's what >>>> I'm using. We could add properties there, but that seems improper. >>> >>> OK, so that's just reusing the DT binding in which case everything >>> that's valid for the DT binding should also be valid for ACPI - I >>> thought that actually worked automatically without you having to do >>> anything in the code but ICBW. >> >> I thought that it would be improper as we could be mixing ACPI methods to >> describe the serial bus (SPI Serial Bus Connection Resource Descriptor) and >> also DT properties which could conflict, like CS active high. >> >> However I do see extra properties than "compatible" being added in DSD for >> PRP0001: >> https://patchwork.ozlabs.org/patch/662813/ (see EEPROM part) > Hi Andy, > PRP method is only for vendors to *test* the hardware in ACPI environment. > The proper method is to allocate correct ACPI ID. Yes, that would seem the proper thing to do. So the SPI NOR driver is based on micron m25p80 and compatible string is "jedec,spi-nor", so I don't know who should or would do this registration. > > Properties (_DSD in ACPI) may be used in the same way as for DT if we have no > other means in ACPI specification for them. > >> And if we were to do this, I think that we would need to add some >> device_property_read_u32("spi-rx-bus-width", ...), etc calls in the SPI FW >> parsing for ACPI path - I couldn't see that. > > It's okay as long as you have ACPI ID. Well there is none AFAIK. > > P.S. Most of the sensor drivers were updated in order to support ACPI PRP > method due to DIY hobbyist on IoT sector and embedded devices. This should not > be an official way how we support hardware on ACPI-based platforms. Yeah, so we could do this. But, as I mentioned already, this could mean that we conflicting properties. For this the kernel driver prob should only pay attention to properties which ACPI cannot describe. Even better would be to update the ACPI spec, especially for something general like SPI bus width BTW, Do any of these sensors you mention have any ACPI standardization? Thanks, John
On Mon, Jan 13, 2020 at 10:09:27AM +0000, John Garry wrote: > On 10/01/2020 19:31, Andy Shevchenko wrote: > > PRP method is only for vendors to *test* the hardware in ACPI environment. > > The proper method is to allocate correct ACPI ID. > Yes, that would seem the proper thing to do. So the SPI NOR driver is based > on micron m25p80 and compatible string is "jedec,spi-nor", so I don't know > who should or would do this registration. The idiomatic approach appears to be for individual board vendors to allocate IDs, you do end up with multiple IDs from multiple vendors for the same thing. > BTW, Do any of these sensors you mention have any ACPI standardization? In general there's not really much standardizaiton for devices, the bindings that do exist aren't really centrally documented and the Windows standard is just to have the basic device registration in the firmware and do all properties based on quirking based on DMI information.
On 13/01/2020 11:42, Mark Brown wrote: > On Mon, Jan 13, 2020 at 10:09:27AM +0000, John Garry wrote: >> On 10/01/2020 19:31, Andy Shevchenko wrote: > >>> PRP method is only for vendors to *test* the hardware in ACPI environment. >>> The proper method is to allocate correct ACPI ID. > >> Yes, that would seem the proper thing to do. So the SPI NOR driver is based >> on micron m25p80 and compatible string is "jedec,spi-nor", so I don't know >> who should or would do this registration. > Hi Mark, > The idiomatic approach appears to be for individual board vendors > to allocate IDs, you do end up with multiple IDs from multiple > vendors for the same thing. So we see sort of approach a lot when vendors integrate 3rd party IP into a SoC and then assign some vendor specific ID for that. But I am not sure how appropriate that same approach would be for some 3rd party memory part which we're simply wiring up on our board. Maybe it is. > >> BTW, Do any of these sensors you mention have any ACPI standardization? > > In general there's not really much standardizaiton for devices, > the bindings that do exist aren't really centrally documented and > the Windows standard is just to have the basic device > registration in the firmware and do all properties based on > quirking based on DMI information. > OK, so there is always DMI. I hoped to avoid this sort of thing in the linux driver :) Cheers, John
On Mon, Jan 13, 2020 at 01:01:06PM +0000, John Garry wrote: > On 13/01/2020 11:42, Mark Brown wrote: > > The idiomatic approach appears to be for individual board vendors > > to allocate IDs, you do end up with multiple IDs from multiple > > vendors for the same thing. > But I am not sure how appropriate that same approach would be for some 3rd > party memory part which we're simply wiring up on our board. Maybe it is. It seems to be quite common for Intel reference designs to assign Intel IDs to non-Intel parts on the board (which is where I became aware of this practice). > > In general there's not really much standardizaiton for devices, > > the bindings that do exist aren't really centrally documented and > > the Windows standard is just to have the basic device > > registration in the firmware and do all properties based on > > quirking based on DMI information. > OK, so there is always DMI. I hoped to avoid this sort of thing in the linux > driver :) Yes, there are some merits to an approach like that.
On Mon, Jan 13, 2020 at 4:07 PM Mark Brown <broonie@kernel.org> wrote: > On Mon, Jan 13, 2020 at 01:01:06PM +0000, John Garry wrote: > > On 13/01/2020 11:42, Mark Brown wrote: > > > > The idiomatic approach appears to be for individual board vendors > > > to allocate IDs, you do end up with multiple IDs from multiple > > > vendors for the same thing. > > > But I am not sure how appropriate that same approach would be for some 3rd > > party memory part which we're simply wiring up on our board. Maybe it is. > > It seems to be quite common for Intel reference designs to assign > Intel IDs to non-Intel parts on the board (which is where I > became aware of this practice). Basically vendor of component in question is responsible for ID, but it seems they simple don't care.
On Mon, Jan 13, 2020 at 04:17:32PM +0200, Andy Shevchenko wrote: > On Mon, Jan 13, 2020 at 4:07 PM Mark Brown <broonie@kernel.org> wrote: > > On Mon, Jan 13, 2020 at 01:01:06PM +0000, John Garry wrote: > > > On 13/01/2020 11:42, Mark Brown wrote: > > > > The idiomatic approach appears to be for individual board vendors > > > > to allocate IDs, you do end up with multiple IDs from multiple > > > > vendors for the same thing. > > > But I am not sure how appropriate that same approach would be for some 3rd > > > party memory part which we're simply wiring up on our board. Maybe it is. > > It seems to be quite common for Intel reference designs to assign > > Intel IDs to non-Intel parts on the board (which is where I > > became aware of this practice). > Basically vendor of component in question is responsible for ID, but > it seems they simple don't care. AFAICT a lot of the time it seems to be that whoever is writing the software ends up assigning an ID, that may not be the silicon vendor.
On Mon, Jan 13, 2020 at 02:27:54PM +0000, Mark Brown wrote: > On Mon, Jan 13, 2020 at 04:17:32PM +0200, Andy Shevchenko wrote: > > On Mon, Jan 13, 2020 at 4:07 PM Mark Brown <broonie@kernel.org> wrote: > > > On Mon, Jan 13, 2020 at 01:01:06PM +0000, John Garry wrote: > > > > On 13/01/2020 11:42, Mark Brown wrote: > > > > > > The idiomatic approach appears to be for individual board vendors > > > > > to allocate IDs, you do end up with multiple IDs from multiple > > > > > vendors for the same thing. > > > > > But I am not sure how appropriate that same approach would be for some 3rd > > > > party memory part which we're simply wiring up on our board. Maybe it is. > > > > It seems to be quite common for Intel reference designs to assign > > > Intel IDs to non-Intel parts on the board (which is where I > > > became aware of this practice). > > > Basically vendor of component in question is responsible for ID, but > > it seems they simple don't care. > > AFAICT a lot of the time it seems to be that whoever is writing > the software ends up assigning an ID, that may not be the silicon > vendor. ...which is effectively abusing the ACPI ID allocation procedure. (And yes, Intel itself did it in the past — see badly created ACPI IDs in the drivers)
On 13/01/2020 14:34, Andy Shevchenko wrote: > On Mon, Jan 13, 2020 at 02:27:54PM +0000, Mark Brown wrote: >> On Mon, Jan 13, 2020 at 04:17:32PM +0200, Andy Shevchenko wrote: >>> On Mon, Jan 13, 2020 at 4:07 PM Mark Brown <broonie@kernel.org> wrote: >>>> On Mon, Jan 13, 2020 at 01:01:06PM +0000, John Garry wrote: >>>>> On 13/01/2020 11:42, Mark Brown wrote: >> >>>>>> The idiomatic approach appears to be for individual board vendors >>>>>> to allocate IDs, you do end up with multiple IDs from multiple >>>>>> vendors for the same thing. >> >>>>> But I am not sure how appropriate that same approach would be for some 3rd >>>>> party memory part which we're simply wiring up on our board. Maybe it is. >> >>>> It seems to be quite common for Intel reference designs to assign >>>> Intel IDs to non-Intel parts on the board (which is where I >>>> became aware of this practice). >> >>> Basically vendor of component in question is responsible for ID, but >>> it seems they simple don't care. >> >> AFAICT a lot of the time it seems to be that whoever is writing >> the software ends up assigning an ID, that may not be the silicon >> vendor. > > ...which is effectively abusing the ACPI ID allocation procedure. > > (And yes, Intel itself did it in the past — see badly created ACPI IDs > in the drivers) > Hi Mark, About this topic of ACPI having no method to describe device buswidth in the resource descriptor, it may be an idea for me to raise a Tianocore feature request @ https://bugzilla.tianocore.org/ There seems to be an avenue there for raising new features for the spec. I (or my org) can't participate in AWSG. I would have no concrete proposal for spec update for now, though. Hopefully others with more expertise could contribute. In the meantime, I have an RFC for using DMI to quirk support for this on the driver - I can share when ready. Thanks, John
On Fri, Jan 31, 2020 at 12:08 PM John Garry <john.garry@huawei.com> wrote: > > On 13/01/2020 14:34, Andy Shevchenko wrote: > > On Mon, Jan 13, 2020 at 02:27:54PM +0000, Mark Brown wrote: > >> On Mon, Jan 13, 2020 at 04:17:32PM +0200, Andy Shevchenko wrote: > >>> On Mon, Jan 13, 2020 at 4:07 PM Mark Brown <broonie@kernel.org> wrote: > >>>> On Mon, Jan 13, 2020 at 01:01:06PM +0000, John Garry wrote: > >>>>> On 13/01/2020 11:42, Mark Brown wrote: > >> > >>>>>> The idiomatic approach appears to be for individual board vendors > >>>>>> to allocate IDs, you do end up with multiple IDs from multiple > >>>>>> vendors for the same thing. > >> > >>>>> But I am not sure how appropriate that same approach would be for some 3rd > >>>>> party memory part which we're simply wiring up on our board. Maybe it is. > >> > >>>> It seems to be quite common for Intel reference designs to assign > >>>> Intel IDs to non-Intel parts on the board (which is where I > >>>> became aware of this practice). > >> > >>> Basically vendor of component in question is responsible for ID, but > >>> it seems they simple don't care. > >> > >> AFAICT a lot of the time it seems to be that whoever is writing > >> the software ends up assigning an ID, that may not be the silicon > >> vendor. > > > > ...which is effectively abusing the ACPI ID allocation procedure. > > > > (And yes, Intel itself did it in the past — see badly created ACPI IDs > > in the drivers) > > > > Hi Mark, > > About this topic of ACPI having no method to describe device buswidth in > the resource descriptor, it may be an idea for me to raise a Tianocore > feature request @ https://bugzilla.tianocore.org/ > The 19.6.126 describes the SPI resource, in particular: ---8<---8<--- DataBitLength is the size, in bits, of the smallest transfer unit for this connection. _LEN is automatically created to refer to this portion of the resource descriptor. ---8<---8<--- Is it what you are looking for? (As far as I know most of the firmwares simple abuse this field among others) > There seems to be an avenue there for raising new features for the spec. > I (or my org) can't participate in AWSG. But have you read 19.6.126? > I would have no concrete proposal for spec update for now, though. > Hopefully others with more expertise could contribute.
On 31/01/2020 11:39, Andy Shevchenko wrote: > On Fri, Jan 31, 2020 at 12:08 PM John Garry <john.garry@huawei.com> wrote: >> >> On 13/01/2020 14:34, Andy Shevchenko wrote: >>> On Mon, Jan 13, 2020 at 02:27:54PM +0000, Mark Brown wrote: >>>> On Mon, Jan 13, 2020 at 04:17:32PM +0200, Andy Shevchenko wrote: >>>>> On Mon, Jan 13, 2020 at 4:07 PM Mark Brown <broonie@kernel.org> wrote: >>>>>> On Mon, Jan 13, 2020 at 01:01:06PM +0000, John Garry wrote: >>>>>>> On 13/01/2020 11:42, Mark Brown wrote: >>>> >>>>>>>> The idiomatic approach appears to be for individual board vendors >>>>>>>> to allocate IDs, you do end up with multiple IDs from multiple >>>>>>>> vendors for the same thing. >>>> >>>>>>> But I am not sure how appropriate that same approach would be for some 3rd >>>>>>> party memory part which we're simply wiring up on our board. Maybe it is. >>>> >>>>>> It seems to be quite common for Intel reference designs to assign >>>>>> Intel IDs to non-Intel parts on the board (which is where I >>>>>> became aware of this practice). >>>> >>>>> Basically vendor of component in question is responsible for ID, but >>>>> it seems they simple don't care. >>>> >>>> AFAICT a lot of the time it seems to be that whoever is writing >>>> the software ends up assigning an ID, that may not be the silicon >>>> vendor. >>> >>> ...which is effectively abusing the ACPI ID allocation procedure. >>> >>> (And yes, Intel itself did it in the past — see badly created ACPI IDs >>> in the drivers) >>> >> >> Hi Mark, >> Hi Andy, >> About this topic of ACPI having no method to describe device buswidth in >> the resource descriptor, it may be an idea for me to raise a Tianocore >> feature request @ https://bugzilla.tianocore.org/ >> > > The 19.6.126 describes the SPI resource, in particular: > > ---8<---8<--- > DataBitLength is the size, in bits, of the smallest transfer unit for > this connection. _LEN is automatically > created to refer to this portion of the resource descriptor. > ---8<---8<--- > > Is it what you are looking for? (As far as I know most of the > firmwares simple abuse this field among others) I didn't think so - I thought that there was a distinction between width and length in SPI terms. So how do you find that most firmwares abuse this field? AFAICS, linux kernel doesn't interpret this field at all. > >> There seems to be an avenue there for raising new features for the spec. >> I (or my org) can't participate in AWSG. > > But have you read 19.6.126? > Maybe some clarification at least could be achieved :) Cheers, John
On Fri, Jan 31, 2020 at 2:03 PM John Garry <john.garry@huawei.com> wrote: > On 31/01/2020 11:39, Andy Shevchenko wrote: > > On Fri, Jan 31, 2020 at 12:08 PM John Garry <john.garry@huawei.com> wrote: > >> On 13/01/2020 14:34, Andy Shevchenko wrote: ... > >> About this topic of ACPI having no method to describe device buswidth in > >> the resource descriptor, it may be an idea for me to raise a Tianocore > >> feature request @ https://bugzilla.tianocore.org/ > >> > > > > The 19.6.126 describes the SPI resource, in particular: > > > > ---8<---8<--- > > DataBitLength is the size, in bits, of the smallest transfer unit for > > this connection. _LEN is automatically > > created to refer to this portion of the resource descriptor. > > ---8<---8<--- > > > > Is it what you are looking for? (As far as I know most of the > > firmwares simple abuse this field among others) > > I didn't think so - I thought that there was a distinction between width > and length in SPI terms. My interpretation of this field is a data width of the slave. Basically what we have as transfer->size inside SPI in the Linux kernel. > So how do you find that most firmwares abuse this field? AFAICS, linux > kernel doesn't interpret this field at all. From all tables I have this is the result of appearance (some of the tables are like 10x times present in my data base, but nevertheless) 140 SpiSerialBusV2(0x0000,PolarityHigh,FourWireMode,0x08, 411 SpiSerialBusV2(0x0000,PolarityLow,FourWireMode,0x08, 1 SpiSerialBusV2(0x0000,PolarityLow,FourWireMode,0x08, 36 SpiSerialBusV2(0x0000,PolarityLow,FourWireMode,0x10, 35 SpiSerialBusV2(0x0000,PolarityLow,FourWireMode,0x18, 35 SpiSerialBusV2(0x0000,PolarityLow,FourWireMode,0x20, 1 SpiSerialBusV2(0x0000,PolarityLow,ThreeWireMode,0x10, 8 SpiSerialBusV2(0x0001,PolarityLow,FourWireMode,0x08, 1 SpiSerialBusV2(0x0001,PolarityLow,FourWireMode,0x10, So, it seems I stand corrected, the field is in right use, although cases like 0x10 and 0x20 should be carefully checked. We may teach kernel to get something meaningful out of it.
On 31/01/2020 15:46, Andy Shevchenko wrote: > On Fri, Jan 31, 2020 at 2:03 PM John Garry <john.garry@huawei.com> wrote: >> On 31/01/2020 11:39, Andy Shevchenko wrote: >>> On Fri, Jan 31, 2020 at 12:08 PM John Garry <john.garry@huawei.com> wrote: >>>> On 13/01/2020 14:34, Andy Shevchenko wrote: > > ... > >>>> About this topic of ACPI having no method to describe device buswidth in >>>> the resource descriptor, it may be an idea for me to raise a Tianocore >>>> feature request @ https://bugzilla.tianocore.org/ >>>> >>> >>> The 19.6.126 describes the SPI resource, in particular: >>> >>> ---8<---8<--- >>> DataBitLength is the size, in bits, of the smallest transfer unit for >>> this connection. _LEN is automatically >>> created to refer to this portion of the resource descriptor. >>> ---8<---8<--- >>> Hi Andy, >>> Is it what you are looking for? (As far as I know most of the >>> firmwares simple abuse this field among others) >> >> I didn't think so - I thought that there was a distinction between width >> and length in SPI terms. > > My interpretation of this field is a data width of the slave. > Basically what we have as transfer->size inside SPI in the Linux > kernel. > >> So how do you find that most firmwares abuse this field? AFAICS, linux >> kernel doesn't interpret this field at all. > >>From all tables I have this is the result of appearance (some of the > tables are like 10x times present in my data base, but nevertheless) > > 140 SpiSerialBusV2(0x0000,PolarityHigh,FourWireMode,0x08, > 411 SpiSerialBusV2(0x0000,PolarityLow,FourWireMode,0x08, > 1 SpiSerialBusV2(0x0000,PolarityLow,FourWireMode,0x08, > 36 SpiSerialBusV2(0x0000,PolarityLow,FourWireMode,0x10, > 35 SpiSerialBusV2(0x0000,PolarityLow,FourWireMode,0x18, > 35 SpiSerialBusV2(0x0000,PolarityLow,FourWireMode,0x20, > 1 SpiSerialBusV2(0x0000,PolarityLow,ThreeWireMode,0x10, > 8 SpiSerialBusV2(0x0001,PolarityLow,FourWireMode,0x08, > 1 SpiSerialBusV2(0x0001,PolarityLow,FourWireMode,0x10, > > So, it seems I stand corrected, the field is in right use, although > cases like 0x10 and 0x20 should be carefully checked. > > We may teach kernel to get something meaningful out of it. > It seems that someone already had a go at that: https://lore.kernel.org/lkml/20170317212143.bogj6efzyvvf24yd@sirena.org.uk/ Thanks, John
On Fri, Jan 31, 2020 at 05:46:39PM +0200, Andy Shevchenko wrote: > On Fri, Jan 31, 2020 at 2:03 PM John Garry <john.garry@huawei.com> wrote: > > On 31/01/2020 11:39, Andy Shevchenko wrote: > > > DataBitLength is the size, in bits, of the smallest transfer unit for > > > this connection. _LEN is automatically > > > created to refer to this portion of the resource descriptor. > > > Is it what you are looking for? (As far as I know most of the > > > firmwares simple abuse this field among others) > > I didn't think so - I thought that there was a distinction between width > > and length in SPI terms. > My interpretation of this field is a data width of the slave. > Basically what we have as transfer->size inside SPI in the Linux > kernel. This discussion is about the number of data lines, SPI_TX_QUAD and friends. > 1 SpiSerialBusV2(0x0000,PolarityLow,FourWireMode,0x08, > 36 SpiSerialBusV2(0x0000,PolarityLow,FourWireMode,0x10, > 35 SpiSerialBusV2(0x0000,PolarityLow,FourWireMode,0x18, > 35 SpiSerialBusV2(0x0000,PolarityLow,FourWireMode,0x20, > 1 SpiSerialBusV2(0x0000,PolarityLow,ThreeWireMode,0x10, > 8 SpiSerialBusV2(0x0001,PolarityLow,FourWireMode,0x08, > 1 SpiSerialBusV2(0x0001,PolarityLow,FourWireMode,0x10, > So, it seems I stand corrected, the field is in right use, although > cases like 0x10 and 0x20 should be carefully checked. Those look like they're mainly controlling SPI_3WIRE so it does look like a reasonable fit, yes.
On Fri, Jan 31, 2020 at 04:26:46PM +0000, John Garry wrote: > On 31/01/2020 15:46, Andy Shevchenko wrote: > > So, it seems I stand corrected, the field is in right use, although > > cases like 0x10 and 0x20 should be carefully checked. > > We may teach kernel to get something meaningful out of it. > It seems that someone already had a go at that: > https://lore.kernel.org/lkml/20170317212143.bogj6efzyvvf24yd@sirena.org.uk/ This is a discussion about supporting DT bindings for bits-per-word which is a different thing again, that's the size of a data word which is not connected with the physical wiring. Please include human readable descriptions of things like commits and issues being discussed in e-mail in your mails, this makes them much easier for humans to read especially when they have no internet access. I do frequently catch up on my mail on flights or while otherwise travelling so this is even more pressing for me than just being about making things a bit easier to read.
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index 870f7797b56b..d6ed0c355954 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -281,6 +281,15 @@ config SPI_FSL_QUADSPI This controller does not support generic SPI messages. It only supports the high-level SPI memory interface. +config SPI_HISI_SFC_V3XX + tristate "HiSilicon SPI-NOR Flash Controller for Hi16XX chipsets" + depends on (ARM64 && ACPI) || COMPILE_TEST + depends on HAS_IOMEM + select CONFIG_MTD_SPI_NOR + help + This enables support for HiSilicon v3xx SPI-NOR flash controller + found in hi16xx chipsets. + config SPI_NXP_FLEXSPI tristate "NXP Flex SPI controller" depends on ARCH_LAYERSCAPE || HAS_IOMEM diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index bb49c9e6d0a0..9b65ec5afc5e 100644 --- a/drivers/spi/Makefile +++ b/drivers/spi/Makefile @@ -48,6 +48,7 @@ obj-$(CONFIG_SPI_FSL_LPSPI) += spi-fsl-lpspi.o obj-$(CONFIG_SPI_FSL_QUADSPI) += spi-fsl-qspi.o obj-$(CONFIG_SPI_FSL_SPI) += spi-fsl-spi.o obj-$(CONFIG_SPI_GPIO) += spi-gpio.o +obj-$(CONFIG_SPI_HISI_SFC_V3XX) += spi-hisi-sfc-v3xx.o obj-$(CONFIG_SPI_IMG_SPFI) += spi-img-spfi.o obj-$(CONFIG_SPI_IMX) += spi-imx.o obj-$(CONFIG_SPI_LANTIQ_SSC) += spi-lantiq-ssc.o diff --git a/drivers/spi/spi-hisi-sfc-v3xx.c b/drivers/spi/spi-hisi-sfc-v3xx.c new file mode 100644 index 000000000000..4cf8fc80a7b7 --- /dev/null +++ b/drivers/spi/spi-hisi-sfc-v3xx.c @@ -0,0 +1,284 @@ +// SPDX-License-Identifier: GPL-2.0-only +// +// HiSilicon SPI NOR V3XX Flash Controller Driver for hi16xx chipsets +// +// Copyright (c) 2019 HiSilicon Technologies Co., Ltd. +// Author: John Garry <john.garry@huawei.com> + +#include <linux/acpi.h> +#include <linux/bitops.h> +#include <linux/iopoll.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/spi/spi.h> +#include <linux/spi/spi-mem.h> + +#define HISI_SFC_V3XX_VERSION (0x1f8) + +#define HISI_SFC_V3XX_CMD_CFG (0x300) +#define HISI_SFC_V3XX_CMD_CFG_DATA_CNT_OFF 9 +#define HISI_SFC_V3XX_CMD_CFG_RW_MSK BIT(8) +#define HISI_SFC_V3XX_CMD_CFG_DATA_EN_MSK BIT(7) +#define HISI_SFC_V3XX_CMD_CFG_DUMMY_CNT_OFF 4 +#define HISI_SFC_V3XX_CMD_CFG_ADDR_EN_MSK BIT(3) +#define HISI_SFC_V3XX_CMD_CFG_CS_SEL_OFF 1 +#define HISI_SFC_V3XX_CMD_CFG_START_MSK BIT(0) +#define HISI_SFC_V3XX_CMD_INS (0x308) +#define HISI_SFC_V3XX_CMD_ADDR (0x30c) +#define HISI_SFC_V3XX_CMD_DATABUF0 (0x400) + +struct hisi_sfc_v3xx_host { + struct device *dev; + void __iomem *regbase; + int max_cmd_dword; +}; + +#define HISI_SFC_V3XX_WAIT_TIMEOUT_US 1000000 +#define HISI_SFC_V3XX_WAIT_POLL_INTERVAL_US 10 + +static int hisi_sfc_v3xx_wait_cmd_idle(struct hisi_sfc_v3xx_host *host) +{ + u32 reg; + + return readl_poll_timeout(host->regbase + HISI_SFC_V3XX_CMD_CFG, reg, + !(reg & HISI_SFC_V3XX_CMD_CFG_START_MSK), + HISI_SFC_V3XX_WAIT_POLL_INTERVAL_US, + HISI_SFC_V3XX_WAIT_TIMEOUT_US); +} + +static int hisi_sfc_v3xx_adjust_op_size(struct spi_mem *mem, + struct spi_mem_op *op) +{ + struct spi_device *spi = mem->spi; + struct hisi_sfc_v3xx_host *host; + uintptr_t addr = (uintptr_t)op->data.buf.in; + int max_byte_count; + + host = spi_controller_get_devdata(spi->master); + + max_byte_count = host->max_cmd_dword * 4; + + if (!IS_ALIGNED(addr, 4) && op->data.nbytes >= 4) + op->data.nbytes = 4 - (addr % 4); + else if (op->data.nbytes > max_byte_count) + op->data.nbytes = max_byte_count; + + return 0; +} + +/* + * memcpy_{to,from}io doesn't gurantee 32b accesses - which we require for the + * DATABUF registers -so use __io{read,write}32_copy when possible. For + * trailing bytes, copy them byte-by-byte from the DATABUF register, as we + * can't clobber outside the source/dest buffer. + * + * For efficient data read/write, we try to put any start 32b unaligned data + * into a separate transaction in hisi_sfc_v3xx_adjust_op_size(). + */ +static void hisi_sfc_v3xx_read_databuf(struct hisi_sfc_v3xx_host *host, + u8 *to, unsigned int len) +{ + void __iomem *from; + int i; + + from = host->regbase + HISI_SFC_V3XX_CMD_DATABUF0; + + if (IS_ALIGNED((uintptr_t)to, 4)) { + int words = len / 4; + + __ioread32_copy(to, from, words); + + len -= words * 4; + if (len) { + u32 val; + + to += words * 4; + from += words * 4; + + val = __raw_readl(from); + + for (i = 0; i < len; i++, val >>= 8, to++) + *to = (u8)val; + } + } else { + for (i = 0; i < DIV_ROUND_UP(len, 4); i++, from += 4) { + u32 val = __raw_readl(from); + int j; + + for (j = 0; j < 4 && (j + (i * 4) < len); + to++, val >>= 8, j++) + *to = (u8)val; + } + } +} + +static void hisi_sfc_v3xx_write_databuf(struct hisi_sfc_v3xx_host *host, + const u8 *from, unsigned int len) +{ + void __iomem *to; + int i; + + to = host->regbase + HISI_SFC_V3XX_CMD_DATABUF0; + + if (IS_ALIGNED((uintptr_t)from, 4)) { + int words = len / 4; + + __iowrite32_copy(to, from, words); + + len -= words * 4; + if (len) { + u32 val = 0; + + to += words * 4; + from += words * 4; + + for (i = 0; i < len; i++, from++) + val |= *from << i * 8; + __raw_writel(val, to); + } + + } else { + for (i = 0; i < DIV_ROUND_UP(len, 4); i++, to += 4) { + u32 val = 0; + int j; + + for (j = 0; j < 4 && (j + (i * 4) < len); + from++, j++) + val |= *from << j * 8; + __raw_writel(val, to); + } + } +} + +static int hisi_sfc_v3xx_generic_exec_op(struct hisi_sfc_v3xx_host *host, + const struct spi_mem_op *op, + u8 chip_select) +{ + int ret, len = op->data.nbytes; + u32 config = 0; + + if (op->addr.nbytes) + config |= HISI_SFC_V3XX_CMD_CFG_ADDR_EN_MSK; + + if (op->data.dir != SPI_MEM_NO_DATA) { + config |= (len - 1) << HISI_SFC_V3XX_CMD_CFG_DATA_CNT_OFF; + config |= HISI_SFC_V3XX_CMD_CFG_DATA_EN_MSK; + } + + if (op->data.dir == SPI_MEM_DATA_OUT) + hisi_sfc_v3xx_write_databuf(host, op->data.buf.out, len); + else if (op->data.dir == SPI_MEM_DATA_IN) + config |= HISI_SFC_V3XX_CMD_CFG_RW_MSK; + + config |= op->dummy.nbytes << HISI_SFC_V3XX_CMD_CFG_DUMMY_CNT_OFF | + chip_select << HISI_SFC_V3XX_CMD_CFG_CS_SEL_OFF | + HISI_SFC_V3XX_CMD_CFG_START_MSK; + + writel(op->addr.val, host->regbase + HISI_SFC_V3XX_CMD_ADDR); + writel(op->cmd.opcode, host->regbase + HISI_SFC_V3XX_CMD_INS); + + writel(config, host->regbase + HISI_SFC_V3XX_CMD_CFG); + + ret = hisi_sfc_v3xx_wait_cmd_idle(host); + if (ret) + return ret; + + if (op->data.dir == SPI_MEM_DATA_IN) + hisi_sfc_v3xx_read_databuf(host, op->data.buf.in, len); + + return 0; +} + +static int hisi_sfc_v3xx_exec_op(struct spi_mem *mem, + const struct spi_mem_op *op) +{ + struct hisi_sfc_v3xx_host *host; + struct spi_device *spi = mem->spi; + u8 chip_select = spi->chip_select; + + host = spi_controller_get_devdata(spi->master); + + return hisi_sfc_v3xx_generic_exec_op(host, op, chip_select); +} + +static const struct spi_controller_mem_ops hisi_sfc_v3xx_mem_ops = { + .adjust_op_size = hisi_sfc_v3xx_adjust_op_size, + .exec_op = hisi_sfc_v3xx_exec_op, +}; + +static int hisi_sfc_v3xx_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct hisi_sfc_v3xx_host *host; + struct spi_controller *ctlr; + u32 version; + int ret; + + ctlr = spi_alloc_master(&pdev->dev, sizeof(*host)); + if (!ctlr) + return -ENOMEM; + + ctlr->mode_bits = SPI_RX_DUAL | SPI_RX_QUAD | + SPI_TX_DUAL | SPI_TX_QUAD; + + host = spi_controller_get_devdata(ctlr); + host->dev = dev; + + platform_set_drvdata(pdev, host); + + host->regbase = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(host->regbase)) { + ret = PTR_ERR(host->regbase); + goto err_put_master; + } + + ctlr->bus_num = -1; + ctlr->num_chipselect = 1; + ctlr->mem_ops = &hisi_sfc_v3xx_mem_ops; + + version = readl(host->regbase + HISI_SFC_V3XX_VERSION); + + switch (version) { + case 0x351: + host->max_cmd_dword = 64; + break; + default: + host->max_cmd_dword = 16; + break; + } + + ret = devm_spi_register_controller(dev, ctlr); + if (ret) + goto err_put_master; + + dev_info(&pdev->dev, "hw version 0x%x\n", version); + + return 0; + +err_put_master: + spi_master_put(ctlr); + return ret; +} + +#if IS_ENABLED(CONFIG_ACPI) +static const struct acpi_device_id hisi_sfc_v3xx_acpi_ids[] = { + {"HISI0341", 0}, + {} +}; +MODULE_DEVICE_TABLE(acpi, hisi_sfc_v3xx_acpi_ids); +#endif + +static struct platform_driver hisi_sfc_v3xx_spi_driver = { + .driver = { + .name = "hisi-sfc-v3xx", + .acpi_match_table = ACPI_PTR(hisi_sfc_v3xx_acpi_ids), + }, + .probe = hisi_sfc_v3xx_probe, +}; + +module_platform_driver(hisi_sfc_v3xx_spi_driver); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("John Garry <john.garry@huawei.com>"); +MODULE_DESCRIPTION("HiSilicon SPI NOR V3XX Flash Controller Driver for hi16xx chipsets");
Add the driver for the HiSilicon v3xx SPI NOR flash controller, commonly found in hi16xx chipsets. This is a different controller than that in drivers/mtd/spi-nor/hisi-sfc.c; indeed, the naming for that driver is poor, since it is really known as FMC, and can support other memory technologies. The driver module name is "hisi-sfc-v3xx", as recommended by HW designer, being an attempt to provide a distinct name - v3xx being the unique controller versioning. Only ACPI firmware is supported. DMA is not supported, and we just use polling mode for operation completion notification. The driver uses the SPI MEM OPs. Signed-off-by: John Garry <john.garry@huawei.com> --- drivers/spi/Kconfig | 9 + drivers/spi/Makefile | 1 + drivers/spi/spi-hisi-sfc-v3xx.c | 284 ++++++++++++++++++++++++++++++++ 3 files changed, 294 insertions(+) create mode 100644 drivers/spi/spi-hisi-sfc-v3xx.c