| Message ID | 20231130083854.55221-6-jaimeliao.tw@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add octal DTR support for Macronix flash | expand |
Hi Jaime, > Some SPI-NOR flash swap the bytes on a 16-bit boundary when > configured in Octal DTR mode. It means data format D0 D1 D2 D3 > would be swapped to D1 D0 D3 D2. So that whether controller > support swapping bytes should be checked before enable Octal > DTR mode. Add swap byte support on a 16-bit boundary when > configured in Octal DTR mode for Macronix xSPI host controller > dirver. > > Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw> > --- > drivers/spi/spi-mxic.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c > index 60c9f3048ac9..085c9037d6f5 100644 > --- a/drivers/spi/spi-mxic.c > +++ b/drivers/spi/spi-mxic.c > @@ -572,6 +572,7 @@ static const struct spi_controller_mem_ops > mxic_spi_mem_ops = { > > static const struct spi_controller_mem_caps mxic_spi_mem_caps = { > .dtr = true, > + .dtr_swab16 = true, > .ecc = true, > }; I'm confused. How can this swap the bytes depending on the flashes requirements? I.e. the controller should look at the spi-mem operation and either swap the bytes or it should leave them as is. -michael
Hi Michael > > Hi Jaime, > > > Some SPI-NOR flash swap the bytes on a 16-bit boundary when > > configured in Octal DTR mode. It means data format D0 D1 D2 D3 > > would be swapped to D1 D0 D3 D2. So that whether controller > > support swapping bytes should be checked before enable Octal > > DTR mode. Add swap byte support on a 16-bit boundary when > > configured in Octal DTR mode for Macronix xSPI host controller > > dirver. > > > > Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw> > > --- > > drivers/spi/spi-mxic.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c > > index 60c9f3048ac9..085c9037d6f5 100644 > > --- a/drivers/spi/spi-mxic.c > > +++ b/drivers/spi/spi-mxic.c > > @@ -572,6 +572,7 @@ static const struct spi_controller_mem_ops > > mxic_spi_mem_ops = { > > > > static const struct spi_controller_mem_caps mxic_spi_mem_caps = { > > .dtr = true, > > + .dtr_swab16 = true, > > .ecc = true, > > }; > > I'm confused. How can this swap the bytes depending on the flashes > requirements? I.e. the controller should look at the spi-mem operation > and either swap the bytes or it should leave them as is. Byte order in 8D-8D-8D mode could get by parsing BFPT 18th Dword 31th bit. I think flash driver should notify controller driver and verify whether the controller driver support byte swap functionality. If flash requires byte swap in octal DTR mode but the controller driver doesn't support it, then octal DTR should not be enabled. The controller driver should indicate in the description of "spi_controller_mem_caps" whether this feature is supported. The implementation of this feature should be handled separately by each controller drive since the design may vary for each one. > > -michael > Thanks Jaime
Hi Jaime, >> > Some SPI-NOR flash swap the bytes on a 16-bit boundary when >> > configured in Octal DTR mode. It means data format D0 D1 D2 D3 >> > would be swapped to D1 D0 D3 D2. So that whether controller >> > support swapping bytes should be checked before enable Octal >> > DTR mode. Add swap byte support on a 16-bit boundary when >> > configured in Octal DTR mode for Macronix xSPI host controller >> > dirver. >> > >> > Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw> >> > --- >> > drivers/spi/spi-mxic.c | 1 + >> > 1 file changed, 1 insertion(+) >> > >> > diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c >> > index 60c9f3048ac9..085c9037d6f5 100644 >> > --- a/drivers/spi/spi-mxic.c >> > +++ b/drivers/spi/spi-mxic.c >> > @@ -572,6 +572,7 @@ static const struct spi_controller_mem_ops >> > mxic_spi_mem_ops = { >> > >> > static const struct spi_controller_mem_caps mxic_spi_mem_caps = { >> > .dtr = true, >> > + .dtr_swab16 = true, >> > .ecc = true, >> > }; >> >> I'm confused. How can this swap the bytes depending on the flashes >> requirements? I.e. the controller should look at the spi-mem operation >> and either swap the bytes or it should leave them as is. > Byte order in 8D-8D-8D mode could get by parsing BFPT 18th Dword 31th > bit. > I think flash driver should notify controller driver and verify whether > the > controller driver support byte swap functionality. If flash requires > byte swap > in octal DTR mode but the controller driver doesn't support it, then > octal DTR > should not be enabled. Correct. But the above means, the controller *supports* swapping the bytes if the flash requires it. But where is the code in the spi-mxic driver which actually controls the swapping depending on the dtr_swab16 property in spi_mem_op? > The controller driver should indicate in the > description of > "spi_controller_mem_caps" whether this feature is supported. The > implementation > of this feature should be handled separately by each controller drive > since the design > may vary for each one. Correct. And because your flashes apparently need that swapping, the driver will have to have support for that. -michael
Hi Michael > > Hi Jaime, > > >> > Some SPI-NOR flash swap the bytes on a 16-bit boundary when > >> > configured in Octal DTR mode. It means data format D0 D1 D2 D3 > >> > would be swapped to D1 D0 D3 D2. So that whether controller > >> > support swapping bytes should be checked before enable Octal > >> > DTR mode. Add swap byte support on a 16-bit boundary when > >> > configured in Octal DTR mode for Macronix xSPI host controller > >> > dirver. > >> > > >> > Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw> > >> > --- > >> > drivers/spi/spi-mxic.c | 1 + > >> > 1 file changed, 1 insertion(+) > >> > > >> > diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c > >> > index 60c9f3048ac9..085c9037d6f5 100644 > >> > --- a/drivers/spi/spi-mxic.c > >> > +++ b/drivers/spi/spi-mxic.c > >> > @@ -572,6 +572,7 @@ static const struct spi_controller_mem_ops > >> > mxic_spi_mem_ops = { > >> > > >> > static const struct spi_controller_mem_caps mxic_spi_mem_caps = { > >> > .dtr = true, > >> > + .dtr_swab16 = true, > >> > .ecc = true, > >> > }; > >> > >> I'm confused. How can this swap the bytes depending on the flashes > >> requirements? I.e. the controller should look at the spi-mem operation > >> and either swap the bytes or it should leave them as is. > > Byte order in 8D-8D-8D mode could get by parsing BFPT 18th Dword 31th > > bit. > > I think flash driver should notify controller driver and verify whether > > the > > controller driver support byte swap functionality. If flash requires > > byte swap > > in octal DTR mode but the controller driver doesn't support it, then > > octal DTR > > should not be enabled. > > Correct. But the above means, the controller *supports* swapping the > bytes > if the flash requires it. But where is the code in the spi-mxic driver > which > actually controls the swapping depending on the dtr_swab16 property in > spi_mem_op? I add it additionally because of controller driver should enable swap byte feature on special case only. I programed data in 1s-1s-1s mode and read back in 8d-8d-8d mode with controller swapped byte. Then check program data(in 1s-1s-1s) and read data(in 8d-8d-8d). In normal case, 8d-8d-8d prgram then 8d-8d-8d read, controller driver should disable swapping byte feature. So that I don't think controller driver swapping byte feature should depends on dtr_swab16. > > > The controller driver should indicate in the > > description of > > "spi_controller_mem_caps" whether this feature is supported. The > > implementation > > of this feature should be handled separately by each controller drive > > since the design > > may vary for each one. > > Correct. And because your flashes apparently need that swapping, the > driver > will have to have support for that. > > -michael Thanks Jaime
Hi, Jaime, Michael, Allow me to try to intermediate this :). On 12/4/23 06:44, liao jaime wrote: > Hi Michael > > >> >> Hi Jaime, >> >>>>> Some SPI-NOR flash swap the bytes on a 16-bit boundary when >>>>> configured in Octal DTR mode. It means data format D0 D1 D2 D3 >>>>> would be swapped to D1 D0 D3 D2. So that whether controller >>>>> support swapping bytes should be checked before enable Octal >>>>> DTR mode. Add swap byte support on a 16-bit boundary when >>>>> configured in Octal DTR mode for Macronix xSPI host controller >>>>> dirver. >>>>> >>>>> Signed-off-by: JaimeLiao <jaimeliao@mxic.com.tw> >>>>> --- >>>>> drivers/spi/spi-mxic.c | 1 + >>>>> 1 file changed, 1 insertion(+) >>>>> >>>>> diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c >>>>> index 60c9f3048ac9..085c9037d6f5 100644 >>>>> --- a/drivers/spi/spi-mxic.c >>>>> +++ b/drivers/spi/spi-mxic.c >>>>> @@ -572,6 +572,7 @@ static const struct spi_controller_mem_ops >>>>> mxic_spi_mem_ops = { >>>>> >>>>> static const struct spi_controller_mem_caps mxic_spi_mem_caps = { >>>>> .dtr = true, >>>>> + .dtr_swab16 = true, >>>>> .ecc = true, >>>>> }; >>>> >>>> I'm confused. How can this swap the bytes depending on the flashes >>>> requirements? I.e. the controller should look at the spi-mem operation >>>> and either swap the bytes or it should leave them as is. >>> Byte order in 8D-8D-8D mode could get by parsing BFPT 18th Dword 31th >>> bit. >>> I think flash driver should notify controller driver and verify whether >>> the >>> controller driver support byte swap functionality. If flash requires >>> byte swap >>> in octal DTR mode but the controller driver doesn't support it, then >>> octal DTR >>> should not be enabled. >> >> Correct. But the above means, the controller *supports* swapping the >> bytes >> if the flash requires it. But where is the code in the spi-mxic driver >> which >> actually controls the swapping depending on the dtr_swab16 property in >> spi_mem_op? > I add it additionally because of controller driver should enable swap byte > feature on special case only. > I programed data in 1s-1s-1s mode and read back in 8d-8d-8d mode > with controller swapped byte. > Then check program data(in 1s-1s-1s) and read data(in 8d-8d-8d). > In normal case, 8d-8d-8d prgram then 8d-8d-8d read, controller driver > should disable swapping byte feature. > So that I don't think controller driver swapping byte feature should depends > on dtr_swab16. > I assume Michael's concerns are that if we don't have a controller that actually supports and implements dtr_swab16, then we risk that all the SPI NOR related code to be removed on a further cleanup, as in kernel we don't add support that's not used. Then Michael rightly asks about the details of this patch. Why adding a dtr_swab16 mem caps in this driver if it's not used? We expect a register write, specifying the controller to swap bytes. You missed that. Can this controller swap the bytes? If yes, what needs to be configured in order to swap the bytes? Michael, I know for sure that mchp's sama7g5 xSPI controller can swap the bytes back on the fly. What I did was to check the dtr_swab16 bool at runtime in the exec_op() method and if it was true, I had to write a configuration bit. I can't remember why I haven't posted a v2 on that series, maybe I'll do it if I find some time or I ever get bored. I have a board. Anyway, knowing that there's a board out there, sama7g5ek, that contains a macronix flash that swaps bytes and also have a controller that can swap them back, would it be acceptable to queue just the SPI NOR patches for now? Thanks, ta
Hi, > I know for sure that mchp's sama7g5 xSPI controller can swap the bytes > back on the fly. What I did was to check the dtr_swab16 bool at runtime > in the exec_op() method and if it was true, I had to write a > configuration bit. I can't remember why I haven't posted a v2 on that > series, maybe I'll do it if I find some time or I ever get bored. I > have > a board. Anyway, knowing that there's a board out there, sama7g5ek, > that > contains a macronix flash that swaps bytes and also have a controller > that can swap them back, would it be acceptable to queue just the SPI > NOR patches for now? I'm still leaning towards not putting any unused code into the kernel and (maybe) increase maintenance. Or code which might need to be adjusted later if it will actually be used. Without a user, we won't see the whole picture and - at least I - cannot judge whether that approach is correct then. I'd say, if you ever get board, take this patch again together with the user :) -michael
Hi Michael > > Hi, > > > I know for sure that mchp's sama7g5 xSPI controller can swap the bytes > > back on the fly. What I did was to check the dtr_swab16 bool at runtime > > in the exec_op() method and if it was true, I had to write a > > configuration bit. I can't remember why I haven't posted a v2 on that > > series, maybe I'll do it if I find some time or I ever get bored. I > > have > > a board. Anyway, knowing that there's a board out there, sama7g5ek, > > that > > contains a macronix flash that swaps bytes and also have a controller > > that can swap them back, would it be acceptable to queue just the SPI > > NOR patches for now? > > I'm still leaning towards not putting any unused code into the kernel > and (maybe) increase maintenance. Or code which might need to be > adjusted > later if it will actually be used. Without a user, we won't see the > whole picture and - at least I - cannot judge whether that approach is > correct then. > > I'd say, if you ever get board, take this patch again together with the > user :) Is it necessary to prepare v7 or spi-nor patches will be accept? > > -michael Thanks Jaime
diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c index 60c9f3048ac9..085c9037d6f5 100644 --- a/drivers/spi/spi-mxic.c +++ b/drivers/spi/spi-mxic.c @@ -572,6 +572,7 @@ static const struct spi_controller_mem_ops mxic_spi_mem_ops = { static const struct spi_controller_mem_caps mxic_spi_mem_caps = { .dtr = true, + .dtr_swab16 = true, .ecc = true, };